From 306dd14fd76d9f271ba63287aea83d0569f6ffb7 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Thu, 9 Jul 2026 12:33:01 +0100 Subject: [PATCH] fix(cli): propagate delegated Go child exit codes through finalizers (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. --- .../legacy/commands/bootstrap/SIDE_EFFECTS.md | 9 +- .../legacy/commands/db/reset/SIDE_EFFECTS.md | 29 +-- .../legacy/commands/db/reset/reset.handler.ts | 82 ++++--- .../db/reset/reset.integration.test.ts | 201 ++++++++++++++++-- .../declarative/declarative.smart-target.ts | 7 +- .../shared/legacy-db-bootstrap.seam.layer.ts | 29 ++- .../legacy-db-bootstrap.seam.service.ts | 10 +- .../db/shared/legacy-pgdelta.seam.layer.ts | 7 + .../legacy/commands/db/start/SIDE_EFFECTS.md | 16 +- .../db/start/start.integration.test.ts | 60 +++++- apps/cli/src/shared/cli/run.ts | 36 +++- apps/cli/src/shared/cli/run.unit.test.ts | 59 ++++- apps/cli/src/shared/legacy/go-proxy.layer.ts | 36 +++- .../shared/legacy/go-proxy.layer.unit.test.ts | 166 +++++++++++---- .../cli/src/shared/legacy/go-proxy.service.ts | 19 +- .../legacy/legacy-go-child-exit.error.ts | 55 +++++ .../src/shared/output/json-error-handling.ts | 10 +- .../output/json-error-handling.unit.test.ts | 20 ++ 18 files changed, 688 insertions(+), 163 deletions(-) create mode 100644 apps/cli/src/shared/legacy/legacy-go-child-exit.error.ts diff --git a/apps/cli/src/legacy/commands/bootstrap/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/bootstrap/SIDE_EFFECTS.md index 6c5c7cefbe..2a28897a48 100644 --- a/apps/cli/src/legacy/commands/bootstrap/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/bootstrap/SIDE_EFFECTS.md @@ -99,10 +99,11 @@ Human banners are suppressed; a single structured result is emitted: - **Interim Go-proxy delegation for migration push.** The push step shells out to the bundled Go binary (`db push --include-roles --include-seed`) until `db push` gets its own native port (separate Linear issue). The sub-step is **not** instrumentation-wrapped (the subprocess fires - its own push telemetry). Known divergence: `LegacyGoProxy.exec` propagates the exit code, so Go's - push backoff is **not** reproduced (single attempt) — to be restored when `db push` is natively - ported. (`LegacyGoProxy.exec` exits the process on a non-zero exit rather than returning a - failure, so the step cannot be wrapped in `Effect.retry`.) + its own push telemetry). Known divergence: Go's push backoff is **not** reproduced (single + attempt) — to be restored when `db push` is natively ported. (`LegacyGoProxy.exec` fails with a + typed `LegacyGoChildExitError` on a non-zero exit rather than exiting the process — CLI-1879 — so + the step COULD now be wrapped in `Effect.retry`; leaving that unimplemented here is a deliberate + scope decision for the native `db push` port, not a technical blocker.) - **DB password is forwarded on the same channel the user supplied it (CLI-1617).** The proxy must be called 1:1 with the user's input: a flag stays a flag, an env var stays an env var. So when the user passed `-p/--password`, the push sub-step receives `--password ` (flag → flag); when diff --git a/apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md index 6089303d32..4331637e72 100644 --- a/apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md @@ -81,18 +81,23 @@ seeded over the Storage gateway (reusing the `seed buckets` local path). ## Exit Codes -| Code | Condition | -| ---- | ---------------------------------------------------------------- | -| `0` | success | -| `1` | mutually exclusive target flags (`[db-url linked local]`) | -| `1` | `--version` + `--last` together (`[last version]`) | -| `1` | `--version` not an integer (`invalid version number`) | -| `1` | `--version` has no matching migration file | -| `1` | local: database not running (`supabase start is not running.`) | -| `1` | user declined the reset confirmation (`context canceled`) | -| `1` | `config.toml` parse failure | -| `1` | drop / migrate / seed / vault apply failure, or connection error | -| `1` | local: container recreate / storage health-gate failure (seam) | +| Code | Condition | +| -------------------- | -------------------------------------------------------------------------------------------------------------------------- | +| `0` | success | +| `1` | mutually exclusive target flags (`[db-url linked local]`) | +| `1` | `--version` + `--last` together (`[last version]`) | +| `1` | `--version` not an integer (`invalid version number`) | +| `1` | `--version` has no matching migration file | +| `1` | local: database not running (`supabase start is not running.`) | +| `1` | user declined the reset confirmation (`context canceled`) | +| `1` | `config.toml` parse failure | +| `1` | drop / migrate / seed / vault apply failure, or connection error | +| child's exact code\* | local: container recreate / storage health-gate failure (seam), or `--experimental`/`--linked` delegate (proxy) child exit | + +\* The `db __db-bootstrap` seam and the `--experimental` remote delegate both +propagate the spawned `supabase-go` child's real exit code (e.g. `130` after a +Ctrl-C mid-recreate) instead of collapsing every failure to `1` — in every +`--output-format` (CLI-1879). ## Output diff --git a/apps/cli/src/legacy/commands/db/reset/reset.handler.ts b/apps/cli/src/legacy/commands/db/reset/reset.handler.ts index 640392a7c0..aaa0117183 100644 --- a/apps/cli/src/legacy/commands/db/reset/reset.handler.ts +++ b/apps/cli/src/legacy/commands/db/reset/reset.handler.ts @@ -227,6 +227,36 @@ export const legacyDbReset = Effect.fn("legacy.db.reset")(function* (flags: Lega } const connType = target.connType ?? "local"; + // Single source of truth for "does this reset delegate to the Go child?" — + // checked at both delegation sites below (before `resolve()` for a linked + // target, after it for a `--db-url` target) so the two call sites can never + // drift apart. + const shouldDelegateExperimental = experimental && resolvedVersion === ""; + + // Delegates the remaining `--experimental` schema-files apply path + // (`apply.MigrateAndSeed`, not ported) to the Go child. In text mode inherit + // its stdio. Under a machine-output mode (`--output-format json|stream-json`) + // the Go child emits no TS envelope, so suppress its stdout (capture + discard) + // and emit the same structured success the native local and remote paths do, + // keeping the JSON contract consistent across all reset paths. + const delegateExperimentalReset = () => + Effect.gen(function* () { + const env = { SUPABASE_TELEMETRY_DISABLED: "1" }; + if (output.format === "text") { + yield* proxy.exec(buildResetArgs(flags, connType, yes), { env }); + } else { + // Machine-output mode is non-interactive: give the Go child a non-TTY stdin + // (`stdin: "ignore"`) so it can't block on (or be answered at) Go's + // destructive reset prompt — it takes the default `false`, matching the + // native reset path which suppresses prompts under json/stream-json. + yield* proxy.execCapture(buildResetArgs(flags, connType, yes), { env, stdin: "ignore" }); + yield* output.success("Reset remote database.", { + target: "remote", + version: resolvedVersion, + }); + } + }); + // Go's ParseDatabaseConfig runs LoadProjectRef BEFORE the fallible linked // resolution (db_url.go:87-95), and Execute() writes the linked-project cache // even when a later step errors (root.go:171-181). Pre-load the ref so the @@ -235,7 +265,23 @@ export const legacyDbReset = Effect.fn("legacy.db.reset")(function* (flags: Lega if (connType === "linked") { const refResolver = yield* LegacyProjectRefResolver; linkedRefForCache = yield* refResolver.loadProjectRef(Option.none()); + + // A linked target is never local (`resolver.resolve()`'s "linked" branch + // always returns `isLocal: false`), so the delegated-experimental check can + // run BEFORE calling `resolve()`. This matters: for `connType === "linked"`, + // `resolve()` mints/verifies a temporary Postgres login role over the + // Management API — and the delegated Go child re-runs that exact same + // `ParseDatabaseConfig` work itself once delegation happens. Calling + // `resolve()` here would mint the temp role twice for zero downstream use on + // this branch (Go's own reset flow mints it exactly once, as part of the code + // path being delegated to — confirmed against `apps/cli-go/internal/utils/ + // flags/db_url.go`'s `NewDbConfigWithPassword`/`initLoginRole`). CLI-1879. + if (shouldDelegateExperimental) { + yield* delegateExperimentalReset(); + return; + } } + const cfg = yield* resolver.resolve({ dbUrl: flags.dbUrl, connType, dnsResolver }); // Local target → native local reset. The container-recreate primitives live @@ -308,34 +354,20 @@ export const legacyDbReset = Effect.fn("legacy.db.reset")(function* (flags: Lega return; } - // Resolve the linked ref before any return so the post-run cache (Go's - // `PersistentPostRun` `ensureProjectGroupsCached`) is written even on the - // delegated `--experimental` path below — the Go child runs with telemetry - // disabled and skips that cache, so the TS finalizer must own it. + // Re-confirm `linkedRefForCache` from the now-resolved `cfg.ref` for the native + // remote linked path below (a linked+experimental+versionless target already + // delegated and returned above, before `resolve()` was ever called — see the + // `connType === "linked"` block earlier in this function). A `connType === + // "db-url"` target leaves `linkedRefForCache` as whatever the pre-load block + // set (nothing, for `db-url`), since this assignment only fires when linked. const linkedRef = Option.getOrUndefined(cfg.ref ?? Option.none()); if (connType === "linked" && linkedRef !== undefined) linkedRefForCache = linkedRef; - // Remote path. The niche `--experimental` schema-files apply path - // (`apply.MigrateAndSeed`) is not ported; delegate it to the Go child. In text - // mode inherit its stdio. Under a machine-output mode (`--output-format - // json|stream-json`) the Go child emits no TS envelope, so suppress its stdout - // (capture + discard) and emit the same structured success the native local and - // remote paths do, keeping the JSON contract consistent across all reset paths. - if (experimental && resolvedVersion === "") { - const env = { SUPABASE_TELEMETRY_DISABLED: "1" }; - if (output.format === "text") { - yield* proxy.exec(buildResetArgs(flags, connType, yes), { env }); - } else { - // Machine-output mode is non-interactive: give the Go child a non-TTY stdin - // (`stdin: "ignore"`) so it can't block on (or be answered at) Go's - // destructive reset prompt — it takes the default `false`, matching the - // native reset path which suppresses prompts under json/stream-json. - yield* proxy.execCapture(buildResetArgs(flags, connType, yes), { env, stdin: "ignore" }); - yield* output.success("Reset remote database.", { - target: "remote", - version: resolvedVersion, - }); - } + // Remaining remote target: a `--db-url` pointing at a non-local host (the + // `connType === "linked"` case already delegated above, before `resolve()`, + // without resolving a connection at all). + if (shouldDelegateExperimental) { + yield* delegateExperimentalReset(); return; } diff --git a/apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts b/apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts index 853f933134..a5bd97e893 100644 --- a/apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts +++ b/apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts @@ -3,7 +3,7 @@ import { dirname, join } from "node:path"; import { BunServices } from "@effect/platform-bun"; import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Layer, Option } from "effect"; +import { Cause, Effect, Exit, Layer, Option } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; @@ -31,6 +31,7 @@ import { LegacyYesFlag, } from "../../../../shared/legacy/global-flags.ts"; import { LegacyGoProxy } from "../../../../shared/legacy/go-proxy.service.ts"; +import { LegacyGoChildExitError } from "../../../../shared/legacy/legacy-go-child-exit.error.ts"; import type { OutputFormat } from "../../../../shared/output/types.ts"; import { LegacyDbConfigResolver } from "../../../shared/legacy-db-config.service.ts"; import type { @@ -69,15 +70,24 @@ const DEFAULT_FLAGS: LegacyDbResetFlags = { last: Option.none(), }; +/** + * Tracks every `resolve`/`resolvePoolerFallback` invocation so tests can prove a + * connection was (or, for the delegated-experimental path, was NOT) resolved — + * `resolve()` mints/verifies a temporary Postgres login role over the Management + * API, so calling it on a path that immediately discards the result is wasted + * (and duplicated) work (CLI-1879). + */ function mockResolver(opts: { isLocal: boolean; ref?: string; omitRef?: boolean; resolveFails?: boolean; }) { - return Layer.succeed(LegacyDbConfigResolver, { - resolve: (_flags: LegacyDbConfigFlags) => - opts.resolveFails === true + let calls = 0; + const layer = Layer.succeed(LegacyDbConfigResolver, { + resolve: (_flags: LegacyDbConfigFlags) => { + calls++; + return opts.resolveFails === true ? Effect.fail( new LegacyDbConfigConnectTempRoleError({ message: "failed to create login role: network error", @@ -91,9 +101,19 @@ function mockResolver(opts: { isLocal: opts.isLocal, ref: opts.ref !== undefined ? Option.some(opts.ref) : Option.none(), }) satisfies LegacyResolvedDbConfig, - ), - resolvePoolerFallback: () => Effect.succeed(Option.none()), + ); + }, + resolvePoolerFallback: () => { + calls++; + return Effect.succeed(Option.none()); + }, }); + return { + layer, + get calls() { + return calls; + }, + }; } function mockConnection(opts: { remoteSeeds?: Readonly> }) { @@ -142,8 +162,15 @@ function mockConnection(opts: { remoteSeeds?: Readonly> } * Stateful mock of the container-bootstrap seam. `running` drives * `AssertSupabaseDbIsRunning`; `storageReady` drives the bucket-seed gate. Records * the recreate args so tests can assert version / `--no-seed` propagation. + * `awaitStorageReadyExitCode`, when set, fails `awaitStorageReady` with a + * `LegacyGoChildExitError` carrying that code — simulating the seam's real + * `captureStdout` bootstrap-child path exiting non-zero (CLI-1879). */ -function mockBootstrapSeam(opts: { running?: boolean; storageReady?: boolean }) { +function mockBootstrapSeam(opts: { + running?: boolean; + storageReady?: boolean; + awaitStorageReadyExitCode?: number; +}) { const recreateCalls: Array<{ version: string; noSeed: boolean; @@ -164,8 +191,18 @@ function mockBootstrapSeam(opts: { running?: boolean; storageReady?: boolean }) awaitStorageReady: () => Effect.sync(() => { storageChecked = true; - return opts.storageReady ?? false; - }), + }).pipe( + Effect.flatMap(() => + opts.awaitStorageReadyExitCode !== undefined + ? Effect.fail( + new LegacyGoChildExitError({ + exitCode: opts.awaitStorageReadyExitCode, + message: `failed to bootstrap the local database: exit ${opts.awaitStorageReadyExitCode}`, + }), + ) + : Effect.succeed(opts.storageReady ?? false), + ), + ), }); return { layer, @@ -188,14 +225,33 @@ const mockStorageHttp = Layer.succeed( ), ); -function mockProxy() { +/** + * `execCaptureExitCode`, when set, makes `execCapture` fail with a + * `LegacyGoChildExitError` carrying that code instead of succeeding — simulating + * a delegated Go child exiting non-zero under a machine-output mode (CLI-1879). + */ +function mockProxy(opts: { execCaptureExitCode?: number } = {}) { const calls: Array<{ args: ReadonlyArray; env?: Record }> = []; const layer = Layer.succeed(LegacyGoProxy, { - exec: (args, opts) => + exec: (args, execOpts) => Effect.sync(() => { - calls.push({ args, env: opts?.env }); + calls.push({ args, env: execOpts?.env }); }), - execCapture: () => Effect.succeed(""), + execCapture: (args, execOpts) => + Effect.sync(() => { + calls.push({ args, env: execOpts?.env }); + }).pipe( + Effect.flatMap(() => + opts.execCaptureExitCode !== undefined + ? Effect.fail( + new LegacyGoChildExitError({ + exitCode: opts.execCaptureExitCode, + message: `supabase-go exited with code ${opts.execCaptureExitCode}`, + }), + ) + : Effect.succeed(""), + ), + ), }); return { layer, @@ -222,6 +278,8 @@ function setup( resolveFails?: boolean; running?: boolean; storageReady?: boolean; + awaitStorageReadyExitCode?: number; + execCaptureExitCode?: number; }, ) { if (opts.toml !== undefined) { @@ -236,25 +294,30 @@ function setup( const out = mockOutput({ format: opts.format ?? "text", promptConfirmResponses: opts.confirm }); const conn = mockConnection(opts); - const proxy = mockProxy(); - const seam = mockBootstrapSeam({ running: opts.running, storageReady: opts.storageReady }); + const proxy = mockProxy({ execCaptureExitCode: opts.execCaptureExitCode }); + const seam = mockBootstrapSeam({ + running: opts.running, + storageReady: opts.storageReady, + awaitStorageReadyExitCode: opts.awaitStorageReadyExitCode, + }); const telemetry = mockLegacyTelemetryStateTracked(); const linkedCache = mockLegacyLinkedProjectCacheTracked(); // The local-reset bucket-seed core statically requires the (lazy) Management-API // factory; never invoked on `--local` (projectRef === ""). const platformApi = mockLegacyPlatformApiService({}); + const resolver = mockResolver({ + isLocal: opts.isLocal ?? false, + ref: opts.ref ?? LEGACY_VALID_REF, + omitRef: opts.omitRef, + resolveFails: opts.resolveFails, + }); const layer = Layer.mergeAll( out.layer, conn.layer, proxy.layer, seam.layer, - mockResolver({ - isLocal: opts.isLocal ?? false, - ref: opts.ref ?? LEGACY_VALID_REF, - omitRef: opts.omitRef, - resolveFails: opts.resolveFails, - }), + resolver.layer, mockLegacyCliConfig({ workdir }), BunServices.layer, mockRuntimeInfo(), @@ -284,7 +347,7 @@ function setup( telemetry.layer, linkedCache.layer, ); - return { layer, out, conn, proxy, seam, telemetry, linkedCache }; + return { layer, out, conn, proxy, seam, telemetry, linkedCache, resolver }; } const migrationFile = (version: string, body = "create table t ();") => ({ @@ -718,6 +781,94 @@ describe("legacy db reset", () => { }); }); + it.live("does not resolve a linked DB connection before delegating an experimental reset", () => { + const { layer, proxy, resolver } = setup(tmp.current, { + toml: 'project_id = "test"\n', + experimental: true, + }); + return Effect.gen(function* () { + yield* legacyDbReset({ ...DEFAULT_FLAGS, linked: true }).pipe(Effect.provide(layer)); + expect(proxy.calls).toHaveLength(1); + // The delegated Go child re-runs its own connection resolution (including + // minting/verifying the temp login role) once it starts — the TS wrapper + // must not do that same Management-API work first only to discard it (CLI-1879). + expect(resolver.calls).toBe(0); + }); + }); + + it.live("still caches the linked ref when delegating an experimental reset", () => { + // `linkedRefForCache` is pre-loaded via `LegacyProjectRefResolver.loadProjectRef` + // separately from `resolver.resolve()`, specifically so the post-run + // linked-project-cache finalizer still fires on this path even though + // `resolve()` itself is skipped entirely (CLI-1879). + const { layer, linkedCache } = setup(tmp.current, { + toml: 'project_id = "test"\n', + experimental: true, + ref: LEGACY_VALID_REF, + }); + return Effect.gen(function* () { + yield* legacyDbReset({ ...DEFAULT_FLAGS, linked: true }).pipe(Effect.provide(layer)); + expect(linkedCache.cached).toBe(true); + expect(linkedCache.cachedRef).toBe(LEGACY_VALID_REF); + }); + }); + + it.live( + "surfaces a delegated experimental-reset child failure as a LegacyGoChildExitError under json output", + () => { + const { layer } = setup(tmp.current, { + toml: 'project_id = "test"\n', + experimental: true, + format: "json", + execCaptureExitCode: 3, + }); + return Effect.gen(function* () { + // Under json/stream-json, the delegated path uses `execCapture` (non-text + // branch of `delegateExperimentalReset`) — this must flow through the normal + // Effect failure channel (reachable by `withJsonErrorHandling` at the + // command-wiring layer) instead of an immediate `ProcessControl.exit()` that a + // handler-level test could never observe (CLI-1879). + const exit = yield* legacyDbReset({ ...DEFAULT_FLAGS, linked: true }).pipe( + Effect.provide(layer), + Effect.exit, + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(3); + } + }); + }, + ); + + it.live( + "propagates the storage-ready check's exact exit code and still flushes telemetry on a local reset", + () => { + // The bootstrap seam's `awaitStorageReady` (the `captureStdout` bootstrap-child + // path) failing non-zero must reach the handler as the exact `LegacyGoChildExitError` + // it fails with, and the handler's own `Effect.ensuring(telemetryState.flush)` + // finalizer must still run despite the typed failure (CLI-1879). + const { layer, telemetry } = setup(tmp.current, { + toml: 'project_id = "test"\n', + args: ["db", "reset"], + isLocal: true, + running: true, + awaitStorageReadyExitCode: 4, + }); + return Effect.gen(function* () { + const exit = yield* legacyDbReset(DEFAULT_FLAGS).pipe(Effect.provide(layer), Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(4); + } + expect(telemetry.flushed).toBe(true); + }); + }, + ); + it.live("forwards the linked selector to the delegate even for --linked=false", () => { // Cobra `Changed` semantics: `--linked=false` still selects the linked/remote target in // the parent, so the delegated argv must carry `--linked` — otherwise the Go child falls @@ -817,7 +968,7 @@ describe("legacy db reset", () => { }); it.live("forwards --db-url and --no-seed on an experimental remote db-url reset", () => { - const { layer, proxy } = setup(tmp.current, { + const { layer, proxy, resolver } = setup(tmp.current, { toml: 'project_id = "test"\n', experimental: true, args: ["db", "reset", "--db-url", "postgresql://db.example.com:5432/postgres"], @@ -836,6 +987,10 @@ describe("legacy db reset", () => { "--no-seed", "--yes=false", ]); + // Unlike the `connType === "linked"` branch above, a `--db-url` target still + // resolves a connection before delegating — the pre-delegation skip (CLI-1879) + // is scoped to the linked branch only, not "never call resolve when delegating". + expect(resolver.calls).toBe(1); }); }); diff --git a/apps/cli/src/legacy/commands/db/schema/declarative/declarative.smart-target.ts b/apps/cli/src/legacy/commands/db/schema/declarative/declarative.smart-target.ts index 7e6cbab323..77fb087c26 100644 --- a/apps/cli/src/legacy/commands/db/schema/declarative/declarative.smart-target.ts +++ b/apps/cli/src/legacy/commands/db/schema/declarative/declarative.smart-target.ts @@ -169,9 +169,10 @@ export const legacyResolveSmartTargetUrl = Effect.fnUntraced(function* ( } if (shouldReset) { // Go runs reset in-process and returns the error (`cmd/db_schema_declarative.go:262-267`). - // Use the non-exiting seam (not LegacyGoProxy.exec, which process.exits on a - // non-zero child and would skip the handler's telemetry flush / error handling), - // and propagate a failure on a non-zero reset exit. + // `execInherit` (not `LegacyGoProxy.exec`) returns the child's exit code as a + // catchable value rather than exiting the host process — the same + // typed-failure design CLI-1879 gave `LegacyGoProxy.exec` itself, predating + // it here as its own seam. Propagate a failure on a non-zero reset exit. const seam = yield* LegacyDeclarativeSeam; // Forward --network-id: Go's in-process reset.Run honors the root viper // network-id (`apps/cli-go/internal/utils/docker.go:267-271`), so the diff --git a/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts b/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts index bcf5735978..d6b1793166 100644 --- a/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts +++ b/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts @@ -8,6 +8,7 @@ import { legacyResolveExperimental, } from "../../../../shared/legacy/global-flags.ts"; import { resolveBinary } from "../../../../shared/legacy/go-proxy.layer.ts"; +import { LegacyGoChildExitError } from "../../../../shared/legacy/legacy-go-child-exit.error.ts"; import { ProcessControl } from "../../../../shared/runtime/process-control.service.ts"; import { LegacyCliConfig } from "../../../config/legacy-cli-config.service.ts"; import { spawnContainerCli } from "../../../shared/legacy-container-cli.ts"; @@ -112,15 +113,19 @@ export const legacyDbBootstrapSeamLayer = Layer.effect( .exitCode(command) .pipe(Effect.mapError(() => seamFailure("failed to run supabase-go."))); if (exitCode !== 0) { - // Fail (rather than `processControl.exit`) so the handler's finalizers — - // `Effect.ensuring(telemetryState.flush)` + the legacy command - // instrumentation — still run; an immediate `process.exit` here would - // skip them. Go likewise exits non-zero on a bootstrap error only after - // its `PersistentPostRun`. The child's detailed failure is already on the - // inherited stderr. (Preserving the child's *exact* exit code while still - // running finalizers would require a shared `runCli` change — deferred.) + // `LegacyGoChildExitError` (not `seamFailure`/`processControl.exit`) so the + // handler's finalizers — `Effect.ensuring(telemetryState.flush)` + the legacy + // command instrumentation — still run (an immediate `process.exit` would skip + // them), AND the child's exact exit code (e.g. 130 after Ctrl-C cleanup) reaches + // `runCli`'s `processControl.exit()` instead of collapsing to a generic 1. The + // child's detailed failure is already on the inherited stderr, so `runCli` + // special-cases this error class to suppress its own normally-would-print + // generic stderr line — Go itself never prints a second line here. CLI-1879. return yield* Effect.fail( - seamFailure(`failed to bootstrap the local database: exit ${exitCode}`), + new LegacyGoChildExitError({ + exitCode, + message: `failed to bootstrap the local database: exit ${exitCode}`, + }), ); } return ""; @@ -138,8 +143,14 @@ export const legacyDbBootstrapSeamLayer = Layer.effect( Effect.mapError(() => seamFailure("failed to bootstrap the local database.")), ); if (exitCode !== 0) { + // See the `!captureStdout` branch above for why `LegacyGoChildExitError` + // replaces `seamFailure` here — same exact-code + finalizer + no-duplicate-line + // reasoning (CLI-1879). return yield* Effect.fail( - seamFailure(`failed to bootstrap the local database: exit ${exitCode}`), + new LegacyGoChildExitError({ + exitCode, + message: `failed to bootstrap the local database: exit ${exitCode}`, + }), ); } return decodeChunks(chunks); diff --git a/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.service.ts b/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.service.ts index 4c29ddb67a..157b290c4b 100644 --- a/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.service.ts +++ b/apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.service.ts @@ -1,5 +1,6 @@ import { Context, type Effect } from "effect"; +import type { LegacyGoChildExitError } from "../../../../shared/legacy/legacy-go-child-exit.error.ts"; import type { LegacyDbBootstrapError } from "./legacy-db-bootstrap.errors.ts"; /** @@ -35,7 +36,7 @@ interface LegacyDbBootstrapSeamShape { */ readonly startDatabase: (opts: { readonly fromBackup?: string; - }) => Effect.Effect; + }) => Effect.Effect; /** * The PG14/PG15 container-recreate half of local `db reset` * (`reset.RecreateLocalDatabase`): recreate the db container/volume, init schema, @@ -51,7 +52,7 @@ interface LegacyDbBootstrapSeamShape { readonly version: string; readonly noSeed: boolean; readonly sqlPaths: ReadonlyArray; - }) => Effect.Effect; + }) => Effect.Effect; /** * The storage health gate local `db reset` runs before seeding buckets * (`reset.AwaitStorageReady`): if the storage container exists but is unhealthy, @@ -59,7 +60,10 @@ interface LegacyDbBootstrapSeamShape { * the caller should run the ported bucket seeding) and `false` when it does not * — matching Go, which silently skips buckets when storage is absent. */ - readonly awaitStorageReady: () => Effect.Effect; + readonly awaitStorageReady: () => Effect.Effect< + boolean, + LegacyDbBootstrapError | LegacyGoChildExitError + >; } export class LegacyDbBootstrapSeam extends Context.Service< diff --git a/apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.layer.ts b/apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.layer.ts index 29469069f8..6a52434c89 100644 --- a/apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.layer.ts +++ b/apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.layer.ts @@ -505,6 +505,13 @@ export const legacyDeclarativeSeamLayer = Layer.effect( }), ); +// Intentionally NOT `LegacyGoChildExitError` (contrast `legacy-db-bootstrap.seam.layer.ts`, +// fixed under CLI-1879): this seam's failure is a TS-authored domain summary over noisy +// docker/pgdelta child stderr, not a passthrough of a real Go-CLI child the user invoked +// directly — Go itself wraps every shadow-DB failure into a generic error that `cmd/root.go`'s +// `recoverAndExit` exits `1` for, so propagating THIS child's exact exit code would itself +// diverge from Go, and suppressing this message (as `LegacyGoChildExitError` does for its own, +// already-detailed child stderr) would drop the only actionable line the user sees. const failure = (exitCode?: number) => new LegacyDeclarativeShadowDbError({ message: diff --git a/apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md index dcf8466552..dacbedff73 100644 --- a/apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md @@ -49,12 +49,16 @@ fresh PG15 volume; that is internal to the seam, not the TS handler.) ## Exit Codes -| Code | Condition | -| ---- | --------------------------------------------------------------------- | -| `0` | success — database started, or already running | -| `1` | malformed `supabase/config.toml` | -| `1` | Docker daemon unreachable / inspect failure | -| `1` | container bootstrap failed (the seam cleans up via `DockerRemoveAll`) | +| Code | Condition | +| -------------------- | --------------------------------------------------------------------- | +| `0` | success — database started, or already running | +| `1` | malformed `supabase/config.toml` | +| `1` | Docker daemon unreachable / inspect failure | +| child's exact code\* | container bootstrap failed (the seam cleans up via `DockerRemoveAll`) | + +\* The `db __db-bootstrap` seam propagates the spawned `supabase-go` child's +real exit code (e.g. `130` after a Ctrl-C mid-bootstrap) instead of collapsing +every failure to `1` — in every `--output-format` (CLI-1879). ## Output diff --git a/apps/cli/src/legacy/commands/db/start/start.integration.test.ts b/apps/cli/src/legacy/commands/db/start/start.integration.test.ts index bc560dd44e..e53edc5f2b 100644 --- a/apps/cli/src/legacy/commands/db/start/start.integration.test.ts +++ b/apps/cli/src/legacy/commands/db/start/start.integration.test.ts @@ -3,7 +3,7 @@ import { join } from "node:path"; import { BunServices } from "@effect/platform-bun"; import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Layer, Option } from "effect"; +import { Cause, Effect, Exit, Layer, Option } from "effect"; import { mockOutput, mockRuntimeInfo } from "../../../../../tests/helpers/mocks.ts"; import { @@ -11,6 +11,7 @@ import { mockLegacyTelemetryStateTracked, useLegacyTempWorkdir, } from "../../../../../tests/helpers/legacy-mocks.ts"; +import { LegacyGoChildExitError } from "../../../../shared/legacy/legacy-go-child-exit.error.ts"; import type { OutputFormat } from "../../../../shared/output/types.ts"; import { LegacyDbBootstrapError } from "../shared/legacy-db-bootstrap.errors.ts"; import { LegacyDbBootstrapSeam } from "../shared/legacy-db-bootstrap.seam.service.ts"; @@ -22,22 +23,40 @@ const DEFAULT_FLAGS: LegacyDbStartFlags = { fromBackup: Option.none() }; /** * Stateful mock of the container-bootstrap seam. `running` drives * `AssertSupabaseDbIsRunning`; `runningFails` / `startFails` make the respective - * call fail (Docker daemon down / StartDatabase error). Records the args passed to - * `startDatabase`. + * call fail (Docker daemon down / StartDatabase error). `startExitCode`, when set, + * fails `startDatabase` with a `LegacyGoChildExitError` carrying that code instead — + * simulating the seam's real bootstrap child (`db __db-bootstrap --mode start`) + * exiting non-zero (CLI-1879). Records the args passed to `startDatabase`. */ -function mockSeam(opts: { running?: boolean; runningFails?: boolean; startFails?: boolean } = {}) { +function mockSeam( + opts: { + running?: boolean; + runningFails?: boolean; + startFails?: boolean; + startExitCode?: number; + } = {}, +) { const startCalls: Array<{ fromBackup?: string }> = []; const layer = Layer.succeed(LegacyDbBootstrapSeam, { isDbRunning: () => opts.runningFails === true ? Effect.fail(new LegacyDbBootstrapError({ message: "failed to inspect service" })) : Effect.succeed(opts.running ?? false), - startDatabase: (args: { fromBackup?: string }) => - opts.startFails === true + startDatabase: (args: { fromBackup?: string }) => { + if (opts.startExitCode !== undefined) { + return Effect.fail( + new LegacyGoChildExitError({ + exitCode: opts.startExitCode, + message: `failed to bootstrap the local database: exit ${opts.startExitCode}`, + }), + ); + } + return opts.startFails === true ? Effect.fail(new LegacyDbBootstrapError({ message: "failed to bootstrap" })) : Effect.sync(() => { startCalls.push(args); - }), + }); + }, recreateDatabase: () => Effect.void, awaitStorageReady: () => Effect.succeed(false), }); @@ -57,6 +76,7 @@ function setup( running?: boolean; runningFails?: boolean; startFails?: boolean; + startExitCode?: number; /** Caller cwd (Go's `CurrentDirAbs`) for relative `--from-backup` resolution. */ cwd?: string; }, @@ -211,6 +231,32 @@ describe("legacy db start", () => { }); }); + it.live( + "propagates the bootstrap child's exact exit code as LegacyGoChildExitError and still flushes telemetry", + () => { + // The bootstrap seam's `startDatabase` (the `!captureStdout` bootstrap-child + // path) failing non-zero must reach the handler as the exact `LegacyGoChildExitError` + // it fails with — not a generic `LegacyDbBootstrapError` collapsing every exit code to + // 1 — and the handler's own `Effect.ensuring(telemetryState.flush)` finalizer must + // still run despite the typed failure (CLI-1879). + const { layer, telemetry } = setup(tmp.current, { + toml: 'project_id = "test"\n', + running: false, + startExitCode: 3, + }); + return Effect.gen(function* () { + const exit = yield* legacyDbStart(DEFAULT_FLAGS).pipe(Effect.provide(layer), Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(3); + } + expect(telemetry.flushed).toBe(true); + }); + }, + ); + it.live("emits a json result when the database is already running", () => { const { layer, out } = setup(tmp.current, { toml: 'project_id = "test"\n', diff --git a/apps/cli/src/shared/cli/run.ts b/apps/cli/src/shared/cli/run.ts index 48ab86fa05..b0db350a35 100644 --- a/apps/cli/src/shared/cli/run.ts +++ b/apps/cli/src/shared/cli/run.ts @@ -11,6 +11,7 @@ import { outputLayerFor } from "../output/output.layer.ts"; import { normalizeCause } from "../output/normalize-error.ts"; import type { OutputFormat } from "../output/types.ts"; import { Output } from "../output/output.service.ts"; +import { LegacyGoChildExitError } from "../legacy/legacy-go-child-exit.error.ts"; import { cliConfigLayer } from "../../next/config/cli-config.layer.ts"; import { projectHomeLayer } from "../../next/config/project-home.layer.ts"; import { ProjectLocalServiceVersions } from "../../next/config/project-local-service-versions.service.ts"; @@ -115,6 +116,27 @@ export function exitCodeForFailure(cause: Cause.Cause): number { return Runtime.getErrorExitCode(Cause.squash(cause)); } +/** + * Whether `handledProgram` should render its generic `output.fail` stderr line + * for a failed run, given the run's cause and the exit code `exitCodeForFailure` + * already computed for it. False for a clean exit (`0`), an interrupt (`130`), + * and a `LegacyGoChildExitError` (CLI-1879) — a delegated Go child already wrote + * its own detailed failure to the inherited stderr, so a second generic line + * here would be a line Go itself never prints. + * + * Checked by concrete type, NOT Effect's shared `[Runtime.errorReported]` + * marker: `CliError.ShowHelp` also sets that marker to `false`, for an + * unrelated reason (the CLI framework already rendered help/usage text) — + * gating on the marker would ALSO suppress `normalizeCause`'s Go-parity + * rendering for a `MissingOption` wrapped in `ShowHelp` (e.g. `Error: required + * flag(s) "type" not set`), a real parity regression. See the test suite for + * the regression this guards. + */ +export function shouldReportFailure(cause: Cause.Cause, exitCode: number): boolean { + if (exitCode === 0 || exitCode === 130) return false; + return !(Cause.squash(cause) instanceof LegacyGoChildExitError); +} + function projectContextLayerFor(runtimeLayer: Layer.Layer) { return projectContextLayer.pipe(Layer.provide(runtimeLayer), Layer.provide(BunServices.layer)); } @@ -242,13 +264,13 @@ export async function runCli(rootCommand: Command.Command.Any, options: RunCliOp const exit = yield* program.pipe(Effect.exit); if (Exit.isFailure(exit)) { const exitCode = exitCodeForFailure(exit.cause); - // Skip reporting for an interrupted run (130 — a signal, not a - // reportable error) and for a clean `ShowHelp` failure (0). Literal - // `--help` never reaches this branch — it's handled as a successful - // `GlobalFlag.Action` and exits 0 via the success path below. See - // `exitCodeForFailure` for why a "clean" ShowHelp failure (e.g. a bare - // group command with no subcommand) also maps to exit 0. - if (exitCode !== 0 && exitCode !== 130) { + // See `shouldReportFailure` for the reporting rules (and why they're + // NOT keyed on Effect's shared `[Runtime.errorReported]` marker). + // Literal `--help` never reaches this branch — it's handled as a + // successful `GlobalFlag.Action` and exits 0 via the success path + // below. See `exitCodeForFailure` for why a "clean" ShowHelp failure + // (e.g. a bare group command with no subcommand) also maps to exit 0. + if (shouldReportFailure(exit.cause, exitCode)) { yield* output.fail(normalizeCause(exit.cause)); } return yield* processControl.exit(exitCode); diff --git a/apps/cli/src/shared/cli/run.unit.test.ts b/apps/cli/src/shared/cli/run.unit.test.ts index f87d160533..290c9e9557 100644 --- a/apps/cli/src/shared/cli/run.unit.test.ts +++ b/apps/cli/src/shared/cli/run.unit.test.ts @@ -2,7 +2,13 @@ import { Cause } from "effect"; import { CliError } from "effect/unstable/cli"; import { describe, expect, it } from "vitest"; -import { exitCodeForFailure, extractCommandPath, shouldUseGlobalSignalInterrupt } from "./run.ts"; +import { LegacyGoChildExitError } from "../legacy/legacy-go-child-exit.error.ts"; +import { + exitCodeForFailure, + extractCommandPath, + shouldReportFailure, + shouldUseGlobalSignalInterrupt, +} from "./run.ts"; describe("extractCommandPath", () => { it("returns positional command-path tokens", () => { @@ -87,4 +93,55 @@ describe("exitCodeForFailure", () => { it("exits 130 when interrupted, regardless of any other failure reason", () => { expect(exitCodeForFailure(Cause.interrupt())).toBe(130); }); + + // CLI-1879: a delegated Go child's exact exit code (not just a generic 1) + // must reach the user, via the `LegacyGoChildExitError`'s + // `[Runtime.errorExitCode]` marker. + it("exits with a LegacyGoChildExitError's exact exit code", () => { + const cause = Cause.fail( + new LegacyGoChildExitError({ exitCode: 130, message: "supabase-go exited with code 130" }), + ); + expect(exitCodeForFailure(cause)).toBe(130); + }); +}); + +describe("shouldReportFailure", () => { + it("does not report a clean exit (0)", () => { + expect(shouldReportFailure(Cause.fail(new Error("unused")), 0)).toBe(false); + }); + + it("does not report an interrupt (130)", () => { + expect(shouldReportFailure(Cause.interrupt(), 130)).toBe(false); + }); + + // CLI-1879: the child already wrote its own detailed failure to the + // inherited stderr, so `runCli`'s generic line would be a duplicate Go + // itself never prints. + it("does not report a LegacyGoChildExitError", () => { + const cause = Cause.fail( + new LegacyGoChildExitError({ exitCode: 1, message: "supabase-go exited with code 1" }), + ); + expect(shouldReportFailure(cause, 1)).toBe(false); + }); + + it("reports a non-ShowHelp failure", () => { + expect(shouldReportFailure(Cause.fail(new Error("boom")), 1)).toBe(true); + }); + + // Regression guard: `CliError.ShowHelp` ALSO sets Effect's shared + // `[Runtime.errorReported]` marker to `false` (for an unrelated reason — the + // CLI framework already rendered help/usage text). `shouldReportFailure` + // must NOT key on that shared marker, or it would also suppress + // `normalizeCause`'s Go-parity rendering for a `MissingOption` wrapped in + // `ShowHelp` (e.g. `Error: required flag(s) "type" not set`) — silently + // dropping that message for every command with a required flag. + it("still reports a ShowHelp failure carrying a genuine validation error (e.g. a missing required flag)", () => { + const cause = Cause.fail( + new CliError.ShowHelp({ + commandPath: ["sso", "add"], + errors: [new CliError.MissingOption({ option: "--type" })], + }), + ); + expect(shouldReportFailure(cause, 1)).toBe(true); + }); }); diff --git a/apps/cli/src/shared/legacy/go-proxy.layer.ts b/apps/cli/src/shared/legacy/go-proxy.layer.ts index b190d9f109..d359113450 100644 --- a/apps/cli/src/shared/legacy/go-proxy.layer.ts +++ b/apps/cli/src/shared/legacy/go-proxy.layer.ts @@ -8,6 +8,7 @@ import * as ChildProcess from "effect/unstable/process/ChildProcess"; import { ChildProcessSpawner } from "effect/unstable/process/ChildProcessSpawner"; import { CLI_VERSION } from "../cli/version.ts"; import { ProcessControl } from "../runtime/process-control.service.ts"; +import { LegacyGoChildExitError } from "./legacy-go-child-exit.error.ts"; import { LegacyGoProxy } from "./go-proxy.service.ts"; // --------------------------------------------------------------------------- @@ -139,8 +140,8 @@ export function makeGoProxyLayer(opts?: { * Override binary resolution. Primarily a test seam so specs don't have to * mutate `process.env.SUPABASE_GO_BINARY` or stub the filesystem: * - `string` — treat as the resolved Go binary path. - * - `{ notFound: [...] }` — simulate the not-found path; `.exec` will - * print the diagnostic and exit non-zero. + * - `{ notFound: [...] }` — simulate the not-found path; `.exec` will print + * the diagnostic and fail with a non-zero exit code. * * In production, leave unset and let `resolveBinary()` pick the right * artifact for the host platform. @@ -166,12 +167,16 @@ export function makeGoProxyLayer(opts?: { // CLI-1488: never silently fall back to `supabase` on PATH — // when the shim is on PATH and `supabase-go` is not co-located, // that fallback resolves to the shim itself and fork-bombs. - // Print a specific diagnostic and exit non-zero instead. + // Print a specific diagnostic and fail non-zero instead. yield* Effect.sync(() => { process.stderr.write(`${formatGoBinaryNotFoundError(resolved.notFound)}\n`); }); - yield* processControl.exit(1); - return; + return yield* Effect.fail( + new LegacyGoChildExitError({ + exitCode: 1, + message: "supabase-go binary not found", + }), + ); } const binary = resolved.found; @@ -212,7 +217,12 @@ export function makeGoProxyLayer(opts?: { }); const exitCode = yield* spawner.exitCode(command).pipe(Effect.orDie); if (exitCode !== 0) { - yield* processControl.exit(exitCode); + return yield* Effect.fail( + new LegacyGoChildExitError({ + exitCode, + message: `supabase-go exited with code ${exitCode} (see stderr for details)`, + }), + ); } }), ), @@ -223,7 +233,12 @@ export function makeGoProxyLayer(opts?: { yield* Effect.sync(() => { process.stderr.write(`${formatGoBinaryNotFoundError(resolved.notFound)}\n`); }); - return yield* processControl.exit(1); + return yield* Effect.fail( + new LegacyGoChildExitError({ + exitCode: 1, + message: "supabase-go binary not found", + }), + ); } const binary = resolved.found; yield* processControl.holdSignals(["SIGINT", "SIGTERM", "SIGHUP"]); @@ -252,7 +267,12 @@ export function makeGoProxyLayer(opts?: { ); const exitCode = yield* handle.exitCode.pipe(Effect.orDie); if (exitCode !== 0) { - return yield* processControl.exit(exitCode); + return yield* Effect.fail( + new LegacyGoChildExitError({ + exitCode, + message: `supabase-go exited with code ${exitCode} (see stderr for details)`, + }), + ); } return captured; }), diff --git a/apps/cli/src/shared/legacy/go-proxy.layer.unit.test.ts b/apps/cli/src/shared/legacy/go-proxy.layer.unit.test.ts index 8fc4713d7b..7a305197a8 100644 --- a/apps/cli/src/shared/legacy/go-proxy.layer.unit.test.ts +++ b/apps/cli/src/shared/legacy/go-proxy.layer.unit.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it, vi } from "@effect/vitest"; -import { Deferred, Effect, Fiber, Layer, Sink, Stream } from "effect"; +import { Cause, Deferred, Effect, Exit, Fiber, Layer, Sink, Stream } from "effect"; import { ChildProcessSpawner } from "effect/unstable/process"; import { type CliProcessSignal, ProcessControl } from "../runtime/process-control.service.ts"; +import { LegacyGoChildExitError } from "./legacy-go-child-exit.error.ts"; import { LegacyGoProxy } from "./go-proxy.service.ts"; import { formatGoBinaryNotFoundError, makeGoProxyLayer } from "./go-proxy.layer.ts"; @@ -54,12 +55,14 @@ type HoldEvent = * event log. Each acquire gets a monotonically increasing id so tests can * pair an acquire with its release and distinguish concurrent scopes. * - * `exitBehavior`: - * - "never" → exit() blocks on Effect.never (test manages the fiber) - * - "terminateDie" → exit() dies with a tagged defect so callers can - * observe via Effect.exit without juggling fibers + * The layer under test no longer calls `ProcessControl.exit()` itself on a + * non-zero exit or an unresolved binary (CLI-1879 routes both through + * `LegacyGoChildExitError` instead, so `runCli` can run finalizers before + * exiting) — `exit()` here only guards against a future regression that + * reintroduces a direct call; it blocks on `Effect.never` since nothing in + * this file exercises it. */ -function mockProcessControl(opts: { exitBehavior?: "never" | "terminateDie" } = {}) { +function mockProcessControl() { const holdEvents: HoldEvent[] = []; const exitCalls: number[] = []; let nextHoldId = 0; @@ -67,11 +70,7 @@ function mockProcessControl(opts: { exitBehavior?: "never" | "terminateDie" } = const exit = (code: number) => Effect.sync(() => { exitCalls.push(code); - }).pipe( - Effect.flatMap(() => - opts.exitBehavior === "terminateDie" ? Effect.die("EXIT_CALLED" as const) : Effect.never, - ), - ); + }).pipe(Effect.flatMap(() => Effect.never)); return { get holdEvents() { @@ -284,18 +283,52 @@ describe("makeGoProxyLayer", () => { }).pipe(Effect.provide(layer)); }); - it.effect("propagates non-zero exit codes via ProcessControl.exit", () => { + it.effect("propagates non-zero exit codes via LegacyGoChildExitError", () => { const spawner = mockSpawner({ kind: "success", code: 7 }); - // Use the terminating exit variant so we can observe via Effect.exit - // without juggling forked fibers around Effect.never. - const pc = mockProcessControl({ exitBehavior: "terminateDie" }); + const pc = mockProcessControl(); const layer = makeGoProxyLayer({ binary: TEST_BINARY }).pipe( Layer.provide(Layer.mergeAll(spawner.layer, pc.layer)), ); return Effect.gen(function* () { const proxy = yield* LegacyGoProxy; - yield* proxy.exec(["some", "command"]).pipe(Effect.exit); - expect(pc.exitCalls).toEqual([7]); + const exit = yield* proxy.exec(["some", "command"]).pipe(Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(7); + } + // The layer itself never calls `ProcessControl.exit` — that's now + // `runCli`'s job, after finalizers have run. + expect(pc.exitCalls).toEqual([]); + }).pipe(Effect.provide(layer)); + }); + + it.effect("lets an Effect.ensuring finalizer run after a non-zero exit (CLI-1879)", () => { + // The whole point of routing a non-zero exit through `LegacyGoChildExitError` + // instead of `ProcessControl.exit()` (a real `process.exit()` in production): + // a caller's own `Effect.ensuring` finalizer — e.g. a handler's + // `Effect.ensuring(telemetryState.flush)` — must still run. Under the old + // `processControl.exit()`-based implementation this finalizer would never fire + // (production: the process would already be dead; this mock's `exit()` blocks + // forever on `Effect.never`, so the fiber never reaches `Effect.exit` either). + const spawner = mockSpawner({ kind: "success", code: 5 }); + const pc = mockProcessControl(); + const layer = makeGoProxyLayer({ binary: TEST_BINARY }).pipe( + Layer.provide(Layer.mergeAll(spawner.layer, pc.layer)), + ); + let finalizerRan = false; + return Effect.gen(function* () { + const proxy = yield* LegacyGoProxy; + yield* proxy.exec(["some", "command"]).pipe( + Effect.ensuring( + Effect.sync(() => { + finalizerRan = true; + }), + ), + Effect.exit, + ); + expect(finalizerRan).toBe(true); }).pipe(Effect.provide(layer)); }); @@ -401,36 +434,75 @@ describe("makeGoProxyLayer", () => { // literal string "supabase" when no Go binary was found, which when run from // a PATH that contained the shim would fork-bomb the shim against itself // (silent multi-minute hang in CI followed by SIGTERM). The layer must now - // refuse to spawn anything and surface a specific diagnostic + non-zero exit. - it.effect("prints a diagnostic and exits 1 when supabase-go cannot be resolved", () => { - const spawner = mockSpawner({ kind: "success", code: 0 }); - const pc = mockProcessControl({ exitBehavior: "terminateDie" }); - const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); - const tried = [ - "$SUPABASE_GO_BINARY (unset)", - "/usr/local/bin/supabase-go (not found alongside the shim)", - ]; - const layer = makeGoProxyLayer({ binary: { notFound: tried } }).pipe( - Layer.provide(Layer.mergeAll(spawner.layer, pc.layer)), - ); - return Effect.gen(function* () { - const proxy = yield* LegacyGoProxy; - yield* proxy.exec(["db", "start"]).pipe(Effect.exit); - - // Did NOT spawn anything — the whole point is to refuse the fork-bomb. - expect(spawner.spawned).toHaveLength(0); - // Exited with code 1 via ProcessControl.exit. - expect(pc.exitCalls).toEqual([1]); - // Wrote the diagnostic to stderr, including each tried location. - expect(stderr).toHaveBeenCalledTimes(1); - const written = String(stderr.mock.calls[0]![0]); - expect(written).toContain("Could not find the `supabase-go` binary"); - expect(written).toContain("$SUPABASE_GO_BINARY (unset)"); - expect(written).toContain("/usr/local/bin/supabase-go"); - expect(written).toContain("SUPABASE_GO_BINARY"); - stderr.mockRestore(); - }).pipe(Effect.provide(layer)); - }); + // refuse to spawn anything and surface a specific diagnostic + a + // `LegacyGoChildExitError` carrying exit code 1. + it.effect( + "prints a diagnostic and fails with exit code 1 when supabase-go cannot be resolved", + () => { + const spawner = mockSpawner({ kind: "success", code: 0 }); + const pc = mockProcessControl(); + const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + const tried = [ + "$SUPABASE_GO_BINARY (unset)", + "/usr/local/bin/supabase-go (not found alongside the shim)", + ]; + const layer = makeGoProxyLayer({ binary: { notFound: tried } }).pipe( + Layer.provide(Layer.mergeAll(spawner.layer, pc.layer)), + ); + return Effect.gen(function* () { + const proxy = yield* LegacyGoProxy; + const exit = yield* proxy.exec(["db", "start"]).pipe(Effect.exit); + + // Did NOT spawn anything — the whole point is to refuse the fork-bomb. + expect(spawner.spawned).toHaveLength(0); + // Failed with a LegacyGoChildExitError carrying exit code 1, rather than + // calling `ProcessControl.exit` directly — that's now `runCli`'s job. + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(1); + } + expect(pc.exitCalls).toEqual([]); + // Wrote the diagnostic to stderr, including each tried location. + expect(stderr).toHaveBeenCalledTimes(1); + const written = String(stderr.mock.calls[0]![0]); + expect(written).toContain("Could not find the `supabase-go` binary"); + expect(written).toContain("$SUPABASE_GO_BINARY (unset)"); + expect(written).toContain("/usr/local/bin/supabase-go"); + expect(written).toContain("SUPABASE_GO_BINARY"); + stderr.mockRestore(); + }).pipe(Effect.provide(layer)); + }, + ); + + it.effect( + "execCapture also prints a diagnostic and fails with exit code 1 when supabase-go cannot be resolved", + () => { + const spawner = mockSpawner({ kind: "success", code: 0 }); + const pc = mockProcessControl(); + const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + const tried = ["$SUPABASE_GO_BINARY (unset)"]; + const layer = makeGoProxyLayer({ binary: { notFound: tried } }).pipe( + Layer.provide(Layer.mergeAll(spawner.layer, pc.layer)), + ); + return Effect.gen(function* () { + const proxy = yield* LegacyGoProxy; + const exit = yield* proxy.execCapture(["db", "dump"]).pipe(Effect.exit); + + expect(spawner.spawned).toHaveLength(0); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause); + expect(error).toBeInstanceOf(LegacyGoChildExitError); + expect((error as LegacyGoChildExitError).exitCode).toBe(1); + } + expect(pc.exitCalls).toEqual([]); + expect(stderr).toHaveBeenCalledTimes(1); + stderr.mockRestore(); + }).pipe(Effect.provide(layer)); + }, + ); it.effect("opens and closes a fresh hold scope per sequential exec call", () => { const spawner = mockSpawner({ kind: "success", code: 0 }); diff --git a/apps/cli/src/shared/legacy/go-proxy.service.ts b/apps/cli/src/shared/legacy/go-proxy.service.ts index e9539e75a4..6ea6a50eb4 100644 --- a/apps/cli/src/shared/legacy/go-proxy.service.ts +++ b/apps/cli/src/shared/legacy/go-proxy.service.ts @@ -1,11 +1,15 @@ import type { Effect } from "effect"; import { Context } from "effect"; +import type { LegacyGoChildExitError } from "./legacy-go-child-exit.error.ts"; interface LegacyGoProxyShape { /** * Forward the given args to the Go binary, inheriting stdin/stdout/stderr - * and propagating the exit code. On a non-zero exit the process exits with - * the same code — callers do not need to handle the failure case. + * and propagating the exit code. On a non-zero exit (or when the binary + * cannot be resolved at all), fails with `LegacyGoChildExitError` carrying + * the child's exact exit code; callers don't need to special-case it — it + * flows through the normal Effect failure channel up to `runCli`, which + * maps it to the real process exit code after running any finalizers. * * `opts.cwd` overrides the working directory for this call (falls back to the * layer's construction-time cwd). `opts.env` overlays extra environment @@ -17,13 +21,16 @@ interface LegacyGoProxyShape { readonly exec: ( args: ReadonlyArray, opts?: { readonly cwd?: string; readonly env?: Record }, - ) => Effect.Effect; + ) => Effect.Effect; /** * Like `exec`, but captures the child's stdout and returns it as a string * instead of inheriting stdout. stderr is still inherited (so progress / - * diagnostics pass straight through), and a non-zero exit still terminates the - * process with the same code. + * diagnostics pass straight through). On a non-zero exit (or when the binary + * cannot be resolved at all), fails with `LegacyGoChildExitError` carrying + * the child's exact exit code; callers don't need to special-case it — it + * flows through the normal Effect failure channel up to `runCli`, which + * maps it to the real process exit code after running any finalizers. * * `opts.stdin` controls the child's stdin: `"inherit"` (default) keeps the * child interactive (its prompts reach the terminal); `"ignore"` gives it a @@ -43,7 +50,7 @@ interface LegacyGoProxyShape { readonly env?: Record; readonly stdin?: "inherit" | "ignore"; }, - ) => Effect.Effect; + ) => Effect.Effect; } export class LegacyGoProxy extends Context.Service()( diff --git a/apps/cli/src/shared/legacy/legacy-go-child-exit.error.ts b/apps/cli/src/shared/legacy/legacy-go-child-exit.error.ts new file mode 100644 index 0000000000..d2b7f22ef3 --- /dev/null +++ b/apps/cli/src/shared/legacy/legacy-go-child-exit.error.ts @@ -0,0 +1,55 @@ +import { Data, Runtime } from "effect"; + +/** + * A spawned `supabase-go` child process — via `LegacyGoProxy.exec`/`execCapture`, + * or the hidden `db __db-bootstrap` seam (`legacy-db-bootstrap.seam.layer.ts`) — + * exited non-zero, or could not be spawned at all (binary not found). + * + * Carries the child's exact exit code through Effect's `Runtime.errorExitCode` + * marker, so both `runCli` (`shared/cli/run.ts`, text mode) and + * `withJsonErrorHandling` (`shared/output/json-error-handling.ts`, `json`/ + * `stream-json` mode) map the process's own exit code to this EXACT number — + * not a generic `1` — and only AFTER every `Effect.ensuring` finalizer between + * the call site and there has already run (telemetry flush, command + * instrumentation). Calling `ProcessControl.exit()` directly from deep inside a + * handler skips those finalizers entirely (`process.exit()` halts the process + * before the Effect runtime can unwind the remaining scopes) — this error type + * lets the child's status flow through the normal Effect failure channel + * instead, all the way up to the single `ProcessControl.exit()` call `runCli` + * itself makes once finalizers are done. See CLI-1879. + * + * `runCli`'s `handledProgram` special-cases this exact class (an `instanceof` + * check, not a shared Effect marker) to skip its generic `output.fail` stderr + * line in text mode — the child already wrote its own detailed failure (or, + * for the not-found case, `LegacyGoProxy`'s own specific diagnostic) to the + * parent's inherited stderr, and Go itself never prints a second, generic line + * on top of that. This is deliberately NOT keyed on Effect's shared + * `[Runtime.errorReported]` marker: `CliError.ShowHelp` also sets that marker + * to `false` for an unrelated reason (the CLI framework already rendered + * help/usage text), and gating on the marker there would ALSO suppress + * `normalizeCause`'s Go-parity rendering for a `MissingOption` wrapped in + * `ShowHelp` (e.g. `Error: required flag(s) "type" not set`) — a real parity + * regression. `withJsonErrorHandling` has no such collision (it runs upstream + * of `runCli`, catching every error uniformly) and still emits the structured + * JSON error envelope for this error like any other. + * + * The envelope's `message` is deliberately generic (`"supabase-go exited with + * code N (see stderr for details)"`) rather than the child's specific failure + * reason: the child's real detail is on stderr (see above), which a + * machine-output consumer reading only stdout won't see — this is an accepted, + * TS-only tradeoff (Go itself has no JSON error-envelope concept to match + * against here), not a parity gap. + * + * Invariant: `exitCode` must be a real non-zero child exit status (1-255, + * matching `ChildProcessSpawner.ExitCode`'s POSIX range), never `0` — every + * construction site guards on `exitCode !== 0` (or hardcodes `1` for the + * binary-not-found case) before constructing this error, since a `0` here + * would be a failure that both `runCli` and `withJsonErrorHandling` read back + * as a *successful* exit. + */ +export class LegacyGoChildExitError extends Data.TaggedError("LegacyGoChildExitError")<{ + readonly exitCode: number; + readonly message: string; +}> { + override readonly [Runtime.errorExitCode] = this.exitCode; +} diff --git a/apps/cli/src/shared/output/json-error-handling.ts b/apps/cli/src/shared/output/json-error-handling.ts index 756077692f..2a886e8cf8 100644 --- a/apps/cli/src/shared/output/json-error-handling.ts +++ b/apps/cli/src/shared/output/json-error-handling.ts @@ -1,4 +1,4 @@ -import { Effect } from "effect"; +import { Effect, Runtime } from "effect"; import { Output } from "./output.service.ts"; import { ProcessControl } from "../runtime/process-control.service.ts"; import { normalizeCliError } from "./normalize-error.ts"; @@ -13,7 +13,13 @@ export const withJsonErrorHandling = ( const processControl = yield* ProcessControl; if (output.format === "text") return yield* Effect.fail(error); yield* output.fail(normalizeCliError(error)); - yield* processControl.setExitCode(1); + // `Runtime.getErrorExitCode` defaults to 1 for any error without a + // `[Runtime.errorExitCode]` marker, so this is a no-op for every existing + // error type. It only changes behavior for an error that opts in — e.g. + // `LegacyGoChildExitError` (CLI-1879), so a delegated Go child's exact exit + // code (not just a generic 1) still reaches the user under json/stream-json, + // matching the exit code `runCli`'s text-mode path already propagates. + yield* processControl.setExitCode(Runtime.getErrorExitCode(error)); }), ), ); diff --git a/apps/cli/src/shared/output/json-error-handling.unit.test.ts b/apps/cli/src/shared/output/json-error-handling.unit.test.ts index e763b8df19..29e7ec90c8 100644 --- a/apps/cli/src/shared/output/json-error-handling.unit.test.ts +++ b/apps/cli/src/shared/output/json-error-handling.unit.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; import { Data, Effect, Exit, Layer, Option } from "effect"; import { mockProcessControl } from "../../../tests/helpers/mocks.ts"; +import { LegacyGoChildExitError } from "../legacy/legacy-go-child-exit.error.ts"; import { Output } from "./output.service.ts"; import { withJsonErrorHandling } from "./json-error-handling.ts"; @@ -181,5 +182,24 @@ describe("withJsonErrorHandling", () => { expect(out.failCalls[0]?.message).toBe("plain error message"); }).pipe(Effect.provide(out.layer), Effect.provide(processControl.layer)); }); + + // CLI-1879: a delegated Go child's exact exit code must reach the user under + // json/stream-json too, not just a generic 1 — matching the exit code + // `runCli`'s text-mode path already propagates via the same + // `[Runtime.errorExitCode]` marker. + it.live("sets the exact exit code for a LegacyGoChildExitError, not a generic 1", () => { + const out = mockOutput("json"); + const processControl = mockProcessControl(); + return Effect.gen(function* () { + const error = new LegacyGoChildExitError({ + exitCode: 130, + message: "supabase-go exited with code 130 (see stderr for details)", + }); + yield* withJsonErrorHandling(Effect.fail(error)).pipe(Effect.provide(out.layer)); + expect(out.failCalls).toHaveLength(1); + expect(out.failCalls[0]?.code).toBe("LegacyGoChildExitError"); + expect(processControl.exitCode).toBe(130); + }).pipe(Effect.provide(out.layer), Effect.provide(processControl.layer)); + }); }); });