refactor: iterate login response tokens directly

[arthurschreiber]

The four login-path response handlers (performSentLogin7WithStandardLogin, ...WithNTLMLogin, ...WithFedAuth, and performLoggedInSendingInitialSql) consumed their response tokens by wrapping the StreamParser.parseTokens async generator back into a Readable/EventEmitter (via createTokenStreamParser) just to await once(parser, 'end') while token dispatch happened through data events.

They now iterate the generator directly through a small shared processTokens(message, handler, signalAborted) helper: pull a token, dispatch it to the handler, until the message ends or the abort promise rejects.

Besides removing a layer of indirection, this fixes a latent error-handling hole: a parser error during login previously fired as an error event on the internal Readable — which had no error listener, crashing the process — while the end event the login methods were waiting on never fired. With direct iteration, a parser error simply rejects and surfaces as a clean connection failure through the existing error handling in initialiseConnection. A regression test covers this: on the old code it reproduces the uncaught exception, followed by the connect callback only firing at the connect timeout with a misleading Failed to connect error; on this branch the parser error surfaces immediately.

The token stream parser wrapper is still used by the SENT_CLIENT_REQUEST/SENT_ATTENTION states; those move to direct iteration (and the wrapper gets deleted) in the upcoming makeRequest refactor.

Performance: connect-path only, and measured as neutral — benchmarks/connection/open.js before/after (median of 3 runs, local SQL Server): 32.2/37.1 conn/s on master vs 32.3/36.9 conn/s with this change (n=10/100).

🤖 Generated with Claude Code

[claude]

Review

Nice, focused refactor — replacing the Readable/once('end') wrapper with direct async-generator iteration for the login-path handlers is a good simplification, and the error-propagation improvement it enables is real and valuable.

Correctness

• Error propagation fix checks out. Previously, an error thrown while parsing tokens surfaced as an error event on the internal Readable created by Readable.from() in token-stream-parser.ts, and since nothing was listening for 'error' on that stream, it would crash the process (unhandled 'error' event) while the awaited 'end' event never fired. With processTokens, tokens.next() rejecting simply propagates as a normal promise rejection through Promise.race, withAbortRace, and up to the existing initialiseConnection error handling. This is a genuinely good fix for a real crash bug, not just a style refactor.
• processTokens control flow looks correct: it mirrors the dispatch logic in token-stream-parser.ts's Parser class exactly (debug.token(token) then handler[token.handlerName](...)), and the result.done check correctly matches the generator's return path in stream-parser.ts (clean end-of-message vs. thrown NotEnoughDataError/parse error, which propagates instead of returning normally).
• Dangling tokens.next() on abort: when signalAborted wins the race, the pending tokens.next() call is left running against the message stream. This is consistent with the existing pattern already used everywhere else in this function (e.g. Promise.race([this.messageIo.readMessage()..., signalAborted])), and since both promises are attached to Promise.race, there's no unhandled-rejection risk — just flagging that it's the same "abandon in place" pattern as before, not a regression.
• The token === undefined branch in processTokens looks unreachable given the current parseTokens implementation (it only ever yields defined tokens), but it's harmless defensive code that mirrors the generator's return type — not a concern.

Style / scope

• Good, minimal diff — no unrelated churn. Leaving createTokenStreamParser/TokenStreamParser in place for the SENT_CLIENT_REQUEST/SENT_ATTENTION states (with a stated follow-up) rather than doing a bigger-bang refactor is the right call.
• Doc comment on processTokens is clear and states the @private contract consistently with the rest of the file.

Test coverage

• test/unit/connection-failure-test.ts has good coverage for abort-during-login scenarios (aborted after Login7 sent, while response is streaming, after response received, timeout, etc.), but I didn't find a test that exercises a malformed/truncated token stream during login — i.e. the actual crash scenario this PR fixes. Since the PR description calls out a fix for a process-crashing bug, a regression test (server sends an incomplete/invalid Login7 response, e.g. a truncated LOGINACK token, and the connection should reject/emit 'error' rather than crash) would be valuable to lock in the fix and prevent a future regression back to the Readable-wrapper pattern. Might be worth adding before merge, or as a fast follow.

Performance / security

• No concerns — this is connect-path only, and the PR description includes reasonable before/after benchmark numbers showing neutral impact. No security-relevant surface changed (parsing logic itself is untouched, only how the caller drives it).

Overall this is a solid, well-scoped refactor with a legitimate bug fix. Main suggestion is adding a regression test for the crash scenario described in the PR body.

[codecov]

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.89%. Comparing base (a99826d) to head (6079393).

Files with missing lines	Patch %	Lines
src/connection.ts	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1759      +/-   ##
==========================================
+ Coverage   79.88%   80.89%   +1.00%     
==========================================
  Files          90       90              
  Lines        4887     4893       +6     
  Branches      924      926       +2     
==========================================
+ Hits         3904     3958      +54     
+ Misses        694      639      -55     
- Partials      289      296       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.