fs: back createWriteStream(path) with a FileSink that adopts the fd - #35993
Open
robobun wants to merge 32 commits into
Open
fs: back createWriteStream(path) with a FileSink that adopts the fd#35993robobun wants to merge 32 commits into
robobun wants to merge 32 commits into
Claude / Claude Code Review
completed
Jul 26, 2026 in 19m 29s
Code review found 3 important issues
Found 3 candidates, confirmed 5. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/internal/fs/streams.ts:458 |
kSyncWrite gate does not exclude user-supplied fd — event-loop hang / EAGAIN error on pipe fds |
| 🟡 Nit | src/js/internal/fs/streams.ts:592 |
writeAllSync retry counter never reset — diverges from writeAll's consecutive-zero semantics |
Annotations
Check failure on line 458 in src/js/internal/fs/streams.ts
claude / Claude Code Review
kSyncWrite gate does not exclude user-supplied fd — event-loop hang / EAGAIN error on pipe fds
The `kSyncWrite` gate (`!fastPath && start === undefined`) does not exclude a user-supplied numeric `fd`, so `fs.createWriteStream(null, {fd})` now routes every write through `writeSync` on the JS thread. On a blocking pipe/FIFO fd that freezes the event loop until the reader drains; on an `O_NONBLOCK` fd `writeSync` throws EAGAIN and `writeAllSync` passes it straight to `cb(e)`, erroring a stream that pre-PR retried via `writeAll`'s EAGAIN handling. Add `fd == null` to the gate (as the FileSink
Check warning on line 592 in src/js/internal/fs/streams.ts
claude / Claude Code Review
writeAllSync retry counter never reset — diverges from writeAll's consecutive-zero semantics
`writeAllSync` counts *cumulative* zero-byte writes (`++retries` on `n === 0`, never reset), while its async siblings `writeAll`/`writevAll` count *consecutive* zeros via `retries = bytesWritten ? 0 : retries + 1`. Unreachable in practice — `write(2)` on a regular file with a non-zero request never returns 0 — so this is purely sync/async-twin consistency; add `else retries = 0;` (or drop the retry guard entirely, since it's dead for regular files).
Loading