Conversation
|
Updated 2:51 AM PT - Jul 10th, 2026
✅ @autofix-ci[bot], your commit f92d9f602245f6deab28fdde5b1d21dbad6e2d6f passed in 🧪 To try this PR locally: bunx bun-pr 33533That installs a local version of the PR into your bun-33533 --bun |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
CI status: diff is green, red checks are unrelated
Jarred's review feedback is addressed in |
…live A FileSink.writer() on a FIFO or pipe only held the event loop open while a write to it was in flight. The JS-facing ref()/unref() could not change that: - JSFileSink::m_refCount starts at 1, so ref() from the default state never reached native updateRef() (only a 0->1 transition calls it). - unref() did reach native and dropped the poll's keep-alive, but the next on_write / on_auto_flush called update_ref(has_pending_data) and silently re-armed it. Record the user's ref/unref choice in keep_alive_allowed (mirroring m_refCount > 0) and route every automatic keep-alive change through one helper that ANDs the automatic decision with that flag. unref() now sticks across later writes; ref() restores the automatic management without pinning an idle sink. The docs and JSDoc claimed the process stays alive until .end(), which was never true (Bun.stdout.writer() on a pipe is pollable too, so 'bun x.js | cat' would hang). Corrected both to describe the actual in-flight-write behavior.
The previous ref() test drained the FIFO cross-process and relied on the child exiting to signal EOF, which hung on darwin aarch64 CI. Rewrite it to assert, via a bounded race against proc.exited, that ref() after unref() keeps the child alive for a pending write, then kill it. No cross-process drain, no dependence on child-exit timing.
db94ad0 to
bb72224
Compare
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Why ins't this using poll_ref, which encapsulates this behavior more cleanly than this one-off approach?
|
The short answer: The two inputs:
The bug was that (2) was never consulted: If I replace the poll-flag route with
So any shape ends up storing the "ref allowed" bit explicitly. I put it on The other option is to drop the auto-management entirely and make the default "keep alive until const w = Bun.stdout.writer();
w.write("hi");piped to Happy to flip to either of those shapes if you'd prefer — a |
|
@robobun add a UserKeepAlive? that has both has the user opted out and are we currently able to keep the event loop alive? |
Instead of a one-off Cell<bool> on FileSink, introduce UserKeepAlive in bun_io::keep_alive next to KeepAlive. It holds the two bits that FileSink needs: allowed (what JS ref()/unref() asked for) and wanted (what the automatic in-flight-write management asks for right now), and is_active() is the AND of both. FileSink stores it as poll_ref and applies is_active() to the writer's existing poll/uv-handle, so nothing about platform plumbing changes. Addresses review feedback.
|
Done in 6c1499d. pub struct UserKeepAlive { allowed: bool, wanted: bool }
impl UserKeepAlive {
pub fn set_allowed(&mut self, value: bool) -> bool; // JS ref()/unref()
pub fn set_wanted(&mut self, value: bool) -> bool; // auto management
pub fn is_active(&self) -> bool; // allowed && wanted
}
I started with the version that embeds a |
|
This still reproduces on main at b52d3e5 (bun 1.4.3) through the import fs from "node:fs"; import { execSync, spawn } from "node:child_process"; import os from "node:os"; import path from "node:path";
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pf-")); const fifo = path.join(dir, "ff"); execSync(`mkfifo ${fifo}`);
// reader: opens now, takes 8192 bytes after 300 ms, then stalls 4 s with the fd open
spawn("sh", ["-c", `exec 3<${fifo}; sleep 0.3; head -c 8192 <&3 >/dev/null; echo 'reader took 8192' >&2; sleep 4`], { stdio: ["ignore", "ignore", "inherit"] }).unref();
let wfd; for (;;) { try { wfd = fs.openSync(fifo, fs.constants.O_WRONLY | fs.constants.O_NONBLOCK); break; } catch (e) { if (e.code !== "ENXIO") throw e; } }
const w = Bun.file(wfd).writer();
const r = w.write(Buffer.alloc(256 * 1024, "u")); // pipe full, tail buffered, writable poll armed
w.unref();
r.catch(() => {});
const t0 = Date.now();
setTimeout(() => console.error(`timer done at +${Date.now() - t0}ms`), 1000);
process.on("exit", () => console.error(`exit at +${Date.now() - t0}ms`));Expected: exit at about +1000 ms, when the timer fires. Actual: #41835 depends on a sticky The branch conflicts with main in |
…allback (#44250) ### Problem - With nothing else alive, a JS stream piped into a `FileSink` on a pipe stops at the first full drain. Un-awaited `Bun.write(Bun.stdout, new Response(stream))` delivers 4,194,304 of 8,388,608 bytes, exits with code 0, and never settles. - `FileSink::on_write` (`src/runtime/webcore/FileSink.rs:366`) ran its microtask checkpoint before it resumed the pump. The run loop does not count the microtask the pump queues. `on_close` had no checkpoint. ### Fix - `completion_scope()` opens an event-loop scope while a stream is piped in. Its exit is the checkpoint. `on_write` and `on_close` open it before they enter JS. - Correct because a callback from the loop owes a checkpoint for the JS it enters. - Verified: `test/js/bun/util/filesink.test.ts`, seven new tests, 0 of 7 on main. Each scope has a test that fails without it. Also 15 more Linux suites. - Self-reviewed: 6 concerns raised, 5 addressed as proposed, 1 otherwise (Notes). ### Background - The pump (`readStreamIntoSink`) feeds the sink from a JS stream and waits for `ready()` after a short write. A microtask checkpoint runs queued promise reactions. - Considered a drain in the run loop after each poll callback: every loop turn pays. A scope around every `FileSink` poll callback charges plain writers too. ### Downsides - A sink written from script pays 7 more instructions per `on_write` and per `on_close`. `.text` grows by 256 bytes. - An un-awaited `Bun.write` whose stream fails now exits with code 1. Main exits 0 in 9 of 10 runs. - After `proc.unref()`, a stream stdin that can always produce now arrives in full. Main cuts it. <details><summary>Notes</summary> All numbers are from release builds of main ad60a9b and of this branch at 028985b on Linux x64, unless a line says debug. **Repro** (no `beforeExit` listener, no timer, no top-level await): ```js // bun repro.mjs | (sleep 0.4; wc -c) expected 8388608, main prints 4194304 const first = new Uint8Array(4 * 1024 * 1024).fill(97); const chunk = new Uint8Array(64 * 1024).fill(98); let pulls = 0; Bun.write(Bun.stdout, new Response(new ReadableStream({ pull(c) { if (++pulls === 1) return c.enqueue(first); if (pulls <= 65) return c.enqueue(chunk); c.close(); }, }))); ``` **Why main loses the step.** The run loop is `while vm.is_event_loop_alive() { vm.tick(); vm.auto_tick_active(); }` (`src/runtime/cli/run_command.rs`). `auto_tick_active` runs the poll callback. `on_write` drops the loop ref of the poll because the buffer is empty, and `src.ready()` makes the pump read the next chunk. The chunk steps, close steps and error steps of that read are queued with `queueReactionJob` (`src/jsc/bindings/webcore/streams/JSReadRequest.cpp`). The callback returns, nothing is alive, and the loop ends before the next `tick()`. One ref'd timer gives the loop another turn and hides the bug. **The fix itself**, 20 runs per shape. A run counts when every byte arrived and the reaction of the promise ran. | Shape | main | this branch | | --- | --- | --- | | chunks arrive after the pump parked, to a piped stdout | 0 of 20 | 20 of 20 | | the same into a FIFO | 0 of 20 | 20 of 20 | | stream closes while the pump is parked | 2 of 20 | 20 of 20 | | stream fails while the pump is parked | 1 of 20 | 20 of 20 | | direct stream, closed with bytes still buffered | 0 of 20 | 20 of 20 | | small chunk, sink ended against a full socket | 0 of 20 | 20 of 20 | | direct stream that awaits each `flush()` (control) | 20 of 20 | 20 of 20 | **Each scope is needed** (debug+ASAN builds, the seven new tests): | Build | Failing tests | | --- | --- | | main | 7 | | this branch | 0 | | this branch without the scope in `on_write` | 5: chunks to stdout, chunks to FIFO, `beforeExit` count, close, error | | this branch without the scope in `on_close` | 1: sink closed with no source parked | **Microtask checkpoints and `enter()` calls per callback**, from debug logs (`BUN_DEBUG_ALL=1`), constant over 3 runs. The column for main is from a debug build of 1313ca6, where `FileSink.rs` is the same file as on ad60a9b. | Callback | main | this branch | | --- | --- | --- | | drain that resumes the pump | 1 checkpoint, 1 `enter()` | 1 checkpoint, 2 `enter()` | | drain that leaves bytes in the buffer | 0, 0 | 0, 0 | | close, a source had parked | 1, 1 | 1, 3 | | close, no source parked | 1, 1 | 2, 2 | | sink written from script, drain or close | 1, 1 | 1, 1 | On main the one checkpoint of a drain runs before the pump is resumed. On this branch it runs after. **`beforeExit` emissions** for the 8 MiB pump with a listener installed, 10 runs: main 1 to 21, this branch 1 in 10 of 10, Node v26.3.0 1 in 10 of 10. **Cost for a sink written from script.** One `on_write(65536, Drained)` call: 46 instructions on main, 53 on this branch, no `call` instruction in either. One `on_close` call: 28 and 35, 2 `call` instructions in both. Counted with gdb `nexti` on release builds with symbols, identical over 8 calls. Binary size with `size`: `.text` 80,667,862 to 80,668,118, `data` and `bss` equal, the file keeps its size of 80,836,168 bytes. `on_write` grows from 1,302 to 1,530 bytes and `on_close` from 1,860 to 1,983. **Not measured:** `write(2)` calls that return `EAGAIN` per drain. `strace`, `perf` and `valgrind` are not installed where I measured, and under gdb `catch syscall` the writer is so slow that the pipe never fills. **A stream that fails with no handler**, 10 runs. Main: exit code 0 and nothing on stderr in 9 runs, `error: boom` and exit code 1 in 1 run. This branch: `error: boom` and exit code 1 in 10 runs. The same stream awaited at top level gives `error: boom` and exit code 1 on both builds. **`proc.unref()` and a stream stdin**, 10 runs per cell. The child starts to read only after the pump has taken the first chunk. | Stream | main | this branch | | --- | --- | --- | | can always produce | 4,194,304 of 8,388,608 | 8,388,608 | | all data queued and closed before the spawn | 219,264 in 9 of 10 runs | 219,264 in 9 of 10 runs | | produces one chunk, then waits on a promise that never settles | 4,194,304, parent exits | 4,194,304, parent exits | | can always produce, no `unref()` | 8,388,608 | 8,388,608 | This change does not touch `Writable::unref`, which clears the loop ref of the stdin writer also while it holds accepted bytes (second row). What `unref()` must mean for a writer that owes bytes is a decision for a maintainer and relates to #33533. No test here uses `unref()`. **Sites that this change leaves alone, and why.** | Site | Reason | | --- | --- | | `on_ready` | The POSIX writer never calls it. It is the `on_writable` slot on Windows, where the unfixed build has no failing case. | | `EndOfFile` arm of `on_write` | No test reaches it with a piped stream on POSIX. | | Windows settle for a borrowed fd, in `end_writer` and in the abort handler | No failing case on Windows. #42819 rewrites that code. | | `on_auto_flush` | It runs inside the deferred task queue, where `EventLoop::exit()` does not drain. The task that `run_pending_later` queues just before the resume keeps the loop alive for the next `tick()`. | | Other sinks and other poll owners | Not examined here. | **Other PRs that touch the same lines.** - #43761 added a ref guard at the top of `on_close` and merged while this PR was open. This branch contains it: guard first, scope second. Its stdin test passes and the tests here pass. - #42032 adds a checkpoint after the `beforeExit` dispatch. On unfixed code with that hunk, six of the seven tests pass, because the loop passes through that dispatch after each drain. The seventh fails: `beforeExit` fires more than once for one stream (8 times in my run). With this change and that hunk together all seven pass. **Self-review.** Concerns raised and what I did: 1. A ref guard in `on_close` repeated #43761 without its tests. The proposal was to stack this PR on #43761. I removed the guard and did not stack: the scope does not read the sink when it ends, so it does not need the guard. #43761 has merged since. 2. A `cfg(windows)` scope in `end_writer` had no failing test and sits in code that #42819 rewrites. Removed. 3. A scope and a guard in `on_ready`, and an `EndOfFile` clause in `on_write`, had no failing test. Removed. 4. A gate on `VM::is_entered()` had no failing test. I built the change without the gate and ran a script that closes the sink inside the `Bun.write` call with a microtask already queued. The order of the microtask did not change, because a script frame already runs inside an entered scope. Removed. 5. The predicate also matched a `stdin: "pipe"` sink that script never read, and shell pipes. It now tests only for a piped stream. 6. The tests would pass on unfixed code once #42032 lands. Added the test that counts `beforeExit`. **Platforms.** On Windows Server 2019 x64 a debug build of main passes every portable shape, also with a first chunk of 64 MiB and of 256 MiB where the child waits 0.6 to 2.3 s for the reader. So on Windows the five portable tests guard against a regression only. The first revision of the FIFO test read its end with `Bun.file(fd).bytes()`. On macOS that read never finished, 8 of 8 attempts, although the child had exited. The test now reads with `read(2)` in a poll with a deadline, and passes on macOS. **Suites** on the debug build of this branch, 60 s per test, all pass: `filesink` (77 after the merge with main), `bun-write` (86), `spawn-stdin-readable-stream` (4 files, 55), `spawn-streaming-stdin`, `spawn-stdin-destroy`, `spawn-stdin-pipe-fd-leak`, `readablestream-helpers`, `streams` (624), `compression`, `sync-pull-fast-path`, `native-source-onclose-leak`, `direct-readable-stream`, `bunshell` (436). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · 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 <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
What
FileSink.ref()/unref()on a FIFO or pipe sink were effectively no-ops, contradicting the documented keep-alive contract.Repro
Cause
A pipe
FileSinkonly held the event loop open while a write to it was in flight, and the JS-facingref()/unref()could not override that:JSFileSink::m_refCountstarts at 1 (generate-jssink.ts), soref()from the default state never reaches nativeupdateRef()(only a 0→1 transition calls it).unref()did reach native and dropped the poll's keep-alive, but the nexton_write/on_auto_flushcalledupdate_ref(has_pending_data)and silently re-armed it.So
unref()was undone by the very next write, andref()on the default sink did nothing.Fix
Record the user's ref/unref choice in a new
keep_alive_allowedcell (mirroringm_refCount > 0) and route every automatic keep-alive change through one helper,set_keep_alive, that ANDs the automatic decision with that flag:unref()now sticks across later writes.ref()restores the automatic management (in-flight-write keep-alive) without pinning an idle sink.The docs and JSDoc claimed the process "stays alive until
.end()", which was never true:Bun.stdout.writer()on a pipe is pollable too, sobun x.js | catwould hang forever. Correcteddocs/runtime/file-io.mdxand thebun-typesJSDoc to describe the actual in-flight-write behavior.Verification
New
ref/unref keep-alivetests intest/js/bun/util/filesink.test.ts:unref()before a pending write is not re-armed by itunref()after a write already went pending lets the process exitref()puts the automatic keep-alive back (the pending write drains once the reader consumes it)ref()/unref()on a regular file are no-ops and don't break writesno test proof · iteration 6 · 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