fix(gateway): retry detached restart watcher without breakaway (salvage #42993)

contributors/emails/phixxation@gmail.com

@@ -0,0 +1 @@
+VerbalChainsaw

gateway/run.py

@@ -6970,7 +6970,10 @@ async def _launch_detached_restart_command(self) -> None:
         # that triggered the /restart command closing its console.
         if sys.platform == "win32":
             import textwrap
-            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,
+            )
 
             cmd_argv = [*hermes_cmd, "gateway", "restart"]
             watcher = textwrap.dedent(
@@ -7042,13 +7045,61 @@ def _alive(p):
                 if watcher_env.get("PYTHONPATH"):
                     pythonpath.append(watcher_env["PYTHONPATH"])
                 watcher_env["PYTHONPATH"] = os.pathsep.join(dict.fromkeys(pythonpath))
-            subprocess.Popen(
-                [watcher_python, "-c", watcher, str(current_pid), str(restart_after_s), *cmd_argv],
-                stdout=subprocess.DEVNULL,
-                stderr=subprocess.DEVNULL,
-                env=watcher_env,
-                **windows_detach_popen_kwargs(),
-            )
+            watcher_argv = [
+                watcher_python,
+                "-c",
+                watcher,
+                str(current_pid),
+                str(restart_after_s),
+                *cmd_argv,
+            ]
+            # The watcher process must itself break away from any job object the
+            # parent CLI lives in (Electron/Tauri-wrapped Hermes Desktop, Windows
+            # Terminal, schtasks shells); otherwise it is reaped when the CLI
+            # exits and the gateway never respawns.  windows_detach_popen_kwargs()
+            # carries CREATE_BREAKAWAY_FROM_JOB, but a restrictive job object
+            # (no JOB_OBJECT_LIMIT_BREAKAWAY_OK) rejects that bit with
+            # ERROR_ACCESS_DENIED, surfaced as OSError.  Retry once without the
+            # breakaway bit, preserving argv and the scrubbed watcher_env.
+            # Mirrors the canonical fallback in
+            # hermes_cli/gateway_windows.py::_spawn_detached.
+            try:
+                subprocess.Popen(
+                    watcher_argv,
+                    stdout=subprocess.DEVNULL,
+                    stderr=subprocess.DEVNULL,
+                    env=watcher_env,
+                    **windows_detach_popen_kwargs(),
+                )
+            except OSError:
+                try:
+                    subprocess.Popen(
+                        watcher_argv,
+                        stdout=subprocess.DEVNULL,
+                        stderr=subprocess.DEVNULL,
+                        env=watcher_env,
+                        creationflags=windows_detach_flags_without_breakaway(),
+                    )
+                except OSError as exc:
+                    # Both spawn attempts failed (a breakaway-denying job object
+                    # is the common cause, but OSError covers others too).
+                    # Record a minimal, path-safe diagnostic and return without
+                    # crashing the caller: state plainly that no watcher was
+                    # started, and log only the interpreter basename and a
+                    # numeric error code — never argv, env, watcher source, or
+                    # str(exc) (which can carry a full interpreter path for a
+                    # FileNotFoundError).
+                    winerror = getattr(exc, "winerror", None)
+                    error_code = winerror if winerror is not None else exc.errno
+                    error_field = "winerror" if winerror is not None else "errno"
+                    logger.warning(
+                        "Detached restart watcher was not started after the "
+                        "no-breakaway retry (%s; %s=%r). The gateway will not "
+                        "be respawned by this restart attempt.",
+                        os.path.basename(watcher_python),
+                        error_field,
+                        error_code,
+                    )
             return
 
         cmd = " ".join(shlex.quote(part) for part in hermes_cmd)

tests/tools/test_windows_native_support.py

@@ -11,8 +11,10 @@
 
 from __future__ import annotations
 
+import asyncio
 import os
 import signal
+import subprocess
 import sys
 from pathlib import Path
 from unittest import mock
@@ -1139,3 +1141,199 @@ def test_windows_keeps_console_python_and_preserves_tail(self):
         assert cwd == "C:/hermes"
         assert env["VIRTUAL_ENV"] == str(Path("C:/venv"))
         assert "PYTHONPATH" in env
+
+
+# ---------------------------------------------------------------------------
+# gateway/run.py :: GatewayRunner._launch_detached_restart_command
+# outer watcher Popen breakaway-denied fallback (PR #42993)
+# ---------------------------------------------------------------------------
+
+
+class TestGatewayRunRestartWatcherOuterPopenFallback:
+    """The Windows ``/restart`` watcher in ``gateway.run`` spawns an outer
+    detached ``python -c <watcher>`` process with
+    ``windows_detach_popen_kwargs()`` (which carries
+    ``CREATE_BREAKAWAY_FROM_JOB``).  A restrictive parent job object rejects
+    the breakaway bit with ``ERROR_ACCESS_DENIED`` (surfaced as ``OSError``);
+    the launcher must retry once without breakaway, preserving argv and the
+    scrubbed environment, and only warn — never crash, never leak secrets —
+    if the retry also fails.
+
+    Behavioral: drives the real coroutine with a mocked ``subprocess.Popen``
+    rather than asserting on source text.  Runs on Linux CI via a
+    ``sys.platform`` patch; the breakaway-bit assertions are gated on the
+    real host being Windows because ``_subprocess_compat`` caches
+    ``IS_WINDOWS`` at import time.
+    """
+
+    @staticmethod
+    def _fake_self():
+        from types import SimpleNamespace
+
+        return SimpleNamespace(
+            _detached_restart_helper_started=False,
+            _restart_drain_timeout=0.0,
+        )
+
+    @classmethod
+    def _drive(cls, gr):
+        asyncio.run(
+            gr.GatewayRunner._launch_detached_restart_command(cls._fake_self())
+        )
+
+    def test_outer_watcher_retries_without_breakaway_on_oserror(self, monkeypatch):
+        import gateway.run as gr
+        from hermes_cli._subprocess_compat import (
+            IS_WINDOWS,
+            windows_detach_flags_without_breakaway,
+            windows_detach_popen_kwargs,
+        )
+
+        monkeypatch.setattr(gr.sys, "platform", "win32")
+        monkeypatch.setattr(gr, "_resolve_hermes_bin", lambda: ["hermes"])
+
+        calls = []
+
+        def fake_popen(argv, **kwargs):
+            calls.append((argv, kwargs))
+            if len(calls) == 1:
+                raise OSError(5, "Access is denied")  # ERROR_ACCESS_DENIED
+            return MagicMock()
+
+        monkeypatch.setattr("subprocess.Popen", fake_popen)
+
+        self._drive(gr)
+
+        assert len(calls) == 2, "outer watcher must retry exactly once on OSError"
+        (argv1, kw1), (argv2, kw2) = calls
+
+        # argv is identical across primary and fallback, and every current
+        # watcher parameter survives:
+        #   [watcher_python, "-c", <script>, str(pid), str(restart_after_s), *cmd_argv]
+        assert argv1 == argv2
+        assert argv1[1] == "-c"
+        assert argv1[3] == str(os.getpid())
+        assert float(argv1[4]) >= 5.0  # restart deadline preserved
+        assert argv1[-2:] == ["gateway", "restart"]
+
+        # Scrubbed env preserved and identical on both calls.
+        assert kw1["env"] is kw2["env"]
+        assert "_HERMES_GATEWAY" not in kw1["env"]
+
+        # Stable, non-flag spawn configuration preserved across both attempts.
+        assert kw1["stdout"] is subprocess.DEVNULL
+        assert kw1["stderr"] is subprocess.DEVNULL
+        assert kw2["stdout"] is subprocess.DEVNULL
+        assert kw2["stderr"] is subprocess.DEVNULL
+
+        # Primary spreads the full detach helper.  Assert every returned helper
+        # kwarg is present on the call — meaningful on Linux CI too, where the
+        # helper returns {"start_new_session": True} (no creationflags entry):
+        # dropping the spread entirely would fail here, not just on Windows.
+        # The fallback uses the explicit no-breakaway creationflags.
+        expected_primary = windows_detach_popen_kwargs()
+        for key, value in expected_primary.items():
+            assert kw1[key] == value
+        assert kw2["creationflags"] == windows_detach_flags_without_breakaway()
+        assert "start_new_session" not in kw2
+
+        if IS_WINDOWS:
+            _BREAKAWAY = 0x01000000
+            assert kw1["creationflags"] & _BREAKAWAY, (
+                "primary spawn must request CREATE_BREAKAWAY_FROM_JOB"
+            )
+            assert not (kw2["creationflags"] & _BREAKAWAY), (
+                "fallback spawn must drop CREATE_BREAKAWAY_FROM_JOB"
+            )
+
+    def test_outer_watcher_inline_respawn_stays_no_breakaway(self, monkeypatch):
+        """The embedded respawn script (argv[2]) must keep calling
+        ``windows_detach_flags_without_breakaway()`` — current main
+        intentionally respawns the gateway without the breakaway bit, and this
+        port must not reintroduce a breakaway-first inline shape."""
+        import gateway.run as gr
+
+        monkeypatch.setattr(gr.sys, "platform", "win32")
+        monkeypatch.setattr(gr, "_resolve_hermes_bin", lambda: ["hermes"])
+
+        captured = {}
+
+        def fake_popen(argv, **kwargs):
+            captured["argv"] = argv
+            return MagicMock()
+
+        monkeypatch.setattr("subprocess.Popen", fake_popen)
+        self._drive(gr)
+
+        watcher_script = captured["argv"][2]
+        assert "windows_detach_flags_without_breakaway()" in watcher_script
+        assert "CREATE_BREAKAWAY_FROM_JOB" not in watcher_script
+
+    def test_outer_watcher_happy_path_spawns_once(self, monkeypatch):
+        import gateway.run as gr
+
+        monkeypatch.setattr(gr.sys, "platform", "win32")
+        monkeypatch.setattr(gr, "_resolve_hermes_bin", lambda: ["hermes"])
+
+        calls = []
+        monkeypatch.setattr(
+            "subprocess.Popen",
+            lambda argv, **kwargs: calls.append((argv, kwargs)) or MagicMock(),
+        )
+        warn = MagicMock()
+        monkeypatch.setattr(gr.logger, "warning", warn)
+
+        self._drive(gr)
+
+        assert len(calls) == 1, "no retry when the primary spawn succeeds"
+        warn.assert_not_called()
+
+    def test_outer_watcher_dual_failure_warns_without_leaking_secrets(
+        self, monkeypatch
+    ):
+        import gateway.run as gr
+
+        monkeypatch.setattr(gr.sys, "platform", "win32")
+        monkeypatch.setattr(gr, "_resolve_hermes_bin", lambda: ["hermes"])
+
+        calls = []
+
+        def always_fail(argv, **kwargs):
+            calls.append((argv, kwargs))
+            raise OSError(5, "Access is denied")
+
+        monkeypatch.setattr("subprocess.Popen", always_fail)
+        warn = MagicMock()
+        monkeypatch.setattr(gr.logger, "warning", warn)
+
+        # Deterministic sentinel in the environment the watcher inherits
+        # (watcher_env = os.environ.copy()); the warning must never echo it.
+        secret = "maxwell-do-not-log-this-secret-42993"
+        monkeypatch.setenv("HERMES_TEST_SECRET", secret)
+
+        # Dual failure must NOT propagate — the user's CLI still exits cleanly.
+        self._drive(gr)
+
+        assert len(calls) == 2, "both primary and fallback attempted"
+        warn.assert_called_once()
+
+        # Secret-safe logging: only (interpreter basename, error field, error
+        # code) are logged — never the exception object (str(exc) can carry a
+        # path), the argv (watcher source + interpreter path), or env contents.
+        argv_used, kwargs_used = calls[0]
+        fmt, *log_args = warn.call_args.args
+        assert len(log_args) == 3, "warning should log only (basename, field, code)"
+        basename_arg, error_field, error_code = log_args
+        assert basename_arg == os.path.basename(argv_used[0])
+        assert error_field in ("winerror", "errno")
+        assert isinstance(error_code, int)
+        for arg in log_args:
+            assert not isinstance(arg, (OSError, list, dict))
+
+        # The watcher's env carried the sentinel; the rendered warning must not.
+        assert secret in (kwargs_used.get("env") or {}).get("HERMES_TEST_SECRET", "")
+        rendered = fmt % tuple(log_args)
+        assert secret not in rendered
+        assert argv_used[2] not in rendered  # watcher script body
+        assert "argv" not in fmt.lower()
+        assert "env=" not in fmt.lower()