state: pool SessionDB read connections instead of leaking one per (SessionDB x thread)

[ghost]

Problem. SessionDB._get_read_conn (hermes_state.py:2065) opens its read-only
connection without check_same_thread=False, unlike both writer opens
(hermes_state.py:1909, :1957). It caches that connection in a threading.local()
(:1855) and pins it in a strong set self._read_conns (:1860) that is drained only
by close().

Starlette dispatches sync routes on anyio worker threads, so every distinct thread
that ever performs a read pins a new connection — and two fds, the database and its
-wal — for the life of the process. Nothing prunes the set. The growth is unbounded
in practice on SessionDBs that are never closed: the dashboard's module-global _db
(tui_gateway/server.py:148, assigned :1170, never closed) and each
_sessions[sid]["session_db"].

Two independent consequences:

1. fd exhaustion. The process walks into its RLIMIT_NOFILE (256 soft, which is
   what a launchd-managed service gets on macOS), after which every request fails with
   OSError EMFILE while the process stays alive — so a process supervisor's
   restart-on-exit never fires and the service hangs in a broken-but-running state
   rather than restarting.

2. The leaked connections are permanently unclosable. Because they lack
   check_same_thread=False, conn.close() raises sqlite3.ProgrammingError: SQLite objects created in a thread can only be used in that same thread. close() swallows
   it in a bare except Exception: pass (hermes_state.py:2651-2655), so the fds
   survive close() too, and has_live_connection() keeps reporting them — which
   permanently refuses the byte-probe-guarded raw-copy backup in
   _backup_db_file (hermes_state.py:1009) for that path.

   Note this is not the sibling registry-drift bug — that one is already fixed
   upstream by [Bug]: sqlite_safe_read untracks a connection before closing it, so a failed close leaves the byte-probe guard believing nothing is live #75629, which reordered _TrackingMixin.close()
   (hermes_cli/sqlite_safe_read.py:154-164) to untrack only after a successful
   super().close(). Post-[Bug]: sqlite_safe_read untracks a connection before closing it, so a failed close leaves the byte-probe guard believing nothing is live #75629 the registry is accurate; it reports these connections
   as live because they genuinely are. The count is correct and the leak is still there.

This bug class was already fixed once in this repo: gateway/readiness.py:35-40
carries a closing(...) guard citing #69678 / #69567. The SessionDB read path was
missed.

Change. Three parts, in hermes_state.py only:

• (a) Add check_same_thread=False to the read-only open (:2132 post-patch),
  matching the writer opens. Required for (b): pooled connections are handed between
  threads, which is exactly what the default thread check forbids — including on
  close().
• (b) Replace the threading.local() + unbounded strong set with a bounded
  queue.LifoQueue(maxsize=8) (:1872; import queue added at :23, previously
  absent). Acquisition goes through one seam, _checkout_read_conn() (:2166), which
  applies the WAL/read_only gate, borrows with get_nowait(), and opens a fresh
  connection only on queue.Empty; _read_ctx (:2183) calls it and in a finally:
  returns the connection with put_nowait() — closing it instead when the pool is full
  or when close() has already drained. Checkout/return gives exclusive ownership for
  the duration of the block, so no two threads ever touch one connection concurrently.
  close() (:2726-2733) drains the queue instead of the set. LIFO keeps the working
  set hot and lets idle connections age out at the tail.
• (c) Replace the bare except Exception: pass with a logged failure in
  _close_read_conn (:2152). A close that fails leaks a tracked fd; it must not be
  invisible.

_get_read_conn's "read-only opens don't work here" memo moves from the thread-local to
an instance-wide backoff timestamp _read_open_failed_at (:1893), gated on
_READ_OPEN_RETRY_SECONDS (60 s, :242). With a shared pool the open is no longer a
per-thread event, and the locked writer connection still serves reads while the backoff
holds.

It must be a timestamp rather than a sticky boolean, and this is worth stating explicitly
because the boolean is the obvious implementation: the likeliest trigger for a failed
read-only open is transient fd pressure — the very condition this patch exists to
prevent — and a SessionDB is commonly shared process-wide (the gateway hands one
instance to every agent). A permanent flag would therefore convert one momentary EMFILE
blip into a permanent global convoy, every reader serialized behind the writer lock for
the life of the process. That is a worse failure than the leak being fixed. The expiring
stamp lets the read path self-heal.

The non-WAL / read_only fallback to the locked writer connection is unchanged.

Evidence/Repro. 25 short-lived reader threads per round against a temp DB, joined
each round, measuring pinned connections and the live-connection registry:

db = SessionDB(db_path=tmp / "state.db")
db.create_session(session_id="s1", source="cli", model="m")
db.append_message("s1", role="user", content="hello graphiti world")
db.get_session("s1")                      # main-thread read

def reader():
    db.get_session("s1"); db.search_messages("graphiti", limit=5); db.get_messages("s1")

for _ in range(10):
    ts = [threading.Thread(target=reader) for _ in range(25)]
    for t in ts: t.start()
    for t in ts: t.join()
    gc.collect()
    print(len(db._read_conns), _live_connections[_key(db.db_path)])   # pre-patch
    print(db._read_pool.qsize(), _live_connections[_key(db.db_path)]) # post-patch

Measured on this branch's base (cd6585ab) vs. this branch:

cumulative threads	pre: pinned / live / process fds	post: pinned / live / process fds
25	26 / 27 / 59	8 / 9 / 39
50	51 / 52 / 109	8 / 9 / 39
125	126 / 127 / 259	8 / 9 / 40
250	251 / 252 / 509	8 / 9 / 40

Pre-patch grows +25 pinned connections and +50 descriptors per round without bound — two
fds per connection, the database and its -wal. 509 open descriptors on one temp DB
after 250 threads; a 256-fd soft limit is crossed at roughly 125. Post-patch is flat.

After db.close(): pre-patch the registry still reports 250 live connections (every
cross-thread conn.close() raised ProgrammingError into the bare except, so those
250 fds are still open — the registry is right); post-patch it reports 0.

The post-patch fd count plateaus above the 9 live connections because SQLite's unix VFS
parks a closed descriptor on a per-inode reuse list while any connection still holds
POSIX locks on that inode — the same close()-cancels-all-locks hazard documented in
hermes_cli/sqlite_safe_read.py. Those descriptors are handed back to later opens rather
than accumulating: the plateau tracks peak concurrency, not cumulative threads. This
is also why the tests assert on pool/registry counts and never on lsof.

Tests. Added tests/test_session_db_read_conn_pool.py (9 tests): pool stays bounded
across 150 short-lived threads; connections are returned and reused; a pooled connection
is usable off-thread; close() drains the pool and returns the registry to zero for a
connection opened on a since-dead thread; a read racing close() does not repopulate the
pool; concurrent reads stay correct; the backoff stamp expires; the checkout seam is the
only acquisition point; and the locked-writer fallback still serves reads.

scripts/run_tests.sh tests/test_session_db_read_conn_pool.py     # 9 passed

8 of those 9 fail against the unpatched tree (the ninth is the
reads-stay-correct-under-concurrency invariant, which must hold either way).

Regression sweep: 28 state/sqlite files (331 passed, 0 failed) and 631
gateway/dashboard/tui-gateway files (4892 passed). Two gateway failures in that sweep —
test_systemd_notify.py::test_notify_supports_systemd_abstract_socket and
test_shutdown_forensics.py::TestSpawnAsyncDiagnostic::test_spawns_subprocess_and_writes_output
— reproduce identically on unpatched cd6585ab on the same machine (Linux-only abstract
sockets; a subprocess spawn) and are unrelated to this change.

One existing test encodes the old contract and is updated with this patch:
tests/test_session_db_read_path_split.py:42

def test_read_conn_reused_within_thread(db):
    assert db._get_read_conn() is db._get_read_conn()

_get_read_conn is now an opener, not a per-thread cache, so it returns a fresh
connection each call. It is replaced in place by test_read_conn_reused_via_pool, which
pins the equivalent post-patch contract — two successive _read_ctx() blocks yield the
same object — rather than deleting the coverage. The file's module docstring is updated
to describe the pool rather than the thread-local.

Two other tests in that file were checked and deliberately left alone.
test_read_conn_is_per_thread (:29) still passes and still pins a real contract:
concurrent readers never share a connection. That guarantee simply moves from
threading.local() to checkout/return exclusivity. test_read_conn_open_failure_marks_thread
(:89) also still passes — the "don't retry the failed open on every query" property it
asserts is exactly what the backoff stamp preserves; only the scope of the memo changed
(per-thread → per-instance, with expiry), and the expiry half is covered by the new
test_read_open_failure_backs_off_but_recovers.

One note on a test that no longer exists. test_fallback_when_read_conn_unavailable
monkeypatched _get_read_conn to return None to exercise the locked-writer fallback.
It was removed from tests/test_session_db_read_path_split.py upstream (along with
test_title_resolution_does_not_take_writer_lock) some time before cd6585ab, so there
is nothing to repoint. The coverage is restored in the new file as
test_fallback_to_locked_writer_when_read_conn_unavailable, patched at the
_checkout_read_conn seam rather than at _get_read_conn — because once acquisition
checks the pool first, a patched _get_read_conn is never reached while the pool is warm
and the test would keep passing while exercising nothing.
test_checkout_seam_is_the_single_acquisition_point is added to keep it that way: it
fails if a future edit re-inlines the pool checkout into _read_ctx and silently
re-orphans the fallback test.

Alternatives considered.

• check_same_thread=False alone (a, without b). Makes close() work, but leaves
  the set growing one connection per thread for the process's life. The production
  failure is a dashboard that runs for days without ever calling close(), so this
  fixes only the shutdown symptom, not the outage.
• Keep the thread-local, prune dead threads. Requires a weak-keyed registry plus a
  reaper, and still peaks at the number of live worker threads. A bounded pool caps
  the connection count directly and is far less machinery.
• weakref.WeakSet for _read_conns. Would let dead threads' connections be
  collected, but relies on GC timing for fd release under an fd-exhaustion failure mode,
  and connections are still closed from the wrong thread. Deterministic return-to-pool
  is strictly better.
• One connection under a lock. Reintroduces exactly the convoy the read-path split
  was added to remove (a measured 0.2s FTS query stretched to 112s).
• maxsize. 8 covers the observed concurrent-read fan-out with headroom; above it,
  surplus connections are closed on release rather than queued, so the pool degrades to
  open/close per read instead of blocking or leaking.

[teknium1]

Thanks for tracing the thread-local lifetime and the tracked-close path; current main still has the reported premise at hermes_state.py:1855-1865, 2080-2115, and 2647-2656.

Problems

• hermes_state.py:2179-2181 uses a nonblocking idle-cache checkout. When N readers arrive while the pool is empty, all N can open a connection; maxsize=8 only limits later returns at 2201-2205. This does not bound peak live descriptors, so a sufficiently concurrent burst can still hit EMFILE.
• tests/test_session_db_read_conn_pool.py:65-76 checks counts only after joining workers, so it cannot detect that peak.

Suggested changes

• Bound open plus checked-out readers (for example, a nonblocking permit with locked-writer fallback once exhausted), and add a barrier-based peak-registry test.

Automated hermes-sweeper review.

[teknium1]

maxsize bounds only idle entries. If many readers reach this empty queue together, each falls through to _get_read_conn() and opens a connection, so peak tracked connections/descriptors remain unbounded. Please cap checked-out plus idle connections too, with fallback rather than an additional open once the cap is reached.

[teknium1]

This assertion runs after every worker has returned its connection, so it cannot prove the live descriptor bound during the concurrent interval. Use a barrier to hold more than maxsize readers inside _read_ctx, then assert the registry count before releasing them.

[ghost]

Pushed in 5b78c93 — you were right, and the test is why I could not see it.

The flaw. maxsize=8 bounds how many connections are returned, not how many are open. _checkout_read_conn opened unconditionally on a pool miss, so N readers arriving on a cold pool all missed, all opened, and peaked at N. The surplus was closed on release, so nothing accumulated forever — but EMFILE is a peak-instant condition, and the burst that empties the pool is exactly the burst that exhausts the fd table. Measured on the previous commit: 64 concurrent readers held 64 live connections at once, so the original wedge was still reachable.

The fix. A connection now holds a permit for its whole lifetime — acquired in _get_read_conn() before the open, released in _close_read_conn() after the close — so open+checked-out is bounded together. A pool hit costs no permit because the connection it hands back already holds one, which leaves _get_read_conn() as the only place that can open. The acquire is non-blocking, as you suggested: past the ceiling readers fall back to the locked writer connection rather than queueing, because blocking would convert descriptor exhaustion into a stall — the same outage with a different stack trace. Same burst now peaks at 8.

BoundedSemaphore rather than Semaphore so an unpaired release raises instead of silently widening the ceiling.

On the test. Agreed it structurally could not see this: the existing one joins every worker before counting, so it measures the pool at rest by construction. The new one uses a barrier — all 64 workers hold their connections until every worker has checked out, so the count taken at that moment is the actual simultaneous peak. Verified it fails against the previous commit and passes now:

WITH permit bound   : peak_checked_out=  8  peak_live=  9  fell_back=56
WITHOUT permit bound: peak_checked_out= 64  peak_live= 65  fell_back=0

Also added coverage for the writer-connection fallback, permit recovery after a failed open, and that close() releases exactly the permits it drained.

Two latent leaks turned up in the same function while doing this, both fixed here:

1. A CJK extension load that failed after a successful open returned None without closing the connection — leaking a descriptor the tracking registry still counted, the same leak shape one level down.
2. Any non-sqlite3.Error between open and return stranded a permit permanently, which would ratchet the ceiling to zero and silently demote every later read to the writer lock.

208 tests pass across the state suite. Rebase status: the branch is on cd6585abf; hermes_state.py has had no commits on main since, so this applies cleanly as-is.

[Reksely]

Production validation found two shutdown races in this bounded-pool design:

1. close() can finish while _get_read_conn() is blocked inside the physical open, after the pre-open closed check.
2. A checked-out reader can remain usable after close() returns because only idle pool entries are drained. A post-open check alone still leaves the checkout-to-context handoff race.

A robust fix registers both in-flight opens and active leases under one Condition; close() marks closed and waits for both counts to drain. Importantly, the counts remain held until the physical conn.close() completes, or shutdown can still return a few instructions too early. Deterministic blocked-open, blocked-handoff, and blocked-physical-close regressions are in commit 54f1aa6e4 on https://github.com/Reksely/hermes-agent/tree/codex/fix-gateway-fd-leaks. Focused state/gateway tests pass on current main.

One additional hardening: if conn.close() fails, do not release its descriptor permit; the FD may still exist, so releasing the permit can violate the hard ceiling.

[teknium1]

Merged via PR #83406 — your commits were cherry-picked onto current main with your authorship preserved in git log (rebase merge). Thank you for the contribution!

The omnibus combined the WAL read-connection pool (#76700), the configurable nofile floor (#77587), the parent-death watchdog (#73066), the Desktop-boot orphan serve reap incl. the backend.lock.json spare-guard (#78873), and the orphan gateway reap (#78312). All pieces were live-tested end-to-end: real orphaned processes reaped on actual Desktop boot with a lock-owned backend surviving, 151→9 connections under 150 reader threads, and zero fd growth over 800 requests against a live serve.