robobun 99c3824b31 fix(napi): Make cleanup hooks behavior match Node.js exactly (#21883) ...

# Fix NAPI cleanup hook behavior to match Node.js

This PR addresses critical differences in NAPI cleanup hook
implementation that cause crashes when native modules attempt to remove
cleanup hooks. The fixes ensure Bun's behavior matches Node.js exactly.

## Issues Fixed

Fixes #20835
Fixes #18827
Fixes #21392
Fixes #21682
Fixes #13253

All these issues show crashes related to NAPI cleanup hook management:
- #20835, #18827, #21392, #21682: Show "Attempted to remove a NAPI
environment cleanup hook that had never been added" crashes with
`napi_remove_env_cleanup_hook`
- #13253: Shows `napi_remove_async_cleanup_hook` crashes in the stack
trace during Vite dev server cleanup

## Key Behavioral Differences Addressed

### 1. Error Handling for Non-existent Hook Removal
- **Node.js**: Silently ignores removal of non-existent hooks (see
`node/src/cleanup_queue-inl.h:27-30`)
- **Bun Before**: Crashes with `NAPI_PERISH` error
- **Bun After**: Silently ignores, matching Node.js behavior

### 2. Duplicate Hook Prevention 
- **Node.js**: Uses `CHECK_EQ` which crashes in ALL builds when adding
duplicate hooks (see `node/src/cleanup_queue-inl.h:24`)
- **Bun Before**: Used debug-only assertions
- **Bun After**: Uses `NAPI_RELEASE_ASSERT` to crash in all builds,
matching Node.js

### 3. VM Termination Checks
- **Node.js**: No VM termination checks in cleanup hook APIs
- **Bun Before**: Had VM termination checks that could cause spurious
failures
- **Bun After**: Removed VM termination checks to match Node.js

### 4. Async Cleanup Hook Handle Validation
- **Node.js**: Validates handle is not NULL before processing
- **Bun Before**: Missing NULL handle validation 
- **Bun After**: Added proper NULL handle validation with
`napi_invalid_arg` return

## Execution Order Verified

Both Bun and Node.js execute cleanup hooks in LIFO order (Last In, First
Out) as expected.

## Additional Architectural Differences Identified

Two major architectural differences remain that affect compatibility but
don't cause crashes:

1. **Queue Architecture**: Node.js uses a single unified queue for all
cleanup hooks, while Bun uses separate queues for regular vs async
cleanup hooks
2. **Iteration Safety**: Different behavior when hooks are added/removed
during cleanup iteration

These will be addressed in future work as they require more extensive
architectural changes.

## Testing

- Added comprehensive test suite covering all cleanup hook scenarios
- Tests verify identical behavior between Bun and Node.js
- Includes edge cases like duplicate hooks, non-existent removal, and
execution order
- All tests pass with the current fixes

The changes ensure NAPI modules using cleanup hooks (like LMDB, native
Rust modules, etc.) work reliably without crashes.

---------

Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Kai Tamkun <kai@tamkun.io>
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>

2025-08-15 21:08:53 -07:00

2.0 KiB

Raw Blame History

View Raw