fix(windows): retry watcher Popen without breakaway when parent job denies it, plus regression tests for the breakaway bit

hermes_cli/_subprocess_compat.py

@@ -35,6 +35,7 @@
     "IS_WINDOWS",
     "resolve_node_command",
     "windows_detach_flags",
+    "windows_detach_flags_without_breakaway",
     "windows_hide_flags",
     "windows_detach_popen_kwargs",
 ]
@@ -152,6 +153,36 @@ def windows_detach_flags() -> int:
     )
 
 
+def windows_detach_flags_without_breakaway() -> int:
+    """Same as :func:`windows_detach_flags` minus ``CREATE_BREAKAWAY_FROM_JOB``.
+
+    The docstring on :func:`windows_detach_flags` notes that a process in
+    a job which disallows breakaway (no ``JOB_OBJECT_LIMIT_BREAKAWAY_OK``)
+    will see ``ERROR_ACCESS_DENIED`` from CreateProcess, surfacing as
+    ``OSError`` (``PermissionError``) on the ``subprocess.Popen`` call.
+    Callers that want to recover — by retrying without the breakaway
+    bit — can pair the two helpers symbolically rather than coding the
+    ``& ~0x01000000`` magic at every site:
+
+    .. code-block:: python
+
+        try:
+            subprocess.Popen(argv, creationflags=windows_detach_flags(), …)
+        except OSError:
+            subprocess.Popen(
+                argv,
+                creationflags=windows_detach_flags_without_breakaway(),
+                …,
+            )
+
+    See ``gateway_windows.py::_spawn_detached`` for the canonical
+    implementation of this pattern.  Returns 0 on non-Windows.
+    """
+    if not IS_WINDOWS:
+        return 0
+    return _CREATE_NEW_PROCESS_GROUP | _DETACHED_PROCESS | _CREATE_NO_WINDOW
+
+
 def windows_hide_flags() -> int:
     """Return Win32 creationflags that merely hide the child's console
     window without detaching the child.  0 on non-Windows.

hermes_cli/gateway.py

@@ -641,7 +641,10 @@ def launch_detached_profile_gateway_restart(profile: str, old_pid: int) -> bool:
     #
     # ``windows_detach_popen_kwargs()`` returns the right kwargs for the
     # host platform and is a no-op on POSIX (just ``start_new_session=True``).
-    from hermes_cli._subprocess_compat import windows_detach_popen_kwargs
+    from hermes_cli._subprocess_compat import (
+        windows_detach_flags_without_breakaway,
+        windows_detach_popen_kwargs,
+    )
 
     watcher = textwrap.dedent(
         """
@@ -677,35 +680,66 @@ def launch_detached_profile_gateway_restart(profile: str, old_pid: int) -> bool:
             _DETACHED_PROCESS = 0x00000008
             _CREATE_NO_WINDOW = 0x08000000
             _CREATE_BREAKAWAY_FROM_JOB = 0x01000000
-            _popen_kwargs["creationflags"] = (
+            _flags = (
                 _CREATE_NEW_PROCESS_GROUP
                 | _DETACHED_PROCESS
                 | _CREATE_NO_WINDOW
                 | _CREATE_BREAKAWAY_FROM_JOB
             )
+            try:
+                _popen_kwargs["creationflags"] = _flags
+                subprocess.Popen(cmd, **_popen_kwargs)
+            except OSError:
+                # CREATE_BREAKAWAY_FROM_JOB can be rejected with
+                # ERROR_ACCESS_DENIED when the parent's job object refuses
+                # breakaway. Retry without it — DETACHED_PROCESS et al.
+                # alone are enough in most setups. Mirrors the canonical
+                # fallback in gateway_windows._spawn_detached.
+                _popen_kwargs["creationflags"] = _flags & ~_CREATE_BREAKAWAY_FROM_JOB
+                subprocess.Popen(cmd, **_popen_kwargs)
         else:
             _popen_kwargs["start_new_session"] = True
-        subprocess.Popen(cmd, **_popen_kwargs)
+            subprocess.Popen(cmd, **_popen_kwargs)
         """
     ).strip()
 
+    watcher_argv = [
+        sys.executable,
+        "-c",
+        watcher,
+        str(old_pid),
+        *_gateway_run_args_for_profile(profile),
+    ]
+
+    # Same platform-aware detach for the watcher process itself — so
+    # closing the user's terminal doesn't kill the watcher.
     try:
-        # Same platform-aware detach for the watcher process itself — so
-        # closing the user's terminal doesn't kill the watcher.
         subprocess.Popen(
-            [
-                sys.executable,
-                "-c",
-                watcher,
-                str(old_pid),
-                *_gateway_run_args_for_profile(profile),
-            ],
+            watcher_argv,
             stdout=subprocess.DEVNULL,
             stderr=subprocess.DEVNULL,
             **windows_detach_popen_kwargs(),
         )
     except OSError:
-        return False
+        # CREATE_BREAKAWAY_FROM_JOB rejected by the parent job object
+        # (Electron, Windows Terminal with restrictive job settings, …).
+        # Retry without it. POSIX never reaches this branch — there
+        # ``start_new_session=True`` cannot raise OSError — so the
+        # fallback is only meaningful on Windows.
+        try:
+            fallback_kwargs: dict = (
+                {"creationflags": windows_detach_flags_without_breakaway()}
+                if sys.platform == "win32"
+                else {"start_new_session": True}
+            )
+            subprocess.Popen(
+                watcher_argv,
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL,
+                **fallback_kwargs,
+            )
+        except OSError:
+            return False
     return True
 
 

tests/tools/test_windows_native_support.py

@@ -541,10 +541,12 @@ def test_resolve_node_command_fallback_when_absent(self):
     def test_windows_flags_zero_on_posix(self):
         from hermes_cli._subprocess_compat import (
             windows_detach_flags,
+            windows_detach_flags_without_breakaway,
             windows_hide_flags,
         )
         if sys.platform != "win32":
             assert windows_detach_flags() == 0
+            assert windows_detach_flags_without_breakaway() == 0
             assert windows_hide_flags() == 0
 
     def test_windows_detach_popen_kwargs_is_posix_equivalent_on_posix(self):
@@ -556,7 +558,9 @@ def test_windows_detach_popen_kwargs_is_posix_equivalent_on_posix(self):
             # branch behaviour.  Do NOT break Linux/macOS here.
             assert kwargs == {"start_new_session": True}
         else:
-            # Windows path must include creationflags with all 3 bits set.
+            # Windows path must include creationflags with all 4 bits set
+            # (including CREATE_BREAKAWAY_FROM_JOB — see the dedicated
+            # breakaway test below for the rationale).
             assert "creationflags" in kwargs
             assert kwargs["creationflags"] != 0
             # No start_new_session on Windows (silently no-op there).
@@ -567,10 +571,57 @@ def test_windows_detach_flags_has_expected_win32_bits(self, monkeypatch):
         from hermes_cli import _subprocess_compat as sc
         monkeypatch.setattr(sc, "IS_WINDOWS", True)
         flags = sc.windows_detach_flags()
-        # CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_NO_WINDOW
+        # CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_NO_WINDOW |
+        # CREATE_BREAKAWAY_FROM_JOB
         assert flags & 0x00000200, "missing CREATE_NEW_PROCESS_GROUP"
         assert flags & 0x00000008, "missing DETACHED_PROCESS"
         assert flags & 0x08000000, "missing CREATE_NO_WINDOW"
+        assert flags & 0x01000000, "missing CREATE_BREAKAWAY_FROM_JOB"
+
+    def test_windows_detach_flags_includes_breakaway_from_job(self, monkeypatch):
+        """CREATE_BREAKAWAY_FROM_JOB is load-bearing for the GUI-driven update path.
+
+        Without it, the gateway-respawn watcher spawned by ``hermes update``
+        (which runs under hermes-setup.exe, itself a grandchild of the
+        Electron Desktop app) gets reaped when Electron exits and its
+        Win32 job object is torn down by the OS.  Result: gateway dies
+        during update and never comes back.
+
+        Regression guard against accidentally dropping the breakaway bit
+        from the default detach bundle.  This was fixed in
+        ``fix/windows-gateway-reliability`` (PR #40909) and the bit must
+        stay in the default bundle going forward.
+        """
+        from hermes_cli import _subprocess_compat as sc
+        monkeypatch.setattr(sc, "IS_WINDOWS", True)
+        assert sc.windows_detach_flags() & 0x01000000, (
+            "CREATE_BREAKAWAY_FROM_JOB (0x01000000) must remain in the "
+            "default detach flag bundle so the Desktop GUI update flow "
+            "can respawn the gateway after Electron exits."
+        )
+
+    def test_windows_detach_flags_without_breakaway_drops_only_that_bit(
+        self, monkeypatch
+    ):
+        """Fallback retry payload for restrictive job objects.
+
+        Some Windows Terminal / container / kiosk configurations refuse
+        CREATE_BREAKAWAY_FROM_JOB with ERROR_ACCESS_DENIED.  Callers
+        catch ``OSError`` and retry with this payload (see
+        ``gateway_windows._spawn_detached`` for the canonical pattern).
+        It must drop ONLY the breakaway bit — DETACHED_PROCESS et al.
+        are still required for the child to survive the parent's exit.
+        """
+        from hermes_cli import _subprocess_compat as sc
+        monkeypatch.setattr(sc, "IS_WINDOWS", True)
+        full = sc.windows_detach_flags()
+        fallback = sc.windows_detach_flags_without_breakaway()
+        # Fallback equals full minus the breakaway bit, nothing else changed.
+        assert fallback == full & ~0x01000000
+        # And the three "detach" bits we still need are present.
+        assert fallback & 0x00000200, "fallback missing CREATE_NEW_PROCESS_GROUP"
+        assert fallback & 0x00000008, "fallback missing DETACHED_PROCESS"
+        assert fallback & 0x08000000, "fallback missing CREATE_NO_WINDOW"
 
 
 # ---------------------------------------------------------------------------
@@ -880,3 +931,77 @@ def test_gateway_run_update_has_windows_branch(self):
         assert 'if sys.platform == "win32":' in source
         # Windows branch uses windows_detach_popen_kwargs
         assert "windows_detach_popen_kwargs" in source
+
+    def test_launch_detached_profile_gateway_restart_inlined_watcher_uses_breakaway(self):
+        """The inlined respawn script (stringified Python passed to ``python -c``)
+        must include CREATE_BREAKAWAY_FROM_JOB so the *respawned gateway* also
+        breaks away from any job-object the watcher itself inherits.
+
+        Static check — the watcher source is built at import time and embedded
+        verbatim in the module text.  Parsing it for an exact AST node would be
+        brittle; the textual presence of the hex flag plus the symbolic name is
+        a sufficient regression guard.
+
+        The bit was added to the inlined payload by PR #40909.  This test
+        ensures a future refactor of the dedent block doesn't silently drop it.
+        """
+        root = Path(__file__).resolve().parents[2]
+        text = (root / "hermes_cli" / "gateway.py").read_text(encoding="utf-8")
+        marker = "watcher = textwrap.dedent("
+        idx = text.find(marker)
+        assert idx != -1, "watcher block not found in gateway.py"
+        end = text.find(").strip()", idx)
+        assert end != -1, "watcher block end not found"
+        block = text[idx:end]
+        assert "0x01000000" in block, (
+            "Inlined respawn watcher must set CREATE_BREAKAWAY_FROM_JOB "
+            "(0x01000000) on the respawned gateway — without it, the new "
+            "gateway is reaped when the parent job is torn down."
+        )
+        assert "_CREATE_BREAKAWAY_FROM_JOB" in block, (
+            "Inlined respawn watcher must name CREATE_BREAKAWAY_FROM_JOB "
+            "symbolically so the intent is greppable."
+        )
+
+    def test_launch_detached_profile_gateway_restart_outer_popen_has_access_denied_fallback(
+        self,
+    ):
+        """When the outer watcher Popen raises OSError (breakaway denied by
+        the parent job object), the watcher launch must retry without the
+        breakaway bit instead of giving up.
+
+        This mirrors the canonical pattern in
+        ``gateway_windows._spawn_detached`` and brings the post-update
+        watcher path into parity with the gateway-start path: a
+        breakaway-denied job object on the parent process (rare but
+        possible on Windows Terminal with restrictive job settings,
+        containers, kiosk-mode shells) shouldn't take out the entire
+        gateway-respawn chain.
+
+        Static check — without standing up a real Windows job object
+        with breakaway forbidden, we can't trigger the OSError in a unit
+        test.  The textual presence of the fallback helper import +
+        ``windows_detach_flags_without_breakaway`` in the fallback path
+        is the regression guard.
+        """
+        root = Path(__file__).resolve().parents[2]
+        text = (root / "hermes_cli" / "gateway.py").read_text(encoding="utf-8")
+        assert "windows_detach_flags_without_breakaway" in text, (
+            "launch_detached_profile_gateway_restart must import "
+            "windows_detach_flags_without_breakaway so it can retry a "
+            "breakaway-denied Popen without giving up on the watcher."
+        )
+        # And the inlined watcher's respawn must also handle the denial —
+        # check the symbol is referenced INSIDE the watcher block (not
+        # just at module scope).
+        marker = "watcher = textwrap.dedent("
+        idx = text.find(marker)
+        end = text.find(").strip()", idx)
+        block = text[idx:end]
+        # The inlined script catches OSError on the respawn and retries
+        # with breakaway cleared via ``& ~_CREATE_BREAKAWAY_FROM_JOB``.
+        assert "~_CREATE_BREAKAWAY_FROM_JOB" in block, (
+            "Inlined respawn must catch OSError on the breakaway-denied "
+            "CreateProcess and retry without the breakaway bit, matching "
+            "gateway_windows._spawn_detached's fallback pattern."
+        )