Repository navigation
fix: connection lifecycle fixes for socket death and close - #9
Dodothereal wants to merge 4 commits into
Conversation
- In fail(): destroy socket with error, set sock=null, shookHands=false, ready=null, and clear buffers. - In close(): clear buffers after closing socket. - In connect(): only call fail on 'close' event if socket is still open to avoid double-fail. Fixes alexanderolvera#6 Assisted-by: Claude Code
alexanderolvera
left a comment
alexanderolvera
left a comment
There was a problem hiding this comment.
Thanks for this — and for picking up a first issue here. The core of it is right: resetting shookHands/ready and destroying the socket in fail() is exactly what issue #6 asked for, and I confirmed it fixes the headline bug. Against main, an instance whose socket dies still reports connected === true; with your branch it correctly reports false and the instance can reconnect.
I built an offline lifecycle harness to check the branch and hit two regressions I'd like resolved before merging. Both are reproducible, not theoretical.
1. close() now strands in-flight calls forever
Rejecting pending calls on close used to happen indirectly: close() set this.sock = null, the socket emitted 'close', and the handler called fail(), which rejected them. Now that the handler skips sockets already cleared by close(), nothing rejects them — so anything awaiting a reply hangs forever instead of getting an error.
### against PR #9 as filed ###
FAIL close() rejects the in-flight call (HUNG)
FAIL close() rejects the queued call (HUNG)
2. The socket guard checks a socket, not this socket
if (this.sock) is true whenever any socket is installed — including a newer one from a reconnect. Reconnecting at the first moment a caller can know the connection died (the rejection of the in-flight call) leaves the dead socket's trailing 'close' to tear down the socket that replaced it, so recovery never survives:
FAIL reconnect succeeds immediately after a failure (connected=false)
FAIL the dead socket does not take the new one down (connected=false)
This one also throws an unhandled Error: connection closed out of the 'close' handler at src/connection.ts:89 and crashes the process.
Suggested shape
Give the handlers an identity check, and let fail() and close() share one teardown so both always settle pending calls — the only difference being whether the socket is destroyed or ended cleanly. Clearing state before touching the socket matters: it makes the events that destroy()/end() themselves trigger fall through the guards instead of re-entering.
const sock = net.createConnection({ host: this.host, port: this.port });
this.sock = sock;
const current = () => this.sock === sock;
sock.on('data', (chunk: Buffer) => {
if (!current()) return;
/* ...unchanged... */
});
sock.on('error', (err) => {
if (current()) this.fail(err);
reject(err);
});
sock.on('close', () => {
if (current()) this.fail(new Error('connection closed'));
});private teardown(err: Error, destroy: boolean): void {
const sock = this.sock;
this.sock = null;
this.shookHands = false;
this.ready = null;
this.buf = Buffer.alloc(0);
this.textFrames = [];
if (sock) {
if (destroy) sock.destroy(err);
else sock.end();
}
const pending = this.active ? [this.active, ...this.queue] : [...this.queue];
this.active = null;
this.queue = [];
for (const call of pending) call.reject(err);
}
private fail(err: Error): void {
this.teardown(err, true);
}
close(): void {
this.teardown(new Error('connection closed'), false);
}Regression test
I've written scripts/lifecycle-test.js — a mock server that handshakes and then misbehaves on purpose, covering all three parts of issue #6 plus the two regressions above. It's worth having in the repo either way, since it fails on main too (item 3, the uncleared parse buffer, crashes outright there). Say the word and I'll push it to this branch or land it separately so you can rebase onto it — whichever you prefer. With the changes above, it goes 8/8 green alongside the existing protocol test.
No rush on this, and thanks again — the diagnosis in your commit message was on the money.
close() now shares the failure teardown, rejecting in-flight and queued calls while ending the socket cleanly. Socket handlers only affect the connection that created them, so a dead socket's delayed close or error cannot tear down a replacement. Add an offline fake-socket lifecycle suite covering close, reconnect ordering, repeated events, and intentional reuse.
connect() stored its handshake promise and then dropped the reference in teardown(), so a socket that closed (or was explicitly closed) before the handshake reply left the caller awaiting forever. teardown() now owns a one-shot settlement handle for the outstanding attempt: it rejects the pending connect() with the causal error before taking the socket down, and a successful handshake clears it, so a stale error from a dead socket cannot settle a replacement's promise or re-settle an already rejected one. The lifecycle harness could also print FAIL lines and exit 0: it awaited raw promises that never settle, with nothing left on the event loop. It now sets a non-zero exit code before the first await, gives every awaited promise a timeout, adds a whole-suite watchdog, and records unhandled rejections. New cases cover remote close and explicit close before the handshake, stale close/error/duplicate events around an immediate reconnect, and the DwarfClient.connect() path.
|
Thanks for the detailed reproductions. Both reported regressions are fixed: close now rejects the active and queued calls, and each handler checks its own socket identity, so delayed events from a dead socket leave the replacement alone. The review also caught a connect promise that stayed pending if the socket closed before the handshake. Teardown now settles that attempt with its original error. I added offline lifecycle coverage and made the harness fail if an awaited promise stalls. Typecheck, lint, build, the five protocol checks and 42 lifecycle checks pass, with separate loopback TCP and regression verification. I have not tested against a live DFHack server. Hosted CI is waiting for maintainer approval: https://github.com/alexanderolvera/dfhack-remote-node/actions/runs/40252224414. |
Socket failures can leave
connectedstale; closing the client can strand queued calls; and a dead socket's late close event can tear down an immediate reconnect.Fixes #6.
Share teardown between failure and intentional close, reject the active and queued calls, and guard every socket handler by socket identity. Also settle an outstanding
connect()when the socket closes before the handshake reply, so both DfConnection and DwarfClient callers receive the causal error.The offline lifecycle suite covers close/reconnect ordering, stale and repeated events, pre-handshake aborts, and public-client behavior. Its waits are bounded and a stalled suite exits unsuccessfully.
Validation on the final head: typecheck, lint, build, five existing protocol checks, and 42 lifecycle checks pass. Separate verification reproduced the prior failures, passed independent promise/event checks and real loopback TCP tests, and confirmed that a never-settling mutation fails the harness. Node 24 was tested; live DFHack interoperability remains unverified. Hosted CI requires maintainer approval: https://github.com/alexanderolvera/dfhack-remote-node/actions/runs/40252224414.
AI assistance: the original patch was prepared with Claude Code. This revision used DeepSeek V4.1 Flash, separate independent verification, and final Codex review. No human review is claimed.