From 4e7d3f1f780630d8ef1ccefc4bdc9213a6a6e5fa Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sun, 14 Jun 2026 17:50:55 -0400 Subject: [PATCH 1/2] fix(e2e): cap scenario concurrency and narrow transient classifier Each scenario stands up its own docker network + gitea + act with nested act job containers. Run the heavy hotfix-family scenarios concurrently and they oversubscribe the docker address pool, host RAM, or gitea; act then exits non-zero with no parsed job failure and no cascade crash frame, lands in the classifier catch-all transient bucket, and burns the whole retry budget against the same ceiling. Add a host-sized scenarioConcurrency semaphore (independent of go test -parallel, E2E_MAX_CONCURRENT override) so heavy stacks serialize and stop self-inflicting the contention. Narrow the transient classifier so a non-zero act exit is retryable ONLY when a named infra-saturation signature is present (address-pool exhaustion, no space left, cannot allocate memory, gitea connection reset/refused, docker daemon unreachable); every other job-less, crash-free non-zero exit is now deterministic. Cut the retry budget from 5 to 2. Justified retries (gitea-405, network removal, convergence polls, product push) are untouched. Signed-off-by: Joshua Temple --- e2e/harness/act.go | 25 +++++- e2e/harness/concurrency.go | 101 ++++++++++++++++++++++ e2e/harness/concurrency_test.go | 142 +++++++++++++++++++++++++++++++ e2e/harness/crash_test.go | 46 +++++++--- e2e/harness/memory_darwin.go | 26 ++++++ e2e/harness/memory_linux.go | 38 +++++++++ e2e/harness/memory_other.go | 9 ++ e2e/harness/parser.go | 65 ++++++++++++++ e2e/harness/scenario_retry.go | 31 +++++-- e2e/harness/transient_test.go | 144 ++++++++++++++++++++++++++++---- 10 files changed, 593 insertions(+), 34 deletions(-) create mode 100644 e2e/harness/concurrency.go create mode 100644 e2e/harness/concurrency_test.go create mode 100644 e2e/harness/memory_darwin.go create mode 100644 e2e/harness/memory_linux.go create mode 100644 e2e/harness/memory_other.go diff --git a/e2e/harness/act.go b/e2e/harness/act.go index 0c4a81b5..1d00d808 100644 --- a/e2e/harness/act.go +++ b/e2e/harness/act.go @@ -389,6 +389,17 @@ func (a *ActRunner) RunWorkflowFromRepo(ctx context.Context, opts RunOpts) (*Ext // workflow (e.g. a missing orchestrate.yaml). Without this, such a run // masqueraded as Conclusion="success" with 0 jobs. A missing workflow showing // up as a green-but-empty scenario (#25). +// +// Transient classification is DELIBERATELY narrow. A non-zero act exit is only +// tagged transient (ExecError true, retryable) when the raw output carries a +// NAMED host-saturation signature (see detectInfraSaturation): docker +// address-pool exhaustion (#125), disk-full, OOM, a gitea connection +// reset/refused (#121), or "Cannot connect to the Docker daemon". Those are +// genuine, externally-imposed transients a clean-slate retry can clear. Every +// OTHER non-zero exit with no job failure and no crash signature is now a +// DETERMINISTIC failure: it fails on the first attempt and surfaces a bug +// instead of being silently retried away. This closes the masking bucket that +// let self-inflicted contention burn the whole retry budget. func normalizeWorkflowResult(result *ExtendedWorkflowResult, workflowPath string, exitCode int) { if exitCode != 0 { // Classify BEFORE forcing the conclusion to "failure": hasJobFailure @@ -414,10 +425,16 @@ func normalizeWorkflowResult(result *ExtendedWorkflowResult, workflowPath string result.Error = "workflow execution failed" default: // A non-zero exit with no parsed job failure and no crash signature - // is a benign act/docker transport or exec hiccup: tag it transient - // so the scenario runner may retry from a clean slate. - result.ExecError = true - result.Error = "workflow execution failed" + // is transient ONLY when a named host-saturation signature is in the + // raw output; otherwise it is a deterministic real failure that must + // surface (not be masked by a retry). + if saturated, reason := detectInfraSaturation(infraLogs(result)); saturated { + result.ExecError = true + result.Error = fmt.Sprintf("infra saturation: %s", reason) + } else { + result.ExecError = false + result.Error = "workflow execution failed" + } } // A non-zero exit is always a failure; set the conclusion after // classification so hasJobFailure above saw the pre-exit conclusion. diff --git a/e2e/harness/concurrency.go b/e2e/harness/concurrency.go new file mode 100644 index 00000000..d8a5deef --- /dev/null +++ b/e2e/harness/concurrency.go @@ -0,0 +1,101 @@ +package harness + +import ( + "context" + "os" + "runtime" + "strconv" + "sync" +) + +// maxConcurrentEnv is the environment variable that overrides the computed +// scenario-concurrency cap. A positive integer pins the cap exactly (clamped to +// at least 1); any other value (unset, empty, zero, negative, non-numeric) falls +// back to the host-derived default. This lets a constrained or oversized runner +// tune the cap without a code change, e.g. E2E_MAX_CONCURRENT=1 to force fully +// serial heavy-scenario setup. +const maxConcurrentEnv = "E2E_MAX_CONCURRENT" + +// scenarioConcurrency is a process-wide semaphore that bounds how many scenarios +// stand up their docker/gitea/act stack CONCURRENTLY, independent of +// `go test -parallel`. Each scenario stands up its own docker network + gitea +// container + act container, and act spawns nested job containers per workflow +// run; the heaviest (hotfix-family) scenarios hold many concurrent network +// endpoints, containers, and memory. Run enough of them at once and they +// oversubscribe the docker address pool (#125), host RAM, or gitea (#121) - the +// self-inflicted contention that the old five-attempt retry budget masked. +// `go test -parallel` alone cannot bound this because it counts test functions, +// not the heavyweight infra each one provisions. This semaphore caps the real +// resource pressure so heavy scenarios serialize against each other under load +// and infra-saturation transients stop happening in the first place. +var scenarioConcurrency = make(chan struct{}, computeScenarioConcurrency()) + +// computeScenarioConcurrency derives the concurrency cap. An E2E_MAX_CONCURRENT +// override (positive integer) wins. Otherwise the cap is host-derived and +// deliberately conservative, because each admitted scenario provisions a full +// docker/gitea/act stack plus nested job containers: +// +// - From CPUs: floor(NumCPU/2), so a 4-core runner admits 2 and an 8-core +// runner admits 4. Halving leaves headroom for the docker daemon, gitea, and +// act's own orchestration rather than saturating every core with job +// containers. +// - From memory: floor(totalGiB/3), budgeting ~3 GiB per concurrent stack +// (gitea + act + nested job containers). On an 8 GiB runner that is 2. +// - The cap is the MIN of the two (the binding resource), floored at 1 so the +// suite always makes progress even on a tiny host. Total memory is read +// best-effort; when it is unknown the CPU bound alone applies. +// +// On the diagnosis's 4-core / ~8 GiB reference runner this yields min(2, 2) = 2, +// which keeps the heavy hotfix scenarios from oversubscribing docker/gitea. +func computeScenarioConcurrency() int { + if n, ok := parsePositiveInt(os.Getenv(maxConcurrentEnv)); ok { + return n + } + + limit := runtime.NumCPU() / 2 + if limit < 1 { + limit = 1 + } + + if gib := totalMemoryGiB(); gib > 0 { + memLimit := gib / 3 + if memLimit < 1 { + memLimit = 1 + } + if memLimit < limit { + limit = memLimit + } + } + + return limit +} + +// parsePositiveInt parses s as a positive integer, returning (n, true) only for +// a strictly-positive value. Empty, non-numeric, zero, and negative inputs +// return (0, false) so the caller falls back to the host-derived default. +func parsePositiveInt(s string) (int, bool) { + if s == "" { + return 0, false + } + n, err := strconv.Atoi(s) + if err != nil || n < 1 { + return 0, false + } + return n, true +} + +// acquireScenarioSlot blocks until a scenario-concurrency slot is free (or ctx +// is cancelled), then returns a release func the caller must invoke (via defer) +// to return the slot. Acquiring before SetupInfra and releasing after Cleanup +// bounds how many docker/gitea/act stacks exist at once. On ctx cancellation it +// returns a non-nil error and a no-op release, so a cancelled caller neither +// blocks nor double-releases. +func acquireScenarioSlot(ctx context.Context) (release func(), err error) { + select { + case scenarioConcurrency <- struct{}{}: + var once sync.Once + return func() { once.Do(func() { <-scenarioConcurrency }) }, nil + case <-ctx.Done(): + return func() {}, ctx.Err() + } +} diff --git a/e2e/harness/concurrency_test.go b/e2e/harness/concurrency_test.go new file mode 100644 index 00000000..5951134e --- /dev/null +++ b/e2e/harness/concurrency_test.go @@ -0,0 +1,142 @@ +package harness + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" +) + +func TestParsePositiveInt(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + wantN int + wantOK bool + }{ + {name: "empty falls back", in: "", wantN: 0, wantOK: false}, + {name: "zero falls back", in: "0", wantN: 0, wantOK: false}, + {name: "negative falls back", in: "-3", wantN: 0, wantOK: false}, + {name: "non-numeric falls back", in: "abc", wantN: 0, wantOK: false}, + {name: "one is honored", in: "1", wantN: 1, wantOK: true}, + {name: "larger value is honored", in: "6", wantN: 6, wantOK: true}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + n, ok := parsePositiveInt(tt.in) + if n != tt.wantN || ok != tt.wantOK { + t.Fatalf("parsePositiveInt(%q) = (%d,%v), want (%d,%v)", tt.in, n, ok, tt.wantN, tt.wantOK) + } + }) + } +} + +// TestComputeScenarioConcurrency_EnvOverride pins the cap via E2E_MAX_CONCURRENT +// and confirms a positive override wins over the host-derived default while an +// invalid one falls back to a sane (>=1) host value. +func TestComputeScenarioConcurrency_EnvOverride(t *testing.T) { + t.Setenv(maxConcurrentEnv, "3") + if got := computeScenarioConcurrency(); got != 3 { + t.Fatalf("computeScenarioConcurrency() with override=3 = %d, want 3", got) + } + + t.Setenv(maxConcurrentEnv, "bogus") + if got := computeScenarioConcurrency(); got < 1 { + t.Fatalf("computeScenarioConcurrency() with invalid override = %d, want >= 1", got) + } +} + +// TestComputeScenarioConcurrency_HostDefault confirms the default is always at +// least 1 (the suite must make progress on any host) when no override is set. +func TestComputeScenarioConcurrency_HostDefault(t *testing.T) { + t.Setenv(maxConcurrentEnv, "") + if got := computeScenarioConcurrency(); got < 1 { + t.Fatalf("computeScenarioConcurrency() host default = %d, want >= 1", got) + } +} + +// TestAcquireScenarioSlot_BoundsConcurrency proves the semaphore never admits +// more than its capacity at once: with a cap of 2 and many contending +// goroutines, the observed in-flight count never exceeds 2. +func TestAcquireScenarioSlot_BoundsConcurrency(t *testing.T) { + const capLimit = 2 + // Swap the package semaphore for a capacity-controlled one for this test, + // then restore it so other tests see the real cap. + orig := scenarioConcurrency + scenarioConcurrency = make(chan struct{}, capLimit) + t.Cleanup(func() { scenarioConcurrency = orig }) + + var ( + inFlight int32 + maxSeen int32 + wg sync.WaitGroup + ) + for i := 0; i < 20; i++ { + wg.Add(1) + go func() { + defer wg.Done() + release, err := acquireScenarioSlot(context.Background()) + if err != nil { + t.Errorf("acquireScenarioSlot returned error: %v", err) + return + } + defer release() + + cur := atomic.AddInt32(&inFlight, 1) + for { + prev := atomic.LoadInt32(&maxSeen) + if cur <= prev || atomic.CompareAndSwapInt32(&maxSeen, prev, cur) { + break + } + } + time.Sleep(2 * time.Millisecond) + atomic.AddInt32(&inFlight, -1) + }() + } + wg.Wait() + + if maxSeen > capLimit { + t.Fatalf("max concurrent slots = %d, want <= %d", maxSeen, capLimit) + } + // All slots must have been returned: a fresh acquire must not block. + release, err := acquireScenarioSlot(context.Background()) + if err != nil { + t.Fatalf("post-run acquire errored: %v", err) + } + release() +} + +// TestAcquireScenarioSlot_ContextCancelled confirms a caller cancelled while the +// semaphore is full gets a non-nil error and a no-op release (never blocks, +// never frees a slot it does not hold). +func TestAcquireScenarioSlot_ContextCancelled(t *testing.T) { + orig := scenarioConcurrency + scenarioConcurrency = make(chan struct{}, 1) + t.Cleanup(func() { scenarioConcurrency = orig }) + + // Fill the only slot. + hold, err := acquireScenarioSlot(context.Background()) + if err != nil { + t.Fatalf("initial acquire errored: %v", err) + } + defer hold() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + release, err := acquireScenarioSlot(ctx) + if err == nil { + t.Fatalf("acquireScenarioSlot with cancelled ctx returned nil error") + } + // The returned release must be a safe no-op (must not free the held slot). + release() + select { + case scenarioConcurrency <- struct{}{}: + t.Fatalf("no-op release freed a slot it did not own (semaphore under-counted)") + default: + } +} diff --git a/e2e/harness/crash_test.go b/e2e/harness/crash_test.go index 4f5886eb..b9cd9200 100644 --- a/e2e/harness/crash_test.go +++ b/e2e/harness/crash_test.go @@ -241,12 +241,16 @@ func TestNormalizeWorkflowResult_CrashIsNotTransient(t *testing.T) { } } -// 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) { +// TestNormalizeWorkflowResult_ActOnlyDumpIsDeterministic is the sibling +// guarantee to the crash test, updated for the narrowed classifier: a non-zero +// act exit whose logs carry only an act/docker dump (no cascade frame) is still +// correctly NOT flagged Crashed by the parser - the #146/#147 crash-frame logic +// is intact - but it is NO LONGER auto-retried. With no cascade crash frame and +// no named infra-saturation signature, it is a deterministic failure that must +// surface on the first attempt rather than being masked by the old catch-all +// transient bucket. An infra signature in the SAME shape of dump is what makes +// it transient again (covered by the infra subtest below). +func TestNormalizeWorkflowResult_ActOnlyDumpIsDeterministic(t *testing.T) { t.Parallel() for _, tt := range []struct { @@ -265,18 +269,40 @@ func TestNormalizeWorkflowResult_ActOnlyDumpIsTransient(t *testing.T) { if err != nil { t.Fatalf("ParseActOutput returned error: %v", err) } + if result.Crashed { + t.Fatalf("an act-only dump must NOT be flagged Crashed (no cascade frame)") + } 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") + if result.ExecError { + t.Fatalf("an act-only dump with no infra signature must be deterministic (not retried)") } failErr := workflowFailureError("orchestrate", result) - if !IsTransientWorkflowError(failErr) { - t.Fatalf("an act-only dump failure must be classified transient: %v", failErr) + if IsTransientWorkflowError(failErr) { + t.Fatalf("an act-only dump with no infra signature must NOT be classified transient: %v", failErr) } }) } + + // The same act-only dump shape, but carrying a named infra-saturation + // signature, IS a genuine retryable transient - proving the gate is the + // signature, not the dump. + t.Run("act dump with infra signature is transient", func(t *testing.T) { + t.Parallel() + logs := bareActGoroutineDump + "\nError response from daemon: all predefined address pools have been fully subnetted\n" + result, err := ParseActOutput(logs) + if err != nil { + t.Fatalf("ParseActOutput returned error: %v", err) + } + normalizeWorkflowResult(result, ".github/workflows/orchestrate.yaml", 2) + if !result.ExecError { + t.Fatalf("an act dump carrying an infra signature must be transient (ExecError)") + } + if !IsTransientWorkflowError(workflowFailureError("orchestrate", result)) { + t.Fatalf("an act dump carrying an infra signature must be classified transient") + } + }) } diff --git a/e2e/harness/memory_darwin.go b/e2e/harness/memory_darwin.go new file mode 100644 index 00000000..79d08304 --- /dev/null +++ b/e2e/harness/memory_darwin.go @@ -0,0 +1,26 @@ +//go:build darwin + +package harness + +import ( + "os/exec" + "strconv" + "strings" +) + +// totalMemoryGiB returns the host's total physical memory in whole GiB on +// darwin, read best-effort via `sysctl -n hw.memsize` (bytes). It returns 0 when +// the value cannot be read or parsed so the caller falls back to the CPU-only +// concurrency bound. Reading via sysctl avoids promoting a memory-introspection +// dependency to direct for what is a best-effort sizing hint. +func totalMemoryGiB() int { + out, err := exec.Command("sysctl", "-n", "hw.memsize").Output() + if err != nil { + return 0 + } + bytes, err := strconv.ParseUint(strings.TrimSpace(string(out)), 10, 64) + if err != nil { + return 0 + } + return int(bytes / (1 << 30)) +} diff --git a/e2e/harness/memory_linux.go b/e2e/harness/memory_linux.go new file mode 100644 index 00000000..b3aac812 --- /dev/null +++ b/e2e/harness/memory_linux.go @@ -0,0 +1,38 @@ +//go:build linux + +package harness + +import ( + "bufio" + "os" + "strconv" + "strings" +) + +// totalMemoryGiB returns the host's total physical memory in whole GiB on +// linux, read best-effort from /proc/meminfo's MemTotal (kibibytes). It returns +// 0 when the file cannot be read or the field cannot be parsed so the caller +// falls back to the CPU-only concurrency bound. Reading /proc directly avoids +// promoting a memory-introspection dependency to direct for what is a +// best-effort sizing hint. +func totalMemoryGiB() int { + f, err := os.Open("/proc/meminfo") + if err != nil { + return 0 + } + defer func() { _ = f.Close() }() + + scanner := bufio.NewScanner(f) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + // MemTotal: 16331156 kB + if len(fields) >= 2 && fields[0] == "MemTotal:" { + kib, err := strconv.ParseUint(fields[1], 10, 64) + if err != nil { + return 0 + } + return int(kib / (1 << 20)) + } + } + return 0 +} diff --git a/e2e/harness/memory_other.go b/e2e/harness/memory_other.go new file mode 100644 index 00000000..76720f7b --- /dev/null +++ b/e2e/harness/memory_other.go @@ -0,0 +1,9 @@ +//go:build !darwin && !linux + +package harness + +// totalMemoryGiB returns 0 on platforms without a best-effort total-memory +// probe, so the scenario-concurrency cap falls back to the CPU-only bound. The +// e2e suite runs on darwin and linux; this stub keeps the package buildable +// everywhere without a platform-specific dependency. +func totalMemoryGiB() int { return 0 } diff --git a/e2e/harness/parser.go b/e2e/harness/parser.go index 4d7aef55..af88bfd2 100644 --- a/e2e/harness/parser.go +++ b/e2e/harness/parser.go @@ -114,6 +114,71 @@ func detectCrashSignature(logs string) (bool, string) { return false, "" } +// infraSaturationSignatures are the NAMED, externally-imposed host-saturation +// strings whose presence in act's raw stdout/stderr makes a non-zero act exit a +// genuine, retryable transient. Each entry names a specific saturation the +// harness itself can provoke under concurrency (and a clean-slate retry can +// clear), NOT a generic "act exited non-zero" mystery: +// +// - docker address-pool exhaustion (#125): the daemon has no free subnet left +// to create the scenario network. +// - disk-full: no host disk for image layers / container writable layers. +// - out-of-memory: the host cannot allocate memory for a job container. +// - gitea connection reset/refused (#121): gitea is overloaded and dropping or +// refusing connections. +// - docker daemon unreachable: the socket is momentarily unavailable. +// +// Matching is case-insensitive (see detectInfraSaturation). Anything NOT on this +// list is deterministic: a non-zero act exit with no job failure, no crash +// signature, and none of these signatures is a real failure that must surface, +// not a flake to retry away. +var infraSaturationSignatures = []string{ + // docker address-pool exhaustion (#125) + "all predefined address pools have been fully subnetted", + "could not find an available, non-overlapping ipv4 address pool", + // disk-full + "no space left on device", + // out-of-memory + "cannot allocate memory", + "fatal error: out of memory", + "oom-kill", + "oomkilled", + // gitea overload (#121) + "connection reset by peer", + "connection refused", + // docker daemon unreachable + "cannot connect to the docker daemon", +} + +// detectInfraSaturation reports whether raw act stdout/stderr carries a named +// host-saturation signature (see infraSaturationSignatures), returning the +// matched signature as the reason. The match is case-insensitive so a signature +// is caught regardless of how docker/gitea cased it. An empty input never +// matches. This is the ONLY gate that makes a job-less, crash-free non-zero act +// exit retryable; every other such exit is deterministic. +func detectInfraSaturation(logs string) (bool, string) { + if logs == "" { + return false, "" + } + lower := strings.ToLower(logs) + for _, sig := range infraSaturationSignatures { + if strings.Contains(lower, sig) { + return true, sig + } + } + return false, "" +} + +// infraLogs returns the raw act stdout/stderr to scan for an infra-saturation +// signature, tolerating a nil result so the classifier can call it +// unconditionally. +func infraLogs(result *ExtendedWorkflowResult) string { + if result == nil { + return "" + } + return result.Logs +} + // JobResultExtended contains detailed result of a single job type JobResultExtended struct { Name string diff --git a/e2e/harness/scenario_retry.go b/e2e/harness/scenario_retry.go index f4de2c13..c73935f3 100644 --- a/e2e/harness/scenario_retry.go +++ b/e2e/harness/scenario_retry.go @@ -15,13 +15,16 @@ import ( // scenarioRetryAttempts bounds how many times a single multi-step scenario is // run end to end. Each attempt runs against a fresh gitea repo and fresh act // containers, so a retry starts from a clean slate with no partial mutation -// carried over. Only transient act/docker execution failures consume an -// attempt; real assertion or job-level failures fail on the first attempt. +// carried over. Only NAMED infra-saturation transients (see +// detectInfraSaturation) consume an attempt; real assertion, crash, or +// job-level failures fail deterministically on the first attempt. // -// Five attempts gives contention-driven transients a couple more chances under -// heavy CI load: the recovery logs show several scenarios passing on attempt 2 -// or 3, so the mechanism works and the extra headroom covers the slowest tail. -const scenarioRetryAttempts = 5 +// Two attempts (one retry) is the safety net now that the harness concurrency +// cap (see scenarioConcurrency) removes the self-inflicted contention the old +// five-attempt budget existed to absorb. With oversubscription gone, a genuine +// named transient is a rare one-off that a single clean-slate retry clears; a +// deterministic failure must not be retried at all, so it fails on attempt 1. +const scenarioRetryAttempts = 2 // scenarioRetryBackoff is the pause between scenario attempts. It lets a burst // of container/docker contention subside before the next clean-slate attempt @@ -157,9 +160,25 @@ func pruneDockerNetworks(ctx context.Context) { // to a real conclusion). A real job-level failure, an expect_failure mismatch, // or any state/branch/tag assertion mismatch fails deterministically on the // first attempt. +// +// Each attempt acquires a scenarioConcurrency slot BEFORE standing up its +// docker/gitea/act stack and releases it AFTER teardown, so no more than the +// host-derived cap of heavyweight stacks exist at once regardless of +// `go test -parallel`. This is what prevents the self-inflicted oversubscription +// (docker address pool / RAM / gitea) that the retry budget used to mask. func RunMultiStepScenario(ctx context.Context, t *testing.T, scenario *MultiStepScenario) error { t.Helper() return runScenarioWithRetry(ctx, t, scenario.Name, func(ctx context.Context) error { + // Bound concurrent stack standup independent of -parallel. Acquire before + // SetupInfra so the docker network + gitea + act containers (and act's + // nested job containers) only come up once a slot is free; release after + // Cleanup so the slot covers the stack's entire lifetime. + release, err := acquireScenarioSlot(ctx) + if err != nil { + return err + } + defer release() + h := New(t) defer h.Cleanup() diff --git a/e2e/harness/transient_test.go b/e2e/harness/transient_test.go index e5100f4a..857490d0 100644 --- a/e2e/harness/transient_test.go +++ b/e2e/harness/transient_test.go @@ -104,16 +104,20 @@ func TestWorkflowFailureError_TransientClassification(t *testing.T) { } } -// TestNormalizeWorkflowResult_ExecErrorTagging verifies that only an act/docker -// exec hiccup (non-zero exit) is tagged transient, while a real "no jobs -// parsed" failure (a missing or unloadable workflow) and a successful run are -// not. +// TestNormalizeWorkflowResult_ExecErrorTagging verifies the NARROWED transient +// classifier: a non-zero act exit is tagged transient (ExecError true, +// retryable) ONLY when the raw logs carry a named infra-saturation signature. +// Every other job-less, crash-free non-zero exit is now a DETERMINISTIC failure +// (ExecError false, not retried) - this is the load-bearing proof that the +// catch-all masking bucket is gone. Real job failures, crashes, missing +// workflows, and successful runs keep their prior classification. func TestNormalizeWorkflowResult_ExecErrorTagging(t *testing.T) { t.Parallel() tests := []struct { name string jobs map[string]*JobResultExtended + logs string crashed bool crashReason string workflowPath string @@ -122,20 +126,83 @@ func TestNormalizeWorkflowResult_ExecErrorTagging(t *testing.T) { wantExecError bool }{ { - // Preserved behavior: a non-zero exit with no parsed jobs and NO - // crash signature is a benign act/docker transport hiccup and stays - // transient (safe to retry from a clean slate). - name: "non-zero exit with no jobs and no crash signature tags transient exec error", + // LOAD-BEARING: a non-zero exit with no parsed jobs, no crash + // signature, and NO infra signature is no longer swallowed as a + // transient flake. It is now a deterministic real failure that fails + // on attempt 1 and surfaces a bug instead of burning the retry budget. + name: "non-infra non-zero exit is NOT transient", jobs: map[string]*JobResultExtended{}, + logs: "act exited 1: some unexpected step error with no named infra signature", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: false, + }, + { + // New: a non-zero exit carrying a named docker address-pool + // exhaustion signature (#125) is a genuine, retryable infra transient. + name: "address-pool exhaustion is transient", + jobs: map[string]*JobResultExtended{}, + logs: "Error response from daemon: all predefined address pools have been fully subnetted", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: true, + }, + { + name: "no space left on device is transient", + jobs: map[string]*JobResultExtended{}, + logs: "write /var/lib/docker/tmp/x: no space left on device", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: true, + }, + { + name: "cannot allocate memory is transient", + jobs: map[string]*JobResultExtended{}, + logs: "fork/exec: cannot allocate memory", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: true, + }, + { + name: "gitea connection reset is transient", + jobs: map[string]*JobResultExtended{}, + logs: "fatal: unable to access gitea: Recv failure: Connection reset by peer", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: true, + }, + { + name: "docker daemon unreachable is transient", + jobs: map[string]*JobResultExtended{}, + logs: "Cannot connect to the Docker daemon at unix:///var/run/docker.sock", workflowPath: ".github/workflows/promote.yaml", exitCode: 1, wantConclusion: "failure", wantExecError: true, }, { - // New: a non-zero exit with no parsed jobs but carrying a Go-runtime - // crash signature is a REAL crash, not a flake. ExecError must stay - // false so the scenario runner does not retry it away. + // An infra signature must NOT override a real, parsed job failure: + // the job-failure branch is checked first and stays deterministic. + name: "real job failure beats an infra signature in logs", + jobs: map[string]*JobResultExtended{ + "build": {Name: "build", Conclusion: "failure"}, + }, + logs: "connection refused (noise) but a job really failed", + workflowPath: ".github/workflows/promote.yaml", + exitCode: 1, + wantConclusion: "failure", + wantExecError: false, + }, + { + // A non-zero exit with no parsed jobs but carrying a Go-runtime crash + // signature is a REAL crash, not a flake. ExecError must stay false so + // the scenario runner does not retry it away - even if the logs also + // contain an infra phrase, the crash branch wins. name: "non-zero exit with crash signature is not transient", jobs: map[string]*JobResultExtended{}, crashed: true, @@ -147,9 +214,9 @@ func TestNormalizeWorkflowResult_ExecErrorTagging(t *testing.T) { }, { // Regression: act exits non-zero when a job genuinely concludes - // "failure". That is a real, deterministic defect, not an - // act/docker transport hiccup, so ExecError must stay false and the - // scenario runner must NOT retry it. + // "failure". That is a real, deterministic defect, not an act/docker + // transport hiccup, so ExecError must stay false and the scenario + // runner must NOT retry it. name: "non-zero exit with a failed job is a real failure not transient", jobs: map[string]*JobResultExtended{ "build": {Name: "build", Conclusion: "failure"}, @@ -184,6 +251,7 @@ func TestNormalizeWorkflowResult_ExecErrorTagging(t *testing.T) { result := &ExtendedWorkflowResult{ Conclusion: "success", Jobs: tt.jobs, + Logs: tt.logs, Crashed: tt.crashed, CrashReason: tt.crashReason, } @@ -197,3 +265,51 @@ func TestNormalizeWorkflowResult_ExecErrorTagging(t *testing.T) { }) } } + +// TestDetectInfraSaturation verifies that each named host-saturation signature +// is matched (case-insensitively) and that unrelated output is not, since this +// detector is the sole gate that makes a job-less, crash-free non-zero act exit +// retryable. +func TestDetectInfraSaturation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + logs string + wantMatch bool + }{ + {name: "empty never matches", logs: "", wantMatch: false}, + {name: "ordinary failure does not match", logs: "step 'build' failed with exit code 1", wantMatch: false}, + { + name: "address pool exhaustion matches", + logs: "all predefined address pools have been fully subnetted", + wantMatch: true, + }, + { + name: "case-insensitive address pool matches", + logs: "Error: All Predefined Address Pools Have Been Fully Subnetted", + wantMatch: true, + }, + {name: "overlapping pool matches", logs: "could not find an available, non-overlapping IPv4 address pool", wantMatch: true}, + {name: "no space matches", logs: "no space left on device", wantMatch: true}, + {name: "cannot allocate memory matches", logs: "cannot allocate memory", wantMatch: true}, + {name: "oomkilled matches", logs: "container state: OOMKilled", wantMatch: true}, + {name: "connection reset matches", logs: "Connection reset by peer", wantMatch: true}, + {name: "connection refused matches", logs: "dial tcp: connection refused", wantMatch: true}, + {name: "docker daemon unreachable matches", logs: "Cannot connect to the Docker daemon", wantMatch: true}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, reason := detectInfraSaturation(tt.logs) + if got != tt.wantMatch { + t.Fatalf("detectInfraSaturation(%q) = %v (reason %q), want %v", tt.logs, got, reason, tt.wantMatch) + } + if got && reason == "" { + t.Fatalf("detectInfraSaturation matched but returned empty reason for %q", tt.logs) + } + }) + } +} From ececc4274c16f4f1ea06d35d869b637aad845716 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sun, 14 Jun 2026 18:04:30 -0400 Subject: [PATCH 2/2] ci: upload e2e crash evidence in build-cli.yaml on failure Signed-off-by: Joshua Temple --- .github/workflows/build-cli.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/workflows/build-cli.yaml b/.github/workflows/build-cli.yaml index b2933852..05770f06 100644 --- a/.github/workflows/build-cli.yaml +++ b/.github/workflows/build-cli.yaml @@ -88,3 +88,19 @@ jobs: # the product. Serial execution removes that contention; the longer # 60m timeout covers the resulting slower wall-clock. run: go test -v -parallel 1 -timeout 60m ./... + + # On a retry-exhausted scenario the harness writes the last attempt's raw + # act stdout/stderr to e2e/_artifacts/-attempt.log so the + # stack-trace origin survives the CI log retention window. Upload it on + # every run (always()) so the evidence is recoverable whether the job + # failed or a flake was absorbed. The directory may not exist when no + # scenario exhausted its retries; if-no-files-found: ignore keeps that a + # clean no-op rather than a warning. + - name: Upload e2e crash evidence + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: e2e-crash-evidence + path: e2e/_artifacts/ + if-no-files-found: ignore + retention-days: 14