[SDK-547] Reduce excess JWT auth token requests (timer race) - #1077
Merged
franco-zalamena-iterable merged 11 commits intoAug 21, 2026
Conversation
rtlsilva
requested changes
Aug 17, 2026
franco-zalamena-iterable
changed the title
franco-zalamena-iterable
force-pushed
the
feature/SDK-547-jwt-timer-race
branch
from
August 19, 2026 12:57
8e672fc to
07e7137
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
franco-zalamena-iterable
changed the title
franco-zalamena-iterable
changed the title
rtlsilva
self-requested a review
August 20, 2026 17:52
rtlsilva
requested changes
Aug 20, 2026
The refresh timer, app foreground, and 401 retry paths all call scheduleAuthTokenRefresh from different threads. The isTimerScheduled guard was a non-atomic check-then-act: concurrent callers could all pass it and each schedule a TimerTask, and each foreground reschedule could create a new Timer while orphaning the previous one. Orphaned timers could not be cancelled by clearRefreshTimer and kept firing onAuthTokenRequested, inflating backend JWT generation over time. Make scheduleAuthTokenRefresh and clearRefreshTimer synchronized, and set isTimerScheduled before scheduling (reset on failure) so only one refresh timer is ever active. Adds a concurrency test asserting a single timer under 8 racing callers, and a test asserting foregrounding with a valid, far-from-expiry token does not request a new token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
IterableKeychain decrypts/encrypts the stored auth token under a 500ms timeout. A slow AndroidKeyStore operation that exceeded it was caught in the same branch as a genuine decryption failure, which wipes the stored email, userId, and auth token and permanently disables encryption. That forces a re-login and a fresh onAuthTokenRequested on the next launch — on slower devices this can recur, inflating backend JWT generation. Handle TimeoutException separately from real crypto failures: on a timeout, preserve the encrypted data and fall back only for the current read/write (plaintext on save), without wiping credentials or disabling encryption. Genuine decryption errors still wipe as before. Adds a test asserting a crypto timeout does not wipe credentials, disable encryption, or invoke the decryption-failure handler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up. runWithTimeout ran crypto on a single-thread executor and, on timeout, left the Future running — a slow/hung AndroidKeyStore operation kept occupying the only worker thread, so every subsequent read/write queued behind it and also timed out. Cancel the Future on timeout (interrupting the task if interruptible) to free the thread. Also clarify the CHANGELOG to note that a write timeout stores that one value unencrypted (the existing non-encrypted fallback), rather than implying nothing is written. Adds a test asserting a slow crypto op that times out does not block the next read (fails without the cancel). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The auth manager submitted token requests to a privately constructed executor, so tests could not observe or order them; 17 tests in IterableApiAuthTests are @ignore'd for exactly this reason. Make the executor injectable so a test can drain it deliberately. Adds 20 tests covering token creation, replacement, recovery after failure, foreground/background transitions, and concurrent scheduling (the regression net for the timer race fixed in 866845f). The tests queue work instead of running it inside submit(): the SDK holds the auth manager's monitor across executor.submit(), and the submitted task re-enters that monitor via queueExpirationRefresh, so any same-thread executor deadlocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep timed-out token reads distinct from confirmed absence and retry them off the caller thread without minting a replacement token on exhausted reads. Track the pending refresh task itself, attach named reasons to refresh decisions, and prevent stale restore or timer work from overtaking identity changes.
An auth token can arrive from two async sources that both outlive the identity that asked for them: the blocking onAuthTokenRequested callback and an encrypted-storage read. Neither store site checked who was signed in, so a token minted for the previous user could be installed on the current session, and a second initialize() left an unreachable restorer that could overwrite a fresh login's token. Track the signed-in identity in an AtomicReference and compare it at both store sites. Atomics rather than a lock: IterableAuthDataRestorer invokes its callback while holding its own monitor and that callback re-enters this class, so taking the manager's monitor around a restore would invert the established restorer -> manager order. The handler snapshot is taken immediately before onAuthTokenRequested, not at submit time. An identity change before the handler runs is served by that request, because pendingAuth makes the new login reuse it rather than start its own; discarding there would leave the new user with no token and nothing in flight. A discarded result still clears pendingAuth and replays any refresh deferred behind it, since both are otherwise released only by a stored result. Also gate useExplicitAuthToken on the token actually changing. Its caller compares by reference, so an equal-but-distinct String - a token read from disk or JSON - reached it on every repeated login and replaced the refresh timer while downgrading INVALID to UNKNOWN, marking a token ready that a 401 had just rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IterableApi is the SDK public entry point and grows with the API surface, so the 2000-line default is fighting the architecture rather than protecting anything. Suppress the check for that one file instead of raising the limit, so every other file keeps the guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
franco-zalamena-iterable
force-pushed
the
feature/SDK-547-jwt-timer-race
branch
from
August 21, 2026 14:15
2c6adb9 to
7b19e5e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses an Android-specific race that can inflate
IterableAuthHandler.onAuthTokenRequested()calls and backend JWT generation, reported by a customer seeing ~4x more Android JWT requests than iOS (SDK-547).Fix — One refresh timer at a time (race)
scheduleAuthTokenRefreshused a non-atomicisTimerScheduledcheck-then-set. The refresh timer (timer thread), app foreground (main thread), and 401 retry (network thread) can hit it concurrently; each could pass the guard and schedule its ownTimerTask.clearRefreshTimer()only cancels theTimerit holds, so extra tasks could remain active and each request another token.Scheduling and clearing are now synchronized, and the scheduler tracks the exact
TimerTaskthat owns the refresh slot instead of a boolean. Only the owning task may fire or release that slot. A stale task that runs after cancellation cannot clear or execute in place of its replacement. Duplicate scheduling continues to retain the first scheduled task, callback, and retry policy.Refresh call sites now provide an
IterableAuthRefreshReason. Structured logs record each schedule, skip, fire, cancellation, stale-task rejection, and scheduling error with that reason.Testing
IterableAuthRefreshOwnershipTest— 5 deterministic tests covering eight-thread concurrent scheduling, stale-task ownership, duplicate callback and policy retention, scheduling-failure cleanup, and lifecycle refresh behavior while failure retries are paused.iterableapiunit suite passed locally../gradlew :iterableapi:checkstylepassed.git diff --checkpassed.Scope note
This PR is intentionally limited to the refresh timer ownership race and its observability. It does not change public APIs, callback contracts, existing retry priority, crypto timeout handling, stored-token restoration, or identity coordination. Those concerns should be handled independently so any intended behavior changes can be reviewed and tested separately.
The exact ~4x production ratio is a fleet-scale and device-dependent effect, and was not reproduced 1:1 with a long-lived token across configurations. Full attribution still requires backend request segmentation and customer configuration details.
One known related divergence is deliberately not included: when a token is already inside its refresh window, Android schedules a retry at
getNextRetryInterval()while iOS waits for the next foreground or API call. Choosing the canonical behavior is a cross-platform product decision and should remain separate from this concurrency fix.