Skip to content

Commit b0acf15

Browse files
Rollback before close and clean up DateTime passthrough
- AsyncAdaptedConnection.close() now attempts a rollback before closing so a NullPool / unpooled async connection that exits without an explicit commit does not leave a dangling server-side transaction. The underlying rollback is a silent no-op for never-used and already-idle connections, so the extra call is safe. Rollback failures are suppressed — close is more important and proceeds unconditionally. - Remove the no-op bind_processor/result_processor overrides from _DqliteDateTime. They existed only to ensure the generic DateTime's literal_processor (which calls ``isoformat()`` directly) is selected instead of pysqlite's iso-string-based converter. The parent's defaults already return None, so explicit overrides were redundant and made _DqliteDate's non-trivial processor look stylistically inconsistent for no reason. Replace the overrides with a docstring explaining why the class still exists. Tests: - tests/test_async_adapter.py adds TestAsyncAdaptedConnectionClose: verifies rollback-then-close ordering and verifies close() still runs when rollback raises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent de14008 commit b0acf15

3 files changed

Lines changed: 64 additions & 6 deletions

File tree

src/sqlalchemydqlite/aio.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,17 @@ def rollback(self) -> None:
134134
await_only(self._connection.rollback())
135135

136136
def close(self) -> None:
137+
# Attempt rollback before close so a caller that exits without
138+
# committing does not leave a dangling server-side transaction
139+
# (ISSUE-91). 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):
147+
await_only(self._connection.rollback())
137148
await_only(self._connection.close())
138149

139150

src/sqlalchemydqlite/base.py

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,15 @@ class _DqliteDateTime(sqltypes.DateTime):
1919
"""Passthrough DateTime — ``dqlitedbapi`` already returns ``datetime.datetime``
2020
for columns declared as DATETIME/TIMESTAMP (matching PEP 249 and the
2121
psycopg/mysqlclient convention), so no string parsing is needed.
22-
"""
23-
24-
def bind_processor(self, dialect: Any) -> None:
25-
return None
2622
27-
def result_processor(self, dialect: Any, coltype: Any) -> None:
28-
return None
23+
Inheriting from ``sqltypes.DateTime`` (not ``sqlite.DATETIME``) is
24+
deliberate: the generic parent's ``literal_processor`` calls
25+
``value.isoformat()`` directly — bypassing pysqlite's
26+
iso-string-based bind processor that would double-convert our
27+
already-datetime values (ISSUE-114). The parent's default
28+
``bind_processor`` / ``result_processor`` return ``None`` already,
29+
so no explicit overrides are needed here.
30+
"""
2931

3032

3133
class _DqliteDate(sqltypes.Date):

tests/test_async_adapter.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,48 @@ def test_executemany_cursor_closed_on_error(self) -> None:
101101
assert _has_finally_with_close(AsyncAdaptedCursor.executemany), (
102102
"cursor.close() should be in a finally block to prevent leaks on error"
103103
)
104+
105+
106+
class TestAsyncAdaptedConnectionClose:
107+
"""ISSUE-91: close() attempts rollback before closing.
108+
109+
SQLAlchemy's async adapter previously closed the connection without
110+
a rollback, leaving any open server-side transaction dangling in
111+
unpooled / NullPool usage. Verify rollback is called first, and
112+
verify rollback failure does not prevent close.
113+
"""
114+
115+
def test_close_attempts_rollback_first(self) -> None:
116+
mock_conn = MagicMock()
117+
calls: list[str] = []
118+
mock_conn.rollback.side_effect = lambda: calls.append("rollback") or object()
119+
mock_conn.close.side_effect = lambda: calls.append("close") or object()
120+
121+
adapted = AsyncAdaptedConnection.__new__(AsyncAdaptedConnection)
122+
adapted._connection = mock_conn
123+
124+
with patch("sqlalchemydqlite.aio.await_only", side_effect=_run_sync):
125+
adapted.close()
126+
127+
assert calls == ["rollback", "close"], f"Expected rollback then close, got {calls}"
128+
129+
def test_close_proceeds_when_rollback_raises(self) -> None:
130+
"""A failing rollback (e.g. connection already in a bad state)
131+
must not block close — resource cleanup is more important."""
132+
mock_conn = MagicMock()
133+
calls: list[str] = []
134+
135+
def failing_rollback() -> None:
136+
calls.append("rollback-attempt")
137+
raise RuntimeError("simulated rollback failure")
138+
139+
mock_conn.rollback.side_effect = failing_rollback
140+
mock_conn.close.side_effect = lambda: calls.append("close") or object()
141+
142+
adapted = AsyncAdaptedConnection.__new__(AsyncAdaptedConnection)
143+
adapted._connection = mock_conn
144+
145+
with patch("sqlalchemydqlite.aio.await_only", side_effect=_run_sync):
146+
adapted.close() # must not raise
147+
148+
assert "close" in calls, "close() must run even if rollback raised"

0 commit comments

Comments
 (0)