[Store] Decouple HTTP metadata cleanup from MasterService#2957
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the HTTP metadata cleanup logic out of MasterService into a dedicated HttpMetadataCleanupWorker class, introducing a callback-based event system (ClientLeaseExpiredEvent) to decouple resource reclamation from metadata cleanup. The reviewer feedback focuses on improving thread safety and lifetime management: specifically, suggesting the use of std::shared_ptr and std::weak_ptr for the cleanup worker to prevent potential use-after-free issues in callbacks, and introducing a separate lifecycle_mutex_ in HttpMetadataCleanupWorker to avoid potential deadlocks during Start() and Stop() operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Declared before master_service_ so it is destroyed afterwards. This | ||
| // keeps the callback target alive until MasterService has stopped and | ||
| // joined its client monitor thread. | ||
| std::unique_ptr<HttpMetadataCleanupWorker> http_metadata_cleanup_worker_; |
There was a problem hiding this comment.
Relying on member declaration order for thread safety and lifetime management is fragile and prone to future regressions if the members are reordered. Using std::shared_ptr for the worker and capturing a std::weak_ptr in the callback lambda is a much more robust and idiomatic C++ pattern.
| // Declared before master_service_ so it is destroyed afterwards. This | |
| // keeps the callback target alive until MasterService has stopped and | |
| // joined its client monitor thread. | |
| std::unique_ptr<HttpMetadataCleanupWorker> http_metadata_cleanup_worker_; | |
| std::shared_ptr<HttpMetadataCleanupWorker> http_metadata_cleanup_worker_; |
| if (http_metadata_server || !http_metadata_remote_url.empty()) { | ||
| http_metadata_cleanup_worker_ = | ||
| std::make_unique<HttpMetadataCleanupWorker>( | ||
| http_metadata_server, http_metadata_remote_url); | ||
| if (http_metadata_cleanup_worker_->Start()) { | ||
| master_service_.RegisterClientLeaseExpiredCallback( | ||
| [worker = http_metadata_cleanup_worker_.get()]( | ||
| const ClientLeaseExpiredEvent& event) { | ||
| worker->Enqueue(event); | ||
| }); | ||
| } else { | ||
| http_metadata_cleanup_worker_.reset(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of capturing a raw pointer (worker = http_metadata_cleanup_worker_.get()) in the callback lambda, capture a std::weak_ptr to prevent potential use-after-free issues if the worker is destroyed before the callback is executed.
if (http_metadata_server || !http_metadata_remote_url.empty()) {
http_metadata_cleanup_worker_ =
std::make_shared<HttpMetadataCleanupWorker>(
http_metadata_server, http_metadata_remote_url);
if (http_metadata_cleanup_worker_->Start()) {
master_service_.RegisterClientLeaseExpiredCallback(
[worker_weak = std::weak_ptr<HttpMetadataCleanupWorker>(http_metadata_cleanup_worker_)](
const ClientLeaseExpiredEvent& event) {
if (auto worker = worker_weak.lock()) {
worker->Enqueue(event);
}
});
} else {
http_metadata_cleanup_worker_.reset();
}
}| std::mutex mutex_; | ||
| std::condition_variable cv_; | ||
| std::vector<ClientLeaseExpiredEvent> queue_; |
There was a problem hiding this comment.
Add a separate lifecycle_mutex_ to protect lifecycle operations (Start() and Stop()) from concurrent races. Using mutex_ for this is unsafe because holding it during thread_.join() can cause a deadlock if the worker thread is waiting on the condition variable.
std::mutex mutex_;
std::mutex lifecycle_mutex_;
std::condition_variable cv_;
std::vector<ClientLeaseExpiredEvent> queue_;| bool HttpMetadataCleanupWorker::Start() { | ||
| if (!remove_key_) { | ||
| return false; | ||
| } | ||
|
|
||
| bool expected = false; | ||
| if (!running_.compare_exchange_strong(expected, true)) { | ||
| return true; | ||
| } | ||
| try { | ||
| thread_ = std::thread(&HttpMetadataCleanupWorker::ThreadFunc, this); | ||
| } catch (...) { | ||
| running_ = false; | ||
| throw; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| void HttpMetadataCleanupWorker::Stop() { | ||
| { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| if (!running_.exchange(false)) { | ||
| return; | ||
| } | ||
| queue_.clear(); | ||
| } | ||
| cv_.notify_all(); | ||
| if (thread_.joinable()) { | ||
| thread_.join(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement thread-safe Start() and Stop() using the newly introduced lifecycle_mutex_ to prevent concurrency races on thread_ and avoid deadlocks.
bool HttpMetadataCleanupWorker::Start() {
if (!remove_key_) {
return false;
}
std::lock_guard<std::mutex> lock(lifecycle_mutex_);
if (running_) {
return true;
}
running_ = true;
try {
thread_ = std::thread(&HttpMetadataCleanupWorker::ThreadFunc, this);
} catch (...) {
running_ = false;
throw;
}
return true;
}
void HttpMetadataCleanupWorker::Stop() {
std::lock_guard<std::mutex> lock(lifecycle_mutex_);
if (!running_) {
return;
}
{
std::lock_guard<std::mutex> queue_lock(mutex_);
running_ = false;
queue_.clear();
}
cv_.notify_all();
if (thread_.joinable()) {
thread_.join();
}
}
github-actions
Bot
added
documentation
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
he-yufeng
left a comment
he-yufeng
left a comment
There was a problem hiding this comment.
Reviewed the decoupling end to end. The direction is good and the tricky parts are handled well.
The HttpMetadataCleanupWorker concurrency is solid: Stop() flips running_ and clears the queue under mutex_, notifies, and joins, while ThreadFunc swaps the queue into a local batch and does the (slow) HTTP removals outside the lock; an in-flight batch finishes and unstarted work is dropped, matching the documented contract. Enqueue no-ops once running_ is false, so a late lease-expiry callback after Stop() is safe, and the destructor's Stop() is idempotent. Publishing is done the right way too: expirations are buffered into expiration_events under the master locks and the callbacks fire afterward, and Enqueue only takes the worker's own mutex_, so there's no lock-ordering hazard even if that ever changed.
I also chased the teardown lifetime, since the callback captures a raw worker pointer: it's correct today because http_metadata_cleanup_worker_ is declared before master_service_, so ~MasterService runs first and joins client_monitor_thread_ (the thread that fires NotifyClientLeaseExpired) before the worker is destroyed. Good that this is called out in a comment.
One thing worth hardening (agreeing with the bot, but concretely): that safety rests entirely on the declaration order plus the raw-pointer capture, and a future member reorder or a second callback-firing path would reintroduce a use-after-free silently. Two robust options: (a) give WrappedMasterService a destructor body that explicitly stops master_service_'s client monitor (or clears the lease-expired callbacks) before the members tear down, so ordering no longer matters; or (b) hold the worker as a shared_ptr and capture a weak_ptr in the callback, so lock() keeps it alive for the duration of the call or bails if it's gone. Non-blocking for this PR given the current order is correct and documented, but I'd take one of these as a fast follow-up.
Separately, note the async removal of rpc_meta/<segment> was already async before this PR (this just moves it out of MasterService), so no regression here; if a restarted client can re-register the same segment name inside the cleanup window, that reuse race predates this change and is a separate item.
LGTM.
|
Thanks for the refactor. Moving HTTP metadata configuration, key construction, queueing, and worker lifecycle out of
Could you please confirm whether I am missing an ordering or ownership guarantee that rules out either case? |
4 participants
Description
Decouple stale HTTP metadata cleanup from
MasterService.MasterServicenow publishes a typedClientLeaseExpiredEventafter the expired client's resources have been reclaimed and all internal locks have been released. A standaloneHttpMetadataCleanupWorker, owned byWrappedMasterService, subscribes to that event and asynchronously removes the client'sram/andrpc_meta/entries for both co-located and remote HTTP metadata servers.This keeps HTTP backend configuration, key construction, queueing, and worker lifecycle outside the core master service while preserving the existing best-effort behavior, including no retry or shutdown-drain guarantee.
Overlap notes:
main; it does not duplicate [Store] Revive HTTP metadata cleanup from #1219 #2256's HTTP metadata server APIs.Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
Test results:
master_service_test: passed in 327.68 seconds.USE_HTTP=ONandUSE_HTTP=OFF.git diff --checkpass.sphinx-buildis unavailable in the environment.Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure
OpenAI Codex helped design and implement the event boundary and cleanup worker extraction, add tests and documentation, run validation, and draft this PR description. The human submitter must review every changed line and be able to defend the change end-to-end before marking this PR ready for review.