Runtime merge callback issues#1942
Runtime merge callback issues#1942cursor[bot] wants to merge 1 commit intofeat/sync-013-wasm-merge-callbackfrom
Conversation
- Fix empty WASM imports by creating stub imports for all host functions
declared by the module (bug 1). The WASM spec requires all declared
imports to be provided at instantiation time.
- Fix hardcoded memory offset by using 65536 (start of second page)
instead of 1024 to avoid corrupting static data, stack, and heap
metadata in the first page (bug 2).
- Refactor duplicated merge function logic into shared execute_merge
helper to reduce code duplication and ensure consistent behavior
(bug 3).
- Remove unused timeout field and related DEFAULT_MERGE_TIMEOUT constant
and with_timeout method per no-dead-code guidelines (bug 4).
- Add type name normalization in lookup functions to match the
normalization applied during registration. Both MergeRegistry and
global try_merge_by_type_name now apply rsplit('::') normalization
so 'my_app::MyState' will match registered 'MyState' (bug 5).
|
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 1 agents | Quality score: 33% | Review time: 89.6s
🟡 1 warnings, 💡 2 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-7bcdd3f7
| // Merge functions shouldn't call host functions, so this is safe | ||
| let results_arity = func_type.results().len(); | ||
| let stub = Function::new(store, func_type, move |_args| { | ||
| // Return default values (zeros) for all result types |
There was a problem hiding this comment.
🟡 Stub functions return I64 regardless of declared type
The stub always returns Value::I64(0) but the actual function signature may expect different types (i32, f32, f64), which could cause a type mismatch error at runtime if a merge function accidentally calls a host function.
Suggested fix:
Match on `func_type.results()` to return the correct `Value` variant for each expected return type (e.g., `Value::I32(0)`, `Value::F64(0.0)`).
| for import in self.module.imports() { | ||
| if import.module() == "env" { | ||
| if let wasmer::ExternType::Function(func_type) = import.ty() { | ||
| // Create a stub function that returns default values |
There was a problem hiding this comment.
💡 Stub invocation silently returns zeros instead of signaling a bug
If a merge function erroneously calls a host function, the stub silently returns zeros which could cause subtle data corruption; adding a tracing::warn! or panic would make such bugs immediately visible during development.
Suggested fix:
Add `tracing::warn!("Merge function called host import '{}' - this indicates a bug", import.name());` inside the stub closure before returning.
| let (type_name_ptr, type_name_len) = | ||
| type_name_info.expect("type_name_info must be Some for custom merge"); | ||
|
|
||
| let merge_fn = instance |
There was a problem hiding this comment.
💡 expect() in closure could propagate cleaner error
Using .expect() inside the closure could panic; while logically safe given the calling code, returning a WasmMergeError would be more consistent with the rest of the error handling.
Suggested fix:
Replace `.expect(...)` with `.ok_or_else(|| WasmMergeError::MergeFailed("internal: missing type_name_info".into()))?`
| remote_data, | ||
| None, | ||
| |instance, store, local_ptr, local_len, remote_ptr, remote_len, _type_name_info| { | ||
| let merge_fn = instance |
There was a problem hiding this comment.
📝 Nit: Unnecessary clone of already-owned String
export_name is already String from to_owned() at line 654; using clone() in the error path allocates again when the string could be moved.
Suggested fix:
Consider restructuring to avoid the clone, or acknowledge this is acceptable for the error path.
|
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
Fixes for WASM Merge Callback and Type Registry
Description
This PR addresses several critical and quality-of-life issues related to WASM merge callbacks and the merge type registry.
call_merge_functionandcall_custom_merge_functioninto a sharedexecute_mergehelper to eliminate code duplication and improve maintainability.timeoutfield and related API fromRuntimeMergeCallbackto prevent misleading users about timeout functionality, as it's a future async enhancement.MergeRegistryand the globaltry_merge_by_type_namefunction to normalize input type names, matching the normalization applied during registration and ensuring successful lookups for fully-qualified names.Test plan
All existing unit and integration tests were run and passed successfully after these changes. The fixes primarily address correctness issues in the WASM runtime and registry logic, which are covered by existing test infrastructure. No new end-to-end tests were added as part of this fix, but the existing ones should now pass reliably.
Documentation update
The removal of the
with_timeoutmethod fromRuntimeMergeCallbackand the clarified behavior of type name normalization inMergeRegistry(and its global functions) should be reflected in any relevant public or internal documentation.