fix: global Lamport clock max prevents first-writer tick collisions#39
fix: global Lamport clock max prevents first-writer tick collisions#39flyingrobots wants to merge 2 commits intomainfrom
Conversation
Extract scanFrontierForMaxLamport and scanPatchesForMaxLamport as module-private functions in materialize.methods.js, replacing inline scanning loops that pushed materialize() to complexity 38 (max 35) and nesting depth 7 (max 6). No behaviour change: same logic, same _maxObservedLamport update semantics. All Lamport clock monotonicity tests continue to pass.
…S narrowing `patch.lamport` is typed `number | undefined`. The previous assignment `graph._maxObservedLamport = patch.lamport` was guarded by `?? 0` only in the condition, which TypeScript could not narrow through. Extracting `const tick = patch.lamport ?? 0` makes the type `number` at the assignment site and satisfies strict checks with no behaviour change.
📝 WalkthroughWalkthroughThis PR enhances Lamport clock monotonicity tracking in a distributed graph system by introducing global max-lamport state tracking across the WarpGraph. It reworks lamport calculation in commit paths to preserve monotonic progression and updates materialization to synchronize observed lamport ticks from frontier patches. Changes
Sequence Diagram(s)sequenceDiagram
participant Writer as Writer (PatchBuilderV2)
participant Graph as WarpGraph
participant Patch as Patch Methods
participant Materialize as Materialize
participant Reader as Reader (new instance)
Writer->>Graph: Read _maxObservedLamport
Writer->>Patch: _nextLamport(currentRef, maxObservedLamport)
Patch-->>Patch: Decode ref patch or compute fresh start
Patch-->>Patch: lamport = max(ownTick, maxObservedLamport) + 1
Patch-->>Writer: Return lamport + parentSha
Writer->>Writer: Create PatchV2, encode CBOR, commit blob/tree
Writer->>Patch: _onPatchCommitted(lamport)
Patch->>Graph: Update _maxObservedLamport = max(current, lamport)
Reader->>Materialize: materialize()
Materialize->>Materialize: Load patches from storage
Materialize->>Materialize: scanFrontierForMaxLamport(frontier)
Materialize-->>Materialize: Decode tips, extract lamport values
Materialize->>Graph: Update _maxObservedLamport from frontier
Materialize->>Materialize: scanPatchesForMaxLamport(patches)
Materialize->>Graph: Final _maxObservedLamport reflects all observed ticks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/warp/patch.methods.js (1)
88-122:⚠️ Potential issue | 🟡 MinorDocument the materialize-before-patch expectation in
createPatch()andpatch()JSDoc.While
_nextLamport()correctly integrates the global max viaMath.max(ownTick, _maxObservedLamport) + 1, the guarantee relies on a priormaterialize()call to populate_maxObservedLamport. Since neithercreatePatch()norpatch()calls_ensureFreshState()ormaterialize(), a caller who uses these directly in a multi-writer graph without materializing first will issue lamports without knowledge of other writers' observed maximums—even though commits succeed and local monotonicity is preserved.The tests and
PatchBuilderV2.commit()show this is safe in practice (the builder re-reads the writer's own ref at commit time, and the join reducer handles order). However, this expectation—thatmaterialize()should be called before committing in multi-writer scenarios, or that query methods (which auto-materialize) should be used first—is not obvious from the API. Add a note to the JSDoc of bothcreatePatch()andpatch()to clarify this precondition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/warp/patch.methods.js` around lines 88 - 122, The createPatch() and patch() JSDoc must explicitly state the precondition that callers should materialize the graph (or call _ensureFreshState()) before creating/committing patches so that _maxObservedLamport is populated and _nextLamport() can incorporate the global max; update the JSDoc on both createPatch() and patch() to note that in multi-writer scenarios callers should call materialize() (or rely on query methods that auto-materialize) prior to committing, and reference that PatchBuilderV2.commit() already re-reads the writer ref and that _nextLamport() uses Math.max(ownTick, this._maxObservedLamport) + 1 to compute the next lamport.
🧹 Nitpick comments (3)
src/domain/services/PatchBuilderV2.js (2)
544-560: Nit: duplicate step number "4" in comments.Line 544 says "4. Build PatchV2 structure" and line 560 says "4. Encode patch as CBOR". Renumber from step 4 onward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/PatchBuilderV2.js` around lines 544 - 560, The inline comments in PatchBuilderV2.js duplicate the step number "4" (one at the start of the PatchV2 construction and again before encoding), so update the comment sequence after the Build step to be sequential—e.g., keep "4. Build PatchV2 structure..." and change the later "4. Encode patch as CBOR" to "5. Encode patch as CBOR" (and increment any subsequent step numbers if present); search around the createPatchV2 call, vvSerialize, this._ops, this._reads, and this._writes usages to find and renumber the remaining step comments accordingly.
535-542: Pre-existing:decodePatchMessagewill throw if the parent is a non-patch commit.
_nextLamport()inpatch.methods.jscorrectly guards against non-patch parents by checking the message kind before decoding, but thiscommit()path unconditionally decodes the parent as a patch message. If a writer ref points to a checkpoint or anchor commit,decodePatchMessagewill throw before the commit is persisted.Since
detectMessageKindis already exported fromWarpMessageCodec.js, add the same guard here for consistency:♻️ Add a non-patch guard to match _nextLamport()
if (currentRefSha) { - // Read the current patch commit to get its lamport timestamp and take the max, - // so the chain stays monotonic even if the ref advanced since createPatch(). const commitMessage = await this._persistence.showNode(currentRefSha); + const kind = detectMessageKind(commitMessage); + if (kind === 'patch') { const patchInfo = decodePatchMessage(commitMessage); lamport = Math.max(this._lamport, patchInfo.lamport + 1); + } parentCommit = currentRefSha; }Update the import from
WarpMessageCodec.jsto includedetectMessageKind:-import { encodePatchMessage, decodePatchMessage } from './WarpMessageCodec.js'; +import { encodePatchMessage, decodePatchMessage, detectMessageKind } from './WarpMessageCodec.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/PatchBuilderV2.js` around lines 535 - 542, The code unconditionally calls decodePatchMessage on the parent commit in the commit() path (when currentRefSha is set), which will throw if the parent is not a patch; fix it by importing and using detectMessageKind from WarpMessageCodec.js and guard the decode with a check similar to _nextLamport(): call this._persistence.showNode(currentRefSha), run detectMessageKind on the returned commitMessage, and only call decodePatchMessage and update lamport/parentCommit when detectMessageKind indicates a patch; otherwise skip decoding and leave lamport/parentCommit as-is (or handle non-patch parents the same way _nextLamport() does).src/domain/warp/patch.methods.js (1)
99-102: Minor style inconsistency in the non-patch early return.Line 101 computes
Math.max(1, this._maxObservedLamport + 1), which is equivalent tothis._maxObservedLamport + 1(since_maxObservedLamport >= 0). The standard return path on line 119 usesMath.max(ownTick, this._maxObservedLamport) + 1. For the non-patch case,ownTickis effectively 0 (fresh start), soMath.max(0, _maxObservedLamport) + 1would produce the same result and be stylistically consistent.♻️ Optional: unify with the standard return
if (kind !== 'patch') { // Writer ref doesn't point to a patch commit - treat as fresh start - return { lamport: Math.max(1, this._maxObservedLamport + 1), parentSha: currentRefSha }; + return { lamport: Math.max(0, this._maxObservedLamport) + 1, parentSha: currentRefSha }; }Or simply fall through to the standard return at line 118–121 (since
ownTickis already0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/warp/patch.methods.js` around lines 99 - 102, The non-patch early return uses Math.max(1, this._maxObservedLamport + 1) which is equivalent but inconsistent with the standard path; update the early-return to compute lamport the same way as the normal branch by returning { lamport: Math.max(ownTick, this._maxObservedLamport) + 1, parentSha: currentRefSha } (or simply remove the early return so execution falls through and uses the existing Math.max(ownTick, this._maxObservedLamport) + 1 logic), referencing kind, ownTick, this._maxObservedLamport, parentSha and currentRefSha to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/domain/warp/patch.methods.js`:
- Around line 88-122: The createPatch() and patch() JSDoc must explicitly state
the precondition that callers should materialize the graph (or call
_ensureFreshState()) before creating/committing patches so that
_maxObservedLamport is populated and _nextLamport() can incorporate the global
max; update the JSDoc on both createPatch() and patch() to note that in
multi-writer scenarios callers should call materialize() (or rely on query
methods that auto-materialize) prior to committing, and reference that
PatchBuilderV2.commit() already re-reads the writer ref and that _nextLamport()
uses Math.max(ownTick, this._maxObservedLamport) + 1 to compute the next
lamport.
---
Nitpick comments:
In `@src/domain/services/PatchBuilderV2.js`:
- Around line 544-560: The inline comments in PatchBuilderV2.js duplicate the
step number "4" (one at the start of the PatchV2 construction and again before
encoding), so update the comment sequence after the Build step to be
sequential—e.g., keep "4. Build PatchV2 structure..." and change the later "4.
Encode patch as CBOR" to "5. Encode patch as CBOR" (and increment any subsequent
step numbers if present); search around the createPatchV2 call, vvSerialize,
this._ops, this._reads, and this._writes usages to find and renumber the
remaining step comments accordingly.
- Around line 535-542: The code unconditionally calls decodePatchMessage on the
parent commit in the commit() path (when currentRefSha is set), which will throw
if the parent is not a patch; fix it by importing and using detectMessageKind
from WarpMessageCodec.js and guard the decode with a check similar to
_nextLamport(): call this._persistence.showNode(currentRefSha), run
detectMessageKind on the returned commitMessage, and only call
decodePatchMessage and update lamport/parentCommit when detectMessageKind
indicates a patch; otherwise skip decoding and leave lamport/parentCommit as-is
(or handle non-patch parents the same way _nextLamport() does).
In `@src/domain/warp/patch.methods.js`:
- Around line 99-102: The non-patch early return uses Math.max(1,
this._maxObservedLamport + 1) which is equivalent but inconsistent with the
standard path; update the early-return to compute lamport the same way as the
normal branch by returning { lamport: Math.max(ownTick,
this._maxObservedLamport) + 1, parentSha: currentRefSha } (or simply remove the
early return so execution falls through and uses the existing Math.max(ownTick,
this._maxObservedLamport) + 1 logic), referencing kind, ownTick,
this._maxObservedLamport, parentSha and currentRefSha to locate and change the
code.
1 participant
Problem
First-time writers were defaulting to Tick 1 because
_nextLamport()only inspected their own (empty) chain. If an existing writer had already committed at Tick 1, both writers shared the same Lamport timestamp and the LWW tiebreaker resolved to alphabetical WriterID order — silently discarding the later mutation.Fix
Introduce a per-instance
_maxObservedLamportcounter (initialized to0) that is maintained in three places:materialize()— scans checkpoint frontier tip commits and incremental patches to capture the global max before any new commit is issued_nextLamport()— returnsMath.max(ownTick, _maxObservedLamport) + 1, guaranteeing a globally-monotonic tick_onPatchCommitted()— updates the counter after each successful commit so the instance stays currentLint fix
The inline scanning loops pushed
materialize()to complexity 38 (limit 35) and nesting depth 7 (limit 6). Extracted two module-private helpers:scanFrontierForMaxLamport(graph, frontier)— async, best-effort frontier scanscanPatchesForMaxLamport(graph, patches)— sync patch array scanNo behaviour change; same semantics, cleaner structure.
Tests
New regression suite:
test/unit/domain/WarpGraph.noCoordination.test.js_maxObservedLamportincrements after each commit on the same instancematerialize()updates_maxObservedLamportfrom observed patchesAll 2,877 unit tests pass.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests