fix(watch): claim the pidfile before displacing the previous holder (#595)

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:

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 '<no-file>' )"
+  [ "$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