FIX: Prevent pooled connections from retaining empty transactions

[Sumit Sarabhai (sumitmsft)]

AB#48076

Summary

Sanitize physical connections before returning them to the pool by rolling back and restoring autocommit mode. Connections that cannot be sanitized are discarded without consuming pool capacity.

Adds regression coverage that inspects the parked SQL Server session from a separate connection and verifies open_transaction_count is zero.

[Copilot]

🟡 Changes recommended

The new discardConnection path can dereference a null shared_ptr and crash when a pool exists, so it needs a small null-guard fix before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the connection-pooling lifecycle by sanitizing physical connections before they’re parked back into the pool, specifically addressing cases where SQL Server can show an “empty” open transaction on an idle pooled session. It also adds a regression test that validates open_transaction_count from a separate observer connection.

Changes:

• Add a pre-check-in sanitation step (rollback() + restore autocommit) when returning a connection to the pool, and discard unsanitizable connections without consuming pool capacity.
• Make native destructors (Connection, ConnectionHandle) non-throwing to avoid exception propagation during GC/finalization paths.
• Add regression coverage inspecting the parked session’s open_transaction_count, plus a CHANGELOG entry.

File summaries

File	Description
tests/test_009_pooling.py	Adds a subprocess-based regression test ensuring pooled close does not leave an open transaction on the parked SQL Server session.
mssql_python/pybind/connection/connection.h	Marks Connection destructor noexcept and introduces prepareForPool() API for pool sanitation.
mssql_python/pybind/connection/connection.cpp	Implements prepareForPool() and makes Connection destructor non-throwing.
mssql_python/pybind/connection/connection_pool.h	Adds pool/manager APIs to discard a connection and release reserved capacity.
mssql_python/pybind/connection/connection_pool.cpp	Implements discard logic and integrates discard path for sanitation failures.
CHANGELOG.md	Documents the pooling fix as GH-754.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

The Python non-pooled Connection.close() path no longer performs the rollback it claims to, and the new sys.dm_exec_sessions-based test needs permission-error handling to avoid false CI failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core native pooling/cleanup behavior (including destructor and capacity accounting paths) and warrants final human validation across concurrency and platform-specific ODBC behaviors.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 81%

🎯 Overall Coverage 83%

📈 Total Lines Covered: 8585 out of 10251 📁 Project: mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql_python/connection.py (100%)
• mssql_python/pybind/connection/connection.cpp (77.8%): Missing lines 24-25,152-154,192-193,218-220,230-231,233-234,237-242,245-246,356-360,749-751,770,824
• mssql_python/pybind/connection/connection_pool.cpp (93.3%): Missing lines 525-526
• mssql_python/pybind/ddbc_bindings.cpp (78.5%): Missing lines 1595,1602,1611-1612,1616-1617,1622-1623,1625-1626,1628-1629,1649-1650

Summary

• Total: 257 lines
• Missing: 48 lines
• Coverage: 81%

mssql_python/pybind/connection/connection.cpp

Lines 20-29

  20 #include "performance_counter.hpp"
  21 
  22 static bool isPythonFinalizing() {
  23     if (Py_IsInitialized() == 0) {
! 24         return true;
! 25     }
  26 #if PY_VERSION_HEX >= 0x030D0000
  27     return Py_IsFinalizing() != 0;
  28 #else
  29     return _Py_IsFinalizing() != 0;

---

Lines 148-158

  148                 childHandles.reserve(afterCompactSize);
  149                 for (auto& weakHandle : _childStatementHandles) {
  150                     if (auto handle = weakHandle.lock()) {
  151                         if (handle->type() != SQL_HANDLE_STMT) {
! 152                             ++badHandleCount;
! 153                             continue;
! 154                         }
  155                         childHandles.push_back(std::move(handle));
  156                     }
  157                 }
  158             }

---

Lines 188-197

  188         if (!SQL_SUCCEEDED(ret)) {
  189             if (hasGil) {
  190                 checkError(ret);
  191             } else {
! 192                 std::fputs("mssql-python: native disconnect failed\n", stderr);
! 193             }
  194             // Keep ownership and child-handle tracking intact for a cleanup retry.
  195             return;
  196         }
  197         // Log after releasing _childHandlesMutex (#671): LOG()/LOG_ERROR() acquire

---

Lines 214-224

  214     }
  215 }
  216 
  217 void Connection::disconnectNoThrow() noexcept {
! 218     try {
! 219         if (isPythonFinalizing()) {
! 220             abandonDuringFinalization();
  221             return;
  222         }
  223         if (!_dbcHandle) {
  224             return;

---

Lines 226-250

  226         // disconnect() already supports GIL-less cleanup. Drop the GIL once so
  227         // neither its diagnostics nor handle destruction can enter Python.
  228         if (PyGILState_Check()) {
  229             py::gil_scoped_release release;
! 230             disconnect(true);
! 231         } else {
  232             disconnect(true);
! 233         }
! 234     } catch (...) {
  235         std::fputs("mssql-python: unexpected failure during native connection cleanup\n", stderr);
  236     }
! 237 }
! 238 
! 239 void Connection::abandonDuringFinalization() noexcept {
! 240     {
! 241         std::lock_guard<std::mutex> lock(_childHandlesMutex);
! 242         _childStatementHandles.clear();
  243         _allocationsSinceCompaction = 0;
  244     }
! 245     // SqlHandle::free() already suppresses SQLFreeHandle during finalization.
! 246     // Clearing the shared pointer leaves process teardown to the operating system.
  247     _dbcHandle.reset();
  248 }
  249 
  250 // TODO(microsoft): Add an exception class in C++ for error handling,

---

Lines 352-364

  352         }
  353         updateLastUsed();
  354         SQLHANDLE stmt = nullptr;
  355         SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt);
! 356         if (!SQL_SUCCEEDED(ret)) {
! 357             // Snapshot diagnostics before disconnect can overwrite/free the DBC.
! 358             ErrorInfo err = SQLReadError(SQL_HANDLE_DBC, _dbcHandle->get(), ret);
! 359             ThrowStdException(err.sqlState.length() == 5
! 360                                   ? "SQLSTATE:" + err.sqlState + ":" + err.ddbcErrorMsg
  361                                   : err.ddbcErrorMsg);
  362         }
  363         stmtHandle = std::make_shared<SqlHandle>(static_cast<SQLSMALLINT>(SQL_HANDLE_STMT),
  364                                                 stmt, _cleanupState);

---

Lines 745-755

  745             _conn->abandonDuringFinalization();
  746             _conn = nullptr;
  747             return;
  748         }
! 749         try {
! 750             // Discard ends abandoned work without returning this connection to
! 751             // the pool or entering Python from a native destructor.
  752             ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn);
  753         } catch (...) {
  754             std::fputs("mssql-python: failed to release native connection pool capacity\n", stderr);
  755             _conn->disconnectNoThrow();

---

Lines 766-774

  766         try {
  767             _conn->prepareForPool(transactionAlreadyRolledBack);
  768         } catch (...) {
  769             // Never retain a connection whose transaction state could not be
! 770             // sanitized. Discarding also releases this connection's reserved
  771             // pool capacity. Preserve the original check-in error.
  772             try {
  773                 ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn);
  774             } catch (...) {

---

Lines 820-828

  820     auto conn = _conn;
  821     if (!conn) {
  822         ThrowStdException("Connection object is not initialized");
  823     }
! 824     return conn->allocStatementHandle();
  825 }
  826 
  827 py::object Connection::getInfo(SQLUSMALLINT infoType) const {
  828     if (!_dbcHandle) {

---

mssql_python/pybind/connection/connection_pool.cpp

Lines 521-530

  521 }
  522 
  523 void ConnectionPoolManager::discardConnection(
  524     const std::weak_ptr<ConnectionPool>& originating_pool,
! 525     const std::shared_ptr<Connection> conn) {
! 526     if (!conn) {
  527         return;
  528     }
  529     std::shared_ptr<ConnectionPool> pool = originating_pool.lock();
  530     if (pool) {

---

mssql_python/pybind/ddbc_bindings.cpp

Lines 1591-1599

  1591 void SqlHandle::free() {
  1592     freeHandle();
  1593 }
  1594 
! 1595 SQLRETURN SqlHandle::freeHandle() {
  1596     PERF_TIMER("SqlHandle::free");
  1597     bool pythonShuttingDown = is_python_finalizing();
  1598     bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC;
  1599 #ifdef _WIN32

---

Lines 1598-1606

  1598     bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC;
  1599 #ifdef _WIN32
  1600     // The static ENV is destroyed during DLL_PROCESS_DETACH, after Python
  1601     // finalization. Calling ODBC then can access already-torn-down SSPI state.
! 1602     skipDuringShutdown = skipDuringShutdown || _type == SQL_HANDLE_ENV;
  1603 #endif
  1604     if (pythonShuttingDown && skipDuringShutdown) {
  1605         // Do not wait for another thread's ODBC cleanup during finalization.
  1606         // Process teardown owns any resources not released by atexit cleanup.

---

Lines 1607-1633

  1607         _handle = nullptr;
  1608         return SQL_SUCCESS;
  1609     }
  1610 
! 1611     auto freeNative = [this]() -> SQLRETURN {
! 1612         auto cleanupLock = lockForCleanup();
  1613         if (!_handle || !SQLFreeHandle_ptr) {
  1614             return SQL_INVALID_HANDLE;
  1615         }
! 1616         describeCache.clear();
! 1617         if (_implicitly_freed || (_cleanupState && _cleanupState->disconnected)) {
  1618             _handle = nullptr;
  1619             return SQL_SUCCESS;
  1620         }
  1621         SQLRETURN ret = SQLFreeHandle_ptr(_type, _handle);
! 1622         if (SQL_SUCCEEDED(ret)) {
! 1623             _handle = nullptr;
  1624         }
! 1625         return ret;
! 1626     };
  1627     // The same gate is held through SQLDisconnect and child invalidation.
! 1628     // Release the GIL before waiting, and unlock before reacquiring it.
! 1629     if (!pythonShuttingDown && PyGILState_Check()) {
  1630         py::gil_scoped_release release;
  1631         return freeNative();
  1632     }
  1633     return freeNative();

---

Lines 1645-1654

  1645         }
  1646         if (!SQLFreeStmt_ptr) {
  1647             ThrowStdException("SQLFreeStmt function not loaded");
  1648         }
! 1649         return SQLFreeStmt_ptr(_handle, SQL_CLOSE);
! 1650     };
  1651     SQLRETURN ret;
  1652     if (PyGILState_Check()) {
  1653         py::gil_scoped_release release;
  1654         ret = closeNative();

---

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 78.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

🔵 Needs a closer look

It changes native connection lifecycle and pooling semantics (including teardown behavior and capacity accounting), which can have subtle cross-platform/concurrency impacts best validated with final human review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

A newly added pooling regression test can incorrectly skip (exit 77) when the subject session is unexpectedly not visible, which can mask real regressions and should be turned into an assertion failure.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tests/test_009_pooling.py:190

• This early guard treats open_transaction_count(...) is None as a reason to skip, but None here means the subject session_id wasn’t visible/found (unexpected) rather than a permission problem (which already raises and is handled in open_transaction_count). Skipping in this case can mask a real pooling/session lifecycle regression; it should fail the test with a clear assertion instead of exiting 77.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved transaction-sanitization and cleanup issues block safe approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

mssql_python/connection.py:2165

• For every pooled manual-commit connection, Python close() already performs SQLEndTran(SQL_ROLLBACK) and the native check-in immediately performs another rollback in prepareForPool(). That adds a redundant blocking ODBC round trip to the hot close path; let the native pooled check-in own this rollback (while preserving the non-pooled/error-mapping behavior) or otherwise avoid issuing it twice.

                rollback_error = None
                if not self.autocommit:
                    # End caller work before native close. Pooled connections are
                    # additionally restored to autocommit by native check-in,
                    # which atomically discards them if sanitation fails.

tests/test_006_exceptions.py:289

• _pooling is only Python bookkeeping and is not consulted by Connection.close; changing it after construction cannot change the native ConnectionHandle's _usePool value. Because the native Connection is mocked here, this assignment does not exercise the non-pooled native close/destructor path claimed by the test name and docstring; disable pooling before construction or add a test against the actual native handle.

    conn._pooling = False

• Files reviewed: 8/8 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Four moderate findings remain unresolved, covering duplicate rollback, shutdown safety, and test reliability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

mssql_python/pybind/connection/connection.cpp:688

• This destructor can run during interpreter shutdown or from a GIL-less finalizer, but the new close() call now enters pooled prepareForPool(), which invokes getAutocommit(), rollback(), and setAutocommit(). Those paths use Python logging and py::gil_scoped_release without the shutdown guards used by Connection::disconnect(), so an unclosed pooled handle can call into CPython after finalization and crash or hang. Use a shutdown-safe no-throw destructor path that avoids pooled sanitation while Python is finalizing.

        try {
            close();

tests/test_006_exceptions.py:289

• Assigning _pooling = False after connect() does not make the native ddbc_bindings.Connection non-pooled: the pooling flag was already passed to the native constructor, and Connection.close() never consults this Python attribute. As written, this test does not cover the non-pooled native path despite its name; disable/mock pooling before construction or rename the test to reflect the behavior it actually exercises.

    conn._pooling = False

tests/test_009_pooling.py:923

• This loop assumes KILL must make the next query fail, but the repository documents that ODBC Idle Connection Resiliency can transparently re-establish a dropped pooled session (connection.cpp:454-458), and the neighboring test explicitly accounts for that outcome. A successful reconnect makes this test wait 10 seconds and fail, or lets victim.close() succeed and trigger the assertion below, so disable reconnect for this test or use a deterministic sanitation failure instead of requiring KILL to surface an exception.

                victim.cursor().execute("SELECT 1").fetchone()
            except Exception:
                break
            if time.monotonic() >= deadline:
                raise AssertionError("KILL did not terminate the victim connection")

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The exception test changes pooling after native construction and does not exercise the intended non-pooled close path.

Review details

Suppressed comments (1)

tests/test_006_exceptions.py:289

• conn._pooling = False only changes Python-side bookkeeping after the native ConnectionHandle has already been constructed; Connection.close() does not consult this field, and the mock cannot change the constructor's original use_pool argument. Therefore this test does not exercise the non-pooled native close path described by its name. Construct the connection with pooling disabled (or patch the pooling state before connect()) instead of mutating this field afterward.

    conn._pooling = False

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

A moderate test-skipping issue remains, and the pooling lifecycle changes warrant human review.

Review details

Suppressed comments (1)

tests/test_009_pooling.py:992

• This skip condition treats any error message containing kill as a permission failure, so an invalid SPID, syntax error, or other KILL failure can silently skip the regression instead of failing it. Restrict the skip to an actual permission error (or a specific SQLSTATE) so the test still detects unexpected server-side failures.

            if "permission" in message.lower() or "kill" in message.lower():

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The changes span native connection lifecycle and pooling cleanup paths, warranting final human review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Gaurav Sharma (bewithgaurav)]

normal close now leaves pooled sessions transaction-clean, but abandoned native connections can still retain locks and hang shutdown. requesting changes for that cleanup path.

[Copilot]

🟡 Changes recommended

A critical concurrent child-handle cleanup race remains unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A critical statement-handle lifecycle race and two regression-test issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

tests/test_009_pooling.py:1000

• This skip guard only recognizes one exact KILL permission message. SQL Server can report denied KILL access with other permission wording, which will make this regression test fail instead of taking the documented skip path (the earlier KILL test already handles this broadly). Match the broader permission condition here so environments without KILL permission remain skippable.

            if "does not have permission to use the kill statement" in message.lower():

tests/test_009_pooling.py:1262

• These barriers do not make GC concurrent with disconnect: the worker finishes gc.collect() and waits at its second barrier before the main thread passes this barrier and calls native.close(). Consequently no child wrapper can be collected during disconnect, so this regression test does not exercise the race that the new lifetime-retention code is intended to fix. Coordinate the close with collection (or add a deterministic hook) so the two operations overlap.

                    barrier.wait()
                    if explicit_close:
                        native.close()

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two unresolved critical native cleanup concurrency issues remain.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Lite

[Sumit Sarabhai (sumitmsft)]

Thanks for flagging this. I am leaving the conversation open because the shared-mutex contention is real, but the dependency needed to establish the proposed deadlock has not yet been demonstrated.

In the documented reader-close path, _ArrowReader.close() calls _cancel() before closing the generator; it does not call Connection.close() or SQLDisconnect. Fetch/execute do not acquire the cleanup mutex, and the active reader retains its cursor and connection. A separate connection disconnect concurrent with fetching is therefore not the same as the supported cross-thread reader-cancellation sequence.

That does not rule out a driver-dependent case, particularly if cleanup of an abandoned sibling cursor blocks behind the active fetch while holding the shared gate. Could you provide a bounded reproduction, including driver/version and native stacks showing which ODBC call holds the gate and waits for the operation that cancellation must interrupt?

The gate also protects cancellation from using an invalidated statement handle. Removing it, or simply skipping cancellation when it is contended, could trade this potential hang for a handle-lifetime bug or a lost cancellation. I have not made that change without evidence of the failing sequence and a solution that preserves both guarantees. This is a request for substantiation, not a claim that the concern is impossible or already fixed.

[Copilot]

🔵 Needs a closer look

Native connection cleanup and concurrency changes require final human review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite