Guard nextWrite against a closed socket - fixes #1208 - #1209
Open
XavierGeerinck wants to merge 1 commit into
Open
Guard nextWrite against a closed socket - fixes #1208#1209XavierGeerinck wants to merge 1 commit into
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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