From 17b8cc81cdd1d2e6647bb8267bb3fe9bbfea9c60 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 11:44:57 -0700 Subject: [PATCH 1/5] fix(watch): claim the pidfile before displacing the previous holder (#595) --- scripts/watch.sh | 46 ++++++++++++++++++++++++++++++++++++------- tests/test_watch.bats | 32 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/scripts/watch.sh b/scripts/watch.sh index eb1da7fc0..4f53ae91c 100755 --- a/scripts/watch.sh +++ b/scripts/watch.sh @@ -200,23 +200,39 @@ mkdir -p "$RUN_DIR" 2>/dev/null || true # Sequential re-invocation of Monitor for this same session_id leaves the # previous watch.sh running but loses track of it (pidfile gets clobbered). -# Stop the prior holder before claiming the slot. ps args check defends -# against pid recycling — only touch processes whose cmdline still matches -# our watch.sh. See #66. +# The prior holder has to go. ps args check defends against pid recycling — +# only touch processes whose cmdline still matches our watch.sh. See #66. # # When ps is unavailable (e.g. Claude Code sandbox), fall back to kill -0 # which confirms the pid is alive but cannot validate the cmdline. +# +# WHO to displace is decided here; the signal is sent AFTER this process has +# written its own pid (#595). Sending it first is what made the predecessor's +# own EXIT guard unsound: that guard removes the pidfile only if it still +# records the predecessor's pid, and read-check-remove is three steps, so a +# successor writing between the read and the remove has its record deleted by +# a process on its way out. The successor never writes again, so the slot +# stays empty while a live watcher owns it. +# +# Claiming first removes the interleaving rather than narrowing it: once the +# file names the successor before the predecessor is ever signalled, the read +# that the predecessor's cleanup performs cannot see the predecessor's own +# pid, so the guard's condition is false and it deletes nothing. +# +# The comment on that guard already described this order as the one in force. +# It was not; the code signalled first. +PREV_PID_TO_DISPLACE="" if [ -f "$PIDFILE" ]; then prev_pid=$(cat "$PIDFILE" 2>/dev/null || true) if [ -n "$prev_pid" ] && [ "$prev_pid" != "$$" ] && _agmsg_pid_alive_local "$prev_pid"; then prev_cmd=$(compat_get_cmdline "$prev_pid" 2>/dev/null || true) if [ -n "$prev_cmd" ]; then case "$prev_cmd" in - *"$SKILL_DIR/scripts/watch.sh"*) kill "$prev_pid" 2>/dev/null || true ;; + *"$SKILL_DIR/scripts/watch.sh"*) PREV_PID_TO_DISPLACE="$prev_pid" ;; esac else # ps unavailable (sandboxed) — skip cmdline validation, rely on kill -0 - kill "$prev_pid" 2>/dev/null || true + PREV_PID_TO_DISPLACE="$prev_pid" fi fi fi @@ -252,6 +268,13 @@ printf '%s\n%s\n%s\n' "${ACTIVE_NAME:-unfiltered}" "$PROJECT_PATH" "$$" > "$FILT echo $$ > "$PIDFILE" +# The slot is ours on disk; now the previous holder can be told to go (#595). +# Nothing waits for it to finish: it is displaced, not depended on, and its +# EXIT will find a pidfile that names this process and leave it alone. +if [ -n "$PREV_PID_TO_DISPLACE" ]; then + kill "$PREV_PID_TO_DISPLACE" 2>/dev/null || true +fi + # --- Say when this watcher is sharing an inbox with another (#683). --- # # A watcher started WITHOUT an active name subscribes to every (team, agent) @@ -317,8 +340,17 @@ READY_FILES="" cleanup() { # EXIT only removes the pidfile if it still records our pid. A successor # watcher (Monitor re-invoked for the same session_id) overwrites $PIDFILE - # with its own pid before killing us; without this guard our EXIT trap - # would erase the successor's record. See #66. + # with its own pid before signalling us, so this read sees the successor's + # pid and leaves its record alone. See #66, and #595 for what happened when + # the signal came first: read, then the successor's write, then this remove + # — a guard that is three steps cannot decide anything about a file another + # process may write between them. The order is what makes it sound, not the + # comparison. + # + # This is still not atomic, and it is not relied on to be: a predecessor + # that entered cleanup for its OWN reasons before any successor existed can + # still race a newcomer's write. That window is not the relaunch path and + # is not what #595 observed. local pidfile_pid="" [ -f "$PIDFILE" ] && IFS= read -r pidfile_pid < "$PIDFILE" || true # Both files are removed by their owner, but they do not share an owner test: diff --git a/tests/test_watch.bats b/tests/test_watch.bats index 7b63848ea..7462d9fcc 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -517,6 +517,38 @@ run_named_watcher_for() { _stop_watcher "$pid" } +@test "watch: the slot is claimed before the previous holder is signalled (#595)" { + # The property is an ORDER between two statements, and the failure it + # prevents is a race, so this is asserted where the order lives rather than + # by trying to lose the race on purpose. A timing test here would pass on + # every machine that happens to win it -- which is how the defect survived: + # `bats tests/test_watch.bats` is green on a developer machine and the + # failure only ever appeared on a CI runner. + # + # What the order buys: the predecessor's EXIT guard removes the pidfile only + # if it still records the predecessor's pid, and that read-check-remove is + # three steps. Signalling first lets the successor's write land between the + # read and the remove, and the successor's record is deleted by a process on + # its way out. Writing first makes the guard's own read see the successor. + local watch_sh="$SCRIPTS/watch.sh" claim displace + claim="$(grep -n '^echo \$\$ > "\$PIDFILE"$' "$watch_sh" | head -1 | cut -d: -f1)" + displace="$(grep -n 'kill "\$PREV_PID_TO_DISPLACE"' "$watch_sh" | head -1 | cut -d: -f1)" + + # Both anchors must exist, or this test passes by finding nothing -- the + # failure mode of every grep-based check. + [ -n "$claim" ] + [ -n "$displace" ] + [ "$displace" -gt "$claim" ] + + # And the takeover block must not signal anyone on its own. The first + # version of this line anchored the pattern to the start of a line, and a + # mutation that put the old `kill "$prev_pid"` back INSIDE the case arm -- + # where it lived before, after the pattern and a `)` -- left this test + # green. Unanchored, because what matters is that the previous holder is + # never signalled through that variable at all, wherever it is written. + refute grep -n 'kill "\$prev_pid"' "$watch_sh" +} + @test "watch: a second unfiltered watcher says it is sharing, and a lone one does not (#683)" { # Two watchers with no active name subscribe to the same unclaimed pairs. The # read cursor is one per (team, agent), so whoever polls first takes the row From 42d04019fc7b33c653d7c57463573264aa39ba1c Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 12:57:20 -0700 Subject: [PATCH 2/5] test(watch): ship the interleaving control, not just its result --- tests/test_watch.bats | 53 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/test_watch.bats b/tests/test_watch.bats index 7462d9fcc..aa82d6010 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -517,6 +517,59 @@ run_named_watcher_for() { _stop_watcher "$pid" } +# Pin the handover's interleaving instead of racing for it (#595). +# +# The defect only appears when three things happen in one order: the departing +# predecessor reads the pidfile, the successor writes its own pid, and only +# then does the predecessor remove what it read. On a developer machine that +# order essentially never occurs — the predecessor is asleep in its poll when +# the signal arrives, so its read lands after the successor's write, the guard +# sees a pid that is not its own, and nothing is deleted. That is why the suite +# is green here and was red on CI runners for weeks. +# +# So the order is imposed rather than waited for: two stalls are injected into +# THIS TEST'S OWN COPY of the script (`$SCRIPTS` is a per-test tree), one +# holding the predecessor between its read and its remove, one delaying the +# successor's write into that window. Nothing outside the test tree is touched. +_pin_handover_interleaving() { + local sh="$SCRIPTS/watch.sh" applied + perl -0pi -e 's/(\[ -f "\$PIDFILE" \] && IFS= read -r pidfile_pid < "\$PIDFILE" \|\| true)/$1\n sleep 3 # PINNED: hold between the read and the remove/' "$sh" + perl -0pi -e 's/^echo \$\$ > "\$PIDFILE"$/sleep 1 # PINNED: write inside that window\necho \$\$ > "\$PIDFILE"/m' "$sh" + # A control on the injection itself: an edit that silently matched nothing + # would leave this test asserting the ordinary case and calling it the race. + applied="$(grep -c 'PINNED:' "$sh")" + [ "$applied" -eq 2 ] +} + +@test "watch: a predecessor stopped mid-cleanup cannot delete the successor's pidfile (#595)" { + skip_on_windows "watcher process mgmt under Git Bash (#182)" + _pin_handover_interleaving + + local sesspid; sleep 600 3>&- & sesspid=$! + local iid="solo.$sesspid" + local pf="$TEST_SKILL_DIR/run/watch.$iid.pid" + + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + local w1=$! + _wait_pidfile "$pf" "$w1" + + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + local w2=$! + # Past the successor's delayed write AND past the predecessor's delayed + # remove, so what is read here is the state both have finished acting on. + sleep 6 + + # The successor is alive: this is about its record, not about it dying. + run kill -0 "$w2"; [ "$status" -eq 0 ] + # And the record it wrote is still there. Before this fix the predecessor's + # remove landed on it and the file stayed gone for the rest of the run. + local seen; seen="$( [ -e "$pf" ] && cat "$pf" 2>/dev/null || printf '' )" + [ "$seen" = "$w2" ] + + kill "$w1" "$w2" "$sesspid" 2>/dev/null || true + wait "$w2" 2>/dev/null || true +} + @test "watch: the slot is claimed before the previous holder is signalled (#595)" { # The property is an ORDER between two statements, and the failure it # prevents is a race, so this is asserted where the order lives rather than From 5477dd544400c3ac1cad192cfbe7a0bc22fb0ed7 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 13:07:43 -0700 Subject: [PATCH 3/5] test(watch): rendezvous on the two events instead of sleeping between them --- tests/test_watch.bats | 53 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/tests/test_watch.bats b/tests/test_watch.bats index aa82d6010..f6e08cda5 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -531,14 +531,32 @@ run_named_watcher_for() { # THIS TEST'S OWN COPY of the script (`$SCRIPTS` is a per-test tree), one # holding the predecessor between its read and its remove, one delaying the # successor's write into that window. Nothing outside the test tree is touched. +# The two processes RENDEZVOUS on files; they do not sleep and hope. Each wait +# is bounded, and the bound is a failure bound rather than a timing assumption: +# reaching it means the other side never arrived, which is itself the answer. +# +# predecessor, after its read: announce read_done, then wait for write_done +# successor, before its write: wait for read_done, write, announce write_done +# +# Under the OLD order the successor signals first, so the predecessor enters +# cleanup, reads its own pid, and blocks — the successor's write then lands +# strictly between that read and the remove. A -> B -> C, by construction. +# +# Under the FIXED order the successor is not waited for by anybody: it waits +# for a read_done that cannot come (the predecessor has not been signalled +# yet), hits its bound, writes, and only then signals. The predecessor's +# cleanup reads a pidfile that already names the successor. The bound is spent, +# not raced. _pin_handover_interleaving() { local sh="$SCRIPTS/watch.sh" applied - perl -0pi -e 's/(\[ -f "\$PIDFILE" \] && IFS= read -r pidfile_pid < "\$PIDFILE" \|\| true)/$1\n sleep 3 # PINNED: hold between the read and the remove/' "$sh" - perl -0pi -e 's/^echo \$\$ > "\$PIDFILE"$/sleep 1 # PINNED: write inside that window\necho \$\$ > "\$PIDFILE"/m' "$sh" + export AGMSG_TEST_PIN_DIR="$TEST_SKILL_DIR/run/pin" + mkdir -p "$AGMSG_TEST_PIN_DIR" + perl -0pi -e 's/(\[ -f "\$PIDFILE" \] && IFS= read -r pidfile_pid < "\$PIDFILE" \|\| true)/$1\n if [ "\${AGMSG_TEST_PIN_ROLE:-}" = predecessor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then # PINNED\n : > "\$AGMSG_TEST_PIN_DIR\/read_done"\n _pin_i=0\n while [ ! -f "\$AGMSG_TEST_PIN_DIR\/write_done" ] && [ "\$_pin_i" -lt 100 ]; do sleep 0.1; _pin_i=\$((_pin_i+1)); done\n fi/' "$sh" + perl -0pi -e 's/^echo \$\$ > "\$PIDFILE"$/if [ "\${AGMSG_TEST_PIN_ROLE:-}" = successor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then # PINNED\n _pin_i=0\n while [ ! -f "\$AGMSG_TEST_PIN_DIR\/read_done" ] && [ "\$_pin_i" -lt 20 ]; do sleep 0.1; _pin_i=\$((_pin_i+1)); done\nfi\necho \$\$ > "\$PIDFILE"\nif [ "\${AGMSG_TEST_PIN_ROLE:-}" = successor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then : > "\$AGMSG_TEST_PIN_DIR\/write_done"; fi # PINNED/m' "$sh" # A control on the injection itself: an edit that silently matched nothing # would leave this test asserting the ordinary case and calling it the race. - applied="$(grep -c 'PINNED:' "$sh")" - [ "$applied" -eq 2 ] + applied="$(grep -c 'PINNED' "$sh")" + [ "$applied" -eq 3 ] } @test "watch: a predecessor stopped mid-cleanup cannot delete the successor's pidfile (#595)" { @@ -549,15 +567,32 @@ _pin_handover_interleaving() { local iid="solo.$sesspid" local pf="$TEST_SKILL_DIR/run/watch.$iid.pid" - AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + AGMSG_TEST_PIN_ROLE=predecessor AGMSG_WATCH_INTERVAL=1 \ + bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & local w1=$! _wait_pidfile "$pf" "$w1" - AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + AGMSG_TEST_PIN_ROLE=successor AGMSG_WATCH_INTERVAL=1 \ + bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & local w2=$! - # Past the successor's delayed write AND past the predecessor's delayed - # remove, so what is read here is the state both have finished acting on. - sleep 6 + + # Wait for the rendezvous to complete rather than for a duration: the + # successor announces its write, and only then can the predecessor's remove + # run at all. Bounded, and the bound failing is a real failure. + local i + for i in $(seq 1 150); do + [ -f "$AGMSG_TEST_PIN_DIR/write_done" ] && break + sleep 0.1 + done + [ -f "$AGMSG_TEST_PIN_DIR/write_done" ] + # The predecessor's remove is the last thing it does before exiting, so its + # exit is the point after which nothing more can touch the pidfile. Waiting + # for the process rather than for a number is what makes this deterministic. + for i in $(seq 1 150); do + kill -0 "$w1" 2>/dev/null || break + sleep 0.1 + done + run kill -0 "$w1"; [ "$status" -ne 0 ] # The successor is alive: this is about its record, not about it dying. run kill -0 "$w2"; [ "$status" -eq 0 ] From 08cde20e19bf080519f0a063822b397ca35146fc Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 13:16:30 -0700 Subject: [PATCH 4/5] test(watch): assert the order that happened, and say which process each event came from --- tests/test_watch.bats | 106 ++++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 45 deletions(-) diff --git a/tests/test_watch.bats b/tests/test_watch.bats index f6e08cda5..646e5ea7b 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -531,80 +531,96 @@ run_named_watcher_for() { # THIS TEST'S OWN COPY of the script (`$SCRIPTS` is a per-test tree), one # holding the predecessor between its read and its remove, one delaying the # successor's write into that window. Nothing outside the test tree is touched. -# The two processes RENDEZVOUS on files; they do not sleep and hope. Each wait -# is bounded, and the bound is a failure bound rather than a timing assumption: -# reaching it means the other side never arrived, which is itself the answer. +# Record the ORDER THAT ACTUALLY HAPPENED; do not try to impose one (#595). # -# predecessor, after its read: announce read_done, then wait for write_done -# successor, before its write: wait for read_done, write, announce write_done +# Two earlier versions of this control were wrong in the same way twice. The +# first slept between the steps, so the ordering was decided by machine load. +# The second had the processes rendezvous on marker files, with a bounded wait +# — and a bound that PROCEEDS when it expires turns the negative control green +# on the broken code: if the predecessor is slow to reach its cleanup, the +# successor's wait times out, it writes anyway, and the predecessor then reads +# a pidfile that already names the successor and correctly deletes nothing. +# The control would have reported the defect as fixed (raised in review). # -# Under the OLD order the successor signals first, so the predecessor enters -# cleanup, reads its own pid, and blocks — the successor's write then lands -# strictly between that read and the remove. A -> B -> C, by construction. +# So nothing is imposed and nothing is waited for. Each process APPENDS a word +# to one file as it passes the point that matters, and the assertion is about +# the sequence that came out. Under either implementation the events happen in +# whatever order they happen; the fix's whole content is which order that is, +# and a recorded order cannot be lost to load. # -# Under the FIXED order the successor is not waited for by anybody: it waits -# for a read_done that cannot come (the predecessor has not been signalled -# yet), hits its bound, writes, and only then signals. The predecessor's -# cleanup reads a pidfile that already names the successor. The bound is spent, -# not raced. -_pin_handover_interleaving() { +# claim the successor wrote its own pid to the pidfile +# signal the successor sent the previous holder its signal +# read the departing predecessor read the pidfile in its EXIT guard +# +# The fix says `claim` precedes `signal`. Everything else follows from that: +# a `read` triggered by the signal necessarily lands after the claim, and the +# guard then sees a pid that is not its own. +_record_handover_events() { local sh="$SCRIPTS/watch.sh" applied - export AGMSG_TEST_PIN_DIR="$TEST_SKILL_DIR/run/pin" - mkdir -p "$AGMSG_TEST_PIN_DIR" - perl -0pi -e 's/(\[ -f "\$PIDFILE" \] && IFS= read -r pidfile_pid < "\$PIDFILE" \|\| true)/$1\n if [ "\${AGMSG_TEST_PIN_ROLE:-}" = predecessor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then # PINNED\n : > "\$AGMSG_TEST_PIN_DIR\/read_done"\n _pin_i=0\n while [ ! -f "\$AGMSG_TEST_PIN_DIR\/write_done" ] && [ "\$_pin_i" -lt 100 ]; do sleep 0.1; _pin_i=\$((_pin_i+1)); done\n fi/' "$sh" - perl -0pi -e 's/^echo \$\$ > "\$PIDFILE"$/if [ "\${AGMSG_TEST_PIN_ROLE:-}" = successor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then # PINNED\n _pin_i=0\n while [ ! -f "\$AGMSG_TEST_PIN_DIR\/read_done" ] && [ "\$_pin_i" -lt 20 ]; do sleep 0.1; _pin_i=\$((_pin_i+1)); done\nfi\necho \$\$ > "\$PIDFILE"\nif [ "\${AGMSG_TEST_PIN_ROLE:-}" = successor ] && [ -n "\${AGMSG_TEST_PIN_DIR:-}" ]; then : > "\$AGMSG_TEST_PIN_DIR\/write_done"; fi # PINNED/m' "$sh" - # A control on the injection itself: an edit that silently matched nothing - # would leave this test asserting the ordinary case and calling it the race. - applied="$(grep -c 'PINNED' "$sh")" - [ "$applied" -eq 3 ] + export AGMSG_TEST_EVENTS="$TEST_SKILL_DIR/run/handover.events" + mkdir -p "$TEST_SKILL_DIR/run" + : > "$AGMSG_TEST_EVENTS" + perl -0pi -e 's/(\[ -f "\$PIDFILE" \] && IFS= read -r pidfile_pid < "\$PIDFILE" \|\| true)/$1\n [ -n "\${AGMSG_TEST_EVENTS:-}" ] && printf %s\\ %s\\\\n read \$\$ >> "\$AGMSG_TEST_EVENTS" # RECORDED/' "$sh" + perl -0pi -e 's/^echo \$\$ > "\$PIDFILE"$/echo \$\$ > "\$PIDFILE"\n[ -n "\${AGMSG_TEST_EVENTS:-}" ] && printf %s\\ %s\\\\n claim \$\$ >> "\$AGMSG_TEST_EVENTS" # RECORDED/m' "$sh" + perl -0pi -e 's/(kill "\$PREV_PID_TO_DISPLACE" 2>\/dev\/null \|\| true|kill "\$prev_pid" 2>\/dev\/null \|\| true)/[ -n "\${AGMSG_TEST_EVENTS:-}" ] \&\& printf %s\\ %s\\\\n signal \$\$ >> "\$AGMSG_TEST_EVENTS" # RECORDED\n $1/g' "$sh" + # A control on the instrumentation: an edit that matched nothing would leave + # every assertion below reading an empty file and passing. + applied="$(grep -c 'RECORDED' "$sh")" + [ "$applied" -ge 3 ] } -@test "watch: a predecessor stopped mid-cleanup cannot delete the successor's pidfile (#595)" { +@test "watch: the successor claims the pidfile before it signals, and keeps its record (#595)" { skip_on_windows "watcher process mgmt under Git Bash (#182)" - _pin_handover_interleaving + _record_handover_events local sesspid; sleep 600 3>&- & sesspid=$! local iid="solo.$sesspid" local pf="$TEST_SKILL_DIR/run/watch.$iid.pid" - AGMSG_TEST_PIN_ROLE=predecessor AGMSG_WATCH_INTERVAL=1 \ - bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & local w1=$! _wait_pidfile "$pf" "$w1" - AGMSG_TEST_PIN_ROLE=successor AGMSG_WATCH_INTERVAL=1 \ - bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$iid" "$PROJ" claude-code >/dev/null 2>&1 3>&- 4>&- & local w2=$! - # Wait for the rendezvous to complete rather than for a duration: the - # successor announces its write, and only then can the predecessor's remove - # run at all. Bounded, and the bound failing is a real failure. + # Wait for the PREDECESSOR TO BE GONE, which is a condition and not a + # duration: its remove is the last thing it does, so once it is gone nothing + # else can touch the pidfile. local i - for i in $(seq 1 150); do - [ -f "$AGMSG_TEST_PIN_DIR/write_done" ] && break - sleep 0.1 - done - [ -f "$AGMSG_TEST_PIN_DIR/write_done" ] - # The predecessor's remove is the last thing it does before exiting, so its - # exit is the point after which nothing more can touch the pidfile. Waiting - # for the process rather than for a number is what makes this deterministic. - for i in $(seq 1 150); do + for i in $(seq 1 200); do kill -0 "$w1" 2>/dev/null || break sleep 0.1 done run kill -0 "$w1"; [ "$status" -ne 0 ] - - # The successor is alive: this is about its record, not about it dying. run kill -0 "$w2"; [ "$status" -eq 0 ] - # And the record it wrote is still there. Before this fix the predecessor's - # remove landed on it and the file stayed gone for the rest of the run. + + # The order that actually occurred. Both events must be present -- an + # assertion over a sequence that is missing one of its terms proves nothing. + local claim_at signal_at read_at + # BY PID, not by event name. Both watchers claim the slot -- the predecessor + # when it starts -- so a search for the first `claim` finds the wrong one and + # the assertion passes on the broken code. The mutation caught that before + # CI did; the events carry the pid that wrote them for exactly this reason. + claim_at="$(grep -n "^claim $w2\$" "$AGMSG_TEST_EVENTS" | head -1 | cut -d: -f1)" + signal_at="$(grep -n "^signal $w2\$" "$AGMSG_TEST_EVENTS" | head -1 | cut -d: -f1)" + read_at="$(grep -n "^read $w1\$" "$AGMSG_TEST_EVENTS" | head -1 | cut -d: -f1)" + [ -n "$claim_at" ] + [ -n "$signal_at" ] + [ -n "$read_at" ] + # The fix, stated as the thing it is: the slot is claimed first. + [ "$claim_at" -lt "$signal_at" ] + # And the consequence, which is what the operator actually loses when the + # order is wrong: the record the live watcher wrote is still there. + [ "$read_at" -gt "$claim_at" ] local seen; seen="$( [ -e "$pf" ] && cat "$pf" 2>/dev/null || printf '' )" [ "$seen" = "$w2" ] - kill "$w1" "$w2" "$sesspid" 2>/dev/null || true + kill "$w2" "$sesspid" 2>/dev/null || true wait "$w2" 2>/dev/null || true } + @test "watch: the slot is claimed before the previous holder is signalled (#595)" { # The property is an ORDER between two statements, and the failure it # prevents is a race, so this is asserted where the order lives rather than From 3041cefa10fd37338c8a690a6073d21be2377483 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 15:14:48 -0700 Subject: [PATCH 5/5] docs(test): delete a comment describing the control this one replaced --- tests/test_watch.bats | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/test_watch.bats b/tests/test_watch.bats index 646e5ea7b..6e6d50929 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -517,20 +517,6 @@ run_named_watcher_for() { _stop_watcher "$pid" } -# Pin the handover's interleaving instead of racing for it (#595). -# -# The defect only appears when three things happen in one order: the departing -# predecessor reads the pidfile, the successor writes its own pid, and only -# then does the predecessor remove what it read. On a developer machine that -# order essentially never occurs — the predecessor is asleep in its poll when -# the signal arrives, so its read lands after the successor's write, the guard -# sees a pid that is not its own, and nothing is deleted. That is why the suite -# is green here and was red on CI runners for weeks. -# -# So the order is imposed rather than waited for: two stalls are injected into -# THIS TEST'S OWN COPY of the script (`$SCRIPTS` is a per-test tree), one -# holding the predecessor between its read and its remove, one delaying the -# successor's write into that window. Nothing outside the test tree is touched. # Record the ORDER THAT ACTUALLY HAPPENED; do not try to impose one (#595). # # Two earlier versions of this control were wrong in the same way twice. The