Fix: OnyxUpdateManager ends up in a recursive endless loop, when previously applied Onyx updates are not chained (caused crashes on web)

src/libs/actions/OnyxUpdateManager/index.ts

@@ -75,7 +75,7 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry<OnyxUpdatesFromSer
         // When ONYX_UPDATES_FROM_SERVER is set, we pause the queue. Let's unpause
         // it so the app is not stuck forever without processing requests.
         SequentialQueue.unpause();
-        console.debug(`[OnyxUpdateManager] Ignoring Onyx updates while OpenApp hans't finished yet.`);
+        console.debug(`[OnyxUpdateManager] Ignoring Onyx updates while OpenApp hasn't finished yet.`);
         return;
     }
     // This key is shared across clients, thus every client/tab will have a copy and try to execute this method.

src/libs/actions/OnyxUpdateManager/utils/index.ts

@@ -33,37 +33,54 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdate
     for (const [index, update] of updateValues.entries()) {
         const isFirst = index === 0;
 
+        const lastUpdateID = Number(update.lastUpdateID);
+        const previousUpdateID = Number(update.previousUpdateID);
+
+        // If an update is older than the last update that was applied to the client, we can skip it
+        // This should already be handled before, but we'll keep this as a safeguard.
+        if (lastUpdateID <= lastUpdateIDFromClient) {
+            // eslint-disable-next-line no-continue
+            continue;
+        }
+
         // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap.
         // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient.
         // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates.
         // If an update is chained, we can add it to the applicable updates.
-        const isChained = isFirst ? update.previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)];
+        const isChained = isFirst ? previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)];
         if (isChained) {
             // If a gap exists already, we will not add any more updates to the applicable updates.
             // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap.
             if (gapExists) {
                 // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps.
                 if (!firstUpdateAfterGaps) {
-                    firstUpdateAfterGaps = Number(update.previousUpdateID);
+                    firstUpdateAfterGaps = previousUpdateID;
                 }
             } else {
                 // If no gap exists yet, we can add the update to the applicable updates
-                applicableUpdates[Number(update.lastUpdateID)] = update;
+                applicableUpdates[lastUpdateID] = update;
             }
         } else {
+            // If the previousUpdateID of the update after the gap is older than the lastUpdateIDFromClient,
+            // we don't want to detect a missing update, since this would cause a recursion loop in "validateAndApplyDeferredUpdates".
+            if (previousUpdateID <= lastUpdateIDFromClient) {
+                // eslint-disable-next-line no-continue
+                continue;
+            }
+
             // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable,
             // so that we can continue searching for the next update after all gaps
             gapExists = true;
             firstUpdateAfterGaps = undefined;
 
             // If there is a gap, it means the previous update is the latest missing update.
-            latestMissingUpdateID = Number(update.previousUpdateID);
+            latestMissingUpdateID = previousUpdateID;
         }
     }
 
     // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list,
     // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates.
-    const firstUpdateAfterGapWithFallback = firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID);
+    const firstUpdateAfterGapWithFallback = Math.max(firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID), lastUpdateIDFromClient);
 
     let updatesAfterGaps: DeferredUpdatesDictionary = {};
     if (gapExists) {
@@ -99,8 +116,8 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa
 
     const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates, lastUpdateIDFromClient);
 
-    // If we detected a gap in the deferred updates, only apply the deferred updates before the gap,
-    // re-fetch the missing updates and then apply the remaining deferred updates after the gap
+    // If newer updates got applied in the meantime, we don't need to refetch for missing updates
+    // and can just re-trigger the "validateAndApplyDeferredUpdates" process
     if (latestMissingUpdateID) {
         Log.info('[DeferredUpdates] Gap detected in deferred updates', false, {lastUpdateIDFromClient, latestMissingUpdateID});
 

tests/unit/OnyxUpdateManagerTest.ts

@@ -18,6 +18,7 @@ const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock;
 const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock;
 
 const mockUpdate3 = createOnyxMockUpdate(3);
+const offsetedMockUpdate3 = createOnyxMockUpdate(3, undefined, 2);
 const mockUpdate4 = createOnyxMockUpdate(4);
 const mockUpdate5 = createOnyxMockUpdate(5);
 const mockUpdate6 = createOnyxMockUpdate(6);
@@ -193,7 +194,7 @@ describe('OnyxUpdateManager', () => {
             // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch.
             expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);
 
-            // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.
+            // validateAndApplyDeferredUpdates should be called once for the initial deferred updates
             // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself.
             // The intended assertion would look like this:
             // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1);
@@ -254,4 +255,28 @@ describe('OnyxUpdateManager', () => {
             expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7});
         });
     });
+
+    it('should only fetch missing updates that are not stale', () => {
+        OnyxUpdateManager.handleOnyxUpdateGap(offsetedMockUpdate3);
+        OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4);
+
+        return OnyxUpdateManager.queryPromise.then(() => {
+            // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 4.
+            expect(lastUpdateIDAppliedToClient).toBe(4);
+
+            // validateAndApplyDeferredUpdates should be called once for the initial deferred updates
+            // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself.
+            // The intended assertion would look like this:
+            // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1);
+
+            // There should be only one call to applyUpdates. The call should contain all the deferred update,
+            // since the locally applied updates have changed in the meantime.
+            expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1);
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: offsetedMockUpdate3, 4: mockUpdate4});
+
+            // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered
+            expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);
+        });
+    });
 });

tests/utils/createOnyxMockUpdate.ts

@@ -1,10 +1,10 @@
 import type {OnyxUpdate} from 'react-native-onyx';
 import type {OnyxUpdatesFromServer} from '@src/types/onyx';
 
-const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = []): OnyxUpdatesFromServer => ({
+const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = [], previousUpdateIDOffset = 1): OnyxUpdatesFromServer => ({
     type: 'https',
     lastUpdateID,
-    previousUpdateID: lastUpdateID - 1,
+    previousUpdateID: lastUpdateID - previousUpdateIDOffset,
     request: {
         command: 'TestCommand',
         successData,
@@ -15,7 +15,7 @@ const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] =
     response: {
         jsonCode: 200,
         lastUpdateID,
-        previousUpdateID: lastUpdateID - 1,
+        previousUpdateID: lastUpdateID - previousUpdateIDOffset,
         onyxData: successData,
     },
 });