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

grahamlangford · 2024-10-03T18:02:07Z

What does this PR do?

Part of Failed to read large IndexedDB value error #9211

Remaining work

identify idb calls we want to enable retries for
Add retries (and onRetry where it makes sense) for each

For more information on our expectations for the PR process, see the
code review principles doc

codecov · 2024-10-03T18:09:30Z

Codecov Report

Attention: Patch coverage is 69.09091% with 51 lines in your changes missing coverage. Please review.

Project coverage is 74.82%. Comparing base (8318d74) to head (45968ea).
Report is 335 commits behind head on main.

Files with missing lines	Patch %	Lines
src/telemetry/trace.ts	33.33%	24 Missing ⚠️
src/telemetry/logging.ts	75.00%	17 Missing ⚠️
src/registry/packageRegistry.ts	80.00%	5 Missing ⚠️
src/background/telemetry.ts	71.42%	4 Missing ⚠️
src/utils/idbUtils.ts	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9229      +/-   ##
==========================================
+ Coverage   74.24%   74.82%   +0.58%     
==========================================
  Files        1332     1367      +35     
  Lines       40817    42232    +1415     
  Branches     7634     7897     +263     
==========================================
+ Hits        30306    31602    +1296     
- Misses      10511    10630     +119

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2024-10-03T18:17:45Z

src/utils/idbUtils.ts

+        {
+          retries: retry ? MAX_RETRIES : 0,
+          shouldRetry: (error) =>
+            isIDBConnectionError(error) || isIDBLargeValueError(error),


It'd be good to document here for future context why we think "Large value error" is recoverable. Are you suspecting it might be a race condition with when the blob is written to disk?

We can link this chromium discussion where devs have reported retries working for them in the case where this error is thrown:
https://issues.chromium.org/issues/342779913#comment8

This needs to be passed in. It's not necessarily recoverable without some kind of onRetry, though we're going to attempt in a couple of areas as a test. We also want to be flexible enough to be able to change the reasons to retry for other errors in the future

We can link this chromium discussion where devs have reported retries working for them in the case where this error is thrown:

Agree that's extremely helpful for explaining the "why" behind the code

github-actions · 2024-10-03T18:20:00Z

Playwright test results

130 passed
4 flaky
4 skipped

Details

Open report ↗︎
138 tests across 45 suites
13 minutes, 23 seconds
45968ea
For more information on how to debug and view this report, see our readme

Flaky tests

chrome › tests/modLifecycle.spec.ts › create, run, package, and update mod
chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
chrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options

twschiller · 2024-10-03T18:24:18Z

src/utils/idbUtils.ts

@@ -98,6 +100,11 @@ export function isIDBQuotaError(error: unknown): boolean {
  return QUOTA_ERRORS.some((quotaError) => message.includes(quotaError));
 }

+export function isIDBLargeValueError(error: unknown): boolean {


It would be good to link to the Chrome Roadmap context on what this error means: https://chromestatus.com/feature/5140210640486400

I.e., that it could be a large value, or a missing blob. Also, this check might break in Chrome 130 if the error messages change?

Chrome commits corresponding to the roadmap might have the text of the new messages

As per comment here: https://github.com/pixiebrix/pixiebrix-extension/pull/9229/files#r1786666504. Which of those 2 underlying causes do we think might be recoverable?

I.e., that it could be a large value, or a missing blob. Also, this check might break in Chrome 130 if the error messages change?

I tested on Chrome 131 and it works, so should be fine on Chrome 130

The only change to the error is it can be a NotFoundError now. But it still has the same error.message

Thanks for investigating. NIT: I think think it'd be valuable to link to that Chrome Roadmap entry that explains what that error means in Chrome

The log in the latest version already has a relevant link

…retry-on-failed-to-read-large-indexeddb-value

src/utils/idbUtils.ts

fungairino · 2024-10-03T21:10:35Z

src/registry/packageRegistry.ts

+    },
+    {
+      operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].GET_BY_KINDS,
+      // The large value error is fixable by re-syncing the packages


Nit: this comment is not exactly true since large value error right now could be one of several issues, some of which are retryable, and some are not.

github-actions · 2024-10-03T21:12:35Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

twschiller · 2024-10-03T23:40:57Z

src/registry/packageRegistry.ts

+      shouldRetry: (error) =>
+        isIDBConnectionError(error) || isIDBLargeValueError(error),
+      async onRetry(error) {
+        if (isIDBLargeValueError(error)) {


I think I'm missing the explanation of why getByKinds is the only operation that re-runs syncPackages in this PR. (E.g., why not the "find" operation). Is it because it's a bulk operation that needs to read multiple bricks (i.e., we think the find method is less risky because it would only fail on specific bricks?)

Is that in Slack/Notion somewhere?

We were able to trigger a getByKinds error locally and the potential impact is higher. We could implement it in a lot more places, but we decided to keep the change limited until we had the additional logging information from this release.

If you think it's valuable to implement it more widely, I can definitely see to that

If you think it's valuable to implement it more widely, I can definitely see to that

Up to you/Eduardo, I don't have a strong opinion. Just wanted to ensure we had the context captured

twschiller · 2024-10-03T23:47:44Z

src/utils/idbUtils.ts

+        message: `${operationName} failed for IDB database ${databaseName}`,
+      });
+
+      if (isIDBLargeValueError(error)) {


Adding a "why" here would be helpful. I.e., that 1/ an error for a single value can break bulk operations on the whole DB, and 2/ we don't know of a way to drop the single bad record even if we know which one it is

Yes, adding a comment here would be useful to explain, along with a log statement

twschiller · 2024-10-03T23:50:09Z

src/utils/idbUtils.ts


 // IDB Connection Error message strings
 const CONNECTION_ERRORS = ["Error Opening IndexedDB"] as const;
+const MAX_RETRIES = 3;


Any context on how we picked 3 here?

No concept. Just seemed a sane default. We typically use 3 retries for sandbox messaging for example

twschiller · 2024-10-03T23:52:05Z

src/registry/packageRegistry.ts

+      operationName: IDB_OPERATION[DATABASE_NAME.PACKAGE_REGISTRY].GET_BY_KINDS,
+      // The large value error is fixable by re-syncing the packages
+      shouldRetry: (error) =>
+        isIDBConnectionError(error) || isIDBLargeValueError(error),


Given that the isIDBConnectionError(error) || isIDBLargeValueError(error) check is repeated, you might consider introducing a helper isMaybeTemporaryIDBError or a similar name to more directly reflect the meaning

* 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 <eduardo@pixiebrix.com>

grahamlangford added 2 commits October 3, 2024 11:24

make operationName part of a config object

e2ede6e

implement first retry

d914ec6

grahamlangford marked this pull request as draft October 3, 2024 18:02

grahamlangford requested review from fungairino and twschiller October 3, 2024 18:02

grahamlangford self-assigned this Oct 3, 2024

grahamlangford added this to the 2.1.3 milestone Oct 3, 2024

twschiller reviewed Oct 3, 2024

View reviewed changes

grahamlangford added 2 commits October 3, 2024 14:16

Merge branch 'main' of github.com:pixiebrix/pixiebrix-extension into …

d7d3992

…retry-on-failed-to-read-large-indexeddb-value

fix errors from type change

40ff617

vercel bot deployed to Preview October 3, 2024 19:19 View deployment

update error messaging; add handling

513ecf9

vercel bot deployed to Preview October 3, 2024 19:45 View deployment

move shouldRetry to the callsites

6cd845a

vercel bot deployed to Preview October 3, 2024 20:32 View deployment

document isIDBLargeValueError

d0e3449

vercel bot deployed to Preview October 3, 2024 20:41 View deployment

twschiller added the bug Something isn't working label Oct 3, 2024

grahamlangford and others added 2 commits October 3, 2024 15:42

improve comment

589b6bc

Merge branch 'main' into retry-on-failed-to-read-large-indexeddb-value

b272b1f

grahamlangford marked this pull request as ready for review October 3, 2024 20:43

vercel bot deployed to Preview October 3, 2024 20:45 View deployment

handle retry in test

0b59331

vercel bot deployed to Preview October 3, 2024 21:01 View deployment

fungairino reviewed Oct 3, 2024

View reviewed changes

src/utils/idbUtils.ts Show resolved Hide resolved

remove retry flag and depend on shouldRetry instead

cd091d6

fungairino reviewed Oct 3, 2024

View reviewed changes

vercel bot deployed to Preview October 3, 2024 21:10 View deployment

fungairino approved these changes Oct 3, 2024

View reviewed changes

clear db on large value error

ec9b7fd

vercel bot deployed to Preview October 3, 2024 21:28 View deployment

twschiller reviewed Oct 3, 2024

View reviewed changes

adds requested comments

45968ea

vercel bot deployed to Preview October 4, 2024 14:04 View deployment

grahamlangford enabled auto-merge (squash) October 4, 2024 14:14

fungairino approved these changes Oct 4, 2024

View reviewed changes

grahamlangford merged commit 3ba3f57 into main Oct 4, 2024
26 checks passed

grahamlangford deleted the retry-on-failed-to-read-large-indexeddb-value branch October 4, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

grahamlangford commented Oct 3, 2024 •

edited

Loading

codecov bot commented Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

fungairino Oct 3, 2024

grahamlangford Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024

github-actions bot commented Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024

grahamlangford Oct 3, 2024

grahamlangford Oct 3, 2024

twschiller Oct 3, 2024

grahamlangford Oct 4, 2024

fungairino Oct 3, 2024

github-actions bot commented Oct 3, 2024

twschiller Oct 3, 2024 •

edited

Loading

grahamlangford Oct 4, 2024

twschiller Oct 4, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

fungairino Oct 4, 2024

twschiller Oct 3, 2024

grahamlangford Oct 4, 2024

twschiller Oct 3, 2024

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

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

Conversation

grahamlangford commented Oct 3, 2024 • edited Loading

What does this PR do?

Remaining work

codecov bot commented Oct 3, 2024 • edited Loading

Codecov Report

twschiller Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024 • edited Loading

Playwright test results

Details

twschiller Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024

twschiller Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford commented Oct 3, 2024 •

edited

Loading

codecov bot commented Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

grahamlangford Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading

twschiller Oct 4, 2024 •

edited

Loading

twschiller Oct 3, 2024 •

edited

Loading