Reclaim stale empty-part directory instead of LOGICAL_ERROR

[groeneai]

Related: #82473

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a LOGICAL_ERROR ("New empty part is about to materialize but the directory already exist") that could abort the server in debug and sanitizer builds when a DROP/DETACH/MOVE/REPLACE PARTITION on a MergeTree table ran after a previous such operation was interrupted (for example a rolled-back transaction or a crash) and left a stale tmp_empty_<part> directory behind. The stale directory is now reclaimed instead of failing.

Description

The covering empty part created by MergeTreeData::createEmptyPart() (for DROP/DETACH/MOVE/REPLACE PARTITION on a non-replicated MergeTree) is written under a tmp_empty_<part> directory and renamed to its persistent name only later, via the operation's transaction (rename_in_transaction = true).

If the operation is interrupted after the empty part reaches the PreActive state (so is_temp is already false) but before the deferred rename runs, and the transaction then rolls back, the physical tmp_empty_<part> directory is left on disk. The rolled-back part is no longer treated as temporary, so its directory is reclaimed only later by the temporary-directory sweeper (temporary_directories_lifetime, one day by default). In CI this was triggered on plain_rewritable object storage by a fault-injected metadata commit (Cannot schedule a task: fault injected ... While committing metadata operation) during a stress run.

A subsequent covering operation on the same still-visible source part derives the same name and createEmptyPart() found the leftover directory. The old code aborted the server with a LOGICAL_ERROR; the assumption stated in its comment ("New part have to capture its name, therefore there is no concurrency in directory creation") does not hold for such stale leftovers.

The in-memory temporary-parts holder acquired just above (getTemporaryPartDirectoryHolder, which throws if any live operation already holds the name) already guarantees no concurrent operation owns this name, so an existing directory can only be a stale leftover and is safe to remove. The fix reclaims it (logs a warning and removeRecursive) before beginTransaction(), mirroring the regular INSERT path in MergeTreeDataWriter::writeTempPartImpl, which already reclaims a stale temporary directory instead of failing.

A regression test (04000_empty_part_stale_tmp_dir) reproduces the original crash deterministically using a new create_empty_part_inject_stale_dir failpoint that injects the stale directory.

Found by the AST fuzzer under the Stress test (arm_ubsan) job (STID 3202-2fe9). First seen on master at commit dd89f87010a4d59d6278b02d3030683e29940c9f. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=dd89f87010a4d59d6278b02d3030683e29940c9f&name_0=MasterCI&name_1=Stress%20test%20%28arm_ubsan%29

Version info

• Merged into: 26.7.1.893 (included in 26.7 and later)

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. New ONCE failpoint create_empty_part_inject_stale_dir pre-creates the tmp_empty_<part> directory right before the exists() check in createEmptyPart(), deterministically reproducing the original abort: with the failpoint armed, ALTER TABLE ... DROP PARTITION 2 aborts the server (Exit 134) with the exact CI message New empty part is about to materialize but the directory already exist ... 2_3_3_1 ... tmp_empty_2_3_3_1/ via StorageMergeTree::dropPartition. The failpoint storage-independently models the real CI trigger (a fault-injected plain_rewritable metadata commit rolling back a deferred-rename PreActive empty part).
b	Root cause explained?	Yes. A covering empty part (DROP/DETACH/MOVE/REPLACE PARTITION on a non-replicated MergeTree) is written under tmp_empty_<part> and renamed to its persistent name only later via the operation's transaction (rename_in_transaction = true). If the op is interrupted after the part reaches PreActive (so is_temp is already false) but before the deferred rename runs, and the transaction rolls back, the physical tmp_empty_<part> dir is orphaned — IMergeTreeDataPart::removeIfNeeded early-returns because is_temp == false, so the dir is reclaimed only later by the temporary-directory sweeper (temporary_directories_lifetime, 1 day default). A later covering op on the same still-visible source part derives the same name; createEmptyPart() found the leftover dir and aborted with a LOGICAL_ERROR based on the false invariant in its comment ("there is no concurrency in directory creation").
c	Fix matches root cause?	Yes. The fix targets the false invariant at the single chokepoint all covering-empty-part creators funnel through (createEmptyPart). A tmp_empty_<part> directory is by definition temporary, and stale temp dirs are an EXPECTED, recoverable condition everywhere else in the codebase (deleted at startup; reclaimed by the INSERT path). The fatal LOGICAL_ERROR was the anomaly. Reclaiming the stale dir (warn + removeRecursive) makes this path consistent with that established handling. (Alternative considered: clean up the orphan at Transaction rollback time. Rejected as both narrower — it only covers the rollback source, not crash/OOM/kill — and riskier, since it touches the shared deferred-rename rollback path used by all engines and part types. Happy to revisit if reviewers prefer that direction.)
d	Test intent preserved / new tests added?	New regression test 04000_empty_part_stale_tmp_dir.sh added. It creates parts, arms the failpoint to inject the stale dir, runs DROP PARTITION, and asserts the partition is gone (0), surviving rows remain (40), and the server is healthy. No existing test was weakened.
e	Both directions demonstrated?	Yes. BASELINE (reclaim reverted to the original throw) + failpoint → server SIGABRT (Exit 134), exact LOGICAL_ERROR reproducing STID 3202-2fe9. FIXED + failpoint → DROP succeeds, server alive, 0 LOGICAL_ERROR; clickhouse-test --test-runs 30 --no-random-settings = 30/30 PASS. Also confirmed via an isolated manual repro (a bare orphan tmp_empty_ dir with no ghost part: crashes baseline, succeeds on fix).
f	Fix is general across code paths?	Yes — the fix is at the single createEmptyPart() chokepoint, so it covers every covering-empty-part producer: StorageMergeTree::dropPartition, movePartitionToTable/replacePartitionFrom source side, MutateTask::replacePartWithEmpty (TTL), and the delete-only mutation path. It is the root-cause location, not a per-caller symptom guard, and it handles orphans from any interruption source (rollback, crash, OOM, kill), not just the one rollback path that CI hit.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	N/A for data inputs — the bug is a directory-name collision independent of column types, values, or wrappers. Generality across the SQL operations that create covering empty parts is covered by (f).
h	Backward compatible?	Yes. No setting, on-disk, wire, or replication format change. The only behavior change is that a previously-fatal recoverable condition (a stale temp dir) is now recovered from. There is no "old behavior" worth preserving (the old behavior was a buggy server abort), so no gate/SettingsChangesHistory entry is needed.
i	Invariants and contracts preserved?	Yes. The safety of removeRecursive() rests on the in-memory tmp_dir_holder acquired just above via getTemporaryPartDirectoryHolder: TemporaryParts::add() throws LOGICAL_ERROR "already added" if any live operation already holds this name, so reaching the exists() check means no concurrent op owns the directory — it can only be a stale leftover, making removal safe. The removal is placed BEFORE beginTransaction() so it is not staged into the part's own (not-yet-started) write transaction. ClickHouse is single-process per data dir, so the in-memory set is authoritative.

Session id: cron:clickhouse-worker-slot-0:20260630-021400

[groeneai]

cc @CurtizJ @alesapin — could you review this? It fixes a LOGICAL_ERROR server abort in MergeTreeData::createEmptyPart() when a covering-empty-part operation (DROP/DETACH/MOVE/REPLACE PARTITION) finds a stale tmp_empty_<part> directory left behind by a previously-interrupted such operation (deferred-rename PreActive part rolled back, or a crash). The directory is now reclaimed instead of aborting, mirroring the INSERT path; safe because the in-memory tmp_dir_holder proves no live operation owns the name. Found by the AST fuzzer (STID 3202-2fe9, first seen on master).

[clickhouse-gh]

Workflow [PR], commit [dec95c9]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR makes MergeTreeData::createEmptyPart reclaim stale tmp_empty_<part> directories instead of throwing LOGICAL_ERROR, and it updates the MutateTask empty-part paths so the temporary-directory holder remains consistent with that reclaim logic. I reviewed the current diff and the existing discussion on the PR head dec95c9, including both resolved inline threads, and I did not find any remaining correctness, safety, or coverage issues that warrant a new review comment.

Final Verdict

• Status: ✅ Approve

[groeneai]

CI finish ledger — 90792cd

Every failure below has an owner: a fixing PR (ours or external). Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stress test (amd_tsan) / Hung check failed, possible deadlock found	deadlock (chronic trunk; 109 distinct PRs + 76 master hits in 24h, not caused by this PR)	#101680 / #105905 (ours, open)
Stress test (arm_tsan) / Hung check failed, possible deadlock found	deadlock (chronic trunk; same family)	#101680 / #105905 (ours, open)
Stress test (arm_asan_ubsan) / Hung check failed, possible deadlock found	deadlock (chronic trunk; same family)	#101680 / #105905 (ours, open)
PR (rollup)	aggregates the stress sub-checks above	#101680 / #105905 (ours, open)
CH Inc sync	-	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260630-073000

[PedroTadim]

@groeneai merge with master

[alexey-milovidov]

@groeneai, merge with master

[groeneai]

Merged master @ 2ed2e36. Rebuilt clean; the fix (4 files: FailPoint.cpp, MergeTreeData.cpp, test) is unchanged.

[groeneai]

CI finish ledger — 350a696

CI fully finished on this head (Finish Workflow pass, Mergeable Check pass, PR aggregator pass). No test failures. The earlier arm_tidy duplicate-include failure was on head 2ed2e36d and is resolved on this head.

Check / test	Reason	Owner / fixing PR
(no failures)	—	—
Sync	—	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260713-093000

[groeneai]

@alexey-milovidov merged master @ 5aa0f8d (base 7b7de46). Also addressed the follow-up bot review on the TTL-to-empty path: in replacePartWithEmpty the old tmp_mut_<part> part is now dropped while its holder in ctx->temporary_directory_lock is still alive, and the new tmp_empty_<part> holder is installed only after, preserving lock-before-cleanup. Rebuilt clean; 04000_empty_part_stale_tmp_dir passes.

[hanfei1991]

@groeneai, merge with master

[groeneai]

@hanfei1991 Merged current master and pushed @ dec95c9. Branch was 5 commits behind; now up to date. CI re-running.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.80%	85.80%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	78.00%	78.00%	+0.00%

Changed lines: Changed C/C++ lines covered: 36/36 (100.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — dec95c9

CI fully finished on this head (Finish Workflow + Mergeable Check both pass). Zero test-level failures on the current head; every stateless/integration/build/stress check is green.

Check / test	Reason	Owner / fixing PR
(no failed tests on dec95c9)	—	—
Sync	—	CH Inc sync (private, not actionable)

Note: the earlier reds on the pre-merge commit 5aa0f8d3 (02346_text_index_bug108519_qcc_skip_index, test_replicated_database::test_replicated_table_structure_alter, arm_tidy) are gone on this head after merging master; those tests are unrelated to this PR (stale-empty-part reclamation) and pass on the fresh base.

Session id: cron:our-pr-ci-monitor:20260713-220000