Skip to content

fix(mobile): bound startup channel relay load - #5831

Open
wesbillman wants to merge 3 commits into
mainfrom
carl/mobile-startup-relay-load-safe
Open

fix(mobile): bound startup channel relay load#5831
wesbillman wants to merge 3 commits into
mainfrom
carl/mobile-startup-relay-load-safe

Conversation

@wesbillman

@wesbillman wesbillman commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • replace 1,565 per-channel startup last-message lookups with 16 bounded authenticated HTTP query batches sharing one eight-second budget
  • admit required per-channel #h live subscriptions through four paced workers with 125 ms start spacing, while overlapping admission with the startup snapshot
  • wait for relay EOSE before treating each live subscription as ready, including across retryable and rate-limited CLOSED responses
  • preserve delivery across snapshot/live overlap, reconnect, cancellation, refresh, and terminal-close recovery with max-merged timestamps
  • add relay fan-out, provider lifecycle, retry, cancellation, and gap-recovery regressions

Problem

Large accounts launched one history request per channel during mobile startup. At 1,565 channels this caused a relay request storm and quota failures. The previous replacement in #5802 paced history requests, but still made startup wait minutes for all per-channel work and did not fully close snapshot/live delivery races.

This version keeps channel-scoped live REQs because relay fan-out requires #h, but paces their admission and moves snapshot discovery to bounded HTTP batches. Large startup can publish after the shared snapshot budget while live admission continues safely in the background.

Correctness details

  • subscribeWhenReady resolves only after EOSE; retryable/rate-limited CLOSED stays pending through replay
  • replay events flush synchronously before readiness resolves
  • pending admissions cancel on disconnect, channel removal, relay change, and provider disposal
  • terminal CLOSED before or after readiness cannot leave stale provider state; attempt identity prevents an obsolete callback deleting a replacement
  • catch-up runs only for successfully ready channels and reruns when a failed/closed channel is later admitted
  • snapshot, live, refresh, and catch-up timestamp updates use max semantics so older results cannot roll back lastMessageAt

Validation

Exact commit: a9a786ddfabea27552a6633494c789ffd2ad9a80

  • focused channel-provider suite: 32/32 passed
  • relay-session suite: 43/43 passed
  • analyzer and formatting: clean
  • mobile file-size ratchet: passed (channels_provider.dart 999 lines, relay_session.dart 996)
  • pre-push hooks: passed on the exact pushed head, including all 1,371 mobile tests and branch-skew
  • independent architecture review: keep the direction with snapshot/EOSE separation and terminal-close quarantine; both are implemented

Follow-up device validation

Measure full live-subscription/unread convergence time and quota rejection counts on the 1,565-channel account. The channel snapshot itself remains bounded to eight seconds.

@wesbillman
wesbillman requested a review from a team as a code owner August 14, 2026 03:19
@wesbillman
wesbillman force-pushed the carl/mobile-startup-relay-load-safe branch from a0ddbb8 to 39d6d38 Compare August 14, 2026 03:31
Aggregate initial last-message discovery through bounded HTTP query chunks,
and pace unread catch-up with cancellation while preserving per-channel live
subscriptions. Add regression coverage for relay routing scope.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman
wesbillman force-pushed the carl/mobile-startup-relay-load-safe branch from 39d6d38 to 10077f2 Compare August 14, 2026 03:36

@jedwards27 jedwards27 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.

Requesting changes at exact head 10077f235e373ff5d36c7aa29f4438f99de7bacf.

Blocking: initial channel loading can wait forever for live-subscription EOSE.

For the common case of 100 or fewer channels, _fetchChannels awaits bootstrapLiveSync (mobile/lib/features/channels/channels_provider.dart:433-435). Each paced task awaits subscribeWhenReady (channels_provider.dart:680-699), and that API deliberately has no fallback timeout: it resolves only when _handleEose completes its readiness completer (mobile/lib/shared/relay/relay_session.dart:315-317, 684-714).

A connected relay can accept the REQ but stall or omit EOSE (network half-open, relay bug, or overloaded query). In that state the bounded HTTP snapshot finishes, but the provider's initial future never returns, leaving the channel list loading indefinitely. This regresses the old subscribe() path, which had a 500 ms readiness fallback. The >100 branch avoids awaiting the live pass, which makes the failure account-size dependent rather than safe.

Please keep initial rendering bounded independently of live readiness—e.g. never await the full live pass before publishing the snapshot, or impose an explicit startup deadline while allowing subscriptions to continue/retry—and add a provider-level regression where a small account's live REQ never receives EOSE but the bounded snapshot is still published.

Blocking gate: the Mobile check deterministically fails the repository file-size ratchet. channels_provider.dart grows from 937 to 1,238 lines and relay_session.dart from 980 to 1,039; the CI log reports both. Please split the new responsibilities into focused modules rather than bypassing the ratchet.

Exact-head validation on a clean worktree:

  • just mobile-test: 1,370/1,370 passed.
  • git diff --check origin/main...HEAD: passed.
  • cargo test -p buzz-relay --lib: 867 passed, 13 failed, 43 ignored; failures were broad local-environment/infrastructure cases (notably absent DB schema and mesh-demo timeout), so I am not treating that run as PR validation.

The snapshot/live overlap handling, cancellation plumbing, retry-pending readiness tests, and bounded HTTP batching are thoughtful, but the startup wait above violates the central bounded-load contract and needs behavioral coverage at the provider seam.

@jedwards27

Copy link
Copy Markdown

Supplemental lifecycle finding at the same exact head, independently probed by Princess Donut and confirmed against the production/test paths:

A post-ready terminal CLOSED leaves the channel unsubscribed until an unrelated refresh or the 60-second backstop. The callback removes _liveSubscriptionsByChannel[channelId] and the catch-up marker (channels_provider.dart:689-696) but schedules no resync. The test named post-ready terminal close retries subscription and catches up gap (mobile/test/features/channels/channels_provider_test.dart:499-539) only succeeds because it explicitly calls refresh() after the close; it does not prove autonomous retry.

An independent temporary probe terminally closed the only channel, injected a missed event, then drained 100 event-loop turns without calling refresh: subscribe count stayed at 1, active subscriptions stayed empty, catch-up count stayed at 1, and the event remained unseen. The probe was reverted after validation.

This needs an explicit product policy and honest coverage. Immediate unconditional retry is unsafe because terminal closure includes permanent cases such as restricted: access revoked; a bounded/cooldown resync with gap catch-up or an explicit terminal state would avoid both a blind interval and an authorization hot loop. At minimum, the current test should not claim automatic retry when it manually causes it.

Related test-fidelity gap: _FakeRelaySession.subscribeWhenReady accepts but ignores its cancelled future (channels_provider_test.dart:1415-1420), so provider tests cannot establish the real pending-subscription cancellation contract across removal/disconnect/dispose. Please make the fake honor cancellation and cover that seam.

Publish the bounded channel snapshot independently of live EOSE, quarantine
terminally closed channel subscriptions until a material invalidator, and
extract channel sync and relay support code below the file-size ceiling.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman
wesbillman dismissed jedwards27’s stale review August 14, 2026 16:00

Addressed by a9a786d: startup snapshot no longer waits for EOSE, lifecycle state was extracted under the file-size ratchet, terminal closures are quarantined until a material invalidator, and cancellation-aware regressions were added.

@wesbillman

Copy link
Copy Markdown
Collaborator Author

Carl, an automated reviewer, commenting via Wes’s GitHub account.

The findings in the earlier review/comment are addressed at a9a786ddfabea27552a6633494c789ffd2ad9a80:

  • initial snapshot publication no longer waits for live EOSE, with a small-account no-EOSE regression
  • channel sync and relay support responsibilities were extracted; the file-size ratchet now passes
  • post-ready terminal closures are quarantined until reconnect, changed channel set, or explicit refresh, avoiding both the blind retry claim and an authorization hot-loop
  • the provider fake honors pending-subscription cancellation

I dismissed my stale changes-requested review against 10077f235e373ff5d36c7aa29f4438f99de7bacf. This is not an approval. Current-head CI is green.

@jedwards27 jedwards27 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.

Requesting changes at exact head a9a786ddfabea27552a6633494c789ffd2ad9a80.

Blocking: a missing EOSE can permanently strand live admission and catch-up after the initial snapshot.

Publishing the initial snapshot without awaiting bootstrapLiveSync fixes the visible startup hang, and the new regression causally covers that part. But each paced worker still awaits subscribeWhenReady without a deadline (mobile/lib/features/channels/channels_provider.dart:556-637). Four accepted REQs that never receive EOSE occupy all four workers indefinitely. Consequently later channel subscriptions are never admitted, _startUnreadCatchUpIfNeeded is never reached (channels_provider.dart:654-657), and even the 60-second backstop is never armed because its timer is installed only after the paced pass completes (channels_provider.dart:659-663). The UI loads, but the account can remain blind for every channel beyond the first four with no autonomous recovery.

Please bound each readiness attempt independently, cancel/close the timed-out REQ so its worker can continue, and add a provider regression with more channels than the worker limit where the first four never receive EOSE but later admissions and catch-up still proceed.

Blocking: error: too many subscriptions is quarantined as permanently terminal although capacity can recover.

classifyRelayClosed labels that response terminal (mobile/lib/shared/relay/relay_closed_policy.dart:19-28). The provider then adds the channel to _terminallyClosedChannelIds (channels_provider.dart:578-586), and unchanged-set backstop syncs exclude it (channels_provider.dart:550-555). If capacity later becomes available—another subscription closes, for example—the refused channel remains unsubscribed until manual refresh, reconnect, foreground resume, or a material channel-set change. Worse, the periodic backstop does not clear this quarantine, so it cannot provide the claimed bounded recovery.

Please treat capacity refusal as retryable/backoff-governed, or expire/reconsider this quarantine on a bounded autonomous path, with a regression proving admission after capacity is released without user action or reconnect.

Exact-head evidence:

  • just mobile-test: 1,371/1,371 passed.
  • just mobile-check: format, analyzer, and file-size ratchet passed.
  • Startup regression mutation (unawaitedawait): failed by its one-second deadline as required; pristine full suite passed after restoration.
  • Required CI is settled green and local worktree is clean at the SHA above.

The previous startup-render and file-size blockers are fixed. These remaining lifecycle failures are downstream of the same no-EOSE/capacity conditions and still violate the bounded autonomous convergence contract.

@brow

brow commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🤖 Additive review notes for #5831 at head a9a786ddfabea27552a6633494c789ffd2ad9a80. Base is b30f1f61299f6f559777f797be27f193a6a4f0b3. main has not moved in any of the nine changed files since that base, so base comparisons are also current-main comparisons.

jedwards27 already requested changes at this exact head. We reached the same two blockers independently on our own rigs. We do not restate them, and we do not add a second request for changes. This comment adds only evidence and coverage findings that his review does not contain.

The direction of the change is right. Bounded snapshot batches, paced live admission, and EOSE-gated readiness are the correct shape.

1. The missing-EOSE blocker is reachable in production, not only in theory

This is the part we think is new. The relay can drop an EOSE frame while leaving the socket healthy.

  • ConnectionState::send uses try_send on the bounded data channel. On a full buffer it drops the message and returns false.
  • Every EOSE send site in the REQ handler ignores that return value.
  • The default data buffer is 1000 messages. The slow-client grace limit is 15, so up to 15 consecutive drops occur before the connection is cancelled.
  • heartbeat_loop sends its Ping on the separate control channel, which the comment in handle_connection describes as "guaranteed delivery even when the data buffer is full".

Put together: a client can lose one EOSE, keep a fully healthy socket, never miss a pong, and never be disconnected. A startup that opens more than 1500 subscriptions is exactly the workload that fills a 1000-slot data buffer.

Before this change the same dropped frame was harmless, because subscribe proceeded after its 500 ms fallback. After this change the paced worker waits forever. That is what turns a tolerated frame drop into a permanent startup stall.

We measured the client half on two separate rigs. With four never-EOSE channels out of twenty and maxConcurrent at four, four tasks started and none completed. The control with no hung task started and completed all twenty. A plain subscribe control resolved through its 500 ms fallback. We did not run a real relay under load, so the drop itself is a code-level reading of the send path plus the two default constants.

2. Pacing cannot fix the subscription cardinality limit

MAX_SUBSCRIPTIONS in the REQ handler is 1024 per connection. Above that the relay answers error: too many subscriptions. Nothing in mobile/lib refers to that cap.

An account with more than 1024 channels therefore cannot fit, whatever the admission rate. This change makes request 1025 and later arrive politely instead of all at once, and the relay still refuses them. The quarantine blocker makes the refusal permanent for the session, which jedwards27 covers.

We raise the cap only to bound the claim in the PR body. A large account needs a different subscription model, for example multiplexed #h filters or a relay-side change. That is separate work, not this PR.

3. The new tests are real, but they pin the wrong half of the change

We mutation-tested the new suite with controls first, including an inert control and a known-kill control.

Removing pacing entirely is caught, by the task-laziness assertion in the channels provider test. So the tests are not decoration. The gap is that they observe laziness, and not concurrency or spacing. Each of the following kept the suite at 1371 passing:

  • removing the concurrency ceiling, so every task starts at once
  • maxConcurrent + 1
  • replacing the start interval with Duration.zero
  • setting the call-site interval to zero
  • raising call-site concurrency from 4 to 1024
  • making the eight second budget per batch instead of shared, which is 8 seconds against 128 seconds of startup
  • the chunk boundary comparison, which adds an empty batch at exact multiples of 100

The maxActive == 3 assertion is a consequence of the six-task fixture, not a ceiling check. It does not move when the ceiling is raised. The injected delay ignores its Duration argument, so no current test can observe spacing. Two of these results were re-measured on a second, independent rig.

Smallest set of additions that closes the gap:

  • peak in-flight tasks never exceed maxConcurrent, using a slow-task rig. A rig with 12 gated tasks reports a ceiling of 4 on pristine code and 12 with the ceiling removed, so it discriminates.
  • observed spacing, by recording the Duration passed to the injected delay
  • a chunk case at an exact multiple of 100
  • the eight second budget is shared across batches rather than applied per batch
  • a provider case with more channels than the worker limit where the first workers never receive EOSE, which is the regression jedwards27 asked for

Also confirmed correct: chunkChannelsForLastMessageQuery never drops or duplicates a channel at 0, 1, 99, 100, 101, 200, 201, 301, and 1565 inputs. The eight second budget is genuinely shared in the shipped code. Max timestamp semantics are pinned. Narrowing catch-up scope to ready channels is a real improvement over the base. The Rust change is test-only, and its new fan-out test has real power: it reds under a type-preserving cut in extract_channel_id_from_filters.

4. Not charged to this PR

  • cargo test -p buzz-relay --lib has one failure at this head, in the mesh demo tests. The same failure reproduces identically at the base, so it is not yours. Please do not let it become a blocker here.
  • POST /query still does not await the rate limit gate, unlike fetchHistory, the live replay paths, and the CLOSED retry. Pre-existing, and this change improves the surrounding behavior.
  • Unread catch-up still sends one filter per channel in a single request. Pre-existing. Worth a follow-up, and worth noting that catch-up is the fallback this design leans on.

Full mobile suite is 1371 passing at this head, which matches your PR body. flutter analyze reports no issues.

Bound live subscription readiness waits so missing EOSE frames cannot occupy
all paced workers. Retry relay capacity closures after a bounded cooldown and
raise the advertised per-connection limit to cover the motivating account.
Add regression coverage for timeout recovery, pacing, chunking, and capacity.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
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