fix(cli): propagate delegated Go child exit codes through finalizers#5841
Conversation
…(CLI-1879) LegacyGoProxy.exec/execCapture and the hidden db __db-bootstrap seam called ProcessControl.exit() directly on a non-zero child exit, which skipped Effect.ensuring finalizers (telemetry flush, instrumentation) and, for execCapture, bypassed withJsonErrorHandling entirely so json/stream-json delegated failures emitted no structured error envelope. Route both through a new LegacyGoChildExitError instead: it carries the child's exact exit code via Runtime.errorExitCode (read by both runCli and withJsonErrorHandling) so finalizers run first and the real code still reaches the user in every output format, and runCli special-cases it (by concrete type, not Effect's shared Runtime.errorReported marker, which CliError.ShowHelp also sets and would otherwise suppress the Go-parity "required flag(s) not set" message) to skip a duplicate generic stderr line the child already printed itself. Also stop db reset --experimental --linked from minting a temporary Management API login role before delegating to the Go child, which mints its own and made the TS wrapper's mint pure duplicate work.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@306dd14fd76d9f271ba63287aea83d0569f6ffb7Preview package for commit |
![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
cli/apps/cli/src/shared/cli/run.ts
Line 57 in 306dd14
Keep the global signal handler for native reset paths
This opt-out matches every db reset invocation, but only the local/delegated branches install their own holdSignals listener. For native remote resets such as supabase db reset --linked --version ... or non-experimental remote resets, reset.handler.ts never delegates to the Go child, so disabling signalAwareProgram leaves SIGINT to Bun/Node's default immediate exit. In that scenario the reset handler's finalizers (linked-project cache and telemetry flush) are skipped, regressing the finalizer guarantee this change is trying to preserve; the opt-out needs to be limited to the child-owning path or the native path needs its own signal handling.
ℹ️ 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".
Coly010
deleted the
columferry/cli-1879-legacy-shell-child-exit-code-delegated-path-semantics
branch
2 participants
What kind of change does this PR introduce?
Bug fix — legacy-shell child exit-code and delegated-path semantics.
What is the current behavior?
Fixes CLI-1879.
Three related gaps around Go-delegated subprocess paths in the legacy shell:
LegacyGoProxy.exec/execCapture(shared/legacy/go-proxy.layer.ts) calledProcessControl.exit()directly on a non-zero child exit or "binary not found". That's a realprocess.exit()— it skips everyEffect.ensuringfinalizer between the call site andrunCli(telemetry flush, command instrumentation), and loses the child's exact exit code (collapsing everything to whateverprocess.exit()was called with, with no chance forrunCli's own exit-code logic to run).execCapture's non-zero-exit branch hard-exited the process, it never reachedwithJsonErrorHandling, so a--output-format json/stream-jsondb reset --experimentalfailure emitted no structured error envelope at all — the process just died mid-flight.db reset --experimental --linked(no resolved version) unconditionally resolved the linked DB connection — including minting/verifying a temporary Postgres login role over the Management API — before checking whether the remaining flow delegates to the Go child. On that branch the resolved connection was never used: the delegated Go child re-runs its ownParseDatabaseConfig(and mints its own temp role) once it starts, so the TS-side mint was pure duplicate privileged work.(A fourth item from the same issue — forwarding
--linked=false's target selector verbatim to the delegated child — was already correctly implemented and already covered by a passing test; no code change was needed there.)What is the new behavior?
LegacyGoChildExitError(shared/legacy/legacy-go-child-exit.error.ts) carries a spawned child's exact exit code via Effect'sRuntime.errorExitCodemarker.LegacyGoProxy.exec/execCaptureand the hiddendb __db-bootstrapseam now fail with this typed error instead of callingProcessControl.exit(), so the failure flows through the normal Effect channel: finalizers run first, thenrunCli/withJsonErrorHandlingexit with the child's real code — in every output format, including a Ctrl-C mid-recreate (e.g. exit130) instead of a generic1.runCli'shandledProgramspecial-casesLegacyGoChildExitError(by concrete type) to skip its own genericoutput.failstderr line, since the child already printed its own detailed failure to the inherited stderr and Go itself never prints a second line on top of that. This is deliberately not keyed on Effect's sharedRuntime.errorReportedmarker —CliError.ShowHelpalso sets that marker, and gating on it would have silently suppressed the Go-parityError: required flag(s) "..." not setmessage for every missing-required-flag error. (Caught by architect review before merge; regression test added.)withJsonErrorHandlingnow readsRuntime.getErrorExitCodeinstead of hardcoding1when setting the process exit code, so the exact code propagates underjson/stream-jsontoo, not just text mode.db reset --experimental --linkednow checks whether it's about to delegate to the Go child before callingLegacyDbConfigResolver.resolve(), skipping the redundant temp-role mint on that path. The linked-project-cache finalizer is unaffected (it already reads a separately pre-loaded ref, exactly so this case still works).SIDE_EFFECTS.md's exit-code tables fordb reset/db startto reflect the child's exact code, and touched-up a couple of stale doc comments describing the oldprocess.exit()-based behavior.Deliberately left open (judgement calls, not blockers)
"supabase-go exited with code N (see stderr for details)"rather than the specific TS error the old (duplicate-minting) code path used to surface. This is an accepted tradeoff: the real detail is on the inherited stderr, and Go itself has no JSON error-envelope concept to hold this to a parity standard against.codefield changes fromLegacyDbBootstrapErrortoLegacyGoChildExitErrorfor a non-zero child exit specifically. Nothing in this codebase treats that field as a stable public contract (mostLegacy*Errortags already leak their raw class name into it), so this wasn't treated as a compatibility break.db __shadowseam (legacy-pgdelta.seam.layer.ts) intentionally keeps its own generic domain error rather than adoptingLegacyGoChildExitError— its failure is a TS-authored summary over noisy docker/pgdelta stderr (not a passthrough of a real user-facing Go child), and Go itself collapses shadow-DB failures to a generic exit1. Left a comment in place explaining the divergence so a future reader doesn't "fix" it into inconsistency.Test plan
New/updated coverage in
go-proxy.layer.unit.test.ts,run.unit.test.ts,json-error-handling.unit.test.ts,reset.integration.test.ts, andstart.integration.test.ts— exact exit-code propagation, finalizers running after a non-zero exit, the JSON error envelope, the skipped pre-delegation resolve (and that the sibling--db-urldelegate path still resolves), and a regression guard for theShowHelp/MissingOptionsuppression bug caught during review.