Skip to content

fix(tui): settle session close against active turns - #86933

Merged
teknium1 merged 4 commits into
NousResearch:mainfrom
100yenadmin:upstream/tui-close-settlement-20260815
Aug 16, 2026
Merged

fix(tui): settle session close against active turns#86933
teknium1 merged 4 commits into
NousResearch:mainfrom
100yenadmin:upstream/tui-close-settlement-20260815

Conversation

@100yenadmin

@100yenadmin 100yenadmin commented Aug 15, 2026

Copy link
Copy Markdown

Summary

  • publish a session's closing state atomically with its registry removal
  • reject a prompt worker when close wins the final publication race
  • give an already-published turn thread a bounded chance to settle before
    ordinary teardown closes its agent and related resources
  • preserve a queued prompt without dispatching it when a compute-host
    completion arrives after close has claimed the session
  • preserve the process-shutdown deadline by skipping the turn join for
    tui_shutdown

Closes #86932.

Why

On main, session.close can pop and tear down a session while
_run_prompt_submit() is between message.start and worker publication.
Because the pop did not mark the session as closing, the worker could still
start against resources that teardown had already closed. Close also finalized
the session without waiting for the recorded _run_thread, even when the turn
was only unwinding its post-terminal tail.

Both ownership decisions now use _sessions_lock: close publishes _closing
before detaching, and prompt dispatch rechecks that state while publishing and
starting its worker. Once a close has claimed the record, ordinary teardown
waits up to five seconds for that exact worker, outside the registry and resume
locks.

Tests

Both new behavior tests were verified red on upstream
bb4f680f22b8d6ac66cecd0dec310c5a68f6b556:

2 failed, 573 deselected

They pass at this PR head:

2 passed, 573 deselected

Related lifecycle and dispatch selection:

18 passed, 557 deselected

Full changed test file:

575 passed

After the review delta, both changed test files pass together:

606 passed

Commands:

scripts/run_tests.sh tests/test_tui_gateway_server.py \
  -k 'session_close_settles_active_turn_before_teardown or run_prompt_submit_rejects_worker_when_close_wins_publication'

scripts/run_tests.sh tests/test_tui_gateway_server.py \
  -k 'session_close or close_session or run_prompt_submit'

scripts/run_tests.sh tests/test_tui_gateway_server.py

scripts/run_tests.sh \
  tests/test_tui_gateway_queue_on_busy.py \
  tests/test_tui_gateway_server.py

Additional checks:

ruff check tui_gateway/server.py tests/test_tui_gateway_server.py
All checks passed!

git diff --check
clean

Related work

Scope

This is source-and-test proof at commit
f0bcb3de7cab72f63e925da31e78f5d3b376f0b5. It does not claim merge, release,
deployment, or runtime/customer proof. No configuration, dependency, protocol,
or persisted-data format changes are included.

@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/) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 15, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(tui): settle session close against active turns

  1. _pop_session_by_id (server.py ~lines 964–976) now unconditionally sets session["_closing"] = True on every pop, not just on close. Today all three callers (close, close-with-predicate, WS orphan reaper) are terminal teardown paths, so it is safe — but the flag becomes a permanent property of the dict, so any future non-terminal reuse of _pop_session_by_id (e.g. detach-and-reattach for resume) would permanently block that session from starting turns. Worth documenting the invariant (or making the pop teardown-only).

  2. The reject path in _run_prompt_submit (~lines 10308–10311) returns False after message.start may already have been emitted, without emitting a terminal event (message.error/complete). In the close race the UI is closing anyway, so it is mostly moot — but any other caller observing _closing mid-dispatch would leave the client's running state unresolved.

  3. _teardown_popped_session (~lines 983–998) joins the turn thread for up to _TURN_SETTLE_BEFORE_CLOSE_SECONDS (5s) — and _close_sessions_for_transport iterates sessions, so N mid-turn sessions on transport disconnect block the disconnect handler for up to N×5s sequentially. After the timeout, teardown proceeds while the turn thread is still alive and can still emit on a closing transport; the warning acknowledges it, but a comment on expected turn-thread behavior once _closing is observed would help.

  4. Minor: session.get("_run_thread") is read without _sessions_lock in the teardown path while the write happens under it — benign (miss → skip join) but inconsistent with the locked write.

@teknium1
teknium1 merged commit 089b437 into NousResearch:main Aug 16, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists 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 race active turn dispatch and teardown

4 participants