modern concurrency#93
Open
KishanBagaria wants to merge 69 commits into
Open
Conversation
Contributor
|
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR migrates automation from a DispatchQueue-based idle queue to async/await: it adds a MessagesControllerAutomationLane actor, converts MessagesController and PlatformAPI to async/await with new automation boundaries, applies ChangesConcurrency Infrastructure and MessagesController Async Migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/IMessage/Sources/IMessage/Messages/MessagesController.swift (1)
1317-1349:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftManual activation/deactivation now wait behind unrelated automation.
These handlers are awaited from the AX event loop, but both now queue behind the same serial lane as long-running sends/edits/reactions. A busy automation run can therefore keep Messages hidden after the user manually activates it, then replay stale activate/deactivate transitions once the lane drains. The user-facing hide/unhide path needs to stay responsive; either keep the coordinator-facing part off-lane or coalesce pending lifecycle state before enqueueing.
🤖 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 `@src/IMessage/Sources/IMessage/Messages/MessagesController.swift` around lines 1317 - 1349, The activateMessages() and deactivateMessages() handlers are getting queued behind long-running work because they call windowCoordinator and window operations inside PlatformAPI.runOnMessagesControllerLane; fix by keeping the coordinator-facing/window operations off that serial lane or by coalescing state before enqueuing: move the user-facing state updates (lastActivate, messagesIsManuallyActivated, log.debug) inside PlatformAPI.runOnMessagesControllerLane but perform windowCoordinator.userManuallyActivated(app), userManuallyDeactivated(app), reset(window), closeAllNonMainWindows(), and resetWindow() outside the lane (e.g., dispatch them asynchronously on a different queue or call a PlatformAPI method that runs off the messages controller lane), or alternatively capture and check the latest messagesIsManuallyActivated value inside the runOnMessagesControllerLane closure before calling the coordinator to avoid replaying stale transitions; update activateMessages() and deactivateMessages() to use one of these approaches referencing activateMessages(), deactivateMessages(), PlatformAPI.runOnMessagesControllerLane, windowCoordinator.userManuallyActivated/userManuallyDeactivated, reset(window), closeAllNonMainWindows(), and resetWindow().src/IMessage/Sources/IMessage/PlatformAPI+MessagesController.swift (1)
18-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
@TaskLocalre-entry state leaks into spawned tasks.
TaskLocalvalues propagate into childTask {}s, so any delayed follow-up created inside a lane action inheritsisExecutingOnLane = trueand will hit this assertion even after the original lane work has finished.MessagesController.scheduleCancelReplyTranscriptView()now does exactly that, so normal automation will trap in debug; in release, true nested re-entry still hangs becauseassertdisappears. This guard needs a non-inherited/token-based check, or a separate non-inheriting path for legitimate follow-up tasks.Also applies to: 35-39, 63-65
🤖 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 `@src/IMessage/Sources/IMessage/PlatformAPI`+MessagesController.swift around lines 18 - 21, The TaskLocal flag isExecutingOnLane wrongly propagates into child Tasks and causes false positives in run and callers like MessagesController.scheduleCancelReplyTranscriptView; fix by replacing the propagation-prone guard with a non-inheriting mechanism or by ensuring follow-up tasks don’t inherit TaskLocal state. Concretely: either (A) replace `@TaskLocal` private static var isExecutingOnLane with a simple actor-based tracker/guard (e.g., LaneExecutionTracker actor with enter()/exit()/isExecuting() used by run) so spawned tasks don’t see a propagated boolean, or (B) keep the TaskLocal but change callers that spawn delayed follow-ups (notably MessagesController.scheduleCancelReplyTranscriptView) to create those follow-ups with Task.detached { ... } (or otherwise clear the TaskLocal) so they won’t inherit isExecutingOnLane; update the assertion/check in run to use the new actor guard or the non-inheriting path accordingly.
🤖 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.
Duplicate comments:
In `@src/IMessage/Sources/IMessage/Messages/MessagesController.swift`:
- Around line 1317-1349: The activateMessages() and deactivateMessages()
handlers are getting queued behind long-running work because they call
windowCoordinator and window operations inside
PlatformAPI.runOnMessagesControllerLane; fix by keeping the
coordinator-facing/window operations off that serial lane or by coalescing state
before enqueuing: move the user-facing state updates (lastActivate,
messagesIsManuallyActivated, log.debug) inside
PlatformAPI.runOnMessagesControllerLane but perform
windowCoordinator.userManuallyActivated(app), userManuallyDeactivated(app),
reset(window), closeAllNonMainWindows(), and resetWindow() outside the lane
(e.g., dispatch them asynchronously on a different queue or call a PlatformAPI
method that runs off the messages controller lane), or alternatively capture and
check the latest messagesIsManuallyActivated value inside the
runOnMessagesControllerLane closure before calling the coordinator to avoid
replaying stale transitions; update activateMessages() and deactivateMessages()
to use one of these approaches referencing activateMessages(),
deactivateMessages(), PlatformAPI.runOnMessagesControllerLane,
windowCoordinator.userManuallyActivated/userManuallyDeactivated, reset(window),
closeAllNonMainWindows(), and resetWindow().
In `@src/IMessage/Sources/IMessage/PlatformAPI`+MessagesController.swift:
- Around line 18-21: The TaskLocal flag isExecutingOnLane wrongly propagates
into child Tasks and causes false positives in run and callers like
MessagesController.scheduleCancelReplyTranscriptView; fix by replacing the
propagation-prone guard with a non-inheriting mechanism or by ensuring follow-up
tasks don’t inherit TaskLocal state. Concretely: either (A) replace `@TaskLocal`
private static var isExecutingOnLane with a simple actor-based tracker/guard
(e.g., LaneExecutionTracker actor with enter()/exit()/isExecuting() used by run)
so spawned tasks don’t see a propagated boolean, or (B) keep the TaskLocal but
change callers that spawn delayed follow-ups (notably
MessagesController.scheduleCancelReplyTranscriptView) to create those follow-ups
with Task.detached { ... } (or otherwise clear the TaskLocal) so they won’t
inherit isExecutingOnLane; update the assertion/check in run to use the new
actor guard or the non-inheriting path accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89a3f762-fe9a-44ab-ad85-ee9bfc32d990
📒 Files selected for processing (6)
src/IMessage/Sources/IMessage/Messages/MessagesController.swiftsrc/IMessage/Sources/IMessage/PlatformAPI+MessagesController.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/SpacesWindowCoordinator.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/WindowCoordinator.swiftsrc/IMessage/Sources/IMessageTests/MessagesControllerAutomationLaneTests.swift
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-03T17:00:19.662Z
Learnt from: KishanBagaria
Repo: beeper/platform-imessage PR: 69
File: src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift:89-93
Timestamp: 2026-05-03T17:00:19.662Z
Learning: In the beeper/platform-imessage Swift codebase, keep message IDs (`PlatformSDK.MessageID`) as raw GUIDs. When mapping from DB/event rows to `message.id`, set the ID directly from `msgRow.guid` (no GUID→public-ID hashing or transformation). For multi-part messages, append the part index as `_<part.index>` to the GUID-derived ID. During code review, if changes touch message ID creation/mapping, ensure this raw GUID + optional `_<part.index>` suffix behavior is preserved.
Applied to files:
src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/WindowCoordinator.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/SpacesWindowCoordinator.swiftsrc/IMessage/Sources/IMessageTests/MessagesControllerAutomationLaneTests.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swiftsrc/IMessage/Sources/IMessage/PlatformAPI+MessagesController.swiftsrc/IMessage/Sources/IMessage/Messages/MessagesController.swift
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMessageTests/MessagesControllerAutomationLaneTests.swift
[Warning] 25-25: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 51-51: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 62-62: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 72-72: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 82-82: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 100-100: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 116-116: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 142-142: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 154-154: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 20-20: Magic numbers should be replaced by named constants
(no_magic_numbers)
…n throws too, so a partial pin state can still be cleaned up. Generated with [Indent](https://indent.com) Co-Authored-By: KishanBagaria <KishanBagaria@users.noreply.github.com>
…st so the _mainWindowReally retry path doesn't iterate before LaunchServices has processed the open. Generated with [Indent](https://indent.com) Co-Authored-By: KishanBagaria <KishanBagaria@users.noreply.github.com>
…es app, refactor openDeepLink
Co-authored-by: indent[bot] <216979840+indent[bot]@users.noreply.github.com>
…to kb/modern-concurrency
This reverts commit d4d991f.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Messages.app automation stack by replacing the legacy DispatchQueue/locks/semaphores/sleeps approach with Swift structured concurrency (actors + async/await), introducing a single serial “automation lane”, improved cancellation propagation, and bounded retry/timeout primitives to avoid runaway loops and improve testability.
Changes:
- Introduces
MessagesControllerAutomationLane(actor) and routes all MessagesController automation through it (PlatformAPI.runOnMessagesControllerLane), including idle-callback behavior and cancellation. - Converts major MessagesController/WindowCoordinator/PlatformAPI automation APIs from sync
throwstoasync throws, adding targeted@MainActorhops for AppKit usage. - Adds new async utilities (
Task.withTimeout, asyncretry(withTimeout:), async Result initializer, KVO async stream helpers) and corresponding unit tests; removes the oldPassivelyAwareDispatchQueueand its tests.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| todos.md | Updates concurrency follow-ups (strict concurrency adoption, coordinator factory seam, withAutomation cleanup edge). |
| src/IMessage/Sources/IMessageTests/WaitForLaunchTests.swift | Adds stress/behavior tests for async NSRunningApplication.waitForLaunch. |
| src/IMessage/Sources/IMessageTests/RetryTests.swift | Adds coverage for new async retry overloads (timeout, cancellation, onError behavior). |
| src/IMessage/Sources/IMessageTests/PassivelyAwareDispatchQueueTests.swift | Removes tests for deleted legacy idle-aware dispatch queue. |
| src/IMessage/Sources/IMessageTests/MessagesControllerAutomationLaneTests.swift | Adds tests for the new serial automation lane actor (ordering, idle, cancellation, error isolation). |
| src/IMessage/Sources/IMessageTests/KeyPresserTests.swift | Adds tests for KeyPresser’s new injection points and thread dispatch behavior. |
| src/IMessage/Sources/IMessageTests/Eventually.swift | Adds an async polling helper used by new tests. |
| src/IMessage/Sources/IMessageTests/DatabaseTickWaitTests.swift | Removes a local eventually helper in favor of the new shared one. |
| src/IMessage/Sources/IMessageTests/AsyncUtilityTests.swift | Adds tests for Task.withTimeout, KVO wait helpers, and deep link timeout behavior. |
| src/IMessage/Sources/IMessageNode/PlatformAPINodeWrapper.swift | Updates Node method signature for onThreadSelected to accept nullable thread IDs. |
| src/IMessage/Sources/IMessageNode/IMessageNodeExports.swift | Ensures SystemSettingsOnboarding start/stop execute on MainActor. |
| src/IMessage/Sources/IMessageCore/Retry.swift | Adds async retry(withTimeout:) overload. |
| src/IMessage/Sources/IMessageCore/Result+Async.swift | Adds async Result(catching:) initializer. |
| src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift | Removes legacy idle-aware dispatch queue implementation. |
| src/IMessage/Sources/IMessageCLI/IMessageCLI.swift | Marks authorization prompting as @MainActor. |
| src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/WindowCoordinator.swift | Updates window coordinator protocol for async operations and main-actor boundaries. |
| src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/SpacesWindowCoordinator.swift | Updates coordinator to async methods and safe @MainActor app access. |
| src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift | Refactors coordinator for async + main-actor isolation of AppKit reads and sendable anchor data. |
| src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/Eclipsing/EclipsingDebugger.swift | Annotates NSScreen helper with @MainActor. |
| src/IMessage/Sources/IMessage/Utilities/Task+Timeout.swift | Adds a cancellation-aware timeout wrapper utility for async operations. |
| src/IMessage/Sources/IMessage/Utilities/KVO+AsyncStream.swift | Adds KVO async streams and async-sequence “first until deadline” helper. |
| src/IMessage/Sources/IMessage/SystemSettingsOnboarding.swift | Marks onboarding API as @MainActor. |
| src/IMessage/Sources/IMessage/PromptAutomation.swift | Converts notifications automation to async NSWorkspace open + async retry/sleep. |
| src/IMessage/Sources/IMessage/PlatformAPI+MessagesController.swift | Replaces queue-based serialization with lane actor; bounds invalidation loops; improves disposal behavior. |
| src/IMessage/Sources/IMessage/PlatformAPI.swift | Converts controller calls to async/await; refactors thread-activity selection observation state. |
| src/IMessage/Sources/IMessage/Pasteboard+Backup.swift | Makes pasteboard restoration helper async-compatible. |
| src/IMessage/Sources/IMessage/OnboardingManager.swift | Moves onboarding window/timer handling onto @MainActor and reduces cross-thread hopping. |
| src/IMessage/Sources/IMessage/MessagesControllerAutomationLane.swift | Adds the new serial automation lane actor implementation with idle callbacks. |
| src/IMessage/Sources/IMessage/Messages/MessagesInstanceTarget.swift | Converts secondary Messages launch to async with timeout and async waitForLaunch. |
| src/IMessage/Sources/IMessage/Messages/MessagesDeepLink.swift | Reworks deep link model from enum to struct with target-thread metadata. |
| src/IMessage/Sources/IMessage/Messages/MessagesController+PlatformOperations.swift | Converts platform operation wrappers to async. |
| src/IMessage/Sources/IMessage/Messages/MessagesController.swift | Major async conversion; introduces withAutomation scoping; serializes user-activation paths through the lane. |
| src/IMessage/Sources/IMessage/Messages/MessagesAppElements.swift | Converts element lookup paths to async and updates deep-link open callback type. |
| src/IMessage/Sources/IMessage/MacPermissions.swift | Forces automation permission prompt onto MainActor. |
| src/IMessage/Sources/IMessage/KeyPresser.swift | Refactors KeyPresser for testability via injection; changes default thread behavior. |
| src/IMessage/Sources/IMessage/IMessageHost.swift | Updates notifications-disable flow to await async PromptAutomation. |
| src/IMessage/Sources/IMessage/Extensions.swift | Replaces sync waitForLaunch with async + timeout and KVO-based waiting; adds @MainActor rect helper. |
| src/IMessage/Sources/IMessage/Defaults.swift | Updates documentation comment for watchThreadActivity. |
| src/IMessage/Sources/IMDatabaseTestBench/TestBench.swift | Removes legacy idle-aware queue testbench command. |
| src/IMessage/lib/index.ts | Updates TypeScript type for onThreadSelected to allow null. |
| src/api.ts | Adjusts runtime behavior to allow null thread selection to flow through (no early-return). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+73
to
+85
| res = .failure(error) | ||
| do { | ||
| try await onError?(attempt, error) | ||
| attempt += 1 | ||
| } catch let error as CancellationError { | ||
| throw error | ||
| } catch { | ||
| Log.errors.error("retry onError errored \(error)") | ||
| } | ||
| } | ||
| if let interval { | ||
| try await Task.sleep(nanoseconds: UInt64(interval * 1_000_000_000)) | ||
| } |
Comment on lines
+26
to
+28
| } catch is CancellationError { | ||
| return nil | ||
| } |
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
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.
What
Moves the entire Messages.app automation path off the legacy
DispatchQueue+UnfairLock+ semaphore/Thread.sleepmodel and onto Swift structured concurrency (actors +async/await), with real cancellation, bounded retries, and unit-testable seams. Net: +1,786 / −1,201 across 34 files.The old model serialized automation by hopping onto a
PassivelyAwareDispatchQueueand blocking; sleeps and waits pinned threads, and there was no way to cancel an in-flight operation. The new model funnels every operation through one serial async lane, propagates cancellation cooperatively, and bounds the retry/invalidation loops so a stuck controller can't spin forever.Key changes
MessagesControllerAutomationLane(new actor) replacesPassivelyAwareDispatchQueue. A single serial async lane that allMessagesControllerautomation funnels through (PlatformAPI.runOnMessagesControllerLane), so only one operation touches Messages.app at a time. It handles:idleDelayand keep firing while the lane stays quiet; new active work cancels a stale idle cycle.@TaskLocalflag turns re-entry into a loudpreconditioncrash rather than a silent deadlock. (Documented why an "inline fallback" is unsafe: the task-local leaks into spawnedTask {}children.)PlatformAPI/MessagesControllerCoordinator— controller actions are nowasync throws;onMessagesControllerQueue→runOnMessagesControllerLane. The coordinator's invalidation loop is now bounded (max 30) withTask.checkCancellation()checks, and disposal runs inside a cancellation-immune unstructuredTaskso cancelling a caller mid-construction can no longer leak a half-launched Messages.app.MessagesController— nearly all automation methods converted from syncthrowstoasync throws(send, react, edit, undo-send, mute, delete, typing, mark-read, deep-link open, etc.).prepareForAutomation/finishedAutomationreplaced by a scopedwithAutomation { … }/automationDidComplete. Dead paths removed (markAsReadWithMenu,revealReplyTranscriptViaMenu).Window coordinators —
makeAutomatable/resetare nowasync throws, with explicit@MainActorannotations andMainActor.runhops where AppKit work happens. The coordinator protocol is intentionally not@MainActor(implementations do blocking Accessibility I/O that would freeze the UI on the main actor; they hop to@MainActoronly for the AppKit bits) — documented inline.New async primitives:
NSWorkspace+AsyncOpen— bridgesNSWorkspace.open(…completionHandler:)toasyncwith a single-completion-point continuation, timeout, and cancellation handling. Used for deep-link opens.retry(withTimeout:)— cancellation-aware async retry (re-throwsCancellationError, swallows/logsonErrorfailures).Result(catching:)async initializer.KeyPresser— refactored for testability with injectable key-post and keymap closures (defaults preserve real behavior). Key presses now default toonMainThread: falsesince the lane is no longer the main thread.Extensions/IMessageHost—waitForLaunchand related waits converted fromThread.sleeptoTask.sleep.Tests
MessagesControllerAutomationLaneTests(232 lines — ordering, idle callbacks, re-entrancy, cancellation),NSWorkspaceAsyncOpenTests,RetryTests,KeyPresserTests, and anEventuallyasync test helper.PassivelyAwareDispatchQueueTests(along with its subject).Follow-ups (tracked in
todos.md)-strict-concurrency=targetedand clear the ~26 concurrency warnings with realSendable/isolation annotations (not blanket@unchecked). Surfaced by plan-eng-review; deferred so it can land as a focused pass with its own QA.MessagesControllerCoordinatorso the dedup/invalidate/dispose logic is unit-testable without Accessibility + Messages.app.withAutomationcleanup fire whenmakeAutomatablepartially succeeds (pre-existing edge: a throw after unhide/resize can leave Messages.app visible with stalewindowFramePreEclipse).