diff --git a/scripts/lib/registry-lock.sh b/scripts/lib/registry-lock.sh index 313257333..4a870f573 100644 --- a/scripts/lib/registry-lock.sh +++ b/scripts/lib/registry-lock.sh @@ -53,6 +53,86 @@ _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. +# +# CLAIM FIRST, JUDGE SECOND, and the order is the whole correctness argument. +# +# 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}" 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 record goes back where it was. + 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 +165,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 +192,32 @@ 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 + # 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 + 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 +279,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..8d5c6b8e6 100644 --- a/tests/test_registry_lock.bats +++ b/tests/test_registry_lock.bats @@ -339,3 +339,166 @@ 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"* ]] +} + +@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 +}