fix(chat): replay attached completion after close - #4204
huangruiteng merged 11 commits into
Conversation
|
Exact-head validation for
Future-facing pass: no adjacent refactor is needed; the existing store already owns durable receipt replay, so the fix only stops the outer lifecycle guard from hiding that terminal readback. |
498b91b to
35baa1f
Compare
|
Rebased onto current Post-rebase validation:
The diff remains limited to the closed-session completion replay guard and its lifecycle regression. |
Duang777
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
审查对象:35baa1fb376ac657441cd56c81afb04353e19afd
动机
这个 PR 修复的是 attached host completion 的终态重放缺口。已有公开契约把 completion receipt 定义为 duplicate-safe,但旧路径在 Session 关闭后会先由 _require_attached_host() 抛出 KeyError,导致一个已经成功持久化、只是宿主未收到确认的 completion 无法安全重试。独立基线/HEAD 对照确认:基线在“完成 → 关闭 → 使用相同 completion_id 重试”时返回 missing-session;本 PR 返回 created=false,且只保留原有一条 Agent 消息。范围明确不包括重新打开 Session、接收新 claim、改变持久化格式或扩大运行时所有权。
改动思路
方案保留现有职责边界:_require_attached_host() 仍负责 Session 存在性、attached mode 与 host identity 校验;只有 complete_attached_agent_turn() 显式传入 allow_closed=True。真正的终态幂等判断继续由 ChatSessionStore.complete_attached_turn() 持有,它在 turn lock 下比较持久化的 completion_id,相同 receipt 返回既有结果,不同 receipt 失败。正向路径是 worker bridge completion → host identity 校验 → store receipt 校验 → created=false;负向路径中 claim 未选择该例外,关闭后仍在 broker 层失败,而冲突 receipt 在 store 层失败。相比在 CLI、adapter 或新 journal 中重复实现重放规则,这是一处共享入口上的最小修复。
具体改动
loopx/attached_session.py:为_require_attached_host()增加默认关闭的allow_closed参数,并只在complete_attached_agent_turn()中启用。默认值保证当前和未来未显式选择的调用方仍拒绝 closed Session。tests/test_attached_session_broker.py:扩展现有 close lifecycle 测试,覆盖关闭后新 claim 仍被拒绝、相同 completion receipt 返回created=false、Agent 消息不重复,以及不同 receipt 仍被拒绝。
关键代码讲解
_require_attached_host()(loopx/attached_session.py:178)仍执行完整的 mode/host binding 校验;新分支只改变 closed 状态是否能继续进入后续操作,没有绕过身份约束。complete_attached_agent_turn()(loopx/attached_session.py:259)是唯一 opt-in 调用方;response normalization、必填/长度校验和 store 委托均未改变。ChatSessionStore.complete_attached_turn()(loopx/chat_store.py:1125)仍是 receipt 的唯一权威:运行中的 turn 要求 active turn 与 claim 匹配;已 completing/completed 的 turn 只接受相同completion_id;写入与 finalize 仍在既有锁和原子写路径中。
对主干的风险
最强回归风险是 allow_closed 被误用于 claim,或关闭后不同 receipt 被当成合法重放。当前 caller inventory 显示只有 claim 和 completion 两个 helper caller:claim 保持默认 false,completion 才显式启用;新增测试同时锁住这两个失败路径。独立文件存储对照也验证 HEAD 上 closed claim 为 KeyError、冲突 receipt 为 ValueError、相同 receipt 不产生第二条消息。
独立验证结果:4 个相关测试文件共 49 passed;Ruff 通过;git diff --check 通过;远端 19 个检查均成功。PR 当前落后 main 5 个提交,但从其 base 到最新 main,相关变动仅在 loopx/chat_runtime.py,未修改本 PR 的 loopx/attached_session.py、loopx/chat_store.py 或测试文件,因此这是合并策略/新 CI 新鲜度事项,不构成本次 exact-head 正确性阻断。剩余非阻断风险:即使是 receipt replay,response 仍会在 store lookup 前做语法和内容校验;普通重试会重发同一个有效请求,已覆盖目标场景。
我的整体评价
未发现阻断问题。改动只有 4 行生产代码和 36 行针对性测试,没有新增状态、协议、CLI 或迁移成本,并复用了已有的 receipt 权威与锁语义。它恢复了已声明的 duplicate-safe 行为,同时保持关闭后的新工作 fail-closed;从收益、风险和维护面看是比例合适的最小修复。结论:APPROVE。由于我是该 PR 作者,GitHub 不允许正式 self-approval,因此以 COMMENTED review 记录独立技术结论;code-owner approval 与合并门禁仍需有写权限的 reviewer 完成。
English verdict: APPROVE for exact head 35baa1fb376ac657441cd56c81afb04353e19afd; no blocking finding. Independent baseline/head execution confirms post-close replay returns created=false without duplicating the Agent message, while closed claims and conflicting receipts still fail; 49 focused tests, Ruff, diff-check, and all 19 remote checks passed. Author-owned PR, so this is recorded as COMMENTED rather than formal self-approval.
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本 PR 修复关闭会话后的幂等完成回放:调用方已经成功提交 attached turn completion,但响应丢失;会话随后关闭时,使用同一 completion id 的安全重试不应因为 host 已关闭而先于 receipt readback 被拒绝。受影响的是 attached host 的控制器恢复路径;旧行为会把已经提交的成功结果表现成不可恢复失败。最小修复是在既有 host guard 增加一个默认关闭、仅由 completion 路径启用的 allow_closed,继续让 ChatSessionStore.complete_attached_turn 在 Turn 锁内掌管 receipt 身份与幂等冲突,而不是在外层复制 receipt 规则。非目标是允许关闭后首次完成、放宽 active claim 所有权或重新打开会话。
改动思路
入口仍是 complete_attached_agent_turn。它调用 _require_attached_host(..., allow_closed=True) 以允许请求到达既有 store;其他 host 操作继续使用默认 False,所以关闭态仍被拒绝。store 随后按 completion id 做权威判断:同 id 已有 receipt 时回放,异 id 冲突时拒绝;运行中的 Turn 仍要求 active claim 属于当前 attached host。由于 close 本身会拒绝 active/queued work,这条新路径只恢复“完成已经提交、响应丢失、之后关闭、同意图重试”的状态,不创造关闭后的首次完成能力。
具体改动
exact head 为 c2fe075903cd807e3827501d2a45997170ea8cd9,只修改 2 个文件,约 +40/-1。生产改动是一处 guard 参数和一处调用点;其余增量是聚焦回归。
关键代码讲解
_require_attached_host新增allow_closed=False,默认行为完全保持。complete_attached_agent_turn是唯一传入True的生产调用者,把关闭态请求送入 receipt owner,而不是在 host guard 处提前丢失幂等语义。ChatSessionStore.complete_attached_turn未被复制或旁路:同 id 回放、异 id 拒绝和 claim 所有权仍在 Turn 锁内判定。- 回归覆盖关闭后同 id 只产生一条消息、关闭态 claim 拒绝以及冲突 receipt 拒绝。
对主干的风险
行为本身范围很窄;我运行了 16 个聚焦测试、Ruff 和 git diff --check,均通过,且与当前 main 的 clean merge-tree 成功。主要语义风险是把 allow_closed 扩散给其他 host 操作或允许关闭后首次完成;调用者扫描与 store/close 前置条件表明当前 head 没有这两种扩散。但当前 exact head 仍有一个仓库级硬门禁失败:DCO Sign-off 检查为红色,日志明确指出 merge commit c2fe075903cd807e3827501d2a45997170ea8cd9 缺少 Signed-off-by trailer。作者的行为提交虽已签名,当前 PR head 的 merge commit 仍是 PR 范围的一部分,因此不能在这个 head 上批准。
我的整体评价
实现选择正确、可逆且复用了现有 receipt authority;我没有发现额外语义阻断。未来向 refactor pass 也不需要扩大范围:这里的默认参数正好把例外限制在一个调用点。当前唯一阻断是 exact-head DCO。请 rebase 或重建/补签该 merge commit,确保所有 PR-range commit 都带 sign-off,再让最终 checks 在新 head 上完成;head 改变后需要重新核对,而不能继承本次结论。
English verdict: REQUEST_CHANGES at exact head c2fe075903cd807e3827501d2a45997170ea8cd9; the idempotent closed-session replay design is sound and 16 focused tests plus Ruff/diff checks pass, but the current head merge commit lacks the required DCO sign-off.
f5f52f1 to
f71e7ae
Compare
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
f71e7ae to
cef5f24
Compare
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com> (cherry picked from commit 3120939) Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
…mpletion-replay Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
…mpletion-replay Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com> # Conflicts: # examples/personal-workspace-browser-smoke.mjs
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
…mpletion-replay Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
@huangruiteng Current exact head 181d9d0 is synchronized with main 3aa05cc. The unsigned-history blocker was removed by rebasing the business change and all subsequent integration commits are signed. Local focused validation: 16 attached-session tests passed; full mypy, Ruff, and diff checks passed. Current GitHub checks are all green, including merge-gate, both Stage2C E2E jobs, mutants, four pytest shards, Windows, and Node compatibility. Please re-review this exact head and clear the stale CHANGES_REQUESTED state if no new finding remains. |
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion
审阅绑定 exact head:181d9d036e32c58d658eed3d85a5658d9baee259。结论:REQUEST_CHANGES。
[P1] 请 rebase 到最新 main,并删除两个已被主干取代的 shared-file carryover。 当前 GitHub mergeStateStatus=DIRTY;我对 origin/main@84334f794126735a83a6cfb77882dd58ca8066d9 执行真实 git merge-tree --write-tree,在 examples/personal-workspace-browser-smoke.mjs 与 loopx/control_plane/todos/decision_scope.py 稳定产生冲突。前者已在主干拆为模块化场景,后者已由主干补成带 schema/shape 校验的 typed projection;如果冲突时取 PR 一侧,会回退与本修复无关的广泛验证和控制面 hardening。最小修复是保留主干这两个文件,只留下 attached_session.py 与 focused broker test 的业务差异,再跑生命周期负例和完整 checks。
动机
本 PR 要恢复 attached host 已承诺的 duplicate-safe completion:Turn 已经成功持久化,但调用方丢失响应;Session 随后关闭时,同一 completion_id 的安全重试不应在读取 receipt 前被统一的 closed-session guard 拒绝。旧行为把一个已经提交的成功结果表现成 missing Session,让宿主无法安全区分“未写入”和“写入但响应丢失”。范围明确不包含关闭后的首次完成、新 claim、Session reopen、lease/reclaim、持久化格式或 host authority 扩张。
改动思路
入口仍是 complete_attached_agent_turn。它唯一显式调用 _require_attached_host(..., allow_closed=True);claim 等其它调用者继续使用默认 False,所以 closed Session 仍拒绝新工作。真正的幂等权威继续由 ChatSessionStore.complete_attached_turn 持有:同一 receipt 返回已有结果,不同 receipt 失败,running Turn 仍必须匹配 active turn 与 claim。这个边界比在 broker/CLI 新增 receipt lookup 更小,也避免复制锁、消息追加和 finalize 语义。
我用同一份 file-backed fixture 分别跑了 baseline 3aa05cc256b1d77e59576dc3e943f8d7e626afe3 与 exact head:baseline 的 close 后同 receipt 返回 KeyError;head 返回 created=false 且只有一条 Agent message。关闭后 claim 仍是 KeyError,不同 receipt 在 head 到达 store 后按契约返回 ValueError。
具体改动
当前 GitHub diff 是 4 文件、+61/-12。真正业务变更是 loopx/attached_session.py 的 4 行和 tests/test_attached_session_broker.py 的 37 行;另外两份 shared-file 差异来自旧 integration head,现已被主干版本取代。
关键代码讲解
_require_attached_host(loopx/attached_session.py:178):新增默认关闭的allow_closed,但 mode 与 exact host binding 校验始终执行。complete_attached_agent_turn(loopx/attached_session.py:259):唯一选择 closed terminal readback 的调用者;response normalization 与 store 委托未复制 receipt 规则。ChatSessionStore.complete_attached_turn(loopx/chat_store.py:1126,未改):仍在 Turn lock 下拥有 receipt/claim/active-turn 判断、原子 reservation 与 idempotent message/event finalize。- focused close lifecycle test:覆盖同 receipt replay、不重复 message、关闭后新 claim 拒绝及不同 receipt 拒绝。
对主干的风险
业务语义本身没有发现新 blocker:tests/test_attached_session_broker.py 共 16 个测试通过;changed-file Ruff 与 git diff --check 通过;远端 23 个 checks 全部成功或预期 skip,DCO 也已修复。最强风险不在 completion 逻辑,而在 final integration:这些 checks 对应旧 base,无法证明当前主干合并结果;现在 merge-tree 已明确冲突。
错误地选 PR 一侧会把约 3,000 行旧单体 browser smoke 重新带回,并丢掉主干最新的 decision-scope result schema/shape 校验。正确 rebase 后,要求 merge-tree clean、最终 diff 只保留业务两文件,并重跑 complete → close → same receipt、closed claim、different receipt 三条路径。
我的整体评价
closed completion replay 的实现选择正确、可逆、比例合适,也复用了已有 receipt authority;不需要再抽象新 journal 或兼容层。此前 DCO blocker 已解决,本轮没有发现业务语义回归。但 exact head 已无法安全进入当前 main,因此不能继承旧 approval 或凭绿色 stale-head checks 批准。完成 rebase、保留主干 shared files 并刷新验证后可做窄复审。
Future-facing pass:当前最有价值的简化就是去掉两份无关 carryover;attached store/broker 边界本身无需扩大重构。
English verdict: REQUEST_CHANGES on exact head 181d9d036e32c58d658eed3d85a5658d9baee259. The post-close exact-receipt replay is semantically sound: baseline/head execution proves created=false with one message while closed claims and conflicting receipts still fail, and 16 focused tests plus all 23 remote checks pass. However, latest main now conflicts in the old monolithic browser smoke and decision-scope adapter; rebase, keep main's superseding implementations, retain only the attached-session fix/tests, and rerun the final-tree checks.
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
@huangruiteng Updated exact head 305a841 onto current main 787e8dd. Both conflicts were resolved by retaining the superseding main versions of examples/personal-workspace-browser-smoke.mjs and loopx/control_plane/todos/decision_scope.py; the final base-to-head diff is limited to loopx/attached_session.py and tests/test_attached_session_broker.py. Local validation: 16 broker tests passed, scoped Ruff passed, full mypy passed, and git diff --check passed. Fresh exact-head CI is running; please re-review this head after it completes. |
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
@huangruiteng Updated exact head 21869e2 onto current main bb0ba5b. The merge was clean and the final diff remains limited to loopx/attached_session.py plus its broker regression test. Local validation: 16 tests, scoped Ruff, full mypy, and git diff --check passed. Main now contains the UTC-without-tzdata fix, so fresh CI should clear the prior Windows baseline failure. Please re-review this exact head after CI completes. |
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
@huangruiteng Main advanced by docs-only #4265 while the previous exact-head CI completed. Current exact head eac5950 now contains latest main 481497e; the merge was clean and the 2-file business diff is unchanged. The prior exact head completed 22/22 checks successfully, including Windows and merge-gate; fresh CI is running. |
huangruiteng
left a comment
There was a problem hiding this comment.
English verdict: APPROVE. Exact head: eac5950. No blocking finding; attached completion replay remains closed-session safe and receipt-idempotent.
动机
attached host 的网络重试可能发生在 Session 已关闭之后;原实现把已完成 receipt 与“新 claim”一并拒绝,导致响应不可读回。
改动思路
只在 completion 入口允许 matching attached host 访问 closed Session;真正的写入/去重仍由 ChatSessionStore 的 turn lock、completion_id 和 finalize 路径负责。close 仍拒绝 active/queued work,因此没有重新打开 Session 或放宽 claim。
具体改动
_require_attached_host 增加 allow_closed,complete path 传 true;complete_attached_turn 对 completing/completed 的同 receipt 返回 duplicate-safe replay,对不同 receipt 抛 ValueError。新增测试覆盖 close 后新 claim 失败、同 receipt 不重复消息、冲突 receipt 拒绝。
对主干的风险
正向路径和负向路径均通过:16 个 attached-session broker tests 在 exact head 与 origin/main 都通过,远程 25 项 checks 全绿。close 仍以 turn terminal/queue drained 为前提,host identity 仍校验。未发现阻塞 finding;剩余风险是更高并发/跨进程压力未在本地 smoke 中覆盖。
我的整体评价
APPROVE。exact head eac59503e1a91848b89418e76cf5d57f3574b9bf 已复核;改动小、复用既有 durable receipt owner,未引入新 authority 或 actor lifecycle。
Summary
Root cause
The outer attached-host guard rejected every closed Session before
ChatSessionStore.complete_attached_turn()could replay an already committed completion receipt. A caller that lost the first response could therefore not recover its durable result after a concurrent close.Validation
python -m pytest -qacross 15 Chat/attached suites — 139 passedloopx canary premerge --from-git-diff --git-diff-base origin/main --tier standard— passedgit diff --check— passedScope
This only permits exact terminal completion replay on a closed attached Session. It does not reopen the Session, admit new claims, accept a first completion after close, add lease/reclaim behavior, or change attached runtime ownership.