fix(M12): sync safety + cache coherence (T1 + T2)#50
Merged
flyingrobots merged 5 commits intomainfrom Feb 27, 2026
Merged
Conversation
… op allowlist, surface skippedWriters B105: Replace direct _cachedState assignment + _invalidateDerivedCaches() in applySyncResponse with canonical _setMaterializedState() call. Derived caches (adjacency, indexes, view) are now rebuilt rather than nulled, fixing C1 stale-read bug. Surface skippedWriters in syncWith() return. Remove unused _invalidateDerivedCaches() method. B106: Add isKnownOp() validation in SyncProtocol.applySyncResponse before join(). Unknown ops throw SchemaUnsupportedError (fail closed), fixing C2 silent data loss. B107: Add optional isAncestor() pre-check in processSyncRequest to detect diverged writers without expensive chain walk. Falls back to loadPatchRange throw for persistence layers without isAncestor. Updated misleading comment in computeSyncDelta.
…lear stale view hash
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralized canonical materialization was added: sync responses now install state via a new _setMaterializedState path, skippedWriters are surfaced through sync APIs, unknown patch ops are rejected, divergence checks use isAncestor when available, and WarpGraph join/GC/patch paths clear cached view hashes and update version vectors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncController
participant SyncProtocol
participant JoinReducer
participant WarpGraph
Client->>SyncController: applySyncResponse(response)
SyncController->>SyncProtocol: validateOps(patches)
SyncProtocol->>SyncProtocol: isKnownOp() check
alt Unknown op found
SyncProtocol-->>SyncController: SchemaUnsupportedError (throws)
SyncController-->>Client: Error
else All ops known
SyncProtocol-->>SyncController: Valid patches
SyncController->>JoinReducer: applyPatches(currentState, patches)
JoinReducer-->>SyncController: newState
SyncController->>WarpGraph: _setMaterializedState(newState)
WarpGraph->>WarpGraph: rebuild adjacency/index, clear derived caches, set _stateDirty=false
WarpGraph-->>SyncController: installed
SyncController-->>Client: {state, skippedWriters, writersApplied,...}
end
sequenceDiagram
participant Client
participant Process as processSyncRequest
participant Persistence
participant ChainWalker
Client->>Process: processSyncRequest(range)
Process->>Persistence: isAncestor(range.from, range.to)?
alt isAncestor available and false
Persistence-->>Process: false
Process-->>Client: skip writer (divergence), record skippedWriters
else isAncestor true or unavailable
Persistence-->>Process: true/NA
Process->>ChainWalker: loadPatchRange(range) (chain walk fallback)
ChainWalker-->>Process: patch(es)
Process-->>Client: patches
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/domain/services/SyncController.test.js (1)
366-388: Add a rejection-path regression for_setMaterializedState.This suite covers the success path well, but it doesn’t verify behavior when
_setMaterializedStaterejects. Please add a test that ensuresapplySyncResponse()does not leave partially updated bookkeeping (_lastFrontier,_patchesSinceGC) on failure.🧪 Suggested test addition
+ it('does not advance frontier/counters when _setMaterializedState rejects', async () => { + const fakeState = { + observedFrontier: new Map(), + nodeAlive: { dots: new Map() }, + edgeAlive: { dots: new Map() }, + }; + const newState = { observedFrontier: new Map(), nodeAlive: { dots: new Map() }, edgeAlive: { dots: new Map() } }; + const previousFrontier = new Map([['alice', 'sha-1']]); + const nextFrontier = new Map([['alice', 'sha-2']]); + applySyncResponseMock.mockReturnValue({ state: newState, frontier: nextFrontier, applied: 2 }); + + const host = createMockHost({ + _cachedState: fakeState, + _lastFrontier: previousFrontier, + _patchesSinceGC: 5, + _setMaterializedState: vi.fn().mockRejectedValue(new Error('install failed')), + }); + const ctrl = new SyncController(/** `@type` {*} */ (host)); + + await expect(ctrl.applySyncResponse({ type: 'sync-response', frontier: {}, patches: [] })) + .rejects.toThrow('install failed'); + expect(host._lastFrontier).toBe(previousFrontier); + expect(host._patchesSinceGC).toBe(5); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/SyncController.test.js` around lines 366 - 388, Add a new unit test in the SyncController.test.js that simulates _setMaterializedState rejecting and verifies that applySyncResponse leaves bookkeeping unchanged (i.e., _lastFrontier and _patchesSinceGC remain their pre-call values) when SyncController.applySyncResponse throws; mock applySyncResponseMock to return a successful state/frontier, set host._setMaterializedState to a mock that returns a rejected Promise, call await expect(ctrl.applySyncResponse(...)).rejects.toThrow(), and then assert host._lastFrontier and host._patchesSinceGC are equal to their original values and that _materializedGraph was not set/changed; reference _setMaterializedState, SyncController.applySyncResponse, _lastFrontier, and _patchesSinceGC to locate the code under test.src/domain/services/SyncController.js (1)
303-304: AlignskippedWritersJSDoc shape with runtime payload.
applySyncResponse()andsyncWith()now surface detailed skipped-writer entries (localSha,remoteSha), butsyncWith()JSDoc currently types entries as{ writerId, reason }only. Please make these signatures consistent so typed consumers can use the full payload.Also applies to: 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncController.js` around lines 303 - 304, Update the JSDoc return-type for both applySyncResponse() and syncWith() in SyncController.js so the skippedWriters entry shape matches runtime payload; change the typed element from just `{writerId, reason}` to include `localSha: string` and `remoteSha: string|null` (e.g. `{writerId: string, reason: string, localSha: string, remoteSha: string|null}`) in the Promise return annotation and any other JSDoc references (also update the occurrence around the second location noted near line 377) so consumers see the full payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/SyncController.js`:
- Around line 337-347: The host bookkeeping (_lastFrontier and _patchesSinceGC)
is being updated before awaiting _setMaterializedState, which can leave
_cachedState inconsistent if install fails; fix by moving the two mutations
(this._host._lastFrontier = result.frontier and this._host._patchesSinceGC +=
result.applied) to after the await
this._host._setMaterializedState(result.state) so they only apply when
materialized state install succeeds (ensuring hasFrontierChanged/_cachedState
remain consistent); keep the await call and consider preserving current error
propagation behavior (no swallow) so failures still surface.
---
Nitpick comments:
In `@src/domain/services/SyncController.js`:
- Around line 303-304: Update the JSDoc return-type for both applySyncResponse()
and syncWith() in SyncController.js so the skippedWriters entry shape matches
runtime payload; change the typed element from just `{writerId, reason}` to
include `localSha: string` and `remoteSha: string|null` (e.g. `{writerId:
string, reason: string, localSha: string, remoteSha: string|null}`) in the
Promise return annotation and any other JSDoc references (also update the
occurrence around the second location noted near line 377) so consumers see the
full payload.
In `@test/unit/domain/services/SyncController.test.js`:
- Around line 366-388: Add a new unit test in the SyncController.test.js that
simulates _setMaterializedState rejecting and verifies that applySyncResponse
leaves bookkeeping unchanged (i.e., _lastFrontier and _patchesSinceGC remain
their pre-call values) when SyncController.applySyncResponse throws; mock
applySyncResponseMock to return a successful state/frontier, set
host._setMaterializedState to a mock that returns a rejected Promise, call await
expect(ctrl.applySyncResponse(...)).rejects.toThrow(), and then assert
host._lastFrontier and host._patchesSinceGC are equal to their original values
and that _materializedGraph was not set/changed; reference
_setMaterializedState, SyncController.applySyncResponse, _lastFrontier, and
_patchesSinceGC to locate the code under test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdROADMAP.mdsrc/domain/WarpGraph.jssrc/domain/services/SyncController.jssrc/domain/services/SyncProtocol.jssrc/domain/warp/checkpoint.methods.jssrc/domain/warp/materializeAdvanced.methods.jssrc/domain/warp/patch.methods.jstest/unit/domain/WarpGraph.coverageGaps.test.jstest/unit/domain/services/SyncController.test.jstest/unit/domain/services/SyncController.trustGate.test.jstest/unit/domain/services/SyncProtocol.test.js
… assertion H1: add skippedWriters to SyncResponse, ApplySyncResult, syncWith in index.d.ts M1: fix misleading assertOpsCompatible comment in SyncProtocol M2/M3: widen SyncController.syncWith @returns to full skippedWriters shape L1: assert _stateDirty in dirty-path coverage gap test L3: replace bare Function with (...args: unknown[]) => unknown
Release Preflight
If you tag this commit as |
Move _lastFrontier/_patchesSinceGC mutations after _setMaterializedState await so a failed install does not leave bookkeeping advanced while _cachedState remains stale. Add rejection-path regression test. Addresses CodeRabbit review: 1 major (ordering), 1 nitpick (test), 1 nitpick (JSDoc — already fixed in prior commit).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
21-21: Consider moving this bullet to aRemovedsubsection.
Removed _invalidateDerivedCaches()is currently under### Changed; categorizing it under### Removedwould improve changelog taxonomy consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 21, Move the bullet referencing `_invalidateDerivedCaches()` from the "### Changed" section into a new or existing "### Removed" subsection; update the heading grouping so the line "- **Removed `_invalidateDerivedCaches()`** — replaced by canonical `_setMaterializedState()` path; derived caches are now rebuilt rather than nulled. (B105)" appears under "### Removed" and ensure surrounding section ordering and numbering remain consistent with the changelog format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Line 21: Move the bullet referencing `_invalidateDerivedCaches()` from the
"### Changed" section into a new or existing "### Removed" subsection; update
the heading grouping so the line "- **Removed `_invalidateDerivedCaches()`** —
replaced by canonical `_setMaterializedState()` path; derived caches are now
rebuilt rather than nulled. (B105)" appears under "### Removed" and ensure
surrounding section ordering and numbering remain consistent with the changelog
format.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdindex.d.tssrc/domain/services/SyncController.jssrc/domain/services/SyncProtocol.jstest/unit/domain/WarpGraph.coverageGaps.test.js
Member
Author
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
applySyncResponsethrough_setMaterializedState()instead of raw cache assignment. Validate ops againstisKnownOp()allowlist (fail closed). AddisAncestor()pre-check inprocessSyncRequestfor early divergence detection. SurfaceskippedWritersinsyncWithreturn.join()to install merged state as canonical (_stateDirty = false, adjacency built synchronously) instead of marking dirty which caused_ensureFreshState()to discard the merge result. Clear_cachedViewHashin all dirty paths. WidenstateHashtype tostring|nullfor deferred hash computation.Test plan
WarpGraph.noCoordination.test.js— 7/7 pass (non-negotiable gate)WarpGraph.coverageGaps.test.js— 38 tests including 6 new B108 regression testsSyncController.test.js— 39 tests including new sync-apply coherence testsSyncProtocol.test.js— 32 tests including divergence + op validation testsSummary by CodeRabbit
Bug Fixes
Types
Tests
Documentation