Repository navigation
Restore the session without decrypting its token - #1210
Merged
Merged
Conversation
The startup session restore only needs to know whether a token is stored, but it read and decrypted it, so the main thread (blocked in StartupInitializer's runBlocking) waited on the Android Keystore. Check for the token with TokenStorage.has instead. Closes #1190
Generated by 🚫 Danger Kotlin against 7f2dc46 |
Collaborator
Code Coverage
|
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
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.
On a signed-in cold start, the session restore no longer reads (and decrypts) the stored token, so
CampfireApplication.onCreatedoesn't wait on the Android Keystore at all. It only checks that a token is stored.Closes #1190
Where the issue stands after #1185
The issue's measurements (54 main-thread
keystore2calls, 115–132 ms on a Galaxy A51) were taken on builds based oneb7b2075, before #1185 landed. They came from the eagerEncryptedSharedPreferencesproviders, which no longer exist. #1185 already covers most of the issue's suggestions:toSuspendSettings(io);security-cryptois only used to read the legacy files.On current
mainthe main thread makes no Keystore calls during startup. It does still wait on them:StartupInitializerruns the restore underrunBlocking, andDatabaseUserSessionRestorercalledgetToken(). That one call loads the Keystore key and decrypts both the access and refresh tokens on the IO dispatcher while the main thread blocks. The value was thrown away: the restore only checked that it wasn't null. The Ktor auth provider then decrypts the token again for the first request.What changed
TokenStorage.has(userId)(SecureTokenStorage:hasKeyon the access-token key). It reads nothing, so it costs no decryption.DatabaseUserSessionRestorertakesTokenStorageinstead ofAccountManagerand useshas()to choose betweenLoggedInandNeedsAuthentication.Behavior change in one edge case: a token that is stored but can no longer be decrypted (its Keystore key is gone) used to send the restore straight to re-authentication. Now the restore reports
LoggedIn. The first request then goes out without a token and gets a 401. The refresh is also rejected with a 401 (ABS answers every refresh failure with 401), andinvalidateAccountmoves the session to re-authentication. On device, the app shows cached Home briefly, then the Reauthenticate screen with "'demo' was logged out and requires reauthentication". While the server is unreachable, the user keeps their cached and downloaded content until it comes back.The first launch after upgrading from a pre-#1185 version still migrates the legacy store before answering
has(). That's a one-time cost, and it has to finish before the restore can tell whether a token exists.Verification
:data:account:impl:jvmTest: newDatabaseUserSessionRestorerTest(in-memory DB: signed out without a current user; signed out andcurrentUserIdcleared when the account row is gone;LoggedInwith a stored token;NeedsAuthenticationwithout one) andSecureTokenStorageTest(has/ round trip). The restorer tests use aTokenStoragewhosegetthrows. Changing the restorer back toget() != nullfails both token tests.:data:account:impl:compileTestKotlinIosSimulatorArm64,./scripts/ktlint --check, and:app:desktop:compileKotlin :app:android:compileAlphaDebugKotlin :app:ios:compileKotlinIosSimulatorArm64 jvmTest test: all pass. I excluded:infra:audioplayer:engine-tests:test: it needs the native ffmpeg/libvlc libraries and a login to the personal test server, which was down, and this change doesn't touch it.Perfetto,
fossBenchmarkReleaseon the testbed emulator (android-36, signed in to the throwaway server, fully AOT-compiled, 8 cold starts each).keystore2binder transactions from the app process:mainUserComponentslice), on an IO threadonCreateThe emulator's Keystore is software-backed, so those 11 calls cost only ~2.4 ms there and its timings were within noise.
DiStartupBenchmarkson the Galaxy A51 (SM-A515U, Android 13, the issue's device;fossBenchmarkRelease, fully AOT-compiled, 20 cold starts per run). The prefilled test server was down (Cloudflare 523), so the benchmark signed in to the testbed server throughadb reverse, using-Poverrides of the test credentials. Medians from the traces, IQR in brackets. Both columns ran on the charger, back to back:mainonCreateonCreateUserComponent)onCreate→ first frameandroid_startups)timeToInitialDisplayMskeystore2calls during the restoreAn earlier
mainrun on battery agrees ononCreate(182.2 ms, 168–202), so theonCreatesaving is clear. The phase afteronCreatedoesn't grow, so the decrypt that moved to the first request (on a background thread) doesn't stall the main thread later. The cold-start gain looks real but is within this device's run-to-run variation: the twomainruns differed by 23 ms.Device, edge case: on the testbed with root, the stored ciphertexts were overwritten with Base64 that doesn't authenticate, then the app was cold-started. This build: Home, then Reauthenticate (above). Same experiment on
main: straight to re-authentication. Both builds then crash withIllegalArgumentException: Required value was nullinKtorAudioBookShelfApi.getCurrentUser→requireServerUrl, with identical stacks. That crash is pre-existing and already in Crashlytics for 1.2.0 (issue4318e55f), after a rejected refresh token. It needs its own issue; this PR neither causes nor fixes it.Changelog
[Android] Slightly faster startup when signed inunder Changed.🤖 Generated with Claude Code