fix(lock): break a registry lock whose recorded holder is gone (#865)

[fujibee]

Refs #865 — not Closes: this is one of the three fixes the issue describes, and merging it must not close the other two.

Draft, and third in line. The order was changed after this was opened: doctor reports the locks first (#867, no removal at all), doctor --fix sweeps second, and this — acquire breaking a stale lock on its own — comes last, because a wrong verdict here takes a lock away from a live process. Its own review history is the argument: the first version bound the removal to a path rather than to the holder it had judged, so a lock that changed hands between the verdict and the rename could be taken from somebody using it.

Describes head 15d2e1bc4d6be98a9eb1cc05fd5a13ddfbe081b2.

Affects existing users. A team that is wedged today becomes usable again on the next command, without anyone removing a directory by hand. Nothing changes for a lock that is genuinely held.

What was wrong

The acquire loop never asked whether the recorded holder was still running. A lock left behind by a killed process was indistinguishable, to that loop, from one held by a live process doing slow work — so it waited out its budget and failed. Every time. Forever. The message said timed out acquiring registry lock, which describes contention, so the operator went looking for a process that was not there.

Observed on a machine eight minutes after boot under load average 8: two lock directories six seconds apart, both still present nearly three hours later.

Scope: one of the three pieces

The issue names three fixes. This is the one it calls second — asking whether the holder is alive — and it is scheduled last, for the reason above.

#867	doctor names every lock and which directory to remove — removes nothing
next	doctor --fix sweeps
this PR	acquire asks whether the holder is alive and breaks the lock itself — last, and riskiest
later	make an empty lock impossible — build the holder first, put the lock in place atomically
later	fence the pid with a start time, so a recycled number is not read as the same process

The scope was chosen from how these locks are actually made, and there are two ways with very different windows:

• killed between mkdir and the holder write — leaves an empty lock. A narrow window; it takes load and simultaneity. The two observed locks are these.
• killed while the lock is held — leaves a lock with a holder record. The window is however long the work takes, and roster-sync-driver.sh acquires and then runs node in the foreground, so it is the whole of that run. One kill -9, one OOM kill or one force-quit is enough.

The second is far likelier and it has a holder record to ask about. So asking is the change that closes the most for the least.

What it does not do, and why that is a decision

A lock with no holder record is left alone. Nothing here can tell a lock written by an older version of this file from one created microseconds ago whose owner has not written its record yet, and breaking on "I cannot tell" takes a live lock away — which is worse than the leak. There is a case pinning that, so changing it has to be a decision rather than a drift. The piece that makes empty locks impossible removes the question instead of answering it.

A recycled pid reads as alive. Without a start-time fence, pid 1598 is alive cannot be told from the pid 1598 that took this lock is alive, so this waits and reports contention rather than breaking a stranger's lock. That is the safe direction, and it is the third piece.

How it is done

Liveness is _agmsg_pid_alive_local, which already exists for this: it reads EPERM as alive, a zombie as gone, and cross-checks with ps. A bare kill -0 reads EPERM as dead, which in a sandbox turns "cannot signal" into "not running" — and here that would break a lock somebody is holding.

The break claims the holder before it judges or removes anything, and the order is the correctness argument. The first version read the holder, decided it was dead, and then renamed whatever was at that path — between those two steps another breaker can claim the old holder and free the directory, a new owner can take the same path and write a live holder, and the rename then succeeds against the new file. Raised in review, and fixed by renaming first: two processes cannot both claim, and once the holder is claimed no new owner can appear, because the directory is still there.

That fix has no mutation row, and saying so is the point. The interleaving happens between two statements inside one function, so nothing outside it can drive the sequence; a fixture that forces the state anyway reddens the correct code, because with claim-first that state is unreachable. It rests on the structure, not on a measurement.

And one residual it does not close: an operator removing the lock directory by hand — which this code's own message tells them to do — while a breaker is mid-judgement. No ordering inside the helper reaches outside the primitive.

The earlier claim-before-remove reasoning Two processes can reach the same verdict in the same instant; mv of a given file succeeds for exactly one of them, so exactly one goes on to rmdir. It also binds the removal to the holder that was judged: if the lock changed hands in between, the file being renamed is not the file that was read, the rename fails, and no rmdir lands on a lock somebody else has since taken.

The timeout message now says which of the two situations it is — a holder that answered as alive, or a lock with no holder record — because that is what decides where the operator looks next.

And the comment above the traps, which read "a crash with one or two locks held leaves no stale lock": true of EXIT/INT/TERM, not of SIGKILL, an OOM kill, or the machine going down. That is the crash this issue was reported from, and the sentence had no qualifier.

Mutations

Each reverted alone, and each reddens its own case:

reverted	went red
the break never fires	a gone holder's lock is broken
stop asking whether the holder is alive (treat every holder as gone)	a live holder's lock is never broken, and the timeout that names it
remove the lock without claiming the holder first	two racing breakers, only one removes

The live-holder row is the one that matters: breaking a lock somebody is using is worse than the leak this fixes.

Both liveness controls are asserted, not assumed. The "gone" case spawns a process, waits for it, and then checks _agmsg_pid_alive_local says gone before writing that number into the holder — a pid that merely happened to be free would pass for the wrong reason. The "alive" case checks the holder is alive at the moment the acquire runs, because a sleep that failed to start would make it pass by breaking a lock.

Suites

tests/test_registry_lock.bats 21/21 (5 new), tests/test_team.bats 79/79, tests/test_local_team_ids.bats 5/5 — the last one because this sources instance-id.sh, and that suite is the one that runs the core join under an allow-listed PATH.

Not the full suite locally; CI is the gate.