test(service): assert circuit-breaker parallelism by overlap, not by stopwatch - #4106
Open
aryanmehrotra wants to merge 2 commits into
Open
test(service): assert circuit-breaker parallelism by overlap, not by stopwatch#4106aryanmehrotra wants to merge 2 commits into
aryanmehrotra wants to merge 2 commits into
Conversation
…stopwatch TestCircuitBreaker_MixedHTTPMethods and TestCircuitBreaker_ParallelExecution both proved "these requests ran in parallel" by timing them: the handler slept one second, five requests were fired, and the test asserted the total came in under two (respectively four) seconds. That is a race between the implementation and the machine. The bound sat one second above a one-second floor, so a loaded runner fails a circuit breaker that is behaving perfectly -- MixedHTTPMethods returned 2.021348s against its 2s bound during a parallel local sweep. It is also weak in the other direction: a serialized-but-fast implementation would have passed both, because elapsed time cannot distinguish "concurrent" from "quick". Replaced with a barrier the handler blocks on. Every request waits until all five are inside the handler at once, which is reachable only if they were dispatched concurrently and unreachable if they were not, at any speed. A request whose peers never arrive answers 503 and the test names it. No wall clock is consulted, so there is nothing left to tune per machine. The barrier's timeout is a liveness bound, not a performance one -- it exists only so serialization fails as an assertion rather than hanging until go test's deadline. The first waiter to time out releases the others, which keeps a failing run at one timeout instead of five and holds it inside the CI step budget. Verified in both directions. Healthy: 10/10 passes under -race, and dropping the two one-second sleeps takes the pair from ~2s to ~0.26s. Regressed: moving `cb.mu.Lock()` above the `f(ctx)` call in executeWithCircuitBreaker -- the exact mistake the comment there warns against -- fails both tests in 15s with "requests were serialized, not parallel".
…eaker-timing-flake
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestCircuitBreaker_MixedHTTPMethodsandTestCircuitBreaker_ParallelExecutionboth proved "these requests ran in parallel" by timing them: the handler slept one second, five requests were fired, and the test asserted the total came in under two (respectively four) seconds.That is a race between the implementation and the machine.
It fails when it shouldn't. The bound sat one second above a one-second floor, so a loaded runner fails a circuit breaker that is behaving perfectly. Caught in the act during a parallel local sweep:
And it passes when it shouldn't. A serialized-but-fast implementation sails through both, because elapsed time cannot distinguish concurrent from quick. The 4s bound on
ParallelExecutionis loose enough to admit a lot of serialization.What replaces it
A barrier the handler blocks on. Every request waits until all five are inside the handler at the same instant — reachable only if the client dispatched them concurrently, unreachable if it did not, at any speed. A request whose peers never arrive answers 503 and the test names it:
No wall clock is consulted, so there is nothing left to tune per machine.
The barrier's timeout is a liveness bound, not a performance one — it exists only so serialization fails as an assertion instead of hanging until
go test's own deadline. What it has to cover is five goroutines each opening a localhost connection, so 15s is roughly four orders of magnitude of headroom, and it is spent only on a run that was going to fail anyway. The first waiter to time out releases the others, which keeps a failing run at one timeout rather than five (120s → 15s) and holds it inside the 5-minute CI step budget.Verified in both directions
A test that no longer fails is only half the job — the other half is proving it still can.
-racegolangci-lintv2.12.2 onpkg/gofr/service/...pkg/gofr/servicepackageThe regression injected was moving
cb.mu.Lock()above thef(ctx)call inexecuteWithCircuitBreaker— holding the lock across the request, which is the exact mistake the "try recovery without holding lock" comment there warns against. The old stopwatch assertions caught it too, but only because the handler slept a second; they would not have caught a fast one.Found while verifying #4105 (Go 1.27), but unrelated to it — the flake reproduces on 1.26, so it is split out to keep that PR to the upgrade.