Bun.serve: stop FIFO file responses from keeping the process alive - #37083
Open
robobun wants to merge 6 commits into
Open
Bun.serve: stop FIFO file responses from keeping the process alive#37083robobun wants to merge 6 commits into
robobun wants to merge 6 commits into
Claude / Claude Code Review
completed
Aug 7, 2026 in 41m 49s
Code review found 1 important issue
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/server/FileResponseStream.rs:548-550 |
Windows: fd closed while uv_fs_read may still be running on threadpool |
| 🟡 Nit | test/js/bun/http/bun-serve-file.test.ts:1201-1203 |
Abort test does not isolate the finish() take_read_ref() fix |
Annotations
Check failure on line 550 in src/runtime/server/FileResponseStream.rs
claude / Claude Code Review
Windows: fd closed while uv_fs_read may still be running on threadpool
On Windows, `finish()`'s new `drop(self.take_read_ref())` lets `FileResponseStream::Drop` run on abort while a `uv_fs_read` is still executing on a threadpool worker; `Drop` queues `Closer::close(fd)` (async `uv_fs_close`) *before* the reader field-drop reaches `detach_borrowed_fd() → uv_cancel`, and `uv_cancel` cannot stop an op that is already running — so another worker can `_close(fd)` concurrently with the in-flight read on a possibly-recycled CRT fd slot. This is the fd half of the same ha
Check warning on line 1203 in test/js/bun/http/bun-serve-file.test.ts
claude / Claude Code Review
Abort test does not isolate the finish() take_read_ref() fix
The abort fixture's `closeSync(writerFd)` lets the process exit via the pre-existing `on_reader_done` path (last-writer-close → HUP → `take_read_ref()`), so on Linux deleting `drop(self.take_read_ref())` at FileResponseStream.rs:550 breaks neither new test. Drop the `closeSync(writerFd)` (and the now-unused `closeSync` import) from this fixture — the raw sync fd doesn't ref the event loop and is reaped at process exit — so the abort is the only exit path and the test isolates the `finish()` clau
Loading