Skip to content

Fix server abort on cancelled INSERT into TimeSeries table#108796

Merged
tiandiwonder merged 1 commit into
masterfrom
fix-timeseries-sink-cancel-pipeline-executor
Jun 30, 2026
Merged

Fix server abort on cancelled INSERT into TimeSeries table#108796
tiandiwonder merged 1 commit into
masterfrom
fix-timeseries-sink-cancel-pipeline-executor

Conversation

@tiandiwonder

@tiandiwonder tiandiwonder commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Related: #108783
Related: #57097

Problem

Inserting into a TimeSeries table runs nested INSERT pipelines inside TimeSeriesSink. When the outer query is cancelled without an exception — for example with timeout_overflow_mode = 'break', an early LIMIT, or sibling-processor teardown — the server aborts in debug and sanitizer builds:

Logical error: 'finished || std::uncaught_exceptions() || std::current_exception()'.
2. DB::PushingPipelineExecutor::~PushingPipelineExecutor()  PushingPipelineExecutor.cpp:68
3. DB::TimeSeriesSink::TargetPipeline::~TargetPipeline()    TimeSeriesSink.cpp:362
4. DB::TimeSeriesSink::~TimeSeriesSink()
... Received signal Aborted (6)

This is the same cancellation-without-exception class as AliasSink (fixed in #108783) and issue #57097, found while explaining that PR.

Root cause

TimeSeriesSink owns up to three TargetPipeline structs, each holding a PushingPipelineExecutor created and immediately started in createTargetPipeline. The sink finalized those executors only in onFinish (executor->finish()); it did not override onException or onCancel, and ~TargetPipeline was = default. On a no-exception cancel, the graph is cancelled via PipelineExecutor::cancel(CancelledByTimeout)graph->cancel, and IProcessor::cancel only invokes the default no-op onCancel, so neither onFinish nor onException runs. The defaulted destructor then destroyed a started-but-unfinished executor with no in-flight exception, tripping ~PushingPipelineExecutor's finished || std::uncaught_exceptions() || std::current_exception() invariant. The exception path was already safe because std::uncaught_exceptions() holds during unwinding; only the no-exception cancel path was broken.

Fix

Give TargetPipeline a destructor that cancels the executor, mirroring AliasSink::~AliasSink (#108783), CreatingSetsTransform, and MaterializingCTETransform. PushingPipelineExecutor::cancel sets finished = true, satisfying the destructor invariant; on the normal path onFinish already finished the executor, so the destructor's cancel is a no-op. Regression test 04416_timeseries_insert_cancel_no_crash exercises the timeout_overflow_mode = 'break' path and asserts the server survives; it reproduces the abort on the unfixed binary and passes after the fix (verified locally in a debug build).

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

Fixed a server abort (LOGICAL_ERROR / assertion in debug and sanitizer builds) when an INSERT ... SELECT into a TimeSeries table is cancelled without an exception, for example with timeout_overflow_mode = 'break'.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Version info

  • Merged into: 26.7.1.277 (included in 26.7 and later)

AliasSink and TimeSeriesSink share the same shape: a SinkToStorage that
owns a started PushingPipelineExecutor and finalizes it only in onFinish.
When the outer pipeline is cancelled without an exception (for example
with timeout_overflow_mode = 'break', an early LIMIT, or sibling-processor
teardown), the graph is cancelled via PipelineExecutor::cancel ->
graph->cancel, and IProcessor::cancel only invokes the default no-op
onCancel. So neither onFinish nor onException runs.

TimeSeriesSink had no onException/onCancel override and TargetPipeline's
destructor was defaulted, so it destroyed a started-but-unfinished executor
with no in-flight exception, tripping ~PushingPipelineExecutor's invariant
'finished || std::uncaught_exceptions() || std::current_exception()' and
aborting the server in debug and sanitizer builds.

Give TargetPipeline a destructor that cancels the executor, mirroring
AliasSink (PR #108783), CreatingSetsTransform, and MaterializingCTETransform.
PushingPipelineExecutor::cancel sets finished = true, so the invariant holds;
on the normal path onFinish already finished the executor, so cancel is a
no-op. This is the same cancellation-without-exception class as issue #57097.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [98a0f03]

Summary:


AI Review

Summary

This PR fixes cancellation teardown for TimeSeriesSink by cancelling owned inner PushingPipelineExecutors when TargetPipeline is destroyed, and adds a stateless regression that cancels INSERT ... SELECT via timeout_overflow_mode = 'break' before checking server liveness. I did not find correctness, safety, or coverage issues in the changed code.

Missing context / blind spots
  • ⚠️ Full PR CI was still in progress at review time; the remaining build and functional-test jobs will close that validation gap.
Final Verdict

Status: ✅ Approve

No code-review changes required.

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jun 29, 2026
@tiandiwonder tiandiwonder added the can be tested Allows running workflows for external contributors label Jun 29, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.40% 85.40% +0.00%
Functions 92.60% 92.60% +0.00%
Branches 77.60% 77.60% +0.00%

Changed lines: Changed C/C++ lines covered by tests: 5/6 (83.33%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

@scanhex12 scanhex12 self-assigned this Jun 29, 2026
@tiandiwonder
tiandiwonder added this pull request to the merge queue Jun 30, 2026
Merged via the queue into master with commit d70da4f Jun 30, 2026
185 of 190 checks passed
@tiandiwonder
tiandiwonder deleted the fix-timeseries-sink-cancel-pipeline-executor branch June 30, 2026 00:55
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants