[fix](be) Clean up empty spill query directories

[mrhhsg]

What problem does this PR solve?

Issue Number: None

Problem Summary:

Spill file cleanup removes each operator-level directory but leaves its empty per-query parent under the active spill root. Long-running backends can therefore accumulate one directory inode for every query that writes spill data.

This change performs cleanup at the query-lifetime boundary:

• the first write to a spill file records its selected SpillDataDir in the owning QueryContext;
• QueryContext asks SpillFileManager to clean only the roots actually touched by that query, after all query execution resources have been released;
• queries that never spill do not traverse configured roots or issue filesystem cleanup calls;
• empty directories are pruned bottom-up with atomic, non-recursive rmdir semantics, so nested spill paths are handled;
• residual files and all non-empty directories are preserved;
• failures are rate-limited and logged while cleanup continues on the other touched spill data directories.

Release note

Remove empty per-query spill directories when a query finishes.

Check List (For Author)

• Test:
  • Unit Test: ./run-be-ut.sh --run --filter='SpillFileTest.GCCleansUpFiles:SpillFileTest.QueryContextCleansUpEmptySpillDirectory:SpillFileTest.QueryContextCleansUpNestedSpillDirectory:SpillFileTest.QueryContextPreservesNonEmptySpillDirectory:SpillFileTest.QueryContextSkipsCleanupWithoutSpill'
• Behavior changed: Yes. Empty temporary query directory trees under spill roots touched by the query are removed when the query finishes.
• Does this need documentation: No

[hello-stephen]

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]

/review

[github-actions]

Review result: changes requested.

Two issues need to be addressed before merge: nested spill suffixes can still leave the query directory behind, and the new tests do not exercise the parent-rmdir/recursive-mkdir race that motivates the mutex.

Critical checkpoint conclusions:

• Goal and proof: the flat query-id/file cleanup works, preserves siblings, and recreates a removed parent, but the goal is incomplete for nested suffixes accepted by the documented API. The tests prove the sequential flat cases, not the concurrency guarantee.
• Scope and clarity: the six-file change is focused and reuses the local filesystem abstraction; the remaining contract mismatch is localized to cleanup ownership for nested paths.
• Concurrency and locking: pipeline spill tasks can lazily create different files while another file under the same query is cleaned up. The per-SpillDataDir mutex correctly serializes recursive child mkdir with parent rmdir for the ordinary two-level layout, has no nested lock order, and exposes no substantiated deadlock. A deterministic concurrent test is still required.
• Lifecycle and ownership: SpillFile destructor/manual GC, writer close, weak/shared ownership, repeated cleanup, sibling lifetime, and the mkdir-to-create-file gap were traced. No reachable same-file writer/GC overlap or cross-TU/static-lifetime issue was found.
• Configuration: no configuration item is added or changed.
• Compatibility: no function symbol, storage format, RPC, FE-BE protocol, or rolling-upgrade compatibility change is involved.
• Parallel paths: all production create_spill_file call sites were checked and currently use flat query-prefixed paths. SpillRepartitioner also accepts nested label prefixes, which is the concrete uncovered path reported inline.
• Special conditions and errors: the query-id prefix invariant is enforced with DORIS_CHECK; ENOENT is idempotent success; ENOTEMPTY is the expected sibling case; other parent-removal failures are logged. Suppressing ENOTEMPTY also masks the nested empty-ancestor case reported inline.
• Test coverage: the new BE-UTs cover flat cleanup, sibling preservation, repeated GC, root preservation, and sequential lazy recreation. They do not cover nested cleanup or deterministically overlap mkdir/rmdir.
• Test results: the PR reports targeted SpillFileTest BE-UTs. Per the review-runner instruction, this was a static-only review and no build or test command was run here.
• Observability: existing warning logs and spill-root metrics are otherwise adequate; nested leftover directories can keep the root-nonempty metric asserted.
• Transactions and persistence: no EditLog, database transaction, visible-version, delete-bitmap, or persisted metadata path is modified.
• Data writes and crash behavior: only temporary spill filesystem directories are affected; non-recursive parent removal protects live siblings, and no new data-visibility or atomic commit path is involved.
• FE-BE variables: no transmitted variable or FE-BE propagation path is added.
• Performance: the new mutex serializes first-directory creation and empty-parent removal per spill data directory; no material hot-path regression was substantiated beyond this bounded filesystem operation.
• Other issues: no additional correctness, memory tracking, status propagation, configuration, compatibility, or performance defect survived verification.

User focus: no additional review focus was provided; the complete PR was reviewed.

Review completion: complete after two rounds. The normal concurrency/lifecycle review, normal API/error/test review, and separate risk-focused review all converged on this frozen two-comment set with NO_NEW_VALUABLE_FINDINGS.

[github-actions]

[P2] Handle nested spill paths when pruning the query directory

The new contract only requires the relative path to start with a query-id component, and SpillRepartitioner::create_output_spill_files already accepts prefixes containing / (for example, RouteBlockConservesTotalRowCount writes test/route_block). That creates <root>/<query-id>/test/route_block_sub.... This GC removes only the leaf and then immediately calls rmdir(<root>/<query-id>), so the empty test directory makes the call return DIRECTORY_NOT_EMPTY and the query directory is still leaked. Please either reject nested suffixes at creation or prune empty ancestors up to _spill_query_dir under the same synchronization, and cover this layout in the GC test.

[github-actions]

[P2] Exercise the mkdir/rmdir race deterministically

This test recreates the query directory only after first_spill_file->gc() has completed, so it still passes if _spill_dir_mutex is removed. The mutex is the key part of the fix—the unsafe interleaving is parent rmdir racing recursive child mkdir—but neither added test overlaps those operations. Please add a deterministic concurrent BE-UT (for example with sync points/barriers around the two SpillDataDir methods) that forces the interleaving and verifies the writer's part survives and the final empty query directory is removed.

[mrhhsg]

/review

[mrhhsg]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Review step was cancelled (possibly timeout or cancelled)
Workflow run: https://github.com/apache/doris/actions/runs/30622501588

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

[github-actions]

Review result: no new blocking issues found on the current head.

The revised design addresses the two earlier-head concerns: recursive bottom-up cleanup now handles nested spill paths, and cleanup has moved from per-SpillFile parent removal to the last QueryContext lifetime boundary after spill-capable task state is finalized. The former per-directory mutex/racing-creator design is no longer present.

Critical checkpoint conclusions:

• Goal and proof: the change removes empty per-query directory trees from every configured spill root at query teardown while preserving residual files and the spill root itself. The added BE-UTs cover flat cleanup, nested empty ancestors, and non-empty preservation. Production teardown ordering was also traced through scheduler close/finalize, pipeline-fragment destruction, runtime-state/local-state release, and the final QueryContext reference.
• Scope and clarity: the four-file change is focused: one bottom-up helper, one manager API, one query-lifetime call site, and targeted tests. It reuses LocalFileSystem rather than introducing a parallel filesystem path.
• Concurrency and locking: no new lock is required at the chosen lifetime boundary. TaskScheduler::close_task closes and finalizes tasks before fragment removal; task finalization drops operator/shared spill state, and PipelineFragmentContext clears task runtime states and fragment shared state before resetting its strong query-context reference. Atomic POSIX rmdir prevents removal of any directory that gains a live entry. No new lock-order or deadlock risk was found.
• Lifecycle and ownership: spill files, writers, readers, repartitioners, and revocation wrappers remain owned through task/local/shared state and are released before QueryContext cleanup. ExecEnv stops workload execution and FragmentMgr while SpillFileManager is still allocated, and releases _vstream_mgr before deleting the spill manager. No cross-TU static initialization or ownership-cycle issue is introduced.
• Configuration: no configuration item is added or changed.
• Compatibility: no RPC, FE-BE protocol, function symbol, storage format, persisted metadata, or rolling-upgrade behavior is changed.
• Parallel paths: all reviewed production spill creators use query-id-prefixed paths; nested labels are handled recursively, and manager cleanup iterates every immutable spill-store entry so queries that selected different disks are covered.
• Special conditions and errors: ENOENT is idempotent success; ENOTEMPTY drives the recursive walk; regular/residual files are preserved; unexpected child errors are retained while sibling cleanup continues; and failures include the directory and status in a rate-limited warning.
• Test coverage: the tests prove the destructor hook plus flat, nested, root-preservation, and residual-file behavior. They use a synthetic single-root fixture rather than a production pipeline ownership relationship, but the production ownership fence was established from the call chain. The prior deterministic mkdir/rmdir test request applied to the removed per-file cleanup/mutex design and was not duplicated.
• Test results: the PR reports targeted SpillFileTest BE-UTs. This review runner was explicitly static-only, so no build or test command was run here. At review preparation time, formatting and other completed checks were green, while the live macOS BE-UT check was still pending; no generated result file is changed by this PR.
• Observability: the new failure path has a rate-limited warning with the affected directory and error. No additional metric was warranted for this best-effort temporary-directory cleanup.
• Transactions and persistence: no EditLog, database transaction, visible-version, delete-bitmap, or persisted metadata path is involved.
• Data writes and crash behavior: only temporary spill directories are affected. Bottom-up non-recursive rmdir preserves non-empty data at every level; no user-data atomicity or visibility path changes.
• FE-BE variables: no transmitted variable or scattered serialization path is added.
• Performance and memory: the normal path adds one rmdir attempt per configured spill root at query destruction; recursive listing occurs only when a directory tree remains. Query teardown already attaches the query memory tracker, and no material hot-path regression or untracked long-lived allocation was found.
• Other issues: no additional correctness, error-propagation, memory-safety, lifecycle, compatibility, observability, or performance defect survived verification.

User focus: no additional review focus was provided; the complete PR was reviewed.

Review completion: complete after one convergence round. Two normal full-coverage reviewers and one separate risk-focused reviewer all returned NO_NEW_VALUABLE_FINDINGS against the same frozen ledger and live duplicate set. Final inline comment set: zero.

[mrhhsg]

/review

[github-actions]

Review result: approval opinion; no new blocking issues found on the current head.

The current design fixes the two concerns raised on the earlier head. Empty nested ancestors are pruned bottom-up at the final QueryContext lifetime boundary, and first-part recursive mkdir plus file creation is serialized with each atomic empty-directory removal. The new sync-point test forces that overlap and proves the writer's part survives; the companion teardown tests cover final empty-query-directory removal. The existing inline threads were treated as already-known context and were not duplicated.

Critical checkpoint conclusions:

• Goal and proof: the change removes empty per-query spill directory trees from every configured spill root without deleting residual files or the spill root. The BE-UT additions cover flat cleanup, nested ancestors, non-empty preservation, first-write/cleanup ordering, and root preservation.
• Scope and clarity: the five-file change is focused on one manager cleanup API, one query-lifetime hook, one first-part filesystem fence, and targeted unit tests. It reuses the local filesystem abstraction.
• Concurrency and locking: first-part recursive mkdir and successful open(O_CREAT) hold the same per-SpillDataDir mutex used by each rmdir. Listings, block serialization, data writes, and later-part creation stay outside the mutex. Part 0 fences later parts, no second lock is nested, and no lock-order or deadlock path was found.
• Lifecycle and ownership: PipelineFragmentContext clears task runtime states, pipelines, sink/root operators, and shared spill state before releasing its strong QueryContext. Delayed query-context ownership only extends that lifetime. FragmentMgr teardown occurs while SpillFileManager is still alive, and no ownership cycle, dangling data-dir use, or cross-TU static-initialization issue was found.
• Special conditions and errors: each deletion is non-recursive atomic rmdir; ENOENT is idempotent success; ENOTEMPTY triggers the bottom-up walk; regular and residual files are preserved; failed first opens retry at part index zero; and unexpected cleanup failures are rate-limited with directory and status context.
• Parallel paths: all production spill creators reviewed use the query ID as the first path component and reach the same SpillFileWriter first-part path. Nested repartitioner prefixes are handled, and cleanup visits every immutable spill-store entry.
• Memory safety and observability: no new long-lived allocation, cache handle, ownership cycle, or untracked buffer is introduced. Query teardown already uses the query memory tracker, and the new warning is sufficient for best-effort temporary-directory cleanup.
• Configuration and compatibility: no configuration, RPC, FE-BE variable, function symbol, storage format, persisted metadata, or rolling-upgrade behavior changes.
• Transactions, persistence, and data correctness: no EditLog, database transaction, visible-version, delete-bitmap, or user-data path is involved. Only temporary spill directories are affected, and non-empty directory entries prevent removal.
• Performance: the healthy path performs one rmdir attempt per configured spill root. Directory enumeration is outside the mutex and occurs only for a residual tree; no material hot-path regression was substantiated.
• Test coverage and results: the PR reports the targeted SpillFileTest BE-UT command. This runner was explicitly static-only, so no build or test was run here. At the final sweep, formatter and other completed checks were green while the live macOS BE-UT check was still in progress.
• Other issues: no additional correctness, lifecycle, error-propagation, memory-accounting, compatibility, configuration, observability, performance, or coverage defect survived verification.

User focus: no additional review focus was provided; the complete PR was reviewed.

Review completion: complete after one convergence round. Two normal full-coverage reviewers and one separate risk-focused reviewer all returned NO_NEW_VALUABLE_FINDINGS against the same frozen ledger, head, and live duplicate set. Final inline comment set: zero.

[mrhhsg]

/review

[github-actions]

Review result: changes requested.

One performance issue remains: the new query-lifetime cleanup performs synchronous spill-root filesystem work for every query, including queries that never spill. The inline comment requests an actual-spill/touched-store guard.

Critical checkpoint conclusions:

• Goal and proof: the bottom-up helper removes empty flat and nested per-query spill directories while preserving regular files, non-empty subtrees, special entries, and the spill root. The added BE-UTs cover flat cleanup, nested empty ancestors, and residual-file preservation. The cleanup goal is functionally met, but the all-query scope is broader than necessary.
• Scope and clarity: the four-file change is focused and reuses LocalFileSystem. The remaining issue is localized to the unconditional destructor hook rather than the cleanup algorithm itself.
• Concurrency and locking: scheduler execution retains a strong PipelineFragmentContext; task finalization and PipelineFragmentContext::_release_resource() clear operator/shared spill state before releasing the strong query reference. Revokable tasks re-lock the fragment, multicast spilling is synchronous, and the Iceberg async writer holds its task execution context through close. Therefore same-query lazy creation cannot overlap final query cleanup, and no new lock or deadlock risk was substantiated.
• Lifecycle and ownership: sort, aggregation, join, repartition, multicast, reader/writer, stream-receiver, and async-writer ownership chains were traced. Spill-capable state is released before the last QueryContext; ExecEnv also stops fragment/stream owners before deleting SpillFileManager. No ownership cycle, dangling store pointer, cache-handle leak, or cross-TU static initialization issue was found.
• Configuration: no configuration item is added or changed. However, an empty spill_storage_root_path inherits storage_root_path, so the accepted performance issue scales with the configured disk count.
• Compatibility: no RPC, FE-BE protocol, function symbol, storage format, persisted metadata, or rolling-upgrade behavior changes.
• Parallel paths: every production spill creator reviewed uses the query ID as the first path component and reaches the same manager/writer path. Nested repartitioner labels are handled by the recursive cleanup, and all immutable spill-store entries are visited.
• Special conditions and errors: missing paths are idempotent success; DIRECTORY_NOT_EMPTY triggers the recursive walk; atomic rmdir preserves newly non-empty directories; unexpected child errors are retained while sibling/root processing continues; and warnings include the path and status. The earlier inline concerns about leaf-only nested cleanup and the removed per-file mutex design were treated as already-known context and not duplicated.
• Test coverage: the tests establish the synchronous helper behavior for flat, nested, and residual-file cases. They use a one-root synthetic context rather than a production pipeline ownership chain, but that ownership fence was established from production code. A no-spill fast-path test should accompany the performance fix.
• Test results: the PR reports targeted SpillFileTest BE-UTs. This review runner was explicitly static-only, so no build or test command was run here. At payload freeze time, formatter and other completed checks were green while the live macOS BE-UT job was still pending.
• Observability: the rate-limited warning provides the affected directory and error. No additional metric was required for best-effort temporary-directory cleanup.
• Transactions and persistence: no EditLog, database transaction, visible-version, delete-bitmap, or persisted metadata path is involved.
• Data writes and crash behavior: only temporary spill files/directories are affected. Non-recursive rmdir cannot remove a directory with live entries, and no user-data atomicity or visibility path changes.
• FE-BE variables: no transmitted variable or scattered serialization path is added.
• Performance and memory: MMF-1 is the sole accepted issue. Each BE-side QueryContext currently performs one nominal absent-path rmdir per unique spill root even when no spill object was created; bthread callers offload to shared AsyncIO but still wait synchronously. A query-owned participation bit set before lazy directory creation is the smallest safe fast path; a synchronized touched-store registry can optimize spilling queries further. Recursive allocations are otherwise short-lived under the query tracker, and no additional hot-path or accounting defect was found.
• Other issues: no additional correctness, lifecycle, error-propagation, memory-safety, compatibility, configuration, observability, or coverage defect survived verification.

User focus: no additional review focus was provided; the complete PR was reviewed.

Review completion: complete after two convergence rounds. Two normal full-coverage reviewers and one separate risk-focused reviewer all returned NO_NEW_VALUABLE_FINDINGS against the same current head, live duplicate set, and frozen one-comment set.

[github-actions]

[P2] Skip spill-root cleanup for queries that never spill

This destructor runs for every query, but delete_empty_query_spill_directories() walks every configured spill store and begins each walk with a synchronous rmdir, even when this query never created a spill file. Because an empty spill_storage_root_path inherits all storage_root_path entries, the common no-spill path now pays one redundant filesystem lookup per disk at query teardown; at query-serving QPS this scales as O(disks), and a slow root can hold up the thread releasing the query. Please record spill participation/the touched SpillDataDir values when a spill file is actually created (or provide an equivalent guard) and skip this work for no-spill queries.

[mrhhsg]

/review

[github-actions]

Review result: approval opinion; no new blocking issues found on the current head.

The current design addresses all three earlier-head concerns without duplicating their inline threads: cleanup now runs at the final QueryContext lifetime boundary, prunes nested empty ancestors bottom-up, and visits only SpillDataDir roots recorded by an actual first-part write attempt. Queries that never spill do no teardown filesystem work.

Critical checkpoint conclusions:

• Goal and proof: the change removes empty per-query spill directory trees while preserving residual files, non-empty directories, and the spill root. The added BE-UTs cover flat cleanup, nested empty ancestors, residual-file preservation, and the no-spill fast path.
• Scope and clarity: the six-file change is focused on one cleanup helper/API, one first-write participation hook, one query-lifetime call site, and targeted tests. It reuses LocalFileSystem and the existing spill manager/store selection.
• Concurrency and locking: concurrent first writers only insert into a mutex-protected query-owned set. Revocation locks the query and retains strong PipelineFragmentContext owners; the async result writer locks its PFC task context through writer close. Final cleanup therefore starts only after spill-capable tasks, operators, writers, readers, and shared state have been released. No nested lock order or deadlock path was found.
• Lifecycle and ownership: PipelineFragmentContext::_release_resource() clears task/operator/shared spill ownership before its strong query reference is reset. Fragment/stream owners are drained before SpillFileManager is deleted during shutdown, and the manager's immutable unique_ptr<SpillDataDir> map keeps every recorded raw pointer valid. No ownership cycle, cache-handle leak, or cross-TU static-initialization issue is introduced.
• Configuration: no configuration item is added or changed.
• Compatibility: no RPC, FE-BE protocol, function symbol, storage format, persisted metadata, or rolling-upgrade behavior changes.
• Parallel paths: every production spill creator reviewed (sort, partitioned aggregation/hash join, repartition, multicast, and Iceberg) reaches the same SpillFileWriter first-part path with the matching query RuntimeState. First-open retries are idempotent, rotations reuse the recorded root, and multiple selected roots are recorded separately.
• Special conditions and errors: missing paths are idempotent success; DIRECTORY_NOT_EMPTY triggers the bottom-up walk; regular residual files are skipped; symlinks/special entries are preserved after non-directory errors; the first concrete child error is retained; and every final removal is atomic non-recursive rmdir, so repopulated directories survive.
• Test coverage: the new tests establish the destructor hook and the claimed flat, nested, residual-file, root-preservation, and no-spill behavior. The earlier deterministic mkdir/rmdir test request applied to a removed per-file cleanup/mutex design; the current query-lifetime ownership fence was established from production call chains.
• Test results: the PR reports the targeted SpillFileTest BE-UT command. This runner is explicitly static-review-only, so no build or test command was run here. At payload freeze time, formatter and other completed checks were green while the live macOS BE-UT check was still pending.
• Observability: unexpected cleanup failures are rate-limited and include the affected directory and status. No additional metric is warranted for this best-effort temporary-directory cleanup.
• Transactions and persistence: no EditLog, database transaction, visible-version, delete-bitmap, or persisted metadata path is involved.
• Data writes and crash behavior: only temporary spill files/directories are affected. Non-recursive rmdir cannot remove live non-empty content, and no user-data atomicity or visibility path changes.
• FE-BE variables: no transmitted variable or scattered serialization path is added.
• Performance and memory: non-spilling queries avoid all cleanup filesystem calls. Spilling queries add one mutex-protected idempotent set insertion attempt per spill file's first part and one nominal rmdir per distinct touched root at final teardown; recursive listing occurs only for a residual tree. Temporary traversal allocations remain under the query memory tracker, and no material hot-path or accounting defect was found.
• Other issues: no additional correctness, lifecycle, error-propagation, memory-safety, configuration, compatibility, observability, performance, or coverage defect survived verification.

User focus: no additional review focus was provided; the complete PR was reviewed.

Review completion: complete after one convergence round. Two normal full-coverage reviewers and one separate risk-focused reviewer all returned NO_NEW_VALUABLE_FINDINGS against head 8a49fcbe2e7a4cc1f8c2f878aa4b084a80f69290, the same ledger, and the live duplicate set. Final inline comment set: zero.