Skip to content

Guard nextWrite against a closed socket - fixes #1208 - #1209

Open
XavierGeerinck wants to merge 1 commit into
porsager:masterfrom
XavierGeerinck:fix/next-write-null-socket
Open

XavierGeerinck wants to merge 1 commit into
porsager:masterfrom
XavierGeerinck:fix/next-write-null-socket

Conversation

@XavierGeerinck

Copy link
Copy Markdown

Fixes #1208.

The defect

closed() sets socket = null and only reconnects on a later timer, so a connection can sit with no socket across ticks. nextWrite() is the one consumer of socket in connection.js without a guard — terminate() has if (socket), 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. 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. Returning false alone is not enough — the write vanishes and the caller awaits forever (verified: the query hangs). error(...) rejects query/initial and drains sent via queryError, so the caller gets a catchable CONNECTION_CLOSED and the process survives.

The test

pg_terminate_backend turned out to be the wrong trigger for a portable test: on Linux the backend's close arrives as an RST, so closed(hadError=true) runs, the pending query rejects with ECONNRESET, and the next one merely hangs. On macOS the same kill arrives as a clean FIN, hadError is 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 that end()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:

TypeError: Cannot read properties of null (reading 'write')
    at Immediate.nextWrite (src/connection.js:255:22)
    at process.processImmediate (node:internal/timers:574:21)

Verification

Full ESM suite in a container replicating .github/workflows/test.yml (Debian, PostgreSQL 17, pg_hba.conf from tests/, ssl on, wal_level=logical, second cluster on 5433):

result
upstream/master 264 passed, 0 failed 🎉
this branch 265 passed, 0 failed 🎉

Exactly 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 on upstream/master without 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 call end(). Happy to look at it separately if you want it filed or fixed.

🤖 Generated with Claude Code

https://claude.ai/code/session_012njhLokwkVZMjV7KcHsbA5

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
@JakeThomson

Copy link
Copy Markdown

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:

TypeError: Cannot read properties of null (reading 'write')
    at Immediate.nextWrite (node_modules/postgres/src/connection.js:255:22)
    at process.processImmediate (node:internal/timers:574:21)

Because it's thrown from a setImmediate, no application-level handling can catch it, so the only fix is in the driver.

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 CONNECTION_CLOSED.

+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.

demattosanthony pushed a commit to sixb-ai/sixb that referenced this pull request Oct 1, 2026
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.
yannickmonney added a commit to tale-project/tale that referenced this pull request Oct 2, 2026
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nextWrite() throws an uncaughtException when a reserved connection's backend dies

2 participants