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)); + }); }); });