From 9607c517003c9dd9a620b4095e87e63f573141d8 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 07:36:18 +0000 Subject: [PATCH] tools(GATE-GPU-LOCK-WRAPPER): one sanctioned way to take the GPU, and it records which lock it took (#587) GPU serialisation on `dgx.casa` was split across two lock FILES. `$HOME/gpu.lock` is what `.agents/benchmarking.md` and `.agents/environment.md` document and what 432 references in this tree take; `~/work/marlin442` took `/tmp/gpu.lock`. Two jobs holding different files run concurrently while each believes it owns the box, and nothing reveals it: `fuser $HOME/gpu.lock` shows an empty waiter list while the other job is mid-run. On GB10 that is an OOM-reboot mechanism, because unified memory makes a `gpu_memory_utilization` reservation HOST RAM, and it voids any contention-sensitive number silently. `scripts/gpu-lock.sh` is the one sanctioned way to take it, so the path stops being something each script re-decides. `$HOME/gpu.lock` is canonical: it has the callers and the documentation, and it is per-user, where a lock under `/tmp` is world-writable -- any process on the box can hold or clobber the mutex guarding someone else's measurement -- and is cleared by the very reboot it exists to prevent, taking the evidence of who held it with it. `$GPU_LOCK` still overrides, and the wrapper stamps whichever path it RESOLVED, so a divergence shows up in the record instead of surfacing when two jobs collide. Two properties are the design. It REFUSES, never falls back. An untakeable lock aborts with exit 78 and the command does not run; a bounded wait expires with its own exit 75 rather than sitting forever behind a dead holder. A wrapper that proceeds on the assumption it holds a lock it never took is one more instrument that cannot report its own failure -- the class that produces a `Status: SUCCESS!` from a binary an ENOSPC build never recompiled, and a "policy violation" that is really a checker unable to write a temp file. It RECORDS, and the exit code comes FIRST. Disk and load say the box was unhealthy; the exit code says WHICH thing killed the run. `137` (SIGKILL -- another agent's `pkill`) versus a compiler diagnostic versus an ENOSPC line are three different diagnoses and three different repairs, and only the first says it was not your code at all. The stamp carries the resolved lock path, the requested spelling, holder pid, wait, timeout, `df -h /` and load average at acquire and again at exit, the command, and `exit-code` + `exit-reason` as the first two fields of the release block. The near-miss this is built from: a peer's same-binary A/B on this box survived #587 only because its conclusion rested on `diff -r -q` over two output directories -- byte-identity is contention-immune -- while the wall times printed beside it were not. Had the conclusion rested on the times, #587 would have voided it silently and nothing in the record would have said so. Stamping does not prevent contention. It prevents contention being undetectable afterwards. Nesting passes through on the same resolved path instead of deadlocking on a lock its own parent holds; a different path is still taken for real. RED first: 48 tests written against an absent script, all 48 erroring, then green. Eight mutations prove the checks catch their defect rather than describing it -- the refusal returning instead of exiting, dropping the `lock-path` field, dropping the `exit-code` field, swallowing the exit code, collapsing the timeout status to 0, skipping the acquisition entirely, unbounding the wait, and removing the pass-through so nesting self-deadlocks. Each is caught. THE SWEEP IS NOT CLOSED. The repo has 432 `$HOME/gpu.lock` references and zero `/tmp/gpu.lock` -- but a THIRD spelling #587 did not name: `/tmp/gpu`, no extension, taken as `exec 9>/tmp/gpu` by `scripts/dgx-online-serving.sh` and `scripts/dgx-gdn-packed-component.sh`, named by `scripts/opt-dgx-gate.sh` and `scripts/dgx-sglang-low-concurrency.sh`, and documented as this box's `${GPU_LOCK}` in `.agents/coordination.md:99`. Those callers are outside this change's authority and are owed a follow-up: until they are repointed, a run under this wrapper does NOT exclude an online-serving or gdn-packed-component run, which is #587 with different filenames. On dgx, `~/*.sh` converges on `$HOME/gpu.lock` across 14 spellings that all resolve to one file, and `~/work/marlin442`'s own scripts take NO lock at all; `/tmp/gpu.lock` did not survive the 09:00 reboot, so what held it is not recoverable from the filesystem and is not asserted here. `.agents/benchmarking.md` and `.agents/environment.md` name the wrapper as the sanctioned path, so the next agent does not read the old instruction and take a raw `flock`. Disk logged beside every result, per the box discipline: dev box 88% used / 56G free throughout; dgx 99% used / 61G free, read-only sweep only, nothing built or deleted there. Not wired into `scripts/agent-preflight.sh` SUITES or `.github/workflows/ci.yml` -- both outside this change's authority, and owed. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/benchmarking.md | 25 ++ .agents/environment.md | 32 +- scripts/gpu-lock.sh | 295 +++++++++++++++++ tests/scripts/test_gpu_lock.py | 578 +++++++++++++++++++++++++++++++++ 4 files changed, 927 insertions(+), 3 deletions(-) create mode 100755 scripts/gpu-lock.sh create mode 100644 tests/scripts/test_gpu_lock.py diff --git a/.agents/benchmarking.md b/.agents/benchmarking.md index 8d3d957b2..4311b298d 100644 --- a/.agents/benchmarking.md +++ b/.agents/benchmarking.md @@ -22,6 +22,31 @@ One GPU job at a time. Take the box lock before any measurement, stop competing services, and never run two large models at once — unified-memory boxes reboot rather than swap. +**Take it with `scripts/gpu-lock.sh`, not a raw `flock`.** It is the one +sanctioned way to take the GPU, and the reason is #587: the discipline said +`$HOME/gpu.lock`, one job on `dgx` took `/tmp/gpu.lock`, and two jobs holding +different files ran concurrently while each believed it owned the box. Nothing +in the record said so afterwards. A wrapped leg looks like + +```sh +scripts/gpu-lock.sh --label 'ours-c1' --record evidence/lock.txt -- ./bench ... +``` + +and it refuses loudly (exit 78) rather than proceeding without the lock, expires +its bounded wait with its own status (exit 75) rather than sitting behind a dead +holder, and passes the wrapped command's exit code through unchanged. Its stamp +carries the **resolved** lock path it actually took, the wait, `df -h /` and +load average, and the exit code first — disk and load say the box was unhealthy, +the exit code says which thing killed the run, and `137` (someone else's +`pkill`) is not a result about your code at all. + +The stamp does not prevent contention; it prevents contention being undetectable +afterwards. A same-binary A/B on this box survived #587 only because its +conclusion rested on `diff -r -q` over two output directories — byte-identity is +contention-immune — while the wall times printed beside it were not. Had the +conclusion rested on the times, #587 would have voided it silently. Quote the +lock stamp beside any number that a second job could have moved. + Calibrate the noise band from repeated identical legs *before* interpreting a delta. Discard cold legs for a named cause, never because they are inconvenient. Use paired, order-alternated A/B legs and a majority rule; a diff --git a/.agents/environment.md b/.agents/environment.md index 1762e8762..9df5c2985 100644 --- a/.agents/environment.md +++ b/.agents/environment.md @@ -133,11 +133,37 @@ environment: - **GPU mutex:** every CUDA test/model/serve/benchmark/profile holds the `${GPU_LOCK}` file mutex for the whole job or multi-arm series WHEN other agents may run GPU work concurrently (sole owner verified idle via - `nvidia-smi` may skip). Mechanism: run GPU work as - `flock ${GPU_LOCK} -c ''`, or take the lock once around an entire - benchmark series so arms are never interleaved; waiting on the lock is + `nvidia-smi` may skip). **`scripts/gpu-lock.sh` is the ONE sanctioned way + to take it** — a raw `flock` is how #587 happened. Run GPU work as + `scripts/gpu-lock.sh -- `, or wrap an entire benchmark series in + one invocation so arms are never interleaved; waiting on the lock is normal, stealing it is not. Compilation, source inspection and file transfer do not need the lock. Never kill an unowned PID. + - **The canonical lock file is `$HOME/gpu.lock`** (`${GPU_LOCK}` overrides + it, and the wrapper stamps whichever it resolved). Never invent a second + spelling: until 2026-08-13 one job on this box took `/tmp/gpu.lock` while + everything else took `$HOME/gpu.lock`, so two jobs ran concurrently while + each believed it owned the GPU, and `fuser $HOME/gpu.lock` showed an empty + waiter list throughout (#587). On unified memory that is an OOM-reboot + mechanism, and it voids any contention-sensitive number silently. + `scripts/gpu-lock.sh` **refuses** (exit 78) rather than proceeding without + the lock, times out with its own status (exit 75) rather than waiting + forever on a dead holder, propagates the wrapped command's exit code + unchanged — `137` still means SIGKILL, not "your code failed" — and stamps + the resolved lock path, the wait, `df -h /` and load average, and that exit + code **first**, so contention is at worst detectable afterwards. Do not + double-wrap by hand: a nested invocation on the same resolved path passes + through instead of deadlocking, and that is the only nesting that is safe. + Guarantees pinned by `tests/scripts/test_gpu_lock.py`. + - **A third spelling is still open (2026-08-13 sweep).** `/tmp/gpu`, with no + extension, is taken as `exec 9>/tmp/gpu` by `scripts/dgx-online-serving.sh` + and `scripts/dgx-gdn-packed-component.sh`, named by `scripts/opt-dgx-gate.sh` + and `scripts/dgx-sglang-low-concurrency.sh`, and recorded as this box's + `${GPU_LOCK}` in `coordination.md`. Until those are repointed at + `scripts/gpu-lock.sh`, a run under the wrapper does **not** exclude an + online-serving or gdn-packed-component run — #587 with different filenames. + Do not read a clean `fuser $HOME/gpu.lock` as an idle box; check + `nvidia-smi` and `fuser /tmp/gpu` too. - Disk cleanup 2026-07-10 reclaimed ~368 GB from unrelated cached model sets, April-era autoresearch logits/F16-GGUF cache artifacts, the vLLM compile cache and stale rebuildable CUDA build trees. Active latency/PR workspaces, diff --git a/scripts/gpu-lock.sh b/scripts/gpu-lock.sh new file mode 100755 index 000000000..7a59b3d05 --- /dev/null +++ b/scripts/gpu-lock.sh @@ -0,0 +1,295 @@ +#!/usr/bin/env bash +# The one sanctioned way to take the GPU. Serialise, then prove it. +# +# scripts/gpu-lock.sh [options] -- [args...] +# +# Options: +# --lock PATH lock file (default: $GPU_LOCK, else $HOME/gpu.lock) +# --timeout SECONDS bounded wait (default: $GPU_LOCK_TIMEOUT, else 1800) +# --label TEXT free-text tag stamped into the record +# --record FILE append the stamp here as well as to stderr +# -h, --help this text +# +# Exit status: +# 78 REFUSED the lock could not be taken; the command did NOT run +# 75 TIMEOUT the bounded wait expired; the command did NOT run +# 2 usage error; the command did NOT run +# * the wrapped command's own exit status, unchanged (137 stays 137) +# +# WHY THIS EXISTS (#587). GPU work on dgx was serialised through flock, but two +# different lock FILES were in use -- $HOME/gpu.lock, which the discipline +# documents and most callers take, and /tmp/gpu.lock, which one job took. Two +# jobs holding different files run concurrently while each believes it owns the +# box, and nothing in the flock discipline reveals it: `fuser $HOME/gpu.lock` +# shows an empty waiter list while the other job is mid-run. On GB10 that is an +# OOM-reboot mechanism, because unified memory means a `gpu_memory_utilization` +# reservation is HOST RAM. It also silently voids measurements. +# +# CANONICAL PATH: $HOME/gpu.lock. It has 432 references in this repository +# against zero for /tmp/gpu.lock, it is what .agents/environment.md and +# .agents/benchmarking.md document, and it is per-user -- a lock under /tmp is +# world-writable, which lets any process on the box hold or clobber the mutex +# that guards someone else's measurement, and /tmp is cleared on reboot, so the +# evidence of who held it does not survive the OOM-reboot it was supposed to +# prevent. The env override $GPU_LOCK (the key already in .env.example) exists +# for a host whose profile genuinely differs; it is stamped, so a divergence is +# visible in the record instead of invisible until two jobs collide. +# +# The 2026-08-13 sweep found a THIRD spelling #587 did not name: `/tmp/gpu`, +# with no extension, taken as `exec 9>/tmp/gpu` by scripts/dgx-online-serving.sh +# and scripts/dgx-gdn-packed-component.sh, named by scripts/opt-dgx-gate.sh and +# scripts/dgx-sglang-low-concurrency.sh, and documented as the DGX profile's +# ${GPU_LOCK} in .agents/coordination.md:99. Those callers are outside this +# change's authority and are owed a follow-up: until they are repointed, a +# benchmark taken through this wrapper does NOT exclude an online-serving or +# gdn-packed-component run, which is #587 with different filenames. +# +# TWO PROPERTIES, BOTH LEARNED THE HARD WAY. +# +# 1. It REFUSES, never falls back. If the lock cannot be taken the wrapper +# aborts loudly and non-zero and does not run the command. A wrapper that +# proceeds on the assumption it holds a lock it never took is one more +# instrument that cannot report its own failure -- the class of defect that +# produced stale-green builds from an ENOSPC that left the old binary in +# place, and a policy "violation" that was really a checker unable to write +# a temp file. +# +# 2. It RECORDS. The lock is the mechanism; the stamp is what makes a number +# defensible afterwards. The wrapped command's exit code comes FIRST in the +# release block, ahead of disk and load, because disk and load say the box +# was unhealthy while the exit code says WHICH thing killed the run: 137 +# (SIGKILL -- another agent's pkill) versus a compiler diagnostic versus an +# ENOSPC line are three different diagnoses and three different repairs, and +# only the first says it was not your code at all. +# +# Stamping does not prevent contention. It prevents contention being +# undetectable afterwards. A peer's same-binary A/B on this box survived +# #587 only because its conclusion rested on `diff -r -q` over two output +# directories -- byte-identity is contention-immune -- while the wall times +# printed beside it were not. Had the conclusion rested on the times, #587 +# would have voided it silently and nothing in the record would have said so. +# +# NESTING. The wrapper exports VLLM_CPP_GPU_LOCK_HELD with the resolved path it +# took. A nested invocation on that same path passes through instead of waiting +# on a lock its own parent holds, which would deadlock until the timeout. A +# nested invocation on a DIFFERENT path is taken for real. +# +# Guarantees are pinned by tests/scripts/test_gpu_lock.py, including mutations +# that break each one and must be caught. + +set -uo pipefail + +EXIT_REFUSED=78 +EXIT_TIMEOUT=75 +EXIT_USAGE=2 + +LOCK_ARG="" +TIMEOUT="" +LABEL="" +RECORD="" +RECORD_OK=0 + +usage() { + sed -n '2,17p' "$0" | sed 's/^# \{0,1\}//' +} + +die_usage() { + printf 'gpu-lock: usage error: %s\n' "$1" >&2 + printf 'gpu-lock: the command did NOT run\n' >&2 + exit "$EXIT_USAGE" +} + +while [ "$#" -gt 0 ]; do + case "$1" in + --lock) [ "$#" -ge 2 ] || die_usage "--lock needs a path"; LOCK_ARG="$2"; shift 2 ;; + --lock=*) LOCK_ARG="${1#--lock=}"; shift ;; + --timeout) [ "$#" -ge 2 ] || die_usage "--timeout needs seconds"; TIMEOUT="$2"; shift 2 ;; + --timeout=*) TIMEOUT="${1#--timeout=}"; shift ;; + --label) [ "$#" -ge 2 ] || die_usage "--label needs text"; LABEL="$2"; shift 2 ;; + --label=*) LABEL="${1#--label=}"; shift ;; + --record) [ "$#" -ge 2 ] || die_usage "--record needs a path"; RECORD="$2"; shift 2 ;; + --record=*) RECORD="${1#--record=}"; shift ;; + -h|--help) usage; exit 0 ;; + --) shift; break ;; + -*) die_usage "unknown option: $1" ;; + *) break ;; + esac +done + +[ "$#" -ge 1 ] || die_usage "no command given (use: gpu-lock.sh [options] -- )" + +TIMEOUT="${TIMEOUT:-${GPU_LOCK_TIMEOUT:-1800}}" +case "$TIMEOUT" in + '' | *[!0-9.]* | *.*.* | .) die_usage "--timeout must be a number of seconds: $TIMEOUT" ;; +esac +awk -v t="$TIMEOUT" 'BEGIN { exit !(t + 0 > 0) }' \ + || die_usage "--timeout must be greater than zero: $TIMEOUT" + +# ---------------------------------------------------------------- stamping -- +stamp() { + printf 'gpu-lock: %s\n' "$1" >&2 + if [ "$RECORD_OK" -eq 1 ]; then + printf 'gpu-lock: %s\n' "$1" >>"$RECORD" + fi +} + +banner() { + stamp "=== $1 ===" +} + +now() { date +%s.%N; } + +since() { awk -v a="$1" -v b="$2" 'BEGIN { printf "%.3f", b - a }'; } + +loadavg() { + if [ -r /proc/loadavg ]; then + cut -d' ' -f1-3 /proc/loadavg + else + uptime | sed 's/.*load averages\{0,1\}: //' | tr -d ',' + fi +} + +df_root() { + # `-P` keeps one filesystem per line, so the row cannot wrap and lose the + # use% that is the whole reason to look. A full disk does not fail loudly. + df -hP / 2>/dev/null | tail -1 | tr -s ' ' +} + +# ---------------------------------------------------------------- refusal --- +# The single exit in this function is the anchor for the "refusal falls +# through" mutation. Reaching the end of this function without exiting is the +# defect the wrapper exists to make impossible. +refuse() { + banner "REFUSED" + stamp "outcome=REFUSED" + stamp "reason=$1" + stamp "lock-path=$LOCK_PATH" + stamp "requested-lock=$LOCK_REQUESTED" + stamp "loadavg=$(loadavg)" + stamp "df-root=$(df_root)" + stamp "the command did NOT run" + exit "$EXIT_REFUSED" +} + +# ------------------------------------------------------- resolve the path --- +LOCK_REQUESTED="${LOCK_ARG:-${GPU_LOCK:-${HOME:-}/gpu.lock}}" +if [ -z "${LOCK_ARG:-}" ] && [ -z "${GPU_LOCK:-}" ] && [ -z "${HOME:-}" ]; then + LOCK_PATH="" + refuse "no lock path: HOME is unset and neither --lock nor GPU_LOCK was given" +fi + +resolve() { + local p="$1" resolved d b + if resolved="$(readlink -f -- "$p" 2>/dev/null)" && [ -n "$resolved" ]; then + printf '%s\n' "$resolved" + return 0 + fi + d="$(dirname -- "$p")" + b="$(basename -- "$p")" + if d="$(cd -- "$d" 2>/dev/null && pwd -P)"; then + printf '%s/%s\n' "$d" "$b" + return 0 + fi + return 1 +} + +if ! LOCK_PATH="$(resolve "$LOCK_REQUESTED")"; then + LOCK_PATH="$LOCK_REQUESTED" + refuse "lock directory does not exist or is unreachable: $(dirname -- "$LOCK_REQUESTED")" +fi + +# ------------------------------------------------- validate before stamping -- +if [ -e "$LOCK_PATH" ]; then + [ -f "$LOCK_PATH" ] || refuse "lock path exists but is not a regular file" + [ -w "$LOCK_PATH" ] || refuse "lock file is not writable: $LOCK_PATH" +else + LOCK_DIR="$(dirname -- "$LOCK_PATH")" + [ -d "$LOCK_DIR" ] || refuse "lock directory does not exist: $LOCK_DIR" + [ -w "$LOCK_DIR" ] || refuse "lock directory is not writable: $LOCK_DIR" +fi + +if [ -n "$RECORD" ]; then + # A record we cannot write is the instrument that cannot report its own + # failure, so it refuses here rather than measuring into nowhere. + if ( : >>"$RECORD" ) 2>/dev/null; then + RECORD_OK=1 + else + refuse "record file is not appendable: $RECORD" + fi +fi + +# --------------------------------------------------------------- acquire ---- +CMD_QUOTED="$(printf '%q ' "$@")" +START="$(now)" + +if [ "${VLLM_CPP_GPU_LOCK_HELD:-}" = "$LOCK_PATH" ]; then + # An ancestor of this process already holds exactly this lock. Waiting on it + # would deadlock until the timeout, so pass through and say so in the record. + MODE="PASS-THROUGH" + WAITED="0.000" +else + MODE="ACQUIRED" + if ! exec {LOCK_FD}>>"$LOCK_PATH"; then + refuse "could not open the lock file for writing: $LOCK_PATH" + fi + if ! flock -w "$TIMEOUT" "$LOCK_FD"; then + WAITED="$(since "$START" "$(now)")" + banner "TIMEOUT" + stamp "outcome=TIMEOUT" + stamp "lock-path=$LOCK_PATH" + stamp "waited-seconds=$WAITED" + stamp "timeout-seconds=$TIMEOUT" + stamp "holder=$(fuser -v "$LOCK_PATH" 2>&1 | tr -s ' \n' ' ' || echo unknown)" + stamp "loadavg=$(loadavg)" + stamp "df-root=$(df_root)" + stamp "the command did NOT run" + exit "$EXIT_TIMEOUT" + fi + WAITED="$(since "$START" "$(now)")" +fi + +ACQUIRED_AT="$(now)" + +banner "ACQUIRE" +stamp "mode=$MODE" +stamp "lock-path=$LOCK_PATH" +stamp "requested-lock=$LOCK_REQUESTED" +stamp "holder-pid=$$" +stamp "waited-seconds=$WAITED" +stamp "timeout-seconds=$TIMEOUT" +stamp "acquired-utc=$(date -u +%Y-%m-%dT%H:%M:%SZ)" +stamp "host=$(hostname 2>/dev/null || echo unknown)" +stamp "label=$LABEL" +stamp "loadavg=$(loadavg)" +stamp "df-root=$(df_root)" +stamp "command=$CMD_QUOTED" + +export VLLM_CPP_GPU_LOCK_HELD="$LOCK_PATH" +export VLLM_CPP_GPU_LOCK_PID="$$" + +# ------------------------------------------------------------------- run ---- +"$@" +RC=$? + +# ---------------------------------------------------------------- release --- +# exit-code FIRST. Everything below it describes the box; this line describes +# what happened to the job, and it is the one that says whether the box was +# even the reason. +banner "RELEASE" +stamp "exit-code=$RC" +if [ "$RC" -gt 128 ] && [ "$RC" -lt 192 ]; then + SIG=$((RC - 128)) + stamp "exit-reason=killed by signal $SIG ($(kill -l "$SIG" 2>/dev/null || echo unknown))" +else + stamp "exit-reason=exited with status $RC" +fi +stamp "outcome=$([ "$MODE" = "PASS-THROUGH" ] && echo PASS-THROUGH || echo RAN)" +stamp "lock-path=$LOCK_PATH" +stamp "holder-pid=$$" +stamp "waited-seconds=$WAITED" +stamp "elapsed-seconds=$(since "$ACQUIRED_AT" "$(now)")" +stamp "released-utc=$(date -u +%Y-%m-%dT%H:%M:%SZ)" +stamp "loadavg-at-exit=$(loadavg)" +stamp "df-root-at-exit=$(df_root)" + +exit "$RC" diff --git a/tests/scripts/test_gpu_lock.py b/tests/scripts/test_gpu_lock.py new file mode 100644 index 000000000..5ffb20530 --- /dev/null +++ b/tests/scripts/test_gpu_lock.py @@ -0,0 +1,578 @@ +#!/usr/bin/env python3 +"""Guarantees for ``scripts/gpu-lock.sh`` -- the one sanctioned way to take the GPU. + +Issue #587: GPU serialisation on ``dgx.casa`` was split across ``$HOME/gpu.lock`` +and ``/tmp/gpu.lock``, so two jobs each holding a *different* file ran +concurrently while each believed it owned the box. The wrapper exists so the +path stops being a thing every script re-decides, and so the path it *actually* +took is stamped into the evidence rather than assumed. + +Two properties are load-bearing and both are claims about an instrument, which +is exactly the class that quietly stops being true: + +1. **It refuses, never falls back.** A wrapper that proceeds without the lock it + failed to take is a measurement that cannot report its own invalidity. +2. **It records.** The lock path actually taken, the wait, the box health, and + -- first, because it is the field that says *which* thing killed the run -- + the wrapped command's exit code. ``137`` (someone else's ``pkill``) and a + compiler diagnostic and an ENOSPC line are three different diagnoses. + +The ``Mutations`` class at the bottom proves these tests catch their defect +rather than merely describing it: each mutant breaks one guarantee in a scratch +copy of the script and the corresponding check must go red. +""" + +from __future__ import annotations + +import contextlib +import os +import re +import shlex +import shutil +import signal +import stat +import subprocess +import sys +import tempfile +import time +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +SCRIPT = ROOT / "scripts" / "gpu-lock.sh" + +# Exit statuses the wrapper reserves for its own failures. They must stay +# distinct from each other; a wrapped command that happens to exit 75 is told +# apart by the stamped `outcome=` field, which only a RUN emits. +EXIT_REFUSED = 78 +EXIT_TIMEOUT = 75 + +FIELD = re.compile(r"^gpu-lock: ([a-z0-9-]+)=(.*)$") + + +def has_flock() -> bool: + return shutil.which("flock") is not None + + +def run(args, *, env=None, cwd=None, timeout=60, base_env=True): + """Invoke the wrapper. Never inherits a real GPU_LOCK/HOME by accident.""" + full = dict(os.environ) if base_env else {} + full.pop("GPU_LOCK", None) + full.pop("GPU_LOCK_TIMEOUT", None) + full.pop("VLLM_CPP_GPU_LOCK_HELD", None) + if env: + full.update(env) + return subprocess.run( + [str(SCRIPT), *args], + env=full, + cwd=cwd, + capture_output=True, + text=True, + timeout=timeout, + ) + + +def fields(stderr: str) -> "list[tuple[str, str]]": + """Every ``gpu-lock: key=value`` line, in emission order.""" + out = [] + for line in stderr.splitlines(): + m = FIELD.match(line) + if m: + out.append((m.group(1), m.group(2))) + return out + + +def block(stderr: str, name: str) -> "list[tuple[str, str]]": + """Fields between ``=== NAME ===`` and the next ``===`` banner.""" + lines = stderr.splitlines() + start = None + for i, line in enumerate(lines): + if line.strip() == f"gpu-lock: === {name} ===": + start = i + 1 + break + if start is None: + return [] + end = len(lines) + for j in range(start, len(lines)): + if lines[j].strip().startswith("gpu-lock: ==="): + end = j + break + return fields("\n".join(lines[start:end])) + + +def value(pairs, key): + for k, v in pairs: + if k == key: + return v + return None + + +@contextlib.contextmanager +def other_holder(lock_path: str, seconds: int = 30): + """A second job holding the SAME lock file, as #587's two jobs did not.""" + marker = lock_path + ".held" + proc = subprocess.Popen( + ["flock", lock_path, "-c", f"touch {shlex.quote(marker)}; sleep {seconds}"], + start_new_session=True, + ) + try: + deadline = time.time() + 15 + while not os.path.exists(marker): + if time.time() > deadline: + raise AssertionError("background holder never acquired the lock") + time.sleep(0.02) + yield + finally: + # Kill the GROUP: killing only the shell orphans `sleep`, which keeps the + # inherited descriptor open and the lock held for the rest of the suite. + with contextlib.suppress(ProcessLookupError): + os.killpg(proc.pid, signal.SIGKILL) + proc.wait() + with contextlib.suppress(FileNotFoundError): + os.unlink(marker) + + +class Base(unittest.TestCase): + @classmethod + def setUpClass(cls): + if not has_flock(): + raise unittest.SkipTest("flock(1) is required") + if os.geteuid() == 0: + raise unittest.SkipTest("root ignores the mode bits the refusal rests on") + + def setUp(self): + # realpath: the wrapper stamps the RESOLVED path, and on some hosts the + # temp root is itself a symlink. + self.tmp = os.path.realpath(tempfile.mkdtemp(prefix="gpulock-")) + self.addCleanup(self._cleanup) + self.lock = os.path.join(self.tmp, "gpu.lock") + self.marker = os.path.join(self.tmp, "ran.marker") + + def _cleanup(self): + for root, dirs, _ in os.walk(self.tmp): + for d in dirs: + with contextlib.suppress(OSError): + os.chmod(os.path.join(root, d), 0o755) + shutil.rmtree(self.tmp, ignore_errors=True) + + def touch_cmd(self): + return ["sh", "-c", f"touch {shlex.quote(self.marker)}"] + + def assertNotRun(self, res): + self.assertFalse( + os.path.exists(self.marker), + f"the command RAN despite {res.returncode}; stderr:\n{res.stderr}", + ) + + +# -------------------------------------------------------------------------- +# 1. It refuses, never falls back. +# -------------------------------------------------------------------------- +class Refusal(Base): + def test_unwritable_lock_directory_refuses(self): + ro = os.path.join(self.tmp, "ro") + os.mkdir(ro) + os.chmod(ro, stat.S_IRUSR | stat.S_IXUSR) + res = run(["--lock", os.path.join(ro, "gpu.lock"), "--", *self.touch_cmd()]) + self.assertEqual(res.returncode, EXIT_REFUSED, res.stderr) + self.assertNotRun(res) + + def test_unwritable_existing_lock_file_refuses(self): + Path(self.lock).write_text("") + os.chmod(self.lock, stat.S_IRUSR) + res = run(["--lock", self.lock, "--", *self.touch_cmd()]) + self.assertEqual(res.returncode, EXIT_REFUSED, res.stderr) + self.assertNotRun(res) + + def test_missing_lock_parent_refuses(self): + res = run(["--lock", os.path.join(self.tmp, "nope", "gpu.lock"), + "--", *self.touch_cmd()]) + self.assertEqual(res.returncode, EXIT_REFUSED, res.stderr) + self.assertNotRun(res) + + def test_unwritable_record_file_refuses(self): + ro = os.path.join(self.tmp, "ro") + os.mkdir(ro) + os.chmod(ro, stat.S_IRUSR | stat.S_IXUSR) + res = run(["--lock", self.lock, "--record", os.path.join(ro, "ev.txt"), + "--", *self.touch_cmd()]) + self.assertEqual(res.returncode, EXIT_REFUSED, res.stderr) + self.assertNotRun(res) + + def test_refusal_is_loud_and_names_the_path(self): + ro = os.path.join(self.tmp, "ro") + os.mkdir(ro) + os.chmod(ro, stat.S_IRUSR | stat.S_IXUSR) + target = os.path.join(ro, "gpu.lock") + res = run(["--lock", target, "--", *self.touch_cmd()]) + self.assertIn("gpu-lock: === REFUSED ===", res.stderr) + pairs = block(res.stderr, "REFUSED") + self.assertEqual(value(pairs, "outcome"), "REFUSED") + self.assertEqual(value(pairs, "lock-path"), target) + self.assertTrue(value(pairs, "reason"), "refusal carried no reason") + + def test_empty_command_is_a_usage_error_not_a_silent_success(self): + res = run(["--lock", self.lock, "--"]) + self.assertNotEqual(res.returncode, 0) + + def test_nonnumeric_timeout_is_rejected(self): + res = run(["--lock", self.lock, "--timeout", "soon", "--", *self.touch_cmd()]) + self.assertNotEqual(res.returncode, 0) + self.assertNotRun(res) + + +# -------------------------------------------------------------------------- +# 2. The wait is bounded and its expiry has its own status. +# -------------------------------------------------------------------------- +class Timeout(Base): + def test_contended_lock_times_out_with_distinct_status(self): + with other_holder(self.lock): + res = run(["--lock", self.lock, "--timeout", "0.5", + "--", *self.touch_cmd()], timeout=30) + self.assertEqual(res.returncode, EXIT_TIMEOUT, res.stderr) + self.assertNotRun(res) + self.assertIn("gpu-lock: === TIMEOUT ===", res.stderr) + self.assertEqual(value(block(res.stderr, "TIMEOUT"), "outcome"), "TIMEOUT") + + def test_timeout_status_differs_from_refusal(self): + self.assertNotEqual(EXIT_TIMEOUT, EXIT_REFUSED) + + def test_wait_is_bounded_not_indefinite(self): + # A holder outliving the wrapper's patience by 20x. An unbounded flock + # here is how a job silently sits forever behind a dead holder. + with other_holder(self.lock, seconds=30): + start = time.time() + res = run(["--lock", self.lock, "--timeout", "1", + "--", *self.touch_cmd()], timeout=20) + waited = time.time() - start + self.assertEqual(res.returncode, EXIT_TIMEOUT, res.stderr) + self.assertLess(waited, 15, "the wrapper waited far past its timeout") + + def test_lock_is_actually_taken_so_a_holder_excludes_us(self): + # The #587 defect in miniature: if the wrapper does not really take the + # lock it will run happily beside a job that holds it. + with other_holder(self.lock): + res = run(["--lock", self.lock, "--timeout", "0.5", + "--", *self.touch_cmd()], timeout=30) + self.assertNotRun(res) + + def test_uncontended_lock_is_acquired_immediately(self): + res = run(["--lock", self.lock, "--timeout", "5", "--", *self.touch_cmd()]) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertTrue(os.path.exists(self.marker)) + + +# -------------------------------------------------------------------------- +# 3. It records: the stamped evidence. +# -------------------------------------------------------------------------- +class Stamp(Base): + def acquire(self, *extra, env=None): + res = run(["--lock", self.lock, *extra, "--", "true"], env=env) + self.assertEqual(res.returncode, 0, res.stderr) + return res + + def test_acquire_block_carries_every_required_field(self): + res = self.acquire("--timeout", "5") + pairs = block(res.stderr, "ACQUIRE") + keys = [k for k, _ in pairs] + for required in ("mode", "lock-path", "holder-pid", "waited-seconds", + "timeout-seconds", "loadavg", "df-root", "command"): + self.assertIn(required, keys, f"missing {required}; got {keys}") + + def test_lock_path_is_absolute(self): + res = self.acquire() + self.assertTrue(value(block(res.stderr, "ACQUIRE"), "lock-path").startswith("/")) + + def test_relative_lock_path_is_stamped_absolute(self): + res = run(["--lock", "gpu.lock", "--", "true"], cwd=self.tmp) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "lock-path"), self.lock) + + def test_symlinked_lock_path_is_stamped_resolved(self): + # #587 made visible: two spellings of one file, or one spelling of two + # files, are only distinguishable once the RESOLVED path is in the record. + real = os.path.join(self.tmp, "real") + os.mkdir(real) + link = os.path.join(self.tmp, "link") + os.symlink(real, link) + res = run(["--lock", os.path.join(link, "gpu.lock"), "--", "true"]) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "lock-path"), + os.path.join(real, "gpu.lock")) + + def test_default_lock_path_is_home_gpu_lock(self): + res = run(["--", "true"], env={"HOME": self.tmp}) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "lock-path"), self.lock) + + def test_gpu_lock_env_overrides_the_default(self): + other = os.path.join(self.tmp, "elsewhere.lock") + res = run(["--", "true"], env={"HOME": self.tmp, "GPU_LOCK": other}) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "lock-path"), other) + + def test_explicit_lock_flag_overrides_the_env(self): + other = os.path.join(self.tmp, "elsewhere.lock") + res = run(["--lock", self.lock, "--", "true"], + env={"HOME": self.tmp, "GPU_LOCK": other}) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "lock-path"), self.lock) + + def test_holder_pid_is_a_live_process(self): + res = self.acquire() + pid = value(block(res.stderr, "ACQUIRE"), "holder-pid") + self.assertRegex(pid, r"^\d+$") + self.assertGreater(int(pid), 0) + + def test_waited_seconds_is_numeric(self): + res = self.acquire() + self.assertRegex(value(block(res.stderr, "ACQUIRE"), "waited-seconds"), + r"^\d+\.\d+$") + + def test_waited_seconds_reflects_a_real_wait(self): + with other_holder(self.lock, seconds=2): + res = run(["--lock", self.lock, "--timeout", "20", "--", "true"], + timeout=40) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertGreater(float(value(block(res.stderr, "ACQUIRE"), + "waited-seconds")), 0.5) + + def test_df_root_is_the_real_root_filesystem_row(self): + res = self.acquire() + df = value(block(res.stderr, "ACQUIRE"), "df-root") + self.assertTrue(df.endswith(" /"), f"not a `df -h /` data row: {df!r}") + self.assertRegex(df, r"\d+%") + + def test_loadavg_has_three_numbers(self): + res = self.acquire() + nums = value(block(res.stderr, "ACQUIRE"), "loadavg").split() + self.assertEqual(len(nums), 3) + for n in nums: + float(n) + + def test_command_is_recorded(self): + res = run(["--lock", self.lock, "--", "sh", "-c", "exit 0"]) + self.assertIn("sh", value(block(res.stderr, "ACQUIRE"), "command")) + + def test_record_file_receives_the_same_stamp(self): + rec = os.path.join(self.tmp, "evidence.txt") + res = run(["--lock", self.lock, "--record", rec, "--", "true"]) + self.assertEqual(res.returncode, 0, res.stderr) + text = Path(rec).read_text() + self.assertIn("=== ACQUIRE ===", text) + self.assertIn("=== RELEASE ===", text) + self.assertIn(f"lock-path={self.lock}", text) + self.assertIn("exit-code=0", text) + + def test_record_file_is_appended_not_truncated(self): + rec = os.path.join(self.tmp, "evidence.txt") + Path(rec).write_text("EARLIER-LEG\n") + run(["--lock", self.lock, "--record", rec, "--", "true"]) + self.assertIn("EARLIER-LEG", Path(rec).read_text()) + + def test_stdout_of_the_wrapped_command_is_not_polluted(self): + res = run(["--lock", self.lock, "--", "sh", "-c", "echo PAYLOAD"]) + self.assertEqual(res.stdout, "PAYLOAD\n") + + +# -------------------------------------------------------------------------- +# 4. The exit code comes FIRST, and comes through unchanged. +# -------------------------------------------------------------------------- +class ExitCode(Base): + def release(self, res): + return block(res.stderr, "RELEASE") + + def test_zero_propagates(self): + res = run(["--lock", self.lock, "--", "sh", "-c", "exit 0"]) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertEqual(value(self.release(res), "exit-code"), "0") + + def test_nonzero_propagates_unchanged(self): + res = run(["--lock", self.lock, "--", "sh", "-c", "exit 42"]) + self.assertEqual(res.returncode, 42, res.stderr) + self.assertEqual(value(self.release(res), "exit-code"), "42") + + def test_sigkill_137_propagates_unchanged(self): + # The case that matters most: 137 says another agent's `pkill` killed the + # run, not that the code is wrong. A wrapper that flattens it to 1 turns + # "not mine" into "mine". + res = run(["--lock", self.lock, "--", "sh", "-c", "kill -9 $$"]) + self.assertEqual(res.returncode, 137, res.stderr) + self.assertEqual(value(self.release(res), "exit-code"), "137") + + def test_sigkill_is_named_in_the_record(self): + res = run(["--lock", self.lock, "--", "sh", "-c", "kill -9 $$"]) + reason = value(self.release(res), "exit-reason") or "" + self.assertIn("9", reason) + self.assertIn("signal", reason.lower()) + + def test_missing_executable_propagates_127(self): + res = run(["--lock", self.lock, "--", "definitely-not-a-real-binary-587"]) + self.assertEqual(res.returncode, 127, res.stderr) + self.assertEqual(value(self.release(res), "exit-code"), "127") + + def test_exit_code_is_the_first_field_of_the_release_block(self): + # Order is the requirement, not just presence: disk and load say the box + # was unhealthy, the exit code says WHICH thing killed the run. + res = run(["--lock", self.lock, "--", "sh", "-c", "exit 3"]) + pairs = self.release(res) + self.assertTrue(pairs, f"no RELEASE block:\n{res.stderr}") + self.assertEqual(pairs[0][0], "exit-code", + f"first RELEASE field was {pairs[0][0]}") + + def test_release_block_repeats_the_lock_path_and_health(self): + res = run(["--lock", self.lock, "--", "true"]) + keys = [k for k, _ in self.release(res)] + for required in ("exit-code", "outcome", "lock-path", "elapsed-seconds", + "loadavg-at-exit", "df-root-at-exit"): + self.assertIn(required, keys, f"missing {required}; got {keys}") + + def test_ran_outcome_is_distinguishable_from_a_wrapper_failure(self): + # A wrapped command exiting 75 must not read as the wrapper timing out. + res = run(["--lock", self.lock, "--", "sh", "-c", "exit 75"]) + self.assertEqual(res.returncode, EXIT_TIMEOUT) + self.assertEqual(value(self.release(res), "outcome"), "RAN") + self.assertNotIn("=== TIMEOUT ===", res.stderr) + + +# -------------------------------------------------------------------------- +# 5. It does not self-deadlock. +# -------------------------------------------------------------------------- +class Nesting(Base): + def nested(self, *extra): + inner = [str(SCRIPT), "--lock", self.lock, "--timeout", "2", + "--", *self.touch_cmd()] + return run(["--lock", self.lock, "--timeout", "2", *extra, + "--", *inner], timeout=45) + + def test_nested_invocation_does_not_deadlock(self): + res = self.nested() + self.assertEqual(res.returncode, 0, res.stderr) + self.assertTrue(os.path.exists(self.marker), res.stderr) + + def test_nested_invocation_is_stamped_as_a_pass_through(self): + res = self.nested() + self.assertIn("PASS-THROUGH", res.stderr) + + def test_outer_invocation_is_stamped_as_an_acquire(self): + res = run(["--lock", self.lock, "--", "true"]) + self.assertEqual(value(block(res.stderr, "ACQUIRE"), "mode"), "ACQUIRED") + + def test_a_different_lock_still_acquires(self): + # Pass-through keys on the RESOLVED path, so an unrelated second lock is + # taken for real rather than waved through. + other = os.path.join(self.tmp, "other.lock") + inner = [str(SCRIPT), "--lock", other, "--timeout", "2", "--", "true"] + res = run(["--lock", self.lock, "--timeout", "2", "--", *inner], timeout=45) + self.assertEqual(res.returncode, 0, res.stderr) + self.assertIn(f"lock-path={other}", res.stderr) + + +# -------------------------------------------------------------------------- +# 6. Mutations: prove the checks above catch their defect. +# -------------------------------------------------------------------------- +class Mutations(unittest.TestCase): + """Break one guarantee at a time in a scratch copy; the check must go red. + + A test that merely describes a property is not evidence the property is + enforced. Restoring the tree is free here because nothing outside the + temporary directory is written. + """ + + @classmethod + def setUpClass(cls): + if not has_flock(): + raise unittest.SkipTest("flock(1) is required") + if os.geteuid() == 0: + raise unittest.SkipTest("root ignores the mode bits the refusal rests on") + cls.source = SCRIPT.read_text() + + def mutant(self, transform): + text = transform(self.source) + self.assertNotEqual(text, self.source, "mutation matched nothing") + d = tempfile.mkdtemp(prefix="gpulock-mut-") + self.addCleanup(shutil.rmtree, d, True) + path = Path(d) / "gpu-lock.sh" + path.write_text(text) + path.chmod(0o755) + return path + + def check(self, path, cls, *names): + """Run named tests from a guarantee class against a mutant wrapper.""" + global SCRIPT + saved = SCRIPT + SCRIPT = path + try: + suite = unittest.TestSuite(cls(n) for n in names) + with open(os.devnull, "w") as quiet: + result = unittest.TextTestRunner(stream=quiet, verbosity=0).run(suite) + return len(result.failures) + len(result.errors) + finally: + SCRIPT = saved + + # -- M1: the refusal falls through instead of aborting ------------------- + def test_M1_refusal_that_returns_instead_of_exiting_is_caught(self): + m = self.mutant(lambda s: s.replace('exit "$EXIT_REFUSED"', 'return 0', 1)) + self.assertGreater( + self.check(m, Refusal, + "test_unwritable_lock_directory_refuses", + "test_missing_lock_parent_refuses"), 0) + + # -- M2: the lock actually taken is not recorded ------------------------- + def test_M2_dropping_the_lock_path_field_is_caught(self): + m = self.mutant(lambda s: "\n".join( + l for l in s.splitlines() if "lock-path=" not in l) + "\n") + self.assertGreater( + self.check(m, Stamp, + "test_acquire_block_carries_every_required_field", + "test_symlinked_lock_path_is_stamped_resolved"), 0) + + # -- M3: the exit code is not recorded ----------------------------------- + def test_M3_dropping_the_exit_code_field_is_caught(self): + m = self.mutant(lambda s: "\n".join( + l for l in s.splitlines() if "exit-code=" not in l) + "\n") + self.assertGreater( + self.check(m, ExitCode, + "test_exit_code_is_the_first_field_of_the_release_block", + "test_nonzero_propagates_unchanged"), 0) + + # -- M4: the exit code is swallowed -------------------------------------- + def test_M4_swallowing_the_exit_code_is_caught(self): + m = self.mutant(lambda s: s.replace('exit "$RC"', "exit 0", 1)) + self.assertGreater( + self.check(m, ExitCode, + "test_nonzero_propagates_unchanged", + "test_sigkill_137_propagates_unchanged"), 0) + + # -- M5: timeout stops being a distinct status --------------------------- + def test_M5_collapsing_the_timeout_status_is_caught(self): + m = self.mutant(lambda s: s.replace("EXIT_TIMEOUT=75", "EXIT_TIMEOUT=0", 1)) + self.assertGreater( + self.check(m, Timeout, + "test_contended_lock_times_out_with_distinct_status"), 0) + + # -- M6: the lock is never actually taken -------------------------------- + def test_M6_skipping_the_acquisition_is_caught(self): + m = self.mutant(lambda s: s.replace( + 'flock -w "$TIMEOUT" "$LOCK_FD"', "true", 1)) + self.assertGreater( + self.check(m, Timeout, + "test_lock_is_actually_taken_so_a_holder_excludes_us", + "test_contended_lock_times_out_with_distinct_status"), 0) + + # -- M7: the wait becomes unbounded -------------------------------------- + def test_M7_unbounded_wait_is_caught(self): + m = self.mutant(lambda s: s.replace( + 'flock -w "$TIMEOUT" "$LOCK_FD"', 'flock "$LOCK_FD"', 1)) + self.assertGreater( + self.check(m, Timeout, "test_wait_is_bounded_not_indefinite"), 0) + + # -- M8: nesting self-deadlocks ------------------------------------------ + def test_M8_removing_the_pass_through_is_caught(self): + m = self.mutant(lambda s: s.replace( + '[ "${VLLM_CPP_GPU_LOCK_HELD:-}" = "$LOCK_PATH" ]', "false", 1)) + self.assertGreater( + self.check(m, Nesting, "test_nested_invocation_does_not_deadlock"), 0) + + +if __name__ == "__main__": + unittest.main(verbosity=2)