Skip to content

Fix subscription model encoding and notification races#1685

Open
chunhuidu-dev wants to merge 1 commit into
OneSignal:mainfrom
chunhuidu-dev:duchunhui/fix-subscription-model-encode-race
Open

Fix subscription model encoding and notification races#1685
chunhuidu-dev wants to merge 1 commit into
OneSignal:mainfrom
chunhuidu-dev:duchunhui/fix-subscription-model-encode-race

Conversation

@chunhuidu-dev

Copy link
Copy Markdown

Description

One Line Summary

Prevents concurrent OSSubscriptionModel mutations from crashing during archival or emitting stale, out-of-order subscription updates.

Details

Motivation

Fixes #1588.

OSSubscriptionModel.encode(with:) previously read mutable reference fields while other queues could replace them. Archiving could therefore race ARC retain/release operations and crash in objc_retain.

State writes and set(property:newValue:) notifications were also separate operations. Concurrent setters could update the model in one order but enqueue deltas in another order. Push subscription observers also rebuilt their snapshots from live state, which could combine values from different mutations.

Scope

  • Store mutable subscription fields in a lock-protected value-type state.
  • Archive and serialize one atomic state snapshot.
  • Serialize each mutation together with its model delta and push observer notification.
  • Build push subscription previous and current values from paired mutation snapshots.
  • Add deterministic concurrency coverage for archival snapshots and mutation notification ordering.
  • No public API changes.

Testing

Unit testing

Added deterministic tests that:

  • Pause keyed archiving at the address field, mutate the model concurrently, then decode and verify the archive contains one consistent snapshot.
  • Hold the first address notification while confirming a second setter is actually waiting on the mutation lock, then verify notifications remain ordered.

Validation performed on an iPhone 16 simulator running iOS 26.5:

  • OneSignalUserTests build-for-testing succeeded.
  • testEncodingSubscriptionModel_withConcurrentMutation_usesConsistentSnapshot passed.
  • testConcurrentSubscriptionChangesNotifyInMutationOrder passed.

Manual testing

Physical-device testing was not performed because this is an internal synchronization fix covered by deterministic unit tests.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all required sections above.
  • This PR addresses one concurrency issue and its regression coverage.
  • There are no public API changes.

Testing

  • I have included test coverage for these changes.
  • The relevant targeted build and automated tests pass; the full suite was not run because the change is scoped to OSSubscriptionModel concurrency.
  • The lack of physical-device testing is explained above.

Final pass

  • The code is as readable as possible.
  • I reviewed the complete PR diff.

@chunhuidu-dev chunhuidu-dev marked this pull request as ready for review July 15, 2026 13:36
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.

[Bug]: Crash in v5.2.14: OneSignal.OSOperationRepo - OSSubscriptionModel.encode(with:) + 298 (OSSubscriptionModel.swift:298)

1 participant