diff --git a/scripts/hostlock.sh b/scripts/hostlock.sh index dfe46968b9..3091940aa9 100755 --- a/scripts/hostlock.sh +++ b/scripts/hostlock.sh @@ -142,6 +142,11 @@ # wrapped command that exits 6 by itself, exactly as 5 collides with the # gate: `run` multiplexes two status spaces onto one, and the `cpu ... # verdict=` line, not the exit code, is what tells them apart. +# 7 lock dir unusable -- no lock can be created at LOCK_DIR on this host, so +# the answer is neither "yours" nor "somebody else's". It is separate from +# 1 because a misconfigured or unwritable host is not a bad argument and +# must not be retried as one, and separate from 2 and 3 because those two +# assert a peer holds the box. See lock_dir_problem. # # `run` otherwise does NOT use this table: it returns the wrapped command's # own status, so a command exiting 5 is indistinguishable from a gate failure. @@ -922,8 +927,75 @@ publish_lock() { # reads STALE by the liveness test and BUSY by `acquire`, and of those two # the reassuring one is STALE: it tells a human the box is abandoned, and # they take it. Report what the tool will actually do. +# Echo why no lock can be taken at LOCK_DIR and return 0; return 1 when one can. +# +# FREE is not a fact about the directory, it is a promise to the caller: "the +# host is yours, go ahead". On a host where the lock cannot be CREATED that +# promise is false, and until this existed `status` made it anyway -- printing +# `FREE (runnable=4)`, and `state=FREE` in porcelain, in bytes IDENTICAL to a +# genuinely free and usable host. That is the same defect `warn_if_private` +# was written for one level up (#1942): output that is not wrong about what it +# measured, and silent about what it measured. +# +# It is not hypothetical here. This box's default lock_dir is under /tmp, and +# at least one agent on it is under a hard prohibition on writing /tmp at all; +# `acquire` failed for that agent with a raw `mkdir: Permission denied` and +# exit 1 -- "usage/error", the code for a bad argument -- while `status` went +# on saying FREE. An unattended harness reading either one proceeds unlocked, +# which is precisely what the lock became mandatory to prevent. +# +# What is tested is the PARENT, never LOCK_DIR itself: publish_lock stages at +# `${LOCK_DIR}.stage.$$`, a SIBLING, and renames it into place. A lock dir that +# exists but is unwritable is therefore still perfectly publishable, and an +# absent one whose parent is writable is fine too -- so the question is always +# "can we create entries next to it", answered at the nearest ancestor that +# exists, because `mkdir -p` will make the rest. +# +# `-w` lies under CAP_DAC_OVERRIDE, so root sees "usable" where a mortal would +# not. That is the safe direction: it is exactly today's behaviour, and root +# can in fact write there. +lock_dir_problem() { + local p + if [ -e "$LOCK_DIR" ] && [ ! -d "$LOCK_DIR" ]; then + printf '%s\n' "${LOCK_DIR} exists and is not a directory" + return 0 + fi + case "$LOCK_DIR" in + */*) p=${LOCK_DIR%/*}; [ -n "$p" ] || p=/ ;; + *) p=. ;; + esac + while [ ! -e "$p" ]; do + case "$p" in + */*) p=${p%/*}; [ -n "$p" ] || p=/ ;; + *) p=. ; break ;; + esac + done + if [ ! -d "$p" ]; then + printf '%s\n' "${p} exists and is not a directory" + return 0 + fi + if [ ! -w "$p" ] || [ ! -x "$p" ]; then + printf '%s\n' "${p} is not writable by $(id -un 2>/dev/null || echo "uid $(id -u 2>/dev/null)")" + return 0 + fi + return 1 +} + +# Say the UNUSABLE part out loud, on stderr, wherever a caller was about to be +# told the host is available. Three lines, because one is not enough to stop an +# agent that has been told the lock is mandatory and has just read "free". +explain_unusable() { + local problem=$1 + echo "hostlock: UNUSABLE: ${problem}" >&2 + echo "hostlock: no lock can be created at ${LOCK_DIR} (${LOCK_DIR_SOURCE}, ${LOCK_SCOPE}), so this host cannot participate." >&2 + echo "hostlock: set lock_dir in ${HOSTLOCK_CONF_PATH} to a path you can write; do NOT run saturating benchmarks unlocked." >&2 +} + lock_state() { - [ -d "$LOCK_DIR" ] || { echo FREE; return 0; } + if [ ! -d "$LOCK_DIR" ]; then + if lock_dir_problem >/dev/null; then echo UNUSABLE; else echo FREE; fi + return 0 + fi # unverifiable_live_anchor is the class this tool cannot verify but can # SEE running. Routing it through the STALE arm made `status` say "is # gone" about a pid the reaper had just confirmed present -- the same @@ -1100,6 +1172,22 @@ status_lock_dir_note() { return 0 } +print_free_or_unusable() { + local problem + if problem=$(lock_dir_problem); then + echo "UNUSABLE ${problem}" + # Exactly one lock line either way: status_lock_dir_note prints it for + # every non-default source, and the default source is the case that + # needs it most -- /tmp is where this actually bites. + [ "$LOCK_DIR_SOURCE" = default ] && echo " lock: ${LOCK_DIR} (${LOCK_DIR_SOURCE}, ${LOCK_SCOPE})" + echo " no lock can be created here; this host cannot participate" + echo " runnable=$(runnable_now)" + return 0 + fi + echo "FREE (runnable=$(runnable_now))" + return 0 +} + cmd_status() { local legacy legacy=$(legacy_holder) || legacy="none" @@ -1109,6 +1197,11 @@ cmd_status() { echo "lock_dir=${LOCK_DIR}" echo "lock_scope=${LOCK_SCOPE}" echo "lock_dir_source=${LOCK_DIR_SOURCE}" + # Always emitted, empty when there is none, so the key set does not + # change shape between hosts -- a consumer that has to test for a key's + # PRESENCE to learn the state is one `grep` away from reading absence + # as "fine", which is the failure this whole field exists to report. + echo "lock_dir_problem=$(lock_dir_problem || true)" echo "legacy_dir=$(legacy_consult_path)" echo "legacy_held_by=${legacy%% *}" if [ -d "$LOCK_DIR" ]; then @@ -1121,7 +1214,7 @@ cmd_status() { return 0 fi if [ ! -d "$LOCK_DIR" ]; then - echo "FREE (runnable=$(runnable_now))" + print_free_or_unusable status_lock_dir_note "$legacy" return 0 fi @@ -1157,7 +1250,7 @@ cmd_status() { *) # lock_state saw no lock dir; it was released under us since the # test above. Report what is true now rather than a stale label. - echo "FREE (runnable=$(runnable_now))" + print_free_or_unusable ;; esac status_lock_dir_note "$legacy" @@ -1178,7 +1271,18 @@ abandon_lock() { } cmd_acquire() { - local deadline=$((SECONDS + TIMEOUT)) rc + local deadline=$((SECONDS + TIMEOUT)) rc problem + # Refuse BEFORE anything else, including the legacy consult. An acquire + # that cannot possibly publish must not spend --timeout looking busy: with + # --wait (which `run` always sets) the unusable host reported "timed out + # after 900s waiting for the lock", i.e. it blamed peers for contention + # that did not exist, and returned 3 -- a code whose documented meaning is + # that somebody else has the box. Distinct outcomes, or the label launders + # the fault. + if problem=$(lock_dir_problem); then + explain_unusable "$problem" + return 7 + fi refuse_if_legacy_held || return $? while :; do if publish_lock; then @@ -1325,7 +1429,14 @@ cmd_release() { } cmd_wait() { - local deadline=$((SECONDS + TIMEOUT)) + local deadline=$((SECONDS + TIMEOUT)) problem + # An unusable lock dir is never HELD, so the loop below falls straight + # through and announces "free" -- the one word this caller is waiting to + # hear, on the one host where it cannot be true. + if problem=$(lock_dir_problem); then + explain_unusable "$problem" + return 7 + fi # Wait only while somebody actually has a live claim. An EXPIRED lock is # one the next acquirer would take over, so blocking on it would disagree # with what `status` and `acquire` both say. diff --git a/scripts/hostlock_test.sh b/scripts/hostlock_test.sh index b28b94e5c2..a75b4e41d1 100755 --- a/scripts/hostlock_test.sh +++ b/scripts/hostlock_test.sh @@ -60,7 +60,7 @@ HL=scripts/hostlock.sh pass=0 fail=0 -cleanup() { rm -rf "$LOCK" "$LOCK".cpuself "$LOCK".reaper "$LOCK".reaper.stage.* "$LOCK".reaper.dead.* "$LOCK".reaper.rel.* "$LOCK".dead.* "$LOCK".stage.* "$LOCK".gate "$LOCK".warn "$LOCK".zombie.* "$LOCK".zpid "$LOCK".ttlmarker "$LOCK".ran "$LOCK".conf "$LOCK".legacy "$LOCK".box "$LOCK".sourced "$LOCK".legacyran "$LOCK".owner "$LOCK".nested 2>/dev/null; } +cleanup() { rm -rf "$LOCK" "$LOCK".cpuself "$LOCK".reaper "$LOCK".reaper.stage.* "$LOCK".reaper.dead.* "$LOCK".reaper.rel.* "$LOCK".dead.* "$LOCK".stage.* "$LOCK".gate "$LOCK".warn "$LOCK".zombie.* "$LOCK".zpid "$LOCK".ttlmarker "$LOCK".ran "$LOCK".conf "$LOCK".legacy "$LOCK".box "$LOCK".sourced "$LOCK".legacyran "$LOCK".owner "$LOCK".nested "$LOCK".ro "$LOCK".deep "$LOCK".file "$LOCK".nox "$LOCK".conf2 2>/dev/null; } trap cleanup EXIT chk() { @@ -2070,13 +2070,184 @@ chk "and that place is cmd_run, which is the only one with a child" \ rm -rf "$LOCK.nested" cleanup +# == a host that cannot take the lock must say so, not say FREE == +# +# FREE is read as "the host is yours, go ahead". Where the lock directory +# cannot be created that reading is false, and the tool used to make it in +# three measured ways. All six lines below are what `origin/main` does, run +# against these same two fixtures rather than reasoned about: +# +# unwritable parent (chmod 500): +# status FREE (runnable=4) rc 0 <-- fail open +# status --porcelain state=FREE rc 0 <-- fail open +# wait hostlock: free rc 0 <-- fail open +# acquire / run mkdir: ... Permission denied rc 1 <-- refuses, but +# as "usage/error" +# lock path is a plain file: +# acquire --wait timed out after 12s waiting for the lock, AND +# prints FREE in the same breath rc 3 +# +# The first three tell a harness to proceed on the one host where the lock is +# unavailable -- `status` is the README's own "is anybody benchmarking?" and +# `wait` exists to be followed by a run. The fourth is a correct refusal with +# a wrong classification: exit 1 is the code for a bad argument, so an +# unattended caller cannot tell a broken host from its own typo. The fifth is +# worse than either, because the box is idle and the tool spends the whole +# --timeout before blaming peers for contention that does not exist -- while +# printing FREE underneath it. +# +# This matters beyond the fixtures: the lock is mandatory for saturating runs, +# and the default lock_dir is under /tmp, which not every host can use. +UNUSABLE_PARENT="$LOCK.ro" +rm -rf "$UNUSABLE_PARENT" +mkdir -p "$UNUSABLE_PARENT" +chmod 500 "$UNUSABLE_PARENT" +UNUSABLE_DIR="$UNUSABLE_PARENT/hl" + +chk "status calls an uncreatable lock dir UNUSABLE, not FREE" \ + "$(HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" status 2>/dev/null | head -1 | awk '{print $1}')" \ + "UNUSABLE" +chk "and porcelain says so in the state field" \ + "$(HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" status --porcelain 2>/dev/null | sed -n 's/^state=//p')" \ + "UNUSABLE" +chk "porcelain names the reason rather than only the verdict" \ + "$(HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" status --porcelain 2>/dev/null | sed -n 's/^lock_dir_problem=//p' | grep -c "$UNUSABLE_PARENT")" \ + "1" +# The negative control for the three above, and the reason the key is emitted +# unconditionally: a consumer that learns the state from the key's PRESENCE +# reads absence as "fine", which is the failure the field exists to report. +chk "a usable free host emits the same key, empty" \ + "$(HOSTLOCK_DIR="$LOCK" "$HL" status --porcelain 2>/dev/null | grep -c '^lock_dir_problem=$')" \ + "1" +chk "and still calls itself FREE" \ + "$(HOSTLOCK_DIR="$LOCK" "$HL" status 2>/dev/null | head -1 | awk '{print $1}')" \ + "FREE" + +# Both halves of the guard, separately. A directory that is writable but not +# SEARCHABLE cannot host entries either -- `mkdir 0600-parent/sub` fails with +# EACCES -- and every other fixture in this section is chmod 500, i.e. not +# writable but executable, which exercises only the `-w` clause. Without this +# cell, deleting `|| [ ! -x "$p" ]` is a one-line fail-open that the whole +# suite stays green through. +NOSEARCH_PARENT="$LOCK.nox" +rm -rf "$NOSEARCH_PARENT" +mkdir -p "$NOSEARCH_PARENT" +chmod 600 "$NOSEARCH_PARENT" +chk "a writable but unsearchable parent is UNUSABLE too" \ + "$(HOSTLOCK_DIR="$NOSEARCH_PARENT/hl" "$HL" status --porcelain 2>/dev/null | sed -n 's/^state=//p')" \ + "UNUSABLE" +chmod 700 "$NOSEARCH_PARENT" +rm -rf "$NOSEARCH_PARENT" + +HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" acquire --owner leon >/dev/null 2>&1 +chk "acquire exits 7, not 1 (a bad host is not a bad argument)" "$?" "7" +HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" wait >/dev/null 2>&1 +chk "wait exits 7 instead of announcing free" "$?" "7" +chk "and prints no free announcement at all" \ + "$(HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" wait 2>/dev/null | grep -c free)" \ + "0" +chk "the refusal names the path, so it is actionable" \ + "$(HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" acquire --owner leon 2>&1 >/dev/null | grep -c "$UNUSABLE_DIR")" \ + "1" + +rm -f "$LOCK.ran" +HOSTLOCK_DIR="$UNUSABLE_DIR" "$HL" run --owner leon -- touch "$LOCK.ran" >/dev/null 2>&1 +chk "run exits 7" "$?" "7" +chk "and does not run the command" \ + "$([ -e "$LOCK.ran" ] && echo ran || echo refused)" "refused" +rm -f "$LOCK.ran" + +# A path that exists as a FILE can never become a lock dir, and this is the +# fixture where the pre-fix loop is reachable: the staging dir is a SIBLING, +# so it is created fine, and only the final `mv -T` onto the file fails -- +# which publish_lock reports the same way it reports "somebody got there +# first". So the old code looped, slept, and after --timeout announced +# contention on an idle box. Timed as well as coded, because a refusal that +# takes --timeout to arrive is still the defect: `run` sets --wait +# unconditionally, so this did not fail, it HUNG. +: >"$LOCK.file" +chk "a lock path that is a plain file is UNUSABLE" \ + "$(HOSTLOCK_DIR="$LOCK.file" "$HL" status --porcelain 2>/dev/null | sed -n 's/^state=//p')" \ + "UNUSABLE" +t0=$SECONDS +HOSTLOCK_DIR="$LOCK.file" "$HL" acquire --wait --timeout 30 --owner leon >/dev/null 2>&1 +rc=$? +chk "acquire --wait refuses instead of waiting out the timeout" "$rc" "7" +chk "and refuses promptly rather than blaming peers for 30s" \ + "$([ "$((SECONDS - t0))" -lt 5 ] && echo prompt || echo slow)" "prompt" +rm -f "$LOCK.file" + +# The check is on the PARENT, never the leaf. publish_lock stages at a SIBLING +# of LOCK_DIR and renames it into place, so "can we create entries next to it" +# is the real question -- and `mkdir -p` will build any missing intermediate +# levels, so a deep path under a writable ancestor is usable too. A guard that +# demanded the immediate parent exist would refuse this host wrongly, which is +# the mirror-image failure: a lock that refuses a usable box is a lock nobody +# keeps using. +DEEP="$LOCK.deep/a/b/hl" +rm -rf "$LOCK.deep" +chk "a deep path under a writable ancestor is usable" \ + "$(HOSTLOCK_DIR="$DEEP" "$HL" status --porcelain 2>/dev/null | sed -n 's/^state=//p')" \ + "FREE" +HOSTLOCK_DIR="$DEEP" "$HL" run --owner leon -- true >/dev/null 2>&1 +chk "and can actually be acquired, mkdir -p building the rest" "$?" "0" +rm -rf "$LOCK.deep" + +# Usability must not leak into the occupied path: a HELD lock is HELD whatever +# the directory's mode says, or the state that stops a second agent taking the +# box would be answerable by a chmod. +"$HL" acquire --owner leon-held --ttl 60 >/dev/null 2>&1 +chk "a held lock still reads HELD, not UNUSABLE" \ + "$("$HL" status --porcelain 2>/dev/null | sed -n 's/^state=//p')" "HELD" + +# The sharp end of "the parent, never the leaf", and the cell that a mutant +# testing LOCK_DIR itself survives without: a lock dir that EXISTS and is +# unwritable is still perfectly publishable (the stage is a sibling), and the +# honest answer for a second agent is BUSY -- somebody has the box. Reporting +# UNUSABLE there would relabel real contention as a broken host, which is the +# most expensive direction to get this wrong: it tells the agent to go fix its +# config and try again, while a peer's benchmark is running. +chmod 500 "$LOCK" +"$HL" acquire --owner leon-other >/dev/null 2>&1 +chk "a held-but-unwritable lock dir is BUSY, not UNUSABLE" "$?" "2" +chmod 700 "$LOCK" +"$HL" release >/dev/null 2>&1 +rm -rf "$LOCK" + +# Ordering pin. The preflight has to run BEFORE the legacy consult, and that +# ordering is invisible to every cell above: they all set HOSTLOCK_DIR, so +# LOCK_DIR_SOURCE is `env`, and refuse_if_legacy_held returns immediately for +# any source but `config`. Swap the two and the whole suite stays green while +# a host that cannot take the lock at all is told a peer has the box -- exit 2 +# instead of 7, which sends the agent away to wait for a lock it could never +# have acquired. Raised by review as the one surviving mutation; this is the +# fixture that reddens it. +rm -rf "$LOCK.legacy" +printf 'lock_dir=%s\n' "$UNUSABLE_DIR" >"$LOCK.conf2" +sleep 300 & +unusable_legacy_pid=$! +unusable_legacy_start=$(sed 's/.*) //' "/proc/${unusable_legacy_pid}/stat" | awk '{print $20}') +mkdir -p "$LOCK.legacy" +printf 'owner=roy\nanchor_pid=%s\nstart_time=%s\nacquired_epoch=%s\nttl=0\nreason=prefill matrix\n' \ + "$unusable_legacy_pid" "$unusable_legacy_start" "$(date +%s)" >"$LOCK.legacy/meta" +chk "an unusable configured dir refuses as 7 even while the legacy path is live" \ + "$(env -u HOSTLOCK_DIR HOSTLOCK_CONF="$LOCK.conf2" HOSTLOCK_LEGACY_DIR="$LOCK.legacy" \ + "$HL" acquire --owner leon >/dev/null 2>&1; echo $?)" "7" +kill "$unusable_legacy_pid" 2>/dev/null +wait "$unusable_legacy_pid" 2>/dev/null +rm -rf "$LOCK.legacy" "$LOCK.conf2" + +chmod 700 "$UNUSABLE_PARENT" 2>/dev/null +rm -rf "$UNUSABLE_PARENT" +cleanup + # Finally, pin the assertion count itself. Two of the checks in this file sit # behind environment probes, and an assertion that quietly stops running is # indistinguishable from one that passes -- which is the same failure mode as # the inert R1 block and the vacuous STALE arm that this PR exists to fix. # Both probe branches now assert something, so the total is invariant across # environments; if a refactor drops a check, this fails and says so. -chk "every assertion in this file ran" "$((pass + fail + 1))" "321" +chk "every assertion in this file ran" "$((pass + fail + 1))" "341" echo echo "passed=${pass} failed=${fail}" diff --git a/scripts/ort_ab/README.md b/scripts/ort_ab/README.md index a8c77fe83f..43012dfc6c 100644 --- a/scripts/ort_ab/README.md +++ b/scripts/ort_ab/README.md @@ -236,7 +236,17 @@ never reaped, which still resolves in `/proc`. The same is true of the internal guard that serialises reclaiming, so there is no state a kill can leave that requires a human with `rm -rf`. If you want to see it before trusting it: `hostlock.sh status` distinguishes FREE / HELD / STALE / -EXPIRED, and `provenance` prints who holds it and since when. +EXPIRED / UNUSABLE, and `provenance` prints who holds it and since when. + +`UNUSABLE` is the answer that is neither "yours" nor "somebody else's": no +lock can be **created** at the configured path on this host, so nobody here +can participate. `status` names the reason (also `lock_dir_problem=` in +`--porcelain`, always emitted and empty when there is none), and `acquire`, +`wait` and `run` refuse with exit **7** — distinct from 1, because a +misconfigured host is not a bad argument, and distinct from 2 and 3, which +both assert that a peer holds the box. `run` refuses *without* running your +command, which is the whole point: the failure it replaces was a host that +reported `FREE`, ran the benchmark unlocked, and said nothing. ### Where the lock lives @@ -250,6 +260,11 @@ mkdir -p ~/.config/onnx-genai echo 'lock_dir=/var/lib/onnx-genai/hostlock' > ~/.config/onnx-genai/hostlock.conf ``` +You will be told when you need this rather than having to guess: on a host +where the path cannot be created, `status` reports `UNUSABLE` with the reason +and `acquire`/`run`/`wait` exit 7. Until that existed the same host reported +`FREE` and ran unlocked. + `$HOSTLOCK_DIR` also moves the path and is **not** the same thing: it is set per process, so it does not move your peers with you. It is a **private** lock. It acquires instantly every time, collides with nobody, and — before this was