perf(chat): wait on turn completion notifications - #4430
huangruiteng merged 1 commit into
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
详细中文评审
结论:APPROVE。评审 head 105be9fbb05543e101ff8433e7b7a694cad24a9b(对照 base main)。无阻塞发现。
动机
wait_for_turn 原本是 while time.monotonic() < deadline: 每轮调一次 store.load_turn 再 sleep(0.02)。ChatSessionStore.load_turn 就是 _read_json(turn 文件),所以等待一个默认 920 秒的长 Turn,最多会产生约 4.6 万次磁盘读加 JSON 解析;受影响的是两个真实调用点:Chat HTTP 等待路径 chat_server.py:709 与管家 Lark 通道 goal_topic_runtime.py:886。作者用 0.5 秒真实 store 探针测到 main 上 23 次 turn 文件读、本分支 3 次。第二个问题是正确性:while 条件在 sleep 之后不再读状态,若 Turn 在最后一次 sleep 窗口内完成,循环直接退出并抛 TimeoutError,调用方会把一次已成功的 Turn 报成失败(424 或 LarkGoalTopicTurnFailed)。更小的修法是把最后一次读取挪到 deadline 判定之前,那能修掉误报但完全不动 IO 成本;本 PR 同时接上既有完成 Event,是在不新增同步 owner 的前提下把两个问题一并解决。
改动思路
入口是 ChatRuntimeController.wait_for_turn,权威状态仍是 ChatSessionStore 里的 Turn JSON;turn_done_events[(session_id, turn_id)] 只是完成通知,不参与状态判定,决策 owner 没有转移。正路径:Turn 开始时(start_turn 的 :824、队列 worker 的 :1039)在 self.lock 下注册一个 threading.Event 且先于执行体启动;wait_for_turn 先读一次状态,若未终止则取出 Event 并 wait(remaining);_run_turn 的 finally(:1263)把 Event 从映射中 pop 出来再 set(),等待方醒来后重新读状态并返回 completed。关键不变量是 set 与 pop 成对、且发生在终态写入之后,因此不存在「Event 已 set 但状态仍非终态」的驻留项,也就不可能空转;这一点我是按 rg -n "turn_done_events" 逐个核对注册/移除点的。既有实现对比:controller 早就用这套 Event 服务于 close()(:1485)的排空等待,本 PR 只是让等待方复用它,因此没有引入第二个同步权威、新依赖或新配置。所有权取舍合理:状态权威留在 store,通知权威留在 chat_runtime 的 Turn 生命周期,二者没有互相复制规则。
具体改动
分类:loopx/chat_runtime.py 生产代码 +7/-5,tests/test_chat_turn_wait.py 新增 37 行;无 docs、generated 或机械搬移。
关键代码讲解
ChatRuntimeController.wait_for_turn(chat_runtime.py:1399) —— 由while monotonic() < deadline改为while True:先load_turn,命中TERMINAL_TURN_STATES立即返回,未终止才计算remaining,remaining <= 0抛TimeoutError。有 Event 时done_event.wait(remaining),否则sleep(min(0.02, remaining))。关键分支是终止判定必须先于 deadline 判定,否则同一轮里完成与超时仍会竞态;同时min(0.02, remaining)保证最后一次 sleep 不会越过 deadline。_run_turn的finally(chat_runtime.py:1256 起) —— 未改动,但它是等待方依赖的完成信号源:在锁内pop掉 Event 后set(),等待方由此唤醒并重新读状态。interrupted、failed、启动失败(:1072)等分支同样走这条路,所以唤醒条件覆盖全部终态。- Event 注册点(chat_runtime.py:823 与 :1039) —— 直接启动与队列启动两条路径都在执行体启动前注册,保证等待方不会漏掉尚未开始的 Turn;
close()仍以 0.5 秒上限等待这些 Event,行为与 main 一致。
对主干的风险
无阻塞风险。逐项核对:failure_analysis —— 最强回归场景是「deadline 剩余不足一次 sleep 且 Turn 恰在该窗口完成」,触发状态明确,修复点是读取顺序,最小修复就是把终态读取提到超时判定之前,回归用例是 test_wait_for_turn_performs_final_fallback_read_at_deadline;爆炸半径是两个生产等待点,症状是把成功 Turn 报成失败。负路径 —— 领取失败路径(:1072)会 pop+set 却不写终态,此时等待方拿到的是已移除的 Event,退化为 20ms 有界轮询,仍受 deadline 约束,不会空转。validation_matrix —— 两个聚焦用例覆盖 Event 正路径与边界路径,远端必需门禁(Sign-off、merge-gate、pytest、test-shard 1-4、dashboard-acceptance、stage2c、node-*、kernel-static-checks)在 exact head 全绿。scope_fit —— 生产调用点已点名为 chat_server.py:709 与 goal_topic_runtime.py:886,不是仅覆盖率边界。change_proportionality —— 问题每次等待都发生,而机制只有 12 行生产改动且复用既有 Event,属正向且相称。typed_state_rule —— 终态判定仍只有 chat_store.py:28 的 TERMINAL_TURN_STATES 一个来源,未复制字面量。authority_semantics —— 无新增公共协议名,未授予任何权限,wait_for_turn 名称与「只等待」的范围一致。domain_neutrality / guidance_vs_obligation / default_off_isolation / behavior_change_disclosure —— 未新增错误文本、义务或开关,行为变化已在 PR 描述中披露,未改对外协议,因此无需迁移或 release note。
我的整体评价
base/head 对照显示五条可观测语义等价、两条为已验证的有意变更:completed Turn 返回该 Turn、从未完成的 Turn 仍超时、无内存 Event 仍 20ms 轮询、Turn 不存在仍 KeyError、读次数 23→3、deadline 边界由 TimeoutError 变为返回 terminal Turn。这些恰好就是作者声明的全部改动面,没有发现额外漂移。残余风险有两点且都非阻塞:新增用例用 Mock store 与 Mock Event,属单元边界,作者的真实 store 探针没有固化成可重跑 smoke;真实 Lark 网络往返下的端到端等待耗时未测量。若要再提高证据强度,建议后续把那条 0.5 秒读次数探针做成 examples/ 下的确定性 smoke,这样读次数回退会被持续守住。
English verdict: APPROVE at exact head 105be9fbb05543e101ff8433e7b7a694cad24a9b. No blocking finding. The change replaces a 20 ms load_turn polling loop (a real disk read plus JSON parse per iteration, up to ~46,000 per 920 s wait, and the same code serves the steward Lark path at goal_topic_runtime.py:886) with a wait on the controller's existing per-Turn completion Event, while keeping bounded polling for attached/restarted Turns and moving the terminal-state read ahead of the deadline check so a Turn completing inside the final sleep window no longer raises TimeoutError. The reused notification contract is sound: the Event is registered before the runner starts and is always pop+set in _run_turn's finally after the terminal state is persisted, so there is no lingering set Event and no spin. Verified: both focused regressions (one red on main), full Chat suite 221 passed, ruff, maintainability ratchet, and every required remote check green at this head. Residual, non-blocking: the new tests are mock-level, the author's real-store read-count probe is not retained as a re-runnable smoke, and end-to-end wait latency over live Lark is unmeasured.
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
105be9f to
2ea6a16
Compare
|
合并后审计(exact head 本 PR 已合并。合并后的提交与我此前审批的内容在语义上等价,我按审计要求在新 head 上复核了一遍,没有新的可执行发现,因此这里只做结论回读,不重复先前的完整评审。 复核要点:
残余(非阻塞,与先前评审一致):新测试为 mock 级别;作者的真实 store 读次数探针没有作为可复跑 smoke 保留;真实飞书链路上的端到端等待延迟未测量。 |
Summary
Evidence
A 0.5-second real-store probe performed 23 turn-file reads on main and 3 on this branch, including the completion writer read: an 87% reduction. A deterministic boundary probe also showed main raising TimeoutError after the Turn completed before the deadline because the polling sleep crossed the deadline without a final read.
Validation
Scope
This reuses the controller-owned completion Event. No new synchronization owner, dependency, configuration, or public API is introduced.