#9211 Retry on failed to read large indexeddb value (#9229)

* make operationName part of a config object * implement first retry * fix errors from type change * update error messaging; add handling * move shouldRetry to the callsites * document isIDBLargeValueError * improve comment * handle retry in test * remove retry flag and depend on shouldRetry instead * clear db on large value error * adds requested comments --------- Co-authored-by: Eduardo <[email protected]>
pixiebrix · Oct 4, 2024 · 3ba3f57 · 3ba3f57
1 parent 41b948a
commit 3ba3f57
Show file tree

Hide file tree

Showing 6 changed files with 381 additions and 232 deletions.
diff --git a/src/background/telemetry.ts b/src/background/telemetry.ts
@@ -204,18 +204,24 @@ const withTelemetryDB = withIdbErrorHandling(
 );
 
 async function addEvent(event: UserTelemetryEvent): Promise<void> {
-  await withTelemetryDB(async (db) => {
-    await db.add(TELEMETRY_EVENT_OBJECT_STORE, event);
-  }, IDB_OPERATION[DATABASE_NAME.TELEMETRY].ADD_EVENT);
+  await withTelemetryDB(
+    async (db) => {
+      await db.add(TELEMETRY_EVENT_OBJECT_STORE, event);
+    },
+    { operationName: IDB_OPERATION[DATABASE_NAME.TELEMETRY].ADD_EVENT },
+  );
 }
 
 export async function flushEvents(): Promise<UserTelemetryEvent[]> {
-  return withTelemetryDB(async (db) => {
-    const tx = db.transaction(TELEMETRY_EVENT_OBJECT_STORE, "readwrite");
-    const allEvents = await tx.store.getAll();
-    await tx.store.clear();
-    return allEvents;
-  }, IDB_OPERATION[DATABASE_NAME.TELEMETRY].FLUSH_EVENTS);
+  return withTelemetryDB(
+    async (db) => {
+      const tx = db.transaction(TELEMETRY_EVENT_OBJECT_STORE, "readwrite");
+      const allEvents = await tx.store.getAll();
+      await tx.store.clear();
+      return allEvents;
+    },
+    { operationName: IDB_OPERATION[DATABASE_NAME.TELEMETRY].FLUSH_EVENTS },
+  );
 }
 
 /**
@@ -224,29 +230,30 @@ export async function flushEvents(): Promise<UserTelemetryEvent[]> {
 export async function recreateDB(): Promise<void> {
   await deleteDatabase(DATABASE_NAME.TELEMETRY);
 
-  await withTelemetryDB(
-    async () => {},
-    IDB_OPERATION[DATABASE_NAME.TELEMETRY].RECREATE_DB,
-  );
+  await withTelemetryDB(async () => {}, {
+    operationName: IDB_OPERATION[DATABASE_NAME.TELEMETRY].RECREATE_DB,
+  });
 }
 
 /**
  * Returns the number of telemetry entries in the database.
  */
 export async function count(): Promise<number> {
-  return withTelemetryDB(
-    async (db) => db.count(TELEMETRY_EVENT_OBJECT_STORE),
-    IDB_OPERATION[DATABASE_NAME.TELEMETRY].COUNT,
-  );
+  return withTelemetryDB(async (db) => db.count(TELEMETRY_EVENT_OBJECT_STORE), {
+    operationName: IDB_OPERATION[DATABASE_NAME.TELEMETRY].COUNT,
+  });
 }
 
 /**
  * Clears all event entries from the database.
  */
 export async function clear(): Promise<void> {
-  await withTelemetryDB(async (db) => {
-    await db.clear(TELEMETRY_EVENT_OBJECT_STORE);
-  }, IDB_OPERATION[DATABASE_NAME.TELEMETRY].CLEAR);
+  await withTelemetryDB(
+    async (db) => {
+      await db.clear(TELEMETRY_EVENT_OBJECT_STORE);
+    },
+    { operationName: IDB_OPERATION[DATABASE_NAME.TELEMETRY].CLEAR },
+  );
 }
 
 async function flush(): Promise<void> {

diff --git a/src/registry/packageRegistry.ts b/src/registry/packageRegistry.ts
@@ -24,6 +24,8 @@ import {
   DATABASE_NAME,
   withIdbErrorHandling,
   IDB_OPERATION,
+  isIDBLargeValueError,
+  isMaybeTemporaryIDBError,
 } from "@/utils/idbUtils";
 import { PACKAGE_REGEX } from "@/types/helpers";
 import { memoizeUntilSettled } from "@/utils/promiseUtils";
@@ -174,9 +176,12 @@ const ensurePopulated = memoizeUntilSettled(async () => {
  * Clear the brick definition registry.
  */
 export async function clear(): Promise<void> {
-  await withRegistryDB(async (db) => {
-    await db.clear(BRICK_STORE);
-  }, IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].CLEAR);
+  await withRegistryDB(
+    async (db) => {
+      await db.clear(BRICK_STORE);
+    },
+    { operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].CLEAR },
+  );
 }
 
 /**
@@ -186,10 +191,9 @@ export async function recreateDB(): Promise<void> {
   await deleteDatabase(DATABASE_NAME.PACKAGE_REGISTRY);
 
   // Open the database to recreate it
-  await withRegistryDB(
-    async () => {},
-    IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].RECREATE_DB,
-  );
+  await withRegistryDB(async () => {}, {
+    operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].RECREATE_DB,
+  });
 
   // Re-populate the packages from the remote registry
   await syncPackages();
@@ -204,47 +208,64 @@ export async function getByKinds(
 ): Promise<PackageVersion[]> {
   await ensurePopulated();
 
-  return withRegistryDB(async (db) => {
-    const bricks = flatten(
-      await Promise.all(
-        kinds.map(async (kind) =>
-          db.getAllFromIndex(BRICK_STORE, "kind", kind),
+  return withRegistryDB(
+    async (db) => {
+      const bricks = flatten(
+        await Promise.all(
+          kinds.map(async (kind) =>
+            db.getAllFromIndex(BRICK_STORE, "kind", kind),
+          ),
         ),
-      ),
-    );
-
-    return Object.entries(groupBy(bricks, (x) => x.id)).map(
-      ([, versions]) =>
-        // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- there's at least one element per group
-        latestVersion(versions)!,
-    );
-  }, IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].GET_BY_KINDS);
+      );
+
+      return Object.entries(groupBy(bricks, (x) => x.id)).map(
+        ([, versions]) =>
+          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- there's at least one element per group
+          latestVersion(versions)!,
+      );
+    },
+    {
+      operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].GET_BY_KINDS,
+      shouldRetry: (error) => isMaybeTemporaryIDBError(error),
+      async onRetry(error) {
+        if (isIDBLargeValueError(error)) {
+          // If the large value error is a NotFoundError, syncPackages will likely fix it
+          // In a future version of Chrome, we will be able to distinguish between NotFoundErrors and DataErrors
+          await syncPackages();
+        }
+      },
+    },
+  );
 }
 
 /**
  * Return the number of records in the registry.
  */
 export async function count(): Promise<number> {
   // Don't need to ensure populated, because we're just counting current records
-  return withRegistryDB(
-    async (db) => db.count(BRICK_STORE),
-    IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].COUNT,
-  );
+  return withRegistryDB(async (db) => db.count(BRICK_STORE), {
+    operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].COUNT,
+  });
 }
 
 /**
  * Replace all packages in the local database.
  * @param packages the packages to put in the database
  */
 async function replaceAll(packages: PackageVersion[]): Promise<void> {
-  await withRegistryDB(async (db) => {
-    const tx = db.transaction(BRICK_STORE, "readwrite");
+  await withRegistryDB(
+    async (db) => {
+      const tx = db.transaction(BRICK_STORE, "readwrite");
 
-    await tx.store.clear();
-    await Promise.all(packages.map(async (obj) => tx.store.add(obj)));
+      await tx.store.clear();
+      await Promise.all(packages.map(async (obj) => tx.store.add(obj)));
 
-    await tx.done;
-  }, IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].REPLACE_ALL);
+      await tx.done;
+    },
+    {
+      operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].REPLACE_ALL,
+    },
+  );
 }
 
 export function parsePackage(
@@ -290,9 +311,12 @@ export async function find(id: string): Promise<Nullishable<PackageVersion>> {
 
   await ensurePopulated();
 
-  return withRegistryDB(async (db) => {
-    const versions = await db.getAllFromIndex(BRICK_STORE, "id", id);
+  return withRegistryDB(
+    async (db) => {
+      const versions = await db.getAllFromIndex(BRICK_STORE, "id", id);
 
-    return latestVersion(versions);
-  }, IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].FIND);
+      return latestVersion(versions);
+    },
+    { operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].FIND },
+  );
 }
diff --git a/src/telemetry/logging.test.ts b/src/telemetry/logging.test.ts
@@ -104,8 +104,9 @@ describe("logging", () => {
     await expect(appendEntry(logEntryFactory())).toResolve();
 
     await waitFor(() => {
-      expect(sendMessageSpy).toHaveBeenCalledOnce();
+      expect(sendMessageSpy).toHaveBeenCalled();
     });
+
     expect(sendMessageSpy).toHaveBeenCalledWith(
       expect.objectContaining({
         target: "offscreen-doc",