Skip to content

fix(tui): close slash_worker sessions on WebSocket disconnect - #31474

Closed
sea-monsters wants to merge 1 commit into
NousResearch:mainfrom
sea-monsters:fix/tui-ws-disconnect-cleanup-sessions
Closed

fix(tui): close slash_worker sessions on WebSocket disconnect#31474
sea-monsters wants to merge 1 commit into
NousResearch:mainfrom
sea-monsters:fix/tui-ws-disconnect-cleanup-sessions

Conversation

@sea-monsters

Copy link
Copy Markdown

What does this PR do?

When a TUI WebSocket disconnects (browser tab closed, network drop, OS kills the tab), the session's slash_worker subprocess stays alive indefinitely -- it's a persistent for raw in sys.stdin: loop that never exits unless stdin is explicitly closed. These orphaned workers accumulate over time, wasting ~100MB RAM each.

This PR fixes the lifecycle disconnect in two layers:

P0: Clean up sessions on WebSocket disconnect (ws.py)

handle_ws() now finalizes the affected sessions and closes their slash_worker / agent when the WebSocket transport disconnects, instead of only detaching the transport.

P1: Worker self-protection (slash_worker.py)

Even if the P0 fix misses a code path, the worker now exits on its own:

  • Idle timeout: exits after 30 minutes without receiving a command
  • Parent-PID check: exits if the parent (server) process disappears

Related Issue

No open issue. Found during a routine environment audit: 5 orphaned slash_worker processes had accumulated over 6 days, consuming ~350MB RAM.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tui_gateway/ws.py: in handle_ws's finally block, call _finalize_session, close the slash_worker and agent, and remove the session from _sessions for every session that owned the closing transport
  • tui_gateway/slash_worker.py: replace blocking for raw in sys.stdin with select.select-based polling that checks idle timeout and parent-PID continuity every 60s

How to Test

  1. Start Hermes Dashboard with --tui: hermes dashboard --port 8089 --tui
  2. Open the dashboard in a browser and start a chat session
  3. Verify ps aux | grep slash_worker shows a worker process
  4. Close the browser tab
  5. Verify the worker process exits within ~60 seconds (poll interval)

Alternatively, test the worker self-protection directly (run in Python):

import subprocess, time, os
proc = subprocess.Popen(
    ["python", "-m", "tui_gateway.slash_worker", "--session-key", "test_123"],
    stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE
)
time.sleep(3)
os.kill(os.getppid(), 9)  # simulate parent death
# worker should exit within 60s
proc.wait(timeout=90)
print("Worker exited on parent death:", proc.returncode)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (235 existing tests pass)
  • I've tested on my platform: Ubuntu 24.04

Screenshots / Logs

Before: 5 orphaned slash_worker processes consuming ~350MB RAM

PID    PPID  %MEM  RSS    ELAPSED   SESSION_KEY
581575 363605 0.7  27552  6-05:35   20260518_142738
640442 363605 1.9  74284  6-01:48   20260518_181453
925207 363605 1.3  50044  5-05:24   20260519_143847
1877100 363605 2.7  103552 2-10:56  20260522_090612
1972117 363605 2.5  96568  2-04:06  20260522_155616

@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/) labels May 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to the slash_worker leak family: #21370, #21467, #22855, #24775. Main fix PR is #22863 which also addresses lifecycle management.

This PR adds both server-side cleanup (ws.py finalize on disconnect) and worker self-protection (idle timeout + parent-PID check). Worth comparing scope with #22863.

Three-layer defence-in-depth against orphaned slash_worker subprocesses:

1. **Server-side cleanup (P0)** — when a WebSocket disconnects, sessions
   marked close_on_disconnect=true (sidecar/short-lived) are finalised
   and their worker is killed. Normal TUI sessions still fall back to the
   stdio transport for historical reconnect compatibility.
   (Design from NousResearch#21401, adapted with permission.)

2. **Parent watchdog (P1, psutil)** — a daemon thread monitors the parent's
   PID + create_time fingerprint every 10 s and exits if the parent
   disappears. Handles crashes, SIGKILL, and PID reuse (critical on
   Windows). (Design from NousResearch#22863, adapted with permission.)

3. **Idle timeout + getppid() poll (P1, no deps)** — the main stdin loop
   uses select.select() with a 60 s timeout so it can periodically check
   os.getppid() and a 30-minute idle deadline. Works without psutil,
   adding defence even when the watchdog thread is not available.

Also refactors session teardown into a shared _close_session_by_id()
helper so that session.close RPC, WebSocket-disconnect cleanup, and
server shutdown all use the identical code path.

Co-authored-by: Hermes Agent
@sea-monsters
sea-monsters force-pushed the fix/tui-ws-disconnect-cleanup-sessions branch from 97df2ab to 2f668de Compare May 24, 2026 14:13
@sea-monsters

Copy link
Copy Markdown
Author

Thanks @alt-glitch for the references — I've reviewed both #22863 and #21401 in detail and restructured this PR to incorporate the best of all three approaches:

Updated scope (force-pushed):

P0 — Server-side cleanup (adopted #21401's design)

  • Added _close_session_by_id() shared helper used by session.close RPC, WebSocket-disconnect cleanup, and server shutdown
  • Added _close_sessions_for_transport() that only closes sessions with close_on_disconnect=True and falls back to stdio transport for normal sessions — much more precise than my original approach of closing all sessions on WS disconnect
  • Added close_on_disconnect flag to _init_session() (default False) and session.create (configurable)

P1 — Worker self-protection (merged #22863's watchdog + our idle timeout)

The key differentiator from #22863 is:

  1. Idle timeout bounds worst-case resource usage even if both server-side cleanup and the watchdog miss a code path
  2. No psutil dependency required for basic protection (getppid + idle timeout via select)
  3. _close_session_by_id() refactor ensures all teardown paths are identical

Would love to hear your thoughts on whether this combined approach addresses the concerns from the related issues better than each PR individually. Happy to collaborate on merging this or splitting it into follow-ups to #21401 / #22863.

@teknium1

teknium1 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Superseded by #42132 (merged), which closes the slash_worker subprocess leak via two guards: process-group kill on PTY teardown + a cross-platform parent-death watchdog in the worker. Closing as resolved — thanks for tackling this; the merged fix salvaged the process-group-kill and watchdog approaches with contributor authorship preserved.

@teknium1 teknium1 closed this Jun 8, 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants