Skip to content

Create custom disks outside storage_policies_mutex - #2573

Open
filimonov wants to merge 4 commits into
antalya-26.6from
fix/antalya-26.6/cas-392-parallel-stale-mount-wait
Open

filimonov wants to merge 4 commits into
antalya-26.6from
fix/antalya-26.6/cas-392-parallel-stale-mount-wait

Conversation

@filimonov

Copy link
Copy Markdown
Member

Run the creator of a disk(...) custom disk outside the server-wide storage_policies_mutex.

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. Each caller gets its own copy of a Poco::Exception; the dynamic type is kept for exception types that override rethrow (ErrnoException, NetException). Other exceptions 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.
  • 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.
  • 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 of that reload that use it are not applied.

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, each gets its own ErrnoException copy.
    • A configuration reload during a creation does not create the configured disk of the same name.

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.

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:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

filimonov and others added 4 commits October 15, 2026 07:41
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

Copy link
Copy Markdown

Workflow [PR], commit [c905ab3]

@alsugiliazova

Copy link
Copy Markdown
Member

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(...), 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

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

@alsugiliazova

Copy link
Copy Markdown
Member

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:

  1. 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.
  2. 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:

  1. 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.
  2. Optionally rerun Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) so the
    baseline agrees with the mechanism finding.
  3. 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.

@alsugiliazova alsugiliazova added verified Approved for release verified-with-issues Verified by QA and issues found. labels Oct 23, 2026
@filimonov
filimonov changed the base branch from antalya-26.6-20261008 to antalya-26.6 October 24, 2026 11:30

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

antalya antalya-26.6 CAS verified Approved for release verified-with-issues Verified by QA and issues found.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants