Separate saga top level error handling #9458

raymondjacobson · 2024-08-15T18:04:48Z

Description

@schottra and I were chatting... following this guide https://redux-saga.js.org/docs/advanced/RootSaga/#keeping-the-root-alive
this PR:

Separates out saga errors so one saga failing is isolated and restarts itself
In the case of a top level error, toast and report to sentry as well

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

Manually made one of our profile sagas throw and now watched the app recover. This also solves QA-1495 in a different way, although that has been already fixed

changeset-bot · 2024-08-15T18:04:51Z

⚠️ No Changeset found

Latest commit: e205893

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

sliptype

This seems much better to me! Since we're going to be showing the error page a lot less, is it possible that in some cases a full refresh is actually required to resolve the error? Users may not know to refresh the page if they just see a "Something went wrong" toast

sliptype · 2024-08-15T18:10:20Z

packages/web/src/store/sagas.ts

    reloadSagas()
  )
-  yield all(sagas.map(fork))
+  yield all(sagas.map((saga) => spawn(sagaWithErrorHandler, saga)))


Should we do the same on mobile?

yea was planning to if we think this makes sense

I'm wondering if there are certain sagas where this could cause havoc, like the ones that are meant to run once at app startup and then immediately do work to bootstrap and initialize. Worth considering whether those just just be allowed to fail and require a refresh.

Yeah we should make sure that waitForRead and waitForWrite etc all still work on restart... I wonder if there's any other gotchas in our app. For the most part I think we're ok?

This does feel like one of those things that we definitely don't want to rely on and we should still try our best to have great error handling in each saga

raymondjacobson · 2024-08-15T18:15:26Z

This seems much better to me! Since we're going to be showing the error page a lot less, is it possible that in some cases a full refresh is actually required to resolve the error? Users may not know to refresh the page if they just see a "Something went wrong" toast

yeah- it's possible, though i'm not totally sure what. we still will have the error handlers when they are explicitly used to push a user to /error. i think this just stops the "all sagas silently die" sitch

audius-infra · 2024-08-15T18:18:21Z

Preview this change https://demo.audius.co/rj-sagas-errors

schottra

Legendary. Thank you so much for this.

schottra · 2024-08-15T18:23:55Z

packages/web/src/store/sagas.ts

    reloadSagas()
  )
-  yield all(sagas.map(fork))
+  yield all(sagas.map((saga) => spawn(sagaWithErrorHandler, saga)))


I'm wondering if there are certain sagas where this could cause havoc, like the ones that are meant to run once at app startup and then immediately do work to bootstrap and initialize. Worth considering whether those just just be allowed to fail and require a refresh.

schottra · 2024-08-15T18:27:39Z

packages/common/src/utils/sagaHelpers.ts

+      break
+    } catch (e) {
+      yield* put(toast({ content: messages.somethingWrong }))
+      console.error(`Saga ${saga.name} failed, restarting...`, e)


I think this will result in two reports to sentry per error? Since sentry picks up console.error() and pipes it through automatically.
IIRC, most of the sagas that catch errors and reportToSentry don't do separate console statements since the call to reportToSentry() will spit out it's own console.error()
Would recommend making this warn/info.

I think we should probably stop sentry from picking up console.errors tbh. It feels kinda chaotic to do so. wdyt? will change to warn here though

I'm open to it. I like that it helps us find the errors we aren't catching. But it might be the case that Sentry catches exceptions and reports them independently of console.error() statements. If so, I vote we ditch the console reporting. I think it ends up being a lot of third-party code and things that aren't actually actionable.

schottra · 2024-08-15T18:30:00Z

packages/common/src/utils/sagaHelpers.ts

+import { isResponseError } from './error'
+
+const messages = {
+  somethingWrong: 'Something went wrong'


Is there a chance we end up on the error splash screen and throwing a toast with the same message? My guess is no since landing on the splash would be a result of catching the error before the saga dies. But I could be wrong.

I think it's not possible, but maybe can merge this and play around a bunch on staging.audius.co as a group, before doing any mobile work too

schottra · 2024-08-15T18:39:39Z

This seems much better to me! Since we're going to be showing the error page a lot less, is it possible that in some cases a full refresh is actually required to resolve the error? Users may not know to refresh the page if they just see a "Something went wrong" toast

I think the beauty of this solution is that sagas that throw errors will restart independent of the rest. I'm not sure there a whole lot of cases where a single saga throwing an error actually requires that we refresh the app. I think that's more of a side effect of the whole root saga dying whenever we fail to catch an error?

audius-infra · 2024-08-15T18:48:53Z

Preview this change https://demo.audius.co/rj-sagas-errors

rickyrombo · 2024-08-15T19:55:07Z

packages/common/src/utils/sagaHelpers.ts

+        level: ErrorLevel.Error,
+        error: e as Error,
+        additionalInfo: {
+          response: isResponseError(e) ? e.response : undefined


This is already handled in reportToSentry

oh huh. wasn't showing for me looking, but ill look

https://github.com/AudiusProject/audius-protocol/blob/92d4d8100ace1005dd3c7a733006053fac0f9be4/packages/web/src/store/errors/reportToSentry.ts#L50-L60 oh hmm this looks like it's only in web? That might be my mistake...

rickyrombo · 2024-08-15T19:56:26Z

packages/common/src/utils/sagaHelpers.ts

+
+      // Force typing of reportToSentry. We're not able to pull it in
+      // from getContext in ~common as that would require the store.
+      const reportToSentry: (args: ReportToSentryArgs) => void =


Is this like a circular dep thing? Maybe we can still pull the type of reportToSentry out?

perhaps, can look into more

rickyrombo · 2024-08-15T20:02:21Z

packages/web/src/store/sagas.ts

    reloadSagas()
  )
-  yield all(sagas.map(fork))
+  yield all(sagas.map((saga) => spawn(sagaWithErrorHandler, saga)))


Yeah we should make sure that waitForRead and waitForWrite etc all still work on restart... I wonder if there's any other gotchas in our app. For the most part I think we're ok?

This does feel like one of those things that we definitely don't want to rely on and we should still try our best to have great error handling in each saga

gitguardian · 2024-08-19T19:00:26Z

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9412812	Triggered	Generic Password	`54697f4`	core/infra/docker-compose.yml	View secret
11648676	Triggered	Generic High Entropy Secret	`54697f4`	core/infra/dev_config/sandbox-one.env	View secret
11648676	Triggered	Generic High Entropy Secret	`54697f4`	core/infra/dev_config/discovery-one.env	View secret
11648678	Triggered	Generic High Entropy Secret	`54697f4`	core/infra/dev_config/content-three.env	View secret
11648679	Triggered	Generic High Entropy Secret	`54697f4`	core/infra/dev_config/content-two.env	View secret
11648680	Triggered	Generic High Entropy Secret	`54697f4`	core/infra/dev_config/content-one.env	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

audius-infra · 2024-08-19T19:13:36Z

Preview this change https://demo.audius.co/rj-sagas-errors

[1ba94b8] Reland "[PAY-3357] Migrate Notifications to SDK" (#9539) Marcus Pasell [c33b4e3] Revert "[PAY-3357] Migrate Notifications to SDK" (#9538) Marcus Pasell [b8c360e] [PAY-3357] Migrate Notifications to SDK (#9516) Marcus Pasell [eaef9fd] [QA-1498] Fix long press on DMs (#9533) JD Francis [4bbfeff] [PAY-3373] Purchasers endpoint (#9525) Reed [72ff6a3] [C-4909] Improve track metadata list, fix album backlinks (#9531) Dylan Jeffers [e5248b7] [QA-1515] Remove dismiss on leave from popup (#9527) JD Francis [46305bb] [PAY-2918] Migrate track endpoints to SDK - Round 3 (#9522) Randy Schott [b957a52] Fix challenge api tx confirming (#9526) Isaac Solo [6999b53] [QA-1519] Fix buyer metadata caching (#9512) Isaac Solo [7f17c95] Hook up remixers count to web blast UI (#9523) Reed [75ab270] [PAY-3359] Chat blast thread UI on sender side (#9515) Reed [93e3ac9] Mobile comment fixes (#9507) Sebastian Klingler [c81ed5d] [QA-1499] Fix long usernames overflowing manager mode button (#9519) JD Francis [7763deb] Add react-router dependency (#9520) Isaac Solo [6de56bd] Move trpc to common (#9518) Dylan Jeffers [76d4095] Active styles for web chat blasts (#9517) Reed [3464a2a] Add row and column props to flex (#9505) Dylan Jeffers [e6b1da1] [C-4902] Add harmony checkbox (#9510) Dylan Jeffers [0fcf9e4] [PAY-3361] ChatBlastSelectAudienceScreen (#9502) Andrew Mendelsohn [2566eb8] [PAY-3228] Implement composer unfurl in mobile (#9504) Raymond Jacobson [b5bf00a] [QA-1210] Fix popup scrolling issues (#9508) JD Francis [5349315] Update logic for allowing chat blast sending (#9509) Reed [51391d9] [PAY-3362] Move link resolver to common hook (#9503) Raymond Jacobson [45dc83d] Add feature flag refresh (#9506) Dylan Jeffers [3af4e69] [PAY-2918] Migrate track endpoints to SDK, Round 2 (#9494) Randy Schott [78897c4] [PAY-3355] Address link preview / unfurl QA items (#9492) Raymond Jacobson [c2a9387] [PAY-3366] Fix CTA hidden on small screens (#9499) Andrew Mendelsohn [e6dbb7a] Fix CTA empty states reversed web/mobile (#9498) Andrew Mendelsohn [f3d7825] Remove listenUserId from chat api (#9479) Reed [d55dbb1] [PAY-3360] Chat Blast CTA (mobile) (#9491) Andrew Mendelsohn [adb99e3] Fix challenge claim notification amount (#9485) Isaac Solo [d4518ce] [C- 4926] Add Menu/MenuItem and update Select (#9427) Dylan Jeffers [b914ade] [PAY-2918] Migrate tracks endpoints to SDK - round 1 (#9474) Randy Schott [893ed42] Revamp local solana-test-validator (#9482) Marcus Pasell [7ff4ab0] Separate saga top level error handling (#9458) Raymond Jacobson [06a19e6] Remote config for chat blast tier requirement (#9490) Reed [2b804b2] [PAY-3345] Prevent artist reacting to their own blasts (#9488) Reed [1cc1915] [C-4976] Refactor comment hooks (#9489) Sebastian Klingler [b3a4c77] [QA-1511] Fix space & arrow hotkeys not working (#9486) JD Francis [1342d1f] [C-4823] Mobile comment drawer (#9478) Sebastian Klingler [075d757] [QA-1514] Fix search filter typing (#9484) Sebastian Klingler [bfaa536] Enabling blocking users from commenting (#9469) Isaac Solo [c2d5e85] Bump mobile apps to 111 (#9481) Dylan Jeffers [5944ced] [PAY-3358] Fire chat message RPCs on blast write (#9476) Reed [83c97ed] [C-4972] Add comment sort bar (#9477) JD Francis

Separate saga top level error handling

bde3a4f

pull-request-size bot added the size/M label Aug 15, 2024

Remove wait for account

d525dbf

raymondjacobson requested review from rickyrombo, schottra and sliptype August 15, 2024 18:05

sliptype approved these changes Aug 15, 2024

View reviewed changes

schottra approved these changes Aug 15, 2024

View reviewed changes

Switch to warn

ca9616b

rickyrombo approved these changes Aug 15, 2024

View reviewed changes

raymondjacobson added 2 commits August 19, 2024 10:06

Merge remote-tracking branch 'origin/main' into rj-sagas-errors

54697f4

Address feedback / clean up

e205893

raymondjacobson merged commit 7ff4ab0 into main Aug 20, 2024

raymondjacobson deleted the rj-sagas-errors branch August 20, 2024 00:00

Separate saga top level error handling #9458

Separate saga top level error handling #9458

Uh oh!

Conversation

raymondjacobson commented Aug 15, 2024

Description

How Has This Been Tested?

Uh oh!

changeset-bot bot commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

sliptype left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondjacobson commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

audius-infra commented Aug 15, 2024

Uh oh!

schottra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schottra commented Aug 15, 2024

Uh oh!

audius-infra commented Aug 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitguardian bot commented Aug 19, 2024

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Uh oh!

audius-infra commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changeset-bot bot commented Aug 15, 2024 •

edited

Loading

raymondjacobson commented Aug 15, 2024 •

edited

Loading