#6408: jq error handling improvements #6415

twschiller · 2023-09-09T21:13:38Z

What does this PR do?

Closes End of JSON input error running jq bricks #6408
The main change is to try to recover from what appear to be transient jq errors

Work remaining

Adds retry on potentially transient errors: generic error, no stack
Improve error message for JSON.parse error in jq-web
Fix jq execution errors being wrapped as invalid input errors

Discussion

If the generic error, no stack error with jq is transient, we might consider automatically retrying it
I opened tickets in jq-web project to see if other people are hitting:
- Intermittent <generic error, no stack> exception using jq.promised.json fiatjaf/jq-web#31
- Unexpected end of JSON input from jq.promised.json if no result set produced fiatjaf/jq-web#32

Future Work

Fix invalid error message not appearing in error telemetry: Missing Error Message in Mod Telemetry #6405
Fix 2048 run limitation in upstream dependency: Unclosed streams caused FS error after calling jq.json 2048 times. fiatjaf/jq-web#18

Checklist

Add tests
Designate a primary reviewer: @mnholtz

codecov · 2023-09-09T21:24:57Z

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.01% ⚠️

Comparison is base (e237877) 70.08% compared to head (ea9375f) 70.08%.

❗ Current head ea9375f differs from pull request most recent head ff18deb. Consider uploading reports for the commit ff18deb to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6415      +/-   ##
==========================================
- Coverage   70.08%   70.08%   -0.01%     
==========================================
  Files        1167     1167              
  Lines       36403    36431      +28     
  Branches     6852     6860       +8     
==========================================
+ Hits        25512    25531      +19     
- Misses      10891    10900       +9

Files Changed	Coverage Δ
src/utils/promiseUtils.ts	`70.31% <83.33%> (+3.00%)`	⬆️
src/bricks/transformers/jq.ts	`91.30% <86.66%> (-2.03%)`	⬇️

... and 2 files with indirect coverage changes

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

mnholtz · 2023-09-10T18:05:48Z

src/bricks/util.ts

+      }
+
+      if (
+        (retryError && !error.message.includes(retryError)) ||


Is there a better way to compare errors in javascript?

It depends on the error, e.g., depending on the error you might look at the class name. In this case, we do want to compare on the messages.

See comment here: https://github.com/pixiebrix/pixiebrix-extension/pull/6415/files#r1320839837

For this particular scenario, I think want the retry to happen on both on the generic error, and also unexpected end of JSON error. I suspect some of the unexpected end of JSON errors are transient

mnholtz · 2023-09-10T18:07:45Z

src/bricks/util.ts

+  retryError?: string
+): Promise<T> {
+  for (let failedAttempts = 0; failedAttempts <= retries; failedAttempts++) {
+    const delayMs = Math.random() * 100;


Happy to adjust this number as needed. I didn't choose this number for any particular reason, other than the fact that it seemed like a good ballpark number.

src/bricks/util.ts

twschiller · 2023-09-10T18:42:28Z

src/bricks/transformers/jq.ts

+        3,
+        (error) => {
+          if (!isErrorObject(error)) {
+            throw error;


The shouldRetry method should not throw. The check should be isErrorObject(error) && (....)

twschiller · 2023-09-10T18:43:57Z

src/bricks/transformers/jq.ts

+          }
+
+          return (
+            error.message.includes(JSON_ERROR) ||


Leave a comment that we're not retrying on FS_STREAM_ERROR because it's not recoverable. The error kicks in when the jq brick has been run too many times, so retrying won't succeed

…ix-extension into feature/6408-jq-errors

mnholtz · 2023-09-10T23:07:17Z

src/bricks/transformers/jq.ts

+ * Return true for jq errors that might be transient.
+ *
+ * We're excluding FS_STREAM_ERROR because it most likely indicated emscripten has hit the stream limit, so additional
+ * retries would not success: https://github.com/fiatjaf/jq-web/issues/18


Suggested change

* retries would not success: https://github.com/fiatjaf/jq-web/issues/18

* retries would not succeed: https://github.com/fiatjaf/jq-web/issues/18

mnholtz · 2023-09-10T23:07:28Z

src/bricks/transformers/jq.ts

+/**
+ * Return true for jq errors that might be transient.
+ *
+ * We're excluding FS_STREAM_ERROR because it most likely indicated emscripten has hit the stream limit, so additional


Suggested change

* We're excluding FS_STREAM_ERROR because it most likely indicated emscripten has hit the stream limit, so additional

* We're excluding FS_STREAM_ERROR because it most likely indicates emscripten has hit the stream limit, so additional

mnholtz · 2023-09-10T23:09:56Z

src/bricks/transformers/jq.ts

+ *
+ * JSON_ERROR can be deterministic, but we've also seen some error telemetry indicating it might be transient.
+ */
+function isTransientError(error: unknown): boolean {


This refactor is a huge improvement 👍

github-actions · 2023-09-10T23:15:18Z

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.

[WIP] jq error handling

54e89dd

twschiller assigned twschiller and mnholtz Sep 9, 2023

vercel bot deployed to Preview September 9, 2023 21:14 View deployment

#6408: improve error message

147df2d

vercel bot deployed to Preview September 9, 2023 21:31 View deployment

#6408: add more know runtime errors

616919f

vercel bot deployed to Preview September 9, 2023 21:38 View deployment

twschiller added this to the 1.7.40 milestone Sep 9, 2023

#6408: test more concurrent runs

2cfc171

vercel bot deployed to Preview September 9, 2023 22:09 View deployment

twschiller and others added 2 commits September 10, 2023 11:31

#6408: WIP: error handling/test cases

f5b58fb

add fs stream error handling

e23b4eb

vercel bot deployed to Preview September 10, 2023 17:05 View deployment

add retryWithJitter func

85b37e6

vercel bot deployed to Preview September 10, 2023 17:28 View deployment

mnholtz added 2 commits September 10, 2023 10:34

add retryWithJitter to applyJq

921f480

add tests for retryWithJitter

1e32a57

vercel bot deployed to Preview September 10, 2023 17:58 View deployment

use includes instead of strict equal

c4aecb2

vercel bot deployed to Preview September 10, 2023 18:05 View deployment

mnholtz reviewed Sep 10, 2023

View reviewed changes

twschiller commented Sep 10, 2023

View reviewed changes

src/bricks/util.ts Outdated Show resolved Hide resolved

change retryWithJitter signature to shouldRetry

59fa99a

vercel bot deployed to Preview September 10, 2023 18:42 View deployment

twschiller commented Sep 10, 2023

View reviewed changes

remove throw from shouldRetry func

68e10d8

twschiller commented Sep 10, 2023

View reviewed changes

mnholtz added 2 commits September 10, 2023 11:46

add comment about exclusion of fs stream error

91bf621

fix arrow-body style

8ea320e

vercel bot deployed to Preview September 10, 2023 18:51 View deployment

twschiller added 2 commits September 10, 2023 15:41

Merge branch 'feature/6408-jq-errors' of github.com:pixiebrix/pixiebr…

e0f226e

…ix-extension into feature/6408-jq-errors

#6408: refactor retry with jitter

81bf092

twschiller changed the title ~~#6408: [WIP] jq error handling fixes~~ #6408: jq error handling improvements Sep 10, 2023

twschiller requested a review from grahamlangford September 10, 2023 20:42

vercel bot deployed to Preview September 10, 2023 20:43 View deployment

#6408: link to github issue

ea9375f

vercel bot deployed to Preview September 10, 2023 20:48 View deployment

twschiller mentioned this pull request Sep 10, 2023

#6430: implement @pixies/util/use-async-state using JS #6431

Merged

4 tasks

twschiller requested review from BLoe and turbochef September 10, 2023 22:01

mnholtz reviewed Sep 10, 2023

View reviewed changes

mnholtz approved these changes Sep 10, 2023

View reviewed changes

Fix comments

ff18deb

twschiller enabled auto-merge (squash) September 10, 2023 23:23

vercel bot deployed to Preview September 10, 2023 23:27 View deployment

twschiller merged commit 74c52f2 into main Sep 10, 2023

twschiller deleted the feature/6408-jq-errors branch September 10, 2023 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6408: jq error handling improvements #6415

#6408: jq error handling improvements #6415

twschiller commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

mnholtz Sep 10, 2023

twschiller Sep 10, 2023

twschiller Sep 10, 2023

mnholtz Sep 10, 2023

mnholtz Sep 10, 2023

twschiller Sep 10, 2023 •

edited

Loading

twschiller Sep 10, 2023

mnholtz Sep 10, 2023

mnholtz Sep 10, 2023

mnholtz Sep 10, 2023

github-actions bot commented Sep 10, 2023

	* retries would not success: https://github.com/fiatjaf/jq-web/issues/18
	* retries would not succeed: https://github.com/fiatjaf/jq-web/issues/18

	* We're excluding FS_STREAM_ERROR because it most likely indicated emscripten has hit the stream limit, so additional
	* We're excluding FS_STREAM_ERROR because it most likely indicates emscripten has hit the stream limit, so additional

#6408: jq error handling improvements #6415

#6408: jq error handling improvements #6415

Conversation

twschiller commented Sep 9, 2023 • edited Loading

What does this PR do?

Work remaining

Discussion

Future Work

Checklist

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

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

twschiller Sep 10, 2023 • 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

github-actions bot commented Sep 10, 2023

twschiller commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

twschiller Sep 10, 2023 •

edited

Loading