[cache] Fix descriptor duplication in getAvailableBlobs

[Fricounet]

The walkBlob function was called with a function having a side effect: appending a descriptor to a list. However, the function could be called multiple times for the same descriptor (once in walkBlob and once in walkBlobVariantsOnly in that case) leading to duplicate entries in the final list. Instead of the list, we can use a map to store the descriptors. With the digest as key, it will ensure we don't store duplicates.

This fixes one of the problem encountered in #6088

cc @ktock

[ktock]

Thanks.
Please squash the commits.

[Fricounet]

@ktock done, thanks for the review 🙇

[Fricounet]

@ktock any idea why the tests would be failing? I'm don't really understand the errors + it was not failing before I sqashed 🤔

[ktock]

TestGetRemotes failure doesn't look like caused by this change (posted a possible fix #6099). I'm rerunning the test.

https://github.com/moby/buildkit/actions/runs/16344069969/job/46175402958?pr=6092#step:8:4328

=== FAIL: cache TestGetRemotes (1.05s)
    manager_test.go:1783: 
        	Error Trace:	/src/cache/manager_test.go:1783
        	            				/src/vendor/golang.org/x/sync/errgroup/errgroup.go:130
        	            				/usr/local/go/src/runtime/asm_amd64.s:1700
        	Error:      	An error is expected but got nil.
        	Test:       	TestGetRemotes

[Fricounet]

Another failure but it also looks unrelated

[tonistiigi]

If I look at the implementation of walkBlob it is calling walkBlobVariantsOnly with visited map to avoid duplications. Isn't that one working correctly, and if it is not then probably should fix it there.

[Fricounet]

@tonistiigi it doesn't work as expected because:
in a case with 2 compressions (gzip and zstd) where the gzip image was built first, then the zstd one and you decide to rebuild + export cache for the gzip image:

• f is called once in walkBlob for the gzip layer
• on the first iteration of walkBlobVariantOnly, f is called once for the zstd layer here
• then walkBlobVariantOnly is called again with the zstd layer (which will have the gzip layer in its content store info because it was built after the initial gzip image) which means that f will be called a third time for the gzip layer there because the visited check only occur after

I thought about fixing by moving the visited check to about here however this would mean that walkBlobVariantOnly never calls f for the inital blob which is a breaking change and It means we need to consider the consequences for all the cases where f is different when calling walkBlobVariantOnly and if it has, or not, side effects.
So I felt that just making sure that there can't be duplicates in the end result of the function used to call walkBlob (by using a map) is safer as it means we don't have to care about how many times the function gets called.
What do you think?

[Fricounet]

@tonistiigi do you mind giving me your updated opinion based on my message above?

[tonistiigi]

The current patch seems to make output order potentially undeterministic. Could this be a problem?

[Fricounet]

@tonistiigi that's a good point. I tried to track all the usages of descs in the calling functions but it branches out fast and I can't guarantee that nondeterministic ordering won't ever be an issue.
I've modified the code to revert to using a slice and do a basic for loop on it to ensure deduplication (which is think is fine performance wise, see updated comment in 622757e)
What do you think?

[tonistiigi]

This can be done with https://pkg.go.dev/slices#ContainsFunc in a slightly cleaner way in the new Go.

[Fricounet]

addressed in 7526d59

[tonistiigi]

Please squash commits as well

[Fricounet]

commits squashed @tonistiigi

[Fricounet]

tests seem to have irrelevant failures again 😓 can they be retried?