fix: stop SessionManager racing (and copying) recentSessions on the main thread - #297
Conversation
…n thread persistCurrentSessionIfNeeded() mutated `recentSessions` on the caller's thread - the once-per-second updateSessionDuration timer plus the app lifecycle handlers - and then handed the same array to persistenceQueue to be JSON-encoded. The queued encode read the array while the next tick was already mutating it (the ThreadSanitizer race in TelemetryDeck#236), and because the pending encode kept the buffer alive the array was never uniquely referenced, so every tick also triggered a full copy-on-write copy on the main thread. Move the read-modify-write onto persistenceQueue and guard the array with a lock that is released before the encode and the UserDefaults write, so readers can never block behind that I/O. Since the queue is serial, the snapshot taken for encoding is released before the next tick runs, so that tick mutates the array in place and the per-tick copy disappears rather than moving to the background. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note on CI: 5 of the 6 legs are green (macOS, iOS, watchOS, tvOS, Mac Catalyst). The visionOS leg is red, but the failures are in pre-existing suites unrelated to this change: They look like timing flakes on the slow visionOS simulator — I don't have rerun rights on this repo, so I can't confirm by re-running that job — happy to rebase or push an empty commit if you'd like a fresh run. |
kkostov
left a comment
There was a problem hiding this comment.
Thanks a lot @Aurther-Nadeem this looks okay!
Addresses #236.
Problem
persistCurrentSessionIfNeeded()mutatesrecentSessionson the caller's thread — the once-per-secondupdateSessionDurationtimer, plus thedidEnterBackground/willEnterForegroundhandlers — and then hands that same array topersistenceQueueto be JSON-encoded:That has two consequences:
Data race. The queued encode reads
recentSessionswhile the next tick is already mutating it. This is the ThreadSanitizer report in Thread sanitizer reports a warning in SessionManager #236, reproduced by several people on macOS since 2.9.x and still present onmain.A copy-on-write copy on the main thread, once per second, for the life of the app. Because the pending encode still references the buffer, the array is not uniquely referenced, so each tick's mutation has to copy the whole array before it can mutate it. The field reports in Thread sanitizer reports a warning in SessionManager #236 land exactly there —
Array.subscript.modify→Array._makeMutableAndUnique→_ArrayBuffer._consumeAndCreateNew— both as crashes and as app hangs.There is currently no way for a consumer to opt out:
sessionStatsEnabledonly gatesstartNewSession(), while thewillBecomeActiveobserver installed ininit()schedules the timer regardless, and every signal instantiates the singleton viaDefaultSignalPayload.parameters.Fix
Move the read-modify-write onto
persistenceQueueand guard the array with a lock:recentSessionsis now backed byunsafeRecentSessions, only touched while holdingsessionsLock.persistCurrentSessionIfNeeded()captures the two scalars it needs and does the whole update inside the existingpersistenceQueue.async, so the caller's thread does no array work at all.UserDefaultswrite, so a reader —averageSessionSecondsand friends, read on every signal fromDefaultSignalPayload.parameters, which is@MainActor— can never end up blocked behind that I/O. This deliberately avoids reintroducing the shape of App hang / deadlock in SignalCache.count() due to sync barrier on concurrent queue #265.Behaviour change
The session update is applied one queue hop later instead of synchronously. The exposed stats are whole-second session durations refreshed once per second, so the lag is not observable in the reported values. Durability is unchanged — the
UserDefaultswrite was already asynchronous.Tests
Adds
SessionManagerConcurrencyTests, which drives ticks the way the run-loop timer does while the stats are read concurrently, and asserts the bookkeeping stays correct (one session updated per tick, not one appended per tick).To keep it isolated it avoids
SessionManager.sharedandstartNewSession()— the latter emits an internal signal that requires a globally initializedTelemetryManager. That needed twoprivate→ internal relaxations (init,updateSessionDuration), both commented at the declaration. Counts are asserted as deltas, because whetherTelemetryDeck.customDefaultsresolves to a real suite depends on whether another suite has initialized the SDK in the same process.Two caveats I'd rather state than have you discover:
Sanitizer load violates platform policy, macOS 26), and even a trivial suite crashes at bootstrap under it. The TSan reports in Thread sanitizer reports a warning in SessionManager #236 remain the authoritative signal that the race is real; I'd appreciate confirmation from someone who can run TSan against this branch.Verified: full suite green (68 tests), and the package builds for macOS, iOS and watchOS. tvOS/visionOS platforms aren't installed on my machine, but the change only uses
NSLockandDispatchQueue.Related
#272 fixes a different defect in this file — session duration timers stacking up because
handleWillEnterForegroundNotificationreplacessessionDurationUpdaterwithout invalidating the previous one. This PR does not touch the timer lifecycle, so the two are complementary rather than conflicting.