diff --git a/app/core/process.py b/app/core/process.py index 965c4e2..e6a8647 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 8b70403..92eec9c 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 96331d9..2020329 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 0c12b81..38fa3bb 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."""