fix(mobile): bound startup channel relay load - #5831
Conversation
a0ddbb8 to
39d6d38
Compare
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>
39d6d38 to
10077f2
Compare
jedwards27
left a comment
There was a problem hiding this comment.
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.
|
Supplemental lifecycle finding at the same exact head, independently probed by Princess Donut and confirmed against the production/test paths: A post-ready terminal 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 Related test-fidelity gap: |
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>
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.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. The findings in the earlier review/comment are addressed at
I dismissed my stale changes-requested review against |
jedwards27
left a comment
There was a problem hiding this comment.
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 (
unawaited→await): 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.
|
🤖 Additive review notes for #5831 at head 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 theoryThis is the part we think is new. The relay can drop an EOSE frame while leaving the socket healthy.
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 We measured the client half on two separate rigs. With four never-EOSE channels out of twenty and 2. Pacing cannot fix the subscription cardinality limit
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 3. The new tests are real, but they pin the wrong half of the changeWe 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:
The Smallest set of additions that closes the gap:
Also confirmed correct: 4. Not charged to this PR
Full mobile suite is 1371 passing at this head, which matches your PR body. |
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>
Summary
#hlive subscriptions through four paced workers with 125 ms start spacing, while overlapping admission with the startup snapshotCLOSEDresponsesProblem
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
subscribeWhenReadyresolves only after EOSE; retryable/rate-limitedCLOSEDstays pending through replayCLOSEDbefore or after readiness cannot leave stale provider state; attempt identity prevents an obsolete callback deleting a replacementlastMessageAtValidation
Exact commit:
a9a786ddfabea27552a6633494c789ffd2ad9a80channels_provider.dart999 lines,relay_session.dart996)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.