mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(memory): share one SQLite connection per holographic store database
Every MemoryStore instance opened its own SQLite connection guarded by its own RLock. Several providers coexist in one process (the main agent plus every delegate_task subagent), so instances pointing at the same memory_store.db raced as independent WAL writers. Combined with writes that were not rolled back on error, one connection could leave an open write transaction that pinned the write lock and made every other connection's writes fail with "database is locked" for the full busy timeout. Instances for the same database now share ONE process-wide connection and ONE re-entrant lock, so access is fully serialized and cross-connection contention is impossible. The shared connection is refcounted: closing one instance never tears it out from under a live sibling, and the last close releases it. The connection runs in autocommit (isolation_level=None) so a write that raises mid-method can never leave a dangling transaction holding the write lock; the existing explicit commit() calls become harmless no-ops. The provider's shutdown() now calls the refcount-guarded close() instead of just dropping the reference: leaving finalization to GC kept the connection (and its write lock) alive indefinitely on long-running gateways, prolonging the exact contention this fix removes. The last provider now releases the connection deterministically while siblings stay live; regression tests fail without the wiring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
79f1274802
commit
b5226caff8
3 changed files with 294 additions and 16 deletions
|
|
@ -251,12 +251,12 @@ class HolographicMemoryProvider(MemoryProvider):
|
|||
logger.debug("Holographic memory_write mirror failed: %s", e)
|
||||
|
||||
def shutdown(self) -> None:
|
||||
# Close the SQLite connection deterministically instead of leaking it
|
||||
# to GC. MemoryStore opens its connection with check_same_thread=False
|
||||
# (store.py), so without an explicit close() the sqlite3.Connection's
|
||||
# fd is released by refcount/GC at a non-deterministic time on a
|
||||
# non-deterministic thread, churning a DB fd through the kernel's free
|
||||
# pool on every session teardown. close() already exists and is cheap.
|
||||
# Release the shared SQLite connection deterministically on the
|
||||
# caller's thread. Dropping the reference alone leaves fd finalization
|
||||
# to GC, which keeps the connection (and its write lock) alive on a
|
||||
# long-running gateway and prolongs the "database is locked" contention
|
||||
# this store's shared-connection refcounting is meant to eliminate.
|
||||
# close() is idempotent and refcount-guarded, so siblings stay safe.
|
||||
if self._store is not None:
|
||||
try:
|
||||
self._store.close()
|
||||
|
|
|
|||
|
|
@ -98,6 +98,21 @@ def _clamp_trust(value: float) -> float:
|
|||
class MemoryStore:
|
||||
"""SQLite-backed fact store with entity resolution and trust scoring."""
|
||||
|
||||
# --- Process-wide shared connection registry -------------------------
|
||||
# SQLite permits only one writer at a time. Each MemoryStore instance used
|
||||
# to open its own connection guarded by its own RLock, so the several
|
||||
# providers that coexist in one process (the main agent plus every
|
||||
# delegate_task subagent) raced as independent WAL writers. Combined with
|
||||
# writes that were not rolled back on error, one connection could leave an
|
||||
# open write transaction that pinned the write lock and made every other
|
||||
# connection's write fail with "database is locked" for the full busy
|
||||
# timeout. All instances for the same database now share ONE connection and
|
||||
# ONE re-entrant lock, so access is fully serialized and cross-connection
|
||||
# contention is impossible. The shared connection is refcounted, so closing
|
||||
# one instance never tears the connection out from under a live sibling.
|
||||
_shared: dict = {}
|
||||
_shared_guard = threading.Lock()
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
db_path: "str | Path | None" = None,
|
||||
|
|
@ -112,14 +127,35 @@ class MemoryStore:
|
|||
self.default_trust = _clamp_trust(default_trust)
|
||||
self.hrr_dim = hrr_dim
|
||||
self._hrr_available = hrr._HAS_NUMPY
|
||||
self._conn: sqlite3.Connection = sqlite3.connect(
|
||||
str(self.db_path),
|
||||
check_same_thread=False,
|
||||
timeout=10.0,
|
||||
)
|
||||
self._lock = threading.RLock()
|
||||
self._conn.row_factory = sqlite3.Row
|
||||
self._init_db()
|
||||
|
||||
# Acquire (or open) the process-wide shared connection for this DB.
|
||||
self._key = str(self.db_path)
|
||||
with MemoryStore._shared_guard:
|
||||
entry = MemoryStore._shared.get(self._key)
|
||||
if entry is None:
|
||||
conn = sqlite3.connect(
|
||||
self._key,
|
||||
check_same_thread=False,
|
||||
timeout=10.0,
|
||||
# Autocommit: every statement is its own transaction, so a
|
||||
# write that raises mid-method can never leave a dangling
|
||||
# transaction (and its write lock) open. The explicit
|
||||
# commit() calls below become harmless no-ops.
|
||||
isolation_level=None,
|
||||
)
|
||||
conn.row_factory = sqlite3.Row
|
||||
entry = {"conn": conn, "lock": threading.RLock(), "refs": 0, "ready": False}
|
||||
MemoryStore._shared[self._key] = entry
|
||||
entry["refs"] += 1
|
||||
self._entry = entry
|
||||
self._conn = entry["conn"]
|
||||
self._lock = entry["lock"]
|
||||
|
||||
# Initialise the schema once per shared connection.
|
||||
with self._lock:
|
||||
if not self._entry["ready"]:
|
||||
self._init_db()
|
||||
self._entry["ready"] = True
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Initialisation
|
||||
|
|
@ -575,8 +611,25 @@ class MemoryStore:
|
|||
return dict(row)
|
||||
|
||||
def close(self) -> None:
|
||||
"""Close the database connection."""
|
||||
self._conn.close()
|
||||
"""Release this instance's reference to the shared connection.
|
||||
|
||||
The underlying connection is closed only when the last MemoryStore
|
||||
referencing the same database is closed, so closing one instance can
|
||||
never break sibling instances that still hold it. Idempotent.
|
||||
"""
|
||||
if getattr(self, "_entry", None) is None:
|
||||
return
|
||||
with MemoryStore._shared_guard:
|
||||
entry = self._entry
|
||||
if entry is None:
|
||||
return
|
||||
entry["refs"] -= 1
|
||||
if entry["refs"] <= 0:
|
||||
try:
|
||||
entry["conn"].close()
|
||||
finally:
|
||||
MemoryStore._shared.pop(self._key, None)
|
||||
self._entry = None
|
||||
|
||||
def __enter__(self) -> "MemoryStore":
|
||||
return self
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue