fix(stack): complete runtime and test resource cleanup

[jgoux]

Local stack shutdown could leave native processes, containers, database helpers, or attached tools running after cleanup returned. Runtime handles now retain cleanup ownership through partial launches and failures, and orderly shutdown waits for admitted work, service and tool cleanup, container absence, and the detached host's exit. Failed cleanup remains observable and retryable; destroy removes saved state only after resource cleanup succeeds.

Container cleanup runs synchronously in the existing stack lifecycle, with no separate sentinel process. Startup and shutdown use the same sweep restricted to the stack ID and canonical data directory, keeping parallel stacks isolated. SIGTERM and SIGINT follow the ordinary stop path. Stop preserves service data; force-killing a host may require manual cleanup.

Native startup helpers use temporary OS-assigned ports, and serving ports stay reserved together until launch configuration is complete. This prevents helper port collisions and duplicate port assignment within a service.

[github-actions]

Superseded by a newer AI review

🤖 AI Review

Verified all 11 reported findings and merged the shared shutdown issue, yielding 10 confirmed findings. The two major issues are loss of host retryability when cleanup defects and possible container orphaning when identity recovery fails. The remaining findings concern native cleanup idempotence, test reliability/coverage, and readability/performance nits.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	packages/stack/src/StackHost.ts:412	error-handling	claude+codex	Cleanup defects bypass the shutdown retry loop and can terminate the stack host instead of retaining ownership for retry.
🟠 MAJOR	packages/stack/src/runtime/Container.ts:251	resource-cleanup	codex	If the identity-recovery query fails after container creation completed, the launch exits without registering cleanup authority and can orphan the container.
🟡 MINOR	packages/stack/src/runtime/NativeProcess.ts:260	error-handling	claude	Native cleanup lacks an already-stopped guard, so scope closure invokes killProcess again after an explicit stop; a failure during that second cleanup becomes a defect and bypasses the service cleanup-error path.
🟡 MINOR	packages/stack/src/runtime/native-process.integration.test.ts:175	test-quality	claude	The new descendant-cleanup tests can fail transiently because assertExited probes each PID only once immediately after sending SIGKILL.
🟡 MINOR	packages/stack/src/runtime/native-process.integration.test.ts:426	test-quality	claude	The finalizer-failure test asserts only that some failure occurred, without proving that the injected process-group cleanup error caused it.
🟡 MINOR	packages/stack/src/services/ProcessRecipe.integration.test.ts:31	test-coverage	claude	The new ProcessRecipe cleanup suite covers only the container runtime, leaving the newly added native ServiceLaunchError paths untested.
🟡 MINOR	packages/stack/src/runtime/Container.integration.test.ts:393	test-cleanup	codex	The fallback test cleanup ignores a nonzero docker rm exit code and can silently leak test containers.
⚪ NIT	packages/stack/src/runtime/Container.ts:226	readability	claude	The recovery logic uses Exit.succeed("") as a sentinel for "recovery not attempted," obscuring the distinction from an attempted query that returned no identity.
⚪ NIT	packages/stack/src/services/ProcessRecipe.ts:435	readability	claude	The ContainerLaunchError handler names its destructured process field process, shadowing the enclosing startup-loop variable and the Node global.
⚪ NIT	packages/stack/src/runtime/Container.ts:229	performance	claude	The recovery query runs in the uninterruptible region, so cancellation can be delayed by an additional engine invocation, and ordinary create failures without an identity also incur that query.

Stats

Claude findings: 8 · Codex findings: 3 · Confirmed: 10 · 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.

[jgoux]

/ai-review

[github-actions]

Superseded by a newer AI review

🤖 AI Review

Both independent reviews were available. Ten distinct findings remain after merging the overlapping shutdown finding. The most serious are a shutdown failure that cannot be retried, cleanup that can remove live containers belonging to another state root, and a FIFO startup race that can block the host thread. The engine availability and stdin findings are stated more narrowly than in the original reviews.

Findings

Severity	Location	Category	Sources	Claim
🔴 CRITICAL	packages/stack/src/ContainerSentinel.ts:399	data-loss	claude	Startup and stale-container sweeps can force-remove live containers from a different state root that has the same stack identity.
🟠 MAJOR	packages/stack/src/StackHost.ts:293	shutdown-retry	claude+codex	A cleanup failure after the sentinel fails or starts closing leaves the failed shutdown fiber cached. Later stop requests and the sentinel failure handler rejoin that fiber instead of retrying cleanup.
🟠 MAJOR	packages/stack/src/ContainerSentinel.ts:215	startup-hang	codex	If the sentinel dies after sending its watching message and leaves a FIFO pathname behind, the host can block in a synchronous FIFO writer open with no reader.
🟡 MINOR	packages/stack/src/runtime/NativeProcess.ts:264	resource-leak	claude	Scope cleanup before fd5 reports the workload group can kill the launcher group while leaving a detached workload group alive.
🟡 MINOR	packages/stack/src/runtime/Container.ts:262	reliability	claude	A hung container create command can prevent cancellation and shutdown indefinitely because create has neither a timeout nor an interruptible region.
🟡 MINOR	packages/stack/src/StackHost.ts:481	compatibility	claude	Launching a Docker or Podman host now requires an available engine even for a destroy request when no containers exist.
🟡 MINOR	packages/stack/src/ContainerSentinel.ts:492	reliability	claude	A previous generation's FIFO writer that never closes can keep sentinel startup waiting without an internal bound.
⚪ NIT	packages/stack/src/runtime/Container.ts:118	behavior-change	claude	Engine commands issued through run no longer explicitly set stdin to ignore and instead use the spawner's default.
⚪ NIT	packages/stack/src/effect.ts:273	code-quality	claude	The failed-destroy status distinguishes a running owner from an inconclusive exit probe by matching error-message text.
⚪ NIT	apps/cli/tests/helpers/stack-cleanup.ts:3	code-organization	claude	CLI tests depend on a helper inside the stack package's private tests directory.

Stats

Claude findings: 9 · Codex findings: 2 · Confirmed: 10 · Refuted: 0 · Uncertain: 0

---

Models: claude-opus-5-5 + gpt-6-sol · Trigger: manual · Workflow run

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

[jgoux]

/ai-review

[github-actions]

🤖 AI Review

Verified all 13 reported findings against the checked-out code and trusted conventions. One overlapping engine-command finding was merged, leaving 12 findings. The most consequential risks are Windows descendant cleanup, unbounded uninterruptible engine calls, and an unbounded native-launcher handshake. The remaining findings concern cleanup reporting, legacy containers, and test quality.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	packages/stack/src/runtime/Container.ts:242	cancellation	claude+codex	Container creation and Docker helper creation can wait indefinitely inside uninterruptible regions if the engine command stalls, blocking cancellation and shutdown.
🟠 MAJOR	packages/stack/src/runtime/native-launcher.ts:102	process-cleanup	codex	On Windows, descendants may survive a workload's own exit because tree termination targets the workload PID after that process has exited.
🟠 MAJOR	packages/stack/src/runtime/NativeProcess.ts:274	cancellation	codex	Native process startup can wait indefinitely if the launcher stays alive without sending a newline on fd5.
🟡 MINOR	packages/stack/src/runtime/native-launcher.ts:204	cross-platform	claude	Every Windows native workload is now spawned detached, which can give it a separate console window even though Windows cleanup uses taskkill rather than process groups.
🟡 MINOR	apps/cli/src/command-internal/db-config.integration.test.ts:15	test-correctness	codex	The node:net.connect mock rejects valid numeric and path overloads and fails to forward optional callback arguments.
⚪ NIT	packages/stack/src/runtime/Container.ts:458	migration	claude	The new sweeps cannot remove containers created by earlier versions because those containers lack the required stack-root label.
⚪ NIT	packages/stack/src/storage/DockerDatabaseStorage.ts:430	error-handling	claude	The helper-removal finalizer logs a removal failure but suppresses it from the scope-close result.
⚪ NIT	packages/stack/src/effect.ts:273	maintainability	claude	The failed-destroy response distinguishes a still-running owner by matching error-message text, so changing that wording can change the classification.
⚪ NIT	packages/stack/src/effect.ts:264	performance	claude	When destroy and its fallback stop both fail, the client still waits through the owner-exit probe even though the host remains running for retry.
⚪ NIT	packages/stack/src/StackHost.ts:123	type-safety	claude	StackHostRuntime.shutdown declares a ChildProcessSpawner environment requirement that its implementation already supplies internally.
⚪ NIT	packages/stack/src/host/ToolRunner.ts:99	error-handling	claude	ToolRunner tries cleanup for every native job but reports only the first failure cause.
⚪ NIT	apps/cli/src/command-internal/pg-dump.native.integration.test.ts:14	maintainability	claude	CLI tests import an internal test helper from another package through relative paths, coupling the tests to that package's directory layout.

Stats

Claude findings: 9 · Codex findings: 4 · Confirmed: 12 · Refuted: 0 · Uncertain: 0

---

Models: claude-opus-5-5 + gpt-6-sol · Trigger: manual · Workflow run

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

[avallete]

Shutdown, destroy, and the stack-scoped container sweep match the contract in this PR. Stop keeps service data. Destroy removes saved state after containers are gone. A second stack is left alone.

Follow-up, outside the files this PR changes. A failed first stack start can leave the detached host running after the CLI has exited. Example: Cannot bind TCP listener when another stack already holds the database port. The host is spawned and unref'd in HostProcess.spawnDetached before service ports are bound. cleanupInitialSafe in start.handler.ts then stops and destroys the new composition, and does not shut the host down. Neither file is in this diff.

What remains is that one supervisor. Postgres and containers from the failed start are gone. The next stack start reuses the host. stack stop retires it.

When this invocation created the stack, that failure path should shut the host down and wait for it to exit.

[jgoux]

@avallete Addressed in c153686 and preserved through the develop merge in e098214. Failed startup now runs the existing stack.stop path after service cleanup and waits for the detached host to exit, provided no host was running when the command resolved the stack. This also covers retries against the saved stack; a previously running host is preserved.

The cleanup scope now starts at handle acquisition because open({ startOwner: true }) can launch the host before config validation. The native regression holds a real database port open for two failed starts, then exercises malformed config before composition on a third attempt. It verifies each exact host PID has exited before the command returns. The prepared-stack case verifies its existing host survives a failed start.

The original regression failed against the old cleanup behavior. On the merged code, all 34 targeted CLI integration tests, 8 service-startup tests, 3 focused host lifecycle tests, and repository quality checks pass locally. Full CI is now green on e098214: https://github.com/supabase/cli/actions/runs/36030232835.