A test asserted five of fifteen waited, and CI got one - #776
Merged
Merged
Conversation
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>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new helper’s slots argument does not configure child bulkheads, causing non-default usages to hang.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Stabilizes the oversubscription gate test by holding acquired slots until contention is observed.
Changes:
- Adds explicit
WAITINGandHELDsynchronization. - Replaces timing-dependent contention with deterministic coordination.
[!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/gate_test.go |
Adds deterministic bulkhead oversubscription coordination. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
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.
TestGateQueuesOversubscribedInvocationsWithinTheSlotLimitwent red on a pull request that had not touchedinternal/resilience: it expects five of fifteen callers to find every slot taken, and got one. The assertion is mine, from Two gate tests were failing on the clock, not on the gate, and it is wrong in exactly the way that card was about — it described a race and called it a fact.Follows up A gate test spends its 10-second budget on the machine, not on the gate.
Why a barrier was not enough
#774 replaced a wall-clock bound with a barrier so the children arrive together instead of as fast as the box can fork. That fixed the start. It does not keep them together afterwards: a child released and then descheduled arrives after an earlier holder has finished, takes the slot it freed, and never waits. On a two-core runner with fifteen processes that gap is wide, and the count drops.
The witness was doing its job, incidentally — it refused to say five when five had not queued. What was wrong was the expectation, not the instrument.
Holding the table full
The parent now keeps the slot table saturated until the contention has actually happened. Each child announces the first time the bulkhead turns it away, and announces when it holds 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.
That makes the oversubscription something the test waited for rather than something it hoped for, so five is exact rather than likely. 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, reproduces the failure deliberately:
slotWaits0 against 5 — the CI symptom and worseMain
Main has had one Test run since #774 merged and it was green, so this is a latent flake rather than a broken main. It is on its own branch rather than inside Nothing started the connector again after a crash, and no skill could start it at all, whose Tests check it is currently blocking.