Arham-Begani opened a new pull request, #4552: URL: https://github.com/apache/iggy/pull/4552
## Which issue does this PR address? Closes #2396 ## Rationale An app using the Node.js Client cannot tell when the client is dialing, when it is connected, when the connection fails, or when it has shut down. The only emitter is the internal CommandResponseStream, and Client does not expose it. So there is no clean way to log reconnects, show connection status, or release resources after destroy(). The Rust SDK already reports this through DiagnosticEvent. #4061 started on this and was closed for inactivity. Its review asked for an end event for cleanup, an e2e test, and a reason for the error listener guard. This PR covers all three with a smaller change, and leaves the exported RawClient type as it is so existing custom clients still compile. ## What changed? `Client` now has typed `on`, `once`, `off` and `removeListener` for four events: `connecting` (once per dial), `connected` (TCP connected or TLS handshake done, before login, like Rust's `DiagnosticEvent::Connected`), `error`, and `end` (once, after `destroy()` closes the socket, and `await client.destroy()` resolves after it). - Every dial gets its own socket, so `connecting` is announced once per installed socket, with no extra state to deduplicate. It is deferred one tick so the constructor's first dial reaches listeners attached after it. - `error` is re-emitted only when a listener exists, because `EventEmitter` throws on an `error` nobody listens for. That is the only event that needs the guard, and it keeps a client without an `error` listener exactly as quiet as before. Commands still fail with the same errors. - `end` waits for the socket's own `close`, so no lifecycle event can follow it, and a second `destroy()` cannot repeat it. The connection records which sockets already closed, because Node sets `socket.closed` inside `destroy()`, before `close` fires. - The exported `RawClient` type is unchanged, so a custom `RawClient` passed to `SimpleClient` still type-checks. Questions for maintainers: 1. `off`/`removeListener`: without them an `on` listener can never be removed, and `events.once(client, 'connected')` from `node:events` throws `emitter.removeListener is not a function` if an error arrives first. Happy to drop them if you prefer `on`/`once` only. 2. `disconnected` and `eviction` (suggested on #4061) are not included. The internal `disconnected` fires on every socket close, including dials that never connected and `destroy()`, so it needs agreed rules first. `eviction` is a session event rather than a transport event. I can add either in a follow-up once the semantics are agreed. 3. While testing I found that one outage emits `reconnect maxRetries exceeded` twice on the internal connection: the last failed dial's `close` starts a second `reconnect()` that gives up at once. This exists on master today and becomes visible through the public `error` event. I left it out of this PR and can open a separate issue with a failing test. ## Local Execution - Passed: `npm run lint`, `npm run build`, `npm run test:unit` (419 passed: 405 existing plus 14 new), `npm run test:e2e` against a local `iggy-server` built from this branch (72 passed, including the new lifecycle test), and `src/e2e/tls.system.e2e.ts` against a TLS server (8 passed). - Pre-commit hooks ran: `prek run` passed (markdownlint, license headers, typos, node eslint, whitespace checks). - Ran on Node 22.22.0. Not run on Node 23 or 24, the versions CI uses. ## AI Usage 1. Claude Code (Claude). 2. Claude investigated #4061 and the existing connection code, proposed the design, and wrote the implementation, the tests and the README section. It also ran the repository's `team-review-slim` review on the branch. The review found that `end` could come before the internal `disconnected` when the socket was already closing, and that a throwing `end` listener could leave `destroy()` pending. Both are fixed, with a regression test. 3. The checks listed above ran locally (in my Claude Code session, not CI), including the e2e suites against a real server. The `error` guard test was checked by removing the guard and confirming that the test fails. 4. Yes -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
