Skip to content

fix: reap stale TUI slash workers - #41473

Closed
Fewmanism wants to merge 1 commit into
NousResearch:mainfrom
Fewmanism:fix/tui-stale-slash-worker-cleanup
Closed

fix: reap stale TUI slash workers#41473
Fewmanism wants to merge 1 commit into
NousResearch:mainfrom
Fewmanism:fix/tui-stale-slash-worker-cleanup

Conversation

@Fewmanism

Copy link
Copy Markdown
Contributor

Summary

  • keep a registry of live TUI slash workers keyed by session id so a newly-created continuation/reconnect worker supersedes any stale duplicate for the same session key
  • make _SlashWorker.close() idempotent, log cleanup by session key/PID, close pipes, and wait() after kill() so children are reaped instead of becoming zombies
  • add regressions for duplicate same-session workers and kill-after-timeout reaping

Context

Observed in a TUI/dashboard session after compression handoff: the parent session ended with end_reason=compression, the continuation session stopped responding, and two idle tui_gateway.slash_worker --session-key <continuation> processes remained. Manual cleanup left them as <defunct> under the dashboard parent, which pointed at missing process reaping in the worker lifecycle.

Verification

  • uv run --extra dev python -m pytest tests/test_tui_gateway_server.py -q -k 'slash_worker or ws_orphan_reap' -o 'addopts='5 passed, 217 deselected
  • uv run --extra dev python -m py_compile tui_gateway/server.py tests/test_tui_gateway_server.py

Note

A full tests/test_tui_gateway_server.py run hit an unrelated browser-manage expectation in this local environment (test_browser_manage_connect_default_local_reports_launch_hint expected a no-browser-executable message). The focused TUI slash-worker/orphan regressions pass.

@Fewmanism
Fewmanism force-pushed the fix/tui-stale-slash-worker-cleanup branch from c32f247 to 5af663b Compare June 7, 2026 19:25
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive verification: clean zombie-prevention fix for TUI slash workers.

The implementation correctly addresses a real process-leak problem:

  • Registry pattern is sound_slash_workers_by_key dict with _slash_worker_registry_lock ensures at most one live worker per session key. The supersede path in __init__ cleanly closes the stale worker before the new one takes over.

  • Zombie reaping is correctclose() does terminate → wait(1s) → kill → wait(1s), which is the proper POSIX sequence. The finally block closes all three streams (stdin/stdout/stderr) to prevent FD leaks.

  • Thread safety — the lock is held only around dict operations (fast), and close() uses _closed as a guard against double-close. The if _slash_workers_by_key.get(self.session_key) is self identity check prevents a newer worker from being evicted by an older worker's close.

  • Tests cover both pathstest_slash_worker_replaces_existing_worker_for_same_session_key (supersede) and test_slash_worker_close_waits_after_kill_to_reap_process (zombie kill+wait).

LGTM.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused TUI worker-lifecycle investigation. This automated hermes-sweeper review found that the reported reaping guarantee is already implemented on current main through a broader session-lifecycle fix.

  • tui_gateway/server.py:369-396 now makes _SlashWorker.close() idempotent, waits after escalation to kill(), and closes all worker pipes.
  • tui_gateway/server.py:721-729 and :2783-2803 prevent a newly built/restarted worker from being orphaned when its session has already been torn down.
  • tui_gateway/server.py:765-829 reaps detached WebSocket sessions through the unified teardown path.
  • Current regression coverage includes zombie reaping and FD closure at tests/test_tui_gateway_server.py:8225-8261, plus restart/create-close race coverage at :8298-8367.
  • The current implementation was shipped by ae94ed17288a8547aee6af1b73d1d3f5f126281d (fix(tui-gateway): reap leaked slash_worker sessions on disconnect + active_list liveness) and is contained in v2026.6.19.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 14, 2026
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:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants