Two gate tests were failing on the clock, not on the gate - #774
Merged
Merged
Conversation
Two tests in internal/resilience were failing on the clock rather than on anything they are named after, and between them they blocked an unrelated pull request's Race Detection run. TestGateQueuesTenParallelWorkersThroughTheDefaults gave eighty child process invocations a budget of DefaultMaxWait, which is one operation's queueing budget and no statement about a run of eighty. The run is bounded below by three seconds of deliberate token refill and above by nothing but the box, so on a contended one it spent the margin on the machine: 3 of 3 red on a saturated core, and red on CI at 10.73s and again at 10.97s with the gate having behaved perfectly every time. The queueing claim never needed a stopwatch. Eighty calls against a fifty-token bucket is the claim: thirty of them cannot be served from the starting bucket at all, so "every call succeeded" is only reachable by waiting for refills. That is now asserted against the config rather than written out as literals, and the wall-clock bound is gone. Its sibling TestGateQueuesOversubscribedInvocationsWithinTheSlotLimit had the same upper bound and loses it too; its lower bound, that the overflow waited for a second round, is a real claim and stays. TestRateLimiterRetryAfterOnlyUpdatesIfLater set a 200ms block, set a 50ms one, and asserted 150ms still remained — an assertion that fewer than fifty milliseconds of real time passed between two statements. It now asserts the stored deadline itself, in both directions: a nearer deadline does not shorten the block, a further one does extend it, so neither half can pass for free. Evidence: under a saturated core with race instrumentation the old gate test failed 10.120778248s > 10s with 80 ok, 0 rejected and peak 10; the new one passes 3 of 3 under the same load at 10.06-10.14s. Mutating the rate limiter to stop queueing for refills still reds it (51 ok, 29 rejected). Mutating SetRetryAfter to accept any deadline reds the retry-after test, and a 60ms stall injected between the two statements reds the old assertion while the new one holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The gate tests can still pass without exercising either queueing path because process startup and token refills are not deterministically controlled.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Stabilizes resilience tests by replacing scheduler-sensitive timing assertions with state-based checks.
Changes:
- Verifies retry-after deadlines via persisted state.
- Removes unreliable upper time bounds from gate tests.
[!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 | Description |
|---|---|
internal/resilience/rate_limiter_test.go |
Tests retry-after ordering using stored deadlines. |
internal/resilience/gate_test.go |
Revises timing assertions in multi-process gate tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Dropping the wall-clock bound left both multi-process gate tests able to pass without the gate ever having queued anything, which is the same defect wearing the other face: the assertion runs over nothing and is green either way. The cause is that the children raced each other into the gate, so how hard the gate was pushed was decided by how fast the box could fork. On a slow one the ten workers arrive spread out, the bucket refills between them, and every call is served from a full bucket; in the oversubscribed test the first ten invocations finish before the last five start, and nothing is oversubscribed. Both would then pass with a gate that failed fast instead of queueing. The children now come off a barrier. Each announces itself and holds before its first operation, and nobody is released until the last one has arrived, so the load the gate sees is the load the test asked for. Where the eighty calls used to be eighty process spawns paced by the machine, they are now ten already-running processes making eight each. Queueing is then observed rather than inferred. Each child reads the bucket before it gates and reports how many of its operations were admitted out of an empty one, which is only reachable by waiting for a refill — a gate that failed fast would have rejected that same operation. The count is a floor, not an accounting, since a call that reads the bucket just after a refill lands is served without waiting; what it rules out is a run where the wait path was never entered. The oversubscribed test asserts the slot table filled to exactly MaxConcurrent, which says the overflow waited, where the old evidence was that the run took two holds — something slow spawning satisfies on its own. Evidence, all on the saturated single core with race instrumentation that made the old test fail 10.12s > 10s: 3 of 3 green as written, and 3 of 3 red with the rate limiter mutated to stop queueing for refills and the bulkhead mutated to stop queueing for slots. Removing the concurrency limit altogether puts the peak at 15 against 10. Off the barrier, the oversubscription is now exact: the fail-fast bulkhead rejects precisely five of fifteen. The package is also 4 seconds faster under -race, 19.4s against 23.7s, from the seventy process spawns that are no longer needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
🤖 Rebuilt on the finding above: the children now come off a barrier and the queueing is observed rather than inferred. Re-requesting a review on the new head. |
The barriered runner drives the children through pipes rather than running each to completion with its output in hand, and in the move the children's stderr stopped being kept. A child that panics now fails the parent with "exit status 2" and nothing else, where before the whole transcript came back with the error. Its stderr is buffered again and handed to the failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 19, 2026 10:23
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The queue signal can report waiting when the gate actually succeeded immediately, leaving the regression test potentially vacuous.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
The queue signal added in the last commit was inferred from outside: the child read the bucket just before gating and counted the calls that found it empty. A refill landing between the look and the take makes that a wait that never happened, so the witness could report queueing where the gate had returned immediately — the assertion still running over nothing, now with a witness that can lie about it. The rate limiter and the bulkhead each get an onWait seam, called at the point where the caller is actually turned away and settles in to sleep. The clock is already injected here for the same kind of reason; this is the same seam for the same kind of question. Nothing in production sets either one. A counter that always fires would satisfy the queueing test just as well as an honest one, so the witness now has to say no as well as yes: against a bucket nobody can exhaust and a slot for every caller, the same eighty calls go through and both counters must read zero. With that in place the oversubscribed test can say the thing it is named after directly — five of the fifteen found every slot taken and polled for one, not four and not six. The queueing count is thirty calls the starting bucket cannot cover, less the odd one that arrives in the instant after a refill lands and is served without waiting: 29 idle, 28 on a saturated core, over eight runs. One free pickup per worker is the allowance. Evidence, all mutations against the committed tests: a witness rewired to fire on every call, including the ones served immediately, fails the new test at 80 where it wants 0. A rate limiter that stops queueing for refills fails the queueing test at 0 where it wants 20 or more. A bulkhead that stops queueing for slots fails the oversubscribed test with exactly five rejections out of fifteen. All three gate tests pass 3 of 3 on the saturated single core with race instrumentation that made the original fail 10.12s > 10s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc comment said thirty calls sleep for a refill and the assertion two paragraphs down explains that it is 29 idle and 28 on a saturated core. Say roughly, and leave the exact figures where they are measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 19, 2026 10:50
jorgemanrubia
added a commit
that referenced
this pull request
Sep 19, 2026
* Wait for the oversubscription instead of hoping for it TestGateQueuesOversubscribedInvocationsWithinTheSlotLimit asserted that five of fifteen callers found every slot taken. CI got one. The assertion was mine, from #774, and it was wrong in the way that card was about: it described a race and called it a fact. A barrier at the start lines up the arrivals, which is what the last change fixed. It does not keep them together afterwards. A child released and then descheduled arrives after an earlier holder has already finished and takes the slot it freed, so it never waits — and on a two-core runner under fifteen processes that gap is wide. So the parent now holds the slot table full. Each child announces the first time the bulkhead turns it away, and announces holding a slot and then waits before releasing it; the parent blocks until ten are held and five have been refused, and only then lets the holders go. The oversubscription is something the test waited for rather than something it hoped for, and five is exact. It waits rather than polls on purpose. A bulkhead that stopped excluding anyone cannot make this test pass with a smaller number — it hangs and fails on the test's own deadline, loudly, instead of reporting one and looking like a flake. Evidence. Spreading the arrivals 50ms apart, which is what a loaded runner does to fifteen forks, fails the old version with peak 4 against 10 and slotWaits 0 against 5 — the CI symptom, reproduced deliberately — and leaves the new one passing, four times out of four. Removing the concurrency limit entirely makes the new test hang and fail on its deadline rather than pass. Five runs on a saturated core under race instrumentation pass. Main has had one run since #774 merged and it was green, so this was a latent flake rather than a broken main; it surfaced on a pull request that had not touched this package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * The slot count the parent waits on is the one the children run under runSlotOversubscription took a slots argument and did not use it for anything but its own arithmetic: the children went on running under DefaultConfig's ten. Any other value left the parent waiting for a split that cannot happen. That matters more here than an ignored parameter usually would. This helper waits for the oversubscription rather than sampling it, which is the point of the change — but it means a mismatch no longer shows up as a wrong number. It shows up as the test deadline, with a message about a timeout and nothing about the cause. Confirmed before fixing: eight callers against five slots hung for the full forty-five seconds. The count now goes to the children through BH_MAX_CONCURRENT, beside BH_MAX_TOKENS, and the helper refuses a caller environment that would override it. Its own test, because the failure mode is a deadline rather than an assertion: five slots for eight callers, five hold, three are refused. Removing the wiring hangs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <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.
Race Detection went red on A refusal could be thrown away while an earlier one was being written with two failures, neither of them in that pull request's diff. Both were tests in
internal/resiliencethat read a wall clock and so reported on the box they ran on rather than on the behavior they are named after. An unrelated change was blocked by a job nobody trusts, which is the more expensive half of the problem: a genuine data race would arrive in the same red tick and be read as more of the same noise.Originally tracked in A gate test spends its 10-second budget on the machine, not on the gate.
The gate test spawns child processes and gave the whole run a budget of
DefaultMaxWait— one operation's queueing budget, and no statement at all about a run of eighty calls. The run is bounded below by three seconds of deliberate token refill and above by nothing but the machine, so the margin gets spent on the machine: red 3 of 3 on a saturated core here, and red on CI at 10.73s and again at 10.97s with every assertion about the gate itself passing.The retry-after test set a 200ms block, then a 50ms one, then asserted 150ms still remained — an assertion that fewer than fifty milliseconds of real time passed between two statements, safe on an idle box and not on a two-core runner under race instrumentation. It now asserts the stored deadline, in both directions, so neither "the nearer deadline was ignored" nor "the further one was taken" can pass for free.
Taking the machine out of the gate tests
Dropping a wall-clock bound is the easy half. The harder half, and what two rounds of review were about, is that a test freed of the clock can be green over nothing at all.
Both multi-process tests let their children race each other into the gate, which means how hard the gate got pushed was decided by how fast the box could fork. On a slow one the workers arrive spread out, the bucket refills between them, and the gate is never asked to queue; in the oversubscribed test the first ten invocations finish before the last five start, and nothing is oversubscribed. So the children now come off a barrier — each announces itself and holds, and nobody is released until the last one has arrived.
And the queueing is now reported by the gate rather than inferred around it. The rate limiter and the bulkhead each carry an
onWaitseam, called where the caller is actually turned away and settles in to sleep; the rate limiter already carries an injectedclockfor the same kind of reason. Reading the bucket from outside just before the call does not work — a refill landing between the look and the take is a wait that never happened, and the witness would say it did.A counter that always fires would satisfy the queueing assertion just as well as an honest one, so the witness has to be able to say no: against a bucket nobody can exhaust and a slot for every caller, the same eighty calls go through and both counters must read zero.
Evidence
Under a saturated core with race instrumentation the old gate test failed
10.120778248s is not less than 10swith 80 ok, 0 rejected and peak 10 — the gate was faultless. All three gate tests now pass 3 of 3 under that same load.Nothing here passes over an empty set. Mutations, each against the committed tests:
SetRetryAfteraccepting any deadline: the retry-after test redsSide effect worth naming: the package is around 4 seconds faster under
-race, 19.4s against 23.7s, from the seventy process spawns that are no longer needed.Companion to A rate-limiter test spends its 20ms margin on the scheduler, which fixed the same shape in the clamp. Sharding the race job would have made all of this arrive faster and fixed none of it.