Repository navigation
[Bugfix] Preserve deferred batch cleanup ownership after query errors - #11
ShuweiShen772 wants to merge 7 commits into
Conversation
Retain library-owned deferred descriptors and delete hooks on query errors. Back off recoverable queries and use sealed completion publication to drain errors safely after callback pins reach zero. Add focused ownership, persistent-error, pin, and shutdown regression coverage. Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
独立复核：延迟回收所有权修复成立，不能替代另一组生命周期修复
评审 head 20dfd86858e5，base ff3258bbd305，检查全部 4 个改动文件，并沿 freeBatchID → tryFreeBatchID → deferredCleanupLoop → Slice::finishIfComplete 核对。评论由 Codex 辅助审查。
判干净
multi_transport.cpp:259–275：查询错误后保留 descriptor 的唯一回收记录和before_delete，不再像旧版一样 erase；初次普通错误仍不转移调用方所有权。:180–188,213–219：错误后的 completion fallback 没有跳过“全部 task + callback pin”条件；永久 InvalidArgument/NotImplemented 仅停止 transport polling，并非直接删除 batch。:269–277：查询重试有退避，只有错误变化才记录日志。
本轮实际运行
在无设备、无网络的隔离 CPU 容器中，使用生产 reaper/allocator 函数体、实际 Slice/Task 布局、真实 glog/GTest 和本 PR 原始测试；transport 委托及关闭的 metrics 是边界：
- 本 PR：EDC 关闭 8/8，开启 8/8；
- 准确旧基线 reaper：两种配置各 7/8 失败，仅 InitialQueryErrorDoesNotTransferOwnership 通过。
这独立验证了本 PR 的目标缺陷确实被覆盖，不是整库 native 链接或新的 NPU 测试。作者披露的双 NPU AscendDirect 结果也不能替代 #10 的 HCCL 失败退休验证。
合入判定
库侧这 4 个文件本轮未发现新增确定性代码缺陷。行级评论 :181 说明组合前置：#10 修 terminal publication、!791 保住借用数组/页面、!800 保住句柄回收责任，各自不能替代。
库侧修复逻辑认可；xLLM 恢复 0.3.14 尚不具备单项放行条件。 等 #10/#11 进入同一个官方可获取版本后统一 pin，并验证两个 xLLM MR 的最终合并结果；不能只凭本 PR 的回收测试宣布页面生命周期安全。
| // publication can substitute for polling. CompletionCallbackPin still | ||
| // protects callback accesses until the batch-wide check below. | ||
| if (allow_completion_fallback && | ||
| __atomic_load_n(&task.completion_published, __ATOMIC_ACQUIRE)) { |
There was a problem hiding this comment.
【代码复核 / 跨 PR 前置】这条 fallback 保住了回收责任，但不能单独证明 HCCL I/O 已退休，也不持有调用方的请求数组和 KV 页
本轮用当前 reaper 生产函数体、实际 TransferTask/Slice 头和本 PR 的原始 8 条测试做 CPU 复核：EDC 开/关均 8/8 通过；换成准确 ff3258bbd305 的旧 reaper 后，两种配置均为 7 条失败、初次普通查询错误仍归调用方那条通过。transport 查询委托和关闭的 metrics 是测试边界，并非整库 native 构建或设备测试。
当前实现并没有把查询错误当成完成：先保留 entry/hook，只有 completion_published 或已有 is_finished 才前进；全部 task 完成之后仍检查 active_completion_callbacks==0（213–219 行）。这部分本轮没有发现新的确定性缺陷。
但固定基线的旧 HCCL 会在 stream 同步前 markFailed()，它也可能提前置位这个 publication marker。因而重升级组合必须同时包含 #10 的晚发布修复；同时，xLLM !800 的 BatchCleanupDeferred 后返回并不能保住局部 request vector 或上层 KV 页面，仍需 !791 的 drain-before-return。
建议明确写入组合合入条件：#10+#11 的统一官方依赖 + !791 的排空语义 + !800 的句柄回收责任。库侧 descriptor 所有权和业务页面生命周期是两个不同不变量，不能用本 PR 的 8 条回收测试替代页面复用验证。
There was a problem hiding this comment.
Addressed in 6107763; the original retained-reaper ownership fix remains unchanged.
The classic SDK now exposes getBatchRetirementStatus(batch_id, retired), preserving caller ownership while every task and completion callback drains. Successful terminal query observations are tracked separately from legacy is_finished flags; a query error or TIMEOUT alone cannot authorize buffer reuse. Counter transports require sealed publication. MR800 uses this API and a valid-handle guard, preserving the initial failure and borrowed arrays/pages until retirement.
The description now explicitly requires #10's late HCCL publication plus #11 in one official revision, GitCode availability, and final MR791/MR800 helper/dependency integration. The TENT interface does not advertise this new classic API.
Latest validation: 46 CPU integration cases and the same 46 with GCC12 ASan/UBSan + leak detection passed; full AscendDirect/RDMA transfer_engine build and native-linked controlled-transport suite passed 11/11. The exact v0.3.12 Status compiles/runs with the caller compatibility guard. Earlier cleanup-core NPU runs are recorded separately from this API extension.
Keep borrowed buffers live until every task and completion callback has retired. Distinguish successful terminal queries from legacy finished flags, and preserve completion notification behavior. Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
第二轮独立复核（head 6107763e5b0f，base ff3258bbd305）
重新检查全部 8 个改动文件。本轮新增的公开退休 API 已把业务错误与资源可回收性分开，并消除 !800 从跨库私有布局推导生命周期的需求。
上轮整改确认
- entry/hook 在查询错误后继续保留；caller-owned 句柄不由退休观察失效。
- counter transport 必须有 sealed
completion_published；poll-driven transport 使用实际成功 terminal query 的retirement_observed，不把遗留 is_finished 当作查询错误后的证明。 - 全 task 完成仍要 callback pin 归零。状态查询错误/FAILED/TIMEOUT 不能自行授权复用业务页。
- #10 的 producer 晚发布仍是 0.3.14 HCCL 的必要前置；header 对 USE_TENT 构建不公布本能力是当前兼容范围限制，不能当作 TENT 已有安全退休 API。
本轮实际运行
原始 11 条 cleanup/retirement 用例：EDC 关闭 11/11、开启 11/11；使用实际生产 reaper、getTransferStatus/aggregate/retirement 函数体及 Slice/Task，配置和 metadata/metrics 为受控 CPU 边界，真实 glog/GTest。非新的设备测试，也不冒用作者声称的整库/ASan 结果。
一条新问题（P2，行评 transfer_engine_impl.h:352）
新 wrapper 重复观察已退休成功 batch 时重复累计整批成功字节；!800 的 cleanup 合法重试会重复调用。实际 wrapper 函数体的 WITH_METRICS 回归：64 字节两次观察记成 128，期望 64 的断言失败。请补幂等记账/回归。
判定
With fixes：所有权与退休方向认可，字节计数问题应修。 !791 已换成 0.3.12/PR12，不再是相同 SDK 构建；最终先选择版本路线，再统一 helper 和官方 GitCode pin，不能用“API 同名”推定两份 SDK 已能混用。
Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
| } | ||
|
|
||
| Status MultiTransport::getBatchRetirementStatus(BatchID batch_id, | ||
| bool& retired) { |
There was a problem hiding this comment.
[P2] 新公开 API 对「已 deferred（reaper 所有）」句柄零防护：free 返回 BatchCleanupDeferred 后继续查询，与 reaper 的 delete 竞争即 use-after-free
贴码（multi_transport.cpp:745-748，API 入口直接解引用裸指针 BatchID）：
Status MultiTransport::getBatchRetirementStatus(BatchID batch_id,
bool& retired) {
retired = false;
auto& batch = *((BatchDesc*)(batch_id)); // 无 deferred_cleanup_mutex_、无 map 检查对照 reaper 侧删除点（tryFreeBatchID 尾部，reaper 持 deferred_cleanup_mutex_ 调用，multi_transport.cpp:220-223）：
if (before_delete) { before_delete(); }
delete &batch_desc;而所有权契约写在 multi_transport.h:59-61：// An OK or BatchCleanupDeferred return invalidates batch_id.
触发条件：caller 调 freeBatchID 得到 BatchCleanupDeferred（句柄已失效、reaper 接管），但把「任何 free 错误」当「稍后重试」处理（未检查 IsBatchCleanupDeferred()，status.h:106 提供该判别），随后对同一指针调 getBatchRetirementStatus。若 reaper 已完成清理并 erase+delete，查询即解引用已释放内存；若 entry 仍在 map，则与 reaper 的并发轮询在同批 task 字段上竞争（本 API 不持任何锁）。
影响：内存破坏。需要调用方违反契约才触发，但本 API 的目标用户恰是「错误路径重试型」调用方（transfer_engine.h:293-301 文档自述的场景）——最容易踩进这个窗口的人群。
建议（最小）：API 入口加 std::lock_guard<std::mutex> guard(deferred_cleanup_mutex_); if (deferred_cleanup_batches_.count(batch_id) != 0) return Status::InvalidArgument("batch cleanup has been deferred; handle is invalid");——reaper 全程持同一 mutex，检查无竞争。代价：查询方可能阻塞至一轮 reaper 轮询结束（错误路径 API 可接受）。
验证：新增测试——free→deferred→立即 getBatchRetirementStatus，断言返回干净错误且无 ASan 报告。
There was a problem hiding this comment.
这里的触发是对已失效句柄继续调用，违反既有 API 契约。未采用 proposed map-check：reaper已经delete/erase时map为空，地址复用时甚至指向另一对象；加锁和count!=0只能挡住部分窗口，不能使stale ID安全。已在公开文档明确OK/deferred后禁止free/query、API不校验过期pointer-derived ID；监控改用无BatchID的snapshot。xllm guard在deferred后失效句柄并不再查询/释放，回归覆盖该纪律。完整防误用需generation handle/lifetime registry，属于另一个API设计。请复核这一契约边界。
修订：ab4cc79e159217acb693f7c3d055893ca687b446；本轮59项CPU及同组ASan/UBSan/LSan通过。
Review 总结：所有权修复与 retirement API 核心语义成立（With fixes，无 P0/P1）独立复核了全部 8 个改动文件（head 6107763，base ff3258b，物化树逐行核验）。 正面核验（已证实）：
Findings（7×P2 + 2×Q，详见 inline）：
与 ShuweiShen772 三条既有评论的关系：①fallback 保住回收责任但不能单独证明 HCCL 退休、②API 已加、③impl 层 metrics 重复累计——均不重发；以上为增量（P2-3/P2-4 与③同域不同层次，已显式区分）。 合入建议：With fixes——P2-2（deferred 句柄防护或契约注释）与 P2-6（TIMEOUT 可判别）建议合入前处理；其余可随后。建议修改顺序：P2-2 → P2-6 → P2-5 → 其余。 （审查方法：物化 base/head 树静态对照 + PR12 树跨线核验；作者口径验证声明（46 用例、ASan/UBSan、两 NPU 131072 tasks）均未本地复现，物化计数为 11 个 gtest 入口。） |
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
第三轮独立复核（head b2fccb83c335，当前 target base 57a42b2afcba）
重新取得全部 9 个改动文件的实际内容并核对 Git blob SHA，与上一轮 6107763e5b0f 比较；本轮集中审查共享字节 claim、新 wrapper 回归及退休/清理调用链。Codex 辅助审查。
已修复与判干净
- 上轮 P2 重复退休观察的字节累计已修复：
transfer_engine_impl.h:317–330,547–553使用原子 per-task claim，成功 task/batch/retirement 查询共享一次字节发布；没有另建 BatchID 历史表。 - 新的 claim 不替代既有 latency 标志；混合查询后仍能各自按对应生命周期发布。
- 退休 API 保持“错误 Status 与 retired 分离”；query error/TIMEOUT 不被当成页面可复用，所有 task 加 callback pin 的门槛未被放宽。
- deferred entry/hook 的保留、普通首次错误仍归调用方的所有权约定未被指标修订破坏。
本轮实际测试
- 原始 cleanup/retirement 11项×EDC两种模式=22项通过。
- 新增两条原始 wrapper 指标测试 2项×EDC两种模式=4项通过。换回准确旧 wrapper 方法体的负对照，两种模式各2项失败，包含明确64→128。
- 配套 !800 原始 guard/helper 10/10通过。当前 helper cleanup 重试只观察退休一次；旧 helper 同断言查询2次失败，新 helper查询1次通过。
- 当前 classic
transfer_engine.cpp实际 wrapper TU 语法编译通过，使用真实 TransferEngineImpl 声明，未替换这个类定义。它不是整库链接或 USE_ASCEND/HCCL 设备构建。
合计本轮两个 PR 的原始 CPU 用例 36个通过，另有明确负对照。wrapper 方法体/实际Task/MultiTransport参与，配置、wrapper构造、metric/metadata等仍有受控边界；没有把作者的50项ASan/NPU数据冒充为本轮运行，也未执行新的设备故障测试。
尚存的合入前置（非新增源码缺陷）
当前 head 的 HCCL producer 仍在 stream 同步前调用 markFailed；本 API 的退休证明仍须和 #10 的晚发布一起进入官方 revision。当前 !800 gitlink cc67eda04bfe 的公开 TransferEngine 头也确实没有退休 API，所以“编译/安全拒绝”并不等于 PD 已可用。
作者已明确采用 0.3.14 classic 的 #10+#11 路线；这解决了上轮未确定路线的问题，不再机械要求和0.3.12的!791/PR12混用。仍需统一官方SDK、GitCode同步和 pin更新、fmt/YLT完整构建、最终 merge result 及真实HCCL/NPU验收。
判定：上一轮 P2 已修，本轮未发现新增确定性源码缺陷。就已审代码方向认可；组合部署/SDK门槛尚未完成，不作生产放行或 GitHub Approve。
Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
|
已将 xLLM !810 指出的成功 deferred 误报纳入 #11 / !800 联合路线。库侧提交 65项受控CPU及同组ASan/UBSan/LSan通过，两代实际Status兼容探针通过。deferred事件累计与实时reaper backlog分别观测。官方统一revision、镜像pin及真实HCCL/NPU合入门槛维持。 |
ShuweiShen772
left a comment
ShuweiShen772
left a comment
There was a problem hiding this comment.
第四轮独立复核（head a9e588be0035，base 57a42b2afcba）
重新拉取并检查全部 9 个改动文件，与上一轮 b2fccb83c335 对比；重点复核新增 backlog 观测、TIMEOUT 区分、退休发布排序、is_finished release 语义及 !810 deferred-cleanup 容忍路径。Codex 辅助审查。
上轮问题与新增逻辑
- 上轮重复退休观察的字节计数 P2 仍保持修复；per-task claim 未被本轮改动破坏。
- callback pin 全部退出前不报告可复用，查询错误不会冒充退休；TIMEOUT 单独返回 Context 且不置退休证明，符合“超时不等于 I/O 停止”。
- 新增 backlog 快照在读表锁内生成，统计的是 reaper 当前实际持有的 entry，不跨 BatchID 保存历史状态；告警和 release 发布顺序没有发现新竞争。
- 成功退休后的 wrapper 仍只发布一次字节，通知仍由原 side table 清理。
新发现
- P2，
multi_transport.cpp:819：CANCELED/INVALID是 NVMeoF 已声明的终态失败，但退休 API 返回OK + retired=false。实际探针两种状态均复现，调用 helper 会进入 100us 的无界轮询并丢失失败状态。需显式映射为 Context；满足 transport 停止访问契约的 CANCELED 再进入退休判断，并补两个测试。 - P3/style，
multi_transport.cpp:304：新增告警窗口重复写60/60000，统计分支缺花括号，不符合项目强制格式规则。
本轮实际运行
无设备、无网络隔离 CPU 容器，真实 glog/GTest；生产 retirement/cleanup 方法体和实际 Task/Slice 布局参与：
- 本 PR cleanup/retirement 15 项 × EDC 关闭/开启 = 30 项通过。
- wrapper 字节幂等 2 项 × EDC 两种模式 = 4 项通过；换回旧 wrapper，两种模式各 2 项预期失败（明确复现 64→128）。
- 当前 classic
transfer_engine.cppwrapper TU 语法编译通过（真实TransferEngineImpl声明，不替换类）。 - 当前 v0.3.14 Status 与 xllm guard/helper 的 deferred 成功/原失败及旧 Status 能力拒绝探针通过。
- 新增
CANCELED/INVALID退休状态探针 失败：实际均为message=OK, retired=false，与预期非 OK 相反。 - 未执行完整 SDK 构建、真实 HCCL/NPU、ASan/LSan 或性能测试；不继承作者的 sanitizer/设备报告为本轮执行。
合入判定
With fixes：P2 需修，P3 建议顺手修。 当前 #11 与 !800 的 deferred cleanup 语义已对齐，!810 根因修复在 !800 中完整体现。仍需 #10 的 HCCL 晚发布进入同一官方 revision，并完成 GitCode/pin、实际 pin 的完整构建及真实 NPU 验收。API 文档已明确“free 返回 OK/deferred 后禁止再查询”，所以我没有把调用方故意使用失效句柄作为本 PR 新缺陷。
Signed-off-by: ShuweiShen772 <shenshuwei.3@jd.com>
Description
修复 deferred batch 后台回收在传输状态查询出错时丢失跟踪记录的问题，完善 batch 描述符、完成回调和借用传输内存的生命周期管理，并提供公开的退休查询与回收进度观测接口。
本修改适用于
v0.3.14_xllm_patch的 classic Transfer Engine 路径。主要改动
保持后台回收责任
提供公开的 batch 退休查询接口
getBatchRetirementStatus(BatchID, bool&)，在有效句柄上驱动任务查询，分别报告传输结果与资源退休状态。统一清理状态与句柄所有权约定
freeBatchID()返回值OKBatchCleanupDeferred清理状态与传输结果分别处理。成功传输可以使用 deferred cleanup；借用内存的生命周期以任务及回调全部退休为准。
完善指标与后台积压观测
getDeferredCleanupStats()，提供当前 pending batch 数、查询错误 batch 数和最老入队年龄。配套关系
HCCL 后端结合 PR #10 的 stream 退休后终态发布逻辑使用本接口。调用方在有效 batch 句柄上确认退休状态，再处理清理返回值与内存生命周期。
Suggested Merge Priority
Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
验证采用生产回收/退休方法体、实际 Task/Slice 布局及调用方 helper，I/O、metadata、metrics 后端使用受控边界。
重点覆盖：瞬时及持续查询错误、完成发布与 callback pin、成功传输的 deferred cleanup、失败后的退休回收、TIMEOUT 后续完成、重复查询指标幂等性、TIMEOUT/CANCELED/INVALID 终态处理、实时回收积压和累计 handoff 计数。
Test commands:
Checklist
Signed-off-by。AI Assistance Disclosure
Codex 辅助实现、回归验证和代码检查。