modern concurrency#93
Conversation
|
|
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[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}
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>
| func idleCallback(observingThreadID threadID: String, statusSender: @escaping (ThreadActivityObservation) -> Void) throws -> ((Quiescence) throws -> Void) { | ||
| /// Returns an observer that checks one thread while controller work is idle. | ||
| /// The platform-level idle observer calls it repeatedly after active automation drains. | ||
| func makeIdleActivityObserver(observingThreadID threadID: String, statusSender: @escaping (ThreadActivityObservation) -> Void) throws -> (() async throws -> Void) { |
There was a problem hiding this comment.
Code review finding (P2) — idle observer can send stale activity after disposal
Flagged independently by the correctness and adversarial reviewers during a /ce-code-review pass on this PR.
What's wrong: The idle activity observer (makeIdleActivityObserver) lost the old activityLock.tryLock() skip-if-busy behavior and re-checks isValid only at lane entry — not after each await. After a disposal or an automation burst, the observer can still run its multi-await body and send stale activity (reporting on a controller that's since been torn down, or state that changed mid-flight). The deleted serial queue's tryLock skip used to prevent this.
Suggested fix: Re-check guard isValid else { return } after the withAutomation block (before observing/sending activity), and ideally at the top of the withAutomation closure — reinstating "bail if no longer valid" semantics in a form suited to the actor model.
Location: src/IMessage/Sources/IMessage/Messages/MessagesController.swift (around the makeIdleActivityObserver body, ~L1559)
Deferred from interactive code review (finding #10). Severity P2, confidence 75, reviewers: correctness + adversarial.
1 participant
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).