From 221bb45441be2ef379f6ce83317c23508e87bd4e Mon Sep 17 00:00:00 2001 From: Eduardo Fungairino Date: Fri, 4 Oct 2024 13:55:39 -0400 Subject: [PATCH] fix idb error retry behavior (#9231) --- src/registry/packageRegistry.ts | 7 +- src/utils/idbUtils.test.ts | 130 ++++++++++++++++++++++++++++++++ src/utils/idbUtils.ts | 38 +++++++--- 3 files changed, 158 insertions(+), 17 deletions(-) create mode 100644 src/utils/idbUtils.test.ts diff --git a/src/registry/packageRegistry.ts b/src/registry/packageRegistry.ts index 965de395be..d86362f087 100644 --- a/src/registry/packageRegistry.ts +++ b/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> { 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( diff --git a/src/utils/idbUtils.test.ts b/src/utils/idbUtils.test.ts new file mode 100644 index 0000000000..2ee90af7e5 --- /dev/null +++ b/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 . + */ + +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; + 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; + 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; + 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; + 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 +}); diff --git a/src/utils/idbUtils.ts b/src/utils/idbUtils.ts index f9063464ca..af91d7591f 100644 --- a/src/utils/idbUtils.ts +++ b/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 . + */ + 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) => Promise, { operationName, - onRetry, + onFailedAttempt, shouldRetry, }: { operationName: OperationNames; - onRetry?: (error: FailedAttemptError) => void | Promise; + onFailedAttempt?: (error: FailedAttemptError) => void | Promise; shouldRetry?: (error: FailedAttemptError) => boolean | Promise; }, ) => { @@ -193,27 +210,23 @@ 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 @@ -221,6 +234,9 @@ export const withIdbErrorHandling = * So we delete the database */ if (isIDBLargeValueError(error)) { + console.error( + `Deleting ${databaseName} database due to permanent IndexDB large value error.`, + ); await deleteDatabase(databaseName); }