Repository navigation
feat(http): max_throttle_retries for 429s that carry Retry-After - #135
Ankur Gupta (ankur-pattern) wants to merge 2 commits into
Conversation
| if delay, ok := retryAfter(response); ok && throttled < h.MaxThrottleRetries { | ||
| throttled++ | ||
| attempt-- | ||
| time.Sleep(delay) |
There was a problem hiding this comment.
The throttle path sleeps for whatever Retry-After the server sends, with no upper bound, and up to max_throttle_retries times. With the suggested max_throttle_retries: 120, a single upstream (or a proxy in front of it) answering Retry-After: 300 would hang this record for about 10 hours before failing, and time.Sleep cannot be interrupted. Before this change the same uncapped sleep could only happen max_retries times. Nothing is logged either, so a stalled task gives no sign that it is being throttled.
Fix: cap the per-throttle delay (a constant, or a max_throttle_delay option), or fall through to the normal max_retries path when delay is above the cap. A debug or info log per throttle retry would also help.
There was a problem hiding this comment.
Fixed in b0898d0. Each throttle wait is now capped at 60s (maxThrottleDelay). A Retry-After above that skips the throttle budget and takes the normal max_retries path, so the worst case per call is max_throttle_retries × 60s instead of unbounded. Every throttle retry logs http: 429 throttle retry n/N in <delay>. The URL is left out of the log line so query params don't end up in task logs. Added a test case for a Retry-After above the cap, and the README documents the 60s limit. The PR body example now uses 60, the value data-airflow sets, not 120.
|
Withdrawn: we don't need max_throttle_retries. |
Summary
Adds
max_throttle_retriesto thehttptask: extra attempts for429responses that carry aRetry-Afterheader. Each one sleeps forRetry-Afterand doesn't count againstmax_retries. The default is0, so existing pipelines behave exactly as before.Also fixes a small bug: a
429whoseRetry-Aftercouldn't be parsed used to retry with no sleep at all. It now falls back to the existing2^attemptbackoff.Why
The SP-API pipelines in data-airflow (catalog search and competitive pricing) call Amazon through rate-gate. Rate-gate keeps a token bucket per seller account (2 calls a second for catalog, 0.5 for pricing). When the bucket is empty it answers
429withRetry-After: 1or2andX-Intercepted-By: rate-gate. Those 429s share themax_retries: 5budget with every other error, so a busy account fails its chunks within seconds instead of making them wait.On 2026-10-28 the SERP catalog run had 517 of 548 chunks running at once, 8 to 12 per account. 246 chunks needed an Airflow retry, and 14 reached try 4 or 5; try 5 failing would have blocked publish. Raising
max_retriesitself isn't the fix, because it also stretches Amazon's own 429 backoff (2^attempt, uncapped). Amazon's 429s don't sendRetry-After, so they keep usingmax_retries.With this change a pipeline can set, for example,
max_throttle_retries: 60. Oversubscribing an account then slows its chunks down instead of failing them.Changes
http/http.go: newMaxThrottleRetriesfield, copied innewFromInput. Incall, a throttled 429 within budget sleepsRetry-Afterand retries without using an attempt. Each throttle wait is capped at 60s (maxThrottleDelay); a longerRetry-Aftertakes the normalmax_retriespath, so the budget can't stall a record for hours. Each throttle retry is logged.http/retry.go:retryAfter(response)parsesRetry-After(seconds) for 429s.handleBackoffuses it and falls back to2^attemptwhen it's missing or unparseable.http/README.md: documents the field.http/retry_test.go: tests against anhttptestserver covering throttles beyondmax_retries, no budget, both budgets used up, a missingRetry-After, an unparseableRetry-After, and aRetry-Afterabove the cap.Testing
go vet -copylocks=false ./...andgo test -race ./...pass locally, the same as CI.Rollout
ECS ingest tasks run the image
caterpillar:manual-latest(heimdall-config/configs/cp_task_def.json.tmpl), so that image needs a rebuild after this merges. YAML parsing isn't strict, so data-airflow can addmax_throttle_retriesbefore then; it's ignored until the new binary runs.