feat(cli): port supabase start command to native TypeScript#5847
feat(cli): port supabase start command to native TypeScript#5847Coly010 wants to merge 38 commits into
Conversation
Ports `supabase start` from a Go-binary proxy to native TypeScript in the legacy CLI shell (CLI-1323), continuing the local-dev-stack shell migration alongside the already-ported `stop`/`status`/`db push`/`db reset`/`db start`. ## What changed Talks directly to Docker/Podman via subprocess, mirroring Go's sequential per-container `DockerStart` — no Docker Compose, and no `@supabase/stack/effect` orchestration (that runtime targets a deliberately different local-dev product and would silently manage the wrong set of containers). Brings up all 14 containers in Go's real start order, with per-service config-boolean + `--exclude` gating, Go-byte-exact env/image resolution, a bulk health-check phase (Docker healthcheck for most services, an HTTP-HEAD-through-Kong bypass for PostgREST/Edge Runtime), and full rollback on any bring-up failure. Also natively implements the fresh-volume DB schema/migration/seed pipeline, Edge Runtime container bring-up, and fresh-volume storage bucket seeding — previously tracked as out-of-scope follow-ups for this port, now closed. Only the linked-project version-check suggestion (a best-effort "update available" hint with zero Management API dependency otherwise) remains out of scope by design. ## Notable review-driven fixes - `legacyParseGoDuration` (the `config.toml` duration-string parser feeding Go-parity env vars like `GOTRUE_SESSIONS_TIMEBOX`) silently accepted a malformed duration with no digits (a bare unit like `"s"`, or a lone `"."` with no digits on either side) and returned `0` instead of erroring like Go's real `time.ParseDuration` — both cases now throw Go's exact `time: invalid duration "..."` message. - `start.services.ts`'s descriptive `enabledGate` metadata for Mailpit referenced the deprecated `inbucket` config section instead of its `local_smtp` rename. Fixed, and added a mechanical cross-check test that evaluates every service's `enabledGate` string against `start.gates.ts`'s real computed gate across a battery of synthetic configs, so a future drift between the two fails loudly instead of silently. - Confirmed and closed the PostgREST health-check's TLS/CA trust gap for `[api.tls] enabled = true` local stacks (self-signed Kong cert) — the local-Kong-CA-trust mechanism already used by `seed buckets`/`storage`/ `db reset` is now wired into `start`'s health-check HTTP client too. Closes CLI-1323
|
👋 Thanks for the contribution! This pull request isn't linked to a tracked issue, so it's being closed automatically. Please open an issue first, wait for a maintainer to add the |
…3-port-supabase-start
|
👋 Thanks for the contribution! This pull request isn't linked to a tracked issue, so it's being closed automatically. Please open an issue first, wait for a maintainer to add the |
|
👋 Thanks for the contribution! This pull request isn't linked to a tracked issue, so it's being closed automatically. Please open an issue first, wait for a maintainer to add the |
|
👋 Thanks for the contribution! This pull request isn't linked to a tracked issue, so it's being closed automatically. Please open an issue first, wait for a maintainer to add the |
) The contribution gate identified internal maintainers solely from the PR's `author_association`, which GitHub only reports as `MEMBER` when a user's organization membership is public. A private org member (e.g. a `supabase/cli` team member who keeps membership private) is reported as `CONTRIBUTOR`/`NONE`, so the gate wrongly closed their PRs as "no-linked-issue" (see supabase#5847). Resolve maintainer status from the author's effective repository permission (`admin`/`write`), which reflects team/org-granted access that `author_association` does not surface, falling back to it only when the cheap signals (bot, public internal association) are inconclusive. The permission endpoint needs just `Metadata: read`, already covered by the workflow's `contents: read`. Claude-Session: https://claude.ai/code/session_01D41gYiFBSU7adE4ppnUUKg --------- Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 623dd8b18e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 623dd8b18e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
supabase start's TS port had five spots where Go's real behaviour was
silently dropped:
- supabase/.temp/storage-migration (the linked project's Storage
migration pin) was never read, so DB_MIGRATIONS_FREEZE_AT was always
empty on a fresh DB setup and the Storage container itself.
- supabase/.temp/{gotrue,rest,storage,realtime,studio,pgmeta,logflare,
pooler}-version pins were never applied to the images start
pulls/creates, unlike Go's Config.Load rewrite. Hoisted the existing
services command's reader into shared/legacy-service-version-overrides.ts
and reused services.shared.ts's tag-rewrite helpers instead of a third
copy.
- --network-id was read by several other native ports but never by
start itself, so the override never reached any container or the
Docker network start creates.
- SUPABASE_API_PORT's env override was computed for status URLs but
never exposed for Kong/Edge Runtime's own container specs, so they
kept publishing/binding the un-overridden config.api.port.
- Studio's function bind mounts were hardcoded to [], unlike Go's
unconditional (Edge-Runtime-independent) PopulatePerFunctionConfigs
call — extracted the existing per-function bind computation in
shared/functions/serve.ts into a reusable resolveFunctionBindMounts
so both callers share one calculation.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…l in start Three more Go-parity gaps in supabase start found by review: - db.root_key (including encrypted: values) was never resolved or passed to the Postgres container, so every project booted with the hard-coded default pgsodium key instead of a customized one, breaking decryption of existing encrypted data. Resolved in legacyResolveLocalConfigValues off the raw config document (unmodeled in @supabase/config's schema), reusing the existing decrypt helpers. - DockerStart's Linux-only host.docker.internal:host-gateway mapping was already correctly ported for the one-shot migrate jobs and Edge Runtime bring-up, but never made it into the common legacyStartContainer path the other 13 services go through. - auth.external_url (also unmodeled in the schema, with its own Go regression test) was never read, so GoTrue's API_EXTERNAL_URL/JWT issuer default/mailer verify URL/OAuth redirect fallbacks always derived from apiUrl even when a project intentionally exposed auth at a different host.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…es tag ordering Three more Go-parity gaps found by review: - [auth.email.smtp] present without an explicit enabled key should default to enabled (Go sets this at load time), but the schema always decodes enabled: false when the key is absent, so GoTrue silently fell back to Mailpit. Reuses the existing correct resolution already implemented for config validation, exposed as legacyResolveAuthEmailSmtp. - auth.external providers outside the schema's fixed ~19-provider set (e.g. a custom [auth.external.my_oidc] block) never reached GoTrue's env, even though Go's Auth.External is a genuine map iterated unconditionally and this port's own config validation already accepts arbitrary provider names. Reuses the same raw-document iteration validateAuthExternalProviders already established. - A registry override with a port (SUPABASE_INTERNAL_IMAGE_REGISTRY= localhost:5000) broke the Postgres version-tag comparison, since Go compares the pre-registry-rewrite image while this port was comparing the already-rewritten one. Threaded the original image through as a separate configImage field used only for that comparison.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e5cd58c9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Effect.tapError(() => | ||
| legacyRollbackStart(spawner, filterValue, isFreshVolume, cliConfig.workdir), | ||
| ), |
There was a problem hiding this comment.
When the new native start flow is aborted with Ctrl-C/SIGTERM after Docker side effects begin, this tapError cleanup does not run because interruptions/process termination are not typed failures. The root runtime still treats start as self-managed in shared/cli/run.ts, so there is no global signal wrapper to coordinate cancellation either; the result is that containers, volumes, networks, and staged secret directories already created by bringUp can be left behind instead of taking the Go rollback path. Please add an interrupt/signal finalizer around the start flow, or re-enable global signal handling and roll back on interrupt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this — you're right, and I dug into it enough to confirm it's a real gap, not just an Effect-semantics quibble.
Go genuinely does clean up on Ctrl-C for start, but not via a dedicated signal handler: cmd/root.go:99 wraps the command's context in signal.NotifyContext(..., os.Interrupt), and the ctx-aware Docker/health-check waits throughout the bring-up (e.g. internal/db/start/start.go:200-212, internal/utils/docker.go:544-553) turn a SIGINT into a plain context.Canceled error value that reaches internal/start/start.go:73-81's ordinary if err != nil { rollback } branch — the same branch any other failure takes. Effect's interruption doesn't work that way: it's a separate signal outside the E channel, which is exactly why shared/functions/serve.ts already uses Effect.onInterrupt (not tapError) for its own cleanup. start.handler.ts's rollback (line ~1948) is tapError-only, so it won't fire on interruption — and worse, start is currently excluded from the global signal wrapper (shared/cli/run.ts's selfManagedSignalCommands, a holdover from when start was still a Go-proxy) and never calls ProcessControl.awaitSignal() itself, so today a raw Ctrl-C most likely kills the process outright before any finalizer — including the telemetry-flush Effect.ensuring — gets a chance to run.
I'm not comfortable landing a narrow patch for this in isolation, though, because the "obvious" fix collides with something this codebase already solved deliberately: the one-shot Postgres migrate job inside start's bring-up goes through LegacyDockerRun.runCapture, which calls processControl.holdSignals(...) specifically so a foreground SIGINT reaches its inherited-stdio child rather than being captured by the parent — the identical reasoning that keeps db reset out of the global wrapper. Wiring start into the generic wrapper's Fiber.interrupt path risks re-racing that. On top of that, Go's own exit code for an interrupted, rolled-back start is a plain 1 (cmd/root.go:235-239,310), not the 130 convention exitCodeForFailure already gives every other native command that goes through the shared wrapper — so a real fix needs a deliberate decision on exit-code handling too, not just rollback wiring.
Given this touches the shared signal wrapper, an already-tuned sibling behavior (the migrate-job's holdSignals), and an existing cross-command exit-code convention, I'd like to loop in @columferry (or whoever owns this design) on whether the right shape is (a) a start-local signal race like functions serve's, scoped so it doesn't touch the migrate-job's existing holdSignals window, or (b) opting start into the generic global wrapper with an explicit exit-code override for the interrupted case. Flagging this as a design decision rather than folding a speculative fix into this PR's diff — happy to scope and implement once there's agreement on the approach.
|
@codex review |
…in-container behavior (review: #5847) Go writes Kong's kong.yml and Postgres's pgsodium_root.key inside the container via root-authored heredocs, landing at world-readable 0644. This port instead stages them on the host at 0600 and bind-mounts them read-only; on real Linux/Podman bind mounts that host uid/mode is preserved verbatim, so Kong (uid 100) and Postgres's post-privilege-drop postgres user (uid 100) get EACCES reading them. The enclosing 0700 per-container directory is unchanged and remains the actual host-side protection via ancestor-directory execute/search checks, so this does not reopen host-level access.
|
@codex review |
…5847) Go's DockerImagePullWithRetry(ctx, image, 2) retries any non-canceled pull failure unconditionally, twice, with escalating 4s/8s backoff. This port allowed only one retry at a flat 500ms and gated it on the failure text matching a local regex allowlist, so transient errors worded differently than expected (or a second consecutive failure) would give up where Go would retry and often recover. Removed the message-pattern gating and matched Go's retry count/backoff exactly, with a new unit test covering a non-matching error message. Also bumped one existing integration test's timeout (10s -> 45s, matching this file's own convention for multi-candidate scenarios) since exhausting all 3 registry candidates now takes ~36s of real backoff, same as Go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f53bf09a5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // raw `--exclude` values are enough on their own. | ||
| const statusExcluded = flags.exclude; | ||
|
|
||
| if (output.format === "text") { |
There was a problem hiding this comment.
When a user runs supabase start -o json (or env/toml/yaml), legacy/cli/root.ts keeps the Output service in the quiet text layer for legacy machine --output values and only forwards --output through globalArgs to Go proxies. Since this native handler no longer reads LegacyOutputFlag, this output.format === "text" branch is taken and emits the pretty table/progress text instead of the machine status payload that the previous proxy produced by running status --output <format> after start, breaking scripts that depend on start -o ....
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same question as the other threads on this PR asking start to honor LegacyOutputFlag for machine output formats — already investigated, verdict applies identically here.
Go's own start command never honors -o/--output for its status render. In internal/start/start.go, the already-running shortcut hardcodes utils.OutputPretty (not utils.OutputFormat.Value) when calling status.Run, and the fresh-start tail calls status.PrettyPrint(os.Stdout, ...), which has no format parameter at all. grep -rn OutputFormat apps/cli-go/internal/start/ returns nothing — Go's start binary prints the pretty table on stdout for -o json exactly as it does with no -o.
The -o json → JSON behavior from the previous shell was itself a TS-only addition (#5687, addressing #4675) that ran Go's start binary silently and then shelled out a second time to Go's status --output <fmt> — it was never something Go's own start.Run did. This PR's SIDE_EFFECTS.md documents the resulting decision: start has no Go -o flag of its own, so machine output is still available via --output-format json/stream-json, just no longer double-triggered by the Go-compat -o flag. Adding a LegacyOutputFlag branch here would make our start -o json diverge from real Go CLI output, not restore parity.
| debug, | ||
| platform: runtimeInfo.platform, | ||
| }; | ||
| const runtime: StartedRuntime = yield* legacyStartEdgeRuntimeContainer(edgeRuntimeInput); |
There was a problem hiding this comment.
On Podman-only hosts (no docker binary, podman available), this Edge Runtime path bypasses the container CLI fallback used by the rest of native start: image pulls and legacyStartContainer go through spawnContainerCli, but legacyStartEdgeRuntimeContainer delegates to startEdgeRuntimeContainer, whose volume/network/run steps still invoke runChildProcess("docker", ...) in apps/cli/src/shared/functions/serve.ts. Since [edge_runtime] enabled = true is the default, supabase start now fails at Edge Runtime even though the earlier containers can be created with Podman.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this — I looked closely and don't believe there's an actual gap here, though I understand why it reads that way.
startEdgeRuntimeContainer's volume/network/run/reload/inspect calls (shared/functions/serve.ts — ensureDockerNamedVolume/ensureDockerNetwork at ~1398-1399, the docker run at ~1501, reloadKong/bestEffortRemoveContainer/assertLocalDbRunning/inspectContainerExitCode at ~1082-1201) all go through the shared runChildProcess helper (shared/functions/deploy.ts:1284-1316). runChildProcess doesn't actually spawn the literal string it's handed — it always delegates to spawnContainerCli (legacy/shared/legacy-container-cli.ts:51-70), which tries docker and falls back to podman if that can't be spawned. The "docker" argument at each call site only sets the extendEnv default and preserves a DI seam for functions serve; the helper's own comment spells this out explicitly. That wiring shipped in #5660, before this start port, so every Edge Runtime helper already has Podman fallback — not just docker logs streaming, which calls spawnContainerCli directly (serve.ts:1111).
On the Go side there's no "docker vs podman binary" distinction to compare this against in the first place: Go never shells out to a CLI binary for container ops — ServeFunctions (internal/functions/serve/serve.go:212) and every other Docker-touching code path use the same singleton Docker Engine API client (internal/utils/docker.go:39-55) uniformly, including Edge Runtime bring-up, with any Podman support coming from Podman's Docker-API-compatible socket rather than binary selection. So Go's actual invariant — one mechanism, used everywhere including Edge Runtime — is exactly what this port already has; it's just split across two equivalent call shapes (spawnContainerCli directly in container-lifecycle.ts, and via the runChildProcess wrapper in serve.ts/deploy.ts).
This is a duplicate of the same claim raised across multiple threads on this PR — closing all of them with the same evidence. Happy to reopen if you can point to a specific Edge Runtime code path that reaches a bare docker/podman spawn outside spawnContainerCli/runChildProcess.
| // outright (Go's per-block `if err != nil { return err }`, propagated to | ||
| // `Run`'s rollback branch, `start.go:73-81`). | ||
| const bringUp = Effect.gen(function* () { | ||
| yield* legacyEnsureStartNetwork(spawner, networkId, { |
There was a problem hiding this comment.
When --network-id is set to a built-in Docker network such as host, bridge, or none, this unconditional create step runs before any container starts. Go's DockerNetworkCreateIfNotExists skips network modes that are not user-defined, and the shared TS helper in functions/deploy.ts does the same for default/bridge/host/none; here we always call docker network create <networkId>, so a valid built-in network override can fail at startup with a network-create error instead of using the existing network.
Useful? React with 👍 / 👎.
…review: #5847) Go's DockerStart sets both the CLI project label and the compose project label on config.Labels before creating anything, then passes that same map into DockerNetworkCreateIfNotExists, so Go's network carries both labels just like its containers and volumes. This port's containers/volumes already merge both labels correctly, but the network-creation call site only passed the CLI label, leaving fresh start networks outside compose-project-label-based grouping/tooling.
…th minutes/hours (review: #5847) Go's time.Duration.String() computes the fractional-seconds remainder from the whole nanosecond count before peeling off minutes/hours, so it always survives in the output (e.g. 1h0.5s -> 1h0m0.5s). This port built the seconds component as a bare integer whenever an hours/minutes prefix was present, silently truncating any sub-second remainder. auth.sessions.timebox, auth.sms.max_frequency, and similar config duration fields are formatted through this function before being passed as GoTrue env vars, so a configured value like "1h0.5s" was regressing to a shorter effective timeout/rate-limit than Go CLI sets.
|
@codex review |
…eview: #5847) Go's ServeFunctions (the actual Edge Runtime bring-up) never reloads Kong; only restartEdgeRuntime does, after ServeFunctions succeeds, and that wrapper is used exclusively by functions serve's own restart loop. start.go calls serve.ServeFunctions directly, bypassing restartEdgeRuntime entirely, so Go's start never reloads Kong. This port had the reload baked into startEdgeRuntimeContainer (the ServeFunctions port), so start's Edge Runtime bring-up inherited a Kong reload Go's start never performs -- which can race Kong's own startup on a cold start and emit a misleading warning. Moved the reload into startEdgeRuntime (the restartEdgeRuntime port), so only functions serve's restart path triggers it, matching Go exactly.
…r, gated on confirmed removal (review: #5847) Two related bugs in stop's staged-secret cleanup (Kong/Postgres/ Supavisor plaintext files under <workdir>/supabase/.temp/start-secrets): Go has no equivalent here (it never stages secrets on host disk), so neither is a parity question, but both are genuine TS-side correctness bugs. 1. Cleanup always used the invoking cwd's workdir, never the actual container's. Tearing down another project's containers via --all or --project-id orphaned that project's plaintext secret files on disk forever, since the removed containers can no longer be rediscovered via docker ps. Fixed by stamping each container with its own workdir label at creation time and reading it back through the existing single docker ps listing (no extra Engine API call), falling back to the caller's workdir only when the label is absent. 2. Cleanup ran via Effect.ensuring keyed off the pre-teardown listing, so it fired even when the stop stage itself failed and container prune never ran -- deleting secrets for containers that were never actually removed and might still be live. Fixed by moving the capture point to fire only once container prune has confirmed removal, while still firing on a later volume/network prune failure (preserving the existing 983eab9 fix for that case). Applied the same fix to start's own rollback path (start.rollback.ts), which had the identical pattern.
There was a problem hiding this comment.
💡 Codex Review
cli/apps/cli/src/shared/functions/serve.ts
Line 1512 in f799a9f
Keep Kong reload out of start
Checked the Go paths: supabase start calls ServeFunctions directly (apps/cli-go/internal/start/start.go:1101-1108), while only standalone functions serve's restartEdgeRuntime reloads Kong afterward (apps/cli-go/internal/functions/serve/serve.go:124-130). Because this shared helper is also used by legacyStartEdgeRuntimeContainer, supabase start --exclude kong still attempts docker exec <kong> kong reload and emits a warning for an intentionally excluded gateway; move the reload back to the standalone serve wrapper or gate it away for start.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // raw `--exclude` values are enough on their own. | ||
| const statusExcluded = flags.exclude; | ||
|
|
||
| if (output.format === "text") { |
There was a problem hiding this comment.
When users pass the Go-compatible machine flag (for example supabase start -o json or --output yaml), legacyRoot keeps output.format as "text" because LegacyOutputFlag is separate from --output-format; the previous wrapper explicitly read LegacyOutputFlag and ran status --output <format>. Since this branch only keys off output.format, those invocations now print the pretty status text instead of the requested JSON/YAML/TOML/env payload, and the already-running branch has the same text-only check, breaking scripts that consume start -o ....
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same question already investigated and rejected on other threads on this PR asking start to honor LegacyOutputFlag for machine output formats — verdict applies identically here.
Go's own start command never honors -o/--output for its status render. In internal/start/start.go, the already-running shortcut hardcodes utils.OutputPretty (not utils.OutputFormat.Value) when calling status.Run, and the fresh-start tail calls status.PrettyPrint(os.Stdout, ...), which has no format parameter at all. grep -rn OutputFormat apps/cli-go/internal/start/ returns nothing — Go's start binary prints the pretty table on stdout for -o json exactly as it does with no -o.
The -o json → JSON behavior from the previous shell was itself a TS-only addition (#5687, addressing #4675) that ran Go's start binary silently and then shelled out a second time to Go's status --output <fmt> — it was never something Go's own start.Run did. This PR's SIDE_EFFECTS.md documents the resulting decision: start has no Go -o flag of its own, so machine output is still available via --output-format json/stream-json, just no longer double-triggered by the Go-compat -o flag. Adding a LegacyOutputFlag branch here would make our start -o json diverge from real Go CLI output, not restore parity.
| Effect.tapError(() => | ||
| legacyRollbackStart(spawner, filterValue, isFreshVolume, cliConfig.workdir), | ||
| ), |
There was a problem hiding this comment.
If the user presses Ctrl-C during the new native bring-up, this cleanup path is skipped: start remains excluded from the global ProcessControl.awaitSignal wrapper, and tapError only runs for Effect failures rather than a process signal/interrupt. The old Go proxy owned SIGINT through its command context and removed resources on cancellation; now a partially completed start can leave created containers, volumes, the network, and staged secret files behind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this — you're right, and I dug into it enough to confirm it's a real gap, not just an Effect-semantics quibble.
Go genuinely does clean up on Ctrl-C for start, but not via a dedicated signal handler: cmd/root.go:99 wraps the command's context in signal.NotifyContext(..., os.Interrupt), and the ctx-aware Docker/health-check waits throughout the bring-up (e.g. internal/db/start/start.go:200-212, internal/utils/docker.go:544-553) turn a SIGINT into a plain context.Canceled error value that reaches internal/start/start.go:73-81's ordinary if err != nil { rollback } branch — the same branch any other failure takes. Effect's interruption doesn't work that way: it's a separate signal outside the E channel, which is exactly why shared/functions/serve.ts already uses Effect.onInterrupt (not tapError) for its own cleanup. start.handler.ts's rollback (line ~1948) is tapError-only, so it won't fire on interruption — and worse, start is currently excluded from the global signal wrapper (shared/cli/run.ts's selfManagedSignalCommands, a holdover from when start was still a Go-proxy) and never calls ProcessControl.awaitSignal() itself, so today a raw Ctrl-C most likely kills the process outright before any finalizer — including the telemetry-flush Effect.ensuring — gets a chance to run.
I'm not comfortable landing a narrow patch for this in isolation, though, because the "obvious" fix collides with something this codebase already solved deliberately: the one-shot Postgres migrate job inside start's bring-up goes through LegacyDockerRun.runCapture, which calls processControl.holdSignals(...) specifically so a foreground SIGINT reaches its inherited-stdio child rather than being captured by the parent — the identical reasoning that keeps db reset out of the global wrapper. Wiring start into the generic wrapper's Fiber.interrupt path risks re-racing that. On top of that, Go's own exit code for an interrupted, rolled-back start is a plain 1 (cmd/root.go:235-239,310), not the 130 convention exitCodeForFailure already gives every other native command that goes through the shared wrapper — so a real fix needs a deliberate decision on exit-code handling too, not just rollback wiring.
Given this touches the shared signal wrapper, an already-tuned sibling behavior (the migrate-job's holdSignals), and an existing cross-command exit-code convention, I'd like to loop in @columferry (or whoever owns this design) on whether the right shape is (a) a start-local signal race like functions serve's, scoped so it doesn't touch the migrate-job's existing holdSignals window, or (b) opting start into the generic global wrapper with an explicit exit-code override for the interrupted case. Flagging this as a design decision rather than folding a speculative fix into this PR's diff — happy to scope and implement once there's agreement on the approach.
| networkId, | ||
| image: resolveImage(edgeRuntimeDefaultImage), | ||
| workdir: cliConfig.workdir, | ||
| dbUrl: values.dbUrl, |
There was a problem hiding this comment.
When Edge Runtime is enabled, this passes the host-facing status URL from legacyResolveLocalConfigValues (postgresql://postgres:...@${hostname}:${dbPort}/postgres, see legacy-local-config-values.ts:2738) into SUPABASE_DB_URL, but the Edge Runtime container needs the Docker-network address that Go builds from dbConfig before ServeFunctions (apps/cli-go/internal/start/start.go:66-72,1101-1104). With the default 127.0.0.1:54322 value, functions running inside the container try to connect back to themselves instead of Postgres, so local functions that use SUPABASE_DB_URL cannot reach the database; pass an internal URL using the db container name and port 5432 instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this — the failure mode you describe (Edge Runtime trying 127.0.0.1:<host-port> from inside its own container and hitting a dead loopback) would definitely be a real bug if that's what's happening, so I traced it end-to-end to be sure.
values.dbUrl at start.handler.ts:1844 is indeed the host-facing status URL (legacy-local-config-values.ts:2738), but it's only used as an input to extract the DB password — the same pattern every other service builder here uses (gotrue.service.ts:672, postgrest.service.ts:99, realtime.service.ts:123, storage.service.ts:228 all call legacyStartInternalDbPassword(input.dbUrl)). The actual SUPABASE_DB_URL sent to the Edge Runtime container is built separately in edge-runtime.service.ts:181-188 via legacyStartInternalDbUrl("postgres", legacyServiceContainerName("db", projectId), <password>), which hardcodes the Docker network container name and port 5432 — matching Go's dbConfig := pgconn.Config{Host: utils.DbId, Port: 5432, ...} (internal/start/start.go:66-72) used right before serve.ServeFunctions (start.go:1101-1104).
There's also an integration test asserting the actual env var value: edge-runtime.service.integration.test.ts:133 — SUPABASE_DB_URL=postgresql://postgres:postgres@supabase_db_proj:5432/postgres, i.e. the Docker-network form, not 127.0.0.1:54322.
So this is already Go-parity-correct as written; no change needed. Appreciate the careful eye on this though — internal vs. host-facing DB URLs is exactly the kind of thing worth double-checking in a port like this.
|
@codex review |
#5847) Go's Config.Load decodes and validates every Viper-bound field -- including realtime.ip_version, realtime.max_header_length, api.max_rows, api.enabled, api.tls.enabled, storage.vector.enabled, and the Mailpit/analytics ports -- in one atomic pass before any Docker work runs (pkg/config/config.go:749-756, called before ensureImagesCached at start.go:289). This port's pooler/Edge Runtime overrides already matched that by wrapping in Effect.try + LegacyStartInvalidConfigError, but these 10 sibling fields threw raw exceptions instead -- becoming untyped Effect defects that bypass withJsonErrorHandling's Effect.catch (breaking --output-format json/ stream-json error formatting) and wasting image pre-pull work on a config Go would have already rejected. Extracted the existing inline Effect.try pattern into a shared wrapConfigOverride helper, applied it to all 10 previously-unwrapped call sites plus the 4 existing pooler ones (removing the duplicated boilerplate), and added regression tests for the malformed-value path on 4 representative fields.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99f0bedc22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| yield* legacyWaitForHealthyServices(spawner, [postgresContainerId], { | ||
| timeoutSeconds: dbHealthTimeoutSeconds, | ||
| }); |
There was a problem hiding this comment.
When the Postgres container never becomes healthy, this legacyWaitForHealthyServices failure happens inside bringUp, so the surrounding tapError rolls the stack back and the later --ignore-health-check downgrade path is never reached. Go's start.Run catches any start.IsUnhealthyError from run() under ignoreHealthCheck (including the DB WaitForHealthyService error from internal/db/start/start.go) and exits successfully without rollback, so supabase start --ignore-health-check still fails on the exact DB health-check case it is supposed to bypass.
Useful? React with 👍 / 👎.
| toDockerPath(input.projectRoot), | ||
| "--ulimit", | ||
| "nofile=65536:65536", | ||
| "--label", |
There was a problem hiding this comment.
Edge Runtime now stages JWT/JWKS/env artifacts under <projectRoot>/supabase/.temp/start-secrets/<containerId>, but this docker run only sets the project labels and omits the com.supabase.cli.workdir label that legacyCleanupStartSecrets relies on. If the container is later removed by supabase stop --all or --project-id from a different cwd, cleanup falls back to the caller's workdir and leaves the original project's secret files orphaned on disk; the other start containers avoid this by stamping the workdir label in container-lifecycle.ts.
Useful? React with 👍 / 👎.
| projectEnvValues?: Readonly<Record<string, string>>, | ||
| ): (image: string) => Effect.Effect<string, LegacyDockerRunError> { | ||
| const hasLocalImage = (image: string): Effect.Effect<boolean> => | ||
| containerCliExitCode(spawner, ["image", "inspect", image]).pipe( |
There was a problem hiding this comment.
On hosts where a candidate image is already cached, docker image inspect emits the full image JSON to stdout; this exit-code-only call leaves stdio at the default pipes while nothing consumes them. Large inspect payloads can fill the pipe and block the child, hanging supabase start in the pre-pull phase before any container is created. The other exit-code-only Docker calls in this change pass stdout/stderr: "ignore" for this reason, so this inspect call should do the same.
Useful? React with 👍 / 👎.
| containerCliExitCode(spawner, ["stop", id], { | ||
| stdin: "ignore", | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| }).pipe(Effect.result), |
There was a problem hiding this comment.
Because the preceding container list uses --all, it includes Supabase containers that have already exited; Docker documents container stop as stopping running containers, and the Go implementation explicitly ignores errdefs.IsNotModified from ContainerStop in this case. Here any non-zero docker stop result becomes LegacyDockerRemoveAllStopError, so supabase stop or a failed-start rollback aborts before prune whenever one matching service is already stopped instead of cleaning up the remaining containers.
Useful? React with 👍 / 👎.
2 participants
What changed
Ports
supabase startfrom a Go-binary proxy to native TypeScript in the legacy CLI shell (CLI-1323), continuing the local-dev-stack shell migration alongside the already-portedstop/status/db push/db reset/db start.Talks directly to Docker/Podman via subprocess, mirroring Go's sequential per-container
DockerStart— no Docker Compose, and no@supabase/stack/effectorchestration (that runtime targets a deliberately different local-dev product and would silently manage the wrong set of containers). Brings up all 14 containers in Go's real start order, with per-service config-boolean +--excludegating, Go-byte-exact env/image resolution, a bulk health-check phase (Docker healthcheck for most services, an HTTP-HEAD-through-Kong bypass for PostgREST/Edge Runtime), and full rollback on any bring-up failure.Also natively implements the fresh-volume DB schema/migration/seed pipeline, Edge Runtime container bring-up, and fresh-volume storage bucket seeding — previously tracked as out-of-scope follow-ups for this port, now closed. Only the linked-project version-check suggestion (a best-effort "update available" hint with zero Management API dependency otherwise) remains out of scope by design.
Why
Continues the legacy-shell migration from Go-binary-proxy commands to native TypeScript, closing CLI-1323.
Reviewer-relevant context
legacyParseGoDuration(theconfig.tomlduration-string parser feeding Go-parity env vars likeGOTRUE_SESSIONS_TIMEBOX) silently accepted a malformed duration with no digits (a bare unit like"s", or a lone"."with no digits on either side) and returned0instead of erroring like Go's realtime.ParseDuration— both cases now throw Go's exacttime: invalid duration "..."message.start.services.ts's descriptiveenabledGatemetadata for Mailpit referenced the deprecatedinbucketconfig section instead of itslocal_smtprename. Fixed, and added a mechanical cross-check test that evaluates every service'senabledGatestring againststart.gates.ts's real computed gate across a battery of synthetic configs, so a future drift between the two fails loudly instead of silently.[api.tls] enabled = truelocal stacks (self-signed Kong cert) — the local-Kong-CA-trust mechanism already used byseed buckets/storage/db resetis now wired intostart's health-check HTTP client too.Note
This branch is currently 9 commits behind
develop(unrelateddeps/dockerbump commits) — opening as draft to get CI/review visibility; happy to rebase before this comes out of draft.