FIX: Prevent connection pool size drift when close races failed open (#746)

[RKS (rksharma-owg)]

Work Item / Issue Reference

GitHub Issue: #746

---

Summary

This pull request addresses issue #746: connection pool can briefly exceed max_size when pooling is disabled or closed under load.

Root Cause

In ConnectionPool::acquire(), when a thread prepares to open a new connection, it increments _current_size under the mutex to reserve a slot before connecting outside the lock (Phase 3). If connection creation fails, the catch (...) block decrements _current_size to release the reservation.
However, if ConnectionPool::close() (invoked by close_pooling() or pool disposal) runs while that thread is attempting connection creation outside the lock, close() removes all idle connections and resets _current_size = 0. If a new thread subsequently calls acquire(), it successfully reserves a slot (_current_size = 1). When the first failing thread finally enters its catch (...) block, its unconditional --_current_size decrements the counter to 0, wiping out the new thread's reservation. As a result, _current_size drifts below the true number of active connections, allowing the pool to exceed max_size under concurrent load.

Solution

• Added a uint64_t _generation = 0; member to ConnectionPool.
• In ConnectionPool::close(), ++_generation; is executed whenever the pool is reset alongside _current_size = 0.
• In ConnectionPool::acquire(), the current _generation is recorded into a local reservation_generation variable when reserving a slot (++_current_size).
• In the Phase 3 catch (...) block, _current_size is decremented only if _generation == reservation_generation, ensuring that a failed connection attempt from a previous pool generation never decrements a reservation made in a newer generation.

Testing

• Added regression test test_pool_size_accounting_race_on_close_interleave in tests/test_009_pooling.py. The test simulates the exact interleaving (using Connection factory hooks in a subprocess with max_size=1) and asserts that a failed open in an older generation cannot erase a new generation's slot reservation or allow subsequent connections beyond max_size.
• Verified the test passes locally and all formatting / linting guidelines pass.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

A critical reservation-accounting race remains, and the header dependency and regression-test issues should be addressed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes connection-pool reservation drift when pool closure races a failed connection open.

Changes:

• Adds generation tracking to pool reservations and reset handling.
• Guards failed-open cleanup against stale generations.
• Adds subprocess regression coverage and environment handling updates.

Review findings:

• connection_pool.cpp — critical, 1 vote: Phase 2 candidate cleanup remains unguarded and can decrement a newer reservation after close().
• connection_pool.h — moderate, 1 vote: Add <cstdint> for a self-contained uint64_t declaration.
• tests/test_009_pooling.py — nit, 3 votes: The test creates a new pool after manager reset and does not exercise the generation fix.

File summaries

File	Summary
tests/test_009_pooling.py	Adds race regression coverage and subprocess environment handling.
mssql_python/pybind/connection/connection_pool.h	Adds pool generation state.
mssql_python/pybind/connection/connection_pool.cpp	Tracks generations and conditionally releases reservations.

Review details

Suppressed comments (1)

mssql_python/pybind/connection/connection_pool.h:48

• The new member uses uint64_t, but this header does not include <cstdint> and instead relies on a transitive include from connection.h/pybind11. That makes the header non-self-contained and can break a platform or translation unit when those unrelated includes change; add <cstdint> alongside the other standard-library includes.

    uint64_t _generation = 0;  // Pool reset generation for reservation attribution (#746)

• Files reviewed: 3/3 changed files
• Comments generated: 2
• 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]

🔵 Needs a closer look

Two moderate issues remain in generation handling and test-seam connection lifecycle.

Review details

Suppressed comments (2)

mssql_python/pybind/connection/connection_pool.cpp:298

• This generation check only covers the Phase 3 reservation. A pooled candidate popped in Phase 2 is validated outside the mutex, and its failure still unconditionally decrements _current_size at lines 233-235; if close() resets the pool during that validation, a newer reservation can be decremented in the same way. Track the candidate's generation (or otherwise attribute that decrement) as part of this fix.

            {
                std::lock_guard<std::mutex> lock(_mutex);
                if (_generation == reservation_generation && _current_size > 0) {
                    --_current_size;
                }

mssql_python/pybind/ddbc_bindings.cpp:6134

• The test seam drops the std::shared_ptr<Connection> returned by pool.acquire(). On any successful acquire, the Connection is destroyed immediately but _current_size is never decremented via release(), so this exposed class permanently consumes a slot and no longer models the real pool lifecycle. Please provide a way for the seam to retain/release the acquired connection (or otherwise balance the reservation) before using it for successful-acquire tests.

            [](ConnectionPool& pool, const std::u16string& connStr,
               const py::object& token_factory) {
                pool.acquire(connStr, py::dict(), token_factory);
            },

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

[Copilot]

🟡 Changes recommended

Invalidated-generation candidates can still bypass capacity accounting and allow connections beyond max_size.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[RKS (rksharma-owg)]

Update on Copilot Review Feedback

All points from Copilot review #5242055056 have been addressed and verified:

1. Candidate Validation & Overflow Disconnect Generation Guarding (commit e67da28e):

   • Popped candidates now record candidate_generation = _generation under _mutex, and candidate validation cleanup (lines 233-235) as well as token rotation sibling drain are guarded with if (_generation == candidate_generation && _current_size > 0).
   • ConnectionPool::release() now records release_generation = _generation to guard overflow disconnect decrements.
   • Added regression test test_pool_size_accounting_race_on_candidate_validation_close_interleave in tests/test_009_pooling.py testing this exact interleave.

2. Test Seam Connection Retain & Release Support (commit 6f8369c6):

   • Registered _TestPooledConnection in ddbc_bindings.
   • _TestConnectionPool::acquire now returns the acquired connection rather than dropping it.
   • Exposed _TestConnectionPool::release(conn) so connections can be returned to the pool for normal lifecycle balancing.

[Copilot]

🟡 Changes recommended

Successful old-generation opens after close() can still bypass accurate pool-size accounting.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[RKS (rksharma-owg)]

Update on Copilot Review Feedback (Iteration 3)

All feedback regarding candidate reuse success and connect success racing with close() has been addressed in commit 5282437c:

1. Candidate Reuse Success Generation Guarding:

   • In Phase 2, when candidate validation succeeds (reuse_candidate == true), we now verify under _mutex that _generation == candidate_generation.
   • If the generation changed (i.e. close() was called during validation outside the lock), the candidate belongs to an wiped generation and is discarded into to_disconnect. The acquire loop retries to reserve capacity and connect under the new generation.
   • Added regression test test_pool_size_accounting_race_on_successful_candidate_reuse_close_interleave in tests/test_009_pooling.py.

2. Connect Success Generation Guarding:

   • In Phase 3, after connect() succeeds outside _mutex, we check under _mutex that _generation == reservation_generation.
   • If close() ran during connection establishment (_generation != reservation_generation), the slot reservation was wiped. The newly opened connection is discarded into to_disconnect, and acquire() retries under the current generation.
   • Added regression test test_pool_size_accounting_race_on_successful_open_close_interleave in tests/test_009_pooling.py.

3. Deterministic Test Seam:

   • Added mock capability (set_mock_mode) to _TestConnectionPool and Connection to allow deterministic end-to-end race verification without requiring external database dependencies.

All 4 race regression tests pass cleanly in local verification.

[Copilot]

🟡 Changes recommended

Critical pool-race and regression-test issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[RKS (rksharma-owg)]

Update on Copilot Review Feedback (Iteration 4)

All feedback from review #5242178325 has been addressed in commit 2753fb77:

1. Atomic Publication Under Mutex:
   • Both reused candidates (Phase 2) and newly opened connections (Phase 3) now assign valid_conn under _mutex within the same lock block as the generation check, eliminating the check-then-publish race with concurrent close().
2. Origin Tracking on Connection & Release Protection:
   • Added _originPool and _originGeneration tracking to Connection.
   • Connections stamp their originating pool pointer and generation when acquired or injected.
   • In ConnectionPool::release(conn), if !conn->matchesPoolOrigin(this, _generation), the connection is detected as stale/invalidated: it is not added to _pool, does not decrement the new generation's _current_size, and is cleanly disconnected outside the mutex.
   • Added regression test test_pool_release_from_stale_generation_does_not_pollute_pool in tests/test_009_pooling.py.
3. Pruning Prevention in Candidate Tests:
   • Initialized _lastUsed to steady_clock::now() in Connection::Connection.
   • Updated _TestConnectionPool::inject_candidate to explicitly touch updateLastUsed() and stamp pool origin, preventing Phase 1 from prematurely pruning injected candidates before Phase 2 validation.

All 5 race regression tests pass locally in 0.18s.

[Copilot]

🟡 Changes recommended

Unresolved critical issues affect pool identity safety, capacity enforcement, and concurrent connection removal.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[RKS (rksharma-owg)]

Iteration 5 Review Updates (Commit c4cdde54)

All 4 review items raised in the latest review have been addressed:

1. Prevent ABA Address-Reuse Invalidation:

   • Replaced pointer comparison with a process-wide monotonic pool ID counter (static std::atomic<uint64_t> s_next_pool_id{1}).
   • Updated Connection to record _originPoolId (uint64_t) and _originGeneration (uint64_t).
   • Added test_pool_release_after_pool_recreation to ensure releasing a connection to a newly recreated pool instance never corrupts or decrements the new pool's size.

2. Fix Sibling Drain Interleaving Race:

   • Guarded the entire sibling drain block (_pool.erase(std::remove_if(...))) with if (_generation == candidate_generation), ensuring that sibling connections from a newly initialized generation are never removed if a pool close/reset interleaved during token validation.

3. Retain Checked-Out Capacity Across Close (Connection pool can briefly exceed max_size when pooling is disabled under load #746):

   • Added _checked_out tracking to ConnectionPool.
   • In ConnectionPool::close(), reserved capacity is retained for checked-out connections (_current_size = _checked_out) across the generation bump, preventing new acquisitions in the new generation from exceeding max_size while previous-generation connections remain live.
   • In ConnectionPool::release(), when an old-generation connection is disconnected, both _checked_out and _current_size are decremented once it belongs to the pool.
   • Updated test_pool_release_from_stale_generation_does_not_pollute_pool to assert that new acquisitions are blocked while old connections are checked out, and succeed immediately once the old connections are released.

[Copilot]

🟡 Changes recommended

A critical close/open race can still temporarily exceed max_size while an older connection attempt is in flight.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[Copilot]

🟡 Changes recommended

A critical pool-replacement lifecycle issue remains, and several regression tests do not reliably validate the intended fixes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

mssql_python/pybind/connection/connection_pool.cpp:475

• close() removes the idle connections from _pool and immediately reduces _current_size to only _checked_out + _in_flight, but the connections in to_close are not disconnected until afterward. A concurrent acquire() can therefore reserve those newly freed slots and open physical connections while the popped idle handles are still live, temporarily exceeding max_size (the same invariant this change is intended to preserve). Keep the to_close capacity reserved until disconnection completes, or otherwise synchronize new reservations with that cleanup.

        _current_size = _checked_out + _in_flight;
        ++_generation;

tests/test_009_pooling.py:1383

• The final count assertions also pass if the Phase 3 generation check is removed: with in-flight capacity retained, the old-generation connection can be published alongside B and the totals still reach 2 without exceeding max_size. Add an observable assertion (for example, count A's factory calls or identify the returned mock connection) proving that A disconnects its stale result and retries under the new generation.

        # 2. OLD-GENERATION OPEN COMPLETES SECOND:
        # Thread A finishes connecting outside the lock.
        # Under generation fix, Thread A detects reservation generation mismatch (0 != 1),
        # disconnects its stale connection, decrements in_flight (1 -> 0) and current_size (2 -> 1).
        # Thread B's connection is intact and valid!
        # Thread A retries acquire under generation 1 and successfully acquires the freed slot.
        release_factory_a.set()
        t_a.join(timeout=5.0)
        assert len(t_a_error) == 0
        assert len(t_a_conn) == 1
        assert pool.checked_out == 2
        assert pool.in_flight == 0
        assert pool.current_size == 2

tests/test_009_pooling.py:1006

• This scenario constructs the pool with max_size=2, while close() now deliberately retains Thread A's in-flight reservation. If the generation check is removed from the Phase 3 failure cleanup, the counter still goes from 2 to 1 (releasing A's own reservation), so this test passes unchanged and does not exercise the claimed generation-attribution fix. Please change the interleave/assertions so the test fails without the guard (or explicitly test the retained-reservation invariant instead).

        pool = ddbc_bindings._TestConnectionPool(2, 600)

tests/test_009_pooling.py:1098

• This failure-path regression also uses max_size=2, so the in-flight reservation retained across close() means an unconditional cleanup decrement releases exactly the popped candidate's capacity (2 -> 1). The test therefore passes even if the generation guard is removed. Add an assertion that distinguishes the old-generation cleanup from a newer reservation, rather than only checking aggregate counts.

        pool = ddbc_bindings._TestConnectionPool(2, 600)

tests/test_009_pooling.py:1509

• The release regression only inspects pool counters. If the generation/origin check is removed, release(conn_1) can put the stale connection back into the idle deque with the same _current_size, and every assertion here still passes because the next acquire simply reuses it. Add an identity or factory-call assertion proving conn_2 is a newly opened connection rather than the stale conn_1.

        pool.release(conn_1)

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

[Copilot]

🟡 Changes recommended

A critical race can temporarily exceed the configured pool size, and regression tests do not fully validate generation protection.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

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

tests/test_009_pooling.py:1070

• This test does not distinguish the claimed generation-attributed cleanup from the existing in-flight retention logic: after close() the counter is 1, Thread B makes it 2, and Thread A's unconditional cleanup reaches 1 whether a generation check is present or not. Consequently it would still pass if the failure-path generation protection were removed, so it cannot regress the original accounting bug as documented. Make the scenario/assertion fail without the intended fix (or update the test documentation to cover only in-flight retention).

mssql_python/pybind/connection/connection_pool.cpp:555

• The current-key replacement does not use the use_count() guard applied by the sweep at lines 542-543. Thread A can copy this pool while holding _manager_mutex and pause before calling pool->acquire(); Thread B then sees canEvict() true, erases the pool, and creates a replacement. A resumes on the orphaned pool and can open up to max_size connections while later callers open another max_size on the replacement. Require that no external pool reference exists before replacing this entry.

        if (it != _pools.end() && it->second && it->second->canEvict()) {

mssql_python/pybind/connection/connection_pool.cpp:670

• This final cleanup also erases a pool based only on canEvict(), even though to_close still holds a reference and an acquirer can hold another reference after the first close() returns. That acquirer can continue opening on the erased pool while a later call creates a replacement for the same key, defeating the capacity sharing. Retain the map entry while any external references exist, accounting for the snapshot reference held by to_close.

        for (auto it = _pools.begin(); it != _pools.end();) {
            if (!it->second || it->second->canEvict()) {
                it = _pools.erase(it);
            } else {

mssql_python/pybind/connection/connection_pool.cpp:475

• close() removes idle connections from _pool and immediately drops their contribution from _current_size, but the physical disconnects run only afterward at lines 477-483. An acquire racing in that window can reserve and open a replacement while the old idle handle is still connected, temporarily exceeding the configured physical-connection limit. Keep these close-in-progress handles accounted for until each disconnect completes, then release their reservations under the mutex.

        // Retain reserved capacity for checked-out connections and in-flight opens
        // so a new acquire cannot exceed _max_size while old connections or opens
        // are still live (#746).
        _current_size = _checked_out + _in_flight;
        ++_generation;

tests/test_009_pooling.py:1101

• This candidate-validation regression has the same coverage gap as the failed-open test: because close() retains the popped candidate in _in_flight, the unconditional cleanup at the end of validation releases that candidate's own reservation, so the current_size == 1 assertion still passes without any generation guard. The test does not actually prove that an old-generation validation failure cannot decrement a newer reservation; make the interleave distinguish those cases or narrow the test's stated contract.

        pool = ddbc_bindings._TestConnectionPool(2, 600)
        # Inject an expired candidate into the idle pool
        pool.inject_candidate("SERVER=dummy_test_746;", 1)
        assert pool.current_size == 1

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

[RKS (rksharma-owg)]

Iteration 8 Update: In-Flight Teardown Accounting & Reference-Guarded Eviction

Pushed commit f0c8804c addressing review 5242600931 / comment 4042424690:

1. In-Flight Teardown Accounting in release() and close():

   • In release(), when disconnecting a stale or overflow connection, the connection is moved from _checked_out to _in_flight (--_checked_out; ++_in_flight;) before dropping _mutex. Only after conn->disconnect() completes outside the lock are _in_flight and _current_size decremented.
   • In close(), closing idle connections are added to _in_flight (_in_flight += to_close.size()), and each connection decrements _in_flight and _current_size under the lock only after its physical conn->disconnect() completes.
   • Result: Physical sockets are never detached from capacity accounting while tearing down, preventing concurrent acquires from observing freed capacity and exceeding max_size (Connection pool can briefly exceed max_size when pooling is disabled under load #746).

2. use_count() == 1 Guards on Eviction and Replacement:

   • In ConnectionPoolManager::acquireConnection(), current-key replacement checks it->second.use_count() == 1 && it->second->canEvict(), preventing pool recreation if another thread holds a reference.
   • In ConnectionPoolManager::closePools(), to_close.clear() is called before the final sweep, and pool erasure guards on it->second.use_count() == 1 && it->second->canEvict().

3. Docstrings & Regression Tests:

   • Updated docstrings for test_pool_size_accounting_race_on_close_interleave and test_pool_size_accounting_race_on_candidate_validation_close_interleave.
   • Added test_pool_release_disconnect_keeps_in_flight_until_disconnected and test_pool_close_disconnect_keeps_in_flight_until_disconnected via a deterministic set_on_disconnect_hook seam, verifying in_flight and current_size remain accounted during disconnect and block concurrent acquires.

[Copilot]

🟡 Changes recommended

Three critical findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

mssql_python/pybind/connection/connection_pool.cpp:193

• The sibling drain similarly removes connections and decrements _current_size before adding any in-flight teardown accounting, while the actual disconnects are deferred to Phase 4. If another acquire runs before those siblings are disconnected, it can open into the slots they still occupy and temporarily exceed the configured physical-connection cap. Transition drained siblings into _in_flight and release that capacity only after each disconnect finishes.

                                                if (sibling->currentAccessToken() == stale_token) {
                                                    to_disconnect.push_back(sibling);
                                                    if (_current_size > 0) {
                                                        --_current_size;
                                                    }

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

[RKS (rksharma-owg)]

Iteration 9 Update: Phase 1 & Sibling Drain In-Flight Capacity, Serialized Replacement Pool, and GIL-Safe Callbacks

Pushed commit fa8ae17c addressing review 5243381227:

1. Phase 1 and Sibling Drain In-Flight Teardown Accounting:

   • In acquire() Phase 1, pruned stale idle connections are transitioned to _in_flight (_in_flight += pruned;) instead of dropping _current_size prematurely.
   • In Phase 2 sibling drains, connections drained on token rotation are transitioned to _in_flight (++_in_flight;).
   • Pruned connections are disconnected outside _mutex via drainDisconnectList() before candidate validation or slot reservation, decrementing _in_flight and _current_size under _mutex only after each physical disconnect completes.
   • An RAII DisconnectGuard guarantees that any early exit or exception in acquire() drains and decrements all queued connections.

2. Serialized Replacement Pool Creation:

   • In ConnectionPoolManager::acquireConnection(), when replacing an evictable pool, old_pool_to_close->close() is now executed outside _manager_mutex before creating and publishing the replacement pool in _pools[key]. All physical handles of the old pool are completely closed before any thread can acquire from the replacement pool.

3. GIL-Safe Callback Wrapper:

   • Implemented GilSafeCallback wrapping py::object with py::gil_scoped_acquire on copy, move, destruction, and operator() execution, preventing any Python refcount decrements without the GIL.

4. Testing:

   • Added regression test test_pool_prune_stale_disconnect_keeps_in_flight_until_disconnected.
   • All 11 regression tests pass cleanly (100% pass rate in 1.68s).

[Copilot]

🟡 Changes recommended

Two unresolved critical concurrency issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[RKS (rksharma-owg)]

Iteration 10 Update: Synchronized Same-Key Pool Replacement, Decoupled GIL-Safe Hooks, and GIL-Released CV Waits

Pushed commit 9eac00b5 addressing review 5243442210:

1. Synchronized Same-Key Replacement via _closing_keys and _manager_cv:

   • Added std::unordered_set<std::u16string> _closing_keys; and std::condition_variable _manager_cv; to ConnectionPoolManager.
   • When an evictable pool is evicted for replacement, key is inserted into _closing_keys under _manager_mutex. An RAII ClosingGuard guarantees keys are erased and _manager_cv.notify_all() is fired even on exception or early return.
   • Any concurrent acquireConnection(key) for the same key waits on _manager_cv until _closing_keys no longer contains key (or pooling is disabled). Other keys proceed without waiting.
   • Once old pool handles finish closing outside _manager_mutex, _manager_mutex is reacquired, the replacement pool is published in _pools[key], key is removed from _closing_keys, and _manager_cv.notify_all() wakes waiting threads. Concurrent callers for the same key reuse the newly published replacement pool, strictly enforcing max_size.
   • closePools() waits on _manager_cv until _closing_keys.empty() before sweeping pools.
   • setAccepting(false) notifies _manager_cv.notify_all() so waiting threads abort immediately.

2. Decoupled GIL-Safe Disconnect Hooks & PyObjectHolder:

   • Removed struct GilSafeCallback.
   • In ConnectionPool, _on_disconnect_hook is stored as std::shared_ptr<std::function<void()>>.
   • Copying hook = _on_disconnect_hook; under ConnectionPool::_mutex is a pure C++ atomic pointer increment with ZERO GIL involvement, completely eliminating lock inversion between _mutex and Python GIL.
   • Disconnect hook execution is centralized in ConnectionPool::invokeDisconnectHook(): copies the shared_ptr under _mutex, releases _mutex, calls (*hook)() outside the lock, and resets hook outside _mutex.
   • In ddbc_bindings.cpp, wrapped the captured callable in PyObjectHolder, which holds raw PyObject* and executes Py_XDECREF exclusively inside its destructor under py::gil_scoped_acquire. Destruction is guaranteed to happen under the GIL outside _mutex.

3. GIL-Released Condition Variable Waiting:

   • Released Python GIL via py::gil_scoped_release release_gil; during ConnectionPoolManager::acquireConnection() coordination and closePools() wait, allowing Python threads and callbacks to execute unblocked while condition variable waits are in flight.

4. Testing:

   • Added regression test test_pool_manager_serializes_same_key_replacement_while_old_pool_closing.
   • All 12 unit and regression tests pass cleanly in subprocesses with 100% pass rate in 3.08s.

[Gaurav Sharma (bewithgaurav)]

RKS (@rksharma-owg) thanks for raising the PR!
no need to make further changes on copilot suggestions, we'll take a look and review this

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🟡 Changes recommended

Critical concurrency and interpreter-shutdown issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

mssql_python/pybind/connection/connection_pool.cpp:809

• When pooling is disabled, a pre-disable acquireConnection() can still hold a shared pointer while closePools() runs. Because use_count() > 1, this loop retains the pool even when close() found no live work; that acquire can then open on the retained pool and returnConnection() will put the handle back into _pool. Disabled acquisitions never revisit the map, so the physical connection remains idle until process exit. Coordinate preselected acquires with disable, or make releases discard connections and remove the now-empty pool when _accepting is false.

        for (auto it = _pools.begin(); it != _pools.end();) {
            if (!it->second || (it->second.use_count() == 1 && it->second->canEvict())) {
                it = _pools.erase(it);
            } else {

mssql_python/pybind/ddbc_bindings.cpp:6158

• The move-assignment cleanup also unconditionally acquires the GIL. If this holder is destroyed or moved during native pool teardown after Python finalization, this path has the same shutdown crash/hang as the destructor; apply the finalization guard before py::gil_scoped_acquire here as well.

        PyObjectHolder& operator=(PyObjectHolder&& other) noexcept {
            if (this != &other) {
                if (ptr) {
                    py::gil_scoped_acquire gil;
                    Py_XDECREF(ptr);
                }

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

[github-actions]

Code Coverage Report

Diff coverage	Overall coverage	Lines covered
89%	83%	8846 of 10534

Files needing attention

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: 77.9%
mssql_python.pybind.connection.connection.h: 78.5%
mssql_python.pybind.connection.connection.cpp: 85.5%
mssql_python.logging.py: 86.2%
mssql_python.pybind.connection.connection_pool.cpp: 87.4%

View Azure DevOps build