From b82c12036e7a95650d400bc63f2ff043c3d83c72 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 7 Sep 2026 18:18:04 +0300 Subject: [PATCH 1/5] feat: give Windows a real eval lock and a killable process tree Two of the three guarantees the harness declared absent off POSIX were absent because nothing had implemented them, not because Windows cannot express them. claim_eval's lock reported unconditional success there, so two evals could run against the same pinned baseline worktree at once; it now takes a real one-byte lock through msvcrt, seeking to 0 first so the lock is the same lock on every call. The release order flips on Windows, which refuses to unlink an open file: unlock, close, then delete, instead of the deliberate unlink-before-close the POSIX side needs. The pid write moves off os.pwrite, which does not exist there at all. A timed-out benchmark had only its direct child killed, leaking grandchildren that keep burning CPU and corrupt every later measurement. runner now puts each child in a kill-on-close job object -- the Windows equivalent of the process group -- and _kill_tree terminates that, falling back to killing the child alone if the job could not be created. The job is closed on every path, so a leaked handle cannot accumulate across a long loop, and an eval killed outright takes its benchmark tree with it. The race this leaves is documented rather than hidden: a grandchild spawned between CreateProcess and AssignProcessToJobObject is outside the job. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EDNWUNHzZaAPxM6yhX7riC --- src/autor3search_python/runner.py | 79 ++++++++++----- src/autor3search_python/runstop.py | 59 +++++++++-- src/autor3search_python/winjob.py | 156 +++++++++++++++++++++++++++++ tests/test_runner.py | 45 ++++++++- tests/test_runstop.py | 83 +++++++++++++++ tests/test_winjob.py | 71 +++++++++++++ 6 files changed, 456 insertions(+), 37 deletions(-) create mode 100644 src/autor3search_python/winjob.py create mode 100644 tests/test_winjob.py diff --git a/src/autor3search_python/runner.py b/src/autor3search_python/runner.py index e7f5009..fc9d804 100644 --- a/src/autor3search_python/runner.py +++ b/src/autor3search_python/runner.py @@ -1,10 +1,11 @@ -"""Executes subprocesses with a timeout, captured output and a process group. +"""Executes subprocesses with a timeout, captured output and a killable tree. -The process group matters more than it sounds. pytest runs its benchmark inside -the same process, but plugins, xdist workers and the interpreter's own children -are grandchildren of this harness; a round killed without cleaning up its group +The tree matters more than it sounds. pytest runs its benchmark inside the same +process, but plugins, xdist workers and the interpreter's own children are +grandchildren of this harness; a round killed without cleaning up after it would leave one running, burning CPU and corrupting every later measurement on -the machine. +the machine. A process group carries that on POSIX and a job object carries it +on Windows (see `winjob`); both are reached through `_kill_tree`. """ from __future__ import annotations @@ -22,7 +23,7 @@ from pathlib import Path from typing import IO, TYPE_CHECKING -from autor3search_python import discover +from autor3search_python import discover, winjob if TYPE_CHECKING: from autor3search_python.config import Config @@ -172,8 +173,14 @@ def text(self) -> str: return decoded + _TRUNCATED if self._truncated else decoded -def _kill_group(proc: subprocess.Popen) -> None: - """Terminate the whole process group, then insist.""" +def _kill_tree(proc: subprocess.Popen, job: winjob.Job | None) -> None: + """Terminate the child and everything it started, then insist. + + Two mechanisms for one guarantee: a process group on POSIX, a job object + on Windows. `job` is None only where one could not be created at all, and + then this degrades to killing the direct child — worse than the guarantee, + but better than abandoning the child along with its grandchildren. + """ if _POSIX: try: pgid = os.getpgid(proc.pid) @@ -189,10 +196,13 @@ def _kill_group(proc: subprocess.Popen) -> None: return except subprocess.TimeoutExpired: continue - else: # pragma: no cover - exercised only on Windows + return + if job is not None: + job.terminate() + else: proc.kill() - with contextlib.suppress(subprocess.TimeoutExpired): - proc.wait(timeout=_GRACE_SECONDS) + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait(timeout=_GRACE_SECONDS) class Runner: @@ -226,6 +236,12 @@ def run(self, *args: str) -> Result: popen_kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP proc = subprocess.Popen(list(args), **popen_kwargs) + # Off POSIX the process group above is only a Ctrl-C routing detail, + # not something that can be killed as a unit; a job object is. It is + # taken here, immediately after the spawn, because subprocess offers + # no way to create a process directly into one (see winjob for the + # race that leaves). + job = None if _POSIX else winjob.assign(proc.pid) # Draining starts before wait(): if nothing reads either pipe while # the child fills the other one, both this process and the child # deadlock. Same reason `communicate()` uses threads internally — @@ -237,22 +253,31 @@ def run(self, *args: str) -> Result: err_reader.start() timed_out = False try: - proc.wait(timeout=self.timeout) - except subprocess.TimeoutExpired: - timed_out = True - _kill_group(proc) - # Bounded: a grandchild that escapes the process group (by calling - # setsid itself) would otherwise block this call forever. An - # unattended overnight loop must return timed_out=True rather than - # hang — a wrong answer beats nobody noticing it never came back. - with contextlib.suppress(subprocess.TimeoutExpired): - proc.wait(timeout=_GRACE_SECONDS) - # Bounded for the same reason as the wait() above: a pipe an escaped - # grandchild still holds open stays "readable" (blocked, not EOF) - # indefinitely, and the reader thread for it would otherwise never - # return. Whatever each thread has already captured is kept either way. - out_reader.join(timeout=_GRACE_SECONDS) - err_reader.join(timeout=_GRACE_SECONDS) + try: + proc.wait(timeout=self.timeout) + except subprocess.TimeoutExpired: + timed_out = True + _kill_tree(proc, job) + # Bounded: a grandchild that escapes the process group (by calling + # setsid itself) would otherwise block this call forever. An + # unattended overnight loop must return timed_out=True rather than + # hang — a wrong answer beats nobody noticing it never came back. + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait(timeout=_GRACE_SECONDS) + # Bounded for the same reason as the wait() above: a pipe an escaped + # grandchild still holds open stays "readable" (blocked, not EOF) + # indefinitely, and the reader thread for it would otherwise never + # return. Whatever each thread has already captured is kept either way. + out_reader.join(timeout=_GRACE_SECONDS) + err_reader.join(timeout=_GRACE_SECONDS) + finally: + # Always: a loop that runs thousands of commands cannot leak a + # kernel handle per command. The close is also the last line of + # defence — kill-on-job-close ends anything the command left + # behind, which is a Windows-only strictness the POSIX side has no + # equivalent for outside a timeout. + if job is not None: # pragma: no cover - Windows only + job.close() duration = time.monotonic() - start res = Result( args=tuple(args), diff --git a/src/autor3search_python/runstop.py b/src/autor3search_python/runstop.py index 8196090..0c0feac 100644 --- a/src/autor3search_python/runstop.py +++ b/src/autor3search_python/runstop.py @@ -55,23 +55,47 @@ def stop_requested(state_dir: str | Path) -> bool: def _try_lock(fd: int) -> bool: - if not _POSIX: # pragma: no cover - Windows degrades to existence only + """Take the claim, or report that someone else holds it. + + Both platforms lock the file itself rather than trusting its existence: a + pid file left behind by an eval that was killed must read as free, and a + pid file whose holder is alive must read as taken, and only the kernel + knows which is which. Windows locks a one-byte range from the current file + position, so every call here seeks to 0 first — a lock taken at whatever + offset the last write left behind would be a different lock each time, and + two evals would both believe they had it. + """ + if _POSIX: + import fcntl + + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + return False return True - import fcntl + import msvcrt + + os.lseek(fd, 0, os.SEEK_SET) try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) except OSError: return False return True def _unlock(fd: int) -> None: - if not _POSIX: # pragma: no cover + if _POSIX: + import fcntl + + fcntl.flock(fd, fcntl.LOCK_UN) return - import fcntl - fcntl.flock(fd, fcntl.LOCK_UN) + import msvcrt + + os.lseek(fd, 0, os.SEEK_SET) + with contextlib.suppress(OSError): + msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) def _read_pid(path: Path) -> tuple[int, bool]: @@ -113,20 +137,37 @@ def claim_eval(state_dir: str | Path, pid: int) -> Iterator[None]: ) locked = True os.ftruncate(fd, 0) - os.pwrite(fd, f"{pid}\n".encode(), 0) + # lseek + write rather than pwrite: os.pwrite does not exist on + # Windows. The seek is not incidental — _try_lock and _unlock both + # lock the byte at offset 0, so the position must be left where they + # expect it either way. + os.lseek(fd, 0, os.SEEK_SET) + os.write(fd, f"{pid}\n".encode()) yield finally: # Only the process that actually took the lock may remove the file. # A refused claim never reaches here with locked=True, so it can never # unlink the live holder's pid file, blind eval_running, and let a # later claim take a fresh inode while the holder is still running. - if locked: + if locked and _POSIX: # Remove before closing: closing drops the lock, and a concurrent # eval_running that acquired it in between would otherwise read a # pid file this process is about to delete. with contextlib.suppress(OSError): path.unlink(missing_ok=True) - os.close(fd) + os.close(fd) + elif locked: + # Windows will not unlink a file that is still open, so the order + # above would leave the pid file behind after every eval. Release + # the lock, close, then delete. The gap that opens between the + # close and the unlink is harmless: nothing holds the claim during + # it, so a claim taken in that window is a correct one. + _unlock(fd) + os.close(fd) + with contextlib.suppress(OSError): + path.unlink(missing_ok=True) + else: + os.close(fd) def eval_running(state_dir: str | Path) -> tuple[int, bool]: diff --git a/src/autor3search_python/winjob.py b/src/autor3search_python/winjob.py new file mode 100644 index 0000000..86d3c07 --- /dev/null +++ b/src/autor3search_python/winjob.py @@ -0,0 +1,156 @@ +"""A Windows job object — the killable process tree a POSIX process group +gives this harness for free. + +`runner` needs one guarantee from the operating system: when a benchmark hits +its timeout, everything it started dies with it. On POSIX that is +`start_new_session` plus `killpg`. Windows has no process groups in that sense +(its CREATE_NEW_PROCESS_GROUP is a Ctrl-C routing detail, not a kill target), +so the equivalent is a job object: every process a job member starts is a job +member too, and `TerminateJobObject` ends all of them at once. + +The job is created with KILL_ON_JOB_CLOSE, so the tree also dies when the last +handle to it goes away. That is deliberate and load-bearing twice over: an eval +that crashes cannot leak a benchmark tree, and `stop --force` — which +terminates the eval process rather than signalling it — takes the benchmark +down with the eval that was holding its job open. + +One race is real and not hidden: a grandchild started between CreateProcess and +AssignProcessToJobObject below is outside the job and would survive it. Windows +offers no way to create a process directly into a job through subprocess, and +the window is microseconds wide against a pytest launch that takes hundreds of +milliseconds to reach anything worth spawning. +""" + +from __future__ import annotations + +import os + +_WINDOWS = os.name == "nt" + +if _WINDOWS: # pragma: no cover - every line below is a Windows API binding + import ctypes + from ctypes import wintypes + + _JobObjectExtendedLimitInformation = 9 + _JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE = 0x2000 + _PROCESS_TERMINATE = 0x0001 + _PROCESS_SET_QUOTA = 0x0100 + + class _IoCounters(ctypes.Structure): + _fields_ = [ + ("ReadOperationCount", ctypes.c_ulonglong), + ("WriteOperationCount", ctypes.c_ulonglong), + ("OtherOperationCount", ctypes.c_ulonglong), + ("ReadTransferCount", ctypes.c_ulonglong), + ("WriteTransferCount", ctypes.c_ulonglong), + ("OtherTransferCount", ctypes.c_ulonglong), + ] + + class _BasicLimitInformation(ctypes.Structure): + _fields_ = [ + ("PerProcessUserTimeLimit", wintypes.LARGE_INTEGER), + ("PerJobUserTimeLimit", wintypes.LARGE_INTEGER), + ("LimitFlags", wintypes.DWORD), + ("MinimumWorkingSetSize", ctypes.c_size_t), + ("MaximumWorkingSetSize", ctypes.c_size_t), + ("ActiveProcessLimit", wintypes.DWORD), + ("Affinity", ctypes.c_size_t), + ("PriorityClass", wintypes.DWORD), + ("SchedulingClass", wintypes.DWORD), + ] + + class _ExtendedLimitInformation(ctypes.Structure): + _fields_ = [ + ("BasicLimitInformation", _BasicLimitInformation), + ("IoInfo", _IoCounters), + ("ProcessMemoryLimit", ctypes.c_size_t), + ("JobMemoryLimit", ctypes.c_size_t), + ("PeakProcessMemoryUsed", ctypes.c_size_t), + ("PeakJobMemoryUsed", ctypes.c_size_t), + ] + + _k32 = ctypes.WinDLL("kernel32", use_last_error=True) + # Declared rather than left to ctypes' defaults: a HANDLE returned as the + # default c_int is truncated on 64-bit Windows, which turns a valid job + # into a handle that silently fails every later call made with it. + _k32.CreateJobObjectW.restype = wintypes.HANDLE + _k32.CreateJobObjectW.argtypes = [wintypes.LPVOID, wintypes.LPCWSTR] + _k32.OpenProcess.restype = wintypes.HANDLE + _k32.OpenProcess.argtypes = [wintypes.DWORD, wintypes.BOOL, wintypes.DWORD] + _k32.SetInformationJobObject.argtypes = [ + wintypes.HANDLE, + ctypes.c_int, + wintypes.LPVOID, + wintypes.DWORD, + ] + _k32.AssignProcessToJobObject.argtypes = [wintypes.HANDLE, wintypes.HANDLE] + _k32.TerminateJobObject.argtypes = [wintypes.HANDLE, wintypes.UINT] + _k32.CloseHandle.argtypes = [wintypes.HANDLE] + + +class Job: + """One process and everything it goes on to start.""" + + def __init__(self, handle: int) -> None: + self._handle: int | None = handle + + def terminate(self) -> None: + """Kill every process in the job. Already-dead is not an error.""" + if self._handle is not None: # pragma: no branch + _k32.TerminateJobObject(self._handle, 1) + + def close(self) -> None: + """Drop the harness's handle. With no other handle open this kills + whatever is still in the job — see KILL_ON_JOB_CLOSE above.""" + if self._handle is not None: + _k32.CloseHandle(self._handle) + self._handle = None + + +def available() -> bool: + """Whether this platform has job objects at all.""" + return _WINDOWS + + +def assign(pid: int) -> Job | None: + """Put `pid` and its future descendants in a new job. + + None means the caller gets no tree guarantee and must fall back to killing + the direct child: a job object is a sequence of API calls, and every one of + them can fail (a process that already exited, a sandbox that denies + PROCESS_SET_QUOTA). Refusing to run at all over that would be worse than + the degraded kill, and pretending it worked would be worse still. + """ + if not _WINDOWS: + return None + return _assign(pid) # pragma: no cover - Windows only + + +def _assign(pid: int) -> Job | None: # pragma: no cover - Windows only + try: + handle = _k32.CreateJobObjectW(None, None) + if not handle: + return None + info = _ExtendedLimitInformation() + info.BasicLimitInformation.LimitFlags = _JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE + if not _k32.SetInformationJobObject( + handle, + _JobObjectExtendedLimitInformation, + ctypes.byref(info), + ctypes.sizeof(info), + ): + _k32.CloseHandle(handle) + return None + process = _k32.OpenProcess(_PROCESS_TERMINATE | _PROCESS_SET_QUOTA, False, pid) + if not process: + _k32.CloseHandle(handle) + return None + try: + if not _k32.AssignProcessToJobObject(handle, process): + _k32.CloseHandle(handle) + return None + finally: + _k32.CloseHandle(process) + return Job(handle) + except OSError: + return None diff --git a/tests/test_runner.py b/tests/test_runner.py index 062c307..c5cce66 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -58,7 +58,7 @@ def test_timeout_kills_grandchildren(tmp_path): @pytest.mark.slow def test_recovery_communicate_is_bounded_when_a_grandchild_escapes_the_group(tmp_path): - """A grandchild that calls os.setsid() escapes _kill_group's killpg + """A grandchild that calls os.setsid() escapes _kill_tree's killpg entirely and, inheriting the pipe fd, can keep it open indefinitely. An unbounded recovery communicate() would then block forever; Runner.run must still return, with timed_out=True, within a bounded wall time.""" @@ -402,3 +402,46 @@ def test_bench_env_strips_ambient_pytest_flags(tmp_path): assert "PYTEST_ADDOPTS" not in env assert "PYTEST_PLUGINS" not in env assert env["HOME"] == "/home/x" # only the pytest knobs go + + +class _FakeProc: + """Just enough subprocess.Popen for _kill_tree's non-POSIX branch.""" + + pid = 4242 + + def __init__(self, events: list[str]) -> None: + self.events = events + + def kill(self) -> None: + self.events.append("kill") + + def wait(self, timeout=None) -> int: + return 0 + + +class _FakeJob: + def __init__(self, events: list[str]) -> None: + self.events = events + + def terminate(self) -> None: + self.events.append("terminate job") + + +def test_a_timeout_terminates_the_job_where_there_is_no_process_group(monkeypatch): + """Off POSIX there is no group to kill, and killing the direct child alone + leaves its grandchildren burning CPU. The job object holding the whole + tree is what replaces the group there.""" + monkeypatch.setattr(runner, "_POSIX", False) + events: list[str] = [] + runner._kill_tree(_FakeProc(events), _FakeJob(events)) + assert events == ["terminate job"] + + +def test_a_timeout_still_kills_the_child_when_no_job_could_be_created(monkeypatch): + """A job object is an API call that can fail. Degrading to the old + direct-child kill is worse but not nothing; crashing would abandon the + child entirely.""" + monkeypatch.setattr(runner, "_POSIX", False) + events: list[str] = [] + runner._kill_tree(_FakeProc(events), None) + assert events == ["kill"] diff --git a/tests/test_runstop.py b/tests/test_runstop.py index 3581f6f..77f63c0 100644 --- a/tests/test_runstop.py +++ b/tests/test_runstop.py @@ -1,6 +1,8 @@ +import os import subprocess import sys import textwrap +from pathlib import Path import pytest @@ -125,3 +127,84 @@ def test_clear_eval_pid_removes_a_leftover(tmp_path): runstop.clear_eval_pid(d) assert not (d / runstop.EVAL_PID_FILE).exists() runstop.clear_eval_pid(d) # again, not an error + + +class _FakeMsvcrt: + """Stands in for the module this test runner does not have. + + The real cross-process refusal above is what proves the Windows lock on + Windows (CI runs it there); these fakes prove the far cheaper thing a + POSIX machine can still check — that the non-POSIX branch reaches for a + real lock at all, rather than the unconditional success it used to + report, which let two evals share one pinned worktree. + """ + + LK_NBLCK = 3 + LK_UNLCK = 0 + + def __init__(self, *, refuse: bool = False) -> None: + self.calls: list[tuple[int, int]] = [] + self.refuse = refuse + + def locking(self, fd: int, mode: int, nbytes: int) -> None: + self.calls.append((mode, nbytes)) + if self.refuse and mode == self.LK_NBLCK: + raise OSError(13, "another process has locked a portion of the file") + + +@pytest.fixture +def fake_windows(monkeypatch): + def install(*, refuse: bool = False) -> _FakeMsvcrt: + fake = _FakeMsvcrt(refuse=refuse) + monkeypatch.setattr(runstop, "_POSIX", False) + monkeypatch.setitem(sys.modules, "msvcrt", fake) + return fake + + return install + + +def test_windows_claim_takes_a_real_lock(tmp_path, fake_windows): + fake = fake_windows() + with runstop.claim_eval(tmp_path / "state", 4242): + assert (fake.LK_NBLCK, 1) in fake.calls + + +def test_windows_claim_is_refused_when_the_lock_is_held(tmp_path, fake_windows): + """The whole guarantee: a locked pid file means an eval is already running + against this baseline, and the second one must not start.""" + fake_windows(refuse=True) + with ( + pytest.raises(runstop.StopError, match="already running"), + runstop.claim_eval(tmp_path / "state", 4242), + ): + pass + + +def test_windows_eval_running_reports_a_held_claim(tmp_path, fake_windows): + """`stop --force` asks this question before it signals anything.""" + fake_windows(refuse=True) + (tmp_path / "state").mkdir() + (tmp_path / "state" / runstop.EVAL_PID_FILE).write_text("4242\n") + assert runstop.eval_running(tmp_path / "state") == (4242, True) + + +def test_windows_release_unlocks_and_closes_before_deleting(tmp_path, fake_windows, monkeypatch): + """Windows refuses to unlink a file that is still open, so the POSIX order + (unlink first, deliberately, so no one can take a fresh inode) would leave + the pid file behind on every run. Release, close, then delete.""" + fake_windows() + order: list[str] = [] + real_close = os.close + monkeypatch.setattr(runstop, "_unlock", lambda fd: order.append("unlock")) + monkeypatch.setattr(runstop.os, "close", lambda fd: (order.append("close"), real_close(fd))[1]) + real_unlink = Path.unlink + monkeypatch.setattr( + Path, + "unlink", + lambda self, **kw: (order.append("unlink"), real_unlink(self, **kw))[1], + ) + d = tmp_path / "state" + with runstop.claim_eval(d, 4242): + pass + assert order == ["unlock", "close", "unlink"] + assert not (d / runstop.EVAL_PID_FILE).exists() diff --git a/tests/test_winjob.py b/tests/test_winjob.py new file mode 100644 index 0000000..72ad463 --- /dev/null +++ b/tests/test_winjob.py @@ -0,0 +1,71 @@ +"""The Windows half of `runner`'s "a timed-out benchmark takes its whole tree +with it" guarantee. On POSIX a process group carries it; here a job object +does, and the only honest test of that is one that actually runs on Windows — +so the real one below is skipped everywhere else, and CI runs it on +windows-latest. +""" + +import os +import subprocess +import sys +import textwrap +import time + +import pytest + +from autor3search_python import winjob + + +def test_assign_is_unavailable_off_windows(): + """Callers must be able to degrade to killing the direct child rather than + crash on a platform with no job objects at all.""" + if os.name == "nt": + pytest.skip("this asserts the fallback taken where job objects do not exist") + assert winjob.available() is False + assert winjob.assign(os.getpid()) is None + + +@pytest.mark.skipif(os.name != "nt", reason="job objects are a Windows API") +def test_terminating_a_job_kills_a_grandchild(tmp_path): + """The whole point: a grandchild the harness never spawned itself must not + outlive the timeout that killed its parent.""" + marker = tmp_path / "grandchild.txt" + grandchild = textwrap.dedent(f""" + import time + time.sleep(5) + open({str(marker)!r}, "w").write("survived") + """) + parent = textwrap.dedent(f""" + import subprocess, sys, time + subprocess.Popen([sys.executable, "-c", {grandchild!r}]) + time.sleep(5) + """) + proc = subprocess.Popen([sys.executable, "-c", parent]) + try: + job = winjob.assign(proc.pid) + assert job is not None + time.sleep(1.0) # let the parent get its grandchild started + job.terminate() + job.close() + proc.wait(timeout=10) + finally: + if proc.poll() is None: # pragma: no cover - only if the job failed + proc.kill() + time.sleep(2.0) + assert not marker.exists(), "a grandchild outlived the job that held it" + + +@pytest.mark.skipif(os.name != "nt", reason="job objects are a Windows API") +def test_closing_a_job_kills_what_is_still_in_it(): + """`stop --force` relies on this: terminating eval drops the last handle to + every job it holds, and kill-on-close takes the benchmark tree with it. + """ + proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(5)"]) + try: + job = winjob.assign(proc.pid) + assert job is not None + job.close() + assert proc.wait(timeout=10) != 0 + finally: + if proc.poll() is None: # pragma: no cover - only if kill-on-close failed + proc.kill() From 6302b6076d3696f898aa274d56d086a8b3bf357f Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 7 Sep 2026 18:23:04 +0300 Subject: [PATCH 2/5] feat: let eval run off POSIX, and make -force reach the eval there The refusal existed because three guarantees were missing, not because the platform is unmeasurable; with them implemented, refusing to start is itself the wrong answer, and the override environment variable that unlocked it goes with it. `stop --force` no longer reports that it cannot help. It terminates the eval instead of signalling it, which is the honest shape of the thing there: no signal exists that a benchmark can act on mid-round, so the experiment is ended rather than asked to end, and it does not get to report what it abandoned. The kill takes the eval's job objects with it, so the benchmark tree dies too. Plain `stop` is unchanged everywhere. doctor stops calling the platform a FAIL that cannot be trusted and reports what is actually true: the guarantees hold, -force is immediate rather than a request, and doctor itself knows less here (no load average, no CPU governor) so it warns about less. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EDNWUNHzZaAPxM6yhX7riC --- src/autor3search_python/cli/eval.py | 27 ------------- src/autor3search_python/cli/stop.py | 60 ++++++++++++++++++----------- src/autor3search_python/doctor.py | 24 ++++++------ tests/test_cli_eval.py | 44 +++++---------------- tests/test_cli_stop.py | 49 +++++++++++++++-------- tests/test_doctor.py | 35 ++++++++--------- 6 files changed, 110 insertions(+), 129 deletions(-) diff --git a/src/autor3search_python/cli/eval.py b/src/autor3search_python/cli/eval.py index 4fe38af..ee9c498 100644 --- a/src/autor3search_python/cli/eval.py +++ b/src/autor3search_python/cli/eval.py @@ -35,19 +35,6 @@ from autor3search_python.cli import runctx from autor3search_python.cli.main import EXIT_USAGE -# eval's whole product is a number the human can stand behind. On a -# non-POSIX platform it cannot deliver one: claim_eval's concurrency guard -# always reports success (two evals can run against the same pinned -# baseline at once), `stop --force` cannot signal a running eval to abandon -# its experiment, and a timed-out benchmark leaks grandchild processes that -# keep burning CPU and corrupting every later measurement. A silent wrong -# number is worse than no number, so eval refuses outright rather than -# producing one doctor already warned about. Set this to run anyway. -ALLOW_UNSUPPORTED_PLATFORM_ENV = "AUTOR3SEARCH_PYTHON_ALLOW_UNSUPPORTED_PLATFORM" - -# Computed once, same as runstop._POSIX and runner._POSIX. -_POSIX = os.name == "posix" - def best_bench_delta(deltas: Sequence[benchio.Delta]) -> float: """The largest single-benchmark improvement, percent. 0.0 for no deltas.""" @@ -138,20 +125,6 @@ def run(args: list[str]) -> int: ) opts = parser.parse_args(args) - if not _POSIX and not os.environ.get(ALLOW_UNSUPPORTED_PLATFORM_ENV): - print( - "autor3search-python eval: refusing to run on a non-POSIX platform. This " - "harness has never been run or tested here, and cannot deliver a trustworthy " - "verdict: the concurrency guard cannot detect a second eval already running " - "against the same pinned baseline, `stop --force` cannot signal this eval to " - "abandon its experiment, and a timed-out benchmark would leak grandchild " - "processes that keep burning CPU and corrupt every later measurement. Run " - "`autor3search-python doctor` for details. Set " - f"{ALLOW_UNSUPPORTED_PLATFORM_ENV}=1 to run anyway.", - file=sys.stderr, - ) - return EXIT_USAGE - try: ctx = runctx.resolve(opts.directory, opts.tag) except runctx.ContextError as e: diff --git a/src/autor3search_python/cli/stop.py b/src/autor3search_python/cli/stop.py index d6c2806..c6b6fd0 100644 --- a/src/autor3search_python/cli/stop.py +++ b/src/autor3search_python/cli/stop.py @@ -7,6 +7,11 @@ additionally signals the running eval to abandon the experiment — that experiment is lost, because nothing was measured, and no results.tsv row is written for it. Ctrl+C on the agent is equivalent to -force. + +Off POSIX, -force is immediate rather than a request: there is no signal an +eval can act on mid-benchmark, so the process is ended instead of asked, and it +does not get to report what it abandoned. Plain `stop` behaves identically +everywhere. """ from __future__ import annotations @@ -20,8 +25,9 @@ from autor3search_python.cli import runctx from autor3search_python.cli.main import EXIT_OK, EXIT_USAGE -# Computed once, same as runstop._POSIX: `_signal_group` below calls -# os.killpg, which does not exist off this platform. +# Computed once, same as runstop._POSIX: it selects between the two ways +# -force can reach a running eval — `_signal_group` calls os.killpg, which +# does not exist off this platform, and `_terminate` ends the process instead. _POSIX = os.name == "posix" @@ -42,6 +48,21 @@ def _signal_group(pid: int) -> None: os.killpg(group_signal_target(pid), signal.SIGINT) +def _terminate(pid: int) -> None: + """End the eval outright, for a platform with no signal it can act on. + + os.kill is TerminateProcess on Windows: there is no SIGINT a benchmark + loop could notice mid-round, so the process is stopped rather than asked. + It takes the pid itself, never the negated one a process group needs. + Everything the eval was measuring dies with it — the job objects holding + each benchmark's process tree are kill-on-close, and the last handle to + them goes when the eval does. + """ + if pid <= 1: + raise ValueError(f"pid {pid} is not a process this command will signal") + os.kill(pid, signal.SIGTERM) + + def run(args: list[str]) -> int: parser = argparse.ArgumentParser(prog="autor3search-python stop") parser.add_argument("-C", dest="directory", default=".", help="repository root") @@ -98,28 +119,23 @@ def run(args: list[str]) -> int: runstop.clear_eval_pid(ctx.state_dir) else: if running: - if not _POSIX: - # `_signal_group` calls os.killpg, which does not exist on this - # platform (AttributeError, not a clean refusal) — so this must - # be caught before it is ever called, not after it raises. - print( - f"autor3search-python stop: cannot signal the running eval (pid {pid}) " - "on this platform — stop --force relies on POSIX process groups, which " - "do not exist here.", - file=sys.stderr, - ) - print( - "The graceful stop request has still been written and will be read at " - "the next verdict. To abandon the current experiment now, interrupt the " - "running agent yourself (e.g. Ctrl+C in its terminal).", - file=sys.stderr, - ) - return EXIT_USAGE try: - _signal_group(pid) - print(f"signalled the running eval (pid {pid}) to abandon its experiment.") + if _POSIX: + _signal_group(pid) + print(f"signalled the running eval (pid {pid}) to abandon its experiment.") + else: + # `_signal_group` would call os.killpg, which does not exist + # here at all (an AttributeError traceback, not a refusal), + # so the branch is taken before it is ever reached. + _terminate(pid) + print(f"terminated the running eval (pid {pid}) and everything it started.") + print( + "On this platform -force is immediate rather than a request: there is " + "no signal a benchmark can act on mid-round, so the eval was ended " + "rather than asked, and did not get to report what it abandoned." + ) except (ValueError, OSError) as e: - print(f"autor3search-python stop: could not signal pid {pid}: {e}", file=sys.stderr) + print(f"autor3search-python stop: could not stop pid {pid}: {e}", file=sys.stderr) return EXIT_USAGE else: print("no eval is running; the stop request is written and will be read next time.") diff --git a/src/autor3search_python/doctor.py b/src/autor3search_python/doctor.py index 30453d4..404ac3f 100644 --- a/src/autor3search_python/doctor.py +++ b/src/autor3search_python/doctor.py @@ -36,9 +36,9 @@ _MISSING_MODULE_RE = re.compile(r"No module named ['\"]([\w.]+)['\"]") -# Computed once, same as runstop._POSIX and runner._POSIX: this is the one -# thing that determines whether the concurrency guard, `stop --force`, and -# the timeout's process-group kill actually work. +# Computed once, same as runstop._POSIX and runner._POSIX: it selects which +# mechanism carries the concurrency guard, `stop --force`, and the timeout's +# tree kill, and it is the one thing that changes what `stop --force` means. _POSIX = os.name == "posix" @@ -119,15 +119,15 @@ def check_platform() -> Finding: if not _POSIX: return Finding( "platform", - "this platform is not POSIX; this harness has never been run or tested here, " - "and three of its guarantees are silently absent rather than merely degraded. " - "(1) The concurrency guard in claim_eval always reports success, so two evals " - "can run against the same pinned baseline worktree at once. (2) `stop --force` " - "cannot signal the running eval's process group here, so it cannot stop one. " - "(3) A benchmark that times out has only its direct child killed, not its " - "process group, so its grandchildren keep running and burn CPU, corrupting " - "every later measurement. Do not trust a number produced here.", - Severity.FAIL, + "this platform is not POSIX. The run claim is a real lock and a job object " + "gives a timed-out benchmark the killable process tree a process group gives " + "it elsewhere, so the guarantees hold; two differences are worth knowing. " + "`stop --force` is immediate here rather than a request — there is no signal " + "an eval can act on mid-benchmark, so it is ended rather than asked and does " + "not report what it abandoned (plain `stop` is unaffected). And this check " + "knows less about the machine here: no load average, no CPU governor, so it " + "warns you about less than it would on Linux.", + Severity.WARN, ) if sys.platform == "darwin": return Finding( diff --git a/tests/test_cli_eval.py b/tests/test_cli_eval.py index e3f6010..e2f6162 100644 --- a/tests/test_cli_eval.py +++ b/tests/test_cli_eval.py @@ -5,7 +5,6 @@ from autor3search_python import benchio, pipeline, results, runstop, state, verdict from autor3search_python.cli import eval as cli_eval from autor3search_python.cli import main as cli_main -from autor3search_python.cli.main import EXIT_USAGE from tests.conftest import git @@ -305,38 +304,15 @@ def test_human_output_does_not_render_sub_microsecond_benchmarks_as_all_zero( assert "0.000ms" not in out -def _boom(_opts): - raise AssertionError("pipeline.evaluate must not run when eval refuses to start") - - -def test_eval_refuses_outright_on_non_posix(run_ready, monkeypatch, capsys): - """This machine cannot actually run Windows, so this fakes only os.name - and checks our own refusal fires — not real Windows behaviour. A silent - wrong number is worse than no number: pipeline.evaluate raises if it is - ever reached, so a regression that lets eval proceed anyway fails loudly - instead of quietly producing a verdict this platform cannot back up.""" - monkeypatch.setattr(cli_eval, "_POSIX", False) - monkeypatch.delenv(cli_eval.ALLOW_UNSUPPORTED_PLATFORM_ENV, raising=False) - monkeypatch.setattr(pipeline, "evaluate", _boom) - code = cli_main.main(["eval", "-C", str(run_ready), "-desc", "x"]) - err = capsys.readouterr().err - assert code == EXIT_USAGE - assert "refusing to run on a non-POSIX platform" in err - assert "concurrency guard cannot detect a second eval" in err - assert "stop --force` cannot signal this eval" in err - assert "grandchild processes" in err - assert f"Set {cli_eval.ALLOW_UNSUPPORTED_PLATFORM_ENV}=1 to run anyway." in err - assert results.load(run_ready / results.PATH) == [] - assert runstop.eval_running(state.state_dir(run_ready, "t1")) == (0, False) - - -def test_eval_override_env_var_lets_it_run_anyway(run_ready, monkeypatch): - """The override exists for someone who wants to experiment anyway, with - eyes open — it must actually let eval proceed, not just suppress the - message.""" - monkeypatch.setattr(cli_eval, "_POSIX", False) - monkeypatch.setenv(cli_eval.ALLOW_UNSUPPORTED_PLATFORM_ENV, "1") +def test_eval_has_no_platform_refusal_left(run_ready, monkeypatch): + """eval used to refuse to run off POSIX, because three of its guarantees + were missing there. They are implemented now (runstop's claim lock, + runner's job objects, stop --force), so nothing platform-specific gates it + any more: no flag to consult, and no override environment variable — one + that still existed would read as a switch that changes what eval trusts. + """ + assert not hasattr(cli_eval, "_POSIX") + assert not hasattr(cli_eval, "ALLOW_UNSUPPORTED_PLATFORM_ENV") monkeypatch.setattr(pipeline, "evaluate", stub(keep(), [delta()])) assert cli_main.main(["eval", "-C", str(run_ready), "-desc", "x"]) == 0 - rows = results.load(run_ready / results.PATH) - assert [r.status for r in rows] == ["KEEP"] + assert [r.status for r in results.load(run_ready / results.PATH)] == ["KEEP"] diff --git a/tests/test_cli_stop.py b/tests/test_cli_stop.py index 0c83f82..07aaa7d 100644 --- a/tests/test_cli_stop.py +++ b/tests/test_cli_stop.py @@ -1,3 +1,5 @@ +import signal + import pytest from autor3search_python import runstop, state @@ -77,29 +79,44 @@ def test_group_signal_target_negates_a_real_pid(): assert cli_stop.group_signal_target(4242) == -4242 -def test_force_on_non_posix_refuses_to_signal_and_reports_clearly(started, monkeypatch, capsys): +def test_force_on_windows_terminates_the_eval_outright(started, monkeypatch, capsys): """This machine cannot actually run Windows, so this fakes only os.name and - checks our own guard fires — not real Windows behaviour. Without the guard, - `_signal_group` calls os.killpg, which does not exist on that platform: - an AttributeError traceback, not a clean refusal. `_signal_group` is - deliberately NOT monkeypatched here (unlike test_force_signals_the_running_eval) - so a regression that removes the guard would hit the real os.killpg call - and fail with a traceback instead of passing silently.""" + checks our own branch fires. Windows has no signal an eval can act on + mid-benchmark, so -force there ends the process instead of asking it to + stop — and the process, not its negated pid: os.kill takes a real pid on + that platform, and a group target of -4242 would be nonsense. + + The eval's job objects die with it, so the benchmark tree goes too.""" monkeypatch.setattr(cli_stop, "_POSIX", False) monkeypatch.setattr(runstop, "eval_running", lambda d: (4242, True)) + killed = [] + monkeypatch.setattr(cli_stop.os, "kill", lambda pid, sig: killed.append((pid, sig))) code = cli_main.main(["stop", "-C", str(started), "-force"]) - err = capsys.readouterr().err - assert code == EXIT_USAGE - assert "cannot signal the running eval (pid 4242)" in err - assert "stop --force relies on POSIX process groups" in err - assert "graceful stop request has still been written" in err - assert "interrupt the running agent yourself" in err + out = capsys.readouterr().out + assert code == 0 + assert killed == [(4242, signal.SIGTERM)] + assert "terminated the running eval (pid 4242)" in out + assert "immediate" in out assert runstop.stop_requested(state.state_dir(started, "t1")) is True -def test_force_on_posix_is_unaffected_when_no_eval_is_running(started, monkeypatch): - """The non-POSIX guard must not fire when there is nothing to signal — - that path already behaves identically to POSIX (nothing to break).""" +def test_force_on_windows_reports_a_termination_it_could_not_perform(started, monkeypatch, capsys): + """An eval that exited between the claim check and the kill, or one this + user may not touch, must be reported rather than swallowed — the human is + about to assume nothing is running.""" + monkeypatch.setattr(cli_stop, "_POSIX", False) + monkeypatch.setattr(runstop, "eval_running", lambda d: (4242, True)) + + def boom(pid, sig): + raise PermissionError("access denied") + + monkeypatch.setattr(cli_stop.os, "kill", boom) + code = cli_main.main(["stop", "-C", str(started), "-force"]) + assert code == EXIT_USAGE + assert "could not stop pid 4242" in capsys.readouterr().err + + +def test_force_off_posix_is_unaffected_when_no_eval_is_running(started, monkeypatch): monkeypatch.setattr(cli_stop, "_POSIX", False) monkeypatch.setattr(runstop, "eval_running", lambda d: (0, False)) assert cli_main.main(["stop", "-C", str(started), "-force"]) == 0 diff --git a/tests/test_doctor.py b/tests/test_doctor.py index 821cbfb..d750214 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -57,16 +57,17 @@ def test_outside_a_repository_is_a_failure(tmp_path): assert by_name(doctor.check(tmp_path), "git repo").severity is doctor.Severity.FAIL -def test_doctor_reports_windows_as_unsupported(monkeypatch): +def test_doctor_reports_windows_as_supported_with_its_one_difference(monkeypatch): """This machine cannot actually run Windows, so this fakes only the branch condition (os.name) and asserts our own logic fires — not real Windows - behaviour. It must name the three concrete gaps, not just say - "unsupported": the concurrency guard, `stop --force`, and the timeout's - grandchild leak. A weak `"FAIL" in detail` or `"OK" not in detail` - assertion would still pass if the severity itself were wrong (see the - project history of an `assert "OK" in out` that passed when every - severity label was remapped to "OK"), so the severity is checked by - identity, not string-sniffed out of the detail. + behaviour. The three gaps it used to name are implemented now (a real + claim lock, a job object per benchmark tree, a -force that reaches the + eval), so a FAIL here would be a lie in the opposite direction; what is + left is the one real difference, and the checks this platform cannot make. + A weak `"WARN" in detail` assertion would still pass if the severity itself + were wrong (see the project history of an `assert "OK" in out` that passed + when every severity label was remapped to "OK"), so the severity is + checked by identity, not string-sniffed out of the detail. Patches doctor._POSIX rather than the real os.name: os.name also drives which concrete Path class pathlib hands back, so patching it globally on @@ -80,20 +81,18 @@ def test_doctor_reports_windows_as_unsupported(monkeypatch): monkeypatch.setattr(doctor, "_POSIX", False) f = doctor.check_platform() assert f.name == "platform" - assert f.severity is doctor.Severity.FAIL - assert "claim_eval always reports success" in f.detail - assert "stop --force" in f.detail and "cannot signal" in f.detail - assert "grandchildren keep running" in f.detail - assert "Do not trust a number produced here." in f.detail + assert f.severity is doctor.Severity.WARN + assert "stop --force" in f.detail and "immediate" in f.detail + assert "load average" in f.detail and "governor" in f.detail + assert "Do not trust a number produced here." not in f.detail -def test_doctor_command_still_exits_zero_when_platform_check_fails(monkeypatch, tmp_path, capsys): - """doctor reports, it never gates — even a FAIL-severity platform check - must not change the command's own exit code.""" - monkeypatch.setattr(doctor, "_POSIX", False) +def test_doctor_command_still_exits_zero_when_a_check_fails(tmp_path, capsys): + """doctor reports, it never gates — a FAIL-severity check (here: not a git + repository at all) must not change the command's own exit code.""" assert cli_main.main(["doctor", "-C", str(tmp_path)]) == 0 labels = label_by_name(capsys.readouterr().out, doctor.check(tmp_path)) - assert labels["platform"] == "FAIL" + assert labels["git repo"] == "FAIL" def test_coverage_in_addopts_is_warned_about(tmp_path): From ca5e4d5e7d81ac3e0a9bd7a749747124fd24c7be Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 7 Sep 2026 18:24:23 +0300 Subject: [PATCH 3/5] docs: hand the agent the run, and say what Windows now does Start here was a bash block for a human to translate; the Go and TypeScript siblings hand the agent one prompt to paste, and this one now matches -- setup, the KEEP/DISCARD contract, what the agent may never touch, and how the human stops the loop. The install commands and file names are this project's own (config.toml, conftest.py, pytest-benchmark benchmarks), not the Go original's. The POSIX-only warning at the top and its counterpart under Limitations are gone, replaced by what is actually true now, including the residual job-assignment race and the fact that -force is immediate there. The claim rests on CI: windows-latest joins the matrix in the same commit, because a README that says Windows works while nothing has ever run there would be exactly the guessed number this project refuses to print. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EDNWUNHzZaAPxM6yhX7riC --- .github/workflows/ci.yml | 2 +- README.md | 114 ++++++++++++++++++++++++++------------- 2 files changed, 77 insertions(+), 39 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 871049f..da9cdfc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,7 +9,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, macos-latest] + os: [ubuntu-latest, macos-latest, windows-latest] python: ["3.11", "3.12", "3.13"] runs-on: ${{ matrix.os }} steps: diff --git a/README.md b/README.md index 27720d0..271e73f 100644 --- a/README.md +++ b/README.md @@ -39,39 +39,72 @@ timings out of [pytest-benchmark](https://pytest-benchmark.readthedocs.io/). ## Start here -> **POSIX only (Linux, macOS). Windows is not supported and not tested.** -> `autor3search-python` has never been run on Windows, and `eval` refuses to -> start there unless you explicitly override it. Three guarantees depend on -> POSIX process groups and are simply absent there: -> -> - the concurrency guard that stops two evals from running against the same -> pinned baseline at once; -> - `stop --force`'s ability to signal a running eval at all; -> - killing a timed-out benchmark's whole process tree, rather than leaking -> grandchildren that keep burning CPU. -> -> `doctor` reports all three by name if you run it anyway. See -> [Limitations](#limitations) for the override. +Open your coding agent inside the Python repository you want to make faster, +and paste this: + +```text +Install and run autor3search-python on this repository, then optimize it. + +Setup: +1. uv tool install autor3search-python (or: pipx install autor3search-python) +2. autor3search-python init + Show me the benchmarks it discovered. If it reports none, STOP and tell me: + this tool can only optimize what it can measure. +3. git add -A && git commit -m "autor3search-python init" +4. autor3search-python doctor + Show me any warnings. If the machine looks unfit to measure, stop and ask me + before continuing. +5. autor3search-python baseline -tag + This copies the repository into a pinned worktree and freezes what the + verdict depends on, so it takes a moment. + +Then: +6. Read program.md in this repository, in full. It is your instruction set for + the rest of this run. Follow it exactly. + +Rules for the whole run: +- One hypothesis per commit. Commit before each experiment, then run + `autor3search-python eval --json` and apply its verdict before touching + anything else: KEEP means the commit stays; anything else (DISCARD, FAIL, + CRASH) means `git reset --hard HEAD~1`. +- Never edit program.md, .autor3search/config.toml, results.tsv, any test or + benchmark file, conftest.py, pyproject.toml, or a lockfile. They are not + yours. +- Never pass -force to any autor3search-python command. (I may run + `autor3search-python stop -force` myself; that one is mine, not yours.) +- Print one context line before each experiment, so I can see where you are: + [exp | | vs | stop: autor3search-python stop] + +Run the loop until I stop you. I stop you by running +`autor3search-python stop` in my own terminal — you will see it as +"stop_requested": true in a verdict. When you do: apply that verdict, do not +start another experiment, run `autor3search-python report`, summarize what you +tried, and exit the loop. +``` -If you are a coding agent that has just been pointed at this README, this is -everything you need: +That's the whole handoff. The agent installs the tool, discovers your +benchmarks, freezes a baseline, and then follows `program.md` — generated for +your repository by `init` — which tells it how to run the keep-or-discard loop. +`program.md` names the benchmarks in scope, spells out the KEEP/DISCARD/FAIL/ +CRASH contract, lists everything the agent must never touch, and ends with a +bank of generic Python performance ideas for when the agent is out of +hypotheses. -```bash -# 1. Install the harness. -uv tool install autor3search-python # or: pipx install autor3search-python +What you get back: one commit per accepted change on a branch named +`autor3search-python/`, and a `results.tsv` recording every experiment +that was tried, including the ones that failed. `autor3search-python report` +summarizes it. -# 2. From the repository you want to optimize: -autor3search-python init # discovers benchmarks, writes config + program.md -git add -A && git commit -m "autor3search-python init" -autor3search-python doctor # is this machine fit to measure? (informational) -autor3search-python baseline -tag +Two things worth knowing before you start it: -# 3. Read program.md. It is the complete instruction set — the loop, the exit -# codes, the JSON contract, what you may and may not edit. Follow it exactly. -``` +- **It needs benchmarks.** This optimizes what it can measure, and refuses to + guess: `init` looks for pytest-benchmark benchmarks and tells you plainly + when it finds none. +- **Numbers are only as good as the machine.** Run `doctor` and read it. A + thermally throttled laptop on battery produces noise dressed as data. Everything past this point is for the human setting the run up, or for -understanding what the agent in step 3 is actually bound by. +understanding what the agent in step 6 is actually bound by. ## The idea @@ -344,17 +377,22 @@ them should not be over-read: ## Limitations -**POSIX only (Linux, macOS); Windows is not supported and not tested.** See -the note under [Start here](#start-here) for what specifically breaks — -`doctor` names the same three gaps as a FAIL when run on a non-POSIX -platform. `eval` refuses to start there at all, because the whole point of -this tool is a number you can stand behind and it cannot produce a -trustworthy one on a platform none of its safety mechanisms have ever run -on. Set `AUTOR3SEARCH_PYTHON_ALLOW_UNSUPPORTED_PLATFORM=1` to run it anyway, -having read the three gaps above. `stop --force` also refuses to signal a -running eval on such a platform (it would otherwise crash trying), and -prints instead: the graceful stop request is still written, and you should -interrupt the agent yourself. +**Windows works, with one difference worth knowing.** CI runs the full suite on +`windows-latest` alongside Linux and macOS. The run claim is a real lock there +(`msvcrt`, not the unconditional success it used to report), and a job object +gives `eval` the killable process tree a process group gives it elsewhere — so +a benchmark that hits its timeout takes its pytest subprocess and every +grandchild with it, and `stop -force` reaches the benchmark rather than +orphaning it. The difference: `stop -force` there is **immediate rather than a +request**. Windows offers no signal a benchmark can act on mid-round, so the +eval is ended rather than asked, and it does not get to record what it +abandoned. Plain `stop` is unaffected and behaves identically everywhere. +`doctor` also has no load average or CPU governor to read there, so it warns +you about less. One residual race is real: a grandchild started in the +microseconds between spawning a benchmark subprocess and putting it in its job +object is outside that job and would survive a timeout kill. Windows offers no +way to create a process directly into a job through `subprocess`, and the Go +original has the same window. Ported from the Go original: From 70cc80ce9d33c23afa754526a9ae81509b656b26 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 7 Sep 2026 18:25:58 +0300 Subject: [PATCH 4/5] chore: declare Windows in the package metadata The classifiers listed POSIX and macOS only, which is the same claim the README used to make in prose. A package that says it works on Windows in its README and not in its metadata is telling pip something different from what it tells the reader. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EDNWUNHzZaAPxM6yhX7riC --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 511f612..d47574f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -18,6 +18,7 @@ classifiers = [ "Operating System :: POSIX", "Operating System :: POSIX :: Linux", "Operating System :: MacOS :: MacOS X", + "Operating System :: Microsoft :: Windows", ] [project.scripts] From 62db19396f6f993896b14a4ac8a6133f11864aa5 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 7 Sep 2026 18:35:44 +0300 Subject: [PATCH 5/5] fix: the Windows bugs the Windows CI job found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four production defects, none of them visible from a POSIX machine. The compile gate compiled nothing. compileall reports what it walks with the platform's own separator, so the "." this gate passes came back as ".\bad.py" -- and the exclude regex's `\.[^/]` dotfile branch read that backslash as "not a slash" and excluded the entire tree. The gate exited 0 on a syntax error; the pytest gate caught it a stage later as a collection FAIL where it should have been a compile CRASH. Both separators now, in every branch of the pattern. The claim lock locked the pid text. Windows locks are mandatory, not advisory: a locked range cannot be read by another handle either, so eval_running and every refused claim -- the paths whose whole job is to say which pid holds the run -- failed with a permission error instead of an answer. The lock moves to a fixed byte past EOF, which is just as exclusive and blocks nothing anyone reads. git output was decoded with the platform locale (cp1252 on a default Windows install), so a repository holding a café.py reported a caf?.py that matches nothing on disk to every gate that reads a diff. git speaks UTF-8; the calls now say so. gitx.root also normalizes git's POSIX separators to the platform's own form, since doctor prints it and the run's state directory is keyed on the repository path. profile's site paths render with one separator everywhere -- the elision that shortens a long path cuts on "/", and a native "pkg\mod.py" gave it no boundary to cut on. The remaining six were tests encoding POSIX assumptions: a grandchild script that put C:\Users into a non-raw string literal and died of a SyntaxError rather than of the timeout it was there to test, PYTHONPATH split on ":" instead of os.pathsep, an unguarded `import resource`, two interpreter paths written into TOML without escaping, and a -force test that asserted the POSIX mechanism by name. test_winjob's kill-on-close check now measures that the process died early rather than trusting an exit code a killed process does not have to set. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EDNWUNHzZaAPxM6yhX7riC --- src/autor3search_python/gitx.py | 15 ++++++++++++- src/autor3search_python/profile.py | 6 +++++- src/autor3search_python/runner.py | 11 ++++++++-- src/autor3search_python/runstop.py | 30 ++++++++++++++++++-------- tests/test_cli_stop.py | 5 +++++ tests/test_config.py | 6 +++++- tests/test_doctor.py | 5 ++++- tests/test_gitx.py | 31 +++++++++++++++++++++++++++ tests/test_runner.py | 34 ++++++++++++++++++++++++++---- tests/test_runstop.py | 17 +++++++++++++++ tests/test_winjob.py | 8 +++++-- 11 files changed, 147 insertions(+), 21 deletions(-) diff --git a/src/autor3search_python/gitx.py b/src/autor3search_python/gitx.py index b1f60ff..9910cc7 100644 --- a/src/autor3search_python/gitx.py +++ b/src/autor3search_python/gitx.py @@ -19,6 +19,12 @@ def _git(d: str | Path, *args: str) -> str: otherwise successful command — lfs filter warnings, advice hints, a user's own hooks — never gets parsed as part of the result. stderr appears in the error message only when the command fails. + + The encoding is named rather than left to the locale, which is what + `text=True` alone would use: git speaks UTF-8 on every platform, but a + default Windows install decodes with cp1252, and a repository holding a + `café.py` then reported a `caf?.py` that matches nothing on disk to every + gate that reads a diff. """ try: proc = subprocess.run( @@ -26,6 +32,7 @@ def _git(d: str | Path, *args: str) -> str: cwd=str(d), capture_output=True, text=True, + encoding="utf-8", timeout=_TIMEOUT, ) except OSError as e: @@ -38,7 +45,13 @@ def _git(d: str | Path, *args: str) -> str: def root(d: str | Path) -> str: - return _git(d, "rev-parse", "--show-toplevel") + """The repository root, in this platform's own path form. + + git answers with POSIX separators everywhere, Windows included. Callers + that wrap this in Path() do not care, but `doctor` prints it and the run's + state directory is keyed on the repository path — one form, not two. + """ + return str(Path(_git(d, "rev-parse", "--show-toplevel"))) def head_commit(d: str | Path) -> str: diff --git a/src/autor3search_python/profile.py b/src/autor3search_python/profile.py index 6b1b36b..3901fb4 100644 --- a/src/autor3search_python/profile.py +++ b/src/autor3search_python/profile.py @@ -67,7 +67,11 @@ def _display_site(file: str, line: int, root: Path | None) -> str: display = file if root is not None: try: - display = str(Path(file).resolve().relative_to(root)) + # One separator in the report, on every platform: `_elide` cuts on + # "/" when a site is too long, and a Windows-native "pkg\mod.py" + # has no such boundary to cut on — it would be truncated + # mid-component instead of at a directory. + display = Path(file).resolve().relative_to(root).as_posix() except ValueError: display = file return _elide(f"{display}:{line}", _SITE_WIDTH) diff --git a/src/autor3search_python/runner.py b/src/autor3search_python/runner.py index fc9d804..45a684c 100644 --- a/src/autor3search_python/runner.py +++ b/src/autor3search_python/runner.py @@ -82,13 +82,20 @@ def _compile_exclude() -> str: experiment on a syntax error in a directory no one was measuring. compileall matches this with `search` against each path it walks. The extra - `[^/]` in the dotfile branch matters when a caller passes "." (this + `[^/\\]` in the dotfile branch matters when a caller passes "." (this module's own test does): compileall then reports files as "./bad.py", and a bare `(^|/)\\.` matches that leading "./" itself, excluding every file in the tree instead of just dotfiles and dotdirs. + + Both separators, because compileall reports what it walks in the platform's + own form: on Windows that same call yields ".\\bad.py", where a `/`-only + character class reads the backslash as "any character but /" and excludes + the entire tree. That is not a cosmetic difference — the compile gate then + compiles nothing and passes, so a syntax error reached the pytest gate as a + collection error (FAIL) instead of failing here as a CRASH. """ names = "|".join(re.escape(d) for d in sorted(discover.SKIP_DIRS)) - return rf"(^|/)(\.[^/]|({names})(/|$))" + return rf"(^|[/\\])(\.[^/\\]|({names})([/\\]|$))" def validate_node_ids(node_ids: Sequence[str]) -> None: diff --git a/src/autor3search_python/runstop.py b/src/autor3search_python/runstop.py index 0c0feac..cce862d 100644 --- a/src/autor3search_python/runstop.py +++ b/src/autor3search_python/runstop.py @@ -20,6 +20,11 @@ _POSIX = os.name == "posix" +# The byte the Windows claim locks: past the end of the pid file, never inside +# it. See _try_lock for why locking the data itself was a bug rather than a +# detail. Any fixed offset beyond the file works; this one is far past a pid. +_LOCK_OFFSET = 1 << 20 + class StopError(Exception): """A stop sentinel or eval claim that cannot be read or taken.""" @@ -60,10 +65,18 @@ def _try_lock(fd: int) -> bool: Both platforms lock the file itself rather than trusting its existence: a pid file left behind by an eval that was killed must read as free, and a pid file whose holder is alive must read as taken, and only the kernel - knows which is which. Windows locks a one-byte range from the current file - position, so every call here seeks to 0 first — a lock taken at whatever - offset the last write left behind would be a different lock each time, and - two evals would both believe they had it. + knows which is which. + + Windows locks a one-byte range from the current file position, so every + call here seeks to the same offset first — a lock taken at whatever offset + the last write left behind would be a different lock each time, and two + evals would both believe they had it. That offset is deliberately past the + end of the file, not byte 0: a Windows lock is mandatory rather than + advisory, and a locked range cannot be READ by another handle either, so + locking the pid text made `eval_running` and every refused claim fail with + a permission error in place of the answer they exist to give. A byte past + EOF locks nothing anyone needs to read; a lock there is legal and is + exactly as exclusive. """ if _POSIX: import fcntl @@ -76,7 +89,7 @@ def _try_lock(fd: int) -> bool: import msvcrt - os.lseek(fd, 0, os.SEEK_SET) + os.lseek(fd, _LOCK_OFFSET, os.SEEK_SET) try: msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) except OSError: @@ -93,7 +106,7 @@ def _unlock(fd: int) -> None: import msvcrt - os.lseek(fd, 0, os.SEEK_SET) + os.lseek(fd, _LOCK_OFFSET, os.SEEK_SET) with contextlib.suppress(OSError): msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) @@ -138,9 +151,8 @@ def claim_eval(state_dir: str | Path, pid: int) -> Iterator[None]: locked = True os.ftruncate(fd, 0) # lseek + write rather than pwrite: os.pwrite does not exist on - # Windows. The seek is not incidental — _try_lock and _unlock both - # lock the byte at offset 0, so the position must be left where they - # expect it either way. + # Windows. _try_lock and _unlock each seek to _LOCK_OFFSET themselves, + # so this seek only has to put the pid at the start of the file. os.lseek(fd, 0, os.SEEK_SET) os.write(fd, f"{pid}\n".encode()) yield diff --git a/tests/test_cli_stop.py b/tests/test_cli_stop.py index 07aaa7d..8752e5f 100644 --- a/tests/test_cli_stop.py +++ b/tests/test_cli_stop.py @@ -61,9 +61,14 @@ def test_force_survives_a_corrupt_pid_file(started, capsys, pid_contents): def test_force_signals_the_running_eval(started, monkeypatch, capsys): + """Whichever mechanism this platform uses, -force must reach the eval: + its process group on POSIX, the process itself where there is no signal + it could act on. Both are patched so the assertion is about the pid + reaching one of them, not about which platform is running the test.""" monkeypatch.setattr(runstop, "eval_running", lambda d: (4242, True)) signalled = [] monkeypatch.setattr(cli_stop, "_signal_group", lambda pid: signalled.append(pid)) + monkeypatch.setattr(cli_stop, "_terminate", lambda pid: signalled.append(pid)) cli_main.main(["stop", "-C", str(started), "-force"]) assert signalled == [4242] diff --git a/tests/test_config.py b/tests/test_config.py index a6be735..91bae20 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,3 +1,4 @@ +import json import shutil import sys @@ -81,7 +82,10 @@ def test_a_benchmark_entry_with_an_internal_dash_is_not_rejected(): def test_python_accepts_the_running_interpreter(tmp_path): - config.load(write(tmp_path, f'python = "{sys.executable}"\n')) # must not raise + # json.dumps, not an f-string: a TOML basic string escapes backslashes the + # same way JSON does, and C:\Users\... interpolated raw is a parse error + # rather than a path. + config.load(write(tmp_path, f"python = {json.dumps(sys.executable)}\n")) # must not raise def test_python_accepts_a_bare_name_resolvable_on_path(tmp_path): diff --git a/tests/test_doctor.py b/tests/test_doctor.py index d750214..80943c0 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -1,3 +1,4 @@ +import json from pathlib import Path import pytest @@ -306,7 +307,9 @@ def test_doctor_uses_the_configured_interpreter(git_repo, capsys): fake_python = git_repo / "fake-python" fake_python.write_text("#!/bin/sh\necho 'not really python' 1>&2\nexit 1\n") fake_python.chmod(0o755) - (cfg_dir / "config.toml").write_text(f'python = "{fake_python}"\n') + # json.dumps: a TOML basic string needs its backslashes escaped, and a + # Windows path written raw is a parse error rather than an interpreter. + (cfg_dir / "config.toml").write_text(f"python = {json.dumps(str(fake_python))}\n") assert config.load(cfg_dir / "config.toml").python == str(fake_python) # sanity cli_main.main(["doctor", "-C", str(git_repo)]) diff --git a/tests/test_gitx.py b/tests/test_gitx.py index 56a388f..0d4fbc1 100644 --- a/tests/test_gitx.py +++ b/tests/test_gitx.py @@ -1,3 +1,6 @@ +import subprocess +from pathlib import Path + import pytest from autor3search_python import gitx @@ -93,3 +96,31 @@ def test_path_in_tree_reads_the_commit_not_the_worktree(git_repo): assert gitx.path_in_tree(git_repo, "HEAD", "pytest.ini") is False git(git_repo, "add", "-f", "pytest.ini") assert gitx.path_in_tree(git_repo, "HEAD", "pytest.ini") is False + + +def test_git_output_is_decoded_as_utf8_not_the_platform_locale(monkeypatch, git_repo): + """text=True alone decodes with the locale's encoding, which is cp1252 on + a default Windows install: a path like `café.py` came back as `caf?.py`, + and a gate that compares those names against the ones on disk would then + be reasoning about a file that does not exist. git speaks UTF-8; say so. + """ + seen = {} + real_run = subprocess.run + + def capture(*args, **kwargs): + seen.update(kwargs) + return real_run(*args, **kwargs) + + monkeypatch.setattr(gitx.subprocess, "run", capture) + gitx.head_commit(git_repo) + assert seen.get("encoding") == "utf-8" + + +def test_root_is_returned_in_the_platforms_own_path_form(monkeypatch, git_repo): + """git prints POSIX separators everywhere, Windows included, so its + `rev-parse --show-toplevel` answer is `C:/Users/...` there. Every caller + wraps this in Path() and does not care, but `doctor` prints it verbatim, + and a run's state directory is keyed on the repository path — one form, + not two.""" + monkeypatch.setattr(gitx, "_git", lambda d, *a: "C:/Users/x/repo") + assert gitx.root(git_repo) == str(Path("C:/Users/x/repo")) diff --git a/tests/test_runner.py b/tests/test_runner.py index c5cce66..7367371 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -1,3 +1,5 @@ +import os +import re import sys import time @@ -44,10 +46,16 @@ def test_timeout_kills_grandchildren(tmp_path): """go test runs its benchmark as a grandchild; pytest does too. A survivor burns CPU and corrupts every later measurement on the machine.""" marker = tmp_path / "alive.txt" + # marker.as_posix(): a Windows path interpolated raw would put "\\U" of + # C:\\Users into the inner script's own non-raw string literal, and the + # child would die of a SyntaxError instead of outliving anything — a test + # that passes for the wrong reason. Python opens forward-slash paths on + # Windows too. script = ( "import subprocess, sys, time\n" f"subprocess.Popen([sys.executable, '-c', " - f"\"import time, pathlib; time.sleep(6); pathlib.Path(r'{marker}').write_text('x')\"])\n" + f'"import time, pathlib; time.sleep(6); ' + f"pathlib.Path(r'{marker.as_posix()}').write_text('x')\"])\n" "time.sleep(30)\n" ) res = runner.Runner(tmp_path, 1).run(sys.executable, "-c", script) @@ -105,7 +113,9 @@ def test_harness_memory_stays_bounded_for_a_very_chatty_child(tmp_path): timed. Reading in bounded chunks and discarding past CAP_BYTES keeps this process's own memory close to constant regardless of how chatty the child is.""" - import resource + resource = pytest.importorskip( + "resource", reason="peak RSS is measured through a POSIX-only module" + ) def peak_rss_kb() -> int: # ru_maxrss is KB on Linux, bytes on macOS. @@ -154,7 +164,7 @@ def test_bench_env_pins_hashseed_and_pythonpath(tmp_path): (tmp_path / "src" / "pkg" / "__init__.py").write_text("") env = runner.bench_env(tmp_path, config.default(), base_env={"PATH": "/bin"}) assert env["PYTHONHASHSEED"] == "0" - parts = env["PYTHONPATH"].split(":") + parts = env["PYTHONPATH"].split(os.pathsep) assert str(tmp_path) in parts assert str(tmp_path / "src") in parts @@ -167,7 +177,7 @@ def test_bench_env_omits_hashseed_when_disabled(tmp_path): def test_bench_env_prepends_configured_pythonpath(tmp_path): cfg = config.default().__class__(pythonpath=("lib",)) env = runner.bench_env(tmp_path, cfg, base_env={}) - assert env["PYTHONPATH"].split(":")[0] == str(tmp_path / "lib") + assert env["PYTHONPATH"].split(os.pathsep)[0] == str(tmp_path / "lib") def test_compile_gate_fails_on_a_syntax_error(tmp_path): @@ -445,3 +455,19 @@ def test_a_timeout_still_kills_the_child_when_no_job_could_be_created(monkeypatc events: list[str] = [] runner._kill_tree(_FakeProc(events), None) assert events == ["kill"] + + +def test_compile_exclude_does_not_swallow_a_windows_relative_path(): + """The gate compiles "." and compileall reports what it walks with the + platform's own separator, so on Windows every file arrives as ".\\name.py". + A `\\.[^/]` dotfile branch matches that leading ".\\" — excluding the whole + tree, which is how a syntax error walked through a green compile gate on + Windows while the same code failed correctly on Linux. + """ + pattern = runner._compile_exclude() + assert re.search(pattern, r".\bad.py") is None + assert re.search(pattern, "./bad.py") is None + # Still excludes what it is for, whichever separator the platform uses. + assert re.search(pattern, r".\.hidden.py") is not None + assert re.search(pattern, r"build\stale.py") is not None + assert re.search(pattern, "build/stale.py") is not None diff --git a/tests/test_runstop.py b/tests/test_runstop.py index 77f63c0..b62f72f 100644 --- a/tests/test_runstop.py +++ b/tests/test_runstop.py @@ -144,10 +144,12 @@ class _FakeMsvcrt: def __init__(self, *, refuse: bool = False) -> None: self.calls: list[tuple[int, int]] = [] + self.offsets: list[int] = [] self.refuse = refuse def locking(self, fd: int, mode: int, nbytes: int) -> None: self.calls.append((mode, nbytes)) + self.offsets.append(os.lseek(fd, 0, os.SEEK_CUR)) if self.refuse and mode == self.LK_NBLCK: raise OSError(13, "another process has locked a portion of the file") @@ -208,3 +210,18 @@ def test_windows_release_unlocks_and_closes_before_deleting(tmp_path, fake_windo pass assert order == ["unlock", "close", "unlink"] assert not (d / runstop.EVAL_PID_FILE).exists() + + +def test_windows_locks_a_byte_nothing_needs_to_read(tmp_path, fake_windows): + """Windows file locks are mandatory, not advisory: the locked range cannot + be READ by another handle either. Locking byte 0 therefore locked the pid + text itself, and every eval_running and every refused claim — the paths + that exist to report which pid holds the run — failed with a permission + error instead of an answer. The lock has to sit past anything the file + contains, so the bytes stay readable while the claim is held. + """ + fake = fake_windows() + d = tmp_path / "state" + with runstop.claim_eval(d, 4242): + assert (d / runstop.EVAL_PID_FILE).stat().st_size < runstop._LOCK_OFFSET + assert fake.offsets and set(fake.offsets) == {runstop._LOCK_OFFSET} diff --git a/tests/test_winjob.py b/tests/test_winjob.py index 72ad463..d3dbda4 100644 --- a/tests/test_winjob.py +++ b/tests/test_winjob.py @@ -60,12 +60,16 @@ def test_closing_a_job_kills_what_is_still_in_it(): """`stop --force` relies on this: terminating eval drops the last handle to every job it holds, and kill-on-close takes the benchmark tree with it. """ - proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(5)"]) + proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) try: job = winjob.assign(proc.pid) assert job is not None + started = time.monotonic() job.close() - assert proc.wait(timeout=10) != 0 + proc.wait(timeout=15) + # The exit code is not the evidence — a killed process can report 0. + # Dying decades before its own sleep would have ended is. + assert time.monotonic() - started < 10 finally: if proc.poll() is None: # pragma: no cover - only if kill-on-close failed proc.kill()