Wait for off-thread jobs before freeing a terminated worker's VM - #36983
Closed
robobun wants to merge 14 commits into
Closed
Wait for off-thread jobs before freeing a terminated worker's VM#36983robobun wants to merge 14 commits into
robobun wants to merge 14 commits into
Claude / Claude Code Review
completed
Aug 5, 2026 in 46m 3s
Code review found 2 important issues
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/event_loop.rs:1027-1034 |
InitialStatTask (fs.watchFile) is not bracketed by the off-thread fence |
| 🔴 Important | src/runtime/napi/napi_body.rs:1788-1792 |
napi_async_work::release_for_shutdown frees addon-owned box (potential double-free) |
| 🟡 Nit | test/js/node/zlib/zlib-worker-terminate.test.ts:57-59 |
zlib-worker-terminate.test.ts: weak stderr assertion vs sibling test |
Annotations
Check failure on line 1034 in src/jsc/event_loop.rs
claude / Claude Code Review
InitialStatTask (fs.watchFile) is not bracketed by the off-thread fence
Another in-tree copy of the exact `KeepAlive::ref_`-then-`WorkPool::schedule`-then-pool-thread-`enqueue_task_concurrent` shape this PR brackets is missed by the sibling sweep: `InitialStatTask` in `src/runtime/node/node_fs_stat_watcher.rs` (`create_and_schedule` at :1160-1168 has no `offthread_job_begin()`; `run_owned` blocks in `restat_impl` then dereferences `ctx: BackRef<VirtualMachine>` via `enqueue_task_concurrent` at :666). `StatWatcherScheduler::shutdown_for_exit` spin-waits only on the p
Check failure on line 1792 in src/runtime/napi/napi_body.rs
claude / Claude Code Review
napi_async_work::release_for_shutdown frees addon-owned box (potential double-free)
`release_for_shutdown` calls `Self::destroy(this)`, freeing the `napi_async_work` box — but per the N-API contract this box is addon-owned (allocated by `napi_create_async_work`, freed only by the addon via `napi_delete_async_work`; `run_from_js` deliberately does not free it). If an addon's env-cleanup hook (which runs in `on_exit()` *before* this drain) or a synchronous EXPERIMENTAL-module finalizer during `teardownJSCVM` (*after* this drain) calls `napi_delete_async_work` on its stored handle
Check warning on line 59 in test/js/node/zlib/zlib-worker-terminate.test.ts
claude / Claude Code Review
zlib-worker-terminate.test.ts: weak stderr assertion vs sibling test
nit: This test only asserts `stderr` does not contain two specific ASAN substrings, then checks `{stdout, exitCode}` — any other stderr output (LSan reports, warnings, or the pre-existing `assertNoException` abort the sibling test explicitly tolerates) either passes silently or fails with an unhelpful diff on line 59. The sibling `worker-terminate-offthread.test.ts` added in this same PR asserts `expect(stderr).toBe("")` (with the `onlyKnownTerminateAssert` filter); align this test with that pat
Loading