Repository navigation
fix(addon): addObservableCleanupHook registers a distinct pair per call - #123
Merged
Merged
Conversation
Every call registered (observable_cleanup_hook, NULL), so a second call in one environment handed Node's cleanup queue a duplicate pair, which it refuses with a CHECK that aborts the process (Deno panics on it too). Found auditing the cleanup-hook sites after #122's function-pointer fix; this one is a duplicate pair, not a pointer mismatch. Each call now registers its own heap byte as `arg`, freed by the hook, which runs once at teardown. tests/cleanup_hook_observed.test.js gains a test that registers twice and asserts exit 0 and two markers after the program's output; it failed with SIGABRT before the fix. Checked by hand under Bun 1.3.11 and Deno 2.9.6 as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013tXV7MkRfP97g5hPfNNXHC
codetalcott
marked this pull request as ready for review
September 26, 2026 18:39
codetalcott
enabled auto-merge
September 26, 2026 18:39
Merged
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
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.
Problem
addObservableCleanupHook()(a test export) registered(observable_cleanup_hook, NULL)on every call. A second call in one environment therefore handed Node's cleanup queue a duplicate pair. Node refuses that with a CHECK inCleanupQueue::Addthat aborts the process, and Deno panics on it too.I found this while auditing the cleanup-hook call sites after the function-pointer fix in codetalcott/napi-mojo#122. It is a separate bug: the pair really is duplicated, rather than two pointers failing to match. The rest of the audit found nothing:
napi_remove_env_cleanup_hookis the only N-API call that matches a removal by function pointer, and the only other add/remove pair (removeCleanupHook) uses a single local pointer for both calls.Change
arg. The hook frees it when it runs, which happens once, at teardown.check_statusraises.tests/cleanup_hook_observed.test.jsgains a test that registers twice and asserts exit 0, empty stderr, and two markers after the program's own output.Testing
SIGABRT(Node's duplicate-pair assertion) before the fix, and passes after it.🤖 Generated with Claude Code
https://claude.ai/code/session_013tXV7MkRfP97g5hPfNNXHC
Generated by Claude Code