diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 7a7665d8..2cde2923 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -404,6 +404,295 @@ jobs: [ "$FAIL" -eq 0 ] && echo "PASS: counts reached the gate and the gate failed the build." exit $FAIL + # 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: '8801' + 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:8801/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8801/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:8801' + 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: '8802' + 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:8802/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8802/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:8802' + 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-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: '8804' + 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:8804/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8804/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:8804' + 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-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: '8805' + 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:8805/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8805/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:8805' + 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: '8806' + 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:8806/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8806/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:8806' + 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 + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (poll succeeds, first results fetch 429s) + env: + PORT: '8803' + 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:8803/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8803/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:8803' + 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..6e8a4fd4 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, 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. diff --git a/github-action/action.yml b/github-action/action.yml index 8fe5ee3c..170dceaf 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,51 @@ 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 + # 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="" ;; + *) + 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 +246,21 @@ 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 + # ...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})" + fi sleep "$BACKOFF" POLL_COUNT=$((POLL_COUNT+1)) continue @@ -254,25 +314,53 @@ 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). + # 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="" ;; + *) + 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 +368,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 +390,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 23af6cc5..ad9f99fb 100644 --- a/github-action/tests/mock-rafter-api.py +++ b/github-action/tests/mock-rafter-api.py @@ -18,7 +18,19 @@ 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) + 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 + 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) @@ -50,6 +62,10 @@ 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") +EARLY_HINTS_RETRY_AFTER = os.environ.get("EARLY_HINTS_RETRY_AFTER") COMPLETE_AFTER = int(os.environ.get("COMPLETE_AFTER", str(FAIL_ON))) RESULTS_SHAPE = os.environ.get("RESULTS_SHAPE", "ok") SHAPE_FROM = int(os.environ.get("SHAPE_FROM", str(COMPLETE_AFTER + 1))) @@ -68,13 +84,18 @@ class Handler(BaseHTTPRequestHandler): - def _send(self, code, payload): - self._send_raw(code, json.dumps(payload).encode(), "application/json") + def _send(self, code, payload, retry_after=None): + self._send_raw( + code, json.dumps(payload).encode(), "application/json", retry_after + ) - def _send_raw(self, code, body, content_type): + def _send_raw(self, code, body, content_type, retry_after=None): self.send_response(code) self.send_header("Content-Type", content_type) self.send_header("Content-Length", str(len(body))) + if retry_after is not None: + for value in retry_after.split("|"): + self.send_header("Retry-After", value) self.end_headers() self.wfile.write(body) @@ -115,6 +136,20 @@ 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"}, + retry_after=FAIL_RETRY_AFTER, + ) return self._send( FAIL_STATUS, {"error": "Failed to fetch report from storage: Object not found"}, @@ -139,5 +174,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 d68b4462..ce1f7c47 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -224,6 +224,93 @@ 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. + +# 18. 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 + +# 19. 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 + +# 20. 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 + +# 21. 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 + +# 22. ...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 + +# 23. 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 + +# 24. 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..d334fe87 100644 --- a/node/src/commands/backend/scan-status.ts +++ b/node/src/commands/backend/scan-status.ts @@ -33,6 +33,56 @@ 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; + } + } + } + // 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; + // 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 +102,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 +166,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 +212,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 +237,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 +287,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 +332,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..d18e4b4d --- /dev/null +++ b/node/tests/scan-poll-429-retry-after.test.ts @@ -0,0 +1,300 @@ +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("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", () => { + // 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..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 @@ -109,6 +110,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 +151,51 @@ 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() + # 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) + + +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 +256,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 +290,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 +340,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 +358,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 +373,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..1c3dee24 --- /dev/null +++ b/python/tests/test_scan_poll_429_retry_after.py @@ -0,0 +1,233 @@ +"""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_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 + 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 + + +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 ba6e0707..debc2286 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,12 +146,21 @@ 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`), 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: - 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). - Its results step validates the payload **before** counting. A body with no `vulnerabilities` array — not JSON, a `200` carrying an error object, or a parseable payload missing the key — is `status=unreadable`, the job fails, and **no count outputs are written**: a consumer reading `findings-count` sees an empty string, never a fabricated `0`. A report the action cannot read is not a clean scan. An empty array is a clean scan and counts as `0`. +- 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]