perf(chat): bound completed event retention - #4463
Conversation
fdaa08d to
44f2da6
Compare
|
CI follow-up: the prior shard 3/4 failures were both the semantic inventory freshness checks; Validation on the updated head:
|
44f2da6 to
b4416ec
Compare
|
Updated the branch through current The original shard-1 steward failure is fixed. Revalidation also exposed two independent regressions introduced by intervening main commits:
Validated locally on the current head:
The previous complete GitHub Actions run passed all 23 executed checks. A fresh run for |
b4416ec to
a295aa3
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
PR 要解决的问题是真实的:终态 Turn 的事件历史会长期留在内存缓存里,session / flush 锁按 key 累积到进程结束,重启时还要对未变化的 event 文件重新做一遍压缩。对长期运行的 chat server 来说,这三项都随会话数单调增长,body 里给出的量级(1000 个已完成 Turn:2.712s / 1000 cached histories / 16.5MB → 0.143s / 0 / 0)方向合理,机制上也对得上:终态事件到达时淘汰缓存、锁改弱引用、把压缩修订持久化后跳过未变化文件。这部分我没有异议。
改动思路
chat 侧的做法是把"缓存增长"从"永远追加"改成"终态即淘汰":TERMINAL_EVENT_KINDS 命中时调用 _drop_event_cache(key) 并清掉缓存的 revision,后续读取回落到文件行(仍然在 exclusive_file_lock 下读);同时把 _session_locks / _event_flush_locks 换成 WeakValueDictionary,让空闲 key 不再被进程长期持有。这套思路跟仓库里"有界存储 + 读取回落持久层"的既有模式一致,新增的 tests/test_chat_event_retention.py 也直接断言了"压缩后没有缓存历史、没有 flush 锁"。
问题出在同一分支里还夹带了第二处改动,而且它削弱了一条既有守卫。
具体改动
4 个文件、+199/-42:
loopx/chat_store.py(70 行):终态事件淘汰缓存;WeakValueDictionary承接 session/flush 锁;新增_drop_event_cache;读取路径在 revision 不匹配时回落文件。tests/test_chat_event_retention.py(新增 155 行):1000 个已完成 Turn 的保留/重启行为。loopx/capabilities/steward_executor/machine_defaults.py(15 行):把函数内的from ...chat_manager import MANAGER_ENDPOINT_KINDS改成importlib.import_module("loopx.chat_manager")再取属性,read_stored_machine_configuration同理换成importlib.import_module("loopx.capabilities.machine_configuration.store")。tests/test_manager_channel_binding.py(-1 行):从"每个生产 steward 调用者都传 machine defaults"的期望集合里删掉loopx/chat_server.py。
关键代码讲解
loopx/chat_store.py:1318 _drop_event_cache:带expected参数做同一性判断,避免与并发写入竞争;这是这次改动里最需要小心的一处,写法是对的。loopx/chat_store.py:1367终态分支:只有终态事件走淘汰,非终态仍扩展缓存——我核过这点,否则流式读取会失去缓存意义。loopx/capabilities/steward_executor/machine_defaults.py:51-61:importlib.import_module替换直接相对导入。base(dd584fab4)里是from ...chat_manager import MANAGER_ENDPOINT_KINDS并在 docstring 里解释了"保持局部导入以免闭合 chat 层循环"。改成 importlib 之后,静态检查器不再看到这条依赖边——提交信息写的是 "preserve the static type-check boundary",也就是说这是一个为了类型检查/CI 的服务性改动。tests/test_manager_channel_binding.py:840:这一行删除是覆盖收窄。断言原本要求loopx/chat_server.py出现在 steward machine-defaults 的调用点集合里;删掉之后,即使 chat_server 将来不再传 machine defaults,这条守卫也不会红。
对主干的风险
阻塞项(P2):本 PR 声称的主题是 chat 事件保留,但 head 同时改写了 steward executor 的词汇解析机制,并从守卫中移除了 chat_server.py。这两件事都没有出现在 PR body 里,也没有证据说明它们是 retention 改动的必要条件(base 与 head 的对比只能看到"改动存在",看不到"为什么必须")。这类"顺手带上、还顺手放宽一条守卫"的组合,正是最容易被绿灯并吞掉的形态:测试在收窄前后都是绿的,所以绿并不能证明 chat_server 真的不再需要 machine defaults。最小修复二选一:(a) 拆成独立 PR;(b) 留在本 PR 但在 body 里写明 importlib 改动解决的具体失败(哪条检查/哪个 cycle 在什么命令下失败),并给出 chat_server 不再传 machine defaults 的证据(调用点、替代路径),否则请恢复该断言。
其余验证:head 上 pytest -q tests/test_chat_event_retention.py tests/test_manager_channel_binding.py 31 项通过;body 里的 94.7% 启动耗时我没复现(需要构建 1000-Turn fixture),这一项在本卡里保持 unverified,但保留机制本身有测试覆盖,所以不构成阻塞。
我的整体评价
结论是 REQUEST_CHANGES。retention 那部分我是认可的:淘汰点选在终态、读取回落持久层、新增测试直接断言缓存与锁为零,符合"有界存储 + 持久层权威"的既有模式。但一个自称 perf(chat) 的分支不该同时改另一条能力线的导入机制,更不该在无证据的情况下把一条守卫的期望集合缩小——这不是风格问题,而是"以后这类回归没人会拦"的问题。把 steward 改动拆出去(或补上必要性与 chat_server 事实的证据)之后,这个 PR 的 chat 部分我认为可以直接批准。
English verdict: REQUEST_CHANGES — exact head a295aa3c84b0ecfc3ad9d13f08b4cdca1776b08a of #4463. The chat retention work itself looks sound: terminal event kinds evict the cached history through _drop_event_cache (with an identity check against concurrent writers), non-terminal kinds still extend it, session and flush locks become weak-value dictionaries, and tests/test_chat_event_retention.py asserts zero cached histories and zero flush locks after the compaction pass — the 31-test local run (tests/test_chat_event_retention.py plus tests/test_manager_channel_binding.py) passes. The blocking problem is scope and guard coverage: the same branch rewrites loopx/capabilities/steward_executor/machine_defaults.py from a direct relative import of the manager channel vocabulary to importlib.import_module, and removes loopx/chat_server.py from the expected caller set of test_every_production_steward_caller_passes_the_machine_defaults. Neither change appears in the PR body, neither is tied by evidence to the retention fix, and narrowing that assertion means a future caller could stop passing machine defaults without the suite noticing — a green run cannot prove otherwise. Minimum repair: split the steward change into its own PR, or keep it with disclosure (the concrete check that fails without the importlib switch) plus evidence that chat_server.py no longer needs the machine defaults, otherwise restore the assertion. Unverified: the 94.7% restart reduction in the body was not reproduced here because it needs the 1000-turn fixture; the retention mechanism itself is covered by the new test.
39894f4 to
3c081b5
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
这个 PR 的目标是给聊天已完成 Turn 的内存保留加上边界:终态事件一旦落盘,就不再把该 Turn 的事件历史留在进程内的热缓存里,键控锁不再随进程寿命增长,压缩也不再每次启动重扫未变化的文件。我上一轮在 a295aa3c8 上给的 REQUEST_CHANGES 有两个阻塞项,这一版都动了;其中一个我这一轮验证下来要撤回,下面写清楚。
先说撤回的部分:我上一轮说"删掉 loopx/chat_server.py 的调用点期待是在削弱守卫"。本 PR 现在的 head 已经 rebase 到 0346a31c2,也就是当前 origin/main,我就在这个 revision 上跑 pytest -q tests/test_manager_channel_binding.py -k steward,结果是 1 failed:因为 loopx/chat_server.py 现在只有一个注释、没有 steward resolver 调用点了,而 main 的期待集合里(第 840 行)仍然写着它。也就是说 main 上这个测试本来就是红的,这个 PR 把那行删掉是在修 main,不是降级守卫;同一个测试在本 PR head 上是 2 passed。这条我判错了,向作者更正。
改动思路
保留策略分三块,都落在 ChatSessionStore 自己已经拥有的状态上:新增两个精确匹配集合 TERMINAL_EVENT_KINDS / REPLAY_ONLY_EVENT_KINDS 来区分"终态事件"和"可丢弃的重放增量";把 _session_locks、_event_flush_locks 换成 WeakValueDictionary,让锁只在调用方持有期间存活;给 compact_completed_events 加"已压缩版本号"跳过逻辑,并把版本号持久化到 Turn 记录里,这样升级后第一次启动做一次全量压缩,之后走快路径。此外这一版还修了两处 base 侧红灯(steward 调用点期待、mypy 跟随 deferred import),并把分支 rebase 到了当前 origin/main(0346a31c2):git merge-base HEAD origin/main 就是 0346a31c2,git merge-tree 干净,因此下面的基线对照就是"合并目标本身",不再需要跨版本推断。
具体改动
loopx/chat_store.py:第 30-31 行新增两个常量;第 144/149 行两个锁字典改为弱引用;第 1318 行新增_drop_event_cache;第 1370 行起flush_events在终态事件落盘后丢弃缓存而不是写入缓存;第 1403 行起events_after读到终态行后按对象身份校验丢弃缓存;第 1412-1440 行compact_completed_events增加版本号跳过与标记写入。loopx/capabilities/steward_executor/machine_defaults.py:第 54/61/240 行的延迟导入改为importlib.import_module,保住"函数内延迟导入"的运行时边界,同时让 mypy 不再跟随进 137 个未类型化模块。tests/test_chat_event_retention.py(新增 155 行):6 个用例覆盖终态缓存驱逐、弱引用锁释放、压缩跳过未变化文件。tests/test_manager_channel_binding.py:第 840 行删除loopx/chat_server.py期待,与 main 的实际调用点一致。
关键代码讲解
loopx/chat_store.py:1370—if any(row["kind"] in TERMINAL_EVENT_KINDS for row in pending): self._drop_event_cache(key):这是整个保留策略的闸门。落盘仍然照旧(_append_jsonl_rows先执行,序列号在文件锁内分配),只是不再把合并后的历史写回_event_cache。我用变异验证过它有实际作用:把这一行换成if False,test_terminal_event_history_does_not_remain_in_the_hot_cache立刻失败(缓存里仍留着两行),恢复后 6 个用例全绿。loopx/chat_store.py:1318—_drop_event_cache(key, expected):把"缓存失效"收敛成一个 owner,并用expected is None or self._event_cache.get(key) is expected做身份校验。这一点是必要的:events_after在读到终态行后也要丢弃缓存,而它丢弃的是自己刚读到的那个 list 对象;没有这层身份校验,一次并发刷新写进缓存的新 list 会被误删。loopx/chat_store.py:144—_session_locks/_event_flush_locks(第 149 行)改为WeakValueDictionary:锁的生命周期跟着调用方走。我核对了全部调用点,8 处都是with self._session_lock(session_id)形式,临界区内始终持有强引用,所以"锁被回收导致两个线程同时进入"的窗口不存在;新用例test_keyed_locks_are_released_after_callers_drop_them也把"引用期间同一把锁、gc 后键消失"钉住了。loopx/chat_store.py:1426—compact_completed_events的版本号逻辑:if turn.get("event_compaction_revision") == list(revision or ()): continue,随后在事件文件锁内压缩、在 Turn 文件锁内重读并只在仍是终态时写标记。_event_revision用(st_ino, st_size, st_mtime_ns)作 token,所以事件文件一旦被追加就会失效并重新压缩;这一条正是把"每次启动都重扫"变成"第一次启动记录、之后跳过"的关键。loopx/capabilities/steward_executor/machine_defaults.py:54—manager = importlib.import_module("loopx.chat_manager"); return frozenset(manager.MANAGER_ENDPOINT_KINDS):这是本 PR 里唯一与聊天保留无关的改动,但它是必要的:我在origin/main上跑python -m mypy得到Found 1051 errors in 141 files (checked 22 source files),head 上是Success: no issues found in 22 source files。代价我也量化了:我加了一行探针from ...chat_manager import NOT_A_REAL_NAME,用原先的类型化延迟导入时 mypy 报machine_defaults.py:29: error: Module "loopx.chat_manager" has no attribute "NOT_A_REAL_NAME" [attr-defined],改成 importlib 后这条静默消失了——也就是这三处调用点丢掉了静态的属性名校验。我另外试了"改 pyproject 加一行follow_imports = "silent""这条替代路径:它确实能让 main 的 mypy 直接变绿,但同一个探针在那种配置下也静默通过,所以那只是把同样的校验损失扩散到全仓,并不是更小或更安全的修法。因此我不把它当成阻塞项,只提一个便宜的补强(见 P3)。
对主干的风险
两个历史阻塞项的处理结果:chat_server 期待删除经我独立复验是"修 base 红灯",不是削弱守卫(merge base = origin/main 0346a31c2 上该测试 1 failed,head 上 2 passed);mypy/steward 改动经我独立复验同样是"修 base 红灯"(同一 revision 上 1051 errors → head Success)。核心保留逻辑与我上轮审过的 a295aa3c8、以及本轮 rebase 前的 39894f4d7 逐字节相同(git diff 39894f4d7..HEAD -- loopx/chat_store.py tests/test_chat_event_retention.py 无输出),head 上新增的只有一次 rebase。
P2(非阻断,披露面):PR 正文 ## Scope 写的是"Backend owner-local Chat persistence only. No frontend, CLI, authority, or public protocol behavior changes.",但 head 还改了 steward capability 的导入机制与一处测试期待。理由是写在提交信息和评论里的,正文没写。合并时读者看的是正文,建议把这一块补进 Scope/Validation 并带上 mypy 证据(或把它拆成独立 PR)。这只需要改正文,不会产生新 head,因此不影响本次 APPROVE 的有效性。
P3(非阻断,重复权威):loopx/chat_store.py:1412 用内联字面量 {"completed","interrupted","timed_out","failed"} 过滤 Turn,而同一个函数第 1438 行写标记时用的是模块常量 TERMINAL_TURN_STATES(第 29 行,内容当前完全一致)。这两处必须同步:以后往常量里加一个终态(例如 canceled),过滤条件不会选中它,于是"标记逻辑"对该状态永久失效。一行替换即可消除,属于本 PR 已经触碰的函数内的行为保持型收口。
P3(非阻断,词汇表校验缺口):上面量化过的 attr-defined 校验丢失,全仓目前没有任何测试引用 MANAGER_ENDPOINT_KINDS / MANAGER_REASONING_EFFORTS(我按符号名全仓搜过)。补一个便宜用例即可恢复保证:断言 steward_executor_endpoints() == frozenset(chat_manager.MANAGER_ENDPOINT_KINDS)、steward_reasoning_efforts() == tuple(chat_manager.MANAGER_REASONING_EFFORTS)。这条也解释了为什么我要顺手提它:改名后会以 AttributeError 形式出现,而 load_effective_steward_executor_defaults 只捕获 OSError/TypeError/ValueError。
P3(非阻断,锁域不一致):新增的标记写入走 exclusive_file_lock(turn_path, ...) 读改写,而同一文件的其他写入(loopx/chat_store.py:1155、:1212)是在每会话线程锁下直接 _atomic_write_json。两个写者用不同锁域,理论上会互相覆盖字段。触发窗口很窄——压缩只挑"终态且 completed_at 超过截止时间"的 Turn,另一个写者得正好在同一时刻更新这条 24 小时以上的旧记录——所以我按 P3 报,而不是当作阻塞;修法是在标记读改写外面再取一次 self._session_lock(session_id)。
验证(全部在 3c081b51a 上跑):tests/test_chat_event_retention.py 6 passed;test_chat_event_retention + test_chat_event_buffer + test_chat_event_cursor + test_chat_session_active_turn + test_manager_channel_binding + test_chat_manager_details + test_chat_machine_configuration_api 合计 83 passed;steward 调用点测试 2 passed;python -m mypy 成功。对照组就是本 PR 的 merge base(= origin/main 0346a31c2):steward 调用点测试 1 failed、mypy 1051 errors in 141 files。变异验证:把终态判定换成 if False 会让新用例失败,随后我把文件恢复成与 head 逐字节一致并确认工作区干净。按本 lane 的 review 配置我不拉取也不等待 GitHub CI,因此以上结论只基于本地仓库验证;本地必跑项没有失败或跳过。
我的整体评价
APPROVE。核心保留改动是可信的:它修的是真问题(进程内缓存与锁字典随 Turn 数无界增长,压缩每次启动重扫),实现方式复用了这个类自己的 _event_cache / _event_revision / 终态词汇,没有引入第二套状态或新的策略开关;关键是它没有牺牲重放正确性——丢弃缓存后 events_after 走文件锁 + 磁盘读,压缩只过滤重放增量、保留终态行,新用例把这两点都钉住了,我的变异实验也证明用例不是空转。我上一轮的两个阻塞项,一个经复验要撤回(chat_server 期待是修 main 红灯),另一个经复验确认真实必要(mypy 1051 → 0),作者的说明与我的独立实验一致。剩下的四条都是非阻断:正文 Scope 漏写 steward 面(改正文即可,不动 head)、同一函数里终态集合的重复字面量、importlib 之后词汇表的静态校验缺口(补一个用例即可)、以及标记写入与既有写者的锁域不一致(窗口很窄)。这些都是可以在后续或顺手一行处理的东西,不构成合并前的阻塞。
English verdict: APPROVE — exact head 3c081b51a95f0acafc297bf99cbad7d5a3b97de3 of #4463. The chat retention work is byte-identical to the head I reviewed at a295aa3c8 and is sound: terminal Turns are evicted from the hot cache while replay still reads durable rows, keyed locks are weakly held and every caller holds them for the critical section, and compaction records event_compaction_revision so later startups skip unchanged files. The branch is now rebased onto current origin/main 0346a31c2 (merge base equals origin/main, git merge-tree clean), so the two base-side repairs can be measured against the exact merge target. I also correct my previous review: removing loopx/chat_server.py from the caller expectation is a fix, not a weakened guard, because that test fails at the merge base (pytest -q tests/test_manager_channel_binding.py -k steward → 1 failed) and passes here, and the steward import rewrite repairs a real base-side mypy regression (python -m mypy at the merge base → 1051 errors in 141 files; here → success). Validation at this head: 6 new retention tests pass, an 83-test chat/steward sweep passes, the steward caller test passes, mypy succeeds, and reverting the terminal-eviction branch in chat_store.py makes the new cache test fail, so the suite has teeth. Non-blocking findings: (P2) the PR body's Scope still claims "Chat persistence only" although the head also changes the steward import boundary and one test expectation - a body edit suffices and does not change this head; (P3) compact_completed_events filters turns with an inline set that duplicates TERMINAL_TURN_STATES, which the same function uses for its own marker write; (P3) the dynamic import drops the attr-defined verification I demonstrated with a probe, and no test pins the vocabulary; (P3) the new marker write uses the turn file lock while the other writers of the same file use the session lock, a narrow lost-update window.
3c081b5 to
ee02d77
Compare
ee02d77 to
8a6950e
Compare
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
8a6950e to
2858bbf
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
这个 PR 的目标是给聊天已完成 Turn 的内存保留加上边界:终态事件一旦落盘,就不再把该 Turn 的事件历史留在进程内的热缓存里,键控锁不再随进程寿命增长,压缩也不再每次启动重扫未变化的文件。我上一轮在 a295aa3c8 上给的 REQUEST_CHANGES 有两个阻塞项,这一版都动了;其中一个我这一轮验证下来要撤回,下面写清楚。
先说撤回的部分:我上一轮说"删掉 loopx/chat_server.py 的调用点期待是在削弱守卫"。本 PR 现在的 head 已经 rebase 到 0346a31c2,也就是当前 origin/main,我就在这个 revision 上跑 pytest -q tests/test_manager_channel_binding.py -k steward,结果是 1 failed:因为 loopx/chat_server.py 现在只有一个注释、没有 steward resolver 调用点了,而 main 的期待集合里(第 840 行)仍然写着它。也就是说 main 上这个测试本来就是红的,这个 PR 把那行删掉是在修 main,不是降级守卫;同一个测试在本 PR head 上是 2 passed。这条我判错了,向作者更正。
改动思路
保留策略分三块,都落在 ChatSessionStore 自己已经拥有的状态上:新增两个精确匹配集合 TERMINAL_EVENT_KINDS / REPLAY_ONLY_EVENT_KINDS 来区分"终态事件"和"可丢弃的重放增量";把 _session_locks、_event_flush_locks 换成 WeakValueDictionary,让锁只在调用方持有期间存活;给 compact_completed_events 加"已压缩版本号"跳过逻辑,并把版本号持久化到 Turn 记录里,这样升级后第一次启动做一次全量压缩,之后走快路径。此外这一版还修了两处 base 侧红灯(steward 调用点期待、mypy 跟随 deferred import),并把分支 rebase 到了当前 origin/main(0346a31c2):git merge-base HEAD origin/main 就是 0346a31c2,git merge-tree 干净,因此下面的基线对照就是"合并目标本身",不再需要跨版本推断。
具体改动
loopx/chat_store.py:第 30-31 行新增两个常量;第 144/149 行两个锁字典改为弱引用;第 1318 行新增_drop_event_cache;第 1370 行起flush_events在终态事件落盘后丢弃缓存而不是写入缓存;第 1403 行起events_after读到终态行后按对象身份校验丢弃缓存;第 1412-1440 行compact_completed_events增加版本号跳过与标记写入。loopx/capabilities/steward_executor/machine_defaults.py:第 54/61/240 行的延迟导入改为importlib.import_module,保住"函数内延迟导入"的运行时边界,同时让 mypy 不再跟随进 137 个未类型化模块。tests/test_chat_event_retention.py(新增 155 行):6 个用例覆盖终态缓存驱逐、弱引用锁释放、压缩跳过未变化文件。tests/test_manager_channel_binding.py:第 840 行删除loopx/chat_server.py期待,与 main 的实际调用点一致。
关键代码讲解
loopx/chat_store.py:1370—if any(row["kind"] in TERMINAL_EVENT_KINDS for row in pending): self._drop_event_cache(key):这是整个保留策略的闸门。落盘仍然照旧(_append_jsonl_rows先执行,序列号在文件锁内分配),只是不再把合并后的历史写回_event_cache。我用变异验证过它有实际作用:把这一行换成if False,test_terminal_event_history_does_not_remain_in_the_hot_cache立刻失败(缓存里仍留着两行),恢复后 6 个用例全绿。loopx/chat_store.py:1318—_drop_event_cache(key, expected):把"缓存失效"收敛成一个 owner,并用expected is None or self._event_cache.get(key) is expected做身份校验。这一点是必要的:events_after在读到终态行后也要丢弃缓存,而它丢弃的是自己刚读到的那个 list 对象;没有这层身份校验,一次并发刷新写进缓存的新 list 会被误删。loopx/chat_store.py:144—_session_locks/_event_flush_locks(第 149 行)改为WeakValueDictionary:锁的生命周期跟着调用方走。我核对了全部调用点,8 处都是with self._session_lock(session_id)形式,临界区内始终持有强引用,所以"锁被回收导致两个线程同时进入"的窗口不存在;新用例test_keyed_locks_are_released_after_callers_drop_them也把"引用期间同一把锁、gc 后键消失"钉住了。loopx/chat_store.py:1426—compact_completed_events的版本号逻辑:if turn.get("event_compaction_revision") == list(revision or ()): continue,随后在事件文件锁内压缩、在 Turn 文件锁内重读并只在仍是终态时写标记。_event_revision用(st_ino, st_size, st_mtime_ns)作 token,所以事件文件一旦被追加就会失效并重新压缩;这一条正是把"每次启动都重扫"变成"第一次启动记录、之后跳过"的关键。loopx/capabilities/steward_executor/machine_defaults.py:54—manager = importlib.import_module("loopx.chat_manager"); return frozenset(manager.MANAGER_ENDPOINT_KINDS):这是本 PR 里唯一与聊天保留无关的改动,但它是必要的:我在origin/main上跑python -m mypy得到Found 1051 errors in 141 files (checked 22 source files),head 上是Success: no issues found in 22 source files。代价我也量化了:我加了一行探针from ...chat_manager import NOT_A_REAL_NAME,用原先的类型化延迟导入时 mypy 报machine_defaults.py:29: error: Module "loopx.chat_manager" has no attribute "NOT_A_REAL_NAME" [attr-defined],改成 importlib 后这条静默消失了——也就是这三处调用点丢掉了静态的属性名校验。我另外试了"改 pyproject 加一行follow_imports = "silent""这条替代路径:它确实能让 main 的 mypy 直接变绿,但同一个探针在那种配置下也静默通过,所以那只是把同样的校验损失扩散到全仓,并不是更小或更安全的修法。因此我不把它当成阻塞项,只提一个便宜的补强(见 P3)。
对主干的风险
两个历史阻塞项的处理结果:chat_server 期待删除经我独立复验是"修 base 红灯",不是削弱守卫(merge base = origin/main 0346a31c2 上该测试 1 failed,head 上 2 passed);mypy/steward 改动经我独立复验同样是"修 base 红灯"(同一 revision 上 1051 errors → head Success)。核心保留逻辑与我上轮审过的 a295aa3c8、以及本轮 rebase 前的 39894f4d7 逐字节相同(git diff 39894f4d7..HEAD -- loopx/chat_store.py tests/test_chat_event_retention.py 无输出),head 上新增的只有一次 rebase。
P2(非阻断,披露面):PR 正文 ## Scope 写的是"Backend owner-local Chat persistence only. No frontend, CLI, authority, or public protocol behavior changes.",但 head 还改了 steward capability 的导入机制与一处测试期待。理由是写在提交信息和评论里的,正文没写。合并时读者看的是正文,建议把这一块补进 Scope/Validation 并带上 mypy 证据(或把它拆成独立 PR)。这只需要改正文,不会产生新 head,因此不影响本次 APPROVE 的有效性。
P3(非阻断,重复权威):loopx/chat_store.py:1412 用内联字面量 {"completed","interrupted","timed_out","failed"} 过滤 Turn,而同一个函数第 1438 行写标记时用的是模块常量 TERMINAL_TURN_STATES(第 29 行,内容当前完全一致)。这两处必须同步:以后往常量里加一个终态(例如 canceled),过滤条件不会选中它,于是"标记逻辑"对该状态永久失效。一行替换即可消除,属于本 PR 已经触碰的函数内的行为保持型收口。
P3(非阻断,词汇表校验缺口):上面量化过的 attr-defined 校验丢失,全仓目前没有任何测试引用 MANAGER_ENDPOINT_KINDS / MANAGER_REASONING_EFFORTS(我按符号名全仓搜过)。补一个便宜用例即可恢复保证:断言 steward_executor_endpoints() == frozenset(chat_manager.MANAGER_ENDPOINT_KINDS)、steward_reasoning_efforts() == tuple(chat_manager.MANAGER_REASONING_EFFORTS)。这条也解释了为什么我要顺手提它:改名后会以 AttributeError 形式出现,而 load_effective_steward_executor_defaults 只捕获 OSError/TypeError/ValueError。
P3(非阻断,锁域不一致):新增的标记写入走 exclusive_file_lock(turn_path, ...) 读改写,而同一文件的其他写入(loopx/chat_store.py:1155、:1212)是在每会话线程锁下直接 _atomic_write_json。两个写者用不同锁域,理论上会互相覆盖字段。触发窗口很窄——压缩只挑"终态且 completed_at 超过截止时间"的 Turn,另一个写者得正好在同一时刻更新这条 24 小时以上的旧记录——所以我按 P3 报,而不是当作阻塞;修法是在标记读改写外面再取一次 self._session_lock(session_id)。
本轮补充(head 2858bbf):这一版在我上轮批准的 chat-retention 与 steward 两块之外又叠加了两个小型模块预算重构——loopx/control_plane/quota/should_run_prepare.py(+8/-9,控制决策复杂度预算),以及把 manager-routing 逻辑从 loopx/extensions/lark/goal_topic_runtime.py 移入 loopx/extensions/lark/manager_routing.py(四个文件 48/37 行,另含 manager SKILL.md 与两主题测试)。我在这个 head 上跑 tests/test_manager_channel_binding.py + tests/test_manager_team_plan_guidance.py + tests/test_chat_event_retention.py 得 33 passed;tests/control_plane/test_quota_settlement_cli.py 的两个 sqlite 失败已在 merge base 300eda256 复现为既有问题;head 已 rebase,与 main 合并干净。正文 Scope 仍写 “Chat persistence only”,与这四个主题不符(P2)。
验证(全部在 3c081b51a 上跑):tests/test_chat_event_retention.py 6 passed;test_chat_event_retention + test_chat_event_buffer + test_chat_event_cursor + test_chat_session_active_turn + test_manager_channel_binding + test_chat_manager_details + test_chat_machine_configuration_api 合计 83 passed;steward 调用点测试 2 passed;python -m mypy 成功。对照组就是本 PR 的 merge base(= origin/main 0346a31c2):steward 调用点测试 1 failed、mypy 1051 errors in 141 files。变异验证:把终态判定换成 if False 会让新用例失败,随后我把文件恢复成与 head 逐字节一致并确认工作区干净。按本 lane 的 review 配置我不拉取也不等待 GitHub CI,因此以上结论只基于本地仓库验证;本地必跑项没有失败或跳过。
我的整体评价
APPROVE。核心保留改动是可信的:它修的是真问题(进程内缓存与锁字典随 Turn 数无界增长,压缩每次启动重扫),实现方式复用了这个类自己的 _event_cache / _event_revision / 终态词汇,没有引入第二套状态或新的策略开关;关键是它没有牺牲重放正确性——丢弃缓存后 events_after 走文件锁 + 磁盘读,压缩只过滤重放增量、保留终态行,新用例把这两点都钉住了,我的变异实验也证明用例不是空转。我上一轮的两个阻塞项,一个经复验要撤回(chat_server 期待是修 main 红灯),另一个经复验确认真实必要(mypy 1051 → 0),作者的说明与我的独立实验一致。剩下的四条都是非阻断:正文 Scope 漏写 steward 面(改正文即可,不动 head)、同一函数里终态集合的重复字面量、importlib 之后词汇表的静态校验缺口(补一个用例即可)、以及标记写入与既有写者的锁域不一致(窗口很窄)。这些都是可以在后续或顺手一行处理的东西,不构成合并前的阻塞。
English verdict: APPROVE — exact head 2858bbffd451aa2909484bd31929a640ebaa67e2 of #4463. The chat retention work is byte-identical to the head I reviewed at a295aa3c8 and is sound: terminal Turns are evicted from the hot cache while replay still reads durable rows, keyed locks are weakly held and every caller holds them for the critical section, and compaction records event_compaction_revision so later startups skip unchanged files. The branch is now rebased onto current origin/main 0346a31c2 (merge base equals origin/main, git merge-tree clean), so the two base-side repairs can be measured against the exact merge target. I also correct my previous review: removing loopx/chat_server.py from the caller expectation is a fix, not a weakened guard, because that test fails at the merge base (pytest -q tests/test_manager_channel_binding.py -k steward → 1 failed) and passes here, and the steward import rewrite repairs a real base-side mypy regression (python -m mypy at the merge base → 1051 errors in 141 files; here → success). Validation at this head: 6 new retention tests pass, an 83-test chat/steward sweep passes, the steward caller test passes, mypy succeeds, and reverting the terminal-eviction branch in chat_store.py makes the new cache test fail, so the suite has teeth. Non-blocking findings: (P2) the PR body's Scope still claims "Chat persistence only" although the head also changes the steward import boundary and one test expectation - a body edit suffices and does not change this head; (P3) compact_completed_events filters turns with an inline set that duplicates TERMINAL_TURN_STATES, which the same function uses for its own marker write; (P3) the dynamic import drops the attr-defined verification I demonstrated with a probe, and no test pins the vocabulary; (P3) the new marker write uses the turn file lock while the other writers of the same file use the session lock, a narrow lost-update window.
PR #4463 bounded completed-event retention by dropping a Turn's rows from the cache as soon as its terminal event was read. That also made every replay of a finished Turn re-read its whole event log, so an SSE reconnect storm paid one full read per replay and `loopx-chat-stream-throughput-smoke` (which pins `read_calls <= 1` for 20 replays) has been red on main since. Both obligations are real: retention must stay bounded, and a finished Turn's replay must be served from memory. `loopx/chat_event_cache.py` now owns both -- it keeps the rows a reader just read and evicts finished Turns past `TERMINAL_EVENT_CACHE_TURNS` (8), so a repeated replay reads its log once and memory is still bounded by a fixed number of Turns rather than by history. The extraction also puts `chat_store.py` back under its module budget: it was exactly at the 1500-line ceiling, and the cache is the piece that belongs in its own bounded context. `rows`/`revisions` stay public mappings and the store keeps `_event_cache`, `_event_cache_revision` and `_event_revision` aliases, so no caller or test changes shape. The retention rule is now testable without a file log: the replay contract (5 replays, 1 read) and the budget (oldest finished Turns evicted, newest replayable) are pinned in `tests/test_chat_event_retention.py`. Boundary: control-plane runtime change (loopx/**), proposed as a PR with an exact-head review and left for the maintainer - never self-merged. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
PR #4463 bounded completed-event retention by dropping a Turn's rows from the cache as soon as its terminal event was read. That also made every replay of a finished Turn re-read its whole event log, so an SSE reconnect storm paid one full read per replay and `loopx-chat-stream-throughput-smoke` (which pins `read_calls <= 1` for 20 replays) has been red on main since. Both obligations are real: retention must stay bounded, and a finished Turn's replay must be served from memory. `loopx/chat_event_cache.py` now owns both -- it keeps the rows a reader just read and evicts finished Turns past `TERMINAL_EVENT_CACHE_TURNS` (8), so a repeated replay reads its log once and memory is still bounded by a fixed number of Turns rather than by history. The extraction also puts `chat_store.py` back under its module budget: it was exactly at the 1500-line ceiling, and the cache is the piece that belongs in its own bounded context. `rows`/`revisions` stay public mappings and the store keeps `_event_cache`, `_event_cache_revision` and `_event_revision` aliases, so no caller or test changes shape. The retention rule is now testable without a file log: the replay contract (5 replays, 1 read) and the budget (oldest finished Turns evicted, newest replayable) are pinned in `tests/test_chat_event_retention.py`. Boundary: control-plane runtime change (loopx/**), proposed as a PR with an exact-head review and left for the maintainer - never self-merged. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Summary
Measurement
Synthetic fixture: 1,000 completed Turns, each retaining a 16 KiB terminal response after compaction.
origin/mainrestart: 2.712 s, 1,000 cached histories, 1,000 flush locks, 16,477,000 retained JSON bytesThe first startup after upgrade records compaction revisions; subsequent startups take the fast path. A changed event file invalidates its revision and is compacted again.
Validation
pytest -q tests/test_chat*.py tests/canary/test_maintainability_ratchet.py— 235 passedruff check loopx/chat_store.py tests/test_chat_event_retention.py— passedloopx canary premerge --from-git-diff— passed, no manual holdsgit diff --check— passedScope
Backend owner-local Chat persistence only. No frontend, CLI, authority, or public protocol behavior changes.