Create custom disks outside storage_policies_mutex by filimonov · Pull Request #2573 · Altinity/ClickHouse

filimonov · 2026-10-15T11:08:58Z

Run the creator of a disk(...) custom disk outside the server-wide storage_policies_mutex. This covers custom disks only: disks defined in storage_configuration.disks are still created under that mutex, at server start and on configuration reload.

Context::getOrCreateDisk ran the disk creator under that mutex. A CAS disk whose previous process exited without a farewell waits out the stale mount lease inside the creator (about 36.5 s with the default 30 s lease). While it waited, every other custom-disk creation and every getDisk, getDisksMap and getStoragePolicy call queued behind it. With three stale mounts, server start would take about 110 s; the stress test harness gave up at its limit with "Cannot start clickhouse-server":
#2516 (Stress test (amd_debug), https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2516&sha=b7e424247ab997ae2b0d3c06853e10a2c445f2a5&name_0=PR&name_1=Stress%20test%20%28amd_debug%29)

Changes

getOrCreateDisk releases the mutex while the creator runs. The mutex still guards selector initialization, lookup, the disks-map snapshot passed to the creator, the in-flight creation map, the final insert into DiskSelector, and configuration reload.
Concurrent requests for the same name share one creation. A second open of one CAS root would fail as a live twin.
Concurrent callers of a failed creation all get its failure and do not run their own creator; previously each blocked caller ran its creator in turn. The creator rethrows its own exception. Each waiter gets its own copy of a Poco::Exception; the dynamic type is kept for exception types that override rethrow (for example ErrnoException, NetException, MountFencedException), and other Poco::Exception types arrive as their nearest ancestor that does. Exceptions that are not Poco::Exception are rethrown unchanged.
A failed creation is not cached: the next request creates the disk again, as before.
A configuration reload skips a configured disk whose name is being created as a custom disk and logs a warning: creating it could open the same storage twice. Storage policies of that reload that name a disk being created as a custom disk are skipped and keep their previous definition; all other policy changes of the reload are applied.
getDisk and getStoragePolicyFromDisk report "is being created" instead of Unknown disk for a name whose custom disk is being created.
The stale-mount wait itself is unchanged.

Risks / notes

Parallel stale-mount waits apply to custom disks created from independent requests (for example, table loads). Config-defined disks are still created one by one in DiskSelector::initialize and DiskSelector::updateFromConfig, under storage_policies_mutex: a stale mount of a config-defined disk still blocks disk and storage policy lookups.
One stale mount still costs about 36.5 s of server start.
A configured disk skipped by a reload is not created by that reload. If the custom creation succeeds, the name then refers to the custom disk. If it fails, the configured disk stays absent until the next reload, and storage policies that use it stay as they were until then.

Testing

src/Disks/tests/gtest_cas_startup_stale_mounts.cpp, all also with --gtest_repeat=3:
- 3 stale disks open in about 3.6 s instead of 10.8 s.
- A getDisksMap call during a stale-mount wait returns under the 1.15 s bound instead of after 3.6 s.
- Two concurrent creations of one disk share one open.
- A failed creation is retried by the next request.
- Two concurrent requests share one failure: one creator call, the creator rethrows its original ErrnoException, the waiter gets its own copy.
- A configuration reload during a creation does not create the configured disk of the same name.
- That reload applies the policies that do not name the disk and keeps the previous definition of those that do.
- A failed creation rethrows the creator's own exception type; a waiter gets a copy of the same type (MountFencedException).
- A lookup of a disk being created reports "is being created".
CAS*, *StoragePolic* and *DiskSelector* unit tests on b558a36234a: release 2644, ASan 2648, all passed.

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 to CHANGELOG.md):

Creating a disk(...) custom disk no longer blocks other disk creations and disk or storage policy lookups. Server start with several CAS disk(...) custom disks that wait out a stale mount lease now waits for them in parallel instead of one after another. Disks defined in storage_configuration.disks are unchanged.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

No documentation change: no setting, syntax or documented behavior changes.

CI/CD Options

Exclude tests:

Regression jobs to run:

Server start creates each table's `disk = disk(...)` custom disk inside `Context::getOrCreateDisk`, which holds `storage_policies_mutex` while the disk starts. A CAS disk whose previous process left no farewell waits out the stale mount lease during that start, so N stale mounts cost N observation windows and every disk or storage-policy lookup in the server blocks meanwhile. Both tests fail on the current code: three stale mounts take 10820 ms against a 3450 ms window, and `getDisksMap` blocks for 3603 ms. CI report: https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2516&sha=b7e424247ab997ae2b0d3c06853e10a2c445f2a5&name_0=PR&name_1=Stress%20test%20%28amd_debug%29 PR: #2516 Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

`Context::getOrCreateDisk` ran the disk creator under the server-wide `storage_policies_mutex`. A CAS disk waiting out a stale mount lease (about 36.5 s) inside the creator blocked every other disk creation and every disk and storage policy lookup, so stale mounts were waited out one after another at server start. The mutex now guards only the map of in-flight creations and the final insert into `DiskSelector`. Concurrent requests for one name share one creation and get the same disk or a copy of the same exception. A failed creation is still not cached. CI report: #2516 Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A configuration reload could create a configured disk whose name was being created as a custom disk, opening the same storage twice. The reload now skips such names and logs a warning. Callers of a shared disk creation rethrow a copy of the stored exception through `Poco::Exception::rethrow`, so `ErrnoException` and other derived types keep their type and fields. Other exceptions become a `DB::Exception` with the same code and message. Tests: shared failure with independent copies, config reload during a creation, unique disk names so the suite survives `--gtest_repeat`, and the CAS logger channel is restored only after the disks stop. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Use the existing `copyMutableException` helper: `Poco` exceptions are copied through their virtual `rethrow`, other exceptions are rethrown unchanged. State the `disks_to_skip` contract in `DiskSelector.h`, wrap a long log line, and bound the waits in the shared-failure test. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-15T11:10:25Z

Workflow [PR], commit [b558a36]

alsugiliazova · 2026-10-23T10:46:56Z

PR #2573 Audit Review

Verdict

The core concurrency change is correct. The hand-off between the creating thread and the
waiters is sound, the in-flight map is erased in the same critical section that inserts the disk, so
a config reload sees either "being created" or "created" and never both, and a failed creation
leaves no state behind.

Two Medium findings and three Low. The most consequential is not a bug in the new code but the
limit of its reach: the stall this PR exists to remove is still present for CAS disks defined in
config.xml rather than by disk(...) 8000, which is how this repository's own CAS test environment
defines them. The second Medium is that a configuration reload landing inside a custom-disk
creation discards every storage-policy change in that reload, not just the ones that name the
affected disk.

What the change does

Context::getOrCreateDisk used to hold storage_policies_mutex across the whole creation. A CAS
disk whose previous process died without a farewell waits out the stale mount lease inside
DiskFactory::create, so every other disk creation and every getDisk, getDisksMap and
getStoragePolicy call queued behind that wait.

The function now runs in three phases.

Phase	Under the lock	What happens
Admission	yes	`tryGet(name)` returns an existing disk. Otherwise the caller either joins an in-flight `std::shared_future<DiskPtr>` from `disks_being_created`, or registers its own and becomes the creator with a copy of the disks map
Creation	no	Only the creator runs `creator(disks_map)`. Any exception is captured into a local `std::exception_ptr` rather than thrown
Completion	yes	`disks_being_created.erase(name)` and, on success, `addToDiskMap(name, disk)` — one critical section. The promise is then satisfied outside the lock

updateStorageConfiguration passes the names currently in disks_being_created to
DiskSelector::updateFromConfig as disks_to_skip; a configured disk with such a name is logged
and not created, because creating it could open the same storage twice.

Model


Actors	Table-loader threads creating `disk(...)` disks, the config-reloader thread, any thread resolving a disk or storage policy, and the CAS object store holding the mount lease
Channels	One process-local mutex (`storage_policies_mutex`), one in-flight map, one `shared_future` per creation. Across processes: the conditional PUT of the mount object
Authoritative state	`DiskSelector::disks` for which names exist; the CAS mount object for which process owns a pool
Failure model assumed by the code	The creator can block for tens of seconds on the network. A creator either returns a disk or throws. Callers for one name must not open the same storage twice
Safe failure criterion	No two live opens of one CAS root, no lost or half-registered disk, no caller blocked without a reason, and a lookup that never queues behind a creation

Why the core hand-off holds

The admission and completion phases are the same critical section, so the window the old code
closed with one long lock hold is closed instead by the invariant that a name is in
disks_being_created or in the disks map, never in neither and never in both. A reload that runs
while a creation is in flight therefore cannot create a second disk for that name
(DiskSelector.cpp:169), and a reload that runs after the completion phase finds the name already
in the map and takes the applyNewSettings branch instead.

A caller arriving between erase and set_value is also safe: on success it finds the disk
through tryGet because addToDiskMap ran in the same critical section as the erase, and on
failure it becomes a new creator, which is the documented "a failed creation is not cached"
behavior that FailedDiskCreationIsRetriedByTheNextRequest pins.

The promise lives on the creator's stack but its shared state is reference-counted by the
shared_future copies, so a waiter that has not yet called get() when the creator returns still
observes the result.

One pre-existing hazard is removed rather than added. Under the old code a creator that called
getDisk, getDisksMap or getOrCreateDisk re-entered a non-recursive mutex it already held. Now
only a creator that requests its own name blocks, and Context.h:1691 states that precondition.

Findings

Partition / availability

Medium: the stall remains for CAS disks defined in the configuration.

Impact. On a server whose CAS disks come from <storage_configuration><disks> rather than
disk(...), server start still waits out stale mount leases one after another, and every
getDisk, getDisksMap and getStoragePolicy call still queues behind each wait. That is the
failure mode the PR describes, unchanged.
Anchor. Context::getDiskSelector (Context.cpp:6934-6943) calls DiskSelector::initialize
while holding storage_policies_mutex, and initialize calls factory.create per configured
disk (DiskSelector.cpp:119). Context::updateStorageConfiguration holds the same mutex across
updateFromConfig (Context.cpp:6964-6973), which calls factory.create
(DiskSelector.cpp:177).
Trigger. Start a server, or reload the configuration, with one or more configured CAS disks
whose previous process exited without a farewell. One stale mount blocks all disk and policy
lookups for the observation window, about 36.5 s at the default 30 s lease; three block for about
110 s.
Why defect. Not a defect introduced here — the new code is correct for what it covers. It is
a scope gap against the PR's own problem statement. The PR body discloses that configured disks
are still created one at a time, but not that lookups still queue behind them. This repository's
CAS environment declares its disk in
cas/configs/clickhouse/config.d/storage.xml with <metadata_type>cas</metadata_type>, so the
regression suite exercises the path the fix does not cover.
Fix direction. Either route configured-disk creation through getOrCreateDisk as well, or
move the factory.create calls in initialize and updateFromConfig outside the lock with the
same in-flight map.
Regression test direction. Seed a stale mount for a disk declared in XML and assert that a
getDisksMap call during start returns inside the observation window — the configured-disk twin
of DiskLookupDoesNotWaitForAStaleMountObservation.

Recovery & rollback

Medium: a configuration reload inside a custom-disk creation drops every storage-policy change in
that reload.

Impact. The operator edits config.xml, the reload reports success for disks, and no policy
change from that reload is applied. Only an ERROR line says so, and the disk the reload was
supposed to add does not exist either. Tables are not told about new disks because
disks_to_reinit stays empty.
Anchor. The skip at DiskSelector.cpp:169-176 leaves the configured name out of the new
selector. Context.cpp:6975-6987 then passes that selector to
StoragePolicySelector::updateFromConfig, which builds the entire new selector in one
constructor call (StoragePolicy.cpp:522) and throws on the first policy naming a disk the
selector does not have. Context.cpp:6982-6986 catches that and logs "storage policies were not
applied", discarding the whole result. shared->merge_tree_disk_selector has already been
replaced by then.
Trigger. A reload that introduces a disk named X while a table load is creating
disk(name = 'X', ...) — the window is the full creation, which for a stale CAS mount is about
36.5 s. The configured name must be new to this reload, since names already in the map take the
applyNewSettings branch instead of the skip.
Why defect. The PR body states the consequence as "storage policies of that reload that use
it are not applied". StoragePolicySelector::updateFromConfig has no per-policy error handling,
so the actual blast radius is every policy in the reload. The reload is not retried; the operator
must trigger another one.
Fix direction. Drop only the policies that name a skipped disk, or refuse the whole reload
loudly rather than partially applying the disk half and silently dropping the policy half.
Regression test direction. Extend ConfigReloadSkipsADiskBeingCreated with a policy that
names the skipped disk, and assert what happens to an unrelated policy in the same reload. The
current test asserts only that the custom disk wins the name.

Observability & diagnosability

Low: every failed disk creation is now rethrown through copyMutableException, which slices
exception types that do not override rethrow().

Impact. The dynamic type of a creation failure is replaced by the nearest ancestor that
overrides rethrow(). For CAS that turns MountFencedException into a plain DB::Exception,
and the header that defines it states the contract it breaks.
Anchor. getDiskCreationResult (Context.cpp:6786-6796) is on both the waiter's and the
creator's return path. copyMutableException (Common/Exception.cpp:735-756) dispatches through
Poco::Exception::rethrow(), which for a type without its own override resolves to
DB::Exception::rethrow() (Common/Exception.h:198) and slices on throw *this.
MountFencedException (CasServerRoot.h:269-274) declares neither rethrow() nor clone(),
and its comment says "Catch sites match BY TYPE (catch (const MountFencedException &)), never
by code". It escapes Pool::open at CasPool.cpp:790-791 once max_fence_recoveries is spent,
inside the creator. Coordination::Exception (KeeperException.h:62-64) has the same shape and
additionally loses its code member.
Trigger. Any failed custom-disk creation, including the single-threaded case with no waiters
at all. For the CAS instance: the GC fences this server's own fresh lease four times during one
open.
Why defect. The behavior change is not confined to the shared-failure path the PR describes.
The PR body says "Other exceptions are rethrown unchanged", but every Poco-derived exception goes
through e.rethrow() and is sliced unless its own type overrides it; only non-Poco exceptions
reach the catch (...) arm, and there std::current_exception() hands back the same object, so
those are not copied either. No catch site above getOrCreateDisk matches
MountFencedException today, which is why this is Low rather than Medium — the loss is the type
name in the log and a trap for the next catch site.
Fix direction. Rethrow the creator's own local error directly instead of routing it through
the future, and add rethrow() and clone() overrides to MountFencedException.
Regression test direction. Throw a MountFencedException from a creator and assert
EXPECT_THROW(..., MountFencedException) for the creator and for a waiter.

Low: a disk that is being created is reported as Unknown disk.

Impact. For the whole creation window a name that is about to exist is indistinguishable from
one that never will. The message offers the operator nothing to wait for.
Anchor. Context::getDisk (Context.cpp:6772-6779) and getStoragePolicyFromDisk
(Context.cpp:6886-6887) consult only DiskSelector::get, which throws UNKNOWN_DISK
(DiskSelector.cpp:242-247); neither looks at disks_being_created.
Trigger. During the creation window, a statement that resolves that name by reference rather
than through a disk(...) function: MergeTreeData.cpp:868 and registerStorageMergeTree.cpp:858
via getStoragePolicyFromDisk, or MergeTreeSettings.cpp:2735 via
DiskFromAST::ensureDiskIsNotCustom.
Why defect. This is the intended trade and the gtest asserts the fast return, but the
diagnosis regressed with it. The old code blocked and then produced a specific message —
ensureDiskIsNotCustom would say "Disk name X is a custom disk that is used in other table"
(DiskFromAST.cpp:180-186). The new code says the disk does not exist.
Fix direction. Have Context::getDisk check disks_being_created and report that the disk is
being created.
Regression test direction. Call getDisk for a name whose creation is waiting out a stale
mount and assert the message names the in-flight creation.

Low: ConcurrentRequestsShareOneFailure always burns its full 5 s timeout.

Impact. Five seconds of every unit-test run, on every build, spent waiting for a condition
that cannot be signalled.
Anchor. gtest_cas_startup_stale_mounts.cpp:490 has the creator wait up to 5 s for
second_returned, but second_returned is set at :532, only after the second request returns —
which cannot happen until the creator throws. The predicate is unsatisfiable while the creator
holds the wait, so the wait always ends by timeout. The CI runs confirm it: 5006 ms under tsan,
5024 ms under asan_ubsan, against 0–3.7 s for the other five tests in the suite.
Trigger. Every run.
Why defect. The comment reads as though the wait normally ends early — "Gives the second
request time to join this creation; it returns early only if it did not join." It never returns
early. The test is correct and its assertion holds, it is the duration that is unintended.
Fix direction. Signal on the waiter having joined — a counter incremented when a caller takes
the in-flight future — rather than on the second request having returned.
Regression test direction. Not applicable; this is the test.

The timing bound in StaleMountsOfDifferentDisksAreWaitedOutConcurrently
(gtest_cas_startup_stale_mounts.cpp:348) asserts three parallel opens finish within
2 * observation_ms, a 6.9 s ceiling over a 3.45 s floor. That looked tight for a loaded
sanitizer runner, but the measured times are 3701 ms under tsan and 3767 ms under asan_ubsan —
about 46% headroom — so the margin is adequate on the evidence available.

Coverage

Class	Result
Network / transport	N/A. No timeout, retry or connection policy changed. The stale-mount wait itself is untouched.
Partition / availability	Reviewed. The lookup path is freed for custom disks; the configured-disk path is the first finding. Two disk names sharing one `cas_server_root_id` can now claim concurrently in one process, but `claimMountAwaitingExpiry` returns `LiveDoubleStart` on a body carrying our own uuid (`CasServerRoot.cpp:977-982`) and `Pool::open` fails closed with the same message the serialized order produced, so the outcome class is unchanged.
Timing / ordering	Reviewed. No clock, epoch or sequence change. Ordering between a reload and a creation is total: both phases that touch the map hold `storage_policies_mutex`.
Partial / gray failure	Reviewed. A creator that blocks forever still blocks its waiters, but they previously blocked on the mutex for the same duration, with the same absence of a timeout or a cancellation point.
Idempotency and duplicates	Reviewed. The in-flight map is what prevents a second open of one CAS root, and it is consulted and cleared under the lock. A retry after a failed creation runs a fresh creator, which is the pre-existing behavior.
Backpressure and resource limits	Reviewed. `disks_being_created` has no cap, but concurrency is bounded by the table-loader pool (`tables_loader_foreground_pool_size`, default 0 meaning one thread per CPU), so parallel CAS opens are bounded by core count rather than by table count.
Recovery and rollback	Reviewed. A failed creation leaves no entry and no disk. The reload interaction is the second finding. A disk whose `addToDiskMap` throws is destroyed without `shutdown()`, which leaves a CAS mount lease claimed for the life of the process — unchanged from the old code, which also let that `DiskPtr` go out of scope on the same throw.
Configuration and version skew	Reviewed. Single process; no wire format or peer parsing. The skip changes which side wins a config/custom name collision but both orders were already reachable before this PR.
Security / trust across nodes	N/A. No authentication or authorization change.
Observability	Reviewed. The last three findings are this class.

Assumptions: static reading of head c905ab39e7a, with the base taken as
antalya-26.6-20261008. CAS mount-claim semantics were read from CasPool.cpp and
CasServerRoot.cpp at that commit. No server, and no gtest, was run.

Operational readiness

Scope. Server start, configuration reload, and table loads on a server with CAS disks, at head
c905ab39e7a. Persona: operator / DBA.

Issues

1. Configured CAS disks still freeze the server during start

Symptom. Start takes about 36.5 s per stale mount and nothing resolves a disk or a policy
meanwhile. Identical to the behavior before this PR.
Trigger. CAS disks declared in <storage_configuration><disks> rather than with
disk(...). See the first finding.
Signal quality. The CasPool log says a stale-looking mount lease is being waited out, which
is accurate. Nothing says that the whole server is waiting with it.
Severity. Medium.
Recovery. Stop servers cleanly so a farewell is written. Lowering
cas_mount_lease_ttl_ms shortens the wait but must be the same on every server sharing the pool.

2. A reload during a table load can silently discard policy changes

Symptom. An error has occurred while reloading storage policies, storage policies were not applied: Unknown disk X, after which the disk and policy sections of config.xml and the
server's live state disagree until the next reload.
Trigger. Adding disk X to the configuration while a table is creating disk(name = 'X').
See the second finding.
Signal quality. Poor. The cause is a DiskSelector warning in a different log line, and
nothing connects the two.
Severity. Medium when a custom disk uses an explicit name =; unreachable otherwise, since
generated names carry the TMP_INTERNAL_DISK_PREFIX.
Recovery. Touch the configuration again to trigger a second reload once the table has
finished loading, then confirm with system.disks and system.storage_policies.

3. A disk in the middle of being created looks like a typo

Symptom. Unknown disk X for a name that exists a few seconds later.
Trigger. Referring to an in-flight custom disk by name. See the third Low finding.
Signal quality. Misleading; the message suggests a configuration error.
Severity. Low.
Recovery. Retry after the creation finishes. system.disks does not list the disk during the
window either, so there is no way to tell waiting from a real mistake except the CasPool log.

Readiness verdict

Mergeable for what it targets. The new code is the correct shape and its tests pin the behaviors
that matter. Operators should not read the changelog entry as "CAS disks no longer stall server
start" — it applies only to disk(...) disks, and the deployment this repository tests uses
configured disks (issue 1). Operators who give custom disks explicit names that collide with
configured names should expect a reload to be dropped rather than merged (issue 2).

Recommended fixes

Cover the configured-disk path, or state in the changelog entry that the fix applies only to
disk(...) disks.
Make the reload interaction all-or-nothing, or drop only the affected policies.
Rethrow the creator's own exception directly, and give MountFencedException a rethrow() and
a clone().
Report "being created" instead of Unknown disk for a name in disks_being_created.
Replace the 5 s join timeout in ConcurrentRequestsShareOneFailure with an observable waiter
count.

alsugiliazova · 2026-10-23T10:47:09Z

PR #2573 CI Verification

Verdict

No failure is caused by this PR. Approvable on CI grounds.

Seven failing checks resolve to five distinct causes, all of which reproduce on sibling PRs that
change unrelated code, or have no path from this diff to the failure. The PR's own six new unit
tests pass on both builds that ran them.

Summary

Category	Count	Failures
PR-caused regression	0	—
Pre-existing	4	Iceberg unit-test null dereference (2 jobs), CAS attach-partition contention, concurrent-delete row count (2 jobs)
Infrastructure	3	Stateless session-count warning, two Grype image scans
Cascade	0	—
Unknown	0	—

Funnel: 7 failing checks, 0 removed as cascade, 4 placed by sibling-PR comparison, 2 placed by
error text, 1 investigated individually.

The PR's own tests pass

All six tests in the new CASStartupStaleMounts suite pass on both builds that ran unit tests.

Test	tsan	asan_ubsan
`StaleMountsOfDifferentDisksAreWaitedOutConcurrently`	OK, 3701 ms	OK, 3767 ms
`DiskLookupDoesNotWaitForAStaleMountObservation`	OK, 3644 ms	OK, 3720 ms
`ConcurrentCreationsOfOneDiskShareOneOpen`	OK, 3653 ms	OK, 3645 ms
`FailedDiskCreationIsRetriedByTheNextRequest`	OK, 0 ms	OK, 0 ms
`ConcurrentRequestsShareOneFailure`	OK, 5006 ms	OK, 5024 ms
`ConfigReloadSkipsADiskBeingCreated`	OK, 1 ms	OK, 1 ms

The first test is the one that would have failed before the change: three stale mounts opened in
3.7 s rather than the 10.4 s that serialized waits would cost. The 5 s in
ConcurrentRequestsShareOneFailure is the test's own timeout, not a stall — see the audit.

Failures

Unit tests (asan_ubsan) and Unit tests (tsan) — `pre-existing`

Both jobs die in the same test, IcebergMetadataGenerator.ModifyColumnAfterItselfWithoutTypeIsNoop,
on a null DataTypePtr:

/ClickHouse/src/DataTypes/IDataType.h:380:67: runtime error: member call on null pointer of type 'DB::IDataType'
    #1 DB::isNothing(...) src/DataTypes/IDataType.cpp:520
    #2 DB::Iceberg::stripNothing(...) .../Iceberg/MultipleFileWriter.cpp:34
    #3 DB::Iceberg::checkUnknownTypeAllowed(...) .../Iceberg/Utils.cpp:554
    #4 DB::MetadataGenerator::generateModifyColumnMetadata(...) .../Iceberg/MetadataGenerator.cpp:845
    #5 IcebergMetadataGenerator_ModifyColumnAfterItselfWithoutTypeIsNoop_Test::TestBody()
       .../Iceberg/tests/gtest_iceberg_metadata_generator.cpp:829

The two jobs are the same defect seen through different instrumentation. UBSan reports the null
dereference and lets the process continue; the tsan build has no UBSan, so the same dereference
reaches the hardware and the run ends with SIGSEGV immediately after
[ RUN ] IcebergMetadataGenerator.ModifyColumnAfterItselfWithoutTypeIsNoop, at the same point in
the same test.

Evidence it is not this PR. The identical UBSan report — same test, same
IDataType.h:380:67, same gtest_iceberg_metadata_generator.cpp:829 frame — appears on
PRs #2570 and
#2594, verified by reading their
unit_tests_asan_ubsan.log directly, and the same check fails with the same summary line on
#2574 and
#2577. Those four PRs change CAS namespace
republication, blob-publication logging, server-root rejoin and ref-log commit messages — four
unrelated areas. This PR touches no Iceberg or data-type code.

It fails on every run rather than intermittently, so it is a pre-existing defect on the base
branch, not a flaky test. It deserves its own issue; it does not block this PR.

Regression release cas_alter_attach_3 — `pre-existing`

20 leaf scenarios fail under /alter/attach partition/part 3, across partition key datetime,
attach partition from with id, simple attach partition 2, update in partition and rbac. The
errors are CAS request-engine contention, not attach logic:

CAS write could not be committed (CAS _ckpt for namespace '...': persistent CAS contention, the checkpoint contribution was not published)
CAS write could not be committed (read of 'data/cas/ref_catalog': gave up at the policy deadline after 13 attempt(s)); retrying later. (NETWORK_ERROR)

Evidence it is not this PR. #2567 — a
decommission change, unrelated to disk creation — fails the same job with 28 entries carrying the
same signatures (persistent CAS contention, read of 'data/cas/ref_catalog': gave up at the policy deadline, CAS ref catalog update: gave up at the policy deadline). The job passes on
#2552 and on
#2614, the merge of antalya-26.6 into this
base. Failing on two unrelated PRs at comparable magnitude and passing on two others is the profile
of load-dependent contention in this suite.

No path from the diff. The suite creates its tables with storage_policy = 'cas_policy', a
policy defined in cas/configs/clickhouse/config.d/storage.xml. Configured disks are created by
DiskSelector::initialize and updateFromConfig, neither of which this PR changes.
Context::getOrCreateDisk, the only function whose behavior changes, is reached only from
DiskFromAST for disk(...) disks and is therefore not on this suite's path at all.

Regression release cas_lightweight_delete_4 and cas_s3_cache_lightweight_delete_4 — `pre-existing`

One scenario each, both under /lightweight delete/concurrent delete/MergeTree: random delete 75 percent of the table without overlap and random delete entire table without overlap. Both are
exact row-count assertions after concurrent deletes — 253 observed against 250 expected at
lightweight_delete/tests/concurrent_delete.py:352. No server exception; the deletes returned
success and three rows had not yet been removed when the count ran.

Evidence it is not this PR. cas_lightweight_delete_4 also fails on
#2614 and
#2567, and passes on
#2552. #2614 is the base-branch merge, which
carries none of this PR's changes. The same no-path argument as cas_alter_attach_3 applies: these
tables also use the configured cas_policy.

Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) — `infrastructure`

One test of 5721 fails: 03254_timeseries_functions, with Reason: having stderror. The stderr is
not an error from the test but a server-wide warning that landed in it:

<Warning> ConnectionGroup: Too many active sessions in group Disk, count 9300, warning limit 8000, next warning at 9400

The harness fails any test whose stderr is non-empty, so the test that happens to be running when
the session count crosses a 100-boundary is the one that fails. The test's own output is correct.

This is the only failure where the baseline initially pointed at the PR: the same job on the same
build and shard passes on all five sibling PRs, and the warning appears in none of their server
logs, each about 4.2 million lines. Rather than accept a 0-of-5 baseline as proof, the mechanism
was checked, and it rules the PR out:

The 16 warnings fall in a 3-second window, 18:33:18 to 18:33:21, and their query IDs are merge
tasks (...::202610_1060_1096_5, ...::202610_1_487_61), not disk operations. The spike is
concurrent merges opening S3 connections.
No disk was created anywhere near that window. The server log's 102 Starting up disk events
bracket it at 18:27:50 and 18:36:46 — a nine-minute gap containing every one of the warnings. The
code this PR changes did not run while the sessions accumulated.
The diff contains nothing in the HTTP connection pool, the S3 client or the merge path.

So the elevated session count is this run's load profile under ASAN, not an effect of the change.
Stated honestly: the category rests on the disproved mechanism, not on the baseline, and five
sibling runs is a small sample. A rerun of that single job is the cheap confirmation if a reviewer
wants the baseline to agree with the mechanism.

GrypeScanKeeper and GrypeScanServer (-alpine) — `infrastructure`

Container image CVE scans: 4 high/critical in clickhouse-keeper, 1 in clickhouse-server:*-alpine.
These scan published base images, not the diff. Both fail identically on
#2577 and
#2570.

What CI does not cover

Two gaps worth knowing before approving, both from the audit rather than from a failure:

The configured-disk path is untested for this change. Every CAS regression suite and the CAS
stateless runs use cas_policy from config.xml, so they exercise
DiskSelector::initialize, which still creates disks under storage_policies_mutex. The one
place the new code is covered is the six new gtests.
The reload interaction is covered only single-threaded. ConfigReloadSkipsADiskBeingCreated
calls updateStorageConfiguration from inside the creator on the same thread, which proves the
skip but not the cross-thread interleaving, and it does not assert what happens to storage
policies that name the skipped disk.

Recommendation

Approve on CI. No regression. The two blocking-category counts are zero.

Suggested follow-ups, none blocking this PR:

File an issue for IcebergMetadataGenerator.ModifyColumnAfterItselfWithoutTypeIsNoop — it
dereferences a null DataTypePtr on every run of the base branch and crashes the tsan unit-test
binary outright, which hides any test ordered after it.
Optionally rerun Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) so the
baseline agrees with the mechanism finding.
Treat the CAS attach-partition contention failures as the known suite-level issue they are on
CAS: decommission keeps a member's control objects; owner life and epoch nonce #2567 and this PR, and track them separately from PR review.

…eation - A reload during a custom disk creation skips only the storage policies that name that disk; they keep their previous definition and the other policy changes apply. - The creator of a failed disk rethrows its own exception; waiters get a copy. `MountFencedException` gets `clone` and `rethrow` so a copy keeps its type. - `getDisk` and `getStoragePolicyFromDisk` say "is being created" for a name whose custom disk is being created instead of "Unknown disk". - `ConcurrentRequestsShareOneFailure` waits for the joined waiter count instead of a 5 s timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>

The creator test now throws a type with no `rethrow`, so it fails if the creator rethrows a copy. The waiter counter accessor is named for tests, the skip warning names the disk, and the policy reload test no longer depends on earlier `--gtest_repeat` runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>

filimonov · 2026-11-04T06:26:23Z

Thanks for the audit. Fixed in e188b14 and b558a36:

Medium, config-defined CAS disks still initialise under the lock. Not changed in code; the PR now claims only what it does. The description and the changelog entry say the change covers custom disks created by disk(...); a stale mount of a config-defined disk still blocks lookups during startup.
Medium, a reload during a custom-disk creation dropped all policy changes. Fixed: the reload skips only the policies that name a disk being created as a custom disk; they keep their previous definition, the warning names the disk, and the other policy changes apply. A policy naming a disk that neither exists nor is being created still fails the reload. Test: ConfigReloadKeepsPoliciesThatDoNotNameTheDiskBeingCreated.
Low, exception slicing. The creator rethrows its own exception; only waiters get a copy. MountFencedException has clone and rethrow, so a waiter's copy keeps its type. Coordination::Exception has no rethrow, so a waiter of that failure still gets a DB::Exception copy. Tests: FailedCreationKeepsTheExceptionTypeForTheCreator (an exception type without rethrow), WaiterOfAFailedCreationGetsTheExceptionType.
Low, Unknown disk for an in-flight disk. getDisk and getStoragePolicyFromDisk now say the disk is being created by another request (same UNKNOWN_DISK code). Test: LookupOfADiskBeingCreatedSaysSo.
Low, ConcurrentRequestsShareOneFailure waited 5 s. It now waits for the count of joined waiters and takes 0 ms.

…be file Every disk probes its access with the same file name, clickhouse_access_check_<server uuid>. Since custom disks are created outside storage_policies_mutex (#2573), two tables whose custom disks sit on one backing disk are started in parallel at server start, and their probes overwrite and unlink each other's file: the tables fail to attach with FILE_DOESNT_EXIST "While checking access for disk ...". Seen in test_filesystem_cache on #2805 (tables test and test_size_in_name, cache disks over hdd_blob). A process-wide mutex around the probe; config disks are created one after another anyway. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>

filimonov and others added 4 commits October 15, 2026 07:41

filimonov added the CAS label Oct 15, 2026

svb-alt added antalya antalya-26.6 labels Oct 22, 2026

alsugiliazova added verified Approved for release verified-with-issues Verified by QA and issues found. labels Oct 23, 2026

alsugiliazova mentioned this pull request Oct 23, 2026

Rejoin a decommissioned CAS server root on server start #2574

Merged

11 of 31 tasks

filimonov changed the base branch from antalya-26.6-20261008 to antalya-26.6 October 24, 2026 11:30

filimonov assigned k-morozov Nov 3, 2026

k-morozov self-requested a review November 3, 2026 11:13

filimonov and others added 2 commits November 4, 2026 02:04

k-morozov approved these changes Nov 4, 2026

View reviewed changes

filimonov merged commit 52c95e2 into antalya-26.6 Nov 4, 2026
379 of 394 checks passed

filimonov mentioned this pull request Nov 6, 2026

Antalya 26.8: port the CAS PRs merged into antalya-26.6 after the PR 2540 cut #2805

Open

11 of 31 tasks

filimonov mentioned this pull request Nov 6, 2026

Serialize IDisk::checkAccess: disks created in parallel share the probe file #2812

Open

11 of 31 tasks

Navigation Menu

Repository navigation

Create custom disks outside storage_policies_mutex - #2573

Create custom disks outside storage_policies_mutex#2573
filimonov merged 6 commits into
antalya-26.6from
fix/antalya-26.6/cas-392-parallel-stale-mount-wait

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Uh oh!

Changelog category (leave one):

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

Documentation entry for user-facing changes

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

Uh oh!

Uh oh!

PR #2573 Audit Review

Verdict

What the change does

Model

Why the core hand-off holds

Findings

Partition / availability

Recovery & rollback

Observability & diagnosability

Coverage

Operational readiness

Issues

1. Configured CAS disks still freeze the server during start

2. A reload during a table load can silently discard policy changes

3. A disk in the middle of being created looks like a typo

Readiness verdict

Recommended fixes

Uh oh!

PR #2573 CI Verification

Verdict

Summary

The PR's own tests pass

Failures

Unit tests (asan_ubsan) and Unit tests (tsan) — pre-existing

Regression release cas_alter_attach_3 — pre-existing

Regression release cas_lightweight_delete_4 and cas_s3_cache_lightweight_delete_4 — pre-existing

Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) — infrastructure

GrypeScanKeeper and GrypeScanServer (-alpine) — infrastructure

What CI does not cover

Recommendation

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Unit tests (asan_ubsan) and Unit tests (tsan) — `pre-existing`

Regression release cas_alter_attach_3 — `pre-existing`

Regression release cas_lightweight_delete_4 and cas_s3_cache_lightweight_delete_4 — `pre-existing`

Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) — `infrastructure`

GrypeScanKeeper and GrypeScanServer (-alpine) — `infrastructure`