[fix](be) Avoid local runtime filter merge deadlock#64866
Open
BiteTheDDDDt wants to merge 4 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Local runtime filter merge can deadlock when one join build instance publishes a local-merge runtime filter while another instance sends its runtime filter size. The old local merge context lock protected both the merger and the producer list, so one path could hold a producer runtime filter lock and then wait for the context lock while another path held the context lock and then waited for a producer lock.
This change gives RuntimeFilterMerger its own internal synchronization and makes LocalMergeContext expose a snapshot of the merger and producers. Publish, send-size, and sync-size paths take the context lock only while copying that snapshot, then merge filters or update producer sizes outside the context lock. RuntimeFilterMerger returns the ready transition from merge_from directly, removing the separate unlocked ready check.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- build-support/clang-format.sh be/src/exec/runtime_filter/runtime_filter_merger.h be/src/exec/runtime_filter/runtime_filter_mgr.cpp be/src/exec/runtime_filter/runtime_filter_mgr.h be/src/exec/runtime_filter/runtime_filter_producer.cpp be/test/exec/runtime_filter/runtime_filter_merger_test.cpp be/test/exec/runtime_filter/runtime_filter_mgr_test.cpp
- git diff --cached --check
- ./run-be-ut.sh --run --filter=RuntimeFilterMgrTest.*
- ./run-be-ut.sh --run --filter=RuntimeFilterMergerTest.*
- Behavior changed: No
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary for PR #64866
I reviewed the runtime-filter merger/producer/manager changes and the adjacent runtime-filter tests. I did not find a substantiated issue that should be raised as an inline review comment.
Critical checkpoints:
- Goal/test: the change targets the local runtime-filter merge deadlock by moving producer/merger work out of
LocalMergeContextlocking and synchronizingRuntimeFilterMergerinternally. The updated BE UTs cover the adjusted merger readiness and manager API behavior. I could not run BE UTs locally becausethirdparty/installedis missing in this checkout. - Scope/clarity: the patch is focused on runtime-filter locking, snapshots, and related unit-test updates.
- Concurrency/lifecycle: I checked producer
publish()/send_size(), local snapshots, merger locking, global merge reset, and recursive CTE reset/rebuild sequencing. I did not find a new lock-order cycle or a substantiated stale-stage publication issue. - Config/compatibility: no new config was added; the runtime-filter stage fields used on RPCs are existing optional protobuf fields.
- Parallel paths: local-only, local-merge, remote merge, size-sync, stale recursive-CTE RPC handling, and local consumer signaling paths were reviewed.
- Tests/style:
git diff --checkpassed, andbuild-support/check-format.shpassed whenclang-format16 was selected inPATH. BE UT execution was not possible locally because the checkout lacksthirdparty/installed.
Subagent conclusions:
optimizer-rewriteproposed OPT-1 about stale recursive-CTE local publication. I dismissed it with lifecycle evidence:WAIT_FOR_DESTROYcompletes old PFC teardown beforeREBUILDregisters new-stage consumers/producers, while remote stale messages are stage-checked.tests-session-configreported no candidates.- Convergence round 1 ended with both live subagents reporting
NO_NEW_VALUABLE_FINDINGSfor the final no-inline-comment set.
User focus: no additional user-provided review focus was supplied.
Contributor
TPC-H: Total hot run time: 28845 ms |
Contributor
TPC-DS: Total hot run time: 173691 ms |
Contributor
ClickBench: Total hot run time: 25.24 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #64866.
I reviewed the six changed runtime-filter files and the relevant producer/helper, manager, pipeline prepare/submit, recursive CTE rerun, and sync/publish paths. I did not find a substantiated issue requiring an inline review comment.
Critical checkpoint conclusions:
- Goal: the PR targets a local runtime filter merge deadlock by moving merge/size work out from the old context lock and giving RuntimeFilterMerger its own synchronization. The changed
merge_from(..., &ready), size aggregation, shared context lookup, and producer publish/send-size call sites are consistent with that goal. - Scope and focus: the change is confined to BE runtime-filter manager/producer/merger code and matching BE unit-test API updates.
- Concurrency/lifecycle: I specifically checked producer registration, send-size, publish, global merge, and recursive CTE stage reset ordering. Producer registration happens during task prepare before fragment submit; parallel prepare waits for all instances; recursive CTE runs WAIT_FOR_DESTROY -> reset_global_rf -> REBUILD/prepare -> SUBMIT; stale-stage requests are filtered through stage checks. I did not find an executable same-stage registration/read race in the reviewed paths.
- Configuration, protocol, storage, persistence: no new config, thrift/protobuf field, storage format, or persistence behavior is introduced.
- Parallel paths: local merge publish, local size sync, and global merge-controller merge paths were updated to use the new ready transition. I did not find a missed parallel runtime-filter path in the changed surface.
- Tests: the PR updates BE unit tests for the new
merge_fromready result and local merge context API. I could not run BE UT in this checkout becausethirdparty/installedis absent. I did rungit diff --checkon the exact PR range, and it passed. - Observability/security: no security-sensitive path is changed; debug output is preserved through the new
LocalMergeContext::debug_string()path. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite or runtime-filter semantic regression found.tests-session-config: raised TEST-1 about unsynchronizedLocalMergeContext::producersreads. I dismissed it with lifecycle evidence above; it remains a useful concurrent-regression test gap but not a substantiated inline issue.- Convergence round 1 ended with both live subagents reporting
NO_NEW_VALUABLE_FINDINGSfor the same ledger and empty proposed inline comment set.
Contributor
TPC-H: Total hot run time: 29309 ms |
Contributor
TPC-DS: Total hot run time: 171988 ms |
Contributor
ClickBench: Total hot run time: 25.22 s |
yiguolei
reviewed
Jun 26, 2026
yiguolei
reviewed
Jun 26, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29536 ms |
Contributor
TPC-DS: Total hot run time: 171619 ms |
Contributor
ClickBench: Total hot run time: 25.3 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29130 ms |
Contributor
TPC-DS: Total hot run time: 172406 ms |
Contributor
ClickBench: Total hot run time: 25.26 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
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
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #64866.
I found one issue: the changed unit tests do not cover the nonzero recursive-stage local merge path that the production change now relies on for context replacement and stale-stage filtering.
Critical checkpoint conclusions:
- Goal/test: the production change addresses local runtime-filter merge synchronization and recursive-stage stale filtering. Merger readiness, manager API renaming, and default stage-0 behavior are covered, but nonzero-stage local merge replacement/stale filtering is not covered.
- Scope/clarity: the patch is focused on BE runtime-filter manager/producer/merger code and matching BE unit-test API updates.
- Concurrency/lifecycle: I checked local merge registration, publish, send-size, sync-size, global merge reset, PipelineFragmentContext prepare/submit ordering, and recursive CTE WAIT_FOR_DESTROY -> reset_global_rf -> REBUILD -> SUBMIT ordering. The producer-vector synchronization notes overlap the existing inline thread and are not submitted again.
- Config/compatibility/persistence/storage: no new config, storage format, persistence, or FE-BE protocol field is introduced by this PR.
- Parallel paths: local merge publish, local size sync, global merge, stale-stage RPC checks, hash join and set sink runtime-filter paths were reviewed.
- Tests/style: exact-range
git diff --checkpassed locally. I did not run BE UTs because this checkout lacksthirdparty/installed; local clang-format validation was also not run because only clang-format 18 is available and Doris requires v16. Current GitHub status contexts show BE UT and compile success on the head; the failed macOS BE UT check exits early because the runner has Java 25 instead of JDK 17. - Observability/security: no security-sensitive behavior is changed; debug output is preserved through
LocalMergeContext::debug_string().
Subagent conclusions:
optimizer-rewrite: OPT-1 was treated as duplicate of the existing producer-vector synchronization thread; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: TSC-1 was merged as the same duplicate synchronization point; TSC-2 became the single inline test-coverage comment; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.
User focus: no additional user-provided review focus was supplied.
| // (graceful skip for recursive CTE stage reset). | ||
| EXPECT_TRUE(global_runtime_filter_mgr | ||
| ->get_local_merge_producer_filters(filter_id, &local_merge_filters) | ||
| ->get_local_merge_context(filter_id, producer_filter->stage(), &context) |
Contributor
There was a problem hiding this comment.
This test still only covers the default stage-0 path: producer_filter->stage() is never changed, and the PSyncFilterSizeRequest below also keeps protobuf stage 0. The production change now relies on nonzero recursive stages to replace the local merge context and ignore stale-stage lookups/size syncs, while the recursive CTE regression has runtime_filter_mode=off, so it will not exercise this path. Please add a focused BE UT that registers a stage-0 producer, then a stage-1 producer for the same filter, verifies stage-0 get_local_merge_context/sync_filter_size are ignored, and verifies the current stage still syncs normally.
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
4 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: None
Problem Summary: Local runtime filter merge can deadlock when one join build instance publishes a local-merge runtime filter while another instance sends its runtime filter size. The old local merge context lock protected both the merger and the producer list, so one path could hold a producer runtime filter lock and then wait for the context lock while another path held the context lock and then waited for a producer lock.
This change gives RuntimeFilterMerger its own internal synchronization and makes LocalMergeContext expose a snapshot of the merger and producers. Publish, send-size, and sync-size paths take the context lock only while copying that snapshot, then merge filters or update producer sizes outside the context lock. RuntimeFilterMerger returns the ready transition from merge_from directly, removing the separate unlocked ready check.
Release note
None
Check List (For Author)