fix: restore local tags cleared by a concurrent FetchUser

[nan-li]

Description

One Line Summary

Restore locally set tags that a concurrent FetchUser can wipe, by re-asserting server-confirmed tags when an UpdateProperties request succeeds.

Details

Motivation

getTags() could return {} or missing recently-added tags, even when the tags exist on the server. addTags writes tags optimistically to the local model and sends them via OSRequestUpdateProperties. If a FetchUser runs concurrently (e.g. a new session after backgrounding/foregrounding) and its response is missing the just-written tags, its clearUserData() + hydrate wipes the local tag cache and nothing restores it — so getTags() keeps returning only a subset of tags. The write itself succeeds; only the SDK's local mirror ends up wrong.

Fix

On a successful OSRequestUpdateProperties (202) response, merge the tags the server just confirmed (echoed back in the response) into the local properties model. This is a merge, never a replace:

• only the keys present in the response are touched, so tags managed elsewhere (e.g. the app's backend via the REST API) are left intact;
• a "" value is treated as a removal, matching how the server echoes removed tags.

Scope

Affects only user tags, on UpdateProperties success. The FetchUser flow and other user properties are unchanged. No public API changes.

Alternatives considered

• Restore the full local tag snapshot during FetchUser hydration: rejected — it would re-introduce stale local-only tags and drop tags the app's backend set via the REST API, clobbering backend state.
• Preserve only the pending tag writes during FetchUser hydration: viable and race-equivalent to the chosen approach, but needs more cross-executor plumbing (inspecting the operation repo + property executor queues). Merge-on-confirm is simpler, self-contained, and only ever re-asserts tags the server just confirmed.

Trade-offs / concessions / limitations

• This is a best-effort, eventually-consistent, last-writer-wins system. Inherent staleness (e.g. an offline write that lands later and overwrites a newer backend change) is out of scope and not solvable client-side. This fix only addresses the SDK destroying its own successfully-written tags locally.
• The UpdateProperties response echoes only the tags that were sent (confirmed from logs), not the user's full tag set — so this is necessarily a merge, not a full resync.
• Relies on the response echoing tags: the merge reads properties.tags from the 202 response. If a response omits it, the merge simply no-ops for that round (no restoration); a later clean fetch reconciles.
• Assumes removals are echoed as key: "": removed tags come back as a non-empty tags object with empty-string values, which the merge treats as deletions. If the backend ever collapsed an all-removals update to tags: {}, the !confirmedTags.isEmpty guard would skip the merge; a later clean fetch still reconciles.
• Residual race (accepted): a stale FetchUser applied after the write is already confirmed is not covered in that instant; it self-corrects on the next fetch user call.
• Minor wart (accepted): removing a tag whose earlier add is still in flight in a separate request can briefly re-add it until the removal confirms; self-corrects on next fetch user. This is an inherent problem outside the scope of this PR. If 2 concurrent requests updating the same tag key are sent to the server, the value persisted on server just depends on random chance.

Testing

Unit testing

• testMergeConfirmedTags_mergesAndRemovesWithoutTouchingOtherTags — merge semantics: updates, "" removals, additions, and other tags left untouched (never a wholesale replace).
• testUpdatePropertiesResponse_restoresTagsClearedByConcurrentFetch — regression: add tags, simulate a concurrent fetch clearing the local cache, then assert the tags are restored once the update confirms.
• OneSignalUserTests, UserExecutorTests, UserConcurrencyTests, and SwitchUserIntegrationTests pass.

Manual testing

Not performed on-device: the race is timing-dependent and not naturally reproducible, so it is covered by the regression test above, which simulates the clobber via the property model's clearData() — the same clearing a stale FetchUser performs.

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
• PR does one thing
• Any Public API changes are explained in the PR details and conform to existing APIs (no public API changes)

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 (race is timing-dependent and not naturally reproducible; covered by an automated regression test)

Final pass

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

[nan-li]

@claude review

[claude]

LGTM — small, well-scoped fix for the FetchUser tag-clobber race, with the merge re-asserting only server-confirmed tags under the existing lock and good test coverage for both the merge semantics and the regression.

Extended reasoning...

Overview

This PR adds a narrowly-scoped fix for a known race where a stale concurrent FetchUser response wipes locally-set tags. Two source-file changes:

• OSPropertiesModel.mergeConfirmedTags(_:) — a new method that overlays server-confirmed tags onto the local tags dictionary under the existing tagsLock, treating "" as a removal.
• OSPropertyOperationExecutor.executeUpdatePropertiesRequest success handler — after a 202, if this is still the current user and the response echoes a non-empty properties.tags, the executor calls mergeConfirmedTags to re-assert what the server just confirmed.

A unit test exercises the merge semantics (update/remove/add while leaving unrelated keys intact) and an integration-style regression test asserts that tags survive a simulated concurrent FetchUser clobber.

Security risks

None apparent. The change does not touch auth, crypto, permissions, network transport, identifiers, or serialization of credentials. It only mutates an in-memory tag dictionary that the SDK already owns, gated by a current-user check.

Level of scrutiny

Low-to-moderate. It is user-state code (not security-sensitive), the diff is small, the new primitive is pure and easy to reason about, and the call site is guarded (isCurrentUser, non-empty tags). The author already documented the residual race and limitations in the description, and the bug hunting pass found no issues.

Other factors

• Reuses the existing tagsLock (recursive) — no new locking primitives, no risk of deadlock with the existing addTags/removeTags/clearData/encode/hydrateModel call sites that already use the same lock.
• The merge is additive-only relative to keys present in the response, so it cannot drop backend-managed tags.
• The !confirmedTags.isEmpty guard correctly preserves the documented "all-removals echoed as tags: {}" no-op behavior, with the author's note that a later clean fetch reconciles.
• No public API surface changes, no CODEOWNERS file in the repo, and the prior 'nan-li' approval is consistent with the scope of the change.