Skip to content

test(agent): give the peer reconciler a clock seam and delete the timing sleeps - #392

Closed
gen16k wants to merge 1 commit into
mainfrom
test/357-reconciler-clock-seam
Closed

gen16k wants to merge 1 commit into
mainfrom
test/357-reconciler-clock-seam

Conversation

@gen16k

@gen16k gen16k commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

TestReconciler_SafetyNetSilencedByRecentDirectEvidence is a coin flip on
Windows. It injects direct evidence, sleeps 35 ms, then requires that less than
FallbackAfter (50 ms) has passed — 15 ms of slack against a platform whose
default timer granularity is ~15.6 ms and whose time.Sleep rounds up to the
next tick. One tick of overshoot inverts the assertion. Linux's ~1 ms timers
hide it, so it only ever fails on the Windows leg, on PRs that never touched
cmd/waired-agent — #355, #367, and twice in a row just now on #389.

Both #357 and #384 asked for a clock seam rather than a wider window, and
CLAUDE.md §Test discipline says the same thing ("put the seam below the
behaviour under test"). Widening only moves the coin toss.

The seam is small

The reconciler reaches for the wall clock in exactly two places — Apply
and Tick. Every disco-driven decision is already stamped from the event's own
At (evaluateSwitchLocked(st, e.At, …)), so one now func() time.Time,
defaulted to time.Now in newReconciler, covers the entire surface.

 func (r *reconciler) Tick(ctx context.Context) {
-	now := time.Now()
+	now := r.now()

Production wiring is unchanged in behaviour; the field is read under the same
mutex that already guards Apply/Tick.

What that buys

All seven time.Sleep calls in reconcile_test.go are gone:

  • the five Apply/Tick-timed tests install the fakeClock that
    setup_desired_test.go already defines for the setup executor, and state the
    elapsed time exactly instead of approximating it;
  • TestReconciler_NoFlapWithinDwellTime needed no seam at all — dwell is
    measured from lastSwitchAt, which its own event stamped, so it just passes a
    later At.

Of the seven, only the two in the silencing test were ever at risk: the rest
sleep past a threshold and want it crossed, so overshoot was harmless. They
are converted for determinism, not because they were failing — #357 asked for
exactly that sweep of the siblings. Package tests drop from ~0.5 s of real
sleeping to 0.27 s.

The tests did not go green for the wrong reason

#384 set this bar explicitly (citing #368). Each converted test still fails with
its subject behaviour removed:

mutation test that must fail result
silencing rule (reconcile.go:841) neutralised SafetyNetSilencedByRecentDirectEvidence caught
handshake gate (:847) neutralised StaysDirectIfHandshakeSucceeds caught
dwell gate (:512) neutralised NoFlapWithinDwellTime caught
safety net forced never to fire (:835) SafetyNetFiresWhenProbesSilent + ColdStartUsesSafetyNet both caught

TestNewReconcilerHasAClock pins the production wiring: a constructor path that
forgets the clock would not fail any timing test — those install their own — it
would nil-panic in Apply on a real agent.

Verification

go build ./..., go vet, gofmt, golangci-lint (0 issues),
TestReconciler_* at -count=30. -race not run locally (no cgo toolchain on
this box); CI covers it.

docs-not-needed: test-only seam plus a private struct field; no user-visible surface changes.

Fixes #357
Fixes #384

…ing sleeps (#357, #384)

TestReconciler_SafetyNetSilencedByRecentDirectEvidence was a coin flip on
Windows. It injected direct evidence, slept 35ms, then required that less
than FallbackAfter (50ms) had passed -- 15ms of slack against a platform
whose default timer granularity is ~15.6ms and whose time.Sleep rounds up
to the next tick. One tick of overshoot inverted the assertion. Linux's
~1ms timers hid it, so it only ever failed on the Windows leg, on PRs that
had not touched cmd/waired-agent at all (#355, #367, and again here).

Both issues asked for the seam rather than a wider window, and CLAUDE.md
§Test discipline says the same: put the seam below the behaviour under
test. Widening only moves the coin toss.

The reconciler turns out to need a very small one. It reaches for the wall
clock in exactly two places -- Apply and Tick -- because every disco-driven
decision is already stamped from the event's own At (evaluateSwitchLocked
takes e.At). So a single `now func() time.Time`, defaulted to time.Now in
newReconciler, covers the entire surface.

With it, all seven time.Sleep calls in reconcile_test.go are gone:

  * the five Apply/Tick-timed tests install the fakeClock that
    setup_desired_test.go already defines for the setup executor, and say
    the elapsed time exactly rather than approximating it;
  * TestReconciler_NoFlapWithinDwellTime needed no seam at all -- dwell is
    measured from lastSwitchAt, which its own event stamped, so it just
    passes a later At.

Of those seven, only the two in the safety-net silencing test were ever at
risk: the rest sleep PAST a threshold and want it crossed, so overshoot was
harmless. They are converted for determinism, not because they were
failing. #357 asked for exactly that sweep. Package tests drop from ~0.5s
of real sleeping to 0.27s.

Verified the tests did not go green for the wrong reason -- each one still
fails with its subject behaviour removed:

  * silencing rule (reconcile.go:841) neutralised -> SafetyNetSilenced fails
  * handshake gate (:847) neutralised            -> StaysDirectIfHandshake fails
  * dwell gate (:512) neutralised                -> NoFlapWithinDwellTime fails
  * safety net forced to never fire (:835)       -> both firing tests fail

TestNewReconcilerHasAClock pins the production wiring: a constructor path
that forgets the clock would not fail any timing test (those install their
own) -- it would nil-panic in Apply on a real agent.

Verified: go build ./..., go vet, gofmt, golangci-lint (0 issues),
TestReconciler_* at -count=30.

Fixes #357
Fixes #384

Signed-off-by: gen16k <gen16k@gmail.com>
@gen16k
gen16k force-pushed the test/357-reconciler-clock-seam branch from de54217 to b66c895 Compare August 2, 2026 06:57
@gen16k

gen16k commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate of #395, which landed on main (674a03d) while this was
in CI. Same diagnosis, same seam — now func() time.Time on the reconciler,
defaulted to time.Now in newReconciler, with Apply and Tick as the only
wall-clock reads. I did not see it in flight; two sessions reached the same
place independently.

#395 is the better change and it, not this, should stand:

The only delta left here is converting the five remaining time.Sleep calls in
the file. #395 argues those are safe and is right: they are all lower bounds
("wait PAST FallbackAfter") and sleep never undershoots. That makes converting
them a determinism/runtime nicety (~130 ms), not a fix — not worth churning a
file another session just rewrote. Dropping it.

@gen16k gen16k closed this Aug 2, 2026
@gen16k
gen16k deleted the test/357-reconciler-clock-seam branch August 2, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant