From 0c07f1bca10d6270a77d086f781834e31177976f Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 1 Sep 2026 21:37:35 -0700 Subject: [PATCH 1/2] fix(buildkit): heal + retry builds that hit a missing cache-record lease; grace-window the dead-record sweep (#193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #193 failure — failed to solve: lease "4igb...": not found — is the lease half of the two-databases-one-store corruption the dangling-snapshot healer already covers: a BuildKit cache record survives in the bbolt metadata while the containerd lease it owns (same 25-char base36 ID) is gone. Nothing on the record-load path checks the lease, so the stale record keeps being served from cache and detonates only when a build cache-hits it and attaches a resource to the missing lease (a cached FROM in buildkit's source/containerimage/pull.go, or setBlob on export), failing the job with the raw containerd error. The desync creators are the out-of-band classes heal.go already documents (manual ctr lease removal, a restored data dir, a crash between a lease deletion and the matching metadata clear); no ephemerd sweep deletes leases in the shared namespace — the sweeps only remove image records, which cannot take a lease with them. Fix, same principle as the snapshot healer: recognise the signature (narrowly — only a quoted 25-char record-lease ID, so BuildKit's temporary "-" leases never trigger a cache wipe), run the existing prune -> rebuild escalation ladder, and retry the build once. The cheap rung genuinely repairs it: the failed solve released its refs, PruneAll collects the stale record (its remove tolerates the already-missing lease) and the retry re-pulls a clean copy — which is why the incident's plain rerun passed. Also close a TOCTOU in the dead-build-record sweep while in there: the live set is a container listing captured before the record listing, so a job that starts and exports a build between the two would have its records judged dead and deleted mid-job. Records written within a 15-minute grace are now exempt (an unknown timestamp fails safe), same never-collect-a-live-owner rule as runtime.ReapDeadContainers. --- pkg/buildkit/heal.go | 95 +++++++++++++++++- pkg/buildkit/heal_test.go | 162 ++++++++++++++++++++++++++++++ pkg/dind/buildheal.go | 69 +++++++++++-- pkg/dind/buildkit_build.go | 11 +- pkg/dind/buildkit_cleanup.go | 60 +++++++++-- pkg/dind/buildkit_cleanup_test.go | 82 ++++++++++----- 6 files changed, 426 insertions(+), 53 deletions(-) diff --git a/pkg/buildkit/heal.go b/pkg/buildkit/heal.go index cb9b087..5adb35b 100644 --- a/pkg/buildkit/heal.go +++ b/pkg/buildkit/heal.go @@ -137,6 +137,72 @@ func DanglingSnapshotFromError(err error) (DanglingSnapshot, bool) { return ParseDanglingSnapshot(err.Error()) } +// danglingLeaseRe matches containerd's lease-not-found message as it surfaces +// through a BuildKit solve (#193): +// +// lease "4igb5uptxddpdjw9lwatd9psk": not found +// +// This is the LEASE half of the same "two databases describe one store" +// corruption the dangling-snapshot signature covers: BuildKit's bbolt +// metadata still holds a cache record while the containerd lease that record +// owns is gone (an out-of-band `ctr leases rm`, a restored containerd data +// dir, or a crash between a lease deletion and the matching metadata clear). +// The record loads fine — nothing on the load path checks the lease — and +// detonates only when a build cache-hits it and tries to attach a resource +// to the missing lease (buildkit source/containerimage/pull.go on a cached +// FROM, cache/blobs.go setBlob on export), failing the job with the raw +// containerd error above. +// +// Deliberately narrow, for the same reason as danglingSnapshotRe: repair +// throws away build cache. Only a quoted BuildKit cache-record lease ID — 25 +// base-36 characters (identity.NewID), optionally the -view / -variants +// derivative — followed by containerd's exact "not found" qualifies. +// BuildKit's temporary leases have a different shape ("-") and +// a missing one does not indicate a persistent desync, so they do not match. +var danglingLeaseRe = regexp.MustCompile(`lease "([a-z0-9]{25}(?:-view|-variants)?)": not found`) + +// DanglingLease identifies a containerd lease BuildKit's metadata believes in +// and containerd no longer has. +type DanglingLease struct { + // ID is the missing lease's ID from the error message — a BuildKit + // cache record ID, since the record's lease shares its ID. + ID string +} + +// ParseDanglingLease extracts the missing lease ID from an error message, +// reporting false when the message is some other failure. Pure — the whole +// detection rule, testable without standing up BuildKit. +func ParseDanglingLease(msg string) (DanglingLease, bool) { + m := danglingLeaseRe.FindStringSubmatch(msg) + if m == nil { + return DanglingLease{}, false + } + return DanglingLease{ID: m[1]}, true +} + +// DanglingLeaseFromError is ParseDanglingLease over an error value. Nil +// errors report false. +func DanglingLeaseFromError(err error) (DanglingLease, bool) { + if err == nil { + return DanglingLease{}, false + } + return ParseDanglingLease(err.Error()) +} + +// HealKeyFromError returns the escalation-ladder key for err's repairable +// shared-store signature, if it has one. It is what a caller that only needs +// the Healer bookkeeping key (e.g. to Forget after a successful retry) should +// use, so it stays in sync with every signature healAndRetryBuild acts on. +func HealKeyFromError(err error) (string, bool) { + if d, ok := DanglingSnapshotFromError(err); ok { + return d.ID, true + } + if l, ok := DanglingLeaseFromError(err); ok { + return l.ID, true + } + return "", false +} + // chainIDRe matches an OCI layer chain ID: an algorithm, a colon and a hex // digest. Anything else is BuildKit's own record ID. var chainIDRe = regexp.MustCompile(`^[a-z0-9]+(?:[.+_-][a-z0-9]+)*:[0-9a-fA-F]{32,}$`) @@ -211,7 +277,20 @@ type Healer struct { // Next reports the action to take for d and records it, so a second sighting // of the same key escalates instead of repeating a repair that did not work. func (h *Healer) Next(d DanglingSnapshot) HealAction { - if d.ID == "" { + return h.nextID(d.ID) +} + +// NextLease is Next for the missing-lease signature. Lease IDs and snapshot +// keys share one ladder map: they cannot collide (a lease ID is a full +// 25-char record ID, a repairable snapshot key is a chain ID or record ID +// with different provenance), and even a collision would only escalate a +// repair one rung early. +func (h *Healer) NextLease(l DanglingLease) HealAction { + return h.nextID(l.ID) +} + +func (h *Healer) nextID(id string) HealAction { + if id == "" { return HealNone } h.mu.Lock() @@ -220,13 +299,13 @@ func (h *Healer) Next(d DanglingSnapshot) HealAction { h.seen = map[string]HealAction{} } next := HealPrune - switch h.seen[d.ID] { + switch h.seen[id] { case HealPrune: next = HealRebuild case HealRebuild, HealGiveUp: next = HealGiveUp } - h.seen[d.ID] = next + h.seen[id] = next return next } @@ -242,8 +321,12 @@ func (h *Healer) Forget(id string) { // HealReport summarises one repair attempt, for logging and for the message // surfaced to the job when repair fails. type HealReport struct { - // Snapshot is the key that triggered the repair. + // Snapshot is the key that triggered the repair, when the trigger was + // the dangling-snapshot signature. Snapshot DanglingSnapshot + // Lease is the key that triggered the repair, when the trigger was the + // missing-lease signature. Exactly one of Snapshot and Lease is set. + Lease DanglingLease // Action is the rung of the ladder that was executed. Action HealAction // ImagesEvicted counts containerd image records dropped because their @@ -258,6 +341,10 @@ type HealReport struct { // String renders the report for a log line or an error message. func (r HealReport) String() string { + if r.Lease.ID != "" { + return fmt.Sprintf("lease=%s action=%s bytes_released=%d rebuilt=%t", + r.Lease.ID, r.Action, r.BytesReleased, r.Rebuilt) + } return fmt.Sprintf("snapshot=%s kind=%s action=%s images_evicted=%d bytes_released=%d rebuilt=%t", r.Snapshot.ID, r.Snapshot.Kind, r.Action, r.ImagesEvicted, r.BytesReleased, r.Rebuilt) } diff --git a/pkg/buildkit/heal_test.go b/pkg/buildkit/heal_test.go index 8ccfcad..f8df04a 100644 --- a/pkg/buildkit/heal_test.go +++ b/pkg/buildkit/heal_test.go @@ -104,6 +104,141 @@ func TestDanglingSnapshotFromError(t *testing.T) { } } +func TestParseDanglingLease(t *testing.T) { + tests := []struct { + name string + msg string + wantID string + wantOK bool + }{ + { + // The #193 incident string, verbatim: a cached FROM hit a + // cache record whose containerd lease was gone. + name: "missing cache record lease", + msg: `failed to solve: lease "4igb5uptxddpdjw9lwatd9psk": not found`, + wantID: "4igb5uptxddpdjw9lwatd9psk", + wantOK: true, + }, + { + name: "view lease derivative", + msg: `failed to solve: lease "4igb5uptxddpdjw9lwatd9psk-view": not found`, + wantID: "4igb5uptxddpdjw9lwatd9psk-view", + wantOK: true, + }, + { + name: "compression variants lease derivative", + msg: `lease "4igb5uptxddpdjw9lwatd9psk-variants": not found`, + wantID: "4igb5uptxddpdjw9lwatd9psk-variants", + wantOK: true, + }, + { + // A wrong positive throws away the node's build cache, so the + // near misses matter as much as the hits. BuildKit's temporary + // leases ("-") are not a persistent desync — a + // retry mints a fresh one — and must not trigger a repair. + name: "temporary lease id is a different failure", + msg: `lease "123456789-AbC_": not found`, + wantOK: false, + }, + { + // containerd's other missing-lease shape carries no ID and + // comes from a leased-context write, not a stale record. + name: "lease does not exist is a different failure", + msg: "lease does not exist: not found", + wantOK: false, + }, + { + // 24 chars: not an identity.NewID (always exactly 25). + name: "id of the wrong length", + msg: `lease "4igb5uptxddpdjw9lwatd9ps": not found`, + wantOK: false, + }, + { + name: "missing content is a different failure", + msg: `failed to solve: content digest sha256:abc: not found`, + wantOK: false, + }, + { + name: "empty", + msg: "", + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := ParseDanglingLease(tt.msg) + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v (got %+v)", ok, tt.wantOK, got) + } + if tt.wantOK && got.ID != tt.wantID { + t.Errorf("ID = %q, want %q", got.ID, tt.wantID) + } + }) + } +} + +func TestDanglingLeaseFromError(t *testing.T) { + if _, ok := DanglingLeaseFromError(nil); ok { + t.Fatal("nil error reported a dangling lease") + } + + // The signature must survive wrapping: it arrives from the solver + // through several layers of fmt.Errorf. + base := errors.New(`lease "4igb5uptxddpdjw9lwatd9psk": not found`) + wrapped := fmt.Errorf("build failed: %w", fmt.Errorf("failed to solve: %w", base)) + got, ok := DanglingLeaseFromError(wrapped) + if !ok { + t.Fatal("wrapped error not recognised") + } + if got.ID != "4igb5uptxddpdjw9lwatd9psk" { + t.Errorf("ID = %q", got.ID) + } +} + +func TestHealKeyFromError(t *testing.T) { + tests := []struct { + name string + err error + wantKey string + wantOK bool + }{ + { + name: "dangling snapshot", + err: errors.New("snapshot jfvdzwv6tyfkgcimx9uaifsfh does not exist: not found"), + wantKey: "jfvdzwv6tyfkgcimx9uaifsfh", + wantOK: true, + }, + { + name: "dangling lease", + err: errors.New(`failed to solve: lease "4igb5uptxddpdjw9lwatd9psk": not found`), + wantKey: "4igb5uptxddpdjw9lwatd9psk", + wantOK: true, + }, + { + name: "unrelated failure", + err: errors.New("no space left on device"), + wantOK: false, + }, + { + name: "nil", + err: nil, + wantOK: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + key, ok := HealKeyFromError(tt.err) + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v", ok, tt.wantOK) + } + if key != tt.wantKey { + t.Errorf("key = %q, want %q", key, tt.wantKey) + } + }) + } +} + func TestClassifySnapshotKey(t *testing.T) { tests := []struct { id string @@ -160,6 +295,33 @@ func TestHealerIgnoresEmptyID(t *testing.T) { if got := h.Next(DanglingSnapshot{}); got != HealNone { t.Fatalf("empty ID = %v, want HealNone", got) } + if got := h.NextLease(DanglingLease{}); got != HealNone { + t.Fatalf("empty lease ID = %v, want HealNone", got) + } +} + +func TestHealerLeaseLadder(t *testing.T) { + // The missing-lease signature climbs the same prune → rebuild → give-up + // ladder as the snapshot one, keyed by the lease ID. + var h Healer + l := DanglingLease{ID: "4igb5uptxddpdjw9lwatd9psk"} + + if got := h.NextLease(l); got != HealPrune { + t.Fatalf("first = %v, want HealPrune", got) + } + if got := h.NextLease(l); got != HealRebuild { + t.Fatalf("second = %v, want HealRebuild", got) + } + if got := h.NextLease(l); got != HealGiveUp { + t.Fatalf("third = %v, want HealGiveUp", got) + } + + // Forget re-arms the ladder after a successful retry, via the same key + // HealKeyFromError hands the build handler. + h.Forget(l.ID) + if got := h.NextLease(l); got != HealPrune { + t.Fatalf("after Forget = %v, want HealPrune", got) + } } func TestHealerConcurrent(t *testing.T) { diff --git a/pkg/dind/buildheal.go b/pkg/dind/buildheal.go index 8d62f75..e345746 100644 --- a/pkg/dind/buildheal.go +++ b/pkg/dind/buildheal.go @@ -25,9 +25,10 @@ import ( // reclaim, a restored data dir, a crash mid-delete — costs one build instead // of every build. -// healAndRetryBuild inspects a failed solve. When the failure is the -// dangling-snapshot signature it repairs the shared store and reports whether -// the caller should retry the build once. +// healAndRetryBuild inspects a failed solve. When the failure is one of the +// repairable shared-store corruption signatures — a dangling snapshot, or a +// cache record whose containerd lease is gone (#193) — it repairs the shared +// store and reports whether the caller should retry the build once. // // Repairing before retrying rather than merely marking the store dirty for the // next job is deliberate: the job that paid for the discovery is the one that @@ -35,11 +36,18 @@ import ( // from a slightly slow build; a job that hits it and fails is a red release // pipeline someone has to triage. func (s *Server) healAndRetryBuild(ctx context.Context, bk *buildkit.Server, solveErr error) (retry bool) { - d, ok := buildkit.DanglingSnapshotFromError(solveErr) - if !ok { - return false + if d, ok := buildkit.DanglingSnapshotFromError(solveErr); ok { + return s.healDanglingSnapshot(ctx, bk, d) + } + if l, ok := buildkit.DanglingLeaseFromError(solveErr); ok { + return s.healDanglingLease(ctx, bk, l) } + return false +} +// healDanglingSnapshot repairs the "metadata names a snapshot containerd no +// longer has" half of the shared-store corruption. See heal.go. +func (s *Server) healDanglingSnapshot(ctx context.Context, bk *buildkit.Server, d buildkit.DanglingSnapshot) bool { action := bk.Healer().Next(d) log := s.log.With("component", "build-heal", "snapshot", d.ID, "kind", d.Kind.String(), "action", action.String()) @@ -65,6 +73,53 @@ func (s *Server) healAndRetryBuild(ctx context.Context, bk *buildkit.Server, sol report.ImagesEvicted = n } + if !s.applyHealAction(ctx, bk, action, log, &report) { + return false + } + + log.Warn("shared build store repaired; retrying the build once", "report", report.String()) + return true +} + +// healDanglingLease repairs the reverse half of the same corruption: a cache +// record that SURVIVES in BuildKit's metadata while the containerd lease it +// owns is gone (#193). Nothing on the record-load path checks the lease, so +// the record keeps being served from cache and every build that hits it dies +// attaching a resource to the missing lease. +// +// The repair ladder is the same as the snapshot signature's, and the cheap +// rung genuinely fixes it: the failed solve released its refs, so PruneAll +// selects the stale record, and BuildKit's record removal tolerates the +// already-missing lease (cacheRecord.remove ignores NotFound) and clears the +// metadata. The retried build then re-pulls a clean copy. +// +// Unlike the snapshot path there is no image-record eviction half: image +// records reference snapshots via gc.ref labels, never leases, so no +// containerd image record can be poisoned by a missing lease. +func (s *Server) healDanglingLease(ctx context.Context, bk *buildkit.Server, l buildkit.DanglingLease) bool { + action := bk.Healer().NextLease(l) + log := s.log.With("component", "build-heal", "lease", l.ID, "action", action.String()) + + if action == buildkit.HealGiveUp { + log.Error("shared build store still references a lease containerd does not have, after both a prune and a metadata rebuild — not retrying; inspect the node") + return false + } + + log.Warn("build failed on a lease the shared store references but containerd does not have; repairing the shared store") + + report := buildkit.HealReport{Lease: l, Action: action} + if !s.applyHealAction(ctx, bk, action, log, &report) { + return false + } + + log.Warn("shared build store repaired; retrying the build once", "report", report.String()) + return true +} + +// applyHealAction executes one rung of the repair ladder, escalating a failed +// prune to a rebuild. Returns false when the store could not be repaired at +// all — the caller must then fail the build rather than retry. +func (s *Server) applyHealAction(ctx context.Context, bk *buildkit.Server, action buildkit.HealAction, log *slog.Logger, report *buildkit.HealReport) bool { switch action { case buildkit.HealPrune: released, err := bk.PruneAll(ctx) @@ -87,8 +142,6 @@ func (s *Server) healAndRetryBuild(ctx context.Context, bk *buildkit.Server, sol } report.Rebuilt = true } - - log.Warn("shared build store repaired; retrying the build once", "report", report.String()) return true } diff --git a/pkg/dind/buildkit_build.go b/pkg/dind/buildkit_build.go index f62b1f9..4b8fc09 100644 --- a/pkg/dind/buildkit_build.go +++ b/pkg/dind/buildkit_build.go @@ -105,15 +105,16 @@ func (s *Server) handleImageBuildBuildkit(bk *buildkit.Server) http.HandlerFunc w.WriteHeader(http.StatusOK) flusher, _ := w.(http.Flusher) - // The solve runs at most twice. A first failure carrying the - // dangling-snapshot signature means the SHARED build store names a - // snapshot containerd no longer has — a state no job can repair + // The solve runs at most twice. A first failure carrying a + // repairable shared-store signature — a dangling snapshot, or a + // cache record whose containerd lease is gone — means the SHARED + // build store disagrees with containerd in a way no job can repair // from inside its own namespace (see buildheal.go), so ephemerd // repairs it host-side and gives this build the second chance. Any // other failure, and any second failure, is final. solveErr := s.runSolve(r.Context(), bk, opt, w, flusher) if solveErr != nil { - healed, _ := buildkit.DanglingSnapshotFromError(solveErr) + healKey, _ := buildkit.HealKeyFromError(solveErr) if s.healAndRetryBuild(r.Context(), bk, solveErr) { if err := writeJSONLine(w, map[string]any{ "stream": "ephemerd: repaired a corrupt entry in the shared build store; retrying the build\n", @@ -129,7 +130,7 @@ func (s *Server) handleImageBuildBuildkit(bk *buildkit.Server) http.HandlerFunc // The repair stuck. Drop the escalation state so a // recurrence weeks from now starts from the cheap // rung again instead of rebuilding the store. - bk.Healer().Forget(healed.ID) + bk.Healer().Forget(healKey) } } } diff --git a/pkg/dind/buildkit_cleanup.go b/pkg/dind/buildkit_cleanup.go index 29bb3be..ebba8ef 100644 --- a/pkg/dind/buildkit_cleanup.go +++ b/pkg/dind/buildkit_cleanup.go @@ -4,6 +4,7 @@ import ( "context" "log/slog" "strings" + "time" "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/core/images" @@ -41,8 +42,34 @@ func buildRecordJobID(name string) (string, bool) { return jobID, true } -// DeadBuildRecords returns the job-scoped build result records in names -// whose job is not in live, together with the job each belongs to. +// DeadBuildRecordGrace is how recently a job-scoped record may have been +// written and still be exempt from the dead-job sweep. +// +// The grace closes a TOCTOU in the sweep's liveness check: the live set is a +// container listing captured BEFORE the record listing, so a job that starts +// — and exports a build — between the two listings has records the stale +// live set would judge dead, and the sweep would delete a live job's build +// output mid-job (breaking its subsequent docker push, and any containerd GC +// the synchronous delete triggers runs against the job's in-flight solve). +// A record can only be at risk if it was written after the live set was +// captured, i.e. within the last pass; anything younger than the grace is +// therefore skipped and picked up by a later tick once its job is provably +// gone. Same principle as runtime.ReapDeadContainers' grace window: never +// collect a resource whose owning operation could still be live. +const DeadBuildRecordGrace = 15 * time.Minute + +// BuildRecord is one image record in the buildkit namespace as the dead-job +// sweep sees it: its name and when containerd last wrote it. +type BuildRecord struct { + Name string + // UpdatedAt is the record's last containerd write (falling back to its + // creation time when it was never updated). Zero means the age is + // unknown. + UpdatedAt time.Time +} + +// DeadBuildRecords returns the job-scoped build result records in records +// whose job is not in live and whose last write is older than grace. // // This is the decision half of the buildkit-namespace leak fix, kept pure // so the "which records are garbage" rule is testable on its own. @@ -58,22 +85,27 @@ func buildRecordJobID(name string) (string, bool) { // // live holds container IDs as ephemerd knows them; matching is done on the // lowercased form because scopedBuildRef lowercases the ID to satisfy -// Docker's reference grammar. -func DeadBuildRecords(names []string, live map[string]struct{}) []string { +// Docker's reference grammar. Records younger than grace — or with no +// usable timestamp, since an unknown age must never satisfy an "older than" +// test — are never selected; see DeadBuildRecordGrace for why. +func DeadBuildRecords(records []BuildRecord, live map[string]struct{}, now time.Time, grace time.Duration) []string { lowerLive := make(map[string]struct{}, len(live)) for id := range live { lowerLive[strings.ToLower(id)] = struct{}{} } var out []string - for _, name := range names { - jobID, ok := buildRecordJobID(name) + for _, rec := range records { + jobID, ok := buildRecordJobID(rec.Name) if !ok { continue } if _, alive := lowerLive[jobID]; alive { continue } - out = append(out, name) + if rec.UpdatedAt.IsZero() || now.Sub(rec.UpdatedAt) <= grace { + continue + } + out = append(out, rec.Name) } return out } @@ -129,7 +161,9 @@ func CleanupJobBuildRecords(ctx context.Context, c *client.Client, buildkitNS, j // or a daemon that predates per-job cleanup entirely. // // Safe to run while jobs are in flight — records belonging to a live job -// are skipped. +// are skipped, and records written more recently than DeadBuildRecordGrace +// are left for a later tick because their job may have started after live +// was captured. func PruneDeadBuildRecords(ctx context.Context, c *client.Client, buildkitNS string, live map[string]struct{}, log *slog.Logger) (int, error) { if c == nil || buildkitNS == "" { return 0, nil @@ -142,13 +176,17 @@ func PruneDeadBuildRecords(ctx context.Context, c *client.Client, buildkitNS str } return 0, err } - names := make([]string, 0, len(imgs)) + records := make([]BuildRecord, 0, len(imgs)) for _, img := range imgs { - names = append(names, img.Name) + rec := BuildRecord{Name: img.Name, UpdatedAt: img.UpdatedAt} + if rec.UpdatedAt.IsZero() { + rec.UpdatedAt = img.CreatedAt + } + records = append(records, rec) } deleted := 0 - for _, name := range DeadBuildRecords(names, live) { + for _, name := range DeadBuildRecords(records, live, time.Now(), DeadBuildRecordGrace) { if derr := deleteBuildRecord(nsCtx, c, name); derr != nil { log.Warn("buildkit cleanup: image delete", "namespace", buildkitNS, "image", name, "error", derr) diff --git a/pkg/dind/buildkit_cleanup_test.go b/pkg/dind/buildkit_cleanup_test.go index c350f51..ef73e06 100644 --- a/pkg/dind/buildkit_cleanup_test.go +++ b/pkg/dind/buildkit_cleanup_test.go @@ -3,6 +3,7 @@ package dind import ( "reflect" "testing" + "time" ) func TestBuildScopePrefix(t *testing.T) { @@ -98,18 +99,24 @@ func TestDeadBuildRecords(t *testing.T) { ) scoped := func(job, ref string) string { return BuildScopePrefix(job) + ref } + now := time.Date(2026, 9, 2, 12, 0, 0, 0, time.UTC) + const grace = 15 * time.Minute + old := now.Add(-time.Hour) // safely past the grace window + young := now.Add(-time.Minute) // inside the grace window + rec := func(name string, at time.Time) BuildRecord { return BuildRecord{Name: name, UpdatedAt: at} } + tests := []struct { - name string - names []string - live map[string]struct{} - want []string + name string + records []BuildRecord + live map[string]struct{} + want []string }{ { name: "selects records of dead jobs only", - names: []string{ - scoped(liveJob, "ephpm:dev"), - scoped(dead1, "ephpm:dev"), - scoped(dead2, "ephpm:dev"), + records: []BuildRecord{ + rec(scoped(liveJob, "ephpm:dev"), old), + rec(scoped(dead1, "ephpm:dev"), old), + rec(scoped(dead2, "ephpm:dev"), old), }, live: map[string]struct{}{liveJob: {}}, want: []string{scoped(dead1, "ephpm:dev"), scoped(dead2, "ephpm:dev")}, @@ -119,37 +126,62 @@ func TestDeadBuildRecords(t *testing.T) { // from containerd container IDs, which may not be. Matching // must be case-insensitive or a live job's build output gets // deleted mid-job. - name: "live job matching is case-insensitive", - names: []string{scoped("Ephemerd-Mixed-Case", "img:v1")}, - live: map[string]struct{}{"Ephemerd-Mixed-Case": {}}, - want: nil, + name: "live job matching is case-insensitive", + records: []BuildRecord{rec(scoped("Ephemerd-Mixed-Case", "img:v1"), old)}, + live: map[string]struct{}{"Ephemerd-Mixed-Case": {}}, + want: nil, + }, + { + // The live set is captured BEFORE the record listing, so a + // record written moments ago may belong to a job that started + // in between — it must survive this pass even though its job + // is not in the (stale) live set. A later tick collects it + // once the job is provably gone. + name: "never selects a record younger than the grace", + records: []BuildRecord{rec(scoped(dead1, "ephpm:dev"), young)}, + live: nil, + want: nil, + }, + { + // An unknown age can never satisfy "older than the grace" — + // fail safe, exactly like the dead-container reaper. + name: "never selects a record with no timestamp", + records: []BuildRecord{rec(scoped(dead1, "ephpm:dev"), time.Time{})}, + live: nil, + want: nil, }, { // BuildKit's own cache records live in the same namespace // and are owned by its bbolt DB — deleting them behind its // back leaves snapshots pinned and corrupts the index. - name: "never selects records that are not job-scoped", - names: []string{"docker.io/library/alpine:3", "sha256:deadbeef"}, - live: nil, - want: nil, + name: "never selects records that are not job-scoped", + records: []BuildRecord{ + rec("docker.io/library/alpine:3", old), + rec("sha256:deadbeef", old), + }, + live: nil, + want: nil, }, { - name: "no live jobs means every scoped record is dead", - names: []string{scoped(dead1, "a:1"), scoped(dead2, "b:2")}, - live: nil, - want: []string{scoped(dead1, "a:1"), scoped(dead2, "b:2")}, + name: "no live jobs means every old scoped record is dead", + records: []BuildRecord{ + rec(scoped(dead1, "a:1"), old), + rec(scoped(dead2, "b:2"), old), + }, + live: nil, + want: []string{scoped(dead1, "a:1"), scoped(dead2, "b:2")}, }, { - name: "empty input", - names: nil, - live: map[string]struct{}{liveJob: {}}, - want: nil, + name: "empty input", + records: nil, + live: map[string]struct{}{liveJob: {}}, + want: nil, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := DeadBuildRecords(tc.names, tc.live) + got := DeadBuildRecords(tc.records, tc.live, now, grace) if !reflect.DeepEqual(got, tc.want) { t.Errorf("DeadBuildRecords() = %v, want %v", got, tc.want) } From f65642af7a7ddd8c5b9db3d264ec3044d05083f8 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 1 Sep 2026 22:12:46 -0700 Subject: [PATCH 2/2] fix(buildkit): tail-anchor the repair matchers; never escalate under a canceled ctx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the parent commit: - Both dangling-signature regexes matched anywhere in the error string, and job-influenced text reaches that string: a hostile registry's HTTP reason phrase (preserved verbatim by containerd's remote error) or a RUN command's own echoed output could embed a fake match and trigger a shared-cache PruneAll — repair throws away every job's warm cache, and a repeated fake key escalates to a Rebuild that stops the shared gRPC core. Genuine containerd not-founds are always the TAIL of the wrap chain while job-influenced text always gets further wrapping appended, so anchoring both matchers to end-of-string closes the vector without losing a single real signature (all existing genuine-string tests pass unchanged). Matters doubly with the fork-PR flip on the roadmap. - A canceled ctx (runner disconnect mid-build) made PruneAll fail with context.Canceled, which the ladder read as store inconsistency and escalated to Rebuild under the same dead ctx — the worst path, since a failed Rebuild leaves the shared core stopped. applyHealAction now refuses to repair or escalate under a canceled ctx; a genuine desync re-fires the ladder from the next live build. - Pinned the near-miss shapes review flagged as untested: history-lease ref_ prefix, 26/24-char ids, temporary-lease shape, id-less variant, and both wrapped job-influenced forms. --- pkg/buildkit/heal.go | 17 +++++++++++++++-- pkg/buildkit/heal_test.go | 33 +++++++++++++++++++++++++++++++++ pkg/dind/buildheal.go | 19 ++++++++++++++++++- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/pkg/buildkit/heal.go b/pkg/buildkit/heal.go index 5adb35b..3243bb3 100644 --- a/pkg/buildkit/heal.go +++ b/pkg/buildkit/heal.go @@ -62,7 +62,12 @@ import ( // // The ID character class allows the ':' and digest hex of a chain ID as well // as BuildKit's base-36 record IDs. -var danglingSnapshotRe = regexp.MustCompile(`(?:parent )?snapshot ([A-Za-z0-9][A-Za-z0-9._:+\-]*) does not exist`) +// Tail-anchored for the same reason as danglingLeaseRe below: the genuine +// containerd error is the wrap chain's suffix ("... does not exist: not +// found" ends the string), while a RUN command that merely PRINTS a matching +// phrase gets further wrapping appended ("did not complete successfully: +// exit code ..."), so job-controlled text cannot reach the repair ladder. +var danglingSnapshotRe = regexp.MustCompile(`(?:parent )?snapshot ([A-Za-z0-9][A-Za-z0-9._:+\-]*) does not exist(?:: not found)?$`) // SnapshotKind classifies a dangling snapshot key by who owns it, which // decides what repairing it involves. @@ -159,7 +164,15 @@ func DanglingSnapshotFromError(err error) (DanglingSnapshot, bool) { // derivative — followed by containerd's exact "not found" qualifies. // BuildKit's temporary leases have a different shape ("-") and // a missing one does not indicate a persistent desync, so they do not match. -var danglingLeaseRe = regexp.MustCompile(`lease "([a-z0-9]{25}(?:-view|-variants)?)": not found`) +// +// Anchored to the END of the error string: a genuine containerd not-found is +// always the tail of the wrap chain, while job-influenced text (a hostile +// registry's HTTP reason phrase, a RUN command's own output echoed into a +// failure) is followed by more wrapping ("did not complete successfully", +// status codes, ...). Without the anchor, a fork PR could embed a fake +// lease line in an error it controls and trigger a shared-cache prune — +// repair must never be reachable from attacker-influenced text. +var danglingLeaseRe = regexp.MustCompile(`lease "([a-z0-9]{25}(?:-view|-variants)?)": not found$`) // DanglingLease identifies a containerd lease BuildKit's metadata believes in // and containerd no longer has. diff --git a/pkg/buildkit/heal_test.go b/pkg/buildkit/heal_test.go index f8df04a..d69e6a1 100644 --- a/pkg/buildkit/heal_test.go +++ b/pkg/buildkit/heal_test.go @@ -399,3 +399,36 @@ func dirExists(p string) bool { st, err := os.Stat(p) return err == nil && st.IsDir() } + +// TestMatchersRejectJobInfluencedText pins the tail anchoring and the +// near-miss shapes: the repair ladder throws away shared build cache, so it +// must be unreachable from any error string a job can influence, and from +// lease shapes that are not cache-record leases. A future "loosen the regex" +// edit must fail here, not in production. +func TestMatchersRejectJobInfluencedText(t *testing.T) { + reject := []struct{ name, msg string }{ + // Attacker-influenced text is never the tail of the wrap chain — + // buildkit appends its own wrapping after job output / registry + // reason phrases. The anchor is what rejects these. + {"registry reason phrase gets wrapped", + `failed to solve: failed to load cache key: unexpected status from HEAD request to https://evil.example/v2/x/manifests/latest: 404 lease "aaaaaaaaaaaaaaaaaaaaaaaaa": not found (server message)`}, + {"RUN output echoed into a failure gets wrapped", + `process "/bin/sh -c echo snapshot deadbeef does not exist: not found; false" did not complete successfully: exit code: 1`}, + // Lease shapes that are not cache-record leases. + {"temporary lease shape", `lease "1755846719000000000-aBcD": not found`}, + {"history lease ref_ prefix", `lease "ref_4igb5uptxddpdjw9lwatd9psk": not found`}, + {"26-char id", `lease "4igb5uptxddpdjw9lwatd9pskx": not found`}, + {"24-char id", `lease "4igb5uptxddpdjw9lwatd9ps": not found`}, + {"id-less lease error", `lease does not exist: not found`}, + } + for _, c := range reject { + t.Run(c.name, func(t *testing.T) { + if _, ok := ParseDanglingLease(c.msg); ok { + t.Fatalf("ParseDanglingLease matched %q, must reject", c.msg) + } + if _, ok := ParseDanglingSnapshot(c.msg); ok { + t.Fatalf("ParseDanglingSnapshot matched %q, must reject", c.msg) + } + }) + } +} diff --git a/pkg/dind/buildheal.go b/pkg/dind/buildheal.go index e345746..7b67769 100644 --- a/pkg/dind/buildheal.go +++ b/pkg/dind/buildheal.go @@ -120,13 +120,30 @@ func (s *Server) healDanglingLease(ctx context.Context, bk *buildkit.Server, l b // prune to a rebuild. Returns false when the store could not be repaired at // all — the caller must then fail the build rather than retry. func (s *Server) applyHealAction(ctx context.Context, bk *buildkit.Server, action buildkit.HealAction, log *slog.Logger, report *buildkit.HealReport) bool { + // A canceled ctx (runner disconnected mid-build) must never escalate: + // the prune "failure" it produces is context.Canceled, not evidence of + // store inconsistency, and a Rebuild attempted under a dead ctx is the + // worst-case path — Rebuild stops the shared gRPC core first, and if + // newCore then fails the node is left build-dead until restart. Fail + // the (already-abandoned) build instead; a genuine desync will fire + // the ladder again from a live build. + if ctx.Err() != nil { + log.Warn("skipping shared-store repair under a canceled context", "error", ctx.Err()) + return false + } switch action { case buildkit.HealPrune: released, err := bk.PruneAll(ctx) if err != nil { // A prune that cannot even enumerate the store is itself // evidence the metadata is inconsistent. Escalate now rather - // than burning another build to learn the same thing. + // than burning another build to learn the same thing — but + // never off the back of a canceled ctx (see above), whose + // prune failure proves nothing about the store. + if ctx.Err() != nil { + log.Warn("build cache prune canceled; not escalating", "error", err) + return false + } log.Warn("build cache prune failed; escalating to a metadata rebuild", "error", err) if rerr := bk.Rebuild(ctx); rerr != nil { log.Error("rebuilding the buildkit metadata store failed", "error", rerr)