[GH-ISSUE #3890] [Bug][TransferEngine] Failed notify batches retain stale notification state #993

New issue

Closed

opened 2026-10-29 21:21:44 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

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

Owner

Originally created by @CorgiBoyG on GitHub (Sep 6, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3890

Bug Report

Description

Classic Transfer Engine keeps notify metadata after a failed transfer batch is successfully freed.

TransferEngineImpl::submitTransferWithNotify() stores the target and NotifyDesc in notifies_to_send_, keyed by BatchID. getBatchTransferStatus() erases that entry only when the whole batch reaches COMPLETED. On FAILED or TIMEOUT, callers can later release the batch through freeBatchID(), but that method only delegates to MultiTransport::freeBatchID().

MultiTransport::freeBatchID() deletes the underlying BatchDesc, while the notifies_to_send_ entry remains alive with the same pointer-derived key and its notification strings.

Runtime path

The Python synchronous transfer paths exercise this lifecycle:

TransferEnginePy::transferSync / batchTransferSync
  -> allocateBatchID()
  -> submitTransferWithNotify()
       -> notifies_to_send_[batch_id] = notify
  -> getTransferStatus() / getBatchTransferStatus()
       -> FAILED
  -> freeBatchID()
       -> delete BatchDesc
       -> notification entry remains

The same side table is shared by the multi-protocol notify submission path.

Impact

The immediate deterministic impact is lifetime retention: each failed notify batch can retain an unordered_map node, target ID, notification name, and arbitrary notification payload until the engine is destroyed.

There is also an ABA correctness risk because BatchID is the integer representation of the allocated BatchDesc*. If the allocator later reuses that address for a different non-notify batch, a successful status query for the new batch can find the stale entry and send the old notification for unrelated data.

Expected behavior

When freeBatchID() successfully releases a batch, it should also remove any pending notification associated with that batch. A failed freeBatchID() (for example, BatchBusy) must keep the notification because the batch is still live and may later complete successfully.

Reproduction

A deterministic unit test can use the existing TransferEngineImplTestPeer and fake transports in mooncake-transfer-engine/tests/transport_uint_test.cpp:

Submit a notify transfer through a fake transport that reaches terminal FAILED.
Confirm the batch status is FAILED.
Successfully call freeBatchID(batch_id).
Inspect the pending-notify side table through the test peer.

Current behavior leaves one entry after step 3. The expected count is zero. A companion case should verify that BatchBusy leaves the entry intact.

Relevant code

mooncake-transfer-engine/include/transfer_engine_impl.h
- submitTransferWithNotify()
- mp_submitTransferWithNotify()
- freeBatchID()
- getBatchTransferStatus()
mooncake-transfer-engine/src/multi_transport.cpp
- allocateBatchID()
- freeBatchID()
- getBatchTransferStatus()
mooncake-integration/transfer_engine/transfer_engine_py.cpp
- transferSync()
- batchTransferSync()

I plan to add hardware-independent regression coverage for both successful release and BatchBusy, then make TransferEngineImpl::freeBatchID() erase the side-table entry only after the underlying batch is actually freed. I expect to have a reviewed implementation ready within one day.

This issue was identified with AI assistance; the contributor independently traced the current upstream/main call path and collision-checked existing issues and pull requests.

Originally created by @CorgiBoyG on GitHub (Sep 6, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3890 ## Bug Report ### Description Classic Transfer Engine keeps notify metadata after a failed transfer batch is successfully freed. `TransferEngineImpl::submitTransferWithNotify()` stores the target and `NotifyDesc` in `notifies_to_send_`, keyed by `BatchID`. `getBatchTransferStatus()` erases that entry only when the whole batch reaches `COMPLETED`. On `FAILED` or `TIMEOUT`, callers can later release the batch through `freeBatchID()`, but that method only delegates to `MultiTransport::freeBatchID()`. `MultiTransport::freeBatchID()` deletes the underlying `BatchDesc`, while the `notifies_to_send_` entry remains alive with the same pointer-derived key and its notification strings. ### Runtime path The Python synchronous transfer paths exercise this lifecycle: ```text TransferEnginePy::transferSync / batchTransferSync -> allocateBatchID() -> submitTransferWithNotify() -> notifies_to_send_[batch_id] = notify -> getTransferStatus() / getBatchTransferStatus() -> FAILED -> freeBatchID() -> delete BatchDesc -> notification entry remains ``` The same side table is shared by the multi-protocol notify submission path. ### Impact The immediate deterministic impact is lifetime retention: each failed notify batch can retain an `unordered_map` node, target ID, notification name, and arbitrary notification payload until the engine is destroyed. There is also an ABA correctness risk because `BatchID` is the integer representation of the allocated `BatchDesc*`. If the allocator later reuses that address for a different non-notify batch, a successful status query for the new batch can find the stale entry and send the old notification for unrelated data. ### Expected behavior When `freeBatchID()` successfully releases a batch, it should also remove any pending notification associated with that batch. A failed `freeBatchID()` (for example, `BatchBusy`) must keep the notification because the batch is still live and may later complete successfully. ### Reproduction A deterministic unit test can use the existing `TransferEngineImplTestPeer` and fake transports in `mooncake-transfer-engine/tests/transport_uint_test.cpp`: 1. Submit a notify transfer through a fake transport that reaches terminal `FAILED`. 2. Confirm the batch status is `FAILED`. 3. Successfully call `freeBatchID(batch_id)`. 4. Inspect the pending-notify side table through the test peer. Current behavior leaves one entry after step 3. The expected count is zero. A companion case should verify that `BatchBusy` leaves the entry intact. ### Relevant code - `mooncake-transfer-engine/include/transfer_engine_impl.h` - `submitTransferWithNotify()` - `mp_submitTransferWithNotify()` - `freeBatchID()` - `getBatchTransferStatus()` - `mooncake-transfer-engine/src/multi_transport.cpp` - `allocateBatchID()` - `freeBatchID()` - `getBatchTransferStatus()` - `mooncake-integration/transfer_engine/transfer_engine_py.cpp` - `transferSync()` - `batchTransferSync()` I plan to add hardware-independent regression coverage for both successful release and `BatchBusy`, then make `TransferEngineImpl::freeBatchID()` erase the side-table entry only after the underlying batch is actually freed. I expect to have a reviewed implementation ready within one day. This issue was identified with AI assistance; the contributor independently traced the current `upstream/main` call path and collision-checked existing issues and pull requests.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

@github-actions[bot] commented on GitHub (Sep 6, 2026):

Thanks for opening this issue, @CorgiBoyG!

Field	Value
Issue	#3890
GitHub user ID	`111257566`
Reporter	@CorgiBoyG

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 (Sep 6, 2026): Thanks for opening this issue, @CorgiBoyG! | Field | Value | |-------|-------| | **Issue** | #3890 | | **GitHub user ID** | `111257566` | | **Reporter** | @CorgiBoyG | 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 referenced this issue

2026-10-29 21:23:17 +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

bsakaguchi referenced this issue

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

[PR #993] [MERGED] [Store]: Cleanup processing objects if transferring timedout (#975) #1938