fix(state): close leaked SessionDB connections on exception paths (salvage #83237 + #83620 + #72822) - #86667
Merged
Conversation
…air exception paths (#83226) Two call sites create SessionDB instances without closing them on error: 1. gateway/slash_commands.py: /insights command - db.close() was on the success path but not in a finally block, so exceptions between SessionDB() and db.close() leak the connection. 2. hermes_cli/sessions_cmd.py: sessions repair - SessionDB() created inline with no .close() at all, leaking the FD on every call. Salvage note: the original PR (#83237) also added a __del__ safety-net finalizer to SessionDB; review showed the atexit hook registered by queue_token_counts() strongly retains the instance, so the finalizer never fires for the leak class it claimed to cover. Dropped here in favor of the deterministic constructor-finally ownership repair salvaged from #83620.
…3226) SessionDB could leave native SQLite handles open when construction failed partway through schema/pragma/FTS/repair/lock/interrupt handling. Other short-lived callers (MCP reads/polling, session search, reactions, trace upload, insights, shutdown recovery) opened temporary SessionDB handles without a complete ownership boundary. API-server profile caches and RetainDB shutdown had similar late-close races. Under sustained load this exhausted file descriptors (EMFILE). - Close partially initialized SessionDB connections on every constructor exception path via a finally block guarded by an initialization-complete flag. - Close temporary/cross-profile SessionDB handles in finally blocks across CLI, MCP, search, trace, reactions, insights, and recovery paths. - Add API-server per-profile cache ownership and disconnect cleanup. - Make RetainDB writer-queue shutdown exception-safe: track connections per thread, close on worker exit, reject new enqueues after shutdown starts, and sweep any connections left by short-lived threads. - Add regression coverage for constructor failures, worker-thread readers, API disconnect failures, shutdown recovery, RetainDB late enqueue, and foreign-loop async clients. Salvage notes: the original PR's per-thread WAL-reader ownership changes were superseded by main's read-connection pool (permits + checkout/return); its cron timeout-abandon fix is credited separately to #72822's earlier identical fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…imeout-abandoned worker (#72782) run_job() submits SessionDB() to a one-worker executor and abandons the worker (shutdown(wait=False)) when init exceeds the cron timeout. If the constructor later completes inside that abandoned worker, the Future's result — an open SessionDB holding .db/WAL/SHM handles — was orphaned and never closed, leaking descriptors until EMFILE. Attach a done-callback on the timeout path that retrieves and closes any eventual late result. Salvage note: the lazy-recall ownership half of #72822 (_owns_session_db tracked on AIAgent, owned handle closed in close()) already landed on main; this carries the remaining cron timeout-abandon half with its regression test.
…d by read pool - tests/cron/test_sessiondb_init_hang.py: add threading/time imports the salvaged late-close regression tests rely on. - tests/test_hermes_state.py: drop test_close_closes_wal_read_connection_created_on_worker_thread — main replaced per-thread WAL reader ownership with the pooled read-connection design (permits + checkout/return), so cross-thread reader draining no longer exists in the form the test asserted.
alt-glitch
added
type/bug
૮ >ﻌ< ა ci reviewrunning on a7dbc2f — test: fix salvage test imports; drop WAL worker-thread test waiting for more jobs to start…
|
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidated salvage of the SessionDB file-descriptor / connection-leak bug class (#83226, #83512-adjacent, #72782). Long-running processes (dashboard, gateway, CLI) leaked SQLite fds from
SessionDBhandles orphaned on exception paths until the process hit EMFILE ([Errno 24] Too many open files— 981/1024 fds werestate.db/state.db-walin the #83226 report).Three commits, contributor authorship preserved:
try/finallyclose on the/insightsgateway command andsessions repairCLI path. The original PR's__del__safety-net was dropped: review on fix: close leaked SessionDB connections on exception paths (#83226) #83237 showed thequeue_token_counts()atexit hook strongly retains the instance, so the finalizer can never fire for the leak class it claimed to cover. Deterministic ownership instead.SessionDB.__init__closes its partially-built connection in afinallyguarded by an initialization-complete flag on every exit path; temporary/cross-profile handles closed infinallyblocks across CLI, MCP serve/poll, session search, trace upload, reactions, insights, and shutdown recovery; API-server per-profile cache ownership + disconnect cleanup; RetainDB writer-queue shutdown made exception-safe (per-thread connection tracking, close-on-worker-exit, reject-after-shutdown, final sweep). Regression tests for each path.run_job()timeout-abandon path — whenSessionDB()init exceeds the cron timeout and the worker is abandoned, a done-callback now retrieves and closes any late-completing handle instead of leaking its.db/WAL/SHM fds.Follow-up commit (ours): salvage test-import fixes and dropped the worker-thread WAL-reader test superseded by main's pooled read-connection design.
What was intentionally NOT carried
__del__finalizer — provably ineffective under the atexit retention cycle (see its review)._owns_session_dbhalf — already on main (agent/agent_init.py:1643,run_agent.py:624/4462).Verification
SessionDB, tempHERMES_HOME, counting/proc/self/fd): 5 forced constructor failures leak 11 fds on current main, 0 on this branch; normal open/close returns to baseline.test_search_projection_skips_context_enrichment_queries, which fails identically on pristineorigin/main(pre-existing, unrelated).ruffclean on all touched modules; attribution audit green; base gate 0.Fixes #83226. Fixes #72782. Closes #83512 (mechanism superseded by the read pool; residual exception-path leaks fixed here).
Infographic