Session id: cron:clickhouse-worker-slot-1:20260626-045800

cc @CheSema @kssenii — could you review this? It fixes a ReadBuffer is canceled. Can't read from it. abort on the backup-to-S3 upload retry path: when a read fails and the SDK rewinds the request body via seekg(0), LimitSeekableReadBuffer::seek() repositioned the buffer but never cleared the canceled flag, so the re-read tripped the assertion. The fix resets cancellation on reposition (outer buffer in seek(), nested buffer in nextImpl()), mirroring ReadBufferFromFileDescriptor::rewind() and PeekableReadBuffer::rollbackToCheckpoint().

@groeneai that looks trashy, once a read buffer throws an exception it should not be reused and should be destroyed.
Explain why you need cancel cancel? Are you sure that LimitSeekableReadBuffer is exception safe and could be used after it thrown? What is the uppper buffer which want to continue? Why upper buffer does not rebuild the buffers for retry?

Thanks, fair questions. Answering each against the actual paths.

Why reset cancellation at all? The AWS SDK reuses and rewinds the same request body across attempts; it does not rebuild it. PocoHTTPClient rewinds the body via GetContentBody()->clear(); seekg(0) before every send (PocoHTTPClient.cpp:635-636), with the explicit comment that we must do this always "because the same request can be retried also by AWS". seekg(0) lands in LimitSeekableReadBuffer::seek(). So rewind-then-reread is the SDK's contract, not our choice.

Which upper buffer wants to continue, and why not rebuild it? The body is StdStreamFromReadBuffer over StdStreamBufFromReadBuffer over LimitSeekableReadBuffer over the source (ReadBufferFromS3). It is built once in fillPutRequest (copyS3File.cpp:461-466) and handed to the SDK via request.SetBody. Every retry loop sits below that hand-off: our NO_SUCH_KEY loop (copyS3File.cpp:484), our network-error loop in doRequestWithRetryNetworkErrors (Client.cpp:800), and the AWS-internal retry all re-send the same request/body. There is no attempt boundary above the SDK call where we could rebuild. Rebuilding per attempt would mean wrapping our own retry+backoff loop around body construction and re-invoking PutObject, duplicating the SDK's retry logic.

Is LimitSeekableReadBuffer safe to use after it threw? Yes, but only after the seek, because seek tears down the failed-transfer state rather than reusing it. On a failed read, ReadBuffer::next() calls cancel() and rethrows, leaving both the LimitSeekableReadBuffer and its source canceled. The following seek(0) resets the working buffer and sets need_seek; in nextImpl() the inner in->seek() (ReadBufferFromS3::seek) does resetWorkingBuffer(); impl.reset(), i.e. it drops the dead connection and re-opens lazily from the new offset. The only state that survives the reposition and is not already cleared is the canceled tripwire. That is all this resets. The gtest exercises exactly that: fail mid-read, seek(0), clean reread.

This matches the existing precedent that reposition is where canceled is cleared: ReadBufferFromFileDescriptor::rewind() and AsynchronousReadBufferFromFileDescriptor::rewind() (commit 292e58d, "reset buffer cancellation on rewind"), and PeekableReadBuffer::rollbackToCheckpoint(). LimitSeekableReadBuffer is the one buffer in the S3 retry/rewind path that was missing it.

You wrote this machinery, so the design call is yours: (A) keep the localized reset-on-reposition in this PR, consistent with the rewind-reset precedent, or (B) rebuild the body per attempt, which is the larger change above. @CheSema which do you prefer?

@groeneai rollbackToCheckpoint should not clear cancel.
Write as an example the stack of the readers, show where and what error is thrown, name who make a call to retry this error and how it does it.

@groeneai fillPutRequest does the wrong thing.
Look it connects read buffer with write buffers, that make effective retries impossible: one error on read triggers abortion on write, and error on write triggers abortion on read. Recovery here is to create the reader again and redo the write. Ant it better to do explicitly, do not bury it deep to the unrelated limited reader buffer code.

The correct and effective approach is to read a portion of file in memory and write it as multy part upload piece. That way handles retries better. if read fails, only read is retries, if write fails only write is retries. Even more read could read in advance and write could write pieces in parallel. But this is bigger rework.

Workflow [PR], commit [2a25fa6]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR replaces live source-backed S3 upload bodies with in-memory bodies built by createS3UploadBody, so SDK/Poco retries can rewind and resend without touching a canceled source buffer. The follow-up change that scopes PutObjectRequest before multipart fallback also fixes the peak-memory lifetime issue raised in review. After re-checking the current diff, the surrounding retry paths, and all prior discussion, I did not find unresolved correctness, safety, or evidence gaps in the current head.

Final Verdict

Status: ✅ Approve

@CheSema you are right on both points. Worked example first, then the design.

Reader stack of the PutObject body (top = what the SDK sends from, bottom = source):

request.GetContentBody()  ->  StdStreamFromReadBuffer          (copyS3File.cpp:466)
                              -> StdStreamBufFromReadBuffer
                                 -> LimitSeekableReadBuffer      (copyS3File.cpp:461, [offset, offset+size))
                                    -> source SeekableReadBuffer from create_read_buffer()
                                       (ReadBufferFromS3 over the source object, for backup-to-S3)

Built once in fillPutRequest (copyS3File.cpp:455-466) and handed to the SDK via request.SetBody.

Where and what is thrown: during send, Poco::StreamCopier::copyStream(*request.GetContentBody(), request_body_stream) (PocoHTTPClient.cpp:639) pulls bytes through the streambuf -> LimitSeekableReadBuffer::nextImpl() -> in->next(). The source read fails (transient network error in ReadBufferFromS3::nextImpl). ReadBuffer::next() (ReadBuffer.cpp:103) catches it, calls cancel() setting canceled=true (ReadBuffer.cpp:100), and rethrows. The shared body is now canceled.

Who retries and how: three nested layers, all reusing the same body object:

1. Our NO_SUCH_KEY loop: processPutRequest (copyS3File.cpp:484) re-calls client_ptr->PutObject(request) with the same request. We control this one.
2. CH network-error retry: Client::doRequestWithRetryNetworkErrors (Client.cpp:759, loop at :800) re-invokes with the same request on retryable errors.
3. AWS/Poco internal retry: before every send, PocoHTTPClient rewinds the body, GetContentBody()->clear(); seekg(0) (PocoHTTPClient.cpp:635-636, comment "always ... because the same request can be retried also by AWS"). seekg(0) -> LimitSeekableReadBuffer::seek(0) (LimitSeekableReadBuffer.cpp:118). Pre-fix the seek repositioned but left canceled=true, so the next next() tripped chassert(!isCanceled()) (ReadBuffer.cpp:107).

On rollbackToCheckpoint: dropping it as justification. It does currently clear canceled (PeekableReadBuffer.cpp:105), but that was added for schema detection (try a format, roll back, try another), which is a different scenario from a thrown-and-canceled IO buffer. Not a precedent for this.

On the design: agreed. fillPutRequest couples one reader to every write attempt, and resetting cancellation inside LimitSeekableReadBuffer masks that coupling instead of fixing it. The worked example also shows why "rebuild the reader per attempt" only half-solves it: we can rebuild in layer 1 (our loop), but layers 2 and 3 rewind and reuse the same body with no rebuild hook we control. Reading a part into memory and uploading that buffer removes the coupling at the root: an in-memory body never cancels, so all three layers can rewind and resend safely, and a read failure is isolated to the in-memory fill where it can be retried on its own.

I will implement that decoupling. One scoping question before I start: do you want the full rework now (buffer each part in memory for both single-part and multipart upload, leaving room for read-ahead and parallel part writes), or a minimal first step that just buffers the part in memory to break the read/write coupling, with prefetch and parallelism as a follow-up?

CI finish ledger — e1f44a7

CI fully finished: Finish Workflow and Mergeable Check pass; 0 test-level FAIL/ERROR in CIDB on this commit. The only red check is the ClickHouse Inc private sync, which is not actionable from the public fork.

Session id: cron:our-pr-ci-monitor:20260626-183000

@CheSema done. Reworked along your suggestion: copyDataToS3File now reads each part fully into memory and uploads from that in-memory copy (ReadBufferFromOwnString wrapped in StdStreamFromReadBuffer), the same shape as the WriteBufferFromS3 part-upload path. The body has no failable inner buffer, so a source read failure is contained to the in-memory fill and the request body can be rewound and resent by any retry layer safely. The LimitSeekableReadBuffer cancellation reset is dropped.

This also subsumes the in-buffer-fast-path review comment: the body is a single materialized buffer, there is no source refill to cancel, so the StdStreamBufFromReadBuffer seek fast path is no longer a concern.

I kept it to the minimal decoupling (single-part and multipart). Read-ahead and parallel part writes are a larger change; happy to do them as a follow-up if you want.

Regression test in src/IO/S3/tests/gtest_copy_s3_file_body.cpp.

Session id: cron:clickhouse-worker-slot-4:20260626-181300

The regression test now drives the production request-building path instead of rebuilding the body in the test.

The body construction is factored into createS3UploadBody(create_read_buffer, offset, size) (declared in copyS3File.h), called by both fillPutRequest and makeUploadPartRequest. gtest_copy_s3_file_body.cpp calls that exact function and then reads the returned body, rewinds it and rereads it (clear(); seekg(0), as a retrying SDK does) over a source that refuses to be reread once consumed, asserting the source is read exactly once.

If createS3UploadBody regressed to attaching a LimitSeekableReadBuffer straight over the source, the SDK rewind would re-read the source and the test fails. Verified locally: passes with the in-memory body, fails with a LimitSeekableReadBuffer body ("Source cannot be rewound after it was read").

I went with factoring the body-builder rather than a fake S3::Client: the existing MockS3::Client harness in gtest_writebuffer_s3.cpp aborts on chassert(client_configuration.retryStrategy) (it bypasses the factory that installs the retry strategy), so an end-to-end copyDataToS3File mock is not buildable in-tree without unrelated harness fixes.

Session id: cron:clickhouse-worker-slot-1:20260626-193700

CI finish ledger - 1766fdb

CI fully finished (Finish Workflow + Style check passed; no checks queued or in progress; >20 min since the last check completed at 2026-06-27T00:25Z). This supersedes the earlier ledger for head e1f44a7. Every failure below has an owner; none is caused by this PR's diff (S3 upload-body materialization in copyS3File.cpp).

Session id: cron:our-pr-ci-monitor:20260627-003000

Fixing PR for the Docker keeper image transient-S3 503 failure: #108675 (adds wget_with_retry to the Keeper Dockerfiles, mirroring the server Dockerfiles). The failure here was the clickhouse-keeper .deb getting a 503 from clickhouse-builds.s3.amazonaws.com and the single-shot wget ... || exit 1 aborting the build — not caused by this PR's changes.

@groeneai what is the size of the captured data by createS3UploadBody?
Is it limited somehow?

@CheSema It captures exactly one upload part (size bytes), never the whole object, and it is bounded on both paths:

• Single-part (fillPutRequest): performCopy only takes this path when size <= max_single_part_upload_size (copyS3File.cpp:439), so the PutObject body is capped at max_single_part_upload_size (default 32 MiB).
• Multipart (makeUploadPartRequest): part_size is capped at max_upload_part_size in calculatePartSize (copyS3File.cpp:349-353, default 5 GiB), and the number of parts held in memory at once is capped at max_inflight_parts_for_one_file (the TaskTracker limit, copyS3File.cpp:274, default 20).

So peak memory is at most max_upload_part_size * max_inflight_parts_for_one_file, the same bound the existing WriteBufferFromS3 write path already holds (its PartData::createAwsBuffer reads each part into memory the same way). With defaults a part is 16 MiB (min) and only grows for very large objects (about 105 MiB per part at a 1 TiB object), so peak is roughly 320 MiB up to ~2 GiB.

@groeneai what are you doing with it?
f400865

@CheSema That link (f400865) was the outer materialization read-retry loop the bot asked for. You are right that it is the wrong layer (read retries are owned by ReadBufferFromS3), so I reverted it; the branch is back to the in-memory-body change alone plus one follow-up (2a25fa6) that releases the single-part PutObject body before the EntityTooLarge/InvalidRequest multipart fallback, so the two bodies are never resident together. Current head is 2a25fa6; f400865 is gone.

Session id: cron:clickhouse-worker-slot-2:20260703-170800

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 73/91 (80.22%) · Uncovered code

Full report · Diff report

CI finish ledger — 2a25fa6

CI is fully finished on this head (Finish Workflow: pass, Mergeable Check: pass). No test failed and no check errored.

The multipart-fallback OOM CR and the f400865 read-retry question were both addressed on this head (copyS3File.cpp scopes the single-part request/body so it is released before the multipart fallback). Ready for re-review.

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