diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index b1c9ec86..6800de8c 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -137,27 +137,25 @@ jobs: # on purpose, which is what makes this "recreate" rather than "first # ever". # - # THE CHOWN IS WHAT MAKES THE REMOVE POSSIBLE, on a runner. A launch - # leaves its clone owned by the container's user -- uid 1000 in the - # standard devcontainer base image -- and a GitHub runner's own user is - # not that uid, which is the "when part of the cache will not go" case - # README.md describes. `dl rm` then deletes the workspace, warns that - # the clone would not go, and exits 0; the next launch dies writing - # `.git/index.lock` into a clone it does not own, and the shape reports - # a failed run instead of a time (run 31838698495). Nothing about `dl` - # is wrong there -- it cannot unlink what it does not own -- so the - # recovery belongs to the environment that creates the mismatch, and - # passwordless sudo is a property of GitHub-hosted runners. This step is - # the only place in the repo that assumes it. + # The reset is `scripts/bench_cold_reset.sh`, which takes the clone back + # from the container's uid before removing it and says there why that + # recovery belongs to a runner rather than to `dl` (run 31838698495). # - # It sits INSIDE `--before`, which is the only place with the right - # cardinality: every run recreates those files, so a one-time step - # before the bench would recover run 1 and hand runs 2..5 exactly the - # clone that broke it. `test -d` because a reset also runs before the - # first launch, when there may be nothing there yet. + # It is a FILE rather than a string here because the string did not + # survive the trip: `--before` reaches the bench through + # `pixi run bench`, whose task shell re-joins and re-parses a task's + # appended arguments, so the quotes nested inside this step's own quotes + # were gone by the time argparse saw them. That run exited 2 on the + # wreckage, pixi's shell ran the tail after the `;` as a command of its + # own, and the STEP passed with no record written (run 31840842480). + # One level of quoting has nothing left to lose. + # + # It stays INSIDE `--before`, the only place with the right cardinality: + # every run recreates those files, so a one-time step before the bench + # would recover run 1 and hand runs 2..5 exactly the clone that broke it. run: | pixi run bench -n 5 --record cold-recreate.json --shape cold-recreate \ - --before "bash -c 'test -d $XDG_CACHE_HOME/devlaunch/repos && sudo chown -R $(id -u):$(id -g) $XDG_CACHE_HOME/devlaunch/repos; dl $TARGET rm --force'" \ + --before "scripts/bench_cold_reset.sh $TARGET" \ -- dl "$TARGET" -- true - name: Turn the records into this commit's trend points diff --git a/README.md b/README.md index 698b7468..bcc8e572 100644 --- a/README.md +++ b/README.md @@ -959,7 +959,7 @@ python3 scripts/bench_points.py warm.json cold-recreate.json --out bench.json \ (`pixi run bench-points ...` in the devcontainer.) One case per stage the shape reported, plus that shape's own total: `warm / host-prep`, -`cold-recreate / devpod-up`, `warm / total`. Five properties of it are +`cold-recreate / devpod-up`, `warm / total`. Six properties of it are load-bearing: - **The published value is the median, and the case name is a key.** The trend @@ -980,6 +980,10 @@ load-bearing: lost one publishes nothing and goes red. Naming a shape that was not benched fails too — an assertion that covers nothing reads exactly like one that passed. +- **A record that is missing or unreadable refuses the same way.** The step + that writes the records can exit 0 without having written one, so the absence + arrives here — and it prints `bench_points: ` naming the file and + writes nothing, like every other refusal, rather than a traceback. - **A regression alerts; it never gates.** The workflow is deliberately not a job in `ci.yml`: a job there would join the CI gate's `needs` by house convention and turn a noisy wall-clock measurement into a merge gate. A point diff --git a/scripts/bench_cold_reset.sh b/scripts/bench_cold_reset.sh new file mode 100755 index 00000000..295489ee --- /dev/null +++ b/scripts/bench_cold_reset.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# Re-establish the cold-recreate shape's starting state, once per benched run. +# +# `.github/workflows/bench.yml` passes this as the bench's `--before`, which is +# the only place with the right cardinality: every run recreates the files this +# recovers, so a one-time step before the bench would fix run 1 and hand runs +# 2..5 exactly the clone that broke it. +# +# THE CHOWN IS WHAT MAKES THE REMOVE POSSIBLE, on a runner. A launch leaves its +# clone owned by the container's user -- uid 1000 in the standard devcontainer +# base image -- and a GitHub runner's own user is not that uid, which is the +# "when part of the cache will not go" case README.md describes. `dl rm` then +# deletes the workspace, warns that the clone would not go, and exits 0; the +# next launch dies writing `.git/index.lock` into a clone it does not own, and +# the shape reports a failed run instead of a time (run 31838698495). Nothing +# about `dl` is wrong there -- it cannot unlink what it does not own -- so the +# recovery belongs to the environment that creates the mismatch, and +# passwordless sudo is a property of GitHub-hosted runners. This script is the +# only place in the repo that assumes it. +# +# It is a FILE rather than a string in the step because the string did not +# survive the trip: `--before` reaches the bench through `pixi run bench`, whose +# task shell re-joins and re-parses the arguments appended to a task, and the +# quotes nested inside the step's own quotes were gone by the time argparse saw +# them (run 31840842480). A path plus one argument has no second quoting level +# to lose. +set -eu + +if [ "$#" -ne 1 ]; then + echo "usage: $(basename "$0") " >&2 + exit 2 +fi + +# `test -d` because a reset also runs before the first launch, when there may +# be nothing there yet. Scoped to the job's own clone cache: a recursive chown +# is a blunt instrument, and a wider one would take ownership of things this +# job did not create. +repos="${XDG_CACHE_HOME:?the bench scopes this cache away from the one a developer launches into}/devlaunch/repos" +if [ -d "$repos" ]; then + sudo chown -R "$(id -u):$(id -g)" "$repos" +fi + +exec dl "$1" rm --force diff --git a/scripts/bench_points.py b/scripts/bench_points.py index 43c681c3..5d289c18 100755 --- a/scripts/bench_points.py +++ b/scripts/bench_points.py @@ -44,6 +44,11 @@ changed shape, and a red job says so where a flattening trend line does not. Naming a shape that was not benched is itself a failure, for the same reason: an assertion that covers nothing reads like one that passed. + +A record that is missing, unreadable or not a bench record takes the same +exit: `bench_points: `, and no output file. The step that writes the +records can go green without having written one, so that absence is the first +thing this reads and the first thing it can report. """ UNIT = "s" @@ -129,6 +134,45 @@ def shape_of(record: Dict[str, Any]) -> str: return str(shape) +def load_record(path: str) -> Dict[str, Any]: + """The bench record at *path*, or a refusal saying which one and why. + + Reading is a refusal like every other one here, and it earns that the same + way: the bench step upstream can go green without writing its record at all + -- a mangled `--before` exited the bench 2 and the step passed anyway (run + 31840842480) -- and what this script printed then was a raw traceback whose + first readable line named a Python builtin rather than the absent file. A + missing record is a fact about the step before this one, so it is reported + like one instead of raised through it. + """ + try: + with open(path, encoding="utf-8") as handle: + document = json.load(handle) + except OSError as unreadable: + raise Refused( + f"cannot read the record {path}: {unreadable.strerror}. The bench step that " + "writes it can exit 0 without having written it, so this is a report about " + "that step rather than about this one" + ) from unreadable + except ValueError as unparsable: + raise Refused( + f"the record {path} is not readable JSON ({unparsable}); a bench interrupted " + "mid-write leaves exactly this" + ) from unparsable + if not isinstance(document, dict): + raise Refused( + f"the record {path} is a {type(document).__name__}, not the one JSON object " + "per bench invocation this reads" + ) + missing = [key for key in ("median", "runs") if key not in document] + if missing: + raise Refused( + f"the record {path} carries no {' and no '.join(missing)}, which is what its " + "points are made of -- so there is no point in it to publish" + ) + return document + + def check_one_record_per_shape(records: List[Dict[str, Any]]) -> None: """Refuse two records of the same shape. @@ -246,11 +290,8 @@ def build_parser() -> argparse.ArgumentParser: def main(argv=None) -> int: parser = build_parser() args = parser.parse_args(argv) - records = [] - for path in args.records: - with open(path, encoding="utf-8") as handle: - records.append(json.load(handle)) try: + records = [load_record(path) for path in args.records] check_one_record_per_shape(records) check_required( records, args.require_stages_on, [s for s in args.require_stages.split(",") if s] diff --git a/test/test_bench_points.py b/test/test_bench_points.py index 07a991f0..2b475f0e 100644 --- a/test/test_bench_points.py +++ b/test/test_bench_points.py @@ -326,6 +326,50 @@ def test_a_record_with_no_shape_cannot_name_a_trend_line(self, tmp_path): assert "Traceback" not in result.stderr assert not out.exists() + def test_a_record_that_was_never_written_names_the_file_it_wanted(self, tmp_path): + """The step before this one can pass without writing its record — run + 31840842480 is a bench whose arguments were mangled, whose script + exited 2, and whose step went green anyway. This converter is where + that surfaces, so it has to surface as a sentence naming the missing + record, like every other refusal, rather than as a traceback.""" + out = tmp_path / "bench.json" + result = run_points(str(tmp_path / "cold-recreate.json"), "--out", str(out)) + assert result.returncode != 0 + assert "cold-recreate.json" in result.stderr + assert "Traceback" not in result.stderr + assert result.stderr.startswith("bench_points:") + assert not out.exists() + + def test_a_record_that_is_not_readable_json_is_refused(self, tmp_path): + """A truncated record is what an interrupted bench leaves behind, and + it is an absence like any other.""" + record = tmp_path / "warm.json" + record.write_text('{"shape": "warm", "runs": [', encoding="utf-8") + out = tmp_path / "bench.json" + result = run_points(str(record), "--out", str(out)) + assert result.returncode != 0 + assert "warm.json" in result.stderr + assert "Traceback" not in result.stderr + assert result.stderr.startswith("bench_points:") + assert not out.exists() + + def test_a_record_without_the_runs_and_median_it_publishes_is_refused(self, tmp_path): + """Readable JSON is not yet a bench record. These two keys are what + every point is made of, so a document missing either has nothing to + publish and says so once — rather than raising a KeyError further in, + at a line that names neither the file nor the reason.""" + for missing in ("median", "runs"): + record = write_record( + tmp_path / f"{missing}.json", {k: v for k, v in WARM.items() if k != missing} + ) + out = tmp_path / f"{missing}-bench.json" + result = run_points(record, "--out", str(out)) + assert result.returncode != 0, missing + assert missing in result.stderr + assert "Traceback" not in result.stderr + assert result.stderr.startswith("bench_points:") + assert not out.exists() + def test_two_records_of_the_same_shape_would_collide(self, tmp_path): """The case name is the trend's key. Two records both labelled `warm` publish two points per case in one commit, and the chart has no way to diff --git a/test/test_bench_workflow.py b/test/test_bench_workflow.py index c56b0cdf..873659f9 100644 --- a/test/test_bench_workflow.py +++ b/test/test_bench_workflow.py @@ -14,12 +14,14 @@ written. """ +import os from pathlib import Path WORKFLOWS = Path(__file__).parent.parent / ".github" / "workflows" BENCH = WORKFLOWS / "bench.yml" CI = WORKFLOWS / "ci.yml" BENCH_JOB = "bench" +RESET = Path(__file__).parent.parent / "scripts" / "bench_cold_reset.sh" def bench_workflow() -> str: @@ -51,6 +53,17 @@ def cold_reset() -> str: return lines[0] +def reset_script() -> str: + """What the cold reset actually does, which is a file rather than a string. + + The step names a script and the script holds the reset; both halves are + asserted below, because a reset that is named but does nothing and a reset + that does the right thing but is never named fail identically in CI. + """ + assert RESET.is_file(), f"{RESET.name} is what re-establishes the cold state" + return RESET.read_text(encoding="utf-8") + + class TestItCannotBecomeAMergeGate: """Three independent ways this could turn into a required check, and the reason each stays shut.""" @@ -125,10 +138,30 @@ class TestTheColdResetSurvivesAContainerWrittenClone: in a clone it does not own (run 31838698495, follow-up to #198). The runner has passwordless sudo, so the reset takes the clone back before removing it. This is a property of a CI runner, not of `dl`, which is why the - recovery lives here.""" + recovery lives here — in a script, for the reason the first test gives.""" + + def test_the_reset_is_a_script_the_step_names_rather_than_quoting_inside_quoting(self): + """`--before` reaches the bench through `pixi run bench`, and pixi's + task shell re-joins and re-parses the arguments appended to a task — + so a `bash -c '...'` nested inside the step's own quotes arrives with + its inner quotes gone. Run 31840842480 is what that costs: argparse + saw `-d` as a stray argument and exited 2, pixi's shell ran the tail + after the `;` as a command of its own, and the STEP still passed with + no record written. A bare path plus one argument has nothing left to + re-parse.""" + reset = cold_reset() + assert RESET.name in reset + assert "bash -c" not in reset, "a nested shell is the quoting that did not survive" + assert "'" not in reset, "a second level of quotes is what pixi's task shell eats" + assert ";" not in reset, "pixi's shell reads a `;` in the argument as its own separator" + + def test_the_script_is_executable_so_the_bench_can_run_it_directly(self): + """The bench splits `--before` and runs the list without a shell, so + the file has to be runnable on its own.""" + assert os.access(RESET, os.X_OK), f"{RESET.name} is run as a command, not sourced" def test_the_reset_takes_the_clone_back_before_removing_it(self): - reset = cold_reset() + reset = reset_script() assert "sudo chown" in reset assert "rm --force" in reset assert reset.index("sudo chown") < reset.index("rm --force"), ( @@ -139,15 +172,23 @@ def test_the_reset_only_reclaims_this_jobs_own_clone_cache(self): """Scoped to the cache the job already scoped to `/tmp`. A recursive chown is a blunt instrument and a wider one would take ownership of things this job did not create.""" - assert "XDG_CACHE_HOME" in cold_reset() + assert "XDG_CACHE_HOME" in reset_script() + + def test_the_script_says_why_it_exists_where_somebody_would_delete_it(self): + """A three-line script that chowns a cache reads like leftover + scaffolding unless the uid mismatch that forces it is written down + beside it.""" + assert "uid" in reset_script() def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self): """Every run recreates those files, so a one-time step before the bench recovers run 1 and leaves runs 2..5 with exactly the clone that broke - it. `--before` is the only place with the right cardinality.""" - assert bench_settings().count("chown") == 1, ( - "a second chown is a one-time step doing per-run work" + it. `--before` is the only place with the right cardinality — so the + script is named once in the workflow, and that once is there.""" + assert bench_settings().count(RESET.name) == 1, ( + "a second mention is a one-time step doing per-run work" ) + assert RESET.name in cold_reset() class TestTheExclusionsSayWhyInTheFile: