Repository navigation
fix(update): Windows 'Already up to date' run warns on an unverified gateway relaunch instead of exiting 1 twice (#115563, salvage #115615) - #116804
Merged
Conversation
Two distinct defects in the Windows gateway pause/resume path, one
symptom: "hermes update" aborting exit 1 on a checkout that was
already current and needed no code change.
1. Asymmetric error handling between the two finish paths. The pull
path wraps the Windows gateway resume in
_resume_windows_gateways_and_merge_outcome, whose contract is
explicit: "Must never abort the update" -- it catches the failure,
marks the outcome incomplete, warns, and continues (receipt status
"partial", exit 1 only via the existing incomplete-repair gate).
_finish_already_up_to_date ("Already up to date" path) called
_resume_windows_gateways_after_update bare, so the identical
RuntimeError -- e.g. the relaunch-verification race with a parent
Job Object kill, #48820 -- killed the run as an unhandled exception
instead. Fixed by routing through the same merge helper and folding
a failed resume into current_checkout_complete, landing on the
pre-existing "partial" finalize + sys.exit(1) contract the repair
path already uses for other incomplete states.
2. The atexit callback could replay the same failure a second time.
Every foreground call site registers
_resume_windows_gateways_after_update via atexit.register as a
dead-process safety net, but the function only cleared
token["resume_needed"] on its OWN success path -- a raise from
_verify_relaunched_gateways_alive left the registration live, so
interpreter teardown called the function again with the same
still-armed token, repeated the whole relaunch + verify, failed
identically, and printed "Exception ignored in atexit callback".
Fixed by unregistering the function from atexit as soon as
execution reaches it (foreground or the atexit fallback itself) --
ownership transfers to whichever call site got there first, and a
failure below is reported exactly once. atexit.unregister is a
no-op when the function was never registered, so a non-Windows or
no-resume-needed call is unaffected.
Both fixes are surgical: they leave the "a failed relaunch keeps
resume_needed=True so the token is retryable" contract completely
untouched (see the existing
test_resume_windows_gateway_service_failure_stays_retryable /
test_resume_windows_gateway_launcher_refresh_failure_stays_retryable
tests, which this PR does not modify) -- that design is intentional,
not the bug.
Closes #115563.
Testing: Windows-specific behavior verified by code trace + full
resolution-chain tests against the real functions (mocked I/O, real
control flow) on Linux/macOS, per the "Don't fake the host OS" rule
this repo's own AGENTS.md sets -- no sys.platform patching, and the
Windows-only branch (_is_windows() == True) is exercised exactly as
the existing test suite for this module already does (monkeypatching
_is_windows on the shared test fixtures, not faking the platform).
True live-Windows process-topology proof is out of scope for this PR
(the wine2e lane is reserved for process-topology E2E per AGENTS.md);
this fix is control-flow/bookkeeping, not host-specific syscalls.
- 2 new regression tests, both proven red on the pre-fix code via
git stash (both fail with the exact defect signature: the "Already
up to date" path re-raises instead of demoting to partial; the
atexit unregister call never happens) and green on the fix
- scripts/run_tests.sh across the full affected surface (Windows
update/resume/reconciliation + related pause/resume/venv-repair
test files): 228 passed, 0 failed, 4 skipped (pre-existing,
unrelated to this change)
૮ >ﻌ< ა ci reviewran on 0ad5e93 — fix(cli): symmetric Windows resume-failure handling + atexit all good! |
alt-glitch
added
type/bug
3 tasks done
alt-glitch
added
os/windows
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Windows
hermes updatethat is already up to date no longer aborts withRuntimeError: Windows gateway relaunch after update was not verified alive, and the failure is no longer replayed a second time from theatexitcallback.hermes_cli/update_cmd.py::_finish_already_up_to_date: the Windows resume now goes through_resume_windows_gateways_and_merge_outcome(the pull path's "must never abort the update" wrapper) — a failed relaunch verification prints the existing ⚠ block, marks the outcome incomplete and finishes with the partial receipt, exactly like the pull path.hermes_cli/update_cmd_windows.py::_resume_windows_gateways_after_update: unregisters itself fromatexitthe moment it runs, so the safety-net registration (armed for process death before the foreground call) cannot re-run the whole relaunch + verify at interpreter teardown and printException ignored in atexit callbackwith the same traceback.Salvages #115615 (@tylerbrevard) — cherry-picked with authorship.
Live repro
Linux, real
_resume_windows_gateways_after_update+ real_verify_relaunched_gateways_alive, only the two fleet boundaries stubbed (relaunch → one profile,_wait_for_gateway_ready→[], the #48820 Job-Object kill),atexitarmed asupdate_cmddoes:foreground: RuntimeError …thenVERDICT: verify ran 2x— the atexit callback replayed the relaunch + failure (Exception ignored in atexit callbacktraceback)foreground: RuntimeError …(caught by the wrapper in production),VERDICT: verify ran 1xReal windows-latest runner (
wine2e/**): redorigin/main+ tests → https://github.com/NousResearch/hermes-agent/actions/runs/35492704174 (test_already_up_to_date_path_demotes_on_windows_resume_failure FAILED,test_resume_unregisters_its_own_atexit_fallback_before_running FAILED); green this head → https://github.com/NousResearch/hermes-agent/actions/runs/35492706307 (7 passed).Root cause: the pull path wrapped the Windows resume in a never-abort helper while
_finish_already_up_to_datecalled_resume_windows_gateways_after_updatebare, and the function only disarmed its token on the success path so theatexitregistration re-ran it after a failure.Out of scope, as the issue says: the 30 s verify poll losing the race against the detached restart watcher (#48820 / #107002) — the relaunch that appears 19 s after the updater exits is still reported as unverified, now as one ⚠ instead of a fatal traceback printed twice. The three early-abort call sites (
parked branch skipped,detached HEAD,wrong branch) already exit 1 by design and are unchanged.Fixes #115563
Dropped hunks
None.
Infographic