Report cancellation instead of a network error for a cancelled `url` failover read by groeneai · Pull Request #113453 · ClickHouse/ClickHouse

groeneai · 2026-08-05T10:32:17Z

Report cancellation instead of a network error for a cancelled url failover read

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

Fixes a cancelled read from a url or ENGINE = URL table being reported as a transport failure instead of as a cancellation: with several |-separated options it reported NETWORK_ERROR with the message All uri (N) options are unreachable, and with a single option during schema inference the cancellation was wrapped as CANNOT_EXTRACT_TABLE_STRUCTURE. Also stops the failover loop from starting another URL option after the query was cancelled.

Description

StorageURLSource::getFirstAvailableURIAndReadBuffer walks the |-separated URL options and returns the first that opens. Its catch (...) treated every exception as an ordinary endpoint failure: try the next option, then throw a hardcoded NETWORK_ERROR. A cancellation was swallowed the same way, so KILL QUERY and max_execution_time reported 210 NETWORK_ERROR and still attempted every remaining option.

The fix asks the query's cancellation status via CurrentThread::checkIfNotCancelled, which throws only when the query was killed, so the cancellation keeps its own code and no further option is started. It runs on the failure paths and before the successful return, because the last option is returned without revisiting the loop top: with a single empty option under engine_url_skip_empty_files nothing throws at all. #108673 shipped the same primitive for cancelled S3 requests. The decision comes from the query status, not an error code, so legitimate failover is untouched and an empty file is still reported as empty. The in-flight attempt still runs to its receive timeout (#104929).

Overlap, measured against #112930 at 6395a10676e8. Its loop-top check is the same statement as the one here, byte for byte; only the comment differs, and that is the branches' sole merge conflict. Unique here are the checks in the catch and before the return: its catch guard is a bare throw;, rethrowing the in-flight transport error, and is gated on a CancellationPtr only the read path passes, so it is inert on both schema-inference call sites. Its head fails 3 of the 10 rows added here; adding those two lines to it makes all 10 pass, so folding them in is likely tidier than merging this. #111845 restructures the same catch and also conflicts.

The timeout_overflow_mode = 'break' test row asserts only that a soft break timeout is not reported as a cancellation, which holds under #112930's partial-result contract too.

…failover read StorageURLSource::getFirstAvailableURIAndReadBuffer walks the |-separated URL options and returns the first one that opens. Its catch(...) treated every exception as an ordinary endpoint failure: it kept only the message text, moved on to the next option, and after the loop synthesized a new exception with a hardcoded NETWORK_ERROR. A cancellation was swallowed the same way, so KILL QUERY or max_execution_time on a multi-option read reported 210 NETWORK_ERROR with the real code surviving only inside the message text, and every remaining option was still attempted. Single-option reads kept their code only because of the existing "if (options == 1) throw". Ask the query's cancellation status on that failure path via CurrentThread::checkIfNotCancelled, which throws the real cancellation exception when the query was killed and returns immediately otherwise. One statement covers both halves: the cancellation keeps its own code, and control leaves the loop so no further option is started. This mirrors the primitive and the control-flow position that ClickHouse#108673 shipped for cancelled S3 requests. The three callers, including both schema-inference sites, share this callee and are all covered. Legitimate failover is untouched because the decision comes from the query status rather than from an error code: a slow but uncancelled option still fails over, a genuinely unreachable set of options still reports the aggregate NETWORK_ERROR, and timeout_overflow_mode = 'break' does not mark the query killed so that mode is unchanged. The attempt already in flight when the cancellation arrives still runs to its receive timeout, which is the separate concern tracked by ClickHouse#104929. The new test pins six cases: three cancellation rows, including one where the cancellation lands while the LAST option is in flight, plus three must-not-regress rows. The last-option row is what distinguishes reporting the cancellation from the handler itself from a check placed at the top of the failover loop, which cannot help once no further option remains.

The third group of 04674 was bounded on both sides by wall clocks: option 1 had to fail before the query deadline, and the deadline had to fire before option 2 failed. That left about one second of margin on either side, which is not safe under sanitizers plus the CI thread fuzzer, and it was the one group that distinguishes reporting the cancellation from the failover handler itself from checking at the top of the failover loop. The dead listener now publishes one byte per accepted connection, so the group can wait until option 2 is actually in flight and then issue KILL QUERY, instead of hoping a deadline lands inside a window. With max_execution_time = 0 the query is never registered with the cancellation checker, so no timeout can race the kill. KILL QUERY also reaches QueryStatus::throwQueryWasCancelled rather than the timeout branch, so the group additionally pins QUERY_WAS_CANCELLED, which no group asserted before. The two groups driven by max_execution_time are kept as they are, so both cancellation reasons are now asserted. Also trim the test comments to the usual per-block length and drop the parentheses from function names.

The KILL-QUERY group waited on a shared accept-marker file written by the dead listener. The readiness probes are accepted by that listener too, and TCP completes an inbound connect from the kernel backlog before the listener thread reaches accept(), so probe accepts could be recorded after the group sampled its baseline and satisfy the +2 threshold with zero requests from the target query. The kill could then land before the first option was even issued, reddening the one group that distinguishes reporting the cancellation from the failover handler itself from a check at the top of the failover loop. Poll the query's own ReadWriteBufferFromHTTPRequestsSent counter from system.processes instead. The counter belongs to the query's thread group, so no readiness probe, earlier group or concurrent copy of the test can contribute to it, and the accept marker plumbing is gone. A non-numeric reply is treated as "not observed yet" so it never reaches the arithmetic comparison, which would otherwise write to stderr and fail the test outright. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…er loop The failover loop in StorageURLSource::getFirstAvailableURIAndReadBuffer has two exits that advance to the next URL option, and only the exception handler was guarded. When engine_url_skip_empty_files is enabled an option that answers with an empty body is stashed and skipped, and nothing between that continue and the next option's create() consults the query status. A query cancelled while such an empty response is being received therefore still issued an HTTP request for every remaining option, which is the second half of the behaviour this change claims: the error code was already correct on that path, because the next option is dead and its exception reaches the guarded handler. Applying the same check before the skip continue makes both paths that advance to the next option stop after a cancellation. The regression test gains a group covering the skip path. Its empty listener holds its response back until the shell has observed is_cancelled in system.processes, so the skip and the cancellation are ordered by observation rather than by a wall clock. wait_for_port now reports and exits on failure like the file's other poll helpers, instead of returning an ignored status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Give both poll helpers a liveness guard: each now reads the value it waits for and the existence of the query's row in one query, so the two cannot be sampled at different instants. A query that was observed and then left system.processes can never reach the value, so it is reported at once instead of polled for until the harness timeout. The previous shape could not distinguish "not cancelled yet" from "query already gone", which made the empty-option group fail 9 of 50 runs by 600 s timeout. Remove the ordering cycle in that group. Its cancellation was ordered after the response was released and the release after the cancellation was observed, with nothing outside the cycle keeping the query alive: if the kill did not land within the receive timeout the read completed on its own, the query finished with the aggregate error, and the release was never created. The held read is now pinned above the harness window so the cancellation is the only way out of it, and the group's second option is the responsive listener rather than the dead one, so an unwanted attempt on it is counted immediately instead of blocking for that same timeout. Not paying those two receive timeouts is what brings the runtime down: over 50 randomized runs the median goes from 98 s to 51 s and the maximum from 170 s to 52 s, against a 180 s per-test ceiling. No check-silencing tag was added. The reference is unchanged, and deleting the check on the empty-option path still reddens exactly that group's request count. ifNull is needed around the request-count sum because sum() over an empty set is NULL, which the non-numeric guard would otherwise read as "not observed yet", making the new liveness guard vacuous in exactly the state it exists to catch. The source comment is trimmed to the invariant it states; the rest was motivation, which belongs here.

The check on the empty-option skip path sat inside a branch guarded by `option != std::prev(end)`, so a final empty option never reached it: the helper returned normally and both callers re-entered their outer `do ... while` on `eof()` and issued another HTTP request for the next URI after the query had been cancelled. `StorageURLSource::generate` cannot intercept that, because its `isCancelled` check runs before `initialize` rather than inside its loop. Moving the call to the first statement of the failover loop covers the same skip path (its `continue` returns there) plus that re-entry, in one line instead of two, through the single callee both callers share. The check in the `catch (...)` handler stays: the loop is not re-entered after the last option, so only a check there can report the cancellation for it. The `timeout_overflow_mode = 'break'` regression row now asserts the property it exists to pin, that a soft break timeout is not reported as a cancellation, instead of the aggregate `NETWORK_ERROR` message text. A soft break does not set `is_killed`, so `CurrentThread::checkIfNotCancelled` is a no-op for it either way; pinning the text additionally froze a contract that ClickHouse#112930 deliberately changes, which would have broken CI for whichever of the two merged second. Verified by mutation: making the guard treat a query past its soft deadline as cancelled reddens exactly that row.

The check in the `catch (...)` handler is deliberately placed before the `if (options == 1) throw;` rethrow, so a cancelled single-option read is reported as a cancellation too. Until now no test row exercised that, and the mutation arm that moves the check after the rethrow therefore passed. The covered single-option path is schema inference, not the read path. `delay_initialization` is the last parameter of `StorageURLSource::getFirstAvailableURIAndReadBuffer`, and the read-path caller passes `current_uri_options.size() == 1` for it while both schema-inference callers pass `false`. `ReadWriteBufferFromHTTP` calls `next` from its constructor only when initialization is not delayed, so with one option on the read path no request is issued inside the failover `try` and the handler is never entered. Omitting the structure argument routes the query through schema inference instead, where the request is issued inside the `try`. Without the fix that path reports `CANNOT_EXTRACT_TABLE_STRUCTURE`, not the `NETWORK_ERROR` of the multi-option shape: the swallowed cancellation makes the failover loop rethrow the raw `Poco` exception, which is not a `DB::Exception`, so `readSchemaFromFormatImpl` wraps it in its `catch (...)` instead of rethrowing it with its code intact. The `TSV` argument selects that code rather than `CANNOT_DETECT_FORMAT`, so it is load-bearing for the row. Measured on the fix, on pre-fix and with the handler check moved after the rethrow: the row reads `TIMEOUT_EXCEEDED` only on the fix, and the moved check reddens exactly it and no other row. ClickHouse#112930 places its equivalent guard after that rethrow and gates it on a `CancellationPtr` that only the read path passes, so this row also fails there. Adding the handler line to it makes all eight rows pass.

groeneai · 2026-08-05T10:32:19Z

Internal second-model review

Before opening this PR I ran an independent review of the diff with a second model, plus my own
cold read of the resulting code. Findings and how each was resolved:

❌ Blockers: none.

⚠️ Major: the regression test's readiness observation was not scoped to the query under test
(raised twice, in two separate rounds; fixed both times).

The test group that cancels the read while the last URL option is in flight has to know that the
second option really is in flight before it cancels, because that is the group that shows why the
check belongs in the failure handler rather than at the top of the failover loop.

Round 1 bounded that moment with wall clocks. Measured margins were about 1.0 s on one side and
0.9 s on the other, which is not safe under sanitizers plus the CI thread fuzzer, and losing
either side changes the reported outcome. Replaced with an event.
Round 2 found the replacement event was still not query-scoped: it counted accept() calls on a
shared listener via a file. TCP completes an inbound connection from the kernel backlog before the
listener thread ever calls accept(), so the earlier readiness probes could still be queued at the
moment the baseline was taken and could satisfy the threshold with zero requests from the target
query. Verified on a plain socket: connect() completes in 0.0001 s against a listener that never
calls accept(), and that connection is retrievable 0.3 s later.

Final form observes the query's own counter,
ProfileEvents['ReadWriteBufferFromHTTPRequestsSent'] for its query_id in system.processes, so
nothing outside the query can contribute to it. Both polls are bounded and fail loudly rather than
falling through.

⚠️ Major: one of the two cancellation reasons was reachable but never asserted. All cancellation
groups drove max_execution_time, which takes the TIMEOUT branch and reports TIMEOUT_EXCEEDED.
KILL QUERY takes the other branch and reports QUERY_WAS_CANCELLED, and nothing asserted it. The
last-option group now uses KILL QUERY with max_execution_time = 0, so both reasons are asserted
and the timeout racer is structurally absent rather than merely unlikely.

⚠️ Major: the fix widened the contract to single-option reads, and that was neither pinned nor
stated. The cancellation check sits before if (options == 1) throw, so it also fires when there is
only one option. My first attempt to pin that was wrong in an instructive way, and measuring it is
what found the real shape:

On the read path a single option is genuinely unaffected. That caller passes
delay_initialization = options == 1, and the buffer only issues its request in the constructor when
initialization is not delayed, so for one option nothing is attempted inside the failover try
and the failure handler is never entered. Measured identical on three binaries (with the fix, with
the two calls deleted, and with the check moved after the single-option branch), against a
two-option control that moves on all three. Instrumenting both check sites confirmed it directly:
zero handler hits for that shape.
Schema inference is where the widening is real: those two callers pass
delay_initialization = false, so one option does issue its request inside the try. Without the
fix the cancellation is swallowed there and schema inference wraps the transport error as
CANNOT_EXTRACT_TABLE_STRUCTURE; with it the query reports the cancellation.

A test row now pins that case, and it is what makes the placement mutation meaningful: moving the
check after the single-option rethrow reverts the reported code, where previously that arm passed only
because no test covered a shape in which the moved line executes. The changelog and description were
broadened to match.

💡 Noted, not blocking: over 50 randomized runs of the final test the per-run time was 55.50 s
minimum, 56.16 s median, 76.21 s maximum against the 180 s ceiling, so it needs no long tag.

⚠️ Major: a second open PR restructuring the same catch was not on the record. Re-running the
competing-work check immediately before opening this, over every open pull request rather than by
keyword, surfaced two things that the description now states. #112930's check at the top of the
failover loop is the same statement as the one added here, byte for byte, and the differing comment
above it is the only merge conflict between the branches, which leaves the check inside the failure
handler as the unique content here. And #111845 hoists the single-option rethrow out of that same
handler, so it conflicts too, although it adds no cancellation check of its own and so fixes neither
half of this. Keyword search finds neither overlap; enumerating the open pull requests and reading
each one's file list does.

Every round re-verified both directions on rebuilt binaries, asserting the served buildId() against
each build's own readelf -n: the test passes with the fix, fails without it reddening only the
cancellation rows, and fails on exactly the last-option group when the check is moved to the top of
the failover loop instead. Plus a 50-run repeat with randomized settings, and three concurrent copies
for parallel safety.

Session id: cron:clickhouse-review-slot-8:20260805-093500

groeneai · 2026-08-05T10:33:11Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, 100% on demand. Dead HTTP listener (accepts every connection, never answers) plus a two-option `url()` with `max_execution_time = 2, http_receive_timeout = 5, http_max_tries = 1`. Pre-fix: `210 NETWORK_ERROR`, `All uri (2) options are unreachable`, `ReadWriteBufferFromHTTPRequestsSent = 2`. Not a percentage flake. Every readiness observation goes through the query's OWN counters in `system.processes`, so no wall clock and no shared file is involved, and no readiness probe, earlier group or concurrent copy can satisfy one. Both poll helpers read the awaited value and the row's existence in a single query, so a query that vanishes is reported at once rather than polled for. Fresh 50/50 with randomization on: 50 OK / 0 FAIL, min 55.50 / median 56.16 / p90 61.15 / max 76.21 s against the 180 s per-run cap, measured while the box was carrying 31 concurrent `clickhouse-test` runs. Earlier rounds recorded a lower median (51.0 s) on a quieter box with a 50.10 s floor, and a 15-run control on the same binary at lower load reproduced min 50.05 / median 50.41 / max 51.02 s, so the spread is contention from neighbouring slots rather than a per-run regression.
b	Root cause explained?	Yes. `StorageURLSource::getFirstAvailableURIAndReadBuffer`'s `catch (...)` treats a cancellation as an ordinary endpoint failure: it keeps only `getCurrentExceptionMessage` text, `continue`s to the next option, and after the loop synthesizes a new exception with a hardcoded `NETWORK_ERROR`. The original `std::exception_ptr` is never retained, so the code cannot be recovered downstream. `if (options == 1) throw` rethrows the raw in-flight transport error for a single option, which keeps that error only on the read path: through schema inference it is not a `DB::Exception`, so `readSchemaFromFormatImpl` wraps it as `CANNOT_EXTRACT_TABLE_STRUCTURE` instead of preserving its code.
c	Fix matches root cause?	Yes. The defect is a misclassification inside that handler, so the fix asks the query's cancellation status there via `CurrentThread::checkIfNotCancelled`. Nothing widened, no error code pattern-matched, no timeout bumped, no randomization disabled, no upstream bug masked: the HTTP layer already raises the correct exception, only this handler misfiled it. Same primitive and same control-flow position that #108673 shipped for cancelled S3 requests.
d	Test intent preserved / new tests added?	Yes. New stateless test with 10 assertion groups: 6 cancellation rows plus 4 must-not-regress rows (an honestly empty single option is still reported as an empty file and still reads as zero rows, all options down still reports the aggregate `NETWORK_ERROR`, failover to a working option still returns data, a soft `timeout_overflow_mode = 'break'` timeout is still not reported as a cancellation). No existing test weakened, removed or retagged, and no check-silencing tag added. Seven mutations pin that the groups are not vacuous: deleting all three checks reddens all 6 cancellation groups; deleting only the loop-top check reddens exactly the empty-option group's request count (`1` to `2`); the loop-top-only design reddens exactly the last-option group and nothing else; moving the handler check after the `options == 1` rethrow reddens exactly the schema-inference group and nothing else; treating a query past its soft deadline as cancelled reddens exactly the `break` group; and reverting only the check before the successful return reddens exactly the single-empty-option schema-inference group and nothing else. The `break` group asserts the property (was this reported as a cancellation) rather than the aggregate message text, so it holds under both the current contract and the partial-result contract #112930 introduces; when a group is added the other reference rows are verified byte-identical positionally, not by eye. A first candidate mutation for the `break` group passed and was discarded as a vacuous probe once a direct measurement showed its branch never executed.
e	Both directions demonstrated?	Yes. `[ FAIL ]` on the pre-fix build and `[ OK ]` on the fixed build, both asserting the served `buildId()` equals that arm's own `readelf -n`. The pre-fix arm is this worktree with only `src/Storages/StorageURL.cpp` reverted, and it relinked bit-identically to the base build. The diff is exactly the cancellation lines; all must-not-regress lines are byte-identical in both arms.
f	Fix is general across code paths?	Yes. All 3 call sites of the fixed function are covered by the single shared-callee hunk: the read path plus two schema-inference sites, and schema inference was measured rather than assumed, in both arities: with several options `210` pre-fix to `159` post-fix, and with a single option `636 CANNOT_EXTRACT_TABLE_STRUCTURE` pre-fix to `159` post-fix. That single-option case is reached only through schema inference, because the read-path caller passes `delay_initialization = options == 1` while both schema-inference callers pass `false`, so on the read path a single option issues no request inside the guarded block at all; a read-path control measured identically (`1000`) on both arms, which is what proves the two call sites differ rather than the arms being noisy. All three exits from the loop body are covered: the `catch (...)` `continue` by the handler check, the `engine_url_skip_empty_files` skip by the loop-top check its `continue` returns to, and the successful `return`, which revisits neither, by the check before it. That last exit is reachable with a laundered outcome only through schema inference: measured, a cancelled read of a single empty option reports `394` on the pre-fix binary too, because `PipelineExecutor::finalizeExecution` calls `checkTimeLimit`. The 4 sibling failover loops were enumerated and none is a carrier: the `web` disk ends with `std::rethrow_exception` so the code is preserved there, `getStructureOfRemoteTable` catches only `NetException`, and the PostgreSQL pool's `catch (...)` rethrows.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes for the applicable axes: option counts 1, 2 and 3; cancellation landing during the first, a middle, and the last option, and during an empty-option skip; both cancellation reasons asserted, not merely reachable (`max_execution_time` gives `TIMEOUT_EXCEEDED`, `KILL QUERY` gives `QUERY_WAS_CANCELLED`); `timeout_overflow_mode` both `throw` and `break`, the latter pinned as a non-cancellation; explicit structure and schema inference. Type wrappers are not applicable: the change touches no data-type surface.
h	Backward compatible? (maintainer-approved exception only)	Yes. No setting added or re-defaulted, no serialization or on-disk format, no protocol, so no `SettingsChangesHistory.cpp` entry is needed. The only behaviour change is the error code reported for an already-failing cancelled query, which is the bug being fixed; the two non-cancelled rows pin that ordinary failover reporting is unchanged.
i	Invariants and contracts preserved?	Yes. The handler's call is the first statement of the catch, so it runs before the `options == 1` throw and before the message, log and `continue` path: no ordering hazard on any exit. The other call is the first statement of the loop body, so it observes a cancellation before any request is built, and it covers the empty-option skip (whose `continue` returns to it) and re-entry from either caller's outer `do ... while`; `last_skipped_empty_res` is still assigned unconditionally on that path, so the fallback after the loop is unaffected. Each call either throws or does nothing, so no caller contract or return value changes, and nothing is allocated or held across it. Both are no-ops unless the query is killed (`QueryStatus::throwIfKilled` returns immediately otherwise) and also no-ops with no current thread or no thread group, so `skip_not_found_url_for_globs` and `engine_url_skip_empty_files` reaching these paths are unaffected.

Session id: cron:clickhouse-review-slot-8:20260805-093500

groeneai · 2026-08-05T10:34:31Z

cc @alexey-milovidov @Algunenano, could you review this? The failover loop in StorageURLSource::getFirstAvailableURIAndReadBuffer treated a cancellation as an ordinary endpoint failure, so a killed url() read reported NETWORK_ERROR and kept probing the remaining options; the check inside that handler is the one thing here that #112930 does not already cover, and folding just that line into #112930 may be tidier than merging this.

clickhouse-gh · 2026-08-05T11:47:03Z

Workflow [PR], commit [ed4b110]

Summary: ❌

job_name	test_name	status	info
Style check		FAIL
	functional_tests_check	FAIL	cidb
Finish Workflow		FAIL
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	FAIL
Code Review		DROPPED
Fast test (arm_darwin)		DROPPED
Build (amd_debug)		DROPPED
Build (amd_asan_ubsan)		DROPPED
Build (amd_tsan)		DROPPED
Build (amd_msan)		DROPPED
Build (amd_binary)		DROPPED
Build (arm_debug)		DROPPED

`check_functional_test_cases` in `ci/jobs/check_style.py` rejects any file under `tests/queries` whose path contains the substring `fail`, which `failover` did. The stem now says `url_option_fallback`, matching the `uri_options` vocabulary of the code under test. Contents are unchanged: both files are byte-identical to their previous revisions and the executable bit is preserved.

getFirstAvailableURIAndReadBuffer returns the last option without revisiting the top of the failover loop and without throwing, so neither the loop-top nor the handler cancellation check is reached for it. With engine_url_skip_empty_files and a single empty option, option == std::prev(end) bypasses the skip branch and the empty buffer is returned; schema inference then runs out of options and reports CANNOT_EXTRACT_TABLE_STRUCTURE, which is one of the two shapes this change's changelog entry already promises to fix. Ask the cancellation status before the successful return as well. checkIfNotCancelled returns immediately unless the query is killed, so guarding the whole successful exit needs no new predicate and leaves an honestly empty file reported as empty. The read path was measured NOT to be affected: a cancelled read of a single empty option already reports QUERY_WAS_CANCELLED, because PipelineExecutor::finalizeExecution calls checkTimeLimit, which throws for a killed query. Only the schema-inference callers, which pass delay_initialization = false, reached the laundered outcome. Reverting only the added statement reddens exactly the new test row and nothing else; the pre-fix binary additionally reddens the five existing cancellation rows. Two non-cancellation rows pin that an empty single option is still reported as an empty file and still reads as zero rows, and read identically on every arm. Reported by clickhouse-gh[bot] on the previous head.

…etry retry) The Post Hooks failure on Build (arm_release) is the LogCluster telemetry INSERT dying with HTTP 500. ClickHouse#113409 merged that retry into ci/jobs/scripts/log_cluster.py at 2026-08-06T00:05:00Z; this branch predated it, so CI re-runs with the fix present.

groeneai · 2026-08-06T01:16:33Z

CI finish ledger -- `1b5e603`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Build (arm_release) / Post Hooks	infra -- build-profile telemetry `INSERT` into the CI logs cluster returns HTTP 500	#113409 (external, merged 2026-08-06T00:05:00Z) -- merged master @ `08eef55622353b4` to pick it up
Integration tests (amd_msan, 4/8) / test_tcp_handler_connection_limits::test_query_count_limit	flaky -- oracle asserts exactly 1 closure on a shared global counter, observed 2	a fix task is moved to pending (investigating at full effort -- fixing-PR link to follow here)
Stress test (arm_msan) / MemorySanitizer: use-of-uninitialized-value (STID None)	sanitizer, stackless -- MSan aborts with "nested bug in the same thread", no frames captured	a fix task is moved to pending (investigating at full effort -- fixing-PR link to follow here)

None of the three is PR-caused. This PR's diff is three files (src/Storages/StorageURL.cpp
plus the new 04674 test and its reference); it touches no build hook, no TCP handler and
nothing the arm_msan stress run exercises.

Bugfix validation is green, and I merged master rather than only linking #113409 because
this branch predated that fix, so its ci/jobs/scripts/log_cluster.py retry was absent.
Fresh CI is now running on 08eef55622353b4, which carries the fix.

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

PedroTadim · 2026-08-06T09:16:24Z

@@ -0,0 +1,26 @@
+--- cancelled, two options ---


@groeneai just remove the test file, it's long and may become flaky

or maybe not

You were right to flag it, and the retraction was too kind: this test now genuinely breaches the cap, so I am cutting its runtime rather than leaving it as is.

Measured on Stateless tests (amd_asan_ubsan, flaky check), per head:

head OK FAIL p50 max

c157ee94 50 0 135.3s 142.1s

1b5e603e 50 0 155.9s 167.0s

08eef556 0 6 185.7s 190.0s

At the current head all six attempted runs report Test runs too long (> 180s) and the check then stops early on --max-failures. It is not one slow outlier against a healthy distribution: the minimum over those runs is 182.7s. The amd_debug, amd_msan and amd_tsan flaky checks are still 50/50 OK at 110-120s, which is why only the asan lane shows it. The ordinary amd_asan_ubsan, distributed plan, parallel run is 119.8s, so the cost is real and not an artifact of the 50x repetition.

Two things I am not going to do. Delete the test, because it is the only witness for the cancellation-instead-of-NETWORK_ERROR behaviour this PR fixes. And add a long tag, which exempts a test from that cap without making it any cheaper.

What the numbers say about where to cut: the growth tracks assertion groups, 8 groups at 135.3s against 10 at 155.9s, and the file is byte-identical between the last two heads, so the final 30s is not test content. I have not yet attributed the per-group wall clock, so I will measure that before choosing the cut, and size it for real headroom instead of to just clear 180s. I will post the numbers here.

Numbers, as promised, and the cut is pushed in 9798fbd5815c0a3df1a5a3cf8c02836a1c035814.

Per-group wall clock at the previous head, one debug binary, one server, markers around each group:

group sec %

KILL QUERY while the last option is in flight 20.44 36.9

timeout_overflow_mode = break 10.30 18.6

cancelled, two options 5.37 9.7

cancelled, one option, schema inference 5.31 9.6

cancelled, three options 5.28 9.5

not cancelled, all options down 4.29 7.7

not cancelled, failover to a working option 2.09 3.8

the three remaining groups 1.64 3.0

setup: port allocation, listeners, readiness 0.69 1.2

So 53 of the 55 seconds is http_receive_timeout, and my earlier guess that the poll loops were the
expensive part was wrong. A micro-probe settled it: an option pointing at a listener that accepts and
never answers costs one full http_receive_timeout, one that accepts and closes costs 0.12s, and both
are counted by ReadWriteBufferFromHTTPRequestsSent. Wall clock tracked http_receive_timeout to
within 0.1s and did not move with max_execution_time.

That is where the waste was. Four groups only need their first option to have been attempted and
failed, and nothing asserts how long it took, so only the option each group is about has to stay open;
those four now point their first option at a listener that fails at once.
All uri (N) options are unreachable reports the option count rather than durations, so that message
is unchanged. The three cancellation groups keep max_execution_time = 2 and drop
http_receive_timeout 5 to 3, which a sweep showed is the last safe value: at 2 the asserted request
count becomes 2 instead of 1, and at 1 the group reports NETWORK_ERROR, because the transport
timeout rather than the soft one ends the read. And kill_and_wait polled every 0.1s while waiting for
a query that cannot leave system.processes until its own receive timeout elapses, so that is now
0.5s with the same overall cap; client invocations per run went 137 to 43.

Result on one binary and one server with randomization on: 50 of 50 pass, min 26.26 / p50 27.14 /
max 29.57s, against min 57.55 / p50 57.83 / max 58.62s over 12 runs of the previous file. 2.13x at the
p50, and 16.4% of the cap at the maximum rather than 106%. Largest group is now 4.45s, so nothing
dominates any more.

Coverage is unchanged and I checked that rather than assuming it. The .reference file is untouched,
so the same 26 lines are asserted. Reverting the fix entirely still reddens all six cancellation rows
and leaves all four must-not-regress rows green, and each single-line revert still reddens only its own
row, each on its own rebuilt binary with the served buildId() checked; the build ID returned
byte-exactly to the shipping one afterwards.

groeneai · 2026-08-06T09:25:55Z

Fixing PR for the test_tcp_handler_connection_limits/test.py::test_query_count_limit row on my CI finish ledger: #113530 (merged 2026-08-06T00:24:35Z).

It replaces the oracle that counted every Done processing connection. line with one that counts only connections closed because a limit was reached (Closing connection due to limits: queries=), so the startup readiness probe and a reconnecting client no longer pollute the count.

The failure on this PR was on a build that predates that merge. Probing the test file per build (marker get_limit_closed_count, with a control string present in both the pre- and post-fix versions scored on every probe): of 28 recent msan runs, the 14 whose build contains the fix are 14 OK / 0 FAIL, and every failure sits on a build without it. On msan flavours the rate went from 63 FAIL / 197 OK before the merge to 0 FAIL / 219 OK after it.

No action needed here beyond a rerun or a rebase onto current master.

The flaky check runs a changed test 50 times and fails any run over 180s. On `amd_asan_ubsan` this test reached a median of 185.7s and failed 6 of 6 attempted runs. Locally its cost was 55.0s per run, and per-arm instrumentation attributed 53.0s of that to `http_receive_timeout` waits and 0.7s to setup. The waits were paid for options nothing asserts about. A micro-probe measured that an option pointing at a listener which accepts but never answers costs one full `http_receive_timeout`, while an option pointing at a listener which accepts and closes costs 0.12s, and that in both cases the attempt is counted by `ReadWriteBufferFromHTTPRequestsSent`. Four arms only need their first option to have been attempted and failed; only the option each one asserts about has to stay open. Those four now use a new fail-at-once listener for the first option. `All uri (N) options are unreachable` reports the option count rather than how long each option took, so that message is unchanged. The three cancellation arms keep `max_execution_time = 2` and drop `http_receive_timeout` from 5 to 3. A sweep pinned 3 as the last safe value: at 2 the asserted request count becomes 2 instead of 1, and at 1 the arm reports `NETWORK_ERROR` instead of the cancellation, because the transport timeout rather than the soft one ends the read. `kill_and_wait` polled every 0.1s. Instrumenting it showed the wait is the awaited read finishing its own receive timeout, so the query cannot leave `system.processes` sooner and a finer cadence only spends client startups. It now polls every 0.5s with the same 60s cap, taking client invocations per run from 138 to 43. Measured on one binary and one server with randomization on: 50 of 50 runs pass at min 26.26 / median 27.14 / max 29.57s, against min 57.55 / median 57.83 / max 58.62s over 12 runs of the previous version. The reference file is untouched, and the mutants that pin the assertions still redden exactly as before: reverting the fix entirely reddens all six cancellation rows and no must-not-regress row, while each single-line revert reddens only its own row.

clickhouse-gh · 2026-08-06T14:06:26Z

            }

+            /// The last option is returned without revisiting the loop top, empty or not.
+            CurrentThread::checkIfNotCancelled();


This still leaves one unguarded return path: if engine_url_skip_empty_files = 1, an earlier option was saved into last_skipped_empty_res, and the final option throws normally, a KILL QUERY that lands after the catch-side CurrentThread::checkIfNotCancelled() but before the post-loop return last_skipped_empty_res; will still hand EOF back to the caller instead of the cancellation.

That matters for schema inference, because StorageURLSchemaReader::next consumes that EOF in its outer do ... while and on a single URL entry then falls out as CANNOT_EXTRACT_TABLE_STRUCTURE / “all files are empty”. I think this needs the same cancellation probe before returning last_skipped_empty_res, plus a regression row for an empty option followed by a failing option under engine_url_skip_empty_files = 1.

Agreed, and the window is wider in scope than reported: it also reaches the NETWORK_ERROR throw two lines below the return, so it hits the read path too, not only schema inference. Fixed in ed4b110.

I measured it by widening the span between the handler's check and the post-loop exits to 12 seconds and logging whether the cancellation was observed inside it, so the kill lands there by construction:

cell (kill lands inside the window) before after

EOF return, schema inference 636 CANNOT_EXTRACT_TABLE_STRUCTURE 394 QUERY_WAS_CANCELLED

NETWORK_ERROR throw, schema inference 210 NETWORK_ERROR 394

NETWORK_ERROR throw, read path 210 NETWORK_ERROR 394

each of the three, not cancelled 636 / 210 / 210 unchanged

One statement covers both exits, because the handler's continue is the only edge that can leave the loop: the empty-option skip is guarded by option != std::prev(end) so it cannot run on the final iteration, and the successful return leaves the function. So nothing else reaches the post-loop code.

On the regression row: a row that races a KILL QUERY against this window would be a control rather than a witness. The span is straight-line code only, and instrumenting it measured 35 to 64 microseconds (median 37, n=25). Firing the kill the instant the query's own request counter shows the final option in flight, 70 attempts landed in it zero times, all reporting 394 on a binary without the fix. I pinned it with a PAUSEABLE_ONCE failpoint parked in the window instead, in 04812_url_failover_post_loop_honors_cancellation: three cancellation rows and two not-cancelled controls. Reverting only the added check reddens exactly the three and leaves both controls green.

It is a separate file because a pauseable failpoint is a global channel and needs no-parallel, which the existing ten rows do not. That also keeps it cheap: 2.9 seconds, and 50 randomized runs give 50 passes at a median of 5.5 seconds against the 180 second cap, so it does not spend the runtime headroom recovered in 9798fbd.

clickhouse-gh · 2026-08-06T15:38:51Z

Build profile diff (arm_release)

No arm_release build profile data for commit ed4b110 - the build was skipped, reused from cache, or predates profile upload.

… option Once the loop has tried every URL option, control reaches the post-loop code only through the exception handler's `continue`. That handler checks the query status, but the post-loop code then has two exits of its own, and a cancellation observed between the handler's check and those exits was reported as an ordinary failure: the kept empty option was handed back as end-of-input, which schema inference turns into CANNOT_EXTRACT_TABLE_STRUCTURE, and with no empty option the aggregate NETWORK_ERROR was thrown instead. Both replace the cancellation this change set exists to preserve. The handler's own check is what makes one statement enough here: the loop's other two exits (the empty-option skip and the successful return) cannot leave the loop, so nothing else reaches the post-loop code and no second check is needed. The window between the handler's check and the post-loop exits is only straight-line code -- an exception message, a log call, the loop increment and one branch. Measured on a debug build it is 35 to 64 microseconds wide (median 37), and 70 attempts that fire KILL QUERY while the final option is in flight never landed inside it, so a test that races a kill against it would be a control rather than a witness. It is instead pinned with a PAUSEABLE_ONCE failpoint parked in exactly that window, which needs no receive timeout and adds under three seconds. Reverting only the added check reports CANNOT_EXTRACT_TABLE_STRUCTURE, NETWORK_ERROR and NETWORK_ERROR for the three cancelled rows while both not-cancelled rows stay unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

`ci/jobs/check_style.py` rejects any functional test file whose name contains the substring `fail`, which `failover` matches. The rule walks the whole `tests/queries` tree rather than the change, so the check reports `test case ./tests/queries/0_stateless/04812_url_failover_post_loop_honors_cancellation.sh includes 'fail' in its name`. The sibling test in this change set was renamed for the same reason, so this adopts the same stem. The file contents and mode are unchanged. Removing only the post-loop check still reports `CANNOT_EXTRACT_TABLE_STRUCTURE`, `NETWORK_ERROR` and `NETWORK_ERROR` for the three cancelled rows under the new name while both not-cancelled rows stay unchanged, so the rename does not weaken what the test observes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

PedroTadim · 2026-08-07T07:44:21Z

#112930 supersedes this PR

groeneai · 2026-08-07T08:31:07Z

Agreed, and thanks for triaging it. #112930 covers the failover read path, its loop-top check is byte-identical to the one here (md5 9eeea0f5910740417e5b35217e941b0a), and it threads a CancellationPtr through ReadWriteBufferFromHTTP besides. Nothing left in this PR is worth merging on its own, so closing it is right and I am not reopening it.

One shape in getFirstAvailableURIAndReadBuffer that #112930 does not reach yet, in case it is useful there. At head a623a25f both of its new guards read if (cancellation && cancellation->isCancelled()) (StorageURL.cpp:685, :734), and cancellation is a defaulted parameter (StorageURL.h:266). Argument counts at the three call sites: :408 passes 12 with a live cancellation, :1045 and :1100 pass 11, so both schema-inference exits receive nullptr and both guards are inert on them. The one guard not gated on it, CurrentThread::checkIfNotCancelled() at :676, is at the loop top, so once the last option has failed the loop exits at :744 and reaches throw Exception(ErrorCodes::NETWORK_ERROR, "All uri ({}) options are unreachable: {}") at :751 with no check in between. A cancellation landing in that window is reported as the aggregate network error rather than as a cancellation. That is what the second test here pins, and its two uncancelled rows are the controls for it.

Closing it is one statement before :748; making it cover the read path too would additionally mean passing cancellation at the two schema-inference call sites. Happy to send that as a small follow-up against master once #112930 lands, or it can fold into #112930 while that is open, whichever you and @alexey-milovidov prefer.

PedroTadim · 2026-08-07T08:33:12Z

@groeneai can you add that comment on #112930 ?

groeneai · 2026-08-07T10:32:12Z

Done: #112930 (comment)

I re-measured everything at a623a25f before posting rather than copying the text from here, and that turned up two refinements worth having there. The schema-inference iterator holds no Cancellation object at all, so covering that path is not just a matter of forwarding the parameter at the two call sites as I put it above. And CurrentThread::checkIfNotCancelled() is a better fit for the missing check than a synthesized QUERY_WAS_CANCELLED, because it throws the real cancellation cause and so stays clear of the open review comment on :685.

groeneai and others added 7 commits August 1, 2026 08:04

PedroTadim added the can be tested Allows running workflows for external contributors label Aug 5, 2026

PedroTadim self-assigned this Aug 5, 2026

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

PedroTadim assigned PedroTadim and unassigned PedroTadim Aug 5, 2026

clickhouse-gh Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/Storages/StorageURL.cpp

groeneai added 2 commits August 5, 2026 16:55

PedroTadim reviewed Aug 6, 2026

View reviewed changes

clickhouse-gh Bot reviewed Aug 6, 2026

View reviewed changes

groeneai and others added 2 commits August 6, 2026 16:22

PedroTadim closed this Aug 7, 2026

groeneai mentioned this pull request Aug 7, 2026

Improve canceling queries with the url function #112930

Open

head	OK	FAIL	p50	max
`c157ee94`	50	0	135.3s	142.1s
`1b5e603e`	50	0	155.9s	167.0s
`08eef556`	0	6	185.7s	190.0s

group	sec	%
KILL QUERY while the last option is in flight	20.44	36.9
timeout_overflow_mode = break	10.30	18.6
cancelled, two options	5.37	9.7
cancelled, one option, schema inference	5.31	9.6
cancelled, three options	5.28	9.5
not cancelled, all options down	4.29	7.7
not cancelled, failover to a working option	2.09	3.8
the three remaining groups	1.64	3.0
setup: port allocation, listeners, readiness	0.69	1.2

cell (kill lands inside the window)	before	after
EOF return, schema inference	`636 CANNOT_EXTRACT_TABLE_STRUCTURE`	`394 QUERY_WAS_CANCELLED`
`NETWORK_ERROR` throw, schema inference	`210 NETWORK_ERROR`	`394`
`NETWORK_ERROR` throw, read path	`210 NETWORK_ERROR`	`394`
each of the three, not cancelled	`636` / `210` / `210`	unchanged

Uh oh!

Conversation

groeneai commented Aug 5, 2026 • edited 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 Aug 5, 2026

Uh oh!

groeneai commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groeneai commented Aug 5, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

groeneai commented Aug 6, 2026

CI finish ledger -- 1b5e603

Uh oh!

PedroTadim Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

PedroTadim Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai commented Aug 6, 2026

Uh oh!

clickhouse-gh Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Build profile diff (arm_release)

Uh oh!

PedroTadim commented Aug 7, 2026

Uh oh!

groeneai commented Aug 7, 2026

Uh oh!

PedroTadim commented Aug 7, 2026

Uh oh!

groeneai commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

groeneai commented Aug 5, 2026 •

edited

Loading

groeneai commented Aug 5, 2026 •

edited

Loading

clickhouse-gh Bot commented Aug 5, 2026 •

edited

Loading

CI finish ledger -- `1b5e603`

clickhouse-gh Bot commented Aug 6, 2026 •

edited

Loading