Repository navigation
fix(gateway): route the post-update restart handoff through a job-surviving owner - #127240
JoaoMarcos44 wants to merge 1 commit into
Conversation
…viving owner When the updater's watcher sits inside a Job Object that refuses (or later tears down around) CREATE_BREAKAWAY_FROM_JOB, the respawned gateway dies with the job. Delegate the handoff to a transient Task Scheduler task, whose service outlives the updater's job, whenever the watcher is inside a job; keep the existing breakaway spawn (and its no-breakaway retry) elsewhere. Also teach tests/home_io_guard about the stable-ABI stdlib directory so the new job-handoff test is not tripped by interpreter-source file I/O.
arkheioncorp
left a comment
There was a problem hiding this comment.
Review: fix(gateway): route the post-update restart handoff through a job-surviving owner
Verdict: APPROVE
On Windows, when the watcher process sits inside a Job Object (Electron/Tauri parent), respawning the gateway with CREATE_BREAKAWAY_FROM_JOB can fail with ERROR_ACCESS_DENIED if the job refuses breakaway. This PR detects process_is_in_job() and, when inside a job, delegates the respawn to a transient Task Scheduler task (_spawn_gateway_via_transient_task) whose launcher runs outside the updater job. The temp task + VBS launcher are cleaned up in a finally block.
Findings
- No security concerns. The VBS launcher receives only non-secret runtime env vars (no credentials). Credentials remain profile-scoped in
HERMES_HOME. The temp directory is created with a predictable prefix (hermes-gateway-handoff-) under the system temp — low risk, but worth noting. Cleanup infinallyis correct; ifschtasks /Deletefails, the warning is logged, not swallowed silently. - No logic errors. The job-detection path and the non-job
CREATE_BREAKAWAYfallback are both preserved. Exception handling in the job path writes to_stdio_fhand re-raises — consistent with the existing watcher contract. - Suggestions (non-blocking):
- The temp dir prefix is predictable. On a crowded/shared temp dir, a 12-hex-uuid suffix makes collisions very unlikely, but the prefix itself is not secret. This is not a practical risk for a desktop product, but if the threat model ever includes a malicious local actor racing temp-file creation, consider whether the prefix should be randomized too.
_spawn_gateway_via_transient_taskhas a 30s timeout for the launcher to markstarted.marker. If WScript fails to launch (e.g., blocked by policy), the task is deleted and the temp dir removed — good. Confirm the 30s bound is acceptable for the slowest supported Windows configuration; if not, the error message could include the timeout value so operators can tune it.
|
Thanks — the premise holds up. #121635 (which closed #107002) says in its own body that the breakaway-denied teardown is "not fixed here", and Microsoft's nested-jobs docs support the claim that a breakaway can succeed while an outer job still contains the process. I reviewed this on macOS: tests, a simulated run of the generated watcher with the job check / spawn /
Two other open PRs target the same gap: #84409 and #115495. It would help to settle one design across the three. Whichever lands, it needs a receipt from a real Windows run — the |
|
Thanks for the careful Windows-path analysis. The premise is real, but this head leaves the gateway down when Task Scheduler handoff fails, loses the existing breakaway diagnostic/stdio trace, and can orphan a hidden task if the watcher dies before its |
Root cause
_spawn_gateway_restart_watcher(hermes_cli/gateway.py) respawned the gateway withCREATE_BREAKAWAY_FROM_JOBand, when the parent Job Object refused breakaway, retried with a plainPopeninside the same job. When that job tears down — the updater's non-breakaway job closing isthe reported case — the respawned gateway dies with it, and
hermes updatereports failure after asuccessful code swap. #121635 fixed the watcher identity matcher and the verification window but
its own body states the breakaway-denied teardown "is not fixed here". No open PR examined covers
the post-update handoff (audit ownership analysis: #108720/#125117 cover other spawns, #115163 does
not integrate with the updater resume path).
Fix
Per the issue's gap, the handoff needs an owner that survives job teardown — not another
Popenretry with different flags. On Windows, when the watcher is inside a job (
process_is_in_job()),the respawn is delegated to a transient Task Scheduler task (
_spawn_gateway_via_transient_task),whose service outlives the updater's job. Outside a job, the existing breakaway spawn and its
no-breakaway retry are preserved unchanged.
Also extends
tests/home_io_guard.pywithsysconfig.get_path("stdlib")in the allowed interpreterprefixes: on stable-ABI Python installs the stdlib source lives under a directory that
sys.base_prefixdoes not cover, and the guard's false positive blocked the new test's interpreterfile I/O (no weakening of the guard's scope for user files).
Verification
tests/hermes_cli/test_gateway_restart_job_handoff.pyruns the productionrespawn path in a real Windows Job Object:
ImportError: cannot import name 'process_is_in_job'(and the audit reprotests/audit/test_issue_107002.py shows the behavioural failure on unfixed code:
"respawned gateway died when the updater's non-breakaway Job Object closed");
scripts/run_tests.sh).returned 1 passed, 0 failed.
References: #107002 (original report), #127089 (audit tracking), #127076 (audit umbrella),
#121635 (partial fix that documents this gap).
Tracking
Closes #127089 (premature-closure tracking)
Refs #107002 (original report), #127076 (audit umbrella)