fix: [SDK-4899] lock OSSubscriptionModel state to stop encode crashes#1694
fix: [SDK-4899] lock OSSubscriptionModel state to stop encode crashes#1694nan-li wants to merge 1 commit into
Conversation
OSSubscriptionModel is archived on background queues (operation repo delta queue, executor pending-request caches, model store persistence) while other threads mutate its String-typed properties (APNs token, permission changes, server hydration). The unsynchronized reads crashed in production with EXC_BAD_ACCESS in objc_retain during encode(with:) (GitHub #1588). Move the stored properties into a value-type State guarded by a single NSLock held only for raw reads/writes - never while firing change events, notifying observers, or writing UserDefaults - so the re-entrant archive path and callouts into app code cannot deadlock. encode(with:), jsonRepresentation(), and currentPushSubscriptionState now read one consistent snapshot. All existing setter side-effect logic is preserved, including notificationTypes pinning to -2 while disabled without firing a change event. Co-Authored-By: chunhuidu-dev <270835659+chunhuidu-dev@users.noreply.github.com>
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a non-trivial concurrency refactor of a core model class (OSSubscriptionModel) that's used in REST subscription requests and fixes a production crash, so I'd like a human to weigh in before merge.
What was reviewed:
- Verified the new
NSLock-guardedStatestruct is only ever held for raw reads/writes, never acrossset(property:)/change-notifier callouts, ruling out re-entrant deadlock on the setter -> model store save ->encodepath. - Checked
encode(with:),jsonRepresentation(), andcurrentPushSubscriptionStateall take a singlesnapshot()so they no longer read torn state. - Examined the
notificationTypespin-to--2-while-disabled path (flagged by the bug hunting system) — confirmed it's a real, transient value visible during concurrent encode/jsonRepresentation, but the lock ensures each individual read is internally consistent, not corrupted.
Extended reasoning...
This PR converts all stored properties of OSSubscriptionModel into a private lock-guarded State struct to fix a production EXC_BAD_ACCESS crash in encode(with:) (issue #1588) caused by concurrent mutation during archiving. It touches getters/setters for every property (address, subscriptionId, notificationTypes, reachable, isDisabled, testType, deviceOs, sdk, deviceModel, appVersion, netType), the two initializers, encode/decode, jsonRepresentation, and currentPushSubscriptionState. Test coverage is good: a deterministic pause-mid-encode snapshot test, a 1000-iteration concurrent hammer test, and a targeted test for the notificationTypes pin-to-2 behavior.
No injection/auth/crypto surface is introduced — this is purely internal state synchronization for a model used to build REST request bodies (subscription create/update). The main risk class here is concurrency correctness (deadlock or missed synchronization) rather than a classic security vulnerability.
I traced the lock usage carefully: every setter releases stateLock before calling self.set(property:...) or firing change/observer callouts, so the documented invariant (never hold the lock across notifications or UserDefaults writes) holds, and I did not find a path where the lock is re-acquired while already held on the same thread. I also compared the new notificationTypes setter against the original didSet-based implementation; the new version no longer fires a "notificationTypes" change event on the corrective pin-to--2 path (previously a re-entrant didSet callback did fire one), but since the value being corrected to was already the pinned state, this looks like an intentional and harmless simplification rather than a regression — the accompanying test explicitly asserts no change event fires here.
Given the production-crash motivation, the breadth of the refactor across a widely-used model, and the general principle that concurrency-lock refactors in core state classes benefit from a second set of human eyes, I'm deferring rather than approving outright, even though I did not find any concrete bugs.
1 participant
Description
One Line Summary
Lock-protect
OSSubscriptionModelstored state so concurrent encode can no longer crash.Details
Motivation
Fixes production
EXC_BAD_ACCESScrashes inOSSubscriptionModel.encode(with:)when the model is archived on a background queue (operation repo delta queue / subscription executor request caches) while another thread mutates its String-typed properties. Same crash as GitHub #1588.Scope
OSSubscriptionModelonly: stored properties move into a value-typeStateguarded by oneNSLock, held only for raw reads/writes (never across change events, observers, or UserDefaults).encode(with:),jsonRepresentation(), andcurrentPushSubscriptionStateread one consistent snapshot.Thanks @chunhuidu-dev for #1685 that informed this fix.
Testing
Unit testing
Added
SubscriptionModelConcurrencyTests:notificationTypespin-to--2while disabled without firing a change event, such as when already opted out and something tries to write a non-negative-two value (e.g. a permission refresh with 15). Disabled is supposed to supersede the real notification type. While disabled, don’t let a stray write of 15 fire a change.Manual testing
Not personally reproduced on device (customer crash; not reliably reproducible). Covered by the unit tests above.
Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor