Tighten Windows named-pipe handling per review

Address aponcedeleonch's review on PR #5201:

- Update --socket help to mention named pipes and regen CLI docs.
- Promote namedPipePrefix to discovery.NamedPipePrefix as the
  canonical definition; pkg/api re-aliases it locally so listener
  and dialer cannot drift.
- Make isNamedPipeAddress case-insensitive via strings.EqualFold;
  the Windows pipe namespace is case-insensitive at the kernel
  layer, so \\.\Pipe\foo must not silently fall through to AF_UNIX.
- Collapse stat-then-remove into a single os.Remove that tolerates
  fs.ErrNotExist on both POSIX and the Windows AF_UNIX fallback.
- Close the listener and remove the socket file when Chmod fails,
  rather than leaking a bound AF_UNIX listener.
- Replace stdruntime.GOOS with a per-platform supportsNamedPipe()
  helper, dropping the runtime-package alias and its collision
  with pkg/container/runtime.
- Rename socket_other.{go,_test.go} to socket_unix.{go,_test.go}
  to match the client_unix/client_windows convention used by
  pkg/container/docker/sdk.
- Add TestCreateListener_NamedPipe_Unsupported to round out the
  listener-side reject coverage on non-Windows builds.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: samuv

cmd/thv/app/server.go

@@ -192,8 +192,9 @@ func init() {
 	serveCmd.Flags().IntVar(&port, "port", 8080, "Port to bind the server to")
 	serveCmd.Flags().BoolVar(&enableDocs, "openapi", false,
 		"Enable OpenAPI documentation endpoints (/api/openapi.json and /api/doc)")
-	serveCmd.Flags().StringVar(&socketPath, "socket", "", "UNIX socket path to bind the "+
-		"server to (overrides host and port if provided)")
+	serveCmd.Flags().StringVar(&socketPath, "socket", "",
+		`UNIX socket path or, on Windows, a named pipe (\\.\pipe\<name>) to bind the `+
+			"server to (overrides host and port if provided)")
 
 	// Add experimental MCP server flags
 	serveCmd.Flags().BoolVar(&enableMCPServer, "experimental-mcp", false,

docs/cli/thv_serve.md

@@ -27,7 +27,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	stdruntime "runtime"
 	"strings"
 	"time"
 
@@ -367,16 +366,21 @@ func (b *ServerBuilder) setupDefaultRoutes(r *chi.Mux) {
 	}
 }
 
-// namedPipePrefix is the Windows named-pipe namespace prefix. Both per-platform
-// socket files use it, and ListenURL/createListener inspect the address prefix
-// to choose between AF_UNIX and named-pipe semantics.
-const namedPipePrefix = `\\.\pipe\`
+// namedPipePrefix is the Windows named-pipe namespace prefix. The canonical
+// definition lives in pkg/server/discovery so the listener and dialer cannot
+// drift; pkg/api re-aliases it here so per-platform socket files do not need
+// to import discovery directly.
+const namedPipePrefix = discovery.NamedPipePrefix
 
 // isNamedPipeAddress reports whether address is a Windows named-pipe path.
 // The check is platform-agnostic so callers on non-Windows can fail fast with
-// a clear error before reaching the listener code.
+// a clear error before reaching the listener code. The comparison is
+// case-insensitive because the Windows pipe namespace is case-insensitive at
+// the kernel layer; without EqualFold an address like \\.\Pipe\foo would
+// silently fall through to AF_UNIX and then fail to bind.
 func isNamedPipeAddress(address string) bool {
-	return strings.HasPrefix(address, namedPipePrefix)
+	return len(address) >= len(namedPipePrefix) &&
+		strings.EqualFold(address[:len(namedPipePrefix)], namedPipePrefix)
 }
 
 func setupTCPListener(address string) (net.Listener, error) {
@@ -709,7 +713,7 @@ func createListener(address string, isUnixSocket bool) (net.Listener, string, er
 
 	addrType := "UNIX socket"
 	if isNamedPipeAddress(address) {
-		if stdruntime.GOOS != "windows" {
+		if !supportsNamedPipe() {
 			return nil, "", fmt.Errorf("named pipe addresses are only supported on Windows: %s", address)
 		}
 		addrType = "Windows named pipe"

pkg/api/server.go

@@ -6,21 +6,26 @@
 package api
 
 import (
+	"errors"
 	"fmt"
+	"io/fs"
 	"log/slog"
 	"net"
 	"os"
 	"path/filepath"
 )
 
+// supportsNamedPipe reports whether the current build target can host a
+// Windows named-pipe listener. Used by createListener to reject pipe addresses
+// before reaching the per-platform setupUnixSocket implementation.
+func supportsNamedPipe() bool { return false }
+
 // setupUnixSocket creates a UNIX domain socket listener at the given path.
 // On non-Windows platforms named-pipe addresses are not supported; callers
 // guard against that in createListener.
 func setupUnixSocket(address string) (net.Listener, error) {
-	if _, err := os.Stat(address); err == nil {
-		if err := os.Remove(address); err != nil {
-			return nil, fmt.Errorf("failed to remove existing socket: %w", err)
-		}
+	if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return nil, fmt.Errorf("failed to remove existing socket: %w", err)
 	}
 
 	if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil {
@@ -33,6 +38,11 @@ func setupUnixSocket(address string) (net.Listener, error) {
 	}
 
 	if err := os.Chmod(address, socketPermissions); err != nil {
+		// Roll back the bound listener and the socket file rather than leaking
+		// either: the listener owns the AF_UNIX file and net.Listen will not
+		// rebind the same path while the file exists.
+		_ = listener.Close()
+		_ = os.Remove(address)
 		return nil, fmt.Errorf("failed to set socket permissions: %w", err)
 	}
 
@@ -42,7 +52,7 @@ func setupUnixSocket(address string) (net.Listener, error) {
 // cleanupUnixSocket removes the socket file at address. Missing files are not
 // an error since cleanup may run after a partial startup.
 func cleanupUnixSocket(address string) {
-	if err := os.Remove(address); err != nil && !os.IsNotExist(err) {
+	if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		slog.Warn("failed to remove socket file", "error", err)
 	}
 }

pkg/api/socket_other.go pkg/api/socket_unix.gopkg/api/socket_other.go renamed to pkg/api/socket_unix.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSocketURL_Unix(t *testing.T) {
@@ -25,6 +26,7 @@ func TestIsNamedPipeAddress(t *testing.T) {
 	}{
 		{"plain socket", "/tmp/thv.sock", false},
 		{"named pipe", `\\.\pipe\thv-api`, true},
+		{"named pipe mixed case", `\\.\Pipe\thv-api`, true},
 		{"empty", "", false},
 	}
 	for _, tt := range tests {
@@ -34,3 +36,13 @@ func TestIsNamedPipeAddress(t *testing.T) {
 		})
 	}
 }
+
+// TestCreateListener_NamedPipe_Unsupported asserts that createListener rejects
+// pipe addresses on non-Windows up front, mirroring the dialer-side guard
+// covered by TestCheckHealth_NamedPipe_Unsupported_OnNonWindows.
+func TestCreateListener_NamedPipe_Unsupported(t *testing.T) {
+	t.Parallel()
+	_, _, err := createListener(`\\.\pipe\thv-api`, true)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "only supported on Windows")
+}

pkg/api/socket_other_test.go pkg/api/socket_unix_test.gopkg/api/socket_other_test.go renamed to pkg/api/socket_unix_test.go

@@ -6,12 +6,13 @@
 package api
 
 import (
+	"errors"
 	"fmt"
+	"io/fs"
 	"log/slog"
 	"net"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/Microsoft/go-winio"
 )
@@ -21,6 +22,11 @@ import (
 // (Docker, containerd, Podman) and is well above any single HTTP header chunk.
 const namedPipeBufferSize = 64 * 1024
 
+// supportsNamedPipe reports whether the current build target can host a
+// Windows named-pipe listener. Used by createListener to choose between the
+// pipe and AF_UNIX paths without dragging the runtime package into server.go.
+func supportsNamedPipe() bool { return true }
+
 // setupUnixSocket creates either a Windows named-pipe listener (when address
 // has the \\.\pipe\ prefix) or an AF_UNIX listener at a filesystem path.
 //
@@ -33,7 +39,7 @@ const namedPipeBufferSize = 64 * 1024
 // AF_UNIX is supported on Windows 10 1803+. The chmod step is dropped on this
 // path because POSIX file modes do not apply on Windows.
 func setupUnixSocket(address string) (net.Listener, error) {
-	if strings.HasPrefix(address, namedPipePrefix) {
+	if isNamedPipeAddress(address) {
 		// MessageMode is left at false (byte stream) explicitly because HTTP
 		// requires byte-oriented framing.
 		listener, err := winio.ListenPipe(address, &winio.PipeConfig{
@@ -47,10 +53,8 @@ func setupUnixSocket(address string) (net.Listener, error) {
 		return listener, nil
 	}
 
-	if _, err := os.Stat(address); err == nil {
-		if err := os.Remove(address); err != nil {
-			return nil, fmt.Errorf("failed to remove existing socket: %w", err)
-		}
+	if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return nil, fmt.Errorf("failed to remove existing socket: %w", err)
 	}
 
 	if err := os.MkdirAll(filepath.Dir(address), 0750); err != nil {
@@ -68,10 +72,10 @@ func setupUnixSocket(address string) (net.Listener, error) {
 // cleanupUnixSocket removes the AF_UNIX socket file at address, or no-ops for
 // named pipes (the pipe is destroyed when the listener closes).
 func cleanupUnixSocket(address string) {
-	if strings.HasPrefix(address, namedPipePrefix) {
+	if isNamedPipeAddress(address) {
 		return
 	}
-	if err := os.Remove(address); err != nil && !os.IsNotExist(err) {
+	if err := os.Remove(address); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		slog.Warn("failed to remove socket file", "error", err)
 	}
 }
@@ -80,8 +84,8 @@ func cleanupUnixSocket(address string) {
 // the discovery file. Named pipes are emitted as npipe://<name> where <name>
 // is everything after the \\.\pipe\ prefix.
 func socketURL(address string) string {
-	if strings.HasPrefix(address, namedPipePrefix) {
-		return "npipe://" + strings.TrimPrefix(address, namedPipePrefix)
+	if isNamedPipeAddress(address) {
+		return "npipe://" + address[len(namedPipePrefix):]
 	}
 	return "unix://" + address
 }

pkg/api/socket_windows.go

@@ -23,9 +23,11 @@ const (
 	NonceHeader = "X-Toolhive-Nonce"
 )
 
-// namedPipePrefix is the Windows named-pipe namespace prefix used to
-// reconstruct addresses for winio.DialPipeContext.
-const namedPipePrefix = `\\.\pipe\`
+// NamedPipePrefix is the Windows named-pipe namespace prefix. The discovery
+// dialer uses it to reconstruct addresses for winio.DialPipeContext, and the
+// API listener (pkg/api) imports it as the canonical definition so the two
+// sides cannot drift.
+const NamedPipePrefix = `\\.\pipe\`
 
 // CheckHealth verifies that a server at the given URL is healthy and optionally
 // matches the expected nonce. It supports http://, unix://, and npipe:// URL
@@ -184,5 +186,5 @@ func ParseNamedPipeURL(rawURL string) (string, error) {
 	if strings.Contains(name, "..") {
 		return "", fmt.Errorf("named pipe name must not contain '..': %s", name)
 	}
-	return namedPipePrefix + name, nil
+	return NamedPipePrefix + name, nil
 }