Apply the sign-in response on arrival - #97690
Conversation
A SAML sign-in returns jsonCode 200 with a new session. When the client is behind on Onyx updates, SaveResponseInOnyx parks the whole response in ONYX_UPDATES_FROM_SERVER behind a gap fill. That payload holds the finallyData that clears isAuthenticatingWithShortLivedToken and account.isLoading. A gap fill that fails or aborts leaves both flags set, reauthenticate() gives up on every 407, and every write returns 407 for the rest of the session. Add both sign-in commands to the exemption list so the response applies when the response arrives. The client watermark stays where the client left it, so the next response that carries a previousUpdateID finds the same gap and fetches it with a live auth token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c653c1da3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| READ_COMMANDS.SIGN_IN_WITH_SHORT_LIVED_AUTH_TOKEN, | ||
| READ_COMMANDS.SIGN_IN_WITH_SUPPORT_AUTH_TOKEN, |
There was a problem hiding this comment.
When a short-lived/support sign-in response arrives with previousUpdateID ahead of the client's watermark, adding it to requestsToIgnoreLastUpdateID bypasses the gap-fill path and calls OnyxUpdates.apply; that helper advances ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT to the response lastUpdateID after applying only the sign-in payload/finallyData. In the behind-client scenario from the new test, the client now records the missing range as already applied, so later responses with previousUpdateID within that range will not refetch those server updates and the app can remain stale instead of catching up.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
One scenario. A user signs in with SAML and the client is behind on Onyx updates.
Real numbers from the reported session: the client had applied update
14868706738. The sign-in response came back withpreviousUpdateID: 14886370217. That is a gap of about 17 million updates.Before the change,
SaveResponseInOnyxsaw the gap and parked the whole sign-in response inONYX_UPDATES_FROM_SERVER. The parked payload held thefinallyDatathat clearsisAuthenticatingWithShortLivedTokenandaccount.isLoading. The app then ran a gap fill to catch up. That gap fill failed and later aborted. The two flags stayed set.reauthenticate()checks those flags and gives up, so every write returned 407 for the next 8 hours. The user saw approvals and renames succeed on screen. The server got none of them.After the change, the two sign-in commands sit in
requestsToIgnoreLastUpdateID, so the response applies the moment it arrives. The flags clear. The next 407 renews the token as normal.The gap is not ignored. The client watermark stays where it was, so the next response that carries a
previousUpdateIDfinds the same gap and fetches it, this time with a live token.Fixed Issues
$ #97480
PROPOSAL: n/a
Tests
npx jest tests/unit/MiddlewareTest.ts.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENTin Onyx far below the current server update ID, for example to 1./transitionwith a fresh short-lived auth token.Applying update type: https ... command: 'SignInWithShortLivedAuthToken'appears and noRe-authentication abortedline follows.Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENTin Onyx to 1 so the client looks behind on updates./transitionwith a fresh short-lived auth token.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A