mirror of
https://github.com/kvcache-ai/Mooncake.git
synced 2026-11-05 01:17:55 +00:00
[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
Labels
No labels
ascend
auto-closed
bug
bug
CI/Build
Common
documentation
enhancement
feature request
good first issue
help wanted
high priority
MetaX
nightly-failure
pull-request
PyTorch Backend
question
RFC
Roadmap
stale
Store
Tests
Transfer Engine
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
kvcache-ai/Mooncake#1142
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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").PutRevokedoesn't use it.Environment
mooncake-transfer-engine-cuda13==0.3.13.post1; source checked ate5598b0(v0.3.13-rc1). Same code on currentmain.6.8.0-1047-nvidia,OFED-internal-25.10-1.2.8, rdma-core2510.0.10-1.mlx5_4(InfiniBand), firmware28.47.2526.Reproduce
Single host, RDMA (
MC_STORE_MEMCPY=0), standalone master with defaults, one 16 MiB segment.transferWorkerthreads with ptrace (RPC and heartbeat threads keep running), then put 12 MiB of0x5C.0x33to a new key, then read it back. It lands at the same address and reads back0x33.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.transfer_task.cpp#L889-L895). Queued slices aren't removed or marked cancelled.freeBatchID(transfer_task.h#L226), which returnsBatchBusyfor unfinished tasks (transport.cpp#L42-L50). The result is ignored, so the batch is leaked, and the slices stay valid to post later.PutRevokeerases the replica right away (master_service.cpp#L4869). Stale put-start cleanup (#L4440-L4450) and upsert preemption (#L4944) move replicas todiscarded_replicas_instead.Impact
Questions for maintainers
Before proposing a code change, we'd like your view on:
Possible fix
If you agree this is a real problem and not just a theoretical one, we'd propose a quarantine:
PutRevokemoves the replicas todiscarded_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...
@github-actions[bot] commented on GitHub (Oct 7, 2026):
Thanks for opening this issue, @yubofredwang!
10526540A maintainer will triage this when possible. To help us respond faster, please include:
Useful links: Documentation · Contributing guide
@he-yufeng commented on GitHub (Oct 7, 2026):
Confirmed against current main (
245e7106). The mechanism reads exactly as you describe, and the drift frome5598b0is only a few lines:TRANSFER_FAILwithout touching queued tasks: the event-driven wait (transfer_task.cpp:956) and the polling loop (:967). Nothing marks the batch cancelled.~TransferEngineOperationState(transfer_task.cpp:772) callsfreeBatchID, which refuses withBatchBusywhile any task is unfinished (transport.cpp:41-51), and the result is ignored. The slices stay valid to post later.PutRevokeerases 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 todiscarded_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:
MOONCAKE_STORE_CHECKSUMturns the overwrite into aCHECKSUM_MISMATCHon 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:
PutRevokemoves the processing replicas todiscarded_replicas_instead of erasing, reusing the stale-cleanup TTL config. Two decisions worth making in the PR, not after: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.
@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.
@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 totcp_write_visibility_test.cpp(+70 lines, test only). It reuses the file's existing lane hooks:submitTransferreturns, and a 64 KiB WRITE slice sits ingroup.queue, unposted. This stands in for a peer that has stopped answering.freeBatchIDon the batch. This is what~TransferEngineOperationStatedoes after the 60 s wait gives up. It returnsBatchCleanupDeferred.PutRevoke.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.On main @
559f83cit 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 throughPutRevoke. A Store-level end-to-end test can follow if you want one.Proposed change
MultiTransport::freeBatchIDdefers a busy batch, set anabandonedflag onBatchDescbefore handing it to the deferred-cleanup thread. A busyfreeBatchIDalready means "the owner has stopped waiting", so no caller has to change.markFailed()without being posted.group.queue(tcp_transport_lane_impl.h:824). Pending admissions reach the wire only through that queue.ibv_post_send.BatchDesconce every slice is terminal.PutRevokequarantine, the discard TTL then has to cover only in-flight bytes, not an unbounded queue. I'll say this in a code comment.BatchDesc/freeBatchIDlifecycle, including the pin andtask.batch_idon TCP. I'll build on it after it lands rather than touch that path in parallel.Tests I plan to add with the fix
tebenchTCP 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
freeBatchIDas abandonment? Or does any caller free a busy batch and still expect delivery (fire-and-forget)? If so, I'd add an explicitabandonBatch()/ cancel call instead and use it fromTransferEngineOperationStateonly.cancelTransfer. Should the classic path match its semantics, for example aCANCELEDstatus rather thanFAILED?The test diff is ready; I can push it as a draft PR if that is easier to look at.