Fix use-after-free when a worker is terminated with a fetch in flight - #31692
Closed
robobun wants to merge 6 commits into
Closed
Fix use-after-free when a worker is terminated with a fetch in flight#31692robobun wants to merge 6 commits into
robobun wants to merge 6 commits into
Claude / Claude Code Review
completed
Jun 7, 2026 in 33m 46s
Code review found 2 potential issues
Found 2 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/web_worker.rs:1253-1266 |
Step-2 SAFETY comment '&mut is exclusive' now contradicted by the adjacent close() comment |
| 🟡 Nit | test/js/web/workers/worker-terminate-lifetime.test.ts:124-128 |
Test fixture's inFlight promise lacks an error-rejection path |
Annotations
Check warning on line 1266 in src/jsc/web_worker.rs
claude / Claude Code Review
Step-2 SAFETY comment '&mut is exclusive' now contradicted by the adjacent close() comment
The pre-existing SAFETY comment at `let vm = unsafe { &mut *vm_ptr }` (line 1220-1221: "unpublished above under vm_lock, so no other thread can dereference it now — `&mut` is exclusive") now reads as contradicted by the PR's own new comment ~33 lines below: "`close()` synchronizes with any in-flight gated enqueue" — i.e., while this `&mut` binding is live across `on_exit()`/`close_all_socket_groups()` and into `close()`, the HTTP thread may be inside a gated section holding `&VirtualMachine` via
Check warning on line 128 in test/js/web/workers/worker-terminate-lifetime.test.ts
claude / Claude Code Review
Test fixture's inFlight promise lacks an error-rejection path
Nit: the per-iteration `await inFlight` only resolves via `worker.onmessage` — there's no rejection path, so if the worker errors before posting (load failure, the `fetch` rejecting, etc.) the subprocess hangs at `await inFlight` until the outer 20s/60s timeout with no diagnostic. Per CLAUDE.md ("Tests reviewers reject" → "Wire EVERY failure event … to reject the awaited promise"), and matching the sibling tests in this directory (`worker.test.ts`, `worker-postmessage-transfer.test.ts`), add `wo
Loading