diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de5846..ceafe1ad2 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -221,6 +221,113 @@ _remote_prompt_read() { # token, no state change, safe whether or not the team is already connected. # Currently just the age-binary-presence check (§8) — the natural home for # any future preflight check added later. +# WHICH DIRECTORY TO REMOVE — the thing that was missing (#865). +# +# A registry lock left behind by a killed process is never broken by anything: +# nothing expires, nothing sweeps, and acquire waits out its budget and fails +# with `timed out acquiring registry lock`, which describes contention. The +# operator's next move is to look for the process holding it; there is no +# process, and no message anywhere names the directory to remove. A machine in +# that state cannot get itself out. +# +# This REPORTS and removes nothing. The report alone ends "cannot recover": +# it says which team, what the lock records, whether that process is running, +# and prints the removal as a line to paste. Sweeping automatically is a +# separate decision with a worse failure mode — a wrong verdict takes a lock +# away from a process that is using it — and it is not made here. +# +# Not a check, so no `[x]`/`[ ]` and no effect on the exit code: a lock that +# exists is not a failed prerequisite, and a doctor that exits non-zero because +# a team is busy would be wrong every time somebody is joining. +# One lock, reported. Split out so the two ways of finding them — a named team, +# or a sweep — share this body and neither has to feed a loop from a string. +# Joining the paths into one variable and reading it back would mean either an +# unquoted heredoc, which runs command substitution on a team name, or unquoted +# word splitting, which globs one. +_remote_doctor_one_lock() { + local lock="$1" team holder pid state q + [ -d "$lock" ] || return 0 + team="${lock%/.config.lock}" + team="${team##*/}" + if [ "$shown" -eq 0 ]; then + echo "Registry locks:" + shown=1 + fi + holder="$lock.holder" + pid="" + [ -f "$holder" ] && pid="$(sed -n 's/^pid //p' "$holder" 2>/dev/null | head -1)" + # THREE ANSWERS, NOT TWO. "held" and "the holder is gone" are what the + # operator acts on; "cannot tell" is neither — a lock this could not ask + # about, and saying it is gone would be a guess. The guess that costs is the + # one that calls a live lock dead and then prints the command to remove it. + # + # WHICH IS WHY THE NUMBER IS VALIDATED BEFORE IT IS ASKED ABOUT. + # `_agmsg_pid_alive_local` returns false for a value it never put to the + # process table at all — non-numeric, leading zero, past the POSIX ceiling — + # and folding that into "not running" turned `pid not-a-pid` into a stale + # verdict with a removal beside it (raised in review). The same ceiling the + # helper uses, for the same reason it uses it. + if [ -z "$pid" ]; then + state="cannot tell — no holder recorded" + elif ! _agmsg_pid_valid "$pid" 2147483647; then + state="cannot tell — the holder record's pid is not a usable number" + elif _agmsg_pid_alive_local "$pid"; then + state="held — pid $pid is running" + else + state="stale — pid $pid is not running" + fi + echo " $team: $state" + if [ -f "$holder" ]; then + echo " records: $(tr '\n' ' ' < "$holder" 2>/dev/null)" + fi + echo " created: $(ls -ld "$lock" 2>/dev/null || printf '%s (cannot stat)' "$lock")" + # QUOTED, because this is meant to be pasted. The store root and the team + # name can both contain a space, and an unquoted path becomes several + # arguments to `rm -r`. Same scheme as lib/shquote.sh. + q="$(printf "'%s'" "$(printf '%s' "$lock" | sed "s/'/'\\\\''/g")")" + if [ "$state" = "held — pid $pid is running" ]; then + echo " a command is using this team. Nothing to do." + else + echo " if no agmsg command is running for this team, remove it:" + echo " rm -r $q" + [ -f "$holder" ] && echo " rm -f $q.holder" + echo " nothing but the lock lives in there — it holds no team data." + fi +} + +_remote_doctor_locks() { + local only_team="${1:-}" lock shown=0 + # A NAMED TEAM IS LOOKED AT DIRECTLY, and the sweep does not stop at `*`. + # + # Team names may begin with a dot — the validator rejects empty, `.`, `..`, a + # leading `-`, `/`, `\` and control characters, and nothing else — and `*` + # does not match a leading dot, so `.foo` was invisible to the sweep (raised + # in review). The two extra patterns cover `.foo` and `..foo`; `.` and `..` + # themselves cannot be team names. + # + # Globs rather than `find`, which is not on every PATH this tree is required + # to run under. Quoted, so a team name containing a space stays one path. + if [ -n "$only_team" ]; then + # VALIDATED BEFORE IT BECOMES A PATH. `cmd_doctor` takes its argument and, + # until this line, only ever put it in a header sentence — so nothing had + # ever checked it. Building `$TEAMS_DIR/$only_team/.config.lock` from it + # turned `doctor ../outside` into a read of a directory outside the store, + # reported with its records and a `rm -r` to paste (raised in review). The + # sweep it replaced never reached one only because no team was named that. + # + # The same validator every other team-taking path uses: it rejects empty, + # `.`, `..`, `/`, `\`, a leading `-` and control characters, and allows a + # leading dot and a space, which this reports on and must keep. + agmsg_validate_team_name "$only_team" || return 1 + _remote_doctor_one_lock "$TEAMS_DIR/$only_team/.config.lock" + else + for lock in "$TEAMS_DIR"/*/.config.lock "$TEAMS_DIR"/.[!.]*/.config.lock "$TEAMS_DIR"/..?*/.config.lock; do + _remote_doctor_one_lock "$lock" + done + fi + [ "$shown" -eq 0 ] || echo +} + cmd_doctor() { local team="${1:-}" echo "Checking prerequisites${team:+ for team '$team'}..." @@ -275,8 +382,16 @@ cmd_doctor() { failed=1 fi echo + # A REJECTED TEAM NAME ENDS THE COMMAND. The validator prints why; carrying on + # to "All prerequisite checks passed." after refusing to look at what was + # asked about would report success for a question nobody answered. + _remote_doctor_locks "$team" || exit 1 if [ "$failed" -eq 0 ]; then - echo "All checks passed." + # NARROWED, because a lock report can sit above this line. "All checks + # passed." after "stale — pid 4711 is not running" and a `rm -r` reads as + # cancelling it, and the exit code deliberately does not move: a locked team + # is not a failed prerequisite (raised in review). + echo "All prerequisite checks passed." else echo "Some checks failed. See above." exit 1 diff --git a/tests/test_remote.bats b/tests/test_remote.bats index 6b0665bc0..d544c3751 100644 --- a/tests/test_remote.bats +++ b/tests/test_remote.bats @@ -115,7 +115,7 @@ skip_if_no_age() { run bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] [[ "$output" == *"age / age-keygen on PATH"* ]] - [[ "$output" == *"All checks passed."* ]] + [[ "$output" == *"All prerequisite checks passed."* ]] } @test "remote doctor: is read-only (no token required, no state touched)" { @@ -1550,7 +1550,7 @@ VALUES ('remote-pending.$key', $owner_pid, strftime('%Y-%m-%dT%H:%M:%SZ','now')) || skip "all doctor prerequisites are not installed" run bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] - [[ "$output" == *"All checks passed."* ]] + [[ "$output" == *"All prerequisite checks passed."* ]] } PULL_TEAM_ID=018f3f7e-2222-7000-8000-000000000002 @@ -2582,7 +2582,7 @@ PY local no_age; no_age="$(path_without_age)" run env PATH="$no_age" bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] - [[ "$output" == *"All checks passed"* ]] + [[ "$output" == *"All prerequisite checks passed"* ]] [[ "$output" == *"optional"* ]] [[ "$output" != *"is required for end-to-end encryption"* ]] } @@ -2714,3 +2714,196 @@ assert_lookup_rejected() { [[ "$output" != *"No such file or directory"*"remote.sh"* ]] [[ "$output" != *"Usage: remote.sh unlock"* ]] } + +# --- doctor names the wedged lock, and removes nothing (#865) --------------- +# +# A registry lock left by a killed process is never broken: acquire waits out +# its budget and says "timed out acquiring registry lock", which describes +# contention, and no message anywhere names the directory to remove. `doctor` is +# where that becomes findable. + +doctor_lock_dead_pid() { # a pid that is genuinely not running + local p + sleep 0 & + p=$! + wait "$p" 2>/dev/null || true + printf '%s' "$p" +} + +make_lock() { # make_lock [pid] + mkdir -p "$TEST_SKILL_DIR/teams/$1" + mkdir -p "$TEST_SKILL_DIR/teams/$1/.config.lock" + if [ -n "${2:-}" ]; then + printf 'token t\npid %s\ncommand join.sh\nhost h\n' "$2" \ + > "$TEST_SKILL_DIR/teams/$1/.config.lock.holder" + fi +} + +@test "remote doctor: a lock whose holder is gone is named, with the way out (#865)" { + local gone; gone="$(doctor_lock_dead_pid)" + # CONTROL: the pid really is not running, asserted before it is written into + # the holder — a number that merely happened to be free would make this pass + # for a reason that has nothing to do with the report. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local $gone" + [ "$status" -ne 0 ] + + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "wedged: stale — pid $gone is not running" <<<"$output" + grep -qF -- "rm -r " <<<"$output" + # REPORTS ONLY. The whole point of doing this before an automatic sweep is + # that a wrong verdict must not cost anything. + [ -d "$TEST_SKILL_DIR/teams/wedged/.config.lock" ] +} + +@test "remote doctor: a lock whose holder is alive is not called stale (#865)" { + sleep 30 & + local live=$! + # CONTROL: alive at the moment doctor runs, not merely spawned. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local $live" + [ "$status" -eq 0 ] + + make_lock busy "$live" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "busy: held — pid $live is running" <<<"$output" + # And it must NOT offer to remove a lock somebody is using. + refute grep -qF -- "if no agmsg command is running for this team" <<<"$output" + kill "$live" 2>/dev/null || true +} + +@test "remote doctor: a lock with no holder record says it cannot tell (#865)" { + # Neither "held" nor "gone": written by a version that recorded nothing, or by + # a process killed between creating the lock and writing its record. Guessing + # here is what would cost a live lock. + make_lock unknown + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "unknown: cannot tell — no holder recorded" <<<"$output" + grep -qF -- "rm -r " <<<"$output" +} + +@test "remote doctor: says nothing about locks when there are none (#865)" { + # The negative control for the three above: the section is absent on a healthy + # install, so "Registry locks:" appearing at all is a finding. + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + refute grep -qF -- "Registry locks:" <<<"$output" +} + +@test "remote doctor: a lock does not fail the run (#865)" { + # A team being locked is not a failed prerequisite. If it set the exit code, + # doctor would fail every time somebody is joining. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "All prerequisite checks passed." <<<"$output" +} + +@test "remote doctor : reports that team's lock and not another's (#865)" { + local gone; gone="$(doctor_lock_dead_pid)" + make_lock mine "$gone" + make_lock theirs "$gone" + run bash "$SCRIPTS/remote.sh" doctor mine + [ "$status" -eq 0 ] + grep -qF -- "mine: stale" <<<"$output" + refute grep -qF -- "theirs: stale" <<<"$output" +} + +@test "remote doctor: a pid that was never asked about is not called stale (#865)" { + # THE VERDICT AND THE REMOVAL RIDE TOGETHER, so "false" from the liveness + # helper is not enough on its own: it is also false for a value the helper + # refused to put to the process table at all. `pid not-a-pid` and a number + # past the POSIX ceiling were being reported as stale, with `rm -r` beside + # them (raised in review). + make_lock badpid + printf 'token t\npid not-a-pid\ncommand join.sh\nhost h\n' \ + > "$TEST_SKILL_DIR/teams/badpid/.config.lock.holder" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "badpid: cannot tell" <<<"$output" + refute grep -qF -- "badpid: stale" <<<"$output" + + # CONTROL: the helper does answer false for it, so this case is measuring the + # validation and not some other difference. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local not-a-pid" + [ "$status" -ne 0 ] +} + +@test "remote doctor: a pid past the POSIX ceiling is not called stale (#865)" { + make_lock hugepid + printf 'token t\npid 2147483648\ncommand join.sh\nhost h\n' \ + > "$TEST_SKILL_DIR/teams/hugepid/.config.lock.holder" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "hugepid: cannot tell" <<<"$output" + refute grep -qF -- "hugepid: stale" <<<"$output" +} + +@test "remote doctor: a team whose name begins with a dot is swept too (#865)" { + # `*` does not match a leading dot, and the team validator allows one — so the + # sweep walked past `.hidden` entirely (raised in review). The validator + # rejects empty, `.`, `..`, a leading `-`, `/`, `\` and control characters, + # and nothing else. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock .hidden "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- ".hidden: stale" <<<"$output" +} + +@test "remote doctor: the summary does not cancel a lock finding (#865)" { + # "All checks passed." above a stale lock and a removal command reads as + # withdrawing them. The exit code deliberately stays 0 — a locked team is not + # a failed prerequisite — so the wording is what has to carry the distinction. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "All prerequisite checks passed." <<<"$output" + refute grep -qE '^All checks passed\.$' <<<"$output" +} + +@test "remote doctor : a traversal argument is refused and reads nothing (#865)" { + # The named-team lookup builds a path from the argument, and until this was + # added nothing had ever validated it — `cmd_doctor` only ever put the value + # in a header sentence. A sentinel outside the store proves the refusal is + # about reach and not about the string. + local outside="$BATS_TEST_TMPDIR/outside" + mkdir -p "$outside/.config.lock" + printf 'token t\npid 1\ncommand join.sh\nhost h\n' > "$outside/.config.lock.holder" + # CONTROL: the sentinel is real and would be reported if it were reached — + # the same shape, under a team name, IS reported (the case above). + [ -d "$outside/.config.lock" ] + + run bash "$SCRIPTS/remote.sh" doctor "../../$(basename "$BATS_TEST_TMPDIR")/outside" + [ "$status" -ne 0 ] + refute grep -qF -- "Registry locks:" <<<"$output" + refute grep -qF -- "rm -r " <<<"$output" + grep -qF -- "invalid team name" <<<"$output" + # And it must not claim the prerequisites verdict for a question it refused. + refute grep -qF -- "All prerequisite checks passed." <<<"$output" +} + +@test "remote doctor : a slash in the name is refused (#865)" { + run bash "$SCRIPTS/remote.sh" doctor "a/b" + [ "$status" -ne 0 ] + grep -qF -- "invalid team name" <<<"$output" +} + +@test "remote doctor : an ordinary and a dot-leading name still resolve (#865)" { + # The validator allows both, and the direct lookup has to keep working for + # them — a refusal that took the legitimate names with it would be the + # cheapest way to pass the two cases above. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock plain "$gone" + make_lock .dotted "$gone" + run bash "$SCRIPTS/remote.sh" doctor plain + [ "$status" -eq 0 ] + grep -qF -- "plain: stale" <<<"$output" + run bash "$SCRIPTS/remote.sh" doctor .dotted + [ "$status" -eq 0 ] + grep -qF -- ".dotted: stale" <<<"$output" +}