bench: put the cold reset in a script pixi's task shell cannot re-parse

.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

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: <reason>` 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

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") <owner/repo[@branch]>" >&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

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: <reason>`, 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 @@
     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 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]

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

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: