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: 8639 out of 10295 📁 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 1596,1603,1612-1613,1617-1618,1623-1624,1626-1627,1629-1630,1650-1651

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 1592-1600

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

---

Lines 1599-1607

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

---

Lines 1608-1634

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

---

Lines 1646-1655

  1646         }
  1647         if (!SQLFreeStmt_ptr) {
  1648             ThrowStdException("SQLFreeStmt function not loaded");
  1649         }
! 1650         return SQLFreeStmt_ptr(_handle, SQL_CLOSE);
! 1651     };
  1652     SQLRETURN ret;
  1653     if (PyGILState_Check()) {
  1654         py::gil_scoped_release release;
  1655         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: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
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.row.py: 83.4%
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

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

[Copilot]

🟡 Changes recommended

Fix the critical SQLResetStmt_wrap cleanup race before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[gargsaumya]

Reviewed the latest revision. The previously reported cleanup and allocation races have been addressed. The remaining SQLResetStmt_wrap thread is defensive hardening for unsupported execute-vs-close concurrency and is not a blocking regression from this PR. All checks pass. No additional findings from my review.

[Gaurav Sharma (bewithgaurav)]

lgtm, things are addressed

[github-actions]

PR Performance Report

Performance assessment pending.

Waiting for the matching performance run for head 5c57601612066862cc26e04cceba36e1666c3ea6.

[Copilot]

🟡 Changes recommended

Resolve the pool shutdown race and synchronize SQLResetStmt_wrap with disconnect cleanup.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

mssql_python/pybind/ddbc_bindings.cpp:1673

• The new cleanup gate still does not cover SQLResetStmt_wrap: that path reads the statement handle and calls SQLFreeStmt(SQL_CLOSE), SQLFreeStmt(SQL_RESET_PARAMS), and SQLSetStmtAttr directly. A cursor reset/finalizer concurrent with disconnectNative() can therefore use the HSTMT while the parent is invalidating it, bypassing the synchronization added here. Route all reset operations through the same cleanup gate and disconnected-state checks.

        auto cleanupLock = lockForCleanup();
        if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || !SQLCancel_ptr ||
            (_cleanupState && _cleanupState->disconnected)) {

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

[Copilot]

🔵 Needs a closer look

SQLResetStmt_wrap still lacks the required cleanup synchronization around statement reset operations.

Review details

Suppressed comments (1)

mssql_python/pybind/ddbc_bindings.cpp:1673

• The cleanup gate introduced here still does not cover SQLResetStmt_wrap immediately below: that wrapper reads the HSTMT and issues SQLFreeStmt(SQL_CLOSE/SQL_RESET_PARAMS) without acquiring lockForCleanup() or checking the shared disconnected state. A concurrent connection disconnect can therefore invalidate or implicitly free the statement while soft reset is using it, so the new handle-lifetime synchronization remains incomplete. Route the reset validation and all three ODBC calls through the same cleanup gate, as discussed in the existing thread.

        auto cleanupLock = lockForCleanup();
        if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || !SQLCancel_ptr ||
            (_cleanupState && _cleanupState->disconnected)) {

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

[Copilot]

Copilot review overview

🔵 Needs a closer look

Native connection-pool lifecycle and cleanup changes require final human review.

Review effort: Lite
Findings: None

Resolved since last review (1)

• The registration check and pool->release(conn) are not atomic with closePools(). A concurrent…