[P0] Replace the single-sample watcher-status assertion in the lease-contention test with a bounded settled-state wait

[mohanagy]

Parent: #649
Milestone: v0.33.0-beta.1 — Trust Foundation
Family: #689, #692, #695 — the deterministic-test-evidence stream that closed #654
Currently blocks: a clean six-lane matrix on PR #681, and therefore every roadmap merge into next

Outcome

tests/unit/watch.test.ts > watch > keeps startup unsettled during live lease contention and recovers after release samples a persisted watcher status exactly once, at a moment when two independent and platform-dependent inputs can legitimately have set it to pending. The assertion therefore fails nondeterministically on macOS while the production behaviour it observes is correct.

Make the assertion deterministic without weakening it.

Observed failure

run   : https://github.com/mohanagy/madar/actions/runs/31767671776
job   : validate (macos-latest, Node 20)   (id 94666770382)
head  : 1a1c5030f9481be25a5f0dc6a21641f988870d44   (PR #681)
lanes : 5 of 6 green; macos-latest / Node 20 alone failed

FAIL tests/unit/watch.test.ts > watch > keeps startup unsettled during live lease contention and recovers after release
AssertionError: expected 'pending' to be 'idle' // Object.is equality

Expected: "idle"
Received: "pending"

 ❯ tests/unit/watch.test.ts:370:71

 Test Files  1 failed | 210 passed (211)
      Tests  1 failed | 2990 passed | 1 expected fail | 2 skipped (2994)

The three assertions immediately before it passed:

await refresh.startupSettled
expect(refresh.startupComplete?.()).toBe(true)   // passed
expect(refresh.initialRebuilt).toBe(true)        // passed
expect(readWatcherStateForGraph(generated.graphPath)?.status).toBe('idle')   // line 370 — failed

So startup completed and the initial rebuild ran. Only the persisted status disagreed.

Mechanism

src/infrastructure/watch.ts:1038, at the end of the startup reconcile:

state.status = eventDirty || pending ? 'pending' : 'idle'

Two independent inputs can each set pending, and neither is synchronised with startupSettled:

1. eventDirty — a filesystem event delivered during startup. The test writes main.ts twice and starts the watcher immediately after the second write. On macOS, FSEvents can surface a write that happened just before the watcher attached, so the event may or may not arrive inside the startup window purely by platform timing.
2. postBuildChanges (watch.ts:1030-1036) — the post-rebuild snapshot diff. When it is non-empty, pending is set to true on the same path.

In both cases pending is the correct persisted status: a change really was observed during startup. The defect is that the test treats one of two legitimate settled outcomes as the only acceptable one, sampled once with no synchronisation.

startupSettled is a promise about startup, not about the watcher having quiesced. The test uses it as if it were both.

Why this surfaced now rather than during #654

The same test passed on all 18 lanes across the three consecutive acceptance matrices 31748654971, 31749252628, 31749780064 at 6b6b3d84. This is a latent race, not a regression introduced on next.

It is not caused by PR #681's diff. That branch touches only docs/qualification/, .github/, package.json, .gitattributes and tests/unit/qualification-contract.test.ts, and nothing in the watcher or its tests. Adding a test file does change how work distributes across the four Vitest workers, which plausibly shifted the timing that exposed the race — a trigger, not a cause. Recorded here rather than on #681 so that the correction is not smuggled into an unrelated diff.

This is material to #654's outcome. The protected complete-suite merge gate is real, but this failure shows the suite still contains at least one single-sample assertion over a value that a live watcher keeps mutating, of the same family #689, #692 and #695 corrected. #654 remains closed; this issue carries the remaining instance.

Required correction

Wait for the settled state instead of sampling it, keeping idle as the required outcome:

await refresh.startupSettled
expect(refresh.startupComplete?.()).toBe(true)
expect(refresh.initialRebuilt).toBe(true)
// A change observed during startup legitimately persists 'pending'. The invariant
// under test is that lease contention resolves to a settled 'idle', not that the
// status is already idle the instant startup settles.
await waitFor(() => readWatcherStateForGraph(generated.graphPath)?.status === 'idle')

waitFor is already the idiom two lines above in the same test, which waits for 'reconciling'.

Improve the helper's failure evidence at the same time: waitFor currently throws the generic Timed out waiting for graph refresh, which names neither the condition nor the value actually observed. A timeout here should report the last observed status.

Non-goals

• Do not relax the assertion to accept pending as a final state. Reaching idle is the point of the test.
• Do not add retry, .skip, .todo, or quarantine.
• Do not change maxWorkers or testTimeout.
• Do not change watcher production semantics. pending after an observed startup change is correct behaviour.

Acceptance criteria

1. The failing assertion is replaced with a bounded wait that still requires idle.
2. waitFor's timeout error names the condition and the last observed value.
3. Every other single-sample assertion over readWatcherStateForGraph(...)?.status in tests/unit/watch.test.ts is audited for the same pattern, and each one is either shown to be synchronised or corrected.
4. A six-lane exact-head matrix is green on the correction branch, with the raw logs inspected rather than the step names trusted.
5. No test is weakened, skipped, retried or quarantined.

Rollback

Single test-only commit; revert restores the previous assertion and the previous nondeterminism.

Refs #654. Refs #689. Refs #692. Refs #695. Related pull request: #681.

[mohanagy]

Acceptance matrix — all 5 criteria met; closing

Completed across two PRs: #701 fixed the reported failure, #702 closed the residuals.

#	Criterion	Evidence
1	The failing assertion is replaced with a bounded wait that still requires idle	#701. idle remains required — the assertion waits for it rather than sampling for it, so a genuinely stuck pending still fails on the 5 s timeout.
2	waitFor's timeout error names the condition and the last observed value	#701. A timeout now reads watcher status 'idle' (last observed 'pending') instead of the generic Timed out waiting for graph refresh.
3	Every other single-sample assertion is audited, and each is shown synchronised or corrected	#702. This issue recorded one residual; the audit found five. See below.
4	A six-lane exact-head matrix is green on the correction branch, raw logs inspected rather than step names trusted	#701 at 3b10bbf9 and #702 at 1c478399, both 6/6, raw logs audited with a positive control so a zero could not be a silent read failure: 0 worker-start signatures, 0 handshake timeouts, one guarded vitest invocation per lane. Post-merge next at ee2115a2 is also 6/6.
5	No test is weakened, skipped, retried or quarantined	Confirmed field by field in independent review.

Criterion 3, discharged in full

#701 replaced the instantaneous sampling but left the second half of the pattern: several sites waited for a status and then read again to assert on it. The watcher is still running at those points, so the second read is another sample of a live poll loop.

Five sites corrected, each now asserting on the state the wait actually observed:

• the pending site asserted pending_since matches a date, which becomes null the moment the debounce fires;
• the transient-rebuild-failure site asserted failed, which the automatic retry it had just scheduled advances away from;
• the recovery site asserted idle with a 10 ms reconcile interval live;
• the two startup sites test(watch): wait for the settled watcher status instead of sampling it (#700) #701 itself touched, which re-read after waitForWatcherStatus with refresh.stop() only in the finally.

waitForWatcherStatus now returns the state it observed, so the class is removed rather than patched site by site — a caller cannot take a second sample without deliberately reintroducing one. Four bare asserting reads remain and all four are terminal: after await watch(...) completes, or after abort plus await watcher.

On the evidence

A race fix cannot be proven by a passing test — the previous code passed too, which is exactly what made it dangerous. The argument is structural, and the site-by-site enumeration is the evidence. That reasoning was checked independently rather than accepted: the review verified each of the four terminal reads by line, confirmed the returned-observation contract closes the class, and confirmed field-by-field that no assertion lost a requirement.

Related parent: #649.