Fix QueryConditionCache OOB write on part-name/layout reuse (#2342)

[coreground]

Root-cause fix, validated: QueryConditionCache key missing mark layout

Following up on the crash trace package (708 rows across a 14-node production cluster) and the direct QueryConditionCache::write faults reported in #2342.

Mechanism

QueryConditionCache::Key was {table_id, part_name, condition_hash}. A part name can be reused after the underlying part's mark layout changes (exact lifecycle path not confirmed — see Open Items). write()'s cache.getOrSet(key, load_func) can then return a pre-existing Entry sized for the old layout, which the code proceeds to index using the current task's marks_count/mark_ranges:

std::fill(entry->matching_marks.begin() + mark_range.begin,
          entry->matching_marks.begin() + mark_range.end, false);
...
entry->matching_marks[marks_count - 1] = false;

On std::vector<bool>'s bit-packed storage, this is an out-of-bounds write, not a read.

Note: this branch was built starting from the actual v25.8.28.10001.altinitystable production tag, not a later revision — the Key fix below was not yet present there and had to be derived from scratch as part of this work, alongside the defense-in-depth hardening.

Fix

1. Key now includes marks_count and has_final_mark — two different layouts are structurally two different cache entries, so getOrSet() can no longer return a mismatched Entry.
2. The size-mismatch checks (write, shared-lock path; write, exclusive-lock path; read; plus final-mark/zero-marks and mark-range-bounds validation) are LOG_ERROR + skip rather than throw, so a residual mismatch degrades to "don't use the cache for this entry" instead of failing the user's query — with chassert retained alongside each, so debug/CI builds still catch a regression. A new QueryConditionCacheLayoutMismatch ProfileEvent surfaces if any of these ever fire.
3. FilterTransform's buffered-write flush comparison now also checks marks_count/has_final_mark (not just table_uuid/part_name) — this was a second, independent path where a differently-shaped part could otherwise be merged into a stale buffer before ever reaching write().
4. system.query_condition_cache now exposes marks_count/has_final_mark so entries that previously looked like indistinguishable duplicates are distinguishable.
5. gtest coverage for layout separation, invalid ranges, reused-part-name-with-different-layout, and concurrent writes.

Validation performed

• Write-path marks_count/has_final_mark derivation vs. read path — confirmed matching. All 4 real write() call sites (FilterTransform.cpp WHERE path, MergeTreeSelectProcessor.cpp PREWHERE path) derive these from the same live data_part->index_granularity object the read path (MergeTreeDataSelectExecutor.cpp) uses — not a stale copy. Confirmed by static tracing and by a live smoke test: QueryConditionCacheHits > 0 on repeated queries, confirming the fix doesn't silently zero out the cache hit rate.
• gtests: 4/4 pass, built and run against a real compiled binary.
• Full release build succeeded with this commit's own pinned toolchain (clang 19.1.7, rust nightly-2025-07-07) after fixing an unrelated, external issue (a stale LLVM apt signing-key hash that no longer matched what apt.llvm.org currently serves — a local build-environment fix, not part of this patch).
• A working Docker image was built and smoke-tested: boots, answers queries, cache-hit-rate/no-regression check passes; a best-effort attempt to reproduce the original part-name-reuse trigger via ALTER ... DELETE + ALTER ... UPDATE was inconclusive (no crash, but the mutation produced a new part name rather than reusing the original in place, so this likely didn't exercise the actual original trigger).
• This PR went through an internal final-review pass (11 findings fixed: a misplaced namespace ErrorCodes, two throws this branch had added that should have been log-and-skip like the rest, missing chasserts, a metric double-counting bug, the FilterTransform gap in item 3 above, stale test comments/duplicate test, file permissions, wording, and doc-comment updates).

Newly discovered issue — separate from this fix, reported for visibility

While verifying that the write path's cache-population exclusions mirror the read path's guard (!isFinal() && !mutations_snapshot->hasDataMutations()/hasPatchParts() && !vector_search), a real, independently-verified gap was found: the write path does not fully mirror the read path's guard.

• The two PREWHERE write call sites (MergeTreeSelectProcessor.cpp) have zero parity with the read-path guard — isFinal(), in-flight mutations/patch parts, and vector search are all unchecked before writing.
• The two WHERE write call sites (FilterTransform.cpp) correctly guard isFinal(), but not mutations/patch parts or vector search.

Consequence: a query the read path would refuse to consult the cache for can still populate a cache entry via these write paths, and a later "plain" query can read back results computed under different semantics — a silent wrong-results correctness bug, not a crash, and orthogonal to the Key fix above. This is a distinct issue from #2342 and is not fixed by this PR — flagging for a separate follow-up.

Still not resolved, flagged honestly

• Does not explain why prefer_localhost_replica = 0 independently reduced the crash rate ~19x before the cache was touched. Possible secondary factor: local-read concurrency increasing concurrent write() calls against the same Entry — out of scope for this PR.
• The exact part-lifecycle sequence that reuses a part name with a different mark layout is still not confirmed live. The Key fix makes this class of bug structurally impossible regardless of the exact trigger, but the trigger itself was not reproduced.

Confidence: high that this closes a concrete cache-owned OOB-write path. Not a claim this was the first or only corruption site in the original crash traces, nor that the prefer_localhost_replica question or the newly-discovered write-path exclusion gap are resolved.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn