[SDK-566] Accept fractional seconds for expiringAuthTokenRefreshPeriod - #1079
Open
franco-zalamena-iterable wants to merge 2 commits into
Open
Conversation
Android only accepted whole seconds while iOS, React Native and Flutter accept fractional ones, so the same configuration value could behave differently per platform. Add a double overload and deprecate the Long one, which now delegates to it. Existing callers keep compiling. The setter also accepted any value unguarded. Because the period is subtracted when computing the refresh time, a negative value scheduled the refresh after the token had already expired, a very large value overflowed to a negative period with the same effect, and null threw an NPE on unboxing. Values carrying no usable intent (null, NaN, negative) now fall back to the 60s default; an excessive period still expresses an intent, so it is clamped to a ceiling. Logged rather than thrown. Also rename the internal carriers to ...Millis so the seconds-in, milliseconds-stored split is explicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop four tests that re-covered behaviour already guarded elsewhere or never changed: whole-second conversion (covered via the deprecated Long overload's delegation), 90.25s sub-second precision (covered by 0.5s), Long.MIN_VALUE (delegates to the double path's negative guard), and the RetryPolicy interval conversion, which this branch only renamed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rtlsilva
requested changes
Aug 12, 2026
Comment on lines
+363
to
+364
| * Invalid values are logged rather than throwing. Meaningless values fall back to the 60 | ||
| * second default ({@code null}, {@code NaN}, negatives); values above ~10 years are clamped |
There was a problem hiding this comment.
There seems to be a mismatch between docs and behavior - the isNaN / negative branches return this without assigning the field, so the field only equals the 60s default if it was never set earlier in the builder chain: if I wrote setExpiringAuthTokenRefreshPeriod(30.0).setExpiringAuthTokenRefreshPeriod(-60.0), the field would still be 30000ms.
Non-blocking, but probably worth a precision rewording in the javadoc and changelog to "Invalid values are ignored" or similar if this is the intended behavior - if it is not, we'd need to change it so that the value gets reset back to the default in those branches, matching the current docs.
Probably should add a test to exercise this set-then-invalidate case either way.
| ### Fixed | ||
| - Fixed a race in JWT auth token refresh scheduling that could leave multiple overlapping refresh timers running. When the refresh timer, an app foreground, and a 401 retry raced to schedule a refresh, the non-atomic timer guard let each create its own timer; the orphaned timers could not be cancelled and each kept requesting new auth tokens, inflating the number of `IterableAuthHandler.onAuthTokenRequested()` calls (and backend JWT generation) over time. Scheduling and clearing of the refresh timer are now synchronized so only one refresh timer is ever active. | ||
| - Fixed the keychain treating a transient crypto timeout as a permanent decryption failure. A slow AndroidKeyStore operation that exceeded the 500 ms timeout would wipe the stored email, userId, and auth token and disable encryption, forcing the user to re-authenticate (and request a new auth token) on the next launch. Crypto timeouts are now handled as transient without wiping credentials or disabling encryption for the device: a read that times out returns no value for that call (the stored ciphertext is left intact for the next attempt), and a write that times out stores that one value unencrypted (as the non-encrypted fallback already did) rather than clearing everything. The timed-out crypto operation is also cancelled so it no longer blocks subsequent reads/writes. | ||
| - `setExpiringAuthTokenRefreshPeriod` now validates its input instead of silently producing a broken refresh schedule. Previously a negative value was converted to a negative millisecond period and then *subtracted* when computing the refresh time, scheduling the refresh after the token had already expired; a very large value overflowed to a negative period with the same effect; and `null` threw a `NullPointerException` on unboxing. Invalid values are now logged and corrected — `null`, `NaN` and negative values fall back to the 60 second default, and values above ~10 years are clamped to that ceiling. Zero remains valid and means the token is refreshed only once it has expired. |
There was a problem hiding this comment.
null,NaNand negative values fall back to the 60 second default
See related comment
| static final long DEFAULT_EXPIRING_AUTH_TOKEN_REFRESH_PERIOD_SECONDS = 60L; | ||
|
|
||
| /** | ||
| * Ceiling for {@link Builder#setExpiringAuthTokenRefreshPeriod(Long)}, in seconds (~10 years). |
There was a problem hiding this comment.
This javadoc links to the deprecated Long overload, pointing to the double one would be more apt.
Suggested change
| * Ceiling for {@link Builder#setExpiringAuthTokenRefreshPeriod(Long)}, in seconds (~10 years). | |
| * Ceiling for {@link Builder#setExpiringAuthTokenRefreshPeriod(double)}, in seconds (~10 years). |
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
🎟️ Jira Ticket: SDK-566
📖 Description
SDK-566 states that Android interprets
expiringAuthTokenRefreshPeriodin milliseconds while the other SDKs use seconds, and asks for a breaking unit change plus a coordinated version bump.Android already uses seconds. Verified against all four codebases:
Builder.setExpiringAuthTokenRefreshPeriod(...)* 1000Linternally)config.expiringAuthTokenRefreshPeriod: TimeIntervalIterableConfig.expiringAuthTokenRefreshPeriodIterableConfig.expiringAuthTokenRefreshPeriodThe
60000Lthe ticket cites is the Builder's internal millisecond representation of 60 s, not a public seconds-valued default. The* 1000Lconversion has been there since 2020 (090bff97f,2cc5f9800).Corroboration: the RN and Flutter bridges pass seconds straight into the Android setter. If the premise were true, every hybrid app would already have a 1000x bug.
So making the requested change would introduce the bug the ticket aims to prevent. It is deliberately not done here. No breaking change, no coordinated version bump.
What the real gap was
Android accepted whole seconds only (
Long), while iOS/RN/Flutter accept fractional seconds.0.5was not expressible. That is a genuine parity gap, and it is what this PR fixes.1. Fractional seconds (
Added)New
setExpiringAuthTokenRefreshPeriod(double)overload. TheLongoverload is deprecated and delegates to it, so existing callers are unaffected — source and binary compatible. Overload resolution verified for120L, a boxedLong, a bareint, and0.5; none are ambiguous.2. Input validation (
Fixed) — two real bugs, both reproduced:-60became-60000ms, and the period is subtracted when computing the refresh time, so the refresh was scheduled 60 s after the token had already expired.Long.MAX_VALUE * 1000Lwraps to-1000, same past-expiry path.@NonNull Long.Now logged and corrected in the SDK's existing log-and-continue style (throwing would be a new crash surface for an init-time setter):
null/NaN/negative fall back to the 60 s default; values above ~10 years are clamped. Zero stays valid — it means "refresh once expired" — and is covered by a test so it isn't "fixed" later.3. Internal naming (
Changed)The core confusion is that one name meant seconds publicly and milliseconds internally. Package-private carriers renamed to
...Millis(IterableConfig,IterableAuthManager,RetryPolicy). Allfinal/package-private — invisible outside the SDK. Public setter names and parameters are untouched.Follow-ups (not in this PR)
optLong, so12.7truncates to12while iOS keeps12.7. Fix is committed onfeature/SDK-566-rn-fractional-refresh-periodbut cannot compile until aniterableapirelease carries thedoubleoverload — RN pins3.6.2, which publishes only theLongoverload (javac:double cannot be converted to Long). Sequenced after this ships.Extensions.ktsuggests it truncates the same way.