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: 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] 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/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 e7f5009..45a684c 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 @@ -81,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: @@ -172,8 +180,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 +203,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 +243,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 +260,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..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.""" @@ -55,23 +60,55 @@ 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 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 + + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + return False return True - import fcntl + import msvcrt + + os.lseek(fd, _LOCK_OFFSET, 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, _LOCK_OFFSET, os.SEEK_SET) + with contextlib.suppress(OSError): + msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) def _read_pid(path: Path) -> tuple[int, bool]: @@ -113,20 +150,36 @@ 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. _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 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_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..8752e5f 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 @@ -59,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] @@ -77,29 +84,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_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 821cbfb..80943c0 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -1,3 +1,4 @@ +import json from pathlib import Path import pytest @@ -57,16 +58,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 +82,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): @@ -307,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 062c307..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) @@ -58,7 +66,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.""" @@ -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): @@ -402,3 +412,62 @@ 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"] + + +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 3581f6f..b62f72f 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,101 @@ 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.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") + + +@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() + + +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 new file mode 100644 index 0000000..d3dbda4 --- /dev/null +++ b/tests/test_winjob.py @@ -0,0 +1,75 @@ +"""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(30)"]) + try: + job = winjob.assign(proc.pid) + assert job is not None + started = time.monotonic() + job.close() + 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()