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 }