fix: only update emulated transaction state when the statement succeeds

[arthurschreiber]

Summary

With TDS versions below 7.2, transactions are emulated with SQL batches and client-side transactionDepth/inTransaction bookkeeping. That bookkeeping was updated unconditionally: a BEGIN TRAN/COMMIT TRAN/ROLLBACK TRAN/SAVE TRAN batch that failed — with a server error, or before ever being sent (EINVALIDSTATE, ECANCEL, ECLOSE) — still incremented or decremented the counters, leaving the client-side transaction state out of sync with the server.

This guards the four legacy-path callbacks with if (!err), so the counters only move when the statement actually succeeded.

Extracted from #1766, where it was flagged by review as an independent, pre-existing bug fix that deserves its own release entry rather than shipping silently inside a feature. #1766 will be rebased to drop its copy of these hunks once this merges.

What is deliberately unchanged

The pre-existing rule in the end-of-message handling that a failed SQL batch clears inTransaction on these TDS versions (mirroring the server aborting the transaction on a batch error) is left as is — this PR only fixes the counter updates for statements that failed.

Testing

New test/unit/transaction-state-test.ts using the fake-server pattern, negotiating TDS 7.1 (with the pre-7.2 wire formats — 32-bit DONE row count, 16-bit ERROR line number):

• a BEGIN TRAN that fails with a server error leaves transactionDepth/inTransaction untouched,
• a successful BEGIN TRAN updates them,
• a COMMIT TRAN rejected with EINVALIDSTATE (attempted while another request is in flight, so it fails before anything is sent) leaves them untouched.

Verified the test fails without the guards.

• npm test: 433 passing, 0 failing
• npx eslint src test + tsc: clean

🤖 Generated with Claude Code

---

Generated by 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-31T13:01:07.995197Z	fbb91ce	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

This is a clean, well-scoped fix. Verified the diff against the surrounding code in src/connection.ts:

Correctness

• The if (!err) guards in beginTransaction, commitTransaction, rollbackTransaction, and saveTransaction correctly prevent the counter bookkeeping from running when the emulated-transaction batch fails, including the pre-send failure paths (EINVALIDSTATE at connection.ts:3234, ECANCEL at connection.ts:3237/3259/3900, ECLOSE at connection.ts:2192) — all of these invoke request.callback(err) directly, which is the same callback wrapped here, so the guard applies uniformly regardless of whether the request was ever sent.
• Double-checked that the pre-existing unconditional inTransaction = false on a failed SQL batch (connection.ts:3835, if (... && sqlRequest.error && this.isSqlBatch)) runs before these per-call callbacks fire, so it's not in conflict with the new guards — it was already forcing inTransaction false on any batch error independent of transactionDepth, and that behavior is unchanged here.

One edge case worth a mention (not new to this PR, but sharpened by it)
For nested emulated transactions (transactionDepth > 1), a failed COMMIT TRAN/ROLLBACK TRAN now leaves transactionDepth completely untouched, while connection.ts:3835 still unconditionally clears inTransaction to false on any SQL batch error. Since the server aborts the entire transaction on a batch error (not just one nesting level), the "correct" post-failure state would arguably be transactionDepth = 0 alongside inTransaction = false, rather than leaving a stale nonzero depth. This mismatch existed before this PR too (the old code decremented depth by only 1 regardless of nesting), so this change doesn't make it worse and is explicitly called out as out-of-scope in the PR description — just flagging it as a pre-existing gap that might be worth a follow-up if nested legacy transactions are a supported use case.

Tests

• test/unit/transaction-state-test.ts is a solid addition: it uses the fake-server pattern to negotiate TDS 7.1, and exercises both a server-side error (BEGIN TRAN failing with an ERROR token) and a pre-send failure (COMMIT TRAN rejected with EINVALIDSTATE because another request is in flight). Assertions on both transactionDepth and inTransaction for each case give good coverage of the fix.
• Minor nit: the unused beginTransaction/inner-beginTransaction success callback chain isn't asserting connection.transactionDepth/inTransaction after the final commitTransaction failure resolves (the test ends via request's callback closing the connection instead) — harmless since the assertions happen synchronously inside the commitTransaction callback, but worth double-checking test cleanup doesn't mask an unhandled rejection if commitTransaction's callback throws.

Style/scope

• Change is minimal and consistent with the existing code style (no new abstractions, matches the pattern used elsewhere in the file).
• Good call extracting this from feat: add AbortSignal support to request execution #1766 as an independent, reviewable fix with its own changelog-worthy entry.

No security or performance concerns — this only touches in-memory counter bookkeeping on the error path of a pre-existing legacy code path.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c63727eb52

ℹ️ 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 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.81%. Comparing base (760e96d) to head (fbb91ce).

Files with missing lines	Patch %	Lines
src/connection.ts	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
+ Coverage   80.71%   80.81%   +0.10%     
==========================================
  Files          90       90              
  Lines        4880     4885       +5     
  Branches      927      931       +4     
==========================================
+ Hits         3939     3948       +9     
+ Misses        643      638       -5     
- Partials      298      299       +1     

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

Review

Solid, well-scoped fix. The core change — guarding the four legacy-path (TDS < 7.2) transaction bookkeeping updates with if (!err) — correctly addresses the client/server state divergence described in the PR body, and the reasoning in "What is deliberately unchanged" is sound: a failed batch already clears inTransaction in the end-of-message handler, so adding transactionDepth = 0 there (2nd commit) is the right complement rather than duplicating the reset in each callback.

I traced the interaction between the two commits to be sure they compose correctly: for commitTransaction/rollbackTransaction/beginTransaction/saveTransaction, onEndOfMessage runs its reset (inTransaction = false; transactionDepth = 0) before invoking sqlRequest.callback, so by the time the guarded if (!err) { … } block in each transaction method runs, the state has already been zeroed and the guard correctly leaves it that way. No double-decrement/race issue there.

Possible follow-up (out of scope, but same bug class)

reset() (src/connection.ts:3336-3341) has the identical inconsistency this PR just fixed in the end-of-message handler: it unconditionally sets inTransaction = false on TDS < 7.2 but never resets transactionDepth. Since reset() triggers a server-side connection reset (resetConnectionOnNextRequest = true), which implicitly rolls back any active transaction, transactionDepth should presumably be zeroed alongside inTransaction here too — otherwise a reset() after a nested transaction leaves inTransaction === false with a stale positive transactionDepth, the exact inconsistent state the second commit exists to prevent elsewhere. Might be worth a tiny follow-up PR (or note in this one) rather than blocking this fix.

Test coverage

The new test/unit/transaction-state-test.ts is thorough and exercises real wire-level scenarios (server error via ERROR token, and pre-send failure via EINVALIDSTATE while a request is in flight) rather than mocking internals — good use of the fake-server pattern already established in this test directory (mirrors connection-failure-test.ts/rerouting-test.ts). Confirmed the TDS 7.1 negotiation and 32-bit DONE/16-bit ERROR line-number wire formats match the pre-7.2 spec differences called out in the PR description. One minor nit: the single it(...) block chains four scenarios sequentially with nested callbacks — splitting into separate it blocks (or at least intermediate assertions with clearer comments per stage) would make a future failure easier to localize, but this is a style preference, not a blocker.

Nits

• The added comment at src/connection.ts:3834-3837 is genuinely useful (explains the why, not the what) — good adherence to keeping comments non-obvious/justified.
• No security or performance concerns; this is pure client-side bookkeeping affecting only the legacy (pre-7.2) code path, no I/O or new surface area introduced.

Overall: correct, well-tested, appropriately narrow in scope. Approve modulo the optional reset() follow-up noted above.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbb91ce520

ℹ️ 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]

Superseded by #1768: rather than hardening the TDS 7.1 emulated transaction bookkeeping against cancellation races that are fundamentally unresolvable client-side (see the review threads), TDS 7.1 is now deprecated, and support - including this entire code path - will be removed in the next major version.

Two pieces of this work survive elsewhere:

• The success-only guards on the transaction callbacks also land via feat: add AbortSignal support to request execution #1766, where they keep aborted transaction statements from corrupting the emulated state for the remainder of v20's lifetime.
• Clearing transactionDepth/inTransaction when the connection dies is protocol-version-independent and will be proposed as its own small PR.

---

Generated by Claude Code