Stop repeated ReconnectApp calls when the device clock is behind the server#93426
Conversation
… AppTest The subscription now reaches reconnectApp through the internal triggerFullReconnect call, so the AppTest spy on the reconnectApp export no longer observes the full-reconnect trigger. Spying on triggerFullReconnect (called via the live export binding) restores a real assertion in both subscription tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Read the LAST_FULL_RECONNECT_TIME value back from Onyx in assertions instead of a mirrored local, and fold the re-arm drain into applyServerResponse so the headline test reads top-to-bottom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
Drop the duplicated connectWithoutView boilerplate above reconnectAppIfFullReconnectBefore and tighten the FullReconnectUtils, triggerFullReconnect, and test docstrings — keep the clock-skew rationale, shed the restatement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eea1cb77ae
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
TLDR to the current AI reviews, this is 1/2 of fixing failure mode 1, I will open a followup. |
|
@codex review |
|
@mkhutornyi 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a72edfd54
ℹ️ 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".
ReviewThe fix is sound: writing the receipt as What I verified
Minor / non-blocking:
Nice work — the clock-skew framing and the seed-before-request invariant are clearly reasoned and well-tested. |
mkhutornyi
left a comment
There was a problem hiding this comment.
Otherwise looks good.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #92541 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@adhorodyski is this 1 of a few PRs to address the linked issue? It seems like you've described 4 sub-problems this issue is about, and I think this PR solves the first one correct? If so, can we change the PR body description from to That way the issue doesn't auto-close once this is deployed? |
|
This targets 50% of the first issue, sure let's update this |
Name the two real values plainly and explain the concept once where it lives, so the code reads clearly without the old metaphors. - "receipt"/"seed" prose removed; getFullReconnectSeedTime renamed to getLastFullReconnectTimeToRecord (it writes that value before the reconnect finishes, so "completion" would be inaccurate). - reconnectAppIfFullReconnectBefore renamed to serverReconnectCutoff. - Concept now explained once in FullReconnectUtils; other files point to it instead of repeating. Comments kept only where they carry a why: the clock-skew clamp, the write-before-fire ordering, and the connectWithoutView safety note tgolen asked for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The old comment ("A timestamp of when the last full reconnect should have
been done") read as if this were the client's own last reconnect time,
which is a separate key (LAST_FULL_RECONNECT_TIME). Describe it as what it
is: a cutoff the server sets, which the client only reads.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fix lives in the shared getOnyxDataForOpenOrReconnect, but every existing test drove only the subscribe -> triggerFullReconnect (ReconnectApp) path. The OpenApp entry into the same code had no direct coverage. Add two tests isolating the OpenApp leg on a client clock behind the server: - white-box: OpenApp successData records max(now, cutoff), not the behind-clock now - behavioral: an OpenApp response re-delivering a known cutoff starts no reconnect loop Both fail on pre-fix behavior (recorded now < cutoff) and pass on the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The parameter shadowed the module-level serverReconnectCutoff Onyx variable. Both are needed, so rename the parameter to cutoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I read it all again and this is everything for the failure mode 1, so I updated the description as well to read 'fixed issues' 👍🏼 |
tgolen
left a comment
There was a problem hiding this comment.
The improved comments are great. Thank you!
mountiny
left a comment
There was a problem hiding this comment.
Agreeed, thanks! The comments are very useful now
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Looks like PR Reviewer Checklist is failed because of #92926 merged 18 hrs ago. |
|
yes |
|
🚧 @blimpich 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.15-0 🚀
|
|
🤖 Help site review: no docs changes required. This PR is an internal sync/networking fix — it changes when the The help site under @adhorodyski, if you believe a user-facing behavior here should be documented, reply with the specifics and I'll draft a help site PR. Otherwise no action is needed. |
|
Hi @adhorodyski. Is it an internal PR? |
|
@IuliiaHerets I think the QA steps can be followed by QA, what is not clear about them? |
|
@mountiny Step 5 is not clear |
|
@IuliiaHerets Those are tests before the PR was merged, but |
|
@mountiny, you're right, thanks for pointing that out |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.15-3 🚀
|

@mountiny
Explanation of Change
A "full reconnect" is when the app refetches all of its data from the server. The server can ask for one by setting a cutoff time in the Onyx NVP value
NVP_RECONNECT_APP_IF_FULL_RECONNECT_BEFORE. The app keeps its own last reconnect time inLAST_FULL_RECONNECT_TIME.subscribeToFullReconnectcompares the two: if the last reconnect is older than the cutoff, it triggers a reconnect.The bug
After a reconnect the app records the time so the check stops firing. It recorded the current time. Every
OpenAppand fullReconnectAppresponse also re-delivers the cutoff (inresponse.onyxData), and that is applied before thesuccessDatathat records the time.So on a device whose clock is behind the server, the recorded time lands before the cutoff. The app still reads as stale, fires another full reconnect, and repeats. Each reconnect waits for its response, so the loop runs at one round trip per cycle. In production this was about 816,000
ReconnectAppcalls over 14 days, with traces of 31 calls in 9 seconds.The fix
Record a time that is never earlier than the cutoff it answers: the later of the current time and the cutoff. That holds whatever the device clock reads. We record it in two places:
triggerFullReconnect(), before the request is sent, so the cutoff re-delivered by the response cannot start another reconnect.successData, as before.subscribeToFullReconnectnow goes throughtriggerFullReconnect()instead of callingreconnectApp()directly. The comparison and the recorded-time logic live inFullReconnectUtils.tsasshouldTriggerFullReconnectandgetLastFullReconnectTimeToRecord.Fixed Issues
#92541
PROPOSAL:
Tests
ReconnectApp.ReconnectApprequest fires per offline → online transition, with no repeatedReconnectApprequests looping afterwards.ReconnectAppper reconnect, and the app keeps responding to clicks.npx jest tests/unit/FullReconnectUtilsTest.ts tests/unit/SubscribeToFullReconnectTest.ts tests/actions/AppTest.ts— the integration suite simulates a full response cycle on a client clock behind the server (onyxDataapplied beforesuccessData) and asserts exactly oneReconnectAppfires; it fails on pre-fix code.Offline tests
ReconnectApprequest and queued actions process normally — no repeated full-payload reconnect downloads.QA Steps
Same as Tests (steps 1–4). This change only affects when the
ReconnectAppAPI command fires; there are no UI changes.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