WASM callback merge failure#1996
WASM callback merge failure#1996cursor[bot] wants to merge 1 commit intofeat/sync-wasm-merge-callbackfrom
Conversation
…hods The WasmMergeCallback trait requires &self methods, but the runtime implementation needed &mut self to access the WASM store. This caused both trait methods (merge_custom and merge_root_state) to always return MergeError::WasmCallbackFailed, making custom CRDT merges via the trait dispatch path fail and fall back to LWW behavior. Fix by wrapping the Store in a Mutex<Store> to enable interior mutability. The trait methods now properly acquire the lock and invoke WASM merge functions instead of returning immediate errors. The _mut variants are kept for backwards compatibility but now delegate to the same internal implementation.
|
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: 94% | Review time: 157.3s
🟡 2 warnings, 💡 3 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-52ec967b
| @@ -420,6 +357,64 @@ impl RuntimeMergeCallback { | |||
| Err(MergeError::WasmCallbackFailed { message: error_str }) | |||
There was a problem hiding this comment.
🟡 Bug fix lacks regression test
This PR fixes a bug where callbacks always returned failure, but no test demonstrates the fix works; a regression could silently reintroduce the bug.
Suggested fix:
Add a unit test that creates a RuntimeMergeCallback with a mock WASM module and verifies merge_custom/merge_root_state return Ok() on valid input.
| @@ -201,9 +205,9 @@ impl RuntimeMergeCallback { | |||
| } | |||
There was a problem hiding this comment.
🟡 Unbounded memory allocation from WASM-controlled length
The read_from_wasm method allocates vec![0u8; len as usize] where len comes from WASM memory (via WasmMergeResult); a malicious WASM module could return a huge length to cause memory exhaustion (DoS).
Suggested fix:
Add a maximum length check (e.g., `const MAX_MERGE_DATA_LEN: u64 = 16 * 1024 * 1024;`) and return an error if exceeded.
| local: &[u8], | ||
| remote: &[u8], | ||
| ) -> Result<Vec<u8>, MergeError> { | ||
| /// |
There was a problem hiding this comment.
💡 Mutex held during WASM execution without timeout
The store mutex is held while calling into WASM; without the timeout (noted as TODO for Issue #1780), a malicious or buggy WASM module could block indefinitely, preventing other merge operations.
Suggested fix:
Prioritize implementing the timeout handling mentioned in the TODO to prevent deadlock/DoS scenarios.
| remote: &[u8], | ||
| ) -> Result<Vec<u8>, MergeError> { | ||
| /// | ||
| /// Acquires the store mutex internally. |
There was a problem hiding this comment.
💡 Poisoned lock error message lacks diagnostic detail
When a Mutex is poisoned (due to a prior panic while holding the lock), the error message says 'Failed to acquire store lock' but doesn't indicate the Store may be in an inconsistent state, making debugging harder.
Suggested fix:
Include 'lock poisoned - prior panic occurred' in the message to help diagnose state corruption issues.
| local: &[u8], | ||
| remote: &[u8], | ||
| ) -> Result<Vec<u8>, MergeError> { | ||
| /// |
There was a problem hiding this comment.
💡 Consider extracting lock acquisition into a helper
The same lock-acquisition-with-error-mapping pattern is duplicated in call_merge_root_state and call_merge_custom; a small helper would reduce repetition.
Suggested fix:
Extract a method like `fn acquire_store(&self) -> Result<MutexGuard<Store>, MergeError>` to centralize the lock acquisition and error mapping.
| remote: &[u8], | ||
| type_name: &str, | ||
| ) -> Result<Vec<u8>, MergeError> { | ||
| self.call_merge_custom(local, remote, type_name) |
There was a problem hiding this comment.
📝 Nit: _mut methods no longer require mutable self
The merge_root_state_mut and merge_custom_mut methods retain &mut self for API stability but internally use interior mutability; the doc comment could clarify that mutable self is no longer semantically required.
Suggested fix:
Update doc comment to: 'This method is provided for backwards compatibility. The `&mut self` signature is preserved but interior mutability is used internally.'
|
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: WASM callback trait always returns failure
Description
This PR fixes a bug where the
WasmMergeCallbacktrait implementation forRuntimeMergeCallbackalways returnedMergeError::WasmCallbackFailed, preventing custom CRDT merges from being correctly dispatched and executed via WASM.The
RuntimeMergeCallback's trait methods (merge_customandmerge_root_state) now correctly acquire mutable access to the WASMStore(by wrapping it in aMutex) and invoke the underlying WASM merge functions, ensuring custom CRDT types are merged as intended instead of falling back to LWW behavior.Test plan
The following commands were run to verify the changes:
cargo buildcargo clippycargo fmtcargo testAll tests passed, and the code compiles without warnings. No new end-to-end test cases were added, as the fix addresses an internal logic error in the callback mechanism.
Documentation update
None