fix(querier): serve sharded windows whose shards are at mixed compaction levels#5349
Open
1linkovdim wants to merge 1 commit into
Open
fix(querier): serve sharded windows whose shards are at mixed compaction levels#53491linkovdim wants to merge 1 commit into
1linkovdim wants to merge 1 commit into
Conversation
Contributor
Author
|
Moving back to draft until I decide how to address the pointed out issue by the bugbot. |
1linkovdim
marked this pull request as draft
July 11, 2026 02:03
1linkovdim
marked this pull request as ready for review
July 11, 2026 06:08
1linkovdim
force-pushed
the
fix/querier-mixed-level-shard-prune
branch
from
July 13, 2026 20:53
6497363 to
0a6b318
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0a6b318. Configure here.
…ion levels
pruneIncompleteShardedBlocks grouped blocks by {level, minTime} and dropped
any group missing a shard. But the N shards of a window can legitimately sit
at different compaction levels: when late-arriving source blocks re-merge
only the shards their data touches (e.g. with stacktracePartition splitting,
where a small increment hashes to a subset of shards), those shards advance a
level while the rest lag. Keying completeness on level then split one intact
window into several sub-groups, each missing shards, and dropped all of them
- silently returning zero results for a fully-intact window.
Check shard completeness per sharding (shard count) and per time instant
instead, ignoring compaction level:
- For each distinct window start, require every shard to have a trusted
block covering that instant. A window whose shards span L3/L4 is complete;
pruneSupersededBlocks then collapses the per-shard level duplicates.
- Coverage (minTime <= t < maxTime), not an exact minTime match, so a shard
merged to a wider span still counts for the later windows it overlaps and
does not orphan sibling shards' blocks there (which would under-count).
- Different shard counts (e.g. a split-and-merge-shards reconfiguration) are
distinct shardings, checked independently.
Two subtleties that would otherwise reintroduce a silent under-count:
- Only blocks at/above the deduplication level count. Intermediate L2 split
blocks are never served (pruneSupersededBlocks always drops them for the
L1 ancestor), so letting one satisfy a shard would make an incomplete set
look whole and under-count once the L2 block is dropped.
- pruneIncompleteShardedBlocks still runs BEFORE pruneSupersededBlocks, so an
incomplete sharded set is dropped while its lower-level ancestors still
exist and can serve as a query-time fallback.
If more than one sharding (shard count) survives into the plan, they partition
the same series differently, so force query-time deduplication rather than
serving both directly and double-counting.
Also log a WARN when the guard prunes a genuinely incomplete window, so this
can no longer silently hide data.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1linkovdim
force-pushed
the
fix/querier-mixed-level-shard-prune
branch
from
July 14, 2026 01:35
0a6b318 to
8eb7142
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Fixes a query-time false-negative that silently returned zero results for a fully-intact time window whose sharded blocks happened to sit at different compaction levels.
pruneIncompleteShardedBlocks(pkg/querier/replication.go) grouped blocks by{compaction level, minTime}and dropped every block in any group missing a shard. But the N shards of one window can legitimately be at different compaction levels: when late-arriving source blocks re-merge only the shards their data touches (notably withcompaction_split_by: stacktracePartition, where a small increment hashes to a subset of shards), those shards advance a level while the rest lag. Keying completeness onlevelthen split one complete window into multiple sub-groups — each missing shards — and pruned all of them. With no lower-level fallback present, the query returns nothing, with no error and no log.Fix
Check shard completeness per sharding (shard count) and per time instant, ignoring compaction level:
t, require every shard to have a trusted block coveringt(minTime ≤ t < maxTime). A window whose shards span e.g. L3/L4 is now seen as complete;pruneSupersededBlocksthen collapses the per-shard level duplicates (an L3 and its L4 descendant) down to one block per shard.minTimematch. A shard merged to a wider span still counts for the later windows it overlaps, so sibling shards' blocks there are not orphaned and silently pruned. (Mixed-span case from review — see below.)2_of_4and a2_of_8block cover different hash ranges (e.g. after asplit-and-merge-shardsreconfiguration), so they can't be pooled. This also removes the oldshard length mismatcherror, which is now structurally impossible.Subtleties, each of which would otherwise reintroduce a silent miscount
pruneSupersededBlocksalways drops them in favour of the deduplicated L1 ancestor — so letting an L2 block satisfy a shard would make an incomplete set look whole and under-count once the L2 is dropped.pruneIncompleteShardedBlocksruns beforepruneSupersededBlocks. An incomplete sharded set is dropped while its lower-level ancestors still exist, so those ancestors survive as a query-time fallback. (A kept sharded block would otherwise supersede its shared, un-sharded ancestor — the very fallback the missing shards need.)_of_2and a complete_of_4), they partition the same series differently, so a series can appear in both. The sharded fast path assumes each series is served by exactly one block, which only holds for a single complete sharding — so we setDeduplication=trueto reconcile the overlap instead of double-counting. (Normally a window has one sharding, and when a new sharding is derived from an old onepruneSupersededBlockscollapses to one, so the fast path is preserved.)A genuinely incomplete window (a shard absent at every trusted level) is still pruned — and now emits a
WARNlog, so this can never again silently hide data.Mixed-span note (from review)
An earlier revision keyed completeness on
{minTime, shardCount}(exact start match). That under-counted a transient mixed-span window — one shard merged to a wide block while a sibling still had separate blocks in the tail — by orphaning the tail blocks. The per-instant coverage check above resolves it. It's a self-healing transient (a compaction cycle merges the lagging shard and converges), but coverage handles it correctly rather than serving a partial result.Scope
This is the v1 read path only (
pkg/querier). v2's read path (pkg/querybackend+ metastore-drivenqueryplan) has no analogous completeness guard and is unaffected.A related, separate limitation remains and is not addressed here: a genuinely low-cardinality window (some shards legitimately empty, so the compactor omits them per
compact.go) is also treated as incomplete and falls back to L1. The querier can't distinguish "empty shard" from "missing shard" without compactor-side metadata; that's a follow-up.Why this layer (vs. lockstep-promoting shards in the compactor)
Compaction level means "how many merge generations / has this been deduplicated," not "is this window complete." Shards are independent partitions with no reason to share a generation count. The bug was the read path assuming
level ≈ completeness; the fix removes that assumption where it lives. Forcing lockstep levels on the write path (e.g. emitting empty/placeholder blocks) would reshape the write path to satisfy a read-path invariant that shouldn't exist, at ongoing storage/I-O cost, and would remain racy. Thefingerprintsplit mode already yields lockstep levels implicitly if that property is desired.Testing
go test ./pkg/querier/passes. Cases inTest_replicasPerBlockID_blockPlan:keep sharded window with shards at mixed compaction levels— the incident. Verified it fails pre-fix (empty plan) and passes after. AssertsDeduplication=false(complete set served directly).keep mixed-span shards when a wide block covers a later window— the mixed-span review case: a wide block + sibling tail blocks; all kept, no orphaning.collapse L3+L4 duplicate of a shard within a mixed-level window— a shard with both its L3 and derived L4 present collapses to the L4.prune sharded window with a shard missing at all levels— genuinely-missing shard still pruned, falls back to the non-sharded block (Deduplication=true).serve complete old sharding when a shard-count change leaves a partial new one— distinct shardings counted separately; verified it fails against upstream (shard-length-mismatch empties the plan).deduplicate when two complete shardings overlap— two complete, unrelated shardings both kept withDeduplication=true(no double-count).single sharding after superseding keeps the fast path— a derived_of_4supersedes the_of_2; one sharding remains,Deduplication=false(guard doesn't over-trigger).do not let an intermediate L2 block satisfy shard completeness— a mid-merge window must not serve a shard alone off an about-to-be-dropped L2 block.Test_pruneIncompleteShardedBlocks_logging— a WARN is emitted when a genuinely incomplete window is pruned, and not for a healthy mixed-level window. The pre-existingignore_incomplete_shardscase still passes (fallback preserved by the prune-before-supersede ordering).🤖 Generated with Claude Code