From abbac1ae17db38aabf23871c1c97089b3f54ae5a Mon Sep 17 00:00:00 2001 From: fujibee Date: Wed, 26 Aug 2026 01:05:27 +0900 Subject: [PATCH 1/2] probe(instance-id): record which branch decided liveness -- NOT FOR MERGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnostic only, for #970 / #954. This branch exists to produce one CI run and be deleted. `_agmsg_pid_alive_local` gained two verdicts that used to be "dead": ps ran, pid absent, canary absent dead -> alive ps failed entirely dead -> alive Both open in the same direction — unknown becomes alive — and two macOS shards fail on this PR in ways that would follow from a liveness answer that never says dead. Whether they actually enter those branches on the runner is not something I can tell from here, and this stops me guessing. Appends to a file rather than writing stderr. The helper has 61 call sites, some of which capture stderr and some of whose tests assert on it; a probe that changes what a caller sees is measuring itself. Off unless AGMSG_PIDALIVE_TRACE is set. One trace point had to be removed after it broke a test: `instance-id: liveness answers alive on the builtin, before any subshell` reads the function's own text and requires the fast path to end in `return 0;`. Wrapping that line in braces for the probe changed the shape it asserts. That is the hazard this file's conditions were about, met in the first attempt. Measured, same suite, probe off and on: 67/67 both ways, 17 trace lines written. Locally every UNKNOWN comes from #954's own tests, which break `ps` on purpose — the ordinary paths do not reach it here. --- scripts/lib/instance-id.sh | 65 +++++++++++++++++++++++-- tests/test_instance_id.bats | 94 +++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 5 deletions(-) diff --git a/scripts/lib/instance-id.sh b/scripts/lib/instance-id.sh index 6eaee956..d8422075 100644 --- a/scripts/lib/instance-id.sh +++ b/scripts/lib/instance-id.sh @@ -109,8 +109,23 @@ _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). +# TEMPORARY (probe/954-branch-trace, never to be merged). Records which branch +# of _agmsg_pid_alive_local produced the verdict, so a CI run can say what it +# did instead of us guessing from the outside. +# +# Appends to a file rather than writing stderr: this helper has 61 call sites, +# some of which capture stderr and some of whose tests assert on it. A probe +# that changes what callers see would be measuring itself. +# +# Off unless AGMSG_PIDALIVE_TRACE is set, so the probe cannot alter a normal run. +_agmsg_pidalive_trace() { + [ -n "${AGMSG_PIDALIVE_TRACE:-}" ] || return 0 + printf '%s\t%s\t%s\n' "${BATS_TEST_NAME:-}" "$1" "$2" \ + >> "$AGMSG_PIDALIVE_TRACE" 2>/dev/null || true +} + _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 +146,50 @@ _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 < 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 + _agmsg_pidalive_trace ps-absent-canary-seen "$pid"; return 1 + fi + _agmsg_pidalive_trace UNKNOWN-assumed-alive "rc=$rc canary=$canary pid=$pid" 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)" { From 6688a1800712b481218e9b318beba61130047bdc Mon Sep 17 00:00:00 2001 From: fujibee Date: Wed, 26 Aug 2026 01:07:02 +0900 Subject: [PATCH 2/2] probe(ci): collect the liveness branch trace from every shard -- NOT FOR MERGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enables AGMSG_PIDALIVE_TRACE for the shard run and uploads the file. always() on the upload, because a GREEN shard's distribution is the control: reading only the red ones cannot separate 'entered that branch and therefore failed' from 'enters that branch every time'. Writes to a file, never to the step's stdout — the same reason the hang sampler beside it does, and the reason the helper's own probe does: an instrument on a channel some callers capture would be measuring itself. Step shape and indentation checked against the existing 'Upload hang samples' step, since no YAML parser is available locally. --- .github/workflows/tests.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index eeb5f4e9..29432ce8 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -330,6 +330,14 @@ jobs: - name: Run bats suite (this shard) if: needs.changes.outputs.docs_only != 'true' + env: + # TEMPORARY (probe/954-branch-trace). Makes _agmsg_pid_alive_local + # record which branch produced each verdict. Writes to a file and + # never to this step's stdout, for the same reason the hang sampler + # does: the helper has 61 call sites, some capturing stderr and some + # asserting on it, so an instrument on that channel would be measuring + # itself. + AGMSG_PIDALIVE_TRACE: ${{ runner.temp }}/pidalive-trace.tsv run: | set +e xargs bats --print-output-on-failure < shard-files.txt @@ -337,6 +345,19 @@ jobs: : > "$RUNNER_TEMP/bats-done" exit $status + # always(), because a GREEN shard's distribution is the control. Reading + # only the red ones cannot tell "it entered that branch and therefore + # failed" from "it enters that branch every time". + - name: Upload liveness branch trace + if: always() && needs.changes.outputs.docs_only != 'true' + continue-on-error: true + uses: actions/upload-artifact@v4 + with: + name: pidalive-trace-${{ matrix.os }}-${{ matrix.shard }} + path: ${{ runner.temp }}/pidalive-trace.tsv + if-no-files-found: ignore + retention-days: 3 + # always() so this also runs when the job is cancelled at its cap, which # is the only case that matters here. # continue-on-error and a best-effort body: this step exists to describe a