fix(napi): shutdown runtime at env cleanup on windows by Brooooooklyn · Pull Request #3026 · napi-rs/napi-rs

Brooooooklyn · 2025-11-21T08:53:55Z

Summary by CodeRabbit

New Features
- Adds a Promise-based HTTP fetch API for non‑WASI targets with optional request options (method/headers), streaming responses as ReadableStreams, plus JS exports and type declarations.
Tests
- Adds end-to-end tests for fetch and async process exit handling, with timeout enforcement and a WASI skip path.
Chores
- Updates CI workflows to propagate tokens and refine job permissions.
Runtime
- Adjusts native runtime cleanup/initialization behavior for more robust env teardown.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Brooooooklyn · 2025-11-21T08:54:15Z

fix(napi): shutdown runtime at env cleanup on windows #3026 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

ready-to-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

coderabbitai · 2025-11-21T08:54:16Z

Walkthrough

Adds an async N‑API fetch binding (Rust + reqwest) that returns a JS ReadableStream, exports fetch and types in example entry points, adds AVA end-to-end tests, updates some runtime lifetimes/signatures, introduces an env cleanup hook and a C‑ABI thread cleanup callback, and updates CI token/permissions.

Changes

Cohort / File(s)	Summary
Fetch implementation `examples/napi/src/fetch.rs`	New N‑API async `fetch(env, url, request_init)` returning `AsyncBlock<Unknown<'static>>` that streams response bytes into a JS `ReadableStream`; adds `RequestInit` object (`method`, `headers`) and reqwest usage.
Example exports `examples/napi/index.cjs`, `examples/napi/example.wasi.cjs`, `examples/napi/example.wasi-browser.js`	Added `fetch` export bindings to CommonJS/WASI/browser example entry points (`module.exports.fetch` / `export const fetch`).
Type declarations & snapshots `examples/napi/index.d.cts`, `examples/napi/__tests__/__snapshots__/values.spec.ts.md`	Added TypeScript declaration for `fetch(url, requestInit?) : Promise<Response>` and `RequestInit` interface (`method?`, `headers?`); updated snapshots.
Example crate wiring & deps `examples/napi/src/lib.rs`, `examples/napi/Cargo.toml`	Registered `fetch` module under `#[cfg(not(target_family = "wasm"))]` and added `reqwest = "0.12"` with specific features and `default-features = false`.
Tests `examples/napi/__tests__/async-exit.spec.ts`, `examples/napi/__tests__/async-exit.js`	New AVA E2E tests that spawn/import example `index.cjs`, exercise `fetch`, enforce timeouts, handle process errors, allow WASI bypass via `WASI_TEST`, and assert successful exit.
Runtime lifetime & function signatures `crates/napi/src/bindgen_runtime/env.rs`, `crates/napi/src/bindgen_runtime/js_values/function.rs`	Signature changes: `Env::get_global` now returns `JsGlobal<'static>`; `Function::new_instance` now has explicit lifetime `fn new_instance<'a>(...) -> Result<Unknown<'a>>`.
Module register & cleanup hooks `crates/napi/src/bindgen_runtime/module_register.rs`	Added module-private `ENV_CLEANUP_HOOK_ADDED` (tokio_rt), start Tokio runtime and register env cleanup hook via `napi_add_env_cleanup_hook` when needed; changed `thread_cleanup` to `unsafe extern "C" fn thread_cleanup(_data: *mut c_void)`; adjusted N‑API string pointer usage.
CI workflows `.github/workflows/test-release.yaml`, `.github/workflows/asan.yml`, `.github/workflows/zig.yaml`	Added top-level/per-job `permissions`, propagated `GITHUB_TOKEN` and `TARGET_CC` into job/step envs, and adjusted docker-run options across workflows.

Sequence Diagram(s)

sequenceDiagram
    participant JS as JavaScript
    participant CJS as CommonJS binding
    participant NAPI as N-API (AsyncBlock)
    participant Rust as Rust (tokio + reqwest)
    participant HTTP as Remote HTTP

    JS->>CJS: fetch(url, requestInit?)
    CJS->>NAPI: nativeBinding.fetch(...)
    NAPI->>Rust: schedule async task (tokio runtime)
    Rust->>HTTP: send request (async)
    HTTP-->>Rust: response stream (chunks)
    Rust->>NAPI: forward chunks
    NAPI->>JS: resolve Promise with ReadableStream (pushes chunks)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pay extra attention to:
- examples/napi/src/fetch.rs — async → JS bridging, ReadableStream construction, error mapping.
- crates/napi/src/bindgen_runtime/module_register.rs — env cleanup hook registration and C‑ABI callback signature changes.
- Lifetime/signature changes in crates/napi/src/bindgen_runtime/* — callers and safety.
- CI workflow changes — permissions and token propagation.

Poem

🐇 I tunneled bytes through Rust and streams,

Promises hopped into JS dreams,
Lifetimes pinned and hooks set neat,
Tests sprint, CI hums a steady beat,
A little rabbit cheers — fetch is complete!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing runtime shutdown at env cleanup specifically on Windows, which aligns with the core modifications in module_register.rs that register an env cleanup hook.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/windows-env-cleanup-hook

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

examples/napi/__tests__/async-exit.spec.ts (1)
11-75: Consider including child output in timeout error for easier debugging

The test logic and cleanup look solid. On timeout you already capture stdout/stderr but the thrown Error message omits them, which can make debugging flaky CI timeouts harder.

You could inline the captured output into the timeout error without changing behavior otherwise:
-      new Promise<never>((_, reject) => {
-        timeout = setTimeout(() => {
-          if (child.exitCode === null && child.signalCode === null) {
-            child.kill()
-          }
-          reject(
-            new Error(`Timed out waiting ${timeoutMs}ms for child to exit`),
-          )
-        }, timeoutMs)
-      }),
+      new Promise<never>((_, reject) => {
+        timeout = setTimeout(() => {
+          if (child.exitCode === null && child.signalCode === null) {
+            child.kill()
+          }
+          reject(
+            new Error(
+              `Timed out waiting ${timeoutMs}ms for child to exit` +
+                ` (stdout: ${stdout.join('')}, stderr: ${stderr.join('')})`,
+            ),
+          )
+        }, timeoutMs)
+      }),
Purely optional; the current test is otherwise in good shape.
crates/napi/src/bindgen_runtime/module_register.rs (1)
583-605: Cleanup logic duplication and memory ordering (minor, optional)

The three cleanup paths:

non-WASM, non-Windows dtor thread_cleanup,

WASM thread_cleanup, and

Windows windows_env_cleanup_hook

all implement the same pattern:
if MODULE_COUNT.fetch_sub(1, Ordering::Relaxed) == 1 {
  crate::tokio_runtime::shutdown_async_runtime();
}
This is correct and matches prior behavior; the new Windows hook cleanly reuses the same semantics.

To reduce duplication and avoid future drift, you could factor this into a small helper, e.g.:
#[cfg(all(not(feature = "noop"), all(feature = "tokio_rt", feature = "napi4")))]
fn dec_modules_and_maybe_shutdown() {
  if MODULE_COUNT.fetch_sub(1, Ordering::Relaxed) == 1 {
    crate::tokio_runtime::shutdown_async_runtime();
  }
}
and call that from each cleanup.

If you prefer stricter consistency with the SeqCst increment, you could also consider Ordering::SeqCst here, but there’s no functional issue with Relaxed given the current use as a pure refcount.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d23d7d and 662ab5b.

📒 Files selected for processing (2)

crates/napi/src/bindgen_runtime/module_register.rs (3 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

crates/napi/src/bindgen_runtime/module_register.rs (2)

crates/napi/src/lib.rs (1)

napi_add_env_cleanup_hook (71-75)

crates/napi/src/tokio_runtime.rs (1)

shutdown_async_runtime (95-99)

examples/napi/__tests__/async-exit.spec.ts (2)

examples/napi/__tests__/unload.spec.js (1)

__dirname (10-10)

examples/napi/__tests__/test.framework.js (1)

test (97-97)

⏰ 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). (29)

GitHub Check: Test Zig Cross Compiled armv7-unknown-linux-musleabihf
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: Test freebsd target
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: Memory leak detect job
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: ASAN - windows-latest

🔇 Additional comments (1)

crates/napi/src/bindgen_runtime/module_register.rs (1)

474-483: Windows env cleanup hook wiring is correct; N-API gate aligns properly

Verification confirms that napi_add_env_cleanup_hook was introduced in N-API v3 (Node.js v10.2.0+) and is available on all platforms including Windows. The codebase feature hierarchy shows napi4 = ["napi3", "napi-sys/napi4"], meaning the napi4 gate provides N-API v4 (which includes all v3 APIs).

The code's gating on napi4 is appropriately conservative—while technically napi3 would suffice for this API, requiring napi4 is a valid design choice. The Windows platform-specific registration is properly guarded and aligns with the N-API guarantees.

No issues found.

bmjhversteeg · 2025-11-21T14:27:16Z

@Brooooooklyn I was also investigating this issue, but you beat me to it with the PR.

I found that as per mmastrac/rust-ctor#304 (comment) and https://github.com/mmastrac/rust-ctor/wiki/FAQ:-Life%E2%80%90before-and-life%E2%80%90after-main#important-considerations, using #[dtor] to cleanup tokio is actually discouraged.

Maybe stupid question, but I'm not familiar enough with ffi to know the answer, but why can't the targets windows and wasm use the existing thread_cleanup(), as was the case before #2850 ?

Edit: Just for reference, added my fix as well #3028

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6bf95f and 2bb854a.

📒 Files selected for processing (1)

examples/napi/__tests__/async-exit.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

examples/napi/__tests__/async-exit.spec.ts (2)

examples/napi/__tests__/unload.spec.js (1)

__dirname (10-10)

examples/napi/__tests__/test.framework.js (1)

test (97-97)

⏰ 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). (38)

GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: Memory leak detect job
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: Test node wasi target
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: Test freebsd target
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-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@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: ASAN - windows-latest
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)

🔇 Additional comments (5)

examples/napi/__tests__/async-exit.spec.ts (5)

1-9: LGTM! Clean imports and path setup.

The imports and path construction follow ES module best practices and match patterns used elsewhere in the test suite.

11-15: LGTM! Appropriate WASI skip.

Skipping in WASI environments is correct since child process spawning is not supported there.

24-33: LGTM! Proper child process setup and I/O collection.

The spawn configuration and stream handling are correct. Optional chaining guards against null streams, and collecting output enables useful error diagnostics.

35-56: LGTM! Solid timeout and race handling.

The race between child exit and timeout is implemented correctly. The defensive check before killing (lines 48-50) prevents errors if the child exits right as the timeout fires.

58-71: LGTM! Thorough assertions and cleanup.

The assertions verify both signal and exit code, with helpful error messages including stderr output. The finally block ensures proper cleanup of both the timeout and child process.

examples/napi/__tests__/async-exit.spec.ts

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

examples/napi/__tests__/async-exit.spec.ts (1)

24-63: Child process lifecycle and timeout handling look solid; consider including stdout in failure output

The spawn + Promise.race + finally cleanup covers normal exit, error, and timeout cases well, and the guards around child.kill() prevent unnecessary signals. For easier debugging when this test fails, you might optionally include stdout in the failure message alongside stderr.join(''), since some environments log useful info only to stdout.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bb854a and 847404b.

📒 Files selected for processing (1)

examples/napi/__tests__/async-exit.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

examples/napi/__tests__/async-exit.spec.ts (2)

examples/napi/__tests__/unload.spec.js (1)

__dirname (10-10)

examples/napi/__tests__/test.framework.js (1)

test (97-97)

⏰ 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). (38)

GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Test node wasi target
GitHub Check: Test freebsd target
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@22 - 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: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: build - 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)
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: ASAN - windows-latest
GitHub Check: Memory leak detect job

🔇 Additional comments (1)

examples/napi/__tests__/async-exit.spec.ts (1)

7-22: Windows path escaping for bindingPath looks correct now

Using fileURLToPath(import.meta.url) combined with JSON.stringify(bindingPath) inside the template literal avoids the previous Windows backslash-escaping issue for both require and the readFileAsync argument. This should be robust across platforms.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 847404b and f1eb81f.

📒 Files selected for processing (5)

crates/napi/Cargo.toml (1 hunks)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2 hunks)
crates/napi/src/lib.rs (0 hunks)
examples/napi/__tests__/async-call-exit.js (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)

💤 Files with no reviewable changes (1)

crates/napi/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (1)

examples/napi/tests/async-exit.spec.ts

⏰ 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). (35)

GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Test freebsd target
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: Test node wasi target
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: x86_64-pc-windows-msvc - node@22 - 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-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
GitHub Check: ASAN - windows-latest
GitHub Check: Memory leak detect job

🔇 Additional comments (3)

crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)

449-449: LGTM!

The trait implementation path update from futures_core::Stream to futures::stream::Stream is correct. Since futures::stream::Stream is a re-export of futures_core::Stream, this is functionally equivalent and simplifies the dependency graph.

13-13: Imports are safe to change; no tokio_stream-specific methods are used in the crate.

Verification confirms the import consolidation is safe. The codebase uses only .next() on streams (in crates/napi/src/bindgen_runtime/js_values/stream/read.rs at lines 566 and 640), which is available in both futures::stream::StreamExt and tokio_stream::StreamExt. No tokio_stream-specific methods like throttle, timeout, chunks_timeout, merge, or fuse are used anywhere in the main crate. The only remaining tokio_stream import is in the examples directory (examples/napi/src/stream.rs), which is separate from the crate changes.

examples/napi/__tests__/async-call-exit.js (1)

1-10: LGTM!

The test correctly exercises async streaming functionality by reading its own source file through acceptStream. The use of top-level await and the simple structure are appropriate for verifying that the N-API binding can handle async operations and clean up properly on exit, which aligns with the PR's objective to fix runtime shutdown during environment cleanup on Windows.

crates/napi/Cargo.toml

coderabbitai

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 (2)

crates/napi/src/bindgen_runtime/env.rs (1)

12-26: Critical soundness regression confirmed: revert Env::get_global to return JsGlobal<'_> instead of JsGlobal<'static>

The change is unsound. JsGlobal<'env> relies solely on PhantomData<&'env ()> to tie the lifetime to the environment—Value has no lifetime parameters. Changing to 'static removes this constraint entirely.

Since Value contains raw pointers (napi_env, napi_value) backed by a specific environment instance, safe code can now stash JsGlobal<'static> in statics or long-lived containers and keep using them after the environment is destroyed (e.g., on worker teardown or addon unload), resulting in use-after-free with dangling raw pointers.

The workaround example in examples/napi/src/object.rs (returning JsGlobal<'_>) suggests this constraint was intentional. If async callbacks (like fetch.rs) require 'static, confine that coercion to a localized, documented unsafe helper rather than exposing 'static from the public API.
crates/napi/src/bindgen_runtime/js_values/function.rs (1)
189-206: Function::new_instance signature enables unsound Unknown<'static> construction

The verification confirms the review's concern. The fetch.rs example (line 6) requires returning AsyncBlock<Unknown<'static>>, but Unknown doesn't implement Send. The new signature pub fn new_instance<'a>(&self, args: Args) -> Result<Unknown<'a>> with unconstrained 'a allows callers to specify 'a='static and construct Unknown<'static> from safe code:
// fetch.rs, line 6
pub fn fetch(env: &Env, url: String) -> Result<AsyncBlock<Unknown<'static>>> {
  // ... line 24:
  response_constructor.new_instance(js_stream)  // Returns Unknown<'a> where 'a unifies to 'static
}
The underlying napi_value is only valid as long as napi_env exists (typically the callback scope), yet this signature permits creating Unknown<'static> that outlives the env. This breaks the lifetime invariant that Unknown::from_napi_value (which is unsafe) requires.

Recommended fix:
Either (1) implement Send for Unknown to make AsyncBlock<Unknown<'_>> directly usable, (2) switch to UnknownRef which already implements Send, or (3) redesign the async API to avoid the 'static requirement. Changing the method to return the unconstrained Unknown<'a> treats a symptom rather than fixing the root incompatibility.

🧹 Nitpick comments (1)

examples/napi/src/fetch.rs (1)

5-37: fetch implementation is solid, but consider preserving status/headers and clarifying Response expectations

The overall structure looks good: using AsyncBlockBuilder::build_with_map with reqwest::get(url) and piping bytes_stream() into ReadableStream::create_with_stream_bytes gives a proper streaming body into JS, and error mapping is handled cleanly.

Two follow‑ups to consider:

HTTP metadata is dropped

The constructed JS Response receives only the body stream; status code, status text, and headers from reqwest::Response are ignored.

Callers expecting full fetch semantics (e.g., checking res.status, res.ok, or headers) will see default values from new Response(body) instead of the real HTTP response.

If this is intended to behave like fetch, it’s worth wiring response.status() and response.headers() into the Response init options.

Dependency on global Response

global.get_named_property("Response")? assumes globalThis.Response exists and is a compatible constructor. On Node runtimes without built‑in WHATWG fetch/Response, or in custom embeddings, this will throw unless a polyfill has been installed.

If you intend to support such environments, you may want a fallback (e.g., using an undici Response import exposed into the global, or a feature flag in the examples) or at least document the Node version / polyfill requirement.

Can you confirm:

Whether tests or consumers rely on Response.status / headers today, and if not, whether you want this binding to be spec‑accurate or “body‑only” for now?

That the minimum supported Node/runtime for these examples guarantees a global Response (or that you’re comfortable requiring a polyfill)?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54c4925 and da7b96f.

⛔ Files ignored due to path filters (1)

yarn.lock is excluded by !**/yarn.lock, !**/*.lock

📒 Files selected for processing (11)

crates/napi/src/bindgen_runtime/env.rs (1 hunks)
crates/napi/src/bindgen_runtime/js_values/function.rs (1 hunks)
examples/napi/Cargo.toml (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)
examples/napi/example.wasi-browser.js (1 hunks)
examples/napi/example.wasi.cjs (1 hunks)
examples/napi/index.cjs (1 hunks)
examples/napi/index.d.cts (1 hunks)
examples/napi/package.json (1 hunks)
examples/napi/src/fetch.rs (1 hunks)
examples/napi/src/lib.rs (1 hunks)

✅ Files skipped from review due to trivial changes (1)

examples/napi/package.json

🚧 Files skipped from review as they are similar to previous changes (1)

examples/napi/tests/async-exit.spec.ts

🧰 Additional context used

🧬 Code graph analysis (6)

crates/napi/src/bindgen_runtime/js_values/function.rs (2)

crates/napi/src/bindgen_runtime/js_values/class.rs (1)

new_instance (243-286)

crates/napi/src/js_values/function.rs (3)

new_instance (98-118)

args (54-57)

args (104-107)

crates/napi/src/bindgen_runtime/env.rs (1)

examples/napi/src/object.rs (1)

get_global (17-19)

examples/napi/example.wasi-browser.js (1)

examples/napi/src/fetch.rs (1)

fetch (6-37)

examples/napi/src/fetch.rs (2)

crates/napi/src/tokio_runtime.rs (1)

build_with_map (308-320)

crates/napi/src/bindgen_runtime/js_values/stream/read.rs (1)

Function (199-204)

examples/napi/src/lib.rs (2)

examples/napi/example.wasi-browser.js (2)

fetch (248-248)

fetch (248-248)

examples/napi/src/fetch.rs (1)

fetch (6-37)

examples/napi/index.d.cts (2)

examples/napi/example.wasi-browser.js (2)

fetch (248-248)

fetch (248-248)

examples/napi/src/fetch.rs (1)

fetch (6-37)

🔇 Additional comments (5)

examples/napi/src/lib.rs (1)

65-79: mod fetch; wiring looks consistent

Registering the new fetch module alongside the other example modules is straightforward and consistent with the existing layout; no issues from this file’s side.

examples/napi/Cargo.toml (1)

32-32: Reqwest dependency matches new fetch example usage

Adding reqwest 0.12 with the stream feature aligns with the reqwest::get(...).bytes_stream() usage in examples/napi/src/fetch.rs; looks appropriate for this examples crate.

examples/napi/example.wasi-browser.js (1)

248-248: Re-exporting fetch from __napiModule.exports looks consistent

This follows the existing pattern of passthrough exports in this file and cleanly exposes the new native fetch binding to wasi-browser consumers; no issues here.

examples/napi/example.wasi.cjs (1)

293-293: Wiring fetch through WASI CJS exports looks correct

The passthrough module.exports.fetch = __napiModule.exports.fetch is consistent with the rest of the export table and keeps the WASI CJS surface in sync.

examples/napi/index.cjs (1)

757-757: CJS export of fetch is aligned with other bindings

Exporting fetch as module.exports.fetch = nativeBinding.fetch matches the existing pattern for other N-API symbols and keeps the CJS surface complete.

examples/napi/index.d.cts

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da7b96f and 8bd9cd8.

⛔ Files ignored due to path filters (1)

examples/napi/__tests__/__snapshots__/values.spec.ts.snap is excluded by !**/*.snap

📒 Files selected for processing (11)

crates/napi/src/bindgen_runtime/env.rs (1 hunks)
crates/napi/src/bindgen_runtime/js_values/function.rs (1 hunks)
examples/napi/Cargo.toml (1 hunks)
examples/napi/__tests__/__snapshots__/values.spec.ts.md (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)
examples/napi/example.wasi-browser.js (1 hunks)
examples/napi/example.wasi.cjs (1 hunks)
examples/napi/index.cjs (1 hunks)
examples/napi/index.d.cts (1 hunks)
examples/napi/src/fetch.rs (1 hunks)
examples/napi/src/lib.rs (1 hunks)

✅ Files skipped from review due to trivial changes (1)

examples/napi/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (5)

examples/napi/index.cjs
examples/napi/example.wasi-browser.js
examples/napi/Cargo.toml
crates/napi/src/bindgen_runtime/js_values/function.rs
examples/napi/index.d.cts

🧰 Additional context used

🧬 Code graph analysis (2)

examples/napi/src/fetch.rs (2)

crates/napi/src/tokio_runtime.rs (1)

build_with_map (308-320)

crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)

Function (199-204)

create_with_stream_bytes (317-361)

crates/napi/src/bindgen_runtime/env.rs (1)

examples/napi/src/object.rs (1)

get_global (17-19)

⏰ 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). (32)

GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: Test freebsd target
GitHub Check: Test latest bun
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - 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@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: Memory leak detect job
GitHub Check: ASAN - windows-latest
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)

🔇 Additional comments (4)

examples/napi/example.wasi.cjs (1)

294-294: LGTM!

The export is correctly added and follows the established pattern in this file.

examples/napi/__tests__/async-exit.spec.ts (1)

1-9: LGTM!

The test effectively validates that async NAPI functions complete properly and allow the process to exit. The test name clearly indicates its purpose of verifying the runtime cleanup behavior mentioned in the PR objectives. Using httpbin.org/delay/1 provides a realistic async scenario with artificial latency.

examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)

759-760: LGTM!

The snapshot correctly captures the type declaration for the new fetch function. Using import('undici-types').Response is appropriate for Node.js compatibility.

crates/napi/src/bindgen_runtime/env.rs (1)

12-26: The lifetime change from JsGlobal<'_> to JsGlobal<'static> is backward compatible.

The JsGlobal<'env> struct binds its lifetime to a reference via PhantomData<&'env ()>. When the return type changes from the inferred JsGlobal<'_> to explicit JsGlobal<'static>, this is a narrowing of the lifetime—the most restrictive lifetime can be safely used where a flexible inferred lifetime is expected.

The example wrapper function in examples/napi/src/object.rs:17 that returns Result<JsGlobal<'_>> will still compile: the '_ will infer to 'static from the returned value, which is valid. All 18 callers throughout the codebase immediately consume the result through method chains and don't impose additional lifetime constraints. No code breakage will occur.

examples/napi/src/fetch.rs

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

examples/napi/index.d.cts (1)

719-719: This issue was already flagged in a previous review.

The import('undici-types').Response reference will fail to resolve if undici-types is not present in dependencies. Please see the previous review comment for this line which provides detailed guidance on resolving this issue.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7deca3 and b994ced.

📒 Files selected for processing (6)

.github/workflows/test-release.yaml (9 hunks)
examples/napi/Cargo.toml (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)
examples/napi/index.d.cts (2 hunks)
examples/napi/package.json (0 hunks)
examples/napi/src/fetch.rs (1 hunks)

💤 Files with no reviewable changes (1)

examples/napi/package.json

🚧 Files skipped from review as they are similar to previous changes (2)

examples/napi/Cargo.toml
.github/workflows/test-release.yaml

🧰 Additional context used

🧬 Code graph analysis (3)

examples/napi/src/fetch.rs (3)

examples/napi/index.d.cts (1)

RequestInit (1001-1004)

crates/napi/src/tokio_runtime.rs (1)

build_with_map (308-320)

crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)

Function (199-204)

create_with_stream_bytes (317-361)

examples/napi/__tests__/async-exit.spec.ts (3)

examples/napi/__tests__/test.framework.js (1)

test (97-97)

examples/napi/src/fetch.rs (1)

fetch (15-66)

examples/napi/example.wasi-browser.js (2)

fetch (249-249)

fetch (249-249)

examples/napi/index.d.cts (2)

examples/napi/src/fetch.rs (1)

fetch (15-66)

examples/napi/example.wasi-browser.js (2)

fetch (249-249)

fetch (249-249)

⏰ 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). (39)

GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: Test node wasi target
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
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@24 - toolchain@ 1.88.0
GitHub Check: x86_64-pc-windows-msvc - node@22 - 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-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: Test freebsd target
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: Memory leak detect job
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)
GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
GitHub Check: ASAN - windows-latest
GitHub Check: ASAN - ubuntu-24.04

🔇 Additional comments (2)

examples/napi/__tests__/async-exit.spec.ts (1)

3-19: LGTM! Test correctly exercises the async fetch API.

The test structure is clean: it correctly skips for WASI environments, performs a real HTTP fetch with appropriate headers, and validates the response structure. The optional GITHUB_TOKEN (line 12) is fine—the GitHub API will work without authentication, just with lower rate limits.

examples/napi/index.d.cts (1)

1001-1004: LGTM! Interface correctly mirrors the Rust struct.

The TypeScript RequestInit interface properly reflects the Rust #[napi(object)] struct defined in fetch.rs, with optional method and headers fields matching the expected signature.

examples/napi/src/fetch.rs

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

.github/workflows/zig.yaml (1)
146-146: Normalize GITHUB_TOKEN reference across workflows.

The new environment injections use ${{ secrets.GITHUB_TOKEN }} while pre-existing install-action steps (lines 49, 69) use ${{ github.token }}. Both provide the same token, but adopt a consistent pattern for clarity. Consider standardizing to one convention—${{ github.token }} is the modern, scoped approach for GitHub Actions workflows.

Optionally standardize to ${{ github.token }}:
-        env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        env:
+          GITHUB_TOKEN: ${{ github.token }}
Apply the same change to lines 160, 178, and 187.

Also applies to: 159-160, 177-178, 186-187

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d4928a4 and 8dfb73f.

📒 Files selected for processing (3)

.github/workflows/asan.yml (4 hunks)
.github/workflows/test-release.yaml (10 hunks)
.github/workflows/zig.yaml (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

.github/workflows/test-release.yaml

⏰ 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). (40)

GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - 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: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: Test node wasi target
GitHub Check: Test freebsd target
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
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: Memory leak detect job
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
GitHub Check: ASAN - ubuntu-24.04

🔇 Additional comments (3)

.github/workflows/zig.yaml (1)

7-7: ✓ Defensive permissions configuration.

The top-level permissions: {} establishes a secure baseline by starting with no default permissions.

.github/workflows/asan.yml (2)

10-10: ✓ Appropriate GitHub Actions security posture.

The top-level permissions: {} combined with job-level permissions: contents: read follows the principle of minimal permissions. The contents: read scope is appropriate for the test job's artifact download and checkout operations.

Also applies to: 22-23

88-88: Verify GITHUB_TOKEN injection necessity in test environments.

GITHUB_TOKEN is now injected into both Linux (line 88) and Windows (lines 110–111) test environments. Confirm whether:

Tests or tools invoked by yarn test require GitHub API access or authentication

These docker-run steps perform actions/checkout or artifact operations that require token propagation

The token is used by ASAN tooling or external dependencies

If tests do not require network access to GitHub, this token may be unnecessary. If needed, add a comment explaining the reason to aid future maintenance.

Also applies to: 110-111

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

examples/napi/__tests__/async-exit.js (1)
14-14: Expand assertion coverage.

The instanceof Response check is minimal. With the response status validation added above, you should also assert that the response was successful.
 assert(res instanceof Response)
+assert.strictEqual(res.ok, true, 'Response should be successful')

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70a1eab and f8023fc.

📒 Files selected for processing (2)

examples/napi/__tests__/async-exit.js (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

examples/napi/__tests__/async-exit.spec.ts (3)

examples/napi/__tests__/unload.spec.js (1)

__dirname (10-10)

wasm-runtime/rollup.config.js (1)

dirname (11-11)

examples/napi/__tests__/test.framework.js (1)

test (97-97)

⏰ 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). (40)

GitHub Check: Test node wasi target
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: Test freebsd target
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@ stable
GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
GitHub Check: aarch64-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@ 1.88.0
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
GitHub Check: build - s390x-unknown-linux-gnu
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 (aarch64-unknown-linux-musl)

examples/napi/__tests__/async-exit.js

examples/napi/__tests__/async-exit.spec.ts

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed82260 and 3e1ab80.

📒 Files selected for processing (1)

crates/napi/src/bindgen_runtime/module_register.rs (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

crates/napi/src/bindgen_runtime/module_register.rs (2)

crates/napi/src/bindgen_runtime/js_values/string.rs (1)

new (291-293)

crates/napi/src/lib.rs (1)

napi_add_env_cleanup_hook (71-75)

⏰ 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). (38)

GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: Test freebsd target
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: aarch64-apple-darwin - 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: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build-and-test-msys2 (CLANG64)
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)
GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
GitHub Check: Memory leak detect job
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: ASAN - windows-latest

crates/napi/src/bindgen_runtime/module_register.rs

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (3)

examples/napi/__tests__/async-exit.js (1)
5-12: Avoid silent auth failures and make the GitHub call more robust

This script will still pass if GITHUB_TOKEN is missing or invalid (401/403), and it always sends Authorization: Bearer undefined when the env var is unset. It also never inspects the HTTP status, so you don't notice GitHub-side issues.

I’d suggest at least making the auth header conditional and asserting res.ok so the test fails on auth/API errors:
-const res = await fetch('https://api.github.com/repos/napi-rs/napi-rs', {
-  headers: {
-    'X-GitHub-Api-Version': '2022-11-28',
-    Authorization: `Bearer ${process.env.GITHUB_TOKEN}`,
-    Accept: 'application/json',
-    'User-Agent': 'napi-rs/napi-rs',
-  },
-})
+const headers = {
+  'X-GitHub-Api-Version': '2022-11-28',
+  Accept: 'application/json',
+  'User-Agent': 'napi-rs/napi-rs',
+}
+
+if (process.env.GITHUB_TOKEN) {
+  headers.Authorization = `Bearer ${process.env.GITHUB_TOKEN}`
+}
+
+const res = await fetch('https://api.github.com/repos/napi-rs/napi-rs', {
+  headers,
+})
+
+if (!res.ok) {
+  throw new Error(`GitHub API request failed with status ${res.status}`)
+}
+
+// Optionally: await res.text() to fully consume the body if that better matches
+// what you want to exercise for the runtime shutdown behavior.
This keeps local runs simple (no mandatory token) but makes real auth/API problems visible in CI.
crates/napi/src/bindgen_runtime/module_register.rs (2)
112-113: This process-wide flag causes cleanup to fail for Node.js worker threads.

The ENV_CLEANUP_HOOK_ADDED flag prevents additional envs (e.g., Node.js workers) from registering their own cleanup hooks. Since every env increments MODULE_COUNT at line 270 but only the first env registers a cleanup hook, subsequent envs never decrement the counter when they shut down. This leaves the Tokio runtime running indefinitely.

This is the same critical issue flagged in the previous review for lines 470-485.

473-485: Register the cleanup hook for every env.

Line 476 protects the hook behind the process-wide ENV_CLEANUP_HOOK_ADDED flag. This means only the first napi_env ever registers thread_cleanup, but every subsequent napi_register_module_v1 call still increments MODULE_COUNT at line 270. When additional envs (e.g., Node workers) shut down, their count is never decremented, so MODULE_COUNT never reaches 1 and the Tokio runtime is left running—precisely the regression this PR is trying to fix.

Drop the global flag (or track per-env) so that each env registration pairs its MODULE_COUNT increment with a matching cleanup hook invocation.

Apply this diff to address the issue:
-#[cfg(all(feature = "tokio_rt", not(feature = "noop")))]
-static ENV_CLEANUP_HOOK_ADDED: RwLock<bool> = RwLock::new(false);
       crate::tokio_runtime::start_async_runtime();
       #[cfg(not(target_family = "wasm"))]
       {
-        let mut env_cleanup_hook_added = ENV_CLEANUP_HOOK_ADDED.write().unwrap();
-        if !*env_cleanup_hook_added {
-          check_status_or_throw!(
-            env,
-            unsafe { sys::napi_add_env_cleanup_hook(env, Some(thread_cleanup), ptr::null_mut()) },
-            "Failed to add env cleanup hook"
-          );
-          *env_cleanup_hook_added = true;
-          drop(env_cleanup_hook_added);
-        }
+        check_status_or_throw!(
+          env,
+          unsafe { sys::napi_add_env_cleanup_hook(env, Some(thread_cleanup), ptr::null_mut()) },
+          "Failed to add env cleanup hook"
+        );
       }

🧹 Nitpick comments (2)

examples/napi/__tests__/async-exit.js (1)
16-18: Use path.basename with import.meta.filename for cross‑platform logging

On Windows, import.meta.filename uses backslashes, so split('/').pop() won’t strip the directory and the log message will include the full path. You can make this portable and a bit clearer by using path.basename:
-import assert from 'node:assert'
+import assert from 'node:assert'
+import { basename } from 'node:path'
@@
-console.info(
-  `[${import.meta.filename.split('/').pop()}] All assertions passed.`,
-)
+console.info(
+  `[${basename(import.meta.filename)}] All assertions passed.`,
+)
Also, import.meta.filename is only available in relatively recent Node versions (20.11+), so just ensure that matches the minimum Node version you expect for running these examples/tests.
crates/napi/src/bindgen_runtime/module_register.rs (1)

535-548: Good: Prefer C string literals over manual pointer casts.

Using c"..." syntax (lines 535, 548) is more idiomatic and safer than the previous "...".as_ptr().cast() pattern. The compiler ensures proper null-termination and returns the correct pointer type directly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1c00ff and ac980c7.

⛔ Files ignored due to path filters (1)

examples/napi/__tests__/__snapshots__/values.spec.ts.snap is excluded by !**/*.snap

📒 Files selected for processing (16)

.github/workflows/asan.yml (4 hunks)
.github/workflows/test-release.yaml (13 hunks)
.github/workflows/zig.yaml (5 hunks)
crates/napi/src/bindgen_runtime/env.rs (1 hunks)
crates/napi/src/bindgen_runtime/js_values/function.rs (1 hunks)
crates/napi/src/bindgen_runtime/module_register.rs (5 hunks)
examples/napi/Cargo.toml (1 hunks)
examples/napi/__tests__/__snapshots__/values.spec.ts.md (2 hunks)
examples/napi/__tests__/async-exit.js (1 hunks)
examples/napi/__tests__/async-exit.spec.ts (1 hunks)
examples/napi/example.wasi-browser.js (1 hunks)
examples/napi/example.wasi.cjs (1 hunks)
examples/napi/index.cjs (1 hunks)
examples/napi/index.d.cts (2 hunks)
examples/napi/src/fetch.rs (1 hunks)
examples/napi/src/lib.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (11)

examples/napi/example.wasi-browser.js
examples/napi/tests/async-exit.spec.ts
crates/napi/src/bindgen_runtime/js_values/function.rs
examples/napi/index.cjs
examples/napi/tests/snapshots/values.spec.ts.md
.github/workflows/zig.yaml
examples/napi/src/lib.rs
examples/napi/src/fetch.rs
examples/napi/index.d.cts
examples/napi/example.wasi.cjs
crates/napi/src/bindgen_runtime/env.rs

🧰 Additional context used

🧬 Code graph analysis (1)

crates/napi/src/bindgen_runtime/module_register.rs (2)

crates/napi/src/bindgen_runtime/js_values/stream/read.rs (1)

new (214-260)

crates/napi/src/lib.rs (1)

napi_add_env_cleanup_hook (71-75)

⏰ 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). (38)

GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@20
GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
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@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@24 - 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@22 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: Memory leak detect job
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: Build only test - aarch64-linux-android
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: Test freebsd target
GitHub Check: build - x86_64-unknown-linux-gnu
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: Test node wasi target
GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
GitHub Check: ASAN - windows-latest
GitHub Check: ASAN - ubuntu-24.04

🔇 Additional comments (12)

.github/workflows/asan.yml (3)

10-10: Permission scoping added correctly.

Top-level permissions: {} appropriately restricts default workflow permissions. This aligns with GitHub Actions security best practices.

22-23: Job-level permissions properly scoped.

The contents: read permission is the minimal scope needed for the test job to checkout and read repository content.

88-88: GITHUB_TOKEN propagation appears appropriate.

Token injection into test environments is consistent across both Linux and Windows test paths. Verify that downstream tooling (yarn, cargo, or test runners) actually consumes GITHUB_TOKEN for package installation or API calls; otherwise, the injection can be removed to reduce token exposure surface.

Also applies to: 110-111

.github/workflows/test-release.yaml (5)

9-9: Top-level permissions scoped consistently.

Matches the pattern in .github/workflows/asan.yml. Default workflow permissions are appropriately restricted.

62-63: Per-job permissions consistently applied.

All modified jobs declare permissions: { contents: read }, which is the minimal scope needed for repository access. This is applied uniformly across build_and_test, test_in_docker, test-freebsd, and test-latest-bun jobs.

Also applies to: 463-464, 683-684, 777-778

212-212: GITHUB_TOKEN propagated across test environment chains.

Token is injected consistently into environments where build/test tooling (yarn, cargo) may need to authenticate for package resolution or API calls. This pattern is uniform across all modified test jobs. Like the ASAN workflow, verify that all downstream consumers actually require the token; otherwise, restrict token propagation to reduce exposure surface.

Also applies to: 554-555, 595-595, 603-604, 701-701, 811-812

337-337: TARGET_CC: clang added to cross-compilation contexts.

Explicit compiler specification in cross-build steps (build_only, build_for_test_in_docker, build-and-test-linux-armv7) improves reproducibility and consistency. Verify that clang is available in all environments where these steps execute.

Also applies to: 384-384, 593-593

549-549: Docker run options are correctly preserved.

The current line 549 explicitly includes the required volume and workdir settings: -v ${{ github.workspace }}:/build -w /build. These are hardcoded in the options parameter and always applied, independent of the matrix.settings.args content (which contains platform-specific options like --platform linux/arm64). The simplification to use matrix.settings.args does not remove any required behavior.

examples/napi/Cargo.toml (2)

58-65: Context mismatch: PR title emphasizes env cleanup, but only a dependency addition is visible.

The PR title ("fix(napi): shutdown runtime at env cleanup on windows") and PR comments focus on runtime cleanup and thread cleanup mechanisms, yet this Cargo.toml file only shows the addition of a reqwest dependency for the HTTP fetch binding. The actual env cleanup hook implementation that addresses the Windows-specific runtime shutdown is not visible in this file.

Additionally, reviewer feedback from bmjhversteeg raises architectural concerns about using #[dtor] for Tokio cleanup (which is discouraged) and questions why the existing thread_cleanup() approach cannot be used on Windows/wasm targets. These concerns cannot be assessed without seeing the actual cleanup implementation.

To better review this PR, could you provide:

The files that contain the actual env cleanup hook implementation (likely in crates/napi or examples/napi source files)?

Verification that the architectural approach addresses the reviewer's concerns about cleanup strategies?

58-65: No issues found with reqwest dependency configuration.

Verification confirms that reqwest 0.12 is appropriately specified—the latest stable version is 0.12.24 (released Oct 13, 2025). All selected features are legitimate and align with reqwest's documented capabilities: rustls-tls (HTTPS with rustls), stream (streaming bodies), charset, http2 (HTTP/2), system-proxy (proxy support), and json (JSON serialization) are all part of the library's feature set and reasonable for HTTP operations. The configuration pattern with default-features = false and explicit feature selection is sound.

crates/napi/src/bindgen_runtime/module_register.rs (2)

489-503: Good: Platform-specific cleanup logic is now explicit.

Adding target_family = "wasm" makes it clear that this napi_wrap-based cleanup path is wasm-specific, complementing the non-wasm env cleanup hook registered above.

588-592: Correct signature change to match N-API cleanup hook ABI.

The function signature now matches the napi_add_env_cleanup_hook callback signature (unsafe extern "C" fn(arg: *mut c_void)), which is necessary for the env cleanup hook registered at line 479. Removing #[ctor::dtor] aligns with the guidance that using #[dtor] for Tokio cleanup is discouraged (referenced in PR comments).

coderabbitai bot reviewed Nov 21, 2025

View reviewed changes

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch 2 times, most recently from e6bf95f to 2bb854a Compare November 23, 2025 06:18

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

examples/napi/__tests__/async-exit.spec.ts Outdated Show resolved Hide resolved

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch from 2bb854a to 847404b Compare November 23, 2025 06:32

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch from 847404b to f1eb81f Compare November 23, 2025 08:28

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

crates/napi/Cargo.toml Outdated Show resolved Hide resolved

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch from f1eb81f to 54c4925 Compare November 23, 2025 13:29

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

examples/napi/index.d.cts Outdated Show resolved Hide resolved

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch 2 times, most recently from 0d57687 to 8bd9cd8 Compare November 23, 2025 15:21

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

examples/napi/src/fetch.rs Show resolved Hide resolved

coderabbitai bot reviewed Nov 24, 2025

View reviewed changes

examples/napi/src/fetch.rs Show resolved Hide resolved

coderabbitai bot reviewed Nov 24, 2025

View reviewed changes

examples/napi/__tests__/async-exit.js Show resolved Hide resolved

examples/napi/__tests__/async-exit.spec.ts Show resolved Hide resolved

coderabbitai bot reviewed Nov 24, 2025

View reviewed changes

crates/napi/src/bindgen_runtime/module_register.rs Show resolved Hide resolved

Brooooooklyn added 10 commits November 25, 2025 17:13

fix(napi): process doesn't exit on Windows

9cc9a39

add stream tests

1dda7e7

snapshot

d1350a4

fix build

7cc5653

fix wasi

3f4c720

fix test 502

4d58f12

update snapshot

cfc6fa8

update test config

589ecb0

fix token

fcee634

fix permissions

ea5edfd

Brooooooklyn added 6 commits November 25, 2025 17:13

fix

e06d8e2

align behavior

f420708

increase timeout

146b559

change to napi_add_env_cleanup_hook

d41d886

fix compile

87d9b4d

skip arm

ac980c7

Brooooooklyn force-pushed the fix/windows-env-cleanup-hook branch from d1c00ff to ac980c7 Compare November 25, 2025 09:13

coderabbitai bot reviewed Nov 25, 2025

View reviewed changes

Brooooooklyn merged commit a10be50 into main Nov 25, 2025
75 checks passed

Brooooooklyn deleted the fix/windows-env-cleanup-hook branch November 25, 2025 09:31

github-actions bot mentioned this pull request Nov 24, 2025

chore: release #3024

Merged

coderabbitai bot mentioned this pull request Dec 2, 2025

chore(sys): add back non dyn-symbols behavior #3045

Merged

coderabbitai bot mentioned this pull request Dec 30, 2025

fix(napi): wasi debug compile error #3081

Merged

Uh oh!

Comments

Conversation

Brooooooklyn commented Nov 21, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

Brooooooklyn commented Nov 21, 2025

How to use the Graphite Merge Queue

Uh oh!

coderabbitai bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bmjhversteeg commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Brooooooklyn commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 21, 2025 •

edited

Loading

bmjhversteeg commented Nov 21, 2025 •

edited

Loading