fix: SDK-4508 offload SessionService.onFocus / onUnfocused off the main thread

[abdulraqeeb33]

Summary

ANR-dump analysis of logs/2026-05-12 (500 ANR entries, all on sdk_background_threading) attributes 25 of 500 ANRs (5.0%) to SessionService.onFocus blocking the main thread. Stack signature:

java.util.concurrent.LinkedBlockingQueue.offer
java.util.concurrent.ThreadPoolExecutor.execute
kotlinx.coroutines.scheduling.CoroutineScheduler.dispatch
com.onesignal.common.threading.OneSignalDispatchers.<lazy chain>
... operation-repo / IAM trigger eval handlers ...
com.onesignal.session.internal.session.impl.b$c.invoke   (sessionLifeCycleNotifier.fire)
com.onesignal.session.internal.session.impl.b.onFocus    (SessionService.onFocus)
com.onesignal.core.internal.application.impl.a.handleFocus  (fanout on main)

SessionService.onFocus runs synchronously on the main thread via ApplicationService.handleFocus -> applicationLifecycleNotifier.fire. Inside, sessionLifeCycleNotifier.fire { onSessionStarted / onSessionActive } invokes the registered session-lifecycle handlers (operation repo, IAM trigger eval, etc.) synchronously; the first one to touch OneSignalDispatchers pays the cold-init cost on the main thread (5–20s blocks before the watchdog fires).

Same SDK-4505 / SDK-4506 / SDK-4507 fanout pattern — but on a pre-existing handler that predates background threading.

Linear: SDK-4508

Fix

Wrap onFocus / onUnfocused bodies in runOnSerialIOIfBackgroundThreading (the same gated helper SDK-4505, SDK-4506, and SDK-4507 use). FF off retains the original inline semantics for the rollout control cohort and existing synchronous tests; FF on routes through the dedicated serial IO dispatcher so the fanout no longer runs on main.

Timing semantics preserved

The body of onFocus writes session.startTime = _time.currentTimeMillis and session.focusTime = _time.currentTimeMillis to record when Android delivered the lifecycle event. If we simply deferred everything to the serial dispatcher, those timestamps would shift to whenever the dispatcher got around to the block — which can be hundreds of ms later under load.

To keep the existing semantics:

• onFocus captures _time.currentTimeMillis on the caller's thread BEFORE the wrapper, passes it into the deferred handleOnFocus(firedOnSubscribe, focusTimeMs) block.
• onUnfocused does the same so activeDuration accounting stays accurate (dt = unfocusTimeMs - session.focusTime, both captured on the main thread).

Base branch

Stacked on ar/sdk-4506-notifications-onfocus-anr (#2644) because the fix relies on runOnSerialIOIfBackgroundThreading, which was introduced in that PR. Rebases cleanly onto main after #2643 and #2644 land.

Test plan

• SessionServiceTests existing assertions about state mutation continue to pass (the FF defaults to off, so the wrapper runs inline; the new private handleOnFocus / handleOnUnfocused preserve behavior).
• Adds three new regression tests:
  • onFocus dispatches the session-mutation body through runOnSerialIOIfBackgroundThreading.
  • onUnfocused dispatches the activeDuration update through runOnSerialIOIfBackgroundThreading.
  • Rapid onUnfocused -> onFocus burst routes both events through the same gated helper in submission order (mirrors SDK-4505 BackgroundManager burst coverage; protects against activeDuration drift if the events ever raced across an unbounded IO pool).
• :core detekt — no new findings.

Made with Cursor

[github-actions]

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

• OneSignalDispatchers.kt: 21/23 touched executable lines (91.3%) (64 touched lines in diff)
• SyncJobService.kt: 1/1 touched executable lines (100.0%) (10 touched lines in diff)
• OneSignalImp.kt: 0/2 touched executable lines (0.0%) (14 touched lines in diff)
  • 2 uncovered touched lines in this file
• SessionService.kt: 13/13 touched executable lines (100.0%) (40 touched lines in diff)

Overall (aggregate gate)

35/39 touched executable lines covered (89.7% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

• OneSignalImp.kt: 0.0% (2 uncovered touched lines)

📥 View workflow run

[claude]

🔴 After offloading sessionLifeCycleNotifier.fire { onSessionStarted/onSessionActive } to serial IO, InfluenceManager.onSessionStarted (InfluenceManager.kt:57) and onSessionActive (line 61) re-read _applicationService.entryState at execution time rather than capture time. If the main thread mutates entryState (via handleFocus/handleLostFocus) between dispatch and serial-IO execution, notification influence attribution can be misclassified (e.g., a NOTIFICATION_CLICK session reset to UNATTRIBUTED under the APP_OPEN branch of getChannelsToResetByEntryAction). Fix: capture entryState alongside focusTimeMs on the caller thread and pass it through to handleOnFocus/handleOnUnfocused (or thread it via an explicit parameter on ISessionLifecycleHandler).

Extended reasoning...

What the bug is

The PR carefully captures focusTimeMs = _time.currentTimeMillis on the caller's thread before offloading the body of SessionService.onFocus/onUnfocused to runOnSerialIOIfBackgroundThreading, so session.startTime / session.focusTime reflect the moment Android delivered the lifecycle event. But _applicationService.entryState, which is just as time-sensitive, is not captured. It is read at execution time by subscribers inside the deferred fire:

• InfluenceManager.onSessionStarted() → restartSessionTrackersIfNeeded(_applicationService.entryState) (InfluenceManager.kt:56-58)
• InfluenceManager.onSessionActive() → attemptSessionUpgrade(_applicationService.entryState) (InfluenceManager.kt:60-62)

How it manifests — step-by-step proof

1. T0 (main): User clicks notification. NotificationLifecycleService.notificationOpened runs on main:
   • Sets _applicationService.entryState = AppEntryAction.NOTIFICATION_CLICK (NotificationLifecycleService.kt:177)
   • Calls _influenceManager.onDirectInfluenceFromNotification(notificationId) → attemptSessionUpgrade(NOTIFICATION_CLICK, notificationId) → notification tracker becomes DIRECT.
2. T0+ε (main): Android fires Activity lifecycle. ApplicationService.handleFocus runs on main, the if (entryState != NOTIFICATION_CLICK) guard at line 391 preserves entryState = NOTIFICATION_CLICK, then applicationLifecycleNotifier.fire { onFocus(false) }.
3. T0+ε' (main): SessionService.onFocus(false) captures focusTimeMs and submits handleOnFocus(false, focusTimeMs) to the serial IO executor.
4. T0+δ (main): Before serial IO runs the submitted task, the user briefly backgrounds the activity. ApplicationService.handleLostFocus on main sets entryState = APP_CLOSE (line 375). User refocuses; handleFocus sees entryState == APP_CLOSE != NOTIFICATION_CLICK and overwrites it with APP_OPEN (line 392).
5. T0+Δ (serial IO): Step 3's submitted block executes. session.isValid == false (cold start), so the !session.isValid branch fires sessionLifeCycleNotifier.fire { it.onSessionStarted() }.
6. InfluenceManager.onSessionStarted reads _applicationService.entryState → now APP_OPEN, not the NOTIFICATION_CLICK that triggered step 3.
7. restartSessionTrackersIfNeeded(APP_OPEN) → getChannelsToResetByEntryAction(APP_OPEN) takes a different branch than NOTIFICATION_CLICK would (InfluenceManager.kt:71-84):
   • APP_OPEN: adds notificationChannelTracker (because entryAction.isAppOpen) and iAMChannelTracker → both get reset.
   • NOTIFICATION_CLICK: isAppOpen is false, so only iAMChannelTracker is added — the notification tracker is deliberately left alone to preserve direct attribution.
8. Because the wrong branch ran, the notification tracker's DIRECT influence (set in step 1) is overwritten via setSessionTracker (DIRECT → INDIRECT or UNATTRIBUTED, depending on lastReceivedIds). willChangeSessionTracker returns true because the new influenceType differs from the existing DIRECT, so the change is committed and cacheState() persists the misclassification.

Why pre-PR didn't have this race

Pre-PR, sessionLifeCycleNotifier.fire ran synchronously on the main thread inside SessionService.onFocus. The block executed on the same thread that just set entryState (handleFocus runs the lifecycle fanout synchronously after setting entryState), and because it occupied the main thread, no subsequent main-thread Activity lifecycle callbacks could fire and mutate entryState during execution. The very main-thread block that this PR is fixing (the 5-20s cold-init stall) had the side effect of protecting these reads.

Why the refutation's mitigations don't hold

The refutation argues that direct attribution is set out-of-band via onDirectInfluenceFromNotification, so even if the deferred onSessionStarted reads the wrong entryState, attribution survives. This is wrong: restartSessionTrackersIfNeeded(APP_OPEN) explicitly resets the notification tracker (lines 76-78 add it because entryAction.isAppOpen), and willChangeSessionTracker will return true when transitioning DIRECT → INDIRECT/UNATTRIBUTED, so the DIRECT attribution set by the direct call path is actively overwritten. NOTIFICATION_CLICK protects the tracker by leaving notificationChannel = null (it is not in the reset list at all).

The refutation also calls the race "microseconds-narrow," but that contradicts the PR's own motivation: the entire reason for offloading is that the cold-init dispatcher chain takes 5-20s on the main thread. Even after prewarm() is added, the race window is at minimum the serial-IO queue depth plus dispatcher overhead; under load this can be hundreds of ms, more than enough for a user-paced focus/unfocus sequence (e.g. notification-shade dismiss, biometric prompt return, etc.) to mutate entryState.

How to fix

Capture entryState on the caller thread alongside focusTimeMs and thread it through to the deferred handler:

override fun onFocus(firedOnSubscribe: Boolean) {
    val focusTimeMs = _time.currentTimeMillis
    val entryStateSnapshot = _applicationService.entryState
    runOnSerialIOIfBackgroundThreading {
        handleOnFocus(firedOnSubscribe, focusTimeMs, entryStateSnapshot)
    }
}

Then either (a) pass entryStateSnapshot as a parameter on ISessionLifecycleHandler.onSessionStarted/onSessionActive (cleaner; explicit), or (b) snapshot it into a thread-local-equivalent state SessionService owns and have InfluenceManager read it via a getter parameter. Option (a) parallels how focusTimeMs is already threaded through and makes the timing invariant explicit at the contract level.

[claude]

🔴 With SDK_BACKGROUND_THREADING on, SessionService.onFocus now defers handleOnFocus to the serial IO thread while StartupService continues iterating IStartableService.start() on the Default thread — and several of those start() methods (SessionListener, InfluenceManager, SubscriptionManager, InAppMessagesManager, UserRefreshService) call sessionService.subscribe(this). This introduces a cross-thread race where a subscriber added before fire snapshots subscribers can still observe shouldFireOnSubscribe = true afterwards, causing onSessionStarted() to fire twice — duplicate TrackSessionStartOperation enqueues, duplicate UserRefreshService.refreshUser() network calls, duplicate influence/IAM trigger evaluations. Pre-PR this path was deterministic because both handleOnFocus and the subsequent subscribe calls ran on the same startables thread.

Extended reasoning...

Threading topology (post-PR, FF on).

StartupService.scheduleStart() iterates IStartableService.start() sequentially on the Default dispatcher (StartupService.kt:28-37). SessionModule.kt:39-44 registers SessionService before SessionListener, so the loop reaches SessionService.start() first, which calls _applicationService.addApplicationLifecycleHandler(this). ApplicationService.addApplicationLifecycleHandler synchronously invokes handler.onFocus(true) when current != null (ApplicationService.kt:121-128).

Pre-PR, onFocus(true) ran handleOnFocus inline on the Default thread, so the entire body — shouldFireOnSubscribe = true, then sessionLifeCycleNotifier.fire { onSessionStarted() } — completed before SessionService.start() returned. The next startable's subscribe() always observed shouldFireOnSubscribe = true AFTER the fire (with no subscribers yet), giving exactly one onSessionStarted() per handler.

Post-PR, onFocus captures focusTimeMs then does runOnSerialIOIfBackgroundThreading { handleOnFocus(...) } and returns. The Default thread continues to SessionListener.start() → _sessionService.subscribe(this) (and so on for SubscriptionManager, InfluenceManager, UserRefreshService, InAppMessagesManager).

The double-fire interleaving.

SessionService.subscribe(handler):
A1. sessionLifeCycleNotifier.subscribe(handler) → synchronized(subscribers) { add(handler) }
A2. if (shouldFireOnSubscribe) handler.onSessionStarted()

handleOnFocus (in the !session.isValid branch, which is the cold-start path because endSession() invalidates the prior session):
B1. shouldFireOnSubscribe = firedOnSubscribe (true)
B2. sessionLifeCycleNotifier.fire { ... } → synchronized(subscribers) { subscribers.toList() } then for (s in localList) callback(s)

Walk through interleaving A1 → B1 → B2 → A2:

1. Default thread acquires subscribers monitor, adds the handler, releases. A1's monitor exit synchronizes-with any subsequent acquire of the same monitor.
2. SerialIO thread sets shouldFireOnSubscribe = true.
3. SerialIO thread acquires subscribers monitor (after A1's release, so A1 HB B2). Snapshot includes the handler. Releases monitor. Iterates → handler.onSessionStarted() fires (FIRE gradle? #1).
4. Default thread reads shouldFireOnSubscribe. With B1 HB B2 (program order) and A1 HB B2 (monitor sync), the read at A2 happens after A1. On real Android ART with cache-coherent ARM cores and the intervening monitor ops, the read commonly observes the new true value → handler.onSessionStarted() fires again (FIRE Added possibility to send status 'opened' for a message back to onesignal backend from client source code #2).

Observable impact. SessionListener.onSessionStarted() (SessionListener.kt:45-55) enqueues a TrackSessionStartOperation; double-fire produces duplicate session-start operations against the user, observable as inflated session counts in analytics. UserRefreshService.onSessionStarted() = refreshUser() (a network call) double-fires. InfluenceManager and InAppMessagesManager re-run session-start side effects. The window is small (the time between the SerialIO worker dequeuing the task and the Default thread's next instruction after subscribe() returns), but real and gated on the exact FF this PR targets.

Why the refutation's "narrow theoretical window" argument doesn't hold. The refutation argues that on cold start the SerialIO dispatcher is slow to spin up, so by the time handleOnFocus runs, all subscribe() calls have completed (no double fire). But the same PR series adds OneSignalDispatchers.prewarm() (OneSignalDispatchers.kt:186-238), called from initWithContext before bootstrapServices() and scheduleStart(). prewarm submits empty coroutines to all three dispatchers — including SerialIO — so the SerialIO worker thread is alive and idle by the time scheduleStart() reaches SessionService.start(). The "cold dispatcher absorbs the race window" assumption is invalidated by prewarm.

Re: missed-fire (Race #2). Acknowledging the refutation's and the third verifier's correct point: the alternative interleaving where subscribe.add happens after fire's snapshot establishes a monitor synchronizes-with edge from fire's monitor exit to subscribe's monitor acquire, which gives B1 HB A2 transitively. So A2 always sees the new true value, and there is exactly one fire (via subscribe). A missed fire is not possible. The bug here is strictly the double-fire scenario; the synthesis description's mention of "missed fire" should be disregarded.

Fix. Restore atomicity between the subscriber-registration check and the fire by either: (a) inside subscribe, take the same monitor used by fire while checking shouldFireOnSubscribe, plus mark shouldFireOnSubscribe @Volatile and read it AFTER releasing the subscribers monitor only if the snapshot was taken before B2; or simpler — (b) run the body inline on the caller thread when firedOnSubscribe == true (the synchronous addApplicationLifecycleHandler path), and only defer the natural lifecycle-event onFocus to serial IO; or (c) have fire and subscribe use the same lock so that "subscribed before fire snapshotted" deterministically implies "did not see shouldFireOnSubscribe = true".

[claude]

🟡 The OneSignalDispatchers.prewarm() call in SyncJobService.onStartJob is unlikely to deliver the protection the new comment claims (lines 38-46): because nothing sits between prewarm() and suspendifyOnIO, and Kotlin's by lazy defaults to LazyThreadSafetyMode.SYNCHRONIZED, the main thread typically wins the race to the lazy chain (and pays the init cost) or — if the prewarm daemon wins — blocks on the synchronized monitor for the full init duration. Android's ANR detector treats a monitor-blocked main thread the same as one running slow code, so the cold-start SDK-4507 block this call site specifically targets is not actually prevented here. Consider either inlining the prewarm body into the start of the suspendifyOnIO block (so it always runs off-main), removing the call (the rest of the PR is the real fix), or at minimum updating the comment to reflect the probabilistic semantics.

Extended reasoning...

What the bug is

SyncJobService.onStartJob is delivered on the main thread. The new code is:

override fun onStartJob(jobParameters: JobParameters): Boolean {
    OneSignalDispatchers.prewarm()      // 1
    suspendifyOnIO { ... }              // 2
    return true
}

The accompanying comment asserts: "prewarm() shifts that cost to a short-lived daemon thread." But in this specific call site there is zero synchronous work between (1) and (2), which is exactly what the prewarm optimization needs to be effective. Contrast with OneSignalImp.initWithContext, where prewarm() is followed by synchronized(initLock) { ... }, ensureApplicationServiceStarted(), etc. — substantial wall-clock work that gives the daemon time to win the race.

Why the optimization is ineffective here

All the relevant properties in OneSignalDispatchers (IO, Default, SerialIO, IOScope, DefaultScope, SerialIOScope, ioExecutor, defaultExecutor, serialIOExecutor) are declared by lazy { ... } with no explicit thread-safety mode, which means Kotlin's default LazyThreadSafetyMode.SYNCHRONIZED applies: whichever thread enters the initializer first owns a synchronized monitor; every other thread blocks on that monitor until init completes.

The race timeline on a cold process (the documented SDK-4507 scenario where JobScheduler resurrects a process without initWithContext having run):

1. Main: prewarm() → Thread.start() returns immediately. The daemon is in NEW/RUNNABLE, awaiting OS scheduling.
2. Main: suspendifyOnIO { ... } → suspendifyWithCompletion → if (ThreadingMode.useBackgroundThreading) OneSignalDispatchers.launchOnIO { ... } → accesses IOScope (lazy) → IO (lazy) → ioExecutor (lazy).

The wall-clock gap between Thread.start() returning and the main thread reaching the lazy chain is on the order of nanoseconds (a few method dispatches). The daemon thread needs to be scheduled by the OS and then execute its own launchOnIO to reach the same lazy chain.

Further compounding the race: the prewarm thread is created with priority = Thread.NORM_PRIORITY - 2 (see OneSignalDispatchers.kt:236), which the OS scheduler treats as a hint to favor the main thread.

The "either outcome is bad" key observation

Regardless of who wins the race, the main thread blocks for ~init_time:

• Main wins → main runs the slow initializer inline.
• Daemon wins → main reaches the lazy access a few microseconds later and blocks on the synchronized monitor until the daemon finishes the 5–20 s init. Android's ANR detector treats a WAITING/monitor-blocked main thread the same as one running slow code.

Step-by-step proof on a cold process

1. Process starts because JobScheduler is firing SyncJobService. OneSignalImp.initWithContext has not run; prewarmStarted == false.
2. Main enters onStartJob. Calls prewarm(): it sets prewarmStarted = true, constructs Thread("OneSignal-prewarm", priority=NORM_PRIORITY-2), calls .start(), returns. Daemon is RUNNABLE but not yet on a CPU.
3. Main proceeds to suspendifyOnIO { ... }. Within a few ns: enters suspendifyWithCompletion, checks ThreadingMode.useBackgroundThreading (the SDK-4507 cohort = true), calls OneSignalDispatchers.launchOnIO. That function body is return IOScope.launch { block() } — touching IOScope triggers by lazy { CoroutineScope(SupervisorJob() + IO) }, which in turn touches IO, which in turn touches ioExecutor. The first of these synchronized monitors that the daemon hasn't already entered, main enters.
4. Case A — main wins the monitor: main constructs ioExecutor (ThreadPoolExecutor, allocating its internal LinkedBlockingQueue, spawning core threads, etc.), wraps it in asCoroutineDispatcher(), constructs IOScope, then calls .launch { block() } which hits ThreadPoolExecutor.execute → LinkedBlockingQueue.offer — exactly the stack the PR description cites. Main is blocked for the full init duration.
5. Case B — daemon wins the monitor (less common because of priority + the head-start gap): daemon runs the same construction. Main reaches the lazy access ~μs later, finds the monitor held, blocks in WAITING until daemon releases. Same wall-clock cost on main.

What about the refutation?

The refutation argues this is not actionable because (a) the common case is idempotent (already-initialized SDK → prewarmStarted == true → no-op, lazy chain already warm), (b) the code is not worse than pre-PR, and (c) the bug is really a critique of the comment. Those points are fair:

• The code is correct: no crash, no incorrect behavior.
• It does not regress anything: pre-PR, the main thread blocked 100% of the time on cold start; post-PR, it still blocks (deterministically when main wins, monitor-blocked when daemon wins).

But the reason this is still worth surfacing rather than abstaining: the PR description and the new comment at SyncJobService.kt:38-46 both frame this prewarm() call as a real piece of the SDK-4507 fix, and the JobService cold-start scenario is precisely the one cited in the description ("Android cold-starts a process just to run SyncJobService"). A future maintainer reading the comment will reasonably believe the SyncJobService path is covered, when in practice the cold-process block remains. The real protection in this PR comes from runOnSerialIOIfBackgroundThreading on the lifecycle handlers, not from this prewarm() call.

How to fix

Options (any one is sufficient):

1. Inline the warming into the dispatched block: move all of onStartJob's body inside the suspendifyOnIO (already does that today) — but skip prewarm() entirely here, since by the time suspendifyOnIO runs the cold-init cost has been paid on the IO worker thread anyway. The current prewarm() is dead weight in the cold-process case.
2. Use a non-ANR-sensitive thread for the JobService entry: spawn a worker thread (or use runOnSerialIOIfBackgroundThreading) for the entire onStartJob body, so the cold-init cost — whoever pays it — never lands on main.
3. At minimum, update the comment to acknowledge the race semantics: prewarm is only effective when meaningful synchronous work follows it on the caller's thread, which is not the case here. This keeps a future maintainer from assuming SyncJobService is covered when it isn't.

The third option is the lightest touch and removes the misleading load-bearing claim without changing behavior.

[abdulraqeeb33]

Closing — the SessionService.onFocus / onUnfocused offload from this PR has been consolidated into #2644 (the unified onFocus / onUnfocused offload PR). All five lifecycle handlers (BackgroundManager, NotificationsManager, NotificationPermissionController, FeatureFlagsRefreshService, SessionService) now move together on the same FF in #2644.