Doris-Breakwater commented on issue #67031: URL: https://github.com/apache/doris/issues/67031#issuecomment-5369664438
Breakwater-GitHub-Analysis-Slot: slot_0d7ad34c956d ## Initial maintainer analysis **Disposition:** the `PipelineTask::set_running` defect is confirmed in code and should be treated as a high-priority BE correctness issue, especially for aarch64 deployments under scheduler/spill contention. The proposed `std::atomic::exchange` change in #67032 is the correct minimal fix. The reported aggregation crash is technically consistent with the resulting double execution, but the currently supplied production evidence is not sufficient to prove that this specific race caused those crashes. The issue currently has no labels. It also combines two independent defects, and both linked PRs use `close #67031`; that should be cleaned up so merging one PR does not hide the outstanding work in the other. ### Verified facts - At the reported master SHA `1f53b285c3548de12ad521234e27f1805a4f313e`, `PipelineTask::set_running` performs one `compare_exchange_weak` and returns the mutated `expected` value. For `set_running(true)`, a permitted spurious failure while `_running == false` returns `false` without storing `true`. - `TaskScheduler::_do_work` interprets that `false` return as successful acquisition and proceeds to `execute()`. `RevokableTask::set_running` delegates to the underlying `PipelineTask`, so normal execution and spill revocation rely on the same gate. - The aggregation race mechanism is credible: `AggSharedState::reset_hash_table()` destroys aggregate states, sets mapped pointers to `nullptr`, and replaces the hash table, while `AggSinkLocalState::close()` swaps out `_places`. These structures are not designed for concurrent mutation by two executions of one pipeline task. - `std::atomic::exchange(running)` exactly implements the documented contract (always store the requested state and return the previous state). Its default sequentially consistent ordering also preserves the ordering strength of the current default CAS. - The same weak-CAS implementation is present at the current public `branch-4.0` tip (`8a9961723e`) and `branch-4.1` tip (`4c8235a2cb`), so the stated 4.x branch impact is confirmed for those two branches. - The separate `SpillContext` accounting imbalance is also present: the context is initialized with `chosen_tasks.size()`, and a failed `PipelineTask::revoke_memory()` submission returns before the failed and remaining unsubmitted tasks call `on_task_finished()`. ### Important qualifications - The C++ correctness problem is not formally ARM-only: `compare_exchange_weak` is allowed to fail spuriously on any conforming implementation. It is reasonable to describe aarch64/LL-SC as the observed high-risk environment, but “x86 can never fail spuriously” and “every ARM deployment is exposed” should be phrased as implementation/operational observations rather than language guarantees. - At the reported master SHA, `AggSinkOperatorX::sink_impl` spans lines 989-1004 and line 999 is the end of the input-row block; the executor call is at line 995. Optimized/inlined symbolization can explain this, but `si_addr == 0`, a `sink_impl` frame, and two workers crashing in the same second do not by themselves prove that both workers were executing the same `PipelineTask`. - The claimed permanent silent hang from the submit-error path is **not established on current master**. `QueryTaskController::revoke_memory()` returns the submit error to `WorkloadGroupMgr::handle_single_query_()`, which calls `cancel(status)`, and `QueryContext::cancel()` calls `_memory_sufficient_dependency->set_always_ready()`. The accounting invariant is still worth fixing, but the current code supports “incomplete `SpillContext`/callback accounting followed by query cancellation,” not an indefinitely blocked query. - #67033's added test only demonstrates the generic `SpillContext` counter contract. It does not inject a scheduler submit failure or exercise `QueryTaskController::revoke_memory()` and therefore does not regress the branch being changed. - Public CI has not yet run the relevant BE unit tests for either #67032 or #67033; both PRs are still awaiting review. ### Information still needed For correlation of the production SIGSEGV to this confirmed gate defect, please attach: 1. The exact Doris build commit for the crashing binary, compiler/toolchain version, and the exact affected 4.x branch/commit if different from master. 2. A complete symbolized stack for all worker threads from at least one matching core dump, plus the binary/debug-symbol build ID. Include query/fragment/task identifiers where available so maintainers can verify that the two workers held the same pipeline task rather than only the same query. 3. BE log excerpts covering the crash window, especially spill revocation submission/completion, scheduler wake/requeue, cancellation, and the affected query ID. 4. The command and result for the new mutual-exclusion test on an aarch64 BE test build. The stress test is useful coverage but probabilistic against the old implementation. For the second defect, please provide any observed submit-error and hang logs if a hang was actually seen, or identify a call path that does not pass through `handle_single_query_()` cancellation. Otherwise the issue/PR description and release note should be narrowed to the verified accounting defect. ### Recommended next steps 1. Prioritize review of #67032. Run the focused BE unit test on aarch64 (and the normal x86 BE test lane), plus formatting/static checks. The one-line `exchange` fix is mechanically and semantically appropriate. 2. After #67032 is validated, backport it independently to the supported 4.0 and 4.1 branches with branch-specific compilation/tests. 3. Track the spill-accounting change separately (new issue or non-closing related reference from #67033). Revise the hang claim unless new evidence demonstrates a non-canceling path, and add a failure-injection/unit test that makes submission fail inside `QueryTaskController::revoke_memory()` and verifies the returned status and full task accounting. 4. Use only one closing PR for #67031, or split the issues before merging, so GitHub does not close this report while the second independent change remains open. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
