Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add idb error handling for Internal error opening backing store #9252

Merged
merged 4 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/utils/idbUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,28 @@ describe("withIdbErrorHandling", () => {

expect(deleteDB).toHaveBeenCalledWith(databaseName, expect.anything());
}, 10_000); // Increased timeout due to default backoffs in p-retry

it("should delete the database if it fails all retries with Internal error opening backing store", async () => {
const onFailedAttemptMock = jest.fn();
mockOpenIDB.mockRejectedValue(
new DOMException(
"Internal error opening backing store for indexedDB.open.",
),
);

await expect(
withIdbErrorHandling(mockOpenIDB, databaseName)(mockDbOperation, {
operationName,
shouldRetry: () => true,
onFailedAttempt: onFailedAttemptMock,
}),
).rejects.toThrow(
"Internal error opening backing store for indexedDB.open.",
);

expect(mockOpenIDB).toHaveBeenCalledTimes(4); // 1 initial + 3 retries
expect(onFailedAttemptMock).toHaveBeenCalledTimes(4);

expect(deleteDB).toHaveBeenCalledWith(databaseName, expect.anything());
}, 10_000); // Increased timeout due to default backoffs in p-retry
});
34 changes: 26 additions & 8 deletions src/utils/idbUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ export function isIDBQuotaError(error: unknown): boolean {
* Before Chrome 130, there is no way to determine if the file is missing or if some other error occurred.
* In Chrome 130 and later, the error message remains the same, the type of error is different.
* NotFoundError if the file is missing, DataError for any other error.
*
* This error for a single value can break bulk operations on the whole DB,
* and we don't know of a way to drop the single bad record even if we know which one it is.
*
* @see https://chromestatus.com/feature/5140210640486400
* @param error the error object
*/
Expand All @@ -138,6 +142,21 @@ export function isIDBLargeValueError(error: unknown): boolean {
return message.includes("Failed to read large IndexedDB value");
}

/**
* Corrupt chrome profile. Not sure how this happens, but it seems to happen to users
* on citrix machines. This error seems to be unrecoverable and the only solution is to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just citrix. We've seen this on other devices as well. It's even happened to me on my Mac, though not in a long time

* delete the database.
* https://jasonsavard.com/forum/discussion/4233/unknownerror-internal-error-opening-backing-store-for-indexeddb-open
*
* @param error the error object
*/
function isIDBErrorOpeningBackingStore(error: unknown): boolean {
const message = getErrorMessage(error);
return message.includes(
"Internal error opening backing store for indexedDB.open",
);
}

/**
* The large value error could be a NotFoundError or a DataError.
* NotFoundError may be fixable on a case-by-case basis.
Expand All @@ -150,6 +169,10 @@ export function isMaybeTemporaryIDBError(error: unknown): boolean {
return isIDBConnectionError(error) || isIDBLargeValueError(error);
}

export function isPermanentIDBError(error: unknown): boolean {
Copy link
Contributor

@twschiller twschiller Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance, this check seems inconsistent with isMaybeTemporaryIDBError given that isIDBConnectionError checks for a very similar message: https://github.com/pixiebrix/pixiebrix-extension/pull/9252/files#diff-ac059c8d1b896fde28f537e24bb46cd62088057e599d99dde3d9152d17fa3f8eR28

Do we know what's going on with the different capitalization/etc. in those messages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have enough context on the "Error Opening IndexedDB". Doing a quick search I can't find anything regarding this specific error and looking at the git history, it points to an old rollbar log link.

Maybe you could tell me more about this connection error message?
75bb247#diff-ac059c8d1b896fde28f537e24bb46cd62088057e599d99dde3d9152d17fa3f8eR5-R7

return isIDBLargeValueError(error) || isIDBErrorOpeningBackingStore(error);
}

// Rather than use reportError from @/telemetry/reportError, IDB errors are directly reported
// to application error telemetry to avoid attempting to record the error in the idb log database.
function handleIdbError(
Expand Down Expand Up @@ -227,15 +250,10 @@ export const withIdbErrorHandling =
},
);
} catch (error) {
/**
* 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)) {
// If all retries have failed and the latest error is a permanent error, we delete the database.
if (isPermanentIDBError(error)) {
console.error(
`Deleting ${databaseName} database due to permanent IndexDB large value error.`,
`Deleting ${databaseName} database due to permanent IndexDB error.`,
);
await deleteDatabase(databaseName);
}
Expand Down
Loading