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

Guard nextWrite against a closed socket - fixes #1208#1209
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
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

1 participant