Conversation
…ays up The manager-release test kept the subscription token on file, so the second fetch was released by that token whether or not the bar had come down. It now asserts the bar is down after the release, forgets the tokens, and has a fresh user token release the second fetch on its own. The timeout test gave the second fetch the same short timeout as the first, so it could return by timing out again. It now asserts the bar is down after the timeout and restores the full timeout, so the second fetch can only return by being released.
…times out Timing out lowers the bar an in-session subscription change raised, and every fetch waiting on the id shares that bar, so a sibling that already had its user token could go out at that moment. Nothing re-checked it, so it sat out its own timeout, up to 30 seconds, and then went out with the token it had all along. The timed-out waiter is deregistered first, so the re-check cannot release it twice. The timeout is read before the waiter registers, so the test can shorten it for one waiter and restore it before registering the next.
…sters later is not held for it The executors released only the fetches already waiting when a response came back without a token, so a fetch that registered a little later waited the full timeout for a token that was never coming. On a first launch that is the normal order: the Create User response schedules the fetch through two queue hops and releases waiters before the fetch has registered. The four executors now file a blank entry under their own key, which the fetch condition counts as the write having completed, and resolveConditions is gone. A fetch held for an in-session subscription change now waits for that subscription write to complete, with or without a token, where before any tokenless response for the user released it. The manager's token map is readable through a mocks helper, so the executor tests can check what a fetch would find on file.
…create-subscription response The create-subscription response carries `ryw_token` and `ryw_delay` next to `subscription`, where the update path and Android read them. The create path read them out of the nested object, so it always filed nil. Now that a tokenless write files a blank entry, that blank replaced a real subscription token on every email or SMS add, and the next fetch went out without it. Test: a create-subscription response with a top-level token reaches the map under the subscription key. Fails on the previous commit.
nan-li
marked this pull request as ready for review
September 18, 2026 01:24
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6) of the IAM fetch-wait follow-ups.
Blank entries, the sibling timeout re-check, the tightened bar tests, and reading ryw_token beside subscription match the stated intent. No consensus blocker for the current API (it still returns ryw_token).
Act on
None that are both consensus and clearly unintended on today's path.
Consider
- (2/3 Opus+Grok) A tokenless Create User now leaves a blank
userCreateentry.isMetshort-circuits on anyuserCreatepresence, so a later in-session subscription change never holds the fetch. OldresolveConditionsreleased only current waiters and left the map empty. A real create token already behaves this way — confirm the fallback should match that for the rest of the process. - (Grok; related Opus overwrite)
subscriptionUpdateis one slot. Create-subscription now files there, so a leftover entry can satisfy a bar raised for a later push update. The bar-lowering test has toreset()for this. Same presence model as before once an update has already returned a token. - (Opus)
setRywTokenAndDelayoverwrites unconditionally. A later tokenless write under the same key drops a real token — the hazard the follow-up commit fixed for the nested-token misread. - (Opus) Timeout re-check uses the shared per-id bar. Waiter A timing out can release waiter B that registered after a newer subscription write was armed.
- (2/3 Opus+Sol) Create-subscription still returns before filing when
subscriptionis missing (documented already-exists empty body). Filing that blank could also satisfy a bar raised for a different write; add does not raise the bar itself.
Noted
- (2/3) Sibling-timeout test uses a 1s setup window and can flake if the first waiter times out early.
- (Sol warning / Opus nit) Removing
@objc public resolveConditionsis a framework ABI break; already called out, no in-repo callers. - (Opus)
@testable import OneSignalOSCorefrom the Mocks framework target; Release hasENABLE_TESTABILITY = NO.
Dismissed
- Cross-platform
resolveConditionsdivergence — stated as intentional.
Sent by Cursor Automation: PR Reviews
Comment on lines
364
to
+371
| if let onesignalId = request.identityModel.onesignalId { | ||
| if let rywToken = response["ryw_token"] as? String | ||
| { | ||
| let rywDelay = response["ryw_delay"] as? NSNumber | ||
| OSConsistencyManager.shared.setRywTokenAndDelay( | ||
| id: onesignalId, | ||
| key: OSIamFetchOffsetKey.userCreate, | ||
| value: OSReadYourWriteData(rywToken: rywToken, rywDelay: rywDelay) | ||
| ) | ||
| } else { | ||
| // handle a potential regression where ryw_token is no longer returned by API | ||
| OSConsistencyManager.shared.resolveConditions(conditionId: OSIamFetchReadyCondition.CONDITIONID, forId: onesignalId) | ||
| } | ||
| // Filed even when the response has no ryw_token, so a fetch that registers after this write | ||
| // is not held for a token that never comes. | ||
| let rywData = OSReadYourWriteData( | ||
| rywToken: response["ryw_token"] as? String, | ||
| rywDelay: response["ryw_delay"] as? NSNumber | ||
| ) | ||
| OSConsistencyManager.shared.setRywTokenAndDelay(id: onesignalId, key: OSIamFetchOffsetKey.userCreate, value: rywData) |
There was a problem hiding this comment.
Consider (2/3 Opus+Grok): this files a blank under userCreate when ryw_token is missing. OSIamFetchReadyCondition.isMet returns true as soon as any userCreate entry exists, and indexedTokens is never pruned in production, so a later in-session subscription change will not hold the fetch.
Old resolveConditions released only waiters already registered and left the map empty. A real create token already short-circuits the same way — confirm the tokenless fallback should match that for the rest of the process. If not, treat a nil-token create as completion of create only (rywToken != nil for the short-circuit, or also require !hasSubscriptionUpdatePending).
Comment on lines
360
to
+375
| @@ -366,21 +366,16 @@ extension OSSubscriptionOperationExecutor { | |||
| } | |||
|
|
|||
| if let onesignalId = request.identityModel.onesignalId { | |||
| if let rywToken = response["ryw_token"] as? String | |||
| { | |||
| let rywDelay = response["ryw_delay"] as? NSNumber | |||
| OSConsistencyManager.shared.setRywTokenAndDelay( | |||
| id: onesignalId, | |||
| key: OSIamFetchOffsetKey.subscriptionUpdate, | |||
| value: OSReadYourWriteData(rywToken: rywToken, rywDelay: rywDelay) | |||
| ) | |||
| } else { | |||
| // handle a potential regression where ryw_token is no longer returned by API | |||
| OSConsistencyManager.shared.resolveConditions(conditionId: OSIamFetchReadyCondition.CONDITIONID, forId: onesignalId) | |||
| } | |||
| // Filed even when the response has no ryw_token, so a fetch that registers after this write | |||
| // is not held for a token that never comes. The token sits beside `subscription`, not inside it. | |||
| let rywData = OSReadYourWriteData( | |||
| rywToken: response?["ryw_token"] as? String, | |||
| rywDelay: response?["ryw_delay"] as? NSNumber | |||
| ) | |||
| OSConsistencyManager.shared.setRywTokenAndDelay(id: onesignalId, key: OSIamFetchOffsetKey.subscriptionUpdate, value: rywData) | |||
There was a problem hiding this comment.
Consider (2/3 Opus+Sol; Grok on the leftover slot): the RYW file sits after the subscription guard. MockUserRequests.setAddEmailResponse documents that a real create-subscription response can be empty when the address already exists, so this path still files nothing — the hole this PR closes on the other three executors.
Filing before the guard is not free: create-subscription now occupies the one subscriptionUpdate slot, and isMet only checks presence. A leftover (or already-exists blank) can satisfy a bar raised for a later push update. getAddModelDelta does not raise the bar, so this mainly matters when an update is already in flight.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Three follow-ups from the review of #1706, all in how an in-app message fetch waits for the writes before it. 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 three items from the consistency manager review on #1706. #1740 and #1743 cover the rest.
Scope
Each fix is its own commit with its own test.
ryw_tokenis now recorded like any other. Before, the executor released only the fetches already waiting at that moment, so a fetch that registered a little later waited the full timeout for a token that was never coming. On a first launch that is the normal order of events, since the Create User response schedules the fetch through two queue hops and releases waiters before the fetch has registered. The four executors now file a blank entry under their own key, the fetch condition only checks that the entry is there, and the separate release call is gone. This only matters if the API stops returningryw_token; today the stored token already covers a late fetch.One behavior change to know about. A fetch held for an in-session subscription change now waits for that subscription write to complete, with or without a token, where before any tokenless response for the user released it. That is what the bar exists for, and the timeout still bounds the wait.
Follow-ups from review, each its own commit:
Not changed: request signing, what the fetch request contains. One method does go away from the OneSignalOSCore framework, the release call the executors no longer need, shipped in 5.6.1 as
resolveConditionsWithID. Nothing outside the SDK calls it. Android still uses it and does not record a write that returns no token, so the two platforms differ here on purpose.Testing
Unit testing
For the first fix, the new
ExecutorReadYourWriteTestsdrives each of the four executors with a tokenless response and checks that the fetch condition is met afterwards. All four fail on the previous commit. The manager and condition tests that covered the old release call are rewritten against the blank entry. For the second fix, the new sibling test inOSIamFetchReadyConditionTestsfails on the previous commit. For the third, the two tightened tests were run with bar lowering disabled in the manager and both failed. For the follow-up, the same file checks that a create-subscription response carrying a token records it for the fetch; that test fails on the previous commit.Manual testing
Not run on a device. The full unit test plan was run locally on an iPhone 17 Pro simulator. Xcode 27 rejects the project's iOS 12 deployment target, so the local build overrode it on the command line; nothing in the project changed for that.
Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code