fix(core): doctor and the lease health monitor make their registry writes in the decision section

[V3RON]

Root cause

Registry writes can be lost when two of them overlap, and Doctor made its writes outside the shared SerializedDecision. So did leasing's LeaseHealthMonitor, which has the same race on leased devices.

1. Every Registry mutation builds its new devices array from this.#devices before it awaits the write. #commit only sets this.#devices = devices after writeFileAtomic resolves (src/core/registry.ts:918-947; markDeviceMissing builds its array at registry.ts:608). Suppose writer W reads the registry while writer M's write is in flight, and W's write finishes after M's. Then W sets #devices to its stale array and undoes M's change. On this architecture that is safe only because callers make each read-decide-commit inside the one SerializedDecision (COMPONENTS.md: Registry "does not decide a transition: callers do, inside a decision section").
2. The warm pool's boot commits through ManagedDeviceLifecycle#commit / #commitWithoutRelease, which run inside decisions.run (src/core/managed-device-lifecycle.ts:306-333).
3. Doctor made all of its registry writes outside any decision section: markDeviceMissing (doctor.ts:748 on main), the foreign-state-change transition plus clearForeignStateDetected (doctor.ts:717, 805), and markForeignStateDetected / markForeignProvenanceDetected (doctor.ts:451-457).
4. In the e2e, doctor --fix first corrects deviceA: ready -> shutdown (doctor.ts:805). That emits device.shutdown. The warm pool reacts, calls makeReady on deviceA (the call the test already expects), and commits shutdown -> ready for deviceA in a decision section. Meanwhile Doctor moves on and runs markDeviceMissing(deviceC).
5. The failing interleaving: the warm pool's transitionDevice(A, "ready") reads #devices while markDeviceMissing(C) is still writing, so it reads C as ready. M's write finishes and sets C to deleted. Then W's write finishes and sets #devices back to its array, where A is ready and C is still ready. list --devices reads C as ready, and the test fails with vanished deviceC should be marked deleted, not recreated: expected 'ready' to be 'deleted' (e2e/doctor-drift.test.ts:217). The other order, where M lands last, undoes A's ready instead. The test accepts that (["shutdown", "ready"]), which is why only C ever showed up as the failure.
6. The same gap in leasing: LeaseHealthMonitor wrote clearRecovery (when a leased device reads running again, lease-health-monitor.ts:240 on main, and after a successful reboot, :335) and markRecoveryAttempt (:283) outside any decision section. A sectioned commit for another device that overlaps one of these writes (an idle shutdown, a warm-pool boot, Doctor) can undo it, or be undone by it, in the same way: a recovery attempt that is not counted, recovery markers that come back after recovery, or the other device's transition lost.

Fix

Doctor takes the shared decisions (wired in createCore, like every other component that writes the registry) and makes each of its own registry read-decide-commit sections inside decisions.run:

• the registry-device-missing fix and the legacy fix's final mark-missing (the legacy destroyLegacy driver call stays outside, because a decision section holds no driver work);
• the foreign-state-change fix (the transition and the flag clear, together);
• markForeignStateDetected / markForeignProvenanceDetected, which run on every reconcile, --fix or not.

leaseExpirer.expire and quarantine.enterFromStalledTransition already take the decision section themselves, so they are not wrapped. Wrapping them would nest run and deadlock. Startup and doctor.run both call reconcile outside any decision section, so nothing nests.

LeaseHealthMonitor gets the same treatment. It takes decisions (leasing already gets core's shared gate in createLeasing, where the acquisition, release, request-book and startup components take it too), and runs clearRecovery (both call sites) and markRecoveryAttempt inside decisions.run. Each of those registry methods re-reads #devices itself, so the read, the decision and the commit are all inside the section. Nothing nests: the tick and the detached recovery are started by a timer, never from inside a section. recoverLeased and releaseDeviceLost take the section themselves, and they are called outside these wraps.

Why this removes the race by construction: SerializedDecision.run starts a section only after the previous one has finished. Doctor's read of the snapshot, its write, and the point where #devices is set now all happen inside one section. The warm pool's commit happens inside another section. So one of them always reads the registry after the other's write has landed, and neither can set #devices from a stale array. Each fix also re-reads the snapshot inside its section, so it decides on current state.

Why this is the simplest fix: it is the rule the code already has (callers decide inside a decision section), applied to the two writers that skipped it. Making Registry itself queue writes would change the commit and emit contract for every caller. The rest of the diff is mechanical: decisions added to each new Doctor(...) in the tests and to the monitor's test harness, because the option is required, as it is on every other component that takes it. HeldWritesFilesystem moved from doctor.test.ts to src/core/testing.ts so both regression tests use it.

Evidence

Permanent regression test (no production test hooks): src/core/doctor.test.ts, the it.each "… keeps its write, and another device's transition committed in the decision section while that write is in flight keeps its own". A HeldWritesFilesystem holds every state write until the test releases it. The test starts the reconcile and waits until Doctor's first write is held. Then it starts what the warm pool does, decisions.run(() => registry.transitionDevice(warm, "ready")), and releases the held writes one at a time, oldest first, each after all pending microtasks have run. There are no timers and no randomness: the order is fixed. There is one case per Doctor write: mark missing, the foreign-state fix, the foreign-state flag, and the provenance flag.

On the unfixed code (git show origin/main:src/core/doctor.ts > src/core/doctor.ts; for the e2e build also restore src/core/create-core.ts from main, or the build fails with TS2353 on decisions):

$ pnpm exec vitest run --project unit src/core/doctor.test.ts -t "keeps its write"
     × '--fix marking a missing device deleted' keeps its write, and another device's transition committed in the decision section while that write is in flight keeps its own
     × '--fix shutting down a device observed…' keeps its write, …
     × 'a run flagging a device observed stop…' keeps its write, …
     × 'a run flagging a device whose erasabl…' keeps its write, …
AssertionError: expected { createdAt: 10000, …(9) } to match object { state: 'deleted' }
-   "state": "deleted",
+   "state": "ready",
AssertionError: expected 'shutdown' to be 'ready' // Object.is equality
AssertionError: expected { createdAt: 10000, …(9) } to match object { foreignStateDetectedAt: 10000, …(1) }
AssertionError: expected { createdAt: 10000, …(9) } to match object { …(2) }
      Tests  4 failed | 79 skipped (83)

The first case is the issue's failure exactly: a device --fix marked deleted reads back as ready.

With the fix: Tests 4 passed. The whole file: Tests 83 passed (83).

The e2e itself, forced into the interleaving (temporary patch, not committed): this patch makes the warm pool's commit of deviceA read the registry before Doctor marks C missing and land after it, and makes the CLI return after both:

--- a/src/core/registry.ts
+++ b/src/core/registry.ts
@@ transitionDevice
     const updated = this.#transitioned(device, to, update);
     const devices = [...this.#devices];
     devices[index] = updated;
+    const flags = globalThis as { __simlockDelayReady?: boolean };
+    if (to === "ready" && flags.__simlockDelayReady === true) {
+      flags.__simlockDelayReady = false;
+      await new Promise((resolve) => setTimeout(resolve, 1500));
+    }
     await this.#commit(devices, this.#leases);
--- a/src/core/doctor.ts
+++ b/src/core/doctor.ts
@@ #applySafeFix, case "registry-device-missing"
       case "registry-device-missing":
+        await new Promise((resolve) => setTimeout(resolve, 1000));
         await this.options.decisions.run(() => this.#fixMissingDevice(finding.deviceId));
+        await new Promise((resolve) => setTimeout(resolve, 2000));
         break;
@@ #fixForeignStateChange, after the ready -> shutdown transition
         payload: { deviceId: finding.deviceId, initiator: "doctor" },
       });
+      (globalThis as { __simlockDelayReady?: boolean }).__simlockDelayReady = true;

The waits go around the call in #applySafeFix, outside the decision section. On the unfixed code that line is await this.#fixMissingDevice(finding.deviceId);, and the same two waits go around it. Do not put them inside #fixMissingDevice: with the fix it runs inside decisions.run at both call sites (doctor.ts:712 and :793), so a wait there holds the section, and the trace below would not happen. The __simlockDelayReady lines and the trace are the same in both runs. Both runs below were made with exactly this patch (and a trace line in transitionDevice and markDeviceMissing).

Unfixed + patch:

$ pnpm test:e2e e2e/doctor-drift.test.ts
 FAIL  |e2e| e2e/doctor-drift.test.ts > doctor and drift > reports registry vs. reality drift, applies only registry-safe fixes, and skips leased devices
AssertionError: vanished deviceC should be marked deleted, not recreated: expected 'ready' to be 'deleted' // Object.is equality
 ❯ e2e/doctor-drift.test.ts:217:85
      Tests  1 failed | 2 passed (3)

A trace from the patched registry shows the order: A ready->shutdown at t, A shutdown->ready read at t+2 ms (it lands at t+1502), C marked missing at t+1004.

Fixed + the same patch: Tests 3 passed (3). The trace shows A shutdown->ready read at t+7 and C marked missing at t+1513 instead. Doctor's section waited for the warm pool's section to finish, then read the registry with A already ready and marked C deleted on top of it.

Sanity check, not proof: the unpatched e2e file passes after the fix.

The health monitor (permanent regression test): src/leasing/lease-health-monitor.test.ts, the it.each "… keeps its write, and another device's transition committed in the decision section while that write is in flight keeps its own", one case per monitor write: recording a recovery attempt, clearing recovery markers on a device observed running, and clearing them after a successful reboot. Same method as Doctor's test: the setup brings the monitor to just before one write (fake clock ticks; failOn / hangMakeReady on the FakeDriver to stop at the right point), the HeldWritesFilesystem holds that write, the test commits ready -> shutdown for an idle device in decisions.run while it is held, and releases the held writes oldest first. It then checks the leased device's recoveryAttempts and that the idle device is shutdown.

On the unfixed code (git stash push src/leasing/lease-health-monitor.ts src/leasing/create-leasing.ts):

$ pnpm exec vitest run --project unit src/leasing/lease-health-monitor.test.ts -t "keeps its write"
     × 'recording a recovery attempt' keeps its write, and another device's transition committed in the decision section while that write is in flight keeps its own
     × 'clearing recovery markers on a device…' keeps its write, …
     × 'clearing recovery markers after a suc…' keeps its write, …
AssertionError: expected undefined to be 1 // Object.is equality
AssertionError: expected 1 to be undefined // Object.is equality
      Tests  3 failed | 15 skipped (18)

The idle device's shutdown commit, which read the registry while the monitor's write was in flight, landed last and set #devices back to its array: the attempt is gone, or the cleared markers are back. With the fix: the whole file, Tests 18 passed (18).

Mutation testing: the hook counted #fixMissingDevice's body as changed (the branch's commit pair re-indented it and put it back), and listed its mutants. Fixed in 516e775 and 48b8f13: the state !== "deleted" guard repeated markDeviceMissing's own early return, so it is gone; the "doctor" initiator is asserted on the device.deleted event; the lease guard's per-device match is pinned by "--fix marks a missing device deleted although another device holds a lease". The last push still lists src/core/doctor.ts lease.deviceId === deviceId -> true in that guard, with an empty coveredBy in the report, so it looks like a reused earlier result. Applied by hand, the new test fails on it. The round 3 code review confirmed this: with the mutant applied, that test fails. The wiring of decisions into Doctor and LeaseHealthMonitor is pinned by one test each in create-core.test.ts and create-leasing.test.ts, each of which fails with a pass-through section.

The last push (4b02565) ran the hook on every changed range: 48 mutants, 6 alive.

• src/core/doctor.ts:717 ConditionalExpression: true: the reused result above (26 of 41 results were reused); the lease-guard test kills it.
• src/core/testing.ts:70-72 and :84 (five mutants in HeldWritesFilesystem.held and releaseUntil): test-only helper code. The loop guards only decide how long held() polls before the test's own releaseUntil drains the held writes in order, and the two strings only appear in the failure of a test that has already hung. A mutant there can make a test start releasing earlier or hang, never pass an assertion it would fail. The helper's own tests were dropped (round 3 found two of their titles unproven) rather than rewritten.

Assumptions

• Assumption: wrapping only the registry call is enough for the monitor. clearRecovery and markRecoveryAttempt read #devices themselves, and clearRecovery does nothing when no markers are left, so the tick's earlier snapshot only decides whether to call it, not what it writes.
• Assumption: the monitor's other registry changes need no wrap here. #giveUp goes through releaseDeviceLost, which takes the section itself (LeaseReleaseCoordinator), and recoverLeased takes it itself as well. Wrapping either would nest run and deadlock.

Review

Spec review: 0 blocking, 0 fixed, 1 notes. Code review: 10 blocking, 5 fixed, 2 notes. Claims review: 8 blocking, 8 fixed, 8 notes. Comment review: 36 blocking, 26 fixed, 0 notes.
Mutate: 48 mutants, 6 alive.

Round 3 confirmed two code findings (both titles in the helper's own test file src/core/testing.test.ts) and two comment findings. By the maintainer's choice they were fixed in 4b02565 without another review round: the helper test file is deleted and the two comments removed (fixed unreviewed).

Rejected:

• code: src/core/testing.ts:74 doc comment is a what-comment — duplicate of the comment review finding, fixed there
• code: src/core/testing.ts:82 doc comment is a what-comment — duplicate of the comment review finding, fixed there
• code: src/core/doctor.test.ts:460 table comment is a what-comment — duplicate of the comment review finding, fixed there
• code: src/leasing/lease-health-monitor.test.ts:638 table comment is a what-comment — duplicate of the comment review finding, fixed there
• comments: src/core/doctor.test.ts:1000 threshold arithmetic is a what-comment — it explains why the advance value was chosen
• comments: src/core/doctor.test.ts:1051 threshold arithmetic is a what-comment — it explains why 9_001 is past the threshold
• comments: src/core/doctor.test.ts:1201 threshold arithmetic is a what-comment — it explains why 6_001 is past the threshold
• comments: src/core/doctor.test.ts:1259 threshold arithmetic is a what-comment — it explains why 6_001 sits between the two thresholds
• comments: src/core/doctor.test.ts:2313 comment points to ADR 0001 — it is a why (the leak); the ADR mention is incidental
• comments: src/core/doctor.test.ts:2332 "Same reasoning" is a pointer — it states its own why: unobservable is not gone
• comments: src/core/doctor.test.ts:2777 comment is a what-comment — it says why the test compares the spy instead of counting timers
• comments: src/core/doctor.test.ts:1070 comment restates the test's claim — it says why the claim, not the clock, decides
• claims: src/core/doctor.test.ts:2494 title promises more than the body proves — not blocking: the body proves the transition survives the held write
• claims: src/core/create-core.ts:292 history in a why comment — not blocking: the clause states a current fact that supports the why
• claims: src/leasing/lease-health-monitor.ts:253 comment restates the early return — not blocking: it says why skipping the device is safe
• comments: src/core/doctor.ts:710 "(safety rule 4)" is a pointer that goes stale — the comment says why --fix installs nothing; citing a rule does not point at a declaration

Closes #407

Written by an agent.

[V3RON]

Review notes

Not blocking, not verified. Each is one reviewer's claim.

• spec: src/leasing/lease-health-monitor.ts:243 monitor writes now wait on the decision section (also :286, :337, create-leasing.ts:220); the issue names only the doctor flake, so this adds deadlock risk outside its scope
• code: src/core/doctor.ts:712 both callers of #fixMissingDevice wrap it separately
• code: src/core/doctor.ts:718 the push hook's lease.deviceId === deviceId -> true survivor is a stale reused result; the HeldWritesFilesystem mutants were not re-run by an agent
• claims: src/core/doctor.test.ts:138 the title says the other transition commits "while that write is in flight"; the section serializes them, so it only starts then and commits afterwards
• claims: src/leasing/lease-health-monitor.test.ts:885 the title says the idle shutdown commits "while the held write is in flight"; it actually waits behind the monitor's section
• claims: docs/internal/ARCHITECTURE.md:1531 lists only lease requests, doctor and the health monitor as writing inside the gate; reclaim, quarantine, the provisioner and enrollment also do
• claims: src/core/doctor.test.ts:1186 the comment and the title at 1165 say "emulator", but the test uses an iOS FakeDriver
• claims: src/core/doctor.test.ts:2494 the title says the transition is "committed while that write is in flight"; it starts during the held write but commits after it
• claims: src/core/create-core.ts:292 "and is now how every release purges" reads as history inside a why comment
• claims: src/leasing/lease-health-monitor.ts:253 "Cap reached; ... retried next tick" mostly restates the early return

Written by an agent.