fix(cli): report functions serve outcomes from real container state (CLI-2426)

[Prashansa-K]

TL;DR

supabase functions serve reported almost every session as a failure, and almost none of those failures carried a usable classification. This makes the command report what actually happened.

Refs CLI-2426

What was wrong

Outcomes that were not failures were reported as failures. A container that stopped on its own with exit 0 was returned as Effect.fail. A container torn down by a supervisor — supabase stop in another terminal stops every project container — exited 143 and was charged to the internal-bug counter. Both are ordinary ways for a serve session to end.

Decisions were made from the wrong source. streamContainerLogs judged whether the Docker daemon was reachable, and whether a stream failure was retriable, by substring-matching the tail of the container's own logs. That text is arbitrary user function output: a function logging "Cannot connect to the Docker daemon" earned a Docker suggestion, and one logging "no such container" could turn a fatal stream failure into an endless retry. A private inspectContainerState shadowed an existing shared helper, parsed a different wire format, and failed with bare Errors.

A clean docker logs -f exit was read as a stopped container. The code checked only .State.ExitCode, which is also 0 while a container runs, so a daemon that closed the stream underneath a healthy runtime got it torn down and reported as a graceful stop.

The check that runs on every invocation carried no classification. assertLocalDbRunning failed with bare Errors. Those have no tag and no actionability declaration, so they fall through to unknown — which is most of what this command's failures looked like in telemetry.

The shutdown race is real, but only on Windows. The Effect spawner defaults to detached off Windows and spawnContainerCli never overrides it, so the docker logs -f child runs in its own session and never receives the terminal's SIGINT — the two arms of the race cannot become ready together there. On Windows the default flips and CTRL_C_EVENT reaches every console-attached process, so the tie-break does matter. An earlier revision of this description had that backwards.

What changed

Container state decides the outcome. streamContainerLogs inspects Running and ExitCode together through the shared inspectContainerState, and re-attaches when the container is still alive. A container that has already been removed ends the session instead of surfacing raw Docker text.

Ordinary endings end ordinarily. Exit 0 and the supervisor-teardown codes (129/130/131/143) end the session successfully, each with its own message so scrollback can tell "I stopped it" from "the runtime walked out". Real crash signals still fail as runtime_crash. 137 keeps its existing retry.

Classification comes from Docker's own output. daemonDown and the retry/fatal decision are derived from a follow-up docker container inspect, never from the log tail. No container log text is emitted to telemetry.

The failure paths carry tagged identities — including the local-database pre-flight, which is the check every invocation runs: a missing stack suggests supabase start, an unreachable daemon suggests starting Docker. A daemon that dies mid-session now says "start Docker" rather than "install Docker Desktop", which is what it said before despite having just created and started a container.

Re-attaches are bounded. They resume with --since instead of replaying the container's whole log history, are capped, and run in their own scope so child handles stop accumulating for the session.

Shutdown is latched once for the loop's lifetime, so a signal arriving between races is not dropped, and the grace period guards both the startup race and the log-stream race — a cold image pull is where a Windows user is most likely to reach for Ctrl-C. Both timers are injectable, so their tests do not race a real clock.

New runtime_crash category. A runtime we launch and supervise can die on its own; panic (the CLI threw) and impossible_state (unreachable branch) don't describe that, so these were filed as unknown. Note this makes the internal-bug counter-metric go up. That reading is worse and correct: those failures were always ours, just mis-filed.

Known gaps, deliberately not here

• Exit 137 is still swallowed by a retry loop. The container has no --restart policy. Filed separately.
• gen types has the same untagged local-stack pre-flight failure. Filed separately.
• time_to_ready_ms for functions serve — next PR.

🤖 Generated with Claude Code

[github-actions]

🤖 AI Review

Both independent reviews completed. After verification, all 11 deduplicated findings are confirmed: one major false-success path, four runtime/error-handling issues, one stale compatibility contract, and five test/comment maintainability nits.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	apps/cli/src/shared/functions/serve.ts:1327	correctness	codex	A clean docker logs -f exit can make the command treat a still-running container as gracefully stopped because it checks only .State.ExitCode, which is also zero while a container is running.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:1332	telemetry-classification	claude	Every inspected non-zero container exit except 137 is classified as an internal runtime_crash, including signal-derived exits such as 143 that can result from an external docker stop.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:1981	race-condition	codex	A filesystem restart arriving during the 50 ms shutdown grace period can win the outer race and suppress a log-stream or container failure that occurred first.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:1974	concurrency	claude	The shutdown signal is latched only within each log-stream race, leaving gaps between startup, race iterations, and teardown in which a held SIGINT or SIGTERM is discarded.
🟡 MINOR	apps/cli/src/shared/functions/serve.errors.ts:42	error-handling	claude	The daemon-down error advertises a canonical start_docker suggestion to telemetry but carries no user-visible remediation text.
🟡 MINOR	apps/cli/src/commands/functions/serve/SIDE_EFFECTS.md:66	documentation	claude+codex	The compatibility contract omits the new successful exit when the edge-runtime container stops with code zero.
⚪ NIT	apps/cli/src/commands/functions/serve/serve.integration.test.ts:2085	test-coverage	claude	The Ctrl-C regression test does not exercise the grace interval because its queued signal is retained and can resolve the shutdown latch without any delay.
⚪ NIT	apps/cli/src/commands/functions/serve/serve.integration.test.ts:2114	maintainability	claude+codex	The assertion embeds literal ESC control bytes instead of visible Unicode escape sequences.
⚪ NIT	apps/cli/src/shared/functions/serve.ts:127	comment-policy	claude+codex	The new timing and race comments exceed the repository's limit for inline rationale and substantially narrate the implementation.
⚪ NIT	apps/cli/src/commands/functions/serve/serve.integration.test.ts:2048	comment-policy	claude+codex	The test suite name uses an internal ticket identifier as provenance.
⚪ NIT	apps/cli/src/commands/functions/serve/serve.integration.test.ts:2086	comment-policy	claude	The new fixture comment is eight lines long and uses the specifically banned emphasis word exactly.

Findings outside the diff

• 🟡 MINOR apps/cli/src/commands/functions/serve/SIDE_EFFECTS.md:66 — The compatibility contract omits the new successful exit when the edge-runtime container stops with code zero.

Stats

Claude findings: 7 · Codex findings: 6 · Confirmed: 11 · Refuted: 0 · Uncertain: 0

---

Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

[Coly010]

Verdict: Request changes. The mechanical pieces here are good and I want most of them merged — the hoisted Deferred, the tagged errors, exit 0 on a clean container exit, daemonDown. What I can't approve is the root-cause story: it only holds on Windows, and the telemetry the PR cites points at a different bucket that this PR leaves untouched.

1. The docker logs -f child is not in the CLI's process group on macOS/Linux

The PR body and serve.ts:127 say spawnContainerCli never passes detached, so the child shares the process group and one Ctrl-C hits both arms. The spawner's default is the opposite:

// @effect/platform-node-shared/dist/internal/nodeChildProcessSpawner.js
const detached = options.detached ?? platform !== "win32";

Verified empirically on Bun 1.3.14 through BunServices.layer with the same options object streamContainerLogs passes ({ stdin: "ignore", stdout: "pipe", stderr: "pipe", extendEnv: true }):

{"via":"effect-spawner-default","childPgid":"5118","parentPgid":"5091","sameGroup":false}
{"via":"effect-spawner-detached-false","childPgid":"5091","parentPgid":"5091","sameGroup":true}

A detached child is in its own session, so the tty's SIGINT never reaches it. The race this PR arbitrates with a 50 ms sleep cannot occur on macOS/Linux — and the codebase already knows this: go-proxy.layer.ts:180-183 sets detached: false precisely so Ctrl-C does reach the Go child. On Windows the default flips to detached: false and CTRL_C_EVENT goes to every console-attached process, so there the race is real.

2. What the 30-day telemetry actually shows for functions serve

cli_command_executed, current versions only (2.116.0 + 2.117.0), os × outcome:

os	outcome	runs	devices	p50 duration	p90	in a TTY
darwin	success	41,446	4,454	41.9 s	2,105 s	12%
darwin	error:unknown	40,811	2,792	0.0 s	48.5 s	4%
linux	error:unknown	406,963	11,059	0.1 s	0.3 s	0.2%
linux	success	32,322	17,924	24.7 s	504 s	4%
win32	error:unknown	11,639	3,432	141.6 s	12,394 s	22%
win32	success	345	111	279.8 s	7,391 s	99%

• On macOS/Linux the unknown failures die in well under a second and almost never in a TTY. Those are startup failures, not Ctrl-C. The only sub-second path that runs on every invocation and fails with a bare Error is assertLocalDbRunning — serve.ts:1388 (supabase start is not running.) and :1400-1401 (failed to inspect service: …, including the daemon-down branch). That is most likely the bucket CLI-2426 is actually about, and this PR doesn't tag it. (Inferred from timing — the fingerprint carries no message — but two tagged errors there would settle it in a week of data.)
• The fingerprint a signal-killed docker logs child would leave (tag:PlatformError:platform_error — exitCode fails on code === null) appears 34 times on darwin in 30 days, against ~88k error:unknown.
• Windows is the one platform whose unknowns are long-lived sessions, consistent with the race as described. It is ~3% of runs.

3. What this means for the PR

Keep the mechanism; fix the explanation and close the gap the data points at:

1. serve.ts:127, serve.integration.test.ts:2086 and the PR body should describe a Windows console-signal tie-break. If the 50 ms grace stays for Windows (harmless elsewhere), apply it to the startup race too — a Windows user sits on Ctrl-C during docker pull, not during log streaming.
2. Tag the two assertLocalDbRunning failures (a user_actionable "run supabase start" and the existing dockerNotRunning preset) — here or in a follow-up opened before this merges. Without them the KPI-1 goal in CLI-2426 doesn't move.
3. The inline items below: the duplicated inspectContainerState, the re-attach log replay, the 143/cancelled inconsistency, the wrong Docker suggestion text, and the SIDE_EFFECTS drift.

Stress-tested the two timing tests locally: 8 unloaded runs plus 6 under 2× CPU saturation, all green. Nice work on the Deferred hoist — that was a real dropped-signal bug.