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

@jeremy jeremy 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. Closes Flaky 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
Copilot AI balanced review requested due to automatic review settings August 4, 2026 18:06
@github-actions github-actions Bot added the tests Tests (unit and e2e) label Aug 4, 2026

Copilot AI 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 main Aug 4, 2026
24 of 25 checks passed
@jeremy
jeremy deleted the fix/circuit-breaker-clock branch August 4, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky under -race: TestCircuitBreakerResetsStaleHalfOpenAttempts

2 participants