fix(tui_gateway): join _SlashWorker drain threads on close - #53308
fix(tui_gateway): join _SlashWorker drain threads on close#53308pasevin wants to merge 3 commits into
Conversation
…rch#53303) _SlashWorker spawns two daemon threads (_drain_stdout, _drain_stderr) in __init__ to drain the subprocess stdout/stderr. close() terminated the subprocess and closed the pipes but never joined these threads, leaving 2 leaked daemon threads per session. The threads are blocked on readline() from proc.stdout/proc.stderr. After proc.terminate() and stream.close(), the readline hits EOF and the threads exit — but only if close() waits for them. Without the join, the threads linger (and in some edge cases where the subprocess is mid-write, they can stay blocked indefinitely). Fix: store thread references in __init__, join(timeout=2) in close() after closing the streams. Also give the threads explicit names (slash-drain-stdout, slash-drain-stderr) so they're identifiable in py-spy/thread dumps.
alt-glitch
added
type/bug
|
Recommendation: keep this as the #53303 lane, but update |
…new__ tests Existing test test_slash_worker_close_reaps_zombie_and_closes_fds builds _SlashWorker via object.__new__ without setting drain thread attributes. Use getattr with None fallback so close() doesn't raise AttributeError on workers constructed without __init__. Reported by @harjothkhara in PR NousResearch#53308 review.
|
Thanks @harjothkhara — good catch. Fixed in 226f2b4: On the contributor attribution check: I'll add the |
Fixes contributor-check CI failure on PRs NousResearch#53308 and NousResearch#53315.
|
Note on CI failures: the |
c87ef8e to
8433680
Compare
Fixes contributor-check CI failure on PRs NousResearch#53308 and NousResearch#53315.
…rch#53303, by @pasevin) _SlashWorker.close() terminated the subprocess and closed the pipes but never joined the _drain_stdout and _drain_stderr daemon threads. This left 2 leaked daemon threads per dashboard/TUI session (NousResearch#53303). Changes: - Store thread references in __init__ as _drain_thread_stdout/_drain_thread_stderr - Give threads explicit names (slash-drain-stdout, slash-drain-stderr) for py-spy visibility - Join both threads with timeout=2 in close() after closing the streams - Use getattr() in close() for compat with object.__new__ tests Original PR: NousResearch#53308 by @pasevin (pasevin@gmail.com) Squashed for cherry-pick tracking.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the slash-worker cleanup path. The underlying defect is still present on current main: tui_gateway/server.py:326-327 starts unreferenced drain threads, while tui_gateway/server.py:369-396 closes streams without joining them.
Problems
tests/tui_gateway/test_slash_worker_drain.py:63setsexit_eventbefore starting the threads, and line 78 waits for them to finish before callingclose(). The final dead-thread assertions therefore also pass on current main, which does not join either thread. This does not regress the behavior the PR is intended to protect.
Suggested changes
- Make the test observe
join(timeout=2)on both stored thread objects, or keep controlled drain threads alive untilclose()triggers their release. Preserve thegetattrcompatibility path noted by the existingobject.__new__regression attests/test_tui_gateway_server.py:8252-8261.
Automated hermes-sweeper review.
|
|
||
| # Use threads that exit quickly (simulating EOF on the pipe) | ||
| exit_event = threading.Event() | ||
| exit_event.set() # let them exit immediately |
There was a problem hiding this comment.
Because this event is set before either thread starts, both drain threads have already exited before close() at line 81. The test therefore passes on current main even though current close() never calls join(). Please assert join(timeout=2) directly or keep a controlled thread alive until close().
teknium1
added
sweeper:risk-compatibility
4 participants
Summary
Fixes #53303
_SlashWorker.close()terminated the subprocess and closed the pipes but never joined the_drain_stdoutand_drain_stderrdaemon threads. This left 2 leaked threads per dashboard/TUI session.Root cause
_SlashWorker.__init__(line 260-261) spawns two anonymous daemon threads that drain the subprocess stdout/stderr viafor line in proc.stdout.close()(line 303) callsproc.terminate()and closesstdin/stdout/stderr, but the drain threads — blocked onreadline()— are never joined.In production, closing
proc.stdoutusually causes thereadline()to hit EOF and the thread exits. Butclose()returns before that happens, so the threads are unaccounted for. In edge cases where the subprocess is mid-write, the thread can stay blocked longer.Evidence
Live measurement (dashboard with memory provider disabled, isolating this leak from #46082):
2 threads leak per session, 0 subprocesses leak.
Changes
__init__asself._drain_thread_stdout/self._drain_thread_stderrslash-drain-stdout,slash-drain-stderr) for py-spy/thread dump visibilitytimeout=2inclose()after closing the streamsTesting
Type of Change
Related Issues