fix(relay): anti-enum tests blew their budget on harness overhead, not a hang - #13
Open
angusbezzina wants to merge 1 commit into
Open
fix(relay): anti-enum tests blew their budget on harness overhead, not a hang#13angusbezzina wants to merge 1 commit into
angusbezzina wants to merge 1 commit into
Conversation
…t a hang The two anti-enumeration cases had been failing 'Relay Tests (Ubuntu)' on almost every run since 2026-07-27. attn-i38g recorded them as hanging only under GitHub Actions, on the grounds that they finish in ~350ms locally, and told us not to touch the budgets until the hang was explained: if the limiter can stall a request path, a longer timeout would hide a production bug. There is no hang, and nothing is Actions-specific. The ~350ms figure came from running the file on its own. Run the whole integration suite and the same two cases take 19,660ms and 9,216ms on macOS. The cost is the SELF.fetch round trip, not anything under test. An unknown-room GET costs 1.6ms when this file runs alone and ~250ms when the suite shares the workerd isolate. A GET /health -- no Durable Object, no quota, no rate limiter on that path -- costs ~217ms under the same load, the same price. So it scales with sequential calls times suite activity, and these four cases make the most sequential calls in the repo: the cap (antiEnumPerFiveMin=30) can only be exercised by 30+ distinct probes. They were simply first over the line, which is also why they were intermittent rather than uniformly broken. That clears the concern the bead raised. The limiter does not stall anything; a no-op route is equally slow under the same conditions. Ruled out by measurement: file parallelism (--no-file-parallelism changed nothing), accumulated Durable Objects (500 preloaded DOs left the tests fast), a single bad neighbour file (only the full integration set reproduces), and concurrency as a fix (Promise.all over 20 probes measured 1.0x against the sequential loop, loaded and idle -- the pool serializes them regardless). So the only lever is fewer sequential calls. The 'existing rooms' known-room loop drops from 50 hits to 5: it was ~11s of that case's ~19s, and the property under test is a per-request branch that holds on the first hit or not at all, so five guards against an accumulator as well as fifty did. Worst case under full-integration load falls 19,660ms -> 8,967ms. All four cases then get an explicit 30s budget. Every one of them is structurally 30+ sequential calls and was marginal at the 15s default, so the two that still passed were next. Raising the budget here is the measured response to harness overhead, documented in the block comment, not a way to hide a suspected stall. Making antiEnumPerFiveMin configurable would be the real speed fix -- 30 probes becomes ~6 -- but the limiter is a module-level singleton shared across the isolate, and files other than this one still share the "unknown" IP bucket, so a smaller global cap would make unrelated cases 429. Left alone deliberately. Refs: attn-i38g Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Fixes the
Relay Tests (Ubuntu)job, which has failed on almost every run since 2026-07-27 on the same anti-enumeration cases inrelay/test/integration/rate-limit.test.ts.The recorded diagnosis was wrong
attn-i38ghad these down as hanging only under GitHub Actions, on the grounds that they finish in ~350ms locally, and explicitly said not to raise the budgets until the hang was explained — if the limiter can stall a request path, a longer timeout would hide a production bug. That was the right instinct, so the first job was to test it.There is no hang, and nothing is Actions-specific. The ~350ms figure came from running the file on its own. Run the whole integration suite and the same two cases take 19,660ms and 9,216ms on macOS.
The cost is the
SELF.fetchround trip, not anything under test:/health— no DO, no quota, no rate limiterA no-op route is the same price as a limiter-exercising one, so the overhead is per-round-trip in the shared workerd isolate and scales with (sequential calls) x (suite activity). These four cases make the most sequential calls in the repo, because the cap (
antiEnumPerFiveMin = 30) can only be exercised by 30+ distinct probes. They were simply first over the line — which is also why they were intermittent rather than uniformly broken.This clears the production concern. The limiter does not stall anything.
Ruled out by measurement
--no-file-parallelismchanged nothing (19,214ms).Promise.allover 20 probes measured 1.0x against the sequential loop, both loaded and idle. The pool serializesSELF.fetchregardless. This killed the obvious fix before it shipped.The change
Only lever left is fewer sequential calls.
existing roomsknown-room loop drops 50 hits to 5. It was ~11s of that case's ~19s (50 round trips plus 50 admission-header signings), and the property under test — a known room never callsrecordUnknownRoom— is a per-request branch that holds on the first hit or not at all. Five guards against an accumulator as well as fifty did.Results
Full relay suite: 474/474 passing in 206.79s.
tsc --noEmitclean.~3.4x headroom against the budget on the worst case.
Deliberately not done
Making
antiEnumPerFiveMinconfigurable would be the real speed fix (30 probes becomes ~6), but the limiter is a module-level singleton shared across the isolate, and test files other than this one still share the"unknown"IP bucket — a smaller global cap would make unrelated cases 429. Noted in the code and on the bead.Refs: attn-i38g
🤖 Generated with Claude Code