Repository navigation
Guard nextWrite against a closed socket - fixes #1208 - #1209
XavierGeerinck wants to merge 1 commit into
Conversation
closed() nulls the socket and only reconnects on a later timer, so a connection can sit with no socket across ticks. nextWrite() was the one consumer of socket in this file without a guard - terminate() has `if (socket)` and end() has `socket && ...` - and it is also the one reached from the 'data' handler, where a throw has no query to reject and escapes as an uncaughtException that takes the process down. A pooled connection hides this because the pool rotates the dead connection away. A reserved one is pinned and cannot be rotated, so every subsequent write on it is fatal. Settle the pending queries rather than just dropping the write: returning false alone leaves the caller awaiting a write that never happens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012njhLokwkVZMjV7KcHsbA5
Runbook entry in docs/deployment.md for the 2026-09-15 outage: symptoms, recovery, and the bun patch steps for porsager/postgres#1209, tested against a local reproduction. Comments in connect.ts and catalog-assembly.ts point to it.
|
We hit #1208 in production on postgres 3.4.8 / Node 24, behind PgBouncer in transaction mode. When our Postgres restarted, the uncaught exception took down our API processes: Because it's thrown from a We're shipping this PR as-is as a local patch. A regression test modelled on yours, using a reserved connection whose socket the server ends, fails on unpatched 3.4.8 with exactly this trace and passes with the patch; the query now rejects with +1 for merging. This is the same bug as #1066 and #1154, and #1168 is an earlier PR for it, so landing one of them would close all three issues. |
When PostgreSQL or the network dropped a connection in the middle of a transaction, the whole process died with "null is not an object (evaluating 'socket.write')". postgres.js 3.4.9 answers the failed statement of sql.begin with a ROLLBACK written to the closed socket from a setImmediate, outside any promise. reserve().release() has the same flaw: it hands a dead connection back to the pool, and the next query sent on it crashes the same way. runPgTransaction now holds a reserved connection and sends BEGIN, COMMIT and ROLLBACK itself. Once the connection is lost, nothing more is sent on it and it is never released: the server has already rolled the transaction back, and the pool replaces a closed connection on its own. The transaction rejects with the connection error and the process keeps running. Migrations, which already drove their own transaction on a reserved connection, use the same helpers. isConnectionLost replaces the unused isConnectionError: porsager's closed or destroyed codes, socket resets, and any FATAL or PANIC server error, after which PostgreSQL always ends the session. A COMMIT that PostgreSQL answers with a ROLLBACK tag now fails. That happens when a statement failed and the callback caught its error, and it used to be reported as a success whose writes were gone. The fix for the same bug in postgres.js is porsager/postgres#1209. Until it ships, a connection lost between two statements still crashes, because nothing reports the loss before the next deferred write.
…#4061) postgres.js 3.4.7 crashed the backend when a PostgreSQL server process died under an open transaction or a reserved connection: the closed connection's socket is nulled, then the transaction's ROLLBACK or the reservation's next statement is written to it (#4041). A Bun patch applied to the package's ESM, CommonJS and workerd builds carries the upstream nextWrite guard (porsager/postgres#1209) and settles what a closed connection leaves pending, so the statements reject with CONNECTION_CLOSED and the pool reconnects. Every Dockerfile that installs from the root manifests copies patches/, and patches/README.md records the removal condition.
|
Another way to hit the same crash, which this PR also fixes: end the client while an async let release
const sql = postgres({ ...options, pass: () => new Promise(r => release = r) })
const query = sql`select 1`.catch(e => e.code)
// once pass() has been called:
await sql.end({ timeout: 0 })
release('password')
// -> uncaughtException: Cannot read properties of null (reading 'write')
// at nextWrite (src/connection.js)The server closes on the Terminate message and On master 411429e this crashes in 5 of 5 runs each with SCRAM and md5 (Postgres 17, Node 24). With only this PR's |
Fixes #1208.
The defect
closed()setssocket = nulland only reconnects on a later timer, so a connection can sit with no socket across ticks.nextWrite()is the one consumer ofsocketinconnection.jswithout a guard —terminate()hasif (socket),end()hassocket && …— and it is also the one reached from the'data'handler, where a throw has no query to reject and escapes as an uncaughtException that takes the process down.A pooled connection hides this, because the pool rotates the dead connection away. A reserved one is pinned and cannot be rotated, so every subsequent write on it is fatal. That is why it shows up as an unrecoverable crash loop for
reserve()users — in our case an advisory-lock helper, roughly one process exit every 8 minutes in production.The fix
Guard
nextWrite()and settle rather than drop. Returningfalsealone is not enough — the write vanishes and the caller awaits forever (verified: the query hangs).error(...)rejectsquery/initialand drainssentviaqueryError, so the caller gets a catchableCONNECTION_CLOSEDand the process survives.The test
pg_terminate_backendturned out to be the wrong trigger for a portable test: on Linux the backend's close arrives as an RST, soclosed(hadError=true)runs, the pending query rejects withECONNRESET, and the next one merely hangs. On macOS the same kill arrives as a clean FIN,hadErroris false, and you get the crash. The defect is really about the clean-close path, so the test drives that directly with a small TCP proxy thatend()s the client side — deterministic on both platforms, and it does not depend on how the OS reports a killed backend.Without the fix it crashes the test runner outright:
Verification
Full ESM suite in a container replicating
.github/workflows/test.yml(Debian, PostgreSQL 17,pg_hba.conffromtests/, ssl on,wal_level=logical, second cluster on 5433):upstream/masterExactly the one added test, no regressions.
npm run test:cjs(transpiled) also passes. The original issue repro was additionally confirmed on Node 23.10.0 and Bun 1.2.23 / 1.3.14 / 1.4.0 — same defect on all four, and all four fixed by this change.One thing I did not touch
sql.end()hangs on a pool whose reserved connection was killed. It reproduces identically onupstream/masterwithout this patch (I checked before assuming it was mine), so it looks like a separate defect and I kept it out of this PR rather than widen the diff. The test therefore does not callend(). Happy to look at it separately if you want it filed or fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_012njhLokwkVZMjV7KcHsbA5