ab.py: refuse to measure a host nobody declared

.github/workflows/hostlock.yml

@@ -80,13 +80,17 @@ 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:
       - main
     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

scripts/ort_ab/README.md

@@ -241,6 +241,35 @@ 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` 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:<state>` — for smoke
+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**
 that pid's start time, and the next acquirer reclaims it as soon as that

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<native>[\d.]+) ms .*?ort=(?P<ort>[\d.]+) ms .*?"
     r"native/ort=(?P<ratio>[\d.]+) native_p90=(?P<np90>[\d.]+) ort_p90=(?P<op90>[\d.]+) "
@@ -49,6 +51,209 @@
 )
 
 
+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/<pid>/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 <you> --reason "<what this measures>" -- \\
+        python3 scripts/ort_ab/ab.py <your args>
+
+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.
+
+    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")
+    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.
+    """
+    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"),
+    )
+    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,
@@ -126,8 +331,36 @@ 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:
+        # `--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}"
+        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 +416,13 @@ 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())
+    columns = lock_columns(lock_label, prov)
+    for r in rows:
+        r.update(columns)
+
     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 +431,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}"]