fix: free Rust-owned FFI strings to stop native memory leak - #1141
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe JavaScript native resolver now declares five FFI results as ChangesNative resolver FFI updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized binding change preserves the JavaScript client’s returned values while freeing Rust-owned native strings to prevent memory growth. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
@gyash1512 tests are failing with segmentation fault |
700e6c9 to
975e949
Compare
|
can you please update the description with the latest changes |
|
Possible to resolve the conflicts ? |
975e949 to
405d78f
Compare
Resolved @sauraww |
Problem
The JavaScript client's native resolver leaks native (Rust-owned) memory on every FFI call that returns a string. The five string-returning functions were declared with a
char*return type, so koffi auto-decodes the return into a JS string (a copy) and discards the original pointer. That makestypeof result === "string"always true, which means thecore_free_string(result)calls are dead code and the RustCString(CString::into_raw) is never freed.Every evaluation leaks the full returned string (for
core_get_resolved_config, that's the entire resolved config, ~KBs). In a long-running, high-throughput consumer this grows the process's native heap linearly until it OOMs. Confirmed with jemalloc heap profiling (jeprofdiff):alloc::ffi::c_str::CString::_from_vec_unchecked, reached viakoffi → libsuperposition_core, accounted for 90%+ of native heap growth. Present in every published version through the latest.Solution
Use koffi's disposable type — the documented idiom for freeing heap-allocated strings returned from C. Register a named type
OwnedStrderived fromstrwithcore_free_stringas its free callback, then declare the five owned-string functions to returnOwnedStr:koffi converts the returned
char*to a JS string and then automatically callscore_free_stringon the original Rust pointer, so the native allocation is freed instead of leaked.Changes in
clients/javascript/bindings/native-resolver.ts:core_get_resolved_config,core_get_applicable_variants,core_parse_toml_config,core_parse_json_config,core_provider_cache_eval_configto returnOwnedStr(waschar*).OwnedStrdisposable type once via a module-levelownedStrTypeRegisteredguard (koffi type names are process-global, so registering per-instance throwsDuplicate type name).core_free_string's ownchar*argument type is unchanged (it's an input pointer, correct as-is).No behavior change to return values — callers still receive the same string/parsed object; the pointer is now freed instead of leaked.
Environment variable changes
None.
Pre-deployment activity
dist/reflects the source change:npm run build -w bindings -w sdk -w open-feature-provider.superposition-providerversion (this ships indist/index.esm.js/dist/index.js).pnpm patchuntil they can upgrade).Post-deployment activity
jeprofdiff thatCString::_from_vec_uncheckedno longer dominates growth.API changes
None — no public API, request, or response shape changes. (FFI return types are internal to the binding.)
Possible Issues in the future
disposabletype calling the free callback on the original pointer after C→JS conversion. Pin/verify koffi behavior on major upgrades.core_free_stringon every returned pointer, which assumes the Rust side always returns an ownedCString::into_rawpointer. If a function is ever changed to return a borrowed/static pointer, this would become an invalid free — keep theOwnedStrreturn paired withinto_raw-style ownership.console.logper call (including result/param dumps). Not a leak, but noisy/costly at high volume — worth removing in a follow-up.