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..6e6d50929 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -517,6 +517,128 @@ run_named_watcher_for() { _stop_watcher "$pid" } +# 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 +# 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). +# +# 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. +# +# 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_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: the successor claims the pidfile before it signals, and keeps its record (#595)" { + skip_on_windows "watcher process mgmt under Git Bash (#182)" + _record_handover_events + + 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=$! + + # 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 200); do + kill -0 "$w1" 2>/dev/null || break + sleep 0.1 + done + run kill -0 "$w1"; [ "$status" -ne 0 ] + run kill -0 "$w2"; [ "$status" -eq 0 ] + + # 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 "$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 + # 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