Report cancellation instead of a network error for cancelled S3 requests by jkartseva · Pull Request #108673 · ClickHouse/ClickHouse

jkartseva · 2026-06-27T00:49:51Z

When a query issuing S3 requests is cancelled mid-request (e.g. by KILL QUERY), the retry loop returned the last network error, so it was reported as S3_ERROR instead of cancelled. This affects any cancelled S3 request, including backups and restores. Surface QUERY_WAS_CANCELLED instead.

Closes: https://github.com/ClickHouse/clickhouse-private/issues/45444

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

A query reading from or writing to S3 that is cancelled (e.g. by KILL QUERY) while an S3 request is in flight is now reported as cancelled instead of with a misleading network/S3 error. In particular, a cancelled backup or restore to S3 now reports BACKUP_CANCELLED/RESTORE_CANCELLED in system.backups.

Version info

Merged into: 26.7.1.568 (included in 26.7 and later)

clickhouse-gh · 2026-06-27T00:50:29Z

Workflow [PR], commit [ef5f275]

Summary: ❌

job_name	test_name	status	info	comment
Stateless tests (amd_debug, parallel)		FAIL
	04061_spilling_hash_join_overflow_limits	FAIL	cidb, issue	ISSUE EXISTS
Stateless tests (arm_binary, parallel)		FAIL
	02346_text_index_parallel_replicas	FAIL	cidb	IGNORED

AI Review

Summary

This PR routes failed S3 outcomes through CurrentThread::checkIfNotCancelled() so cancelled requests are reported as cancellation instead of stale S3/network errors, and it adds backup/restore plus s3() regression coverage. The main failed-outcome paths are covered, but the read contract is still incomplete for GetObject body-stream failures, and one new mock-control helper regresses the integration harness back to an unbounded curl call.

Findings

⚠️ Major: [src/IO/S3/Client.cpp:711] The new cancellation translation only runs when doRequest() gets a failed AWS outcome. After a successful GetObject, the response body is read later in ReadBufferFromS3::nextImpl() / readBigAt(), and that retry loop (ReadBufferFromS3::processException()) never checks whether the query was killed before retrying or surfacing the last network error. A KILL QUERY during an in-flight download can therefore still end as S3_ERROR for SELECT FROM s3(...) / RESTORE FROM S3 when the failure happens while consuming the body rather than while building the AWS outcome.
Suggested fix: translate cancellation in ReadBufferFromS3's read retry loop as well, for example by calling CurrentThread::checkIfNotCancelled() before retrying and before returning the final stream-read exception.
⚠️ Major: [tests/integration/helpers/s3_mocks/broken_s3.py:135] setup_slow_get_answers() bypasses _apply() and calls curl without bounded retry or timeout. If the mock is briefly unreachable or overloaded, this helper can fail on the first transient refusal or hang until the container's TCP timeout, which makes the new cancellation tests flaky.
Suggested fix: reuse _apply(url) here so slow_get gets the same capped retry behavior as the other mock-control endpoints.

Final Verdict

Changes requested.

When a query issuing S3 requests is cancelled mid-request (e.g. by KILL QUERY), the retry loop returned the last network error, so it was reported as S3_ERROR instead of cancelled. This affects any cancelled S3 request, including backups and restores. Surface QUERY_WAS_CANCELLED instead. Closes: ClickHouse/clickhouse-private#45444 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Throwing a fresh QUERY_WAS_CANCELLED when the S3 retry loop stops on a killed query loses the real cause: a query cancelled by max_execution_time should be TIMEOUT_EXCEEDED, and a query cancelled with a stored exception (e.g. a backup/restore coordination failure on another host) should rethrow it. Use QueryStatus::throwIfKilled when a process-list element is available, falling back to a fresh QUERY_WAS_CANCELLED only when there is none. Add a max_execution_time backup test that keeps failing S3 uploads (retryable) until the timeout fires, proving the S3 layer reports TIMEOUT_EXCEEDED rather than QUERY_WAS_CANCELLED. Shorten the slow-answer timeouts in the cancellation tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Route HeadObject through headObjectInternal so the wrong-region and redirect retries also report cancellation instead of a stale S3/network outcome. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`throwIfQueryWasCanceled` runs after any failed S3 outcome. It previously called `Context::getProcessListElement`, which throws `LOGICAL_ERROR` when the `Context` still exists but the process-list entry has expired during query start/finish (the same edge that made `Context::isCurrentQueryKilled` use the safe accessor). That could replace the original S3/network error with an unrelated logical exception at a normal query-lifecycle edge. Switch to `getProcessListElementSafe`, which returns a null `QueryStatusPtr` instead of throwing. When the safe element is absent the existing `ThreadStatus::isQueryCanceled` fallback still reports the generic `QUERY_WAS_CANCELLED`, so a real cancellation is preserved while ordinary S3 failures are no longer turned into `LOGICAL_ERROR`. Addresses review feedback on #108673 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This pull request fixes misreporting of cancelled in-flight S3 operations by translating the stale final S3/network failure outcome into a proper query cancellation error (QUERY_WAS_CANCELLED). This improves user-facing error reporting for cancelled S3-backed operations (notably backups/restores).

Changes:

Add a cancellation check in the S3 client retry/redirect paths to surface query cancellation (and preserve the true cancellation cause via the process-list element when available).
Wrap HeadObject redirect/region handling so failed outcomes also report cancellation correctly.
Add integration regression tests for cancelled async S3 backup/restore, plus a test ensuring timeout cancellations still report TIMEOUT_EXCEEDED.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`tests/integration/test_backup_restore_s3/test.py`	Adds regression coverage for cancelled async S3 backup/restore and ensures timeout cancellation cause is preserved.
`tests/integration/helpers/s3_mocks/broken_s3.py`	Extends the S3 mock to support slow GET/HEAD responses for reliable in-flight restore cancellation testing.
`src/IO/S3/Client.h`	Declares internal `HeadObject` helper to centralize retry/redirect handling and enable consistent cancellation surfacing.
`src/IO/S3/Client.cpp`	Implements cancellation surfacing in S3 request loops and `HeadObject` failure handling, preserving timeout/kill causes via process list.

Introduce a reusable static method that throws the real cancellation cause of the current query (`TIMEOUT_EXCEEDED`, or an exception stored by `QueryStatus::cancelQuery`) instead of a generic error, falling back to `QUERY_WAS_CANCELLED` when the process-list element is gone. It is backed by a new `ThreadStatus` predicate set alongside `query_is_canceled_predicate`. Review: #108673 (review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Replace the local `throwIfQueryWasCanceled` helper with the shared `CurrentThread::checkIfNotCancelled`, called after a failed S3 outcome in `headObject` and `doRequestWithRetryNetworkErrors`. Drop the now-unused `ProcessList.h` include and the `QUERY_WAS_CANCELLED` error code, whose logic moved into `CurrentThread`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The fallback in CurrentThread::checkIfNotCancelled consulted the same process-list element as the primary check, so a cancellation was lost once the entry expired during query teardown. Record the killed state on the thread group (which outlives the entry) via QueryStatus::markKilled, and let ThreadStatus::isQueryCanceled consult this durable flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

This reverts commit 2bc98de.

Michicosun

LGTM, but maybe we need to remove most of the comments; in most cases, they're not needed since the method names are good enough.

clickhouse-gh · 2026-07-06T20:19:53Z

        if (result.IsSuccess())
            return result;

+        CurrentThread::checkIfNotCancelled();


CurrentThread::checkIfNotCancelled() only translates failed AWS outcomes returned from doRequest(), but GetObject body reads still bypass it. Once client_ptr->GetObject(req) succeeds, ReadBufferFromS3::nextImpl() / readBigAt() handle stream timeouts and resets in their own retry loop (ReadBufferFromS3::processException()), and that path never checks whether the query was killed before retrying or surfacing the last network error. A KILL QUERY during an in-flight download can therefore still end as S3_ERROR for SELECT FROM s3(...) / RESTORE FROM S3 when the failure happens while consuming the response body rather than while creating the AWS outcome, so the read-path contract of this PR is still incomplete.

A legit suggestion. The fix belongs in ReadBufferFromS3::processException. I'll take a closer look and file a follow-up PR if needs be, since ReadBufferFromS3 is a different subsystem.

clickhouse-gh · 2026-07-06T20:19:53Z

+        if count is not None:
+            url += f"&count={count}"
+
+        response = self._cluster.exec_in_container(


setup_slow_get_answers() bypasses _apply() and invokes curl with no bounded retry or timeout. The rest of MockControl uses _apply() specifically because the mock can be briefly unreachable or overloaded; here the new helper can fail on the first transient refusal or hang until the container's TCP timeout, which makes the new cancellation tests flaky in exactly the failure mode they are trying to exercise. Please route this through _apply(url) as well so the slow_get control path inherits the same capped retry behavior.

There is no _apply() in broken_s3.py. Every MockControl method – reset, setup_action, setup_fake_puts, and the pre-existing setup_slow_answers this mirrors – issues the same single exec_in_container(["curl", "-s", url], nothrow=True) call.

clickhouse-gh · 2026-07-07T00:12:29Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.60%	85.60%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	77.80%	77.80%	+0.00%

Changed lines: Changed C/C++ lines covered: 27/32 (84.38%) · Uncovered code

Full report · Diff report

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

clickhouse-gh Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread src/IO/S3/Client.cpp Outdated

Comment thread src/IO/S3/Client.cpp Outdated

jkartseva force-pushed the jkartseva/backup-restore-cancel-error branch from 8674fec to 61b41ab Compare June 27, 2026 01:45

jkartseva and others added 2 commits June 27, 2026 03:11

Apply S3 cancellation check to all HeadObject retry paths

7017ae4

Route HeadObject through headObjectInternal so the wrong-region and redirect retries also report cancellation instead of a stale S3/network outcome. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread src/IO/S3/Client.cpp Outdated

jkartseva requested a review from Copilot June 29, 2026 20:23

Copilot started reviewing on behalf of jkartseva June 29, 2026 20:23 View session

Copilot AI reviewed Jun 29, 2026

View reviewed changes

alexey-milovidov mentioned this pull request Jul 1, 2026

Make RestorerFromBackup use its own context copy, not the live query context #108765

Merged

Michicosun reviewed Jul 2, 2026

View reviewed changes

Comment thread src/IO/S3/Client.cpp Outdated

Michicosun self-assigned this Jul 2, 2026

jkartseva and others added 2 commits July 3, 2026 03:55

Copilot started work on behalf of jkartseva July 3, 2026 04:02 View session

Fix style check: put lambda opening brace on its own line

afb7417

Copilot finished work on behalf of jkartseva July 3, 2026 04:26

jkartseva marked this pull request as draft July 3, 2026 05:22

clickhouse-gh Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread src/Interpreters/ProcessList.cpp

Michicosun reviewed Jul 3, 2026

View reviewed changes

Comment thread src/Interpreters/ProcessList.cpp Outdated

jkartseva added 3 commits July 3, 2026 22:54

Revert "Preserve cancellation after the process-list entry expires"

d48aa66

This reverts commit 2bc98de.

Remove unreachable fallback from CurrentThread::checkIfNotCancelled

98a12e9

Add tests for cancellation reporting of killed s3() queries

dd50ae1

jkartseva marked this pull request as ready for review July 4, 2026 02:39

Michicosun approved these changes Jul 6, 2026

View reviewed changes

Shorten documentation

ef5f275

clickhouse-gh Bot reviewed Jul 6, 2026

View reviewed changes

jkartseva added this pull request to the merge queue Jul 7, 2026

Merged via the queue into master with commit 9728786 Jul 7, 2026
172 of 175 checks passed

jkartseva deleted the jkartseva/backup-restore-cancel-error branch July 7, 2026 02:29

robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 7, 2026

jkartseva mentioned this pull request Jul 23, 2026

RESTORE: max_execution_time cancellation doesn't propagate into the S3 client retry loop - teardown reports a misleading "connect timed out" error #108120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Report cancellation instead of a network error for cancelled S3 requests#108673

Report cancellation instead of a network error for cancelled S3 requests#108673
jkartseva merged 12 commits into
masterfrom
jkartseva/backup-restore-cancel-error

jkartseva commented Jun 27, 2026 •

edited by robot-ch-test-poll3

Loading

Uh oh!

clickhouse-gh Bot commented Jun 27, 2026 •

edited by jkartseva

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Michicosun left a comment •

edited

Loading

Uh oh!

clickhouse-gh Bot Jul 6, 2026

Uh oh!

jkartseva Jul 7, 2026

Uh oh!

clickhouse-gh Bot Jul 6, 2026

Uh oh!

jkartseva Jul 7, 2026

Uh oh!

clickhouse-gh Bot commented Jul 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jkartseva commented Jun 27, 2026 • edited by robot-ch-test-poll3 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):

Version info

Uh oh!

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

Uh oh!

AI Review

Summary

Findings

Final Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Michicosun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Jul 7, 2026

LLVM Coverage Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkartseva commented Jun 27, 2026 •

edited by robot-ch-test-poll3

Loading

clickhouse-gh Bot commented Jun 27, 2026 •

edited by jkartseva

Loading

Michicosun left a comment •

edited

Loading