From f30cb4c3e4e9dfa95497968ece2ac099838705ca Mon Sep 17 00:00:00 2001 From: Kieran Mann Date: Wed, 7 Oct 2026 17:20:17 -0700 Subject: [PATCH] fix(remotecache): make cache-export loop detection linear and cancellable checkLoops walked the normalized cache graph with only the current DFS path in its visited set and never remembered fully explored nodes, so it enumerated every root-to-leaf path. That is exponential in the number of fan-in "diamonds" (A -> {B, C} -> D) in the graph: each extra diamond doubles the work. On large build graphs with many FROM / COPY --from fan-ins, "preparing build cache for export" never finishes. Nothing on that path checked the context either, and ExportTo turned a child's context error into an empty result, so a disconnected or cancelled client could not stop the export. Stuck exports kept a core busy each and accumulated until the daemon ran out of memory. - remember fully explored nodes in checkLoops (three-colour DFS), so loop removal visits each node and edge once - check ctx in normalize and checkLoops, and return its error - return the context error from ExportTo instead of nil, nil when the context is done; other child errors are still skipped as before Upstream moby/buildkit removed this normalization code entirely in 2d7ef04df ("remotecache: refactor to new cache export interface", moby/buildkit#6129, v0.25.0). That refactor needs Go 1.23 (`unique`), containerd v2 and a series of follow-up fixes, so it is not backported here. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Kieran Mann --- cache/remotecache/v1/chains.go | 7 +- cache/remotecache/v1/chains_test.go | 102 ++++++++++++++++++++++++++++ cache/remotecache/v1/utils.go | 30 ++++++-- solver/exporter.go | 6 ++ 4 files changed, 137 insertions(+), 8 deletions(-) diff --git a/cache/remotecache/v1/chains.go b/cache/remotecache/v1/chains.go index 11ea24b86..93441826b 100644 --- a/cache/remotecache/v1/chains.go +++ b/cache/remotecache/v1/chains.go @@ -60,13 +60,18 @@ func (c *CacheChains) normalize(ctx context.Context) error { c.items = validated for _, it := range c.items { + if err := ctx.Err(); err != nil { + return err + } _, err := normalizeItem(it, st) if err != nil { return err } } - st.removeLoops(ctx) + if err := st.removeLoops(ctx); err != nil { + return err + } items := make([]*item, 0, len(st.byKey)) for _, it := range st.byKey { diff --git a/cache/remotecache/v1/chains_test.go b/cache/remotecache/v1/chains_test.go index 5e7bcd069..322aae568 100644 --- a/cache/remotecache/v1/chains_test.go +++ b/cache/remotecache/v1/chains_test.go @@ -3,6 +3,7 @@ package cacheimport import ( "context" "encoding/json" + "fmt" "testing" "time" @@ -95,3 +96,104 @@ func TestSimpleMarshal(t *testing.T) { func dgst(s string) digest.Digest { return digest.FromBytes([]byte(s)) } + +// addDiamondChain adds a chain of n fan-in "diamonds" (a_i -> {b_i, c_i} -> a_i+1) +// to cc. The graph has 3n+1 items but 2^n distinct root-to-leaf paths. +func addDiamondChain(cc *CacheChains, n int) { + prev := cc.Add(outputKey(dgst("a0"), 0)) + prev.AddResult("", 0, time.Now(), &solver.Remote{}) + for i := 0; i < n; i++ { + b := cc.Add(outputKey(dgst(fmt.Sprintf("b%d", i)), 0)) + b.LinkFrom(prev, 0, "") + c := cc.Add(outputKey(dgst(fmt.Sprintf("c%d", i)), 0)) + c.LinkFrom(prev, 0, "") + a := cc.Add(outputKey(dgst(fmt.Sprintf("a%d", i+1)), 0)) + a.LinkFrom(b, 0, "") + a.LinkFrom(c, 1, "") + prev = a + } +} + +// TestMarshalDiamondChain guards against loop detection enumerating every +// path through the cache graph, which is exponential in the number of +// diamonds and made cache export hang on large build graphs. +func TestMarshalDiamondChain(t *testing.T) { + const n = 1000 + cc := NewCacheChains() + addDiamondChain(cc, n) + + done := make(chan error, 1) + var cfg *CacheConfig + go func() { + var err error + cfg, _, err = cc.Marshal(context.TODO()) + done <- err + }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(30 * time.Second): + t.Fatalf("marshaling a chain of %d diamonds did not finish in time", n) + } + require.Equal(t, 3*n+1, len(cfg.Records)) +} + +func TestMarshalCanceled(t *testing.T) { + cc := NewCacheChains() + addDiamondChain(cc, 10) + + ctx, cancel := context.WithCancel(context.TODO()) + cancel() + _, _, err := cc.Marshal(ctx) + require.ErrorIs(t, err, context.Canceled) +} + +// TestMarshalLoop checks that loops created by deduplication during +// normalization are still broken, including when the loop is reachable +// through several paths (so its nodes are already fully explored when the +// second path reaches them). +func TestMarshalLoop(t *testing.T) { + cc := NewCacheChains() + + root := cc.Add(outputKey(dgst("root"), 0)) + root.AddResult("", 0, time.Now(), &solver.Remote{}) + // diamond in front of the loop: root -> {p, q} -> x + p := cc.Add(outputKey(dgst("p"), 0)) + p.LinkFrom(root, 0, "") + q := cc.Add(outputKey(dgst("q"), 0)) + q.LinkFrom(root, 0, "") + x1 := cc.Add(outputKey(dgst("x"), 0)) + x1.LinkFrom(p, 0, "") + x1.LinkFrom(q, 0, "") + y := cc.Add(outputKey(dgst("y"), 0)) + y.LinkFrom(x1, 0, "") + // x2 has the same digest as x1 and one input alternative in common, so + // normalization merges it into x1, which creates the loop x -> y -> x. + x2 := cc.Add(outputKey(dgst("x"), 0)) + x2.LinkFrom(p, 0, "") + x2.LinkFrom(y, 0, "") + + cfg, _, err := cc.Marshal(context.TODO()) + require.NoError(t, err) + require.Equal(t, 5, len(cfg.Records)) + + // the exported records must form a DAG + state := map[int]int{} + var visit func(i int) + visit = func(i int) { + require.NotEqual(t, 1, state[i], "loop in exported cache records") + if state[i] == 2 { + return + } + state[i] = 1 + for _, inputs := range cfg.Records[i].Inputs { + for _, in := range inputs { + visit(in.LinkIndex) + } + } + state[i] = 2 + } + for i := range cfg.Records { + visit(i) + } +} diff --git a/cache/remotecache/v1/utils.go b/cache/remotecache/v1/utils.go index 213e670a6..6b99bce56 100644 --- a/cache/remotecache/v1/utils.go +++ b/cache/remotecache/v1/utils.go @@ -128,7 +128,7 @@ type normalizeState struct { next int } -func (s *normalizeState) removeLoops(ctx context.Context) { +func (s *normalizeState) removeLoops(ctx context.Context) error { roots := []digest.Digest{} for dgst, it := range s.byKey { if len(it.links) == 0 { @@ -136,21 +136,35 @@ func (s *normalizeState) removeLoops(ctx context.Context) { } } + // visited holds the digests on the current DFS path; done holds the + // digests whose subtrees have been fully explored. Without done, every + // root-to-leaf path is enumerated, which is exponential in the number of + // fan-in "diamonds" (A->{B,C}->D) in the graph. visited := map[digest.Digest]struct{}{} + done := map[digest.Digest]struct{}{} for _, d := range roots { - s.checkLoops(ctx, d, visited) + if err := s.checkLoops(ctx, d, visited, done); err != nil { + return err + } } + return nil } -func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visited map[digest.Digest]struct{}) { +func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visited, done map[digest.Digest]struct{}) error { + if _, ok := done[d]; ok { + return nil + } + if err := ctx.Err(); err != nil { + return err + } it, ok := s.byKey[d] if !ok { - return + return nil } links, ok := s.links[it] if !ok { - return + return nil } visited[d] = struct{}{} defer func() { @@ -168,11 +182,13 @@ func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visite bklog.G(ctx).Warnf("failed to remove looping cache key %s %s", d, id) } delete(links[l], id) - } else { - s.checkLoops(ctx, id, visited) + } else if err := s.checkLoops(ctx, id, visited, done); err != nil { + return err } } } + done[d] = struct{}{} + return nil } func normalizeItem(it *item, state *normalizeState) (*item, error) { diff --git a/solver/exporter.go b/solver/exporter.go index 44e788ee2..95f9235db 100644 --- a/solver/exporter.go +++ b/solver/exporter.go @@ -172,6 +172,9 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach for _, dep := range deps { recs, err := dep.CacheKey.Exporter.ExportTo(ctx, t, opt) if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } return nil, nil } for _, r := range recs { @@ -184,6 +187,9 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach for _, de := range e.edge.secondaryExporters { recs, err := de.cacheKey.CacheKey.Exporter.ExportTo(ctx, t, opt) if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } return nil, nil } for _, r := range recs {