fix imessage idle queue scheduling crash#83
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughPassivelyAwareDispatchQueue's idle-state management was refactored to consolidate separate tracking variables into a single protected ChangesIdle-state refactoring and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates PassivelyAwareDispatchQueue’s idle detection mechanism to avoid crashes caused by retaining/cancelling DispatchWorkItems during passive scheduling, and adds Swift tests to validate the new idle/epoch behavior under various timing and concurrency scenarios.
Changes:
- Replace
DispatchWorkItem-based passive scheduling with an epoch/pending-state approach that suppresses stale idle callbacks. - Adjust idle scheduling logic to re-arm passive checks based on queue quiescence and epoch stability.
- Add a new test suite covering idle transitions, stale idle suppression, continuing idle callbacks, and rapid concurrent submissions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift |
Reworks idle scheduling to avoid DispatchWorkItem retention/cancellation and uses epoch/pending validation to ignore stale idle events. |
src/IMessage/Sources/IMessageTests/PassivelyAwareDispatchQueueTests.swift |
Adds tests validating idle callback behavior, stale suppression, repeated idle callbacks, and concurrency stress cases. |
Comments suppressed due to low confidence (1)
src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift:81
- The PR description says idle checks are coalesced through a single
DispatchSourceTimer, but the implementation here uses repeatedqueue.asyncAfterscheduling and recursive re-arming. Either update the implementation to match the described timer-based approach, or adjust the PR description so it accurately reflects the current behavior.
func armPassive(expectingEpoch expectedEpoch: UInt, quiescence: Quiescence) {
// Submission-side epoch changes logically cancel delayed idle checks
// without retaining and releasing DispatchWorkItems across threads.
queue.asyncAfter(deadline: .now() + idleDelay) { [weak self] in
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func armPassive(expectingEpoch expectedEpoch: UInt, quiescence: Quiescence) { | ||
| // Submission-side epoch changes logically cancel delayed idle checks | ||
| // without retaining and releasing DispatchWorkItems across threads. | ||
| queue.asyncAfter(deadline: .now() + idleDelay) { [weak self] in | ||
| guard let self else { return } |
| Thread.sleep(forTimeInterval: 0.1) | ||
|
|
||
| queue.async { | ||
| secondWorkFinished.signal() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift (1)
78-81: 🏗️ Heavy liftThis still queues one delayed block per idle transition.
armPassivenow avoidsDispatchWorkItemcancellation, but it still allocates a freshasyncAfterclosure every timependingdrops to zero. On a bursty drain/refill pattern, those stale closures still pile up and later contend with real work on this serial queue even though the epoch check makes them harmless. If the goal here is true coalescing, this needs a single reusable timer/source rather than per-arm delayed closures.🤖 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/IMessageCore/PassivelyAwareDispatchQueue.swift` around lines 78 - 81, The current armPassive(expectingEpoch:quiescence:) still schedules a new queue.asyncAfter closure on every transition to zero, causing many stale delayed blocks to accumulate; replace this per-arm delayed closure with a single reusable DispatchSourceTimer (e.g. an idleTimer property created once in the DispatchQueue wrapper or initializer) and have armPassive only arm/reset that timer and disarm/suspend it when work arrives. Locate armPassive and the places where pending is decremented/checked, stop creating asyncAfter closures there, instead schedule the single idleTimer with the idleDelay, keep the expectedEpoch check in the timer handler, and ensure the timer is coalescing by rescheduling (setTime/resume) rather than recreating the DispatchSourceTimer each time.
🤖 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 `@src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift`:
- Around line 97-105: The idle rearm logic currently always calls
armPassive(expectingEpoch:expectedEpoch, quiescence:.continuing) even when the
optional uponIdle callback is nil; change the condition so you only re-arm if an
idle callback is set (check uponIdle.read() != nil or equivalent) after
computing shouldContinue from activityState.withLock (so you still respect
pending and epoch), and ensure callers that clear the callback via
setIdleCallback(nil) stop further re-scheduling.
---
Nitpick comments:
In `@src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift`:
- Around line 78-81: The current armPassive(expectingEpoch:quiescence:) still
schedules a new queue.asyncAfter closure on every transition to zero, causing
many stale delayed blocks to accumulate; replace this per-arm delayed closure
with a single reusable DispatchSourceTimer (e.g. an idleTimer property created
once in the DispatchQueue wrapper or initializer) and have armPassive only
arm/reset that timer and disarm/suspend it when work arrives. Locate armPassive
and the places where pending is decremented/checked, stop creating asyncAfter
closures there, instead schedule the single idleTimer with the idleDelay, keep
the expectedEpoch check in the timer handler, and ensure the timer is coalescing
by rescheduling (setTime/resume) rather than recreating the DispatchSourceTimer
each time.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffd39688-8745-49d4-af60-7523aee56629
📒 Files selected for processing (2)
src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swiftsrc/IMessage/Sources/IMessageTests/PassivelyAwareDispatchQueueTests.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/IMessageTests/PassivelyAwareDispatchQueueTests.swiftsrc/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMessageTests/PassivelyAwareDispatchQueueTests.swift
[Warning] 6-6: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 26-26: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 55-55: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 76-76: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift
[Warning] 54-54: Prefer not to use extension access modifiers
(no_extension_access_modifier)
| uponIdle.read()?(quiescence) | ||
|
|
||
| if pending.read() == 0 { | ||
| let shouldContinue = activityState.withLock { state in | ||
| state.pending == 0 && state.epoch == expectedEpoch | ||
| } | ||
| if shouldContinue { | ||
| // If no active work was scheduled while we were busy with | ||
| // passive work, schedule the passive work to run again soon. | ||
| armPassive(expectingEpoch: expectedEpoch, quiescence: .continuing) |
There was a problem hiding this comment.
Stop rearming when the idle callback is nil.
uponIdle is optional, but the .continuing path ignores that and re-schedules forever as long as pending == 0 and the epoch stays unchanged. That means a queue with no idle callback registered — or one whose callback clears itself with setIdleCallback(nil) — will keep waking up indefinitely with nothing to run.
♻️ Proposed fix
uponIdle.read()?(quiescence)
- let shouldContinue = activityState.withLock { state in
- state.pending == 0 && state.epoch == expectedEpoch
- }
+ let shouldContinue = uponIdle.read() != nil && activityState.withLock { state in
+ state.pending == 0 && state.epoch == expectedEpoch
+ }
if shouldContinue {
// If no active work was scheduled while we were busy with
// passive work, schedule the passive work to run again soon.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uponIdle.read()?(quiescence) | |
| if pending.read() == 0 { | |
| let shouldContinue = activityState.withLock { state in | |
| state.pending == 0 && state.epoch == expectedEpoch | |
| } | |
| if shouldContinue { | |
| // If no active work was scheduled while we were busy with | |
| // passive work, schedule the passive work to run again soon. | |
| armPassive(expectingEpoch: expectedEpoch, quiescence: .continuing) | |
| uponIdle.read()?(quiescence) | |
| let shouldContinue = uponIdle.read() != nil && activityState.withLock { state in | |
| state.pending == 0 && state.epoch == expectedEpoch | |
| } | |
| if shouldContinue { | |
| // If no active work was scheduled while we were busy with | |
| // passive work, schedule the passive work to run again soon. | |
| armPassive(expectingEpoch: expectedEpoch, quiescence: .continuing) |
🤖 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/IMessageCore/PassivelyAwareDispatchQueue.swift` around
lines 97 - 105, The idle rearm logic currently always calls
armPassive(expectingEpoch:expectedEpoch, quiescence:.continuing) even when the
optional uponIdle callback is nil; change the condition so you only re-arm if an
idle callback is set (check uponIdle.read() != nil or equivalent) after
computing shouldContinue from activityState.withLock (so you still respect
pending and epoch), and ensure callers that clear the callback via
setIdleCallback(nil) stop further re-scheduling.
Summary
Selecting an iMessage chat no longer risks the idle queue crash caused by retaining and cancelling
DispatchWorkIteminstances around passive work scheduling. The queue now coalesces idle checks through a singleDispatchSourceTimerand validates pending work plus epoch state before callbacks run, so stale idle events are harmless and active work can continue to reschedule cleanly.The new tests cover the active-to-idle transition, stale idle suppression, continuing idle callbacks, outdated timer events, and rapid concurrent submissions.
Testing
swift testswift test --filter PassivelyAwareDispatchQueueswift test --sanitize=thread --filter PassivelyAwareDispatchQueueyarn typecheckswiftlint lint --config .swiftlint.yml src/IMessage/Sources/IMessageCore/PassivelyAwareDispatchQueue.swift src/IMessage/Sources/IMessageTests/PassivelyAwareDispatchQueueTests.swiftgit diff --check