kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #4725] [Store] Offload completion can attach a stale LOCAL_DISK replica to a republished key #1203

New issue

Open

opened 2026-10-29 21:23:54 +00:00 by bsakaguchi · 7 comments

bsakaguchi commented

2026-10-29 21:23:54 +00:00

Owner

Originally created by @Aionw on GitHub (Oct 16, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4725

An offload completion is matched to the key, not to the publication it was queued for. The offload task carries tenant, key and size; the completion carries where it wrote. The master's only link between them is state.offloading_task.

EraseMetadata clears that marker, but a store worker that has already drained the task keeps writing its SSD copy. If the key is erased and republished before the completion arrives, the completion finds no marker. handled_existing_object stays false, and the completion falls through to AddReplicaForRetainedClient, which attaches the LOCAL_DISK replica to whatever publication the key holds now. Nothing compares the object's size or content.

A plain Remove is enough to trigger it: the lease is already expired after PutEnd, and Remove does not check for an offload task. (UpsertStart, by contrast, refuses while a worker owns the task.)

Possible fixes:

have the offload task carry a publication identity and the completion echo it back (a wire change), or
have every erase path refuse, or leave a tombstone, while a worker owns the offload task.

Pre-existing on main. Found by @CAICAIIs in review of #4717.

Originally created by @Aionw on GitHub (Oct 16, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4725 An offload completion is matched to the key, not to the publication it was queued for. The offload task carries tenant, key and size; the completion carries where it wrote. The master's only link between them is `state.offloading_task`. `EraseMetadata` clears that marker, but a store worker that has already drained the task keeps writing its SSD copy. If the key is erased and republished before the completion arrives, the completion finds no marker. `handled_existing_object` stays false, and the completion falls through to `AddReplicaForRetainedClient`, which attaches the LOCAL_DISK replica to whatever publication the key holds now. Nothing compares the object's size or content. A plain `Remove` is enough to trigger it: the lease is already expired after `PutEnd`, and `Remove` does not check for an offload task. (`UpsertStart`, by contrast, refuses while a worker owns the task.) Possible fixes: - have the offload task carry a publication identity and the completion echo it back (a wire change), or - have every erase path refuse, or leave a tombstone, while a worker owns the offload task. Pre-existing on main. Found by @CAICAIIs in review of #4717.

bsakaguchi commented

2026-10-29 21:23:55 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Oct 16, 2026):

Thanks for opening this issue, @Aionw!

Field	Value
Issue	#4725
GitHub user ID	`41376987`
Reporter	@Aionw

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Oct 16, 2026): Thanks for opening this issue, @Aionw! | Field | Value | |-------|-------| | **Issue** | #4725 | | **GitHub user ID** | `41376987` | | **Reporter** | @Aionw | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:23:55 +00:00

Author

Owner

@WryGill commented on GitHub (Oct 16, 2026):

Hi, I'd like to work on this issue. This would be my first contribution to Mooncake, and I'd be happy to give it a try. Could you assign it to me? Thanks!

@WryGill commented on GitHub (Oct 16, 2026): Hi, I'd like to work on this issue. This would be my first contribution to Mooncake, and I'd be happy to give it a try. Could you assign it to me? Thanks!

bsakaguchi commented

2026-10-29 21:23:55 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 16, 2026):

Welcome, @WryGill. Happy to have you pick this up, with one honest caveat up front: this lives in the master's offload-completion state machine, where the failure is a race between an erase and a late completion, so the hard part is proving the fix under concurrency rather than writing it.

A good first deliverable that does not commit us to a design: a failing regression test in master_service covering Aionw's sequence (PutEnd, worker drains the offload task, Remove plus republish of the same key, then the late completion arrives and must not attach its LOCAL_DISK replica to the new publication). That pins the bug and gives whichever fix we pick a target.

On the design fork Aionw listed, my lean is the refuse-or-tombstone variant for the immediate fix, since it does not change the task/completion wire shape; carrying a publication identity on the task is the cleaner long-term answer but is a protocol change worth its own discussion. @Aionw you found this in the #4717 review, so your call on which direction to take first carries the most context.

@he-yufeng commented on GitHub (Oct 16, 2026): Welcome, @WryGill. Happy to have you pick this up, with one honest caveat up front: this lives in the master's offload-completion state machine, where the failure is a race between an erase and a late completion, so the hard part is proving the fix under concurrency rather than writing it. A good first deliverable that does not commit us to a design: a failing regression test in `master_service` covering Aionw's sequence (PutEnd, worker drains the offload task, Remove plus republish of the same key, then the late completion arrives and must not attach its LOCAL_DISK replica to the new publication). That pins the bug and gives whichever fix we pick a target. On the design fork Aionw listed, my lean is the refuse-or-tombstone variant for the immediate fix, since it does not change the task/completion wire shape; carrying a publication identity on the task is the cleaner long-term answer but is a protocol change worth its own discussion. @Aionw you found this in the #4717 review, so your call on which direction to take first carries the most context.

bsakaguchi commented

2026-10-29 21:23:55 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 17, 2026):

An update since my earlier note, because the landscape changed: #4687 (fixing #4686) is now in flight and takes the publication-identity route — every offload/promotion task gets an L4 task id, and the master registers a disk replica only from the completion of the exact task the reporting store drained, with at most one task per key. Aionw's sequence here (Remove plus republish before the late completion) is the same race as #4686's cause 1, and under #4687 the late report echoes the old task's id and is rejected rather than attached to the new publication.

So the practical guidance shifts. @WryGill, rather than building a separate refuse-or-tombstone fix that would land in the same state machine at the same time, the most valuable first contribution is still that failing regression test for Aionw's sequence in master_service: it becomes the acceptance check proving whether #4687 closes this issue end to end, and if any gap remains it tells us exactly where. If the test passes on #4687's branch, this issue closes with it; if you would rather wait and pick it up after #4687 merges, that is fine too.

@Aionw your earlier question on direction is answered by events: the wire-change route is already implemented and green on CI, pending a rebase and a second maintainer's review.

@he-yufeng commented on GitHub (Oct 17, 2026): An update since my earlier note, because the landscape changed: #4687 (fixing #4686) is now in flight and takes the publication-identity route — every offload/promotion task gets an L4 task id, and the master registers a disk replica only from the completion of the exact task the reporting store drained, with at most one task per key. Aionw's sequence here (Remove plus republish before the late completion) is the same race as #4686's cause 1, and under #4687 the late report echoes the old task's id and is rejected rather than attached to the new publication. So the practical guidance shifts. @WryGill, rather than building a separate refuse-or-tombstone fix that would land in the same state machine at the same time, the most valuable first contribution is still that failing regression test for Aionw's sequence in `master_service`: it becomes the acceptance check proving whether #4687 closes this issue end to end, and if any gap remains it tells us exactly where. If the test passes on #4687's branch, this issue closes with it; if you would rather wait and pick it up after #4687 merges, that is fine too. @Aionw your earlier question on direction is answered by events: the wire-change route is already implemented and green on CI, pending a rebase and a second maintainer's review.

bsakaguchi commented

2026-10-29 21:23:56 +00:00

Author

Owner

@WryGill commented on GitHub (Oct 17, 2026):

Thanks for the update! I’ll work on the regression test now, covering the Remove → republish → late offload completion sequence in master_service. I’ll verify that it fails on main and check whether it passes on #4687’s branch, then share the results here.

@WryGill commented on GitHub (Oct 17, 2026): Thanks for the update! I’ll work on the regression test now, covering the Remove → republish → late offload completion sequence in `master_service`. I’ll verify that it fails on main and check whether it passes on #4687’s branch, then share the results here.

bsakaguchi commented

2026-10-29 21:23:56 +00:00

Author

Owner

@WryGill commented on GitHub (Oct 19, 2026):

Hi @he-yufeng , I tested the Remove → republish → late offload completion sequence on main (d750cdd) and #4687 (4c2bd97).

All three regression cases—no new offload task, queued task, and already-taken task—reproduce the bug on main and pass on #4687. These results confirm that #4687 fixes the race described in this issue at the tested commit.

Once #4687 is merged, I can submit a follow-up PR adding these regression tests.

@WryGill commented on GitHub (Oct 19, 2026): Hi @he-yufeng , I tested the Remove → republish → late offload completion sequence on main (`d750cdd`) and #4687 (`4c2bd97`). All three regression cases—no new offload task, queued task, and already-taken task—reproduce the bug on main and pass on #4687. These results confirm that #4687 fixes the race described in this issue at the tested commit. Once #4687 is merged, I can submit a follow-up PR adding these regression tests.

bsakaguchi commented

2026-10-29 21:23:56 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 20, 2026):

Thanks for running this down — three cases failing on main and passing on #4687 is exactly the evidence the race is covered. Agreed on the order: once #4687 lands, the regression tests go in as their own PR. When you open it, tag this issue and I'll review.

@he-yufeng commented on GitHub (Oct 20, 2026): Thanks for running this down — three cases failing on main and passing on #4687 is exactly the evidence the race is covered. Agreed on the order: once #4687 lands, the regression tests go in as their own PR. When you open it, tag this issue and I'll review.

bsakaguchi referenced this issue

2026-10-29 21:32:32 +00:00

[PR #1203] [MERGED] feat(Store): Support get local ssd object #2069