Search the whole storage policy when picking a detached/ directory name by groeneai · Pull Request #112416 · ClickHouse/ClickHouse

groeneai · 2026-07-29T10:36:30Z

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):

Fix ALTER TABLE ... DETACH PART creating a second detached/ directory under a name already in use on another disk of a multi-disk storage policy. The duplicate silently duplicated rows on a following ATTACH PARTITION and made ATTACH PART pick an arbitrary one of the copies. The name is now checked against the disks that directory is enumerated from, so the existing _tryN suffix is applied as intended.

Description

What breaks. detached/ is a table-wide namespace, enumerated by getDetachedParts across the policy's disks. If detached/P exists on disk A and the live part P sits on disk B, DETACH PART 'P' creates a second logical detached/P. tryLoadPartsToAttach feeds both into one ActiveDataPartSet; the identical dir_name makes the second tryAdd return HasCovering, unhandled by the loop, so both attach under fresh block numbers.

Root cause. The allocator detaches funnel through, DataPartStorageOnDiskBase::getRelativePathForPrefix, probed only its own SingleDiskVolume, blind to the other disks, so it skipped the _tryN suffix meant for this case.

The change. IDataPartStorage::getRelativePathForPrefix now takes a required table-wide name predicate, and IMergeTreeDataPart supplies MergeTreeData::isDetachedNameTakenOnEnumerableDisk, applying the same read-only and write-once skip as getDetachedParts, so a name taken only on a disk ATTACH never offers is ignored. The own disk is still probed, so the answer is a superset of the old one. Two consequences: the broken-part content comparison stays own-disk, since a twin elsewhere cannot be read; and exhausting the 10 attempts now throws DIRECTORY_ALREADY_EXISTS instead of returning a taken name, tolerated under ignore_error for that code only. Single-disk policies are a no-op.

Scope. The predicate covers the 15 detach call sites using that allocator. Two Replicated paths rename into detached/ directly and stay out of scope: a _tryN name is unattachable there, so they must refuse instead. The fetchPart publish is fixed in #112215; executeClonePartFromShard needs an ownership marker, since its entry is re-executed and a refusing rename would stall the shard-move orchestrator.

Validation. New integration test on a two-disk JBOD policy (functional tests may not fabricate parts). Reverting only the predicate reproduces the 2 rows / 1 distinct name failure; both exhaustion directions, a non-exhaustion failure and a read-only disk are covered. test_partition and the stateless detached tests stay green.

detached/ is a table-wide namespace: every reader resolves a detached directory by scanning all disks of the storage policy. The allocator that every detach goes through, DataPartStorageOnDiskBase::getRelativePathForPrefix, probed only the part's own disk, because its volume is a per-part SingleDiskVolume. So on a multi-disk policy, if detached/P already exists on disk A and the live part P sits on disk B, the allocator did not see the collision, skipped the _tryN suffix that exists for exactly this case, and created a second logical detached/P. The damage lands on the next statement. tryLoadPartsToAttach feeds both directories into one ActiveDataPartSet; because the two dir_names are identical strings the second tryAdd returns HasCovering, and the loop only handles HasIntersectingPart, so neither copy is demoted to ignored_. Both become attach candidates and each commits under a fresh block number, so ATTACH PARTITION silently attaches both copies. The single-part form was affected too: it resolves the disk through getDiskForDetachedPart, which returns the first policy disk carrying the name, so which copy got attached was decided by disk order. MergeTreeData owns the storage policy and already exposes the canonical table-wide answer, so the fix injects it into the allocator as a predicate rather than teaching the single-disk part storage about other disks. The allocator is the single choke point for all 17 renameToDetached / makeCloneInDetached producers, so no per-site checks are needed and no producer can drift. Two details: The broken-part content check stays own-disk. looksLikeBrokenDetachedPartHasTheSameContent builds its comparison storage on the part's own volume, so a twin on another disk cannot be inspected. The loop keeps the own-disk result as a separate, narrower question and only advances to _tryN in that case, never dropping a broken part on the strength of a directory it could not read. Exhaustion of the 10 attempts now fails closed. It used to return the last candidate even though it knew the name was taken, relying on the caller's own guard: Backup() throws DIRECTORY_ALREADY_EXISTS and rename(..., remove_new_dir_if_exists = true) deletes the local twin. Both guards are own-disk, so a name taken only on another disk at _try9 would have recreated exactly the duplicate this change prevents, leaving the fix complete for _try0..8 and silently broken at _try9. The throw applies only when a table-wide predicate was supplied, so non-detached callers are untouched, and no catch was widened to swallow it. Single-disk policies are a no-op: tryGetDiskForDetachedPart iterates the one disk and checks the same path. Verified byte-identical system.detached_parts output before and after, and pinned by a control case in the new test. The collision cannot be built from one table's own detaches, since a DETACH covers the active part and ATTACH PART re-activates the copy on the disk it already sits on. The new integration test uses FETCH to publish the twin, MOVE PART ... TO DISK to split live from detached, then DETACH, and asserts mid-way that the split actually happened so it cannot degenerate into a single-disk run. Related: ClickHouse#112215 fixes the fetch-side half of the same hole.

…ustion Review round 1 follow-ups to the table-wide detached/ name search. The new trailing parameter of getRelativePathForPrefix carried a default argument on a virtual method, which clang-tidy's google-default-arguments check rejects and Build (arm_tidy) turns into an error. The sole caller repo-wide already passes it explicitly, so the parameter is now required rather than suppressed: a future detach path cannot silently omit the table-wide predicate. The fail-closed DIRECTORY_ALREADY_EXISTS thrown when all ten candidate names are taken was raised outside renameToDetached's try block, and its catch list covers ErrnoException and fs::filesystem_error but not DB::Exception, so callers that pass ignore_error could not skip it. Two broken-on-start sites run inside function-try-blocks whose handler calls std::terminate, so such an escape would take the server down instead of skipping one part. The name resolution now happens inside that try and one narrow catch honours the same ignore_error policy as the arms already there; callers that did not ask for errors to be ignored still see the throw, so the hole the throw closes stays closed. The own-disk existsDirectory probe is now evaluated lazily, only for the broken-part content comparison that needs it, so a detach no longer pays for it in addition to the table-wide check. Adds coverage for both directions of the exhaustion branch, which had none. The survival case asserts the exhaustion was reached and logged, so it cannot pass without exercising the branch.

Moving the detached-name resolution inside renameToDetached's existing try block, so an ignore_error caller can skip a detach that cannot pick a name, also put the rename itself under the newly added handler. An unqualified catch therefore swallowed every DB::Exception the block can raise, including storage failures from the rename and LOGICAL_ERROR. That matters because the caller erases the part from data_parts_indexes on the statement right after renameToDetached returns, so a swallowed rename failure forgets a part that is still sitting in its original directory, and LOGICAL_ERROR would be skipped in release while aborting a debug build. Qualify the handler on DIRECTORY_ALREADY_EXISTS, the code the allocator raises when all ten candidate names are taken. Everything else propagates as before. The arm stays after the ErrnoException one, which derives from Exception and would otherwise be shadowed. The regression test drives the surviving hole through SYSTEM RESTORE REPLICA, which detaches every part with ignore_error = true, on a disk whose metadata is rewritable so that a directory move can fail with a DB::Exception at all; a local disk only ever raises fs::filesystem_error there, which a pre-existing handler already owns. Without the qualification the injected fault is swallowed and the part is then reported missing. Also scope the startup test's anti-vacuity log check to the current server run and attribute it to the frame that swallows. The same sentence is emitted by a user query in the sibling exhaustion test and by AsyncLoader::worker when the exception escapes instead of being swallowed, so an unscoped search proved only that exhaustion had been reached. A pre-restart line offset cannot be used for the window: the integration logger runs with rotateOnOpen, so a restart opens a fresh log file.

groeneai · 2026-07-29T10:36:32Z

Internal second-model review (4 rounds, 12 findings, all resolved)

Every change in this PR was reviewed by a second model independently of the model that wrote it,
across four rounds. Three rounds ended in a bounce; findings and verdicts below.

Round 4 (final)

⚠️ broken-disk-false-free, an unavailable policy disk could be read as a free name. DISAGREE,
refuted by measurement. The claim needs existsDirectory to return a silent false for an
inaccessible disk. DiskLocal::existsDirectory is fs::is_directory(disk_path / path), the
overload with no error_code, so libc++ maps only no_such_file_or_directory and not_a_directory
to "not found" and throws fs::filesystem_error for anything else. A standalone probe over an
unreadable parent returns THREW filesystem_error: Permission denied, against returned false for
a genuinely absent path. That throw lands in the pre-existing fs::filesystem_error handler in
renameToDetached, which is already fail-closed in both directions: it propagates when
ignore_error is false and skips the detach when it is true, so no directory is created on the other
disk either way. The suggested remedy of filtering broken disks out of tryGetDiskForDetachedPart
would also be wrong here: getDetachedParts skips only readonly and write-once disks, and
checkIfDetachedPartExists filters nothing, so excluding a broken disk would put the writer back out
of step with the readers and reopen this very bug. The probe primitive is unchanged from before this
PR; only the set of disks probed changed.

💡 Residual noted, not blocking: because name resolution and the rename share one try, a
DIRECTORY_ALREADY_EXISTS raised by the rename itself would also be tolerated under ignore_error.
Every such site is unreachable from this call site (rename's own throw needs
remove_new_dir_if_exists == false, and the plain-rewritable sites require a destination that
UncommittedState::moveDirectory already returns early on), leaving only a concurrent creation of
the just-allocated name, the same TOCTOU documented as out of scope, where skipping is the desired
behaviour.

Round 3

❌ The ignore_error handler swallowed more than name exhaustion. AGREED and fixed. Moving name
resolution inside the existing try (round 2's own fix) made that block span the rename too, while
the handler was an unqualified catch (const Exception &). Plain-rewritable
DIRECTORY_DOESNT_EXIST / LOGICAL_ERROR, the read-only disk wrapper, and the projection
changeRootPath error were all newly tolerated, and the caller erases the part from
data_parts_indexes on the next statement, so a swallowed storage failure would forget a part still
sitting in its original directory. The handler is now qualified on DIRECTORY_ALREADY_EXISTS only,
kept after the ErrnoException arm so that arm is not shadowed, with a test proving a
non-exhaustion failure still propagates under ignore_error.

❌ The anti-vacuity log assertion was itself vacuous, two ways. AGREED and fixed. It was
unscoped, so a sibling test's identical log line satisfied it from the second repeat onward; and it
was unattributed, so it also matched when the exception escaped instead of being swallowed,
which the mutant arm proved. The guard is now windowed to the current server run and requires the
swallowing frame's own logger prefix, and dropping the setup step now reddens it, which the previous
version never demonstrated.

💡 makeCloneInDetached reaches the same throw with no ignore_error parameter. Verified this is not
a terminate path (it lands in ReplicatedMergeTreeRestartingThread's catch (...), which retries),
so it is the correct fail-closed direction, now disclosed in the description.

Round 2

❌ Build (arm_tidy) would have failed. AGREED and fixed. The new trailing parameter carried a
default argument on a virtual method at three sites; google-default-arguments is enabled with
warnings-as-errors, and every pre-existing instance in the repo carries a suppression. Reproduced
three errors at exactly those sites by driving the real translation unit from
compile_commands.json. Fixed by making the parameter required rather than by suppressing the check.

❌ The new fail-closed throw would have killed the server at startup. AGREED and fixed. Two
broken-on-start callers sit in function-try-blocks whose handler terminates the process, and the
old code could not abort there because it returned a taken name and let the rename clobber the local
twin. Name resolution now sits inside the try and the exhaustion is honoured under the existing
ignore_error contract, with both directions tested.

💡 Two nits folded in: a redundant own-disk probe per attempt (now lazy), and no coverage of the new
exhaustion branch (now covered in both directions).

Confirmed sound

Carrier enumeration was re-derived independently each round and matched: getRelativePathForPrefix
has exactly one caller repo-wide, neither storage subclass overrides it, all 17 detach producers
funnel through it, and projections are rejected earlier. The predicate's namespace is exactly the
readers': same path shape, and a part's disk can never leave the policy because
StoragePolicy::checkCompatibleWith rejects any ALTER that drops one, so the new answer is a
strict superset of the old own-disk answer and can never miss a name the old probe caught. The
broken-part content comparison is deliberately left own-disk: it reads checksums.txt through the
part's own volume and physically cannot inspect a twin elsewhere, so gating it prevents a cross-disk
name being mistaken for "already cloned". Test liveness is pinned by mutation in every round, and the
single-disk control passes on the fixed, pristine and mutant binaries alike, so it is a real control
rather than a copy of the regression assertion.

Session id: cron:clickhouse-review-slot-50:20260729-093800

groeneai · 2026-07-29T10:36:47Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, 100% on demand, no randomization needed: `python3 -m ci.praktika run 'integration' --test 'test_detach_detached_name_cross_disk'`. FETCH publishes `detached/all_0_0_0`, `MOVE PART ... TO DISK` splits the live part onto the other disk, `DETACH PART` then allocates the name. The exhaustion cases occupy all ten candidate names on the other disk with a `mkdir` and then detach. Every case was driven by hand against a local two-disk server before being encoded as a test.
b	Root cause explained?	Yes. `DataPartStorageOnDiskBase::getRelativePathForPrefix` tested uniqueness with `volume->getDisk()->existsDirectory(...)`, and `volume` is a per-part `SingleDiskVolume`, so the predicate was scoped to one disk while the `detached/` namespace it allocates into is scoped to the whole storage policy. The `_tryN` disambiguation therefore never fired for a name taken on another disk. `tryLoadPartsToAttach` then got two identical `dir_name` strings, the second `tryAdd` returned `HasCovering` which the loop does not handle, so neither copy was demoted and both attached under fresh block numbers.
c	Fix matches root cause?	Yes. The predicate's scope is corrected at the allocator by injecting `MergeTreeData::tryGetDiskForDetachedPart` (the same table-wide resolver the readers use). No bound relaxed, no assertion weakened, no guard bolted onto a failure site.
d	Test intent preserved / new tests added?	No existing test weakened or removed. New integration test with six cases: the name assertion, ATTACH PART determinism, both directions of the 10-attempt exhaustion branch (which had no coverage anywhere: `grep -rn _try9 tests/` was empty), a case pinning that a non-`DIRECTORY_ALREADY_EXISTS` failure still propagates under `ignore_error`, and a single-disk control. Assertions are on data (`count()`, the row value), not only on directory names; the absence of `ignored_*` is deliberately not the regression signal since it holds before and after. The startup case's anti-vacuity guard is scoped to the current server run and attributed to the swallowing frame, because the same log sentence is also emitted by a user query in the sibling case and by `AsyncLoader::worker` when the exception escapes instead of being swallowed.
e	Both directions demonstrated?	Yes, per assertion, with four mutations. Pristine master (Build ID `b4f3ef4d`): 2 failed, 1 passed, signature `assert '2\t1' == '2\t2'`. With the fix: 6 passed. Mutation reverting only the predicate at the call site (`00f3bcb8`): 2 failed again. Mutation deleting only the new `catch` arm (`c5ca573e`): exactly 1 failed, the exhaustion escaping via `AsyncLoader::worker`. Mutation leaving the `catch` arm unqualified, i.e. the shape before this round (`f03cb783`): the propagation case fails with `Code: 233 ... Detached part not found`, the part having been erased from memory while still on disk. Mutation making the exhaustion branch unreachable while the server stays healthy: the anti-vacuity guard itself fails, so it is load-bearing rather than decorative. Each mutation was followed by a bit-identical restore to `8e513641`, and every measurement had `SELECT buildId()` matched against `readelf -n`.
f	Fix is general across code paths?	Yes, that is why the allocator was the chosen layer. `git grep getRelativePathForPrefix` returns 7 hits with no caller outside the allocator and its two part-level entry points, and all 17 `renameToDetached` / `makeCloneInDetached` producers funnel through it (DETACH PART, DETACH PARTITION, DROP range, broken-on-start, `unexpected`, `ignored`, `broken`, `covered-by-broken`, `clone`, `noquorum`, merge and mutate not-byte-identical, RESTORE of a broken part). No per-site checks added. Neither `DataPartStorageOnDiskFull` nor `DataPartStorageOnDiskPacked` overrides the method, so both are covered; projections throw earlier. Writers into `detached/` that are not name allocators are disclosed in the description rather than silently folded in.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	The input is a policy shape rather than a value type. Covered: one disk (byte-identical no-op, verified against both binaries), two disks with the live part and the detached twin on the same disk, two disks split, and both part-storage shapes via the base class. The `broken` + `isFullPartStorage` content-comparison branch is preserved and now also gated on the own-disk answer, so it can never run against a directory it cannot read.
h	Backward compatible? (maintainer-approved exception only)	Yes. No setting, serialization format or protocol change, so no `SettingsChangesHistory.cpp` entry. The `_try0..9` budget is unchanged. Single-disk policies are a verified no-op. The only externally visible change is which directory name a detach picks on a multi-disk policy, and only in the case that is currently broken.
i	Invariants and contracts preserved?	The allocator's postcondition is strengthened from "unused on this disk" to "unused across the policy", which is what its consumers already assumed. Early-return paths walked: the skip-clone `{}` return is now unreachable for a twin that could not be inspected, which is the conservative direction. Exhaustion of the 10 attempts no longer returns a known-taken name but throws `DIRECTORY_ALREADY_EXISTS`, because both existing caller guards (`Backup()` and `rename(..., remove_new_dir_if_exists = true)`) are own-disk and would otherwise recreate the duplicate at `_try9`. The throw is gated on a predicate having been supplied, so non-detached callers are untouched. `renameToDetached`'s own `ignore_error` contract now covers that throw as well: the name resolution moved inside its existing `try` and one `catch` arm was added with the same `if (ignore_error) ... else throw;` shape as the two arms already present, placed after them because `ErrnoException` derives from `Exception`. That arm is qualified on `DIRECTORY_ALREADY_EXISTS` rather than catching every `DB::Exception`, because the block now spans the rename as well: the caller erases the part from `data_parts_indexes` on the statement after this returns, so swallowing a storage failure would forget a part still sitting in its original directory, and `LOGICAL_ERROR` would additionally be skipped in release while aborting a debug build. Verified by mutation, the unqualified form loses the part with `Code: 233`. `makeCloneInDetached` has no `ignore_error` parameter, so the throw propagates raw from its background callers, which is the intended fail-closed direction and lands in `ReplicatedMergeTreeRestartingThread`'s `catch (...)` for a retry after a partial shutdown rather than a terminate. `ignore_error == false` callers still see the throw, so this restores no hole; it prevents the escape taking down the server, since two `broken-on-start` sites run inside function-try-blocks whose handler calls `std::terminate`. Both directions are measured and mutation-tested. Concurrency: the background producers that hold no `lockParts` get a narrowed TOCTOU window rather than mutual exclusion, disclosed in the description; user-issued `ALTER ... DETACH` is serialized by the locks its callers already hold.

groeneai · 2026-07-29T10:37:45Z

cc @tiandiwonder @alesapin — could you review this? The detached part name allocator probed only the part's own disk (a per-part SingleDiskVolume) while the detached/ namespace is resolved across the whole storage policy, so on a multi-disk policy DETACH PART skipped the _tryN suffix and created a second directory under a name already taken on another disk, which then silently duplicated rows on ATTACH PARTITION.

clickhouse-gh · 2026-07-29T10:58:17Z

Workflow [PR], commit [1f78c07]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Stress test (amd_debug)		FAIL
	Test script failed	FAIL	cidb

AI Review

Summary

This PR fixes the cross-disk detached-name allocation bug for the detach flows that go through IMergeTreeDataPart by checking the whole enumerable storage policy before choosing a detached/ name. That part looks sound, but the fix is still incomplete because replicated fetch/clone-to-detached writers bypass the new check and can recreate the same duplicate-name state.

Findings

❌ Blockers

[src/Storages/MergeTree/IMergeTreeDataPart.cpp:2603] The new whole-policy name check only covers detached names allocated through IMergeTreeDataPart::getRelativePathForPrefix. StorageReplicatedMergeTree::executeClonePartFromShard still renames fetched parts straight to detached/<part> at StorageReplicatedMergeTree.cpp:3527, and fetchPart(..., to_detached = true) still does the same at StorageReplicatedMergeTree.cpp:5720 after only a best-effort precheck. If detached/<part> already exists on another disk, these paths can still create the second logical detached directory that tryLoadPartsToAttach later attaches twice. Route those writers through the same whole-policy collision check, or reject the operation once the name is already taken anywhere in the enumerable detached namespace.

Final Verdict

❌ Changes needed before merge.

The table-wide search introduced here probed every disk of the storage policy, but detached/ is not enumerated across all of them: getDetachedParts() deliberately skips read-only and write-once disks, and ATTACH PARTITION additionally drops every '_tryN' directory from its candidate set. So a name occupied only on a skipped disk was treated as taken. The live part was then renamed to '<part>_try1', which nothing can attach, while the directory that caused the collision stayed invisible: the partition became unattachable even though both copies were on disk. Reproduced on a policy holding one writable and one read-only disk, where ATTACH PARTITION recovered the rows before this change and none after it. The allocator now asks whether the name is taken on the disks that enumeration actually covers. The part's own disk is still probed unconditionally, so the answer stays a superset of the original own-disk probe and the duplicate detached/<part> this branch fixes is still prevented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

groeneai · 2026-07-29T22:17:10Z

CI finish ledger - `e2198c8`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 01666_merge_tree_max_query_limit	deterministic under parallel replicas (3 of 3 reruns failed)	#112385 (ours, merged 2026-07-29 15:37Z) - not yet on this branch (`e2198c8` diverged from that merge), so merging master picks it up
Stateless tests (arm_binary, parallel) / 02354_distributed_with_external_aggregation_memory_usage	query-level `Code: 241` at the shared 4.66 GiB CI profile cap on the test's own INSERT, 13 of 21 reruns failed with the same settings; a trunk regression with a master hit, not caused by this diff	#112478 (ours, open)

The two Bugfix validation (integration tests, *) jobs are green (check_status: success); their
per-test rows are this PR's own new regression test being inverted, which is the check working as
designed.

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

clickhouse-gh · 2026-08-01T16:49:52Z

+    /// so the name must be free on those too, not just on the disk this part happens to live on.
+    IDataPartStorage::NameTakenChecker name_taken_anywhere;
+    if (detached)
+        name_taken_anywhere = [this](const String & dir_name) { return storage.isDetachedNameTakenOnEnumerableDisk(dir_name); };


detached/ is still only table-wide on the IMergeTreeDataPart paths. Replicated fetch-to-detached flows bypass this helper entirely: StorageReplicatedMergeTree::executeClonePartFromShard still does part->renameTo(detached/<part>, true) at StorageReplicatedMergeTree.cpp:3527, and fetchPart(..., to_detached = true) does the same at StorageReplicatedMergeTree.cpp:5720 after only a best-effort precheck.

If detached/<part> already exists on another disk, those paths can still create the second logical detached/<part> that tryLoadPartsToAttach later attaches twice. These writers need the same whole-policy collision check, or they need to reject the operation once the name is already taken anywhere in the enumerable detached namespace.

Correct on the mechanism, and both line numbers still land on the right code after the master merge. git grep -nE 'renameTo\(\s*(fs::path\()?(DETACHED_DIR_NAME|"detached")' -- src/ returns exactly those two hits, and both reach DataPartStorageOnDiskBase::rename, whose probe is volume->getDisk()->existsDirectory(to) at DataPartStorageOnDiskBase.cpp:709. Neither goes through getRelativePathForPrefix, so the new predicate does not apply to them. The PR body's "all 17 detach producers" was wrong in both directions and I have corrected it: 15 call sites use the allocator, 2 write directly.

I am not extending the predicate to those two here, for a reason beyond scope.

The allocator's remedy for a taken name is a _tryN suffix, and both ATTACH readers erase _tryN entries before choosing candidates: MergeTreeData.cpp:9300 and StorageReplicatedMergeTree.cpp:2424 filter on part_info.has_try_suffix. For a detach that is right, since the part is leaving the working set and the copy is recoverable residue. For a fetch-to-detached the output exists so the user can ATTACH it, so detached/P_try1 would be silently unattachable. These two need your second remedy, refusal, not the allocator. This PR hit the mirror image of that from the other side: an earlier revision treated a name on a read-only disk as taken, pushed the live part to a _tryN directory the same enumeration filters out, and left the partition unattachable.

So they are owned separately:

fetchPart(..., to_detached = true) at :5720 is already fixed in #112215, which takes lockParts, rejects when tryGetDiskForDetachedPart(part_name) finds a copy anywhere in the policy (MergeTreeData.cpp:10696 iterates getStoragePolicy()->getDisks()), and drops remove_new_dir_if_exists. That is your refusal remedy verbatim. Adding it here too would put the same change in two open PRs.

executeClonePartFromShard at :3527 is genuinely unfixed, and it is the one you also raised on #112215 as discussion_r3663716371. It cannot take a name check as it stands: the CLONE_PART_FROM_SHARD entry is re-executed indefinitely, executeLogEntry's already-exists short circuit is gated on is_get_or_attach || MERGE_PARTS || MUTATE_PART so this type falls through, and a refusing rename therefore fails forever on the entry's own leftover while PartMovesBetweenShardsOrchestrator.cpp:383 waits before DESTINATION_ATTACH. A wedged MOVE PART TO SHARD is worse than the duplicate. Making it safe needs an ownership marker, either a staging name or a checksum on the entry so re-execution recognises its own output, which is a design change rather than a parameter. It is tracked, and the feature is gated behind part_moves_between_shards_enable, default 0.

One writer your list does not name, for completeness rather than as an action item: ReplicatedMergeTreeSink.cpp:641 renames a deduplicated ATTACH PART candidate back from detached/attaching_<part>. It passes remove_new_dir_if_exists = false, so a collision throws, and the name it restores is one ATTACH had just taken out of that namespace.

clickhouse-gh · 2026-08-01T21:38:39Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.50%	86.50%	+0.00%
Functions	91.90%	91.90%	+0.00%
Branches	78.70%	78.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 32/41 (78.05%) · Uncovered code

Full report · Diff report

groeneai · 2026-08-01T23:14:43Z

CI finish ledger - `1f78c07`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stress test (amd_debug) / `Test script failed`	job-level wrapper exit 1 while every named gate (server start, lost s3 keys, SharedMergeTree) is green and no server-side error was captured; 6 unrelated PRs plus master in 30 days	a fix task is created (investigating at full effort, fixing-PR link to follow here)

Session id: cron:our-pr-ci-monitor:20260801-230000

groeneai added 3 commits July 28, 2026 14:09

PedroTadim added the can be tested Allows running workflows for external contributors label Jul 29, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 29, 2026

clickhouse-gh Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/Storages/MergeTree/IMergeTreeDataPart.cpp Outdated

Merge branch 'master' into groeneai/fix-detach-detached-name-table-wide

1f78c07

clickhouse-gh Bot reviewed Aug 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

Search the whole storage policy when picking a detached/ directory name - #112416

Search the whole storage policy when picking a detached/ directory name#112416
groeneai wants to merge 5 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-detach-detached-name-table-wide

groeneai commented Jul 29, 2026 •

edited

Loading

Uh oh!

groeneai commented Jul 29, 2026

Round 4 (final)

Round 3

Round 2

Confirmed sound

Uh oh!

groeneai commented Jul 29, 2026

Uh oh!

groeneai commented Jul 29, 2026

Uh oh!

clickhouse-gh Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

groeneai commented Jul 29, 2026

Uh oh!

clickhouse-gh Bot Aug 1, 2026

Uh oh!

groeneai Aug 1, 2026

Uh oh!

clickhouse-gh Bot commented Aug 1, 2026

Uh oh!

groeneai commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

groeneai commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jul 29, 2026

Round 4 (final)

Round 3

Round 2

Confirmed sound

Uh oh!

groeneai commented Jul 29, 2026

Uh oh!

groeneai commented Jul 29, 2026

Uh oh!

clickhouse-gh Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Final Verdict

Uh oh!

Uh oh!

groeneai commented Jul 29, 2026

CI finish ledger - e2198c8

Uh oh!

clickhouse-gh Bot Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Aug 1, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Aug 1, 2026

CI finish ledger - 1f78c07

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

groeneai commented Jul 29, 2026 •

edited

Loading

clickhouse-gh Bot commented Jul 29, 2026 •

edited

Loading

CI finish ledger - `e2198c8`

CI finish ledger - `1f78c07`