Skip to content

feat: [SDK-5376] persistent queue and retry for sessions API requests - #1766

Closed
fadi-george wants to merge 2 commits into
fadi/sdk-5375from
fg/sdk-5376-sessions-persistent-queue-retry
Closed

fadi-george wants to merge 2 commits into
fadi/sdk-5375from
fg/sdk-5376-sessions-persistent-queue-retry

Conversation

@fadi-george

@fadi-george fadi-george commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

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) and OSSessionRequestQueue in OSCore, persisted to OSUD_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.
  • Ordering: one request in flight at a time. Only the first queued request for a session can go out, so it's always create, then updates, then end. A create waits until it has a onesignal_id and push subscription_id. An update waits until its create returns the backend session ID.
  • IDs: missing IDs are resolved at send time through 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 writes serverSessionId back into the current record.
  • Coalescing at enqueue: queued updates for a session collapse to the highest duration, an end replaces them, and an update that arrives after a queued end is discarded. An update with no create and no server session ID is discarded. In-flight requests are left alone.
  • Cap: 100 requests. Past that the oldest is dropped with a warning, along with its updates if it was a create.
  • Retry: uses the retry/drop classification from feat: [SDK-5375] add sessions API client #1763. Exponential backoff with jitter (5s doubling, max 5m), and Retry-After is 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.
  • Wake-ups: retryNow() ends the backoff early but still waits out a Retry-After. It's called on app focus, when the network comes back (NWPathMonitor, only running while requests are pending), and from OSSessionService.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.
  • Flag off: enqueueCreate/enqueueUpdate do nothing unless record.usesSessionsApi is true.
  • Not in this PR: enqueueing from the session lifecycle, so nothing is enqueued yet.

Testing

Unit testing

OSSessionRequestQueueTests covers:

  • persist before send, and removal on success
  • flag off enqueues nothing
  • create waits for its IDs
  • update waits for its create and uses the server session ID
  • an update enqueued after the create returned picks up the server ID
  • a create is never enqueued twice
  • coalescing to the highest duration, end replaces update, and updates after an end are discarded
  • an in-flight update isn't coalesced
  • orphan updates are discarded
  • sessions are coalesced independently
  • the cap drops the oldest create along with its updates
  • the idempotency key is kept across a restart
  • exponential backoff, and retryNow ending it early
  • Retry-After honored by retryNow
  • network return triggers a retry
  • the network monitor only runs while requests are pending
  • drop after 5 answered failures, while no-response failures don't count
  • a 4xx drops the create along with its updates
  • an earlier session's create doesn't set the current record's server ID

The UnitTestApp_TestPlan_Reduced plan 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

  • 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

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

Final pass

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

🤖 Generated with Claude Code

@fadi-george
fadi-george added this pull request to stack #1765 October 8, 2026 20:33
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george
fadi-george force-pushed the fg/sdk-5376-sessions-persistent-queue-retry branch from b4b70c3 to 83522a1 Compare October 8, 2026 20:39
@fadi-george
fadi-george marked this pull request as ready for review October 8, 2026 20:42

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Open in Web View Automation 

Sent by Cursor Automation: PR Reviews

Comment on lines +55 to +57
@objc public static func reset() {
lock.withLock { _shared = nil }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added resetAndClearStoredQueue, called from handleAppIdChange. It retires the old instance first, so a late completion cannot write the key back.

Comment on lines +128 to +131
let isCreated = record.serverSessionId != nil || self.serverSessionIds[record.sessionId] != nil
guard !isCreated, !self.requests.contains(where: { $0.isCreate && $0.localSessionId == record.sessionId }) else {
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +176 to +186
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)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unchanged body now keeps the queued request and its key. Failed attempts carry over either way.

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, max(failedAttempts) carries over when the duration goes up too.

idempotencyKey: request.idempotencyKey,
directAttributionId: directAttributionId
)
inFlightKey = request.idempotencyKey

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tearDown now calls OSSessionRequestQueue.resetAndClearStoredQueue().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george

fadi-george commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed dd918f4. Also from the summary:

  • A drop at the attempt limit still backs off when there is a Retry-After.
  • The queue processes as soon as it loads, so saved requests replay at launch.
  • Not changed: no delay after a create before the first PATCH. Happy to add one if the backend can 404 a session it just accepted.
  • Not changed: requests that never become ready for other reasons still leave only through the 100 cap. Identity Verification was the case that can never resolve.
  • import Network autolinks in Swift. The simulator build passes locally. I have not checked Catalyst, so CI will cover it.

@fadi-george

Copy link
Copy Markdown
Collaborator Author

Moved to #1768 on the SDK-5379 branch. Same code, including the review fixes.

@fadi-george fadi-george closed this Oct 8, 2026
An error occurred while trying to automatically change base from fadi/sdk-5375 to new-session-events October 9, 2026 20:20
@fadi-george
fadi-george deleted the fg/sdk-5376-sessions-persistent-queue-retry branch October 9, 2026 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants