Skip to content

fix: stop SessionManager racing (and copying) recentSessions on the main thread - #297

Merged
kkostov merged 1 commit into
TelemetryDeck:mainfrom
Aurther-Nadeem:fix/session-manager-data-race
Aug 1, 2026
Merged

fix: stop SessionManager racing (and copying) recentSessions on the main thread#297
kkostov merged 1 commit into
TelemetryDeck:mainfrom
Aurther-Nadeem:fix/session-manager-data-race

Conversation

@Aurther-Nadeem

@Aurther-Nadeem Aurther-Nadeem commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Addresses #236.

Problem

persistCurrentSessionIfNeeded() mutates recentSessions on the caller's thread — the once-per-second updateSessionDuration timer, plus the didEnterBackground / willEnterForeground handlers — and then hands that same array to persistenceQueue to be JSON-encoded:

self.recentSessions[existingSessionIndex].durationInSeconds = Int(self.currentSessionDuration)

self.persistenceQueue.async {
    if let updatedSessionData = try? Self.encoder.encode(self.recentSessions) {  }
}

That has two consequences:

  1. Data race. The queued encode reads recentSessions while 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 on main.

  2. 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.modifyArray._makeMutableAndUnique_ArrayBuffer._consumeAndCreateNew — both as crashes and as app hangs.

There is currently no way for a consumer to opt out: sessionStatsEnabled only gates startNewSession(), while the willBecomeActive observer installed in init() schedules the timer regardless, and every signal instantiates the singleton via DefaultSignalPayload.parameters.

Fix

Move the read-modify-write onto persistenceQueue and guard the array with a lock:

  • recentSessions is now backed by unsafeRecentSessions, only touched while holding sessionsLock.
  • persistCurrentSessionIfNeeded() captures the two scalars it needs and does the whole update inside the existing persistenceQueue.async, so the caller's thread does no array work at all.
  • The lock is released before the JSON encode and the UserDefaults write, so a reader — averageSessionSeconds and friends, read on every signal from DefaultSignalPayload.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.
  • Because the queue is serial, the snapshot taken for encoding is released before the next tick runs, so that tick mutates the array in place. The per-tick copy disappears rather than moving to the background.

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 UserDefaults write 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.shared and startNewSession() — the latter emits an internal signal that requires a globally initialized TelemetryManager. That needed two private → internal relaxations (init, updateSessionDuration), both commented at the declaration. Counts are asserted as deltas, because whether TelemetryDeck.customDefaults resolves 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:

  • These tests pass before and after the fix. I verified that explicitly by reverting the fix and re-running. They are regression guards and a deterministic repro vehicle for anyone running ThreadSanitizer — not a standalone failing test for the race.
  • I could not verify under ThreadSanitizer locally: the sanitizer runtime refuses to load on my machine (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 NSLock and DispatchQueue.

Related

#272 fixes a different defect in this file — session duration timers stacking up because handleWillEnterForegroundNotification replaces sessionDurationUpdater without invalidating the previous one. This PR does not touch the timer lifecycle, so the two are complementary rather than conflicting.

…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>
@Aurther-Nadeem

Copy link
Copy Markdown
Contributor Author

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:

SignalManagerBackoffTests.consecutiveFailures_resetsOnSuccess()
SignalManagerDispositionTests.serverError500_requeuesBatch_andIncrementsFailures()
SignalManagerDispositionTests.clientError408_requeuesBatch_andIncrementsFailures()

They look like timing flakes on the slow visionOS simulator — consecutiveFailures_incrementsOnErrorResponse took 13.9s there (0.17s locally), and the one that failed took 52.5s. The same suites pass on the other five legs of this run, the visionOS leg has gone red on main before, and the new SessionManagerConcurrencyTests are not among the failures.

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 kkostov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @Aurther-Nadeem this looks okay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants