fix(issue-fix): close grouped Monitor execution and recovery - #4862
huangruiteng wants to merge 2 commits into
Conversation
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Exact head reviewed: cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509
动机
这个 PR 的目标是对的:把 grouped issue-fix Monitor 的 eligibility/grouping/operation decision 收到 TypeScript typed planner,Python 只负责读取完整 ledger、执行 public Todo writers、持有并释放 lease。它解决的也不是表面问题——空 bucket 要真实完成,membership 变化要被观察,进程中断后同一执行还要能恢复,不能借用别人的 lease。
我没有只验证正常 observe/complete,而是把“中断后恢复”当作最强产品承诺来反证。结果发现当前 exact head 只在测试复用完全相同 generated_at 时可恢复;真实 CLI 重跑会生成新的时间戳,因此会把自己的旧 lease 当成另一次执行并被冲突拒绝。
改动思路
总体边界合理:
planIssueFixMonitorReconciliation从完整 ledger 与 canonical Todo 状态生成 typed steps;- Python materializer 在 effect 前 acquire,拿到自己的 execution proof 后 re-plan,再调用
add_goal_todo/update_goal_todo/complete_goal_todo; _release_attempt只允许相同 owner、相同 attempt prefix、相同 proof 释放,避免误放别人的执行;- legacy/file/SQLite 都走同一 lease/public-writer contract。
阻塞点不在这个分层,而在 attempt identity 的输入:
identity = json.dumps(
[goal_id, step.get("todo_id"), claimed_by, generated_at, cadence, rows],
sort_keys=True,
separators=(",", ":"),
)
prefix = "issue-fix-monitor:" + sha256(identity) + ":"generated_at 是每次命令的 observation time,rows 还是原始有序列表;它们不是同一业务执行的稳定身份。进程在 durable acquire 后退出时,下一次正常命令即使 Goal、Todo、actor、cadence、ledger 语义和 planned operation 全部相同,只要时间变化一秒,prefix 就不同。随后代码拒绝复用旧 key、生成新 key,再被现存 active lease 拒绝。
具体改动
本 PR 已经完成且验证有效的部分:
- TypeScript planner 正确接管 group construction、add/observe/reactivate/complete/unchanged 的决策;
- Python effect adapter 使用现有 public Todo/lease API,没有再保留第二套业务 transition owner;
- effect 前 acquire、acquire 后 re-plan、effect 后 exact-proof release 的顺序合理;
- complete ledger、explicit boolean、namespace、priority 和 projection 字段都采用 typed contract;
- 三个 provider 的正常 execution、lost response、wrong actor/lease 与 projection settlement 覆盖扎实。
但需要修复以下阻塞问题:
[P1] 真实重试的 generated_at 变化会破坏自己的 lease recovery
- 位置:
loopx/capabilities/issue_fix/pr_monitor_materialization.py:99-136 - 触发:第一次执行在
acquire_task_lease已持久化之后进程退出;相同命令重新运行,generated_at从01:00:00Z变为01:00:01Z; - 实际结果:legacy 报
todo already has an active lease,file/SQLite 报todo_lease_conflict; - 影响:Monitor bucket 被卡到 TTL 到期或人工释放,和 PR 的“interrupted execution recovery”目标直接相反;
- 为什么现有测试没发现:
test_retry_after_interrupted_acquisition_recovers_its_own_attempt和 real-process retry 都把两次generated_at固定成完全相同的值,因此恰好得到同一 prefix。
最小修复建议:
- attempt prefix 由稳定的执行意图派生,例如 Goal/Todo/actor/typed target operation 与必要的稳定 source fingerprint;不要包含每次 observation 的时间戳或 incidental list order;
- source freshness 继续在 acquire 后的 typed re-plan 中单独校验,不能为了稳定 key 而复用陈旧业务决定;
- 保留“unrelated execution 即使同 actor 也不能借 lease”的现有负例;
- 增加三后端回归:第一次在 acquire 后退出,第二次使用更晚
generated_at,要求成功恢复并保持同一 lease key/epoch;最好再覆盖语义相同但 ledger 行顺序变化。
对主干的风险
exact head 的正常路径质量较高:
- focused Python suite:
37 passed(70.37s); - source-checkout TypeScript/authority suite:
419 passed(189.71s); ruff check:通过;git diff --check:通过;- 对当前
origin/main的git merge-tree --write-tree:无冲突。
第一次直接调用 system node 时,子进程误用了系统 Python,出现 dataclass(slots=...) 环境错误;随后改用 uv run --extra test node ...,完整套件全绿。这一环境纠正不影响上面的 blocker。
我另外运行了独立的真实边界负例,只改变重试时间戳,legacy/file/SQLite 3/3 全部失败。临时测试文件已删除,两个 review worktree 都保持干净。这个失败比正常 suite 更接近实际 CLI 行为,因此不能用现有绿测覆盖掉。
我的整体评价
结论是 REQUEST_CHANGES。TypeScript 决策权、Python effect boundary、public writer/lease 复用以及 normal-path tests 都是正确方向;我不建议推翻这套设计。需要阻止合并的是一个很窄但直接命中核心承诺的语义错误:把 observation timestamp/raw row ordering 当成 recoverable execution identity。
修复后请不要只重跑现有同时间戳测试;要让 fresh-invocation counterexample 在三个 provider 上先红后绿,并重新检查 unrelated lease 仍然拒绝。完成这个最小修复后,PR 的整体结构有机会直接通过复审。
English verdict: REQUEST_CHANGES - exact head cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509 has a blocking recovery bug: the lease-attempt prefix includes generated_at, so a normal retry after a post-acquire crash cannot recognize its own active lease; a one-second timestamp change reproduced todo_lease_conflict on legacy, file, and SQLite despite the existing same-timestamp suites passing.
Goal And Delivered Outcome
Close the retained issue-fix grouped Monitor execution caller under the shared-authority and TS migration roadmap (#4574, #3225, #3245).
Before, the public PR-lifecycle command could create a grouped Monitor but could not observe changed membership or stop an empty bucket under hard leases. The immutable main baseline fails the member-change step on File/SQLite and the empty-group step on legacy. The caller now completes create → observe → stop → reactivate through the existing typed Todo and lease owners, with recovery after interrupted execution.
Base:
main. This closes one real L4 caller; it does not change provider defaults or complete whole-Goal migration.Behavior And Refactor
--runtime-rootoverride through every affected read/write and use the explicit priority parameter for creation. No new capability, provider, settings surface or raw-patch authority.Buckets remain independent commits. A later failure does not undo earlier committed buckets; read back and retry the observation. Unchanged retry can perform lease cleanup and display delivery while reporting no Todo business write. This is not an external-tool fencing protocol or an all-bucket atomic transaction.
Validation
cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509.Cold CLI observations on the retained source took roughly 3–18 seconds per invocation under concurrent validation load. Acquiring and revalidating has a cost; this PR claims correctness and ownership convergence, not throughput improvement. Further batching belongs to the existing transaction-payoff workstream.
Exact-scope quality receipt:
cqr_232a33f42dcbc0e8bfc4, verifiedvalid; zero blockers/warnings, one disclosed efficiency advisory, no policy safe-fix pass.Scope And Continuation
Complete for the grouped Monitor caller. Bilingual operator docs and RFC checkpoints describe the changed error/recovery behavior and retain the conditional 5–8 remaining delivery-package estimate: remaining execution callers/fences, D1 consumers, contributor-owned SQLite D2 (#4224), capture/whole-Goal qualification, and default onboarding/retirement. No active Goal was promoted or used as a mutation fixture. PostgreSQL deployment, capacity and elapsed soak remain separate.
The affected entrypoint is the existing issue-fix CLI and its shared command template. No frontend change is needed: no setting, action schema or visual surface is introduced; existing Todo rendering and projection recovery are reused. No first-screen changes. The bounded future-facing pass was applied at the planner/execution boundary; no speculative provider framework was added.
Two DCO-signed commits separate runtime behavior from durable validation and documentation. Maintainer review and merge are required for this control-plane change.