Skip to content

fix(acp): pace targeted overflow recovery on consumer capacity - #7325

Open
loganj wants to merge 3 commits into
mainfrom
fix/acp-recovery-narrow-9429207a
Open

fix(acp): pace targeted overflow recovery on consumer capacity#7325
loganj wants to merge 3 commits into
mainfrom
fix/acp-recovery-narrow-9429207a

Conversation

@loganj

@loganj loganj commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

🤖

Summary

When a Buzz agent falls behind on incoming messages, its connection can make the backlog worse while trying to recover. The connection buffers messages from the relay server until the agent is ready to process them; if that buffer overflows, recovery previously requested history for every subscribed channel and paused socket reads while sending those requests. That adds traffic to an already overloaded connection. This change requests history only for affected subscriptions, once the code consuming those messages has room, with at least five seconds between attempts.

The recovery path now:

  • Combines repeated losses into one pending recovery per affected subscription, keeping the oldest dropped timestamp so replay starts early enough.
  • Waits until at least half the consumer queue is free and the relay's existing rate-limit delay has expired. The queue wakes recovery when space becomes available; recovery does not periodically sample capacity or hold queue space away from live messages.
  • Attempts one subscription at a time, choosing the least recently attempted so a busy channel cannot crowd out other channels or membership notifications. The five-second delay starts when an attempt finishes, including a failed write; failed writes leave recovery pending.

Recovery is paced by available capacity, not by how often messages are lost. This is not a larger buffer or a cutoff that abandons recovery. Subscription identifiers, message filters, replay timestamp overlap and duplicate filtering are unchanged; no downstream agent changes are required.

This targets a reproducible overload amplifier, not every cause of overload or every catch-up limitation. The initial live overload's cause has not been established. Recovery remains best effort: a successful request write is not proof of delivery, and existing history/retention limits, bounded duplicate tracking and replay limitations still apply. There is no exactly-once or complete catch-up guarantee. A stalled write can still pause socket reads for the existing ten-second timeout; the pacing bound does not cover initial subscriptions, reconnects or other retry paths.

Related issue

Closest related: #5014 (channel re-subscription); also #6661 (membership reconciliation) and #6090 (relay backpressure gap signaling). This addresses local overflow recovery scheduling, not those separate mechanisms.

Testing

Recorded offline comparisons against the previous behavior, with the final implementation at 8000636f3073167c5a5107bb179c7d91160f1729:

Same fixture: 18 subscriptions, three overload rounds Before After
Recovery history requests 108 3
Ping-response delay About 4.6 seconds Below the measurement's 1 ms resolution

A separate bounded-history fixture delivered all 320 events plus subsequent live traffic in both versions. Regression coverage exercises the real socket-handling task, including intermittent consumer capacity, fairness, failed writes and cancellation of capacity waits before live delivery. These are synthetic results, not production throughput measurements or evidence of a deployed cure.

The full local RUST_TEST_THREADS=4 just ci run passed on September 4, 2026. Earlier unsuccessful local runs remain part of the validation history. The recorded validation evidence and separate desktop follow-up preserve the original desktop mock-history scroll failure, its passing rerun and the remaining investigation. That desktop path does not run the agent connection code; neither this repair nor the passing rerun fixes the observed scroll problem.

Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
@loganj
loganj requested a review from a team as a code owner September 4, 2026 11:43
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is ee883d73fb84752d26f52c6606ad3cadc9f12cc7...6e398931169a6d64bb4e13a835fbb1d85ac144cb.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 6e398931169a6d64bb4e13a835fbb1d85ac144cb to authorize a new review.
Any previous review applies only to its recorded range.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review verdict: approve

Reviewed base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6 through exact live head 8000636f3073167c5a5107bb179c7d91160f1729, integrating both assigned systems and adversarial lanes. No author-actionable defect remains.

Targeted recovery is integrated into the production socket-owner select loop without blocking reads, commands, ping, or shutdown. It waits cancellation-safely for half-queue capacity, then rechecks receiver state, cooldown, shared quota, and active intent; fairness selects the least-recently-attempted eligible membership/channel intent. Failed writes retain the recovery cursor, successful REQ writes retire it at the existing boundary, unsubscribe clears associated recovery state, and reconnect preserves the established min(last_seen, dropped) restoration behavior. Production-seam tests prove replayed event delivery to the consumer rather than merely REQ emission.

Exact-head evidence: cargo test -p buzz-acp passed 916 unit tests plus 9 integration tests; git diff --check passed with a clean exact-head worktree. At submission, the live head remained exact, completed lint/security/DCO/cross-compile/platform checks were green, and no completed required check had failed. Remaining running unit, Desktop, relay-artifact, image, and other matrix jobs are owned by CI.

Author action: none.

Confidence gaps / verification owner: neither lane ran sustained real-relay + real-child loss/load or mutation-tested every regression. The production owner and real loopback WebSocket seams are covered, but relay retention/proxy behavior and downstream prompt completion remain reviewer/tooling follow-up. Catch-up remains intentionally best-effort after successful REQ write rather than exactly-once delivery, as documented.

— :bot: Jude’s code review agent

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Team verdict: APPROVE at exact head 8000636f3073167c5a5107bb179c7d91160f1729 (base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6).

No author-actionable material defect was found across the systems/integration and adversarial product review lanes.

The review traced the production socket-owner path end to end: overflow preserves the oldest recovery cursor while releasing the dropped event from bounded transport dedup; capacity readiness participates in the existing responsive select!; recovery rechecks receiver state, half-queue headroom, cooldown, and shared quota; least-recently-attempted eligible channel or membership work is selected; failed writes retain recovery state; only a successful REQ write retires it; and unsubscribe/reconnect clean up or reconstruct the corresponding state. The changed regressions exercise the real background task and loopback WebSocket seam, including recurring headroom, fairness, cancellation, failed or stalled writes, reconnect, ping/shutdown responsiveness, and delivery of the replayed event to the consumer.

Exact-head evidence:

  • . ./bin/activate-hermit && cargo test -p buzz-acp: 916 library tests and 9 integration tests passed; 0 failed (plus clean main/doc suites).
  • git diff --check b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6...8000636f3073167c5a5107bb179c7d91160f1729: passed.
  • Clean reviewer worktrees; local/remote PR head equality rechecked at lane close.
  • Live integration state at submission still reports the same base/head and mergeable; Rust lint, security, DCO, Linux cross-compile results, and completed relevant checks are green. Remaining matrix/build jobs are still in progress.

Residual confidence gap: the reviewers did not run a sustained real-relay + real ACP-child stress cohort. The production-seam tests prove replay through the socket owner to the consumer, but not relay retention/history-limit behavior, proxy replay, or downstream ACP prompt completion. This is a reviewer/tooling follow-up, not author rework; the README accurately preserves the best-effort boundary after REQ write.

Author action: none.

@loganj

loganj commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Final CI disposition — source ready, no merge performed

Exact head 8000636f3073167c5a5107bb179c7d91160f1729, base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6.

The one scoped rerun of Desktop Smoke E2E shard 4, attempt 2 completed successfully at 2026-09-04 12:39:40 UTC: 334 passed (20.1m), including the previously failed cascading-older-pages test. Required Desktop is now SUCCESS. Final verification: 12 required contexts SUCCESS, 2 normal path-selection SKIPPED (Mobile/Web), none pending or failed. GitHub reports APPROVED, no review requests, MERGEABLE/CLEAN; published head and main/base remain exact. The earlier actual full RUST_TEST_THREADS=4 just ci pass remains valid; no new source changes, tests, pushes or additional reruns were performed in this follow-up.

Separate desktop follow-up retained — not fixed by green CI

The original failure is not erased. In virtualization.spec.ts, “08 — cascading older pages never snap the viewport toward newest,” retry 1's trace shows normal prepend compensation 180→5540 followed by an extra 4047px jump to 9587, the exact viewport floor; upward input then leaves only 125px bottom distance. The exact writing callback is unresolved. Original/retry-2 forward travel was 120 versus required >200, without traces proving their input timing.

This smoke path runs browser mock IPC/local fixture history, not ACP. The checked-out merge tree equals candidate; every tracked non-ACP input equals base, including desktop/build/workflow dependencies. That establishes unaffected runtime/source equivalence, not a pristine-baseline failure reproduction.

Desktop follow-up: instrument bottom-settle/prepend/first-input ordering and Virtua scroll writers to identify the floor write; bind boundary/exit observations to actual input dispatch and prepend settlement (the saved first wheel command alone took 344ms versus a 120ms early sampler). Preserve drift/travel assertions rather than relax thresholds. A real reader snap needs its own desktop fix, not expansion of this ACP PR or another rerun-to-green.

Original artifacttest-results/virtualization-list-virtua-bbaa3--the-viewport-toward-newest-smoke-retry1/trace.zip. Its complete local copy, extracted trace and durable actionable follow-up survive remote artifact expiry in WORK_LOGS/ACP_RECOVERY_NARROW_AGGREGATE/, ACP_RECOVERY_NARROW_CI_DIAGNOSIS/, and ACP_RECOVERY_NARROW_FINAL_STATUS/DESKTOP_FOLLOWUP.md in the work discussion. No separate maintainer acceptance is implied.

This is source-engineering completion only, not deployment or a production cure. Successful REQ write remains best-effort replay; history/retention, EOSE/delivery, bounded dedup and additive-watch limitations remain as documented in the PR.

Signed-off-by: Logan Johnson <loganj@squareup.com>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed-head team verdict: APPROVE at exact head 6e398931169a6d64bb4e13a835fbb1d85ac144cb (base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6; prior approved head 8000636f3073167c5a5107bb179c7d91160f1729).

No author-actionable material defect was found.

The exact changed-head delta is one documentation-only commit: crates/buzz-acp/README.md is +4/-38; the production/test source tree, package manifest, and lockfile relevant to this change are byte-identical to the prior approved head. The condensed README contract remains accurate: affected-subscription replay is consumer-capacity and quota gated, paced no more often than every five seconds, depends on relay history and consumer progress, and does not guarantee complete delivery.

Both review lanes revalidated the unchanged production behavior: overflow preserves the oldest cursor while releasing undelivered IDs from bounded transport dedup; recovery readiness remains one arm of the responsive socket-owner select!; select-local capacity permits are dropped before dispatch and capacity is rechecked; least-recently-attempted eligible channel or membership work is selected; failed writes retain the cursor while successful REQ writes retire it; unsubscribe/access denial clears associated state; and reconnect resumes from min(last_seen, dropped). Existing production-seam regressions prove the replayed event reaches the consumer by exact ID and cover recurring pressure/fairness, stale EOSE, failed/stalled writes, headroom wake/cancellation, reconnect, unsubscribe, ping, shutdown, and transport loss.

Exact-head evidence:

  • cargo test -p buzz-acp under pinned Hermit: 916 library tests + 9 integration tests passed; 0 failed; main/doc suites clean.
  • git diff --check b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6...6e398931169a6d64bb4e13a835fbb1d85ac144cb: passed.
  • Remote/local exact-head equality and clean worktrees rechecked after testing; merge-base equals the supplied base; all three commits carry DCO sign-offs.
  • Live integration state at submission still reports this exact base/head as mergeable: 20 checks passed, 11 pending, 0 failed. Completed ACP-relevant Rust lint, security, DCO, Linux cross-compile, and image-build checks are green.

Residual confidence gap: sustained real-relay + real ACP-child pressure and mutation testing remain unrun. Relay retention/proxy behavior and downstream prompt completion therefore remain best-effort boundaries already documented by the PR. This is reviewer/tooling follow-up, not author rework.

Author action: none.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE at exact head 6e398931169a6d64bb4e13a835fbb1d85ac144cb against base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6.

No unresolved author-actionable defect remains after independent systems/integration and product/adversarial review.

The changed-head delta is documentation-only: the ACP production and test trees are byte-identical to the previously approved head. The condensed README remains accurate about targeted affected-subscription replay, capacity/quota gating, pacing, relay-history and consumer-progress dependency, and the absence of complete-delivery guarantees.

The integrated review revalidated socket-owner responsiveness, cancellation-safe capacity reservation, least-recently-attempted fairness, cursor retention on failed writes and retirement after successful REQ writes, unsubscribe cleanup, reconnect recovery, dedup handling, and production-seam tests that prove the lost event reaches the consumer rather than merely proving REQ emission.

Exact-head evidence: cargo test -p buzz-acp passed 916 library + 9 integration tests, with main/doc suites clean; git diff --check, merge-base, DCO, policy audit, exact local/remote SHA, and clean-tree checks passed. Completed Rust lint, security, Linux cross-compile, Windows Desktop, image, and release checks were green.

Confidence gaps, not author rework: sustained real-relay + real ACP-child pressure and mutation depth were not run. Several unrelated matrix/build checks remain in progress with no observed failure. Author action: none unless a pending required gate produces a PR-caused failure. Verification owner: CI for exact-head gate completion; reviewer/tooling for optional sustained live stress.

Any new head invalidates this approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants