[GH-ISSUE #4491] [Bug]: Timed-out put is revoked while its queued slices are still pending; they later overwrite the next object at that address #1142

Open
opened 2026-10-29 21:23:17 +00:00 by bsakaguchi · 4 comments
Owner

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

Bug Report

Summary

When a Store put times out (60 s, hardcoded), the client returns TRANSFER_FAIL (-800) and revokes the object. The master frees the replica immediately, and the next put can get the same address. The timed-out transfer is not cancelled: slices still in the transfer engine queue are posted later and overwrite the new object. Nothing reports an error.

#975 / #993 added discarded_replicas_ for this hazard ("the faulted client ... is still writing data to discarded replicas"). PutRevoke doesn't use it.

Environment

  • mooncake-transfer-engine-cuda13==0.3.13.post1; source checked at e5598b0 (v0.3.13-rc1). Same code on current main.
  • Ubuntu 22.04.5, kernel 6.8.0-1047-nvidia, OFED-internal-25.10-1.2.8, rdma-core 2510.0.10-1.
  • mlx5_4 (InfiniBand), firmware 28.47.2526.

Reproduce

Single host, RDMA (MC_STORE_MEMCPY=0), standalone master with defaults, one 16 MiB segment.

  1. Client A: stop its two transferWorker threads with ptrace (RPC and heartbeat threads keep running), then put 12 MiB of 0x5C.
  2. After 60 s, A's put returns -800 and the object is revoked.
  3. Client B (a separate process): put 12 MiB of 0x33 to a new key, then read it back. It lands at the same address and reads back 0x33.
  4. Resume A's workers. B's key now reads back all 0x5C, and so does the segment memory.

Result: 8 out of 8 trials, same address every time, no crash. An earlier TCP run with a blackholed port, with no threads stopped, showed the same overwrite once the port came back.

Stopping the workers is artificial. It only stands in for anything that keeps slices queued past the timeout (e.g. a paused rail, rdma_rail_pause_seconds = 30, or a blocked worker). We have not reproduced it with a real link failure across two nodes.

Expected

A write from a revoked put should never land in memory that has been handed to another object.

Why

Links are pinned to e5598b0.

  1. The timeout only stops waiting (transfer_task.cpp#L889-L895). Queued slices aren't removed or marked cancelled.
  2. The destructor calls freeBatchID (transfer_task.h#L226), which returns BatchBusy for unfinished tasks (transport.cpp#L42-L50). The result is ignored, so the batch is leaked, and the slices stay valid to post later.
  3. PutRevoke erases the replica right away (master_service.cpp#L4869). Stale put-start cleanup (#L4440-L4450) and upsert preemption (#L4944) move replicas to discarded_replicas_ instead.

Impact

  • The overwritten object belongs to another key whose put succeeded. Reads succeed and return wrong data.
  • For a KV cache, every later prefix hit on that key gets the wrong KV until it is evicted.
  • Slices read their source at post time. If the caller has reused its source buffer after the -800, the bytes that land aren't even the failed key's data.

Questions for maintainers

Before proposing a code change, we'd like your view on:

  1. Have we seen other reports related to this issue in production?
  2. Can it be prevented without code changes? For example, settings that keep the time a slice can spend queued well under 60 s (rail pause, retry count), or object checksum validation to catch the overwrite on read.

Possible fix

If you agree this is a real problem and not just a theoretical one, we'd propose a quarantine: PutRevoke moves the replicas to discarded_replicas_ instead of erasing them, the same way stale put-start cleanup does. Dropping a timed-out batch's unposted slices would make that quarantine time a real upper bound. We can send a PR.

#975 / #993 (discarded_replicas_), #4247 (same problem in the sync transfer API on Ascend), #3030.

Before submitting...

  • Ensure you searched for relevant issues and read the documentation
Originally created by @yubofredwang on GitHub (Oct 7, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4491 ### Bug Report #### Summary When a Store put times out (60 s, hardcoded), the client returns `TRANSFER_FAIL (-800)` and revokes the object. The master frees the replica immediately, and the next put can get the same address. The timed-out transfer is not cancelled: slices still in the transfer engine queue are posted later and overwrite the new object. Nothing reports an error. #975 / #993 added `discarded_replicas_` for this hazard ("the faulted client ... is still writing data to discarded replicas"). `PutRevoke` doesn't use it. #### Environment - `mooncake-transfer-engine-cuda13==0.3.13.post1`; source checked at `e5598b0` (`v0.3.13-rc1`). Same code on current `main`. - Ubuntu 22.04.5, kernel `6.8.0-1047-nvidia`, `OFED-internal-25.10-1.2.8`, rdma-core `2510.0.10-1`. - `mlx5_4` (InfiniBand), firmware `28.47.2526`. #### Reproduce Single host, RDMA (`MC_STORE_MEMCPY=0`), standalone master with defaults, one 16 MiB segment. 1. Client A: stop its two `transferWorker` threads with ptrace (RPC and heartbeat threads keep running), then put 12 MiB of `0x5C`. 2. After 60 s, A's put returns -800 and the object is revoked. 3. Client B (a separate process): put 12 MiB of `0x33` to a new key, then read it back. It lands at the same address and reads back `0x33`. 4. Resume A's workers. B's key now reads back all `0x5C`, and so does the segment memory. Result: 8 out of 8 trials, same address every time, no crash. An earlier TCP run with a blackholed port, with no threads stopped, showed the same overwrite once the port came back. Stopping the workers is artificial. It only stands in for anything that keeps slices queued past the timeout (e.g. a paused rail, `rdma_rail_pause_seconds` = 30, or a blocked worker). We have not reproduced it with a real link failure across two nodes. #### Expected A write from a revoked put should never land in memory that has been handed to another object. #### Why Links are pinned to `e5598b0`. 1. The timeout only stops waiting ([`transfer_task.cpp#L889-L895`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-store/src/transfer_task.cpp#L889-L895)). Queued slices aren't removed or marked cancelled. 2. The destructor calls `freeBatchID` ([`transfer_task.h#L226`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-store/include/transfer_task.h#L226)), which returns `BatchBusy` for unfinished tasks ([`transport.cpp#L42-L50`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-transfer-engine/src/transport/transport.cpp#L42-L50)). The result is ignored, so the batch is leaked, and the slices stay valid to post later. 3. `PutRevoke` erases the replica right away ([`master_service.cpp#L4869`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-store/src/master_service.cpp#L4869)). Stale put-start cleanup ([`#L4440-L4450`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-store/src/master_service.cpp#L4440-L4450)) and upsert preemption ([`#L4944`](https://github.com/kvcache-ai/Mooncake/blob/e5598b0992cc258b06d22f24d875480e8931a28e/mooncake-store/src/master_service.cpp#L4944)) move replicas to `discarded_replicas_` instead. #### Impact - The overwritten object belongs to another key whose put succeeded. Reads succeed and return wrong data. - For a KV cache, every later prefix hit on that key gets the wrong KV until it is evicted. - Slices read their source at post time. If the caller has reused its source buffer after the -800, the bytes that land aren't even the failed key's data. #### Questions for maintainers Before proposing a code change, we'd like your view on: 1. Have we seen other reports related to this issue in production? 2. Can it be prevented without code changes? For example, settings that keep the time a slice can spend queued well under 60 s (rail pause, retry count), or object checksum validation to catch the overwrite on read. #### Possible fix If you agree this is a real problem and not just a theoretical one, we'd propose a quarantine: `PutRevoke` moves the replicas to `discarded_replicas_` instead of erasing them, the same way stale put-start cleanup does. Dropping a timed-out batch's unposted slices would make that quarantine time a real upper bound. We can send a PR. #### Related #975 / #993 (`discarded_replicas_`), #4247 (same problem in the sync transfer API on Ascend), #3030. ### Before submitting... - [x] Ensure you searched for relevant issues and read the documentation
Author
Owner

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

Thanks for opening this issue, @yubofredwang!

Field Value
Issue #4491
GitHub user ID 10526540
Reporter @yubofredwang

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.

<!-- gh-comment-id:6034346399 --> @github-actions[bot] commented on GitHub (Oct 7, 2026): Thanks for opening this issue, @yubofredwang! | Field | Value | |-------|-------| | **Issue** | #4491 | | **GitHub user ID** | `10526540` | | **Reporter** | @yubofredwang | 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.
Author
Owner

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

Confirmed against current main (245e7106). The mechanism reads exactly as you describe, and the drift from e5598b0 is only a few lines:

  1. The timeout only stops waiting. Both completion paths return TRANSFER_FAIL without touching queued tasks: the event-driven wait (transfer_task.cpp:956) and the polling loop (:967). Nothing marks the batch cancelled.
  2. The batch then leaks. ~TransferEngineOperationState (transfer_task.cpp:772) calls freeBatchID, which refuses with BatchBusy while any task is unfinished (transport.cpp:41-51), and the result is ignored. The slices stay valid to post later.
  3. The master frees immediately. PutRevoke erases the processing replicas and releases their buffers in both flavors: oplog (FinalizeRemovedReplicasAfterDurable, master_service.cpp:5642) and non-oplog (EraseReplicasWithCacheTotalAccounting, :5664). The quarantine it should share sits in the same file: stale put-start cleanup (:5216) and upsert preemption (:5963, :6192) both move replicas to discarded_replicas_ with a TTL for exactly this reason, the previous writer may still be writing to those buffers.

The client half of the trigger is client_service.cpp:2101: transfer failure, finalize decision, PutRevoke. An honest client on a slow rail walks into this without doing anything exotic, which matches your blackholed-port TCP run.

On your questions:

  1. Nothing else in the tracker with this exact signature beyond what you linked. #4247 is the same shape on the Ascend sync API, #3030 is adjacent. Not a duplicate.
  2. Not preventable without code changes. MOONCAKE_STORE_CHECKSUM turns the overwrite into a CHECKSUM_MISMATCH on read for the honest consumer: detection, not prevention, and off by default. Rail pause and retry tuning shrink the window but cannot close it, because nothing bounds how long a slice may sit queued. Creating that bound is precisely what the fix has to do.

On direction, the quarantine is the right and minimal shape: PutRevoke moves the processing replicas to discarded_replicas_ instead of erasing, reusing the stale-cleanup TTL config. Two decisions worth making in the PR, not after:

  • The TTL only becomes a real upper bound once the engine drops unposted slices of a timed-out batch. Without that half, quarantine shrinks the corruption window from unbounded to TTL-minus-post-drift, better but not closed. The engine half touches the batch lifecycle (the BatchBusy-ignoring destructor is the leak point), so I would take it as a second PR rather than one big one.
  • The HA flavor needs one explicit comment: the oplog path persists REMOVE and finalizes after durability, while discarded_replicas_ is in-memory only, so a master failover inside the TTL loses the quarantine. The stale-cleanup precedent accepts exactly this gap, so matching it is fine; just say so in a comment so nobody assumes the discard survives promotion.

For coverage I would want a master-level scenario: PutRevoke on an in-flight put keeps the buffers out of the allocator until the discard TTL fires (a second PutStart must not land on the same address), plus a positive control that the address is reusable after expiry.

Please send the PR and I will review it. If you would rather hand it off, say so and I will pick it up.

<!-- gh-comment-id:6037135802 --> @he-yufeng commented on GitHub (Oct 7, 2026): Confirmed against current main (245e7106). The mechanism reads exactly as you describe, and the drift from e5598b0 is only a few lines: 1. The timeout only stops waiting. Both completion paths return `TRANSFER_FAIL` without touching queued tasks: the event-driven wait (transfer_task.cpp:956) and the polling loop (:967). Nothing marks the batch cancelled. 2. The batch then leaks. `~TransferEngineOperationState` (transfer_task.cpp:772) calls `freeBatchID`, which refuses with `BatchBusy` while any task is unfinished (transport.cpp:41-51), and the result is ignored. The slices stay valid to post later. 3. The master frees immediately. `PutRevoke` erases the processing replicas and releases their buffers in both flavors: oplog (`FinalizeRemovedReplicasAfterDurable`, master_service.cpp:5642) and non-oplog (`EraseReplicasWithCacheTotalAccounting`, :5664). The quarantine it should share sits in the same file: stale put-start cleanup (:5216) and upsert preemption (:5963, :6192) both move replicas to `discarded_replicas_` with a TTL for exactly this reason, the previous writer may still be writing to those buffers. The client half of the trigger is `client_service.cpp:2101`: transfer failure, finalize decision, `PutRevoke`. An honest client on a slow rail walks into this without doing anything exotic, which matches your blackholed-port TCP run. On your questions: 1. Nothing else in the tracker with this exact signature beyond what you linked. #4247 is the same shape on the Ascend sync API, #3030 is adjacent. Not a duplicate. 2. Not preventable without code changes. `MOONCAKE_STORE_CHECKSUM` turns the overwrite into a `CHECKSUM_MISMATCH` on read for the honest consumer: detection, not prevention, and off by default. Rail pause and retry tuning shrink the window but cannot close it, because nothing bounds how long a slice may sit queued. Creating that bound is precisely what the fix has to do. On direction, the quarantine is the right and minimal shape: `PutRevoke` moves the processing replicas to `discarded_replicas_` instead of erasing, reusing the stale-cleanup TTL config. Two decisions worth making in the PR, not after: - The TTL only becomes a real upper bound once the engine drops unposted slices of a timed-out batch. Without that half, quarantine shrinks the corruption window from unbounded to TTL-minus-post-drift, better but not closed. The engine half touches the batch lifecycle (the BatchBusy-ignoring destructor is the leak point), so I would take it as a second PR rather than one big one. - The HA flavor needs one explicit comment: the oplog path persists REMOVE and finalizes after durability, while `discarded_replicas_` is in-memory only, so a master failover inside the TTL loses the quarantine. The stale-cleanup precedent accepts exactly this gap, so matching it is fine; just say so in a comment so nobody assumes the discard survives promotion. For coverage I would want a master-level scenario: PutRevoke on an in-flight put keeps the buffers out of the allocator until the discard TTL fires (a second PutStart must not land on the same address), plus a positive control that the address is reusable after expiry. Please send the PR and I will review it. If you would rather hand it off, say so and I will pick it up.
Author
Owner

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

Thanks @yubofredwang and @he-yufeng for the analysis. If nobody has started on the engine half yet (dropping unposted slices of a timed-out batch so the discard TTL becomes a real bound), I'd like to take it as the follow-up PR to the PutRevoke quarantine. Plan: reproduce on the TCP path with a failing test first, then a minimal change, with RDMA limits stated explicitly. Happy to step back if the reporter prefers to do both halves.

Note: #4569 is reworking the BatchDesc / freeBatchID lifecycle in the same code, so I'll build on top of it rather than touch that path in parallel.

<!-- gh-comment-id:6115487723 --> @apex-mochen commented on GitHub (Oct 12, 2026): Thanks @yubofredwang and @he-yufeng for the analysis. If nobody has started on the engine half yet (dropping unposted slices of a timed-out batch so the discard TTL becomes a real bound), I'd like to take it as the follow-up PR to the PutRevoke quarantine. Plan: reproduce on the TCP path with a failing test first, then a minimal change, with RDMA limits stated explicitly. Happy to step back if the reporter prefers to do both halves. Note: #4569 is reworking the BatchDesc / freeBatchID lifecycle in the same code, so I'll build on top of it rather than touch that path in parallel.
Author
Owner

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

Update on the engine half: I have a failing test, and a proposed shape for the fix. I'd like feedback before writing the change.

Reproduction (TCP, CPU only)

TcpWriteVisibilityTest.AbandonedBatchMustNotDeliverUnpostedSlices, added to tcp_write_visibility_test.cpp (+70 lines, test only). It reuses the file's existing lane hooks:

  1. Hold the lane's connect handler. submitTransfer returns, and a 64 KiB WRITE slice sits in group.queue, unposted. This stands in for a peer that has stopped answering.
  2. Call freeBatchID on the batch. This is what ~TransferEngineOperationState does after the 60 s wait gives up. It returns BatchCleanupDeferred.
  3. Write a new pattern into the destination range. This stands in for the next owner after PutRevoke.
  4. Release the lane, then push a probe WRITE through the same single lane (MC_TCP_LANES_PER_PEER=1). Lanes are FIFO, so once the probe completes, the abandoned slice has been either delivered or dropped. No sleeps are needed.
  5. Assert the next owner's bytes are intact.

On main @ 559f83c it fails 20/20 runs. Each time, 65536/65536 bytes are overwritten, and all of them carry the abandoned write's pattern. The other 53 tests in the file still pass, and the case takes about 1 s. It is at the engine level and does not go through PutRevoke. A Store-level end-to-end test can follow if you want one.

Proposed change

  1. Mark, don't add an API. When MultiTransport::freeBatchID defers a busy batch, set an abandoned flag on BatchDesc before handing it to the deferred-cleanup thread. A busy freeBatchID already means "the owner has stopped waiting", so no caller has to change.
  2. Check before posting. Each transport checks the flag at the point where a slice would leave the process. If it is set, the slice is completed through markFailed() without being posted.
    • TCP: where a lane takes work from group.queue (tcp_transport_lane_impl.h:824). Pending admissions reach the wire only through that queue.
    • RDMA: in the worker pool before WRs are built for ibv_post_send.
    • Completion accounting is unchanged, so the deferred-cleanup thread still reclaims the BatchDesc once every slice is terminal.
  3. Boundary. Slices or WRs that have already been posted cannot be recalled. The fix bounds only what is still queued. Together with the PutRevoke quarantine, the discard TTL then has to cover only in-flight bytes, not an unbounded queue. I'll say this in a code comment.
  4. Sequencing with #4569. That PR reworks the same BatchDesc / freeBatchID lifecycle, including the pin and task.batch_id on TCP. I'll build on it after it lands rather than touch that path in parallel.

Tests I plan to add with the fix

  • The reproduction above, which should pass after the fix.
  • Abandon a batch with some slices already posted: posted slices may land, queued ones must not.
  • Abandonment racing with completion, 1000 iterations under TSan.
  • The deferred batch is actually reclaimed (ASan/LSan).
  • The non-timeout path is unchanged: the existing suite plus a tebench TCP before/after comparison.

RDMA: I don't have RDMA hardware. The RDMA check would be covered by compilation and code review only, and I'll mark it as unverified in the PR. If you would rather keep the first PR TCP-only and do RDMA as a follow-up, that works for me.

Questions

  1. Is it right to treat a busy freeBatchID as abandonment? Or does any caller free a busy batch and still expect delivery (fire-and-forget)? If so, I'd add an explicit abandonBatch() / cancel call instead and use it from TransferEngineOperationState only.
  2. TENT already has cancelTransfer. Should the classic path match its semantics, for example a CANCELED status rather than FAILED?
  3. Is TCP + RDMA in one PR acceptable, or TCP first?

The test diff is ready; I can push it as a draft PR if that is easier to look at.

<!-- gh-comment-id:6117373714 --> @apex-mochen commented on GitHub (Oct 12, 2026): Update on the engine half: I have a failing test, and a proposed shape for the fix. I'd like feedback before writing the change. **Reproduction (TCP, CPU only)** `TcpWriteVisibilityTest.AbandonedBatchMustNotDeliverUnpostedSlices`, added to `tcp_write_visibility_test.cpp` (+70 lines, test only). It reuses the file's existing lane hooks: 1. Hold the lane's connect handler. `submitTransfer` returns, and a 64 KiB WRITE slice sits in `group.queue`, unposted. This stands in for a peer that has stopped answering. 2. Call `freeBatchID` on the batch. This is what `~TransferEngineOperationState` does after the 60 s wait gives up. It returns `BatchCleanupDeferred`. 3. Write a new pattern into the destination range. This stands in for the next owner after `PutRevoke`. 4. Release the lane, then push a probe WRITE through the same single lane (`MC_TCP_LANES_PER_PEER=1`). Lanes are FIFO, so once the probe completes, the abandoned slice has been either delivered or dropped. No sleeps are needed. 5. Assert the next owner's bytes are intact. On main @ 559f83c it fails 20/20 runs. Each time, 65536/65536 bytes are overwritten, and all of them carry the abandoned write's pattern. The other 53 tests in the file still pass, and the case takes about 1 s. It is at the engine level and does not go through `PutRevoke`. A Store-level end-to-end test can follow if you want one. **Proposed change** 1. **Mark, don't add an API.** When `MultiTransport::freeBatchID` defers a busy batch, set an `abandoned` flag on `BatchDesc` before handing it to the deferred-cleanup thread. A busy `freeBatchID` already means "the owner has stopped waiting", so no caller has to change. 2. **Check before posting.** Each transport checks the flag at the point where a slice would leave the process. If it is set, the slice is completed through `markFailed()` without being posted. - TCP: where a lane takes work from `group.queue` (`tcp_transport_lane_impl.h:824`). Pending admissions reach the wire only through that queue. - RDMA: in the worker pool before WRs are built for `ibv_post_send`. - Completion accounting is unchanged, so the deferred-cleanup thread still reclaims the `BatchDesc` once every slice is terminal. 3. **Boundary.** Slices or WRs that have already been posted cannot be recalled. The fix bounds only what is still queued. Together with the `PutRevoke` quarantine, the discard TTL then has to cover only in-flight bytes, not an unbounded queue. I'll say this in a code comment. 4. **Sequencing with #4569.** That PR reworks the same `BatchDesc` / `freeBatchID` lifecycle, including the pin and `task.batch_id` on TCP. I'll build on it after it lands rather than touch that path in parallel. **Tests I plan to add with the fix** - The reproduction above, which should pass after the fix. - Abandon a batch with some slices already posted: posted slices may land, queued ones must not. - Abandonment racing with completion, 1000 iterations under TSan. - The deferred batch is actually reclaimed (ASan/LSan). - The non-timeout path is unchanged: the existing suite plus a `tebench` TCP before/after comparison. RDMA: I don't have RDMA hardware. The RDMA check would be covered by compilation and code review only, and I'll mark it as unverified in the PR. If you would rather keep the first PR TCP-only and do RDMA as a follow-up, that works for me. **Questions** 1. Is it right to treat a busy `freeBatchID` as abandonment? Or does any caller free a busy batch and still expect delivery (fire-and-forget)? If so, I'd add an explicit `abandonBatch()` / cancel call instead and use it from `TransferEngineOperationState` only. 2. TENT already has `cancelTransfer`. Should the classic path match its semantics, for example a `CANCELED` status rather than `FAILED`? 3. Is TCP + RDMA in one PR acceptable, or TCP first? The test diff is ready; I can push it as a draft PR if that is easier to look at.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
kvcache-ai/Mooncake#1142
No description provided.