Skip to content

Commit e264346

Browse files
committed
[fix](be) Hand off query spill cleanup to background GC
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Query teardown previously deleted the complete spill directory tree synchronously. A large residual spill tree could block a scheduler worker, and a filesystem failure was only logged without retry. In addition, recursive CTE rerunnable parameters retained QueryContext after a successful final close. Empty query directories are now removed with a bounded rmdir; non-empty trees are atomically handed off to the spill GC directory, with failed handoffs retried. Rerunnable parameters are released after FINAL_CLOSE and during query-manager shutdown outside the protecting mutex, allowing QueryContext and its spill directories to be reclaimed. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-be-ut.sh --run --filter='SpillFileTest.QueryContextDeletesEmptySpillDirectoryWithoutHandoff:SpillFileTest.QueryContextCleansUpNestedSpillDirectory:SpillFileTest.QueryContextCleansUpResidualSpillDirectory:SpillFileTest.QueryContextContinuesCleanupAfterRootFailure:SpillFileTest.FinalCloseReleasesRerunnableQueryContextAndHandsOffSpillDirectory:FragmentMgrRerunnableParamsTest.StopReleasesLastQueryContextRefOutsideLock'` - Behavior changed: Yes. Query spill cleanup is handed off to background GC and retried after transient failures; recursive CTE final-close state no longer retains QueryContext. - Does this need documentation: No
1 parent 1cd8d29 commit e264346

10 files changed

Lines changed: 607 additions & 20 deletions

be/src/exec/spill/spill_file.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ void SpillFile::gc() {
5757
_spill_dir, status.to_string());
5858
}
5959
}
60-
// decrease spill data usage anyway, since in ~QueryContext() spill data of the query will be
61-
// clean up as a last resort
60+
// Decrease spill data usage even if per-file cleanup failed. QueryContext teardown hands the
61+
// whole query spill directory to the spill GC path as the final cleanup step.
6262
_data_dir->update_spill_data_usage(-_total_written_bytes);
6363
_total_written_bytes = 0;
6464
}

be/src/exec/spill/spill_file_manager.cpp

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,27 +24,46 @@
2424
#include <filesystem>
2525
#include <memory>
2626
#include <string>
27+
#include <utility>
2728

2829
#include "common/logging.h"
2930
#include "common/metrics/doris_metrics.h"
3031
#include "exec/spill/spill_file.h"
3132
#include "io/fs/file_system.h"
3233
#include "io/fs/local_file_system.h"
3334
#include "storage/olap_define.h"
35+
#include "util/debug_points.h"
3436
#include "util/parse_util.h"
3537
#include "util/pretty_printer.h"
3638
#include "util/time.h"
39+
#include "util/uid_util.h"
3740

3841
namespace doris {
3942

4043
SpillFileManager::~SpillFileManager() {
44+
// QueryContext destruction can still hand off directories after stop(), for example while
45+
// VDataStreamMgr is being destroyed. Retry the bounded metadata operation once more before
46+
// dropping the in-memory retry state. Any source path that still cannot be moved remains under
47+
// the active spill root and will be picked up by init() after restart.
48+
_retry_pending_query_spill_directories();
4149
DorisMetrics::instance()->metric_registry()->deregister_entity(_entity);
4250
}
4351

4452
SpillFileManager::SpillFileManager(
4553
std::unordered_map<std::string, std::unique_ptr<SpillDataDir>>&& spill_store_map)
4654
: _spill_store_map(std::move(spill_store_map)), _stop_background_threads_latch(1) {}
4755

56+
void SpillFileManager::stop() {
57+
_stop_background_threads_latch.count_down();
58+
if (_spill_gc_thread) {
59+
_spill_gc_thread->join();
60+
}
61+
// The GC thread may observe the stop latch before processing a recently queued handoff. Keep
62+
// shutdown bounded by retrying only rmdir/rename here; recursive deletion remains asynchronous
63+
// or is recovered by init() on the next start.
64+
_retry_pending_query_spill_directories();
65+
}
66+
4867
Status SpillFileManager::init() {
4968
LOG(INFO) << "init spill stream manager";
5069
RETURN_IF_ERROR(_init_spill_store_map());
@@ -162,6 +181,71 @@ void SpillFileManager::delete_spill_file(SpillFileSPtr spill_file) {
162181
spill_file->gc();
163182
}
164183

184+
void SpillFileManager::handoff_query_spill_directory(const std::string& query_id,
185+
SpillDataDir* data_dir) {
186+
PendingQuerySpillDirectory pending_directory {
187+
.query_dir = data_dir->get_spill_data_path(query_id),
188+
.gc_dir = data_dir->get_spill_data_gc_path(
189+
fmt::format("{}-{}", query_id, UniqueId::gen_uid().to_string())),
190+
};
191+
192+
auto status = Status::OK();
193+
DBUG_EXECUTE_IF("fault_inject::spill_file_manager::handoff_query_spill_directory", {
194+
status = Status::Error<INTERNAL_ERROR>("injected query spill directory handoff failure");
195+
});
196+
if (status.ok()) {
197+
status = _try_handoff_query_spill_directory(pending_directory);
198+
}
199+
if (!status.ok()) {
200+
{
201+
std::lock_guard lock(_pending_query_spill_directories_mutex);
202+
_pending_query_spill_directories.emplace_back(std::move(pending_directory));
203+
}
204+
LOG(WARNING) << fmt::format(
205+
"failed to hand off spill query directory, dir {}, error: {}; "
206+
"the spill GC will retry",
207+
data_dir->get_spill_data_path(query_id), status.to_string());
208+
}
209+
}
210+
211+
Status SpillFileManager::_try_handoff_query_spill_directory(
212+
const PendingQuerySpillDirectory& pending_directory) {
213+
const auto& fs = io::global_local_filesystem();
214+
auto status = fs->delete_empty_directory(pending_directory.query_dir);
215+
if (status.ok()) {
216+
return Status::OK();
217+
}
218+
if (!status.is<ErrorCode::DIRECTORY_NOT_EMPTY>()) {
219+
return status;
220+
}
221+
return fs->rename(pending_directory.query_dir, pending_directory.gc_dir);
222+
}
223+
224+
void SpillFileManager::_retry_pending_query_spill_directories() {
225+
std::vector<PendingQuerySpillDirectory> pending_directories;
226+
{
227+
std::lock_guard lock(_pending_query_spill_directories_mutex);
228+
pending_directories.swap(_pending_query_spill_directories);
229+
}
230+
231+
std::vector<PendingQuerySpillDirectory> failed_directories;
232+
for (auto& pending_directory : pending_directories) {
233+
auto status = _try_handoff_query_spill_directory(pending_directory);
234+
if (!status.ok()) {
235+
LOG_EVERY_T(WARNING, 10) << fmt::format(
236+
"failed to retry spill query directory handoff, dir {}, error: {}",
237+
pending_directory.query_dir, status.to_string());
238+
failed_directories.emplace_back(std::move(pending_directory));
239+
}
240+
}
241+
if (!failed_directories.empty()) {
242+
std::lock_guard lock(_pending_query_spill_directories_mutex);
243+
for (auto& pending_directory : failed_directories) {
244+
_pending_query_spill_directories.emplace_back(std::move(pending_directory));
245+
}
246+
}
247+
}
248+
165249
void SpillFileManager::gc(int32_t max_work_time_ms) {
166250
bool exists = true;
167251
bool has_work = false;
@@ -181,6 +265,7 @@ void SpillFileManager::gc(int32_t max_work_time_ms) {
181265
LOG(INFO) << msg;
182266
}
183267
}};
268+
_retry_pending_query_spill_directories();
184269
for (const auto& [path, store_dir] : _spill_store_map) {
185270
std::string gc_root_dir = store_dir->get_spill_data_gc_path();
186271

be/src/exec/spill/spill_file_manager.h

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@
2020
#include <atomic>
2121
#include <memory>
2222
#include <mutex>
23+
#include <string>
2324
#include <unordered_map>
2425
#include <vector>
2526

2627
#include "common/metrics/metrics.h"
28+
#include "common/status.h"
2729
#include "exec/spill/spill_file.h"
2830
#include "storage/options.h"
2931
#include "util/threadpool.h"
@@ -118,12 +120,7 @@ class SpillFileManager {
118120

119121
Status init();
120122

121-
void stop() {
122-
_stop_background_threads_latch.count_down();
123-
if (_spill_gc_thread) {
124-
_spill_gc_thread->join();
125-
}
126-
}
123+
void stop();
127124

128125
// Create SpillFile and register it
129126
// @param relative_path Operator-formatted path under the spill root,
@@ -133,26 +130,41 @@ class SpillFileManager {
133130
/// Get a unique ID for constructing spill file paths.
134131
uint64_t next_id() { return id_++; }
135132

136-
// Mark SpillFile for deletion; asynchronously delete spill files in the GC thread
133+
// Delete SpillFile data synchronously.
137134
void delete_spill_file(SpillFileSPtr spill_file);
138135

136+
// Remove an empty per-query directory directly. A non-empty tree is atomically handed off to
137+
// the spill GC directory so query teardown never recursively walks the tree. Failed handoffs
138+
// are retained by the manager and retried by the spill GC thread.
139+
void handoff_query_spill_directory(const std::string& query_id, SpillDataDir* data_dir);
140+
139141
void gc(int32_t max_work_time_ms);
140142

141143
void update_spill_write_bytes(int64_t bytes) { _spill_write_bytes_counter->increment(bytes); }
142144

143145
void update_spill_read_bytes(int64_t bytes) { _spill_read_bytes_counter->increment(bytes); }
144146

145147
private:
148+
struct PendingQuerySpillDirectory {
149+
std::string query_dir;
150+
std::string gc_dir;
151+
};
152+
146153
void _init_metrics();
147154
Status _init_spill_store_map();
148155
void _spill_gc_thread_callback();
156+
Status _try_handoff_query_spill_directory(const PendingQuerySpillDirectory& pending_directory);
157+
void _retry_pending_query_spill_directories();
149158
std::vector<SpillDataDir*> _get_stores_for_spill(TStorageMedium::type storage_medium);
150159

151160
std::unordered_map<std::string, std::unique_ptr<SpillDataDir>> _spill_store_map;
152161

153162
CountDownLatch _stop_background_threads_latch;
154163
std::shared_ptr<Thread> _spill_gc_thread;
155164

165+
std::mutex _pending_query_spill_directories_mutex;
166+
std::vector<PendingQuerySpillDirectory> _pending_query_spill_directories;
167+
156168
std::atomic_uint64_t id_ = 0;
157169

158170
std::shared_ptr<MetricEntity> _entity {nullptr};

be/src/exec/spill/spill_file_writer.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,9 @@ Status SpillFileWriter::write_block(RuntimeState* state, const Block& block) {
149149

150150
// Lazily open the first part
151151
if (!_file_writer) {
152+
if (_current_part_index == 0) {
153+
state->get_query_ctx()->record_spill_data_dir(_data_dir);
154+
}
152155
RETURN_IF_ERROR(_open_next_part());
153156
}
154157

be/src/runtime/fragment_mgr.cpp

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,10 +353,14 @@ void FragmentMgr::stop() {
353353
// destructred and remove it from _query_ctx_map_delay_delete which is destructring. it's UB.
354354
_query_ctx_map_delay_delete.clear();
355355
_pipeline_map.clear();
356+
decltype(_rerunnable_params_map) rerunnable_params_map;
356357
{
357358
std::lock_guard<std::mutex> lk(_rerunnable_params_lock);
358-
_rerunnable_params_map.clear();
359+
rerunnable_params_map.swap(_rerunnable_params_map);
359360
}
361+
// RerunableFragmentInfo holds QueryContext. Destroy it after releasing the lock because
362+
// QueryContext::~QueryContext() can re-enter remove_query_context().
363+
rerunnable_params_map.clear();
360364
}
361365

362366
static void empty_function(RuntimeState*, Status*) {}
@@ -431,16 +435,21 @@ void FragmentMgr::remove_pipeline_context(std::pair<TUniqueId, int> key) {
431435
void FragmentMgr::remove_query_context(const TUniqueId& key) {
432436
// Clean up any saved rerunnable params for this query to avoid memory leaks.
433437
// This covers both cancel and normal destruction paths.
438+
decltype(_rerunnable_params_map) rerunnable_params_map;
434439
{
435440
std::lock_guard<std::mutex> lk(_rerunnable_params_lock);
436441
for (auto it = _rerunnable_params_map.begin(); it != _rerunnable_params_map.end();) {
437442
if (it->first.first == key) {
438-
it = _rerunnable_params_map.erase(it);
443+
auto current = it++;
444+
rerunnable_params_map.insert(_rerunnable_params_map.extract(current));
439445
} else {
440446
++it;
441447
}
442448
}
443449
}
450+
// Releasing query_ctx can re-enter this method, so destroy the detached entries outside
451+
// _rerunnable_params_lock.
452+
rerunnable_params_map.clear();
444453
_query_ctx_map_delay_delete.erase(key);
445454
#ifndef BE_TEST
446455
_query_ctx_map.erase(key);
@@ -1252,7 +1261,7 @@ Status FragmentMgr::transmit_rec_cte_block(
12521261
// wait_for_destroy: collect deregister RF IDs, store brpc closure, trigger old PFC close
12531262
// rebuild: increment stage, deregister old RFs, create+prepare new PFC from saved params
12541263
// submit: submit the new PFC's pipeline tasks for execution
1255-
// final_close: async wait for close, send final report, clean up (last round only)
1264+
// final_close: async wait for close, send final report, clean up saved params (last round only)
12561265
//
12571266
// The brpc ClosureGuard is stored in the PFC so the RPC response is deferred until
12581267
// the PFC is fully destroyed. This gives the caller (RecCTESourceOperatorX) a
@@ -1293,6 +1302,16 @@ Status FragmentMgr::rerun_fragment(const std::shared_ptr<brpc::ClosureGuard>& gu
12931302
SCOPED_ATTACH_TASK(query_ctx);
12941303
RETURN_IF_ERROR(
12951304
fragment_ctx->listen_wait_close(guard, stage == PRerunFragmentParams::FINAL_CLOSE));
1305+
1306+
if (stage == PRerunFragmentParams::FINAL_CLOSE) {
1307+
// extract() transfers ownership without destroying RerunableFragmentInfo under the
1308+
// lock. In particular, releasing its QueryContext may re-enter remove_query_context().
1309+
decltype(_rerunnable_params_map)::node_type final_close_info;
1310+
{
1311+
std::lock_guard<std::mutex> lk(_rerunnable_params_lock);
1312+
final_close_info = _rerunnable_params_map.extract({query_id, fragment_id});
1313+
}
1314+
}
12961315
fragment_ctx->notify_close();
12971316
return Status::OK();
12981317
} else if (stage == PRerunFragmentParams::REBUILD) {

be/src/runtime/fragment_mgr.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,10 @@ class FragmentMgr : public RestMonitorIface {
228228

229229
// Saved params and callback for rerunnable (recursive CTE) fragments.
230230
// Only populated when need_notify_close == true during exec_plan_fragment.
231-
// Lifecycle: created in exec_plan_fragment(), used in rerun_fragment(rebuild)
232-
// to recreate PFC with fresh state, cleaned up in remove_query_context().
231+
// Lifecycle: created in exec_plan_fragment(), retained across wait/rebuild/submit rounds,
232+
// and removed after a successful final_close. remove_query_context() and stop() provide
233+
// fallback cleanup. Entries must be destroyed without holding _rerunnable_params_lock because
234+
// releasing query_ctx can re-enter FragmentMgr::remove_query_context().
233235
struct RerunableFragmentInfo {
234236
// Runtime filter IDs registered by the old PFC, collected during wait_for_destroy.
235237
// These are deregistered from the RuntimeFilterMgr before the new PFC is created.

be/src/runtime/query_context.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,11 @@ void QueryContext::init_query_task_controller() {
219219
#endif
220220
}
221221

222+
void QueryContext::record_spill_data_dir(SpillDataDir* data_dir) {
223+
std::lock_guard lock(_spill_data_dirs_mutex);
224+
_spill_data_dirs.emplace(data_dir);
225+
}
226+
222227
QueryContext::~QueryContext() {
223228
SCOPED_SWITCH_THREAD_MEM_TRACKER_LIMITER(query_mem_tracker());
224229
// query mem tracker consumption is equal to 0, it means that after QueryContext is created,
@@ -247,6 +252,12 @@ QueryContext::~QueryContext() {
247252
obj_pool.clear();
248253
_merge_controller_handler.reset();
249254

255+
if (auto* spill_file_mgr = _exec_env->spill_file_mgr()) {
256+
for (auto* data_dir : _spill_data_dirs) {
257+
spill_file_mgr->handoff_query_spill_directory(print_id(_query_id), data_dir);
258+
}
259+
}
260+
250261
DorisMetrics::instance()->query_ctx_cnt->increment(-1);
251262
// fragment_mgr is nullptr in unittest
252263
if (ExecEnv::GetInstance()->fragment_mgr()) {

be/src/runtime/query_context.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include <mutex>
2929
#include <string>
3030
#include <unordered_map>
31+
#include <unordered_set>
3132

3233
#include "common/config.h"
3334
#include "common/factory_creator.h"
@@ -55,6 +56,7 @@ class PipelineTask;
5556
class QueryTaskController;
5657
class Dependency;
5758
class RecCTEScanLocalState;
59+
class SpillDataDir;
5860

5961
struct ReportStatusRequest {
6062
const Status status;
@@ -203,6 +205,10 @@ class QueryContext : public std::enable_shared_from_this<QueryContext> {
203205

204206
TUniqueId query_id() const { return _query_id; }
205207

208+
// Record a spill data directory before opening the first spill part so teardown only visits
209+
// touched roots.
210+
void record_spill_data_dir(SpillDataDir* data_dir);
211+
206212
// Expose task-level query progress counters for runtime statistics reporting.
207213
void add_total_task_num(int delta);
208214
void inc_finished_task_num();
@@ -329,6 +335,9 @@ class QueryContext : public std::enable_shared_from_this<QueryContext> {
329335
MonotonicStopWatch _query_watcher;
330336
bool _is_nereids = false;
331337

338+
std::mutex _spill_data_dirs_mutex;
339+
std::unordered_set<SpillDataDir*> _spill_data_dirs;
340+
332341
std::shared_ptr<ResourceContext> _resource_ctx;
333342

334343
void _init_resource_context();

be/test/runtime/fragment_mgr_cross_cluster_cancel_test.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,4 +183,41 @@ TEST(FragmentMgrDelayDeleteMapTest, ClearShouldNotAbortWhenReleasingLastQueryCon
183183
exec_env->_fragment_mgr = previous_fragment_mgr;
184184
}
185185

186+
TEST(FragmentMgrRerunnableParamsTest, StopReleasesLastQueryContextRefOutsideLock) {
187+
auto* exec_env = ExecEnv::GetInstance();
188+
auto* previous_fragment_mgr = exec_env->_fragment_mgr;
189+
auto* fragment_mgr = new FragmentMgr(exec_env);
190+
exec_env->_fragment_mgr = fragment_mgr;
191+
192+
TUniqueId query_id;
193+
query_id.__set_hi(303);
194+
query_id.__set_lo(404);
195+
196+
TQueryOptions query_options;
197+
query_options.__set_query_type(TQueryType::SELECT);
198+
query_options.__set_execution_timeout(60);
199+
query_options.__set_mem_limit(64L * 1024 * 1024);
200+
201+
TNetworkAddress fe_addr;
202+
fe_addr.hostname = "127.0.0.1";
203+
fe_addr.port = 9030;
204+
205+
auto query_ctx =
206+
QueryContext::create(query_id, exec_env, query_options, fe_addr,
207+
/*is_nereids*/ true, fe_addr, QuerySource::INTERNAL_FRONTEND);
208+
std::weak_ptr<QueryContext> weak_query_ctx = query_ctx;
209+
{
210+
std::lock_guard<std::mutex> lock(fragment_mgr->_rerunnable_params_lock);
211+
fragment_mgr->_rerunnable_params_map[{query_id, 1}].query_ctx = query_ctx;
212+
}
213+
query_ctx.reset();
214+
215+
EXPECT_FALSE(weak_query_ctx.expired());
216+
fragment_mgr->stop();
217+
EXPECT_TRUE(weak_query_ctx.expired());
218+
219+
exec_env->_fragment_mgr = previous_fragment_mgr;
220+
delete fragment_mgr;
221+
}
222+
186223
} // namespace doris

0 commit comments

Comments
 (0)