fix: [SDK-5099] respect REST API-disabled push subscriptions - #2728
fix: [SDK-5099] respect REST API-disabled push subscriptions#2728nan-li wants to merge 13 commits into
Conversation
📊 Diff Coverage ReportDiff Coverage Report (Changed Lines Only)Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff). Changed Files Coverage
Overall (aggregate gate)90/91 touched executable lines covered (98.9% — requires ≥ 80%) |
A push subscription disabled through the REST API (notification_types -31) was re-enabled by the SDK: RefreshUser discarded the server's disable state for push, the session-start self-heal re-asserted local truth over it, and every subscription payload recomputed enabled from device state. Mirror the server's disable code on the push model when RefreshUser reports it, report it back in subscription payloads instead of the device-derived values, skip the stuck-subscription self-heal for it, and carry it across the login/logout user switch. The mirror clears when the server reports any other state and on an explicit optIn(). The 404 recovery paths (user rebuild and update-404 re-create) treat the dead record's disable as gone and recreate from device truth. Also remove the mislabeled DISABLED_FROM_REST_API_DEFAULT_REASON(-30) enum case; no OneSignal API has ever written -30 as a REST disable. Enum-name persistence now parses leniently in the shared model accessor, so models and queued operations persisted under an unknown enum name read as SUBSCRIBED instead of throwing on upgrade.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review
Models: Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6.
The -31 mirror, self-heal skip, payload reporting, optIn() clear, and 404 device-truth recreate are internally consistent. Two holes can still re-enable a REST-disabled push subscription — the bug this PR is meant to close.
Act on
- Session-start / queued update wins over hydrate (2/3).
getUpdateOperationfreezesenabled/statusat enqueue time. On first launch of this build,SubscriptionManager.refreshPushSubscriptionStatewrites a newsdk(NORMAL) whilerestApiDisabledReasonis still0, enqueueingUpdateSubscriptionOperation(enabled=true, SUBSCRIBED). RefreshUser then records-31, but that already-queued PATCH still re-enables. Same shape for any token/permission write while the GET is in flight. Snapshot at execute time, or drop/rewrite in-flight push updates once-31is recorded. - 4.x→5.x legacy sync never sets
restApiDisabledReason(3/3).createPushSubscriptionFromLegacySyncnow parses-31into devicestatusand leaves the new field at0.addOrUpdatePushSubscriptionTokenthen overwritesstatuswithSUBSCRIBED, and the next payload is enabled. SeedrestApiDisabledReasonwhenisRestApiDisable(notificationTypes). - 404 recovery fallback recreates the dead disable (2/3). If the cached model is missing, recovery uses
lastOperation.enabled/status, which can be(false, DISABLED_FROM_REST_API). That contradicts “the dead record’s disable is gone.” Do not reuse-31from the failed op.
Consider
- Unknown persisted enum names (including the removed
-30case) fall back toSUBSCRIBED(3/3). Fail-open can re-enable on downgrade;ERRORis safer for modelstatus. notificationTypes == nullleaves a recorded-31in place but does not skip self-heal (3/3). Treat omit as unknown: no clear and no self-heal.optIn()then a still-in-flight RefreshUser GET can write-31back after the clear (Opus). PublicoptedInwill not show it.- Opt-out (or permission loss) during
-31is never sent; when the server later clears-31, hydrate isHYDRATEso nothing re-asserts local opted-out (Opus).
Noted / dismissed
optIn()tests!= 0instead ofisRestApiDisable— same today; they diverge if a second code is added.- Detekt baseline still lists
DISABLED_FROM_REST_API_DEFAULT_REASON$30. One new MagicNumber is unlikely to fail CI (maxIssues: 10); still worth regenerating the baseline. - Leftover pre-upgrade self-heal ops: possible if the last PATCH never ran, not “almost certainly” still queued.
- No
RebuildUserServicetest for the 404 clear; no test that publicoptedInstays true while-31is recorded.
Sent by Cursor Automation: PR Reviews
Holistic multi-model review (not just the diff)Models: Claude Opus 5, GPT 5.6 Sol, Claude Sonnet 5 — findings independently re-checked against the subscription lifecycle, operation queue, and iOS #1731. Request changes. The Act on1. Upgrade session PATCHes
Same shape for token rotation while RefreshUser is in flight, and for persisted pre-upgrade ops. iOS builds PATCH / Create User from a live snapshot ( 2. 4.x→5.x legacy sync never sets
If 3.
That Create groups with iOS clears when the subscription ID resets, which is the right trigger. This path has no unit test ( Consider
Product calls (PR asked reviewers to confirm; iOS matches)
What is solidA separate Tests that would have caught the Act-on itemsSession-start upgrade (sdk write + RefreshUser Traced beyond the 14-file diff: |
nan-li
changed the title
The update-404 recovery now starts from device truth even when the cached model is missing, replaceAll carries restApiDisabledReason across the push model copy, and the user-404 rebuild is covered by tests. IPushSubscription.optedIn documents that it reflects the user's preference and permission rather than a server-side disable, and the detekt baseline is regenerated. A session-start device-metadata write that precedes RefreshUser can still send enabled=true once before the server's disable is learned; that bounded window is accepted, matching iOS.
|
As mentioned in the ios equivalent PR: this PR fixes the coordination between "the server switched this subscription off through the API" and local SDK state. Opting in and out can happen on the SDK and enabling and disabling can happen on the REST API. Many review comments are about a moment where this server state is missing, stale, or ignored. The sync between SDK and server is a fundamental inherent problem regardless of this PR. It will never be completely right for all. Consider that if someone adds a tag or calls optIn() and there is no connection. 30 minutes later, they update something via the REST API. Later, the SDK gets a connection 8 hours later, it will send those updates even though it is technically stale. |
Items 1 to 3 are answered on their own threads. On the rest. If a stored status is one this version doesn't recognize, we treat it as subscribed, see the SubscriptionModel thread. If the server's reply leaves out notification_types, nothing turns back on, because once a disable is recorded the SDK already treats itself as disabled. If the user opts in while we're still waiting on the server, the opt-in still gets sent, because it was queued as enabled before the reply came back. The old disable can sit locally until the next fetch, then it clears. If the user opts out while a REST disable is in effect, the opt-out goes out with the next regular update. Same as any change made offline, the most recent change wins. Smaller notes. The detekt baseline is regenerated. Updates left over from before the upgrade are the same one-time gap. RebuildUserService tests are in 9807ac1. There's no test that optedIn stays true while a disable is recorded, the doc comment covers it. |
Smaller items. The missing-model case is fixed and replaceAll now keeps the field, both in 9807ac1. An opt-in during a pending fetch still goes out. An opt-out during a disable goes out with the next regular update. A reply missing notification_types doesn't turn anything back on. Unknown status is treated as subscribed. Product calls, yes to both. optedIn means the user's choice plus OS permission only, and the doc comment says so on both platforms. Keeping the disable across login and logout is intended because the same push record is reused. |
Keep both detekt baseline LongMethod entries from main after the CoreModule and FeatureFlagsBackendService changes landed there. Co-authored-by: Cursor <cursoragent@cursor.com>
SDK review — request changesFan-out: Opus + Grok + GPT, cross-checked against iOS #1731. The Please address (also inline):
|
optIn() always sends a subscription update, but a RefreshUser fetch queued before that update still reports the disable the opt-in cleared. Recording that stale -31 again meant the next device update re-sent it and the next fetch read it back, silently undoing the opt-in. The push model now carries an in-memory flag set by optIn() and cleared when the server reports any state other than a REST API disable. While it is set, RefreshUser leaves the recorded reason alone. The flag arms on every opt-in rather than only when a disable was already recorded, because the same race exists on the first fetch after the customer disables the subscription.
|
Notification type -22 (from dashboard) is not handled. |
|
Woks for rest api disabling. small nit: after setting to unsubscribed via rest api and doing cold-start on demo. the enabled toggle is still shown as enabled (visual bug). |
The server reports notification_types -22 when someone turns a subscription off by hand from the dashboard. That means the same thing as -31, disabled through the REST API, so both codes now suppress outgoing subscription payloads the same way and neither is derived from device state. The two codes stay distinct. SubscriptionModel.remoteDisabledReason records whichever one the server sent, and SubscriptionModelStoreListener maps it back to its own status rather than reporting every remote disable as -31. The widened check also covers the optedIn derivation on refresh, the stuck-subscription self-heal guard, and the dead-record recovery in SubscriptionOperationExecutor. Renamed isRestApiDisable to isRemoteDisable and restApiDisabledReason to remoteDisabledReason, since "REST API" no longer describes the concept. The property name doubles as the persistence key, but no release has written the old name, so this needs no migration.
hydrateRemoteDisableState changed the model silently, so parsing -22 or -31 off the wire left no trace at any log level short of the raw HTTP body. Add a DEBUG line at the point the value actually changes.
optedIn documented that "the user is able to receive notifications through this subscription", and the public reference says it returns true when the subscription status is subscribed. Neither holds once the SDK respects a remote disable, because the disable now sticks instead of being flipped back on by the next routine update, and nothing else in the public API reveals it. An app syncing preferences through the REST API would read "subscribed" forever on a device receiving nothing. The disable is read from the model's recorded reason rather than its status, which stays device-owned, so a device-recoverable error code still reports opted in. optIn() already clears the reason, so a preference-center toggle reading false is not a dead end. The push observer already fires on the hydration write, so its payload now carries a real optedIn transition instead of an unchanged pair.
The observer callback goes out through Dispatchers.Main, so nothing in this repo covered an IPushSubscriptionObserver receiving a change. That left the delivery half of the remote disable untested: the payload was pinned through refreshState, but not the path from a model write to the app's callback. This installs a test main dispatcher in its own spec, wires a real SubscriptionModelStore to a SubscriptionManager, and asserts that a HYDRATE-tagged write of the server's disable code reaches an attached observer as optedIn true to false. Kept separate from SubscriptionManagerTests so the dispatcher swap does not touch the other specs in that file.
|
Disabled from dashboard seems to work now. |
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
Potential issues:
remoteDisableClearedByUseris set by everyoptIn()call but is only cleared after RefreshUser observes a non-disabled server state. If the server is legitimately disabled again before that happens, all subsequent-22/-31responses are ignored for the lifetime of the process. Could this guard instead be tied to completion or ordering of the opt-in update?- On session start, SDK/app metadata changes can enqueue an
UpdateSubscriptionOperationwithenabled=truebefore RefreshUser hydratesremoteDisabledReason. Hydration uses theHYDRATEtag, so it does not enqueue a corrective update. The earlier PATCH can therefore re-enable the subscription. This seems especially problematic for dashboard-22, since it will not be automatically reapplied and is not limited to one sync cycle. - While
remoteDisabledReasonis present,getSubscriptionEnabledAndStatus()gives it precedence over localoptedIn=false. AnoptOut()therefore sends-22/-31rather thanUNSUBSCRIBE. If the subscription is later enabled remotely, RefreshUser clears the reason usingHYDRATE, leaving the server enabled without reasserting the local opt-out. UserSwitcher.createAndSwitchToNewUser()copiesremoteDisabledReason, but notremoteDisableClearedByUser. If login/logout occurs while an opt-in update and an older RefreshUser request overlap, the replacement model can record the stale disable and undo the opt-in.
So we can see what info changed
Device metadata written at session start enqueues a subscription update before RefreshUser knows about a disable, and the operation freezes its enabled at that point. Recording the disable under HYDRATE generated no operation of its own, so the queued update re-enabled the subscription and the next fetch cleared the local record to match. A dashboard unsubscribe never recovered from that on its own, unlike a REST API disable that the integration re-sends on its next sync. Record under NORMAL so the write produces its own update. Executors read the last operation in a merged batch, so the correction replaces the stale one while it is still queued, and re-disables the subscription when it already went out. Clearing the record now generates an update too. That is the first chance to send an opt-out made while the disable was suppressing every payload.
remoteDisableClearedByUser armed on every optIn() and only came down when a fetch reported a non-disabled state. If the app owner disabled the subscription again before that arrived, every later -22 and -31 was discarded for the rest of the process. Clear it once the opt-in's write has been sent. The operation repo runs one batch at a time, so a fetch issued after that point reports current state and the disable it carries has to be recorded. A retry or an auth failure means the write is still coming, so the guard stays armed. Also covers the merge precedence the previous commit depends on. Nothing asserted which values a batch holding both a stale enabled and a corrective disable actually sends.
createAndSwitchToNewUser copied remoteDisabledReason but not remoteDisableClearedByUser, which lives in memory rather than as a model property and so does not travel with the copy beside it. A login or logout landing between an opt-in and its write handed the replacement model a cleared guard, letting an older fetch record the disable the opt-in had just cleared.
|
Thanks, fixed in three commits, one per item.
|
SDK-5128 rewrote DemoLog to hold its own tag, so its methods take a message and nothing else, and it dropped the TAG constant from MainViewModel. The onPushSubscriptionChange log this branch added still passed a tag. Neither side conflicted textually, since they touched different lines, so the merge compiled on each branch alone and failed only once combined. Drop the tag argument from that one call.
Stepping back, we're chasing gaps one round at a time. The SDK writes while offline and the server keeps whatever it hears last, so the two will never be perfectly in sync. I'd rather name the gaps we accept and merge on that than keep patching orderings. After this PR, they are: updates left in the queue from an older SDK build go out once; a new push token or a login that lands before the first server check after a disable; an opt-out during a disable sends the disable code until the disable lifts; recreating a subscription the server deleted, when we have nothing local, starts it subscribed. Each happens once. If that list is the bar, I'd like to merge on it. |
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Nits only on this HEAD. The -22/-31 mirror, optedIn, observers, and 404/login carry look consistent with iOS.
Fadi's settleOptInGuard item is still the open request. Not re-raising it.
Small split: public optedIn tests remoteDisabledReason == 0, payloads use isRemoteDisable. Same today, they diverge if a third code is added.
3 participants
Description
One Line Summary
Stop re-enabling push subscriptions that the app owner disabled remotely, from the dashboard (
notification_types-22) or through the REST API (-31).Details
Motivation
Customers who suppress users by disabling subscriptions via the REST API see them come back subscribed. RefreshUser discarded the server's disable state for push, the session-start self-heal (SDK-4474) re-asserted local truth over it on every app open, and every subscription payload recomputed
enabledfrom device state. A dashboard unsubscribe writes a different code and was undone the same way. Tracked internally as SDK-5099.Scope
Push subscriptions only. The push model gains a server-owned
remoteDisabledReasonfield that mirrors the server'snotification_types.SubscriptionStatusgainsMANUALLY_UNSUBSCRIBED(-22)besideDISABLED_FROM_REST_API(-31), andremoteDisableStatus/isRemoteDisableanswer for both. RefreshUser records whichever of the two the server reported, verbatim, and clears on any other reported value. While a code is recorded,getSubscriptionEnabledAndStatusreportsenabled = falsewith that same code, so create, update and login payloads preserve what the server sent rather than collapsing both codes into one. The self-heal skips both, and the field is carried across the login/logout user switch.optIn()clears it. Recording and clearing log at DEBUG. The 404 recovery paths (user rebuild and update-404 re-create, with or without the cached model) treat the dead record's disable as gone and recreate from device truth, andreplaceAllcarries the field across the push model copy.Public
IPushSubscription.optedInnow reports false while a disable is recorded, as doesPushSubscriptionState.optedIn, and both doc comments say so. It reads the recorded reason rather thanstatus, which stays device-owned. RefreshUser writes the field with theHYDRATEtag, and nothing between the model and the observer filters on that tag, soonPushSubscriptionChangefires with the new opt-in answer while the store listener still declines to send an update for it. The demo app logs the observer payload.Also removes the mislabeled
DISABLED_FROM_REST_API_DEFAULT_REASON(-30)enum case. No OneSignal API writes -30 as a remote disable, and the docs list -30 among the APNs error codes. The dashboard code is -22, now handled above. Because models and operations persist enum properties by name,Model.getOptEnumPropertynow parses leniently so anything persisted under an unknown enum name reads asSUBSCRIBEDinstead of throwing on upgrade. AddingDISABLED_FROM_REST_API(-31)also fixes -31 email/SMS rows previously misparsing toSUBSCRIBED.Decisions from review
optedInnow reports false while a remote disable is recorded, which reverses the earlier decision on this branch to keep it to user preference plus app permission. A remote disable suppresses delivery as surely as a missing permission or an opt-out, and it is the only one of the three an app has no other way to see. iOS matches.optIn()overrides the suppression. An in-memory flag keeps a fetch that started before the opt-in's update from recording the disable again, which would otherwise be re-sent by the next device update. The user-404 rebuild starts fresh from device truth rather than carrying the disable: an exclusion list is disable-only and re-applied daily, so a born-disabled record would strand users who leave the list. An opt-out recorded during a suppression goes out on the next routine update rather than outranking the recorded disable, consistent with every other offline edit. The legacy 4.x seed is skipped (v4 cached device-derived codes), and the -22 email/SMS misparse is out of scope. Known limit: a device-metadata update queued before RefreshUser on the first launch after a disable (guaranteed once at upgrade) sendsenabled: trueonce, bounded to one sync cycle; omitting those fields from metadata updates was built and reverted as too much branching for that case. Matches the companion PR in OneSignal-iOS-SDK.Testing
Unit testing
New coverage across six suites. RefreshUser records and mirror-clears both codes and skips the self-heal for them; payload reporting through
getSubscriptionEnabledAndStatusechoes the recorded code;optIn()clears it and reportsoptedIntrue again; the user switch carries it; update-404 recovery starts from device truth;RebuildUserServicerecreates fresh; unknown persisted enum names parse leniently for both models and operations, throughinitializeFromJson. A newPushSubscriptionObserverTestssuite drives a hydrated disable through the model store to a registered observer and asserts the transition the app sees, pinning both codes end to end.Manual testing
Device tested before and after with REST API calls.
Affected code checklist
Checklist
Overview
Testing
Final pass