fix(analytics-browser): should send batches sequentially (#935)

amplitude · Dec 17, 2024 · ad319d6 · ad319d6
1 parent 368f7d3
commit ad319d6
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 3 deletions.
diff --git a/packages/analytics-core/src/plugins/destination.ts b/packages/analytics-core/src/plugins/destination.ts
@@ -151,7 +151,12 @@ export class Destination implements DestinationPlugin {
 
     const batches = chunk(list, this.config.flushQueueSize);
 
-    await Promise.all(batches.map((batch) => this.send(batch, useRetry)));
+    // Promise.all() doesn't guarantee resolve order.
+    // Sequentially resolve to make sure backend receives events in order
+    await batches.reduce(async (promise, batch) => {
+      await promise;
+      return await this.send(batch, useRetry);
+    }, Promise.resolve());
 
     this.scheduleTryable(later);
   }

diff --git a/packages/analytics-core/test/plugins/destination.test.ts b/packages/analytics-core/test/plugins/destination.test.ts
@@ -321,6 +321,57 @@ describe('destination', () => {
       expect(result).toBe(undefined);
       expect(send).toHaveBeenCalledTimes(0);
     });
+    test('should send batches in order', async () => {
+      const destination = new Destination();
+      destination.config = {
+        ...useDefaultConfig(),
+        flushQueueSize: 1,
+      };
+
+      const context1 = {
+        event: { event_type: 'event_type_1' },
+        attempts: 0,
+        callback: () => undefined,
+        timeout: 0,
+      };
+      const context2 = {
+        event: { event_type: 'event_type_2' },
+        attempts: 0,
+        callback: () => undefined,
+        timeout: 0,
+      };
+      destination.queue = [context1, context2];
+
+      const resolveOrder: number[] = [];
+      const send = jest
+        .spyOn(destination, 'send')
+        .mockImplementationOnce(
+          () =>
+            new Promise((resolve) =>
+              setTimeout(() => {
+                resolveOrder.push(1);
+                resolve();
+              }, 1000),
+            ),
+        ) // 1st call resolves in 1 sec
+        .mockImplementationOnce(
+          () =>
+            new Promise((resolve) =>
+              setTimeout(() => {
+                resolveOrder.push(2);
+                resolve();
+              }, 500),
+            ),
+        ); // 2nd call resolves in 0.5 sec
+
+      const result = await destination.flush();
+
+      expect(result).toBe(undefined);
+      expect(send).toHaveBeenNthCalledWith(1, [context1], false);
+      expect(send).toHaveBeenNthCalledWith(2, [context2], false);
+      expect(send).toHaveBeenCalledTimes(2);
+      expect(resolveOrder).toEqual([1, 2]);
+    });
   });
 
   describe('send', () => {
@@ -1233,9 +1284,10 @@ describe('destination', () => {
           code: 200,
         });
 
-        expect(transportProvider.send).toHaveBeenCalledTimes(eventCount + 1);
+        // When 413, retry of the second batch won't be recorded here as it's async
+        expect(transportProvider.send).toHaveBeenCalledTimes(2);
         // eslint-disable-next-line @typescript-eslint/unbound-method
-        expect(loggerProvider.warn).toHaveBeenCalledTimes(eventCount);
+        expect(loggerProvider.warn).toHaveBeenCalledTimes(1);
         // eslint-disable-next-line @typescript-eslint/unbound-method,@typescript-eslint/restrict-template-expressions
         expect(loggerProvider.warn).toHaveBeenCalledWith(jsons(response.body));
       },