Workflow [PR], commit [0fe8607]

Summary: ✅

AI Review

Summary

This PR fixes a startup false-positive in StorageReplicatedMergeTree by treating a covered ZooKeeper part as satisfied when a different local active covering part already exists, and adds a targeted stateless repro test. I did not find correctness, safety, or compatibility issues in the current diff.

ClickHouse Rules

Final Verdict

• Status: ✅ Approve

LLVM Coverage Report

Changed lines: 37.50% (3/8) | lost baseline coverage: 9 line(s) · Uncovered code

Full report · Diff report

@groeneai, review this PR.

@alexey-milovidov — review below.

TL;DR

Fix is correct, test is good. This addresses the same false-positive in paranoidCheckForCoveredPartsInZooKeeperOnStart as my open PR #102864 (STIDs 2508-5dc3 / 2508-6644). Your approach is shorter and more idiomatic than mine; happy to close #102864 in favor of this PR.

Correctness of the source change

The added check on a no-found part_name:

if (!found)
{
    auto local_covering_part = getActiveContainingPart(part_name);
    if (local_covering_part && local_covering_part->name != part_name)
        found = true;
}

is sound. The non-obvious invariant it relies on is getActiveContainingPart cannot return an unexpected (not-in-ZK) local part during startup. I traced this carefully for #102864:

• MergeTreeData::getActiveContainingPart (src/Storages/MergeTree/MergeTreeData.cpp:5944-5968) walks only data_parts_by_state_and_info filtered by DataPartState::Active.
• During loadDataParts, parts on disk are partitioned into disk_parts (in expected_parts, sourced from replica_path/parts in ZK) and unexpected_disk_parts (MergeTreeData.cpp:2351-2354).
• Only disk_parts enter the PartLoadingTree and data_parts_indexes (MergeTreeData.cpp:2373). unexpected_disk_parts are loaded asynchronously into the separate unexpected_data_parts collection (MergeTreeData.cpp:2497-2517) and detached as ignored later in checkPartsImpl. They never enter data_parts_by_state_and_info.

So a covering part returned by getActiveContainingPart(part_name) is necessarily a part that was in ZK at load time, i.e., a legitimate covering part whose data is preserved.

This is exactly what was discussed on #102864 — @tavplubix raised a concern that "the local part is not in zk, this part is going to be removed as unexpected", and after tracing the load path I found the concern doesn't apply (unexpected parts never reach the Active index that getActiveContainingPart queries). I switched #102864 to a check on covering_part directly (the ZK covering part name, taken from active_set) to avoid the indirect reasoning, but the getActiveContainingPart form here is equivalent and shorter.

A small benefit of getActiveContainingPart: it transparently finds a deeper local covering part if active_set.getContainingPart happened to return a stale intermediate covering name. The disk-existence check on covering_part only succeeds when that exact ZK covering part is on disk; getActiveContainingPart succeeds for any local active ancestor.

Test 04215_replicated_missing_covered_part_on_start.sh

Reproduces the bug class from issue #101070 with a clean, deterministic recipe:

1. Two replicas, system stop merges rmt2 so only rmt1 produces the covering part all_0_1_1.
2. rm -rf the active part directory on rmt1 to force the missing-on-disk state.
3. First detach/attach plus a fetch-impossible state — fine.
4. insert (3), restart rmt2 merges, sync, second optimize final — produces a fresh covering part on rmt1. The ZK entry for all_0_1_1 survives as a stale node (verified by the system.zookeeper query).
5. Second detach/attach — this is where the paranoid check would have hit chassert(false) before the fix: all_0_1_1 is in ZK, covered by another ZK part, missing on disk, and not in parts_to_fetch because the local active covering part already covers it.

Final result 3\t6 (1+2+3 == 6) is correct.

old_parts_lifetime=100500 is a nice touch — keeps outdated parts alive across the restart. The substring('$path', 1, 1) != '/' guard before rm -rf is good safety, matching 02369_lost_part_intersecting_merges.

The pre-existing test 02369_lost_part_intersecting_merges exercises the broader covered-part-missing-on-disk path; the new check is strictly additive (only fires after the existing on-disk and parts_to_fetch checks both miss), so 02369 should be unaffected.

Minor notes (non-blocking)

• The function still has the existing pre-fix log line "...but doesn't exist on any disk" whose wording is slightly stale once we’ve also taken getActiveContainingPart into account — but the message only fires on the failure path, where part_name, covering_part, and any local containing part are all genuinely missing, so it’s still accurate.
• 02369 uses CURDIR; the new 04215 uses CUR_DIR. Pure cosmetics, no fix needed.

CI status

Currently 143 / 161 success, 18 skipped, 0 failures.

Conclusion

LGTM. I’ll close PR #102864 once this is merged.