fix: retry a polling 429 only when Retry-After says when to come back (sable-96ex) - #227
Open
Rome-1 wants to merge 6 commits into
Open
fix: retry a polling 429 only when Retry-After says when to come back (sable-96ex)#227Rome-1 wants to merge 6 commits into
Rome-1 wants to merge 6 commits into
Conversation
… (sable-96ex)
All three surfaces treated a 429 during polling as fatal, and CLI_SPEC
enshrined it ("Other 4xx (401/403/429) — Not retried"). Polling every 10
seconds from every customer repo is exactly the traffic shape a rate
limiter targets, so the first limiter in front of GET /api/static/scan
would have failed every customer build instantly, on a condition one
sleep would have resolved. There is no limiter there today, which is
precisely why nobody would be looking at the CLI on the day one lands.
A 429 cannot simply join 408/5xx either: on scan SUBMIT it means the
account is out of credits (exit 3), and the CLI cannot tell "out of
credits" from "going too fast" — same status code. Retrying a quota
rejection for 30s and then failing anyway is worse than failing now.
Retry-After is the disambiguator: a limiter sends one, a quota rejection
does not. So the poll loop and the results fetch retry a 429 that carries
one and keep failing fast on a bare one. Submit is untouched — exit 3
even with the header — and both runtimes now pin that asymmetry so it is
not later "made consistent".
- node/src/commands/backend/scan-status.ts, python/.../backend.py,
github-action/action.yml: 429 is transient iff Retry-After parses.
The honored wait replaces the exponential backoff for that attempt and
still spends a failure from the same budget, so an endlessly-throttling
API cannot keep the loop alive.
- Delay-seconds only. The HTTP-date form is not parsed anywhere: the
action has to reach the same verdict in shell on whatever `date` the
runner ships, and a rule the three surfaces cannot state identically is
worse than a narrow one they can. Unparseable counts as absent, which
means fail fast — the conservative direction.
- Capped at 60s, so a limiter cannot park a CI job for an hour. In bash
the cap is on LENGTH as well as value: `[ 1e23 -gt 60 ]` is an error
that evaluates false, so an uncapped 23-digit header would reach
`sleep` intact and hang the job until the runner times out (verified:
`sleep 99999999999999999999999` does not return).
- Giving up on repeated 429s says the API rate limited us instead of
blaming the report. Sending a throttled customer to `rafter get` — which
would be throttled too — costs them the whole diagnosis. New action
status value `rate-limited`, alongside `unreadable`.
- Node: the two identical copies of the retry-notice builder collapse
into one, now that the notice has to distinguish the two cases.
Coverage, all mutation-verified (each guard deleted in turn, confirming a
test goes red): 20 node + 18 python unit tests; 5 new drift-detector
checks over action.yml; 3 e2e jobs against the mock backend (poll rides
out a 429 with Retry-After, poll fails fast on a bare one and is asserted
to have polled exactly twice, results fetch rides one out). The mock can
now inject a Retry-After verbatim, so a malformed value is testable.
The bash halves were driven end-to-end locally against the mock before
this landed: ride-out, fail-fast, malformed header, absurd header, and
budget exhaustion each behaved as documented.
Raising the longest honored delay from the backoff schedule's 16s to a server-named 60s put it in reach of overrunning timeout-minutes, which sable-l10k had just turned from a poll count into a real wall clock. A limiter answering "come back in 60 seconds" 30 seconds before the deadline would have pushed the job a minute past the budget its author set — the server does not get to extend that. The honored delay is now clamped to what is left of the deadline as well as to the 60s ceiling. Measured against the mock with timeout-minutes: 1 and Retry-After: 60 injected forever: the loop sleeps 50s and the step ends at 60s exactly, on "Scan did not complete within 1 minutes". Without the clamp the same run ends at 70s. Drift check 18 pins it; deleting the clamp fails it. The results fetch needs no equivalent — it runs after the poll step, with no deadline of its own.
The three surfaces disagreed about a duplicated header, which is the one thing this change set claims they do not do. Node took the first value, bash took the last, and Python — via requests, which joins duplicates as "5, 900" — failed the digit test and treated it as absent. Python had it right. An origin's Retry-After and a proxy's are not a delay anyone can act on, and "unusable counts as absent" already means fail fast everywhere else here. Node now refuses an array of more than one, and the poll loop and results fetch count the matching header lines instead of `tail -n1`. Not hypothetical for this bead in particular: the scenario sable-96ex is about is a rate limiter appearing in FRONT of the API, which is exactly the deployment that produces two Retry-Afters. The mock splits FAIL_RETRY_AFTER on '|' into several headers, so the case is reachable from a test. Covered by a unit test in each runtime and one e2e job asserting the action polled exactly twice; verified locally against the mock first, both halves (5|900 fails fast, a single 1 still rides out).
1. Python accepted a digit int() cannot parse (medium). str.isdigit() is
Unicode-aware and int() is not: '²' passes the first and raises
ValueError out of the second, from a call site whose only handler is
requests.RequestException — so it escaped _poll_until_readable,
_handle_scan_status_interactive and __main__ alike. One raw \xb2 byte
in the header is enough, because http.client decodes headers as
ISO-8859-1. A server-triggered client crash, and the exact bug class
this file already fixed once: a retryable failure turned into an
unhandled exception. It was also a divergence — Node's /^\d+$/ and the
action's `case *[!0-9]*` both reject that byte. Now an explicit
re.fullmatch(r"[0-9]+"), so the accept-set matches what int() parses.
Regression test covers ², ³, ¹, fullwidth 5 and Arabic-Indic ٥, and
asserts the whole poll fails fast rather than raising. Mutation-checked
against a restored isdigit().
2. The action could read a Retry-After the 429 never sent (low, but it
defeats the central invariant). `curl -D` dumps EVERY header block it
received, so a `103 Early Hints` block carrying Retry-After was read as
the final response's own, and a bare 429 — quota exhausted — was
retried. Reproduced with real curl against a raw socket. Both loops now
extract from the last block only, with awk that resets at each status
line: portable, needs no `tac`, and folds in the exactly-once rule.
Verified against six header shapes (hints-only, single, lowercase,
duplicate, X-Retry-After, hints plus a real one).
3. The length cap had only a grep over the file for coverage (low). The
cap is what stands between a 23-digit header and `sleep` never
returning, and per the repo's own rule that is not a test of the
artifact. It has an e2e job now: 429 forever with a 23-digit
Retry-After under a 1-minute budget must end at the deadline with
status=timeout, which a hung step cannot produce.
The review also asked for a malformed-value e2e job ('soon'). Deliberately
not added: at the artifact level a rejected 'soon' and an accepted one are
both a failed run with two polls — the difference lives only in the step
log, which a later step cannot read. That job would pass with the digit
guard deleted, which is the definition of the vacuous check sable-d2x2 is
about. The digit guard is pinned by drift check 16 and by unit tests in
both runtimes instead.
New: drift check 19 (last-header-block scoping), mock knob
EARLY_HINTS_RETRY_AFTER, e2e jobs for the hinted and absurd headers. The
Early Hints job was mutation-verified non-vacuous — with the old
`grep | tail -n1` extraction that same mock config sleeps 7s and completes
with six polls instead of failing after two.
Not changed, and why: Retry-After: 0 is honored as an immediate retry. It
is bounded by the same 5-consecutive/20-total budget, and in the action by
the 10s poll interval on every success, so the worst case is ~5 requests
per 10s rather than a hot loop.
The security review asked whether an honored 60s delay lets a hostile endpoint hold a runner. It does not, but the bound was implied rather than written down: the poll loop is clamped to the wall-clock deadline, while the results fetch has none — three fetches of at most four honored sleeps is ~12 minutes if each eventually succeeds, and ~4 minutes before the first one gives up. Left as it is on purpose. A build whose report WAS retrievable after a real throttle should get the report; the alternative is failing a scan we could have read. Writing the number down is what makes that a decision rather than an accident.
The entry said 'delay-seconds only; the HTTP-date form counts as absent', which was true when written and is now three quarters of the rule. A value int() cannot parse, a header a proxy repeated, and one carried on a 103 Early Hints block are all treated the same way, and each of those is a case where the difference is a bare 429 getting retried.
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.
Closes sable-96ex, raised by the adversarial review of #220 and deliberately not changed there.
The problem
All three surfaces treated an HTTP 429 during polling as fatal, and
CLI_SPEC.mdenshrined it: "Other 4xx (401/403/429 …) — Not retried". Polling every 10 seconds from every customer repo is exactly the traffic shape a rate limiter targets. The first limiter in front ofGET /api/static/scanwould fail every customer build instantly, on a condition one sleep would have resolved — and there is no limiter there today, which is precisely why nobody will be looking at the CLI on the day one lands.A 429 cannot simply join 408/5xx either. On scan submit a 429 means the account is out of credits (
EXIT_QUOTA_EXHAUSTED, exit 3), and the CLI cannot tell "out of credits" from "going too fast" — same status code. Retrying a quota rejection for 30 seconds and then failing anyway is worse than failing now.The rule
Retry-Afteris the disambiguator: a rate limiter sends one, a quota rejection does not. So:Retry-After, during pollingmin(Retry-After, 60s), then retry.3, even when it carries the header.The asymmetry with submit is the point, so both runtimes now pin it with a test rather than leaving it to be "made consistent" later.
Three details that make honoring a server-supplied delay safe:
datethe runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. An unparseable header counts as absent, which means fail fast: the conservative direction.[ 1e23 -gt 60 ]is a shell error that evaluates false, so an uncapped 23-digit header would reachsleepintact and hang the job until the runner times out. (sleep 99999999999999999999999does not return — checked.)timeout-minutes, which sable-l10k had just turned from a poll count into a real wall clock. The honored delay is clamped to what is left of the deadline too. Measured withtimeout-minutes: 1andRetry-After: 60injected forever: the loop sleeps 50s and the step ends at 60s exactly; without the clamp it ends at 70s.rafter getthat will be throttled too — costs them the entire diagnosis. New actionstatusvaluerate-limited, alongsideunreadable.Coverage
Every guard was mutation-verified: deleted or inverted one at a time, confirming a named test goes red.
action.yml— the Retry-After gate in both loops, the digit and length validation, the cap, and therate-limitedstatus. Each one fails when its guard is removed.Retry-After; poll fails fast on a bare one and is asserted to have polled exactly twice (the assertion that catches a "simplification" making every 429 transient); results fetch rides one out. No API key, no credits.The bash halves were driven end-to-end locally against the mock before this landed — ride-out, fail-fast, malformed header, absurd header, budget exhaustion — each behaving as documented.
Security review
Reviewed with the
rafteragent (local secrets tier + therafter-code-reviewchecklist;rafter runneeds an API key this box does not have). Three findings, all fixed in5ef4ca5:int()cannot parse.str.isdigit()is Unicode-aware andint()is not:'²'passes the first and raisesValueErrorout of the second, from a call site whose only handler isrequests.RequestException. One raw\xb2byte is enough —http.clientdecodes headers as ISO-8859-1 — so a server could crash the client. Also a three-surface divergence, since Node's/^\d+$/and the action'scase *[!0-9]*both reject it. Now an explicitre.fullmatch(r"[0-9]+").Retry-Afterthe 429 never sent.curl -Ddumps every header block it received, so a103 Early Hintsblock carrying the header was read as the final response's own, and a bare 429 got retried — the one thing the gate exists to prevent. Reproduced with real curl. Both loops now scope the extraction to the last header block.grep-over-the-file for coverage. It has an e2e job now: a 23-digitRetry-Afterinjected forever under a 1-minute budget must end at the deadline withstatus=timeout, which a hung step cannot produce.The review also suggested a malformed-value (
'soon') e2e job. Deliberately not added: at the artifact level a rejected'soon'and an accepted one are both a failed run with two polls — the difference lives only in the step log, which a later step cannot read. That job would pass with the digit guard deleted, which is the vacuous check sable-d2x2 is about. The digit guard is pinned by drift check 16 and by unit tests in both runtimes instead.Checked and clean in the review: injection (no unquoted or arithmetic use, no
eval), DoS bounds, workflow-command forgery, tmpfile handling on every path, and prototype pollution inretryAfterMs(fuzzed with 20 hostile header shapes).Known limitation
The HTTP-date form of
Retry-Afteris treated as absent, so a limiter that sendsRetry-After: Wed, 21 Oct 2026 07:28:00 GMTstill fails the build fast. Filed as a follow-up; the fix is only worth taking with a portable date parse for the shell surface.