Code Review

Overview

This PR fixes a genuine connection hang bug in performSentLogin7WithStandardLogin. Before the fix, if the socket stalled or errored after the first (non-EOM) Login7 response packet, await once(tokenStreamParser, 'end') would block forever — the abort signal had no effect on it. The change races the parser's end event against signalAborted, exactly as the NTLM, federated auth, and initial-SQL login paths already do. The fix is small, correct, and consistent with the established pattern.

Production Code (src/connection.ts)

The fix is correct. signalAborted is already properly set up at the top of the method (line 3420–3423) and torn down in the finally block, so adding it to the Promise.race here works without any additional plumbing. The pattern is identical to the equivalent lines in performSentLogin7WithNTLMLogin (line 3476–3477), performSentLogin7WithFederatedAuthentication (3532–3533), and performLogin7WithInitialSQLRequest (3640–3641), which is exactly what the PR description claims.

One observation out of scope for this PR but worth flagging: line 3825 has a bare await once(tokenStreamParser, 'end') in the attention-token handler path. That code lives inside a socket event callback in a different state machine context and may not need the same treatment, but it's worth a second look to confirm it can't hang under similar conditions.

Tests (test/unit/connection-failure-test.ts)

Two new tests are added, targeting exactly the right scenarios:

1. Socket abort mid-response — sends one non-EOM packet then destroys the socket. Verifies that connect() gets a ConnectionError with the expected 'Connection lost - socket hang up' message and cause.
2. Response stall / connect timeout — sends one non-EOM packet then goes silent. Verifies that connect() gets a timeout error with code 'ETIMEOUT'.

The test structure follows the existing conventions in the file exactly (mock server, IncomingMessageStream/OutgoingMessageStream, messageIterator, etc.). Using new Packet(0x04) directly to construct a packet without the EOM status bit is the right way to simulate an incomplete response.

Minor nits (non-blocking):

• Both new tests (and several pre-existing ones) declare const chunks: Buffer[] = [] and push into it inside for await loops, but never actually read the accumulated data. The loop is necessary to drain the message stream so messageIterator.next() can proceed — the variable just adds visual noise. Consider for await (const _ of message) {} or a // drain comment to make the intent clearer. This is consistent with the existing file style, so it is a low-priority cleanup.
• console.log(err) in the server-side catch blocks is a pre-existing pattern throughout the test file; it is fine for test diagnostics.
• connectTimeout: 1000 in the stall test means it takes a full second to run. That is the minimum needed to avoid flakiness, so it is the right call.

Summary

The bug fix is correct, minimal, and well-motivated. The two new tests provide solid regression coverage for both failure modes (hard abort and soft stall). The import { Packet } addition is the only structural change needed and is scoped correctly. No issues blocking merge.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.66%. Comparing base (2035eda) to head (a2eb626).

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   78.82%   79.66%   +0.83%     
==========================================
  Files          90       90              
  Lines        4892     4892              
  Branches      922      922              
==========================================
+ Hits         3856     3897      +41     
+ Misses        737      692      -45     
- Partials      299      303       +4     

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