From a77a3cedca41264cb967e9d0486813fc944efa78 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 28 Aug 2026 17:23:03 +0200 Subject: [PATCH 1/9] Count in-flight Pusher applies as applied when detecting update gaps applyPusherOnyxUpdates serializes every event through one module-level promise, and the watermark only advances once that chain settles. So the next event, chained on an update the client already holds, reads as a gap: the client pauses its own write queue and refetches data it is part-way through writing. Measured over 2026-08-26 production logs, 24.5% of gap detections had the missing update delivered before the gap fired and applied after it, at p50 209ms arrival to detection. lastUpdateIDPendingApply records the highest update ID accepted into the Pusher apply chain, and getLastUpdateIDForGapCheck counts it as applied. It is deliberately absent from getEffectiveLastUpdateID, which is the lower bound of the catch-up fetch range. An accepted update is not a written one. If the apply then rejects, a range fetched from it skips that update for good: the catch-up response is exempt from gap detection, so it advances the persisted watermark past the hole and nothing detects it again. Fetching from the persisted watermark refetches the in-flight range instead, which only costs bandwidth. The test named "keeps a Pusher update that is still applying out of the catch-up fetch range" holds that line. lastUpdateIDPendingFlush already carries the same hazard for the WRITE path, since it does sit in getEffectiveLastUpdateID. Left as-is here; it wants its own issue. Internal tracking: callstack-internal/expensify-issues#2882 Co-Authored-By: Claude Opus 5 (1M context) --- src/libs/actions/OnyxUpdates.ts | 17 +++++- tests/unit/OnyxUpdatesTest.ts | 97 +++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 156754d0a350..f71cd53ae49d 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -24,10 +24,16 @@ let lastUpdateIDAppliedToClient: number | undefined = 0; // applied so queued WRITE responses don't look like gaps; reset if the flush fails so recovery can kick in. let lastUpdateIDPendingFlush = 0; +let lastUpdateIDPendingApply = 0; + function getEffectiveLastUpdateID(): number { return Math.max(lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush); } +function getLastUpdateIDForGapCheck(clientLastUpdateID?: number): number { + return Math.max(clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush, lastUpdateIDPendingApply); +} + function getPersistedLastUpdateID(): number { return lastUpdateIDAppliedToClient ?? 0; } @@ -42,6 +48,7 @@ Onyx.connectWithoutView({ // too — a stale value from the previous session would mask real gaps after signing back in. if (val === undefined) { lastUpdateIDPendingFlush = 0; + lastUpdateIDPendingApply = 0; } }, }); @@ -209,6 +216,8 @@ function apply({lastUpdateID, type, request, response, upd return result; }) .catch((error) => { + lastUpdateIDPendingApply = 0; + if (shouldAdvanceLastUpdateID) { Log.alert('[OnyxUpdateManagerError] Applying the updates failed, not advancing lastUpdateID so the client can recover on the next reconnect', { type, @@ -241,6 +250,10 @@ function apply({lastUpdateID, type, request, response, upd return advanceLastUpdateIDAfterApply(applyPromise); } if (type === CONST.ONYX_UPDATE_TYPES.PUSHER && updates) { + if (shouldAdvanceLastUpdateID) { + lastUpdateIDPendingApply = Math.max(lastUpdateIDPendingApply, Number(lastUpdateID)); + } + return advanceLastUpdateIDAfterApply(applyPusherOnyxUpdates(updates, Number(lastUpdateID))); } if (type === CONST.ONYX_UPDATE_TYPES.AIRSHIP && updates) { @@ -282,9 +295,7 @@ function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID}: DoesC return false; } - // Updates staged for the deferred WRITE flush count as applied here, otherwise the responses of queued - // WRITE requests would look like gaps until the flush runs and needlessly pause the queue to refetch. - const lastUpdateIDFromClient = Math.max(clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush); + const lastUpdateIDFromClient = getLastUpdateIDForGapCheck(clientLastUpdateID); // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp if (!lastUpdateIDFromClient) { diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index 0fee69e61e0a..47a70e979319 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -1,4 +1,5 @@ import {SIDE_EFFECT_REQUEST_COMMANDS} from '@libs/API/types'; +import PusherUtils from '@libs/PusherUtils'; import CONST from '@src/CONST'; import * as OnyxUpdates from '@src/libs/actions/OnyxUpdates'; @@ -315,6 +316,102 @@ describe('OnyxUpdatesTest', () => { await flushQueue(); }); + const applyHeldPusherUpdate = (previousUpdateID: number, lastUpdateID: number) => { + let releaseApply: () => void = () => {}; + const handlerSpy = jest.spyOn(PusherUtils, 'triggerMultiEventHandler').mockReturnValueOnce( + new Promise((resolve) => { + releaseApply = resolve; + }), + ); + const applyPromise = OnyxUpdates.apply({ + type: CONST.ONYX_UPDATE_TYPES.PUSHER, + previousUpdateID, + lastUpdateID, + updates: [{eventType: 'onyxApiUpdate', data: []}], + }); + + return { + release: () => { + releaseApply(); + handlerSpy.mockRestore(); + return applyPromise; + }, + }; + }; + + it('does not report a gap for a Pusher update that is still applying', async () => { + // Given the client is caught up to update 10 + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + + // When update 20 arrives over Pusher and its apply is held mid-flight + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + + // Then the next event, chained on update 20, is not treated as a gap even though the watermark is still at 10 + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(false); + + await heldApply.release(); + }); + + it('keeps a Pusher update that is still applying out of the catch-up fetch range', async () => { + // Given the client is caught up to update 10 + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + + // When update 20 arrives over Pusher and its apply is held mid-flight + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + + // Then a genuinely later gap is still detected + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 30})).toBe(true); + + // And it fetches from the persisted watermark, so a rejected apply cannot strand update 20 + expect(OnyxUpdates.getEffectiveLastUpdateID()).toBe(10); + + await heldApply.release(); + }); + + it('clears the pending apply watermark on sign-out', async () => { + // Given the client is caught up to update 10 and update 20 from Pusher is held mid-apply + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + + // When the user signs out, which clears Onyx storage + await Onyx.clear(); + await waitForBatchedUpdates(); + + // Then the pending marker from the previous session no longer masks gaps in the new session + expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15})).toBe(true); + + await heldApply.release(); + }); + + it('resumes gap detection when the Pusher apply fails, and leaves the shared Pusher chain rejected so no Pusher test can follow it', async () => { + // Given the client is caught up to update 10 + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + + // When applying update 20 from Pusher fails + const handlerSpy = jest.spyOn(PusherUtils, 'triggerMultiEventHandler').mockRejectedValueOnce(new Error('storage write failed')); + await expect( + OnyxUpdates.apply({ + type: CONST.ONYX_UPDATE_TYPES.PUSHER, + previousUpdateID: 10, + lastUpdateID: 20, + updates: [{eventType: 'onyxApiUpdate', data: []}], + }), + ).rejects.toThrow('storage write failed'); + await waitForBatchedUpdates(); + + // Then the update no longer counts as applied, so the gap is detected and recovery can refetch it + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(true); + + handlerSpy.mockRestore(); + }); + it('does not move the watermark backwards when a slower older update settles after a newer one', async () => { // Given the client is caught up to update 10 await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); From 1bee93a49002a07e236ef485bdf4ed848cb232a8 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 31 Aug 2026 15:01:56 +0200 Subject: [PATCH 2/9] Release a held Pusher apply after every test so a failure stays local applyPusherOnyxUpdates reassigns the module-level pusherEventsPromise from itself, so a held apply blocks every later Pusher apply in the file. The held promise was only released on each test's happy path, after its assertions. A failing assertion threw first, the mocked handler never settled, and the following tests hit the 240s Jest timeout instead of asserting -- the file reported a timeout, not a diff, and --testTimeout did not bound it. Releasing it from afterEach turns that into one local failure: with the first Pusher assertion inverted the suite now reports 1 failed, 16 passed in 1.5s. The rejection case still has to run last. afterEach releases a held apply, it cannot un-reject a rejected chain, so that test keeps its name. Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/OnyxUpdatesTest.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index 47a70e979319..3960c453445d 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -25,6 +25,9 @@ describe('OnyxUpdatesTest', () => { beforeEach(() => Onyx.clear().then(waitForBatchedUpdates)); + let releaseHeldApply: (() => void) | undefined; + afterEach(() => releaseHeldApply?.()); + it('applies Airship Onyx updates correctly', () => { const reportID = NumberUtils.rand64(); const reportActionID = NumberUtils.rand64(); @@ -321,6 +324,7 @@ describe('OnyxUpdatesTest', () => { const handlerSpy = jest.spyOn(PusherUtils, 'triggerMultiEventHandler').mockReturnValueOnce( new Promise((resolve) => { releaseApply = resolve; + releaseHeldApply = resolve; }), ); const applyPromise = OnyxUpdates.apply({ From 9a0e6757b1aacc6a32a607908d208c097f739003 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 2 Sep 2026 11:46:09 +0200 Subject: [PATCH 3/9] Say why the pending-apply marker is cleared for every failed apply The reset in the shared catch was bare, and a reviewer read it as an oversight and asked for it to be scoped to Pusher failures. Scoping it that way is a data-loss path: the marker is read as a flat max, so a lower-ID non-Pusher failure would stay masked once the in-flight Pusher apply advances the watermark, whereas today it costs one redundant GetMissingOnyxMessages. Name the trade at the reset, and pin it with a test that fails an unrelated Airship apply while a Pusher apply is held. Carry the rest by naming: getLastUpdateIDForGapCheck said when it is called, not how it differs from getEffectiveLastUpdateID, so the difference needed a comment on the marker to explain it. Renamed to getLastUpdateIDIncludingInFlightApplies, which states the difference and leaves the declaration to speak for itself. Co-Authored-By: Claude Opus 5 (1M context) --- src/libs/actions/OnyxUpdates.ts | 6 ++++-- tests/unit/OnyxUpdatesTest.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index f71cd53ae49d..4e00a897edfd 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -30,7 +30,7 @@ function getEffectiveLastUpdateID(): number { return Math.max(lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush); } -function getLastUpdateIDForGapCheck(clientLastUpdateID?: number): number { +function getLastUpdateIDIncludingInFlightApplies(clientLastUpdateID?: number): number { return Math.max(clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush, lastUpdateIDPendingApply); } @@ -216,6 +216,8 @@ function apply({lastUpdateID, type, request, response, upd return result; }) .catch((error) => { + // Cleared for any failed apply, not just Pusher: the marker is a flat max, so keeping it after an + // unrelated lower-ID failure would mask that gap. Errs toward a redundant refetch. lastUpdateIDPendingApply = 0; if (shouldAdvanceLastUpdateID) { @@ -295,7 +297,7 @@ function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID}: DoesC return false; } - const lastUpdateIDFromClient = getLastUpdateIDForGapCheck(clientLastUpdateID); + const lastUpdateIDFromClient = getLastUpdateIDIncludingInFlightApplies(clientLastUpdateID); // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp if (!lastUpdateIDFromClient) { diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index 3960c453445d..0661911b1c92 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -393,6 +393,32 @@ describe('OnyxUpdatesTest', () => { await heldApply.release(); }); + it('clears the Pusher pending apply marker when an unrelated Airship apply fails, so a real gap is never masked', async () => { + // Given the client is caught up to update 10 and update 20 from Pusher is held mid-apply + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + + // When an unrelated Airship apply fails, which leaves the shared Airship chain rejected so no Airship test can follow it + const updateSpy = jest.spyOn(Onyx, 'update').mockRejectedValueOnce(new Error('storage write failed')); + await expect( + OnyxUpdates.apply({ + type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, + previousUpdateID: 20, + lastUpdateID: 30, + updates: [{eventType: 'onyxApiUpdate', data: []}], + }), + ).rejects.toThrow('storage write failed'); + await waitForBatchedUpdates(); + + // Then the held Pusher update stops counting as applied, so the gap at update 20 is still detected + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(true); + + updateSpy.mockRestore(); + await heldApply.release(); + }); + it('resumes gap detection when the Pusher apply fails, and leaves the shared Pusher chain rejected so no Pusher test can follow it', async () => { // Given the client is caught up to update 10 await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); From abe7602d1c8f7b0e2667ad8fc49320ebb74a559a Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 2 Sep 2026 13:30:52 +0200 Subject: [PATCH 4/9] Stop two gap-detection tests depending on where they sit in the file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tests rejected an apply whose promise chain lives in module scope, so the rejection outlived them and only the next test's position kept the file green. Adding an Airship or Pusher test below either one would have failed on a poisoned chain rather than on a diff, which is the fragility that already had to be fixed once for the held-apply helper. Fail a READ request's apply instead of an Airship one: applyHTTPSOnyxUpdates returns a fresh promise per call, so nothing leaks, and failing it at update 15 under a Pusher apply held at 20 is the case that argues for the unconditional reset — the marker masks 15 if the reset is scoped by type. The Pusher chain has no such escape, so that test loads its own copy of the module and passes the watermark in, keeping it independent of both the chain and the Onyx instance the fresh copy connects to. Verified by deleting the reset: these two go red, the other sixteen stay green. Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/OnyxUpdatesTest.ts | 70 ++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index 0661911b1c92..378659605c53 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -393,53 +393,65 @@ describe('OnyxUpdatesTest', () => { await heldApply.release(); }); - it('clears the Pusher pending apply marker when an unrelated Airship apply fails, so a real gap is never masked', async () => { + it('clears the Pusher pending apply marker when an unrelated apply fails below it, so a real gap is never masked', async () => { // Given the client is caught up to update 10 and update 20 from Pusher is held mid-apply await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); await waitForBatchedUpdates(); const heldApply = applyHeldPusherUpdate(10, 20); await waitForBatchedUpdates(); - // When an unrelated Airship apply fails, which leaves the shared Airship chain rejected so no Airship test can follow it + // When an unrelated READ request's apply fails at update 15, below the held Pusher update const updateSpy = jest.spyOn(Onyx, 'update').mockRejectedValueOnce(new Error('storage write failed')); await expect( OnyxUpdates.apply({ - type: CONST.ONYX_UPDATE_TYPES.AIRSHIP, - previousUpdateID: 20, - lastUpdateID: 30, - updates: [{eventType: 'onyxApiUpdate', data: []}], + type: CONST.ONYX_UPDATE_TYPES.HTTPS, + previousUpdateID: 10, + lastUpdateID: 15, + request: {command: 'OpenReport', data: {apiRequestType: CONST.API_REQUEST_TYPE.READ}}, + response: { + jsonCode: 200, + onyxData: [{onyxMethod: 'merge', key: `${ONYXKEYS.COLLECTION.REPORT}${NumberUtils.rand64()}`, value: {}}], + }, }), ).rejects.toThrow('storage write failed'); await waitForBatchedUpdates(); - // Then the held Pusher update stops counting as applied, so the gap at update 20 is still detected - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(true); + // Then update 20 stops counting as applied, so the gap left by update 15 is detected instead of masked + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 15})).toBe(true); updateSpy.mockRestore(); await heldApply.release(); }); - it('resumes gap detection when the Pusher apply fails, and leaves the shared Pusher chain rejected so no Pusher test can follow it', async () => { - // Given the client is caught up to update 10 - await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); - await waitForBatchedUpdates(); - - // When applying update 20 from Pusher fails - const handlerSpy = jest.spyOn(PusherUtils, 'triggerMultiEventHandler').mockRejectedValueOnce(new Error('storage write failed')); - await expect( - OnyxUpdates.apply({ - type: CONST.ONYX_UPDATE_TYPES.PUSHER, - previousUpdateID: 10, - lastUpdateID: 20, - updates: [{eventType: 'onyxApiUpdate', data: []}], - }), - ).rejects.toThrow('storage write failed'); - await waitForBatchedUpdates(); - - // Then the update no longer counts as applied, so the gap is detected and recovery can refetch it - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(true); - - handlerSpy.mockRestore(); + // applyPusherOnyxUpdates keeps its promise chain in module scope, so a rejected apply poisons every later Pusher + // apply in that module instance. Load a fresh copy per test so the rejection cannot outlive the test that caused it. + describe('when a failed Pusher apply leaves the shared event chain rejected', () => { + beforeEach(() => jest.resetModules()); + + it('resumes gap detection so recovery can refetch the update', async () => { + const {apply, doesClientNeedToBeUpdated} = await import('@src/libs/actions/OnyxUpdates'); + const {default: FreshPusherUtils} = await import('@libs/PusherUtils'); + + // Given the client is caught up to update 10 + const clientLastUpdateID = 10; + + // When applying update 20 from Pusher fails + const handlerSpy = jest.spyOn(FreshPusherUtils, 'triggerMultiEventHandler').mockRejectedValueOnce(new Error('storage write failed')); + await expect( + apply({ + type: CONST.ONYX_UPDATE_TYPES.PUSHER, + previousUpdateID: 10, + lastUpdateID: 20, + updates: [{eventType: 'onyxApiUpdate', data: []}], + }), + ).rejects.toThrow('storage write failed'); + await waitForBatchedUpdates(); + + // Then the update no longer counts as applied, so the gap is detected and recovery can refetch it + expect(doesClientNeedToBeUpdated({clientLastUpdateID, previousUpdateID: 20})).toBe(true); + + handlerSpy.mockRestore(); + }); }); it('does not move the watermark backwards when a slower older update settles after a newer one', async () => { From 77bd7e1844685347147cd95292b4f55471c073b1 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 2 Sep 2026 13:42:19 +0200 Subject: [PATCH 5/9] Name the reason for resetModules instead of commenting it The describe now says the event chain is module-scoped, which is the whole reason the block resets modules and imports its own copy. Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/OnyxUpdatesTest.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index 378659605c53..bdbd45ed7d7a 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -423,9 +423,7 @@ describe('OnyxUpdatesTest', () => { await heldApply.release(); }); - // applyPusherOnyxUpdates keeps its promise chain in module scope, so a rejected apply poisons every later Pusher - // apply in that module instance. Load a fresh copy per test so the rejection cannot outlive the test that caused it. - describe('when a failed Pusher apply leaves the shared event chain rejected', () => { + describe('when a failed Pusher apply leaves the module-scoped event chain rejected', () => { beforeEach(() => jest.resetModules()); it('resumes gap detection so recovery can refetch the update', async () => { From 2e59104081d15c6d683a7c149b7fc9e2314e2f8e Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Thu, 3 Sep 2026 16:42:56 +0200 Subject: [PATCH 6/9] Read the pending apply marker only for Pusher gap checks An in-flight Pusher apply is only safe to count as applied for callers serialized behind it on pusherEventsPromise. HTTPS and Airship applies run on their own chains, so reading the marker there could advance the watermark past updates the held apply has not written, leaving no gap to recover from. Also clear the marker once its apply settles, so it stops standing for an update that is no longer in flight and cannot override a lower clientLastUpdateID that a caller passed explicitly. Co-Authored-By: Claude Opus 5 (1M context) --- src/libs/actions/OnyxUpdates.ts | 17 +++-- src/libs/actions/applyOnyxUpdatesReliably.ts | 2 +- tests/unit/OnyxUpdatesTest.ts | 74 +++++++++++--------- 3 files changed, 53 insertions(+), 40 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 4e00a897edfd..0ac0e66b29e6 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -30,10 +30,6 @@ function getEffectiveLastUpdateID(): number { return Math.max(lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush); } -function getLastUpdateIDIncludingInFlightApplies(clientLastUpdateID?: number): number { - return Math.max(clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush, lastUpdateIDPendingApply); -} - function getPersistedLastUpdateID(): number { return lastUpdateIDAppliedToClient ?? 0; } @@ -213,6 +209,9 @@ function apply({lastUpdateID, type, request, response, upd if (lastUpdateIDPendingFlush && lastUpdateIDPendingFlush <= Number(lastUpdateID)) { lastUpdateIDPendingFlush = 0; } + if (lastUpdateIDPendingApply && lastUpdateIDPendingApply <= Number(lastUpdateID)) { + lastUpdateIDPendingApply = 0; + } return result; }) .catch((error) => { @@ -283,6 +282,7 @@ function saveUpdateInformation(updateParams: OnyxUpdatesFr type DoesClientNeedToBeUpdatedParams = { clientLastUpdateID?: number; previousUpdateID?: number; + updateType?: AnyOnyxUpdatesFromServer['type']; }; /** @@ -290,14 +290,19 @@ type DoesClientNeedToBeUpdatedParams = { * and return if an update is needed * @param previousUpdateID The previousUpdateID contained in the response object * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient + * @param updateType the transport the update being checked arrived on */ -function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID}: DoesClientNeedToBeUpdatedParams): boolean { +function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID, updateType}: DoesClientNeedToBeUpdatedParams): boolean { // If no previousUpdateID is sent, this is not a WRITE request so we don't need to update our current state if (!previousUpdateID) { return false; } - const lastUpdateIDFromClient = getLastUpdateIDIncludingInFlightApplies(clientLastUpdateID); + const lastUpdateIDFromClient = Math.max( + clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, + lastUpdateIDPendingFlush, + updateType === CONST.ONYX_UPDATE_TYPES.PUSHER ? lastUpdateIDPendingApply : 0, + ); // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp if (!lastUpdateIDFromClient) { diff --git a/src/libs/actions/applyOnyxUpdatesReliably.ts b/src/libs/actions/applyOnyxUpdatesReliably.ts index 11b276ec1eab..1204577b892d 100644 --- a/src/libs/actions/applyOnyxUpdatesReliably.ts +++ b/src/libs/actions/applyOnyxUpdatesReliably.ts @@ -48,7 +48,7 @@ export default function applyOnyxUpdatesReliably( } const previousUpdateID = Number(updates.previousUpdateID) ?? CONST.DEFAULT_NUMBER_ID; - if (!doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID})) { + if (!doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID, updateType: updates.type})) { return onyxApply(updates).then(); } diff --git a/tests/unit/OnyxUpdatesTest.ts b/tests/unit/OnyxUpdatesTest.ts index bdbd45ed7d7a..271d0ba4571b 100644 --- a/tests/unit/OnyxUpdatesTest.ts +++ b/tests/unit/OnyxUpdatesTest.ts @@ -26,7 +26,10 @@ describe('OnyxUpdatesTest', () => { beforeEach(() => Onyx.clear().then(waitForBatchedUpdates)); let releaseHeldApply: (() => void) | undefined; - afterEach(() => releaseHeldApply?.()); + afterEach(() => { + releaseHeldApply?.(); + releaseHeldApply = undefined; + }); it('applies Airship Onyx updates correctly', () => { const reportID = NumberUtils.rand64(); @@ -253,7 +256,7 @@ describe('OnyxUpdatesTest', () => { // Then a following response chained on update 20 is not treated as a gap, even though the // persisted watermark is still at 10 — otherwise every queued WRITE would pause the queue - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(false); + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(false); // And once the flush applies the staged updates, the persisted watermark catches up await flushQueue(); @@ -306,14 +309,14 @@ describe('OnyxUpdatesTest', () => { }, }); await waitForBatchedUpdates(); - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(false); + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(false); // When the user signs out, which clears Onyx storage await Onyx.clear(); await waitForBatchedUpdates(); // Then the pending watermark from the previous session no longer masks gaps in the new session - expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15})).toBe(true); + expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); // Drain the staged updates so they don't leak into other tests await flushQueue(); @@ -353,7 +356,23 @@ describe('OnyxUpdatesTest', () => { await waitForBatchedUpdates(); // Then the next event, chained on update 20, is not treated as a gap even though the watermark is still at 10 - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(false); + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(false); + + await heldApply.release(); + }); + + it('reports a gap for an HTTPS response chained on a Pusher update that is still applying', async () => { + // Given the client is caught up to update 10 + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + + // When update 20 arrives over Pusher and its apply is held mid-flight + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + + // Then an HTTPS response chained on update 20 still reports the gap, because its apply runs on its own + // promise chain and would advance the watermark past the updates the held apply has not written yet + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 20})).toBe(true); await heldApply.release(); }); @@ -368,7 +387,7 @@ describe('OnyxUpdatesTest', () => { await waitForBatchedUpdates(); // Then a genuinely later gap is still detected - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 30})).toBe(true); + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 30, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); // And it fetches from the persisted watermark, so a rejected apply cannot strand update 20 expect(OnyxUpdates.getEffectiveLastUpdateID()).toBe(10); @@ -388,9 +407,12 @@ describe('OnyxUpdatesTest', () => { await waitForBatchedUpdates(); // Then the pending marker from the previous session no longer masks gaps in the new session - expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15})).toBe(true); + expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); + // And the held apply settling afterwards does not reintroduce it await heldApply.release(); + await waitForBatchedUpdates(); + expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); }); it('clears the Pusher pending apply marker when an unrelated apply fails below it, so a real gap is never masked', async () => { @@ -417,39 +439,25 @@ describe('OnyxUpdatesTest', () => { await waitForBatchedUpdates(); // Then update 20 stops counting as applied, so the gap left by update 15 is detected instead of masked - expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 15})).toBe(true); + expect(OnyxUpdates.doesClientNeedToBeUpdated({previousUpdateID: 15, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); updateSpy.mockRestore(); await heldApply.release(); }); - describe('when a failed Pusher apply leaves the module-scoped event chain rejected', () => { - beforeEach(() => jest.resetModules()); - - it('resumes gap detection so recovery can refetch the update', async () => { - const {apply, doesClientNeedToBeUpdated} = await import('@src/libs/actions/OnyxUpdates'); - const {default: FreshPusherUtils} = await import('@libs/PusherUtils'); - - // Given the client is caught up to update 10 - const clientLastUpdateID = 10; - - // When applying update 20 from Pusher fails - const handlerSpy = jest.spyOn(FreshPusherUtils, 'triggerMultiEventHandler').mockRejectedValueOnce(new Error('storage write failed')); - await expect( - apply({ - type: CONST.ONYX_UPDATE_TYPES.PUSHER, - previousUpdateID: 10, - lastUpdateID: 20, - updates: [{eventType: 'onyxApiUpdate', data: []}], - }), - ).rejects.toThrow('storage write failed'); - await waitForBatchedUpdates(); + it('stops counting a Pusher update as in flight once its apply has settled', async () => { + // Given the client is caught up to update 10 + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); - // Then the update no longer counts as applied, so the gap is detected and recovery can refetch it - expect(doesClientNeedToBeUpdated({clientLastUpdateID, previousUpdateID: 20})).toBe(true); + // When update 20 arrives over Pusher and its apply finishes + const heldApply = applyHeldPusherUpdate(10, 20); + await waitForBatchedUpdates(); + await heldApply.release(); + await waitForBatchedUpdates(); - handlerSpy.mockRestore(); - }); + // Then a caller that overrides the watermark with its own lower value is no longer told it is caught up + expect(OnyxUpdates.doesClientNeedToBeUpdated({clientLastUpdateID: 5, previousUpdateID: 15, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); }); it('does not move the watermark backwards when a slower older update settles after a newer one', async () => { From a46cca92caa3eb674d27be2783268e609bf0e541 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Thu, 3 Sep 2026 16:42:57 +0200 Subject: [PATCH 7/9] Pin the pusherEventsPromise coupling in its own test Nothing checks that update IDs are contiguous before advancing the watermark, so a follower chained on a failed apply only stays unwritten because the rejected pusherEventsPromise rejects it too. Fixing that promise in isolation would turn the pending apply marker into data loss. The test needs its own file: a rejected apply leaves the module-scoped promise rejected for the rest of the module's life. Co-Authored-By: Claude Opus 5 (1M context) --- .../unit/OnyxUpdatesPusherChainFailureTest.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 tests/unit/OnyxUpdatesPusherChainFailureTest.ts diff --git a/tests/unit/OnyxUpdatesPusherChainFailureTest.ts b/tests/unit/OnyxUpdatesPusherChainFailureTest.ts new file mode 100644 index 000000000000..72801c755bca --- /dev/null +++ b/tests/unit/OnyxUpdatesPusherChainFailureTest.ts @@ -0,0 +1,75 @@ +import PusherUtils from '@libs/PusherUtils'; + +import CONST from '@src/CONST'; +import {apply, doesClientNeedToBeUpdated} from '@src/libs/actions/OnyxUpdates'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; + +import type {OnyxKey} from 'react-native-onyx'; + +import Onyx from 'react-native-onyx'; + +import getOnyxValue from '../utils/getOnyxValue'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +const pusherUpdate = (previousUpdateID: number, lastUpdateID: number): OnyxUpdatesFromServer => ({ + type: CONST.ONYX_UPDATE_TYPES.PUSHER, + previousUpdateID, + lastUpdateID, + updates: [{eventType: 'onyxApiUpdate', data: []}], +}); + +// A rejected Pusher apply leaves the module-scoped pusherEventsPromise rejected for the rest of the module's life, +// so this lives in its own file rather than poisoning the chain for the other tests. +describe('OnyxUpdates, when a Pusher apply fails', () => { + beforeAll(() => { + Onyx.init({ + keys: ONYXKEYS, + }); + }); + + beforeEach(() => Onyx.clear().then(waitForBatchedUpdates)); + + it('relies on pusherEventsPromise staying rejected to stop a follower whose gap check the failed update had suppressed', async () => { + // Given the client is caught up to update 10 and update 20 from Pusher is held mid-apply + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 10); + await waitForBatchedUpdates(); + + let failHeldApply: (error: Error) => void = () => {}; + let handlerCallCount = 0; + const handlerSpy = jest.spyOn(PusherUtils, 'triggerMultiEventHandler').mockImplementation(() => { + handlerCallCount += 1; + if (handlerCallCount > 1) { + return Promise.resolve(); + } + return new Promise((resolve, reject) => { + failHeldApply = reject; + }); + }); + const heldApply = apply(pusherUpdate(10, 20)); + await waitForBatchedUpdates(); + + // When update 30 arrives chained on it, so the pending marker tells it there is no gap + expect(doesClientNeedToBeUpdated({previousUpdateID: 20, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(false); + const followerApply = apply(pusherUpdate(20, 30)); + await waitForBatchedUpdates(); + + // And update 20 then fails to apply + failHeldApply(new Error('storage write failed')); + await expect(heldApply).rejects.toThrow('storage write failed'); + await expect(followerApply).rejects.toThrow('storage write failed'); + await waitForBatchedUpdates(); + + // Then update 30 is never written either. Nothing checks that update IDs are contiguous before advancing the + // watermark, so were it written the watermark would move to 30 and updates 11 to 20 would be lost with no gap + // left to trigger recovery. Serializing on pusherEventsPromise is the only thing preventing that. + expect(handlerCallCount).toBe(1); + expect(await getOnyxValue(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT)).toBe(10); + + // And both updates are back in the gap, so recovery can refetch them + expect(doesClientNeedToBeUpdated({previousUpdateID: 20, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); + expect(doesClientNeedToBeUpdated({previousUpdateID: 30, updateType: CONST.ONYX_UPDATE_TYPES.PUSHER})).toBe(true); + + handlerSpy.mockRestore(); + }); +}); From 275c1a54be44961db32c19b28f84826c39c08cee Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Tue, 8 Sep 2026 14:36:57 +0200 Subject: [PATCH 8/9] Name the Pusher gap-check gate and restore the deferred-write note --- src/libs/actions/OnyxUpdates.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 0ac0e66b29e6..4555fc82ba63 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -285,6 +285,10 @@ type DoesClientNeedToBeUpdatedParams = { updateType?: AnyOnyxUpdatesFromServer['type']; }; +function isSerializedBehindPusherApply(updateType?: AnyOnyxUpdatesFromServer['type']): boolean { + return updateType === CONST.ONYX_UPDATE_TYPES.PUSHER; +} + /** * This function will receive the previousUpdateID from any request/pusher update that has it, compare to our current app state * and return if an update is needed @@ -298,10 +302,11 @@ function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID, update return false; } + // QueuedOnyxUpdates defers the Onyx write for WRITE requests, so their own responses arrive before the watermark moves. const lastUpdateIDFromClient = Math.max( clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush, - updateType === CONST.ONYX_UPDATE_TYPES.PUSHER ? lastUpdateIDPendingApply : 0, + isSerializedBehindPusherApply(updateType) ? lastUpdateIDPendingApply : 0, ); // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp From 9c0c59c43708f36c518d66ae27768e395bf6267b Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 9 Sep 2026 12:17:46 +0200 Subject: [PATCH 9/9] rename variables to address the review round --- src/libs/actions/OnyxUpdates.ts | 34 ++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libs/actions/OnyxUpdates.ts b/src/libs/actions/OnyxUpdates.ts index 4555fc82ba63..5ac284da37ea 100644 --- a/src/libs/actions/OnyxUpdates.ts +++ b/src/libs/actions/OnyxUpdates.ts @@ -22,12 +22,12 @@ let lastUpdateIDAppliedToClient: number | undefined = 0; // Highest update ID staged for the deferred WRITE flush but not yet persisted. Gap detection treats these as // applied so queued WRITE responses don't look like gaps; reset if the flush fails so recovery can kick in. -let lastUpdateIDPendingFlush = 0; +let lastUpdateIDPendingWriteFlush = 0; -let lastUpdateIDPendingApply = 0; +let lastUpdateIDPendingPusherApply = 0; function getEffectiveLastUpdateID(): number { - return Math.max(lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingFlush); + return Math.max(lastUpdateIDAppliedToClient ?? 0, lastUpdateIDPendingWriteFlush); } function getPersistedLastUpdateID(): number { @@ -43,8 +43,8 @@ Onyx.connectWithoutView({ // The persisted watermark is only ever cleared by Onyx.clear (sign-out), so drop the pending marker // too — a stale value from the previous session would mask real gaps after signing back in. if (val === undefined) { - lastUpdateIDPendingFlush = 0; - lastUpdateIDPendingApply = 0; + lastUpdateIDPendingWriteFlush = 0; + lastUpdateIDPendingPusherApply = 0; } }, }); @@ -206,18 +206,18 @@ function apply({lastUpdateID, type, request, response, upd Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Number(lastUpdateID)); } // The persisted watermark now covers the staged WRITE updates, so the pending marker is no longer needed - if (lastUpdateIDPendingFlush && lastUpdateIDPendingFlush <= Number(lastUpdateID)) { - lastUpdateIDPendingFlush = 0; + if (lastUpdateIDPendingWriteFlush && lastUpdateIDPendingWriteFlush <= Number(lastUpdateID)) { + lastUpdateIDPendingWriteFlush = 0; } - if (lastUpdateIDPendingApply && lastUpdateIDPendingApply <= Number(lastUpdateID)) { - lastUpdateIDPendingApply = 0; + if (lastUpdateIDPendingPusherApply && lastUpdateIDPendingPusherApply <= Number(lastUpdateID)) { + lastUpdateIDPendingPusherApply = 0; } return result; }) .catch((error) => { - // Cleared for any failed apply, not just Pusher: the marker is a flat max, so keeping it after an - // unrelated lower-ID failure would mask that gap. Errs toward a redundant refetch. - lastUpdateIDPendingApply = 0; + // Intentionally cleared for any failed apply, including HTTPS and Airship: the marker is a flat max, so + // keeping it after an unrelated lower-ID failure would mask that gap. Errs toward a redundant refetch. + lastUpdateIDPendingPusherApply = 0; if (shouldAdvanceLastUpdateID) { Log.alert('[OnyxUpdateManagerError] Applying the updates failed, not advancing lastUpdateID so the client can recover on the next reconnect', { @@ -239,12 +239,12 @@ function apply({lastUpdateID, type, request, response, upd // SequentialQueue only flushes after this promise settles, so awaiting the flush here would deadlock. if (request.data?.apiRequestType === CONST.API_REQUEST_TYPE.WRITE) { if (shouldAdvanceLastUpdateID) { - lastUpdateIDPendingFlush = Math.max(lastUpdateIDPendingFlush, Number(lastUpdateID)); + lastUpdateIDPendingWriteFlush = Math.max(lastUpdateIDPendingWriteFlush, Number(lastUpdateID)); } advanceLastUpdateIDAfterApply(applyPromise.then(() => getCurrentFlushPromise())).catch(() => { // The staged updates never applied, so stop counting them as pending — the next gap check // then sees the missing range against the persisted watermark and triggers recovery. - lastUpdateIDPendingFlush = 0; + lastUpdateIDPendingWriteFlush = 0; }); return applyPromise; } @@ -252,7 +252,7 @@ function apply({lastUpdateID, type, request, response, upd } if (type === CONST.ONYX_UPDATE_TYPES.PUSHER && updates) { if (shouldAdvanceLastUpdateID) { - lastUpdateIDPendingApply = Math.max(lastUpdateIDPendingApply, Number(lastUpdateID)); + lastUpdateIDPendingPusherApply = Math.max(lastUpdateIDPendingPusherApply, Number(lastUpdateID)); } return advanceLastUpdateIDAfterApply(applyPusherOnyxUpdates(updates, Number(lastUpdateID))); @@ -305,8 +305,8 @@ function doesClientNeedToBeUpdated({previousUpdateID, clientLastUpdateID, update // QueuedOnyxUpdates defers the Onyx write for WRITE requests, so their own responses arrive before the watermark moves. const lastUpdateIDFromClient = Math.max( clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0, - lastUpdateIDPendingFlush, - isSerializedBehindPusherApply(updateType) ? lastUpdateIDPendingApply : 0, + lastUpdateIDPendingWriteFlush, + isSerializedBehindPusherApply(updateType) ? lastUpdateIDPendingPusherApply : 0, ); // If we don't have any value in lastUpdateIDFromClient, this is the first time we're receiving anything, so we need to do a last reconnectApp