From e1a78115543210f74116197eba6a01e2b1e24b90 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 21:26:20 -0700 Subject: [PATCH 1/2] fix(lock): break a registry lock whose recorded holder is gone (#865) The acquire loop never asked whether the holder was still running. A lock left by a killed process was indistinguishable from one held by a live process doing slow work, so every later command waited out its budget and failed - forever, until somebody removed a directory by hand, working out which of several causes it was from a message that named none of them. Observed on a machine 8 minutes after boot under load average 8: two lock directories six seconds apart, still there three hours later. The widest source of these is roster-sync-driver.sh, which acquires and then runs node in the foreground: the lock is held for as long as that runs, so one kill -9 or one OOM kill in that span leaves a lock that does have a holder record. Those are the ones this breaks. Liveness comes from _agmsg_pid_alive_local, not a bare kill -0, which reads EPERM as dead and would take a live lock away in a sandbox. A recycled pid reads as alive, which is the safe direction: this waits and reports contention rather than breaking a stranger's lock. The break claims the holder file by renaming it before removing the directory, so two processes reaching the same verdict cannot both remove - and a lock that changed hands between the verdict and the removal is not the file that was claimed. Not in this change, and pinned as cases so they stay decisions: a lock with no holder record at all is left alone, because nothing here can tell an older version's lock from one being created right now. Also the prose above the traps, which said a crash leaves no stale lock. True of EXIT/INT/TERM, not of SIGKILL, an OOM kill, or the machine going down - which is the crash this issue was reported from. --- scripts/lib/registry-lock.sh | 100 +++++++++++++++++++++++++- tests/test_registry_lock.bats | 129 ++++++++++++++++++++++++++++++++++ 2 files changed, 227 insertions(+), 2 deletions(-) diff --git a/scripts/lib/registry-lock.sh b/scripts/lib/registry-lock.sh index 313257333..235574076 100644 --- a/scripts/lib/registry-lock.sh +++ b/scripts/lib/registry-lock.sh @@ -53,6 +53,70 @@ _agmsg_lock_describe_dir() { echo "agmsg: running as: $(id 2>/dev/null || echo 'unknown')" >&2 } +# LIVENESS, from the library that already answers this question (#865). +# +# `kill -0` on its own reads EPERM as dead, which in a sandbox turns "cannot +# signal" into "not running" — and here that would break a lock somebody is +# holding. `_agmsg_pid_alive_local` treats EPERM as alive, a zombie as gone, and +# cross-checks with `ps`. Sourced rather than reimplemented; guarded, so a +# caller that already has it pays nothing. +if ! declare -f _agmsg_pid_alive_local >/dev/null 2>&1; then + # shellcheck source=instance-id.sh + source "$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)/instance-id.sh" +fi + +# Is this lock's recorded holder gone? +# +# TRUE ONLY WHEN THERE IS SOMETHING TO ASK ABOUT. No holder file, or one with no +# pid in it, answers "no" — not because such a lock is healthy, but because +# nothing here can tell a lock written by an older version of this file from one +# created microseconds ago whose owner has not written its record yet. Breaking +# on "I cannot tell" would take a live lock away, which is worse than the leak. +# That case is #865's remaining half and is left for the change that makes an +# empty lock impossible in the first place. +# +# A recycled pid reads as ALIVE here, and that is the safe direction: this +# process waits and reports contention instead of breaking a lock whose number +# now belongs to a stranger. Fencing the number with a start time is the third +# piece of #865 and is not in this change. +_agmsg_lock_holder_gone() { + local lock="$1" pid + [ -f "$lock.holder" ] || return 1 + pid="$(sed -n 's/^pid //p' "$lock.holder" 2>/dev/null | head -1)" + [ -n "$pid" ] || return 1 + _agmsg_pid_alive_local "$pid" && return 1 + return 0 +} + +# Break a lock whose recorded holder is gone. +# +# CLAIMED BEFORE IT IS BROKEN, and the claim is the holder file itself. Two +# processes can reach the same verdict in the same instant; `mv` of a given file +# succeeds for exactly one of them, so exactly one goes on to remove the +# directory. The loser's `mv` fails and it re-evaluates — by which time the +# winner has either taken the lock or left the path free. +# +# The claim also binds the removal to the holder that was JUDGED. If the lock +# changed hands between the verdict and here, the file this renames is not the +# file that was read, the rename fails, and no `rmdir` lands on a lock somebody +# else has since taken. +_agmsg_lock_break_dead() { + local lock="$1" claimed="$lock.dead.$$.${RANDOM:-0}" + mv "$lock.holder" "$claimed" 2>/dev/null || return 1 + if ! rmdir "$lock" 2>/dev/null; then + # Not taking it after all, so the next reader must still find out who the + # lock says is holding it. + mv "$claimed" "$lock.holder" 2>/dev/null || true + return 1 + fi + # `rm` is not on every allow-listed PATH that takes this lock — the same + # reason the holder lives beside the directory rather than inside it. + if command -v rm >/dev/null 2>&1; then + rm -f "$claimed" 2>/dev/null || true + fi + return 0 +} + agmsg_lock_acquire() { local team_dir="$1" lock i=0 max="${AGMSG_LOCK_TRIES:-1000}" err="" local budget="${AGMSG_LOCK_SECONDS:-10}" started elapsed @@ -85,6 +149,18 @@ agmsg_lock_acquire() { _agmsg_lock_describe_dir "$team_dir" return 1 fi + # IS ANYBODY THERE? Until #865 this loop never asked. A lock whose holder + # had been killed was indistinguishable from one held by a live process + # doing slow work, so it waited out its budget and failed — every time, + # forever, and the team stayed wedged until somebody removed a directory by + # hand. The observed case is `roster-sync-driver.sh`, which acquires and + # then runs node in the foreground: the lock is held for as long as that + # runs, so one `kill -9`, one OOM kill or one force-quit in that span leaves + # a lock with a holder record and no holder. + if _agmsg_lock_holder_gone "$lock" && _agmsg_lock_break_dead "$lock"; then + echo "agmsg: broke a registry lock in $team_dir whose recorded holder is gone" >&2 + continue + fi i=$((i + 1)) elapsed=$(( $(date +%s) - started )) # Whichever bound arrives first, and the message says which — "1000 tries" @@ -100,6 +176,19 @@ agmsg_lock_acquire() { else echo "agmsg: timed out acquiring registry lock for $team_dir after $i attempts (${elapsed}s)" >&2 fi + # WHAT WAS HOLDING IT, in the same breath. Since #865 a timeout means one + # of exactly two things — a holder that answered as alive, or a lock this + # process could not account for and would not break — and which one + # decides where the operator looks next. Without this the message says + # "contention" for a lock that has no holder at all, which is the sentence + # that sent the last three diagnoses after processes that were not there. + if [ -f "$lock.holder" ]; then + echo "agmsg: the lock records: $(tr '\n' ' ' < "$lock.holder" 2>/dev/null)" >&2 + echo "agmsg: that process answered as alive, so this was contention." >&2 + else + echo "agmsg: the lock records no holder, so nothing here could ask whether it is held." >&2 + echo "agmsg: a lock with no holder record is not broken automatically — see #865." >&2 + fi # The reason travels with the timeout too. If the wait was hopeless for # a cause this function did not anticipate, the errno is the only thing # that will say so. @@ -161,8 +250,15 @@ agmsg_lock_acquire() { } > "$lock.holder" 2>/dev/null || true AGMSG_HELD_LOCKS="${AGMSG_HELD_LOCKS:+$AGMSG_HELD_LOCKS }$lock" - # Idempotent: re-arming the same handlers each acquire is harmless. They release - # every held lock, so a crash with one or two locks held leaves no stale lock. + # Idempotent: re-arming the same handlers each acquire is harmless. + # + # WHAT THEY COVER, AND WHAT THEY DO NOT. These release every lock this process + # holds, so an ordinary exit or a Ctrl-C leaves nothing behind. `SIGKILL`, an + # OOM kill and the machine going down run no trap at all, and the lock stays. + # This sentence used to end at "a crash leaves no stale lock", with no + # qualifier, so the next reader believed crashes were covered — and the crash + # that is not covered is the one #865 was reported from. What covers it is the + # staleness check in the loop above, not this. # EXIT releases only. INT/TERM release AND exit, so a signal arriving between # commands in a critical section can't release the lock and then let the script # continue into an unprotected config move/write (matters for 2-lock diff --git a/tests/test_registry_lock.bats b/tests/test_registry_lock.bats index 6ee6651e0..483e00f39 100644 --- a/tests/test_registry_lock.bats +++ b/tests/test_registry_lock.bats @@ -339,3 +339,132 @@ acquire() { # runs the acquire in its own shell, with a short spin budget run diff "$BATS_TEST_TMPDIR/holder.before" "$TEAM_DIR/.config.lock.holder" [ "$status" -eq 0 ] } + +# --- a holder that is gone, and one that is not (#865) ---------------------- +# +# The lock was permanent because acquire never asked whether the recorded holder +# was still running. Both halves are here, and the second is the one that has to +# hold: breaking a lock somebody IS using is worse than the leak this fixes. + +# A process that is alive for as long as the test needs it, and whose pid is +# real. `sleep` rather than a made-up number: a pid that happens to be free +# would make the "live" case pass for the wrong reason. +start_live_holder() { + sleep 30 & + LIVE_PID=$! +} + +# A pid that is genuinely not running: spawn, wait for it, then reuse the number. +# `wait` is what makes this a measurement rather than a guess — the process has +# been reaped before its number is written into the holder. +dead_pid() { + local p + sleep 0 & + p=$! + wait "$p" 2>/dev/null || true + printf '%s' "$p" +} + +write_holder() { # write_holder + printf 'token %s\npid %s\ncommand %s\nhost %s\n' \ + "test.token.$1" "$1" "join.sh" "testhost" > "$TEAM_DIR/.config.lock.holder" +} + +@test "lock: a lock whose holder is gone is broken, and the acquire succeeds (#865)" { + mkdir "$TEAM_DIR/.config.lock" + local gone; gone="$(dead_pid)" + # CONTROL FIRST: the pid really is not running. Without this the case passes + # whenever the number happens to be free, which is not what it claims. + run env PID="$gone" LOCKLIB="$LOCKLIB" bash -c '. "$LOCKLIB"; _agmsg_pid_alive_local "$PID"' + [ "$status" -ne 0 ] + + write_holder "$gone" + acquire + [ "$status" -eq 0 ] + [[ "$output" == *"broke a registry lock"* ]] + [[ "$output" == *"holder is gone"* ]] + # HELD, not merely removed — and asserted INSIDE the acquiring shell, because + # its EXIT trap releases the lock the moment it returns. Checking afterwards + # would find no directory and prove nothing either way. + mkdir -p "$TEAM_DIR/.config.lock" + write_holder "$gone" + run env AGMSG_LOCK_TRIES=5 LOCKLIB="$LOCKLIB" TEAM_DIR="$TEAM_DIR" bash -c ' + . "$LOCKLIB" + agmsg_lock_acquire "$TEAM_DIR" 2>/dev/null + [ -d "$TEAM_DIR/.config.lock" ] && echo HELD + sed -n "s/^pid //p" "$TEAM_DIR/.config.lock.holder" | head -1 + ' + [ "$status" -eq 0 ] + [[ "$output" == *"HELD"* ]] + # The record is the new holder's, not the dead one's. + [[ "$output" != *"$gone"* ]] +} + +@test "lock: a lock whose holder is ALIVE is never broken (#865)" { + mkdir "$TEAM_DIR/.config.lock" + start_live_holder + # CONTROL: the holder is alive AT THE MOMENT the acquire runs, asserted here + # rather than assumed from having spawned it — a `sleep` that failed to start + # would make this case pass by breaking a lock, which is the outcome it + # exists to forbid. + run env PID="$LIVE_PID" LOCKLIB="$LOCKLIB" bash -c '. "$LOCKLIB"; _agmsg_pid_alive_local "$PID"' + [ "$status" -eq 0 ] + + write_holder "$LIVE_PID" + acquire + [ "$status" -ne 0 ] + [[ "$output" == *"timed out acquiring registry lock"* ]] + [[ "$output" != *"broke a registry lock"* ]] + # And the holder record is untouched — the lock is still theirs. + run grep -c "^pid $LIVE_PID\$" "$TEAM_DIR/.config.lock.holder" + [ "$output" = "1" ] + kill "$LIVE_PID" 2>/dev/null || true +} + +@test "lock: a lock with NO holder record is left alone, and the timeout says so (#865)" { + # The half this change does not take on: nothing here can tell a lock written + # by an older version from one created microseconds ago whose owner has not + # recorded itself yet, so it does not guess. Pinned so that changing it has to + # be a decision. + mkdir "$TEAM_DIR/.config.lock" + acquire + [ "$status" -ne 0 ] + [[ "$output" != *"broke a registry lock"* ]] + [[ "$output" == *"records no holder"* ]] + [ -d "$TEAM_DIR/.config.lock" ] +} + +@test "lock: the timeout on a live holder prints what the lock records (#865)" { + mkdir "$TEAM_DIR/.config.lock" + start_live_holder + write_holder "$LIVE_PID" + acquire + [ "$status" -ne 0 ] + # The pid is the part an operator acts on; asserting the whole line would + # pin the format instead. + [[ "$output" == *"the lock records:"* ]] + [[ "$output" == *"pid $LIVE_PID"* ]] + kill "$LIVE_PID" 2>/dev/null || true +} + +@test "lock: two racing breakers, and only one of them removes the lock (#865)" { + # The claim-then-remove is what makes this safe: `mv` of one holder file + # succeeds for exactly one caller. Without it both would rmdir, and the second + # could take the directory away from a lock the first had already re-taken. + mkdir "$TEAM_DIR/.config.lock" + local gone; gone="$(dead_pid)" + write_holder "$gone" + run env LOCKLIB="$LOCKLIB" TEAM_DIR="$TEAM_DIR" bash -c ' + . "$LOCKLIB" + a=1; b=1 + _agmsg_lock_break_dead "$TEAM_DIR/.config.lock" && a=0 + mkdir -p "$TEAM_DIR/.config.lock" + _agmsg_lock_break_dead "$TEAM_DIR/.config.lock" && b=0 + echo "first=$a second=$b" + ' + [ "$status" -eq 0 ] + # The second call faces a lock whose holder file it already consumed: it must + # refuse, because it cannot show that the directory now there is the one it + # judged. + [[ "$output" == *"first=0 second=1"* ]] +} From 15d2e1bc4d6be98a9eb1cc05fd5a13ddfbe081b2 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 22:20:59 -0700 Subject: [PATCH 2/2] fix(lock): claim the holder before judging it, not after (#865 review) The first version read the holder, decided it was dead, and then renamed whatever was at that path. Between those two steps a different breaker can claim the old holder and remove the directory, a new owner can take the same path and write a live holder, and the rename then succeeds against the new file - the path is the same. The rmdir after it removes a lock somebody is using. The claim bound nothing. Renaming first makes the claim the thing that is judged, and once it is claimed no other breaker can act (the file is gone) and no new owner can appear (the directory is still there). A claim that turns out to be alive is put back. A case drives the handoff: break, a new owner takes the freed path with a live holder, then the late breaker runs with its old verdict. Also: the timeout said a holder answered as alive whenever a holder file existed. A record with no pid line, or an unusable one, is 'could not be asked' - which is what sent the last diagnoses after processes that were not there. --- scripts/lib/registry-lock.sh | 55 ++++++++++++++++++++++++++--------- tests/test_registry_lock.bats | 34 ++++++++++++++++++++++ 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/scripts/lib/registry-lock.sh b/scripts/lib/registry-lock.sh index 235574076..4a870f573 100644 --- a/scripts/lib/registry-lock.sh +++ b/scripts/lib/registry-lock.sh @@ -90,22 +90,38 @@ _agmsg_lock_holder_gone() { # Break a lock whose recorded holder is gone. # -# CLAIMED BEFORE IT IS BROKEN, and the claim is the holder file itself. Two -# processes can reach the same verdict in the same instant; `mv` of a given file -# succeeds for exactly one of them, so exactly one goes on to remove the -# directory. The loser's `mv` fails and it re-evaluates — by which time the -# winner has either taken the lock or left the path free. +# CLAIM FIRST, JUDGE SECOND, and the order is the whole correctness argument. # -# The claim also binds the removal to the holder that was JUDGED. If the lock -# changed hands between the verdict and here, the file this renames is not the -# file that was read, the rename fails, and no `rmdir` lands on a lock somebody -# else has since taken. +# The first version read the holder, decided it was dead, and then renamed +# whatever was at that path. Review took it apart: between the two, a different +# breaker can claim the old holder and remove the directory, a new owner can +# take the same path and write a LIVE holder — and the rename then succeeds +# against the new file, because the path is the same. The `rmdir` after it +# removes the new owner's lock. "If the lock changed hands the rename fails" was +# not true of a path. +# +# Renaming first makes the claim the thing that is judged. Two processes cannot +# both claim: `mv` of a given file succeeds for exactly one of them. And once +# the holder is claimed, nobody else can legitimately break this lock (the file +# they would have to claim is gone) and no new owner can appear (the directory +# is still there, so `mkdir` still fails) — so the directory removed below is +# necessarily the one the claimed holder belonged to. +# +# A claim that turns out to be alive is put back. That costs a rename on a live +# lock, which is why the caller checks `_agmsg_lock_holder_gone` first: the +# cheap read filters the common case, and this is the judgement that counts. _agmsg_lock_break_dead() { - local lock="$1" claimed="$lock.dead.$$.${RANDOM:-0}" + local lock="$1" claimed="$lock.dead.$$.${RANDOM:-0}" pid mv "$lock.holder" "$claimed" 2>/dev/null || return 1 + pid="$(sed -n 's/^pid //p' "$claimed" 2>/dev/null | head -1)" + if [ -z "$pid" ] || _agmsg_pid_alive_local "$pid"; then + # Alive, or nothing here can say otherwise. Put it back — the next reader + # must still find out who the lock says is holding it. + mv "$claimed" "$lock.holder" 2>/dev/null || true + return 1 + fi if ! rmdir "$lock" 2>/dev/null; then - # Not taking it after all, so the next reader must still find out who the - # lock says is holding it. + # Not taking it after all, so the record goes back where it was. mv "$claimed" "$lock.holder" 2>/dev/null || true return 1 fi @@ -184,7 +200,20 @@ agmsg_lock_acquire() { # that sent the last three diagnoses after processes that were not there. if [ -f "$lock.holder" ]; then echo "agmsg: the lock records: $(tr '\n' ' ' < "$lock.holder" 2>/dev/null)" >&2 - echo "agmsg: that process answered as alive, so this was contention." >&2 + # ALIVE AND UNCHECKABLE ARE NOT THE SAME ANSWER. A holder file with no + # `pid` line, an empty one, or a value `_agmsg_pid_valid` rejects all + # leave `_agmsg_lock_holder_gone` false — which is "this could not be + # asked", not "it answered yes". Saying the second puts the operator + # back where the old message put them: hunting a process on the strength + # of a sentence that never checked (raised in review). + _agmsg_timeout_pid="$(sed -n 's/^pid //p' "$lock.holder" 2>/dev/null | head -1)" + if [ -z "$_agmsg_timeout_pid" ]; then + echo "agmsg: that record names no pid, so nothing here could ask whether it is held." >&2 + elif _agmsg_pid_alive_local "$_agmsg_timeout_pid"; then + echo "agmsg: that process answered as alive, so this was contention." >&2 + else + echo "agmsg: that process is not running — the lock was being broken as this wait ended." >&2 + fi else echo "agmsg: the lock records no holder, so nothing here could ask whether it is held." >&2 echo "agmsg: a lock with no holder record is not broken automatically — see #865." >&2 diff --git a/tests/test_registry_lock.bats b/tests/test_registry_lock.bats index 483e00f39..8d5c6b8e6 100644 --- a/tests/test_registry_lock.bats +++ b/tests/test_registry_lock.bats @@ -468,3 +468,37 @@ write_holder() { # write_holder # judged. [[ "$output" == *"first=0 second=1"* ]] } + +@test "lock: a breaker arriving after the lock changed hands leaves the new owner alone (#865)" { + # THE INTERLEAVING THE FIRST VERSION GOT WRONG, raised in review. Judging the + # holder and then renaming "whatever is at that path" are not the same thing: + # between them the lock can be broken by somebody else and re-taken by a new + # owner, and the rename then succeeds against the NEW holder — after which the + # rmdir removes a lock that is being used. + # + # Driven as the handoff, not as a repeat: the second breaker faces a live + # holder at the same path, which is precisely the state that used to be + # indistinguishable. + mkdir "$TEAM_DIR/.config.lock" + local gone; gone="$(dead_pid)" + write_holder "$gone" + start_live_holder + run env LOCKLIB="$LOCKLIB" TEAM_DIR="$TEAM_DIR" LIVE="$LIVE_PID" bash -c ' + . "$LOCKLIB" + lock="$TEAM_DIR/.config.lock" + _agmsg_lock_break_dead "$lock" || { echo "first-refused"; exit 1; } + # A new owner takes the freed path and records itself, alive. + mkdir "$lock" + printf "token t\npid %s\ncommand join.sh\nhost h\n" "$LIVE" > "$lock.holder" + # The late breaker, still carrying the verdict it formed on the OLD holder. + if _agmsg_lock_break_dead "$lock"; then echo "late-broke-it"; else echo "late-refused"; fi + [ -d "$lock" ] && echo "lock-survived" + sed -n "s/^pid //p" "$lock.holder" | head -1 + ' + [ "$status" -eq 0 ] + [[ "$output" == *"late-refused"* ]] + [[ "$output" == *"lock-survived"* ]] + # The new owner's record is intact — put back byte for byte, not consumed. + [[ "$output" == *"$LIVE_PID"* ]] + kill "$LIVE_PID" 2>/dev/null || true +}