Repository navigation
[improvement](be) Name the task of every resource context still alive at graceful exit - #68942
Merged
hello-stephen merged 2 commits intoOct 14, 2026
Merged
Conversation
… at graceful exit
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
The NonConcurrent regression pipeline fails a few percent of runs with
"Lsan fail" although every case passes. At the BE's graceful exit
(enable_graceful_exit_check=true), LeakSanitizer finds one stream load's
ResourceContext that was never released, together with what it keeps alive:
its CPU/memory/IO contexts, QueryTaskController and memory trackers, the
std::make_shared block of its QueryContext (kept allocated by the weak_ptr in
QueryTaskController), and the "normal" workload group (kept alive by
ResourceContext::_workload_group). The QueryContext itself was destroyed:
every "query_id: ... use wg" line in be.INFO has its "Query ... deconstructed".
So one reference to the load's ResourceContext is never released, from
somewhere LeakSanitizer does not scan (every leak is reported as indirect).
The report cannot say which of the ~430 HTTP stream loads of a run it was:
the allocation stacks are the same for all of them, and the identifying data
(the tracker label holding the load id) is inside the leaked memory, which
LeakSanitizer does not print.
In memleak-check mode, every ResourceContext now registers itself in a
process-wide set when constructed and removes itself when destroyed. The set
keeps the addresses bitwise inverted, so LeakSanitizer does not count it as a
reference and still reports a leaked context. Right after ExecEnv::destroy()
on the graceful exit path, doris_main prints one line per context still alive
(address, references still held, task id, finished, running time, memory
tracker, workload group, and for a query whether its QueryContext is gone) to
be.WARNING and to be.out, where the LeakSanitizer report follows. The address
falls inside one of the "Objects leaked above" ranges of that report.
Nothing changes when enable_graceful_exit_check is false (the default) except
one boolean check per ResourceContext construction and destruction.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Added ResourceContextLeakCheckTest.
- Locally: clang-format 16, build-support/check-build-hygiene.sh, and
-fsyntax-only of every changed file with the BE UT ASAN flags against
regenerated gensrc. The BE UT binary was not built or run locally; the
BE UT pipeline runs the new test.
- Behavior changed: No (adds logging in memleak-check mode only)
- Does this need documentation: No
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29216 ms |
Contributor
TPC-DS: Total hot run time: 152518 ms |
Contributor
ClickBench: Total hot run time: 24.63 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
Complete static review of PR #68942 at head 120a444a79f718ad84840f2a9c4cc94ef79d442a, covering all five changed files and their query/load/background-task and shutdown interactions. Two review rounds completed; both normal reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS in round 2. All candidates and initial risks were independently adjudicated, and the final changed-file sweep found no unresolved point.
One new P2 issue is reported inline: a detached manual-compaction worker can construct a ResourceContext during shutdown enumeration, racing the weak-owner initialization that follows constructor registration. This should be addressed before relying on the diagnostic's creator-quiescence assumption. No P0/P1 issue was substantiated. Refreshed existing reviews/inline comments contain no earlier finding to duplicate or carry as an existing blocker. No additional user review focus was supplied.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | The patch adds task/address/reference/tracker/query-liveness attribution for shutdown survivors. The ordinary behavior is covered by inspected unit-test assertions; the construction-overlap defect is reported inline. No executed test proves end-to-end shutdown behavior in this review. |
| Scope and reuse | Focused five-file diagnostic change; reuses ResourceContext construction, existing task controllers, trackers and workload groups. No unrelated change found. |
| Concurrency and synchronization | Registry insertion, erasure and lookup share one mutex, but make_shared's later weak-owner initialization does not. Detached manual base/cumulative compaction escapes pool/server draining, producing the reported P2. |
| Lock cost, order and deadlocks | Snapshot reserve/pinning occurs under the registry mutex; formatting and temporary-owner destruction occur outside it. No additional lock inversion or allocation-to-factory recursion substantiated. |
| Lifecycle and static initialization | Existing initialized survivors retain their child contexts, tracker and workload group. Query liveness uses a const weak observer without reviving QueryContext. Destructor erasure precedes weak-base teardown; lazy immortal registry avoids static-order dependencies and permanent owning references. Unsafe constructor publication is the inline finding. |
| Configuration and dynamic changes | No new configuration. Existing nonmutable enable_graceful_exit_check is fixed at startup; default-disabled mode skips registry insertion and uses the existing fast-exit path. |
| Compatibility and rolling upgrades | No wire/storage-format or distributed function-interface change. No source-visible newer libc/TLS dependency found; final GLIBC_2.17 ELF compatibility was not audited because builds are prohibited. |
| Thrift/protobuf IDs | No gensrc definition changed; branch-by-branch field-ID comparison is not applicable. |
| Parallel paths | Shared constructor covers query, standalone/reused load, snapshot and tracker/orphan attachments in cloud and shared-nothing modes. The detached local manual-compaction exception is included in the finding; no other distinct instance substantiated. |
| Conditional checks and failure handling | Mode gate, tracking flag, optional metadata, expired-owner skip, empty snapshot and output cap correspond to supported states. Erase invariant is checked; no new ignored Status or THROW_IF_ERROR boundary found. |
| Test coverage | Four GTest cases cover registration/release, reference counts, finished/task/tracker fields, disabled creation, query alive/destroyed and truncation; recursive runtime glob discovers them. Concurrent publication/final-release and actual LSan/graceful-shutdown behavior are untested here. |
| Test results | No golden/output result file changed. Expected strings agree with inspected implementation; runtime results were not produced or verified. |
| Observability | WARNING plus stderr supplies task IDs, addresses, tracker/workload metadata and state for LSan correlation. Caller limits detailed entries to 100. No running-path metric is required for this shutdown diagnostic. |
| Transactions and persistence | No EditLog/replay, persistent metadata, transaction or master-failover behavior changed; not applicable. |
| Data writes and crash atomicity | No data-write, version-visibility or delete-bitmap semantics changed; not applicable. |
| FE-BE variable propagation | No new transmitted variable or send path; not applicable. |
| Performance and memory accounting | Disabled mode adds an early branch; enabled insert/erase is O(1) average and snapshot is O(N) in live contexts with bounded formatting. Registry stores complemented addresses, snapshot owners expire before main returns, and accounting does not recursively construct contexts. No separate concrete scaling/accounting failure found. |
| Other BE/module obligations | No PCH, gate-table, template-pairing or test-includes-src-cpp change. Pure-text build-hygiene checks passed. No additional substantiated correctness or lifecycle issue remains beyond the inline finding. |
Validation was static-only. bash build-support/check-build-hygiene.sh passed header layering/bans/budgets/PCH, extern-template pairing and unity-skip coverage. No Doris build, unit/regression test, live cluster or sanitizer run was attempted; repository source files were not modified.
| std::lock_guard<std::mutex> l(live.lock); | ||
| contexts.reserve(live.disguised_contexts.size()); | ||
| for (uintptr_t disguised_context : live.disguised_contexts) { | ||
| if (auto reference = undisguise(disguised_context)->weak_from_this().lock()) { |
Contributor
There was a problem hiding this comment.
[P2] Synchronize context publication before taking the shutdown snapshot
ExecEnv::destroy() does not ensure that all context creators have stopped: CompactionAction::_handle_run_compaction() detaches manual base/cumulative compaction and waits only two seconds. A worker already owning its tablet and memory tracker can be delayed immediately before SCOPED_ATTACH_TASK(_mem_tracker) in BaseCompaction::execute_compact() / CumulativeCompaction::execute_compact(), then construct a context while this shutdown snapshot runs. The constructor registers this before make_shared initializes enable_shared_from_this's weak pointer; that later write is outside live.lock, so this weak_from_this() read races it (the C++ shared-pointer requirements explicitly make that initialization non-atomic). This introduces undefined behavior in memleak-check shutdown, and can omit the context from the diagnostic. Ensure all creators are joined before this call, or synchronize publication and metadata initialization before exposing a context to the registry; cover this overlap with a concurrency test.
… at graceful exit
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
The previous commit made doris_main describe the resource contexts that are
still alive after ExecEnv::destroy(), right before LeakSanitizer checks the
heap. Describing a context touches it, and doing that on the main thread can
leave a pointer to a leaked context in a dead slot of the main thread's
stack. LeakSanitizer scans that stack when the process exits; if it finds the
stale pointer it takes the context for referenced and does not report it, so
the very run that reproduces the leak could pass without "Lsan fail".
ResourceContext::log_live_contexts() now builds the description on a thread
of its own and joins it; the stack of a finished thread is not scanned, and
the main thread only handles the resulting text. It logs to be.WARNING and
stderr (be.out), or one INFO line when no context is alive, and doris_main
just calls it. Moving the logging out of doris_main, whose code after
"Doris main exiting" never shows up in regression coverage, also lets the unit
test cover it; the test now covers the workload group and missing tracker
cases too.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Extended ResourceContextLeakCheckTest.
- Locally: clang-format 16, build-support/check-build-hygiene.sh, and
-fsyntax-only of every changed file with the BE UT ASAN flags. The BE UT
binary was not built or run locally; the BE UT pipeline runs the test.
- Behavior changed: No (memleak-check mode only)
- Does this need documentation: No
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Complete static review of PR #68942 at c9c422199945c07be28665c590171d19c58fc03f, using the supplied aggregate diff and all five changed files. No additional substantiated inline findings.
The existing P2 publication/shutdown finding remains applicable. Detached manual base/cumulative compaction can create a ResourceContext while the shutdown snapshot runs; constructor registration precedes initialization of its shared-ownership observer and task metadata. The C++ specification confirms that the observer initialization is non-atomic. This issue is already reported and has not been duplicated. No existing P0/P1 comment applies, so existing_blocking_comment_ids is empty. C++ shared-pointer requirements
The reviewed lifecycle is:
enabled graceful exit
-> stop/join managed services and workers
-> ExecEnv::destroy()
-> diagnostic thread: snapshot under mutex -> unlock -> format -> release references
-> join diagnostic thread -> log returned text -> main returns
Detached manual compaction can overlap the snapshot: existing P2 above.
Checkpoint conclusions:
- Goal and proof: the diagnostic identifies residual contexts by address, task id, reference count, finish/runtime state, tracker usage, workload group, and QueryContext lifetime. Ordinary initialized contexts are covered by unit assertions; the existing publication issue prevents an unconditional shutdown-correctness conclusion.
- Scope and clarity: changes are focused on the registry, diagnostic methods, weak QueryContext accessor, one shutdown call, and five unit cases.
- Concurrency, lock weight, and order: registry insertion, erasure, and snapshot share one mutex. The snapshot reserves and traverses all entries under that mutex; formatting and final reference release occur outside it. No separate lock-order or final-release deadlock was substantiated. Actual constructor/metadata overlap is the existing P2.
- Lifecycle and static initialization: lazy function-local registry initialization avoids cross-TU ordering; its intentional process lifetime permits late unregistering. Snapshot strong references protect objects, and QueryTaskController retains a weak QueryContext observer. No new ownership cycle was found. From LLVM's pointer filtering and finished-thread cleanup, the inverted registry and joined helper do not introduce a demonstrated persistent context root; this is a source-based inference, without an executed LSan test. LLVM LSan root scanning, LLVM ASan thread cleanup
- Configuration: no new setting. Existing
enable_graceful_exit_checkis immutable and defaults to false; the ordinary main path exits before this diagnostic. Per-object_trackedkeeps unregistering balanced across test-only flag changes. - Compatibility: no storage/wire-format or rolling-upgrade change. The added C++ APIs are internal; no new libc API, dependency, or toolchain requirement was identified. Release ELF/glibc-2.17 compatibility was not tested.
- Thrift/protobuf ids: not applicable; no generated protocol definitions changed.
- Parallel paths: centralized constructor registration covers the production shared factories for query, load-channel/load-stream, task attachment/orphan fallback, and snapshot-loader contexts. Cloud and shared-nothing modes converge on the same diagnostic call.
- Conditional and error handling: disabled tracking, tracked membership, expired observers, missing tracker/workload group, query-specific controller, and output cap are actual documented states. Exact registry removal is asserted; no new ignored Status or silent invariant failure was found.
- Test coverage: the five discovered TEST_F cases cover registration/removal, external reference counts, finish/tracker/workload-group fields, disabled-mode construction, QueryContext expiration, and truncation. Fixtures restore configuration and identify their own task without depending on set order. Concurrent shutdown and actual LSan leak preservation are not covered; concurrency coverage is already requested in the existing thread.
- Test results: no regression goldens changed. Unit cases were inspected, not executed; author/CI claims are not treated as executed validation here.
- Observability: WARNING plus stderr provides task/address/tracker correlation; empty shutdown emits one INFO line. Output is capped at 100 contexts. A new operational metric is unnecessary for this shutdown diagnostic.
- Transactions and persistence: not applicable; transaction, EditLog/replay, and failover semantics are unchanged.
- Data writes and crash atomicity: not applicable; no data-write, visibility/version, or delete-bitmap logic changed.
- FE-BE variables: not applicable; no transmitted variable or sending path changed.
- Performance and memory accounting: disabled mode adds a registration check and historical tracking check; enabled registration is average O(1), and shutdown snapshot time/storage are O(live contexts). No allocation-driven factory recursion or distinct contention failure was found. The helper runs after ExecEnv readiness/accounting are disabled and uses ordinary diagnostic allocations without accessing deleted managers.
- Other concerns: all changed files and initial risk points were rechecked; no additional substantiated defect remains to report. The pure-text
build-support/check-build-hygiene.shpassed header layering/bans/budgets/PCH, extern pairing, and unity-skip coverage.
No additional user-provided review focus was supplied. Review analysis is COMPLETE: both normal reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS in round 1; the main agent independently verified/deduplicated every observation and completed the final changed-file/candidate sweep. No builds, unit/regression tests, or source edits were performed.
Contributor
TPC-H: Total hot run time: 29132 ms |
Contributor
TPC-DS: Total hot run time: 151843 ms |
Contributor
ClickBench: Total hot run time: 24.32 s |
bobhan1
approved these changes
Oct 14, 2026
suxiaogang223
approved these changes
Oct 14, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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:
In short. The NonConcurrent regression pipeline fails a few percent of its runs with
Lsan failwhile every case passes. LeakSanitizer finds, at the BE's graceful exit, one stream load'sResourceContextthat was never released, but its report only shows allocation stacks, which are the same for every stream load, so nobody can tell which load it was. This PR makes the BE print, at graceful exit in memleak-check mode, everyResourceContextstill alive with its task id, the references still held, its memory tracker and workload group, and whether itsQueryContextis already gone. Nothing changes unlessenable_graceful_exit_check=true.Background.
enable_graceful_exit_check=true(memleak-check mode, used by the regression pipelines on the ASAN build),doris_mainstops every service and runsExecEnv::destroy()instead of_exit(), so that LeakSanitizer checks the heap when the process exits. The NonConcurrent pipeline grepsbe.outfordetected memory leaksafterstop_be.sh --graceand reportsLsan fail.ResourceContext: CPU/memory/IO contexts, a task controller, the query memory tracker and the workload group. TheQueryContextowns it, and the components that work for the task (load channel, memtables, scanner context,RuntimeQueryStatisticsMgr, threads attached to the task, ...) hold references of their own while they run.The problem, and what it cost.
Lsan failhits 1–5% of NonConcurrent runs depending on the month (448 of the last 14000, back to March 2026), on PRs that have nothing to do with it; authors rerun or skip. Every report TeamCity still keeps (10 runs from 2026-10-05 to 2026-10-12, on [feature](lance) Search historical versions, tags and branches with vector_search and full_text_search #68707, [fix](fe) Bound session execution and scan concurrency #68281, [improvement](fe) Add opt-in spreading for remote external scan splits #68791, [test](fe) Cover credential-aware FileSystem cache keys for object stores #68803, branch-4.1: [fix](expr) Canonicalize logical OR results #68798, [fix](join) Support RIGHT OUTER JOIN in OrExpansion #68543, [improvement](datasource) Drop the statement scan reuse memo when planning ends #68901, [test](paimon) Port the branch-4.x Paimon write distribution and Variant write tests #68919, [fix](fs) Match object storage endpoints case-insensitively #68879) has the same shape, down to the byte counts, and all of its leaks are indirect:ResourceContextwith its CPU/memory/IO contexts,QueryTaskControllerand both trackers (Load#Id=…,Memtable#Load#Id=…)StreamLoadAction::_process_put→StreamLoadExecutor→FragmentMgr::_get_or_create_query_ctxnormalworkload group created byWorkloadGroupMgr::create_internal_wg()ResourceContext::_workload_groupstd::make_sharedblock of the load'sQueryContextweak_ptrinQueryTaskControllerThe
QueryContextitself was destroyed: inbe.INFO, each of the 21894query_id: … use wg:lines of the run has itsQuery … deconstructed. So one reference to the load'sResourceContextis never released, from a place LeakSanitizer does not scan (a bthread stack, whose memory brpc maps itself) or a count that is never decremented. LeakSanitizer cannot tell which of these two it is.A NonConcurrent run issues about 430 HTTP stream loads (about 380 through the load channel, 150 of them audit-log loads), and the report cannot say which one leaked: the stacks are identical, and the one thing that would identify the load, the tracker label with its id, is inside the leaked memory, which LeakSanitizer does not print. Without the load there is nothing to follow, so the flake stays unexplained.
How this PR fixes it.
ResourceContextregisters itself in a process-wide set when it is constructed and removes itself when it is destroyed. The set stores the addresses bitwise inverted, so LeakSanitizer does not take it for a reference and still reports a leaked context.ResourceContext::describe_live_contexts()returns one line per context still alive: address, strong references still held, task id, finished flag, running time, memory tracker (label, consumption, peak), workload group and, for a query, whether itsQueryContextis still alive.doris_maincallsResourceContext::log_live_contexts()right afterExecEnv::destroy()on the graceful exit path. It writes the description tobe.WARNINGand tobe.out, where the LeakSanitizer report follows oncemain()returns, or one INFO line when no context is alive. The printed address falls inside one of theObjects leaked aboveranges of that report.Results. What the next
Lsan failwill tell, compared with today:task_id=…; its label, table and test are then inbe.INFOreferences=NQueryContextis gonequery_context=destroyedbe.outbe.out, just before that report (the build log'shead -n 100 be.outshows it)enable_graceful_exit_check=false(default)ResourceContextconstruction and destructionA line looks like this (values made up):
Classes, and how they call each other.
ResourceContext(resource_context.{h,cpp}): registers in the constructor and unregisters in the destructor when memleak-check mode is on;describe_live_contexts()formats the live ones;log_live_contexts()runs that on its own thread and logs the result.QueryTaskController: newis_query_context_alive().doris_main: one call tolog_live_contexts()afterExecEnv::destroy().No holder of
ResourceContextchanges, and neither do the LeakSanitizer options or suppressions. Once the leaked load is known, the fix for the leak itself will be a separate PR.Release note
None
Check List (For Author)
Test
Added
ResourceContextLeakCheckTest(be/test/runtime/workload_management/resource_context_test.cpp): a context is listed with its task id, references and tracker until it is released, both bydescribe_live_contexts()and bylog_live_contexts(); contexts created outside memleak-check mode are not tracked; a query's context showsquery_context=destroyedonce itsQueryContextis gone; workload group and missing tracker are shown; the output is capped. On the first revision, NonConcurrent loggedNo resource context is alive after shutdownat both graceful exits (no live context in a normal run) and took 141 minutes, the median of the 43 previous full runs. Locally: clang-format 16,build-support/check-build-hygiene.sh, and-fsyntax-onlyof every changed file with the BE UT ASAN flags against regenerated gensrc. The BE UT binary was not built or run locally; the BE UT pipeline runs the new test.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)