Skip to content

FileSink: close a file before end() or Bun.write() reports it closed - #46088

Open
robobun wants to merge 3 commits into
mainfrom
robobun/87678816/close-fd-before-reporting-closed
Open

robobun wants to merge 3 commits into
mainfrom
robobun/87678816/close-fd-before-reporting-closed

Conversation

@robobun

@robobun robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • await writer.end() and await Bun.write(path, stream) settle with the file still open. Running it gives ETXTBSY (809 to 882 of 1,000). No user reported this.
  • PollOrFd::close_impl (src/io/pipes.rs:107) queues close(2) on the work pool, then calls on_close at once (wip use wrapper for managing process  #8456, 1.0.31).

Fix

  • A POSIX FileSink owns the fd of its file and closes it in on_close, before it reports. close_impl is unchanged.
  • Bun.write(dest, stream) reports through a promise: the pool closes the file, then it settles.
  • writer.end() and close() report by returning: they close in place.
  • Verified: test/js/bun/util/filesink.test.ts (17 of 18 new rows fail on main). Self-reviewed: 32 concerns raised, 30 addressed. Not taken: two test rows (a race, a pipe).

Background

Downsides

Notes

History of this PR. The first revision (41dbee2) closed the fd on the calling thread in PollOrFd::close_impl, for every POSIX handle. A self-review found two things. Every measured failure belongs to one owner, a FileSink on a file. And the same change for pipes, sockets and readers needs a ruling that nobody has given: #45424 and #42819 close in the owner and leave close_impl. So this revision closes the file in its owner. close_impl, FileReader and nine other test files are as on main again. The pipe and reader half goes to a PR of its own.

No user issue. Nobody filed this. An automated fuzz run found it. What is in the repository:

Repro. Each round writes a small shell script to a file, waits for bun to say that it is done, makes it executable and runs it.

// bun etxtbsy.mjs [writer | unawaited | response | existing | large | control] [rounds]
import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
const how = process.argv[2] ?? "writer", R = Number(process.argv[3] ?? 1000), dir = mkdtempSync(join(tmpdir(), "run-after-end-"));
const stream = t => new Response(new ReadableStream({ start(c) { c.enqueue(typeof t === "string" ? new TextEncoder().encode(t) : t); c.close(); } }));
const pad = Buffer.alloc(256 * 1024, "#");
const put = {
  writer: async (f, t) => { const w = Bun.file(f).writer(); w.write(t); await w.end(); },
  unawaited: (f, t) => { const w = Bun.file(f).writer(); w.write(t); w.end(); },
  response: (f, t) => Bun.write(f, stream(t)),
  existing: (f, t) => { writeFileSync(f, "old"); return Bun.write(f, stream(t)); },
  large: (f, t) => Bun.write(f, stream(Buffer.concat([Buffer.from(t), pad]))),
  control: (f, t) => writeFileSync(f, t),
}[how];
let busy = 0, other = 0;
for (let k = 0; k < R; k++) {
  const f = join(dir, "tool" + k);
  const p = put(f, `#!/bin/sh\necho v${k}\n`);
  if (how !== "unawaited") await p;
  chmodSync(f, 0o755);
  let out;
  try { out = Bun.spawnSync([f]).stdout.toString().trim(); } catch (e) { out = e.code ?? String(e); }
  if (out !== "v" + k) { out === "ETXTBSY" ? busy++ : other++; }
  rmSync(f, { force: true });
}
rmSync(dir, { recursive: true, force: true });
console.log(`${how}: ETXTBSY ${busy} of ${R}, other failures ${other}`);

ETXTBSY per 1,000 rounds, Linux x64, 12 cores, release builds. "main" is 9634520, "this PR" is 9092e8e. The rows of released versions are from the first revision of this PR.

build writer unawaited response existing large control
1.0.30 0 0
1.0.31 (first release with #8456) 872
1.4.3 861 899 0
main 809 879 882 870 834 0
main, taskset -c 2,3 754 978
this PR 0 0 0 0 0 0
this PR, taskset -c 2,3 0 0

Before #8456 FileSink.cleanup() closed the fd in place and then ran the pending promise.

What "closed" means. The descriptor is closed in this process when end() returns or the promise settles. A child that another thread forks at that moment holds a copy until its exec. With 2 and 4 Workers that spawn children in a loop, the writer repro got ETXTBSY in 2 and 14 of 2,000 rounds on the first revision (the same in-place close as now), and in 5 and 15 of 2,000 with fs.writeFileSync. On main it was 1,744 of 2,000 with 2 Workers.

The design.

  • A FileSink reports "closed" in three ways: end() returns, close() returns, or the promise of Bun.write(dest, stream) settles.
  • FileSink::setup takes the fd of every sink that is not polled (a file). PosixStreamingWriter.close_fd is false for it, and the fd goes to FileSink.file with the id of the script that opened it. The writer still calls on_close when it lets go of its handle. It does not close the fd. The shell's IOWriter owns its fd in the same way on main.
  • FileSink::on_close closes the file before report_closed. A sink with a pending promise (pipe_stream made one: Bun.write(dest, stream), for a path or an fd) gives the fd to a bun_jsc::Job on the work pool. The job's completion calls report_closed, which settles the promise. Every other sink (writer()) closes in place.
  • The job is filed under the script that called Bun.write() (OwnedFile.opener), and not under the script that runs when the stream ends. With Bun.ModuleGraph these can differ. A job of a disposed graph comes back without its completion, so its promise stays pending, as docs/runtime/module-graph.mdx says for a graph. The fd is closed and the sink is released on that path too (Drop for StreamFile, Drop for ClosingSink).
  • report_closed asserts in debug builds that the sink holds no file.
  • A sink that is never ended sends its file to bun_io::Closer, as on main. Nothing reports there.
  • start() on an open writer: the old file is closed through on_close when the new fd is a file, and in setup when the new fd is a pipe (main leaves that one open).
  • A direct stream whose pull() throws made Bun.write() return a rejected promise with the file still open: the sink was dropped without on_close. That throw now goes through handle_reject_stream, so the promise rejects after the close.
  • One report does not wait: controller.end() inside a direct stream that Bun.write() pumps returns its number when the job is queued. The promise of Bun.write() is the report there.

The question for a maintainer: writer.end(). There are three answers.

  1. Close in place (this PR). end() returns a number with the file closed, also when the caller does not await it, as the docs show. It is what 1.0.30 did. The cost is a close(2) on the JS thread: the tables below, a flush on NFS or FUSE (not measured), and the close after a truncate if Bun.file(path).writer(): truncate the destination file #31737 makes writer() truncate (168 µs at 20 bytes, 38 ms at 64 MiB on ext4).
  2. end() returns a promise for a file, and the same job settles it. The type allows it (number | Promise<number>). Code that uses the number breaks, and a caller that does not await has the file open again.
  3. The close is not waited for, as on main. Then this PR fixes Bun.write(dest, stream) only.

For answer 2 or 3 the file.fd.close() line in on_close changes, and the rest stays.

Weighed, and not taken.

  • The close in close_impl for every POSIX handle (the first revision). It also fixes pipes, but it reverses the pool close for every owner. It goes to its own PR.
  • Close in place for Bun.write(dest, stream) too. The promise can wait for the pool, so nothing has to run on the JS thread there: not the ext4 flush at the close after the truncate (0.17 ms at 20 bytes, 11 ms at 16 MiB, seconds on a busy disk), and not a flush on NFS or FUSE.
  • The pool close only for a file that existed, or from 256 KiB (an unpushed revision). Two things that the caller cannot see decided whether the promise waits for the pool, and it needed a second openat to know whether the file existed.
  • A BUN_FEATURE_FLAG_DISABLE_* switch that brings the pool close of writer() back. It would bring the early report back with it.

The new tests. Each script runs with UV_THREADPOOL_SIZE=2 and parks each of the two pool threads in readFile() of a FIFO of its own. A close that waits for the pool cannot run, so each row is the same in every run. (The first revision parked both threads on one FIFO. That hangs on macOS: fifo_read in XNU clears the end-of-file state after the first reader gets it, and the second reader stays blocked. CI build 129000 timed out there.) A row has the number of descriptors that are still open where the API reports done. Release builds of main and of this PR. This PR: 20 of 20 runs the same.

row main this PR
writer.end() at return (a number), then run the file open, ETXTBSY closed, runs
await writer.end(), then run the file open, ETXTBSY closed, runs
writer.close() at return, then run the file open, ETXTBSY closed, runs
writer.end() over a file that exists, then run the file open, ETXTBSY closed, runs
Bun.file(fd).writer(): the sink's duplicate after await end() open closed
Bun.write(newPath, body), four rows: Response(stream), a ReadableStream with an async pull, a direct ReadableStream, Bun.file(src).stream() open, settles with the pool parked closed, settles once the pool is free
Bun.write(existingPath, body), three rows: Response(stream), a ReadableStream, Bun.file(src).stream() open, settles with the pool parked closed, settles once the pool is free
Bun.write(Bun.file(fd), Response(stream)): the sink's duplicate open, settles with the pool parked closed, settles once the pool is free
Bun.write(path, stream) that rejects, two rows: the stream fails, and pull() of a direct stream throws open, rejects with the pool parked closed, rejects once the pool is free
a Bun.ModuleGraph writes and ends the stream, and is disposed with the close in the queue promise fulfilled promise pending, file closed, sink released
the graph writes, the host ends the stream, the graph is disposed promise fulfilled promise pending, file closed, sink released
the host writes, the graph ends the stream, the graph is disposed promise fulfilled promise fulfilled, file closed

Two mutations of this PR on a debug build, to check the rows:

  • The job filed under the script that is running (js_thread_of_caller_no_frame()): the last two rows fail. The graph's promise is fulfilled and the host's stays pending.
  • Without the handle_reject_stream arm: the pull() row has the file open and rejects with the pool parked.

Two more tests cover the job whose completion does not run.

  • A Worker is terminated while the close job is in the queue of the parked pool. The file is closed after that and has the new contents. The pool closes it, or the pool hands the job back and the worker's thread closes it: that is a race and the test does not pin it. This test passes on main.
  • Debug and ASAN builds only: BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE holds the completion until the worker's teardown waits for it, and names the place of its ticket (FileSink.rs). On main no ticket is taken there, so this test fails on a debug build of main (not run here).

One test is new in the start() on an open writer group: start({ path }) moves a writer from a file onto a FIFO. Main leaves the file open for good (a restart onto a polled fd does not close the old handle). This PR closes it.

The rest of filesink.test.ts is as on main. Its waits for the fd leak checker stay: pipes still close on the pool.

What the change costs, per call. Release builds of main 9634520 and of this PR (9092e8e, same base). strace, perf, valgrind and bloaty are not installed in this container, and the kernel has no syscall tracepoints. A small ptrace counter that splits by thread counted the system calls. The numbers are the slope between 100 and 300 calls. futex counts change with timing under ptrace.

main this PR
writer().write().end(): close on the JS thread / on a pool thread 0 / 1.00 1.00 / 0
same: futex on the JS thread / on other threads 0.45 / 1.6 0 / 0
await Bun.write(newPath, stream): close on the JS thread / on a pool thread 0 / 1.00 0.01 / 1.00
same: openat 1.00 1.01
same: wake-up write from the pool, epoll_pwait2 0, 0 1.00, 0.95
same: futex on the JS thread / on other threads 0.63 / 2.2 1.4 / 5.4
await Bun.write(existingPath, stream): openat 1.00 1.02
same: futex on the JS thread / on other threads 0.42 / 1.9 1.6 / 5.6
allocations (read from the code, not counted) one Closer task for each close writer(): none. Stream write: one Job and one completion task
release binary, size text 107,350,736 B 107,353,296 B (+2,560)

Wall clock, idle 12-core VM, interleaved runs. p50 of 300, three runs, unless the row says otherwise.

main this PR
writer().write(20 bytes), await end() 11.2 to 12.1 µs 9.8 to 10.3 µs
await Bun.write(newPath, stream), 20 bytes 13.3 to 14.4 µs 216 to 459 µs
same, 256 KiB 127 to 164 µs 478 to 572 µs
same, 1 MiB 632 to 685 µs 835 to 850 µs
await Bun.write(existingPath, stream), 20 bytes 14.5 to 16.6 µs 199 to 532 µs
await fs.promises.writeFile(path, 20 bytes) (one run) 382 µs 370 µs
2,048 stream writes of 20 bytes, 1 in flight (two runs) 33 to 44 µs each 383 to 434 µs each
same, 16 in flight 33 to 37 µs each 62 to 69 µs each
same, 128 in flight 37 µs each 40 to 41 µs each

On main the promise of a stream write settles before the close. With this PR it settles after it, and the JS thread is free in between. A stream write that is awaited alone now takes what await fs.promises.writeFile() takes: one round trip to the pool. Writes that are in flight together share the wait.

How long close(2) takes on the calling thread. Linux 6.1, ext4 (delalloc, auto_da_alloc), idle machine, p50 and max. Measured for the first revision.

file written, then closed 20 B 64 KiB 1 MiB 16 MiB 64 MiB
new file 0.5 µs (15.9) 0.5 µs (2.9) 1.0 µs (4.3) 7.6 µs (11.5) 8.6 µs (15.8)
existing file, opened without O_TRUNC (writer()) 0.8 µs (1.3) 0.8 µs (2.8) 1.3 µs (1.6) 4.8 µs (7.0) 8.7 µs (10.0)
existing file, opened with O_TRUNC (Bun.write(path, stream): on the pool) 168 µs (343) 211 µs (454) 1.66 ms (3.27) 11.4 ms (14.2) 37.8 ms (69.4)

Closes that can be slow and now run on the JS thread. Only writer.end() and writer.close().

  • The last close of a file whose name is gone frees its pages. writer.end() after an unlink took 15 ms at 64 MiB and 34 ms at 256 MiB (median of 5, max 132 ms) on the first revision, which has the same in-place close. On main it is 0.3 ms: the pool does it.
  • On NFS, SMB or FUSE a close writes the file back. Not measured: the kernel of this machine has an NFS client but no server, and no FUSE or SMB.
  • Bun.file(path).writer(): truncate the destination file #31737 makes writer() truncate (it fixes FileSink does not truncate existing file #25968). The close after a truncate is the third row of the table above.

Not changed.

  • Windows: BaseWindowsPipeWriter::close starts uv_fs_close and reports at once. Windows: close a FileSink's file before end() and Bun.write(path, stream) report it closed #46344 (draft) is the sibling. Windows was not run here.
  • Pipes, sockets, ttys and readers: close_impl is as on main, so proc.stdin.end(), child.stdin 'close' and the end of Bun.file(path).stream() still report before the pool closes the fd. Measured in the self-review: with two crypto.pbkdf2 jobs on a pool of two threads, a cat child exits 710 to 999 ms after child.stdin.end() on 1.4.2, and 1 to 2 ms on Node 26.3.
  • A writer() that is dropped without end() goes to the pool, as on main.
  • Bun.serve file responses close through Closer. They report no close to JS.
  • A sink on a pipe that is backed up settles its parked promise before it ends the writer. FileSink: end the writer before the parked promise settles #45318 changes that order.

Related.

Self-review. Thirty-two concerns were raised on an earlier revision. Thirty are addressed:

  • The split: close_impl is not changed, and the pipe half has its own PR.
  • The job is chosen by the pending promise, not by truncate or a size. It is filed under the script that called Bun.write().
  • New rows: an fd destination, writer.close(), a native source, a direct stream, two rejecting streams, writer.end() over a file that exists, and three Bun.ModuleGraph rows.
  • The restart and drop paths, the one-line comments, and the text of this PR (no user report, no bun fmt line).

Two are not taken:

  • A test that pins which thread closes the file of a terminated worker. The pool hands a job back unrun only once the VM drains, and no hook holds a pool job until then.
  • A Bun.Terminal row. A terminal is a pty, which is polled, so it belongs to the pipe half.

Also run. Debug build of this PR:

  • test/js/bun/util/filesink.test.ts: 88 pass, 1 skip.
  • test/js/bun/io/bun-write.test.js: 107 pass.
  • test/js/bun/spawn/spawn-stdin-readable-stream.test.ts and spawn-stdin-readable-stream-edge-cases.test.ts: 70 pass.
  • test/js/web/streams/streams.test.js: 711 pass.
  • test/js/bun/module-graph/module-graph-isolation.test.ts: 409 of 414 in a whole-file run. The 4 failures are deadlines of rows that use no FileSink, and each passes in a narrower run.

cargo check --workspace passes for x86_64-pc-windows-msvc, aarch64-apple-darwin and x86_64-unknown-freebsd. macOS was not run here. CI build 129000 ran the first revision on both macOS lanes: its one failure was the test parking that this revision changes.


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts

On POSIX, PollOrFd::close_impl queued close(2) on the work pool and
called on_close in the same frame. FileSink.end() returned, and
Bun.write(path, stream) resolved, with the file still open for writing:
a script that was written this way and then run got ETXTBSY. A child
did not see the end of its stdin while the pool was busy.

close_impl now closes the fd on the calling thread before it calls
on_close, for every handle. FileReader does the same for a directory.

One close can flush a file: the close that follows a truncate on ext4.
Bun.write(path, stream) is the only sink that truncates. It now opens
with O_EXCL first. A file that exists is opened with O_TRUNC and closed
by a bun_jsc::Job on the work pool. The job's completion settles the
promise. A new file is closed in place.
@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

  • Reproduced on Linux x64 with the script in the description. Each round writes a shell script with Bun.file(path).writer() or Bun.write(path, stream), awaits it, makes the file executable and runs it.
  • ETXTBSY per 1,000 rounds: 809 to 882 on main (9634520), 872 on 1.0.31, 0 on 1.0.30, 0 with this PR.
  • test/js/bun/util/filesink.test.ts: 17 of the 18 new rows fail on main and pass with this PR.
  • PR: FileSink: close a file before end() or Bun.write() reports it closed #46088

@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:05 PM PT - Oct 21st, 2026

❌ @robobun, your commit 9092e8e has some failures in Build #130373 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 46088

That installs a local version of the PR into your bun-46088 executable, so you can run:

bun-46088 --bun

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 801fd787-7432-4f77-8a28-e4f68c8b5dc6
📥 Commits

Reviewing files that changed from the base of the PR and between 9634520 and 9092e8e.

📒 Files selected for processing: 3
  • src/io/PipeWriter.rs
  • src/runtime/webcore/FileSink.rs
  • test/js/bun/util/filesink.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked the CloseReplacedFile job's completion path for a double release: then runs report_closed and then drops ClosingSink, which repeats release_pipe and clear_keep_alive_ref — both are idempotent (the pipe cell is swapped for a default, the keep-alive deref is flag-guarded), and the single deref balances the ref_ taken in on_close, so the refcount is balanced on both the run and the dropped-unrun job paths.

Extended reasoning...

The change moves close(2) for every POSIX PollOrFd onto the calling thread in src/io/pipes.rs, threads a close_fd flag through PosixStreamingWriter, and adds a work-pool job in src/runtime/webcore/FileSink.rs for Bun.write(existingPath, stream) over a truncated file, plus test updates. It touches no security-sensitive surface (no auth, injection, or data exposure), but it changes fd-lifetime and thread-affinity semantics shared by FileSink, pipe writers and readers, and four confirmed findings (event-loop stalls on slow mounts, VM teardown waiting on the pool, a weakened double-close test, and Windows left unfixed) are posted inline, so a human decision on the design is still needed.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/io/pipes.rs Resolved
Comment thread src/runtime/webcore/FileSink.rs Resolved
Comment thread test/js/bun/util/filesink.test.ts Resolved
Comment thread src/io/pipes.rs Resolved
The first revision closed the fd on the calling thread in
PollOrFd::close_impl, for every POSIX handle. That also reverses the
pool close for pipes, sockets and readers, and no maintainer has ruled
on that. This revision leaves close_impl, FileReader and the nine other
test files as on main. It fixes the early report where it was measured:
a FileSink on a file.

A FileSink now owns the fd of a file it opened (close_fd = false on the
writer) and closes it in on_close, before it reports:
- Bun.write(dest, stream) reports through a promise. The pool closes
  the file in a bun_jsc::Job that is filed under the script that called
  Bun.write(), and the completion settles the promise.
- writer.end() and writer.close() report when they return. The file is
  closed in place.
- A sink that is never ended goes to Closer, as on main.

A direct stream whose pull() throws now also rejects the promise of
Bun.write() after the close.

The tests park each pool thread on a FIFO of its own. Two readers on
one FIFO hang on macOS: fifo_read clears the end of file after the
first reader gets it.
@robobun robobun changed the title io: close the fd before PollOrFd reports it closed FileSink: close a file before end() or Bun.write() reports it closed Oct 21, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment on lines +623 to +637
if let Some(file) = (*this).file.take() {
// Only `Bun.write(dest, stream)` reports through a promise, which can wait for the pool.
let global = (*this).js_global().filter(|global| {
(*this).pipe.get().has_done() && !global.bun_vm().is_shutting_down()
});
if let Some(global) = global {
(*this).ref_();
let opener = global.bun_vm().context_of(file.opener);
bun_jsc::Job::<CloseStreamFile>::schedule(
&global.js_thread(opener),
StreamFile(file.fd),
ClosingSink(this),
);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Every await Bun.write(path, stream) now settles only after a work-pool thread runs the close, so a busy pool delays it; on the base branch it settled at once. src/runtime/webcore/FileSink.rs:623-637 schedules Job::<CloseStreamFile> whenever pipe.has_done(), with no check that the file was replaced. A fresh file, a Bun.file(fd) dup or /dev/null needs no flush, yet still queues behind unrelated pool work. Fix: close in place when the sink created the file (or the destination is not a truncated regular file) and take the pool only for the O_TRUNC-over-existing case the description names, while still never reporting before the fd is closed.

Why this was flagged

A script calls Bun.write(newPath, response.body) or Bun.write(Bun.file(fd), stream) while the work pool is busy, e.g. two slow fs.promises.readFile calls on a container whose pool has get_thread_count() == 2 threads (src/bun_core/util.rs:2800-2847). The stream ends, FileSink::on_close runs, and src/runtime/webcore/FileSink.rs:623-637 takes the OwnedFile and schedules Job:: for any sink with pipe.has_done(), returning before report_closed. The promise is settled only in CloseStreamFile::then (FileSink.rs:138-141) after WorkPool::schedule (src/jsc/job.rs:381) hands the job to a free thread, so the await waits out the whole pool backlog. On the base branch the close was also queued on the pool but on_close settled the promise immediately. The PR description says a new file is closed in place and only a replaced file (O_EXCL -> EEXIST) takes the pool, but Options::flags() (FileSink.rs:252-260) has no O_EXCL probe, so the code does not implement that split; the test at test/js/bun/util/filesink.test.ts expects settledWithPoolParked: false even for "new file" rows.

Verification: src/runtime/webcore/FileSink.rs:931-935 sets self.file = Some(OwnedFile{..}) for every fd where !self.pollable.get(), with no O_EXCL/EEXIST distinction. FileSink.rs:622-637: in on_close, Job::<CloseStreamFile>::schedule(...); return;, so report_closed runs only in CloseStreamFile::then (line 139-142). On base, Closer::close(fd, ()) queued the close and immediately invoked on_close_fn, so the promise settled at once.

Comment on lines +91 to +99
#[cfg(unix)]
struct StreamFile(Fd);

#[cfg(unix)]
impl Drop for StreamFile {
fn drop(&mut self) {
let fd = core::mem::replace(&mut self.0, Fd::INVALID);
if fd != Fd::INVALID {
fd.close();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers get a second hand-rolled fd-owning type where bun_sys::File already closes on Drop. src/runtime/webcore/FileSink.rs:91-99 defines StreamFile(Fd) with its own Drop plus a mem::replace in run (FileSink.rs:127), duplicating what bun_sys::File (src/sys/file.rs:12-23) provides. Fix: make CloseStreamFile::OffThread a bun_sys::File (or wrap one) so the job and its release-unrun path both close through the one owning type, keeping the explicit close in run so the fd is released on the pool rather than at Drop.

Why this was flagged

src/CLAUDE.md asks runtime code to prefer bun_sys::File, which owns an fd and closes it on Drop, over hand-written ownership. The diff adds StreamFile at src/runtime/webcore/FileSink.rs:91-99 with a manual Drop that does the same thing, and run at FileSink.rs:127 repeats the mem::replace(.., Fd::INVALID).close() idiom. No user-visible failure; this is a convention and duplication nit only. On the base branch neither type exists, so nothing regresses.

Verification: nit. The diff adds struct StreamFile(Fd) at /home/claude/bun/src/runtime/webcore/FileSink.rs:91 with a hand-written impl Drop (lines 94-101), and CloseStreamFile::run at line 127 repeats the idiom. bun_sys::File (/home/claude/bun/src/sys/file.rs:11-23) is already exactly this. Nothing breaks at runtime either way; this is duplication of an existing owning type.

//
// Skipped on Windows: the writer there still reports before uv_fs_close completes.
describe.skipIf(isWindows)("a file is closed when its FileSink reports done", () => {
async function rowsWithParkedPool(files: Record<string, string>, rows: string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): Maintainers get a slower suite run than needed: the five new subprocess tests in this describe run one after another. test/CLAUDE.md asks that tests which spawn processes or write files use describe.concurrent/it.concurrent, and the existing describe.concurrent("start() on an open writer") block in the same file follows that. Fix: make the new block describe.concurrent (each test already spawns its own bun -e child with its own tempDir and UV_THREADPOOL_SIZE=2, so nothing is shared), or state in a comment why they must stay serial.

Why this was flagged

test/js/bun/util/filesink.test.ts:691 opens describe.skipIf(isWindows)(...) and the tests at :763, :826, :940, :972 and :1024 each spawn a Bun child via rowsWithParkedPool (:692-:761) or Bun.spawn (:1000). Each child parks its own thread pool and polls with 5000 ms deadlines (:731, :741), so the block can take several seconds serially. test/CLAUDE.md:22 says to make such tests concurrent with test.concurrent or describe.concurrent unless very difficult; the file's own describe.concurrent("start() on an open writer") block does so. No comment gives a reason for staying serial. On the base branch these tests do not exist; after merging the file's wall time grows by the sum of the five children rather than their maximum.

Verification: nit. Triggering condition: every run of test/js/bun/util/filesink.test.ts. The new block at test/js/bun/util/filesink.test.ts:690 is describe.skipIf(isWindows)(...), not describe.concurrent. Nothing is shared between the tests, so running them serially is a pure cost. test/CLAUDE.md:22: "When multiple tests in the same file spawn processes or write files, make them concurrent".

Comment on lines +931 to +935
let file = (!self.pollable.get()).then(|| OwnedFile {
fd,
opener: context.id(),
});
self.writer.with_mut(|w| w.close_fd = file.is_none());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: sinks on pipes, FIFOs and sockets (including subprocess.stdin) still report "closed" while close(2) is queued on the work pool, so a child waiting for EOF stays blocked until a pool thread is free. The fix only takes ownership of the fd when !self.pollable at FileSink.rs:931; every pollable handle keeps close_fd = true and goes through pipes.rs:107, which hands the fd to Closer and then calls on_close_fn at pipes.rs:115 in the same frame. Fix: make every POSIX FileSink close before it reports, e.g. let the sink own pollable fds too (or make close_impl report after the pool close completes), which the PR description claims for "every POSIX handle" but the code does not do.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

A script spawns a child with stdin: "pipe", writes, and calls subprocess.stdin.end(); the stdin FileSink is on a pollable fd. FileSink::setup at src/runtime/webcore/FileSink.rs:931-935 only creates an OwnedFile when !self.pollable.get(), so for a pipe file is None and w.close_fd stays true. PosixStreamingWriter::close (src/io/PipeWriter.rs:1134) then calls close_impl with close_fd = true; on Linux close_async is true (src/io/pipes.rs:66), so the fd is queued on the pool via Closer::close at pipes.rs:108 and on_close_fn runs immediately at pipes.rs:115-118. FileSink::on_close finds no OwnedFile and reports closed at FileSink.rs:640 with the write end still open in this process, so a child reading stdin does not see EOF until a pool thread runs the close; with the pool busy the child blocks indefinitely. This is the same as the base branch; the PR fixes the regular-file sibling only, while its description says close_impl closes before it reports for every POSIX handle. The new test suite has no FIFO or pipe row, so nothing detects it.

Verification: pre-existing. Any POSIX FileSink on a pollable fd ends while the work pool is busy. At src/runtime/webcore/FileSink.rs:931-935 a pollable fd leaves close_fd = true and file = None; pipes.rs:107-108 hands the fd to Closer::close(fd, ()) and pipes.rs:115-118 invokes on_close_fn in the same frame. The base fails identically. The new filesink.test.ts rows are all regular files.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileSink does not truncate existing file

2 participants