Fix panic on concurrent index-header lazy reader usage and unload

[pracucci]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

The LazyBinaryReader can be unloaded at any time. When it gets unloaded, the values returned by its functions (eg. LabelValues()) are no more valid because they're yolo strings whose underlying buffer is released (and file unmmaped) on unload().

I would guess this not to be a real issue assuming a long enough idle timeout. For example, if the idle timeout is 20 minutes, the values returned by the LazyBinaryReader needs to be around for 20 minutes before it panics, which is currently not the case.

However, the combination of ReaderPool and LazyBinaryReader doesn't guarantee that unload() is always executed on idle readers. It may happen that a reader which is idle gets returned by ReaderPool.getIdleReaders() and gets used right before calling LazyBinaryReader.unload(), causing a panic like this (line numbers match with Thanos at commit 2f7d37d7368cce61ee028a27c2a4a21f9d8fe552):

unexpected fault address 0x7f3e16fe7905
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0x7f3e16fe7905 pc=0x4020ac]

goroutine 10902586 [running]:
runtime.throw(0x2533220, 0x5)
	/usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc0e3094f38 sp=0xc0e3094f08 pc=0x434de2
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:702 +0x3cc fp=0xc0e3094f68 sp=0xc0e3094f38 pc=0x44bb8c
cmpbody()
	/usr/local/go/src/internal/bytealg/compare_amd64.s:115 +0xec fp=0xc0e3094f70 sp=0xc0e3094f68 pc=0x4020ac
github.com/thanos-io/thanos/pkg/block/indexheader.BinaryReader.postingsOffset(0x2b73f00, 0xc1182fc5a0, 0xc114ce36d0, 0x2b44ce0, 0xc1182fc560, 0xc0b4452180, 0x0, 0xc0d7977140, 0xc10c1ba420, 0xc353b7c190, ...)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/block/indexheader/binary_reader.go:696 +0xe54 fp=0xc0e30950e8 sp=0xc0e3094f70 pc=0x18f5b94
github.com/thanos-io/thanos/pkg/block/indexheader.BinaryReader.PostingsOffset(0x2b73f00, 0xc1182fc5a0, 0xc114ce36d0, 0x2b44ce0, 0xc1182fc560, 0xc0b4452180, 0x0, 0xc0d7977140, 0xc10c1ba420, 0xc353b7c190, ...)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/block/indexheader/binary_reader.go:646 +0xbf fp=0xc0e30951c8 sp=0xc0e30950e8 pc=0x18f4b6f
github.com/thanos-io/thanos/pkg/block/indexheader.(*LazyBinaryReader).PostingsOffset(0xc00bf6d4a0, 0xc020951520, 0x15, 0x7f3e16fe7905, 0x14, 0x0, 0x0, 0x0, 0x0)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/block/indexheader/lazy_binary_reader.go:162 +0x1b3 fp=0xc0e3095318 sp=0xc0e30951c8 pc=0x18f7db3
github.com/thanos-io/thanos/pkg/store.(*bucketIndexReader).fetchPostings(0xc00b6e1c80, 0xc093896e00, 0x5, 0x8, 0x0, 0x0, 0x0, 0x0, 0x0)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/store/bucket.go:1783 +0xb01 fp=0xc0e3095578 sp=0xc0e3095318 pc=0x199db71
github.com/thanos-io/thanos/pkg/store.(*bucketIndexReader).ExpandedPostings(0xc00b6e1c80, 0xc00b9642e0, 0x2, 0x2, 0xc182bf6120, 0x253113a, 0x3, 0x2bb7920, 0x0)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/store/bucket.go:1601 +0x5ac fp=0xc0e3095818 sp=0xc0e3095578 pc=0x199b68c
github.com/thanos-io/thanos/pkg/store.blockSeries(0xc00933edb0, 0xc00b6e1c80, 0xc11d7a3f80, 0xc00b9642e0, 0x2, 0x2, 0xc11d7a2540, 0x2b44f80, 0xc02b22f530, 0x125fde00, ...)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/store/bucket.go:687 +0x6a fp=0xc0e3095d00 sp=0xc0e3095818 pc=0x19931ba
github.com/thanos-io/thanos/pkg/store.(*BucketStore).Series.func2(0x125ff840, 0x125ff8a0)
	/__w/cortex/cortex/vendor/github.com/thanos-io/thanos/pkg/store/bucket.go:938 +0xbe fp=0xc0e3095f78 sp=0xc0e3095d00 pc=0x19a373e
golang.org/x/sync/errgroup.(*Group).Go.func1(0xc02b22f4d0, 0xc0829de0e0)
	/__w/cortex/cortex/vendor/golang.org/x/sync/errgroup/errgroup.go:57 +0x59 fp=0xc0e3095fd0 sp=0xc0e3095f78 pc=0x997bc9
runtime.goexit()
	/usr/local/go/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc0e3095fd8 sp=0xc0e3095fd0 pc=0x467ca1
created by golang.org/x/sync/errgroup.(*Group).Go
	/__w/cortex/cortex/vendor/golang.org/x/sync/errgroup/errgroup.go:54 +0x66

This PR is not a definitive fix, in the sense that the values returned by LazyIndexHeader can potentially become invalid any time, but mitigates the issue fixing the race condition between ReaderPool and LazyBinaryReader guaranteeing that unload() is always executed on a idle reader.

Verification

Unit tests (but this specific race condition is hard to reproduce with a test).

[pstibrany]

Great job finding an explanation for this panic! It makes sense in retrospect, but is super tricky to spot.

yolo strings are dangerous :(

[pracucci]

Thanks @pstibrany for the quick review. I've addressed your comments.

[GiedriusS]

Looks good & the code seems clean. Great work 💪 However, it seems like quay is having problems? :s

[bwplotka]

Well spotted, thanks.

Only minor nits, not blocking.

[bwplotka]

I think this function is nice if it would be used more times than once, otherwise it might be just confusing. (It brings question why one method is mutex, another not).

Maybe inlining it would be better?

Not a blocker.

[pracucci]

Good point. Inlined.

[bwplotka]

e2e blocked on #3762

[pracucci]

Thanks a lot @bwplotka and @GiedriusS for your super quick review. I should have addressed your comments and rebased master after merging #3762.

[bwplotka]

LGTM!

Thanks 💪🏽