Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The registered retry path and hand-off cleanup behavior still need to be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR preserves the Windows updater’s intended exit-2 refusal behavior when gateway recovery fails.
Changes:
- Handles gateway recovery as best effort.
- Preserves pending recovery state.
- Adds regression coverage for clean refusal behavior.
File summaries
| File | Summary |
|---|---|
tests/hermes_cli/test_update_orphan_backend_reap.py |
Adds coverage for recovery failure preserving exit status 2. |
hermes_cli/update_cmd_windows.py |
Contains two unresolved moderate findings: atexit retry exceptions remain unsuppressed (2 votes), and hand-off hard exit skips the registered cleanup (1 vote). |
Review details
Suppressed comments (1)
hermes_cli/update_cmd_windows.py:1215
- This converts a resume failure into
SystemExit(2), but the Windows hand-off child (HERMES_UPDATE_REEXEC=1) treats that as an early exit and callsos._exitincmd_update'sfinally;os._exitskips Pythonatexithandlers. Therefore the callback registered before this guard never gets the promised retry, and any gateway that failed to resume remains down. Please run the pending cleanup before the hand-off hard exit, or avoid hard-exiting this refusal path.
with _best_effort("Windows gateway service restart incomplete: %s"):
_m()._resume_windows_gateways_after_update(_windows_gateway_resume)
# Keep a failed token pending for the registered atexit retry.
sys.exit(2)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9 of 22 tasks
Author
|
Closing this to free an open-PR slot. The branch stays. |
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.
What does this PR do?
Keeps the Windows venv-holder refusal path on its intended exit-2 contract when best-effort gateway recovery raises. Recovery exceptions are contained on both the synchronous refusal path and the registered
atexitretry, so neither can replace the refusal with a traceback.The Windows hand-off child performs its pending retry before the existing
os._exitboundary, which skipsatexit. If the first launch reached liveness verification, the retry verifies that launch again instead of creating duplicate detached restart watchers.This complements #106702, which covers the separate already-up-to-date path.
Related Issue
Related to #106692. Complements #106702.
Type of Change
Changes Made
atexitpath so repeated liveness failures cannot emit an exit-time traceback.atexit, hand-off retry, and duplicate-launch paths.How to Test
SystemExit(2).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — N/A; no user-facing documentation changedcli-config.yaml.exampleif I added/changed config keys — N/A; no config keys changedCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A; no architecture or workflow changedScreenshots / Logs
Not applicable. Deterministic regression and focused-suite evidence are listed above.
Infographic
The infographic reflects the initial candidate. The current candidate passed the expanded 151-test focused gate described above.