diff --git a/api/updates.py b/api/updates.py index 8262ac7f..512c652d 100644 --- a/api/updates.py +++ b/api/updates.py @@ -76,6 +76,29 @@ _GIT_LOCK_SIGNATURES = ( 'another git process seems to be running', 'unable to create .git/index.lock', ) + + +def _windows_restart_spawn(args, **kwargs): + """Spawn the replacement process for a Windows self-restart.""" + return subprocess.Popen(args, **kwargs) + + +def _windows_restart_exit(code): + """Exit the old process after a Windows replacement is running.""" + os._exit(code) + + +def _windows_restart_command(): + """Return the canonical replacement command for the current packaging mode.""" + if getattr(sys, "frozen", False): + return list(sys.argv) + + executable = sys.executable + if executable.lower().endswith("python.exe"): + windowless_executable = executable[:-4] + "w.exe" + if os.path.isfile(windowless_executable): + executable = windowless_executable + return [executable, str(REPO_ROOT / "server.py")] # Lock files we previously enumerated for auto-removal in v2. v2.2 no longer # removes anything on the server, so the enumerable list is no longer needed; # ``_inventory_locks`` reports whatever ``.git/**/*.lock`` files currently exist @@ -1717,9 +1740,9 @@ def _schedule_restart(delay: float = 2.0) -> None: loaded on the next request, rather than running with a mix of old and new Python modules in sys.modules. - os.execv() replaces the current process image with a fresh interpreter - running the same argv — sessions are preserved on disk, the HTTP port - is reclaimed within the delay window, and the client's own + The restart replaces the current process image or starts the canonical + server entrypoint, depending on platform and packaging mode. Sessions are + preserved on disk, the HTTP port is reclaimed within the delay window, and the client's own ``setTimeout(() => location.reload(), 2500)`` lands after the restart. Coordinates with ``_apply_lock``: when the user updates both webui @@ -1757,84 +1780,47 @@ def _schedule_restart(delay: float = 2.0) -> None: try: # Re-exec into the just-pulled image. # - # sys.argv[0]'s meaning depends on how the server was launched: - # - # * Source checkout (`python server.py` via bootstrap.py / - # ctl.sh / start.sh): sys.argv[0] is the SCRIPT path - # (e.g. "/root/hermes-webui/server.py"), sys.executable is - # the interpreter. CPython treats argv[1] as the script to - # run, so we must pass [sys.executable] + sys.argv. - # - # * Frozen/packaged build (PyInstaller, embedded zipapp, - # etc.): sys.argv[0] == sys.executable == . Passing - # [sys.executable] + sys.argv would re-insert the binary as - # argv[1] — the kernel launches it, the interpreter treats - # the binary itself as the "script" to run, and execv - # effectively becomes a recursive no-op that never reaches - # bind(), leaving the WebUI stuck "offline" after every - # self-update. Pass argv as-is instead. - # - # Distinguish the two cases with sys.frozen (set by - # PyInstaller / zipapp / similar). For source checkouts the - # `[sys.executable] + sys.argv` form is the canonical CPython - # re-exec idiom (same shape Flask/Django reloaders use) and - # is the correct path. - # # IMPORTANT: On Windows, os.execv() does NOT replace the # current process — it spawns a new process while the old # one keeps running. This causes "address already in use" # because the old process still holds the port. On Windows - # we use subprocess.Popen() + os._exit() instead. + # we use a detached spawn + exit instead. if sys.platform == 'win32': - import subprocess - if getattr(sys, "frozen", False): - args = sys.argv - else: - args = [sys.executable] + sys.argv - # Prefer pythonw.exe over python.exe so the restarted - # server does not create a visible console window. - # sys.executable may point at python.exe (console - # subsystem); substitute pythonw.exe if it exists - # next to python.exe. - _exe = sys.executable - if _exe.lower().endswith('python.exe'): - _w_exe = _exe[:-4] + 'w.exe' # python.exe -> pythonw.exe - if os.path.isfile(_w_exe): - if getattr(sys, "frozen", False): - args = sys.argv - else: - args = [_w_exe] + sys.argv + args = _windows_restart_command() # Start new process fully detached with NO console # window. DETACHED_PROCESS alone is not sufficient # on modern Windows — without CREATE_NO_WINDOW a # python.exe (console-subsystem) child still flashes # an empty terminal window, which the user then # manually kills (taking the WebUI with it). - subprocess.Popen( - args, - cwd=os.getcwd(), - creationflags=( - subprocess.DETACHED_PROCESS - | subprocess.CREATE_NEW_PROCESS_GROUP - | subprocess.CREATE_NO_WINDOW - ), - close_fds=True, - stdin=subprocess.DEVNULL, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - ) + try: + _windows_restart_spawn( + args, + cwd=os.getcwd(), + creationflags=( + subprocess.DETACHED_PROCESS + | subprocess.CREATE_NEW_PROCESS_GROUP + | subprocess.CREATE_NO_WINDOW + ), + close_fds=True, + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + except Exception: + logger.exception("Windows WebUI restart spawn failed") + return # Exit immediately — the port is released as soon as # this process dies, allowing the new process to bind. - os._exit(0) + _windows_restart_exit(0) else: if getattr(sys, "frozen", False): os.execv(sys.executable, sys.argv) else: os.execv(sys.executable, [sys.executable] + sys.argv) except Exception: - # Last-resort: if execv fails for any reason, just exit so the - # process supervisor (start.sh / Docker) restarts us. - os._exit(0) + # Last-resort: let the process supervisor restart us. + _windows_restart_exit(0) threading.Thread(target=_do, daemon=True).start() @@ -2406,11 +2392,8 @@ def _apply_update_inner(target, channel=DEFAULT_UPDATE_CHANNEL): } # Schedule a self-restart so the updated code is loaded fresh. A plain - # git pull leaves stale Python modules in sys.modules — agent imports that - # reference new symbols (functions, classes) added in the update will fail - # on the next request with AttributeError / ImportError. os.execv() re- - # execs the same interpreter with the same argv, picking up the new code - # cleanly without requiring the user to restart manually. + # git pull leaves stale Python modules in sys.modules. Replacing the process + # loads the updated code cleanly without requiring a manual restart. # # The 2 s delay gives the HTTP response time to flush to the client before # the process replaces itself. The client already does diff --git a/tests/conftest.py b/tests/conftest.py index f11dba34..5f37c1f5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1192,6 +1192,24 @@ _AGENT_PATH_ENV_KEYS = ("HERMES_WEBUI_AGENT_DIR", "PYTHONPATH", "HERMES_WEBUI_PY _REAL_AGENT_ENV = {k: os.environ.get(k) for k in _AGENT_PATH_ENV_KEYS} _REAL_SYS_PATH = list(sys.path) +# Keep the Windows restart seams inert after the suite isolation snapshots. +from api import updates as _updates + +_real_windows_restart_spawn = _updates._windows_restart_spawn +_real_windows_restart_exit = _updates._windows_restart_exit + + +def _pytest_session_safe_windows_restart_spawn(_args, **_kwargs): # pragma: no cover + return None + + +def _pytest_session_safe_windows_restart_exit(_code): # pragma: no cover + return None + + +_updates._windows_restart_spawn = _pytest_session_safe_windows_restart_spawn +_updates._windows_restart_exit = _pytest_session_safe_windows_restart_exit + def _hermes_cli_is_healthy() -> bool: mod = sys.modules.get("hermes_cli") diff --git a/tests/fixtures/issue7195_windows_restart.json b/tests/fixtures/issue7195_windows_restart.json new file mode 100644 index 00000000..b15ab7d8 --- /dev/null +++ b/tests/fixtures/issue7195_windows_restart.json @@ -0,0 +1,13 @@ +{ + "source": "https://github.com/nesquena/hermes-webui/issues/7195#issuecomment-5381720224", + "issue": "https://github.com/nesquena/hermes-webui/issues/7195", + "base": "3b9c632a1fa339abcfd457973dcf10810640e760", + "command": "python -m pytest tests/test_updates.py tests/test_update_checker.py tests/test_update_channels.py -q --tb=short -p no:cacheprovider", + "launch": { + "platform": "win32", + "frozen": false, + "argv_shape": ["C:\\Users\\david\\AppData\\Local\\hermes\\hermes-agent\\venv\\Lib\\site-packages\\pytest\\__main__.py", "tests/test_updates.py", "tests/test_update_checker.py", "tests/test_update_channels.py", "-q", "--tb=short", "-p", "no:cacheprovider"], + "restart_payload_before_fix": ["C:\\Users\\david\\AppData\\Local\\hermes\\hermes-agent\\venv\\Scripts\\python.exe", "C:\\Users\\david\\AppData\\Local\\hermes\\hermes-agent\\venv\\Lib\\site-packages\\pytest\\__main__.py", "tests/test_updates.py", "tests/test_update_checker.py", "tests/test_update_channels.py", "-q", "--tb=short", "-p", "no:cacheprovider"], + "observed_symptom": "Each pytest generation starts another pytest generation and its server fixtures, producing recursive children and leaked processes." + } +} diff --git a/tests/test_issue7195_windows_restart.py b/tests/test_issue7195_windows_restart.py new file mode 100644 index 00000000..8972ab99 --- /dev/null +++ b/tests/test_issue7195_windows_restart.py @@ -0,0 +1,167 @@ +"""Behavioral regression coverage for the Windows restart entrypoint.""" +from __future__ import annotations + +import json +import os +import pathlib +import subprocess +import sys +import threading +import time + +import pytest + +import api.updates as updates + + +FIXTURE = pathlib.Path(__file__).parent / "fixtures" / "issue7195_windows_restart.json" +REPO = pathlib.Path(__file__).resolve().parent.parent + + +def _run_restart(monkeypatch, *, argv, frozen=False, pythonw=False, spawn=None, exit=None): + events = [] + exit_event = threading.Event() + spawn_finished = threading.Event() + spawn_failed = [] + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr(sys, "executable", r"C:\Python\python.exe") + monkeypatch.setattr(sys, "argv", list(argv)) + monkeypatch.setattr(sys, "frozen", frozen, raising=False) + monkeypatch.setattr(updates, "REPO_ROOT", REPO) + monkeypatch.setattr(updates, "_AGENT_DIR", None) + monkeypatch.setattr(updates, "_wait_until_restart_safe", lambda: events.append("wait") or {"restart_blocked": False}) + monkeypatch.setattr(updates, "_purge_agent_pycache", lambda path: events.append(("purge", path))) + monkeypatch.setattr(updates.subprocess, "DETACHED_PROCESS", 1, raising=False) + monkeypatch.setattr(updates.subprocess, "CREATE_NEW_PROCESS_GROUP", 2, raising=False) + monkeypatch.setattr(updates.subprocess, "CREATE_NO_WINDOW", 4, raising=False) + monkeypatch.setattr(updates.subprocess, "DEVNULL", subprocess.DEVNULL) + real_isfile = updates.os.path.isfile + monkeypatch.setattr( + updates.os.path, + "isfile", + lambda path: pythonw if str(path).lower().endswith("pythonw.exe") else real_isfile(path), + ) + + def record_spawn(args, **kwargs): + events.append(("spawn", list(args), kwargs)) + try: + if spawn: + spawn(args, **kwargs) + except BaseException: + spawn_failed.append(True) + raise + finally: + spawn_finished.set() + + def record_exit(code): + events.append(("exit", code)) + exit_event.set() + if exit: + exit(code) + + monkeypatch.setattr(updates, "_windows_restart_spawn", record_spawn) + monkeypatch.setattr(updates, "_windows_restart_exit", record_exit) + updates._schedule_restart(delay=0) + deadline = time.monotonic() + 2 + while not any(event[0] == "spawn" for event in events if isinstance(event, tuple)): + if time.monotonic() >= deadline: + pytest.fail(f"restart worker did not spawn: {events!r}") + time.sleep(0.01) + if not spawn_finished.wait(timeout=2): + pytest.fail(f"restart worker did not finish spawning: {events!r}") + if not spawn_failed and not exit_event.wait(timeout=2): + pytest.fail(f"restart worker did not exit after spawning: {events!r}") + return events + + +def test_reported_pytest_argv_restarts_canonical_server(monkeypatch): + fixture = json.loads(FIXTURE.read_text(encoding="utf-8")) + events = _run_restart(monkeypatch, argv=fixture["launch"]["argv_shape"]) + spawn = next(event for event in events if event[0] == "spawn") + assert spawn[1] == [r"C:\Python\python.exe", str(REPO / "server.py")] + assert not any("pytest" in token.lower() for token in spawn[1]) + assert events[-1] == ("exit", 0) + + +@pytest.mark.parametrize("argv", [ + ["server.py"], + ["python", "-m", "server"], + ["hermes-webui", "--profile", "default"], + ["wrapper", "--run", "server.py"], +]) +def test_source_restart_always_targets_server(monkeypatch, argv): + events = _run_restart(monkeypatch, argv=argv) + assert next(event for event in events if event[0] == "spawn")[1] == [r"C:\Python\python.exe", str(REPO / "server.py")] + + +def test_frozen_restart_preserves_argv(monkeypatch): + argv = [r"C:\Apps\Hermes\hermes.exe", "--profile", "default"] + assert next(event for event in _run_restart(monkeypatch, argv=argv, frozen=True) if event[0] == "spawn")[1] == argv + + +def test_windows_restart_spawns_once_then_exits_once(monkeypatch): + events = _run_restart(monkeypatch, argv=["pytest", "tests/test_issue7195_windows_restart.py"], pythonw=True) + assert events[0] == "wait" + assert events[1][0] == "purge" + assert [event[0] for event in events if isinstance(event, tuple) and event[0] in {"spawn", "exit"}] == ["spawn", "exit"] + spawn = next(event for event in events if event[0] == "spawn") + assert spawn[1][0].endswith("pythonw.exe") + assert spawn[2]["cwd"] == os.getcwd() + assert spawn[2]["creationflags"] == 7 + assert spawn[2]["close_fds"] is True + assert spawn[2]["stdin"] is subprocess.DEVNULL + assert spawn[2]["stdout"] is subprocess.DEVNULL + assert spawn[2]["stderr"] is subprocess.DEVNULL + + +def test_windows_spawn_failure_keeps_current_process(monkeypatch, caplog): + def fail(*_args, **_kwargs): + raise OSError("spawn failure") + + messages = [] + monkeypatch.setattr(updates.logger, "exception", lambda message: messages.append(message)) + events = _run_restart(monkeypatch, argv=["server.py"], spawn=fail) + assert not any(event[0] == "exit" for event in events if isinstance(event, tuple)) + assert messages == ["Windows WebUI restart spawn failed"] + + +def test_pytest_session_restart_operations_are_inert(): + assert updates._windows_restart_spawn(None) is None + assert updates._windows_restart_exit(0) is None + + +def test_local_restart_recorders_restore_session_guards(monkeypatch): + monkeypatch.setattr(updates, "_windows_restart_spawn", lambda *_args, **_kwargs: None) + monkeypatch.setattr(updates, "_windows_restart_exit", lambda *_args: None) + monkeypatch.undo() + assert updates._windows_restart_spawn(None) is None + assert updates._windows_restart_exit(0) is None + + +@pytest.mark.parametrize("frozen", [False, True]) +def test_posix_restart_keeps_execv_shape(monkeypatch, frozen): + events = [] + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setattr(sys, "executable", "/usr/bin/python") + monkeypatch.setattr(sys, "argv", ["wrapper", "--profile", "default"]) + monkeypatch.setattr(sys, "frozen", frozen, raising=False) + monkeypatch.setattr(updates, "_AGENT_DIR", None) + monkeypatch.setattr(updates, "_wait_until_restart_safe", lambda: {}) + monkeypatch.setattr(updates, "_purge_agent_pycache", lambda _path: None) + monkeypatch.setattr(os, "execv", lambda exe, argv: events.append((exe, argv))) + updates._schedule_restart(delay=0) + deadline = time.monotonic() + 2 + while not events and time.monotonic() < deadline: + time.sleep(0.01) + expected_argv = ["wrapper", "--profile", "default"] + assert events == [("/usr/bin/python", expected_argv if frozen else ["/usr/bin/python", *expected_argv])] + + +def test_fixture_subprocess_is_not_guarded(): + child = subprocess.Popen([sys.executable, "-c", "pass"]) + assert child.wait(timeout=5) == 0 + + +def test_restart_callers_remain_complete(): + source = (REPO / "api" / "updates.py").read_text(encoding="utf-8") + assert source.count("_schedule_restart()") == 3