fix(goals): bind lifecycle confirmation to source registry - #4949
huangruiteng merged 6 commits into
Conversation
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
@huangruiteng Please re-review this control-plane fix at exact head The latest commit resolves the P1 lock-order finding:
The prior authority-route fix remains intact: the versioned fingerprint binds Goal id, normalized source identity, and source content revision; equal-content A→B route switches and route races are rejected without writes; same-route lost-response recovery remains allowed. Exact-head local verification:
All commits are signed off and the branch was updated with a normal push. CI has started for the new exact head. Related baseline follow-up: #4986 now passes the complete Windows PowerShell lifecycle, native scheduler/SQLite, source rebuild, and bundle verification path. Its current shard-4 failure is an unrelated Exact-head CI update (09:20 UTC): shard 4 completed with 1 failed and 2935 passed. The only failure is the project registry I/O manifest census. It reports the four main-branch metadata moves tracked by #4990 plus eight #4949-specific activation/global-registry sites (one new site and seven moved sites). No behavioral test failed. After #4990 and the Windows baseline fix #4986 land, this branch should merge the new main and regenerate the manifest once; changing it on the older base now would create avoidable churn. |
huangruiteng
left a comment
There was a problem hiding this comment.
动机
Exact head: 85bd51805a2e7c96f24ba12a069128480cd9ab80(base eb16c5443)。这个修复针对 Chat 的 goal.lifecycle:旧预览保存的是全局只读投影的摘要,真正的 stop/resume 却写项目源注册表。源注册表在确认后独立变化时,旧实现会把过期确认继续执行。这个问题值得修,但当前 head 还不能证明“确认绑定到所预览的源”。
改动思路
保留 set_goal_activation_state() 作为激活状态的单一写入与读回 owner;Chat 预览改读其源注册表摘要,apply 将预期摘要传入源注册表事务内做 CAS。正常路径:预览源 A → 源 A 内容变化 → apply 返回 stale,不写激活状态;已执行但响应丢失时,目标状态已成立可返回 already。删除仍走原来的全局注册表 CAS。这是对现有 owner 的复用,规模基本合适。
具体改动
关键代码讲解
chat_actions.py的预览分支把 stop/resume 接到_goal_lifecycle_preview_fingerprint,并把确认权限标为 durable write。chat_goal_lifecycle_actions.py的新 helper 从激活服务取observed_state_fingerprint;_apply_goal_lifecycle在执行前重读、判 stale/重放,并把预期摘要交给set_goal_activation_state()。activation_service.py原有源事务负责最终 CAS 与源/全局读回;测试新增源内容变更、锁前竞争及已提交后响应丢失三类回归。生产改动没有引入第二个写入 owner,也没有新的版本化兼容层。
对主干的风险
P1 阻断:摘要绑定了内容,没有绑定源路由。 我用真实的临时项目/全局注册表做 A/B 对照:在预览源 A 后,把同字节内容复制到源 B,仅将全局 Goal 的 source_registry 从 A 改为 B,再确认 stop。基线返回 stale,A/B 都保持 active;本 head 返回 applied,A 仍 active,B 却被 stopped。原因是新 helper 只存源文件 SHA-256(chat_goal_lifecycle_actions.py:34),apply 重新解析路由后,B 的相同摘要通过了比较和源 CAS。读回验证的是 B 与全局投影一致,无法证明 B 是用户确认时看到的 A。请把规范化的源身份与内容修订一起绑定到 proposal,并在应用/同步边界重新校验;补 A→B 等内容切换的零写入 stale 回归,并覆盖路由校验与写入间的竞争。
语义与 CI 对齐
受影响的是现有“owner 确认同一权威 Goal 源”的权限语义,不是新增 vocabulary。聚焦测试 42/42、Ruff、diff check、premerge 2/2 均通过,且当前 main 可干净合并;新增的源内容变更测试在基线失败、在 head 通过,但未覆盖上述路由反例。Goal 配置 wait_for_ci=false,因此未轮询远端 CI。这里未见 default-off、通用控制面措辞或 guidance/obligation 的额外问题。
我的整体评价
REQUEST_CHANGES。实现方向和复用边界正确,但 head 相比基线引入了可复现的 authority-route 回归,现有绿灯不足以批准。修复后请在同一真实入口重跑 A/B 反例、现有生命周期套件和 premerge;顺手与 #4950 共用一个有类型的“源身份 + 修订”确认依据,比各自继续增加内容摘要分支更易维护。这个关联的小重构适合与修复同批完成;无须扩成全面 TS 迁移。
English verdict: REQUEST_CHANGES - head 85bd51805a2e7c96f24ba12a069128480cd9ab80 passes focused tests and premerge, but an equal-content source-route switch applies the confirmed stop to an unreviewed source; bind and recheck route identity.
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
…cle-source-cas Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
…cle-source-cas Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
复审 #4949 精确 head 978d4d9432d0957d0a3fec64aa81fe95749cd7a1(base 25b9f73661bc7b8243c38b5b57f5e312318b3fee)。Chat 旧的 Goal stop/resume 确认绑定全局投影摘要,却写项目源注册表;上一轮的内容摘要修复又允许“源 A 换成同内容源 B”后继续执行。当前 head 把源身份与内容修订一起绑定,并在写入时复核,这个目标和复用现有激活服务的方向是成立的。
改动思路
预览从源路由生成版本化 fingerprint,Chat proposal 保存源身份依据;应用时先判路由与摘要,真正执行则在源与全局锁内重验路由、源内容,提交后同步全局投影并读回。已在同一源完成但响应丢失的重放仍可恢复;delete 保留原全局 CAS。operator_actions 使用同一 fingerprint,global_registry 拆出已持锁的 reducer,避免同步时重入全局锁。项目源仍是状态权威,全局注册表仍是投影。
具体改动
整份 base-to-head diff 是 7 个文件,约 +664/-85:6 个生产文件覆盖 Chat、CLI help、激活服务、动作目录及全局同步;test_goal_activation.py 增加 362 行源变更、等内容换路由、锁前竞争及响应丢失回归。
关键代码讲解
ChatGoalLifecycleActionMixin._goal_lifecycle_preview为 stop/resume 取源身份和修订;_apply_goal_lifecycle只在同一路由允许已提交状态的重放,其他变化判 stale。goal_activation_source_fingerprint把 Goal id、规范化源身份、源内容摘要绑定;set_goal_activation_state在持锁事务内再次比较,随后同步并读回。sync_project_registry_to_global的已持锁分支复用原 reducer,没有另建全局状态写入 owner。- 但
set_goal_activation_state第 386–400 行按路径字典序取两把锁;现存register_agent_via_source_registry第 210–267 行先持项目源事务,再同步并等待全局锁。两者顺序不一致。
对主干的风险
[P1] 新锁顺序与现存注册 Agent 的写入路径形成环形等待。 常见的全局路径排在项目源路径之前时,stop/resume 先持全局锁、等待源锁;同时 register-agent 先持源锁、等待全局锁。用隔离的合成项目/全局注册表和真实文件锁强制两条生产入口重叠,27 个激活测试与 8 个 TS 动作测试都通过,但 stop 最终抛出 LockAcquireTimeoutError(约 5 秒),注册操作在它释放全局锁后才成功。为隔离锁序,此并发复现只替换了前置的可写性探针;源/全局事务及文件锁是真实的。请让两个写入入口遵守同一源→全局顺序,或统一改成覆盖所有写入者的确定顺序;保留锁内路由 CAS、同步与读回。补并发回归:两个操作均完成且各自状态读回正确,不得以超时作为正常串行化。
语义与 CI 对齐
旧的等内容 A→B 反例在上一版 85bd51805 上仍会把 proposal 判为 applied,在本 head 已返回 stale;新回归确实关掉了上一轮权限缺口。当前阻断是共享 registry 写入/锁序契约的另一处回归,并非新增语义词汇。精确 head 的激活测试 27/27、TS 动作测试 8/8、Ruff、control-plane typecheck 与 diff check 通过;Goal 配置 wait_for_ci=false,未轮询远端 CI。并发负例失败不能被这些绿灯抵消。
我的整体评价
REQUEST_CHANGES。 源路由绑定及同源响应丢失恢复比上轮完整,代码继续复用现有 owner,规模对权限敏感的跨入口修复也基本相称;但锁顺序使普通并发维护操作可让 owner stop/resume 失败。最小相关重构是共享跨注册表锁序,而不是增加新的状态或全面迁移。修复后请在新 exact head 重跑并发反例、完整激活套件和动作计划测试,再做主干集成判断。
English verdict: REQUEST_CHANGES - head 978d4d9432d0957d0a3fec64aa81fe95749cd7a1 fixes the prior equal-content source-route switch, but its global-then-source locking conflicts with the existing source-then-global register-agent path; a real-file concurrent smoke made owner stop time out while focused tests passed.
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
复审 #4949 精确 head ac89af887486946ca372effde87652464674c773(base 25b9f73661bc7b8243c38b5b57f5e312318b3fee)。这个 PR 修复 owner 在 Chat 确认 Goal stop/resume 时的 authority gap:旧路径预览的是全局投影,真正写入的是项目源注册表,因此确认后发生 source-only 变化仍可能执行;上一轮 source-bound 修复又与现存 Agent 注册路径形成了相反的跨注册表锁序。当前 head 同时关闭了源/路由漂移与锁序死锁,交付边界完整。
改动思路
项目 registry 继续作为权威状态,全局 registry 继续作为同步投影。预览时生成绑定 Goal id、规范化 source identity 与 source content revision 的版本化 fingerprint,并在 proposal 中保存 owner 实际审阅过的 source basis;执行时先取项目源事务,再取全局投影锁,在两把锁内复核 route 与 source revision,提交源状态、用已持锁 reducer 同步全局并读回。Chat 的 response-loss recovery 只允许“同一 source 且目标状态已完成”,delete 保留原来的全局 CAS。
反对上线的最强理由是:为一个确认竞态引入约 670 行净增量和内部 held-lock seam,若只是换 hash 会更小。但内容 hash 无法区分等内容 A→B,global-only CAS 无法感知 source-only change,按路径排序又与已有 register_agent_via_source_registry 的 source→global 顺序冲突。当前实现把规则放回既有 set_goal_activation_state / registry reducer owner,没有新增第二套状态或策略引擎,因此机制与风险相称。
具体改动
整份 base-to-head diff 修改 7 个文件(+744/-73):6 个生产文件覆盖 Chat proposal、CLI 提示、activation owner、operator catalog 与全局同步;test_goal_activation.py 增加 461 行真实文件回归,覆盖源变化、等内容换路由、写锁前竞态、响应丢失和并发锁序。
关键代码讲解
goal_activation_source_fingerprint(activation_service.py:100)把 Goal id、source identity 和 source bytes digest 合成确认依据,Chat 与 operator catalog 共用同一个定义。set_goal_activation_state(activation_service.py:248)统一负责 authority route、source CAS、source→global 锁序、提交、同步及双端读回;route 或 source revision 漂移会返回goal_action_stale,不产生写入或 receipt。ChatGoalLifecycleActionMixin._apply_goal_lifecycle(chat_goal_lifecycle_actions.py:131)只在 source identity 相同且目标状态已达成时允许 response-loss recovery;其他 fingerprint 变化将 proposal 标为 stale。_mutate_global_registry_locked(global_registry.py:61)复用原 reducer/write 语义,让已持全局锁的 lifecycle 路径避免重入锁;普通调用仍由mutate_global_registry负责获取锁。
对主干的风险
上一轮 P1 已关闭:当前 head 明确采用 source→global,与 register_agent_via_source_registry 一致。新增并发回归用真实项目/全局 JSON、真实 transaction 与 file lock 强制 register-agent 和 owner stop 重叠;两者都完成,source/global 同时保留新 Agent 与 stopped 状态,读回均验证,不再出现约 5 秒的 LockAcquireTimeoutError。
源绑定也有独立反证:同一个 reviewer-owned real-file harness 在 base 25b9f7366 上把 source-only change 后的 proposal 判为 applied,在本 head 判为 stale 且无 receipt。等内容 A→B、route-before-commit、changed-source、same-route committed-response-loss 等路径都在精确 head 覆盖。
本地验证:activation 28/28、Agent registration 2/2、TS action review 8/8、control-plane typecheck、changed-file Ruff 全部通过;标准 premerge 13/13 通过,0 failure、0 warning、0 skip、0 manual hold。Goal 策略 wait_for_ci=false,因此没有轮询远端 CI。未发现 default-off 隔离、authority 命名、typed-state、domain-neutrality 或 guidance/obligation 方面的额外问题。
语义与 CI 对齐
本 PR 强化既有 goal.lifecycle / goal_action_stale 语义,不引入新的跨域 vocabulary:旧默认允许 source/route 漂移后的确认继续执行,新默认 fail stale 并要求刷新;delete、Todo、team-plan、scheduler 和 quota 语义不变。版本化 source basis 由系统自动生成,不需要用户维护第二份状态。
我的整体评价
APPROVE。 当前精确 head 修复了最初的错误 authority owner,也关闭了复审发现的 equal-content route switch 与跨 registry 死锁;生产规则仍集中在原有 lifecycle/registry owners,正负路径、真实后端、base/head 反证和风险型 premerge 均有证据。剩余风险是 _global_registry_lock_held 依赖内部调用纪律,但当前 caller search 只有在明确持锁处使用,且真实锁回归覆盖了这条 seam。该 PR 属控制面行为变更,批准不等于自合并授权,仍应按仓库政策走 maintainer merge 与 exact-head readiness。
English verdict: APPROVE - head ac89af887486946ca372effde87652464674c773 binds lifecycle confirmation to the authoritative source revision, preserves same-source recovery, aligns source-to-global lock order with Agent registration, and passes the reviewer base/head regression, focused suites, typecheck, Ruff, and 13/13 standard premerge checks.
Summary
set_goal_activation_state()so the transaction checks it again while holding the source registry lock.Root cause
ChatActionServicestored the global registry digest in a lifecycle proposal, butset_goal_activation_state()writes the project source registry. The apply path checked the global digest and then omittedexpected_state_fingerprintfrom the source transaction. A source-only change after owner confirmation therefore remained invisible, and the stale stop or resume still executed.Validation
3/3on the unfixed code because each proposal becameapplied.tests/control_plane/test_goal_activation.pypassed23/23.ChatActionServicesuites passed221/221.tests/control_plane_ts/action_review_plan.test.tspassed8/8.npm run typecheck:control-planepassed.loopx canary premerge --from-git-diffpassed both selected canaries.The project-registry I/O census still reports the same 11 metadata drifts already present on
main@eb16c5443; this diff adds no census drift.Scope
This changes the owner-confirmed Chat lifecycle path only. The typed Goal action catalog already uses the source registry digest. Todo and team-plan actions retain their own canonical revision contracts.