fix: prevent crash when monitoring Live Activities#46
Draft
tarikfp wants to merge 2 commits intocallstackincubator:mainfrom
Draft
fix: prevent crash when monitoring Live Activities#46tarikfp wants to merge 2 commits intocallstackincubator:mainfrom
tarikfp wants to merge 2 commits intocallstackincubator:mainfrom
Conversation
|
@tarikfp is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
6621483 to
8407adb
Compare
Add safety checks to prevent crashes in monitorActivity: 1. Skip activities that are already in .ended state before accessing their properties (prevents accessing deallocated objects) 2. Use serial dispatch queue for thread-safe access to monitoredActivityIds Set (prevents concurrent modification crashes when multiple async tasks call monitorActivity simultaneously) The crashes occurred when: - Accessing activity.id on activities that had already ended - Multiple async tasks from activityUpdates stream tried to modify the monitoredActivityIds Set concurrently
8407adb to
1aef2a6
Compare
DispatchQueue.sync doesn't work correctly with Swift Concurrency. OSAllocatedUnfairLock provides proper synchronous locking that works with the cooperative thread pool.
Contributor
|
Could you write a harness test that causes this to happen during a test run and then fix it with your changes? This will confirm your assumptions in a case where starting and stopping happen one millisecond after the other. |
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.
fix: prevent crash when monitoring Live Activities
Summary
Fixes two crashes in
monitorActivitycaused by thread-unsafe Set access from multiple Swift Concurrency Tasks.Problem
We're seeing intermittent crashes when Live Activities are started/stopped quickly. Found two distinct crash types:
Crash 1: Set.contains() blows up
Crash 2: Set.insert() blows up with PAC failure
What's happening
The
for awaitloops inobserveLiveActivityUpdates()spawn Tasks that callmonitorActivity()concurrently. Multiple Tasks hit themonitoredActivityIdsSet at the same time → race condition → crash.Why not DispatchQueue.sync?
Mixing GCD with Swift Concurrency doesn't work. GCD's memory barriers don't apply to Swift's cooperative thread pool. The Tasks can still see stale data.
Fix: OSAllocatedUnfairLock
Using
OSAllocatedUnfairLock(iOS 16+) for the Set access. It's synchronous, lower overhead than actors, and should work correctly with Swift Concurrency Tasks.Changes
Also captures
activityNameandpushEnabledbefore the async boundary to avoid accessing potentially stale activity references.Test Plan