[dhensby]

Stacked PR

This builds on the msnodesqlv8 stream-completion work, which has no PR of its own. The first two commits are that work:

• test: read the test config afresh for each test
• fix: complete msnodesqlv8 streams when an error ends the batch

Please review the last commit: fix: stop msnodesqlv8 streams resumed after they end from hanging the connection.

The branch is rebased onto master, which now includes #1918 and #1919, and the fix was rebuilt on #1919's design. The stream-completion commit also runs #1918's query with pipe and an error tests on every msnodesqlv8 version. They had been skipped because a stream never completed after an error.

Problem

msnodesqlv8 does not check that a query is still running when it is paused, resumed or cancelled. Its work queue implements park/resume as remove + add, so pausing and then resuming a query that has ended, or resuming one that ended while it was paused, puts the dead work item back at the head of the connection's queue. The next query on that pooled connection never runs. From 5.1.9, resume() also re-runs the query's completion: it loops on rowcount events, releases the statement twice, and has segfaulted the process.

#1919 already stops a stream from pausing or resuming its query once msnodesqlv8 reports 'done'. The stream-completion commit makes 'free' do the same. Before that report, the request could still reach an ended query:

• while the errors that end a batch are emitted, which msnodesqlv8 >= 5.1 does before it releases the statement;
• between such an error and 'free'. A stream paused before its first result set (as toReadableStream() is) and read on after the error is one example;
• in the few ticks between the query's reader stopping and msnodesqlv8 reporting the end;
• on a cancel in any of those windows.

Fix

• The request leaves the query alone once msnodesqlv8 reports that it has ended. That means 'done', 'free', an error with more=false, or msnodesqlv8 2.0's statement-terminated message. Nothing it emits from then on can reach the query.
• It takes the query up again if msnodesqlv8 carries on to a result set after an error it said had nothing more to follow.
• A cancel made once the query looks to have ended is held. It is sent if msnodesqlv8 carries on to a result set after all. Once the query is through, a paused stream completes as cancelled without waiting to be resumed, and anything else completes normally.
• While an error that is not the last of its set is handled, the query is set aside. A cancel made then waits until the error has been handled. Afterwards the query is paused if the stream was paused before its first result set, unless it is being cancelled.
• From msnodesqlv8 3, the request leaves the query alone once its reader has stopped.

Evidence

All live runs used Windows CI, SQL Server 2022 and msnodesqlv8 2.7.0, 3.1.0, 4.5.0, 5.2.1 and 5.6.0, from a throwaway branch.

Before #1919, on the old parent:

• Resuming after a stream ended hung the next query on every version, and looped or segfaulted on 5.2.1/5.6.0.
• The fixed adapter passed all 20 probe scenarios on all five versions.

On current master:

• Error-path scenarios: master hangs in every one, and the stream-completion commit fixes all of them.
• The other scenarios: master + stream completion and the fixed adapter both pass every one tried. That covers 23 scenarios, plus a 0–16 ms resume-timing sweep on a stream paused before it runs.
• Where the fix adds value: it stops pause, resume and cancel from reaching an ended query in the windows above. Live, the parent sent two driver calls to an ended query when a paused stream was cancelled after its error (3.1–5.6), and the fix sent none.
• Where the windows are only reproduced off the live server: the rest are reproduced with msnodesqlv8's real JS over a fake native layer, and are pinned by unit tests.

Tests:

• Unit: the new tests in the msnodesqlv8 query events block each fail without the guard they target. All 17 mutations of the guards fail at least one, and npm run test-unit passes (215).
• Integration: pausing and resuming a stream once it is over (completed, failed, cancelled), wired for both drivers. On today's master it is a regression test: fix: stop msnodesqlv8 streams losing rows under back pressure #1919 together with the stream-completion commit already pass it.
• Local: npm run lint. npm run test-tedious has 175 passing; the 3 failures are CLR tests that fail on Azure SQL Edge and also fail on master.

Known, unchanged

• On msnodesqlv8 5.6, resuming a stream that was paused before it ran while its query is still executing can add a spurious HY010 Function sequence error. The next query still runs. This happens the same way with or without this PR.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The documented msnodesqlv8 2.0 termination sequence can still leave streams incomplete and reattach an ended query.

1 open finding

• Prevent reattaching requests after settlement · New

What changed in this PR

Fixes msnodesqlv8 stream completion and prevents ended queries from being resumed onto the connection queue.

Changes:

• Detaches completed queries and guards pause/resume/cancel handling.
• Completes streams when errors end a batch.
• Adds extensive unit and cross-driver integration coverage.

File	Description
lib/​msnodesqlv8/​request.js	Implements query completion and detachment guards.
test/​common/​unit.js	Adds msnodesqlv8 event-sequence tests.
test/​common/​tests.js	Adds shared streaming integration scenarios.
test/​msnodesqlv8/​msnodesqlv8.js	Enables new msnodesqlv8 tests.
test/​tedious/​tedious.js	Enables equivalent tedious tests.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Cancellation and termination-event paths can still invoke driver operations on ended or temporarily isolated queries.

1 open finding

• Defer cancellation during synchronous driver error dispatch · New

1 resolved since last review

• Prevent reattaching requests after settlement

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Cancellation can still reach an ended query after its reader stops but before completion events arrive.

1 open finding

• Guard cancellation when query worker has stopped · New

1 resolved since last review

• Defer cancellation during synchronous driver error dispatch

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

A cancellation can be silently lost when metadata reattaches a query after a provisional terminal error.

0 open findings

1 resolved since last review

• Guard cancellation when query worker has stopped

Previously missed (1)

In code that hasn't changed since last review

Preserve cancellation requested while query is provisionally detached

lib/​msnodesqlv8/​request.js:675

Reattaching the query here drops a cancellation made from this error's listener. While detached is true, cancel() returns without recording anything for an unpaused stream; if meta then proves that the driver carried on after the more=false error, the remaining statements execute despite the caller's request.cancel(). Preserve a cancel requested during this provisional-detach window and replay it when metadata makes the query reachable again, while retaining the no-op behavior when free/done confirms that it really ended.

🧠 Review effort: Balanced

[github-actions]

🎉 This PR is included in version 12.7.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀