improvement(core): log aborted nodes on dep error (#5360)

When a dependency fails, we now log a line for each aborted node.

Also fixed the counting logic used in the command error summary. Before
this fix, we used to include aborted nodes in the failed count.

core/src/commands/base.ts

@@ -1025,10 +1025,11 @@ export async function handleProcessResults(
   const graphResults = results.results
   const graphResultsForExport = graphResults.export()
 
-  const failed = pickBy(graphResultsForExport, (r) => r && r.error)
+  const failed = pickBy(graphResultsForExport, (r) => r && !r.aborted && !!r.error)
   const failedCount = size(failed)
+  const abortedCount = size(pickBy(graphResultsForExport, (r) => r && !!r.aborted && !r.error))
 
-  const success = failedCount === 0
+  const success = failedCount === 0 && abortedCount === 0
 
   const buildResults = prepareProcessResults("build", graphResults) as ProcessCommandResult["build"]
   const deployResults = prepareProcessResults("deploy", graphResults) as ProcessCommandResult["deploy"]
@@ -1053,8 +1054,14 @@ export async function handleProcessResults(
       return f && f.error ? [toGardenError(f.error)] : []
     })
 
+    const errMsg = abortedCount
+      ? failedCount
+        ? `${failedCount} requested ${taskType} action(s) failed, ${abortedCount} aborted!`
+        : `${abortedCount} requested ${taskType} action(s) aborted!`
+      : `${failedCount} requested ${taskType} action(s) failed!`
+
     const error = new RuntimeError({
-      message: `${failedCount} ${taskType} action(s) failed!`,
+      message: errMsg,
       wrappedErrors,
     })
     return { result, errors: [error] }

core/src/graph/nodes.ts

@@ -110,7 +110,7 @@ export abstract class TaskNode<T extends Task = Task> {
    * If the node was already completed, this is a no-op (may e.g. happen if the node has been completed
    * but a dependency fails and is aborting dependants).
    */
-  complete({ startedAt, error, result, aborted }: CompleteTaskParams): GraphResult<TaskResultType<T>> {
+  complete({ startedAt, error, result, aborted, abortedKeys }: CompleteTaskParams): GraphResult<TaskResultType<T>> {
     if (this.result) {
       return this.result
     }
@@ -146,14 +146,32 @@ export abstract class TaskNode<T extends Task = Task> {
     }
 
     if (aborted || error) {
-      // Fail every dependant
+      // We abort every dependant, and complete the corresponding request node for the failed node with an error.
+      const keys = abortedKeys || new Set<string>([task.getKey()])
       for (const d of Object.values(this.dependants)) {
+        const depKey = d.task.getKey()
+        let depAborted: boolean
+        let depError: Error | null
+        if (depKey === task.getKey() && d.executionType === "request" && error) {
+          depAborted = false
+          depError = new GraphNodeError({ resultError: error, node: d })
+        } else {
+          depAborted = true
+          depError = null
+          if (!keys.has(depKey)) {
+            d.task.log.info({
+              msg: `Aborting because upstream dependency failed.`,
+              metadata: metadataForLog(d.task, "error", inputVersion),
+            })
+            keys.add(depKey)
+          }
+        }
         d.complete({
           startedAt,
-          aborted: true,
+          aborted: depAborted,
           result: null,
-          // If it was aborted without error, we don't need a GraphNodeError
-          error: error ? new GraphNodeError({ resultError: error, node: d }) : null,
+          error: depError,
+          abortedKeys: keys,
         })
       }
     }
@@ -337,6 +355,10 @@ export interface CompleteTaskParams {
   error: Error | null
   result: ValidResultType | null
   aborted: boolean
+  // Used to track the unique task keys that have been aborted when a dependency fails and we recursively cancel
+  // its dependants (see `TaskNode#complete`). This helps us log each aborted key only once (since we may need to
+  // cancel e.g. both a request node and a process node for the same key).
+  abortedKeys?: Set<string>
 }
 export interface GraphNodeErrorParams {
   resultError: Error

core/test/unit/src/graph/solver.ts

@@ -275,8 +275,7 @@ describe("GraphSolver", () => {
     const result = await processTask(taskB)
 
     expect(result).to.exist
-    expect(result!.error).to.exist
-    expect(result!.error?.message).to.include("Throwing error in process method")
+    expect(result!.aborted).to.be.true
   })
 
   it("cascades an error recursively from dependency and fails the execution", async () => {
@@ -291,8 +290,39 @@ describe("GraphSolver", () => {
     const result = await processTask(taskC)
 
     expect(result).to.exist
-    expect(result!.error).to.exist
-    expect(result!.error?.message).to.include("Throwing error in process method")
+    expect(result!.aborted).to.be.true
+  })
+
+  it("recursively aborts unprocessed task requests when a dependency fails", async () => {
+    const failTask = makeTask({ name: "fail-task" })
+    const failTask2 = makeTask({ name: "fail-task-2" })
+    const taskB1 = makeTask({ name: "task-b1", dependencies: [failTask] })
+    const taskB2 = makeTask({ name: "task-b2", dependencies: [failTask2] })
+    const taskC = makeTask({ name: "task-c", dependencies: [taskB1] })
+
+    failTask.process = async () => {
+      throw new Error(`Throwing error in process method`)
+    }
+    failTask2.process = async () => {
+      throw new Error(`Throwing error in process method`)
+    }
+
+    const result = await garden.processTasks({ tasks: [taskC, taskB2, failTask2], throwOnError: false })
+    const exported = result.results.export()
+    const failTask2Result = exported[failTask2.getKey()]
+    const taskB2Result = exported[taskB2.getKey()]
+    const taskCResult = exported[taskC.getKey()]
+
+    expect(exported[failTask.getKey()]).to.be.undefined
+
+    expect(failTask2Result?.aborted).to.eql(false)
+    expect(failTask2Result?.error).to.exist
+
+    expect(taskB2Result?.aborted).to.eql(true)
+    expect(taskB2Result?.error).to.not.exist
+
+    expect(taskCResult?.aborted).to.eql(true)
+    expect(taskCResult?.error).to.not.exist
   })
 
   it("returns status directly and skips processing if state is ready", async () => {