Skip to content

Commit e5cadf2

Browse files
Fix stale rows and cursor leak in async adapter
Clear _rows deque after non-query execute() and executemany() to prevent stale data from a previous SELECT leaking through fetchone(). Wrap cursor operations in try/finally to ensure the underlying cursor is always closed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 76fb63f commit e5cadf2

2 files changed

Lines changed: 128 additions & 20 deletions

File tree

src/sqlalchemydqlite/aio.py

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,29 +39,34 @@ def close(self) -> None:
3939

4040
def execute(self, operation: str, parameters: Any = None) -> Any:
4141
cursor = self._connection.cursor()
42-
if parameters is not None:
43-
await_only(cursor.execute(operation, parameters))
44-
else:
45-
await_only(cursor.execute(operation))
46-
47-
if cursor.description:
48-
self.description = cursor.description
49-
self.lastrowid = self.rowcount = -1
50-
self._rows = deque(await_only(cursor.fetchall()))
51-
else:
52-
self.description = None
53-
self.lastrowid = cursor.lastrowid
54-
self.rowcount = cursor.rowcount
55-
56-
await_only(cursor.close())
42+
try:
43+
if parameters is not None:
44+
await_only(cursor.execute(operation, parameters))
45+
else:
46+
await_only(cursor.execute(operation))
47+
48+
if cursor.description:
49+
self.description = cursor.description
50+
self.lastrowid = self.rowcount = -1
51+
self._rows = deque(await_only(cursor.fetchall()))
52+
else:
53+
self.description = None
54+
self.lastrowid = cursor.lastrowid
55+
self.rowcount = cursor.rowcount
56+
self._rows.clear()
57+
finally:
58+
await_only(cursor.close())
5759

5860
def executemany(self, operation: str, seq_of_parameters: Any) -> Any:
5961
cursor = self._connection.cursor()
60-
await_only(cursor.executemany(operation, seq_of_parameters))
61-
self.description = None
62-
self.lastrowid = cursor.lastrowid
63-
self.rowcount = cursor.rowcount
64-
await_only(cursor.close())
62+
try:
63+
await_only(cursor.executemany(operation, seq_of_parameters))
64+
self.description = None
65+
self.lastrowid = cursor.lastrowid
66+
self.rowcount = cursor.rowcount
67+
self._rows.clear()
68+
finally:
69+
await_only(cursor.close())
6570

6671
def fetchone(self) -> Any:
6772
if self._rows:

tests/test_async_adapter.py

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
"""Tests for async adapter cursor behavior."""
2+
3+
import ast
4+
import inspect
5+
import textwrap
6+
from collections import deque
7+
from unittest.mock import MagicMock, patch
8+
9+
from sqlalchemydqlite.aio import AsyncAdaptedConnection, AsyncAdaptedCursor
10+
11+
12+
def _make_cursor() -> AsyncAdaptedCursor:
13+
"""Create an AsyncAdaptedCursor with a mocked connection."""
14+
mock_conn = MagicMock()
15+
adapted_conn = AsyncAdaptedConnection.__new__(AsyncAdaptedConnection)
16+
adapted_conn._connection = mock_conn
17+
cursor = AsyncAdaptedCursor(adapted_conn)
18+
return cursor
19+
20+
21+
def _run_sync(coro_or_value: object) -> object:
22+
"""Replacement for await_only that resolves coroutines synchronously."""
23+
if hasattr(coro_or_value, "__await__") or hasattr(coro_or_value, "cr_await"):
24+
# It's a coroutine -- we can't actually await it in sync context,
25+
# but our mocks return plain values, so send(None) is enough.
26+
try:
27+
coro_or_value.send(None) # type: ignore[union-attr]
28+
except StopIteration as e:
29+
return e.value
30+
return coro_or_value
31+
32+
33+
class TestAsyncAdaptedCursorRowsCleared:
34+
def test_rows_cleared_after_non_query_execute(self) -> None:
35+
"""After a SELECT then an INSERT, fetchone() must return None."""
36+
cursor = _make_cursor()
37+
38+
# Simulate that a previous SELECT populated _rows
39+
cursor._rows = deque([(1, "alice")])
40+
41+
# Set up a mock inner cursor that returns no description (DML)
42+
mock_inner = MagicMock()
43+
mock_inner.description = None
44+
mock_inner.lastrowid = 1
45+
mock_inner.rowcount = 1
46+
mock_inner.execute.return_value = None
47+
mock_inner.close.return_value = None
48+
cursor._connection.cursor.return_value = mock_inner
49+
50+
with patch("sqlalchemydqlite.aio.await_only", side_effect=_run_sync):
51+
cursor.execute("INSERT INTO t VALUES (1)")
52+
53+
result = cursor.fetchone()
54+
assert result is None, f"Expected None after non-query execute, got {result}"
55+
56+
def test_rows_cleared_after_executemany(self) -> None:
57+
"""After executemany(), fetchone() must return None."""
58+
cursor = _make_cursor()
59+
60+
# Simulate that a previous SELECT populated _rows
61+
cursor._rows = deque([(1, "alice"), (2, "bob")])
62+
63+
mock_inner = MagicMock()
64+
mock_inner.lastrowid = 3
65+
mock_inner.rowcount = 2
66+
mock_inner.executemany.return_value = None
67+
mock_inner.close.return_value = None
68+
cursor._connection.cursor.return_value = mock_inner
69+
70+
with patch("sqlalchemydqlite.aio.await_only", side_effect=_run_sync):
71+
cursor.executemany("INSERT INTO t VALUES (?)", [(1,), (2,)])
72+
73+
result = cursor.fetchone()
74+
assert result is None, f"Expected None after executemany, got {result}"
75+
76+
77+
def _has_finally_with_close(func: object) -> bool:
78+
"""Check if a function has cursor.close() inside a finally block."""
79+
source = textwrap.dedent(inspect.getsource(func))
80+
tree = ast.parse(source)
81+
82+
for node in ast.walk(tree):
83+
if isinstance(node, ast.Try) and node.finalbody:
84+
for stmt in ast.walk(node):
85+
if isinstance(stmt, ast.Call):
86+
func_node = stmt.func
87+
if isinstance(func_node, ast.Attribute) and func_node.attr == "close":
88+
return True
89+
return False
90+
91+
92+
class TestAsyncAdaptedCursorCleanup:
93+
def test_cursor_closed_on_execute_error(self) -> None:
94+
"""Underlying cursor must be closed even if execute() raises."""
95+
assert _has_finally_with_close(AsyncAdaptedCursor.execute), (
96+
"cursor.close() should be in a finally block to prevent leaks on error"
97+
)
98+
99+
def test_executemany_cursor_closed_on_error(self) -> None:
100+
"""Underlying cursor must be closed even if executemany() raises."""
101+
assert _has_finally_with_close(AsyncAdaptedCursor.executemany), (
102+
"cursor.close() should be in a finally block to prevent leaks on error"
103+
)

0 commit comments

Comments
 (0)