Skip to content

fix: close leaked SessionDB connections on exception paths (#83226) - #83237

Closed
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/sessiondb-fd-leak-slash-repair
Closed

JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/sessiondb-fd-leak-slash-repair

Conversation

@JonthanaHanh

Copy link
Copy Markdown

Summary

Fixes leaked SessionDB SQLite file descriptors on two exception paths that accumulate until the process hits EMFILE (Too many open files).

Changes

gateway/slash_commands.py — /insights command: db.close() was on the success path but not wrapped in try/finally. If InsightsEngine.generate() or format_gateway() raises, the connection leaks.

hermes_cli/sessions_cmd.py — sessions repair: SessionDB() was created inline with no .close() at all, leaking the FD on every invocation.

hermes_state.py — Added __del__ safety net to SessionDB so instances orphaned by callers who forget .close() are cleaned up when garbage collected, rather than pinning FDs alive until process exit via the atexit hook.

Notes

The 4 other leak sites mentioned in the issue (tui_gateway/server.py, tui_gateway/compute_host.py, tui_gateway/methods_session.py ×2) already have proper finally blocks with _transfer_db_to_agent / owns_db guards on current main.

Fixes #83226

…rch#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.

Additionally, add a __del__ safety net to SessionDB itself so that
instances orphaned by callers who forget .close() are cleaned up when
garbage collected, rather than pinning FDs alive until process exit
via the atexit hook.

Fixes NousResearch#83226
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state area/sessions Session lifecycle, resume, persistence, history labels Aug 10, 2026
@egilewski

Copy link
Copy Markdown

suggesting changes

SessionDB.__del__() does not close the descriptor-leak class it is documented to cover. queue_token_counts() registers self._drain_token_queue_at_exit; that bound method strongly retains the SessionDB instance. On the current-main-integrated PR tree, after starting token accounting, dropping the last caller reference, and forcing collection, the instance remained live and its state.db, state.db-wal, and state.db-shm handles remained open. An explicit close() then released the instance and all three handles.

The two explicit call-site changes do close correctly on their exception paths, but the new fallback cannot recover any other forgotten close after token accounting starts. Please either remove the ineffective finalizer and its claim, or break the atexit strong-retention cycle/establish equivalent deterministic ownership, with a regression test that starts token accounting, drops the caller reference, collects, and verifies that both the instance and its descriptors are gone.

Security evidence:

  • trust boundary: long-lived gateway/CLI ownership of SQLite connections versus the process file-descriptor ceiling.
  • source/sink/invariant: queue_token_counts() registers a bound atexit callback that retains self; every orphaned SessionDB connection must nevertheless be released before process exit.
  • current-main reproduction: both targeted exception-path probes observed zero close() calls, and a token-used orphan remained live after collection with all three SQLite handles open.
  • PR-head or patch-replay validation: the current-main-integrated patch changed both targeted exception paths to one close() call, but the same token-used orphan still remained live with all three handles open.
  • positive/negative cases: explicit close() made the weak reference dead and removed every tracked SQLite handle; collection alone did neither after token accounting started.
  • residual bypass search: any unbalanced owner after the first queued token remains pinned by the atexit registry, and this PR adds no regression test for that claimed fallback.
  • reviewer validation: 20 focused SessionDB, insights, and CLI tests passed and the changed modules compiled; the standalone descriptor probe still refuted the fallback invariant.

Not checked:

  • Full test suite
  • CodeRabbit review

Signed: GPT-5.6-sol-xhigh in Codex

teknium1 pushed a commit that referenced this pull request Aug 15, 2026
…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.
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #86667 — your commit (the /insights and sessions repair try/finally fixes) was cherry-picked onto current main with your authorship preserved in git log. You were the earliest submitter for #83226, so your call-site fixes lead the consolidated salvage.

One part was not carried: the SessionDB.__del__ safety-net. Review here showed queue_token_counts() registers a bound atexit callback that strongly retains the instance, so the finalizer cannot fire for the orphaned-instance class it targeted. The consolidated PR covers that class deterministically instead, via the constructor finally ownership repair from #83620. Thanks for the fix!

@teknium1 teknium1 closed this Aug 15, 2026
@kshitijk4poor

Copy link
Copy Markdown

Merged via #86691 — your commits cherry-picked with authorship preserved via rebase-merge.

Your fix for the SessionDB FD leak is now on main. The two try/finally wrappers in /insights and sessions repair correctly close the connection on exception paths. The __del__ safety net was improved on top: it now delegates to self.close() for full cleanup (read pool, token writer, atexit unregister) instead of only closing the writer connection.

Thanks for the clear issue report (#83226) and the well-targeted fix.

bobaba76 pushed a commit to bobaba76/hermes-agent that referenced this pull request Aug 27, 2026
…air exception paths (NousResearch#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 (NousResearch#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 NousResearch#83620.
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…air exception paths (NousResearch#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 (NousResearch#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 NousResearch#83620.
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/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround 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.

SQLite file descriptor leak — SessionDB connections not closed on exception paths (EMFILE crash)

6 participants