This will throw THERE_IS_NO_QUERY whenever the context has no query context (e.g. EXPLAIN PLAN, analysis-only paths, or contexts constructed outside a real query). Same applies to the three other call sites in ExecuteScalarSubqueriesVisitor.cpp and PreparedSets.cpp.

All four should be guarded:

auto cancel_callback = context->hasQueryContext()
    ? context->getQueryContext()->getSubqueryCancelCallback()
    : InteractiveCancelCallback{};

This is also the most likely cause of the flaky test — the unguarded call crashes the client immediately, before it registers in system.processes.

Minor: interactive_delay is in microseconds, so dividing by 1000 gives milliseconds. If the setting is less than 1000 (i.e. < 1ms), interactive_delay_ms becomes 0, which would busy-loop in executor->pull(chunk, 0). Consider clamping to a minimum:

const UInt64 interactive_delay_ms = std::max<UInt64>(100, context->getSettingsRef()[Setting::interactive_delay] / 1000);

The return value of cancel_callback() is discarded here (and in all other call sites). The callback returns true when the query should be cancelled — ignoring it means the cancellation only takes effect on the next pull() call, which might block for another interactive_delay_ms.

Consider:

while (chunk.getNumRows() == 0 && executor->pull(chunk, interactive_delay_ms))
    if (cancel_callback())
        break;

Worth considering whether a separate subquery_cancel_callback field is necessary. The only difference from interactive_cancel_callback is skipping sendProgress(). An alternative would be to pass a bool send_progress parameter to the existing callback, or have the subquery execution sites just call the existing interactive_cancel_callback and handle the progress reset issue differently.

Not a blocker, but adding per-concern callbacks to Context adds up over time.

Same thought here. I'd rather reduce the field and reuse interactive_cancel_callback. Also, what's the issue with sending progress generated by subqueries? AFAICT we only execute it every interactive_delay or 100ms, so it seems fine to send progress during the query analysis.

In fact I'd suggest this is desirable, the user sees the query is doing work instead of appearing stuck

Also, what's the issue with sending progress generated by subqueries?

When sendProgress() is called, it internally calls fetchValuesAndResetPiecewiseAtomically(), which resets the progress counters after reading them. This is fine for normal query execution where progress is sent periodically and the counters accumulate between sends.

However, for scalar and IN subqueries:

1. Subqueries execute during the analysis/preparation phase - they run before the main query execution starts
2. Progress counters are shared - the subquery uses the same progress tracking mechanism as the main query
3. Timing conflict - When we call sendProgress() during subquery execution (via the cancel callback), it resets the counters while the subquery is still running
4. Statistics loss - The JSON formatter reads from state.progress.read_rows, which gets reset to near-zero, showing only 1 row instead of the actual 100,001 rows read. This is the reason why 02136_scalar_read_rows_json failed in the init commit

I need to investigate this. I think having progress is desirable, so we should have both

Sounds good, I'll wait for your investigation and update the code accordingly.

The make_cancel_callback refactor is nice — clean extraction.

The comment on line 790 says "without sending progress" but the callback still calls sendSelectProfileEvents() and sendLogs(). Might be more accurate to say "without resetting progress counters" or "without sending progress packets".

A few issues with the test:

1. The poll loop doesn't detect early client death. If the client crashes (e.g. from the unguarded getQueryContext() above), the loop spins for 30 seconds then kill -INT fails with "No such process". Add kill -0 $client_pid 2>/dev/null || break in the loop.

2. The query_log query should filter on type = 'ExceptionWhileProcessing' — without it you might match the initial QueryStart event which has no exception, or get duplicate lines.

3. No cleanup trap — if the test fails midway, ${CLICKHOUSE_TEST_UNIQUE_NAME}_t is left behind. Add trap 'cleanup' EXIT at the top.

Cancel callback wrapping duplicated across four call sites

Medium Severity

The identical cancel-callback wrapping logic (check raw_cancel_callback(), throw QUERY_WAS_CANCELLED_BY_CLIENT if true, return false) is copy-pasted across four call sites in three files (evaluateScalarSubqueryIfNeeded.cpp, ExecuteScalarSubqueriesVisitor.cpp, PreparedSets.cpp ×2). If the cancellation behavior ever needs adjustment (e.g., different error message, additional cleanup, or logging), all four copies must be updated in lockstep. A single helper function—either a free function or a method on Context—would eliminate this duplication.

This is a valid concern. There are 5 copies of this.

Let's deduplicate the wrapper into a helper and use interactive_cancel_callback directly.

Test query_log query lacks type filter for reliability

Low Severity

The system.query_log queries at lines 20 and 40 don't filter on type (e.g., type = 'ExceptionWhileProcessing'). Multiple log entries can exist per query_id (QueryStart, ExceptionWhileProcessing, etc.). While QueryStart rows typically have empty exception fields so grep ignores them, the test is fragile—if duplicate exception rows appear or log behavior changes, the output could contain more or fewer QUERY_WAS_CANCELLED lines than the reference expects.

This is a valid concern. There are 5 copies of this.

Let's deduplicate the wrapper into a helper and use interactive_cancel_callback directly.

Same thought here. I'd rather reduce the field and reuse interactive_cancel_callback. Also, what's the issue with sending progress generated by subqueries? AFAICT we only execute it every interactive_delay or 100ms, so it seems fine to send progress during the query analysis.

In fact I'd suggest this is desirable, the user sees the query is doing work instead of appearing stuck

We don't need a wrapper in this file. CompletedPipelineExecutor::execute polls the callback every interactive_timeout_ms, and when it returns true, calls data->executor->cancel() which stops the pipeline. The cancelled pipeline will then cause the set not to be fully built, and the existing "timeout_overflow_mode=break" logic at line 359 already handles that case.

Cancel callback throws instead of returning true to executor

Medium Severity

The cancel callback passed to CompletedPipelineExecutor::setCancelCallback throws an exception instead of returning true. The CompletedPipelineExecutor API contract states "If returns true, query would be cancelled" — all other callers (executeQuery.cpp, LocalConnection.cpp) follow this by returning true. Throwing bypasses the executor's normal cancel path: data->executor->cancel() is never called through line 94 of CompletedPipelineExecutor::execute(), and the data->has_exception check at line 98 is skipped, silently dropping any exception from the pipeline execution thread.