Skip to content

fix(state): reap finished-thread WAL read connections at runtime (#75269) - #75322

Closed
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/75269-sessiondb-read-conn-reap
Closed

fix(state): reap finished-thread WAL read connections at runtime (#75269)#75322
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/75269-sessiondb-read-conn-reap

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Summary

Fixes #75269.

A long-lived shared SessionDB retained one read-only SQLite WAL connection for every worker thread that ever used the read path. _get_read_conn() cached the connection in threading.local() but also added it to the strong _read_conns set, which was drained only by SessionDB.close(). In a long-lived gateway, close() may not run for days, so descriptor usage grew with the historical worker count rather than current reader concurrency — eventually exhausting RLIMIT_NOFILE (EMFILE).

Root cause

_read_conns was a strong set with no runtime reaping. The unbounded retention was introduced when tracked readers were added in f228e145ba.

Fix

  1. Track the owning thread per connection: _read_conns is now dict[Connection, Thread] instead of set.
  2. Reap at runtime: _reap_dead_read_conns() closes and drops connections whose owner thread has exited (not thread.is_alive()), invoked lazily when a new connection registers (under the existing _read_conns_lock).
  3. Allow cross-thread close: read connections now open with check_same_thread=False (matching the writer and cross-profile read-only connections), so the reaper can close a finished worker's connection from a different thread. These are SELECT-only, autocommit, no Python callbacks — safe for serialized SQLite.

close() still drains whatever remains; list(dict) yields connection keys and .clear() works on the dict.

Why not close-on-every-_read_ctx?

The issue body notes this avoids the leak but adds ~20× overhead for get_session() and ~10× for search_messages() in local benchmarks.

Relationship to #74304

#74304 (jmeadlock, OPEN) is complementary: it adds check_same_thread=False for shutdown-time cross-thread drain. It does not add runtime reaping, so descriptor growth still accumulates before close(). This PR is a superset — it includes check_same_thread=False (same line) and adds lifetime reaping.

Test plan

4 new regression tests in tests/test_session_db_read_path_split.py:

  • test_finished_read_threads_do_not_accumulate_conns — 40 sequential worker threads, asserts retained < 12 (RED on main: 40 retained; GREEN with fix)
  • test_reaped_connection_is_actually_closed — dead-thread connection closed on reap, execute raises ProgrammingError (RED→GREEN)
  • test_live_thread_connection_not_reaped — a live reader thread's connection survives reap sweeps (guard)
  • test_reaping_is_thread_safe_under_concurrency — 8 concurrent readers + interleaved reaps, no errors (guard)

Tests force _wal_active=True to exercise the per-thread read path on runtimes where WAL falls back to DELETE mode; the reaping contract is platform-independent.

tests/test_session_db_read_path_split.py — 7 passed, 4 skipped (WAL-runtime)
tests/hermes_state/ + test_web_server_sessiondb_eventloop.py — 42 passed, 4 skipped
ruff check — All checks passed!

Auto-published by Moonsong via Path B automated pipeline.

…sResearch#75269)

SessionDB._get_read_conn() creates one read-only WAL connection per worker
thread and registers it in the strong _read_conns set. When a short-lived
worker exits the strong set keeps its connection reachable, so the
.db/.wal/.shm descriptors stay open until SessionDB.close() runs -- which in
a long-lived gateway may be days. Descriptor usage grew with the historical
worker count rather than current reader concurrency, eventually exhausting
RLIMIT_NOFILE (EMFILE).

Fix: track the owning thread per connection (_read_conns becomes
dict[conn -> Thread]) and reap (close + drop) connections whose owner thread
has exited, lazily when a new connection registers. Read connections are now
opened with check_same_thread=False so the cross-thread close succeeds,
matching the writer and cross-profile read-only connections.

This is complementary to NousResearch#74304, which only made close() able to drain
worker connections at shutdown; it did not bound lifetime accumulation.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused connection-lifecycle fix. The premise remains present on current main: hermes_state.py:1800-1805 retains worker readers in a strong set, _get_read_conn() adds them at hermes_state.py:2051, and they are otherwise drained only by close() at hermes_state.py:2529-2537.

The PR's owner-aware reap is serialized by the existing _read_conns_lock, and its close() calls also release tracked-connection registry state through hermes_cli/sqlite_safe_read.py:154-160. The added regressions cover retention, actual close behavior, live-owner preservation, and concurrent registration/reaping. GitHub reports the branch as one commit ahead and zero commits behind main.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 31, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 31, 2026
@sexycoke

sexycoke commented Aug 7, 2026

Copy link
Copy Markdown

Independent macOS production confirmation on v0.20.0 / f88f6f8e6. I also fetched current origin/main at 623d5c93e0; none of the relevant SessionDB lifecycle files have changed there.

Environment:

  • macOS, Apple Silicon
  • Python 3.11.15
  • launchd soft RLIMIT_NOFILE=256
  • long-lived hermes serve process

Live descriptor growth after a clean restart:

Uptime Total FDs state.db state.db-wal state.db-shm
5.5 min 79 17 15 1
20.6 min 109 32 29 1
144.6 min 148 48 45 1

A separate temp-DB reproduction on the same checkout produced:

baseline FDs:                  6
after 40 short-lived readers: 86
retained _read_conns:         40
after SessionDB.close():      85

This matches the approximately two-descriptor-per-finished-reader signature. The live process continued accumulating after an application update and restart; the restart only reset the descriptor count temporarily.

The owner-aware runtime reaper plus check_same_thread=False in this PR directly matches the observed failure mode. A rebase onto current main and merge would address a reproducible process-wide EMFILE failure on macOS. I can retest the updated branch against the same synthetic and long-running-process measurements if useful.

@teknium1

Copy link
Copy Markdown
Contributor

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.

@teknium1 teknium1 closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: SessionDB retains WAL readers from finished worker threads until shutdown, exhausting RLIMIT_NOFILE

4 participants