fix(gateway): retry detached restart watcher without breakaway - #42993
fix(gateway): retry detached restart watcher without breakaway#42993VerbalChainsaw wants to merge 1 commit into
Conversation
Code Review — VerificationReviewed: Clean ✅ This PR adds to the inlined restart watcher's call in , with a proper fallback that retries without the breakaway bit. The pattern mirrors the canonical fallback in (PR #40909). Strengths:
No issues found. The implementation is well-structured and the test coverage is comprehensive. |
2a9ec7a to
1730f12
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the Windows job-object failure mode. The outer watcher fallback is still needed on current main: gateway/run.py:6278-6284 calls windows_detach_popen_kwargs() without an OSError retry, while hermes_cli/_subprocess_compat.py:156-183 provides the no-breakaway fallback.
Problems
- The
gateway/run.pyhunk predates the current watcher refactor. Current main now passes a restart deadline through the watcher (gateway/run.py:6207-6210), preserves a scrubbed environment (gateway/run.py:6251-6277), and may usepythonw.exe(gateway/run.py:6257-6269). The proposed argv at PRgateway/run.py:4285would need a focused port rather than a direct cherry-pick. - Current main intentionally uses
windows_detach_flags_without_breakaway()for the inline respawn atgateway/run.py:6243-6247(introduced bye7d2f0b93). The new AST tests requiring the former breakaway-first inline shape should not be carried forward.
Suggested changes
- Port only the outer
Popenretry around currentgateway/run.py:6278, preserving its argv and environment, then retry withwindows_detach_flags_without_breakaway(). - Add a focused regression test for a first-call
OSErroron the current outer watcher spawn rather than source-shape assertions for the superseded inline implementation.
Automated hermes-sweeper review.
| subprocess.Popen( | ||
| cmd, | ||
| [sys.executable, "-c", watcher, str(current_pid), *cmd_argv], |
There was a problem hiding this comment.
Current main now passes watcher_python, restart_after_s, and the scrubbed watcher_env here (gateway/run.py:6251-6283). Port the retry wrapper around that current call; do not restore this older argv shape.
| @@ -1007,3 +1007,508 @@ def test_launch_detached_profile_gateway_restart_outer_popen_has_access_denied_f | |||
| "CreateProcess and retry without the breakaway bit, matching " | |||
| "gateway_windows._spawn_detached's fallback pattern." | |||
| ) | |||
|
|
|||
| def test_gateway_run_restart_watcher_inlined_respawn_uses_breakaway(self): | |||
There was a problem hiding this comment.
This test hard-requires a breakaway-first inline respawn, but current main intentionally uses windows_detach_flags_without_breakaway() at gateway/run.py:6243-6247. Replace it with a focused behavioral test for the outer watcher retry on current main.
The Windows /restart watcher's outer Popen spawns the watcher with windows_detach_popen_kwargs() (which carries CREATE_BREAKAWAY_FROM_JOB), but a restrictive parent job object can reject that bit with OSError and the current call has no retry. Preserve the current watcher implementation and add a focused breakaway-denied fallback. Preserved from current main: watcher_python / pythonw.exe selection, the str(restart_after_s) deadline, the scrubbed watcher_env, the intentional no-breakaway inline respawn, and the entire POSIX setsid/bash path. - primary keeps **windows_detach_popen_kwargs() - on OSError, retry the same argv/env with creationflags=windows_detach_flags_without_breakaway() - on dual failure, log a definitive, path-safe warning (interpreter basename + numeric winerror/errno only) and return without crashing Replace the superseded breakaway-first inline design and its AST tests with focused behavioral coverage that drives the real coroutine with a mocked subprocess.Popen (retry, argv/env/DEVNULL preservation, POSIX single-session kwarg, no-breakaway inline respawn, secret-safe logging). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1730f12 to
ffdf3a0
Compare
|
Thanks for the detailed steer — reconstructed against current
|
|
Merged via PR #70262 — your commit was cherry-picked onto current main with authorship preserved in git log (rebase-merge). The focused retry, argv/env preservation, and behavioral tests all landed as you wrote them. Thanks for the clean fix and for correctly scoping it to the one missing retry site. |
What does this PR do?
Repairs the Windows detached
/restartwatcher against currentmain.The outer watcher spawn uses
windows_detach_popen_kwargs(), which includesCREATE_BREAKAWAY_FROM_JOB. Restrictive parent job objects can reject that bit withOSError, but the current call has no retry. This PR preserves the current watcher implementation and adds a focused retry without the breakaway bit.Current-main behavior preserved
watcher_python/pythonw.exeselectionstr(restart_after_s)deadline argumentwatcher_env, including removal of_HERMES_GATEWAYwindows_detach_flags_without_breakaway()setsid/bashpathChange
**windows_detach_popen_kwargs().OSError, retry the same argv and environment withcreationflags=windows_detach_flags_without_breakaway().winerror/errno, then return without crashing the CLI.Tests
Added focused behavioral coverage that drives the real coroutine with mocked
subprocess.Popenand verifies:OSErrortriggers exactly one no-breakaway retryVerification
Native Windows, Python 3.11.9, pytest 9.0.3:
test_windows_native_support.py: 68 passed, 2 pre-existing Windows-host failures reproduced on pristineorigin/maingit apply --checkagainst currentorigin/main: passed