A rate-limiter test spends its 20ms margin on the scheduler, not on the clamp it is named after - #763
Merged
Merged
Conversation
TestRateLimiterWaitNeverSleepsPastTheDeadline ran Wait against a real 200ms Retry-After block with a 220ms deadline and timed it. A wake-up more than 20ms late spends the budget, Wait correctly reports that the deadline passed, and the test fails with the client-limit error rather than on the jitter clamp it is named after. Nothing promises a 200ms sleep returns within 20ms of its due time; a wider margin is the same defect with a longer fuse. The clamp is now sleepWithin(wait, remaining), and the test checks that decision directly -- no timers, no wall clock. Waiting a block out end to end stays covered by TestRateLimiterWaitOutlastsAShortRetryAfter. 30 of 30 failures with the test and 256 busy loops pinned to the same two cores, and again on one saturated core; 0 of 30 on both after.
Contributor
There was a problem hiding this comment.
🟢 Approved
The refactor preserves behavior while removing the test’s dependence on scheduler timing.
Pull request overview
Replaces a scheduler-sensitive rate-limiter timing test with deterministic clamp testing.
Changes:
- Extracts jitter/deadline calculation into
sleepWithin. - Tests jitter, refill-floor, and budget-clamping cases directly.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/resilience/rate_limiter.go |
Extracts and uses the sleep-budget calculation. |
internal/resilience/gate_test.go |
Replaces wall-clock assertions with deterministic cases. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 18, 2026
jorgemanrubia
added a commit
that referenced
this pull request
Sep 18, 2026
Both new tests raced the clock they were testing. The server one set a Retry-After 20ms out and made that the deadline, with two store writes between the deadline being set and the take that had to observe the block: on a loaded runner the budget is gone before take ever looks, the gate never records a server block, and the test reports the client limit — the very defect it exists to catch, passing itself off as the fix failing. The client one had the same shape with a 100ms window, and both computed the reported wait from a real elapsed time that a slow enough machine rounds up a second. This is the 20ms margin from #763 again, the thing this whole card came out of. A gate reaches a spent budget by waking past its deadline, and no real timer can be asked for that. So the tests no longer ask: the limiter reads a clock it is given, pause is a variable beside jitter, and the test's pause does not sleep — it advances the held clock by the sleep it was handed plus a millisecond of lateness. The deadline boundary is now something the test states. Both run in 0.00s and there is no wall clock left in either. The seam buys the other half too. The interloping 429 that the client test needs — another invocation blocking the shared store while this gate sleeps for a refill of its own — is now written inside the pause, so the order is the test's rather than a goroutine's. That is the case the suppressed review comment named, and it is the one the wall-clock version could not reach at all. take reads the Retry-After against the transaction's own now while it is there, rather than against a second clock reading inside the state, which is what the refill beside it already did. Red again, and on more than before. The server case fails 10 of 10 with main's spent-budget branch restored. The client case fails 10 of 10 with the store-reading first pass restored, on the mid-sleep 429 specifically. 50 of 50 and 3 of 3 under -race with both in place.
jorgemanrubia
added a commit
that referenced
this pull request
Sep 18, 2026
#766) * A budget spent on the server's block no longer blames the client limit When a gated operation runs out of its wait budget, the rejection told the user "Too many requests (client limit 10/s); waited 10s" whatever it had actually been waiting on. The branch that reports a spent budget passed blocked=false unconditionally, so a server Retry-After that the gate slept out until the deadline passed came back named as our own token bucket, with "Re-run, or lower parallelism." underneath it. That is wrong advice, not just an imprecise one. Parallelism is the knob for the client limit; a Retry-After is the server asking everybody to wait, and turning workers down does nothing about it. This is exactly why the CI failure in "Check the clamp, not the clock, in the deadline test" read "client limit 10/s" when the test had blocked on a server Retry-After — the wording was left alone there because it is a call, not a refactor. The rejection now names the limit that held the gate. A budget spent on a server block says so and stops there: Rate limited by the server; waited 10s Re-run. and a budget spent on our own bucket is unchanged. Which one is true is read from the store at the point the error is produced: a Retry-After whose end falls after this gate started queueing covered the wait, whether it is still in force or lifted while being slept out. The block that lifted is the common case — sleeping it out to within a wakeup of the deadline is how the budget usually goes — so a check for a block still in force would miss the failure this came from. A Retry-After that expired before the gate started takes no blame. The block that outlasts the budget outright is untouched: it is still reported immediately as "Rate limited by the server; retry after 30s", which already named the right limit and carries a number worth acting on. Two of the three new tests are red without the fix, both server ones. The client-limit test passes either way by construction — that behaviour is unchanged and was already correct — and it is there so the fix cannot reach past the case it is for. * Observe which limit held the gate, do not read it back from the store The first pass named the limit by loading the store at the moment the error was produced and asking whether the Retry-After it held ended after this gate started queueing. That is a reconstruction, and the store is shared, so it can be wrong in the direction this card is about, only mirrored: another invocation's 429 lands while this gate sleeps for a refill of its own, and a client-side timeout is handed the server's name. A block that expired between the gate's start and its first take does the same. The reader is then told to sit and wait when they should lower parallelism. It also put a locking read on the path after the budget is spent. Store.Load takes the file lock with LockTimeout, 2s, so a contended store could stretch a 10s gate to 12s to choose a message. take already distinguishes the two: blocked is true only for a Retry-After still in force. waitSince now keeps that answer for the sleep it is in, and a spent budget reports the cause of the sleep that reached the deadline. No second read, no reconstruction, and the extra lock wait goes with it. The tests are red on both halves. The server one fails 10 of 10 against main, where a budget spent on a block reports the client limit. The client one fails 10 of 10 against the store-reading first pass, where an expired Retry-After in the shared store takes the blame for a wait the bucket imposed; it passes against main, since main never gets that wrong. Neither test times a sleep. A gate reaches a spent budget only by sleeping to the deadline, so both set the deadline to the instant the thing being waited for ends -- the block lifting, the token refilling -- and pin the jitter to zero. The sleep is then exactly that wait, and a timer that never fires early wakes past the deadline by construction. 20 of 20 each, and 5 of 5 under -race. * Decide when the gate's deadline passes instead of racing it Both new tests raced the clock they were testing. The server one set a Retry-After 20ms out and made that the deadline, with two store writes between the deadline being set and the take that had to observe the block: on a loaded runner the budget is gone before take ever looks, the gate never records a server block, and the test reports the client limit — the very defect it exists to catch, passing itself off as the fix failing. The client one had the same shape with a 100ms window, and both computed the reported wait from a real elapsed time that a slow enough machine rounds up a second. This is the 20ms margin from #763 again, the thing this whole card came out of. A gate reaches a spent budget by waking past its deadline, and no real timer can be asked for that. So the tests no longer ask: the limiter reads a clock it is given, pause is a variable beside jitter, and the test's pause does not sleep — it advances the held clock by the sleep it was handed plus a millisecond of lateness. The deadline boundary is now something the test states. Both run in 0.00s and there is no wall clock left in either. The seam buys the other half too. The interloping 429 that the client test needs — another invocation blocking the shared store while this gate sleeps for a refill of its own — is now written inside the pause, so the order is the test's rather than a goroutine's. That is the case the suppressed review comment named, and it is the one the wall-clock version could not reach at all. take reads the Retry-After against the transaction's own now while it is there, rather than against a second clock reading inside the state, which is what the refill beside it already did. Red again, and on more than before. The server case fails 10 of 10 with main's spent-budget branch restored. The client case fails 10 of 10 with the store-reading first pass restored, on the mid-sleep 429 specifically. 50 of 50 and 3 of 3 under -race with both in place.
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.
TestRateLimiterWaitNeverSleepsPastTheDeadlineput a real 200ms Retry-After block in front of a 220ms deadline, ranWaitagainst it, and timed the result. On an idle machine the sleep lands with 20ms to spare. On a loaded one the timer fires on time and the goroutine is rescheduled late, the budget is gone, andWaitcorrectly reports that the deadline passed — so the test fails on the scheduler with "Too many requests (client limit 10/s); waited 0s", an error that has nothing to do with what it is named after.Originally tracked in A rate-limiter test fails on its 20ms margin, not on what it is named after.
The test is what was wrong here, not the limiter.
waitSincehonours the deadline it was handed, which is its documented job, and nothing promises that a 200ms sleep returns within 20ms of its due time — Go guarantees a timer never fires early and says nothing about late. The 20ms was never a margin; widening it would only move the number the scheduler has to beat.What the test exists to protect is a decision, not a duration: jitter spreads retries by up to half again, and that stretch must not push a sleep past the budget, or a request is rejected unheard after having waited out the very block it was waiting for. That decision is now a named function taking a wait and the time remaining, and the test checks it directly — no timers, no wall clock, four cases including two the old test could not see. Waiting a block out end to end is still covered by
TestRateLimiterWaitOutlastsAShortRetryAfter, which gives a 30ms block a full second and only asserts a lower bound.Reproduced 30 of 30 with 256 busy loops and the test pinned to the same two cores, and again 30 of 30 on a single saturated core; 0 of 30 on both after. With the clamp deleted the new test fails in 0.00s and names the overshoot.
Follows Queue for a resilience slot instead of failing parallel invocations, which introduced the test, and is the sixth of this shape today after #752, #758 and #759.
Two things found while in here and deliberately left alone, both worth their own decision. When the deadline expires,
waitSincealways reports the client limit — which is why this failure's message blames client parallelism for what was a serverRetry-After. AndTestGateQueuesTenParallelWorkersThroughTheDefaultsfailed 3 of 3 on a single saturated core, 13–16s against its 10s budget, though it passed 3 of 3 on two.