From ba4ce0888eb3211f5bc38358d0b44e9f1766dbbd Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 13 Jun 2026 19:07:09 -0400 Subject: [PATCH] fix: only treat a crash with a cascade frame as non-transient The crash classifier introduced in #146 flagged any act exit carrying a Go crash signature as a non-transient hard failure. On CI run 27480827218 this hard-failed Hotfix_Refusal_Guards on a bare goroutine dump from act's own monitorJobCancellation teardown watchdog (under cancel-in-progress): no panic/fatal/signal trigger and zero cascade frames, just nektos/act frames. That scenario passes locally with no retries, so it was an act-infra teardown flake that retry should absorb, not a real bug. Refine detectCrashSignature to classify a crash as non-transient only when BOTH a real runtime trigger (panic:/fatal error: at line start, or a signal SIG report) AND a real cascade stack frame are present. Drop the standalone goroutine-dump-header trigger. A non-zero act exit lacking either stays transient and retryable, preserving #146's benign-transient and real-job-failure paths. A genuine cascade-CLI panic still carries both a trigger and cmd/cascade or internal/... frames, so it is still caught and fast-failed. Signed-off-by: Joshua Temple --- e2e/harness/crash_test.go | 180 +++++++++++++++++++++++++++++++++----- e2e/harness/parser.go | 48 +++++++--- 2 files changed, 193 insertions(+), 35 deletions(-) diff --git a/e2e/harness/crash_test.go b/e2e/harness/crash_test.go index dc90201a..4f5886eb 100644 --- a/e2e/harness/crash_test.go +++ b/e2e/harness/crash_test.go @@ -6,10 +6,11 @@ import ( ) // goroutineDump is a representative Go runtime panic + goroutine dump as it -// appears interleaved in act's stdout/stderr when the cascade CLI (or act -// itself) crashes mid-run. The dump corrupts act's --json stream, so no -// "Job failed" event is parseable and the run would otherwise be misclassified -// as a transient flake and retried away. +// appears interleaved in act's stdout/stderr when the cascade CLI crashes +// mid-run. The dump corrupts act's --json stream, so no "Job failed" event is +// parseable. It carries a real cascade stack frame +// (internal/orchestrate.(*Planner).Plan), so it is a definitive cascade crash +// and must not be retried away as a transient flake. const goroutineDump = `time="2026-06-13T00:00:00Z" level=info msg="⭐ Run Main cascade orchestrate" panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x10a3f20] @@ -21,10 +22,59 @@ main.main() /cascade/main.go:33 +0x9c ` -// TestDetectCrashSignature_GoPanicAndGoroutineDump verifies a real Go-runtime -// crash signature in raw act output is detected so it can be treated as a -// definitive failure rather than a transient flake. -func TestDetectCrashSignature_GoPanicAndGoroutineDump(t *testing.T) { +// actOnlyPanicDump is a panic whose goroutine dump contains ONLY nektos/act and +// stdlib frames, with no cascade frame. act can itself panic on an +// infrastructure path (a docker transport hiccup, a teardown race); such a +// crash is not a cascade defect and a retry should absorb it, so it must NOT be +// classified as a (non-transient) cascade crash. +const actOnlyPanicDump = `time="2026-06-13T00:00:00Z" level=info msg="⭐ Run Set up job" +panic: runtime error: invalid memory address or nil pointer dereference +[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9f1d40] + +goroutine 84 [running]: +github.com/nektos/act/pkg/runner.(*runnerImpl).NewPlanExecutor.func1(0x0) + /go/pkg/mod/github.com/nektos/act/pkg/runner/runner.go:201 +0x120 +net/http.(*conn).serve(0xc0001a4000, {0x1, 0x2}) + /usr/local/go/src/net/http/server.go:2092 +0x4f4 +internal/poll.(*FD).Read(0xc0000b8000, {0xc0001, 0x10, 0x10}) + /usr/local/go/src/internal/poll/fd_unix.go:164 +0x29a +` + +// bareActGoroutineDump is the actual shape act prints on a benign teardown: +// monitorJobCancellation under cancel-in-progress dumps every goroutine with a +// "goroutine N [wselect]:" header and NO panic / fatal error / signal trigger. +// On CI run 27480827218 this flaked Hotfix_Refusal_Guards step 2 even though the +// scenario passes 4/4 locally with zero retries. It is pure act-infra noise and +// must stay transient. +const bareActGoroutineDump = `time="2026-06-13T00:00:00Z" level=info msg="⭐ Run Set up job" +goroutine 470 [wselect]: +github.com/nektos/act/pkg/runner.(*runnerImpl).monitorJobCancellation.func1() + /go/pkg/mod/github.com/nektos/act/pkg/runner/runner.go:312 +0x88 +created by github.com/nektos/act/pkg/runner.(*runnerImpl).monitorJobCancellation + /go/pkg/mod/github.com/nektos/act/pkg/runner/runner.go:305 +0x14c + +goroutine 12 [chan receive]: +github.com/nektos/act/pkg/container.(*containerReference).Exec.func2() + /go/pkg/mod/github.com/nektos/act/pkg/container/docker_run.go:401 +0x40 +` + +// signalWithCascadeFrame is a fatal signal whose dump carries a cascade frame +// but no leading "panic:" line. The signal report is itself a crash trigger, so +// paired with a cascade frame it is a definitive cascade crash. +const signalWithCascadeFrame = `time="2026-06-13T00:00:00Z" level=info msg="⭐ Run Main cascade promote" +[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x10b4c10] + +goroutine 1 [running]: +github.com/stablekernel/cascade/cmd/cascade.run({0x0, 0x0}) + /cascade/cmd/cascade/main.go:88 +0x210 +` + +// TestDetectCrashSignature verifies a crash is classified (non-transient) ONLY +// when a real runtime trigger (panic/fatal error/signal) is paired with a real +// cascade stack frame. An act/docker/stdlib-only dump, or a bare goroutine dump +// with no trigger, stays uncaught here so the scenario runner treats it as a +// transient flake and retries it. +func TestDetectCrashSignature(t *testing.T) { t.Parallel() tests := []struct { @@ -34,28 +84,47 @@ func TestDetectCrashSignature_GoPanicAndGoroutineDump(t *testing.T) { wantReasony string // substring expected in the reason when wantCrash }{ { - name: "panic plus goroutine dump", + name: "panic plus goroutine dump with cascade frame is a crash", logs: goroutineDump, wantCrash: true, wantReasony: "panic", }, { - name: "fatal error at line start", - logs: "some log\nfatal error: concurrent map writes\n\ngoroutine 7 [running]:\n", + name: "fatal error at line start with cascade frame is a crash", + logs: "some log\nfatal error: concurrent map writes\n\ngoroutine 7 [running]:\ngithub.com/stablekernel/cascade/internal/state.(*Store).Write(0x0)\n\t/cascade/internal/state/store.go:51 +0x40\n", wantCrash: true, wantReasony: "fatal error", }, { - name: "goroutine dump with runtime frame", - logs: "goroutine 42 [running]:\nruntime.gopanic(0x1, 0x2)\n\t/usr/local/go/src/runtime/panic.go:884\n", + name: "SIGSEGV signal line with cascade frame is a crash", + logs: signalWithCascadeFrame, wantCrash: true, - wantReasony: "goroutine", + wantReasony: "SIGSEGV", }, { - name: "SIGSEGV signal line", - logs: "unexpected\n[signal SIGSEGV: segmentation violation code=0x1 addr=0x0]\n", - wantCrash: true, - wantReasony: "SIGSEGV", + name: "panic with only act and stdlib frames is not a cascade crash", + logs: actOnlyPanicDump, + wantCrash: false, + }, + { + name: "bare act goroutine dump with no trigger is not a crash", + logs: bareActGoroutineDump, + wantCrash: false, + }, + { + name: "fatal error with no cascade frame is not a cascade crash", + logs: "fatal error: concurrent map writes\n\ngoroutine 7 [running]:\nruntime.gopanic(0x1, 0x2)\n\t/usr/local/go/src/runtime/panic.go:884\n", + wantCrash: false, + }, + { + name: "cascade frame present but no trigger is not a crash", + logs: "goroutine 9 [running]:\ngithub.com/stablekernel/cascade/internal/orchestrate.(*Planner).Plan(0x0)\n\t/cascade/internal/orchestrate/planner.go:142\n", + wantCrash: false, + }, + { + name: "git-url prose mentioning the cascade module path is not a crash", + logs: `{"jobID":"deploy","msg":"cloning github.com/stablekernel/cascade/internal at v1.2.3","level":"info"}`, + wantCrash: false, }, { name: "benign log mentioning the word panic in prose is not a crash", @@ -104,17 +173,44 @@ func TestParseActOutput_SetsCrashFields(t *testing.T) { t.Fatalf("ParseActOutput returned error: %v", err) } if !result.Crashed { - t.Fatalf("expected Crashed=true for a panic+goroutine dump") + t.Fatalf("expected Crashed=true for a panic+goroutine dump with a cascade frame") } if result.CrashReason == "" { t.Fatalf("expected a non-empty CrashReason") } } +// TestParseActOutput_ActOnlyDumpIsNotCrash verifies an act/docker-only panic or +// a bare act goroutine dump does NOT set the crash fields, so the run falls +// through to the transient path and is retried as an infrastructure flake. +func TestParseActOutput_ActOnlyDumpIsNotCrash(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + logs string + }{ + {name: "act-only panic", logs: actOnlyPanicDump}, + {name: "bare act goroutine dump", logs: bareActGoroutineDump}, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result, err := ParseActOutput(tt.logs) + if err != nil { + t.Fatalf("ParseActOutput returned error: %v", err) + } + if result.Crashed { + t.Fatalf("expected Crashed=false for an act-only dump (reason=%q)", result.CrashReason) + } + }) + } +} + // TestNormalizeWorkflowResult_CrashIsNotTransient is the core safety guarantee: -// a non-zero act exit carrying a Go-runtime crash signature must be classified -// as a REAL (non-transient) failure so the scenario runner does not retry it as -// a flake and the stack trace surfaces. +// a non-zero act exit carrying a cascade crash signature must be classified as a +// REAL (non-transient) failure so the scenario runner does not retry it as a +// flake and the stack trace surfaces. func TestNormalizeWorkflowResult_CrashIsNotTransient(t *testing.T) { t.Parallel() @@ -144,3 +240,43 @@ func TestNormalizeWorkflowResult_CrashIsNotTransient(t *testing.T) { t.Fatalf("a crash failure must not be classified transient: %v", failErr) } } + +// TestNormalizeWorkflowResult_ActOnlyDumpIsTransient is the sibling guarantee: +// a non-zero act exit whose logs carry only an act/docker dump (no cascade +// frame) is NOT flagged Crashed by the parser, so it falls through to the +// transient ExecError path and the scenario runner retries it as an infra +// flake. +func TestNormalizeWorkflowResult_ActOnlyDumpIsTransient(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + logs string + }{ + {name: "act-only panic", logs: actOnlyPanicDump}, + {name: "bare act goroutine dump", logs: bareActGoroutineDump}, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // Mirror the production path: the parser decides Crashed, then + // normalizeWorkflowResult classifies a non-zero exit. + result, err := ParseActOutput(tt.logs) + if err != nil { + t.Fatalf("ParseActOutput returned error: %v", err) + } + normalizeWorkflowResult(result, ".github/workflows/orchestrate.yaml", 2) + + if result.Conclusion != "failure" { + t.Fatalf("Conclusion = %q, want failure", result.Conclusion) + } + if !result.ExecError { + t.Fatalf("an act-only dump must be tagged transient (ExecError) so it is retried") + } + failErr := workflowFailureError("orchestrate", result) + if !IsTransientWorkflowError(failErr) { + t.Fatalf("an act-only dump failure must be classified transient: %v", failErr) + } + }) + } +} diff --git a/e2e/harness/parser.go b/e2e/harness/parser.go index de6ec577..4d7aef55 100644 --- a/e2e/harness/parser.go +++ b/e2e/harness/parser.go @@ -50,27 +50,51 @@ type ExtendedWorkflowResult struct { CrashReason string } -// goroutineDumpRE matches the header of a Go goroutine dump, e.g. -// "goroutine 1 [running]:". This is the anchored runtime signature emitted on a -// panic or fatal error; it does not match prose that merely contains the word -// "goroutine". -var goroutineDumpRE = regexp.MustCompile(`(?m)^goroutine \d+ \[`) - // signalCrashRE matches a Go runtime fatal-signal line, e.g. // "[signal SIGSEGV: segmentation violation ...]" or a SIGABRT report. var signalCrashRE = regexp.MustCompile(`signal SIG(SEGV|ABRT|BUS|FPE|ILL)`) +// cascadeFrameRE matches a Go stack-trace frame that belongs to cascade's own +// code, e.g. "github.com/stablekernel/cascade/internal/orchestrate.(*Planner).Plan(...)" +// or "github.com/stablekernel/cascade/cmd/cascade.run(...)". It is deliberately +// strict: the package path must be under cascade's internal or cmd tree AND be +// followed by a "." and a function/receiver identifier, so it matches a real +// stack frame and not git-URL prose, the bare word "cascade", stdlib packages +// such as "internal/poll", or vendored "internal" packages like +// "go.opentelemetry.io/otel/internal/global". +var cascadeFrameRE = regexp.MustCompile(`stablekernel/cascade/(internal|cmd)[^\s]*\.[A-Za-z0-9_]`) + // detectCrashSignature reports whether raw act stdout/stderr carries a Go -// runtime crash signature, returning the first matched signature line as the -// reason. It anchors on real runtime signatures (a goroutine dump header, a -// "panic:" or "fatal error:" at the start of a line, a fatal signal report) so -// it does not misfire on benign occurrences of the words "panic" or -// "goroutine" inside an ordinary log line (e.g. a deploy script's prose). +// runtime crash originating in cascade's own code, returning the matched crash +// trigger line as the reason. +// +// A run is classified as a cascade crash only when BOTH conditions hold: +// +// 1. A real crash TRIGGER is present: a line beginning (trimmed) with "panic:" +// or "fatal error:", or a fatal-signal report ("signal SIGSEGV" and +// friends). +// 2. The dump contains a real cascade STACK FRAME (see cascadeFrameRE). +// +// This pairing is intentional. act routinely prints full goroutine dumps on its +// own cancellation/teardown paths (for example monitorJobCancellation under +// cancel-in-progress emits a bare "goroutine N [...]:" dump with no panic and +// only nektos/act frames). A bare goroutine dump, or a panic that carries only +// act/docker/stdlib frames, is act-infrastructure noise that a retry can absorb, +// not a cascade defect. Requiring both a trigger and a cascade frame keeps a +// genuine cascade-CLI panic a definitive failure while letting infra flakes stay +// transient. A bare goroutine-dump header is therefore NOT a trigger. func detectCrashSignature(logs string) (bool, string) { if logs == "" { return false, "" } + // First, require a real cascade stack frame anywhere in the dump. Without a + // cascade frame the crash (if any) is act/docker/stdlib only, which is + // infrastructure noise a retry should absorb. + if !cascadeFrameRE.MatchString(logs) { + return false, "" + } + scanner := bufio.NewScanner(strings.NewReader(logs)) // A crash dump line can be long (a deeply nested stack frame); raise the // scanner's token limit so a long frame cannot truncate detection. @@ -83,8 +107,6 @@ func detectCrashSignature(logs string) (bool, string) { return true, trimmed case strings.HasPrefix(trimmed, "fatal error:"): return true, trimmed - case goroutineDumpRE.MatchString(line): - return true, trimmed case signalCrashRE.MatchString(line): return true, trimmed }