fix(relay,acp,cli): rate-limit overhaul — ephemeral kinds off the Messages quota, limit_type discriminator, client retry fixes - #4912
Open
tlongwell-block wants to merge 6 commits into
Open
Conversation
Ephemeral kinds (20000-29999) are never persisted — buzz-db rejects them
with EphemeralEventRejected — yet WS admission billed every EVENT frame
against the per-minute Messages budget regardless of kind. An agent's
telemetry alone (observer 90/min + typing + presence ≈ 111/min vs the
120/min standard-agent budget) could exhaust the durable-message quota
without a single durable write. Admission now uses the same
is_ephemeral() range check the storage layer and the EVENT handler's
scope gate already use: ephemeral frames ride on the WsEvents budget
alone.
Also name the exhausted budget in quota rejections, on both transports:
the NOTICE/CLOSED/429 text becomes
'rate-limited: quota exceeded ({msg|ws|api}); retry in {N}s' and the
buzz_admission_rejections_total quota counter gains a limit_type label.
The 'rate-limited:' prefix and 'retry in {N}s' hint that all clients
parse by substring are unchanged. This is the discriminator needed to
attribute short-TTL denials (WsEvents' 5s window vs Messages' 60s
window overlap below 5s) that field logs could not distinguish.
Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Two defects in the ACP client's backoff jitter, one arithmetic and one
structural.
The arithmetic defect: jitter divided subsec_nanos() (max 999,999,999)
by u32::MAX (4,294,967,295), a value 4.295x larger than the input domain
can ever reach. The "+/-20%" factor was therefore confined to
[0.8, 0.8931] and never reached 1.0. Two consequences: every backoff was
shortened, never lengthened, contradicting the comment on its own line;
and the anti-thundering-herd stagger spanned 9.31% of base instead of
the intended 40%, so retrying clients bunched 4.295x more tightly than
designed. lib.rs carried a third hand-rolled copy with the same shape
and comment but the correct /1e9 divisor -- the typo is proven by its
own sibling.
Rather than patch the two broken copies in place, the three hand-rolled
implementations are consolidated into one backoff module. The divergence
between them was the root cause; a single definition is what stops it
recurring.
The structural defect: the rate-limit gate applied symmetric jitter to
the relay's "retry in {N}s" hint. That hint is the relay's authoritative
window TTL, so shortening it wakes the client inside a window the relay
has already declared closed -- earning a fresh denial and burning a
counter increment for no chance of success. The gate now uses a
one-sided factor in [1.0, 1.2), so it can only ever arm at or beyond the
hint. This is enforced by construction rather than by a check at the
call site: an early gate is unrepresentable.
Testing notes, since two of these tests exist to kill specific mutants:
The gate computation is extracted into a pure gate_delay(retry_secs,
jitter_nanos), with the wall clock acquired by the caller. Entropy as a
parameter is what allows the jitter endpoints to be driven rather than
sampled. An earlier version of this change injected the sample only at
the call site and swept it in a loop; that test was flaky against a
symmetric-jitter mutant, because a mutant that ignores the parameter
reads the clock instead, and a tight loop re-reads the same instant
(measured: 101 iterations span 1-4 distinct subsec_nanos values). The
sweep was a single draw wearing a sweep's costume.
The discriminating assertion checks the ceiling at nanos = 999_999_999,
not a range. A bound of the form 0.8*base <= d < 1.2*base is satisfied
by both the correct and the broken divisor, because the broken range is
a strict subset of the asserted one. The upper bound is inclusive:
Duration is nanosecond-resolution, so 2s * 1.1999999998 rounds to
exactly 2.4s and a strict < would be unsatisfiable.
Mutation results: reintroducing the u32::MAX divisor is killed 3/3 by
the backoff ceiling tests. Swapping the gate's one-sided factor for the
symmetric one, inside the seam, is killed 10/10 by the endpoint test.
A mutation that bypasses the seam entirely is a structural change these
unit tests do not claim to kill; unused_variables on the parameter and
the integration test cover that case.
Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…window
The CLI honoured a relay `retry in {N}s` hint with
`Duration::from_secs(s.min(30))` — no lower bound, and an upper bound
below the window it is meant to outlast. Both ends were wrong, in the
same direction: they made the client retry sooner than the relay asked.
No floor meant a `retry in 0s` hint slept zero and retried instantly.
The relay emits the hint precisely to stop that, and each early attempt
is not free: it is denied again and still costs a counter increment, so
a zero-length sleep converts one rate-limited client into the storm the
hint exists to prevent. Sub-second hints now floor to RETRY_IN_MIN_SECS.
The 30s cap sat below the relay's longest quota window. Hints of 51s
have been observed in production against a 60s window, so any hint above
30s was silently truncated and the client woke inside a window it had
been told to sit out — a guaranteed wasted attempt, and another
increment. The cap is now 90s, above the window with margin. These
sleeps happen between requests, so the cap is independent of the
per-request BUZZ_TIMEOUT_SECS budget.
Both 429 paths (with_retry_body and the moderation-command path) shared
the same open-coded expression; they now share one pure
clamp_retry_hint_secs, so the policy cannot drift between them.
The existing parse_retry_in_zero_seconds test is kept as-is: a parser
returning Some(0) for `retry in 0s` is correct, since it reports what
the relay said. That test was not pinning the defect — the missing
clamp was. Its docstring now points at the clamp test so the division of
responsibility is not rediscovered later.
Mutation results: removing the floor (reverting to `.min(MAX)`) fails
the clamp test. Restoring the 30s cap fails to compile, caught by the
const assertion that the cap outlasts a 60s window; that assertion pins
the property rather than the literal, so the cap can be retuned without
silently reintroducing the defect.
Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
added 3 commits
August 5, 2026 12:17
…ilent The setup nudge is a durable kind:9 reply, but it was published through the WS path, which is built for ephemera. While the rate-limit gate is armed, or while disconnected, that path drops any non-observer publish and returns Ok: the command is queued and the caller never learns what happened to it. The nudge was therefore lost while being logged as "nudge published" — the user asked how to set the agent up and got nothing back. Dedup made the loss permanent. should_nudge_for_event recorded the event id by insert-on-check, before the publish was attempted, so the retry on the next delivery of the same event was suppressed by an entry written for a message that was never sent. One dropped nudge meant no nudge ever. Both halves are fixed by subtraction rather than new machinery: The nudge now goes through RestClient::submit_event, joining the four other durable publishes on the HTTP bridge. The rest_client was already in scope at the call site. submit_event surfaces the relay's actual response, so a refusal is an Err. The gate stops mutating: it takes &HashSet and only checks. The caller inserts the id in the Ok arm of the publish, so a nudge that failed to send stays retryable. An earlier draft of this change instead widened the WS drop guard from kind == KIND_AGENT_OBSERVER_FRAME to is_ephemeral(kind). That is wrong, and the comment now says why: observer frames are themselves range-ephemeral, so testing is_ephemeral would route them into the drop path and undo the parking directly above it. The boundary is encoded as a debug_assert instead. Enumerating the callers shows it holds: presence (20001), typing (20002), observer frames (24200, parked before the assert), and HarnessRelay::publish_event, which has no in-repo callers. That assertion immediately caught publish_during_replay_pacing_is_sent_ on_live_socket publishing a kind:1 through the publish path. It was a fixture artifact, not a defect — the test asserts on the event id and never on the kind, so it reached for the generic make_test_event. The fixture now builds a typing indicator, and both helpers' docs say which path they belong to. Mutation results. Reverting the dedup insert back into the gate is killed by test_failed_nudge_remains_retryable; note the pre-existing test_same_event_id_twice_nudges_exactly_once survives it, so the new test is the discriminator rather than decoration. Reverting the transport to publisher.publish_event initially survived the entire suite: the dedup tests are pure and structurally cannot see which transport is used. That is what the three transport tests are for -- they drive publish_setup_nudge against a real socket, and they kill that mutant 3/3. Removing the debug_assert is caught by restoring the kind:1 fixture. Co-authored-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz> Signed-off-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
The HTTP bridge's retry ladder treated a 429 as a generic transient
failure. It retried on its own rungs — 500ms, 1s, 2s — and never read
the `retry in {N}s` hint out of the response body, even though the relay
puts one there on every quota rejection and the client already has a
parser for it, used on the WS path.
Every rung is shorter than any window the relay would name. A 429 with a
51s hint was retried three times inside that window, and all three were
refused: the limiter's INCR runs on denied checks too, so each one made
the caller's own situation slightly worse while it waited. The ladder
exhausted in about 3.5s and the call failed anyway.
request_with_retry now parses the hint from a 429 body and passes it to
a new pure rest_retry_delay(rung, hint_secs, jitter_nanos), which takes
whichever of the rung and the hint is longer. The hint is extended with
the one-sided jitter added in the earlier commit, so jitter can never
pull the wake-up back inside the window the relay named; an absent or
sub-rung hint keeps the ladder rung and its symmetric jitter, which is
correct for a self-chosen delay. A hint is capped at REST_RETRY_HINT_MAX
(90s) so a pathological value cannot park a caller for hours, with the
same `const` assertion the CLI cap carries: the property pinned is that
the cap outlasts the relay's 60s window, not the number.
Only a 429 is read for a hint. 502/503/504 stay on the ladder, and a
body that cannot be read or parsed leaves the rung in charge, so the
worst case is the behaviour that exists today.
Reachability is partial and the docstring says so. Most REST callers
wrap these requests in their own 500ms-5s tokio timeout, which cancels
a multi-second hint sleep rather than sleeping it out. That is the right
direction — those callers were going to fail regardless, and this way
they stop spending the relay's counter on attempts that cannot succeed —
but it does mean the full benefit lands on the unbudgeted callers, which
now include the setup nudge.
Mutation results. Ignoring the hint, swapping the one-sided extension
for a symmetric one, and dropping the cap are each killed by the pure
tests (3, 3, and 1 failures respectively). Reverting the *wiring* — so
the hint is never parsed from the body — initially survived all 686
tests, because pure tests of the delay function structurally cannot see
whether the call site supplies a hint. That is the same blind spot the
transport tests closed for the nudge. The added paused-time test drives
a real 429-then-200 exchange and asserts the retry slept 51s rather than
the 560ms rung; it kills that mutant with the measured sleep in the
failure message.
Co-authored-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
Signed-off-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
A 429 with a `retry in {N}s` hint followed by a network error kept the
hint for the next sleep, but only because the sole assignment to
`retry_hint_secs` sat in the Ok-retriable arm. The behaviour was correct
and entirely incidental: moving that line, or adding an arm, would have
silently changed a policy nobody had written down.
Make the match an expression whose value is the hint governing the next
sleep, so every outcome names its own policy. The network-error arm now
yields `retry_hint_secs` explicitly, with a comment giving the reason
(a network error says nothing about the quota window) and the cost
asymmetry that picks the direction: over-sleeping wastes time we were
told to wait anyway, while under-sleeping earns a fresh denial that
still costs a counter increment, because the limiter's INCR runs on
refused checks too.
The comment also fences the one nuance a future reader would trip on:
the kept hint is exactly right only for the sleep immediately after the
429, and subtracting elapsed time to "fix" the over-sleep would
reintroduce under-sleep via clock arithmetic — the expensive direction.
Spelling note: the literal `retry_hint_secs = retry_hint_secs` in that
arm is a deny-by-default clippy error (clippy::self_assignment), so the
policy is expressed by making every arm yield its hint instead.
Covered by a behavioral test, because the arm is otherwise invisible:
it holds the value it already had, so yielding `None` instead is a
silent revert that all four pure `rest_retry_delay` tests stay green
under, as does the existing 429->200 wiring test (which never produces
a network error). The new test serves one 429 with a 51s hint and then
drops the listener, so the ladder sleeps hint/hint/hint (~153s) when
the hint is carried and hint/1s/2s (~54s) when it is dropped, and
asserts into the gap. Mutating the arm to `None` fails it at 53.8s;
across the whole 688-test lib run that mutant is caught by exactly one
test, so the coverage claim is measured rather than assumed.
Verified at rustc 1.95.0: buzz-acp 688 + 9 integration, buzz-cli 322,
0 failed; cargo fmt --all --check and clippy -p buzz-acp -p buzz-cli
--all-targets -D warnings both clean.
Co-authored-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
Signed-off-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
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
1 participant
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.
What
Single PR for the rate-limit fix stack converged in #buzz-relay-rate-limiting (channel 826fc99b-1472-40e7-a529-6b9db8943b8c). Relay half is in; client half (ACP/CLI retry + F9 nudge move, owned by Sami) lands on this same branch next.
Relay: stop billing ephemeral kinds against
Messages(landed)WS admission billed every EVENT frame against the per-minute
Messagesbudget, kind-blind. Ephemeral kinds (20000–29999) are never persisted —buzz-dbrejects them withEphemeralEventRejected(event.rs:277,1127) — yet an agent's telemetry alone (observer 90/min + typing ~20/min + presence 1/min ≈ 111/min vs the 120/min standard-agent budget) could exhaust the durable-message quota with zero durable writes. Two concurrent turns went over quota on telemetry alone, producing the 40s+ NOTICE gate ladders in field logs.Admission now uses the same
is_ephemeral()range check that storage and the EVENT handler's scope gate (handlers/event.rs:698) already use: ephemeral frames ride on theWsEventsbudget alone.Relay: name the exhausted budget in rejections (landed)
Quota rejections on both transports now read
rate-limited: quota exceeded ({msg|ws|api}); retry in {N}s, and thebuzz_admission_rejections_totalquota counter gains alimit_typelabel. Therate-limited:prefix andretry in {N}shint that all clients (desktop TS, ACP, CLI, tauri) parse by substring are unchanged — verified against every parser in-repo.This is the discriminator field logs were missing: the retry hint is the Redis window TTL, so
WsEvents(5s window) andMessages(60s window) denials are indistinguishable below 5s.Clients (incoming on this branch, Sami)
jittered_durationdivisor bug (nanos / u32::MAX→ factor ∈ [0.800, 0.893), never above 1.0 — always-negative "jitter") at 6 relay.rs call sites +queue.rs:461Some(0)test pinning immediate-retryrest_client.submit_event(), dedup commits only afterOk;debug_assert!(is_ephemeral(kind))boundary + regression testsAcceptance checks (adopted in-channel)
Messagespressure → ~0; any residual(msg)hint >5s on the WS path = an unenumerated durable WS publisherTesting
cargo test -p buzz-relay(full package, rustup 1.95.0): 842 passed, 1 failed —api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo(504 vs 200). Pre-existing: fails identically on unmodified4a2305170and on cleandc17965c7(verified via stash), unrelated to this change.cargo clippy -p buzz-relay --all-targetsclean; fmt clean.retry in {N}s+ discriminator).