Mobile state sync v2: per-record deltas replace the invalidate-and-refetch loop - #8284
Merged
Merged
Conversation
Adds a versioned delta protocol for the iOS workspace list. The Mac keeps an epoch + per-collection revision store of typed workspace/group records, answers mobile.sync.fetch with a snapshot or the exact missing span, and pushes mobile.sync.delta events carrying only the rows a change touched. The phone mirrors records with a cursor, projects them through the same applyRemoteWorkspaceList path the legacy full list uses, and stops re-fetching the entire list on every workspace.updated push. Legacy phones and Macs keep today's behavior via method_not_found negotiation. Design doc: docs/mobile-state-sync-v2.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR replaces the invalidate-and-refetch workspace sync loop with a cursor-based delta protocol (mobile state sync v2). The Mac tracks per-collection revisions in a new
Confidence Score: 4/5Safe to merge with the fallback retry timing issue addressed; the delta protocol, mirror logic, and deadline race are well-designed and covered by 33 new tests. The fallback retry loop in Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swift — specifically the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Phone as iOS Phone
participant Mac as Mac MobileStateSyncHost
participant Store as MobileStateSyncStore
Phone->>Mac: mobile.events.subscribe
Mac-->>Phone: subscribe ACK
Phone->>Phone: beginStateSyncNegotiation
Phone->>Mac: mobile.sync.fetch cold start
Mac->>Store: apply rows
Mac-->>Phone: mobile.sync.delta broadcast
Mac-->>Phone: fetch response snapshot rev N
Phone->>Phone: "stateSyncActive = true"
Phone->>Phone: applyStateSyncProjection
loop workspace change 80ms throttle
Mac->>Store: apply rows
Mac-->>Phone: mobile.sync.delta fromRev N toRev N+1
alt delta applied
Phone->>Phone: applyStateSyncProjection
else gap detected
Phone->>Mac: mobile.sync.fetch cursor repair
Mac-->>Phone: fetch response delta or snapshot
Phone->>Phone: applyStateSyncProjection
end
end
note over Phone,Mac: workspace.updated still fires but v2 phone ignores it
note over Phone,Mac: Legacy Mac returns method_not_found and phone stays on refetch loop
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Phone as iOS Phone
participant Mac as Mac MobileStateSyncHost
participant Store as MobileStateSyncStore
Phone->>Mac: mobile.events.subscribe
Mac-->>Phone: subscribe ACK
Phone->>Phone: beginStateSyncNegotiation
Phone->>Mac: mobile.sync.fetch cold start
Mac->>Store: apply rows
Mac-->>Phone: mobile.sync.delta broadcast
Mac-->>Phone: fetch response snapshot rev N
Phone->>Phone: "stateSyncActive = true"
Phone->>Phone: applyStateSyncProjection
loop workspace change 80ms throttle
Mac->>Store: apply rows
Mac-->>Phone: mobile.sync.delta fromRev N toRev N+1
alt delta applied
Phone->>Phone: applyStateSyncProjection
else gap detected
Phone->>Mac: mobile.sync.fetch cursor repair
Mac-->>Phone: fetch response delta or snapshot
Phone->>Phone: applyStateSyncProjection
end
end
note over Phone,Mac: workspace.updated still fires but v2 phone ignores it
note over Phone,Mac: Legacy Mac returns method_not_found and phone stays on refetch loop
Reviews (16): Last reviewed commit: "Re-check v2 authority after the legacy l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+144
to
+185
| /// `MobileSyncDeltaEvent` record type for a `mobile.sync.delta` payload. | ||
| public struct MobileSyncDeltaEventHeader: Codable, Equatable, Sendable { | ||
| public let collection: MobileSyncCollectionID | ||
|
|
||
| public init(collection: MobileSyncCollectionID) { | ||
| self.collection = collection | ||
| } | ||
| } | ||
|
|
||
| /// JSON bridging between the typed frames and the `[String: Any]` payloads the | ||
| /// mobile RPC envelope carries. One round-trip through `JSONSerialization` per | ||
| /// frame; frames are small (changed rows only), so this stays off every hot | ||
| /// path that matters. | ||
| public enum MobileSyncFrameJSON { | ||
| public static func jsonObject(from value: some Encodable) throws -> [String: Any] { | ||
| let data = try JSONEncoder().encode(value) | ||
| guard let object = try JSONSerialization.jsonObject(with: data) as? [String: Any] else { | ||
| throw MobileSyncFrameJSONError.notAnObject | ||
| } | ||
| return object | ||
| } | ||
|
|
||
| public static func decode<Value: Decodable>( | ||
| _ type: Value.Type, | ||
| fromJSONObject object: [String: Any] | ||
| ) throws -> Value { | ||
| let data = try JSONSerialization.data(withJSONObject: object) | ||
| return try JSONDecoder().decode(type, from: data) | ||
| } | ||
|
|
||
| public static func decode<Value: Decodable>( | ||
| _ type: Value.Type, | ||
| fromJSONString string: String | ||
| ) throws -> Value { | ||
| try JSONDecoder().decode(type, from: Data(string.utf8)) | ||
| } | ||
| } | ||
|
|
||
| /// Failure bridging a sync frame to or from the RPC envelope's JSON container. | ||
| public enum MobileSyncFrameJSONError: Error, Equatable, Sendable { | ||
| case notAnObject | ||
| } |
Contributor
There was a problem hiding this comment.
MobileSyncFrameJSON has no cases and exposes only static funcs — this is the static-namespace anti-pattern the cmux-no-ambient-global-state rule flags. The preferred shape is a private/fileprivate file-scope helper (not exported) or, since this needs to be public, a struct with a private init() to prevent accidental instantiation while keeping the type system honest. As a public enum with no cases Swift will warn callers that exhaustive switches are trivially satisfied, and the enum form gives no semantic benefit over a namespace struct.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+13
to
+14
| final class MobileStateSyncHost { | ||
| static let shared = MobileStateSyncHost() |
Contributor
There was a problem hiding this comment.
MobileStateSyncHost introduces static let shared = MobileStateSyncHost() for runtime state that varies per app run (the epoch, the store, the preview cache). Per the cmux-no-ambient-global-state rule this should be owned by a constructable, injectable type and wired in at the app seam. The custom-learning carve-out for "inherently process-global singletons with injectable seams" likely applies here (the store and mirror are both constructable and tested independently), so this may be intentional — just worth a confirmation that the injectable-seam path is sufficient for the Mac-side integration tests being deferred.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…etches Merges origin/main to pull in the dev-build flakiness fix from #8299. The negotiation fetch now checks client currency and cancellation before sending, so a fetch task from a replaced listener generation can never redial its stale client's route underneath the replacement connection (caught by manualReconnectRedialsWhenLiveStreamIsUnavailableButRPCStateIsConnected after the merge). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesMobile state sync v2
Reconnect deadline handling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant iOS MobileShellComposite
participant Mac MobileStateSyncHost
participant MobileStateSyncStore
participant MobileStateSyncMirror
participant applyRemoteWorkspaceList
iOS MobileShellComposite->>Mac MobileStateSyncHost: mobile.sync.fetch(cursor)
Mac MobileStateSyncHost->>MobileStateSyncStore: fetchResponse(for:)
MobileStateSyncStore-->>Mac MobileStateSyncHost: snapshot or delta response
Mac MobileStateSyncHost-->>iOS MobileShellComposite: fetch response
iOS MobileShellComposite->>MobileStateSyncMirror: apply(response:)
MobileStateSyncMirror-->>iOS MobileShellComposite: applied, staleIgnored, or gap
Mac MobileStateSyncHost-->>iOS MobileShellComposite: mobile.sync.delta
iOS MobileShellComposite->>MobileStateSyncMirror: apply(delta:)
MobileStateSyncMirror-->>iOS MobileShellComposite: gap triggers repair fetch
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (7 errors, 2 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/mobile-state-sync-v2.md`:
- Around line 101-103: Update the outbox coalescing statement in the mobile
state sync documentation by removing the unsupported “1000x smaller” claim or
replacing it with a measured, defensible worst-case ratio. Ensure the
justification for relying on the bounded per-connection event queue does not
depend on an unverified reduction figure.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 6353-6358: Update the mobile state synchronization flow around
beginTerminalEventSubscriptionStart and beginStateSyncNegotiation so negotiation
begins only after ack.isSubscribed succeeds. Keep stream consumption immediate,
but move the negotiation call into the successful subscription-acknowledgement
path and remove the earlier concurrent invocation, ensuring the snapshot fetch
cannot race subscription activation.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+StateSync.swift:
- Around line 32-48: Update handleStateSyncDeltaEvent so known workspace and
group collections schedule a mobile.sync.fetch repair whenever their delta
payload cannot be decoded, instead of silently returning. Preserve ignoring
unknown collections, and use the existing synchronization-fetch mechanism while
retaining normal stateSyncMirror application for valid deltas.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncMirror.swift`:
- Around line 29-32: Update MobileStateSyncMirror’s orderedRecords projection to
avoid sorting recordsByID.values on every read. Maintain a cached ordered-ID
snapshot, rebuild it only when records are added, removed, or their
syncSortIndex changes affect ordering, and have orderedRecords resolve records
through that snapshot while preserving syncSortIndex/syncID ordering.
- Around line 48-55: Update the .snapshot handling in MobileStateSyncMirror to
reject snapshots from the current epoch when payload.rev is older than the
mirror’s current rev, preserving recordsByID, epoch, and rev without applying
the stale response. Keep snapshot application for newer or otherwise valid
revisions, and add a test covering a newer delta arriving before an older
same-epoch snapshot while ensuring the authoritative revision remains current
immediately.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncStore.swift`:
- Around line 134-141: Update the delta construction in MobileStateSyncStore so
removals exclude IDs present in stampedByID, ensuring a record re-added after
rev appears only in upserts and remains live when MobileSyncCollectionMirror
applies the payload. Add a regression test covering remove-then-readd behavior
and verifying the ID is not included in removedIDs.
In `@Sources/Mobile/MobileStateSync.swift`:
- Around line 12-19: Remove the ambient singleton declaration from
MobileStateSyncHost and make the host constructable by an owning process
component. Have MobileHostService or AppDelegate create and retain one instance,
then pass that instance through the observer and RPC paths instead of accessing
MobileStateSyncHost.shared; preserve deltaTopic as a type-level constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a90e5d2-3f4e-4f0a-8dce-91f09d73a6a0
📒 Files selected for processing (18)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncFrames.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncMirror.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncRecords.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncStore.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileStateSyncFrameCodingTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileStateSyncMirrorTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileStateSyncStoreTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncWorkspaceListResponse.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellStateSyncTests.swiftSources/Mobile/MobileHostService+TicketAuthorization.swiftSources/Mobile/MobileStateSync.swiftSources/Mobile/MobileWorkspaceListObserver.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojdocs/mobile-state-sync-v2.md
Comment on lines
+101
to
+103
| - Per-client outbox coalescing for slow phones (today's bounded per-connection | ||
| event queue suffices because delta frames are ~1000x smaller than the | ||
| refetches they replace). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct or qualify the “1000x smaller” claim.
The earlier estimates imply roughly a 50–200x reduction for a single changed row (0.5–1KB versus a 50–100KB full list), not 1000x. Replace this with a measured worst-case figure, or remove the ratio and avoid using it as justification that the bounded per-connection queue is sufficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mobile-state-sync-v2.md` around lines 101 - 103, Update the outbox
coalescing statement in the mobile state sync documentation by removing the
unsupported “1000x smaller” claim or replacing it with a measured, defensible
worst-case ratio. Ensure the justification for relying on the bounded
per-connection event queue does not depend on an unverified reduction figure.
Comment on lines
+29
to
+32
| public var orderedRecords: [Record] { | ||
| recordsByID.values.sorted { | ||
| ($0.syncSortIndex, $0.syncID) < ($1.syncSortIndex, $1.syncID) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid sorting the full mirror on every projection read.
This performs an O(n log n) sort after sync updates over a collection expected to scale toward roughly 1000 workspaces. Maintain an ordered ID snapshot and only rebuild it when additions, removals, or syncSortIndex changes affect ordering.
As per path instructions, production paths over scalable user data must avoid repeated hot-path sorting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncMirror.swift`
around lines 29 - 32, Update MobileStateSyncMirror’s orderedRecords projection
to avoid sorting recordsByID.values on every read. Maintain a cached ordered-ID
snapshot, rebuild it only when records are added, removed, or their
syncSortIndex changes affect ordering, and have orderedRecords resolve records
through that snapshot while preserving syncSortIndex/syncID ordering.
Source: Path instructions
Comment on lines
+12
to
+19
| @MainActor | ||
| final class MobileStateSyncHost { | ||
| static let shared = MobileStateSyncHost() | ||
|
|
||
| /// Event topic v2 phones subscribe to through `mobile.events.subscribe`. | ||
| static let deltaTopic = "mobile.sync.delta" | ||
|
|
||
| let store = MobileStateSyncStore() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move the sync host under an injectable process owner.
static let shared introduces ambient mutable runtime state. Have MobileHostService or AppDelegate construct and own one MobileStateSyncHost, then inject that instance into the observer and RPC path.
As per coding guidelines, “Avoid new ambient global runtime state … and runtime singletons. Prefer constructable injectable owners and private/fileprivate helpers.” <coding_guidelines>
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 13-13: Classes should have an explicit deinit method
(required_deinit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Mobile/MobileStateSync.swift` around lines 12 - 19, Remove the
ambient singleton declaration from MobileStateSyncHost and make the host
constructable by an owning process component. Have MobileHostService or
AppDelegate create and retain one instance, then pass that instance through the
observer and RPC paths instead of accessing MobileStateSyncHost.shared; preserve
deltaTopic as a type-level constant.
Source: Coding guidelines
main's SidebarWorkspaceRowSlotViews.swift fails to compile on the Blacksmith macos-26 runners (escaping-closure capture of 'color' needs explicit self there). Out of this PR's feature scope, but required to produce any dev build of a branch containing current main on that toolchain; behavior unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+99
to
+107
| request, | ||
| timeoutNanoseconds: runtime?.rpcRequestTimeoutNanoseconds | ||
| ) | ||
| guard remoteClient === client, connectionState == .connected, !Task.isCancelled else { return } | ||
| let response = try JSONDecoder().decode(MobileSyncFetchResponse.self, from: data) | ||
| let result = stateSyncMirror.apply(response: response) | ||
| stateSyncActive = true | ||
| switch result { | ||
| case .applied: |
Contributor
There was a problem hiding this comment.
scheduleWorkspaceListRefreshFromEvent correctly suppresses new tasks once stateSyncActive == true, but any workspaceListRefreshTask already in-flight from the negotiation window keeps running. If the Mac changes state between when that legacy mobile.workspace.list request was sent and when mobile.sync.fetch returned its snapshot, the legacy response carries the older state; when it completes it calls applyRemoteWorkspaceList and overwrites the v2 snapshot. The delta stream won't self-correct until the next workspace change on the Mac.
The fix is to cancel workspaceListRefreshTask (currently private var in MobileShellComposite.swift) at the point stateSyncActive = true is assigned. Because the property is file-private, the cleanest path is to add a small internal func cancelLegacyRefetchTask() helper in the main file and call it here before applyStateSyncProjection().
Main fixed the Blacksmith toolchain compile with a [color] capture; drop this branch's interim explicit-self variant so the file matches main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a session-level test: a typed state-sync delta pushed through the independent event stream (the lane an admitted Iroh connection negotiates via iroh_server_events_v1) reaches a mobile.sync.delta topic listener and decodes to the typed frame. Locks in that sync v2 is lane-agnostic on the Iroh transport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Collaborator
Author
|
Rebased onto the Iroh-integrated main (merge of PR 8484 et al.) and verified the protocol against it:
🤖 Generated with Claude Code |
Comment on lines
+111
to
+114
| case .gap: | ||
| // A fetch section can only gap if the store moved between | ||
| // building the response's sections; one repair round covers it. | ||
| scheduleStateSyncFetch(client: client) |
Contributor
There was a problem hiding this comment.
defer
scheduleStateSyncFetch creates a new Task T2 and assigns it to stateSyncFetchTask, but the currently-running fetch task T1 has defer { self?.stateSyncFetchTask = nil } that fires as soon as runStateSyncFetch returns — clearing T2's handle before it can be cancelled. A subsequent beginStateSyncNegotiation sees stateSyncFetchTask == nil, skips the cancel, and creates T3. Now T2 and T3 race: both pass remoteClient === client (connection unchanged) and send independent fetch requests. Whichever applies its snapshot last wins — if T2's older snapshot lands after T3's newer one the mirror regresses. Calling runStateSyncFetch directly keeps the repair inside T1's lifetime, avoids creating T2 entirely, and preserves cancel semantics.
Suggested change
| case .gap: | |
| // A fetch section can only gap if the store moved between | |
| // building the response's sections; one repair round covers it. | |
| scheduleStateSyncFetch(client: client) | |
| case .gap: | |
| // A fetch section can only gap if the store moved between | |
| // building the response's sections; one repair round covers it. | |
| // Call directly (not scheduleStateSyncFetch) so we stay inside | |
| // this task's lifetime: the outer Task's defer clears | |
| // stateSyncFetchTask when runStateSyncFetch returns, which would | |
| // drop the new task's cancel handle before it could run. | |
| await runStateSyncFetch(client: client) |
A transport dial that parks forever (wedged Iroh dial, issue 8531) holds the recovery owner's in-flight claim indefinitely: no failure settles, no backoff retry is scheduled, and every other trigger defers forever. Fails without the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ckoff Every automatic stored-Mac redial now races a per-attempt deadline (runtime-injectable, default 30s). At expiry the attempt is abandoned (generation guards make late completion harmless), settled as timedOut, and transient backoff schedules the next automatic try, so a hung Iroh dial can no longer freeze the recovery machine into a permanent "Disconnected - Tap Reconnect" dead end. The user's explicit reconnect/pull gesture now clears transient backoff the same way recoverMobileConnection(.manual) does, so a recorded cooldown can never swallow a manual tap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # cmux.xcodeproj/project.pbxproj
- Store payloads no longer emit tombstones for ids that are live again (remove-then-readd inside a cursor span would have deleted the re-added record on the client); mirror applies removals before upserts as belt-and-braces. - Same-epoch snapshots older than the mirror cursor are ignored (a stale in-flight fetch response can no longer roll the mirror back). - An undecodable delta for a known collection now schedules a cursor repair fetch instead of leaving the mirror silently stale. - While v2 owns the list, the legacy full-list reload path keeps its liveness-probe role but re-bases the mirror through a cursor fetch instead of overwriting projected state. - The single-flight fetch handle is generation-guarded so a cancelled predecessor's deferred cleanup cannot erase its replacement's handle. - Qualify the payload-reduction claim in the design doc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ck, tombstone rev bound - raceAgainstDeadline no longer uses a task group (which structurally awaits a cancellation-ignoring dial); the operation runs unstructured and a lock-guarded once resumes whichever side finishes first. Direct tests cover an operation that never completes and ignores cancellation. - A transiently failed gap-repair fetch now drops back to legacy list semantics (stateSyncActive off + one authoritative reload) instead of stranding the mirror behind a suppressed refetch loop; v2 re-negotiates on the next listener generation. - Tombstone pruning tracks the highest discarded revision; coverability is judged against that bound so a same-revision batch split by the ring cap can never produce a delta that omits a removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… retries - Deadline races hand back the abandoned operation task; the composite counts unresolved abandoned dials, pauses automatic retries above a small ceiling, and re-arms the retry loop when an abandoned dial finally resolves while still disconnected. A persistently wedged transport can no longer accumulate unbounded retained reconnect tasks. - The legacy fallback reload after a failed gap-repair fetch retries up to three times with short pauses instead of fire-and-forget, and reports exhaustion; connection-death cases remain owned by the recovery paths. Rejected (named boundary): moving MobileStateSyncHost off `shared` — the Mac mobile plane is currently rooted in TerminalController.shared / MobileHostService.shared at every call site, the host's epoch is process-lifetime by design, and the testable sync logic lives in the injected CMUXMobileCore classes; the injectable-owner move rides the de-singletonizing follow-up rather than this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swift (1)
150-167: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip legacy fallback on cancelled state-sync fetches
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swift:150-167:MobileCoreRPCClient.sendRequest(...)can surfaceCancellationErrorfor an aborted request, so a superseded fetch still falls into these catches and incorrectly disablesstateSyncActive/ triggers the legacy reload. Guard!Task.isCancelledbefore falling back so restart-on-newest stays benign.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+StateSync.swift around lines 150 - 167, Update the error handling around the state-sync fetch catches in the state-sync flow to call fallBackToLegacyListAfterFetchFailure only when !Task.isCancelled. Continue logging the error as appropriate, but ensure cancelled or superseded requests do not disable stateSyncActive or trigger the legacy reload; preserve the existing method_not_found handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+StateSync.swift:
- Around line 175-182: Make fallBackToLegacyListAfterFetchFailure(client:) async
and replace its fire-and-forget MainActor Task with a direct await of
reloadWorkspaceListFromMac(). Update both call sites in runStateSyncFetch to
await the fallback method, keeping the existing guards and stateSyncActive
handling unchanged so the reload remains tied to the caller-owned fetch
lifecycle.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+StateSync.swift:
- Around line 150-167: Update the error handling around the state-sync fetch
catches in the state-sync flow to call fallBackToLegacyListAfterFetchFailure
only when !Task.isCancelled. Continue logging the error as appropriate, but
ensure cancelled or superseded requests do not disable stateSyncActive or
trigger the legacy reload; preserve the existing method_not_found handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37ed0b5d-9eed-44ee-b3e2-7c51117d56d3
📒 Files selected for processing (7)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileStateSyncStore.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileStateSyncStoreTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellStateSyncTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectAttemptDeadlineTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift:
- Around line 908-916: Update the timeout coordination around operationTask and
once so the timeout Task is captured and cancelled when operationTask completes
successfully. Replace try? around Task.sleep with do-catch, and ensure the
timeout path calls once.finish(nil) only when the sleep reaches its deadline,
not when cancellation throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 539d2939-5242-4325-82f1-03a15d9f370b
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectAttemptDeadlineTests.swift
Comment on lines
+908
to
+916
| Task { | ||
| once.finish(await operationTask.value) | ||
| } | ||
| Task { | ||
| try? await Task.sleep(nanoseconds: nanoseconds) | ||
| operationTask.cancel() | ||
| once.finish(nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cancel the timeout task on success to avoid a 30-second resource leak.
When the operation completes before the deadline, the timeout Task is currently never cancelled. It continues sleeping in the background for the full deadline duration (up to 30 seconds). This keeps operationTask and anything captured by it retained in memory long after a successful reconnect.
You can prevent this by capturing the timeout task and cancelling it when the operation completes. Note that you must use do-catch instead of try? around the sleep; otherwise, the cancelled sleep would wake up, swallow the cancellation error, and incorrectly invoke once.finish(nil).
♻️ Proposed fix to cancel the timeout task
- Task {
- once.finish(await operationTask.value)
- }
- Task {
- try? await Task.sleep(nanoseconds: nanoseconds)
- operationTask.cancel()
- once.finish(nil)
- }
+ let timeoutTask = Task {
+ do {
+ try await Task.sleep(nanoseconds: nanoseconds)
+ operationTask.cancel()
+ once.finish(nil)
+ } catch {
+ // Cancelled because the operation finished first; do nothing.
+ }
+ }
+ Task {
+ let result = await operationTask.value
+ timeoutTask.cancel()
+ once.finish(result)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Task { | |
| once.finish(await operationTask.value) | |
| } | |
| Task { | |
| try? await Task.sleep(nanoseconds: nanoseconds) | |
| operationTask.cancel() | |
| once.finish(nil) | |
| } | |
| } | |
| let timeoutTask = Task { | |
| do { | |
| try await Task.sleep(nanoseconds: nanoseconds) | |
| operationTask.cancel() | |
| once.finish(nil) | |
| } catch { | |
| // Cancelled because the operation finished first; do nothing. | |
| } | |
| } | |
| Task { | |
| let result = await operationTask.value | |
| timeoutTask.cancel() | |
| once.finish(result) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift
around lines 908 - 916, Update the timeout coordination around operationTask and
once so the timeout Task is captured and cancelled when operationTask completes
successfully. Replace try? around Task.sleep with do-catch, and ensure the
timeout path calls once.finish(nil) only when the sleep reaches its deadline,
not when cancellation throws.
…antics - v2 negotiation now starts from the mobile.events.subscribe ACKNOWLEDGEMENT instead of racing the handshake, so a fetch snapshot can never miss a change emitted before the Mac registered this connection. - The watchdog's lost-registration recovery repairs the v2 cursor (missed events include missed deltas) instead of no-opping under v2. - The pull-to-refresh/Computers refresh path awaits the v2 cursor fetch and returns its outcome, so the spinner ends with authoritative state and a failed fetch is not reported as success. - Fetch failure handling is gated by owning generation + cancellation, so a cancelled predecessor surfacing as a timeout can no longer disable v2 underneath its successful replacement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w superseded fetches - The abandoned-dial handle is registered immediately after the race returns, before cancellation/supersession guards can drop it, so wedged dials from replaced attempts stay inside the accounting ceiling. - performStateSyncFetch follows cancel-and-replace supersessions (bounded) so a user refresh reports the authoritative replacement's outcome instead of a superseded cancel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…reset, no legacy list under v2 - A fetch response missing a requested collection can no longer activate v2 or project an empty mirror over a valid list; it is treated as a fetch failure. - signOut tears down state sync: mirror wiped (previous account's titles, directories, previews), v2 deactivated, in-flight fetch invalidated by generation so late completions cannot write into the next session. - While v2 is active the legacy full-list request is never built or sent; the cursor fetch is both the liveness probe (caller timeout honored) and the authoritative refresh. Also widens a load-flaky poll window in the hung-redial test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Ordinary workspace.updated events are suppressed under v2 (each pairs with a delta); only the watchdog's lost-registration branch repairs, through the dedicated repairMissedEventWindow (cursor fetch under v2, full refetch under legacy). Removes the per-event fetch RPC and the cancel-storm that could starve a genuine gap repair. - Abandoned-dial janitors no longer record transient backoff on resolution: that write could land mid-manual-retry and re-block the dial the user just requested. They now kick the coalesced recovery entry directly, only when no attempt or scheduled retry is active. (Found via deterministic full-suite reproduction of the hung-redial test.) - The hung-redial test releases parked dials when lifting the hang (eternal hangs were a test artifact racing auto-retry state) and uses starvation-proof poll windows. Full shell suite green 3x consecutively; the suite's residual flakiness on loaded machines reproduces on origin/main (6 issues/run) and predates this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…client Replaces the bare stateSyncActive flag with stateSyncAuthorityClientID: v2 is active only while the client that earned authority (successful mobile.sync.fetch) IS the current remoteClient. This fixes the class the three findings shared: client promotion/replacement implicitly demotes to legacy (no suppressed-invalidation window on the new Mac), deltas are ignored outside the authoritative window (no legacy/v2 concurrent writers after a fallback), and a superseded fetch waiter consults the last settled generation's outcome (a liveness caller can no longer read a replacement's success as failure and tear down a healthy session). Regression test: replacingTheForegroundClientDemotesStateSyncAuthority. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sweep Replaces the fetch slot's cancel-and-replace semantics with the codebase's proven single-flight + follow-up pattern (same shape as scheduleSecondaryRefresh): same-client demand coalesces onto the in-flight runner and requests one trailing sweep; only a different client's demand replaces the runner. This resolves the round-8 class at its root: - gap repairs can no longer be starved by sustained 80ms churn (deltas coalesce instead of cancelling the repair they need); - negotiation-window deltas request a trailing sweep instead of being dropped (a change postdating the fetch snapshot is swept, not lost); - no concurrent fetch generations exist, so a superseded task can neither misreport nor overwrite a replacement's success (the settled-outcome bookkeeping is deleted, not patched). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fetch deadlines - The per-attempt deadline and abandoned-dial accounting move inside reconnectActiveMacOutcome itself, so startup restore, team-scope restore, and the manual workspace-list fallback are bounded identically to the recovery owner (whose special-case deadline branch is deleted). The raw dial is reconnectActiveMacOutcomeUnbounded. - performStateSyncFetch bounds each WAITER by its own timeout when one is provided (a 3s liveness probe joining a slow fetch reports within its contract); the shared runner is never cancelled by a waiter's deadline and keeps converging in the background. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l forwarding The round-9 blanket wrapper detached every reconnect's synchronous prefix, breaking reconnect serialization semantics (registry-snapshot reuse and initial-connect tests). The per-attempt deadline returns to the recovery owner path (round-8 shape, proven 3x-green); bounding the remaining lifecycle callers (startup restore, team restore, manual fallback) is a consciously deferred follow-up needing per-call-site deadline policy. Kept from round 9: per-waiter timeout bounds on coalesced state-sync fetches, and explicit caller-cancellation forwarding in raceAgainstDeadline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ock deadline - Every public symbol in the state-sync package surface carries DocC. - MobileSyncFrameJSON (caseless namespace enum) becomes the injectable MobileSyncFrameCoder instance type; error renamed accordingly. - The deadline race's timer uses ContinuousClock (intentional bounded deadline, cancellation-wired). Remaining P2 policy findings are rejected with named boundaries recorded on the PR: the protocol files each own a closed set of tightly coupled wire DTOs; DeadlineRaceOutcome/RaceContinuationOnce are private helpers co-located with their sole consumer; the once-guard must be callable from the synchronous onCancel handler, which an actor cannot be; the static race helper lives on a heavily stateful owning type, not a namespace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent failure Renegotiation clears authority before the fetch, so the fallback's guard on current authority skipped exactly the recovery it exists for: a transient negotiation-fetch failure after a same-client resubscribe left events missed in the subscription gap unrecovered. The bounded legacy reload now runs on every transient fetch failure regardless of authority (currency-guarded per iteration). Adds trace lines on the fallback path and a regression test (transientNegotiationFailureStillRunsTheLegacyReload). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Negotiation can grant v2 while a legacy full-list request is in flight; applying the captured response then would overwrite newer mirror state. The legacy path now re-checks authority after the await and reports liveness success without applying when v2 took ownership. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 22, 2026
This was referenced Jul 29, 2026
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
1 participant
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.
Any workspace change today emits
workspace.updatedwith an empty payload (80ms throttle) and every subscribed phone re-fetches the wholemobile.workspace.list. Each row is ~0.5-1KB of JSON, so at 100+ workspaces one changed field costs ~100KB of main-actor serialization plus ~100KB on the wire, per phone, per change burst, and the event carries no ordering, so a missed edge is silent staleness.This adds mobile state sync v2 (design:
docs/mobile-state-sync-v2.md). The Mac keeps an epoch + per-collection revision store of typed workspace/group records (Sources/Mobile/MobileStateSync.swift, store inCMUXMobileCore). Each observer tick diffs typed rows into the store and emits onemobile.sync.deltaevent carrying only the rows that changed.mobile.sync.fetchanswers a cursor with the exact missing span, or a snapshot when the cursor is cold, from another epoch, or older than the retained tombstones. The phone mirrors records with a cursor and projects them through the existingapplyRemoteWorkspaceListpath, so everything downstream (per-Mac state, group collapse, selection) is shared. A gapped delta self-heals with a cursor fetch. While v2 is active,workspace.updatedno longer schedules refetches.Compatibility is the method itself: a legacy Mac answers
method_not_foundand the phone stays on the refetch loop; legacy phones never call it and the empty event still fires. No settings flag. Transport-agnostic: rides the existing framed RPC + event subscription on both the Tailscale TCP path and Iroh; no dependency on the Iroh work in flight.Verification:
swift test --package-path Packages/Shared/CMUXMobileCore— 238 tests passed (30 new: store diff/revs/tombstones, mirror apply/gap/idempotent overlap, wire coding).swift test --package-path Packages/iOS/CmuxMobileShell— 538 tests passed (3 new behavior tests through the scripted host: negotiation applies snapshot and suppresses legacy refetch, gapped delta repairs via cursor fetch, legacy Mac keeps the refetch loop).swift test --package-path Packages/iOS/CmuxMobileRPC— 84 tests passed.Localization audit: no user-facing strings added or changed; the payloads are wire data only.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Mobile state sync v2 replaces the invalidate-and-refetch loop with cursor-based per-record deltas for the workspace list. It scopes authority to the owning client, repairs gaps, bounds recovery redials with a per-attempt deadline, suppresses legacy reloads while v2 is active, stays transport-agnostic (proven on the independent Iroh server-events lane), adds DocC docs plus an injectable frame coder with ContinuousClock-based deadlines, and falls back to a bounded legacy reload on transient negotiation failure.
New Features
workspacesandgroupsinCMUXMobileCore.mobile.sync.fetch(snapshot or delta from a cursor) andmobile.sync.delta; iOS mirrors via a cursor, applies deltas or repairs via fetch, and projects throughapplyRemoteWorkspaceList.workspace.updatedstops full-list refetches while v2 is active; legacy Macs returnmethod_not_foundand phones fall back.Bug Fixes
mobile.sync.fetchrunner with a trailing sweep; recovery-owner reconnects run under a per-attempt deadline with abandoned-dial caps; manual reconnect/pull clears transient backoff; sign-out wipes the mirror and deactivates v2.Written for commit 11a7e9e. Summary will update on new commits.
Summary by CodeRabbit