Skip to content

Commit 8f70c48

Browse files
Run underlying close() even if rollback raises outside narrow tuple
AsyncAdaptedConnection.close() did rollback then close as sequential awaits without a try/finally. If rollback raised CancelledError (a BaseException, common during async pool dispose) or a programming bug (AttributeError, RuntimeError), control skipped the underlying close() and the AsyncConnection leaked. SQLAlchemy's pool does not re-call close() after a raise, so the leak was permanent. Wrap the rollback + narrow-catch block in try/finally so close() runs on any rollback outcome. The narrow-catch path from ISSUE-135 is preserved for the in-scope transport errors; the finally ensures close runs when the catch does not match. Mirror of ISSUE-224 which addressed the inverse leak in connect().
1 parent a05854a commit 8f70c48

2 files changed

Lines changed: 120 additions & 24 deletions

File tree

src/sqlalchemydqlite/aio.py

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -231,31 +231,40 @@ def close(self) -> None:
231231
# so programming bugs (AttributeError, TypeError, bare RuntimeError,
232232
# etc.) still propagate. The tuple mirrors the client layer's own
233233
# is_disconnect classification in base.py.
234+
#
235+
# Wrap in ``try/finally`` so close() runs regardless of how
236+
# rollback() exits — narrow-caught, programming bug, or
237+
# ``BaseException`` like ``CancelledError`` during pool dispose.
238+
# SA's pool does not re-call close() on failure, so skipping
239+
# close would leak the underlying AsyncConnection. Mirror of the
240+
# inverse leak fixed in DqliteDialect_aio.connect().
234241
try:
235-
await_only(self._connection.rollback())
236-
except (
237-
OperationalError,
238-
InterfaceError,
239-
DqliteConnectionError,
240-
OSError,
241-
TimeoutError,
242-
ConnectionError,
243-
) as exc:
244-
# Silent suppression used to hide e.g. "leader flip
245-
# mid-rollback" from operators — a DEBUG line preserves the
246-
# diagnostic without masking or propagating. Include both
247-
# id(self) and the peer address so a noisy pool can be
248-
# correlated to specific adapter instances and nodes.
249-
peer = getattr(self._connection, "address", None)
250-
logger.debug(
251-
"AsyncAdaptedConnection.close (id=%s, peer=%s): "
252-
"rollback failed (%s); proceeding to close",
253-
id(self),
254-
peer,
255-
type(exc).__name__,
256-
exc_info=True,
257-
)
258-
await_only(self._connection.close())
242+
try:
243+
await_only(self._connection.rollback())
244+
except (
245+
OperationalError,
246+
InterfaceError,
247+
DqliteConnectionError,
248+
OSError,
249+
TimeoutError,
250+
ConnectionError,
251+
) as exc:
252+
# Silent suppression used to hide e.g. "leader flip
253+
# mid-rollback" from operators — a DEBUG line preserves
254+
# the diagnostic without masking or propagating. Include
255+
# both id(self) and the peer address so a noisy pool can
256+
# be correlated to specific adapter instances and nodes.
257+
peer = getattr(self._connection, "address", None)
258+
logger.debug(
259+
"AsyncAdaptedConnection.close (id=%s, peer=%s): "
260+
"rollback failed (%s); proceeding to close",
261+
id(self),
262+
peer,
263+
type(exc).__name__,
264+
exc_info=True,
265+
)
266+
finally:
267+
await_only(self._connection.close())
259268

260269

261270
class DqliteDialect_aio(DqliteDialect): # noqa: N801
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
"""AsyncAdaptedConnection.close() must run the underlying close() even if
2+
rollback() raises outside the narrow suppression tuple.
3+
4+
SQLAlchemy's async pool calls close() during dispose / teardown. If
5+
rollback() raises an exception not in the narrow catch tuple
6+
(``BaseException`` like ``CancelledError``, programming bugs, etc.),
7+
control previously skipped the final ``await_only(self._connection.close())``
8+
line and the underlying AsyncConnection leaked. SA's pool does NOT
9+
re-call close() on failure, so the leak is permanent.
10+
11+
Wrap rollback in ``try/finally`` so close() runs on any rollback
12+
outcome. Mirror of ISSUE-224 which fixed the inverse leak in connect().
13+
"""
14+
15+
from __future__ import annotations
16+
17+
import asyncio
18+
19+
import pytest
20+
21+
from sqlalchemydqlite.aio import AsyncAdaptedConnection
22+
23+
24+
class _FakeAsyncConn:
25+
def __init__(self, rollback_exc: BaseException | None) -> None:
26+
self._rollback_exc = rollback_exc
27+
self.close_calls = 0
28+
29+
async def rollback(self) -> None:
30+
if self._rollback_exc is not None:
31+
raise self._rollback_exc
32+
33+
async def close(self) -> None:
34+
self.close_calls += 1
35+
36+
37+
def _install_fake_await_only() -> tuple[object, object]:
38+
"""Replace await_only in the aio module with a sync driver so tests
39+
don't need a live SA engine / greenlet context.
40+
"""
41+
from sqlalchemydqlite import aio as aio_module
42+
43+
def _fake_await_only(coro: object) -> object:
44+
return asyncio.new_event_loop().run_until_complete(coro) # type: ignore[arg-type]
45+
46+
orig = aio_module.await_only
47+
aio_module.await_only = _fake_await_only # type: ignore[assignment]
48+
return aio_module, orig
49+
50+
51+
def _restore_await_only(aio_module: object, orig: object) -> None:
52+
aio_module.await_only = orig # type: ignore[attr-defined]
53+
54+
55+
def test_close_runs_after_rollback_raises_cancelled_error() -> None:
56+
"""CancelledError is a BaseException and not in the narrow catch
57+
tuple. The underlying close() must still run — and then the
58+
CancelledError propagates as the active exception.
59+
"""
60+
fake = _FakeAsyncConn(asyncio.CancelledError())
61+
adapter = AsyncAdaptedConnection(fake) # type: ignore[arg-type]
62+
63+
aio_module, orig = _install_fake_await_only()
64+
try:
65+
with pytest.raises(asyncio.CancelledError):
66+
adapter.close()
67+
finally:
68+
_restore_await_only(aio_module, orig)
69+
70+
assert fake.close_calls == 1
71+
72+
73+
def test_close_runs_after_rollback_raises_attribute_error() -> None:
74+
"""A programming bug in rollback (AttributeError) still must not
75+
prevent close() from running, even though the bug propagates.
76+
"""
77+
fake = _FakeAsyncConn(AttributeError("bug"))
78+
adapter = AsyncAdaptedConnection(fake) # type: ignore[arg-type]
79+
80+
aio_module, orig = _install_fake_await_only()
81+
try:
82+
with pytest.raises(AttributeError, match="bug"):
83+
adapter.close()
84+
finally:
85+
_restore_await_only(aio_module, orig)
86+
87+
assert fake.close_calls == 1

0 commit comments

Comments
 (0)