fix: [SDK-5137] user request queue and identity model follow-ups from the JWT review by nan-li · Pull Request #1740 · OneSignal/OneSignal-iOS-SDK

nan-li · 2026-09-11T21:13:24Z

Description

One Line Summary

Six follow-ups from the Identity Verification review in the user request queue, request authorization, and the identity model, tracked on SDK-5137.

Details

Motivation

Review feedback on the Identity Verification stack was collected on [SDK-5137] instead of amending the stacked branches. This PR takes the items that live in the user request queue and the identity model. One of them is a regression against main that does not depend on Identity Verification.

Scope

Each fix is its own commit with its own test, and each test was run against the previous commit to confirm it fails without the fix.

A login made while offline could be lost on the next launch. With Identity Verification off or not yet known, a restored Identify User that was waiting on its own Create User was dropped at startup. main kept it. That rule is restored, along with main's guard against a request that can never be sent blocking the queue.
The app could be told the wrong user was signed in. A Create User held for a token can finish after the app has switched to another user. Its response no longer reports that earlier user and no longer overwrites the saved current user.
A login could get stuck until the next launch. A Create User held by the short cool-down after a new subscription was treated as if it were waiting for a token, so it was never retried and the app was never asked for one. The queue now tells the two apart and retries.
Changes made during a user fetch could be silently dropped. The identity model lost its IDs for a moment before each fetch response was applied, so work built in that moment looked anonymous and was discarded, and the request queue could read the user as not yet created. Both IDs now survive. One side effect is deliberate. A fetch response without an external ID no longer marks the user anonymous; only a login as a different user replaces it.
Removing an email or SMS subscription no longer waits for a token. The server does not check the user token on that endpoint, so waiting only delayed the unsubscribe, and after a logout it could delay it forever.
Two tests read the operation repo's queue from the test thread while the repo was writing to it. They now read a snapshot taken on the repo's own queue. Test code only.

Follow-ups from review, each its own commit:

A request now leaves the queue only after its response has been applied. Before, a send that ran in between could drop a login whose ID was about to arrive, and save that drop.
The removal of an email or SMS subscription is exempted from the token check inside the authorization step itself, rather than by skipping that step, so one place still decides every request's header.
The wrong-user report is now tested at all three places a response applies a user's ID, not just one. Two of those cases do not need Identity Verification at all.
A test covers the deliberate side effect above: a fetch response without an external ID keeps the user identified.
The OneSignal ID also survives the clear before a fetch response is applied, so a request that needs it during that moment is never dropped.

Not changed: the public API, the push subscription path, and in-app messaging.

Testing

Unit testing

One test per fix, next to the code it covers. Two new files, UserStateReportingTests and UserExecutorRetryTests, because the existing test classes are at SwiftLint's size limit.

Manual testing

Not run on a device. The full unit test plan was run locally on an iPhone 17 Pro simulator, 479 tests. One known intermittent test failed once and passed on re-run; nothing else failed. Each new test was also run against the commit before its fix and fails there.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
Any Public API changes are explained in the PR details and conform to existing APIs

Testing

I have included test coverage for these changes, or explained why they are not needed
All automated tests pass, or I explained why that is not possible
I have personally tested this on my device, or explained why that is not possible

Final pass

Code is as readable as possible.
I have reviewed this PR myself, ensuring it meets each checklist item

🤖 Generated with Claude Code

…reate User `uncacheUserRequests` on 5.7-main kept a restored Identify User only when Identity Verification was on or the Request could prepare at once, and the Identify prepare needs the `onesignal_id` of the user it identifies. That dropped the archive an offline first launch with a `login` leaves behind, the anonymous Create User followed by the Identify User that logs A in, on relaunch whenever the requirement was off or unknown, so the login was lost. On main the Identify was kept whenever its identify model was already in the repo, which a restored Create User ahead of it guarantees, and it went out once that response supplied the id. PR6 (#1711) collapsed those cases into one check. Restore main's rule. Keep the Identify when its identify model is in the repo, or while the requirement is not known to be off so reshape can decide, otherwise only when it can be sent as is. Keep main's guard too. Once the requirement is known off, `reshapeInvalidRequests` drops an Identify whose user never received an `onesignal_id` and has nothing queued to supply one, because the FIFO loop stops at the first unexecutable Request and would otherwise retry it for the life of the install. Three tests in UserExecutorTests: the archive above with the requirement off, the same archive turning out to require auth, and a never-preparable Identify kept while unknown then dropped once auth is known to be off. The first two fail without the fix.

…its model `OSIdentityModel.hydrateModel` fired the user-state observer for whichever model it hydrated. Stepping over a parked Create User (#1711) lets a later login proceed, so that parked Request can now complete after another user is current, and its response reported the earlier user as signed in and persisted that pair, so nothing later re-reported the current user. Move the fire out of the model to the three executor sites that hydrate an identity model (Create User and Fetch User through `parseFetchUserResponse`, Identify User, and Fetch Identity By Subscription), through `OSUserStateSnapshot.fireUserStateChangedIfCurrent`, which reports only when the model still belongs to the current user. The hydration itself stays, since Requests queued behind that user need the `onesignal_id`. Test in a new UserStateReportingTests, since UserJwtLifecycleTests is at SwiftLint's type body limit: login(A) with no token, login(B) with one, then answer A's ask; the app hears nothing new and the persisted pair still names B. Fails without the fix.

…ting for a token `_executePendingRequests` steps over a Request the auth layer parked for a token, so a login for another user behind it is not stranded, and it schedules no retry for a wait only the app can end. It told a park apart from any other failed prepare with `awaitsToken`, which only asked whether the owner had a token. A Create User still inside the new-records cool-down on its push subscription fails to prepare before the auth layer runs, so with Identity Verification on and no token yet it was stepped over as if parked: nobody was asked, nothing retried, and the login sat in the queue until the next launch. Replace `awaitsToken` with `parkedForToken`, which answers whether the last authorization of that Request actually parked it. `OSRequestAuth` keeps the parked Requests in a weak table: `park` adds one, every authorization starts by forgetting it so the entry reflects the latest attempt, and the executor consumes the answer right after a failed prepare. A prepare that fails before authorizing therefore reads as not parked and gets the delayed retry. Tests: three unit tests on the mark's lifecycle in OSRequestAuthTests, and UserExecutorRetryTests, a new file since UserExecutorTests is at SwiftLint's type body limit, holding a Create User in the cool-down and asserting the app is asked once it lifts. Fails without the fix.

… hydrates `OSIdentityModel.clearData` blanked every alias in preparation for the Fetch User response that hydrates them, and `executeFetchUserRequest` runs that clear right before parsing the response, on the response thread. A properties or subscription Delta built in that gap on another queue read the current user's `externalId` as nil, and with Identity Verification on `OSOperationRepo.enqueueDelta` drops an anonymous Delta before it is ever persisted, so the change was lost for good. Keep `external_id` in `clearData`. The fetch that follows is by `onesignal_id`, so it cannot change who the user is, and its response overwrites the alias anyway. One behavior change comes with it: a fetch response that omits `external_id` no longer demotes the local user to anonymous. Only a server-side unlink produces one, and the next `login` corrects it, so that reads as the right trade against silently dropping an identified user's work. Test in OSIdentityModelTests: an identified model keeps `externalId` across `clearData` and loses every other alias. Fails without the fix. The ownership convention comment in OSUserRequest.swift no longer cites the blanked aliases as the reason for the owner stamp.

…tests `OSOperationRepoTestSupport.snapshotDeltaQueue()` reads `deltaQueue` on the repo's own queue, but it lived in OneSignalOSCoreTests, so DeltaOwnershipTests and UserJwtLifecycleTests read the array directly from the test thread while the repo appends to it on `dispatchQueue`. Move the helper into OneSignalOSCoreMocks, which both test targets link, and route the two reads through it. No remove or update coverage is added, per the decision on #1710: neither Delta's owner reaches the server.

…g it for a token `OSRequestDeleteSubscription.prepareForExecution` ran the Request through `authorize`, so under Identity Verification a delete whose owner had no valid token was parked and the app was asked for one, and the executor's failure handler treated a 401 as a rejected token. The route the SDK uses, `DELETE /apps/{app}/subscriptions/{id}`, performs no JWT check: the server registers it with auth skipped, only the by-alias variant validates Identity Verification claims, and the OAuth layer in front treats the user bearer as not applicable. Signing bought nothing, and parking cost an unsubscribe: `removeEmail` or `removeSms` while the user had no token, followed by a logout or a switch, left the delete parked, restored on every launch, asking for a token the app could no longer supply, and the subscription stayed on the server. Drop the `authorize` call and the 401 branch. The owner stays on the Request for the anonymous purge, which is what it was for. The ownership convention in OSUserRequest.swift records the exception next to Update Subscription, which sends unsigned for the same reason. Test in ExecutorAnonymousPurgeTests: with Identity Verification on, a delete owned by a user with no token goes out with no Authorization header and nobody is asked. Fails without the fix.

…se hydrates the id `dropIdentifyUsersThatCanNeverPrepare` runs on every send and drops an Identify User whose user has no `onesignal_id` and no queued Create User or Fetch Identity By Subscription to supply one. The response handlers that supply an id dispatched `removeFromQueue` first and hydrated the model after it, on the callback thread. A send pass landing on the executor queue between the two saw no id and no supplier, dropped the Identify User and persisted the drop. Narrow, since the pass has to be dispatched inside that gap by remote params hydrating, a session start, a login, a token update or a delayed retry, but the loss is the offline login the restored `[Create User, Identify User]` archive exists to keep. Dispatch the removal after the hydrate at the three sites that supply an id (Create User, Fetch Identity By Subscription, Identify User). A pass that runs before the removal still sees the supplier queued; one that runs after it is ordered behind the hydrate by the removal's dispatch. The drop itself stays per send, since it also ends the livelock `main` had when a Fetch Identity By Subscription failed for good. The ordering is documented on the drop rule and at each site; no deterministic test can pin it without a hook inside the response handler, so none is added. Found by the adversarial review of #1740.

…estAuth 0b5fc76 stopped signing Delete Subscription by skipping `authorize` altogether, while `sendsUnsigned` still read as if only Update Subscription went out with no header. Two mechanisms for one exemption, and the header doc's claim that `OSRequestAuth` is the one place a header is decided no longer held. Have `authorize` let any `sendsUnsigned` Request through unsigned up front, owner or not, and have Delete Subscription declare the flag and call `authorize` again from its prepare. Update Subscription is unaffected: its owner is always nil, so it took the exempt branch either way. The doc on `sendsUnsigned` now names both Requests, and the ownership convention says Delete declares the flag rather than that it is "never signed either". Tests in OSRequestAuthTests: an owned exempt Request with no token goes through unsigned, unparked and without an ask, and one with a token on hand is still not signed. Both fail against the previous commit's `authorize`, as does the purge test's unsigned delete, since the Delete prepare calls `authorize` again. Found by the adversarial review of #1740.

… sites 1d1f050 moved the user-state report to three executor sites but tested only the parked Create User. The other two are reachable with Identity Verification off: a `login` while anonymous followed by a second `login` before the Identify User returns reported the first user after the app had switched to the second, and a 3.x fetch-identity landing after a `login` reported the anonymous user the app had logged in over. Both new tests fail against an unconditional fire at their site. A deleted call would not fail them, since the Fetch User that follows re-reports the current user; the guard is what they pin. Also from the review of that file: the idle check read the mock client's request lists off the test thread while the mock appends under its lock, so the mock gains a lock-backed `isIdle` and the test observer keeps its states under a lock. The negative count assertion does not depend on the 0.1s pause after it, since the report decision is made inside the response block and the mock records completion after that block returns; the comment now says so, and the pause stays to drain the executor queue before teardown.

…nal_id d9d6f11 keeps `external_id` across `clearData` and accepted that a Fetch User response omitting it no longer demotes the user, but the test stopped at the clear. What makes the kept alias safe is `internalAddAliases` merging rather than replacing the dictionary, and nothing locked that in. Add the hydrate step: after a clear, a response carrying only `onesignal_id` leaves `external_id` in place.

Comment-only. Each hydrate-then-remove site is down to one line that points at `dropIdentifyUsersThatCanNeverPrepare`, which states the constraint once. The test docs name the scenario in a sentence or two instead of retelling it, and the rest drop wording that contrasted with the code they replaced.

cursor

Multi-model review (Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6) of the SDK-5137 user-queue / identity-model follow-ups.

The five fixes match the stated intent: hydrate-before-dequeue closes the Identify drop race, token-park is no longer inferred from a missing JWT, external_id survives clearData, and Delete Subscription is unsigned on purpose. No consensus blocker on the main paths.

Act on: none

Consider

Empty-success Create User / Fetch Identity dequeue without waking the queue, then a later send can drop the Identify (Grok; related Opus).
When IV is off, a dead Identify is dropped rather than promoted to Create User (Opus). Same-user login is a no-op, so that promotion can be lost for the session.
parkedForToken is consumable; an inner execute*Request park plus a later pre-authorize failure can skip the delayed retry (Opus warning, Grok nit).
fireUserStateChangedIfCurrent releases currentUser(matching:) before persist/notify, so a concurrent login can still report the previous user (Sol; Grok dismissed as overwritten by a later hydrate).
authorize now lets any sendsUnsigned request through before the no-owner refuse (Opus nit + Grok warning; Sol dismissed). Purge still drops anonymous deletes.

Noted: a fetch that omits external_id no longer demotes the user (documented; empty string still unlinks).

Dismissed: hydrate-then-remove race (2/3 closed); treating a 401 on unsigned Delete as JWT handling (2/3 Sol+Grok; same as Update Subscription).

_{Sent by Cursor Automation: PR Reviews}

fadi-george · 2026-09-21T17:59:30Z

Potential issues:

dropIdentifyUsersThatCanNeverPrepare can run after Fetch User is dequeued but while clearUserData has removed its onesignal_id and before hydration restores it. A concurrent login/send pass then sees no ID or queued supplier and permanently drops the Identify request. Consider keeping Fetch User queued until hydration completes, or promoting the Identify to Create User when its target is still current.

abdulraqeeb33

Hydrate after clearData merges, so a Fetch User that omits external_id never unlinks. Same-user login("A") no-ops because the kept alias still matches. Under IV those writes are addressed as A.

Keep the alias only in the clear-to-hydrate gap. Replace the identity map when a complete identity object arrives.

…ser hydrates `clearData` kept only `external_id`, so for the moment between the clear and the hydrate a live user's model had no `onesignal_id`. `dropIdentifyUsersThatCanNeverPrepare` runs on every send and does not count a Fetch User as a supplier, so a `login` landing in that moment, with a send pass behind it, dropped the Identify User and persisted the drop. Keep `onesignal_id` too: the fetch is addressed by one of the two ids, so neither can change. The same doc comment claimed the next `login` corrects a fetch response that omits `external_id`. It does not; a same-user login returns early while the local alias still matches, so only a login as someone else replaces it. The comment and the test docs now say that. The replace-on-hydrate alternative raised in review is deferred to its own ticket. The clear test's `onesignal_id` assertion flips from nil to kept, which the previous commit fails by construction.

nan-li · 2026-09-21T20:20:15Z

Potential issues:

dropIdentifyUsersThatCanNeverPrepare can run after Fetch User is dequeued but while clearUserData has removed its onesignal_id and before hydration restores it. A concurrent login/send pass then sees no ID or queued supplier and permanently drops the Identify request. Consider keeping Fetch User queued until hydration completes, or promoting the Identify to Create User when its target is still current.

Fixed in de1bb50. The clear now keeps the OneSignal ID as well as the external ID, so the drop rule never sees a live user without an ID; the fetch is addressed by one of the two, so neither can change. Keeping Fetch User queued would not have covered it on its own, since the rule does not count a Fetch User as a supplier. Promoting a dead Identify User is a separate, pre-existing case on main.

UserExecutorTests is 348 body lines on this branch and 331 on 5.8-main, each under SwiftLint's 350-line type_body_length error, but the two additions meet in the merge and the merged class is 407 lines, which fails the Swift Lint job. Move the three restored-login tests and their three helpers into UserExecutorRestoredLoginTests in the same file, next to the archive tests that already live there, so nothing changes in the project file. The branch class drops to 272 lines and the merged one to 331.

abdulraqeeb33

Thank you for addressing the changes

…utor

nan-li added 6 commits September 11, 2026 12:35

nan-li mentioned this pull request Sep 16, 2026

fix: [SDK-5137] Identity Verification config and token request follow-ups from the JWT review #1743

Merged

18 tasks

nan-li added 5 commits September 16, 2026 16:57

nan-li mentioned this pull request Sep 17, 2026

fix: [SDK-5137] in-app message fetch wait follow-ups from the JWT review #1744

Merged

18 tasks

nan-li marked this pull request as ready for review September 18, 2026 02:34

cursor Bot reviewed Sep 18, 2026

View reviewed changes

nan-li changed the base branch from 5.7-main to 5.7-main-temp September 18, 2026 16:42

nan-li requested a review from a team September 21, 2026 16:24

nan-li changed the base branch from 5.7-main-temp to 5.8-main September 21, 2026 17:55

abdulraqeeb33 requested changes Sep 21, 2026

View reviewed changes

Comment thread iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift

nan-li requested review from abdulraqeeb33 and fadi-george September 21, 2026 21:06

fadi-george approved these changes Sep 21, 2026

View reviewed changes

abdulraqeeb33 approved these changes Sep 22, 2026

View reviewed changes

Merge remote-tracking branch 'origin/5.8-main' into nan/sdk-5137-exec…

7b404fc

…utor

nan-li merged commit 56af6f6 into 5.8-main Sep 22, 2026
13 of 16 checks passed

nan-li deleted the nan/sdk-5137-executor branch September 22, 2026 16:58

Navigation Menu

fix: [SDK-5137] user request queue and identity model follow-ups from the JWT review - #1740

fix: [SDK-5137] user request queue and identity model follow-ups from the JWT review#1740
nan-li merged 14 commits into
5.8-mainfrom
nan/sdk-5137-executor

nan-li commented Sep 11, 2026 •

edited

Loading

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fadi-george commented Sep 21, 2026

Uh oh!

abdulraqeeb33 left a comment

Uh oh!

Uh oh!

nan-li commented Sep 21, 2026

Uh oh!

abdulraqeeb33 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nan-li commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fadi-george commented Sep 21, 2026

Uh oh!

abdulraqeeb33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nan-li commented Sep 21, 2026

Uh oh!

abdulraqeeb33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nan-li commented Sep 11, 2026 •

edited

Loading