fix(tui): close profile-owned SessionDB handles on teardown - #78970
fix(tui): close profile-owned SessionDB handles on teardown#78970SilentKnight87 wants to merge 1 commit into
Conversation
Every gateway session built for a non-launch profile constructs a dedicated SessionDB (that profile's state.db) and hands it to its AIAgent. AIAgent.close() deliberately leaves _session_db open (it finalizes the session row through it as its last step), and nothing else owned the handle — so each profile chat leaked its db+WAL+SHM fds (kept alive by the token-writer atexit registration) until the dashboard process hit the 256-fd launchd soft limit and started failing with EMFILE ([Errno 24] Too many open files) on auth reads, cron jobs, skills, log files, and socket.accept. Same leak class as NousResearch#69678, on the tui_gateway surface. The ownership contract this change introduces: the shared _get_db() launch handle is borrowed and never closed; a dedicated profile handle is owned by exactly one session record under _owned_session_db and closed exactly once at teardown, strictly after agent.close() (which still writes end_session through it); and a handle that was only read from is closed on every exit — return or raise — of the non-eager resume phase (TrackedConnection untracks a path only on close(), so a handle dropped for GC would permanently inflate _live_connections and disable byte-probe recovery for that state.db). - _close_session_agent_and_owned_db(): single teardown helper. Claims the agent and the owned db atomically under _sessions_lock (a concurrent claimant takes both or neither, so the db can never be closed out from under agent.close()'s final end_session write), then closes agent first, db second. - _start_agent_build: publishes agent + profile db to the live session atomically iff the sid still maps to this record; a build that lost the publish race disposes of its own locals; a _make_agent failure closes the db in the finally. The build thread deliberately does NOT also close published resources when it later finds the session detached: publish and pop are totally ordered by _sessions_lock, so the closer that popped the session always tears it down itself, and a second closer would race _finalize_session's unflushed-message persist and end_session writes. - _init_session grows owned_session_db so the eager construction sites record ownership too: session.resume (eager_build), session.branch (branch_db), and the compute-host session bootstrap. Registration refuses to displace a live record (two compute-host turns racing one sid can no longer strand the loser's resources), so a closer's pop-claim is the only way a registered record leaves the slot. A failed _init_session no longer strands a half-initialized registered session: it reclaims its record with the same atomic pop teardown uses and re-raises — unmarked when the pop proves no closer touched it (the agent/db revert to the caller's locals, which every caller then disposes of), marked _resources_claimed_by_closer when a concurrent close won the pop (that closer closes both; callers must not double-close). Callers mutate the returned record rather than re-looking up the registry, so a concurrent pop can't divert them into the local-close path; the compute-host fallback is built complete (ownership included) and published atomically only into an empty slot. - Every discarded duplicate agent — the resume double-checked-locking loser, the compute-host initializer that lost its sid, and the deferred build whose session was closed before publication, and the eager resume whose _init_session failed — is released with _end_session_on_close=False so it cannot end_session a durable row that the winning session (or the still-resumable stored conversation) owns. - session.resume's whole non-eager phase runs under try/finally, so the transient profile read handle closes on every exit — not-found, fast live reuse, lazy watch, deferred, and any mid-read exception. Regression tests: owned profile db closed exactly once at teardown (deferred build, eager resume, branch), shared launch handle never closed, build-failure and close-during/after-build races, repeated teardown idempotency, agent-before-db close ordering, init-failure unregistration with ownership return, init-failure concurrent-close claim marking (no double-close, no ghost session, no fallback resurrection), registration displacement refusal, discarded-duplicate release without end_session (resume loser and detached deferred build), transient-handle close on the deferred path and on mid-read exceptions, and a real-SessionDB churn test asserting the process fd table stays flat.
|
Full-suite re-run on the exact PR commit (a6d80bd), same environment as the controlled comparison in the description: 25,436 passed, 72 failed — the failing set is byte-identical to the pristine-base baseline (all pre-existing environment-dependent tests; missing optional extras). Zero regressions attributable to this change. |
alt-glitch
added
type/bug
|
Independent validation + production incident evidence for this fix (we hit this bug for real): Production repro (this PR fixes a real failure): a long-lived Independent A/B verification (close-count instrumentation on
Test evidence: full tui_gateway + state suites green on the patched tree (1049 passed, 0 failed). Review note — no gaps found. I specifically looked for: not-found return, fast-path reuse, eager Happy to help review the next revision or test anything specific. One suggestion: this deserves a mention in the PR description that the desktop serve backend ( |
3 participants
What does this PR do?
Fixes a file-descriptor leak in the dashboard/TUI gateway: every session scoped to a non-launch profile builds a dedicated
SessionDBfor that profile'sstate.dband hands it to itsAIAgent— but nothing ever closed it.AIAgent.close()intentionally leaves_session_dbopen (its last step isend_session()through that handle, and on ordinary sessions the handle is the process-shared_get_db()that must stay open), so the dedicated profile handles had no owner. Each one holds ~5 fds (db + WAL + SHM + read conn), pinned by the token-writeratexitregistration, so session churn (desktop chat switching + the TTL/LRU reapers) grows the fd table monotonically. A long-lived dashboard under app-global remote mode exhausts the macOS launchd 256-fd soft limit and dies in an EMFILE storm (auth reads, cron jobs, skills, log writes,socket.accept). Observed in production: 255/256 fds, ~224 SQLite-related, repeated handles on the same profilestate.dbfiles.This is the same leak class as #69678 (fixed for the delivery/delegation/verification ledgers) on a surface that fix didn't cover. It's also distinct from the WAL-reader-per-thread work in #75269/#75546/#74304 (read-pool internals) and from #78500 (notification-poller profile scoping): those bound other fd sources; this one gives the agent-held profile handle an owner.
The ownership contract: the shared
_get_db()launch handle is borrowed and never closed; a dedicated profile handle is owned by exactly one session record (under_owned_session_db) and closed exactly once at teardown, strictly afteragent.close()(which still writesend_sessionthrough it). This extends the(db, owns_handle)idiom the file already uses for scoped handles (_db_for_profile/_profile_db/_session_db) across time instead of scope.Origin: the leak arrived with 6f6eb87 (#39993, deferred builds), and the same unowned pattern was repeated by the eager resume path (02d6bf1),
session.branch(29dd621), and the compute host (7d27a31). The teardown fix in 6da970f predates profile DBs and closed only the agent.Related Issue
Related: #69678 (same fd-leak class; closed — its fix covered the ledgers but not this tui_gateway surface). No open issue tracks this specific leak.
Type of Change
Review tip:
git diff -w— thesession.resumenon-eager phase was mechanically reindented into atry/finally, which inflates the raw line count.Changes Made
tui_gateway/server.py_close_session_agent_and_owned_db(): single teardown disposal helper. Claims the agent and the owned db atomically under_sessions_lock(a concurrent claimant takes both resources or neither, so the db can never be closed out from underagent.close()'s finalend_sessionwrite), then closes agent first, db second._teardown_sessionroutes through it._start_agent_build: publishes agent + profile db to the live session atomically iff the sid still maps to this record; a build that lost the publish race disposes of its own locals with_end_session_on_close=False(a deferred resume's row belongs to the still-resumable conversation, not the discarded agent); a_make_agentfailure closes the db in thefinally. The build thread deliberately does not also close published resources when it later finds the session detached — publish and pop are totally ordered by_sessions_lock, so the closer that popped the session always tears it down itself, and a second closer would race_finalize_session's unflushed-message persist andend_sessionwrites._init_sessiongrowsowned_session_db=so eager construction sites can record ownership at registration time (same lock as registration, so a concurrent close always finds the handle it must close). Registration refuses to displace a live record, so a closer's pop-claim is the only way a registered record leaves the slot (two compute-host turns racing one sid can no longer strand the loser's resources). A failed_init_sessionno longer strands a half-initialized registered session (nobody ever receives its sid, so nothing could close it until the idle reaper hours later): it reclaims its record with the same atomic identity-checked pop teardown uses and re-raises — unmarked when the pop proves no concurrent closer touched the record (agent/db revert to the caller's locals, which every caller disposes of), marked_resources_claimed_by_closerwhen a concurrent close won the pop (that closer closes both resources; callers must not double-close). Callers mutate the record_init_sessionreturns instead of re-looking up the registry, so a concurrent pop can't divert them into the local-close path.tui_gateway/methods_session.pysession.resume(eager_build): ownership transferred via_init_session; the double-checked-locking race loser is released with_end_session_on_close=False(it must notend_sessionthe durable row the winner owns) and closes its profile db after its agent; build/init failures close the local agent + db unless the exception is marked claimed-by-closer. The stale "the agent OWNS a long-lived db handle" comment now documents the actual contract.session.resume's whole non-eager phase runs under onetry/finally, closing its transient profile read handle on every exit — not-found, fast live reuse, lazy watch, deferred, and any mid-read exception.TrackedConnectionuntracks a path only onclose(), so dropping the handle for GC frees the fds but permanently inflates_live_connectionsand disables byte-probe recovery for thatstate.db.session.branch:branch_dbownership transferred via_init_session; the failure path disposes of the built agent + db under the same claimed-by-closer guard.tui_gateway/compute_host.py_ensure_server_session: closes the profile db if_make_agentraises; transfers ownership via_init_session; the minimal fallback record is built complete (ownership included) and published atomically under_sessions_lockonly into an empty slot; a claimed failure re-raises instead of resurrecting closed handles, and losing the slot to a concurrent initializer disposes of the losing build's resources with_end_session_on_close=Falseso the winner's durable row is never finalized by the loser.tests/test_tui_gateway_server.py— 20 regression tests (see below).How to Test
scripts/run_tests.sh tests/test_tui_gateway_server.py— 536 passed (517 existing + 19 new, plus two strengthened). New coverage: owned profile db closed exactly once at teardown (deferred build, eager resume, branch); shared launch handle never closed;_make_agentfailure closes the db; close-during-build and close-after-publish races close each resource exactly once; repeated teardown is idempotent; the agent closes while its db is still open (soend_sessionstill lands); init-failure unregisters the record and returns ownership to the caller (no ghost session); init-failure with a concurrent close marks the exception and nothing double-closes or resurrects; registration refuses to displace a live record; every discarded duplicate agent (resume loser, compute-host slot loser, detached deferred build, failed eager-resume init) releases withoutend_session; the transient read handle closes on every non-eager exit including mid-read exceptions; andtest_profile_session_churn_keeps_fd_count_stable, which churns 13 sessions with realSessionDBinstances and asserts the process fd table stays flat (the incident regression — it fails by ~40+ fds without the fix). All were written red-first against the unfixed code.Note: while validating,
test_write_json_serializes_concurrent_writeswas observed to flake (~1 in 4 under heavy machine load) on the unmodified base as well — an extra event line lands in its patched stdout from a thread leaked by an earlier test in the file. Pre-existing, unrelated to this change; happy to file it separately.scripts/run_tests.sh tests/test_tui_gateway_server.py tests/test_hermes_state.py tests/test_session_db_read_path_split.py tests/hermes_state tests/state— 801 passed, 0 failed.anthropic/daytona/fal), delta exactly the new tests. Re-run on the final commit posted as a PR comment.Checklist
Code
fix(scope):,feat(scope):, etc.)scripts/run_tests.shand all tests passDocumentation & Housekeeping
docs/, docstrings) — docstrings/comments updated in the touched functionscli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A/dev/fdis unavailableScreenshots / Logs
Second-incident evidence (before fix): dashboard process at 255 open fds against the 256 launchd soft limit, ~224 SQLite-related, repeated open handles on the same profile
state.db/WAL/SHM paths, EMFILE spreading from platform probes to auth reads, cron, skills, log writes, andsocket.accept. After fix: fd count stable at 38 across session churn and sustained health checks.