Skip to content

fix: report a request cancelled twice on msnodesqlv8 6 as cancelled - #1947

Draft
dhensby wants to merge 3 commits into
tediousjs:masterfrom
dhensby:fix/msnodesqlv8-v6-repeat-cancel
Draft

dhensby wants to merge 3 commits into
tediousjs:masterfrom
dhensby:fix/msnodesqlv8-v6-repeat-cancel

Conversation

@dhensby

@dhensby dhensby commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #1938. The first two commits on this branch are #1938's. This PR uses #1938's msnodesqlv8 version check, and GitHub can't base a PR here on a fork branch. This PR's own change is the last commit. Once #1938 is in, I'll rebase it onto master.

What goes wrong

msnodesqlv8 6 takes one cancel per query. It refuses any other in its callback (cannot cancel query (cancel already requested)), while the one it took goes on to end the query: HY008, then done, then free.

The adapter treats a refused cancel as a query that runs on, which is true before 6. A cancel refused by msnodesqlv8 < 5.6 means the query keeps running. So when cancel() is called twice before msnodesqlv8 has the query's statement, the second cancel's refusal undoes the first:

  • A promise or callback request: the query's HY008 is no longer dropped. The request is rejected with EREQUEST "Operation canceled" instead of ECANCEL.
  • A paused stream: it completes as if it had succeeded. That completion waits behind the pause, so the stream never ends unless something resumes it.

The window is short, but calling cancel() twice in a row is enough to reach it.

The fix

With msnodesqlv8 6, the request sends msnodesqlv8 one cancel per query, and the outcome of that one stands. A later cancel() does nothing: msnodesqlv8 would refuse it anyway.

Earlier versions are unchanged. There, a cancel refused while the query was flowing has to leave room for another once the stream is paused (should report a cancel of a paused stream although an earlier cancel was refused).

To share the version check, #1938's driverHoldsPausedQueries now reads the major version through a driverMajorVersion helper.

Tests

Unit tests (test/common/unit.js), with a mock that takes one cancel and refuses the next the way 6.1 does:

  • A request cancelled twice is rejected with ECANCEL, and msnodesqlv8 is sent one cancel.
  • A paused stream cancelled twice ends with ECANCEL and completes without being resumed.

Both fail without the fix. One mutant applies the one-cancel rule to every version, and an existing 5.x test fails on it, so the version gate is needed.

Integration test: cancelling a request twice as soon as it is sent has a promise and a paused-stream variant. Each runs on both drivers on a pool of one connection, and checks that the next query runs. msnodesqlv8 < 5.6 cancels only a paused query, so there the promise variant expects the request to run to the end, as cancel a running request already does.

With tedious, a paused stream cancelled just as its batch starts gets SQL Server's "the batch is aborted" error before the ECANCEL, even with a single cancel. That was already so before this change. The test therefore checks that the request ends with exactly one ECANCEL.

Live results (Windows CI, SQL Server 2022)

These ran on a throwaway fork branch, since deleted. The two new integration tests ran three times without the fix (#1938's adapter) and three times with it. The full msnodesqlv8 suite then ran with the fix.

msnodesqlv8 without the fix with the fix full suite with the fix
^2 (node 18) 3 passed 3 passed 164 passing
^3 (node 20) 3 passed 3 passed 161 passing
^4 (node 22) 3 passed 3 passed 161 passing
5.2.1 (node 20) 3 passed 3 passed 161 passing
^5 (5.6.0, node 22) 3 passed 3 passed 161 passing
6.2.0 (node 22) 3 failed: the request rejected with EREQUEST, and the paused stream never completed 3 passed 161 passing

Earlier probes found the same failure on 6.0.0, 6.1.0 and 6.2.0, in three runs out of three each. They covered master's adapter, and #1938 with #1928 on top.

Local checks

  • npm run lint: clean.
  • npm run test-unit: 290 passing.
  • test-tedious against Azure SQL Edge: 182 passing. The 3 failures are the CLR-dependent tests that always fail on SQL Edge.

Part of #1940.

🤖 Generated with Claude Code

dhensby and others added 3 commits October 25, 2026 22:33
…sult set is cancelled

A stream paused before its query reaches a result set, as
toReadableStream() leaves one until it is read, holds msnodesqlv8's
query paused. The query still runs the statements ahead of that result
set, which have none, and msnodesqlv8 cancels a paused query by freeing
its statement. Once the server has sent the row count of one of those
statements, msnodesqlv8 is reading on to the next result when that free
lands, which can crash the process. It did so in nearly every run with
each msnodesqlv8 version tried, 2.7 to 5.6.0.

A held query is now cancelled once it stops at its first result set,
where it has nothing under way, and one that ends first completes as
cancelled. Such a cancel no longer cuts short the statements ahead of
that result set: it takes effect when they end, or when the request
times out. With no request timeout, nothing limits the wait.

msnodesqlv8 6 fixes the crash itself, and emits nothing for a paused
query until it is resumed, so there a held query is cancelled at once,
as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
msnodesqlv8 6 requires Node 22 or later, and ships a Node-API binary for
win32-x64, so the Windows jobs run its tests on Node 22 and 24 after the
v5 run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
msnodesqlv8 6 takes one cancel per query. It refuses any other in its
callback, while the one it took goes on to end the query. The request
took a refused cancel to mean that the query was running on, as it is
with earlier versions. So a second cancel() made before msnodesqlv8 had
the query's statement undid the first one. The request then failed
with 'Operation canceled' rather than ECANCEL, and a paused stream never
completed.

With msnodesqlv8 6 the request now sends one cancel per query, and the
outcome of that one stands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant