Investigate and fix sequential queue issues

[mjasikowski]

Coming from #69669, we're currently encountering problems with sequential queue that take significant amount of engineering time (by that I mean months) to fix.

This is a tracking issue, so we can investigate, share our findings and propose further actions.

Issue Owner

Current Issue Owner: @ChavdaSachin

[mjasikowski]

@adhorodyski will be taking care of this ASAP

[adhorodyski]

Next step is on me to list potential bugs in the sequential queue together with scenarios they can surface in. I'll update later in this thread.

[OlimpiaZurek]

I think in the meantime, we can review and merge this PR to track expense creation errors in Sentry.

[adhorodyski]

Here are my current investigation notes, with distinct issues documented.

---

Issue 1: Optimistic data is applied before the request is persisted to the queue

In API.write(), the call to prepareRequest() applies optimistic Onyx updates (Onyx.update(optimisticData)) as a side effect before the request is ever persisted to the queue. The persist happens later in PersistedRequests.save() via Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, ...), which is also async and not awaited.

This means there is a window where the UI reflects an optimistic state, but the write request backing it has not been durably saved to disk. If the app is killed by the OS, crashes, or the user closes it during this window, the optimistic data becomes stale/orphaned — the request that would have reconciled it with the server is gone.

Current order:

1. prepareRequest() — applies optimistic Onyx data (async, not awaited) as a side effect
2. pushToSequentialQueue() — calls PersistedRequests.save() which does Onyx.set() (async, not awaited)
3. flush() may start processing immediately

Safer order would be:

1. Prepare request (pure — no side effects, just assemble the request object)
2. Persist to queue (await — guarantee the request survives crashes)
3. Apply optimistic data (now safe — even if we crash after this, the request is queued and will eventually fire)
4. Fire the request

Concrete reproduction (slow/flaky network):

1. Network is slow or flapping — app detects offline
2. API.write() is called — prepareRequest() applies optimistic data (user sees the UI change)
3. pushToSequentialQueue() calls PersistedRequests.save() — fires Onyx.set() (async, not awaited)
4. push() checks isOffline() — network just dropped, so it returns early: "Unable to push request due to offline status" (line 367). No flush, no process.
5. App gets backgrounded before the Onyx.set() from step 3 completes — the request never made it to disk
6. App comes back online → onReconnection fires flush() — but the queue is empty, nothing to flush
7. User is stuck with stale optimistic UI, request is permanently lost

Impact: Lost write requests in production. The user sees the optimistic UI update, but if the app dies or gets backgrounded before Onyx.set() in PersistedRequests.save() completes, the request is never sent and never retried. Especially reproducible on slow/flaky networks where the offline check triggers an early return in push() and the app gets backgrounded before the async persist finishes.

---

Issue 2: Nothing in the write pipeline is awaited

API.write() returns Promise.resolve() immediately (via processRequest()). None of the critical async operations in the pipeline are awaited:

• Onyx.update(optimisticData) in prepareRequest() — fire and forget
• Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, ...) in PersistedRequests.save() — fire and forget
• flush() and process() in SequentialQueue — fire and forget

The caller of API.write() has zero guarantee that:

• The optimistic update was applied
• The request was persisted to disk
• The request was sent or even queued for sending

This makes it impossible for calling code to handle errors or react to failures in the pipeline. Under load, on slow storage, or on mobile where the OS can kill the app at any moment, this "fire and forget everything" approach increases the risk of silently lost requests.

Impact: There is no way to know if a write request was successfully queued. Combined with Issue 1, this creates a scenario where requests can be silently dropped with no error surfaced to the user or to monitoring.

---

Issue 3: Memory vs. disk state split can cause lost or duplicated requests

Some parts of the queue lifecycle live only in memory while others are persisted to disk via Onyx. If the app dies at the wrong moment, these two sources of truth diverge.

3a. Ongoing request lost on restart (high concern)

processNextRequest() pops the first request from the in-memory persistedRequests array and assigns it to ongoingRequest. However, it only persists ongoingRequest to disk if persistWhenOngoing is true — which is not the default. Meanwhile, the shortened persistedRequests array is not written back to disk at this point either.

If the app dies after processNextRequest() but before the request completes:

• ongoingRequest was never written to disk (default case) — it's gone
• The disk still has the old PERSISTED_REQUESTS array, but the dedup check in the Onyx connect callback relies on ongoingRequest being set to remove the duplicate — and since PERSISTED_ONGOING_REQUESTS is null on disk, no dedup happens
• Result: The request either gets duplicated (re-executed on restart) or lost (if the server already processed it but the client doesn't know)

3b. pendingSaveOperations lost before initialization (lower concern, but real on slow devices)

If write requests arrive before Onyx has loaded PERSISTED_REQUESTS (i.e., before isInitialized is true), they are buffered into the in-memory pendingSaveOperations array. These are only flushed to disk when the Onyx connect callback fires.

If the app dies before that callback fires, the buffered requests are lost. On slow/old devices or during heavy app startup, this window could be meaningful.

3c. In-memory persistedRequests diverges from disk (high concern)

processNextRequest() mutates the in-memory array immediately (removes the first element), but does not write the updated array back to ONYXKEYS.PERSISTED_REQUESTS. The disk version still contains the full queue including the request that was just "popped."

The disk only gets updated later when endRequestAndRemoveFromQueue() is called after successful processing. If the app dies between the pop and the completion, the disk and memory are out of sync. On restart, the request is still in the disk queue and may be re-executed — leading to duplicate server-side actions.

The deepEqual dedup check in the Onyx connect callback is the safety net, but it's fragile: if middleware mutates the request object during processing (e.g., adding fields), the objects won't be equal and dedup fails.

Impact: Depending on timing, requests can be lost (3a, 3b) or duplicated (3c). Individually each window is narrow, but across millions of users on varied devices, the aggregate probability is non-trivial — consistent with the lost requests observed in production.

---

Issue 4: Last-write-wins race condition in PersistedRequests.save() permanently loses requests

The Onyx connect callback on PERSISTED_REQUESTS (line 28) fires every time Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, ...) is called. The callback blindly overwrites the in-memory array: persistedRequests = val ?? [] (line 30).

Meanwhile, save() builds the new array from the current in-memory state and fires Onyx.set() without awaiting it:

persistedRequests = [...persistedRequests, newRequest];   // line 91
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); // line 92 — async, not awaited

The race

When multiple save() calls happen in rapid succession (e.g., user taps multiple actions quickly):

1. save(requestA) → in-memory becomes [requestA], fires Onyx.set([requestA]) (async, pending)
2. save(requestB) → in-memory becomes [requestA, requestB], fires Onyx.set([requestA, requestB]) (async, pending)
3. If Onyx.set from step 2 resolves first → connect callback sets persistedRequests = [requestA, requestB] ✓
4. Then Onyx.set from step 1 resolves second → connect callback sets persistedRequests = [requestA] — requestB is gone from memory
5. Onyx's on-disk state is now also [requestA] because step 1's Onyx.set was the last write to complete

requestB is permanently lost — from memory and from disk. No retry, no error, no trace.

Why this is the primary culprit

• It doesn't require an app crash or restart — just two writes close together
• It affects the happy path, not just edge cases
• The probability increases with user activity (more writes = more races)
• It's invisible — no error is thrown, no log indicates the loss
• The next save() call builds on the stale in-memory array, compounding the problem

Additional amplification

The same race pattern exists in other functions that do read-modify-write on persistedRequests without awaiting:

• endRequestAndRemoveFromQueue() — reads persistedRequests, splices, calls Onyx.multiSet() (not awaited)
• deleteRequestsByIndices() — filters persistedRequests, calls Onyx.set() (not awaited)
• update() — splices persistedRequests, calls Onyx.set() (not awaited)

Any of these running concurrently with a save() can cause the same last-write-wins data loss.

Impact: Permanently lost requests in production. It requires no special conditions beyond rapid successive writes — a common pattern in normal app usage. The lack of any error or logging makes it particularly insidious.

---

Issue 5: Conflict resolution operates on stale data

In push(), conflict resolution runs via checkAndFixConflictingRequest(requests) (line 345-346) where requests comes from getAllPersistedRequests() — the in-memory array. But as established in Issue 4, the in-memory array can be stale (out of sync with disk due to pending Onyx.set() calls that trigger the connect callback to overwrite it).

The same problem also exists in prepareRequest(), which runs its own conflict check at line 67-70 using getAll() before the request even reaches the queue.

Conflict resolution might:

• Miss a request that was just saved but whose Onyx.set() callback hasn't fired yet — leading to duplicates that should have been deduplicated
• See a request that was already removed but whose removal hasn't been persisted — leading to wrongly applied conflict actions (e.g., replacing or deleting based on a ghost entry)
• Make delete/replace decisions based on an outdated view of the queue — e.g., deleteRequestsByIndices() uses indices that may no longer correspond to the correct requests if the array shifted

This is compounded by the fact that conflict resolution happens in two places (prepareRequest() and push()), both reading the same stale in-memory array independently.

Impact: Duplicate requests that should have been deduplicated, or legitimate requests wrongly deleted/replaced by conflict resolution acting on stale queue state. Particularly likely during rapid user interactions where multiple writes with conflict resolvers fire in quick succession.

[adhorodyski]

Here's my current plan:

• dive into existing test suites around SQ (sequential queue) and map what's not being tested based on the above notes.
• write down the plan for what needs to be tested.
• continue with the implementation of failing tests.