Skip to content

fix(crdt): unify add fold, fix counter time-travel reconstruction, harden apply path#1615

Open
kriszyp wants to merge 3 commits into
mainfrom
kris/crdt-hardening
Open

fix(crdt): unify add fold, fix counter time-travel reconstruction, harden apply path#1615
kriszyp wants to merge 3 commits into
mainfrom
kris/crdt-hardening

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

Three CRDT fixes found during a correctness review of resources/crdt.ts + the Table.ts write path, plus two hardenings surfaced by cross-model review (Codex + Gemini + domain).

  1. Unify the add fold. A single addValues() now backs both the storage path (crdt.add via updateAndFreeze) and the read/serialize path (Addition.update). They previously diverged: a numeric-string prior value concatenated on the storage path ("5" + 3 = "53") but coerced on the read path, and a bigint folded on the storage path but threw +bigint on the read path.

  2. Fix counter time-travel reconstruction in getRecordAtTime. Fields left "unknown" by the reverse walk (a newer plain set has no inverse) are now resolved by forward-replaying from the nearest base (reconstructForward) instead of copying the nearest audit entry's raw field. The old code returned an unresolved { __op__: 'add', value: N } object (or a single delta) for a counter later overwritten by a plain set. Also switched the fill to Object.hasOwn so an unknown field named like a prototype member isn't filled from the prototype chain.

  3. Harden the apply path against unrecognized ops. updateAndFreeze now warns + skips an unknown __op__ (corruption, or a newer-node op type) instead of throwing — a throw here aborts the commit and can wedge a replicated subscription. Ops are resolved against the explicit operations registry rather than the module export namespace, so a crafted __op__ naming another export (addValues, getRecordAtTime, …) can't be invoked with the wrong arguments. Read-path reconstruction (applyForward/applyReverse) still throws loudly by design.

Where to look

Related: partially addresses #1413; a follow-up design issue for a state-based counter (which would retire the best-effort dedup this hardening lives alongside) is #1614.

Unit tests cover all three fixes plus the two hardenings (30 in crdt.test.js + tracked.test.js).

Generated by Claude (Opus 4.8).

…rden apply path

Three fixes to CRDT handling found during a correctness review, plus two
hardenings surfaced by the cross-model review:

- Unify the numeric `add` fold behind a single `addValues()` shared by the
  storage path (crdt.add via updateAndFreeze) and the read/serialize path
  (Addition.update). Previously they diverged: a numeric string concatenated on
  one and coerced on the other, and a bigint folded on one but threw `+bigint`
  on the other.

- getRecordAtTime: reconstruct "unknown" fields (a newer plain set with no
  inverse) by forward-replaying from the nearest base (reconstructForward)
  instead of copying the nearest audit entry's raw field, which returned an
  unresolved `{__op__}` object (or a single delta) for a counter later
  overwritten by a plain set. Partially addresses the reverse-reconstruction
  family in #1413 (a)/(b); pruned-history and out-of-order-branch residuals
  remain there. Use Object.hasOwn so an unknown field named like a prototype
  member isn't filled from the prototype chain. Pruned-history "keep live value"
  behavior preserved.

- updateAndFreeze: an unrecognized CRDT op (corruption / newer-node op type) now
  warns and skips on the write/replication apply path instead of throwing, so it
  can't abort a commit and wedge a subscription. Resolve ops against the explicit
  `operations` registry rather than the crdt.ts export namespace, so a crafted
  `__op__` naming another export (addValues, getRecordAtTime, …) can't be invoked
  with the wrong arguments. Read-path reconstruction still throws loudly by design.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gemini-code-assist[bot]

This comment was marked as resolved.

@claude

This comment has been minimized.

…olding

Address cross-model review comments on #1615:
- operations registry is now a null-prototype object, so a crafted `__op__`
  naming an Object.prototype member (toString, valueOf, constructor) resolves to
  undefined rather than an inherited function in applyForward/applyReverse and
  updateAndFreeze.
- addValues: explicitly establish from a null/undefined prior (avoids `0 + 3n`
  TypeError), and keep a number-typed field a number when a stray bigint delta
  arrives instead of throwing a number+bigint TypeError on the apply path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp kriszyp marked this pull request as ready for review July 6, 2026 15:46
@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

AI-generated (Claude Opus 4.8).

CI note for reviewers — the red checks are pre-existing flakes, not this change.

  • Unit Test (v22/v24/v26): the only failure is unitTests/resources/subscriptionReplay.test.js"audit records of types not in ACTIONS_OF_INTEREST are still delivered by cursor" (a race-condition/timing test). This same test is currently failing on main (Unit Test run 28800905069, 2026-07-06) with no changes from this PR — so it's a pre-existing flake this branch merely inherits. It also passed 6/6 locally on this branch (including that exact sub-test), and the suite has a documented flaky history (test: replace fixed-delay sleep-then-assert races in subscriptionReplay #1219, "replace fixed-delay sleep-then-assert races").

  • Integration Tests 3/6: the earlier doorlock schema missing / 404 was a schema-deploy race — it passed green on re-run.

There is no causal path from this diff (resources/crdt.ts, resources/tracked.ts) to audit-cursor delivery counts or schema deployment. The change's own coverage — all crdt/tracked unit tests, every build, and all integration shards — is green.

Marking Ready for Review.

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.

1 participant