fix(gateway): spare supervised gateways from the reaper robustly (all platforms) (#83683) - #86702
EvanProgramming wants to merge 1 commit into
Conversation
… platforms) (NousResearch#83683) Improves on the exclusion-based reap fix (NousResearch#86658). The reaper must never kill a supervised gateway, but NousResearch#86658's exclusion depended on gateway.pid being present: on Windows _get_service_pids() is empty and a Scheduled-Task gateway that lost its pidfile would still be SIGTERM'd. It also walked the entire parent chain up to PID 1, broader than needed. Changes in _reap_unsupervised_gateway_orphans(): - Exclude service-managed PIDs (launchd / systemd) unconditionally — same as NousResearch#86658, preserves the NousResearch#51325/NousResearch#75936 duplicate-port protection. - Exclude the recorded gateway PID + its immediate supervisor parent (the bootstrap whose argv matches the scan), bounded to one level instead of the whole chain to PID 1. - Add a supervisor-owned backstop in the orphan filter: any gateway process whose parent chain reaches the supervisor (launchd / systemd / the Windows services tree) is spared even with no pidfile — closing the Windows gap NousResearch#86658 left. Tests: existing macOS launchd tests pinned deterministic; new TestReapSparesSupervisorOwnedWithoutPidfile covers the pidfile-less Windows (Scheduled Task) and macOS (launchd) cases. Relaunch half of NousResearch#83683 is tracked separately in NousResearch#86693.
alt-glitch
added
type/bug
|
Salvaged via #86757 with your authorship preserved (cherry-pick — the commit on the salvage branch is authored by you). What survived and what didn't, with the reasoning: Kept — the Windows pidfile-less backstop. Your gap identification was correct and is real on current main: Dropped — the POSIX ancestry checks (launchd / systemd / init). We verified these empirically on a real macOS host before deciding: every Unix process has PID 1 in its ancestry within a few hops, and a genuine orphan is reparented directly to PID 1 — so Dropped — the two hunks #86658 already merged. The unconditional On your defect 1 (Windows pidfile-less gap): confirmed and fixed via your backstop. Defect 2 (unbounded walk): main's walk terminates at PID 1 and runs once (not per-candidate), so it's bounded in practice and cheaper than the per-candidate alternative; we kept main's version. Defect 3 (test gap): the salvage adds the pidfile-less Windows test you called for, plus a bootstrap-exited fail-open test documenting the backstop's honest limitation. Thanks for the careful iteration on this bug class — #86693 (the relaunch half) remains open for separate review. |
…han reaper on Windows (#83683) On Windows _get_service_pids() is empty (no systemd/launchd query), so a Scheduled-Task-supervised gateway whose gateway.pid record is missing or stale is invisible to both the service-PID and recorded-PID exclusions the reaper already applies (#86658) — and gets SIGTERM'd on every desktop open (#86098 class, pidfile-less path). Add a Windows-only backstop: any reaper candidate whose parent chain reaches services.exe (the Task Scheduler launches tasks under the services tree) is spared even with no pidfile. The backstop is deliberately inert on POSIX: every process there has PID 1 (launchd/init/systemd) in its ancestry — and a genuine orphan is reparented directly to PID 1 — so supervisor-name ancestry carries zero supervision signal and would disable the reaper entirely on macOS/WSL (#51325, #75936). POSIX supervised gateways are already covered pidfile-independently by the _get_service_pids() exclusion. Known limitation (fail-open, documented): if the Task-launched bootstrap parent has already exited, Windows does not reparent the gateway, the chain breaks before services.exe, and the gateway is treated as an orphan. Salvaged from #86702 by @EvanProgramming (authorship preserved); reduced to the genuinely-new Windows backstop — the PR's other two hunks were already merged on main via #86658 (one in a strictly stronger full-parent-chain form) and its POSIX ancestry checks were dropped as unsound (verified empirically: a true double-fork orphan's psutil parent IS launchd).
…han reaper on Windows (NousResearch#83683) On Windows _get_service_pids() is empty (no systemd/launchd query), so a Scheduled-Task-supervised gateway whose gateway.pid record is missing or stale is invisible to both the service-PID and recorded-PID exclusions the reaper already applies (NousResearch#86658) — and gets SIGTERM'd on every desktop open (NousResearch#86098 class, pidfile-less path). Add a Windows-only backstop: any reaper candidate whose parent chain reaches services.exe (the Task Scheduler launches tasks under the services tree) is spared even with no pidfile. The backstop is deliberately inert on POSIX: every process there has PID 1 (launchd/init/systemd) in its ancestry — and a genuine orphan is reparented directly to PID 1 — so supervisor-name ancestry carries zero supervision signal and would disable the reaper entirely on macOS/WSL (NousResearch#51325, NousResearch#75936). POSIX supervised gateways are already covered pidfile-independently by the _get_service_pids() exclusion. Known limitation (fail-open, documented): if the Task-launched bootstrap parent has already exited, Windows does not reparent the gateway, the chain breaks before services.exe, and the gateway is treated as an orphan. Salvaged from NousResearch#86702 by @EvanProgramming (authorship preserved); reduced to the genuinely-new Windows backstop — the PR's other two hunks were already merged on main via NousResearch#86658 (one in a strictly stronger full-parent-chain form) and its POSIX ancestry checks were dropped as unsound (verified empirically: a true double-fork orphan's psutil parent IS launchd).
…han reaper on Windows (NousResearch#83683) On Windows _get_service_pids() is empty (no systemd/launchd query), so a Scheduled-Task-supervised gateway whose gateway.pid record is missing or stale is invisible to both the service-PID and recorded-PID exclusions the reaper already applies (NousResearch#86658) — and gets SIGTERM'd on every desktop open (NousResearch#86098 class, pidfile-less path). Add a Windows-only backstop: any reaper candidate whose parent chain reaches services.exe (the Task Scheduler launches tasks under the services tree) is spared even with no pidfile. The backstop is deliberately inert on POSIX: every process there has PID 1 (launchd/init/systemd) in its ancestry — and a genuine orphan is reparented directly to PID 1 — so supervisor-name ancestry carries zero supervision signal and would disable the reaper entirely on macOS/WSL (NousResearch#51325, NousResearch#75936). POSIX supervised gateways are already covered pidfile-independently by the _get_service_pids() exclusion. Known limitation (fail-open, documented): if the Task-launched bootstrap parent has already exited, Windows does not reparent the gateway, the chain breaks before services.exe, and the gateway is treated as an orphan. Salvaged from NousResearch#86702 by @EvanProgramming (authorship preserved); reduced to the genuinely-new Windows backstop — the PR's other two hunks were already merged on main via NousResearch#86658 (one in a strictly stronger full-parent-chain form) and its POSIX ancestry checks were dropped as unsound (verified empirically: a true double-fork orphan's psutil parent IS launchd).
3 participants
Critique of #86658 (why this PR exists)
#86658 is on the right track — it replaces my earlier full-bail guard with an exclusion-based reap (spare service-managed + recorded PIDs), which correctly preserves the #51325/#75936 duplicate-port protection that a
_gateway_has_active_supervisor()full-bail would have disabled. I'm not re-litigating that; the exclusion direction is correct. But #86658 has three concrete defects that leave the original #83683 bug partially unfixed:gateway.pid. On Windows_get_service_pids()returns an empty set (no systemd/launchd query), so the only thing sparing a Scheduled-Task gateway isget_running_pid()(the pidfile) plus a parent-chain walk starting from that recorded PID. If the supervised gateway is alive but its pidfile is missing/stale, the reaper SIGTERMs it — the exact Desktop backend orphan-reap kills Windows Scheduled-Task gateway on every GUI relaunch #86098 class of bug, just on the pidfile-less path. macOS is robust (launchd list returns the PID directly); Windows was not.psutilimporting cleanly at reap time. The only ancestor that matters is the immediate supervisor bootstrap.get_running_pid()to return the recorded PID, so the pidfile-less path (defect 1) is never exercised — the gap is unguarded.This PR keeps what #86658 got right and fixes all three.
Summary
An improved version of the reap-side fix for #83683 (supersedes the reap half of #86658).
What #86658 got right (kept)
gateway restart/ dashboard //restartleave an orphaned, untracked gateway holding the port; status reports "stopped" while it is running #51325/[Bug]: macOS manual gateway restart leaves orphan gateway processes — one Discord message gets N threads/N replies #75936 duplicate-port protection on every platform. Correct call.What this PR changes in
_reap_unsupervised_gateway_orphans()gateway restart/ dashboard //restartleave an orphaned, untracked gateway holding the port; status reports "stopped" while it is running #51325/[Bug]: macOS manual gateway restart leaves orphan gateway processes — one Discord message gets N threads/N replies #75936.launchd(macOS),systemd/init(Linux), or a descendant ofservices.exe(Windows Task Scheduler) — is spared even with no pidfile — closes the Windows gap (defect 1).Honest limitations (not overstated)
services.execheck is conservative: it spares a gateway whose ancestor isservices.exe. A genuinely-orphanedgateway restarton Windows is normally a child of the desktop app, notservices.exe, so it is still reaped. The recorded-PID exclusion already covers the common Windows case; the backstop is the pidfile-less safety net.Changes
hermes_cli/gateway.py: bounded recorded-PID + parent exclusion;_reaper_candidate_is_supervisor_owned()backstop in the orphan filter.tests/hermes_cli/test_gateway.py: macOS launchd tests pinned deterministic; newTestReapSparesSupervisorOwnedWithoutPidfile(pidfile-less Windows Scheduled-Task + macOS launchd cases).Validation
tests/hermes_cli/test_gateway.py(reaper classes) — 4 new/updated cases pass;test_spawn_gateway_restart_reap.py2 passed;test_gateway_proc_fallback.py2 skipped (pre-existing).