feat(stack): replace remote runtime protocol with Effect RPC - #6303
feat(stack): replace remote runtime protocol with Effect RPC#6303jgoux wants to merge 36 commits into
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@4e7e3fccf49fa66303bff017e2923514e7f71360Preview 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 530175d916
ℹ️ 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: 8214e4afaa
ℹ️ 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: bbfff50ae8
ℹ️ 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".
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
Review summary
Control-plane cutover is coherent (session fence, same-version RPC, CLI-version identity, start-only replace). Cross-review (Codex + Claude) + Cursor review agree Changes requested.
Must-fix before merge
- BLOCKER — delete ownership race (
managed/lifecycle.ts):/stopcan release the delete owner mid-cleanup → new start acquires identity → original delete wipes new data. - SIGNIFICANT —
UpgradePreflightError/UpgradeRestartErrorlose typed identity across supervisor IPC (open Codex thread still VALID). - SIGNIFICANT —
statuscan fail config parse before rendering degraded version mismatch. - SIGNIFICANT — filtered exclusions written back via
updateManagedLaunchcan erase unknown exclusions across CLI versions. - SIGNIFICANT —
branches switchstale path →stopDaemoncan stop an incompatible live owner. - SIGNIFICANT —
RemoteStackstop short-circuits if RPC-scope close fails; HTTP stop maps non-409 toControlProtocolError(no ambiguous-delivery recovery).
Non-blocking
statuscopy always says “older CLI” even when the daemon is newer.
Happy to re-review once these land.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc78052a1
ℹ️ 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".
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
Round 2 review (7bc78052a)
Explicit-restart redesign looks right: ordinary connect/start fail closed with typed DaemonUpgradeRequired; only supabase start runs the full stop/start upgrade path. All seven round-1 findings are fixed (delete ownership fence, typed IPC upgrade errors, status config ordering, raw exclusions, switch stop hole, RemoteStack stop recovery, wording).
Still blocking
- BLOCKER —
applyPersistedLaunchtreats a filled version manifest as “enable every service,” which breaks native-mode upgrade restart after the old owner is already stopped. - SIGNIFICANT — mismatch
statushardcodesrunning/readytrue even for non-ready incompatible owners.
Non-blocking notes
- Restart warning is printed before preflight succeeds.
- Transient delete owner (
daemonCliVersion: "managed") can briefly surface as a version mismatch. - Concurrent dual
upgrade-restartconvergence lost direct coverage when the old replacement race test was rewritten.
Verdict: Changes requested — prior list cleared; please land the native filled-manifest fix (plus the status ready fields) before merge.
|
Final review triage is complete in
This is the final automated-feedback change batch. The branch is ready for human re-review. |
There was a problem hiding this comment.
💡 Codex Review
cli/packages/stack/src/effect-bun.ts
Line 60 in 006da50
Provide the control transport in the delete wrapper
When supabase stop --no-backup reaches managed-data deletion, deleteManagedStackPersistenceCore calls acquireControl directly and therefore still requires ControlTransport, but this platform wrapper provides only the manager layer. That layer consumes controlTransportLayer while constructing ManagedStackManager; it does not export the service for the outer deletion effect, and the production CLI runtime provides only httpTransportClientLayer and BunServices.layer. Consequently every real --no-backup deletion reaches a missing-service defect after stopping the stack; provide controlTransportLayer here (and in the Node wrapper) or move acquisition behind the manager service.
AGENTS.md reference: AGENTS.md:L101-L106
ℹ️ 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".
|
Addressed the non-inline delete-wrapper review in 2149c34: both Bun and Node Effect entrypoints now provide their platform ControlTransport for managed persistence deletion. Public-entrypoint integration coverage verifies that an absent stack returns the typed NoRunningStackError instead of a missing-service defect. Review: #6303 (review) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2149c34b77
ℹ️ 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
Replace the 100 ms read race with deterministic coordination
On slow or loaded CI, the second document read may legitimately start after this 100 ms window even when both mutations can read concurrently, so the test takes the serialized branch and can pass without exercising the stale-write race it is intended to prevent. Coordinate through an observable gate or controlled test clock rather than using elapsed wall time to infer whether the second operation reached the read.
AGENTS.md reference: AGENTS.md:L291-L293
ℹ️ 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".
|
Addressed the non-inline P1 from review #6303 (review) in 2a8165b. The lifecycle race integration now uses explicit Deferred gates: the launch update captures the first document snapshot, the lifecycle update starts afterward, and the release order makes any unserialized second snapshot deterministically overwrite launch metadata. There are no wall-clock sleeps or timing-based pass branches. The scenario passed 20 consecutive stress runs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8165b17d
ℹ️ 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: 8e586f2afd
ℹ️ 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: 02133856cd
ℹ️ 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".
|
Final automated-review batch completed in a1f4c79. The three current findings were reproduced and fixed with regressions. The CI-only failure came from a redundant process-heavy concurrency scenario; its product invariant remains covered by the adjacent subprocess test with typed mismatch and owner-liveness assertions, so the duplicate test and its file-gate scaffolding were removed. This concludes the automated feedback loop for this PR; any further changes should be driven by a concrete human-identified blocker. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f4c7903c
ℹ️ 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: dd56b48c60
ℹ️ 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: 043398bc9d
ℹ️ 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: 4e7e3fccf4
ℹ️ 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".
| ports: syntheticPorts, | ||
| }, | ||
| ).pipe(Effect.mapError((cause) => preflightError(context, causeMessage(cause)))); | ||
| return { effectiveConfigInput, launch: existing.launch }; |
There was a problem hiding this comment.
When upgrading a document created before a service was added to the catalog, effectiveConfigInput starts that service using the current CLI's version, but this return value persists the old launch with no version for it. A later CLI therefore fills the missing entry from its own then-current defaults and can silently upgrade the service again without supabase stack update; merge versions for fields absent from the persisted launch while preserving all existing pins.
Useful? React with 👍 / 👎.
| yield* context.manager | ||
| .validateManagedPortReservations({ | ||
| stackId: context.stackId, |
There was a problem hiding this comment.
When an upgrade activates a sticky exact port that has no persisted assignment and an unrelated local process already owns that port, this preflight passes because validateManagedPortReservations checks only managed documents and control candidates. The exact host bind is attempted later by reservePortSet, after the healthy incompatible owner has been stopped, leaving the stack down with an allocation error. Fresh evidence beyond the prior managed-reservation thread is that this path never performs the host-port check used by the subsequent allocation.
Useful? React with 👍 / 👎.
| yield* acquisition.requestStop; | ||
| return yield* Effect.fail(new ManagedStopPending()); |
There was a problem hiding this comment.
When the exact owner's shutdown consumes nearly the full 30-second budget but still succeeds, requestStop returns only after that session has ended and this unconditional failure asks the outer Schedule.upTo(30 seconds) for another retry. If the budget has expired, the failure is converted to ManagedStackNotStoppedError without the final ownership probe, so supabase stop reports failure even though the target stopped successfully; perform the post-stop probe independently of the retry schedule's remaining delay budget.
Useful? React with 👍 / 👎.
| StackReadinessError: () => actionability.startStack, | ||
| StackUnavailableError: () => actionability.startStack, | ||
| StackRpcTransportError: () => ({ | ||
| ...actionability.externalNetwork, |
There was a problem hiding this comment.
StackRpcTransportError is produced only by RPC calls to the managed loopback supervisor, so a daemon exit, restart, or refused local connection is currently recorded as an external network-service outage. This misattributes failures in KPI reporting and selects the RerunDebug remediation even though the rendered user guidance correctly directs users to check or restart the local stack; use the startStack classification while retaining the daemon transport fingerprint.
AGENTS.md reference: apps/cli/AGENTS.md:L370-L372
Useful? React with 👍 / 👎.
| suggestion: "Restart the stack with `supabase start`, then retry the command.", | ||
| }; | ||
| } | ||
| case "StopTimeout": { |
There was a problem hiding this comment.
When stopDaemon or branch switching reaches the new stable control path and owner acquisition fails with ControlTransportError, ControlBindError, or another control-protocol error, several of those tagged errors carry neither a message nor a detail field. Because this mapping ends after StopTimeout, normalization falls back to the tag itself, so text and structured output can report only ControlTransportError with no remediation; add user-facing mappings for the stable control error family.
Useful? React with 👍 / 👎.
2 participants
Summary
Linked issue
None.
Reviewer context
This intentionally has no legacy protocol window, compatibility adapter, adoption path, or in-place supervisor swap. Runtime RPC is a same-version boundary; owner discovery and session-fenced shutdown remain the stable cross-build protocol. Upgrading through supabase start restarts the complete stack, so active application connections and tooling streams disconnect briefly while durable state and launch selections remain intact.