From 3e15c7f3c1701ff8954eb8c13d59e6200a706635 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 22 Aug 2026 12:10:11 -0700 Subject: [PATCH 1/4] fix(instance-id): a failed ps observation is not proof of death -- distinguish it with a $$ canary (#954) _agmsg_pid_alive_local's ps cross-check (reached after kill(2) returns ESRCH) read an EMPTY ps result as "gone". But a transient ps failure produces the same empty output as a truly-absent pid, so a blip returned "dead" -- and its ~38 callers delete pidfiles/records, release locks, reclaim runtime locks, and respawn on that false (the central sync-engine status oracle, the roster lock release, the launcher reuse-check, session-start's watcher GC, and more; leaf fix, so all are covered at once). This is the same observation-as-state trap fixed across the reap path in #943, now applied to the shared helper itself. Mix a known-live pid into the SAME observation: our own $$, alive by definition. Query the target and $$ together -- - $$ absent from the output => ps could not answer => UNKNOWN => assume alive, exactly as the existing EPERM branch does. A failed observation is not proof. - $$ present, target absent => ps answered and did not list the target => positive proof the target is gone => dead. - target present, zombie => gone too. A pid ps rejects outright ("process id too large") poisons any query it appears in; one retry (a transient failure does not repeat, a poison pid does) plus a $$-only probe tells "ps works, only the target is unacceptable => gone" from "ps cannot observe at all => alive", so a valid-but-unreal pid still reads dead without reintroducing a false-dead for a real one. Parsed with builtins (no awk/grep) so a stripped PATH cannot itself become the failed observation -- only ps is external. Tests assert both halves 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: proven-dead => cleanup fires; ps-cannot-answer => reads alive, cleanup suppressed; ps-answers-but-omits-target => dead. --- scripts/lib/instance-id.sh | 50 ++++++++++++++++++++++++++++++++----- tests/test_instance_id.bats | 44 ++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index 6eaee956d..707a313f2 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 canary tstat _p _s _rest i # 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,11 +131,49 @@ _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 - return 0 + # into "not running" (#505). But an EMPTY ps result is NOT proof of death: a + # transient ps failure produces the same empty output as a truly-absent pid, and + # reading that as "gone" is #954 -- callers delete files, release locks, and + # respawn on this false. Distinguish "proof of absence" from "absence of proof" + # by mixing a known-live pid into the SAME observation: our own $$. Query the + # target AND $$ together; $$ is alive by definition, so: + # - $$ absent from the output => ps did not answer at all (could not exec, + # resource blip, sandbox) => UNKNOWN => assume alive, exactly as the EPERM + # branch above does. A failed observation is not proof of absence. + # - $$ present, target absent => ps answered and did not list the target + # => POSITIVE proof the target is gone => dead. + # - target present, zombie => gone too. + # Parsed with builtins (no awk/grep) so a stripped PATH cannot itself become the + # failed observation -- only ps is external here. + # One retry: a transient ps failure does not repeat, but a target pid ps rejects + # outright (e.g. "process id too large") poisons every query it appears in. + for i in 1 2; do + probe="$(ps -o pid=,stat= -p "$pid,$$" 2>/dev/null)" + canary=0; tstat="" + while read -r _p _s _rest; do + if [ "$_p" = "$$" ]; then canary=1; fi + if [ "$_p" = "$pid" ]; then tstat="${_s:-?}"; fi + done < proven gone + case "$tstat" in Z*) return 1 ;; esac # zombie: exited, not yet reaped + return 0 # target present -> alive + fi + done + # Two queries in a row could not even list our own $$. Either ps cannot run at + # all, or the TARGET pid is out of ps's acceptable range and poisons any query + # it appears in -- and no live process ever has such a pid. Ask for ONLY $$: + probe="$(ps -o pid= -p "$$" 2>/dev/null)" + canary=0 + while read -r _p _rest; do + if [ "$_p" = "$$" ]; then canary=1; fi + done < gone + return 0 # ps cannot observe at all -> assume alive (#954) } # Liveness for a pid that came from OUTSIDE these shells -- reached by walking diff --git a/tests/test_instance_id.bats b/tests/test_instance_id.bats index 0d6857a12..70589f2a1 100644 --- a/tests/test_instance_id.bats +++ b/tests/test_instance_id.bats @@ -199,6 +199,50 @@ 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; } +} + # --- agmsg_normalize_instance_id --- @test "normalize: a composite token passes through unchanged (idempotent)" { From d86937582c43d1f611f047d2c35a05f8a8eb7026 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 22 Aug 2026 12:23:13 -0700 Subject: [PATCH 2/4] fix(instance-id): take the canary from a full ps snapshot, not a filtered query (#954) The previous form queried the target and $$ together (`ps -p "$pid,$$"`) and, when that produced no canary twice, fell back to a $$-only probe and inferred "the target poisoned the query -> dead". That inference is unsound: a successful $$-only probe proves only that ps could list itself on that third call, not that the two earlier combined observations failed because of the target. Two transient failures of the multi-pid query followed by a working $$-only probe -- or any partial output that omits the canary -- produce the same sequence for a genuinely live target, so UNKNOWN could still reach the cleanup side as "dead". Remove the inference entirely by never handing the target to ps. Take one FULL snapshot (`ps -Ao pid=,stat=`) and parse it with builtins for both the target and $$: canary present + target absent is the only path to "dead"; canary absent is always "alive". Because the target pid is not a query argument, an out-of-range value cannot poison the observation (it is simply not in the snapshot -> gone), so the retry and the $$-only fallback are gone with it. Add a test pinning that a snapshot which returns output but omits the canary is still UNKNOWN -> alive and suppresses cleanup, using the same genuinely-dead fixture. --- scripts/lib/instance-id.sh | 63 ++++++++++++++----------------------- tests/test_instance_id.bats | 16 ++++++++++ 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index 707a313f2..df44194c8 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 probe canary tstat _p _s _rest i + local pid="$1" err stat probe 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 @@ -132,48 +132,31 @@ _agmsg_pid_alive_local() { # 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" (#505). But an EMPTY ps result is NOT proof of death: a - # transient ps failure produces the same empty output as a truly-absent pid, and - # reading that as "gone" is #954 -- callers delete files, release locks, and - # respawn on this false. Distinguish "proof of absence" from "absence of proof" - # by mixing a known-live pid into the SAME observation: our own $$. Query the - # target AND $$ together; $$ is alive by definition, so: - # - $$ absent from the output => ps did not answer at all (could not exec, - # resource blip, sandbox) => UNKNOWN => assume alive, exactly as the EPERM - # branch above does. A failed observation is not proof of absence. - # - $$ present, target absent => ps answered and did not list the target - # => POSITIVE proof the target is gone => dead. + # 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. - # Parsed with builtins (no awk/grep) so a stripped PATH cannot itself become the - # failed observation -- only ps is external here. - # One retry: a transient ps failure does not repeat, but a target pid ps rejects - # outright (e.g. "process id too large") poisons every query it appears in. - for i in 1 2; do - probe="$(ps -o pid=,stat= -p "$pid,$$" 2>/dev/null)" - canary=0; tstat="" - while read -r _p _s _rest; do - if [ "$_p" = "$$" ]; then canary=1; fi - if [ "$_p" = "$pid" ]; then tstat="${_s:-?}"; fi - done < proven gone - case "$tstat" in Z*) return 1 ;; esac # zombie: exited, not yet reaped - return 0 # target present -> alive - fi - done - # Two queries in a row could not even list our own $$. Either ps cannot run at - # all, or the TARGET pid is out of ps's acceptable range and poisons any query - # it appears in -- and no live process ever has such a pid. Ask for ONLY $$: - probe="$(ps -o pid= -p "$$" 2>/dev/null)" - canary=0 - while read -r _p _rest; do + probe="$(ps -Ao pid=,stat= 2>/dev/null)" + canary=0; tstat="" + while read -r _p _s _rest; do if [ "$_p" = "$$" ]; then canary=1; fi - done < gone - return 0 # ps cannot observe at all -> assume alive (#954) +PROBE + [ "$canary" = 1 ] || return 0 # no usable snapshot -> assume alive (#954) + [ -n "$tstat" ] || return 1 # snapshot lists us but not the target -> proven gone + case "$tstat" in Z*) return 1 ;; esac # zombie: exited, not yet reaped + return 0 } # Liveness for a pid that came from OUTSIDE these shells -- reached by walking diff --git a/tests/test_instance_id.bats b/tests/test_instance_id.bats index 70589f2a1..e9b1a7d29 100644 --- a/tests/test_instance_id.bats +++ b/tests/test_instance_id.bats @@ -243,6 +243,22 @@ gone_pid() { [ "$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; } +} + # --- agmsg_normalize_instance_id --- @test "normalize: a composite token passes through unchanged (idempotent)" { From 95f8ce1eaac035eb992a5424487bfecc9e91892a Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 22 Aug 2026 12:32:27 -0700 Subject: [PATCH 3/4] fix(instance-id): trust the snapshot's "absent" only when ps exited 0 (#954) The full snapshot removed the poison, but the verdict still ignored ps's exit status. ps can print part of a listing -- even our own $$ -- and then fail non-zero; a target missing from that TRUNCATED output is not proof it is gone, its line may simply never have been reached. The canary shows only that WE were listed, never that the listing FINISHED, so "canary present + target absent" could still read a live pid as dead when ps died mid-write. Capture the exit status and gate on it: a "gone" verdict now requires ps to have exited 0 AND that completed snapshot to include $$. Any non-zero exit reads as UNKNOWN -> alive regardless of the partial output -- which also makes a platform whose ps does not support `-Ao` fail safe (it exits non-zero rather than lying "gone"). A present target is still alive on sight (seeing its line is proof enough, even from a partial listing). Test added: a ps that prints the canary and then exits non-zero must read alive and suppress cleanup. --- scripts/lib/instance-id.sh | 18 ++++++++++++++---- tests/test_instance_id.bats | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index df44194c8..a042b374f 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 probe canary tstat _p _s _rest + 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 @@ -146,6 +146,7 @@ _agmsg_pid_alive_local() { # positive proof the target is gone => dead. # - target present, zombie => gone too. 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 @@ -153,9 +154,18 @@ _agmsg_pid_alive_local() { done < assume alive (#954) - [ -n "$tstat" ] || return 1 # snapshot lists us but not the target -> proven gone - case "$tstat" in Z*) return 1 ;; esac # zombie: exited, not yet reaped + if [ -n "$tstat" ]; then + case "$tstat" in Z*) return 1 ;; esac # zombie: exited, not yet reaped + return 0 # target present -> 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 e9b1a7d29..6e679b6c3 100644 --- a/tests/test_instance_id.bats +++ b/tests/test_instance_id.bats @@ -259,6 +259,22 @@ gone_pid() { [ -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; } +} + # --- agmsg_normalize_instance_id --- @test "normalize: a composite token passes through unchanged (idempotent)" { From a20ef46d0b2fa16b3ccbd26e5d3ba664f8c1e0ce Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 22 Aug 2026 12:48:06 -0700 Subject: [PATCH 4/4] fix(instance-id): keep the ps probe out of set -e's reach (#954) Capturing the snapshot as `probe="$(ps ...)"` returns ps's exit status as the assignment's own. Under errexit, in a caller that did NOT invoke the helper as a condition (if/while/! suppress errexit inside the function; a bare statement does not), a non-zero ps would terminate the shell at that assignment -- before rc is captured and before the UNKNOWN => alive verdict -- so a failed observation leaks to caller-process death instead of "alive". The leaf helper's contract cannot depend on how each of its ~38 call sites spelled the call. Default rc=0 and capture a failure with `|| rc=$?`, which places the assignment in a condition so errexit does not fire, while still recording ps's real exit status for the exit-0 gate. Test added: called as a bare statement under `set -e` with a failing ps, the caller shell continues and gets the alive verdict. --- scripts/lib/instance-id.sh | 10 ++++++++-- tests/test_instance_id.bats | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index a042b374f..9e25c06d9 100644 --- a/scripts/lib/instance-id.sh +++ b/scripts/lib/instance-id.sh @@ -145,8 +145,14 @@ _agmsg_pid_alive_local() { # - $$ present, target absent => ps listed us and did not list the target => # positive proof the target is gone => dead. # - target present, zombie => gone too. - probe="$(ps -Ao pid=,stat= 2>/dev/null)" - rc=$? + # `|| 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 diff --git a/tests/test_instance_id.bats b/tests/test_instance_id.bats index 6e679b6c3..334db0566 100644 --- a/tests/test_instance_id.bats +++ b/tests/test_instance_id.bats @@ -275,6 +275,24 @@ gone_pid() { [ -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)" {