fix: prevent deadlock for batchinserting usage metrics (#1100)

* fix: prevent deadlock for batchinserting usage metrics

In client metrics v2 we utilize postgres to count the usage
across a few dimentions (featureName, app_name, environment).

It turns out that if the UPDATE values are not executed in a predictable
order we can end up in a deadlock scenario with postgresql.

In this fix we thus sort the metrics on the feature_name, app_name and
envrionment, to make sure they always are executed in a predictabel
order, and thus avoiding independent inserts colliding in to a deadlock
waiting for eachother.

* fix: tests cannot assume order

src/lib/db/client-metrics-store-v2.ts

@@ -116,9 +116,17 @@ export class ClientMetricsStoreV2 implements IClientMetricsStoreV2 {
             return prev;
         }, {});
 
+        // Sort the rows to avoid deadlocks
+        const batchRow = Object.values<ClientMetricsEnvTable>(batch).sort(
+            (a, b) =>
+                a.feature_name.localeCompare(b.feature_name) ||
+                a.app_name.localeCompare(b.app_name) ||
+                a.environment.localeCompare(b.environment),
+        );
+
         // Consider rewriting to SQL batch!
         const insert = this.db<ClientMetricsEnvTable>(TABLE)
-            .insert(Object.values(batch))
+            .insert(batchRow)
             .toQuery();
 
         const query = `${insert.toString()} ON CONFLICT (feature_name, app_name, environment, timestamp) DO UPDATE SET "yes" = "client_metrics_env"."yes" + EXCLUDED.yes, "no" = "client_metrics_env"."no" + EXCLUDED.no`;

src/test/e2e/api/admin/client-metrics.e2e.test.ts

@@ -155,14 +155,19 @@ test('should return toggle summary', async () => {
         .expect('Content-Type', /json/)
         .expect(200);
 
+    const test = demo.lastHourUsage.find((u) => u.environment === 'test');
+    const defaultEnv = demo.lastHourUsage.find(
+        (u) => u.environment === 'default',
+    );
+
     expect(demo.featureName).toBe('demo');
     expect(demo.lastHourUsage).toHaveLength(2);
-    expect(demo.lastHourUsage[0].environment).toBe('default');
+    expect(test.environment).toBe('test');
-    expect(demo.lastHourUsage[0].yes).toBe(5);
+    expect(test.yes).toBe(2);
-    expect(demo.lastHourUsage[0].no).toBe(4);
+    expect(test.no).toBe(6);
-    expect(demo.lastHourUsage[1].environment).toBe('test');
+    expect(defaultEnv.environment).toBe('default');
-    expect(demo.lastHourUsage[1].yes).toBe(2);
+    expect(defaultEnv.yes).toBe(5);
-    expect(demo.lastHourUsage[1].no).toBe(6);
+    expect(defaultEnv.no).toBe(4);
     expect(demo.seenApplications).toStrictEqual(['backend-api', 'web']);
 });
 
@@ -219,13 +224,18 @@ test('should only include last hour of metrics return toggle summary', async ()
         .expect('Content-Type', /json/)
         .expect(200);
 
+    const test = demo.lastHourUsage.find((u) => u.environment === 'test');
+    const defaultEnv = demo.lastHourUsage.find(
+        (u) => u.environment === 'default',
+    );
+
     expect(demo.featureName).toBe('demo');
     expect(demo.lastHourUsage).toHaveLength(2);
-    expect(demo.lastHourUsage[0].environment).toBe('default');
+    expect(defaultEnv.environment).toBe('default');
-    expect(demo.lastHourUsage[0].yes).toBe(5);
+    expect(defaultEnv.yes).toBe(5);
-    expect(demo.lastHourUsage[0].no).toBe(4);
+    expect(defaultEnv.no).toBe(4);
-    expect(demo.lastHourUsage[1].environment).toBe('test');
+    expect(test.environment).toBe('test');
-    expect(demo.lastHourUsage[1].yes).toBe(2);
+    expect(test.yes).toBe(2);
-    expect(demo.lastHourUsage[1].no).toBe(6);
+    expect(test.no).toBe(6);
     expect(demo.seenApplications).toStrictEqual(['backend-api', 'web']);
 });