feat: add AbortSignal support to request execution

[arthurschreiber]

Summary

Adds AbortSignal support to request execution, as laid out in the plan on #1765 (part 2 of the sequencing there): a caller-controlled way to express total-time bounds (AbortSignal.timeout()), deadlines, and linked cancellation (AbortSignal.any()) — instead of introducing a second driver-level timeout option.

All request-execution entry points accept a new trailing options?: { signal?: AbortSignal } argument:

• execSql, execSqlBatch, execute, callProcedure, prepare, unprepare
• execBulkLoad (the signal covers both the insert bulk statement and the bulk load message)
• beginTransaction, commitTransaction, rollbackTransaction, saveTransaction

connection.execSql(request, { signal: AbortSignal.timeout(60_000) });

The signal is scoped to that single execution — matching fetch(url, { signal }) — so a Request that is executed multiple times (the prepare/execute flow) gets a fresh signal per execution.

Semantics

• Abort mid-flight cancels the request through the existing graceful cancellation mechanism (terminating the request message with the IGNORE bit, or sending an attention message). The connection remains usable afterwards, and the worst case is bounded by cancelTimeout (the existing backstop tears the connection down if the server never acknowledges the attention). Connection-fatal errors during the cancellation (socket failure, cancelTimeout teardown, close()) supersede the request-level outcome, as they do for every other cancellation source.
• The request completes with the signal's abort reason (err === signal.reason), so a TimeoutError from AbortSignal.timeout() is distinguishable from a manual ECANCEL cancellation. A non-Error abort reason is wrapped in a RequestError with code EABORT; Error reasons from other realms are recognized via util.types.isNativeError and passed through unchanged.
• First failure cause wins: an error recorded before the abort (a server error — including one whose token arrives in the same tick the abort is triggered from, a request timeout's ETIMEOUT, or a manual request.cancel() before or during execution) takes precedence over the abort reason. Aborting also stops the request timer, so a slow attention acknowledgement can no longer replace the abort reason with a spurious ETIMEOUT.
• An already-aborted signal fails the request immediately with the abort reason, without anything being sent to the server (for a bulk load, the row iterable is never started).
• Validation is duck-typed, byte-for-byte matching Node core's validateAbortSignal, so cross-realm AbortSignals and ponyfills are accepted; invalid values throw a TypeError synchronously at every entry point, before any request or connection state is touched.
• The abort listener is armed per execution and removed on every completion path, so a long-lived signal shared across many requests does not accumulate listeners, and signals of sequential executions of the same Request stay isolated.

Note for wrappers switching on err.code: a default abort reason is a DOMException, whose legacy code property is numeric — match abort outcomes on err.name ('AbortError', 'TimeoutError') or on the signal's state. This is called out in the execSqlBatch typedoc, which the other methods reference.

Relationship to the existing cancellation API

request.cancel() and connection.cancel() stay, undeprecated — they remain the imperative convenience for canceling a request without having set up a signal beforehand (the same coexistence as SqlCommand.Cancel() alongside CancellationToken in SqlClient, or Statement.cancel() alongside query timeouts in JDBC). The docs now recommend signal for new code. As part of the async makeRequest rework (#1656/#1492), the internal layering should invert: cancellation becomes one internal per-request AbortController (feeding writeMessage's cancelSignal from #1756), with request.cancel(), requestTimeout, and the user's signal as its triggers.

Implementation

The per-execution state lives on the Connection, alongside its existing request-scoped state: requestAbortSignal mirrors requestTimer/cancelTimer, the pre-bound _onRequestAbort handler mirrors _cancelAfterRequestSent, and clearRequestAbortListener() is called at the same completion sites as the timer clears (cleanupConnection, the end-of-message completion, and the attention-ack delivery). Request and BulkLoad are untouched apart from docs. This shape is also a rehearsal for the async makeRequest rework, where the field collapses into a try/finally local.

Testing

24 tests in test/unit/request-abort-signal-test.ts, using the fake-server pattern from the cancel tests: already-aborted (request, transaction, bulk load — including that the row iterable is never consumed), non-Error reason, cross-realm Error reason, abort after send (attention exchange + stream alignment), abort during response streaming, abort from within the errorMessage event, AbortSignal.timeout() reason identity, abort/requestTimeout/manual-cancel precedence in all orderings (pre-execution and in-flight), abort during each bulk-load phase (with generator finalization asserted), the TDS < 7.2 transaction bookkeeping guards, close() with an armed listener, listener cleanup, signal isolation across sequential executions of the same Request, duck-typed signal acceptance, and synchronous TypeError validation at every entry point.

• npm test: 451 passing (427 on master + 24), 0 failing; repeat runs green
• npx eslint src test + tsc: clean

Independent behavior fixes bundled in this PR (release-note worthy on their own)

• TDS < 7.2 emulated-transaction bookkeeping: transactionDepth/inTransaction are now only updated when the transaction statement succeeded. Previously any failure on that path (ECLOSE, ECANCEL, EINVALIDSTATE, server errors) corrupted the client-side counters.
• Bulk-load row-pipeline teardown: when the insert bulk statement fails — for any reason, not just an abort (e.g. the existing schema-mismatch UNKNOWN error) — the row stream and rowToPacketTransform are now destroyed, finalizing a suspended row generator instead of leaving it paused indefinitely. This was a pre-existing resource leak on master.
• Server errors are recorded before the errorMessage event is emitted (token/handler.ts), closing a race where a listener reacting synchronously could observe/act before the error was recorded.

Known trade-offs

• Aborting a transaction method mid-flight can leave the client-side transactionDepth/inTransaction bookkeeping out of sync with server state if the server already processed the request — the same exposure requestTimeout has on those requests today.
• An abort during a bulk load's insert bulk phase does not set bulkLoad.canceled (the bulk load still completes with the abort reason); an abort during the bulk-load message phase does.
• An aborted execution sets the Request's canceled flag, so the same Request object cannot be re-executed — matching master's existing behavior for manual cancels and request timeouts; retry with a fresh Request.
• Aborting connection establishment is out of scope; connect() is internally signal-based already, so a public connect(callback, { signal }) would be a natural follow-up.

Refs #1765

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-31T11:22:57.292823Z	f2a7bb9	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Review

Went through src/connection.ts and the new test/unit/request-abort-signal-test.ts in detail. Overall this is a well-designed, carefully-implemented feature — the "first failure cause wins" semantics (request.error ??= ...), stopping the request timer on abort to prevent a spurious ETIMEOUT overwrite, and the duck-typed signal validation are all solid, and the test suite is unusually thorough (already-aborted signals, mid-flight aborts, bulk load, timer races, listener cleanup on connection close, etc.).

One correctness concern and a related coverage gap stood out:

Potential bug: reusing a Request across multiple executions after a cancellation/abort

Request.canceled (src/request.ts:362) is only ever set to false in the constructor, and to true in cancel() — it's never reset back to false. Connection#makeRequest (src/connection.ts:3322) short-circuits with a generic ECANCEL whenever request.canceled is true, regardless of whether a new, non-aborted signal is passed in.

The PR description explicitly calls out the prepare/execute flow as a supported reuse pattern ("a Request that is executed multiple times ... gets a fresh signal per execution"), and that's exactly the case where this bites: if a prepared Request is executed once with, say, AbortSignal.timeout(...) and that execution times out (or is otherwise aborted/cancelled), request.canceled becomes true permanently. Every subsequent connection.execute(request, params, { signal: freshSignal }) call on that same Request will now fail immediately with ECANCEL before anything is sent to the server — even though the connection itself and the fresh signal are both fine. This seems to contradict the documented "connection remains usable after an aborted request" guarantee at the Request level (a very plausible pattern: retry a prepared statement with a new per-call timeout after a previous timeout).

This latent behavior predates this PR (it would already affect a plain connection.cancel() + reuse), but this PR's headline use case (per-call AbortSignal.timeout()) makes hitting it far more likely in practice, so it seems worth addressing here — e.g. resetting canceled (and error) at the top of makeRequest alongside the existing request.error = undefined reset, rather than only clearing error.

Test coverage

Related to the above: none of the new tests reuse the same Request instance for a second execution after an abort — the "connection remains usable" tests (e.g. lines ~310, ~540, ~816, ~955, ~1273) all issue a new Request for the follow-up execSqlBatch call. Given the PR text specifically highlights the prepare/execute reuse scenario, a test that does prepare → execute (abort mid-flight) → execute again on the same Request would be valuable, and would likely surface the issue above.

Minor notes (no action needed, just flagging)

• errorForAbortedSignal's reason instanceof Error check correctly treats Node's DOMException as an Error (so default AbortSignal.timeout()/controller.abort() reasons pass through unwrapped), and the typedoc callout about err.code being numeric on DOMException is a nice, easy-to-miss gotcha to document.
• The synchronous TypeError for an invalid options.signal (thrown before the connection-state/request.error reset) is consistent with how this codebase already treats other programmer errors like assertValidIsolationLevel in beginTransaction, rather than routing through the callback — good consistency call.
• No security concerns: no new user-controlled data reaches the wire, validation is appropriately duck-typed without eval/prototype-pollution risk, and listener lifecycle looks leak-free (armed once per execution, removed on every completion path including connection teardown).
• Performance impact is negligible — one extra addEventListener/removeEventListener pair per execution, mirroring the existing timer bookkeeping pattern.

Nice work overall — the design doc-quality comments in the diff make the intended semantics very easy to follow.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3225ebb720

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

I read through the src/connection.ts diff and the new test/unit/request-abort-signal-test.ts (15 tests) in full, and cross-checked the surrounding state-machine code (SENT_CLIENT_REQUEST/SENT_ATTENTION, cleanupConnection, makeRequest) that isn't touched by the diff but interacts with it. Overall this is a well-designed, carefully-reasoned addition — the "first failure cause wins" precedence (via request.error ??= … before request.cancel()), the request-timer teardown to stop a delayed ETIMEOUT from clobbering the abort reason, and the listener cleanup on every completion path (cleanupConnection, onEndOfMessage, attention-ack success) all check out against the state machine. The duck-typed signal validation mirrors Node core's validateAbortSignal, and the 15 new tests do a good job exercising real races against a fake TDS server (abort-vs-timeout in both orders, abort mid-attention-ack, abort mid bulk-load message, listener accumulation, etc.).

A couple of smaller things worth a look:

Potential bug: invalid signal passed to execBulkLoad can still consume the row iterable

In execBulkLoad, the guard against starting the row stream is keyed only off options?.signal?.aborted !== true:

if (options?.signal?.aborted !== true) {
  const rowStream = Readable.from(rows);
  ...
  rowStream.pipe(bulkLoad.rowToPacketTransform);
}

The actual signal validation (duck-typing, throwing TypeError for a non-AbortSignal value) happens later, inside makeRequest, when execSqlBatch(request, options) is called at the end of execBulkLoad. makeRequest's TypeError throw is synchronous, but Readable.from(rows).pipe(...) doesn't pull from the source synchronously — pipe()'s resume() is scheduled via process.nextTick. So if a caller passes something signal-like but invalid (e.g. {}, matching the exact case the existing execSqlBatch "throws a TypeError" test uses), execBulkLoad throws synchronously as expected, but the row generator/iterable has already been wired up to start pulling on the next tick regardless — the exception doesn't prevent that. For a generator with side effects (e.g. opening a resource, or the rowsConsumed style assertion used in the "already aborted" test in this same PR), this means rows can still be consumed for a bulk load that will never be sent. The "already aborted" case is explicitly guarded against exactly this problem; the "invalid signal shape" case isn't. Might be worth doing the signal validation up front in execBulkLoad (or reusing a shared validator) before wiring up the row stream, so both cases short-circuit the same way. Low severity since it requires a malformed signal argument, but easy enough to fix while it's fresh, and there's no test today that would catch it either way.

Bundled behavior change: transactionDepth/inTransaction bookkeeping fix on the TDS < 7.2 path

beginTransaction/commitTransaction/rollbackTransaction/saveTransaction now guard the transactionDepth++/-- and inTransaction updates with if (!err) on the legacy (tdsVersion < '7_2') fallback path. Previously the bookkeeping was updated unconditionally even when the SET TRANSACTION ISOLATION LEVEL.../COMMIT TRAN/etc. request failed — which looks like a genuine pre-existing bug (not abort-specific): any failure on that path, including a plain server-side error unrelated to AbortSignal, would leave transactionDepth/inTransaction out of sync with the server. Good catch bundling this in, but it's a behavioral change beyond the scope of "add abort support," and I didn't find any existing or new unit test that exercises transaction bookkeeping on an error path for the TDS < 7.2 branch (the new test file only exercises the already-aborted case for beginTransaction). A small regression test asserting transactionDepth/inTransaction are left unchanged after a failed commitTransaction/rollbackTransaction on that path would pin down the fix and guard against it being silently reverted later.

Note, not a bug: bulkLoad.canceled asymmetry

Confirmed the trade-off called out in the PR description: an abort during the insert bulk phase cancels the underlying (internal) Request, not the BulkLoad itself, so bulkLoad.canceled stays false even though the bulk load ultimately fails with the abort reason; an abort during the bulk-load message phase does set it, since this.request === bulkLoad at that point. This matches the existing (pre-abort-signal) cancellation plumbing, so it's not a new bug, but since BulkLoad#canceled is public API, it might be worth a one-line mention in the execBulkLoad TSDoc itself (not just the PR description) so it shows up in the published typedocs for anyone branching on that flag.

Security / performance

No concerns — no new I/O, no injection surface, and the abort path reuses the existing graceful-cancellation/attention machinery rather than adding a new one. The once: true listener option and the cleanup-on-every-path design avoid the accumulating-listener footgun on long-lived signals, which the tests explicitly verify (getEventListeners(...).length === 0 after various completion paths).

Test coverage

Very thorough for the core feature — the fake-server based tests cover real interleavings (abort racing the attention ack, abort racing requestTimeout in both orders, abort mid-response-stream, etc.) rather than just mocking. The two gaps I'd add are the ones above: an invalid signal passed to execBulkLoad, and a non-abort failure on the TDS < 7.2 transaction bookkeeping path.

Nice work overall — this is a solid, minimal-surface-area way to add cancellation-with-reason without introducing a second timeout concept.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e867340212

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[codecov]

Codecov Report

❌ Patch coverage is 87.32394% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.90%. Comparing base (cda8066) to head (03ee9aa).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/connection.ts	87.14%	4 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1766      +/-   ##
==========================================
+ Coverage   80.84%   80.90%   +0.05%     
==========================================
  Files          90       90              
  Lines        4887     4922      +35     
  Branches      924      940      +16     
==========================================
+ Hits         3951     3982      +31     
- Misses        640      641       +1     
- Partials      296      299       +3     

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

[claude]

Reviewed the diff (src/connection.ts, src/request.ts, test/unit/request-abort-signal-test.ts). This is a well-designed, carefully-implemented feature — the per-execution AbortSignal state mirrors the existing requestTimer/cancelTimer pattern nicely, and the precedence rules between server errors, ETIMEOUT, and the abort reason are handled with clear, explicit ordering (??= semantics + stopping the request timer on abort). The duck-typed signal validation and cross-realm Error handling (isNativeError) are nice touches that will avoid friction for ponyfills/other realms.

Code quality / simplification

In the SENT_ATTENTION completion handler (src/connection.ts, around line 4036-4045), the new branch duplicates the action of the existing ETIMEOUT branch:

```js
if (sqlRequest.error && sqlRequest.error instanceof RequestError && sqlRequest.error.code === 'ETIMEOUT') {
sqlRequest.callback(sqlRequest.error);
} else if (sqlRequest.error && abortSignal !== undefined && abortSignal.aborted) {
sqlRequest.callback(sqlRequest.error);
} else {
sqlRequest.callback(new RequestError('Canceled.', 'ECANCEL'));
}
```

Both of the first two branches do exactly the same thing (sqlRequest.callback(sqlRequest.error)), and given the invariants in this PR, sqlRequest.error can only be truthy at this point because either requestTimeout() set it unconditionally or _onRequestAbort's ??= set it — there's no other path that leaves a truthy sqlRequest.error reaching this state without a cancel()/ECANCEL already being folded in earlier. So the abortSignal.aborted condition doesn't actually verify that the abort caused the cancellation (a signal that's aborted after an unrelated request.cancel() also makes this true) — it happens to produce the right result today only because _onRequestAbort bails out early via if (request.canceled) return; and leaves sqlRequest.error untouched in that case.

This could be simplified (and made more robust against future changes that might set request.error through some other path) to:

```js
if (sqlRequest.error) {
sqlRequest.callback(sqlRequest.error);
} else {
sqlRequest.callback(new RequestError('Canceled.', 'ECANCEL'));
}
```

which drops the now-redundant ETIMEOUT/abortSignal distinction entirely and defers to whatever error was actually recorded — arguably safer than silently falling back to a generic ECANCEL if a future code path ever sets sqlRequest.error to something else. Not a bug as it stands (the existing tests all pass with today's invariants), just a spot that could be tightened up.

Other observations (no issues found)

• Listener lifecycle: traced all this.request = undefined completion sites (cleanupConnection, the SentClientRequest/SentAttention end-of-message paths, the two socketError handlers) — clearRequestAbortListener() is called at every one either directly or via cleanupConnection(), so no listener leak on a long-lived signal.
• Precedence ordering: the abort-vs-ETIMEOUT race is handled correctly in both directions per the dedicated tests (abort clears the request timer so a late timer fire can't overwrite the abort reason; timeout firing first sets canceled = true, causing a later abort to short-circuit via the request.canceled guard).
• Bulk load two-phase abort: verified the insert bulk phase and the bulk-load-message phase both correctly wire the same options.signal through, and an abort during the first phase short-circuits before makeRequest is called for the second phase — matches the documented "does not set bulkLoad.canceled during the insert bulk phase" trade-off, which has no observable effect since bulkLoad.canceled isn't read anywhere except by BulkLoad#cancel() itself.
• Security: no concerns — the signal validation is pure duck-typing against safe property checks (signal === null is special-cased before the in operator to avoid a TypeError), and no user-controlled data reaches string interpolation, logging, etc. that isn't already flowing through today.
• Performance: negligible — one addEventListener/removeEventListener pair per execution when a signal is passed, no-op otherwise.
• Backward compatibility: all new options parameters are trailing and optional, and existing positional-arg call sites are unaffected.
• Test coverage: the 15 new tests are thorough and use the existing fake-server pattern; they specifically exercise the tricky orderings (abort-before-timeout, timeout-before-abort, cancel-before-abort) rather than just the happy path, and check listener cleanup (getEventListeners) after every completion path. Good use of runInNewContext to test the cross-realm Error case.

Nice work overall — the doc comments (especially the err.name vs. numeric DOMException.code callout) will save real debugging time for consumers.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84efe94797

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

This is a well-designed and carefully-iterated feature. The semantics (per-execution signal, "first failure cause wins", listener lifecycle tied to request completion, already-aborted fast path, cross-realm Error/AbortSignal handling) are all thought through, and the state-machine integration (SENT_CLIENT_REQUEST / SENT_ATTENTION) correctly threads the abort reason through the existing cancellation machinery. The 1400+ line test suite exercises the tricky races well (abort vs. request-timeout ordering, abort during bulk-load insert bulk vs. during the bulk-load message itself, row-generator finalization, listener leak checks). A few smaller things worth a look before merge:

Bug: orphaned/mismatched doc comment (src/connection.ts:177-203)

During the last commit's refactor (extracting validateAbortSignal out of makeRequest), the doc comment for errorForAbortedSignal got left behind:

/**
 * The error a request aborted via its `AbortSignal` completes with: ...
 * @private
 */
/**
 * Duck-typed like Node core's `validateAbortSignal`, ...
 * @private
 */
function validateAbortSignal(signal: AbortSignal | undefined) { ... }

function errorForAbortedSignal(signal: AbortSignal): Error { ... }

Now validateAbortSignal has two stacked docblocks (the first describing the wrong function), and errorForAbortedSignal has none. Worth moving the first docblock down to errorForAbortedSignal.

Nit: stale reasoning in _onRequestAbort comment (src/connection.ts:1877-1881)

"Stop the request timer ... If the timer fired first, its ETIMEOUT error is already recorded and kept by the ??= above."

This is no longer quite accurate: since the request.canceled guard was added (commit 2), if the request timeout fired first, requestTimeout() already called request.cancel(), so _onRequestAbort returns at the top-of-function guard and never reaches the ??= line at all in that case. The end behavior is still correct (ETIMEOUT wins), just via the early return rather than the mechanism the comment describes — worth tightening the comment so it doesn't mislead a future reader chasing this logic.

Test coverage gaps

• The "only update transactionDepth/inTransaction on success" fix (commit 2) applies to beginTransaction, commitTransaction, rollbackTransaction, and saveTransaction's TDS < 7.2 fallback path, but the new tests only exercise it via beginTransaction. Since this fixes a real (pre-existing, not abort-specific) counter-corruption bug for the other three methods too, a couple of targeted tests there would lock in the fix.
• execute, prepare, unprepare, and callProcedure gained the options.signal parameter but have no direct test coverage with a signal — they all funnel through the same makeRequest, so risk is low, but a quick "passes options through" test per method (or at least one beyond execSqlBatch) would guard against a future argument-order slip.

Minor/optional

• Connection.prototype.transaction() (the higher-level helper wrapping beginTransaction/saveTransaction) doesn't expose options.signal, so it can't be used with the new mechanism. Might be intentionally out of scope given the PR's explicit method list — just flagging in case it was an oversight.
• validateAbortSignal runs up to three times for a single execBulkLoad call (once explicitly at the top, once via the internal execSqlBatch, once via the direct makeRequest for the bulk load payload). Harmless given how cheap the check is, but worth noting.

No security concerns — validation happens before any connection state is touched or data is sent, and the duck-typed signal check can't be leveraged to inject anything (it only reads aborted/reason and calls addEventListener/removeEventListener).

Nice work overall — the "first failure cause wins" semantics and the listener/timer cleanup discipline across every exit path (normal completion, cancellation, socket error, connection close) are the hard part of this feature, and they look solid.

[claude]

Review: feat: add AbortSignal support to request execution

I did a close read of src/connection.ts, src/request.ts, and the new test/unit/request-abort-signal-test.ts (I wasn't able to run npm ci/npm test in this sandbox due to network restrictions, so this is a static review only).

Overall

This is a very well-executed feature. The design choices are sound and match precedent in comparable drivers (SqlCommand.Cancel()/CancellationToken, Statement.cancel()/query timeouts):

• Signal is scoped per-execution (fetch-style), not per-Request, which correctly handles the prepare/execute-with-a-fresh-signal case.
• _onRequestAbort / clearRequestAbortListener mirror the existing requestTimer/cancelTimer lifecycle closely, and I traced every completion path (cleanupConnection, the onEndOfMessage handler in SENT_CLIENT_REQUEST, and the attention-ack handler in SENT_ATTENTION) - the abort listener is removed before this.request is cleared on all of them, so there is no listener leak and no risk of _onRequestAbort firing against a stale/undefined this.request.
• The 'first failure cause wins' precedence (request.error ??= ..., stopping the request timer on abort so a late ETIMEOUT can't clobber the abort reason) is correct and well covered by the ordering tests (abort-before-cancel, cancel-before-abort, timeout-before-abort, abort-before-timeout).
• isNativeError for cross-realm Error reasons and the duck-typed validateAbortSignal (matching Node core's own implementation) are the right calls for ponyfill/cross-realm compatibility.
• The bulk-load row-pipeline teardown fixes (destroying the row stream/rowToPacketTransform on insert bulk failure, not starting the row iterable when already aborted) close real resource leaks - good catches, proven with a generator finally block in the test rather than just asserting the callback fired.
• The TDS < 7.2 emulated-transaction bugfix (only updating transactionDepth/inTransaction on success) is a legitimate, independent bug fix and is called out clearly as such in the PR description.

Minor / nit-level observations (nothing blocking)

1. execBulkLoad streaming-mode typing gap (src/connection.ts ~L2915): the only exported overload requires the rows argument. A caller using BulkLoad's streaming mode (writing rows manually, calling execBulkLoad(bulkLoad) with no rows) has no typed way to pass options.signal without passing undefined for rows explicitly. This predates the PR, so not a regression, just slightly more visible now.
2. Import style nit: import { isNativeError } from 'util/types' - the rest of the codebase (e.g. debug.ts) imports * as util from 'util'. Purely cosmetic.
3. Minor redundancy: options.signal gets validated once explicitly in execBulkLoad and then again inside makeRequest (once for the insert bulk execSqlBatch call, once for the BULK_LOAD payload call). Harmless - it is what makes the 'fail before touching bulk-load state' guarantee correct.

Test coverage

The 18 tests in test/unit/request-abort-signal-test.ts are thorough and use the existing fake-TDS-server pattern rather than mocking Connection internals, giving good confidence in real message-stream behavior (attention exchange, IGNORE bit, message-stream realignment after cancellation). Coverage includes already-aborted signals (request/transaction/bulk load, including 'row iterable never started'), non-Error/cross-realm reasons, precedence ordering against manual cancel and requestTimeout in both orderings, listener cleanup on normal completion and on close(), and duck-typed signal acceptance/rejection. I do not see an obvious gap.

Security

No concerns - this only affects client-side cancellation plumbing; no new user input is parsed or trusted differently.

Nice work - the iterative commit history shows the trickier edge cases (row-pipeline teardown, listener-arming order, cross-realm errors, transaction bookkeeping) were already run down before this review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0519998d1e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[arthurschreiber]

Follow-up on the review comments above (870d174):

Actioned:

• The stale reasoning in the _onRequestAbort comment is corrected — since the request.canceled guard was added, a timeout-first abort returns early rather than relying on the ??=, and the comment now says so.
• The TDS < 7.2 transaction bookkeeping guards are now pinned by a test that negotiates TDS 7.1 against the fake server: an aborted BEGIN TRAN/COMMIT TRAN leaves transactionDepth/inTransaction untouched, a successful BEGIN TRAN still updates them.
• The orphaned errorForAbortedSignal docblock was already reattached in 0519998.

Declined, with reasons:

• Collapsing the SENT_ATTENTION branches to if (sqlRequest.error) would change behavior: a plain cancel() after a server error token currently completes with ECANCEL (asserted by the existing bulk-load cancel test), and the abortSignal.aborted condition is what preserves that. The three-way branch is deliberate, not redundant.
• Per-method signal pass-through tests for execute/prepare/unprepare/callProcedure: these are one-line options forwards, verified against the emitted declarations; a test per method would pin nothing beyond what TypeScript already enforces.
• Connection#transaction() (the nesting helper) intentionally doesn't expose signal yet — its savepoint/rollback orchestration gives an abort mid-helper murkier semantics, so it's left for a follow-up alongside connect(callback, { signal }).

---

Generated by Claude Code

[claude]

Review

I read through the full commit history (6 commits) and the final state of src/connection.ts, src/request.ts, and the new test/unit/request-abort-signal-test.ts. This is a well-scoped, carefully iterated feature — the commit history shows the author already caught and fixed several subtle issues themselves (row-iterable leaks on an already-aborted bulk load, validating the signal before mutating any state, cross-realm Error recognition via isNativeError, ignoring a late abort after an earlier cancellation, and the TDS < 7.2 transaction-bookkeeping-on-failure bug). That self-review loop is exactly the kind of thing that's easy to skip, so it's good to see it done up front.

Code quality / correctness

• The state machine changes are minimal and follow existing patterns closely: requestAbortSignal/_onRequestAbort/clearRequestAbortListener mirror requestTimer/_cancelAfterRequestSent almost 1:1, which keeps this easy to review against the existing cancellation code.
• _onRequestAbort's early return on request.canceled (src/connection.ts:1868) correctly ensures "first failure cause wins" — a manual cancel() or requestTimeout that fires first keeps its own error/code instead of being overwritten by a later abort.
• One small simplification opportunity in the SENT_ATTENTION completion handler (src/connection.ts:4064-4073): the ETIMEOUT branch and the abortSignal.aborted branch both just do sqlRequest.callback(sqlRequest.error). They could be collapsed into one if (sqlRequest.error && (isTimeout || abortSignal?.aborted)) { … } else { ECANCEL } — not a bug, just a minor readability nit since the two conditions are mutually exclusive in practice (the timer is cleared as soon as an abort is recorded).
• Connection#transaction() (the auto begin/commit/rollback/savepoint helper) doesn't grow an options parameter, so it has no way to plumb a signal down to the beginTransaction/commitTransaction/rollbackTransaction/saveTransaction calls it makes internally. That's probably fine as an intentional scope cut for this PR, but it means users who want cancellable transactions have to drop down to the manual transaction API — might be worth a one-line callout in the PR description or a follow-up issue so it doesn't get lost.
• Minor API ergonomics: beginTransaction(callback, name, isolationLevel, options) requires passing undefined for isolationLevel to reach options when the default isolation level is wanted (as the new legacy-transaction test itself has to do). This matches the existing positional-args style of these methods, so it's consistent, just a bit awkward at the call site — not something this PR needs to fix given the constraint of staying backward compatible.

Bugs

Nothing that looks like an actual defect. I traced through the interaction between _onRequestAbort, the request timer, request.cancel(), and both completion paths (onEndOfMessage and the attention-ack handler), including the "abort while already canceled," "abort races with ETIMEOUT," and "signal already aborted at call time" cases, and the precedence rules described in the PR body hold up in the code.

Performance

No concerns — the added work per execution is one duck-typed check and one addEventListener/removeEventListener pair, and the listener is always torn down on every completion path (including cleanupConnection), so a long-lived shared signal won't accumulate listeners across many requests.

Security

No concerns. This is pure control-flow/cancellation plumbing; no new user input reaches string interpolation, no new I/O surface, and validation of options.signal happens synchronously before any connection or request state is touched.

Test coverage

The 19 tests in test/unit/request-abort-signal-test.ts are thorough and use a realistic fake-TDS-server harness rather than mocking internals, which gives good confidence the behavior holds at the protocol level. Coverage includes: already-aborted signals (request/transaction/bulk load, including that the row iterable is never consumed), duck-typed/cross-realm signals, TypeError validation (including for bulk loads specifically, before any state mutation), abort-after-send vs. abort-during-response-streaming, precedence ordering against requestTimeout and manual cancel(), bulk-load row-pipeline teardown on abort during each phase, listener cleanup counting, and the TDS < 7.2 legacy transaction bookkeeping guard. I didn't spot an obvious gap; if anything, a test that composes two signals via AbortSignal.any() would be a nice belt-and-suspenders addition, but since the implementation only ever touches the standard AbortSignal interface, that's very low risk and largely redundant with the existing tests.

Overall this looks ready to merge from a correctness/quality standpoint — the two notes above (the collapsible branch and transaction() scope) are non-blocking.

[claude]

Review: AbortSignal support for request execution

Nicely scoped and well-documented implementation. The precedence rules (first failure wins, ETIMEOUT vs abort vs manual cancel), the per-execution (not per-Request) signal lifecycle, and the duck-typed validation are all sensible design choices that mirror fetch's semantics well. I traced through the state machine (makeRequest, _onRequestAbort, SENT_CLIENT_REQUEST/SENT_ATTENTION) and the bulk-load row-pipeline teardown, and didn't find correctness bugs — the listener-arming-before-mutation ordering, the clearRequestAbortListener() call sites mirroring the existing timer-clearing sites, and the request.canceled early-return in _onRequestAbort all line up with what the 17 new tests assert.

Code quality / correctness

• _onRequestAbort (src/connection.ts:1862) correctly sets request.error before calling request.cancel(), so the pre-existing onCancel handlers' request.error ??= new RequestError('Canceled.', 'ECANCEL') never clobber the abort reason. Good ordering, and it's exercised by the "already canceled" precedence test.
• The addEventListener('abort', ...) call in makeRequest is placed before any connection state (isSqlBatch, this.request, timers) is mutated, so a throwing duck-typed signal leaves the connection untouched — matches the commit message's stated intent and is a nice defensive touch, though it isn't directly covered by a test (only the TypeError validation path and a well-behaved fake signal are tested, not one whose addEventListener itself throws). Minor, probably not worth a dedicated test given how contrived that input is.
• The TDS < 7.2 emulated-transaction bookkeeping fix (only updating transactionDepth/inTransaction if (!err)) is a genuine, separable bug fix — it now also protects against non-abort failures on that path, not just aborts. That's called out in "Known trade-offs" in the PR description, but might be worth its own sentence in the CHANGELOG/release notes since it's a behavior change independent of the AbortSignal feature.
• execBulkLoad passes the same options (and thus the same signal) into both the insert bulk execSqlBatch call and the subsequent makeRequest for the bulk load message. Since clearRequestAbortListener() runs at the completion of the first before the second's listener is armed, there's no double-listener risk — confirmed by reading the completion paths, good.
• errorForAbortedSignal's isNativeError fallback for cross-realm Errors is a nice touch that a plain instanceof Error check would miss (vm.runInNewContext-created errors, workers, etc.).

Test coverage

The 17 new tests in test/unit/request-abort-signal-test.ts are thorough — they cover both bulk-load phases, precedence ordering in both directions (abort-before-cancel, cancel-before-abort, timeout-before-abort, abort-before-timeout-fires), cross-realm/ponyfilled signals, listener leak prevention, and the emulated-transaction bookkeeping fix. This is a solid regression net for a feature this subtle (a lot of the value here is in not changing behavior on the many existing paths that don't pass a signal).

One gap worth considering: there's no test for a signal shared across multiple sequential executions of the same Request object (e.g. calling execSqlBatch(request, { signal }) twice with a fresh signal each time, per the prepare/execute flow called out in the PR description) — i.e. confirming a signal from execution 1 that later aborts has no effect on execution 2. Given requestAbortSignal is connection-scoped and cleared/reassigned per makeRequest call, this is very likely already correct, but it's the kind of aliasing bug that's easy to introduce in a future refactor (e.g. the planned async makeRequest rework) without a test catching it.

Security

No concerns — this is purely client-side cancellation plumbing; no new attack surface (no user input is trusted differently, no new network behavior beyond the existing attention/cancel path).

Performance

Negligible overhead (one duck-type check and one addEventListener/removeEventListener pair per execution when a signal is supplied; zero cost when omitted).

Minor nit

• makeRequest calls validateAbortSignal(signal) for both execBulkLoad's internal execSqlBatch call and its own makeRequest(bulkLoad, ...) call — a harmless double-validation of the same signal object, not worth changing given how cheap the check is.

Overall this looks ready to merge; nothing above is blocking.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 870d174b17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd2d511224

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

Re-traced the current head (870d174) independently — the AbortSignal plumbing (validateAbortSignal, errorForAbortedSignal, requestAbortSignal/_onRequestAbort/clearRequestAbortListener, and the SENT_CLIENT_REQUEST/SENT_ATTENTION completion paths) all line up with the "first failure cause wins" semantics described in the PR body. A few things I specifically checked given this has gone through several iterations already:

• Listener arm/mutate ordering in makeRequest: signal.addEventListener is called before this.request, isSqlBatch, or any timer state is touched, and since nothing awaits between that call and the already-aborted check earlier in the function, _onRequestAbort's this.request! is always populated by the time the listener could actually fire (abort is necessarily async once past the sync signal.aborted check).
• Precedence races: worked through timeout-fires-first vs abort-fires-first, manual-cancel-before-abort, and server-error-before-abort — all resolve correctly via the request.canceled early-return in _onRequestAbort plus the ??= write to request.error, and the reordered onErrorMessage (record error, then emit) in token/handler.ts closes the synchronous-listener race cleanly.
• Bulk load teardown: the new rowStream?.destroy(); bulkLoad.rowToPacketTransform.destroy(); on the insert bulk error path is a good generalization — it now cleans up the row pipeline for any failure of that statement, not just abort-triggered ones, which looks like a latent leak fix beyond the stated scope.
• Legacy (TDS < 7.2) transaction bookkeeping: gating transactionDepth/inTransaction updates on if (!err) is a genuine correctness fix independent of AbortSignal — worth a one-line mention in release notes since it's a behavior change on its own.
• Listener cleanup: every completion path (onEndOfMessage, the attention-ack handler, and cleanupConnection for socket errors/close) calls clearRequestAbortListener(), so a long-lived shared signal won't accumulate listeners — consistent with the test assertions using getEventListeners.

No new correctness, security, or performance issues found. Test coverage (fake-TDS-server based, not mocked internals) is thorough — already-aborted signals, cross-realm/ponyfilled signals, precedence ordering in both directions, bulk-load teardown at each phase, and the legacy transaction guard are all exercised.

The previously-raised nits (collapsing the SENT_ATTENTION branches, Connection#transaction() not yet taking a signal, per-method pass-through tests) were already addressed or explicitly declined with reasoning in the thread above, and I agree with those calls. Nothing here is blocking — this looks ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1122372fda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review: feat: add AbortSignal support to request execution

Overall this is a well-designed, carefully implemented feature. The trailing options?: { signal? } shape mirrors fetch(url, { signal }), the precedence rules (existing error/manual cancel/request timeout > abort reason > generic ECANCEL) are documented and consistently enforced, and the commit history shows real iterative hardening (listener leaks, cross-realm Errors, row-pipeline teardown, validation ordering) rather than a single drive-by patch. I read through src/connection.ts, src/request.ts, src/token/handler.ts, and skimmed the new test/unit/request-abort-signal-test.ts.

Correctness

• Traced all completion paths (onEndOfMessage, the attention-ack branch in SENT_ATTENTION, cleanupConnection, socketError) — clearRequestAbortListener() is called on every one of them, so a long-lived shared signal won't accumulate listeners. Good.
• The precedence logic checks out: _onRequestAbort bails early via request.canceled, makeRequest checks request.canceled before the already-aborted-signal branch, and onErrorMessage now records the server error before emitting errorMessage (closing a real race where a synchronous listener could abort the signal and have it win over an error whose token had already arrived). All of these have dedicated tests.
• errorForAbortedSignal's isNativeError fallback correctly handles cross-realm Errors that fail instanceof.
• Didn't find any logic bugs in the parts I traced.

Nitpicks (non-blocking)

1. Scope creep, worth calling out explicitly: the execBulkLoad fix that now does rowStream?.destroy(); bulkLoad.rowToPacketTransform.destroy(); when the insert bulk statement fails applies to any failure of that statement (e.g. the existing UNKNOWN/schema-mismatch error), not just abort-triggered ones. It looks like a legitimate pre-existing resource-leak fix, but it's a behavior change beyond "add AbortSignal support" and isn't mentioned in the PR's "Known trade-offs" section — might be worth a one-line callout so reviewers don't miss that it changes behavior on master today, independent of signals.
2. Comment slightly imprecise: the comment above signal.addEventListener(...) in makeRequest says the listener is armed "before any connection state is mutated," but request.error = undefined already runs a few lines earlier in the same function. Not a functional issue (that assignment is idempotent/harmless either way), just could be tightened to say "before any other state" or similar.
3. Minor inconsistency: execSql, execute, callProcedure, prepare, and execBulkLoad all call validateAbortSignal(options?.signal) up front (to win the synchronous-TypeError-vs-async-EPARAM/half-prepared-state race), but unprepare does not — it relies solely on makeRequest's internal validation. That's functionally fine today since unprepare has no side effects before makeRequest to protect against, but it stands out as the one sibling method without the explicit guard; a maintainer scanning the six methods for the pattern might wonder if it was missed.
4. API ergonomics: for beginTransaction/commitTransaction/rollbackTransaction/saveTransaction, options is a 4th positional parameter after callback, name, isolationLevel, so callers who just want a signal have to pass undefined for the params in between (as the new tests do: beginTransaction(cb, '', undefined, { signal })). Consistent with the existing style of the file, so not asking for a change — just flagging the ergonomics for anyone who hits it.

Test coverage

The new test file is thorough — 24 tests (more than the 17 mentioned in the PR description, possibly a stale count) covering: up-front TypeError validation for all affected entry points, already-aborted signals (request/transaction/bulk load, including that the row iterable is never consumed), non-Error and cross-realm Error abort reasons, abort-after-send via the attention/cancel path, abort during response streaming, AbortSignal.timeout() reason identity, full precedence-ordering matrices (abort vs. manual cancel vs. request timeout, in multiple orderings), each bulk-load phase, listener cleanup on both normal completion and close(), and duck-typed/ponyfilled signal acceptance. This is genuinely good coverage for a concurrency-sensitive feature like this.

Security / Performance

No concerns. No new external inputs are introduced; validation is synchronous and duck-typed input is only checked structurally ('aborted' in signal), not executed. The per-execution overhead (one addEventListener/removeEventListener pair) is negligible.

Nice work — the layering (mirroring requestTimer/cancelTimer with requestAbortSignal/_onRequestAbort) also reads as a sensible rehearsal for the planned async makeRequest rework mentioned in the description.

[claude]

Review

I went through src/connection.ts, src/request.ts, src/token/handler.ts, and the new test/unit/request-abort-signal-test.ts, plus traced the state machine (SENT_CLIENT_REQUEST / SENT_ATTENTION) and the request-timer/cancel interactions by hand. This is a well-designed, carefully iterated implementation (the commit history shows the author already hardened several precedence/race edge cases across review rounds). I could not run npm test/tsc/eslint in this sandbox (network/npm install is blocked here), so this is a static review — the PR description's reported "444 passing" numbers are not independently re-verified.

Correctness — looks solid

I traced the tricky precedence/race scenarios and they all check out:

• First-failure-cause-wins: request.canceled is checked before the already-aborted-signal check in makeRequest (connection.ts:3384-3396), and _onRequestAbort bails out early if request.canceled is already true (connection.ts:1868-1870) — so a manual cancel()/requestTimeout always wins over a later abort.
• Timer vs. abort race: _onRequestAbort clears the request timer before calling request.cancel() (connection.ts:1884-1886), so a slow attention ack can't let requestTimeout() overwrite the abort reason with ETIMEOUT afterwards. If the timer fires first, request.canceled is already true and _onRequestAbort short-circuits, so ETIMEOUT correctly stands.
• Server error vs. synchronous abort-from-listener: onErrorMessage in token/handler.ts now records request.error before emitting errorMessage (handler.ts:388-409), so a listener that calls controller.abort() synchronously from within that event still loses to the server error that arrived first. Good catch, and it's covered by a test (request-abort-signal-test.ts:1002).
• Listener leak / cleanup: clearRequestAbortListener() is invoked from every completion path I could find (cleanupConnection, onEndOfMessage, the SENT_ATTENTION ack handler), so a long-lived shared signal shouldn't accumulate listeners across many requests — confirmed by the getEventListeners assertion in the tests.
• Bulk load teardown: the row-stream/rowToPacketTransform teardown added for an already-aborted signal (never starting Readable.from(rows)) and for a failed insert bulk phase (destroying the row pipeline) both look correct, and are exercised by generator/async-generator tests that assert rows are never consumed.

Minor suggestions (non-blocking)

1. Duplicated inline options type. { signal?: AbortSignal } is repeated verbatim across all 12 public methods (execSql, execSqlBatch, execute, callProcedure, prepare, unprepare, execBulkLoad, the 4 transaction methods, and makeRequest). Pulling this into a single exported type (e.g. export type ExecutionOptions = { signal?: AbortSignal }) would remove the duplication and give consumers (especially TypeScript wrappers) a nameable type to reference instead of a repeated structural type.
2. Validation-order inconsistency for beginTransaction. execSql/execute/callProcedure/prepare/execBulkLoad were explicitly reworked (per the last two commits) so an invalid signal throws before parameter validation. beginTransaction, however, still runs assertValidIsolationLevel(isolationLevel, ...) before the signal is ever validated (validation happens later, inside execSqlBatch/makeRequest). Both are synchronous throws so there's no async/sync-ordering hazard like the ones fixed elsewhere, but if an invalid isolation level and an invalid signal are passed together, the isolation-level error wins rather than the (arguably more fundamental) TypeError on signal. Probably not worth a fix on its own, but worth being aware of for consistency with the stated contract ("an invalid signal throws synchronously... even when parameter validation would fail the request first").
3. execBulkLoad's public overload still requires rows. This predates this PR, but the new options parameter makes it slightly more visible: there's no exported overload for execBulkLoad(bulkLoad, options) (streaming mode + signal) — only the implementation signature accepts rows as optional. Not introduced by this PR, but since bulk-load streaming mode + abort signal is a plausible real use case, might be worth a follow-up overload.

Test coverage

The 17 new tests in request-abort-signal-test.ts are thorough — already-aborted signals (request/transaction/bulk load, including proof the row iterable is never consumed), cross-realm errors via runInNewContext, non-Error reasons, duck-typed/ponyfilled signals, abort-during-attention-exchange races via a hand-rolled fake TDS server, and the TDS < 7.2 legacy transaction-bookkeeping fix. This is a good level of rigor for protocol-level state-machine code where the interesting bugs are almost all in interleaving/ordering, not straight-line logic.

Security

No concerns — this is purely client-side cancellation plumbing; no new user input parsing, no new external I/O, and the RequestError/DOMException wrapping doesn't leak anything beyond what the caller's own abort reason already contains.

Overall: nice piece of work, small nits only.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2a7bb9ee0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Destroy the bulk row source when aborting the bulk message

When execBulkLoad is given an iterable and the signal aborts after the actual bulk-load message has started, this cancellation destroys the payload reader, which closes rowToPacketTransform, but it does not destroy the upstream rowStream. Node unpipes that source and leaves a backpressured generator paused without calling iterator.return(), so the bulk-load callback can complete while the row generator remains unfinalized and retains its resources. Tear down the row stream as well for this phase, as is already done when the preliminary insert bulk request fails.

Useful? React with 👍 / 👎.

[claude]

Review: AbortSignal support for request execution

I read through src/connection.ts, src/request.ts, src/token/handler.ts, and the new test/unit/request-abort-signal-test.ts (2101 lines / 24 tests), and traced the state-machine paths (SENT_CLIENT_REQUEST, SENT_ATTENTION, cleanupConnection, socket-error handlers) to check listener/timer cleanup on every completion path. Overall this is a very solid, carefully-reasoned PR — the precedence rules described in the PR body (server error / manual cancel / request timeout all beat the abort reason) are correctly implemented and each has a dedicated regression test using the fake-server pattern.

Code quality & design

• The requestAbortSignal / _onRequestAbort / clearRequestAbortListener() triad mirrors the existing requestTimer/cancelTimer pattern closely, which makes the change easy to follow for anyone already familiar with the cancellation code.
• validateAbortSignal duck-typing (matching Node core's validateAbortSignal) instead of instanceof AbortSignal is the right call for cross-realm/ponyfill support, and it's nicely exercised by the "fake signal" test that also asserts the listener add/remove pair.
• Good attention to where validation happens relative to state mutation (prepare, execBulkLoad validate before touching executionStarted/preparing; execSql/execute/callProcedure validate before parameter validation so the synchronous TypeError always wins). The commit history shows this was iterated on carefully.
• errorForAbortedSignal's use of util.types.isNativeError in addition to instanceof Error is a nice touch for cross-realm Error reasons (covered by the runInNewContext test).
• Recording the server error before emitting errorMessage (token/handler.ts) closes a real reentrancy race (listener aborts synchronously) — good catch, and it's covered by the dedicated test.

Potential issues / things worth a second look

1. Pre-existing SENT_ATTENTION quirk not fully closed by this PR. In the attention-ack handler, only ETIMEOUT and "aborted via the new signal" are special-cased to preserve sqlRequest.error; a manual request.cancel()/connection.cancel() issued after a server error was already recorded still gets overwritten with a generic ECANCEL (this existed on master before this PR — I diffed it to confirm). Not a regression, but it does mean the two cancellation mechanisms now behave inconsistently in that corner case (signal-based abort preserves the earlier error, imperative cancel doesn't). Might be worth a follow-up issue/PR for consistency, especially since the PR description leans on "first failure cause wins" as a general principle.
2. Trailing-options ergonomics for beginTransaction. Because isolationLevel has a default value ahead of options, callers must pass undefined explicitly to reach the signal (connection.beginTransaction(cb, '', undefined, { signal })), as the tests do. Not a bug, just a minor rough edge worth a mention in the docs/example snippet if not already there, since it's easy to get wrong silently (passing { signal } as isolationLevel wouldn't throw immediately in JS, though TS would catch it).
3. Double-checked the bulk-load two-phase flow (insert bulk SQL batch, then the bulk-load message) for signal reuse across the two makeRequest calls — the listener from phase 1 is guaranteed to be cleared (via clearRequestAbortListener in onEndOfMessage) before phase 2 arms a fresh one, and an abort during phase 1 short-circuits phase 2 entirely. This looks correct and matches the documented trade-off about bulkLoad.canceled not being set for phase-1 aborts.
4. Listener cleanup: I traced all this.request = undefined sites (cleanupConnection, both onEndOfMessage/onCancel-driven transitions, both socketError handlers) and clearRequestAbortListener() is invoked on every one of them (directly or via cleanupConnection), so I don't see a path that leaks the abort listener or leaves requestAbortSignal stale. The "signal isolation across sequential executions" test backs this up.

Performance

No concerns — one addEventListener/removeEventListener pair per request execution, only when a signal is actually passed. Negligible relative to a network round trip.

Security

Nothing concerning. Non-Error abort reasons are wrapped via RequestError's cause option rather than being interpolated into anything executable/loggable in a dangerous way, and the duck-typed validation still guards against non-object garbage being treated as a signal.

Test coverage

Excellent — the 24 tests cover already-aborted signals (request/transaction/bulk load, including that the row generator is never consumed), abort ordering against server errors/timeouts/manual cancels in both pre-execution and in-flight positions, all three bulk-load phases, TDS < 7.2 legacy transaction bookkeeping, cross-realm signals/errors, and listener cleanup/isolation. This is thorough enough that I'd trust it to catch regressions in the cancellation machinery going forward, not just in the new signal path.

Nice work — nothing here blocks merging from my read; the two items above are worth a comment/decision but not necessarily changes to this PR.