Skip to content

remotecache: fix silently dropped cache link - #7048

Open
mispencer wants to merge 1 commit into
moby:masterfrom
mispencer:fix/remotecache-cycle-link-loss
Open

remotecache: fix silently dropped cache link#7048
mispencer wants to merge 1 commit into
moby:masterfrom
mispencer:fix/remotecache-cycle-link-loss

Conversation

@mispencer

Copy link
Copy Markdown

Summary

addItemToStorage (import, cache/remotecache/v1/cachestorage.go) and
marshalItem (export, cache/remotecache/v1/utils.go) 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 whenever 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.

An item's final id (computed by computeIDs before addItemToStorage
runs) and a record's array index (known as soon as its slot in
state.records is reserved) are both available up front, before any
recursion. This PR allocates and registers 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.

Related issues

I believe this is the same underlying mechanism as #2279 ("Docker
BuildKit caching w/ --cache-from fails (roughly 50% rate)") — that issue's
own investigation (multi-platform builds where two platforms coincidentally
produce a byte-identical blob for one step) traced the corruption to
before the upload/export stage and found a layer graph with links missing
exactly where two branches reconverge on identical content, which matches
what I found here. I haven't reproduced #2279's specific multi-platform
trigger, so I'm not marking this as closing it, but I'd appreciate it if
anyone still hitting that issue could try this branch against their repro.

Likely related, same subsystem, same "identical content confuses cache
chain dedup" family: #1876, #2973, #3009, #3188, #2822, #2996, #2383.

Test plan

  • Added 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.
  • 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.
  • go build ./..., go vet ./cache/remotecache/v1/..., gofmt -s,
    and the full existing test suite (cache/remotecache/v1,
    solver) all pass.

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 <spencer.jones2@tylertech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant