feat(doctor): name the registry locks, and which directory to remove (#865)

[fujibee]

Refs #865 — the first of the three pieces it describes, and the one that removes nothing.

Describes head 3d3b0b784d49cbc1926bf07c617bc05049fa1318.

Affects existing users, and only by printing more: doctor gains a section. No behaviour changes, no exit code changes, nothing is deleted.

What is missing today

A registry lock left behind by a killed process is never broken. Nothing expires, nothing sweeps, and agmsg_lock_release is only ever reached from a trap — which SIGKILL, an OOM kill and the machine going down do not run. Acquire then waits out its budget and says:

agmsg: timed out acquiring registry lock for <dir> after 10s

That describes contention, so the operator goes looking for the process holding it. There is no process. And no message anywhere names the directory to remove, so a machine in this state cannot get itself out. Observed: two locks, six seconds apart, still there nearly three hours later.

What this adds

Registry locks:
  wedged: stale — pid 4711 is not running
    records: token t pid 4711 command join.sh host h
    created: drwx------  2 me  staff  64 18 Aug 05:12 …/teams/wedged/.config.lock
    if no agmsg command is running for this team, remove it:
      rm -r '…/teams/wedged/.config.lock'
      rm -f '…/teams/wedged/.config.lock.holder'
    nothing but the lock lives in there — it holds no team data.

Three answers, not two. held and the holder is gone are what an operator acts on. cannot tell is neither: it names what could not be established rather than asserting the lock is dead.

What the three states change is the sentence, not whether a removal is offered. stale and cannot tell both print the removal, because the operator on a wedged machine needs it in exactly those two cases — a legacy lock with no holder record is the one koit's machine is sitting on. What carries the safety is that the line is conditional (if no agmsg command is running for this team) and that the state above it does not claim more than was measured. Only held withholds it, and says so. Calling cannot tell stale would put a measurement's weight behind a guess.

Two things reach cannot tell, and the second was a defect found in review. A lock with no holder record at all — written either by a library version that recorded nothing or by a process killed between creating the lock and writing its record. And a holder whose pid was never put to the process table: _agmsg_pid_alive_local returns false for a non-numeric value, a leading zero, or a number past the POSIX ceiling, and folding that into "not running" turned pid not-a-pid into a stale verdict with a rm -r beside it. The number is validated with the same ceiling the helper uses before it is asked about.

It removes nothing, and it does not touch the exit code. A team being locked is not a failed prerequisite — reporting it as one would fail doctor every time somebody is joining. Sweeping automatically is a separate decision with a worse failure mode, and it is not made here.

The named team is validated before it becomes a path. cmd_doctor takes an optional team and, until the direct lookup landed, only ever put it in a header sentence — so nothing had ever checked it, and doctor ../outside read a directory outside the store and offered to remove it. Raised in review. It goes through agmsg_validate_team_name now, the same one every other team-taking path uses, and a rejection ends the command rather than falling through to a prerequisites verdict for a question nobody answered.

Reporting one lock is its own function, shared by both callers, so neither has to join paths into a string and read them back — which would mean either an unquoted heredoc (running command substitution on a team name) or unquoted word splitting (globbing one). The first attempt did build such a string, and fed it to a while read loop with no redirection at all: it read stdin and reported nothing. Caught by reading the structure back, not by a test — the cases were written against the behaviour, and a loop that reports nothing fails them, but the run that would have said so was still going when the code was re-read.

A named team is looked at directly, and the sweep does not stop at *. Team names may begin with a dot — the validator rejects empty, ., .., a leading -, /, \ and control characters, and nothing else — and * does not match a leading dot, so .foo was invisible to it. Raised in review.

The summary line is narrowed to All prerequisite checks passed. All checks passed. printed under a stale lock and its removal command reads as withdrawing them, and the exit code deliberately does not move.

Liveness is _agmsg_pid_alive_local, already sourced by remote.sh: 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 would report a live lock as stale.

The removal line is quoted (lib/shquote.sh's scheme), because the store root and a team name can both contain a space and an unquoted path becomes several arguments to rm -r.

Order, and why this piece is first

The issue names three fixes. This is the one with no way to make things worse:

this PR	doctor finds them and says which directory to remove
next	doctor --fix sweeps
later	acquire breaks a stale lock on its own — the riskiest, deliberately last

That third one exists as a draft (#866) and has already earned the caution: its 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 a live process. Review caught it. A wrong verdict there costs a lock somebody is using; a wrong verdict here costs a line of output.

Mutations

Each reverted alone, and each reddens its own case:

reverted	went red
never call a holder alive	a live holder's lock is not called stale
report an unaskable lock as stale	a lock with no holder says it cannot tell
ignore the team argument	doctor <team> reports that team and not another
report even when no lock exists	a healthy install says nothing about locks
fold an unusable pid into "not running"	pid not-a-pid and a pid past the ceiling say cannot tell
sweep with * alone	a team whose name begins with a dot
the old All checks passed.	the summary does not cancel a lock finding
skip the team-name validation	a traversal argument reads nothing; a slash is refused

Both liveness controls are asserted, not assumed. The stale case spawns a process, waits for it, and 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 live case checks the holder is alive at the moment doctor runs.

Before this lands: one rebase step, written down here so it is not re-derived

This branch renames the summary line, and #862 — already on the 1.2.1 integration branch — adds and edits assertions on the old wording. Landing order is #862 first, so when this rebases onto main after 1.2.1 the following will be present and must move to All prerequisite checks passed. (measured on #862's head 15d0067e, four assertions in tests/test_remote.bats):

• remote doctor: passes when age is installed
• remote doctor: passes with the full toolchain installed
• remote doctor: age is optional — its absence does not fail the run
• remote doctor: reports a SHA-256 tool, and its absence does not fail the run — this one is fix(hash): SHA-256 without shasum, and no fingerprint it could not compute (#861) #862's own edit, and the reason the collision exists at all

The five on this branch are already updated. The check after rebasing is grep -c 'All checks passed' tests/test_remote.bats returning 0 for assertions (one comment mentions the phrase and is not one).

Suites

tests/test_remote.bats -f 865 13/13, -f doctor 22/22 (13 new). Not the full suite locally; CI is the gate.

[fujibee]

Shipped in v1.2.1.

This landed through integration/1.2.1, which was squash-merged into main as #868, so none of the commits here appear on main by SHA and GitHub could not close this automatically.

Verified present in main by looking for what this change introduced, not by inference from the merge:

• agmsg 1.2.1 is on npm; main carries the marker this PR added.

Closing. The behaviour is live.

[fujibee]

Shipped in v1.2.1 via integration/1.2.1 (squash-merged as #868), so GitHub could not close this by SHA. Verified in main: the doctor lock report is present.