Skip to content

Commit bb87ee2

Browse files
Clear _bound_loop on _invalidate; clear _pool_released before pool-side close; narrow close_impl pending-drain suppress
Three client-side correctness fixes: - ``_invalidate`` cleared every transaction-state field but did not clear ``self._bound_loop``. After invalidation on loop A, a reconnect on loop B was rejected by ``_check_in_use``'s cross-loop guard even though every other state field was scrubbed and the connection was otherwise ready for fresh use. Three of the four lifecycle scrub sites already cleared ``_bound_loop``; this completes the parity. - ``ConnectionPool.acquire``'s liveness short-circuit and ``_drain_idle`` both called ``conn.close()`` on connections carrying ``_pool_released=True`` (set by the prior ``_release``). ``DqliteConnection.close()`` early-returns when ``_pool_released`` is True so user-side close on a checked-in conn is a no-op — but the same guard silently absorbed the pool-side cleanup close, leaking writer / reader pairs on FIN-seen idle conns and on ``pool.close()`` cascades. Clear the flag immediately before the close call at both sites. - ``DqliteConnection._close_impl``'s pending-drain await wrapped the await in ``contextlib.suppress(BaseException)``. The in-source comment claimed parity with ``connect()``'s pending-retire path, but connect actually uses NARROW catches (CancelledError + Exception). asyncio cancellation gets re-delivered at the next await boundary; signal-driven KeyboardInterrupt / SystemExit are one-shot and were lost forever if absorbed here. Narrow the suppress to ``(Exception, asyncio.CancelledError)``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a251a9a commit bb87ee2

3 files changed

Lines changed: 39 additions & 11 deletions

File tree

src/dqliteclient/connection.py

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1217,15 +1217,17 @@ async def _close_impl(self) -> None:
12171217
# the second caller's resumption — verified by code
12181218
# review, not coverage.
12191219
#
1220-
# Suppress ``BaseException`` (not just ``Exception``) so a
1221-
# ``CancelledError`` delivered during ``await pending``
1222-
# does not propagate out of close() before the protocol
1223-
# tear-down at lines below runs — leaking ``_protocol``.
1224-
# Symmetric with ``connect()``'s pending-retire path which
1225-
# already uses ``BaseException``. The user's outer cancel
1226-
# gets re-delivered at the next await boundary inside
1227-
# close (e.g., ``writer.wait_closed`` below).
1228-
with contextlib.suppress(BaseException):
1220+
# Narrow suppress to ``(Exception, asyncio.CancelledError)``
1221+
# so KeyboardInterrupt and SystemExit propagate. Cycle 23
1222+
# noted that the previous ``BaseException`` suppress was
1223+
# mis-justified as "symmetric with connect()" — connect's
1224+
# pending-retire path actually uses NARROW catches
1225+
# (``except asyncio.CancelledError`` + ``except Exception``).
1226+
# asyncio cancellation gets re-delivered at the next await
1227+
# boundary (Python 3.11+ ``Task.cancelling()`` re-arm
1228+
# contract), but signal-driven KI/SE are one-shot and
1229+
# would be lost forever if absorbed here.
1230+
with contextlib.suppress(Exception, asyncio.CancelledError):
12291231
await pending
12301232
# Mirror ``_invalidate``'s atomic clear of the transaction
12311233
# bookkeeping. Without this, a raw ``BEGIN`` followed by an
@@ -1489,6 +1491,14 @@ async def _bounded_drain() -> None:
14891491
self._savepoint_stack.clear()
14901492
self._savepoint_implicit_begin = False
14911493
self._has_untracked_savepoint = False
1494+
# Clear the loop binding so a subsequent ``connect()`` after
1495+
# invalidation can bind to a different loop without tripping
1496+
# the ``_check_in_use`` cross-loop guard. ``_close_impl``
1497+
# already clears this; without it here, an invalidated
1498+
# connection (where every other state field has been
1499+
# scrubbed) is still rejected as "bound to a different
1500+
# event loop" on a fresh-loop reconnect.
1501+
self._bound_loop = None
14921502
# Preserve the FIRST cause: ``_ensure_connected`` raises a
14931503
# synthetic ``DqliteConnectionError("Not connected")`` chained
14941504
# from ``self._invalidation_cause`` whenever an

src/dqliteclient/pool.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,15 @@ async def _drain_idle(self) -> None:
601601
# Verified by code review, not coverage.
602602
break
603603
try:
604+
# Clear ``_pool_released`` BEFORE close so the
605+
# close path actually runs. The release flag is
606+
# set when the conn was put back into the queue
607+
# in ``_release``; ``DqliteConnection.close()``
608+
# early-returns on True (user-side close on a
609+
# checked-in conn must be a no-op). Without this
610+
# clear, the pool's drain-side close would be
611+
# silently absorbed and the writer would leak.
612+
conn._pool_released = False
604613
# Shield each per-connection close against an outer
605614
# ``asyncio.timeout(pool.close())``. Without the shield,
606615
# a cancel that lands mid-``wait_closed`` propagates out
@@ -867,6 +876,14 @@ async def acquire(self) -> AsyncIterator[DqliteConnection]:
867876
# never-connected conn (``_protocol is None``) — it short-
868877
# circuits to a noop. Shielded so an outer cancel does not
869878
# leave the writer dangling.
879+
#
880+
# Clear ``_pool_released`` BEFORE close so the close
881+
# actually runs. ``DqliteConnection.close()`` early-returns
882+
# when ``_pool_released`` is True (so user-side close on a
883+
# checked-in conn is a no-op); without this clear, the
884+
# pool-side close on a dead conn we just dequeued would
885+
# be silently absorbed and the writer would leak.
886+
conn._pool_released = False
870887
with contextlib.suppress(*_POOL_CLEANUP_EXCEPTIONS):
871888
await asyncio.shield(conn.close())
872889
# Wrap _drain_idle so a cancel mid-drain releases the dead

tests/test_refresh_pid_cache_contract.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ def test_refresh_pid_cache_is_callable_zero_arg() -> None:
6060
registration's call shape) is caught."""
6161
saved = conn_mod._current_pid
6262
try:
63-
result = conn_mod._refresh_pid_cache()
63+
# ``register_at_fork`` calls the function with no arguments;
64+
# this raises TypeError if a refactor adds a parameter.
65+
conn_mod._refresh_pid_cache()
6466
finally:
6567
conn_mod._current_pid = saved
66-
assert result is None # PEP 257-style: side-effect function returns None
6768

6869

6970
def test_after_in_child_fork_actually_refreshes_cache() -> None:

0 commit comments

Comments
 (0)