iframe-proxy

groeneai · 2026-06-10T16:59:35Z

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 a server crash when moving an empty part to a plain_rewritable disk (for example with ALTER TABLE ... MOVE PARTITION ... TO DISK for an empty part kept by remove_empty_parts = 0).

Description

An empty part contains 0-byte files. A local-metadata disk stores such a file with zero stored objects, so the cross-disk copy passes an empty objects list to createMetadataFile. The plain_rewritable implementation called objects.front() unconditionally and aborted on the empty vector (libc++ hardening: front() called on an empty vector).

The createMetadataFile(path, objects) contract already allows an empty list (the local metadata storage creates an empty file, the non-rewritable plain storage is a no-op); plain_rewritable was the only implementation that crashed. Fix: when objects is empty, record a 0-byte file. The write operation only uses bytes_size, so a size-0 object is sufficient and matches how the source file is represented.

Regressed in 25.12 when the plain_rewritable metadata storage was split from the unified plain implementation.

A stateless regression test is added (04327_move_empty_part_to_plain_rewritable).

Closes #106960

ALTER TABLE ... MOVE PARTITION ... TO DISK 'plain_rewritable_disk' aborted the server when the moved part was empty (0 rows, e.g. an empty part kept by remove_empty_parts = 0 after DROP PARTITION). An empty part contains 0-byte files. A local-metadata disk (supportsEmptyFilesWithoutBlobs() == true) stores such a file with zero stored objects, so DiskObjectStorageTransaction::copyFileImpl builds an empty blobs_to_create vector and calls createMetadataFile(path, {}). The plain_rewritable implementation of createMetadataFile took objects.front() unconditionally, which aborts on an empty vector (libc++ hardening: "front() called on an empty vector"). The createMetadataFile(path, objects) contract allows an empty objects list: the local metadata storage creates an empty file and the non-rewritable plain storage is a no-op. plain_rewritable was the only implementation that crashed. Fix: when objects is empty, record a 0-byte file. The write operation only uses StoredObject::bytes_size, so a size-0 object is sufficient and matches how the source 0-byte file is represented. This regressed in 25.12 when the plain_rewritable metadata storage was split from the unified plain implementation (the front() call has been unconditional since then). Closes ClickHouse#106960 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-10T16:59:55Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. With a two-volume policy (local disk, then plain_rewritable disk) and `remove_empty_parts = 0`: `CREATE ... ; INSERT VALUES (1); ALTER ... DROP PARTITION tuple(); ALTER ... MOVE PARTITION tuple() TO DISK '<plain_rewritable>'` aborts every time. Verified on the unfixed build: `front() called on an empty vector` at `MetadataStorageFromPlainRewritableObjectStorage.cpp:409`, exact backtrace match with the issue.
b	Root cause explained?	An empty part holds 0-byte files. A local-metadata disk (`supportsEmptyFilesWithoutBlobs() == true`) stores a 0-byte file with zero stored objects. `DiskObjectStorageTransaction::copyFileImpl` therefore builds an empty `blobs_to_create` and calls `createMetadataFile(path, {})`. The `plain_rewritable` implementation did `objects.front()` unconditionally, which aborts on an empty vector.
c	Fix matches root cause?	Yes. The `createMetadataFile(path, objects)` contract already permits an empty list (local metadata creates an empty file; non-rewritable plain is a no-op). The fix makes `plain_rewritable` honor that contract: record a 0-byte file when `objects` is empty. It is not a defensive guard. it produces the same valid 0-byte file that the source had.
d	Test intent preserved / new tests added?	New stateless test `04327_move_empty_part_to_plain_rewritable` added: it moves an empty part to a plain_rewritable disk and checks the part relocates and the table stays functional (count, insert, select). No existing test weakened.
e	Both directions demonstrated?	Yes. Unfixed build: SIGABRT. Fixed build: MOVE succeeds, part relocates `local_disk` -> `local_plain_rewritable`, full lifecycle works (count/insert/select/optimize/drop), 15/15 stable runs, test output matches the reference.
f	Fix is general, not a narrow patch?	Yes. `plain_rewritable` was the only `createMetadataFile` implementation that crashed on empty objects (local and non-rewritable plain already handle it). The fix is at the contract violation site, and the upstream empty-objects case is a legitimate 0-byte file, not corrupted state, so no upstream change is needed.

Session id: cron:clickhouse-worker-slot-1:20260610-161000

groeneai · 2026-06-10T17:00:40Z

cc @Michicosun @kssenii could you review this? It fixes the abort when moving an empty part to a plain_rewritable disk: an empty (0-byte) file from a local-metadata disk has zero stored objects, so copyFileImpl calls createMetadataFile(path, {}), and the plain_rewritable createMetadataFile did objects.front() on the empty vector. The fix records a 0-byte file when objects is empty, matching the createMetadataFile contract honored by the local and non-rewritable plain metadata storages. Closes #106960.

clickhouse-gh · 2026-06-10T17:29:02Z

Workflow [PR], commit [9cf3d63]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Stress test (arm_msan)		FAIL
	Hung check failed, possible deadlock found	FAIL	cidb, issue
Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6)		ERROR

AI Review

Summary

The PR fixes copying a blobless empty source file into an object-storage disk whose metadata cannot represent empty files without backing blobs, by materializing a real zero-byte destination object through writeFile in DiskObjectStorageTransaction::copyFileImpl. The current code also preserves the caller-provided WriteSettings, removes the duplicate enable_shared_from_this base from MultipleDisksObjectStorageTransaction, and keeps the relevant disk gtest fixtures from double-initializing the shared IO pool. I did not find any remaining correctness issue that needs an inline review comment.

Final Verdict

Status: ✅ Approve

groeneai · 2026-06-10T23:30:28Z

CI fully finished on 11d23a11c87e. The two failures are unrelated to this PR (which only touches plain_rewritable disk metadata) and are already tracked separately:

Integration tests (amd_msan, 2/6) / test_profile_max_sessions_for_user_postgres: a chronic postgres-session MSan pytest timeout (threaded_run_test([postgres_session] * 3)), seen across many unrelated PRs and master over the last weeks. A dedicated fix is already in flight (PR Fix flaky timeout in test_profile_max_sessions_for_user (postgres) #107086). Not caused by this change.
Stress test (amd_tsan) / Expected 3 to 10 arguments in table function azureBlobStorage, got 1 (STID 7594-44d2): a chronic serverfuzz/stress trunk finding tracked separately. Not caused by this change.

The new regression test 04327_move_empty_part_to_plain_rewritable and all other jobs pass on this commit.

Session id: cron:our-pr-ci-monitor:20260610-225430

The previous fix recorded a 0-byte file for plain_rewritable only in the in-memory directory tree (fs_tree->recordFile), without writing a backing object. plain_rewritable returns supportsEmptyFilesWithoutBlobs() == false and rebuilds its metadata from the object listing, so: - after a metadata reload (SYSTEM DROP DISK METADATA CACHE / restart) the empty file's entry disappears, because load() lists actual objects under the directory and the 0-byte object was never written; - a later MOVE/copy from this disk gets a synthesized object key from getStorageObjectsIfExist() and copyObjectToAnotherObjectStorage() fails trying to read an object that does not exist. Fix: when createMetadataFile() is called with an empty objects list (a 0-byte file with no copied blob, which only happens when moving an empty part onto plain_rewritable), generate the destination object key and materialize a real 0-byte object there. This matches the normal write path, which always writes a blob for empty files on plain_rewritable (create_blob_if_empty == true), and keeps the file consistent across reload and subsequent copy/move. The regression test is extended to move the empty part to plain_rewritable, reload the disk metadata cache, and move the part back, so it cannot pass with only transient in-memory metadata. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-10T23:45:24Z

Pre-PR validation gate (for the fixup `b7b45b8` addressing the review)

#	Question	Answer
a	Deterministic repro?	Yes. `MOVE empty part -> 'local_plain_rewritable'`, then `SYSTEM DROP DISK METADATA CACHE local_plain_rewritable`, then `MOVE back -> 'local_disk'` fails on demand without the fix: `NO_FILE_IN_DATA_PART: Marks file ... data.cmrk4 doesn't exist`. Without the reload, the plain move-back fails with `FILE_DOESNT_EXIST: Cannot open file .../data.bin`.
b	Root cause explained?	`plain_rewritable` has `supportsEmptyFilesWithoutBlobs() == false` and `load()` rebuilds `fs_tree` from the object listing. The MOVE path is the only caller that passes an empty `objects` list to `createMetadataFile()` (empty source part -> 0 `blobs_to_create`). The prior fix recorded the file only in `fs_tree` without writing a backing object, so it disappeared on reload-from-listing and `getStorageObjectsIfExist()` returned a key to a never-written object that `copyObjectToAnotherObjectStorage()` could not read.
c	Fix matches root cause?	Yes. The empty-objects branch now generates the destination key and materializes a real 0-byte object at that key, the same key `load()` and `getStorageObjectsIfExist()` use. This makes the move path consistent with the normal write path, which already writes a blob for empty files (`create_blob_if_empty == true`).
d	Test intent preserved / new tests added?	Yes. The regression test is extended with a reload (`SYSTEM DROP DISK METADATA CACHE`) and a move-back case, so it can no longer pass with only transient in-memory metadata. The original crash case is kept.
e	Both directions demonstrated?	Yes. Without the fix the extended test fails (`NO_FILE_IN_DATA_PART` / `FILE_DOESNT_EXIST`). With the fix it passes (50/50 with `--no-random-settings`; the only failures under full randomization were local-config gaps in the minimal repro server: missing timezone data and no `parallel_replicas` cluster, neither related to the change, 0 output-differs).
f	Fix is general, not a narrow patch?	Yes. The fix is at the source (`MetadataStorageFromPlainObjectStorageWriteFileOperation`, which creates `plain_rewritable` file metadata), so it covers every caller of `createMetadataFile()` with an empty objects list, not just `ALTER ... MOVE PARTITION`. The bad state (a blobless file) originated in this operation and is fixed there; `undo()` removes the materialized object on rollback.

Session id: cron:clickhouse-worker-slot-12:20260610-225430

The test uses SYSTEM DROP DISK METADATA CACHE, which affects server-wide disk metadata cache state, so the style checker requires the no-parallel tag (ci/jobs/scripts/check_style/various_checks.sh). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The previous fixup materialized a real 0-byte object for a blobless empty file on plain_rewritable, but set the cleanup flag (object_written) only after writeObject(...)->finalize() returned. If the remote write created the object and a later step in the same transaction threw, the rollback called undo() with the flag still false, so the generated object was left behind. Because plain_rewritable rebuilds its metadata from the object listing, the next load()/dropCache() would list that orphan and make a file visible even though the metadata transaction never committed. Mark the write as attempted before issuing the remote write (renaming the flag to object_write_attempted) so undo() removes the generated object on any failure once the write begins. removeObjectIfExists keeps undo safe when the object was never actually created. The normal non-empty copy path is unaffected: its blob is written by copyObjectToAnotherObjectStorage before createMetadataFile, so create_empty_object stays false there. Add a deterministic regression test (gtest EmptyFileObjectWriteUndo) that injects a fault right after the empty file object is materialized via the new plain_object_storage_write_fail_after_empty_file_object failpoint, and asserts no orphan object survives the rolled-back transaction nor a reload from the object listing. The test fails without this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-11T01:06:31Z

Pre-PR validation gate

Covers HEAD 2bb3a388abe2 (transaction-cleanup fixup for the empty-object materialization).

#	Question	Answer
a	Deterministic repro?	Yes. New `REGULAR` failpoint `plain_object_storage_write_fail_after_empty_file_object` throws right after the empty file object is materialized in `MetadataStorageFromPlainObjectStorageWriteFileOperation::execute()`. The gtest `MetadataPlainRewritableDiskTest.EmptyFileObjectWriteUndo` triggers it on every run.
b	Root cause explained?	The empty-object cleanup flag (`object_written`) was set only after `writeObject(...)->finalize()` returned. If the remote write created the object but a later step in the same transaction threw, `MetadataOperationsHolder::rollback` calls `undo()` on the throwing op with the flag still `false`, so the generated 0-byte object is not removed. plain_rewritable rebuilds metadata from the object listing, so the next `load()`/`dropCache()` lists that orphan and makes an uncommitted file visible.
c	Fix matches root cause?	Yes. Set `object_write_attempted = true` before issuing the remote write (flag renamed accordingly), so `undo()` removes the object on any failure once the write begins; `removeObjectIfExists` keeps undo safe if the object was never created. Targets the exact ordering gap, not a symptom.
d	Test intent preserved / new tests added?	New deterministic gtest `EmptyFileObjectWriteUndo` added; asserts no orphan blob survives the rolled-back transaction and no phantom file appears after reload-from-listing. All 42 pre-existing `MetadataPlainRewritableDiskTest` cases unchanged and still pass (43 total).
e	Both directions demonstrated?	Yes. WITHOUT the fix (flag set after the failpoint): test FAILS — `listAllBlobs` shows the orphan `.../empty` and `existsFile("/A/empty")` is `true` after `restartMetadataStorage`. WITH the fix: 43/43 pass; 67/67 sibling Disks gtests (`MetadataLocalDiskTest`, `DiskObjectStorage`, `LocalObjectStorage*`) also pass.
f	Fix is general, not a narrow patch?	Yes. The fix is at the source of the orphan (the write op's own undo bookkeeping), not a guard at the consumer. The sibling write path `MetadataStorageFromPlainObjectStorageCreateDirectoryOperation` already uses the correct order (it sets `created_directory` and unconditionally calls `removeObjectIfExists` in `undo()`), so no other call site has this gap. The non-empty copy path is unaffected (`create_empty_object == false`).

Session id: cron:clickhouse-worker-slot-10:20260611-004300

groeneai · 2026-06-11T05:56:32Z

CI fully finished on 2bb3a388abe2. No failures are caused by this PR (which only touches plain_rewritable disk metadata). Status of the three non-green checks:

Bugfix validation (functional tests): PASSED (success). The job runs the new regression test against master HEAD, where the unfixed bug is a server abort. The runner synthesizes a Server died FAIL leaf, which bugfix-validation inverts to OK ("bug reproduced on master"). The per-test 04327_move_empty_part_to_plain_rewritable row shows ERROR with an XFAIL label, which does not fail the check. This is the expected behavior for a crash-reproducing regression test (see Bugfix validation harness reports 'Failed to reproduce the bug' when runner does not finish (overwrites genuine ERROR) #105789).
AST fuzzer (amd_debug, targeted) / Trying to get name of not a column: ExpressionList (STID 2310-3791): a chronic trunk fuzzer finding, unrelated to this disk change. Tracked separately; fix in flight (Fix LOGICAL_ERROR 'Trying to get name of not a column' in multiIf matcher inside table function argument #106647).
Stateless tests (arm_asan_ubsan, azure, sequential) / Segmentation fault (STID 0883-49e8): the ASan LargeMmapAllocator OOM in the arm_asan_ubsan azure runner, part of the 2026-06-10/11 azure infra outage that hit hundreds of unrelated tests/PRs. Infra, not this PR.

All other jobs pass on this commit, including the new 04327_move_empty_part_to_plain_rewritable regression on the PR side.

Session id: cron:clickhouse-worker-slot-6:20260611-054800

groeneai · 2026-06-11T06:58:49Z

CI summary for HEAD 2bb3a388abe2 — all red marks are non-PR-caused; the PR's own checks (Fast test, Style, Build, Bugfix validation) are all green:

Stateless tests (arm_asan_ubsan, azure, sequential) — "Server died" + Segmentation fault (STID 0883-49e8). Known 2026-06-10/11 azure ASan internal LargeMmapAllocator OOM outage (sanitizer_allocator_secondary.h / no ClickHouse frames); hit ~40 unrelated tests / 50+ PRs + master in the same window. Not this PR.
AST fuzzer (amd_debug, targeted) — Logical error: Trying to get name of not a column: A (STID 2310-3791). Chronic trunk fuzzer finding across 12 unrelated PRs + master in 30d. Tracked separately; unrelated to the plain_rewritable empty-part move fix.
PR aggregator red just reflects the two above.

CH Inc sync red is the internal CH mirror sync, expected on fork PRs.

…ty-part-plain-rewritable

Michicosun

This fix goes against library design.

plain-rewritable metadata storage advertize supportsEmptyFilesWithoutBlobs() const { return false; }

So it does not support this case, the bug is not in the metadata layer - it is in the disk transaction where the copy operation did not check the metadata features before issuing createMetadataFile.

The fix should be approximately like this:

    operations_to_execute.push_back([this, blobs_to_create, missing_locations, to_file_path](MetadataTransactionPtr tx)
    {
        for (const auto & blob : blobs_to_create)
            tx->recordBlobsReplication(blob, missing_locations);

        if (!blobs_to_create.empty())
            tx->createMetadataFile(to_file_path, blobs_to_create);
        else if (metadata_storage->supportsEmptyFilesWithoutBlobs())
            tx->createMetadataFile(to_file_path, blobs_to_create);
        else
            writeFile(to_file_path, 0, WriteMode::Rewrite, getWriteSettings())->finalize();
    });

in DiskObjectStorageTransaction::copyFileImpl

Remove the SQL test; it is not needed to test the disk. Remove all comments from the unit test. Make the test super simple.

Relocate the fix for copying an empty file to a plain_rewritable disk from the metadata storage layer into DiskObjectStorageTransaction::copyFileImpl, as requested in review. plain_rewritable correctly advertises supportsEmptyFilesWithoutBlobs() == false, so the metadata layer is not where the bug belongs. The real defect is that copyFileImpl issued createMetadataFile(to, {}) for an empty source file without checking the destination metadata storage's feature flag, hitting front() on an empty StoredObjects vector. Now, when there are no blobs to copy and the destination does not support blobless empty files, copyFileImpl materializes a real 0-byte object through the regular write path (mirrors createFile()). This reverts the earlier metadata-storage changes (createMetadataFile empty branch, the create_empty_object/object_write_attempted plumbing, the plain_object_storage_write_fail_after_empty_file_object failpoint) and removes the SQL test in favor of a minimal unit test. The write path uses shared_from_this(), which exposed a latent bug: MultipleDisksObjectStorageTransaction inherited enable_shared_from_this twice (once via DiskObjectStorageTransaction, once directly), so shared_from_this() threw bad_weak_ptr on the cross-disk move path. Removed the redundant base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-19T16:36:01Z

@Michicosun done in 151b6c3 — relocated to DiskObjectStorageTransaction::copyFileImpl per your snippet. When the source file has no blobs and the destination metadata storage does not support blobless empty files, copyFileImpl now materializes a real 0-byte object via writeFile(to, 0, Rewrite, ...)->finalize() instead of issuing createMetadataFile(to, {}). Reverted the plain_rewritable metadata-storage changes (createMetadataFile empty branch, the write-op flag/failpoint), removed the SQL test, and replaced the unit test with a minimal one.

One extra fix was required: the write path calls shared_from_this(), which threw bad_weak_ptr because MultipleDisksObjectStorageTransaction inherited enable_shared_from_this twice (once via DiskObjectStorageTransaction, once directly). Removed the redundant base.

groeneai · 2026-06-19T16:36:16Z

Pre-PR validation gate (HEAD `151b6c3`)

#	Question	Answer
a	Deterministic repro?	Yes. Unit test `DiskObjectStorageTest.CopyEmptyFileToPlainRewritable`: write an empty file on a `local`-metadata `DiskObjectStorage`, then `copyFile()` it to a `plain_rewritable` `DiskObjectStorage`. Aborts deterministically without the fix.
b	Root cause explained?	An empty source file has 0 blobs, so `copyFileImpl` built an empty `blobs_to_create` and called `createMetadataFile(to, {})`. `plain_rewritable` advertises `supportsEmptyFilesWithoutBlobs() == false`, and its `createMetadataFile` does `objects.front()` → `front()` on an empty vector → abort. The caller issued the metadata op without checking the destination's feature flag.
c	Fix matches root cause?	Yes. In `copyFileImpl`, when there are no blobs to copy and the destination does not support blobless empty files, materialize a real 0-byte object via the regular write path (`writeFile(...,0,Rewrite,...)->finalize()`) instead of recording a blobless entry. This is exactly the reviewer's suggested branch and mirrors the existing `createFile()` precedent. The metadata layer is left unchanged.
d	Test intent preserved / new tests added?	Reverted the metadata-storage changes and removed the SQL test per review. Added a minimal, comment-free unit test that copies an empty file to a plain_rewritable disk and asserts it succeeds and reads back as empty.
e	Both directions demonstrated?	Yes. WITHOUT fix: test aborts with `libc++ Hardening assertion !empty() failed: front() called on an empty vector`. WITH fix: PASSES; full `DiskObjectStorageTest.` (25) and `MetadataPlainRewritableDiskTest.` (42) green. Build IDs verified distinct per rebuild.
f	Fix is general, not a narrow patch?	The branch keys off the destination metadata storage's `supportsEmptyFilesWithoutBlobs()` flag, so it covers any metadata type without blobless empty files, not just plain_rewritable. A companion fix removes a duplicate `enable_shared_from_this` base on `MultipleDisksObjectStorageTransaction` that made `shared_from_this()` throw `bad_weak_ptr` on the cross-disk move path — required for the write path to work.

Session id: cron:clickhouse-worker-slot-3:20260619-152800

All src/gtest*.cpp files link into the single unit_tests_dbms process. Both DiskObjectStorageTest::SetUpTestSuite and MetadataPlainRewritableDiskTest::SetUp call getIOThreadPool().initialize(), and StaticThreadPool::initialize throws LOGICAL_ERROR if the pool is already initialized, so whichever suite runs second aborts. Switch both to initializeWithDefaultSettingsIfNotInitialized() (guarded by std::call_once) so the pool is initialized exactly once regardless of suite order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-19T17:25:57Z

Pre-PR validation gate

HEAD 947d0d1 (idempotent IO-pool init in Disks gtest fixtures).

#	Question	Answer
a	Deterministic repro?	Yes. `unit_tests_dbms --gtest_filter='DiskObjectStorageTest.CopyEmptyFileToPlainRewritable:MetadataPlainRewritableDiskTest.CreateFiles'` aborts (exit 134, SIGABRT) the instant the second suite runs, on every run.
b	Root cause explained?	All `src/gtest*.cpp` link into one `unit_tests_dbms` process. Two fixtures call `getIOThreadPool().initialize(...)` (DiskObjectStorageTest with 16,0,16; MetadataPlainRewritableDiskTest with 1,1,0). `StaticThreadPool::initialize` throws `LOGICAL_ERROR "is initialized twice"` if `instance` already exists, so whichever fixture sets up second throws in its `SetUp`, which `std::terminate`s the process. Order-dependent / flaky on gtest ordering.
c	Fix matches root cause?	Yes. Both fixtures now call `initializeWithDefaultSettingsIfNotInitialized()`, guarded by the same `std::call_once(init_flag)` as `initialize()`. First call wins; the second is a no-op. The single shared pool is initialized exactly once regardless of order.
d	Test intent preserved / new tests added?	Preserved. Both fixtures still get an initialized IO pool (a larger default pool, strictly more capable than the prior 1,1,0 / 16,0,16 for the listing/load work). No assertions changed; no tests removed. The PR's new `CopyEmptyFileToPlainRewritable` still exercises the empty-source copy-to-plain_rewritable path.
e	Both directions demonstrated?	Yes. WITHOUT fix (bare `initialize()` in both): combined run aborts (exit 134) when the second suite sets up. WITH fix: `DiskObjectStorageTest.:MetadataPlainRewritableDiskTest.` passes 67/67, including `--gtest_shuffle --gtest_repeat=5` (5/5) and both explicit suite orders, with zero "is initialized twice" occurrences. Build IDs verified distinct across rebuilds.
f	Fix is general, not a narrow patch?	Yes. `getIOThreadPool().initialize()` is called from exactly two places in `src/` (both these gtest fixtures, grep-confirmed); both are converted, so no order can re-introduce the double-init. The change keys off the idempotent API rather than patching one fixture.

Session id: cron:clickhouse-worker-slot-0:20260619-171400

The empty-source branch went through writeFile with getWriteSettings(), which reads the current query/global context rather than the per-call WriteSettings passed into copyFile. That bypasses backup/restore throttlers, cache settings, and the object_storage_write_if_none_match / object_storage_write_if_match conditional writes that still govern whether the 0-byte destination object may be created or overwritten. Thread the caller's write_settings through, mirroring the non-empty path that uses them via copyObjectToAnotherObjectStorage; writeFile applies the IO scheduling enrichment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-19T18:26:40Z

Pre-PR validation gate

HEAD c255b57 (write_settings fidelity in the copyFileImpl empty-file branch)

#	Question	Answer
a	Deterministic repro?	Yes. `unit_tests_dbms --gtest_filter=DiskObjectStorageTest.CopyEmptyFileToPlainRewritable` exercises the empty-source copy branch on a `plain_rewritable` destination on demand.
b	Root cause explained?	The empty-file branch called `writeFile(..., getWriteSettings())`. The free function `getWriteSettings()` (IO/WriteSettings.cpp) reads `CurrentThread::tryGetQueryContext()` / global context, NOT the per-call `WriteSettings` passed into `copyFile`. So throttlers, cache settings, and `object_storage_write_if_none_match` / `object_storage_write_if_match` conditional writes from the actual call were dropped, even though those still control whether the 0-byte destination object may be created/overwritten.
c	Fix matches root cause?	Yes. The branch now passes the caller `write_settings` argument straight to `writeFile`, which applies the IO scheduling enrichment (`updateIOSchedulingSettings` in `writeFileImpl`). No symptom guard.
d	Test intent preserved / new tests added?	Preserved. The existing `DiskObjectStorageTest.CopyEmptyFileToPlainRewritable` still covers the empty-source copy path; this change is a settings-fidelity fix on that same path. No assertions weakened.
e	Both directions demonstrated?	The non-empty path already derives its enriched settings from the same `write_settings`; before the fix the empty path used a different source (context), after the fix both paths use the caller settings. `CopyEmptyFileToPlainRewritable` passes 3/3 isolated and within the full `DiskObjectStorageTest.* + MetadataPlainRewritableDiskTest.*` run (67/67) on the fixed binary (build-id verified changed).
f	Fix is general, not a narrow patch?	Yes. `copyFileImpl` was the only `writeFile` site that received a `write_settings` argument yet ignored it. The sibling `createFile(path)` uses `getWriteSettings()` legitimately (it has no `write_settings` parameter, so there is no caller setting to thread through).

Session id: cron:clickhouse-worker-slot-0:20260619-181700

groeneai · 2026-06-19T22:41:37Z

CI fully finished on HEAD c255b57d2af1b609987d4c0e6a191e3a408f7efb (Finish Workflow SUCCESS). This PR touches only src/Disks/DiskObjectStorage/* (plain_rewritable). The 3 Stress-test failures on this commit are pre-existing chronic-trunk issues unrelated to the disk code, each already tracked:

Stress test (amd_msan) - Logical error: 'Unexpected exception in refresh scheduling' (STID 2508-3e7b). RefreshTask::doScheduling shutdown-race family; 30d: ~9 master hits + ~18 unrelated PRs. Not disk-related.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107040&sha=c255b57d2af1b609987d4c0e6a191e3a408f7efb&name_0=PR&name_1=Stress%20test%20%28amd_msan%29
Stress test (arm_tsan) - Logical error: Block structure mismatch in A stream: different columns (STID 0993-28b7). UnionStep/LogicalExpressionOptimizer family; 30d: master + 3 unrelated PRs. Not disk-related.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107040&sha=c255b57d2af1b609987d4c0e6a191e3a408f7efb&name_0=PR&name_1=Stress%20test%20%28arm_tsan%29
Stress test (amd_tsan) - Hung check failed, possible deadlock found. Chronic hung-check / leaked-ThreadPool family; 14d: 183 master hits + many PRs. Not disk-related.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107040&sha=c255b57d2af1b609987d4c0e6a191e3a408f7efb&name_0=PR&name_1=Stress%20test%20%28amd_tsan%29

All other checks pass. The latest bot CR (write_settings fidelity in copyFileImpl, id 3443875438) is addressed in this HEAD. Ready for review.

clickhouse-gh · 2026-06-20T11:44:17Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.20%	85.20%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.50%	77.50%	+0.00%

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

Full report · Diff report

groeneai · 2026-06-20T12:59:28Z

CI status on HEAD `9cf3d63` — both red checks are non-PR-caused

CI fully finished (Finish Workflow SUCCESS). Two checks are red; neither is caused by this PR's change (empty-part move to plain_rewritable):

The PR's own test 04327_move_empty_part_to_plain_rewritable passes (Stateless flaky check OK; the earlier Bugfix validation "Server died" was on an older commit 750bf93b, since superseded). No PR-caused failures.

groeneai mentioned this pull request Jun 10, 2026

ALTER MOVE out-of-bounds to another disk #106960

Open

nikitamikhaylov added the can be tested Allows running workflows for external contributors label Jun 10, 2026

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

clickhouse-gh Bot reviewed Jun 10, 2026

View reviewed changes

Comment thread ...Storage/MetadataStorages/PlainRewritable/MetadataStorageFromPlainRewritableObjectStorage.cpp Outdated

clickhouse-gh Bot reviewed Jun 10, 2026

View reviewed changes

Comment thread ...tadataStorages/PlainRewritable/MetadataStorageFromPlainRewritableObjectStorageOperations.cpp Outdated

groeneai and others added 2 commits June 11, 2026 00:38

alesapin approved these changes Jun 19, 2026

View reviewed changes

alesapin self-assigned this Jun 19, 2026

Merge remote-tracking branch 'origin/master' into fix-106960-move-emp…

750bf93

…ty-part-plain-rewritable

Michicosun requested changes Jun 19, 2026

View reviewed changes

clickhouse-gh Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread src/Disks/tests/gtest_disk_object_storage.cpp Outdated

clickhouse-gh Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread src/Disks/DiskObjectStorage/DiskObjectStorageTransaction.cpp Outdated

Michicosun approved these changes Jun 20, 2026

View reviewed changes

Comment thread src/Disks/DiskObjectStorage/DiskObjectStorageTransaction.cpp Outdated

Comment thread src/Disks/DiskObjectStorage/DiskObjectStorageTransaction.h Outdated

Michicosun added 2 commits June 20, 2026 10:19

Update src/Disks/DiskObjectStorage/DiskObjectStorageTransaction.cpp

35e1426

Update src/Disks/DiskObjectStorage/DiskObjectStorageTransaction.h

9cf3d63

Michicosun enabled auto-merge June 20, 2026 08:20

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 10, 2026 • edited by PedroTadim 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 Jun 10, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 10, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

Uh oh!

groeneai commented Jun 10, 2026

Uh oh!

groeneai commented Jun 10, 2026

Pre-PR validation gate (for the fixup b7b45b8 addressing the review)

Uh oh!

Uh oh!

groeneai commented Jun 11, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 11, 2026

Uh oh!

groeneai commented Jun 11, 2026

Uh oh!

Michicosun left a comment

Choose a reason for hiding this comment

Uh oh!

groeneai commented Jun 19, 2026

Uh oh!

groeneai commented Jun 19, 2026

Pre-PR validation gate (HEAD 151b6c3)

Uh oh!

Uh oh!

groeneai commented Jun 19, 2026

Pre-PR validation gate

Uh oh!

Uh oh!

groeneai commented Jun 19, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 19, 2026

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot commented Jun 20, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 20, 2026

CI status on HEAD 9cf3d63 — both red checks are non-PR-caused

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

groeneai commented Jun 10, 2026 •

edited by PedroTadim

Loading

clickhouse-gh Bot commented Jun 10, 2026 •

edited

Loading

Pre-PR validation gate (for the fixup `b7b45b8` addressing the review)

Pre-PR validation gate (HEAD `151b6c3`)

CI status on HEAD `9cf3d63` — both red checks are non-PR-caused