📋 PR Summary

Title: fix(tsdb): chunk overflow on ooo query PR Link: prometheus/prometheus#18692 Author: krajorama Branch: krajo/fix-chunk-overlap-query → main

What This PR Does

This PR fixes a potential silent data corruption issue where TSDB chunks could overflow their 16-bit sample counter (exceeding 65,535 samples). This could happen during queries that merge out-of-order (OOO) and in-order samples if a single output chunk spans many OOO samples. The PR updates the querier to cut new chunks before they exceed size/sample limits, and adds defensive panics to chunk appenders to catch any other potential overflows.

Key Changes

• Updated tsdb/querier.go to proactively cut new chunks during iterable population if the current chunk exceeds byte limits (which guards against sample count overflow for supported encodings).
• Added explicit checks for math.MaxUint16 sample count in xor, xor2, histogram, and floathistogram appenders, panicking with "chunk capacity exceeded" to prevent silent wrapping of the uint16 counter.
• Added comprehensive unit tests verifying the overflow panics and a new integration test TestChunkQuerier_OverlappingInOrderAndOOOChunks that exercises the OOO query chunk splitting logic.

Key Discussions

None. PR has no comments yet.

📖 Review Guidelines

Based on the type of changes in this PR, reviewers should focus on:

Area	Priority	What to Check
TSDB Chunk Encoding	🔴 High	Ensure chunk appenders correctly identify capacity limits and avoid silent corruption.
Querier Logic	🔴 High	Verify that populateChunksFromIterable correctly splits chunks before they hit appender limits without introducing performance regressions or incorrect chunk ranges.
Test Coverage	🟡 Medium	Confirm edge cases (exceeding 65k samples) are realistically simulated and tested.

---

🚨 Issues Found

Issues are sorted by severity (Critical → High → Medium → Low → Nitpick).

🟢 Low (Nice to Have)

Consider Error Return vs Panic for Histogram Appenders

• File with exact line number: tsdb/chunkenc/histogram.go:L762
• Link to a file line on GitHub PR: https://github.com/prometheus/prometheus/pull/18692/files#diff-da47fac849fR762
• Problem: The AppendHistogram method signature supports returning an error, and historically uses appendOnly=false to signal that it should cut a new chunk if the current one cannot accommodate the sample (e.g. schema change). Panicking on full capacity prevents callers who pass appendOnly=false from gracefully receiving a new chunk.
• Impact: Minimal in current codebase since querier.go now preemptively cuts chunks, and standard head appenders cut on byte limits. However, it makes the Appender interface slightly less robust for generic usage where a caller might expect AppendHistogram to handle chunk rotation on full capacity when appendOnly=false.
• Existing Discussions: None.
• Suggestion: For Histogram and FloatHistogram appenders, consider returning an error (or handling recoding/new chunk creation if appendOnly=false) instead of panicking, similar to how schema changes are handled. For XOR appenders, panic remains necessary as Append has no error return.

---

🧪 Test Coverage Analysis

Test Changes in This PR

Test File	Type	Coverage
tsdb/chunkenc/chunk_test.go	unit	Added generic testChunkOverFlowPanics helper.
tsdb/chunkenc/*_test.go	unit	Added specific tests invoking the overflow helper for each encoding.
tsdb/querier_test.go	integration	Added TestChunkQuerier_OverlappingInOrderAndOOOChunks verifying correct multi-chunk output when merging >65k OOO samples.

Coverage Assessment

• New code tested: Yes - Explicit tests added for the new panic conditions and the querier splitting logic.
• Edge cases covered: Yes - Tests specifically push sample counts beyond math.MaxUint16.
• Regression tests: N/A - Fixes a latent bug rather than a regression.

Recommended Additional Tests

None. Coverage is excellent and targets the exact overflow scenario.

❓ Questions & Uncertainties

Things I'm not certain about and would like clarification on:

1. Panic vs Graceful Chunk Rotation in Histogram Appenders
   • Context: I noticed AppendHistogram panics on math.MaxUint16 even if called with appendOnly=false.
   • My assumption: The author intended this as a strict safety net for unexpected usage, relying on upper layers (querier.go) to manage chunk sizes. This is acceptable but differs slightly from how other limits (schema changes) are handled in the same method.

---

✅ Review Verdict

Aspect	Status	Notes
Code Quality	🟢	Clean, focused changes. Good reuse of constants.
Security	🟢	Prevents potential denial of service / data corruption from malformed chunks.
Performance	🟢	Preemptive size checks in querier loop are cheap.
Test Coverage	🟢	Thorough testing of the overflow conditions.
Documentation	🟢	Clear comments explaining the constants and test scenarios.
Overall: APPROVE