fix: mark an op published at its record ack and mirror the gate stage 2 on the produce side - #1058
Conversation
…ge 2 on the produce side Two gaps in the write plane's fail-closed publish path. A publish whose record PUT acked and whose self-adopt then failed left the op queued with the published-op mark unraised, so a restart replayed it, re-uploaded the identical leaves, and a cancel landing in that transfer unpinned content the live record already names. The mark now rises inside the confirmed-publish arm of publish_head, on the last record of an op's plan: the ack is the moment the version stops being replayable, not the adopt. An unconfirmed or race-losing publish still raises nothing, and neither does a record the plan will follow with another - a create marks on its parent, so an op is never dropped with a child no parent names. The scope-root produce mirror verified the commitment's ipnsName binding and every structure signature but never the owner's signature over the commitment itself. authenticate_section_structures authenticates structures against the pseudonyms the section's own commitment names, so a wholly self-consistent attacker-authored section passed. The drain reads its scope root from the snapshot cache, which is floor-checked but not re-verified, so a tampered entry was caught for internal consistency and not for owner authority. check_scope_root now runs verify_grant_set at the gate's own stage-2 position, release-active, on both scope-root authoring paths (AGENTS.md rule 8). Produce-side refusals also carry stable check names, and the drain charges the trust class against the attempt budget instead of retrying it free forever. They are charged rather than dead-lettered on sight: on the drain path these inputs are the cached scope root, which a later resolve replaces, so an immediate permanent verdict would abandon a user's ops over a cache another tick repairs. Closes #1045 Closes #1048 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rror fix /simplify, /security-review and /crypto-privacy-review on the branch diff. - Raise the mark in publish_node rather than threading an op id into publish_head, which returns Ok only on a confirmed publish anyway. Same instant, one fewer parameter and no clippy suppression. - Pin the other half of the mark's rule: a create whose child published and whose parent never did keeps the mark down, so it stays replayable rather than stranding a live child no parent names. Nothing asserted that before, and it is the direction that loses data. - Refuse a reload that lands on a rotated root instead of authoring at the epoch the pass opened on. That skew is a self-healing race, and the new classification would otherwise spend the op's budget on it. - Correct two invariants the trust class made stale: an authoring refusal is raised before its own record reaches the transport, not before every PUT of the op, and the abandonment retires on the target still being unreachable rather than on no PUT having acked. - Route AuthorError's Display through check(), so the refusal name is the one surface rather than a second table to keep in sync, and state where the produce mirror stops - stage 3's ascent-link cross-check takes a reader secret the authoring inputs do not carry. - Generalize the floor fake's fault to every raise, keep it to one lock, and share the drop-without-replay assertions between the two mark tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughScope-root authoring now verifies owner-authorized commitments and reports stable refusal types. Drain publishing marks operations after final record confirmation, tracks confirmation state, and classifies trust refusals. Test fakes and write-plane tests cover failure injection, compensation, replay suppression, and retries. ChangesScope-root validation and publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/net/author.rs`:
- Around line 223-226: Update the grant-section decoding path around
decode_grant_section to map malformed data to a distinct stable AuthorError
variant instead of AuthorError::Seal, mark that variant as a trust refusal in
AuthorError::is_trust_refusal, and add a regression test covering malformed
grantSection classification and attempt-budget charging.
In `@crates/engine/src/sync/drain.rs`:
- Line 1070: Update the publish_folder calls in
crates/engine/src/sync/drain.rs:1070-1070, :1157-1159, and :1172-1175 to mark
final records with Some(applied.op_id): always for the delete record, only when
source == dest for the destination record, and for the source-removal record
when source != dest. Add restart tests covering delete and both reference-move
shapes when final self-adoption fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ab961de-054c-4403-8e62-a866a0f18729
📒 Files selected for processing (5)
crates/engine/src/net/author.rscrates/engine/src/net/rotation.rscrates/engine/src/sync/drain.rscrates/engine/src/testkit/fakes/floor_store.rscrates/engine/tests/write_plane.rs
…very plan CodeRabbit's two Major findings on the PR, plus a data-loss window the review gates found in the second one's fix. A carried grantSection that fails to decode was an AuthorError::Seal, which is_trust_refusal reports false for, so the drain retried it free forever. The same bytes are a whole-record reject on arrival, so they are a trust refusal on the produce side too: a distinct InvalidGrantSection with its own stable check name. The narrowing is exactly the carried-bytes call site. The other three Seal sites describe the body this pass built, which a rebase onto other state may not build again, so they stay availability - reclassifying those would be the mirror-image bug. Delete and reference-move plans passed None throughout, leaving the window #1045 closes still open for those two shapes. Each now marks its plan's last record: the delete's only record, the reference move's dest-add when source and destination are one folder, and the source-remove when they are not. Verified against the code rather than the review's word, and the dest-add's non-marking half now has its own test - the existing move test passes either way, so nothing was pinning the split. That fix opened a window of its own. The compensation fires on any error from the source-remove, including one raised after the record confirmed, and it decides whether to roll back by re-reading the source - a read the failed publish just left stale in the cache, where the dest arm of the same function deliberately reads the network. With the mark up, a wrong rollback would drop the op on restart with the move undone and never retried. A publish failure now carries whether its record confirmed, and a confirmed source-remove is never compensated: the fact the compensation infers is one the publish already has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/testkit/fakes/record_store.rs (1)
108-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
heal_put_forcounterpart.
fail_endpointandfail_put_endpointeach pair with a heal method.fail_put_forhas none, so a test cannot inject a source-remove failure and then let a retry complete the plan in the sameFakeWorld. The current tests do not need it. Add it when a retry-completion test arrives.♻️ Proposed heal counterpart
pub fn fail_put_for(&self, routing_key: &str) { self.put_failing_keys .lock() .expect("lock") .insert(routing_key.to_owned()); } + + /// Restore `routing_key`'s PUT path. + pub fn heal_put_for(&self, routing_key: &str) { + self.put_failing_keys.lock().expect("lock").remove(routing_key); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/testkit/fakes/record_store.rs` around lines 108 - 116, Add a heal_put_for counterpart next to fail_put_for that removes the routing key from put_failing_keys, matching the existing fail/heal pairing and allowing retries to complete within the same FakeWorld.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/testkit/fakes/record_store.rs`:
- Around line 108-116: Add a heal_put_for counterpart next to fail_put_for that
removes the routing key from put_failing_keys, matching the existing fail/heal
pairing and allowing retries to complete within the same FakeWorld.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 64959b6b-a17c-4b8d-907f-ccb74a19030a
📒 Files selected for processing (4)
crates/engine/src/net/author.rscrates/engine/src/sync/drain.rscrates/engine/src/testkit/fakes/record_store.rscrates/engine/tests/write_plane.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/net/author.rs
…lands The dest-add test asserted the move stays queued for the retry that completes it without ever running that retry. Heal the source-remove's PUT and drive the next drain pass, so the claim is proven rather than stated, and pair fail_put_for with the heal that path needs.
Addressed in 91cdac1 — but not as a bare helper, because an unused The asymmetry was real, though, and it was pointing at a genuine coverage gap rather than a missing method. "the move stays queued for the retry that completes it"…and then never runs that retry. The claim was asserted, not proven. So the retry-completion test you said to wait for is in this commit, and it is what justifies the helper:
It passed first run, so the engine already behaved as the original test claimed — the gap was in the proof, not the code.
|
1 participant
Two gaps in the write plane's fail-closed publish path, both extending what landed tonight in #1046 and #1049.
The mark's line is the ack, not the self-adopt
publish_nodereturnsOkonly once the self-adopt has also succeeded, so a record whose PUT confirmed and whose adopt then failed left the op queued with the published-op mark unraised. On the next boot that op replayed, re-uploaded the identical content-addressed leaves, and a cancel landing in that transfer reachedretire_cancelledand unpinned content the live record already names. Within one session the publish-entry interlock covers it; across a rebootUploadCancelsis empty, so it does not.The mark now rises inside
publish_nodethe instantpublish_headreturns, which it does only onPublishOutcome::Published. An unconfirmed or race-losing publish raises nothing: those bytes may never have landed, and dropping the op on the strength of that would be loss.The decision the issue flagged as open is which record of a plan marks. It is the plan's last:
updateContentmarks on the target's own publish,createon the parent-folder publish, and every other publish passesNone. Marking the create's child publish would drop an op on restart whose child no parent names, so the negative direction is pinned by its own test.The produce mirror covered stage 3 but not stage 2
check_scope_rootverified the commitment'sipnsNamebinding and every structure signature, but nevercommitment_sig.authenticate_section_structuresauthenticates structures against the pseudonyms the section's own commitment names, so a wholly self-consistent attacker-authored section — own commitment, own pseudonym, own signatures, correctipnsName— passed it completely. The drain reads its scope root from the snapshot cache, which is floor-checked but never re-verifies the section, so a tampered entry was caught for internal consistency and not for owner authority.check_scope_rootnow runsverify_grant_setat the gate's own stage-2 position, release-active, on both scope-root authoring paths. Reject rows for a foreign-signed commitment, an unusable signature, and the re-seal path's own equivalent, all returningErrso they fire in a release build.Produce-side refusals also carry stable check names —
AuthorError::check, rendered byDisplay— and the drain charges the trust class against the attempt budget instead of retrying it free forever.What differs from the issue bodies
#1048asks for a permanent verdict. These refusals are charged instead. On the drain path they can only come from the cached scope root, which a later resolve replaces, so an immediate dead-letter would abandon a user's ops over a cache another tick repairs — the mirror-image of the bug being fixed. The budget bounds the spin either way, andSeal/HeadTooLargestay availability because a rebase may not build that body again.#1048's second half also namesnet/rotation.rs. That file is a sibling PR's, so this branch makes only the one-argument call-site change the newcheck_scope_rootsignature forces; the classification there is engine: classify rotation-path authoring refusals instead of flattening them to not-published #1055.#1045'sMissingGrantSection/GrantSectionOnChildare unreachable from the drain today — a gated child never carries a section and a gated root always does — but they are classified with the rest rather than left as the one uncharged arm.Also folded in from the review gates
A reload that lands on a rotated root is now refused rather than authored at the epoch the pass opened on: that skew is a self-healing race, and the new classification would otherwise spend the op's budget on it. Two invariants the trust class made stale are corrected — an authoring refusal is raised before its own record reaches the transport, not before every PUT of the op, and the abandonment retires on the target still being unreachable rather than on no PUT having acked.
Deferred
#1055rotation-path classification,#1056surfacing the refusal name pastAttemptsExhausted,#1057the ascent-link half of stage 3 on the re-seal's produce side. Each carries a native dependency edge on#1048.Closes #1045
Closes #1048
Summary by CodeRabbit
Bug Fixes
Tests
Note
Mark ops published at record PUT ack and verify commitment signatures on scope-root authoring
publish_node), rather than after self-adopt; this improves crash safety and prevents redundant re-uploads on restart.check_scope_rootnow performs stage-2/3 validation: decodes the grant section, verifies the commitment signature against the owner identity, checksipns_namematch, and authenticates structure signatures. Two newAuthorErrorvariants (InvalidGrantSection,CommitmentSignatureInvalid) surface these failures.publish_ref_moveno longer rolls back a dest-add if the source-remove already confirmed; compensation is gated on thePublishHalt.confirmedflag introduced by the newPublishHaltstruct.reload_folder: if the loaded epoch differs from the pass epoch, the pass halts and retries rather than authoring with a stale epoch.Macroscope summarized 91cdac1.