Skip to content

fix(acp): verify subscriptions after reconnect - #5913

Open
LucasMoskun wants to merge 1 commit into
block:mainfrom
LucasMoskun:fix/acp-confirm-resubscriptions
Open

fix(acp): verify subscriptions after reconnect#5913
LucasMoskun wants to merge 1 commit into
block:mainfrom
LucasMoskun:fix/acp-confirm-resubscriptions

Conversation

@LucasMoskun

Copy link
Copy Markdown

Summary

  • require EOSE confirmation for every channel and control subscription REQ
  • force reconnect when a written REQ remains unconfirmed for 60 seconds
  • clear stale socket deadlines and re-arm them for replayed subscriptions
  • expose confirmed EOSE at info level for host watchdogs

Regression coverage

  • EOSE clears a pending subscription health deadline
  • a written subscription without EOSE expires
  • a fresh reconnect discards old-socket deadlines and tracks the replayed REQ

Verification

  • cargo fmt --all -- --check
  • cargo clippy -p buzz-acp --all-targets -- -D warnings
  • cargo test -p buzz-acp (691 unit + 9 lifecycle tests)
  • just ci (full workspace, web, and mobile checks)

Co-authored-by: LucasMoskun <contact@lucasmoskun.com>
Signed-off-by: LucasMoskun <contact@lucasmoskun.com>
@LucasMoskun
LucasMoskun requested a review from a team as a code owner August 14, 2026 21:53

@themiguelamador themiguelamador left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No findings. I traced every channel, membership, and observer-control REQ path through initial subscribe, targeted CLOSED recovery, rate-limit drain, proactive replay, and fresh reconnect. Deadlines are armed only after successful writes, EOSE and terminal CLOSED frames retire them, unsubscribe removes channel deadlines, and a fresh socket clears old expectations before replay arms replacements.

Verified with cargo test -p buzz-acp (691 unit tests plus 9 lifecycle tests), strict cargo clippy -p buzz-acp --all-targets -- -D warnings, cargo fmt --all -- --check, and git diff --check.

@mfethe1

mfethe1 commented Aug 20, 2026

Copy link
Copy Markdown

Field data in support of this PR, from 46 local harness run logs spanning 17 days (~67,900 lines, ~10 agents against two independent relays).

The failure mode this PR hypothesizes is real and common in practice. Over two days (Aug 18–19) those logs contain:

  • 571 × resubscribing to N channel(s) after reconnect
  • 0 × EOSE acknowledgement of any kind — the string does not appear anywhere in the corpus

So every resubscribe in that window completed on the current send_subscribe contract: the REQ was written, the write succeeded, and nothing ever confirmed the subscription became live. That is exactly the "connected but deaf" window described here, and it is entered 571 times in two days rather than rarely.

Context for why the rate is that high: those 775 reconnects are not relay instability. The agents talk to two independent relays (one on Railway, one behind Cloudflare) and both drop within the same few seconds of each other, repeatedly — e.g. T06:07:31/T06:07:32, T17:30:12/T17:30:14 — which points at local egress, not either provider. Only 2 pong timeouts were ever recorded against those 775 drops, i.e. the socket dies well before the 30 s keepalive notices. Whatever the cause, a harness on a flaky link re-enters the unverified-subscription window constantly, which is what makes this fix worth having rather than theoretical.

One blind spot worth naming, since it borders on this PR's health model rather than contradicting it: the deadline can only fire for a REQ that was actually written. If subscribe_channel fails for every channel at startup, there is no outstanding REQ to expire, and the membership + observer-control subscriptions still EOSE-confirm normally — so the connection reports healthy while the agent has zero channel reach. The pre-existing guard in lib.rs could not catch it either, because it ran before the subscribe loop and keyed on resolved filters rather than live subscriptions.

The two halves compose:

REQ written No REQ live
detects unconfirmed REQ this PR
detects zero reach #6365

I opened #6365 for that second half. It is deliberately not stacked on this branch — it touches lib.rs where this touches relay.rs, there is no code dependency, and stacking would impose a review order for nothing. Either can merge first, in any order.

No change requested here; the diff reads correctly to me, including clearing pending_eose on reconnect so deadlines owned by the dead socket cannot fire against the new one.

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.

3 participants