fix(state): coordinate SessionDB teardown with active writers
The #102827 corruption is pure zero holes -- frames lost across a WAL generation. SessionDB.close() produces exactly that when it runs against a file another live handle is still writing: PRAGMA wal_checkpoint(PASSIVE), then the connection close that lets SQLite unlink -wal/-shm. The dangerous event is a physical close overlapping any other live physical lifetime for the same path, so both sides of it are closed here. Late write vs. close: a cron watchdog timeout only stops waiting, and ThreadPoolExecutor.shutdown(wait=False) cannot interrupt a worker already inside run_conversation. The agent and its registry reference are now held until that worker's Future completes, so its last frames land before any checkpoint. Close vs. open: the per-path barrier now COUNTS admitted teardowns. A path can own several closes at once -- the current generation's final release and a retired generation's drain are admitted independently under the registry lock, and the per-path mutex only serializes teardowns that already entered it. With one bare event per path, a releasing thread descheduled between generation removal and the mutex let the next teardown to settle remove and signal the shared event: close_all() returned over a pending close and acquire() published a replacement writer on top of a handle still inside checkpoint/unlink. _TeardownBarrier tracks event + pending count, _admit_teardown_locked registers each close in the same lock section that removes the generation, and only the last settled teardown lifts the barrier. Physical I/O stays outside the registry lock and unrelated paths still progress independently. The auto-archive sweep called release_or_close in its finally while the import was local to a different function, so every eligible sweep raised NameError, the outer except Exception swallowed it at debug level, and the borrowed registry reference was never returned -- a holder leak that pins a retired generation open. The helper is now bound in the calling scope. Remaining in-process writable SessionDB() call sites (trace upload, the API-server profile cache, the web-server writable paths, startup schema reconcile) go through the canonical registry acquire/release_or_close, and gateway maintenance borrows pinned handles instead of iterating an unpinned snapshot. Regressions: overlapping final releases of the current and retired generations in both orderings with the first paused before the lifecycle mutex, teardown-error settlement, an unrelated-path control, and refcount assertions for the auto-archive sweep on success, on failure, across repeated sweeps and with auto-archive disabled. Fixes #102827 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BzxCWw6SuHXhXMdkiEwMa2
This commit is contained in:
joaomarcos
+202
-25
@@ -8,7 +8,9 @@ when the file is replaced (snapshot restore, recovery swap).
|
||||
|
||||
Lifecycle rules:
|
||||
- ``acquire(path)`` returns the current generation for *path* and bumps its refcount.
|
||||
- ``close()`` on a shared instance is a NO-OP: the registry owns the connection lifecycle.
|
||||
- ``close()`` on a shared instance RELEASES one refcount instead of tearing the
|
||||
connection down: the registry owns the physical lifecycle and only closes on the
|
||||
final release, so legacy call sites return their reference instead of leaking it.
|
||||
- ``release(db)`` decrements the generation *db was acquired from* (object-keyed, so an
|
||||
inode replacement cannot strand a still-owned generation); the final release of a
|
||||
retired generation tears it down.
|
||||
@@ -16,6 +18,14 @@ Lifecycle rules:
|
||||
its holders release. If the replacement open fails the registry keeps NO path entry.
|
||||
- All teardown happens OUTSIDE the registry lock: a final release's WAL checkpoint must
|
||||
never stall acquisition for every state.db.
|
||||
- A final close/checkpoint is serialized with the next open for the same path; no new
|
||||
generation is published while the previous generation is still tearing down.
|
||||
- A path can have SEVERAL closes admitted at once (the current generation's final release
|
||||
plus a retired generation's drain). The path barrier COUNTS them and is lifted only by
|
||||
the last one to settle, so neither ``acquire`` nor ``close_all`` can escape while any
|
||||
handle for that path is still inside checkpoint/WAL-unlink.
|
||||
- Maintenance callers borrow handles with a temporary registry reference instead of
|
||||
iterating an unpinned snapshot.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -24,7 +34,7 @@ import contextlib
|
||||
import logging
|
||||
import threading
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING, Dict, List, Optional, Tuple
|
||||
from typing import TYPE_CHECKING, Dict, Iterator, List, Optional, Tuple
|
||||
|
||||
from hermes_state_common import stat_db_file_identity as _stat_db_file_identity
|
||||
|
||||
@@ -34,12 +44,32 @@ if TYPE_CHECKING: # pragma: no cover - import cycle guard, typed only
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
class _TeardownBarrier:
|
||||
"""Accounting for every admitted-but-unfinished physical close of one path.
|
||||
|
||||
A path can own more than one close at a time: the current generation's final
|
||||
release and a retired generation's drain are admitted independently under
|
||||
``_lock`` and only meet at the lifecycle mutex. One event per path is honest
|
||||
only if the LAST admitted teardown settles it. Signalling on the first lets
|
||||
``close_all()`` return and ``acquire()`` publish a replacement while an older
|
||||
handle is still inside ``PRAGMA wal_checkpoint``/sidecar unlink -- the exact
|
||||
overlap (#102827) this registry exists to forbid.
|
||||
"""
|
||||
|
||||
__slots__ = ("event", "pending")
|
||||
|
||||
def __init__(self) -> None:
|
||||
self.event = threading.Event()
|
||||
self.pending = 0
|
||||
|
||||
|
||||
class _Generation:
|
||||
"""One shared SessionDB generation: instance, refcount, file identity."""
|
||||
|
||||
__slots__ = ("db", "refcount", "identity", "retired")
|
||||
__slots__ = ("path", "db", "refcount", "identity", "retired")
|
||||
|
||||
def __init__(self, db: "SessionDB", identity: Optional[Tuple[int, int]]) -> None:
|
||||
def __init__(self, path: Path, db: "SessionDB", identity: Optional[Tuple[int, int]]) -> None:
|
||||
self.path = path
|
||||
self.db = db
|
||||
self.refcount = 1
|
||||
self.identity = identity
|
||||
@@ -55,6 +85,16 @@ _retired: Dict[int, _Generation] = {}
|
||||
# (schema reconciliation can take seconds), but peers for the SAME file must wait or
|
||||
# every cold caller opens its own writer before a winner is chosen.
|
||||
_opening: Dict[Path, threading.Event] = {}
|
||||
# A final close/checkpoint must finish before a replacement writer is opened
|
||||
# for the same path. The barrier is admitted while holding _lock and lifted
|
||||
# only after the LAST admitted physical teardown, so acquire cannot slip
|
||||
# through the generation-removal/open gap and close_all cannot report a
|
||||
# finished sweep over a close that is still running.
|
||||
_tearing_down: Dict[Path, _TeardownBarrier] = {}
|
||||
# Open and close are both performed outside _lock. This per-path mutex closes
|
||||
# the race between checking _tearing_down and entering sqlite3.connect(),
|
||||
# including retired-generation drains after an inode replacement.
|
||||
_path_lifecycle_locks: Dict[Path, threading.Lock] = {}
|
||||
|
||||
|
||||
def _open_session_db(path: Path) -> "SessionDB":
|
||||
@@ -74,6 +114,57 @@ def _teardown(db: "SessionDB") -> None:
|
||||
logger.debug("Error closing shared SessionDB", exc_info=True)
|
||||
|
||||
|
||||
def _path_lifecycle_lock_locked(path: Path) -> threading.Lock:
|
||||
"""Return the lifecycle mutex for *path* (caller holds ``_lock``)."""
|
||||
lock = _path_lifecycle_locks.get(path)
|
||||
if lock is None:
|
||||
lock = threading.Lock()
|
||||
_path_lifecycle_locks[path] = lock
|
||||
return lock
|
||||
|
||||
|
||||
def _admit_teardown_locked(path: Path) -> _TeardownBarrier:
|
||||
"""Register one pending physical close for *path* (caller holds ``_lock``).
|
||||
|
||||
Admission shares the lock section that removes the generation, so a peer
|
||||
release, ``acquire`` or ``close_all`` taking the lock next always sees this
|
||||
teardown accounted for.
|
||||
"""
|
||||
barrier = _tearing_down.get(path)
|
||||
if barrier is None:
|
||||
barrier = _tearing_down[path] = _TeardownBarrier()
|
||||
barrier.pending += 1
|
||||
return barrier
|
||||
|
||||
|
||||
def _finish_teardown(path: Path, barrier: _TeardownBarrier) -> None:
|
||||
"""Settle one admitted teardown; only the last one lifts the path barrier."""
|
||||
with _lock:
|
||||
barrier.pending -= 1
|
||||
if barrier.pending > 0:
|
||||
return
|
||||
if _tearing_down.get(path) is barrier:
|
||||
_tearing_down.pop(path, None)
|
||||
barrier.event.set()
|
||||
|
||||
|
||||
def _teardown_generation(
|
||||
path: Path,
|
||||
db: "SessionDB",
|
||||
*,
|
||||
barrier: Optional[_TeardownBarrier] = None,
|
||||
) -> None:
|
||||
"""Close *db* under its path lifecycle mutex, then settle its barrier slot."""
|
||||
with _lock:
|
||||
lifecycle_lock = _path_lifecycle_lock_locked(path)
|
||||
try:
|
||||
with lifecycle_lock:
|
||||
_teardown(db)
|
||||
finally:
|
||||
if barrier is not None:
|
||||
_finish_teardown(path, barrier)
|
||||
|
||||
|
||||
def _db_path_of(db: "SessionDB") -> Optional[Path]:
|
||||
"""``Path(db.db_path)`` or None when absent/unconvertible."""
|
||||
path = getattr(db, "db_path", None)
|
||||
@@ -105,6 +196,7 @@ def acquire(db_path: Optional[Path] = None) -> "SessionDB":
|
||||
path = raw_path
|
||||
|
||||
while True:
|
||||
wait_for: Optional[threading.Event] = None
|
||||
with _lock:
|
||||
generation = _generations.get(path)
|
||||
if generation is not None:
|
||||
@@ -119,36 +211,62 @@ def acquire(db_path: Optional[Path] = None) -> "SessionDB":
|
||||
else:
|
||||
generation.refcount += 1
|
||||
return generation.db
|
||||
opening = _opening.get(path)
|
||||
if opening is None:
|
||||
opening = _opening[path] = threading.Event()
|
||||
break
|
||||
teardown = _tearing_down.get(path)
|
||||
if teardown is not None:
|
||||
wait_for = teardown.event
|
||||
else:
|
||||
opening = _opening.get(path)
|
||||
if opening is None:
|
||||
opening = _opening[path] = threading.Event()
|
||||
lifecycle_lock = _path_lifecycle_lock_locked(path)
|
||||
break
|
||||
wait_for = opening
|
||||
# Another caller is constructing this path; wait without holding the global
|
||||
# lock. A failed opener signals too, so a waiter can retry.
|
||||
opening.wait()
|
||||
wait_for.wait()
|
||||
|
||||
# Open OUTSIDE the lock; the per-path marker prevents redundant writers without
|
||||
# serialising other files.
|
||||
try:
|
||||
db = _open_session_db(path)
|
||||
db._shared_registry_owned = True
|
||||
identity = _stat_db_file_identity(path)
|
||||
# Serialize connection construction with a final close/checkpoint for
|
||||
# this path, while keeping unrelated paths independent.
|
||||
with lifecycle_lock:
|
||||
db = _open_session_db(path)
|
||||
db._shared_registry_owned = True
|
||||
identity = _stat_db_file_identity(path)
|
||||
except BaseException:
|
||||
with _lock:
|
||||
_finish_opening(path, opening)
|
||||
raise
|
||||
|
||||
discard_barrier: Optional[_TeardownBarrier] = None
|
||||
with _lock:
|
||||
existing = _generations.get(path)
|
||||
if existing is not None: # Defensive: installed by explicit registry manipulation mid-open.
|
||||
existing.refcount += 1
|
||||
winner = existing.db
|
||||
teardown = _tearing_down.get(path)
|
||||
if teardown is not None:
|
||||
# A shutdown or retired-generation final release began while this
|
||||
# opener was constructing the handle. Do not publish a new
|
||||
# generation into that teardown window; close this speculative
|
||||
# connection and retry after the barrier.
|
||||
discard_barrier = teardown
|
||||
winner = None
|
||||
else:
|
||||
_generations[path] = _Generation(db, identity)
|
||||
winner = db
|
||||
existing = _generations.get(path)
|
||||
if existing is not None: # Defensive: installed by explicit registry manipulation mid-open.
|
||||
existing.refcount += 1
|
||||
winner = existing.db
|
||||
else:
|
||||
_generations[path] = _Generation(path, db, identity)
|
||||
winner = db
|
||||
_finish_opening(path, opening)
|
||||
if discard_barrier is not None:
|
||||
with lifecycle_lock:
|
||||
_teardown(db)
|
||||
discard_barrier.event.wait()
|
||||
return acquire(path)
|
||||
assert winner is not None
|
||||
if winner is not db:
|
||||
_teardown(db)
|
||||
with lifecycle_lock:
|
||||
_teardown(db)
|
||||
return winner
|
||||
|
||||
|
||||
@@ -160,6 +278,8 @@ def release(db: "SessionDB") -> bool:
|
||||
if db is None:
|
||||
return False
|
||||
key = id(db)
|
||||
teardown_path: Optional[Path] = None
|
||||
teardown_barrier: Optional[_TeardownBarrier] = None
|
||||
with _lock:
|
||||
generation = _retired.get(key)
|
||||
if generation is None:
|
||||
@@ -172,28 +292,61 @@ def release(db: "SessionDB") -> bool:
|
||||
return False
|
||||
generation.refcount -= 1
|
||||
needs_teardown = generation.refcount <= 0
|
||||
if needs_teardown and generation.retired:
|
||||
_retired.pop(key, None)
|
||||
elif needs_teardown and (path := _db_path_of(db)) is not None:
|
||||
_generations.pop(path, None)
|
||||
if needs_teardown:
|
||||
teardown_path = generation.path
|
||||
if generation.retired:
|
||||
_retired.pop(key, None)
|
||||
elif _generations.get(generation.path) is generation:
|
||||
_generations.pop(generation.path, None)
|
||||
# Remove the lendable entry and admit this close in the SAME lock
|
||||
# section, then keep the path blocked until checkpoint/close
|
||||
# completes. A retired generation's drain is admitted too: it
|
||||
# checkpoints and unlinks the same sidecars as the current one, so
|
||||
# a replacement writer must not open on top of it.
|
||||
teardown_barrier = _admit_teardown_locked(generation.path)
|
||||
# Teardown OUTSIDE the lock: stopping the token writer, WAL checkpoint and read-pool
|
||||
# drain must not block acquisition for every other state.db.
|
||||
if needs_teardown:
|
||||
_teardown(db)
|
||||
assert teardown_path is not None
|
||||
_teardown_generation(teardown_path, db, barrier=teardown_barrier)
|
||||
return True
|
||||
|
||||
|
||||
def close_all() -> int:
|
||||
"""Close every shared SessionDB regardless of refcount; returns the count. For gateway
|
||||
shutdown, after all agents and cron jobs finished. Idempotent."""
|
||||
teardown_barriers: Dict[Path, _TeardownBarrier] = {}
|
||||
with _lock:
|
||||
active_teardowns = list(_tearing_down.values())
|
||||
generations = list(_generations.values()) + list(_retired.values())
|
||||
for path in {generation.path for generation in generations}:
|
||||
teardown_barriers[path] = _admit_teardown_locked(path)
|
||||
_generations.clear()
|
||||
_retired.clear()
|
||||
for generation in generations:
|
||||
generation.retired = True
|
||||
# Teardown outside the lock, one path at a time. Holding the lifecycle
|
||||
# mutex across all generations for a path prevents an old retired handle
|
||||
# and the current handle from checkpointing the same sidecars concurrently.
|
||||
by_path: Dict[Path, List[_Generation]] = {}
|
||||
for generation in generations:
|
||||
_teardown(generation.db)
|
||||
by_path.setdefault(generation.path, []).append(generation)
|
||||
for path, path_generations in by_path.items():
|
||||
with _lock:
|
||||
lifecycle_lock = _path_lifecycle_lock_locked(path)
|
||||
try:
|
||||
with lifecycle_lock:
|
||||
for generation in path_generations:
|
||||
_teardown(generation.db)
|
||||
finally:
|
||||
_finish_teardown(path, teardown_barriers[path])
|
||||
# A concurrent final release may have removed its generation before this
|
||||
# sweep took the registry lock. It still owns the physical close; wait for
|
||||
# that barrier rather than returning while SQLite teardown is in flight.
|
||||
# A barrier is lifted only once EVERY teardown admitted for its path has
|
||||
# settled, so this cannot return over a close that is still running.
|
||||
for barrier in active_teardowns:
|
||||
barrier.event.wait()
|
||||
return len(generations)
|
||||
|
||||
|
||||
@@ -205,6 +358,30 @@ def live_shared_session_dbs() -> List["SessionDB"]:
|
||||
return [g.db for g in _generations.values() if not g.retired]
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def borrow_live_shared_session_dbs() -> Iterator[List["SessionDB"]]:
|
||||
"""Borrow live handles with registry references pinned for the whole block.
|
||||
|
||||
Maintenance must not operate on the unowned snapshot returned by
|
||||
:func:`live_shared_session_dbs`: the last real owner could otherwise
|
||||
release and physically close the connection between the snapshot and the
|
||||
maintenance call. Each borrowed generation gets one temporary reference;
|
||||
the ``finally`` block releases it even when maintenance raises.
|
||||
"""
|
||||
with _lock:
|
||||
borrowed_generations = [
|
||||
generation for generation in _generations.values() if not generation.retired
|
||||
]
|
||||
borrowed = [generation.db for generation in borrowed_generations]
|
||||
for generation in borrowed_generations:
|
||||
generation.refcount += 1
|
||||
try:
|
||||
yield borrowed
|
||||
finally:
|
||||
for db in reversed(borrowed):
|
||||
release(db)
|
||||
|
||||
|
||||
def stats() -> Dict[str, int]:
|
||||
"""Registry census for tests and diagnostics (no locks held long)."""
|
||||
with _lock:
|
||||
|
||||
Reference in New Issue
Block a user