fix(coordination): give the delta-reconstruction proof one owner - #4468
Conversation
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
Self-review — JSON key fidelity and the pre-swap reconstruction proof (head a39c2443c)
Re-read the final diff against origin/main 26eaefc21, starting from the reviewer's two-layer diagnosis and checking each layer separately.
Layer 1 — encoding. A delta path segment is now any string, and object reads/writes go through own data properties rather than container[key] / container[last] = value. I checked the other direction too: what else could the old code get wrong for a legal key? A 13-key probe over a real migrated database (inherited names constructor/toString/hasOwnProperty/valueOf/__defineGetter__/prototype, "\u0000" and embedded NUL, "\u2028", an emoji, "é", "", "__proto__") migrates and reads back current status and history correctly, with Object.prototype empty afterwards. Only "" and __proto__ were broken before; the fix is exactly the key space the review identified, and it cannot weaken a check because reads are still guarded by Object.hasOwn and writes still define enumerable own properties.
Layer 2 — the migration's missing proof. verifyMigratedStore compared copied cursor, operation id and commit digest sequence, so a log the V2 read path rejects could still be committed. authorityStateDeltaReconstructs now proves delta-vs-projection byte-for-byte before the swap, and both the live commit path and the migration call that one function instead of holding two copies of the rule. I verified the failure mode is safe rather than assumed: re-reverting only the empty-key path check makes the migration report status=failed reason=migration_protocol_violation with the V1 tables (commits, head, metadata) untouched and no V2 tables created. The throw happens inside BEGIN IMMEDIATE, so the rollback path leaves V1 exact.
What could still be wrong that this PR does not claim to fix. An undecodable or inapplicable delta now reports a failed reconstruction rather than a distinct error, which is intentional and documented, but it does mean a future codec bug surfaces as a generic migration failure message rather than a precise one. A real production-scale cutover of a live goal was not performed - validation is isolated temp databases with synthetic projections, as the repository's evidence boundary requires.
Checks. Targeted suites on real SQLite: 124 pass / 0 fail; typecheck:control-plane clean; full test:control-plane compared test-for-test against a pristine origin/main baseline (identical 14 pre-existing failures, +3 new tests); counterexample, fidelity and fail-closed probes re-run on the rebased head.
Verdict: both layers of the reported defect are fixed with one owner each and load-bearing tests; no blockers found in self-review.
|
One more piece of context for this fix: the reference doc already promised the property the code did not enforce. |
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Exact head reviewed: b588445ea51b810e9a154cef1f258e950045aaf4
动机
上一轮独立评审在真实 SQLite 上发现 V1 → V2 权威状态日志迁移的两个契约缺口：一个已经提交的 V1 投影，如果它的 JSON 对象键是 "" 或 __proto__，迁移会"成功"，但迁移后的 V2 库读不回来——"" 会让当前状态与历史读取都失败（authority state delta operation 0 path segment is invalid），__proto__ 会让历史读取失败（读写走了原型链，键丢失或容器原型被替换）。两层问题都真实存在：新增量编码没有完整保留旧版本接受的键语义，而迁移从未证明"写进去的增量能还原它声称描述的投影"，后者只核对了复制过去的游标、操作 id 与提交摘要。
这条路径的核心承诺就是"旧版本合法数据迁移后仍然可读"，而它正是默认 provider 接下来要依赖的路径。影响面是每个使用 SQLite 权威存储的 goal，以及其后所有读取 head/history 的调用者；不做这件事的代价是问题会在切换默认 provider 之后才暴露，届时只能从一份读者拒绝的日志里恢复。修法也已经是最小形态：两条缺陷各一条规则、各一个 owner——键一律按 own data property 读写，重建判定收敛成一个共享规则并由两个写入方在落盘前各自证明。其他备选（迁移时拒绝这类键、或给迁移单独再写一份校验）分别会丢数据和重新制造漂移。
改动思路
入口有两个，都是写入方：实时提交路径 sqlite_authority_store.ts:465 和 V1 → V2 迁移 sqlite_authority_migration.ts:201。权威输入是"规范化后的投影"和"它上一版投影"，增量只是描述前者如何变成后者的表达式，所以判断标准只能是字节级重建。决定者集中在 authority_state_log.ts：编码 authorityStateDelta、解码 decodeAuthorityStateDelta、应用 applyAuthorityStateDelta，以及本次新增的重建判定 authorityStateDeltaReconstructs。
正路径是：投影编码成增量 → 经存储边界 JSON.stringify/JSON.parse → 解码时接受任意字符串路径段（含空串）→ 应用时用 Object.defineProperty 写 own data property → 用 canonicalAuthorityBytes 比对，必须完全一致。副作用只有一个：写入方式从 container[key] = value 改成定义自有属性，因此 __proto__ 不再触发原型 setter，容器保持朴素对象。
与既有实现的关系是这次改动的重点：重建判定原本只存在于 store 的私有内联比较里，迁移侧只做了弱检查；现在两边都调用同一个导出规则，迁移在换表之前逐个证明，失败就在事务里抛出，V1 原样保留。set 的值仍然走 structuredClone，我单独验证过带 own __proto__ 键的对象经它复制、经 JSON 往返后仍是 own key，且没有污染 Object.prototype。
具体改动
6 个文件：loopx/control_plane/coordination/authority_state_log.ts（新增 ownValue、defineOwnValue、authorityStateDeltaReconstructs；放宽路径段；读写改 own property）、sqlite_authority_migration.ts（换表前的重建证明）、sqlite_authority_store.ts（改为调用共享规则）、以及对应的三个测试文件。增量格式、摘要、游标、检查点布局、协议字段与默认 provider 行为都没有变化。
关键代码讲解
ownValue（loopx/control_plane/coordination/authority_state_log.ts:66）：用 Object.getOwnPropertyDescriptor 读值，替代 container[key]。后者对 __proto__ 会返回继承来的原型对象，读取就"看起来有值"，这也是原缺陷能静默通过的一部分。改后读取只认自己拥有的属性。
defineOwnValue（loopx/control_plane/coordination/authority_state_log.ts:79）：用 Object.defineProperty 写 {value, writable: true, enumerable: true, configurable: true}，替代 container[last] = structuredClone(value)。普通赋值对 __proto__ 会调用原型 setter：键没被存下、容器原型被替换。我的探针断言改写后 Object.hasOwn(nested, "__proto__") 为真、Object.getPrototypeOf(nested) === Object.prototype、且 Object.prototype 仍是空对象。
authorityStateDeltaReconstructs（loopx/control_plane/coordination/authority_state_log.ts:200）：把"这份增量精确重建了这份投影"收敛成一个 owner，比较的是 canonicalAuthorityBytes。不可解码或不适用都算"重建失败"而不是另一种结果，调用方一律 fail closed——store 拒绝提交，迁移在事务内抛出。这消除了迁移侧此前"只核对身份序列"的弱检查。
decodeAuthorityStatePath（loopx/control_plane/coordination/authority_state_log.ts:284）：路径段判定从"非空字符串"改为"任意字符串"。这是本次唯一的输入放宽，依据是旧版本接受这类键，且编码器本来就会为它们生成路径；非字符串段仍然报协议错误。
迁移的换表前证明（loopx/control_plane/coordination/sqlite_authority_migration.ts:201）：写出增量后立刻用冻结的 V1 投影证明它可重建，不成立就抛 V1 authority state delta does not reconstruct its commit，事务回滚、V1 不动。这条是"迁移后的库能读回来"这句承诺第一次被机器检查。
对主干的风险
非阻断（P3）——诊断信息变粗。 共享规则用 catch { return false; } 把所有失败折叠成同一种结果，于是"增量无法解码（schema/路径违规）"与"能解码但描述的是另一份投影"现在报同一句 ... does not reconstruct its commit；改动前 store 路径会把解码器的具体消息透出来。fail-closed 的行为是对的，丢掉的只是排障细节。可选修法：让规则返回原因（或把原始错误挂到 cause），写入方据此区分两类失败。
非阻断（P3）——本次在评审环境无法复跑真实 SQLite 层，需在合格运行时补证。 本机两个运行时（Node 25.5.0、Node 22.22.0）带的 SQLite 都是 3.51.2，低于仓库要求的 3.51.3+，所以所有 SQLite 用例都以 SQLite authority runtime is not qualified (SQLite 3.51.2 ...); require ... 3.51.3+ ... use the qualified Node 22.22.3 runtime fail closed：merge base 121 tests / 3 pass / 118 fail，本 PR head 124 tests / 4 pass / 120 fail，差值正好是新增的 3 个用例与 1 个新通过的 codec 用例，属于环境限制而非本 PR 引入。因此在合并前（或任何默认 provider 切换前），请在合格运行时上重跑迁移与存储两个套件并把结果附到 PR 上。我在这一层只能给出源码级核对与作者自述的 124 pass / 0 fail。
残余风险与证据边界。 我实跑的是：head 与 merge base 上同一份探针（head 9/9 通过：字节级重建、空键可读、嵌套 __proto__ 为 own key、无原型污染、JSON 往返保留两键、重建规则对真实增量为真/对被篡改与不可解码增量为假、删除空键表达为同路径 remove；merge base 在第一步就以 authority state delta operation 0 path segment is invalid 失败）、npm run typecheck:control-plane（干净）、loopx canary premerge --from-git-diff（exit 0，5 条 catalog canary + 8 条 risk-profile smoke 全过，无人工 hold），以及 head/base 两侧的三个 TS 用例文件对比。没有复跑的是合格运行时上的真实 SQLite 迁移/存储套件（环境不具备），这也是上面第二条 P3 的内容。
我的整体评价
这次修复把两条缺陷各自收到一条规则、一个 owner 上，而且把"迁移后的库必须能读回来"从人工检查变成了落盘前的机器证明——这正是上一轮评审要求的形状：不是给旧路径加更多检查，而是让新格式被迫证明自己。我在 exact head 上独立复现了两个方向：head 上带 "" 与嵌套 __proto__ 的投影编码→存储往返→重建完全一致且不污染原型；merge base 上同一份投影在解码阶段就失败，说明这条回归确实是被这次改动关掉的。类型检查干净、仓库 pre-merge 门 0 失败。
需要保留的诚实边界只有一个：存储层（真实 SQLite 的迁移与提交路径）在本机运行时不合格，无法复跑，只能依赖作者的合格运行时结果与源码核对；请在合并前补上那次复跑。剩余 P3 是诊断粒度，不构成阻断。下一次评审请从新的 exact head 开始。
English verdict: APPROVE — exact head b588445ea51b810e9a154cef1f258e950045aaf4 of #4468. The fix addresses both reported defects with one rule each: object access goes through own property descriptors (ownValue/defineOwnValue) so __proto__ is a stored key rather than a prototype write, path segments accept any string so the empty key survives the storage boundary, and authorityStateDeltaReconstructs is now the single owner of "this delta rebuilds exactly this projection", called by the live commit path and by the migration before its table swap (failing closed inside the transaction and leaving V1 untouched). Independently reproduced at this head: a projection with an empty-string key and a nested __proto__ key encodes, survives a JSON storage round trip and rebuilds byte-for-byte, both keys stay own data properties on plain objects, Object.prototype is untouched, and the reconstruction rule is true for the real delta and false for tampered and undecodable ones — while the same probe against the merge base fails immediately with authority state delta operation 0 path segment is invalid. npm run typecheck:control-plane is clean and loopx canary premerge --from-git-diff exits 0 (5 catalog canaries, 8 risk-profile smokes, no manual holds). Two non-blocking P3s: the shared rule collapses "undecodable" and "mismatched" into one generic message (optional: carry the reason/cause), and the real-SQLite migration/store suites could not be re-run here because neither installed runtime qualifies (Node 25.5.0 and Node 22.22.0 both ship SQLite 3.51.2, below 3.51.3+; base 118 failures vs head 120, the delta being the three new tests plus one newly passing codec test), so a qualified Node 22.22.3 run of tests/control_plane_ts/sqlite_authority_migration.test.ts, sqlite_authority_store.test.ts and authority_state_log.test.ts should be attached before merge. No storage format, digest, protocol field, or default-provider behavior changes.
The live SQLite writer and the V1 migration each proved that the delta they were about to persist really rebuilds the projection they were publishing, but as two inline comparisons in two different forms: canonical bytes against the in-memory next state in the writer, and a recomputed digest in the migration. Same rule, two copies, already diverged in what they compare. `authorityStateDeltaReconstructs(previous, delta, projection)` is now the one owner of that proof and both callers use it. A delta that cannot be decoded, or that does not apply to `previous`, is a failed reconstruction instead of a thrown error, so each caller still fails closed with its own message. Coverage the live path was missing: the writer now commits and reads back a key space carrying `""` and `__proto__` through real SQLite. The migration path already carries that coverage, and the rule's own contract is now unit-tested for a keyed projection plus mismatched, undecodable and inapplicable deltas. The empty-key/`__proto__` migration defect itself is already fixed on main; this change removes the duplicated proof and closes the writer-path coverage gap that remained. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
changed the title
b588445 to
0807fd0
Compare
Self-merge validation (0807fd0)Owner-authorized self-merge. Before merging:
One failing job, and why it is not this diff
Superseded scopeThe migration defect this branch originally reported is already fixed on main (#4497). This PR carries only the residual: one owner for the "this delta rebuilds exactly this projection" proof, plus the live-writer key-space coverage that main was missing. |
1 participant
Summary
The migration defect this branch originally reported — a committed V1 projection keyed with
""or__proto__migrating "successfully" into a state log the V2 read path could not read — is already fixed on main (#4497: any string is a legal path segment, decoded keys are written as own data properties, and the migration replays its rows through the store decoder before swapping tables).This PR carries only what is still missing after that fix:
authorityStateDeltaReconstructs(previous, delta, projection)is now the single owner and both callers use it. A delta that cannot be decoded, or that does not apply toprevious, is a failed reconstruction rather than a thrown error, so each caller still fails closed with its own message.""and__proto__(head plus retained history byte-equal, no prototype pollution).Validation
unitpassedauthority_state_log+sqlite_authority_migration+sqlite_authority_storesuites on real SQLite with qualified Node — 125 pass / 0 failunitpassednpm run test:control-plane— 1622 pass / 14 fail / 3 skip. The 14 failures are byte-identical to a pristineorigin/mainrun of the same suite (Python/TS entry identity, transport aliases, successor normalization, Python CLI--from-contextcases and--format digest), i.e. pre-existing and environmental, not caused by this diffstaticpassednpm run typecheck:control-plane;loopx canary premerge --from-git-diff—status: passed,merge_gate_passed: true,manual_holds: 0, 5 changed files, surfacecontrol_planereal_backendpassedreal_backendblockedpostgresql-authority (real server)job covers the shared codec against a real server