Repository navigation
Fix iPad-simulator flake in renderGridTerminalInputWaitsForLiveEventBeforeReplay (#5911) - #6330
austinywang wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 2 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR introduces a
Confidence Score: 3/5The infrastructure added is correct, but the fix was only applied to one of the three tests the PR explicitly targeted; the other two still have the polling loops that caused the original flake. The ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift — lines 2448, 2456, and 2550 still contain the fragile polling loops in the two unfixed sibling tests. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Test
participant Collector as TerminalOutputCollector
participant Stream as terminalOutputStream
participant Watchdog as Watchdog Task
Test->>Collector: waitForLines(2)
Collector->>Collector: "park CheckedContinuation (id=0)"
Collector->>Watchdog: spawn sleep(10s) then resumeWaiter(0)
Stream-->>Collector: chunk 1 append + resumeSatisfiedWaiters
Note over Collector: count=1 < 2 waiter stays parked
Stream-->>Collector: chunk 2 append + resumeSatisfiedWaiters
Note over Collector: count=2 >= 2 continuation.resume()
Collector->>Watchdog: watchdog.cancel()
Collector-->>Test: waitForLines returns
Test->>Test: assert collector.lines
Test->>Collector: unmount()
Collector->>Collector: cancel task and release parked waiters
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Test
participant Collector as TerminalOutputCollector
participant Stream as terminalOutputStream
participant Watchdog as Watchdog Task
Test->>Collector: waitForLines(2)
Collector->>Collector: "park CheckedContinuation (id=0)"
Collector->>Watchdog: spawn sleep(10s) then resumeWaiter(0)
Stream-->>Collector: chunk 1 append + resumeSatisfiedWaiters
Note over Collector: count=1 < 2 waiter stays parked
Stream-->>Collector: chunk 2 append + resumeSatisfiedWaiters
Note over Collector: count=2 >= 2 continuation.resume()
Collector->>Watchdog: watchdog.cancel()
Collector-->>Test: waitForLines returns
Test->>Test: assert collector.lines
Test->>Collector: unmount()
Collector->>Collector: cancel task and release parked waiters
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func terminate(processGroupIDs: [Int], graceSeconds: TimeInterval = 3) { | ||
| let pgids = processGroupIDs.filter { $0 > 1 } | ||
| guard !pgids.isEmpty else { return } | ||
| for pgid in pgids { | ||
| _ = kill(pid_t(-pgid), SIGTERM) | ||
| } | ||
| DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + graceSeconds) { | ||
| for pgid in pgids { | ||
| _ = kill(pid_t(-pgid), SIGKILL) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
DispatchQueue.global(qos:).asyncAfter for SIGKILL scheduling
PaneMemoryProcessKiller.terminate uses DispatchQueue.global(qos: .userInitiated).asyncAfter to delay the SIGKILL 3 seconds after SIGTERM. This is new production Swift using both DispatchQueue.global and asyncAfter, both explicitly flagged by the cmux-swift-blocking-runtime and cmux-swift-concurrency-modernization rules. Beyond the pattern violation, the fire-and-forget dispatch has no cancellation path: if the pane is closed or clearAll() is called before the 3-second window expires, the SIGKILL to the old process group IDs still fires. Rapid double-invocations of killActivePaneProcess() also stack unrelated pending SIGKILL closures with no way to discard the earlier one. The Swift-idiomatic replacement is a stored Task.detached { try? await Task.sleep(for: .seconds(graceSeconds)); for pgid in pgids { kill(pid_t(-pgid), SIGKILL) } } that can be cancelled.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @MainActor | ||
| final class PaneMemoryGuardrail: ObservableObject { | ||
| static let shared = PaneMemoryGuardrail() | ||
|
|
||
| enum DefaultsKeys { | ||
| static let enabled = "terminal.runawayMemoryGuardrail.enabled" | ||
| static let thresholdGB = "terminal.runawayMemoryGuardrail.thresholdGB" | ||
| } | ||
|
|
||
| private static let pollInterval: TimeInterval = 4 | ||
| private static let defaultThresholdGB: Double = 8 | ||
| private static let minThresholdGB: Double = 1 | ||
|
|
||
| /// The banner content for the most recent un-dismissed crossing, or nil. | ||
| @Published private(set) var activeBanner: PaneMemoryWarning? |
There was a problem hiding this comment.
PaneMemoryGuardrail is a new production class with a single @Published observable property — exactly the pattern cmux-swiftui-state-layout flags as needing @Observable. With @Observable, PaneMemoryGuardrailBannerView replaces @ObservedObject var guardrail with a plain stored property, and SwiftUI tracks only activeBanner reads rather than invalidating on every @Published change.
| @MainActor | |
| final class PaneMemoryGuardrail: ObservableObject { | |
| static let shared = PaneMemoryGuardrail() | |
| enum DefaultsKeys { | |
| static let enabled = "terminal.runawayMemoryGuardrail.enabled" | |
| static let thresholdGB = "terminal.runawayMemoryGuardrail.thresholdGB" | |
| } | |
| private static let pollInterval: TimeInterval = 4 | |
| private static let defaultThresholdGB: Double = 8 | |
| private static let minThresholdGB: Double = 1 | |
| /// The banner content for the most recent un-dismissed crossing, or nil. | |
| @Published private(set) var activeBanner: PaneMemoryWarning? | |
| @MainActor | |
| @Observable | |
| final class PaneMemoryGuardrail { | |
| static let shared = PaneMemoryGuardrail() | |
| enum DefaultsKeys { | |
| static let enabled = "terminal.runawayMemoryGuardrail.enabled" | |
| static let thresholdGB = "terminal.runawayMemoryGuardrail.thresholdGB" | |
| } | |
| private static let pollInterval: TimeInterval = 4 | |
| private static let defaultThresholdGB: Double = 8 | |
| private static let minThresholdGB: Double = 1 | |
| /// The banner content for the most recent un-dismissed crossing, or nil. | |
| private(set) var activeBanner: PaneMemoryWarning? |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private let queue = DispatchQueue(label: "com.cmux.pane-memory-guardrail", qos: .utility) | ||
| private var timer: DispatchSourceTimer? | ||
| private var isScanning = false | ||
| private var lastSamplesByKey: [PaneMemoryPaneKey: PaneMemorySample] = [:] | ||
| private var lastWarnedWorkspaceIds: Set<UUID> = [] | ||
|
|
||
| func start() { | ||
| guard timer == nil else { return } | ||
| let timer = DispatchSource.makeTimerSource(queue: queue) | ||
| timer.schedule( | ||
| deadline: .now() + Self.pollInterval, | ||
| repeating: Self.pollInterval, | ||
| leeway: .seconds(1) | ||
| ) | ||
| timer.setEventHandler { [weak self] in | ||
| Task { @MainActor in self?.tick() } | ||
| } | ||
| self.timer = timer | ||
| timer.resume() | ||
| } | ||
|
|
||
| // MARK: Settings | ||
|
|
||
| private var isEnabled: Bool { | ||
| UserDefaults.standard.object(forKey: DefaultsKeys.enabled) as? Bool ?? true | ||
| } | ||
|
|
||
| private func thresholdBytes() -> Int64 { | ||
| let configured = UserDefaults.standard.object(forKey: DefaultsKeys.thresholdGB) as? Double | ||
| ?? Self.defaultThresholdGB | ||
| let gb = max(Self.minThresholdGB, configured) | ||
| return Int64(gb * 1024 * 1024 * 1024) | ||
| } | ||
|
|
||
| // MARK: Tick | ||
|
|
||
| private func tick() { | ||
| guard isEnabled else { | ||
| clearAll() | ||
| return | ||
| } | ||
| guard !isScanning, let paneProvider else { return } | ||
| let descriptors = paneProvider() | ||
| guard !descriptors.isEmpty else { | ||
| clearAll() | ||
| return | ||
| } | ||
| let thresholdBytes = thresholdBytes() | ||
| isScanning = true | ||
| queue.async { [weak self] in | ||
| let samples = Self.computeSamples(descriptors: descriptors) | ||
| Task { @MainActor in | ||
| self?.applySamples(samples, thresholdBytes: thresholdBytes) | ||
| } | ||
| } |
There was a problem hiding this comment.
tick() dispatches onto a private DispatchQueue and creates a nested Task { @MainActor in ... } to bounce results back. start() uses DispatchSource.makeTimerSource(queue:) bridged into Swift concurrency via Task { @MainActor in self?.tick() }. Per cmux-swift-concurrency-modernization, DispatchQueue for background work is the legacy pattern: computeSamples is already nonisolated static, so the canonical shape is Task.detached { let samples = Self.computeSamples(descriptors:); await MainActor.run { self.applySamples(...) } } initiated from a Swift Clock-based timer rather than a GCD source.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
…eforeReplay (#5911) The terminal-output collector tests waited for stream chunks with a fixed 200ms polling budget (200×1ms). On the slower iPad simulator leg the live "current" replay chunk lands after that window, so the assertion saw only the "old" snapshot and the test failed — a flake previously masked by the test-ios false-green override (#5906). Replace the time-budget poll with a deterministic continuation-based synchronization point on TerminalOutputCollector: waitForLines(_:) parks a CheckedContinuation and resumes the instant the stream delivers the target chunk count, with no per-platform timing budget to overrun. unmount() releases any parked waiter so a pending wait never hangs. Applied to all three sibling tests that shared the fragile poll pattern. Closes #5911 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses autoreview: the continuation-based wait had no deadline, so a genuinely dropped or miscounted output chunk would hang the Swift Testing run until the global CI timeout. Add a generous (10s) watchdog that resumes the parked continuation, letting the test fail on its assertion (showing the actual collected lines) instead of hanging. The success path stays fully event-driven and resumes the instant the chunk lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses autoreview: revert the two sibling tests (selfHealing two-replay and the render-grid mounted-sink) to their original bounded polls. The mounted-sink test relied on the bounded poll as a settle window to verify the stale replay frame is suppressed; switching it to waitForLines(1) asserted the instant the first chunk landed and weakened that negative coverage. Only renderGridTerminalInputWaitsForLiveEventBeforeReplay — the test the issue reported as flaking — now uses the deterministic waitForLines helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Closes #5911
Root cause
cmuxFeatureTests' terminal-output collector tests waited for stream chunks to be delivered using a fixed 200ms polling budget (for _ in 0..<200 where collector.lines.count < N { try await Task.sleep(nanoseconds: 1_000_000) }). TherenderGridTerminalInputWaitsForLiveEventBeforeReplay()test expects two chunks — the replayed"old"snapshot followed by the live"current"render-grid event. On the slower iPad simulator leg, the second (live) chunk's response flows back through the transport after the 200ms window closes, so the test proceeds with only["old"]collected and the equality assertion fails.This flake was previously masked by the test-ios false-green override (being removed in #5906); once that lands it would fail iOS PR runs at roughly the observed rate (3/13 iPad legs in the June 9–11 sample, 0/15 iPhone legs).
Fix
Replace the time-budget poll with a deterministic synchronization point on
TerminalOutputCollector:waitForLines(_ count:)parks aCheckedContinuationand is resumed the instant the stream delivers the required number of chunks (resumeSatisfiedWaiters()is called from the stream-consuming task right after each append). No per-platform timing budget for a slow simulator to overrun.unmount()releases any still-parked waiter so a pending wait can never hang.Tradeoff
Test-only change; no production code touched. The wait is unbounded by design (resumes immediately on delivery); a genuinely missing chunk would surface as the job-level test timeout rather than a fixed-window assertion mismatch — an honest signal instead of a flaky one.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Deflakes the iPad-simulator run of renderGridTerminalInputWaitsForLiveEventBeforeReplay by replacing the 200ms poll with a deterministic, continuation-based wait for stream chunks (fixes #5911). Adds waitForLines(...) with a 10s watchdog and ensures unmount() releases parked waiters; scoped to this test only, keeping sibling tests on their bounded polls to preserve coverage.
Written for commit 1da3eca. Summary will update on new commits.