From a7657c76e6f01b14f23f4bf1497b13a1205437bb Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 4 Aug 2026 09:35:34 -0700 Subject: [PATCH 1/6] =?UTF-8?q?imagecache:=20eviction=20engine=20=E2=80=94?= =?UTF-8?q?=20root=20set,=20EvictUnused,=20startup=20orphan=20recovery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine that reclaims cache disk, built on the primitives from the size/last-use, spec-digest, record-first-pull, and layer-retirement PRs. Inert in production: nothing calls EvictUnused yet (the watermark GC loop and its flags come separately); the only behavior New gains is the startup orphan scan. - Store.InUse: scans bundle overlay specs under the actors dir (opt-in via WithActorsDir) for the root set — image digests, layer hexes, and exact layer-set signatures. The exact-set rule roots the multi-arch twin record and digestless (pre-ImageDigest) specs' records, without making every superset image unevictable. - Store.EvictUnused: LRU eviction of unprotected images and the layers their removal leaves unreferenced, with min-age vetoes, per-victim re-checks under hitMu held exclusive against the cache-hit touch, and a restore-on-keep commit protocol: if any layer of a deleted record must be kept (still referenced, rooted, fresh, or failed to retire), the record is rewritten from bytes captured at listing time so the kept layer is never stranded unreachable. Freed bytes are credited optimistically from recorded sizes; dry-run mutates nothing. - Store.RecoverOrphans, called once from New: reclaims layer dirs no record references. Runs only at startup, the one moment the scan is race-free (no pull in flight), and skips itself entirely when the record enumeration is incomplete — refcounts from partial data make referenced layers look like garbage. Failure never fails New. - Per-item log lines (root-set entries, per-layer keep/retire, skips) are Debug and gated on Enabled so suppressed passes don't pay for attr construction; pass summaries, evictions, and restores stay Info. - pull_gated_test.go: the slow-pull progress-touch test now asserts the freshened record vetoes a concurrent EvictUnused pass (deferred from the record-first-pull PR, which had no engine to call). Tests cover every veto, shared-layer survival, both restore-on-keep shapes (wedged-pull-fresh-layer and rooted-subset), orphan reclaimed at startup but ignored by the periodic pass, the enumeration completeness gate, dry-run purity, and an ensure-vs-evict race loop under -race. --- internal/imagecache/gc.go | 664 ++++++++++++++++++++++ internal/imagecache/gc_regression_test.go | 379 ++++++++++++ internal/imagecache/gc_test.go | 310 ++++++++++ internal/imagecache/imagecache.go | 54 +- internal/imagecache/pull_gated_test.go | 12 +- 5 files changed, 1412 insertions(+), 7 deletions(-) create mode 100644 internal/imagecache/gc.go create mode 100644 internal/imagecache/gc_regression_test.go create mode 100644 internal/imagecache/gc_test.go diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go new file mode 100644 index 000000000..df9d7d26c --- /dev/null +++ b/internal/imagecache/gc.go @@ -0,0 +1,664 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package imagecache + +// Garbage collection for the layer pool. +// +// The pool is a two-level DAG — image records reference layers by diffID — +// so liveness is plain refcounting, recomputed from disk on every pass; +// there is nothing to maintain between passes and nothing to rebuild after +// a restart. An image (and then any layer its removal leaves unreferenced) +// is evictable unless vetoed by, in order: +// +// 1. the root set — bundle overlay specs under the actors dir, the same +// authority that hands out mounts (atelet cannot see the ateoms' mount +// namespaces); +// 2. min-age — records and layers younger than minAge are never touched, +// covering the pull → spec-write → mount window; +// 3. per-victim re-checks against current disk state immediately before +// acting. +// +// The periodic pass reaches layers ONLY through records: pull writes the +// image record before unpacking (see Store.pull), so every layer is +// referenced — and thereby protected — before it can exist on disk. +// Unexplained layers can therefore only be crash debris, reclaimed once at +// startup by RecoverOrphans; there is no online whole-pool scan. +// +// Deletion is two-phase: the only steps that contend with the pull path +// are one os.Remove of a record and one rename of a layer dir to a ".rm-*" +// name inside the layer's singleflight (see retireLayer); the slow +// RemoveAll of multi-GB trees happens afterwards, on dirs nothing can +// reach by diffID. A crash in between leaves a ".rm-*" dir for the +// startup sweep. + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "sort" + "strings" + "time" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "golang.org/x/sync/errgroup" +) + +// --- root set --- + +// RootSet is the set of images and layers that eviction must not touch, +// recomputed from disk at the start of every pass. +type RootSet struct { + // ImageDigests are rooted image digest strings ("sha256:"). + ImageDigests map[string]bool + // LayerHexes are rooted layer diffID hexes (the layer dir base names), + // covering bundle specs written before ImageDigest existed and + // belt-and-suspenders for those written after. + LayerHexes map[string]bool + // LayerSets holds a signature per rooted bundle's *exact* layer set. + // A record whose layer set matches one is rooted too: that is the + // multi-arch twin (same image recorded under the index and the + // per-platform child digest, identical layers) and the digestless + // spec written before ImageDigest existed, whose record would otherwise + // be evicted while its layers survive — manufacturing an orphan the + // moment the bundle goes. + // + // Deliberately an exact match, not "every layer is rooted somewhere": + // the union of all rooted layers makes any image whose layers are a + // subset — e.g. the base image of a running actor's image — + // permanently unevictable, which quietly weakens --image-cache-max-bytes + // on nodes with heavy layer sharing. + LayerSets map[string]bool +} + +// layerSetSignature canonicalizes a set of layer hexes for comparison. +func layerSetSignature(hexes []string) string { + uniq := make([]string, 0, len(hexes)) + seen := make(map[string]bool, len(hexes)) + for _, h := range hexes { + if !seen[h] { + seen[h] = true + uniq = append(uniq, h) + } + } + sort.Strings(uniq) + return strings.Join(uniq, ",") +} + +// InUse scans the actors directory for bundle overlay specs and returns the +// images and layers referenced by actors currently placed on this node. +// Bundles exist exactly while an actor is running or mid-transition here — +// Run/Restore write the spec before any ateom is asked to mount, and a +// successful Checkpoint deletes the bundle after unmount — so this scan +// protects actively mounted images, derived from the same authority that +// hands out mounts. Unreadable specs root nothing (logged); leftover +// bundles from crashed actors conservatively over-pin until the next +// Run/Restore/Checkpoint wipes them. +func (s *Store) InUse() RootSet { + // Per-item lines are Debug and gated: on a full node this loop emits + // hundreds of them, and ungated slog calls build their attr args even + // when suppressed. + dbg := slog.Default().Enabled(context.Background(), slog.LevelDebug) + rs := RootSet{ImageDigests: map[string]bool{}, LayerHexes: map[string]bool{}, LayerSets: map[string]bool{}} + if s.actorsDir == "" { + return rs + } + actorEntries, err := os.ReadDir(s.actorsDir) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + slog.Warn("Failed to list actors dir for image-cache root set", slog.String("dir", s.actorsDir), slog.Any("err", err)) + } + return rs + } + for _, actor := range actorEntries { + if !actor.IsDir() { + continue + } + bundlesDir := filepath.Join(s.actorsDir, actor.Name(), "bundles") + bundles, err := os.ReadDir(bundlesDir) + if err != nil { + continue // no bundles: actor not placed / already torn down + } + for _, bundle := range bundles { + if !bundle.IsDir() { + continue + } + // WriteSpec renames its temp file into place, so a spec read here + // is always whole; a partial read would under-report an actor's + // layers and fail toward deleting them. + spec, err := ReadSpec(filepath.Join(bundlesDir, bundle.Name())) + if err != nil { + slog.Warn("Unreadable bundle overlay spec during image-cache root-set scan", + slog.String("bundle", filepath.Join(bundlesDir, bundle.Name())), slog.Any("err", err)) + continue + } + if spec == nil { + continue + } + if spec.ImageDigest != "" { + rs.ImageDigests[spec.ImageDigest] = true + if dbg { + slog.Debug("Image cache root-set: bundle roots image", + slog.String("bundle", filepath.Join(actor.Name(), "bundles", bundle.Name())), + slog.String("digest", spec.ImageDigest), + slog.Int("layers", len(spec.Layers))) + } + } else if len(spec.Layers) > 0 && dbg { + slog.Debug("Image cache root-set: digestless bundle roots layers only", + slog.String("bundle", filepath.Join(actor.Name(), "bundles", bundle.Name())), + slog.Int("layers", len(spec.Layers))) + } + hexes := make([]string, 0, len(spec.Layers)) + for _, layerDir := range spec.Layers { + hex := filepath.Base(layerDir) + rs.LayerHexes[hex] = true + hexes = append(hexes, hex) + } + if len(hexes) > 0 { + rs.LayerSets[layerSetSignature(hexes)] = true + } + } + } + return rs +} + +// --- eviction --- + +// EvictStats reports what an eviction pass did (or, dry-run, would do). +type EvictStats struct { + // FreedBytes is the sum of recorded sizes of retired layers. Optimistic: + // tar-stream sizes, credited at retire time; the caller's next statfs + // self-corrects. + FreedBytes int64 + // EvictedImages / EvictedLayers count deleted records and retired layer + // dirs. + EvictedImages, EvictedLayers int + // Candidates is the number of LRU-ordered eviction candidates after all + // listing-stage vetoes. + Candidates int + // RootedImages counts image records excluded because a bundle overlay + // spec roots them (the "actively placed" protection). + RootedImages int + // SkippedRooted / SkippedFresh count vetoes at listing time and during + // the pass (a veto can fire in either place; the double-check re-runs + // them against current disk state). + SkippedRooted, SkippedFresh int + // OrphanLayers counts layer dirs reclaimed by the startup orphan scan + // (RecoverOrphans): layers no record references, which the record-driven + // pass can never reach. Always zero for periodic passes — a live process + // cannot create orphans, since pull writes the record before unpacking. + // Bytes are included in FreedBytes. + OrphanLayers int +} + +type evictionCandidate struct { + digest v1.Hash + modTime time.Time + diffIDs []string // unique, in record order + raw []byte // record file bytes, for restoration if a layer must be kept +} + +// EvictUnused evicts least-recently-used unprotected images — and the +// layers their removal leaves unreferenced — until ~targetBytes has been +// freed or candidates run out. Passing math.MaxInt64 means "free +// everything eligible" (the urgent path); targetBytes <= 0 evicts nothing. +// With dryRun nothing is deleted or renamed; the stats report what the +// pass would have freed. +// +// Failed deletions are skipped, not fatal: the error return aggregates +// them, but the pass continues to the next candidate (each retries next +// pass). Concurrent passes are serialized. +func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) (EvictStats, error) { + var stats EvictStats + s.evictMu.Lock() + defer s.evictMu.Unlock() + + roots := s.InUse() + cutoff := time.Now().Add(-s.minAge) + + candidates, refcount, _, listErr := s.listEviction(roots, cutoff, &stats) + // listErr is deferred, not fatal: evict what was listed, report at the end. + stats.Candidates = len(candidates) + + slog.InfoContext(ctx, "Image cache eviction pass", + slog.Int64("target_bytes", targetBytes), + slog.Bool("dry_run", dryRun), + slog.Int("rooted_images", stats.RootedImages), + slog.Int("rooted_layers", len(roots.LayerHexes)), + slog.Int("candidates", len(candidates)), + slog.Duration("min_age", s.minAge)) + + var errs []error + if listErr != nil { + errs = append(errs, listErr) + } + dbg := slog.Default().Enabled(ctx, slog.LevelDebug) // see InUse + var retired []string // renamed-aside dirs awaiting RemoveAll + + for _, cand := range candidates { + if targetBytes <= 0 || stats.FreedBytes >= targetBytes { + break + } + if err := ctx.Err(); err != nil { + errs = append(errs, err) + break + } + + // Double-check every veto against *current* disk state: a pull + // or bundle placement may have happened since the list was built. + // Held exclusive against the cache-hit path (see hitMu) so a hit's + // last-use touch and this re-check cannot interleave. + skip, err := func() (skip string, err error) { + s.hitMu.Lock() + defer s.hitMu.Unlock() + fi, err := os.Stat(s.recordPath(cand.digest)) + if errors.Is(err, os.ErrNotExist) { + return "gone", nil // already gone (e.g. its multi-arch twin's pass) + } else if err != nil { + return "", err + } + if fi.ModTime().After(cutoff) { + return "fresh", nil // touched since listing: in use moments ago + } + if !dryRun { + if err := os.Remove(s.recordPath(cand.digest)); err != nil && !errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("while deleting image record %s: %w", cand.digest, err) + } + } + return "", nil + }() + if err != nil { + errs = append(errs, err) + continue + } + switch skip { + case "fresh": + if dbg { + slog.DebugContext(ctx, "Image cache eviction skipped image", + slog.String("digest", cand.digest.String()), + slog.String("reason", skip), + slog.Time("last_used", cand.modTime)) + } + stats.SkippedFresh++ + continue + case "gone": + continue + } + slog.InfoContext(ctx, "Image cache evicting image record", + slog.String("digest", cand.digest.String()), + slog.Time("last_used", cand.modTime), + slog.Int("layers", len(cand.diffIDs)), + slog.Bool("dry_run", dryRun)) + + // The record is gone (in dry-run: would be). Retire the layers its + // removal un-references — and track any that must be KEPT. A kept + // layer at refcount 0 with no record is unreachable by the runtime + // pass until the next restart, so if anything is kept the record is + // restored below and the image simply is not evicted this pass. + kept := false + for _, hex := range cand.diffIDs { + refcount[hex]-- + if refcount[hex] > 0 { + if dbg { + slog.DebugContext(ctx, "Image cache keeping layer: still referenced", + slog.String("diffid", hex), slog.Int("refcount", refcount[hex])) + } + continue + } + if roots.LayerHexes[hex] { + // Rooted by a bundle spec (the digestless-spec case). Without + // its record this layer would strand the moment that bundle + // goes away; keep the record with it. + if dbg { + slog.DebugContext(ctx, "Image cache keeping layer: rooted by a bundle spec", + slog.String("diffid", hex)) + } + stats.SkippedRooted++ + kept = true + continue + } + var size int64 + var retiredPath string + var st retireStatus + if dryRun { + size, st = s.dryRunRetire(hex, cutoff) + } else { + // Sized before retiring (retireLayer no longer reports it). + // Backfilling a size-file-less layer here can rewind a + // concurrent reuse-touch — bounded by retireLayer's veto and + // the pull path's post-unpack re-verify. + var rerr error + if size, rerr = s.layerSize(filepath.Join(s.layersDir(), hex)); rerr != nil { + size = 0 // unknown size: still evict, credit nothing + } + if retiredPath, st, rerr = s.retireLayer(hex, cutoff); rerr != nil { + errs = append(errs, rerr) + kept = true + continue + } + } + switch st { + case retireGone: + continue + case retireVetoed: + kept = true + continue + } + if dbg { + slog.DebugContext(ctx, "Image cache retiring layer", + slog.String("diffid", hex), + slog.Int64("size_bytes", size), + slog.Bool("dry_run", dryRun)) + } + if retiredPath != "" { + retired = append(retired, retiredPath) + } + stats.FreedBytes += size + stats.EvictedLayers++ + } + if kept { + // Put the reference back: rewrite the record so every kept layer + // stays reachable, and restore this candidate's refcounts so + // later victims in this pass do not under-count shared layers. + // Already-retired layers stay retired — a record with missing + // layers re-pulls only the gaps (stale-record-harmless). + if !dryRun { + if err := s.restoreRecord(cand.digest, cand.raw); err != nil { + errs = append(errs, fmt.Errorf("while restoring record %s after kept layer: %w", cand.digest, err)) + } else { + slog.InfoContext(ctx, "Image cache restored image record: some of its layers must be kept", + slog.String("digest", cand.digest.String())) + } + } + for _, hex := range cand.diffIDs { + refcount[hex]++ + } + continue + } + stats.EvictedImages++ + } + + // The slow half, outside every lock: the retired dirs are unreachable by + // diffid, so this contends with nothing. A crash before completion + // leaves ".rm-*" dirs for the startup sweep. + if len(retired) > 0 { + tRemove := time.Now() + g := new(errgroup.Group) + g.SetLimit(4) + for _, dir := range retired { + g.Go(func() error { + if err := RemoveAllWritable(dir); err != nil { + return fmt.Errorf("while removing retired layer %q: %w", dir, err) + } + return nil + }) + } + if err := g.Wait(); err != nil { + errs = append(errs, err) + } + slog.InfoContext(ctx, "Image cache removed retired layer dirs", + slog.Int("count", len(retired)), + slog.Duration("took", time.Since(tRemove))) + } + + return stats, errors.Join(errs...) +} + +// RecoverOrphans reclaims layer dirs that no image record references. It +// is called once, from New, before the store serves any request — the one +// moment the scan is structurally race-free: no pull can be in flight, so +// a layer without a record is definitionally garbage, not work in +// progress. (During normal operation orphans cannot arise: pull writes the +// record before unpacking, and eviction retires layers in the same pass +// that drops their records. What this scan reaps is crash debris — a +// crash between record-delete and layer-rename — and operator damage. +// A retireLayer failure while the process lives therefore leaks until the +// next restart: rare, logged, bounded, and the accepted trade for not +// running an fsck against a live pool every pass.) +// +// Conservative by construction: if the record enumeration is not complete +// (unreadable dir, undecodable record), the scan is skipped entirely and +// logged at ERROR — refcounts from partial data make referenced layers +// look like garbage. Bundle-spec roots and min-age still veto, covering +// actors running across the restart and any near-boundary mtimes. +func (s *Store) RecoverOrphans(ctx context.Context) (EvictStats, error) { + var stats EvictStats + s.evictMu.Lock() + defer s.evictMu.Unlock() + + roots := s.InUse() + cutoff := time.Now().Add(-s.minAge) + _, refcount, complete, listErr := s.listEviction(roots, cutoff, &stats) + if !complete { + slog.ErrorContext(ctx, "Image cache startup orphan scan skipped: image records could not be fully enumerated; orphaned layers (if any) will persist until the records are repaired", + slog.Any("err", listErr)) + return stats, listErr + } + + retired, errs := s.sweepOrphanLayers(ctx, roots, refcount, cutoff, false, &stats) + for _, dir := range retired { + if err := RemoveAllWritable(dir); err != nil { + errs = append(errs, fmt.Errorf("while removing retired orphan %q: %w", dir, err)) + } + } + if stats.OrphanLayers > 0 { + slog.InfoContext(ctx, "Image cache startup scan reclaimed orphan layers", + slog.Int("count", stats.OrphanLayers), + slog.Int64("freed_bytes", stats.FreedBytes)) + } + return stats, errors.Join(errs...) +} + +// sweepOrphanLayers retires layer dirs that no surviving image record +// references. Returns the renamed-aside paths for the caller's async +// removal batch, plus any per-layer errors (never fatal). +func (s *Store) sweepOrphanLayers(ctx context.Context, roots RootSet, refcount map[string]int, cutoff time.Time, dryRun bool, stats *EvictStats) ([]string, []error) { + entries, err := os.ReadDir(s.layersDir()) + if err != nil { + return nil, []error{fmt.Errorf("while listing layer pool for orphan sweep: %w", err)} + } + + dbg := slog.Default().Enabled(ctx, slog.LevelDebug) // see InUse + var retired []string + var errs []error + for _, e := range entries { + hex := e.Name() + // Dot-prefixed dirs are in-flight unpacks (".tmp-") or already + // retired (".rm-"); both are handled elsewhere. Anything that is + // not a well-formed digest was not put there by the store, so the + // sweep leaves it alone rather than deleting an operator's file (or + // panicking on a name too short to abbreviate). + if !e.IsDir() || strings.HasPrefix(hex, ".") || !isLayerDirName(hex) { + continue + } + if refcount[hex] > 0 { + continue // referenced by a surviving record + } + if roots.LayerHexes[hex] { + // A bundle spec references it directly — the digestless-spec + // case, where the record is gone but an actor is still using + // these layers. + continue + } + var size int64 + var retiredPath string + var st retireStatus + if dryRun { + size, st = s.dryRunRetire(hex, cutoff) + } else { + var rerr error + if size, rerr = s.layerSize(filepath.Join(s.layersDir(), hex)); rerr != nil { + size = 0 // unknown size: still evict, credit nothing + } + if retiredPath, st, rerr = s.retireLayer(hex, cutoff); rerr != nil { + errs = append(errs, rerr) + continue + } + } + if st != retireRetired { + continue // too young, or vanished under us + } + if dbg { + slog.DebugContext(ctx, "Image cache retiring orphan layer", + slog.String("diffid", hex), + slog.Int64("size_bytes", size), + slog.Bool("dry_run", dryRun)) + } + if retiredPath != "" { + retired = append(retired, retiredPath) + } + stats.FreedBytes += size + stats.OrphanLayers++ + } + return retired, errs +} + +// dryRunRetire reports what retireLayer would do, without renaming: +// the same pre-flight checks, plus the size credit. +func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) { + dir := filepath.Join(s.layersDir(), hex) + fi, err := os.Stat(dir) + if err != nil { + return 0, retireGone + } + if fi.ModTime().After(cutoff) { + return 0, retireVetoed + } + size, err := s.layerSize(dir) + if err != nil { + size = 0 + } + return size, retireRetired +} + +// listEviction builds the LRU-ordered candidate list and the layer +// refcounts over ALL image records (rooted and fresh ones included — their +// references are what keep shared layers alive), counting listing-stage +// vetoes into stats. +// +// The returned complete flag reports whether every record was read and +// decoded. Refcounts from a partial listing understate references, which +// is safe for the record-driven pass (it only ever skips work) but fatal +// for the orphan sweep, which would read "no references" as "garbage". +func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) (cands []evictionCandidate, refcount map[string]int, complete bool, err error) { + refcount = map[string]int{} + entries, err := os.ReadDir(s.manifestsDir()) + if err != nil { + return nil, refcount, false, fmt.Errorf("while listing manifest records: %w", err) + } + complete = true + var candidates []evictionCandidate + var errs []error + for _, e := range entries { + name := e.Name() + if strings.HasPrefix(name, ".") || !strings.HasSuffix(name, ".json") { + continue + } + hex := strings.TrimSuffix(name, ".json") + digest := v1.Hash{Algorithm: "sha256", Hex: hex} + + b, err := os.ReadFile(filepath.Join(s.manifestsDir(), name)) + if err != nil { + // This record's layers get no refcounts while the record itself + // survives, so its layers would look like orphans. + complete = false + errs = append(errs, fmt.Errorf("while reading image record %s: %w", digest, err)) + continue + } + var rec imageRecord + if err := json.Unmarshal(b, &rec); err != nil { + complete = false + // An undecodable record can't produce refcounts; deleting it is + // safe (layers it referenced become orphans, collected next pass) + // but do so only via the normal candidate path so vetoes apply. + errs = append(errs, fmt.Errorf("while decoding image record %s: %w", digest, err)) + } + + // Dedupe diffIDs per record (images may list a layer twice) so the + // decrement in EvictUnused stays symmetric with this count. + seen := map[string]bool{} + var unique []string + for _, d := range rec.DiffIDs { + diffID, err := v1.NewHash(d) + if err != nil { + continue + } + if !seen[diffID.Hex] { + seen[diffID.Hex] = true + unique = append(unique, diffID.Hex) + refcount[diffID.Hex]++ + } + } + + fi, err := e.Info() + if err != nil { + continue + } + // A record is rooted either by digest (a bundle spec naming it) or + // because every layer it lists is rooted. The latter covers the + // multi-arch twin — pull records an image under both the index + // and per-platform child digest, but a bundle spec carries only the + // requested one — and digestless (pre-ImageDigest) specs. Without it the + // twin is evicted and rewritten on every pull of a rooted image: + // harmless but pure churn, and it inflates the eviction counters. + if roots.ImageDigests[digest.String()] || (len(unique) > 0 && roots.LayerSets[layerSetSignature(unique)]) { + stats.RootedImages++ + continue + } + if fi.ModTime().After(cutoff) { + stats.SkippedFresh++ + continue + } + candidates = append(candidates, evictionCandidate{digest: digest, modTime: fi.ModTime(), diffIDs: unique, raw: b}) + } + + // LRU by last use, ties broken by name for determinism. + sort.Slice(candidates, func(i, j int) bool { + if candidates[i].modTime.Equal(candidates[j].modTime) { + return candidates[i].digest.Hex < candidates[j].digest.Hex + } + return candidates[i].modTime.Before(candidates[j].modTime) + }) + return candidates, refcount, complete, errors.Join(errs...) +} + +// restoreRecord atomically rewrites a record from the bytes captured at +// listing time. Used when eviction deleted a record but then had to keep +// one of its layers: without the record the kept layer is unreachable by +// the runtime pass. The rewrite bumps the record's mtime, so min-age keeps +// it off the next pass's candidate list — the retry happens once the kept +// layer is itself old enough to go. +func (s *Store) restoreRecord(digest v1.Hash, raw []byte) error { + path := s.recordPath(digest) + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return fmt.Errorf("while creating record temp file: %w", err) + } + defer os.Remove(tmp.Name()) // no-op once the rename succeeds + if _, err := tmp.Write(raw); err != nil { + tmp.Close() + return fmt.Errorf("while writing record: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("while closing record: %w", err) + } + if err := os.Rename(tmp.Name(), path); err != nil { + return fmt.Errorf("while moving record into place: %w", err) + } + return nil +} diff --git a/internal/imagecache/gc_regression_test.go b/internal/imagecache/gc_regression_test.go new file mode 100644 index 000000000..f900ef0c7 --- /dev/null +++ b/internal/imagecache/gc_regression_test.go @@ -0,0 +1,379 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package imagecache + +// Regression tests pinning the eviction engine's load-bearing +// dispositions: +// +// - orphans (layers no record references) are crash debris, reclaimed at +// startup and never by the periodic pass; +// - a mid-pull layer is protected by the pre-written record's refcount +// plus the record's progress-touched freshness; +// - a record whose deletion would strand a kept layer is restored; +// - a partial record enumeration skips the startup scan entirely rather +// than sweeping on bad refcounts. + +import ( + "archive/tar" + "context" + "io" + "math" + "os" + "path/filepath" + "strings" + "testing" + "time" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/tarball" +) + +// A layer left in the pool with no referencing image record must be +// reclaimable. That state is crash debris by definition (pull pre-writes +// its record; eviction retires layers in the pass that drops their +// records), so it is reclaimed by the STARTUP scan — and deliberately NOT +// by the periodic pass, which reaches layers only through records. +func TestOrphanLayerReclaimedAtStartup(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/orphan:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("o", 4096)}, + })) + + store := newTestStore(t) + img := mustEnsure(t, store, ref) + + // Crash-debris state: layer on disk, no record referencing it. + if err := os.Remove(store.recordPath(img.Digest)); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + // The periodic pass must NOT touch it: no online whole-pool scans. + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if _, err := os.Stat(img.LayerDirs[0]); err != nil { + t.Fatalf("periodic pass swept an orphan; that scan belongs to startup only: %v", err) + } + + // "Restart": reopening the store runs RecoverOrphans and reclaims it. + reopened, err := New(store.root) + if err != nil { + t.Fatalf("New (restart): %v", err) + } + if got := layerDirsOnDisk(t, reopened); len(got) != 0 { + t.Errorf("orphan layers survive startup recovery: %v", got) + } + if size, err := reopened.CacheSize(); err != nil || size != 0 { + t.Errorf("CacheSize() = %d, %v after startup recovery; orphan bytes still counted", size, err) + } +} + +// A digestless bundle spec (written before OverlaySpec.ImageDigest +// existed, i.e. an actor running across that upgrade) must not strand its +// layers. The exact-layer-set rooting rule keeps the RECORD alive while +// the bundle exists, so when the bundle goes the record and layers are +// evicted together through the ordinary path — no orphan is ever +// manufactured, and no sweep is needed. +func TestDigestlessSpecLayersReclaimedAfterBundleGone(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/upgrade:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("u", 4096)}, + })) + + actorsDir := t.TempDir() + store := newTestStore(t, WithActorsDir(actorsDir)) + img := mustEnsure(t, store, ref) + + // A bundle written before ImageDigest existed: layers only. + bundle := filepath.Join(actorsDir, "actor-old", "bundles", "main") + if err := os.MkdirAll(bundle, 0o700); err != nil { + t.Fatal(err) + } + if err := WriteSpec(bundle, &OverlaySpec{Layers: img.LayerDirs}); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + // Pass 1, while the actor runs: the exact-layer-set rule roots the + // record, so record AND layer both survive. + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if _, err := os.Stat(img.LayerDirs[0]); err != nil { + t.Fatalf("layer of a running (digestless) actor was evicted: %v", err) + } + if _, err := os.Stat(store.recordPath(img.Digest)); err != nil { + t.Fatalf("record of a running (digestless) actor was evicted — that would strand its layers: %v", err) + } + + // The actor finishes; atelet removes the bundle. + if err := os.RemoveAll(filepath.Join(actorsDir, "actor-old")); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if got := layerDirsOnDisk(t, store); len(got) != 0 { + t.Errorf("digestless-spec layers not reclaimed after bundle removal: %v", got) + } +} + +// Layers of an in-flight pull must survive eviction, structurally: pull +// writes the record before unpacking, so a mid-pull layer is held by the +// record's refcount, renewed by the per-layer progress touch for as long +// as the pull advances. The shape here: a fresh record protecting even an +// OLD layer (which may have been in the pool for months from another +// image). +func TestRecordFirstProtectsInFlightPull(t *testing.T) { + store := newTestStore(t) // default min-age: 2m + + diffID := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("5c", 32)} + pending := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("6d", 32)} + digest := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("7e", 32)} + dir := store.layerDir(diffID) + if err := os.MkdirAll(filepath.Join(dir, layerFSDirName), 0o700); err != nil { + t.Fatal(err) + } + rec := imageRecord{Version: 1, DiffIDs: []string{diffID.String(), pending.String()}} + if err := store.writeRecord(digest, rec); err != nil { + t.Fatal(err) + } + backdate(t, dir, 3*time.Hour) + + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if _, err := os.Stat(dir); err != nil { + t.Fatalf("eviction reclaimed a layer referenced by a fresh (in-flight) record: %v", err) + } + if _, err := os.Stat(store.recordPath(digest)); err != nil { + t.Fatalf("eviction removed a fresh (in-flight) record: %v", err) + } +} + +// The wedged-pull disposal, with the partial layer FRESH — the realistic +// state, since a wedged pull's landed layers are seconds old. The +// contract: the stale record may be selected, but because min-age vetoes +// the fresh layer, the record is RESTORED — nothing is stranded, and the +// whole unit becomes evictable together once the layer ages. +func TestWedgedPullFreshLayersNotStranded(t *testing.T) { + store := newTestStore(t) // default min-age: 2m + + diffID := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("5c", 32)} + digest := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("7e", 32)} + dir := store.layerDir(diffID) + if err := os.MkdirAll(filepath.Join(dir, layerFSDirName), 0o700); err != nil { + t.Fatal(err) + } + rec := imageRecord{Version: 1, DiffIDs: []string{diffID.String()}} + if err := store.writeRecord(digest, rec); err != nil { + t.Fatal(err) + } + // Wedged: the record has seen no progress touch for > min-age, but the + // layer itself landed moments ago. + backdate(t, store.recordPath(digest), 3*time.Hour) + + for i := 0; i < 4; i++ { + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused pass %d: %v", i, err) + } + } + if _, err := os.Stat(dir); err != nil { + t.Fatalf("fresh partial layer was evicted: %v", err) + } + // The layer must not be stranded: its record must still exist (restored + // after the min-age veto fired), keeping it reachable by the runtime + // pass. + if _, err := os.Stat(store.recordPath(digest)); err != nil { + t.Fatalf("record gone while its fresh layer survives: the layer is stranded until restart: %v", err) + } + + // Once the layer ages past min-age (and the restored record does too), + // the unit is evicted together — reclaimed, not leaked. + backdate(t, store.recordPath(digest), 3*time.Hour) + backdate(t, dir, 3*time.Hour) + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if _, err := os.Stat(store.recordPath(digest)); !os.IsNotExist(err) { + t.Errorf("aged wedged-pull record survived: %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("aged wedged-pull layer survived: %v", err) + } +} + +// The direct restore-on-keep shape: a record whose deletion would strand +// a kept layer must be restored. Here the keep is caused by a rooted layer +// from a digestless bundle spec whose layer set does NOT exactly match the +// record (so LayerSets does not root the record itself). +func TestEvictionRestoresRecordWhenLayerRooted(t *testing.T) { + actorsDir := t.TempDir() + store := newTestStore(t, WithActorsDir(actorsDir)) + + shared := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("1a", 32)} + private := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("2b", 32)} + digest := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("3c", 32)} + for _, d := range []v1.Hash{shared, private} { + if err := os.MkdirAll(filepath.Join(store.layerDir(d), layerFSDirName), 0o700); err != nil { + t.Fatal(err) + } + } + rec := imageRecord{Version: 1, DiffIDs: []string{shared.String(), private.String()}} + if err := store.writeRecord(digest, rec); err != nil { + t.Fatal(err) + } + // A digestless spec roots ONLY the shared layer (subset — LayerSets + // cannot root the record). + bundle := filepath.Join(actorsDir, "actor-x", "bundles", "main") + if err := os.MkdirAll(bundle, 0o700); err != nil { + t.Fatal(err) + } + if err := WriteSpec(bundle, &OverlaySpec{Layers: []string{store.layerDir(shared)}}); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + // The private layer may go; the rooted layer stays — and therefore the + // record must have been restored, or the rooted layer is stranded the + // moment the bundle disappears. + if _, err := os.Stat(store.layerDir(shared)); err != nil { + t.Fatalf("rooted layer evicted: %v", err) + } + if _, err := os.Stat(store.recordPath(digest)); err != nil { + t.Fatalf("record gone while its rooted layer survives (stranded once the bundle goes): %v", err) + } +} + +// Refcounts derived from a partial record enumeration must never drive +// orphan reclamation: the startup scan skips itself entirely +// (conservative, logged) when any record fails to read or decode — while +// New still succeeds, because a corrupt record must not keep atelet from +// serving actors. +func TestStartupOrphanScanSkippedWhenEnumerationIncomplete(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/enum:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("e", 2048)}, + })) + store := newTestStore(t) + img := mustEnsure(t, store, ref) + backdateStore(t, store, 3*time.Hour) + + // A genuine orphan AND a corrupt record: the orphan must be spared + // because the corrupt record makes the enumeration untrustworthy. + orphan := filepath.Join(store.layersDir(), strings.Repeat("aa", 32)) + if err := os.MkdirAll(filepath.Join(orphan, layerFSDirName), 0o700); err != nil { + t.Fatal(err) + } + backdate(t, orphan, 3*time.Hour) + if err := os.WriteFile(store.recordPath(v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("bb", 32)}), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + + reopened, err := New(store.root) + if err != nil { + t.Fatalf("New must survive a corrupt record (atelet must still serve): %v", err) + } + if _, err := os.Stat(orphan); err != nil { + t.Errorf("startup scan ran on an incomplete enumeration and swept a layer: %v", err) + } + // The intact image is untouched either way. + for _, d := range img.LayerDirs { + if _, err := os.Stat(d); err != nil { + t.Errorf("intact image layer swept: %v", err) + } + } + _ = reopened +} + +// A directory name that is not a layer digest must never be treated as a +// layer — not by the periodic pass, and not by the startup scan (which +// would otherwise also panic abbreviating a short name for rename-aside). +func TestNonLayerDirsIgnored(t *testing.T) { + store := newTestStore(t, WithMinAge(0)) + junk := filepath.Join(store.layersDir(), "notadigest") + if err := os.MkdirAll(junk, 0o700); err != nil { + t.Fatal(err) + } + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, false) + if err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if stats.OrphanLayers != 0 { + t.Errorf("periodic pass swept something: %+v", stats) + } + if _, err := New(store.root, WithMinAge(0)); err != nil { + t.Fatalf("New: %v", err) + } + if _, err := os.Stat(junk); err != nil { + t.Errorf("non-layer dir was removed: %v", err) + } +} + +// The record must exist BEFORE unpacking — the load-bearing ordering. A +// pull that fails partway therefore leaves a record (resumable progress +// that ages out via LRU), never unexplained layers. +func TestFailedPullLeavesResumableRecordNotOrphans(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/badlayer:latest" + good := layerFromEntries(t, []tarEntry{ + {name: "ok", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("g", 2048)}, + }) + // Valid blob, invalid tar: unpack fails after download succeeds. + bad, err := tarball.LayerFromOpener(func() (io.ReadCloser, error) { + return io.NopCloser(strings.NewReader("this is not a tar archive at all")), nil + }) + if err != nil { + t.Fatal(err) + } + pushImage(t, ref, v1.Config{}, good, bad) + + store := newTestStore(t) + _, err = store.EnsureImage(context.Background(), ref) + if err == nil { + t.Fatal("EnsureImage succeeded on an image with an untarrable layer") + } + + recs, err := os.ReadDir(store.manifestsDir()) + if err != nil { + t.Fatal(err) + } + if len(recs) == 0 { + t.Fatal("failed pull left no record: its landed layers are unexplained orphans") + } + // Whatever layers landed are referenced by that record — assert none is + // an orphan by running the startup scan and checking nothing is swept. + backdateStore(t, store, 3*time.Hour) + for _, r := range recs { + backdate(t, filepath.Join(store.manifestsDir(), r.Name()), 0) // keep records fresh + } + before := layerDirsOnDisk(t, store) + reopened, err := New(store.root) + if err != nil { + t.Fatalf("New: %v", err) + } + if after := layerDirsOnDisk(t, reopened); len(after) != len(before) { + t.Errorf("startup scan swept layers of a failed-but-recorded pull: %v -> %v", before, after) + } +} diff --git a/internal/imagecache/gc_test.go b/internal/imagecache/gc_test.go new file mode 100644 index 000000000..ef51cabc1 --- /dev/null +++ b/internal/imagecache/gc_test.go @@ -0,0 +1,310 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package imagecache + +import ( + "archive/tar" + "context" + "math" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + v1 "github.com/google/go-containerregistry/pkg/v1" +) + +// backdateStore ages every image record and layer dir so the min-age veto +// no longer applies, making eviction tests deterministic without sleeps. +func backdateStore(t *testing.T, s *Store, age time.Duration) { + t.Helper() + for _, dir := range []string{s.manifestsDir(), s.layersDir()} { + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("listing %q: %v", dir, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".") { + continue + } + backdate(t, filepath.Join(dir, e.Name()), age) + } + } +} + +func layerDirsOnDisk(t *testing.T, s *Store) []string { + t.Helper() + entries, err := os.ReadDir(s.layersDir()) + if err != nil { + t.Fatalf("listing layer pool: %v", err) + } + var out []string + for _, e := range entries { + if !strings.HasPrefix(e.Name(), ".") { + out = append(out, e.Name()) + } + } + return out +} + +func TestEvictUnusedMinAgeVeto(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/fresh:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: "hi"}, + })) + + store := newTestStore(t) // default minAge = 2m; everything is younger + mustEnsure(t, store, ref) + + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, false) + if err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if stats.EvictedImages != 0 || stats.EvictedLayers != 0 { + t.Errorf("evicted fresh content: %+v", stats) + } + if got := layerDirsOnDisk(t, store); len(got) == 0 { + t.Error("fresh layers were removed") + } +} + +func TestEvictUnusedLRUSharedLayersAndRepull(t *testing.T) { + _, host := newTestRegistry(t) + shared := layerFromEntries(t, []tarEntry{ + {name: "shared", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("s", 2048)}, + }) + onlyA := layerFromEntries(t, []tarEntry{ + {name: "a", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("a", 2048)}, + }) + onlyB := layerFromEntries(t, []tarEntry{ + {name: "b", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("b", 2048)}, + }) + refA := host + "/test/a:latest" + refB := host + "/test/b:latest" + pushImage(t, refA, v1.Config{}, shared, onlyA) + pushImage(t, refB, v1.Config{}, shared, onlyB) + + store := newTestStore(t) + imgA := mustEnsure(t, store, refA) + imgB := mustEnsure(t, store, refB) + if imgA.LayerDirs[0] != imgB.LayerDirs[0] { + t.Fatalf("shared layer not deduplicated: %q vs %q", imgA.LayerDirs[0], imgB.LayerDirs[0]) + } + + backdateStore(t, store, 3*time.Hour) + // Make A strictly older than B so LRU picks A first. + backdate(t, store.recordPath(imgA.Digest), 4*time.Hour) + + // A small target: only the LRU image (A) should go. Its private layer is + // deleted; the shared layer survives via B's reference. + stats, err := store.EvictUnused(context.Background(), 1, false) + if err != nil { + t.Fatalf("EvictUnused(1): %v", err) + } + if stats.EvictedImages != 1 { + t.Fatalf("evicted %d images, want 1 (stats %+v)", stats.EvictedImages, stats) + } + if stats.FreedBytes <= 0 { + t.Errorf("FreedBytes = %d, want > 0", stats.FreedBytes) + } + if _, err := os.Stat(store.recordPath(imgA.Digest)); !os.IsNotExist(err) { + t.Error("LRU image record A still present") + } + if _, err := os.Stat(store.recordPath(imgB.Digest)); err != nil { + t.Errorf("newer image record B missing: %v", err) + } + if _, err := os.Stat(imgA.LayerDirs[1]); !os.IsNotExist(err) { + t.Error("A's private layer not evicted") + } + if _, err := os.Stat(imgA.LayerDirs[0]); err != nil { + t.Errorf("shared layer evicted while B still references it: %v", err) + } + + // Free-everything pass: B goes too, and the pool empties. + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Fatalf("EvictUnused(max): %v", err) + } + if got := layerDirsOnDisk(t, store); len(got) != 0 { + t.Errorf("layers remain after free-everything pass: %v", got) + } + + // The registry is still up: an evicted image is simply a re-pull away. + imgA2 := mustEnsure(t, store, refA) + if _, err := os.Stat(filepath.Join(imgA2.LayerDirs[1], layerFSDirName, "a")); err != nil { + t.Errorf("re-pulled image content missing: %v", err) + } +} + +func TestEvictUnusedRootSet(t *testing.T) { + _, host := newTestRegistry(t) + refRooted := host + "/test/rooted:latest" + refLayerRooted := host + "/test/layer-rooted:latest" + refLoose := host + "/test/loose:latest" + for _, r := range []string{refRooted, refLayerRooted, refLoose} { + pushImage(t, r, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f-" + r[len(r)-8:], typeflag: tar.TypeReg, mode: 0o644, body: r}, + })) + } + + actorsDir := t.TempDir() + store := newTestStore(t, WithActorsDir(actorsDir)) + imgRooted := mustEnsure(t, store, refRooted) + imgLayerRooted := mustEnsure(t, store, refLayerRooted) + imgLoose := mustEnsure(t, store, refLoose) + + // A modern bundle spec (with digest) roots imgRooted entirely. + bundle1 := filepath.Join(actorsDir, "actor-1", "bundles", "main") + if err := os.MkdirAll(bundle1, 0o700); err != nil { + t.Fatal(err) + } + if err := WriteSpec(bundle1, &OverlaySpec{ImageDigest: imgRooted.Digest.String(), Layers: imgRooted.LayerDirs}); err != nil { + t.Fatal(err) + } + // A digestless (pre-ImageDigest) spec roots only imgLayerRooted's layers. + bundle2 := filepath.Join(actorsDir, "actor-2", "bundles", "main") + if err := os.MkdirAll(bundle2, 0o700); err != nil { + t.Fatal(err) + } + if err := WriteSpec(bundle2, &OverlaySpec{Layers: imgLayerRooted.LayerDirs}); err != nil { + t.Fatal(err) + } + + rs := store.InUse() + if !rs.ImageDigests[imgRooted.Digest.String()] { + t.Error("InUse missing digest-rooted image") + } + if !rs.LayerHexes[filepath.Base(imgLayerRooted.LayerDirs[0])] { + t.Error("InUse missing layer-rooted layer") + } + + backdateStore(t, store, 3*time.Hour) + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, false) + if err != nil { + t.Fatalf("EvictUnused: %v", err) + } + + // Rooted image: record and layers untouched. + if _, err := os.Stat(store.recordPath(imgRooted.Digest)); err != nil { + t.Errorf("digest-rooted image record evicted: %v", err) + } + if _, err := os.Stat(imgRooted.LayerDirs[0]); err != nil { + t.Errorf("digest-rooted image layer evicted: %v", err) + } + // Layer-rooted (old, digestless spec): the layers must stay, and so must + // the record — a record all of whose layers are rooted is itself treated + // as rooted. Without that rule the record is evicted while its layers + // survive, which manufactures an orphan the moment the bundle goes away + // (see TestDigestlessSpecLayersReclaimedAfterBundleGone) and churns the + // record on every pull. + if _, err := os.Stat(imgLayerRooted.LayerDirs[0]); err != nil { + t.Errorf("layer-rooted layer evicted: %v", err) + } + if _, err := os.Stat(store.recordPath(imgLayerRooted.Digest)); err != nil { + t.Errorf("record with all layers rooted was evicted: %v", err) + } + if stats.RootedImages < 2 { + t.Errorf("RootedImages = %d, want >= 2 (digest-rooted + all-layers-rooted), stats %+v", stats.RootedImages, stats) + } + // The loose image is fully evicted. + if _, err := os.Stat(store.recordPath(imgLoose.Digest)); !os.IsNotExist(err) { + t.Error("unrooted image record survived a free-everything pass") + } + if _, err := os.Stat(imgLoose.LayerDirs[0]); !os.IsNotExist(err) { + t.Error("unrooted layer survived a free-everything pass") + } +} + +func TestEvictUnusedDryRun(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/dryrun:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("d", 1024)}, + })) + + store := newTestStore(t) + img := mustEnsure(t, store, ref) + backdateStore(t, store, 3*time.Hour) + + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, true) + if err != nil { + t.Fatalf("EvictUnused(dry): %v", err) + } + if stats.EvictedImages != 1 || stats.EvictedLayers != 1 || stats.FreedBytes <= 0 { + t.Errorf("dry-run stats = %+v, want 1 image / 1 layer / >0 bytes", stats) + } + if _, err := os.Stat(store.recordPath(img.Digest)); err != nil { + t.Errorf("dry run deleted the record: %v", err) + } + if _, err := os.Stat(img.LayerDirs[0]); err != nil { + t.Errorf("dry run deleted the layer: %v", err) + } +} + +// TestConcurrentEnsureImageAndEvict races cache hits, re-pulls, and +// free-everything eviction passes. The invariant under test: whatever +// interleaving happens, EnsureImage never returns an Image whose layer +// dirs have been (or will be) retired — either its touch wins and the +// evictor skips, or the evictor wins and EnsureImage re-pulls fresh dirs. +// Run with -race. +func TestConcurrentEnsureImageAndEvict(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/race:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("r", 512)}, + })) + + store := newTestStore(t) + digestRef := host + "/test/race@" + mustEnsure(t, store, ref).Digest.String() + + for i := 0; i < 25; i++ { + backdateStore(t, store, 3*time.Hour) + + var wg sync.WaitGroup + var img *Image + var ensureErr error + wg.Add(2) + go func() { + defer wg.Done() + img, ensureErr = store.EnsureImage(context.Background(), digestRef) + }() + go func() { + defer wg.Done() + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err != nil { + t.Errorf("EvictUnused: %v", err) + } + }() + wg.Wait() + + if ensureErr != nil { + t.Fatalf("iteration %d: EnsureImage: %v", i, ensureErr) + } + // The returned image must reference live, complete layer dirs. Its + // record must be fresh (either the hit's touch or the re-pull wrote + // it), which is what protects it until an ateom would mount it. + for _, dir := range img.LayerDirs { + if _, err := os.Stat(filepath.Join(dir, layerFSDirName, "f")); err != nil { + t.Fatalf("iteration %d: returned layer dir unusable: %v", i, err) + } + } + if fi, err := os.Stat(store.recordPath(img.Digest)); err != nil { + t.Fatalf("iteration %d: record of returned image missing: %v", i, err) + } else if time.Since(fi.ModTime()) > time.Minute { + t.Fatalf("iteration %d: returned image's record is stale (mtime %v)", i, fi.ModTime()) + } + } +} diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go index e84983ea5..864d21add 100644 --- a/internal/imagecache/imagecache.go +++ b/internal/imagecache/imagecache.go @@ -84,6 +84,9 @@ const ( // across nodes. layerSizeFileName = "size" + // defaultMinAge is the default eviction minimum age (see WithMinAge). + defaultMinAge = 2 * time.Minute + // layerPullConcurrency bounds concurrent layer download+unpack streams per // image pull. Memory use is O(stream buffers) per slot, independent of // layer size. @@ -107,12 +110,30 @@ type Store struct { // nodes it validates for. platform *v1.Platform + // actorsDir is scanned by InUse for bundle overlay specs; empty disables + // the scan (the root set is then empty). + actorsDir string + + // minAge vetoes eviction of any layer or image record younger than this, + // covering the window between a pull (or cache-hit stat) and the bundle + // spec write / ateom mount that roots it. + minAge time.Duration + imageSF singleflight.Group layerSF singleflight.Group - // hitMu makes the hit path's record read + last-use touch atomic with - // respect to eviction, which will hold it exclusive around record - // removal. No exclusive holder yet, so effectively free. + // evictMu serializes EvictUnused passes (concurrent passes would fight + // over the same candidates for no benefit). + evictMu sync.Mutex + + // hitMu closes the last hit-vs-evict window: the cache-hit path holds it + // shared across its record read, layer stats, and last-use touch, and + // eviction holds it exclusive across each victim's final veto re-check + // and record removal. Either the hit's touch lands first (the re-check + // sees a fresh mtime and skips the image, layers included) or the + // removal lands first (the hit sees no record and falls into the pull + // path, which is fully serialized by the layer singleflight). Uncontended + // except during an eviction pass. hitMu sync.RWMutex } @@ -137,6 +158,20 @@ func WithPlatform(p v1.Platform) Option { return func(s *Store) { s.platform = &p } } +// WithActorsDir points the eviction root-set scan at the node's actors +// directory (the per-actor state dirs under ateompath.BasePath). Each +// //bundles//rootfs-overlay.json roots its +// image and layers against eviction. Empty disables the scan. +func WithActorsDir(dir string) Option { + return func(s *Store) { s.actorsDir = dir } +} + +// WithMinAge overrides the eviction minimum age (default 2m): layers and +// image records younger than this are never evicted. +func WithMinAge(d time.Duration) Option { + return func(s *Store) { s.minAge = d } +} + // Image describes one cached, ready-to-compose image. type Image struct { // Digest is the manifest digest the caller's ref resolved to (for a @@ -162,7 +197,7 @@ type imageRecord struct { // startup recovery: verifying the layout version and sweeping temp dirs left // by unpacks that were in flight when a previous atelet died. func New(root string, opts ...Option) (*Store, error) { - s := &Store{root: root} + s := &Store{root: root, minAge: defaultMinAge} for _, o := range opts { o(s) } @@ -192,6 +227,12 @@ func New(root string, opts ...Option) (*Store, error) { if err := s.sweepTempDirs(); err != nil { return nil, err } + // Startup-only orphan recovery (see RecoverOrphans for why it must not + // run during normal operation). Failure is logged inside, never fatal: + // a corrupt record must not keep atelet from serving actors. + if _, err := s.RecoverOrphans(context.Background()); err != nil { + slog.Warn("Image cache startup orphan recovery incomplete", slog.Any("err", err)) + } return s, nil } @@ -283,7 +324,10 @@ func (s *Store) EnsureImage(ctx context.Context, ref string) (*Image, error) { s.hitMu.RLock() img, err := s.cachedImage(digest) if err == nil && img != nil { - // Record last-use for eviction's LRU ordering. + // Record last-use for eviction's LRU ordering. Refreshing the mtime + // also renews the min-age veto, so an image in active use cannot age + // into eviction between this stat and the ateom's mount (see hitMu + // for why this ordering is airtight, not just probabilistic). s.touchRecord(digest) } s.hitMu.RUnlock() diff --git a/internal/imagecache/pull_gated_test.go b/internal/imagecache/pull_gated_test.go index 3d565029d..e605cd873 100644 --- a/internal/imagecache/pull_gated_test.go +++ b/internal/imagecache/pull_gated_test.go @@ -196,8 +196,8 @@ func TestPullRewritesRecordEvictedMidPull(t *testing.T) { // The per-layer progress touch keeps a slow pull's record fresh mid-flight: // freshness must come from layer completions, not only from the final -// rewrite. (The eviction pass asserting min-age against that freshness is -// covered with the eviction engine's tests.) +// rewrite — and that freshness must veto an eviction pass running while +// the pull is still in flight. func TestProgressTouchKeepsSlowPullRecordFresh(t *testing.T) { reg := newGatedRegistry(t) free, gated1, gated2 := gatedTestLayers(t) @@ -244,6 +244,14 @@ func TestProgressTouchKeepsSlowPullRecordFresh(t *testing.T) { return err == nil && time.Since(fi.ModTime()) < time.Hour }) + // The pull is still mid-flight: the freshened record must veto eviction. + if _, err := store.EvictUnused(context.Background(), 1<<62, false); err != nil { + t.Fatalf("EvictUnused: %v", err) + } + if _, err := os.Stat(recPath); err != nil { + t.Fatalf("freshly-touched mid-pull record evicted: %v", err) + } + // Unblock the last layer and drain the pull. release2() select { From 1b377197573703c8159fe98bb89133158249f31b Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 4 Aug 2026 10:11:05 -0700 Subject: [PATCH 2/6] imagecache: fail eviction toward retention when records can't be enumerated Review fixes, all in the error paths around unreadable records: - EvictUnused now gates on complete record enumeration, like the startup scan already did. Refcounts from a partial listing understate references - a layer shared with an unreadable record hits zero and is retired while that record still names it - so the doc claim that a partial listing "only ever skips work" was inverted. The pass skips entirely, logs at ERROR naming the records, and returns the error; every later pass retries. Skipping only layer retirement instead would strand the evicted records' layers as orphans until restart, so the whole pass fails toward retention. - An undecodable record is never an eviction candidate: deleting it would strand its (unknown) layers the same way. It stays in place, surfaced in the enumeration error for the operator. - Dry-run no longer writes size-file backfills: dryRunRetire sizes via a read-only variant of layerSize (split into recordedLayerSize + walkLayerSize). The affected population is exactly the pre-size-file layers a dry-run soak on upgraded nodes exists to observe. - restoreRecord documents the cost of its mtime bump: the record's true last-use is lost, accepted because preserving it would churn the same doomed candidate through delete-and-restore every pass. One test per finding: gated pass with an unreadable shared-layer record (root-gated on euid 0), undecodable record survival, and dry-run leaving no size file behind. --- internal/imagecache/gc.go | 48 ++++++++++----- internal/imagecache/gc_test.go | 108 +++++++++++++++++++++++++++++++++ internal/imagecache/sizes.go | 52 +++++++++++----- 3 files changed, 179 insertions(+), 29 deletions(-) diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go index df9d7d26c..fdd062be7 100644 --- a/internal/imagecache/gc.go +++ b/internal/imagecache/gc.go @@ -222,7 +222,10 @@ type evictionCandidate struct { // // Failed deletions are skipped, not fatal: the error return aggregates // them, but the pass continues to the next candidate (each retries next -// pass). Concurrent passes are serialized. +// pass). Concurrent passes are serialized. If the record enumeration is +// incomplete (an unreadable or undecodable record), the pass is skipped +// entirely: refcounts from partial data would retire layers the unread +// records still reference. func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) (EvictStats, error) { var stats EvictStats s.evictMu.Lock() @@ -231,8 +234,16 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) roots := s.InUse() cutoff := time.Now().Add(-s.minAge) - candidates, refcount, _, listErr := s.listEviction(roots, cutoff, &stats) - // listErr is deferred, not fatal: evict what was listed, report at the end. + candidates, refcount, complete, listErr := s.listEviction(roots, cutoff, &stats) + if !complete { + // A layer shared with an unread record would hit refcount zero and + // be retired while that record still names it. Fail the whole pass + // toward retention; the error names the records to repair or + // delete, and every later pass retries. + slog.ErrorContext(ctx, "Image cache eviction pass skipped: image records could not be fully enumerated", + slog.Any("err", listErr)) + return stats, listErr + } stats.Candidates = len(candidates) slog.InfoContext(ctx, "Image cache eviction pass", @@ -244,9 +255,6 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) slog.Duration("min_age", s.minAge)) var errs []error - if listErr != nil { - errs = append(errs, listErr) - } dbg := slog.Default().Enabled(ctx, slog.LevelDebug) // see InUse var retired []string // renamed-aside dirs awaiting RemoveAll @@ -529,7 +537,9 @@ func (s *Store) sweepOrphanLayers(ctx context.Context, roots RootSet, refcount m } // dryRunRetire reports what retireLayer would do, without renaming: -// the same pre-flight checks, plus the size credit. +// the same pre-flight checks, plus the size credit. Sizing is read-only — +// even the size-file backfill would break dry-run's mutate-nothing +// contract. func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) { dir := filepath.Join(s.layersDir(), hex) fi, err := os.Stat(dir) @@ -539,7 +549,7 @@ func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) if fi.ModTime().After(cutoff) { return 0, retireVetoed } - size, err := s.layerSize(dir) + size, err := s.layerSizeReadOnly(dir) if err != nil { size = 0 } @@ -552,9 +562,11 @@ func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) // vetoes into stats. // // The returned complete flag reports whether every record was read and -// decoded. Refcounts from a partial listing understate references, which -// is safe for the record-driven pass (it only ever skips work) but fatal -// for the orphan sweep, which would read "no references" as "garbage". +// decoded. Refcounts from a partial listing understate references — a +// layer shared with an unread record looks unreferenced, so it would be +// retired (record-driven pass) or swept (orphan scan) while that record +// still names it. Both callers therefore gate on the flag and do nothing +// with a partial listing. func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) (cands []evictionCandidate, refcount map[string]int, complete bool, err error) { refcount = map[string]int{} entries, err := os.ReadDir(s.manifestsDir()) @@ -582,11 +594,13 @@ func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) } var rec imageRecord if err := json.Unmarshal(b, &rec); err != nil { + // An undecodable record contributes no refcounts, so evicting it + // would strand its (unknown) layers as orphans until the next + // restart. Never a candidate: leave it in place and fail the + // enumeration — the error names the file for the operator. complete = false - // An undecodable record can't produce refcounts; deleting it is - // safe (layers it referenced become orphans, collected next pass) - // but do so only via the normal candidate path so vetoes apply. errs = append(errs, fmt.Errorf("while decoding image record %s: %w", digest, err)) + continue } // Dedupe diffIDs per record (images may list a layer twice) so the @@ -642,7 +656,11 @@ func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) // one of its layers: without the record the kept layer is unreachable by // the runtime pass. The rewrite bumps the record's mtime, so min-age keeps // it off the next pass's candidate list — the retry happens once the kept -// layer is itself old enough to go. +// layer is itself old enough to go. The cost is the record's true last-use: +// a cold restored image now sorts as freshly used and can outlive genuinely +// warmer images until it re-ages. Accepted — preserving the old mtime +// instead would make the same doomed candidate churn through +// delete-and-restore on every pass until its kept layer ages out. func (s *Store) restoreRecord(digest v1.Hash, raw []byte) error { path := s.recordPath(digest) tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") diff --git a/internal/imagecache/gc_test.go b/internal/imagecache/gc_test.go index ef51cabc1..0b15b8d8c 100644 --- a/internal/imagecache/gc_test.go +++ b/internal/imagecache/gc_test.go @@ -229,6 +229,114 @@ func TestEvictUnusedRootSet(t *testing.T) { } } +// An unreadable record must gate the whole pass: its refcounts are +// invisible, so a layer it shares with a readable candidate would hit +// zero and be retired while the unreadable record still names it. +func TestEvictUnusedSkipsPassOnUnreadableRecord(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("chmod 0000 does not block reads for root") + } + _, host := newTestRegistry(t) + shared := layerFromEntries(t, []tarEntry{ + {name: "shared", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("s", 2048)}, + }) + onlyA := layerFromEntries(t, []tarEntry{ + {name: "a", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("a", 2048)}, + }) + onlyB := layerFromEntries(t, []tarEntry{ + {name: "b", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("b", 2048)}, + }) + refA := host + "/test/unreadable-a:latest" + refB := host + "/test/unreadable-b:latest" + pushImage(t, refA, v1.Config{}, shared, onlyA) + pushImage(t, refB, v1.Config{}, shared, onlyB) + + store := newTestStore(t) + imgA := mustEnsure(t, store, refA) + imgB := mustEnsure(t, store, refB) + backdateStore(t, store, 3*time.Hour) + + if err := os.Chmod(store.recordPath(imgA.Digest), 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(store.recordPath(imgA.Digest), 0o600) }) + + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, false) + if err == nil { + t.Fatal("EvictUnused returned no error on an unreadable record") + } + if stats.EvictedImages != 0 || stats.EvictedLayers != 0 || stats.FreedBytes != 0 { + t.Errorf("gated pass still evicted: %+v", stats) + } + // B and the shared layer survive: evicting B on partial refcounts would + // have retired the shared layer out from under A. + if _, err := os.Stat(store.recordPath(imgB.Digest)); err != nil { + t.Errorf("record B evicted during a gated pass: %v", err) + } + if _, err := os.Stat(imgB.LayerDirs[0]); err != nil { + t.Errorf("shared layer retired while an unreadable record references it: %v", err) + } +} + +// An undecodable record contributes no refcounts, so evicting it would +// strand its layers as orphans until restart. It must survive the pass +// (which its presence gates entirely) and be surfaced in the error. +func TestEvictUnusedLeavesUndecodableRecord(t *testing.T) { + store := newTestStore(t) + layer := filepath.Join(store.layersDir(), strings.Repeat("cc", 32)) + if err := os.MkdirAll(filepath.Join(layer, layerFSDirName), 0o700); err != nil { + t.Fatal(err) + } + badPath := store.recordPath(v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("dd", 32)}) + if err := os.WriteFile(badPath, []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + if _, err := store.EvictUnused(context.Background(), math.MaxInt64, false); err == nil { + t.Fatal("EvictUnused returned no error on an undecodable record") + } + if _, err := os.Stat(badPath); err != nil { + t.Errorf("undecodable record was evicted (its unknown layers would be stranded): %v", err) + } + if _, err := os.Stat(layer); err != nil { + t.Errorf("layer removed during a gated pass: %v", err) + } +} + +// Dry-run must not write anything — including the size-file backfill for +// layers unpacked before sizes were recorded, the exact population a +// dry-run soak on upgraded nodes exists to observe. +func TestEvictUnusedDryRunDoesNotBackfillSizes(t *testing.T) { + _, host := newTestRegistry(t) + ref := host + "/test/nobackfill:latest" + pushImage(t, ref, v1.Config{}, layerFromEntries(t, []tarEntry{ + {name: "f", typeflag: tar.TypeReg, mode: 0o644, body: strings.Repeat("n", 1024)}, + })) + + store := newTestStore(t) + img := mustEnsure(t, store, ref) + sizePath := filepath.Join(img.LayerDirs[0], layerSizeFileName) + if err := os.Remove(sizePath); err != nil { + t.Fatal(err) + } + backdateStore(t, store, 3*time.Hour) + + stats, err := store.EvictUnused(context.Background(), math.MaxInt64, true) + if err != nil { + t.Fatalf("EvictUnused(dry): %v", err) + } + if stats.FreedBytes <= 0 { + t.Errorf("FreedBytes = %d, want > 0 (walked size despite missing size file)", stats.FreedBytes) + } + if _, err := os.Stat(sizePath); !os.IsNotExist(err) { + t.Error("dry run wrote a size file into the layer pool") + } + if _, err := os.Stat(img.LayerDirs[0]); err != nil { + t.Errorf("dry run deleted the layer: %v", err) + } +} + func TestEvictUnusedDryRun(t *testing.T) { _, host := newTestRegistry(t) ref := host + "/test/dryrun:latest" diff --git a/internal/imagecache/sizes.go b/internal/imagecache/sizes.go index bae433cda..04dcf74bb 100644 --- a/internal/imagecache/sizes.go +++ b/internal/imagecache/sizes.go @@ -58,17 +58,47 @@ func (s *Store) touchRecord(digest v1.Hash) { // layerSize returns the layer's recorded byte count, backfilling the size // file for layers unpacked before sizes were recorded. func (s *Store) layerSize(layerDir string) (int64, error) { + if n, ok, err := recordedLayerSize(layerDir); err != nil || ok { + return n, err + } + total := walkLayerSize(layerDir) + // Preserve the dir mtime (the eviction age signal) across the write. + if fi, statErr := os.Stat(layerDir); statErr == nil { + if err := os.WriteFile(filepath.Join(layerDir, layerSizeFileName), []byte(strconv.FormatInt(total, 10)+"\n"), 0o600); err == nil { + _ = os.Chtimes(layerDir, fi.ModTime(), fi.ModTime()) + } + } + return total, nil +} + +// layerSizeReadOnly is layerSize without the backfill write, for dry-run +// eviction: "dry run mutates nothing" includes derived metadata. +func (s *Store) layerSizeReadOnly(layerDir string) (int64, error) { + if n, ok, err := recordedLayerSize(layerDir); err != nil || ok { + return n, err + } + return walkLayerSize(layerDir), nil +} + +// recordedLayerSize reads the layer's size file. ok is false when the file +// is absent or corrupt (the caller falls back to walking the tree). +func recordedLayerSize(layerDir string) (n int64, ok bool, err error) { b, err := os.ReadFile(filepath.Join(layerDir, layerSizeFileName)) - if err == nil { - n, perr := strconv.ParseInt(strings.TrimSpace(string(b)), 10, 64) - if perr == nil { - return n, nil + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return 0, false, nil } - // Corrupt size file: fall through to backfill. - } else if !errors.Is(err, os.ErrNotExist) { - return 0, fmt.Errorf("while reading layer size: %w", err) + return 0, false, fmt.Errorf("while reading layer size: %w", err) + } + n, perr := strconv.ParseInt(strings.TrimSpace(string(b)), 10, 64) + if perr != nil { + return 0, false, nil } + return n, true, nil +} +// walkLayerSize sums regular-file sizes under the layer's fs tree. +func walkLayerSize(layerDir string) int64 { var total int64 fsRoot := filepath.Join(layerDir, layerFSDirName) // The callback swallows every error (skip-and-under-count contract), @@ -90,13 +120,7 @@ func (s *Store) layerSize(layerDir string) (int64, error) { } return nil }) - // Preserve the dir mtime (the eviction age signal) across the write. - if fi, statErr := os.Stat(layerDir); statErr == nil { - if err := os.WriteFile(filepath.Join(layerDir, layerSizeFileName), []byte(strconv.FormatInt(total, 10)+"\n"), 0o600); err == nil { - _ = os.Chtimes(layerDir, fi.ModTime(), fi.ModTime()) - } - } - return total, nil + return total } // CacheSize returns the sum of recorded sizes of every layer in the pool From 752fddd378d408c3f872d6b1274b8548533bfd08 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 4 Aug 2026 10:28:52 -0700 Subject: [PATCH 3/6] imagecache: replace the regression-test file header with a factual one The bulleted summary duplicated the per-test comments; sibling test files carry at most a short note on what distinguishes the file. --- internal/imagecache/gc_regression_test.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/internal/imagecache/gc_regression_test.go b/internal/imagecache/gc_regression_test.go index f900ef0c7..1107137e7 100644 --- a/internal/imagecache/gc_regression_test.go +++ b/internal/imagecache/gc_regression_test.go @@ -14,16 +14,10 @@ package imagecache -// Regression tests pinning the eviction engine's load-bearing -// dispositions: -// -// - orphans (layers no record references) are crash debris, reclaimed at -// startup and never by the periodic pass; -// - a mid-pull layer is protected by the pre-written record's refcount -// plus the record's progress-touched freshness; -// - a record whose deletion would strand a kept layer is restored; -// - a partial record enumeration skips the startup scan entirely rather -// than sweeping on bad refcounts. +// Eviction tests for damaged and in-between pool states — crash debris, +// interrupted or wedged pulls, unreadable records — where the required +// behavior is failing toward retention. Mainline eviction behavior is +// covered in gc_test.go. import ( "archive/tar" From c9797fb1f557ec1ff746bef9b9842df99078ba2d Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 4 Aug 2026 11:02:53 -0700 Subject: [PATCH 4/6] imagecache: tighten engine doc comments; extract helpers from EvictUnused --- internal/imagecache/gc.go | 428 +++++++++++++++++++------------------- 1 file changed, 209 insertions(+), 219 deletions(-) diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go index fdd062be7..4de689b97 100644 --- a/internal/imagecache/gc.go +++ b/internal/imagecache/gc.go @@ -71,18 +71,12 @@ type RootSet struct { // belt-and-suspenders for those written after. LayerHexes map[string]bool // LayerSets holds a signature per rooted bundle's *exact* layer set. - // A record whose layer set matches one is rooted too: that is the - // multi-arch twin (same image recorded under the index and the - // per-platform child digest, identical layers) and the digestless - // spec written before ImageDigest existed, whose record would otherwise - // be evicted while its layers survive — manufacturing an orphan the - // moment the bundle goes. - // - // Deliberately an exact match, not "every layer is rooted somewhere": - // the union of all rooted layers makes any image whose layers are a - // subset — e.g. the base image of a running actor's image — - // permanently unevictable, which quietly weakens --image-cache-max-bytes - // on nodes with heavy layer sharing. + // A record whose layer set matches one is rooted too: the multi-arch + // twin and the digestless pre-ImageDigest spec, whose record would + // otherwise be evicted while its layers survive — manufacturing an + // orphan when the bundle goes. Exact match on purpose: rooting every + // subset image (e.g. a running actor's base image) would quietly + // weaken --image-cache-max-bytes under heavy layer sharing. LayerSets map[string]bool } @@ -100,15 +94,13 @@ func layerSetSignature(hexes []string) string { return strings.Join(uniq, ",") } -// InUse scans the actors directory for bundle overlay specs and returns the -// images and layers referenced by actors currently placed on this node. -// Bundles exist exactly while an actor is running or mid-transition here — -// Run/Restore write the spec before any ateom is asked to mount, and a -// successful Checkpoint deletes the bundle after unmount — so this scan -// protects actively mounted images, derived from the same authority that -// hands out mounts. Unreadable specs root nothing (logged); leftover -// bundles from crashed actors conservatively over-pin until the next -// Run/Restore/Checkpoint wipes them. +// InUse scans the actors directory for bundle overlay specs and returns +// the images and layers referenced by actors placed on this node. Bundles +// exist exactly while an actor runs or transitions here (spec written +// before any mount, deleted after unmount), so the scan protects actively +// mounted images via the same authority that hands out mounts. Unreadable +// specs root nothing (logged); leftover bundles from crashed actors +// over-pin until wiped. func (s *Store) InUse() RootSet { // Per-item lines are Debug and gated: on a full node this loop emits // hundreds of them, and ungated slog calls build their attr args even @@ -150,40 +142,45 @@ func (s *Store) InUse() RootSet { if spec == nil { continue } - if spec.ImageDigest != "" { - rs.ImageDigests[spec.ImageDigest] = true - if dbg { - slog.Debug("Image cache root-set: bundle roots image", - slog.String("bundle", filepath.Join(actor.Name(), "bundles", bundle.Name())), - slog.String("digest", spec.ImageDigest), - slog.Int("layers", len(spec.Layers))) - } - } else if len(spec.Layers) > 0 && dbg { - slog.Debug("Image cache root-set: digestless bundle roots layers only", - slog.String("bundle", filepath.Join(actor.Name(), "bundles", bundle.Name())), - slog.Int("layers", len(spec.Layers))) - } - hexes := make([]string, 0, len(spec.Layers)) - for _, layerDir := range spec.Layers { - hex := filepath.Base(layerDir) - rs.LayerHexes[hex] = true - hexes = append(hexes, hex) - } - if len(hexes) > 0 { - rs.LayerSets[layerSetSignature(hexes)] = true - } + addSpecRoots(&rs, spec, filepath.Join(actor.Name(), "bundles", bundle.Name()), dbg) } } return rs } +// addSpecRoots roots one bundle spec's image digest, layers, and exact +// layer-set signature. +func addSpecRoots(rs *RootSet, spec *OverlaySpec, bundle string, dbg bool) { + if spec.ImageDigest != "" { + rs.ImageDigests[spec.ImageDigest] = true + if dbg { + slog.Debug("Image cache root-set: bundle roots image", + slog.String("bundle", bundle), + slog.String("digest", spec.ImageDigest), + slog.Int("layers", len(spec.Layers))) + } + } else if len(spec.Layers) > 0 && dbg { + slog.Debug("Image cache root-set: digestless bundle roots layers only", + slog.String("bundle", bundle), + slog.Int("layers", len(spec.Layers))) + } + hexes := make([]string, 0, len(spec.Layers)) + for _, layerDir := range spec.Layers { + hex := filepath.Base(layerDir) + rs.LayerHexes[hex] = true + hexes = append(hexes, hex) + } + if len(hexes) > 0 { + rs.LayerSets[layerSetSignature(hexes)] = true + } +} + // --- eviction --- // EvictStats reports what an eviction pass did (or, dry-run, would do). type EvictStats struct { - // FreedBytes is the sum of recorded sizes of retired layers. Optimistic: - // tar-stream sizes, credited at retire time; the caller's next statfs - // self-corrects. + // FreedBytes sums recorded sizes of retired layers. Optimistic + // (tar-stream sizes); the caller's next statfs self-corrects. FreedBytes int64 // EvictedImages / EvictedLayers count deleted records and retired layer // dirs. @@ -194,15 +191,12 @@ type EvictStats struct { // RootedImages counts image records excluded because a bundle overlay // spec roots them (the "actively placed" protection). RootedImages int - // SkippedRooted / SkippedFresh count vetoes at listing time and during - // the pass (a veto can fire in either place; the double-check re-runs - // them against current disk state). + // SkippedRooted / SkippedFresh count vetoes, whether fired at listing + // time or by the per-victim re-check during the pass. SkippedRooted, SkippedFresh int - // OrphanLayers counts layer dirs reclaimed by the startup orphan scan - // (RecoverOrphans): layers no record references, which the record-driven - // pass can never reach. Always zero for periodic passes — a live process - // cannot create orphans, since pull writes the record before unpacking. - // Bytes are included in FreedBytes. + // OrphanLayers counts layers reclaimed by the startup scan + // (RecoverOrphans) — always zero for periodic passes, which reach + // layers only through records. Bytes are included in FreedBytes. OrphanLayers int } @@ -214,18 +208,15 @@ type evictionCandidate struct { } // EvictUnused evicts least-recently-used unprotected images — and the -// layers their removal leaves unreferenced — until ~targetBytes has been -// freed or candidates run out. Passing math.MaxInt64 means "free -// everything eligible" (the urgent path); targetBytes <= 0 evicts nothing. -// With dryRun nothing is deleted or renamed; the stats report what the -// pass would have freed. +// layers their removal leaves unreferenced — until ~targetBytes is freed +// or candidates run out. math.MaxInt64 means "free everything eligible"; +// targetBytes <= 0 evicts nothing. With dryRun nothing is deleted or +// renamed. // -// Failed deletions are skipped, not fatal: the error return aggregates -// them, but the pass continues to the next candidate (each retries next -// pass). Concurrent passes are serialized. If the record enumeration is -// incomplete (an unreadable or undecodable record), the pass is skipped -// entirely: refcounts from partial data would retire layers the unread -// records still reference. +// Per-item failures are aggregated into the error, not fatal: the pass +// continues and each item retries next pass. Passes are serialized. An +// incomplete record enumeration skips the pass entirely — refcounts from +// partial data would retire layers the unread records still reference. func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) (EvictStats, error) { var stats EvictStats s.evictMu.Lock() @@ -267,29 +258,7 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) break } - // Double-check every veto against *current* disk state: a pull - // or bundle placement may have happened since the list was built. - // Held exclusive against the cache-hit path (see hitMu) so a hit's - // last-use touch and this re-check cannot interleave. - skip, err := func() (skip string, err error) { - s.hitMu.Lock() - defer s.hitMu.Unlock() - fi, err := os.Stat(s.recordPath(cand.digest)) - if errors.Is(err, os.ErrNotExist) { - return "gone", nil // already gone (e.g. its multi-arch twin's pass) - } else if err != nil { - return "", err - } - if fi.ModTime().After(cutoff) { - return "fresh", nil // touched since listing: in use moments ago - } - if !dryRun { - if err := os.Remove(s.recordPath(cand.digest)); err != nil && !errors.Is(err, os.ErrNotExist) { - return "", fmt.Errorf("while deleting image record %s: %w", cand.digest, err) - } - } - return "", nil - }() + skip, err := s.removeStaleRecord(cand, cutoff, dryRun) if err != nil { errs = append(errs, err) continue @@ -313,78 +282,14 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) slog.Int("layers", len(cand.diffIDs)), slog.Bool("dry_run", dryRun)) - // The record is gone (in dry-run: would be). Retire the layers its - // removal un-references — and track any that must be KEPT. A kept - // layer at refcount 0 with no record is unreachable by the runtime - // pass until the next restart, so if anything is kept the record is - // restored below and the image simply is not evicted this pass. - kept := false - for _, hex := range cand.diffIDs { - refcount[hex]-- - if refcount[hex] > 0 { - if dbg { - slog.DebugContext(ctx, "Image cache keeping layer: still referenced", - slog.String("diffid", hex), slog.Int("refcount", refcount[hex])) - } - continue - } - if roots.LayerHexes[hex] { - // Rooted by a bundle spec (the digestless-spec case). Without - // its record this layer would strand the moment that bundle - // goes away; keep the record with it. - if dbg { - slog.DebugContext(ctx, "Image cache keeping layer: rooted by a bundle spec", - slog.String("diffid", hex)) - } - stats.SkippedRooted++ - kept = true - continue - } - var size int64 - var retiredPath string - var st retireStatus - if dryRun { - size, st = s.dryRunRetire(hex, cutoff) - } else { - // Sized before retiring (retireLayer no longer reports it). - // Backfilling a size-file-less layer here can rewind a - // concurrent reuse-touch — bounded by retireLayer's veto and - // the pull path's post-unpack re-verify. - var rerr error - if size, rerr = s.layerSize(filepath.Join(s.layersDir(), hex)); rerr != nil { - size = 0 // unknown size: still evict, credit nothing - } - if retiredPath, st, rerr = s.retireLayer(hex, cutoff); rerr != nil { - errs = append(errs, rerr) - kept = true - continue - } - } - switch st { - case retireGone: - continue - case retireVetoed: - kept = true - continue - } - if dbg { - slog.DebugContext(ctx, "Image cache retiring layer", - slog.String("diffid", hex), - slog.Int64("size_bytes", size), - slog.Bool("dry_run", dryRun)) - } - if retiredPath != "" { - retired = append(retired, retiredPath) - } - stats.FreedBytes += size - stats.EvictedLayers++ - } + kept, candRetired, retireErrs := s.retireCandidateLayers(ctx, cand, refcount, roots, cutoff, dryRun, &stats) + errs = append(errs, retireErrs...) + retired = append(retired, candRetired...) if kept { - // Put the reference back: rewrite the record so every kept layer - // stays reachable, and restore this candidate's refcounts so - // later victims in this pass do not under-count shared layers. - // Already-retired layers stay retired — a record with missing - // layers re-pulls only the gaps (stale-record-harmless). + // Restore the record so kept layers stay reachable, and put the + // refcounts back so later victims don't under-count shared + // layers. Already-retired layers stay retired — the stale record + // re-pulls only the gaps. if !dryRun { if err := s.restoreRecord(cand.digest, cand.raw); err != nil { errs = append(errs, fmt.Errorf("while restoring record %s after kept layer: %w", cand.digest, err)) @@ -401,24 +306,9 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) stats.EvictedImages++ } - // The slow half, outside every lock: the retired dirs are unreachable by - // diffid, so this contends with nothing. A crash before completion - // leaves ".rm-*" dirs for the startup sweep. if len(retired) > 0 { tRemove := time.Now() - g := new(errgroup.Group) - g.SetLimit(4) - for _, dir := range retired { - g.Go(func() error { - if err := RemoveAllWritable(dir); err != nil { - return fmt.Errorf("while removing retired layer %q: %w", dir, err) - } - return nil - }) - } - if err := g.Wait(); err != nil { - errs = append(errs, err) - } + errs = append(errs, removeRetiredDirs(retired)...) slog.InfoContext(ctx, "Image cache removed retired layer dirs", slog.Int("count", len(retired)), slog.Duration("took", time.Since(tRemove))) @@ -427,23 +317,133 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) return stats, errors.Join(errs...) } -// RecoverOrphans reclaims layer dirs that no image record references. It -// is called once, from New, before the store serves any request — the one -// moment the scan is structurally race-free: no pull can be in flight, so -// a layer without a record is definitionally garbage, not work in -// progress. (During normal operation orphans cannot arise: pull writes the -// record before unpacking, and eviction retires layers in the same pass -// that drops their records. What this scan reaps is crash debris — a -// crash between record-delete and layer-rename — and operator damage. -// A retireLayer failure while the process lives therefore leaks until the -// next restart: rare, logged, bounded, and the accepted trade for not -// running an fsck against a live pool every pass.) +// removeStaleRecord deletes the candidate's record unless a re-check +// against current disk state vetoes it — a pull or cache hit may have +// landed since listing. Runs under hitMu held exclusive so a hit's +// last-use touch cannot interleave with the re-check. Returns "gone" or +// "fresh" when the candidate must be skipped, "" to proceed. +func (s *Store) removeStaleRecord(cand evictionCandidate, cutoff time.Time, dryRun bool) (skip string, err error) { + s.hitMu.Lock() + defer s.hitMu.Unlock() + fi, err := os.Stat(s.recordPath(cand.digest)) + if errors.Is(err, os.ErrNotExist) { + return "gone", nil // e.g. removed by its multi-arch twin's pass + } else if err != nil { + return "", err + } + if fi.ModTime().After(cutoff) { + return "fresh", nil // touched since listing: in use moments ago + } + if !dryRun { + if err := os.Remove(s.recordPath(cand.digest)); err != nil && !errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("while deleting image record %s: %w", cand.digest, err) + } + } + return "", nil +} + +// retireCandidateLayers retires the layers un-referenced by the +// candidate's removal, crediting stats, and reports kept=true if any +// layer must stay (still referenced, rooted, fresh, or failed to retire). +// The caller must then restore the record: a kept layer with no record is +// unreachable until the next restart. +func (s *Store) retireCandidateLayers(ctx context.Context, cand evictionCandidate, refcount map[string]int, roots RootSet, cutoff time.Time, dryRun bool, stats *EvictStats) (kept bool, retired []string, errs []error) { + dbg := slog.Default().Enabled(ctx, slog.LevelDebug) // see InUse + for _, hex := range cand.diffIDs { + refcount[hex]-- + if refcount[hex] > 0 { + if dbg { + slog.DebugContext(ctx, "Image cache keeping layer: still referenced", + slog.String("diffid", hex), slog.Int("refcount", refcount[hex])) + } + continue + } + if roots.LayerHexes[hex] { + // Rooted by a bundle spec (the digestless-spec case): without + // its record this layer would strand when that bundle goes. + if dbg { + slog.DebugContext(ctx, "Image cache keeping layer: rooted by a bundle spec", + slog.String("diffid", hex)) + } + stats.SkippedRooted++ + kept = true + continue + } + var size int64 + var retiredPath string + var st retireStatus + if dryRun { + size, st = s.dryRunRetire(hex, cutoff) + } else { + // Sized before retiring. Backfilling here can rewind a + // concurrent reuse-touch — bounded by retireLayer's veto and + // the pull path's post-unpack re-verify. + var rerr error + if size, rerr = s.layerSize(filepath.Join(s.layersDir(), hex)); rerr != nil { + size = 0 // unknown size: still evict, credit nothing + } + if retiredPath, st, rerr = s.retireLayer(hex, cutoff); rerr != nil { + errs = append(errs, rerr) + kept = true + continue + } + } + switch st { + case retireGone: + continue + case retireVetoed: + kept = true + continue + } + if dbg { + slog.DebugContext(ctx, "Image cache retiring layer", + slog.String("diffid", hex), + slog.Int64("size_bytes", size), + slog.Bool("dry_run", dryRun)) + } + if retiredPath != "" { + retired = append(retired, retiredPath) + } + stats.FreedBytes += size + stats.EvictedLayers++ + } + return kept, retired, errs +} + +// removeRetiredDirs RemoveAlls renamed-aside layer dirs — the slow half +// of two-phase deletion, run outside every lock: the dirs are unreachable +// by diffID, so this contends with nothing. A crash mid-removal leaves +// ".rm-*" dirs for the startup sweep. +func removeRetiredDirs(dirs []string) []error { + var errs []error + g := new(errgroup.Group) + g.SetLimit(4) + for _, dir := range dirs { + g.Go(func() error { + if err := RemoveAllWritable(dir); err != nil { + return fmt.Errorf("while removing retired layer %q: %w", dir, err) + } + return nil + }) + } + if err := g.Wait(); err != nil { + errs = append(errs, err) + } + return errs +} + +// RecoverOrphans reclaims layer dirs that no image record references. +// Called once from New, before the store serves — the one moment the +// scan is race-free: no pull is in flight, so a layer without a record +// is definitionally garbage. Orphans cannot arise in normal operation +// (pull writes the record first; eviction retires layers in the pass +// that drops their records): this reaps crash debris and operator +// damage, the accepted alternative to an fsck against a live pool every +// pass — mid-life debris leaks until the next restart, logged. // -// Conservative by construction: if the record enumeration is not complete -// (unreadable dir, undecodable record), the scan is skipped entirely and -// logged at ERROR — refcounts from partial data make referenced layers -// look like garbage. Bundle-spec roots and min-age still veto, covering -// actors running across the restart and any near-boundary mtimes. +// Skipped entirely (ERROR) when the record enumeration is incomplete: +// refcounts from partial data make referenced layers look like garbage. +// Bundle-spec roots and min-age still veto. func (s *Store) RecoverOrphans(ctx context.Context) (EvictStats, error) { var stats EvictStats s.evictMu.Lock() @@ -459,11 +459,7 @@ func (s *Store) RecoverOrphans(ctx context.Context) (EvictStats, error) { } retired, errs := s.sweepOrphanLayers(ctx, roots, refcount, cutoff, false, &stats) - for _, dir := range retired { - if err := RemoveAllWritable(dir); err != nil { - errs = append(errs, fmt.Errorf("while removing retired orphan %q: %w", dir, err)) - } - } + errs = append(errs, removeRetiredDirs(retired)...) if stats.OrphanLayers > 0 { slog.InfoContext(ctx, "Image cache startup scan reclaimed orphan layers", slog.Int("count", stats.OrphanLayers), @@ -536,10 +532,9 @@ func (s *Store) sweepOrphanLayers(ctx context.Context, roots RootSet, refcount m return retired, errs } -// dryRunRetire reports what retireLayer would do, without renaming: -// the same pre-flight checks, plus the size credit. Sizing is read-only — -// even the size-file backfill would break dry-run's mutate-nothing -// contract. +// dryRunRetire reports what retireLayer would do, without renaming. +// Sizing is read-only: even a size-file backfill would break dry-run's +// mutate-nothing contract. func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) { dir := filepath.Join(s.layersDir(), hex) fi, err := os.Stat(dir) @@ -557,16 +552,14 @@ func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) } // listEviction builds the LRU-ordered candidate list and the layer -// refcounts over ALL image records (rooted and fresh ones included — their -// references are what keep shared layers alive), counting listing-stage -// vetoes into stats. +// refcounts over ALL image records — rooted and fresh included, since +// their references are what keep shared layers alive — counting +// listing-stage vetoes into stats. // -// The returned complete flag reports whether every record was read and -// decoded. Refcounts from a partial listing understate references — a -// layer shared with an unread record looks unreferenced, so it would be -// retired (record-driven pass) or swept (orphan scan) while that record -// still names it. Both callers therefore gate on the flag and do nothing -// with a partial listing. +// complete reports whether every record was read and decoded. Refcounts +// from a partial listing understate references — a layer shared with an +// unread record looks unreferenced — so both callers gate on it and do +// nothing with a partial listing. func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) (cands []evictionCandidate, refcount map[string]int, complete bool, err error) { refcount = map[string]int{} entries, err := os.ReadDir(s.manifestsDir()) @@ -651,16 +644,13 @@ func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) return candidates, refcount, complete, errors.Join(errs...) } -// restoreRecord atomically rewrites a record from the bytes captured at -// listing time. Used when eviction deleted a record but then had to keep -// one of its layers: without the record the kept layer is unreachable by -// the runtime pass. The rewrite bumps the record's mtime, so min-age keeps -// it off the next pass's candidate list — the retry happens once the kept -// layer is itself old enough to go. The cost is the record's true last-use: -// a cold restored image now sorts as freshly used and can outlive genuinely -// warmer images until it re-ages. Accepted — preserving the old mtime -// instead would make the same doomed candidate churn through -// delete-and-restore on every pass until its kept layer ages out. +// restoreRecord atomically rewrites a record from bytes captured at +// listing time, after eviction deleted it but had to keep a layer — +// without the record the kept layer is unreachable until restart. The +// rewrite bumps the mtime, so min-age keeps the image off the next +// candidate list instead of churning delete-and-restore every pass; the +// cost is the true last-use, so a cold restored image sorts as fresh +// until it re-ages. func (s *Store) restoreRecord(digest v1.Hash, raw []byte) error { path := s.recordPath(digest) tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") From 70814e987ed16beaa864c109a85324d63fb1cc31 Mon Sep 17 00:00:00 2001 From: Ivy Date: Wed, 5 Aug 2026 13:26:33 -0700 Subject: [PATCH 5/6] imagecache: extract cachedImageHit so the hit path's lock is defer-scoped The hit block in EnsureImage released hitMu.RUnlock explicitly because a defer would have held the read lock across the pull path. Extracting the block into cachedImageHit makes lock scope equal function scope, so defer works - and a panic between lock and unlock can no longer leak a read lock. That leak was free before this PR (hitMu had no exclusive holder); now it would wedge removeStaleRecord, and with it every future eviction pass, in a process whose RPC layer survives panics. Mirrors removeStaleRecord on the write side: both parties to the hitMu contract are now named functions whose body is their critical section, and the field comment points at each. --- internal/imagecache/imagecache.go | 35 ++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go index 864d21add..449ece276 100644 --- a/internal/imagecache/imagecache.go +++ b/internal/imagecache/imagecache.go @@ -126,10 +126,10 @@ type Store struct { // over the same candidates for no benefit). evictMu sync.Mutex - // hitMu closes the last hit-vs-evict window: the cache-hit path holds it - // shared across its record read, layer stats, and last-use touch, and - // eviction holds it exclusive across each victim's final veto re-check - // and record removal. Either the hit's touch lands first (the re-check + // hitMu closes the last hit-vs-evict window: the cache-hit path + // (cachedImageHit) holds it shared across its record read, layer stats, + // and last-use touch, and eviction (removeStaleRecord) holds it + // exclusive across each victim's final veto re-check and record removal. Either the hit's touch lands first (the re-check // sees a fresh mtime and skips the image, layers included) or the // removal lands first (the hit sees no record and falls into the pull // path, which is fully serialized by the layer singleflight). Uncontended @@ -321,16 +321,7 @@ func (s *Store) EnsureImage(ctx context.Context, ref string) (*Image, error) { digest = desc.Digest } - s.hitMu.RLock() - img, err := s.cachedImage(digest) - if err == nil && img != nil { - // Record last-use for eviction's LRU ordering. Refreshing the mtime - // also renews the min-age veto, so an image in active use cannot age - // into eviction between this stat and the ateom's mount (see hitMu - // for why this ordering is airtight, not just probabilistic). - s.touchRecord(digest) - } - s.hitMu.RUnlock() + img, err := s.cachedImageHit(digest) if err != nil { return nil, err } @@ -353,6 +344,22 @@ func (s *Store) EnsureImage(ctx context.Context, ref string) (*Image, error) { return v.(*Image), nil } +// cachedImageHit is the hit side of the hitMu contract: it verifies the +// cached image and records last-use for eviction's LRU ordering, atomic +// with respect to eviction's record removal (removeStaleRecord holds +// hitMu exclusive). Refreshing the mtime also renews the min-age veto, so +// an image in active use cannot age into eviction between this stat and +// the ateom's mount. +func (s *Store) cachedImageHit(digest v1.Hash) (*Image, error) { + s.hitMu.RLock() + defer s.hitMu.RUnlock() + img, err := s.cachedImage(digest) + if err == nil && img != nil { + s.touchRecord(digest) + } + return img, err +} + // cachedImage returns the cached image for digest, or nil if the record or // any of its layer dirs is missing (in which case the caller re-pulls; only // the missing layers cost anything). From 34518e67e2f3a111c693027454f7578c3b6910aa Mon Sep 17 00:00:00 2001 From: Ivy Date: Wed, 5 Aug 2026 13:37:32 -0700 Subject: [PATCH 6/6] imagecache: fix hitMu comment rewrap; hoist dbg back to once per pass The hitMu field comment picked up a 123-column line when the function names were inserted; rather than reflowing, shrink it to the invariant - the two named functions (cachedImageHit, removeStaleRecord) now carry the mechanics, so the field comment restating them was duplication. retireCandidateLayers takes dbg as a parameter again instead of recomputing Enabled per candidate, restoring the once-per-pass hoist the extraction had quietly undone. --- internal/imagecache/gc.go | 5 ++--- internal/imagecache/imagecache.go | 12 ++++-------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go index 4de689b97..60ffda867 100644 --- a/internal/imagecache/gc.go +++ b/internal/imagecache/gc.go @@ -282,7 +282,7 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) slog.Int("layers", len(cand.diffIDs)), slog.Bool("dry_run", dryRun)) - kept, candRetired, retireErrs := s.retireCandidateLayers(ctx, cand, refcount, roots, cutoff, dryRun, &stats) + kept, candRetired, retireErrs := s.retireCandidateLayers(ctx, cand, refcount, roots, cutoff, dryRun, dbg, &stats) errs = append(errs, retireErrs...) retired = append(retired, candRetired...) if kept { @@ -347,8 +347,7 @@ func (s *Store) removeStaleRecord(cand evictionCandidate, cutoff time.Time, dryR // layer must stay (still referenced, rooted, fresh, or failed to retire). // The caller must then restore the record: a kept layer with no record is // unreachable until the next restart. -func (s *Store) retireCandidateLayers(ctx context.Context, cand evictionCandidate, refcount map[string]int, roots RootSet, cutoff time.Time, dryRun bool, stats *EvictStats) (kept bool, retired []string, errs []error) { - dbg := slog.Default().Enabled(ctx, slog.LevelDebug) // see InUse +func (s *Store) retireCandidateLayers(ctx context.Context, cand evictionCandidate, refcount map[string]int, roots RootSet, cutoff time.Time, dryRun, dbg bool, stats *EvictStats) (kept bool, retired []string, errs []error) { for _, hex := range cand.diffIDs { refcount[hex]-- if refcount[hex] > 0 { diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go index 449ece276..b5b6f0eab 100644 --- a/internal/imagecache/imagecache.go +++ b/internal/imagecache/imagecache.go @@ -126,14 +126,10 @@ type Store struct { // over the same candidates for no benefit). evictMu sync.Mutex - // hitMu closes the last hit-vs-evict window: the cache-hit path - // (cachedImageHit) holds it shared across its record read, layer stats, - // and last-use touch, and eviction (removeStaleRecord) holds it - // exclusive across each victim's final veto re-check and record removal. Either the hit's touch lands first (the re-check - // sees a fresh mtime and skips the image, layers included) or the - // removal lands first (the hit sees no record and falls into the pull - // path, which is fully serialized by the layer singleflight). Uncontended - // except during an eviction pass. + // hitMu closes the hit-vs-evict window: held shared by the hit path + // (cachedImageHit), exclusive by eviction's record removal + // (removeStaleRecord), so a hit's last-use touch and eviction's final + // re-check can never interleave. Uncontended except during a pass. hitMu sync.RWMutex }