fix(acp): coordinate HTTP rate-limit retries across RestClient clones - #5620
Open
dbett4 wants to merge 1 commit into
Open
fix(acp): coordinate HTTP rate-limit retries across RestClient clones#5620dbett4 wants to merge 1 commit into
dbett4 wants to merge 1 commit into
Conversation
Signed-off-by: Dave Bettner <dbett4@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RestClient::request_with_retrytreats HTTP 429 as a generic transient error —is_retriable_statusgroups it with 502/503/504 — so a rate-limited request retries on the fixed 500ms/1s/2s ladder and gives up after ~3.5s total. Two consequences:Retry-Afteris never read. A relay that answersRetry-After: 30sees four attempts land inside its own limit window, and the call still fails.RestClientisClone, holds no rate-limit state, and is Arc-shared with spawned prompt tasks by design. Concurrent callers each run the ladder independently, so one caller's 429 does not slow any other — a limit produces a retry storm instead of a drain.The WebSocket path in this same file already handles this properly:
BgState::rate_limit_gateandrate_limited_pendingpark admission-counted frames until the gate clears. This change gives the HTTP bridge path equivalent behavior.Change
HTTP_RATE_GATEshared by everyRestClientclone. A 429 arms or extends it; a shorter hint never shortens an already-armed gate.Retry-Afterparsed as integer seconds, floored to 5s when absent/unparseable/< 2sand capped at 300s, with the existing jitter applied.HTTP_BRIDGE_PERMITSserializes bridge admission so a request already queued re-checks the gate after a peer arms it, rather than racing past it.Tests
parse_http_retry_after_secs_valid_and_invalid— integer seconds honored; HTTP-date and malformed values fall through to the conservative default.http_rate_gate_extends_without_shorteningandhttp_rate_gate_missing_retry_after_uses_default— gate arithmetic.concurrent_rest_client_clones_share_http_rate_gate— wiremock server, four concurrent clones, first response429+Retry-After: 2. Asserts all four calls succeed, total upstream hits stay within2..=6, and — the assertion that actually does the work — that the second upstream hit lands at least 1500ms after the first, i.e. the queued clone waited out the hint rather than racing past it.Verified as a detector, not just a passing test: with the gate wait, the permit acquisition, and the 429 arm removed from
request_with_retry(helpers and tests left intact, so the only delta is the fix itself), this test fails onthe first queued clone bypassed the shared Retry-After gate. It is not green-by-default.cargo test -p buzz-acpon this branch: 740 passed, 0 failed (plus 9 inpool_lifecycle_state).Notes for review
wiremockas a dev-dependency; theCargo.lockdelta is entirely that.HTTP_BRIDGE_PERMITSisSemaphore::const_new(1), so bridge requests serialize process-wide even when no gate is armed. That is the deliberate trade-off here — it is what lets a queued request observe a gate armed by a peer instead of racing past it — but it is a real throughput cost on the happy path and the part I would most expect pushback on. The alternative is a wider permit count with a re-check after acquisition, accepting a small stampede window on the first 429. Happy to switch if you prefer that shape.