fix(stack): stage binary downloads and extract via atomic rename to avoid cross-process cache races - #6003
Merged
Merged
Conversation
…void cross-process cache races BinaryResolver cached native service binaries at a deterministic, process-shared path with no concurrency protection: two processes resolving the same service+version could download to the same fixed temp file and extract directly into the final cache directory at the same time, corrupting each other's output. A partially-extracted directory then looked identical to a valid cache hit to any future invocation, so the corruption was silent and persistent. Downloads now write to a per-invocation-unique temp file and extract into a per-invocation-unique staging directory sibling to the cache dir. The cache dir is only ever made visible via an atomic rename once extraction, chmod, and codesign all succeed, so it can never be observed in a partial state. If another process already published the cache dir first, the loser discards its own staging directory and resolves to the winner's instead of failing. The staging directory is cleaned up on every failure path (download error, checksum mismatch, extraction failure, or interruption).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d36b72f842
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@9e51edb2eabac5924625d3231c532c786ff3a494Preview package for commit |
…eanup (review: #6003) Effect.ensuring around the combined stage+publish sequence replaces the two separate cleanup paths (stage.onError, and a manual if(!published) check after rename) with one finalizer. Previously, a genuine fs.rename failure (not the lost-the-race case) re-failed before reaching the manual cleanup check, and nothing covered an interruption between staging and publish — both left a .tmp-* staging directory behind in the shared binary cache.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09523726c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…g reap (review: #6003) Two related hardenings to the binary cache staging/publish path: - Completion marker: cacheDir is now only ever recognized as a valid cache hit if it carries a marker file written as the final staging step (and therefore only present after a successful atomic rename). Replaces the "non-empty ⇒ complete" assumption at both the initial cache-hit check and the rename-failure "lost the race" check, neither of which could actually distinguish a genuine winner from a broken, non-empty leftover (e.g. from an older, pre-atomic-rename CLI version killed mid-extraction). On an unmarked cacheDir, the resolver now reclaims the spot and republishes its own staged build instead of trusting or returning the broken directory. - Stale staging reap: on a cache miss, before staging its own download, the resolver opportunistically sweeps sibling `.tmp-*` directories older than 24h. Effect.ensuring can't run past a hard process kill (SIGKILL/OOM), and every attempt mints a fresh UUID, so nothing else ever revisited these siblings otherwise. Added test coverage for both: stale-sibling reaping (swept vs. left alone), and reclaiming a broken/unmarked cacheDir on both a plain resolve and a verification that the reclaimed entry is genuinely complete afterward.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ce335a14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pt late winners (review: #6003) Three related hardenings from a third automated review pass, plus a doc update: - Stale-staging sweep now runs unconditionally before the cache-hit check (previously only ran on a cache miss). Once cacheDir becomes a complete cache hit, a sweep placed after that check would never run again for that entry's siblings for the rest of its lifetime. - The upfront broken/markerless cacheDir removal now passes force: true, matching every other removal in this file. Without it, two processes racing to reclaim the same pre-marker cacheDir could have the loser's removal throw on an already-gone path, forcing an unnecessary Docker fallback even though the winner is about to publish a good native binary. - The reclaim-and-retry publish path is now a small self-recursive attemptPublish: if a legitimate winner lands in the gap between the marker check and the reclaim's remove+retry, the retry's failure is rechecked against the marker and the winner is adopted, instead of surfacing a spurious DownloadError. The mirror case (clobbering a destination published moments before the check) is documented as an accepted, narrow residual limitation rather than closed with locking, which felt disproportionate for a P2. Also updates docs/architecture.md's BinaryResolver section, which still described the pre-staging fs.exists-based cache-hit lifecycle this PR's earlier commits replaced. Added test coverage for the sweep-ordering fix and the late-winner adoption fix. The force: true fix does not have a dedicated regression test — reliably constructing the exact non-flaky two-process race in the fake-fs/Effect-fiber-scheduling test harness felt disproportionate for a one-line change already verified correct by reading Effect's FileSystem source directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9caf6adac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…aches (review: #6003) Two correctness fixes from a fourth automated review pass, both on the publish/cache-completeness logic added in prior rounds this PR: - attemptPublish's reclaim-and-retry recursion had no bound. Any rename failure unrelated to a competing destination (permissions, a read-only filesystem, disk I/O) always leaves the marker absent, so the old code looped forever instead of surfacing a DownloadError -- not a stack overflow (each step sits behind a real async boundary), but a fiber that spins forever and never resolves. Bounded the reclaim-and-retry path to a small fixed number of attempts; the "is there a legitimate winner now" check stays unconditional on every attempt, including the last, since adopting a real winner is always safe regardless of attempt count. - The upfront isCached/remove block deleted any existing markerless cacheDir before attempting a download. Since every cache entry written by a CLI build before this PR's marker existed is markerless by definition, this destroyed every legacy cached binary on first use after upgrading, before knowing whether the download would even succeed -- turning what used to be a successful cache hit (e.g. offline, or a GitHub outage) into both a failure and a destroyed cache. Removed the upfront block entirely; stage never touches cacheDir directly, so the existing reclaim-on-rename-failure logic naturally becomes the only place a markerless cacheDir is ever removed, and only once a fully-staged replacement is ready to atomically take its place. Also updates docs/architecture.md's flow diagram and prose to reflect both changes plus the sweep-ordering fix from the previous round, which the diagram hadn't caught up to. Added regression tests for both: one that forces every rename to fail permanently and asserts a bounded number of attempts before a DownloadError (verified against the pre-fix code that this test times out rather than passing, confirming it catches the regression), and one that fails the download and asserts a markerless legacy cache survives untouched.
kanadgupta
approved these changes
Jul 31, 2026
kanadgupta
left a comment
kanadgupta
left a comment
Contributor
There was a problem hiding this comment.
Test assertion enhancement suggestion per Claude, otherwise LGTM!
…failure The rename-failure test only checked the error type, so a regression that moved cleanupTmpDir off the Effect.ensuring finalizer would still pass. Add the same .tmp-/_download leftover sweep the concurrency test uses, over fakeFs.dirs/fakeFs.files.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 3, 2026
7ttp
added a commit
to 7ttp/cli
that referenced
this pull request
Aug 4, 2026
## TL;DR
Clearing the two things currently sitting on the develop→main rollup PR.
First, the failing CodeQL check: two high-severity
`incomplete-url-substring-sanitization` alerts on the sso update tests —
`startsWith("http://first.example")` would also match a lookalike host
like `first.example.evil`,
so both assertions now end with a `/` delimiter, same play as supabase#5957
(stricter check, alert gone, meaning unchanged since every real request
carries a `/v1/...` path)...
Second, the codex note on the rollup that turned out to be real:
a cache entry written before the completion-marker change (supabase#6003) is
non-empty but markerless, so the resolver skips it and hard-fails
offline, with the previously-working binary sitting right there on disk.
The code even preserved that entry for exactly this case, it just never
used it.
A failed download now falls back to the non-empty markerless dir
(`downloaded: false`) and still fails when there's nothing to fall back
to — strictly no worse than any pre-marker release,
which resolved from that same dir on mere existence. Existing offline
test updated to the corrected contract, plus a no-cache negative case...
## Refs
- Resolves the two open CodeQL alerts on `develop` (same situation supabase#5957
handled)
- Follow-up to supabase#6003, surfaced by the codex review on the rollup PR
- unblocks: supabase#6056
<details>
<summary> fixes: (ss)</summary>
<img width="822" height="561" alt="image"
src="https://github.com/user-attachments/assets/9fb299fe-c9d1-499e-a0da-4800dce88cc8"
/>
</details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
packages/stack/src/BinaryResolver.tscaches downloaded native service binaries (postgres, postgrest, auth, edge-runtime) at a path that is intentionally shared across every process on the machine, so parallelsupabase startinvocations don't re-download the same binary. That sharing is correct, but there was no concurrency protection around it:_download.tar/_download.zip, no per-invocation uniqueness), so concurrent writes could corrupt each other's bytes.tar/unzip) ran directly into the final cache directory rather than a private staging location, so an interrupted extraction (this race, a killed process, disk pressure) could leave the directory partially populated.entries.length > 0, so a partially-extracted, broken directory looked exactly like a valid cache hit to every future invocation — silent, persistent corruption.What is the new behavior?
Downloads now write to a per-invocation-unique temp file, and extraction happens in a per-invocation-unique staging directory (
${cacheDir}.tmp-<uuid>) sibling to the real cache directory instead of the cache directory itself. Once extraction, the chmod fixup, and (on macOS) ad-hoc codesign all succeed, the staging directory is atomically renamed into place as the final cache directory — so the cache directory is now only ever observable in a fully-complete state, and the existing "empty directory looks like a cache hit" bug can no longer be produced by this code path going forward.If another process already published the cache directory by the time this process tries to publish its own (i.e. it lost the race), it discards its own staging directory and resolves to the winner's cache entry instead of failing. The staging directory is also cleaned up on every failure path (download error, checksum mismatch, extraction failure, or interruption), so no
.tmp-*directories are left behind in the cache root.Added a regression test in
BinaryResolver.unit.test.tsthat runs two concurrentresolveWithMetadatacalls against the same service+version+assetName cache path (with mockedHttpClient/ChildProcessSpawner/FileSystemlayers) and asserts both succeed to the same complete cache path with no stray temp artifacts left behind.