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 #4830] [Bug]: [Store] offload_on_evict is silently bypassed when OpLog is enabled; evicted objects never reach LOCAL_DISK #1228

New issue

Open

opened 2026-10-29 21:24:12 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-29 21:24:12 +00:00

Owner

Originally created by @he-yufeng on GitHub (Oct 21, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4830

Describe the bug

With enable_ha=true + enable_oplog=true + enable_offload=true + offload_on_evict=true, evicted objects never reach the LOCAL_DISK tier: they are removed from DRAM on the primary (and via the OpLog on the standby) without any disk replica ever being created. The startup log still says offload happens at eviction time.

On current main (3b08b7716, mooncake-store/src/master_service.cpp):

PutEnd queues an eager offload only when offload_on_evict_ is false (L5386), so nothing is offloaded at write time in this mode. That part is intended.
In the BatchEvict path, try_evict_or_offload returns early with a plain evict_replicas when enable_oplog_ is set (L11642-L11650), ahead of the whole offload branch.
Right before that, persist_evict_oplog_or_skip (L11710) has already marked the memory replicas REMOVED and appended the REMOVE record, so the standby drops its copy too.
The startup log (L379-L381) still reports "Offload-on-evict mode enabled: DRAM offload to LOCAL_DISK will occur at eviction time instead of PutEnd".

Net effect for an operator running HA with the disk tier: eviction silently deletes objects that the configuration says should be spilled to disk. The first visible symptom is elevated cache misses; nothing in the log says the disk tier is inactive.

The tenant-quota pass added in #4723 orders its own record correctly (offload decision first, record only on actual eviction), but the global BatchEvict path above is the one that runs on the watermark trigger.

Identification and reproduction

Found by code reading while measuring the HA paths for #4666 (Appendix §F item 3); the same reading was confirmed independently in #4723. We have not run this exact flag combination in the rig yet. A probe that should demonstrate it: master with enable_ha, enable_oplog, enable_offload, offload_on_evict, small segments; fill past the eviction high watermark; then check that evicted keys have no LOCAL_DISK replica (has_local_disk_replica never became true) and that reads of those keys miss instead of restoring from disk.

If the early return is deliberate (for example because the offload ordering cannot be made durable with the current OpLog), the startup log line and the deployment docs should say that the two flags are mutually exclusive; otherwise the fix is to let the offload decision precede the record in this path as well.

Related: #4723, #4666.

Originally created by @he-yufeng on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4830 **Describe the bug** With `enable_ha=true` + `enable_oplog=true` + `enable_offload=true` + `offload_on_evict=true`, evicted objects never reach the LOCAL_DISK tier: they are removed from DRAM on the primary (and via the OpLog on the standby) without any disk replica ever being created. The startup log still says offload happens at eviction time. On current `main` (3b08b7716, `mooncake-store/src/master_service.cpp`): - `PutEnd` queues an eager offload only when `offload_on_evict_` is false (L5386), so nothing is offloaded at write time in this mode. That part is intended. - In the `BatchEvict` path, `try_evict_or_offload` returns early with a plain `evict_replicas` when `enable_oplog_` is set (L11642-L11650), ahead of the whole offload branch. - Right before that, `persist_evict_oplog_or_skip` (L11710) has already marked the memory replicas `REMOVED` and appended the REMOVE record, so the standby drops its copy too. - The startup log (L379-L381) still reports "Offload-on-evict mode enabled: DRAM offload to LOCAL_DISK will occur at eviction time instead of PutEnd". Net effect for an operator running HA with the disk tier: eviction silently deletes objects that the configuration says should be spilled to disk. The first visible symptom is elevated cache misses; nothing in the log says the disk tier is inactive. The tenant-quota pass added in #4723 orders its own record correctly (offload decision first, record only on actual eviction), but the global `BatchEvict` path above is the one that runs on the watermark trigger. **Identification and reproduction** Found by code reading while measuring the HA paths for #4666 (Appendix §F item 3); the same reading was confirmed independently in #4723. We have not run this exact flag combination in the rig yet. A probe that should demonstrate it: master with `enable_ha`, `enable_oplog`, `enable_offload`, `offload_on_evict`, small segments; fill past the eviction high watermark; then check that evicted keys have no LOCAL_DISK replica (`has_local_disk_replica` never became true) and that reads of those keys miss instead of restoring from disk. If the early return is deliberate (for example because the offload ordering cannot be made durable with the current OpLog), the startup log line and the deployment docs should say that the two flags are mutually exclusive; otherwise the fix is to let the offload decision precede the record in this path as well. Related: #4723, #4666.

bsakaguchi added the

bug

label

2026-10-29 21:24:12 +00:00

bsakaguchi commented

2026-10-29 21:24:12 +00:00

Author

Owner

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

Thanks for opening this issue, @he-yufeng!

Field	Value
Issue	#4830
GitHub user ID	`40085740`
Reporter	@he-yufeng

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 21, 2026): Thanks for opening this issue, @he-yufeng! | Field | Value | |-------|-------| | **Issue** | #4830 | | **GitHub user ID** | `40085740` | | **Reporter** | @he-yufeng | 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:24:13 +00:00

Author

Owner

@apex-mochen commented on GitHub (Oct 21, 2026):

I ran the probe as two scenario tests in master_service_evict_scenario_test.cpp. Both use enable_ha + enable_oplog + enable_offload + offload_on_evict, with offload_force_evict=false and one completed, lease-expired 1 KiB object cold, followed by EvictMemory(1.0), which goes through BatchEvict.

// with a local disk (the case in this issue)
auto config = HaConfig(cluster_id);
config.enable_offload = true;
config.offload_on_evict = true;
config.offload_force_evict = false;
MasterScenario scenario("probe batch evict offload with disk", config, backend);
scenario.Given(MemoryNode("memory"))
    .When(MountLocalDisk("memory"))
    .Given(Objects({"cold"}).Size(kObjectSize).By("memory")
               .CompleteOn("memory").ExpiresAt(ExpiredBase()));
scenario.When(EvictMemory(1.0))
    .When(OffloadHeartbeat("memory").ExpectTasks({"cold"}, kObjectSize))
    .Then(Object("cold").IsReadable());

The second test is the same without MountLocalDisk. There the offload cannot be queued, so with offload_force_evict=false the object should stay.

Results on d855a6a (the base of #4723; #4723 does not touch BatchEvict). Each test was run 3 times, with the same result every time:

case	result
with local disk	`OffloadHeartbeat(memory) returned 0 offload tasks; expected 1`, then `Object(cold) is not readable: OBJECT_NOT_FOUND`
without local disk	`Object(cold) is not readable: OBJECT_NOT_FOUND`

The log shows [EVICT-RESULT] evicted_count=1. I got the same "without disk" result earlier on 129c961.

So both effects reproduce:

nothing is queued for disk, which is this issue;
with offload_force_evict=false, the only copy is deleted even when the offload cannot be queued.

For comparison, the tenant-watermark pass in #4723 keeps the object in both cases; two regression tests there cover this.

I haven't checked the grouped-object path. If the early return turns out not to be deliberate, I'm happy to send a PR that applies the same ordering to BatchEvict (offload decision first, record only right before an actual eviction), with these two tests as regressions.

@apex-mochen commented on GitHub (Oct 21, 2026): I ran the probe as two scenario tests in `master_service_evict_scenario_test.cpp`. Both use `enable_ha` + `enable_oplog` + `enable_offload` + `offload_on_evict`, with `offload_force_evict=false` and one completed, lease-expired 1 KiB object `cold`, followed by `EvictMemory(1.0)`, which goes through `BatchEvict`. ```cpp // with a local disk (the case in this issue) auto config = HaConfig(cluster_id); config.enable_offload = true; config.offload_on_evict = true; config.offload_force_evict = false; MasterScenario scenario("probe batch evict offload with disk", config, backend); scenario.Given(MemoryNode("memory")) .When(MountLocalDisk("memory")) .Given(Objects({"cold"}).Size(kObjectSize).By("memory") .CompleteOn("memory").ExpiresAt(ExpiredBase())); scenario.When(EvictMemory(1.0)) .When(OffloadHeartbeat("memory").ExpectTasks({"cold"}, kObjectSize)) .Then(Object("cold").IsReadable()); ``` The second test is the same without `MountLocalDisk`. There the offload cannot be queued, so with `offload_force_evict=false` the object should stay. Results on d855a6a (the base of #4723; #4723 does not touch `BatchEvict`). Each test was run 3 times, with the same result every time: | case | result | |---|---| | with local disk | `OffloadHeartbeat(memory) returned 0 offload tasks; expected 1`, then `Object(cold) is not readable: OBJECT_NOT_FOUND` | | without local disk | `Object(cold) is not readable: OBJECT_NOT_FOUND` | The log shows `[EVICT-RESULT] evicted_count=1`. I got the same "without disk" result earlier on 129c961. So both effects reproduce: - nothing is queued for disk, which is this issue; - with `offload_force_evict=false`, the only copy is deleted even when the offload cannot be queued. For comparison, the tenant-watermark pass in #4723 keeps the object in both cases; two regression tests there cover this. I haven't checked the grouped-object path. If the early return turns out not to be deliberate, I'm happy to send a PR that applies the same ordering to `BatchEvict` (offload decision first, record only right before an actual eviction), with these two tests as regressions.

bsakaguchi commented

2026-10-29 21:24:13 +00:00

Author

Owner

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

Thanks for running the probe — that matches the reading exactly, and it pins the second effect too, which is the worse one: with offload_force_evict=false the delete already happened by the time the offload queue comes up empty, so the object is gone on the one configuration that explicitly asked not to lose it. The eviction-counting half of this family is what #4723 fixed for the watermark pass; BatchEvict taking the same ordering (decide offload first, record only when an eviction actually happens) sounds right to me.

On scope: the two scenario tests you ran are the regression pair I would want, one with the disk mounted and one without. The grouped-object path is worth a look while you are in there — if a group member is evicted while its siblings are mid-write the same ordering question applies, but I have not traced whether group eviction shares this code path. If it turns out to be separate, leaving it for a follow-up is fine; the single-object case is the one the probe proves.

Happy to review when the PR is up.

@he-yufeng commented on GitHub (Oct 21, 2026): Thanks for running the probe — that matches the reading exactly, and it pins the second effect too, which is the worse one: with `offload_force_evict=false` the delete already happened by the time the offload queue comes up empty, so the object is gone on the one configuration that explicitly asked not to lose it. The eviction-counting half of this family is what #4723 fixed for the watermark pass; BatchEvict taking the same ordering (decide offload first, record only when an eviction actually happens) sounds right to me. On scope: the two scenario tests you ran are the regression pair I would want, one with the disk mounted and one without. The grouped-object path is worth a look while you are in there — if a group member is evicted while its siblings are mid-write the same ordering question applies, but I have not traced whether group eviction shares this code path. If it turns out to be separate, leaving it for a follow-up is fine; the single-object case is the one the probe proves. Happy to review when the PR is up.

bsakaguchi commented

2026-10-29 21:24:13 +00:00

Author

Owner

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

Correction to the write-up above: the parenthetical about the tenant-quota pass was wrong, and worth fixing since #4723 is referenced there.

The tenant pass does not order its own record correctly because it writes no record at all. #4722 tracks exactly that gap (tenant-watermark eviction removes replicas and releases quota with no OpLog entry, while BatchEvict persists first), and #4723 is the fix that adds the persistence. What the tenant pass does order correctly in the current tree is its internal offload-versus-evict bookkeeping; the record side is still missing there and is #4723's to land.

This does not change anything in this issue or in #4868: the BatchEvict path this issue is about had its own bypass, which #4868 fixes, independent of the tenant pass.

@he-yufeng commented on GitHub (Oct 22, 2026): Correction to the write-up above: the parenthetical about the tenant-quota pass was wrong, and worth fixing since #4723 is referenced there. The tenant pass does not order its own record correctly because it writes no record at all. #4722 tracks exactly that gap (tenant-watermark eviction removes replicas and releases quota with no OpLog entry, while BatchEvict persists first), and #4723 is the fix that adds the persistence. What the tenant pass does order correctly in the current tree is its internal offload-versus-evict bookkeeping; the record side is still missing there and is #4723's to land. This does not change anything in this issue or in #4868: the BatchEvict path this issue is about had its own bypass, which #4868 fixes, independent of the tenant pass.