From c8ec7eafddd74cd82e6ac71189b1d3e0a71dd3b2 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Fri, 18 Sep 2026 04:59:45 +0800 Subject: [PATCH] emrg: the client asks the one liveness probe instead of spelling it twice --- emrg/client/daemon_manager.py | 48 ++++++++++++++++++------ tests/test_daemon_manager.py | 70 +++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/emrg/client/daemon_manager.py b/emrg/client/daemon_manager.py index b5f835d7..505f05cd 100644 --- a/emrg/client/daemon_manager.py +++ b/emrg/client/daemon_manager.py @@ -384,6 +384,41 @@ async def _await_daemon_ready( ) +def _old_daemon_alive(pid: int, *, platform: str = "", kill=None, + win_probe=None) -> bool: + """Is the daemon we just signalled gone? — asked once, not respelled. + + The platform decision belongs to :func:`emrg._stop_all.pid_alive`, which is + what `emrg server stop` already asks; this used to be a second copy of it + here (issue #1349). A bare ``os.kill(pid, 0)`` is **not** a liveness probe on + Windows: ``signal.CTRL_C_EVENT`` is 0, so CPython routes it to + ``GenerateConsoleCtrlEvent`` and the call becomes a Ctrl+C delivered to that + pid's console process group — every process sharing it, this CLI's shell + included. Windows SIGTERM is an immediate hard kill, so a *port* probe is + what answers there, and :func:`is_running` is this caller's Windows probe. + + ``platform`` / ``kill`` / ``win_probe`` are forwarded to `pid_alive` so both + answers can be pinned on every runner: the Windows branch is the unsafe one, + and a probe whose Windows behaviour is only observable on Windows is a defect + discovered on Windows. + + One reading changes, and it is worth naming rather than discovering: the copy + here read ``EPERM`` as *alive* (the process exists, we may not signal it) and + `pid_alive` reads any ``OSError`` as *gone*. The pid this is asked about is + the daemon this client just SIGTERMed — its own child, same user — so an + ``EPERM`` cannot arise for it, and one answer for the class is the point of + asking here instead of spelling the question twice. + """ + from emrg._stop_all import pid_alive + + return pid_alive( + pid, + platform=platform, + kill=kill, + win_probe=is_running if win_probe is None else win_probe, + ) + + async def check_and_restart_if_stale() -> None: """Ping the server. If source has changed since server started, restart it. @@ -447,18 +482,7 @@ async def check_and_restart_if_stale() -> None: pass def _old_pid_alive() -> bool: - if sys.platform == "win32": - # os.kill(pid, 0) would TerminateProcess on Windows — - # never use it as a liveness probe. Windows SIGTERM is - # an immediate hard kill, so the port probe suffices. - return is_running() - try: - os.kill(server_pid, 0) - return True - except ProcessLookupError: - return False - except OSError: - return True # EPERM → process exists + return _old_daemon_alive(server_pid) for _ in range(50): # up to 10s for graceful shutdown await asyncio.sleep(0.2) diff --git a/tests/test_daemon_manager.py b/tests/test_daemon_manager.py index e6a439ad..ce5c06da 100644 --- a/tests/test_daemon_manager.py +++ b/tests/test_daemon_manager.py @@ -271,6 +271,76 @@ def test_server_programming_error_propagates(self, mock_connect, mock_src, mock_ asyncio.run(daemon_manager.check_and_restart_if_stale()) +# ── the old daemon's liveness probe (issue #1349) ──────────── + +class TestTheOldDaemonProbe: + """`_old_daemon_alive` — the probe the restart path waits on. + + Both answers are pinned on every runner: the Windows one must never enter + `os.kill` (signal 0 is `CTRL_C_EVENT` there — a Ctrl+C delivered to that + console process group, this CLI's shell included), and the POSIX one is a + signal-0 question. This file is skipped on Windows, so pinning the *Windows* + decision here is the whole point: it is observable on the runners that do + run these tests, which is where a defect like this has to be caught. + """ + + def test_posix_asks_with_signal_zero(self): + calls: list = [] + assert daemon_manager._old_daemon_alive( + 4321, platform="linux", + kill=lambda pid, sig: calls.append((pid, sig)), + ) is True + assert calls == [(4321, 0)] + + def test_posix_reads_a_gone_pid_as_gone(self): + """Both states of the POSIX answer — the live side alone proves nothing.""" + def _gone(pid, sig): + raise ProcessLookupError(3, "No such process") + + assert daemon_manager._old_daemon_alive( + 4321, platform="linux", kill=_gone) is False + + def test_windows_never_enters_os_kill(self): + """The negative control: reaching `os.kill` on Windows IS the bug.""" + def _would_signal(pid, sig): + raise AssertionError( + f"os.kill({pid}, {sig}) on Windows is CTRL_C_EVENT — a Ctrl+C to " + "that console process group") + + seen: list = [] + assert daemon_manager._old_daemon_alive( + 4321, platform="win32", kill=_would_signal, + win_probe=lambda pid: seen.append(pid) or True, + ) is True + assert seen == [4321] + + def test_windows_default_answer_is_the_port_probe(self): + """With no probe injected, Windows is answered by `is_running()`.""" + with patch("emrg.client.daemon_manager.is_running", + return_value=False) as mock_running: + assert daemon_manager._old_daemon_alive( + 4321, platform="win32") is False + assert mock_running.called + + def test_the_platform_decision_is_not_respelled_here(self): + """One spelling of the rule — the deletion is what is being pinned. + + The scan is checked in the direction that makes it an instrument: the + positive half asserts text this file must still contain, so a scan that + cannot read what it claims to read fails instead of passing. The negative + half pins the *mechanism* — a platform verdict read here, or a signal-0 + probe — rather than the word `win32`, which this file legitimately carries + in `win32_no_window_kwargs` (a spawn flag, not a liveness decision). The + file does still *signal*, with SIGTERM, which is not a probe. + """ + src = Path(daemon_manager.__file__).read_text() + assert "_old_daemon_alive" in src, "the scan cannot see the file it reads" + assert "os.kill(server_pid, signal.SIGTERM)" in src, ( + "the scan cannot see the file it reads") + assert "os.kill(server_pid, 0)" not in src + assert "sys.platform" not in src + + # ── ensure_connected ───────────────────────────────────────── class TestEnsureConnected: