fix: eliminate the kitsRegistry thread-safety crash by nickolas-dimitrakas · Pull Request #916 · mParticle/mparticle-apple-sdk

nickolas-dimitrakas · 2026-09-02T13:04:54Z

Summary

Stacked on #878 — closes the remaining kitsRegistry/kitsSemaphore scope mismatch that #878 identified but didn't fix: kitsRegistry is class-level (static) state, but kitsSemaphore was a per-instance ivar, so two container instances could mutate the same registry while each held its own, unrelated lock.

Moved kitsSemaphore to static, created alongside kitsRegistry in +initialize. Guarded removeAllSideloadedKits/removeKitsFromRegistryInvalidForWorkspaceSwitch, which mutated the static set with no lock at all.
That alone dropped the crash rate but didn't close it: the actual fault was objc_retain/objc_release on kitRegister.wrapperInstance, read under the lock by activeKitsRegistryWhenLocked but written unlocked elsewhere. Split every wrapperInstance-nilling teardown path (flushSerializedKits, freeKitRegister:) into a synchronous detach under kitsSemaphore plus deferred stop()/cleanup off of it.
That detach-ordering change caused a real regression: removeAllSideloadedKits identified sideloaded kits via wrapperInstance respondsToSelector:, which broke once wrapperInstance was nil'd before it ran. Fixed by identifying sideloaded kits by their code range instead (independent of wrapperInstance). Caught by this PR's own CI, not by me — see commit history for the full account.
Merged forward from fix: close the kitsRegistry flush race and stabilise the ObjC test suite #878: the same freeKitRegister: lock-holding fix, the brackets lock, and the workspace-switch teardown-ordering fix that PR's later commits added.

Testing Plan

testActiveKitsRegistryThreadSafety × 80+ combined local runs: 0 crashes.
testSwitchWorkspaceSideloadedKits × 20 (after the regression fix): 19/20, the one failure a different, pre-existing assertion unrelated to this regression's signature.
Full local suite × 6+, MParticleTests + MPKitContainerTests together × 40+: 0 failures.
CI: all 4 native-tests jobs pass post-merge from fix: close the kitsRegistry flush race and stabilise the ObjC test suite #878.
trunk check clean.

📦 SDK Size Impact Report

Measures how much the SDK adds to an app's size (with-SDK minus without-SDK).

Metric	Target Branch	This PR	Change
App Bundle Impact	1.81 MB	1.81 MB	+N/A
Executable Impact	848 bytes	848 bytes	+N/A
XCFramework Size	6.52 MB	6.52 MB	+N/A

➡️ SDK size impact change is minimal.

Raw measurements

Target branch (main):

{"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":1936,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":1852,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6672}

This PR:

{"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":1936,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":1852,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6672}

Brings this branch's native-tests / build-kits CI up to date with main's #867 (stop simulator cloning that starved the daemons one Swift-scheme test blocks on; bound the SPM resolve with a per-attempt watchdog instead of trusting git's own low-speed abort, which does not reliably fire) and #870 (the workspace-switch tests moved off nested 10-second dispatch_after chains onto MPWaitForCondition, which polls the SDK's own readiness state instead of guessing at a fixed delay). Auto-merged cleanly everywhere except UnitTests/ObjCTests/MParticleTests.m, where both branches had independently rewritten the same five workspace-switch test bodies - main by removing dispatch_after for MPWaitForCondition, this branch by adding migration-specific assertions (resetRegistry, kitContainer_PRIVATE, registeredKits.anyObject) on top of the old dispatch_after version. Git's line-based merge "succeeded" there without a conflict marker, but silently interleaved hunks from both rewrites - 10 dispatch_after calls survived alongside 3 MPWaitForCondition calls, split unevenly across the five methods. Replaced that whole block with a hand-reconciled version carrying both: MPWaitForCondition throughout, this branch's own assertions preserved. The one real conflict marker, on the MParticle() class-extension additions, was two property declarations that both belonged. testActiveKitsRegistryThreadSafety merged without incident - this branch's independent registry work (MPKitContainer.swift's registryLock/registry are both static, unlike main's per-instance kitsSemaphore that #916 fixes) never touched the same lines as main's #870 change to that test, which only reworked how the test itself tracks failures across its four dispatch_group blocks. Verified: no duplicate method names introduced, build-for-testing succeeds, and MParticleTests + MPKitContainerTests run 147/147 clean, including all five workspace-switch tests (0.1-0.4s each, versus 20-30s before) and the thread-safety stress test.

PR #878/#916's own new stress test, testActiveKitsRegistryThreadSafety, crashed CI 2/2 times (once on iOS, once on tvOS, both restarts mid-test) while passing 60/60 isolated local runs plus 3/3 full-suite local runs, both before and after this fix - consistent with a rare, CI-hardware- specific timing window rather than something reproducible on demand. Auditing the locked paths this test exercises turned up a real inconsistency with the architecture the rest of this stack established. flushSerializedKits (895696a, a40a516) deliberately detaches wrapperInstance to nil under kitsSemaphore, then defers stop(), disk cleanup and the mParticleKitDidBecomeInactiveNotification post to dispatch_async(main) - specifically so that arbitrary kit/observer code never runs while every other thread is blocked on the lock. freeKitRegister:integrationId: - reached via configureKits:'s deactivateKits cleanup, which this exact stress test's writer thread exercises on every iteration - detaches wrapperInstance the same way, but then called teardownDetachedWrapperInstance: synchronously, still holding kitsSemaphore. That both stalls every reader thread for however long stop()/disk I/O takes, and risks an outright deadlock if any observer of the notification calls back into a kitsSemaphore-guarded method, since dispatch_semaphore_t is not reentrant. Deferred the same way flushSerializedKits does. The detach itself stays inline under the lock, since that is what synchronizes with isActiveAndNotDisabled:'s reads on other threads. Validated: MPKitContainerTests class x20 and full local suite x3 (both modes previously used to validate this stack), 0 failures, 0 crash restarts. This does not reproduce the CI-only crash locally to confirm root cause directly - filed as the most concrete, evidence-backed lead found by auditing every locked path the failing test touches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

cursor · 2026-09-02T21:05:06Z

PR Summary

Medium Risk
Touches core kit lifecycle, workspace switch, and global registry locking; incorrect teardown ordering could still affect multi-workspace or sideloaded kit behavior.

Overview
Fixes kitsRegistry thread-safety crashes by aligning the lock with shared static state and synchronizing wrapperInstance teardown.

kitsSemaphore moves from a per-instance ivar to a static semaphore created in +initialize alongside kitsRegistry, so every MPKitContainer instance serializes access to the same set. Registry mutations in removeAllSideloadedKits and removeKitsFromRegistryInvalidForWorkspaceSwitch now run under that lock; projection paths use the static semaphore instead of strongSelf->kitsSemaphore.

flushSerializedKits now nils wrapperInstance under kitsSemaphore (with detached instances stored for async teardown), fixing retain/release races with activeKitsRegistryWhenLocked / isActiveAndNotDisabled:. stop(), disk cleanup, and notifications still run on the main queue without holding the lock.

removeAllSideloadedKits no longer detects sideloaded kits via [wrapperInstance respondsToSelector:] (broken after early detach); it uses kit codes >= sideloadedKitCodeStartValue instead.

^{Reviewed by Cursor Bugbot for commit ab721dc. Bugbot is set up for automated code reviews on this repo. Configure here.}

The merge-base changed after approval.

PR #878/#916's own new stress test, testActiveKitsRegistryThreadSafety, crashed CI 2/2 times (once on iOS, once on tvOS, both restarts mid-test) while passing 60/60 isolated local runs plus 3/3 full-suite local runs, both before and after this fix - consistent with a rare, CI-hardware- specific timing window rather than something reproducible on demand. Auditing the locked paths this test exercises turned up a real inconsistency with the architecture the rest of this stack established. flushSerializedKits (895696a, a40a516) deliberately detaches wrapperInstance to nil under kitsSemaphore, then defers stop(), disk cleanup and the mParticleKitDidBecomeInactiveNotification post to dispatch_async(main) - specifically so that arbitrary kit/observer code never runs while every other thread is blocked on the lock. freeKitRegister:integrationId: - reached via configureKits:'s deactivateKits cleanup, which this exact stress test's writer thread exercises on every iteration - detaches wrapperInstance the same way, but then called teardownDetachedWrapperInstance: synchronously, still holding kitsSemaphore. That both stalls every reader thread for however long stop()/disk I/O takes, and risks an outright deadlock if any observer of the notification calls back into a kitsSemaphore-guarded method, since dispatch_semaphore_t is not reentrant. Deferred the same way flushSerializedKits does. The detach itself stays inline under the lock, since that is what synchronizes with isActiveAndNotDisabled:'s reads on other threads. Validated: MPKitContainerTests class x20 and full local suite x3 (both modes previously used to validate this stack), 0 failures, 0 crash restarts. This does not reproduce the CI-only crash locally to confirm root cause directly - filed as the most concrete, evidence-backed lead found by auditing every locked path the failing test touches. #agentic

kitsRegistry is class-level state, created once in +initialize, but kitsSemaphore was a per-instance ivar created in -init. A lock scoped per-instance cannot protect state scoped per-class: two containers could mutate the same set while each held its own semaphore, so the lock could not do the job it was there for. Move kitsSemaphore to a static created alongside kitsRegistry in +initialize, and update the four sites that reached it through strongSelf-> - a static is not reachable through the instance, so those would not have compiled otherwise. Also guard removeAllSideloadedKits and removeKitsFromRegistryInvalidForWorkspaceSwitch, which mutated the static set with no lock at all. The [kitsRegistry copy] in each protects the enumeration but not the removeObject: that follows. Both are called only from mParticle.m, outside any locked region, so taking the lock cannot deadlock against an existing holder. testActiveKitsRegistryThreadSafety over 20 runs: 3 crashes, down from roughly 1 in 3. Reduced, not eliminated - something in this path is still unguarded, so this is a step rather than a fix. Note the tradeoff: a shared static lock means two containers now block each other where they previously raced. That is correct, but dispatch_semaphore is not recursive, so a nested cross-instance call taken under the lock would deadlock rather than race. No such path was found, but it is worth a reviewer's eye. #agentic

Scoping kitsSemaphore to the class and closing the unlocked mutators (previous commit) cut the crash rate in testActiveKitsRegistryThreadSafety from ~1 in 3 to 3 in 20, but did not close it. Real crash reports (.ips, not just the generic "unexpected exit" xcodebuild prints) show all three remaining crashes faulting in objc_retain/objc_release inside isActiveAndNotDisabled:, called from activeKitsRegistryWhenLocked - a use-after-free on a single element, not a set-structure error. kitRegister.wrapperInstance is declared nonatomic, so it has no synchronization of its own; kitsSemaphore only protects code that remembers to acquire it. activeKitsRegistry does, reading wrapperInstance under the lock. But flushSerializedKits's dispatch_async(main) block called freeKitRegister:, which set wrapperInstance = nil, without the lock - deliberately, per the previous commit's comment, to avoid stalling the main thread across file I/O and a notification post. That left the one write this teardown needed to synchronize outside the one section that was supposed to cover it, and a background reader's objc_retain could land mid-write on the object being released. Split the detach from the teardown: flushSerializedKits now nils out wrapperInstance for every snapshotted kit while still holding kitsSemaphore - a pointer swap, not file I/O - then runs stop(), file cleanup and the notification afterward, unlocked, via a new teardownDetachedWrapperInstance: forIntegrationId: that no longer touches kitRegister.wrapperInstance at all. freeKit:/freeKitRegister: (called from configureKits: while it already holds the lock) are unchanged. testActiveKitsRegistryThreadSafety: 0 crashes in 60 runs (was 3/20, was ~1/3). Full ObjC suite x4: 0 crashes in any run. One assertion failure recurred in testSwitchWorkspaceSideloadedKits across those runs; bisected against the parent commit (this fix reverted) under the same load and it fails there too, with a different assertion failing on a different run - pre-existing timing-sensitive flakiness in that test's MPWaitForCondition wait under a heavily loaded machine, unrelated to this change. startKit: and registerSideloadedKits still read/write this registry unlocked. Nothing in the current suite calls them concurrently, so they are not implicated in anything observed, but they are the same category of bug and remain open. #agentic

CI's run-analyzer job flagged the previous commit: 'incompatible operand types (id<MPKitProtocol> _Nullable and NSNull * _Nonnull)' at the ?: that boxes a possibly-nil wrapperInstance for storage in detachedWrapperInstances (NSArray cannot hold nil directly). The idiom is correct at runtime - this is the standard way to carry an optional through an NSArray - clang's ternary type-unification just doesn't like the two branch types. Any new warning fails this job per its filter, so an explicit (id) cast on the protocol side unifies the branches without changing behavior. Verified locally: xcodebuild ... analyze under the same warning filters CI uses exits 0, and no warning is reported at this line (previously present). #agentic

@selector

…rInstance testSwitchWorkspaceSideloadedKits went from passing to failing 4/4 on this PR's CI, on both platforms, always the same way: after switching workspaces, registeredKits.count stayed at 2 instead of dropping to 1, and anyOTAobject's wrapperInstance was nil instead of the new kit instance. That is a regression from the previous commit, not the pre-existing flakiness this branch already carries elsewhere. removeAllSideloadedKits identified sideloaded registers by asking [kitRegister.wrapperInstance respondsToSelector:@selector(sideloadedKitCode)]. Both call sites that matter - resetForSwitchingWorkspaces: and reset:, in mParticle.m - call flushSerializedKits immediately before removeAllSideloadedKits, every time. The previous commit made flushSerializedKits detach wrapperInstance to nil synchronously, before it returns, specifically so the detach happens under kitsSemaphore and closes the race with activeKitsRegistry. That also means wrapperInstance is already nil by the time removeAllSideloadedKits runs right after it - [nil respondsToSelector:] is NO, so the check stopped matching anything, and stale sideloaded kits from the previous workspace were never removed from kitsRegistry. initWithInstance:kitCode: assigns every sideloaded kit a code starting at sideloadedKitCodeStartValue (1e9), on the register itself, independent of wrapperInstance and unaffected by the detach. Switched the check to that. removeKitsFromRegistryInvalidForWorkspaceSwitch has the same wrapperInstance-dependent shape, but it runs before flushSerializedKits at both call sites, so it is not affected by this ordering and is left alone. testSwitchWorkspaceSideloadedKits: 19/20 (the one failure was a different, earlier assertion - "Sideloaded kit was not registered" on the very first wait, before any switch happens - not this regression's signature). testActiveKitsRegistryThreadSafety: 0 crashes in 20, unaffected by this change. MParticleTests + MPKitContainerTests together x4: 140/140, 0 failures each run. #agentic

PR #878/#916's own new stress test, testActiveKitsRegistryThreadSafety, crashed CI 2/2 times (once on iOS, once on tvOS, both restarts mid-test) while passing 60/60 isolated local runs plus 3/3 full-suite local runs, both before and after this fix - consistent with a rare, CI-hardware- specific timing window rather than something reproducible on demand. Auditing the locked paths this test exercises turned up a real inconsistency with the architecture the rest of this stack established. flushSerializedKits (895696a, a40a516) deliberately detaches wrapperInstance to nil under kitsSemaphore, then defers stop(), disk cleanup and the mParticleKitDidBecomeInactiveNotification post to dispatch_async(main) - specifically so that arbitrary kit/observer code never runs while every other thread is blocked on the lock. freeKitRegister:integrationId: - reached via configureKits:'s deactivateKits cleanup, which this exact stress test's writer thread exercises on every iteration - detaches wrapperInstance the same way, but then called teardownDetachedWrapperInstance: synchronously, still holding kitsSemaphore. That both stalls every reader thread for however long stop()/disk I/O takes, and risks an outright deadlock if any observer of the notification calls back into a kitsSemaphore-guarded method, since dispatch_semaphore_t is not reentrant. Deferred the same way flushSerializedKits does. The detach itself stays inline under the lock, since that is what synchronizes with isActiveAndNotDisabled:'s reads on other threads. Validated: MPKitContainerTests class x20 and full local suite x3 (both modes previously used to validate this stack), 0 failures, 0 crash restarts. This does not reproduce the CI-only crash locally to confirm root cause directly - filed as the most concrete, evidence-backed lead found by auditing every locked path the failing test touches. #agentic

nickolas-dimitrakas self-assigned this Sep 2, 2026

nickolas-dimitrakas mentioned this pull request Sep 2, 2026

fix: merge main into workstation/swift-migration and apply pending test fixes #915

Merged

This was referenced Sep 2, 2026

refactor: move attribute projection parsing to Swift #927

Merged

refactor: move webview JSON field reading to Swift #929

Merged

nickolas-dimitrakas marked this pull request as ready for review September 2, 2026 21:05

nickolas-dimitrakas requested a review from a team as a code owner September 2, 2026 21:05

thomson-t previously approved these changes Sep 3, 2026

View reviewed changes

nickolas-dimitrakas dismissed thomson-t’s stale review via 49025c5 September 3, 2026 15:37

nickolas-dimitrakas mentioned this pull request Sep 3, 2026

fix: close the kitsRegistry flush race and stabilise the ObjC test suite #878

Merged

thomson-t previously approved these changes Sep 3, 2026

View reviewed changes

thomson-t force-pushed the fix/kit-registry-static-lock branch from 49025c5 to 4f79d22 Compare September 4, 2026 16:14

Base automatically changed from fix/kit-registry-lock-and-rokt-test-stability to main September 4, 2026 16:16

nickolas-dimitrakas added 5 commits September 4, 2026 12:16

thomson-t force-pushed the fix/kit-registry-static-lock branch from 4f79d22 to ab721dc Compare September 4, 2026 16:16

thomson-t approved these changes Sep 4, 2026

View reviewed changes

thomson-t merged commit 5eb0c41 into main Sep 4, 2026
78 checks passed

thomson-t deleted the fix/kit-registry-static-lock branch September 4, 2026 16:44

cursor Bot mentioned this pull request Sep 4, 2026

chore: Release v9.4.3 #961

Merged

nickolas-dimitrakas mentioned this pull request Sep 9, 2026

fix: merge main into workstation/swift-migration and port pending concurrency fixes #982

Closed

nickolas-dimitrakas mentioned this pull request Sep 9, 2026

fix: merge main into workstation/swift-migration and port pending concurrency fixes #983

Merged

4 tasks

Navigation Menu

Uh oh!

fix: eliminate the kitsRegistry thread-safety crash - #916

fix: eliminate the kitsRegistry thread-safety crash#916
thomson-t merged 5 commits into
mainfrom
fix/kit-registry-static-lock

nickolas-dimitrakas commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nickolas-dimitrakas commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing Plan

Related

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 SDK Size Impact Report

Uh oh!

cursor Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nickolas-dimitrakas commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

cursor Bot commented Sep 2, 2026 •

edited

Loading