Repository navigation
fix: waiting token refreshes re-read the token instead of spending a stale copy #150 - #151
Merged
Merged
Conversation
wmcparla
marked this pull request as ready for review
November 15, 2026 03:26
Member
|
Thanks, merged. The other race you mention (an app refresh writing its result over a newer stored bundle) is still open. With Stalwart it's harmless, since the one-rotation-old token returns the same successor. |
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.
Summary
Android signs OAuth accounts out when the app wakes after the push handler has refreshed the token twice (#150). Several requests call
ensureFreshToken()at once; the first takes the stored bundle, and the others refresh with the in-memory copy they read before awaitingadoptStoredTokens(). That token is two rotations old, and Stalwart revokes the whole grant when it comes back.With this change every waiting caller reads the in-memory tokens again after
adoptStoredTokens(), whatever it returned, and refreshes only if the token it finds is still due.Changes
All in
src/api/jmap-client.ts.ensureFreshToken()re-readscurrentOAuthTokens()afteradoptStoredTokens()instead of only when it returned true. A caller that lost the race to another one now finds the adopted tokens and returns.forceRefreshToken(): when another caller took newer tokens while this one waited, it returns true so the request is retried with them, instead of refreshing a token that is good. This one only cost an extra rotation, not a sign-out.expiresAtis not refreshed, as at the top of the function (an account switch while waiting could otherwise reach the refresh).Related issues
Closes #150
Type of change
Checklist
npm run typecheckand there are no errors (native has nolintscript)npm run buildin native; an arm64 + x86_64 release APK,assembleGithubRelease -PallowDebugSignedRelease=true, builds and runs)locales/) if my changes affect user-facing text (no user-facing text)Tests:
npx vitest run: 7105 passed, 163 skipped (the live Stalwart suites), 0 failed. Two new cases insrc/api/__tests__/jmap-client-hardening.test.ts:ensureFreshToken()calls on a token two hours stale while storage holds a newer bundle: none refreshes (on main, six do);forceRefreshToken()calls in the same situation: both return true without refreshing (on main, the second refreshes).Tested on the Android 16 emulator against a self-hosted Stalwart 1.1.0 with the steps in #150 (OAuth sign-in, battery Restricted, two push-handler refreshes an hour apart by moving the clock, then open the app), using builds with token logging added:
The same build has also been running on a Galaxy S25 (One UI 8.5), where the race was first captured.
Not tested: iOS (no Mac here). The iOS notification extension also refreshes and writes back, so the same race should apply there and the same fix covers it.
Notes for reviewers
invalid_grantand the grant is revoked. So a refresh whose answer is lost is harmless; only a copy two rotations old signs the account out. The comment abovetakes tokens stored just after its own refresh was refusedassumes the server refuses the one-rotation case, which Stalwart does not, but that path still works.persistRefreshedTokens()writes its result over memory and storage without checking that its input is still current. If the push handler stores a newer bundle while an app refresh is in flight, the older result can overwrite it. I have not seen this happen; it would need its own change (persist only when the refreshed token is still the current one).🤖 Generated with Claude Code