fix(napi): handle ThreadsafeFunction callback errors gracefully during shutdown#3188
fix(napi): handle ThreadsafeFunction callback errors gracefully during shutdown#3188Brooooooklyn merged 1 commit intomainfrom
Conversation
…g shutdown When a Node.js environment is tearing down (e.g. Ctrl+C during watch mode in a worker thread), NAPI calls can fail with unexpected status codes. Reported in rolldown/rolldown#8951: pressing Ctrl+C while rolldown's watch mode is performing its initial build causes a reproducible panic in napi-rs. The panic occurs in `handle_call_js_cb_status` in the `else` branch, which handles status codes that are neither `napi_ok` nor `napi_pending_exception`. The full control flow is: 1. `call_js_cb` is the C callback invoked by Node's ThreadsafeFunction machinery on the main/JS thread. 2. It calls `napi_call_function` to invoke the JS callback, or `napi_fatal_exception` when the Rust callback errors. 3. The return status flows to `handle_call_js_cb_status(status, raw_env)`. 4. If status is `napi_pending_exception`, the function clears the exception and calls `napi_fatal_exception` — this path already tolerates `napi_pending_exception` as a return from `napi_fatal_exception`. 5. For any other non-ok status, the `else` branch constructs a JS Error object (via `napi_create_string_utf8` × 2 + `napi_create_error`) and calls `napi_fatal_exception`. Every one of these calls used `assert_eq!` against `napi_ok`, which panics during environment teardown. During shutdown, `napi_fatal_exception` returns `napi_pending_exception` (status 10) instead of `napi_ok`, and the string/error construction calls can also fail. The fix: - Replace `assert_eq!` with early returns on `napi_create_string_utf8` and `napi_create_error` — if we can't even construct the error object in a half-torn-down environment, there's nothing useful we can do. - Tolerate `napi_pending_exception` from `napi_fatal_exception`, matching the pattern already used in the `napi_pending_exception` branch above. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughError handling in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
When a Node.js environment is tearing down (e.g. Ctrl+C during watch mode
in a worker thread), NAPI calls can fail with unexpected status codes.
Reported in rolldown/rolldown#8951: pressing Ctrl+C while rolldown's watch
mode is performing its initial build causes a reproducible panic in napi-rs.
The panic occurs in
handle_call_js_cb_statusin theelsebranch, whichhandles status codes that are neither
napi_oknornapi_pending_exception.The full control flow is:
call_js_cbis the C callback invoked by Node's ThreadsafeFunctionmachinery on the main/JS thread.
napi_call_functionto invoke the JS callback, ornapi_fatal_exceptionwhen the Rust callback errors.handle_call_js_cb_status(status, raw_env).napi_pending_exception, the function clears the exceptionand calls
napi_fatal_exception— this path already toleratesnapi_pending_exceptionas a return fromnapi_fatal_exception.elsebranch constructs a JS Errorobject (via
napi_create_string_utf8× 2 +napi_create_error) andcalls
napi_fatal_exception. Every one of these calls usedassert_eq!against
napi_ok, which panics during environment teardown.During shutdown,
napi_fatal_exceptionreturnsnapi_pending_exception(status 10) instead of
napi_ok, and the string/error construction callscan also fail. The fix:
assert_eq!with early returns onnapi_create_string_utf8andnapi_create_error— if we can't even construct the error object in ahalf-torn-down environment, there's nothing useful we can do.
napi_pending_exceptionfromnapi_fatal_exception, matchingthe pattern already used in the
napi_pending_exceptionbranch above.Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Note
Medium Risk
Changes error-handling behavior in
ThreadsafeFunctioncallbacks by replacing panicking assertions with early returns, which could mask some unexpected failures but should prevent crashes during teardown.Overview
Prevents panics in
handle_call_js_cb_statuswhenThreadsafeFunctioncallback invocation fails during Node environment shutdown.Instead of
assert_eq!-ing N-API calls tonapi_okwhile constructing a JSError, the code now bails out early ifnapi_create_string_utf8/napi_create_errorfail, and it toleratesnapi_pending_exceptionfromnapi_fatal_exception(matching the existing pending-exception path).Written by Cursor Bugbot for commit 9a7d9cc. This will update automatically on new commits. Configure here.
Summary by CodeRabbit