fix(mobile): recover and pace live subscriptions - #3053
Open
brow wants to merge 8 commits into
Open
Conversation
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Contributor
Author
|
@builderbot review |
Contributor
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
brow
changed the title
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
brow
marked this pull request as ready for review
July 27, 2026 03:01
wesbillman
requested changes
Jul 27, 2026
wesbillman
left a comment
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
P2 — Restore the previously visible channel when a nested channel route is popped.
ChannelDetailPage sets the session's singleton visible channel on mount, then unconditionally clears it on disposal. Channel links push a second ChannelDetailPage over the still-mounted first page. In an A → B → back-to-A flow, disposing B writes null; A becomes visible again but never remounts, so its effect does not rerun. A subsequent reconnect therefore loses A's first-batch replay priority in _replayLiveSubscriptions.
Please make visibility route-aware (or restore the prior active channel) rather than treating every page disposal as “no channel visible,” and add a navigation-stack regression test covering A → B → pop → reconnect ordering. The current relay test only calls setVisibleChannelId directly, so it cannot catch this lifecycle failure.
Reviewed at exact head fc2f4e5d928d89a1dc7b59de59d8527175af4c63. Mobile CI and all required checks are green; I did not duplicate CI locally.
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Contributor
Author
|
🤖 Addressed the nested navigation lifecycle: visible-channel priority is now owner-scoped, so popping B releases only B and restores A for reconnect replay. Added regression coverage for A → B → pop → reconnect, route replacement, nested ownership, and stale or out-of-order disposal. |
ss251
added a commit
to ss251/buzz
that referenced
this pull request
Jul 28, 2026
…relay session Closes gaps 3, 4 and 5 of the mobile relay client's connection resilience (block#3224), all in mobile/lib/shared/relay/relay_session.dart. Gap 1 (stall detection) is PR block#3231's and gap 2 (timeouts) needs a design decision; neither is touched here. Gap 3 — reconnect backoff had no jitter. Each scheduled wait is now randomised by ±20% via `_jitteredDelay`, mirroring the desktop's `jittered_duration` (crates/buzz-acp/src/relay.rs). The ladder position `_reconnectDelayMs` stays un-jittered and jitter is applied per scheduled wait, matching the desktop's split between `backoff_step` and the jitter applied at call time. Without this a fleet of clients reconnects in lockstep after a relay blip. Gap 4 — the backoff ladder reset on connect rather than on stability, so a socket that died seconds after connecting reset to base on every cycle: the doubling never accumulated and a flapping link hammered the relay indefinitely. `_handleConnected` now arms a 60s timer (`_stableConnectionMs`, mirroring the desktop's `STABLE_CONNECTION_SECS`) that resets the ladder only if the connection survives; the timer is cancelled on disconnect, pause and dispose. There are three reset sites and only one changed. The two left resetting immediately are deliberate, and each now says why in a comment: - `_handleConnected` (was relay_session.dart:378) — CHANGED to stability-gated. This is the automatic reconnect path and the only one that can self-spin. - `reconnect()` (:311) — unchanged. Caller-driven, so it cannot loop on its own. - `onAppResumed()` (:344) — unchanged. The preceding disconnect was our own backgrounding, not relay trouble, and the user is looking at the app; making them wait out a ladder the relay never asked for would read as a hang. The desktop has no equivalent of the latter two (no app-backgrounding concept), so desktop parity does not decide them; this is the conservative reading, being the smallest behavioural change that closes the gap. Gap 5 — NOTICE frames were dropped by `_handleMessage`, so the relay's explicit "slow down" was discarded. Since gaps 3+4+5 compound, the client could not hear the rate limit it was provoking. NOTICE is now logged via debugPrint, and a `rate-limited:` notice raises the reconnect backoff floor from the relay's own `retry in Ns` hint: absent or sub-2s hints floor to 5s and the value is clamped to `_maxReconnectDelayMs`, both mirroring the desktop's `set_rate_limit_gate`. It takes the maximum against the current ladder, so overlapping notices can never shorten a longer backoff already in place. Gap 5 deliberately feeds the existing `_reconnectDelayMs` ladder rather than introducing a gate object. PR block#3053 (open, unmerged) adds a RelayRateLimitGate driven by CLOSED frames and rewrites `_handleConnected`; it contains no NOTICE handling and no jitter, so these gaps are unclaimed. Routing NOTICE through the ladder keeps this free of a hard dependency on block#3053 while pointing at the same backpressure concept, so NOTICE can be redirected into that gate in a small follow-up if block#3053 lands. Deviation from the work order worth flagging to review: the order specified ±25% jitter as the desktop's value. The desktop's `jittered_duration` actually uses a factor of [0.8, 1.2) — ±20% — so ±20% is implemented for real parity. Tests: 11 new cases in mobile/test/shared/relay/relay_session_test.dart. Each of the three fixes was negative-controlled by reverting only its lib/ change and confirming the new tests fail — jitter neutralised gives a flat 1000ms, the reset-on-connect restored collapses the ladder to 2000 where 4000 is expected, and the NOTICE case removed leaves the backoff at 1000. `Random` is now injectable through the constructor (alongside the existing `httpClient` and `socketFactory`) so the jitter assertions are deterministic rather than timing dependent. Gate from mobile/: dart format --set-exit-if-changed clean, flutter analyze clean, flutter test 834 passed / 1 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HU2jkzBjmJSBGz2ciGFdej Signed-off-by: ss251 <ss251@uw.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Mobile now recovers live subscriptions after retryable or rate-limited relay
CLOSEDresponses. It ports the existing desktop model: classify terminal versus retryable closures, honor retry hints through a session-owned rate-limit gate, retry with bounded backoff, and replay visible-channel subscriptions first in bounded batches.Channel refreshes also retain unchanged live subscriptions instead of clearing and recreating them. This is desktop parity, not a new relay policy.
Why?
On reconnect or resume, mobile replayed its retained live subscriptions while
channelsProviderindependently cleared and recreated roughly the same set, alongside unread catch-up and open-channel requests. The relay allows 50 REQs per 5 seconds, so users in many channels could predictably exceed the budget. In live reproduction, 55 subscriptions produced 9 rate-limit closures, 60 produced 18, and 80 produced 36.Mobile then treated every live
CLOSEDas terminal, removed the affected subscription, and never restored it. Channel updates could remain dead until a later session reconstruction. This is the primary causal chain behind BOT-1449.Desktop already handles this as normal transient pressure by classifying closures, gating and backing off retries, pacing reconnect replay, and retaining unchanged subscriptions. This change brings mobile to the same recovery model while removing the avoidable request burst.
How is it tested?
Full mobile suite: 721 passed, 1 skipped. Analyzer and formatting checks pass. Required CI checks pass.
Added and updated tests cover
CLOSEDclassification, retry hints, rate-limit gating, bounded retry and reset behavior, terminal failures, timer cleanup, history gating, visible-first batched replay, and retention of unchanged subscriptions.