From ce1995bc8ea4a4c7669310bf44108de47af8dfe0 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Wed, 8 Nov 2023 11:09:36 +0100 Subject: [PATCH] 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 | 13 +++++++--- core/src/graph/nodes.ts | 32 +++++++++++++++++++++---- core/test/unit/src/graph/solver.ts | 38 ++++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 12 deletions(-) diff --git a/core/src/commands/base.ts b/core/src/commands/base.ts index 13770eea55..cb6e8619a9 100644 --- a/core/src/commands/base.ts +++ b/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] } diff --git a/core/src/graph/nodes.ts b/core/src/graph/nodes.ts index 7831d31888..7ad33c9eeb 100644 --- a/core/src/graph/nodes.ts +++ b/core/src/graph/nodes.ts @@ -110,7 +110,7 @@ export abstract class TaskNode { * 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> { + complete({ startedAt, error, result, aborted, abortedKeys }: CompleteTaskParams): GraphResult> { if (this.result) { return this.result } @@ -146,14 +146,32 @@ export abstract class TaskNode { } 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([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 } export interface GraphNodeErrorParams { resultError: Error diff --git a/core/test/unit/src/graph/solver.ts b/core/test/unit/src/graph/solver.ts index 3059bfffd5..61b40b715c 100644 --- a/core/test/unit/src/graph/solver.ts +++ b/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 () => {