fix: support the msnodesqlv8 5.6 error, cancel and info events

.github/copilot-instructions.md

@@ -64,6 +64,7 @@ Driver index files use `Object.assign()` to merge base exports with driver-speci
 ### Test patterns
 
 - **Unit tests** (`test/common/unit.js`): No database needed. Tests type mapping, table parsing, error construction.
+- **msnodesqlv8 event stubs**: the `msnodesqlv8 query events` block at the end of `test/common/unit.js` stands a bare object in for `require('msnodesqlv8')` and plays the events a given msnodesqlv8 version emits. The msnodesqlv8 integration suite only runs on Windows CI, so extend these stubs for any change to how `lib/msnodesqlv8/request.js` handles driver events.
 - **Integration tests** (`test/common/tests.js`): Exported as a factory `(sql, driver) => { ... }` — called by each driver's test file. Many tests exercise three execution styles: callback, promise, and streaming.
 - **Framework**: Mocha + Node.js built-in `assert` (no chai/sinon).
 - **Keeping this file up to date:** If a change affects the architecture, conventions, build process, or any other information documented here, update this file as part of the same PR.

.github/workflows/nodejs.yml

@@ -165,11 +165,12 @@ jobs:
       #   v2: Node 18    v3: Node 18-20    v4: Node 18-24    v5: Node 20-24
       #
       # v5 is split: msnodesqlv8 5.2.2 dropped the Node 20 (ABI 115) win32-x64
-      # prebuild, so `@^5` (5.2.3+) is gated to Node 22-24 and Node 20 is pinned
-      # to 5.2.1 — the last v5 release that ships one. Upstream now builds
-      # prebuilds for Node 22/24/26 only, so a floating `@^5` will not regain a
-      # Node 20 prebuild; leaving Node 20 on `@^5` source-builds via node-gyp,
-      # which is slow on windows-2022 and fails outright on windows-2025.
+      # prebuild, so `@^5` is gated to Node 22-24 and Node 20 is pinned to 5.2.1,
+      # the last release before that drop. A combination without a prebuild
+      # source-builds via node-gyp, which is slow on windows-2022 and fails
+      # outright on windows-2025. From 5.3.0 msnodesqlv8 ships a single Node-API
+      # binary instead of one per ABI, which may load on Node 20 again but has not
+      # been tried here.
       - name: Install msnodesqlv8 v2
         if: ${{ matrix.node == '18.x' }}
         run: npm install --no-save msnodesqlv8@^2

README.md

@@ -1246,6 +1246,10 @@ request.query('waitfor delay \'00:00:05\'; select 1 as number', (err, result) =>
 request.cancel()
 ```
 
+A cancelled stream emits an `error` event with code `ECANCEL`, followed by `done`.
+
+With the `msnodesqlv8` driver, a request that is already running can only be cancelled from msnodesqlv8 5.6. Earlier versions can cancel a paused stream, but let any other running request run to the end and complete as if `cancel()` had not been called.
+
 ## Transaction
 
 **IMPORTANT:** always use `Transaction` class to create transactions - it ensures that all your requests are executed on one connection. Once you call `begin`, a single connection is acquired from the connection pool and all subsequent requests (initialized with the `Transaction` object) are executed exclusively on this connection. After you call `commit` or `rollback`, connection is then released back to the connection pool.

lib/msnodesqlv8/request.js

@@ -334,13 +334,22 @@ class Request extends BaseRequest {
       let handleOutput = false
       let isChunkedRecordset = false
       let chunksBuffer = null
+      let terminated = null
+      // set while the error just collected is being dispatched, along with whatever came with it
+      let failing = false
+      let cancelRequested = false
+      // 'done' ends the request. msnodesqlv8 >= 5.6 emits it as soon as a cancel is through, and
+      // the query can go on emitting after it, so the listeners ignore anything that follows
+      let finished = false
 
       const handleError = (req, connection, info, moreErrors) => {
-        const doReturn = !moreErrors
-        if ((typeof info.sqlstate === 'string') && (info.sqlstate.toLowerCase() === '08s01')) {
-          connection.hasError = true
+        // a cancel is reported from 'done'. msnodesqlv8 >= 5.6 also sends an 'Operation canceled'
+        // error after a cancel, and one that arrives before 'done' is dropped here
+        if (cancelRequested && info.sqlstate === 'HY008') {
+          return
         }
 
+        const doReturn = !moreErrors
         const err = new RequestError(info, 'EREQUEST')
         err.code = 'EREQUEST'
 
@@ -360,13 +369,8 @@ class Request extends BaseRequest {
               debug('request(%d): failed', IDS.get(this), err)
             }
 
-            let previous
             if (errors.length) {
-              previous = errors.pop()
-              if (!err.precedingErrors) {
-                err.precedingErrors = []
-              }
-              err.precedingErrors.push(previous)
+              err.precedingErrors = errors.splice(0)
             }
 
             hasReturned = true
@@ -376,6 +380,10 @@ class Request extends BaseRequest {
 
         // we must collect errors even in stream mode
         errors.push(err)
+        if (!failing) {
+          failing = true
+          queueMicrotask(() => { failing = false })
+        }
       }
 
       // nested = function is called by this.execute
@@ -482,8 +490,19 @@ class Request extends BaseRequest {
 
         this._cancel = () => {
           debug('request(%d): cancel', IDS.get(this))
+          // newer msnodesqlv8 versions drop a repeat cancel without calling back, so the
+          // outcome of the first one stands
+          if (typeof req.getCancelSent === 'function' && req.getCancelSent()) {
+            return
+          }
+          cancelRequested = true
           req.cancelQuery(err => {
-            if (err) debug('request(%d): failed to cancel', IDS.get(this), err)
+            if (err) {
+              debug('request(%d): failed to cancel', IDS.get(this), err)
+              // msnodesqlv8 < 5.6 refuses to cancel a query that is neither paused nor polling,
+              // and the query runs to the end
+              if (!hasReturned) cancelRequested = false
+            }
             // this fixes an issue where paused connections don't emit a done event
             try {
               if (req.isPaused()) req.emit('done')
@@ -494,6 +513,8 @@ class Request extends BaseRequest {
         }
 
         req.on('meta', metadata => {
+          if (finished) return
+
           if (row) {
             if (isChunkedRecordset) {
               const concatenatedChunks = chunksBuffer.join('')
@@ -571,7 +592,7 @@ class Request extends BaseRequest {
         })
 
         req.on('row', rownumber => {
-          if (row && isChunkedRecordset) return
+          if (finished || (row && isChunkedRecordset)) return
 
           if (this.arrayRowMode) {
             row = []
@@ -583,6 +604,8 @@ class Request extends BaseRequest {
         })
 
         req.on('column', (idx, data, more) => {
+          if (finished) return
+
           if (isChunkedRecordset) {
             chunksBuffer.push(data)
           } else {
@@ -622,13 +645,17 @@ class Request extends BaseRequest {
         })
 
         req.on('rowcount', rowCount => {
+          if (finished) return
+
           rowsAffected.push(rowCount)
           if (this.stream) {
             this.emit('rowsaffected', rowCount)
           }
         })
 
-        req.on('info', msg => {
+        req.on('info', (msg, more) => {
+          if (finished) return
+
           const parsedMessage = (/^\[Microsoft\]\[SQL Server Native Client 11\.0\](?:\[SQL Server\])?([\s\S]*)$/).exec(msg.message)
           if (parsedMessage) {
             msg.message = parsedMessage[1]
@@ -646,16 +673,75 @@ class Request extends BaseRequest {
 
           // query terminated
           if (msg.code === 3621 && !hasReturned) {
-            // if the query has been terminated it's probably best to throw the last meaningful error if there was one
-            // pop it off the errors array so it doesn't get put in twice
-            const error = errors.length > 0 ? errors.pop() : msg
-            handleError(req, connection, error.originalError || error, false)
+            if (failing && !more) {
+              // msnodesqlv8 2.0 ends a statement's error with more=true and this message, and then
+              // nothing follows: the error just collected is the one that terminated the statement.
+              // A stream has already emitted it
+              if (!this.stream) {
+                const error = errors.pop()
+                handleError(req, connection, error.originalError || error, false)
+              }
+            } else if (!terminated) {
+              // msnodesqlv8 >= 5.6 emits a failed statement's info messages ahead of its error, and
+              // carries on after it, so any error collected so far is an earlier statement's. The
+              // error still to come, or 'done', settles the request; 'done' falls back to this
+              terminated = msg
+            }
           }
         })
 
-        req.on('error', errorHandlers.error = handleError.bind(null, req, connection))
+        req.on('error', errorHandlers.error = (info, moreErrors) => {
+          // a dead link is recorded even after 'done', so the pool does not lend the connection out again
+          if ((typeof info.sqlstate === 'string') && (info.sqlstate.toLowerCase() === '08s01')) {
+            connection.hasError = true
+          }
+
+          if (finished) return
+
+          handleError(req, connection, info, moreErrors)
+        })
 
         req.once('done', () => {
+          finished = true
+
+          if (hasReturned) {
+            return
+          }
+
+          if (cancelRequested) {
+            // msnodesqlv8 emits 'done' once a cancel is through: for a paused query in every
+            // version, and for a running one from 5.6. What was read up to then is not a result.
+            // The error listener stays attached and drops the 'Operation canceled' error that can
+            // still arrive after 'done'.
+            hasReturned = true
+            delete this._cancel
+            // released now, as at any 'done', rather than at the statement's later 'free':
+            // msnodesqlv8 queues whatever the connection is sent next behind the cancelled
+            // statement, and a transaction can carry on as soon as the cancel is reported
+            this.parent.release(connection)
+
+            debug('request(%d): canceled', IDS.get(this))
+
+            const err = new RequestError('Canceled.', 'ECANCEL')
+            if (this.stream) {
+              // as with tedious, a cancelled stream reports the cancel as an error and still
+              // completes with what was read up to it
+              this.emit('error', err)
+              return callback(null, null, output, rowsAffected, recordsetcolumns)
+            }
+            return callback(err)
+          }
+
+          if (terminated && !errors.length) {
+            // the statement was terminated, but no error followed to say why
+            handleError(req, connection, terminated, false)
+          } else if (errors.length && !this.stream) {
+            // msnodesqlv8 >= 5.6 reports an error that does not end the batch with more=true and
+            // carries on to 'done', so the request has failed although no error said it was the last
+            const error = errors.pop()
+            handleError(req, connection, error.originalError || error, false)
+          }
+
           if (hasReturned) {
             return
           }

test/common/tests.js

@@ -538,6 +538,34 @@ module.exports = (sql, driver) => {
       })
     },
 
+    'query with an error that does not end the batch' (done) {
+      const req = new TestRequest()
+      req.query('raiserror(N\'Something went wrong\', 16, 1); select 1 as a', err => {
+        assert.ok(err instanceof sql.RequestError, 'the query should fail')
+        assert.strictEqual(err.message, 'Something went wrong', 'the error should be the one the batch raised')
+        assert.strictEqual(err.number, 50000, 'the error should be the one the batch raised')
+
+        done()
+      })
+    },
+
+    'query with a constraint violation' (afterAResult, done) {
+      const statements = [
+        // the batch fails before it returns anything
+        'declare @t table (id int primary key); insert into @t values (1), (1)'
+      ]
+      if (afterAResult) {
+        // a statement after a result fails
+        statements.push('declare @t table (id int primary key); insert into @t values (1); insert into @t values (1)')
+      }
+      statements.reduce((previous, statement) => previous.then(() => new TestRequest().query(statement).then(() => {
+        throw new Error(`${statement} should fail`)
+      }, err => {
+        assert.ok(err instanceof sql.RequestError, `${statement} should fail with a RequestError`)
+        assert.strictEqual(err.number, 2627, `${statement} should fail with the violation, not the statement's termination`)
+      })), Promise.resolve()).then(() => done(), done)
+    },
+
     'query with raiseerror' (done) {
       const notices = []
       const req = new TestRequest()
@@ -1712,7 +1740,11 @@ module.exports = (sql, driver) => {
         const req = tran.request()
         req.input('date', sql.TinyInt, 1561651515615)
 
-        req.execute('someStoreProc').catch(() => {
+        // the transaction is rolled back either way, so a failure is reported rather than
+        // left holding a connection
+        req.execute('someStoreProc').then(() => {
+          tran.rollback(() => done(new Error('executing a procedure that does not exist should fail')))
+        }, () => {
           tran.rollback(done)
         })
       }).catch(done)
@@ -1729,6 +1761,20 @@ module.exports = (sql, driver) => {
       req.cancel()
     },
 
+    'cancel a running request' (cancels, done) {
+      const req = new TestRequest()
+      req.query('waitfor delay \'00:00:05\'; select 1 as a').then(result => {
+        assert.ok(!cancels, 'the request should be cancelled')
+        assert.deepStrictEqual(result.recordset, [{ a: 1 }], 'a request that cannot be cancelled should run to the end')
+      }, err => {
+        assert.ok(cancels, `the request should not fail: ${err.message}`)
+        assert.strictEqual(err.code, 'ECANCEL', 'the request should fail as cancelled')
+      }).then(() => done(), done)
+
+      // by then the query is running
+      setTimeout(() => req.cancel(), 1000)
+    },
+
     'repeat calls to connect resolve' (config, done) {
       const pool = new sql.ConnectionPool(config)
       Promise.all([pool.connect(), pool.connect()])