fix(cli): detect dead db connections (CLI-2207)

[7ttp]

TL;DR

fixes supabase db reset and supabase start hanging forever with no error when the database connection dies while migrations are being applied

whats 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:

• failing a batch that never reached the wire as a connection error, carrying the driver's own reason instead of blaming the migration's first statement
• discarding that pooled connection, so the next batch redials instead of writing into the same dead socket
• turning TCP keepalive on for every connection the CLI opens, so a silently dead peer is eventually detected instead of waited on

a server that stays alive but never answers still waits, matching the Go CLI, since that is indistinguishable from a long running statement

ref:

• closes: supabase db reset/start: migration-apply SQL pipeline stops mid-stream with no server-side counterpart #6244

[github-actions]

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@0dca536941c5f6fa3d1fd925f7d63cdf7cdbf3d5

Preview package for commit 0dca536.

[avallete]

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

[avallete]

Blocking, reproduced: idle death of the raw client crashes the whole CLI — and keepalive widens the trigger.

acquireRawClient builds a bare new Pg.Client(winningRawConfig) and never attaches an 'error' listener. Any socket death while that client is idle (e.g. between inspect report's COPYs) makes pg emit an unlistened 'error' (client.js:422) and crashes the process. Reproduced on this branch under both Node and Bun with pg 8.23: idle RST → Unhandled 'error': read ECONNRESET, exit 1; idle FIN → Connection terminated unexpectedly, exit 1; same scenario with one 'error' listener attached → survives gracefully.

The mechanism predates this PR (a server crash or docker stop already triggers it), but keepalive widens it: silent peer deaths, which previously produced no event at all while idle, now surface as ETIMEDOUT on the same unlistened path (that leg not reproduced — it needs a genuinely blackholed peer — but it's the same event class, per socket semantics). One 'error' handler that invalidates the rawClient cache fixes the crash and the stale-client reuse at line 1010.

repro-raw-client-crash.mjs

// Repro: an idle socket error on the listener-less cached raw client crashes the CLI.
// Mirrors acquireRawClient (legacy-db-connection.sql-pg.layer.ts:1009-1025): a bare
// Pg.Client with no 'error' listener, with the PR's keepAlive config. The fake server
// completes the postgres startup handshake, then kills the socket while the client is
// idle — the same event a failed keepalive probe (ETIMEDOUT) or a dying server
// (FIN/RST) delivers.
//
// Save under apps/cli/ (so `pg` resolves) and run:
//   node repro-raw-client-crash.mjs rst       -> process crash (unhandled 'error')
//   node repro-raw-client-crash.mjs fin       -> process crash
//   node repro-raw-client-crash.mjs listener  -> control: survives gracefully
// (bun works too)
import net from "node:net";
import pg from "pg";

const mode = process.argv[2];
if (!["fin", "rst", "listener"].includes(mode)) {
  console.error("usage: repro-raw-client-crash.mjs <fin|rst|listener>");
  process.exit(2);
}

const sockets = [];
const server = net.createServer((socket) => {
  sockets.push(socket);
  let handshakeDone = false;
  socket.on("data", () => {
    if (handshakeDone) return;
    handshakeDone = true;
    const authOk = Buffer.from([0x52, 0, 0, 0, 8, 0, 0, 0, 0]); // AuthenticationOk
    const ready = Buffer.from([0x5a, 0, 0, 0, 5, 0x49]); // ReadyForQuery 'I'
    socket.write(Buffer.concat([authOk, ready]));
  });
});

await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve));
const port = server.address().port;

const client = new pg.Client({
  host: "127.0.0.1",
  port,
  user: "u",
  database: "d",
  keepAlive: true, // the PR's addition; not itself the trigger here
  connectionTimeoutMillis: 5000,
});
if (mode === "listener") {
  client.on("error", (err) => {
    console.log(`GRACEFUL: error listener caught: ${err.message}`);
  });
}

await client.connect();
console.log(`connected; client now idle (no active query), mode=${mode}`);

setTimeout(() => {
  for (const s of sockets) (mode === "fin" ? s.end() : s.resetAndDestroy());
}, 100);

setTimeout(() => {
  console.log("SURVIVED: process still alive 1.5s after idle socket death");
  process.exit(0);
}, 1500);

[avallete]

Mechanism reproduced: the standalone pipeline-incompatible path still blames the SQL for a dead connection.

The reclassification only covers execBatch. It's structural — session.exec is typed Effect<void, LegacyDbExecError>, so the connect carve-out can't reach it. Feeding the real dead-client driver error through the real mapping on this branch renders:

Error: Client has encountered a connection error and is not queryable
At statement: 3
CREATE INDEX CONCURRENTLY idx ON t (c)

— exactly the "blame a statement that provably never ran" output the divergence-doc entry says was removed. (I verified the mapping+formatter chain, not a full db reset against a dying server.) Either extend the mapping to exec or scope the doc entry to say the standalone path intentionally keeps Go's rendering.

[avallete]

Source-verified fragility: recovery here depends on a private pg-pool internal.

After a submitted-then-died batch (discard === false → release(undefined)), the dead client is evicted only because pg-pool's _release removes clients where !client._queryable (pg-pool@3.14.0 index.js:392, marked TODO(bmc): expose a proper, public interface) — and pg-pool is an unpinned transitive dep. The no-discard choice is right (a syntax error shouldn't force a redial + SET SESSION ROLE replay on a max:1 pool), but the "then recovers" integration test locks in this dependency without naming it. Worth a comment here stating the pg-pool behavior this branch relies on, so a future dep bump that breaks it is traceable.

[avallete]

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.

[avallete]

Not reproduced (arithmetic only): the "~11 minutes on Linux" figure looks wrong.

Documented kernel defaults give 300s idle + 75s interval × 9 probes ≈ 975s ≈ 16 minutes. I haven't measured it live (that needs a blackholed peer and the full wait), and the true figure is runtime-dependent — newer Node/libuv versions set TCP_KEEPINTVL/TCP_KEEPCNT themselves, and this runs under Bun. Suggest dropping the concrete minutes claim and keeping the 5-minute-idle rationale; the "sooner than Go's window" comparison still holds either way.

[avallete]

Reproduced (low priority): a poisoned batch always blames statement 0.

For a poisoned batch, completed is 0 (corked stream, server acked nothing), so the failure always lands on statement 0. Driving the real class on this branch with a bind that throws on statement 7 of 10 renders:

Error: frame serialization blew up on statement 7
At statement: 0
SELECT 0 /* stmt 0 */

Pre-existing mapping, but this PR re-states it in exported code, and the new test "keeps a partially written batch on the statement path" uses completed: 1 and never asserts statementIndex. What triggers a mid-serialization throw in production is admittedly speculative (oversized frames is my best guess), so: at minimum the test could pin the realistic completed: 0 case.

[avallete]

Design note (take or leave): submitted and poisoned are mutually exclusive by construction, but that's an unenforced convention two consumers project differently (!submitted && !poisoned at 228 vs submitted === false at 256). A single outcome: "unsent" | "poisoned" | "submitted" set once in submit() would make the exclusivity structural — ~4 sites.

[avallete]

Nit: "Guards the driver path that already worked" is temporal narration — the durable invariant is just "a socket drop after the batch was written must fail the batch and not recycle the client".

Coverage: this test exercises the post-write drop, but the exact path the PR fixes (unwritable-yet-_queryable socket → LegacyDbConnectError → discard → next batch redials) is only unit-tested in isolation. One integration case that makes the checked-out socket non-writable without a close would catch a wiring regression between submit/legacyBatchFailureError/legacyShouldDiscardBatchClient that would otherwise re-introduce the hang unseen.