[Payment due @nyomanjyotisa] Fix stuck isAuthenticatingWithShortLivedToken blocking auto-reauth#91633
Conversation
|
@nyomanjyotisa Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@allgandalf can you sync latest main to fix the jest |
|
@nyomanjyotisa can you please prioritize this pr? thanks |
There was a problem hiding this comment.
Pull request overview
Fixes a bug where session.isAuthenticatingWithShortLivedToken could persist as true in IndexedDB if a SignInWithShortLivedAuthToken request was interrupted between optimisticData and finallyData. The stuck value would rehydrate on subsequent loads and cause Reauthentication.ts to permanently abort all reauth attempts, leaving the user unable to recover from a 407 without clearing cache. The fix moves the flag to a new RAM-only Onyx key so an interrupted request cannot persist a stuck value, and previously stuck users recover automatically on next load.
Changes:
- Added new RAM-only Onyx key
RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKENand registered it insetup/index.ts. - Updated
Session/index.ts,Reauthentication.ts, andAppState/index.tsto read/write the flag via the new key, and removedisAuthenticatingWithShortLivedTokenfrom theSessionOnyx type. - Added two unit tests covering both the new RAM-only abort path and the recovery path for legacy persisted stuck flags.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ONYXKEYS.ts | Defines the new RAM-only key and adds its type mapping. |
| src/setup/index.ts | Registers the new key in ramOnlyKeys. |
| src/types/onyx/Session.ts | Removes the persisted isAuthenticatingWithShortLivedToken field. |
| src/libs/Reauthentication.ts | Reads the flag from the new RAM-only key instead of SESSION. |
| src/libs/AppState/index.ts | Captures the flag for logging via the new RAM-only key. |
| src/libs/actions/Session/index.ts | Writes optimistic/finally updates to the new RAM-only key instead of SESSION. |
| tests/actions/SessionTest.ts | Adds tests for the new abort path and legacy recovery path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reviewing now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS-Chrome.mp4
MacOS-Chrome-2.mp4 |
nyomanjyotisa
left a comment
There was a problem hiding this comment.
LGTM! The failing test is unrelated
|
🎯 @nyomanjyotisa, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
Restarted the test |
| function captureSessionState(): SessionStateInfo { | ||
| // Check multiple authentication states to get complete picture | ||
| const isSessionLoading = !!currentSession?.loading; | ||
| const isAuthenticatingWithShortLivedToken = !!currentSession?.isAuthenticatingWithShortLivedToken; |
There was a problem hiding this comment.
What if we update the app version and the user has isAuthenticatingWithShortLivedToken in the session and not in the ram only key? should we temporarily coalesce both values here to cover this case and then later remove the key from the session?
There was a problem hiding this comment.
i think coalescing them would actually re-trap the stuck users :)
the whole point of moving the flag to RAM-only is that we stop reading session.isAuthenticatingWithShortLivedToken. that persisted field IS the bug. currently-stuck users have it persisted as true in IndexedDB and the legacy code reads it back on every reload, blocking every reauth attempt. by ignoring it in the new code, they get unblocked on the next reload because the RAM-only key starts undefined.
if we OR both values here, a stuck user's session.isAuthenticatingWithShortLivedToken=true would still evaluate to true and reauth would keep aborting, which is exactly what we are trying to escape from.
the second unit test (reauthenticate proceeds even when a legacy session.isAuthenticatingWithShortLivedToken=true is persisted) covers this app-upgrade scenario: legacy stuck session field + undefined RAM-only key -> reauth proceeds normally.
also fwiw, this AppState file is just the diagnostic log captured when ActivityIndicator hangs. it isnt the actual reauth abort path, that lives in Reauthentication.ts. the log reflecting the new RAM-only state is correct (it shows what reauth actually reads, not the dead legacy value).
happy to add a follow-up cleanup migration to delete the leftover session.isAuthenticatingWithShortLivedToken from IndexedDB if you want it gone, but the fix here doesnt depend on it. WDYT?
There was a problem hiding this comment.
Nah I think we can leave it
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.99-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — no changes requiredI reviewed the changes in this PR against Why: This is a purely internal session/authentication bug fix with no user-facing surface:
I also searched Since no documentation changes are needed, I did not create a draft help site PR. @allgandalf, no linked help site PR was created because this change has no customer-facing documentation impact. If you believe a help article should be updated for this behavior, let me know which flow and I'll draft it. |
|
Hi @allgandalf. Can it be checked internally? |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.99-9 🚀
|
|
🤖 Payment issue created: #92796 |
Explanation of Change
session.isAuthenticatingWithShortLivedTokenwas a regular persisted Onyx field. when aSignInWithShortLivedAuthTokenrequest was interrupted between optimisticData (settrue) and finallyData (setfalse), the optimistic IndexedDB write was already committed and never got cleared. on every later page load the stucktruerehydrated, and the abort check in Reauthentication.ts blocked every reauth attempt, so the user could not recover from a 407 without "Clear Cache and Restart".moved the flag to a new RAM-only Onyx key
RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN. an interrupted request can no longer persist a stuck value, and anyone currently stuck recovers on the next page load because the new key starts undefined.full RCA (with logs) on the tracking issue: https://github.com/Expensify/Expensify/issues/637360#issuecomment-4466727707
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/637360
PROPOSAL: N/A (internal RCA, see linked comment)
Tests
main): 407 + "Your session has expired. Please sign in again." toastOffline tests
N/A. The offline write queue is not affected by this change.
QA Steps
Same as Tests above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari