Repository navigation
feat: [SDK-5376] persistent queue and retry for sessions API requests - #1766
fadi-george wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b4b70c3 to
83522a1
Compare
There was a problem hiding this comment.
Multi-model review (Opus 5.5, GPT 5.6 Sol, Grok 4.7) of the persistent sessions queue.
Act on (3/3): handleAppIdChange clears OSUD_SESSION_RECORD and the user stores, but not OSUD_SESSION_REQUEST_QUEUE or the live OSSessionRequestQueue singleton. Each request already has the old appId baked in, so those calls still go to the previous app after every other app-scoped blob was wiped. Requests waiting on IDs can also stall against the new user (they count toward the cap and keep NWPathMonitor running). clearStatics only nils _shared; it does not delete the blob. Add a resetAndClearStoredQueue next to resetAndClearStoredRecord.
Consider (2/3 Sol+Grok): OneSignalClient already retries status 0 / 5xx up to 5 times (5s / 15s / 45s / 135s) before the queue's completion runs. Five queue failedAttempts can be many more HTTP calls; inner retries ignore Retry-After; inFlightKey stays set so retryNow cannot wake the request. Same layering as #1763.
Consider (2/3 Opus+Grok): A create/update that never becomes ready (login before the push sub exists; identity gone; pin only inside readyRequest, which is skipped while another request is in flight or backing off) is never failed and only leaves via the 100 cap.
Consider (2/3 Opus+Grok): Coalesce replaces queued updates with a new UUID and failedAttempts = 0. A timeout the backend may already have applied loses its idempotency key; a session that keeps emitting duration updates never hits the 5-attempt drop and can pin the global backoff.
Consider (Grok): serverSessionIds is process-local and not rebuilt on load(). After a restart, admitUpdate / enqueueCreate do not read a sibling request that already has a server id, so a snapshot with serverSessionId == nil is treated as orphaned or can fire a second create with a new key.
Consider (Grok): No post-create delay; a 404 on the first PATCH is .drop, so the end/duration for a just-created session is discarded. Android #2777 waits 5s.
Noted: Create idempotency key is a fresh UUID, not derived from sessionId (Opus) — cap eviction / 5xx drop then re-enqueue can double-create. Fifth answered failure ignores Retry-After and processNexts immediately (Opus). notBefore is wall-clock, so a clock step can freeze or skip backoff (Grok). load() does not processNext() (Opus). First Network.framework use; relies on autolinking (Opus).
Dismissed: Update-after-end once the end is removed (Opus) — lifecycle should not enqueue after end. failedAttempts missing-key decode (Grok) — new type, no prior blob. refreshPinnedIds waking shared in unit tests (Opus) — test isolation only.
Sent by Cursor Automation: PR Reviews
| @objc public static func reset() { | ||
| lock.withLock { _shared = nil } | ||
| } |
There was a problem hiding this comment.
Act on (3/3): This only nils _shared. handleAppIdChange never calls it and never removes OSUD_SESSION_REQUEST_QUEUE, so the live queue (and the persisted blob) keep sending the previous app's requests after the session record and user stores were wiped. Pair a resetAndClearStoredQueue with resetAndClearStoredRecord.
There was a problem hiding this comment.
Added resetAndClearStoredQueue, called from handleAppIdChange. It retires the old instance first, so a late completion cannot write the key back.
| let isCreated = record.serverSessionId != nil || self.serverSessionIds[record.sessionId] != nil | ||
| guard !isCreated, !self.requests.contains(where: { $0.isCreate && $0.localSessionId == record.sessionId }) else { | ||
| return | ||
| } |
There was a problem hiding this comment.
Consider (Grok): serverSessionIds is memory-only and load() does not rebuild it. After a restart this guard is only the caller's record.serverSessionId, so a stale snapshot (create already succeeded and left the queue) can enqueue a second create with a new idempotency key. Rebuild the map from persisted requests that already carry a server id, and treat a sibling with that id as "already created".
There was a problem hiding this comment.
load() now rebuilds the map from queued updates that already have a server ID. The create key is also the session ID now, so a second create would be deduplicated anyway.
| let queued = session.filter { !$0.isCreate && $0.idempotencyKey != inFlightKey } | ||
| guard !queued.isEmpty else { | ||
| return true | ||
| } | ||
| if case .update(let activeDuration, let endTime) = request.kind { | ||
| let highest = queued.compactMap(\.activeDuration).max() ?? activeDuration | ||
| request.kind = .update(activeDuration: max(activeDuration, highest), endTime: endTime) | ||
| } | ||
| let removedKeys = Set(queued.map(\.idempotencyKey)) | ||
| requests.removeAll { removedKeys.contains($0.idempotencyKey) } | ||
| OneSignalLog.onesignalLog(.LL_DEBUG, message: "OSSessionRequestQueue coalesced \(queued.count) update(s) for \(request.localSessionId)") |
There was a problem hiding this comment.
Consider (2/3 Opus+Grok): Folding drops the queued updates and keeps this new request (failedAttempts = 0, new UUID). A timeout the backend may already have applied loses the key that was supposed to make the retry safe, and a session that keeps enqueueing durations never reaches maxFailedAttempts while holding the global backoff. Carry the existing key and max(failedAttempts) when the body is unchanged.
There was a problem hiding this comment.
An unchanged body now keeps the queued request and its key. Failed attempts carry over either way.
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Persist-before-send, the reused idempotency key, and per-session ordering look right.
Agree with the comment on reset(): an app-id change needs to clear OSUD_SESSION_REQUEST_QUEUE, and a late completion must not write that key back.
Inline: coalesced updates still need max(failedAttempts) when the duration changes, the shared client retries status 0 and 5xx under this queue, an anonymous create waits forever once Identity Verification is on, Retry-After follows the wall clock, the drop warning logs the whole request, and refreshPinnedIds() now builds the production queue singleton.
| } | ||
| if case .update(let activeDuration, let endTime) = request.kind { | ||
| let highest = queued.compactMap(\.activeDuration).max() ?? activeDuration | ||
| request.kind = .update(activeDuration: max(activeDuration, highest), endTime: endTime) |
There was a problem hiding this comment.
The note on the coalesced log covers an unchanged body. When this fold raises the duration, keep the new idempotency key and still copy max(failedAttempts) from the updates removed just below. A longer update otherwise restarts the drop counter, stays first, and the one-at-a-time queue keeps backing off behind it.
There was a problem hiding this comment.
Done, max(failedAttempts) carries over when the duration goes up too.
| idempotencyKey: request.idempotencyKey, | ||
| directAttributionId: directAttributionId | ||
| ) | ||
| inFlightKey = request.idempotencyKey |
There was a problem hiding this comment.
OneSignalClient retries status 0 and 5xx on its own (5s, 15s, 45s, 135s) before this completion runs. inFlightKey is already set, so retryNow() waits out that loop and then counts it as one attempt. Set reattemptCount on the session requests so willReattemptRequest returns false. 429 already skips that client loop.
There was a problem hiding this comment.
Session requests now start at the client's last attempt (reattemptCount = MAX_ATTEMPT_COUNT - 1), so OneSignalClient no longer retries under the queue.
| persist() | ||
| } | ||
| } | ||
| guard request.onesignalId != nil, request.subscriptionId != nil else { |
There was a problem hiding this comment.
With Identity Verification on, createUserIfNeverCreated never creates an anonymous user, so a create missing onesignalId stays here and the network monitor stays up. Drop that create and its updates once the requirement is .on. Leave it queued while the requirement is still .unknown.
There was a problem hiding this comment.
Added sessionUserCanBeCreated to the user provider. A create still missing onesignal_id is dropped with its updates only when the requirement is .on and its user has no external ID. .unknown leaves it queued.
| consecutiveFailures += 1 | ||
| let exponential = min(Self.maxBackoffSeconds, Self.baseBackoffSeconds * pow(2, Double(consecutiveFailures - 1))) | ||
| let retryAfter = TimeInterval(retryAfterSeconds ?? 0) | ||
| notBefore = now() + retryAfter |
There was a problem hiding this comment.
notBefore is absolute wall-clock time, and the timer below is monotonic. A backward clock step makes retryNow() wait out Retry-After plus the step. A forward step makes retryNow() send while that timer is still running. Store the deadline on the same clock as asyncAfter.
There was a problem hiding this comment.
now defaults to CLOCK_UPTIME_RAW now, the same clock DispatchTime uses.
| let oldest = requests.first(where: { $0.idempotencyKey != inFlightKey }) { | ||
| OneSignalLog.onesignalLog( | ||
| .LL_WARN, | ||
| message: "OSSessionRequestQueue more than \(Self.maxQueuedRequests) requests queued, dropping oldest: \(oldest)" |
There was a problem hiding this comment.
This interpolates the whole request, so onesignalId and subscriptionId are in the message. The remote sink uploads WARN when that level is enabled. The failure drops further down do the same. Log localSessionId, kind, and failedAttempts, as the other lines in this file do.
There was a problem hiding this comment.
Drop logs now use logDescription: kind, local session ID, and failed attempts.
| let ids = pinnableIds() | ||
| stateLock.withLock { fillPinnedIds(ids) } | ||
| // Queued session requests wait for these IDs, including those of an earlier session's user. | ||
| OSSessionRequestQueue.shared.retryNow() |
There was a problem hiding this comment.
This constructs OSSessionRequestQueue.shared against the same OSUD_SESSION_REQUEST_QUEUE key the new queue tests use. OSSessionServiceTests already calls refreshPinnedIds(). Reset the singleton and delete the key in that class's tearDown.
There was a problem hiding this comment.
tearDown now calls OSSessionRequestQueue.resetAndClearStoredQueue().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed dd918f4. Also from the summary:
|
|
Moved to #1768 on the SDK-5379 branch. Same code, including the review fixes. |


Description
One Line Summary
Add
OSSessionRequestQueue, a persisted queue for sessions API requests with idempotent retries, coalescing, backoff, and a size cap.Details
Motivation
Session requests need to survive restarts, process death, and offline periods, and be retried without double counting. Stacked on #1763 (sessions API client). Mirrors Android's OneSignal/OneSignal-Android-SDK#2777.
Scope
OSSessionRequest(Codable) andOSSessionRequestQueuein OSCore, persisted toOSUD_SESSION_REQUEST_QUEUE. A request is saved before it's sent and removed only on success or a non-retryable failure. Its idempotency key is generated once and reused on every retry, including after a restart.onesignal_idand pushsubscription_id. An update waits until its create returns the backend session ID.OSSessionService.pinnedIds. For the current session they come from the record, so the request and the record always match. Earlier sessions use the same pinning rules. A successful create writesserverSessionIdback into the current record.retry/dropclassification from feat: [SDK-5375] add sessions API client #1763. Exponential backoff with jitter (5s doubling, max 5m), andRetry-Afteris a floor. After 5 failed attempts that the backend answered, the request is dropped. No-response failures (code 0) don't count toward that, so offline requests keep retrying.retryNow()ends the backoff early but still waits out aRetry-After. It's called on app focus, when the network comes back (NWPathMonitor, only running while requests are pending), and fromOSSessionService.refreshPinnedIds(), which the User module already calls when a user is hydrated or the push subscription ID is set. Unlike the user executors, a failed request is retried in the same run instead of waiting for relaunch.enqueueCreate/enqueueUpdatedo nothing unlessrecord.usesSessionsApiis true.Testing
Unit testing
OSSessionRequestQueueTestscovers:retryNowending it earlyRetry-Afterhonored byretryNowThe
UnitTestApp_TestPlan_Reducedplan passes locally (664 tests).SwitchUserIntegrationTests.testAnonUser_thenIdentifyUserWithConflict...flakes now and then, and it does the same on the base branch.Manual testing
Not done. Nothing enqueues requests until the session lifecycle is wired up.
Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code