Repository navigation
Keep values written while Onyx.clear is in progress - #881
carlosmiceli merged 2 commits into
Conversation
Onyx.clear resets the cache right away but writes defaults to storage and notifies subscribers only after storage finishes clearing, using the values it captured at the start. A write that lands in between, like a new sign-in saving its session while the previous sign-out is still clearing, was then overwritten in storage and subscribers were told the key was reset. Skip the default write and the stale notification for keys written during the clear, and save those keys again after removeItems.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 566009cb40
ℹ️ 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".
Comparing the cache value with the value from before the clear treated a write that repeats that value (e.g. a re-auth restoring unchanged session data) as a read refilling the cache, so clear still wiped it. Record the keys of writes issued while clear runs through PendingWrites instead, since reads never register there.
|
Hi @carlosmiceli , is there anything from my side to do to merge this one ? |
Details
Onyx.clearresets the cache right away, but it writes the default values to storage and notifies subscribers only after storage has finished clearing. It uses the values it captured when it started. If something writes to a key while storage is still clearing, the end ofclearoverwrites that write: the key's default is written over it in storage, and subscribers are told the key was reset even though the cache holds the new value.In E/App this happens when a user switches accounts in a browser that is already signed in (Supportal, OldDot transition, re-auth). The sign-out clear can take from 200ms to over 10s, and the new sign-in saves its session and queues
OpenAppduring that time. When the clear finishes,sessiongoes back to its default{loading: false}in storage and in every subscriber, so every request after that gets a 407, and the queuedOpenAppis dropped. The user is left on infinite skeletons. Production logs from the last few days show 34 of these overlaps across 16 accounts; in 32 of them the new login token was wiped.This PR makes
clearleave such writes alone:clearrecords the keys of every write issued through the public API (set,merge,update, ...), using the key list each write already registers inPendingWrites. Reads that refill the cache from storage never register there, so they are not mistaken for writes, even when a write stores the same value the key had before the clear. A recorded key counts as written during the clear only if its cache value is no longer the oneclearreset it to, so a write issued before the reset still loses to it, as before.removeItems, in case it deleted them after they were stored.Keys nobody writes during the clear behave exactly as before.
App ships Onyx 3.0.117, and 3.0.118 to 3.0.123 need App changes that are not merged yet (async
Onyx.get,react-native-nitro-sqlite10). So the linked E/App PR pins this fix applied on top of 3.0.117 (callstack-internal/react-native-onyx#b5f84a4a), which isolates it in E/App CI.Related Issues
Expensify/App#104458
Linked E/App PR
Expensify/App#105110
Automated Tests
Added
tests/unit/onyxClearConcurrentWriteTest.ts(Write data while Onyx.clear is in progress). It holdsStorage.removeItemsso writes land while a clear is running:should keep values written to keys after the clear reset them: a key with a default, a regular key and a collection member are written during the clear, and all three keep the new value in subscribers, the cache and storage. Without the fix this fails withdefault/undefined.should keep a value written during the clear even when it equals the value from before the clear: writing the pre-clear value again during the clear (like a re-auth restoring unchanged session data) is kept, not wiped.should still reset keys that were not written during the clear: a key that is not written still goes back to its default.The full unit suite passes (24 suites, 710 tests).
Manual Tests
Tested through the linked E/App PR:
[SignInRedirect] Sign-out clear finished after a new sign-in startedline withisNewSessionWiped: true.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)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
height="900" alt="1-account-a-inbox" src="https://github.com/user-attachments/assets/1863d00b-23b6-46f2-b243-a2f7ff6aef1f" />