Skip to content

FileSink: settle the pending write()'s promise in every synchronous close()/end() flush arm - #35365

Merged
Jarred-Sumner merged 4 commits into
mainfrom
farm/f1049b3d/filesink-close-orphaned-pending
Jul 24, 2026
Merged

FileSink: settle the pending write()'s promise in every synchronous close()/end() flush arm#35365
Jarred-Sumner merged 4 commits into
mainfrom
farm/f1049b3d/filesink-close-orphaned-pending

Settle the pending write() in end()'s Done/Wrote arms too; move test …

dd00437
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 24, 2026 in 24m 46s

Code review found 2 important issues

Found 2 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 2
🟡 Nit 0
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important test/js/bun/util/filesink.test.ts:352-373 drainedArm sub-test is platform-dependent and will fail on macOS CI
🔴 Important src/runtime/webcore/FileSink.rs:1058-1066 end_from_js's Done/Wrote arms leave a backpressured write()'s promise unsettled

Annotations

Check failure on line 373 in test/js/bun/util/filesink.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

drainedArm sub-test is platform-dependent and will fail on macOS CI

The `drainedArm` sub-test assumes `close()`'s flush drains the whole remaining buffer in one shot, but that depends on the platform's AF_UNIX buffer size relative to the 300KB payload. On macOS (`net.local.stream.sendspace` defaults to 8192) `flush()` returns `Pending` — not `Done`/`Wrote` — so `run_pending_later()` is never scheduled, nobody drains `readFd` during the 500ms await, and `settled` stays `"timeout"` → the assertion fails on macOS CI. Either drop the drained-arm sub-test (the errArm

Check failure on line 1066 in src/runtime/webcore/FileSink.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

end_from_js's Done/Wrote arms leave a backpressured write()'s promise unsettled

`end_from_js()`'s `Done` and `Wrote` arms have the exact same bug this PR fixes in `end()`: they call `writer.end()` and return a plain `js_number` without checking `self.pending` or scheduling `run_pending`, so a backpressured `write()`'s promise is stranded forever. The PR's own `drainedArm` test with `sink.end()` in place of `sink.close()` triggers it, and the new comment here claims the fix "mirrors … `end_from_js`" — but `end_from_js` only mirrors the `Err` arm. Per REVIEW.md's fix-the-whol