perf(stack): copy snapshot directories with one host command by avallete · Pull Request #6709 · supabase/cli

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

Native database snapshots copy plain directory trees with one host command: cp on macOS and Linux, or robocopy on Windows. This speeds up the host snapshot copy used during db diff.

The copy keeps metadata, rejects symlinks and special files through portable validation, and removes a partial destination after failure or interruption. Windows junctions are checked with lstat and excluded from robocopy. Docker snapshots continue to use the volume helper.

github-actions

Superseded by a newer AI review

🤖 AI Review

Only Claude's independent review was available; the Codex pass did not complete. All nine findings were verified, although claude-1 is narrowed and downgraded to minor, claude-5 is downgraded to nit, and claude-6 is restated to reflect that the existing test would exercise the fast path on macOS but is not run there in CI and cannot force either branch. The main correctness concerns are the fail-closed optimization probe, inconsistent handling of non-directory sources, ignored cleanup failures, and uninterruptible subprocesses.

Findings

Severity	Location	Category	Sources	Claim
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:181`	`error-handling`	claude	A failure to execute or complete the macOS `find` optimization probe aborts `copyDirectory` instead of falling back to the portable traversal.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:182`	`correctness`	claude	On macOS, a regular-file source can pass the `find` gate and be accepted by `cp`, bypassing the directory validation used by the portable path.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:184`	`error-handling`	claude	If clone cleanup fails, its error is ignored and the portable fallback can instead fail with an unrelated destination-exists error, masking the clone and cleanup failures.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:199`	`concurrency`	claude	The full `find` and `cp` subprocess lifetimes are uninterruptible and receive no abort signal, so Effect interruption cannot cancel these potentially long-running operations.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:181`	`test-coverage`	claude	Tests do not force either the macOS clone-success branch or clone-failure fallback, and this test file is not included in the repository's macOS CI integration subset.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:2`	`conventions`	claude	The new subprocess boundary bypasses the package's established Effect `ChildProcessSpawner`, and the lint suppression incorrectly cites missing `FileSystem` operations as its justification.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:202`	`dead-code`	claude	The `typeof result === "string"` branch is unreachable because promisified `execFile` resolves to an object containing `stdout` and `stderr`.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:214`	`security`	claude	The macOS system utilities are resolved through the inherited `PATH`, allowing an earlier shadowing `find` or `cp` executable to run.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:192`	`readability`	claude	`runExec` places `operation` last even though error construction and the file's other operation-labelled helpers put it first, making call sites inconsistent.

Stats

Claude findings: 9 · Codex findings: 0 · Confirmed: 9 · 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.

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

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

avallete · 2026-09-23T14:18:37Z

/ai-reivew

avallete · 2026-09-23T14:29:21Z

/ai-review

github-actions

🤖 AI Review

The new host-copy path introduces two cleanup risks in the exported copy function: cleanup can follow a symlink and change permissions outside the destination, and a failed copy can remove a destination created by another process. The current snapshot caller reduces the second risk by copying into a UUID stage under a cross-process lock. Most remaining findings concern diagnostics, fallback performance, and tests. One claimed fork convention is contradicted by other tests in the repository.

Findings

Severity	Location	Category	Sources	Claim
🔴 CRITICAL	`packages/stack/src/storage/DirectoryCopy.ts:348`	`security`	codex	Cleanup can follow a symlink introduced after the source scan and recursively change permissions outside the partial destination.
🟠 MAJOR	`packages/stack/src/storage/DirectoryCopy.ts:195`	`data-loss`	codex	A failed host copy can remove a destination created by another process after the initial absence check.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:238`	`error-handling`	claude	runExec discards stdout when constructing a failed command's error, losing robocopy diagnostics written there.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:206`	`observability`	claude	The host-copy failure cause is discarded without a log before the per-file fallback runs.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:205`	`correctness`	claude	An accepted host-copy exit status returns success without verifying that the destination exists.
🟡 MINOR	`packages/stack/src/storage/DirectoryCopy.ts:396`	`performance`	claude	If macOS clone copy fails, there is no plain cp candidate; the code falls straight back to the per-file walk.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:382`	`error-handling`	claude	The unanchored usage-error regex can classify an ordinary cp error as option misuse when a path in its message contains a matched phrase.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:2`	`comment-accuracy`	claude+codex	The lint-disable reason does not explain the newly added native readdir import.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:298`	`testability`	claude	The Windows pre-flight scan bypasses an injected FileSystem service, so substituting that service cannot control the scan.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:209`	`performance`	claude	After a copyability scan succeeds and the host copy fails, fallback traverses the source again in both validateTree and copyTree.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.ts:396`	`robustness`	claude	Some find and cp candidates can interpret a source or destination beginning with a hyphen as an option.
⚪ NIT	`packages/stack/src/storage/DirectoryCopy.integration.test.ts:203`	`test-coverage`	claude	On unsupported platforms, the mocked-spawner interrupt tests wait for signals that no host-copy command can send.

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

packages/stack/src/storage/DirectoryCopy.integration.test.ts:203 (test-quality): Bare Effect.forkChild violates an established repository convention and makes the interrupt tests timing-sensitive.
Refuted: The claimed convention does not exist: the same package uses bare Effect.forkChild in Orchestrator.integration.test.ts:157 and :268-269 and Service.integration.test.ts:1048. The new tests await Deferred signals at lines 205 and 245 before interrupting, so they synchronize on the event they need.

Stats

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

…ereview

The node:fs imports fail the required quality check.

## 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 supabase#6709 are included in the merged develop base. --------- Co-authored-by: Julien Goux <hi@jgoux.dev>

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 marked this pull request as ready for review September 22, 2026 14:26

avallete requested a review from a team as a code owner September 22, 2026 14:26

github-actions Bot reviewed Sep 22, 2026

View reviewed changes

avallete changed the title ~~perf(stack): clone snapshot directories with one cp on macOS~~ perf(stack): copy snapshot directories with one host command Sep 22, 2026

avallete mentioned this pull request Sep 22, 2026

perf(stack): shorten warm docker shadow diffs #6710

Merged

Base automatically changed from perf/cli-compile-bytecode to perf/stack-managed-snapshots September 23, 2026 08:11

avallete added 2 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.

jgoux force-pushed the perf/stack-clone-copy branch from eb070af to 5d46e26 Compare September 23, 2026 08:11

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.

Base automatically changed from perf/stack-managed-snapshots to develop September 23, 2026 13:01

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.

depthfirst-app Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread packages/stack/src/storage/DirectoryCopy.ts

github-actions Bot reviewed Sep 23, 2026

View reviewed changes

jgoux approved these changes Sep 23, 2026

View reviewed changes

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

30a3019

…ereview

avallete marked this pull request as draft September 23, 2026 18:40

avallete marked this pull request as ready for review September 23, 2026 18:40

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

avallete marked this pull request as draft September 23, 2026 18:47

avallete marked this pull request as ready for review September 23, 2026 18:47

depthfirst-app Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread packages/stack/src/storage/DirectoryCopy.ts

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

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

Merged via the queue into develop with commit e942fe1 Sep 24, 2026
43 of 45 checks passed

jgoux deleted the perf/stack-clone-copy branch September 24, 2026 08:22

Conversation

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

Uh oh!

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

Stats

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!

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!

Uh oh!

Uh oh!

Reviewers

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