From b1303676cd3e3c895ccac1ce09206433afd981d0 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 11:08:49 -0400 Subject: [PATCH] fix(ci): keep other homes' containers out of the orphan sweep; failed or unplaced deps block waiters (D-1117, D-1119) The container sweep lists every labelled container on the daemon and treated an unknown coordinator as stale, so a run with another NSELF_CI_HOME could remove that home's live container. Only coordinators the store knows are now swept. depOK let jobs run after a failed allow_failure dependency (contrary to plan/dag.go) and after a prerequisite that was never placed, admitted or run. Both now block the waiter. --- free/ci/internal/exec/sweep.go | 16 ++++++ free/ci/internal/run/depok_test.go | 55 +++++++++++++++++++++ free/ci/internal/run/loop.go | 13 +++-- free/ci/internal/run/recovery.go | 18 ++++++- free/ci/internal/run/recovery_sweep_test.go | 53 ++++++++++++++++++++ 5 files changed, 151 insertions(+), 4 deletions(-) create mode 100644 free/ci/internal/run/depok_test.go create mode 100644 free/ci/internal/run/recovery_sweep_test.go diff --git a/free/ci/internal/exec/sweep.go b/free/ci/internal/exec/sweep.go index 5f6d10cc5..83d17f934 100644 --- a/free/ci/internal/exec/sweep.go +++ b/free/ci/internal/exec/sweep.go @@ -15,6 +15,13 @@ type StaleChecker interface { Stale(context.Context, string) (bool, error) } +// KnownChecker is an optional StaleChecker extension: Known reports whether the coordinator belongs to the state +// this sweep serves. The container listing is daemon-wide, so a container of a coordinator that is not known +// here may be live in another CI home and is never removed (D-1117). +type KnownChecker interface { + Known(context.Context, string) (bool, error) +} + // Sweep removes only labelled containers (docker or podman) owned by stale coordinators. func (e *Executor) Sweep(ctx context.Context, current string) error { home := os.Getenv("NSELF_CI_HOME") @@ -126,6 +133,15 @@ func (e *Executor) sweepRuntime(ctx context.Context, current, binary string) err if len(parts) != 2 || parts[1] == current { continue } + if k, ok := e.Stale.(KnownChecker); ok { + known, err := k.Known(ctx, parts[1]) + if err != nil { + return err + } + if !known { + continue + } + } stale, err := e.Stale.Stale(ctx, parts[1]) if err != nil { return err diff --git a/free/ci/internal/run/depok_test.go b/free/ci/internal/run/depok_test.go new file mode 100644 index 000000000..a31001f48 --- /dev/null +++ b/free/ci/internal/run/depok_test.go @@ -0,0 +1,55 @@ +package run + +// Purpose: what lets a job's waiters run (D-1119). A failed allow_failure job +// blocks them as plan/dag.go promises; a prerequisite that never ran blocks them too. + +import ( + "testing" + + "github.com/nself-org/plugins/free/ci/internal/model" +) + +func TestDepOK(t *testing.T) { + t.Parallel() + cases := []struct { + name string + d JobOutcome + want bool + }{ + {"passed", JobOutcome{State: StatePassed}, true}, + {"failed", JobOutcome{State: StateFailed}, false}, + {"failed allow_failure", JobOutcome{State: StateFailed, AllowFailure: true}, false}, + {"cancelled", JobOutcome{State: StateCancelled}, false}, + {"skipped not affected", JobOutcome{State: StateSkipped, Reason: "selection.affected"}, true}, + {"skipped by when", JobOutcome{State: StateSkipped, Reason: "when.paths"}, true}, + {"skipped dep.failed", JobOutcome{State: StateSkipped, Reason: "dep.failed"}, false}, + {"skipped fail-fast", JobOutcome{State: StateSkipped, Reason: "fail_fast.cancelled"}, false}, + {"skipped unplaceable", JobOutcome{State: StateSkipped, Reason: "unplaceable.tool"}, false}, + {"skipped admission", JobOutcome{State: StateSkipped, Reason: "admission.battery"}, false}, + {"skipped advisory", JobOutcome{State: StateSkipped, Reason: "advisory.tool_missing"}, false}, + } + for _, tc := range cases { + if got := depOK(tc.d); got != tc.want { + t.Errorf("%s: depOK = %v, want %v", tc.name, got, tc.want) + } + } +} + +func TestFailedAllowFailureJobBlocksItsWaiters(t *testing.T) { + t.Parallel() + opt := func(script string, deps ...string) model.Job { + j := sh(script, deps...) + j.Required = no() + return j + } + build := opt("exit 1") + build.AllowFailure = true + r := newRig(t, map[string]model.Job{"build": build, "publish": opt("true", "build")}, gitFake{}) + res := r.run(t) + if o := outcome(res, "build"); o.State != StateFailed { + t.Fatalf("build: %+v", o) + } + if o := outcome(res, "publish"); o.State != StateSkipped || o.Reason != "dep.failed" { + t.Fatalf("publish ran or was skipped for the wrong reason: %+v", o) + } +} diff --git a/free/ci/internal/run/loop.go b/free/ci/internal/run/loop.go index a39047f14..0b8d29257 100644 --- a/free/ci/internal/run/loop.go +++ b/free/ci/internal/run/loop.go @@ -13,6 +13,7 @@ package run import ( "context" + "strings" "sync" "github.com/nself-org/plugins/free/ci/internal/model" @@ -99,14 +100,20 @@ func cancelled(o JobOutcome, why model.Reason, detail string) JobOutcome { return o } +// depOK reports whether a finished dependency lets its waiters run. A failed +// job always blocks them, allow_failure or not (plan/dag.go: a failing +// allow_failure job still blocks everything that waits on it). A skipped job +// lets them run only when it was scoped out by design (selection, when.*, +// pipeline); one that never ran for want of a place, a tool or admission, or +// that was cancelled, blocks them like a failure. func depOK(d JobOutcome) bool { switch d.State { case StatePassed: return true - case StateFailed: - return d.AllowFailure case StateSkipped: - return d.Reason != "dep.failed" && d.Reason != "fail_fast.cancelled" + r := string(d.Reason) + return r != "dep.failed" && r != "fail_fast.cancelled" && + !strings.HasPrefix(r, "unplaceable.") && !strings.HasPrefix(r, "admission.") && !strings.HasPrefix(r, "advisory.") } return false } diff --git a/free/ci/internal/run/recovery.go b/free/ci/internal/run/recovery.go index 059ebeff4..95b102c33 100644 --- a/free/ci/internal/run/recovery.go +++ b/free/ci/internal/run/recovery.go @@ -54,7 +54,23 @@ func (c staleChecker) Stale(ctx context.Context, id string) (bool, error) { return false, nil } } - return true, nil // no such coordinator: its leftovers belong to nobody + return true, nil // no such coordinator: its own-home leftovers (process markers) belong to nobody; containers are gated by Known +} + +// Known reports whether the store has a coordinator with this (safe) id. The +// container sweep lists every labelled container on the daemon, so an id this +// store has never seen may be a live coordinator of another CI home. +func (c staleChecker) Known(ctx context.Context, id string) (bool, error) { + all, err := c.st.Coordinators(ctx) + if err != nil { + return false, err + } + for _, a := range all { + if safeID(a.ID) == id { + return true, nil + } + } + return false, nil } // Recover cleans up after coordinators that died. current is the live diff --git a/free/ci/internal/run/recovery_sweep_test.go b/free/ci/internal/run/recovery_sweep_test.go new file mode 100644 index 000000000..bf319477e --- /dev/null +++ b/free/ci/internal/run/recovery_sweep_test.go @@ -0,0 +1,53 @@ +//go:build !windows + +package run + +// Purpose: the container sweep of Recover removes only containers of coordinators +// this store knows to be stale; another CI home's container is left alone (D-1117). +// Constraints: a fake docker script on a PATH that holds nothing else; no real daemon. + +import ( + "context" + "os" + osexec "os/exec" + "path/filepath" + "strings" + "testing" + + ciexec "github.com/nself-org/plugins/free/ci/internal/exec" +) + +func TestRecoverSweepSkipsContainersOfUnknownCoordinators(t *testing.T) { + ciexec.ResetRuntimeInfoCache() + st := openState(t) + ctx := context.Background() + gone := osexec.Command("true") + if err := gone.Run(); err != nil { + t.Fatal(err) + } + dead, err := st.RegisterCoordinator(ctx, gone.Process.Pid, "t0", bootID()) + if err != nil { + t.Fatal(err) + } + bin := t.TempDir() + t.Setenv("PATH", bin) + home := t.TempDir() + t.Setenv("NSELF_CI_HOME", home) + listing := "dead1 " + safeID(dead.ID) + "\nforeign1 another-home-coordinator\n" + script := "#!/bin/sh\necho \"$*\" >> \"${0%/*}/docker.calls\"\n" + + "case \"$1\" in\ninfo) echo '{}';;\nps) printf '%s' '" + listing + "';;\nesac\nexit 0\n" + if err := os.WriteFile(filepath.Join(bin, "docker"), []byte(script), 0700); err != nil { + t.Fatal(err) + } + if rep := Recover(ctx, st, "", home); len(rep.Errors) > 0 { + t.Fatalf("recover errors: %v", rep.Errors) + } + calls, _ := os.ReadFile(filepath.Join(bin, "docker.calls")) + out := string(calls) + if !strings.Contains(out, "rm -f dead1") { + t.Errorf("the container of a stale known coordinator was not removed:\n%s", out) + } + if strings.Contains(out, "foreign1") { + t.Errorf("a container of a coordinator unknown to this store was removed:\n%s", out) + } +}