Make event-owned Todo completion atomic and source-bound - #5003
Conversation
|
Premerge evidence for head
These checks cover the changed transaction, lost-ack retry, concurrent source drift, event-only capture hold and source-copy readback. This PR is left for maintainer review; it does not grant provider promotion or remove the event-writer capture hold. |
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
动机
审查对象:PR #5003,精确 head 710b262f2bfcb5b674018fa4cc4b0477a13c381f,基线 bdf1519cfe4a4d044dcb2eb8091a6242bd178173。事件源 Goal 的 Todo 完成旧路径按顺序写 successor add/claim,再写父 Todo completion;后段编码或源状态变化时可能留下可执行的孤儿 successor。接受标准是同一源版本下整组事件要么一起持久化,要么完全不发布,并在不确定提交后可安全读回与重试。
改动思路
完成入口先由既有 TypeScript successor owner 派生提案,Python 仅编码 add/claim/completion 事件;AppendOnlyStateEventStore.append_many 在事件锁内比对投影 checksum,让 TypeScript planner 决定冲突、重放与连续序号,再通过临时文件替换和 fsync 一次发布完整 JSONL。文件/SQLite/PostgreSQL 主权写入路径与前端配置未改,event-writer capture hold 仍在。
具体改动
event_writeback.py(约 267、481、536、614 行)把 successor 的编码与写入分开,并把全部事件连同父完成事件交给一次 source-bound append;空批次 replay 也校验源并补做耐久确认。state_event_append.ts(25–67 行)处理源摘要、同 ID 冲突、同批重放与序号分配;event_sourced_state.py(594–712 行)保持旧行字节不变,锁内一次发布并把 fsync 后不确定状态显式报出。- 合成的相同输入经真实 store 路径对比:批次第二个事件无效时,基线抛错但留有第一个事件;此 head 抛错且日志未创建。精确 head 的 Todo 完成/并发源漂移/丢失 fsync ACK/重放以及共享 store 用例本地 90 passed;TS planner 6 passed,Ruff 与 diff check 通过。
对主干的风险
未发现可复现的代码阻断项。最大权衡是所有 append(不仅 Todo 完成)现在也会整文件替换:旧 inode 不再持续增长,长期 tail 读者需重开;PR/RFC 已明确披露,仓库内共享消费者检索未发现保持打开的读句柄。RFC 的同负载样本约从 11.33ms 增至 28.94ms/批;这是为耐久原子性支付的成本,应在日志继续增长时监测。当前 GitHub 精确头仍有失败的测试/合并检查;本次 review policy 不以远端 CI 替代本地代码证据,也不据此放行合并,维护者需查明并修复/重跑这些门禁。完整仓库测试和完整 TS suite 未在本次独立复跑。
我的整体评价
APPROVE(代码审查结论,不是合并许可)。 该改动击中了经基线/head 反例验证的孤儿 successor 问题,复用了既有 Todo/事件 owner,正反路径与重试读回有定向覆盖,未扩大 capture 或 Agent 权限。相邻的未来向整理已体现在 TS planner 单一决策 owner 与 Python 编码/IO 分工;进一步抽离通用原子文件写入或改进大日志成本可在有新需求/实测瓶颈时处理,不应阻断这个完整切片。控制面 PR 按仓库规则交维护者合并;失败的必需检查在合并前仍需解决。
English verdict: APPROVE - exact head 710b262 fixes the reproduced eager-batch partial-publication defect, with focused local Python/TypeScript validation and source-bound retry coverage. This is a code-review conclusion only; maintainers must resolve failing required checks, and control-plane changes are not self-merged.
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
710b262 to
92ba6b1
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval). Reviewed exact head 92ba6b11d8c76aefd9d64b8a58aaea1dbedcdf8a, rebased onto origin/main 1882feb0c1c0d13b0b3149b8d68404a7954f63d0. The rebase touched one file: loopx/control_plane/effect_runtime_handlers.ts now keeps the upstream withCoordinationSourceTransfer wrappers for the two source-carrying operations and registers goal.state_event.plan_append unwrapped, because the Python caller sends an inline bounded identity payload rather than a source artifact.
动机
已复核基线行为:event 承载的 Todo 完成原先先写后继 add/claim 事件,再写父事件本身;编码阶段一旦报错,就留下一个「可运行但没有父完成事件」的孤儿子项。第二个反例是完成上下文读取之后源文件被改写,旧实现仍返回成功。集成 head 上这两个反例分别被拒绝或产出连贯状态,而它们在基线 1882feb0c 上失败。
对长期推进而言,这个修复让「完成一个 Todo 并原子发布其后继」成为一次可重放的持久动作:修复前一次失败会同时污染后继与父事件,恢复只能靠人工判断;修复后整个批次在事件锁内比较源校验和,并用一次原子替换发布,重放只确认持久性而不新增后继。对用户旅程而言,受影响入口是 loopx todo complete(事件投影 Goal 的 legacy 适配路径)与消费事件流的读侧:成功仍然返回空列表字段,重复调用返回幂等结果,源在读取后变化时返回可操作的源漂移失败而不是静默成功。
改动思路
新 owner 是 TypeScript 的 goal.state_event.plan_append:Python 在持有文件锁时把「已存事件身份 + 请求批次身份 + 源校验和 + 期望校验和 + 当前最大序号」交给 TS,TS 一次性决定整批的 append/replay 与序号分配,并对 expected_checksum 不匹配返回 event_source_changed、对同 id 不同指纹返回 event_id_conflict。Python 保留事件编解码、锁、字节与文件系统副作用,并新增 StateEventSourceChangedError 与 StateEventCommitUnknownError,把「源变了」和「可能已落盘」区分开。后继派生也改为 consumer 角色:Python 不再自己决定 continuation policy、capabilities、repository、excluded agents 与绑定默认值,而是编码 TS 已给出的 typed proposal。
语义与集成
append_many(events, expected_checksum=...) 只接受 list/tuple 的 eager 批次;lazy iterable 仍走每条可见的旧路径,并在带 expected_checksum 时直接拒绝,避免「部分可见」被误当成原子批次。历史 JSONL 字节(包括无害空行)被完整保留,重放路径在文件存在时额外做一次持久性验证,因此「上一次替换已成功但目录 fsync 失败」不会被当成未写入。RFC ledger 与两本双语 RFC 页只补齐了当前实现清单与 5–8 包的估算口径,没有把计划写成承诺。
具体改动
loopx/control_plane/goals/state_event_append.ts:新增planStateEventAppend,在纯函数内完成整批冲突/重放判定与序号分配,越界或非法身份直接抛EffectRuntimeRequestError。loopx/control_plane/effect_runtime_handlers.ts:注册该 handler;按上游约定只给真正带源传输的操作加withCoordinationSourceTransfer。loopx/event_sourced_state.py:append_many增加expected_checksum与原子发布;新增StateEventSourceChangedError、StateEventCommitUnknownError;重放时校验持久性。loopx/control_plane/todos/event_writeback.py:_append_event_projected_successor改为_encode_event_projected_successor,只编码 TS proposal 并返回事件批次;完成事件与后继事件合并成一次append_many;replay 分支先验证源校验和,漂移则返回既有的完成校验失败结构。- 测试与证据:
tests/control_plane/test_event_todo_transaction.py(原子发布、源漂移、重放)、tests/test_event_sourced_state_store.py(eager/lazy、字节保留、提交未知)、tests/control_plane_ts/state_event_append.test.ts(6 例,含原型键名)、examples/control_plane/event-completion-rehearsal.py(真实源、临时 registry 与事件日志的完成+后继+重放演练),以及双语 ledger 与test_todo_mutation_authority.py的 pair 调整。
对主干的风险
最大风险是发布方式从追加变成整文件原子替换:读者必须重新打开文件,且单次完成要重写整份日志。我在真实 6.1 MB 源上的一次 CLI 完成耗时约 2.4 s,作者测得同一份 707 KB 日志的七组热批中位数从 11.33 ms 升到 28.94 ms;这是可解释的持久性代价,而该适配器不是未来的高吞吐 provider,批次大小由调用方决定。第二个风险是跨语言传输面:RPC 只带身份与校验和,不带正文,因此 3 MB 级证据仍可在 4 KiB 级事实内传递,但也意味着任何身份字段的错误都会表现为拒绝而非静默。第三个风险是目标级门禁当前为红:canary premerge --goal-id loopx-meta 选中 19 项检查,其中语义词汇漂移 smoke 因模块对预算 44 > 43 失败,我在干净的 origin/main 1882feb0c 上复现了同一失败,故它属于继承的基线红灯而非本 diff 引入;本目标的评审策略不等待远端 CI,PostgreSQL 与打包前端不在本切片内。
我的整体评价
没有发现阻断项。这个增量把一个真实的可运行孤儿与「读到旧上下文仍返回成功」修成了类型化、可重放且带源见证的原子批次,并且顺带删掉了 Python 侧与 TS 重复的后继默认值/归一化决策,符合仓库「不要第二个决策 owner」的要求。验证覆盖了正例、负例与真实后端:128 个 focused Python 用例、3076 个 control-plane TS 用例(0 失败)、严格 mypy 23 文件、ruff、类型检查与真实源演练全部通过;变更质量回执 cqr_78837c25c6ea6f655fbc 对该精确指纹有效。需要维护者知道的两点:继承的词汇预算红灯需要单独决策,以及该 legacy 事件写路径应随其最后一个调用方退役。建议按维护者流程合并。
English verdict: APPROVE - exact head 92ba6b11d8c76aefd9d64b8a58aaea1dbedcdf8a; the atomic, source-bound event batch plus the TS-owned planner removes a real orphan/success-after-change risk and de-duplicates Python successor decisions, with focused Python, full control-plane TypeScript, strict typing and a real-source rehearsal green. The only gate failure is the inherited module-pair vocabulary budget, reproduced unchanged on a clean origin/main checkout.
Merge note — #5003 merged on the reviewed exact headMerged via admin bypass as Changed surfaces: the new TypeScript event-append planner and its handler registration, Rebase repair: one conflict in Checks on the reviewed head
Failures, skips and holds: the goal-scoped premerge gate selected 19 checks; 18 passed. Residual risk: atomic publication rewrites the whole event log, so a reader holding an open handle must reopen; the rehearsal measured about 2.4 s for one completion on a 6.1 MB source. This legacy adapter should retire with its last caller, and event-writer/outbox binding remains open. |
Problem and result
Completing an event-owned Todo previously appended successor add/claim events before it encoded the parent's completion event. A late encoding error left a runnable orphan. A source change after the completion context was read could still return success. Both counterexamples fail against the base and pass here.
This PR plans the full event batch in one TypeScript admission step, compares the source checksum under the event lock, and publishes all successor and completion events with one durable atomic replace. Exact replay confirms durability without creating another successor. Existing capture gates remain closed until the event writer is wired to the prepared/committed outbox lifecycle.
Reviewable changes
goal.state_event.plan_appendowns batch conflict/replay and sequence allocation; Python retains the legacy event codec, lock and filesystem effects. Cross-language transport carries only the source witness and relevant identities.Validation and limits
cqr_6fa14563d596ed6b36b1for the rebased exact diff.Runtime changes are limited to the legacy event-owned Todo completion and event append adapter. File/SQLite/PostgreSQL authority stores, provider defaults, frontend settings and persisted event schema are unchanged. The immediate successor is event-writer/outbox binding and mixed-writer whole-Goal rehearsal; SQLite D2 qualification stays with #4224. This PR closes one prerequisite within that package, not a complete migration stage.