Repository navigation
Conversation
Backport HCCL terminal publication and callback lifetime protection to v0.3.12_xllm_patch. Expose library-owned batch retirement status so callers never inspect conditional private task layouts across the SDK boundary.
8 of 19 tasks
THU-syh
marked this pull request as ready for review
November 13, 2026 10:37
8 of 9 tasks
The public TransferEngine::getBatchRetirementStatus wrapper calls the TransferEngineImpl middle layer, which had no such member, so the non-TENT wrapper translation unit did not compile and the new SDK call was unusable from the public API. MultiTransport::getBatchRetirementStatus also called toBatchDesc unqualified from a class that does not derive from Transport, breaking the multi_transport.cpp translation unit in every configuration. Fix both and add mooncake-transfer-engine/tests/batch_retirement_test.cpp: hardware-free coverage for the public/impl forwarding path, drain-before- release on pending slices, the completion-callback gate for both retirement and transport cleanup, failed batches retiring with an error status, and query errors that must not retire. Verified locally (aarch64, GCC 12.3, glog/gflags/gtest, no NPU): - transfer_engine.cpp wrapper TU failed before the impl fix and compiles after it; multi_transport.cpp failed before the qualification fix. - transfer_engine library builds in both USE_ASCEND_DIRECT=ON and default configurations; ctest -R batch_retirement passes 7/7 in the default build. - Not covered here: the HCCL transport TU needs adapter_hccp_common.h from the HCCL adapter, which this host does not provide; no NPU device test.
Terminal slice counters can come from two different events: the transport confirmed that no access is in flight, or it gave up after a timeout/abort it could not prove. Callers must not release borrowed buffers in the second case, but the retirement API only exposed a single `retired` flag. Expose the distinction: - Transport::isBatchQuiescent(): transports that publish give-up terminal states override it. AscendDirect marks a slice unsafe when its terminal state comes from an unproven timeout/abort (local copy stream timeout, async route failure with a submitted ADXL request, sync transfer timeout). - getBatchRetirementStatus(batch_id, retired, quiescent) overload on Transport managers, MultiTransport, TransferEngineImpl and TransferEngine. TENT only certifies COMPLETED, so it reports quiescent == retired. - Transport::abandonBatch()/AbandonOutcome for stalled batches: an explicit, bounded stop request whose outcome tells the caller whether buffers are safe (recoverable) or the transport must be restarted (device fault). Only the recoverable outcome publishes the terminal counters. Tests: hardware-free coverage for quiescent/non-quiescent retirement and the three abandon outcomes. Local build: transfer_engine in both the default and USE_ASCEND_DIRECT configurations; gtest 12/12. No device validation yet.
THU-syh
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
xLLM hands Mooncake a batch of borrowed KV buffers and must not release them before the transport has actually stopped touching them. Two failure modes were unsafe: releasing as soon as the aggregate status said FAILED (in-flight work still wrote into the freed pages), and publishing "retired" from a timeout/abort that never proved the transfer had stopped.
This PR makes retirement self-describing and gives callers a bounded, explicit way to give up:
TransferEngine::getBatchRetirementStatus(BatchID, bool& retired)lets the SDK decide whether a batch may be released; callers no longer read privateBatchDesc/TransferTasklayouts (aWITH_METRICSbuild produced a differentTransferTasklayout across translation units and causedTask ID out of range).freeBatchIDuses the same library-owned check and may clean up a failed batch once it is retired.Transport::freeBatchID), so a callback can no longer outlive the batch descriptor.getBatchRetirementStatus(batch_id, retired, quiescent)(new overload) reports the difference:quiescent == falsemeans the batch retired because of an unproven timeout/abort and the caller must not release the buffers.Transport::isBatchQuiescent()is the transport hook;AscendDirectmarks a slice unsafe when its terminal state comes from an unproven abort/timeout (local-copy stream timeout, async route failure with a submitted ADXL request, synchronous transfer timeout).TransferEngine::abandonBatch(batch_id, timeout, AbandonOutcome&)lets a caller that has waited long enough ask the transport to stop:kQuiescentRecoverablepublishes the terminal counts (safe to free),kQuiescentDeviceFaultkeeps the batch pending on purpose (the caller must restart the engine instead of releasing memory),kNotQuiescentmeans no proof exists and the caller keeps waiting.markSuccess()no longer rewrites the transferred byte counter.Production transport
xLLM builds Mooncake with
USE_ASCEND_DIRECT=ON(xllm/CMakeLists.txt), i.e. the AscendDirect transport. The HCCL transport (USE_ASCEND) is not built there, so the quarantine/abort work above is implemented and tested against AscendDirect; the HCCL hunks from the base commit are kept, but a bounded HCCL abandon implementation is out of scope for this PR and will be proposed separately.TENT only certifies
COMPLETED; a cached failure/timeout is not a quiescence proof, so the TENT variant reportsquiescent == retiredandabandonBatchreturnsNotImplemented. RDMA/TENT abandon is likewise future work.Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
Hardware-free unit tests (
mooncake-transfer-engine/tests/batch_retirement_test.cpp), 12 cases:abandonBatchoutcomes: recoverable publishes terminal state, device fault keeps the batch pending, unsupported transport reportsNotImplemented;retired && quiescentversusretired && !quiescent.Local environment: aarch64 openEuler 24.03, GCC 12.3.1, glog 0.6.0, no NPU device.
transfer_enginebuilds in both the default andUSE_ASCEND_DIRECT=ONconfigurations; the retirement wrapper TU that previously failed to compile now builds. clang-format 20.1.8 applied with the repository.clang-format.Not verified here: no NPU device test and no real HCCL failure-retirement test. Device validation of the AscendDirect unsafe/fault paths (including an opt-in stall injection) is planned on 910C and will be reported separately.
Integration status
Companion: xLLM MR791. Before xLLM can merge, this SDK revision has to be published into the official dependency and its configured GitCode mirror, and MR791 has to pin it and pass its native CI. The current official 0.3.12 pin does not contain the retirement API.