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

[fujibee]

Declared reviewers: 1

Refs #595, #758, #797.

Landing on integration/remote. Head 3041cefa10fd37338c8a690a6073d21be2377483.

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 its successor's record. It is sound only if the successor's write cannot land between the read at 323 and the remove at 333 — and signalling first is precisely what schedules it there. 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. The predecessor's own read then 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 why the ordering was never questioned, so it is corrected here rather than left to mislead.

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

The control records the order that happened; it does not impose one

Three versions of this control were wrong before this one, and the first two were wrong in the same way.

1. Sleeps between the steps. The ordering was then decided by machine load — a control for a race that was itself a race.
2. Marker-file rendezvous with a bounded wait that proceeds when it expires. That 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.
3. What is here. Nothing is imposed and nothing is waited for. Each process appends one word to a file as it passes the point that matters, and the assertion is about the sequence that came out:

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

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.

Each event carries the pid that wrote it. The first version of this recorded only the words — and both watchers claim the slot, the predecessor when it starts, so grep -n '^claim' | head -1 found the predecessor's claim and the assertion passed on the broken code. The mutation found that; the assertions bind to claim <successor>, signal <successor>, read <predecessor> now, and a missing term is an explicit failure rather than a vacuous pass.

The test then waits for the predecessor to be gone — a condition, not a duration — because its remove is the last thing it does.

Mutations

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 fix reverted (old order restored wholesale)	claim < signal reddens

The first was green against the first version of the ordering test, whose refute was anchored to the start of a line while the restored kill sits after a case pattern. Unanchored now, with the reason beside it.

What this does not claim

That the CI failures on ubuntu-latest 4/4 were this race. The evidence is strong and 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, 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.

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

Measurements, at this head

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

CI at this head                                21 checks, 21 success
  including bats (ubuntu-latest 4/4)

The CI green is the weaker half. That assertion fails intermittently, so one green is a coin landing the right way up; the strong evidence is the mutation table above, where reverting the fix reddens a control that does not depend on timing. An earlier head was also green while carrying the false-green control described above — a reason not to count greens.

Drift

merge-base                 d0b762d47747682e5a466cc1ad0df903c2712ac1
origin/integration/remote  (moved twice while this was open)
git diff --stat <merge-base> origin/integration/remote -- scripts/watch.sh tests/test_watch.bats
  → empty

Measured empty rather than structural: the destination advanced and did not touch this surface. Re-measured immediately before landing.

Not in this PR

#797, the instrument that produced the diagnosis, stays open and is not landed on the strength of this.