Repository navigation
Swift 6.3 concurrency warnings: RemoteSession captured-self + a test Sendable flag (slice 1) - #6623
Closed
azooz2003-bit wants to merge 2 commits into
Closed
azooz2003-bit wants to merge 2 commits into
azooz2003-bit wants to merge 2 commits into
Conversation
…ion + a test flag) First slice of the Swift 6.3 concurrency-warning cleanup surfaced by the Xcode 26.x CI bump (#6603). These are warnings, not build errors; this clears the highest-confidence, behavior-preserving subset. CmuxRemoteSession (production, Swift 6 language mode) — "reference to captured var 'self' in concurrently-executing code [#SendableClosureCaptures]" in three Foundation callbacks (Process.terminationHandler, the stderr readabilityHandler, and the proxy-broker update callback). Each double-dereferenced an optional `[weak self]` across the outer callback and the nested `queue.async` block. Bind `self` once with `guard let self` before touching `queue`; the `[weak self]` release semantics and the serial-`queue` synchronization contract are unchanged. cmuxTests AppDelegateRenameShortcutContextTests — "capture of '…' with non-Sendable type 'ShortcutNotificationFlag' in a '@sendable' closure" (8 sites). Make the flag `@unchecked Sendable` with an `NSLock`-guarded Bool (the repo's existing test-box idiom), so it can be flipped from the `@Sendable` NotificationCenter observer and read on the main actor. The public API (`wasPosted` get, `markPosted()`) is unchanged, so no call sites move. Remaining warnings (AppDelegate UI-test recorders, more cmuxTests sites, the Sparkle SUAppcastItem deprecation, and the bonsplit submodule onChange/bounds) are catalogued in the PR description for follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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)
📝 WalkthroughWalkthroughThree coordinator closures ( ChangesConcurrency Safety Fixes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR addresses 11 Swift 6.3 concurrency warnings by binding
Confidence Score: 5/5All three changes are mechanical, behavior-preserving refactors with no logic added or removed; the serial-queue synchronization contract and weak-reference semantics in CmuxRemoteSession are intact. The production CmuxRemoteSession changes only hoist an existing guard let self from inside a queue.async block to just before it — the .endOfFile handler-clearing path remains unchanged, and the serial queue still guards all locked state. The test change adds NSLock protection to a flag type that was previously not thread-safe; the public API is identical. No new control flow, no new mutable state, no new concurrency paths. No files require special attention; both production files touch only the capture binding in existing callbacks. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant OS as OS/Foundation
participant Closure as Callback Closure
participant Self as RemoteSessionCoordinator
participant Queue as serial queue
note over Closure: [weak self] capture
OS->>Closure: fire (terminationHandler / readabilityHandler / broker update)
Closure->>Closure: guard let self (bind once — NEW)
alt self was deallocated
Closure-->>OS: return (no-op)
else self is alive
Closure->>Queue: "queue.async { self.handle...Locked(...) }"
Queue->>Self: handle...Locked (self strongly retained for block lifetime)
end
note over Closure: .endOfFile path (readabilityHandler only)
OS->>Closure: fire with .endOfFile
Closure->>Closure: "handle.readabilityHandler = nil (no self needed)"
%%{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 OS as OS/Foundation
participant Closure as Callback Closure
participant Self as RemoteSessionCoordinator
participant Queue as serial queue
note over Closure: [weak self] capture
OS->>Closure: fire (terminationHandler / readabilityHandler / broker update)
Closure->>Closure: guard let self (bind once — NEW)
alt self was deallocated
Closure-->>OS: return (no-op)
else self is alive
Closure->>Queue: "queue.async { self.handle...Locked(...) }"
Queue->>Self: handle...Locked (self strongly retained for block lifetime)
end
note over Closure: .endOfFile path (readabilityHandler only)
OS->>Closure: fire with .endOfFile
Closure->>Closure: "handle.readabilityHandler = nil (no self needed)"
Reviews (2): Last reviewed commit: "Keep RemoteSessionCoordinator under its ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+45
to
55
| var wasPosted: Bool { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return _wasPosted | ||
| } | ||
|
|
||
| func markPosted() { | ||
| wasPosted = true | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| _wasPosted = true | ||
| } |
Contributor
There was a problem hiding this comment.
lock()/defer { unlock() } pair in both accessors is correct but can be replaced with NSLock.withLock (available since Swift 5.8 / macOS 13), which is shorter and eliminates the risk of forgetting the defer on future edits.
Suggested change
| var wasPosted: Bool { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return _wasPosted | |
| } | |
| func markPosted() { | |
| wasPosted = true | |
| lock.lock() | |
| defer { lock.unlock() } | |
| _wasPosted = true | |
| } | |
| var wasPosted: Bool { | |
| lock.withLock { _wasPosted } | |
| } | |
| func markPosted() { | |
| lock.withLock { _wasPosted = true } | |
| } |
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!
The captured-self fix added one line, tipping the file to 656 over its 655 budget. Collapse the proxy-broker queue.async body to a single line so the fix lands without growing the file (now 654).
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.
Follow-up to #6603 (which moved the macOS CI gates to Xcode 26.x / Swift 6.3). The 6.3 compiler surfaces concurrency/Sendable warnings the 6.1 gate did not. None are build errors (the main app + tests are
SWIFT_VERSION = 5.0with no strict-concurrency; RemoteSession is Swift 6 mode but@unchecked Sendable/closure-capture diagnostics there are warnings). This is the first, highest-confidence, behavior-preserving slice.CmuxRemoteSessionruntime code.Fixed here (11 warnings)
CmuxRemoteSession(3, production) —reference to captured var 'self' in concurrently-executing code [#SendableClosureCaptures]in three Foundation callbacks (Process.terminationHandler, the stderrreadabilityHandler, the proxy-broker update callback). Each dereferenced an optional[weak self]twice (outer callback + nestedqueue.async). Fix: bindselfonce withguard let selfbefore touchingqueue.[weak self]release semantics and the serial-queuesynchronization contract are unchanged; the.endOfFilehandler-clearing path is preserved exactly (bind only inside.data).cmuxTests/AppDelegateRenameShortcutContextTests(8) —capture of '…' with non-Sendable type 'ShortcutNotificationFlag' in a '@Sendable' closure. Make the flag@unchecked Sendablewith anNSLock-guarded Bool (the repo's existing test-box idiom). Public API (wasPosted,markPosted()) unchanged, so no call sites move; one type change clears all 8.Remaining (catalogued for follow-up)
A full survey classified ~48 deferred warnings: 0 are RISKY hot-path (none in
hitTest/forceRefresh/TabItemView), but several need per-site judgment or carry submodule/vendor ceremony, so they're intentionally not in this slice:Sources/AppDelegate.swift(~20) — main-actor-isolation / non-Sendable-capture warnings, all inCMUX_UI_TEST_*-gated diagnostic recorders (recordFocusedState,attemptResolve,attemptFocus,writeUITestDiagnosticsIfNeeded,feedSidebarUITestPush…, etc.) plus one productionscheduleLaunchServicesBundleRegistration@convention(block)site. Uniform fix isMainActor.assumeIsolated { … }(observers registerqueue: .main) orTask { @MainActor in … }. Deferred because AppDelegate is typing/focus-sensitive and warrants its own reviewed+dogfooded PR.cmuxTestssites (~13) — hoistlet id = x.id.uuidStringbeforeDispatchQueue.global(TerminalControllerSocketSecurityTests), convert capture-only helpers tostatic nonisolated(TerminalNotification{Caller,Queue}Tests), anNSLockcounter box (BrowserConfigTests), andassumeIsolatedfor mid-flight-assertion observers (TabManagerSessionSnapshotTests, WorkspacePullRequestSidebarTests, WorkspaceUnitTests). Mechanical but per-site; held back so this PR's test-target delta stays trivially reviewable (the worktree can't resolve the test deps locally, so each is CI-verified).SUAppcastItem(dictionary:)deprecation (1,CmuxUpdater/TestSupport) — no public non-deprecated initializer in Sparkle 2.8.1 (the designated init needs a privateSPUAppcastItemStateResolver). Needs a decision: suppress at the call site or refactor the fake to drop the Sparkle-private dependency.vendor/bonsplitsubmodule (4) —onChange(of:perform:)deprecation ×3 (trivial: drop_ in, use the zero-arg macOS-14 form) + one nonisolatedboundsread (mirror the in-filenonisolated(unsafe)cache sibling). Needs a submodule branch/push + parent-pointer bump, so it's its own PR.Verification
Push runs
swift-package-tests(covers the RemoteSession change) and the app-host/tests-build-and-lagjobs (cover the test-target change) on Xcode 26.x. Expect green with the 11 warnings gone.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Clears 11 Swift 6.3 concurrency/Sendable warnings in
CmuxRemoteSessionand tests by bindingselfonce before dispatch and making a test flagSendable. No behavior change; keeps Swift 6.3 builds quiet.Bug Fixes
CmuxRemoteSession: InProcess.terminationHandler, stderrreadabilityHandler, and the proxy-broker update callback, bindselfwithguard let selfbeforequeue.async; preserves[weak self]semantics and.endOfFilebehavior.ShortcutNotificationFlaginAppDelegateRenameShortcutContextTestsis now@unchecked Sendablevia anNSLock-guarded Bool; API unchanged and clears 8 Sendable-capture warnings.Refactors
RemoteSessionCoordinator: Collapsed the proxy-brokerqueue.asyncbody to one line to stay within the Swift file-length budget; no behavior change.Written for commit f544c12. Summary will update on new commits.
Summary by CodeRabbit