Skip to content

fix(global-registry): serialize global registry read-modify-write - #2825

Merged
huangruiteng merged 3 commits into
huangruiteng:mainfrom
shani-singh1:fix/global-registry-write-serialization
Aug 7, 2026
Merged

fix(global-registry): serialize global registry read-modify-write#2825
huangruiteng merged 3 commits into
huangruiteng:mainfrom
shani-singh1:fix/global-registry-write-serialization

Conversation

@shani-singh1

@shani-singh1 shani-singh1 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Serialize every active registry.global.json read-modify-write path through one canonical mutate_global_registry(global_path, operation, reducer) transaction.
  • Keep dry-run and write-denied previews lock-free, but require the committed payload and receipt to come from a fresh snapshot loaded under the global-registry lock.
  • Revalidate retirement eligibility under that lock and preserve unrelated goals added while project-uninstall is in progress.

Closes #2823.

Why

Several projects can share one common_runtime_root. Before this PR, two processes could both read registry.global.json, independently construct complete replacement payloads, and let the later write silently discard the earlier process's goal. Each command still returned success because its own goal was present in the payload it wrote.

The original contribution correctly serialized sync-global and the retirement write, then supplied a deterministic cross-process reproduction. Maintainer review found two remaining protocol gaps: project-uninstall still bypassed the lock, and retirement eligibility was checked only before acquiring it.

Implementation

mutate_global_registry owns the complete mutation window:

  1. acquire the existing exclusive lock for the global path;
  2. load the latest registry snapshot;
  3. run a typed reducer that validates and derives the next payload plus receipt;
  4. write any requested recovery backup while still holding the lock;
  5. atomically replace the registry when the payload changed.

sync_project_registry_to_global, retire_global_registry_goals, and uninstall_project all consume this primitive. Their lock-free previews remain presentation or permission-probe inputs only; they never determine the final write.

Positive concurrency path

If process A commits goal-a while process B is waiting, B acquires the lock afterward, reloads A's committed state, and writes goal-a + goal-b.

Fail-closed retirement path

If a goal regains a live source registry or state file between preview and lock acquisition, the reducer observes that current state under the lock, raises, and leaves the goal intact.

Uninstall path

If another project commits goal-beta after uninstall's preview, uninstall reloads the current global registry under the lock, removes only the matching goal and source route, and preserves goal-beta.

Scope

Changed surfaces:

  • loopx/global_registry.py
  • loopx/project_uninstall.py
  • tests/test_global_registry_write_serialization.py

This PR does not change the POSIX-only behavior of the existing file-lock implementation, add fsync durability, or implement remote provider CAS/receipts.

Validation

  • pytest focused serialization/runtime tests: 9 passed
  • Registry lifecycle smokes: sync, writability, retirement, uninstall, and boundary contract all passed
  • Ruff lint and format: passed
  • Repository-native Mypy: passed, 15 configured source files
  • Public/private boundary scan: 0 errors, 0 warnings
  • git diff --check: passed
  • Exact-scope change-quality receipt: cqr_052c37184643d8adc555, valid, no blockers

The standard premerge selector also passed packaged install, update, maintainability, compile, and diff checks. Its install-local-smoke failed on a stale assertion for the recently updated PR-review skill description; the identical failure reproduces on clean origin/main at 8a04d1c84 and does not touch these registry paths. The directly affected project-uninstall smoke passed.

Attribution

Thanks to @shani-singh1 for reporting the silent data-loss race, supplying the original locking fix, and adding the deterministic multi-process regression that made this failure observable.

`sync_project_registry_to_global` read `registry.global.json` and wrote it
back with nothing serializing the two, so two `loopx` processes sharing one
`common_runtime_root` could interleave read -> merge -> write and let the
later writer commit a payload built before the earlier writer's goals
existed. The dropped goals belonged to the other process, so every sync
still reported `ok: true`, `wrote: true`, and its own id in
`synced_goal_ids` with no route collision.

The existing `exclusive_file_lock` in `register-agent` guards
`source_registry_path`, the per-project registry, so two projects take two
different locks and both mutate the one shared file.

Hold the global registry lock across the authoritative read, merge, and
write. The pre-lock merge is kept only so the write-denied preflight can
still report a merge preview, and the writability probe stays ahead of the
lock so an unwritable runtime root keeps returning the graceful
`global_registry_write_denied` payload instead of raising on lock creation.
`retire_global_registry_goals` had the same read-modify-write shape and now
re-reads under the same lock rather than writing back its inspection
snapshot. A `dry_run` preview takes no lock.

Refs huangruiteng#2823

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论:请求修改。 sync-global / retire-global-goals 共享同一把锁是正确方向,但 exact head 仍允许另一个全局 registry RMW writer 绕过锁并丢数据;retirement 也会依据锁外的过期资格删除已经重新变为 live 的 goal。

动机

旧路径由 bootstraprefresh-stateregister-agent、显式 sync-global 等活跃调用方把 project registry 合并进一个共享的 registry.global.json。每个进程原先执行“读完整 JSON → 合并自己的 goal → 整体替换”,所以两个项目同时同步时,后写者可能拿旧快照覆盖先写者;两个命令仍都报告 ok=true,长程 agent 只会在后续 status/quota 中看到 goal 神秘消失,恢复成本高且难归因。

这个 PR 要达到的可观察结果是:同一 runtime root 的并发 mutation 形成确定顺序,每个 writer 都在拿到锁后读取最新状态,再生成并提交 payload。正向场景是 A 先提交 goal-a,B 等待后重读并提交 goal-a+goal-b。这个目标真实且必要;仅给最终 replace() 加锁不够,因为 payload 若在锁外生成,仍是 stale write。

改动思路

sync_project_registry_to_global 保留一次锁外 preview,继续服务 write-denied 返回和 dry-run;真正写入时锁住 global_path,重新调用 merge helper,随后在同一 hold 内写 recovery backup 和目标文件。retire_global_registry_goals 也在写前重读当前 registry,避免把较早的完整快照原样写回。dry-run 不取 mutation lock,保持只读预览不阻塞 writer。

执行链是:project registry → sanitize incoming goals → 非权威 preview / writability probe → exclusive_file_lock(global_path) → 重新读取 global registry → merge/filter → sibling temp file replace → 用锁内结果构造 receipt。锁超时由现有 file-lock policy 负责,写权限问题仍走已有的 typed write-denied payload。

具体改动

Exact diff 为 2 个文件、生产代码 +90/-26、测试 +313。新增 5 个聚焦测试,覆盖锁内 read/write、dry-run 不加锁、preview 后插入 goal 的确定性回归、retire 锁内 read/write,以及真实多进程 sync。

关键代码讲解

  1. merge_into_global_registry 把“读取当前文件 + 合并 incoming + 构造完整 payload”收成一个纯粹的 reducer-like helper。关键不变量是:写调用方必须在 global registry lock 内调用它;返回的 actions/synced_ids/collisions 同时成为最终 receipt 的来源。

  2. sync_project_registry_to_global 在 551-565 行实现权威事务窗口:

with exclusive_file_lock(global_path, operation="sync_global_registry"):
    existing, payload, ... = merge_into_global_registry(...)
    write_json(global_path, payload)

因此两个遵守该协议的 sync 不再基于同一个旧 head 写回。锁外 preview 只影响 dry-run 或 write-denied 说明,不应决定最终成功 payload。

  1. retire_global_registry_goals 在锁内重读并按 retired id 过滤,修复了“删除 A 时覆盖并发新增 B”的一半问题;但 eligibility 仍只在 320-365 行的锁外快照验证,锁内没有重验同一目标是否仍可退休。

结构上最值得简化的是建立一个窄的 mutate_global_registry(global_path, operation, reducer) primitive:统一 lock → load → validate/reduce → backup → write,并让 sync、retire、project-uninstall 全部消费它。这样比要求每个调用点自行记住锁纪律更短,也使“所有 writer 必须参与同一协议”可测试。

对主干的风险

P1:project-uninstall 仍是无锁的 global registry RMW writer,能绕过新锁并丢掉无关 goal。 uninstall_project 在 242-247 行读取并构造完整 new_global_registry,之后在 290 行直接替换目标文件,没有获取 global_path lock。精确 head 上我在其 snapshot 后注入一个并发同步的 goal-beta,uninstall 返回 ok=true,最终 registry 仍为空,goal-beta 被静默覆盖。文件锁只有所有 writer 都遵守才提供 serialization。最小修复是让 uninstall 在同一 global lock 内重读、按 route+goal id 删除并写回;补“uninstall preview 后并发新增无关 goal 必须保留”的回归。

P1:retirement 会删除在 inspection 后重新变为 live 的同一 goal。 当前先在锁外确认 source registry 和 state file 均不存在,拿锁后只按 id 删除。我在 lock acquisition 前把 goal-alpha 更新为指向真实存在的 source/state;命令仍返回成功并把它删除。最小修复是基于锁内 current 重新解析目标、检查 missing/duplicate,并再次验证 source/state eligibility;若目标被刷新或重新激活,应 fail closed 且保留它。补“同一 goal 在 preview 与 lock 之间恢复 live route”的负向测试。

当前没有 GitHub checks。聚焦测试虽然覆盖了两个协作 writer 的正确顺序,却没有覆盖仓库中的第三个真实 writer,也没有覆盖 retirement target 自身在窗口内变化。

我的整体评价

审阅 head 223daecb180cd03268c87489ff741e64f19e4337。我运行了 pytest 聚焦文件(5 passed)、目标文件 Ruff、git diff --check,并完成上述两个确定性失败复现。代码量判断为 partly avoidable:核心 +90/-26 与并发测试基本合理,但锁纪律分散在函数内,导致 active writer 漏接;用一个 canonical mutation primitive 可同时缩小实现并关闭漏洞。

对已合入的 NoKV shared-goal RFC 的影响是有限正向而非直接实现进展。RFC 明确把 project/global registry route 定义为 host-local projection,不进入 shared coordination head;NoKV canonical 仍要求 provider generation-CAS、target-scoped precondition 和 state+receipt 同笔提交。这个 PR 能让默认本地模式和 shadow/canary 对照的 route baseline 更可靠,也再次证明 RMW 必须集中到一个 authority owner;但它不能作为 NoKV CAS/receipt qualification,且在上面两个 P1 修复前,连 host-local 双写基线也仍可能产生静默丢失。

@huangruiteng

Copy link
Copy Markdown
Owner

Maintainer refinement is now on head 248ab5806dc95e7b54c85662de8424465210d52a.

The two requested P1s are closed without widening product behavior:

  • project-uninstall now participates in the same global-registry transaction and preserves unrelated concurrent writes;
  • retirement now repeats missing/duplicate and live source/state eligibility checks against the snapshot loaded under the lock.

The implementation also adopts the suggested simplification: one mutate_global_registry(global_path, operation, reducer) primitive owns lock -> load -> validate/reduce -> backup -> write for sync, retirement, and uninstall. Final receipts come from that authoritative reducer result rather than a stale preview.

Validation on the exact three-file scope:

  • focused pytest: 9 passed;
  • five affected lifecycle smokes passed (sync, writability, retirement, project-uninstall, registry boundary);
  • Ruff lint/format, repository-native Mypy, compile checks, public/private scan, and diff checks passed;
  • change-quality receipt cqr_052c37184643d8adc555 is valid with no blockers or manual holds.

The standard canary selected one unrelated failing check: install-local-smoke expects the old PR-review skill short description. The same assertion fails on clean origin/main (8a04d1c84); packaged install, update, maintainability, and every registry-specific check pass. I have not mixed that baseline repair into this contributor PR.

Coverage is sufficient for this mutation boundary because it combines deterministic interleaving tests for all three writers, a real cross-process contention test, lifecycle smokes, and the unchanged graceful write-denied path.

Thank you @shani-singh1 for the strong reproduction and original fix. The report caught a real silent-loss condition in routine multi-project operation.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after maintainer refinement on exact head 248ab5806dc95e7b54c85662de8424465210d52a.

The original silent lost-update race, the uncovered project-uninstall writer, and stale retirement eligibility are all covered by one canonical transaction boundary plus deterministic negative regressions. Focused tests, affected lifecycle smokes, Ruff, Mypy, public-boundary scanning, and exact-scope quality qualification pass. The only standard-canary failure is an independently reproduced origin/main assertion drift in install metadata and is unrelated to this PR.

@huangruiteng
huangruiteng merged commit 3067f2c into huangruiteng:main Aug 7, 2026
@shani-singh1

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review and for carrying it to completion.

Both P1s were fair catches, and the first one was a real gap in my analysis rather than a scoping choice. I audited the writers reachable through global_registry.py and confirmed nothing else locked that path, but I never asked the inverse question — who else replaces registry.global.json without going through this module at all. project_uninstall.uninstall_project does exactly that, so my lock closed two of the three writers and left the invariant still breakable. The lesson I'm taking: for a shared-file mutation boundary, enumerate writers by target path, not by module.

The mutate_global_registry(global_path, operation, reducer) primitive is a better shape than what I submitted. Concentrating lock → load → validate/reduce → backup → write in one owner makes "every writer participates in the same transaction" a property that can be tested once, instead of a discipline each call site has to remember — which is precisely how the uninstall writer slipped through. Deriving the receipt from the authoritative reducer result rather than the pre-lock preview also removes an inconsistency I had left in place: my version still reported a preview-derived payload on the write-denied path.

On the retirement eligibility point — agreed, and I had the ordering backwards. I treated the inspection as a validation step and the lock as protecting only the write, when the eligibility check is itself part of the transaction and has to be re-evaluated against the snapshot loaded under the lock. Re-validating a goal that became live again inside the window is the correct fail-closed behavior.

Noted on the install-local-smoke baseline drift being independently reproduced on origin/main and deliberately kept out of this PR — that separation is the right call, and I see it has since landed in #2865.

Happy to keep contributing in this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(global-registry): concurrent syncs silently drop goals from registry.global.json

2 participants