remotecache: fix silently dropped cache link - #7053
Open
tonistiigi wants to merge 2 commits into
Open
Conversation
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>
The dropped link comes from a stale nil entry in the per-leaf visited map when a later traversal reuses an item already stored in the global memo. Remove the redundant traversal state and keep post-order registration. Restore marshalItem because export does not share this bug. Replace the captured fixture with an integration test that repeatedly imports byte-identical merge branches after pruning local results. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes and replaces #7048