Fix race on chunks multilevel cache + Optimize to avoid refetching already found keys.

[alanprot]

What this PR does:

We can have a race when we partially fetch data on the multilevel cache. The reason for that is internally, the object store changes the dic returned by the cache and this races with the cache internal implementation.

Ex: https://github.com/thanos-io/thanos/blob/d6d19c568f8dc6005a9184d3a5a994da4f0d8c75/pkg/store/cache/caching_bucket.go#L469

Right now this PR only have the unit test demonstrating the issue.

PS: I also optimized the cache to not fetch keys that were already found on the previous cache implementation.

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• [NA] Documentation added
• [NA] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[alanprot]

Test failing before the fix (first commit)

==================
WARNING: DATA RACE
Read at 0x00c0010bc9f0 by goroutine 1663:
  runtime.mapiterinit()
      /usr/local/go/src/runtime/map.go:877 +0x0
  github.com/thanos-io/thanos/pkg/cache.(*InMemoryCache).Store()
      /__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/cache/inmemory.go:294 +0x73
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*multiLevelChunkCache).Fetch.func1.1()
      /__w/cortex/cortex/pkg/storage/tsdb/multilevel_chunk_cache.go:137 +0xc1
  github.com/thanos-io/thanos/pkg/cacheutil.(*AsyncOperationProcessor).asyncQueueProcessLoop()
      /__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/cacheutil/async_op.go:55 +0xc1
  github.com/thanos-io/thanos/pkg/cacheutil.NewAsyncOperationProcessor.gowrap1()
      /__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/cacheutil/async_op.go:36 +0x33

Previous write at 0x00c0010bc9f0 by goroutine 1651:
  runtime.mapdelete_faststr()
      /usr/local/go/src/runtime/map_faststr.go:321 +0x0
  github.com/cortexproject/cortex/pkg/storage/tsdb.Test_MultiLevelChunkCacheFetchRace()
      /__w/cortex/cortex/pkg/storage/tsdb/multilevel_chunk_cache_test.go:106 +0xa56
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x44

Goroutine 1663 (running) created at:
  github.com/thanos-io/thanos/pkg/cacheutil.NewAsyncOperationProcessor()
      /__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/cacheutil/async_op.go:36 +0x13e
  github.com/cortexproject/cortex/pkg/storage/tsdb.newMultiLevelChunkCache()
      /__w/cortex/cortex/pkg/storage/tsdb/multilevel_chunk_cache.go:63 +0x90
  github.com/cortexproject/cortex/pkg/storage/tsdb.Test_MultiLevelChunkCacheFetchRace()
      /__w/cortex/cortex/pkg/storage/tsdb/multilevel_chunk_cache_test.go:99 +0x89b
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x44

Goroutine 1651 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1743 +0x825
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:2168 +0x85
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.runTests()
      /usr/local/go/src/testing/testing.go:2166 +0x8be
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:2034 +0xf17
  main.main()
      _testmain.go:91 +0x164
==================
--- FAIL: Test_MultiLevelChunkCacheFetchRace (0.00s)
    testing.go:[139](https://github.com/cortexproject/cortex/actions/runs/11677372741/job/32515160421#step:6:140)9: race detected during execution of test

[yeya24]

Makes sense. Thanks a lot for fixing this issue!

[yeya24]

How can we do better to catch this before? Maybe we shouldn't use mock in our existing test

[alanprot]

How can we do better to catch this before? Maybe we shouldn't use mock in our existing test

I think the only way is integ tests.. cause the root cause was how the results of the cache interact with the overall code … but catching that would need a very specific test I guess .. idk if we will ever be able to test all combinations of configs and edge cases tbh :(