From c0cd76748244fc85ccb39dedb727297645c4fe12 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Thu, 22 Oct 2026 21:46:34 +0100 Subject: [PATCH] fix: an accepted cancel calls back before the query's HY008, done and free The cancel's own callback and the HY008 from the native call in flight are two native completions on different threads, so either could reach JS first: `error HY008` (and on #479's script even `done`) arrived before `cancel-cb ok`. CI on Linux hit it on most runs, on cancel/again-after-confirm-at-100, cancel/twice-streaming-at-100, cancel/last-row and scripts/issue-479. - notifier: a cancel that has been sent but not called back is pending. Its callback is wrapped to settle it, and what waits on it is released a tick on, after anything the callback queued (e.g. a second cancel's refusal). A cancelQuery that throws settles at once, so nothing is held for good. - reader: what begin, dispatch and nextResult get back from native runs through afterCancel, so it waits for the cancel's callback. With no cancel pending it runs at once, as before. The native cancel worker calls back without waiting on JS (it either calls SQLCancelHandle or sets the poll loop's flag), so holding cannot hang. The two corpus files that accepted either order (`--- or`) now accept only cancel-cb, HY008, done, free. Co-Authored-By: Claude Opus 5.5 --- lib/notifier.js | 45 ++++++++++++++++++- lib/reader.js | 28 +++++++----- test/contract/HANDOVER.md | 21 +++++---- .../cancel/after-error-mid-batch.events.txt | 9 ---- .../cancel/twice-streaming-at-100.events.txt | 11 ----- 5 files changed, 72 insertions(+), 42 deletions(-) diff --git a/lib/notifier.js b/lib/notifier.js index a108a553..fe2bf1ff 100755 --- a/lib/notifier.js +++ b/lib/notifier.js @@ -83,6 +83,9 @@ const notifyModule = ((() => { this.operation = null this.paused = null this.canelSent = false + // a cancel sent whose own callback has not run yet, and what waits for it to run + this.cancelPending = false + this.afterCancelSteps = [] this.prepared = null this.handle = null this.cancelToken = null @@ -181,7 +184,26 @@ const notifyModule = ((() => { logger.debugLazy(() => `${this.queryId} issue cancel request`) this.cancelToken = null this.canelSent = true - this.theConnection.cancelQuery(this, cb) + this.cancelPending = true + // the cancel's callback and the HY008 from the native call in flight complete on + // different threads: the reader holds what that call brings back until this has run + const settled = (e, res) => { + try { + if (cb) { + cb(e, res) + } else if (e) { + this.theConnection.defaultCallback(e) + } + } finally { + this.settleCancel() + } + } + try { + this.theConnection.cancelQuery(this, settled) + } catch (e) { + this.settleCancel() + throw e + } } else { logger.debugLazy(() => `${this.queryId} cancel request but no statement handle`) this.cancelToken = { @@ -195,6 +217,27 @@ const notifyModule = ((() => { } } + // run a step now, or once the cancel in flight has called back + whenCancelSettled (step) { + if (this.cancelPending) { + this.afterCancelSteps.push(step) + } else { + step() + } + } + + // a tick on, so that work the cancel's callback queued (e.g. the refusal of a second + // cancel) still comes first + settleCancel () { + if (!this.cancelPending) return + this.cancelPending = false + const steps = this.afterCancelSteps + this.afterCancelSteps = [] + if (steps.length > 0) { + setImmediate(() => steps.forEach(step => step())) + } + } + pauseQuery () { this.paused = true if (this.queryWorker) { diff --git a/lib/reader.js b/lib/reader.js index b0174666..d508e61f 100755 --- a/lib/reader.js +++ b/lib/reader.js @@ -425,6 +425,12 @@ class Query extends BasePromises { }) } + // what a native call brings back once a cancel has been sent waits for the cancel's own + // callback: an accepted cancel is always cancel-cb, error HY008, done, free + afterCancel (step) { + this.notify.whenCancelSettled(step) + } + // park a step to run on resume, if paused. true when parked - or, for a paused query // that has been cancelled, when the query has ended instead. park (step) { @@ -468,15 +474,15 @@ class Query extends BasePromises { if (this.park(() => this.dispatch())) return logger.traceLazy(() => `dispatch fetching rows for queryId ${this.queryId}, rowBatchSize=${this.rowBatchSize}`, this.context) - this.nativeGetRows(this.queryId, this.rowBatchSize).then(d => { + this.nativeGetRows(this.queryId, this.rowBatchSize).then(d => this.afterCancel(() => { logger.traceLazy(() => `dispatch received ${d?.data?.length || 0} rows for queryId ${this.queryId}, endOfRows=${d?.endOfRows} endOfResults=${d?.endOfResults}`, this.context) this.batchRowIndex = 0 this.batchData = d this.continueBatch() - }).catch(err => { + })).catch(err => this.afterCancel(() => { logger.debugLazy(() => `dispatch error for queryId ${this.queryId}: ${err}`, 'DriverRead.dispatch', this.context) this.endOrPark(err) - }) + })) } // emit what is left of the current batch, then fetch more, or move to the next result. @@ -507,11 +513,11 @@ class Query extends BasePromises { if (this.park(() => this.nextResult())) return this.infoFromNextResult = false this.nativeNextResult(this.queryId) - .then(nextResultSetInfo => { + .then(nextResultSetInfo => this.afterCancel(() => { this.moveToNextResult(nextResultSetInfo) - }).catch(err => { + })).catch(err => this.afterCancel(() => { this.endOrPark(err) - }) + })) } // the query has executed: report what came with it, then read on @@ -546,9 +552,11 @@ class Query extends BasePromises { // the handle at once, so a cancel can reach the statement; what is emitted waits // for a resume, if the query was paused while it executed this.notify.setHandle(res.queryResult.handle) - const begun = () => this.begun(res) - if (!this.park(begun)) begun() - }).catch(err => { + this.afterCancel(() => { + const begun = () => this.begun(res) + if (!this.park(begun)) begun() + }) + }).catch(err => this.afterCancel(() => { logger.debugLazy(() => `dispatch error for queryId ${this.queryId}: ${err}`, this.context) // Don't call end() here - it's already handled in beginQuery for binding errors try { @@ -567,7 +575,7 @@ class Query extends BasePromises { this.close() } } - }) + })) this.notify.emit('submitted', this.query, this.params) } diff --git a/test/contract/HANDOVER.md b/test/contract/HANDOVER.md index 1f486917..9c68399d 100644 --- a/test/contract/HANDOVER.md +++ b/test/contract/HANDOVER.md @@ -123,8 +123,8 @@ ODBC and has the same blind spot, so it is not a reference). - `QueryWorker` (direct execute) still calls back `(null, meta)` on success; info at execute reaches JS because `try_execute_direct` returns false on `SQL_SUCCESS_WITH_INFO`. Works, but it is not the same shape as the other workers. -- `cancel/after-error-mid-batch` shows the cancel-cb / HY008 race (about 1 in 10, on master's - native code too): it has an `--- or` alternative now. +- `cancel/after-error-mid-batch` showed the cancel-cb / HY008 race (about 1 in 10, on master's + native code too). Fixed since: see the cancel race under Smaller items. **To review in the `info/` transcripts** (all `unreviewed`): - each PRINT, and each statement with no result (a DDL), is followed by `rowcount 0`; an insert @@ -354,15 +354,14 @@ that replay covers them. - #483 (Linux, native) on WSL. When it is fixed, the `known-bug` Linux pins show as diffs: `errors/{raiserror-then-result,dup-key-d}.*.linux.txt`, `scripts/issue-478.linux.txt`, `cancel/after-error-mid-batch.events.linux.txt`. -- A race in accepted cancels: the cancel's own callback (`cancel-cb ok`) and the HY008 from the - fetch in flight are two native completions on different threads, so either can reach JS - first. CI showed it on `cancel/twice-streaming-at-100`, which now has an `--- or` - alternative (the first in the corpus). `cancel/again-after-confirm-at-100` failed once - locally, probably the same race, but its transcript was not kept. Other accepted cancels - of a statement in flight can show it too: add the alternative when one does. Or tighten the - contract: hold the HY008 in JS until the cancel callback has run, so that `cancel-cb ok` - always comes first. That is the owner's call. Note that `capture.js --update` rewrites a file - from what it saw, so it drops an alternative it did not see. +- ~~A race in accepted cancels~~, fixed (2026-10-22). The cancel's own callback (`cancel-cb ok`) + and the HY008 from the fetch in flight are two native completions on different threads, so + either could reach JS first. CI showed it on `cancel/twice-streaming-at-100`, + `cancel/again-after-confirm-at-100` and `scripts/issue-479`. Now, once a cancel is sent, + `StreamEvents` holds what a native call brings back to the reader (`afterCancel` in + `begin`, `dispatch`, `nextResult`) until the cancel's callback has run, then releases it a + tick on, after anything that callback queued. An accepted cancel is always `cancel-cb ok`, + `error HY008`, `done`, `free`, and the corpus no longer has `--- or` alternatives for it. - The 36 `unreviewed` transcripts (listed under Where we are): read them against the contract and agree them. `read/print-*` belong with the `info/` work above. diff --git a/test/contract/corpus/cancel/after-error-mid-batch.events.txt b/test/contract/corpus/cancel/after-error-mid-batch.events.txt index 83af1044..b8e9ff15 100644 --- a/test/contract/corpus/cancel/after-error-mid-batch.events.txt +++ b/test/contract/corpus/cancel/after-error-mid-batch.events.txt @@ -12,12 +12,3 @@ error HY008 0 Operation canceled more=false done free check ok ---- or -submitted -error 42000 50000 boom more=true -action cancel -error HY008 0 Operation canceled more=false -cancel-cb ok -done -free -check ok diff --git a/test/contract/corpus/cancel/twice-streaming-at-100.events.txt b/test/contract/corpus/cancel/twice-streaming-at-100.events.txt index 20722168..f9798111 100644 --- a/test/contract/corpus/cancel/twice-streaming-at-100.events.txt +++ b/test/contract/corpus/cancel/twice-streaming-at-100.events.txt @@ -14,14 +14,3 @@ error HY008 0 Operation canceled more=false done free check ok ---- or -submitted -meta name -row x100 -action cancel x2 -cancel-cb error cannot cancel query (cancel already requested) id 0 -error HY008 0 Operation canceled more=false -cancel-cb ok -done -free -check ok