Repository navigation
fix(drivers): a socket dying with a full write buffer no longer hangs the send (#131) - #132
Merged
Merged
Conversation
… the send Closes #131. `sendInternal` waited for backpressure to clear with a promise that had one way to settle: the `drain` event. If the peer vanished while the buffer was still full, `drain` never fired and nothing else reached that promise — `failAll` only rejects waiters queued for a reply, and `commandTimeoutMs` belongs to those same waiters. `sendMessage()` never returned, the pooled connection was never released, and `withRetry` could not help because nothing had failed. Reproduced with a 4 MB message to a server that stops reading and then closes: the promise stayed pending indefinitely. It now races `drain` against `error` and `close`, removing all three listeners whichever wins, so a pooled connection does not accumulate one per write. Same shape as #117, and the second half of it: a promise with a single settlement path and a peer that can disappear before it is taken. While in there: `tls.connect({ socket })` leaves the pre-upgrade listeners on the socket underneath — measured, the `data` handler is still attached and the socket still flowing after wrapping. Anything delivered from there would be TLS records going into the reply parser as noise, and the old close handler would report the failure a second time. They are detached on upgrade now. Whether those bytes actually arrive depends on Node internals I could not exercise without a TLS-capable fake server, so this is written as hardening rather than as a fix for an observed failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb
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.
Closes #131. The last of the leads left open by the driver audit — and it turned out to be the worst defect in the set.
The hang
sendInternal()waited for backpressure to clear with a promise that had exactly one way to settle:If the peer vanished while the write buffer was still full,
drainnever fired and nothing else could reach that promise.failAll()only rejects waiters queued for a reply, andcommandTimeoutMsbelongs to those same waiters — neither covers this one.Reproduced against a server that completes the handshake, stops reading once
DATAbegins so the client backs up, then destroys the socket:Four seconds is only where the probe gave up. The promise stays pending indefinitely.
Why it is the worst kind. A hang is not a failure, so nothing downstream can react to it:
withRetrynever gets control, and the pooled connection is never released.maxConnectionsslots leak one at a time until everyacquire()queues behind a connection that will never come back, and the driver stops entirely. Getting there needs a message past the socket's 16 KB write buffer — any real attachment — and a connection that dies mid-transfer, which is an ordinary flaky-network event.It is the same shape as #117 and the other half of it: a promise with a single settlement path and a peer that can disappear before it is taken. Both are now closed.
The fix races
drainagainsterrorandclose, removing all three listeners whichever wins, so a long-lived pooled connection does not accumulate a listener per write.STARTTLS listeners, as hardening
tls.connect({ socket })leaves the pre-upgrade listeners on the socket underneath. Measured directly:Anything the raw socket delivers after the upgrade is TLS records, which would go into the reply parser as noise, and the stale close handler would report the failure a second time. They are detached on upgrade now.
I could not settle whether those bytes actually reach the listener — that needs a TLS-capable fake server, and Node cannot mint a self-signed certificate without a dependency. So this is written up as hardening rather than as a fix for an observed failure. Removing listeners that should not be there cannot be wrong; claiming a bug I did not reproduce would be.
Verification
294 tests, including a regression test that drives the 4 MB stalling-server scenario end to end. Lint, format, typecheck, JSR dry run, build, bundle budgets and coverage floors all clean.
connection.tsmoved from 60% to 67% statements.Still open
The remaining uncovered region of
connection.tsis the STARTTLS upgrade path itself. Extending the fake server to speak TLS would close both that coverage gap and the question above — worth doing, and larger than this change.🤖 Generated with Claude Code
https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb