fix(device): state when nothing is claimed and reap dead claims - #1519
Conversation
Two defects found while asking which agent held a connected iPhone. device status printed only "21 stale claims hidden" and no verdict: the "No local advisory device claims found" line was gated on there being zero stale claims too, so the one case where a user most needs to hear that nothing holds the device is exactly the case that never said it. It now reports the empty live set alongside the hidden-stale notice. Nothing ever reaped claims whose owner died abruptly. Claims are released on session close and daemon shutdown, but a killed process leaves its file behind, and a real store had accumulated 21 of them spanning two weeks, every owner dead. Daemon startup now prunes them, next to the existing web-browser orphan cleanup. Pruning is deliberately narrower than the CLI's stale filter: it removes only owner-process-dead claims. owner-state-dir-gone describes a LIVE process whose state dir vanished, and deleting that claim could hand its device to a second session.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Two review findings on the startup prune. The scan classified a claim dead and then unlinked it, but claim paths are derived from the device key: a concurrent daemon can prune the same dead claim while a new session writes its live successor to that exact path, and the unlink would take the successor with it. Liveness and owner token are now re-checked while holding the per-device claim lock, matching what clearAdvisoryDeviceClaim already does, and a file whose name is not the canonical path for the key it contains is left alone. Daemon startup runs outside any diagnostics scope, where emitDiagnostic returns without recording, so neither a successful prune nor a failure produced the promised event. The prune now opens its own scope and flushes, the way emitFatalDiagnostic does.
|
Both findings addressed at [P2] race deletes a fresh live claim — correct, and worse than a stale-read: claim paths are Fixed by re-checking under the per-device lock, mirroring what The regression test drives the real race rather than simulating it: the test acquires the actual claim lock, starts the prune (which blocks on it), writes the live successor into that window, then releases. Verified revert-sensitive — removing the re-check makes it fail by deleting the successor, exactly as described. [P2] prune diagnostics are discarded — also correct. One note: the new race test sits at ~3.1 s against the 2.5 s slow-test budget. That is process-spawn cost from real owner-identity probes plus the real lock, not a sleep — the gate reports it inside the load-variance band. I left it exercising the real primitives rather than mocking them, since the mocked version would not have caught this bug; say the word if you would rather it were trimmed.
|
|
Reviewed exact head The lock/token recheck otherwise resolves the successor-deletion race, and no live-device evidence is needed for this host-filesystem-only behavior. Residual risk: no separately authorized cross-vendor review was performed. |
publishDaemonInfo truncates daemon.log, so the prune's diagnostic was written and then wiped: every successful startup left no device_claim_prune event in the log users are pointed at. The prune now runs after publication. The regression starts the real runtime and reads the resulting daemon.log, because the ordering is the bug — a test around the prune alone passes either way.
|
Fixed at Confirmed exactly as you described: the prune ran at line 333, The prune now runs after Regression is a full runtime start, as asked: it plants a dead claim, calls One incidental find while writing it:
|
thymikee
added
the
ready-for-human
|
Re-reviewed exact head Non-blocking test hygiene: when Residual risk: no separately authorized cross-vendor review was performed. |
|
1 participant
Found while investigating "the iPhone is connected to an agent but
device statusshows nothing except a stale-agents message". Two separate defects; the command was telling the truth, but in a way that reads as broken.Bug 1 — the empty-state verdict is swallowed
In
renderDeviceStatus,'No local advisory device claims found.'is gated onhiddenStaleClaims === 0. So when live claims are empty and stale ones exist, both early returns are skipped and the output falls through to just the hidden-stale notice. The one case where a user most needs to hear "nothing holds this device" is exactly the case that never says it.--jsonhad the answer all along:{"claims": [], "hiddenStaleClaims": 21}.Now:
Bug 2 — dead claims are never reaped
Claims are released on session close and daemon shutdown, but a process that dies abruptly leaves its file behind and nothing collects it. My real store had 21 claims spanning Jul 17 → Jul 31, every single owner dead (verified per-claim with
kill(pid, 0)) — including the iPhone's, orphaned by a session from Jul 23.Daemon startup now prunes them, right next to the existing web-browser orphan cleanup, best-effort and non-fatal with a
device_claim_prunediagnostic.Pruning is deliberately narrower than the CLI's stale filter. It removes only
owner-process-dead.owner-state-dir-gonedescribes a live process whose state dir vanished — deleting that claim could hand its device to a second session. The CLI may hide both; only one is safe to delete.This changes no behaviour beyond housekeeping: the conflict path already steps over a dead-owner claim (
device_claim_advisory_conflict … classification: owner-process-dead), so these files were inert — just unbounded and noisy.Testing
states that nothing is claimed when every claim is stale— verified revert-sensitive (fails when the render change is reverted).prunes claims whose owner is gone and keeps every other claim— asserts the dead claim is deleted while a live claim, a live-but-state-dir-gone claim, and an unparseable file all survive.pnpm check:affected --runpasses.