From 13f3abc57f0dba86d0df8bcbe742a20b72f1c277 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 13 Feb 2026 17:09:03 +0100 Subject: [PATCH 1/4] test: add failing test for API.write() persistence ordering (#80759) Adds an it.failing() test asserting that Onyx.set(PERSISTED_REQUESTS) is called before Onyx.update(optimisticData) during API.write(). Currently the order is reversed, meaning a crash after optimistic updates but before persistence would lose the write request. Co-Authored-By: Claude Opus 4.6 --- tests/unit/APITest.ts | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 3fe52aeac4af5..9ade16c0dbd7f 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -855,3 +855,46 @@ describe('APITests', () => { }); }); }); + +// Issue: https://github.com/Expensify/App/issues/80759 +describe('API.write() persistence guarantees', () => { + it.failing('Issue 1: should persist the request before applying optimistic data', () => + Onyx.multiSet({ + [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'}, + [ONYXKEYS.SESSION]: {authToken: 'testToken'}, + [ONYXKEYS.NETWORK]: {isOffline: true}, + }).then(() => { + // Set up spies AFTER setup to only capture calls from API.write() + const setMock = jest.spyOn(Onyx, 'set'); + const updateMock = jest.spyOn(Onyx, 'update'); + + try { + // @ts-expect-error -- passing mock command with onyxData + API.write('MockCommand' as WriteCommand, {param1: 'value1'} as ApiRequestCommandParameters[WriteCommand], { + optimisticData: [ + { + onyxMethod: Onyx.METHOD.SET, + key: ONYXKEYS.SESSION, + value: {authToken: 'testToken'}, + }, + ], + }); + + // Find the Onyx.set call that persisted the request to disk + const persistCallIdx = setMock.mock.calls.findIndex(([key]) => key === ONYXKEYS.PERSISTED_REQUESTS); + expect(persistCallIdx).toBeGreaterThanOrEqual(0); + expect(updateMock).toHaveBeenCalled(); + + // The request must be durably persisted via Onyx.set(PERSISTED_REQUESTS) BEFORE + // optimistic data is applied to the UI via Onyx.update. This ensures that if the + // app crashes right after optimistic updates are visible, the write request survives. + const persistOrder = setMock.mock.invocationCallOrder[persistCallIdx]; + const updateOrder = updateMock.mock.invocationCallOrder[0]; + expect(persistOrder).toBeLessThan(updateOrder); + } finally { + setMock.mockRestore(); + updateMock.mockRestore(); + } + }), + ); +}); From 3b8870f3387d5f38552389a117d1641a0079abf2 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 13 Feb 2026 18:02:45 +0100 Subject: [PATCH 2/4] test: use marker key for robust optimistic data ordering check (#80759) Replace spy-ordering approach with a marker key that filters the exact Onyx.update call, preventing false passes caused by unpredictable invocationCallOrder across environments. Co-Authored-By: Claude Opus 4.6 --- tests/unit/APITest.ts | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 9ade16c0dbd7f..d31cab3f51350 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -858,41 +858,43 @@ describe('APITests', () => { // Issue: https://github.com/Expensify/App/issues/80759 describe('API.write() persistence guarantees', () => { + const TEST_OPTIMISTIC_KEY = 'test_optimisticDataMarker'; + it.failing('Issue 1: should persist the request before applying optimistic data', () => Onyx.multiSet({ [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'}, [ONYXKEYS.SESSION]: {authToken: 'testToken'}, [ONYXKEYS.NETWORK]: {isOffline: true}, }).then(() => { - // Set up spies AFTER setup to only capture calls from API.write() - const setMock = jest.spyOn(Onyx, 'set'); - const updateMock = jest.spyOn(Onyx, 'update'); + 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) => { + const hasMarker = data.some((entry) => entry.key === TEST_OPTIMISTIC_KEY); + if (hasMarker) { + requestPersistedBeforeOptimistic = PersistedRequests.getAll().some((r) => r.command === 'MockCommand'); + } + return Promise.resolve(); + }); try { - // @ts-expect-error -- passing mock command with onyxData API.write('MockCommand' as WriteCommand, {param1: 'value1'} as ApiRequestCommandParameters[WriteCommand], { optimisticData: [ { onyxMethod: Onyx.METHOD.SET, - key: ONYXKEYS.SESSION, - value: {authToken: 'testToken'}, + key: TEST_OPTIMISTIC_KEY, + value: true, }, ], }); - // Find the Onyx.set call that persisted the request to disk - const persistCallIdx = setMock.mock.calls.findIndex(([key]) => key === ONYXKEYS.PERSISTED_REQUESTS); - expect(persistCallIdx).toBeGreaterThanOrEqual(0); - expect(updateMock).toHaveBeenCalled(); - - // The request must be durably persisted via Onyx.set(PERSISTED_REQUESTS) BEFORE - // optimistic data is applied to the UI via Onyx.update. This ensures that if the - // app crashes right after optimistic updates are visible, the write request survives. - const persistOrder = setMock.mock.invocationCallOrder[persistCallIdx]; - const updateOrder = updateMock.mock.invocationCallOrder[0]; - expect(persistOrder).toBeLessThan(updateOrder); + // The request must already be in the persisted queue BEFORE optimistic data + // is applied via Onyx.update. This ensures that if the app crashes right after + // optimistic updates are visible, the write request survives on disk. + expect(requestPersistedBeforeOptimistic).toBe(true); } finally { - setMock.mockRestore(); updateMock.mockRestore(); } }), From a6c264cb1ad6c1b04056b32abbf08c2f40136b0d Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 16 Feb 2026 15:38:07 +0100 Subject: [PATCH 3/4] fix lint errors in the test file --- tests/unit/APITest.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index d31cab3f51350..38efeab20d711 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -858,8 +858,6 @@ describe('APITests', () => { // Issue: https://github.com/Expensify/App/issues/80759 describe('API.write() persistence guarantees', () => { - const TEST_OPTIMISTIC_KEY = 'test_optimisticDataMarker'; - it.failing('Issue 1: should persist the request before applying optimistic data', () => Onyx.multiSet({ [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'}, @@ -872,7 +870,8 @@ describe('API.write() persistence guarantees', () => { // persisted-requests queue. This avoids spy-ordering issues that caused // false passes on CI. const updateMock = jest.spyOn(Onyx, 'update').mockImplementation((data) => { - const hasMarker = data.some((entry) => entry.key === TEST_OPTIMISTIC_KEY); + // 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) { requestPersistedBeforeOptimistic = PersistedRequests.getAll().some((r) => r.command === 'MockCommand'); } @@ -884,7 +883,7 @@ describe('API.write() persistence guarantees', () => { optimisticData: [ { onyxMethod: Onyx.METHOD.SET, - key: TEST_OPTIMISTIC_KEY, + key: ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, value: true, }, ], From 27b1e344654ec2450e5c2896cf92615580bd71c1 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Tue, 17 Feb 2026 15:11:59 +0100 Subject: [PATCH 4/4] test: replace it.failing with explicit bug assertion and guard (#80759) it.failing masks regressions since any error counts as expected failure. Replace with a regular test that guards the mock fired, then explicitly asserts the current buggy ordering (toBe(false)). When fixed, flip to toBe(true). Co-Authored-By: Claude Opus 4.6 --- tests/unit/APITest.ts | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 38efeab20d711..c739dbb5a0ba2 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -858,12 +858,16 @@ describe('APITests', () => { // Issue: https://github.com/Expensify/App/issues/80759 describe('API.write() persistence guarantees', () => { - it.failing('Issue 1: should persist the request before applying optimistic data', () => + // BUG: Currently, prepareRequest() applies optimistic data via Onyx.update() BEFORE + // processRequest() persists the request via SequentialQueue.push(). This test documents + // the buggy ordering. When the bug is fixed, flip the final assertion to toBe(true). + test('Issue 1: should persist the request before applying optimistic data', () => Onyx.multiSet({ [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'}, [ONYXKEYS.SESSION]: {authToken: 'testToken'}, [ONYXKEYS.NETWORK]: {isOffline: true}, }).then(() => { + let optimisticDataApplied = false; let requestPersistedBeforeOptimistic = false; // Mock Onyx.update so that when it receives our marker key we snapshot the @@ -873,6 +877,11 @@ describe('API.write() persistence guarantees', () => { // 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(); @@ -889,13 +898,16 @@ describe('API.write() persistence guarantees', () => { ], }); - // The request must already be in the persisted queue BEFORE optimistic data - // is applied via Onyx.update. This ensures that if the app crashes right after - // optimistic updates are visible, the write request survives on disk. - expect(requestPersistedBeforeOptimistic).toBe(true); + // 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); } finally { updateMock.mockRestore(); } - }), - ); + })); });