fix idb error retry behavior (#9231)

src/registry/packageRegistry.ts

@@ -227,7 +227,7 @@ export async function getByKinds(
     {
       operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].GET_BY_KINDS,
       shouldRetry: (error) => isMaybeTemporaryIDBError(error),
-      async onRetry(error) {
+      async onFailedAttempt(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
@@ -304,11 +304,6 @@ export async function find(id: string): Promise<Nullishable<PackageVersion>> {
     throw new Error("id is required");
   }
 
-  if (typeof id !== "string") {
-    console.error("REGISTRY_FIND received invalid id argument", { id });
-    throw new Error("invalid brick id");
-  }
-
   await ensurePopulated();
 
   return withRegistryDB(

src/utils/idbUtils.test.ts

@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) 2024 PixieBrix, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import { withIdbErrorHandling } from "./idbUtils";
+import { deleteDB, type IDBPDatabase } from "idb";
+import { reportToApplicationErrorTelemetry } from "@/telemetry/reportToApplicationErrorTelemetry";
+
+jest.mock("@/telemetry/reportToApplicationErrorTelemetry");
+jest.mock("idb");
+
+describe("withIdbErrorHandling", () => {
+  const mockOpenIDB = jest.fn();
+  const mockDbOperation = jest.fn();
+  const databaseName = "LOG";
+  const operationName = "appendEntry";
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it("should ensure close is called on db after successful operation", async () => {
+    const closeMock = jest.fn();
+    const mockDb = { close: closeMock } as unknown as IDBPDatabase<any>;
+    mockOpenIDB.mockResolvedValue(mockDb);
+    mockDbOperation.mockResolvedValue("success");
+
+    const result = await withIdbErrorHandling(mockOpenIDB, databaseName)(
+      mockDbOperation,
+      {
+        operationName,
+      },
+    );
+
+    expect(result).toBe("success");
+    expect(mockOpenIDB).toHaveBeenCalledTimes(1);
+    expect(mockDbOperation).toHaveBeenCalledExactlyOnceWith(mockDb);
+    expect(closeMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("should succeed on a retry and report retried error", async () => {
+    const closeMock = jest.fn();
+    const mockDb = { close: closeMock } as unknown as IDBPDatabase<any>;
+    mockOpenIDB.mockResolvedValue(mockDb);
+    mockDbOperation
+      .mockRejectedValueOnce(new Error("Temporary error"))
+      .mockResolvedValue("success");
+
+    const result = await withIdbErrorHandling(mockOpenIDB, databaseName)(
+      mockDbOperation,
+      {
+        operationName,
+        shouldRetry: () => true,
+      },
+    );
+
+    expect(result).toBe("success");
+    expect(mockOpenIDB).toHaveBeenCalledTimes(2);
+    expect(mockDbOperation).toHaveBeenCalledTimes(2);
+    expect(reportToApplicationErrorTelemetry).toHaveBeenCalledOnce();
+    expect(reportToApplicationErrorTelemetry).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({
+        idbOperationName: operationName,
+        idbDatabaseName: databaseName,
+      }),
+      expect.any(String),
+    );
+    expect(closeMock).toHaveBeenCalledTimes(2);
+  });
+
+  it("should handle failed retries", async () => {
+    const closeMock = jest.fn();
+    const onFailedAttemptMock = jest.fn();
+    const mockDb = { close: closeMock } as unknown as IDBPDatabase<any>;
+    mockOpenIDB.mockResolvedValue(mockDb);
+    mockDbOperation.mockRejectedValue(new Error("Permanent error"));
+
+    await expect(
+      withIdbErrorHandling(mockOpenIDB, databaseName)(mockDbOperation, {
+        operationName,
+        shouldRetry: () => true,
+        onFailedAttempt: onFailedAttemptMock,
+      }),
+    ).rejects.toThrow("Permanent error");
+
+    expect(mockOpenIDB).toHaveBeenCalledTimes(4); // 1 initial + 3 retries
+    expect(mockDbOperation).toHaveBeenCalledTimes(4);
+    expect(onFailedAttemptMock).toHaveBeenCalledTimes(4);
+    expect(reportToApplicationErrorTelemetry).toHaveBeenCalledTimes(4);
+    expect(deleteDB).not.toHaveBeenCalled();
+  }, 10_000); // Increased timeout due to default backoffs in p-retry
+
+  it("should delete the database if it fails all retries with IDBLargeValueError", async () => {
+    const closeMock = jest.fn();
+    const onFailedAttemptMock = jest.fn();
+    const mockDb = { close: closeMock } as unknown as IDBPDatabase<any>;
+    mockOpenIDB.mockResolvedValue(mockDb);
+    mockDbOperation.mockRejectedValue(
+      new DOMException("Failed to read large IndexedDB value"),
+    );
+
+    await expect(
+      withIdbErrorHandling(mockOpenIDB, databaseName)(mockDbOperation, {
+        operationName,
+        shouldRetry: () => true,
+        onFailedAttempt: onFailedAttemptMock,
+      }),
+    ).rejects.toThrow("Failed to read large IndexedDB value");
+
+    expect(mockOpenIDB).toHaveBeenCalledTimes(4); // 1 initial + 3 retries
+    expect(mockDbOperation).toHaveBeenCalledTimes(4);
+    expect(onFailedAttemptMock).toHaveBeenCalledTimes(4);
+
+    expect(deleteDB).toHaveBeenCalledWith(databaseName, expect.anything());
+  }, 10_000); // Increased timeout due to default backoffs in p-retry
+});

src/utils/idbUtils.ts

@@ -1,3 +1,20 @@
+/*
+ * Copyright (C) 2024 PixieBrix, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 import pDefer from "p-defer";
 import { deleteDB, type IDBPDatabase } from "idb";
 import { getErrorMessage } from "@/errors/errorHelpers";
@@ -175,11 +192,11 @@ export const withIdbErrorHandling =
     dbOperation: (db: IDBPDatabase<DBType>) => Promise<DBOperationResult>,
     {
       operationName,
-      onRetry,
+      onFailedAttempt,
       shouldRetry,
     }: {
       operationName: OperationNames;
-      onRetry?: (error: FailedAttemptError) => void | Promise<void>;
+      onFailedAttempt?: (error: FailedAttemptError) => void | Promise<void>;
       shouldRetry?: (error: FailedAttemptError) => boolean | Promise<boolean>;
     },
   ) => {
@@ -193,34 +210,33 @@ export const withIdbErrorHandling =
         },
         {
           retries: shouldRetry ? MAX_RETRIES : 0,
-          shouldRetry,
+          // 'p-retry' does not handle an undefined shouldRetry correctly, so we need to ensure it is not passed if undefined
+          // See: https://github.com/sindresorhus/p-retry/issues/36
+          ...(shouldRetry ? { shouldRetry } : {}),
           async onFailedAttempt(error) {
             handleIdbError(error, {
               operationName,
               databaseName,
-              message: `${operationName} failed for IDB database: ${databaseName}. Retrying... Attempt ${error.attemptNumber}`,
+              message: `${operationName} failed for IDB database: ${databaseName}. Attempt Number: ${error.attemptNumber}`,
             });
 
             db?.close();
 
-            await onRetry?.(error);
+            await onFailedAttempt?.(error);
           },
         },
       );
     } catch (error) {
-      handleIdbError(error, {
-        operationName,
-        databaseName,
-        message: `${operationName} failed for IDB database ${databaseName}`,
-      });
-
       /**
        * Any retries have failed by this point
        * An error for a single value can break bulk operations on the whole DB
        * We don't know of a way to drop the single bad record even if we know which one it is
        * So we delete the database
        */
       if (isIDBLargeValueError(error)) {
+        console.error(
+          `Deleting ${databaseName} database due to permanent IndexDB large value error.`,
+        );
         await deleteDatabase(databaseName);
       }