Skip to content

Drive the circuit breaker's clock from tests, not sleep() - #619

Merged
jeremy merged 1 commit into
mainfrom
fix/circuit-breaker-clock
Aug 4, 2026
Merged

Drive the circuit breaker's clock from tests, not sleep()#619
jeremy merged 1 commit into
mainfrom
fix/circuit-breaker-clock

Conversation

@jeremy

@jeremyjeremy commented Aug 4, 2026

Copy link
Copy Markdown
Member

The circuit breaker already had the seam — every time read goes through
CircuitBreaker.now() — it just hardcoded time.Now(). This gives it an
unexported nowFn defaulting to time.Now, so in-package tests can substitute
a clock they advance by hand. The constructor signature is unchanged; the only
non-test caller (hooks.go:36) is untouched.

TestCircuitBreakerResetsStaleHalfOpenAttempts is the reported flake: it slept
against a 100ms StaleAttemptTimeout, so under -race on a loaded runner the
stale reset could fire early. Sleeping to cross a timeout makes the assertion a
bet on scheduler latency.

Every sleeping circuit-breaker test is converted, not just that one — a fake
clock next to four other tests still racing real time would just relocate the
flake. With crossing a timeout now free, they also use realistic production
timeouts (30s open, 2min stale) instead of the millisecond values that only
existed to keep the sleeps short:

  • ClosesAfterSuccesses, StateTransitionsCorrectly, ResetsStaleHalfOpenAttempts
  • SetsHalfOpenLastAttemptAt — its real-time.Now() before/after window
    assertion becomes meaningless (and separately flaky) once the clock is fake,
    so it becomes an exact equality against the Allow() that reserved the slot
  • hooks_test.go's integration test needs no new seam: NewGatingHooks already
    takes the breaker, so the test builds its own, sets nowFn, and passes it in

Verified:go test -race -count=20 ./internal/resilience/ is stable, and
bin/ci is green.

Not claimed: the package is not sleep-free. rate_limiter_test.go has
three sleeps of its own against a separate primitive with no now() seam. They
are untouched, so package runtime does not drop to zero — the claim here is
about the converted tests only.

Closes#586


Summary by cubic

Inject a test-driven clock into the circuit breaker via an unexported nowFn, removing sleeps from tests and fixing the half-open stale attempt flake. No public API changes.

  • Bug Fixes
    • Added nowFn to CircuitBreaker (defaults to time.Now) and routed all time reads through it.
    • Converted circuit breaker and hooks tests to use a fake clock; removed sleeps and used realistic timeouts (30s open, 2m stale).
    • Updated SetsHalfOpenLastAttemptAt to assert the exact timestamp set by Allow().
    • Verified stability with go test -race -count=20 ./internal/resilience/; rate_limiter_test.go sleeps remain. ClosesFlaky under -race: TestCircuitBreakerResetsStaleHalfOpenAttempts #586.

Written for commit 069a5d6. Summary will update on new commits.

Review in cubic

The circuit breaker already had the seam — every time read goes through
CircuitBreaker.now() — it just hardcoded time.Now(). Give it an unexported
nowFn defaulting to time.Now, so in-package tests can substitute a clock they
advance by hand.
TestCircuitBreakerResetsStaleHalfOpenAttempts is the test that flaked: it slept
against a 100ms StaleAttemptTimeout, and under -race on a loaded runner the
stale reset could fire early. Sleeping to cross a timeout makes the assertion a
bet on scheduler latency.
Convert every sleeping circuit-breaker test, not just that one, and use
realistic production timeouts now that crossing them is free:
- ClosesAfterSuccesses, StateTransitionsCorrectly, ResetsStaleHalfOpenAttempts
- SetsHalfOpenLastAttemptAt, whose real-time before/after window assertion is
meaningless once the clock is fake — it becomes an exact equality against the
Allow() that reserved the slot
- The hooks integration test, which needs no new seam: NewGatingHooks already
takes the breaker, so it builds its own and passes it in
`go test -race -count=20 ./internal/resilience/` is stable. The package is not
sleep-free — rate_limiter_test.go has three sleeps of its own against a
primitive with no now() seam, and those are untouched here.
Closes#586
CopilotAI balanced review requested due to automatic review settings August 4, 2026 18:06
@github-actionsgithub-actionsBot added the tests Tests (unit and e2e) label Aug 4, 2026
CopilotAI reviewed Aug 4, 2026

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jeremy
jeremy merged commit 49a3bb5 into mainAug 4, 2026
24 of 25 checks passed
@jeremy
jeremy deleted the fix/circuit-breaker-clock branch August 4, 2026 20:34
jeremy added a commit that referenced this pull request Aug 22, 2026
…or-roundtrip
* origin/main: (96 commits)
ci: bump the github-actions group with 6 updates (#639)
Reject three more doomed invocations before draining stdin (#645)
Stdin `-` support everywhere sensible; usage error for stray `-` elsewhere (#641)
Add hey-cli Windows signing secrets to the release env manifest (#642)
deps: bump the go-dependencies group with 5 updates (#638)
Update nix flake and plugin version for v0.9.1
ci: bump the github-actions group with 4 updates (#633)
Add basecamp files replace: publish a new version of an uploaded file (#634)
Add basecamp files versions — HELD, blocked on the SDK (#622)
Update nix flake and plugin version for v0.9.0
Make the Codex probe's timeout actually bound doctor (#629)
Make the lockstep check catch stale agreement and .yaml workflows (#628)
Keep refreshing opencode's other spelling (#627)
Lint the release the same way we lint everything else (#625)
Install the skill where opencode actually looks (#624)
Take the communiques out of the source tree (#623)
Correct the API coverage claim: 183/184, not 100% (#621)
Stop echoing back step fields the caller never changed (#620)
Drive the circuit breaker's clock from tests, not sleep() (#619)
Tell agents the truth about card column moves (#618)
...
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testsTests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky under -race: TestCircuitBreakerResetsStaleHalfOpenAttempts

2 participants

@jeremy