fetch: encode FetchTasklet's cross-thread ownership in the type system#31745
Open
alii wants to merge 20 commits into
Open
fetch: encode FetchTasklet's cross-thread ownership in the type system#31745alii wants to merge 20 commits into
alii wants to merge 20 commits into
Claude / Claude Code Review
completed
Jun 4, 2026 in 30m 6s
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/runtime/dispatch.rs:1193-1207 |
on_progress_update's shutdown branch is a fourth exit path that skips take_streaming_refs_for_exit |
| 🟡 Nit | src/threading/guarded.rs:120-124 |
GuardedLock !Sync over-restricts vs std::sync::MutexGuard; comment is inaccurate |
Annotations
Check warning on line 1207 in src/runtime/dispatch.rs
claude / Claude Code Review
on_progress_update's shutdown branch is a fourth exit path that skips take_streaming_refs_for_exit
Completeness follow-up to the now-resolved threads at HTTPThread.rs:1082 and dispatch.rs (fixed via `take_streaming_refs_for_exit` in 2bb34e8): there is a fourth `is_shutting_down()` exit path — `on_progress_update` at FetchTasklet.rs:1168-1183 — that releases only `release_js_ref(t.task)` when `is_done`, without calling `take_streaming_refs_for_exit`. Today the JS loop never ticks in that window (per the FIXME at VirtualMachine.rs:1541-1543), so this is defensive-completeness only; but the take
Check warning on line 124 in src/threading/guarded.rs
claude / Claude Code Review
GuardedLock !Sync over-restricts vs std::sync::MutexGuard; comment is inaccurate
The comment "keep the guard !Send/!Sync like MutexGuard" (and the PR description) is inaccurate: `std::sync::MutexGuard<'_, T>` is `!Send` but **does** implement `Sync where T: Sync`, whereas `PhantomData<*const ()>` makes `GuardedLock` both `!Send` *and* `!Sync`. No functional impact (nothing needs `GuardedLock: Sync`), but either reword the comment to say "stricter than `std::sync::MutexGuard`" or add `unsafe impl<Value: Sync, M: RawMutex + Sync> Sync for GuardedLock<'_, Value, M>` to actually
Loading