diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index c68504b82845f..20cf3988c03b4 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -53,10 +53,7 @@ addMiddleware(SaveResponseInOnyx); // FraudMonitoring - Tags the request with the appropriate Fraud Protection event. addMiddleware(FraudMonitoring); -// Use timestamp-based IDs to avoid collisions between browser tabs. -// Each tab has its own JS context with its own counter, so a simple -// incrementing number would collide across tabs. -let requestIndex = Date.now(); +let requestIndex = 0; /** * Prepare the request to be sent. Bind data together with request metadata and apply optimistic Onyx data. @@ -125,13 +122,13 @@ function prepareRequest( /** * Process a prepared request according to its type. */ -async function processRequest(request: OnyxRequest, type: ApiRequestType): Promise> { +function processRequest(request: OnyxRequest, type: ApiRequestType): Promise> { Log.info('[API] Processing request', false, {command: request.command, type}); // Write commands can be saved and retried, so push it to the SequentialQueue if (type === CONST.API_REQUEST_TYPE.WRITE) { Log.info('[API] Write command. Pushing to SequentialQueue', false, {command: request.command}); - await pushToSequentialQueue(request); - return; + pushToSequentialQueue(request); + return Promise.resolve(); } // Read requests are processed right away, but don't return the response to the caller @@ -167,7 +164,6 @@ function write( ): Promise> { Log.info('[API] Called API write', false, {command, ...apiCommandParameters}); const request = prepareRequest(command, CONST.API_REQUEST_TYPE.WRITE, apiCommandParameters, onyxData, conflictResolver); - return processRequest(request, CONST.API_REQUEST_TYPE.WRITE); } diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 0341714c55b06..8bedfec790e89 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -436,7 +436,7 @@ onReconnection(flush); // Flush the queue when the persisted requests are initialized onPersistedRequestsInitialization(flush); -async function handleConflictActions(conflictAction: ConflictData, newRequest: OnyxRequest): Promise { +function handleConflictActions(conflictAction: ConflictData, newRequest: OnyxRequest) { Log.info('[SequentialQueue] handleConflictActions', false, { conflictType: conflictAction.type, newCommand: newRequest.command, @@ -447,14 +447,14 @@ async function handleConflictActions(conflictAction: Confl Log.info('[SequentialQueue] Conflict resolution: PUSH', false, { command: newRequest.command, }); - await savePersistedRequest(newRequest); + savePersistedRequest(newRequest); } else if (conflictAction.type === 'replace') { Log.info('[SequentialQueue] Conflict resolution: REPLACE', false, { command: newRequest.command, replaceIndex: conflictAction.index, replacementRequest: conflictAction.request?.command ?? newRequest.command, }); - await updatePersistedRequest(conflictAction.index, conflictAction.request ?? (newRequest as AnyRequest)); + updatePersistedRequest(conflictAction.index, conflictAction.request ?? (newRequest as AnyRequest)); } else if (conflictAction.type === 'delete') { Log.info('[SequentialQueue] Conflict resolution: DELETE', false, { command: newRequest.command, @@ -462,19 +462,19 @@ async function handleConflictActions(conflictAction: Confl willPushNewRequest: conflictAction.pushNewRequest ?? false, hasNextAction: !!conflictAction.nextAction, }); - await deletePersistedRequestsByIndices(conflictAction.indices); + deletePersistedRequestsByIndices(conflictAction.indices); if (conflictAction.pushNewRequest) { Log.info('[SequentialQueue] Pushing new request after delete', false, { command: newRequest.command, }); - await savePersistedRequest(newRequest); + savePersistedRequest(newRequest); } if (conflictAction.nextAction) { Log.info('[SequentialQueue] Processing next conflict action', false, { command: newRequest.command, nextActionType: conflictAction.nextAction.type, }); - await handleConflictActions(conflictAction.nextAction, newRequest); + handleConflictActions(conflictAction.nextAction, newRequest); } } else { Log.info('[SequentialQueue] No action performed, request ignored', false, { @@ -484,7 +484,7 @@ async function handleConflictActions(conflictAction: Confl } } -function push(newRequest: OnyxRequest): Promise { +function push(newRequest: OnyxRequest) { const currentRequests = getAllPersistedRequests(); Log.info('[SequentialQueue] push() called', false, { command: newRequest.command, @@ -494,11 +494,6 @@ function push(newRequest: OnyxRequest): Promise; - if (newRequest.checkAndFixConflictingRequest) { const requests = currentRequests; Log.info('[SequentialQueue] Checking for conflicts', false, { @@ -515,13 +510,13 @@ function push(newRequest: OnyxRequest): Promise(newRequest: OnyxRequest): Promise(newRequest: OnyxRequest): Promise { diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 9c0c5c39629c0..021061517fac0 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -2,7 +2,6 @@ import {deepEqual} from 'fast-equals'; import type {OnyxKey} from 'react-native-onyx'; import Onyx from 'react-native-onyx'; import Log from '@libs/Log'; -import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; import type {AnyRequest} from '@src/types/onyx/Request'; @@ -11,10 +10,6 @@ let persistedRequests: AnyRequest[] = []; let ongoingRequest: AnyRequest | null = null; let pendingSaveOperations: AnyRequest[] = []; let isInitialized = false; -// Tracks all requestIDs this tab has ever seen (from disk init, save(), or other tabs). -// Used to distinguish stale own-write callbacks (ignore) from new requests enqueued -// by other browser tabs (merge into memory). -const knownRequestIDs = new Set(); let initializationCallback: () => void; function triggerInitializationCallback() { if (typeof initializationCallback !== 'function') { @@ -31,39 +26,10 @@ function onInitialization(callbackFunction: () => void) { Onyx.connectWithoutView({ key: ONYXKEYS.PERSISTED_REQUESTS, callback: (val) => { - Log.info('[PersistedRequests] hit Onyx connect callback', false, {isValNullish: val == null, isInitialized}); - - // After initialization, in-memory is authoritative — ignore stale disk - // callbacks to prevent out-of-order Onyx.set() from overwriting the - // correct in-memory state (Bug #80759 Issue 4). - // Exception 1: Onyx.clear() fires callback with null — allow through. - // Exception 2: Other browser tabs can enqueue requests. We detect these - // by checking for requestIDs not in knownRequestIDs, and merge them in. - if (isInitialized && val != null) { - const newFromOtherTabs = val.filter((r) => r.requestID != null && !knownRequestIDs.has(r.requestID)); - if (newFromOtherTabs.length > 0) { - Log.info('[PersistedRequests] Merging requests from other tabs', false, { - newCount: newFromOtherTabs.length, - newCommands: getCommands(newFromOtherTabs), - }); - for (const r of newFromOtherTabs) { - knownRequestIDs.add(r.requestID ?? CONST.DEFAULT_NUMBER_ID); - } - persistedRequests = [...persistedRequests, ...newFromOtherTabs]; - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); - } - return; - } - + Log.info('[PersistedRequests] hit Onyx connect callback', false, {isValNullish: val == null}); const previousInMemoryRequests = [...persistedRequests]; const diskRequests = val ?? []; persistedRequests = diskRequests; - for (const r of diskRequests) { - if (r.requestID == null) { - continue; - } - knownRequestIDs.add(r.requestID); - } Log.info('[PersistedRequests] DISK vs MEMORY comparison', false, { diskRequestsLength: diskRequests.length, @@ -78,11 +44,6 @@ Onyx.connectWithoutView({ Log.info(`[PersistedRequests] Processing pending save operations, size: ${pendingSaveOperations.length}`, false, { pendingCommands: getCommands(pendingSaveOperations), }); - for (const r of pendingSaveOperations) { - if (r.requestID != null) { - knownRequestIDs.add(r.requestID); - } - } const requests = [...persistedRequests, ...pendingSaveOperations]; persistedRequests = requests; Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); @@ -134,9 +95,6 @@ Onyx.connectWithoutView({ */ function clear() { ongoingRequest = null; - persistedRequests = []; - pendingSaveOperations = []; - knownRequestIDs.clear(); Onyx.set(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, null); return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []); } @@ -146,7 +104,7 @@ function getLength(): number { return persistedRequests.length + (ongoingRequest ? 1 : 0); } -function save(requestToPersist: Request): Promise { +function save(requestToPersist: Request) { Log.info('[PersistedRequests] Saving request to queue started', false, { command: requestToPersist.command, currentQueueLength: persistedRequests.length, @@ -159,16 +117,13 @@ function save(requestToPersist: Request): Promise(requestToPersist: Request): Promise { Log.info('[PersistedRequests] Request successfully persisted to disk', false, { command: requestToPersist.command, @@ -247,7 +202,7 @@ function endRequestAndRemoveFromQueue(requestToRemove: Req }); } -function deleteRequestsByIndices(indices: number[]): Promise { +function deleteRequestsByIndices(indices: number[]) { // Create a Set from the indices array for efficient lookup const indicesSet = new Set(indices); @@ -255,21 +210,18 @@ function deleteRequestsByIndices(indices: number[]): Promise { persistedRequests = persistedRequests.filter((_, index) => !indicesSet.has(index)); // Update the persisted requests in storage or state as necessary - return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests).then(() => { Log.info(`Multiple (${indices.length}) requests removed from the queue. Queue length is ${persistedRequests.length}`); }); } -function update(oldRequestIndex: number, newRequest: Request): Promise { +function update(oldRequestIndex: number, newRequest: Request) { const requests = [...persistedRequests]; const oldRequest = requests.at(oldRequestIndex); Log.info('[PersistedRequests] Updating a request', false, {oldRequest, newRequest, oldRequestIndex}); requests.splice(oldRequestIndex, 1, newRequest as AnyRequest); persistedRequests = requests; - if (newRequest.requestID != null) { - knownRequestIDs.add(newRequest.requestID); - } - return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } function updateOngoingRequest(newRequest: Request) { @@ -318,13 +270,16 @@ function processNextRequest(): AnyRequest | null { newQueueLength: persistedRequests.length, }); - // Persist both the updated queue and the ongoing request to disk atomically. - // This ensures that if the app crashes mid-flight, the ongoing request is not - // lost (Bug #80759 Issue 3a) and the queue on disk matches memory (Issue 3c). - Onyx.multiSet({ - [ONYXKEYS.PERSISTED_REQUESTS]: persistedRequests, - ...(ongoingRequest ? {[ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: ongoingRequest} : {}), - }); + if (ongoingRequest && ongoingRequest.persistWhenOngoing) { + Log.info('[PersistedRequests] Persisting ongoingRequest to disk', false, { + command: ongoingRequest.command, + }); + Onyx.set(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, ongoingRequest); + } else { + Log.info('[PersistedRequests] NOT persisting ongoingRequest to disk (persistWhenOngoing=false)', false, { + command: ongoingRequest?.command ?? 'null', + }); + } return ongoingRequest; } @@ -359,13 +314,6 @@ function rollbackOngoingRequest() { newQueueLength: persistedRequests.length, ongoingRequestCleared: true, }); - - // Persist both changes to disk so a crash after rollback doesn't lose - // the rolled-back request or leave a stale ongoingRequest on disk. - Onyx.multiSet({ - [ONYXKEYS.PERSISTED_REQUESTS]: persistedRequests, - [ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: null, - }); } function getAll(): AnyRequest[] { diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index d6502055b09f0..8e81e4bfd42aa 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -870,11 +870,18 @@ describe('API.write() persistence guarantees', () => { let optimisticDataApplied = false; let requestPersistedBeforeOptimistic = false; + // Mock Onyx.update so that when it receives our marker key we snapshot the + // persisted-requests queue. This avoids spy-ordering issues that caused + // false passes on CI. const updateMock = jest.spyOn(Onyx, 'update').mockImplementation((data) => { // We use ONYXKEYS.IS_CHECKING_PUBLIC_ROOM as a sample key to identify the marker const hasMarker = data.some((entry) => entry.key === ONYXKEYS.IS_CHECKING_PUBLIC_ROOM); if (hasMarker) { optimisticDataApplied = true; + // Note: getAll() checks the in-memory queue, not durable (disk) state. + // This is intentionally a weaker assertion – if even the in-memory + // ordering is wrong (request not queued before optimistic data), the + // stronger disk-persistence guarantee is certainly broken too. requestPersistedBeforeOptimistic = PersistedRequests.getAll().some((r) => r.command === 'MockCommand'); } return Promise.resolve(); @@ -891,7 +898,11 @@ describe('API.write() persistence guarantees', () => { ], }); + // Guard: ensure our mock actually intercepted the optimistic data. + // Without this, the test could pass for the wrong reason (e.g. mock + // never fires, flag stays false, and we'd incorrectly confirm the bug). expect(optimisticDataApplied).toBe(true); + // BUG: The request is NOT in the persisted queue when optimistic data is // applied. When fixed, this assertion should be changed to toBe(true). expect(requestPersistedBeforeOptimistic).toBe(false); @@ -941,11 +952,12 @@ describe('API.write() persistence guarantees', () => { // Flush one microtask to give writePromise.then() a chance to fire. return Promise.resolve() .then(() => { - // FIX: The write promise no longer resolves immediately. - // processRequest() now awaits pushToSequentialQueue() which awaits - // PersistedRequests.save()'s Onyx.set() promise. Since we mocked - // Onyx.set to never resolve, the write promise correctly stalls. - expect(writePromiseResolved).toBe(false); + // BUG: The write promise resolved despite persistence being permanently + // stalled. processRequest() returns Promise.resolve() (API/index.ts:148) + // which is disconnected from the persistence pipeline. + // When fixed, this should be changed to toBe(false) — the write promise + // should NOT resolve until persistence completes. + expect(writePromiseResolved).toBe(true); }) .finally(() => { setMock.mockRestore(); @@ -1004,9 +1016,10 @@ describe('API.write() persistence guarantees', () => { await capturedSets.at(0)?.triggerRealSet(); await waitForBatchedUpdates(); - // FIX (Issue 4): After initialization, the connect callback is a no-op. - // In-memory state is authoritative — both commands survive. - expect(PersistedRequests.getAll()).toHaveLength(2); + // BUG (Issue 4): In-memory state is now [A] — CommandB was lost + // because the stale callback overwrote the correct [A, B] with [A]. + expect(PersistedRequests.getAll()).toHaveLength(1); + expect(PersistedRequests.getAll().at(0)?.command).toBe('CommandA'); // Restore Onyx.set to normal before the next write setMock.mockRestore(); @@ -1025,9 +1038,14 @@ describe('API.write() persistence guarantees', () => { }, ); - // FIX (Issue 5): With Issue 4 fixed, the conflict resolver sees the complete - // queue including CommandB, enabling correct deduplication decisions. - expect(queueSeenByResolver).toContainEqual(expect.objectContaining({command: 'CommandB'})); + // BUG (Issue 5): The conflict resolver cannot see CommandB because the + // in-memory queue was corrupted by the Issue 4 out-of-order race. The stale + // connect callback overwrote [A, B] with [A], making CommandB invisible + // to the resolver. If it needed to deduplicate or resolve conflicts with + // CommandB, it would fail, potentially causing duplicate or conflicting requests. + // When fixed, the resolver should see both CommandA and CommandB. + // Change to: expect(queueSeenByResolver).toContainEqual(expect.objectContaining({command: 'CommandB'})); + expect(queueSeenByResolver).not.toContainEqual(expect.objectContaining({command: 'CommandB'})); } finally { setMock.mockRestore(); } diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index afcb482506bed..e1cc8da03e5e6 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -1,6 +1,5 @@ import Onyx from 'react-native-onyx'; import type {OnyxKey} from 'react-native-onyx'; -import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'; import * as PersistedRequests from '../../src/libs/actions/PersistedRequests'; import ONYXKEYS from '../../src/ONYXKEYS'; import type Request from '../../src/types/onyx/Request'; @@ -114,6 +113,9 @@ describe('PersistedRequests persistence guarantees', () => { expect(request.persistWhenOngoing).toBeUndefined(); expect(PersistedRequests.getAll()).toHaveLength(1); + // Spy on Onyx.set AFTER beforeEach has settled to avoid capturing setup calls + const setMock = jest.spyOn(Onyx, 'set'); + // Move the request from queue to ongoingRequest PersistedRequests.processNextRequest(); @@ -121,12 +123,22 @@ describe('PersistedRequests persistence guarantees', () => { expect(PersistedRequests.getOngoingRequest()).toEqual(request); expect(PersistedRequests.getAll()).toHaveLength(0); - return waitForBatchedUpdates().then(async () => { - // FIX: processNextRequest() now always persists ongoingRequest to disk - // via Onyx.multiSet, regardless of the persistWhenOngoing flag. - const diskOngoing = await OnyxUtils.get(ONYXKEYS.PERSISTED_ONGOING_REQUESTS); - expect(diskOngoing).toEqual(expect.objectContaining({command: 'OpenReport'})); - }); + return waitForBatchedUpdates() + .then(() => { + // BUG: Onyx.set was never called for PERSISTED_ONGOING_REQUESTS + // because persistWhenOngoing is undefined (PersistedRequests.ts:273). + // The ongoing request exists only in memory — no disk backup. + // When fixed, this should persist ALL ongoing requests regardless + // of the persistWhenOngoing flag. Change to: + // expect(setMock).toHaveBeenCalledWith( + // ONYXKEYS.PERSISTED_ONGOING_REQUESTS, + // expect.objectContaining({command: 'OpenReport'}), + // ); + expect(setMock).not.toHaveBeenCalledWith(ONYXKEYS.PERSISTED_ONGOING_REQUESTS, expect.objectContaining({command: 'OpenReport'})); + }) + .finally(() => { + setMock.mockRestore(); + }); })); // BUG: processNextRequest() at PersistedRequests.ts:264-266 does @@ -156,14 +168,25 @@ describe('PersistedRequests persistence guarantees', () => { // In-memory: only requestB remains expect(PersistedRequests.getAll()).toHaveLength(1); - // Read disk state directly to see what's actually persisted - return waitForBatchedUpdates().then(async () => { - const diskRequests = await OnyxUtils.get(ONYXKEYS.PERSISTED_REQUESTS); - const diskArray = diskRequests ?? []; - - // FIX: processNextRequest() now persists the updated queue to disk - // via Onyx.multiSet. Disk matches in-memory — only requestB remains. - expect(diskArray).toHaveLength(1); + // Read disk state via a fresh Onyx connection to see what's actually persisted + return new Promise((resolve) => { + const connection = Onyx.connectWithoutView({ + key: ONYXKEYS.PERSISTED_REQUESTS, + reuseConnection: false, + callback: (diskRequests) => { + Onyx.disconnect(connection); + const diskArray = diskRequests ?? []; + + // BUG: processNextRequest() updates in-memory immediately but does NOT + // write the updated queue back to ONYXKEYS.PERSISTED_REQUESTS. + // In-memory has [requestB] but disk still has [request, requestB]. + // When fixed, disk should match in-memory. + // Change to: expect(diskArray).toHaveLength(1); + expect(diskArray).toHaveLength(2); + + resolve(); + }, + }); }); }); }); @@ -234,10 +257,13 @@ describe('PersistedRequests persistence guarantees', () => { await capturedSets.at(0)?.triggerRealSet(); await waitForBatchedUpdates(); - // FIX: After initialization, the connect callback is a no-op. - // In-memory state is authoritative and not overwritten by stale disk callbacks. - // Both requests survive regardless of Onyx.set() resolution order. - expect(PersistedRequests.getAll()).toHaveLength(2); + // BUG: The connect callback at PersistedRequests.ts:32 blindly overwrites + // in-memory state with whatever disk returns. The stale Onyx.set([A]) + // resolved last, so the callback received [A] and overwrote [A, B]. + // requestB is permanently lost. + // When fixed, both requests should survive regardless of resolution order. + // Change to: expect(PersistedRequests.getAll()).toHaveLength(2); + expect(PersistedRequests.getAll()).toHaveLength(1); } finally { setMock.mockRestore(); }