Repository navigation
fix(mobile): stop host streams after relay subscription cancellation - #18926
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughMobile relay streams now track sent and snapshot states and maintain cancellation records. Cancellation builds request-specific unsubscribe payloads, defers cleanup until required responses arrive, ignores late responses, and preserves live sibling streams. Browser screencast cancellation tests verify host shutdown and zero frame sends. Session failure now clears streams before link closure, preventing late event delivery to released listeners. Merge Risk: ⚪ Minimal · up to Mobile relay stream cancellation now releases host-side subscriptions after client unsubscribe while preserving active shared subscriptions. The covered cancellation, failure, and cleanup paths show no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed, relevant, and explains the behavior, implementation, measurements, testing, and visual-proof rationale. However, it omits the required Linked Issue entry and does not complete several template sections or checklist items.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
MobileRelayRpcStreamscancellation rewrite — cancel now builds the correct host unsubscribe per method, retains only a cleanup route ({ method, unsubscribe? }) in acancelledSubscriptionsmap while an already-sent subscription awaits its server-assigned subscription id, and releases listeners on relay failure.buildStreamUnsubscriberequest-scoped identity — gains an optionalrequestIdsosession.tabs.unsubscribecarriessubscriptionIdand evicts only the originating subscriber (not a same-worktree sibling); the direct client's 2-arg call is unchanged.stream.sent/receivedSnapshotgates — unsent requests never send an unsubscribe; a pre-snapshotsession.tabscancel defers its unsubscribe until the host's initial snapshot resolves cleanup registration.fail()clears streams —mobile-relay-rpc-session.tsnow callsstreams.clear()on failure, matching theclose()path so disposed listeners never fire on a late response.
The fix is well-scoped and the tests are strong: exact-equality assertions (not loose truthiness), a deterministic production-controller budget test that pins retained subscriptions/frames/bytes to zero after cancellation for both ready and late-ready timings, sibling-tab isolation, and the repeated late-ready cycle. I verified the host side accepts each cleanup shape (session.tabs.unsubscribe request-scoped subscriptionId, nativeChat.unsubscribe, browser.screencast.unsubscribe/runtime.clientEvents.unsubscribe), and the only wire change is an optional field on an existing method, which is compatible. No blocking or minor issues to raise.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
nativeChat and terminal unsubscribe tokens are deterministic per view target, and the host evicts on duplicate registration. Skip the unsubscribe RPC while a live sibling on the same connection still owns that token.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This delta adds a sibling-preservation guard on top of the prior cancellation rewrite, so cancelling one stream no longer evicts a live sibling that shares a deterministic host cleanup token.
sendUnsubscribe/hasLiveOwnerguard — before emitting an unsubscribe, the client now checks whether a live, sent sibling derives the same{ method, params }token and skips the frame if so, keepingnativeChat(agent:sessionId) andterminal(terminal:clientId) sibling registrations alive until the last subscriber unmounts.buildParamsUnsubscribeextraction — unifies terminal + generic unsubscribe derivation behind one helper, with a newStreamUnsubscribetype; the deferredsession.tabspath andbrowser.screencast/runtime.clientEventspaths are unchanged.- Sibling tests — new
it.eachcoverage fornativeChat/terminalsibling preservation plus the unsent-sibling case, asserting the unsubscribe is still sent when the would-be owner was never sent to the host.
The guard is correctly scoped: session.tabs never false-matches because its unsubscribe embeds the request-scoped subscriptionId, and browser.screencast/runtime.clientEvents never match because buildStreamUnsubscribe returns null for them (their cleanup uses a unique server-assigned id). Unsent siblings are excluded, matching the fact that the host has no cleanup registration to evict. I traced the token shapes through buildTerminalUnsubscribeParams and buildNativeChatUnsubscribe and confirmed the JSON.stringify comparison is deterministic for these fixed param shapes.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…tablyai#18926) * fix(mobile): release cancelled relay stream subscriptions * fix(mobile): keep shared-token relay siblings live on unsubscribe nativeChat and terminal unsubscribe tokens are deterministic per view target, and the host evicts on duplicate registration. Skip the unsubscribe RPC while a live sibling on the same connection still owns that token.
ELI5
Leaving a mobile browser view could stop showing its pictures without telling the host to stop making and sending them. The relay client built the wrong browser unsubscribe method and forgot subscriptions cancelled before the host returned their cleanup ID. Tab and native-chat subscriptions also missed their existing cleanup RPCs.
Reuse the existing unsubscribe builders, retain only cleanup routing metadata while an already-sent subscription becomes ready, and release listeners on relay failure. Tab cleanup uses the original request ID so one subscriber cannot stop another subscriber to the same workspace; early tab cancellation waits for the host's initial snapshot/cleanup registration. Cancelling an unsent request sends nothing.
Measurements
Production
MobileRelayRpcStreamsconnected to the productionRuntimeBrowserScreencastController, with a controlled frame producer and in-memory transport. After cancellation the producer attempts 100 frames of 65,536 bytes if it is still running. This is a deterministic resource budget, not a measured network rate or device battery estimate.Both cancellation timings reproduce: after ready, and before the client receives ready. The checked-in budget tests fail against original
mobile-relay-rpc-streams.tsfromd8c4c830639bf3d603c9250a536e0323ca0a7a19, reporting all three before counts, and pass with this change.Reproduce:
pnpm --dir mobile test src/transport/mobile-relay-browser-cancel-budget.test.ts. Run the same test with only that production stream file restored from the baseline to reproduce the failure.Validation
39 mobile stream/session/cancellation tests pass in the final focused run, including 100 late-ready cancellation cycles, exactly-once cleanup, unsent requests, error/end/completion/disconnect, native-chat/terminal cleanup identities, and sibling tab subscriptions. Three existing host session-tab unsubscribe tests pass. Mobile typecheck, targeted lint, formatting, and diff checks pass.
Uses existing RPC methods and optional request-scoped cleanup fields; no new stream opcode, capability, or host schema. The existing parent revision's session-tab schema/handler accepts request-scoped cleanup. Execution stays with the owning host; cancellation unsubscribes the view and does not terminate remote PTYs or agent work. Folder workspaces use the same workspace selector path.
Visual proof: N/A — active-view rendering is unchanged; the regression is work continuing after its viewer has unsubscribed. The production-controller budget test verifies that cleanup stops that work. No foreground app or mobile simulator launched.
Negative user-facing trade-offs
None identified. Active subscriptions keep receiving their existing data and same-workspace sibling subscriptions remain active. Pending cancellation retains only request/method/cleanup identifiers until the reply, completion, or disconnect; it does not retain disposed view listeners or frame buffers. No polling delay, shortened output, or cache freshness change.