fix(framework): several error handling improvements #5001

stefreak · 2023-08-30T21:19:10Z

What this PR does / why we need it:

Made sure that InternalError is only used for errors that "should not happen" and
also to wrap unhandled errors that originate from libraries or the NodeJS standard library.
Stop masking leaf errors (leaf means basically the root cause).
One of the most occuring errors on Amplitude is "graph", but that's a
wrapping error. This will give us better insights in what error types
actually causing command failures.
Fixed a bug where garden would print endless amounts of yaml with the --output option in case of simple errors. You can reproduce the bug by running garden deploy -o yaml in the Garden core repository root directory. The bug has been fixed by filtering error details early to avoid issues later.
Prevent infinite recursion by making sure not to call sanitizeValue from toSanitizedValue and toJSON
Removed dead code in the GraphNodeError.
Consistently handle unexpected errors and format them equally on all
levels.
Format InternalErrors as crashes with an explanation
and a call to action to report the bug on all levels
Remove the GardenError interface in favor of just a GardenError abstract base class. This makes GardenError recognition less error-prone because there is only one way left of recognizing a GardenError (instanceof)
Treat uncaught errors on the CLI level (cli.ts) as crashes, even if
they are GardenError instances other than InternalError, because the GardenCli class should not
throw but return an exit code.
Make sure that in case of crashes we print all relevant information to the terminal. Yes, that makes the log output less concise, but it increases the probability that users report the errors in a way where we can understand what happened and fix the bug.
Add a new CRASH issue template, and add a link to reporting crashes on GitHub in the error message.

Crashes that happen in an action handler look like this:

Before it looked like this:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

core/src/actions/base.ts

core/src/cli/cli.ts

core/src/graph/config-graph.ts

- Made sure that InternalError is only used for errors that "should not happen" and also to wrap unhandled errors that originate from libraries or the NodeJS standard library. - Stop masking leaf errors (leaf means basically the root cause). One of the most occuring errors on Amplitude is "graph", but that's a wrapping error. This will give us better insights in what error types actually causing command failures. - Consistently handle unexpected errors and format them equally on all levels. - Format InternalErrors as crashes with an explanation and a call to action to report the bug on all levels - Treat uncaught errors on the CLI level (cli.ts) as crashes, even if they are GardenError instances other than InternalError, because the GardenCli class should not throw but return an exit code.

core/src/graph/results.ts

core/src/plugins/kubernetes/kubernetes.ts

stefreak · 2023-08-30T21:47:29Z

core/src/logger/logger.ts

@@ -51,7 +51,8 @@ export function parseLogLevel(level: string): LogLevel {
    lvl = LogLevel[level]
  }
  if (!getNumericLogLevels().includes(lvl)) {
-    throw new InternalError({
+    // This should be validated on a different level
+    throw new ParameterError({


Question is if this is actually an InternalError, because validation should happen somewhere else.

But because validation does not happen for root-level options, I changed this to ParameterError so the error message does not confuse users.

Otherwise garden --log-level doesnotexist will be rendered as a crash.

core/src/logger/renderers.ts

core/src/analytics/analytics.ts

core/src/cli/cli.ts

Previously we had an exported interface called GardenError together with an exported abstract base class called GardenBaseError. We had a function called isGardenError that checked for interface conformance, and used it in some parts of the code base, and then we also had parts of the code base that checked for `instanceof GardenBaseError`. This commit eliminates the GardenError interface to eliminate a class of errors, where we fail to recognize the GardenError and treat it as a crash.

Bugfix: Before this commit when running `garden deploy -o json` in the Garden root project, the json was several thousands lines long It was a little bit tricky to solve the error, because I ran into infinite recursion issues between `toSanitizedValue` and `sanitizeValue` Now the -o json output looks good to me. This commit also removes dead code in the GraphNodeError.

core/src/exceptions.ts

Usually spawned child processes are expected to fail for various reasons. Wrap them in ChildProcessError Also improved the error message and error type for Kubernetes apply through kubectl.

edvald · 2023-09-01T20:50:12Z

This is great stuff. Haven't yet done a full review, but really appreciate this effort @stefreak and I like the overall approach ✨

core/src/exceptions.ts

stefreak · 2023-09-04T09:29:43Z

core/src/exceptions.ts

  abstract type: string
  public override message: string
-  public detail?: D
+  public detail: D


This is deceptive: the value is being sanitized in the constructor so the types are only correct if they are base types and non-circular. Maybe we can craft a TypeScript type that allows only base types without circular references?

On the other hand, the rabbit hole was already quite deep and maybe we can get away with a comment here.

@TimBeyer and me tried to solve this problem properly, but fell into a deep rabbit hole. Our TLDR conclusion: The detail object should be eliminated. But this is out of scope for this PR. Let's merge this for now.

stefreak · 2023-09-04T09:49:44Z

core/src/plugins/kubernetes/container/build/common.ts

-          output,
-        },
-      })
+    if (err instanceof PodRunnerError) {


Should we throw if it's not a PodRunnerError?

.github/ISSUE_TEMPLATE/CRASH.md