Handle EDGAR rate-limit blocks and spread concurrent retries - #319
Merged
Conversation
…t it out
Exceeding EDGAR's 10 req/s answers with a 403 carrying an HTML interstitial at
least as often as with a 429. A 403 maps to HTTP_CLIENT_ERROR, so the block
arrived as a permanent client error: never retried, and — since the cooldown was
signalled only after the retry gate, on a 429 — never pausing the cluster. SEC's
guidance is that requests made during the time-out extend it, so the sweep kept
firing at full rate for the whole window and renewed the block. The failure was
self-sustaining rather than transient, which is why it recurred instead of
passing.
- Re-label the interstitial as the 429 it describes, at the SafeFetch seam. The
status is the only thing the fetch layer carries forward (the error message
reads a {message} out of a JSON body and drops the rest), so an origin that
explains itself in HTML loses its reason before any caller sees it. Narrow on
purpose: sec.gov, 403, and only when the body matches — the other 403 EDGAR
serves is a User-Agent rejection that shares a headline but not the rate
sentence, and must keep failing fast.
- Signal the cluster cooldown before the retry decision. A block landing on a
job's last attempt is the same evidence as any other, and under a sustained
block that is most of them.
- Sleep the applied cooldown instead of the ordinary backoff. The sentinel gates
dispatch and an already-started job never re-consults the limiter, so a <=30s
backoff put every in-flight request back on the wire inside the penalty
window. Jitter spreads the wake-ups so they cannot re-trip the ceiling.
- Raise the default cooldown from 60s to EDGAR's stated 600s. Observed blocks
carry no Retry-After, so it is the only thing sizing the wait, and a minute
resumed the cluster nine minutes early into a live block.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm
…ptured shape A captured EDGAR 429 confirms the interstitial is served under 429 as well as 403, carries no Retry-After, and has an empty reason phrase (HTTP/2 carries none) — and that the ban is IP-wide, refusing an ordinary browser on the same IP mid-sweep. That reorders the diagnosis. The 429 path was already retryable and already armed the cluster cooldown, so it looked handled; but a 60s cooldown against a 600s penalty, plus every in-flight job retrying on its own <=30s backoff inside a live window, was on its own enough to keep renewing the ban. The 403 translation is hardening, not the load-bearing fix. Say so, and note that the absent Retry-After is what makes the cooldown default the only thing sizing the wait. Model the bare-429 regression test on the captured response rather than an invented one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm
…read A concurrency bound is not a rate bound. ConcurrencyLimiter does cap simultaneity — its token is held until the job reaches a terminal state, retries included — but 16 requests are fine spread over two seconds and a violation arriving in one tick, and a retry re-issues from inside the job, downstream of every limiter. A shared upstream failure produces exactly that tick: every in-flight job fails on the same event and computes the same delay, so a transient 5xx re-issued a full fleet inside 500ms (~32/s) and tripped the very block it was recovering from. Spread every retry, not just a blocked one. Two things that read fine and were not: - backoffDelay jittered too, so the delay was a sum of two independent uniforms — which CONCENTRATES toward the middle rather than spreading. Measured at 11/s against an 8/s ceiling. It is now deterministic; retrySpread is the single jitter source, uniform over a known window. - The cap was applied to the sum of wait and spread, which ate the spread whenever the base already sat at the ceiling — the default block case exactly, a 600s cooldown against a 600s cap. So the anti-herd spread introduced in the previous commit did nothing in the case it was written for. Cap the wait, then add the spread. The rate test drives a uniform sweep in place of Math.random so it asserts the spreading formula rather than one draw's luck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm
…the first trip EDGAR throttles the offending requests when a burst clears 10 req/s and escalates to the ~10-minute IP block only if the caller keeps pushing. Treating the first 429 as the ban stopped the CLI for ten minutes over a condition a few seconds of quiet clears — and the budget is per IP, so the overshoot may not even be all ours: an ordinary browser tab on the same address spends from it. Walk a ladder (5s -> 60s -> 600s) instead, concluding we are genuinely banned only once a retry AFTER a completed cooldown is blocked again. Three rungs reach the full penalty in ~65s: each probe costs a round of requests and requests sent during a real ban extend it, so more rungs are gentler on a false alarm and worse on a true one. Two properties the policy needs: - A fleet blocking at once is ONE trip. Every in-flight job reports the same block, so escalating per caller would climb SEC_FETCH_MAX_CONCURRENT rungs on the first overshoot and land on ten minutes immediately — precisely what this removes. A block arriving inside a live cooldown returns the REMAINING time and does not move the ladder. - The quiet period that resets the ladder is anchored on the END of the last cooldown, so waiting out a ban is not counted as the clean run earning a reset. translateEdgarBlockResponse no longer synthesizes Retry-After: 600. That figure describes the ban, not the first overshoot, and stating it handed every first trip a ten-minute wait and bypassed the ladder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm
…it 429 Two defects in the cooldown path, both found by review. The cluster pause could move BACKWARDS. The ladder rung is process state while the sentinel is cluster state, and PostgresRateLimiterStorage writes it with an unconditional `DO UPDATE SET next_available_at = EXCLUDED`, so a second shard meeting the same block at rung 0 replaced another shard's 600s pause with its own 5s one and resumed every shard nine minutes inside a live ban. Read the sentinel first and write the later of the two, adopting it as this job's wait. The read goes to the STORAGE sentinel, never RateLimiter.getNextAvailableTime(): that returns the latest of the rate wall, the sentinel, AND the instance's localBackoffUntilMs — which libs documents as keeping this process's worker from re-acquiring "without polluting cluster state". Feeding the composite back into setNextAvailableTime publishes that local hint cluster-wide, so a 5s first-rung trip taken while local backoff sat at its 60s ceiling paused every shard for a minute. Verified: with the composite read, the first trip returns 60000 instead of 5000. Read-then-write is not atomic across processes, so this narrows a systematic clobber to a rare one. Closing it properly wants GREATEST(...) in the storage upsert, which is a @workglow/postgres change. Separately, a `Retry-After: 0` means "retry now" and is not evidence of a ban: it set cooldownUntil = now, which is already past for the next caller, so a fleet handed one climbed a rung per in-flight job and landed on the top rung from a response asking for no wait at all. It now climbs no rung and arms no window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm
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
2 participants
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.
Summary
Hardens SEC fetch resilience against EDGAR's rate-limit blocks and prevents thundering-herd retry storms. EDGAR serves rate-limit blocks as both 429 and 403 interstitials; the 403 was treated as a permanent error and never retried or signalled the cluster cooldown, leaving the sweep firing at full rate and renewing the block. Additionally, concurrent retries were re-issued in lockstep, causing transient failures to spike the request rate and trigger the very block they were recovering from.
Key Changes
EDGAR 403 interstitial translation (
edgarBlockResponse.ts): Translates EDGAR's rate-limit 403 interstitial to 429 at the transport seam so it flows through existing retry/cooldown logic. Distinguishes it from the other 403 EDGAR serves (User-Agent misconfiguration) by matching the body text, ensuring the misconfiguration still fails fast.Escalating cooldown ladder (
secFetchThrottle.ts): Replaces the flat 60s default with a three-rung ladder (5s → 60s → 600s) that probes whether a block is genuine before applying the full penalty. A first overshoot costs seconds rather than ten minutes, and the ladder only escalates when a retry after a completed cooldown is blocked again. Coalesces simultaneous blocks from a fleet into one trip.Retry spread (
SecFetchJob.ts): Adds uniform spread over two drain windows (ceil(maxConcurrent / maxPerSec) * 2s) to every retry, not just blocked ones. Removes jitter frombackoffDelayitself (which was concentrating rather than spreading when combined with the spread), making backoff deterministic and leaving de-synchronization to the spread alone.Cooldown signalling before retry decision: Signals the cluster cooldown before deciding whether to retry the job, so blocks arriving on a job's last attempt still pause the cluster. Blocked jobs sleep the applied cooldown rather than the ordinary backoff, since the cluster sentinel gates dispatch and this job is already dispatched.
Test coverage: Comprehensive test suite covering rate-limit detection, escalation ladder behavior, fleet coalescing, quiet-period reset, and retry spreading across concurrent failures.
Notable Implementation Details
translateEdgarBlockResponsedeliberately synthesizes noRetry-Afterheader; EDGAR's ten-minute figure describes the ban it escalates to, not the first overshoot, so stating it would bypass the ladder entirely.https://claude.ai/code/session_01FL82qT6KLmfFP6vM2MqKgm