[dhensby]

What's wrong

When Transaction.begin() fails, the connection the transaction borrowed from the pool is never given back. This has been the case since v6.

On tedious, _begin borrows a connection, records it, attaches the rollbackTransaction listener, and calls connection.beginTransaction(). It can fail in two ways, and neither gives the connection back:

• tedious throws while it checks the arguments, before anything is sent. One way to get there is setting tx.isolationLevel to a level that doesn't exist and calling begin() with no argument: the base class checks only the argument. The throw happens inside the pool's acquire callback. ConnectionPool.acquire() chains .then(cb).catch(cb), so the same callback runs again with the error, and begin() rejects with tedious's own RangeError/TypeError.
• The request to begin fails: a server error, a timeout, and so on. The error comes back as a TransactionError and nothing else happens.

Either way the connection stays borrowed for good. tarn never reclaims a borrowed resource, so as many failures as the pool has connections use it up. After that, every request waits for a connection until it times out, and pool.close() never finishes, because tarn's destroy() waits for borrowed resources to come back. The transaction keeps _acquiredConnection too, so begin() again gives EALREADYBEGUN.

msnodesqlv8 has the same problem:

• isolationLevelDeclaration() throws inside the acquire callback, before the release-on-error code, so the connection leaks the same way.
• Its error path gives the connection back as it is, which brings in the second problem below.

Why the connection is closed rather than just given back

Once the request to begin has been sent, nothing tells us whether the server began the transaction. Reproduced locally with a proxy that delays what the server sends:

1. begin() times out after the server has already run it.
2. If the connection is just given back, the next borrower gets a session with a transaction already open.
3. Their begin(), inserts and commit() all resolve, but their transaction is nested inside the orphaned one (@@trancount is still 1 after their commit).
4. Nothing they wrote is kept once that connection goes away.

So a connection that may have sent the request is not lent out again. I didn't try to infer the session state from error codes: a failed begin is rare and a reconnect is cheap.

Changes

• tedious: on any failure to begin, the transaction removes its rollbackTransaction listener and clears its connection and config. It gives the connection back to the pool, and calls back once with a TransactionError that keeps the original error as originalError. The transaction can then be begun again.
  • When tedious threw, nothing reached the server, so the connection goes back as it is.
  • When the request failed, the connection is closed first. tedious marks it closed straight away, _poolValidate rejects a closed connection whatever validateConnection is, and _poolDestroy already handles one that is closed.
  • Closing, releasing and calling back happen on the next tick. tedious's cleanupConnection() calls the request's callback before it marks the connection closed, so closing it from inside that callback would call it a second time. The deferral also keeps the caller's callback out of the pool's promise chain, so anything it throws isn't passed back in as a second error.
• msnodesqlv8:
  • The begin query is built before a connection is borrowed.
  • When the query fails, the connection is marked hasError and given back, so the pool closes it rather than lending it out again. It isn't closed directly: msnodesqlv8's close() never calls back for a connection that's already closed (2.x to 5.x), so the pool's own destroy, and pool.close(), would hang.
  • The error is now a TransactionError, as it is with tedious and for msnodesqlv8's commit and rollback.
• tedious pool: _poolDestroy now publishes mssql:connection:destroy for a connection that has already closed, as well as for one it closes itself. Before this, subscribers never saw the destroy for a connection that tedious closed when its socket ended. This branch makes that happen after every failed begin, since the transaction now closes the connection itself. This is its own commit, before the fix.
• README: says what happens to the connection when begin() fails.

Tests

• Unit: a fake tedious connection, plus msnodesqlv8 through the usual require stub, both going through the pool's real acquire(). They cover:
  • a synchronous throw;
  • an async error, where the connection should be closed before it goes back;
  • begin(cb) being called once;
  • a callback that throws after tedious called back synchronously, which should reach the process rather than become a second callback;
  • reusing the transaction afterwards;
  • for msnodesqlv8, that _poolValidate refuses the returned connection whether validateConnection is on or off.
• Live, tedious and msnodesqlv8: an invalid isolationLevel with a pool of max 1, after which the next query should succeed and pool.close() should resolve.
• Live, tedious only: the timeout case through a local TCP proxy that holds back what the server sends for 1.5s (requestTimeout: 300, pool max 1, validateConnection: 'socket'). The next borrower's begin, insert and commit should leave @@trancount at 0, and the row should still be there when read back on a fresh connection. It's registered as skipped in the msnodesqlv8 suite, because msnodesqlv8 timeouts are whole seconds, so msnodesqlv8's failed-query path is covered by the unit tests only.
• Diagnostics unit test: _poolDestroy should publish the destroy for a connection that has already closed.
• Each new test fails against the lib files on master. The timeout test also fails against a version that gives the connection back without closing it (@@trancount is 1).
• Run locally on tedious 19.2.2 and 20.3.7. The only failures are the three known ones from the Azure SQL Edge test server (user defined types, and the spatial and hierarchyid bulk loads). The msnodesqlv8 live tests are left to the Windows CI jobs.

Not changed

• begin() still checks only the isolation level it's given, not the isolationLevel property. Checking the property up front looks tidy, but it would reject values tedious accepts today: undefined, which tedious replaces with the pool's options.isolationLevel, and 0, its "no change" level. I left that for a separate change if it's wanted.
• When the acquire callback throws, ConnectionPool.acquire()'s .then(cb).catch(cb) runs the same callback a second time. That's a separate issue and is left alone here. The transaction no longer throws into it.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The proxy integration test fails for valid configurations that omit the port, and cross-driver lifecycle behavior still needs live integration confirmation.

1 open finding

• Apply the connection pool's default port to the proxy · New

What changed in this PR

Fixes connection leaks after failed transaction begins across both drivers, preventing pool exhaustion and reuse of potentially open transactions.

Changes:

• Releases borrowed connections and discards uncertain sessions after begin failures.
• Publishes destroy diagnostics for already-closed tedious connections.
• Adds regression tests and documents recovery behavior.

File	Description
test/​tedious/​tedious.js	Registers recovery integration tests.
test/​prepare.sql	Creates the transaction persistence fixture.
test/​msnodesqlv8/​msnodesqlv8.js	Registers recovery coverage and skips the timeout case.
test/​common/​unit.js	Tests begin failures and connection disposal.
test/​common/​tests.js	Adds recovery and timeout integration scenarios.
test/​common/​diagnostics.js	Tests already-closed connection diagnostics.
test/​cleanup.sql	Removes the new fixture.
README.md	Documents failed-begin behavior.
lib/​tedious/​transaction.js	Cleans up failed begins and disposes uncertain sessions.
lib/​tedious/​connection-pool.js	Publishes destroy events for closed connections.
lib/​msnodesqlv8/​transaction.js	Validates before borrowing and marks failed connections unusable.

🧠 Review effort: Balanced

---

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

[Copilot]

🔵 Needs a closer look

Driver-specific teardown timing warrants human review and confirmation from Windows msnodesqlv8 integration tests.

0 open findings

1 resolved since last review

• Apply the connection pool's default port to the proxy

🧠 Review effort: Balanced

[github-actions]

🎉 This PR is included in version 12.7.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀