Skip to content

[fix](be) Stop spill GC threads in workload group test teardown - #66534

Merged
mrhhsg merged 1 commit into
apache:masterfrom
mrhhsg:fix/spill-gc-thread-shutdown
Aug 6, 2026
Merged

[fix](be) Stop spill GC threads in workload group test teardown#66534
mrhhsg merged 1 commit into
apache:masterfrom
mrhhsg:fix/spill-gc-thread-shutdown

Conversation

@mrhhsg

@mrhhsg mrhhsg commented Aug 6, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Related PR: #66328

Problem Summary:

WorkloadGroupManagerTest creates and initializes a SpillFileManager for every test case. Initialization starts a background spill GC thread, but the fixture did not stop or delete the manager during teardown. Each subsequent test overwrote the global manager pointer, leaving the previous manager and its GC thread alive.

The leaked threads could continue accessing process-wide test utilities during static destruction and trigger an ASAN heap-use-after-free when the test binary exited. They could also interfere with tests that use global spill GC debug points.

This change releases workload-group-owned query resources first, then stops and joins the spill GC thread, deletes the manager, and finally removes the temporary spill directory. This matches the lifecycle used by other spill-related test fixtures.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
      • GTEST_REPEAT=10 ./run-be-ut.sh --run --filter='WorkloadGroupManagerTest.*:DebugPointsTest.AddTest:SpillFileTest.RetryPreservesDirectoryQueuedAfterPendingDrain' -j 8
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No. This only corrects test fixture resource teardown.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### What problem does this PR solve?

Issue Number: None

Related PR: apache#66328

Problem Summary: `WorkloadGroupManagerTest` creates and initializes a `SpillFileManager` for every test case. Initialization starts a background spill GC thread, but the fixture did not stop or delete the manager during teardown. Each subsequent test overwrote the global manager pointer, leaving the previous manager and its GC thread alive. The leaked threads could access process-wide test utilities during static destruction and trigger an ASAN heap-use-after-free when the test binary exited. Release workload-group-owned query resources first, then stop and join the GC thread and delete the fixture-owned manager before removing its spill directory.

### Release note

None

### Check List (For Author)

- Test: Unit Test
    - `GTEST_REPEAT=10 ./run-be-ut.sh --run --filter='WorkloadGroupManagerTest.*:DebugPointsTest.AddTest:SpillFileTest.RetryPreservesDirectoryQueuedAfterPendingDrain' -j 8`
- Behavior changed: No
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@mrhhsg

mrhhsg commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

/review

@mrhhsg

mrhhsg commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

run beut

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z.
Workflow run: https://github.com/apache/doris/actions/runs/31080787368

Please trigger /review again after that time.

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Aug 6, 2026
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 59.77% (26073/43621)
Line Coverage 44.09% (264389/599625)
Region Coverage 39.85% (210607/528465)
Branch Coverage 41.26% (96417/233680)

@hello-stephen

Copy link
Copy Markdown
Contributor

skip buildall

@mrhhsg
mrhhsg merged commit 8e4bd4d into apache:master Aug 6, 2026
33 checks passed
@mrhhsg
mrhhsg deleted the fix/spill-gc-thread-shutdown branch August 6, 2026 09:14
github-actions Bot pushed a commit that referenced this pull request Aug 6, 2026
### What problem does this PR solve?

Issue Number: None

Related PR: #66328

Problem Summary:

`WorkloadGroupManagerTest` creates and initializes a `SpillFileManager`
for every test case. Initialization starts a background spill GC thread,
but the fixture did not stop or delete the manager during teardown. Each
subsequent test overwrote the global manager pointer, leaving the
previous manager and its GC thread alive.

The leaked threads could continue accessing process-wide test utilities
during static destruction and trigger an ASAN heap-use-after-free when
the test binary exited. They could also interfere with tests that use
global spill GC debug points.

This change releases workload-group-owned query resources first, then
stops and joins the spill GC thread, deletes the manager, and finally
removes the temporary spill directory. This matches the lifecycle used
by other spill-related test fixtures.

### Release note

None

### Check List (For Author)

- Test
    - [ ] Regression test
    - [x] Unit Test
- `GTEST_REPEAT=10 ./run-be-ut.sh --run
--filter='WorkloadGroupManagerTest.*:DebugPointsTest.AddTest:SpillFileTest.RetryPreservesDirectoryQueuedAfterPendingDrain'
-j 8`
    - [ ] Manual test (add detailed scripts or steps below)
    - [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
        - [ ] Previous test can cover this change.
        - [ ] No code files have been changed.
        - [ ] Other reason

- Behavior changed:
    - [x] No. This only corrects test fixture resource teardown.
    - [ ] Yes.

- Does this need documentation?
    - [x] No.
    - [ ] Yes.

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
yiguolei pushed a commit that referenced this pull request Aug 7, 2026
### What problem does this PR solve?

Issue Number: None

Related PR: #66328

Problem Summary:

`WorkloadGroupManagerTest` creates and initializes a `SpillFileManager`
for every test case. Initialization starts a background spill GC thread,
but the fixture did not stop or delete the manager during teardown. Each
subsequent test overwrote the global manager pointer, leaving the
previous manager and its GC thread alive.

The leaked threads could continue accessing process-wide test utilities
during static destruction and trigger an ASAN heap-use-after-free when
the test binary exited. They could also interfere with tests that use
global spill GC debug points.

This change releases workload-group-owned query resources first, then
stops and joins the spill GC thread, deletes the manager, and finally
removes the temporary spill directory. This matches the lifecycle used
by other spill-related test fixtures.

### Release note

None

### Check List (For Author)

- Test
    - [ ] Regression test
    - [x] Unit Test
- `GTEST_REPEAT=10 ./run-be-ut.sh --run
--filter='WorkloadGroupManagerTest.*:DebugPointsTest.AddTest:SpillFileTest.RetryPreservesDirectoryQueuedAfterPendingDrain'
-j 8`
    - [ ] Manual test (add detailed scripts or steps below)
    - [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
        - [ ] Previous test can cover this change.
        - [ ] No code files have been changed.
        - [ ] Other reason

- Behavior changed:
    - [x] No. This only corrects test fixture resource teardown.
    - [ ] Yes.

- Does this need documentation?
    - [x] No.
    - [ ] Yes.

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
yiguolei pushed a commit that referenced this pull request Aug 7, 2026
…ardown #66534 (#66544)

Cherry-picked from #66534

Co-authored-by: Jerry Hu <hushenggang@selectdb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/4.1.4-merged reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants