From 6da25dad10c3e32fa44bc1f42b58539a6e452241 Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:20:15 -0700 Subject: [PATCH 1/6] fix: retry a polling 429 only when Retry-After says when to come back (sable-96ex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/test-github-action.yml | 139 +++++++++ CHANGELOG.md | 1 + github-action/action.yml | 105 ++++++- github-action/tests/mock-rafter-api.py | 25 +- .../tests/test-action-yml-defaults.sh | 64 ++++ node/src/commands/backend/scan-status.ts | 153 +++++++-- node/tests/scan-poll-429-retry-after.test.ts | 292 ++++++++++++++++++ node/tests/scan-remote.test.ts | 28 ++ python/rafter_cli/commands/backend.py | 98 +++++- .../tests/test_scan_poll_429_retry_after.py | 210 +++++++++++++ python/tests/test_scan_remote.py | 27 ++ shared-docs/CLI_SPEC.md | 15 +- 12 files changed, 1102 insertions(+), 55 deletions(-) create mode 100644 node/tests/scan-poll-429-retry-after.test.ts create mode 100644 python/tests/test_scan_poll_429_retry_after.py diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 38875523..67a30836 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -227,6 +227,145 @@ jobs: fi echo "PASS: the results fetch retried and completed." + # sable-96ex — the day a rate limiter lands in front of the poll endpoint, + # polling every 10s from every customer repo is exactly the traffic it + # targets. A 429 is retried on one condition only: the server sent a + # Retry-After saying when to come back. Without it a 429 on this API means + # quota exhausted, and sleeping through five retries before failing anyway is + # worse than failing now. These three jobs pin both halves. + test-poll-429-with-retry-after: + name: "Poll: rides out a 429 that carries Retry-After" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (429 + Retry-After on poll #2, then healthy) + env: + PORT: '8791' + FAIL_ON: '2' + FAIL_STATUS: '429' + FAIL_RETRY_AFTER: '1' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8791/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8791/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8791' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the scan survived the throttle + run: | + cat mock.log + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: a 429 that told us when to come back killed the run (status='${{ steps.scan.outputs.status }}')." + exit 1 + fi + echo "PASS: the action honored Retry-After and completed." + + test-poll-429-without-retry-after: + name: "Poll: a 429 with no Retry-After fails fast" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (bare 429 on poll #2) + env: + PORT: '8792' + FAIL_ON: '2' + FAIL_STATUS: '429' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8792/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8792/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8792' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert it failed immediately rather than retrying + run: | + cat mock.log + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: a bare 429 (quota exhausted) did not fail the run." + exit 1 + fi + # The startup probe POSTs; only GETs are polls. Two of them means the + # opening poll and the 429 — no retry. This is the half that catches a + # 'simplification' making every 429 transient: that mutant retries to + # a completed scan, six GETs and status=completed. + gets=$(grep -c '"GET ' mock.log || true) + if [ "$gets" -ne 2 ]; then + echo "FAIL: expected exactly 2 polls (no retry), saw ${gets}." + exit 1 + fi + echo "PASS: a 429 with no Retry-After failed fast, without retrying." + + test-results-fetch-429-with-retry-after: + name: "Results fetch: rides out a 429 that carries Retry-After" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (poll succeeds, first results fetch 429s) + env: + PORT: '8793' + FAIL_ON: '2' + FAIL_COUNT: '1' + FAIL_STATUS: '429' + FAIL_RETRY_AFTER: '1' + COMPLETE_AFTER: '1' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8793/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8793/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8793' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the results fetch honored Retry-After + run: | + cat mock.log + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: a throttled results fetch killed the run (status='${{ steps.scan.outputs.status }}')." + exit 1 + fi + echo "PASS: the results fetch honored Retry-After and completed." + test-yaml-validity: name: action.yml is valid YAML runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cfa1a95..471bfa56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **A 429 during scan polling is retried when it carries `Retry-After`** (sable-96ex). Polling every 10 seconds from every customer repo is exactly the traffic shape a rate limiter targets, and all three surfaces treated a 429 as fatal — 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. A 429 cannot simply join 5xx either: on scan *submit* it means the account is out of credits. `Retry-After` is the disambiguator — a limiter sends one, a quota rejection does not — so the poll and results paths now retry a 429 that carries one, sleeping `min(Retry-After, 60s)` against the same failure budget as any other transient failure, and still fail fast on a bare 429. Submit is unchanged: exit `3`, even with the header. Giving up on repeated 429s now says the API rate limited us rather than blaming the report. Delay-seconds only; the HTTP-date form counts as absent. Full contract in `shared-docs/CLI_SPEC.md`. - **`timeout-minutes` on the GitHub Action is now a wall-clock deadline**, not a poll count. Previously the action ran `timeout-minutes * 6` polls, each costing 10s *plus* API latency, so a slow API pushed real elapsed time past the documented budget. It is now enforced as a real deadline. **This can fail workflows that were relying on the overrun** — if a scan sits near the boundary, raise `timeout-minutes`. - `rafter get ` (without `--interactive`) now retries transient failures too. It is the command the poll loop's give-up message recommends, so a remedy defeated by the same transient failure it is recommended for was not a remedy. - HTTP requests on the poll and results paths now carry connect/read timeouts (`--connect-timeout 10 --max-time 60` for curl, 30s for axios), so a hung server cannot stall inside a request that the retry loop only checks between attempts. diff --git a/github-action/action.yml b/github-action/action.yml index e096efad..068720ca 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -55,7 +55,7 @@ outputs: description: 'Number of low/note findings' value: ${{ steps.results.outputs.low_count }} status: - description: 'Scan status: completed, failed, timeout, unreadable (the scan may have finished but its report could not be read), or unreachable (the Rafter API could not be contacted)' + description: 'Scan status: completed, failed, timeout, unreadable (the scan may have finished but its report could not be read), rate-limited (the API throttled us until the retry budget ran out), or unreachable (the Rafter API could not be contacted)' value: ${{ steps.results.outputs.status || steps.poll.outputs.status }} runs: @@ -150,6 +150,13 @@ runs: MAX_TRANSIENT_FAILURES=5 TRANSIENT_FAILURES=0 LAST_ERROR="" + # sable-96ex — a 429 is ambiguous here: on scan submit it means "out of + # credits", on this endpoint it would mean "going too fast". + # Retry-After is the disambiguator, so a 429 is retried only when the + # server says when to come back. This is the longest such wait we honor; + # a CI job cannot sit out an hour-long limiter window. + MAX_RETRY_AFTER=60 + LAST_RATE_LIMITED=0 # Wall-clock deadline so retry backoff cannot quietly stretch the # documented timeout-minutes budget. @@ -159,16 +166,18 @@ runs: while [ "$(date +%s)" -lt "$DEADLINE" ]; do BODY_FILE="$(mktemp)" + HDR_FILE="$(mktemp)" HTTP_CODE=$(curl -sS --connect-timeout 10 --max-time 60 \ - -o "$BODY_FILE" -w "%{http_code}" \ + -o "$BODY_FILE" -D "$HDR_FILE" -w "%{http_code}" \ -H "x-api-key: ${RAFTER_API_KEY}" \ "${RAFTER_URL}/api/static/scan?scan_id=${SCAN_ID}") || { # A transport error is exactly as transient as a 5xx, and counts # the same. Previously it retried on a flat 10s forever, which # meant an unreachable backend reported "scan did not complete # within N minutes" — a timeout message for a DNS failure. - rm -f "$BODY_FILE" + rm -f "$BODY_FILE" "$HDR_FILE" LAST_ERROR="curl transport error contacting ${RAFTER_URL}" + LAST_RATE_LIMITED=0 TRANSIENT_FAILURES=$((TRANSIENT_FAILURES+1)) if [ "$TRANSIENT_FAILURES" -ge "$MAX_TRANSIENT_FAILURES" ]; then echo "::error::Rafter could not reach the API for scan ${SCAN_ID} after ${MAX_TRANSIENT_FAILURES} attempts." @@ -184,14 +193,41 @@ runs: continue } RESPONSE=$(cat "$BODY_FILE") - rm -f "$BODY_FILE" + # Retry-After is server-controlled and becomes a sleep duration. + # Digits only, so a malformed header cannot produce a garbage sleep; + # and capped in LENGTH as well as value, because `[ 1e23 -gt 60 ]` is + # an error that evaluates false, which would leave `sleep` a number it + # honors literally and hang the job until the runner times out. + RETRY_AFTER="" + if [ "$HTTP_CODE" -eq 429 ]; then + RETRY_AFTER=$(tr -d '\r' < "$HDR_FILE" | grep -i '^retry-after:' \ + | tail -n1 | sed 's/^[^:]*:[[:space:]]*//' || true) + case "$RETRY_AFTER" in + ''|*[!0-9]*) RETRY_AFTER="" ;; + *) + if [ "${#RETRY_AFTER}" -gt 6 ]; then RETRY_AFTER="$MAX_RETRY_AFTER"; fi + ;; + esac + fi + rm -f "$BODY_FILE" "$HDR_FILE" - if [ "$HTTP_CODE" -ge 500 ] || [ "$HTTP_CODE" -eq 408 ] || [ "$HTTP_CODE" -eq 404 ]; then + if [ "$HTTP_CODE" -ge 500 ] || [ "$HTTP_CODE" -eq 408 ] || [ "$HTTP_CODE" -eq 404 ] \ + || { [ "$HTTP_CODE" -eq 429 ] && [ -n "$RETRY_AFTER" ]; }; then ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) LAST_ERROR="HTTP ${HTTP_CODE}${ERROR:+ — $ERROR}" TRANSIENT_FAILURES=$((TRANSIENT_FAILURES+1)) + if [ -n "$RETRY_AFTER" ]; then LAST_RATE_LIMITED=1; else LAST_RATE_LIMITED=0; fi if [ "$TRANSIENT_FAILURES" -ge "$MAX_TRANSIENT_FAILURES" ]; then + if [ "$LAST_RATE_LIMITED" -eq 1 ]; then + # Saying "could not read the report" for a throttled run points + # the customer at the wrong thing entirely. + echo "::error::Rafter was rate limited by the API while polling scan ${SCAN_ID} after ${MAX_TRANSIENT_FAILURES} attempts." + echo "::error::The scan itself may still be running — check it in your dashboard at ${RAFTER_URL}/dashboard" + echo "::error::Last response from the server: ${LAST_ERROR}" + echo "status=rate-limited" >> "$GITHUB_OUTPUT" + exit 1 + fi echo "::error::Rafter could not read the report for scan ${SCAN_ID} after ${MAX_TRANSIENT_FAILURES} attempts." echo "::error::The scan itself may have finished — check it in your dashboard at ${RAFTER_URL}/dashboard" echo "::error::Last response from the server: ${LAST_ERROR}" @@ -200,7 +236,14 @@ runs: fi BACKOFF=$(( 2 ** TRANSIENT_FAILURES )) # 2s, 4s, 8s, 16s - echo "::warning::Report not readable yet (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" + if [ -n "$RETRY_AFTER" ]; then + # The server named the delay; obey it instead of our own schedule. + BACKOFF="$RETRY_AFTER" + if [ "$BACKOFF" -gt "$MAX_RETRY_AFTER" ]; then BACKOFF="$MAX_RETRY_AFTER"; fi + echo "::warning::Rate limited by the API (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" + else + echo "::warning::Report not readable yet (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" + fi sleep "$BACKOFF" POLL_COUNT=$((POLL_COUNT+1)) continue @@ -254,25 +297,50 @@ runs: # here: this runs the instant the scan reports completed, which is the # likeliest moment for the report object to not be readable yet. Retry # transient failures with backoff rather than failing the build. + # + # sable-96ex — a 429 is retried here on exactly one condition: the + # server sent a Retry-After saying when to come back. Without it a 429 + # on this API means quota exhausted, and sleeping through five retries + # before failing anyway is worse than failing now. fetch_results() { local out="$1" local url="$2" local attempt=1 local max_attempts=5 - local code body err last="" + local max_retry_after=60 + local code body err hdr retry_after last="" rate_limited=0 while :; do + hdr="$(mktemp)" + retry_after="" if code=$(curl -sS --connect-timeout 10 --max-time 60 \ - -o "$out" -w "%{http_code}" \ + -o "$out" -D "$hdr" -w "%{http_code}" \ -H "x-api-key: ${RAFTER_API_KEY}" "$url"); then if [ "$code" -ge 200 ] && [ "$code" -lt 300 ]; then + rm -f "$hdr" return 0 fi body=$(cat "$out" || true) err=$(echo "$body" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) last="HTTP ${code}${err:+ — $err}" - if [ "$code" -lt 500 ] && [ "$code" -ne 408 ] && [ "$code" -ne 404 ]; then - # Not transient — a bad key or malformed request. Say so now. + if [ "$code" -eq 429 ]; then + # Server-controlled, and becomes a sleep duration: digits only, + # and length-capped so a 23-digit value cannot slip past the + # numeric comparison below and hang the job (see the poll loop). + retry_after=$(tr -d '\r' < "$hdr" | grep -i '^retry-after:' \ + | tail -n1 | sed 's/^[^:]*:[[:space:]]*//' || true) + case "$retry_after" in + ''|*[!0-9]*) retry_after="" ;; + *) + if [ "${#retry_after}" -gt 6 ]; then retry_after="$max_retry_after"; fi + ;; + esac + fi + if [ "$code" -lt 500 ] && [ "$code" -ne 408 ] && [ "$code" -ne 404 ] \ + && [ -z "$retry_after" ]; then + # Not transient — a bad key, an exhausted quota, or a malformed + # request. Say so now. + rm -f "$hdr" echo "::error::Rafter results fetch failed: ${last}" echo "status=unreadable" >> "$GITHUB_OUTPUT" return 1 @@ -280,8 +348,17 @@ runs: else last="curl transport error fetching ${url}" fi + rm -f "$hdr" + if [ -n "$retry_after" ]; then rate_limited=1; else rate_limited=0; fi if [ "$attempt" -ge "$max_attempts" ]; then + if [ "$rate_limited" -eq 1 ]; then + echo "::error::Rafter was rate limited by the API while fetching the report for scan ${SCAN_ID} after ${max_attempts} attempts." + echo "::error::The scan itself finished — check it in your dashboard at ${RAFTER_URL}/dashboard" + echo "::error::Last response from the server: ${last}" + echo "status=rate-limited" >> "$GITHUB_OUTPUT" + return 1 + fi echo "::error::Rafter could not read the report for scan ${SCAN_ID} after ${max_attempts} attempts." echo "::error::The scan itself finished — check it in your dashboard at ${RAFTER_URL}/dashboard" echo "::error::Last response from the server: ${last}" @@ -293,7 +370,13 @@ runs: fi local backoff=$(( 2 ** attempt )) - echo "::warning::Report not readable yet (${last}); retrying in ${backoff}s (${attempt}/${max_attempts})" + if [ -n "$retry_after" ]; then + backoff="$retry_after" + if [ "$backoff" -gt "$max_retry_after" ]; then backoff="$max_retry_after"; fi + echo "::warning::Rate limited by the API (${last}); retrying in ${backoff}s (${attempt}/${max_attempts})" + else + echo "::warning::Report not readable yet (${last}); retrying in ${backoff}s (${attempt}/${max_attempts})" + fi sleep "$backoff" attempt=$((attempt+1)) done diff --git a/github-action/tests/mock-rafter-api.py b/github-action/tests/mock-rafter-api.py index 2b77ab97..ecf3f151 100644 --- a/github-action/tests/mock-rafter-api.py +++ b/github-action/tests/mock-rafter-api.py @@ -18,7 +18,11 @@ PORT listen port (default 8787) FAIL_ON 1-based GET index that starts failing (default 2) FAIL_STATUS status code to fail with (default 500; 404 exercises the - read-after-write-lag branch) + read-after-write-lag branch, 429 the rate-limit branch) + FAIL_RETRY_AFTER when set, injected failures carry this literal Retry-After + header value. sable-96ex: a 429 is retried ONLY when one is + present, so setting/omitting this is what separates the two + halves of that contract. FAIL_FOREVER if "1", every GET from FAIL_ON onward fails (persistent case) FAIL_COUNT how many consecutive GETs fail starting at FAIL_ON (default 1; ignored when FAIL_FOREVER is set) @@ -37,6 +41,9 @@ FAIL_STATUS = int(os.environ.get("FAIL_STATUS", "500")) FAIL_FOREVER = os.environ.get("FAIL_FOREVER") == "1" FAIL_COUNT = int(os.environ.get("FAIL_COUNT", "1")) +#: Sent verbatim, so a test can inject a malformed value ("soon", "-1") and +#: check the client refuses to act on it. +FAIL_RETRY_AFTER = os.environ.get("FAIL_RETRY_AFTER") COMPLETE_AFTER = int(os.environ.get("COMPLETE_AFTER", str(FAIL_ON))) SCAN_ID = "repro-sable-l10k-0001" @@ -45,11 +52,13 @@ class Handler(BaseHTTPRequestHandler): - def _send(self, code, payload): + def _send(self, code, payload, retry_after=None): body = json.dumps(payload).encode() self.send_response(code) self.send_header("Content-Type", "application/json") self.send_header("Content-Length", str(len(body))) + if retry_after is not None: + self.send_header("Retry-After", retry_after) self.end_headers() self.wfile.write(body) @@ -76,6 +85,12 @@ def do_GET(self): ) if failing: # The verbatim customer-facing body. + if FAIL_STATUS == 429: + return self._send( + FAIL_STATUS, + {"error": "Too many requests"}, + retry_after=FAIL_RETRY_AFTER, + ) return self._send( FAIL_STATUS, {"error": "Failed to fetch report from storage: Object not found"}, @@ -97,5 +112,9 @@ def log_message(self, fmt, *args): if __name__ == "__main__": - print(f"mock rafter api on :{PORT} (500 on poll #{FAIL_ON}, forever={FAIL_FOREVER})", flush=True) + print( + f"mock rafter api on :{PORT} ({FAIL_STATUS} on poll #{FAIL_ON}, " + f"forever={FAIL_FOREVER}, retry-after={FAIL_RETRY_AFTER})", + flush=True, + ) HTTPServer(("127.0.0.1", PORT), Handler).serve_forever() diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 59444a34..18a8cbe7 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -163,6 +163,70 @@ else failures=$((failures+1)) fi +# ── sable-96ex: the 429 / Retry-After contract ─────────────────────────── +# A 429 is retried on exactly one condition — the server said when to come +# back. Both halves are load-bearing: drop the gate and a quota rejection +# costs every build 30 seconds before failing anyway; drop the retry and the +# day a limiter lands in front of the poll endpoint, every customer build +# fails instantly on a condition a sleep would have resolved. + +# 14. The poll loop's 429 branch must be gated on a non-empty Retry-After. +if grep -q '\[ "\$HTTP_CODE" -eq 429 \] && \[ -n "\$RETRY_AFTER" \]' "$ACTION_YML"; then + echo "PASS: poll loop retries 429 only when Retry-After is present" +else + echo "FAIL: poll loop's 429 branch is no longer gated on Retry-After" + failures=$((failures+1)) +fi + +# 15. Same gate in the results fetch: a 429 stays non-transient there unless +# Retry-After came with it. +if grep -q '\[ "\$code" -ne 404 \] \\' "$ACTION_YML" \ + && grep -q '&& \[ -z "\$retry_after" \]; then' "$ACTION_YML"; then + echo "PASS: results fetch retries 429 only when Retry-After is present" +else + echo "FAIL: results fetch's 429 gate changed" + failures=$((failures+1)) +fi + +# 16. Retry-After is server-controlled and becomes a sleep duration. It must be +# validated as digits AND length-capped in both loops: `[ 1e23 -gt 60 ]` is +# an error that evaluates false, so an uncapped 23-digit value would reach +# `sleep` intact and hang the job until the runner times out. +# `set -e` is on: an `x && y` list whose test fails would abort the script +# before it could report anything, so these stay full `if` statements. +digit_guards=0 +if grep -qF "''|*[!0-9]*) RETRY_AFTER=\"\" ;;" "$ACTION_YML"; then + digit_guards=$((digit_guards+1)) +fi +if grep -qF "''|*[!0-9]*) retry_after=\"\" ;;" "$ACTION_YML"; then + digit_guards=$((digit_guards+1)) +fi +len_guards=$(grep -cE '\$\{#(RETRY_AFTER|retry_after)\}" -gt 6' "$ACTION_YML" || true) +if [ "$digit_guards" -eq 2 ] && [ "$len_guards" -eq 2 ]; then + echo "PASS: Retry-After validated (digits + length) in both retry loops" +else + echo "FAIL: Retry-After validation missing (digit guards=${digit_guards}, length guards=${len_guards}; want 2 and 2)" + failures=$((failures+1)) +fi + +# 17. The honored wait must be capped. An unclamped Retry-After lets the server +# park a CI job for as long as it likes. +if [ "$(grep -cE '(BACKOFF|backoff)" -gt "\$(MAX_RETRY_AFTER|max_retry_after)"' "$ACTION_YML" || true)" -eq 2 ]; then + echo "PASS: both loops cap the honored Retry-After" +else + echo "FAIL: a retry loop no longer caps the Retry-After it honors" + failures=$((failures+1)) +fi + +# 18. A throttled give-up must not be reported as an unreadable report — that +# sends the customer to look at their scan instead of their rate limit. +if [ "$(grep -c 'status=rate-limited' "$ACTION_YML" || true)" -ge 2 ]; then + echo "PASS: both give-up paths distinguish rate-limited from unreadable" +else + echo "FAIL: status=rate-limited missing from a give-up path" + failures=$((failures+1)) +fi + echo "" echo "── results ───────────────────────────────────────────────────────────" echo "Failures: $failures" diff --git a/node/src/commands/backend/scan-status.ts b/node/src/commands/backend/scan-status.ts index 2d7090ac..f8604d14 100644 --- a/node/src/commands/backend/scan-status.ts +++ b/node/src/commands/backend/scan-status.ts @@ -33,6 +33,49 @@ export const MAX_TRANSIENT_POLL_FAILURES = 5; */ export const MAX_TOTAL_TRANSIENT_POLL_FAILURES = 20; +/** + * sable-96ex — a 429 during polling is ambiguous in this API: on scan SUBMIT it + * means "you are out of credits" (exit 3), and on the poll endpoint it would + * mean "you are going too fast". `Retry-After` is the disambiguator — a quota + * rejection does not carry one, a rate limiter does — so a 429 is retried here + * only when the server tells us how long to wait, and fails fast otherwise. + * + * Longest Retry-After we honor. A limiter is free to answer "come back in an + * hour"; a CI job cannot sit there for it, and the failure budget still bounds + * the total wait either way. + */ +export const MAX_RETRY_AFTER_MS = 60_000; + +/** + * `Retry-After` in milliseconds, or null when it is absent or unusable. + * + * Only the delay-seconds form is honored. The HTTP-date form is deliberately + * not parsed: the composite action has to make the same decision in bash on + * whatever `date` the runner ships, and a rule the three surfaces cannot state + * identically is worse than a narrow one they can. An unparseable header is + * treated as absent — which means fail fast, the conservative direction. + */ +export function retryAfterMs(e: any): number | null { + const headers = e?.response?.headers; + if (!headers) return null; + let raw: unknown; + if (typeof headers.get === "function") raw = headers.get("retry-after"); + if (raw === undefined || raw === null) { + for (const k of Object.keys(headers)) { + if (k.toLowerCase() === "retry-after") { + raw = (headers as any)[k]; + break; + } + } + } + if (Array.isArray(raw)) raw = raw[0]; + if (raw === undefined || raw === null) return null; + const text = String(raw).trim(); + if (!/^\d+$/.test(text)) return null; + // A 400-digit header parses to Infinity; the clamp is what makes that safe. + return Math.min(Number(text) * 1000, MAX_RETRY_AFTER_MS); +} + /** Longest single error detail we will echo back. Servers can be verbose. */ const MAX_ERROR_DETAIL_CHARS = 200; @@ -52,6 +95,11 @@ function isTransientPollError(e: any, scanExists: boolean): boolean { return Boolean(e?.isAxiosError || e?.request); } if (status === 404) return scanExists; + // 429 is transient only when the server said when to come back. See + // MAX_RETRY_AFTER_MS: without that header a 429 here is a quota rejection, + // and retrying one for half a minute before failing anyway is worse than + // failing now. + if (status === 429) return retryAfterMs(e) !== null; return status >= 500 || status === 408; } @@ -111,6 +159,26 @@ export function unreadableReportMessage( ); } +/** + * The give-up message when polling was throttled rather than blocked on an + * unreadable report. Telling a customer their report could not be read, when + * what actually happened is that we were rate limited, points them at the + * wrong thing — and at a `rafter get` that will be throttled too. + */ +export function rateLimitedMessage( + scan_id: string, + lastError: string, + attempts: number = MAX_TRANSIENT_POLL_FAILURES +): string { + return ( + `Rafter was rate limited by the API while polling scan ${scan_id} ` + + `(${attempts} attempts).\n` + + `The scan itself may still be running — retry with: rafter get ${scan_id}\n` + + `or open the scan in your dashboard at https://rafter.so/dashboard\n` + + `Last response from the server: ${lastError}` + ); +} + export const BASE_BACKOFF_MS = 2000; /** 2s, 4s, 8s, 16s — the 5th failure gives up rather than sleeping again. */ @@ -137,12 +205,15 @@ class FailureBudget { last = ""; /** False once any failure carried no HTTP response at all. */ lastReachedServer = true; + /** True when the failure we gave up on was a throttled 429, not a bad read. */ + lastRateLimited = false; - record(detail: string, reachedServer: boolean): number { + record(detail: string, reachedServer: boolean, rateLimited = false): number { this.consecutive += 1; this.total += 1; this.last = detail; this.lastReachedServer = reachedServer; + this.lastRateLimited = rateLimited; return this.consecutive; } @@ -159,7 +230,29 @@ class FailureBudget { } } -type RetryNotice = (attempt: number, waitMs: number, detail: string) => void; +type RetryNotice = ( + attempt: number, + waitMs: number, + detail: string, + rateLimited: boolean +) => void; + +/** + * Retries are printed to stderr, not just into the spinner: ora renders + * nothing on a non-TTY, and CI is exactly where this diagnostic matters. + */ +function makeRetryNotice(quiet?: boolean): RetryNotice | undefined { + if (quiet) return undefined; + return (attempt, waitMs, detail, rateLimited) => { + const what = rateLimited + ? `Rate limited by the API (${detail})` + : `Report not readable yet (${detail})`; + console.error( + `${what}; retrying in ${Math.round(waitMs / 1000)}s ` + + `(${attempt}/${MAX_TRANSIENT_POLL_FAILURES})` + ); + }; +} /** * One poll, with retry/backoff over transient failures. @@ -187,23 +280,32 @@ async function pollUntilReadable( } catch (e: any) { if (!isTransientPollError(e, scanExists)) throw e; + // Non-null only for a 429 that carried a usable Retry-After — the sole + // reason such a 429 got past the classifier above. + const retryAfter = e?.response?.status === 429 ? retryAfterMs(e) : null; const attempt = budget.record( describeHttpError(e), - e?.response?.status !== undefined + e?.response?.status !== undefined, + retryAfter !== null ); if (budget.exhausted) { throw new PollGaveUpError( - unreadableReportMessage( - scan_id, - budget.last, - budget.total, - budget.lastReachedServer - ) + budget.lastRateLimited + ? rateLimitedMessage(scan_id, budget.last, budget.total) + : unreadableReportMessage( + scan_id, + budget.last, + budget.total, + budget.lastReachedServer + ) ); } - const waitMs = backoffMs(attempt); - onRetry?.(attempt, waitMs, budget.last); + // The server named the delay; obey it instead of our own schedule. It + // still spends a failure from the budget, so an endlessly-throttling + // API cannot keep the loop alive. + const waitMs = retryAfter ?? backoffMs(attempt); + onRetry?.(attempt, waitMs, budget.last, retryAfter !== null); await new Promise((r) => setTimeout(r, waitMs)); } } @@ -223,32 +325,21 @@ export async function fetchScanWithRetry( quiet?: boolean ): Promise { const budget = new FailureBudget(); - const onRetry: RetryNotice | undefined = quiet - ? undefined - : (attempt, waitMs, detail) => { - console.error( - `Report not readable yet (${detail}); retrying in ${Math.round(waitMs / 1000)}s ` + - `(${attempt}/${MAX_TRANSIENT_POLL_FAILURES})` - ); - }; - return pollUntilReadable(scan_id, headers, fmt, budget, false, onRetry); + return pollUntilReadable( + scan_id, + headers, + fmt, + budget, + false, + makeRetryNotice(quiet) + ); } const IN_PROGRESS = ["queued", "pending", "processing"]; export async function handleScanStatus(scan_id: string, headers: any, fmt: string, quiet?: boolean): Promise { const budget = new FailureBudget(); - - // Retries are printed to stderr, not just into the spinner: ora renders - // nothing on a non-TTY, and CI is exactly where this diagnostic matters. - const onRetry: RetryNotice | undefined = quiet - ? undefined - : (attempt, waitMs, detail) => { - console.error( - `Report not readable yet (${detail}); retrying in ${Math.round(waitMs / 1000)}s ` + - `(${attempt}/${MAX_TRANSIENT_POLL_FAILURES})` - ); - }; + const onRetry = makeRetryNotice(quiet); // First poll. A 404 here really does mean "no such scan" — do not retry it. // Transient 5xx IS retried, so that the `rafter get ` this command diff --git a/node/tests/scan-poll-429-retry-after.test.ts b/node/tests/scan-poll-429-retry-after.test.ts new file mode 100644 index 00000000..ba15b90d --- /dev/null +++ b/node/tests/scan-poll-429-retry-after.test.ts @@ -0,0 +1,292 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +/** + * sable-96ex — a 429 during polling used to be classified non-transient, in a + * codebase that polls every 10 seconds from every customer repo. That 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. + * + * It cannot simply join 408/5xx either: on scan SUBMIT a 429 means "out of + * credits" (exit 3), and retrying a quota rejection for half a minute before + * failing anyway is worse than failing now. `Retry-After` is the + * disambiguator — a limiter sends one, a quota rejection does not. + * + * These tests pin both halves, plus the two things that make honoring a + * server-supplied delay safe: the cap, and refusing to guess at a header we + * cannot parse. + */ + +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and the instance carries + // its OWN mocks so a regression to bare `axios.get` fails these assertions + // rather than silently passing them. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); +vi.mock("ora", () => ({ + default: () => ({ + start: vi.fn().mockReturnThis(), + succeed: vi.fn().mockReturnThis(), + fail: vi.fn().mockReturnThis(), + stop: vi.fn().mockReturnThis(), + text: "", + }), +})); + +import axios from "axios"; +import { + handleScanStatus, + retryAfterMs, + rateLimitedMessage, + MAX_RETRY_AFTER_MS, + MAX_TRANSIENT_POLL_FAILURES, +} from "../src/commands/backend/scan-status.js"; +import { EXIT_SUCCESS, EXIT_GENERAL_ERROR } from "../src/utils/api.js"; + +const mockedAxios = vi.mocked((axios as any).create(), true); + +/** A 429 as a limiter sends it: with a delay the client can act on. */ +function throttled(retryAfter?: string | number | string[]) { + return { + response: { + status: 429, + data: { error: "Too many requests" }, + headers: retryAfter === undefined ? {} : { "retry-after": retryAfter }, + }, + }; +} + +/** Record every delay the code asks for, and fire the callback immediately. */ +function recordDelays(): number[] { + const delays: number[] = []; + const real = globalThis.setTimeout; + vi.stubGlobal("setTimeout", ((fn: any, ms?: number) => { + delays.push(ms ?? 0); + return real(fn, 0); + }) as any); + return delays; +} + +describe("retryAfterMs", () => { + it("reads a plain delay-seconds header", () => { + expect(retryAfterMs(throttled("5"))).toBe(5000); + }); + + it("tolerates surrounding whitespace", () => { + expect(retryAfterMs(throttled(" 5 "))).toBe(5000); + }); + + it("is case-insensitive about the header name", () => { + expect( + retryAfterMs({ response: { status: 429, headers: { "Retry-After": "7" } } }) + ).toBe(7000); + }); + + it("reads an AxiosHeaders-style accessor", () => { + const headers: any = { get: (n: string) => (n === "retry-after" ? "9" : null) }; + expect(retryAfterMs({ response: { status: 429, headers } })).toBe(9000); + }); + + it("takes the first value when the header repeats", () => { + expect(retryAfterMs(throttled(["4", "900"]))).toBe(4000); + }); + + it("honors Retry-After: 0", () => { + // Distinct from absent, and the failure budget still bounds the loop. + expect(retryAfterMs(throttled("0"))).toBe(0); + }); + + it("caps a limiter that asks for an hour", () => { + expect(retryAfterMs(throttled("3600"))).toBe(MAX_RETRY_AFTER_MS); + }); + + it("clamps an absurd value rather than overflowing", () => { + // 400 digits parses to Infinity; the clamp is what makes that harmless. + expect(retryAfterMs(throttled("9".repeat(400)))).toBe(MAX_RETRY_AFTER_MS); + }); + + it("returns null for the HTTP-date form", () => { + // Deliberately unparsed: the composite action has to make the same call in + // bash on whatever `date` the runner ships. Unparseable means fail fast. + expect(retryAfterMs(throttled("Wed, 21 Oct 2026 07:28:00 GMT"))).toBeNull(); + }); + + it("returns null for a negative or non-numeric value", () => { + expect(retryAfterMs(throttled("-5"))).toBeNull(); + expect(retryAfterMs(throttled("soon"))).toBeNull(); + expect(retryAfterMs(throttled("1.5"))).toBeNull(); + }); + + it("returns null when the header is absent", () => { + expect(retryAfterMs(throttled())).toBeNull(); + expect(retryAfterMs({ response: { status: 429 } })).toBeNull(); + }); +}); + +describe("handleScanStatus — 429 during polling (sable-96ex)", () => { + const headers = { "x-api-key": "test-key" }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it("rides out a 429 that carries Retry-After", async () => { + mockedAxios.get + .mockResolvedValueOnce({ data: { status: "processing" } }) + .mockRejectedValueOnce(throttled("5")) + .mockResolvedValueOnce({ data: { status: "completed", markdown: "# Done" } }); + + recordDelays(); + const code = await handleScanStatus("s1", headers, "md", true); + + expect(code).toBe(EXIT_SUCCESS); + expect(mockedAxios.get).toHaveBeenCalledTimes(3); + }); + + it("sleeps the server's delay, not its own backoff schedule", async () => { + // 2000 is what the exponential schedule would have chosen for attempt 1. + // Pinning the number, not just "it slept", is what makes this test able to + // fail if the header is read but then ignored. + mockedAxios.get + .mockResolvedValueOnce({ data: { status: "processing" } }) + .mockRejectedValueOnce(throttled("5")) + .mockResolvedValueOnce({ data: { status: "completed", markdown: "# Done" } }); + + const delays = recordDelays(); + await handleScanStatus("s1", headers, "md", true); + + expect(delays).toEqual([10000, 5000]); + }); + + it("caps the honored delay so a limiter cannot park a CI job", async () => { + mockedAxios.get + .mockResolvedValueOnce({ data: { status: "processing" } }) + .mockRejectedValueOnce(throttled("3600")) + .mockResolvedValueOnce({ data: { status: "completed", markdown: "# Done" } }); + + const delays = recordDelays(); + await handleScanStatus("s1", headers, "md", true); + + expect(delays).toEqual([10000, MAX_RETRY_AFTER_MS]); + }); + + it("fails fast on a 429 with no Retry-After — that is quota, not throttling", async () => { + mockedAxios.get + .mockResolvedValueOnce({ data: { status: "processing" } }) + .mockRejectedValueOnce(throttled()); + + recordDelays(); + const code = await handleScanStatus("s1", headers, "md", true); + + expect(code).toBe(EXIT_GENERAL_ERROR); + // The opening poll and the 429. A retry would make this 3 or more. + expect(mockedAxios.get).toHaveBeenCalledTimes(2); + }); + + it("fails fast on a Retry-After it cannot parse", async () => { + mockedAxios.get + .mockResolvedValueOnce({ data: { status: "processing" } }) + .mockRejectedValueOnce(throttled("Wed, 21 Oct 2026 07:28:00 GMT")); + + recordDelays(); + const code = await handleScanStatus("s1", headers, "md", true); + + expect(code).toBe(EXIT_GENERAL_ERROR); + expect(mockedAxios.get).toHaveBeenCalledTimes(2); + }); + + it("spends the failure budget, so an endless throttle cannot loop forever", async () => { + let call = 0; + mockedAxios.get.mockImplementation(async () => { + call += 1; + if (call === 1) return { data: { status: "processing" } }; + throw throttled("1"); + }); + + recordDelays(); + const code = await handleScanStatus("s1", headers, "md", true); + + expect(code).toBe(EXIT_GENERAL_ERROR); + expect(call).toBe(1 + MAX_TRANSIENT_POLL_FAILURES); + }); + + it("says it was rate limited, not that the report could not be read", async () => { + // Sending a throttled customer to look at their scan report — or at a + // `rafter get` that will be throttled too — wastes their time on the wrong + // problem entirely. + let call = 0; + mockedAxios.get.mockImplementation(async () => { + call += 1; + if (call === 1) return { data: { status: "processing" } }; + throw throttled("1"); + }); + const errors: string[] = []; + vi.spyOn(console, "error").mockImplementation((m: any) => { + errors.push(String(m)); + }); + + recordDelays(); + await handleScanStatus("scan-abc", headers, "md", true); + + const said = errors.join("\n"); + expect(said).toContain("rate limited"); + expect(said).toContain("scan-abc"); + expect(said).not.toContain("could not read the report"); + }); + + it("still calls a mid-poll 429 retryable after a success has reset the run", async () => { + // The Retry-After gate is about the header, not about how far into the + // poll we are — unlike 404, which is fatal on the first poll only. + mockedAxios.get + .mockRejectedValueOnce(throttled("2")) + .mockResolvedValueOnce({ data: { status: "completed", markdown: "# Done" } }); + + const delays = recordDelays(); + const code = await handleScanStatus("s1", headers, "md", true); + + expect(code).toBe(EXIT_SUCCESS); + expect(delays).toEqual([2000]); + }); +}); + +describe("rateLimitedMessage", () => { + it("names the scan, the cause, and a next step", () => { + const msg = rateLimitedMessage("scan-abc", "HTTP 429 — Too many requests", 5); + + expect(msg).toContain("rate limited"); + expect(msg).toContain("scan-abc"); + expect(msg).toContain("rafter get scan-abc"); + expect(msg).toContain("dashboard"); + // The raw server wording survives as supporting detail, not as the whole + // explanation. + expect(msg).toContain("HTTP 429"); + expect(msg.split("\n").length).toBeGreaterThan(1); + }); +}); diff --git a/node/tests/scan-remote.test.ts b/node/tests/scan-remote.test.ts index 7d620a1c..97144aa0 100644 --- a/node/tests/scan-remote.test.ts +++ b/node/tests/scan-remote.test.ts @@ -368,6 +368,34 @@ describe("runRemoteScan", () => { expect(exitSpy).toHaveBeenCalledWith(3); // EXIT_QUOTA_EXHAUSTED }); + it("still exits with EXIT_QUOTA_EXHAUSTED on a 429 that carries Retry-After", async () => { + // sable-96ex — the poll loop now retries a 429 when the server says when to + // come back. SUBMIT deliberately does not: a 429 here means the account is + // out of credits, and no amount of waiting changes that. The asymmetry is + // the point, so it is pinned rather than left to be "made consistent". + mockedAxios.post.mockRejectedValueOnce({ + response: { + status: 429, + data: "quota exhausted", + headers: { "retry-after": "30" }, + }, + }); + + const { runRemoteScan } = await import("../src/commands/backend/run.js"); + await expect( + runRemoteScan({ + apiKey: "test-key", + repo: "owner/repo", + branch: "main", + skipInteractive: true, + quiet: true, + }) + ).rejects.toThrow("process.exit"); + + expect(exitSpy).toHaveBeenCalledWith(3); // EXIT_QUOTA_EXHAUSTED + expect(mockedAxios.get).not.toHaveBeenCalled(); // no polling, no retry + }); + it("exits with EXIT_INSUFFICIENT_SCOPE on 403 with scope keyword", async () => { mockedAxios.post.mockRejectedValueOnce({ response: { diff --git a/python/rafter_cli/commands/backend.py b/python/rafter_cli/commands/backend.py index 90ead2f4..4215aa94 100644 --- a/python/rafter_cli/commands/backend.py +++ b/python/rafter_cli/commands/backend.py @@ -109,6 +109,17 @@ def _confirm_plus_scan(mode: str, yes: bool) -> None: BASE_BACKOFF_SECONDS = 2 +# sable-96ex — a 429 during polling is ambiguous in this API: on scan SUBMIT it +# means "you are out of credits" (exit 3), and on the poll endpoint it would +# mean "you are going too fast". ``Retry-After`` is the disambiguator — a quota +# rejection does not carry one, a rate limiter does — so a 429 is retried here +# only when the server tells us how long to wait, and fails fast otherwise. +# +#: Longest Retry-After we honor. A limiter is free to answer "come back in an +#: hour"; a CI job cannot sit there for it, and the failure budget still bounds +#: the total wait either way. +MAX_RETRY_AFTER_SECONDS = 60 + #: Longest single error detail we will echo back. Servers can be verbose. MAX_ERROR_DETAIL_CHARS = 200 @@ -139,15 +150,45 @@ def backoff_seconds(consecutive_failures: int) -> int: return BASE_BACKOFF_SECONDS * 2 ** (consecutive_failures - 1) -def _is_transient_poll_status(status_code: int, scan_exists: bool) -> bool: +def retry_after_seconds(resp) -> "int | None": + """``Retry-After`` in seconds, or ``None`` when absent or unusable. + + Only the delay-seconds form is honored. The HTTP-date form is deliberately + not parsed: the composite action has to make the same decision in bash on + whatever ``date`` the runner ships, and a rule the three surfaces cannot + state identically is worse than a narrow one they can. An unparseable + header is treated as absent — which means fail fast, the conservative + direction. + """ + headers = getattr(resp, "headers", None) + value = headers.get("Retry-After") if hasattr(headers, "get") else None + # requests always yields a str; anything else (notably a test double's + # auto-attribute) is not a header the server sent. + if not isinstance(value, str): + return None + text = value.strip() + if not text.isdigit(): + return None + return min(int(text), MAX_RETRY_AFTER_SECONDS) + + +def _is_transient_poll_status( + status_code: int, scan_exists: bool, retry_after: "int | None" = None +) -> bool: """Transient = the server itself describes the condition as temporary. ``scan_exists`` gates 404: before the first successful poll a 404 means the scan id is wrong, and retrying it just delays a clear answer. After it, a missing scan is read-after-write lag. + + ``retry_after`` gates 429: without that header a 429 here is a quota + rejection, and retrying one for half a minute before failing anyway is + worse than failing now. See ``MAX_RETRY_AFTER_SECONDS``. """ if status_code == 404: return scan_exists + if status_code == 429: + return retry_after is not None return status_code >= 500 or status_code == 408 @@ -208,6 +249,26 @@ def unreadable_report_message( ) +def rate_limited_message( + scan_id: str, + last_error: str, + attempts: int = MAX_TRANSIENT_POLL_FAILURES, +) -> str: + """The give-up message when polling was throttled, not blocked on a bad read. + + Telling a customer their report could not be read, when what actually + happened is that we were rate limited, points them at the wrong thing — and + at a ``rafter get`` that will be throttled too. + """ + return ( + f"Rafter was rate limited by the API while polling scan {scan_id} " + f"({attempts} attempts).\n" + f"The scan itself may still be running \u2014 retry with: rafter get {scan_id}\n" + "or open the scan in your dashboard at https://rafter.so/dashboard\n" + f"Last response from the server: {last_error}" + ) + + class _FailureBudget: """A failure budget shared across every poll in one interactive call. @@ -222,12 +283,17 @@ def __init__(self) -> None: self.last = "" #: False once any failure carried no HTTP response at all. self.last_reached_server = True + #: True when the failure we gave up on was a throttled 429, not a bad read. + self.last_rate_limited = False - def record(self, detail: str, reached_server: bool = True) -> int: + def record( + self, detail: str, reached_server: bool = True, rate_limited: bool = False + ) -> int: self.consecutive += 1 self.total += 1 self.last = detail self.last_reached_server = reached_server + self.last_rate_limited = rate_limited return self.consecutive def reset(self) -> None: @@ -267,7 +333,14 @@ def _poll_until_readable( if 200 <= resp.status_code < 300: budget.reset() return resp - if not _is_transient_poll_status(resp.status_code, scan_exists): + # Non-None only for a 429 that carried a usable Retry-After — the + # sole reason such a 429 gets past the classifier below. + retry_after = ( + retry_after_seconds(resp) if resp.status_code == 429 else None + ) + if not _is_transient_poll_status( + resp.status_code, scan_exists, retry_after + ): raise PollFatalError( _describe_http_error(resp.status_code, resp.text), status_code=resp.status_code, @@ -278,11 +351,14 @@ def _poll_until_readable( # Transport error (DNS, reset, timeout) — as retryable as a 5xx. detail = _truncate(str(e)) reached_server = False + retry_after = None - attempt = budget.record(detail, reached_server) + attempt = budget.record(detail, reached_server, retry_after is not None) if budget.exhausted: raise PollGaveUpError( - unreadable_report_message( + rate_limited_message(scan_id, budget.last, attempts=budget.total) + if budget.last_rate_limited + else unreadable_report_message( scan_id, budget.last, attempts=budget.total, @@ -290,10 +366,18 @@ def _poll_until_readable( ) ) - wait = backoff_seconds(attempt) + # The server named the delay; obey it instead of our own schedule. It + # still spends a failure from the budget, so an endlessly-throttling + # API cannot keep the loop alive. + wait = backoff_seconds(attempt) if retry_after is None else retry_after if not quiet: + what = ( + f"Rate limited by the API ({budget.last})" + if retry_after is not None + else f"Report not readable yet ({budget.last})" + ) print( - f"Report not readable yet ({budget.last}); retrying in {wait}s " + f"{what}; retrying in {wait}s " f"({attempt}/{MAX_TRANSIENT_POLL_FAILURES})", file=sys.stderr, ) diff --git a/python/tests/test_scan_poll_429_retry_after.py b/python/tests/test_scan_poll_429_retry_after.py new file mode 100644 index 00000000..e936fff7 --- /dev/null +++ b/python/tests/test_scan_poll_429_retry_after.py @@ -0,0 +1,210 @@ +"""sable-96ex — a 429 during polling is retried only when Retry-After says so. + +A 429 used to be classified non-transient, in a codebase that polls every 10 +seconds from every customer repo. That 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. + +It cannot simply join 408/5xx either: on scan SUBMIT a 429 means "out of +credits" (exit 3), and retrying a quota rejection for half a minute before +failing anyway is worse than failing now. ``Retry-After`` is the disambiguator — +a limiter sends one, a quota rejection does not. + +Mirrors node/tests/scan-poll-429-retry-after.test.ts. +""" +from __future__ import annotations + +import json +from unittest.mock import MagicMock, patch + +import pytest +import typer + +from rafter_cli.commands.backend import ( + MAX_RETRY_AFTER_SECONDS, + MAX_TRANSIENT_POLL_FAILURES, + _handle_scan_status_interactive, + rate_limited_message, + retry_after_seconds, +) +from rafter_cli.utils.api import EXIT_GENERAL_ERROR, EXIT_SUCCESS + +HEADERS = {"x-api-key": "test-key"} + +THROTTLED_BODY = json.dumps({"error": "Too many requests"}) + + +def _resp(status_code: int, text: str = "", json_body=None, headers=None) -> MagicMock: + resp = MagicMock() + resp.status_code = status_code + resp.text = text + resp.json.return_value = json_body if json_body is not None else {} + # A MagicMock's auto-attribute would answer every header lookup with a + # truthy mock, so the headers of a test double are always explicit. + resp.headers = {} if headers is None else headers + return resp + + +def _processing() -> MagicMock: + return _resp(200, json_body={"status": "processing"}) + + +def _completed() -> MagicMock: + return _resp(200, json_body={"status": "completed", "markdown": "# Done"}) + + +def _throttled(retry_after=None) -> MagicMock: + """A 429 as a limiter sends it: with a delay the client can act on.""" + headers = {} if retry_after is None else {"Retry-After": retry_after} + return _resp(429, text=THROTTLED_BODY, headers=headers) + + +@pytest.fixture(autouse=True) +def sleeps(): + """Yields the sleep mock so tests can assert the SCHEDULE, not just that + sleeping happened — the whole point is obeying the server's number.""" + with patch("rafter_cli.commands.backend.time.sleep") as m: + yield m + + +class TestRetryAfterSeconds: + def test_reads_a_plain_delay_seconds_header(self): + assert retry_after_seconds(_throttled("5")) == 5 + + def test_tolerates_surrounding_whitespace(self): + assert retry_after_seconds(_throttled(" 5 ")) == 5 + + def test_header_lookup_is_case_insensitive(self): + # requests hands us a CaseInsensitiveDict; the production path must not + # depend on the server's capitalization. + import requests + + resp = _resp(429, headers=requests.structures.CaseInsensitiveDict( + {"retry-after": "7"} + )) + assert retry_after_seconds(resp) == 7 + + def test_honors_retry_after_zero(self): + # Distinct from absent, and the failure budget still bounds the loop. + assert retry_after_seconds(_throttled("0")) == 0 + + def test_caps_a_limiter_that_asks_for_an_hour(self): + assert retry_after_seconds(_throttled("3600")) == MAX_RETRY_AFTER_SECONDS + + def test_clamps_an_absurd_value(self): + assert retry_after_seconds(_throttled("9" * 400)) == MAX_RETRY_AFTER_SECONDS + + def test_returns_none_for_the_http_date_form(self): + # Deliberately unparsed: the composite action has to make the same call + # in bash on whatever `date` the runner ships. Unparseable = fail fast. + assert retry_after_seconds(_throttled("Wed, 21 Oct 2026 07:28:00 GMT")) is None + + def test_returns_none_for_negative_or_non_numeric(self): + assert retry_after_seconds(_throttled("-5")) is None + assert retry_after_seconds(_throttled("soon")) is None + assert retry_after_seconds(_throttled("1.5")) is None + + def test_returns_none_when_absent(self): + assert retry_after_seconds(_throttled()) is None + assert retry_after_seconds(MagicMock(spec=[])) is None + + +class TestPolling429: + def test_rides_out_a_429_that_carries_retry_after(self): + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing(), _throttled("5"), _completed()] + assert ( + _handle_scan_status_interactive("s1", HEADERS, "md", True) + == EXIT_SUCCESS + ) + assert get.call_count == 3 + + def test_sleeps_the_servers_delay_not_its_own_backoff(self, sleeps): + # 2 is what the exponential schedule would have chosen for attempt 1. + # Pinning the number is what lets this fail if the header is read and + # then ignored. + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing(), _throttled("5"), _completed()] + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert [c.args[0] for c in sleeps.call_args_list] == [10, 5] + + def test_caps_the_honored_delay(self, sleeps): + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing(), _throttled("3600"), _completed()] + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert [c.args[0] for c in sleeps.call_args_list] == [ + 10, + MAX_RETRY_AFTER_SECONDS, + ] + + def test_fails_fast_on_a_429_with_no_retry_after(self): + """That is a quota rejection, and waiting will not earn more credits.""" + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing(), _throttled()] + with pytest.raises(typer.Exit) as exc: + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert exc.value.exit_code == EXIT_GENERAL_ERROR + # The opening poll and the 429. A retry would make this 3 or more. + assert get.call_count == 2 + + def test_fails_fast_on_a_retry_after_it_cannot_parse(self): + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [ + _processing(), + _throttled("Wed, 21 Oct 2026 07:28:00 GMT"), + ] + with pytest.raises(typer.Exit): + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert get.call_count == 2 + + def test_spends_the_failure_budget(self): + """An endlessly-throttling API must not keep the loop alive.""" + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing()] + [ + _throttled("1") for _ in range(MAX_TRANSIENT_POLL_FAILURES + 5) + ] + with pytest.raises(typer.Exit) as exc: + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert exc.value.exit_code == EXIT_GENERAL_ERROR + assert get.call_count == 1 + MAX_TRANSIENT_POLL_FAILURES + + def test_says_rate_limited_not_unreadable_report(self, capsys): + # Sending a throttled customer to look at their scan report — or at a + # `rafter get` that will be throttled too — wastes their time on the + # wrong problem entirely. + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing()] + [ + _throttled("1") for _ in range(MAX_TRANSIENT_POLL_FAILURES) + ] + with pytest.raises(typer.Exit): + _handle_scan_status_interactive("scan-abc", HEADERS, "md", True) + err = capsys.readouterr().err + assert "rate limited" in err + assert "scan-abc" in err + assert "could not read the report" not in err + + def test_a_429_is_retryable_on_the_first_poll_too(self, sleeps): + # The Retry-After gate is about the header, not about how far into the + # poll we are — unlike 404, which is fatal on the first poll only. + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_throttled("2"), _completed()] + assert ( + _handle_scan_status_interactive("s1", HEADERS, "md", True) + == EXIT_SUCCESS + ) + assert [c.args[0] for c in sleeps.call_args_list] == [2] + + +class TestRateLimitedMessage: + def test_names_the_scan_the_cause_and_a_next_step(self): + msg = rate_limited_message("scan-abc", "HTTP 429 — Too many requests", 5) + + assert "rate limited" in msg + assert "scan-abc" in msg + assert "rafter get scan-abc" in msg + assert "dashboard" in msg + # The raw server wording survives as supporting detail, not as the + # whole explanation. + assert "HTTP 429" in msg + assert len(msg.split("\n")) > 1 diff --git a/python/tests/test_scan_remote.py b/python/tests/test_scan_remote.py index 4acf0198..04c8ed07 100644 --- a/python/tests/test_scan_remote.py +++ b/python/tests/test_scan_remote.py @@ -340,6 +340,33 @@ def test_429_raises_quota_exhausted(self, _mock_repo, mock_post): ) assert exc_info.value.exit_code == EXIT_QUOTA_EXHAUSTED + @patch("rafter_cli.commands.backend.api_get") + @patch("rafter_cli.commands.backend.api_post") + @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) + def test_429_with_retry_after_still_raises_quota_exhausted( + self, _mock_repo, mock_post, mock_get + ): + """sable-96ex — the poll loop now retries a 429 when the server says + when to come back. SUBMIT deliberately does not: a 429 here means the + account is out of credits, and no amount of waiting changes that. The + asymmetry is the point, so it is pinned rather than left to be "made + consistent".""" + resp = _mock_response(429, "quota exhausted") + resp.headers = {"Retry-After": "30"} + mock_post.return_value = resp + + with pytest.raises(click.exceptions.Exit) as exc_info: + _do_remote_scan( + repo="owner/repo", + branch="main", + api_key="test-key", + fmt="json", + skip_interactive=True, + quiet=True, + ) + assert exc_info.value.exit_code == EXIT_QUOTA_EXHAUSTED + mock_get.assert_not_called() # no polling, no retry + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_403_scope_raises_insufficient_scope(self, _mock_repo, mock_post, capsys): diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index cd23373d..ad8b32f6 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -24,7 +24,7 @@ The CLI follows UNIX principles: | 0 | Success | | 1 | General error | | 2 | Scan not found (HTTP 404) | -| 3 | Quota exhausted (HTTP 429 or 403 scan-mode limit) | +| 3 | Quota exhausted (HTTP 429 **on scan submit**, or 403 scan-mode limit) | | 4 | Insufficient scope / forbidden (HTTP 403) | | 5 | Paid Plus scan refused — approval required (`scan.plus_requires_approval` on) and no `--yes`/`RAFTER_CONFIRM=1`/interactive confirmation | @@ -136,7 +136,9 @@ A report is not necessarily durable the instant a scan flips to `completed`, so | Transport error (DNS, reset, timeout) | Same as above. | | HTTP 404, **after** the scan is known to exist | Transient — read-after-write lag, not a wrong id. | | HTTP 404 on the **first** poll | Not retried. The scan genuinely does not exist. Exit code `2`. | -| Other 4xx (401/403/429 …) | Not retried — reported immediately. | +| HTTP 429 **with** a usable `Retry-After` | Transient. Retried after sleeping `min(Retry-After, 60s)` instead of the exponential schedule. | +| HTTP 429 **without** one | Not retried — on this API a bare 429 means quota exhausted, and waiting does not earn credits. | +| Other 4xx (401/403 …) | Not retried — reported immediately. | Two budgets bound the retries. The **consecutive** counter (5) resets on any successful poll, so a long scan with occasional blips is not killed by unrelated failures minutes apart. A **total** counter (20 per command invocation) does *not* reset, so a backend alternating success and failure cannot keep the loop alive indefinitely — the CLI has no wall-clock deadline of its own. @@ -144,11 +146,18 @@ After either budget is exhausted the command exits `1`. If the failures reached `rafter get ` carries the same retry budget, so the remedy the give-up message recommends is not defeated by the transient failure that produced it. +**The 429 rule, in full.** A 429 is ambiguous in this API: on scan *submit* it means the account is out of credits (exit `3`), and on the poll endpoint it would mean the client is going too fast. `Retry-After` is the only thing that tells the two apart — a rate limiter sends one, a quota rejection does not — so it decides whether the poll retries. Submit is unchanged either way: a 429 there is exit `3` even when it carries a `Retry-After`. + +- Only the **delay-seconds** form is honored (`Retry-After: 30`). The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. +- The honored wait is capped at **60 seconds**, so a limiter cannot park a CI job for an hour. A 429 retry still spends a failure from the same budget as any other transient failure, so an endlessly-throttling API cannot keep the loop alive. +- Giving up on repeated 429s produces a **rate-limited** message, not the unreadable-report one: it says the API throttled us, names the scan, and points at `rafter get` and the dashboard. Telling a throttled customer their report could not be read sends them to look at the wrong thing. + **The composite GitHub Action** (`github-action/action.yml`) implements the same classification in both its poll loop and its results fetch, with these differences forced by the shell: - It has no "first poll" distinction: by the time it polls, the trigger step has already returned a `scan_id`, so **every** 404 there is treated as read-after-write lag. A scan id the backend accepted but never persisted therefore fails after the 5-failure budget rather than immediately. - Its poll loop is additionally bounded by a wall-clock deadline derived from `timeout-minutes`. Before v0.11 that input was a poll *count*, so a slow API could overrun it; it is now a real deadline. -- Its `status` output is `completed`, `failed`, `timeout`, `unreadable` (the scan may have finished but its report could not be read), or `unreachable` (the API could not be contacted). +- Its `status` output is `completed`, `failed`, `timeout`, `unreadable` (the scan may have finished but its report could not be read), `rate-limited` (the API throttled us until the retry budget ran out), or `unreachable` (the API could not be contacted). +- It validates `Retry-After` as digits and caps its **length** as well as its value: `[ 1e23 -gt 60 ]` is a shell error that evaluates false, so an uncapped 23-digit header would reach `sleep` intact and hang the job until the runner times out. ### rafter usage [OPTIONS] From 5defad671154d733a0d048e47eeadb66eaed50f7 Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:26:20 -0700 Subject: [PATCH 2/6] ci: a honored Retry-After must not outlive the action's deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- github-action/action.yml | 7 +++++++ github-action/tests/test-action-yml-defaults.sh | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/github-action/action.yml b/github-action/action.yml index 068720ca..2e3d8ac6 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -240,6 +240,13 @@ runs: # The server named the delay; obey it instead of our own schedule. BACKOFF="$RETRY_AFTER" if [ "$BACKOFF" -gt "$MAX_RETRY_AFTER" ]; then BACKOFF="$MAX_RETRY_AFTER"; fi + # ...but never past the deadline. timeout-minutes became a real + # wall clock in sable-l10k, and a 60s honored delay is long enough + # to overrun it — the server does not get to extend a budget the + # workflow author set. + REMAINING=$(( DEADLINE - $(date +%s) )) + if [ "$REMAINING" -lt 0 ]; then REMAINING=0; fi + if [ "$BACKOFF" -gt "$REMAINING" ]; then BACKOFF="$REMAINING"; fi echo "::warning::Rate limited by the API (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" else echo "::warning::Report not readable yet (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 18a8cbe7..991ce575 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -218,7 +218,19 @@ else failures=$((failures+1)) fi -# 18. A throttled give-up must not be reported as an unreadable report — that +# 18. ...and the poll loop's honored delay must also be clamped to what is left +# of the wall-clock deadline. timeout-minutes became a real deadline in +# sable-l10k; a 60s Retry-After is long enough to overrun it, and the server +# does not get to extend a budget the workflow author set. +if grep -q 'REMAINING=$(( DEADLINE - $(date +%s) ))' "$ACTION_YML" \ + && grep -q 'if \[ "$BACKOFF" -gt "$REMAINING" \]' "$ACTION_YML"; then + echo "PASS: the honored Retry-After cannot outlive the deadline" +else + echo "FAIL: the honored Retry-After is no longer clamped to the deadline" + failures=$((failures+1)) +fi + +# 19. A throttled give-up must not be reported as an unreadable report — that # sends the customer to look at their scan instead of their rate limit. if [ "$(grep -c 'status=rate-limited' "$ACTION_YML" || true)" -ge 2 ]; then echo "PASS: both give-up paths distinguish rate-limited from unreadable" From 82df35b94b4355352d36c24f9e62055c1e7b53ef Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:30:10 -0700 Subject: [PATCH 3/6] fix: a repeated Retry-After is not a delay, in all three surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .github/workflows/test-github-action.yml | 49 +++++++++++++++++++ github-action/action.yml | 19 +++++-- github-action/tests/mock-rafter-api.py | 7 ++- node/src/commands/backend/scan-status.ts | 9 +++- node/tests/scan-poll-429-retry-after.test.ts | 12 ++++- .../tests/test_scan_poll_429_retry_after.py | 6 +++ shared-docs/CLI_SPEC.md | 2 +- 7 files changed, 94 insertions(+), 10 deletions(-) diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 67a30836..e6c5775e 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -323,6 +323,55 @@ jobs: fi echo "PASS: a 429 with no Retry-After failed fast, without retrying." + test-poll-429-duplicate-retry-after: + name: "Poll: a repeated Retry-After is not a delay" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (429 carrying TWO Retry-After headers) + env: + PORT: '8794' + FAIL_ON: '2' + FAIL_STATUS: '429' + FAIL_RETRY_AFTER: '5|900' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8794/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8794/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8794' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert it treated the ambiguous header as no header + run: | + cat mock.log + # An origin's Retry-After and a proxy's are not a delay anyone can act + # on. Both runtimes answer null here; the shell must not quietly pick + # one and sleep on it. + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: a repeated Retry-After was acted on instead of refused." + exit 1 + fi + gets=$(grep -c '"GET ' mock.log || true) + if [ "$gets" -ne 2 ]; then + echo "FAIL: expected exactly 2 polls (no retry), saw ${gets}." + exit 1 + fi + echo "PASS: a repeated Retry-After counted as absent, and the run failed fast." + test-results-fetch-429-with-retry-after: name: "Results fetch: rides out a 429 that carries Retry-After" runs-on: ubuntu-latest diff --git a/github-action/action.yml b/github-action/action.yml index 2e3d8ac6..af70c0a3 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -200,8 +200,15 @@ runs: # honors literally and hang the job until the runner times out. RETRY_AFTER="" if [ "$HTTP_CODE" -eq 429 ]; then - RETRY_AFTER=$(tr -d '\r' < "$HDR_FILE" | grep -i '^retry-after:' \ - | tail -n1 | sed 's/^[^:]*:[[:space:]]*//' || true) + # Exactly one header, or none we can act on: two Retry-Afters (an + # origin's and a proxy's, say) is not a delay, and the two runtimes + # answer the same way. + RETRY_AFTER=$(tr -d '\r' < "$HDR_FILE" | grep -i '^retry-after:' || true) + if [ "$(printf '%s' "$RETRY_AFTER" | grep -c . || true)" -eq 1 ]; then + RETRY_AFTER=$(printf '%s' "$RETRY_AFTER" | sed 's/^[^:]*:[[:space:]]*//') + else + RETRY_AFTER="" + fi case "$RETRY_AFTER" in ''|*[!0-9]*) RETRY_AFTER="" ;; *) @@ -334,8 +341,12 @@ runs: # Server-controlled, and becomes a sleep duration: digits only, # and length-capped so a 23-digit value cannot slip past the # numeric comparison below and hang the job (see the poll loop). - retry_after=$(tr -d '\r' < "$hdr" | grep -i '^retry-after:' \ - | tail -n1 | sed 's/^[^:]*:[[:space:]]*//' || true) + retry_after=$(tr -d '\r' < "$hdr" | grep -i '^retry-after:' || true) + if [ "$(printf '%s' "$retry_after" | grep -c . || true)" -eq 1 ]; then + retry_after=$(printf '%s' "$retry_after" | sed 's/^[^:]*:[[:space:]]*//') + else + retry_after="" + fi case "$retry_after" in ''|*[!0-9]*) retry_after="" ;; *) diff --git a/github-action/tests/mock-rafter-api.py b/github-action/tests/mock-rafter-api.py index ecf3f151..1333dc48 100644 --- a/github-action/tests/mock-rafter-api.py +++ b/github-action/tests/mock-rafter-api.py @@ -22,7 +22,9 @@ FAIL_RETRY_AFTER when set, injected failures carry this literal Retry-After header value. sable-96ex: a 429 is retried ONLY when one is present, so setting/omitting this is what separates the two - halves of that contract. + halves of that contract. A '|' splits it into SEVERAL + Retry-After headers ("5|900"), which must count as no usable + delay in every surface. FAIL_FOREVER if "1", every GET from FAIL_ON onward fails (persistent case) FAIL_COUNT how many consecutive GETs fail starting at FAIL_ON (default 1; ignored when FAIL_FOREVER is set) @@ -58,7 +60,8 @@ def _send(self, code, payload, retry_after=None): self.send_header("Content-Type", "application/json") self.send_header("Content-Length", str(len(body))) if retry_after is not None: - self.send_header("Retry-After", retry_after) + for value in retry_after.split("|"): + self.send_header("Retry-After", value) self.end_headers() self.wfile.write(body) diff --git a/node/src/commands/backend/scan-status.ts b/node/src/commands/backend/scan-status.ts index f8604d14..d334fe87 100644 --- a/node/src/commands/backend/scan-status.ts +++ b/node/src/commands/backend/scan-status.ts @@ -68,7 +68,14 @@ export function retryAfterMs(e: any): number | null { } } } - if (Array.isArray(raw)) raw = raw[0]; + // Two Retry-Afters — an origin's and a proxy's, say — is not a delay we can + // act on, so it counts as absent like any other unusable value. Python gets + // this for free (requests joins duplicates as "5, 900", which fails the digit + // test) and bash counts the header lines; all three must answer the same. + if (Array.isArray(raw)) { + if (raw.length !== 1) return null; + raw = raw[0]; + } if (raw === undefined || raw === null) return null; const text = String(raw).trim(); if (!/^\d+$/.test(text)) return null; diff --git a/node/tests/scan-poll-429-retry-after.test.ts b/node/tests/scan-poll-429-retry-after.test.ts index ba15b90d..d18e4b4d 100644 --- a/node/tests/scan-poll-429-retry-after.test.ts +++ b/node/tests/scan-poll-429-retry-after.test.ts @@ -106,8 +106,16 @@ describe("retryAfterMs", () => { expect(retryAfterMs({ response: { status: 429, headers } })).toBe(9000); }); - it("takes the first value when the header repeats", () => { - expect(retryAfterMs(throttled(["4", "900"]))).toBe(4000); + it("reads a single-element array (one header, array-shaped)", () => { + expect(retryAfterMs(throttled(["4"]))).toBe(4000); + }); + + it("refuses a repeated header rather than picking one", () => { + // An origin's Retry-After and a proxy's are not a delay we can act on, and + // the three surfaces must agree: Python sees requests' joined "4, 900" and + // fails the digit test, bash counts the header lines, this returns null. + expect(retryAfterMs(throttled(["4", "900"]))).toBeNull(); + expect(retryAfterMs(throttled("4, 900"))).toBeNull(); }); it("honors Retry-After: 0", () => { diff --git a/python/tests/test_scan_poll_429_retry_after.py b/python/tests/test_scan_poll_429_retry_after.py index e936fff7..8f599d34 100644 --- a/python/tests/test_scan_poll_429_retry_after.py +++ b/python/tests/test_scan_poll_429_retry_after.py @@ -100,6 +100,12 @@ def test_returns_none_for_the_http_date_form(self): # in bash on whatever `date` the runner ships. Unparseable = fail fast. assert retry_after_seconds(_throttled("Wed, 21 Oct 2026 07:28:00 GMT")) is None + def test_refuses_a_repeated_header(self): + # requests joins duplicate headers with ", ". An origin's Retry-After + # and a proxy's are not a delay we can act on, and all three surfaces + # must agree — see the Node and action.yml halves of this contract. + assert retry_after_seconds(_throttled("4, 900")) is None + def test_returns_none_for_negative_or_non_numeric(self): assert retry_after_seconds(_throttled("-5")) is None assert retry_after_seconds(_throttled("soon")) is None diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index ad8b32f6..e6855923 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -148,7 +148,7 @@ After either budget is exhausted the command exits `1`. If the failures reached **The 429 rule, in full.** A 429 is ambiguous in this API: on scan *submit* it means the account is out of credits (exit `3`), and on the poll endpoint it would mean the client is going too fast. `Retry-After` is the only thing that tells the two apart — a rate limiter sends one, a quota rejection does not — so it decides whether the poll retries. Submit is unchanged either way: a 429 there is exit `3` even when it carries a `Retry-After`. -- Only the **delay-seconds** form is honored (`Retry-After: 30`). The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. +- Only the **delay-seconds** form is honored (`Retry-After: 30`), and only when the header appears exactly once. A repeated `Retry-After` — an origin's and a proxy's, say — is not a delay anyone can act on, so it counts as absent in all three surfaces. The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. - The honored wait is capped at **60 seconds**, so a limiter cannot park a CI job for an hour. A 429 retry still spends a failure from the same budget as any other transient failure, so an endlessly-throttling API cannot keep the loop alive. - Giving up on repeated 429s produces a **rate-limited** message, not the unreadable-report one: it says the API throttled us, names the scan, and points at `rafter get` and the dashboard. Telling a throttled customer their report could not be read sends them to look at the wrong thing. From 5ef4ca58682399bafc93787168c61553d0519785 Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:37:37 -0700 Subject: [PATCH 4/6] fix: three findings from the security review of the 429 path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ５ 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. --- .github/workflows/test-github-action.yml | 101 ++++++++++++++++++ github-action/action.yml | 32 +++--- github-action/tests/mock-rafter-api.py | 15 +++ .../tests/test-action-yml-defaults.sh | 13 ++- python/rafter_cli/commands/backend.py | 9 +- .../tests/test_scan_poll_429_retry_after.py | 17 +++ shared-docs/CLI_SPEC.md | 3 +- 7 files changed, 172 insertions(+), 18 deletions(-) diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index e6c5775e..a118b4c5 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -372,6 +372,107 @@ jobs: fi echo "PASS: a repeated Retry-After counted as absent, and the run failed fast." + test-poll-429-early-hints-retry-after: + name: "Poll: a hinted Retry-After is not the 429's own" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (103 Early Hints carries Retry-After, the 429 does not) + env: + PORT: '8795' + FAIL_ON: '2' + FAIL_STATUS: '429' + EARLY_HINTS_RETRY_AFTER: '7' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8795/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8795/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8795' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the hinted header was not read as the 429's + run: | + cat mock.log + # `curl -D` dumps EVERY header block it received. Reading Retry-After + # from the file without scoping it to the last block turns a bare 429 + # into a retried one — the single thing this branch exists to prevent. + # Verified as non-vacuous: with a `grep | tail -n1` extraction this + # mock config sleeps 7s and completes with 6 polls. + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: an Early Hints Retry-After was honored for a bare 429." + exit 1 + fi + gets=$(grep -c '"GET ' mock.log || true) + if [ "$gets" -ne 2 ]; then + echo "FAIL: expected exactly 2 polls (no retry), saw ${gets}." + exit 1 + fi + echo "PASS: only the final response's Retry-After counted." + + test-poll-429-absurd-retry-after: + name: "Poll: an absurd Retry-After cannot hang the job" + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (429 forever, Retry-After of 23 digits) + env: + PORT: '8796' + FAIL_ON: '2' + FAIL_STATUS: '429' + FAIL_FOREVER: '1' + FAIL_RETRY_AFTER: '99999999999999999999999' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8796/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8796/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + timeout-minutes: 3 + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8796' + timeout-minutes: '1' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert it ended at the deadline instead of sleeping forever + run: | + cat mock.log + # The value passes the digit guard, so only the LENGTH cap stands + # between it and `sleep 99999999999999999999999`, which does not + # return: `[ 1e23 -gt 60 ]` is a shell error that evaluates false, so + # an uncapped value reaches sleep intact. Asserting the status output + # is what makes this an artifact-level test rather than a grep over + # the file — a hung step writes no status at all. + if [ "${{ steps.scan.outputs.status }}" != "timeout" ]; then + echo "FAIL: expected status=timeout at the deadline, got '${{ steps.scan.outputs.status }}'." + exit 1 + fi + echo "PASS: the honored delay stayed capped and the deadline held." + test-results-fetch-429-with-retry-after: name: "Results fetch: rides out a 429 that carries Retry-After" runs-on: ubuntu-latest diff --git a/github-action/action.yml b/github-action/action.yml index af70c0a3..e260ae71 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -200,15 +200,18 @@ runs: # honors literally and hang the job until the runner times out. RETRY_AFTER="" if [ "$HTTP_CODE" -eq 429 ]; then - # Exactly one header, or none we can act on: two Retry-Afters (an - # origin's and a proxy's, say) is not a delay, and the two runtimes - # answer the same way. - RETRY_AFTER=$(tr -d '\r' < "$HDR_FILE" | grep -i '^retry-after:' || true) - if [ "$(printf '%s' "$RETRY_AFTER" | grep -c . || true)" -eq 1 ]; then - RETRY_AFTER=$(printf '%s' "$RETRY_AFTER" | sed 's/^[^:]*:[[:space:]]*//') - else - RETRY_AFTER="" - fi + # The LAST header block only, and only if it names Retry-After + # exactly once. `curl -D` dumps every block it received, so a 103 + # Early Hints block carrying a Retry-After would otherwise be read + # as the 429's own — and a bare 429 would be retried, which is the + # one thing this branch exists to prevent. Two Retry-Afters (an + # origin's and a proxy's, say) is not a delay either; both runtimes + # answer the same way. Resetting at each status line is what makes + # this the final response's header, and it needs no `tac`. + RETRY_AFTER=$(tr -d '\r' < "$HDR_FILE" | awk ' + /^[Hh][Tt][Tt][Pp]\// { n = 0; v = ""; next } + /^[Rr][Ee][Tt][Rr][Yy]-[Aa][Ff][Tt][Ee][Rr]:/ { n++; v = $0 } + END { if (n == 1) { sub(/^[^:]*:[ \t]*/, "", v); print v } }') case "$RETRY_AFTER" in ''|*[!0-9]*) RETRY_AFTER="" ;; *) @@ -341,12 +344,11 @@ runs: # Server-controlled, and becomes a sleep duration: digits only, # and length-capped so a 23-digit value cannot slip past the # numeric comparison below and hang the job (see the poll loop). - retry_after=$(tr -d '\r' < "$hdr" | grep -i '^retry-after:' || true) - if [ "$(printf '%s' "$retry_after" | grep -c . || true)" -eq 1 ]; then - retry_after=$(printf '%s' "$retry_after" | sed 's/^[^:]*:[[:space:]]*//') - else - retry_after="" - fi + # Last header block, named exactly once — see the poll loop. + retry_after=$(tr -d '\r' < "$hdr" | awk ' + /^[Hh][Tt][Tt][Pp]\// { n = 0; v = ""; next } + /^[Rr][Ee][Tt][Rr][Yy]-[Aa][Ff][Tt][Ee][Rr]:/ { n++; v = $0 } + END { if (n == 1) { sub(/^[^:]*:[ \t]*/, "", v); print v } }') case "$retry_after" in ''|*[!0-9]*) retry_after="" ;; *) diff --git a/github-action/tests/mock-rafter-api.py b/github-action/tests/mock-rafter-api.py index 1333dc48..9cd4c556 100644 --- a/github-action/tests/mock-rafter-api.py +++ b/github-action/tests/mock-rafter-api.py @@ -19,6 +19,12 @@ FAIL_ON 1-based GET index that starts failing (default 2) FAIL_STATUS status code to fail with (default 500; 404 exercises the read-after-write-lag branch, 429 the rate-limit branch) + EARLY_HINTS_RETRY_AFTER when set, an injected failure is preceded by a raw + "103 Early Hints" block carrying this Retry-After. `curl -D` + dumps EVERY header block it received, so this is what proves the + client reads the final response's headers and not an earlier + block's — a bare 429 preceded by hinted Retry-After must still + fail fast. FAIL_RETRY_AFTER when set, injected failures carry this literal Retry-After header value. sable-96ex: a 429 is retried ONLY when one is present, so setting/omitting this is what separates the two @@ -46,6 +52,7 @@ #: Sent verbatim, so a test can inject a malformed value ("soon", "-1") and #: check the client refuses to act on it. FAIL_RETRY_AFTER = os.environ.get("FAIL_RETRY_AFTER") +EARLY_HINTS_RETRY_AFTER = os.environ.get("EARLY_HINTS_RETRY_AFTER") COMPLETE_AFTER = int(os.environ.get("COMPLETE_AFTER", str(FAIL_ON))) SCAN_ID = "repro-sable-l10k-0001" @@ -89,6 +96,14 @@ def do_GET(self): if failing: # The verbatim customer-facing body. if FAIL_STATUS == 429: + if EARLY_HINTS_RETRY_AFTER is not None: + # Written raw: BaseHTTPRequestHandler has no informational + # response, and the point is the wire, not the API. + self.wfile.write( + b"HTTP/1.1 103 Early Hints\r\nRetry-After: " + + EARLY_HINTS_RETRY_AFTER.encode() + + b"\r\n\r\n" + ) return self._send( FAIL_STATUS, {"error": "Too many requests"}, diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 991ce575..2bae554d 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -230,7 +230,18 @@ else failures=$((failures+1)) fi -# 19. A throttled give-up must not be reported as an unreadable report — that +# 19. Retry-After must be read from the LAST header block only. `curl -D` dumps +# every block it received, so a `grep | tail -n1` over the whole file reads +# a 103 Early Hints Retry-After as if it were the 429's own — and retries a +# bare 429, the one thing the gate exists to prevent. +if [ "$(grep -c 'n = 0; v = ""; next' "$ACTION_YML" || true)" -eq 2 ]; then + echo "PASS: both loops scope Retry-After to the final response's headers" +else + echo "FAIL: a retry loop no longer scopes Retry-After to the last header block" + failures=$((failures+1)) +fi + +# 20. A throttled give-up must not be reported as an unreadable report — that # sends the customer to look at their scan instead of their rate limit. if [ "$(grep -c 'status=rate-limited' "$ACTION_YML" || true)" -ge 2 ]; then echo "PASS: both give-up paths distinguish rate-limited from unreadable" diff --git a/python/rafter_cli/commands/backend.py b/python/rafter_cli/commands/backend.py index 4215aa94..deef6e7f 100644 --- a/python/rafter_cli/commands/backend.py +++ b/python/rafter_cli/commands/backend.py @@ -2,6 +2,7 @@ from __future__ import annotations import json +import re import sys import time @@ -167,7 +168,13 @@ def retry_after_seconds(resp) -> "int | None": if not isinstance(value, str): return None text = value.strip() - if not text.isdigit(): + # NOT str.isdigit(): it is Unicode-aware and int() is not, so '\xb2' — a + # single raw byte, which http.client decodes as ISO-8859-1 into '²' — passes + # isdigit() and then raises ValueError out of a call site whose only handler + # is requests.RequestException. That is a server-triggered client crash, and + # it is also a divergence: Node's /^\d+$/ and the action's `case *[!0-9]*` + # both reject the same byte. The accept-set must match what int() parses. + if not re.fullmatch(r"[0-9]+", text): return None return min(int(text), MAX_RETRY_AFTER_SECONDS) diff --git a/python/tests/test_scan_poll_429_retry_after.py b/python/tests/test_scan_poll_429_retry_after.py index 8f599d34..1c3dee24 100644 --- a/python/tests/test_scan_poll_429_retry_after.py +++ b/python/tests/test_scan_poll_429_retry_after.py @@ -111,6 +111,23 @@ def test_returns_none_for_negative_or_non_numeric(self): assert retry_after_seconds(_throttled("soon")) is None assert retry_after_seconds(_throttled("1.5")) is None + def test_a_unicode_digit_does_not_crash_the_client(self): + # 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. One raw \xb2 byte in the header + # is enough — http.client decodes headers as ISO-8859-1. Node's + # /^\d+$/ and the action's `case *[!0-9]*` both reject it, so accepting + # it here would be a divergence as well as a crash. + for value in ("\u00b2", "\u00b3", "\u00b9", "\uff15", "\u0665"): + assert retry_after_seconds(_throttled(value)) is None + # And the whole poll survives one, by failing fast rather than raising. + with patch("rafter_cli.commands.backend.api_get") as get: + get.side_effect = [_processing(), _throttled("\u00b2")] + with pytest.raises(typer.Exit) as exc: + _handle_scan_status_interactive("s1", HEADERS, "md", True) + assert exc.value.exit_code == EXIT_GENERAL_ERROR + assert get.call_count == 2 + def test_returns_none_when_absent(self): assert retry_after_seconds(_throttled()) is None assert retry_after_seconds(MagicMock(spec=[])) is None diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index e6855923..55e48413 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -148,7 +148,7 @@ After either budget is exhausted the command exits `1`. If the failures reached **The 429 rule, in full.** A 429 is ambiguous in this API: on scan *submit* it means the account is out of credits (exit `3`), and on the poll endpoint it would mean the client is going too fast. `Retry-After` is the only thing that tells the two apart — a rate limiter sends one, a quota rejection does not — so it decides whether the poll retries. Submit is unchanged either way: a 429 there is exit `3` even when it carries a `Retry-After`. -- Only the **delay-seconds** form is honored (`Retry-After: 30`), and only when the header appears exactly once. A repeated `Retry-After` — an origin's and a proxy's, say — is not a delay anyone can act on, so it counts as absent in all three surfaces. The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. +- Only the **delay-seconds** form is honored (`Retry-After: 30`), and only when the header appears exactly once. A repeated `Retry-After` — an origin's and a proxy's, say — is not a delay anyone can act on, so it counts as absent in all three surfaces. The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. So does a value that is not ASCII digits — Python's `str.isdigit()` accepts `²` where `int()` raises, so the accept-set is an explicit `[0-9]+` in all three surfaces. - The honored wait is capped at **60 seconds**, so a limiter cannot park a CI job for an hour. A 429 retry still spends a failure from the same budget as any other transient failure, so an endlessly-throttling API cannot keep the loop alive. - Giving up on repeated 429s produces a **rate-limited** message, not the unreadable-report one: it says the API throttled us, names the scan, and points at `rafter get` and the dashboard. Telling a throttled customer their report could not be read sends them to look at the wrong thing. @@ -157,6 +157,7 @@ After either budget is exhausted the command exits `1`. If the failures reached - It has no "first poll" distinction: by the time it polls, the trigger step has already returned a `scan_id`, so **every** 404 there is treated as read-after-write lag. A scan id the backend accepted but never persisted therefore fails after the 5-failure budget rather than immediately. - Its poll loop is additionally bounded by a wall-clock deadline derived from `timeout-minutes`. Before v0.11 that input was a poll *count*, so a slow API could overrun it; it is now a real deadline. - Its `status` output is `completed`, `failed`, `timeout`, `unreadable` (the scan may have finished but its report could not be read), `rate-limited` (the API throttled us until the retry budget ran out), or `unreachable` (the API could not be contacted). +- It reads `Retry-After` from the final response's header block only. `curl -D` dumps every block it received, so an informational `103 Early Hints` block carrying the header would otherwise be read as the 429's own — and a bare 429 would be retried. The two runtimes get this from their HTTP client. - It validates `Retry-After` as digits and caps its **length** as well as its value: `[ 1e23 -gt 60 ]` is a shell error that evaluates false, so an uncapped 23-digit header would reach `sleep` intact and hang the job until the runner times out. ### rafter usage [OPTIONS] From 622dac19d920449aa61912b2589cc003ffd07128 Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:38:49 -0700 Subject: [PATCH 5/6] docs: state the worst-case Retry-After waiting bound in CLI_SPEC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- shared-docs/CLI_SPEC.md | 1 + 1 file changed, 1 insertion(+) diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 55e48413..3bad2668 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -150,6 +150,7 @@ After either budget is exhausted the command exits `1`. If the failures reached - Only the **delay-seconds** form is honored (`Retry-After: 30`), and only when the header appears exactly once. A repeated `Retry-After` — an origin's and a proxy's, say — is not a delay anyone can act on, so it counts as absent in all three surfaces. The HTTP-date form is not parsed, in any of the three surfaces, because the composite action has to reach the same verdict in shell on whatever `date` the runner ships — a rule the surfaces cannot state identically is worse than a narrow one they can. A header that cannot be parsed counts as absent, which means fail fast. So does a value that is not ASCII digits — Python's `str.isdigit()` accepts `²` where `int()` raises, so the accept-set is an explicit `[0-9]+` in all three surfaces. - The honored wait is capped at **60 seconds**, so a limiter cannot park a CI job for an hour. A 429 retry still spends a failure from the same budget as any other transient failure, so an endlessly-throttling API cannot keep the loop alive. +- Worst-case waiting, stated rather than implied: the action's poll loop is additionally bounded by its wall-clock deadline (an honored delay is clamped to what is left of it, so the server cannot extend a budget the workflow author set). The action's results fetch has no deadline of its own — three fetches of at most four honored sleeps each is up to ~12 minutes if every one of them eventually succeeds, and ~4 minutes before the first fetch gives up. That is deliberate: a build whose report was retrievable after a real throttle should get the report. - Giving up on repeated 429s produces a **rate-limited** message, not the unreadable-report one: it says the API throttled us, names the scan, and points at `rafter get` and the dashboard. Telling a throttled customer their report could not be read sends them to look at the wrong thing. **The composite GitHub Action** (`github-action/action.yml`) implements the same classification in both its poll loop and its results fetch, with these differences forced by the shell: From 535ef46de1a3807da358583ce7fe0e6773b395fc Mon Sep 17 00:00:00 2001 From: raftercli/crew/achebe Date: Wed, 2 Sep 2026 12:39:34 -0700 Subject: [PATCH 6/6] =?UTF-8?q?docs:=20CHANGELOG=20=E2=80=94=20name=20the?= =?UTF-8?q?=20four=20ways=20a=20Retry-After=20counts=20as=20absent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 471bfa56..6e8a4fd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **A 429 during scan polling is retried when it carries `Retry-After`** (sable-96ex). Polling every 10 seconds from every customer repo is exactly the traffic shape a rate limiter targets, and all three surfaces treated a 429 as fatal — 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. A 429 cannot simply join 5xx either: on scan *submit* it means the account is out of credits. `Retry-After` is the disambiguator — a limiter sends one, a quota rejection does not — so the poll and results paths now retry a 429 that carries one, sleeping `min(Retry-After, 60s)` against the same failure budget as any other transient failure, and still fail fast on a bare 429. Submit is unchanged: exit `3`, even with the header. Giving up on repeated 429s now says the API rate limited us rather than blaming the report. Delay-seconds only; the HTTP-date form counts as absent. Full contract in `shared-docs/CLI_SPEC.md`. +- **A 429 during scan polling is retried when it carries `Retry-After`** (sable-96ex). Polling every 10 seconds from every customer repo is exactly the traffic shape a rate limiter targets, and all three surfaces treated a 429 as fatal — 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. A 429 cannot simply join 5xx either: on scan *submit* it means the account is out of credits. `Retry-After` is the disambiguator — a limiter sends one, a quota rejection does not — so the poll and results paths now retry a 429 that carries one, sleeping `min(Retry-After, 60s)` against the same failure budget as any other transient failure, and still fail fast on a bare 429. Submit is unchanged: exit `3`, even with the header. Giving up on repeated 429s now says the API rate limited us rather than blaming the report. Delay-seconds only, ASCII digits only, and only when the final response names the header exactly once — the HTTP-date form, a value `int()` cannot parse, a header repeated by a proxy, and one carried on a `103 Early Hints` block all count as absent, which means fail fast. Full contract in `shared-docs/CLI_SPEC.md`. - **`timeout-minutes` on the GitHub Action is now a wall-clock deadline**, not a poll count. Previously the action ran `timeout-minutes * 6` polls, each costing 10s *plus* API latency, so a slow API pushed real elapsed time past the documented budget. It is now enforced as a real deadline. **This can fail workflows that were relying on the overrun** — if a scan sits near the boundary, raise `timeout-minutes`. - `rafter get ` (without `--interactive`) now retries transient failures too. It is the command the poll loop's give-up message recommends, so a remedy defeated by the same transient failure it is recommended for was not a remedy. - HTTP requests on the poll and results paths now carry connect/read timeouts (`--connect-timeout 10 --max-time 60` for curl, 30s for axios), so a hung server cannot stall inside a request that the retry loop only checks between attempts.