fix(runtime,cacheprune,imagegc): bound teardownContainer's exit wait; make cache clear --all force eviction

pkg/cacheprune/cacheprune.go

@@ -141,7 +141,7 @@ func (p *Pruner) Prune(ctx context.Context, targets []string, all bool) []Result
 		case TargetBuildKit:
 			results = append(results, p.pruneBuildKit(ctx, all))
 		case TargetContainerd:
-			results = append(results, p.pruneContainerd(ctx))
+			results = append(results, p.pruneContainerd(ctx, all))
 		}
 	}
 	return results
@@ -199,17 +199,63 @@ func (p *Pruner) pruneBuildKit(ctx context.Context, all bool) Result {
 	return res
 }
 
-// pruneContainerd runs one image-collector pass. Under no disk pressure
+// collectPass names which image-collector entry point a containerd prune
+// request maps to. Split out as a pure function so the routing is
+// table-testable without a live containerd — the bug it guards against is
+// exactly that the request's `all` flag never reached the collector at all.
+type collectPass string
+
+const (
+	// passWatermark is the ordinary policy pass: evict only what disk
+	// pressure justifies.
+	passWatermark collectPass = "watermark"
+	// passForced is the operator override: evict every unprotected record.
+	passForced collectPass = "forced"
+)
+
+func containerdPass(all bool) collectPass {
+	if all {
+		return passForced
+	}
+	return passWatermark
+}
+
+// pruneContainerd runs one image-collector pass.
+//
+// all=false runs the ordinary watermark-driven pass. Under no disk pressure
 // that is a no-op by design: the collector protects images referenced by
 // running containers and the node's pinned runner images, and evicting a
 // warm image store on demand would only cost a re-pull.
-func (p *Pruner) pruneContainerd(ctx context.Context) Result {
+//
+// all=true is the operator override. It used to be SILENTLY DROPPED — this
+// function took no all parameter at all, so `ephemerd cache clear containerd
+// --all` ran the same watermark pass, correctly evicted nothing on a node
+// whose thresholds were not tripped, and reported success with zero records
+// removed. An operator chasing disk on a Windows node had no way to force
+// eviction and no way to tell the flag was being ignored. It now forces the
+// policy open (CollectAll) while keeping the safety: live-container images
+// and pinned runner images are still an absolute veto.
+func (p *Pruner) pruneContainerd(ctx context.Context, all bool) Result {
 	res := Result{Name: TargetContainerd}
+	pass := containerdPass(all)
 	if p.ImageGC == nil {
-		res.Err = fmt.Errorf("image gc is disabled on this node")
+		// Naming the pass we WOULD have run is not decoration: it is the
+		// only externally visible evidence that the request's `all` flag
+		// reached this function at all, which is exactly the hop where it
+		// used to be dropped silently. It also tells an operator whose
+		// `--all` did nothing that the flag was understood and the
+		// collector is simply not configured on this node.
+		res.Err = fmt.Errorf("image gc is disabled on this node (requested pass: %s)", pass)
 		return res
 	}
-	gcRes, err := p.ImageGC.Collect(ctx)
+	var gcRes imagegc.Result
+	var err error
+	switch pass {
+	case passForced:
+		gcRes, err = p.ImageGC.CollectAll(ctx)
+	default:
+		gcRes, err = p.ImageGC.Collect(ctx)
+	}
 	if err != nil {
 		res.Err = err
 		return res

pkg/cacheprune/cacheprune_test.go

@@ -1,7 +1,9 @@
 package cacheprune
 
 import (
+	"context"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -73,3 +75,82 @@ func TestResolveTargets(t *testing.T) {
 func TestPruner_ImplementsInterface(t *testing.T) {
 	var _ Interface = (*Pruner)(nil)
 }
+
+// TestContainerdPass pins the fix for a documented paper cut: pruneContainerd
+// used to take no `all` parameter at all, so `ephemerd cache clear containerd
+// --all` was accepted, silently downgraded to the ordinary watermark pass,
+// and reported "0 records removed" on a node whose thresholds were not
+// tripped. The operator had no override and no hint the flag was ignored.
+func TestContainerdPass(t *testing.T) {
+	if got := containerdPass(false); got != passWatermark {
+		t.Errorf("containerdPass(false) = %q, want %q — a plain prune must stay policy-driven", got, passWatermark)
+	}
+	if got := containerdPass(true); got != passForced {
+		t.Errorf("containerdPass(true) = %q, want %q — --all must reach the collector", got, passForced)
+	}
+}
+
+// TestPruneContainerd_DisabledIsUnchangedByAll: --all forces the eviction
+// POLICY open, it does not conjure a collector. A node without image GC must
+// report the same clear reason either way (the requested pass is appended for
+// diagnosis; the reason itself does not change).
+func TestPruneContainerd_DisabledIsUnchangedByAll(t *testing.T) {
+	p := &Pruner{}
+	for _, all := range []bool{false, true} {
+		res := p.pruneContainerd(context.Background(), all)
+		if res.Name != TargetContainerd {
+			t.Errorf("all=%v: Name = %q, want %q", all, res.Name, TargetContainerd)
+		}
+		if res.Err == nil {
+			t.Fatalf("all=%v: expected an error when image gc is disabled", all)
+		}
+		if !strings.Contains(res.Err.Error(), "image gc is disabled") {
+			t.Errorf("all=%v: err = %v, want it to name the disabled collector", all, res.Err)
+		}
+	}
+}
+
+// TestPrune_ContainerdTargetThreadsAll walks the PUBLIC entry point to prove
+// the flag survives the Prune -> pruneContainerd hop — the hop where it used
+// to be dropped silently, so `cache clear containerd --all` ran a watermark
+// pass, evicted nothing, and reported success.
+//
+// It asserts on the pass NAME carried in the disabled-collector error, which
+// is the only externally visible evidence of what the flag became. Asserting
+// only "one result came back" (what this test used to do) proves nothing: a
+// nil ImageGC returns at the disabled guard, so the previous version of this
+// test passed identically whether `all` was threaded or hard-coded to false.
+func TestPrune_ContainerdTargetThreadsAll(t *testing.T) {
+	p := &Pruner{}
+	for _, tc := range []struct {
+		all  bool
+		want collectPass
+	}{
+		{false, passWatermark},
+		{true, passForced},
+	} {
+		results := p.Prune(context.Background(), []string{TargetContainerd}, tc.all)
+		if len(results) != 1 || results[0].Name != TargetContainerd {
+			t.Fatalf("all=%v: results = %+v, want one containerd result", tc.all, results)
+		}
+		if results[0].Err == nil {
+			t.Fatalf("all=%v: expected the disabled-collector error", tc.all)
+		}
+		if got := results[0].Err.Error(); !strings.Contains(got, string(tc.want)) {
+			t.Errorf("all=%v: err = %q, want it to name pass %q — the flag did not survive Prune -> pruneContainerd", tc.all, got, tc.want)
+		}
+	}
+}
+
+// TestPrune_ContainerdTargetThreadsAll_DistinguishesThePasses guards the
+// assertion above against a degenerate pass naming (e.g. both constants
+// becoming the same string), which would make it pass vacuously.
+func TestPrune_ContainerdTargetThreadsAll_DistinguishesThePasses(t *testing.T) {
+	if passWatermark == passForced {
+		t.Fatal("passWatermark and passForced are identical; the threading assertions above cannot distinguish them")
+	}
+	if strings.Contains(string(passForced), string(passWatermark)) ||
+		strings.Contains(string(passWatermark), string(passForced)) {
+		t.Fatalf("pass names %q and %q are substrings of one another; a Contains-based assertion would be ambiguous", passWatermark, passForced)
+	}
+}