validate-image-cache: drive eviction through Store.EvictUnused + GC loop review follow-ups

cmd/atelet/imagegc.go

@@ -42,11 +42,11 @@ import (
 )
 
 var (
-	imageCacheGCPeriod = pflag.Duration("image-cache-gc-period", 5*time.Minute, "How often to run the image cache eviction pass. 0 disables eviction entirely.")
+	imageCacheGCPeriod = pflag.Duration("image-cache-gc-period", 5*time.Minute, "How often to run the image cache eviction pass. 0 disables the periodic pass (startup orphan recovery still runs at every atelet start).")
 	imageCacheHighPct  = pflag.Int("image-cache-high-percent", 85, "Cache-volume usage percentage above which eviction starts.")
 	imageCacheLowPct   = pflag.Int("image-cache-low-percent", 80, "Cache-volume usage percentage eviction frees down to. Must be lower than --image-cache-high-percent.")
 	imageCacheMaxBytes = pflag.Int64("image-cache-max-bytes", 0, "Absolute cap on the summed size of cached layers, evicted down to independently of the volume watermarks. 0 means no cap.")
-	imageCacheMinAge   = pflag.Duration("image-cache-min-age", 2*time.Minute, "Layers and image records younger than this are never evicted (protects images pulled but not yet mounted).")
+	imageCacheMinAge   = pflag.Duration("image-cache-min-age", 2*time.Minute, "Layers and image records younger than this are never evicted (protects images pulled but not yet mounted). Governs startup orphan recovery too, so it is live even with the periodic pass disabled.")
 	imageCacheGCDryRun = pflag.Bool("image-cache-gc-dry-run", false, "Compute and log eviction decisions without deleting anything.")
 )
 
@@ -60,6 +60,11 @@ const (
 )
 
 func validateImageCacheGCFlags() error {
+	if *imageCacheGCPeriod < 0 {
+		// A negative period would silently disable the loop (it fails the
+		// > 0 guard at the launch site, which also protects the ticker).
+		return fmt.Errorf("--image-cache-gc-period %v must be >= 0", *imageCacheGCPeriod)
+	}
 	if *imageCacheHighPct < 1 || *imageCacheHighPct > 100 {
 		return fmt.Errorf("--image-cache-high-percent %d out of range [1,100]", *imageCacheHighPct)
 	}
@@ -71,18 +76,26 @@ func validateImageCacheGCFlags() error {
 		// future), making just-pulled layers evictable.
 		return fmt.Errorf("--image-cache-min-age %v must be >= 0", *imageCacheMinAge)
 	}
-	// Outside BasePath the watermarks measure a different volume than
-	// actor state, so the pass would chase pressure the cache doesn't
-	// contribute to. Warn, don't fail: a separate cache volume is
-	// legitimate (recommended for IOPS) — it just wants its own numbers.
-	if !strings.HasPrefix(filepath.Clean(*imageCacheDir), ateompath.BasePath+string(os.PathSeparator)) {
+	if imageCacheDirOutsideBasePath(*imageCacheDir) {
 		slog.Warn("Image cache dir is outside the ateom base path; its volume watermarks are measured separately from actor state",
 			slog.String("image_cache_dir", *imageCacheDir),
 			slog.String("actors_dir", ateompath.ActorsDir))
 	}
 	return nil
 }
 
+// imageCacheDirOutsideBasePath reports whether the cache dir is outside
+// the ateom base path — the watermarks then measure a different volume
+// than actor state. Warn-worthy, not an error: a separate cache volume is
+// legitimate (recommended for IOPS).
+func imageCacheDirOutsideBasePath(dir string) bool {
+	abs, err := filepath.Abs(dir)
+	if err != nil {
+		abs = filepath.Clean(dir)
+	}
+	return !strings.HasPrefix(abs, ateompath.BasePath+string(os.PathSeparator))
+}
+
 // imageCacheGCTarget computes the bytes a pass should free: the larger
 // of the watermark shortfall (kubelet's formula — usage at highPct frees
 // down to lowPct) and the pool's overage past maxBytes, but never more
@@ -121,11 +134,18 @@ func imageCacheGCTarget(capacity, available uint64, cacheSize, maxBytes int64, h
 	return target
 }
 
+// gcStore is what the loop needs from *imagecache.Store — a seam so
+// runPass's skip and recovery paths are testable without a real pool.
+type gcStore interface {
+	CacheSize() (int64, error)
+	EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) (imagecache.EvictStats, error)
+}
+
 // imageCacheGC is the loop's state: configuration snapshotted from the
 // flags at construction (the pass logic never reads globals, so it is
 // testable without flag juggling) plus the shortfall-backoff counter.
 type imageCacheGC struct {
-	store    *imagecache.Store
+	store    gcStore
 	cacheDir string
 	period   time.Duration
 	highPct  int
@@ -151,6 +171,10 @@ func newImageCacheGC(store *imagecache.Store, cacheDir string) *imageCacheGC {
 // Run executes eviction passes on the configured period until ctx is
 // done. Passes are strictly serialized: a slow pass delays the next tick
 // rather than overlapping it.
+//
+// atelet passes its root context (the StartMetricsServer convention), so
+// the loop dies with the process; a pass cut off there leaves only .rm-*
+// dirs for the startup sweep. Cancellation is honored for tests.
 func (g *imageCacheGC) Run(ctx context.Context) {
 	// First pass immediately: a node booting under disk pressure must not
 	// wait a full period (startup recovery reclaims debris, not pressure).
@@ -221,7 +245,13 @@ func (g *imageCacheGC) runPass(ctx context.Context) {
 		slog.Bool("dry_run", g.dryRun),
 		slog.Duration("took", time.Since(tStart)),
 	}
-	outcome := classifyGCPass(err, target, stats.FreedBytes)
+	g.noteOutcome(ctx, classifyGCPass(err, target, stats.FreedBytes), err, attrs)
+}
+
+// noteOutcome logs one finished pass and advances the shortfall backoff.
+// The counter survives a gated pass (which says nothing about whether the
+// cache can meet a target) and resets when a target is met or absent.
+func (g *imageCacheGC) noteOutcome(ctx context.Context, outcome gcPassOutcome, err error, attrs []any) {
 	if outcome == gcPassSkipped {
 		slog.ErrorContext(ctx, "Image cache GC pass skipped", append(attrs, slog.Any("err", err))...)
 		return

cmd/atelet/imagegc_test.go

@@ -15,11 +15,17 @@
 package main
 
 import (
+	"bytes"
+	"context"
 	"errors"
 	"fmt"
+	"log/slog"
+	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/agent-substrate/substrate/internal/ateompath"
 	"github.com/agent-substrate/substrate/internal/imagecache"
 )
 
@@ -112,38 +118,224 @@ func TestImageCacheGCTarget(t *testing.T) {
 }
 
 func TestValidateImageCacheGCFlags(t *testing.T) {
-	setFlags := func(high, low int, minAge time.Duration) {
+	setFlags := func(period time.Duration, high, low int, minAge time.Duration) {
+		*imageCacheGCPeriod = period
 		*imageCacheHighPct = high
 		*imageCacheLowPct = low
 		*imageCacheMinAge = minAge
 	}
-	t.Cleanup(func() { setFlags(85, 80, 2*time.Minute) })
+	t.Cleanup(func() { setFlags(5*time.Minute, 85, 80, 2*time.Minute) })
 
 	cases := []struct {
 		name      string
+		period    time.Duration
 		high, low int
 		minAge    time.Duration
 		wantErr   bool
 	}{
-		{"defaults", 85, 80, 2 * time.Minute, false},
-		{"boundary high=100 low=0", 100, 0, 0, false},
-		{"high over 100", 101, 80, 0, true},
-		{"low equals high", 85, 85, 0, true},
-		{"low above high", 85, 90, 0, true},
-		{"negative low", 85, -1, 0, true},
-		{"negative min-age inverts the veto", 85, 80, -time.Second, true},
+		{"defaults", 5 * time.Minute, 85, 80, 2 * time.Minute, false},
+		{"boundary high=100 low=0", 5 * time.Minute, 100, 0, 0, false},
+		{"zero period disables the periodic pass", 0, 85, 80, 0, false},
+		{"negative period would silently disable the loop", -5 * time.Minute, 85, 80, 0, true},
+		{"high over 100", 5 * time.Minute, 101, 80, 0, true},
+		{"low equals high", 5 * time.Minute, 85, 85, 0, true},
+		{"low above high", 5 * time.Minute, 85, 90, 0, true},
+		{"negative low", 5 * time.Minute, 85, -1, 0, true},
+		{"negative min-age inverts the veto", 5 * time.Minute, 85, 80, -time.Second, true},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			setFlags(tc.high, tc.low, tc.minAge)
+			setFlags(tc.period, tc.high, tc.low, tc.minAge)
 			err := validateImageCacheGCFlags()
 			if (err != nil) != tc.wantErr {
-				t.Errorf("high=%d low=%d minAge=%v: err=%v, wantErr=%v", tc.high, tc.low, tc.minAge, err, tc.wantErr)
+				t.Errorf("period=%v high=%d low=%d minAge=%v: err=%v, wantErr=%v", tc.period, tc.high, tc.low, tc.minAge, err, tc.wantErr)
 			}
 		})
 	}
 }
 
+func TestImageCacheDirOutsideBasePath(t *testing.T) {
+	cases := []struct {
+		name string
+		dir  string
+		want bool
+	}{
+		{"inside", filepath.Join(ateompath.BasePath, "image-cache"), false},
+		{"inside with doubled separator", ateompath.BasePath + "//image-cache", false},
+		{"inside via dot-dot", ateompath.BasePath + "/x/../image-cache", false},
+		{"base path itself is not inside", ateompath.BasePath, true},
+		{"sibling with the base path as name prefix", ateompath.BasePath + "-other/image-cache", true},
+		{"outside", "/var/lib/elsewhere/image-cache", true},
+		{"dot-dot escaping the base path", ateompath.BasePath + "/../elsewhere/image-cache", true},
+		{"relative resolves against the cwd, not the base path", "image-cache", true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := imageCacheDirOutsideBasePath(tc.dir); got != tc.want {
+				t.Errorf("imageCacheDirOutsideBasePath(%q) = %v, want %v", tc.dir, got, tc.want)
+			}
+		})
+	}
+}
+
+type fakeGCStore struct {
+	size         int64
+	sizeErr      error
+	sizeCalls    int
+	evictCalls   int
+	gotTarget    int64
+	gotDryRun    bool
+	evictErr     error
+	stats        imagecache.EvictStats
+	panicOnEvict bool
+}
+
+func (f *fakeGCStore) CacheSize() (int64, error) {
+	f.sizeCalls++
+	return f.size, f.sizeErr
+}
+
+func (f *fakeGCStore) EvictUnused(_ context.Context, target int64, dryRun bool) (imagecache.EvictStats, error) {
+	f.evictCalls++
+	f.gotTarget = target
+	f.gotDryRun = dryRun
+	if f.panicOnEvict {
+		panic("boom")
+	}
+	return f.stats, f.evictErr
+}
+
+func TestRunPassSkipsOnStatfsFailure(t *testing.T) {
+	fake := &fakeGCStore{}
+	g := &imageCacheGC{store: fake, cacheDir: filepath.Join(t.TempDir(), "missing"), highPct: 85, lowPct: 80}
+	g.runPass(context.Background())
+	if fake.sizeCalls != 0 || fake.evictCalls != 0 {
+		t.Errorf("statfs failure: sizeCalls=%d evictCalls=%d, want 0/0", fake.sizeCalls, fake.evictCalls)
+	}
+}
+
+func TestRunPassSkipsOnCacheSizeFailure(t *testing.T) {
+	fake := &fakeGCStore{sizeErr: errors.New("unreadable size file")}
+	g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 85, lowPct: 80}
+	g.runPass(context.Background())
+	if fake.evictCalls != 0 {
+		t.Errorf("CacheSize failure: evictCalls=%d, want 0", fake.evictCalls)
+	}
+}
+
+func TestRunPassEvictsAndPassesDryRun(t *testing.T) {
+	// high=100 sidelines the watermark on any volume with >=1% free, so
+	// the max-bytes overage (99) is the target; a near-full host volume
+	// can lift it to the cacheSize cap, hence >= not ==.
+	fake := &fakeGCStore{size: 100, stats: imagecache.EvictStats{FreedBytes: 100}}
+	g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, maxBytes: 1, dryRun: true}
+	g.consecutiveShortfalls = 5 // a met target must reset it
+	g.runPass(context.Background())
+	if fake.evictCalls != 1 || fake.gotTarget < 99 || !fake.gotDryRun {
+		t.Errorf("evictCalls=%d target=%d dryRun=%v, want 1/>=99/true", fake.evictCalls, fake.gotTarget, fake.gotDryRun)
+	}
+	if g.consecutiveShortfalls != 0 {
+		t.Errorf("consecutiveShortfalls=%d after met target, want 0", g.consecutiveShortfalls)
+	}
+}
+
+func TestRunFirstPassIsImmediate(t *testing.T) {
+	// A cancelled context and an hour-long period: the single call can
+	// only be the immediate first pass, never a tick.
+	fake := &fakeGCStore{}
+	g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, period: time.Hour}
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	g.Run(ctx)
+	if fake.evictCalls != 1 {
+		t.Errorf("evictCalls=%d, want exactly 1 (the immediate first pass)", fake.evictCalls)
+	}
+}
+
+func TestRunTicks(t *testing.T) {
+	fake := &fakeGCStore{}
+	g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, period: 10 * time.Millisecond}
+	ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond)
+	defer cancel()
+	g.Run(ctx)
+	if fake.evictCalls < 2 {
+		t.Errorf("evictCalls=%d, want >=2 (first pass plus at least one tick)", fake.evictCalls)
+	}
+}
+
+func TestRunPassRecoversPanic(t *testing.T) {
+	g := &imageCacheGC{store: &fakeGCStore{panicOnEvict: true}, cacheDir: t.TempDir(), highPct: 100, lowPct: 0}
+	g.runPass(context.Background()) // must not propagate the panic
+}
+
+// TestNoteOutcomeShortfallBackoff drives the shortfall cadence end to end:
+// warn on the first shortfallWarnLimit consecutive shortfalls, then only
+// every shortfallReminderEvery-th, streak preserved across a gated pass,
+// reset (re-arming the warnings) on a met or absent target.
+func TestNoteOutcomeShortfallBackoff(t *testing.T) {
+	var buf bytes.Buffer
+	prev := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil)))
+	t.Cleanup(func() { slog.SetDefault(prev) })
+
+	ctx := context.Background()
+	g := &imageCacheGC{}
+	logCount := func(msg string) int { return strings.Count(buf.String(), msg) }
+
+	for range shortfallWarnLimit {
+		g.noteOutcome(ctx, gcPassShortfall, nil, nil)
+	}
+	if got := logCount("could not reach target"); got != shortfallWarnLimit {
+		t.Errorf("initial warns = %d, want %d", got, shortfallWarnLimit)
+	}
+
+	buf.Reset()
+	for g.consecutiveShortfalls < 2*shortfallReminderEvery {
+		g.noteOutcome(ctx, gcPassShortfall, nil, nil)
+	}
+	if got := logCount("still short of target"); got != 2 {
+		t.Errorf("reminders through streak %d = %d, want 2", g.consecutiveShortfalls, got)
+	}
+	if got := logCount("could not reach target"); got != 0 {
+		t.Errorf("warns past the limit = %d, want 0", got)
+	}
+
+	streak := g.consecutiveShortfalls
+	buf.Reset()
+	g.noteOutcome(ctx, gcPassSkipped, errors.New("gated"), nil)
+	if g.consecutiveShortfalls != streak {
+		t.Errorf("streak after gated pass = %d, want %d (preserved)", g.consecutiveShortfalls, streak)
+	}
+	if got := logCount("Image cache GC pass skipped"); got != 1 {
+		t.Errorf("skip logs = %d, want 1", got)
+	}
+
+	buf.Reset()
+	g.noteOutcome(ctx, gcPassComplete, errors.New("one dir failed"), nil)
+	if g.consecutiveShortfalls != 0 {
+		t.Errorf("streak after complete pass = %d, want 0", g.consecutiveShortfalls)
+	}
+	if got := logCount("pass complete"); got != 1 {
+		t.Errorf("complete logs = %d, want 1", got)
+	}
+	if got := logCount("finished with errors"); got != 1 {
+		t.Errorf("per-item error warns = %d, want 1", got)
+	}
+
+	buf.Reset()
+	g.noteOutcome(ctx, gcPassShortfall, nil, nil)
+	if got := logCount("could not reach target"); got != 1 {
+		t.Errorf("warns after reset = %d, want 1 (re-armed)", got)
+	}
+
+	g.consecutiveShortfalls = shortfallWarnLimit + 1
+	buf.Reset()
+	g.noteOutcome(ctx, gcPassQuiet, nil, nil)
+	if g.consecutiveShortfalls != 0 || buf.Len() != 0 {
+		t.Errorf("quiet pass: streak=%d buf=%q, want silent reset", g.consecutiveShortfalls, buf.String())
+	}
+}
+
 func TestClassifyGCPass(t *testing.T) {
 	gated := fmt.Errorf("pass gated: %w", imagecache.ErrIncompleteEnumeration)
 	perItem := errors.New("while removing retired layer: permission denied")

internal/imagecache/README.md

@@ -167,7 +167,8 @@ it — called from the checkpoint cleanup path in ateom-gvisor and
 ## Garbage collection
 
 `gc.go` holds the eviction engine; atelet drives it as a periodic pass
-(`--image-cache-gc-period`, default 5m; `0` disables it). Each tick
+(`--image-cache-gc-period`, default 5m; `0` disables the periodic pass,
+but startup orphan recovery still runs at every atelet start). Each tick
 measures the cache volume with `statfs` and the pool's own size from the
 per-layer `size` files, then computes a byte target: free down to
 `--image-cache-low-percent` when volume usage reaches
@@ -178,6 +179,18 @@ to fix disk pressure it didn't cause. `--image-cache-gc-dry-run`
 computes and logs every decision while mutating nothing: the
 recommended way to soak the policy on a live fleet.
 
+Two caveats on those numbers. The cap is the pool's *total* size, not
+the evictable subset (rooted and fresh layers can never be freed), so
+under **sustained** foreign pressure the target stays unreachable and
+every pass evicts everything unrooted and older than min-age — hit rate
+goes to zero until the pressure clears. The "could not reach target"
+WARNs are the signal; if this bites in practice, a retention floor
+(never evict below N bytes) is the intended extension. And usage is
+computed against the volume's raw capacity — kubelet's formula, so
+operator intuition transfers — which counts ext4's ~5% reserved blocks
+as used: eviction starts about five points below the configured
+percentage as `df` reports it.
+
 **One pass** (`Store.EvictUnused(ctx, targetBytes, dryRun)`):
 
 1. **Root set** (`Store.InUse`): scan every bundle's