Root WASM merge path#2001
Conversation
The WasmMergeCallback trait has both merge_custom and merge_root_state methods, but only merge_custom was being wired through RuntimeEnv. This left root state conflicts unable to dispatch to the WASM __calimero_merge_root_state export. Changes: - Add wasm_root_merge_callback field to RuntimeEnv - Add with_root_merge_callback and wasm_root_merge_callback methods - Add get_wasm_root_merge_callback function to env module - Add root_merge_callback field and methods to VMLogic - Update build_runtime_env_with_merge to accept root_merge_callback - Update merge_root_state to try WASM callback when registry fails - Wire up root merge callback from RuntimeMergeCallback in Module::run This ensures root state conflicts can now dispatch to the WASM merge function when no in-process merge function is registered.
|
Cursor Agent can help with this pull request. Just |
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![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: 100% | Review time: 173.0s
🔴 1 critical, 🟡 1 warnings, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-70a4f550
| match try_merge_registered(existing, incoming, existing_ts, incoming_ts) { | ||
| MergeRegistryResult::Success(merged) => Ok(merged), | ||
| MergeRegistryResult::NoFunctionsRegistered => { | ||
| // No in-process merge function registered. |
There was a problem hiding this comment.
🔴 Duplicate WASM callback check creates dead code and incorrect error handling
The first WASM callback check at line 109 returns immediately on both success AND failure, making the second check at line 136 unreachable dead code; if the callback returns an error, it propagates up instead of falling back to LWW as the second block's comment suggests.
Suggested fix:
Remove the first early-return WASM check and keep only the second one that properly handles errors with LWW fallback, or change the first check to only return on Ok and fall through on Err.
| @@ -122,6 +133,25 @@ pub fn merge_root_state( | |||
| // - The data type doesn't match any registered type (test contamination) | |||
There was a problem hiding this comment.
🟡 Duplicate WASM callback check is dead code
The second get_wasm_root_merge_callback() call (lines 136-156) is unreachable: if the first check (lines 109-119) finds a callback it returns, otherwise there's no callback and this second check will also find None.
Suggested fix:
Remove the second WASM callback block entirely; the first check already handles this case.
|
|
||
| /// Optional WASM merge callback for root state merging during sync. | ||
| /// This is called by the storage layer when merging root state and no | ||
| /// in-process merge function is registered. |
There was a problem hiding this comment.
📝 Nit: Verbose type signature could use a type alias
The Rc<dyn Fn(&[u8], &[u8]) -> Result<Vec<u8>, MergeError>> type is repeated across multiple files; a type alias would improve readability (DRY).
Suggested fix:
Consider adding `pub type RootMergeCallback = Rc<dyn Fn(&[u8], &[u8]) -> Result<Vec<u8>, MergeError>>;` in a shared location.
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
1 participant
Fix: Root WASM merge path remains disconnected
Description
This PR fixes a high-severity logic bug where the
WasmMergeCallback::merge_root_statepath was not correctly wired into the runtime environment, preventing root state conflicts from being resolved via WASM callbacks.The bug (ID: 11afc4cb-e272-4099-84c4-66de8d7637bd) meant that while the
WasmMergeCallbacktrait and its runtime implementation existed for root merges, theRuntimeEnvandVMLogiconly propagated custom entity merge callbacks. Consequently, themerge_root_statefunction in storage could not dispatch to__calimero_merge_root_state, leading to feature failure for WASM-based root merges.This fix introduces a dedicated channel for the root merge callback in
RuntimeEnvandVMLogic, updatesbuild_runtime_env_with_mergeto accept and propagate it, and modifiesmerge_root_stateto utilize this callback when no host-side merge function is registered.Test plan
The following tests were performed to verify the changes:
cargo buildto ensure successful compilation.cargo test -p runtime -p storageto run unit tests for the modified crates.cargo fmt --checkandcargo clippy --workspace -- -D warningsto ensure code style and linting compliance.All tests passed successfully. It is possible to add an end-to-end test case by simulating a root state conflict and verifying that the
__calimero_merge_root_stateWASM export is invoked.Documentation update
No public or internal documentation updates are required as this fixes an internal plumbing issue for a feature that was not fully functional.