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)