From f8961974742c3280f3aa65c8f04085d5d13a81ce Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 17 Aug 2026 11:56:12 -0700 Subject: [PATCH] remotecache: fix silently dropped cache link addItemToStorage (import) and marshalItem (export) both build their output by recursively walking an item's own dependencies, memoizing each item's storage entry / record slot so a shared dependency is only resolved once. Both allocated that memoized entry only *after* the recursive walk finished, using a sentinel ("" / -1) to mark an item as already in progress in the meantime. When an item is reachable through more than one path - which happens in practice once two unrelated ops coincidentally produce byte-identical content, since their cache records can end up several levels deep in each other's dependency chains despite having distinct provenance - a second path can revisit an item while the first path is still in the middle of resolving it. Before this fix, that reentrant call found only the in-progress sentinel, got back nil/-1, and its caller silently skipped registering the link - with no error, no warning. Which links survived depended on Go's randomized per-process map iteration order (cc.leaves(), and the multi-candidate alternatives at a single input slot), so reconstructing the exact same chain from the exact same bytes could non-deterministically drop a link on some runs and not others. Both an item's final id (computed by computeIDs before addItemToStorage runs) and its record's array index (known as soon as its slot in state.records is reserved) are available up front, before any recursion. Allocate and register the entry immediately instead of after, so a reentrant call gets back the same, real (if not yet fully populated) entry and can append its link to it successfully - nothing is silently dropped anymore, regardless of traversal order. This also makes addItemToStorage's separate `visited` map and the k.byItem "" in-progress sentinel (and its "invalid loop" error branch) redundant: k.byItem/k.byID alone now correctly memoize both same-call and cross-call revisits, so `visited` was removed. Verified with cache/remotecache/v1/cachestorage_test.go using a real `--cache-to type=local,mode=max` export captured from a repro build (testdata/cyclic-merge-chain.json): reparsing it into a fresh NewCacheKeyStorage dropped the affected link on ~60-80% of iterations against the pre-fix implementation (closely matching the failure rate reported in the field) and 0% after this change. Also verified end-to-end against real buildkitd builds (docker-container driver, both type=registry and type=local cache backends, fresh builder per iteration): 0 spurious cache misses across 95+ iterations post-fix versus a 60-100% failure rate before it, including a wider variant with more parallel merge points and a byte-for-byte content comparison between a fresh build and a cache-imported one. Signed-off-by: Spencer G. Jones --- cache/remotecache/v1/cachestorage.go | 45 +- cache/remotecache/v1/cachestorage_test.go | 63 +++ .../v1/testdata/cyclic-merge-chain.json | 475 ++++++++++++++++++ cache/remotecache/v1/utils.go | 31 +- 4 files changed, 575 insertions(+), 39 deletions(-) create mode 100644 cache/remotecache/v1/cachestorage_test.go create mode 100644 cache/remotecache/v1/testdata/cyclic-merge-chain.json diff --git a/cache/remotecache/v1/cachestorage.go b/cache/remotecache/v1/cachestorage.go index 2b9e645543b1..8e1e4545d32b 100644 --- a/cache/remotecache/v1/cachestorage.go +++ b/cache/remotecache/v1/cachestorage.go @@ -23,8 +23,7 @@ func NewCacheKeyStorage(cc *CacheChains, w worker.Worker) (solver.CacheKeyStorag cc.computeIDs() for it := range cc.leaves() { - visited := make(map[*item]*itemWithOutgoingLinks) - if _, err := addItemToStorage(storage, it, visited); err != nil { + if _, err := addItemToStorage(storage, it); err != nil { return nil, nil, err } } @@ -39,31 +38,35 @@ func NewCacheKeyStorage(cc *CacheChains, w worker.Worker) (solver.CacheKeyStorag return storage, results, nil } -func addItemToStorage(k *cacheKeyStorage, it *item, visited map[*item]*itemWithOutgoingLinks) (*itemWithOutgoingLinks, error) { - if v, ok := visited[it]; ok { - return v, nil - } - visited[it] = nil - +func addItemToStorage(k *cacheKeyStorage, it *item) (*itemWithOutgoingLinks, error) { if id, ok := k.byItem[it]; ok { - if id == "" { - return nil, errors.New("invalid loop") - } return k.byID[id], nil } + // it.id is already final at this point (computeIDs runs before this + // function is ever called), so it's safe to register this item's + // storage entry - and make it visible to other callers - before + // recursing into its own dependencies below. Merge ops can coincidentally + // produce byte-identical content at more than one point in the same + // chain, which makes the dependency graph loop back on itself; if a + // reentrant call for the same item arrives while we're still walking + // its parents, it now gets this same, real (if not yet fully populated) + // entry back and can append its link to it, rather than getting nil and + // silently dropping the link. id := it.id - k.byItem[it] = "" + itl := &itemWithOutgoingLinks{ + item: it, + links: map[nlink][]string{}, + } + k.byItem[it] = id + k.byID[id] = itl for i, m := range it.parents { for l := range m { - src, err := addItemToStorage(k, l.src, visited) + src, err := addItemToStorage(k, l.src) if err != nil { return nil, err } - if src == nil { - continue - } cl := nlink{ input: i, dgst: it.dgst, @@ -73,15 +76,6 @@ func addItemToStorage(k *cacheKeyStorage, it *item, visited map[*item]*itemWithO } } - k.byItem[it] = id - - itl := &itemWithOutgoingLinks{ - item: it, - links: map[nlink][]string{}, - } - - k.byID[id] = itl - seen := map[string]struct{}{} for _, res := range it.results { resultID := remoteID(res.Result) @@ -96,7 +90,6 @@ func addItemToStorage(k *cacheKeyStorage, it *item, visited map[*item]*itemWithO } ids[id] = struct{}{} } - visited[it] = itl return itl, nil } diff --git a/cache/remotecache/v1/cachestorage_test.go b/cache/remotecache/v1/cachestorage_test.go new file mode 100644 index 000000000000..108367ca7171 --- /dev/null +++ b/cache/remotecache/v1/cachestorage_test.go @@ -0,0 +1,63 @@ +package cacheimport + +import ( + "os" + "testing" + + "github.com/moby/buildkit/solver" + "github.com/stretchr/testify/require" +) + +// TestNewCacheKeyStorageCyclicMergeChain guards against a regression where +// addItemToStorage could silently drop a dependency link. +// +// testdata/cyclic-merge-chain.json is a real `--cache-to type=local,mode=max` +// export captured from a build where two unrelated COPY operations +// coincidentally produced byte-identical content (see the Add doc comment +// in chains.go): their cache records end up sharing a digest despite having +// distinct dependency chains. Several levels of that coincidence deep, the +// resulting graph makes NewCacheKeyStorage's traversal revisit an item that +// a different path is still in the middle of resolving. +// +// Before the fix, addItemToStorage only allocated an item's storage entry +// after fully walking its own dependencies, so a reentrant call arriving +// mid-walk found no entry yet, got nil, and the caller silently skipped +// registering its link - with no error. Which links survived depended on +// Go's randomized map iteration order (cc.leaves(), and the multi-candidate +// alternatives at a single input slot), so reconstructing the exact same +// chain from the exact same bytes could non-deterministically drop a link +// on some process runs and not others. Parsing this fixture repeatedly +// reproduces that: it drops the link on roughly 60-80% of iterations +// against the pre-fix implementation, closely matching the failure rate +// observed in the field. +func TestNewCacheKeyStorageCyclicMergeChain(t *testing.T) { + dt, err := os.ReadFile("testdata/cyclic-merge-chain.json") + require.NoError(t, err) + + // The selector on the specific link that a merge step's cache key uses + // to depend on another record that coincidentally shares its digest. + const wantSelector = "sha256:a7cb13d089672f15c02003beb688fb5642a1c62b68ffeee7c2ff29d26490deec" + + // A single iteration isn't a reliable regression check: the bug this + // guards against is intermittent (Go randomizes map iteration order + // per process, not per call), so run enough iterations that a + // reintroduced regression would be virtually certain to show up. + for i := range 40 { + cc := NewCacheChains() + require.NoError(t, Parse(dt, DescriptorProvider{}, cc)) + + storage, _, err := NewCacheKeyStorage(cc, nil) + require.NoError(t, err) + + found := false + require.NoError(t, storage.Walk(func(id string) error { + return storage.WalkBacklinks(id, func(_ string, link solver.CacheInfoLink) error { + if link.Selector.String() == wantSelector { + found = true + } + return nil + }) + })) + require.True(t, found, "iteration %d: link for the coincidentally-shared-digest dependency was dropped", i) + } +} diff --git a/cache/remotecache/v1/testdata/cyclic-merge-chain.json b/cache/remotecache/v1/testdata/cyclic-merge-chain.json new file mode 100644 index 000000000000..af3d7163589c --- /dev/null +++ b/cache/remotecache/v1/testdata/cyclic-merge-chain.json @@ -0,0 +1,475 @@ +{ + "layers": [ + { + "blob": "sha256:025fe1949698376d1d9a946f8a39a3529ad3ea540ca92b78c6cd041deb19d63e", + "parent": -1 + }, + { + "blob": "sha256:22f8e2e38443880a75812d458167e63e9e7a5e9aa54d1b992f5ec8207cfa4898", + "parent": -1 + }, + { + "blob": "sha256:47fb4e863644a6ed7938724aca10d7efe5069e18785be71ef47d6eb7bebebee2", + "parent": 17 + }, + { + "blob": "sha256:47fb4e863644a6ed7938724aca10d7efe5069e18785be71ef47d6eb7bebebee2", + "parent": 9 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 17 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 13 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 4 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 6 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 7 + }, + { + "blob": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1", + "parent": 10 + }, + { + "blob": "sha256:5a9c68ae5864046d46074c7a50154dfeb25bbc240f80a9a3736749f9a7331807", + "parent": 8 + }, + { + "blob": "sha256:8b405220e9cf785163b14ffabafeec5d8095edeebfd4ecd84425d45c6e534f5b", + "parent": 12 + }, + { + "blob": "sha256:8c39f78a6f3c4e14ae465f655a8ef1b95c0f0922582868843156886cde6a8ee9", + "parent": 15 + }, + { + "blob": "sha256:ac85ad54dfcbc4b61d3f3daf84a559f9d4876fab1388a143ddbe637881221b51", + "parent": 2 + }, + { + "blob": "sha256:ac85ad54dfcbc4b61d3f3daf84a559f9d4876fab1388a143ddbe637881221b51", + "parent": 3 + }, + { + "blob": "sha256:ad4584a948f1c64e28c66b03af60934d2f5947ada014f3f7c73346a80049f865" + }, + { + "blob": "sha256:cf012e9bbbcb8f4ae2975310ba334772049a4124fd74000ebb790a0304011b00", + "parent": 1 + }, + { + "blob": "sha256:d7c7f2fe3ff8e7aef157319987ae56a33daa0c84eac1db4708cea67f1e839e36", + "parent": 11 + } + ], + "records": [ + { + "layers": [ + { + "layer": 6, + "createdAt": "2026-08-17T17:56:17.020188114Z" + } + ], + "digest": "sha256:0eb34de390f5eb6a9cc007953fbb935ea0e1ad61d593a3d767a292e19f23c58f", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 6 + } + ] + ] + }, + { + "layers": [ + { + "layer": 8, + "createdAt": "2026-08-17T17:56:17.080596489Z" + } + ], + "digest": "sha256:173278230dd9ee6e6f0f4db035b2eb35ddc8c2f8620a4574bfc551f4c31161d7", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 13 + } + ] + ] + }, + { + "layers": [ + { + "layer": 10, + "createdAt": "2026-08-17T17:56:17.086699614Z" + } + ], + "digest": "sha256:1810804cfe31405f0f6217e5191a1051c16ee22cd2292d68ea6d06d47f64345e", + "inputs": [ + [ + { + "link": 1 + } + ], + [ + { + "link": 17 + } + ] + ] + }, + { + "layers": [ + { + "layer": 15, + "createdAt": "2026-08-17T17:56:16.891762281Z" + } + ], + "digest": "sha256:265923364bca6b8972d34e9a9c109c61d48b6556f2b2915e0e3f2a1dce560ba6", + "inputs": [ + [ + { + "link": 5 + }, + { + "link": 15 + } + ] + ] + }, + { + "layers": [ + { + "layer": 5, + "createdAt": "2026-08-17T17:56:17.184666739Z" + } + ], + "digest": "sha256:2b331d4ca7353ab966f76acbe1de91a288de8fd699c3ef7e01c60483d144053e", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 24 + } + ] + ] + }, + { + "digest": "sha256:4871ca2e89efeeea471eb003a501c2fc28dc39c23d238607b92365e32b70c9e3" + }, + { + "layers": [ + { + "layer": 4, + "createdAt": "2026-08-17T17:56:16.997419906Z" + } + ], + "digest": "sha256:53a80a1c2f4b549a01415884488a3a0ef176c50ac356b3cd53a6fc825cb80cd2", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 20 + } + ] + ] + }, + { + "layers": [ + { + "layer": 11, + "createdAt": "2026-08-17T17:56:16.946511406Z" + } + ], + "digest": "sha256:57a5ab45ee83a930675514d2528c657cf1d5e669475f33931afc0e7c848c0e34", + "inputs": [ + [ + { + "link": 14 + } + ] + ] + }, + { + "layers": [ + { + "layer": 11, + "createdAt": "2026-08-17T17:56:16.945978114Z" + } + ], + "digest": "sha256:57a5ab45ee83a930675514d2528c657cf1d5e669475f33931afc0e7c848c0e34", + "inputs": [ + [ + { + "link": 18 + } + ] + ] + }, + { + "layers": [ + { + "layer": 1, + "createdAt": "2026-08-17T17:56:17.135249739Z" + } + ], + "digest": "sha256:58c9526327ee7ce06f76ff8fe1686b841eb2a8b2d7a07ee6875363e079a68129", + "inputs": [ + [ + { + "link": 10 + } + ] + ] + }, + { + "digest": "sha256:60baf207481f4268aeea551a962d98ed980178bab76b4fd97a4c50127c493473" + }, + { + "layers": [ + { + "layer": 3, + "createdAt": "2026-08-17T17:56:17.125285531Z" + } + ], + "digest": "sha256:70d5a9485662b4470b0999fa70935bbbfd98286ebe666a8e43af4ce30f775c24", + "inputs": [ + [ + { + "link": 16 + } + ], + [ + { + "link": 10 + }, + { + "selector": "sha256:572318cf5b02ebf64e8c6c16f26be1c350fcd6f847e3eb43a3a5a08fc269b537", + "link": 22 + } + ] + ] + }, + { + "layers": [ + { + "layer": 2, + "createdAt": "2026-08-17T17:56:17.147109489Z" + } + ], + "digest": "sha256:70d5a9485662b4470b0999fa70935bbbfd98286ebe666a8e43af4ce30f775c24", + "inputs": [ + [ + { + "link": 21 + } + ], + [ + { + "link": 10 + }, + { + "selector": "sha256:572318cf5b02ebf64e8c6c16f26be1c350fcd6f847e3eb43a3a5a08fc269b537", + "link": 22 + } + ] + ] + }, + { + "layers": [ + { + "layer": 7, + "createdAt": "2026-08-17T17:56:17.059104781Z" + } + ], + "digest": "sha256:738d6ac3650a534ae6ce438fe53057915fb1266c973db450aa0f762c61af9632", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 0 + } + ] + ] + }, + { + "layers": [ + { + "layer": 12, + "createdAt": "2026-08-17T17:56:16.935372448Z" + } + ], + "digest": "sha256:7c2e6c252e3d3a600e2ed0225a35fd167b89e8215573d7f6fdfc3a2b59a346a3", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 3 + } + ] + ] + }, + { + "digest": "sha256:a341ffc7ae895238620ce1f01f35e83e185c8b19108ef64f3ae4e4a52be70776" + }, + { + "layers": [ + { + "layer": 9, + "createdAt": "2026-08-17T17:56:17.118762073Z" + } + ], + "digest": "sha256:ab5644a79c85abfa4535160a5199d432926a1bd713e0a35808e6fc91ad9a3f74", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 2 + } + ] + ] + }, + { + "digest": "sha256:be04964483196328799276d87cd2f625f75761854b48262e21d154cb793a5650" + }, + { + "layers": [ + { + "layer": 12, + "createdAt": "2026-08-17T17:56:16.934970573Z" + } + ], + "digest": "sha256:c5441e02039c67e53b9f7cfc99dcf5a75fcc35afae824f144cd6958ce143bece", + "inputs": [ + [ + { + "selector": "sha256:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1", + "link": 3 + } + ] + ] + }, + { + "digest": "sha256:c810abe27f85c286d00664ded1ddfb269d1ec248617c9c9f1e0d56aeec211d16" + }, + { + "layers": [ + { + "layer": 17, + "createdAt": "2026-08-17T17:56:16.954998406Z" + } + ], + "digest": "sha256:d3c0c91dd5b8cb2e3d496a11e72d2d669b829569d2266a6f431d12cbdc7f686b", + "inputs": [ + [ + { + "link": 7 + } + ], + [ + { + "link": 19 + } + ] + ] + }, + { + "layers": [ + { + "layer": 17, + "createdAt": "2026-08-17T17:56:16.954164281Z" + } + ], + "digest": "sha256:e70c9bd188804df2bff748e0c909e58531b1eb9f45319d3a1247f27509c01dbd", + "inputs": [ + [ + { + "link": 8 + } + ], + [ + { + "link": 19 + } + ] + ] + }, + { + "layers": [ + { + "layer": 16, + "createdAt": "2026-08-17T17:56:17.141378073Z" + } + ], + "digest": "sha256:fd7408b5d3d283e38a4830bb624e6fbdbf1f3ed190693c309c2af7fb0d2f2530", + "inputs": [ + [ + { + "link": 9 + } + ], + [ + { + "link": 10 + } + ] + ] + }, + { + "layers": [ + { + "layer": 14, + "createdAt": "2026-08-17T17:56:17.131387823Z" + } + ], + "digest": "sha256:fd7408b5d3d283e38a4830bb624e6fbdbf1f3ed190693c309c2af7fb0d2f2530", + "inputs": [ + [ + { + "link": 11 + } + ], + [ + { + "link": 10 + } + ] + ] + }, + { + "layers": [ + { + "layer": 13, + "createdAt": "2026-08-17T17:56:17.151492906Z" + } + ], + "digest": "sha256:fd7408b5d3d283e38a4830bb624e6fbdbf1f3ed190693c309c2af7fb0d2f2530", + "inputs": [ + [ + { + "link": 12 + } + ], + [ + { + "link": 10 + }, + { + "selector": "sha256:a7cb13d089672f15c02003beb688fb5642a1c62b68ffeee7c2ff29d26490deec", + "link": 22 + } + ] + ] + } + ] +} diff --git a/cache/remotecache/v1/utils.go b/cache/remotecache/v1/utils.go index 8e004d862569..3e04d78d3ceb 100644 --- a/cache/remotecache/v1/utils.go +++ b/cache/remotecache/v1/utils.go @@ -184,28 +184,35 @@ func marshalItem(ctx context.Context, it *item, state *marshalState) error { if _, ok := state.recordsByItem[it]; ok { return nil } - state.recordsByItem[it] = -1 - rec := cacheimporttypes.CacheRecord{ + // Reserve this item's record slot - and make it visible to other + // callers - before recursing into its own dependencies below, then + // mutate it in place from here on. Merge ops can coincidentally + // produce byte-identical content at more than one point in the same + // chain, which makes the dependency graph loop back on itself; if a + // reentrant call for the same item arrives while we're still walking + // its parents, it now gets this same, real (if not yet fully + // populated) record index back and can register its link to it, + // rather than getting a sentinel and silently dropping the link. + idx := len(state.records) + state.recordsByItem[it] = idx + state.records = append(state.records, cacheimporttypes.CacheRecord{ Digest: it.dgst, Inputs: make([][]cacheimporttypes.CacheInput, len(it.parents)), - } + }) for i, m := range it.parents { for l := range m { if err := marshalItem(ctx, l.src, state); err != nil { return err } - idx, ok := state.recordsByItem[l.src] + srcIdx, ok := state.recordsByItem[l.src] if !ok { return errors.Errorf("invalid source record: %v", l.src) } - if idx == -1 { - continue - } - rec.Inputs[i] = append(rec.Inputs[i], cacheimporttypes.CacheInput{ + state.records[idx].Inputs[i] = append(state.records[idx].Inputs[i], cacheimporttypes.CacheInput{ Selector: l.selector, - LinkIndex: idx, + LinkIndex: srcIdx, }) } } @@ -213,16 +220,14 @@ func marshalItem(ctx context.Context, it *item, state *marshalState) error { if res := it.bestResult(); res != nil { id := marshalRemote(ctx, res.Result, state) if id != "" { - idx, ok := state.chainsByID[id] + layerIdx, ok := state.chainsByID[id] if !ok { return errors.New("parent chainid not found") } - rec.Results = append(rec.Results, cacheimporttypes.CacheResult{LayerIndex: idx, CreatedAt: res.CreatedAt}) + state.records[idx].Results = append(state.records[idx].Results, cacheimporttypes.CacheResult{LayerIndex: layerIdx, CreatedAt: res.CreatedAt}) } } - state.recordsByItem[it] = len(state.records) - state.records = append(state.records, rec) return nil }