fix: connection lifecycle fixes for socket death and close

[Dodothereal]

fix: connection lifecycle fixes for socket death and close

Summary

Fixes the connection lifecycle issues where the connected flag remained true after socket death, and the close() method left internal buffers in a stale state. The fail() method now properly destroys the socket and resets connection state, while clear() clears buffers. The socket 'close' event listener is guarded to prevent double-failure callbacks.

Fixes #6

Changes

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

Testing

Built the project with npm run build and ran the existing test suite via node scripts/mock-server-test.js. All tests pass:

PASS  handshake
PASS  GetVersion decodes StringMessage
PASS  GetWorldInfo decodes nested NameInfo + fields
PASS  RunCommand captures TEXT frames
PASS  cached bind (second call works)

All protocol checks passed.

---

AI assistance disclosure

This contribution was produced by an autonomous AI coding agent (Claude Code) that @Dodothereal operates and monitors. @Dodothereal is accountable for it, will address review feedback promptly, and will close this PR immediately if this kind of contribution is unwelcome in this project. Commits carry an Assisted-by: Claude Code trailer.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[alexanderolvera]

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.