Skip to content

[Bugfix] Publish HCCL completion after stream retirement #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
THU-syh wants to merge 2 commits into
base: v0.3.14_xllm_patch
Choose a base branch
from
Open

[Bugfix] Publish HCCL completion after stream retirement #10

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9260034
bugfix: publish hccl completion after stream retirement.
THU-syh Nov 12, 2026
50a9dd5
bugfix: use glog time throttling for retirement.
THU-syh Nov 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions mooncake-transfer-engine/src/transport/ascend_transport/hccl_transport/hccl_transport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ void HcclTransport::initiatorLoop(int deviceLogicId, int selfIdx) {
<< ", source_addr: " << slice->source_addr
<< ", dest_addr: " << slice->hccl.dest_addr
<< ", ret: " << ret;
slice->markFailed();
// 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;

@DragonFive DragonFive Nov 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 轮成功」断言最终状态。

}
}

Expand All @@ -146,7 +149,7 @@ void HcclTransport::initiatorLoop(int deviceLogicId, int selfIdx) {
<< ", remote devicePhyId: " << remote_rank_info_.devicePhyId
<< ", ret: " << ret;
for (auto slice : slice_list) {
slice->markFailed();
slice->status = Slice::FAILED;
}
}
auto addOpfence = std::chrono::high_resolution_clock::now();
Expand All @@ -159,9 +162,21 @@ void HcclTransport::initiatorLoop(int deviceLogicId, int selfIdx) {
<< local_rank_info_.devicePhyId
<< ", remote devicePhyId: " << remote_rank_info_.devicePhyId
<< ", ret: " << ret;
aclrtStreamAbort(stream);
const int abort_ret = aclrtStreamAbort(stream);
if (abort_ret) {
LOG(ERROR) << "aclrtStreamAbort failed, ret: " << abort_ret;
}
// Synchronize to drop ACL_ERROR_RT_STREAM_ABORT error
aclrtSynchronizeStream(stream);
int drain_ret = aclrtSynchronizeStream(stream);
while (drain_ret != ACL_SUCCESS) {

@ShuweiShen772 ShuweiShen772 Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 无法看守此次发布顺序修复。

@ShuweiShen772 ShuweiShen772 Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

第二轮复核：代码未变，配套关系已变更

本轮 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。

// An error result alone cannot release borrowed buffers.

@DragonFive DragonFive Nov 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] PR10 不提供任何 retirement 可观测接口：0.3.14 消费者只能看到无区别的 WAITING；MR791 式 requires 检查对本 PR fail-closed

（对 ShuweiShen772 round-2「统一 producer/consumer 退休契约」要求的代码级机制证据补充）

证据：

  1. 本 PR 树全树 grep getBatchRetirementStatus 零命中（PR11 同线已补完整 API 链：transfer_engine.h:301 / multi_transport.cpp:745）。
  2. 消费者侧（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");
}
  1. 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 设超时后释放缓冲」。

// Keep the batch pending until the stream actually drains.
LOG_EVERY_T(ERROR, 1)

@DragonFive DragonFive Nov 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 导出路径测试。

<< "Waiting for failed HCCL stream retirement, ret: "
<< drain_ret;
std::this_thread::sleep_for(std::chrono::milliseconds(1));
drain_ret = aclrtSynchronizeStream(stream);
}

if (loop < transfer_max_retry_cnt) {
LOG(INFO) << "Hccl transport failed. Retry ...";
Expand All @@ -180,7 +195,7 @@ void HcclTransport::initiatorLoop(int deviceLogicId, int selfIdx) {
LOG(ERROR) << "Hccl transport failed after "
<< transfer_max_retry_cnt << " retries.";
for (auto slice : slice_list) {
slice->markFailed();
slice->status = Slice::FAILED;
}
}

Expand Down Expand Up @@ -221,7 +236,9 @@ void HcclTransport::initiatorLoop(int deviceLogicId, int selfIdx) {
}

for (auto slice : slice_list) {
if (slice->status != Slice::SliceStatus::FAILED) {
if (slice->status == Slice::SliceStatus::FAILED) {
slice->markFailed();

@DragonFive DragonFive Nov 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 查找）外」。

验证：纯静态走查即可关闭（本评论已完成论证）。

} else {
slice->markSuccess();
}
}
Expand Down