refactor(stack): simplify service artifact resolution - #6069
Closed
jgoux wants to merge 2 commits into
Closed
Conversation
jgoux
marked this pull request as ready for review
August 5, 2026 06:55
This was referenced Aug 5, 2026
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@8fcea5ed701093b858880249c7cba6548183d87ePreview package for commit |
jgoux
marked this pull request as draft
August 5, 2026 07:03
![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: 060ba43d51
ℹ️ 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".
jgoux
marked this pull request as ready for review
August 5, 2026 07:16
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fcea5ed70
ℹ️ 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".
Comment on lines
+391
to
+394
| if (yield* isCompleteCache(cacheDir)) { | ||
| return { path: cacheDir, downloaded: false } satisfies ResolveBinaryResult; | ||
| } | ||
|
|
||
| // Write the completion marker last, so it's carried into | ||
| // cacheDir by the same atomic rename as the rest of the | ||
| // payload — its presence is the version-agnostic completeness | ||
| // signal the cache-hit and lost-race checks rely on. | ||
| yield* fs.writeFile(path.join(tmpDir, CACHE_COMPLETE_MARKER), new Uint8Array()); | ||
| }); | ||
|
|
||
| // Publish the completed staging directory by atomically renaming | ||
| // it into place. If another process already published a | ||
| // complete cacheDir first (verified via the completion marker, | ||
| // not mere existence), discard our own copy and resolve to | ||
| // theirs instead of failing. If cacheDir exists but isn't a | ||
| // complete, marker-carrying entry — a broken/incomplete leftover | ||
| // from an older, pre-staging CLI version (see the comment above | ||
| // the marker check: this is the only place such a leftover is | ||
| // ever removed), or the rename failed for some unrelated reason | ||
| // — our own staged build is the only known-good copy: reclaim | ||
| // the spot and retry the rename, up to MAX_RECLAIM_ATTEMPTS | ||
| // times. A legitimate winner can also land in the narrow gap | ||
| // between the marker check and our own reclaim-and-retry (e.g. a | ||
| // third resolver, or a legacy writer); attemptPublish always | ||
| // re-checks the marker on every attempt (including the last) and | ||
| // adopts a winner immediately if one appears, regardless of how | ||
| // many reclaim attempts remain. Only the destructive | ||
| // reclaim-and-retry path is bounded — a rename that keeps | ||
| // failing for a reason unrelated to a competing destination | ||
| // (permissions, a read-only filesystem, disk I/O) can never | ||
| // succeed no matter how many times we retry, so once attempts | ||
| // are exhausted we surface the real rename error instead of | ||
| // retrying forever. The mirror case — clobbering a destination | ||
| // published in the sliver of time between the marker check and | ||
| // our `fs.remove` — is an accepted, narrow residual limitation: | ||
| // fully closing it needs real cross-process locking, which is | ||
| // disproportionate here since the outcome is bounded to a | ||
| // redundant rebuild of the same spec, not data loss. The whole | ||
| // stage-and-publish lifecycle is wrapped in a single | ||
| // `Effect.ensuring(cleanupTmpDir)` finalizer so every exit — | ||
| // stage failure, a genuine rename failure, or an interruption at | ||
| // any point — removes the staging directory. `cleanupTmpDir` | ||
| // force-removes and ignores errors, so it's a safe no-op once | ||
| // the rename has already moved tmpDir into place. | ||
| const MAX_RECLAIM_ATTEMPTS = 3; | ||
|
|
||
| const published = yield* Effect.gen(function* () { | ||
| yield* stage; | ||
|
|
||
| const renameOnce = () => fs.rename(tmpDir, cacheDir).pipe(Effect.as(true)); | ||
|
|
||
| const attemptPublish = ( | ||
| attemptsRemaining = MAX_RECLAIM_ATTEMPTS, | ||
| ): Effect.Effect<boolean, PlatformError.PlatformError> => | ||
| renameOnce().pipe( | ||
| Effect.catchTag("PlatformError", (renameError) => | ||
| fs.exists(path.join(cacheDir, CACHE_COMPLETE_MARKER)).pipe( | ||
| Effect.flatMap((legitimateWinner) => { | ||
| if (legitimateWinner) return Effect.succeed(false); | ||
| if (attemptsRemaining <= 0) return Effect.fail(renameError); | ||
| return fs | ||
| .remove(cacheDir, { recursive: true, force: true }) | ||
| .pipe( | ||
| Effect.ignore, | ||
| Effect.andThen(attemptPublish(attemptsRemaining - 1)), | ||
| ); | ||
| }), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| return yield* attemptPublish(); | ||
| return yield* Effect.fail(publication.failure); |
There was a problem hiding this comment.
When a provider-layout cache directory already exists without .complete but contains files, a successful extraction reaches this publish path and fs.rename(stagingDir, cacheDir) fails because the destination is non-empty; isCompleteCache(cacheDir) is then false, so the rename error is returned and StackPreparation treats it as a download failure, falling back to Docker while leaving the bad cache to repeat on every start. Please reclaim or replace markerless cache directories after staging succeeds.
Useful? React with 👍 / 👎.
Contributor
Author
|
Consolidated into #6072 to simplify review and merging. |
pull Bot
pushed a commit
to oogalieboogalie/cli
that referenced
this pull request
Aug 5, 2026
Consolidates the complete lazy-stack v2 implementation and its architecture hardening into one reviewable change. The implementation replaces the earlier user-space coordination protocols with explicit ownership boundaries and operating-system primitives: - centralizes Docker and native artifact policy in one service catalog - publishes complete native caches through private staging directories and atomic rename - models lifecycle intent directly on each service as inactive, running, or explicitly stopped - activates HTTP services at the existing proxy boundary while keeping direct-listener services eager - reserves real TCP ports until each service reaches its spawn boundary - gives foreground and detached stacks the same allocation, readiness, and lifecycle behavior - enables lazy startup for CLI-managed local stacks while preserving eager startup as the package default The hardening pass makes each service's stable state stream the single lifecycle coordination primitive, removes generation-specific waiter and relaunch machinery, keeps healthy requests off the global lifecycle lock, starts independent eager roots concurrently, recovers incomplete artifact-cache destinations, and makes service port mappings exhaustive. This keeps the simpler v2 architecture while closing the highest-impact concurrency, recovery, and shutdown races identified during review. Realtime remains eager because the HTTP proxy does not bridge its WebSocket traffic, and concurrent artifact downloaders may duplicate work while still publishing through an atomic winner. Supersedes supabase#6041 Supersedes supabase#6042 Supersedes supabase#6043 Supersedes supabase#6044 Supersedes supabase#6045 Supersedes supabase#6046 Supersedes supabase#6047 Supersedes supabase#6069 Supersedes supabase#6070 Supersedes supabase#6071
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
1 participant
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.
Replacement stack 1 of 3.
Stack: #6069 → #6070 → #6071
Centralizes the native service catalog and keeps artifact preparation deliberately small:
This replaces the earlier cache ownership and process-generation protocol with filesystem atomicity as the only publication boundary.
Supersedes #6041