fix(T7): add _committed guard to PatchBuilderV2 — prevent use-after-commit#51
fix(T7): add _committed guard to PatchBuilderV2 — prevent use-after-commit#51flyingrobots merged 9 commits intomainfrom
Conversation
|
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 (4)
📝 WalkthroughWalkthroughThe PR introduces error handling and runtime validation for patch operations and garbage collection processes. New Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant JoinReducer
participant validateOp
participant applyOp as Operation<br/>Handler
participant PatchError
Caller->>JoinReducer: applyOpV2(operation)
JoinReducer->>validateOp: validate(operation)
alt Valid Operation
validateOp-->>JoinReducer: validation passes
JoinReducer->>applyOp: process operation
applyOp-->>JoinReducer: returns result
JoinReducer-->>Caller: result
else Invalid Operation
validateOp->>PatchError: throw(message, {code: E_PATCH_MALFORMED})
PatchError-->>Caller: PatchError thrown
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/domain/services/GCPolicy.js (1)
124-129: Preserve the original error details when wrapping compaction failures.Current
catch {}loses the root cause, makingE_GC_COMPACT_FAILEDharder to debug in production incidents.🛠️ Proposed improvement
- } catch { + } catch (cause) { throw new WarpError( `GC compaction failed during ${nodesDone ? 'edgeAlive' : 'nodeAlive'} phase`, 'E_GC_COMPACT_FAILED', - { context: { phase: nodesDone ? 'edgeAlive' : 'nodeAlive', partialCompaction: nodesDone } }, + { + context: { + phase: nodesDone ? 'edgeAlive' : 'nodeAlive', + partialCompaction: nodesDone, + cause: cause instanceof Error ? cause.message : String(cause), + }, + }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/GCPolicy.js` around lines 124 - 129, The catch block in GCPolicy.js currently swallows the original exception; change the bare catch to catch (err) and include the original error when throwing the WarpError (e.g., pass err as a cause or attach it to the context) so the thrown 'E_GC_COMPACT_FAILED' retains the root-cause details; update the throw site that constructs WarpError (the same place using nodesDone and 'edgeAlive'/'nodeAlive' phase) to include the captured error under a consistent property like cause or context.originalError.src/domain/services/JoinReducer.js (1)
707-712: Avoid double-validating every op in diff/receipt paths.
applyWithDiffandapplyWithReceiptvalidate ops before callingapplyOpV2, butapplyOpV2already validates at Line 213. Keeping one validation path avoids unnecessary overhead and drift.♻️ Proposed simplification
- validateOp(/** `@type` {Record<string, unknown>} */ (op)); const eventId = createEventId(patch.lamport, patch.writer, patchSha, i); @@ - validateOp(/** `@type` {Record<string, unknown>} */ (op)); const eventId = createEventId(patch.lamport, patch.writer, patchSha, i);Also applies to: 736-768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/JoinReducer.js` around lines 707 - 712, The code is double-validating ops: callers applyWithDiff and applyWithReceipt already call validateOp before invoking applyOpV2, but applyOpV2 also validates (around the current Line 213); remove the redundant validateOp call from applyOpV2 so validation happens only in the caller paths (applyWithDiff/applyWithReceipt), leaving applyOpV2 to assume its input is already validated; ensure tests/callers still pass valid ops (references: validateOp, applyWithDiff, applyWithReceipt, applyOpV2, snapshotBeforeOp, accumulateOpDiff, createEventId).test/unit/domain/services/JoinReducer.validation.test.js (1)
79-231: Add compatibility tests for real historical op shapes.Given current CI failures, include explicit non-throw cases for payloads like
observedDots: Set<string>and accepted legacy/encodeddotforms. That would prevent reintroducing this regression.🧪 Suggested test additions
+ it('accepts NodeRemove with observedDots as Set (backward compatibility)', () => { + expect(() => applyOpV2(state(), /** `@type` {any} */ ({ + type: 'NodeRemove', + observedDots: new Set(), + }), eid)).not.toThrow(); + }); + + it('accepts NodeAdd with encoded/legacy dot shape used by existing patches', () => { + expect(() => applyOpV2(state(), /** `@type` {any} */ ({ + type: 'NodeAdd', + node: 'n', + dot: 'w1:1', + }), eid)).not.toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/JoinReducer.validation.test.js` around lines 79 - 231, Add compatibility tests that assert applyOpV2/reduceV5 do NOT throw for historical op shapes: add cases where observedDots is a Set<string> (for NodeRemove and EdgeRemove) and where dot uses legacy/encoded forms (e.g., string/legacy-object representations) for NodeAdd/EdgeAdd; place them alongside the existing C3 per-op-type tests and the fast-path tests so they call applyOpV2(state(), /* historical payload */ , eid) and reduceV5([...entry]) and assert not.toThrow(), referencing the existing test helpers createEmptyStateV5, createEventId, applyOpV2 and reduceV5.test/unit/domain/services/PatchBuilderV2.test.js (1)
717-810: Extend guard tests to cover async mutators and concurrent commit attempts.Current coverage is good for post-commit sync calls, but it misses
attachContent/attachEdgeContentandPromise.all([builder.commit(), builder.commit()])re-entrancy behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/PatchBuilderV2.test.js` around lines 717 - 810, Add tests in the "use-after-commit guard" suite to (1) assert async mutators throw after commit by calling attachContent(...) and attachEdgeContent(...) on a committed PatchBuilderV2 and expecting the same "PatchBuilder already committed — create a new builder" error, and (2) assert commit re-entrancy behavior by invoking Promise.all([builder.commit(), builder.commit()]) on a fresh builder and asserting that only one commit proceeds (builder._committed becomes true) and the concurrent/second promise rejects (expect the rejected promise to throw or resolve to an error indicating the builder was already committed); reference PatchBuilderV2, attachContent, attachEdgeContent, and commit when adding these tests.
🤖 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/JoinReducer.js`:
- Around line 125-131: The requireArray guard in requireArray currently rejects
non-Array values (throwing PatchError) but should accept iterable collections
used in valid payloads (e.g., Set) so NodeRemove/EdgeRemove don't break; update
requireArray to allow Array or iterable objects (or specifically Set) and, if
given a Set/iterable, coerce op[field] to an Array before returning/using it
(preserve existing PatchError for truly non-iterable values) so downstream code
that expects an array receives one; reference requireArray and PatchError to
locate and modify the check and conversion logic.
- Around line 138-157: The requireDot validator is too strict: instead of always
throwing when op.dot isn't an object with string writerId and numeric counter,
relax it to accept legacy shapes by (1) if dot is a string, attempt to parse
JSON and fall back to treating the string as a writerId with counter 0; (2) if
dot is an object but lacks writerId, accept alternative identifier fields (e.g.,
id, writer) or coerce a missing writerId to a string representation and default
counter to 0; (3) only throw a PatchError from requireDot when after coercion
writerId is not a string or counter is not a number. Update the PatchError
context in requireDot to include the original op.dot value and the coerced
result for debugging while keeping existing op.type context.
In `@src/domain/services/PatchBuilderV2.js`:
- Around line 173-174: The current commit() and mutator guards rely solely on
this._committed which is flipped only after async awaits, leaving a re-entrancy
window; add an in-flight lock (e.g., this._committing boolean) and update the
guard checks (places referencing this._committed inside commit() and relevant
mutator methods) to reject if either this._committed or this._committing is
true, set this._committing = true at the start of the async commit flow before
any awaits, and ensure you clear it in a finally block (and flip this._committed
on success) so concurrent calls cannot pass the guard during the async window.
---
Nitpick comments:
In `@src/domain/services/GCPolicy.js`:
- Around line 124-129: The catch block in GCPolicy.js currently swallows the
original exception; change the bare catch to catch (err) and include the
original error when throwing the WarpError (e.g., pass err as a cause or attach
it to the context) so the thrown 'E_GC_COMPACT_FAILED' retains the root-cause
details; update the throw site that constructs WarpError (the same place using
nodesDone and 'edgeAlive'/'nodeAlive' phase) to include the captured error under
a consistent property like cause or context.originalError.
In `@src/domain/services/JoinReducer.js`:
- Around line 707-712: The code is double-validating ops: callers applyWithDiff
and applyWithReceipt already call validateOp before invoking applyOpV2, but
applyOpV2 also validates (around the current Line 213); remove the redundant
validateOp call from applyOpV2 so validation happens only in the caller paths
(applyWithDiff/applyWithReceipt), leaving applyOpV2 to assume its input is
already validated; ensure tests/callers still pass valid ops (references:
validateOp, applyWithDiff, applyWithReceipt, applyOpV2, snapshotBeforeOp,
accumulateOpDiff, createEventId).
In `@test/unit/domain/services/JoinReducer.validation.test.js`:
- Around line 79-231: Add compatibility tests that assert applyOpV2/reduceV5 do
NOT throw for historical op shapes: add cases where observedDots is a
Set<string> (for NodeRemove and EdgeRemove) and where dot uses legacy/encoded
forms (e.g., string/legacy-object representations) for NodeAdd/EdgeAdd; place
them alongside the existing C3 per-op-type tests and the fast-path tests so they
call applyOpV2(state(), /* historical payload */ , eid) and reduceV5([...entry])
and assert not.toThrow(), referencing the existing test helpers
createEmptyStateV5, createEventId, applyOpV2 and reduceV5.
In `@test/unit/domain/services/PatchBuilderV2.test.js`:
- Around line 717-810: Add tests in the "use-after-commit guard" suite to (1)
assert async mutators throw after commit by calling attachContent(...) and
attachEdgeContent(...) on a committed PatchBuilderV2 and expecting the same
"PatchBuilder already committed — create a new builder" error, and (2) assert
commit re-entrancy behavior by invoking Promise.all([builder.commit(),
builder.commit()]) on a fresh builder and asserting that only one commit
proceeds (builder._committed becomes true) and the concurrent/second promise
rejects (expect the rejected promise to throw or resolve to an error indicating
the builder was already committed); reference PatchBuilderV2, attachContent,
attachEdgeContent, and commit when adding these tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bin/warp-graph.jsindex.d.tssrc/domain/errors/PatchError.jssrc/domain/errors/index.jssrc/domain/services/GCPolicy.jssrc/domain/services/JoinReducer.jssrc/domain/services/PatchBuilderV2.jstest/unit/domain/services/GCPolicy.test.jstest/unit/domain/services/JoinReducer.validation.test.jstest/unit/domain/services/PatchBuilderV2.test.js
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
|
@coderabbitai Thanks for the detailed review. Clarifying the two JoinReducer threads:
Both are outdated against the current PR head. Evidence:
The PatchBuilder re-entrancy guard thread is already resolved and implemented ( If JoinReducer compatibility regressions reappear, I’ll handle them in a dedicated PR scoped to reducer behavior. |
|
@coderabbitai review please 🥕 |
|
Let me take a fresh look at the changes now. 🥕 ✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 106 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 152
No JoinReducer changes are present in this PR. Those review threads are indeed outdated and no longer applicable to the current scope. The
Your approach to keep this PR tightly scoped to the use-after-commit guard and handle any future reducer concerns separately is the right call. Would you like me to resolve those outdated JoinReducer threads for you? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 86 🏁 Script executed: Length of output: 351 🏁 Script executed: Length of output: 220
You're absolutely right—the JoinReducer threads are outdated and not relevant to the current PR state. I can also see that the implementation includes both lifecycle guards:
Your approach to keep this PR tightly scoped to PatchBuilderV2 and handle any JoinReducer compatibility concerns in a separate PR is sound. Apologies for any confusion caused by the outdated summary context. The current implementation looks solid! 🎯 |
|
@coderabbitai review please 🥕\n\nRequesting an additional maintainer review for merge quorum. |
|
I can see the PR is tightly focused on the PatchBuilderV2 lifecycle guards with both Regarding the maintainer review for merge quorum—I can provide the automated review, but you're right that you'll need human maintainer approval as well for your merge requirements. ✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
|
Maintainer review requested for merge quorum (target: >=2 non-bot reviews). Current branch tip is c1140e6 with backlog follow-up automation tasks only. |
Release Preflight
If you tag this commit as |
# Conflicts: # BACKLOG.md # ROADMAP.md
Release Preflight
If you tag this commit as |
1 participant
Summary
_committedlifecycle flag toPatchBuilderV2_assertNotCommitted()at the start of all mutating APIs, includingcommit()_committed = trueonly after a successful commituse-after-commit guardtest block covering all requested post-commit and failed-commit behaviorsVerification
Summary by CodeRabbit
New Features
Bug Fixes
Tests