Skip to content

fix(tui): close agents orphaned during session build - #49756

Open
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:fix/tui-orphan-agent-build-cleanup
Open

fix(tui): close agents orphaned during session build#49756
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:fix/tui-orphan-agent-build-cleanup

Conversation

@MaxFreedomPollard

@MaxFreedomPollard MaxFreedomPollard commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The TUI gateway builds a session's AIAgent lazily on a background thread (_start_agent_build_build in tui_gateway/server.py). Agent construction is expensive — tool discovery, model metadata, MCP discovery — so it stays in flight for a while. During that window the session can be torn down: fast /new churn (or a quick reconnect) fires session.close, which pops the session out of _sessions and runs _teardown_session to close the agent + slash worker and unregister the approval notifier.

The bug is the ordering. session.close closes whatever it finds on the session dict at that instant. If it runs before _build has attached the agent, it sees agent=None and closes nothing. _build then finishes constructing the agent and unconditionally stored it on the now-dead session — and nobody ever closes it. The agent, and the network clients / resources it owns, leaks until the gateway shuts down.

The slash worker and the approval-notify registration were already protected against this race: _attach_worker closes the worker if the session was reaped, and the _build finally block unregisters a late notify callback. The freshly-built agent itself was the remaining gap.

This PR closes that gap. After _make_agent returns, _build re-checks — atomically, under _sessions_lock — whether the session is still the live one:

  • If it is, attach the agent and continue exactly as before.
  • If it isn't, the session was closed mid-build: close the just-built agent immediately and return, before spawning a slash worker, registering an approval callback, or wiring any other per-session resources for a dead session.

The check and the attach happen under the same lock session.close takes to pop the session, so there is no window between "still alive" and "attached." The later race windows (session closed between the agent attach and the worker/notify install) remain covered by the existing _attach_worker and finally-block cleanup, so the build path is protected end to end.

Related Issue

Fixes #49852 — found while reading the gateway session-lifecycle code. No other open PR covers this specific leak: the nearby open PRs (e.g. #48656, #41473) reap orphaned slash_worker subprocesses, not the in-process agent that finishes building after session.close.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tui_gateway/server.py (_start_agent_build._build): attach the agent under _sessions_lock only when the session is still live; otherwise close the agent and return early, skipping worker/notify/callback setup for the dead session. Updated the finally-block comment so it describes the now-narrower worker-leak window accurately.
  • tests/test_tui_gateway_server.py: extended the create/close race regression test (renamed test_session_create_close_race_does_not_orphan_worker..._does_not_orphan_resources) to assert the late-built agent is closed, and that no slash worker is created and no notify callback is registered for the closed session.

How to Test

pytest tests/test_tui_gateway_server.py::test_session_create_close_race_does_not_orphan_resources -q

The test drives a real session.create, blocks inside a stubbed _make_agent until the build thread is parked, fires session.close, then releases the build and asserts the agent that finishes late gets closed (and that no worker/notify is installed for the dead session). It fails on current main — the late agent is never closed, so closed_agents == [] — and passes with this change.

Because this is a threading race in the gateway, it is platform-independent — not tied to any OS, shell, or interpreter build.

Verified on macOS, Python 3.11.15 (arm64): the new assertions fail on main and pass with the fix. tests/test_tui_gateway_server.py is otherwise green (280 passed). The one remaining failure in that file, test_browser_manage_connect_default_local_reports_launch_hint, is unrelated — it expects no Chromium-family browser to be installed and fails identically on main. ruff check tui_gateway/server.py tests/test_tui_gateway_server.py is clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected tests and they pass
  • I've added tests for my changes
  • I've tested on my platform: macOS, Python 3.11.15 (arm64)

Documentation & Housekeeping

  • N/A — no config keys, architecture changes, or tool behavior changes beyond the bug fix

Max Freedom Pollard

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery labels Jun 20, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused lifecycle fix. Current main still has the stated race: _start_agent_build() assigns current["agent"] = agent after _make_agent() without a liveness check (tui_gateway/server.py:1382-1388), while session.close removes that session under _sessions_lock before teardown (tui_gateway/server.py:738-746). Since teardown only closes the agent already attached to the removed session (tui_gateway/server.py:709-712), a late-built agent can escape cleanup.

The proposed re-check-and-attach under _sessions_lock in b036591365bc matches the ownership pattern already used by _attach_worker (tui_gateway/server.py:721-729), and calling agent.close() is appropriate because AIAgent.close() is idempotent and performs resource cleanup (run_agent.py:3475-3528). The added deterministic create/close race coverage is aligned with the affected path.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026
@MaxFreedomPollard
MaxFreedomPollard force-pushed the fix/tui-orphan-agent-build-cleanup branch 4 times, most recently from 0e39adc to 9e4e9ae Compare July 31, 2026 04:00
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/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

TUI session.close can leak an AIAgent that finishes building concurrently

3 participants