fix(history): serialize the per-goal index append and repair - #4525
Conversation
Refs GH-C07 The row asks for the global-registry write lock to reach the per-goal todo, refresh and history writers. Todo and refresh are already guarded: every todo mutation runs inside legacy_todo_write_transaction, which holds exclusive_cross_runtime_file_lock on the per-goal todo lock path, and state_refresh.py locks the state file. loopx/history.py had none. That left one real gap. index.jsonl has an appending writer and a rewriting one: write_reserved_run_artifacts appends a row, while repair_index_duplicates reads every line, decides which duplicates to drop, then replaces the file from that snapshot. An append landing between the read and the replace is lost, because the rewrite puts back a snapshot that never saw it. Both sides now take the same lock on the same index path. The repair holds it around the whole read-analyse-rewrite block, not just the final replace, since guarding only the write would still rewrite from an unlocked snapshot. A dry run keeps nullcontext and takes no lock, so a read-only preview does not block writers. No nested lock is introduced: the callers in promotion_gate.py, dreaming.py and cli_commands/history.py hold no lock of their own. Signed-off-by: YZJF <195568136+YZJF@users.noreply.github.com>
5cd9e28 to
9297750
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
每个 goal 的 run history 索引 runs/index.jsonl 有两个写入点:write_reserved_run_artifacts(保留 run 路径、写 json/markdown、追加索引行)与 repair_index_duplicates(读回索引、重写)。此前两者都没有加锁,并发写入会互相插入索引行、甚至争抢同一个保留路径;修复自身也可能与追加竞争而丢行。
改动思路
沿用仓库既有的跨进程锁原语,给"同一个索引文件"配同一把锁,并把锁的作用域放在既有操作之外,而不是引入新队列或写入者抽象:追加路径用 history_run_append,修复的 execute 用 history_index_repair,dry-run 用 nullcontext() 明确不取写锁。
具体改动
loopx/history.py(+146/-67 级改动):第 128 行把reserve_unique_run_paths、两处write_text与索引追加整体包进exclusive_file_lock(index_path, operation="history_run_append");第 517-519 行让repair_index_duplicates在 execute 时取history_index_repair锁、dry-run 时使用nullcontext()。tests/test_history_index_write_serialization.py(新增 129 行,4 个用例):test_append_takes_the_goal_index_lock、test_append_is_visible_after_the_lock_released、test_repair_locks_the_same_index_path_on_execute、test_repair_dry_run_does_not_take_the_write_lock。
关键代码讲解
loopx/history.py:128—with exclusive_file_lock(index_path, operation="history_run_append")::关键点是保留路径也在锁内。原实现先reserve_unique_run_paths再写文件再追加,两个并发写入者理论上可以拿到同一路径;现在保留、写盘、追加构成一个临界区。loopx/history.py:517—exclusive_file_lock(index_path, operation="history_index_repair")/nullcontext():修复与追加共用同一路径的锁,因此"修复跑在执行中"不会再与追加交错;dry-run 走nullcontext(),预览不会被写锁阻塞,也不会持锁。tests/test_history_index_write_serialization.py:55— 这组测试用被 patch 的锁记录调用,断言"追加取锁、修复 execute 取锁、修复 dry-run 不取锁"。这类断言比单纯行为测试更能防回归:删掉锁不会改变单进程行为测试的结果,但会让这三个用例失败。
对主干的风险
没有阻塞项。 第 128 行的锁把原本无锁的三步收进同一临界区;第 517 行的分支保证修复只在 execute 时持锁;4 个新用例通过,分支与当前 main merge-tree 干净(merge base 856dc9a38)。
未验证维度(已写入 result):测试观察的是锁调用而不是真正的多进程争用,因此我无法在本轮证明"两个真实进程并发写同一 goal 索引一定串行化";这依赖 loopx/file_lock.py 的既有语义(仓库其它写入者同样依赖它)。这条不构成阻塞,但如果维护者要进一步加固,可以在测试里起两个真实进程而非 patch 锁。
我的整体评价
APPROVE。这类"把无锁的三步操作收进一把既有锁"的修复,价值在于它同时消除了两个真实竞态(重复/交错索引行、争抢保留路径),而且没有引入新机制——锁的命名与作用域一眼可读,dry-run 明确不持锁也照顾了预览语义。我很看重那组用锁调用记录来断言的测试:它把"删掉锁也能过行为测试"这个盲点堵住了。唯一没覆盖的是真实多进程争用,我把它作为残余风险写在卡片里,而不是当作已证。
English verdict: APPROVE — exact head 9297750db342c5d2562f2e6a190e329b4ca226af of #4525. The per-goal history index now serializes its two writers: write_reserved_run_artifacts wraps path reservation, the artifact writes and the index append in exclusive_file_lock(index_path, 'history_run_append') (loopx/history.py:128), and repair_index_duplicates takes the same index lock when executing while keeping dry-run lock-free via nullcontext() (:517). Validation at this head: the 4 new locking tests pass (append takes the lock, append is visible after release, repair execute locks the same path, dry-run does not take the write lock) and the branch merges cleanly with current main. No blocking findings; the residual risk is that the tests assert lock calls rather than exercising true multi-process contention.
Refs GH-C07 — contributor task board row: "Global registry sync now writes inside a lock; extend the same lock or optimistic-revision guard to per-goal todo/refresh/history writers and include a concurrent todo add/update regression."
Status of the three writers named in the row
Checked on
a28562e97. The row is partly stale:loopx/todos.py(add 863, update 1198, complete 1503, supersede 1845, archive 1990) run insidelegacy_todo_write_transaction, which holdsexclusive_cross_runtime_file_lockonlegacy_coordination_todo_lock_path(runtime_root, goal_id)(control_plane/coordination/legacy_writer_fence.py:301). Also used bytodo_followups.py,bootstrap.py,handoff_mode.py,event_writeback.pyandprojects/registry.py.loopx/state_refresh.pyhas five lock usages, includingexclusive_cross_runtime_file_lock(resolved_state_file)around the state-file mutation.loopx/history.pyhad zero lock usages.So the remaining gap in this row is the per-goal history index. No todo add/update regression was added, because that writer is already serialized; adding a second guard there would be redundant.
The gap
<runtime_root>/goals/<goal_id>/runs/index.jsonlhas two writers that did not agree on anything:write_reserved_run_artifactsappends one index row (opened in"a"mode).repair_index_duplicatesrewrites the whole file from a snapshot it read earlier: it reads every line, groups duplicates, decides which lines to drop, then writes a temp file andreplace()s the index.If an append lands after the repair reads the index but before it replaces it, the repair writes back a snapshot that never saw that row and the run record disappears from history. The append is atomic; the read-modify-write is not, and it was unprotected.
Change
Both sides now take the same lock on the same path,
index_path:write_reserved_run_artifactsholdsexclusive_file_lock(index_path, operation="history_run_append")across path reservation, the JSON/markdown writes and the index append, so a repair can never observe a half-written record.repair_index_duplicatesholdsexclusive_file_lock(index_path, operation="history_index_repair")around the whole per-goal read-analyse-rewrite block, so the snapshot it rewrites from is taken under the lock. Guarding only the finalreplace()would not help; the read has to be inside the hold too.nullcontext()and takes no lock, matching the existing rule inmutate_global_registry/test_global_registry_write_serialization.pythat a read-only preview must not block concurrent writers.No nested-lock risk was introduced: the two call sites of
write_reserved_run_artifacts(promotion_gate.py:89,dreaming.py:577) and the single call site ofrepair_index_duplicates(cli_commands/history.py:138) contain no lock usage of their own.Verification
New
tests/test_history_index_write_serialization.py(4 cases), written in the same style as the existing global-registry serialization test — recording the lock path rather than racing threads, so it is deterministic:index.jsonl;execute=True(this is the invariant that makes the guard real — two different lock paths would protect nothing);The tests fail before this change: on
a28562e97all four error out, becausehistoryhas no lock usage at all to record.Also run:
tests/ -k "history or promotion or dream or run_artifact"→ 157 passed. The 4 failures intests/control_plane/test_sqlite_authority_cli.pyare pre-existing on this machine (5 fail on a cleana28562e97checkout; they need the SQLite/Node authority fixture).ruff checkandpy_compileclean onloopx/history.py.Base
a28562e97, Python 3.13.12.