diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 5fca0b8492..69053796f3 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -180,7 +180,7 @@ Standalone agentctl is launched in its own process group so terminal Ctrl+C is h - **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment. Subscription actions (`*.subscribe`) `return` before reaching the dispatch backstop (`Client.authorizeAction`), so scoping a new subscribe topic means adding a hook to `SubscriptionAccessPolicy` (`internal/gateway/websocket/access.go`) *and* calling it explicitly inside that action's own handler — the backstop's `scopedActionRefs` cannot reach it. `run.subscribe`'s `Subscriptions.Run` hook (added for WO-02, resolving the run's workspace via `runs.GetRunWorkspaceID`) is the worked example. - **Task overview vs. session detail:** Keep cross-task state in the bounded, persisted `TaskStatusSummary` projection and `task.status_summary.updated`; keep unbounded transcript and execution data session-scoped. The [bounded task-status spec](../../docs/specs/platform/requirements/bounded-task-status-delivery.md) and [accepted ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md) define the shared contract. - **Port-proxy subtree capability:** with auth enforced, `/port-proxy/:sessionId/:port/*` needs a credential on *every* request, but browser subresource fetches cannot always carry one (the `` fetch never sends cookies; sandboxed iframes and `?token=`-opened previews drop them elsewhere). `PortProxyHandler` mints a short-lived (15 min, sliding), path-scoped, HMAC-signed capability after the document authenticates and propagates it as a cookie scoped to the proxy subtree plus a `kandev_cap` query parameter appended to rewritten asset URLs. `requireConnectionAuth` (`internal/gateway/websocket/access.go`) accepts either form only for the exact session:port it was minted for and restores the issuing identity, so `CheckSessionAccess` still gates on the real owner. Synthetic identities (auth disabled) skip minting — proxied bodies stay byte-identical to the pre-auth behavior. -- **Self-authenticating webhooks** (automation and office channels) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. Plugin webhooks are not allowlisted: `auth/httpmw` structurally defers only GET/POST relay paths because it cannot read manifests, and `internal/plugins.Controller.webhook` enforces each manifest's `access` policy. +- **Self-authenticating webhooks** (automation and office channels) and `/health`, `/ready`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. Plugin webhooks are not allowlisted: `auth/httpmw` structurally defers only GET/POST relay paths because it cannot read manifests, and `internal/plugins.Controller.webhook` enforces each manifest's `access` policy. ## Execution Flow diff --git a/apps/backend/cmd/preview/sprite_ops.go b/apps/backend/cmd/preview/sprite_ops.go index 900392fe24..5cbd53213a 100644 --- a/apps/backend/cmd/preview/sprite_ops.go +++ b/apps/backend/cmd/preview/sprite_ops.go @@ -297,25 +297,35 @@ func waitForServiceStarted(stream *sprites.ServiceStream) error { } } -// waitForKandev polls the kandev /health endpoint inside the sprite via -// CommandContext until kandev responds or the deadline is exceeded. -// Using the internal address (localhost) avoids Sprites routing state that -// may lag during a service restart triggered by enablePublicURL. +// kandevReadyURL returns the readiness endpoint waitForKandev polls, broken +// out so tests can pin the endpoint without going through the sprite exec/ +// websocket machinery. /health only proves the bootstrap listener has bound +// the socket (it returns 200 immediately, before the real router is wired +// in); /ready is what flips once the real router is swapped in and requests +// stop getting the bootstrap handler's 503 "starting" response. +func kandevReadyURL(port int) string { + return fmt.Sprintf("http://localhost:%d/ready", port) +} + +// waitForKandev polls the kandev /ready endpoint inside the sprite via +// CommandContext until kandev is ready to serve real traffic or the deadline +// is exceeded. Using the internal address (localhost) avoids Sprites routing +// state that may lag during a service restart triggered by enablePublicURL. func waitForKandev(ctx context.Context, sprite *sprites.Sprite, port int) error { const ( timeout = 90 * time.Second retryWait = 3 * time.Second ) - healthURL := fmt.Sprintf("http://localhost:%d/health", port) + readyURL := kandevReadyURL(port) deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { checkCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - out, err := sprite.CommandContext(checkCtx, "curl", "-sf", healthURL).Output() + out, err := sprite.CommandContext(checkCtx, "curl", "-sf", readyURL).Output() cancel() if err == nil && len(out) > 0 { - fmt.Fprintf(os.Stderr, " kandev is healthy\n") + fmt.Fprintf(os.Stderr, " kandev is ready\n") return nil } @@ -326,9 +336,9 @@ func waitForKandev(ctx context.Context, sprite *sprites.Sprite, port int) error } } - // Health check timed out — fetch logs to help diagnose. + // Readiness check timed out — fetch logs to help diagnose. diag := fetchSpriteLogs(ctx, sprite) - return fmt.Errorf("kandev did not become healthy within %v\n%s", timeout, diag) + return fmt.Errorf("kandev did not become ready within %v\n%s", timeout, diag) } // fetchSpriteLogs reads log files from the sprite for failure diagnostics. diff --git a/apps/backend/cmd/preview/sprite_ops_test.go b/apps/backend/cmd/preview/sprite_ops_test.go index dd4e51e29d..dc2c7486de 100644 --- a/apps/backend/cmd/preview/sprite_ops_test.go +++ b/apps/backend/cmd/preview/sprite_ops_test.go @@ -176,3 +176,28 @@ func TestBuildExtractScript(t *testing.T) { t.Errorf("script should not launch the backend binary directly") } } + +// TestKandevReadyURLPollsReadyNotHealth is the regression test for Review +// round 3 finding R3-1: waitForKandev used to poll /health, which this +// branch redefined as an unconditional-200 liveness probe served by the +// bootstrap handler the instant the socket binds. That made preview-env CI +// (.github/workflows/preview-env.yml) declare the deploy ready — and post +// the PR comment with the URL — while every non-/health path, including the +// whole app, was still returning the bootstrap handler's 503 "starting". +// +// Expected pre-fix failure: kandevReadyURL did not exist (waitForKandev +// built the URL inline as ".../health"), so this fails to compile against +// the pre-fix code, and once inlined the URL would end in /health, not +// /ready. +func TestKandevReadyURLPollsReadyNotHealth(t *testing.T) { + got := kandevReadyURL(4173) + want := "http://localhost:4173/ready" + if got != want { + t.Fatalf("kandevReadyURL(4173) = %q, want %q", got, want) + } + if strings.Contains(got, "/health") { + t.Fatalf("kandevReadyURL must not poll /health — the bootstrap handler answers "+ + "200 there before the real router exists, so preview-env would declare "+ + "success while every other path still 503s: %q", got) + } +} diff --git a/apps/backend/internal/auth/httpmw/middleware.go b/apps/backend/internal/auth/httpmw/middleware.go index 9ab00ca116..0548a13acd 100644 --- a/apps/backend/internal/auth/httpmw/middleware.go +++ b/apps/backend/internal/auth/httpmw/middleware.go @@ -303,7 +303,11 @@ func BearerToken(r *http.Request) string { func isPublicPath(method, path string) bool { switch path { case "/health": - // CLI + desktop readiness probes poll before any session can exist. + // CLI + desktop liveness probes poll before any session can exist. + return method == http.MethodGet + case "/ready": + // Kubernetes readinessProbe and the e2e fixture poll before any + // session can exist, same as /health. return method == http.MethodGet case "/api/v1/features", "/api/v1/app-state": // SPA bootstrap reads; app-state returns the auth-aware boot payload diff --git a/apps/backend/internal/auth/httpmw/middleware_test.go b/apps/backend/internal/auth/httpmw/middleware_test.go index 53c30b8c2f..db9fc2809f 100644 --- a/apps/backend/internal/auth/httpmw/middleware_test.go +++ b/apps/backend/internal/auth/httpmw/middleware_test.go @@ -204,6 +204,7 @@ func TestEnabledModeAllowlistMatrix(t *testing.T) { blocked bool }{ {name: "health", method: http.MethodGet, path: "/health"}, + {name: "ready", method: http.MethodGet, path: "/ready"}, {name: "features", method: http.MethodGet, path: "/api/v1/features"}, {name: "app-state", method: http.MethodGet, path: "/api/v1/app-state"}, {name: "login", method: http.MethodPost, path: "/api/v1/auth/login"}, diff --git a/apps/backend/internal/backendapp/binds_before_startup_test.go b/apps/backend/internal/backendapp/binds_before_startup_test.go new file mode 100644 index 0000000000..91741741a9 --- /dev/null +++ b/apps/backend/internal/backendapp/binds_before_startup_test.go @@ -0,0 +1,263 @@ +package backendapp + +import ( + "encoding/json" + "errors" + "fmt" + "net" + "net/http" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/config" + "go.uber.org/goleak" +) + +// getJSON dials host:port+path over HTTP and decodes a JSON object body. +// Keep-alives are disabled so no idle client connection lingers for goleak. +func getJSON(host string, port int, path string) (int, map[string]any, error) { + return requestJSON(host, port, http.MethodGet, path) +} + +func requestJSON(host string, port int, method string, path string) (int, map[string]any, error) { + url := fmt.Sprintf("http://%s%s", net.JoinHostPort(host, fmt.Sprint(port)), path) + client := &http.Client{ + Timeout: 500 * time.Millisecond, + Transport: &http.Transport{DisableKeepAlives: true}, + } + defer client.CloseIdleConnections() + req, err := http.NewRequest(method, url, nil) + if err != nil { + return 0, nil, err + } + resp, err := client.Do(req) + if err != nil { + return 0, nil, err + } + defer func() { _ = resp.Body.Close() }() + var body map[string]any + if decodeErr := json.NewDecoder(resp.Body).Decode(&body); decodeErr != nil { + return resp.StatusCode, nil, decodeErr + } + return resp.StatusCode, body, nil +} + +// TestBindBootstrapListenersServesLivenessBeforeSwap is the regression test +// for docs/plans/startup-listener-before-recovery/task-01-bind-before- +// startup.md: a caller must be able to bind the socket and get a 200 on the +// launcher's liveness path (GET /health) before the real router exists. +// +// Expected pre-fix failure: bindBootstrapListeners does not exist yet, so +// this fails to compile. +func TestBindBootstrapListenersServesLivenessBeforeSwap(t *testing.T) { + defer goleak.VerifyNone(t) + + cfg := &config.Config{} + cfg.Server.Host = "127.0.0.1" + cfg.Server.Port = freePort(t) + log := testLogger(t) + + handler, server, listeners, err := bindBootstrapListeners(cfg, log, "1.2.3-test") + if err != nil { + t.Fatalf("bindBootstrapListeners: %v", err) + } + defer shutdown(t, listeners, server) + + status, body, err := getJSON("127.0.0.1", cfg.Server.Port, "/health") + if err != nil { + t.Fatalf("GET /health before swap: %v", err) + } + if status != http.StatusOK { + t.Fatalf("bootstrap /health status = %d, want %d (launcher liveness probe requires 200)", status, http.StatusOK) + } + if got := body[versionFieldKey]; got != "1.2.3-test" { + t.Fatalf("bootstrap /health version = %v, want 1.2.3-test", got) + } + if got := body[statusKey]; got != "ok" { + t.Fatalf("bootstrap /health status field = %v, want %q (must match healthHandler's contract)", got, "ok") + } + if got := body["mode"]; got != "websocket+http" { + t.Fatalf("bootstrap /health mode = %v, want %q (must match healthHandler's contract)", got, "websocket+http") + } + + status, body, err = requestJSON("127.0.0.1", cfg.Server.Port, http.MethodPost, "/health") + if err != nil { + t.Fatalf("POST /health before swap: %v", err) + } + if status != http.StatusServiceUnavailable { + t.Fatalf("bootstrap POST /health status = %d, want %d", status, http.StatusServiceUnavailable) + } + if body[statusKey] != startingStatus { + t.Fatalf("bootstrap POST /health body = %v, want status=%q", body, startingStatus) + } + + // Every other path must answer deterministically, not hang or 404. + status, body, err = getJSON("127.0.0.1", cfg.Server.Port, "/api/v1/whatever") + if err != nil { + t.Fatalf("GET non-health path before swap: %v", err) + } + if status != http.StatusServiceUnavailable { + t.Fatalf("bootstrap non-health status = %d, want %d", status, http.StatusServiceUnavailable) + } + if body["status"] != startingStatus { + t.Fatalf("bootstrap non-health body = %v, want status=%q", body, startingStatus) + } + + if handler == nil { + t.Fatal("bindBootstrapListeners returned a nil handler switch") + } +} + +// TestBindBootstrapListenersEchoesDesktopHealthToken guards against the +// bootstrap handler poisoning the launcher's health probe: probeHealth +// (internal/launcher/health.go) treats a 200 with a missing/mismatched +// X-Kandev-Desktop-Health-Token as a foreign process and permanently sticks +// that verdict for the whole wait window. +func TestBindBootstrapListenersEchoesDesktopHealthToken(t *testing.T) { + defer goleak.VerifyNone(t) + t.Setenv(desktopHealthTokenEnv, "bootstrap-token") + + cfg := &config.Config{} + cfg.Server.Host = "127.0.0.1" + cfg.Server.Port = freePort(t) + log := testLogger(t) + + _, server, listeners, err := bindBootstrapListeners(cfg, log, "1.2.3-test") + if err != nil { + t.Fatalf("bindBootstrapListeners: %v", err) + } + defer shutdown(t, listeners, server) + + url := fmt.Sprintf("http://%s/health", net.JoinHostPort("127.0.0.1", fmt.Sprint(cfg.Server.Port))) + client := &http.Client{Timeout: 500 * time.Millisecond, Transport: &http.Transport{DisableKeepAlives: true}} + defer client.CloseIdleConnections() + resp, err := client.Get(url) + if err != nil { + t.Fatalf("GET /health: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if got := resp.Header.Get(desktopHealthTokenHeader); got != "bootstrap-token" { + t.Fatalf("bootstrap /health token header = %q, want bootstrap-token", got) + } +} + +// TestHandlerSwitchSwapsWithoutRebind is the regression test for the "no +// window where the socket is closed and reopened" acceptance criterion: the +// same listeners must serve the bootstrap handler and then the real handler +// with no gap and no second bind. +func TestHandlerSwitchSwapsWithoutRebind(t *testing.T) { + defer goleak.VerifyNone(t) + + cfg := &config.Config{} + cfg.Server.Host = "127.0.0.1" + cfg.Server.Port = freePort(t) + log := testLogger(t) + + handler, server, listeners, err := bindBootstrapListeners(cfg, log, "1.2.3-test") + if err != nil { + t.Fatalf("bindBootstrapListeners: %v", err) + } + defer shutdown(t, listeners, server) + + boundBefore := listeners.boundAddrs() + + status, _, err := getJSON("127.0.0.1", cfg.Server.Port, "/api/v1/whatever") + if err != nil { + t.Fatalf("GET before swap: %v", err) + } + if status != http.StatusServiceUnavailable { + t.Fatalf("status before swap = %d, want %d", status, http.StatusServiceUnavailable) + } + + handler.Store(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + status, err = getStatus("127.0.0.1", cfg.Server.Port) + if err != nil { + t.Fatalf("GET after swap: %v", err) + } + if status != http.StatusOK { + t.Fatalf("status after swap = %d, want %d", status, http.StatusOK) + } + + if got := listeners.boundAddrs(); len(got) != len(boundBefore) || got[0] != boundBefore[0] { + t.Fatalf("bound addresses changed across swap: before=%v after=%v (want no rebind)", boundBefore, got) + } +} + +// TestBindBootstrapListenersFailsWhenPortUnavailable pins today's bind- +// failure behaviour: bindBootstrapListeners returns errServerBindFailed +// (already logged by startHTTPServers) rather than a generic error, so the +// caller can skip the misleading "Failed to start orchestrator" log. +func TestBindBootstrapListenersFailsWhenPortUnavailable(t *testing.T) { + defer goleak.VerifyNone(t) + + blocker := listenOnFreePort(t) + t.Cleanup(func() { _ = blocker.Close() }) + port := listenerPort(t, blocker) + + cfg := &config.Config{} + cfg.Server.Host = "127.0.0.1" + cfg.Server.Port = port + log := testLogger(t) + + handler, server, listeners, err := bindBootstrapListeners(cfg, log, "1.2.3-test") + if !errors.Is(err, errServerBindFailed) { + t.Fatalf("bindBootstrapListeners error = %v, want %v", err, errServerBindFailed) + } + if handler != nil || server != nil || listeners != nil { + t.Fatalf("bindBootstrapListeners returned non-nil results on bind failure: handler=%v server=%v listeners=%v", + handler, server, listeners) + } +} + +// TestCloseBoundListenersReleasesPortOnStartupFailure is the regression test +// for the listener-leak flagged in Build round 3 review: a step that fails +// after bindBootstrapListeners already bound the socket (orchestrator start, +// office services, storage composition, buildHTTPServer) must release the +// port before returning, not just stop the background bind-retry loop. +func TestCloseBoundListenersReleasesPortOnStartupFailure(t *testing.T) { + defer goleak.VerifyNone(t) + + cfg := &config.Config{} + cfg.Server.Host = "127.0.0.1" + cfg.Server.Port = freePort(t) + log := testLogger(t) + + _, server, listeners, err := bindBootstrapListeners(cfg, log, "1.2.3-test") + if err != nil { + t.Fatalf("bindBootstrapListeners: %v", err) + } + + closeBoundListeners(server, listeners, log) + + // The port must become free: a later startup failure that leaks the + // listener would leave a same-process retry (or even a fresh process + // racing a restart) unable to bind here. server.Close() closing the + // listener's fd and the OS releasing the address aren't guaranteed to be + // observable in the same instant, so poll briefly rather than asserting + // on the very next syscall. + deadline := time.Now().Add(2 * time.Second) + var lastErr error + for time.Now().Before(deadline) { + ln, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", fmt.Sprint(cfg.Server.Port))) + if err == nil { + if closeErr := ln.Close(); closeErr != nil { + t.Fatalf("close probe listener: %v", closeErr) + } + return + } + lastErr = err + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("port still held after closeBoundListeners: %v", lastErr) +} + +// TestCloseBoundListenersNoopWhenListenersNil covers the pure-bind-failure +// case: bindBootstrapListeners returns a nil listeners/server pair when the +// bind itself failed, so closeBoundListeners must not dereference server. +func TestCloseBoundListenersNoopWhenListenersNil(t *testing.T) { + closeBoundListeners(nil, nil, testLogger(t)) +} diff --git a/apps/backend/internal/backendapp/health_test.go b/apps/backend/internal/backendapp/health_test.go index 36886f9e18..24045373bb 100644 --- a/apps/backend/internal/backendapp/health_test.go +++ b/apps/backend/internal/backendapp/health_test.go @@ -14,7 +14,7 @@ import ( ) // setReadyForTest flips the package-level readiness flag consulted by -// healthHandler and restores its prior value on cleanup, so tests don't leak +// readyHandler and restores its prior value on cleanup, so tests don't leak // state into each other or into TestMain-driven suites. func setReadyForTest(t *testing.T, value bool) { t.Helper() @@ -23,94 +23,121 @@ func setReadyForTest(t *testing.T, value bool) { t.Cleanup(func() { ready.Store(prev) }) } -// TestHealthHandlerReadyBodyIncludesVersion covers AC-1..4: once ready, the -// handler returns 200 with a version key equal to the configured build -// version, alongside the unchanged status/service/mode fields, and no other -// keys. -func TestHealthHandlerReadyBodyIncludesVersion(t *testing.T) { +// TestHealthHandlerBodyIncludesVersionRegardlessOfReadiness covers AC-19/20 +// of the 2026-08-22 liveness/readiness-split amendment +// (docs/specs/health-endpoint-version/spec.md): /health is a pure liveness +// probe, so its 200 body (status/service/mode/version, no other keys) is +// identical whether or not the `ready` flag has flipped. The old ready/ +// starting two-shape split (superseded AC-1..7) now lives on readyHandler, +// covered by TestReadyHandlerBodyShapesByReadiness below. +func TestHealthHandlerBodyIncludesVersionRegardlessOfReadiness(t *testing.T) { gin.SetMode(gin.TestMode) - setReadyForTest(t, true) - router := gin.New() - router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + for _, readyState := range []bool{true, false} { + setReadyForTest(t, readyState) - recorder := httptest.NewRecorder() - router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + router := gin.New() + router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) - if recorder.Code != http.StatusOK { - t.Fatalf("status = %d, want %d", recorder.Code, http.StatusOK) - } - var body map[string]interface{} - if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { - t.Fatalf("decode body: %v", err) - } - if body[versionFieldKey] != "1.2.3" { - t.Fatalf("version = %v, want 1.2.3", body[versionFieldKey]) - } - if body["status"] != "ok" || body["service"] != kandevName || body["mode"] != "websocket+http" { - t.Fatalf("unexpected ready body: %#v", body) - } - if len(body) != 4 { - t.Fatalf("ready body keys = %#v, want exactly status/service/mode/version", body) + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + + if recorder.Code != http.StatusOK { + t.Fatalf("ready=%v: status = %d, want %d", readyState, recorder.Code, http.StatusOK) + } + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("ready=%v: decode body: %v", readyState, err) + } + if body[versionFieldKey] != "1.2.3" { + t.Fatalf("ready=%v: version = %v, want 1.2.3", readyState, body[versionFieldKey]) + } + if body["status"] != "ok" || body["service"] != kandevName || body["mode"] != "websocket+http" { + t.Fatalf("ready=%v: unexpected body: %#v", readyState, body) + } + if len(body) != 4 { + t.Fatalf("ready=%v: body keys = %#v, want exactly status/service/mode/version", readyState, body) + } } } -// TestHealthHandlerStartingBodyIncludesVersion covers AC-5..7: before ready, -// the handler still returns the version alongside status/service, with no -// other keys (mode is ready-path only). -func TestHealthHandlerStartingBodyIncludesVersion(t *testing.T) { +// TestReadyHandlerBodyShapesByReadiness covers AC-16..19: readyHandler now +// owns the two-shape ready/starting split that healthHandler used to have — +// 503 with status="starting" (no mode key) before ready, 200 with +// status="ok" (no mode key) once ready — always carrying version either way. +func TestReadyHandlerBodyShapesByReadiness(t *testing.T) { gin.SetMode(gin.TestMode) setReadyForTest(t, false) router := gin.New() - router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + router.GET("/ready", readyHandler(routeParams{version: "1.2.3"})) - recorder := httptest.NewRecorder() - router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/health", nil)) + startingRec := httptest.NewRecorder() + router.ServeHTTP(startingRec, httptest.NewRequest(http.MethodGet, "/ready", nil)) + if startingRec.Code != http.StatusServiceUnavailable { + t.Fatalf("starting status = %d, want %d", startingRec.Code, http.StatusServiceUnavailable) + } + var startingBody map[string]interface{} + if err := json.Unmarshal(startingRec.Body.Bytes(), &startingBody); err != nil { + t.Fatalf("decode starting body: %v", err) + } + if startingBody[versionFieldKey] != "1.2.3" { + t.Fatalf("starting version = %v, want 1.2.3", startingBody[versionFieldKey]) + } + if startingBody["status"] != startingStatus || startingBody["service"] != kandevName { + t.Fatalf("unexpected starting body: %#v", startingBody) + } + if len(startingBody) != 3 { + t.Fatalf("starting body keys = %#v, want exactly status/service/version", startingBody) + } - if recorder.Code != http.StatusServiceUnavailable { - t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) + setReadyForTest(t, true) + readyRec := httptest.NewRecorder() + router.ServeHTTP(readyRec, httptest.NewRequest(http.MethodGet, "/ready", nil)) + if readyRec.Code != http.StatusOK { + t.Fatalf("ready status = %d, want %d", readyRec.Code, http.StatusOK) } - var body map[string]interface{} - if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { - t.Fatalf("decode body: %v", err) + var readyBody map[string]interface{} + if err := json.Unmarshal(readyRec.Body.Bytes(), &readyBody); err != nil { + t.Fatalf("decode ready body: %v", err) } - if body[versionFieldKey] != "1.2.3" { - t.Fatalf("version = %v, want 1.2.3", body[versionFieldKey]) + if readyBody[versionFieldKey] != "1.2.3" { + t.Fatalf("ready version = %v, want 1.2.3", readyBody[versionFieldKey]) } - if body["status"] != startingStatus || body["service"] != kandevName { - t.Fatalf("unexpected starting body: %#v", body) + if readyBody["status"] != "ok" || readyBody["service"] != kandevName { + t.Fatalf("unexpected ready body: %#v", readyBody) } - if len(body) != 3 { - t.Fatalf("starting body keys = %#v, want exactly status/service/version", body) + if len(readyBody) != 3 { + t.Fatalf("ready body keys = %#v, want exactly status/service/version", readyBody) } } -// TestHealthHandlerDesktopTokenHeaderOnlyOnReadyPath guards a pre-existing -// behavior the spec restates as a must-not-regress ("## What": the desktop -// health-token header SHALL continue to be set on the 200 path only, -// unchanged) but that had no direct test before this change extracted the -// handler body into healthHandler. Confirms the refactor didn't move the -// header write outside its original branch. -func TestHealthHandlerDesktopTokenHeaderOnlyOnReadyPath(t *testing.T) { +// TestHealthHandlerDesktopTokenHeaderAlwaysSetReadyEndpointNever covers AC-21: +// the desktop health-token header is a /health-only concern, unaffected by +// the liveness/readiness split — /health sets it on every request regardless +// of readiness, and /ready never sets it at all. +func TestHealthHandlerDesktopTokenHeaderAlwaysSetReadyEndpointNever(t *testing.T) { gin.SetMode(gin.TestMode) t.Setenv(desktopHealthTokenEnv, " desktop-token ") router := gin.New() router.GET("/health", healthHandler(routeParams{version: "1.2.3"})) + router.GET("/ready", readyHandler(routeParams{version: "1.2.3"})) - setReadyForTest(t, false) - startingRec := httptest.NewRecorder() - router.ServeHTTP(startingRec, httptest.NewRequest(http.MethodGet, "/health", nil)) - if got := startingRec.Header().Get(desktopHealthTokenHeader); got != "" { - t.Fatalf("starting-path %s header = %q, want absent", desktopHealthTokenHeader, got) - } + for _, readyState := range []bool{false, true} { + setReadyForTest(t, readyState) - setReadyForTest(t, true) - readyRec := httptest.NewRecorder() - router.ServeHTTP(readyRec, httptest.NewRequest(http.MethodGet, "/health", nil)) - if got := readyRec.Header().Get(desktopHealthTokenHeader); got != "desktop-token" { - t.Fatalf("ready-path %s header = %q, want desktop-token", desktopHealthTokenHeader, got) + healthRec := httptest.NewRecorder() + router.ServeHTTP(healthRec, httptest.NewRequest(http.MethodGet, "/health", nil)) + if got := healthRec.Header().Get(desktopHealthTokenHeader); got != "desktop-token" { + t.Fatalf("ready=%v: /health %s header = %q, want desktop-token", readyState, desktopHealthTokenHeader, got) + } + + readyRec := httptest.NewRecorder() + router.ServeHTTP(readyRec, httptest.NewRequest(http.MethodGet, "/ready", nil)) + if got := readyRec.Header().Get(desktopHealthTokenHeader); got != "" { + t.Fatalf("ready=%v: /ready %s header = %q, want absent", readyState, desktopHealthTokenHeader, got) + } } } @@ -265,3 +292,39 @@ func TestHealthHandlerAuthEnabledServesVersionWithoutCredential(t *testing.T) { t.Fatalf("status field = %v, want ok", body["status"]) } } + +// TestReadyHandlerAuthEnabledServesWithoutCredential covers AC-22: /ready +// must be on the unauthenticated allowlist (mirrored at the middleware layer +// by TestEnabledModeAllowlistMatrix/ready), so it never returns 401/403 to +// an unauthenticated k8s readinessProbe or e2e fixture poll even with the +// auth feature flag on. +func TestReadyHandlerAuthEnabledServesWithoutCredential(t *testing.T) { + gin.SetMode(gin.TestMode) + setReadyForTest(t, true) + + svc := newSSOTestAuthService(t) + if _, _, err := svc.Setup(context.Background(), "admin@example.com", "adminpass123", "Admin", "", ""); err != nil { + t.Fatalf("Setup admin: %v", err) + } + + router := gin.New() + router.Use(authhttpmw.Middleware(svc)) + router.GET("/ready", readyHandler(routeParams{version: "1.2.3"})) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/ready", nil)) + + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want %d (no auth challenge on /ready)", recorder.Code, http.StatusOK) + } + var body map[string]interface{} + if err := json.Unmarshal(recorder.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body[versionFieldKey] != "1.2.3" { + t.Fatalf("version = %v, want 1.2.3", body[versionFieldKey]) + } + if body["status"] != "ok" { + t.Fatalf("status field = %v, want ok", body["status"]) + } +} diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index b3bddac9a3..67d54c16ab 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -110,6 +110,7 @@ const ( branchDeletionsFieldKey = "branch_deletions" deletedFieldKey = "deleted" versionFieldKey = "version" + serviceFieldKey = "service" kandevName = "kandev" startingStatus = "starting" ) @@ -718,16 +719,27 @@ func registerRoutes(p routeParams) { authhttpapi.RegisterRoutes(p.router, p.authSvc, p.log) } - // /health is a readiness probe, not a liveness probe. It only - // returns 200 after main has flipped the package-level `ready` - // flag — which happens after route registration, agent-registry - // seeding, the HTTP listener accepting connections, and (when - // KANDEV_E2E_MOCK is set) the testharness routes being mounted. - // 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. + // /health is a liveness probe: it answers 200 unconditionally, matching + // the bootstrap handler's /health contract (internal/backendapp/ + // httpserver.go) so the launcher's healthcheck never depends on startup + // progress. By the time this handler is reachable at all, ready is + // already true (see the comment on startGatewayAndServe's Store calls), + // but it does not gate on that flag — liveness must not depend on + // readiness, or the crash loop docs/specs/startup-listener-before- + // recovery/spec.md exists to fix comes back. p.router.GET("/health", healthHandler(p)) + // /ready is a readiness probe. It returns 200 only after main has + // flipped the package-level `ready` flag — which happens after route + // registration, agent-registry seeding, the HTTP listener accepting + // connections, and (when KANDEV_E2E_MOCK is set) the testharness routes + // being mounted. Before that, return 503 so callers (including the e2e + // fixture's readiness wait and Kubernetes' readinessProbe) keep polling + // instead of racing ahead and hitting 404s on routes that aren't wired + // yet. See docs/specs/health-endpoint-version/spec.md for the exact + // contract this endpoint now owns. + p.router.GET("/ready", readyHandler(p)) + // /api/v1/features is a public, unauthenticated read of the runtime // feature-flag map. The frontend SSR-fetches it once per page render to // decide whether to mount Office (and any future flagged feature). The @@ -763,7 +775,7 @@ func registerRoutes(p routeParams) { } else { p.router.NoRoute(func(c *gin.Context) { path := c.Request.URL.Path - if strings.HasPrefix(path, "/api/") || path == "/ws" || path == "/health" { + if strings.HasPrefix(path, "/api/") || path == "/ws" || path == "/health" || path == "/ready" { c.AbortWithStatus(http.StatusNotFound) return } @@ -787,42 +799,62 @@ func registerRoutes(p routeParams) { } } -// healthHandler serves GET /health. The response always carries the -// process's running version — in both the ready and not-ready bodies — so an -// operator can identify the build of a backend that is stuck starting, and so -// a monitor never needs a credential to read it (see docs/specs/ -// health-endpoint-version/spec.md). +// healthHandler serves GET /health, the liveness probe. It always answers +// 200 with the process's running version — regardless of the `ready` flag — +// mirroring the bootstrap handler's /health contract (httpserver.go) so the +// launcher's healthcheck never depends on startup progress. See docs/specs/ +// health-endpoint-version/spec.md for the exact response contract. func healthHandler(p routeParams) gin.HandlerFunc { - // p.version is normally the package-level Version wired in by run()'s - // registerRoutes call. Falling back here keeps the never-empty guarantee - // (AC-11) true regardless of that call-site wiring, since standing up - // run()'s full DI graph in a test just to exercise that one field - // assignment is out of proportion to this change. - version := p.version - if version == "" { - version = Version - } + version := resolveVersion(p) return func(c *gin.Context) { - if !ready.Load() { - c.JSON(http.StatusServiceUnavailable, gin.H{ - "status": startingStatus, - "service": kandevName, - versionFieldKey: version, - }) - return - } if token := desktopHealthToken(); token != "" { c.Header(desktopHealthTokenHeader, token) } c.JSON(http.StatusOK, gin.H{ - "status": "ok", - "service": kandevName, + statusKey: "ok", + serviceFieldKey: kandevName, "mode": "websocket+http", versionFieldKey: version, }) } } +// readyHandler serves GET /ready, the readiness probe. The response always +// carries the process's running version — in both the ready and not-ready +// bodies — so an operator can identify the build of a backend that is stuck +// starting, and so a monitor never needs a credential to read it (see +// docs/specs/health-endpoint-version/spec.md). +func readyHandler(p routeParams) gin.HandlerFunc { + version := resolveVersion(p) + return func(c *gin.Context) { + if !ready.Load() { + c.JSON(http.StatusServiceUnavailable, gin.H{ + statusKey: startingStatus, + serviceFieldKey: kandevName, + versionFieldKey: version, + }) + return + } + c.JSON(http.StatusOK, gin.H{ + statusKey: "ok", + serviceFieldKey: kandevName, + versionFieldKey: version, + }) + } +} + +// resolveVersion returns p.version, falling back to the package-level +// Version. p.version is normally wired in by run()'s registerRoutes call; +// the fallback keeps the never-empty guarantee (AC-11) true regardless of +// that call-site wiring, since standing up run()'s full DI graph in a test +// just to exercise that one field assignment is out of proportion. +func resolveVersion(p routeParams) string { + if p.version != "" { + return p.version + } + return Version +} + func desktopHealthToken() string { return strings.TrimSpace(os.Getenv(desktopHealthTokenEnv)) } diff --git a/apps/backend/internal/backendapp/httpserver.go b/apps/backend/internal/backendapp/httpserver.go index ea578b83f4..4df4c01651 100644 --- a/apps/backend/internal/backendapp/httpserver.go +++ b/apps/backend/internal/backendapp/httpserver.go @@ -2,9 +2,12 @@ package backendapp import ( "context" + "encoding/json" + "errors" "net" "net/http" "sync" + "sync/atomic" "time" "github.com/kandev/kandev/internal/common/config" @@ -12,6 +15,107 @@ import ( "go.uber.org/zap" ) +// errServerBindFailed signals that the HTTP listener could not be bound. +// startGatewayAndServe checks for it with errors.Is so it can skip the +// "Failed to start orchestrator" log for what is actually a bind failure — +// bindBootstrapListeners (via startHTTPServers) already logs the specific +// cause at the appropriate level. +var errServerBindFailed = errors.New("server bind failed") + +// handlerSwitch is an http.Handler whose effective implementation can be +// swapped at runtime without closing or rebinding the underlying listener. +// Startup constructs it holding the bootstrap handler and later Store()s the +// fully wired Gin router once buildHTTPServer returns, so exactly one +// *http.Server/*serverListeners pair flows from bind through shutdown. +type handlerSwitch struct { + current atomic.Pointer[http.Handler] +} + +// newHandlerSwitch returns a handlerSwitch already serving initial. +func newHandlerSwitch(initial http.Handler) *handlerSwitch { + hs := &handlerSwitch{} + hs.Store(initial) + return hs +} + +// Store replaces the handler that serves subsequent requests. +func (hs *handlerSwitch) Store(h http.Handler) { + hs.current.Store(&h) +} + +func (hs *handlerSwitch) ServeHTTP(w http.ResponseWriter, r *http.Request) { + (*hs.current.Load()).ServeHTTP(w, r) +} + +// newBootstrapHandler answers every request while the real router is still +// being built. GET /health always returns 200 (plus the desktop health +// token header, if configured) so the launcher's liveness probe +// (internal/launcher/health.go) succeeds regardless of startup progress — +// making liveness depend on readiness here would bring back the crash loop +// this handler exists to fix. Every other path returns a deterministic 503 +// with a parseable "starting" body instead of hanging, resetting, or 404ing. +// GET /health's body is identical to healthHandler's in helpers.go (status +// "ok", service, mode, version) since /health is a pure liveness probe and +// its shape must not depend on startup progress. +func newBootstrapHandler(version string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/health" { + body := map[string]any{ + statusKey: startingStatus, + serviceFieldKey: kandevName, + versionFieldKey: version, + } + writeBootstrapJSON(w, http.StatusServiceUnavailable, body) + return + } + if token := desktopHealthToken(); token != "" { + w.Header().Set(desktopHealthTokenHeader, token) + } + writeBootstrapJSON(w, http.StatusOK, map[string]any{ + statusKey: "ok", + serviceFieldKey: kandevName, + "mode": "websocket+http", + versionFieldKey: version, + }) + }) +} + +func writeBootstrapJSON(w http.ResponseWriter, status int, body map[string]any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(body) +} + +// bindBootstrapListeners binds the configured HTTP addresses immediately, +// before any startup work runs, and serves them with the bootstrap handler. +// The returned handlerSwitch is later Store()d with the real router; the +// returned *http.Server and *serverListeners are the same instances that +// must flow unmodified into awaitShutdown, so shutdown needs no changes. +func bindBootstrapListeners(cfg *config.Config, log *logger.Logger, version string) (*handlerSwitch, *http.Server, *serverListeners, error) { + handler := newHandlerSwitch(newBootstrapHandler(version)) + server := &http.Server{ + Handler: handler, + ReadTimeout: cfg.Server.ReadTimeoutDuration(), + WriteTimeout: cfg.Server.WriteTimeoutDuration(), + } + + port := resolvedHTTPPort(cfg) + hosts, err := cfg.Server.ResolvedBinds() + if err != nil { + log.Error("Invalid server bind configuration", zap.Error(err)) + return nil, nil, nil, errServerBindFailed + } + listeners, ok := startHTTPServers(server, hosts, port, log) + if !ok { + return nil, nil, nil, errServerBindFailed + } + + log.Info("HTTP listener bound; serving liveness probe while startup continues", + zap.Int("port", port), zap.Strings("hosts", hosts)) + + return handler, server, listeners, nil +} + // serverBindRetryInterval is how often the background retry loop re-attempts a // bind for an address that failed the initial bind (e.g. a tailnet IP that only // exists once tailscaled comes up). It is a var so tests can shorten it. @@ -162,21 +266,3 @@ func (sl *serverListeners) Stop() { } }) } - -// probeAddr returns an address the readiness probe can dial. It prefers a bound -// address that resolves to loopback (always reachable from the box without -// depending on a routable interface), falling back to the first bound address. -func (sl *serverListeners) probeAddr() string { - sl.mu.Lock() - defer sl.mu.Unlock() - if len(sl.bound) == 0 { - return "" - } - for _, a := range sl.bound { - probe := serverProbeAddr(a) - if host, _, err := net.SplitHostPort(probe); err == nil && config.IsLoopbackHost(host) { - return probe - } - } - return serverProbeAddr(sl.bound[0]) -} diff --git a/apps/backend/internal/backendapp/httpserver_test.go b/apps/backend/internal/backendapp/httpserver_test.go index 7655169b67..f688361849 100644 --- a/apps/backend/internal/backendapp/httpserver_test.go +++ b/apps/backend/internal/backendapp/httpserver_test.go @@ -260,25 +260,3 @@ func TestServerListenersStopClosesListenersAndDrains(t *testing.T) { t.Fatal("expected connection failure on 127.0.0.2 after shutdown") } } - -func TestProbeAddrPrefersLoopback(t *testing.T) { - sl := &serverListeners{bound: []string{"100.64.0.1:8080", "127.0.0.1:8080"}} - if got := sl.probeAddr(); got != "127.0.0.1:8080" { - t.Fatalf("probeAddr() = %q, want 127.0.0.1:8080", got) - } - - wildcard := &serverListeners{bound: []string{"0.0.0.0:8080"}} - if got := wildcard.probeAddr(); got != "127.0.0.1:8080" { - t.Fatalf("probeAddr() wildcard = %q, want 127.0.0.1:8080", got) - } - - wildcard6 := &serverListeners{bound: []string{"[::]:8080"}} - if got := wildcard6.probeAddr(); got != "[::1]:8080" { - t.Fatalf("probeAddr() ipv6 wildcard = %q, want [::1]:8080", got) - } - - tailnetOnly := &serverListeners{bound: []string{"100.64.0.1:8080"}} - if got := tailnetOnly.probeAddr(); got != "100.64.0.1:8080" { - t.Fatalf("probeAddr() tailnet-only = %q, want 100.64.0.1:8080", got) - } -} diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 8388ce24bf..a74e75e76e 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -176,12 +176,18 @@ func parseBackendFlags(args []string) (backendFlags, func(), error) { return out, flags.Usage, flags.Parse(args) } -// ready is the readiness flag consulted by the GET /health handler. -// Until it flips true, /health returns 503 — so callers polling for -// readiness (Playwright fixtures, container orchestrators, manual -// curl loops) keep waiting instead of racing ahead. main() flips it -// after every route is mounted, the agent registry is seeded, and -// the HTTP listener is accepting connections. +// ready is the readiness flag consulted by the GET /ready handler. Until it +// flips true, /ready returns 503 — so callers polling for readiness +// (Playwright fixtures, container orchestrators, manual curl loops) keep +// waiting instead of racing ahead of route registration. GET /health is a +// liveness probe and is unaffected by this flag: it answers 200 as soon as +// the listener is bound (see bindBootstrapListeners), because the launcher's +// healthcheck must succeed regardless of startup progress — making it depend +// on readiness would bring back the crash loop docs/specs/startup-listener- +// before-recovery/spec.md exists to fix. startGatewayAndServe flips this flag +// before swapping the bootstrap handler for the fully wired router (in that +// order — see the comment at the call site), so a request racing the swap +// never observes the real router with ready still false. var ready atomic.Bool // Run contains all startup logic and returns 0 on success or 1 on fatal error. @@ -741,14 +747,21 @@ func startAgentInfrastructure( agentSettingsController, lifecycleMgr, agentRegistry, orchestratorSvc, msgCreator, repoCloner, agentctlBinaryPath, addRuntimeCleanup, runCleanups, cancelContext, restoreCleanups) } -// startOrchestratorAndAutomationConsumers establishes the event-consumer -// chain in dependency order. The GitHub poller performs an immediate sweep on -// start, so both downstream consumers must be ready before it is started. +// startOrchestratorAndAutomationConsumers establishes the startup chain in +// dependency order: the HTTP listener must be bound (so the launcher's +// liveness probe can succeed) before the orchestrator runs its startup +// recovery sweeps, which can run long. The GitHub poller performs an +// immediate sweep on start, so both downstream consumers must be ready +// before it is started. func startOrchestratorAndAutomationConsumers( + bindListeners func() error, startOrchestrator func() error, startAutomation func(), startGitHubPoller func(), ) error { + if err := bindListeners(); err != nil { + return err + } if err := startOrchestrator(); err != nil { return err } @@ -757,6 +770,23 @@ func startOrchestratorAndAutomationConsumers( return nil } +// closeBoundListeners releases the HTTP listener(s) bound by bindListeners +// when a later startup step fails and startGatewayAndServe returns early. +// listeners.Stop only halts the background bind-retry loop; the bound TCP +// sockets are released by the shared http.Server (see serverListeners's doc +// comment), so an early return that skips this leaves the port held by this +// process even though the caller sees a start failure. server is non-nil +// whenever listeners is, since bindListeners assigns both together. +func closeBoundListeners(server *http.Server, listeners *serverListeners, log *logger.Logger) { + if listeners == nil { + return + } + listeners.Stop() + if err := server.Close(); err != nil { + log.Warn("failed to close HTTP listeners after startup failure", zap.Error(err)) + } +} + // startGatewayAndServe sets up the WebSocket gateway, HTTP routes, starts the server, // and blocks until a shutdown signal. // @@ -873,7 +903,22 @@ func startGatewayAndServe( services.Plugins.SetUtilityAgent(pluginsUtilityAgentAdapter{svc: services.Utility, userSvc: services.User}, pluginsHostUtilityAdapter{mgr: hostUtilityMgr}) } + var ( + handler *handlerSwitch + server *http.Server + listeners *serverListeners + ) + bindListeners := func() error { + h, s, l, err := bindBootstrapListeners(cfg, log, Version) + if err != nil { + return err + } + handler, server, listeners = h, s, l + return nil + } + if err := startOrchestratorAndAutomationConsumers( + bindListeners, func() error { return orchestratorSvc.Start(ctx) }, func() { if services.Automation == nil { @@ -894,7 +939,10 @@ func startGatewayAndServe( log.Info("GitHub poller started") }, ); err != nil { - log.Error("Failed to start orchestrator", zap.Error(err)) + if !errors.Is(err, errServerBindFailed) { + log.Error("Failed to start orchestrator", zap.Error(err)) + } + closeBoundListeners(server, listeners, log) return false } log.Info("Orchestrator initialized") @@ -921,6 +969,7 @@ func startGatewayAndServe( // ============================================ runProcessorSvc, ok := initOfficeServices(ctx, cfg, log, repos, services, orchestratorSvc, eventBus, agentctlBinaryPath, addCleanup, lifecycleMgr, agentRegistry) if !ok { + closeBoundListeners(server, listeners, log) return false } scheduling := startSchedulingRuntime( @@ -983,6 +1032,7 @@ func startGatewayAndServe( ) if err != nil { log.Error("Failed to initialize storage maintenance", zap.Error(err)) + closeBoundListeners(server, listeners, log) return false } hostUtilityMgr.SetTemporaryArtifactRegistry(storageComposition.tempArtifacts) @@ -1046,23 +1096,18 @@ func startGatewayAndServe( // ============================================ // HTTP SERVER // ============================================ - server, err := buildHTTPServer(cfg, log, gateway, repos, services, agentSettingsController, + // The listener was already bound (and is serving the bootstrap handler) + // by bindListeners above, before orchestratorSvc.Start ran its startup + // recovery sweeps. Build the real router now and swap it in on the same, + // already-bound handler and listeners — no second bind, no window where + // the socket is closed and reopened. + builtServer, err := buildHTTPServer(cfg, log, gateway, repos, services, agentSettingsController, lifecycleMgr, eventBus, orchestratorSvc, notificationCtrl, msgCreator, agentRegistry, hostUtilityMgr, addCleanup, repoCloner, systemSvc, storageComposition.workspaceRestorer, storageComposition.tempArtifacts, dbPool, agentRuntimeAvailability) if err != nil { log.Error("Failed to build HTTP server", zap.Error(err)) - return false - } - - port := resolvedHTTPPort(cfg) - hosts, err := cfg.Server.ResolvedBinds() - if err != nil { - log.Error("Invalid server bind configuration", zap.Error(err)) - return false - } - listeners, ok := startHTTPServers(server, hosts, port, log) - if !ok { + closeBoundListeners(server, listeners, log) return false } @@ -1072,17 +1117,31 @@ func startGatewayAndServe( zap.String("http", "/api/v1"), ) - // Flip the readiness flag once the HTTP listener is actually - // accepting connections, not just "spawned". Serve runs in a goroutine - // after we bind the socket above; probe a reachable local listener with a - // short retry loop — once a single connect succeeds, the kernel queue is up - // and any subsequent /health call will land on a wired route. - go waitListenerThenMarkReady(listeners.probeAddr(), log) + // Flip readiness before swapping in the fully wired router — see + // publishReadiness for why the order matters and + // TestPublishReadinessFlipsReadyBeforeSwappingHandler for the regression + // test pinning it. + publishReadiness(func() { ready.Store(true) }, func() { handler.Store(builtServer.Handler) }) awaitShutdown(server, listeners, scheduling, orchestratorSvc, lifecycleMgr, runCleanups, log) return true } +// publishReadiness flips readiness and then swaps in the fully wired router, +// in that order — never the reverse. A request landing between the two must +// never observe the real router while ready is still false: readyHandler +// gates on ready.Load(), so the reverse order would 503 a client on a router +// that is otherwise already fully up, recreating the exact flap +// docs/specs/startup-listener-before-recovery/spec.md exists to prevent. +// Flipping ready first means any request in that window still hits the +// bootstrap handler, whose /health branch is unconditionally 200 and whose +// every other path (including /ready) is a deterministic 503 "starting" — +// never a stale ready=false read through the real router. +func publishReadiness(markReady func(), swapHandler func()) { + markReady() + swapHandler() +} + // serverListenAddr formats the host and port into a listen address, binding // all interfaces when host is empty. func serverListenAddr(host string, port int) string { @@ -1093,58 +1152,6 @@ func serverListenAddr(host string, port int) string { return net.JoinHostPort(host, fmt.Sprint(port)) } -// serverProbeAddr rewrites a listen address into a loopback address suitable -// for local probing, mapping wildcard hosts to 127.0.0.1 / ::1. -func serverProbeAddr(listenAddr string) string { - host, port, err := net.SplitHostPort(listenAddr) - if err != nil { - return listenAddr - } - switch host { - case "", "0.0.0.0": - host = "127.0.0.1" - case "::": - // Preserve the address family: an IPv6-only wildcard listener isn't - // reachable via 127.0.0.1, so probe the IPv6 loopback instead. - host = "::1" - } - return net.JoinHostPort(host, port) -} - -// waitListenerThenMarkReady probes the local HTTP listener until a -// connect succeeds, then flips the package-level `ready` flag so the -// /health handler stops returning 503. Runs in its own goroutine so -// the caller can proceed into awaitShutdown. -// -// The probe budget is generous (30s) — under heavy parallel-suite -// load the OS scheduler can delay the listen goroutine for a couple -// of seconds. If the budget expires the flag still flips, so the -// backend doesn't permanently advertise "not ready" if probing fails -// for some unrelated reason (e.g. an iptables hiccup on a dev box). -func waitListenerThenMarkReady(listenAddr string, log *logger.Logger) { - addr := serverProbeAddr(listenAddr) - deadline := time.Now().Add(30 * time.Second) - ticker := time.NewTicker(50 * time.Millisecond) - defer ticker.Stop() - for { - conn, err := net.DialTimeout("tcp", addr, 500*time.Millisecond) - if err == nil { - _ = conn.Close() - // A successful TCP dial proves the listener is bound. - ready.Store(true) - log.Info("backend ready", zap.String("addr", addr)) - return - } - if time.Now().After(deadline) { - log.Warn("backend readiness probe never connected; flipping ready anyway", - zap.String("addr", addr)) - ready.Store(true) - return - } - <-ticker.C - } -} - // initOfficeServices constructs the run processor service for every backend and // adds Office-only services, reconciliation, and subscribers when the feature // is enabled. Global run and cron scheduling starts separately so this feature diff --git a/apps/backend/internal/backendapp/server_test.go b/apps/backend/internal/backendapp/server_test.go index 56a39ee22d..fcdbd88f5e 100644 --- a/apps/backend/internal/backendapp/server_test.go +++ b/apps/backend/internal/backendapp/server_test.go @@ -39,45 +39,56 @@ func TestDesktopHealthTokenTrimsEnv(t *testing.T) { } } -func TestHealthHandlerEchoesConfiguredToken(t *testing.T) { +// TestHealthHandlerAlwaysOkRegardlessOfReadiness is the regression test for +// R1-1: /health is a liveness probe and must answer 200 (with the desktop +// health token echoed, when configured) whether or not the `ready` flag has +// flipped. Gating it on readiness would bring back the crash loop docs/specs/ +// startup-listener-before-recovery/spec.md exists to fix. +func TestHealthHandlerAlwaysOkRegardlessOfReadiness(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(routeParams{})) - response := httptest.NewRecorder() - request := httptest.NewRequest(http.MethodGet, "/health", nil) - router.ServeHTTP(response, request) + for _, readyState := range []bool{false, true} { + ready.Store(readyState) + t.Cleanup(func() { ready.Store(false) }) - 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) + router := gin.New() + router.GET("/health", healthHandler(routeParams{})) + response := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/health", nil) + router.ServeHTTP(response, request) + + if response.Code != http.StatusOK { + t.Fatalf("ready=%v: health status = %d, want %d", readyState, response.Code, http.StatusOK) + } + if got := response.Header().Get(desktopHealthTokenHeader); got != "route-health-token" { + t.Fatalf("ready=%v: health token header = %q, want route-health-token", readyState, got) + } } } -func TestServerProbeAddr(t *testing.T) { - tests := []struct { - name string - listenAddr string - want string - }{ - {name: "port-only address probes loopback", listenAddr: ":38429", want: "127.0.0.1:38429"}, - {name: "wildcard ipv4 probes loopback", listenAddr: "0.0.0.0:38429", want: "127.0.0.1:38429"}, - {name: "wildcard ipv6 probes ipv6 loopback", listenAddr: "[::]:38429", want: "[::1]:38429"}, - {name: "loopback address probes itself", listenAddr: "127.0.0.1:38429", want: "127.0.0.1:38429"}, - {name: "ipv6 loopback probes itself", listenAddr: "[::1]:38429", want: "[::1]:38429"}, +// TestReadyHandlerGatesOnReadyFlag is the regression test for the task-02 +// requirement that readiness (unlike liveness) reports not-ready during a +// blocked startup and ready once startup completes. +func TestReadyHandlerGatesOnReadyFlag(t *testing.T) { + gin.SetMode(gin.TestMode) + ready.Store(false) + t.Cleanup(func() { ready.Store(false) }) + + router := gin.New() + router.GET("/ready", readyHandler(routeParams{})) + + response := httptest.NewRecorder() + router.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/ready", nil)) + if response.Code != http.StatusServiceUnavailable { + t.Fatalf("not-ready status = %d, want %d", response.Code, http.StatusServiceUnavailable) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := serverProbeAddr(tt.listenAddr); got != tt.want { - t.Fatalf("serverProbeAddr(%q) = %q, want %q", tt.listenAddr, got, tt.want) - } - }) + ready.Store(true) + response = httptest.NewRecorder() + router.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/ready", nil)) + if response.Code != http.StatusOK { + t.Fatalf("ready status = %d, want %d", response.Code, http.StatusOK) } } diff --git a/apps/backend/internal/backendapp/startup_order_test.go b/apps/backend/internal/backendapp/startup_order_test.go index 472468c9a8..69cdde9a7d 100644 --- a/apps/backend/internal/backendapp/startup_order_test.go +++ b/apps/backend/internal/backendapp/startup_order_test.go @@ -10,6 +10,7 @@ func TestStartOrchestratorAndAutomationConsumersOrder(t *testing.T) { var order []string err := startOrchestratorAndAutomationConsumers( + func() error { order = append(order, "bind"); return nil }, func() error { order = append(order, "orchestrator") return nil @@ -21,7 +22,7 @@ func TestStartOrchestratorAndAutomationConsumersOrder(t *testing.T) { if err != nil { t.Fatalf("start sequence returned error: %v", err) } - want := []string{"orchestrator", "automation", "github"} + want := []string{"bind", "orchestrator", "automation", "github"} if !reflect.DeepEqual(order, want) { t.Fatalf("start order = %v, want %v", order, want) } @@ -32,6 +33,7 @@ func TestStartOrchestratorAndAutomationConsumersStopsAfterOrchestratorFailure(t var automationStarted, githubStarted bool err := startOrchestratorAndAutomationConsumers( + func() error { return nil }, func() error { return startErr }, func() { automationStarted = true }, func() { githubStarted = true }, @@ -44,3 +46,49 @@ func TestStartOrchestratorAndAutomationConsumersStopsAfterOrchestratorFailure(t t.Fatalf("downstream consumers started after orchestrator failure: automation=%v github=%v", automationStarted, githubStarted) } } + +// TestStartOrchestratorAndAutomationConsumersStopsAfterBindFailure pins the +// bind-before-startup ordering invariant (docs/plans/startup-listener-before- +// recovery/task-03-ordering-guard.md): a bind failure must short-circuit +// before the orchestrator, automation, or GitHub poller ever start, exactly +// like an orchestrator-start failure does today. +func TestStartOrchestratorAndAutomationConsumersStopsAfterBindFailure(t *testing.T) { + bindErr := errors.New("bind unavailable") + var orchestratorStarted, automationStarted, githubStarted bool + + err := startOrchestratorAndAutomationConsumers( + func() error { return bindErr }, + func() error { orchestratorStarted = true; return nil }, + func() { automationStarted = true }, + func() { githubStarted = true }, + ) + + if !errors.Is(err, bindErr) { + t.Fatalf("start sequence error = %v, want %v", err, bindErr) + } + if orchestratorStarted || automationStarted || githubStarted { + t.Fatalf("downstream steps started after bind failure: orchestrator=%v automation=%v github=%v", + orchestratorStarted, automationStarted, githubStarted) + } +} + +// TestPublishReadinessFlipsReadyBeforeSwappingHandler is the regression test +// for R1-2 (docs/specs/health-endpoint-version/spec.md's 2026-08-22 +// amendment): startGatewayAndServe must flip the readiness flag before +// swapping in the fully wired router, never the reverse, or a request racing +// the swap could observe the real router with ready still false and get a +// spurious 503 — recreating the exact flap docs/specs/startup-listener- +// before-recovery/spec.md exists to prevent. +func TestPublishReadinessFlipsReadyBeforeSwappingHandler(t *testing.T) { + var order []string + + publishReadiness( + func() { order = append(order, "ready") }, + func() { order = append(order, "handler") }, + ) + + want := []string{"ready", "handler"} + if !reflect.DeepEqual(order, want) { + t.Fatalf("publishReadiness order = %v, want %v", order, want) + } +} diff --git a/apps/backend/internal/launcher/dev.go b/apps/backend/internal/launcher/dev.go index 4caa758652..0b92c8be56 100644 --- a/apps/backend/internal/launcher/dev.go +++ b/apps/backend/internal/launcher/dev.go @@ -71,6 +71,11 @@ func runDev(ctx context.Context, opts Options) int { if readyURL != "" { cfg.ports.BackendURL = readyURL } + if err := waitForReadyFn(ctx, cfg.ports.BackendURL, backend); err != nil { + supervisor.shutdown("backend readiness failure") + fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) + return 1 + } fmt.Printf("[kandev] backend ready at %s\n", cfg.ports.BackendURL) webURL := fmt.Sprintf("http://localhost:%d", cfg.ports.WebPort) diff --git a/apps/backend/internal/launcher/dev_test.go b/apps/backend/internal/launcher/dev_test.go index 10d2984b34..c10b4a027e 100644 --- a/apps/backend/internal/launcher/dev_test.go +++ b/apps/backend/internal/launcher/dev_test.go @@ -28,6 +28,7 @@ func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { oldAttachSignals := attachSignalsFn oldLaunchBackend := launchBackendFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn oldWaitForURL := waitForURLFn oldOpenBrowser := openBrowserFn oldStartProcess := startProcessFn @@ -36,6 +37,7 @@ func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { attachSignalsFn = oldAttachSignals launchBackendFn = oldLaunchBackend waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady waitForURLFn = oldWaitForURL openBrowserFn = oldOpenBrowser startProcessFn = oldStartProcess @@ -66,6 +68,10 @@ func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { readyURL = "http://127.0.0.2:" + itoa(backendCfg.Ports.BackendPort) return readyURL, nil } + waitForReadyFn = func(_ context.Context, url string, _ childState) error { + events = append(events, "wait-ready:"+url) + return nil + } waitForURLFn = func(_ context.Context, url string, _ childState, _ time.Duration) error { events = append(events, "wait-url:"+url) return nil @@ -92,6 +98,7 @@ func TestRunDevLaunchesBackendThenWebAndOpensBrowser(t *testing.T) { "attach-signals", "launch-backend", "wait-health:http://localhost:" + itoa(backendCfg.Ports.BackendPort), + "wait-ready:" + readyURL, "start-web:web", "wait-url:http://localhost:" + itoa(backendCfg.Ports.WebPort), "open:" + readyURL, @@ -166,6 +173,7 @@ func TestRunDevUsesConfiguredHealthTimeoutForBackendAndWeb(t *testing.T) { oldAttachSignals := attachSignalsFn oldLaunchBackend := launchBackendFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn oldWaitForURL := waitForURLFn oldOpenBrowser := openBrowserFn oldStartProcess := startProcessFn @@ -174,6 +182,7 @@ func TestRunDevUsesConfiguredHealthTimeoutForBackendAndWeb(t *testing.T) { attachSignalsFn = oldAttachSignals launchBackendFn = oldLaunchBackend waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady waitForURLFn = oldWaitForURL openBrowserFn = oldOpenBrowser startProcessFn = oldStartProcess @@ -191,6 +200,9 @@ func TestRunDevUsesConfiguredHealthTimeoutForBackendAndWeb(t *testing.T) { backendTimeout = timeout return "", nil } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + return nil + } waitForURLFn = func(_ context.Context, _ string, _ childState, timeout time.Duration) error { webTimeout = timeout return nil @@ -218,6 +230,7 @@ func TestRunDevHeadlessSkipsBrowser(t *testing.T) { oldLaunchBackend := launchBackendFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn oldWaitForURL := waitForURLFn oldOpenBrowser := openBrowserFn oldStartProcess := startProcessFn @@ -225,6 +238,7 @@ func TestRunDevHeadlessSkipsBrowser(t *testing.T) { t.Cleanup(func() { launchBackendFn = oldLaunchBackend waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady waitForURLFn = oldWaitForURL openBrowserFn = oldOpenBrowser startProcessFn = oldStartProcess @@ -241,6 +255,9 @@ func TestRunDevHeadlessSkipsBrowser(t *testing.T) { waitForHealthFn = func(_ context.Context, _ backendEndpointSet, _ childState, _ time.Duration, _ string, _ func()) (string, error) { return "", nil } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + return nil + } waitForURLFn = func(_ context.Context, _ string, _ childState, _ time.Duration) error { return nil } @@ -345,6 +362,71 @@ func TestRunDevShutsDownTreeOnBackendHealthFailure(t *testing.T) { } } +// TestRunDevShutsDownTreeOnBackendReadinessFailure is the R2-1 regression +// test for dev mode: it must not launch the web child or open the browser +// onto a backend that answered /health but never became ready. +func TestRunDevShutsDownTreeOnBackendReadinessFailure(t *testing.T) { + repo := makeRepoTree(t) + t.Chdir(repo) + t.Setenv("KANDEV_TASK_ID", "") + t.Setenv("KANDEV_DATABASE_PATH", "") + t.Setenv("KANDEV_HOME_DIR", "") + + oldLaunchBackend := launchBackendFn + oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn + oldWaitForURL := waitForURLFn + oldOpenBrowser := openBrowserFn + oldStartProcess := startProcessFn + oldAttachSignals := attachSignalsFn + oldStatusOutput := launcherStatusOutput + t.Cleanup(func() { + launchBackendFn = oldLaunchBackend + waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady + waitForURLFn = oldWaitForURL + openBrowserFn = oldOpenBrowser + startProcessFn = oldStartProcess + attachSignalsFn = oldAttachSignals + launcherStatusOutput = oldStatusOutput + }) + + var output strings.Builder + launcherStatusOutput = &output + + browserOpened := false + attachSignalsFn = func(_ *processSupervisor) {} + launchBackendFn = func(_ backendLaunchConfig) (*restartableBackend, func(), error) { + return &restartableBackend{exitCh: make(chan int, 1)}, func() {}, nil + } + waitForHealthFn = func(_ context.Context, _ backendEndpointSet, _ childState, _ time.Duration, _ string, _ func()) (string, error) { + return "", nil + } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + return errTestHealth + } + waitForURLFn = func(_ context.Context, _ string, _ childState, _ time.Duration) error { + t.Fatal("waitForURL called after backend readiness failure") + return nil + } + openBrowserFn = func(_ string) { browserOpened = true } + startProcessFn = func(_ string, _ []string, _ string, _ []string, _ bool, _ string, _ *processSupervisor) (*managedProcess, func(), error) { + t.Fatal("web child launched after backend readiness failure") + return nil, nil, nil + } + + code := runDev(context.Background(), Options{Command: CommandDev}) + if code != 1 { + t.Fatalf("runDev() = %d, want 1", code) + } + if !strings.Contains(output.String(), "graceful shutdown started") { + t.Fatalf("supervisor shutdown not triggered on readiness failure:\n%s", output.String()) + } + if browserOpened { + t.Fatal("browser opened despite readiness failure") + } +} + func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { repo := makeRepoTree(t) t.Chdir(repo) @@ -354,6 +436,7 @@ func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { oldLaunchBackend := launchBackendFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn oldWaitForURL := waitForURLFn oldOpenBrowser := openBrowserFn oldStartProcess := startProcessFn @@ -362,6 +445,7 @@ func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { t.Cleanup(func() { launchBackendFn = oldLaunchBackend waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady waitForURLFn = oldWaitForURL openBrowserFn = oldOpenBrowser startProcessFn = oldStartProcess @@ -381,6 +465,9 @@ func TestRunDevShutsDownTreeWhenWebChildExits(t *testing.T) { waitForHealthFn = func(_ context.Context, _ backendEndpointSet, _ childState, _ time.Duration, _ string, _ func()) (string, error) { return "", nil } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + return nil + } waitForURLFn = func(_ context.Context, _ string, _ childState, _ time.Duration) error { return nil } diff --git a/apps/backend/internal/launcher/health.go b/apps/backend/internal/launcher/health.go index 4495d0fb8b..d2d73e25f6 100644 --- a/apps/backend/internal/launcher/health.go +++ b/apps/backend/internal/launcher/health.go @@ -350,6 +350,57 @@ func safeProbeError(err error) string { return "network error" } +// waitForReady polls GET /ready until the backend reports readiness or the +// process exits. Unlike waitForHealth, it has no timeout of its own and never +// triggers a kill: /health (and its healthTimeoutReleaseMS/healthTimeoutDevMS +// budget) already made the keep-or-kill decision by the time this runs. This +// only decides when the bootstrap handler has handed off to the real router, +// so the launcher can print "backend ready"/open a browser onto real content +// instead of the bootstrap stub's 503 while startup recovery is still +// in flight. Do not fold this into waitForHealth's timeout — that would +// recreate the crash loop docs/specs/startup-listener-before-recovery/spec.md +// exists to fix. +func waitForReady(ctx context.Context, baseURL string, proc childState) error { + readyURL := baseURL + "/ready" + for ctx.Err() == nil { + if exited, code := proc.Exited(); exited { + return fmt.Errorf("backend exited (code %d) before it reported ready", code) + } + if probeReady(ctx, readyURL) { + if err := ctx.Err(); err != nil { + return fmt.Errorf("backend readiness wait canceled at %s: %w", readyURL, err) + } + if exited, code := proc.Exited(); exited { + return fmt.Errorf("backend exited (code %d) before it reported ready", code) + } + return nil + } + select { + case <-ctx.Done(): + case <-time.After(healthPollInterval): + } + } + return fmt.Errorf("backend readiness wait canceled at %s: %w", readyURL, ctx.Err()) +} + +// probeReady reports whether a single readiness request returned 2xx. Unlike +// probeHealth, it does not check the desktop health token: readyHandler never +// sets X-Kandev-Desktop-Health-Token (that header is /health-only, see +// docs/specs/health-endpoint-version/spec.md AC-21). +func probeReady(ctx context.Context, readyURL string) bool { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, readyURL, nil) + if err != nil { + return false + } + resp, err := healthProbeClient.Do(req) + if err != nil { + return false + } + defer func() { _ = resp.Body.Close() }() + _, _ = io.Copy(io.Discard, resp.Body) + return resp.StatusCode >= 200 && resp.StatusCode < 300 +} + func healthPort(baseURL string) string { parsed, err := url.Parse(baseURL) if err == nil && parsed.Port() != "" { diff --git a/apps/backend/internal/launcher/health_test.go b/apps/backend/internal/launcher/health_test.go index cb2efe55b0..ffd717ccba 100644 --- a/apps/backend/internal/launcher/health_test.go +++ b/apps/backend/internal/launcher/health_test.go @@ -20,6 +20,15 @@ type fakeChild struct { func (f fakeChild) Exited() (bool, int) { return f.exited, f.code } +type toggledChild struct { + exited atomic.Bool + code atomic.Int32 +} + +func (c *toggledChild) Exited() (bool, int) { + return c.exited.Load(), int(c.code.Load()) +} + func singleHealthTarget(url string) backendEndpointSet { return backendEndpointSet{ bindHosts: []string{"localhost"}, @@ -426,6 +435,161 @@ func TestWaitForHealthCallsOnFailureWhenContextCanceled(t *testing.T) { } } +func TestWaitForReadyReturnsNilOnReadyResponse(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/ready" { + t.Errorf("path = %q, want /ready", r.URL.Path) + } + // The first probe still reports 503 (bootstrap handler / not-ready-yet), + // exercising the poll loop, before flipping to ready. + if calls.Add(1) == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte(`{"status":"starting"}`)) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"status":"ok"}`)) + })) + t.Cleanup(srv.Close) + + if err := waitForReady(context.Background(), srv.URL, fakeChild{}); err != nil { + t.Fatalf("waitForReady() = %v, want nil", err) + } + if got := calls.Load(); got < 2 { + t.Fatalf("probe calls = %d, want at least 2", got) + } +} + +func TestWaitForReadyRejectsChildExitDuringSuccessfulProbe(t *testing.T) { + requestStarted := make(chan struct{}) + responseRelease := make(chan struct{}) + var child toggledChild + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/ready" { + t.Errorf("path = %q, want /ready", r.URL.Path) + } + close(requestStarted) + <-responseRelease + w.WriteHeader(http.StatusOK) + })) + var releaseOnce sync.Once + t.Cleanup(func() { + releaseOnce.Do(func() { close(responseRelease) }) + srv.Close() + }) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + done := make(chan error, 1) + go func() { + done <- waitForReady(ctx, srv.URL, &child) + }() + + select { + case <-requestStarted: + case <-time.After(2 * time.Second): + t.Fatal("waitForReady did not issue a readiness request") + } + child.code.Store(7) + child.exited.Store(true) + releaseOnce.Do(func() { close(responseRelease) }) + + select { + case err := <-done: + if err == nil { + t.Fatal("waitForReady() = nil, want exit error when child exits during probe") + } + if !strings.Contains(err.Error(), "code 7") { + t.Fatalf("error = %v, want the backend exit code", err) + } + case <-time.After(2 * time.Second): + t.Fatal("waitForReady did not return after the readiness probe completed") + } +} + +func TestWaitForReadyIgnoresMissingDesktopHealthToken(t *testing.T) { + // readyHandler never sets X-Kandev-Desktop-Health-Token (that header is + // /health-only); probeReady must accept a bare 2xx regardless. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + if err := waitForReady(context.Background(), srv.URL, fakeChild{}); err != nil { + t.Fatalf("waitForReady() = %v, want nil", err) + } +} + +func TestWaitForReadyKeepsPollingPastWhatWouldBeAHealthTimeout(t *testing.T) { + // waitForReady has no timeout of its own: unlike waitForHealth's + // healthTimeoutReleaseMS/healthTimeoutDevMS budget, a long readiness wait + // (startup recovery still running) must not be treated as a failure. + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusServiceUnavailable) + })) + t.Cleanup(srv.Close) + + ctx, cancel := context.WithTimeout(context.Background(), 700*time.Millisecond) + defer cancel() + err := waitForReady(ctx, srv.URL, fakeChild{}) + if err == nil { + t.Fatal("waitForReady() = nil, want the injected context deadline to end the wait") + } + if !strings.Contains(err.Error(), "canceled") { + t.Fatalf("error = %v, want a cancellation error", err) + } + if got := calls.Load(); got < 2 { + t.Fatalf("probe calls = %d, want at least 2 (proves it kept polling past a single failure)", got) + } +} + +func TestWaitForReadyReturnsWhenContextCanceled(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case <-release: + case <-r.Context().Done(): + } + })) + t.Cleanup(func() { + close(release) + srv.Close() + }) + + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { + done <- waitForReady(ctx, srv.URL, fakeChild{}) + }() + cancel() + + select { + case err := <-done: + if err == nil { + t.Fatal("waitForReady() = nil, want cancellation error") + } + if !strings.Contains(err.Error(), "canceled") { + t.Fatalf("error = %v, want a cancellation error", err) + } + case <-time.After(10 * time.Second): + t.Fatal("waitForReady ignored context cancellation") + } +} + +func TestWaitForReadyFailsFastWhenBackendExited(t *testing.T) { + err := waitForReady(context.Background(), "http://127.0.0.1:1", fakeChild{exited: true, code: 3}) + if err == nil { + t.Fatal("waitForReady() = nil, want exit error") + } + if !strings.Contains(err.Error(), "code 3") { + t.Fatalf("error = %v, want the backend exit code", err) + } +} + func TestWaitForHealthFailsFastWhenBackendExited(t *testing.T) { var failures int _, err := waitForHealth( diff --git a/apps/backend/internal/launcher/start.go b/apps/backend/internal/launcher/start.go index ce2a8c25a4..2bdd016af0 100644 --- a/apps/backend/internal/launcher/start.go +++ b/apps/backend/internal/launcher/start.go @@ -17,6 +17,7 @@ var ( newSupervisorFn = newSupervisor launchBackendFn = launchRestartableBackend waitForHealthFn = waitForHealth + waitForReadyFn = waitForReady attachSignalsFn = func(supervisor *processSupervisor) { supervisor.attachSignals() } @@ -173,6 +174,12 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { if readyURL != "" { cfg.Ports.BackendURL = readyURL } + shutdownDebugf("runManagedApp backend healthy, waiting for readiness") + if err := waitForReadyFn(ctx, cfg.Ports.BackendURL, backend); err != nil { + supervisor.shutdown("backend readiness failure") + fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) + return 1 + } fmt.Printf("[kandev] backend ready at %s\n", cfg.Ports.BackendURL) if !shouldOpenBrowser(cfg.Opts, cfg.Startup) { diff --git a/apps/backend/internal/launcher/start_test.go b/apps/backend/internal/launcher/start_test.go index 512c1f25ca..5384066bf0 100644 --- a/apps/backend/internal/launcher/start_test.go +++ b/apps/backend/internal/launcher/start_test.go @@ -2,6 +2,7 @@ package launcher import ( "context" + "errors" "os" "path/filepath" "reflect" @@ -200,12 +201,14 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { oldAttachSignals := attachSignalsFn oldStartParentWatch := startParentWatchFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn t.Cleanup(func() { newSupervisorFn = oldNewSupervisor launchBackendFn = oldLaunchBackend attachSignalsFn = oldAttachSignals startParentWatchFn = oldStartParentWatch waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady }) var events []string @@ -242,6 +245,10 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { events = append(events, "wait-health") return "", nil } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + events = append(events, "wait-ready") + return nil + } t.Setenv("KANDEV_HOME_DIR", t.TempDir()) code := runManagedApp(context.Background(), managedAppConfig{ @@ -258,7 +265,7 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { if code != 0 { t.Fatalf("runManagedApp() = %d, want 0", code) } - want := []string{"new-supervisor", "attach-signals", "start-parent-watch", "launch-backend", "wait-health"} + want := []string{"new-supervisor", "attach-signals", "start-parent-watch", "launch-backend", "wait-health", "wait-ready"} if !reflect.DeepEqual(events, want) { t.Fatalf("events = %v, want %v", events, want) } @@ -273,16 +280,85 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { } } +// TestRunManagedAppShutsDownOnReadinessFailure is the regression test for +// R2-1: a launcher that only waited on /health would report "backend ready" +// and open a browser onto the bootstrap handler's 503 the moment the socket +// bound, before startup recovery finished. waitForReadyFn must run after +// waitForHealthFn succeeds, and its failure must abort before either the +// ready print or a browser open, exactly like a health failure does. +func TestRunManagedAppShutsDownOnReadinessFailure(t *testing.T) { + oldNewSupervisor := newSupervisorFn + oldLaunchBackend := launchBackendFn + oldAttachSignals := attachSignalsFn + oldStartParentWatch := startParentWatchFn + oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn + oldStatusOutput := launcherStatusOutput + t.Cleanup(func() { + newSupervisorFn = oldNewSupervisor + launchBackendFn = oldLaunchBackend + attachSignalsFn = oldAttachSignals + startParentWatchFn = oldStartParentWatch + waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady + launcherStatusOutput = oldStatusOutput + }) + + var output strings.Builder + launcherStatusOutput = &output + + newSupervisorFn = newSupervisor + attachSignalsFn = func(_ *processSupervisor) {} + startParentWatchFn = func(_ *processSupervisor) *parentWatchdog { + return newParentWatchdog(0, nil, nil) + } + launchBackendFn = func(_ backendLaunchConfig) (*restartableBackend, func(), error) { + return &restartableBackend{exitCh: make(chan int, 1)}, func() {}, nil + } + waitForHealthFn = func(_ context.Context, _ backendEndpointSet, _ childState, _ time.Duration, _ string, _ func()) (string, error) { + return "", nil + } + readyWaited := false + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + readyWaited = true + return errors.New("test readiness failure") + } + t.Setenv("KANDEV_HOME_DIR", t.TempDir()) + + code := runManagedApp(context.Background(), managedAppConfig{ + Header: "test", + Mode: "start", + Backend: "kandev", + BackendCWD: t.TempDir(), + Ports: portConfig{ + BackendPort: 48123, + BackendURL: "http://localhost:48123", + }, + Opts: Options{Headless: true}, + }) + if code != 1 { + t.Fatalf("runManagedApp() = %d, want 1", code) + } + if !readyWaited { + t.Fatal("waitForReadyFn was never called") + } + if !strings.Contains(output.String(), "graceful shutdown started") { + t.Fatalf("supervisor shutdown not triggered on readiness failure:\n%s", output.String()) + } +} + func TestRunManagedAppPreservesDesktopOwnedHealthToken(t *testing.T) { oldNewSupervisor := newSupervisorFn oldLaunchBackend := launchBackendFn oldAttachSignals := attachSignalsFn oldWaitForHealth := waitForHealthFn + oldWaitForReady := waitForReadyFn t.Cleanup(func() { newSupervisorFn = oldNewSupervisor launchBackendFn = oldLaunchBackend attachSignalsFn = oldAttachSignals waitForHealthFn = oldWaitForHealth + waitForReadyFn = oldWaitForReady }) const desktopToken = "desktop-owned-token" @@ -300,6 +376,9 @@ func TestRunManagedAppPreservesDesktopOwnedHealthToken(t *testing.T) { waitedHealthToken = expectedToken return "", nil } + waitForReadyFn = func(_ context.Context, _ string, _ childState) error { + return nil + } t.Setenv("KANDEV_HOME_DIR", t.TempDir()) t.Setenv("KANDEV_DESKTOP_NATIVE_NOTIFICATIONS", "true") t.Setenv("KANDEV_DESKTOP_HEALTH_TOKEN", desktopToken) diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index b9f337a7f9..c5f2658620 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" "sync" + "sync/atomic" "time" "go.uber.org/zap" @@ -787,6 +788,14 @@ func (s *Service) restoreTaskLifecycleToken(ctx context.Context, taskID, key, ev } } +// lifecycleSweepStuckWarningInterval controls how long +// reconcileTaskLifecycleTokens waits before logging a "still recovering" +// warning while the sweep is still in flight. It is a var so tests can +// shorten it. This is a log-only signal: it never cancels the sweep, because +// the resume calls it drives run under context.WithoutCancel +// (task_operations.go), so cancellation would not work anyway. +var lifecycleSweepStuckWarningInterval = 60 * time.Second + // reconcileTaskLifecycleTokens scans durable lifecycle markers at startup. // A small fixed worker pool and bounded per-task attempts prevent a corrupted // or repeatedly failing row from creating an unbounded goroutine storm. @@ -839,6 +848,19 @@ func (s *Service) reconcileTaskLifecycleTokens(ctx context.Context) { if len(jobs) == 0 { return } + + start := time.Now() + taskCount := len(jobs) + s.logger.Info("startup lifecycle sweep starting", zap.Int("task_count", taskCount)) + + var processed atomic.Int64 + sweepDone := make(chan struct{}) + warnDone := make(chan struct{}) + go func() { + defer close(warnDone) + s.warnIfLifecycleSweepStuck(taskCount, &processed, start, sweepDone) + }() + jobIDs := make(chan string) workerCount := 4 if len(jobs) < workerCount { @@ -851,6 +873,7 @@ func (s *Service) reconcileTaskLifecycleTokens(ctx context.Context) { defer wg.Done() for taskID := range jobIDs { s.recoverTaskLifecycleToken(ctx, taskID) + processed.Add(1) } }() } @@ -859,6 +882,30 @@ func (s *Service) reconcileTaskLifecycleTokens(ctx context.Context) { } close(jobIDs) wg.Wait() + close(sweepDone) + <-warnDone + + s.logger.Info("startup lifecycle sweep finished", + zap.Int("task_count", taskCount), + zap.Duration("elapsed", time.Since(start))) +} + +// warnIfLifecycleSweepStuck logs a warning if the startup lifecycle sweep is +// still running after lifecycleSweepStuckWarningInterval, so a slow sweep is +// diagnosable from logs instead of requiring a goroutine dump. It is +// log-only: it never cancels the sweep, and it always exits (either the +// timer fires once, or done closes first). +func (s *Service) warnIfLifecycleSweepStuck(taskCount int, processed *atomic.Int64, start time.Time, done <-chan struct{}) { + timer := time.NewTimer(lifecycleSweepStuckWarningInterval) + defer timer.Stop() + select { + case <-done: + case <-timer.C: + s.logger.Warn("startup lifecycle sweep still running", + zap.Int("task_count", taskCount), + zap.Int64("processed", processed.Load()), + zap.Duration("elapsed", time.Since(start))) + } } func (s *Service) recoverTaskLifecycleToken(ctx context.Context, taskID string) { diff --git a/apps/backend/internal/orchestrator/lifecycle_sweep_logging_test.go b/apps/backend/internal/orchestrator/lifecycle_sweep_logging_test.go new file mode 100644 index 0000000000..dc2b15feb6 --- /dev/null +++ b/apps/backend/internal/orchestrator/lifecycle_sweep_logging_test.go @@ -0,0 +1,155 @@ +package orchestrator + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + commonlogger "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" +) + +// blockingLifecycleRepo blocks GetTask until release is closed, letting a +// test hold reconcileTaskLifecycleTokens open long enough to observe the +// stuck-sweep warning. +type blockingLifecycleRepo struct { + sessionExecutorStore + release <-chan struct{} +} + +func (r *blockingLifecycleRepo) GetTask(ctx context.Context, id string) (*models.Task, error) { + <-r.release + return r.sessionExecutorStore.GetTask(ctx, id) +} + +func (r *blockingLifecycleRepo) ListTasksWithMetadataKey(ctx context.Context, key string) ([]*models.Task, error) { + return r.sessionExecutorStore.(interface { + ListTasksWithMetadataKey(context.Context, string) ([]*models.Task, error) + }).ListTasksWithMetadataKey(ctx, key) +} + +func observedTestLogger(t *testing.T) (*commonlogger.Logger, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.DebugLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observed logger: %v", err) + } + return log, logs +} + +// observedTestLoggerWatching is observedTestLogger plus a channel that closes +// the first time a log entry with the given message is emitted, so a test can +// synchronize on the log itself instead of polling with time.Sleep. +func observedTestLoggerWatching(t *testing.T, message string) (*commonlogger.Logger, *observer.ObservedLogs, <-chan struct{}) { + t.Helper() + core, logs := observer.New(zapcore.DebugLevel) + + seen := make(chan struct{}) + var once sync.Once + hooked := zapcore.RegisterHooks(core, func(entry zapcore.Entry) error { + if entry.Message == message { + once.Do(func() { close(seen) }) + } + return nil + }) + + log, err := commonlogger.NewFromZap(zap.New(hooked)) + if err != nil { + t.Fatalf("create observed logger: %v", err) + } + return log, logs, seen +} + +// TestReconcileTaskLifecycleTokensLogsStartAndFinish is the regression test +// for docs/plans/startup-listener-before-recovery/task-03-ordering-guard.md: +// a long startup sweep must be diagnosable from logs (task count, elapsed +// time) rather than requiring a goroutine dump. +// +// Expected pre-fix failure: reconcileTaskLifecycleTokens logs neither +// message today, so both FilterMessage results are empty. +func TestReconcileTaskLifecycleTokensLogsStartAndFinish(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "sweep-log-task", "sweep-log-session", "step1") + if err := repo.SetTaskMetadataKey(ctx, "sweep-log-task", models.MetaKeyQueuePromotionPending, true); err != nil { + t.Fatalf("seed promotion token: %v", err) + } + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + log, logs := observedTestLogger(t) + svc.logger = log + + svc.reconcileTaskLifecycleTokens(ctx) + + starts := logs.FilterMessage("startup lifecycle sweep starting").All() + if len(starts) != 1 { + t.Fatalf("start logs = %#v, want exactly one", starts) + } + if got := fmt.Sprint(starts[0].ContextMap()["task_count"]); got != "1" { + t.Fatalf("start log task_count = %v, want 1", starts[0].ContextMap()["task_count"]) + } + + finishes := logs.FilterMessage("startup lifecycle sweep finished").All() + if len(finishes) != 1 { + t.Fatalf("finish logs = %#v, want exactly one", finishes) + } + if got := fmt.Sprint(finishes[0].ContextMap()["task_count"]); got != "1" { + t.Fatalf("finish log task_count = %v, want 1", finishes[0].ContextMap()["task_count"]) + } + if _, ok := finishes[0].ContextMap()["elapsed"]; !ok { + t.Fatalf("finish log missing elapsed field: %#v", finishes[0].ContextMap()) + } +} + +// TestReconcileTaskLifecycleTokensWarnsWhenStuck is the regression test for +// the "still recovering N tasks" log-only signal: a sweep still running +// after lifecycleSweepStuckWarningInterval must warn instead of staying +// silent, and must never cancel the in-flight recovery (context.WithoutCancel +// in task_operations.go means cancellation would not work anyway). +// +// Expected pre-fix failure: lifecycleSweepStuckWarningInterval does not +// exist yet, so this fails to compile. +func TestReconcileTaskLifecycleTokensWarnsWhenStuck(t *testing.T) { + prevInterval := lifecycleSweepStuckWarningInterval + lifecycleSweepStuckWarningInterval = 20 * time.Millisecond + t.Cleanup(func() { lifecycleSweepStuckWarningInterval = prevInterval }) + + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "stuck-task", "stuck-session", "step1") + if err := repo.SetTaskMetadataKey(ctx, "stuck-task", models.MetaKeyQueuePromotionPending, true); err != nil { + t.Fatalf("seed promotion token: %v", err) + } + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + log, _, warned := observedTestLoggerWatching(t, "startup lifecycle sweep still running") + svc.logger = log + + release := make(chan struct{}) + svc.repo = &blockingLifecycleRepo{sessionExecutorStore: repo, release: release} + + done := make(chan struct{}) + go func() { + defer close(done) + svc.reconcileTaskLifecycleTokens(ctx) + }() + + select { + case <-warned: + case <-time.After(2 * time.Second): + close(release) + t.Fatal("stuck-sweep warning never logged") + } + + close(release) + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("sweep did not finish after release") + } +} diff --git a/apps/desktop/AGENTS.md b/apps/desktop/AGENTS.md index a6362a665e..5f1baad4f3 100644 --- a/apps/desktop/AGENTS.md +++ b/apps/desktop/AGENTS.md @@ -29,7 +29,7 @@ Use `scripts/release/prepare-desktop-runtime.sh` and `scripts/release/verify-des ## Architecture -- Frontend code in `src/` is only the startup/error surface. The real product UI is still served by the Go backend after `/health` succeeds. +- Frontend code in `src/` is only the startup/error surface. The real product UI is still served by the Go backend after `/ready` succeeds. - Rust code owns backend process spawning and cleanup. Do not expose broad shell or filesystem permissions to frontend JavaScript. - Native menus emit versioned `kandev-desktop-v1-*` events for SPA-owned context and navigation. Updater, notification, and external-link operations use narrow generated Tauri commands scoped diff --git a/apps/desktop/e2e/desktop-launch-smoke.mjs b/apps/desktop/e2e/desktop-launch-smoke.mjs index 6df788b57a..00e218b443 100644 --- a/apps/desktop/e2e/desktop-launch-smoke.mjs +++ b/apps/desktop/e2e/desktop-launch-smoke.mjs @@ -11,11 +11,17 @@ const __dirname = dirname(fileURLToPath(import.meta.url)); const desktopRoot = resolve(__dirname, ".."); const repoRoot = resolve(desktopRoot, "../.."); -// Keep above HEALTH_TIMEOUT in apps/desktop/src-tauri/src/backend.rs (60s): the Rust side -// retries GET /health every 250ms until that deadline, so this must give it enough room to -// finish, or a launcher Rust would still consider "starting" gets killed here first. -// desktop-launch-smoke.test.mjs asserts the relationship so the two stay in sync. +// The Rust side (apps/desktop/src-tauri/src/backend.rs) does a two-stage wait before it +// navigates the webview: wait_for_backend polls GET /health every 250ms against a bounded +// HEALTH_TIMEOUT (60s), then wait_for_ready polls GET /ready every 250ms with NO timeout — +// it only gives up on child exit or shutdown. HEALTH_REQUESTED_TIMEOUT_MS must stay above +// HEALTH_TIMEOUT so the fake runtime doesn't get killed here before the Rust side even +// finishes the first stage; desktop-launch-smoke.test.mjs asserts that relationship so the +// two stay in sync. READY_REQUESTED_TIMEOUT_MS only bounds this test — the fake runtime +// answers /ready immediately once it's listening, so the real wait_for_ready being unbounded +// doesn't matter here. export const HEALTH_REQUESTED_TIMEOUT_MS = 90_000; +export const READY_REQUESTED_TIMEOUT_MS = 60_000; export const ROOT_REQUESTED_TIMEOUT_MS = 60_000; // Only run the CLI behavior when this file is executed directly (`node desktop-launch-smoke.mjs` @@ -81,6 +87,12 @@ async function runSmoke() { failIfExited, describeChild, ); + await waitForFile( + join(stateDir, "ready-requested"), + READY_REQUESTED_TIMEOUT_MS, + failIfExited, + describeChild, + ); await waitForFile( join(stateDir, "root-requested"), ROOT_REQUESTED_TIMEOUT_MS, @@ -91,7 +103,9 @@ async function runSmoke() { await stopProcess(child); } - console.log("Desktop smoke passed: WebView requested / after backend health succeeded."); + console.log( + "Desktop smoke passed: WebView requested / after backend health and readiness succeeded.", + ); } async function writeFakeRuntime(runtimeDir, stateDir) { @@ -153,6 +167,13 @@ async function runFakeRuntime(stateDir, args) { return; } + if (req.url === "/ready") { + await writeFile(join(stateDir, "ready-requested"), "1"); + res.writeHead(200, { "content-type": "application/json" }); + res.end('{"status":"ok"}'); + return; + } + if (req.url === "/") { await writeFile(join(stateDir, "root-requested"), "1"); res.writeHead(200, { "content-type": "text/html" }); diff --git a/apps/desktop/src-tauri/src/backend.rs b/apps/desktop/src-tauri/src/backend.rs index 17da701f23..6855467f5b 100644 --- a/apps/desktop/src-tauri/src/backend.rs +++ b/apps/desktop/src-tauri/src/backend.rs @@ -287,6 +287,7 @@ fn launch_and_wait(app: &AppHandle, state: &BackendState) -> Result Result<(), String> { + loop { + if state.is_shutdown_started() { + return Err("Desktop startup cancelled".to_string()); + } + if let Some(message) = state.child_exit_message()? { + return Err(format!( + "Kandev launcher exited before /ready reported ready ({message})" + )); + } + if ready_ready(port) { + thread::sleep(HEALTH_READY_SETTLE); + if let Some(message) = state.child_exit_message()? { + return Err(format!( + "Kandev launcher exited before /ready reported ready ({message})" + )); + } + return Ok(()); + } + thread::sleep(Duration::from_millis(250)); + } +} + fn launcher_exit_message(status: &str, output: Option) -> String { let mut message = status.to_string(); append_recent_output(&mut message, output); @@ -577,6 +614,37 @@ fn request_health(port: u16, expected_health_token: &str) -> Result bool { + request_ready(port).unwrap_or(false) +} + +// request_ready hits /ready rather than /health. /ready never sets +// DESKTOP_HEALTH_TOKEN_HEADER (that header is /health-only — see +// docs/specs/health-endpoint-version/spec.md AC-21), so unlike +// request_health this does not check for one. +fn request_ready(port: u16) -> Result { + let addr = (LOOPBACK_HOST, port) + .to_socket_addrs() + .map_err(|err| err.to_string())? + .next() + .ok_or_else(|| format!("Could not resolve {LOOPBACK_HOST}:{port}"))?; + let mut stream = TcpStream::connect_timeout(&addr, Duration::from_millis(250)) + .map_err(|err| err.to_string())?; + stream + .set_read_timeout(Some(Duration::from_millis(250))) + .map_err(|err| err.to_string())?; + stream + .write_all( + format!( + "GET /ready HTTP/1.1\r\nHost: {LOOPBACK_HOST}:{port}\r\nConnection: close\r\n\r\n" + ) + .as_bytes(), + ) + .map_err(|err| err.to_string())?; + let response = read_http_response_head(&mut stream)?; + Ok(response.starts_with(b"HTTP/1.1 200") || response.starts_with(b"HTTP/1.0 200")) +} + fn read_http_response_head(reader: &mut R) -> Result, String> { const MAX_HEADER_BYTES: usize = 8 * 1024; let mut response = Vec::with_capacity(512); @@ -1122,6 +1190,47 @@ mod tests { )); } + #[test] + fn request_ready_accepts_response_without_health_token_header() { + // readyHandler never sets X-Kandev-Desktop-Health-Token (that header + // is /health-only); request_ready must accept a bare 2xx. + let listener = TcpListener::bind((LOOPBACK_HOST, 0)).expect("bind ready listener"); + let port = listener.local_addr().expect("listener addr").port(); + thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0_u8; 512]; + let _ = stream.read(&mut buf); + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n"); + } + }); + + let ready = request_ready(port).expect("request_ready"); + assert!( + ready, + "request_ready must treat a bare 2xx as ready with no token check" + ); + } + + #[test] + fn request_ready_rejects_non_2xx_response() { + let listener = TcpListener::bind((LOOPBACK_HOST, 0)).expect("bind ready listener"); + let port = listener.local_addr().expect("listener addr").port(); + thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0_u8; 512]; + let _ = stream.read(&mut buf); + let _ = stream + .write_all(b"HTTP/1.1 503 Service Unavailable\r\nContent-Length: 0\r\n\r\n"); + } + }); + + let ready = request_ready(port).expect("request_ready"); + assert!( + !ready, + "request_ready must reject a non-2xx response (bootstrap-not-ready-yet)" + ); + } + #[cfg(unix)] #[test] fn stop_terminates_tracked_child() { @@ -1145,6 +1254,48 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn wait_for_ready_rechecks_child_exit_after_success() { + // The child exits well before the /ready response arrives, so a + // wait_for_ready that trusted a bare "GET /ready succeeded" without + // rechecking would return Ok for a backend that is already gone. + let listener = TcpListener::bind((LOOPBACK_HOST, 0)).expect("bind ready listener"); + let port = listener.local_addr().expect("listener addr").port(); + thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0_u8; 512]; + let _ = stream.read(&mut buf); + thread::sleep(Duration::from_millis(300)); + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n"); + } + }); + + let state = BackendState::default(); + let child = Command::new("sh") + .arg("-c") + .arg("sleep 0.1") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn short-lived child"); + assert!(state.set_child(child)); + + let result = wait_for_ready(port, &state); + + assert!( + result.is_err(), + "wait_for_ready must not return Ok once the child has exited" + ); + assert!( + result + .unwrap_err() + .contains("exited before /ready reported ready"), + "wait_for_ready error should name the exit-before-ready reason" + ); + } + #[cfg(unix)] #[test] fn set_child_terminates_child_when_shutdown_already_started() { diff --git a/apps/web/e2e/fixtures/backend.ts b/apps/web/e2e/fixtures/backend.ts index 5d28e2db03..c1b167ef5d 100644 --- a/apps/web/e2e/fixtures/backend.ts +++ b/apps/web/e2e/fixtures/backend.ts @@ -475,7 +475,10 @@ export const backendFixture = base.extend({ // --- Spawn backend --- backendProc = spawnBackendProcess(scopedEnv.apply(baselineEnv), debug, backendPort); registerProcess(backendProc); - await waitForHealth(`${baseUrl}/health`, HEALTH_TIMEOUT_MS, backendProc); + // /ready (not /health) — /health flips green as soon as the listener + // is bound, before routes are wired; tests that immediately issue API + // requests need the readiness contract instead. + await waitForHealth(`${baseUrl}/ready`, HEALTH_TIMEOUT_MS, backendProc); const frontendUrl = baseUrl; /** @@ -499,14 +502,15 @@ export const backendFixture = base.extend({ await waitForPortFree(backendPort); backendProc = spawnBackendProcess(nextEnv, debug, backendPort); registerProcess(backendProc); - // Pass the process so waitForHealth fails fast if it exits (e.g. port still in use) - await waitForHealth(`${baseUrl}/health`, HEALTH_TIMEOUT_MS, backendProc); + // Pass the process so waitForHealth fails fast if it exits (e.g. port still in use). + // /ready, not /health — see the comment on the initial spawn above. + await waitForHealth(`${baseUrl}/ready`, HEALTH_TIMEOUT_MS, backendProc); }; let recovery: Promise | null = null; const ensureReady = async () => { try { - await waitForHealth(`${baseUrl}/health`, 5_000); + await waitForHealth(`${baseUrl}/ready`, 5_000); return; } catch { // A worker can outlive a backend process that a prior test left diff --git a/docs/docker.md b/docs/docker.md index f38b292e0e..60309bd158 100644 --- a/docs/docker.md +++ b/docs/docker.md @@ -281,17 +281,21 @@ The volume at `/data` carries over the database, worktrees, npm globals, and `$H ## Health Check -The backend exposes a `/health` endpoint: +The backend exposes a `/health` endpoint (liveness: the process is up and its listener is +bound, even mid-startup) and a `/ready` endpoint (readiness: startup finished and it can +serve real traffic): ```bash curl http://localhost:38429/health +curl http://localhost:38429/ready ``` -For Docker health checks in compose: +Compose's single `healthcheck:` concept maps to "can serve real traffic," so it should +point at `/ready`, not `/health`: ```yaml healthcheck: - test: ["CMD", "curl", "-f", "http://localhost:38429/health"] + test: ["CMD", "curl", "-f", "http://localhost:38429/ready"] interval: 30s timeout: 5s retries: 3 diff --git a/docs/k8s.md b/docs/k8s.md index f6b6e2fe36..e05d84ec9a 100644 --- a/docs/k8s.md +++ b/docs/k8s.md @@ -203,12 +203,12 @@ spec: ## Health Checks -The deployment includes both probes on the `/health` endpoint: +The deployment's liveness probe calls `/health`; the readiness probe calls `/ready`: -- **Liveness probe**: Restarts the pod if the backend becomes unresponsive (30s interval, 3 failures) -- **Readiness probe**: Removes the pod from service during startup or issues (10s interval, 3 failures) +- **Liveness probe**: Restarts the pod if the backend becomes unresponsive (30s interval, 3 failures). `/health` returns 200 as soon as the TCP listener accepts connections, before startup finishes. +- **Readiness probe**: Removes the pod from service during startup or issues (10s interval, 3 failures). `/ready` returns 503 until the real router is wired in and the pod can serve real traffic, then 200. -The CLI launcher also performs an internal health check — it waits for the backend to be healthy before starting the web server. +The CLI launcher performs the same two-stage check: it waits for `/health` (the backend process is alive) and then for `/ready` (startup finished, the backend can serve real requests) before starting the web server. ## Scaling diff --git a/docs/plans/startup-listener-before-recovery/plan.md b/docs/plans/startup-listener-before-recovery/plan.md new file mode 100644 index 0000000000..c84cf32b48 --- /dev/null +++ b/docs/plans/startup-listener-before-recovery/plan.md @@ -0,0 +1,134 @@ +--- +spec: docs/specs/startup-listener-before-recovery/spec.md +created: 2026-08-22 +revised: 2026-08-23 +status: done +--- + +# Fix Plan: Bind and Answer Liveness Before Startup Recovery + +## Confirmed root cause + +`backendapp.startGatewayAndServe` starts the orchestrator at `main.go:877` and +reaches the first `net.Listen` only at `main.go:1064` -> `httpserver.go:76`. +`/health` is a route on the server built after that point (`helpers.go:716`), +so no socket exists while `Service.Start` runs. + +`Start` runs `reconcileTaskLifecycleTokens` synchronously (`service.go:2255`) +with four workers (`event_handlers_workflow.go:843`) and waits on a +`sync.WaitGroup`. Each task can reach `attemptColdResume`, which waits +`constants.AgentLaunchTimeout` = **15 minutes** (`timeouts.go:44`) for session +readiness (`task_operations.go:2500`), then 30s for prompt readiness, and +retries once (`task_operations.go:2235`). Worst case is roughly 31 minutes per +task, four at a time, against a 45s health window +(`internal/launcher/constants.go`). + +Corroborated empirically: a boot given a full 10-minute window still never +bound, producing only 2 ACP session creations in 10 minutes across 32 pending +tokens. Workers were parked in the session-ready wait, not cycling. + +## Approach + +**Bind the socket first; keep recovery exactly where it is.** + +An earlier revision of this plan proposed extracting the sweeps and running +them after the listener, under a deadline. That approach is rejected: + +- `attemptColdResume` and `reapAndReloadSession` use `context.WithoutCancel` + (`task_operations.go:2308`, `task_operations.go:2255`) specifically so a + resume survives caller cancellation. An external deadline would not stop an + in-flight resume, so the promised bound was fictional. +- The sweeps currently run *before* `watcher.Start` and `scheduler.Start` + (`service.go:2251` vs `service.go:2262`). Moving them after the listener + would race live watcher events, scheduler activity, GitHub polling, and API + mutations. The per-task mutex covers only the manual-move lifecycle body; it + does not serialise queued WIP reconciliation or dependency launches. + +So the fix changes **when the socket is bound**, not when recovery runs: + +``` +bind listeners early, serve a bootstrap handler + -> liveness endpoint returns 200 from this moment + -> every other route returns an explicit "starting" status +orchestrator.Start(ctx) unchanged position, unchanged ordering +buildHTTPServer(...) unchanged +swap the bootstrap handler for the real router +mark ready +``` + +## Correction to a claim in the earlier revision + +`workflowStore.ReconcileQueuedTasks` was listed as a third synchronous +launching sweep. That overclaims: it promotes and publishes `task.moved` +(`workflow_store.go:472`, `workflow_store.go:650`), but the watcher that +handles that event does not start until later in `Start` (`service.go:2262`), +so it does not itself synchronously reach the auto-start path. Its promotion is +instead picked up by the following lifecycle-token sweep through the durable +promotion marker. Two sweeps launch synchronously: +`reconcileTaskLifecycleTokens` and `reconcileDependencyLaunchesOnStartup` +(`event_handlers_dependencies.go:172`). This does not change the fix, which no +longer depends on classifying the sweeps. + +## Implementation waves and parallel candidates + +- [x] [Task 01: Bind listeners and serve liveness before startup work](task-01-bind-before-startup.md) (`done`) +- [x] [Task 02: Gate application routes behind an explicit starting state](task-02-gate-routes-while-starting.md) (`done`) +- [x] [Task 03: Guard the ordering against regression](task-03-ordering-guard.md) (`done`) + +No task is `parallel-safe`; all three touch the same startup sequence. + +## Validation results + +Implemented and validated across 5 Build/Verify/Review rounds; landed in PR +[#2944](https://github.com/kdlbs/kandev/pull/2944) +(`feature/backend-never-binds-k15` -> `main`). `/health` is now bound and +answering liveness immediately at listen time; `/ready` gates readiness +separately per `docs/specs/health-endpoint-version/spec.md`. Full command +receipts and round-by-round findings are recorded in the task's Kandev plan. + +## Regression test that must fail first + +`apps/backend/internal/backendapp/binds_before_startup_test.go` (new): run +startup with a collaborator that blocks inside `orchestrator.Start`, and assert +that the configured port accepts a TCP connection and answers the liveness +probe with 200 while `Start` is still blocked. + +Before Task 01 this fails: no socket exists. After Task 01 it passes. + +## Validation commands + +```bash +make -C apps/backend test +make -C apps/backend lint +cd apps/backend && go test ./internal/backendapp/... -run 'Bind|Ready|Start|Health' -count=1 +``` + +## Risks and guardrails + +- **Do not move, reorder, or parallelise the startup sweeps.** Their position + before `watcher.Start` is load-bearing. This plan deliberately leaves + `Service.Start` untouched. +- **Do not rely on cancelling recovery.** `context.WithoutCancel` makes that + ineffective; anything that claims to bound it must change the resume contract + first, which is out of scope. +- **Shutdown ordering is not what it looks like.** `awaitShutdown` stops the + orchestrator and lifecycle manager first (`helpers.go:1740`), then runs + cleanups (`helpers.go:1755`). Do not register the bootstrap listener's + teardown as a cleanup that must run before those stops; hand the early + listeners to the same shutdown path that owns the real ones so a single + socket is never closed twice or leaked. +- The liveness endpoint must not become a readiness endpoint. If it starts + reporting startup progress in its status code, the launcher will kill the + process again and this fix is undone. +- Binding before the router exists means an early request can arrive before any + route is registered. The bootstrap handler must answer every path + deterministically, never fall through to a nil handler. + +## Out of scope + +- The time a large board spends in "starting". Bulk recovery inheriting the + interactive 15-minute `AgentLaunchTimeout` per task is a real defect, but it + needs its own spec and a change to the `context.WithoutCancel` resume + contract. Recorded in the spec's follow-up section. +- WIP promotion, feeder pull, and manual-move lifecycle semantics. +- Launcher health-window configuration. diff --git a/docs/plans/startup-listener-before-recovery/task-01-bind-before-startup.md b/docs/plans/startup-listener-before-recovery/task-01-bind-before-startup.md new file mode 100644 index 0000000000..52b35bc3d5 --- /dev/null +++ b/docs/plans/startup-listener-before-recovery/task-01-bind-before-startup.md @@ -0,0 +1,83 @@ +--- +id: "01-bind-before-startup" +title: "Bind listeners and serve liveness before startup work" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/startup-listener-before-recovery/spec.md" +parallelism: sequential +--- + +# Task 01: Bind Listeners and Serve Liveness Before Startup Work + +## Intent + +Move socket binding ahead of orchestrator startup so the process can answer a +liveness probe while startup work is still running. `Service.Start` and the +startup sweeps are not touched. + +## Acceptance + +- Listeners are bound before `orchestratorSvc.Start(ctx)` is invoked + (`main.go:877`), using the same host and port resolution as today + (`serverListenAddr`, `startHTTPServers`). +- While startup work runs, the bound socket is served by a bootstrap handler + that answers the launcher's liveness path with 200. The launcher probes + `/health` (`internal/launcher/health.go`); confirm the exact path and method + before choosing what the bootstrap handler answers. +- The bootstrap handler answers **every** path deterministically. No path may + fall through to a nil handler or hang. +- Once `buildHTTPServer` returns, the real router replaces the bootstrap + handler on the already-bound listeners, with no window in which the socket is + closed and reopened and no second bind of the same port. +- The existing readiness flag (`ready`, consulted by the `/health` handler at + `main.go:179`) keeps its current meaning for the real router. Task 02 owns + making "starting" visible; this task must not make liveness depend on + startup progress. +- Shutdown closes each listener exactly once. Follow the existing ownership in + `awaitShutdown` (`helpers.go:1740`, `helpers.go:1755`) rather than adding a + cleanup that races the orchestrator stop. +- Bind failure still aborts startup with the same behaviour and logging as + today. + +## Suggested mechanism + +Serve an `http.Server` whose `Handler` is a small delegate holding an +`atomic.Pointer[http.Handler]`. It starts as the bootstrap handler and is +swapped to the Gin engine once built. This avoids closing and rebinding the +socket, and keeps a single shutdown owner. + +## Regression test (write first, must fail) + +`apps/backend/internal/backendapp/binds_before_startup_test.go` + +- Drive startup with a substituted orchestrator start function that blocks on a + channel the test controls, following the existing + `startOrchestratorAndAutomationConsumers` seam (`main.go:877`) already used + by `startup_order_test.go`. +- Assert a TCP connection to the configured port succeeds and the liveness path + returns 200 while the start function is still blocked. +- Release the block and assert the real router then serves a normal API route + on the same port. + +Expected pre-fix failure: the connection is refused; nothing is bound. + +## Files likely touched + +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/backendapp/httpserver.go` +- `apps/backend/internal/backendapp/binds_before_startup_test.go` (new) + +## Validation + +```bash +cd apps/backend +go test ./internal/backendapp/... -run 'Bind|Start|Health|Ready' -count=1 +make -C . lint +``` + +## Notes + +Do not modify `apps/backend/internal/orchestrator/service.go` in this task. The +sweeps stay where they are; only the bind moves. diff --git a/docs/plans/startup-listener-before-recovery/task-02-gate-routes-while-starting.md b/docs/plans/startup-listener-before-recovery/task-02-gate-routes-while-starting.md new file mode 100644 index 0000000000..def5642cb5 --- /dev/null +++ b/docs/plans/startup-listener-before-recovery/task-02-gate-routes-while-starting.md @@ -0,0 +1,67 @@ +--- +id: "02-gate-routes-while-starting" +title: "Gate application routes behind an explicit starting state" +status: done +wave: 2 +depends_on: ["01-bind-before-startup"] +plan: "plan.md" +spec: "../../specs/startup-listener-before-recovery/spec.md" +parallelism: sequential +--- + +# Task 02: Gate Application Routes Behind an Explicit Starting State + +## Intent + +Task 01 makes the socket answer while the application is still starting. A +caller must be able to tell that apart from a started backend, and must get a +deterministic answer rather than a hang. + +## Acceptance + +- Non-liveness requests arriving before startup completes receive an explicit + starting response with a stable status code (503 is the conventional choice) + and a machine-readable body, not a hang, a reset, or a 404. +- Readiness is exposed distinctly from liveness so an operator can tell "bound, + still starting" from "fully started". Prefer extending the existing `/health` + payload with a field over adding a new endpoint. +- **The liveness status code does not change while starting.** If the path the + launcher probes starts returning non-200 during startup, the crash loop + returns and Task 01 is undone. Add an explicit test asserting the launcher's + probe path returns 200 during the starting state. +- The starting state clears exactly when the real router takes over and the + existing `ready` flag flips; there is no third state that can persist. +- No new user-facing web copy. If any is added later it goes through `t()` per + the i18n rules. + +## Regression test (write first, must fail) + +Extend `apps/backend/internal/backendapp/binds_before_startup_test.go` or add +`apps/backend/internal/backendapp/starting_state_test.go`: + +- While startup is blocked, assert an API route returns the starting status and + a parseable body. +- In the same blocked state, assert the launcher's liveness path returns 200. +- After startup completes, assert the same API route serves normally and + readiness reports ready. + +## Files likely touched + +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/backendapp/helpers.go` (the `/health` payload, + around `helpers.go:716`) +- the corresponding `*_test.go` files + +## Validation + +```bash +cd apps/backend +go test ./internal/backendapp/... -count=1 +make -C . lint +``` + +## Notes + +Check `docs/specs/health-endpoint-version/spec.md` before changing the +`/health` response shape; that spec owns it and may need the same amendment +treatment the WIP-limit spec received. diff --git a/docs/plans/startup-listener-before-recovery/task-03-ordering-guard.md b/docs/plans/startup-listener-before-recovery/task-03-ordering-guard.md new file mode 100644 index 0000000000..0377e0d136 --- /dev/null +++ b/docs/plans/startup-listener-before-recovery/task-03-ordering-guard.md @@ -0,0 +1,63 @@ +--- +id: "03-ordering-guard" +title: "Guard the bind-before-startup ordering against regression" +status: done +wave: 3 +depends_on: ["01-bind-before-startup", "02-gate-routes-while-starting"] +plan: "plan.md" +spec: "../../specs/startup-listener-before-recovery/spec.md" +parallelism: sequential +--- + +# Task 03: Guard the Ordering Against Regression + +## Intent + +The defect is an ordering invariant, and ordering invariants rot silently. The +existing `startup_order_test.go` pins orchestrator-before-automation-before- +GitHub but says nothing about the bind. Pin the new invariant, and make the +cost of a slow startup visible instead of mysterious. + +## Acceptance + +- `apps/backend/internal/backendapp/startup_order_test.go` is extended to + assert that binding happens before orchestrator startup, in the same style as + the existing ordering assertions. +- Startup logs make a long start diagnosable without a goroutine dump: log when + the socket is bound, and when startup reconciliation begins and ends, with + elapsed time and the number of tasks the lifecycle sweep will process. +- A sweep still running after a generous interval logs at `warn` with the count + outstanding, so an operator sees "still recovering N tasks" rather than + silence. This is a log-only signal; it must not cancel anything, because + `context.WithoutCancel` means cancellation would not work + (`task_operations.go:2308`). +- The spec's follow-up section is carried into a tracked issue or a new spec + stub: bulk startup recovery inheriting the interactive 15-minute + `AgentLaunchTimeout` per task (`timeouts.go:44`) is a real defect this plan + deliberately does not fix. + +## Regression test (write first, must fail) + +Extend `apps/backend/internal/backendapp/startup_order_test.go` with an +assertion that the bind step is recorded before the orchestrator start step. +It fails against the pre-Task-01 ordering. + +## Files likely touched + +- `apps/backend/internal/backendapp/startup_order_test.go` +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/orchestrator/event_handlers_workflow.go` (sweep + start/end logging only; no behaviour change) + +## Validation + +```bash +cd apps/backend +go test ./internal/backendapp/... ./internal/orchestrator/... -count=1 +make -C . lint +``` + +## Notes + +Keep the sweep logging cheap. It runs before the watcher starts, so it cannot +publish events; plain structured logs are the right level here. diff --git a/docs/public/authentication.md b/docs/public/authentication.md index c9dbb9ff10..1e41a1089a 100644 --- a/docs/public/authentication.md +++ b/docs/public/authentication.md @@ -63,7 +63,7 @@ External MCP clients (Claude Code, Cursor connecting to `/mcp`) must be configur ## Endpoints that stay public -`/health` (readiness probes), the login/setup/invite pages, `GET /api/v1/features`, and self-authenticating webhook receivers (automation webhooks with `X-Webhook-Secret`, office channel HMAC webhooks). Everything else requires a session or token. +`/health` (liveness probes) and `/ready` (readiness probes), the login/setup/invite pages, `GET /api/v1/features`, and self-authenticating webhook receivers (automation webhooks with `X-Webhook-Secret`, office channel HMAC webhooks). Everything else requires a session or token. Plugin webhooks (`/api/plugins/{id}/webhooks/{key}`) are **not** public by default: a plugin's manifest must explicitly declare `webhooks[].public: true` for that specific webhook to accept anonymous requests. Unflagged webhooks require a session or PAT like any other endpoint. See [Plugin manifest reference](plugins-manifest.md). diff --git a/docs/public/cli.md b/docs/public/cli.md index 0a5a4fe48d..7a177e0bf8 100644 --- a/docs/public/cli.md +++ b/docs/public/cli.md @@ -128,8 +128,9 @@ On a normal start, the launcher: 3. selects backend and `agentctl` ports; 4. starts and supervises the backend; 5. derives one or more `/health` targets from the effective server binds; -6. waits up to 45 seconds for any target to return the launcher's health token; and -7. opens the reachable access URL in the default browser. +6. waits up to 45 seconds for any target to return the launcher's health token (the backend is alive and its socket is bound); +7. waits for `/ready` (the backend has finished startup recovery and can serve real requests); this wait is unbounded by design, since recovery can legitimately take much longer than 45 seconds; and +8. opens the reachable access URL in the default browser. The launcher remains in the foreground. Press `Ctrl+C` or terminate it to stop the backend and its managed children cleanly. A force-kill can leave worktree processes or containers running; inspect them before deleting data. diff --git a/docs/public/docker.md b/docs/public/docker.md index d0fc96dc26..3a040a43cf 100644 --- a/docs/public/docker.md +++ b/docs/public/docker.md @@ -231,7 +231,7 @@ Put `KANDEV_DB_PASSWORD` in a permission-restricted deployment secret, not shell ## Reverse proxy and network policy -The backend serves SPA, API, `/ws`, `/mcp`, and `/health` on one origin. Proxy the entire root path and preserve WebSocket upgrades. Caddy does this automatically: +The backend serves SPA, API, `/ws`, `/mcp`, `/health`, and `/ready` on one origin. Proxy the entire root path and preserve WebSocket upgrades. Caddy does this automatically: ```text kandev.example.com { @@ -263,17 +263,17 @@ Remote Docker profiles are not a workaround: that executor runtime is currently ## Health and observability -`GET /health` returns 503 during startup and 200 after routes are registered and the listener is accepting connections: +`GET /health` returns 200 as soon as the listener is accepting connections, even mid-startup; `GET /ready` returns 503 during startup and 200 after routes are registered: ```bash -curl --fail http://localhost:38429/health +curl --fail http://localhost:38429/ready ``` -It is a startup/readiness signal, not a deep database, Git, Docker, provider, or agent check. A Compose health check can still use it: +Neither is a deep database, Git, Docker, provider, or agent check. A Compose health check should use `/ready`, since Compose's single healthcheck concept (`docker ps` status, `depends_on: condition: service_healthy`) maps to "can serve real traffic," not just "process is alive": ```yaml healthcheck: - test: ["CMD", "curl", "-f", "http://localhost:38429/health"] + test: ["CMD", "curl", "-f", "http://localhost:38429/ready"] interval: 30s timeout: 5s retries: 3 @@ -306,7 +306,7 @@ Delete `kandev-data` only after verifying that its database, workspaces, and cre ## Troubleshooting -- **UI unreachable:** check `docker ps`, published address/port, host firewall, and `docker logs kandev`; then call `/health`. +- **UI unreachable:** check `docker ps`, published address/port, host firewall, and `docker logs kandev`; then call `/ready`. - **Permission denied under `/data`:** inspect mount ownership and root-squash behavior. The runtime user is UID 1000 after entrypoint setup. - **CLI login works only as root:** repeat it with `docker exec --user kandev`; repair ownership of the affected files before restarting. - **Image pull fails:** authenticate to GHCR if your network policy requires it and verify the tag/platform with `docker buildx imagetools inspect`. diff --git a/docs/public/k8s.md b/docs/public/k8s.md index c47a876120..eb56b0c4c4 100644 --- a/docs/public/k8s.md +++ b/docs/public/k8s.md @@ -194,7 +194,7 @@ The universal image already runs as `kandev`; use `kubectl exec -it deployment/k The example requests 250 millicores and 512 MiB, with limits of 2 CPU and 2 GiB. Those are placeholders, not capacity recommendations. Local/Worktree agents share the pod limit with the control plane and can exceed it during builds. Measure workload memory, CPU, ephemeral storage, PVC growth, and process counts; then set requests/limits accordingly. -Both example probes call `/health`. That endpoint returns 503 during startup and 200 once routes are wired and the TCP listener accepts connections. It is a readiness signal, not a deep check of database, repository, Docker, provider, or agent health. The supplied liveness probe therefore tests the same shallow condition. +The example liveness probe calls `/health`; the example readiness probe calls `/ready`. `/health` returns 200 as soon as the TCP listener accepts connections, before startup finishes: it confirms the process is alive, not that it can serve real traffic, so gating liveness on it never restarts a pod that is merely still starting up. `/ready` returns 503 until routes are wired, the agent registry is seeded, and (in e2e builds) the mock-harness routes are mounted, then 200. Neither is a deep check of database, repository, Docker, provider, or agent health. Long migrations or slow storage may need a startup probe to prevent premature liveness restarts: @@ -207,7 +207,7 @@ startupProbe: failureThreshold: 60 ``` -Tune from observed startup time. Keep readiness on `/health`; use separate external monitoring for dependencies and real workflows. +Tune from observed startup time. Keep liveness on `/health` and readiness on `/ready`; use separate external monitoring for dependencies and real workflows. diff --git a/docs/public/operations.md b/docs/public/operations.md index cda225ef06..bcd401f4d2 100644 --- a/docs/public/operations.md +++ b/docs/public/operations.md @@ -12,7 +12,7 @@ Kandev does not currently provide a user-login boundary for the web application, ## Quick path 1. Choose one process owner for the database and Kandev home. -2. Check `/health` for readiness and **System > Status** for diagnostics. +2. Check `/ready` for readiness and **System > Status** for diagnostics. 3. Back up the database and `master.key` before upgrades, resets, or recovery work. 4. Preserve Git branches and external provider state separately from database backups. @@ -58,19 +58,25 @@ if the new backend can provide it and a working task still exists. ## Health and readiness -Use the top-level readiness endpoint for supervisors and probes: +Use the top-level liveness endpoint for process supervisors: ```bash curl -fsS http://127.0.0.1:38429/health ``` -After startup it returns HTTP 200 with: +It returns HTTP 200 as soon as the listener accepts connections, with: ```json {"status":"ok","service":"kandev","mode":"websocket+http","version":"1.2.3"} ``` -It returns HTTP 503 with `status: "starting"` (plus the same `version`) until routes, the agent registry, and the listener are ready. The supplied Kubernetes probes use this endpoint. `/health` is unauthenticated even when auth is enabled, so it's also the credential-free way for monitoring to read the running version; there is no need to authenticate to **System > About** just to check what build is deployed. +`/health` never returns a non-2xx status while the process is alive, even mid-startup: it confirms the process is up, not that it can serve real traffic. Use the readiness endpoint instead when you need to know the backend can actually serve requests: + +```bash +curl -fsS http://127.0.0.1:38429/ready +``` + +It returns HTTP 503 with `status: "starting"` (plus the same `version`) until routes, the agent registry, and the listener are ready, then HTTP 200. The supplied Kubernetes liveness probe uses `/health`; the readiness probe uses `/ready`. Both are unauthenticated even when auth is enabled, so they're also a credential-free way for monitoring to read the running version; there is no need to authenticate to **System > About** just to check what build is deployed. For application diagnostics, open **Settings > System > Status** or request: @@ -313,7 +319,7 @@ The UI flow applies to the configured SQLite path: 2. Open **Settings > System > Backups**, choose **Restore**, type `RESTORE`, and confirm. 3. Kandev stops scheduling, active executions, and database-backed workers. It copies the selected snapshot to `.new`, validates the SQLite checkpoint result, and closes the pool. It then quarantines the configured database and its `-wal`/`-shm` sidecars before installing the staged file. If installation fails, Kandev restores the quarantined files. 4. Click **Restart Kandev** in the success dialog. If automatic restart is unavailable, quit and relaunch Kandev manually. The backend must restart before database-backed work resumes. -5. Check `/health`, **System > Status**, database schema version, secrets, and representative tasks. +5. Check `/ready`, **System > Status**, database schema version, secrets, and representative tasks. Restore does not roll back worktrees or remote/provider state. A database may therefore refer to files, containers, pull requests, or credentials from a different point in time. Reconcile them before restarting automation. @@ -433,7 +439,7 @@ Before any update, finish or stop active sessions, create and export a database - Transient npx: start the desired release with `npx -y kandev@latest`; this does not update a persistent package. - Docker/Kubernetes: replace the image and recreate the workload. Do not treat an in-container package install as a durable update. -After restart, verify `/health`, **System > About**, **System > Status**, the database page, and one non-destructive agent session. Kandev does not perform automatic binary or database rollback. If rollback is necessary, restore a compatible pre-upgrade database and matching application release together. +After restart, verify `/ready`, **System > About**, **System > Status**, the database page, and one non-destructive agent session. Kandev does not perform automatic binary or database rollback. If rollback is necessary, restore a compatible pre-upgrade database and matching application release together. ## Resource metrics @@ -474,8 +480,9 @@ drawer mirrors it as the saved left sequence followed by the saved right sequenc | Symptom | Check | Action | | --- | --- | --- | -| `/health` stays at 503 or cannot connect | Process-manager and launcher output | Confirm port ownership, database reachability, writable Kandev home, and required executables; then restart once | -| Status page says unhealthy while `/health` is 200 | `/api/v1/system/health` issue IDs | Fix Git, GitHub, agent discovery, or Linux inotify warning; readiness and application diagnostics have different meanings | +| `/health` cannot connect | Process-manager and launcher output | Confirm port ownership, database reachability, writable Kandev home, and required executables; then restart once | +| `/ready` stays at 503 while `/health` is 200 | Backend startup logs | Process is alive but still wiring routes, seeding the agent registry, or mounting test-harness routes; give it more time before restarting | +| Status page says unhealthy while `/ready` is 200 | `/api/v1/system/health` issue IDs | Fix Git, GitHub, agent discovery, or Linux inotify warning; readiness and application diagnostics have different meanings | | Backups page reports a 15-second create timeout | Reload the backup list and inspect the `backup-create` job/log | Large `VACUUM INTO` jobs can still finish; avoid double-clicking and ensure free disk | | Backup/maintenance fails on PostgreSQL | Active driver on Database page | Use `pg_dump`, provider snapshots, and PostgreSQL maintenance; System backup/vacuum/reset is SQLite-only | | Restored data looks stale | Whether the backend was restarted immediately | Quit/restart; do not keep using the old open database connections | diff --git a/docs/public/remote-cloud-environment.md b/docs/public/remote-cloud-environment.md index d8cfe8bae2..a63820a578 100644 --- a/docs/public/remote-cloud-environment.md +++ b/docs/public/remote-cloud-environment.md @@ -246,7 +246,7 @@ Review the warning from the OS-package phase. Install a compiler toolchain, `pkg Use the backend port from the latest startup banner, not the preferred port or the Vite port. Confirm locally inside the VM: ```bash -curl --fail http://127.0.0.1:38429/health +curl --fail http://127.0.0.1:38429/ready ``` Substitute the logged port. Then check the provider's port visibility, proxy path/WebSocket support, firewall, and process logs. Do not solve the problem by making an unauthenticated development port public. diff --git a/docs/public/run-as-a-service.md b/docs/public/run-as-a-service.md index 78cf260b20..ef1494914d 100644 --- a/docs/public/run-as-a-service.md +++ b/docs/public/run-as-a-service.md @@ -52,13 +52,13 @@ kandev service status kandev service logs ``` -The installer writes the managed unit or plist and starts the process. It does **not** poll `/health`; use `status` and `logs` first. The logs print the actual listener URL because the launcher selects a free fallback port when `38429` is unavailable. Copy that URL and append `/health`; for example, if the log reports port `43127`: +The installer writes the managed unit or plist and starts the process. It does **not** poll `/health`; use `status` and `logs` first. The logs print the actual listener URL because the launcher selects a free fallback port when `38429` is unavailable. Copy that URL and append `/ready`; for example, if the log reports port `43127`: ```bash -curl --fail http://127.0.0.1:43127/health +curl --fail http://127.0.0.1:43127/ready ``` -`/health` reports backend readiness after routes and the agent registry are initialized and the HTTP listener is accepting connections. It is not a deep health check of the database, message bus, executors, or remote providers. +`/ready` reports backend readiness after routes and the agent registry are initialized and the HTTP listener is accepting connections. It is not a deep health check of the database, message bus, executors, or remote providers. ### System service diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index c5862cb7f8..8ac1799465 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -35,5 +35,6 @@ The former legacy size exceptions for migrated sources were removed. All canonic The following specification was added after this migration and remains in the legacy layout until it moves into an owning system: - [Task Cost & Token Ledger](task-cost-ledger/spec.md) (draft) +- [Startup listener before recovery](startup-listener-before-recovery/spec.md) (draft) ## Authoring rule diff --git a/docs/specs/desktop/system-design/desktop-tauri-app.md b/docs/specs/desktop/system-design/desktop-tauri-app.md index f35424b94f..0176c79b8e 100644 --- a/docs/specs/desktop/system-design/desktop-tauri-app.md +++ b/docs/specs/desktop/system-design/desktop-tauri-app.md @@ -57,10 +57,13 @@ This increment preserves the shipped desktop contract: Vite/React SPA without requiring Node.js or a separate web server at runtime; - desktop and CLI/browser launches share the existing Kandev data directory, database, worktrees, executor settings, integrations, and agent configuration; -- the desktop shell owns the backend process tree, waits for authenticated health readiness, and - cleans up on startup failure, quit, update restart, or WebView failure; +- the desktop shell owns the backend process tree, waits for a two-stage readiness signal (a + token-verified `/health` proving process ownership, then an unauthenticated `/ready` proving + application readiness), and cleans up on startup failure, quit, update restart, or WebView + failure; - the backend binds to an app-selected loopback port, and the WebView navigates only after a - per-launch health token proves that the responding process is the backend the shell started; + per-launch health token first proves that the responding process is the backend the shell + started, and `/ready` then reports the backend has finished startup; - GUI launches retain the predictable process environment and common user binary locations needed to discover configured agent CLIs; - a second launch focuses the existing instance instead of starting another backend; and @@ -75,13 +78,18 @@ dependency. It selects an available loopback port, starts: kandev --headless --port ``` -with `KANDEV_SERVER_HOST=127.0.0.1`, and waits for `GET /health`. Each launch supplies a generated -`KANDEV_DESKTOP_HEALTH_TOKEN`; readiness requires the same value in the successful response's -`X-Kandev-Desktop-Health-Token` header before the WebView navigates to the backend origin. The -desktop shell also sets `KANDEV_DESKTOP_NATIVE_NOTIFICATIONS=true` to identify the launch as -desktop-owned. The nested native launcher must forward the shell's exact non-empty health token to -the backend and use it for its own readiness poll rather than replacing it with another token. A -second instance focuses the first and does not start another backend. +with `KANDEV_SERVER_HOST=127.0.0.1`, and waits for readiness in two stages. First it polls +`GET /health`, which flips to 200 as soon as the listener binds, before startup recovery +finishes; each launch supplies a generated `KANDEV_DESKTOP_HEALTH_TOKEN`, and this stage +requires the same value in the successful response's `X-Kandev-Desktop-Health-Token` header, +proving the responding process is the backend the shell started. Once `/health` passes, the +shell then polls unauthenticated `GET /ready` (which carries no health token), and only +navigates the WebView to the backend origin once `/ready` reports the backend has finished +startup. The desktop shell also sets `KANDEV_DESKTOP_NATIVE_NOTIFICATIONS=true` to identify the +launch as desktop-owned. The nested native launcher must forward the shell's exact non-empty +health token to the backend and use it for its own `/health` readiness poll rather than +replacing it with another token. A second instance focuses the first and does not start another +backend. The shell preserves the existing GUI-launch `PATH` normalization and the existing Kandev data directory, SQLite database, worktrees, executor settings, integrations, and agent configuration. @@ -199,8 +207,10 @@ X-Kandev-Desktop-Health-Token: ``` Desktop launches set `KANDEV_SERVER_HOST=127.0.0.1`. The health response must echo the per-launch -token before the WebView navigates to the loopback SPA. Existing backend HTTP, WebSocket, and boot -payload contracts remain the product API; native integration does not replace them. +token before the shell trusts the responding process; the WebView then navigates to the loopback +SPA only once the subsequent unauthenticated `GET /ready` reports the backend has finished +startup. Existing backend HTTP, WebSocket, and boot payload contracts remain the product API; +native integration does not replace them. ## State machines diff --git a/docs/specs/platform/system-design/health-endpoint-version.md b/docs/specs/platform/system-design/health-endpoint-version.md index 6f7ba0057e..64cbf058b2 100644 --- a/docs/specs/platform/system-design/health-endpoint-version.md +++ b/docs/specs/platform/system-design/health-endpoint-version.md @@ -155,13 +155,18 @@ Numbered, pass/fail, EARS-form. Each is directly testable against the handler. **Starting-path payload** -5. WHILE the readiness flag is unset, a `GET /health` request SHALL receive HTTP 503. -6. WHILE the readiness flag is unset, a `GET /health` request SHALL receive a JSON body +5. **SUPERSEDED, see [Amendment](#amendment-2026-08-22--liveness-readiness-split).** + ~~WHILE the readiness flag is unset, a `GET /health` request SHALL receive HTTP 503.~~ + `/health` now always answers 200; the not-ready 503 contract moved to `GET /ready` + (AC-16). +6. **SUPERSEDED, see [Amendment](#amendment-2026-08-22--liveness-readiness-split).** + ~~WHILE the readiness flag is unset, a `GET /health` request SHALL receive a JSON body containing a `version` key whose value equals the process's configured build version - string. -7. WHILE the readiness flag is unset, a `GET /health` response body SHALL contain exactly + string.~~ Moved to `GET /ready` (AC-17). +7. **SUPERSEDED, see [Amendment](#amendment-2026-08-22--liveness-readiness-split).** + ~~WHILE the readiness flag is unset, a `GET /health` response body SHALL contain exactly the keys `status`, `service`, and `version`, with `status` equal to `"starting"` and - `service` equal to `"kandev"`. + `service` equal to `"kandev"`.~~ Moved to `GET /ready` (AC-18). **Value correctness** @@ -179,9 +184,12 @@ Numbered, pass/fail, EARS-form. Each is directly testable against the handler. 12. WHEN authentication is enabled AND a `GET /health` request arrives with no credential, the backend SHALL respond with the full body including `version`, and SHALL NOT respond 401 or 403. -13. The change SHALL NOT alter the response status code, headers, or the `status` field +13. **SUPERSEDED, see [Amendment](#amendment-2026-08-22--liveness-readiness-split).** + ~~The change SHALL NOT alter the response status code, headers, or the `status` field value for any readiness state, so that an existing prober asserting only HTTP status - or `status` continues to pass unmodified. + or `status` continues to pass unmodified.~~ `/health`'s status code for the not-ready + state changed (503 → 200) by the amendment below; the original no-status-code-change + guarantee now applies to `GET /ready` instead (AC-19, AC-21). 14. WHEN authentication is enabled AND a `GET /api/v1/system/info` request arrives with no credential, the backend SHALL continue to reject it, confirming this change does not widen the allowlist. @@ -241,7 +249,7 @@ Every in-tree `/health` consumer was checked against an additive body field. | Desktop smoke test | `apps/desktop/e2e/desktop-launch-smoke.mjs` | URL match on a fake runtime | None | | Playwright fixture | `apps/web/e2e/fixtures/backend.ts:461` | Waits for HTTP readiness | None | | Homebrew service | `scripts/release/kandev.rb` | Polls the URL | None | -| Docker docs example | `docs/docker.md` | `curl -f`, status only | None | +| Docker docs example | `docs/docker.md` | `curl -f`, status only | None at the time; later repointed to `/ready` by the Amendment below | | Public operations doc | `docs/public/operations.md:70` | **Pins the exact JSON body** | **Must be updated — AC-15** | No consumer deserializes the body into a fixed struct that would reject an unknown field, @@ -307,11 +315,123 @@ Resolved with the requester on 2026-08-07, before this spec was frozen. (AC-15). It is the only doc that reproduces the payload verbatim. - `docs/docker.md`, `docs/k8s.md`, `docs/run-as-a-service.md`, `docs/public/cli.md`, and `docs/public/authentication.md` reference `/health` but do not reproduce its body, so - they need no change. + they need no change for this base spec. (Superseded for `docs/docker.md` and + `docs/k8s.md` by the Amendment below: the liveness/readiness split gave them a second + endpoint to document, which is a structural change independent of the body-shape + question this bullet answers. `docs/run-as-a-service.md` still needs no change from + either spec; it has its own pre-existing `/health` wording inconsistency that predates + both this spec and the Amendment and is tracked separately, not fixed here.) - Consider noting in `docs/public/operations.md` that `/health` is now the credential-free way to read the running version, since that is the operator-visible benefit. +## Amendment (2026-08-22) — Liveness/Readiness Split + +Made during the `backend-never-binds` DEFECT fix +(`docs/specs/startup-listener-before-recovery/spec.md`), by nova28, after Review +flagged that fix's bootstrap handler as violating this spec's frozen AC-5/6/7/13. + +**Why.** `startup-listener-before-recovery` binds the HTTP listener and serves a +bootstrap handler *before* orchestrator startup recovery sweeps run, so the +launcher's liveness probe (`internal/launcher/health.go`) never times out +waiting for a socket that startup recovery is blocking. That bootstrap handler +answers `GET /health` 200 unconditionally, because the launcher's healthcheck +must succeed regardless of startup progress — gating it on readiness is exactly +the crash-loop bug that fix exists to close. But this spec's AC-5/6/7/13 require +`/health` to answer 503 while the readiness flag is unset, and the real router +(`healthHandler`, `internal/backendapp/helpers.go`) enforced that. The two +requirements are mutually exclusive on one endpoint: `/health` cannot be both +"always 200 so liveness never flaps" and "503 until ready so readiness probers +wait." Reordering `ready.Store`/`handler.Store` (the fix's own R1-2 correction) +only fixes *when* the swap between the bootstrap and real handler is safe; it +does not resolve this contract conflict, because the real handler's `!ready` +branch is what conflicts with the bootstrap handler's unconditional 200. + +**Resolution.** Split the concerns onto two endpoints, matching standard +liveness/readiness terminology (Kubernetes' own naming for the same +distinction): + +- **`GET /health` becomes a pure liveness probe.** It answers 200 as soon as + the listener is bound — before, during, and after startup — and never gates + on the `ready` flag. This is now true in **both** the bootstrap handler + (`internal/backendapp/httpserver.go`) and the real router's `healthHandler` + (`internal/backendapp/helpers.go`), so the endpoint's behavior is identical + across the handler swap. All of this spec's ready-path ACs (AC-1..4, AC-8..12, + AC-14, AC-15) continue to apply to `/health`'s response, since it still always + returns the shape those ACs describe (`status: "ok"`, `service`, `mode`, + `version`) — the only change is that shape is now served for *every* request, + not conditionally. +- **`GET /ready` is a new endpoint that inherits `/health`'s original readiness + contract.** It is registered only on the real router (`helpers.go`), not the + bootstrap handler — while the bootstrap handler is in effect, any non-`/health` + path (including `/ready`) already falls through to its existing 503 + `"starting"` branch (`internal/backendapp/httpserver.go`), so `/ready` + correctly reports not-ready during that window with zero bootstrap-handler + code change. Once the real router is swapped in, `/ready` gates on the same + `ready` flag `/health` used to. + +**New acceptance criteria** (supersede AC-5, AC-6, AC-7, AC-13 for `/health`; +apply to `/ready`): + +16. WHILE the readiness flag is unset, a `GET /ready` request SHALL receive + HTTP 503. +17. WHILE the readiness flag is unset, a `GET /ready` request SHALL receive a + JSON body containing a `version` key whose value equals the process's + configured build version string. +18. WHILE the readiness flag is unset, a `GET /ready` response body SHALL + contain exactly the keys `status`, `service`, and `version`, with `status` + equal to `"starting"` and `service` equal to `"kandev"`. +19. WHEN the readiness flag is set, a `GET /ready` request SHALL receive HTTP + 200 and a JSON body containing exactly the keys `status`, `service`, and + `version`, with `status` equal to `"ok"` and `service` equal to `"kandev"`. + `/ready`'s 200 body deliberately omits `mode` — it is not a duplicate of + `/health`, it exists solely to answer "can this backend serve real + traffic yet." +20. A `GET /health` request SHALL receive HTTP 200 regardless of the readiness + flag's state, once the listener is accepting connections. This formalizes + the bootstrap-handler behavior `startup-listener-before-recovery` depends + on and is the reason AC-5/6/7 are superseded rather than merely relocated + unchanged. +21. The desktop health-token response header (AC in the original "## What" + section) SHALL be set only on `GET /health`, never on `GET /ready` — the + token contract belongs to the desktop launcher's liveness probe + (`internal/launcher/health.go`), which does not consult `/ready`. +22. `GET /ready` SHALL be on the unauthenticated allowlist + (`internal/auth/httpmw/middleware.go`), identically to `/health`, so + Kubernetes' `readinessProbe` and the e2e fixture's readiness wait do not + need a credential. + +**Updated Consumer Compatibility Audit** (rows changed from the original +table): + +| Consumer | File | How it reads readiness | Impact | +|---|---|---|---| +| K8s readinessProbe | `k8s/deployment.yaml` | HTTP status only, now against `/ready` | Repointed | +| K8s livenessProbe | `k8s/deployment.yaml` | HTTP status only, stays on `/health` | None | +| Playwright fixture | `apps/web/e2e/fixtures/backend.ts` | Waits for HTTP readiness, now against `/ready` | Repointed | +| Go launcher | `internal/launcher/health.go` | Status code against `/health`; body discarded | None — still liveness, unaffected | +| CLI, Desktop shell, Homebrew service | (unchanged files) | Status/socket against `/health` | None — liveness consumers, correctly unaffected | +| Docker Compose healthcheck example | `docs/docker.md` | Was `curl -f .../health`, status only | Repointed to `/ready` (Review round 3, R3-1 finding) — Compose's single healthcheck concept means "can serve real traffic," which is `/ready`'s contract, not `/health`'s | +| K8s docs prose | `docs/k8s.md` | Described "both probes on `/health`" | Corrected to match `k8s/deployment.yaml`: liveness stays `/health`, readiness is `/ready` (Review round 3, R3-3 finding) | +| Playwright fixture docs | `docs/test_e2e_web.md` | Described waiting for `/health` | Corrected to `/ready`, matching `apps/web/e2e/fixtures/backend.ts`'s actual wait (Review round 3, R3-3 finding) | + +**Updated Out of Scope note.** The original spec's "Renaming, moving, or +changing the auth posture of any endpoint" bullet refers to `/health` itself, +which is unchanged in name, route, or auth posture by this amendment. `/ready` +is a new endpoint, not a rename — adding it does not conflict with that bullet. + +**Updated Files Reference** (in addition to the original table): + +| File | Change | +|---|---| +| `apps/backend/internal/backendapp/helpers.go` | `healthHandler` no longer gates on `ready`; new `readyHandler` gains the original 503/200 gate | +| `apps/backend/internal/backendapp/httpserver.go` | No change required — the bootstrap handler's existing "any non-`/health` path" 503 branch already covers `/ready` | +| `apps/backend/internal/auth/httpmw/middleware.go` | Added `/ready` to `isPublicPath`, mirroring `/health` | +| `k8s/deployment.yaml` | `readinessProbe.httpGet.path` changed to `/ready`; `livenessProbe` stays `/health` | +| `apps/web/e2e/fixtures/backend.ts` | Readiness waits repointed to `/ready` | +| `docs/public/{operations,k8s,docker,run-as-a-service,authentication,remote-cloud-environment}.md` | Updated to describe the liveness/readiness split | +| `docs/docker.md`, `docs/k8s.md`, `docs/test_e2e_web.md` | Updated (Review round 3 / Build round 4, not the initial amendment pass) to repoint their `/health`-only readiness references to `/ready` | + ## Rollback Revert the handler change and the doc change. No migration, no persisted state, no config diff --git a/docs/specs/startup-listener-before-recovery/spec.md b/docs/specs/startup-listener-before-recovery/spec.md new file mode 100644 index 0000000000..ec2a639cc0 --- /dev/null +++ b/docs/specs/startup-listener-before-recovery/spec.md @@ -0,0 +1,114 @@ +--- +status: draft +created: 2026-08-22 +revised: 2026-08-22 +type: repair +--- + +# The Backend Must Bind and Answer Liveness Before Startup Recovery Runs + +## Problem + +The backend does not bind its HTTP socket until orchestrator startup +reconciliation has finished, and that reconciliation can wait on agent +processes for hours. Under the launcher's health window the process is killed +before it ever listens, restarted, and the cycle repeats without converging. + +`backendapp.startGatewayAndServe` calls `orchestrator.Service.Start` at +`main.go:877` and reaches the first `net.Listen` only via `main.go:1064` -> +`httpserver.go:76`. `/health` is a route on the server built after that point +(`helpers.go:716`); no earlier path serves it. So while `Start` runs there is +no listener at all, which is exactly what `lsof` showed. + +`Start` runs `reconcileTaskLifecycleTokens` synchronously +(`service.go:2255`), fanning four workers over every task with a pending +lifecycle token and waiting on a `sync.WaitGroup`. The per-task work reaches +`autoStartStepPrompt` -> `PromptTask` -> `ensureSessionRunning` -> +`attemptColdResume`. + +**The per-task cost is far larger than the prompt-ready timeout suggests.** +`attemptColdResume` first waits for session readiness via `waitForSessionReady` +(`task_operations.go:2498`), whose budget is `constants.AgentLaunchTimeout` = +**15 minutes** (`timeouts.go:44`, 10m setup script + 5m allowance). Only after +that does the 30s `waitForAgentPromptReady` apply, and the attempt is retried +once. Worst case is roughly 31 minutes per task, four at a time. + +Measured on the affected instance: 424 tasks, 32 carrying +`manual_move_lifecycle` metadata, on a 1.0 GB database. A boot given a full +10-minute health window still never bound, and produced only 2 successful ACP +session creations in those 10 minutes. That is the signature of workers parked +in the 15-minute session-ready wait, not of workers cycling through tasks. + +Three consequences, in severity order: + +1. **Socket liveness is coupled to agent readiness.** The process cannot answer + a health probe, serve the UI, or expose the API until agents that may never + become ready are waited out. +2. **The failure is self-sustaining.** `healthTimeoutReleaseMS` is 45s + (`internal/launcher/constants.go`). The launcher declares a backend health + failure and stops the process; a supervisor restarts it; the same tokens are + still pending because none were cleared, and the runtimes orphaned by the + kill are added to the next boot's work. Ten consecutive boots were observed, + none of which bound a socket. +3. **The wait cannot be bounded from outside.** `attemptColdResume` and + `reapAndReloadSession` deliberately use `context.WithoutCancel` + (`task_operations.go:2308`, `task_operations.go:2255`) so a resume survives + its caller's cancellation. A deadline handed to the sweep therefore does not + stop an in-flight resume. Any fix that relies on cancelling the sweep to + protect startup is ineffective. + +This is startup ordering, not a defect in the recovery logic. The same frames +are reached whenever a lifecycle token is pending. + +## Broken behavior + +- No socket is bound, and `/health` cannot be answered, until orchestrator + startup reconciliation returns. +- A backend whose recovery exceeds the health window is killed and restarted + with at least as much recovery work as before, indefinitely. + +## Desired behavior + +- The backend binds its socket and answers a liveness probe within the release + health window, regardless of how much recovery work is pending. +- Startup recovery keeps its current position and ordering: it still runs + before the watcher and scheduler start, so it is not exposed to concurrent + watcher events, scheduler activity, or API mutations. +- Requests that need a started application are answered with an explicit + "starting" response rather than by a hung connection or a refused socket. +- Readiness is observable and distinct from liveness: an operator can tell + "bound, still starting" from "fully started". + +## Regression scenarios + +- **GIVEN** a backend with pending lifecycle tokens whose agent sessions never + reach ready, **WHEN** it starts, **THEN** the socket is bound and the + liveness probe returns success well inside `healthTimeoutReleaseMS`, and the + launcher does not shut the process down. +- **GIVEN** the same backend while startup reconciliation is still running, + **WHEN** an API route is requested, **THEN** it returns an explicit starting + status rather than hanging, and readiness reports not-ready while liveness + reports healthy. +- **GIVEN** startup reconciliation that has completed, **WHEN** readiness is + requested, **THEN** it reports ready and all routes serve normally. + +## Out of scope + +- Reordering, parallelising, or moving startup reconciliation relative to the + watcher and scheduler. Its current position is load-bearing and moving it + introduces races with live traffic. +- Changing WIP promotion, feeder pull, or manual-move lifecycle semantics. +- Changing `AgentLaunchTimeout`, `agentPromptReadyTimeout`, or the cold-resume + retry shape. See the follow-up note below. +- Raising `healthTimeoutReleaseMS`, which hides the coupling instead of fixing + it. Empirically a 10-minute window did not help. + +## Known follow-up, deliberately separate + +Fixing the ordering stops the crash loop but does not make a large board start +quickly: a backend with many pending tokens still spends a long time in +"starting" because bulk recovery inherits the interactive 15-minute +`AgentLaunchTimeout` per task. A bulk startup sweep needs its own, much +smaller per-task budget, and a resume path that can actually be abandoned. That +requires changing the `context.WithoutCancel` resume contract and is a separate +piece of work with its own spec. diff --git a/docs/specs/tasks/system-design/wip-limit-pull-system.md b/docs/specs/tasks/system-design/wip-limit-pull-system.md index c25315c53a..40afdb3b8f 100644 --- a/docs/specs/tasks/system-design/wip-limit-pull-system.md +++ b/docs/specs/tasks/system-design/wip-limit-pull-system.md @@ -304,6 +304,11 @@ promotion. only after the committed promotion is observable. - Startup reconciliation resumes queued promotion without depending on a new watcher poll or browser connection. +- Startup reconciliation keeps its position before the watcher and scheduler + start, but it never delays the HTTP socket from binding: resuming a promotion + can launch an agent, and agent readiness is not bounded by the launcher's + health window. See + [`startup-listener-before-recovery`](../../startup-listener-before-recovery/spec.md). ## Scenarios diff --git a/docs/test_e2e_web.md b/docs/test_e2e_web.md index 87c2acae3b..ba088c2c26 100644 --- a/docs/test_e2e_web.md +++ b/docs/test_e2e_web.md @@ -105,7 +105,8 @@ Each Playwright worker spawns an isolated `kandev` backend that also serves the - `KANDEV_DOCKER_ENABLED=false` — no Docker - `KANDEV_WORKTREE_ENABLED=false` — no worktrees - `GH_TOKEN` / `GITHUB_TOKEN` stripped — prevents accidental real API calls -3. Spawns `apps/backend/bin/kandev __backend` and waits for `/health` to return 200 +3. Spawns `apps/backend/bin/kandev __backend` and waits for `/ready` to return 200 (not `/health`, + which flips green as soon as the listener is bound, before the mock-harness routes are mounted) **Frontend:** 1. Sets `KANDEV_WEB_DIST_DIR` to `apps/web/dist` diff --git a/k8s/deployment.yaml b/k8s/deployment.yaml index 3c863e4542..a85acd74b3 100644 --- a/k8s/deployment.yaml +++ b/k8s/deployment.yaml @@ -44,7 +44,7 @@ spec: failureThreshold: 3 readinessProbe: httpGet: - path: /health + path: /ready port: backend initialDelaySeconds: 10 periodSeconds: 10 diff --git a/scripts/release/kandev.rb b/scripts/release/kandev.rb index d811d83bc5..caad71306a 100644 --- a/scripts/release/kandev.rb +++ b/scripts/release/kandev.rb @@ -45,10 +45,14 @@ class Kandev < Formula ENV["KANDEV_SERVER_HOST"] = "127.0.0.1" port = free_port pid = spawn bin/"kandev", "--headless", "--port", port.to_s - health_url = "http://127.0.0.1:#{port}/health" + # /health flips to 200 as soon as the listener binds, before startup + # recovery finishes, so curl's --retry (which only retries on + # connection-refused/5xx, never on 200) would stop waiting immediately. + # /ready holds at 503 until the app is actually usable. + ready_url = "http://127.0.0.1:#{port}/ready" curl = "curl --silent --show-error --fail --retry 30 --retry-connrefused --retry-delay 1" - health = shell_output("#{curl} #{health_url}") - assert_match '"status":"ok"', health + ready = shell_output("#{curl} #{ready_url}") + assert_match '"status":"ok"', ready assert_match "Kandev", shell_output("#{curl} http://127.0.0.1:#{port}/") ensure Process.kill("TERM", pid) if pid