Conversation
📝 WalkthroughWalkthroughAdds an optional finalize-callback path for async N-API futures: the callback is threaded through JsDeferred and the tokio executor, codegen is updated to use the new executor, and example/tests + CI leak-test wiring are added to validate finalization. Changes
Sequence Diagram(s)sequenceDiagram
participant Codegen as Generated Callback (fn.rs)
participant Tokio as Tokio Runtime (tokio_runtime.rs)
participant Deferred as JsDeferred (deferred.rs)
participant NAPI as N-API Env
Codegen->>Tokio: call execute_tokio_future_with_finalize_callback(fut, resolver, finalize_cb)
Tokio->>Deferred: create JsDeferred (includes finalize_cb) and tsfn
Tokio->>Tokio: spawn async task
Note over Tokio: async task completes -> Result<Data, Error>
Tokio->>Deferred: send result via tsfn / resolver wrapper
Deferred->>NAPI: napi_resolve_deferred / napi_reject_deferred (invoke resolver)
Deferred->>NAPI: invoke finalize_cb(env) -- perform cleanup (drop refs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6b11285ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/napi/src/js_values/deferred.rs (1)
298-321: Missing finalize_callback invocation in error path.When
errorisErr(err)(lines 305-316), thefinalize_callbackis not invoked. This could cause a resource leak ifdeferred_data.trace.into_rejectedfails.🐛 Proposed fix
Err(err) => { if cfg!(debug_assertions) { eprintln!("Failed to reject deferred: {err:?}"); let mut err = ptr::null_mut(); let mut err_msg = ptr::null_mut(); unsafe { sys::napi_create_string_utf8(env, c"Rejection failed".as_ptr().cast(), 0, &mut err_msg); sys::napi_create_error(env, ptr::null_mut(), err_msg, &mut err); sys::napi_reject_deferred(env, deferred, err); } } + if let Some(finalize_callback) = finalize_callback { + finalize_callback(env); + } } } } else {
🤖 Fix all issues with AI agents
In @examples/napi/package.json:
- Line 34: The package.json dependency for "@oxc-node/cli" is pinned to a
non-existent version "^0.0.35"; change the version string for the
"@oxc-node/cli" entry from "^0.0.35" to a valid published version (e.g.
"^0.0.32" or "0.0.32") and then run npm install to verify the dependency
resolves successfully.
🧹 Nitpick comments (3)
examples/napi/memory-test.ts (1)
13-18: Consider adding a timeout to fail the test if the object is not collected.The test runs indefinitely if the
WeakRefnever dereferences toundefined. Consider adding a maximum iteration count or timeout to exit with a non-zero code if the leak persists.♻️ Suggested improvement
+let iterations = 0 +const MAX_ITERATIONS = 30 // 30 seconds timeout + setInterval(() => { global.gc?.() + iterations++ if (wr.deref() === undefined) { console.info('No leak') process.exit(0) } + if (iterations >= MAX_ITERATIONS) { + console.error('Memory leak detected: object was not garbage collected') + process.exit(1) + } }, 1000)crates/napi/src/tokio_runtime.rs (1)
253-320: Consider reducing code duplication withexecute_tokio_future.The new function is nearly identical to
execute_tokio_future. Consider refactoring to haveexecute_tokio_futurecallexecute_tokio_future_with_finalize_callbackwithNoneto reduce duplication.♻️ Suggested refactor
#[cfg(not(feature = "noop"))] #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn execute_tokio_future< Data: 'static + Send, Fut: 'static + Send + Future<Output = std::result::Result<Data, impl Into<Error>>>, Resolver: 'static + FnOnce(sys::napi_env, Data) -> Result<sys::napi_value>, >( env: sys::napi_env, fut: Fut, resolver: Resolver, ) -> Result<sys::napi_value> { - let env = Env::from_raw(env); - let (deferred, promise) = JsDeferred::new(&env)?; - // ... rest of the duplicated code + execute_tokio_future_with_finalize_callback(env, fut, resolver, None) }crates/napi/src/js_values/deferred.rs (1)
170-175: Consider simplifying finalize_callback handling.The
set_finalize_callbackmethod creates a newArc<RwLock<...>>wrapper each time, which means the previous Arc is orphaned. Since this is only called once before use, this is functionally correct but could be simplified.♻️ Alternative approach
Instead of creating a new Arc, you could write to the existing RwLock:
pub fn set_finalize_callback( &mut self, finalize_callback: Option<Box<dyn FnOnce(sys::napi_env)>>, ) { - self.finalize_callback = Arc::new(RwLock::new(finalize_callback)); + *self.finalize_callback.write().expect("RwLock Poison") = finalize_callback; }This avoids creating a new Arc allocation, though in practice the impact is minimal.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/test-release.yamlcrates/backend/src/codegen/fn.rscrates/napi/src/js_values/deferred.rscrates/napi/src/tokio_runtime.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/memory-test.tsexamples/napi/package.jsonexamples/napi/src/async.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
examples/napi/example.wasi.cjsexamples/napi/src/async.rsexamples/napi/index.cjscrates/napi/src/js_values/deferred.rsexamples/napi/example.wasi-browser.js
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
Applied to files:
.github/workflows/test-release.yamlexamples/napi/package.jsonexamples/napi/src/async.rsexamples/napi/memory-test.ts
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `cargo test` for Rust unit tests and `yarn workspace examples/napi test` for integration tests
Applied to files:
.github/workflows/test-release.yamlexamples/napi/package.json
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Rebuild TypeScript definitions after making changes to Rust code using `yarn workspace examples/napi build`
Applied to files:
.github/workflows/test-release.yaml
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
examples/napi/src/async.rscrates/napi/src/js_values/deferred.rs
🧬 Code graph analysis (5)
examples/napi/index.d.cts (1)
examples/napi/example.wasi-browser.js (2)
AsyncThrowClass(77-77)AsyncThrowClass(77-77)
examples/napi/src/async.rs (5)
examples/napi/example.wasi-browser.js (4)
AsyncThrowClass(77-77)AsyncThrowClass(77-77)Status(357-357)Status(357-357)crates/napi/src/tokio_runtime.rs (1)
new(335-340)crates/napi/src/bindgen_runtime/callback_info.rs (1)
new(27-80)crates/napi/src/bindgen_runtime/js_values/array.rs (1)
new(14-29)crates/napi/src/threadsafe_function.rs (1)
new(53-59)
crates/backend/src/codegen/fn.rs (1)
crates/napi/src/tokio_runtime.rs (2)
execute_tokio_future_with_finalize_callback(256-320)new(335-340)
crates/napi/src/tokio_runtime.rs (6)
crates/napi/src/bindgen_runtime/js_values/object.rs (2)
finalize_callback(1030-1053)from_raw(704-713)crates/napi/src/js_values/object.rs (1)
finalize_callback(91-112)crates/napi/src/env.rs (1)
from_raw(72-74)crates/napi/src/js_values/deferred.rs (2)
new(29-46)new(144-157)crates/napi/src/sendable_resolver.rs (1)
new(25-30)crates/napi/src/js_values/unknown.rs (1)
from_raw_unchecked(73-82)
crates/napi/src/js_values/deferred.rs (3)
crates/napi/src/bindgen_runtime/js_values/object.rs (2)
finalize_callback(1030-1053)new(716-733)crates/napi/src/js_values/object.rs (1)
finalize_callback(91-112)crates/napi/src/threadsafe_function.rs (1)
new(53-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (43)
- GitHub Check: Agent
- GitHub Check: Cursor Bugbot
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: Test freebsd target
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
- GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
- GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
- GitHub Check: build-and-test-msys2 (CLANG64)
- GitHub Check: Test node wasi target
- GitHub Check: Test node wasi target (--cfg tokio_unstable)
- GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
- GitHub Check: Build only test - armv7-linux-androideabi
- GitHub Check: Test latest bun
- GitHub Check: Build only test - aarch64-unknown-linux-ohos
- GitHub Check: build-and-test-msys2 (UCRT64)
- GitHub Check: build - s390x-unknown-linux-gnu
- GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
- GitHub Check: build - x86_64-unknown-linux-gnu
- GitHub Check: build - aarch64-unknown-linux-gnu
- GitHub Check: build - powerpc64le-unknown-linux-gnu
- GitHub Check: Build only test - aarch64-linux-android
- GitHub Check: build-and-test-msys2 (MINGW64)
- GitHub Check: Memory leak detect job
- GitHub Check: Build only test - loongarch64-unknown-linux-gnu
- GitHub Check: build - x86_64-unknown-linux-musl
- GitHub Check: build - aarch64-unknown-linux-musl
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: ASAN - windows-latest
- GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
- GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
🔇 Additional comments (10)
.github/workflows/test-release.yaml (1)
75-75: LGTM! Leak tests added consistently across primary platforms.The leak test has been appropriately added to Ubuntu, macOS, and Windows platforms with the stable toolchain. The selective inclusion only for stable toolchain is reasonable, as leak tests don't need to run on every toolchain variant.
Also applies to: 95-95, 106-106
examples/napi/example.wasi.cjs (1)
122-122: LGTM! AsyncThrowClass export properly added.The export follows the established pattern and is correctly positioned alphabetically. Since this is an auto-generated file, the change appropriately reflects the Rust source modifications.
examples/napi/example.wasi-browser.js (1)
77-77: LGTM! AsyncThrowClass export properly added.The export follows the established ES module pattern and is correctly positioned alphabetically, consistent with the other binding files.
examples/napi/index.d.cts (1)
109-113: LGTM! AsyncThrowClass type declaration is correct.The type declaration properly defines the test class for async error scenarios. The constructor and
asyncThrowError()method signatures are appropriate for testing the async function memory leak fix described in the PR objectives.examples/napi/package.json (1)
12-12: The test script is properly configured. Thememory-test.tsfile exists and contains appropriate leak tests forAsyncThrowClassusingWeakRefand garbage collection detection.examples/napi/index.cjs (1)
590-590: LGTM!The new
AsyncThrowClassexport follows the established pattern for exporting native bindings.examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)
150-154: LGTM!The type declaration for
AsyncThrowClasscorrectly reflects the Rust implementation with a constructor and an async method returningPromise<void>.crates/backend/src/codegen/fn.rs (1)
168-172: LGTM! This is the core fix for the memory leak.The change correctly moves the cleanup of N-API references (
_args_ref.drop(env)) into a finalize callback that executes after the async task completes, ensuring proper resource cleanup regardless of success or failure.crates/napi/src/tokio_runtime.rs (1)
264-264: Verifyfinalize_callbackthread safety.The
finalize_callbackisOption<Box<dyn FnOnce(sys::napi_env)>>withoutSendbounds. This is safe because the callback is invoked via the threadsafe function on the JS thread, but it's worth documenting this invariant.examples/napi/src/async.rs (1)
46-55: LGTM!The
AsyncThrowClasstest fixture is correctly implemented following napi-rs conventions. The struct with#[napi(constructor)]and the impl block with#[napi]properly exports the class to JavaScript. The method intentionally throws an error to test the memory leak fix on error paths.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a memory leak in async functions by introducing a finalize callback mechanism that ensures proper cleanup of resources when promises are resolved or rejected.
Key changes:
- Adds a new
execute_tokio_future_with_finalize_callbackfunction that accepts a finalize callback for resource cleanup - Modifies the
JsDeferredstruct to store and invoke finalize callbacks during promise resolution/rejection - Updates code generation to use the new function and pass a cleanup callback that drops argument references
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/napi/src/tokio_runtime.rs | Adds new execute_tokio_future_with_finalize_callback function with finalize callback support |
| crates/napi/src/js_values/deferred.rs | Implements finalize callback storage and invocation in the deferred promise mechanism |
| crates/backend/src/codegen/fn.rs | Updates code generation to use new function and pass cleanup callback for dropping argument references |
| examples/napi/src/async.rs | Adds AsyncThrowClass for testing async error handling and memory leak fix |
| examples/napi/memory-test.ts | Provides memory leak test using WeakRef and garbage collection |
| examples/napi/package.json | Adds test:leak script and @oxc-node/cli dependency |
| yarn.lock | Updates lockfile with new @oxc-node/cli dependency |
| examples/napi/index.d.cts | Adds TypeScript declarations for AsyncThrowClass |
| examples/napi/index.cjs | Exports new AsyncThrowClass |
| examples/napi/example.wasi.cjs | Exports new AsyncThrowClass for WASI |
| examples/napi/example.wasi-browser.js | Exports new AsyncThrowClass for WASI browser |
| examples/napi/tests/snapshots/ | Updates snapshots with new dependencies and type definitions |
| .github/workflows/test-release.yaml | Adds memory leak test to CI workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Brooooooklyn
force-pushed
the
01-08-fix_napi_memory_leak_in_async_fn
branch
from
c6b1128 to
a197014
Compare
Brooooooklyn
force-pushed
the
01-08-fix_napi_memory_leak_in_async_fn
branch
from
a197014 to
4601780
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/napi/src/js_values/deferred.rs (1)
296-320: Finalize callback not invoked when error creation fails.In the error-of-error path (lines 308-319), when
deferred_trace.into_rejectedor error creation fails, the finalize callback is not invoked. This could lead to resource leaks as argument references won't be dropped.Consider adding finalize callback invocation in the
Err(err)arm:🔧 Suggested fix
Err(err) => { + if let Some(finalize_callback) = finalize_callback { + finalize_callback(env); + } if cfg!(debug_assertions) { eprintln!("Failed to reject deferred: {err:?}"); let mut err = ptr::null_mut(); let mut err_msg = ptr::null_mut(); unsafe { sys::napi_create_string_utf8(env, c"Rejection failed".as_ptr().cast(), 0, &mut err_msg); sys::napi_create_error(env, ptr::null_mut(), err_msg, &mut err); sys::napi_reject_deferred(env, deferred, err); } } }
🤖 Fix all issues with AI agents
In @crates/napi/src/js_values/deferred.rs:
- Around line 172-178: The current set_finalize_callback replaces the entire Arc
(self.finalize_callback = Arc::new(...)), which means existing clones keep
pointing to the old Arc and won't see updates; instead, acquire a write lock on
the existing RwLock inside self.finalize_callback and replace its inner
Option<Box<dyn FnOnce(sys::napi_env)>> (e.g., call
self.finalize_callback.write() and set *lock = finalize_callback) so all clones
sharing the Arc observe the new callback.
In @crates/napi/src/tokio_runtime.rs:
- Around line 253-320: The function execute_tokio_future_with_finalize_callback
duplicates most logic from execute_tokio_future; refactor by changing
execute_tokio_future to delegate to execute_tokio_future_with_finalize_callback
with finalize_callback = None and remove the duplicated body from
execute_tokio_future so all shared logic lives in
execute_tokio_future_with_finalize_callback (keep resolver handling and
SendableResolver/new spawn/panic handling in the single implementation). Ensure
signatures and generic bounds remain identical and that execute_tokio_future
simply forwards env, fut, and resolver to
execute_tokio_future_with_finalize_callback.
🧹 Nitpick comments (2)
examples/napi/memory-test.ts (1)
13-19: Add a timeout to prevent the test from hanging indefinitely.The
test:leakscript correctly includes the--expose-gcflag, soglobal.gcwill be available. However, the test lacks a timeout mechanism to catch cases where the object is not collected, which could cause the test to hang. Add a counter to exit with a non-zero code if garbage collection doesn't occur within a reasonable timeframe.Suggested implementation
+let attempts = 0; setInterval(() => { global.gc?.() if (wr.deref() === undefined) { console.info('No leak') process.exit(0) } + attempts++ + if (attempts > 30) { + console.error('Timeout: Object not collected after 30 seconds') + process.exit(1) + } }, 1000)examples/napi/src/async.rs (1)
47-56: Consider documenting that this is a test/example class.While the implementation correctly exercises the memory leak scenario from issues #3086 and #1903, adding a doc comment would clarify its purpose:
📝 Suggested documentation
+/// Test class to demonstrate and verify the fix for memory leaks in async methods +/// that return errors. See issues #3086 and #1903. #[napi(constructor)] pub struct AsyncThrowClass {} #[napi] impl AsyncThrowClass { #[napi] + /// Always returns an error to exercise the async error path and verify + /// that references are properly cleaned up. pub async fn async_throw_error(&self) -> Result<()> { Err(Error::new(Status::GenericFailure, "Throw async error")) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/test-release.yamlcrates/backend/src/codegen/fn.rscrates/napi/src/js_values/deferred.rscrates/napi/src/tokio_runtime.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/memory-test.tsexamples/napi/package.jsonexamples/napi/src/async.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/napi/index.d.cts
- examples/napi/index.cjs
- examples/napi/package.json
- examples/napi/example.wasi.cjs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
crates/backend/src/codegen/fn.rsexamples/napi/example.wasi-browser.jsexamples/napi/src/async.rscrates/napi/src/js_values/deferred.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
Applied to files:
crates/backend/src/codegen/fn.rsexamples/napi/memory-test.tsexamples/napi/src/async.rs.github/workflows/test-release.yaml
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `cargo test` for Rust unit tests and `yarn workspace examples/napi test` for integration tests
Applied to files:
examples/napi/memory-test.ts.github/workflows/test-release.yaml
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
examples/napi/src/async.rscrates/napi/src/js_values/deferred.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Rebuild TypeScript definitions after making changes to Rust code using `yarn workspace examples/napi build`
Applied to files:
.github/workflows/test-release.yaml
🧬 Code graph analysis (4)
crates/backend/src/codegen/fn.rs (1)
crates/napi/src/tokio_runtime.rs (3)
execute_tokio_future_with_finalize_callback(256-320)execute_tokio_future_with_finalize_callback(324-335)new(350-355)
crates/napi/src/tokio_runtime.rs (6)
crates/napi/src/bindgen_runtime/js_values/object.rs (2)
finalize_callback(1030-1053)from_raw(704-713)crates/napi/src/js_values/object.rs (1)
finalize_callback(91-112)crates/napi/src/env.rs (2)
from_raw(72-74)spawn(1041-1046)crates/napi/src/js_values/deferred.rs (2)
new(29-46)new(146-159)crates/napi/src/sendable_resolver.rs (1)
new(25-30)crates/napi/src/js_values/unknown.rs (1)
from_raw_unchecked(73-82)
examples/napi/src/async.rs (2)
examples/napi/example.wasi-browser.js (4)
AsyncThrowClass(77-77)AsyncThrowClass(77-77)Status(357-357)Status(357-357)crates/napi/src/bindgen_runtime/callback_info.rs (1)
new(27-80)
crates/napi/src/js_values/deferred.rs (2)
crates/napi/src/bindgen_runtime/js_values/object.rs (2)
finalize_callback(1030-1053)new(716-733)crates/napi/src/threadsafe_function.rs (1)
new(53-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (41)
- GitHub Check: Cursor Bugbot
- GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
- GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
- GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
- GitHub Check: build - powerpc64le-unknown-linux-gnu
- GitHub Check: build - aarch64-unknown-linux-gnu
- GitHub Check: Build only test - loongarch64-unknown-linux-gnu
- GitHub Check: build-and-test-msys2 (MINGW64)
- GitHub Check: build - x86_64-unknown-linux-gnu
- GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
- GitHub Check: Test node wasi target (--cfg tokio_unstable)
- GitHub Check: build - x86_64-unknown-linux-musl
- GitHub Check: Build only test - aarch64-linux-android
- GitHub Check: build-and-test-msys2 (UCRT64)
- GitHub Check: build-and-test-msys2 (CLANG64)
- GitHub Check: build - aarch64-unknown-linux-musl
- GitHub Check: Build only test - armv7-linux-androideabi
- GitHub Check: Build only test - aarch64-unknown-linux-ohos
- GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
- GitHub Check: build - s390x-unknown-linux-gnu
- GitHub Check: Test node wasi target
- GitHub Check: Test freebsd target
- GitHub Check: Memory leak detect job
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: ASAN - windows-latest
- GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
- GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
🔇 Additional comments (18)
.github/workflows/test-release.yaml (3)
75-75: LGTM! Leak test added to primary CI workflow.The leak test is appropriately placed after the main test suite and before type checking. This ensures memory leak validation is part of the standard CI pipeline for the stable toolchain on Ubuntu.
95-95: LGTM! Consistent leak test coverage for macOS.
106-106: test:leak script is properly defined and integrated.The
test:leakscript is correctly defined inexamples/napi/package.jsonas"oxnode --expose-gc ./memory-test.ts"and is appropriately used in the Windows workflow.crates/napi/src/js_values/deferred.rs (6)
3-6: LGTM! Well-designed finalize callback type.The
FinalizeCallbacktype correctly usesArc<RwLock<Option<Box<dyn FnOnce(...)>>>>to enable:
- Thread-safe shared ownership (
Arc)- Interior mutability for taking the callback (
RwLock)- Single invocation semantics (
FnOnce)- Optional callback support (
Option)Also applies to: 104-104
111-111: LGTM! Finalize callback properly integrated into data structures.The finalize callback is correctly:
- Added to both
DeferredData(for transport) andJsDeferred(for storage)- Cloned in the
Cloneimplementation (Arc ref-count increment)- Initialized with
Default::default()(None callback by default)Also applies to: 118-118, 133-133, 153-153
180-201: LGTM! Finalize callback properly cloned into DeferredData.The Arc clone at line 186 correctly shares the finalize callback with the thread-safe function callback data.
262-265: LGTM! Finalize callback correctly consumed.Lines 263-265 properly take the finalize callback using
RwLock::write().take(), ensuring single invocation semantics. The.expect("RwLock Poison")will panic if the lock is poisoned, which is appropriate as lock poisoning indicates a serious runtime error.
304-306: LGTM! Finalize callback invoked on error path.The finalize callback is correctly invoked after rejecting the deferred promise on the error path.
321-323: LGTM! Finalize callback invoked on success path.The finalize callback is correctly invoked after successfully resolving the deferred promise.
examples/napi/example.wasi-browser.js (1)
77-77: LGTM! AsyncThrowClass properly exposed in WASI browser wrapper.The export follows the established pattern and is correctly positioned alphabetically.
examples/napi/memory-test.ts (3)
1-1: LGTM! Import statement correct.
3-9: LGTM! Test function correctly implements leak detection pattern.The function creates an instance, triggers the async error, and returns a WeakRef for GC detection. The
.catch()handler properly handles promise rejection.
11-11: LGTM! Test invocation correct.crates/backend/src/codegen/fn.rs (1)
168-173: LGTM! Finalize callback correctly ensures argument cleanup.This change is the core fix for the memory leak described in #3086 and #1903. By moving
_args_ref.drop()into the finalize callback, N-API references are now properly released on all code paths (success, error, and panic), rather than being leaked when async functions throw errors.The finalize callback correctly:
- Captures
_args_refwithmoveclosure- Invokes
drop(env)to release N-API references- Executes after promise resolution/rejection
examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)
15-15: LGTM! Snapshot correctly reflects the new test infrastructure.The snapshot correctly captures:
- The new
@oxc-node/clidependency for leak testing- The new
AsyncThrowClasstype definition matching the Rust implementationAlso applies to: 150-154
crates/napi/src/tokio_runtime.rs (3)
322-335: LGTM! Noop stub correctly implements feature-gated fallback.The noop implementation is consistent with the existing
execute_tokio_futurenoop stub and correctly provides a no-op fallback when the noop feature is enabled.
267-273: No action needed—finalize callback behavior is correct by design.The
finalize_callbackis wrapped inArc<RwLock<Option<_>>>and shared across clones. When eitherdeferredordeferred_for_panicresolves/rejects, it calls.take()on the RwLock, which atomically extracts the callback and leavesNonefor subsequent clones. This ensures the callback is invoked exactly once, preventing both double-invocation and resource leaks. The clone pattern is intentional for panic handling, as noted in the code comment.
312-317: The finalize callback execution is thread-safe and does not require changes. The callback is invoked on the JavaScript thread via the threadsafe function mechanism, not on the spawned worker thread.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/napi/src/js_values/deferred.rs (1)
263-265: Consider adding context to the expect message.The
expect("RwLock Poison")will only trigger if a prior panic occurred while holding the lock. While acceptable, a more descriptive message could aid debugging.Suggested improvement
- let finalize_callback = RwLock::write(&deferred_data.finalize_callback) - .expect("RwLock Poison") - .take(); + let finalize_callback = RwLock::write(&deferred_data.finalize_callback) + .expect("finalize_callback RwLock poisoned by prior panic") + .take();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/test-release.yamlcrates/backend/src/codegen/fn.rscrates/napi/src/js_values/deferred.rscrates/napi/src/tokio_runtime.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/memory-test.tsexamples/napi/package.jsonexamples/napi/src/async.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/napi/example.wasi.cjs
- .github/workflows/test-release.yaml
- examples/napi/package.json
- examples/napi/tests/snapshots/values.spec.ts.md
- examples/napi/memory-test.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
examples/napi/src/async.rscrates/backend/src/codegen/fn.rsexamples/napi/example.wasi-browser.jscrates/napi/src/js_values/deferred.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
Applied to files:
examples/napi/src/async.rscrates/backend/src/codegen/fn.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
examples/napi/src/async.rscrates/napi/src/js_values/deferred.rs
🧬 Code graph analysis (4)
examples/napi/src/async.rs (4)
crates/macro/src/lib.rs (1)
napi(27-41)examples/napi/example.wasi-browser.js (4)
AsyncThrowClass(77-77)AsyncThrowClass(77-77)Status(357-357)Status(357-357)crates/napi/src/js_values/deferred.rs (2)
new(29-46)new(146-159)crates/napi/src/bindgen_runtime/callback_info.rs (1)
new(27-80)
crates/backend/src/codegen/fn.rs (1)
crates/napi/src/tokio_runtime.rs (2)
execute_tokio_future_with_finalize_callback(256-320)execute_tokio_future_with_finalize_callback(324-335)
examples/napi/index.d.cts (1)
examples/napi/example.wasi-browser.js (2)
AsyncThrowClass(77-77)AsyncThrowClass(77-77)
crates/napi/src/js_values/deferred.rs (2)
crates/napi/src/bindgen_runtime/js_values/object.rs (2)
finalize_callback(1030-1053)new(716-733)crates/napi/src/threadsafe_function.rs (1)
new(53-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Test Zig Cross Compiled aarch64-apple-darwin
- GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
- GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
- GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
- GitHub Check: Test freebsd target
- GitHub Check: build-and-test-msys2 (CLANG64)
- GitHub Check: build-and-test-msys2 (UCRT64)
- GitHub Check: build-and-test-msys2 (MINGW64)
- GitHub Check: build - powerpc64le-unknown-linux-gnu
- GitHub Check: Build only test - aarch64-linux-android
- GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
- GitHub Check: Memory leak detect job
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: ASAN - windows-latest
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
crates/backend/src/codegen/fn.rs (1)
168-172: Correct fix for the async memory leak.The change properly ensures
_args_ref(theNapiRefContainerholding N-API references) is dropped via the finalize callback after async completion, regardless of whether the promise resolves or rejects. This addresses the root cause of issues #3086 and #1903.examples/napi/src/async.rs (1)
46-56: Good test fixture for memory leak verification.This class provides a minimal reproduction case that always rejects the async promise, which is essential for testing that N-API references are properly released on the error path.
crates/napi/src/js_values/deferred.rs (2)
104-104: Appropriate type design for the finalize callback.The
Arc<RwLock<Option<...>>>pattern is correctly chosen:
Arcenables sharing across the clone for panic handlingRwLockprovides interior mutability totake()the callbackOptionensures the callback is consumed exactly once
304-326: Finalize callback correctly invoked on all exit paths.The callback is properly invoked in all three scenarios:
- Successful resolution (line 324-326)
- Rejection with valid error value (line 304-306)
- Rejection with fallback error on error creation failure (line 309-311)
This ensures N-API references are always cleaned up, fixing the memory leak.
crates/napi/src/tokio_runtime.rs (2)
253-320: Well-implemented finalize callback support.The new function correctly:
- Sets the finalize callback on the deferred before spawning
- Clones the deferred for panic handling (sharing the same
Arc<RwLock<...>>)- Ensures the callback is invoked regardless of success, error, or panic
The mirroring of
execute_tokio_futurestructure makes the code easy to follow and maintain.
322-335: Noop stub correctly implemented.The stub returns
Ok(std::ptr::null_mut())and ignores the finalize callback, consistent with the existingexecute_tokio_futurenoop pattern.examples/napi/index.cjs (1)
590-590: Auto-generated export correctly added.The
AsyncThrowClassexport follows the established pattern and is correctly positioned alphabetically.examples/napi/example.wasi-browser.js (1)
77-77: Auto-generated WASI browser export correctly added.The export is correctly positioned and follows the established pattern.
examples/napi/index.d.cts (1)
109-113: Auto-generated TypeScript declaration correctly added.The declaration accurately reflects the Rust implementation:
- Empty constructor matching the unit struct
asyncThrowError(): Promise<void>matchingasync fn async_throw_error(&self) -> Result<()>
1 participant
asyncmethod leaks memory #3086Note
Addresses async memory leaks by ensuring N-API references are released when async promises settle.
execute_tokio_future_with_finalize_callback,JsDeferred::set_finalize_callback, and finalize-callback plumbing indeferred.rsto run cleanup on resolve/reject; fixes TSFN release handlingexecute_tokio_futureusage withexecute_tokio_future_with_finalize_callbackand passes_args_ref.drop(env)as the finalize callbackAsyncThrowClasswithasyncThrowError, updates exports and type defsmemory-test.tsandtest:leakscript (via@oxc-node/cli) and runs it in CI on Linux/macOS/WindowsWritten by Cursor Bugbot for commit 4601780. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.