perf(mobile): reduce cold startup and channel rendering delays - #6996
Conversation
Publish the channel snapshot before live setup and batch subscriptions into deterministic relay-safe chunks. Install replacements before retiring old coverage, and fence pending work across scope and lifecycle changes. Limit custom-emoji Markdown matching to shortcodes used in the rendered content rather than the full community palette. Honor explicit zero retry hints without introducing the fallback global rate-limit delay. Add lifecycle, rate-limit, and emoji regression coverage. Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Co-authored-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 58cc4b7e9be71223f7898ea5f930a4248830ccfb..13a83b628c8411c5885e6f76a250ba87accf6067 (exact head 13a83b628c8411c5885e6f76a250ba87accf6067)
Risk: high — Mobile startup and live channel delivery now depend on chunk ownership, replacement ordering, stale-callback fences, and bounded failure recovery.
Blocking findings
[P2] Terminal CLOSED leaves false chunk ownership and can permanently stop live delivery
mobile/lib/features/channels/channels_provider_lifecycle.dart:79-124 installs each chunk with session.subscribe and records only its unsubscribe closure. mobile/lib/shared/relay/relay_session.dart:713-722 removes a terminally closed subscription internally, but the provider receives no status/closure notification. Later synchronization sees the retained chunk key at lifecycle line 81 and skips replacement as though coverage remained alive. With this PR’s batching, one stale ownership record can silently stop live delivery for up to 128 channels; during replacement, the falsely retained new key can also satisfy coverage accounting and retire working old chunks.
Author action: track terminal closure with generation-safe ownership: remove only the matching installed key/closure and trigger bounded reconciliation. Add a regression for terminal closure after settled/fallback setup proving refresh reinstalls coverage and does not retire a working fallback chunk.
Verification owner: author for the mutation-proven regression; reviewer for callback/cleanup ordering and the full Mobile package rerun.
[P2] Repeated partial replacement failure retains subscriptions without a bound
channels_provider_lifecycle.dart:153-160 computes coverage only from exact current desired chunk keys. Lines 161-174 then retain every obsolete chunk containing any desired ID not covered by those successful current keys. Repeated sorted-boundary shifts while one replacement chunk fails can retain another obsolete 128-channel subscription every cycle, growing provider map entries, relay live subscriptions/REQs, callback fan-out, and reconnect replay indefinitely. A model of the exact rule grew retained chunks 2→3→4→5→6→7→8→9 over seven partial-failure/front-insertion cycles.
Author action: bound fallback ownership per uncovered channel/range and retire redundant obsolete chunks while preserving install-before-retire coverage. Add a multi-cycle churn/failure regression asserting complete desired coverage and a bounded active-subscription count.
Verification owner: author for the causal regression; reviewer for resource-bound and replacement-order verification.
Other traced contracts
Snapshot publication no longer waits on live setup and remains stale-refresh fenced. Relay/identity/generation/desired-channel callback filters, disconnect/disposal cleanup, deterministic 128-ID chunking, and explicit retry-hint 0 behavior appear sound in the searched paths. The content-bounded emoji matcher preserves unknown/code literals, event-tag URL priority, edits, colon boundaries, case matching, and accessible fallback semantics. The product lane found no separate visible/accessibility defect.
Validation at matching exact head:
- full Mobile
flutter test— 1,890 passed in both independent lanes. just mobile-check— pass; 506 files unchanged, analyzer clean.git diff --check— pass.- snapshot-blocking and full-palette matcher mutations fail causally as intended.
- live exact-head Mobile, Desktop, macOS build, integration, DCO, and applicable checks are successful.
Manual/native evidence: no independent simulator launch, release-profile run, physical-device run, or raw performance-receipt inspection.
Residual risk: the reported latency cohorts are debug-simulator measurements with three samples per cohort and were not independently reproduced. This performance/native gap is reviewer/performance-owner verification, not additional author rework; the two source-established lifecycle defects independently require changes.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 58cc4b7e9be71223f7898ea5f930a4248830ccfb..13a83b628c8411c5885e6f76a250ba87accf6067 (exact live head 13a83b628c8411c5885e6f76a250ba87accf6067)
Risk: high — this changes relay subscription ownership, replacement, retry, and reconnect behavior for the mobile channel list.
Blocking findings
-
A terminal relay
CLOSEDleaves false provider ownership and can permanently stop live delivery for a 128-channel chunk.mobile/lib/features/channels/channels_provider_lifecycle.dart:79-124installs throughsession.subscribeand records only an unsubscribe closure; subsequent syncs skip a retained key at line 81. However,mobile/lib/shared/relay/relay_session.dart:713-722removes a terminally closed subscription internally without notifying this owner. The provider can therefore continue treating dead coverage as installed. During replacement, that false key may also count as coverage and cause working fallback coverage to be retired.Author action: track terminal closure with generation-safe ownership, removing only the matching installed key/closure and triggering bounded reconciliation. Add a regression for terminal closure after settled and fallback setup that proves coverage is reinstalled and working fallback coverage is not retired.
-
Repeated partial replacement failures retain subscriptions without a resource bound.
mobile/lib/features/channels/channels_provider_lifecycle.dart:153-160counts coverage only from exact current desired keys, while lines 161-174 retain every obsolete chunk containing any desired ID not covered by those keys. Repeated sorted-boundary shifts with one failing replacement can retain another obsolete 128-channel subscription each cycle, growing provider entries, relay subscriptions/REQs, callback fan-out, and reconnect replay indefinitely. A model of this cleanup rule grew retained chunks2→3→4→5→6→7→8→9over seven churn/failure cycles.Author action: bound fallback ownership per uncovered channel/range and retire redundant obsolete chunks while preserving install-before-retire coverage. Add a multi-cycle churn/partial-failure regression asserting both complete desired coverage and a bounded active-subscription count.
Verification owner: author for causal regressions; reviewer reruns the full mobile package and re-traces terminal-close/replacement ordering at the new exact head.
Validation and non-blocking review results
- Two independent exact-head runs of the full mobile package passed:
flutter test, 1,890 tests. - Two exact-head runs of
just mobile-checkpassed: 506 files unchanged and analyzer clean. git diff --checkpassed.- Exact-head GitHub
MobileCI is successful; authenticated reviewerjedwards27is distinct from authorwesbillman. - The finite snapshot no longer waits for live setup, and mutation evidence shows the cold-list regression fails when the blocking await is restored.
- Product/UI and emoji review found no concrete defect: content-bounded emoji matching remains case-insensitive, preserves unknown/code text and accessible fallback semantics, and mutation evidence made six focused tests fail when full-palette matching was restored.
- Explicit retry hint
0preserves the ordinary 1-second live retry backoff without incorrectly opening the 10-second session gate.
Manual/native evidence: no physical-device or release-profile run was independently performed. The reported simulator measurements are debug-only and were not independently reproduced; this is a confidence gap, not the reason for the verdict.
Residual risk: native/release latency, flicker, and accessibility-tree behavior remain unwitnessed. Local just file-size-check also selected a moving origin/main base and reported an unrelated Desktop ratchet; exact-head GitHub changed-path gates are green, so that reviewer-tooling mismatch is not an author defect.
Replace terminally closed live channel chunks through the serialized backstop path, fencing stale closure callbacks by generation. Retain only greedy fallback coverage after partial replacement failures so repeated churn cannot accumulate redundant relay subscriptions. Add terminal-closure and repeated partial-failure regressions. Co-authored-by: Carl <9d00794d3df50972eb8b615511783cab12a77a8fd5dd5edd58073ec73b54bd8b@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Implemented the requested live-subscription lifecycle fixes in
|
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 58cc4b7e9be71223f7898ea5f930a4248830ccfb..a1ac2f68355738363923592d23633801c9cc36a7 (exact head a1ac2f68355738363923592d23633801c9cc36a7)
Risk: high — Mobile live-subscription ownership, recovery loops, network/battery amplification, and snapshot→live continuity.
Behavior/contracts traced: finite snapshot publication; chunked live subscription installation/replacement; terminal-close ownership; generation races; install-before-retire coverage; repeated partial-failure churn; reconnect/disposal cleanup; retry-zero semantics; emoji matching; resource bounds.
Blocking finding (P2): persistent terminal CLOSED can self-amplify into an unbounded immediate refresh/re-subscribe loop. RelaySession intentionally treats terminal closure as non-retryable, invokes onClosed, removes the subscription, and returns (mobile/lib/shared/relay/relay_session.dart:713-722). The provider callback removes ownership and immediately requests reconciliation (mobile/lib/features/channels/channels_provider_lifecycle.dart:162-170). Reconciliation immediately performs full _backstopRefresh() work and loops when another closure arrives during the run, without delay, ceiling, or a meaningful state-change prerequisite (channels_provider_lifecycle.dart:172-192; channels_provider.dart:767-786). A relay that persistently rejects the unchanged filter can therefore drive unbounded membership/history requests, live REQs, callbacks/logs, radio, and battery use. The added regression closes only once and lets replacement succeed (mobile/test/features/channels/channels_provider_live_cases.dart:225-257), so it does not constrain this failure mode.
Author action: back off and bound terminal-close recovery, and require a meaningful lifecycle, membership, reconnect, or policy transition before repeatedly reinstalling the same terminally rejected chunk. Add a deterministic persistent-terminal regression that rejects every replacement over multiple attempts and asserts bounded membership requests, subscriptions/REQs, timers, and logs while the finite snapshot remains usable. Mutation-prove it.
Verification owner: author for fix and causal regression; reviewer for lifecycle-policy re-trace, mutation, full Mobile suite, and exact-head freshness.
Resolved former findings: generation-safe terminal ownership removal now permits one-shot reinstall without stale callbacks clobbering newer ownership; greedy fallback retirement preserves desired coverage while bounding obsolete chunks. Both regressions bite under independent mutations.
Validation: both assigned lanes passed the full Mobile suite at the clean exact head (1,892 tests) and just mobile-check (506 files unchanged; analyzer clean). Focused provider suite passed 64 tests. Removing terminal ownership recovery failed coverage; restoring the old fallback algorithm exceeded the asserted bound. git diff --check passed. Exact-head CI was still running with no observed failing check when integrated.
Manual/native evidence: no native/release cold-start, visual/AX, or physical-device performance rerun.
Residual risk: performance claims remain unwitnessed on release hardware, but that confidence gap is not the basis of this request.
— :bot: Jude’s code review agent
|
|
||
| void _handleLiveChunkClosed(String chunkKey, int generation, String message) { | ||
| final subscription = _liveSubscriptionsByChunk[chunkKey]; | ||
| if (subscription == null || subscription.generation != generation) return; |
There was a problem hiding this comment.
🤖 Non-blocking (test adequacy, not a defect): the two guards new in this commit are unfalsifiable by the current suite. I mutated each one separately at a1ac2f68 and the full mobile suite stayed green (1892/1892 both times), with an inert control mutant surviving as expected and a positive control confirming the rig can see this file: deleting the onClosed: wiring that reaches _handleLiveChunkClosed is killed by terminal closure reinstalls desired coverage without retiring fallback.
Surviving mutant 1, this line: dropping || subscription.generation != generation (keeping only the subscription == null check) changes nothing observable. This is the guard that stops a late CLOSED for a superseded chunk generation from evicting the live replacement that now owns the same chunkKey and then triggering a reconcile. Since _syncLiveSubscriptions reuses chunkKey across replacements and installs the new _LiveChunkSubscription before awaiting session.subscribe, a CLOSED still in flight from the old socket is exactly the case the generation compare exists for.
Surviving mutant 2, line 179: dropping _liveReconcileRequested = true (so a reconcile requested while one is running is simply discarded) is also invisible. That flag is the coalescing half of _requestLiveReconcile: without it, a CLOSED arriving during an in-flight _backstopRefresh is dropped and the chunk it evicted from _liveSubscriptionsByChunk stays uncovered until the 60s backstop timer fires. That is a silent up-to-60s live-delivery gap, which looks like the failure mode this commit exists to fix.
Both are reachable through the seams the PR already has (closeSubscriptionContaining plus the existing pauseNextSubscribe / subscribeFailures fakes), so each should be cheap to pin: fire a CLOSED carrying a stale generation and assert coverage plus activeSubscriptionCount are unchanged, and fire a CLOSED while a reconcile is parked and assert the second reconcile still runs.
For attribution: I also found that dropping _liveSubscriptions[subId] != liveSub from the post-gate recheck in relay_session.dart _handleClosed livelocks the closed-retry replay (a pending entry _sendReplayBatches skips but never removes, so the finally reschedules unboundedly; my probe hung at 300s vs 3s pristine). That guard and its blind spot are byte-identical at this PR's merge base e76c8196 and reproduce there identically, so it is pre-existing and explicitly not charged to this PR.
Honor RelaySession terminal admission failures instead of immediately refreshing memberships and reinstalling the unchanged filter. Quarantine rejected desired chunks until the filter, scope, or connection changes. Keep periodic membership discovery and transient session retry policy. Add fake-clock regressions for persistent rejection before/after readiness, bounded requests and timers, reconnect recovery, changed membership, stale callbacks, scope isolation, and pending-close disposal. Preserve fallback coverage until reconnect can restore the rejected chunk. Co-authored-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Pinky is commenting on Wes’s behalf. Addressed the remaining persistent-terminal-CLOSED finding in review #6996 (review) with c9f71c9. Rather than add another retry policy, the provider now respects RelaySession’s terminal classification: it removes ownership and quarantines that exact desired chunk. Ordinary refreshes/polls cannot resubmit the unchanged rejected filter. A changed chunk, community/identity scope, or a new authenticated connection permits another attempt. Obsolete quarantine keys are discarded, existing fallback coverage is retained, and transient/rate-limit retry behavior remains owned by RelaySession. No new production retry timer; the existing 60-second membership discovery remains. Validation
No ML Kit/native changes included. No native-app relaunch or new physical-device performance measurement for this correction. GitHub CI/re-review at this new head is not yet verified; this comment does not approve or merge the PR. |
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 58cc4b7e9be71223f7898ea5f930a4248830ccfb..c9f71c90413d3aa2da1b0858e1b693eee62dce12 (exact live head c9f71c90413d3aa2da1b0858e1b693eee62dce12)
Risk: high — mobile channel startup now decouples finite snapshot publication from relay subscription setup and changes chunk ownership, terminal closure, replacement, and retry behavior.
Findings: no unresolved author-actionable defect. The prior persistent terminal-CLOSED amplification blocker is resolved:
mobile/lib/features/channels/channels_provider_lifecycle.dart:87-90,171-187removes only generation-matching ownership, quarantines a still-desired terminally rejected chunk, and does not launch another refresh/REQ loop.channels_provider_lifecycle.dart:26-32,238-245bounds quarantine to desired chunk keys and clears it on teardown; stale callbacks are generation-fenced.mobile/lib/features/channels/channels_provider.dart:112-123clears quarantine only after a new authenticated connection, allowing one fresh admission attempt per connection rather than internal self-amplification.mobile/lib/shared/relay/relay_session.dart:716-720,728-772keeps terminal closure non-retrying while transient/rate-limit closure retains bounded backoff.- Replacement cleanup at
channels_provider_lifecycle.dart:189-235retains old subscriptions only while necessary for desired-channel coverage, avoiding the former unbounded obsolete-chunk growth.
The deterministic provider/session tests cover persistent closure before/after readiness, repeated ordinary/manual/timer refresh, reconnect-only recovery, changed filters, community/identity isolation, stale callbacks, disposal, bounded counters/timers/logs, chunk caps, and fallback cleanup. Emoji widget/matcher coverage preserves large-palette, event-tag precedence, edits, code literals, unknown/case, adjacency, and accessible fallback behavior. Explicit retry hint 0 preserves the normal one-second live retry without creating the ten-second session gate.
Author action: none.
Verification owner: Mobile CI owns its still-running exact-head gate; release/performance validation owns native-device and release-profile observation.
Validation at matching clean HEAD:
- Independent full Mobile runs:
cd mobile && flutter test— 1,900 passed in each review lane. - Focused production-boundary runs: 71 passed and 127 passed respectively.
just mobile-check— formatting unchanged; Flutter analyzer clean.git diff --check 58cc4b7e...c9f71c90— pass.- Live PR head and reviewed head both
c9f71c90413d3aa2da1b0858e1b693eee62dce12; PR open; authenticated reviewerjedwards27differs from authorwesbillman.
Manual/native evidence: no physical-device/release cold-start, long-channel scrolling, offline→online, background/resume, visual, or accessibility-tree run was performed.
Residual risk: production latency and frame-time claims remain unwitnessed in release/native shape. _referencedUrls still linearly scans the full community emoji palette per rendered message; correctness and regex-cardinality reduction are tested, but large-channel frame time is not independently benchmarked. Mobile CI is still running at submission time; this is an external gate, and author action is required only if it reports a PR-caused failure.
* origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…enericize * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…agent-edit * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…age-rw * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Pinky is opening this PR on Wes’s behalf.
Summary
Reduce two separately measured mobile delays without changing the relay API or removing rich message rendering:
Matched performance results
Medians of three before and three after process-cold launches, alternated on the same authenticated iPhone 17 Pro / iOS 26.5 simulator. Before is mobile source at
e76c81968b65b0755b83efdd59dc3375c59ddf40; after is this production patch before two documentation-only comment fixes.First channel-list frame: 11.617s → 3.179s · 73% lower latency
Live setup duration: 8.475s → 0.185s · 98% lower latency
Channel-open first message-list frame: 2.754s → 1.230s · 55% lower latency
Message data ready → first frame: 1.977s → 0.286s · 86% lower latency
Channel-open reveal complete: 2.845s → 1.394s · 51% lower latency
Channel-open data readiness: 0.770s → 0.944s · 23% higher latency
The gain is client-side orchestration/rendering, not a claim that the relay became faster. First channel-list frame ranges were 10.835–11.788s before and 2.872–3.395s after; channel-open first-frame ranges were 1.560–2.906s before and 1.149–1.317s after.
Measurement boundaries
main; build/install/native pre-main time is excluded. Auth/preferences and OS/disk caches are retained between new processes.Validation
flutter test— 1890 passed.just mobile-check— 506 files unchanged; analyzer clean.just file-size-check— policy tests and all client ratchets passed.git diff --check— passed.13a83b628c8411c5885e6f76a250ba87accf6067, all normal pre-push hooks passed:mobile-checks(formatter, analyzer, and the full 1890-test mobile suite),file-size-check,branch-skew, andpush-head-scope. The commit hook formatted 506 files with no changes. Runtime measurements preceded only the two documentation-comment fixes; no runtime source changed afterward.Limits / follow-ups
RelaySession.subscribestill settles under its existing EOSE/fallback/retryable-CLOSED contract. “Setup completed” is not an unconditional EOSE or live-delivery guarantee. This PR does not add status-aware replacement ownership.Originating Buzz conversation: buzz://message?channel=793b0522-7995-4375-b1a6-fd94a96fa21d&id=6ba88afdec78ab2cfb6728afcd4a6d10f29e6aa33ff0f62f45d6750381e4d789