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 #4993] [BUG] Unregister fails on the passive side because memory still bound to comm #1266

New issue

Open

opened 2026-10-29 21:24:37 +00:00 by bsakaguchi · 6 comments

bsakaguchi commented

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

Owner

Originally created by @dusty-lsc on GitHub (Oct 27, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4993

Originally assigned to: @ascend-direct-dev on GitHub.

mooncake-transfer-engine/src/transport/ascend_transport
mooncake-transfer-engine-npu==0.3.12（Ascend NPU 910 CANN9.1.0）
Background

We are using Mooncake’s Ascend direct transport (HIXL / AdxlEngine) for KV transfer on the NPU. The Prefill phase acts as the initiator, actively establishing connections, while the Decode phase acts as the listener, passively accepting them. This results in a one-sided connection establishment: only the Client side records the peer in connected_segments_. The Server (passive side) does not record these incoming passive connections.

Phenomenon

During unregister, execution reaches TransferExecutorBase::deregisterMem:

disconnectAllForEngine only iterates over connected_segments_. Since the passive side does not have these entries, it returns immediately, and Disconnect is not executed.
Subsequently, AdxlEngine::DeregisterMem is called for each handle.
Attempting to deregister memory that is still bound to the communication channel causes HCCL to reject the operation, resulting in logs similar to:


DeregisterMem failed
Cannot deregister memory ... still bound to comm

The active side can sometimes tear down the outbound connections it initiated, but the passive side cannot tear down these incoming passive links.

Summary

The passive side’s links are not stored in connected_segments_. When DeregisterMem is executed, the memory remains bound to the communication channel via the passive link, causing the deregistration to fail.

Recommendation

Before deregistering, first tear down the channels on both ends of the local side. Relying solely on Disconnect for connected_segments_ does not cover these passive links.

Originally created by @dusty-lsc on GitHub (Oct 27, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4993 Originally assigned to: @ascend-direct-dev on GitHub. mooncake-transfer-engine/src/transport/ascend_transport mooncake-transfer-engine-npu==0.3.12（Ascend NPU 910 CANN9.1.0） **Background** We are using Mooncake’s Ascend direct transport (HIXL / AdxlEngine) for KV transfer on the NPU. The Prefill phase acts as the initiator, actively establishing connections, while the Decode phase acts as the listener, passively accepting them. This results in a one-sided connection establishment: only the Client side records the peer in connected_segments_. The Server (passive side) does not record these incoming passive connections. **Phenomenon** During unregister, execution reaches TransferExecutorBase::deregisterMem: disconnectAllForEngine only iterates over connected_segments_. Since the passive side does not have these entries, it returns immediately, and Disconnect is not executed. Subsequently, AdxlEngine::DeregisterMem is called for each handle. Attempting to deregister memory that is still bound to the communication channel causes HCCL to reject the operation, resulting in logs similar to: ``` DeregisterMem failed Cannot deregister memory ... still bound to comm ``` The active side can sometimes tear down the outbound connections it initiated, but the passive side cannot tear down these incoming passive links. **Summary** The passive side’s links are not stored in connected_segments_. When DeregisterMem is executed, the memory remains bound to the communication channel via the passive link, causing the deregistration to fail. **Recommendation** Before deregistering, first tear down the channels on both ends of the local side. Relying solely on Disconnect for connected_segments_ does not cover these passive links.

bsakaguchi commented

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

Author

Owner

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

Thanks for opening this issue, @dusty-lsc!

Field	Value
Issue	#4993
GitHub user ID	`169739226`
Reporter	@dusty-lsc

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 27, 2026): Thanks for opening this issue, @dusty-lsc! | Field | Value | |-------|-------| | **Issue** | #4993 | | **GitHub user ID** | `169739226` | | **Reporter** | @dusty-lsc | 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:38 +00:00

Author

Owner

@ascend-direct-dev commented on GitHub (Oct 28, 2026):

Thanks for the report. On CANN 9.2.0, hixl_cs can cover this use case (unregister while memory is still bound to the comm). The older communication-domain path doesn’t handle it cleanly, so we’d recommend moving to the CANN 9.2.0 + hixl_cs stack for this requirement rather than trying to force it on the legacy path.

Happy to dig further if you hit issues after upgrading.

@ascend-direct-dev commented on GitHub (Oct 28, 2026): Thanks for the report. On CANN 9.2.0, `hixl_cs` can cover this use case (unregister while memory is still bound to the comm). The older communication-domain path doesn’t handle it cleanly, so we’d recommend moving to the CANN 9.2.0 + `hixl_cs` stack for this requirement rather than trying to force it on the legacy path. Happy to dig further if you hit issues after upgrading.

bsakaguchi commented

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

Author

Owner

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

Your diagnosis holds up against the code. I traced it to the line level and there are two more teardown paths with the same hole, which is probably why this shows up as flaky rather than always.

connected_segments_ is only ever populated on the active path, at transfer_executor_base.cpp:346, right after a successful Connect. Nothing on the passive accept path writes to it.

So disconnectAllForEngine is a no-op for a passive engine. It looks the key up at :411 and returns at :412 when there is no entry, before it ever calls Disconnect. Then deregisterMem runs :571-573 to disconnect, gets nothing, and calls DeregisterMem at :581 anyway. HCCL rejects that because the memory is still bound to the comm, which is the log line you quoted.

The two paths you did not mention:

cleanupConnections (:431-455) has the identical shape. It iterates connected_segments_ at :433 and calls Disconnect per entry at :441. A passive engine contributes nothing to that map, so process teardown never closes those links either.

rollbackRegisteredMem (:457) is worse. It calls DeregisterMem at :470 straight from a partially built handle list, with no disconnect step at all, so a failed registerMem that rolls back can hit the same rejection even on the active side.

On the fix, one thing needs deciding first. disconnectAllForEngine tears links down by naming the peer, Disconnect(connected_segment.c_str(), ...). For the passive side we have no peer names, because nothing recorded them. So either AdxlEngine needs a way to enumerate or close the links an engine currently holds, or the accept path has to hand those names back to us so we can store them in connected_segments_ alongside the active ones. I could not check which is available: the Ascend transport ships as mooncake-transfer-engine-npu against CANN 9.1.0 on 910, and I have neither the toolchain nor the hardware here, so I have not written or built a patch. Flagging it rather than sending something I cannot compile.

Worth saying the underlying model is one-sided connection establishment. Any transport where one end only listens will have the same shape of bug unless the passive links are tracked somewhere, so if the fix lands here it is worth a look at whether the same assumption is doing work elsewhere.

@he-yufeng commented on GitHub (Oct 29, 2026): Your diagnosis holds up against the code. I traced it to the line level and there are two more teardown paths with the same hole, which is probably why this shows up as flaky rather than always. `connected_segments_` is only ever populated on the active path, at `transfer_executor_base.cpp:346`, right after a successful `Connect`. Nothing on the passive accept path writes to it. So `disconnectAllForEngine` is a no-op for a passive engine. It looks the key up at :411 and returns at :412 when there is no entry, before it ever calls `Disconnect`. Then `deregisterMem` runs :571-573 to disconnect, gets nothing, and calls `DeregisterMem` at :581 anyway. HCCL rejects that because the memory is still bound to the comm, which is the log line you quoted. The two paths you did not mention: `cleanupConnections` (:431-455) has the identical shape. It iterates `connected_segments_` at :433 and calls `Disconnect` per entry at :441. A passive engine contributes nothing to that map, so process teardown never closes those links either. `rollbackRegisteredMem` (:457) is worse. It calls `DeregisterMem` at :470 straight from a partially built handle list, with no disconnect step at all, so a failed `registerMem` that rolls back can hit the same rejection even on the active side. On the fix, one thing needs deciding first. `disconnectAllForEngine` tears links down by naming the peer, `Disconnect(connected_segment.c_str(), ...)`. For the passive side we have no peer names, because nothing recorded them. So either AdxlEngine needs a way to enumerate or close the links an engine currently holds, or the accept path has to hand those names back to us so we can store them in `connected_segments_` alongside the active ones. I could not check which is available: the Ascend transport ships as `mooncake-transfer-engine-npu` against CANN 9.1.0 on 910, and I have neither the toolchain nor the hardware here, so I have not written or built a patch. Flagging it rather than sending something I cannot compile. Worth saying the underlying model is one-sided connection establishment. Any transport where one end only listens will have the same shape of bug unless the passive links are tracked somewhere, so if the fix lands here it is worth a look at whether the same assumption is doing work elsewhere.

bsakaguchi commented

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

Author

Owner

@dusty-lsc commented on GitHub (Oct 29, 2026):

Thanks, that matches what we see. connected_segments_ is only filled after a successful Connect on the initiator, so a passive engine never has an entry. disconnectAllForEngine looks the key up and returns before Disconnect, and deregisterMem then calls DeregisterMem anyway. HCCL rejects it because the memory is still bound to the comm.

cleanupConnections has the same shape: it only walks connected_segments_, so process teardown also leaves the passive links up. rollbackRegisteredMem is a separate hole. It calls DeregisterMem on the handles built so far with no disconnect at all, so a failed registerMem can hit the same rejection on the active side too.

On the missing peer names: for the sleep/unregister case we do not need them. AdxlEngine::Finalize() destroys every local client and server channel, including passive ones that were never recorded, and then deregisters the memory. Our workaround, in deregisterMem, is to drop the local registration and connection maps and Finalize each local AdxlEngine, then return success. We do not Initialize again, because this path is tearing the engine down. A later unregister in the same sleep finds an empty map and returns.

That does not cover a partial unregister that must keep other links alive. For that, AdxlEngine still needs either a way to list and close the channels it holds, or the accept path has to record the peer names into connected_segments_. Finalize is the blunt option and it is what sleep needs.

The same one-sided assumption is what makes both disconnectAllForEngine and cleanupConnections miss the passive side, so any similar teardown that only iterates connected_segments_ will have this hole.

@dusty-lsc commented on GitHub (Oct 29, 2026): Thanks, that matches what we see. connected_segments_ is only filled after a successful Connect on the initiator, so a passive engine never has an entry. disconnectAllForEngine looks the key up and returns before Disconnect, and deregisterMem then calls DeregisterMem anyway. HCCL rejects it because the memory is still bound to the comm. cleanupConnections has the same shape: it only walks connected_segments_, so process teardown also leaves the passive links up. rollbackRegisteredMem is a separate hole. It calls DeregisterMem on the handles built so far with no disconnect at all, so a failed registerMem can hit the same rejection on the active side too. On the missing peer names: for the sleep/unregister case we do not need them. AdxlEngine::Finalize() destroys every local client and server channel, including passive ones that were never recorded, and then deregisters the memory. Our workaround, in deregisterMem, is to drop the local registration and connection maps and Finalize each local AdxlEngine, then return success. We do not Initialize again, because this path is tearing the engine down. A later unregister in the same sleep finds an empty map and returns. That does not cover a partial unregister that must keep other links alive. For that, AdxlEngine still needs either a way to list and close the channels it holds, or the accept path has to record the peer names into connected_segments_. Finalize is the blunt option and it is what sleep needs. The same one-sided assumption is what makes both disconnectAllForEngine and cleanupConnections miss the passive side, so any similar teardown that only iterates connected_segments_ will have this hole.

bsakaguchi commented

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

Author

Owner

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

Following up on the fix question I left open. I went and read the AdxlEngine surface rather than leave it as a maybe.

adxl_compat.h exposes Initialize/Finalize, RegisterMem/DeregisterMem, and Connect(remote_engine, timeout) / Disconnect(remote_engine, timeout). There is no way to ask an engine which links it currently holds, and no callback when a passive link comes in.

That settles it: the fix cannot live in TransferExecutorBase alone. Both Connect and Disconnect are keyed by a named remote engine, and on the passive side we have no names and no way to ask for them. So it is one of these two, and which one is a call for whoever owns AdxlEngine:

AdxlEngine grows an enumeration or a close-everything operation, and disconnectAllForEngine uses it for engines with no connected_segments_ entry.
The accept path hands the incoming peer's name back so it can be stored in connected_segments_ next to the active ones, and everything else stays as it is.

Option 2 is smaller and keeps one bookkeeping structure, but it means the accept side has to learn names it does not currently surface. Option 1 works even for links nobody told us about, which also covers the rollbackRegisteredMem path I mentioned earlier, where nothing recorded anything at all.

I have not written a patch either way. The transport ships as mooncake-transfer-engine-npu against CANN 9.1.0 on 910 and I have no toolchain or hardware here, so anything I wrote would be uncompiled and untested. Flagging the constraint instead.

@he-yufeng commented on GitHub (Oct 29, 2026): Following up on the fix question I left open. I went and read the AdxlEngine surface rather than leave it as a maybe. `adxl_compat.h` exposes `Initialize`/`Finalize`, `RegisterMem`/`DeregisterMem`, and `Connect(remote_engine, timeout)` / `Disconnect(remote_engine, timeout)`. There is no way to ask an engine which links it currently holds, and no callback when a passive link comes in. That settles it: the fix cannot live in `TransferExecutorBase` alone. Both `Connect` and `Disconnect` are keyed by a named remote engine, and on the passive side we have no names and no way to ask for them. So it is one of these two, and which one is a call for whoever owns AdxlEngine: 1. AdxlEngine grows an enumeration or a close-everything operation, and `disconnectAllForEngine` uses it for engines with no `connected_segments_` entry. 2. The accept path hands the incoming peer's name back so it can be stored in `connected_segments_` next to the active ones, and everything else stays as it is. Option 2 is smaller and keeps one bookkeeping structure, but it means the accept side has to learn names it does not currently surface. Option 1 works even for links nobody told us about, which also covers the `rollbackRegisteredMem` path I mentioned earlier, where nothing recorded anything at all. I have not written a patch either way. The transport ships as `mooncake-transfer-engine-npu` against CANN 9.1.0 on 910 and I have no toolchain or hardware here, so anything I wrote would be uncompiled and untested. Flagging the constraint instead.

bsakaguchi commented

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

Author

Owner

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

Thanks, that is useful and it changes what I would do here. Moving to hixl_cs on 9.2.0 is clearly the right advice for the failure the reporter hit.

One thing I want to separate before we call this closed, because it is a different question from the one hixl_cs answers. The unregister error is the symptom; the reason it fires is that the passive side never records its incoming links, so nothing ever tears them down.

Traced in transfer_executor_base.cpp:

connected_segments_ is written only at :346, on the initiator path after a successful Connect.
disconnectAllForEngine looks the key up at :411 and returns at :412 when there is no entry, so it never calls Disconnect for a passive engine.
cleanupConnections (:431-455) walks the same map, so process teardown does not close those links either.
~TransferExecutorBase is defaulted, and rollbackRegisteredMem (:457) deregisters with no disconnect step at all.

So if hixl_cs lets DeregisterMem go through while the memory is still bound, the call stops failing but the link is still live. That may well be fine and intended for this transport, in which case say so and I will stop. I raise it only because "the unregister no longer errors" and "the passive links are released" are not the same statement, and I cannot measure either from here.

To be clear about what I can and cannot back up: everything above is from reading the source. I have no CANN install and no 910, so I have not run the hixl_cs path and I am not claiming it leaks. If the new stack does its own teardown of incoming links when the comm is unbound, the bookkeeping gap does not matter and this issue is done.

On the reporter's side the practical answer stands regardless: upgrade to 9.2.0 and use hixl_cs.

@he-yufeng commented on GitHub (Oct 29, 2026): Thanks, that is useful and it changes what I would do here. Moving to `hixl_cs` on 9.2.0 is clearly the right advice for the failure the reporter hit. One thing I want to separate before we call this closed, because it is a different question from the one `hixl_cs` answers. The unregister error is the symptom; the reason it fires is that the passive side never records its incoming links, so nothing ever tears them down. Traced in `transfer_executor_base.cpp`: - `connected_segments_` is written only at :346, on the initiator path after a successful `Connect`. - `disconnectAllForEngine` looks the key up at :411 and returns at :412 when there is no entry, so it never calls `Disconnect` for a passive engine. - `cleanupConnections` (:431-455) walks the same map, so process teardown does not close those links either. - `~TransferExecutorBase` is defaulted, and `rollbackRegisteredMem` (:457) deregisters with no disconnect step at all. So if `hixl_cs` lets `DeregisterMem` go through while the memory is still bound, the call stops failing but the link is still live. That may well be fine and intended for this transport, in which case say so and I will stop. I raise it only because "the unregister no longer errors" and "the passive links are released" are not the same statement, and I cannot measure either from here. To be clear about what I can and cannot back up: everything above is from reading the source. I have no CANN install and no 910, so I have not run the `hixl_cs` path and I am not claiming it leaks. If the new stack does its own teardown of incoming links when the comm is unbound, the bookkeeping gap does not matter and this issue is done. On the reporter's side the practical answer stands regardless: upgrade to 9.2.0 and use `hixl_cs`.