Skip to content

fix(tui): join slash worker drain threads - #53499

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/53303-slash-worker-join
Open

fix(tui): join slash worker drain threads#53499
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/53303-slash-worker-join

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • keep explicit references to slash worker stdout/stderr drain threads
  • close worker stdio pipes and join both drain threads during worker shutdown
  • add a focused regression test for _SlashWorker.close() plus the existing session create/close race guard

Testing

  • uv run python --version
  • git diff --check
  • uv run --extra dev python -m pytest tests/test_tui_gateway_server.py -k "slash_worker_close_joins_drain_threads_and_closes_pipes or session_create_close_race_does_not_orphan_worker"
  • uv run --extra dev ruff check tui_gateway/server.py tests/test_tui_gateway_server.py

Notes

  • tests/test_tui_gateway_server.py::test_browser_manage_connect_default_local_reports_launch_hint is already failing on an untouched browser-manage path and was treated as unrelated baseline noise for this narrow change.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #53308 — same fix (join _SlashWorker stdout/stderr drain threads on close) for the same issue #53303. #53308 is the earlier open PR (canonical). Linking for the maintainer to pick one.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Fixes resource leak in _SlashWorker.close() by joining drain threads and closing stdin/stdout/stderr. Clean fix with a dedicated test that verifies thread joining and stream closing.


Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting the remaining slash-worker cleanup gap. The premise is still valid on current main: tui_gateway/server.py:326-327 starts anonymous drain threads and close() ends after stream cleanup at tui_gateway/server.py:391-396, without joining them.

Problems

  • The added join loop unconditionally reads self._stdout_thread and self._stderr_thread. Current tests/test_tui_gateway_server.py:8252-8255 constructs _SlashWorker via object.__new__ with only proc, so this change raises AttributeError. The canonical duplicate #53308 recorded and fixed this exact compatibility case in da52fdffb80923bf85efaf94d483584eb427322c using getattr(..., None) guards.

Suggested changes

  • Use guarded thread lookup and join only present thread objects, then preserve both the existing zombie/FD test and the new drain-join test.

Automated hermes-sweeper review.

Comment thread tui_gateway/server.py
stream.close()
except Exception:
pass
for thread in (self._stdout_thread, self._stderr_thread):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unconditionally reads attributes that are absent on the existing object.__new__(server._SlashWorker) fixture in tests/test_tui_gateway_server.py:8252. Use getattr(self, "_stdout_thread", None) / getattr(self, "_stderr_thread", None) and skip None values so close() remains compatible with that regression path.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 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/) duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants