Skip to content

fix(acp): honor relay rate limits and pace resubscribes on bad links#2199

Merged
wpfleger96 merged 2 commits into
mainfrom
wpfleger/acp-degraded-network
Jul 21, 2026
Merged

fix(acp): honor relay rate limits and pace resubscribes on bad links#2199
wpfleger96 merged 2 commits into
mainfrom
wpfleger/acp-degraded-network

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Background

On a degraded link (3.4s average RTT, 10s maximum RTT, 40% packet loss, and DNS brownouts), the ACP harness generated more than 12,000 rate-limit notices and 7,756 reconnect attempts in seven minutes. Authentication timed out before slow handshakes completed, reconnect replay exhausted the relay's shared admission quota, and retries could form a reconnect storm.

Changes

  • Raise AUTH_TIMEOUT from 5s to 20s and CONNECT_TIMEOUT from 10s to 30s for high-latency links.
  • Recognize rate-limited CLOSED and NOTICE responses, honor the relay's retry hint, and extend overlapping gate deadlines monotonically.
  • Preserve the community-and-pubkey quota gate across socket replacement. A reconnect rebuilds pending work from active subscription intent without treating a fresh socket as a fresh admission window.
  • Park channel, membership, and observer-control subscriptions while gated, then recover them through budget-bounded, select-integrated drains at no more than one REQ per 125ms tick.
  • Pace reconnect replay and re-check the gate before each send so a gate armed mid-burst stops subsequent REQ frames.
  • Defer commands received during replay pacing into a FIFO queue, then execute their live-socket effects in order. Subscribe, Unsubscribe, and ephemeral PublishEvent commands can no longer disappear during replay; stateful intent is retained if the replay socket fails before the queue drains.
  • Keep CLOSE, local-state commands, and ephemeral publishes immediate rather than adding unnecessary pacing. Typing indicators remain best-effort and are dropped while gated or after a failed socket send rather than replayed stale.
  • Park individual channel replay failures for paced retry while treating control-subscription or deferred-command send failures as connection failures.
  • Retry DNS brownouts on a flat jittered interval without consuming the normal reconnect ladder, and reset the ladder after a stable connection.
  • Add local WebSocket behavior coverage for cross-socket gate preservation and replay-time REQ, CLOSE, and EVENT emission, plus deferred-intent preservation on replay failure.

@wpfleger96
wpfleger96 requested a review from a team as a code owner July 21, 2026 02:04
wpfleger96 added a commit that referenced this pull request Jul 21, 2026
@wpfleger96
fix(acp): apply review fixes — control-sub recovery, select-integrate…
ba1cc67 
…d pacing, encapsulation, tests

Fixes all 16 reviewer findings (F1-F16) on PR #2199.

**F1: control-sub recovery from rate-limited CLOSED**

Membership and observer-control subs now recover from rate-limited CLOSED
instead of dying permanently. Two new `BgState` flags (`membership_resub_needed`,
`observer_resub_needed`) are set when the relay rejects these REQs before
registering them (the relay's admission check runs before subscription
registration, so no drain existed to clean them up). The main-loop drain
condition is extended to wake and recover even when `rate_limited_pending`
is empty, so control subs aren't silently lost.

**F2: select-integrated pacing, shutdown-aware resubscribe sleeps**

Removed all inline `tokio::time::sleep(REQ_PACING_INTERVAL).await` calls
from drain functions and `resubscribe_after_reconnect`. Replaced with:
- A `drain_pacing_next: Option<Instant>` budget gate in the main loop.
- A new `select!` timer arm that fires at `REQ_PACING_INTERVAL` and resets
  the gate, waking the loop for the next drain batch.
- A new `pacing_sleep` helper used by `resubscribe_after_reconnect` that
  selects on `cmd_rx` so `Shutdown` is honoured during inter-REQ sleeps and
  non-Shutdown commands are applied to state while waiting.
- Budget-limited drain invocations (`DRAIN_BUDGET_PER_ITER = 1` REQ shared
  across both drains per main-loop tick), keeping effective rate ≤ 8 REQ/s.

The combined effect: a 48-channel resubscribe no longer blocks the select!
loop for ~6s, `cmd_rx` (capacity 64) can't fill up during drain, ping ticks
fire on schedule, and graceful shutdown is honoured during any inter-REQ gap.

`resubscribe_after_reconnect` now returns `ResubscribeResult` (Ok/ControlFailed/
Shutdown) so callers can handle shutdown correctly without losing reconnect
context.

**F3: gate-clearing scope**

`resubscribe_after_reconnect` now accepts `is_fresh_connection: bool`. Gate
clearing is restricted to fresh connections (new socket = fresh relay admission
window). The proactive-resubscribe call site passes `false`, preserving parked
channels across a backpressure-triggered resubscribe on the existing socket.
Fresh-connection callers (`try_autonomous_reconnect`, `wait_for_reconnect`)
pass `true`.

**F4: remove `unwrap` from `set_rate_limit_gate`**

Compute the winning deadline first, assign to field, return the value.

**F5: evict channels from drain queues on successful live subscribe**

`execute_connected_command`'s Subscribe success path now removes the channel
from `rate_limited_pending` and `resubscribe_retry` so the drain loop can't
send a duplicate REQ for an already-live subscription.

**F6: encapsulate rate-gate reads via `check_rate_gate`**

Renamed `is_rate_gated() -> bool` to `check_rate_gate() -> Option<Instant>`.
Returns the deadline when gated (so callers don't re-read the field directly)
and preserves the lazy-clear behaviour. All call sites updated.

**F7: drain state-transition tests**

New unit tests for: rate-limited CLOSED setting control-sub flags; drain
failure re-queuing with +5s penalty; gate re-arm mid-drain moving channels
from `resubscribe_retry` to `rate_limited_pending`; successful drain clearing
`channel_dropped_since`.

**F8: fix Finding #44 → #46 in `DNS_RETRY_INTERVAL` comment**

**F9: move `DNS_RETRY_INTERVAL` and `REQ_PACING_INTERVAL` to top-of-file
constants block; add `DRAIN_BUDGET_PER_ITER` constant.**

**F10: document DNS retry asymmetry**

`try_autonomous_reconnect` (bounded, capped at 10 DNS flat retries) and
`wait_for_reconnect` (unbounded) each get a one-sentence note explaining
the intentional difference.

**F11: drop `retry_after` alias in CLOSED handler** — use `deadline` directly.

**F12: comment threshold in `set_rate_limit_gate`** — explains why 1s hints
floor to 5s, and notes the deliberate cross-client difference vs. the desktop
TypeScript client's 10s no-hint default.

**F13: `parse_rate_limit_retry_secs`** — replace `String` collect with
subslice parse (ASCII digits → byte count = char count, so `&after[..len]`
is safe).

**F14: `drain_resubscribe_retry`** — budget-bounded `.take(budget)` replaces
full-set clone; pacing enforced by caller.

**F15: `is_dns_error` test** — add production-shaped `RelayError::WebSocket`
wrapping a `tungstenite::Error::Io` variant, which is the actual error shape
emitted by `connect_async` on macOS DNS failures.

**F16: document ephemeral-only invariant on gated `PublishEvent` drop path.**
@wpfleger96 wpfleger96 mentioned this pull request Jul 21, 2026
Merged
@wpfleger96
wpfleger96 force-pushed the wpfleger/acp-degraded-network branch from b3919e8 to 4f3f7d4 Compare July 21, 2026 03:44
@wpfleger96
fix(acp): honor relay rate limits and pace resubscribes on bad links
1d47386 
On a degraded link (3.4s avg RTT, 40% packet loss, DNS brownouts) the ACP
harness generated 12,000+ rate-limit NOTICEs and 7,756 reconnect attempts
in 7 minutes. This commit addresses all root causes.

**Timeout tuning** — `AUTH_TIMEOUT` raised from 5s to 20s; `CONNECT_TIMEOUT`
from 10s to 30s. At 3.4s average RTT the old 5s auth window expired before
the relay's first frame arrived.

**Rate-limit gate** — When the relay sends a CLOSED or NOTICE with a
`"rate-limited:"` prefix, the harness now arms a `rate_limit_gate` deadline
using the relay's `"retry in {N}s"` hint (`parse_rate_limit_retry_secs`).
While gated: new `Subscribe` commands park in `rate_limited_pending`;
ephemeral `PublishEvent` frames (typing indicators) are dropped (they're
worthless when the relay already rejected us). The gate takes the max of
overlapping deadlines. `set_rate_limit_gate` floors hints below 2s to 5s.

**Control-sub recovery** — When the rate-limited CLOSED arrives for the
membership or observer-control subscription (which the relay rejects before
registering, so no per-channel entry exists), `membership_resub_needed` /
`observer_resub_needed` flags are set. The main-loop drain fires even when
`rate_limited_pending` is empty, ensuring control subs are re-sent once the
gate clears.

**Select-integrated pacing** — Inline `tokio::time::sleep(REQ_PACING_INTERVAL)`
calls removed from drain functions and `resubscribe_after_reconnect`. Replaced
with: (a) `drain_pacing_next: Option<Instant>` gate; (b) a `select!` timer arm
(`sleep_until` / `pending()`) that wakes the loop at each pacing tick; (c)
`DRAIN_BUDGET_PER_ITER = 1` REQ per 125ms tick, keeping effective rate ≤8
REQ/s; (d) a `pacing_sleep` helper (no jitter) for shutdown-aware inter-REQ
gaps in `resubscribe_after_reconnect`. A 48-channel drain no longer blocks the
select! loop for ~6s.

**Shutdown-aware resubscribe** — `resubscribe_after_reconnect` accepts `cmd_rx`
and returns `ResubscribeResult` (Ok/ControlFailed/Shutdown). `Shutdown`
commands during pacing sleeps propagate immediately.

**Gate-clearing scope** — `resubscribe_after_reconnect` accepts
`is_fresh_connection: bool`. Gate and pending queues are only cleared on a
fresh connection (new admission window). Proactive resubscribe on the existing
socket passes `false`, preserving parked channels.

**Partial reconnect failure** — Individual channel REQ failures park in
`resubscribe_retry` (drained by the pacing timer) rather than aborting the
entire reconnect. Successful `Subscribe` commands evict stale drain entries to
prevent duplicate REQs. `check_rate_gate() -> Option<Instant>` (renamed from
`is_rate_gated`) returns the deadline so callers don't re-read the field.

**Backoff ladder reset on stability** — The 60s stability block resets
`state.backoff_step` so a brief drop after a long healthy run retries at the
short end of the ladder.

**DNS brownout classification** — DNS resolution failures retry on a flat
`DNS_RETRY_INTERVAL` (2s ± jitter) without consuming a backoff ladder rung.
`try_autonomous_reconnect` caps DNS-only retries at 10 so agent startup can't
hang through a total brownout; `wait_for_reconnect` is unbounded.

Tests: 5 new unit tests covering rate-limit gate arm/expiry/max-extend,
control-sub flag set/clear, drain failure re-queue, gate re-arm mid-drain,
and successful drain cleanup. `is_dns_error` test adds the
`RelayError::WebSocket(tungstenite::Error::Io(...))` production-shaped variant.
560 tests pass.
@wpfleger96
wpfleger96 force-pushed the wpfleger/acp-degraded-network branch from 4f3f7d4 to 1d47386 Compare July 21, 2026 04:23
@wpfleger96
fix(acp): preserve relay recovery intent across reconnects
f5e9724 
Relay admission quotas survive socket replacement, so reconnect replay must keep an active gate rather than burst into the same exhausted window. Commands consumed during paced replay also need their live-socket effects or durable intent preserved if replay fails.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 merged commit 4de3e04 into main Jul 21, 2026
32 checks passed
@wpfleger96
wpfleger96 deleted the wpfleger/acp-degraded-network branch July 21, 2026 07:21
tlongwell-block pushed a commit that referenced this pull request Jul 21, 2026
Merge origin/main into eva/agents-everywhere (5)
5cd0e18 
* origin/main:
  fix(desktop): keep project branches after reload (#2214)
  fix(git): make project branch workflows reliable (#2213)
  feat(desktop): manage project branches (#2212)
  fix(acp): honor relay rate limits and pace resubscribes on bad links (#2199)
  chore(lefthook): add CI-style path filtering to hooks (#2211)
  feat(cli): manage repository protection rules (#2193)
  feat(cli): filter archived instances from --template roster resolution (#2207)
  chore(release): release Buzz Desktop version 0.4.21 (#2209)
  fix(desktop): clarify data deletion action (#2208)
  feat: brand authentication complete page (#2192)
  fix(buzz-acp,buzz-agent): surface stall duration and fate (#2204)
  fix(desktop): resolve activity feed showing channel UUID instead of message content (#2201)
  feat(cli): add agents archive/unarchive/archived subcommands (#2173)
  chore(acp): strip stale finding-number references from comments (#2202)
  chore(release): release Buzz Mobile version 0.4.9 (#2200)
  Simplify first-community onboarding choices (UI only) (#2194)
  fix(mobile): sanitize Android image uploads (#2188)
  fix(cli): paginate channel directory queries (#2181)

Semantic resolutions in crates/buzz-acp/src/lib.rs:
- kept lazy_pool startup + pool_ready select guard from this branch,
  adopted main's comment cleanup (#2202)
- dropped this branch's duplicated steer_renewal_tests module: #2204 moved
  those tests to queue.rs on main

Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
@tlongwell-block tlongwell-block mentioned this pull request Jul 21, 2026
Merged
tlongwell-block pushed a commit that referenced this pull request Jul 21, 2026
fix(acp): park observer frames while rate-gated instead of dropping
6c0fcae 
The generic PublishEvent arm silently drops every publish while the
rate-limit gate (#2199) is armed. That is correct for typing
indicators, but observer telemetry frames (kind 24200) are durable:
one NOTICE from any traffic source erased ~30 frames of turn history
over the default ~5s gate.

Observer frames now park in a bounded drop-oldest FIFO on BgState
(cap 256 ≈ 40s of upstream pacing at 6/s) whenever the gate is armed
or earlier parked frames are still draining (order preserved). The
existing main-loop drain machinery delivers them one frame per 125ms
pacing tick once the gate clears, arming the pacing timer to the gate
deadline so drains fire even with no other traffic. Overflow evicts
oldest and is counted + logged — visible loss, never silent.

The same parking now also covers observer frames hit by a live send
failure or emitted while disconnected, so they survive reconnect via
the post-reconnect drain. Typing indicators keep the existing
drop-while-gated behavior.

Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wpfleger96