[SDK-547] Reduce excess JWT auth token requests (timer race + crypto-timeout wipe) - #1077
Open
franco-zalamena-iterable wants to merge 3 commits into
Open
[SDK-547] Reduce excess JWT auth token requests (timer race + crypto-timeout wipe)#1077franco-zalamena-iterable wants to merge 3 commits into
franco-zalamena-iterable wants to merge 3 commits into
Conversation
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>
rtlsilva
requested changes
Aug 17, 2026
Comment on lines
+315
to
+317
| synchronized (IterableAuthManager.this) { | ||
| isTimerScheduled = false; | ||
| } |
There was a problem hiding this comment.
Because this unconditionally resets isTimerScheduled, an already running task can overwrite the flag after clearRefreshTimer() installs a replacement timer, allowing duplicate scheduling.
Suggest replacing the flag with the actual scheduled TimerTask - to ensure stale tasks cannot alter replacement state, the task should clear/release ownership, not perform an unconditional state reset itself.
| // force a re-login (and a new auth-token request) on every slow launch. Return null for | ||
| // this read; the encrypted value stays intact for the next attempt. (SDK-547) | ||
| IterableLogger.w(TAG, "Crypto operation timed out; keeping encrypted data for retry.") | ||
| null |
There was a problem hiding this comment.
If this returns null on timeout, is that null assigned to the auth token?
If it is, then a decryption timeout can still cause a refresh to be scheduled, which means it doesn't really reduce JWT auth token requests.
Suggest not representing both missing credentials and a timeout with null - retry reading the credentials after a timeout, but only request a new JWT after a successful read confirms that the token is genuinely absent.
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
2 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 two Android-specific defects that inflate
IterableAuthHandler.onAuthTokenRequested()calls (and backend JWT generation), reported by a customer seeing ~4x more Android JWT requests than iOS (SDK-547).Fix 1 — Synchronize JWT refresh timer scheduling (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 the oneTimerit holds, so extra timers leak uncancellable and each keeps requesting tokens. Scheduling and clearing are nowsynchronized, and the flag is set before scheduling.Fix 2 — Don't wipe credentials on a transient crypto timeout
IterableKeychainruns crypto under a 500 ms timeout. A slow AndroidKeyStore operation that exceeded it was caught in the same branch as a genuine decryption failure — wiping the stored email, userId, and auth token and disabling encryption — forcing a re-login and a fresh token request on the next launch. Crypto timeouts are now handled as transient: the encrypted data is preserved and only the current read/write falls back, without wiping credentials or disabling encryption. Genuine decryption failures still wipe as before.Testing
Scope note
These are the two defects we could prove reduce request volume. The exact ~4x production ratio is a fleet-scale / device-dependent effect; full attribution needs backend request segmentation + decryption-failure telemetry (to be tracked separately). So we could not 1-1 reproduce it with a long lived token even with different configurations. Worth asking client what is their Iterable setup to try to replicate what they see on our side