feat(stack): replace remote runtime protocol with Effect RPC - #6303
feat(stack): replace remote runtime protocol with Effect RPC#6303jgoux wants to merge 19 commits into
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@2f99a0c82c00d2779dea7391c9e298364995e8f7Preview package for commit |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
![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: 2f99a0c82c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2639cfb636
ℹ️ 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".
| const close = yield* Effect.cached( | ||
| Effect.tryPromise({ | ||
| try: () => Promise.resolve(server.stop(false)), | ||
| catch: (cause) => cause, | ||
| }).pipe(Effect.asVoid, Effect.orDie), |
There was a problem hiding this comment.
When another client is still consuming WatchServiceStates or WatchLogs—for example, the foreground dashboard from the CLI that originally started the stack—server.stop(false) waits for that active connection, while the RPC stream remains subscribed to an open SubscriptionRef/PubSub until the supervisor scope closes. That scope cannot close until this shutdown transaction completes, so stop and incompatible-build replacement time out instead of releasing ownership. Close the request-handler scope or force-close active connections after flushing the /stop response; the Node server.close() path has the same ordering problem.
AGENTS.md reference: AGENTS.md:L127-L129
Useful? React with 👍 / 👎.
| const stats = statSync(absolutePath); | ||
| if (!stats.isFile()) throw new Error(`untracked source is not a file: ${path}`); |
There was a problem hiding this comment.
When a source checkout contains an untracked symlink, git ls-files --others includes the link, but statSync follows it: a dangling link throws ENOENT, and a link to a directory fails the isFile() check. Since managed CLI commands evaluate currentCliBuildIdentity, either valid Git state makes those commands fail with CliBuildIdentityError; inspect the link itself and hash its link target instead.
Useful? React with 👍 / 👎.
| controlProtocol: Schema.Literal(CONTROL_PROTOCOL), | ||
| controlProtocolVersion: Schema.Literal(CONTROL_PROTOCOL_VERSION), | ||
| ownershipId: Schema.String, | ||
| ownerSessionId: Schema.String, | ||
| daemonCliVersion: Schema.String, | ||
| daemonBuildId: Schema.String, |
There was a problem hiding this comment.
When this CLI encounters a daemon started by the immediately preceding implementation, /owner returns { protocolVersion, ownershipId, state, ready }, so the newly required session/build fields make decoding fail with ControlProtocolError. scanForOwner treats that error as an unrelated listener and binds a later deterministic candidate, allowing a second supervisor for the same stack; startup can then force-remove the first supervisor's containers while its process remains alive. Preserve a migration decoder or fail closed on this legacy owner response rather than treating it as an empty candidate.
Useful? React with 👍 / 👎.
| const servicePolicies = { ...config.servicePolicies }; | ||
| for (const excluded of expandExcludedServices(launch.excludedServices ?? [])) { | ||
| servicePolicies[excluded] = "off"; | ||
| } | ||
| return { ...config, servicePolicies }; |
There was a problem hiding this comment.
When an incompatible-build restart is invoked with a different --exclude set, config already contains false entries for the invocation's newly excluded services, and this helper only adds the persisted exclusions without removing those extra disables. The replacement later writes ownedExisting.launch, so the daemon runs with services disabled that stack.json and status still report as included, and another restart silently changes the service set again. Either derive the effective config solely from the preserved launch or persist the effective exclusion union.
Useful? React with 👍 / 👎.
| Effect.catchTag("SupervisorOwnerReacquirePending", () => Effect.never), | ||
| ), | ||
| }); | ||
| oldSessionEnded = replacement.oldSessionEnded; |
There was a problem hiding this comment.
When the old owner stops successfully but reacquiring its endpoint then fails or times out, replaceIncompatibleOwner never returns, so this assignment is skipped and oldSessionEnded remains false. The outer recovery path consequently emits the original control/start failure rather than UpgradeRestartError, even though the destructive boundary has already been crossed and the user's stack is now stopped; mark the session-ended transition immediately after stopSession completes so this operational failure remains classifiable and retryable.
AGENTS.md reference: AGENTS.md:L166-L167
Useful? React with 👍 / 👎.
| const responseFiber = yield* waitForStarted(child, onReplacing).pipe( | ||
| Effect.timeout(SUPERVISOR_HANDSHAKE_TIMEOUT), |
There was a problem hiding this comment.
When a valid incompatible-owner replacement spends close to its 30-second stop budget and then needs preflight or reacquisition time, the parent still applies the pre-existing 35-second timeout to the entire exchange. The child can therefore remain within each of its own documented bounds yet be terminated by the parent after it has already stopped the old owner, producing a generic startup timeout and leaving the stack down; derive the handshake deadline from the complete replacement budget or use one shared deadline.
Useful? React with 👍 / 👎.
1 participant
Summary
Linked issue
None.
Reviewer context
This intentionally has no legacy protocol window or compatibility adapter. Runtime RPC is a same-build boundary; owner discovery and session-fenced shutdown remain the stable cross-build protocol.