Pre-PR Validation Gate (session: cron:clickhouse-ci-task-worker:20260415-231500)

a) Deterministic repro?
This is a startup race condition in ReplicatedMergeTree that depends on specific ZK state (covered parts with stale entries). It fires sporadically during stress tests (14+ hits in 30 days) and cannot be triggered deterministically from a test script — the same limitation that applied to prior fixes PR #96164, #96705, and #100652. The "reproduction" is the 14+ CI hits across unrelated PRs and master confirming this is a real, persistent trunk bug.

b) Root cause explained?
During startup, createLogEntriesToFetchBrokenParts() calls paranoidCheckForCoveredPartsInZooKeeperOnStart(). The function checks for ZK parts that are covered by another ZK part but don't exist on disk and aren't in parts_to_fetch. However, there's a legitimate case: after a merge creates covering part C, the covered part P is cleaned from disk by clearOldPartsAndRemoveFromZK, but P's ZK entry persists (ZK cleanup lags behind disk cleanup or server was restarted between the two). P is covered locally by C, so getActiveContainingPart(P) returns C, meaning P is NOT in parts_to_fetch. But P also doesn't exist on disk. The function doesn't check for local active covering parts, so chassert(false) fires — a false positive, since the data IS safe in C.

c) Fix matches root cause?
Yes. Adding getActiveContainingPart(part_name) != nullptr check directly addresses the missing case. The assertion is preserved for genuine data loss (no local covering part, no disk copy, not being fetched).

d) Test intent preserved?
Yes. The paranoid check still catches genuine scenarios where a covered ZK part has no local copy anywhere. The fix only suppresses the assertion when the data IS provably safe (covered by a local active part).

e) Both directions demonstrated?
For startup race conditions, the "without fix" direction is demonstrated by 14+ CI crashes in 30 days. The "with fix" direction will be validated by CI stress tests on this PR. The code analysis was verified by an agent that confirmed: const-safety, no deadlock risk, correct scope (non-startup version doesn't need fixing).

f) Fix is general, not a narrow patch?
Yes. The fix is inside paranoidCheckForCoveredPartsInZooKeeperOnStart itself, so it covers ALL call sites (both checkPartsImpl and createLogEntriesToFetchBrokenParts). The non-startup version (paranoidCheckForCoveredPartsInZooKeeper, static function at line ~2757) was verified to NOT need this fix — it only queries ZK and never accesses local data_parts.

cc @tavplubix @alesapin — could you review this? It fixes a false-positive chassert(false) in paranoidCheckForCoveredPartsInZooKeeperOnStart that crashes debug/sanitizer builds during stress tests (14+ hits in 30 days, STID 2508-5dc3 and 2508-6644). The fix adds a getActiveContainingPart() check so the assertion doesn't fire when a local active part already covers the stale ZK entry — a legitimate post-merge state that prior fixes (#96164, #96705, #100652) didn't handle.

Workflow [PR], commit [221f150]

Summary: ❌

AI Review

Summary

This PR fixes a debug/sanitizer-only false-positive assertion in paranoidCheckForCoveredPartsInZooKeeperOnStart by treating a locally present covering ZooKeeper part as a valid state when the covered part entry is stale in ZooKeeper. The change is targeted, keeps the assertion for genuine missing-data scenarios, and looks correct.

ClickHouse Rules

Final Verdict

• Status: ✅ Approve

The data is preserved in the local covering part

@groeneai, this is not true. Since the local part is not in zk, this part is going to be removed as unexpected

@groeneai, continue on this PR according to the reviewer feedback.

@tavplubix thanks for the pushback — you're right that a local part not in ZooKeeper would be detached as unexpected. Re-reading the path I had to convince myself that wasn't what getActiveContainingPart could return; it isn't, but the dependency was implicit. I've pushed 221f150 which avoids the question entirely by checking the ZK covering part on disk directly, instead of any local-only part.

Why the original getActiveContainingPart check was nonetheless safe

getActiveContainingPart searches data_parts_by_state_and_info (MergeTreeData.cpp:5914-5938) — that index is populated only from parts loaded as expected. Parts on disk that are not in expected_parts_on_this_replica (built from replica_path/parts in StorageReplicatedMergeTree.cpp:507-549) are routed to a separate unexpected_data_parts vector at MergeTreeData.cpp:2330-2333, loaded asynchronously by loadUnexpectedDataParts, and detached as ignored later in checkPartsImpl (StorageReplicatedMergeTree.cpp:2047). They never enter data_parts_by_state_and_info, so getActiveContainingPart cannot return one. The "removed as unexpected" failure mode you described therefore couldn't apply to a part returned by that call.

Why I switched anyway

That argument is valid but indirect — it depends on a non-obvious invariant (load-time classification, separate collections, async unexpected loading, detach in checkPartsImpl). The new check is direct: covering_part is a ZooKeeper part by construction (it comes from active_set built from parts_in_zk), so checking whether covering_part exists on disk is exactly the question "is the data preserved in the covering part?" — no reasoning about local-only parts needed.

Updated check (StorageReplicatedMergeTree.cpp:1829-1853)

/// Check if the ZooKeeper covering part itself exists on disk locally.
/// If it does, the covered part's data is preserved in the covering part:
/// a merge or mutation produced `covering_part`, then `part_name` was cleaned
/// from disk by `clearOldPartsAndRemoveFromZK`, but its ZK entry has not
/// been cleaned up yet (ZK cleanup can lag behind disk cleanup, or the
/// server was restarted between the two). The stale ZK entry will be
/// removed by the cleanup thread after startup.
if (!found)
{
    for (const DiskPtr & disk : disks)
    {
        if (disk->existsDirectory(fs::path(path) / covering_part))
        {
            found = true;
            break;
        }
    }
}

Scenario walk-through

After a merge in clearOldPartsAndRemoveFromZK:

Old logic: part_name not on disk, not in parts_to_fetch (because getActiveContainingPart(part_name) returns the local copy of covering_part, so it's excluded from the fetch list at the call site) → chassert(false) fires.

New logic: same as above, then we additionally check whether covering_part exists on disk. It does, because the merge result is still there — so we take the legitimate-state branch, log a WARNING, and skip the chassert.

True data loss path is unchanged: if the covering part is genuinely missing on disk, found stays false and the chassert fires.

Pre-PR validation gate (session: cron:clickhouse-ci-task-worker:20260425-161500)

a) Deterministic repro? Same startup race as before (14+ CI hits in 30 days, STIDs 2508-5dc3 / 2508-6644). No new local repro for this iteration; the change is a strict tightening of the guard.
b) Root cause explained? Yes — the chassert did not consider the case where the ZK covering part exists on disk. parts_to_fetch cannot reach it because the call site excludes covered parts whose covering part is locally present (see ReplicatedMergeTreeQueue.cpp:243-246 and StorageReplicatedMergeTree.cpp:1888-1891).
c) Fix matches root cause? Yes — directly checks the missing case using the ZK covering part name.
d) Test intent preserved? Yes — the assertion still fires for genuine data loss (no part_name on disk, no covering_part on disk, not in parts_to_fetch).
e) Both directions demonstrated? Without the fix: 14+ CI master hits over 30 days. With the fix: CI on this PR.
f) Fix is general, not a narrow patch? Yes — fix is inside paranoidCheckForCoveredPartsInZooKeeperOnStart itself, applying to both call sites (checkPartsImpl and createLogEntriesToFetchBrokenParts).

LLVM Coverage Report

Changed lines: 16.67% (3/18) | lost baseline coverage: 26 line(s) · Uncovered code

Full report · Diff report

Superseded by @alexey-milovidov's PR #104516, merged 2026-05-12 03:19 UTC. His approach is shorter and more idiomatic — uses getActiveContainingPart directly in paranoidCheckForCoveredPartsInZooKeeperOnStart rather than my flag-based approach. Same false-positive (STIDs 2508-5dc3 / 2508-6644) is now fixed on master. Closing this PR as the duplicate. — Groene AI (session: cron:clickhouse-github-inbox-v2:20260512-0330)