fix(state): close threaded SessionDB read conns without leaking FDs

[jmeadlock]

Summary

Fixes a specific file-descriptor leak in SessionDB's per-thread WAL read path.

SessionDB._get_read_conn() creates read connections on worker threads, while SessionDB.close() drains the tracked connections from its caller after those workers finish. The read connections used SQLite's default check_same_thread=True, so owner-thread shutdown raised sqlite3.ProgrammingError. Because shutdown swallowed that exception, the SQLite .db / .wal / .shm descriptors remained open.

The observed behavior is consistent with the repeated SQLite handles seen in long-lived gateway/dashboard processes. This PR fixes that confirmed SessionDB lifecycle bug; it does not claim to address every possible source of process FD growth.

Fix

• Open per-thread read connections with check_same_thread=False, matching the existing writer and cross-profile read-only connection policy.
• Log a concise warning if SessionDB.close() still encounters a thread-affinity ProgrammingError while draining a reader.

The lifecycle contract covered here is shutdown after the worker using the connection has finished. This patch does not introduce a new concurrent-close protocol for an in-flight query.

The read connections remain thread-local during normal use; the cross-thread reference exists only in the shutdown drain set. The runtime SQLite build is serialized (THREADSAFE=1), ordinary overlapping SQL close waits for the query to finish, and these connections register no Python SQLite callbacks (create_function, create_aggregate, create_collation, set_progress_handler, set_trace_callback, or set_authorizer). sqlite3.Row is the only read-path row factory. Existing _read_conns_closed handling immediately closes and rejects a reader opened after shutdown begins.

Regression coverage

The test exercises the behavior directly and without platform-specific FD APIs:

1. A worker thread creates its per-thread read connection and exits.
2. The owner thread calls SessionDB.close().
3. The retained connection must report closed database.

The same invariant fails on upstream main with the original same-thread ProgrammingError and passes on this branch.

Test plan

• scripts/run_tests.sh tests/test_session_db_read_path_split.py — 10 passed
• uv run ruff check hermes_state.py tests/test_session_db_read_path_split.py
• git diff --check
• Upstream CI

Related reports — not claimed fixed by this PR

• Adjacent FD / EMFILE reports: Gateway hits macOS fd limit (256): OSError Too many open files #30230, Cron SessionDB timeout and lazy recall paths leak file descriptors #72782, cron: run_job() leaks a SQLite connection on every wake-gate/skip tick (fd exhaustion → EMFILE) #60859

[jmeadlock]

Opus review (Nous Portal anthropic/claude-opus-4.8) + follow-up

Verdict: APPROVE-WITH-NITS → nits addressed in follow-up commit.

Opus findings (summary)

• Production one-liner is correct and measured (num_fds 30→3 fixed vs 30→29 pre-fix).
• Test integrity issue: path-count open_files() asserts would pass pre-fix on macOS; real signal is cross-thread ProgrammingError + process num_fds.
• Log ProgrammingError in the drain loop so a future regression is visible.
• Do not force-close / refactor mixin in this PR.
• Scope is one leak class, not the entire dashboard EMFILE incident.

Follow-up pushed

• Tests tightened to primary ProgrammingError guard + num_fds delta.
• SessionDB.close() warns on sqlite3.ProgrammingError during read-conn drain.

Raw review artifact: local ~/.hermes/work/pr-74304-opus-out.txt (SHA256 fedc15a5b6b89c445429334ba02ac94d0d47cfec2c668391514ecbfa65b001e5).

[jmeadlock]

Reconsidered and tightened this PR before requesting CI/review:

• replaced the process-wide num_fds() assertions and thread-pool choreography with one direct, cross-platform lifecycle contract;
• verified that contract fails on current upstream main with the original same-thread ProgrammingError and passes on this branch;
• reduced the total PR delta to 30 added lines across the implementation and regression test;
• narrowed the PR wording: this fixes one confirmed SessionDB leak class consistent with the observed dashboard/gateway FD pattern, not every possible source of FD growth.

Local verification: targeted repository runner 10/10, Ruff clean, and git diff --check clean.

[teknium1]

Thanks for the focused lifecycle fix. Current main still creates the per-thread read connection without check_same_thread=False at hermes_state.py:2030-2036, while SessionDB.close() drains the cross-thread _read_conns set and suppresses exceptions at hermes_state.py:2529-2537. The proposed change directly aligns the reader with the existing writer and read-only connection policy (hermes_state.py:1849-1856, hermes_state.py:1898-1910).

The regression test exercises the relevant completed-worker shutdown contract rather than relying on platform-specific FD counting. Current tests already establish that reader connections are created per worker thread (tests/test_session_db_read_path_split.py:29-40).

Automated hermes-sweeper review.

[dklangst-sys]

Confirmed the premise here independently on macOS — the cross-thread drain does raise, universally:

check_same_thread=default(True)  -> ProgrammingError: SQLite objects created in a thread
                                    can only be used in that same thread
check_same_thread=False          -> cross-thread close OK

And the descriptor consequence, measured with lsof rather than the tracking registry: 8 reader threads created 9 descriptors on state.db, and after SessionDB.close() all 9 were still open. Applying check_same_thread=False is what makes that drain to 0.

One thing worth pairing with this PR: #75629. The swallowed ProgrammingError this PR removes was also corrupting the live-connection registry, because TrackedConnection.close() in hermes_cli/sqlite_safe_read.py calls untrack_connection() before super().close() and does not undo the decrement when the close raises. Net effect on current main: descriptor still open, has_live_connection() returns False, and read_header_bytes_preopen() will then byte-probe a live database — cancelling every POSIX advisory lock the process holds on it.

This PR removes the dominant cause of that failed close, which is genuinely most of the exposure. But the ordering bug survives it: any other close failure (I/O error, an already-closed handle, a caller-supplied connection factory) still under-counts the registry, and that registry is a corruption-safety guard rather than bookkeeping. Suggest landing #75629 alongside so the guard is correct regardless of why a close failed — otherwise this PR has the side effect of making the remaining ordering bug much harder to ever observe.

Also relevant to the "not claimed fixed by this PR" section: #75269 covers the pre-shutdown retention (readers from finished threads never reaped while the shared SessionDB stays alive), which is the part that actually exhausts a long-lived dashboard. I implemented that reap on top of check_same_thread=False and the two compose cleanly — 30 dead reader threads settle at 3 descriptors, and close() then drains to 0.

[kwlfmarketing]

Thank you for isolating the cross-thread reader-connection leak. I independently confirmed that check_same_thread=False is required for the completed-worker shutdown contract. This PR clearly owns the focused diagnosis and completed-worker shutdown regression. However, #75546's current patch now also sets check_same_thread=False and covers cross-thread shutdown draining, so the implementations now overlap.

The independent review was bound to baseline 38c09e5d739fd91b8f7d281ff92e3e321312cb3c; on that baseline, both reviewed source blobs were byte-identical to target 7f4d155159e2a5d4098bb2f27d3fccb01ff84c3d. That baseline includes successful-close-only descriptor unregistering in a266155cc, but it does not include the cross-thread-closeable reader change in this PR or several adjacent lifecycle protections.

A reconciled solution still needs to ensure the full lifecycle matrix: route the compression-holder read through _read_ctx() and sample expiry after fallback-lock acquisition; quiesce active readers before shutdown with a bounded five-second fail-closed timeout; reject pre-yield and post-shutdown read races; and retain failed closes across eviction and shutdown, late shutdown-time opens, and post-open setup cleanup failures under strong ownership for retry.

A reviewed two-file candidate covering these lifecycle changes was tested against the byte-identical source blobs. The unchanged source produced 14 passes and 8 intended focused failures. The candidate passed 22 focused tests and 200 broader related tests; a repeat stress probe passed 20/20. Ruff, static, and security checks passed, and an independent exact-diff review reported no blocking findings.

Two newer normal-lifetime retention proposals also overlap this area. #75546's current patch adds bounded generation eviction, per-connection RLock serialization, check_same_thread=False, and eviction-time failed-close re-registration. #76424 proposes dead-thread pruning and a best-effort __del__ drain. In #76424's current patch, _prune_dead_read_conns() removes each entry before attempting close() and suppresses close exceptions; __del__ also suppresses errors escaping close(). A failed close can therefore leave the connection outside SessionDB's strong registry and unavailable for retry. Any reconciliation should preserve each contributor's normal-lifetime work while correcting failed-close ownership.

Rather than publish a competing replacement, I would prefer to preserve this PR's authorship and coordinate it with #73803, #75546, and #76424. #73803 contributes the handoff-read race reproduction, routing, diagnostics, and WAL-path coverage; #75546 and #76424 contribute overlapping normal-lifetime retention approaches. Would the authors and maintainers prefer these overlapping changes to be reconciled first, with explicit credit preserved for each contributor's original work, followed by one narrow contribution for only the remaining lifecycle matrix? I can provide the deterministic regressions and reviewed reference patch. Its SHA-256 is f3357a7dac4f81dea9b36aa285334e84962e4975d72c2cb74e4dd9411f6e3a62.

[GottZ]

This was generated by AI during triage.

You are asking how this repository should reconcile several overlapping lifecycle changes while preserving each contributor's credit and avoiding a competing replacement.

Case context, measured live from our triage graph (2026-08-02T01:57:00+00:00):

• This PR has been open for 3 days; the median open PR is 41 days old and p90 is 93 days — which is below the median wait for this queue.
• Our graph currently records no duplicate candidate for this one — it is queued on its own merits.

If you want to move this one along: keep the diff scoped and rebase onto current main so the change stays cheap to verify.

[jmeadlock]

Rebased onto current main (927662e); the rebased PR head is 2dcb187.

Scope is unchanged: git range-diff maps all three commits exactly (=), the aggregate patch ID is identical, and the diff remains 2 files / 30 additions:

• hermes_state.py
• tests/test_session_db_read_path_split.py

Local verification on the rebased head:

• scripts/run_tests.sh tests/test_session_db_read_path_split.py — 8 passed
• related SessionDB slice (5 files) — 158 passed
• Ruff on both changed files — clean
• git diff --check origin/main...HEAD — clean

GitHub currently reports no CI checks for this branch, so those results are local-only.

I’m keeping this PR limited to the completed-worker shutdown contract: cross-thread-closeable WAL readers, a visible warning if the shutdown drain still hits thread affinity, and the focused regression. The normal-lifetime cache/eviction, dead-thread pruning, quiesce, and handoff-read work should stay with #75546, #76424, and #73803 rather than growing this diff.

I have no preference about authorship or which PR lands first—whatever gets the bug fixed cleanly. Could maintainers make a merge-order call so these branches stop overlapping? If a verified superset lands first, I’m happy to close this one; whichever path wins should retain the completed-worker shutdown regression.

@kwlfmarketing: could you publish the reference patch as a branch or attach the diff? A SHA-256 and test counts aren’t enough to compare or reproduce it. If the actual patch supersedes this cleanly, that makes the decision easy.

[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.