From c0e21fc13d0d196bc1b00b4f30006499f3f8dd1b Mon Sep 17 00:00:00 2001 From: Thales <> Date: Sun, 6 Sep 2026 11:28:59 +0100 Subject: [PATCH] fix(process): make the parent-death watchdog work on Windows, and the suite honest there Thirteen tests fail on a Windows checkout of main. All three causes are real, and one of them is a shipped bug that CI cannot see because CI runs Linux. ## The watchdog never fires on Windows (#579) `process_exists` took a successful `OpenProcess` as proof of life. A Windows process object outlives the process and is destroyed only when the last handle to it closes, so `OpenProcess` keeps succeeding on the pid of something that exited while anyone still holds a handle -- and someone almost always does, namely whoever spawned it and has not reaped it. That is exactly the case the watchdog exists for. A parent killed by Force Quit, Task Manager or a crash runs no cleanup, so nothing is ever reaped, so the check kept answering "alive" and the worker kept its GPU. The failure this was written to prevent is the failure it could not detect. `GetExitCodeProcess` is the call that separates the two states. Its one documented ambiguity, a process that genuinely exits with code 259, lands on the "alive" side, which is the side this function must fail towards: a watchdog that shoots on a question it could not answer kills a live separation. `tests/test_process_liveness.py` asserted the old behaviour as correct, so it is rewritten around the corrected semantics: still-active is alive, an open handle with a real exit code is dead, and an unreadable exit code is alive. The fake kernel32 grows `GetExitCodeProcess` and the handle-leak assertion is kept on every path, since this runs on a one-second timer. ## The log-zip test asserted bytes it never wrote (#580) `Path.write_text` opens in text mode, which turns `\n` into `\r\n` on Windows. The endpoint then zipped the file byte for byte, correctly, and the test compared it against the string it thought it had written. A test about byte preservation has to control its own bytes, so the fixture now passes `newline=""`. ## Beat-grid tests failed unreadably without ffmpeg (#581) `_decode_mono` degrades to `None` rather than raising, deliberately: everything it feeds is a display field. In a test that makes a missing binary and an undetectable grid produce the same `assert grid is not None`, with the real cause only in a log record nothing asserts on. The `stems_dir` fixture now skips when ffmpeg cannot be resolved, through the same `ffmpeg_executable()` the app uses, so a portable install or a `STEMDECK_FFMPEG_DIR` pointing at a bundled build still counts as present. The skip reason names that variable rather than leaving someone to guess. The guard covers all twelve tests taking the fixture, not just the ten that went red. The four `test_returns_none_*` cases were passing without ffmpeg for the wrong reason: they got their None from the decode failing rather than from the audio, so they could not have caught a regression in what they test. Conditional on the binary, never on the platform, so CI keeps full coverage: `.github/workflows/ci.yml` installs ffmpeg into the test container. ## Verified both ways Without ffmpeg: 945 passed, 52 skipped, none failed. With `STEMDECK_FFMPEG_DIR` set: 997 passed, nothing skipped, which is what CI runs. Co-Authored-By: Claude Opus 5 --- app/core/process.py | 25 ++++++++++++++-- tests/test_logs_api.py | 7 ++++- tests/test_pipeline_beatgrid.py | 28 ++++++++++++++++- tests/test_process_liveness.py | 53 ++++++++++++++++++++++++++++++--- 4 files changed, 105 insertions(+), 8 deletions(-) diff --git a/app/core/process.py b/app/core/process.py index 965c4e21..e6a86476 100644 --- a/app/core/process.py +++ b/app/core/process.py @@ -32,12 +32,33 @@ def process_exists(pid: int) -> bool: PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 ERROR_INVALID_PARAMETER = 87 + STILL_ACTIVE = 259 kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid) - if handle: + if not handle: + return ctypes.get_last_error() != ERROR_INVALID_PARAMETER + + # Opening it is not proof it is running. A Windows process object outlives + # the process and is destroyed only once the last handle to it closes, so + # OpenProcess keeps succeeding on the pid of something that exited minutes + # ago while anyone still holds a handle -- and someone almost always does, + # namely whoever spawned it and has not reaped it. That is precisely the + # case this watchdog exists for: a parent killed by Force Quit or Task + # Manager runs no cleanup, so nothing is ever reaped and the worker would + # keep its GPU forever (#579). + # + # GetExitCodeProcess is the call that separates the two states. Its one + # documented ambiguity is a process that genuinely exits with code 259, + # which is indistinguishable from a running one -- and that error lands on + # the "alive" side, which is the side this function must fail towards. + try: + code = ctypes.c_ulong() + ok = kernel32.GetExitCodeProcess(handle, ctypes.byref(code)) + finally: kernel32.CloseHandle(handle) + if not ok: return True - return ctypes.get_last_error() != ERROR_INVALID_PARAMETER + return code.value == STILL_ACTIVE _PARENT_POLL_SECONDS = 1.0 diff --git a/tests/test_logs_api.py b/tests/test_logs_api.py index 8b704037..92eec9c1 100644 --- a/tests/test_logs_api.py +++ b/tests/test_logs_api.py @@ -87,7 +87,12 @@ def test_zip_bundles_every_present_log(client, logs_dir): def test_zip_preserves_contents(client, logs_dir): - (logs_dir / "stemdeck.log").write_text("line one\nline two\n", encoding="utf-8") + # newline="" so the bytes on disk are the bytes written. Text mode + # translates \n to \r\n on Windows, and this test is about whether the + # endpoint preserves a file byte for byte -- a fixture that quietly rewrites + # itself cannot answer that, and made the assertion fail there for a reason + # having nothing to do with the endpoint (#580). + (logs_dir / "stemdeck.log").write_text("line one\nline two\n", encoding="utf-8", newline="") with zipfile.ZipFile(io.BytesIO(client.get("/api/logs.zip").content)) as z: assert z.read("stemdeck.log").decode() == "line one\nline two\n" diff --git a/tests/test_pipeline_beatgrid.py b/tests/test_pipeline_beatgrid.py index 96331d9c..20203296 100644 --- a/tests/test_pipeline_beatgrid.py +++ b/tests/test_pipeline_beatgrid.py @@ -1,11 +1,13 @@ from __future__ import annotations import json +import shutil import wave import pytest from fastapi.testclient import TestClient +from app.core.config import ffmpeg_executable from app.core.models import Job from app.core.registry import _jobs from app.pipeline.beatgrid import ( @@ -26,9 +28,33 @@ def _isolate_registry(): _jobs.clear() +# Resolved the way the app resolves it, so a portable install or a +# STEMDECK_FFMPEG_DIR pointing at a bundled build counts as present. Computed +# once: every test below would otherwise ask the filesystem the same question. +_FFMPEG = shutil.which(ffmpeg_executable()) + + @pytest.fixture def stems_dir(tmp_path, monkeypatch): - """A stems dir that `_load_audio_ffmpeg`'s JOBS_DIR containment check accepts.""" + """A stems dir that `_load_audio_ffmpeg`'s JOBS_DIR containment check accepts. + + Skips the whole test when ffmpeg is missing rather than letting it fail + downstream. `_decode_mono` is deliberately forgiving -- everything it feeds + is a display field, so it logs and returns None instead of raising -- which + means a missing binary and an undetectable beat grid arrive here as the same + `assert grid is not None`, and the real cause is only in a log record + nothing asserts on (#581). + + The guard covers every test taking this fixture, not only the ones that go + red. The four `test_returns_none_*` cases passed without ffmpeg for the + wrong reason: they got their None from the decode failing, not from the + audio, so they could not have caught a regression in what they test. + """ + if _FFMPEG is None: + pytest.skip( + "ffmpeg not found: install it, or point STEMDECK_FFMPEG_DIR at a " + "directory containing an ffmpeg binary" + ) from app.pipeline import analyze as analyze_mod monkeypatch.setattr(analyze_mod, "JOBS_DIR", tmp_path) diff --git a/tests/test_process_liveness.py b/tests/test_process_liveness.py index 0c12b81b..38fa3bb3 100644 --- a/tests/test_process_liveness.py +++ b/tests/test_process_liveness.py @@ -73,38 +73,83 @@ def test_posix_signal_zero_is_a_probe_not_a_kill(monkeypatch): # ── the Windows branch ─────────────────────────────────────────────── +STILL_ACTIVE = 259 + + class _Kernel32: """Stands in for kernel32, so the Windows path runs anywhere.""" - def __init__(self, handle: int, last_error: int = 0): + def __init__( + self, + handle: int, + last_error: int = 0, + exit_code: int = STILL_ACTIVE, + exit_code_ok: bool = True, + ): self._handle = handle + self._exit_code = exit_code + self._exit_code_ok = exit_code_ok self.last_error = last_error self.closed: list[int] = [] def OpenProcess(self, _access, _inherit, _pid): # noqa: N802 - Win32 name return self._handle + def GetExitCodeProcess(self, _handle, out): # noqa: N802 - Win32 name + # `out` is what ctypes.byref produced; _obj is the c_ulong behind it. + out._obj.value = self._exit_code + return self._exit_code_ok + def CloseHandle(self, handle): # noqa: N802 - Win32 name self.closed.append(handle) return True -def _run_windows_branch(monkeypatch, handle: int, last_error: int = 0): +def _run_windows_branch( + monkeypatch, + handle: int, + last_error: int = 0, + exit_code: int = STILL_ACTIVE, + exit_code_ok: bool = True, +): import ctypes monkeypatch.setattr(os, "name", "nt") - fake = _Kernel32(handle, last_error) + fake = _Kernel32(handle, last_error, exit_code, exit_code_ok) monkeypatch.setattr(ctypes, "WinDLL", lambda *_a, **_k: fake, raising=False) monkeypatch.setattr(ctypes, "get_last_error", lambda: fake.last_error, raising=False) return fake, process_exists(1234) -def test_windows_an_open_handle_means_alive(monkeypatch): +def test_windows_a_still_active_process_is_alive(monkeypatch): fake, alive = _run_windows_branch(monkeypatch, handle=42) assert alive is True assert fake.closed == [42], "the handle leaked; this runs on a watchdog timer" +def test_windows_an_open_handle_is_not_enough_to_mean_alive(monkeypatch): + """The bug this file previously asserted as correct (#579). + + A Windows process object outlives the process and dies only with the last + handle to it, so OpenProcess keeps succeeding on something that exited + while anyone still holds one -- and the un-reaped parent of a Force-Quit is + exactly that. Treating the open handle as proof of life meant the watchdog + never fired and the worker kept its GPU. + """ + fake, alive = _run_windows_branch(monkeypatch, handle=42, exit_code=0) + assert alive is False + assert fake.closed == [42], "the handle leaked; this runs on a watchdog timer" + + +def test_windows_an_unreadable_exit_code_means_alive(monkeypatch): + """Same ambiguity rule as everywhere else here: if the call that would + settle it fails, the answer is "alive". A watchdog must not shoot on a + question it could not ask.""" + fake, alive = _run_windows_branch(monkeypatch, handle=42, exit_code=0, exit_code_ok=False) + assert alive is True + assert fake.closed == [42] + + def test_windows_invalid_parameter_means_dead(monkeypatch): """ERROR_INVALID_PARAMETER (87) is what OpenProcess returns for a pid that does not exist. It is the only failure that proves absence."""