fix(cli): exec start wrappers and bump postgres-meta so docker stop is not 10s (CLI-2192) - #6203
fix(cli): exec start wrappers and bump postgres-meta so docker stop is not 10s (CLI-2192)#6203avallete wants to merge 5 commits into
Conversation
avallete
changed the title
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@2c0c7906c9896cc5541ee36ffbbd6e8ea5cc4bf0Preview package for commit |
2383507 to
6c58a71
Compare
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| code: LEGACY_PG_DELTA_NEXT_SKIPPED_STATEMENT_CODE, | ||
| severity: "warning", | ||
| subject: entry.file, | ||
| message: `pg-delta could not load a declarative schema statement from ${entry.file}: ${entry.stmt}`, |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
planSchemaFiles' skipped statement is copied verbatim into the diagnostic message, so declarative CREATE ROLE ... PASSWORD 'secret' SQL reaches debug logging with PGDELTA_DEBUG and terminal output with --strict-coverage. This exposes credential-bearing project SQL in CI or shell logs, despite raw statements being intended as detail-only.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Remove ${entry.stmt} from the message template string. The raw SQL statement is already captured safely in context.statement (intended as detail-only), which aligns with the JSDoc comment that explicitly states 'The raw statement stays in context/detail rather than the aggregate warning, since a skipped CREATE ROLE can carry a password.' The message field should only reference the file name to avoid exposing credential-bearing SQL (e.g., CREATE ROLE ... PASSWORD 'secret') in debug logs and terminal output.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| message: `pg-delta could not load a declarative schema statement from ${entry.file}: ${entry.stmt}`, | |
| message: `pg-delta could not load a declarative schema statement from ${entry.file}`, |
![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: 6c58a710cf
ℹ️ 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".
| code: LEGACY_PG_DELTA_NEXT_SKIPPED_STATEMENT_CODE, | ||
| severity: "warning", | ||
| subject: entry.file, | ||
| message: `pg-delta could not load a declarative schema statement from ${entry.file}: ${entry.stmt}`, |
There was a problem hiding this comment.
When a skipped declarative statement contains a secret, such as CREATE ROLE ... PASSWORD ..., this embeds the complete SQL in the diagnostic message. legacyReportPgDeltaNextDiagnostics emits that message to stderr whenever --strict-coverage or debug diagnostics are enabled, despite the adjacent comment saying raw SQL should remain only in diagnostic context, so credentials can be exposed in terminals and captured logs. Keep the message limited to the filename and store the statement only in the non-rendered context.
Useful? React with 👍 / 👎.
| const targetPath = path.join(declarativeDir, file.name); | ||
| yield* fs.makeDirectory(path.dirname(targetPath), { recursive: true }); | ||
| yield* fs.writeFileString(targetPath, file.sql); |
There was a problem hiding this comment.
When the declarative directory comes from an untrusted checkout and an exported filename already exists as a symlink, this write follows the link and overwrites its target outside the declarative tree. readManagedDeclarativeSqlFiles deliberately skips symlinks, but that makes the destination appear newly created and does not remove or reject the link before writeFileString; nested symlinked directories have the same problem. Verify every destination component without following links or replace symlinks safely before writing.
Useful? React with 👍 / 👎.
| const result = yield* pgDelta.diffExplicit({ | ||
| context: explicitCtx, | ||
| toml: cfg, | ||
| source, | ||
| desired, |
There was a problem hiding this comment.
For db diff --from migrations --to linked, source is captured before the linked target is resolved, but it no longer captures the current cfg; resolving desired then replaces cfg with the linked project's remote-merged config, and this final value is passed to both endpoints. Consequently the migrations shadow/catalog can be built with remote overrides that occur later in argument resolution, unlike the previous implementation that provisioned it immediately from the base config, producing a diff against the wrong PostgreSQL image or platform baseline. Store the TOML/context with each migrations endpoint when it is resolved rather than passing one final cfg.
AGENTS.md reference: apps/cli/AGENTS.md:L53-L56
Useful? React with 👍 / 👎.
| output: Flag.string("output").pipe( | ||
| Flag.withAlias("o"), | ||
| Flag.withDescription( | ||
| "Write the generated declarative schema to this directory without changing the configured declarative schema path.", | ||
| ), | ||
| Flag.optional, |
There was a problem hiding this comment.
This adds the TS-only db schema declarative generate --output flag, but the accompanying docs/go-cli-divergences.md update records only --strict-coverage. Add --output to the flag-divergence section so the documented parity surface remains complete as required for every new legacy-shell flag with no Go equivalent.
AGENTS.md reference: apps/cli/AGENTS.md:L540-L544
Useful? React with 👍 / 👎.
| * like an unmodeled object kind. Silently dropping these would let a statement the | ||
| * user wrote in a declarative file vanish from the plan with no signal at all. | ||
| */ | ||
| export const LEGACY_PG_DELTA_NEXT_SKIPPED_STATEMENT_CODE = "skipped_statement"; |
There was a problem hiding this comment.
The newly exported LEGACY_PG_DELTA_NEXT_SKIPPED_STATEMENT_CODE does not use the mandatory Legacy or legacy prefix convention; LEGACY_ENABLE_LOCAL_WEBHOOKS_SUGGESTION introduced in the same commit has the same issue. Rename both exports and their imports so legacy-only symbols do not pollute completion and the namespace remains unambiguous.
AGENTS.md reference: apps/cli/AGENTS.md:L235-L249
Useful? React with 👍 / 👎.
| const migrationEntries = yield* fs.readDirectory(migrationsDir).pipe( | ||
| Effect.catchTag("PlatformError", (error) => | ||
| error.reason._tag === "NotFound" | ||
| ? Effect.succeed([] as ReadonlyArray<string>) |
There was a problem hiding this comment.
The [] as ReadonlyArray<string> assertion papers over the differing success types in this Effect branch, contrary to the workspace requirement to resolve Effect typing without as casts. Give Effect.succeed an explicit result type or restructure the branch so inference produces the intended readonly array without an assertion.
AGENTS.md reference: apps/cli/AGENTS.md:L99-L99
Useful? React with 👍 / 👎.
6c58a71 to
4d6fac4
Compare
💡 Codex Review Restrict pg-delta debug artifacts to the current user
When cli/apps/cli/src/legacy/shared/db-bootstrap/health-check.ts Lines 585 to 588 in 4d6fac4 Enforce the configured readiness deadline
When a Postgres connection attempt consumes its new 2-second connect timeout—for example, while Docker Desktop routing is black-holed—this schedule still performs cli/apps/cli/src/legacy/shared/db-bootstrap/shadow-cache.ts Lines 399 to 405 in 4d6fac4 Key cached shadows by the effective webhooks baseline
When AGENTS.md reference: apps/cli/AGENTS.md:L387-L394 cli/apps/cli/src/legacy/commands/db/schema/declarative/declarative.flow.ts Lines 167 to 170 in 4d6fac4 Match dollar-quote closing tags before masking SQL
When a declarative function uses a tagged outer body and a differently tagged inner string, such as ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4d6fac4 to
7e0315c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e0315c7be
ℹ️ 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".
| "./logflare eval Logflare.Release.migrate || exit $?\n" + | ||
| "./logflare start --sname logflare &\n" + | ||
| "BEAM_PID=$!\n" + | ||
| 'trap \'kill -TERM "$BEAM_PID" 2>/dev/null; n=0; while [ "$n" -lt 3 ] && kill -0 "$BEAM_PID" 2>/dev/null; do n=$((n+1)); sleep 1; done; kill -KILL "$BEAM_PID" 2>/dev/null\' TERM\n' + |
There was a problem hiding this comment.
When analytics is enabled, this supervisor's correctness depends on real PID-1 signal delivery and the Logflare image's shutdown behavior, but the added unit test only compares the generated shell string. I checked the existing start.live.test.ts and stop.live.test.ts: neither starts analytics/Logflare and measures its shutdown, so CI cannot detect this trap failing and restoring Docker's 10-second timeout. Add or extend a real-Docker live scenario that enables analytics and verifies the bounded stop behavior.
AGENTS.md reference: apps/cli/AGENTS.md:L520-L523
Useful? React with 👍 / 👎.
…s not 10s Full-stack `supabase stop --no-backup` is gated by a 10s Docker SIGTERM grace period: five long-running containers keep `sh` (or a hung Node shutdown) as PID 1, so SIGTERM never reaches the real process and every `docker stop` burns the full timeout before SIGKILL. Stops run in parallel, so wall time = slowest container. Workstream A — `exec` the real process in every CLI-authored wrapper, the same deliberate divergence from Go's scripts as the Postgres entrypoint (timing is outside the Go-parity surface, ADR 0016): - Kong: `exec ./docker-entrypoint.sh kong docker-start …` - Vector: `exec vector --config …` (the Logflare wget wait-loop still runs in the wrapper shell before the exec) - Logflare: `exec sh run.sh` outer + `exec ./logflare start` inside run.sh (the migrate && start sequencing is unchanged — #6088) - Edge runtime: `exec edge-runtime start …` in the long-lived serve/ start container, and in the ephemeral script-runner for cancellation latency Workstream B — postgres-meta v0.98.0 (released today, contains postgres-meta#1103, which fixes the circular onClose hang that made v0.97.0 ride the close-with-grace 10s timeout; the canary measured ~230-320ms). Dockerfile pin bumped + versions synced. Command paths, stdout/stderr text, exit codes, telemetry, and documented side effects are unchanged; no `-t`, no `docker rm -f`, no grace-period env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Measured on a real stack: with beam.smp as PID 1 (both execs in place), docker stop -t 10 on the logflare container still burns the full grace period (~10.5s) — Logflare's own SIGTERM shutdown hangs upstream, the same class of problem postgres-meta#1103 fixed for pg-meta. The execs stay (necessary, one less layer, load-bearing once upstream fixes its handler); the comment no longer claims they alone fix stop time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stop finishes in seconds Two additions to the fast-stop work: - The three postgres entrypoint variants exec docker-entrypoint.sh, so Postgres is PID 1 and docker stop's SIGTERM triggers its own fast shutdown (~4s) instead of burning the 10s grace period. Same change #6184 carries for the shadow's benefit; included here so stop is fast without waiting on that PR. - Logflare gets a trap-and-escalate supervisor instead of a bare exec: measured with beam.smp as PID 1, docker stop STILL burned the full grace period (Logflare's SIGTERM shutdown hangs upstream, the class of problem postgres-meta#1103 fixed for pg-meta). run.sh's shell now stays PID 1 deliberately, forwards SIGTERM to the BEAM, waits up to 3s for a graceful exit, then SIGKILLs — the same end state as today's post-timeout kill, ~7s sooner, and the graceful path wins first if upstream ever fixes its handler. Exit codes are preserved via the interrupted-wait idiom (validated: hung child stops in ~3.4s exit 137; clean child exit code propagates). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7e0315c to
787493f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 787493f3bd
ℹ️ 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".
| // without it every stop burns the full 10s grace period. The wget wait-loop above still runs | ||
| // in the wrapper shell BEFORE the exec, unchanged. Stop timing is outside the Go-parity | ||
| // surface (ADR 0016). | ||
| ":4000/health 2>/dev/null; do sleep 2; done\nexec vector --config /etc/vector/vector.yaml\n" |
There was a problem hiding this comment.
When Vector restarts while Logflare is unhealthy, this loop can run indefinitely and execution never reaches the new exec. The shell therefore remains PID 1 and does not forward Docker's stop signal to the active wget/sleep; Docker then waits for its grace timeout before killing the container, so the latency regression this change addresses remains in this failure state. Install a TERM handler around the polling phase (then reset it before exec) and exercise the unhealthy-Logflare path in the required real-Docker live test. Docker documents that stop signals the container's main process and force-kills it after the timeout.
AGENTS.md reference: apps/cli/AGENTS.md:L520-L523
Useful? React with 👍 / 👎.
| const LEGACY_LOGFLARE_ENTRYPOINT_SCRIPT = | ||
| "cat <<'EOF' > run.sh && sh run.sh\n./logflare eval Logflare.Release.migrate &&\n./logflare start --sname logflare\nEOF\n"; | ||
| "cat <<'EOF' > run.sh && exec sh run.sh\n" + | ||
| "./logflare eval Logflare.Release.migrate || exit $?\n" + |
There was a problem hiding this comment.
When supabase stop or startup rollback runs while Logflare.Release.migrate is still executing, run.sh is already PID 1 but the TERM trap has not yet been installed. Docker signals only that main shell process; it does not signal the foreground migration child, and PID 1's unhandled terminating signal is ignored, so the stop still reaches Docker's grace timeout and ends in SIGKILL. Install the handler before starting the migration (and make it terminate the migration phase as well) so shutdown is bounded throughout container startup, not only after the BEAM has launched.
Useful? React with 👍 / 👎.
| "cat <<'EOF' > /home/kong/custom_nginx.template && \\\n" + | ||
| "./docker-entrypoint.sh kong docker-start --nginx-conf /home/kong/custom_nginx.template\n"; | ||
| "exec ./docker-entrypoint.sh kong docker-start --nginx-conf /home/kong/custom_nginx.template\n"; |
There was a problem hiding this comment.
This intentionally changes legacy Docker behavior relative to the Go reference for Kong, Vector, Logflare, Edge Runtime, and functions serve, but docs/go-cli-divergences.md contains no entry for the new PID-1/signal-handling behavior. Add one behavioral-divergence entry covering these wrappers so support and future parity work do not mistake the changed shutdown semantics for accidental drift.
AGENTS.md reference: apps/cli/AGENTS.md:L540-L544
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d61ad81e9
ℹ️ 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".
| name: "realtime", | ||
| configKey: "realtime", | ||
| defaultVersion: "2.123.1", | ||
| defaultVersion: "2.124.4", |
There was a problem hiding this comment.
When a consumer relies on @supabase/stack defaults, this partial update still selects Realtime 2.124.4 and Storage 1.69.0 even though apps/cli-go/pkg/config/templates/Dockerfile:14-15 pins 2.128.0 and 1.69.9. The package's sync:versions --check logic derives every catalog entry from that manifest, so the catalog remains out of sync and programmatic stacks run a different service set than supabase start; regenerate both changed entries from the current Dockerfile.
Useful? React with 👍 / 👎.
1 participant
Full-stack
supabase stop --no-backupwas gated by Docker's 10s SIGTERM grace period: six long-running containers either keptshas PID 1 (signal never forwarded) or hung in their own shutdown handler, and stops run in parallel, so wall time = slowest container + prune.What changed
execin every CLI-authored wrapper — the real process replacesshas PID 1 (deliberate divergence from Go's scripts; stop timing is outside the Go-parity surface, ADR 0016). Command paths, output, exit codes, telemetry, and documented side effects unchanged; no-t, nodocker rm -f, no grace-period env:stopis fast without waiting on that PR)Logflare: trap-and-escalate supervisor — measured with
beam.smpas PID 1 (a plainexecchain),docker stopstill burned the full grace period: Logflare's own SIGTERM shutdown hangs upstream, the same class of problem postgres-meta#1103 fixed for pg-meta. Sorun.sh's shell stays PID 1 deliberately with a real handler: forward SIGTERM to the BEAM, give its graceful shutdown 3s, then SIGKILL. Today's post-timeout outcome is already SIGKILL, so this is the identical end state ~7s sooner — and the graceful window wins first if upstream fixes its handler. Exit codes are preserved via the interrupted-waitidiom (validated in isolation: hung child stops in ~3.4s with exit 137; a clean child's exit code propagates untouched).migrate && startsequencing unchanged (#6088).postgres-meta v0.98.0 — released 2026-08-14, tag verified identical to the postgres-meta#1103 merge commit (fixes the circular
onClosehang that made v0.97.0 rideclose-with-grace's 10s timeout). Dockerfile pin bumped + versions synced.Measured (real full stack, analytics enabled, Docker 29/OrbStack, CLI run from source)
Per-container
docker stop -t 10, develop → this branch:node(v0.97.0 hang) →node(v0.98.0)sh -c→nginxsh -c→vectorsh -c→edge-runtimesh -c→ supervisedsh run.shsh -c→postgresFull
stop --no-backupwall: 15.8s → 6.58s (analytics on; a default stack with analytics off is gated by Postgres alone). Remaining upstream nicety: a Logflare image whose SIGTERM handler actually completes would let the supervisor's graceful window win and shave the last ~3s — tracked in CLI-2192.🤖 Generated with Claude Code