[jranson]

Description

This PR fixes nested aggregations over TSM ALBs (#1107, #1115) in two ways, as descirbed below

Merge-mode deduplication

Merge-mode ALBs now count label sets returned by several members once for count or group over an inner count or group. This fix is automatic.

TSM Global Evaluation (prometheus backends only)

This feature requires configuration. For exact answers to any PromQL, a TSM ALB can now set tsm.evaluation: global: it evaluates each instant and range query once, in an embedded Prometheus engine, over every member's raw series. Members serve raw series over Prometheus remote read (the new mimir flavor, or series_api on any Prometheus backend) or gRPC StoreAPI (the thanos flavor). Replicas are deduplicated on replica_labels, with counter-aware penalty deduplication or chain. By default, a failed member yields a partial result with a warning. Limits (max_samples, max_fetched_series, max_fetched_samples, the query timeout and concurrency) always fail the query. Complete results are cached whole, keyed by the pool's membership and the request's identity. A global Prometheus ALB can mix-and-match flavors: some can be thanos, others mimir, and others plain prometheus, and they will merge correctly.

Beneath the results, members' raw series are delta-cached in two-hour blocks per member, selector and identity, so each evaluation reads from members only what the cache lacks. Samples within a member's volatile window are always read from the member: 5 minutes for mimir and thanos members, unless the ALB sets volatile_window. So are samples older than tsm.raw_cache_window (31 days by default). Concurrent evaluations share reads, data is charged against limits as it arrives, and chunks are validated before they're kept. Small chunks left by frequent refreshes are compacted, and a narrower selector can be answered from a broader selector's cached blocks. TSM ALBs default max_object_size_bytes to 64 MB.

Bugs fixed along the way

This change also restores the use_cache_chunking setting, ignored since #851, and fixes byte-range chunking defects. It adds local compose profiles for global evaluation, a Mimir seeder, a live integration test, metrics and docs.

Type of Change

• • Bug fix
• • New feature
• • Optimization
• • Test coverage
• • Documentation
• • Infrastructure

AI Disclosure

• • This contribution DOES NOT include AI-generated changes
• • This contribution DOES include AI-generated changes, and I have reviewed the relevant contributing guidelines.

[houyuwushang]

Thanks for this PR — the core machinery is unusually well engineered (the presence/quantile/variance finalizers verified line-by-line against the upstream engine, the delta-cache block math, and the concurrency handling are all solid), and the restored use_cache_chunking wiring is a real fix. However, I found several issues I'd like addressed before merge, listed by severity. All of the Highs have been verified against the PR code (and, where relevant, against prometheus/prometheus v0.315.0 as an oracle).

High

1. rankGroupKey does not drop __name__ from without grouping — tsm_rank_finalize.go:303-308

PromQL without always removes __name__ — and the PR's own aggregationGroupingTags (tsm_variance_finalize.go:226) gets this right — but rankGroupKey only deletes the listed grouping labels:

if grouping.Without {
    kept := tags.Clone()
    for _, label := range grouping.Labels {
        delete(kept, label)
    }
    return kept.JSON()
}

Since raw-selector fanout series carry __name__, topk/bottomk/limitk without (...) partitions groups per metric name and keeps up to k series per metric instead of per group. Reproduced: for x{job=a,instance=i1}=1, y{job=b,instance=i1}=9, x{job=a,instance=i2}=5, y{job=b,instance=i2}=2, topk without (job) (1, {__name__=~"x|y"}) returns 4 series under the finalizer vs 2 in real Prometheus. Same for limitk without. The PR's tests don't cover without over multi-metric selectors — please add engine-oracle coverage for this case.

2. A corrupted cached chunk can crash the process, repeatedly — rawcache.go:663/694 + rawblock.go:329-410

The cache envelope (writeEnvelope) carries no integrity check (magic+version+extents+payload), and newChunk (series.go:47-55) validates only the 2-3 byte chunk header, so a corrupt histogram chunk passes decode — and the vendored Prometheus engine panics on invalid histogram data (tsdb/chunkenc/histogram.go:973: panic(err)). The read path is protected by safego (queryable.go:113-119), but the store path is not: flightGroup.do spawns go g.run(...) where after(f.result) → store → mergeBlock/compact re-iterates previously cached chunks (rawblock.go:346, :400) with no recover. With a persistent cache backend (redis/file/bbolt), one corrupted entry makes every refresh cycle panic in a bare goroutine and kill the whole process, with nothing marking the entry bad. Suggested fixes: add a checksum to the envelope, and wrap the after callback in safego so it meets the same bar as the read path.

3. Slice preallocation from untrusted protobuf counts — remoteread.go:225-226

iterables := make([]chunkenc.Iterable, len(cs.Chunks))
spans := make([]span, len(cs.Chunks))

Each Chunks entry costs >=2 wire bytes, so a 50 MB frame (within maxFrameBytes and the default 256 MB max_capture_bytes) can declare ~25M chunks -> ~600 MB allocated before validation or budget charging, on top of the prompb.Chunk structs Unmarshal itself materializes (~56 B per 2-byte entry). A compromised member can OOM the proxy while honoring every advertised limit. Same class in decodeSamples (unbounded TimeSeries entries within the byte limit). Please bound these counts (against the budget) before unmarshal/preallocation.

Medium

1. max_fetched_series degrades to per-read scope when the raw cache is on — rawcache.go:345-348, 582, 597, 164. Cache-path charges only samples to the evaluation budget (add(0, ...)), counting series post-merge, so with N members a configured 10k per-evaluation limit can admit ~Nx10k pre-dedup series. This contradicts the documented "bounds the raw series one evaluation reads from all its members".
2. In-process result buffering is uncapped — tsm_global_cache.go:364-402. inProcessTransport buffers the entire evaluation response with no size limit, bypassing max_capture_bytes; only max_samples (default 50M) bounds it. Several concurrent heavy range-query misses (default concurrency 20) is an OOM vector.
3. Remote-read truncations are cached as complete coverage — remoteread.go:190-193. A clean close mid-answer is indistinguishable from end-of-stream (no protocol marker), and storeBlock marks the entire requested gap covered, so the missing half is never re-read within the block's TTL — silent permanent data loss for the member. The StoreAPI path is warning-gated; this path isn't. Even a sanity check (did the last series' chunk span reach maxt?) would narrow it.
4. StoreAPI: a series in the same message as a warning is silently dropped — storeapi.go:196-199 (if warning != "" { ...; continue } discards series decoded from the same message). A malicious/buggy store can hide series with no error, and their samples are never charged to the budget.
5. tsm: block validation/behavior asymmetry — alb/options/options.go Validate only calls TSMOptions.Validate() under mechanism: tsm, but UsesCache() reads the block regardless of mechanism. A residual tsm: {evaluation: global} on an rr ALB silently requires a resolvable cache (new startup failures for configs that work today), and a typo like evaluation: gloabl errors on tsm ALBs but passes silently elsewhere. Validation and behavior should agree.
6. Discovery members skip global-eval eligibility validation — validateGlobalEvaluationMember runs only for static pools (client.go:348-356); instantiateMember (dynamic/manager.go:384-409) has no equivalent, and discovery templates can be influxdb/clickhouse. Static pools fail fast at startup; dynamic ones 502 per query with no config-time hint.
7. max_fetched_series doesn't count out-of-range series — remoteread.go:249-251 / storeapi.go:211-214 charge only series with samples inside [mint,maxt]; a member returning unlimited all-out-of-range series trips neither limit while every series is decoded and retained.

Low (summary)

• max_object_size_bytes (64 MB TSM default) is not applied to raw blocks — rawcache.go:104-109 builds nativedelta.Config without MaxObjectSize, unlike every other delta engine.
• Fetch-limit failures can be misclassified as timeout (503 instead of the documented 422) when the deadline fires concurrently — rawcache.go:195-197.
• StoreAPI.Close() and RawCache.Wait() have no callers — backend reconfiguration leaks gRPC connections / never drains in-flight reads.
• limitedReader can turn truncation-at-limit into clean EOF — remoteread.go:347-354.
• Unsorted member sample lists are silently interleaved/dropped rather than errored — remoteread.go:326-339 + series.go:250-263.
• limitk selects the first k in JSON-serialization order, not Prometheus's label order — diverges for label values containing quotes/backslashes/control chars (tsm_limitk_finalize.go:53-72).
• Evaluator construction failure on the first provider has no fallback and is retried per request — mech/tsm/global.go:46-67.
• partial_response defaulting to true matches Thanos but deserves a prominent changelog note, since a dead member now yields incomplete results with only a warning.

Verified fine (no action needed)

For the record, things I specifically checked and found correct: the CodeQL reflected-XSS alerts are false positives (appendJSONString fast path only passes printable ASCII without quotes/angle-brackets/ampersand/control/non-ASCII characters, everything else goes through json.Marshal); the remote-read frame-buffer reuse is safe (gogo-generated prompb Unmarshal copies bytes/string fields — verified in v0.315.0); decompression bombs are covered (snappy DecodedLen pre-check, gRPC recv-size limits); limits are race-free (atomic.Int64 totals); partial results are forced no-store; pool-membership changes correctly rotate the cache namespace; no cross-tenant cache sharing; quantile/variance/presence/dedup math matches the upstream engine.

The Highs are all small, contained fixes — happy to re-review once they land.

[houyuwushang]

Follow-up with reproduction results: I verified the main findings empirically against this branch by writing and running tests against the actual code paths (not just reading). All of the Highs and Mediums #3/#4/#7 reproduced. Two corrections to my earlier comment are noted below.

H1 (topk/bottomk/limitk without) — REPRODUCED

Direct unit test: rankGroupKey(without (job)) on x{job=a,instance=i1} returns {"__name__":"x","instance":"i1"} — __name__ retained — while aggregationGroupingTags correctly returns {"instance":"i1"} for the same input. End-to-end through FinalizeTSMMerge: topk(1) without (job) over x{job=a,instance=i1}=1, y{job=b,instance=i1}=9, x{job=a,instance=i2}=5, y{job=b,instance=i2}=2 keeps 4 series (all of them); the correct answer is 2. The by (instance) control path behaves correctly.

H2 (corrupted cached chunk kills the process) — REPRODUCED end-to-end

Confirmed the full chain empirically: a corrupt histogram chunk body (valid 2-byte header, garbage spans — schema in 9..52 passes IsKnownSchema but fails the forced ReduceResolution(8)) passes newChunk and decodes; seeding it into a raw-cache block and then merging a valid adjacent chunk into that block kills the test binary with exactly the claimed stack:

panic: span number 2 with offset -1: histogram has a span whose offset is negative
chunkenc.(*histogramIterator).AtHistogram  histogram.go:973
global.startsWithReset  rawblock.go:403
global.runEnd   rawblock.go:315
global.compact  rawblock.go:286
global.mergeBlock rawblock.go:267
global.(*RawCache).storeBlock  rawcache.go:409
global.(*RawCache).store       rawcache.go:373
global.(*RawCache).fetch.func2 rawcache.go:326
global.(*flightGroup[...]).run rawcache.go:694
created by ...(*flightGroup[...]).do ... rawcache.go:663

i.e. the panic happens on the bare store-path goroutine with no recover, exactly as described. One correction to my earlier comment: the nativedelta envelope itself has no checksum, but blob-backed cache providers (bbolt/filesystem/redis) do add an outer CRC32 to their entries (pkg/cache/blob/header PayloadCRC), so ordinary disk corruption is caught on retrieve for those backends. The crash vector remains real for: in-memory caches, entries corrupted after retrieval (e.g. bugs, or large payloads ≥ the 10 MiB DeferredVerifyLen, which are verified after serving), and — most importantly — member-served malformed histogram chunks, which are cached verbatim in the first place and then re-iterated by the store path. The safego wrap on the store path is the robust fix regardless of backend.

H3 (untrusted-count preallocation) — REPRODUCED, and it's worse than I stated

Measured with runtime.ReadMemStats against the real decodeChunked:

• A 10 MB frame declaring 5M chunks (2 wire bytes each) forces ~2.17 GB of allocations (217x): ~2.0 GB from res.Unmarshal alone (~400 B per declared chunk), plus the make([]chunkenc.Iterable, ...) / make([]span, ...) preallocations. The frame is under maxFrameBytes (50 MB) and the 256 MB capture limit; no error occurs until after all allocations, and the failure is a decode error, not a limit error.
• Even when the budget does reject the fetch, it does so too late: 2M valid chunks (42 MB frame) with MaxSamples: 1 burn ~1.02 GB before ErrFetchLimit fires at budget.add (after the chunk loop).

All advertised limits are byte-count-based; nothing bounds entry count before unmarshal. I'd suggest bounding the declared chunk/series counts against the budget before unmarshal.

M3 (remote-read truncation cached as complete) — REPRODUCED

A member serving one frame covering [lo, lo+2m] of a requested [lo, lo+10m] and closing cleanly: the read returns no error and no warnings; the stored block's extent is the entire requested range [lo, hi]; a second read of the missing range is served from cache with zero additional member requests, returning an empty success. Silent data loss for the block's TTL, as described.

M4 (StoreAPI warning drops same-message series) — REPRODUCED

Through the real bufconn gRPC path: a message carrying both a series and a warning returns 1 of the expected 2 series, err == nil, and the dropped series' samples are never charged to the budget. Caveat for the fix's priority: stock Thanos encodes Result as a oneof so a spec-compliant member never emits both — this needs a hand-rolled or malicious peer. Still worth handling the series when a warning is present rather than continue-ing past it.

M7 (out-of-range series bypass budget) — REPRODUCED

Two series whose samples all fall outside [mint,maxt] return successfully under a MaxSeries: 1 budget, charging 0 series / 0 samples; an in-range control series is charged normally.

Summary

Everything actionable in the earlier review stands; the two corrections are H3's magnitude (measured ~217x, worse than the ~12-40x I estimated) and H2's scope (blob backends have an outer CRC; the panic vector is real for in-memory caches, deferred-verify large payloads, and member-served malformed chunks in general). I'm happy to share the repro test files if useful.

[jranson]

@houyuwushang i've pushed up fixes for these findings, thanks! Each fix is in a separate commit for verification.

[houyuwushang]

Rechecked at eff96115. The fix commits address the prior Highs and most of the Mediums. One correctness issue remains.

Remote-read partial coverage is still cached as complete

RemoteRead.Select returns no warning when a member sends a complete HTTP response whose payload covers only part of the requested range. RawCache.storeBlock then marks the whole requested lo..hi as covered.

I reproduced this with a temporary regression built on the existing limitTestFrames fixture: serving only the first frame with a correct Content-Length made the first RawCache.Select succeed, and a second Select for the same range was served entirely from cache with no additional member request. This reproduced under both the memory and bytes cache providers. StoreAPI is not affected because a warning suppresses the store; remote-read has no equivalent completeness signal.

Suggested fix: derive the stored block extents from the actual chunk/sample spans the member returned, not from the requested range, or add a completeness signal/heuristic before marking the range covered. Transport truncation at the byte limit is already handled by limitedReader.

Nonblocking note

StoreAPI.Select sets uncompressed after its uncompressed retry fails with any non-Unimplemented error. A transient failure on that retry therefore permanently disables snappy for the source. If that is not intentional, only set the flag when the uncompressed retry succeeds.

[jranson]

@houyuwushang there is no defect on on the remote-read coverage. If we treated every gap as volatile, then every legitimate gap would be refetched on every request, even when the gap is correct/permanent. Your reproduction does not consider that there might be legitimate gaps in a sparse response. For the purpose of late-arriving data and avoiding the caching of temporary gaps, Trickster provides the volatile_window setting to allow recent time ranges that might be incomplete some time to settle before they become cacheable.

For the compression finding, I've made the error check more conservative to ensure only the Unimplemented error disables compression.

[houyuwushang]

Agreed. I rechecked the generated prompb types: ReadResponse, QueryResult, and ChunkedReadResponse carry only results, with no completeness/coverage metadata. A response that legitimately has no samples in part of the requested range is therefore indistinguishable from one that omitted them.

Deriving cache extents from actual chunk/sample spans would not fix that generally; it would turn every legitimate sparse gap into a miss and refetch it on every request. Transport truncation is already handled by Content-Length/limitedReader, and recent late-arriving data is what volatile_window is for. I agree there is no general defect in the current remote-read coverage semantics.

A stronger guarantee would require a protocol extension or a member-specific opt-in completeness signal; it cannot be inferred from a standard remote-read response.