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 #2997] [Bug]: PutEnd silently discards PushOffloadingQueue failures — objects never reach the SSD tier, with no log or metric #774

New issue

Closed

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

bsakaguchi commented

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

Owner

Originally created by @ccgibson on GitHub (Jul 20, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2997

Summary

With eager offload (enable_offload=1, offload_on_evict=0), MasterService::PutEnd calls PushOffloadingQueue and acts only on success. There is no else branch, no log, no metric, and no retry:

// mooncake-store/src/master_service.cpp  (main @ fc04cb1, ~L3283)
auto result = PushOffloadingQueue(object_id, replica);
if (result) {
    if (!task_created) {
        replica.inc_refcnt();
        tenant_state.offloading_tasks.emplace(
            object_id.user_key,
            OffloadingTask{replica.id(), std::chrono::system_clock::now()});
        task_created = true;
    }
}
// failure is discarded here

An object whose enqueue fails therefore gets no offload task, is never written to the SSD tier, and dies RAM-only at the next memory eviction. Because it never had a disk replica, that eviction removes its last replica and the object leaves the pool entirely — a cache entry silently lost despite the SSD tier having free space.

Failure modes that can hit this

PushOffloadingQueue (master_service.cpp, ~L5150) can return:

SEGMENT_NOT_FOUND — segment name not present in the local-disk client_by_name map
UNABLE_OFFLOADING — no local disk segment for the resolved client
UNABLE_OFFLOADING — !enable_offloading on that segment
KEYS_ULTRA_LIMIT — offloading_objects.size() >= offloading_queue_limit_
OBJECT_ALREADY_EXISTS — key already present in offloading_objects

None of these are logged inside PushOffloadingQueue either, so the failure is invisible end-to-end.

Why this is inconsistent

The eviction-path callers of the same function already count and log these failures, e.g.:

LOG(WARNING) << "[EVICT] PushOffloadingQueue failed for "
             << offload_push_failed_forced
             << " object(s); force-evicted without disk offload "
                "(offload_force_evict=true).";

Only the PutEnd hot-path caller discards them. There is also no offload-related metric anywhere in master_metric_manager.{h,cpp}.

Impact observed

In a production deployment we found roughly 9% of pool exits had no corresponding disk eviction — i.e. that fraction of objects left the cache without ever reaching SSD — while the SSD tier was well under its eviction watermark and the offload pipeline was healthy (multi-GB/s end-to-end store throughput, no queue backlog, no dropped events). With no log line and no counter anywhere, there was nothing to attribute it to; localising it required reading the source rather than any telemetry.

The measurement that surfaced it, for anyone hitting the same thing: compare cumulative pool exits against cumulative disk evictions. A persistent excess of the former means objects are leaving the cache without a disk replica.

Note this is distinct from #2632 / #2658 (offloading-task orphans causing 600s expiry warnings). That one produces a visible Offloading task expired warning. This path produces nothing at all.

Two additional silent-success paths

PushOffloadingQueue also returns success ({}) while enqueuing nothing:

if (segment_names.empty()) { return {}; } — early return before the loop
a segment_names list whose entries are all nullopt falls through the loop body

In both cases PutEnd sees success and records an OffloadingTask + inc_refcnt() for work that was never queued, so the task can only be cleared by the TTL expiry path.

Suggested fix

At minimum, make the failure observable: add an else incrementing an offload-enqueue-failure counter and emitting a (sampled, since this is the PutEnd hot path) WARNING carrying the ErrorCode, mirroring what the eviction-path callers already do. That is enough to tell operators which failure mode dominates.

Beyond that, KEYS_ULTRA_LIMIT and OBJECT_ALREADY_EXISTS arguably warrant retry or dedup handling rather than a dropped object, but making it visible seems like the necessary first step.

Happy to open a PR for the observability part if that is a welcome direction.

Environment: observed on v0.3.12-pre1; code path verified unchanged on main @ fc04cb1.

Originally created by @ccgibson on GitHub (Jul 20, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2997 ## Summary With eager offload (`enable_offload=1`, `offload_on_evict=0`), `MasterService::PutEnd` calls `PushOffloadingQueue` and acts **only on success**. There is no `else` branch, no log, no metric, and no retry: ```cpp // mooncake-store/src/master_service.cpp (main @ fc04cb1, ~L3283) auto result = PushOffloadingQueue(object_id, replica); if (result) { if (!task_created) { replica.inc_refcnt(); tenant_state.offloading_tasks.emplace( object_id.user_key, OffloadingTask{replica.id(), std::chrono::system_clock::now()}); task_created = true; } } // failure is discarded here ``` An object whose enqueue fails therefore gets **no offload task**, is never written to the SSD tier, and dies RAM-only at the next memory eviction. Because it never had a disk replica, that eviction removes its last replica and the object leaves the pool entirely — a cache entry silently lost despite the SSD tier having free space. ## Failure modes that can hit this `PushOffloadingQueue` (`master_service.cpp`, ~L5150) can return: - `SEGMENT_NOT_FOUND` — segment name not present in the local-disk `client_by_name` map - `UNABLE_OFFLOADING` — no local disk segment for the resolved client - `UNABLE_OFFLOADING` — `!enable_offloading` on that segment - `KEYS_ULTRA_LIMIT` — `offloading_objects.size() >= offloading_queue_limit_` - `OBJECT_ALREADY_EXISTS` — key already present in `offloading_objects` None of these are logged inside `PushOffloadingQueue` either, so the failure is invisible end-to-end. ## Why this is inconsistent The **eviction-path** callers of the same function already count and log these failures, e.g.: ```cpp LOG(WARNING) << "[EVICT] PushOffloadingQueue failed for " << offload_push_failed_forced << " object(s); force-evicted without disk offload " "(offload_force_evict=true)."; ``` Only the `PutEnd` hot-path caller discards them. There is also no offload-related metric anywhere in `master_metric_manager.{h,cpp}`. ## Impact observed In a production deployment we found roughly **9% of pool exits had no corresponding disk eviction** — i.e. that fraction of objects left the cache without ever reaching SSD — while the SSD tier was well under its eviction watermark and the offload pipeline was healthy (multi-GB/s end-to-end store throughput, no queue backlog, no dropped events). With no log line and no counter anywhere, there was nothing to attribute it to; localising it required reading the source rather than any telemetry. The measurement that surfaced it, for anyone hitting the same thing: compare cumulative pool exits against cumulative disk evictions. A persistent excess of the former means objects are leaving the cache without a disk replica. Note this is distinct from #2632 / #2658 (offloading-task orphans causing 600s expiry warnings). That one produces a *visible* `Offloading task expired` warning. This path produces nothing at all. ## Two additional silent-success paths `PushOffloadingQueue` also returns success (`{}`) while enqueuing nothing: 1. `if (segment_names.empty()) { return {}; }` — early return before the loop 2. a `segment_names` list whose entries are all `nullopt` falls through the loop body In both cases `PutEnd` sees success and records an `OffloadingTask` + `inc_refcnt()` for work that was never queued, so the task can only be cleared by the TTL expiry path. ## Suggested fix At minimum, make the failure observable: add an `else` incrementing an offload-enqueue-failure counter and emitting a (sampled, since this is the `PutEnd` hot path) `WARNING` carrying the `ErrorCode`, mirroring what the eviction-path callers already do. That is enough to tell operators which failure mode dominates. Beyond that, `KEYS_ULTRA_LIMIT` and `OBJECT_ALREADY_EXISTS` arguably warrant retry or dedup handling rather than a dropped object, but making it visible seems like the necessary first step. Happy to open a PR for the observability part if that is a welcome direction. Environment: observed on `v0.3.12-pre1`; code path verified unchanged on `main` @ `fc04cb1`.

bsakaguchi

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

closed this issue
added the
stale

auto-closed
labels

bsakaguchi commented

2026-10-29 21:19:22 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Jul 20, 2026):

Thanks for opening this issue, @ccgibson!

Field	Value
Issue	#2997
GitHub user ID	`13460465`
Reporter	@ccgibson

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 (Jul 20, 2026): Thanks for opening this issue, @ccgibson! | Field | Value | |-------|-------| | **Issue** | #2997 | | **GitHub user ID** | `13460465` | | **Reporter** | @ccgibson | 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:19:22 +00:00

Author

Owner

@bestdo77 commented on GitHub (Jul 21, 2026):

Confirmed on main: PutEnd returns success even when PushOffloadingQueue fails (no local disk segment → SEGMENT_NOT_FOUND; full queue → KEYS_ULTRA_LIMIT), with nothing logged or counted — the object silently loses its only chance to reach the SSD tier.

I have a patch for the observability part ready: an offload_enqueue_failed counter in MasterMetricManager plus a sampled warning (LOG_EVERY_N(WARNING, 100)) in the PutEnd else branch, mirroring what the eviction-path callers already do, with gtest coverage for both failure modes (all master suites pass, 179/179).

@ccgibson you mentioned you'd be happy to PR this as well — since I already have it working, I'll go ahead and open the PR in a bit; very happy to take your review on it.

@bestdo77 commented on GitHub (Jul 21, 2026): Confirmed on main: `PutEnd` returns success even when `PushOffloadingQueue` fails (no local disk segment → `SEGMENT_NOT_FOUND`; full queue → `KEYS_ULTRA_LIMIT`), with nothing logged or counted — the object silently loses its only chance to reach the SSD tier. I have a patch for the observability part ready: an `offload_enqueue_failed` counter in `MasterMetricManager` plus a sampled warning (`LOG_EVERY_N(WARNING, 100)`) in the `PutEnd` else branch, mirroring what the eviction-path callers already do, with gtest coverage for both failure modes (all master suites pass, 179/179). @ccgibson you mentioned you'd be happy to PR this as well — since I already have it working, I'll go ahead and open the PR in a bit; very happy to take your review on it.

bsakaguchi commented

2026-10-29 21:19:22 +00:00

Author

Owner

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

This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

@github-actions[bot] commented on GitHub (Oct 20, 2026): This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

bsakaguchi commented

2026-10-29 21:19:22 +00:00

Author

Owner

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

Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

@github-actions[bot] commented on GitHub (Oct 27, 2026): Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

bsakaguchi referenced this issue

2026-10-29 21:30:33 +00:00

[PR #774] [MERGED] [Doc] Allocator Performance #1813