fix(cli): untrack sqlite connections only after close succeeds
A failed close left the FD open while the byte-probe guard thought nothing was live. Keep the registry entry until close actually works.
This commit is contained in:
@@ -40,7 +40,7 @@ lifecycle**. ``_live_lock`` is therefore held across three critical sections,
|
||||
each of which spans the syscall *and* the registry mutation:
|
||||
|
||||
* open + register (:func:`connect_tracked`)
|
||||
* unregister + close (:meth:`TrackedConnection.close`)
|
||||
* close + unregister (:meth:`TrackedConnection.close`)
|
||||
* check + ``open``/``read``/``close`` (:func:`read_header_bytes_preopen`)
|
||||
|
||||
Without that, a thread could pass the "no live connection" check, a second
|
||||
@@ -154,10 +154,14 @@ class _TrackingMixin:
|
||||
def close(self) -> None: # type: ignore[misc]
|
||||
with _live_lock:
|
||||
path = getattr(self, "_hermes_tracked_path", None)
|
||||
# Close first; untrack only once the descriptor is actually gone.
|
||||
# Untracking before a failing close (e.g. cross-thread
|
||||
# ProgrammingError) leaves the FD open while the byte-probe
|
||||
# guard thinks nothing is live — see #75629.
|
||||
super().close() # type: ignore[misc]
|
||||
if path is not None:
|
||||
self._hermes_tracked_path = None
|
||||
untrack_connection(path)
|
||||
super().close() # type: ignore[misc]
|
||||
|
||||
|
||||
class TrackedConnection(_TrackingMixin, sqlite3.Connection):
|
||||
@@ -169,9 +173,11 @@ class TrackedConnection(_TrackingMixin, sqlite3.Connection):
|
||||
method every close path must go through — keeps the registry from
|
||||
drifting upward and permanently disabling byte-probes.
|
||||
|
||||
The unregister and the real ``close()`` happen together under
|
||||
The real ``close()`` and the unregister happen together under
|
||||
``_live_lock`` so a concurrent probe can never observe "no live
|
||||
connection" while this descriptor is still open.
|
||||
connection" while this descriptor is still open. Unregister runs only
|
||||
after ``close()`` succeeds; a raising close leaves the connection
|
||||
tracked so the byte-probe guard keeps refusing.
|
||||
|
||||
Note ``with conn:`` does NOT close a sqlite3 connection (it only commits or
|
||||
rolls back), so this hook is not fired spuriously by transaction scopes.
|
||||
|
||||
@@ -162,6 +162,44 @@ def test_tracking_registry_does_not_leak_across_close_paths(tmp_path, clean_regi
|
||||
assert not has_live_connection(db)
|
||||
|
||||
|
||||
def test_failed_close_keeps_connection_tracked(tmp_path, clean_registry):
|
||||
"""A raising close must not release the registry (#75629).
|
||||
|
||||
Untracking before ``super().close()`` leaves the descriptor open while
|
||||
``has_live_connection`` reports false, so the byte-probe guard permits
|
||||
``open``/``close`` on a live database — cancelling POSIX advisory locks.
|
||||
"""
|
||||
from hermes_cli.sqlite_safe_read import connect_tracked
|
||||
|
||||
class ControllableConnection(sqlite3.Connection):
|
||||
def close(self):
|
||||
if getattr(self, "_hermes_fail_close", False):
|
||||
raise sqlite3.ProgrammingError(
|
||||
"SQLite objects created in a thread can only be used in "
|
||||
"that same thread"
|
||||
)
|
||||
return super().close()
|
||||
|
||||
db = tmp_path / "state.db"
|
||||
_make_db(db, "WAL")
|
||||
|
||||
conn = connect_tracked(db, factory=ControllableConnection)
|
||||
assert has_live_connection(db)
|
||||
assert read_header_bytes_preopen(db, length=16) is None
|
||||
|
||||
conn._hermes_fail_close = True
|
||||
with pytest.raises(sqlite3.ProgrammingError):
|
||||
conn.close()
|
||||
|
||||
assert has_live_connection(db), "failed close must leave the registry entry"
|
||||
assert read_header_bytes_preopen(db, length=16) is None
|
||||
|
||||
conn._hermes_fail_close = False
|
||||
conn.close()
|
||||
assert not has_live_connection(db)
|
||||
assert read_header_bytes_preopen(db, length=16) is not None
|
||||
|
||||
|
||||
def test_probe_and_connect_do_not_race(tmp_path, clean_registry, monkeypatch):
|
||||
"""The check and the raw read must be atomic w.r.t. connection lifecycle.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user