fix(instance-id): distinguish a failed ps observation from proof of death (#954) - #970
Open
fujibee wants to merge 4 commits into
Open
fix(instance-id): distinguish a failed ps observation from proof of death (#954)#970fujibee wants to merge 4 commits into
fujibee wants to merge 4 commits into
Conversation
Owner
Author
|
Correcting the park reason recorded earlier (it named the wrong blocker). Measured from the actual CI run (32594790457, 2026-08-22), the reds are:
So the blockers are two flakes, one of which (#984) is a pre-existing main-side problem — not an infra wall that makes green unreachable. co1 cleared the change; every relevant local suite passes (instance-id, delivery, remote, launcher, and test_remote_status_liveness). A fresh CI take is planned once the engine-start flake fix (#972) lands. |
…stinguish 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.
…ered 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.
…#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.
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.
fujibee
force-pushed
the
fix/954-pid-alive-observation
branch
from
August 25, 2026 07:45
0944cb7 to
a20ef46
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #954.
_agmsg_pid_alive_local's ps cross-check (reached only afterkill(2)returns ESRCH) read an EMPTY ps result as "gone". A transient ps failure produces the same empty output as a truly-absent pid, so a blip returned "dead" — and roughly 38 call sites act destructively on that: the sync-engine status oracle marks a live engine "stale" (which orphans it and drives a humansync startinto a double-start), the roster lock is released beside a live journal writer, the launcher reuse-check respawns a duplicate bridge, session-start's watcher GC deletes a live watcher's pidfile, and more. Because they all route through this one leaf helper, a single fix covers them.An audit of every caller classified 25 as HARMFUL-if-a-live-pid-reads-dead (they need "definitely absent") and 13 as BENIGN (a false "absent" only skips an optimization, mislabels a status line, or retries next tick). No caller needs "assume-dead-on-doubt", so the helper can return alive on an unresolved observation and every caller is left in a safe state.
The fix
Mix a known-live pid into the SAME observation — our own
$$, alive by definition:$$absent from the output ⇒ ps could not answer ⇒ UNKNOWN ⇒ assume alive, exactly as the existing EPERM branch 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.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 bash builtins (no awk/grep) so a stripped PATH cannot itself become the failed observation — only ps is external.Tests
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: proven-dead ⇒ cleanup fires; ps-cannot-answer ⇒ reads alive and cleanup is suppressed; ps-answers-but-omits-the-target ⇒ dead. Full instance-id suite, and the delivery / remote / watch / launcher caller suites, pass.
Out of scope
The Windows
tasklist | grep -qpath in_agmsg_pid_alivehas the same observation-as-absence shape and is left for a separate change (it needs a Windows environment to exercise).