diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index 6eaee956..9e25c06d 100644 --- a/scripts/lib/instance-id.sh +++ b/scripts/lib/instance-id.sh @@ -110,7 +110,7 @@ _agmsg_pid_valid() { # The EPERM reading and the ps cross-check are the same as _agmsg_pid_alive's -- # a pid we minted is still a pid a sandbox may refuse to let us signal (#505). _agmsg_pid_alive_local() { - local pid="$1" err stat + local pid="$1" err stat probe rc canary tstat _p _s _rest # The POSIX ceiling, explicitly, whatever the host. _agmsg_pid_valid widens to # the DWORD range when MSYSTEM is set, which is right for a number tasklist # will be asked about and wrong for one kill(1) will parse: past INT32_MAX kill @@ -131,10 +131,47 @@ _agmsg_pid_alive_local() { esac # kill(2) says gone. ps does not depend on signalling permission at all, so # requiring it to agree is what keeps a sandbox from turning "cannot signal" - # into "not running". - stat="$(ps -o stat= -p "$pid" 2>/dev/null | tr -d ' ')" - [ -n "$stat" ] || return 1 - case "$stat" in Z*) return 1 ;; esac # exited, just not reaped yet + # into "not running" (#505). But an EMPTY ps result is NOT proof of death: a + # transient ps failure and a truly-absent pid both produce nothing, and reading + # that as "gone" is #954 -- callers delete files, release locks, and respawn on + # it. Distinguish "proof of absence" from "absence of proof" by co-observing a + # known-live pid -- our own $$ -- in the SAME observation. Take a FULL snapshot + # (no -p filter, so the target pid is never handed to ps and cannot poison the + # query, e.g. macOS "process id too large"), parsed with builtins so only ps is + # external and a stripped PATH cannot itself become the failed observation: + # - $$ absent from the snapshot => ps produced nothing usable => UNKNOWN => + # assume alive, exactly as the EPERM branch above. A failed observation is + # not proof of absence. + # - $$ present, target absent => ps listed us and did not list the target => + # positive proof the target is gone => dead. + # - target present, zombie => gone too. + # `|| rc=$?` keeps the assignment out of set -e's reach: a command-substitution + # assignment returns the substituted command's exit status as its OWN, so under + # errexit in a caller that did NOT invoke us as a condition, a non-zero ps would + # terminate the shell right here -- leaking a failed observation to caller death + # instead of the UNKNOWN => alive verdict below. The leaf helper's contract must + # not depend on how the caller spelled the call. + rc=0 + probe="$(ps -Ao pid=,stat= 2>/dev/null)" || rc=$? + canary=0; tstat="" + while read -r _p _s _rest; do + if [ "$_p" = "$$" ]; then canary=1; fi + if [ "$_p" = "$pid" ]; then tstat="${_s:-?}"; fi + done < alive (seeing it is proof enough) + fi + # Target not listed. Trust "absent" ONLY when ps COMPLETED the snapshot (exit 0) + # AND that snapshot included our own $$. A non-zero exit means the listing was + # truncated -- ps can print part of it (even our own line) and then fail -- and a + # pid that would have come later proves nothing; canary presence shows only that + # WE were listed, never that the listing FINISHED. Anything short of a complete, + # self-including snapshot is UNKNOWN -> assume alive (#954), which also fails safe + # where "ps -Ao" is unsupported (it exits non-zero rather than lying "gone"). + if [ "$rc" -eq 0 ] && [ "$canary" = 1 ]; then return 1; fi return 0 } diff --git a/tests/test_instance_id.bats b/tests/test_instance_id.bats index 0d6857a1..334db056 100644 --- a/tests/test_instance_id.bats +++ b/tests/test_instance_id.bats @@ -199,6 +199,100 @@ gone_pid() { _agmsg_pid_alive 42 } +# --- #954: the ps cross-check must tell "proof of absence" from "absence of +# proof". Both halves are asserted with the SAME genuinely-dead pid, so a result +# of "alive" can ONLY be the canary suppressing the death verdict, never the pid +# being live -- exactly the distinction the bug erased. --- + +@test "pid_alive: a truly-gone pid is PROVEN dead when ps answers, and cleanup fires (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + # A pid this shell minted and reaped: kill(2) returns a real ESRCH, real ps + # lists our own $$ but not the gone pid -> positive proof of absence. + sh -c 'exit 0' & local gone=$!; wait "$gone" 2>/dev/null + run _agmsg_pid_alive_local "$gone" + [ "$status" -ne 0 ] || { echo "a gone pid with a working ps read alive"; false; } + # The cleanup-side contract: `... || rm -f` DOES delete for a proven-gone pid. + local marker="$RUN_DIR/marker.$gone"; : > "$marker" + _agmsg_pid_alive_local "$gone" || rm -f "$marker" + [ ! -e "$marker" ] || { echo "cleanup did not fire on a proven-dead pid"; false; } +} + +@test "pid_alive: a truly-gone pid reads ALIVE when ps cannot answer -- a failed observation is not proof, and cleanup is suppressed (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + sh -c 'exit 0' & local gone=$!; wait "$gone" 2>/dev/null + # ps cannot answer: it emits nothing and fails. The canary ($$) is absent from + # the output, so the helper cannot see even itself -> observation failed. The + # pid is genuinely gone, so "alive" here is UNAMBIGUOUSLY the canary firing. + ps() { return 1; } + run _agmsg_pid_alive_local "$gone" + [ "$status" -eq 0 ] || { echo "a failed ps was read as proof of death (the #954 bug)"; false; } + # The cleanup-side contract: `... || rm -f` does NOT delete when we could not + # observe. The file a live process might still own is left intact. + local marker="$RUN_DIR/marker.$gone"; : > "$marker" + _agmsg_pid_alive_local "$gone" || rm -f "$marker" + [ -e "$marker" ] || { echo "cleanup fired on an UNKNOWN observation (UNKNOWN leaked to the cleanup side)"; false; } +} + +@test "pid_alive: ps that omits the target but still lists the canary is proof of death (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + # ps answers (lists $$, proving it ran) but does not list the target -> the + # target is provably gone even though ps was reachable. + kill() { echo "bash: kill: - No such process" >&2; return 1; } + ps() { printf '%s S\n' "$$"; } # only the canary, never the queried target + run _agmsg_pid_alive_local 424242 + [ "$status" -ne 0 ] || { echo "an answered ps that omits the target did not read dead"; false; } +} + +@test "pid_alive: a snapshot with output but WITHOUT the canary is UNKNOWN, not death (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + # The subtle leak: ps returns SOME lines but not our own $$ (a partial or garbage + # snapshot, or a failure that still printed something). Without the canary the + # observation is untrusted, so a genuinely-gone pid must STILL read alive -- a + # non-empty result is not itself proof the snapshot was complete. No retry count + # or partial output may turn this into a death verdict, and cleanup stays put. + sh -c 'exit 0' & local gone=$!; wait "$gone" 2>/dev/null + ps() { printf '999999 R\n'; } # a line, but never $$ and never the target + run _agmsg_pid_alive_local "$gone" + [ "$status" -eq 0 ] || { echo "a canary-less snapshot was read as proof of death"; false; } + local marker="$RUN_DIR/marker.$gone"; : > "$marker" + _agmsg_pid_alive_local "$gone" || rm -f "$marker" + [ -e "$marker" ] || { echo "cleanup fired on a canary-less snapshot (UNKNOWN leaked to cleanup)"; false; } +} + +@test "pid_alive: a ps that lists the canary but EXITS NON-ZERO is a truncated snapshot -> UNKNOWN, not death (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + # The last leak: ps prints part of the snapshot -- even our own $$ -- and THEN + # fails. The target's line may simply never have been reached, so its absence + # from a truncated listing is not proof. A non-zero exit must read as UNKNOWN + # regardless of what partial output was captured. (This is also why an "ps -Ao" + # a platform does not support fails safe rather than lying "gone".) + sh -c 'exit 0' & local gone=$!; wait "$gone" 2>/dev/null + ps() { printf '%s S\n' "$$"; return 1; } # canary printed, then ps fails + run _agmsg_pid_alive_local "$gone" + [ "$status" -eq 0 ] || { echo "a non-zero ps exit was read as proof of death despite partial output"; false; } + local marker="$RUN_DIR/marker.$gone"; : > "$marker" + _agmsg_pid_alive_local "$gone" || rm -f "$marker" + [ -e "$marker" ] || { echo "cleanup fired on a failed (truncated) ps snapshot"; false; } +} + +@test "pid_alive: a failing ps under set -e does not terminate a non-conditional caller (#954)" { + skip_on_windows "POSIX kill path; Windows uses tasklist (#134)" + # The leaf helper's contract must not depend on caller syntax. Called as a bare + # statement under errexit, a ps that fails must not kill the shell before the + # UNKNOWN -> alive verdict: the observation failure has to surface as "alive", + # never as caller termination. + run bash -c ' + set -e + source "'"$SKILL_DIR"'/scripts/lib/instance-id.sh" + ps() { return 1; } + kill() { echo "bash: kill: - No such process" >&2; return 1; } + _agmsg_pid_alive_local 99999999 + echo REACHED-alive + ' + [ "$status" -eq 0 ] || { echo "the caller shell died on a failing ps under set -e"; false; } + printf '%s\n' "$output" | grep -q REACHED-alive || { echo "did not continue past the helper call"; false; } +} + # --- agmsg_normalize_instance_id --- @test "normalize: a composite token passes through unchanged (idempotent)" {