20260816 - Freeze the mock's limiter clock so a rate-limit test stops rolling dice - #15
Merged
Merged
Conversation
… rolling dice CI went red on the merge of #14 with `assert 202 == 429`, in a test of the mock's detection rate limit. #14 changed the registration read timeout and touched nothing near the limiter: its own PR run was green, and the merge commit's tree is byte-identical to the branch head's. Same source, opposite result, three minutes apart. The limiter's windows are aligned on the clock — `(floor(now / w) + 1) * w` — rather than sliding from first use, which is faithful to the server and is documented on `_FixedWindowCounters`. The consequence is that a burst crossing a second boundary is handed a fresh allowance part way through. The test fired nine detections and asserted the ninth was refused, which only holds if all nine land inside one window. So the assertion was really about scheduling. Idle, the burst takes 8.3 ms, so about 0.8% of start phases straddle a boundary; with the machine's sixteen cores saturated it stretches to 39.7 ms and about 4%. The CI runner was slow enough that the suite crawled — 26 s for a block that takes 1.4 s here — and it lost the roll. Nothing was wrong with the code under test, on that run or any of the ones that passed. Both limiters already took a `clock`, because the real server's own tests freeze theirs; the mock just gave no way to reach it. `State` now carries the clock and builds the limiters from it, and `MockServer` passes one through. The two tests that assert on an allowance run on a frozen one, where all nine requests read an identical `now` by construction rather than by being quick. The straddle itself is now pinned by a test that advances the clock across a boundary deliberately. It was real behaviour worth having, and it was only ever reachable before by a runner slow enough to blunder into it. Verified: the two tests 40x under full load, 0 failures, and every gate green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
mainis red from the merge of #14, onassert 202 == 429in a test of the mock's detection rate limit. It is a flaky test, not a regression.#14 changed the registration read timeout and touched nothing near the limiter. Its own PR run was green, and the merge commit's tree is byte-identical to the branch head's —
2ab4608fboth. Same source, opposite result, three minutes apart.Why it flakes
The limiter's windows are aligned on the clock,
(floor(now / w) + 1) * w, rather than sliding from first use. That is faithful to the server and already documented on_FixedWindowCounters, and it means a burst crossing a second boundary gets a fresh allowance part way through. The test fired nine detections and asserted the ninth was refused — true only if all nine land inside one window.So the assertion was about scheduling as much as about the limiter:
The CI runner was slow enough that the suite crawled — 26 s for a block that takes 1.4 s locally — and it lost the roll.
The fix
Both limiters already took a
clock, because the real server's own tests freeze theirs; the mock gave no way to reach it.Statenow carries the clock and builds the limiters from it, andMockServerpasses one through. The two tests that assert on an allowance use a frozen one, where all nine requests read an identicalnowby construction rather than by being quick enough.test_the_heartbeat_keeps_its_own_allowancecarried the same hazard — same 8+1 burst, same 1 s window — and is fixed with it.The straddle behaviour is now pinned by a test that advances the clock across a boundary deliberately, rather than being reachable only by a runner slow enough to blunder into it.
Verification
tools/check.sh --tracked: all gates green.🤖 Generated with Claude Code