Session id: cron:clickhouse-worker-slot-1:20260626-015400

cc @tavplubix @CheSema — could you review this? It stops ReplicatedMergeTreeSink::commitPart from aborting the server with a LOGICAL_ERROR ("already written by concurrent request") when the ZooKeeper block-number counter is reset under a surviving local part (Keeper metadata loss / replica re-creation / DROP-race); the INSERT now fails with a regular DUPLICATE_DATA_PART and the conflicting part is enqueued for a check. @tavplubix, this is the same class you diagnosed on #42120 as a "race with DROP".

Workflow [PR], commit [de9d136]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR changes ReplicatedMergeTreeSink::commitPart so a final-name collision after allocating a new replicated block number is handled as a recoverable replica inconsistency: the conflicting part is queued for checking and the insert fails with DUPLICATE_DATA_PART instead of rethrowing LOGICAL_ERROR. The regression test now rewinds the ZooKeeper counter to the surviving part's actual block number and disables insert Keeper fault injection on the two inserts that must be deterministic. I did not find correctness issues requiring new inline review comments.

Missing context / blind spots

• ⚠️ There is no local build* directory in this checkout, so I did not run 04411_replicated_insert_block_number_counter_reset locally. At review time, the Praktika report had no failed checks, but several GitHub/Praktika jobs were still pending.

Final Verdict

• Status: ✅ Approve
• No code changes requested from this review; wait for the pending CI jobs before merge.

Fixed the Style check failure: the regression test built the ReplicatedMergeTree zookeeper path through a shell variable, so the check requiring $CLICKHOUSE_TEST_ZOOKEEPER_PREFIX on the ReplicatedMergeTree(...) line did not match. The prefix is now inlined on the CREATE line (parallel-safe), and the keeper-client commands derive the path from system.replicas.zookeeper_path so there is one source of truth. No C++ change; the fix and the reset-counter repro are unchanged.

Fixed the flaky check failure on the test this PR adds (amd_msan/amd_tsan, 4+1 FAILs).

Root cause (from the failing run's part_log): the test inserted six parts expecting block numbers 0..5, reset the ZK counter, and relied on the next insert re-issuing block 0 to collide with the surviving all_0_0_0. Under CI load that was racy:

• a background merge fired during the six inserts (all_1_5_1 merging blocks 1..5), and
• block-number allocation was not guaranteed to start at 0 — in the failing iteration the six setup inserts produced all_1_1_0..all_6_6_0, so block 0 was never occupied by a surviving part. After the reset the 7th insert got block 0, hit no collision, and succeeded → NO_EXPECTED_ERROR, 7 rows.

Fix (test-only): insert a single row (deterministically all_0_0_0) with SYSTEM STOP MERGES so the part keeps block 0 and can't be merged away, assert the surviving part name as a precondition guard, then reset and re-insert. The forced collision is now deterministic.

Verified locally on a debug build:

• crashes the pre-fix server (LOGICAL_ERROR at ReplicatedMergeTreeSink::commitPart), and
• passes 150/150 --flaky-check iterations with randomized settings on the fixed binary.

The source fix and its intent (must crash before, must fail with DUPLICATE_DATA_PART after) are unchanged.

Made the regression test reliable. The flaky-check failures (NO_EXPECTED_ERROR, ~1/29) came from the counter reset itself: keeper-client can transiently fail under parallel load, the single-shot rmr+touch silently did not take effect (errors went to /dev/null), so the next INSERT got block 1 and did not collide. Now the reset retries until system.zookeeper confirms block_numbers/all cversion is back at 0 before the colliding INSERT. The block number is derived from that cversion, so the collision is deterministic; the assertion is unchanged (DUPLICATE_DATA_PART + server stays alive). Validated against a locally rebuilt unfixed binary (test FAILS, server aborts) and the fixed binary (180/180 runs pass with randomization on).

Pushed ec42119 correcting the test fix. The previous commit's root-cause was incomplete.

Corrected root cause (proven deterministically locally): the single setup INSERT is NOT guaranteed to produce all_0_0_0. Block-number allocation on a fresh ReplicatedMergeTree does not always start at 0 under load, so ~3% of runs the surviving part lands on block 1 (all_1_1_0). The previous fix reset the counter to 0 (verifying cversion == 0), so the colliding INSERT got block 0, which does NOT name-collide with all_1_1_0 → NO_EXPECTED_ERROR / 2 rows. CIDB confirms it: the failing runs' first output line itself diffed all_0_0_0 → all_1_1_0, with max_insert_threads 1 (so this is not an insert-thread split). Verifying cversion == 0 could never fix this — when the survivor is at block 1 you need the counter at 1, not 0.

New fix: read the surviving part's actual min_block_number B, then advance the recreated counter to B (cversion == B) so the next allocation re-issues exactly B and collides regardless of where the setup INSERT landed. The cversion-verify retry loop (for transient keeper-client failures) is kept. The non-deterministic part-name print is dropped from the output (it was the diffing line); the test still asserts DUPLICATE_DATA_PART + server-alive + no data loss, and still exercises the commitPart fix (server log shows the new DUPLICATE_DATA_PART path + enqueuePartForCheck for both all_0_0_0 and all_1_1_0 survivors).

Validated on a local debug build (external keeper, thread fuzzer on): deterministic block-0 and forced-block-1 scenarios both collide; 50/50 runs pass with full randomization.

Session id: cron:clickhouse-worker-slot-0:20260626-181100

The flaky check kept recurring (last on ec421199) because the earlier fixes targeted the wrong cause. Found and proved the real one.

Root cause: stateless tests run with insert_keeper_fault_injection_probability=0.01 by default (tests/config/users.d/insert_keeper_retries.xml, installed unconditionally). A simulated Keeper fault during an INSERT's block-number allocation triggers a retry, and the retry allocates a higher block number (the number = the parent node's sequential counter, bumped on every child-create). That breaks the forced collision two ways: a fault on the setup INSERT moves the surviving part off its expected block (the all_1_1_0 survivor seen in failing reports), and a fault on the colliding INSERT skips it past the surviving block. Either way no exact-name collision -> the INSERT succeeds -> NO_EXPECTED_ERROR + extra row.

The previous "background allocator consumes block 0" theory was refuted locally: with fault injection off, the setup INSERT lands on block 0 in 40/40 runs.

Fix (test-only, source unchanged): pin insert_keeper_fault_injection_probability=0 on both INSERTs. This targets exactly the responsible setting (not a blanket no-random-settings); the surviving part's block is still read and the counter aligned to it, so the collision holds even if that block is non-zero. .reference unchanged.

Session id: cron:clickhouse-worker-slot-3:20260626-213900

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 8/8 (100.00%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

CI finish ledger - de9d136

Every failure below has an owner: a fixing PR (ours or external). Only CH Inc sync is exempt. None are PR-caused: our new test 04411_replicated_insert_block_number_counter_reset is GREEN on this head (the insert_keeper_fault_injection_probability=0 pin held); the Stress failures are pre-existing chronic trunk crashes, and this branch was forked at 474a3c12c67 and predates the merged refresh-scheduling fix.

Session id: cron:our-pr-ci-monitor:20260627-023000