From 16e904b6a1a1b20519e757dedf8050399cb67cb6 Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 24 Aug 2026 20:48:44 +0000 Subject: [PATCH 1/2] feat(ab): the A/B driver refuses to measure a host nobody declared `ab.py` is the outer harness -- it spans arm A, arm B and the null control -- which makes it the exact process the lock must be held by, and it neither took the lock nor recorded it. The mandate lived in the README, so obeying it meant remembering to wrap the invocation. Remembering is what failed three times in one night: the recorded incidents are a peer sampling `ps` in the gap *between two arms* of somebody's interleaved A/B, a 45% disagreement between two identical binaries, and a 10-502% run-to-run spread that could only be described afterwards as "contention". The gate is ancestry, not liveness, and that is the load-bearing distinction: * a lock anchored to this process or one of its ancestors spans every arm; * a lock anchored to a benchmark *child* is released between arms, so it certifies each arm and protects none of the comparison -- the shape that produced the gap incident, and one that looks identical to any check asking only "is a lock held"; * a lock anchored to anyone else is a reason to stop rather than to start. Refusal is exit 3, before a single arm is launched, and the message carries the wrapping command rather than pointing at a doc. Every unprotected state is distinguishable (`free`, `stale:`, `expired:`, `unusable`, `unknown`), so "nobody has taken it" cannot be confused with "the holder died under it" -- the distinction #1989 added to the tool itself. An unreadable or missing `hostlock.sh` refuses too: fail-closed, because a gate that opens when its instrument breaks is not a gate. Rows now carry the declaration: `host_lock`, `lock_owner`, `lock_anchor_pid`, `runnable_at_start`, `contended`. The label covers the whole window -- the lock is read again at the end and a run that changed hands is stamped `changed` rather than named after whoever held it last -- so a contaminated CSV is self-identifying weeks later instead of depending on someone remembering the night it was taken. `--unlocked` is the escape hatch and it marks what it produces (`unlocked:free`), because the failure mode worth preventing is not an unlocked smoke test, it is an unlocked smoke test whose numbers are quoted later as if they were protected. Seven mutations are each killed by a named cell, including the two that survived the first battery: renaming the `host_lock` column (the assertion matched a substring of the renamed one) and dropping the end-of-window re-read (the logic was inline in `main`, so nothing could reach it -- now `window_label`). The end-to-end cells drive the real `hostlock.sh` against a stub arm that prints a result line and exits, so they cost no measurable CPU and need no quiet host, which is what lets them run in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/hostlock.yml | 12 ++ scripts/ort_ab/README.md | 22 +++ scripts/ort_ab/ab.py | 198 ++++++++++++++++++++++ scripts/ort_ab/test_ab_lock.py | 290 +++++++++++++++++++++++++++++++++ 4 files changed, 522 insertions(+) create mode 100644 scripts/ort_ab/test_ab_lock.py diff --git a/.github/workflows/hostlock.yml b/.github/workflows/hostlock.yml index 4fc931f322..c4e1d6d270 100644 --- a/.github/workflows/hostlock.yml +++ b/.github/workflows/hostlock.yml @@ -80,6 +80,8 @@ on: paths: - "scripts/hostlock.sh" - "scripts/hostlock_test.sh" + - "scripts/ort_ab/ab.py" + - "scripts/ort_ab/test_ab_lock.py" - ".github/workflows/hostlock.yml" push: branches: @@ -87,6 +89,8 @@ on: paths: - "scripts/hostlock.sh" - "scripts/hostlock_test.sh" + - "scripts/ort_ab/ab.py" + - "scripts/ort_ab/test_ab_lock.py" - ".github/workflows/hostlock.yml" permissions: @@ -109,3 +113,11 @@ jobs: # trusted to mean what its name says. - name: Conformance suite run: bash scripts/hostlock_test.sh + + # The admission gate in the A/B driver is lock conformance one layer up: + # it is what makes the outer harness hold the lock across every arm + # rather than each benchmark child holding it between them. Its cells + # drive the real `hostlock.sh` against a stub arm that prints a result + # line and exits, so they cost no measurable CPU and need no quiet host. + - name: A/B driver admission + run: python3 scripts/ort_ab/test_ab_lock.py diff --git a/scripts/ort_ab/README.md b/scripts/ort_ab/README.md index 0e8f883379..4dce6f82f4 100644 --- a/scripts/ort_ab/README.md +++ b/scripts/ort_ab/README.md @@ -241,6 +241,28 @@ the lock is the only statement about the interval, because it is a from people who never took the lock; it is a start admission control and nothing more. +`ab.py` **enforces this rather than documenting it.** Before it launches a +single arm it reads the lock and requires a declaration whose anchor is itself +or one of its ancestors; anything else stops the run with exit 3 and prints the +wrapping command. The ancestry test is what distinguishes the two shapes that +both look like "a lock is held": a lock held by an *ancestor* spans every arm, +while a lock held by a benchmark *child* is released between them, which is the +gap a peer's sweep once started in. A peer's lock stops the run for the +opposite reason — they declared the box. + +```sh +scripts/hostlock.sh run --owner leon --reason "moe mt panel 6-cell" -- \ + python3 scripts/ort_ab/ab.py --arms base=./a mine=./b --null-control ... +``` + +Every CSV row then carries `host_lock`, `lock_owner`, `lock_anchor_pid`, +`runnable_at_start` and `contended`, and the label covers the **whole window**: +the lock is read again at the end, and a run that changed hands halfway through +is stamped `changed` rather than named after whoever happened to hold it last. +`--unlocked` runs anyway and stamps every row `unlocked:` — for smoke +tests, never for anything publishable. `scripts/ort_ab/test_ab_lock.py` covers +the admission table and runs in the `Host lock` workflow. + `SIGKILL` (and a full-box crash) cannot be caught, so it leaves the lock directory behind. Nothing wedges: the lock carries its holder's pid **and** that pid's start time, and the next acquirer reclaims it as soon as that diff --git a/scripts/ort_ab/ab.py b/scripts/ort_ab/ab.py index cf38f02e8d..8e7e461d85 100644 --- a/scripts/ort_ab/ab.py +++ b/scripts/ort_ab/ab.py @@ -32,6 +32,8 @@ from pathlib import Path from statistics import median +HOSTLOCK = Path(__file__).resolve().parents[1] / "hostlock.sh" + RESULT = re.compile( r"native=(?P[\d.]+) ms .*?ort=(?P[\d.]+) ms .*?" r"native/ort=(?P[\d.]+) native_p90=(?P[\d.]+) ort_p90=(?P[\d.]+) " @@ -49,6 +51,168 @@ ) +def parse_provenance(text: str) -> dict[str, str]: + """`hostlock.sh provenance --oneline` into a dict. + + Values cannot contain spaces: the script sanitises owner and reason for + exactly this reason, so splitting on whitespace is the format's contract + rather than an assumption about it. + """ + fields = {} + for token in text.split(): + key, sep, value = token.partition("=") + if sep: + fields[key] = value + return fields + + +def read_provenance(runner=subprocess.run) -> dict[str, str]: + """Asks the lock what it is doing. Empty on any failure. + + An empty reading is fail-closed here: `lock_verdict` refuses anything it + cannot read as a live declaration held by this harness, so a missing or + broken `hostlock.sh` stops the run instead of silently ungating it. + """ + try: + out = runner( + ["bash", str(HOSTLOCK), "provenance", "--oneline"], + capture_output=True, + text=True, + timeout=60, + ) + except Exception: + return {} + return parse_provenance(out.stdout) + + +def parent_of(pid: int) -> int | None: + """`/proc//stat` field 4. + + The comm field is parenthesised and may itself contain spaces and + parentheses, so the split is anchored on the LAST `)` -- a naive + `split()[3]` reads the wrong column for a process whose name has a space + in it, and reads a plausible number rather than failing. + """ + try: + stat = Path(f"/proc/{pid}/stat").read_text() + except OSError: + return None + _, _, rest = stat.rpartition(")") + parts = rest.split() + if len(parts) < 2: + return None + try: + return int(parts[1]) + except ValueError: + return None + + +def ancestry(pid: int, parent=parent_of, limit: int = 64) -> set[int]: + """This process and every ancestor of it, up to init. + + `limit` and the seen-set are not paranoia: pid 1's parent is 0, a + namespaced or reparented process can report a parent that is already in + the chain, and a walk that trusted the chain to terminate would hang the + harness before it ran anything. + """ + chain = {pid} + current = pid + for _ in range(limit): + nxt = parent(current) + if nxt is None or nxt <= 0 or nxt in chain: + break + chain.add(nxt) + current = nxt + return chain + + +# The message is long on purpose: it is read by someone who has just been +# stopped, and the remedy has to be in front of them rather than in a doc. +_REMEDY = """ +Wrap the WHOLE matrix -- every arm, including the null control -- in the lock: + + scripts/hostlock.sh run --owner --reason "" -- \\ + python3 scripts/ort_ab/ab.py + +Wrapping each benchmark child instead leaves the host looking idle in the gap +between two arms, which is how one agent started a sweep in the middle of +another's interleaved A/B. The holder must be the process that spans the arms. + +`--unlocked` runs anyway and stamps every row so the numbers cannot later be +mistaken for protected ones. It is for smoke tests, not for anything you +intend to publish.""" + + +def lock_verdict(prov: dict[str, str], chain: set[int]) -> tuple[str, str | None]: + """The `host_lock=` label for this run, and why it may not proceed. + + Returns `(label, None)` when the run is covered by a declaration held by + this harness or one of its ancestors, and `(label, reason)` otherwise. + + The ancestry test is the point, and it is stronger than "is the lock + held": a lock held by a *child* -- one `hostlock.sh run` per benchmark + invocation -- is released between arms, so it certifies each arm and + protects none of the comparison. A lock held by a *peer* is a reason to + stop rather than to start. + """ + state = prov.get("hostlock_state", "") + owner = prov.get("held_by", "none") + try: + anchor = int(prov.get("held_pid", "none")) + except ValueError: + anchor = 0 + + if not prov: + return "unknown", "the host lock could not be read at all" + if state == "HELD" and anchor in chain: + return f"mine:{owner}", None + if state == "HELD": + return ( + f"foreign:{owner}", + f"{owner} (pid {anchor}) holds this host, and that declaration is " + "not an ancestor of this harness", + ) + if state == "EXPIRED": + return ( + f"expired:{owner}", + "the declaration covering this host has expired, so a peer may " + "take the box mid-matrix. Re-acquire before measuring", + ) + if state == "STALE": + return ( + f"stale:{owner}", + f"the lock is held by a dead anchor ({owner}, pid {anchor}). " + "Reaping it does not stop whatever load it was covering, so " + "check the host before taking it", + ) + if state == "UNUSABLE": + return ( + "unusable", + "this host cannot take the lock at all (see `hostlock.sh status`). " + "Fix `lock_dir=` rather than measuring without one", + ) + if state == "FREE": + return "free", "no declaration covers this run" + return state.lower() or "unknown", f"the lock reports {state or 'nothing'}" + + +def window_label(label: str, before: dict[str, str], after: dict[str, str]) -> str: + """`changed` when custody moved during the run, else `label` unchanged. + + A run that changed hands halfway through was protected for neither half, + and a label naming whoever happened to hold the lock at one end describes + the other end as something it was not. Both the owner and the anchor pid + are compared: the same agent re-acquiring under a new anchor is still a + gap in which the box was free, and on a host cycling ~1.5M pids in four + days the pid alone can repeat. + """ + moved = (after.get("held_by"), after.get("held_pid")) != ( + before.get("held_by"), + before.get("held_pid"), + ) + return "changed" if moved else label + + def run_one( binary: Path, model: Path, @@ -126,8 +290,31 @@ def main() -> None: "run depresses the native arm (up to 6x on long cells here) and its " "noise swamps the comparison", ) + ap.add_argument( + "--unlocked", + action="store_true", + help="run without a host-lock declaration covering the matrix. The " + "rows are stamped `unlocked:` so they cannot later be read as " + "protected. For smoke tests only", + ) args = ap.parse_args() + # The lock is checked before anything is launched, because the whole point + # is to not put load on a host somebody else declared. Refusing after the + # first arm would already have contaminated their run and wasted ours. + prov = read_provenance() + lock_label, refusal = lock_verdict(prov, ancestry(os.getpid())) + if refusal: + if not args.unlocked: + sys.stderr.write(f"ab.py: refusing to measure: {refusal}.\n{_REMEDY}\n") + raise SystemExit(3) + lock_label = f"unlocked:{lock_label}" + sys.stderr.write( + f"ab.py: WARNING: running unlocked ({refusal}). Every row is " + f"stamped host_lock={lock_label} and none of them is publishable.\n" + ) + print(f"host_lock={lock_label} runnable={prov.get('runnable', '?')}", flush=True) + arms = {} for spec in args.arms: name, _, path = spec.partition("=") @@ -183,6 +370,16 @@ def main() -> None: flush=True, ) + # Read the lock again at the end, so the label covers the whole window + # rather than its first instant. + lock_label = window_label(lock_label, prov, read_provenance()) + for r in rows: + r["host_lock"] = lock_label + r["lock_owner"] = prov.get("held_by", "none") + r["lock_anchor_pid"] = prov.get("held_pid", "none") + r["runnable_at_start"] = prov.get("runnable", "unknown") + r["contended"] = prov.get("contended", "unknown") + args.csv.parent.mkdir(parents=True, exist_ok=True) with args.csv.open("w", newline="") as fh: w = csv.DictWriter(fh, fieldnames=list(rows[0].keys())) @@ -191,6 +388,7 @@ def main() -> None: metric = "native ms" if args.native_only else "native/ort ratio" print(f"\n=== medians ({metric}, lower is better) ===") + print(f"host_lock={lock_label} (whole window)") keys = sorted({(r["model"], r["threads"]) for r in rows}) for model, threads in keys: line = [f"{model:28s} t={threads:<3d}"] diff --git a/scripts/ort_ab/test_ab_lock.py b/scripts/ort_ab/test_ab_lock.py new file mode 100644 index 0000000000..047bf54822 --- /dev/null +++ b/scripts/ort_ab/test_ab_lock.py @@ -0,0 +1,290 @@ +#!/usr/bin/env python3 +"""Tests for `ab.py`'s host-lock admission. + +The interesting half is not that a locked run is allowed -- it is that each +*unprotected* shape is refused, and refused distinguishably. A gate that +allowed one of them would put load on a host somebody else had declared, and +the resulting numbers would carry a `host_lock` label saying otherwise. + +The end-to-end cells drive the real `hostlock.sh` and a stub arm binary that +prints a result line and exits. They cost no measurable CPU: nothing is +benchmarked, which is what lets this run in CI on a shared runner. +""" + +from __future__ import annotations + +import csv +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from ab import ( # noqa: E402 + ancestry, + lock_verdict, + parse_provenance, + read_provenance, + window_label, +) + +AB = Path(__file__).resolve().parent / "ab.py" +HOSTLOCK = Path(__file__).resolve().parents[1] / "hostlock.sh" + +# A stub arm: prints the line `ab.py` parses and exits. Using a real benchmark +# here would make the admission test cost minutes and a quiet host, which is +# the thing the lock exists to ration. +STUB_ARM = """#!/bin/sh +echo "native=1.000 ms ort=2.000 ms native/ort=0.500 native_p90=1.1 ort_p90=2.1 \ +native_min=0.9 ort_min=1.9 native_spread=0.2 ort_spread=0.2 parity=PASS" +""" + + +def prov(state: str, pid: str = "none", owner: str = "none") -> dict[str, str]: + return { + "hostlock_state": state, + "held_by": owner, + "held_pid": pid, + "runnable": "3", + "contended": "no", + } + + +class Verdict(unittest.TestCase): + """The decision table, as a table.""" + + def test_a_declaration_held_by_an_ancestor_admits_the_run(self): + label, refusal = lock_verdict(prov("HELD", "4242", "leon"), {1, 99, 4242}) + self.assertIsNone(refusal) + self.assertEqual(label, "mine:leon") + + def test_a_declaration_held_by_a_peer_stops_the_run(self): + label, refusal = lock_verdict(prov("HELD", "4242", "roy"), {1, 99}) + self.assertIsNotNone(refusal) + self.assertEqual(label, "foreign:roy") + + def test_a_lock_held_by_a_child_does_not_cover_the_matrix(self): + """The incident this gate exists for. + + One `hostlock.sh run` per benchmark child releases the lock between + arms, so the host reads idle in the gap and a peer starts a sweep in + the middle of somebody's interleaved A/B. A child's pid is not in our + ancestry, so the shape is refused even though a lock is genuinely + held. + """ + mine = os.getpid() + child = mine + 1000000 # not an ancestor of anything we are + _, refusal = lock_verdict(prov("HELD", str(child), "leon"), ancestry(mine)) + self.assertIsNotNone(refusal) + + def test_every_unprotected_state_is_refused_and_labelled_distinctly(self): + # The mapping is the specification. Two states share the `unknown` + # label on purpose -- an empty reading and a literal UNKNOWN are the + # same fact -- and every other one is distinct, because a reader has to + # be able to tell a host that had no lock from one whose lock died + # under it. + expected = { + "FREE": "free", + "STALE": "stale:roy", + "EXPIRED": "expired:roy", + "UNUSABLE": "unusable", + "UNKNOWN": "unknown", + "": "unknown", + } + for state, want in expected.items(): + label, refusal = lock_verdict(prov(state, "7", "roy"), {7}) + self.assertIsNotNone(refusal, f"{state} must not admit a run") + self.assertEqual(label, want, state) + self.assertEqual( + len(set(expected.values())), + 5, + "collapsing any further would hide a distinction the run needs", + ) + + def test_an_unreadable_lock_is_refused_rather_than_assumed_free(self): + label, refusal = lock_verdict({}, {1}) + self.assertEqual(label, "unknown") + self.assertIsNotNone(refusal) + + def test_a_broken_hostlock_reads_as_unreadable_not_as_free(self): + def explode(*_a, **_kw): + raise OSError("no such tool") + + self.assertEqual(read_provenance(runner=explode), {}) + + +class Window(unittest.TestCase): + """The label has to describe the whole run, not one end of it.""" + + def test_a_steady_holder_keeps_the_label(self): + before = prov("HELD", "17", "leon") + self.assertEqual(window_label("mine:leon", before, dict(before)), "mine:leon") + + def test_a_lock_that_changed_hands_is_not_reported_as_held_throughout(self): + before = prov("HELD", "17", "leon") + for after in ( + prov("FREE"), + prov("HELD", "17", "roy"), + prov("HELD", "18", "leon"), + ): + self.assertEqual( + window_label("mine:leon", before, after), + "changed", + after, + ) + + +class Ancestry(unittest.TestCase): + def test_the_walk_terminates_on_a_cycle(self): + # A reparented or namespaced process can report a parent already in + # the chain. Without the seen-set this loops until `limit`, and with a + # larger limit it would hang the harness before it ran anything. + self.assertEqual(ancestry(5, parent=lambda pid: 5), {5}) + + def test_the_walk_stops_at_init(self): + chain = {5: 4, 4: 1, 1: 0} + self.assertEqual(ancestry(5, parent=lambda pid: chain.get(pid)), {1, 4, 5}) + + def test_a_real_chain_contains_this_process_and_its_parent(self): + chain = ancestry(os.getpid()) + self.assertIn(os.getpid(), chain) + self.assertIn(os.getppid(), chain) + + +class Parsing(unittest.TestCase): + def test_the_oneline_format_round_trips(self): + text = "hostlock_state=HELD held_by=leon held_pid=17 runnable=4 reason=x" + self.assertEqual(parse_provenance(text)["held_by"], "leon") + self.assertEqual(parse_provenance(text)["held_pid"], "17") + + def test_the_real_script_emits_the_keys_this_gate_reads(self): + """Against the script, not against my memory of it. + + A key rename in `hostlock.sh` would otherwise leave the gate reading + `hostlock_state` forever, finding nothing, and -- because the gate is + fail-closed -- refusing every run on a correctly locked host. The + failure would be loud, but it would look like a lock bug rather than a + parser one. + """ + with tempfile.TemporaryDirectory(dir=str(AB.parent)) as tmp: + env = dict(os.environ, HOSTLOCK_DIR=f"{tmp}/hl", HOSTLOCK_PRIVATE_OK="1") + out = subprocess.run( + ["bash", str(HOSTLOCK), "provenance", "--oneline"], + capture_output=True, + text=True, + env=env, + timeout=60, + ) + fields = parse_provenance(out.stdout) + for key in ("hostlock_state", "held_by", "held_pid", "runnable", "contended"): + self.assertIn(key, fields, out.stdout) + + +class EndToEnd(unittest.TestCase): + """`ab.py` invoked for real, with a stub arm and a scratch lock.""" + + def setUp(self): + # Under the repo, never the shared lock and never a temp dir outside + # it: a test that used the real lock directory could release a lock a + # colleague was relying on. + self.tmp = tempfile.mkdtemp(dir=str(AB.parent)) + self.arm = Path(self.tmp) / "arm.sh" + self.arm.write_text(STUB_ARM) + self.arm.chmod(0o755) + self.csv = Path(self.tmp) / "out.csv" + self.env = dict( + os.environ, + HOSTLOCK_DIR=f"{self.tmp}/hl", + HOSTLOCK_PRIVATE_OK="1", + ) + + def tearDown(self): + subprocess.run(["rm", "-rf", self.tmp], check=False) + + def ab_args(self): + return [ + sys.executable, + str(AB), + "--arms", + f"a={self.arm}", + "--models", + "fixture.onnx", + "--threads", + "1", + "--trials", + "1", + "--runs", + "1", + "--warmups", + "0", + "--csv", + str(self.csv), + ] + + def test_an_unlocked_matrix_is_refused_before_any_arm_runs(self): + out = subprocess.run( + self.ab_args(), capture_output=True, text=True, env=self.env, timeout=300 + ) + self.assertEqual(out.returncode, 3, out.stdout + out.stderr) + self.assertIn("refusing to measure", out.stderr) + self.assertIn("hostlock.sh run --owner", out.stderr) + self.assertFalse( + self.csv.exists(), "a refused run must not leave a result file" + ) + + def test_the_wrapped_invocation_is_admitted_and_stamps_every_row(self): + out = subprocess.run( + [ + "bash", + str(HOSTLOCK), + "run", + "--owner", + "leon-selftest", + "--reason", + "ab-admission-selftest", + "--", + *self.ab_args(), + ], + capture_output=True, + text=True, + env=self.env, + timeout=300, + ) + self.assertEqual(out.returncode, 0, out.stdout + out.stderr) + # By column name and value, not by substring: a rename to + # `host_lock_unused` leaves the owner in the file and every + # `"host_lock" in text` assertion green while nothing downstream can + # find the field. + rows = list(csv.DictReader(self.csv.read_text().splitlines())) + self.assertTrue(rows) + for row in rows: + self.assertEqual(row["host_lock"], "mine:leon-selftest", row) + self.assertEqual(row["lock_owner"], "leon-selftest", row) + self.assertNotEqual(row["lock_anchor_pid"], "none", row) + self.assertIn("contended", row) + + def test_unlocked_by_request_runs_but_marks_the_rows(self): + out = subprocess.run( + [*self.ab_args(), "--unlocked"], + capture_output=True, + text=True, + env=self.env, + timeout=300, + ) + self.assertEqual(out.returncode, 0, out.stdout + out.stderr) + self.assertIn("WARNING: running unlocked", out.stderr) + rows = list(csv.DictReader(self.csv.read_text().splitlines())) + self.assertTrue(rows) + for row in rows: + self.assertEqual( + row["host_lock"], + "unlocked:free", + "the escape hatch must leave the rows self-identifying", + ) + + +if __name__ == "__main__": + unittest.main() From 75cfc8da0f5b9bdb38c1050f317155f957ca1bc9 Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Mon, 24 Aug 2026 21:31:00 +0000 Subject: [PATCH 2/2] ab.py: say when the end-of-window reading failed, and stop shipping a column that is always unknown Three findings from review of the A/B driver's lock gate. A run whose second provenance read came back empty was stamped `changed`. That asserts a specific fact -- custody moved -- about a run that may have been protected end to end, and it makes a real handoff indistinguishable from a timeout on the final read. It now says `unverified-end`: not evidence of a handoff, not evidence against one. `contended` was emitted on every row as the literal string `unknown`, because hostlock only computes it against `--expect-runnable N` and there is no honest N on a shared host (#1802). A column that is constant is not data, and a column named `contended` that always says `unknown` invites someone to read the absence of a `yes` as a `no`. Dropped rather than shipped as furniture. `--unlocked` exists so an unprotected run cannot be quoted later as a protected one -- reasoning entirely about our own labels. It does not extend to a box a peer has declared, where the damage lands on their measurement and no label of ours can reach it. It now refuses `foreign:`. The mapping from provenance key to column name is now a pure function with its own table-driven test, using distinct values so that a column reading its neighbour's key is caught; the reviewer noted a field swap survived the old suite, which asserted presence rather than value. Also recorded, in `lock_verdict`, why comparing pids without start times is safe here: the test sits behind `state == "HELD"`, which hostlock reports only after matching pid *and* /proc field-22 start time, so a recycled pid arrives as `STALE` and is refused. That coupling is load-bearing and invisible in the expression. Refs #1803 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/ort_ab/README.md | 19 ++++--- scripts/ort_ab/ab.py | 55 ++++++++++++++++--- scripts/ort_ab/test_ab_lock.py | 98 +++++++++++++++++++++++++++++++++- 3 files changed, 159 insertions(+), 13 deletions(-) diff --git a/scripts/ort_ab/README.md b/scripts/ort_ab/README.md index 4dce6f82f4..271951028c 100644 --- a/scripts/ort_ab/README.md +++ b/scripts/ort_ab/README.md @@ -255,13 +255,20 @@ scripts/hostlock.sh run --owner leon --reason "moe mt panel 6-cell" -- \ python3 scripts/ort_ab/ab.py --arms base=./a mine=./b --null-control ... ``` -Every CSV row then carries `host_lock`, `lock_owner`, `lock_anchor_pid`, -`runnable_at_start` and `contended`, and the label covers the **whole window**: -the lock is read again at the end, and a run that changed hands halfway through -is stamped `changed` rather than named after whoever happened to hold it last. +Every CSV row then carries `host_lock`, `lock_owner`, `lock_anchor_pid` and +`runnable_at_start`, and the label covers the **whole window**: the lock is +read again at the end, and a run that changed hands halfway through is stamped +`changed` rather than named after whoever happened to hold it last. If that +second reading fails the row says `unverified-end`, because an unreadable lock +is not evidence of a handoff and is not evidence against one either. +`runnable_at_start` is a single sample taken before the first arm — a note +about the conditions at the start, not a property of the interval; see the +`--gate` paragraph above for why no threshold on it is honest here. `--unlocked` runs anyway and stamps every row `unlocked:` — for smoke -tests, never for anything publishable. `scripts/ort_ab/test_ab_lock.py` covers -the admission table and runs in the `Host lock` workflow. +tests, never for anything publishable — but it will **not** run over a lock +somebody else declared, because that damage lands on their measurement, where +no label of ours can reach it. `scripts/ort_ab/test_ab_lock.py` covers the +admission table and runs in the `Host lock` workflow. `SIGKILL` (and a full-box crash) cannot be caught, so it leaves the lock directory behind. Nothing wedges: the lock carries its holder's pid **and** diff --git a/scripts/ort_ab/ab.py b/scripts/ort_ab/ab.py index 8e7e461d85..4c64715f35 100644 --- a/scripts/ort_ab/ab.py +++ b/scripts/ort_ab/ab.py @@ -154,6 +154,15 @@ def lock_verdict(prov: dict[str, str], chain: set[int]) -> tuple[str, str | None invocation -- is released between arms, so it certifies each arm and protects none of the comparison. A lock held by a *peer* is a reason to stop rather than to start. + + Only the pid *number* is compared, and that is safe **only** because it + sits behind `state == "HELD"`: `hostlock.sh` reports HELD only for an + anchor whose pid **and** `/proc` start time both still match, so a + recycled pid reaches here as STALE and is refused. This box cycles ~1.5M + pids in four days, so admitting on `held_pid in chain` without the state + check -- or accepting a state the script does not start-time verify -- + would be a genuine false admit. The invariant is recorded because it is + invisible in the expression that depends on it. """ state = prov.get("hostlock_state", "") owner = prov.get("held_by", "none") @@ -206,6 +215,12 @@ def window_label(label: str, before: dict[str, str], after: dict[str, str]) -> s gap in which the box was free, and on a host cycling ~1.5M pids in four days the pid alone can repeat. """ + if not after: + # The second read failed. That is not evidence of a handoff, and + # saying `changed` would assert a specific false fact -- custody + # moved -- about data that may be perfectly good. It is equally not + # evidence the declaration held, so the row says which it is. + return "unverified-end" moved = (after.get("held_by"), after.get("held_pid")) != ( before.get("held_by"), before.get("held_pid"), @@ -213,6 +228,32 @@ def window_label(label: str, before: dict[str, str], after: dict[str, str]) -> s return "changed" if moved else label +def lock_columns(label: str, prov: dict[str, str]) -> dict[str, str]: + """The lock fields stamped onto every row. + + A dict built in one place, so the mapping from provenance key to column + is testable. Built inline it was not: two of these columns could be + swapped, or read from the wrong provenance key, with every cell still + green -- the row would carry a number under a name that did not describe + it, which is worse than carrying nothing. + + `contended` is deliberately absent. `hostlock.sh` only computes it when + given `--expect-runnable`, and there is no honest threshold to pass here: + this host is shared by design (#1802), so "more runnable than expected" + has no fixed value. A column that reads `unknown` on every real run is an + invitation to treat its absence as reassurance. + """ + return { + "host_lock": label, + "lock_owner": prov.get("held_by", "none"), + "lock_anchor_pid": prov.get("held_pid", "none"), + # An instantaneous sample, named so it cannot be read as a property of + # the window. It is the runnable count at the moment the matrix + # started and says nothing about what happened afterwards. + "runnable_at_start": prov.get("runnable", "unknown"), + } + + def run_one( binary: Path, model: Path, @@ -305,7 +346,12 @@ def main() -> None: prov = read_provenance() lock_label, refusal = lock_verdict(prov, ancestry(os.getpid())) if refusal: - if not args.unlocked: + # `--unlocked` never overrides a live declaration by somebody else. + # The escape hatch exists so an unprotected run cannot be mistaken for + # a protected one later -- it is about the honesty of OUR rows. A peer + # holding the box is a different harm entirely: the damage lands on + # THEIR measurement, and no label on ours repairs it. + if not args.unlocked or lock_label.startswith("foreign:"): sys.stderr.write(f"ab.py: refusing to measure: {refusal}.\n{_REMEDY}\n") raise SystemExit(3) lock_label = f"unlocked:{lock_label}" @@ -373,12 +419,9 @@ def main() -> None: # Read the lock again at the end, so the label covers the whole window # rather than its first instant. lock_label = window_label(lock_label, prov, read_provenance()) + columns = lock_columns(lock_label, prov) for r in rows: - r["host_lock"] = lock_label - r["lock_owner"] = prov.get("held_by", "none") - r["lock_anchor_pid"] = prov.get("held_pid", "none") - r["runnable_at_start"] = prov.get("runnable", "unknown") - r["contended"] = prov.get("contended", "unknown") + r.update(columns) args.csv.parent.mkdir(parents=True, exist_ok=True) with args.csv.open("w", newline="") as fh: diff --git a/scripts/ort_ab/test_ab_lock.py b/scripts/ort_ab/test_ab_lock.py index 047bf54822..257bfedce9 100644 --- a/scripts/ort_ab/test_ab_lock.py +++ b/scripts/ort_ab/test_ab_lock.py @@ -26,6 +26,7 @@ from ab import ( # noqa: E402 ancestry, lock_verdict, + lock_columns, parse_provenance, read_provenance, window_label, @@ -123,6 +124,17 @@ def test_a_steady_holder_keeps_the_label(self): before = prov("HELD", "17", "leon") self.assertEqual(window_label("mine:leon", before, dict(before)), "mine:leon") + def test_a_failed_second_reading_is_not_a_handoff(self): + """`changed` asserts custody moved. An unreadable end says nothing. + + Stamping `changed` for a lock that could not be re-read blames a + completed, genuinely protected matrix for a handoff that never + happened -- and, worse, makes a real handoff indistinguishable from a + 60-second timeout on the final read. + """ + before = prov("HELD", "17", "leon") + self.assertEqual(window_label("mine:leon", before, {}), "unverified-end") + def test_a_lock_that_changed_hands_is_not_reported_as_held_throughout(self): before = prov("HELD", "17", "leon") for after in ( @@ -137,6 +149,48 @@ def test_a_lock_that_changed_hands_is_not_reported_as_held_throughout(self): ) +class Columns(unittest.TestCase): + """The provenance-key to column-name mapping, as a table. + + Every value below is distinct on purpose: two columns reading each + other's provenance key, or one reading a plausible neighbour, produces a + row whose numbers sit under names that do not describe them. A cell that + only checked the keys were present could not tell. + """ + + def test_each_column_carries_the_field_it_is_named_after(self): + fields = { + "held_by": "leon", + "held_pid": "4242", + "runnable": "7", + "held_secs": "99", + "contended": "no", + "hostlock_state": "HELD", + } + self.assertEqual( + lock_columns("mine:leon", fields), + { + "host_lock": "mine:leon", + "lock_owner": "leon", + "lock_anchor_pid": "4242", + "runnable_at_start": "7", + }, + ) + + def test_a_missing_reading_stamps_placeholders_rather_than_blanks(self): + # An empty cell in a CSV reads as "not applicable". These rows always + # have an answer, even when the answer is that there was none. + self.assertEqual( + lock_columns("unknown", {}), + { + "host_lock": "unknown", + "lock_owner": "none", + "lock_anchor_pid": "none", + "runnable_at_start": "unknown", + }, + ) + + class Ancestry(unittest.TestCase): def test_the_walk_terminates_on_a_cycle(self): # A reparented or namespaced process can report a parent already in @@ -264,7 +318,49 @@ def test_the_wrapped_invocation_is_admitted_and_stamps_every_row(self): self.assertEqual(row["host_lock"], "mine:leon-selftest", row) self.assertEqual(row["lock_owner"], "leon-selftest", row) self.assertNotEqual(row["lock_anchor_pid"], "none", row) - self.assertIn("contended", row) + self.assertTrue(row["runnable_at_start"].isdigit(), row) + + def test_unlocked_does_not_override_a_peers_declaration(self): + """The escape hatch protects our labels, not their run. + + `--unlocked` exists so an unprotected run cannot be quoted later as a + protected one. That reasoning is entirely about our own rows, and it + does not extend to a box somebody else has declared: the damage there + lands on their measurement, where no label of ours can reach it. + """ + anchor = subprocess.Popen(["sleep", "120"]) + try: + taken = subprocess.run( + [ + "bash", + str(HOSTLOCK), + "acquire", + "--owner", + "someone-else", + "--reason", + "peer-declaration", + "--pid", + str(anchor.pid), + ], + capture_output=True, + text=True, + env=self.env, + timeout=300, + ) + self.assertEqual(taken.returncode, 0, taken.stdout + taken.stderr) + out = subprocess.run( + [*self.ab_args(), "--unlocked"], + capture_output=True, + text=True, + env=self.env, + timeout=300, + ) + self.assertEqual(out.returncode, 3, out.stdout + out.stderr) + self.assertIn("someone-else", out.stderr) + self.assertFalse(self.csv.exists()) + finally: + anchor.kill() + anchor.wait() def test_unlocked_by_request_runs_but_marks_the_rows(self): out = subprocess.run(