perf(stack): shorten warm docker shadow diffs by avallete · Pull Request #6710 · supabase/cli

avallete · 2026-09-22T14:26:06Z

Summary

Reuse a Docker volume helper within each stack owner. Distinct owner identities prevent one CLI process from stopping another process's active snapshot. Interrupted helper commands retire only that owner's helper.
Reuse the recorded Docker daemon identity when its volume is present, avoiding repeated daemon probes during warm shadow diffs.
Reduce durability work for disposable shadow databases. Explicit stops still allow PostgreSQL to checkpoint before a snapshot.
Wait for shadow containers to stop and be removed before deleting their data. Namespace destruction attempts every service and reports any failures together.

The snapshot directory copy changes from #6709 are included in the merged develop base.

github-actions

Superseded by a newer AI review

🤖 AI Review

Both independent reviews completed. After verifying all 15 reported findings against the PR and trusted architecture, they deduplicate to 13 confirmed findings. The most serious issues are asynchronous destruction racing database storage removal, detached cleanup being marked complete before Docker succeeds, unsafe cross-process helper ownership, and cached daemon identity being accepted without verifying the active daemon or current versions.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	`packages/stack/src/Service.ts:611`	`race-condition`	claude	Destroy starts asynchronous container shutdown and immediately deletes the database namespace while PostgreSQL may still be running, racing data removal with active database I/O.
🟠 MAJOR	`packages/stack/src/runtime/Container.ts:273`	`resource-lifecycle`	claude+codex	The asynchronous discard path ignores its configured grace period and marks the container stopped and removed before Docker confirms either operation, allowing a failed background stop to leave an untracked live container.
🟠 MAJOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:471`	`data-safety`	claude+codex	The persisted daemon identity and versions are accepted solely because a derived volume name exists, so the code can bypass active-daemon validation and use stale engine versions for backend selection.
🟠 MAJOR	`packages/stack/src/storage/DockerHelperRegistry.ts:36`	`concurrency`	codex	A process that created a deterministic shared helper forcibly removes it at scope close even when another process has adopted and is actively using it.
🟡 MINOR	`packages/stack/src/runtime/Container.ts:42`	`dead-code`	claude	The PR's only supplied `stopGraceSeconds` value is 0, but the container runtime rejects 0 and falls back to 10 seconds; `beginStop` also hardcodes 10, so the new container-spec field does not implement the shadow's requested fast stop.
🟡 MINOR	`packages/stack/src/services/Database.ts:146`	`api-design`	claude	Setting the public `stopGraceSeconds` field to 0 also silently disables PostgreSQL crash-safety settings, coupling data durability to a stop-timing option.
🟡 MINOR	`packages/stack/src/storage/DockerHelperRegistry.ts:57`	`resource-leak`	claude	Interruption after a deterministic helper container is created but before registry insertion can orphan it permanently, because later processes adopt it as externally owned and never remove it.
🟡 MINOR	`packages/stack/src/storage/DockerHelperRegistry.ts:38`	`concurrency`	claude	A single registry-wide semaphore covers every helper command and the scope finalizer, serializing independent operations and allowing an untimed `docker exec` to block host shutdown indefinitely.
🟡 MINOR	`packages/stack/src/Orchestrator.ts:701`	`error-handling`	claude	Namespace destruction now aborts on the first failed service destroy, leaving all later services running instead of attempting shutdown for every registered service.
🟡 MINOR	`packages/stack/src/services/Database.ts:789`	`duplication`	claude	Container launch writes the root key and computes PostgreSQL settings twice, creating redundant work and two logic paths that can drift.
🟡 MINOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:633`	`error-handling`	codex	The shared-helper opener treats created, restarting, and paused containers as ready even though `docker exec` cannot reliably run in those states.
🟡 MINOR	`packages/stack/src/storage/DockerDatabaseStorage.integration.test.ts:357`	`test-coverage`	claude	No integration test exercises the shared-helper storage path or the asynchronous `beginStop` destruction path.
⚪ NIT	`packages/stack/src/storage/DockerDatabaseStorage.ts:666`	`observability`	claude	A helper shared by deterministic volume identity is labelled as belonging to the first stack that creates it, misattributing the container when other stacks adopt it.

Findings outside the diff

🟡 MINOR packages/stack/src/storage/DockerDatabaseStorage.integration.test.ts:357 — No integration test exercises the shared-helper storage path or the asynchronous beginStop destruction path.

Stats

Claude findings: 11 · Codex findings: 4 · Confirmed: 13 · 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.

Compiled CLI builds now enable minification, ESM, and bytecode at depth 2 to reduce startup work. Release, local, and E2E binaries share the same compilation options, including the local npm release workflow. Depth 2 limits the binary-size cost compared with compiling all nested functions ahead of time. --------- Co-authored-by: Julien Goux <hi@jgoux.dev>

A Postgres data directory is mostly small files, so one clone is cheaper than a clone per file. Unsupported entries still use the per-file copy.

Linux reflinks when the filesystem can, and Windows uses robocopy. A failed or interrupted host copy removes the partial tree first.

Shadow destroy issues docker stop and rm in the background. An explicit stop, including the one before a snapshot, still waits for a checkpoint.

Reuse one helper container per volume, remember the daemon identity, and turn crash safety off for a shadow that is deleted after the diff.

The phase file was only for benchmarks. A shared helper is removed only by the process that created it, so another diff cannot delete the container the first one is using.

docker exec cannot use a container left in created, and adopting it again left later diffs stuck. The helper handle type stays private.

A cancel during chmod left the failed host copy in place, and the interrupt test read the last argument, which is /NP on Windows.

Effect lint rejects node:fs and node:path in this package, and those warnings fail the quality check.

Keep the explicit copy destination from the base and write the partial fixture through Effect file APIs.

Keep develop's oxfmt stub and root-key default. Keep the host-command directory copy and the spawner it needs.

Parallel tests reused one stack id, so on Linux they shared a data directory and the pooler database exited during ready.

avallete · 2026-09-23T14:53:13Z

/ai-review

github-actions

🤖 AI Review

Adjudicated all 19 reported findings against the checked-out code and trusted repository conventions. The most consequential confirmed issues are asynchronous database destruction racing with data removal and replacement, silent failure of detached cleanup, cross-process removal of shared Docker helpers, and acceptance of cached daemon identity without a live check when a matching volume exists. The reported Docker/Podman cache thrashing is refuted: the Podman selection path does not use that cache.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:610`	`concurrency`	claude+codex	A process that created a shared helper can forcibly remove it while another process is running a command in it.
🟠 MAJOR	`packages/stack/src/runtime/Container.ts:270`	`error-handling`	claude+codex	A failed background stop or removal is treated as successful cleanup, potentially leaving a running container and published port.
🟠 MAJOR	`packages/stack/src/Service.ts:611`	`lifecycle`	claude+codex	Destroy can remove a database's mounted data and create a replacement while the old PostgreSQL container is still running.
🟠 MAJOR	`packages/stack/src/services/Database.ts:146`	`data-safety`	claude	Setting `stopGraceSeconds` to zero disables PostgreSQL crash-safety settings for any database, although the public field describes shutdown behavior.
🟠 MAJOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:472`	`data-safety`	codex	A different Docker daemon that contains the expected cached volume name can pass storage identity checks without its live daemon ID being verified.
🟡 MINOR	`packages/stack/src/services/Database.ts:798`	`maintainability`	claude	Container launches write the pgsodium root key twice and calculate PostgreSQL arguments twice.
🟡 MINOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:622`	`correctness`	claude	A new CLI version can adopt a still-running helper built from an older pinned image because adoption checks only its name and status.
🟡 MINOR	`packages/stack/src/Service.ts:250`	`test-coverage`	claude	The new discard, background-stop, durability, daemon-cache, and shared Docker-helper behaviors lack direct tests.
🟡 MINOR	`packages/stack/src/runtime/Container.ts:259`	`validation`	claude+codex	Unsupported finite stop grace values silently become ten seconds, and the background stop always uses ten seconds without this behavior being stated by the container spec.
🟡 MINOR	`packages/stack/src/storage/DockerHelperRegistry.ts:51`	`concurrency`	claude+codex	One long helper command blocks every other helper command in the same registry, including commands for different keys.
🟡 MINOR	`packages/stack/src/storage/DockerDatabaseStorage.ts:634`	`error-handling`	codex	A paused shared helper is adopted as usable even though `docker exec` cannot run in it, and the retry filter does not handle that failure.
⚪ NIT	`packages/stack/src/runtime/Container.ts:297`	`dead-code`	claude	`ContainerProcess.kill` is added and implemented but has no caller.
⚪ NIT	`packages/stack/src/storage/DockerDatabaseStorage.ts:162`	`maintainability`	claude	`resolveDaemon` uses a mutable placeholder even though it can be defined as a constant after `engineCommand`.

Refuted findings (kept for transparency, not posted as review comments)

packages/stack/src/storage/DockerDatabaseStorage.ts:397 (correctness): Alternating Docker and Podman with the same cache root rewrites the Docker daemon identity file on every run.
Refuted: Podman's normal storage selection does not invoke resolveDaemon, readDaemonIdentity, or probeDaemonIdentity, so switching to Podman does not invalidate or rewrite docker-daemon-identity.json.

Stats

Claude findings: 12 · Codex findings: 7 · Confirmed: 13 · Refuted: 1 · Uncertain: 0

Models: claude-opus-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

The shared Docker helper can be removed during another process’s command, and that in-flight failure is not retried.

…ereview

The node:fs imports fail the required quality check.

Use the pinned Postgres service image for database storage operations. The image includes GNU cp, while BusyBox supplies the remaining filesystem tools. This removes the separate Debian helper pull that could fail under registry rate limits. Pin the two database versions in the stack catalog to their republished image digests. The storage integration suite exercises snapshot save, restore, retention, metadata preservation, and cleanup against the published image. The shared volume helper introduced by supabase#6710 keys its container by both mounts and Postgres image, so separate database versions use the correct image. --------- Co-authored-by: Andrew Valleteau <avallete@users.noreply.github.com> Co-authored-by: avallete <andrew.valleteau@supabase.io>

jgoux added 2 commits September 22, 2026 08:37

perf(stack): replace snapshot archives with managed directory snapshots

4e154fc

fix(stack): tighten snapshot ownership and cleanup

fd9b2f4

avallete added this pull request to stack #6711 September 22, 2026 14:26

avallete changed the title ~~perf(stack): stop shadow databases without waiting~~ perf(stack): shorten warm docker shadow diffs Sep 22, 2026

avallete marked this pull request as ready for review September 22, 2026 20:24

avallete requested a review from a team as a code owner September 22, 2026 20:24

avallete requested a review from jgoux September 22, 2026 20:24

avallete added the run-ci Run full develop CI (tests) on drafts and non-develop PRs. label Sep 22, 2026

github-actions Bot reviewed Sep 22, 2026

View reviewed changes

avallete and others added 7 commits September 23, 2026 10:11

perf(stack): clone snapshot directories with one cp on macOS

3be8efc

A Postgres data directory is mostly small files, so one clone is cheaper than a clone per file. Unsupported entries still use the per-file copy.

perf(stack): copy snapshot directories with one host command

5d46e26

Linux reflinks when the filesystem can, and Windows uses robocopy. A failed or interrupted host copy removes the partial tree first.

perf(stack): stop shadow databases without waiting

b4c9db8

Shadow destroy issues docker stop and rm in the background. An explicit stop, including the one before a snapshot, still waits for a checkpoint.

perf(stack): shorten warm shadow database diffs

e236349

Reuse one helper container per volume, remember the daemon identity, and turn crash safety off for a shadow that is deleted after the diff.

perf(stack): drop shadow timing probes

db440ea

The phase file was only for benchmarks. A shared helper is removed only by the process that created it, so another diff cannot delete the container the first one is using.

fix(stack): recreate a helper that never started

dc135b1

docker exec cannot use a container left in created, and adopting it again left later diffs stuck. The helper handle type stays private.

jgoux force-pushed the perf/stack-shadow-diff branch from bd21a45 to dc135b1 Compare September 23, 2026 08:11

avallete added 2 commits September 23, 2026 10:51

fix(stack): finish partial snapshot copy cleanup after cancel

9aab6a4

A cancel during chmod left the failed host copy in place, and the interrupt test read the last argument, which is /NP on Windows.

test(stack): keep the interrupted copy fixture on Effect file APIs

ec146da

Effect lint rejects node:fs and node:path in this package, and those warnings fail the quality check.

depthfirst-app Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread packages/stack/src/storage/DockerDatabaseStorage.ts Resolved

avallete added 3 commits September 23, 2026 11:33

Merge branch 'perf/stack-clone-copy' into perf/stack-shadow-diff

7376fef

Keep the explicit copy destination from the base and write the partial fixture through Effect file APIs.

chore: merge develop into the snapshot directory copy

b90d524

Keep develop's oxfmt stub and root-key default. Keep the host-command directory copy and the spawner it needs.

test(stack): give catalog services separate docker data

519f8f5

Parallel tests reused one stack id, so on Linux they shared a data directory and the pooler database exited during ready.

depthfirst-app Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread packages/stack/src/Orchestrator.ts Resolved

github-actions Bot reviewed Sep 23, 2026

View reviewed changes

jgoux mentioned this pull request Sep 23, 2026

perf(stack): reuse postgres image for storage helper #6760

Merged

jgoux requested changes Sep 23, 2026

View reviewed changes

Comment thread packages/stack/src/storage/DockerDatabaseStorage.ts Resolved

Merge remote-tracking branch 'origin/develop' into avallete/pr-6709-r…

30a3019

…ereview

avallete removed this pull request from stack #6711 September 23, 2026 18:47

avallete and others added 2 commits September 23, 2026 20:58

test(stack): satisfy Effect lint in the partial-copy fixture

e9b11ad

The node:fs imports fail the required quality check.

fix(stack): reject Windows junctions during snapshot copy

31ba99c

Base automatically changed from perf/stack-clone-copy to develop September 24, 2026 08:22

jgoux added 6 commits September 24, 2026 10:24

chore(stack): merge snapshot copy updates

480990f

fix(stack): isolate helpers and await shadow teardown

5d5b57a

chore(stack): sync merged snapshot copy base

429cc32

fix(stack): clean up interrupted helpers and finish destroy

2f3d112

fix(stack): retire only interrupted active helpers

fef2cb2

chore(stack): sync latest develop changes

b634d44

jgoux approved these changes Sep 24, 2026

View reviewed changes

jgoux added this pull request to the merge queue Sep 24, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 24, 2026

fix(stack): isolate native pooler startup listeners

49098d6

jgoux added this pull request to the merge queue Sep 24, 2026

Merged via the queue into develop with commit f3f9889 Sep 24, 2026
21 checks passed

jgoux deleted the perf/stack-shadow-diff branch September 24, 2026 10:21

Conversation

avallete commented Sep 22, 2026 • edited by jgoux Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

🤖 AI Review

Findings

Findings outside the diff

Stats

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avallete commented Sep 23, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

🤖 AI Review

Findings

Stats

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgoux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

avallete commented Sep 22, 2026 •

edited by jgoux

Loading

github-actions Bot left a comment •

edited

Loading