Skip to content

fix: settle msnodesqlv8 bulk loads when the table lookup fails - #1912

Merged
dhensby merged 1 commit into
tediousjs:masterfrom
dhensby:fix/msnodesqlv8-bulk-lookup-error
Oct 13, 2026
Merged

dhensby merged 1 commit into
tediousjs:masterfrom
dhensby:fix/msnodesqlv8-bulk-lookup-error

Conversation

@dhensby

@dhensby dhensby commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Problem

Before loading rows, the msnodesqlv8 driver looks the table up through msnodesqlv8's TableMgr.bind(). When table.create is set, it creates the table first and then looks it up. In every msnodesqlv8 line (v2, v3, v4, 5.2.x, 5.5.x), bind() reports a failed lookup as cb(null, err) from inside a promise .catch. For example, with new sql.Table('nosuchdb.dbo.t') and create off, msnodesqlv8's describe query fails with 208 Invalid object name 'nosuchdb.INFORMATION_SCHEMA.COLUMNS'. With create on, the same name fails earlier, at the create table step (2702).

node-mssql ignored the second argument and read mgr.columns on null. That throw happened inside the .catch, so:

  • bulk() never resolved or rejected;
  • the borrowed connection was never released, so the pool loses one connection per failed load and close() hangs;
  • the TypeError: Cannot read properties of null (reading 'columns') became an unhandled rejection, which is fatal by default on Node 15+.

tedious was not affected: it rejects with EREQUEST and releases the connection.

Fix

The bind() callback now takes (mgr, err) and returns done(err) when there is a lookup error. done() is the path every other bulk failure already uses. It releases the connection and rejects with a RequestError with code EREQUEST. The native error number (208) is kept and the original error stays on originalError.

Tests

New shared test bulk load releases the connection when the table lookup fails, registered in the bulk load block of both the tedious and msnodesqlv8 suites. It loads into notexistingdb.dbo.bulk_table_lookup with create off and asserts that:

  • nothing is left borrowed and the free count is unchanged;
  • the rejection is a RequestError with code EREQUEST;
  • the pool still serves queries afterwards.

It does not assert the message, because the drivers differ there (see caveats).

Verification:

  • npm run lint and npm run test-unit pass.
  • tedious integration on 19.2.2 (lockfile) and 20.3.4: everything passes except the known user defined types failure (no CLR on the local SQL Edge). The new test passes on both.
  • msnodesqlv8 was verified by emulation only: no ODBC is available locally. node-mssql's real lib/msnodesqlv8 ran over a tedious-backed stand-in for the native module, with each connection given the real msnodesqlv8 JS TableMgr of v2.7.0, v3.1.0, v4.5.0, 5.2.1 and 5.5.0. Only the native binding was stubbed.
    • Without the fix, the new test never settles on all five lines and the block's after all close() hangs.
    • With the fix, it passes on all five.
    • The rest of the bulk block's results did not change.
    • The full emulated suite has the same failure list before and after the change.
  • Windows CI is the real check for the msnodesqlv8 side.

Caveats

  • msnodesqlv8 keeps a bulk manager per table name for the lifetime of a connection. The manager holds the columns its lookup found, 5.5.x also caches the table metadata by name, and node-mssql never clears either. A failed lookup is not cached, because describe rejects before anything is stored. A lookup of a table that doesn't exist yet does succeed, though, with 0 columns, and that result is kept. node-mssql reports "Table was not found on the server.", and if the table is created later, loads on the same connection keep failing. This is out of scope here. The new test uses a name no other test uses.
  • The rejection message still differs between drivers:
    • msnodesqlv8 names its own describe query (Invalid object name 'notexistingdb.INFORMATION_SCHEMA.COLUMNS'., ODBC prefix kept).
    • tedious rejects with an empty message: tedious raises an AggregateError for bulk loads, and the 208 errors are in precedingErrors.

Found while bringing the two driver suites in line (#1908, #1910).

🤖 Generated with Claude Code

@dhensby
dhensby force-pushed the fix/msnodesqlv8-bulk-lookup-error branch from d261eb3 to 0e82a80 Compare October 5, 2026 08:02
Before loading rows, the msnodesqlv8 driver looks the table up through
msnodesqlv8's table manager (after creating it, when create is set).
The table manager reports a failed lookup, such as a table in a
database that does not exist, as (null, err) rather than error first,
and the error was ignored. The callback then threw reading the columns
of a null manager, so bulk() never settled, the borrowed connection was
never released, and the TypeError surfaced as an unhandled rejection.

The lookup error now goes back through the same path as any other bulk
failure, which releases the connection and rejects with a RequestError
(code EREQUEST).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dhensby
dhensby force-pushed the fix/msnodesqlv8-bulk-lookup-error branch from 0e82a80 to 8d80e84 Compare October 13, 2026 11:32
@dhensby
dhensby requested a balanced review from Copilot October 13, 2026 12:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused fix uses the established cleanup path and is adequately covered across both drivers.

0 open findings

What changed in this PR

Fixes msnodesqlv8 bulk requests so table lookup failures reject cleanly and release borrowed connections.

Changes:

  • Handles TableMgr.bind() lookup errors through the existing cleanup path.
  • Adds shared regression coverage for connection release and pool recovery.
  • Registers the test for both supported drivers.
File Description
lib/​msnodesqlv8/​request.js Handles the callback’s lookup-error argument.
test/​common/​tests.js Adds the shared regression test.
test/​msnodesqlv8/​msnodesqlv8.js Registers msnodesqlv8 coverage.
test/​tedious/​tedious.js Registers tedious coverage.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@dhensby
dhensby merged commit fe76641 into tediousjs:master Oct 13, 2026
47 checks passed
@dhensby
dhensby deleted the fix/msnodesqlv8-bulk-lookup-error branch October 13, 2026 12:26
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 12.7.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants