Repository navigation
Conversation
5e70e90 to
54d9cc0
Compare
32b3733 to
9306e4f
Compare
|
@codex review |
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. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9306e4f to
db151fe
Compare
LukasMod
changed the title
LukasMod
changed the title
db6a92f to
a8e806b
Compare
|
Assignment here @chuckdries Thanks! |
a8e806b to
b7751b0
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4e82545 to
0905b80
Compare
8881b39 to
cb8bdd5
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
LukasMod
changed the title
fabioh8010
left a comment
fabioh8010
left a comment
There was a problem hiding this comment.
@LukasMod Could you write and verify more manual tests for tab syncing? Just to make sure the change is safe.
|
@fabioh8010 I added 4 more tests, looks okay. I didn't spot issues |
![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: cb8bdd5c6d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Logger.logHmmm(`Sync removed a key while this tab was writing it: ${key}`); | ||
| } | ||
| // Drop like a local removal, so the key leaves the index instead of lingering as an indexed key with no value. | ||
| cache.drop(key); |
There was a problem hiding this comment.
When a key already has a nullish marker (for example, an IndexedDB multiGet returned undefined for an absent member), cache.drop() removes the key from the index but does not clear that marker, so hasCacheForKey() still returns true. If a merge to that key is pending when a new useOnyx subscriber mounts, the hook treats the key as cached and reports loaded instead of remaining loading until the merge lands; the added test covers only the case without a nullish marker. The previous cache.set(key, null) path cleared it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You’re right that drop keeps the marker where set cleared it. That’s intended. The marker means “storage has nothing for this key”, and that’s still true after the other tab removed it. All local removals (remove, merge(key, null), eviction) keep it the same way, so this just makes the sync path match them. The hook already reported loaded for a marked key before the removal arrived, so nothing changes for it. On web this state is nearly impossible to reach anyway
| if (PendingWrites.hasPendingWriteForKey(key)) { | ||
| Logger.logHmmm(`Sync removed a key while this tab was writing it: ${key}`); | ||
| } |
There was a problem hiding this comment.
This new log line also fires when nothing is lost
trackPendingWrite registers a write when it is called, before afterInit and merge batching apply it to the cache.
Line 300 in cb8bdd5
So the check is also true when:
- this tab's merge is still queued, applies after the drop and wins
- this tab's own pending
set(key, null)overlaps the other tab's removal - the other tab signs out and
Onyx.clearremoves every key, giving one line per key with no per-session cap (the Fix cache-miss diagnostic for undefined keys #863 diagnostic caps viareportedCacheMisses)
The new test is the first case. A non-zero count would not prove the race happened.
There was a problem hiding this comment.
Agreed on all three, and I reproduced the third: a sign-out in one tab while the other reloads logged about thirty lines, all derived keys being recomputed while the other tab’s clear removed them. Nothing was lost, both tabs end up signed out, so the line was reporting cleanup, not the race. Removed the line and the helper. This PR is the drop fix only
| it('should log once when another tab removes a key this tab is still writing', async () => { | ||
| const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; | ||
| expect(syncCallback).toBeDefined(); | ||
|
|
||
| await Onyx.set(MEMBER_1, {id: 1}); | ||
| const mergePromise = Onyx.merge(MEMBER_1, {id: 2}); | ||
| syncCallback([[MEMBER_1, undefined]]); | ||
| await mergePromise; | ||
|
|
||
| expect(logHmmmSpy).toHaveBeenCalledTimes(1); | ||
| expect(logHmmmSpy).toHaveBeenCalledWith(expect.stringContaining(MEMBER_1)); | ||
| }); |
There was a problem hiding this comment.
The "log once" test does not check the outcome
It only counts logHmmm calls and never asserts the value left after the removal and the merge collide.
Fix: add expect(await Onyx.get(MEMBER_1)).toEqual({id: 2}).
There was a problem hiding this comment.
Gone together with the log
| const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; | ||
| expect(syncCallback).toBeDefined(); |
There was a problem hiding this comment.
Sync callback setup repeated in every new test
The same two lines open all four new tests (lines 430, 447, 466, 479).
Fix: a typed getSyncCallback() helper next to forgetValueButKeepKey.
There was a problem hiding this comment.
Added getSyncCallback() next to forgetValueButKeepKey, the two remaining tests use it
| // Capture the previous cached value before cache.set() so notifyCollection() can diff old vs new per member. | ||
| const previousValue = isCollectionMember ? cache.get(key) : undefined; | ||
| cache.set(key, value); | ||
| if (value === null || value === undefined) { |
There was a problem hiding this comment.
// Capture the previous cached value before cache.set() so notifyCollection() can diff old vs new per member.
This comment is out-of-date.
It still says "before cache.set()", but the next line can now call cache.drop().
There was a problem hiding this comment.
Fixed, it now says “before the cache changes”
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Bump @chuckdries on this one #884 (comment) as we are coming from follow-up of this review #863 (review) Thanks! |
There was a problem hiding this comment.
@LukasMod Thanks.
The drop does fix the log noise: no false alarm, and a later real miss still logs. But I don't think it should merge on its own.
A local removal is always current. A sync answer isn't, because it can be older than a write this tab still has in flight. If tab A writes a key right after its sync read starts, the drop takes the key out of the index while the write still lands in storage.
In Real IndexedDB, tab B removes the key and tab A's write queues behind its sync read:
| After the race | main |
#884 |
|---|---|---|
Onyx.clear |
row removed | row stays |
mergeCollection({c: 3}) on stored {a, b} |
{a, b, c} |
{c} |
multiSet or mergeCollection with null |
row removed | row stays |
setCollection without that member |
row removed | row stays |
The clear row also reproduces in two real tabs, so a value can survive sign-out. That means "This PR does not change that. It only means such a case now looks like a plain removal instead of a cache miss" isn't right.
I forced the order to make it repeatable. Without forcing it, with tab A writing the same key 0 to 6 ms after tab B's removal, 3 of 120 rounds left a row with no index entry on #884, and 0 on main. I can't say how often this happens in production. Your log run saw this overlap at sign-out, but that log also fired when nothing was lost.
The race itself is already on main, and it loses data there too. The cache loses the value, so a later Onyx.merge builds on nothing and overwrites the row: stored {a, b} becomes {c}, on both main and #884.
Your description already names the fix: "skip or re-read a synced key while a local write to it is pending". I tried both, and I'd go with re-read. Skip can leave a tab holding a value storage no longer has, after a real removal during a pending batch. Re-read fixes every row above plus the merge case, 0 of 120 rounds, and your drop stays as it is: commit on top of this PR (+52/−6 in lib, including a small Storage.isMemoryOnly). I didn't test constant writes to one key, or a re-read that a newer write only partly overtakes.
If you'd rather ship #884 first, this version keeps cache.set and marks the key so only the diagnostic skips it. It matches main on every row above, and the race stays open as it is today.
One more difference from main: after this race, main logs Cache held no value for a key storage still has once, and #884 logs nothing, because the key has left the index. Your #863 test calls that log the only trace of a lost value. The marker version is silent here too. The re-read has nothing to log, because the cache keeps the value.
|
|
||
| expect(logHmmmSpy).toHaveBeenCalledTimes(1); | ||
| expect(logHmmmSpy).toHaveBeenCalledWith(expect.stringContaining(COLLECTION)); | ||
| }); |
There was a problem hiding this comment.
Test for the clear row. It fails here with Received: "mine", and passes on main and on both versions above.
it('should still delete a key on Onyx.clear when a removal answer lands while this tab is writing it', async () => {
// Given another indexed key, and a write to this key still on its way to storage
const syncCallback = getSyncCallback();
await Onyx.set(ONYXKEYS.OTHER_TEST, 'other');
const setItemMock = jest.mocked(StorageMock.setItem);
const writeToStorage = setItemMock.getMockImplementation();
const storageWrite = createDeferredTask();
setItemMock.mockImplementationOnce((key, value) => storageWrite.promise.then(() => writeToStorage?.(key, value)));
const write = Onyx.set(ONYXKEYS.TEST_KEY, 'mine');
await waitForPromisesToResolve();
// When the removal answer arrives before the write lands, and this tab then clears
syncCallback([[ONYXKEYS.TEST_KEY, undefined]]);
storageWrite.resolve();
await write;
await Onyx.clear();
// Then the row doesn't survive into the next session
expect(await StorageMock.getItem(ONYXKEYS.TEST_KEY)).toBeNull();
});| cache.set(key, value); | ||
| if (value === null || value === undefined) { | ||
| // Drop like a local removal, so the key leaves the index instead of lingering as an indexed key with no value. | ||
| cache.drop(key); |
There was a problem hiding this comment.
This answer can be older than a write still in flight here. Core of the re-read (full change in the commit linked above):
+ const isRemoved = value === null || value === undefined;
+
+ // A removal, or any re-read answer, may predate a write still in flight, and acting on it would lose that write, so ask storage again once the writes settle.
+ if ((isRemoved || isReread) && PendingWrites.getPendingWritesForKey(key).length > 0) {
+ keysToReread.push(key);
+ continue;
+ }
There was a problem hiding this comment.
Can you help me understand why this is necessary? Just to prevent an old in-flight write from re-populating the dropped key?
|
I'm stealing the review on this one since I have context on the prior PRs and the project |
Details
Follow-up to #863
On web, a key removed in another tab reaches this tab through the sync handler as
cache.set(key, undefined). That empties the value but keeps the key in the index. Every local removal usescache.drop(key), which also takes the key out of the index. The cache-miss diagnostic from #863 treats "indexed, no value" as a lost value: it reads storage, logs if the other tab has already written the key back, and spends the collection's one report for the session either way.Production logs about 7,200
[Onyx] Cache held no value for a key storage still haslines per working day (average of the five weekdays Oct 28 to Nov 3), all from web browsers. A two-tab run with extra logging traced every hit to the other tab removing a key and writing it back, or removing it twice. None was a lost value.One change in the sync handler in
lib/Onyx.ts: anullorundefinedsynced value callscache.drop(key), the same as a local removal. The key leaves the index and the diagnostic skips it. Subscribers see the sameundefinedthey saw before. Only the index and the collection member registry change, the way they already do for local removals.A key already marked as known empty stays known empty after the removal, because
dropdoes not touch that mark andsetdid. That matchesOnyxUtils.removeandOnyx.merge(key, null).Not in scope: a known edge case in the sync receiver. It reads a key from disk and applies what it finds, so if this tab wrote the same key a moment earlier and that write is still on its way to disk, the older value can land over the newer one. This PR does not change that; it only means such a case now looks like a plain removal instead of a cache miss. I tried logging it and the only hits were sign-out cleanup, both tabs clearing and recomputing derived values, so I dropped the log. If it ever shows up as a real symptom, the fix is in the receiver: skip or re-read a synced key while a local write to it is pending.
Related Issues
Expensify/App#101910
Follow-up to #863
Linked E/App PR
Expensify/App#105902
Automated Tests
tests/unit/OnyxSubscriptionManagerTest.ts, in the cache miss reporting group:main.main, where the removal spends the collection's report.tests/unit/useOnyxTest.ts:useOnyxon a key another tab removed, with a merge pending, still reportsloadingand then the merged value. Passes onmaintoo; it pins that hook behavior did not change.Full suite: 774 tests passing. Lint and
tsc --noEmitclean.Manual Tests
Web, two tabs on one account, console open with Verbose level. In every test, neither tab should log
[Onyx] Cache held no value for a key storage still has.Basic flow
Remove then re-add the same key
Collection shrinks from the other tab
Offline queue across tabs
Expense created and deleted from the other tab
Removal while the other tab is writing the same key
Sign-out during the other tab's reload
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
N/A
Android: mWeb Chrome
android-web.mov
iOS: Native
N/A
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
test.2.mov
test3.mov
test4-5.mov
signout.mov