Send SearchForTodos concurrently with writes instead of waiting on the queue - #97468
Send SearchForTodos concurrently with writes instead of waiting on the queue#97468TMisiukiewicz wants to merge 2 commits into
Conversation
|
@codex review |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
![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: 2867122695
ℹ️ 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".
| if (command === READ_COMMANDS.SIGN_IN_WITH_SHORT_LIVED_AUTH_TOKEN || command === READ_COMMANDS.SIGN_IN_WITH_SUPPORT_AUTH_TOKEN) { | ||
| // Sign in with shortLivedAuthToken commands shouldn't be blocked by write commands, and neither should READS_SENT_DURING_WRITES. | ||
| // Unlike the sign in commands, those hold their Onyx updates until the writes have applied theirs (see applyHTTPSOnyxUpdates). | ||
| if (command === READ_COMMANDS.SIGN_IN_WITH_SHORT_LIVED_AUTH_TOKEN || command === READ_COMMANDS.SIGN_IN_WITH_SUPPORT_AUTH_TOKEN || READS_SENT_DURING_WRITES.has(command)) { |
There was a problem hiding this comment.
When SearchForTodos races an in-flight OpenApp/ReconnectApp, the server can process the write first but deliver the read response first. This branch sends that read immediately, but SaveResponseInOnyx runs doesClientNeedToBeUpdated() before applyHTTPSOnyxUpdates gets a chance to wait for the sequential queue, so a read whose previousUpdateID includes the still-in-flight write is treated as a gap instead of being deferred behind the write (and during initial app loading the saved update is ignored). Please defer the whole response/gap check until the write has at least staged/applied its update ID, not just the final Onyx.update.
Useful? React with 👍 / 👎.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Took a careful pass through this along with the surrounding queue/Onyx machinery. Ran the affected suites locally — NetworkTest, APITest, SequentialQueueTest and the new SearchForTodosDeferralTest all pass (60 tests), and lint is clean apart from pre-existing seatbelt warnings.
The goal is right and the diff is small and well-commented. The reasoning for rejecting queueOnyxUpdates (stranding a response that lands after the buffer already drained) is correct, and keeping the sign-in commands as separate checks with the "must apply immediately" note is the right call. One issue I think blocks, one robustness concern, and a few smaller things.
1. Staleness inversion: the payload is now older, but still applied last
The current gate does more than order the Onyx apply — it means the request is sent after the writes settle, so its payload reflects post-write server state.
With this change SearchForTodos is sent at t0 and applied at t2, on top of data that reflects t2. Onyx merge is last-write-wins per field, so the older payload wins on every overlapping key.
Concrete case — offline approve, then reconnect:
- User is offline and approves report X.
ApproveMoneyRequestis persisted. - Reconnect.
reconnectApp()enqueuesReconnectApp, the queue holds both writes, andSearchForTodosnow goes out immediately. The server has not processed the approval yet, so X comes back in the approve bucket withstateNum: SUBMITTED. - The queue drains, the approval succeeds, X becomes
APPROVEDin Onyx. - The held to-do payload then applies and merges X back to
stateNum: SUBMITTED.
X shows as awaiting approval again until a Pusher update or a reload corrects it. The same shape applies to any Pusher update landing between t0 and t2, and reportActions_ merges can resurrect actions deleted in that window. On a large account the window is the full OpenApp duration — the 5-10s cited in the description.
The description says "its data still lands no earlier than the writes it raced against", which is true, but the hazard isn't when it lands — it's that it is older and lands last.
Suggestion: take the fast path only when the queue holds nothing but OPEN_APP / RECONNECT_APP. Those apply no optimistic data to these keys and their own payload is the fresher one, so the inversion disappears. Fall back to the existing waitForWrites behaviour whenever any other write is pending.
2. waitForIdle() doesn't establish that the write's Onyx data has been applied
resolveIsReadyPromise() fires at src/libs/Network/SequentialQueue.ts:420, and flushOnyxUpdatesQueue() — the thing that actually hands the buffered WRITE batch to Onyx — is called at line 427, in the same synchronous block. The deferred .then(() => Onyx.update(updates)) is a microtask queued at 420, so it runs after Onyx.update(writeBatch) has been called but well before it has resolved.
In practice the write gets a one-microtask head start (Onyx.update runs afterInit synchronously and defers its operations through clearPromise.then(...), so the write's operations are enqueued first). But that is a property of Onyx.update internals, not something this code establishes.
There's also a window with no head start at all: if the SearchForTodos response lands after resolveIsReadyPromise() but while flushQueue()'s Onyx.update is still in flight, waitForIdle() is already resolved and the todos payload applies immediately, concurrent with the flush. That matters because OpenApp / full ReconnectApp send report_, reportNameValuePairs_, transactions_ and transactionViolations_ as SET_COLLECTION, and SearchForTodos merges into exactly those collections.
Worth noting the codebase already treats these as distinct events: getCurrentFlushPromise() was added to QueuedOnyxUpdates precisely because queued WRITE updates "only land in Onyx when flushQueue() runs". It can't be used directly here though — flushQueue() nulls flushPromise before its Onyx.update resolves, so by the time the deferred microtask runs it returns an already-resolved promise.
A gate that does hold: wait for IS_LOADING_REPORT_DATA to go true -> false. It sits in finallyData, which lands in the same Onyx.update(copyUpdates) batch as the report and transaction setCollections, so observing false proves the write's data is applied. No new plumbing needed.
I have not reproduced a failure from this — flagging it as fragility rather than a demonstrated bug.
3. Each updateHandler call re-reads the gate
applyHTTPSOnyxUpdates invokes updateHandler up to three times (onyxData -> success/failureData -> finallyData), chained. Each applyWhenQueueIsIdle call reads waitForSequentialQueueIdle() at its own moment, so an unrelated write arriving between them re-arms isReadyPromise and splits what should be one batch across two very different times.
SearchForTodos carries no success/finally data today so this is latent, but it's a trap for the next command added to READS_SENT_DURING_WRITES. Capturing the gate promise once and reusing it for all three calls would close it.
4. The deferral is a no-op on non-leader tabs
flush() resolves isReadyPromise immediately when !isClientTheLeader(), so on a follower tab the to-do payload applies with no gating at all. Not a regression — the same is true today — but it does mean the safety argument only covers the leader tab.
5. The tests can't catch an ordering problem
TODOS_ONYX_DATA deliberately uses an unrelated key (NVP_PRIORITY_MODE), so the tests prove the deferral fires but not that it orders correctly against the write. A case where the write's onyxData does setCollection on a collection and the deferred read does mergeCollection into the same one — asserting the read's members survive — would cover the interaction that actually matters here.
6. NetworkTest: assertion weakened and the comment is now stale
Step 4 is still labelled "First API Call Verification" but now only asserts the command appears somewhere in the list. Filtering SearchForTodos out of calledCommands() and keeping the ordering assertion on the remainder would preserve what the test was checking.
7. Nit: Fixed Issues
### Fixed Issues is a bare $ with the PROPOSAL: placeholder still in place — needs the full issue URL before this is out of draft.
8. Minor: this also widens the post-sign-out window
Reads don't set canCancel (src/libs/API/makeRequest.ts sets it for writes only), so HttpUtils.cancelPendingRequests() on sign-out doesn't abort SearchForTodos. Its response can now park on isReadyPromise for the length of the queue drain and apply afterwards. Reads have never gone through flushQueue()'s preservedKeys filter so this isn't a new gap, just a wider window.
Explanation of Change
SearchForTodosis fired byloadPostDataForOpenOrReconnect()right afterOpenApp/ReconnectAppare enqueued, butAPI.readgates every read behindwaitForWrites(), so the request didn't actually hit the network until the sequential write queue drained. On large accounts, whereOpenAppcan take 5-10s,SearchForTodossat idle that whole time before even being sent.This PR makes two changes:
READS_SENT_DURING_WRITESset (currently justSEARCH_FOR_TODOS) insrc/libs/API/types.ts.API.readskipswaitForWritesfor commands in this set, so they go out concurrently with in-flight writes, mirroring the existing carve-out for the sign-in short-lived/support auth token commands.SearchForTodos' response touches Onyx keys thatOpenApp/ReconnectAppalso write, its Onyx updates must not apply while those writes are still in flight. InapplyHTTPSOnyxUpdates(src/libs/actions/OnyxUpdates.ts), commands in that set now apply viawaitForSequentialQueueIdle().then(() => Onyx.update(updates))instead of the existingqueueOnyxUpdatesbuffer (that buffer only drains when the queue processor finishes with an empty queue, so a response landing after the queue already went idle would be stranded). Gating on queue-idle applies immediately when nothing is in flight, sinceisReadyPromisestarts resolved.Net effect: the request stops occupying the critical path, and its data still lands no earlier than the writes it raced against. The sign-in commands are left as separate checks since they must apply immediately (deferring an auth token behind writes would break login).
Added
tests/unit/SearchForTodosDeferralTest.tscovering (a) the request firing alongside an in-flight write while its Onyx updates apply only after that write settles, and (b) applying immediately when no write is in flight (regression guard against stranding).Fixed Issues
$
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari