fix: SDK-4504 ConcurrentModificationException in Model.initializeFromModel

[abdulraqeeb33]

Summary

Fixes a java.util.ConcurrentModificationException thrown from Model.initializeFromModel when the source model is mutated by another thread while being copied. Reproduced in production on a coroutine worker (DefaultDispatcher-worker-8) inside suspendifyWithCompletion on SDK 5.9.0 with the sdk_background_threading flag enabled.

Linear: SDK-4504

Root cause

fun initializeFromModel(id: String?, model: Model) {
    val newData = Collections.synchronizedMap(mutableMapOf<String, Any?>())

    for (item in model.data) {            // iterates source map without holding its monitor
        ...
    }

    synchronized(data) {
        data.clear()
        data.putAll(newData)
    }
}

model.data is a Collections.synchronizedMap(LinkedHashMap). The wrapper only synchronizes individual method calls — iteration over the entry view requires the caller to hold the wrapper's monitor (per the Collections.synchronizedMap Javadoc). When another thread mutates the source via setOptAnyProperty, the underlying LinkedHashMap.modCount advances mid-iteration and the iterator's fail-fast check throws ConcurrentModificationException.

This regressed in 536fc7e (Update initializeFromModel to build a local map to replace data), which intentionally moved the iteration out of the synchronized block to fix a deadlock but did not re-protect the iteration on the source map.

Affected callers

• SingletonModelStore.replace() — replaces the singleton ConfigModel / IdentityModel / PropertiesModel.
• ConfigModelStoreListener.fetchParams — applies server config over the local config.
• RebuildUserService.getRebuildOperationsIfCurrentUser() — copies identity / properties / subscription models to build rebuild operations.

Any of these flows can race with concurrent property mutations once background threading is enabled.

Production stack trace

java.util.ConcurrentModificationException
    at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:1061)
    at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:1096)
    at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:1093)
    at fu4.initializeFromModel(SourceFile:26)              // Model.initializeFromModel
    at dn0$b.invokeSuspend(SourceFile:131)
    ...

• App package: com.dvex.movp
• Device: TECNO CM6 / Android 15
• SDK: 5.9.0 (ossdk.sdk_base_version=050900)
• Feature flag: sdk_background_threading
• App state: foreground

Fix

1. Snapshot the source map (model.data.toMap()) under synchronized(model.data).
2. Build the new local map from the snapshot — no shared state, no lock needed.
3. Atomically swap this.data under synchronized(data).

The two monitors are acquired sequentially, never simultaneously, so the deadlock that 536fc7e originally fixed cannot return.

Test plan

• New regression test ModelingTests > initializeFromModel does not throw ConcurrentModificationException when source model is mutated concurrently
  • Mutates a source model in a tight loop on one thread while another thread calls initializeFromModel 500 times.
  • Verified to fail with the exact production CME against the buggy code (Expected null but actual was java.util.ConcurrentModificationException).
  • Verified to pass against the fix.
• Existing deadlock tests still pass:
  • ModelingTests > Deadlock related to Model.setOptAnyProperty
  • ModelingTests > Deadlock related to ModelStore add() or remove()
  • ModelingTests > Unsubscribing handler in change event may cause the concurrent modification exception
  • ModelingTests > ensure Model Store load pulls cached operations and doesn't duplicate models
• Local run: ./gradlew :onesignal:core:testReleaseUnitTest --tests "com.onesignal.common.ModelingTests" — all 5 tests pass.

Made with Cursor

[Copilot]

Pull request overview

This PR fixes a production ConcurrentModificationException in Model.initializeFromModel by taking a thread-safe snapshot of the source model’s synchronized map before iterating, preventing fail-fast iterator crashes when the source is concurrently mutated (notably with sdk_background_threading enabled).

Changes:

• Snapshot model.data under synchronized(model.data) and iterate the snapshot instead of iterating the live synchronized map.
• Keep the destination swap (this.data.clear()/putAll) atomic under synchronized(data) without holding both monitors at once.
• Add a regression test that mutates a source model concurrently while repeatedly calling initializeFromModel.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt	Prevents CME by snapshotting the source map under its monitor before copying.
OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt	Adds a concurrency regression test to reproduce and prevent the CME.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[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

• Model.kt: 9/9 touched executable lines (100.0%) (18 touched lines in diff)

Overall (aggregate gate)

9/9 touched executable lines covered (100.0% — requires ≥ 80%)

📥 View workflow run