Skip to content

Commit 1c4d677

Browse files
[Refactor] Move FragmentContext into ExecRuntime (#74774)
Signed-off-by: alvin-phoenix-ai <alvin.zhao@phoenixdata.ai>
1 parent d6ea9da commit 1c4d677

15 files changed

Lines changed: 175 additions & 40 deletions

be/AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ Primitive execution contracts, DataSink base contract, runtime-filter infrastruc
233233
### ExecRuntime (`execruntime`)
234234
Execution runtime driver lifecycle and scheduling-adjacent behavior above ComputeEnv and ExecPrimitive without concrete operators, storage, service, connector, cache, or broad Exec coupling.
235235
- Targets: `ExecRuntime`
236-
- Allowed internal include prefixes: `compute_env/`, `exec/runtime/`, `exec/pipeline/group_execution/execution_group.h`, `exec/pipeline/group_execution/execution_group_builder.h`, `exec/pipeline/group_execution/execution_group_fwd.h`, `exec/pipeline/adaptive/adaptive_dop_param.h`, `exec/pipeline/pipeline_driver.h`, `exec/pipeline/pipeline_fwd.h`, `exec/pipeline/operator.h`, `exec/pipeline/operator_with_dependency.h`, `exec/pipeline/source_operator.h`, `exec/pipeline/primitives/`, `exec/pipeline/schedule/common.h`, `exec/pipeline/schedule/event_scheduler.h`, `exec/pipeline/schedule/pipeline_driver_observer.h`, `exec/pipeline/schedule/utils.h`, `exec/pipeline/runtime_filter_hub.h`, `exec/pipeline/runtime_filter_core_types.h`, `exec/pipeline/scan/morsel_queue.h`, `exec/pipeline/scan/split_morsel_ticket_checker.h`, `exec/pipeline/scan/ticketed_morsel_queue.h`, `exec/runtime_filter/`, `storage/primitive/`, `exprs/`, `serde/`, `runtime/env/`, `runtime/`, `platform/`, `fs/`, `io/`, `column/`, `types/`, `common/`, `base/`, `gutil/`, `gen_cpp/`
236+
- Allowed internal include prefixes: `compute_env/`, `exec/runtime/`, `exec/pipeline/fragment_context.h`, `exec/pipeline/group_execution/execution_group.h`, `exec/pipeline/group_execution/execution_group_builder.h`, `exec/pipeline/group_execution/execution_group_fwd.h`, `exec/pipeline/adaptive/adaptive_dop_param.h`, `exec/pipeline/pipeline_driver.h`, `exec/pipeline/pipeline_fwd.h`, `exec/pipeline/operator.h`, `exec/pipeline/operator_with_dependency.h`, `exec/pipeline/source_operator.h`, `exec/pipeline/primitives/`, `exec/pipeline/schedule/common.h`, `exec/pipeline/schedule/event_scheduler.h`, `exec/pipeline/schedule/pipeline_driver_observer.h`, `exec/pipeline/schedule/utils.h`, `exec/pipeline/runtime_filter_hub.h`, `exec/pipeline/runtime_filter_core_types.h`, `exec/pipeline/scan/morsel_queue.h`, `exec/pipeline/scan/split_morsel_ticket_checker.h`, `exec/pipeline/scan/ticketed_morsel_queue.h`, `exec/runtime_filter/`, `storage/primitive/`, `exprs/`, `serde/`, `runtime/env/`, `runtime/`, `platform/`, `fs/`, `io/`, `column/`, `types/`, `common/`, `base/`, `gutil/`, `gen_cpp/`
237237
- Allowed target deps: `ComputeEnv`, `ExecPrimitive`, `StoragePrimitive`, `ExprCore`, `Serde`, `RuntimeEnv`, `RuntimeCore`, `Platform`, `FSCore`, `IO`, `ChunkCore`, `ColumnCore`, `Types`, `Common`, `Base`, `Gutil`, `StarRocksGen`
238238
- Core tests: `exec_runtime_test`
239239
- Remediation: Keep ExecRuntime limited to driver lifecycle and runtime behavior that can be expressed through ComputeEnv and ExecPrimitive contracts; move concrete operators, storage, service, connector, cache, HTTP, and broad Exec integration upward.

be/module_boundary_manifest.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,7 @@
542542
"allowed_include_prefixes": [
543543
"compute_env/",
544544
"exec/runtime/",
545+
"exec/pipeline/fragment_context.h",
545546
"exec/pipeline/group_execution/execution_group.h",
546547
"exec/pipeline/group_execution/execution_group_builder.h",
547548
"exec/pipeline/group_execution/execution_group_fwd.h",
@@ -579,7 +580,6 @@
579580
],
580581
"forbidden_include_prefixes": ["exec/", "storage/", "service/", "connector/", "cache/", "http/", "agent/", "util/"],
581582
"forbidden_includes": [
582-
"exec/pipeline/fragment_context.h",
583583
"exec/pipeline/pipeline_driver_executor.h",
584584
"exec/pipeline/pipeline_driver_queue.h",
585585
"exec/pipeline/pipeline_driver_poller.h",

be/src/exec/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,6 @@ set(PIPELINE_RUNTIME_SRCS
159159
pipeline/pipeline_driver_poller.cpp
160160
pipeline/audit_statistics_reporter.cpp
161161
pipeline/exec_state_reporter.cpp
162-
pipeline/fragment_context.cpp
163162
pipeline/fragment_context_cancel.cpp
164163
pipeline/fragment_context_manager.cpp
165164
pipeline/query_context.cpp
@@ -507,6 +506,7 @@ ADD_BE_LIB(ExecRuntime
507506
pipeline/group_execution/execution_group_builder.cpp
508507
pipeline/pipeline.cpp
509508
pipeline/pipeline_driver.cpp
509+
pipeline/fragment_context.cpp
510510
pipeline/schedule/event_scheduler.cpp
511511
pipeline/schedule/pipeline_driver_observer.cpp
512512
)

be/src/exec/pipeline/fragment_context.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@
4444
#include "exec/runtime/query_runtime_state.h"
4545
#include "platform/query_timeout_hook.h"
4646
#include "platform/thrift_rpc_helper.h"
47-
#include "runtime/exec_env.h"
4847
#include "runtime/fragment_attachment.h"
48+
#include "runtime/service_contexts.h"
4949

5050
namespace starrocks::pipeline {
5151

@@ -276,9 +276,10 @@ void FragmentContext::set_final_status(const Status& status) {
276276

277277
const bool finished_cancel = detailed_message == "QueryFinished" || detailed_message == "LimitReach";
278278
if (!s_status.ok() && !finished_cancel) {
279-
const auto* executors = workgroup() != nullptr
280-
? workgroup()->executors()
281-
: ExecEnv::GetInstance()->workgroup_manager()->shared_executors();
279+
const auto* executors =
280+
workgroup() != nullptr
281+
? workgroup()->executors()
282+
: execution_services(_runtime_state.get()).workgroup_manager->shared_executors();
282283
auto* executor = executors->driver_executor();
283284
auto* query_ctx = _runtime_state->query_ctx();
284285
if (query_ctx != nullptr) {
@@ -340,10 +341,6 @@ Status FragmentContext::prepare_all_pipelines() {
340341
return Status::OK();
341342
}
342343

343-
void FragmentContext::_set_default_workgroup() {
344-
set_workgroup(ExecEnv::GetInstance()->workgroup_manager()->get_default_workgroup());
345-
}
346-
347344
void FragmentContext::set_fragment_attachments(std::vector<std::unique_ptr<FragmentAttachment>>&& attachments) {
348345
_fragment_attachments = std::move(attachments);
349346
}

be/src/exec/pipeline/fragment_context.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,6 @@ class FragmentContext : public ExecutionGroupLifecycle {
188188
void init_event_scheduler();
189189

190190
private:
191-
void _set_default_workgroup();
192191
void _close_fragment_attachments();
193192

194193
bool _enable_group_execution = false;

be/src/exec/pipeline/fragment_context_manager.cpp

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,23 +44,6 @@ const RuntimeServices& runtime_services(const RuntimeState* state) {
4444
FragmentContextManager::FragmentContextManager(FragmentLifecycleWeakPtr fragment_lifecycle)
4545
: _fragment_lifecycle(std::move(fragment_lifecycle)) {}
4646

47-
FragmentContext* FragmentContextManager::get_or_register(const TUniqueId& fragment_id) {
48-
std::lock_guard<std::mutex> lock(_lock);
49-
auto it = _fragment_contexts.find(fragment_id);
50-
if (it != _fragment_contexts.end()) {
51-
return it->second.get();
52-
} else {
53-
auto&& ctx = std::make_unique<FragmentContext>();
54-
auto* raw_ctx = ctx.get();
55-
if (!_fragment_lifecycle.expired()) {
56-
raw_ctx->set_fragment_lifecycle(_fragment_lifecycle);
57-
}
58-
_fragment_contexts.emplace(fragment_id, std::move(ctx));
59-
raw_ctx->_set_default_workgroup();
60-
return raw_ctx;
61-
}
62-
}
63-
6447
Status FragmentContextManager::register_ctx(const TUniqueId& fragment_id, FragmentContextPtr fragment_ctx) {
6548
std::lock_guard<std::mutex> lock(_lock);
6649

be/src/exec/pipeline/fragment_context_manager.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ class FragmentContextManager {
3636
FragmentContextManager& operator=(const FragmentContextManager&) = delete;
3737
FragmentContextManager& operator=(FragmentContextManager&&) = delete;
3838

39-
FragmentContext* get_or_register(const TUniqueId& fragment_id);
4039
FragmentContextPtr get(const TUniqueId& fragment_id);
4140

4241
Status register_ctx(const TUniqueId& fragment_id, FragmentContextPtr fragment_ctx);

be/src/exec/pipeline/group_execution/execution_group.cpp

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,20 @@ void ExecutionGroup::prepare_active_drivers_parallel(const std::shared_ptr<Runti
9191
DCHECK(pipeline_prepare_pool != nullptr);
9292

9393
for_each_active_driver(_pipelines, [&](const DriverPtr& driver) {
94-
// Since prepare is async, hold both the runtime state and driver until the task finishes.
95-
bool submitted = pipeline_prepare_pool->try_offer([sync_ctx, driver, runtime_state_holder = state]() {
94+
// Capture the runtime state by value to keep it alive across the async prepare, but capture
95+
// `driver` by reference -- the worker must NOT own the driver. The reference binds to the
96+
// stable element in `pipeline->drivers()`, which the wait barrier below keeps alive until
97+
// every task finishes; the worker only touches it inside prepare_local_state().
98+
//
99+
// Do NOT capture the driver by value (shared_ptr). The waiter wakes as soon as pending_tasks
100+
// hits 0 and, on prepare error, tears down the fragment -- destroying each Pipeline and its
101+
// operator factories. Some operators (e.g. OlapScanOperator) hold a context whose buffer is
102+
// owned by an operator factory (OlapScanContext -> OlapScanContextFactory::_chunk_buffer). If
103+
// the worker owned the driver, it could become the last owner and run ~PipelineDriver on the
104+
// prepare thread AFTER those factories were freed, closing a dangling buffer (use-after-free).
105+
// Keeping the worker non-owning ensures the driver is destroyed only by ~Pipeline, in the safe
106+
// _drivers-before-_op_factories order, on the fragment thread.
107+
bool submitted = pipeline_prepare_pool->try_offer([sync_ctx, &driver, runtime_state_holder = state]() {
96108
auto runtime_state = runtime_state_holder.get();
97109
// make sure mem tracker is instance level
98110
auto mem_tracker = runtime_state->instance_mem_tracker();

be/test/exec/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ set(EXEC_RUNTIME_TEST_FILES
5959
pipeline_driver_exec_runtime_test.cpp
6060
pipeline_event_scheduler_exec_runtime_test.cpp
6161
pipeline_execution_group_exec_runtime_test.cpp
62+
pipeline_fragment_context_exec_runtime_test.cpp
6263
pipeline_fragment_runtime_state_exec_runtime_test.cpp
6364
pipeline_query_runtime_state_exec_runtime_test.cpp
6465
)

be/test/exec/pipeline/pipeline_file_scan_node_test.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <utility>
2222

2323
#include "base/testutil/assert.h"
24+
#include "base/uid_util.h"
2425
#include "base/utility/defer_op.h"
2526
#include "column/chunk.h"
2627
#include "column/column_helper.h"
@@ -73,6 +74,9 @@ class PipeLineFileScanNodeTest : public ::testing::Test {
7374
_exec_env = ExecEnv::GetInstance();
7475
_exec_env->process_metrics_registry()->root_registry()->set_collect_hook_enabled(true);
7576

77+
_request.params.query_id = generate_uuid();
78+
_request.params.fragment_instance_id = generate_uuid();
79+
7680
const auto& params = _request.params;
7781
const auto& query_id = params.query_id;
7882
const auto& fragment_id = params.fragment_instance_id;
@@ -86,15 +90,17 @@ class PipeLineFileScanNodeTest : public ::testing::Test {
8690
_query_ctx->init_mem_tracker(GlobalEnv::GetInstance()->query_pool_mem_tracker()->limit(),
8791
GlobalEnv::GetInstance()->query_pool_mem_tracker());
8892

89-
_fragment_ctx = _query_ctx->fragment_mgr()->get_or_register(fragment_id);
93+
auto fragment_ctx = std::make_shared<FragmentContext>();
94+
_fragment_ctx = fragment_ctx.get();
9095
_fragment_ctx->set_query_id(query_id);
9196
_fragment_ctx->set_fragment_instance_id(fragment_id);
97+
_fragment_ctx->set_workgroup(_exec_env->workgroup_manager()->get_default_workgroup());
9298
_fragment_ctx->set_runtime_state(std::make_unique<RuntimeState>(
9399
_request.params.query_id, _request.params.fragment_instance_id, _request.query_options,
94100
_request.query_globals, &_exec_env->query_execution_services(), _exec_env));
95-
_fragment_ctx->set_workgroup(ExecEnv::GetInstance()->workgroup_manager()->get_default_workgroup());
96101

97102
_fragment_future = _fragment_ctx->finish_future();
103+
ASSERT_OK(_query_ctx->fragment_mgr()->register_ctx(fragment_id, std::move(fragment_ctx)));
98104
_runtime_state = _fragment_ctx->runtime_state();
99105

100106
_runtime_state->set_chunk_size(config::vector_chunk_size);

0 commit comments

Comments
 (0)