perf(authority): validate Todo ordering without serializing full records twice - #5299
Conversation
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
English verdict: APPROVE
Reviewed head: 37a8416
动机
No blocking finding. This is a justified increment toward the shared-authority read-cost checkpoint, not completion of the provider migration or cold-read program. Accumulated Todo records were serialized twice solely to establish their order. Removing that repeated work benefits retained-history workloads without hiding archived records or reducing user-visible detail. The ordinary user task remains reading complete, current status; it requires no new configuration, confirmation or recovery step.
改动思路
Reuse the existing typed identity index: it validates and copies each JSON record, rejects duplicate identities, preserves input insertion order and separately sorts identities with the established Unicode comparator. Compare these two identity sequences. Continue validating the full content digest and every record's versioned field contract. The strongest alternative is to leave the code alone until a broader request-snapshot refactor; however, this independent local simplification removes measured repeated work now without adding cache lifetime, invalidation, provider-specific policy or persisted state.
具体改动
关键代码讲解
indexRecordsincoordination_projection.ts:127is unchanged. It rejects non-array inputs, invalid JSON records and duplicate Todo IDs before returning the insertion-ordered Map. This is the necessary premise: comparing IDs would be insufficient if duplicates could overwrite earlier records.validateCoordinationTodoReadModelincoordination_projection.ts:178compares Map keys with the Unicode-sorted IDs. It still hashes every complete record, checks count/schema/manifest, and runscanonicalTodoRecord. An altered nested archived completion result still fails its digest; a matching digest does not authorize an unknown field or invalid record order.readCanonicalSnapshotFromStoreincanonical_snapshot_page.ts:61remains the production snapshot consumer throughcanonicalTodoCollection. Revision/query/store-incarnation checks and complete paging are unchanged. Both File and SQLite exercise the publicreadCanonicalSnapshotPageboundary; no synthetic reader replaces that path.
The four-file diff is +74/-7: one small runtime simplification, two parameterized negative/positive test cases, and replacement of the existing bilingual RFC checkpoint paragraph. There is no new helper/module, protocol version, cache, provider switch, Python policy owner or frontend presentation change. The future-facing pass is applied by deleting duplicate serialization in the existing owner; larger validated Todo/lease snapshot reuse remains the existing next slice.
对主干的风险
The main risk is accepting unsorted or malformed retained records because the removed byte comparison had additional validation effects. I inspected its callers and codec, then exercised 20 identical real File/SQLite public-read cases at base and head: legal Unicode order, matching-digest wrong order, duplicate IDs, altered archived content and matching-digest unknown fields, for both current and persisted canonical record forms. The complete normalized success/error observations match and failed reads do not mutate the store or return partial Todos. These complement the tests for large paged reads and concurrent revision rejection.
Local validation: TypeScript typecheck; 16 final projection tests; 306 File, 322 SQLite and 305 real PostgreSQL 16.15 tests; 38 Python snapshot/consumer tests. The real PostgreSQL run used a disposable server, with zero skips. Initial first-consumer timeout runs are retained as failures, followed by isolated focused and complete passing reruns with no timeout increase. Initial canary nested-repository discovery failed when TMPDIR was inside the checkout; using an external temporary directory passed the same smoke. The full risk-selected run passed all 19 checks and three diff checks; the quality receipt was separately renewed after unrelated main-branch advancement made the previous base binding stale.
语义与 CI 对齐
The PR reuses existing ordering, field and hash contracts. It does not weaken a budget, trim data, change rejection policy or relax privacy checks. Status parity used the same explicit product scan root on both revisions; the initial whole-repository scan hit existing negative classifier fixtures and is not represented as passing. Remote CI is not used as review evidence.
我的整体评价
Long-horizon semantics and the user journey are preserved: complete retained data, ownership, diagnostics and freshness remain available without added intervention. Ten warm component samples put validation near 42–43 ms before and 27 ms after; this is a component result. Fresh-Python status medians were approximately 1.50→1.52 s for File and 1.63→1.58 s for SQLite, so there is no defensible stable end-to-end speedup claim. Output differed only in eleven reviewed timestamp paths and retained identical byte counts. No UI companion change is needed because response shape, contents, order and paging are unchanged; no packaged frontend or installed-runtime improvement is claimed.
The compatibility assessment retains the existing persisted field-contract readers; this PR does not create a second format. The mechanism is proportionate to the measured redundancy. Residual work is the validated Todo/lease request snapshot and broader cold-read evidence, not choosing a default provider from this microbenchmark. No merge or installation has been performed; this runtime PR remains for the maintainer.
The canonical Todo validator serialized every retained record twice just to compare input order with sorted order. Its identity index has already validated each JSON record and rejected duplicate IDs. Compare that index's insertion order with the existing Unicode-sorted identities, keeping the full content digest and record-contract checks unchanged.
This advances the shared-authority RFC's local read-cost qualification (retirement cadence B, roadmap R3/R5). It removes redundant work in the common TypeScript owner used by File, SQLite and PostgreSQL. It adds no cache, schema, provider-specific branch or Python decision owner.
Validation at
37a84163fab6b4e2b607066cc90ee9a230d4b3f7:Initial broad runs included first Python-consumer timeouts. PostgreSQL passed after Effect-process isolation; File's affected pair and then its entire suite passed on repeat without changing timeouts or assertions. An initial whole-repository status scan also failed on two existing negative classifier fixtures; the matched CLI run uses the same product scan root on both revisions. These initial runs are not counted as passing evidence.
Frontend impact: no rendering or API shape changes. Full CLI response parity protects the existing status consumers; no browser layout change is claimed. Related-refactor pass removes only the redundant order proof; complete validated Todo/lease snapshot reuse remains the next separately qualified step. This PR does not retire writers, select a release default, migrate live state, or qualify the natural-time soak. Updated the existing English/Chinese RFC checkpoint; private source records and raw measurements remain outside Git.
Final qualification: all 19 risk-selected canaries and three diff checks passed. Exact-scope quality receipt
cqr_de36d49f93af78c93b6cis valid (4 files; safe fix allowed, not applied; no reported risks). The previous receipt became stale when an unrelated main-branch review-guidance change landed; scope was rechecked and qualified again. An initial nested canary temp-repository failure was resolved by using TMPDIR outside the checkout; no product assertion or timeout was weakened. Additional base/head public snapshot readback covered 20 identical real File/SQLite cases, including malformed retained state and no-write assertions.Exact-head self-review records no blocking finding; maintainer merge is still required.