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

@@ -16,62 +16,93 @@ Onyx.connect({
 /**
  * In order for the deferred updates to be applied correctly in order,
  * we need to check if there are any gaps between deferred updates.
+ * If there are gaps, we need to split the deferred updates into two parts:
+ * 1. The applicable updates that can be applied immediately
+ * 2. The updates after the gaps that need to be fetched and applied first
  * @param updates The deferred updates to be checked for gaps
- * @param clientLastUpdateID An optional lastUpdateID passed to use instead of the lastUpdateIDAppliedToClient
+ * @param lastUpdateIDFromClient An optional lastUpdateID passed to use instead of the lastUpdateIDAppliedToClient
  * @returns
  */
-function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdateID?: number): DetectGapAndSplitResult {
-    const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0;
+function detectGapsAndSplit(lastUpdateIDFromClient: number): DetectGapAndSplitResult {
+    // We only want to apply deferred updates that are newer than the last update that was applied to the client.
+    // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already,
+    // so we can safely filter out any outdated deferred updates.
+    const pendingDeferredUpdates = DeferredOnyxUpdates.getUpdates({minUpdateID: lastUpdateIDFromClient});
+
+    // If there are no remaining deferred updates after filtering out outdated updates,
+    // we don't need to iterate over the deferred updates and check for gaps.
+    if (Object.values(pendingDeferredUpdates).length === 0) {
+        return {applicableUpdates: {}, updatesAfterGaps: {}, latestMissingUpdateID: undefined};
+    }
 
-    const updateValues = Object.values(updates);
+    const updateValues = Object.values(pendingDeferredUpdates);
     const applicableUpdates: DeferredUpdatesDictionary = {};
 
     let gapExists = false;
-    let firstUpdateAfterGaps: number | undefined;
+    let firstUpdateIDAfterGaps: number | undefined;
     let latestMissingUpdateID: number | undefined;
 
     for (const [index, update] of updateValues.entries()) {
         const isFirst = index === 0;
 
-        // 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.
+        const lastUpdateID = Number(update.lastUpdateID);
+        const previousUpdateID = Number(update.previousUpdateID);
+
+        // Whether the previous update (of the current update) is outdated, because there was a newer update applied to the client.
+        const isPreviousUpdateAlreadyApplied = previousUpdateID <= lastUpdateIDFromClient;
+
+        // If any update's previousUpdateID doesn't match the lastUpdateID of the previous update,
+        // the deferred updates aren't chained and we detected a gap.
+        // For the first update, we need to check that the previousUpdateID of the current 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)];
-        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.
+        const isChainedToPreviousUpdate = isFirst ? isPreviousUpdateAlreadyApplied : !!pendingDeferredUpdates[previousUpdateID];
+        if (isChainedToPreviousUpdate) {
+            // If we found a gap in the deferred updates, we will not add any more updates to the applicable updates.
+            // Instead, if we find two chained updates, we can set "firstUpdateIDAfterGaps" to the first update after the 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);
+                // If there was a gap, "firstUpdateIDAfterGaps" isn't set and we find two chained updates,
+                // we need to set "firstUpdateIDAfterGaps" to the first update after the gap.
+                if (!firstUpdateIDAfterGaps) {
+                    firstUpdateIDAfterGaps = 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 {
-            // 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
+            // If a previous update has already been applied to the client we should not detect a gap.
+            // This can cause a recursion loop, because "validateAndApplyDeferredUpdates" will refetch
+            // missing updates up to the previous update, which will then be applied again.
+            if (isPreviousUpdateAlreadyApplied) {
+                // eslint-disable-next-line no-continue
+                continue;
+            }
+
+            // When we find a (new) gap, we initially need to set "gapExists" to true
+            // and reset "firstUpdateIDAfterGaps" and continue searching the first update after all gaps.
             gapExists = true;
-            firstUpdateAfterGaps = undefined;
+            firstUpdateIDAfterGaps = undefined;
 
-            // If there is a gap, it means the previous update is the latest missing update.
-            latestMissingUpdateID = Number(update.previousUpdateID);
+            // We need to set update the latest missing update to the previous update of the current unchained update.
+            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);
-
-    let updatesAfterGaps: DeferredUpdatesDictionary = {};
+    const updatesAfterGaps: DeferredUpdatesDictionary = {};
     if (gapExists) {
-        updatesAfterGaps = Object.entries(updates).reduce<DeferredUpdatesDictionary>((acc, [lastUpdateID, update]) => {
-            if (Number(lastUpdateID) >= firstUpdateAfterGapWithFallback) {
-                acc[Number(lastUpdateID)] = update;
+        // If there is a gap and we didn't detect two chained updates, "firstUpdateToBeAppliedAfterGap" will always be the the last deferred update.
+        // We will fetch all missing updates up to the previous update and can always apply the last deferred update.
+        const firstUpdateToBeAppliedAfterGap = firstUpdateIDAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID);
+
+        // Add all deferred updates after the gap(s) to "updatesAfterGaps".
+        // If "firstUpdateToBeAppliedAfterGap" is set to the last deferred update, the array will be empty.
+        Object.entries(pendingDeferredUpdates).forEach(([lastUpdateID, update]) => {
+            if (Number(lastUpdateID) < firstUpdateToBeAppliedAfterGap) {
+                return;
             }
-            return acc;
+
+            updatesAfterGaps[Number(lastUpdateID)] = update;
         }, {});
     }
 
@@ -87,20 +118,16 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa
 
     Log.info('[DeferredUpdates] Processing deferred updates', false, {lastUpdateIDFromClient, previousParams});
 
-    // We only want to apply deferred updates that are newer than the last update that was applied to the client.
-    // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out.
-    const pendingDeferredUpdates = DeferredOnyxUpdates.getUpdates({minUpdateID: lastUpdateIDFromClient});
+    const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(lastUpdateIDFromClient);
 
-    // If there are no remaining deferred updates after filtering out outdated ones,
-    // we can just unpause the queue and return
-    if (Object.values(pendingDeferredUpdates).length === 0) {
+    // If there are no applicable deferred updates and no missing deferred updates,
+    // we don't need to apply or re-fetch any updates. We can just unpause the queue by resolving.
+    if (Object.values(applicableUpdates).length === 0 && latestMissingUpdateID === undefined) {
         return Promise.resolve();
     }
 
-    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, we don't need to refetch for missing updates
+    // and will re-trigger the "validateAndApplyDeferredUpdates" process
     if (latestMissingUpdateID) {
         Log.info('[DeferredUpdates] Gap detected in deferred updates', false, {lastUpdateIDFromClient, latestMissingUpdateID});
 
@@ -117,7 +144,8 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa
 
                 DeferredOnyxUpdates.enqueue(updatesAfterGaps, {shouldPauseSequentialQueue: false});
 
-                // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates.
+                // If lastUpdateIDAppliedToClient got updated, we will just retrigger the validation
+                // and application of the current deferred updates.
                 if (latestMissingUpdateID <= newLastUpdateIDFromClient) {
                     validateAndApplyDeferredUpdates(undefined, {newLastUpdateIDFromClient, latestMissingUpdateID})
                         .then(() => resolve(undefined))

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 outdated (older than already locally applied update)', () => {
+        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,
     },
 });