From 6cedc31f524823a2ee337e33607400030405343b Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Mon, 12 Oct 2026 16:11:15 +0100 Subject: [PATCH 1/5] fix: reject msnodesqlv8 requests whose errors do not end the batch msnodesqlv8 5.6 reports an error that does not end the batch with more=true and carries on to 'done', where earlier versions ended the batch on it with more=false. A request only failed on a more=false error, so under 5.6 these requests resolved: executing a procedure that does not exist, a RAISERROR followed by results, a prepared statement that fails to prepare. A request now fails in 'done' when errors were collected on the way, as it does on tedious. Streams are unchanged: they emit each error as it arrives. The msnodesqlv8 driver only runs on Windows CI, so the new unit tests stand a stub in for it and play the events each version emits. Co-Authored-By: Claude Opus 5.5 --- .github/copilot-instructions.md | 1 + lib/msnodesqlv8/request.js | 11 +++ test/common/tests.js | 17 ++++- test/common/unit.js | 129 +++++++++++++++++++++++++++++++- test/msnodesqlv8/msnodesqlv8.js | 1 + test/tedious/tedious.js | 1 + 6 files changed, 158 insertions(+), 2 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6e0b1b0f..755b6f5a 100644 --- a/.github/copilot-instructions.md +++ b/.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. diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index 692064c4..07cb8695 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -660,6 +660,17 @@ class Request extends BaseRequest { return } + 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 + } + hasReturned = true if (!this._nested) { diff --git a/test/common/tests.js b/test/common/tests.js index 1ec40a38..b5854fc4 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -538,6 +538,17 @@ 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 raiseerror' (done) { const notices = [] const req = new TestRequest() @@ -1712,7 +1723,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) diff --git a/test/common/unit.js b/test/common/unit.js index a206755d..7ab0eee3 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -1,6 +1,6 @@ 'use strict' -/* globals describe, it, before, afterEach */ +/* globals describe, it, before, after, afterEach */ const sql = require('../../') const assert = require('node:assert') @@ -1690,3 +1690,130 @@ describe('procedure name validation', () => { } }) }) + +describe('msnodesqlv8 query events', () => { + const Module = require('node:module') + const { EventEmitter } = require('node:events') + const RequestError = require('../../lib/error/request-error') + + // msnodesqlv8 is a native module that only the Windows CI jobs install. The request only + // reaches into it for TVPs, so a bare object stands in for it, and each test plays the + // events a version of it emits on the query it is given. + const STUB = 'msnodesqlv8-stub' + const REQUEST = require.resolve('../../lib/msnodesqlv8/request') + let resolveFilename + let Request + + before(() => { + resolveFilename = Module._resolveFilename + Module._resolveFilename = function (request, ...rest) { + return request === 'msnodesqlv8' ? STUB : resolveFilename.call(this, request, ...rest) + } + require.cache[STUB] = { id: STUB, filename: STUB, loaded: true, exports: {} } + Request = require(REQUEST) + }) + + after(() => { + Module._resolveFilename = resolveFilename + delete require.cache[STUB] + delete require.cache[REQUEST] + }) + + const odbcError = (message, code, sqlstate) => Object.assign(new Error(message), { code, sqlstate }) + const PRIMARY_KEY = () => odbcError('Violation of PRIMARY KEY constraint \'PK_t\'.', 2627, '23000') + + // the request's pool, its connection and the query that connection runs; `play` is given + // the query once it is running and emits on it what msnodesqlv8 would + const setup = (play, query = {}) => { + const notify = Object.assign(new EventEmitter(), { + isPaused: () => false, + cancelQuery () {} + }, query) + // msnodesqlv8 does not emit an error that nothing listens for + notify.fail = (err, more) => { + if (notify.listenerCount('error') > 0) notify.emit('error', err, more) + } + const connection = { + queryRaw () { + setImmediate(play, notify) + return notify + } + } + const pool = { + connected: true, + config: {}, + released: 0, + acquire (request, callback) { callback(null, connection, { requestTimeout: 15000 }) }, + release () { this.released++ } + } + return { request: new Request(pool), notify, pool, connection } + } + + const result = (meta, value) => notify => { + notify.emit('meta', [{ name: meta, sqlType: 'int', size: 4, nullable: false }]) + notify.emit('row', 0) + notify.emit('column', 0, value, false) + } + + it('should reject when an error ends the batch', async () => { + const { request, pool } = setup(notify => { + notify.fail(PRIMARY_KEY(), false) + notify.emit('done') + }) + await assert.rejects(request.query('insert into t values (1)'), err => { + assert.ok(err instanceof RequestError, 'the error should be a RequestError') + assert.strictEqual(err.number, 2627, 'the error should be the one that ended the batch') + assert.strictEqual(err.code, 'EREQUEST', 'the error should be a request error') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 >= 5.6 + it('should reject when an error does not end the batch', async () => { + const { request, pool } = setup(notify => { + notify.fail(odbcError('Something went wrong', 50000, '42000'), true) + result('a', 1)(notify) + notify.emit('done') + }) + await assert.rejects(request.query('raiserror(N\'Something went wrong\', 16, 1); select 1 as a'), err => { + assert.strictEqual(err.message, 'Something went wrong', 'the error should be the one the batch reported') + assert.strictEqual(err.code, 'EREQUEST', 'the error should be a request error') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 >= 5.6, which carries on to the select of the return value + it('should reject an execute of a procedure that cannot be found', async () => { + const { request, pool } = setup(notify => { + notify.fail(odbcError('Could not find stored procedure \'missing\'.', 2812, '42000'), true) + result('___return___', 0)(notify) + notify.emit('done') + }) + await assert.rejects(request.execute('missing'), err => { + assert.strictEqual(err.number, 2812, 'the error should be the missing procedure') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + it('should stream an error that does not end the batch, then complete', async () => { + const { request } = setup(notify => { + notify.fail(odbcError('Something went wrong', 50000, '42000'), true) + result('a', 1)(notify) + notify.emit('done') + }) + request.stream = true + const errors = [] + const rows = [] + request.on('error', err => errors.push(err)) + request.on('row', row => rows.push(row)) + await new Promise(resolve => { + request.on('done', resolve) + request.query('select 1') + }) + assert.deepStrictEqual(errors.map(e => e.message), ['Something went wrong'], 'the error should be streamed once') + assert.deepStrictEqual(rows, [{ a: 1 }], 'the rows after the error should still be streamed') + }) +}) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 9b952408..16a97090 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -93,6 +93,7 @@ describe('msnodesqlv8', function () { it('parameters with length as max', done => TESTS['parameters with length as max'](done)) it('output parameters with a falsy initial value', done => TESTS['output parameters with a falsy initial value'](done)) it('query with error', done => TESTS['query with error'](done)) + it('query with an error that does not end the batch', done => TESTS['query with an error that does not end the batch'](done)) it('query with multiple errors (not supported by msnodesqlv8)', done => TESTS['query with multiple errors'](done)) it.skip('query with raiseerror (not supported by msnodesqlv8)', done => TESTS['query with raiseerror'](done)) it('query with toReadableStream', done => TESTS['query with toReadableStream'](done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index 4d874938..7200696e 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -100,6 +100,7 @@ describe('tedious', () => { it('parameters with length as max', done => TESTS['parameters with length as max'](done)) it('output parameters with a falsy initial value', done => TESTS['output parameters with a falsy initial value'](done)) it('query with error', done => TESTS['query with error'](done)) + it('query with an error that does not end the batch', done => TESTS['query with an error that does not end the batch'](done)) it('query with multiple errors', done => TESTS['query with multiple errors'](done)) it('query with raiseerror', done => TESTS['query with raiseerror'](done)) it('query with toReadableStream', done => TESTS['query with toReadableStream'](done)) From d040e22a13abb96788d3d41f600b21cb2098dd8a Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Mon, 12 Oct 2026 16:11:35 +0100 Subject: [PATCH 2/5] fix: keep every preceding error on msnodesqlv8 requests A failed msnodesqlv8 request kept only the error just before the last one in `precedingErrors`. msnodesqlv8 5.6 carries a batch on after an error, so a batch can now report several, and the earlier ones were lost. Keep them all, as tedious does. Co-Authored-By: Claude Opus 5.5 --- lib/msnodesqlv8/request.js | 7 +------ test/common/unit.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index 07cb8695..fe30da26 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -360,13 +360,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 diff --git a/test/common/unit.js b/test/common/unit.js index 7ab0eee3..38863772 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -1784,6 +1784,24 @@ describe('msnodesqlv8 query events', () => { assert.strictEqual(pool.released, 1, 'the connection should be released once') }) + // msnodesqlv8 >= 5.6 + it('should keep the errors that came before the last one in a batch that carries on', async () => { + const { request } = setup(notify => { + notify.fail(odbcError('first', 50000, '42000'), true) + result('a', 1)(notify) + notify.fail(odbcError('second', 50000, '42000'), true) + result('b', 2)(notify) + notify.fail(odbcError('third', 50000, '42000'), true) + result('c', 3)(notify) + notify.emit('done') + }) + await assert.rejects(request.query('select 1'), err => { + assert.strictEqual(err.message, 'third', 'the error should be the last one reported') + assert.deepStrictEqual(err.precedingErrors.map(e => e.message), ['first', 'second'], 'the earlier errors should precede it') + return true + }) + }) + // msnodesqlv8 >= 5.6, which carries on to the select of the return value it('should reject an execute of a procedure that cannot be found', async () => { const { request, pool } = setup(notify => { From 3468d963e26634b23b1cfd255b66a8307be9fca2 Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Mon, 12 Oct 2026 16:12:24 +0100 Subject: [PATCH 3/5] fix: reject a terminated msnodesqlv8 statement with its own error When a statement is terminated, SQL Server follows its error with 3621 "The statement has been terminated.". msnodesqlv8 5.6 emits that message ahead of the error, and the request failed with the 3621 instead of the error that caused it, such as 2627 for a duplicate key. The message now settles the request only when the error came with it in the same diagnostics, which msnodesqlv8 2.0 needs because it can stop after the message. Otherwise the error that follows, or 'done', settles the request. A stream no longer emits that error a second time. Co-Authored-By: Claude Opus 5.5 --- lib/msnodesqlv8/request.js | 32 ++++++++-- test/common/tests.js | 17 +++++ test/common/unit.js | 107 ++++++++++++++++++++++++++++++++ test/msnodesqlv8/msnodesqlv8.js | 7 +++ test/tedious/tedious.js | 1 + 5 files changed, 158 insertions(+), 6 deletions(-) diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index fe30da26..99579196 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -334,6 +334,9 @@ 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 const handleError = (req, connection, info, moreErrors) => { const doReturn = !moreErrors @@ -371,6 +374,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 @@ -623,7 +630,7 @@ class Request extends BaseRequest { } }) - req.on('info', msg => { + req.on('info', (msg, more) => { const parsedMessage = (/^\[Microsoft\]\[SQL Server Native Client 11\.0\](?:\[SQL Server\])?([\s\S]*)$/).exec(msg.message) if (parsedMessage) { msg.message = parsedMessage[1] @@ -641,10 +648,20 @@ 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 + } } }) @@ -655,7 +672,10 @@ class Request extends BaseRequest { return } - if (errors.length && !this.stream) { + 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() diff --git a/test/common/tests.js b/test/common/tests.js index b5854fc4..ba598286 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -549,6 +549,23 @@ module.exports = (sql, driver) => { }) }, + '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() diff --git a/test/common/unit.js b/test/common/unit.js index 38863772..8c7373a8 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -1721,6 +1721,8 @@ describe('msnodesqlv8 query events', () => { const odbcError = (message, code, sqlstate) => Object.assign(new Error(message), { code, sqlstate }) const PRIMARY_KEY = () => odbcError('Violation of PRIMARY KEY constraint \'PK_t\'.', 2627, '23000') + const TERMINATED = () => odbcError('The statement has been terminated.', 3621, '01000') + const tick = () => new Promise(resolve => setImmediate(resolve)) // the request's pool, its connection and the query that connection runs; `play` is given // the query once it is running and emits on it what msnodesqlv8 would @@ -1834,4 +1836,109 @@ describe('msnodesqlv8 query events', () => { assert.deepStrictEqual(errors.map(e => e.message), ['Something went wrong'], 'the error should be streamed once') assert.deepStrictEqual(rows, [{ a: 1 }], 'the rows after the error should still be streamed') }) + + // msnodesqlv8 >= 5.6 emits a failed statement's info messages ahead of its error + it('should reject with the error a statement was terminated by when it follows the message', async () => { + const { request } = setup(notify => { + notify.emit('info', TERMINATED(), false) + notify.fail(PRIMARY_KEY(), false) + notify.emit('done') + }) + await assert.rejects(request.query('insert into t values (1)'), err => { + assert.strictEqual(err.number, 2627, 'the error should be the one that terminated the statement') + return true + }) + }) + + // msnodesqlv8 >= 5.6, when the statement fails after an earlier result and the batch carries on + it('should reject with the error a statement was terminated by when it follows the message and does not end the batch', async () => { + const { request } = setup(notify => { + notify.emit('info', TERMINATED(), false) + notify.fail(PRIMARY_KEY(), true) + result('___return___', 0)(notify) + notify.emit('done') + }) + await assert.rejects(request.execute('insert_t'), err => { + assert.strictEqual(err.number, 2627, 'the error should be the one that terminated the statement') + return true + }) + }) + + // msnodesqlv8 2.0 could stop after the message that the statement was terminated (node-mssql #1126) + it('should reject with the error a statement was terminated by when it comes before the message', async () => { + const { request } = setup(notify => { + notify.fail(PRIMARY_KEY(), true) + notify.emit('info', TERMINATED(), false) + }) + await assert.rejects(request.query('insert into t values (1)'), err => { + assert.strictEqual(err.number, 2627, 'the error should be the one that terminated the statement') + return true + }) + }) + + it('should reject with the message that a statement was terminated when no error says why', async () => { + const { request } = setup(notify => { + notify.emit('info', TERMINATED(), false) + notify.emit('done') + }) + await assert.rejects(request.query('insert into t values (1)'), err => { + assert.strictEqual(err.number, 3621, 'the error should be the termination') + return true + }) + }) + + // msnodesqlv8 >= 5.6: the batch carries on after an earlier statement's error, and the + // message that a later statement was terminated comes ahead of the error that terminated it + it('should reject with the last error when a later statement is terminated', async () => { + const { request, pool } = setup(async notify => { + notify.fail(odbcError('Something went wrong', 50000, '42000'), true) + await tick() + notify.emit('info', TERMINATED(), false) + notify.fail(PRIMARY_KEY(), true) + await tick() + result('a', 1)(notify) + notify.emit('done') + }) + await assert.rejects(request.query('raiserror(N\'Something went wrong\', 16, 1); insert into t values (1); select 1 as a'), err => { + assert.strictEqual(err.number, 2627, 'the error should be the last one the batch reported') + assert.deepStrictEqual(err.precedingErrors.map(e => e.number), [50000], 'the earlier error should precede it') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 >= 5.6: a procedure whose first statement fails reports the error ahead of the + // message, and carries on to the select of the return value + it('should reject an execute whose first statement is terminated, once the batch is done', async () => { + let releasedBeforeDone + const { request, pool } = setup(notify => { + notify.fail(PRIMARY_KEY(), true) + notify.emit('info', TERMINATED(), true) + result('___return___', 0)(notify) + releasedBeforeDone = pool.released + notify.emit('done') + }) + await assert.rejects(request.execute('insert_t'), err => { + assert.strictEqual(err.number, 2627, 'the error should be the one that terminated the statement') + return true + }) + assert.strictEqual(releasedBeforeDone, 0, 'the connection should not be released before the batch is done') + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + it('should stream an error a statement was terminated by once', async () => { + const { request } = setup(notify => { + notify.fail(PRIMARY_KEY(), false) + notify.emit('info', TERMINATED(), false) + notify.emit('done') + }) + request.stream = true + const errors = [] + request.on('error', err => errors.push(err)) + await new Promise(resolve => { + request.on('done', resolve) + request.query('insert into t values (1)') + }) + assert.deepStrictEqual(errors.map(e => e.number), [2627], 'the error should be streamed once') + }) }) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 16a97090..d748b18a 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -10,6 +10,12 @@ const TIMES = require('../common/times.js')(sql, 'msnodesqlv8') const versionHelper = require('../common/versionhelper') const { readFileSync } = require('node:fs') +const [major, minor, patch] = require('msnodesqlv8/package.json').version.split('.').map(Number) + +// before 5.1.9 msnodesqlv8 reads the next result's metadata after SQLMoreResults has failed, and +// that call replaces an error that follows an earlier result in the batch with one of its own +const reportsErrorsAfterAResult = major > 5 || (major === 5 && (minor > 1 || (minor === 1 && patch >= 9))) + const config = function () { const cfg = JSON.parse(readFileSync(join(__dirname, '../.mssql.json'))) cfg.driver = 'msnodesqlv8' @@ -94,6 +100,7 @@ describe('msnodesqlv8', function () { it('output parameters with a falsy initial value', done => TESTS['output parameters with a falsy initial value'](done)) it('query with error', done => TESTS['query with error'](done)) it('query with an error that does not end the batch', done => TESTS['query with an error that does not end the batch'](done)) + it('query with a constraint violation', done => TESTS['query with a constraint violation'](reportsErrorsAfterAResult, done)) it('query with multiple errors (not supported by msnodesqlv8)', done => TESTS['query with multiple errors'](done)) it.skip('query with raiseerror (not supported by msnodesqlv8)', done => TESTS['query with raiseerror'](done)) it('query with toReadableStream', done => TESTS['query with toReadableStream'](done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index 7200696e..d82ce736 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -101,6 +101,7 @@ describe('tedious', () => { it('output parameters with a falsy initial value', done => TESTS['output parameters with a falsy initial value'](done)) it('query with error', done => TESTS['query with error'](done)) it('query with an error that does not end the batch', done => TESTS['query with an error that does not end the batch'](done)) + it('query with a constraint violation', done => TESTS['query with a constraint violation'](true, done)) it('query with multiple errors', done => TESTS['query with multiple errors'](done)) it('query with raiseerror', done => TESTS['query with raiseerror'](done)) it('query with toReadableStream', done => TESTS['query with toReadableStream'](done)) From 11923a26e8f2ce7a0b681036b693efaab6949736 Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Mon, 12 Oct 2026 16:13:02 +0100 Subject: [PATCH 4/5] fix: report cancelled msnodesqlv8 requests, including paused streams, as ECANCEL msnodesqlv8 ends a cancelled query with 'done': a paused one in every version, and from 5.6 a running one too, followed by an 'Operation canceled' error. The request took that 'done' for success, so a cancelled request resolved with what had been read so far, and on 5.6 a stream went on to emit the late error after 'done'. A cancel is now reported as ECANCEL, as on tedious: a stream emits it as an error and then completes with what was read. The late error is dropped, and nothing msnodesqlv8 emits after 'done' reaches the request, though a broken link still marks the connection. Before 5.6 msnodesqlv8 refuses to cancel a running query that is not paused, which runs to the end as it did before. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 + lib/msnodesqlv8/request.js | 72 ++++++++++- test/common/tests.js | 14 +++ test/common/unit.js | 205 ++++++++++++++++++++++++++++++++ test/msnodesqlv8/msnodesqlv8.js | 5 + test/tedious/tedious.js | 1 + 6 files changed, 295 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 7035c9dd..c05d8e7f 100644 --- a/README.md +++ b/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. diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index 99579196..750f79d5 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -337,13 +337,19 @@ class Request extends BaseRequest { 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' @@ -484,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') @@ -496,6 +513,8 @@ class Request extends BaseRequest { } req.on('meta', metadata => { + if (finished) return + if (row) { if (isChunkedRecordset) { const concatenatedChunks = chunksBuffer.join('') @@ -573,7 +592,7 @@ class Request extends BaseRequest { }) req.on('row', rownumber => { - if (row && isChunkedRecordset) return + if (finished || (row && isChunkedRecordset)) return if (this.arrayRowMode) { row = [] @@ -585,6 +604,8 @@ class Request extends BaseRequest { }) req.on('column', (idx, data, more) => { + if (finished) return + if (isChunkedRecordset) { chunksBuffer.push(data) } else { @@ -624,6 +645,8 @@ class Request extends BaseRequest { }) req.on('rowcount', rowCount => { + if (finished) return + rowsAffected.push(rowCount) if (this.stream) { this.emit('rowsaffected', rowCount) @@ -631,6 +654,8 @@ class Request extends BaseRequest { }) 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] @@ -665,13 +690,48 @@ class Request extends BaseRequest { } }) - 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) diff --git a/test/common/tests.js b/test/common/tests.js index ba598286..0d4800ac 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -1761,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()]) diff --git a/test/common/unit.js b/test/common/unit.js index 8c7373a8..ee832aa3 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -1722,6 +1722,8 @@ describe('msnodesqlv8 query events', () => { const odbcError = (message, code, sqlstate) => Object.assign(new Error(message), { code, sqlstate }) const PRIMARY_KEY = () => odbcError('Violation of PRIMARY KEY constraint \'PK_t\'.', 2627, '23000') const TERMINATED = () => odbcError('The statement has been terminated.', 3621, '01000') + const CANCELED = () => odbcError('[msnodesql] Operation canceled', 0, 'HY008') + const REFUSED = () => new Error('Error: [msnodesql] cancel only supported for statements where polling is enabled.') const tick = () => new Promise(resolve => setImmediate(resolve)) // the request's pool, its connection and the query that connection runs; `play` is given @@ -1941,4 +1943,207 @@ describe('msnodesqlv8 query events', () => { }) assert.deepStrictEqual(errors.map(e => e.number), [2627], 'the error should be streamed once') }) + + // msnodesqlv8 >= 5.6: 'done' as soon as the cancel is through, then an error from the read + // the cancel cut short, then 'free' once the statement is released + const cancels = { + cancelQuery (callback) { + setImmediate(() => { + this.emit('done') + callback(null) + setImmediate(() => { + this.fail(CANCELED(), false) + setImmediate(() => this.emit('free')) + }) + }) + } + } + + // msnodesqlv8 >= 5.6. The connection goes back with the cancel, not at the statement's 'free' + it('should reject a running query that is cancelled', async () => { + const { request, notify, pool } = setup(notify => { + result('a', 1)(notify) + request.cancel() + }, cancels) + const freed = new Promise(resolve => notify.once('free', resolve)) + await assert.rejects(request.query('waitfor delay \'00:00:05\'; select 1 as a'), err => { + assert.strictEqual(err.code, 'ECANCEL', 'the error should be a cancel') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released with the cancel') + await freed + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 >= 5.6 + it('should stream a cancel once, then complete', async () => { + const { request } = setup(notify => { + result('a', 1)(notify) + request.cancel() + }, cancels) + request.stream = true + const errors = [] + let done = 0 + request.on('error', err => errors.push(err)) + await new Promise(resolve => { + request.on('done', () => { + done++ + resolve() + }) + request.query('select * from streaming') + }) + // the error the cancel cut the read short with comes after 'done' + await tick() + await tick() + assert.deepStrictEqual(errors.map(e => e.code), ['ECANCEL'], 'only the cancel should be streamed as an error') + assert.strictEqual(done, 1, 'the stream should complete once') + }) + + // msnodesqlv8 >= 5.6 can report a cancel the statement refused as done, and the query goes on + it('should stream nothing a cancelled query emits after done', async () => { + const { request, pool } = setup(notify => { + result('a', 1)(notify) + request.cancel() + }, { + cancelQuery (callback) { + setImmediate(() => { + this.emit('done') + callback(null) + setImmediate(() => { + result('b', 2)(this) + this.emit('rowcount', 1) + this.emit('info', odbcError('Still running', 0, '01000'), false) + this.fail(odbcError('Something went wrong', 50000, '42000'), false) + this.emit('done') + }) + }) + } + }) + request.stream = true + const events = [] + request.on('recordset', () => events.push('recordset')) + request.on('row', () => events.push('row')) + request.on('rowsaffected', () => events.push('rowsaffected')) + request.on('info', () => events.push('info')) + request.on('error', err => events.push(`error ${err.code}`)) + request.on('done', () => events.push('done')) + request.query('select * from streaming') + while (!events.includes('done')) await tick() + await tick() + await tick() + assert.deepStrictEqual(events, ['recordset', 'row', 'error ECANCEL', 'done'], 'nothing should be streamed after done') + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 < 5.6 + it('should complete a query whose cancel is refused', async () => { + const { request } = setup(notify => { + result('a', 1)(notify) + request.cancel() + setImmediate(() => setImmediate(() => notify.emit('done'))) + }, { + cancelQuery (callback) { + setImmediate(callback, REFUSED()) + } + }) + const { recordset } = await request.query('select 1 as a') + assert.deepStrictEqual(recordset, [{ a: 1 }], 'the query should complete') + }) + + // msnodesqlv8 5.1 to 5.5 note the first cancel, refused or not, and drop any after it + it('should complete a query whose first cancel is refused and later ones dropped', async () => { + let sent = false + const { request } = setup(notify => { + result('a', 1)(notify) + request.cancel() + setImmediate(() => setImmediate(() => { + request.cancel() + notify.emit('done') + })) + }, { + getCancelSent: () => sent, + cancelQuery (callback) { + if (sent) return + sent = true + setImmediate(callback, REFUSED()) + } + }) + const { recordset } = await request.query('select 1 as a') + assert.deepStrictEqual(recordset, [{ a: 1 }], 'the query should complete') + }) + + // every msnodesqlv8 version cancels a paused query, and ends it with 'done' before calling back + it('should stream a cancel of a paused query once, then complete with what was read', async () => { + let paused = false + const { request, pool } = setup(notify => { + result('a', 1)(notify) + notify.emit('rowcount', 1) + result('b', 2)(notify) + }, { + isPaused: () => paused, + pauseQuery () { paused = true }, + resumeQuery () { paused = false }, + cancelQuery (callback) { + setImmediate(() => { + this.emit('done') + callback(null) + }) + } + }) + request.stream = true + const errors = [] + let completed = null + request.on('error', err => errors.push(err)) + request.on('row', row => { + if (row.b) { + request.pause() + request.cancel() + } + }) + await new Promise(resolve => { + request.on('done', result => { + completed = result + resolve() + }) + request.query('select * from streaming') + }) + await tick() + assert.deepStrictEqual(errors.map(e => e.code), ['ECANCEL'], 'the cancel should be streamed as an error') + assert.deepStrictEqual(completed.rowsAffected, [1], 'the stream should complete with the rows affected so far') + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + // msnodesqlv8 >= 5.6 can fail the read a cancel cut short before the cancel is through + it('should reject a cancelled query whose read fails before the cancel is through', async () => { + const { request, pool } = setup(notify => request.cancel(), { + cancelQuery (callback) { + setImmediate(() => { + this.fail(CANCELED(), false) + this.emit('done') + callback(null) + }) + } + }) + await assert.rejects(request.query('waitfor delay \'00:00:05\''), err => { + assert.strictEqual(err.code, 'ECANCEL', 'the error should be a cancel') + return true + }) + assert.strictEqual(pool.released, 1, 'the connection should be released once') + }) + + it('should mark the connection broken when the link fails after a cancel', async () => { + const { request, connection } = setup(notify => request.cancel(), { + cancelQuery (callback) { + setImmediate(() => { + this.emit('done') + callback(null) + setImmediate(() => this.fail(odbcError('Communication link failure', 0, '08S01'), false)) + }) + } + }) + await assert.rejects(request.query('select 1')) + await tick() + await tick() + assert.strictEqual(connection.hasError, true, 'a link failure after a cancel should mark the connection as broken') + }) }) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index d748b18a..ef04cf97 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -16,6 +16,10 @@ const [major, minor, patch] = require('msnodesqlv8/package.json').version.split( // that call replaces an error that follows an earlier result in the batch with one of its own const reportsErrorsAfterAResult = major > 5 || (major === 5 && (minor > 1 || (minor === 1 && patch >= 9))) +// msnodesqlv8 cancels a running query from 5.6; before that it only cancels one that is paused +// or polling, and lets any other run to the end +const cancelsRunningQueries = major > 5 || (major === 5 && minor >= 6) + const config = function () { const cfg = JSON.parse(readFileSync(join(__dirname, '../.mssql.json'))) cfg.driver = 'msnodesqlv8' @@ -127,6 +131,7 @@ describe('msnodesqlv8', function () { it.skip('transaction with error (XACT_ABORT set to ON) (not supported by msnodesqlv8)', done => TESTS['transaction with error'](done)) it('transaction with synchronous error', done => TESTS['transaction with synchronous error'](done)) it('cancel request', done => TESTS['cancel request'](done)) + it('cancel a running request', done => TESTS['cancel a running request'](cancelsRunningQueries, done)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) it('calls to close during connection throw', done => TESTS['calls to close during connection throw'](config(), done)) it('calls to close during connection call back exactly once', done => TESTS['calls to close during connection call back exactly once'](config(), done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index d82ce736..d64142d7 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -127,6 +127,7 @@ describe('tedious', () => { it('transaction with error (XACT_ABORT set to ON)', done => TESTS['transaction with error'](done)) it('transaction with synchronous error', done => TESTS['transaction with synchronous error'](done)) it('cancel request', done => TESTS['cancel request'](done, /Canceled./)) + it('cancel a running request', done => TESTS['cancel a running request'](true, done)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) it('calls to close during connection throw', done => TESTS['calls to close during connection throw'](config(), done)) it('calls to close during connection call back exactly once', done => TESTS['calls to close during connection call back exactly once'](config(), done)) From fc26723bf571bc7e1285024377cc6c175735cc1a Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Mon, 12 Oct 2026 16:13:10 +0100 Subject: [PATCH 5/5] ci: correct the note on msnodesqlv8 v5 prebuilds The note above the msnodesqlv8 v5 steps said upstream only builds prebuilds for Node 22, 24 and 26, so a floating `^5` could never load on Node 20 again. From 5.3.0 msnodesqlv8 ships a single Node-API binary instead of one per ABI, which may well load on Node 20; it has not been tried here, so Node 20 stays on 5.2.1. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/nodejs.yml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index bc583e6c..06805b2d 100644 --- a/.github/workflows/nodejs.yml +++ b/.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