fix(storage)!: fail on unregistered root entity merge#1988
Conversation
Remove silent LWW fallback that violated I5 (No Silent Data Loss). Now returns error with actionable fix instructions.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 67% | Review time: 208.9s
📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-ffd1bf76
This comment has been minimized.
This comment has been minimized.
Address review feedback: introduce MergeError::NoMergeFunctionRegistered variant for better downstream error handling instead of Box<dyn Error>.
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 86% | Review time: 286.1s
🟡 3 warnings, 💡 4 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-b05a5d2c
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 75% | Review time: 322.0s
💡 3 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-2c6cae13
Review Comments Status✅ Addressed Comments
⏳ Pending Comments (for future consideration)
These pending items are valid suggestions for observability improvements but are out of scope for this I5 enforcement PR. |
This comment has been minimized.
This comment has been minimized.
- Add warn! logging when AllFunctionsFailed falls back to LWW (Delivery Contract Rule: any drop MUST be observable) - Handle lock poisoning with abort instead of silent NoFunctionsRegistered (consistent with write side, prevents undefined behavior) - Add dedicated StorageError::MergeError variant (ActionNotAllowed semantically implies permission error)
All PR Review Feedback Addressed ✅Pushed commit 1. AllFunctionsFailed LWW fallback now observableAdded tracing::warn!(
target: "calimero_storage::merge",
"All registered merge functions failed, falling back to LWW. \
This may indicate type mismatch or corrupt data."
);Per Delivery Contract Rule: any drop MUST be observable. 2. Lock poisoning handled correctlyChanged from returning let registry = MERGE_REGISTRY.read().unwrap_or_else(|poisoned| {
tracing::error!(
target: "calimero_storage::merge",
"MERGE_REGISTRY lock poisoned, aborting. This indicates a panic in merge code."
);
std::process::abort();
});Consistent with write side behavior, prevents undefined behavior from propagating. 3. Dedicated StorageError::MergeError variantAdded semantic error type: /// A merge operation failed.
#[error("Merge failed: {0}")]
MergeError(String),
All tests pass, code is formatted and lints clean. |
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 295.8s
🟡 3 warnings, 💡 1 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-308c3873
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 95% | Review time: 344.3s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-835eb0c5
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 92% | Review time: 334.2s
🟡 1 warnings, 💡 3 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-4677e87e
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Use std::sync::Once to ensure merge functions are only registered once, even when called from multiple tests.
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 313.8s
💡 3 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-597effef
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 331.6s
🟡 1 warnings, 💡 2 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-cf853fd0
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (89c70ea4fd)diff --git a/crates/storage/src/tests/common.rs b/crates/storage/src/tests/common.rs
--- a/crates/storage/src/tests/common.rs
+++ b/crates/storage/src/tests/common.rs
@@ -192,18 +192,14 @@
/// This must be called before any test that triggers root entity merging.
/// Required for I5 enforcement - without registration, merge operations will fail.
///
-/// This function is idempotent - safe to call multiple times.
+/// This function is idempotent - safe to call multiple times, even after
+/// `clear_merge_registry()` has been called.
pub fn register_test_merge_functions() {
- use std::sync::Once;
- static INIT: Once = Once::new();
-
- INIT.call_once(|| {
- use crate::merge::register_crdt_merge;
- register_crdt_merge::<Page>();
- register_crdt_merge::<Paragraph>();
- register_crdt_merge::<Person>();
- register_crdt_merge::<EmptyData>();
- });
+ use crate::merge::register_crdt_merge;
+ register_crdt_merge::<Page>();
+ register_crdt_merge::<Paragraph>();
+ register_crdt_merge::<Person>();
+ register_crdt_merge::<EmptyData>();
}
/// Helper to create a test keypair and public key. |
Once prevents re-registration after clear_merge_registry() in #[serial] tests, causing NoMergeFunctionRegistered errors. The registry's HashMap insert is already idempotent, so repeated calls are safe.
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 83% | Review time: 263.5s
💡 2 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-38bf7a24
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 80% | Review time: 341.1s
🟡 1 warnings, 💡 3 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-eec572e3
1 participant
Summary
merge_root_state()to enforce Invariant I5 (No Silent Data Loss)#[app::state]macro or callregister_crdt_merge::<YourState>()Test plan
merge_root_state())cargo fmtandcargo clippypass on storage crateNote
Medium Risk
Behavior change turns previously “best-effort” root conflict resolution into a hard error for unregistered apps/tests, which can break sync flows until registration is added. Core merge/error plumbing and test setup are touched, but the change is localized and includes explicit error messaging.
Overview
Enforces I5 by removing silent LWW fallback for root merges.
merge_root_state()now requires a registered merge function and returnsMergeError::NoMergeFunctionRegisteredwhen the merge registry is empty; callers (Interface::try_merge_data) propagate this asStorageError::MergeFailureinstead of choosing the newer blob.The merge registry API is adjusted to return a
MergeRegistryResult(success / no functions / all failed), with root merge only allowing an LWW fallback when merge functions exist but none succeed (with a warning). Tests are updated to register merge functions up-front (adding simpleMergeableimpls for test types plus a sharedregister_test_merge_functions()helper), and docs are rewritten to reflect the new “must register” behavior and the expected error message.Written by Cursor Bugbot for commit 7a1b5bc. This will update automatically on new commits. Configure here.