iOS render-grid liveness watchdog: probe before teardown, consume during subscribe ack (fixes the 10.5s false-fire replay loop) - #5869
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughDecouples subscribe ack from event consumption, adds a bounded single‑flight liveness probe with configurable timeout, surfaces server-side already_subscribed in subscribe responses, and adds deterministic tests validating watchdog and recovery behaviors. ChangesRender-grid liveness watchdog refactoring and testing
Sequence DiagramsequenceDiagram
participant Client as MobileShellComposite
participant StartTask as Subscription Handshake
participant Consumer as Event Consumer
participant Host as MobileHostService
participant Watchdog as Liveness Watchdog
Client->>StartTask: beginTerminalEventSubscriptionStart (non-blocking)
Client->>Consumer: startTerminalRefreshPolling concurrently
StartTask->>Host: mobile.events.subscribe RPC
Consumer->>Client: recordTerminalEventStreamLiveness() on each consumed event
Host-->>StartTask: TerminalEventSubscriptionAck{subscribed(alreadySubscribed:Bool)|failed}
loop Liveness monitoring
Watchdog->>Watchdog: check silence threshold
alt Silence exceeded
Watchdog->>Host: probeEventSubscriptionLiveness (bounded timeout)
Host-->>Watchdog: ack with already_subscribed flag
alt already_subscribed=false
Watchdog->>Client: replay mounted surfaces + refresh workspace
else already_subscribed=true
Watchdog->>Watchdog: healthy, reset liveness
else probe timeout/failed
Watchdog->>Client: teardown and re-subscribe
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c73fbb335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3021
to
+3025
| guard ack.isSubscribed else { | ||
| MobileDebugLog.anchormux("sync.subscribe_failed reason=start") | ||
| self.diagnosticLog?.record(DiagnosticEvent(.error)) | ||
| self.stopTerminalRefreshPolling() | ||
| self.markMacConnectionUnavailable() |
There was a problem hiding this comment.
When the initial mobile.events.subscribe ack is delayed or dropped while a prior server subscription is still delivering events, the new consumer loop can now keep processing those events and liveness probes can succeed, but this start task still treats the later timeout as authoritative and stops the current listener. In that scenario the stream has already proven healthy via consumed envelopes/probe success, yet the delayed start failure tears it down and marks the Mac unavailable; recheck recent liveness/current probe success (or cancel/supersede the start ack) before stopping polling here.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR fixes the iOS render-grid liveness watchdog false-fire loop by replacing the "silence = death" heuristic with a probe-before-teardown strategy, and by decoupling the start-subscribe ack from event consumption so the liveness clock stays coupled to actual delivery.
Confidence Score: 5/5Safe to merge. The probe-before-teardown redesign is well-guarded with generation tokens on every state transition, backward-compatible with older Mac hosts, and backed by four targeted regression tests that reproduce the exact false-fire conditions found during bisect. The generation-guard pattern (listenerID, probeID) is applied consistently across the ack task, the probe task, and the watchdog tick, preventing any superseded task from touching live state. The concurrent start-ack design is structurally sound: the consumer loop starts immediately, the ack acts only while the generation is current, and the stream-ended-before-ack path converges cleanly to unavailable. The already_subscribed field is nullable and the nil-as-active fallback is correct for older hosts. The DispatchSourceTimer deadline replaces the previously flagged Task.sleep and is the approved primitive. The one observation (inner probe task outliving outer-task cancellation by up to the probe deadline) is bounded, result-discarding, and well below the level of a blocking defect. No files require special attention. The primary change in MobileShellComposite.swift is the most complex, but the generation guards and the new streamEndingBeforeStartAckMarksUnavailable test cover the main edge cases. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as WatchdogTimer
participant C as checkRenderGridLiveness
participant P as probeEventSubscriptionLiveness
participant D as DeadlineTimer
participant M as Mac (MobileHostConnection)
participant R as resyncTerminalOutput
W->>C: tick (listenerID matches)
C->>C: "silent >= 9s threshold?"
alt probe slot empty
C->>P: spawn renderGridLivenessProbeTask
P->>D: arm one-shot timer (livenessProbeTimeoutNanoseconds)
P->>M: mobile.events.subscribe (liveness_probe)
alt host answers in time
M-->>P: "{stream_id, already_subscribed}"
P->>D: deadline.cancel()
alt "already_subscribed == true (or nil)"
P->>C: ".subscribed => recordLiveness + markHealthy"
else "already_subscribed == false (lost registration)"
P->>C: ".subscribed => recordLiveness + replay surfaces + workspace refresh"
end
else timeout fires first
D->>P: probe.cancel()
P-->>C: .failed
C->>C: "recheck silence still >= threshold?"
C->>R: resyncTerminalOutput(reason: liveness)
end
else probe already in-flight
C->>C: no-op (single-flight guard)
end
Reviews (5): Last reviewed commit: "Treat a stream that ends before its subs..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3266
to
+3268
| let deadline = Task { | ||
| try? await Task.sleep(nanoseconds: timeoutNanoseconds) | ||
| probe.cancel() |
Contributor
There was a problem hiding this comment.
Task.sleep as a deadline in production code
cmux-swift-blocking-runtime explicitly lists Task.sleep as a flagged primitive in production Swift. The inline comment calls this "allowed: cancellation-wired timeout, not a polling sleep," but the pass list in the rule is limited to test-only scaffolding, CI YAML, and pure animation timing — a probe deadline doesn't fit any of those.
The file already uses DispatchSourceTimer for the watchdog tick; a one-shot DispatchSourceTimer (or a withThrowingTaskGroup-based structured-concurrency race, which is the idiomatic pattern in Swift 5.9+) would satisfy the rule without changing behaviour. If the team consciously accepts Task.sleep for bounded timeouts as a codebase idiom, that carve-out should be documented in the relevant rule file so future reviewers know the intent.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Comment on lines
+3184
to
+3185
| self.renderGridLivenessProbeTask = nil | ||
| self.renderGridLivenessProbeID = nil |
Contributor
There was a problem hiding this comment.
renderGridLivenessProbeTask and renderGridLivenessProbeID are set to nil (lines 3184-3185) before the generation-validity guards on lines 3186-3190. If stopRenderGridLivenessWatchdog fires on the MainActor between those two points (not possible with cooperative scheduling, but worth noting for auditability), a new probe could start before the old one's post-slot-clear logic completes. More concretely: if the probeID ownership guard (line 3183) passes but then Task.isCancelled fires (line 3186), the slot is already cleared — a subsequent watchdog tick's guard renderGridLivenessProbeTask == nil will pass and re-arm a probe with stale preconditions. Clearing the slot only after all guards pass would be safer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2814-2829: The MobileShellComposite class contains the liveness
subscription/watchdog state machine (including the TerminalEventSubscriptionAck
enum and related orchestration spanning the MobileShellComposite methods around
the shown ranges) which should be extracted into a dedicated component; create a
new LivenessSupervisor (or LivenessSubscriptionManager) type and move the
TerminalEventSubscriptionAck enum, all state, timers/watchdog logic,
subscribe/ack handling, and probe/start-ack orchestration into it, expose a
concise interface (start/stop/handleIncomingAck/subscribeRequest) and replace in
MobileShellComposite any direct state accesses with calls to that interface,
update unit tests to target the new component, and ensure any dependencies
(loggers, dispatch queues, event emitters) are injected into the new type rather
than referenced from MobileShellComposite.
🪄 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: b0f29381-185f-4c3c-a447-7785a28fcb3f
📒 Files selected for processing (5)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftSources/Mobile/MobileHostService.swift
Comment on lines
+2814
to
+2829
| /// Outcome of a `mobile.events.subscribe` round-trip. | ||
| private enum TerminalEventSubscriptionAck { | ||
| case failed | ||
| /// The host registered (or re-asserted) the subscription. | ||
| /// `alreadySubscribed == false` means this acknowledgement INSTALLED | ||
| /// the registration, so events emitted while it was absent were never | ||
| /// delivered; `nil` means the host predates the field (treat as | ||
| /// already active). | ||
| case subscribed(alreadySubscribed: Bool?) | ||
|
|
||
| var isSubscribed: Bool { | ||
| if case .subscribed = self { return true } | ||
| return false | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract the liveness subscription/watchdog state machine from MobileShellComposite.
This PR adds a sizable new coordination subsystem into an already very large, multi-domain class. Please move this liveness/probe/start-ack orchestration into a dedicated component to keep responsibilities bounded and independently testable.
As per coding guidelines, production Swift files in {Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift should be flagged when they are oversized and mixing responsibilities.
Also applies to: 2956-3032, 3138-3273
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 2814 - 2829, The MobileShellComposite class contains the liveness
subscription/watchdog state machine (including the TerminalEventSubscriptionAck
enum and related orchestration spanning the MobileShellComposite methods around
the shown ranges) which should be extracted into a dedicated component; create a
new LivenessSupervisor (or LivenessSubscriptionManager) type and move the
TerminalEventSubscriptionAck enum, all state, timers/watchdog logic,
subscribe/ack handling, and probe/start-ack orchestration into it, expose a
concise interface (start/stop/handleIncomingAck/subscribeRequest) and replace in
MobileShellComposite any direct state accesses with calls to that interface,
update unit tests to target the new component, and ensure any dependencies
(loggers, dispatch queues, event emitters) are injected into the new type rather
than referenced from MobileShellComposite.
Source: Coding guidelines
Contributor
Author
|
Addressed the Greptile P2: the probe deadline now uses a one-shot DispatchSourceTimer (the same sanctioned primitive as the watchdog tick) instead of Task.sleep, cancellation wired both directions. 59 package tests green. |
lawrencecchen
added a commit
that referenced
this pull request
Jun 11, 2026
… watchdog fix, #5869) Conflict resolution: keep both the DEV dogfood checklist listener fields and the watchdog's terminalSubscriptionStartTask; subscribe path takes the watchdog's concurrent-ack helper, with the notifications cold-attach fetch moved into its ack-success branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-watchdog-falsefire
branch
from
June 11, 2026 21:09
fd37830 to
40e6c53
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 3417-3425: The start-ack path currently collapses
TerminalEventSubscriptionAck to ack.isSubscribed and misses the
.subscribed(alreadySubscribed: false) repair case; change the start-ack handler
to switch on TerminalEventSubscriptionAck and treat
.subscribed(alreadySubscribed: false) the same as the watchdog probe repair path
(invoke the shared re-subscribe/repair routine that refreshes mounted surfaces
and the workspace list), reusing the same method used by the watchdog probe to
avoid duplicated logic; also update the other subscribe-ack handling site
referenced around the 3624–3632 change to call that same shared repair routine
so all entrypoints (re-subscribe, watchdog, UI actions) converge on one code
path.
- Around line 237-240: isComposerPresented and the composer-presenting setter
currently read/write composerDismissedTerminalIDs via selectedTerminalID,
causing operations like toggleComposer(forTerminalID:) and
presentAndFocusComposer(forTerminalID:) to act on the wrong surface when
rendered != selected. Fix by making the composer-present state APIs use the
explicit terminal ID: change isComposerPresented to take a TerminalID (or add
isComposerPresented(forTerminalID:)) and change the setter to
setComposerPresented(forTerminalID:isPresented:) (or an overloaded version) so
they read/write composerDismissedTerminalIDs using terminalID.rawValue; update
toggleComposer(forTerminalID:) and presentAndFocusComposer(forTerminalID:) to
call these new/updated helpers instead of relying on selectedTerminalID, and
adjust any other callers (mentioned ranges 2209-2215, 2231-2234, 2292-2299)
accordingly.
🪄 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: 24f9b86d-ecd1-4b22-8659-6e6a469a51a6
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftSources/Mobile/MobileHostService.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 3417-3425: The start-ack path currently collapses
TerminalEventSubscriptionAck to ack.isSubscribed and misses the
.subscribed(alreadySubscribed: false) repair case; change the start-ack handler
to switch on TerminalEventSubscriptionAck and treat
.subscribed(alreadySubscribed: false) the same as the watchdog probe repair path
(invoke the shared re-subscribe/repair routine that refreshes mounted surfaces
and the workspace list), reusing the same method used by the watchdog probe to
avoid duplicated logic; also update the other subscribe-ack handling site
referenced around the 3624–3632 change to call that same shared repair routine
so all entrypoints (re-subscribe, watchdog, UI actions) converge on one code
path.
- Around line 237-240: isComposerPresented and the composer-presenting setter
currently read/write composerDismissedTerminalIDs via selectedTerminalID,
causing operations like toggleComposer(forTerminalID:) and
presentAndFocusComposer(forTerminalID:) to act on the wrong surface when
rendered != selected. Fix by making the composer-present state APIs use the
explicit terminal ID: change isComposerPresented to take a TerminalID (or add
isComposerPresented(forTerminalID:)) and change the setter to
setComposerPresented(forTerminalID:isPresented:) (or an overloaded version) so
they read/write composerDismissedTerminalIDs using terminalID.rawValue; update
toggleComposer(forTerminalID:) and presentAndFocusComposer(forTerminalID:) to
call these new/updated helpers instead of relying on selectedTerminalID, and
adjust any other callers (mentioned ranges 2209-2215, 2231-2234, 2292-2299)
accordingly.
🪄 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: 24f9b86d-ecd1-4b22-8659-6e6a469a51a6
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftSources/Mobile/MobileHostService.swift
🛑 Comments failed to post (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
237-240:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the explicit terminal id for composer presentation state.
toggleComposer(forTerminalID:)/presentAndFocusComposer(forTerminalID:)claim to act on a specific surface, butisComposerPresentedandsetComposerPresentedstill read/write viaselectedTerminalID. When the rendered terminal diverges from the selection, the compose button flips the wrong terminal's dismissed bit and can dismiss instead of focusing the visible composer.Suggested direction
-public var isComposerPresented: Bool { - guard let terminalID = selectedTerminalID?.rawValue else { return false } +public func isComposerPresented(forTerminalID terminalID: String? = nil) -> Bool { + guard let terminalID = terminalID ?? selectedTerminalID?.rawValue else { return false } return !composerDismissedTerminalIDs.contains(terminalID) } -public func toggleComposer(forTerminalID terminalID: String? = nil) { - if isComposerPresented { - setComposerPresented(false) +public func toggleComposer(forTerminalID terminalID: String? = nil) { + if isComposerPresented(forTerminalID: terminalID) { + setComposerPresented(false, forTerminalID: terminalID) } else { - setComposerPresented(true) + setComposerPresented(true, forTerminalID: terminalID) requestComposerFieldFocus(forTerminalID: terminalID) } } -private func setComposerPresented(_ presented: Bool) { - guard let terminalID = selectedTerminalID?.rawValue, - presented != isComposerPresented else { return } +private func setComposerPresented(_ presented: Bool, forTerminalID terminalID: String? = nil) { + guard let terminalID = terminalID ?? selectedTerminalID?.rawValue, + presented != isComposerPresented(forTerminalID: terminalID) else { return } ... }Also applies to: 2209-2215, 2231-2234, 2292-2299
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 237 - 240, isComposerPresented and the composer-presenting setter currently read/write composerDismissedTerminalIDs via selectedTerminalID, causing operations like toggleComposer(forTerminalID:) and presentAndFocusComposer(forTerminalID:) to act on the wrong surface when rendered != selected. Fix by making the composer-present state APIs use the explicit terminal ID: change isComposerPresented to take a TerminalID (or add isComposerPresented(forTerminalID:)) and change the setter to setComposerPresented(forTerminalID:isPresented:) (or an overloaded version) so they read/write composerDismissedTerminalIDs using terminalID.rawValue; update toggleComposer(forTerminalID:) and presentAndFocusComposer(forTerminalID:) to call these new/updated helpers instead of relying on selectedTerminalID, and adjust any other callers (mentioned ranges 2209-2215, 2231-2234, 2292-2299) accordingly.
3417-3425:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
alreadySubscribed == falsethrough one shared subscribe-ack path.
TerminalEventSubscriptionAcknow carries the “registration was missing and got reinstalled” case, but the start-ack handler still collapses everything toack.isSubscribed. Right now only the watchdog probe path repairsalreadySubscribed == false; a plain re-subscribe that repairs a lost registration can still leave mounted surfaces and the workspace list stale until a later watchdog cycle.As per coding guidelines, "When a behavior is exposed through multiple entrypoints (keyboard shortcut, command palette, context menu, CLI, settings, debug menu), implement one shared action/model path and verify every entrypoint that should invoke it. Do not patch one surface while leaving the others with duplicated logic."
Also applies to: 3624-3632
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 3417 - 3425, The start-ack path currently collapses TerminalEventSubscriptionAck to ack.isSubscribed and misses the .subscribed(alreadySubscribed: false) repair case; change the start-ack handler to switch on TerminalEventSubscriptionAck and treat .subscribed(alreadySubscribed: false) the same as the watchdog probe repair path (invoke the shared re-subscribe/repair routine that refreshes mounted surfaces and the workspace list), reusing the same method used by the watchdog probe to avoid duplicated logic; also update the other subscribe-ack handling site referenced around the 3624–3632 change to call that same shared repair routine so all entrypoints (re-subscribe, watchdog, UI actions) converge on one code path.Source: Coding guidelines
The Release-sim bisect (2026-06-10) caught the liveness watchdog firing "render-grid stream silent for 10499ms, re-subscribing" every ~10.5s forever, plus "subscribe failed reason=start: requestTimedOut", while the Mac kept delivering events. Each false fire makes the Mac re-replay the full grid (constant repaint, battery and bandwidth waste). Two reproduced defects, expressed through the real RPC transport and the real consumer path against a scripted host: 1. renderGridEventsArrivingDuringStartSubscribeAreConsumed: events the transport delivers while the mobile.events.subscribe ack is still in flight pile up unconsumed in the subscription stream buffer, because the listener task awaits the ack before starting the for-await consumer loop that stamps the liveness clock. A healthy establishing stream therefore looks silent to the watchdog, and the watchdog's resync then cancels its own in-flight subscribe, which surfaces as requestTimedOut. 2. watchdogDoesNotResubscribeHealthyIdleStream: a healthy idle terminal legitimately emits zero events (the Mac dedupes render-grid frames by row signature and stateSeq), so wall-clock silence alone cannot distinguish idle from dead. The watchdog tears down and full-replays a perfectly healthy subscription every threshold window. watchdogStillResubscribesGenuinelyDeadStream pins the watchdog's original purpose (the ~85s silent-death hang) so the fix cannot regress it. Includes a DEBUG-only seam to run one watchdog evaluation deterministically with the injected clock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ribe ack Fixes the watchdog false-fire loop the 2026-06-10 Release-sim bisect caught: "render-grid stream silent for 10499ms, re-subscribing" every ~10.5s forever on a healthy connection, plus "subscribe failed reason=start: requestTimedOut", with the Mac re-replaying the full grid each cycle (constant repaint, battery and bandwidth waste). Root cause, two halves: 1. Silence is not death. A healthy idle terminal pushes zero events (MobileTerminalRenderObserver dedupes unchanged frames by row signature and stateSeq, and cursor blink changes neither), so the watchdog's wall-clock silence check fired on every idle stream once per threshold window, forever. The watchdog had no positive liveness signal at all. 2. The liveness clock was stamped only inside the listener's for-await loop, which did not start until resolveTerminalOutputTransport and the mobile.events.subscribe ack both completed. Events delivered during that establishment window were buffered unconsumed in the subscription stream, invisible to the clock; each watchdog fire then cancelled its own in-flight start subscribe, which MobileCoreRPCSession.cancelPendingRequest surfaces as requestTimedOut (the real wire timeout is 30s, so the 10.5s cadence of that log was the watchdog, not the network), and the dying generation marked the connection unavailable underneath the fresh one. Fix, structurally: - One liveness ownership point: recordTerminalEventStreamLiveness() is stamped by every consumed envelope, by a successful host probe, and (as the generation reset) when a watchdog generation is armed; the watchdog reads the same record, and generation identity (listenerID) guards every transition. - The silence threshold is now a suspicion, not a verdict: a crossing runs a bounded auth-exempt mobile.host.status probe over the same transport the events ride on. Probe answered = stream healthy but quiet, stamp and stay quiet. Probe failed = re-check silence (events may have resumed mid-probe), then run the existing teardown + re-subscribe + replay recovery. The ~85s silent-death case still heals: dead transport fails the probe at livenessProbeTimeoutNanoseconds (default 3s, runtime-injectable). - The start subscribe ack now runs concurrently with consumption (beginTerminalEventSubscriptionStart): the ack is a server-side enable handshake, not a delivery precondition, so the consumer loop starts immediately and the clock stays coupled to actual event arrival. Success/failure is acted on only while the generation is current, so a superseded ack can no longer mark the connection unavailable, and a cancelled ack logs as cancelled instead of a fake wire timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…status Autoreview P1 on the previous probe design: a mobile.host.status answer proves the RPC channel but not the event subscription, so a dropped or wedged server-side registration behind a live channel would reset the liveness clock forever and mask the staleness. The probe is now an idempotent mobile.events.subscribe for the SAME stream id and current topics. A completed round-trip proves the transport the events ride on is alive AND (re)installs the registration, and the host's subscription tracker re-evaluates producer demand on every replace, so the failure mode the finding describes self-heals instead of being masked. The probe still restarts nothing: no listener teardown, no replay, no stream interruption. The deadline bounds the whole attempt including pre-wire token work, so a wedged token provider cannot pin the single-flight probe slot. Tests updated to the final semantics: the teardown signals on a healthy idle stream are now listener restarts (a second mobile.host.status capability resolve) and replay traffic, since the probe itself legitimately re-sends mobile.events.subscribe. Both liveness tests remain red against the pre-fix sources (verified by checking out the commit-1 code under the amended tests: idle stream restarts plus replayCount 2, and the establishment-window event never delivered). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…irs a lost registration Autoreview P1 round 2: a successful probe could have just REINSTALLED a registration the host had lost, and render-grid deltas emitted during the gap were never delivered, so stamping liveness and returning would leave the phone stale until unrelated activity happened to repaint the affected rows. The Mac's mobile.events.subscribe acknowledgement now reports already_subscribed (whether the stream id was registered on the connection before the idempotent replace). The phone's probe distinguishes the outcomes: already_subscribed=true (or absent, for older Macs whose registrations cannot outlive the connection anyway) is the healthy-idle case, stamp and stay quiet; already_subscribed=false means the probe repaired a lost registration, so it stamps AND requests a catch-up replay for every mounted surface, without restarting the listener (the phone-side stream is intact; only the host-side registration was missing). New regression test probeRepairingLostSubscriptionReplaysMountedSurfaces drives the lost-registration case end to end through the scripted host: replay re-requested, no listener restart, original stream still consuming afterwards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sh on repair Two autoreview P2 fixes: 1. A cancelled probe from a previous watchdog generation could complete late and nil out a newer generation's in-flight probe handle, breaking the single-flight guard and orphaning the newer probe from lifecycle cancellation. The slot now carries an ownership token (renderGridLivenessProbeID): only the probe holding it may clear the slot; superseded probes return without touching it. 2. The repaired-registration path only replayed terminal surfaces, but the same registration carries workspace.updated, so workspace create/rename/delete events emitted during the gap were missed too. The repair now also re-fetches the authoritative workspace list via the existing scheduleWorkspaceListRefreshFromEvent path, asserted in the repair regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ubscribe RPCs are non-interactive host activity Review fixes: a successful liveness probe now calls markMacConnectionHealthy() so a transient RPC failure can't leave the status UI stuck unavailable on an idle terminal, and mobile.events.subscribe/unsubscribe no longer count as interactive mobile activity so the ~9s idle probe can't starve host work gated on mobile quiet (TabManager background git/PR refresh). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile P2: Task.sleep is a flagged primitive in production Swift; the file's sanctioned timing primitive is DispatchSourceTimer (the watchdog tick already uses it). Same behavior: deadline cancels the in-flight probe, probe completion cancels the deadline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…refresh budget Split MobileShellRenderGridLivenessTests.swift (583 > 500 new-file threshold) into the 4 regression tests (212 lines) plus MobileShellRenderGridLivenessTestSupport.swift (381 lines: injected clock, scripted host router, transport mocks, connected-store builder). Budget refresh accepts the remaining growth as known debt: MobileShellComposite.swift +238 (the watchdog probe machinery is single-flight state coupled to the composite's private connection state; extracting it would force ~15 members internal — precedent: the composer merge refreshed this same file's budget) and MobileHostService.swift +14 (alreadySubscribed ack field). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-watchdog-falsefire
branch
from
June 12, 2026 00:31
40e6c53 to
bb1e595
Compare
Contributor
Author
|
Merge-train adjudication of the two open review findings, deferred as follow-ups rather than blockers:
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2794-2799:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate the connection after awaiting paired-Mac persistence.
connect(...)is generation-guarded before this await, but not after it. IfsignOut()or another teardown runs whilepersistPairedMacFromTicket(ticket)is in flight, this stale continuation still reapplies the old workspace list and flipsconnectionStateback to.connectedeven thoughremoteClientwas already cleared.Suggested fix
clearPairingError() await persistPairedMacFromTicket(ticket) + guard generation == connectionGeneration, + isSignedIn, + remoteClient === client else { return nil } applyRemoteWorkspaceList(response, preferActiveTicketTarget: workspaceListRequest.preferActiveTicketTarget) syncSelectedTerminalForWorkspace() connectionState = .connected🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2794 - 2799, The continuation after await persistPairedMacFromTicket(ticket) can run against a torn-down client; capture and revalidate the connection generation (or check remoteClient) before applying state. In MobileShellComposite's connect(...) flow, save a local snapshot of the current generation/token or the presence of remoteClient before the await, and after await compare it to the current generation (and ensure remoteClient is still non-nil); only then call applyRemoteWorkspaceList(...), syncSelectedTerminalForWorkspace(), set connectionState = .connected and markMacConnectionHealthy(); otherwise abort the continuation to avoid reapplying stale state.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1847-1868: The task may resume after awaiting
pairedMacStore.loadAll and then perform an upsert despite the serialized
"currentness" check; re-check the ifStillCurrent condition after the await and
before calling pairedMacStore.upsert so the task aborts if it is no longer the
current writer. In practice, inside the closure passed to
performSerializedPairedMacWrite (the block that uses pairedMacStore.loadAll and
then upsert), call the same ifStillCurrent check again (awaiting it if it's
async) after computing displayName and before executing
pairedMacStore.upsert(macDeviceID:..., markActive: true, ...), and return early
if the check indicates this operation is stale.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`:
- Around line 46-67: The OutputCollector mounted with collector.mount(store:
store, surfaceID: "live-terminal") isn't guaranteed to be unmounted on failures;
add a defer { collector.unmount() } immediately after the mount call so
unmount() always runs even if subsequent helpers like `#require`(box.get()) or
renderGridEventFrame throw, ensuring the store and terminalOutputStream task are
released on error paths.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 159-160: The unsubscribe branch for "mobile.events.unsubscribe"
should clear the scripted subscription state so the router's
hasActiveSubscription is set to false (or call the existing
clearScriptedSubscription/clearSubscription helper) before returning the result;
modify the case that currently returns try? Self.resultFrame(id: id, result:
[:]) to first clear the subscription state (referencing hasActiveSubscription or
clearScriptedSubscription) and then return Self.resultFrame(id: id, result:
[:]).
- Around line 229-279: The transport currently allows late frames to be consumed
after close because receive() drains pendingFrames before checking isClosed and
deliver(_:) still appends frames when closed; update receive() to check isClosed
first and return nil immediately if closed (rather than draining pendingFrames),
change deliver(_:) to drop frames when isClosed is true instead of appending to
pendingFrames or resuming waiters, and have close() clear pendingFrames as well
as resume and clear receiveWaiters; reference the functions receive(),
deliver(_:), close() and the properties pendingFrames, receiveWaiters, isClosed
when making these changes.
- Around line 378-380: Replace the non-fatal test check so the helper aborts
immediately on a failed scripted connect: where the code currently does let
connected = await store.connectPairingURL(try attachURL(for: ticket)) followed
by `#expect`(connected, "scripted connect must succeed"), change that `#expect` to a
hard failure (e.g. precondition(connected, "scripted connect must succeed") or
fatalError with the same message) so connectPairingURL/attachURL failures stop
the test run instead of returning a possibly disconnected store.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 320-387: This file mixes new status/auth logic with an oversized
MobileHostService; extract the status shaping and verification policy into one
or more sibling types under Sources/Mobile (e.g., a MobileHostStatusManager or
MobileHostStatusPayload and MobileHostStatusVerifier types) and move
implementations of publicStatusPayload(routesPayload:),
identityStatusPayload(routesPayload:), networkStatusResult(for:), and any
verifier-cache/limiter logic (MobileHostStatusVerificationLimiter,
MobileHostPublicStatusCache, verifiedStackCaller(for:)) into those new files;
then have MobileHostService call the new type(s) for payload creation and
verification so the service file keeps only lifecycle/transport/RPC dispatch
concerns and the status/auth logic becomes unit-testable and localized.
- Around line 2180-2184: The response payload currently returns "stream_id" in
MobileHostService.handleSubscriptionRPC but the absence of "stream_id" observed
in daemon/remote/cmd/cmuxd-remote/main.go refers to proxy.stream.subscribe (not
mobile.events.subscribe), so update the handling to: make
MobileHostService.handleSubscriptionRPC explicitly produce the
MobileEventSubscribeResponse for mobile.events.subscribe (always include
stream_id when applicable) and ensure proxy.stream.subscribe responses omit
stream_id; refactor MobileHostService by extracting RPC subscription logic into
smaller methods (e.g., separate handleSubscriptionRPC,
buildMobileEventSubscribeResponse, buildProxyStreamSubscribeResponse) to
separate concerns and avoid mixing protocol paths, and update the response
construction to clearly document/emit the correct keys per RPC type.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2794-2799: The continuation after await
persistPairedMacFromTicket(ticket) can run against a torn-down client; capture
and revalidate the connection generation (or check remoteClient) before applying
state. In MobileShellComposite's connect(...) flow, save a local snapshot of the
current generation/token or the presence of remoteClient before the await, and
after await compare it to the current generation (and ensure remoteClient is
still non-nil); only then call applyRemoteWorkspaceList(...),
syncSelectedTerminalForWorkspace(), set connectionState = .connected and
markMacConnectionHealthy(); otherwise abort the continuation to avoid reapplying
stale state.
🪄 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: 588b5735-55ba-4786-9cae-eda2468b2263
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftSources/Mobile/MobileHostService.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2794-2799:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate the connection after awaiting paired-Mac persistence.
connect(...)is generation-guarded before this await, but not after it. IfsignOut()or another teardown runs whilepersistPairedMacFromTicket(ticket)is in flight, this stale continuation still reapplies the old workspace list and flipsconnectionStateback to.connectedeven thoughremoteClientwas already cleared.Suggested fix
clearPairingError() await persistPairedMacFromTicket(ticket) + guard generation == connectionGeneration, + isSignedIn, + remoteClient === client else { return nil } applyRemoteWorkspaceList(response, preferActiveTicketTarget: workspaceListRequest.preferActiveTicketTarget) syncSelectedTerminalForWorkspace() connectionState = .connected🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2794 - 2799, The continuation after await persistPairedMacFromTicket(ticket) can run against a torn-down client; capture and revalidate the connection generation (or check remoteClient) before applying state. In MobileShellComposite's connect(...) flow, save a local snapshot of the current generation/token or the presence of remoteClient before the await, and after await compare it to the current generation (and ensure remoteClient is still non-nil); only then call applyRemoteWorkspaceList(...), syncSelectedTerminalForWorkspace(), set connectionState = .connected and markMacConnectionHealthy(); otherwise abort the continuation to avoid reapplying stale state.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1847-1868: The task may resume after awaiting
pairedMacStore.loadAll and then perform an upsert despite the serialized
"currentness" check; re-check the ifStillCurrent condition after the await and
before calling pairedMacStore.upsert so the task aborts if it is no longer the
current writer. In practice, inside the closure passed to
performSerializedPairedMacWrite (the block that uses pairedMacStore.loadAll and
then upsert), call the same ifStillCurrent check again (awaiting it if it's
async) after computing displayName and before executing
pairedMacStore.upsert(macDeviceID:..., markActive: true, ...), and return early
if the check indicates this operation is stale.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift`:
- Around line 46-67: The OutputCollector mounted with collector.mount(store:
store, surfaceID: "live-terminal") isn't guaranteed to be unmounted on failures;
add a defer { collector.unmount() } immediately after the mount call so
unmount() always runs even if subsequent helpers like `#require`(box.get()) or
renderGridEventFrame throw, ensuring the store and terminalOutputStream task are
released on error paths.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 159-160: The unsubscribe branch for "mobile.events.unsubscribe"
should clear the scripted subscription state so the router's
hasActiveSubscription is set to false (or call the existing
clearScriptedSubscription/clearSubscription helper) before returning the result;
modify the case that currently returns try? Self.resultFrame(id: id, result:
[:]) to first clear the subscription state (referencing hasActiveSubscription or
clearScriptedSubscription) and then return Self.resultFrame(id: id, result:
[:]).
- Around line 229-279: The transport currently allows late frames to be consumed
after close because receive() drains pendingFrames before checking isClosed and
deliver(_:) still appends frames when closed; update receive() to check isClosed
first and return nil immediately if closed (rather than draining pendingFrames),
change deliver(_:) to drop frames when isClosed is true instead of appending to
pendingFrames or resuming waiters, and have close() clear pendingFrames as well
as resume and clear receiveWaiters; reference the functions receive(),
deliver(_:), close() and the properties pendingFrames, receiveWaiters, isClosed
when making these changes.
- Around line 378-380: Replace the non-fatal test check so the helper aborts
immediately on a failed scripted connect: where the code currently does let
connected = await store.connectPairingURL(try attachURL(for: ticket)) followed
by `#expect`(connected, "scripted connect must succeed"), change that `#expect` to a
hard failure (e.g. precondition(connected, "scripted connect must succeed") or
fatalError with the same message) so connectPairingURL/attachURL failures stop
the test run instead of returning a possibly disconnected store.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 320-387: This file mixes new status/auth logic with an oversized
MobileHostService; extract the status shaping and verification policy into one
or more sibling types under Sources/Mobile (e.g., a MobileHostStatusManager or
MobileHostStatusPayload and MobileHostStatusVerifier types) and move
implementations of publicStatusPayload(routesPayload:),
identityStatusPayload(routesPayload:), networkStatusResult(for:), and any
verifier-cache/limiter logic (MobileHostStatusVerificationLimiter,
MobileHostPublicStatusCache, verifiedStackCaller(for:)) into those new files;
then have MobileHostService call the new type(s) for payload creation and
verification so the service file keeps only lifecycle/transport/RPC dispatch
concerns and the status/auth logic becomes unit-testable and localized.
- Around line 2180-2184: The response payload currently returns "stream_id" in
MobileHostService.handleSubscriptionRPC but the absence of "stream_id" observed
in daemon/remote/cmd/cmuxd-remote/main.go refers to proxy.stream.subscribe (not
mobile.events.subscribe), so update the handling to: make
MobileHostService.handleSubscriptionRPC explicitly produce the
MobileEventSubscribeResponse for mobile.events.subscribe (always include
stream_id when applicable) and ensure proxy.stream.subscribe responses omit
stream_id; refactor MobileHostService by extracting RPC subscription logic into
smaller methods (e.g., separate handleSubscriptionRPC,
buildMobileEventSubscribeResponse, buildProxyStreamSubscribeResponse) to
separate concerns and avoid mixing protocol paths, and update the response
construction to clearly document/emit the correct keys per RPC type.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2794-2799: The continuation after await
persistPairedMacFromTicket(ticket) can run against a torn-down client; capture
and revalidate the connection generation (or check remoteClient) before applying
state. In MobileShellComposite's connect(...) flow, save a local snapshot of the
current generation/token or the presence of remoteClient before the await, and
after await compare it to the current generation (and ensure remoteClient is
still non-nil); only then call applyRemoteWorkspaceList(...),
syncSelectedTerminalForWorkspace(), set connectionState = .connected and
markMacConnectionHealthy(); otherwise abort the continuation to avoid reapplying
stale state.
🪄 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: 588b5735-55ba-4786-9cae-eda2468b2263
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftSources/Mobile/MobileHostService.swift
🛑 Comments failed to post (7)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1847-1868:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check
ifStillCurrentafter theloadAllawait.The new display-name fallback adds a suspension between the serialized currentness check and the
upsert(markActive: true). If the user disconnects, forgets the Mac, or switches to another Mac duringloadAll, this stale task still writes the old row back as active.Suggested fix
await performSerializedPairedMacWrite(ifStillCurrent: ifStillCurrent) { [weak self] in guard let self else { return } var displayName = ticketDisplayName if displayName == nil { let knownMacs = (try? await pairedMacStore.loadAll(stackUserID: nil)) ?? [] let matches = knownMacs.filter { $0.macDeviceID == ticket.macDeviceID } displayName = (matches.first { $0.stackUserID == stackUserID } ?? matches.first)? .displayName } + if let ifStillCurrent, !ifStillCurrent() { return } do { try await pairedMacStore.upsert( macDeviceID: ticket.macDeviceID, displayName: displayName, routes: ticket.routes, markActive: true, stackUserID: stackUserID ) - self.hasKnownPairedMac = true + if let ifStillCurrent, !ifStillCurrent() { return } + self.hasKnownPairedMac = true } catch { mobileShellLog.error("paired mac store upsert failed: \(String(describing: error), privacy: .public)") } }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1847 - 1868, The task may resume after awaiting pairedMacStore.loadAll and then perform an upsert despite the serialized "currentness" check; re-check the ifStillCurrent condition after the await and before calling pairedMacStore.upsert so the task aborts if it is no longer the current writer. In practice, inside the closure passed to performSerializedPairedMacWrite (the block that uses pairedMacStore.loadAll and then upsert), call the same ifStillCurrent check again (awaiting it if it's async) after computing displayName and before executing pairedMacStore.upsert(macDeviceID:..., markActive: true, ...), and return early if the check indicates this operation is stale.Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift (1)
46-67:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTear down each mounted
OutputCollectorwithdefer.
mount(store:surfaceID:)starts an unstructured task overterminalOutputStream. Right now cleanup only happens on the happy path, so any thrown#require/helper failure after mount leaves the collector subscribed and the store retained until the stream terminates. Putdefer { collector.unmount() }immediately after each mount to keep failures isolated.🤖 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/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift` around lines 46 - 67, The OutputCollector mounted with collector.mount(store: store, surfaceID: "live-terminal") isn't guaranteed to be unmounted on failures; add a defer { collector.unmount() } immediately after the mount call so unmount() always runs even if subsequent helpers like `#require`(box.get()) or renderGridEventFrame throw, ensuring the store and terminalOutputStream task are released on error paths.Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift (3)
159-160:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the scripted subscription on
mobile.events.unsubscribe.The router keeps
hasActiveSubscription == trueeven after an unsubscribe succeeds. That makes the next subscribe after a client-driven teardown look likealready_subscribed: true, which collapses the distinction between a clean restart and the “lost registration repaired in place” path this PR is exercising.Suggested fix
- case "mobile.events.unsubscribe", "mobile.terminal.replay", "mobile.terminal.viewport": + case "mobile.events.unsubscribe": + hasActiveSubscription = false + return try? Self.resultFrame(id: id, result: [:]) + case "mobile.terminal.replay", "mobile.terminal.viewport": return try? Self.resultFrame(id: id, 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/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift` around lines 159 - 160, The unsubscribe branch for "mobile.events.unsubscribe" should clear the scripted subscription state so the router's hasActiveSubscription is set to false (or call the existing clearScriptedSubscription/clearSubscription helper) before returning the result; modify the case that currently returns try? Self.resultFrame(id: id, result: [:]) to first clear the subscription state (referencing hasActiveSubscription or clearScriptedSubscription) and then return Self.resultFrame(id: id, result: [:]).
229-279:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop late frames once the transport is closed.
close()marks the transport closed, butreceive()still drainspendingFramesbefore honoringisClosed, anddeliver(_:)still queues frames after close. If a held probe/subscribe response is released after teardown, the old client can still consume that stale ack from a “closed” transport, which weakens the late-completion/generation-ownership coverage these tests are supposed to validate.Suggested fix
func receive() async throws -> Data? { - if !pendingFrames.isEmpty { - return pendingFrames.removeFirst() - } if isClosed { return nil } + if !pendingFrames.isEmpty { + return pendingFrames.removeFirst() + } return await withCheckedContinuation { continuation in receiveWaiters.append(continuation) } @@ func close() async { isClosed = true + pendingFrames = [] let waiters = receiveWaiters receiveWaiters = [] for waiter in waiters { waiter.resume(returning: nil) } @@ func deliver(_ frame: Data) { + guard !isClosed else { return } if receiveWaiters.isEmpty { pendingFrames.append(frame) return }🤖 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/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift` around lines 229 - 279, The transport currently allows late frames to be consumed after close because receive() drains pendingFrames before checking isClosed and deliver(_:) still appends frames when closed; update receive() to check isClosed first and return nil immediately if closed (rather than draining pendingFrames), change deliver(_:) to drop frames when isClosed is true instead of appending to pendingFrames or resuming waiters, and have close() clear pendingFrames as well as resume and clear receiveWaiters; reference the functions receive(), deliver(_:), close() and the properties pendingFrames, receiveWaiters, isClosed when making these changes.
378-380:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when the scripted connection precondition breaks.
#expect(connected, ...)records the setup failure but still returns a possibly disconnected store. Since every liveness test shares this helper, a connect regression turns into several follow-on poll timeouts instead of stopping at the real broken precondition.Suggested fix
let connected = await store.connectPairingURL(try attachURL(for: ticket)) - `#expect`(connected, "scripted connect must succeed") + try `#require`(connected, "scripted connect must succeed") return store🤖 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/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift` around lines 378 - 380, Replace the non-fatal test check so the helper aborts immediately on a failed scripted connect: where the code currently does let connected = await store.connectPairingURL(try attachURL(for: ticket)) followed by `#expect`(connected, "scripted connect must succeed"), change that `#expect` to a hard failure (e.g. precondition(connected, "scripted connect must succeed") or fatalError with the same message) so connectPairingURL/attachURL failures stop the test run instead of returning a possibly disconnected store.Sources/Mobile/MobileHostService.swift (2)
320-387: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Split the new status/auth logic out of this file instead of growing
MobileHostService.swiftfurther.This PR adds more status shaping, verification-policy, and verifier-cache code into a file that already owns listener lifecycle, connection tracking, RPC dispatch, and per-connection transport. Moving the status/auth pieces into sibling types under
Sources/Mobile/would keep this path testable without deepening an already oversized service file. As per coding guidelines, production Swift files over 800 lines should be flagged, and files that mix responsibilities should be split. Based on learnings, keep that split local toSources/rather than proposing a new SwiftPM package here.Also applies to: 1227-1279, 1725-1742
🤖 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/MobileHostService.swift` around lines 320 - 387, This file mixes new status/auth logic with an oversized MobileHostService; extract the status shaping and verification policy into one or more sibling types under Sources/Mobile (e.g., a MobileHostStatusManager or MobileHostStatusPayload and MobileHostStatusVerifier types) and move implementations of publicStatusPayload(routesPayload:), identityStatusPayload(routesPayload:), networkStatusResult(for:), and any verifier-cache/limiter logic (MobileHostStatusVerificationLimiter, MobileHostPublicStatusCache, verifiedStackCaller(for:)) into those new files; then have MobileHostService call the new type(s) for payload creation and verification so the service file keeps only lifecycle/transport/RPC dispatch concerns and the status/auth logic becomes unit-testable and localized.Sources: Coding guidelines, Learnings
2180-2184: 🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C3 'already_subscribed|stream_id|subscribed' \ Packages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileEventSubscribeResponse.swift \ Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift \ Sources/Mobile/MobileHostService.swift \ daemon/remote/cmd/cmuxd-remote/main.goRepository: manaflow-ai/cmux
Length of output: 17889
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Find the actual producer for `mobile.events.subscribe` (if any) in cmuxd-remote. rg -n --hidden "mobile\.events\.subscribe" daemon/remote/cmd/cmuxd-remote/main.go # 2) See whether `mobile.events.subscribe` is implemented via/proxies to `proxy.stream.subscribe`. rg -n --hidden "proxy\.stream\.subscribe|handleProxyStreamSubscribe" daemon/remote/cmd/cmuxd-remote/main.go # 3) Get the current line count for the oversized-file concern. wc -l Sources/Mobile/MobileHostService.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash set -uo pipefail echo "== matches for mobile.events.subscribe in daemon/remote/cmd/cmuxd-remote/main.go ==" rg -n --hidden "mobile\.events\.subscribe" daemon/remote/cmd/cmuxd-remote/main.go || true echo "== matches for proxy.stream.subscribe in daemon/remote/cmd/cmuxd-remote/main.go ==" rg -n --hidden "proxy\.stream\.subscribe|handleProxyStreamSubscribe" daemon/remote/cmd/cmuxd-remote/main.go || true echo "== line count ==" wc -l Sources/Mobile/MobileHostService.swiftRepository: manaflow-ai/cmux
Length of output: 518
🏁 Script executed:
#!/bin/bash set -euo pipefail # Where is `mobile.events.subscribe` handled/produced? rg -n --hidden "case \"mobile\.events\.subscribe\"|mobile\.events\.subscribe" daemon/remote cmd Sources Packages cmuxd ghostty ghosttyKit 2>/dev/null || true # Does any code map `mobile.events.subscribe` -> `proxy.stream.subscribe`? rg -n --hidden "mobile\.events\.subscribe.*proxy\.stream\.subscribe|proxy\.stream\.subscribe.*mobile\.events\.subscribe" . || true # Where is `MobileEventSubscribeResponse` used (ensures what fields are expected)? rg -n --hidden "MobileEventSubscribeResponse" Packages Sources daemon/remote . || trueRepository: manaflow-ai/cmux
Length of output: 5714
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== proxy.stream.subscribe call sites ==" rg -n --hidden "\"proxy\.stream\.subscribe\"|proxy\.stream\.subscribe" Sources Packages daemon/remote cmd . || true echo "== MobileEventSubscribeResponse decode call sites ==" rg -n --hidden "MobileEventSubscribeResponse\.decode|MobileEventSubscribeResponse" Sources Packages daemon/remote cmd . || true echo "== proxy.stream.subscribe ack decoding ==" rg -n --hidden "proxy\.stream\.subscribe.*decode|decode.*proxy\.stream\.subscribe|handleProxyStreamSubscribe" Sources Packages . || trueRepository: manaflow-ai/cmux
Length of output: 5307
Clarify
stream_idrollout: omission is forproxy.stream.subscribe, notmobile.events.subscribe
MobileHostService.handleSubscriptionRPCreturnsmobile.events.subscribesuccess with a non-empty"stream_id".daemon/remote/cmd/cmuxd-remote/main.gosuccess payload lacking"stream_id"is fromproxy.stream.subscribe(notmobile.events.subscribe), so it’s not theMobileEventSubscribeResponseack path.Sources/Mobile/MobileHostService.swiftis 2340 lines; split/extract RPC subscription/protocol handling into smaller units to reduce mixed responsibilities and improve maintainability.🤖 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/MobileHostService.swift` around lines 2180 - 2184, The response payload currently returns "stream_id" in MobileHostService.handleSubscriptionRPC but the absence of "stream_id" observed in daemon/remote/cmd/cmuxd-remote/main.go refers to proxy.stream.subscribe (not mobile.events.subscribe), so update the handling to: make MobileHostService.handleSubscriptionRPC explicitly produce the MobileEventSubscribeResponse for mobile.events.subscribe (always include stream_id when applicable) and ensure proxy.stream.subscribe responses omit stream_id; refactor MobileHostService by extracting RPC subscription logic into smaller methods (e.g., separate handleSubscriptionRPC, buildMobileEventSubscribeResponse, buildProxyStreamSubscribeResponse) to separate concerns and avoid mixing protocol paths, and update the response construction to clearly document/emit the correct keys per RPC type.
Reproduces the ipad CI failure of macConnectionStatusMarksUnavailableWhenEventStreamCloses: with the start handshake made concurrent, a transport that drops before the subscribe ack lands lets the stream-end restart supersede the listener generation; the parked ack's failure verdict is then silently dropped by its generation guard and recovery loops in reconnecting forever instead of reaching unavailable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the event stream ends while the generation's enable handshake is still in flight, stop and mark the Mac unavailable instead of restarting: the restart superseded the generation, swallowed the handshake's failure verdict, and livelocked reconnecting against a closed transport. A healthy stream cannot hit this path (it only ends when the transport drops), and a mid-life stream end with a completed handshake still takes the existing restart path. MobileShellComposite budget +15 for the guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Update to the adjudication above: the start-path concern family was partially RIGHT, with a different mechanism than either bot described. The ipad run failed macConnectionStatusMarksUnavailableWhenEventStreamCloses deterministically: when the transport drops before the start ack lands, the stream-end restart supersedes the listener generation, the parked ack's failure verdict gets dropped by its generation guard, and recovery livelocks in reconnecting (iphone passed only because the ack failure sometimes wins the race). Fixed in two commits, red test (98dbf2c) then fix (ab8f139): a stream that ends before its handshake completes is treated as a failed start (stop + unavailable) instead of restarting. Deferral of the two follow-ups stands; the convergence refactor now has a pinned regression test. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ab8f139. Configure here.
| diagnosticLog?.record(DiagnosticEvent(.error)) | ||
| stopTerminalRefreshPolling() | ||
| markMacConnectionUnavailable() | ||
| return |
There was a problem hiding this comment.
Pending ack misread as failed start
Medium Severity
When the terminal event stream ends, the handler treats any in-flight terminalSubscriptionStartTask as a failed start and calls markMacConnectionUnavailable with connectionRecoveryFailed, instead of using the reconnecting path. That check is only “subscribe ack not finished,” not “handshake never delivered.” If events were already consumed while the start ack was still in flight, the stream can end with a healthy consumer but the UI still lands in failed recovery with polling stopped.
Reviewed by Cursor Bugbot for commit ab8f139. Configure here.
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
…watchdog, #5869) Branch advanced past the 11b carry: stream-end-before-start-ack now fails the start (converges to unavailable instead of reconnect livelock), and liveness test fixtures split into MobileShellRenderGridLivenessTestSupport. Composite keeps dog's dismiss-sync notification refresh lines the branch lacks; the liveness test file takes the branch's current form.
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
Keeps dog's dismiss-sync extras over main's pre-dismiss watchdog form and deduplicates the reconcile-sweep call that two dismiss folds each inserted into the subscribe path.
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
…adge (#5916) * Notifications: cross-device dismiss-sync + stable id end-to-end A Mac notification now carries its stable TerminalNotification.id through the APNs push (cmux.notificationId + apns-collapse-id), so a dismiss on one surface targets the same notification on the other. iOS->Mac: the iOS cmux.terminal category gets .customDismissAction; a swipe (or tap) routes UNNotificationDismissActionIdentifier -> MobilePushCoordinator -> a new notification.dismiss mobile-host RPC -> TerminalNotificationStore .markRead. Mac->iOS: user-driven dismiss/clear emits a notification.dismissed peer event the phone uses to removeDeliveredNotifications. Echo loop broken by the markRead/remove no-op guards. Also fixes show-on-iOS: the push never carried the id, so the delivered banner had an OS-random identifier unlinked from the Mac store. Caveat (Phase 1): iOS->Mac dismiss only fires while the phone is attached + foregrounded; the detached/lock-screen case needs a reverse web/APNs path (deferred follow-up). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Notifications: authoritative unread badge + reconcile sweep + cold dismiss lane Badge: the iOS app icon badge mirrors the Mac's unread-notification-entry count. Every lane SETS the absolute server-/Mac-computed total, never local arithmetic, so drift self-heals: - live: new notification.badge peer event emitted from refreshUnreadPresentation() (the same chokepoint as the Dock badge), and notification.dismissed now carries unread_count too - push: every APNs push (notify + dismiss) carries badgeCount, stamped by the web service as aps.badge - reconcile: new notification.reconcile mobile-host RPC; on every (re)subscribe the phone sends its delivered banner ids, the Mac answers with the handled subset (read, or tombstoned dismissed/removed) plus the unread count, and the phone clears those banners and sets the badge Cold dismiss lane: when the Mac dismisses/clears while no phone is live-subscribed, PhonePushClient.forwardDismissed sends a silent content-available push (priority 5, no alert, aps.badge + cmux.dismissedIds, no collapse id) so a pocketed phone drops the badge instantly and removes banners when iOS grants the strictly budgeted background wake; the reconcile sweep heals anything iOS defers. Bursts coalesce structurally in a drain task chunked to the server's MAX_PUSH_DISMISS_IDS cap. Dismiss pushes carry only opaque UUIDs and a count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Tests: badge derivation, reconcile classification, dismiss-push policy Web (bun test): parsePushPayload dismiss kind (text-free, ids required, bounded), badge-count tolerance (malformed ignored, runaway clamped), dismiss payload shape (content-available + badge + dismissedIds, nothing visible), sender headers (dismiss never collapses onto the banner and downgrades to apns-priority 5; notify keeps immediate priority), and the two parse-shape tests updated for the new fields. Mac (cmuxTests): notification.reconcile RPC classifies handled = read-in-store or tombstoned-removed, leaves unread and foreign ids alone, and returns the authoritative unread count; empty delivered_ids is a valid badge-only sync; a markUnread resurrect beats a stale dismiss tombstone; the phone badge counts unread notification entries only (workspace manual unread indicators feed the Dock badge, not the phone). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WIP snapshot: rate-limit outage safety push * Notifications: durable phone→Mac dismiss outbox (enqueue-first send, flush on resubscribe) A banner swipe can arrive when the dismiss cannot be sent: the app may be background-launched from Notification Center before any scene (and store) exists, and the attach channel is usually down in the background. Park every phone-side dismiss in PendingNotificationDismissQueue (UserDefaults- backed) before attempting the notification.dismiss RPC, remove it only on confirmed delivery, and flush the queue at the start of every successful (re)subscribe, before the reconcile sweep so the Mac's answer already reflects the replayed swipes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: send the cold-lane dismiss push unconditionally (multi-device) The APNs push route fans out to every iOS device token registered for the user, but the cold-lane dismiss push was suppressed whenever ANY client was live-subscribed to notification.dismissed. With two devices, the attached one starved the offline one: it got neither the live event nor the silent dismiss+badge push and kept stale banners until its next reconcile. Drop the subscriber gate; the push is idempotent on the live device (removing an already-removed banner is a no-op, the badge is an absolute SET) and bursts coalesce in PhonePushClient.forwardDismissed. Found by autoreview (P2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: persist dismiss tombstones; cap+dedupe notification.dismiss ids Two autoreview P2s. The reconcile lane's tombstone ring was in-memory only, so a Mac relaunch forgot that removed/cleared notifications had been handled and a phone whose silent dismiss push was dropped kept the stale banner forever; the bounded ring is now write-through persisted to UserDefaults and lazy-loaded (session restore keeps notification ids, so the persisted ids stay meaningful across relaunch). notification.dismiss now caps its id array at 256 like notification.reconcile and dedupes ids before processing, so a malformed frame cannot force unbounded main-actor work and a duplicated id cannot double-count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: defer superseded-banner dismiss behind the replacement push Autoreview P1. The supersede path dismissed the old phone banner unconditionally, but the replacement banner push goes through PhonePushClient.forward's 1s per-tab/surface throttle; in a rapid burst the phone lost its only banner for a still-unread notification. forward now reports whether the push was queued, recordNotification stashes the superseded ids in a bounded per-key buffer (tombstoning them immediately so reconcile stays correct), and deliverNotificationSideEffects emits the dismiss only after a replacement push is queued, so clear+replace is atomic from the phone's perspective. A throttled burst now degrades to the pre-existing behavior (stale-text banner stays visible) instead of no banner at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: map dismisses via cmux.notificationId; emit superseded dismiss when no replacement will forward Two autoreview findings. The iOS clearer trusted that a delivered remote notification's request identifier equals the Mac UUID (apns-collapse-id equivalence), which is observed OS behavior, not a contract; it now enumerates delivered notifications and maps the authoritative cmux.notificationId payload key to actual request identifiers for removal, and reports Mac ids (payload key, request-identifier fallback) to the reconcile sweep. And the superseded-banner dismiss deferral could leave ids stuck forever on paths where no replacement push is forwarded (suppressed/focused, non-desktop effects, forwarding off); those paths now emit the dismiss immediately, and only a genuinely throttled replacement defers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: await banner removal so background dismiss wakes finish their work Autoreview P1. application(_:didReceiveRemoteNotification:) returned its fetch result while removeDelivered was still an unstructured Task, so iOS could suspend the process before the delivered-banner enumeration/removal ran, silently breaking the cold dismiss lane it exists for. The DeliveredNotificationClearing removal is now async and awaited end to end (app delegate → coordinator → clearer, and composite event/reconcile paths), so the delegate reports completion only after the banners are gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: spell out UUID(uuidString:) in tombstone load for style consistency The unlabeled initializer reference compiled fine, but every other UUID-parsing site in this file uses the explicit closure form; match it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: drain stale superseded-banner stash on user-driven read/clear paths Autoreview P2. Ids stashed after a throttled replacement push only flushed on a future successful forward for the same key; if the user then read, removed, or cleared the current notification, those paths emitted only the current ids, so an offline phone kept the stale old banner until its next reconcile. Every user-driven read/clear/remove now drains the buffer at the matching scope (precise tab/surface key for single-entry ops, tab-wide for tab ops, everything for clear-all/mark-all-read) and rides the drained ids along on its emit. Once the current notification is handled there is no replacement left to wait for, so emitting the stragglers is exactly right. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: DocC docs for public package symbols (policy) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Split over-budget files for the length guard; no behavior changes The dismiss-sync feature pushed five tracked files over their length budgets. Extractions instead of budget refreshes: - cmuxTests/TerminalAndGhosttyTests.swift: the dismiss-sync tests move to cmuxTests/NotificationDismissSyncTests.swift (wired in pbxproj). - Sources/TerminalController.swift: the mobile notification.dismiss / notification.reconcile handlers and the mobile workspace-action gate move to Sources/TerminalController+MobileNotificationSync.swift. - Sources/Mobile/MobileHostService.swift: mobileHostCapabilities moves to Sources/Mobile/MobileHostService+Capabilities.swift. - MobileShellComposite.swift: the dismiss-sync section moves to MobileShellComposite+NotificationDismissSync.swift and the dogfood feedback round-trip to MobileShellComposite+DogfoodFeedback.swift; four members lose `private` (same module) for the split. - Sources/TerminalNotificationStore.swift: the pre-existing 600-line NotificationSoundSettings enum moves wholesale to Sources/NotificationSoundSettings.swift. Its call graph is too entangled to split under 500 lines without ~12 access relaxations, so it becomes a tracked budget entry at its actual 603 lines; TerminalNotificationStore's budget tightens 2527 -> 2274. The TSV is re-sorted (it had drifted out of size order). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: retrigger checks (Actions never created a check suite for 5bd0f90) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Preview stores get a no-op delivered-notification clearer The reconcile sweep now runs on every successful subscribe ack, and the render-grid liveness tests (post-#5869) drive full subscribe flows on MobileShellComposite.preview(), whose default SystemDeliveredNotificationClearer hits UNUserNotificationCenter.current(), which traps in processes without a bundle proxy (swift test) and would mutate the real notification center in SwiftUI previews. preview() now injects a no-op clearer; the app composition root still gets the system clearer by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Notifications: gate superseded-banner buffering on the real send decision The superseded-banner dismiss was deferred whenever forwarding was merely enabled, but PhonePushClient.forward also drops the replacement push when the .onlyWhenAway presence gate sees the Mac as active. In that path the old phone banner was stashed waiting for a replacement push that never came, so a phone that received the earlier banner (while the Mac was away) kept a stale banner after the Mac cleared it, until the next foreground reconcile. Add PhonePushClient.willForwardReplacement(), which mirrors forward's enable + presence gate but ignores the burst throttle (the throttle is the one legitimate defer-and-flush case). recordNotification now uses it, so a presence-suppressed supersede emits the dismiss immediately (live + cold APNs lanes) instead of stashing it. Unit-tested against the shared client with an injected presence monitor and scratch defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Notifications: extract SupersededPhoneDismissBuffer to its own file File-organization policy: the per-tab/surface superseded-banner stash was a second major type appended to TerminalNotificationStore.swift. Move it to SupersededPhoneDismissBuffer.swift (no behavior change) and wire it into the cmux target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
…#5596/#5625/#5628) over current main Beta queue (#5876/#5872/#5869/#5875/#5927/#5912/#5726/#5776/#5916) is now on main; conflicts resolved by taking main as authoritative for the merged workspace-list/notifications/read-state/close surface, while preserving the carry-set: terminal.paste capability (#5572), hidden-input strings (#5596), smooth-scroll/scroll-to-bottom (#5628), and the live notifications feed (notificationsStore + mobile.notifications.list/mark_read dispatch). Dropped the superseded mute design. Capability flags unified onto main's computed supportedHostCapabilities set (added computed supportsTerminalPaste + DEBUG supportsDogfoodChecklist). xcstrings merged (HEAD-precedence union, mute keys dropped). pbxproj took HEAD consistently; budget regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 27, 2026
This branch was successfully deployed
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.
Fixes the render-grid liveness watchdog false-fire found during the 2026-06-10 Release-sim bisect: the phone logged
render-grid stream silent for 10499ms, re-subscribingevery ~10.5s forever, plussubscribe failed reason=start: requestTimedOut, while the Mac demonstrably kept delivering (mobile.emit -> connection delivered=true). Every false fire made the Mac re-replay the full grid, a constant repaint every 10.5s (plausibly the reported "weird cursor paints") plus wasted battery and bandwidth.Root cause
Two decouplings between the liveness recorder and reality:
e3f418560) assumed an actively-streaming connection and never accounted for idle.for awaitconsumer loop, which did not start untilresolveTerminalOutputTransportand themobile.events.subscribeack both completed. Events delivered during that establishment window piled up unconsumed in the subscription stream's buffer, invisible to the clock. Each watchdog fire then cancelled its own in-flight start subscribe;MobileCoreRPCSession.cancelPendingRequestsurfaces that cancellation asrequestTimedOut(the real wire timeout is 30s, so the 10.5s cadence of that log was the watchdog cancelling itself, not the network), and the dying generation calledmarkMacConnectionUnavailable()underneath the fresh one.Fix
recordTerminalEventStreamLiveness()is stamped by every consumed envelope, by a successful probe, and (as the generation reset) when a watchdog generation is armed. The watchdog reads the same record;listenerIDguards every transition;stopRenderGridLivenessWatchdogresets cleanly.mobile.events.subscribeprobe (samestream_id, current topics) instead of tearing down. A completed round-trip proves the transport the events ride on is alive AND that the registration is installed. Only a failed probe runs the existing teardown + re-subscribe + replay recovery, so the original ~85s silent-death case still heals (probe deadline default 3s, runtime-injectable, bounds the whole attempt including pre-wire token work).already_subscribed. When the probe reinstalls a registration the host had lost (already_subscribed: false), delta continuity is broken, so the phone replays every mounted surface and re-fetches the workspace list, without restarting the intact phone-side listener. Older Macs omit the field; their registrations cannot outlive the connection, sonilis treated as already-active.beginTerminalEventSubscriptionStart). The ack is a server-side enable handshake, not a delivery precondition, so the consumer loop starts immediately and the clock stays coupled to actual event arrival. Ack success/failure is acted on only while the generation is current, so a superseded ack can no longer mark the connection unavailable, and a cancelled ack logs as cancelled instead of a fake wire timeout.Red/green
Commit 1 (4c57e66ba) adds the failing tests plus a DEBUG-only seam to run one watchdog evaluation deterministically against the injected clock; the scripted host drives the real
MobileCoreRPCSessiontransport and the real consumer path.renderGridEventsArrivingDuringStartSubscribeAreConsumedand the healthy-idle watchdog test fail against pre-fix code (the false fire reproduces 1:1:sync.liveness re-subscribe silentMs=10000plus the replay storm);watchdogStillResubscribesGenuinelyDeadStreampins the original recovery so the fix cannot regress it. The probe redesign retargeted the idle test's teardown signals to listener restarts and replay traffic (the probe itself legitimately re-sendsmobile.events.subscribe); both liveness tests were re-verified red against the commit-1 sources after the amendment.swift test --package-path Packages/CmuxMobileShell: 59 tests green.swift test --package-path Packages/CmuxMobileRPC: 23 green. Build-verified Debug and Release for arm64 simulator (/tmp/cmux-watchdog) and the macOS app (/tmp/cmux-watchdog-mac, host-side ack change).Residual risk
A Mac-side producer wedge with the registration intact (demand tracker desync deeper than a re-subscribe replace can re-arm) is not phone-detectable and is not recovered by this watchdog; the input-driven seq self-heal still covers it during typing. Dead-stream detection now takes threshold + probe deadline (~12s) instead of ~9s.
🤖 Generated with Claude Code
Review fixes (autoreview round 1)
markMacConnectionHealthy(): the round-trip is positive proof of the client/host connection, so a transient foreground RPC failure can no longer leave the status UI stuck unavailable on an idle terminal.mobile.events.subscribe/unsubscribeare now non-interactive inMobileHostRequestActivityaccounting (joiningmobile.host.statusand the replays), so the idle probe cannot keep the Mac permanently "recently active" and starve background work gated on mobile quiet (TabManager git/PR metadata refresh).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile sync connection recovery and Mac RPC subscription semantics; behavior is heavily regression-tested but wrong probe/repair logic could still cause stale UI or excess replay traffic.
Overview
Fixes the iOS render-grid liveness watchdog falsely tearing down healthy idle Mac connections (~10.5s full-grid replay loop).
Watchdog behavior: Prolonged silence no longer immediately triggers re-subscribe + replay. It first runs a bounded idempotent
mobile.events.subscribeprobe (configurable vialivenessProbeTimeoutNanoseconds, default 3s). Only a failed probe runs the existing resync path. A successful probe refreshes the liveness clock and marks the connection healthy; if the host reportsalready_subscribed: false, mounted surfaces are replayed and the workspace list is refreshed without restarting the listener.Subscribe handshake: The start
mobile.events.subscribeack runs concurrently with the event consumer so buffered events during establishment update liveness. Stream end before that ack completes now marks the connection unavailable instead of reconnect looping. Cancelled subscribe requests log as cancelled, not wire timeouts.Mac host: Subscribe RPC responses include
already_subscribed.mobile.events.subscribe/unsubscribeare treated as non-interactive so idle probes do not block background Mac work.Tests: New
MobileShellRenderGridLivenessTestsplus scripted host fixtures cover establish-window consumption, healthy idle, lost-registration repair, dead-stream recovery, and pre-ack stream close.Reviewed by Cursor Bugbot for commit ab8f139. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the iOS render‑grid liveness watchdog false‑fires by probing before teardown and consuming events while the start‑subscribe ack is in flight. Also handles a stream that ends before the start ack by marking the Mac unavailable, preventing a reconnect loop.
Bug Fixes
mobile.events.subscribeprobe (samestream_id/topics) before recovery; only a failed probe re‑subscribes and replays. Deadline viaMobileSyncRuntime.livenessProbeTimeoutNanoseconds(default 3s).requestTimedOut.already_subscribed(decoded asalreadySubscribed); whenfalse, replay mounted surfaces and refresh the workspace list without restarting the listener.nilis treated as already active for older hosts.mobile.events.subscribe/unsubscribeare counted as non‑interactive host activity. Probe deadline uses a one‑shotDispatchSourceTimer.Tests
Written for commit ab8f139. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests