Skip to content

Commit 5b5f922

Browse files
Move _run_protocol _in_use=True assignment inside the try block
The assignment sat BEFORE the ``try:`` whose ``finally`` clears the flag. A KeyboardInterrupt / SystemExit delivered at the bytecode boundary between the STORE_ATTR for ``self._in_use = True`` and the SETUP_FINALLY for the try-frame would escape WITHOUT entering the try, so the ``finally`` would not run and ``_in_use`` would stay ``True`` forever — every subsequent operation on the connection would raise ``InterfaceError("another operation is in progress")`` permanently. The dbapi sync facade has the symmetric discipline (KI cleanup in ``_run_sync``); the client layer needed to match. The fix moves ``self._in_use = True`` to be the first statement INSIDE the try block, so any signal delivery on or after the assignment line lands inside the try-frame and the finally runs. Three pins fence the regression: - AST source-shape pin (regressions that move the assignment back outside the try block fail at parse time). - Behavioural pin (KI / SystemExit raised inside ``fn`` clears the flag). - ``sys.settrace`` pin that injects KI on the assignment line specifically — the closest in-process repro of "signal between two bytecodes" — verifies post-call ``_in_use is False``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eaa3374 commit 5b5f922

2 files changed

Lines changed: 215 additions & 1 deletion

File tree

src/dqliteclient/connection.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2006,8 +2006,20 @@ async def _run_protocol[T](self, fn: Callable[[DqliteProtocol, int], Awaitable[T
20062006
"""
20072007
self._check_in_use()
20082008
protocol, db_id = self._ensure_connected()
2009-
self._in_use = True
20102009
try:
2010+
# Set ``_in_use = True`` INSIDE the try block (not before)
2011+
# so a ``KeyboardInterrupt`` / ``SystemExit`` delivered at
2012+
# the bytecode boundary BETWEEN the assignment and the
2013+
# ``try:`` (signals can fire between any two bytecodes per
2014+
# the Python interpreter's signal-eval-frequency machinery)
2015+
# cannot escape WITHOUT entering the try-frame and thus
2016+
# without running the ``finally`` that clears the flag.
2017+
# If the flag stuck at ``True``, every subsequent
2018+
# operation on this connection would raise
2019+
# ``InterfaceError("another operation is in progress")``
2020+
# permanently. The dbapi sync facade has the symmetric
2021+
# discipline; this mirrors it.
2022+
self._in_use = True
20112023
return await fn(protocol, db_id)
20122024
except _WireEncodeError as e:
20132025
# Client-side parameter-encoding error. The wire bytes were
Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
"""Pin: ``_run_protocol`` sets ``self._in_use = True`` INSIDE the
2+
``try:`` block whose ``finally`` clears it.
3+
4+
A ``KeyboardInterrupt`` / ``SystemExit`` delivered at the bytecode
5+
boundary BETWEEN the assignment and the ``try:`` (signals can be
6+
delivered between any two bytecodes by the Python interpreter's
7+
signal-eval-frequency machinery) escapes WITHOUT entering the try, so
8+
the ``finally`` does not run and ``_in_use`` stays ``True`` forever.
9+
Subsequent calls on the same ``DqliteConnection`` would raise
10+
``InterfaceError("another operation is in progress")`` permanently —
11+
the connection is unrecoverable until garbage-collected.
12+
13+
The dbapi sync facade has the symmetric pattern fixed (KI cleanup in
14+
``_run_sync``); the client layer must match. The fix is one line —
15+
move the ``self._in_use = True`` assignment to be the first statement
16+
inside ``try:``.
17+
18+
Both pins (structural source-shape AND functional behaviour-via-
19+
sys.settrace KI injection) defend the contract.
20+
"""
21+
22+
from __future__ import annotations
23+
24+
import ast
25+
import inspect
26+
import sys
27+
import textwrap
28+
from typing import Any
29+
from unittest.mock import MagicMock
30+
31+
import pytest
32+
33+
from dqliteclient import connection as _conn_mod
34+
from dqliteclient.connection import DqliteConnection
35+
36+
37+
def _make_connection() -> DqliteConnection:
38+
"""Construct a minimal ``DqliteConnection`` whose state lets
39+
``_run_protocol``'s preflight checks pass through to ``fn``.
40+
"""
41+
conn = DqliteConnection.__new__(DqliteConnection)
42+
conn._address = "host:9001"
43+
conn._in_use = False
44+
conn._in_transaction = False
45+
conn._tx_owner = None
46+
conn._savepoint_stack = []
47+
conn._savepoint_implicit_begin = False
48+
conn._has_untracked_savepoint = False
49+
conn._invalidation_cause = None
50+
conn._bound_loop_ref = None
51+
conn._pending_drain = None
52+
conn._creator_pid = _conn_mod._current_pid
53+
conn._pool_released = False
54+
conn._database = "main"
55+
conn._protocol = MagicMock()
56+
conn._db_id = 1
57+
# Suppress thread/loop binding so _check_in_use does not raise on
58+
# the first call from a synthetic test.
59+
conn._check_in_use = lambda: None # type: ignore[method-assign,unused-ignore]
60+
return conn
61+
62+
63+
def _get_run_protocol_source() -> str:
64+
"""Read the dedented source of ``_run_protocol``."""
65+
src = inspect.getsource(DqliteConnection._run_protocol)
66+
return textwrap.dedent(src)
67+
68+
69+
def test_in_use_assignment_is_first_statement_inside_try() -> None:
70+
"""Source-level pin: ``self._in_use = True`` must be the first
71+
statement INSIDE the outermost ``try:`` of ``_run_protocol`` (the
72+
one whose ``finally`` clears the flag). A regression that moves
73+
the assignment back outside the ``try:`` (or any other ordering
74+
that places it before the try-frame is established) re-introduces
75+
the KI-leak window.
76+
"""
77+
src = _get_run_protocol_source()
78+
tree = ast.parse(src)
79+
func = tree.body[0]
80+
assert isinstance(func, ast.AsyncFunctionDef), (
81+
f"expected async function, got {type(func).__name__}"
82+
)
83+
84+
# Find the outermost Try whose finally clears ``self._in_use = False``.
85+
target_try: ast.Try | None = None
86+
for stmt in ast.walk(func):
87+
if not isinstance(stmt, ast.Try):
88+
continue
89+
for fin_stmt in stmt.finalbody:
90+
if not isinstance(fin_stmt, ast.Assign):
91+
continue
92+
if (
93+
len(fin_stmt.targets) == 1
94+
and isinstance(fin_stmt.targets[0], ast.Attribute)
95+
and fin_stmt.targets[0].attr == "_in_use"
96+
and isinstance(fin_stmt.value, ast.Constant)
97+
and fin_stmt.value.value is False
98+
):
99+
target_try = stmt
100+
break
101+
if target_try is not None:
102+
break
103+
assert target_try is not None, (
104+
"expected a try/finally clearing self._in_use = False inside _run_protocol"
105+
)
106+
107+
first = target_try.body[0]
108+
assert isinstance(first, ast.Assign), (
109+
f"first statement inside the try should be an Assign; got {type(first).__name__}"
110+
)
111+
assert (
112+
len(first.targets) == 1
113+
and isinstance(first.targets[0], ast.Attribute)
114+
and first.targets[0].attr == "_in_use"
115+
and isinstance(first.value, ast.Constant)
116+
and first.value.value is True
117+
), (
118+
"first statement inside the _run_protocol try block must be "
119+
"``self._in_use = True``; placing it BEFORE the try re-introduces "
120+
"the KI-bytecode-boundary leak window"
121+
)
122+
123+
124+
@pytest.mark.parametrize(
125+
"exc_cls",
126+
[KeyboardInterrupt, SystemExit],
127+
)
128+
@pytest.mark.asyncio
129+
async def test_signal_class_exception_inside_fn_does_not_leak_in_use(
130+
exc_cls: type[BaseException],
131+
) -> None:
132+
"""Behavioural pin: a KI / SystemExit raised inside ``fn`` (the
133+
awaited body of the try) lands inside the existing
134+
``except (CancelledError, KeyboardInterrupt, SystemExit)`` arm; the
135+
finally must clear ``_in_use``. Regression-fence the surrounding
136+
contract; the structural pin above is what defends the bytecode-
137+
boundary leak. The 2078-arm calls ``_invalidate(e)`` which is
138+
expected to clear ``_in_use`` itself, but the finally runs anyway.
139+
"""
140+
conn = _make_connection()
141+
142+
async def raiser(_protocol: Any, _db_id: int) -> None:
143+
raise exc_cls()
144+
145+
with pytest.raises(exc_cls):
146+
await conn._run_protocol(raiser)
147+
assert conn._in_use is False, f"{exc_cls.__name__} escape must not leak _in_use=True"
148+
149+
150+
@pytest.mark.skipif(
151+
sys.gettrace() is not None,
152+
reason="cannot inject via sys.settrace under coverage / debugger",
153+
)
154+
@pytest.mark.asyncio
155+
async def test_kbi_injected_on_assignment_line_is_caught() -> None:
156+
"""Behavioural pin via ``sys.settrace``: inject a
157+
``KeyboardInterrupt`` exactly on the line that holds
158+
``self._in_use = True``. After the fix, the assignment is the
159+
first line inside the try block, so the synthetic KI on that line
160+
lands INSIDE the try-frame and the finally runs. Before the fix,
161+
the assignment was outside the try-frame and the KI escaped without
162+
finally — leaving ``_in_use=True`` forever.
163+
164+
This is the closest in-process repro of the "signal between two
165+
bytecodes" class — the trace hook fires per-line, simulating signal
166+
delivery on the assignment line specifically.
167+
"""
168+
conn = _make_connection()
169+
170+
src = _get_run_protocol_source()
171+
target_lineno_in_src = next(
172+
i + 1 for i, line in enumerate(src.splitlines()) if "self._in_use = True" in line
173+
)
174+
abs_lineno = DqliteConnection._run_protocol.__code__.co_firstlineno + target_lineno_in_src - 1
175+
target_filename = DqliteConnection._run_protocol.__code__.co_filename
176+
177+
fired = {"injected": False}
178+
179+
def trace_hook(frame: Any, event: str, arg: Any) -> Any:
180+
if frame.f_code.co_filename != target_filename:
181+
return None
182+
if event == "line" and frame.f_lineno == abs_lineno and not fired["injected"]:
183+
fired["injected"] = True
184+
raise KeyboardInterrupt("synthetic at-bytecode-boundary")
185+
return trace_hook
186+
187+
async def noop_fn(_protocol: Any, _db_id: int) -> None:
188+
return None
189+
190+
sys.settrace(trace_hook)
191+
try:
192+
with pytest.raises(KeyboardInterrupt):
193+
await conn._run_protocol(noop_fn)
194+
finally:
195+
sys.settrace(None)
196+
197+
assert fired["injected"], "trace hook must have fired on the target line"
198+
assert conn._in_use is False, (
199+
"KI on the assignment line must not leak _in_use=True; the "
200+
"assignment must be the first line inside the try-block so "
201+
"the finally runs"
202+
)

0 commit comments

Comments
 (0)