fix(hermes_state): bound per-thread read-connection cache to prevent fd leak

[RGerrish]

Summary

SessionDB._get_read_conn() opens a per-thread read-only SQLite connection under WAL, caches it in threading.local, and pins it in the strong set self._read_conns. That set is only drained by close(). For process-lifetime SessionDB handles — the desktop dashboard backend's global _get_db(), and the gateway runner's SessionDB — close() never runs, so every unique worker thread that ever performs a read leaves a permanent state.db + state.db-wal (+-shm) connection behind. Under sustained polling the thread pool churns, the cache grows unboundedly, the process pins against RLIMIT_NOFILE, and unrelated opens (even os.scandir on the skills tree during /api/profiles/sessions) start failing with EMFILE / Errno 24.

Reproduction path

1. Run the desktop dashboard backend (hermes dashboard — launchd-managed in our setup, KeepAlive) for several hours. It keeps one process-lifetime SessionDB via _get_db() (tui_gateway/server.py).
2. Drive it with any sustained read load — local sidebar refresh, or a remote desktop client over Tailscale (the tui_gateway websocket accepts remote peers).
3. Watch lsof -p <pid> | grep -c 'state.db' grow monotonically. In our case: ~80 leaked connections ≈ 160 fds after ~10h (evening local session + a morning remote session from a second machine).
4. The process hits its fd soft limit (launchd-spawned processes inherit launchctl limit maxfiles = 256, NOT the interactive-shell ulimit -n from .zshrc). After that, every new open fails — the desktop session dies with OSError: [Errno 24] Too many open files on a filesystem scan (/api/profiles/sessions → _count_skills → rglob → os.scandir), which is the visible symptom.

The broken code: hermes_state.py SessionDB._get_read_conn() — per-thread connection registered in self._read_conns (strong set), drained only in close(); the cache has no bound.

Fix

Bound the per-thread read-connection cache:

• Class constant _MAX_READ_CONNS = 32 — cap on self._read_conns.
• Generation counter self._read_gen on the instance.
• _get_read_conn():
  • On cache hit, if the cached connection's generation doesn't match the current _read_gen, close the stale connection and reopen (threads whose connection was evicted lazily re-establish).
  • On registration, if len(_read_conns) >= _MAX_READ_CONNS, evict + close the whole generation (close every tracked connection, clear the set, bump _read_gen).

Safety: eviction only closes idle read-only WAL connections; the writer connection and close() semantics are untouched. The reopen cost is one sqlite3.connect per evicted thread — negligible compared to pinning the whole process against its fd limit.

Why a cap instead of just raising RLIMIT_NOFILE

The leak is unbounded (one connection per unique thread), so a higher limit only delays the crash. Bumping the limit would also require launchd plist changes since daemons don't inherit shell ulimits; a cap fixes the root cause for every deployment.

Verification

• Threaded stress: 80 reads across 40 threads × 2 waves → 0 errors, len(_read_conns) bounded (16 << 32), generation bumped on overflow.
• Test suite (canonical scripts/run_tests.sh): 196 passed / 0 failed across test_session_db_read_path_split, test_hermes_state, test_hermes_state_compression_locks, test_hermes_state_readonly_preflight, test_hermes_state_wal_fallback, test_zeroed_state_db, test_pty_session, test_web_server_sessiondb_eventloop.
• Live: dashboard backend fd count 315 → 52 after restart, state.db handles 160 → 0, stable over repeated checks. Gateways restarted clean with the fix.

Related

• PR fix(process_registry): release Popen/PTY handles when a session finishes #75162 fixed the sibling leak class (process_registry Popen/PTY handles). This is the same symptom ("file descriptor limit" / Errno 24) from a different source: SQLite read connections on long-lived handles rather than subprocess pipes.

[teknium1]

Thanks for identifying a real lifetime issue in the WAL read path: current main retains each per-thread reader in SessionDB._read_conns until close() (hermes_state.py:1800-1805, 2529-2537), while the TUI gateway keeps its launch-profile DB open (tui_gateway/server.py:1128-1152).

Problems

• The proposed generation eviction clears _read_conns, but the evicted objects remain in their owner threads' threading.local state. Idle owners need not read again, and SessionDB.close() can no longer reach connections removed from the set. This does not establish a bound on live descriptors.
• The proposed overflow loop closes connections from an arbitrary registering thread. Those readers are created without check_same_thread=False (hermes_state.py:2030-2036). If a close fails, the proposed broad exception handler hides it; meanwhile sqlite_safe_read._TrackingMixin.close() untracks before its underlying close (hermes_cli/sqlite_safe_read.py:154-160), so a failed close can leave a live FD untracked.
• The PR changes only hermes_state.py; it adds no overflow/lifetime regression test. Existing coverage in tests/test_session_db_read_path_split.py covers creation and reuse, not retained idle owners after eviction.

Suggested changes

• Use an owner-safe lifecycle that bounds actual descriptors and only unregisters after a successful close.
• Add a multi-wave WAL test with idle first-wave threads, plus tracking and shutdown assertions.

Automated hermes-sweeper review.

[RGerrish]

Revision addressing reviewer feedback

Thanks for the review — all three points are addressed in the follow-up commit (0f658490e). The eviction now uses an owner-safe lifecycle that bounds live descriptors and only unregisters after a successful close.

1. Real bound on live descriptors (was: evicted objects orphaned in thread-locals, unreachable by close())

The evictor no longer relies on the owner thread to close stale connections. On overflow it closes every evicted connection itself, synchronously, through _close_read_conn. A connection is only removed from _read_conns after close() has succeeded:

• close failed → the connection is re-registered into _read_conns, so SessionDB.close() at shutdown retries it
• close succeeded → it is removed; the owner's next read discards the stale thread-local via the generation check and reopens

So a live fd is never dropped from the registry it is reachable through, and the set size is an honest bound on live descriptors. Added test_read_conn_cache_bound_evicts_idle_first_wave — a multi-wave WAL test where wave-1 threads go idle and never re-read; the assertion is that the evictor closed their connections (they were never touched by their owners).

2. Owner-safe cross-thread close (was: close-from-arbitrary-thread + hidden failures + untrack-before-close)

• Read connections are now created with check_same_thread=False and carry a per-connection RLock, held for the whole _read_ctx block. _close_read_conn acquires that lock (bounded by _READ_CONN_CLOSE_TIMEOUT = 2.0s) before closing, so a connection can never be closed mid-statement. Added test_eviction_waits_for_inflight_read (evictor blocks on a held read lock; closes only after the read ends).
• Close failures are logged (not silently swallowed) and cause re-registration instead of disappearance. Added test_failed_close_stays_registered_and_tracked: a simulated close failure leaves the connection in _read_conns and tracked in the byte-probe registry; the shutdown drain then succeeds and untracks.
• Concurrent double-close is impossible via the conn._hermes_read_closed guard — sqlite3 does not tolerate racing close() on the same connection.
• _read_ctx re-verifies the connection is still the current generation after acquiring its lock (the evictor can race the handoff between _get_read_conn and the lock acquisition), reopens under a fresh generation, and falls back to the locked writer path under pathological churn.
• _TrackingMixin.close() now untracks only after the underlying close succeeds, so a failed close can no longer leave a live fd untracked from the byte-probe guard. Normal closes behave identically (untrack exactly once, under _live_lock); existing test_sqlite_lock_safe_inspection.py registry invariants (double-close idempotence, nested lifetimes, churn) still pass unchanged.

3. Regression coverage

Four new tests in tests/test_session_db_read_path_split.py:

Test	Covers
test_read_conn_cache_bound_evicts_idle_first_wave	multi-wave WAL, idle first-wave threads, evictor-close assertion, bound + generation + lazy reopen
test_eviction_waits_for_inflight_read	owner-safe close (evictor blocks on an in-flight read)
test_failed_close_stays_registered_and_tracked	only-unregister-after-successful-close + tracking-registry invariant
test_close_drains_read_conns_and_untracks	shutdown drain reaches every per-thread conn, clears set, untracks, no reopen after close

Verification

• scripts/run_tests.sh tests/test_session_db_read_path_split.py tests/test_sqlite_lock_safe_inspection.py tests/tools/test_process_registry.py — 68/68 pass
• Ad-hoc concurrency stress (12 threads, bound=6, 4s, real _read_ctx queries): 56,944 reads, 0 errors, 111 evictions, bound held, clean drain
• Full suite running (scripts/run_tests.sh)

[ciphercommand]

Added a third commit to this PR: idle-eviction for the bounded read-conn cache (a16a2d945).

Why it's needed: the _MAX_READ_CONNS=32 bound stops runaway growth, but a long-lived handle that goes quiet still parks at the cap forever — we observed ~50 conns / ~240 FDs on an idle dashboard (94% of the macOS 256 soft limit). The pool never drains on its own.

What it does: a per-instance daemon sweeper (_read_conn_sweeper_loop, started lazily on the first registered read connection) wakes every _READ_CONN_IDLE_SWEEP_INTERVAL and closes connections idle past _READ_CONN_IDLE_TIMEOUT — even while the cache is under the bound. Owners detect the close via the existing conn._hermes_read_closed flag and reopen lazily on their next read; no generation bump, so active threads keep warm connections.

Design constraints preserved from commits 1–2:

• Sweeper is per-SessionDB (one daemon thread, started lazily)
• No-op for read_only=True instances (they return None from _get_read_conn)
• Idle detection uses a _hermes_last_used stamp, not wall-clock sampling

Tests: tests/test_session_db_idle_eviction.py (5 tests, incl. a real end-to-end daemon run with tiny timeouts proving the pool drains to zero and re-warms lazily). Full hermes_state suite green locally: 65/65 + read-path split 12/12 + idle-eviction 5/5.

[teknium1]

Resolved on main by PR #83406 (rebase-merged), which landed a bounded read-connection pool for SessionDB: reads borrow from a LIFO pool (max 8) with a lifetime permit per open connection, bounding PEAK descriptors, and close() drains from any thread. Your diagnosis of the (SessionDB x thread) accumulation was correct — thank you for working on this. The pool approach from #76700 was chosen among the four competing fixes because it bounds peak descriptors rather than reaping after the fact; @Yishova's commits carry the class fix, with credit to all four submitters for converging on the root cause.