Skip to content

napi: keep async cleanup hook handles alive until the addon removes them - #37204

Merged
Jarred-Sumner merged 5 commits into
mainfrom
farm/78bea1bc/napi-async-cleanup-hook-handle-uaf
Aug 9, 2026
Merged

napi: keep async cleanup hook handles alive until the addon removes them#37204
Jarred-Sumner merged 5 commits into
mainfrom
farm/78bea1bc/napi-async-cleanup-hook-handle-uaf

test: surface napi statuses in async cleanup hook fixture output

46f360e
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 8, 2026 in 14m 44s

Code review found 2 potential issues

Found 3 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/jsc/bindings/napi.h:388-393 removeAsyncCleanupHook: dead bool return and redundant null check
🟡 Nit test/napi/napi.test.ts:1738-1743 Test lacks explicit assertion of expected lifecycle output

Annotations

Check warning on line 393 in src/jsc/bindings/napi.h

See this annotation in the file changed.

@claude claude / Claude Code Review

removeAsyncCleanupHook: dead bool return and redundant null check

Now that this function unconditionally deletes the handle and returns `true`, the `bool` return type carries no information — the sole caller (`napi_remove_async_cleanup_hook` at napi.cpp:3298) already null-checks `handle` and discards the return value. Since this PR rewrote the function body and made the return degenerate, it should become `void` and drop the redundant `if (handle == nullptr) return false;` guard.

Check warning on line 1743 in test/napi/napi.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Test lacks explicit assertion of expected lifecycle output

The test relies solely on `checkSameOutput` (Bun == Node) without pinning the expected lifecycle lines, so if both runtimes short-circuited identically the test would pass without exercising `napi_remove_async_cleanup_hook`. Since the fixture already prints `status=%d` for this purpose, consider capturing the result and asserting e.g. `expect(result).toContain("async cleanup hook removed: status=0")` and `"tsfn finalize: removing async cleanup hook"` — CodeRabbit's follow-up asked for this and t