fix(cli): detect dead db connections (CLI-2207) - #6277
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@96318dfb5c0e0af870710d268a3eac0d4c790423Preview package for commit |
…ation-apply-sql-pipeline-stops
avallete
left a comment
There was a problem hiding this comment.
The core fix is sound — I traced the load-bearing pieces and they hold: the widened discard predicate strictly covers the old poisoned check, the only execBatch caller guards LegacyDbConnectError before touching statementIndex, keepalive reaches both the pool and the raw client, and pg honors the submit() error return so the writable check settles instead of hanging. I also chased two scary scenarios that turned out impossible: there's no residual hang on exec/query (pg's 'close' → _errorAllQueries fails them within a tick), and no applied-but-unrecorded migration (the history INSERT rides inside the batch).
I reproduced the findings below against this branch where marked; one is a process crash I'd fix before merge. The script used for the reproduced non-crash findings:
verify-findings.ts (run with bun from the repo root)
// Verification of the review findings against this branch's real modules.
// Save at the repo root and run: bun verify-findings.ts
import {
LegacyPgBatchQuery,
legacyBatchFailureError,
legacyToExecError,
} from "./apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts";
import { legacyFormatExecBatchError } from "./apps/cli/src/legacy/shared/legacy-migration-apply.ts";
import type * as Pg from "pg";
console.log("=== standalone exec renders dead connection as 'At statement: N' ===");
const deadConnErr = new Error("Client has encountered a connection error and is not queryable");
const execMapped = legacyToExecError(deadConnErr);
const rendered = legacyFormatExecBatchError(execMapped, 3, "CREATE INDEX CONCURRENTLY idx ON t (c)");
console.log(rendered.message);
console.log("\n=== poisoned batch blames statement 0 ===");
let bindCalls = 0;
const noop = () => {};
const conn = {
stream: { writable: true, cork: noop, uncork: noop },
parse: noop,
bind: () => {
bindCalls += 1;
if (bindCalls === 7) throw new Error("frame serialization blew up on statement 7");
},
describe: noop,
execute: noop,
sync: noop,
} as unknown as Pg.Connection;
const statements = Array.from({ length: 10 }, (_, i) => ({ sql: `SELECT ${i} /* stmt ${i} */` }));
const batch = new LegacyPgBatchQuery(statements, noop);
const submitErr = batch.submit(conn);
console.log("poisoned:", batch.poisoned, "submitted:", batch.submitted, "completed:", batch.completed);
const failure = legacyBatchFailureError(submitErr!, batch);
console.log("mapped:", failure._tag, "statementIndex:", (failure as { statementIndex?: number }).statementIndex);
console.log("\n=== batch-lost ConnectError has no suggestion ===");
const unsent = new LegacyPgBatchQuery([{ sql: "SELECT 1" }], noop);
const unsentErr = unsent.submit({ stream: { writable: false } } as unknown as Pg.Connection);
const connectFailure = legacyBatchFailureError(unsentErr!, unsent);
console.log(connectFailure._tag, "| suggestion:", (connectFailure as { suggestion?: string }).suggestion);
console.log("\n=== message-equality sentinel stutters once wrapped ===");
const wrapped = new Error(`batch submit failed: ${unsentErr!.message}`);
console.log(legacyBatchFailureError(wrapped, unsent).message);| | undefined, | ||
| ): LegacyDbExecError | LegacyDbConnectError { | ||
| if (batch === undefined || (!batch.submitted && !batch.poisoned)) { | ||
| return new LegacyDbConnectError({ |
There was a problem hiding this comment.
Reproduced: this error carries no suggestion, unlike its checkout-path sibling.
Constructing this error on this branch gives suggestion: undefined (and retryable: undefined), while the identical dead-DB failure one tick earlier — pool checkout, line 1046 — goes through legacyToConnectError and carries the profile-aware connect suggestion. Renderers read cause.suggestion conditionally, so nothing crashes, but the user gets an actionable hint or not depending on which side of checkout the connection died.
Related, also reproduced: the error.message === LEGACY_BATCH_CONNECTION_LOST identity check on line 231 stutters as soon as anything wraps the synthetic error (a wrapping idiom this file already uses elsewhere):
connection to the database was lost before the batch could be sent: batch submit failed: connection to the database was lost before the batch could be sent
A tiny dedicated Error subclass thrown from submit() + instanceof still satisfies pg's submit(): Error | null contract.
There was a problem hiding this comment.
suggestion part is fixed, it picks up LEGACY_SUGGEST_LOCAL_STACK now so it lines up with the checkout one
went a slightly different route on the identity check though. submit()'s guard returns its own message now and the mapper always prefixes, so theres no sentinel left to stutter on, wrapped or not.
a subclass felt like the heavier option since it'd trip the error-actionability guard for a marker that never really leaves the module. if you'd still rather have the typed instanceof:
maybe we can take this up as a separate followup and keep this one scoped in..
left retryable unset for now too, only db-setup's connect retry reads it and that wraps connect rather than execBatch.
…pabase-db-resetstart-migration-apply-sql-pipeline-stops # Conflicts: # apps/cli/docs/go-cli-divergences.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7397db3f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
TL;DR
fixes
supabase db resetandsupabase starthanging forever with no error when the database connection dies while migrations are being appliedwhats broken?
node-postgres silently discards every protocol frame once a socket stops being writable, while still reporting the write as successful, so the whole batch goes nowhere and the CLI waits forever on a reply the server was never asked for
it also leaves TCP keepalive off by default, where the Go CLI's driver had it on, so a peer that dies without a FIN or RST is never noticed either
fixed now by:
a server that stays alive but never answers still waits, matching the Go CLI, since that is indistinguishable from a long running statement
ref: