kvcache-ai / kvcache-ai/Mooncake
[Bug]: Replication tasks retain targets and reserved quota after source cleanup
- Dominant language
- C++
- Stars
- 6.6k
- Forks
- 1.2k
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 312
Description
## Bug Report
While reviewing #2877, I found a pre-existing replication-task lifecycle gap. This is not caused by making segment cleanup asynchronous, but async cleanup makes the retained state easier to observe.
When an in-flight Copy/Move source becomes unavailable, `CleanupStaleHandles()` removes the invalid COMPLETE source replica. It only cancels promotion tasks for removed replica IDs, so a replication task whose `source_id` was removed is left behind together with its PROCESSING target replicas and reserved tenant quota. Those resources remain until the client calls `CopyEnd/MoveEnd/CopyRevoke/MoveRevoke` or `DiscardExpiredProcessingReplicas()` reaches the task timeout.
### Reproduction
A deterministic unit test can use the existing `MasterServiceTest` fixture:
1. Create a non-HA `MasterService` with a tenant quota large enough for exactly the source and one temporary target.
2. Mount `source_segment` and `target_segment`.
3. `PutStart` + `PutEnd` a key on `source_segment`.
4. Call either:
- `CopyStart(client, key, ..., "source_segment", {"target_segment"})`, or
- `MoveStart(client, key, ..., "source_segment", "target_segment")`.
5. Unmount `source_segment` and wait until invalid-handle cleanup has removed the source replica.
6. Do not call the corresponding End/Revoke API.
7. Inspect the key's `TenantState`, or attempt another allocation under the strict quota.
Observed state after step 5:
- the source replica is absent;
- the target replica is still PROCESSING;
- `tenant_state.replication_tasks` still contains the key;
- `reserved_quota_charge_bytes` is still held;
- physical metadata remains until the replication-task timeout.
A compact assertion sketch:
```cpp
ASSERT_TRUE(service.CopyStart(client, key, tenant, src, {dst}).has_value());
ASSERT_TRUE(service.UnmountSegment(src_id, src_owner).has_value());
WaitUntil([&] { return source_replica_is_gone(); });
EXPECT_TRUE(tenant_state.replication_tasks.contains(key));
EXPECT_TRUE(target_replica_is_processing());
EXPECT_GT(tenant_state.replication_tasks.at(key)
.reserved_quota_charge_bytes,
0);
```
### Expected behavior
Removing a replica that is the source of a Copy/Move task should invalidate that task promptly, discard its PROCESSING targets, release its reserved quota, and balance the source reference if it still exists.
### Relevant code
- `MasterService::CleanupStaleHandles()`
- `MasterService::CancelPromotionTaskForRemovedReplicas()`
- `MasterService::DiscardExpiredProcessingReplicas()`
This should be handled separately from #2877 so that the asynchronous-unmount PR can remain focused on RPC latency and immediate logical invisibility.
Contributor guide
Research direction
Start with MasterService::CleanupStaleHandles(), then trace CancelPromotionTaskForRemovedReplicas() and DiscardExpiredProcessingReplicas() using the existing MasterServiceTest fixture. Reproduce the CopyStart or MoveStart flow under a strict tenant quota and verify cleanup after source unmount. Done means the replication task, PROCESSING targets, reserved quota, and any remaining source reference are handled promptly.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 66/100