FileSink: deliver deferred-flush EPIPE to the pending write() promise - #35351
Closed
cirospaciari wants to merge 2 commits into
Closed
FileSink: deliver deferred-flush EPIPE to the pending write() promise#35351cirospaciari wants to merge 2 commits into
cirospaciari wants to merge 2 commits into
Claude / Claude Code Review
completed
Jul 24, 2026 in 35m 12s
Code review found 1 important issue
Found 3 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/webcore/FileSink.rs:1137-1161 |
Sibling FileSink::end() Err arm still orphans the pending write() promise |
| 🟡 Nit | test/js/bun/util/filesink.test.ts:293-309 |
New test's assertions pass on pre-fix code — only fails via incidental cleanup hang |
Annotations
Check failure on line 1161 in src/runtime/webcore/FileSink.rs
claude / Claude Code Review
Sibling FileSink::end() Err arm still orphans the pending write() promise
The sibling `FileSink::end()` (reached via `sink.close()` → `${name}__doClose` → `js_close`, and via `${controller}__close` on the piped-stream path) has the identical `WriteResult::Err` arm and is left unfixed: it sets `done=true`, calls `writer.end()`, and returns — never latching `pending.result` or scheduling `run_pending_later()`. A backpressured `write()`'s promise is therefore still orphaned forever when `close()` (rather than `end()`) hits synchronous EPIPE. REVIEW.md requires covering t
Check warning on line 309 in test/js/bun/util/filesink.test.ts
claude / Claude Code Review
New test's assertions pass on pre-fix code — only fails via incidental cleanup hang
Both assertions in this test — `expect(caught?.code).toBe("EPIPE")` and `expect(unhandled).toBeNull()` — already pass on pre-fix code (773be9df); the test only fails there because the finally block's defensive `await sink.end()` returns the never-settled write() promise and times out. To make the assertions load-bearing on the actual fix, hold the write() promise and assert it settles rejected with EPIPE (e.g. `const wp = sink.write(...); …; await expect(wp).rejects.toMatchObject({code:'EPIPE'})
Loading