Fix zinnia sentry grouping

[juliangruber]

The problem was if (!signal.aborted): We only performed the special Sentry error transformation when a child process exited on its own. When the AbortController was triggered (say because there was a module update), then signal.aborted was always true, and therefore the special Sentry code didn't run.

This condition was a mistake, and is now removed in this PR. We want to run the code block on every child process exit.

[bajtos]

The problem was if (!signal.aborted): We only performed the special Sentry error transformation when a child process exited on its own. When the AbortController was triggered (say because there was a module update), then signal.aborted was always true, and therefore the special Sentry code didn't run.

This condition was a mistake, and is now removed in this PR. We want to run the code block on every child process exit.

This doesn't sound right to me. IIUC, with your change in place, we will report module exits to Sentry, even in situations when the exit was expected because it was triggered by us.

IMO, we should implement the following behaviour:

• When the child process exits on its own, we want to report the error to Sentry.
• When the child process exits because we are aborting, no error should be reported to Sentry.

I think the following change would achieve that:

-    if (!signal.aborted) {
+    if (signal.aborted) {
+      Object.assign(err, { reportedToSentry: true })
+    } else {

WDYT?

In this case, your original name, shouldReportToSentry, would work better than the name reportedToSentry I proposed. 🙈

[juliangruber]

The problem was if (!signal.aborted): We only performed the special Sentry error transformation when a child process exited on its own. When the AbortController was triggered (say because there was a module update), then signal.aborted was always true, and therefore the special Sentry code didn't run.
This condition was a mistake, and is now removed in this PR. We want to run the code block on every child process exit.

This doesn't sound right to me. IIUC, with your change in place, we will report module exits to Sentry, even in situations when the exit was expected because it was triggered by us.

IMO, we should implement the following behaviour:

• When the child process exits on its own, we want to report the error to Sentry.
• When the child process exits because we are aborting, no error should be reported to Sentry.

I think the following change would achieve that:

-    if (!signal.aborted) {
+    if (signal.aborted) {
+      Object.assign(err, { reportedToSentry: true })
+    } else {

WDYT?

In this case, your original name, shouldReportToSentry, would work better than the name reportedToSentry I proposed. 🙈

Great catch! Implemented as suggested, and made the detection for abort errors more robust (we want to know where this caught error is from an abort, not whether an abort has happened in general). Also, don't worry about the shouldReportToSentry name, this situation was impossible to predict.