repl: ignore non-regular history files and only read the tail of large ones - #38217
repl: ignore non-regular history files and only read the tail of large ones#38217robobun wants to merge 2 commits into
Conversation
…e ones History::load opened ~/.bun_repl_history with a plain blocking open and read the whole file, so a FIFO at that path hung `bun repl` before it printed anything (and History::save hung it again at exit), and a huge file was read into memory in full before being trimmed to the last 1000 entries. Both now go through History::open_file, which opens with O_NONBLOCK and skips anything that is not a regular file. load reads at most the last MAX_HISTORY_FILE_BYTES of the file and collects entries from the end, which also replaces the quadratic remove(0) trimming loop.
|
Warning Review limit reached
Next review available in: 27 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 (2)
Comment |
|
Updated 4:24 PM PT - Aug 13th, 2026
❌ @robobun, your commit 86ebf2c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38217That installs a local version of the PR into your bun-38217 --bun |
|
Status: fix and tests pushed, waiting for CI. Reproduced on a debug build of main (b7a0431) with
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn open_file(path: &[u8], flags: i32) -> Option<sys::File> { | ||
| let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC; | ||
| let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?; | ||
| match file.kind() { | ||
| Ok(sys::FileKind::File) => Some(file), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 On Windows, sys::O::NONBLOCK passed to File::openat makes openat_windows_impl drop FILE_SYNCHRONOUS_IO_NONALERT from NtCreateFile, so open_file now returns an asynchronous HANDLE — but save()'s write_all calls WriteFile with lpOverlapped=NULL and load()'s pread_all passes a stack-local OVERLAPPED whose SAFETY comment requires a synchronous handle. Before this PR both paths opened synchronous handles, so this regresses Windows history persistence (and the three new non-skipIf(isWindows) tests round-trip through it). Gate the NONBLOCK | NOCTTY OR-in behind #[cfg(not(windows))] — see DirectoryRoute.rs:319-326 for the same pattern and rationale.
Extended reasoning...
What the bug is
History::open_file unconditionally ORs sys::O::NONBLOCK into the open flags for both the load and save paths:
fn open_file(path: &[u8], flags: i32) -> Option<sys::File> {
let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC;
let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?;
...
}The doc comment says "the flag has no effect on the regular file this accepts", which is true on POSIX but false in bun_sys on Windows. On Windows, sys::O::NONBLOCK is 0o4000 (src/sys/lib.rs:1180), and File::openat → openat_a → openat (Windows arm, lib.rs:3969) → openat_windows_a → openat_windows_impl (lib.rs:7137). At line 7153 that function reads let nonblock = (flags & O::NONBLOCK) != 0, and at lines 7182-7186 it does:
let blocking_flag: u32 = if !nonblock {
w::FILE_SYNCHRONOUS_IO_NONALERT
} else {
0
};So passing O::NONBLOCK drops FILE_SYNCHRONOUS_IO_NONALERT from the NtCreateFile options and yields an asynchronous (overlapped) HANDLE.
Why the downstream I/O breaks
Both callers assume a synchronous handle:
save()→file.write_all()→sys::write()(src/sys/lib.rs:3652-3680) callskernel32::WriteFilewithlpOverlapped = null_mut(). Microsoft documents that on a handle opened for asynchronous I/O the caller must supply anOVERLAPPED; passingNULLon such a handle is invalid and in practice fails (there is no I/O-manager-maintained file position). History is therefore silently never saved on Windows.load()→file.pread_all()→sys::pread()(src/sys/lib.rs:3682-3722) constructs a stack-localOVERLAPPEDwithhEvent = nulland passes it toReadFile. Its own// SAFETY:comment (line 3702) states "overlappedlives for the synchronous call (handle was not opened FILE_FLAG_OVERLAPPED)", and the error match (lines 3712-3719) handlesBROKEN_PIPE/HANDLE_EOF/OPERATION_ABORTEDbut notERROR_IO_PENDING. On an asynchronous handleReadFilemay returnFALSE+ERROR_IO_PENDING;preadthen returnsErrand unwinds while the kernel still holds a pointer to the now-dead stackOVERLAPPED— a documented-SAFETY-invariant violation and a stack use-after-free.
Step-by-step proof (Windows)
- User launches
bun replon Windows with a.bun_repl_historyfile present. History::loadcallsSelf::open_file(path, sys::O::RDONLY).open_filecomputesflags = O::RDONLY | O::NONBLOCK | 0 | O::CLOEXEC(O::NOCTTYis 0 on Windows, lib.rs:1228).File::openatreachesopenat_windows_impl;nonblockevaluatestrue;blocking_flagbecomes0;NtCreateFileis called withoutFILE_SYNCHRONOUS_IO_NONALERT→ the returned HANDLE is asynchronous.file.pread_all(&mut content, offset)callssys::pread, which passes the address of a stackOVERLAPPEDtoReadFileon that asynchronous handle. Either the read fails immediately (history not loaded) or it returnsERROR_IO_PENDING, in which casepreadreturnsErrand the kernel later writes into freed stack.- On exit,
History::savecallsSelf::open_file(path, O::WRONLY | O::CREAT | O::TRUNC)→ same asynchronous handle →WriteFile(..., NULL)fails → history not saved.
Why this is a regression introduced by the PR
Before this PR, save() used sys::open_a(path, O::WRONLY | O::CREAT | O::TRUNC, 0o600) (no NONBLOCK) and load() used sys::File::read_from — both produced synchronous handles on Windows. The PR description states only cargo check -p bun_runtime --target x86_64-pc-windows-msvc was run, not the tests. Three of the new tests — "keeps the newest entries…", "loads a file exactly at the size limit…", "loads only the tail of a file over the size limit" — are not gated by isWindows, set USERPROFILE via homeEnv(), and assert the exact contents of the saved history file, so a broken save() fails Windows CI.
The codebase already documents this hazard
src/runtime/server/DirectoryRoute.rs:319-326 has the exact pattern with a comment explaining why:
// NONBLOCK so opening a FIFO without a writer cannot block the event
// loop on POSIX. Not on Windows: there `openat` maps it to omitting
// FILE_SYNCHRONOUS_IO_NONALERT, which breaks the synchronous reads
// FileResponseStream issues.
#[cfg(not(windows))]
let flags = bun_sys::O::RDONLY | bun_sys::O::CLOEXEC | bun_sys::O::NONBLOCK;
#[cfg(windows)]
let flags = bun_sys::O::RDONLY | bun_sys::O::CLOEXEC;The unconditional-NONBLOCK sites elsewhere in the tree (Blob.rs:1481, FileReader.rs:206) all go through bun_sys::open(), which on Windows routes to sys_uv::open — a different codepath that does not hit NtCreateFile.
Fix
Gate the POSIX-only flags on cfg:
fn open_file(path: &[u8], flags: i32) -> Option<sys::File> {
// O_NONBLOCK so opening a FIFO returns immediately on POSIX. Not on
// Windows: File::openat maps it to omitting FILE_SYNCHRONOUS_IO_NONALERT,
// which breaks the synchronous pread/write below (and Windows has no
// filesystem FIFOs anyway).
#[cfg(not(windows))]
let flags = flags | sys::O::NONBLOCK | sys::O::NOCTTY | sys::O::CLOEXEC;
#[cfg(windows)]
let flags = flags | sys::O::CLOEXEC;
let file = sys::File::openat(Fd::cwd(), path, flags, 0o600).ok()?;
...
}2 participants
Problem
~/.bun_repl_historyhangsbun replbefore it prints anything:History::load(src/runtime/cli/repl.rs) opens the file with a plain blockingopen, which on a FIFO waits for a writer forever. The terminal is already in raw mode at that point, so Ctrl+C does nothing.History::saveopens the file the same way when the REPL exits, so a FIFO hangs the exit path too.File::read_from), and every line boxed, before the list is trimmed to the last 1000 entries. A 256 MiB file raises the debug build's peak RSS by 447 MiB; the report's 1.5 GB file got the process OOM-killed.while len > 1000 { entries.remove(0) }, quadratic in the number of lines in the file.Fix
History::open_fileis now the one placeloadandsaveopen the history file. It addsO_NONBLOCK, so opening a FIFO returns immediately instead of waiting for a peer, andfstats the result, ignoring anything that is not a regular file. A FIFO, device or directory at the path is treated like a missing history file on both the read and the write side.loadreads at most the lastMAX_HISTORY_FILE_BYTES(4 MiB) of the file withpread, collects entries from the end of that buffer, and stops afterMAX_HISTORY_SIZEof them. When the read did not start at offset 0, the first line of the buffer is a cut-off fragment of an older entry and is dropped. A file exactly at the limit is still loaded whole..load <file>and.save <file>are also untouched: they open a path the user typed and report errors, unlike the implicit startup read.REPL history file loadingin test/js/bun/repl/repl.test.ts. Before the fix, the FIFO test hung until the test timeout, the 256 MiB sparse file grew RSS by 447 MiB (about 5 MiB now), and the file one byte over the limit kept its cut-off first line. A file exactly at the limit and a 1200-entry file pin down that nothing else about loading changed.cargo check -p bun_runtimepasses forx86_64-pc-windows-msvcandaarch64-apple-darwin.Background
bun replkeeps the lines typed into it in~/.bun_repl_history, one entry per line, newest last.History::loadreads it after the terminal is switched to raw mode and before the banner is printed;History::saverewrites it with the last 1000 entries on exit. Both are silent best-effort operations, so any failure just means no history.open(2)on a FIFO blocks until the other end is opened too. WithO_NONBLOCK, a read-only open succeeds immediately and a write-only open with no reader fails withENXIO; on a regular file the flag does nothing, so it can be passed unconditionally.saveonly ever keeps the newest 1000 entries, reading just the tail of the file and walking it backwards drops nothing that would have survived the next save anyway (other than entries past the byte limit).