Modernize concurrency toward Swift 6 (audit + 15-file landable fixes)#4977
Modernize concurrency toward Swift 6 (audit + 15-file landable fixes)#4977azooz2003-bit wants to merge 5 commits into
Conversation
Surfaced by an exhaustive concurrency audit of the app (Swift 5 mode), CLI, and packages, recorded in REPORT.md (334 findings catalogued). This commit fixes the 41 high-confidence, low/medium-ripple findings across 15 self-contained files that survived two adversarial review passes: - Fix an unguarded data race in NotificationHookProcessRun (didComplete / continuation accessed from multiple dispatch callbacks) so the hook process run resolves exactly once. - Replace NSLock + bare-var state with OSAllocatedUnfairLock<State> and tighten Sendable annotations (FileExplorerStore, SessionIndexStore, TextBoxInput, TerminalImageTransfer, FilePreviewPanel). - Replace empty/timing DispatchQueue hops and wall-clock asyncAfter retries with structured main-actor work driven by real signals, not sleeps. - Tighten main-actor isolation on UI-bound state and dispatch paths. Deferred (catalogued in REPORT.md, not changed here): the large god-files, high-ripple async API conversions, and 8 candidate patches whose adversarial review found compile breaks, policy violations (Task.sleep), or observable behavior regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR modernizes concurrency across many Swift files: replacing NSLock with OSAllocatedUnfairLock, applying ChangesConcurrency Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (14 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 |
Greptile SummaryThis PR replaces
Confidence Score: 4/5Safe to merge for the lock-modernisation and race-fix changes; the TextBoxInput timeout path swaps one timing primitive for another that should be addressed before this lands. The lock migrations and continuation race fix are correct and well-scoped. The one outlier is TextBoxInput.swift: the DispatchSourceTimer is replaced with Task.sleep, which is a sleep-based timeout — the same pattern already called out on UpdateDriver and CmuxConfig in this PR's own deferral notes. That path is in active UI submission flow and the fix should use the same event-driven approach recommended for the deferred sites. Sources/TextBoxInput.swift — the new Task.sleep-based timeout should be revisited before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (MainActor)
participant NHP as NotificationHookProcessRun
participant Lock as OSAllocatedUnfairLock<CompletionState>
participant Cont as CheckedContinuation
Caller->>NHP: run() — installs continuation
NHP->>Lock: "withLock { $0.continuation = cont }"
par Timeout fires
NHP->>Lock: "withLock { didComplete? }"
Lock-->>NHP: false
NHP->>Lock: "withLock { didComplete=true, take cont }"
Lock-->>NHP: continuation
NHP->>Cont: resume(.failure(.timeout))
and Process exits
NHP->>Lock: "withLock { didComplete? }"
Lock-->>NHP: true (already set)
NHP-->>NHP: return (no-op)
end
Reviews (5): Last reviewed commit: "TerminalImageTransfer: use uncheckedStat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pendingCheckTransition = Task { @MainActor [weak self] in | ||
| try? await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) | ||
| guard !Task.isCancelled, let self else { return } | ||
| guard case .checking = self.viewModel.state else { return } | ||
| self.lastCheckStart = nil | ||
| self.applyState(newState) | ||
| } |
There was a problem hiding this comment.
Task.sleep violates cmux-swift-blocking-runtime on the same PR that explicitly deferred it
The PR description notes that UpdateController.swift was skipped specifically because it uses Task.sleep for timing coordination, calling that a policy violation to defer to a separate PR. UpdateDriver now introduces two new Task.sleep calls — setStateAfterMinimumCheckDelay (line 207) and scheduleCheckTimeout (line 230) — replacing the previous asyncAfter sites with the same class of flagged primitive. asyncAfter and Task.sleep are both listed under cmux-swift-blocking-runtime; swapping one for the other keeps the violation present. If the preferred resolution is a continuation-based DispatchSourceTimer (fire once, no sleep), both sites should use it; otherwise the REPORT.md should document these two alongside the deferred sites rather than treating them as fixed.
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!
| startLocalFileWatcher() | ||
| } | ||
|
|
||
| private func scheduleLocalReattach(attempt: Int) { | ||
| guard attempt <= Self.maxReattachAttempts else { return } | ||
| watchQueue.asyncAfter(deadline: .now() + Self.reattachDelay) { [weak self] in | ||
| guard let self else { return } | ||
| DispatchQueue.main.async { | ||
| guard let path = self.localConfigPath else { return } | ||
| if FileManager.default.fileExists(atPath: path) { | ||
| self.loadAll() | ||
| self.startLocalFileWatcher() | ||
| } else { | ||
| self.startLocalDirectoryWatcher() | ||
| } | ||
| private func scheduleLocalReattach() { | ||
| // A deleted config has no inode to attach a DispatchSource to, so this is a | ||
| // genuine poll for the file's reappearance, not a timing hack. A superseding | ||
| // event or deinit cancels the pending retry via the stored handle. | ||
| localReattachTask?.cancel() | ||
| localReattachTask = Task { @MainActor [weak self] in | ||
| try? await Task.sleep(for: .seconds(Self.reattachDelay)) | ||
| if Task.isCancelled { return } | ||
| guard let self, let path = self.localConfigPath else { return } | ||
| if FileManager.default.fileExists(atPath: path) { | ||
| self.loadAll() | ||
| self.startLocalFileWatcher() | ||
| } else { | ||
| self.startLocalDirectoryWatcher() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Task.sleep in retry loops — cmux-swift-blocking-runtime still flagged even for genuine polls
Both scheduleLocalReattach() and scheduleGlobalReattach() now use Task { @MainActor ... try? await Task.sleep(for: .seconds(...)) } for inter-attempt delays. The inline comment correctly identifies this as "genuine poll for file reappearance, not a timing hack" — but the cmux-swift-blocking-runtime rule explicitly lists Task.sleep and polling alongside asyncAfter as patterns to flag regardless of intent. The old implementation used watchQueue.asyncAfter (also flagged), so this is a lateral change that improves structure but does not resolve the policy concern. A DispatchSourceFileSystemObject watching the parent directory for kqueue/vnode events would eliminate the sleep entirely; if that isn't feasible here, these two sites should be listed in REPORT.md with the other deferred Task.sleep findings.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/FilePreviewPanel.swift (1)
2539-2549:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCancellation only stops the wrapper task; the detached decode keeps running.
In
Sources/Panels/FilePreviewPanel.swift(2539-2549),documentLoadTask?.cancel()cancels the outerTask { ... }, but the decode happens inside an untrackedTask.detached { PDFDocument(url: loadURL) }, so rapid switches can still accumulate stale PDF decodes until completion (even though the result is later discarded by theTask.isCancelled/guard checks). Make the decode itself the cancellable/coalesced unit (avoidTask.detached, or store the detached task handle and cancel that instead). Same pattern is referenced forimageLoadTask(3752-3762).🤖 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/Panels/FilePreviewPanel.swift` around lines 2539 - 2549, documentLoadTask currently cancels only the outer Task while the PDF decode runs inside an untracked Task.detached, allowing stale decodes to continue; change this so the decode is the cancellable unit by removing the detached task and performing PDFDocument(url: loadURL) inside the stored documentLoadTask (or create and store a separate Task handle for the decode and cancel it when starting a new load), then await its value and call applyLoadedPDFDocument(for:) on the MainActor only if not cancelled; apply the same fix pattern to imageLoadTask to ensure the actual decode/parse tasks are canceled when replaced.
🤖 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 `@Sources/App/CmuxCLIPathInstaller.swift`:
- Around line 267-272: The comment above the sequential drain in
CmuxCLIPathInstaller.swift overstates safety; update it to state that reading
stdout then stderr (using ProcessPipeReader.readDataToEndOfFileOrEmpty on
stdout.fileHandleForReading and stderr.fileHandleForReading) only protects the
first-drained pipe and therefore only prevents blocking under the assumption
that child processes produce bounded (small) output on both pipes; mention that
the pattern is reused in FileExplorerStore.SSHCommandProcess.run where arbitrary
output can overflow a pipe and deadlock, and clarify this location is safe
because the privileged osascript/mkdir/rm/ln commands produce tiny output.
In `@Sources/TerminalImageTransfer.swift`:
- Around line 90-95: TerminalImageTransferOperation's current UncheckedHandler
unleashes a non-@Sendable closure across executors when cancel() invokes it;
change the design so the stored handler is `@Sendable` or is invoked on the
originating executor. Concretely, update UncheckedHandler to store run:
`@Sendable` () -> Void and update any initializer/signature on
TerminalImageTransferOperation that accepts the cancellation handler to require
an `@Sendable` () -> Void, or alternatively ensure cancel() dispatches the stored
handler back to the correct executor/actor (for example by invoking it via the
originating actor or Task.runDetached with appropriate actor hop) instead of
calling a non-@Sendable closure from another concurrency domain. Ensure all call
sites supply an `@Sendable` handler or the invocation is performed on the
handler’s home executor to restore soundness.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 352-361: The current defer spawns an unstructured Task to call
soundPreparationCoordinator.finish(expandedPath) which can run after the outer
Task exits and allows a race where pendingPaths remains set; instead, inside the
Task that calls beginIfNeeded(expandedPath) and after the
withCheckedContinuation resumes, call await
soundPreparationCoordinator.finish(expandedPath) inline (i.e., remove the defer
{ Task { await ... } } pattern) so finish runs synchronously before the outer
Task exits; keep the customSoundPreparationQueue.async block that calls
prepareCustomFileForNotifications(path: expandedPath) and resume the
continuation there, then immediately await
soundPreparationCoordinator.finish(expandedPath).
---
Outside diff comments:
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 2539-2549: documentLoadTask currently cancels only the outer Task
while the PDF decode runs inside an untracked Task.detached, allowing stale
decodes to continue; change this so the decode is the cancellable unit by
removing the detached task and performing PDFDocument(url: loadURL) inside the
stored documentLoadTask (or create and store a separate Task handle for the
decode and cancel it when starting a new load), then await its value and call
applyLoadedPDFDocument(for:) on the MainActor only if not cancelled; apply the
same fix pattern to imageLoadTask to ensure the actual decode/parse tasks are
canceled when replaced.
🪄 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: 9a7df866-e6ac-45f0-b809-5ab1771861a9
📒 Files selected for processing (16)
REPORT.mdSources/App/CmuxCLIPathInstaller.swiftSources/App/ShortcutRoutingSupport.swiftSources/Auth/AuthManager.swiftSources/BackgroundWorkspacePrimeCoordinator.swiftSources/CmuxConfig.swiftSources/FileExplorerStore.swiftSources/Panels/FilePreviewPanel.swiftSources/SessionIndexStore.swiftSources/SocketControlSettings.swiftSources/TerminalImageTransfer.swiftSources/TerminalNotificationPolicy.swiftSources/TerminalNotificationStore.swiftSources/TextBoxInput.swiftSources/Update/UpdateDriver.swiftSources/WindowDragHandleView.swift
| /// Wraps a non-`Sendable` cancellation handler so it can live inside the | ||
| /// lock-protected ``Protected`` state. The closure is only ever read or | ||
| /// written while the lock is held, which is what makes the unchecked | ||
| /// conformance safe; callers therefore do not need `@Sendable` handlers. | ||
| private struct UncheckedHandler: @unchecked Sendable { | ||
| let run: () -> Void |
There was a problem hiding this comment.
Fix Sendable/cancellation handler safety
TerminalImageTransferOperation’s new Sendable conformance is unsound if cancel() can execute a non-@Sendable captured closure from a different executor. The lock only protects the storage/transfer of the closure; it doesn’t make the closure’s captured state safe to be invoked after crossing concurrency domains.
🤖 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/TerminalImageTransfer.swift` around lines 90 - 95,
TerminalImageTransferOperation's current UncheckedHandler unleashes a
non-@Sendable closure across executors when cancel() invokes it; change the
design so the stored handler is `@Sendable` or is invoked on the originating
executor. Concretely, update UncheckedHandler to store run: `@Sendable` () -> Void
and update any initializer/signature on TerminalImageTransferOperation that
accepts the cancellation handler to require an `@Sendable` () -> Void, or
alternatively ensure cancel() dispatches the stored handler back to the correct
executor/actor (for example by invoking it via the originating actor or
Task.runDetached with appropriate actor hop) instead of calling a non-@Sendable
closure from another concurrency domain. Ensure all call sites supply an
`@Sendable` handler or the invocation is performed on the handler’s home executor
to restore soundness.
The build compiled, but three new Sendable warnings tripped the tests-build-and-lag warning budget: - CmuxConfig: iterating NotificationCenter.notifications transferred the non-Sendable Notification into the @mainactor observer task. Map the sequence to Void so only the change signal crosses isolation. - FilePreviewPanel: the PDF/image Task.detached{...}.value rewrite returned non-Sendable PDFDocument/NSImage to the main actor. Revert just those two load paths to the original background-queue + main-hop form (no behavior change); keep the valuable FilePreviewDragRegistry OSAllocatedUnfairLock refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- FilePreviewPanel: discard the unused removeValue result inside withLock so the call no longer warns "result of call to 'withLock' is unused". - WindowDragHandleView: revert the breadcrumb-limiter NSLock->@mainactor change. Marking the emit helper @mainactor surfaced isolation warnings on its default arguments (NSApp.currentEvent, evaluated in a nonisolated default-arg context). The finding is marginal (a debug-only rate limiter); reverting is cleaner than rippling main-actor isolation through the default args. Moved to deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| process.standardOutput = stdout | ||
| process.standardError = stderr | ||
| let outputGroup = DispatchGroup() | ||
| startDraining(stdout, into: stdoutBuffer, group: outputGroup) | ||
| startDraining(stderr, into: stderrBuffer, group: outputGroup) | ||
| defer { | ||
| stdout.fileHandleForReading.readabilityHandler = nil | ||
| stderr.fileHandleForReading.readabilityHandler = nil | ||
| } | ||
| try process.run() | ||
| // Drain both pipes synchronously to EOF before waiting on exit so a child | ||
| // that fills a pipe buffer (>64KB) cannot block, then wait for termination. | ||
| // Avoids blocking the calling thread on async readabilityHandler callbacks. | ||
| let stdoutData = ProcessPipeReader.readDataToEndOfFileOrEmpty(from: stdout.fileHandleForReading) |
There was a problem hiding this comment.
Reading stdout to EOF then stderr to EOF is safe only when the child writes < 64 KB to each pipe independently. If the child fills its stderr pipe buffer (≥ 64 KB) while the main thread is blocked inside readDataToEndOfFileOrEmpty(stdout), the child stalls writing to stderr, never closes stdout, and both sides hang indefinitely. The previous concurrent readabilityHandler approach on both pipes explicitly avoided this class of deadlock.
A safe synchronous alternative is to drain both descriptors on two concurrent threads (or use Thread.detachNewThread for each) and join them before calling waitUntilExit(). The existing startDraining(_:into:group:) + outputGroup.wait() pattern was already correct for this; the only actual fix needed there was replacing NSLock-backed PrivilegedCommandOutputBuffer with OSAllocatedUnfairLock.
- TerminalImageTransfer (CodeRabbit Critical): keep @unchecked Sendable instead of plain Sendable + an unchecked closure wrapper that falsely implied the non-Sendable cancellation handler was safe to invoke across executors. Keep the OSAllocatedUnfairLock modernization. - TerminalNotificationStore (CodeRabbit Major): await finish() inline after the continuation instead of a fire-and-forget defer Task, so a follow-up request for the same path is not dropped during the race window. - Revert UpdateDriver and CmuxConfig reattach loops: both swapped asyncAfter for Task.sleep, which cmux-swift-blocking-runtime flags identically (lateral, not a fix). Deferred with the already-deferred UpdateController. - Revert CmuxCLIPathInstaller: sequential stdout-then-stderr drain can deadlock on a child that fills the stderr pipe buffer; restore concurrent draining. - Revert WindowDragHandleView: @mainactor on the breadcrumb limiter rippled isolation warnings onto NSApp.currentEvent default args. - REPORT.md: document the post-review deferrals and the final 11-file applied set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@Sources/TerminalImageTransfer.swift`:
- Line 101: The Protected state stores a non-Sendable closure but the code
creates state with OSAllocatedUnfairLock(initialState:) and calls
state.withLock, which requires Sendable; replace uses with the unchecked APIs:
construct the lock the same way but change all state.withLock { ... } calls to
state.withLockUnchecked { ... } (update usages in isCancelled,
clearCancellationHandler, finish and any other occurrences) so the non-@Sendable
Protected (and its handler closure) can be accessed safely without Sendable
constraints; ensure you reference the OSAllocatedUnfairLock instance named state
and the Protected type when making these replacements.
🪄 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: 027b7c2f-d3fc-47f6-98fe-6b87b0dfa86f
📒 Files selected for processing (3)
REPORT.mdSources/TerminalImageTransfer.swiftSources/TerminalNotificationStore.swift
…endable state CodeRabbit: OSAllocatedUnfairLock.init(initialState:)/withLock are constrained to Sendable State, but Protected holds a non-Sendable cancellation closure. Swift 5 mode only warned, but the precise, Swift-6-ready API for non-Sendable protected state is init(uncheckedState:) + withLockUnchecked. The @unchecked Sendable class still manually guarantees safety (all access under the lock). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Exhaustive concurrency audit of the cmux app (Swift 5 language mode), CLI, and SPM packages for patterns with a strictly better Swift 6 / modern-concurrency solution, plus the landable subset of fixes.
REPORT.mdcatalogs all 334 findings (10 critical, 83 high, 241 medium) by file, category, severity, and ripple risk.Notable fixes:
NotificationHookProcessRun(didComplete/continuationtouched from multiple dispatch callbacks) so the hook process run resolves exactly once.NSLock+ bare-varstate replaced withOSAllocatedUnfairLock<State>inFileExplorerStore,SessionIndexStore,TextBoxInput,TerminalImageTransfer,FilePreviewPanel.DispatchQueuehops and wall-clockasyncAfterretries replaced with structured main-actor work driven by real signals (no sleeps), respecting the repo no-sleep-timing policy.Deferred (catalogued in
REPORT.md, not changed here)TerminalController,Workspace,TabManager,ContentView,BrowserPanel,GhosttyTerminalView,cmuxApp,AppDelegate,CLI/cmux.swift) and high-ripple async-API conversions (e.g. CLI websocketreceiveSync/waitForSocketwould make every caller async).allCookies()), deletion of a shared type still in use (AgentForkSupportProcessTerminationGate), missing companion test edits + continuation-under-lock (TerminalDirectoryOpenSupport), rendering-stagger regression (BrowserWindowPortal), aTask.sleeppolicy violation (UpdateController), and behavior/ordering changes in latency-sensitive AppKit / socket-wire paths (SurfaceSearchOverlay, FeedCoordinator).Testing
ci.ymlbuild +cmux-unit) for build/test verification.Task
Plain-text task: "identify every concurrency issue that has a better solution in Swift 6 (swift-guidance); fix, adversarially review, build, PR." Run as a two-phase multi-agent workflow (48-agent scan, then 23 fix + 46 adversarial-review agents).
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Modernizes concurrency toward Swift 6 with a full audit in REPORT.md (334 findings). Tightens main-actor isolation, replaces ad‑hoc locks, fixes the notification hook race, hardens auth session anchoring, and stabilizes custom‑sound staging.
Bug Fixes
ASWebAuthenticationSessionuses a main‑cached presentation anchor for off‑mainstart().NotificationCenter.notificationsto Void inCmuxConfig; revert PDF/image previewTask.detachedpaths; revert the debug window‑drag breadcrumb limiter change.Refactors
NSLockwithOSAllocatedUnfairLockin SSH explorer state/transport, ripgrep cancellation, metadata cache, image transfer op, file‑preview drag registry, and error bag.MainActor.assumeIsolatedin priming.@unchecked Sendableand now usesOSAllocatedUnfairLock.uncheckedState/withLockUncheckedfor its non‑Sendable handler; minordeferfix inSocketControlSettings.Written for commit 6d7564c. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit