Skip to content

Commit 52d00ab

Browse files
Narrow AsyncAdaptedConnection.close rollback suppression
The close path wrapped the pre-close rollback in contextlib.suppress(Exception), which swallowed every non-BaseException error — including AttributeError, TypeError, and bare RuntimeError that signal programming bugs. A bad refactor to the underlying async connection would silently disappear at close time. Narrow the suppression to the categories a rollback can legitimately raise (OperationalError, InterfaceError, DqliteConnectionError, OSError, TimeoutError, ConnectionError) so programming bugs still propagate, mirroring the client dialect's own is_disconnect classification. Move the `import contextlib` to the top of the module as part of the same cleanup and tighten the existing regression test so it exercises a genuine connection-level failure; add a companion test that programmer errors propagate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent acf7f6a commit 52d00ab

2 files changed

Lines changed: 45 additions & 11 deletions

File tree

src/sqlalchemydqlite/aio.py

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""Async dqlite dialect for SQLAlchemy."""
22

3+
import contextlib
34
from collections import deque
45
from collections.abc import Sequence
56
from typing import Any
@@ -9,6 +10,8 @@
910
from sqlalchemy.pool import AsyncAdaptedQueuePool
1011
from sqlalchemy.util import await_only
1112

13+
from dqliteclient.exceptions import DqliteConnectionError
14+
from dqlitedbapi.exceptions import InterfaceError, OperationalError
1215
from sqlalchemydqlite.base import DqliteDialect
1316

1417
__all__ = ["DqliteDialect_aio"]
@@ -136,14 +139,23 @@ def rollback(self) -> None:
136139
def close(self) -> None:
137140
# Attempt rollback before close so a caller that exits without
138141
# committing does not leave a dangling server-side transaction.
139-
# The underlying async connection's rollback is a
140-
# silent no-op when no transaction is active and when the
141-
# connection has never been used, so the double-call is safe.
142-
# Suppress rollback failures — the close is more important and
143-
# happens unconditionally below.
144-
import contextlib
145-
146-
with contextlib.suppress(Exception):
142+
# The underlying async connection's rollback is a silent no-op when
143+
# no transaction is active and when the connection has never been
144+
# used, so the double-call is safe.
145+
#
146+
# Narrow the suppression to the categories a best-effort rollback
147+
# can legitimately raise — connection-level / transport errors —
148+
# so programming bugs (AttributeError, TypeError, bare RuntimeError,
149+
# etc.) still propagate. The tuple mirrors the client layer's own
150+
# is_disconnect classification in base.py.
151+
with contextlib.suppress(
152+
OperationalError,
153+
InterfaceError,
154+
DqliteConnectionError,
155+
OSError,
156+
TimeoutError,
157+
ConnectionError,
158+
):
147159
await_only(self._connection.rollback())
148160
await_only(self._connection.close())
149161

tests/test_async_adapter.py

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,15 +126,18 @@ def test_close_attempts_rollback_first(self) -> None:
126126

127127
assert calls == ["rollback", "close"], f"Expected rollback then close, got {calls}"
128128

129-
def test_close_proceeds_when_rollback_raises(self) -> None:
130-
"""A failing rollback (e.g. connection already in a bad state)
129+
def test_close_proceeds_when_rollback_raises_connection_error(self) -> None:
130+
"""A failing rollback caused by a connection-level error (e.g.
131+
broken transport, OS-level disconnect, server already closed)
131132
must not block close — resource cleanup is more important."""
133+
from dqlitedbapi.exceptions import OperationalError
134+
132135
mock_conn = MagicMock()
133136
calls: list[str] = []
134137

135138
def failing_rollback() -> None:
136139
calls.append("rollback-attempt")
137-
raise RuntimeError("simulated rollback failure")
140+
raise OperationalError("simulated rollback failure")
138141

139142
mock_conn.rollback.side_effect = failing_rollback
140143
mock_conn.close.side_effect = lambda: calls.append("close") or object()
@@ -146,3 +149,22 @@ def failing_rollback() -> None:
146149
adapted.close() # must not raise
147150

148151
assert "close" in calls, "close() must run even if rollback raised"
152+
153+
def test_close_propagates_programming_error_from_rollback(self) -> None:
154+
"""A failing rollback that looks like a programming bug
155+
(AttributeError / TypeError / bare RuntimeError) must propagate
156+
so refactor regressions don't get swallowed silently."""
157+
import pytest
158+
159+
mock_conn = MagicMock()
160+
mock_conn.rollback.side_effect = AttributeError("refactor bug")
161+
mock_conn.close.return_value = object()
162+
163+
adapted = AsyncAdaptedConnection.__new__(AsyncAdaptedConnection)
164+
adapted._connection = mock_conn
165+
166+
with (
167+
patch("sqlalchemydqlite.aio.await_only", side_effect=_run_sync),
168+
pytest.raises(AttributeError),
169+
):
170+
adapted.close()

0 commit comments

Comments
 (0)