fix(serve): exit desktop backend when its parent process dies - #61364
fix(serve): exit desktop backend when its parent process dies#613640xDevNinja wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a verified desktop lifecycle gap. The premise still holds on current upstream main: apps/desktop/electron/main.ts:6323-6333 sends only SIGTERM on non-Windows, and before-quit calls that path at apps/desktop/electron/main.ts:9067-9096. Current hermes_cli/main.py has no equivalent backend watchdog.
Problems
hermes_cli/main.py:11962-11965addsHERMES_DISABLE_ORPHAN_WATCHDOGandHERMES_BACKEND_WATCHDOG_POLL_S.AGENTS.md:62-64requires behavioral settings to use config.yaml/setup rather than rawHERMES_*variables. These appear to be test/debug controls rather than required user configuration.tests/hermes_cli/test_backend_orphan_watchdog.py:52-69replaces the thread with a no-op_DummyThread, so it verifies construction but never executes the target that callsos._exit(0)inhermes_cli/main.py:11980-11983.
Suggested changes
- Remove the new environment knobs, or route any genuinely user-facing setting through config.yaml. Inject or mock sleep for test speed instead.
- Execute a captured thread target with
_backend_is_orphanedforced true andos._exitreplaced by a sentinel, then assert the exit branch is reached.
Automated hermes-sweeper review.
| if os.environ.get("HERMES_DISABLE_ORPHAN_WATCHDOG") == "1": | ||
| return | ||
|
|
||
| poll_s = max(0.05, _watchdog_env_float("HERMES_BACKEND_WATCHDOG_POLL_S", 2.0)) |
There was a problem hiding this comment.
This introduces a non-secret behavioral HERMES_* setting solely for watchdog tuning, alongside the opt-out above. AGENTS.md requires behavioral configuration to use config.yaml rather than raw env vars; please remove these test/debug knobs or wire a genuine user-facing setting through the config surface.
| return _DummyThread() | ||
|
|
||
| monkeypatch.setattr(hermes_main.threading, "Thread", _fake_thread) | ||
| hermes_main._arm_desktop_orphan_watchdog() |
There was a problem hiding this comment.
This fake thread records only kwargs and its start() never invokes the target, so the test does not exercise the branch that calls os._exit(0). Capture and invoke the target with _backend_is_orphaned forced true and a mocked exit function.
461dd37 to
72b17fe
Compare
|
Both fair — fixed:
6 tests, 0.13s. |
The Electron desktop spawns each serve/dashboard backend (the primary window backend plus every per-profile pool backend) as a plain child. On macOS/Linux the `before-quit` handler only SIGTERMs them fire-and-forget, and a force-quit or crash never runs that handler at all. Any backend that doesn't die promptly reparents to launchd (PPID=1) and lingers as a ~330 MB orphan still holding a LISTEN socket — 16 accumulated over a day of normal use in the report (issue NousResearch#61349). tui_gateway.slash_worker already self-terminates when its spawning gateway disappears; the serve backend never got the same guard. Mirror that watchdog: poll the original PPID (with a psutil create_time PID-reuse guard, since Linux reparents to a subreaper rather than PID 1) and os._exit(0) once the parent is gone. Gated on HERMES_DESKTOP=1 (already set on both desktop spawn paths) so a standalone `hermes serve &` / nohup / systemd launch — where the parent legitimately exits — is unaffected. The poll interval is a plain function default injected by tests, not a user-facing env knob. Fixes NousResearch#61349
|
Rebased onto current Re-applied both hunks on the current 6 pass in |
72b17fe to
fb99492
Compare
|
Resolved on main by PR #83406 (rebase-merged), which carries the parent-death watchdog + group-kill (from #73066) and the Desktop-boot reap of already-orphaned serve backends. Special credit here: this PR was the EARLIEST submission of the serve orphan watchdog idea (July 9), predating the implementation that ultimately landed — the design direction was yours first. Thank you! |
What does this PR do?
The Electron desktop spawns each serve/dashboard backend (the primary window backend plus every per-profile pool backend) as a plain child. On macOS/Linux the
before-quithandler only sends a fire-and-forgetSIGTERM, and a force-quit or crash never runs that handler at all. Any backend that doesn't die promptly reparents to launchd (PPID=1) and lingers as a ~330 MB orphan still holding a LISTEN socket — the report saw 16 accumulate (~5.3 GB) over a day of normal use.tui_gateway.slash_workeralready self-terminates when its spawning gateway disappears; theservebackend never got the same guard, so this mirrors that watchdog in theserve/dashboardstartup path: poll the original PPID (with apsutilcreate_timePID-reuse guard, since Linux reparents to a subreaper rather than PID 1) andos._exit(0)once the parent is gone.This is a surface-agnostic fix that also catches the force-quit / crash cases that
before-quit(Option A/B in the issue) can't, and needs no changes to the Electron shutdown path.It's gated on
HERMES_DESKTOP=1— already set on both desktop spawn paths (main.ts:6126,main.ts:6384) — so a standalonehermes serve &/ nohup / systemd launch, where the parent legitimately exits, is unaffected.Related Issue
Fixes #61349
Type of Change
Changes Made
hermes_cli/main.py: added_arm_desktop_orphan_watchdog()(+_backend_is_orphaned()helper) and call it incmd_dashboard()after the named-profile re-exec routing and before the server boots.HERMES_DISABLE_ORPHAN_WATCHDOG=1opts out;HERMES_BACKEND_WATCHDOG_POLL_Stunes the poll interval (default 2s).tests/hermes_cli/test_backend_orphan_watchdog.py: orphan-detection truth table (ppid change, create_time mismatch, healthy parent) + arm/no-op gating on the env flags.How to Test
ps -eo pid,ppid,command | grep "hermes_cli.main.*serve" | grep -v grep— noPPID=1orphans remain (they exit within one poll interval of the app dying).hermes serve &then close the shell — backend keeps running (watchdog only arms underHERMES_DESKTOP=1).pytest tests/hermes_cli/test_backend_orphan_watchdog.py -qChecklist
Code
Documentation & Housekeeping
getppid()+psutilcreate_time (portable); the standalone-launch gate keeps non-desktop use unchanged on every platform