From 402bc1b60b149a9a1960f0ea7019193e23e23c9f Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 11 Oct 2026 13:15:03 +0100 Subject: [PATCH 1/2] fix: prints, errors and results after a failed statement in a batch were lost A statement failing part way through a batch (raiserror severity 11-16, a failed insert, ...) does not end the batch: SQL Server runs the statements after it. The driver stopped at the first error, so every later print, error, result set and proc output param was dropped, and 'done' never fired. - C++: after a statement error, look ahead with SQLMoreResults. SQL_NO_DATA means the batch really ended with the error (one callback, more = false, as before). Otherwise the result it moved to is handed to the next try_read_next_result. Cancel, timeout, broken connection and statement state errors still end it. - Bound params are kept from bind time, so proc output params and the return code arrive even when an earlier statement in the proc failed. - JS: an error from the next result is reported and the query carries on when more results follow. Callback / event 'more' is true until the batch ends; the proc callback now passes 'more' with an error. A promise rejects with the last error, which now always carries the collected results in err._results. Co-Authored-By: Claude Opus 5.5 (1M context) --- cpp/include/odbc/odbc_statement_legacy.h | 5 + cpp/src/odbc/odbc_statement_legacy.cpp | 47 +++++- lib/procedure-bound.js | 3 +- lib/query-aggregator.js | 2 + lib/reader.js | 15 +- test/batch-errors.test.js | 197 +++++++++++++++++++++++ 6 files changed, 261 insertions(+), 8 deletions(-) create mode 100644 test/batch-errors.test.js diff --git a/cpp/include/odbc/odbc_statement_legacy.h b/cpp/include/odbc/odbc_statement_legacy.h index bd91f103..0c9b30e1 100644 --- a/cpp/include/odbc/odbc_statement_legacy.h +++ b/cpp/include/odbc/odbc_statement_legacy.h @@ -6,6 +6,7 @@ #include #include #include +#include #include "odbc_common.h" #include "odbc_driver_types.h" @@ -198,6 +199,8 @@ class OdbcStatementLegacy : public IOdbcStatement { bool return_odbc_error(); bool check_odbc_error(SQLRETURN ret); bool return_cancelled(); + bool more_results_after_error() const; + bool look_ahead_after_error(); shared_ptr _query; @@ -212,6 +215,8 @@ class OdbcStatementLegacy : public IOdbcStatement { std::atomic _cancelRequested; // set by Cancel() on the cancel thread before SQLCancelHandle; read by the worker std::atomic _cancelHandleIssued{false}; + // SQLMoreResults already called by look_ahead_after_error, for try_read_next_result + std::optional _pendingMoreResults; std::atomic _pollingEnabled; bool _numericStringEnabled; bool _bigIntAsNativeEnabled; diff --git a/cpp/src/odbc/odbc_statement_legacy.cpp b/cpp/src/odbc/odbc_statement_legacy.cpp index 766efef8..f1225dfe 100644 --- a/cpp/src/odbc/odbc_statement_legacy.cpp +++ b/cpp/src/odbc/odbc_statement_legacy.cpp @@ -698,6 +698,40 @@ bool OdbcStatementLegacy::return_cancelled() { return false; } +// a statement failing part way through a batch, e.g. raiserror severity 16, does not end +// the batch: SQL Server runs the statements after it and SQLMoreResults moves on to them. +// a cancel, timeout or broken connection does end it, as does an error that is only about +// the statement's own state (which SQLMoreResults would just repeat). +bool OdbcStatementLegacy::more_results_after_error() const { + if (!_statement || _cancelRequested.load() || _cancelHandleIssued.load() || _errors->empty()) { + return false; + } + return std::none_of(_errors->begin(), _errors->end(), [](const auto& e) { + const auto& s = e->sqlstate; + return s == "HY008" || s == "HYT00" || s == "HY010" || s == "HY007" || s == "24000" || + s.rfind("08", 0) == 0; + }); +} + +// whether the batch goes on can only be known by asking: call SQLMoreResults now, so a +// batch that ends with the error still ends with it (one callback, more = false). the +// result it moves to is picked up by the next try_read_next_result. the diag records of +// the failed statement were read before this call replaces them. +bool OdbcStatementLegacy::look_ahead_after_error() { + _pendingMoreResults.reset(); + if (!more_results_after_error()) { + return false; + } + const auto ret = _odbcApi->SQLMoreResults(_statement->get_handle()); + SQL_LOG_DEBUG_STREAM("[" << _handle.toString() << "] look_ahead_after_error SQLMoreResults " + << ret); + if (ret == SQL_NO_DATA) { + return false; + } + _pendingMoreResults = ret; + return true; +} + bool OdbcStatementLegacy::check_odbc_error(const SQLRETURN ret) { if (!SQL_SUCCEEDED(ret)) { SQL_LOG_DEBUG_STREAM("check_odbc_error failed: " << ret); @@ -1082,6 +1116,7 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptrclear(); _errorHandler->ClearErrors(); + _pendingMoreResults.reset(); _query = q; const auto timeout = q->timeout; @@ -1113,6 +1148,8 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptr_end_of_results = true; // reset const auto ret = query_timeout(timeout); if (!check_odbc_error(ret)) { @@ -1173,6 +1210,7 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptr(0); } _resultset->_end_of_rows = true; + _resultset->_end_of_results = !look_ahead_after_error(); return false; } @@ -2263,7 +2301,9 @@ bool OdbcStatementLegacy::try_read_next_result() { return false; } const auto& statement = *_statement; - const auto ret = _odbcApi->SQLMoreResults(statement.get_handle()); + const auto ret = _pendingMoreResults ? *_pendingMoreResults + : _odbcApi->SQLMoreResults(statement.get_handle()); + _pendingMoreResults.reset(); switch (ret) { case SQL_NO_DATA: { SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::try_read_next_result [" @@ -2301,8 +2341,11 @@ bool OdbcStatementLegacy::try_read_next_result() { SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::try_read_next_result [" << _handle.toString() << "] try_read_next_result SQL_ERROR from SQLMoreResults"); + // don't read this result's columns (that is what fails with HY007), but the + // statements after the failed one may still have results to move on to. return_odbc_error(); - _resultset->_end_of_results = true; + _resultset = make_unique(0); + _resultset->_end_of_results = !look_ahead_after_error(); _resultset->_end_of_rows = true; return false; } diff --git a/lib/procedure-bound.js b/lib/procedure-bound.js index fc09aee5..e7ac8b44 100755 --- a/lib/procedure-bound.js +++ b/lib/procedure-bound.js @@ -88,7 +88,8 @@ class ProcedureBound { function onProcedureRaw (err, results, more, outputParams) { if (chunky.callback) { if (err) { - chunky.callback(err) + // more: a failed statement in the proc need not be the end of it + chunky.callback(err, undefined, undefined, more) } else { chunky.callback(err, driverMgr.objectify(results), outputParams, more) } diff --git a/lib/query-aggregator.js b/lib/query-aggregator.js index eb60aaee..7a9b1996 100755 --- a/lib/query-aggregator.js +++ b/lib/query-aggregator.js @@ -238,6 +238,8 @@ class QueryAggregator { ret.end() const last = ret.lastError() if (last) { + // the last error may not be the end of the batch, so attach the results here too + last._results = ret reject(last) } else { resolve(ret) diff --git a/lib/reader.js b/lib/reader.js index 5af388d7..632a86bc 100755 --- a/lib/reader.js +++ b/lib/reader.js @@ -71,7 +71,8 @@ class Query extends BasePromises { } /* route non-critical info messages to its own event to prevent streams based readers from halting */ - routeStatementError (errorsAndInfo, callback, notify) { + // moreResults: the batch carries on after these errors, so 'more' is true for all of them + routeStatementError (errorsAndInfo, callback, notify, moreResults = false) { if (!Array.isArray(errorsAndInfo)) { errorsAndInfo = [errorsAndInfo] } @@ -86,12 +87,12 @@ class Query extends BasePromises { errorsAndInfo.forEach(err => { const info = this.isInfo(err) if (callback && !info) { - const more = i < errorsAndInfo.length - 1 + const more = moreResults || i < errorsAndInfo.length - 1 callback(err, null, more) } else { const ev = info ? 'info' : 'error' if (notify) { - const more = i < errorCount - 1 + const more = moreResults || i < errorCount - 1 if (notify.listenerCount(ev) > 0) { notify.emit(ev, err, more) } @@ -159,7 +160,11 @@ class Query extends BasePromises { setImmediate(() => { // may contain info messages e.g. raised by PRINT statements - do not want to reject these const errorMessages = e ? this.dispatchInfoReturnErrors(e) : [] - if (errorMessages.length > 0) { + if (errorMessages.length > 0 && res && !res.endOfResults) { + // a statement in the batch failed but the batch goes on: report and carry on + this.routeStatementError(errorMessages, this.callback, this.notify, true) + resolve(res) + } else if (errorMessages.length > 0) { reject(errorMessages) } else { this.infoFromNextResult = e != null && Array.isArray(e) && e.length > 0 @@ -429,7 +434,7 @@ class Query extends BasePromises { this.notify.setHandle(res.queryResult.handle) if (res.warning) { - this.routeStatementError(res.warning, this.callback, this.notify) + this.routeStatementError(res.warning, this.callback, this.notify, !res.queryResult.endOfResults) if (!this.cancelled) { res.warning.forEach(err => { if (!this.cancelled) { diff --git a/test/batch-errors.test.js b/test/batch-errors.test.js new file mode 100644 index 00000000..acc22c4c --- /dev/null +++ b/test/batch-errors.test.js @@ -0,0 +1,197 @@ +'use strict' + +// a statement failing part way through a batch (raiserror severity 11 - 16, a failed +// insert, ...) does not end the batch: SQL Server runs the statements after it. the +// driver used to stop at the first error, losing every print, error and result set +// that followed, and never raising 'done'. +// errors that do end the batch (throw, conversion errors, timeouts) must still end it. +// ('done' is not raised when a batch ends on an error - that is unchanged here, so those +// tests don't assert on it either way.) + +import { createRequire } from 'module' +const require = createRequire(import.meta.url) +const { TestEnv } = require('./env/test-env') +const env = new TestEnv() +const assert = require('chai').assert + +const sql = require('../lib/sql') +const { configureTestLogging } = require('./common/logging-helper') +configureTestLogging(sql) + +const strip = message => message.replace(/^(\[[^\]]*\])+/, '') + +const twoErrorsBatch = ` +print 'p1' +raiserror('e1', 16, 1) +print 'p2' +raiserror('e2', 16, 1) +print 'p3' +select 1 as n` + +// every event the query raises, in order +function events (conn, query) { + return new Promise(resolve => { + const ev = [] + const q = conn.query(query) + q.on('info', m => ev.push(`info ${strip(m.message)}`)) + q.on('error', (e, more) => ev.push(`error ${strip(e.message)} more=${more}`)) + q.on('meta', m => ev.push(`meta ${m.map(c => c.name).join(',')}`)) + q.on('column', (c, v) => ev.push(`column ${v}`)) + q.on('done', () => ev.push('done')) + q.on('free', () => resolve(ev)) + }) +} + +// every call of the query's callback, in order +function callbacks (conn, query) { + return new Promise(resolve => { + const calls = [] + conn.query(query, (err, rows, more) => { + calls.push({ err: err ? strip(err.message) : null, rows, more }) + if (!more) resolve(calls) + }) + }) +} + +async function assertUsable (conn) { + const res = await conn.promises.query('select 1 as n') + assert.strictEqual(res.first[0].n, 1) +} + +describe('errors part way through a batch', function () { + this.timeout(30000) + + this.beforeEach(async () => { + await env.open() + }) + + this.afterEach(async () => { + await env.close() + }) + + for (const [label, make] of [['polling', q => sql.PollingQuery(q)], ['non polling', q => q]]) { + it(`${label}: prints, errors and rows after a failed statement all arrive`, async function handler () { + const ev = await events(env.theConnection, make(twoErrorsBatch)) + assert.deepStrictEqual(ev, [ + 'info p1', + 'error e1 more=true', + 'info p2', + 'error e2 more=true', + 'info p3', + 'meta n', + 'column 1', + 'done' + ]) + await assertUsable(env.theConnection) + }) + } + + it('promise rejects with the last error, carrying everything else', async function handler () { + const err = await env.theConnection.promises.query(twoErrorsBatch).then(() => null, e => e) + assert(err, 'expected the query to reject') + assert.strictEqual(strip(err.message), 'e2') + const res = err._results + assert.deepStrictEqual(res.errors.map(e => strip(e.message)), ['e1', 'e2']) + assert.deepStrictEqual(res.info.map(strip), ['p1', 'p2', 'p3']) + assert.deepStrictEqual(res.first, [{ n: 1 }]) + await assertUsable(env.theConnection) + }) + + it('callback: more is true until the batch really ends', async function handler () { + const calls = await callbacks(env.theConnection, twoErrorsBatch) + assert.deepStrictEqual(calls.filter(c => c.err).map(c => c.err), ['e1', 'e2']) + assert(calls.slice(0, -1).every(c => c.more), 'only the last call should have more=false') + const last = calls.at(-1) + assert.strictEqual(last.more, false) + assert.deepStrictEqual(last.rows, [{ n: 1 }]) + }) + + it('result sets either side of an error are both returned', async function handler () { + const err = await env.theConnection.promises.query(` + select 1 as a + raiserror('in the middle', 16, 1) + select 2 as b`).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'in the middle') + assert.deepStrictEqual(err._results.results, [[{ a: 1 }], [{ b: 2 }]]) + }) + + it('a failed insert does not stop the rest of the batch', async function handler () { + const err = await env.theConnection.promises.query(` + declare @t table (id int primary key) + insert into @t values (1) + insert into @t values (1) + insert into @t values (2) + select id from @t order by id`).then(() => null, e => e) + assert.include(err.message, 'PRIMARY KEY') + assert.deepStrictEqual(err._results.first, [{ id: 1 }, { id: 2 }]) + }) + + it('stored procedure: statements after a raiserror run, output and return arrive', async function handler () { + const conn = env.theConnection + const name = 'test_batch_errors_proc' + await conn.promises.query(`if object_id('${name}') is not null drop procedure ${name}`) + await conn.promises.query(` + create procedure ${name} @out int output + as + begin + set nocount on + print 'before' + raiserror('proc failed a step', 16, 1) + print 'after' + set @out = 42 + select 'still here' as status + return 7 + end`) + try { + const err = await conn.promises.callProc(name, {}).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'proc failed a step') + const res = err._results + assert.deepStrictEqual(res.info.map(strip), ['before', 'after']) + assert.deepStrictEqual(res.first, [{ status: 'still here' }]) + assert.deepStrictEqual(res.output, [7, 42]) + await assertUsable(conn) + } finally { + await conn.promises.query(`drop procedure ${name}`) + } + }) + + it('throw ends the batch: nothing after it runs', async function handler () { + const ev = await events(env.theConnection, ` + print 'p1' + ;throw 50001, 'thrown', 1 + print 'never' + select 1 as never`) + assert.deepStrictEqual(ev.filter(e => e !== 'done'), ['info p1', 'error thrown more=false']) + await assertUsable(env.theConnection) + }) + + it('a batch aborting conversion error ends the batch', async function handler () { + const ev = await events(env.theConnection, ` + print 'p1' + select cast('oops' as int) as n + print 'never'`) + assert.strictEqual(ev[0], 'info p1') + assert(ev.some(e => e.startsWith('error') && e.includes('Conversion failed')), ev.join('\n')) + assert(!ev.includes('info never'), ev.join('\n')) + await assertUsable(env.theConnection) + }) + + it('several compile errors are still all reported', async function handler () { + const ev = await events(env.theConnection, 'select no_such_column_a; select no_such_column_b') + assert.deepStrictEqual(ev.filter(e => e !== 'done'), [ + "error Invalid column name 'no_such_column_a'. more=true", + "error Invalid column name 'no_such_column_b'. more=false" + ]) + await assertUsable(env.theConnection) + }) + + it('a timeout ends the batch: the statements after it do not run', async function handler () { + const ev = await events(env.theConnection, { + query_str: "waitfor delay '00:00:10'; print 'never'", + query_timeout: 1 + }) + assert(ev.some(e => e.includes('Query timeout expired')), ev.join('\n')) + assert(!ev.includes('info never'), ev.join('\n')) + await assertUsable(env.theConnection) + }) +}) From 217a7f3df80cf23b6fe83b55b22b31fa5d6eae33 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 11 Oct 2026 13:31:59 +0100 Subject: [PATCH 2/2] fix: keep prints that arrive with a batch's final error; cover unixODBC first-result case - reader.js: info messages in the same diagnostics as an error that ends the batch were dropped (e.g. a print just before a throw inside a proc with nocount on). They are now routed as info as well as the errors being returned. - batch-errors tests: the proc test now produces a result before its raiserror, so it covers continuing after an error on every platform. When the first result of a batch is itself the error, SQLExecDirect fails and the unixODBC driver manager (linux / macOS) answers SQLMoreResults with SQL_NO_DATA without asking the driver, so the batch ends there; on windows it carries on. A new test pins each platform's behaviour, and another checks the print-with-final-error case. Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/reader.js | 8 +++---- test/batch-errors.test.js | 46 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/lib/reader.js b/lib/reader.js index 632a86bc..2b1fac55 100755 --- a/lib/reader.js +++ b/lib/reader.js @@ -144,12 +144,12 @@ class Query extends BasePromises { } }) } - if (errorMessages.length > 0) { - return errorMessages - } else if (infoMessages.length > 0) { + // route info even alongside errors: a print just before a failing statement arrives + // in the same set of diagnostics as the error + if (infoMessages.length > 0) { this.routeStatementError(infoMessages, this.callback, this.notify, false) } - return [] + return errorMessages } async nativeNextResult (queryId) { diff --git a/test/batch-errors.test.js b/test/batch-errors.test.js index acc22c4c..ee87f84d 100644 --- a/test/batch-errors.test.js +++ b/test/batch-errors.test.js @@ -135,6 +135,7 @@ describe('errors part way through a batch', function () { as begin set nocount on + select 'started' as step print 'before' raiserror('proc failed a step', 16, 1) print 'after' @@ -147,7 +148,7 @@ describe('errors part way through a batch', function () { assert.strictEqual(strip(err.message), 'proc failed a step') const res = err._results assert.deepStrictEqual(res.info.map(strip), ['before', 'after']) - assert.deepStrictEqual(res.first, [{ status: 'still here' }]) + assert.deepStrictEqual(res.results, [[{ step: 'started' }], [{ status: 'still here' }]]) assert.deepStrictEqual(res.output, [7, 42]) await assertUsable(conn) } finally { @@ -155,6 +156,49 @@ describe('errors part way through a batch', function () { } }) + // when the very first result of a batch is the error, SQLExecDirect itself fails. on + // linux / macOS the unixODBC driver manager then answers SQLMoreResults with + // SQL_NO_DATA without asking the driver, so the batch ends there. on windows it goes on. + it('error as the first result: windows carries on, unixODBC ends the batch', async function handler () { + const err = await env.theConnection.promises.query(` + raiserror('first thing fails', 16, 1) + print 'after' + select 1 as n`).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'first thing fails') + const res = err._results + if (process.platform === 'win32') { + assert.deepStrictEqual(res.info.map(strip), ['after']) + assert.deepStrictEqual(res.first, [{ n: 1 }]) + } else { + assert.isNull(res.info) + assert.deepStrictEqual(res.results, []) + } + await assertUsable(env.theConnection) + }) + + it('a print in the same diagnostics as the final error is not dropped', async function handler () { + const conn = env.theConnection + const name = 'test_batch_errors_info' + await conn.promises.query(`if object_id('${name}') is not null drop procedure ${name}`) + // inside a proc with nocount on, the print and the throw arrive together + await conn.promises.query(` + create procedure ${name} + as + begin + set nocount on + print 'about to throw' + ;throw 50002, 'thrown in proc', 1 + end`) + try { + const err = await conn.promises.callProc(name, {}).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'thrown in proc') + assert.deepStrictEqual(err._results.info.map(strip), ['about to throw']) + await assertUsable(conn) + } finally { + await conn.promises.query(`drop procedure ${name}`) + } + }) + it('throw ends the batch: nothing after it runs', async function handler () { const ev = await events(env.theConnection, ` print 'p1'