kvcache-ai / kvcache-ai/Mooncake

[Bug]: Replication tasks retain targets and reserved quota after source cleanup

Open
#3,401 1 comment 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.