Repository navigation
feat: [SDK-5492] send sessions requests outside OperationRepo - #2785
fadi-george wants to merge 1 commit into
Conversation
Sessions API requests get their own persisted queue, so a sessions outage never delays user or subscription operations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
📊 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)288/298 touched executable lines covered (96.6% — requires ≥ 80%) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review
Claude Opus 5.5, GPT 5.6 Sol, Grok 4.7. 10 files, +1433/−13 = 1446 (all Kotlin source/tests).
Act on (3/3): onCreated removes the persisted create before SessionModel.serverSessionId is written on serial IO. createdSessionIds is process-local. A kill in that window leaves the next end with no server ID and no queued create, so admitUpdate drops it as never-created.
Act on (3/3): The session does not pin appId. Ends use live config.appId; applyResolvedAppId does not clear session_requests. After an app-id change the end is orphaned against the old create, or PATCHed to apps/<new>/sessions/<old>.
Act on (3/3): A create 404 retries only when onesignalId is in the missing window. An existing user with a just-created subscription drops on the first 404; drop then deletes that session's queued end.
Consider (2/3): Local→backend translation is runOnSerialIO after the identity/config persist. A crash leaves queued local IDs; nothing reconciles them on start.
Consider (2/3): endSession runs off serial IO. A focus during the job can end the wrong or newly started session. Grok: isValid = false and activeDuration = 0 persist before the end is stored, so a kill there loses the end.
Consider (Opus): retryNow on every focus burns persisted failedAttempts, so five app opens during a sessions outage drop the queue.
Consider (2/3): A sessions Retry-After still sets HttpClient.delayNewRequestsUntil, so a 429 still blocks OperationRepo.
Consider (Opus): complete() holds the queue lock; BackgroundManager is Unconfined, so enqueueAndWait can resume under that lock.
Consider (Grok): Cap eviction can drop an in-flight create's queued end.
Noted: Author gaps (IV never-ready until cap; login-before-create). Coalesce unused in production (create+end only). 30s wait holds later background services. NewRecordsState process-local/unbounded. Uncapped Retry-After. Consent 0 retries forever. processForever is not restarted if the loop throws outside send.
Dismissed: OEM callback SecurityException (caught). SessionListener TrackSession* dual-write (out of this PR). Translate-under-lock persist order looks consistent.
Sent by Cursor Automation: PR Reviews
| private fun onCreated( | ||
| create: SessionRequest, | ||
| serverSessionId: String, | ||
| ) { | ||
| remove(create) | ||
| complete(create.id, sent = true) | ||
| newRecordsState.add(serverSessionId) | ||
| createdSessionIds[create.localSessionId] = serverSessionId | ||
| store.list().filter { !it.isCreate && it.isSameSession(create) }.forEach { it.serverSessionId = serverSessionId } | ||
|
|
||
| // Serialized with SessionService, which clears the server ID when the next session starts. | ||
| runOnSerialIO { | ||
| val session = sessionModelStore.model | ||
| if (session.sessionId == create.localSessionId) session.serverSessionId = serverSessionId | ||
| } |
There was a problem hiding this comment.
Create success is removed from the store here, then SessionModel.serverSessionId is written later on serial IO. createdSessionIds is process-local. If the process dies in between, the next end has no server ID and no queued create, so admitUpdate discards it as never-created. Persist the local→server mapping (or keep the create) before removing this row.
| Logging.debug("SessionService.backgroundRun: Session ended. activeDuration: $activeDuration") | ||
|
|
||
| session.isValid = false | ||
| val end = if (session.usesSessionsApi) session.toEndRequest(config?.appId) else null |
There was a problem hiding this comment.
This end uses live config.appId. The session does not pin an app id, and applyResolvedAppId does not clear session_requests. After an app-id change the end either fails isSameSession against the old create and is dropped as orphaned, or PATCHes apps/<new>/sessions/<old>.
| ex: BackendException, | ||
| ) { | ||
| val code = ex.statusCode | ||
| val missingId = if (request.isCreate) request.onesignalId else request.serverSessionId |
There was a problem hiding this comment.
A create 404 is retried only when onesignalId is in the missing window. hasAccessibleIds already waits on both user and subscription. For an existing user with a just-created subscription, the first 404 is dropped and drop then deletes that session's queued end. Check every id the request depends on (subscription for create; session id for update), matching SubscriptionOperationExecutor.
|
Closing in favor of fixing OperationRepo retry for all ops (SDK-5535). |
Description
One Line Summary
Send sessions API requests through their own persisted queue instead of
OperationRepo.Details
Motivation
OperationRepois one ordered queue, so a failing sessions request blocks every user and subscription operation behind it. The fixes on #2777, #2780 and #2781 each added a session-only special case to that shared queue. Sessions are telemetry: they need persistence and retry, but not the repo's ordering or ID translation.This replaces the session operations in #2777, #2780 and #2781.
Scope
SessionRequestQueue:Retry-After. Drops after 5 failures the backend answered; offline failures don't count. A 404 for a just-created record is retried.SessionServiceenqueues a create on session start and an end on session end. The background job waits up to 30s for the end to send. Existing local-to-backend ID translation also updates queued requests.NewRecordsStatemethods are synchronized, since the repo and the queue use it from different threads.OperationRepo.Known gaps
OperationRepo.Testing
Unit testing
SessionRequestQueueTests: ordering, coalescing, end handling, orphans, cap, retry with same key, drop after max attempts, non-retryable errors, offline retries, 404 retry window, replay after kill with the same key, and isolation (a stuck or backing-off request doesn't block others).SessionServiceTests: create/end requests, cold start end, timeout, and ID translation forwarding.ThreadUtils,DeviceServiceandOneSignalImpalready exist onnew-session-events.Manual testing
Not tested on a device yet.
Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code