diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index bb525a711c..ec06868cec 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -380,6 +380,15 @@ jobs: - name: Test windows-sensitive packages run: go test -race -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... + # Keep port-collision coverage targeted: the instance package contains + # broader stdio MCP fixture tests that depend on Unix command behavior. + - name: Test Windows port collision handling + shell: bash + run: | + go test -race -v ./internal/common/netutil -run '^TestIsAddrInUse' + go test -race -v ./internal/agentctl/server/instance -run '^TestAllocatePortAndListenerRetriesAddressInUse$' + go test -race -v ./internal/gateway/websocket -run '^TestResolveAndBindReportsOccupiedPort$' + # Keep this targeted: the service package has broad subprocess coverage, # while this native test is the Windows path-semantics regression. - name: Test Windows path semantics diff --git a/apps/backend/Makefile b/apps/backend/Makefile index e2fbdfa029..919dc67c9a 100644 --- a/apps/backend/Makefile +++ b/apps/backend/Makefile @@ -287,6 +287,10 @@ test: test-windows: @echo "Running Windows-sensitive packages..." $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... + @echo "Running Windows port-collision tests..." + $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/common/netutil -run TestIsAddrInUse + $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/agentctl/server/instance -run TestAllocatePortAndListenerRetriesAddressInUse + $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/gateway/websocket -run TestResolveAndBindReportsOccupiedPort ## Verify run/dev/start-debug expand to the right per-shell command line across ## native Windows (cmd), Git Bash/MSYS, and Unix — the Make-target dispatch the diff --git a/apps/backend/internal/agent/runtime/lifecycle/testing_support.go b/apps/backend/internal/agent/runtime/lifecycle/testing_support.go index e82fabc583..366de2d814 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/testing_support.go +++ b/apps/backend/internal/agent/runtime/lifecycle/testing_support.go @@ -1,7 +1,15 @@ package lifecycle +import agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + // ExecutionStoreForTesting returns the manager's execution store. // Intended for use by test packages that need to inject executions. func (m *Manager) ExecutionStoreForTesting() *ExecutionStore { return m.executionStore } + +// SetAgentCtlClientForTesting injects an agentctl client into a synthetic +// execution used by focused tests in dependent packages. +func (ae *AgentExecution) SetAgentCtlClientForTesting(client *agentctl.Client) { + ae.agentctl = client +} diff --git a/apps/backend/internal/agentctl/server/instance/manager.go b/apps/backend/internal/agentctl/server/instance/manager.go index 91370156cd..4de6e454d5 100644 --- a/apps/backend/internal/agentctl/server/instance/manager.go +++ b/apps/backend/internal/agentctl/server/instance/manager.go @@ -10,13 +10,13 @@ import ( "os/exec" "strings" "sync" - "syscall" "time" "github.com/google/uuid" "github.com/kandev/kandev/internal/agentctl/server/config" "github.com/kandev/kandev/internal/agentctl/server/process" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/netutil" "github.com/kandev/kandev/pkg/agent" "go.uber.org/zap" ) @@ -303,7 +303,7 @@ func (m *Manager) allocatePortAndListener(id string) (int, net.Listener, error) // all interfaces so Docker/remote executors can reach the instance. ln, err := net.Listen("tcp", fmt.Sprintf("%s:%d", m.config.ListenHost(), allocated)) if err != nil { - if errors.Is(err, syscall.EADDRINUSE) || strings.Contains(err.Error(), "address already in use") { + if netutil.IsAddrInUse(err) { m.portAlloc.MarkUnavailable(allocated) m.logger.Warn("port already in use; retrying", zap.String("instance_id", id), diff --git a/apps/backend/internal/agentctl/server/instance/manager_ports_test.go b/apps/backend/internal/agentctl/server/instance/manager_ports_test.go new file mode 100644 index 0000000000..039af79f6b --- /dev/null +++ b/apps/backend/internal/agentctl/server/instance/manager_ports_test.go @@ -0,0 +1,56 @@ +package instance + +import ( + "context" + "net" + "strconv" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" +) + +func TestAllocatePortAndListenerRetriesAddressInUse(t *testing.T) { + var occupied net.Listener + var base int + for attempt := 0; attempt < 10; attempt++ { + candidate, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen for occupied port: %v", err) + } + candidatePort := candidate.Addr().(*net.TCPAddr).Port + if candidatePort < 65535 { + next, err := net.Listen("tcp", "127.0.0.1:"+strconv.Itoa(candidatePort+1)) + if err != nil { + _ = candidate.Close() + continue + } + _ = next.Close() + occupied = candidate + base = candidatePort + break + } + _ = candidate.Close() + } + if occupied == nil { + t.Skip("could not reserve an occupied port with a valid consecutive retry range") + } + t.Cleanup(func() { _ = occupied.Close() }) + + mgr := NewManager(&config.Config{ + Ports: config.PortConfig{Base: base, Max: base + 1}, + }, newTestLogger(t)) + t.Cleanup(func() { _ = mgr.Shutdown(context.Background()) }) + + port, listener, err := mgr.allocatePortAndListener("retry") + if err != nil { + t.Fatalf("allocatePortAndListener: %v", err) + } + t.Cleanup(func() { + _ = listener.Close() + mgr.portAlloc.Release(port) + }) + + if port != base+1 { + t.Fatalf("allocated port = %d, want retry on %d", port, base+1) + } +} diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index c5f98e821e..a4fda901ab 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -675,16 +675,7 @@ func registerRoutes(p routeParams) { // Before that, return 503 so callers (including the e2e fixture's // waitForHealth) keep polling instead of racing ahead and hitting // 404s on routes that aren't wired yet. - p.router.GET("/health", func(c *gin.Context) { - if !ready.Load() { - c.JSON(http.StatusServiceUnavailable, gin.H{"status": "starting", "service": "kandev"}) - return - } - if token := desktopHealthToken(); token != "" { - c.Header(desktopHealthTokenHeader, token) - } - c.JSON(http.StatusOK, gin.H{"status": "ok", "service": "kandev", "mode": "websocket+http"}) - }) + p.router.GET("/health", healthHandler()) // /api/v1/features is a public, unauthenticated read of the runtime // feature-flag map. The frontend SSR-fetches it once per page render to @@ -704,7 +695,6 @@ func registerRoutes(p routeParams) { payload := bootPayload(c.Request.Context(), c.Request, p, route) c.JSON(http.StatusOK, payload) }) - if p.webInternalURL == "" { if handler, distDir, ok := newWebAppHandler(p); ok { p.router.NoRoute(func(c *gin.Context) { @@ -746,6 +736,19 @@ func registerRoutes(p routeParams) { } } +func healthHandler() gin.HandlerFunc { + return func(c *gin.Context) { + if !ready.Load() { + c.JSON(http.StatusServiceUnavailable, gin.H{"status": "starting", "service": "kandev"}) + return + } + if token := desktopHealthToken(); token != "" { + c.Header(desktopHealthTokenHeader, token) + } + c.JSON(http.StatusOK, gin.H{"status": "ok", "service": "kandev", "mode": "websocket+http"}) + } +} + func desktopHealthToken() string { return strings.TrimSpace(os.Getenv(desktopHealthTokenEnv)) } diff --git a/apps/backend/internal/backendapp/server_test.go b/apps/backend/internal/backendapp/server_test.go index 4777371409..23d68bf19f 100644 --- a/apps/backend/internal/backendapp/server_test.go +++ b/apps/backend/internal/backendapp/server_test.go @@ -2,7 +2,11 @@ package backendapp import ( "net" + "net/http" + "net/http/httptest" "testing" + + "github.com/gin-gonic/gin" ) func TestServerListenAddr(t *testing.T) { @@ -35,6 +39,26 @@ func TestDesktopHealthTokenTrimsEnv(t *testing.T) { } } +func TestHealthHandlerEchoesConfiguredToken(t *testing.T) { + gin.SetMode(gin.TestMode) + t.Setenv(desktopHealthTokenEnv, "route-health-token") + ready.Store(true) + t.Cleanup(func() { ready.Store(false) }) + + router := gin.New() + router.GET("/health", healthHandler()) + response := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/health", nil) + router.ServeHTTP(response, request) + + if response.Code != http.StatusOK { + t.Fatalf("health status = %d, want %d", response.Code, http.StatusOK) + } + if got := response.Header().Get(desktopHealthTokenHeader); got != "route-health-token" { + t.Fatalf("health token header = %q, want route-health-token", got) + } +} + func TestServerProbeAddr(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/common/netutil/addr_in_use.go b/apps/backend/internal/common/netutil/addr_in_use.go new file mode 100644 index 0000000000..2b2e8ade1e --- /dev/null +++ b/apps/backend/internal/common/netutil/addr_in_use.go @@ -0,0 +1,8 @@ +// Package netutil contains small helpers for network operations. +package netutil + +// IsAddrInUse reports whether err indicates that a requested network address +// is already bound by another listener. +func IsAddrInUse(err error) bool { + return isAddrInUse(err) +} diff --git a/apps/backend/internal/common/netutil/addr_in_use_test.go b/apps/backend/internal/common/netutil/addr_in_use_test.go new file mode 100644 index 0000000000..814b504371 --- /dev/null +++ b/apps/backend/internal/common/netutil/addr_in_use_test.go @@ -0,0 +1,31 @@ +package netutil + +import ( + "errors" + "net" + "testing" +) + +func TestIsAddrInUseRecognizesDoubleBind(t *testing.T) { + first, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen for first socket: %v", err) + } + t.Cleanup(func() { _ = first.Close() }) + + second, err := net.Listen("tcp", first.Addr().String()) + if err == nil { + _ = second.Close() + t.Fatal("second listener unexpectedly bound to an occupied address") + } + + if !IsAddrInUse(err) { + t.Fatalf("IsAddrInUse(%v) = false, want true", err) + } +} + +func TestIsAddrInUseRejectsOtherErrors(t *testing.T) { + if IsAddrInUse(errors.New("permission denied")) { + t.Fatal("IsAddrInUse reported an unrelated error as address-in-use") + } +} diff --git a/apps/backend/internal/common/netutil/addr_in_use_unix.go b/apps/backend/internal/common/netutil/addr_in_use_unix.go new file mode 100644 index 0000000000..b7e43954cd --- /dev/null +++ b/apps/backend/internal/common/netutil/addr_in_use_unix.go @@ -0,0 +1,12 @@ +//go:build !windows + +package netutil + +import ( + "errors" + "syscall" +) + +func isAddrInUse(err error) bool { + return errors.Is(err, syscall.EADDRINUSE) +} diff --git a/apps/backend/internal/common/netutil/addr_in_use_windows.go b/apps/backend/internal/common/netutil/addr_in_use_windows.go new file mode 100644 index 0000000000..42248df5eb --- /dev/null +++ b/apps/backend/internal/common/netutil/addr_in_use_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package netutil + +import ( + "errors" + "syscall" + + "golang.org/x/sys/windows" +) + +func isAddrInUse(err error) bool { + return errors.Is(err, syscall.EADDRINUSE) || errors.Is(err, windows.WSAEADDRINUSE) +} diff --git a/apps/backend/internal/gateway/websocket/port_tunnel.go b/apps/backend/internal/gateway/websocket/port_tunnel.go index 8c070132b4..daa20256ed 100644 --- a/apps/backend/internal/gateway/websocket/port_tunnel.go +++ b/apps/backend/internal/gateway/websocket/port_tunnel.go @@ -2,22 +2,20 @@ package websocket import ( "context" - "errors" "fmt" "net" "net/http" "net/http/httputil" "net/url" - "os" "strconv" "strings" "sync" - "syscall" "go.uber.org/zap" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/netutil" ) // TunnelInfo describes an active tunnel binding. @@ -139,7 +137,7 @@ func (m *TunnelManager) resolveAndBind(cacheKey, sessionID string, tunnelPort in ln, err := net.Listen("tcp", fmt.Sprintf(":%d", tunnelPort)) if err != nil { cleanup() - if isAddrInUse(err) { + if netutil.IsAddrInUse(err) { return nil, "", nil, fmt.Errorf("port %d is already in use, choose a different port", tunnelPort) } return nil, "", nil, fmt.Errorf("failed to bind tunnel port %d: %w", tunnelPort, err) @@ -255,17 +253,6 @@ func (m *TunnelManager) Shutdown() { } } -func isAddrInUse(err error) bool { - var opErr *net.OpError - if errors.As(err, &opErr) { - var sysErr *os.SyscallError - if errors.As(opErr.Err, &sysErr) { - return errors.Is(sysErr.Err, syscall.EADDRINUSE) - } - } - return false -} - func (m *TunnelManager) removeTunnel(cacheKey string) { m.mu.Lock() defer m.mu.Unlock() diff --git a/apps/backend/internal/gateway/websocket/port_tunnel_test.go b/apps/backend/internal/gateway/websocket/port_tunnel_test.go new file mode 100644 index 0000000000..bb5d20c717 --- /dev/null +++ b/apps/backend/internal/gateway/websocket/port_tunnel_test.go @@ -0,0 +1,57 @@ +package websocket + +import ( + "fmt" + "net" + "strings" + "testing" + + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events/bus" + "go.uber.org/zap" +) + +func TestResolveAndBindReportsOccupiedPort(t *testing.T) { + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("create logger: %v", err) + } + + lifecycleMgr := lifecycle.NewManager( + nil, + bus.NewMemoryEventBus(log), + nil, + nil, + nil, + nil, + lifecycle.ExecutorFallbackDeny, + t.TempDir(), + log, + ) + t.Cleanup(func() { _ = lifecycleMgr.Stop() }) + + execution := &lifecycle.AgentExecution{ID: "execution", SessionID: "session"} + execution.SetAgentCtlClientForTesting(agentctl.NewClient("127.0.0.1", 1, log)) + if err := lifecycleMgr.ExecutionStoreForTesting().Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + occupied, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatalf("listen for occupied tunnel port: %v", err) + } + t.Cleanup(func() { _ = occupied.Close() }) + port := occupied.Addr().(*net.TCPAddr).Port + + mgr := NewTunnelManager(lifecycleMgr, log) + _, _, _, err = mgr.resolveAndBind("session:1", "session", port) + if err == nil { + t.Fatal("resolveAndBind() = nil, want occupied-port error") + } + want := fmt.Sprintf("port %d is already in use", port) + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want substring %q", err, want) + } +} diff --git a/apps/backend/internal/launcher/env.go b/apps/backend/internal/launcher/env.go index f0f1213bf5..1c32b53d51 100644 --- a/apps/backend/internal/launcher/env.go +++ b/apps/backend/internal/launcher/env.go @@ -1,15 +1,28 @@ package launcher import ( + "crypto/rand" + "encoding/hex" "fmt" "os" ) -func backendEnv(ports portConfig, logLevel, consoleLogLevel string, debug bool) []string { +const healthTokenBytes = 32 + +func newHealthToken() (string, error) { + token := make([]byte, healthTokenBytes) + if _, err := rand.Read(token); err != nil { + return "", fmt.Errorf("generate backend health token: %w", err) + } + return hex.EncodeToString(token), nil +} + +func backendEnv(ports portConfig, logLevel, consoleLogLevel string, debug bool, healthToken string) []string { env := os.Environ() env = upsertEnv(env, "KANDEV_SERVER_PORT", fmt.Sprint(ports.BackendPort)) env = upsertEnv(env, "KANDEV_AGENT_STANDALONE_PORT", fmt.Sprint(ports.AgentctlPort)) env = upsertEnv(env, "KANDEV_DATABASE_PATH", resolveDatabasePath()) + env = upsertEnv(env, "KANDEV_DESKTOP_HEALTH_TOKEN", healthToken) if logLevel != "" { env = upsertEnv(env, "KANDEV_LOG_LEVEL", logLevel) } diff --git a/apps/backend/internal/launcher/health.go b/apps/backend/internal/launcher/health.go index f8b9c89547..9ffc9bc859 100644 --- a/apps/backend/internal/launcher/health.go +++ b/apps/backend/internal/launcher/health.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "strconv" "time" @@ -37,11 +38,12 @@ func healthTimeout(defaultMS int) time.Duration { return time.Duration(n) * time.Millisecond } -func waitForHealth(ctx context.Context, baseURL string, proc childState, timeout time.Duration, onFailure func()) error { +func waitForHealth(ctx context.Context, baseURL string, proc childState, timeout time.Duration, expectedToken string, onFailure func()) error { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() healthURL := baseURL + "/health" + sawForeignProcessResponse := false for ctx.Err() == nil { if exited, code := proc.Exited(); exited { if onFailure != nil { @@ -49,7 +51,11 @@ func waitForHealth(ctx context.Context, baseURL string, proc childState, timeout } return fmt.Errorf("backend exited (code %d) before healthcheck passed", code) } - if probeHealth(ctx, healthURL) { + healthy, foreign := probeHealth(ctx, healthURL, expectedToken) + if foreign { + sawForeignProcessResponse = true + } + if healthy { return nil } select { @@ -63,21 +69,43 @@ func waitForHealth(ctx context.Context, baseURL string, proc childState, timeout if errors.Is(ctx.Err(), context.Canceled) { return fmt.Errorf("backend healthcheck canceled at %s: %w", healthURL, ctx.Err()) } + if sawForeignProcessResponse { + return fmt.Errorf( + "backend port %s answered a health check from a different process "+ + "(missing/mismatched launcher token). Another Kandev instance may already own it, "+ + "or the runtime bundle predates v0.66.0", + healthPort(baseURL), + ) + } return fmt.Errorf("backend healthcheck timed out after %s at %s", timeout, healthURL) } // probeHealth reports whether a single health request succeeded. The body is // drained and closed so the connection can be reused by the next poll. -func probeHealth(ctx context.Context, healthURL string) bool { +func probeHealth(ctx context.Context, healthURL, expectedToken string) (healthy, foreign bool) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthURL, nil) if err != nil { - return false + return false, false } resp, err := healthProbeClient.Do(req) if err != nil { - return false + return false, false } defer func() { _ = resp.Body.Close() }() _, _ = io.Copy(io.Discard, resp.Body) - return resp.StatusCode >= 200 && resp.StatusCode < 300 + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return false, false + } + if expectedToken == "" || resp.Header.Get("X-Kandev-Desktop-Health-Token") == expectedToken { + return true, false + } + return false, true +} + +func healthPort(baseURL string) string { + parsed, err := url.Parse(baseURL) + if err == nil && parsed.Port() != "" { + return parsed.Port() + } + return baseURL } diff --git a/apps/backend/internal/launcher/health_test.go b/apps/backend/internal/launcher/health_test.go index f732b01e0f..3075715c02 100644 --- a/apps/backend/internal/launcher/health_test.go +++ b/apps/backend/internal/launcher/health_test.go @@ -53,13 +53,14 @@ func TestWaitForHealthReturnsNilOnHealthyResponse(t *testing.T) { _, _ = w.Write([]byte("starting")) return } + w.Header().Set("X-Kandev-Desktop-Health-Token", "expected") w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("ok")) })) t.Cleanup(srv.Close) var failures int - err := waitForHealth(context.Background(), srv.URL, fakeChild{}, 5*time.Second, func() { failures++ }) + err := waitForHealth(context.Background(), srv.URL, fakeChild{}, 5*time.Second, "expected", func() { failures++ }) if err != nil { t.Fatalf("waitForHealth() = %v, want nil", err) } @@ -71,6 +72,26 @@ func TestWaitForHealthReturnsNilOnHealthyResponse(t *testing.T) { } } +func TestWaitForHealthIgnoresHealthyResponseWithoutMatchingToken(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-Kandev-Desktop-Health-Token", "wrong") + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + err := waitForHealth(context.Background(), srv.URL, fakeChild{}, 400*time.Millisecond, "expected", nil) + if err == nil { + t.Fatal("waitForHealth() = nil, want different-process error") + } + want := "answered a health check from a different process (missing/mismatched launcher token)" + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want %q", err, want) + } + if !strings.Contains(err.Error(), "runtime bundle predates v0.66.0") { + t.Fatalf("error = %v, want legacy runtime guidance", err) + } +} + func TestWaitForHealthTimesOutOnNon2xxResponse(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusInternalServerError) @@ -78,7 +99,7 @@ func TestWaitForHealthTimesOutOnNon2xxResponse(t *testing.T) { t.Cleanup(srv.Close) var failures int - err := waitForHealth(context.Background(), srv.URL, fakeChild{}, 400*time.Millisecond, func() { failures++ }) + err := waitForHealth(context.Background(), srv.URL, fakeChild{}, 400*time.Millisecond, "", func() { failures++ }) if err == nil { t.Fatal("waitForHealth() = nil, want timeout error") } @@ -107,7 +128,7 @@ func TestWaitForHealthReturnsWhenServerHangs(t *testing.T) { done := make(chan error, 1) go func() { - done <- waitForHealth(context.Background(), srv.URL, fakeChild{}, 300*time.Millisecond, nil) + done <- waitForHealth(context.Background(), srv.URL, fakeChild{}, 300*time.Millisecond, "", nil) }() select { @@ -140,7 +161,7 @@ func TestWaitForHealthReturnsWhenContextCanceled(t *testing.T) { done := make(chan error, 1) go func() { // A generous timeout: only cancellation can end this wait in time. - done <- waitForHealth(ctx, srv.URL, fakeChild{}, time.Minute, nil) + done <- waitForHealth(ctx, srv.URL, fakeChild{}, time.Minute, "", nil) }() cancel() @@ -157,6 +178,25 @@ func TestWaitForHealthReturnsWhenContextCanceled(t *testing.T) { } } +func TestWaitForHealthCallsOnFailureWhenContextCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var failures atomic.Int32 + err := waitForHealth(ctx, "http://127.0.0.1:1", fakeChild{}, time.Minute, "", func() { + failures.Add(1) + }) + if err == nil { + t.Fatal("waitForHealth() = nil, want cancellation error") + } + if !strings.Contains(err.Error(), "canceled") { + t.Fatalf("error = %v, want a cancellation error", err) + } + if got := failures.Load(); got != 1 { + t.Fatalf("onFailure called %d times, want 1", got) + } +} + func TestWaitForHealthFailsFastWhenBackendExited(t *testing.T) { var failures int err := waitForHealth( @@ -164,6 +204,7 @@ func TestWaitForHealthFailsFastWhenBackendExited(t *testing.T) { "http://127.0.0.1:1", fakeChild{exited: true, code: 3}, time.Minute, + "", func() { failures++ }, ) if err == nil { diff --git a/apps/backend/internal/launcher/ports.go b/apps/backend/internal/launcher/ports.go index 85529494c1..ce9c43d040 100644 --- a/apps/backend/internal/launcher/ports.go +++ b/apps/backend/internal/launcher/ports.go @@ -15,14 +15,20 @@ type portConfig struct { BackendURL string } +const ( + backendPortFlag = "--port" + backendPortEnv = "KANDEV_BACKEND_PORT" + legacyBackendPortEnv = "KANDEV_PORT" +) + func resolvePorts(opts Options) (int, error) { backend := opts.BackendPort if backend == 0 { - if p, err := envPort("KANDEV_BACKEND_PORT"); err != nil { + if p, err := envPort(backendPortEnv); err != nil { return 0, err } else if p != 0 { backend = p - } else if p, err := envPort("KANDEV_PORT"); err != nil { + } else if p, err := envPort(legacyBackendPortEnv); err != nil { return 0, err } else { backend = p @@ -31,6 +37,19 @@ func resolvePorts(opts Options) (int, error) { return backend, nil } +func backendPortSource(opts Options) string { + if opts.BackendPort != 0 { + return backendPortFlag + } + if _, ok := os.LookupEnv(backendPortEnv); ok { + return backendPortEnv + } + if _, ok := os.LookupEnv(legacyBackendPortEnv); ok { + return legacyBackendPortEnv + } + return "" +} + func envPort(name string) (int, error) { raw, ok := os.LookupEnv(name) if !ok { @@ -43,7 +62,7 @@ func envPort(name string) (int, error) { return n, nil } -func pickPorts(backendPort int) (portConfig, error) { +func pickPorts(backendPort int, source string) (portConfig, error) { used := map[int]bool{} backend := backendPort if backend == 0 { @@ -52,6 +71,12 @@ func pickPorts(backendPort int) (portConfig, error) { return portConfig{}, err } backend = p + } else if !canBind(backend) { + sourceSuffix := "" + if source != "" { + sourceSuffix = fmt.Sprintf(" from %s", source) + } + return portConfig{}, fmt.Errorf("backend port %d%s is already in use", backend, sourceSuffix) } used[backend] = true agentctl, err := pickAvailablePortExcept(defaultAgentctlPort, used) diff --git a/apps/backend/internal/launcher/ports_test.go b/apps/backend/internal/launcher/ports_test.go index a8b43fb3d5..878c486197 100644 --- a/apps/backend/internal/launcher/ports_test.go +++ b/apps/backend/internal/launcher/ports_test.go @@ -1,6 +1,11 @@ package launcher -import "testing" +import ( + "net" + "strconv" + "strings" + "testing" +) func TestPickAvailablePortExceptSkipsUsedPreferredPort(t *testing.T) { port, err := pickAvailablePortExcept(defaultBackendPort, map[int]bool{defaultBackendPort: true}) @@ -11,3 +16,38 @@ func TestPickAvailablePortExceptSkipsUsedPreferredPort(t *testing.T) { t.Fatalf("picked reserved preferred port %d", port) } } + +func TestPickPortsRejectsOccupiedExplicitBackendPort(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = ln.Close() }) + + port := ln.Addr().(*net.TCPAddr).Port + _, err = pickPorts(port, "KANDEV_PORT") + if err == nil { + t.Fatalf("pickPorts(%d) succeeded while the requested port was occupied", port) + } + if !strings.Contains(err.Error(), "backend port "+strconv.Itoa(port)) { + t.Fatalf("error = %v, want the occupied backend port", err) + } + if !strings.Contains(err.Error(), "from KANDEV_PORT") { + t.Fatalf("error = %v, want the configuration source", err) + } +} + +func TestBackendPortSourceUsesLauncherPrecedence(t *testing.T) { + t.Setenv("KANDEV_BACKEND_PORT", "4000") + t.Setenv("KANDEV_PORT", "5000") + if got := backendPortSource(Options{BackendPort: 3000}); got != "--port" { + t.Fatalf("backendPortSource(cli) = %q, want --port", got) + } + if got := backendPortSource(Options{}); got != "KANDEV_BACKEND_PORT" { + t.Fatalf("backendPortSource(specific env) = %q, want KANDEV_BACKEND_PORT", got) + } + t.Setenv("KANDEV_BACKEND_PORT", "") + if got := backendPortSource(Options{}); got != "KANDEV_BACKEND_PORT" { + t.Fatalf("backendPortSource(empty specific env) = %q, want KANDEV_BACKEND_PORT", got) + } +} diff --git a/apps/backend/internal/launcher/run.go b/apps/backend/internal/launcher/run.go index c3913a7704..429bc6ff07 100644 --- a/apps/backend/internal/launcher/run.go +++ b/apps/backend/internal/launcher/run.go @@ -13,7 +13,7 @@ func runInstalled(ctx context.Context, opts Options) int { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 2 } - ports, err := pickPorts(backendPort) + ports, err := pickPorts(backendPort, backendPortSource(opts)) if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 diff --git a/apps/backend/internal/launcher/start.go b/apps/backend/internal/launcher/start.go index 8995903b5d..a061146661 100644 --- a/apps/backend/internal/launcher/start.go +++ b/apps/backend/internal/launcher/start.go @@ -26,7 +26,7 @@ func runStart(ctx context.Context, opts Options) int { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 2 } - ports, err := pickPorts(backendPort) + ports, err := pickPorts(backendPort, backendPortSource(opts)) if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 @@ -92,7 +92,12 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { supervisor := newSupervisorFn() attachSignalsFn(supervisor) shutdownDebugf("runManagedApp signal handler attached") - env := backendEnv(cfg.Ports, cfg.LogLevel, resolveConsoleLogLevel(cfg.Opts), cfg.Opts.Debug) + healthToken, err := newHealthToken() + if err != nil { + fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) + return 1 + } + env := backendEnv(cfg.Ports, cfg.LogLevel, resolveConsoleLogLevel(cfg.Opts), cfg.Opts.Debug, healthToken) backend, dumpLogs, err := launchBackendFn( cfg.Backend, []string{"__backend"}, cfg.BackendCWD, env, false, cfg.Ports, cfg.Mode, supervisor, ) @@ -102,7 +107,7 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { } shutdownDebugf("runManagedApp backend launched") fmt.Println("[kandev] starting backend...") - if err := waitForHealthFn(ctx, cfg.Ports.BackendURL, backend, healthTimeout(healthTimeoutReleaseMS), dumpLogs); err != nil { + if err := waitForHealthFn(ctx, cfg.Ports.BackendURL, backend, healthTimeout(healthTimeoutReleaseMS), healthToken, dumpLogs); err != nil { supervisor.shutdown("backend health failure") fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) return 1 diff --git a/apps/backend/internal/launcher/start_test.go b/apps/backend/internal/launcher/start_test.go index a9f81e2104..be9f1349ce 100644 --- a/apps/backend/internal/launcher/start_test.go +++ b/apps/backend/internal/launcher/start_test.go @@ -42,12 +42,13 @@ func TestResolveLogThresholdsByMode(t *testing.T) { } func TestBackendEnvCarriesIndependentThresholds(t *testing.T) { - env := backendEnv(portConfig{BackendPort: 1234, AgentctlPort: 5678}, "debug", "warn", true) + env := backendEnv(portConfig{BackendPort: 1234, AgentctlPort: 5678}, "debug", "warn", true, "health-token") joined := strings.Join(env, "\n") for _, expected := range []string{ "KANDEV_LOG_LEVEL=debug", "KANDEV_CONSOLE_LOG_LEVEL=warn", "KANDEV_DEBUG_AGENT_MESSAGES=true", + "KANDEV_DESKTOP_HEALTH_TOKEN=health-token", } { if !strings.Contains(joined, expected) { t.Fatalf("backend environment missing %q", expected) @@ -55,6 +56,23 @@ func TestBackendEnvCarriesIndependentThresholds(t *testing.T) { } } +func TestNewHealthTokenIsFreshAndOpaque(t *testing.T) { + first, err := newHealthToken() + if err != nil { + t.Fatal(err) + } + second, err := newHealthToken() + if err != nil { + t.Fatal(err) + } + if len(first) != healthTokenBytes*2 || len(second) != healthTokenBytes*2 { + t.Fatalf("health token lengths = %d and %d, want %d", len(first), len(second), healthTokenBytes*2) + } + if first == second { + t.Fatal("two health token generations returned the same value") + } +} + func TestRunStartUsesSelfExecutableAndBackendCWD(t *testing.T) { oldExecutablePath := executablePath oldLaunchManaged := launchManaged @@ -112,6 +130,8 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { var events []string var launchedQuiet bool + var launchedHealthToken string + var waitedHealthToken string newSupervisorFn = func() *processSupervisor { events = append(events, "new-supervisor") return newSupervisor() @@ -120,13 +140,18 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { _ string, _ []string, _ string, - _ []string, + env []string, quiet bool, _ portConfig, _ string, _ *processSupervisor, ) (*restartableBackend, func(), error) { launchedQuiet = quiet + for _, item := range env { + if strings.HasPrefix(item, "KANDEV_DESKTOP_HEALTH_TOKEN=") { + launchedHealthToken = strings.TrimPrefix(item, "KANDEV_DESKTOP_HEALTH_TOKEN=") + } + } events = append(events, "launch-backend") exitCh := make(chan int, 1) exitCh <- 0 @@ -135,7 +160,8 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { attachSignalsFn = func(_ *processSupervisor) { events = append(events, "attach-signals") } - waitForHealthFn = func(_ context.Context, _ string, _ childState, _ time.Duration, _ func()) error { + waitForHealthFn = func(_ context.Context, _ string, _ childState, _ time.Duration, expectedToken string, _ func()) error { + waitedHealthToken = expectedToken events = append(events, "wait-health") return nil } @@ -162,4 +188,7 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { if launchedQuiet { t.Fatal("backend stdout was not forwarded") } + if launchedHealthToken == "" || launchedHealthToken != waitedHealthToken { + t.Fatalf("health token launched=%q waited=%q, want one shared non-empty token", launchedHealthToken, waitedHealthToken) + } } diff --git a/apps/backend/internal/launcher/supervisor.go b/apps/backend/internal/launcher/supervisor.go index 603454b736..f2336ece83 100644 --- a/apps/backend/internal/launcher/supervisor.go +++ b/apps/backend/internal/launcher/supervisor.go @@ -171,6 +171,7 @@ func allowedSupervisorEnv(env []string) map[string]string { "KANDEV_HOME_DIR": true, "KANDEV_DATABASE_PATH": true, "KANDEV_SERVER_PORT": true, + "KANDEV_DESKTOP_HEALTH_TOKEN": true, "KANDEV_WEB_INTERNAL_URL": true, "KANDEV_AGENT_STANDALONE_PORT": true, "KANDEV_LOG_LEVEL": true, diff --git a/apps/backend/internal/launcher/supervisor_test.go b/apps/backend/internal/launcher/supervisor_test.go index 4d88a37364..6067aaa6f9 100644 --- a/apps/backend/internal/launcher/supervisor_test.go +++ b/apps/backend/internal/launcher/supervisor_test.go @@ -11,6 +11,7 @@ import ( func TestBuildManifestUsesSameBinaryBackendMode(t *testing.T) { env := []string{ "KANDEV_SERVER_PORT=1234", + "KANDEV_DESKTOP_HEALTH_TOKEN=health-token", "KANDEV_CONSOLE_LOG_LEVEL=warn", "UNRELATED=value", } @@ -31,6 +32,9 @@ func TestBuildManifestUsesSameBinaryBackendMode(t *testing.T) { if manifest.Env["KANDEV_CONSOLE_LOG_LEVEL"] != "warn" { t.Fatalf("KANDEV_CONSOLE_LOG_LEVEL = %q", manifest.Env["KANDEV_CONSOLE_LOG_LEVEL"]) } + if manifest.Env["KANDEV_DESKTOP_HEALTH_TOKEN"] != "health-token" { + t.Fatalf("KANDEV_DESKTOP_HEALTH_TOKEN = %q", manifest.Env["KANDEV_DESKTOP_HEALTH_TOKEN"]) + } } func TestPrepareSupervisorEnvWritesExpectedPaths(t *testing.T) { diff --git a/apps/cli/src/args.test.ts b/apps/cli/src/args.test.ts index 73c7e9f603..aae0dce8df 100644 --- a/apps/cli/src/args.test.ts +++ b/apps/cli/src/args.test.ts @@ -106,6 +106,7 @@ describe("resolvePorts", () => { KANDEV_PORT: "5000", } as NodeJS.ProcessEnv); expect(r.backendPort).toBe(3000); + expect(r.backendPortSource).toBe("--port"); }); it("KANDEV_BACKEND_PORT wins over KANDEV_PORT (more specific env wins)", () => { @@ -114,11 +115,13 @@ describe("resolvePorts", () => { KANDEV_BACKEND_PORT: "6666", } as NodeJS.ProcessEnv); expect(r.backendPort).toBe(6666); + expect(r.backendPortSource).toBe("KANDEV_BACKEND_PORT"); }); it("falls back to KANDEV_PORT when KANDEV_BACKEND_PORT is not set", () => { const r = resolvePorts({ command: "run" }, { KANDEV_PORT: "5555" } as NodeJS.ProcessEnv); expect(r.backendPort).toBe(5555); + expect(r.backendPortSource).toBe("KANDEV_PORT"); }); it("KANDEV_WEB_PORT sets the internal web port in dev", () => { @@ -133,7 +136,11 @@ describe("resolvePorts", () => { it("--port maps to backend in every command (including dev)", () => { const r = resolvePorts({ command: "dev", backendPort: 3447 }, {} as NodeJS.ProcessEnv); - expect(r).toEqual({ backendPort: 3447, webPort: undefined }); + expect(r).toEqual({ + backendPort: 3447, + backendPortSource: "--port", + webPort: undefined, + }); }); it("throws ParseError when KANDEV_PORT is not a number", () => { diff --git a/apps/cli/src/args.ts b/apps/cli/src/args.ts index 00a8a62628..9c499976d2 100644 --- a/apps/cli/src/args.ts +++ b/apps/cli/src/args.ts @@ -1,5 +1,7 @@ export type Command = "run" | "dev" | "start"; +export type BackendPortSource = "--port" | "KANDEV_BACKEND_PORT" | "KANDEV_PORT"; + export type CliOptions = { command: Command; runtimeVersion?: string; @@ -111,6 +113,7 @@ function parsePort(raw: string, flag: string): number { export type ResolvedPorts = { backendPort?: number; + backendPortSource?: BackendPortSource; webPort?: number; }; @@ -119,9 +122,27 @@ export function resolvePorts(options: CliOptions, env: NodeJS.ProcessEnv): Resol if (options.webPort !== undefined && options.command !== "dev") { throw new ParseError("--web-internal-port only applies to dev mode"); } + + let backendPort: number | undefined; + let backendPortSource: BackendPortSource | undefined; + if (options.backendPort !== undefined) { + backendPort = options.backendPort; + backendPortSource = "--port"; + } else { + backendPort = envPort(env, "KANDEV_BACKEND_PORT"); + if (backendPort !== undefined) { + backendPortSource = "KANDEV_BACKEND_PORT"; + } else { + backendPort = envPort(env, "KANDEV_PORT"); + if (backendPort !== undefined) { + backendPortSource = "KANDEV_PORT"; + } + } + } + return { - backendPort: - options.backendPort ?? envPort(env, "KANDEV_BACKEND_PORT") ?? envPort(env, "KANDEV_PORT"), + backendPort, + ...(backendPortSource ? { backendPortSource } : {}), webPort: options.command === "dev" ? (options.webPort ?? envPort(env, "KANDEV_WEB_PORT")) : undefined, }; diff --git a/apps/cli/src/cli.ts b/apps/cli/src/cli.ts index 762d6b1e40..4a82a92920 100644 --- a/apps/cli/src/cli.ts +++ b/apps/cli/src/cli.ts @@ -119,7 +119,12 @@ async function main(): Promise { if (!repoRoot) { throw new Error("Unable to locate repo root for dev. Run from the repo."); } - await runDev({ repoRoot, backendPort, webPort }); + await runDev({ + repoRoot, + backendPort, + backendPortSource: resolved.backendPortSource, + webPort, + }); return; } @@ -131,6 +136,7 @@ async function main(): Promise { await runStart({ repoRoot, backendPort, + backendPortSource: resolved.backendPortSource, verbose: options.verbose, debug: options.debug, headless: options.headless, @@ -141,6 +147,7 @@ async function main(): Promise { await runRelease({ runtimeVersion: options.runtimeVersion, backendPort, + backendPortSource: resolved.backendPortSource, verbose: options.verbose, debug: options.debug, headless: options.headless, diff --git a/apps/cli/src/dev.ts b/apps/cli/src/dev.ts index f79134bcbb..b9849859e7 100644 --- a/apps/cli/src/dev.ts +++ b/apps/cli/src/dev.ts @@ -1,6 +1,7 @@ import fs from "node:fs"; import path from "node:path"; +import type { BackendPortSource } from "./args"; import { backupProductionDb, isProductionDb } from "./backup"; import { devKandevHome, HEALTH_TIMEOUT_MS_DEV } from "./constants"; import { resolveHealthTimeoutMs, waitForHealth, waitForUrlReady } from "./health"; @@ -20,11 +21,17 @@ import { launchWebApp, openBrowser } from "./web"; export type DevOptions = { repoRoot: string; backendPort?: number; + backendPortSource?: BackendPortSource; webPort?: number; }; -export async function runDev({ repoRoot, backendPort, webPort }: DevOptions): Promise { - const ports = await pickPorts(backendPort, webPort); +export async function runDev({ + repoRoot, + backendPort, + backendPortSource, + webPort, +}: DevOptions): Promise { + const ports = await pickPorts({ backendPort, backendPortSource, webPort }); const { dbPath, extra } = resolveDevBackendEnv(repoRoot); if (isProductionDb(dbPath)) { @@ -86,13 +93,19 @@ export async function runDev({ repoRoot, backendPort, webPort }: DevOptions): Pr const healthTimeoutMs = resolveHealthTimeoutMs(HEALTH_TIMEOUT_MS_DEV); console.log("[kandev] starting backend..."); - await waitForHealth(ports.backendUrl, backend.proc, healthTimeoutMs, () => { - const buffered = output.read(); - if (buffered.trim().length === 0) return; - console.error("[kandev] --- backend stdout (last captured output) ---"); - console.error(buffered.trimEnd()); - console.error("[kandev] --- end backend stdout ---"); - }); + await waitForHealth( + ports.backendUrl, + backend.proc, + healthTimeoutMs, + backendEnv.KANDEV_DESKTOP_HEALTH_TOKEN, + () => { + const buffered = output.read(); + if (buffered.trim().length === 0) return; + console.error("[kandev] --- backend stdout (last captured output) ---"); + console.error(buffered.trimEnd()); + console.error("[kandev] --- end backend stdout ---"); + }, + ); console.log(`[kandev] backend ready at ${ports.backendUrl}`); console.log("[kandev] starting web..."); diff --git a/apps/cli/src/health.test.ts b/apps/cli/src/health.test.ts index 0e209454e1..8683b03de2 100644 --- a/apps/cli/src/health.test.ts +++ b/apps/cli/src/health.test.ts @@ -57,11 +57,44 @@ describe("waitForHealth", () => { it("returns once /health responds ok", async () => { const proc: ProcState = { exitCode: null }; - fetchMock - .mockRejectedValueOnce(new Error("ECONNREFUSED")) - .mockResolvedValue({ ok: true } as Response); + fetchMock.mockRejectedValueOnce(new Error("ECONNREFUSED")).mockResolvedValue({ + ok: true, + headers: new Headers({ "X-Kandev-Desktop-Health-Token": "expected" }), + } as Response); - await expect(waitForHealth("http://localhost:8080", proc, 1000)).resolves.toBeUndefined(); + await expect( + waitForHealth("http://localhost:8080", proc, 1000, "expected"), + ).resolves.toBeUndefined(); + }); + + it("ignores a successful response with a missing or mismatched launch token", async () => { + const proc: ProcState = { exitCode: null }; + fetchMock + .mockResolvedValueOnce({ ok: true, headers: new Headers() } as Response) + .mockResolvedValueOnce({ + ok: true, + headers: new Headers({ "X-Kandev-Desktop-Health-Token": "wrong" }), + } as Response) + .mockResolvedValue({ + ok: true, + headers: new Headers({ "X-Kandev-Desktop-Health-Token": "expected" }), + } as Response); + + await expect( + waitForHealth("http://localhost:8080", proc, 1500, "expected"), + ).resolves.toBeUndefined(); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it("reports a different process when successful responses never match the launch token", async () => { + const proc: ProcState = { exitCode: null }; + fetchMock.mockResolvedValue({ ok: true, headers: new Headers() } as Response); + + await expect(waitForHealth("http://localhost:38429", proc, 120, "expected")).rejects.toThrow( + "Backend port 38429 answered a health check from a different process " + + "(missing/mismatched launcher token). Another Kandev instance may already own it, " + + "or the runtime bundle predates v0.66.0.", + ); }); it("includes the exit code and invokes onFailure when backend exits early", async () => { @@ -69,7 +102,7 @@ describe("waitForHealth", () => { fetchMock.mockRejectedValue(new Error("ECONNREFUSED")); const onFailure = vi.fn(); - await expect(waitForHealth("http://localhost:8080", proc, 1000, onFailure)).rejects.toThrow( + await expect(waitForHealth("http://localhost:8080", proc, 1000, "", onFailure)).rejects.toThrow( "Backend exited (code 2) before healthcheck passed", ); expect(onFailure).toHaveBeenCalledTimes(1); @@ -80,7 +113,7 @@ describe("waitForHealth", () => { fetchMock.mockRejectedValue(new Error("ECONNREFUSED")); const onFailure = vi.fn(); - await expect(waitForHealth("http://localhost:8080", proc, 120, onFailure)).rejects.toThrow( + await expect(waitForHealth("http://localhost:8080", proc, 120, "", onFailure)).rejects.toThrow( /http:\/\/localhost:8080\/health.*KANDEV_HEALTH_TIMEOUT_MS/s, ); expect(onFailure).toHaveBeenCalledTimes(1); diff --git a/apps/cli/src/health.ts b/apps/cli/src/health.ts index 0c7ec97d27..e4d9bbc4ce 100644 --- a/apps/cli/src/health.ts +++ b/apps/cli/src/health.ts @@ -28,10 +28,12 @@ export async function waitForHealth( baseUrl: string, proc: { exitCode: number | null }, timeoutMs: number, + expectedToken = "", onFailure?: () => void, ): Promise { const deadline = Date.now() + timeoutMs; const healthUrl = `${baseUrl}/health`; + let sawForeignProcessResponse = false; while (Date.now() < deadline) { if (proc.exitCode !== null) { onFailure?.(); @@ -39,7 +41,10 @@ export async function waitForHealth( } try { const res = await fetch(healthUrl); - if (res.ok) { + if (res.ok && expectedToken !== "" && !matchesHealthToken(res, expectedToken)) { + sawForeignProcessResponse = true; + } + if (res.ok && matchesHealthToken(res, expectedToken)) { return; } } catch { @@ -48,6 +53,13 @@ export async function waitForHealth( await delay(300); } onFailure?.(); + if (sawForeignProcessResponse) { + throw new Error( + `Backend port ${healthPort(baseUrl)} answered a health check from a different process ` + + `(missing/mismatched launcher token). Another Kandev instance may already own it, ` + + `or the runtime bundle predates v0.66.0.`, + ); + } throw new Error( `Backend healthcheck timed out after ${timeoutMs}ms at ${healthUrl}. ` + `If your machine is slow on first run (antivirus scan, cold disk), ` + @@ -55,6 +67,18 @@ export async function waitForHealth( ); } +function healthPort(baseUrl: string): string { + try { + return new URL(baseUrl).port || baseUrl; + } catch { + return baseUrl; + } +} + +function matchesHealthToken(res: Response, expectedToken: string): boolean { + return expectedToken === "" || res.headers.get("X-Kandev-Desktop-Health-Token") === expectedToken; +} + export async function waitForUrlReady( url: string, proc: { exitCode: number | null }, diff --git a/apps/cli/src/ports.test.ts b/apps/cli/src/ports.test.ts index 06b2962c48..3d15b2f24f 100644 --- a/apps/cli/src/ports.test.ts +++ b/apps/cli/src/ports.test.ts @@ -2,7 +2,12 @@ import net from "node:net"; import { afterEach, describe, expect, it } from "vitest"; -import { ensureValidPort, pickAvailablePort, __testing } from "./ports"; +import { + assertPortAvailable, + ensureValidPort, + pickAvailablePortExcluding, + __testing, +} from "./ports"; describe("ensureValidPort", () => { it("returns undefined for undefined input", () => { @@ -112,16 +117,24 @@ describe("isPortInUse", () => { }); }); -describe("pickAvailablePort", () => { +describe("pickAvailablePortExcluding", () => { it("returns the preferred port when it is free", async () => { const port = await findReportedAvailablePort(); - expect(await pickAvailablePort(port)).toBe(port); + expect(await pickAvailablePortExcluding(port, new Set())).toBe(port); + }); + + it("skips an excluded preferred port", async () => { + const preferred = await findReportedAvailablePort(); + const picked = await pickAvailablePortExcluding(preferred, new Set([preferred]), 5); + + expect(picked).not.toBe(preferred); + expect(picked).toBeGreaterThan(0); }); it("returns a fallback port when the preferred port is taken", async () => { const { server, port } = await listenOn("127.0.0.1"); try { - const picked = await pickAvailablePort(port, 5); + const picked = await pickAvailablePortExcluding(port, new Set(), 5); expect(picked).not.toBe(port); expect(picked).toBeGreaterThan(0); } finally { @@ -129,3 +142,16 @@ describe("pickAvailablePort", () => { } }); }); + +describe("assertPortAvailable", () => { + it("rejects an occupied explicit port with its configuration source", async () => { + const { server, port } = await listenOn("127.0.0.1"); + try { + await expect(assertPortAvailable(port, "KANDEV_BACKEND_PORT")).rejects.toThrow( + `Backend port ${port} from KANDEV_BACKEND_PORT is already in use`, + ); + } finally { + await closeServer(server); + } + }); +}); diff --git a/apps/cli/src/ports.ts b/apps/cli/src/ports.ts index e1841a06b8..175f9ab019 100644 --- a/apps/cli/src/ports.ts +++ b/apps/cli/src/ports.ts @@ -95,6 +95,12 @@ async function isPortAvailable(port: number): Promise { return canBindPort(port, "127.0.0.1"); } +export async function assertPortAvailable(port: number, source?: string): Promise { + if (await isPortAvailable(port)) return; + const sourceSuffix = source ? ` from ${source}` : ""; + throw new Error(`Backend port ${port}${sourceSuffix} is already in use`); +} + async function reserveSpecificPort(port: number, host = "127.0.0.1"): Promise { return new Promise((resolve) => { const server = net.createServer(); @@ -103,16 +109,17 @@ async function reserveSpecificPort(port: number, host = "127.0.0.1"): Promise, retries = RANDOM_PORT_RETRIES, ): Promise { - if (await isPortAvailable(preferred)) { + if (!excludedPorts.has(preferred) && (await isPortAvailable(preferred))) { return preferred; } for (let i = 0; i < retries; i += 1) { const candidate = crypto.randomInt(RANDOM_PORT_MIN, RANDOM_PORT_MAX + 1); - if (await isPortAvailable(candidate)) { + if (!excludedPorts.has(candidate) && (await isPortAvailable(candidate))) { return candidate; } } diff --git a/apps/cli/src/run.test.ts b/apps/cli/src/run.test.ts index 9fe7976ec1..d07723de1b 100644 --- a/apps/cli/src/run.test.ts +++ b/apps/cli/src/run.test.ts @@ -5,7 +5,12 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { getBinaryName } from "./platform"; -import { attachRingBuffer, cleanOldReleases, findCachedRelease } from "./run"; +import { + attachRingBuffer, + buildReleaseLaunchOptions, + cleanOldReleases, + findCachedRelease, +} from "./run"; // Mock CACHE_DIR to use a temp directory. const mockCacheDir = { value: "" }; @@ -221,3 +226,23 @@ describe("attachRingBuffer", () => { expect(read()).toBe(""); }); }); + +describe("buildReleaseLaunchOptions", () => { + it("preserves the explicit backend port source for release launches", () => { + expect( + buildReleaseLaunchOptions({ + runtimeVersion: "v1.2.3", + backendPort: 18080, + backendPortSource: "KANDEV_BACKEND_PORT", + verbose: true, + debug: true, + }), + ).toEqual({ + runtimeVersion: "v1.2.3", + backendPort: 18080, + backendPortSource: "KANDEV_BACKEND_PORT", + verbose: true, + debug: true, + }); + }); +}); diff --git a/apps/cli/src/run.ts b/apps/cli/src/run.ts index d68af4567e..b51565e4fe 100644 --- a/apps/cli/src/run.ts +++ b/apps/cli/src/run.ts @@ -2,6 +2,7 @@ import { spawn } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; +import type { BackendPortSource } from "./args"; import { ensureExtracted, findBundleRoot } from "./bundle"; import { DEFAULT_AGENTCTL_PORT, @@ -16,13 +17,13 @@ import { ensureAsset, getRelease } from "./github"; import { resolveHealthTimeoutMs, waitForHealth } from "./health"; import { getBinaryName, getPlatformDir } from "./platform"; import { sortVersionsDesc } from "./version"; -import { pickAvailablePort } from "./ports"; import { createProcessSupervisor } from "./process"; import { resolveRuntime, validateBundle } from "./runtime"; import { buildBackendEnv, createOutputRingBuffer, logStartupInfo, + pickBackendPorts, resolveBackendLogLevels, } from "./shared"; import { launchRestartableBackend } from "./supervisor/backend"; @@ -31,6 +32,7 @@ import { openBrowser } from "./web"; export type RunOptions = { runtimeVersion?: string; backendPort?: number; + backendPortSource?: BackendPortSource; /** Show backend info logs on stdout */ verbose?: boolean; /** Retain backend debug logs in its file + agent message dumps */ @@ -143,6 +145,7 @@ async function downloadRuntimeVersion(runtimeVersion: string): Promise { async function prepareBundleForLaunch({ runtimeVersion, backendPort, + backendPortSource, verbose = false, debug = false, }: RunOptions): Promise { @@ -182,9 +185,10 @@ async function prepareBundleForLaunch({ releaseTag = process.env.KANDEV_VERSION ?? `(${resolved.source})`; } - const actualBackendPort = backendPort ?? (await pickAvailablePort(DEFAULT_BACKEND_PORT)); - const agentctlPort = await pickAvailablePort(DEFAULT_AGENTCTL_PORT); - const backendUrl = `http://localhost:${actualBackendPort}`; + const ports = await pickBackendPorts({ backendPort, backendPortSource }); + const actualBackendPort = ports.backendPort; + const agentctlPort = ports.agentctlPort; + const backendUrl = ports.backendUrl; const levels = resolveBackendLogLevels({ verbose, debug }); const logLevel = levels.file; @@ -221,6 +225,22 @@ async function prepareBundleForLaunch({ }; } +export function buildReleaseLaunchOptions({ + runtimeVersion, + backendPort, + backendPortSource, + verbose = false, + debug = false, +}: RunOptions): RunOptions { + return { + runtimeVersion, + backendPort, + backendPortSource, + verbose, + debug, + }; +} + /** * Attach a ring buffer to a readable stream, keeping roughly the last `maxChars` * characters. Note: the limit is measured in JS string length (UTF-16 code units), @@ -290,20 +310,30 @@ async function launchBundle(prepared: PreparedBundle): Promise<{ export async function runRelease({ runtimeVersion, backendPort, + backendPortSource, verbose = false, debug = false, headless = false, }: RunOptions): Promise { - const prepared = await prepareBundleForLaunch({ - runtimeVersion, - backendPort, - verbose, - debug, - }); + const prepared = await prepareBundleForLaunch( + buildReleaseLaunchOptions({ + runtimeVersion, + backendPort, + backendPortSource, + verbose, + debug, + }), + ); const { backendProc, dumpBackendLogs } = await launchBundle(prepared); const healthTimeoutMs = resolveHealthTimeoutMs(HEALTH_TIMEOUT_MS_RELEASE); console.log("[kandev] starting backend..."); - await waitForHealth(prepared.backendUrl, backendProc, healthTimeoutMs, dumpBackendLogs); + await waitForHealth( + prepared.backendUrl, + backendProc, + healthTimeoutMs, + prepared.backendEnv.KANDEV_DESKTOP_HEALTH_TOKEN, + dumpBackendLogs, + ); console.log(`[kandev] backend ready at ${prepared.backendUrl}`); if (headless) { diff --git a/apps/cli/src/shared.test.ts b/apps/cli/src/shared.test.ts index 1db4082059..36760b5f4b 100644 --- a/apps/cli/src/shared.test.ts +++ b/apps/cli/src/shared.test.ts @@ -1,8 +1,11 @@ import os from "node:os"; +import net from "node:net"; import { PassThrough } from "node:stream"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_AGENTCTL_PORT, DEFAULT_WEB_PORT } from "./constants"; +import { __testing as portsTesting } from "./ports"; import { buildBackendEnv, buildWebEnv, @@ -10,6 +13,8 @@ import { listHostNetworkAddresses, logStartupInfo, networkUrlsForPort, + pickPorts, + pickBackendPorts, resolveBackendLogLevels, type PortConfig, } from "./shared"; @@ -87,6 +92,21 @@ describe("buildBackendEnv", () => { expect(env.KANDEV_WEB_INTERNAL_URL).toBeUndefined(); }); + it("generates a fresh backend health token for each launch environment", () => { + const first = buildBackendEnv({ ports }).KANDEV_DESKTOP_HEALTH_TOKEN; + const second = buildBackendEnv({ ports }).KANDEV_DESKTOP_HEALTH_TOKEN; + + expect(first).toMatch(/^[0-9a-f]{64}$/); + expect(second).toMatch(/^[0-9a-f]{64}$/); + expect(second).not.toBe(first); + }); + + it("uses a caller-provided backend health token", () => { + const env = buildBackendEnv({ ports, healthToken: "launch-owned-token" }); + + expect(env.KANDEV_DESKTOP_HEALTH_TOKEN).toBe("launch-owned-token"); + }); + it("uses independent file and console thresholds", () => { expect(resolveBackendLogLevels({ env: {} })).toEqual({ file: "info", console: "warn" }); expect(resolveBackendLogLevels({ debug: true, env: {} })).toEqual({ @@ -123,6 +143,57 @@ describe("buildBackendEnv", () => { }); }); +describe("explicit backend port selection", () => { + it("rejects an occupied port before selecting the other services", async () => { + const server = net.createServer(); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => resolve()); + }); + const address = server.address(); + if (!address || typeof address === "string") { + server.close(); + throw new Error("test server did not expose a port"); + } + + try { + await expect( + pickBackendPorts({ backendPort: address.port, backendPortSource: "KANDEV_PORT" }), + ).rejects.toThrow(`Backend port ${address.port} from KANDEV_PORT is already in use`); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }); + + it("does not reuse an explicit backend port for the automatic web port", async (ctx) => { + if (!(await portsTesting.isPortAvailable(DEFAULT_WEB_PORT))) { + ctx.skip(); + return; + } + + const resolved = await pickPorts({ + backendPort: DEFAULT_WEB_PORT, + backendPortSource: "--port", + }); + + expect(resolved.webPort).not.toBe(DEFAULT_WEB_PORT); + }); + + it("does not reuse an explicit backend port for the automatic agentctl port", async (ctx) => { + if (!(await portsTesting.isPortAvailable(DEFAULT_AGENTCTL_PORT))) { + ctx.skip(); + return; + } + + const resolved = await pickBackendPorts({ + backendPort: DEFAULT_AGENTCTL_PORT, + backendPortSource: "--port", + }); + + expect(resolved.agentctlPort).not.toBe(DEFAULT_AGENTCTL_PORT); + }); +}); + describe("listHostNetworkAddresses", () => { afterEach(() => { vi.restoreAllMocks(); diff --git a/apps/cli/src/shared.ts b/apps/cli/src/shared.ts index 596bebc678..94a3451a10 100644 --- a/apps/cli/src/shared.ts +++ b/apps/cli/src/shared.ts @@ -6,10 +6,12 @@ */ import type { ChildProcess } from "node:child_process"; +import crypto from "node:crypto"; +import type { BackendPortSource } from "./args"; import os from "node:os"; import { DEFAULT_AGENTCTL_PORT, DEFAULT_BACKEND_PORT, DEFAULT_WEB_PORT } from "./constants"; -import { pickAvailablePort } from "./ports"; +import { assertPortAvailable, pickAvailablePortExcluding } from "./ports"; import { createProcessSupervisor } from "./process"; export type PortConfig = { @@ -19,17 +21,41 @@ export type PortConfig = { backendUrl: string; }; +export type PortSelection = { + backendPort?: number; + backendPortSource?: BackendPortSource; + webPort?: number; +}; + /** * Picks available ports for all services, using provided values or finding free ports. * - * @param backendPort - Optional preferred backend port - * @param webPort - Optional preferred web port + * @param options - Optional preferred service ports and backend-port source + * @param options.backendPort - Optional preferred backend port + * @param options.webPort - Optional preferred web port * @returns Resolved ports for all services */ -export async function pickPorts(backendPort?: number, webPort?: number): Promise { - const resolvedBackendPort = backendPort ?? (await pickAvailablePort(DEFAULT_BACKEND_PORT)); - const resolvedWebPort = webPort ?? (await pickAvailablePort(DEFAULT_WEB_PORT)); - const agentctlPort = await pickAvailablePort(DEFAULT_AGENTCTL_PORT); +export async function pickPorts({ + backendPort, + backendPortSource, + webPort, +}: PortSelection = {}): Promise { + if (backendPort !== undefined) { + await assertPortAvailable(backendPort, backendPortSource); + } + const selectedPorts = new Set(); + const resolvedBackendPort = + backendPort ?? (await pickAvailablePortExcluding(DEFAULT_BACKEND_PORT, selectedPorts)); + selectedPorts.add(resolvedBackendPort); + + const resolvedWebPort = + webPort ?? (await pickAvailablePortExcluding(DEFAULT_WEB_PORT, selectedPorts)); + if (selectedPorts.has(resolvedWebPort)) { + throw new Error(`Web port ${resolvedWebPort} conflicts with another selected service port`); + } + selectedPorts.add(resolvedWebPort); + + const agentctlPort = await pickAvailablePortExcluding(DEFAULT_AGENTCTL_PORT, selectedPorts); return { backendPort: resolvedBackendPort, @@ -39,9 +65,18 @@ export async function pickPorts(backendPort?: number, webPort?: number): Promise }; } -export async function pickBackendPorts(backendPort?: number): Promise { - const resolvedBackendPort = backendPort ?? (await pickAvailablePort(DEFAULT_BACKEND_PORT)); - const agentctlPort = await pickAvailablePort(DEFAULT_AGENTCTL_PORT); +export async function pickBackendPorts({ + backendPort, + backendPortSource, +}: Pick = {}): Promise { + if (backendPort !== undefined) { + await assertPortAvailable(backendPort, backendPortSource); + } + const selectedPorts = new Set(); + const resolvedBackendPort = + backendPort ?? (await pickAvailablePortExcluding(DEFAULT_BACKEND_PORT, selectedPorts)); + selectedPorts.add(resolvedBackendPort); + const agentctlPort = await pickAvailablePortExcluding(DEFAULT_AGENTCTL_PORT, selectedPorts); return { backendPort: resolvedBackendPort, @@ -60,6 +95,8 @@ export type BackendEnvOptions = { webProxy?: boolean; /** Additional environment variables to merge */ extra?: Record; + /** Health token owned by the launcher for this backend process */ + healthToken?: string; }; /** @@ -69,7 +106,14 @@ export type BackendEnvOptions = { * @returns Environment object for the backend process */ export function buildBackendEnv(options: BackendEnvOptions): NodeJS.ProcessEnv { - const { ports, logLevel, consoleLogLevel = "warn", webProxy = true, extra } = options; + const { + ports, + logLevel, + consoleLogLevel = "warn", + webProxy = true, + extra, + healthToken, + } = options; if (webProxy && ports.webPort === undefined) { throw new Error("webProxy requires a web port"); } @@ -81,6 +125,7 @@ export function buildBackendEnv(options: BackendEnvOptions): NodeJS.ProcessEnv { ...(logLevel ? { KANDEV_LOG_LEVEL: logLevel } : {}), KANDEV_CONSOLE_LOG_LEVEL: consoleLogLevel, ...extra, + KANDEV_DESKTOP_HEALTH_TOKEN: healthToken ?? crypto.randomBytes(32).toString("hex"), }; if (!webProxy) { delete env.KANDEV_WEB_INTERNAL_URL; diff --git a/apps/cli/src/start.ts b/apps/cli/src/start.ts index 02b3d13688..92a393de22 100644 --- a/apps/cli/src/start.ts +++ b/apps/cli/src/start.ts @@ -13,6 +13,7 @@ import fs from "node:fs"; import path from "node:path"; +import type { BackendPortSource } from "./args"; import { HEALTH_TIMEOUT_MS_RELEASE, resolveDataDir, @@ -37,6 +38,7 @@ export type StartOptions = { repoRoot: string; /** Optional preferred backend port (finds available port if not specified) */ backendPort?: number; + backendPortSource?: BackendPortSource; /** Show backend info logs on stdout */ verbose?: boolean; /** Retain backend debug logs in its file + agent message dumps */ @@ -60,11 +62,12 @@ export type StartOptions = { export async function runStart({ repoRoot, backendPort, + backendPortSource, verbose = false, debug = false, headless = false, }: StartOptions): Promise { - const ports = await pickBackendPorts(backendPort); + const ports = await pickBackendPorts({ backendPort, backendPortSource }); const backendBin = path.join(repoRoot, "apps", "backend", "bin", getBinaryName("kandev")); if (!fs.existsSync(backendBin)) { @@ -117,13 +120,19 @@ export async function runStart({ const healthTimeoutMs = resolveHealthTimeoutMs(HEALTH_TIMEOUT_MS_RELEASE); console.log("[kandev] starting backend..."); - await waitForHealth(ports.backendUrl, backend.proc, healthTimeoutMs, () => { - const buffered = output.read(); - if (buffered.trim().length === 0) return; - console.error("[kandev] --- backend stdout (last captured output) ---"); - console.error(buffered.trimEnd()); - console.error("[kandev] --- end backend stdout ---"); - }); + await waitForHealth( + ports.backendUrl, + backend.proc, + healthTimeoutMs, + backendEnv.KANDEV_DESKTOP_HEALTH_TOKEN, + () => { + const buffered = output.read(); + if (buffered.trim().length === 0) return; + console.error("[kandev] --- backend stdout (last captured output) ---"); + console.error(buffered.trimEnd()); + console.error("[kandev] --- end backend stdout ---"); + }, + ); console.log(`[kandev] backend ready at ${ports.backendUrl}`); console.log("[kandev] open: " + ports.backendUrl); diff --git a/apps/cli/src/supervisor/manifest.test.ts b/apps/cli/src/supervisor/manifest.test.ts index 90aead64e6..b485cd429c 100644 --- a/apps/cli/src/supervisor/manifest.test.ts +++ b/apps/cli/src/supervisor/manifest.test.ts @@ -28,6 +28,7 @@ describe("supervisor launch manifest", () => { env: { KANDEV_HOME_DIR: home, KANDEV_SERVER_PORT: "38429", + KANDEV_DESKTOP_HEALTH_TOKEN: "health-token", SECRET_TOKEN: "do-not-write", }, home_dir: home, @@ -43,6 +44,7 @@ describe("supervisor launch manifest", () => { expect(readLaunchManifest(target).env).toEqual({ KANDEV_HOME_DIR: home, KANDEV_SERVER_PORT: "38429", + KANDEV_DESKTOP_HEALTH_TOKEN: "health-token", }); if (process.platform !== "win32") { expect((fs.statSync(target).mode & 0o777).toString(8)).toBe("600"); @@ -72,12 +74,14 @@ describe("supervisor launch manifest", () => { allowedEnv({ KANDEV_HOME_DIR: "/tmp/kandev", KANDEV_CONSOLE_LOG_LEVEL: "warn", + KANDEV_DESKTOP_HEALTH_TOKEN: "health-token", AWS_SECRET_ACCESS_KEY: "secret", GITHUB_TOKEN: "secret", }), ).toEqual({ KANDEV_HOME_DIR: "/tmp/kandev", KANDEV_CONSOLE_LOG_LEVEL: "warn", + KANDEV_DESKTOP_HEALTH_TOKEN: "health-token", }); }); }); diff --git a/apps/cli/src/supervisor/manifest.ts b/apps/cli/src/supervisor/manifest.ts index 46b87db80a..81686d83eb 100644 --- a/apps/cli/src/supervisor/manifest.ts +++ b/apps/cli/src/supervisor/manifest.ts @@ -24,6 +24,7 @@ const ENV_ALLOWLIST = new Set([ "KANDEV_HOME_DIR", "KANDEV_DATABASE_PATH", "KANDEV_SERVER_PORT", + "KANDEV_DESKTOP_HEALTH_TOKEN", "KANDEV_WEB_INTERNAL_URL", "KANDEV_AGENT_STANDALONE_PORT", "KANDEV_LOG_LEVEL", diff --git a/docs/plans/port-collision-safety/plan.md b/docs/plans/port-collision-safety/plan.md new file mode 100644 index 0000000000..9689380de9 --- /dev/null +++ b/docs/plans/port-collision-safety/plan.md @@ -0,0 +1,214 @@ +--- +spec: docs/specs/port-collision-safety/spec.md +created: 2026-08-07 +status: complete +--- + +# Implementation Plan: Port collision safety and launcher readiness + +## Overview + +Repair the three reported startup/port bugs as one cross-launcher package. Add explicit backend +port preflight to the TypeScript and native Go launchers, make health polling prove ownership with +the existing per-launch token contract, and centralize address-in-use classification for agentctl +and websocket bindings. Keep PR #2368's PORT and WEB_PORT Makefile behavior unchanged. + +The package has no frontend or database work. The launcher health tests use mocked HTTP responses +and Go httptest servers; the port allocator tests use real loopback listeners so the Windows +error path is exercised rather than simulated with an English error string. + +## Root cause and design + +- CLI explicit ports bypass the availability probe in apps/cli/src/shared.ts, and the installed + Go launcher bypasses canBind for an explicit backend port in + apps/backend/internal/launcher/ports.go. +- Both health pollers currently accept any successful response from the target URL. The backend + already echoes KANDEV_DESKTOP_HEALTH_TOKEN in the + X-Kandev-Desktop-Health-Token response header, and the desktop launcher demonstrates the + per-launch ownership pattern. Reuse that contract and add the token to both supervisor + environment allowlists. +- The agentctl allocator compares only against syscall.EADDRINUSE and also has an English string + fallback. The websocket tunnel has a duplicate classifier with the same platform gap. A new + internal common helper will handle wrapped syscall errors and Windows WSAEADDRINUSE. + +## CLI launcher + +### Explicit port preflight + +- Carry the backend-port source selected by args.ts (CLI flag, KANDEV_BACKEND_PORT, or KANDEV_PORT) + alongside the numeric port into the shared port-picking path. +- Add a shared explicit-port assertion in apps/cli/src/ports.ts using the existing + isPortAvailable probe. Use it from pickPorts, pickBackendPorts, and the installed run path so + dev, start, and run have identical behavior. +- Preserve automatic fallback only for an omitted backend port. Report the source and port in a + stable error without changing PR #2368's Makefile flags. + +### Owned health readiness + +- Generate a fresh token with the Node crypto API once per CLI invocation and pass it through + buildBackendEnv. +- Extend waitForHealth to send the launcher's expected token context and require an exact matching + response header in addition to 2xx status. +- Add KANDEV_DESKTOP_HEALTH_TOKEN to the CLI supervisor manifest allowlist so backend restarts + keep the same launch identity. +- Keep tokens out of startup logs, timeout text, and captured diagnostics. Keep direct backend + health behavior unchanged when no expected token is supplied by a caller outside the launcher. + +## Native Go launcher + +### Explicit port preflight + +- Preserve the precedence order in resolvePorts while retaining the selected source for errors. +- Make pickPorts validate an explicit backend port with the existing canBind behavior and return a + source-aware error. Automatic backend selection keeps its current fallback loop. +- Apply the same result to both installed run and local start before their managed backend child is + launched. + +### Owned health readiness + +- Generate one cryptographically random token per managed launcher invocation, not a new token for + each supervisor restart. +- Add the token to backendEnv and to allowedSupervisorEnv, preserving it in the owner-only launch + manifest. +- Extend waitForHealth/probeHealth to require a matching + X-Kandev-Desktop-Health-Token response header while retaining the existing child-exit, + timeout, and cancellation errors. +- Add a route-level backendapp regression assertion that the existing health endpoint still echoes + the configured token; do not change the endpoint's behavior for tokenless direct callers. + +## Cross-platform bind errors + +- Add an internal common netutil helper with Unix and Windows platform files. Unwrap net.OpError, + os.SyscallError, and syscall errno values as needed; recognize + x/sys/windows.WSAEADDRINUSE on Windows. +- Replace the instance manager's syscall/string checks with the helper and leave its + MarkUnavailable/retry behavior intact. +- Replace the websocket tunnel's local helper with the same common helper and preserve its + existing user-facing occupied-port message. +- Add real double-bind tests and include the affected instance, tunnel, and helper packages in + the Windows-sensitive CI test command. Keep the Makefile test-windows target and workflow in + sync. + +## Public documentation + +Update only the user-facing statements that describe startup failure and Windows coverage: + +- docs/public/contributing.md +- docs/public/cli.md +- docs/public/remote-cloud-environment.md +- docs/public/windows-support.md + +Explain that an explicitly selected occupied port is rejected before launcher readiness and that +the launcher never silently substitutes another requested port. Do not document the internal +health-token variable as a user configuration knob. Leave the separate service-install +KANDEV_SERVER_PORT limitation unchanged. + +## Tests + +### CLI + +- Add unit coverage for source precedence and occupied explicit ports from --port, + KANDEV_BACKEND_PORT, and KANDEV_PORT. +- Add health tests for matching, missing, and mismatched response headers, including the + no-early-success guarantee. +- Add manifest/env tests proving the token is passed to the child and retained for supervisor + restarts without logging it. + +### Go launcher/backend + +- Add launcher health tests for matching, missing, and mismatched response headers, plus child + exit and timeout behavior. +- Add env/manifest tests proving the generated token is retained across restart. +- Add a backendapp health route test for the existing echo header. + +### Address-in-use handling + +- Add a netutil test that binds a real loopback listener, attempts a second bind, and verifies the + wrapped error is classified as address-in-use on the host platform. +- Add an instance allocator test that occupies the first configured candidate and verifies the + next candidate is returned. +- Keep non-address-in-use errors on the release/fail path and retain a focused websocket occupied + port assertion where the package test seams permit it. +- Run the affected packages on windows-latest and keep the local cross-compile checks available. + +No browser E2E test applies because this repair changes launcher/backend startup ownership, not +the SPA UI. + +## Implementation waves and parallel candidates + +Wave 1: + +- [x] [Task 01: CLI explicit port preflight](task-01-cli-explicit-port-preflight.md) +- [x] [Task 02: Native launcher port preflight](task-02-native-launcher-port-preflight.md) +- [x] [Task 05: Cross-platform address-in-use handling](task-05-cross-platform-address-in-use.md) + +These are parallel-safe candidates because they own separate source areas. The primary +conversation executes them sequentially unless the user explicitly authorizes subagents. + +Wave 2: + +- [x] [Task 03: CLI health ownership](task-03-cli-health-ownership.md), depends on Task 01. +- [x] [Task 04: Native health ownership](task-04-native-health-ownership.md), depends on Task 02. + +Tasks 03 and 04 are separate-language parallel-safe candidates after their respective preflight +contracts are stable; execute sequentially by default. + +Wave 3: + +- [x] [Task 06: Update launcher documentation](task-06-update-launcher-documentation.md), depends on + Tasks 01–05. + +## Risks and mitigations + +- A preflight probe can race with another process. Token-matched readiness prevents a stranger's + 2xx response from producing a false ready state; child exit still reports the actual bind + failure. +- The existing token name is desktop-specific. Reusing it avoids a backend contract migration; + renaming it is explicitly out of scope. +- Persisting the token in a supervisor manifest is necessary for restart ownership. The existing + manifest is owner-only; implementation must not add token values to logs or user-facing errors. +- Windows test packages include platform-sensitive fixtures. Keep the new regression narrow and + preserve existing skips for tests that cannot run under Windows ACL/socket rules. + +## Open questions + +None blocking. Exact error wording may follow each launcher’s current error style, provided the +port and selected source are present. + +## Verification Results + +Implementation is complete. The following checks passed: + +- `make -C apps/backend test` — all backend packages passed. +- `pnpm --filter kandev test` — 30 files and 297 CLI tests passed. +- `pnpm --filter kandev exec tsc --noEmit` — passed. +- `go test -race -tags fts5 ./internal/launcher ./internal/backendapp ./internal/common/netutil ./internal/agentctl/server/instance ./internal/gateway/websocket` — affected packages passed. +- Windows cross-compilation checks passed for netutil, instance, and websocket packages. +- `make -C apps/backend test-windows` — original Windows-sensitive packages plus targeted + netutil, instance allocator, and websocket tunnel tests passed. +- Public-doc tests passed (58 tests); `validate-public-docs.mjs` validated 41 pages. +- `git diff --check`, Go formatting, and Prettier checks passed. + +Reference commands from the repository root: + +~~~bash +make -C apps/backend test +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +~~~ + +From apps/ after installing the missing worktree dependencies when necessary: + +~~~bash +pnpm --filter kandev test -- src/args.test.ts src/ports.test.ts src/shared.test.ts src/health.test.ts src/supervisor/manifest.test.ts +~~~ + +For the focused Go loop: + +~~~bash +cd apps/backend +go test -tags fts5 ./internal/launcher ./internal/backendapp ./internal/common/netutil ./internal/agentctl/server/instance ./internal/gateway/websocket +GOOS=windows GOARCH=amd64 go test -c -o /tmp/kandev-instance-windows.test.exe ./internal/agentctl/server/instance +GOOS=windows GOARCH=amd64 go test -c -o /tmp/kandev-tunnel-windows.test.exe ./internal/gateway/websocket +~~~ diff --git a/docs/plans/port-collision-safety/task-01-cli-explicit-port-preflight.md b/docs/plans/port-collision-safety/task-01-cli-explicit-port-preflight.md new file mode 100644 index 0000000000..24466e5907 --- /dev/null +++ b/docs/plans/port-collision-safety/task-01-cli-explicit-port-preflight.md @@ -0,0 +1,82 @@ +--- +id: "01-cli-explicit-port-preflight" +title: "CLI explicit port preflight" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 01: CLI explicit port preflight + +## Acceptance + +- dev, start, and installed run reject an occupied explicitly selected backend port before + starting the backend child or opening the browser. +- The failure identifies the numeric port and whether it came from a CLI flag, + KANDEV_BACKEND_PORT, or KANDEV_PORT; an explicit port is never replaced with an automatic one. +- Omitted backend ports retain the current preferred-port and random-fallback behavior, and + make dev PORT/WEB_PORT wiring from PR #2368 remains unchanged. + +## Verification + +Use TDD: add failing cases for each source and for the automatic fallback, implement the shared +assertion, then rerun: + +~~~bash +cd apps +pnpm --filter kandev test -- src/args.test.ts src/ports.test.ts src/shared.test.ts +~~~ + +If the worktree has no dependencies: + +~~~bash +cd apps +pnpm install --frozen-lockfile +~~~ + +## Files likely touched + +- apps/cli/src/args.ts +- apps/cli/src/cli.ts +- apps/cli/src/ports.ts +- apps/cli/src/shared.ts +- apps/cli/src/dev.ts +- apps/cli/src/start.ts +- apps/cli/src/run.ts +- apps/cli/src/args.test.ts +- apps/cli/src/ports.test.ts +- apps/cli/src/shared.test.ts + +## Dependencies + +None. + +## Parallelism + +Parallel-safe candidate with Tasks 02 and 05; the primary conversation executes sequentially by +default. Task 03 owns the same CLI launch context and depends on this task. + +## Inputs + +- Spec sections: Explicit backend-port preflight and Issue #2370 scenarios. +- Plan sections: CLI launcher / Explicit port preflight and CLI tests. +- Existing helpers: resolvePorts, isPortAvailable, pickAvailablePort, pickPorts, and + pickBackendPorts. + +## Risks + +- Keep the source metadata aligned with the existing CLI-over-environment precedence. +- Do not turn a probe failure into a random fallback for an explicitly requested port. +- The preflight does not eliminate the bind race; Task 03 supplies the ownership proof. + +## Completion + +- Behavior: explicit CLI backend ports are preflighted and fail with the configuration source; + automatic web and agentctl selection excludes already selected service ports. +- Files: CLI argument, port-selection, launcher, and focused test files listed above. +- Verification: the focused CLI suite is green (`62 passed` in the fixup run); the original red + case was the missing preflight/collision behavior. +- Caveat: preflight remains a check immediately before launch, while health-token ownership covers + the remaining bind/readiness race. diff --git a/docs/plans/port-collision-safety/task-02-native-launcher-port-preflight.md b/docs/plans/port-collision-safety/task-02-native-launcher-port-preflight.md new file mode 100644 index 0000000000..e6ede0058c --- /dev/null +++ b/docs/plans/port-collision-safety/task-02-native-launcher-port-preflight.md @@ -0,0 +1,75 @@ +--- +id: "02-native-launcher-port-preflight" +title: "Native launcher port preflight" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 02: Native launcher port preflight + +## Acceptance + +- Native run and start reject an occupied explicitly selected backend port before launching the + managed backend child. +- The error identifies the numeric port and whether the value came from --port, + KANDEV_BACKEND_PORT, or KANDEV_PORT; an explicit port is never silently substituted. +- With no explicit backend port, the existing preferred-port and fallback selection remains + unchanged. + +## Verification + +Use TDD around resolvePorts, pickPorts, runInstalled, and runStart: + +~~~bash +cd apps/backend +go test -tags fts5 ./internal/launcher +~~~ + +Run the full backend target after the implementation wave: + +~~~bash +make -C apps/backend test +~~~ + +## Files likely touched + +- apps/backend/internal/launcher/ports.go +- apps/backend/internal/launcher/run.go +- apps/backend/internal/launcher/start.go +- apps/backend/internal/launcher/ports_test.go +- apps/backend/internal/launcher/start_test.go + +## Dependencies + +None. + +## Parallelism + +Parallel-safe candidate with Tasks 01 and 05; the primary conversation executes sequentially by +default. Task 04 owns the same native managed-launch context and depends on this task. + +## Inputs + +- Spec sections: Explicit backend-port preflight and Issue #2370 scenarios. +- Plan sections: Native Go launcher / Explicit port preflight and Go launcher tests. +- Existing helpers: resolvePorts, envPort, pickPorts, pickAvailablePortExcept, and canBind. + +## Risks + +- Preserve the current precedence and parse errors while adding source metadata. +- Validate before child launch without changing runtime-bundle resolution or service behavior outside + the reported run/start paths. +- The source-aware preflight must not claim ownership; Task 04 handles the remaining race. + +## Completion + +- Behavior: native run/start launchers preflight explicit backend ports and preserve their source + metadata in configuration errors. +- Files: native launcher port-resolution, run/start wiring, and launcher test files listed above. +- Verification: launcher race tests are green in the fixup run; the focused test first failed until + the source-aware preflight and collision wiring were implemented. +- Cross-platform: the Windows-specific bind classifier and targeted CI coverage are recorded in + Task 05; the preflight itself remains platform-neutral. diff --git a/docs/plans/port-collision-safety/task-03-cli-health-ownership.md b/docs/plans/port-collision-safety/task-03-cli-health-ownership.md new file mode 100644 index 0000000000..6694ada5e4 --- /dev/null +++ b/docs/plans/port-collision-safety/task-03-cli-health-ownership.md @@ -0,0 +1,80 @@ +--- +id: "03-cli-health-ownership" +title: "CLI health ownership" +status: done +wave: 2 +depends_on: ["01-cli-explicit-port-preflight"] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 03: CLI health ownership + +## Acceptance + +- Each CLI dev/start/run invocation generates a fresh token, passes it to the backend as + KANDEV_DESKTOP_HEALTH_TOKEN, and requires the matching + X-Kandev-Desktop-Health-Token response header before declaring readiness. +- A 2xx response with a missing or mismatched token never triggers the ready message or browser + open; normal child-exit and timeout diagnostics remain intact without printing token values. +- The token is allowlisted in the CLI supervisor manifest so a backend restart for the same + invocation remains recognized, while direct tokenless health callers retain current behavior. + +## Verification + +Use TDD for the HTTP and manifest cases: + +~~~bash +cd apps +pnpm --filter kandev test -- src/health.test.ts src/shared.test.ts src/supervisor/manifest.test.ts +~~~ + +Run the broader CLI focused set with Task 01: + +~~~bash +pnpm --filter kandev test -- src/args.test.ts src/ports.test.ts src/shared.test.ts src/health.test.ts src/supervisor/manifest.test.ts +~~~ + +## Files likely touched + +- apps/cli/src/health.ts +- apps/cli/src/shared.ts +- apps/cli/src/dev.ts +- apps/cli/src/start.ts +- apps/cli/src/run.ts +- apps/cli/src/supervisor/manifest.ts +- apps/cli/src/health.test.ts +- apps/cli/src/shared.test.ts +- apps/cli/src/supervisor/manifest.test.ts + +## Dependencies + +Task 01 provides the shared CLI launch context and source-aware preflight path. + +## Parallelism + +Sequential after Task 01. Parallel-safe with Task 04 after both native preflight contracts are +stable, but the primary conversation executes sequentially by default. + +## Inputs + +- Spec sections: Backend readiness ownership and Issue #2372 scenarios. +- Plan sections: CLI launcher / Owned health readiness and CLI tests. +- Existing contract: apps/backend/internal/backendapp/helpers.go and the desktop + KANDEV_DESKTOP_HEALTH_TOKEN response-header flow. + +## Risks + +- Generate once per launcher invocation, not on each poll or supervisor restart. +- Override any stale inherited token rather than trusting process environment state. +- Keep the token out of logs, errors, ring buffers, and tests' diagnostic output. + +## Completion + +- Behavior: each CLI launch owns one health token, passes it through the backend environment, and + requires an exact token match on a successful health response; the supervisor manifest retains + the allowlisted token. +- Files: shared CLI environment/health/supervisor and launcher test files listed above. +- Verification: CLI health, environment, manifest, and launch tests are green; the fixup adds + explicit-token coverage so token ownership is visible to future callers. +- Compatibility: direct backend health requests without an expected token remain supported. diff --git a/docs/plans/port-collision-safety/task-04-native-health-ownership.md b/docs/plans/port-collision-safety/task-04-native-health-ownership.md new file mode 100644 index 0000000000..0183fe1ae0 --- /dev/null +++ b/docs/plans/port-collision-safety/task-04-native-health-ownership.md @@ -0,0 +1,81 @@ +--- +id: "04-native-health-ownership" +title: "Native health ownership" +status: done +wave: 2 +depends_on: ["02-native-launcher-port-preflight"] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 04: Native health ownership + +## Acceptance + +- Each native run/start managed invocation generates one fresh token, passes it through + KANDEV_DESKTOP_HEALTH_TOKEN, and requires an exact matching + X-Kandev-Desktop-Health-Token response header before printing backend ready. +- Missing or mismatched headers do not satisfy waitForHealth, while child exit, timeout, and + cancellation behavior remains intact and token values never appear in diagnostics. +- The native supervisor manifest retains the token for backend restarts, and the existing backend + health endpoint still echoes configured tokens without changing tokenless direct health calls. + +## Verification + +Use TDD for launcher health, environment/manifest, and backend route coverage: + +~~~bash +cd apps/backend +go test -tags fts5 ./internal/launcher ./internal/backendapp +~~~ + +Run the complete backend verification after implementation: + +~~~bash +make -C apps/backend test +~~~ + +## Files likely touched + +- apps/backend/internal/launcher/health.go +- apps/backend/internal/launcher/env.go +- apps/backend/internal/launcher/run.go +- apps/backend/internal/launcher/start.go +- apps/backend/internal/launcher/supervisor.go +- apps/backend/internal/launcher/health_test.go +- apps/backend/internal/launcher/start_test.go +- apps/backend/internal/launcher/supervisor_test.go +- apps/backend/internal/backendapp/server_test.go + +## Dependencies + +Task 02 provides the native managed-launch context and explicit-port behavior. + +## Parallelism + +Sequential after Task 02. Parallel-safe with Task 03 after both preflight contracts are stable, +but the primary conversation executes sequentially by default. + +## Inputs + +- Spec sections: Backend readiness ownership and Issue #2372 scenarios. +- Plan sections: Native Go launcher / Owned health readiness and Go launcher/backend tests. +- Existing contract: backendapp health-token constants/echo behavior and the desktop launcher’s + per-launch token generation pattern. + +## Risks + +- Generate the token once before the first backend launch and reuse the same environment for + supervisor restarts. +- Keep the manifest allowlist narrow and preserve owner-only manifest permissions. +- Do not require a token from unrelated direct backend health requests. + +## Completion + +- Behavior: native launches generate one token before the first backend process, require the exact + token on health success, and preserve it through supervisor restarts. +- Files: native launcher environment/health/supervisor, backend health handler, and test files + listed above. +- Verification: launcher/backend race tests are green; the fixup adds coverage that cancellation + invokes the health failure callback exactly once. +- Cross-platform: token matching uses ordinary HTTP headers and the existing manifest allowlist. diff --git a/docs/plans/port-collision-safety/task-05-cross-platform-address-in-use.md b/docs/plans/port-collision-safety/task-05-cross-platform-address-in-use.md new file mode 100644 index 0000000000..9a9cd38263 --- /dev/null +++ b/docs/plans/port-collision-safety/task-05-cross-platform-address-in-use.md @@ -0,0 +1,85 @@ +--- +id: "05-cross-platform-address-in-use" +title: "Cross-platform address-in-use handling" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 05: Cross-platform address-in-use handling + +## Acceptance + +- A shared internal helper recognizes wrapped Unix address-in-use errors and Windows + WSAEADDRINUSE (10048), without relying on localized English error text. +- The agentctl instance allocator marks an occupied candidate unavailable and retries the next + candidate on Windows; non-address-in-use errors still release and fail. +- The websocket tunnel uses the shared classifier and preserves its clear occupied-port error. + Real double-bind regression coverage runs in the Windows-sensitive CI job. + +## Verification + +Use TDD with a real loopback double bind and the allocator retry path: + +~~~bash +cd apps/backend +go test -tags fts5 ./internal/common/netutil ./internal/agentctl/server/instance ./internal/gateway/websocket +~~~ + +Build-check the Windows variants from a non-Windows host: + +~~~bash +GOOS=windows GOARCH=amd64 go test -c -o /tmp/kandev-netutil-windows.test.exe ./internal/common/netutil +GOOS=windows GOARCH=amd64 go test -c -o /tmp/kandev-instance-windows.test.exe ./internal/agentctl/server/instance +GOOS=windows GOARCH=amd64 go test -c -o /tmp/kandev-tunnel-windows.test.exe ./internal/gateway/websocket +~~~ + +The windows-latest job must run the real tests, not only compile them. + +## Files likely touched + +- apps/backend/internal/common/netutil/addr_in_use.go +- apps/backend/internal/common/netutil/addr_in_use_unix.go +- apps/backend/internal/common/netutil/addr_in_use_windows.go +- apps/backend/internal/common/netutil/addr_in_use_test.go +- apps/backend/internal/agentctl/server/instance/manager.go +- apps/backend/internal/agentctl/server/instance/manager_ports_test.go +- apps/backend/internal/gateway/websocket/port_tunnel.go +- apps/backend/internal/gateway/websocket/port_tunnel_test.go +- apps/backend/Makefile +- .github/workflows/backend-tests.yml + +## Dependencies + +None. + +## Parallelism + +Parallel-safe candidate with Tasks 01 and 02; the primary conversation executes sequentially by +default. It does not share implementation files with the launcher preflight/health tasks. + +## Inputs + +- Spec sections: Windows address-in-use handling and Issue #2371 scenarios. +- Plan sections: Cross-platform bind errors and Address-in-use handling tests. +- Existing dependencies: golang.org/x/sys already exists in apps/backend/go.mod. + +## Risks + +- Preserve the error-unwrapping behavior for net.OpError and os.SyscallError on every supported OS. +- Do not use a Windows-only import from a non-Windows build; isolate x/sys/windows behind build + tags. +- Keep port allocator release/MarkUnavailable semantics unchanged for non-collision failures. +- Keep the Makefile test-windows target and GitHub Windows job package lists aligned. + +## Completion + +- Behavior: shared Unix/Windows address-in-use classification drives allocator retry and tunnel + errors without string matching. +- Files: netutil classifier, allocator/tunnel callers and tests, plus Windows Makefile/CI scope. +- Verification: real double-bind and occupied-tunnel tests pass under the race detector; the + Windows job now runs the new collision tests separately from the incompatible broad MCP fixture. +- Platform note: the full instance package remains out of the Windows-sensitive broad list because + its stdio fixture expects a Unix command; targeted port tests still run on Windows. diff --git a/docs/plans/port-collision-safety/task-06-update-launcher-documentation.md b/docs/plans/port-collision-safety/task-06-update-launcher-documentation.md new file mode 100644 index 0000000000..690f52173c --- /dev/null +++ b/docs/plans/port-collision-safety/task-06-update-launcher-documentation.md @@ -0,0 +1,66 @@ +--- +id: "06-update-launcher-documentation" +title: "Update launcher documentation" +status: done +wave: 3 +depends_on: ["01-cli-explicit-port-preflight", "02-native-launcher-port-preflight", "03-cli-health-ownership", "04-native-health-ownership", "05-cross-platform-address-in-use"] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 06: Update launcher documentation + +## Acceptance + +- Public contributor, CLI, remote-cloud, and Windows-support docs describe that an occupied + explicitly selected port is rejected before readiness and is never silently replaced. +- Documentation does not expose KANDEV_DESKTOP_HEALTH_TOKEN as a user configuration knob. +- The separate service-install KANDEV_SERVER_PORT limitation remains documented accurately and is + not conflated with this repair. + +## Verification + +Update the smallest affected passages, then run: + +~~~bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +~~~ + +## Files likely touched + +- docs/public/contributing.md +- docs/public/cli.md +- docs/public/remote-cloud-environment.md +- docs/public/windows-support.md + +## Dependencies + +Tasks 01–05 establish the final launcher behavior and CI coverage that the public text must +describe. + +## Parallelism + +Sequential documentation pass after the implementation tasks. It is file-disjoint from source +work but intentionally waits for the final behavior wording. + +## Inputs + +- Spec sections: Explicit backend-port preflight, Windows address-in-use handling, and Out of + scope. +- Plan section: Public documentation. +- Existing public pages identified by the docs-maintainer review. + +## Risks + +- Avoid promising a race-free port reservation; describe the launcher ownership/readiness check + without exposing implementation details. +- Do not claim the service installer’s separate KANDEV_SERVER_PORT behavior was repaired. + +## Completion + +- Pages: contributing, CLI, remote-cloud, and Windows-support public guides were updated. +- Wording: explicit `run`/`start`/development port failures and Windows retry behavior are + documented, while the service installer’s separate port semantics remain explicitly out of + scope. +- Verification: public-doc validators pass, including the final CLI scope clarification. diff --git a/docs/public/cli.md b/docs/public/cli.md index 426aa0f332..36b565f002 100644 --- a/docs/public/cli.md +++ b/docs/public/cli.md @@ -222,7 +222,7 @@ Supported actions are `install`, `uninstall`, `start`, `stop`, `restart`, `statu | Backend (UI, HTTP, WebSocket, MCP) | `38429` | If no port was requested and this port cannot bind on loopback, try up to 10 random ports in `10000`-`60000`. | | Core `agentctl` | `39429` | Uses the same automatic fallback strategy and never shares the backend port. | -There is no separate web-server port in an installed release: the backend serves embedded assets. If `--port`, `KANDEV_BACKEND_PORT`, or `KANDEV_PORT` specifies a port, the launcher does not substitute another one; backend startup fails if the configured listen address cannot bind it. +There is no separate web-server port in an installed release: the backend serves embedded assets. For the `run`, `start`, and development launcher flows, if `--port`, `KANDEV_BACKEND_PORT`, or `KANDEV_PORT` specifies a port, the launcher checks it before declaring the backend ready, does not substitute another one, and fails startup if the configured listen address cannot bind it. `kandev service install --port` remains the separate installer behavior described above. The launcher prints `http://localhost:`, but the backend's default `server.host` is `0.0.0.0`. That can expose Kandev to other machines on the network, and the current local product path is not an authenticated multi-user boundary. Bind it to loopback unless remote access is deliberately protected: diff --git a/docs/public/contributing.md b/docs/public/contributing.md index abd03d59a2..e0b17b8ffd 100644 --- a/docs/public/contributing.md +++ b/docs/public/contributing.md @@ -34,7 +34,7 @@ make dev This is the normal development path. The TypeScript supervisor starts the Go backend and Vite, selects available ports, points Go at Vite, and isolates application state under the checkout's `.kandev-dev/`. Use the printed URLs. Backend logs append to `.kandev-dev/logs/backend-logs.log`; startup prints the resolved path. -Automatic port selection only applies when no port was requested, and `KANDEV_BACKEND_PORT` or `KANDEV_PORT` in the environment counts as a request. An installed Kandev service that exported one therefore pins development mode to the port its own backend already occupies, and the development backend fails to listen. Pass `PORT=` to override both the environment and the automatic choice, `WEB_PORT=` for the internal Vite port, and `DEV_ARGS=` for any other launcher flag. +Automatic port selection only applies when no port was requested, and `KANDEV_BACKEND_PORT` or `KANDEV_PORT` in the environment counts as a request. An installed Kandev service that exported one therefore pins development mode to the port its own backend already occupies; the launcher rejects that request before readiness and never silently substitutes another requested port. Pass `PORT=` to override both the environment and the automatic choice, `WEB_PORT=` for the internal Vite port, and `DEV_ARGS=` for any other launcher flag. ```bash make dev PORT=38430 WEB_PORT=37430 diff --git a/docs/public/remote-cloud-environment.md b/docs/public/remote-cloud-environment.md index 8cc8b5ece3..d8cfe8bae2 100644 --- a/docs/public/remote-cloud-environment.md +++ b/docs/public/remote-cloud-environment.md @@ -110,7 +110,7 @@ make dev - the Vite development server, preferring port `37429`; - standalone `agentctl`, preferring port `39429` when needed. -Occupied preferred ports are replaced with available random ports. Read the `[kandev] url:` line rather than assuming `38429`. Open or forward the **backend** URL: it proxies the Vite page and carries the same-origin HTTP and WebSocket traffic. The Vite port is internal development plumbing and normally does not need a provider public-port rule. +When no port is explicitly selected, occupied preferred ports are replaced with available random ports. Explicitly selected occupied ports are rejected before readiness and are never silently replaced. Read the `[kandev] url:` line rather than assuming `38429`. Open or forward the **backend** URL: it proxies the Vite page and carries the same-origin HTTP and WebSocket traffic. The Vite port is internal development plumbing and normally does not need a provider public-port rule. To request fixed preferred ports: diff --git a/docs/public/windows-support.md b/docs/public/windows-support.md index 29e84497f0..4ecfe2cb54 100644 --- a/docs/public/windows-support.md +++ b/docs/public/windows-support.md @@ -202,7 +202,7 @@ winget install ezwinports.make The repository bootstrap script is Bash/package-manager oriented and does not provision a native Windows toolchain automatically. Use Git Bash or another compatible shell for Unix-oriented root recipes, install the pinned tools manually, and follow the contributor guide. `make dev` builds a `winjob.exe` helper so `Ctrl+C` can close a native development process tree. -Backend Windows CI runs `go build ./...`, `go vet ./...`, and focused race tests for Windows-sensitive process and agent-launcher packages. It does not run every backend package or the full product E2E suite. The repository's broader Windows-clean target adds web and CLI unit tests: +Backend Windows CI runs `go build ./...`, `go vet ./...`, and focused race tests for Windows-sensitive process, agent-launcher, instance-port, and websocket-tunnel packages. It does not run every backend package or the full product E2E suite. The repository's broader Windows-clean target adds web and CLI unit tests: ```powershell make test-windows diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index c28c9aa96c..0a08834fe8 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -225,6 +225,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [homebrew-core](homebrew-core/spec.md) | building | | [native-kandev-cli](native-kandev-cli/spec.md) | draft | | [desktop-tauri-app](desktop-tauri-app/spec.md) | shipped | +| [port-collision-safety](port-collision-safety/spec.md) | building | | [lsp-file-intelligence](lsp-file-intelligence/spec.md) | building | | [public-share-links](public-share-links/spec.md) | draft | | [ssh-executor](ssh-executor/spec.md) | draft | diff --git a/docs/specs/port-collision-safety/spec.md b/docs/specs/port-collision-safety/spec.md new file mode 100644 index 0000000000..8400758c2d --- /dev/null +++ b/docs/specs/port-collision-safety/spec.md @@ -0,0 +1,123 @@ +--- +status: building +created: 2026-08-07 +--- + +# Port collision safety and launcher readiness + +## Why + +Kandev can currently start against the wrong backend when an explicitly requested port is +already occupied. The TypeScript development launcher and the native Go launcher accept a +successful health response from any process on that port, so a second Kandev instance can be +reported as ready while the newly launched backend is still failing to bind. That can open the +wrong SQLite database and make the failure look like a successful startup. + +On Windows, the agentctl instance allocator has a separate failure: the operating system reports +WSAEADDRINUSE (10048), which does not match the synthetic Go syscall.EADDRINUSE value used by the +current retry check. The allocator therefore stops instead of trying the next port. + +This repair covers GitHub issues +[#2370](https://github.com/kdlbs/kandev/issues/2370), +[#2372](https://github.com/kdlbs/kandev/issues/2372), and +[#2371](https://github.com/kdlbs/kandev/issues/2371). PR +[#2368](https://github.com/kdlbs/kandev/pull/2368) remains the correct Makefile wiring for +passing PORT and WEB_PORT, but it does not provide the safety checks described here. + +## What + +### Explicit backend-port preflight + +The TypeScript and native Go launchers must probe an explicitly configured backend port before +starting their backend child: + +- Explicit values include the CLI port flags, KANDEV_BACKEND_PORT, KANDEV_PORT, and the Makefile + PORT value after PR #2368 translates it to a CLI flag. +- An occupied explicit port is a hard startup error. The error names the numeric port and the + configuration source, and does not silently select a different backend port. +- A free explicit port continues through the existing launch path. +- When no backend port is configured, the existing automatic preferred-port and fallback-port + selection remains unchanged. +- The preflight is a race-reduction measure, not the ownership proof: a port can still be taken + between the probe and the child bind. Readiness ownership below closes that remaining race. + +### Backend readiness ownership + +Every TypeScript or native Go launcher invocation must create a fresh opaque health token before +starting its backend. It passes the token to the child through the existing +KANDEV_DESKTOP_HEALTH_TOKEN environment variable and retains it for supervisor-managed backend +restarts. + +The launcher health poll succeeds only when the response is a 2xx response and its +X-Kandev-Desktop-Health-Token response header exactly matches the token generated for that +invocation. A 2xx response without the header or with a different value is not readiness from the +launched backend; polling continues until the child exits or the normal timeout is reached. + +The existing backend health route and desktop token names are reused. Direct backend health +requests without a launcher-supplied token remain compatible with the current route behavior. +The token is not printed in startup output or failure diagnostics. The existing owner-only +supervisor manifest is allowed to carry the launch environment so an intentional backend restart +continues to answer with the same token. + +### Windows address-in-use handling + +The agentctl instance allocator and websocket tunnel must use one shared cross-platform +address-in-use classifier. It must recognize wrapped address-in-use errors on Unix and both the +Go syscall value and x/sys/windows.WSAEADDRINUSE on Windows. + +- The instance allocator marks an occupied candidate unavailable and retries the next candidate. +- The websocket tunnel retains its existing user-facing “port is already in use” error. +- Non-address-in-use bind errors still release the candidate and fail immediately. +- String matching on the English error text is not part of the contract. + +## Scenarios + +### Issue #2370: explicit port collisions + +1. Given a free CLI or environment-selected backend port, when dev, start, or run launches, + then the requested port is used and the normal readiness flow follows. +2. Given another process already owns an explicitly selected port, when the launcher starts, then + it exits non-zero before starting the backend child or opening the browser, and the message + includes the port and source. +3. Given no explicit backend port and the preferred port is occupied, when the launcher starts, + then it chooses an available fallback as it does today. + +### Issue #2372: readiness from the wrong process + +1. Given a stranger responds 2xx without the expected token, when the launcher polls health, then + it does not announce readiness or open the browser. +2. Given a stranger responds 2xx with a mismatched token, when the launcher polls health, then it + continues polling and does not treat that response as success. +3. Given the launched backend responds 2xx with the matching token, when the launcher polls + health, then it announces readiness exactly once. +4. Given the supervisor restarts the backend for the same launcher invocation, when the restarted + backend responds with the retained token, then health succeeds without accepting a stranger. + +### Issue #2371: Windows allocator retry + +1. Given the first agentctl instance candidate is occupied on Windows, when an instance is + allocated, then the allocator marks that candidate unavailable and binds the next candidate. +2. Given every candidate is occupied, when an instance is allocated, then the allocator returns + its existing exhaustion error after releasing candidates correctly. +3. Given a tunnel port is occupied on Windows, when a tunnel is requested, then the caller gets + the existing clear “port is already in use” error. + +## Out of scope + +- Changing default ports, fallback ranges, or automatic-port selection policy. +- Choosing a different port when the user explicitly requested one. +- PID or process-tree identity matching; the launcher wrapper chain makes that unreliable for + dev mode. +- Changing the health response body, the backend health status semantics, or the desktop + WebView flow. +- Renaming KANDEV_DESKTOP_HEALTH_TOKEN to a neutral variable; that would be a separate contract + migration. +- Changing service-install handling of KANDEV_SERVER_PORT, which is a separate installer issue. +- UI changes, database migrations, or new public authentication semantics. + +## Contract notes + +The existing desktop health-token contract in +docs/specs/desktop-tauri-app/spec.md is the authority for the environment variable and response +header. This repair extends its use to CLI/native launcher ownership checks without changing the +backend route contract, so a new architecture decision record is not required.