Repository navigation
Conversation
Defer failed slice publication until stream synchronization and the existing retry loop finish. Publish every slice exactly once so terminal counters cannot authorize early buffer reuse. Retain borrowed buffers while stream synchronization still fails; report abort errors and preserve explicit transfer failure after retirement.
Use LOG_EVERY_T from glog0.6 for failed stream retirement diagnostics. LOG_EVERY_N_SEC belongs to another logging API and prevents the HCCL source from compiling.
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
跨四个 PR/MR 的独立复核：#10 与 #11 不是重复修复
评审 head 50a9dd59df23，base ff3258bbd305。本轮检查全部改动（1 个文件）及 HCCL 状态查询、Slice 终态发布、xLLM 等待/释放调用链；评论由 Codex 辅助审查。
判干净
hccl_transport.cpp:136–152,198：提交/fence/重试错误只写 transport-internalslice->status，不再立即增加终态计数。:238–243：在同步及重试循环结束后才统一markFailed/markSuccess，每个 slice 只发布一次。这同时解决“退休前公布失败”和“重试重复公布失败”。HcclTransport::getTransferStatus(:546–573)读取成功/失败计数，而不是内部slice->status；因此只把内部状态设成 FAILED 不会提前让调用方观察到 terminal。- 当前已使用
LOG_EVERY_T；先前LOG_EVERY_N_SEC的意见不是当前 head 的未修缺陷。
区别与组合关系
本 PR 修产出端终态何时可信，配套 xLLM !791 修聚合 FAILED 后仍排空全部 task/callback，再允许页面复用。
#11 修的是deferred reaper 遇到查询 API 错误时不能丢失回收记录和 hook，它不负责 HCCL 流退休，两者是互补而非重复修复。
尚缺的证据 / 合入判定
一条行级验证意见见 :171：真实 SDK 的 abort/synchronize 退休保证、永久错误的停服策略、以及仓内回归仍需落实。本轮尝试原生 HCCL TU 语法编译，但当前隔离环境缺少私有 SDK 头 adapter_hccp_common.h，因此没有完成原生构建；这不是对 PR 代码编译失败的判定。本轮没有执行真实 HCCL/PD 故障注入，不把作者的 CPU 模拟或另一组 AscendDirect 测试当成该保证的设备证据。
结论：修复方向正确；作为 xLLM 重升级组合，暂不建议直接合入上线。 先落实 HCCL 验证；#10/#11 合入并同步官方 GitCode 后，两个 xLLM MR 应统一使用同时包含两项修复的依赖 pin，而不是各自选一个修复版本。
| // Synchronize to drop ACL_ERROR_RT_STREAM_ABORT error | ||
| aclrtSynchronizeStream(stream); | ||
| int drain_ret = aclrtSynchronizeStream(stream); | ||
| while (drain_ret != ACL_SUCCESS) { |
There was a problem hiding this comment.
[P2 / 合入前验证项] 这里的永久错误退出策略和“流退休”保证还需要真实 HCCL/ACL 证据；不是建议超时后直接释放缓冲区
drain_ret != ACL_SUCCESS 会一直占住 initiatorLoop。对应头文件定义 THREAD_NUM=1，析构又会 join() initiator 线程（47–55 行），所以后续同步永远失败时，不只是当前 batch 等待：这条 initiator 队列和正常析构都会被卡住。PR 描述已经承认会保留 borrowed buffers，我确认这是一项有意的安全取舍，不把“看到 TIMEOUT”误认为 I/O 已停止。
尚缺的证据是：部署 SDK 在提交/fence 失败、stream abort 失败/成功之后，哪一种同步返回值足以证明已经提交的 READ/WRITE（包括远端写入）不会继续访问借用内存。本轮静态检查无法替代这个设备契约。
建议合入门槛：用实际部署的 CANN/HCCL 验证这几条故障路径，记录终态发布只发生一次、目标页复用后无迟到写、以及永久错误时如何停服/重启和告警。不要为了给循环加上界而直接 return、伪造终态或释放页面；若暂时只有无限保留策略，应明确批准它的可用性代价。最好把当前仓外 reproducer 的关键断言接入仓内回归，否则仓库 CI 无法看守此次发布顺序修复。
There was a problem hiding this comment.
第二轮复核：代码未变，配套关系已变更
本轮 head 仍是 50a9dd59df23，该段同步/abort/退休等待代码与上一轮完全一致。因此上一条关于真实 SDK 退休保证、永久错误时资源持有/停服策略和仓内故障回归的验证意见仍未获得新的设备证据；我不把描述更新当作这些验证已经完成，也不重复报旧日志宏。
需要修正上一轮的配套前提：当前 !791 92807b8e64c4 已转到 0.3.12 + PR #12，不再以本 0.3.14 PR 为它的当前 producer。!800/#11 仍走 0.3.14 路线。本 PR 描述已明确区分历史与当前，这一点认可。
本轮还在 !791 的当前配套 PR12 05fc22b3013e 上发现了实际原生 TU 编译阻断：公开 wrapper 调用 TransferEngineImpl::getBatchRetirementStatus，但 impl 头漏掉该方法。这项已评论到 !791 的 API 调用行；不是本 PR 的新增源码缺陷。最终需要先确定采用 0.3.12 还是 0.3.14，再统一 producer/consumer 的退休契约、官方 pin 和构建验证，不能沿用历史联测直接宣布最新 !791 已兼容 #10。
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
第二轮独立复核（head 50a9dd59df23）：代码未更新，配套描述已纠正
重新获取全部 1 个改动文件并与上轮比对：源码 diff 与提交 SHA 均未变。本轮未发现新的确定性代码缺陷；HCCL 在同步/重试结束后统一发布每个 slice 的终态这一方向仍认可。当前 LOG_EVERY_T 不再是旧宏问题。
变化与仍待验证
- 描述已改正配套关系：!791 现为 0.3.12/PR12，之前的 !791/#10 联测是历史证据；!800/#11 仍为 0.3.14。不能把它们当成已经验证的同一依赖版本。
- 上轮
hccl_transport.cpp:171的真实 ACL/HCCL abort/synchronize 退休保证、永久同步失败的可用性策略和仓内回归证据未补齐。本轮没有设备执行，这些合入验证条件仍保留；不建议用超时返回/释放页去“修复”有意保留资源的行为。 - 本轮重新运行另外三项修改的 33 个原始 CPU 用例和 4 个针对性检查，均通过；这不替代本 transport 的设备故障验证。发现的 PR12 wrapper 编译问题和 #11 重复字节计数问题分别评论在对应配套 review 中，不能归到本 PR 的代码。
判定：本 PR 无新增源码问题，但仍不具备设备安全性完整放行依据。 先确定最终 0.3.12/0.3.14 路线，统一官方可获取 pin 和回归，再处理合入。此为 Codex 辅助的复核，不是 GitHub Approve。
| for (auto slice : slice_list) { | ||
| if (slice->status != Slice::SliceStatus::FAILED) { | ||
| if (slice->status == Slice::SliceStatus::FAILED) { | ||
| slice->markFailed(); |
There was a problem hiding this comment.
[P2] PR 描述「每个切片恰好在流同步与重试结束后发布」有一处例外：segment 查找失败路径仍在提交前直接 markFailed（已验证安全，但契约声明与实现不符）
主现场：hccl_transport.cpp:96-104（本 PR 未改动该处）——initiatorLoop 在进入重试 for-loop 之前：
auto segment_desc = metadata_->getSegmentDescByID(slice_list[0]->target_id);
if (!segment_desc) {
LOG(ERROR) << "Unable to get target segment ID, please recheck, ...";
for (auto slice : slice_list) {
slice->markFailed(); // 仍为即时发布
}
continue;
}为什么安全（静态论证）：该检查位于重试循环之前，本批次切片尚未提交任何 stream 操作（transportMemTask 在 loop 内才调用）；initiator 单线程（THREAD_NUM=1）串行执行，上一批次已通过 aclrtSynchronizeStreamWithTimeout 排空——若上批 sync 失败会先进入排空循环，到不了本路径。因此此处发布时不存在在飞的借用内存访问，不构成 early-free 风险。
问题：PR 描述声明 "publish each slice exactly once after stream synchronization and the existing retry loop have finished" 用了全称量词，实现有一处例外。风险是模式脆弱性：未来若有人把 segment 查找移到提交之后、或把该「查不到就 markFailed」模式复制到已提交场景，会静默恢复早发布。
最小修复：在该 markFailed 处加一行注释「本批次零提交、前批已排空，允许立即发布」；或把 PR 描述改为「除提交前失败（segment 查找）外」。
验证：纯静态走查即可关闭（本评论已完成论证）。
| aclrtSynchronizeStream(stream); | ||
| int drain_ret = aclrtSynchronizeStream(stream); | ||
| while (drain_ret != ACL_SUCCESS) { | ||
| // An error result alone cannot release borrowed buffers. |
There was a problem hiding this comment.
[P2] PR10 不提供任何 retirement 可观测接口：0.3.14 消费者只能看到无区别的 WAITING；MR791 式 requires 检查对本 PR fail-closed
（对 ShuweiShen772 round-2「统一 producer/consumer 退休契约」要求的代码级机制证据补充）
证据：
- 本 PR 树全树 grep
getBatchRetirementStatus零命中（PR11 同线已补完整 API 链：transfer_engine.h:301 / multi_transport.cpp:745）。 - 消费者侧（xllm MR791 head 92807b8e6，
mooncake_batch_retirement.h:39-46）用编译期 requires 探测：
if constexpr (!requires(Engine* e, mooncake::BatchID id, bool& retired) {
e->getBatchRetirementStatus(id, retired); }) {
LOG(ERROR) << "Mooncake retirement API unavailable, peer=" << remote_addr;
return mooncake::Status::Context("Mooncake retirement API unavailable");
}- HCCL 侧对调用方唯一可见的状态通道
getTransferStatus（:563-572）在排空期间返回 WAITING——「排空等待（缓冲必须继续持有）」与「传输进行中」不可区分。
后果：(a) 按当前 MR791 模式编写的消费者链接本 PR 的 SDK，requires 判定失败 → 每批 dispatch 前被拒（fail-closed，安全但功能不可用）；(b) 若有消费者对 WAITING 自设超时后释放缓冲，会在调用方层重现本 PR 要修的早释放 bug。这具体化了 PR 描述自称 "Neither review alone establishes complete HCCL failure-path correctness" 在 PR10 单侧的不完整点：transport 修好了发布时机，但没有给 caller 任何受契约保护的等待依据。
建议：若 0.3.14 线要服务 MR791 类消费者，需以 PR11（或其回移植）为配套并在两 PR 描述中互相点名；同时给 0.3.14 直接消费者写明「不得对 WAITING 设超时后释放缓冲」。
| while (drain_ret != ACL_SUCCESS) { | ||
| // An error result alone cannot release borrowed buffers. | ||
| // Keep the batch pending until the stream actually drains. | ||
| LOG_EVERY_T(ERROR, 1) |
There was a problem hiding this comment.
[P2] 排空等待仅有 1 秒节流的 ERROR 日志，无指标、无告警钩子
（round-1 已把「永久错误时如何停服/告警」列为合入验证门槛；本条是最小代码级增量建议，非重发）
贴码（:170-179，运行在唯一 initiator 线程上，THREAD_NUM=1；卡住期间该设备所有后续批次在 allReqQueues_[0] 排队）：
int drain_ret = aclrtSynchronizeStream(stream);
while (drain_ret != ACL_SUCCESS) {
LOG_EVERY_T(ERROR, 1)
<< "Waiting for failed HCCL stream retirement, ret: " << drain_ret;
std::this_thread::sleep_for(std::chrono::milliseconds(1));
drain_ret = aclrtSynchronizeStream(stream);
}问题：每秒最多一条 ERROR 日志，此外无任何可编程观测点；卡住时长、卡住的批次数、涉及的 peer 均不可度量。运维只能靠 grep 日志发现「排空等待」状态，无法回答「排空等待了多久/影响了多少批次」。
最小修复：WITH_METRICS 下增加一个计数器（drain-wait 进入次数）或等待时长直方图；无 metrics 编译时保持现状即可。
验证：静态；若采纳，随 metrics 导出路径测试。
| // Keep the error private until the stream and all retries | ||
| // have retired. Publishing FAILED here lets callers free | ||
| // buffers that a sibling operation still accesses. | ||
| slice->status = Slice::FAILED; |
There was a problem hiding this comment.
[P2·既有] 提交失败具有跨重试粘性：某轮提交失败的切片即使后续重试实际传输成功，最终仍发布 FAILED（假失败）
（pre-existing，base 行为等价，非本 PR 回归，不阻塞合入）
贴码（:139 置位不清除 + :238-244 最终循环对 FAILED 一律 markFailed）：
for (int loop = 0; loop <= transfer_max_retry_cnt; ++loop) {
for (auto slice : slice_list) {
ret = transportMemTask(...);
if (ret) { ... slice->status = Slice::FAILED; } // 置位，后续轮不清除
}
...
}触发条件：切片在第 k 轮提交失败、第 k+1 轮（重连后）提交且 sync 成功。
后果：status 保持 FAILED → 最终 markFailed → 任务报 FAILED，尽管数据已实际写入目的缓冲；调用方收到假失败后重试整个请求 → 目的缓冲被再次写入同一数据（幂等前提是调用方在终态前不修改源缓冲——这正是本 PR 强化的契约）。
最小修复：每轮重试开始时把将重新提交的切片 status 重置为 PENDING；或明确记录为已知语义。
验证：CPU fault-injection 场景「第 0 轮提交失败 + 第 1 轮成功」断言最终状态。
Review 总结：发布时机修复机制成立（With fixes，均为 P2/文档级）独立复核了全部 23 行改动（静态对照 base ff3258b，head 50a9dd5）。 做得好的（已证实）：
Findings（4 条 inline + 1 条既有并入本条）：
与 ShuweiShen772 已有意见的关系：永久错误退出策略/THREAD_NUM=1 析构 join/设备验证证据/仓内回归/版本线统一已由 round-1、round-2 完整覆盖，不重发；以上均为增量。 配对结论（对团队更重要）：0.3.14 线若要承接 MR791 类消费者，PR10 必须与 PR11（retirement API）成对评估，PR10 单独不构成完整的失败路径正确性；MR791/MR800 当前已转投 0.3.12+PR12 线。 （审查方法：物化 base/head 两树静态对照 + 契约提取；作者声称的 CPU fault-injection 复现与 glog 编译验证为作者口径，未本地复现。） |
Companion xLLM review
MR791 has now followed the xLLM main rollback to
v0.3.12_xllm_patch. Its current companion is Mooncake PR12, with an opaque library-owned retirement API and the callback protection missing from 0.3.12. This PR remains the separate 0.3.14 HCCL candidate; its earlier MR791 commit and tests below are historical and do not establish compatibility with the current rollback MR. Official SDK publication and gitlink updates are still required before integration.Description
HCCL currently calls
Slice::markFailed()after a transfer submission or fence error, before synchronizing the stream. That publishes terminal task counters while already-submitted operations may still access borrowed source/destination memory. A caller can then observe all tasks as terminal and reuse those buffers too early. The retry loop can also publish failure more than once for the same slice.Keep errors in the existing transport-internal slice status and publish each slice exactly once after stream synchronization and the existing retry loop have finished. Check the stream-abort result and keep buffers pending while post-abort synchronization has not succeeded. Ordinary transfer failures remain explicit failures; this change adds no process abort, transport fallback, configuration flag, or build-time patch.
Target
v0.3.14_xllm_patchatff3258bbd3051c212653d75850a4e3e0f43808e4, the dependency revision for which the fault-injection validation was performed. The repository's oldermainhas a different HCCL loop and is not included in this focused backport. No existing open PR or issue covers this change.If the stream can never be synchronized successfully, the affected work retains its borrowed buffers instead of authorizing reuse. HCCL maintainers should verify the SDK's abort/error synchronization guarantees before merge.
Suggested Merge Priority
Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
Test commands:
Test results:
The integration-side CPU reproducer executes the production HCCL function body with simulated ACL/HCCL boundaries and the xLLM consumer/page allocator. Ten scenarios passed: submission error (including the default retry count), fence error, stream-sync error, temporarily failing post-abort synchronization, partial submission, query error, READ failure, successful transfer, and callback-lifetime control. Failure scenarios returned an explicit request error after retirement with the process alive and the reused destination unchanged.
Using the exact unmodified HCCL source with the same updated consumer reproduces early page reuse: the replacement request's sentinel changes from 700 to 102. The proposed source preserves 700. The reproducer is external to this repository and is not a linked native project test.
clang-format 20.1.6, pre-commit, and diff checks passed. No NPU/device execution, full native build, real PD/HCCL fault test, or performance validation has been performed. Device validation remains required.
Checklist
Signed-off-bytrailer certifying the DCO./scripts/code_format.shAI Assistance Disclosure
Codex assisted with code analysis, the candidate implementation, and CPU fault-injection validation. Human line-by-line review and DCO certification remain pending; the candidate should remain a draft until those requirements and the HCCL device checks are addressed.
Logging compile correction (2026-11-13)
Commit 50a9dd5 replaces the undefined LOG_EVERY_N_SEC macro with glog LOG_EVERY_T. The original CPU logging adapter concealed this error. The production HCCL/xLLM function bodies now compile against genuine glog0.6 headers; the previous bodies fail on the undefined macro. The existing fault-injection suite and old-code red cases also pass when linked with real glog. Vendor/ACL/RPC and class definitions remain CPU boundaries; no complete native project build or NPU/device validation is claimed. Human review and DCO certification remain pending.