fix(oxfmt): prevent ThreadsafeFunction crash on Node.js exit#18723
Merged
graphite-app[bot] merged 1 commit intomainfrom Jan 30, 2026
Merged
fix(oxfmt): prevent ThreadsafeFunction crash on Node.js exit#18723graphite-app[bot] merged 1 commit intomainfrom
graphite-app[bot] merged 1 commit intomainfrom
Conversation
github-actions
bot
added
A-cli
Member
Author
|
Triggered from a huge monorepo: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a V8 crash (Check failed: node->IsInUse()) that occurs when Node.js exits while ThreadsafeFunction handles are held in Arc closures, preventing use-after-free errors during V8 cleanup.
Changes:
- Wrapped ThreadsafeFunctions in
Arc<Mutex<Option<...>>>to enable explicit cleanup - Added
cleanup()method to drop TSFNs before functions return - Updated wrapper functions to check TSFN availability before use
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/oxfmt/src/core/external_formatter.rs | Implements TsfnHandles struct with mutex-wrapped TSFNs, adds cleanup logic, updates all wrapper functions to lock mutex and check TSFN availability before calling |
| apps/oxfmt/src/main_napi.rs | Adds cleanup() calls before returning in both run_cli and format functions, includes cleanup in all error paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leaysgur
approved these changes
Jan 30, 2026
Member
leaysgur
left a comment
leaysgur
left a comment
There was a problem hiding this comment.
PTAL @Brooooooklyn if possible. 🙏🏻
(We're doing our regular release Monday night.)
Brooooooklyn
approved these changes
Jan 30, 2026
Member
Author
Merge activity
|
Fixes a V8 crash (`Check failed: node->IsInUse()`) that occurs when Node.js exits while ThreadsafeFunction handles are still held in Arc closures. ## Problem When running `pnpm oxfmt --check`, the process crashes on exit with: ``` Fatal error in , line 0 Check failed: node->IsInUse(). ``` The crash happens because: 1. TSFNs are captured in `Arc` closures that may live past V8 cleanup 2. When the `Arc` drops during/after V8 cleanup, the TSFN destructor tries to release an already-freed global handle This is a [known class of issue](napi-rs/napi-rs#1220) with napi-rs ThreadsafeFunction lifecycle management. ## Solution Wrap TSFNs in `Arc<Mutex<Option<...>>>` and add an explicit `cleanup()` method that drops them before the function returns. This ensures TSFNs are released while V8 handles are still valid. The wrapper functions now check if the TSFN is still available before calling, returning an error if cleanup has already occurred.
graphite-app
bot
force-pushed
the
fix/oxfmt-tsfn-cleanup-crash
branch
from
78e76c0 to
48f1e35
Compare
camc314
added a commit
that referenced
this pull request
Feb 2, 2026
# Oxlint ### 💥 BREAKING CHANGES - b34a155 linter/plugins: [**BREAKING**] `RuleTester` set `context.filename` to absolute path (#18702) (overlookmotel) ### 🚀 Features - 1753209 linter/vscode: Run extension when JS configs are detected (#18832) (camc314) - c962dd2 linter/lsp: Implement support for oxlint.config.ts (#18826) (camc314) - da32203 linter: Auto generate oxlint.config.ts types (#18597) (camc314) - 19b4df7 oxlint: Introduce `defineConfig` helper (#18596) (camc314) - ea97231 linter: Implement `oxlint.config.ts` support (#17563) (camc314) - 17ca42d linter: Implement `react/no-multi-comp` rule. (#18794) (connorshea) - 88f30e0 linter/plugins: Move eslint compatible plugin conversion to `eslintCompatPlugin` function (#18791) (overlookmotel) - 2a72794 linter/plugins: `RuleTester` take `cwd` property (#18756) (overlookmotel) - 9f533db linter: Add `find_prev_token_within` method for token search (#18769) (camc314) - 772ea70 linter: Introduce `load_js_configs` napi callback (#18767) (camc314) - e9690c1 linter: Introduce `DiscoveredConfig` in preparation for JS configs (#18674) (camc314) - 558b588 linter/prefer-namespace-keyword: Move to correctness (#18733) (camc314) - 7a5c268 oxlint/lsp: Support `jsPlugins` (#17840) (Sysix) - c07497c linter/prefer-modern-dom-apis: Implement suggestion (#17965) (Mikhail Baev) - 8531bc9 linter: Implement `prefer-const` (#18687) (camchenry) - 8670b18 parser: Error on ambient class accessor implementations (#18592) (camc314) - 6b8a5ae linter: Add `eslint-plugin-import/no-nodejs-modules` rule (#18006) (Mikhail Baev) - 04f400d linter/no-duplicates: Add support for `considerQueryString` option (#18657) (camc314) - 3b7f260 linter/consistent-generic-constructor: Implement fixer (#18616) (camc314) - 794f9e4 linter/prefer-exponentation-operator: Implement suggestion (#18602) (camc314) - 773d916 linter: `eslint/sort_keys` ignore leading and trailing spreads in auto-fix (#18485) (Lonami) - 20d4ede linter: Implement `import/no-relative-parent-imports` rule (#18513) (Valentin Maerten) - 0da45ef vscode: Fallback to globally installed oxlint/oxfmt packages (#18007) (Sysix) ### 🐛 Bug Fixes - a3417b1 linter/plugins: Clear state when reloading workspace (#18837) (overlookmotel) - c879992 linter: Error on arrays passed in as config (#18822) (camc314) - 5c80422 linter/tsdown: Ensure relative path for globals import starts with `./` (#18820) (camc314) - 7419dfb linter: Remove invalid debug assersion, add test (#18819) (camc314) - 0ca6269 ci: Fix the repo path normalization logic for tests on Windows. (#18815) (connorshea) - c7b0a65 linter: Fix config option docs for `react/jsx-boolean-value` rule. (#18811) (connorshea) - cce374e linter/prefer-const: Replace entire declaration over just the `let` kw (#18814) (camc314) - 41f92d1 linter: Error when given config options for a lint rule that has no config options defined. (#18809) (connorshea) - 0867a36 linter/consistent-index-object-style: False positive with mapped + generic types (#18801) (camc314) - 1d34b42 linter: Fix 32 bit build (#18783) (camc314) - 95df577 linter/plugins: Handle error from `destroyWorkspace` (#18763) (overlookmotel) - b3261dc linter: Fix the curly rule config to enforce the shape of the config and emit correct docs (#18743) (connorshea) - d981978 linter/plugins: Use non-blocking mode when calling `destroyWorkspace` (#18762) (overlookmotel) - 3f43d4c linter: Accept bools as valid values for `fixable` (#18772) (camc314) - 005910a linter/plugins: Support plugins outside of workspace (#18755) (overlookmotel) - fd92711 vscode: Use `fsPath` for workspace mapping (#18728) (Sysix) - 358b2c1 linter/consistent-generic-constructors: Check bounds when searching for `:` token (#18745) (connorshea) - abd0c28 linter/capitalized-comments: Fix generated rule option docs (#18748) (connorshea) - d90a9f6 linter: Add more tests for `prefer-const`'s fixer and fix its invalid behavior. (#18747) (connorshea) - f82011b oxlint/lsp: Disable JS plugins support in LSP except in tests (#18727) (overlookmotel) - 94505c8 linter/jest: Change `prefer-spy-on` autofix to suggestion (#18152) (Ben Lowery) - 6ec1112 linter: Mark unused disable directive fix as suggestion (#18703) (ddmoney420) - 49609ec linter/no-useless-constructor: Consider argument transformation as used (#18706) (ddmoney420) - 40218de linter: Fix behavior of jsx-a11y/no-static-element-interactions rule. (#17817) (connorshea) - db9751d linter/no-html-link-for-pages: Handle `target=_blank` correctly (#18693) (camc314) - e440b78 linter/plugins: Pass all args to CFG event handlers when 2 rules use same handler (#18683) (overlookmotel) - b393430 linter/curly: Fix multi-or-nest and consistent conflict (#18660) (camc314) - 2e1fbc2 linter/plugins: Implement `context.parserPath` (#18644) (overlookmotel) - 34951ed linter/plugins: `filename` option takes precedence over `parserOptions.lang` in `RuleTester` (#18643) (overlookmotel) - 28df160 linter/plugins: Allow line number passed to `report` to be 0 (#18642) (overlookmotel) - 14fabec vscode: Use built-in `getWorkspaceFolder` for detecting the right workspace of a given uri (#18583) (Sysix) - 0ff4cea oxlint/cli: Report error when nested config could not be parsed (#18504) (Sysix) ### ⚡ Performance - 9862224 linter/plugins: Reduce cost of workspaces (#18758) (overlookmotel) - 6bc0bde linter: Remove string allocation (#18725) (overlookmotel) - 3a6b41e linter/plugins: Replace ESLint Traverser with lightweight traverseNode (#18529) (Rintaro Itokawa) ### 📚 Documentation - dd1a653 linter: Fix doc comment for ignoreStateless config option. (#18808) (connorshea) - 5909085 linter/plugins: Add doc comments (#18753) (overlookmotel) - ffe53a3 linter: Update lint function docs (#18766) (camc314) - b82faec linter: Glob for any css module for no-unassigned-import (#18713) (Ben Stickley) - cd86347 linter: Mark some react rules as unsupported, misc docs improvements (#18617) (connorshea) - 23401d8 linter: Update fixes and suggestions status for tsgolint rules (#18619) (camchenry) # Oxfmt ### 🚀 Features - ee30de9 oxfmt: Add config migration from biome (#18638) (Luca Fischer) ### 🐛 Bug Fixes - e754b18 oxfmt/migrate-prettier: Set `experimentalSortPackagejson: false` by default (#18831) (leaysgur) - a83c266 formatter: Keep decorated function pattern hugged when params break (#18830) (Dunqing) - 0c8efe1 formatter: Quote numeric property keys with `quoteProps: consistent` (#18803) (Dunqing) - 9c14c3e formatter: Ignore comment does not work for sequence expressions in arrow function body (#18799) (Dunqing) - 54984ae formatter: Handle leading comments in arrow function sequence expressions (#18798) (Dunqing) - 61bb2b5 formatter: Correctly expand JSX returned from arrow callbacks in JSX expression containers (#18797) (Dunqing) - 34ee194 formatter: Tailwindcss sorting doesn't work for object property keys (#18773) (Dunqing) - 48f1e35 oxfmt: Prevent ThreadsafeFunction crash on Node.js exit (#18723) (Boshen) - e96adca formatter: Follow Prettier's approach for for-in initializer parentheses (#18695) (Dunqing) - 1215a6f formatter: Preserve quote for class property key in TypeScript (#18692) (Dunqing) - 059acae formatter: Incorrect comments placement for union type in `TSTypeIntersection` (#18690) (Dunqing) - c3d05c1 formatter,oxfmt: Handle CRLF with embedded formatting (#18686) (leaysgur) - 7cb3085 formatter: Preserve comments on rest elements (#18649) (Dunqing) - 21984dd formatter: Preserve type cast comments on rest parameters (#18648) (Dunqing) - 2f70254 formatter: Don't add extra semicolon on suppressed class properties (#18631) (Dunqing) - ac1ff4e oxfmt: Use `empty_line` IR for empty xxx-in-js line (#18623) (leaysgur) - 8f76900 oxfmt: Dedent xxx-in-js templates before calling prettier (#18622) (leaysgur) - 6b726ef oxfmt: Trim whitespace only xxx-in-js templates (#18621) (leaysgur) Co-authored-by: camc314 <[email protected]>
graphite-app bot
pushed a commit
that referenced
this pull request
Feb 3, 2026
## Summary - Replace `Mutex` with `RwLock` for ThreadsafeFunction handles in `ExternalFormatter` - Wrapper functions now use `.read()` (allows concurrent reads) instead of `.lock()` (exclusive) - Only `cleanup()` uses `.write()` since it's the only operation that modifies the Options This addresses the ~25% performance regression introduced by #18723. The original fix wrapped TSFNs in `Arc<Mutex<Option<...>>>` to prevent V8 crashes on Node.js exit, but `Mutex` serializes all callback invocations even though they only read from the Option. With `RwLock`, concurrent formatting operations can proceed without contention, only blocking for the single cleanup call at the end. Closes #18839 🤖 Generated with [Claude Code](https://claude.ai/code)
graphite-app bot
pushed a commit
that referenced
this pull request
Feb 3, 2026
## Summary - Replace `Mutex` with `RwLock` for ThreadsafeFunction handles in `ExternalFormatter` - Wrapper functions now use `.read()` (allows concurrent reads) instead of `.lock()` (exclusive) - Only `cleanup()` uses `.write()` since it's the only operation that modifies the Options This addresses the ~25% performance regression introduced by #18723. The original fix wrapped TSFNs in `Arc<Mutex<Option<...>>>` to prevent V8 crashes on Node.js exit, but `Mutex` serializes all callback invocations even though they only read from the Option. With `RwLock`, concurrent formatting operations can proceed without contention, only blocking for the single cleanup call at the end. Closes #18839 🤖 Generated with [Claude Code](https://claude.ai/code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a V8 crash (
Check failed: node->IsInUse()) that occurs when Node.js exits while ThreadsafeFunction handles are still held in Arc closures.Problem
When running
pnpm oxfmt --check, the process crashes on exit with:The crash happens because:
Arcclosures that may live past V8 cleanupArcdrops during/after V8 cleanup, the TSFN destructor tries to release an already-freed global handleThis is a known class of issue with napi-rs ThreadsafeFunction lifecycle management.
Solution
Wrap TSFNs in
Arc<Mutex<Option<...>>>and add an explicitcleanup()method that drops them before the function returns. This ensures TSFNs are released while V8 handles are still valid.The wrapper functions now check if the TSFN is still available before calling, returning an error if cleanup has already occurred.