fix(tui_gateway): close slash_worker on WS detach to prevent memory leak

[yingliang-zhang]

Summary

The Desktop app uses one WebSocket for all sessions. When the user switches sessions or the WS reconnects (e.g. after macOS sleep), old sessions are detached to _detached_ws_transport but their slash_worker subprocesses (~13 MB each) stay alive until the 6 h TTL reaper or 20 s orphan reaper fires — which may never happen if a background curator review keeps the session flagged as running.

Over a few hours this accumulates dozens of idle workers, contributing to GIL pressure and memory bloat. In production this was observed as 17 zombie slash_workers (~230 MB) causing event-loop stalls up to 10.6 s.

Changes

1. Close slash_worker on WS detach (core fix)

tui_gateway/server.py — _close_sessions_for_transport():

When a session is detached (WS disconnect without close_on_disconnect), its slash_worker is now closed immediately instead of lingering. The worker is recreated lazily on the next slash command — both the slash.exec handler and _restart_slash_worker already handle worker=None.

2. Add INFO logging to _close_session_by_id (observability)

Previously the entire session-close path was completely silent — no log at any level. This made it impossible to diagnose why sessions were or weren't being reaped. A single INFO line makes the teardown path visible:

INFO tui_gateway.server: session closed sid=abc123 end_reason=ws_orphan_reap

Why this is safe

• _SlashWorker.close() is already idempotent (_closed guard + poll() guard)
• _finalize_session (the session-end chokepoint, PR fix(tui): close slash_worker inside _finalize_session (defense-in-depth, #38095) #42149) already closes the worker — our fix just closes it sooner, at detach time
• The slash.exec handler creates a new worker on demand when session["slash_worker"] is None (server.py ~L12553)
• _restart_slash_worker does the same after each turn

Testing

tests/tui_gateway/test_slash_worker_detach.py::test_close_sessions_for_transport_closes_worker_on_detach PASSED
tests/tui_gateway/test_slash_worker_detach.py::test_close_sessions_for_transport_preserves_worker_on_reap PASSED
tests/tui_gateway/test_slash_worker_detach.py::test_close_sessions_for_transport_handles_worker_close_exception PASSED
tests/tui_gateway/test_slash_worker_detach.py::test_close_session_by_id_logs_end_reason PASSED

All 24 existing slash_worker / ws_orphan / close_session tests still pass.

Related work (complementary, non-overlapping)

This PR addresses a different leak path from three open PRs. They are complementary and can all merge independently:

PR	Author	Fixes	Leak path
#53308	@pasevin	Drain thread join on close	2 leaked threads per session
#41473	@Fewmanism	Worker registry + idempotent close	Same-session-key duplicate workers
#48656	@Nigmat-future	Startup orphan reaper + zombie parent detection	Workers surviving gateway crash
this PR	—	Close worker on WS detach	Workers surviving session switch

Related issues

• tui_gateway.slash_worker processes are not reaped when their parent session is interrupted mid-compaction #48643 — slash_worker not reaped when session interrupted mid-compaction
• [Bug]: TUI slash worker subprocesses leak after session close, accumulating zombie processes #38095 — TUI slash worker zombie subprocesses leak after session close (CLOSED, fix(tui): close slash_worker inside _finalize_session (defense-in-depth, #38095) #42149 merged)
• [Bug]: _SlashWorker.close() does not join drain threads — 2 leaked threads per dashboard session #53303 — _SlashWorker.close() does not join drain threads
• RFC: Dashboard/TUI reliability hardening — prevent event-loop starvation and stale session workers #48564 — RFC: Dashboard/TUI reliability hardening (umbrella)

[yingliang-zhang]

CI slice 7/8 failed on tests/gateway/test_allowed_channels_widening.py — this is a collection timeout (0 tests ran), not a test assertion failure. This file is unrelated to this PR (which only touches tui_gateway/server.py).

Local verification — all pass:

• tests/gateway/test_allowed_channels_widening.py — 27 passed (1.36s)
• tests/tui_gateway/test_slash_worker_drain.py — 3 passed
• tests/tui_gateway/test_slash_worker_detach.py — 4 passed

This looks like a flaky CI timeout. Could a maintainer re-run the failed job?

[Kinkoolino-Hermes]

This matches a real deployment pain point: detached Desktop/TUI WebSocket sessions can leave slash workers around long after the visible chat has moved on or recovered.

The useful boundary here is narrower than the older all-in-one session-leak PRs: close/reap the worker tied to the detached WS transport without changing unrelated CLI session finalization behavior. That narrower scope makes this easier to review as a targeted stale-worker fix.

[yingliang-zhang]

Thanks @Kinkoolino-Hermes for the validation. Agreed on the narrower boundary — closing/reaping the worker tied to the detached WS transport without touching unrelated CLI session finalization is the right scope. The comprehensive lifecycle fix (WS detach close + drain thread join + getattr compat for object.__new__ constructed workers) is all in this PR.

[yingliang-zhang]

Rebased this PR onto current origin/main (964ecef40) and force-pushed the fork branch with lease protection. New head: 1bddff030.

Conflict resolution preserved the three slash-worker lifecycle commits plus the contributor AUTHOR_MAP entry; no unrelated changes were added.

Fresh verification on the rebased head:

• scripts/run_tests.sh tests/tui_gateway/test_slash_worker_detach.py tests/tui_gateway/test_slash_worker_drain.py → 7 passed
• Three existing close-path regressions (zombie_and_closes_fds, close_session_by_id_is_idempotent_and_full, attach_worker_closes_orphan_when_session_already_torn_down) → 3 passed
• ruff check on all four changed files → passed
• py_compile on all four changed files → passed

I also re-audited all worker close/set-None paths: detach closes and nulls the worker, later finalization therefore skips it, and close() remains idempotent.

[teknium1]

Thanks for isolating the detached-session lifecycle path. The premise still holds on current main: tui_gateway/server.py:823-826 detaches non-flagged sessions without closing their worker, while tui_gateway/server.py:760-762 excludes running sessions from orphan reaping. The lazy recreation paths already exist at tui_gateway/server.py:2813-2833 and :13314-13322.

Problems

• tests/tui_gateway/test_slash_worker_drain.py:130 uses inspect.getsource; root AGENTS.md:1358-1379 explicitly bans source-shape tests.
• tests/tui_gateway/test_slash_worker_drain.py:68 releases the test threads before close() is invoked, so the assertion cannot prove the new join(timeout=2) loop is executed.

Suggested changes

• Replace the source inspection with a behavioral constructor test, or omit the thread-name assertion.
• Use fake thread objects with spyable join() methods and assert both receive timeout=2 during _SlashWorker.close().

This is an automated hermes-sweeper review.

[Kinkoolino-Hermes]

Thanks for replacing the source-shape assertion with a behavioral constructor test. One review point still appears open on current head 157c75b001959da83f2f4f52eaa1e4b16b248dcc: test_slash_worker_close_joins_drain_threads starts threads whose event is already set, sleeps before close(), and then only checks is_alive(). It can therefore pass even if the join(timeout=2) loop is removed.

Could you use fake thread spies there and assert that both stored drain-thread references receive join(timeout=2) during close()? That would directly cover the cleanup behavior the test is intended to protect.

[yingliang-zhang]

Addressed in commit be38ba56101eb2623d33f8c91d53f13344e04041: the drain-cleanup regression now uses spyable stored thread doubles and directly asserts both receive join(timeout=2). Focused test: tests/tui_gateway/test_slash_worker_drain.py (3 passed).

[Kinkoolino-Hermes]

Thanks — the spy-based regression described in be38ba56101eb2623d33f8c91d53f13344e04041 directly addresses the remaining review point by checking that both stored drain-thread references receive join(timeout=2). The current head’s required checks are green, so the test gap raised in this review thread is resolved.