[periklis]

Which issue(s) does the PR fix:

Ref: grafana/mimir#4541

Release notes for end users (ALL commits must be considered).

Reviewers should verify clarity and quality.

[BUGFIX] TSDB: Fix out-of-order chunk ID overflow by wrapping `firstOOOChunkID` modulo 2^23 instead of growing unbounded.

Summary

Fix unbounded growth of firstOOOChunkID in the OOO chunk path. When a series accumulates enough OOO chunk truncation cycles, the counter eventually overflows the 23-bit ID space, causing a panic in NewHeadChunkRef and query failures from index-out-of-range in oooChunk.

The fix applies modular arithmetic (bitmask wrapping) at the three sites that produce or consume OOO chunk IDs:

• oooHeadChunkID(): mask the packed position to 23 bits before setting the OOO flag
• oooChunk(): use modular subtraction (& (oooChunkIDMask - 1)) instead of signed subtraction to recover the slice index
• truncateChunksBefore(): wrap firstOOOChunkID with & (oooChunkIDMask - 1) after incrementing

Context

The existing guard in mmapCurrentOOOHeadChunk caps the number of active OOO chunks per series at oooChunkIDMask - 1, but it does not prevent firstOOOChunkID from growing without bound across repeated truncation cycles. Aliasing safety across the wrap boundary is guaranteed by the OOO isolation mechanism (oooIsolationState), which prevents truncation of chunks while a reader holds a reference.

A design document with full correctness reasoning, locking invariant diagrams, and the test plan is available here: https://gist.github.com/periklis/f6bbcdd9cdff32a8fbb36f8d09f412d8

[jesusvazquez]

I've given this code changes two passes on two distinct days and I think it could work. Im still a bit unsure about the consequences of having this in because the ids won't be monotonically increasing anymore and i think that increases complexity a bit on already complex code.

chunks.go:79 still says that the ids are monotonically increasing when they are not anymore if we merge this change.

I've pinged @codesome for review to see if he has any concerns.

[codesome]

Took a look at the main changes and the modulo arithmetic makes sense and LGTM. Haven't thoroughly checked the unit tests.

[periklis]

Took a look at the main changes and the modulo arithmetic makes sense and LGTM. Haven't thoroughly checked the unit tests.

Alright, I resolved the merge conflict in the test file.

[roidelapluie]

I have reviewed this, LGTM.

[bboreham]

Link an issue which was inspired by wanting to detect the root misconfiguration more easily.

• Idea: max rate of out-of-order samples in a single series #17165