Skip to content

fix(dashboard): race PTY reader against WS receive to prevent FD leak (#54028) - #54047

Closed
liuhao1024 wants to merge 4 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-54028-pty-fd-leak
Closed

fix(dashboard): race PTY reader against WS receive to prevent FD leak (#54028)#54047
liuhao1024 wants to merge 4 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-54028-pty-fd-leak

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a PTY file descriptor leak in the dashboard /api/pty WebSocket handler. When the PTY reader task exits (PTY EOF or WebSocket send failure), the writer loop previously blocked forever on ws.receive(), preventing the finally block from running and leaking the bridge's file descriptors. This caused the gateway to hit OSError: [Errno 24] Too many open files after 8-9 hours of uptime.

The fix races ws.receive() against reader_task using asyncio.wait(FIRST_COMPLETED) so the writer loop exits promptly when the reader dies, allowing bridge.close() cleanup to proceed.

Related Issue

Fixes #54028

Type of Change

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

Changes Made

  • hermes_cli/web_server.py: Race ws.receive() against reader_task in the PTY writer loop. When the reader exits (PTY EOF or WS send error), cancel the pending receive future and break the loop so the finally block can call bridge.close(). (+22/-1 lines)
  • tests/hermes_cli/test_pty_ws_bridge_close_on_reader_exit.py: Regression test verifying bridge.close() is called when the PTY reader exits with EOF, even when ws.receive() would block indefinitely.

How to Test

  1. Run the regression test: python -m pytest tests/hermes_cli/test_pty_ws_bridge_close_on_reader_exit.py -xvs — should pass
  2. Run existing PTY import tests: python -m pytest tests/hermes_cli/test_web_server_pty_import.py -xvs — should pass
  3. Run related web server tests: python -m pytest tests/hermes_cli/test_web_server_skills_profiles.py tests/hermes_cli/test_web_server_fs.py tests/hermes_cli/test_dashboard_register.py -x — all should pass
  4. Manual verification: open the dashboard /chat tab, connect, then disconnect — the bridge FD should be released promptly. Observed result: lsof -p PID | grep pty shows the FD count stays stable over time instead of growing monotonically.

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
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (PTY is POSIX-only)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…NousResearch#54028)

When the PTY reader task exits (PTY EOF or WS send failure), the writer
loop previously blocked forever on ws.receive(). The finally block
(which calls bridge.close()) never ran, leaking PTY file descriptors.

Race ws.receive() against reader_task via asyncio.wait(FIRST_COMPLETED)
so the loop exits promptly when the reader dies, allowing cleanup.

22 lines changed in web_server.py + regression test.
@alt-glitch alt-glitch added type/bug Something isn't working comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jun 28, 2026

@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 a PTY bridge FD leak when the reader task exits. When pump_pty_to_ws exits (PTY EOF or WS send failure), the writer loop blocked forever on ws.receive() because nothing unblocked it. The finally block (which calls bridge.close()) never ran. The fix races ws.receive() against reader_task via asyncio.wait(FIRST_COMPLETED) so the loop exits when the reader dies. Test verifies bridge.close() is called even when ws.receive() would block forever.


Reviewed by Hermes Agent

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Upstream commit 6d879d486 (merged via #54123) implements the same fix for the PTY WebSocket FD leak (#54028) — closing the socket in the reader's finally block to unblock ws.receive().

The upstream fix is more comprehensive (42-line diff in web_server.py + reconnect test coverage). Rebase would produce a near-empty diff. Recommending closure in favor of the upstream fix.

Superseded by: 6d879d486 (fix(dashboard): close PTY WebSocket on child EOF to stop FD leak)

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix here. This has been implemented on current main by the upstream fix in #54123, so this PR is now superseded.

Evidence from this automated hermes-sweeper review:

  • Commit 6d879d486b19716f8b09bd47bffb0b6e5b690431 is an ancestor of current main (compare/6d879d486...main reports status: ahead with that commit as the merge base).
  • hermes_cli/web_server.py:12205 now closes the PTY bridge and WebSocket from the reader task's finally path when the child EOFs or the send side breaks, which unblocks the writer loop's ws.receive() path.
  • hermes_cli/web_server.py:12233 guards ws.receive() against the RuntimeError raised after the socket is closed.
  • tests/hermes_cli/test_web_server_pty_reconnect.py:134 includes regression coverage that child EOF closes the socket and reaps the bridge.
  • The linked issue [Bug]: FD leak regression after PTY reconnect PRs (#52962 #53227) #54028 is already closed with a maintainer note pointing to fix(dashboard): close PTY WebSocket on child EOF to stop FD leak (#54028) #54123 / 6d879d48, and the PR author also noted this PR is superseded by that upstream commit.

Closing as implemented on main.

@teknium1 teknium1 closed this Jun 28, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P1 High — major feature broken, no workaround sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: FD leak regression after PTY reconnect PRs (#52962 #53227)

4 participants