SDK-496 Fix inAppConsume behavior and add JSON-only message signal - #1087
Conversation
The public inAppConsume overloads sent the server request but never removed the message locally or posted iterableInboxChanged, despite their docs saying they remove it from the list. They now route through InAppManager.remove, which does all three. Inbox change notifications also fire only after the local mutation completes, so observers no longer read stale message counts; removePrivate and silent-push removal previously posted on a different queue than the mutation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1087 +/- ##
==========================================
+ Coverage 72.32% 73.67% +1.34%
==========================================
Files 114 114
Lines 9594 10202 +608
==========================================
+ Hits 6939 7516 +577
- Misses 2655 2686 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Removing a popup or JSON-only message announced an inbox change that never happened. removePrivate and silent-push removal now post only when the removed message was an inbox message. Silent-push removal also persisted the pre-removal message map because the value-type snapshot was captured before removeValue; it now persists the map after removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JSON-only messages were handed to onNew once and auto-consumed, with no way to recover a payload the app missed during a cold start or background fetch; customers polled getMessages and diffed. Adds an availability contract: an optional onJsonOnlyMessageAvailable delegate method and iterableJsonOnlyInAppMessageAvailable notification, both on the main thread, backed by a durable identity-scoped unhandled queue persisted before either signal fires. Delivery is at least once until the app acknowledges via markJsonOnlyMessageHandled; unhandled records replay on foreground and are queryable with getUnhandledJsonOnlyMessages. Retention defaults (30 days, 100 records per identity, clear on identity change, immediate triggers only) are working values pending product confirmation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sumeruchat
changed the title
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The unhandled queue pruned expired records only on load, but enqueue and prepareDelivery appended and returned a message without checking expiry. A message fetched while valid could expire before the next foreground replay and still be signaled to the app. Reject an already-expired message in enqueue and prepareDelivery using the same rule loadCurrentState applies, so an expired JSON-only message is never delivered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes three gaps in the JSON-only availability path found in review. A message enqueued for user A could be persisted and signaled under user B if the identity switched during the async hop to the main queue: prepareDelivery resolved the live identity and its append-if-missing branch recreated the record under the new user. Store operations now validate against the identity captured at the start of the processing run and fail closed on mismatch, and prepareDelivery only delivers records that already exist. Active fetch also persisted eligible JSON-only records one at a time, so the first availability callback could not see later records from the same response. All eligible records are now enqueued before any signal, on both the active and background paths. Initial active delivery followed display priority; it now follows server arrival order, matching the replay path and the documented contract. HTML display selection keeps priority ordering. Duplicate message IDs keep the first stored payload until acknowledgement; a test now pins that behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A fetch issued for user A whose response resolved after a switch to user B was processed under B: the identity scope was captured only after the network call returned, so A's messages could land in B's message map and JSON-only queue. The scope is now captured before the fetch and the whole response is discarded as a no-op when the identity has changed by the time it resolves, without updating lastSyncTime. Also: the JSON-only store no longer rewrites persisted state on reads that changed nothing, and jsonOnlyMessageQueueData lost its silent no-op protocol default so every conformer must implement storage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A user switch could race in-app processing at several boundaries: a fetch response resolving after the switch, delivery callbacks running mid-switch, queued HTML processing for the previous user, and public getters reading the old map. An IdentityCoordinator (recursive critical section plus a monotonic generation) now scopes every stage: fetch commits merge and map assignment atomically against identity publication, JSON-only delivery revalidates between each customer callback and before the consume mutation, HTML processing prechecks context before the display gate and before onNew without holding the lock across customer code, and getters read context and map in one section. Customer and display callbacks run on a dedicated processing queue instead of the sync queue. Acknowledgements are durable: markJsonOnlyMessageHandled records the message ID with a canonical payload fingerprint (deployment-neutral recursive encoder, iOS 10 safe), so an acknowledged unchanged payload stays suppressed, a changed payload readmits, and retention or capacity eviction of an unacknowledged record cannot readmit it. Acknowledgement and tombstone state apply to JSON-only messages only; an HTML message reusing an ID is unaffected, and cross-type transitions cannot halt batch processing. Server read-state overwrites no longer clear local consumed state, readmitted records reach delivery tracking, and eligible records persist in one batch write. The JSON-only callbacks run inside the identity critical section; the public docs on onNew, onJsonOnlyMessageAvailable, and the notification state the resulting cross-thread wait restriction. Objective-C gains IterableAPI.jsonOnlyInAppMessageAvailableNotification. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joaodordio
left a comment
joaodordio
left a comment
There was a problem hiding this comment.
The code quality and test depth are genuinely very good!
I'm marking as CR so we can make a clear decision on the In AppManager with the callback-under-lock design!
| if delivery.isInitial { | ||
| guard self.identityCoordinator.performIfCurrent(identityContext, | ||
| identityProvider: self.identityProvider, { | ||
| _ = self.inAppDelegate.onNew(message: delivery.message) | ||
| }) else { | ||
| result.resolve(with: false) | ||
| return | ||
| } | ||
| } | ||
| guard self.identityCoordinator.performIfCurrent(identityContext, | ||
| identityProvider: self.identityProvider, { | ||
| self.inAppDelegate.onJsonOnlyMessageAvailable?(message: delivery.message) | ||
| }) else { | ||
| result.resolve(with: false) | ||
| return | ||
| } | ||
| guard self.identityCoordinator.performIfCurrent(identityContext, | ||
| identityProvider: self.identityProvider, { | ||
| self.notificationCenter.post(name: .iterableJsonOnlyInAppMessageAvailable, | ||
| object: delivery.message, | ||
| userInfo: nil) | ||
| }) else { | ||
| result.resolve(with: false) | ||
| return | ||
| } |
There was a problem hiding this comment.
These callbacks run while holding the identity lock, which is a deadlock risk.
onNew, onJsonOnlyMessageAvailable, and the notification post are passed as the block to performIfCurrent, which executes that block inside withCriticalSection's NSRecursiveLock (Auth.swift:33, 61-65, 75), on the main thread (InAppManager.swift:790-794). So the customer callback runs with the identity lock held.
IterableAPI.email and auth (which most track/network calls build their request from) re-enter that same lock through identityValues() (InternalIterableAPI.swift:21, 81-82, 975-977). So a callback that hops to another queue and synchronously waits while that work reads an identity API deadlocks: the worker thread blocks on the lock main holds, and main is blocked on the sync. Same-thread reentry is fine thanks to the recursive lock, but a cross-thread synchronous wait is an easy pattern to hit in a callback whose whole job is to hand the customer a payload to process.
Can we snapshot what we need under the lock, release it, invoke the customer callback outside the critical section, then re-validate identity (generation + snapshot) after it returns? If we keep it as-is, the restriction needs to be a lot louder in both the delegate header and docs PR
There was a problem hiding this comment.
Adopted in 6200375. The three surfaces now run with no SDK lock held: check identity before, invoke unlocked, revalidate on return; a stale result stops everything after that surface, and the mutation/consume guard is unchanged. The cross-thread wait restriction is gone from the public docs, replaced by the actual contract: a callback already selected for the previous user may complete after a concurrent switch, and no later SDK step runs for it.
| private func identityValues() -> (email: String?, userId: String?) { | ||
| identityCoordinator.withCriticalSection { (_email, _userId) } | ||
| } |
There was a problem hiding this comment.
This is the re-entry point that closes the deadlock loop: identityValues() takes the same identity lock, so any identity read (email, auth, and track via auth) called from a callback-spawned thread while the JSON-only callback holds the lock on main will block here. See the callback-under-lock comment in InAppManager.swift:754.
There was a problem hiding this comment.
Resolved by the same change (6200375): callbacks no longer hold the identity lock, so this re-entry can no longer close a deadlock loop.
Removes scaffolding left by iterative hardening: the dead skipAndConsume processor case, the processAndShowMessage entry guard subsumed by the later per-boundary checks, and the store's test-only default coordinator and no-context wrappers. Restores unrelated EOF whitespace. The readmission test intermittently observed the first message's delivery track through the second message's callback because the mock network session invokes callbacks asynchronously; the test now waits for the initial track before acknowledging, so the two phases are serialized without disabling over-fulfillment checks. No production tracking behavior changed. Also bounds the CHANGELOG acknowledgement claim to the retained metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adopts the review request on the delivery design: onNew, the availability delegate, and the notification post are now invoked with no SDK lock held. Each surface is prechecked against the captured identity and generation, invoked unlocked, and revalidated on return; a stale result stops every later surface, mutation, delivery tracking, and consume. This removes the deadlock where a callback synchronously waiting on another thread that touches an identity-reading SDK API blocked forever, and with it the public cross-thread wait restriction. The replacement contract, documented on the delegate methods and the notification, is that a callback already selected for the previous user may complete after a concurrent switch, with no subsequent SDK step performed for it. Concurrency tests now assert the switch completes while a callback is paused and the suffix stays suppressed at every boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joaodordio
left a comment
joaodordio
left a comment
There was a problem hiding this comment.
Thanks for addressing the changes! Much better now.
Let's get the CI to pass green on all tests specifically on the one I've commented on.
Can we please also fix these:
- Remove or resolve
// Product defaults pending confirmation.for the 30-day / 100 caps. set(read:)still postsiterableInboxChangedfor any message. Minor inconsistency with the new inbox-only rule.
| persister.clear() | ||
| } | ||
|
|
||
| func testInboxAndInAppCallbacksTogether() { |
There was a problem hiding this comment.
This test is failing on CI with count 2 on the first iterableInboxChanged callback. The new fetch pipeline resolves the Pending after finishSync posts to callbackQueue, and this observer then hops to main.async before reading getInboxMessages(). That lets payload 2 land before the first callback runs, so the first fulfillment sees both inbox messages. Either wait for the first inbox callback (and the first onNew) before sending payload 2, or assert on the callback queue without the extra main.async.
Let's get this green before merge since it sits on the path this PR reworked.
JSON-only replay was added to start() reading applicationStateProvider.applicationState directly, so when start() ran off the main thread the SDK made an invalid off-main read of UIApplication.applicationState. Under Xcode 16.x, which runs async XCTest bodies on a concurrency worker, this terminated the test host and crashed the three async disableDevice tests in CI; the same access would be invalid for any app initializing the SDK off the main thread. Replay now resolves the active state through a main-thread helper, mirroring the existing getAppIsReady hop but without its display gate, so JSON-only data replay stays decoupled from HTML display state. The regression test asserts the read happens on the main thread, so it fails deterministically on any toolchain rather than depending on scheduling. Also hardens InboxTests.testInboxAndInAppCallbacksTogether, which had no happens-before edge between observing the first inbox notification and submitting the second payload, so the deferred observer could read the later count. It now waits for the first notification and its assertion before the second fetch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Your two follow-ups are in
|
set(read:) posted iterableInboxChanged for every message, so showing a popup announced an inbox change even though the message was never in the inbox. It is now gated on saveToInbox like the add and remove paths. reset() and sync overwrite still post unconditionally. Also drops the provisional marker on the JSON-only queue limits. The 30 day retention and 100 record caps are SDK limits on a local queue rather than a public contract, and are documented as such. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Renames consumeOnReplay to consumePreviouslyDelivered. The flag means consume even though initial delivery already began, so the old name read backwards at the branch that decides it. Adds rationale comments where the constraint is not visible from the code: why identity publication is announced before taking the lock and why the announcement spans logout and republication, what the three processing phases are and what messagesRevision guards, why the per-callback identity checks cannot be collapsed, why JSON-only delivery bypasses the HTML display gate, why one merge branch must precede another, and why a missing unhandled record is never recreated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test gave all queued requests the same JWT failure response and relied on Core Data preserving insertion order for equal scheduled timestamps. Under contention an unauthenticated task could run first, receive the JWT failure, and stop the runner before the test installed its success observer. Assign explicit task ordering, configure only the first authenticated request to fail, and register the success observer before starting the runner so the test deterministically exercises unauthenticated draining during auth pause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What
This PR addresses SDK-496 by fixing local in-app consumption and inbox notifications, adding durable JSON-only availability, and hardening identity switching.
The design and delivery contract are documented here: https://iterable.slab.com/posts/changes-to-in-app-inbox-notifications-and-json-only-message-behavior-ios-gvzb5sco
Changes
inAppConsumenow removes the message locally, sends the consume request, and postsiterableInboxChangedafter local state changes only for inbox messages. Add and remove paths ignore non-inbox messages, and silent-push removal persists the post-removal list.onJsonOnlyMessageAvailable(message:)and notificationiterableJsonOnlyInAppMessageAvailablefire.getUnhandledJsonOnlyMessages()recovers pending messages, which replay on foreground untilmarkJsonOnlyMessageHandled(messageId:)acknowledges them.expiresAt, and applies 30 day retention, 100 messages per user, and immediate triggers only. Those limits are SDK implementation defaults on a local queue, not a public contract.IdentityCoordinatorscopes fetch, delivery, queued processing, and auth reads to the identity that started them. Stale work is discarded, and within a running process public getters do not return messages from another user.Impact
There are no API surface breaking changes and no new dependencies. The six intentional behavior changes are:
inAppConsumecallers now get the documented local removal.iterableInboxChangedno longer fires for non-inbox additions, removals, or read-marking, so showing a popup no longer announces an inbox change.onNewfor a JSON-only message now fires asynchronously on the main thread, at most once, instead of synchronously during fetch processing.Eligible queue insertion uses one UserDefaults write per fetch batch, first-delivery state persists once per record, and the JSON store is consulted on every identified-user fetch; HTML rendering paths are otherwise unchanged.
Known pre-existing limitation: the normal in-app list on disk has no identity label, so a hard process kill between a user switch and queued cleanup can make the previous list load as the current user until the next sync; in-process reads are identity-gated, and the identity-labeled JSON-only queue does not have this gap. Scope is frozen at this revision; further non-regression findings are tracked as follow-up tickets.
Testing
./agent_build.sh../agent_test.sh.The local full run is green with unit 693, notification-extension 12, offline-events 96, and 0 failures; CI can flake on three unrelated SDK-99 asynchronous
disablePushtests that predate this change.Fix tests verify post-mutation notification ordering and no notification for non-inbox removal. A 37-test
InAppTestssuite covers persistence before signaling, ordering, fingerprint acknowledgement and readmission, cold-start recovery, replay, deduplication, identity races, display gates, cross-type ID reuse, consume failures, expiry, retention, capacity, tombstones, and batch persistence. Objective-C compile tests cover both the legacy conformer and the new API surface.