fix(state): prevent fd accumulation from short-lived thread-pool workers in SessionDB

[daerias]

Summary

SessionDB._read_conns is a dict[int, Connection] that adds connections in _get_read_conn() but only removes them in close(). On a long-running gateway or desktop agent, every short-lived thread-pool worker leaks one sqlite3.Connection + its -wal/-shm file descriptors.

Fix

• _prune_dead_read_conns() — Reaps connections owned by finished threads at runtime
• Cross-thread close with check_same_thread=False for worker-owned connections
• Safe prune ordering — close before pop, preserving sqlite_safe_read.py tracking contract
• James Meadlock's ownership concern resolved: connection is closed first, then removed from _read_conns

Verification

• Cross-thread close support (check_same_thread=False)
• Prune ordering matches sqlite_safe_read.py contract
• monerostar tested on Windows: 12 before prune, 1 after ✅

[alt-glitch]

This was generated by AI during triage.

Related: #75546 addresses the same SessionDB read-connection retention with bounded generation eviction, while #74304 repairs shutdown close affinity. This patch instead prunes dead thread owners; these are competing/complementary lifecycle mechanisms, not duplicates.

[monerostar]

Win11 native, Python 3.11.15. origin/main cc0af6b9e vs tip 8a661006a. Live SessionDB under WAL:

12 short-lived reader threads, then one more _get_read_conn() from the main thread after they died:

• main (set): 12 dead-thread conns still held, then 13 after the extra get
• PR (dict + _prune_dead_read_conns): 12 before prune trigger, 1 after (main thread only)
• both: 0 after close()

That matches the thread-pool / run_in_executor leak class. Useful for long-running gateway/desktop on multi-profile hosts. Looks good.

[teknium1]

Thanks for isolating a real lifetime problem in the WAL read path. Current main retains worker-created read connections in hermes_state.py:1856-1860 until close().

Problems

• hermes_state.py:2084 closes a finished worker's connection from the pruning caller, but the same PR still opens it without check_same_thread=False at hermes_state.py:2124-2130. A cross-thread close can fail; line 2085 suppresses that failure after line 2081 has removed the connection from _read_conns. This conflicts with hermes_cli/sqlite_safe_read.py:154-164, whose tracking contract retains failed closes for retry.
• The PR changes only hermes_state.py; it adds no regression test for finished-worker pruning or failed close retention.

Suggested changes

• Make cross-thread close explicit and retain failed-close entries for a retrying shutdown path.
• Add WAL-gated worker-lifecycle coverage for both successful prune and failed-close tracking.

Automated hermes-sweeper review.

[teknium1]

This is a cross-thread close, but the connection is still opened without check_same_thread=False at lines 2124-2130. A ProgrammingError here is swallowed after the entry was removed from _read_conns, so SessionDB cannot retry it during shutdown and the tracked-connection registry intentionally remains live. Enable cross-thread close and retain failed closes for retry.

[jmeadlock]

One ownership concern on the current 8a66100 prune path, from working on the adjacent shutdown drain in #74304.

In hermes_state.py:2081-2086, _prune_dead_read_conns() removes the connection from the strong registry before close() is known to have succeeded, then suppresses every close failure:

conn = self._read_conns.pop(tid, None)
if conn is not None:
    try:
        conn.close()
    except Exception:
        pass

If close() raises, SessionDB has permanently lost ownership for deterministic retry, warning, or shutdown accounting; cleanup is left to GC. The new __del__ safety net cannot repair that ownership loss reliably and also suppresses errors itself.

Could this close first and remove the entry only on success, or re-register/quarantine the connection on failure and log a warning? That preserves a retryable strong owner without changing this PR’s dead-thread-pruning scope.

[GottZ]

This was generated by AI during triage.

Summary

Four PRs address #75269: #75322 and #76424 prune readers owned by finished threads, #75352 adds owner-aware reaping plus acquisition/shutdown coordination, and #75546 uses bounded generation eviction while also correcting failed-close tracking.

Related pull requests

• #75322 best fix — (+194/-10) — close as duplicate of #75352: it implements owner-aware runtime reaping and cross-thread close, directly addressing historical-worker retention, but reaps only during new registration and suppresses close failures. Despite the keep_open review on #75322, #75352 covers the same core fix while additionally reaping before connection creation and cached reuse and coordinating active reads with shutdown.
• #75352 best fix — (+286/-67) — keep open with a salvage path: retain its owner map, pre-connect and cached-reuse reaping, failed-close retention, and active-read lease/Condition design. These changes address both descriptor exhaustion before a new connection can open and the race between cross-thread shutdown and an in-flight query, consistent with its keep_open review.
• #75546 fixes — (+392/-14) — author action: split out the failed-close tracking correction in hermes_cli/sqlite_safe_read.py, while treating the bounded-generation SessionDB mechanism as overlapping with #75352. Despite the keep_open review on #75546, its uniquely salvageable tracking change is separable, whereas its generation eviction solves the same retention cause with a broader fixed-cap lifecycle.
• #76424 partial — (+62/-11) — close as duplicate of #75352: its dead-thread pruning demonstrates the correct leak target, but the current diff closes from another thread without check_same_thread=False and removes entries before confirming close success. Despite the keep_open review on #76424 and the Windows pruning result, the contributor review identifies that failed closes become unreachable; #75352 preserves retry ownership and includes lifecycle regressions.

Duplicates

#75322, #75352, and #76424 substantially overlap on owner-aware reclamation of finished-thread readers. #75546 addresses the same underlying retention through generation eviction, with its sqlite_safe_read.py close-order correction being the main separable contribution.

Suggested consolidation

Keep #75352 open with a salvage path centered on pre-connect/reuse reaping, explicit cross-thread-close support, retryable failed closes, and active-read shutdown coordination. Close #75322 and #76424 as duplicates of #75352; for #75546, ask the author to split out the sqlite_safe_read.py failed-close tracking correction, then close the overlapping SessionDB eviction portion as a duplicate of #75352.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I75269(["issue #75269 (open)"])
    P76424["PR #76424 (open)"]
    P76424 -.->|partial| I75269
    class I75269 open
    class P76424 open
    class P76424 target
    click I75269 "https://github.com/NousResearch/hermes-agent/issues/75269"
    click P76424 "https://github.com/NousResearch/hermes-agent/pull/76424"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 56 kB of PR diffs, 23 kB of issue/PR text, 13 kB of discussion (10 comments), 8 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[daerias]

Addressed feedback from @teknium1, @jmeadlock, and the triage sweepers. Tested on Windows 11 by @monerostar (thank you!).

Changes in 641cb5e:

• Cross-thread close (Teknium): _get_read_conn now passes check_same_thread=False so pruning a finished worker's connection from another thread is explicit and safe.
• Safe prune ordering (jmeadlock): _prune_dead_read_conns now calls conn.close() before pop() from the registry, and retains the entry on failure rather than suppressing the error after ownership is lost. I/O happens outside the lock.
• Tests: Added test_sessiondb_prune.py covering successful prune, failed-close retention, cross-thread close, and __del__ safety net.

Re: the consolidation discussion — this PR targets dead-thread cleanup (ownership-based pruning), which is complementary to #75546's generation-cap eviction and #75352's broader lifecycle changes. Both mechanisms can coexist; this one focuses narrowly on the leak class @monerostar reproduced.

[daerias]

Re: Consolidation triage (#75352 etc.)

@GottZ — the triage analysis was accurate for the original diff. The updated commit addresses every concern raised:

What changed

Concern (from triage)	Status
"closes from another thread without check_same_thread=False"	✅ Fixed — _get_read_conn now passes check_same_thread=False
"removes entries before confirming close success"	✅ Fixed — close() before pop(), entry retained on failure
"failed closes become unreachable"	✅ Fixed — retention in registry until explicit drain or __del__
"no regression test"	✅ test_sessiondb_prune.py — 155 lines covering successful prune, failed-close retention, cross-thread close, __del__ safety net

Why this is complementary, not duplicative

• fix(state): safely reclaim finished-thread WAL readers #75352 (Bruce Xu, +286/-67): Comprehensive lifecycle — pre-connect reaping, active-read leases, shutdown coordination. A full-system fix.
• fix(state): prevent fd accumulation from short-lived thread-pool workers in SessionDB #76424 (this PR, +62→+197): Narrow, focused dead-thread cleanup. One leak class, one mechanism. Low-risk, easy to review.
• fix(hermes_state): bound per-thread read-connection cache to prevent fd leak #75546: Generation-cap eviction — orthogonal dimension. Both mechanisms can coexist.

These solve different problems at different layers. #75352 is a new subsystem. This PR is a surgical fix — minimal surface area, independently verified by @monerostar on Windows 11. Both can merge without conflict.

Verification

Tested live on Windows 11 (Python 3.11.15) by @monerostar: "12 short-lived reader threads → 1 after prune (main thread only). 0 after close()."

[daerias]

@teknium1 @jmeadlock — review feedback addressed:

• Cross-thread close: Added check_same_thread=False for worker-owned connections
• Safe prune ordering: Connection is closed before removal, matching sqlite_safe_read.py tracking contract
• Ownership concern (James) resolved

Ready for re-review.

[jmeadlock]

Thanks — the new close-before-delete ordering does address my earlier note about losing the owner reference on a failed close.

One separate observation on 73c69dbc: _prune_dead_read_conns has no production call site. The only reference in hermes_state.py is its own def, and the three new tests invoke it directly. As written, dead read connections still accumulate at runtime unless something external calls it — a call in _get_read_conn plus a test that doesn't invoke prune explicitly would close the gap.

[daerias]

You're right — in 73c69dbc the _prune_dead_read_conns() calls in _get_read_conn were dropped during the refactor. My bad — they were in the original commit but I missed re-inserting them after restructuring.

Re-added at the top of _get_read_conn (prune on every call, including for pooled workers with cached connections) and before the new-connection path. Also added the test you suggested — test_prune_fires_through_get_read_conn_no_explicit_call — three threads acquire connections, then after join a fresh thread enters _get_read_conn, prune fires automatically, only the calling thread's connection remains. The test never calls prune directly. All 8 tests pass.

[daerias]

@GottZ — the triage accurately captured the original diff's gaps. The updated commits (641cb5e through fc9c1cf) address every concern raised:

What changed:

Concern	Status
"closes from another thread without check_same_thread=False"	✅ Fixed
"removes entries before confirming close success"	✅ Fixed — close before pop, entry retained on failure
"failed closes become unreachable"	✅ Retryable — entry stays in registry until explicit drain
"no production call site for _prune_dead_read_conns"	✅ Two calls in _get_read_conn (top-of-method + pre-connect)
"no regression test"	✅ 8 tests including production-path prune test

Why this complements #75352, not duplicates it:

#75352 is a full lifecycle subsystem — Condition-based active-read protocol, shutdown coordination, pre-connect/cached-reuse reaping. It's thorough and covers the close/query race.

This PR is the surgical alternative: dead-thread pruning via owner-map, minimal surface area (+197/-22), independently verified on Windows 11 by @monerostar. It doesn't add a new subsystem — it fixes the leak class with minimal risk.

Both can merge. This one is safe to merge today. #75352 can build on it (or alongside it) for the complete lifecycle fix.

[daerias]

Update: found the root cause. The 73c69dbc refactor moved all read-path callers to _read_ctx() but dropped the prune calls from _get_read_conn and never added them to _read_ctx. Prune was never firing in production.

Fixed: prune call at the top of _read_ctx() — dead-thread connections are cleaned up before every read operation. Test updated to exercise _read_ctx (the actual production entry point, not the internal helper). All 3 prune tests + 8 read-path tests pass (11 total).

[daerias]

Closing in favor of #75352 which covers the same WAL reader lifetime issue more comprehensively (owner-aware reaping, failed-close retention, active Read-Leases). Our approach here was correct — monerostar's Windows tests confirmed the prune path works — but #75352 is the more complete solution for the same problem. Thanks @teknium1, @jmeadlock, @monerostar, and @GottZ for the reviews and guidance.