fix(update): escape parent job via schtasks for Windows gateway post-update spawn - #84409
fix(update): escape parent job via schtasks for Windows gateway post-update spawn#84409Halldrix wants to merge 1 commit into
Conversation
|
Reporter of #84185 here. This PR is exactly the follow-up @teknium1 asked for when closing #84185 — the honest-reporting fix (#86687) makes the silent death visible, but the gateway still dies after every update until a manual I reproduced the cold-start death 4x on my Windows 10 + Scheduled Task + Telegram setup. Happy to test this PR against my Scheduled Task environment if it helps the review. Requesting maintainer attention so this doesn't sit unmerged: @teknium1 |
PR: fix(update): escape parent job via schtasks for Windows gateway post-update spawn
|
ae4a128 to
761d84f
Compare
…update spawn The current spawn path uses subprocess.Popen with CREATE_BREAKAWAY_FROM_JOB, but CreateProcess accepts the flag silently even when the parent job denies breakaway — the spawned gateway lands inside the updater's job and is killed when the updater exits, before logging initializes. The printed ✓ in _cold_start_windows_gateway_after_update (and the silent watcher in _spawn_gateway_restart_watcher) is therefore invisible and wrong. Replace the primary post-update spawn with schtasks /Run when the Scheduled Task exists: the Task Scheduler service runs the gateway outside any job containing the updater. Fall back to the direct spawn only when no task is registered, and even then gate the success ✓ behind _wait_for_gateway_ready so a dead-on-arrival spawn surfaces as an explicit ✗ + manual-recovery hint (addresses the issue's expected behavior: report failure instead of printing ✓). The same job-escape logic is injected into _spawn_gateway_restart_watcher so unmapped gateways (Scheduled Task / manually-launched) also respawn through the Task Scheduler when it's available — covers both the cold-start and restart-watcher failure modes reported in the issue. Refs: NousResearch#84185 Refs: NousResearch#84212
761d84f to
ba02e37
Compare
|
@Enough1122 thanks for the detailed review. We've evaluated each point: Points 2 and 3 are now implemented — they are the core of the #84185 bug class, not speculative hardening: Point 2 (PID matching): We now snapshot the set of existing gateway PIDs before triggering the Scheduled Task, and the post-trigger poll only counts PIDs that did not exist before ( Point 3 (stale task command): The Scheduled Task scripts are now regenerated and re-registered before every Point 1 is legitimate debt, but the watcher runs as an embedded Point 4 is a theoretical race (task takes >6s and the fallback direct spawn competes for the port). The adaptive-poll fix would introduce an arbitrary long wait in the happy path. We're marking this as a known limitation — if anyone can demonstrate the contention in a reproducible environment, we'll treat it as a prioritized follow-up. Point 5 confirmed, no action needed. |
What does this PR do?
Escape the parent job object for the post-update Windows gateway spawn by routing through the Task Scheduler when the gateway's Scheduled Task exists.
This is the second half of the fix for #84185 — the companion to #84212 (which addresses only the silent/false success report). Where #84212 makes the updater honest ("did the process actually survive?"), this PR makes the spawn actually succeed in the first place by leaving the parent job object via
schtasks /Run /tn <Hermes_Gateway>.Related Issue
Refs: #84185
Refs: #84212
This PR does not close #84185 on its own — #84212 fixes the silent lie, this PR fixes the silent death.
Evidence (validated against a real Windows 11 host)
subprocess.PopenCREATE_BREAKAWAY_FROM_JOBCreateProcesssucceeds, noOSError, but the child is silently kept inside the job and killed with itsubprocess.Popenschtasks /Run /tn <task>Crucially,
breakaway_error=Noneon the test host: no failure signal, no detection path. The current ✓ is doubly invisible.Type of Change
Changes Made
hermes_cli/gateway_windows.py— new helper_spawn_via_scheduled_task(timeout_s=6.0): registers + runs the gateway's own Scheduled Task viaschtasks /Run, then polls_wait_for_gateway_ready()until a new gateway process appears. Before triggering, the task scripts are regenerated (_write_task_script()+_install_scheduled_task()delete+create) so the spawn never replays a stale Python path from task-creation time. The post-trigger poll snapshots existing gateway PIDs (pre_pids) and only returnsTruewhen a genuinely new PID appears (set(ready) - pre_pids) — a pre-update gateway still draining does not satisfy the check. ReturnsFalsewhen no task is registered, script write fails, registration fails, the trigger failed, or no new process showed up in time.hermes_cli/update_cmd.py—_cold_start_windows_gateway_after_update(): (1) prefer_spawn_via_scheduled_task()and print✓ ... (via Scheduled Task); (2) fall back to_spawn_detached()when no task exists; (3) gate the fallback ✓ behind_wait_for_gateway_ready()and print an explicit✗ ... did not survive+hermes gateway startrecovery hint when the spawned process never comes up.hermes_cli/gateway.py—_spawn_gateway_restart_watcher(): inject the same Scheduled-Task escape into the inline watcher script so unmapped gateways (Scheduled Task / vbs / manually-launchedgateway run) also respawn through the Task Scheduler when registered. The watcher refreshes task scripts before triggering (_write_task_script()+_install_scheduled_task()) and snapshots pre-existing PIDs before the poll (_pre_pids), counting only new processes (set(_fgp()) - _pre_pids). Falls back to the currentsubprocess.Popen(breakaway)→Popen(no breakaway)chain only when the task route fails — covers the "no spawn at all" variant of [Bug]: Windows: gateway cold-started after 'hermes update' dies silently (no logs, no PID file, no exit record) — offline until manual restart #84185 (see issue thread).tests/hermes_cli/test_update_gateway_schtasks_escape.py— 11 new tests covering: (a) schtasks preferred + ✓ when task exists; (b) fallback to direct spawn when no task; (c) survival-check reporting (✓ vs ✗ + recovery hint); (d) helper's full contract: no task registered, schtasks /Run failure, no new PID appears, only pre-existing gateway (must NOT pass), script write failure, task registration failure, task triggered + new PID (must pass); (e) watcher source-level assertions confirming task refresh + PID snapshot are present.tests/hermes_cli/test_update_cold_start_gateway_liveness.py— 2 existing tests updated to assert the new output strings (✓ Starting Windows gateway after update/✗ ... did not survive) and mock_spawn_via_scheduled_taskto force the fallback path.How to Test
scripts/run_tests.sh tests/hermes_cli/test_update_gateway_schtasks_escape.py -v scripts/run_tests.sh tests/hermes_cli/test_update_cold_start_gateway_liveness.py -v # plus the neighboring update-path suites: scripts/run_tests.sh tests/hermes_cli/test_gateway_windows.py tests/hermes_cli/test_update_venv_health.py tests/hermes_cli/test_update_orphan_backend_reap.pySabotage-run: with the fix stashed, all 11 new tests fail (error/failure); with the fix restored they all pass. The 2 modified liveness tests also fail without the updated assertions. Verified locally before opening.
Real-Windows validation: the job-object escape was reproduced end-to-end on a real Windows 11 host via a minimal two-process harness (parent creates a
KILL_ON_JOB_CLOSEjob, spawns the "updater" inside it, closes the handle).subprocess.Popen+breakaway → child killed;schtasks /Run→ child survives. See issue #84185 thread for the full evidence table.Review follow-up (review by @Enough1122): Points 2 and 3 were implemented after review — see the reply comment for the full rationale. Point 1 (deduplication of schtasks logic between watcher and helper) is deferred to a follow-up issue due to the watcher's embedded
python -cconstraint. Point 4 (fallback port race) is a known limitation with no demonstrated repro.Checklist
CONTRIBUTING.md)fix(update): escape parent job via schtasks for Windows gateway post-update spawn)🛠️ Dev: Halldrix
🤖 Sidekick: Hermes Agent v0.20.0
🔍 Discipline: Test-first (
tests/hermes_cli/test_update_gateway_schtasks_escape.py), sabotage-run verified (11/11 red without fix), real-Windows prototype validated on hardware affected by #84185, review-bot points 2-3 implemented post-review