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

[fujibee]

Declared reviewers: 1

Refs #595, #758, #769, #797.

Landing on integration/remote. Head 17b8cc81cdd1d2e6647bb8267bb3fe9bbfea9c60.

What was wrong

scripts/watch.sh signalled the previous holder of a pidfile before claiming the slot:

215/219  kill "$prev_pid"                                       displace the holder
251      printf ... > "$FILTERFILE"
253      echo $$ > "$PIDFILE"                                    claim the slot
...
323      [ -f "$PIDFILE" ] && IFS= read -r pidfile_pid < "$PIDFILE"    predecessor's EXIT
333      [ "$pidfile_pid" = "$$" ] && rm -f "$PIDFILE"

The guard at 333 exists so a departing watcher does not erase a successor's record. It is sound only if the successor's write cannot land between the read at 323 and the remove at 333. Signalling first is precisely what schedules it there: the predecessor is woken, reads its own pid, and the successor writes before the remove executes. The successor never writes again, so the slot stays empty for the rest of that watcher's life.

The fix is an order, not a check

Decide who to displace where that is already decided, and send the signal after the claim. Then the predecessor's own read cannot see its own pid, so the guard's condition is false and it removes nothing.

The guard's comment already described this order as the one in force — "a successor watcher overwrites $PIDFILE with its own pid before killing us". The code did the opposite. That comment is the reason the ordering was never questioned, so it is corrected here rather than left to mislead the next reader.

Tightening the check instead would not close it: read-check-remove is three steps whatever the comparison is. The order removes the interleaving; a better comparison only narrows it.

Measured, both directions

The predecessor is stopped between its read and its remove, and the successor's write is pushed into that window — the interleaving pinned rather than raced for. Only the test environment's copy of the script is patched; scripts/watch.sh in the repository was verified unmodified afterwards.

	pidfile after the dust settles	successor
no pinning (control)	the successor's pid	alive
A→B→C pinned, before this change	<no-file>	alive
A→B→C pinned, after this change	the successor's pid	alive

The first attempt at this control did not reproduce it, and that is the useful part. With a 5s poll interval the predecessor is asleep when the signal arrives, so its cleanup read happens after the successor's write, pidfile_pid != $$, and the guard correctly deletes nothing. The guard is not "usually right": it is right or wrong according to an ordering that nothing established.

The test asserts the order, not a timing

A timing test passes on every machine that wins the race — which is how this survived: bats tests/test_watch.bats is green on a developer machine, and the failure only ever appeared on CI runners. So the new case reads the two statements' positions, with both anchors required to exist so it cannot pass by finding nothing.

mutation	result
kill "$prev_pid" restored inside the takeover block	the refute reddens
the displacement kept, but moved above the claim	the ordering assertion reddens

The first mutation was green against the first version of this test: its refute was anchored to the start of a line, and the restored kill sits after a case pattern on the same line. The mutation found the hole before CI could; the pattern is unanchored now, and the reason is written beside it.

What this does not claim

That the CI failures on ubuntu-latest 4/4 were this race. The evidence is strong and it is circumstantial: at the timeout the run directory holds .watch-start.<sid>.<successor-pid> (written at 389) and the filter file (251), and not the pidfile (253) written between them, while the successor is alive and still the same process. The filter file is the natural control — same writer, one line apart, but removed by an owner recorded inside the file (#766) rather than by read-check-remove. Only the deletable-by-a-departing-predecessor one was deleted.

Nobody observed the removal itself, and the write at 253 has no error handling. See #595 for the full account.

Two other explanations were tested and dropped: the poll window (widened 3s → 10s in #797, still red) and load (still red on an idle queue).

Measurements

bats tests/test_watch.bats                     19 tests, 0 failures
.github/scripts/check-enforced-assertions.sh   638, at the baseline (638)

Drift

The destination moved while this branch existed — #792 landed. Drift is measured per file, and the files this touches did not move:

merge-base                 d0b762d47747682e5a466cc1ad0df903c2712ac1
origin/integration/remote  5d3cc9eba4bbaf22ef94e2d652722ff552c2fe13
git diff --stat <merge-base> origin/integration/remote -- scripts/watch.sh tests/test_watch.bats
  → empty

So this is measured empty, not structural — the destination has advanced and did not touch the surface under review. Re-measured immediately before landing.

Not in this PR

The instrument that produced the diagnosis is #797, which stays open and is not landed on the strength of this. If this change is right, the next ubuntu-latest 4/4 on a branch carrying it is green — and that is the only thing that closes #595.