[fix](pipeline) Unify fragment cancellation and close lifecycle - #67261
Open
HappenLee wants to merge 2 commits into
Open
[fix](pipeline) Unify fragment cancellation and close lifecycle#67261HappenLee wants to merge 2 commits into
HappenLee wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#67236 Problem Summary: A query timeout can make every pending pipeline task call PipelineFragmentContext::cancel() with the same error before the fragment task count is drained. The repeated calls duplicate fragment logs, task dumps, stream cancellation, and dependency wakeups. Use one atomic terminal lifecycle for cancellation and close so only the CREATED to CANCELLING winner performs cancellation side effects, while close safely accepts either CREATED or CANCELLING and publishes CLOSED. Keep notify_close() ahead of cancellation arbitration for recursive CTE fragments and publish the QueryContext failure before expensive diagnostics. ### Release note None ### Check List (For Author) - Test: Unit Test - GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=PipelineTaskTest.TEST_FRAGMENT_*LIFECYCLE - PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/check-format.sh - build-support/check-build-hygiene.sh - CLANG_TIDY_BINARY=/mnt/disk6/common/ldb_toolchain_028/bin/clang-tidy build-support/run-clang-tidy.sh --base apache/master --build-dir be/ut_build_ASAN - Behavior changed: Yes; repeated fragment cancellation diagnostics and side effects now run once, and cancellation after close is ignored. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#67261 Problem Summary: Document the monotonic PipelineFragmentContext termination lifecycle next to LifecycleState, including the normal-close and cancellation paths and the ownership of one-shot side effects. ### Release note None ### Check List - Test - No need to test; comment-only change. - check-format - check-build-hygiene - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: complete after two convergence rounds; one blocking correctness issue remains.
- Goal and correctness: the change is focused and the
CREATED -> CANCELLINGelection does suppress repeated fragment diagnostics and side effects, but cancellation is not serialized with final close. The direct asynchronous report-callback path can lose the query failure or queue a successful final load report, so the stated cancel/last-task-close goal is not yet met. - Concurrency and lifecycle: scheduler workers, the FragmentMgr report pool, and query-level cancellation are the relevant concurrent entrants.
_task_mutexstill protects task counts and recursive-close state, map removal remains outside that mutex, and shared callback ownership is sound; no second deadlock, lifetime leak, or lock-order issue was found. The blocking issue is the missing publication edge between the lifecycle CAS andQueryContext::AtomicStatus. - Parallel and special paths: task-originated errors, submit failure, query-wide cancellation, recursive CTE notification, and
LIMIT_REACH/FINISHEDhandling were traced without another defect. No configuration, static initialization, FE/BE variable, wire/storage-format, EditLog, or direct persistence change applies. - Tests: the added BE unit tests cover concurrent cancellation election and sequential close/post-close idempotence, but they join all cancellation threads before close and suppress final reporting, so they cannot prove the claimed cancel-vs-last-close ordering or final report status. A deterministic overlap test is needed. No builds or tests were run by this review because the review prompt prohibits them.
- Data, performance, and observability: there is no direct storage write change, but the stale OK final load report makes the accepted race data-correctness sensitive. The intended reduction in repeated heavy diagnostics is beneficial, and no additional memory, performance, header-hygiene, or observability issue was substantiated.
No additional user review focus was supplied. All candidates were independently validated, deduplicated, accepted, or dismissed with evidence, and all Round 2 reviewers returned NO_NEW_VALUABLE_FINDINGS.
| return; | ||
| } | ||
| auto expected = LifecycleState::CREATED; | ||
| if (!_lifecycle_state.compare_exchange_strong(expected, LifecycleState::CANCELLING, |
Contributor
There was a problem hiding this comment.
Serialize cancellation publication with close
The lifecycle election can still lose a cancellation that began before close. A load's periodic-report callback invokes this cancel() directly on the report-pool thread, while a scheduler worker can finalize the last task independently. After notify_close() sees that task open, the worker can win CREATED -> CLOSING before this CAS, so this path returns without ever publishing the error. If this CAS wins first, _try_start_close() can consume CANCELLING before _query_ctx->cancel() below runs; send_report(true) then snapshots QueryContext::exec_status() as OK and can enqueue a successful final load report. Acquire/release does not help because the error write is sequenced after this CAS. Please keep close from completing until the elected cancellation has published the query failure, ensure a last close that arrives during publication is retried, and add a deterministic test that overlaps cancellation with the last decrement; the current tests join all cancel threads before closing.
Contributor
Author
There was a problem hiding this comment.
I do not think this is a blocking correctness issue. The intended ordering point here is the lifecycle transition when the last task has drained, rather than entry into cancel() or the earlier notify_close() observation.
All task-originated failures are published before a task is counted as closed: TaskScheduler::close_task() calls ctx->cancel() for execute/close/finalize errors before decrement_running_task(). Therefore, final close cannot overtake an unpublished task execution failure.
The direct report-pool callback is a different case. If its cancellation overlaps the last task completion, allowing the last-task close to win is intentional: once _closed_tasks reaches _total_tasks, the PFC has no unfinished execution to cancel. CANCELLING elects the owner of per-fragment cancellation diagnostics and side effects; it is not a publication barrier that requires final close to wait. The final report snapshots QueryContext status at the close linearization point, and a late control-plane report failure does not retroactively invalidate the completed fragment.
For the same reason, the current test joins cancellation threads before close intentionally: it verifies single-winner cancellation side effects and one-shot close. An overlap test would have to allow either operation to win and would not establish that cancellation must take precedence.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17050 ms |
Contributor
TPC-DS: Total hot run time: 82431 ms |
Contributor
ClickBench: Total hot run time: 14.71 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
dismissed
github-actions[bot]’s stale review
August 31, 2026 04:34
no need dispose the problem
Contributor
TPC-H: Total hot run time: 16699 ms |
Contributor
TPC-DS: Total hot run time: 81857 ms |
Contributor
ClickBench: Total hot run time: 14.6 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #67236
Problem Summary:
When a query times out, every pending pipeline task can close with the same error and call
PipelineFragmentContext::cancel()before the fragment task count is drained. Each call previously repeated fragment cancellation logs, the full timeout task dump, stream-pipe cancellation, and dependency wakeups, causing severe log amplification.This PR replaces the separate cancellation/closed flags with one atomic terminal lifecycle inside
PipelineFragmentContext:CREATED -> CANCELLINGelects the only cancellation winner.CREATED/CANCELLING -> CLOSING -> CLOSEDmakes final close one-shot and handles the cancel/last-task-close race.notify_close()remains before cancellation arbitration so recursive CTE fragments retain their external close-notification semantics.QueryContexterror before expensive diagnostics, so closing tasks observe the failed query status.No per-fragment cancellation reason is stored because no runtime reader requires it; the query-wide first error remains owned by
QueryContext::AtomicStatus.Release note
None
Check List (For Author)
GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=PipelineTaskTest.TEST_FRAGMENT_*LIFECYCLEPATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/check-format.shbuild-support/check-build-hygiene.shCLANG_TIDY_BINARY=/mnt/disk6/common/ldb_toolchain_028/bin/clang-tidy build-support/run-clang-tidy.sh --base apache/master --build-dir be/ut_build_ASAN