fix(coordination): keep the held fence when the authority source changed - #4429
Conversation
`captureLeaseWrite` revalidates the authority source receipts before a fence-close lease write. That check is retryable by design: the adapter answers `authority_source_changed` by re-reading the canonical graph and re-sending the same held fence. The throw landed inside `fenceClose`'s claim window, so the `finally` released the mutation lock that carries the fence token. The lease write never ran, leaving an active lease on disk, and the caller's retry was answered with `fence_token_invalid`. A completed Todo then kept an active lease until an explicit release or TTL expiry. Only the shadow-enabled path revalidates, so both shadow-disabled paths released normally, and inspect/qualify still reported matched because both sides observed the same active lease -- parity alone could not see it. Release only this attempt's claim for that one retryable code and leave the fence held, reusing the claim-versus-lock distinction this file already draws when a verify attempt does not own the lock it claimed. Source checks are unchanged. Signed-off-by: song <liusongstep@gmail.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
#4358 在 fence-close 的 lease 写入前加了 revalidateAuthoritySources:当 canonical registry 的字节在验证之后发生变化时抛 authority_source_changed。这个失败按设计是可重试的——Python adapter 会重新读取权威图,然后用同一个持有的 fence 重发关闭请求。但抛出点落在 fenceClose 的 claim 窗口内,原来的 finally 会对任何异常调用 releaseFileMutationLock,而它会退掉承载 fence token 的 *.ts-effect.lock(不只是本次 claim 的 inode)。结果:一次"决策事实未变、仅源文件字节变了"的等价重写,就会让调用方的合法重试拿到 fence_token_invalid,并把已完成 Todo 的 active lease 卡住。这是真实的功能性 P1,不是清理细节。
改动思路
只在 catch 里补一个"这一次失败是不是调用方会用重试回答的那一个"的判断,命中时释放本次 claim(尽力而为)并把 claim 置空,让 finally 不去退锁;其余异常路径保持原样继续退锁。判断依据复用既有类型:revalidateAuthoritySources 只有一处抛出点(task_lease_acquire.ts:976-981),抛的正是 TaskLeaseAcquireError 且 code 为 authority_source_changed,所以 instanceof + code 比对是精确的,没有新增错误分类或重试策略。claim-only 清理也沿用本文件既有先例(:2360-2385 同一模式:best-effort catch + 由 token/PID 保证可回收)。边界保持在 TypeScript owner 内,没有让 Python 侧补偿 fence 生命周期。
具体改动
loopx/control_plane/work_items/task_lease_lifecycle.ts:2506:新增isRetryableAuthoritySourceMismatch(error),并写明它必须与"真正终结 fence 的冲突"保持可区分。- 同文件
:2713-2729:fenceClose增加 catch——命中可重试码时await releaseFileMutationLockClaim(claim)(异常吞掉,不覆盖原始错误)并claim = null,随后原样 rethrow;finally因claim为空而跳过releaseFileMutationLock。 tests/control_plane_ts/task_lease_lifecycle.test.ts:1636:新增端到端回归——acquire → holder_verify 拿到 fence → 在 close 前把权威源等价改写 → 首次 close 断言ok=false、error_code=authority_source_changed且 lease 仍为 active → 用刷新后的 source receipt 重试同一 fence,断言ok=true、released=true、lease 变为 released。
对主干的风险
- 方向性风险是"该终结的失败也被留下 fence":但谓词只匹配唯一生产者产出的单一 code,其他失败(例如写入失败)仍走原路径退锁,既有用例
fence close write failure leaves the lease unchanged and unlocks(测试文件 :596)与interruptedClose/concurrentRetry的fence_token_invalid用例(:857、:943)都仍然通过。 - 次生风险是"调用方永不重试时 fence 一直持有、lease 一直 active";这是原契约里 held fence 的既有语义,本 PR 没有引入新的无界状态,但没有对"多久必须重试"加界。
- 改动落在一个约 2700 行的热文件里,但只有 13 行、一个本地谓词加一个 catch,没有新模块或新抽象;该文件真正需要的是抽出 fence-close 事务级结构,那属于更大且独立的改动,不应塞进本 PR。
验证(exact head e8969c689):
node --no-warnings --experimental-sqlite --experimental-strip-types --test tests/control_plane_ts/task_lease_lifecycle.test.ts→ tests 40 / pass 40 / fail 0。- 反证实验:把 head 的测试文件放到 base(c979cf11c)上跑,新用例在重试断言处失败(
false !== true),说明修复与用例都是承重的、不是装饰。 tsc --project tsconfig.control-plane.json --noEmit→ 退出码 0,无诊断。- 远端检查:该 head 的 workflow 检查(windows-powershell、stage2c-correctness-e2e、各分片等)为 pass。
我的整体评价
这是典型的"新加的校验把既有重试契约打断"的后修:定位准确(claim 窗口内抛出 → finally 退掉 token 锁)、修法最小(只对可重试码放弃本次 claim)、证据双向(head 通过 + base 失败 + 其他失败路径不受影响 + 类型检查通过)。给出 APPROVE。
两点非阻塞说明:其一,本次只覆盖了 file-backed 协调路径,PostgreSQL authority store 与 Python adapter 的重试循环未在本轮执行;其二,若后续希望更保险,可以把"可重试码"集中在错误类型定义处(而不是在调用点比较字符串),并考虑对 held fence 的存活时间给出界。
English verdict: APPROVE at exact head e8969c6. #4358's source-receipt revalidation is retryable by design, but its throw landed inside fenceClose's claim window, so the existing finally block retired the fence token and the documented retry returned fence_token_invalid while the completed Todo kept an active lease. The fix keys on the single producer of authority_source_changed (an instanceof TaskLeaseAcquireError check), drops only this attempt's claim, and leaves every other failure path unchanged: 40/40 lifecycle tests pass at this head, the new test fails at base at the retry assertion, tsc --noEmit is clean and the branch's remote checks pass. Non-blocking: only file-backed coordination was exercised, and a caller that never retries still leaves the fence held.
Summary
Post-merge fix for the P1 reported in the audit on #4358 (audited head
86640c16d11ffe390d3c9ebd8db80c6a284cf69f, merged as part of #4358).#4358 added a source-receipt revalidation before a fence-close lease write. That check is retryable by design: on
authority_source_changedthe Python adapter re-reads the canonical graph and re-sends the same held fence.The throw landed inside
fenceClose's claim window, andreleaseFileMutationLockretires the*.ts-effect.lockthat carries the fence token — not just this attempt's claim inode. So a single equivalent registry rewrite produced:revalidateAuthoritySourcesthrows insidecaptureLeaseWrite;atomicWriteJsonnever runs, so the lease staysactiveon disk;finallyreleases the lock, retiring the caller's fence token;fence_token_invalid.Outcome:
completed=truewithreleased=falseand an active on-disk lease for a completed Todo, recoverable only by an explicit release or TTL expiry. Only the shadow-enabled path revalidates, so both shadow-disabled paths released normally, andinspect/qualifystill reportedmatchedbecause both sides observed the same active lease — parity alone cannot see this.Fix: for that one retryable error code, release only this attempt's claim and leave the fence held. This reuses the claim-versus-lock distinction
task_lease_lifecycle.tsalready draws infenceVerify, where a call that did not create the lock releasesreleaseFileMutationLockClaim(lockClaim)instead of the lock itself. InfenceClosethe lock is always the caller's, so retiring it is correct only once the close has actually committed.Source checks are unchanged — no weakening of continuity or source validation. Not a revert: the archive/lease correction in #4358 is kept.
Changed surfaces:
loopx/control_plane/work_items/task_lease_lifecycle.ts(+28),tests/control_plane_ts/task_lease_lifecycle.test.ts(+65). No schema, CLI, settings, or persisted-state change.Issue Or Task
Validation
e8969c6894eccd3e0bfd100bd3b79a31d897c78fregression_paritypassedfence close keeps its held fence when the authority source changedintests/control_plane_ts/task_lease_lifecycle.test.ts. Fails before the production change with the reportedfence_token_invalidon the retry and passes after, so it is a failing-before regression, not a restatement. Fullnpm run test:control-plane: baseline2b4c3cf261596 tests / 1442 passed / 132 failed; this head 1597 / 1443 / 132 — the single delta is the added case, and the 132 are identical pre-existing failures.unitpassedtests/control_plane_ts/task_lease_lifecycle.test.ts40/40 after rebase onto the current base. Covers the stale-source rejection, the lease stayingactivethrough it, and the same held fence completing the release on retry.integrationpassedtests/control_plane/test_task_lease.py,test_shadow_writer_variant_e2e.py,test_legacy_coordination_writer_fence.py,test_canonical_lease_inspection.py,test_canonical_lease_renew.py,test_local_authority_shadow_cli_e2e.py,test_shadow_management_variant_e2e.py): baseline 73 passed / 10 failed vs this head 73 passed / 10 failed — byte-identical outcome; every failure is a[sqlite]parametrization unavailable in this environment.staticpassedexamples/control_plane/control-plane-maintainability-ratchet-smoke.py: unreviewed 0, stale_exceptions 0, magnitude_regressions 0.staticpassedloopx checkover the premerge scan set returns the same single pre-existing finding as the clean baseline worktree, with no new entry.staticnot_runnpm run typecheck:control-plane— TypeScript is not installed in this environment; the same invocation fails identically on an untouched worktree, so no signal either way. Relying on CI.real_backendnot_run[sqlite]rows above fail on the untouched baseline for the same reason. Relying on required CI.manualnot_runfenceClose, reached only when a fence close withcommitted && release_leaserevalidates a changed source under an enabled runtime shadow. The added case drives exactly that path through the real lifecycle entrypoint and asserts the on-disk lease status directly rather than candidatematched, which is the observation the audit showed parity cannot make. Both baseline comparisons above used a separate clean worktree at the base commit rather than an in-place stash, so the pre-existing failure counts are directly comparable. Untested: live provider matrix, sustained soak, and Windows behavior of the lock paths. The retry budget itself (TASK_LEASE_AUTHORITY_SNAPSHOT_ATTEMPTS) is unchanged and not re-covered here.See validation disclosure guidance.
Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction