fix(acp): honour the relay's retry hint on a rate-limited REST call - #5724
Open
Chessing234 wants to merge 2 commits into
Open
fix(acp): honour the relay's retry hint on a rate-limited REST call#5724Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
The HTTP bridge path backed off on its own schedule (500ms / 1s / 2s) and threw away what the relay said. The relay's admission limiter computes exactly how long the principal must wait and spends it in the 429 body - "rate-limited: quota exceeded; retry in 4s" - so a harness that retries at 2s is guaranteed another rejection, and re-arriving early keeps its own limit window alive. The subscription path already reads that hint via parse_rate_limit_retry_secs and parks until it clears; the REST path did not. Read it off the retriable response and wait at least that long, capped at 30s so a pathological value cannot park the harness. A shorter hint never shortens the existing backoff, and a response with no hint keeps today's schedule exactly. Refs block#5557 Signed-off-by: Taksh <takshkothari09@gmail.com>
Five cases: no hint leaves the schedule alone, a longer hint wins, a shorter one does not shorten the backoff, a pathological one is capped, and the relay's actual 429 body parses end to end into a delay. The last one pins the wire shape against crates/buzz-relay/src/api/bridge.rs, since the hint arrives as prose inside a JSON error field. Refs block#5557 Signed-off-by: Taksh <takshkothari09@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.
Partial fix for #5557 — one of the two mechanisms the report named, and I want to be clear up front about which.
What I found
The report offered two candidate mechanisms and said it could not tell them apart from outside. From inside the code, "no backoff on retry" is not it:
request_with_retry(crates/buzz-acp/src/relay.rs) already sleeps 500ms / 1s / 2s with ±20% jitter between attempts. Sub-millisecond gaps at 20–42/sec cannot come from that loop, so the observed storm is fan-out — many concurrent requests each getting their own 429 — which this PR does not fix.What it does fix is a real gap on the same path. The relay computes exactly how long a rate-limited principal must wait (
enforce_http_admission,crates/buzz-relay/src/api/bridge.rs) and spends it in the 429 body:The subscription path already reads that hint —
parse_rate_limit_retry_secsarms a gate and parks until it clears. The REST path parsed nothing and backed off on its own schedule, which tops out at 2s. Against a 4s hint every retry is a guaranteed second rejection, and a client that re-arrives inside its own window keeps that window alive. That is a self-sustaining component of the loop even if it is not the whole amplifier.The change
Read the hint off the retriable response with the parser the WebSocket path already uses, and wait at least that long before the next attempt:
Not in this PR
Also worth noting:
buzz-clicarries its own private copy of this parser (parse_retry_hint_textincrates/buzz-cli/src/client.rs). I left it alone rather than widen this diff into another crate, but the two could collapse onto a shared helper if you want that.Verification
cargo fmt --all --check,cargo clippy -p buzz-acp --all-targets(clean),cargo test -p buzz-acp --lib— 778 pass, including 5 new tests. The last one parses the relay's real 429 body end to end into a delay, pinning the wire shape. I did not reproduce the live rate-limit storm, so the claim I can stand behind is the delay choice, not a measured reduction in 429 volume.Refs #5557