Repository navigation
Remove libuv on Windows - #33160
Remove libuv on Windows#33160dylan-conway wants to merge 41 commits into
Conversation
Replace the libuv backend with an in-tree IOCP/AFD event-loop engine implementing the us_loop_* contract, a native fd table (bit-tagged indices, CRT lpReserved2 interop), NT-path file I/O with a worker-pool async layer, named-pipe/ConPTY/tty/signal/fs-event handles, and work-pool DNS. The uv symbol surface for N-API addons remains exported (crash stubs + real polyfills); process.versions.uv reports the emulated ABI level. POSIX behavior unchanged.
… Winsock init - make_table_owned now closes the handle on every failure path (classify errors previously leaked it); callers no longer guess opposite cleanup contracts (one leaked, one double-closed). fd adoption errors surface the real Win32 code instead of unconditional EMFILE. - Keep-alive refs taken cross-thread are released through the same accounting (concurrent_origin), so worker teardown reconciliation is a true net; reconcile moved onto EventLoop, Windows-gated, and no longer counts never-applied pending deltas. - Winsock now initializes at the first actual consumer (socket creation, AFD peer setup, getaddrinfo, send/recv) instead of at loop creation, so network-free startups skip the service-provider load. - Spawn stdio error paths close the 2>&1 dup pair's server end. - Pipe adoption failures propagate (ENOTSOCK) instead of silently falling back to blocking file reads on the loop thread. - dup() rejects the INVALID_HANDLE_VALUE sentinel (it aliases the current-process pseudo-handle). - Delete dead code: from_libuv error plumbing, deferred-free machinery in the Windows event loop, duplicate close_engine_pipe copies, unused Closer parameter, stale libuv comments; dedupe SHARE_ALL and uv_hrtime. - Assert stdio repair runs before the fd table snapshot; document quirks provenance without external references.
File writes complete inline on Windows, so routing them through the async writer meant the completion callback fired while the interpreter was still on the stack — the deferred-yield inbox existed mostly to un-nest that. Give Windows the same shape POSIX already has: regular files bypass the writer machinery and return their completion as a value (do_file_write); pipes and console handles keep the async path (pipes are genuinely asynchronous, console writes need the UTF-16 conversion). The pollable flag is now computed from the fd's mode on Windows instead of hardcoded, and redirect writers use the mode the open reported. The inbox remains only for callback-shaped re-entrancy (inline process exit, capture drains).
The idle tick entries (us_loop_pump, us_loop_run_bun_tick) returned early when nothing held a ref, so a completion packet posted for an unref'd handle was never dequeued — an unref'd child's exit was only observed if something else happened to keep the loop referenced. awaiting exited() on an unref'd subprocess hung forever while the event loop spun. The POSIX backend gates this path on registered event sources (num_polls), not refs. Match that: when nothing is ref'd, still collect ready completions with a zero timeout — never idle-wait on unref'd work, same as uv_run(UV_RUN_NOWAIT). Adds a regression test driving an unref'd child's exit through the public tick entry.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| mkdirSync(FIX, { recursive: true }); | ||
| writeFileSync(join(HERE, ".gitignore"), "fixtures/\nout-build/\n"); |
There was a problem hiding this comment.
🟡 This unconditionally overwrites bench/libuv-removal/.gitignore with two lines (fixtures/ and out-build/), but the PR also checks in a .gitignore here with 7 entries (nullchild.exe, *.pdb, *.ilk, .fixtures/, fixtures/, results/, bench/). The composite-* benchmarks auto-invoke this generator on first run, so running any of them clobbers the tracked file and dirties the working tree. The checked-in .gitignore already covers fixtures/, so this line can just be deleted.
Extended reasoning...
What the bug is
bench/libuv-removal/composite-gen-fixtures.mjs line 97 does:
writeFileSync(join(HERE, '.gitignore'), 'fixtures/\nout-build/\n');where HERE resolves to bench/libuv-removal/. But this same PR checks in bench/libuv-removal/.gitignore as a tracked file with seven entries: nullchild.exe, *.pdb, *.ilk, .fixtures/, fixtures/, results/, and bench/. Running the fixture generator unconditionally replaces this tracked file with a two-line version.
The specific code path that triggers it
The three composite benchmarks — composite-startup-resolve.mjs, composite-install-extract-link.mjs, and composite-test-parallel.mjs — each check whether their fixtures exist and, if not, spawn composite-gen-fixtures.mjs:
if (!existsSync(join(CONSUMER, 'package.json'))) {
const r = spawnSync(process.execPath, [join(HERE, 'composite-gen-fixtures.mjs')], { stdio: 'inherit', windowsHide: true });
if (r.status !== 0) process.exit(1);
}So the very first time anyone runs a composite benchmark on a fresh checkout, the generator runs and overwrites the tracked .gitignore.
Why existing code doesn't prevent it
The generator's other outputs (genModules, genInstall, genTests) each have an if (existsSync(...) && !force) return idempotency guard, but the .gitignore write is a bare top-level statement with no guard. It runs on every invocation regardless of --force.
Impact
Running bun bench/libuv-removal/composite-startup-resolve.mjs (or any composite-* script) on a clean checkout leaves bench/libuv-removal/.gitignore modified in git status. The lost entries mean nullchild.exe, *.pdb, *.ilk, .fixtures/, results/, and bench/ are no longer ignored and can be accidentally staged. This is developer-tooling annoyance, not a runtime correctness issue.
Step-by-step proof
- Fresh checkout:
bench/libuv-removal/.gitignorecontains 7 lines (as checked in by this PR). - Run
bun bench/libuv-removal/composite-test-parallel.mjs. existsSync(join(TESTS, 't00.test.ts'))is false → spawnscomposite-gen-fixtures.mjs.- Generator executes line 97:
writeFileSync(join(HERE, '.gitignore'), 'fixtures/\nout-build/\n'). git statusnow showsmodified: bench/libuv-removal/.gitignore;git diffshows 5 lines removed andout-build/added.
Fix
Delete line 97. The checked-in .gitignore already contains fixtures/ (and there is no out-build/ directory referenced anywhere else in the bench scripts), so the write is entirely redundant.
| * uv loop wakes for lsquic's time-driven state. POSIX folds the deadline | ||
| * into the epoll_pwait2 timeout via getTimeout() instead, so this stays | ||
| * NULL there. */ | ||
| struct us_timer_t *quic_timer; | ||
| struct us_socket_group_t *iterator; |
There was a problem hiding this comment.
🟡 The quic_timer field is deleted but its 4-line 'libuv only: a fallthrough us_timer_t…' doc comment is left behind, now attached to the unrelated iterator field. A few other libuv references were similarly orphaned by this PR: src/event_loop/AnyEventLoop.rs:612-622 (the native_loop doc still says 'uv_loop_t* on Windows' / '.uv_loop projection', and the renamed platform_loop() doc still says 'kept for callers that spell uv_loop'), and CLAUDE.md's Vendored Dependencies list still includes 'libuv (Windows event loop)' even though scripts/build/deps/libuv.ts is deleted.
Extended reasoning...
This PR removes the entire libuv backend but leaves several stale comments/docs that now describe deleted mechanisms or misdescribe types.
1. packages/bun-usockets/src/internal/loop_data.h:55-59 — The diff removes struct us_timer_t *quic_timer; from us_internal_loop_data_t but leaves its 4-line doc comment in place:
/* libuv only: a fallthrough us_timer_t armed to quic_next_tick_us so the
* uv loop wakes for lsquic's time-driven state. POSIX folds the deadline
* into the epoll_pwait2 timeout via getTimeout() instead, so this stays
* NULL there. */
struct us_socket_group_t *iterator;The comment now sits directly above the unrelated iterator field, describing a libuv-only mechanism that this PR deletes entirely (the whole LIBUS_USE_LIBUV backend is removed, and the PR updates the corresponding comment in quic.c to say 'getTimeout() … folds this into the poll timeout on every backend'). The 'this stays NULL there' phrasing is nonsensical for the follower field.
2. src/event_loop/AnyEventLoop.rs:612-622 — Two stale doc comments:
- Lines 621-622: the diff renames
uv_loop()→platform_loop()but leaves the doc 'Windows convenience alias for [native_loop] (kept for existingcfg(windows)callers that spelluv_loop)'. After the rename no caller spells ituv_loop— the parenthetical describes a name the method no longer has. - Lines 612-615 (unchanged by the diff): the
native_loop()doc still says 'Platform-native loop pointer (us_loop_t*on POSIX,uv_loop_t*on Windows). See [bun_io::uws_to_native] — collapses the per-site#[cfg(windows)].uv_loopprojection…'. After this PR,bun_io::Loopon Windows is the uws wrapper (seeMiniEventLoop.rs:pub type PlatformEventLoop = UwsLoop;with the#[cfg(windows)]arm removed, andSpawnSyncEventLoop.rs: 'uws.Loop on every platform — the wrapper is the platform loop'). There is nouv_loop_t*and no.uv_loopfield to project; the type description is now factually wrong.
3. CLAUDE.md line 148 — The Vendored Dependencies section still lists 'libuv (Windows event loop)' with the note 'Build configuration for these is in scripts/build/deps/*.ts', but this PR deletes scripts/build/deps/libuv.ts entirely and removes libuv from allDeps in scripts/build/deps/index.ts. CLAUDE.md is not in the PR's changed-files list.
Impact: No runtime effect — these are all stale comments/docs. But they now actively mislead: the loop_data.h comment appears to describe iterator; the AnyEventLoop.rs docs misdescribe the return type of native_loop() on Windows; CLAUDE.md points readers at a nonexistent build script. Per CLAUDE.md's own guidance ('Delete dead code in the same PR that makes it dead' / 'Comments carry only durable non-obvious content' / docs invalidated by a change should be updated in the same PR), these should be cleaned up here.
Fix: Delete the 4-line comment in loop_data.h; rewrite/delete the two doc comments in AnyEventLoop.rs (native_loop can just say 'Platform-native loop pointer (the uws wrapper on every platform)', and platform_loop's parenthetical can be dropped or the whole alias removed if nothing needs it); drop 'libuv (Windows event loop), ' from CLAUDE.md's vendored-deps list.
There was a problem hiding this comment.
The loop_data.h concern is resolved — d04833b restored quic_timer under #ifdef LIBUS_USE_BUN_IOCP with a fresh, accurate 'Windows engine only: fallthrough timer armed inside us_quic_loop_process…' comment, so nothing is orphaned there anymore.
The other two stale-doc locations from this comment are still unchanged:
src/event_loop/AnyEventLoop.rs:612-615—native_loop()doc still says 'us_loop_t*on POSIX,uv_loop_t*on Windows' and 'collapses the per-site#[cfg(windows)].uv_loopprojection'. There is nouv_loop_t*or.uv_loopfield anymore; the return is the uws wrapper on every platform.src/event_loop/AnyEventLoop.rs:621-622—platform_loop()doc still says '(kept for existingcfg(windows)callers that spelluv_loop)'. After the rename no caller spells ituv_loop.CLAUDE.md:148— Vendored Dependencies still lists 'libuv (Windows event loop)', butscripts/build/deps/libuv.tsis deleted andlibuvis removed fromallDeps.
There was a problem hiding this comment.
9d7baee ('Clean up stale libuv references in doc comments') cleaned three other stale-libuv comment sites (node_process.rs, path_watcher.rs, repository.rs) but missed all three from this thread — verified still present at HEAD:
src/event_loop/AnyEventLoop.rs:612-615—native_loop()doc still reads "us_loop_t*on POSIX,uv_loop_t*on Windows" and "collapses the per-site#[cfg(windows)].uv_loopprojection". Should be e.g. "Platform-native loop pointer (the uws wrapper on every platform)".src/event_loop/AnyEventLoop.rs:621-622—platform_loop()doc still says "(kept for existingcfg(windows)callers that spelluv_loop)". Nothing spellsuv_loopafter the rename; drop the parenthetical.CLAUDE.md:148— Vendored Dependencies still lists "libuv (Windows event loop)";scripts/build/deps/libuv.tsis deleted andlibuvis gone fromallDeps. Drop "libuv (Windows event loop), " from the list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR removes libuv on Windows across Bun's runtime, replacing it with new ChangesWindows IOCP Runtime Migration
Windows libuv-removal Benchmark Suite
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/cares_sys/c_ares.rs (1)
2166-2191: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEncode the
sockaddr_storageprecondition in the API.The safety comment requires storage sized/aligned for every sockaddr family, but
pub fn get_sockaddr(..., sa: &mut sockaddr)lets safe callers pass an actualsockaddr; the IPv6 cast then writes past that object. Accept&mut sockaddr_storageor make the functionunsafewith a documented caller contract.As per coding guidelines, unsafe native code must validate memory/lifetime invariants at the boundary.
Preferred shape
-pub fn get_sockaddr(addr: &[u8], port: u16, sa: &mut sockaddr) -> c_int { +pub fn get_sockaddr(addr: &[u8], port: u16, sa: &mut sockaddr_storage) -> c_int { ... - let in_: &mut sockaddr_in = unsafe { - &mut *std::ptr::from_mut::<sockaddr>(sa) + let in_: &mut sockaddr_in = unsafe { + &mut *std::ptr::from_mut::<sockaddr_storage>(sa) .cast::<c_void>() .cast::<sockaddr_in>() }; ... - let in6: &mut sockaddr_in6 = unsafe { - &mut *std::ptr::from_mut::<sockaddr>(sa) + let in6: &mut sockaddr_in6 = unsafe { + &mut *std::ptr::from_mut::<sockaddr_storage>(sa) .cast::<c_void>() .cast::<sockaddr_in6>() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cares_sys/c_ares.rs` around lines 2166 - 2191, The get_sockaddr API currently hides a storage-size/alignment precondition behind a safe &mut sockaddr parameter, but the sockaddr_in/sockaddr_in6 casts in c_ares.rs assume sockaddr_storage-sized backing memory. Update get_sockaddr to take &mut sockaddr_storage (preferred) or mark it unsafe with a documented caller contract, and adjust the sockaddr_in/sockaddr_in6 writes so the boundary enforces the invariant before any family-specific cast occurs.Source: Coding guidelines
src/io/lib.rs (1)
1804-1815: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not dereference the loop pointer on the Windows no-op path.
Line 1805 materializes
&mut Loopbefore the#[cfg]split, so Windowsunregister()still dereferences the raw pointer and creates a mutable borrow even though Line 1812 ignores it. Moveuws_loop_mut()into the POSIX branch.Proposed fix
pub fn unregister(self, loop_: *mut bun_uws_sys::Loop, force: bool) -> sys::Result<()> { - let loop_ = Self::uws_loop_mut(loop_); #[cfg(not(windows))] { + let loop_ = Self::uws_loop_mut(loop_); self.inner().unregister(loop_, force) } #[cfg(windows)] { let _ = (force, loop_);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/lib.rs` around lines 1804 - 1815, Move the raw loop pointer conversion out of `unregister()`’s shared prelude and into the `#[cfg(not(windows))]` branch so the Windows no-op path never calls `Self::uws_loop_mut` or creates a mutable borrow. Keep the POSIX path using `self.inner().unregister(loop_, force)` with the converted `Loop`, and leave the Windows branch as a true no-op that only ignores `force` and `loop_`.src/io/keep_alive.rs (1)
119-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsume
concurrent_originon deferred unrefs too.
unref_on_next_tick*()flipsstatustoInactivebut never clears or reconcilesconcurrent_origin. A ref taken viaref_concurrently()then skips the new concurrent-release bookkeeping entirely, and the stale flag survives into later re-refs/unrefs. That breaks the “true net” invariant called out above and can leave keepalive accounting permanently off by one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/keep_alive.rs` around lines 119 - 138, The deferred unref paths in keep_alive::KeepAliveState are leaving concurrent_origin stale, so a ref later can skip the concurrent-release bookkeeping and break the net keepalive count. Update unref_on_next_tick and unref_on_next_tick_concurrently to consume or clear concurrent_origin when transitioning to Inactive, and make sure any concurrent-release state is reconciled before incrementing the pending unref counter. Keep the logic aligned with ref_concurrently and the existing status handling so later re-refs/unrefs start from a clean state.src/install/PackageInstall.rs (1)
2084-2092: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDirectory symlinks can now be misclassified as dangling on Windows.
Line 2086 switches this probe to
sys::open(path, 0, 0), butinstall_from_link()usesis_dangling_symlink()for linked package directories, and the same file already documents that bare opens on Windows take the file-open path while directory targets needO::DIRECTORY. As written, valid directory links/junctions can fail this probe and abort withDanglingSymlink.Suggested fix
#[cfg(windows)] { - match sys::open(path, 0, 0) { + match sys::open(path, sys::O::RDONLY | sys::O::DIRECTORY, 0) + .or_else(|_| sys::open(path, 0, 0)) + { Err(_) => return true, Ok(fd) => { fd.close(); return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/PackageInstall.rs` around lines 2084 - 2092, The Windows symlink probe in is_dangling_symlink() is using a file open path, which can misclassify valid directory links/junctions as dangling. Update the Windows branch to detect directory targets the same way install_from_link() expects, using the directory-open semantics already documented in this module rather than a bare sys::open(path, 0, 0) probe. Keep the existing control flow in is_dangling_symlink() but make the open call match the target type so directory symlinks are not rejected as DanglingSymlink.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/libuv-removal/asyncfs-singlethread-knee.mjs`:
- Around line 152-160: The baseline run in the asyncfs benchmark still inherits
UV_THREADPOOL_SIZE from the parent process, so the “default env” case may not be
a true baseline. Update the spawnSync call in the benchmark loop so the default
branch explicitly clears UV_THREADPOOL_SIZE while keeping the comparison branch
set to 24, using the existing SELF, spawnSync, and env setup to locate the
change.
In `@bench/libuv-removal/asyncfs-workers-global-pool.mjs`:
- Around line 176-177: The shutdown flow in the worker loop should create the
`exit` waiters before sending `bye`, because a worker can exit too quickly and
miss the later listener registration. In `asyncfs-workers-global-pool.mjs`,
update the final `workers` teardown so the `Promise.all(...)` of `wk.on("exit",
...)` promises is built first, then call `postMessage({ cmd: "bye" })` on each
worker, preserving the same `workers` and `wk` logic.
In `@bench/libuv-removal/compare-fs-after.ts`:
- Around line 50-56: The benchmark child handling in compare-fs-after.ts should
fail the wrapper immediately instead of returning an "EXIT ..." string when
proc.exited is non-zero. Update the logic around the Promise.all result in the
compare function so a non-zero code throws or rejects with the captured
stdout/stderr, ensuring pickSummary() cannot hide a broken BEFORE/AFTER binary.
Use the proc.exited, stdout/stderr reads, and the surrounding
compare/pickSummary flow to locate the change.
In `@bench/libuv-removal/composite-test-parallel.mjs`:
- Around line 36-39: The benchmark setup in composite-test-parallel.mjs is
defaulting BUN to plain "bun", which can measure the wrong runtime instead of
the checked-out build. Update the benchmark entry points that invoke bun test to
require an explicit BENCH_BUN (or another explicit build-under-test input) and
fail fast if it is missing, rather than silently falling back. Make the change
in the configuration near BUN/FAST/RUNS/WARMUP and in the scenario calls that
use BUN so the benchmark always targets the intended binary.
In `@bench/libuv-removal/fswatch-burst.mjs`:
- Around line 157-160: The burst-child launch in fswatch-burst.mjs only waits
for the child process to close, so failures can be mistaken for watcher loss.
Update the spawn/close flow around the child Promise to verify the child exited
successfully before continuing, and surface a failure when the exit code or
signal is non-zero/non-null. Use the existing child variable and the burst-child
spawn block to locate the fix.
- Around line 105-123: The single-touch-latency sampling in fswatch-burst.mjs
leaves the watcher open when Promise.race times out and later assumes samples
always has data. Update the Promise setup around watch() so the watcher is
closed on timeout as well as on matching events, and guard the results.push
aggregation against an empty samples array by skipping median/P90 formatting or
emitting a clear fallback when no samples were collected.
In `@bench/libuv-removal/nullchild.c`:
- Around line 11-18: The argument parsing in nullchild’s main loop currently
uses atoll(), which silently accepts malformed, negative, or out-of-range input
as 0 and can make nullchild.exe foo exit successfully without writing bytes.
Update the argc > 1 handling to parse argv[1] with strtoll/strtoull, validate
errno and the end pointer, and reject any invalid, partial, or negative byte
count before entering the fwrite loop.
In `@bench/libuv-removal/pipes-ipc-throughput.mjs`:
- Around line 1-38: The top-of-file commentary in pipes-ipc-throughput.mjs is
too long and belongs in documentation rather than the benchmark source. Trim the
file header to a short, durable summary and move the extended rationale,
mechanism notes, and run instructions into bench/libuv-removal/README.md,
keeping only minimal context near the benchmark entry point.
- Around line 103-109: The child exit handling in pipes-ipc-throughput.mjs does
not persist failure state when no recv() waiter exists, so a later recv() can
hang forever after the child has already exited. Update the child.on("exit")
path to record the failure in the shared childErr state before or while calling
failAll, and make recv() continue to reject immediately from that stored error
on subsequent calls. Use the existing childErr, failAll, and recv symbols to
keep the failure sticky across calls.
In `@bench/libuv-removal/pipes-raw-readfile-control.mjs`:
- Line 22: The run hint is using the generic bun command, which can resolve to
the system Bun instead of the PR build under review. Update the bench script
hint in pipes-raw-readfile-control.mjs to use the bd wrapper form for running
the benchmark, matching the project guideline and ensuring the reviewed build is
used.
In `@bench/libuv-removal/pipes-stdin-throughput.mjs`:
- Around line 91-123: Update lineWaiter so the Node subprocess path waits on
child “close” instead of “exit” before treating the stream as finished, using
the same failAll wiring in lineWaiter. Make sure next() only resolves DONE after
stdout is fully drained and the process has closed, and reject if the child
closes with a non-zero code or unexpected signal even when the DONE marker was
already buffered.
In `@bench/libuv-removal/spawn-sync-overhead.mjs`:
- Around line 122-123: The NUL handle acquisition in `spawn-sync-overhead.mjs`
is only checking for a falsy result, which misses the `INVALID_HANDLE_VALUE`
failure case from `K32.CreateFileW`. Update the `nulHandle` validation so it
rejects both `null`/`0` and `INVALID_HANDLE_VALUE` (`-1`) before continuing, and
keep the existing error reporting tied to the `CreateFileW` call.
In `@bench/libuv-removal/stdout-throughput.mjs`:
- Around line 1-48: The top-of-file narrative in stdout-throughput.mjs is too
long for a benchmark source comment and should be moved into
bench/libuv-removal/README.md. Keep only a brief header in the benchmark file,
and relocate the detailed rationale, run instructions, and measurement notes out
of stdout-throughput.mjs while preserving the benchmark-specific context.
- Around line 152-165: The `--target=tty` loop in `stdout-throughput.mjs` is not
handling all advertised patterns, so `sync-write-buf` and `sync-writesync`
currently fall into the callback-based fallback instead of being measured as
synchronous writes. Update the pattern dispatch inside the `CONFIGS` iteration
to explicitly handle each row by name, using the same
`process.stdout.write`/buffer approach intended for the TTY benchmark, so the
results stay comparable with the child-mode measurements.
In `@bench/libuv-removal/sync-fs-open-close-readfile.mjs`:
- Around line 131-141: The 1KB benchmark in the readSync loop is accidentally
measuring EOF after the first iteration because fs.readSync is called with
position null; update the readSync call in the benchmark block to use a fixed
start position so each iteration measures a positioned 1KB read, and then thread
that measured readSync result into the summary calculation instead of
subtracting the hardcoded 1800. Use the surrounding run("readSync 1KB on open fd
(pos=null)", ...) benchmark and the final console.log summary as the reference
points when making the change.
In `@bench/libuv-removal/sync-fs-readsync-positioned.mjs`:
- Around line 80-107: The sequential benchmark controls in
sync-fs-readsync-positioned.mjs are skewed because the shared fd is left near
EOF by the positioned rewind reads, so the pos=null cases in run("readSync 4KB
pos=null (1 ReadFile)", run("readSync 64B pos=null", and related fs.readSync
calls end up measuring repeated zero-byte EOF reads. Fix this by ensuring each
scenario starts from a clean offset or uses a separate descriptor/explicit seek
reset before the control loops, and keep the rewind logic isolated so seq4k and
seq64 do not contaminate the shared fd state.
In `@bench/libuv-removal/sync-fs-stat-resolver.mjs`:
- Around line 1-41: The long benchmark preamble in sync-fs-stat-resolver.mjs is
documentation content, not durable code context. Trim the top-of-file comment to
a short 3-line header and move the detailed rationale, measurements, and run
notes into bench/libuv-removal/README.md. Keep the file comment minimal while
preserving any uniquely useful identifiers like fs.existsSync, fs.accessSync,
fs.statSync, and the benchmark name for discoverability.
In `@bench/libuv-removal/tcp-loopback-eventing.mjs`:
- Around line 129-138: The benchmark in the loopback eventing script is
swallowing connection failures by chaining catch(() => {}) onto the Promise.all
calls, which lets exhausted retries look like successful connects. Remove that
suppression from the measured path in the warmup and timed loop around
Promise.all(Array.from({ length: parallel }, loop)), and let real transport
errors surface so the benchmark fails instead of reporting inflated connects/s
from total / secs.
In `@packages/bun-usockets/src/internal/internal.h`:
- Around line 380-384: The ABI checks in us_internal_callback_t currently use
_Static_assert directly in internal.h, but this header is included from C++
translation units; replace those assertions with the existing
BUN_IOCP_STATIC_ASSERT wrapper from bun_iocp.h so the size and offsetof checks
for us_internal_callback_t and its loop field compile correctly in both C and
C++.
In `@src/fdtable/table.rs`:
- Around line 299-317: `import_inherited_blob()` currently accepts each HANDLE
independently and can mint multiple fd slots for the same live handle. Add
duplicate detection in the import loop (around the `classify_handle`/`mint_at`
path) so repeated HANDLE values are rejected or skipped before calling
`mint_at`, using a per-import set keyed by the raw HANDLE value to ensure only
one slot can adopt a given handle.
- Around line 293-297: The inherited-fd parser in the table blob handling is
incorrectly capped at 4096, which can silently drop valid descriptors in the
4096-8191 range. Update the startup blob parsing logic in the code around the
MAX_INHERITED/count calculation to allow the full contract range supported by
the replaced CRT table (up to 8192), and ensure the clamp in this parser still
respects the blob size-derived limit.
In `@src/install/PackageManager/security_scanner.rs`:
- Around line 1180-1190: The pipe ownership is being reclaimed twice because
`pipe_ptr` stays armed in the outer scope while the writer-side closure also
rebuilds a Box from the same raw pointer. Update the ownership flow around the
`pipe` guard and the writer transfer so the source handle is neutralized once
ownership moves, and make deallocation depend on an explicit ownership state.
Ensure only one path can call `spawn::close_engine_pipe`/`bun_core::heap::take`
for `pipe_ptr`, including the transfer logic near the writer closure and the
error cleanup path after `finish_spawn`.
- Around line 1172-1177: The IOCP adoption failure path in `security_scanner.rs`
returns from the `pipe_box` match after `ipc_output_fds` has already been
created, but it does not release those pipe handles. Update the
`PipeHandle::open`/adoption error branch in this section to explicitly close the
`ipc_output_fds` resources before returning, matching the cleanup behavior used
in the `create_pair` failure path. Use the existing `ipc_output_fds` and
`pipe_box` flow as the fix location so each acquisition is paired with its
release at the same site.
In `@src/io/PipeReader.rs`:
- Around line 1564-1579: The `PipeReader` read loop currently panics on
user-reachable large offsets by calling `i64::try_from(offset).expect("int
cast")`, and it can also overflow when advancing `r._offset += n`. Replace both
spots with checked arithmetic/conversion in the `use_pread` branch and when
updating `_offset`, and propagate a `sys::Error` back through the existing
`result`/`on_read` flow instead of panicking. Keep the fix localized to the
`PipeReader` read path so `start_file_offset` and subsequent reads fail
gracefully on invalid or overflowing offsets.
In `@src/io/PipeWriter.rs`:
- Around line 1281-1293: sync_write_all currently treats short/zero-byte writes
as success and returns Ok(()) even when data remains, so the buffered and
streaming Windows write paths can drop unwritten bytes. Update sync_write_all to
preserve and expose how many bytes were actually written (or keep the remaining
slice/buffer mutable in place), and adjust its callers so they only retire
pending_payload_size/current_payload.size() by the OS-accepted byte count. Make
sure the Windows file-write flow still stops on error, but never advances as if
the full payload drained unless all bytes were written.
In `@src/io/source.rs`:
- Around line 559-594: Process-wide STDIN caching is sharing a loop-bound
TtySource across event loops, which can route set_mode and close-path operations
to the wrong owning loop. Update stdin_tty::get and the STDIN storage logic so
stdin access is either confined to the owning loop/thread or marshaled back to
the loop passed into TtySource::open. Use the existing symbols stdin_tty::get,
TtySource::open, STDIN, and is_stdin_tty to keep the ownership model explicit
and prevent cross-loop reuse.
In `@src/iocp/afd.rs`:
- Around line 835-871: The handle tail logic in the AFD completion path is
reading through the raw watcher pointer after invoking a user callback, but that
callback can synchronously call close() and free the handle via the Loop endgame
path. Update the callback/endgame flow around the polling completion handling so
the handle is held or endgames are blocked while a callback is on the stack, and
revalidate liveness before any post-callback access in the submit_poll_req /
is_closing rearm path.
- Around line 348-360: The duplicate-socket guard is being removed too early in
close(), which can let another watcher start on the same socket before this
watcher’s in-flight IRPs and dummy cancel poll are fully drained. Move the
poll_watched_sockets removal out of the close path and into poll_endgame, using
the existing close/endgame flow in afd.rs so the guard stays active until both
slots have finished. Use the same socket-removal logic currently inside the
unsafe block, but trigger it only after the endgame completion state is reached.
In `@src/iocp/event_loop.rs`:
- Around line 232-236: The wakeup path in event_loop::wake leaves wakeup_pending
set when PostQueuedCompletionStatus fails, which blocks later wake() calls from
retrying. Update the failure path around the PostQueuedCompletionStatus call to
either clear wakeup_pending before returning or make the failure unrecoverable,
and use the existing wakeup_req and wakeup_pending state in event_loop::wake to
keep the coalescing logic correct.
In `@src/iocp/fsevent.rs`:
- Around line 543-553: The fs-event dispatch in `src/iocp/fsevent.rs` uses raw
handle state after invoking `cb`, but that callback can synchronously call
`close()` and free the handle before the post-callback re-arm/endgame checks
run. Update the `cb` call sites and the affected endgame paths in
`process_batch`/dispatch logic to hold a callback-in-progress liveness guard (or
otherwise block endgame draining while callbacks are active) so any subsequent
reads of `h` are only done when the handle is still guaranteed alive.
In `@src/iocp/handle.rs`:
- Around line 191-200: Make want_endgame internal instead of public, since it
should only be reachable after close() has established the CLOSING state. Update
the HandleCore::want_endgame queueing path to assert the CLOSING precondition
before setting ENDGAME_QUEUED and calling endgame_push, so live handles cannot
be queued for teardown prematurely. Ensure any call sites that currently use
want_endgame are routed through the closing flow rather than exposed externally.
In `@src/iocp/init.rs`:
- Around line 150-158: `ensure_winsock` currently aborts via `assert_eq!` when
`WSAStartup` fails, which turns a recoverable Windows networking error into a
process crash. Change `ensure_winsock` to return a fallible result and propagate
the `WSAStartup` status instead of asserting, then update `Bun__ensure_winsock`
and its callers to handle/report the `WSA*` error so the underlying failure can
be surfaced without aborting.
---
Outside diff comments:
In `@src/cares_sys/c_ares.rs`:
- Around line 2166-2191: The get_sockaddr API currently hides a
storage-size/alignment precondition behind a safe &mut sockaddr parameter, but
the sockaddr_in/sockaddr_in6 casts in c_ares.rs assume sockaddr_storage-sized
backing memory. Update get_sockaddr to take &mut sockaddr_storage (preferred) or
mark it unsafe with a documented caller contract, and adjust the
sockaddr_in/sockaddr_in6 writes so the boundary enforces the invariant before
any family-specific cast occurs.
In `@src/install/PackageInstall.rs`:
- Around line 2084-2092: The Windows symlink probe in is_dangling_symlink() is
using a file open path, which can misclassify valid directory links/junctions as
dangling. Update the Windows branch to detect directory targets the same way
install_from_link() expects, using the directory-open semantics already
documented in this module rather than a bare sys::open(path, 0, 0) probe. Keep
the existing control flow in is_dangling_symlink() but make the open call match
the target type so directory symlinks are not rejected as DanglingSymlink.
In `@src/io/keep_alive.rs`:
- Around line 119-138: The deferred unref paths in keep_alive::KeepAliveState
are leaving concurrent_origin stale, so a ref later can skip the
concurrent-release bookkeeping and break the net keepalive count. Update
unref_on_next_tick and unref_on_next_tick_concurrently to consume or clear
concurrent_origin when transitioning to Inactive, and make sure any
concurrent-release state is reconciled before incrementing the pending unref
counter. Keep the logic aligned with ref_concurrently and the existing status
handling so later re-refs/unrefs start from a clean state.
In `@src/io/lib.rs`:
- Around line 1804-1815: Move the raw loop pointer conversion out of
`unregister()`’s shared prelude and into the `#[cfg(not(windows))]` branch so
the Windows no-op path never calls `Self::uws_loop_mut` or creates a mutable
borrow. Keep the POSIX path using `self.inner().unregister(loop_, force)` with
the converted `Loop`, and leave the Windows branch as a true no-op that only
ignores `force` and `loop_`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13eea6d5-69b1-4474-99c1-b4d477e4b18a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (267)
Cargo.tomlbench/libuv-removal/.gitignorebench/libuv-removal/README.mdbench/libuv-removal/asyncfs-bunfile-vs-readfile.mjsbench/libuv-removal/asyncfs-singlethread-knee.mjsbench/libuv-removal/asyncfs-workers-global-pool.mjsbench/libuv-removal/compare-fs-after.tsbench/libuv-removal/composite-gen-fixtures.mjsbench/libuv-removal/composite-install-extract-link.mjsbench/libuv-removal/composite-startup-resolve.mjsbench/libuv-removal/composite-test-parallel.mjsbench/libuv-removal/dns-fs-interference.mjsbench/libuv-removal/dns-lookup-concurrency.mjsbench/libuv-removal/eventloop-idle-cpu.mjsbench/libuv-removal/eventloop-setimmediate-chain.mjsbench/libuv-removal/eventloop-settimeout-chain.mjsbench/libuv-removal/eventloop-timer-precision.mjsbench/libuv-removal/eventloop-worker-pingpong.mjsbench/libuv-removal/fswatch-burst.mjsbench/libuv-removal/hrtime-callcost.mjsbench/libuv-removal/nullchild.cbench/libuv-removal/pipes-ipc-throughput.mjsbench/libuv-removal/pipes-raw-readfile-control.mjsbench/libuv-removal/pipes-stdin-throughput.mjsbench/libuv-removal/pipes-stdout-throughput.mjsbench/libuv-removal/spawn-async-throughput.mjsbench/libuv-removal/spawn-pipe-tax.mjsbench/libuv-removal/spawn-sync-overhead.mjsbench/libuv-removal/stdout-throughput.mjsbench/libuv-removal/sync-fs-open-close-readfile.mjsbench/libuv-removal/sync-fs-readsync-positioned.mjsbench/libuv-removal/sync-fs-stat-resolver.mjsbench/libuv-removal/tcp-loopback-eventing.mjspackages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/eventing/bun_iocp.hpackages/bun-usockets/src/internal/eventing/libuv.hpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/quic.cpackages/bun-usockets/src/socket.cpatches/libuv/win-poll-rearm-before-callback.patchscripts/build/bun.tsscripts/build/deps/index.tsscripts/build/deps/libuv.tsscripts/build/deps/nodejs-headers.tsscripts/build/flags.tsscripts/build/source.tssrc/bun_bin/lib.rssrc/bun_core/Cargo.tomlsrc/bun_core/heap.rssrc/bun_core/lib.rssrc/bun_core/output.rssrc/bun_core/string/immutable.rssrc/bun_core/util.rssrc/bun_core/windows_sys.rssrc/bundler/BundleThread.rssrc/cares_sys/Cargo.tomlsrc/cares_sys/c_ares.rssrc/cares_sys/lib.rssrc/collections/vec_ext.rssrc/errno/Cargo.tomlsrc/errno/darwin_errno.rssrc/errno/freebsd_errno.rssrc/errno/lib.rssrc/errno/linux_errno.rssrc/errno/uv_numbers.rssrc/errno/win_error.rssrc/errno/windows_errno.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/Cargo.tomlsrc/event_loop/MiniEventLoop.rssrc/event_loop/SpawnSyncEventLoop.rssrc/fdtable/Cargo.tomlsrc/fdtable/lib.rssrc/fdtable/table.rssrc/install/Cargo.tomlsrc/install/PackageInstall.rssrc/install/PackageManager/security_scanner.rssrc/install/extract_tarball.rssrc/install/lifecycle_script_runner.rssrc/install/windows-shim/main.rssrc/io/Cargo.tomlsrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/keep_alive.rssrc/io/lib.rssrc/io/pipes.rssrc/io/source.rssrc/io/windows_event_loop.rssrc/iocp/Cargo.tomlsrc/iocp/afd.rssrc/iocp/dispatch.rssrc/iocp/event_loop.rssrc/iocp/fsevent.rssrc/iocp/handle.rssrc/iocp/init.rssrc/iocp/lib.rssrc/iocp/pipe.rssrc/iocp/process.rssrc/iocp/process/tests.rssrc/iocp/req.rssrc/iocp/signal.rssrc/iocp/timer.rssrc/iocp/tty.rssrc/iocp/tty/tests.rssrc/iocp/usockets.rssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal-for-testing.tssrc/js_printer/lib.rssrc/jsc/Cargo.tomlsrc/jsc/Debugger.rssrc/jsc/GarbageCollectionController.rssrc/jsc/JSONLineBuffer.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/OsBinding.cppsrc/jsc/bindings/ProcessBindingTTYWrap.cppsrc/jsc/bindings/c-bindings.cppsrc/jsc/bindings/libuv/generate_uv_posix_stubs.tssrc/jsc/bindings/libuv/generate_uv_posix_stubs_constants.tssrc/jsc/bindings/uv-posix-polyfills.csrc/jsc/bindings/uv-posix-polyfills.hsrc/jsc/bindings/uv-posix-stubs.csrc/jsc/bindings/vm/Semaphore.cppsrc/jsc/bindings/vm/Semaphore.hsrc/jsc/event_loop.rssrc/jsc/ipc.rssrc/jsc/ipc_frame.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/virtual_machine_exports.rssrc/jsc/web_worker.rssrc/libarchive/lib.rssrc/libuv_sys/Cargo.tomlsrc/libuv_sys/lib.rssrc/libuv_sys/libuv.rssrc/paths/string_paths.rssrc/resolver/fs.rssrc/resolver/fs/stat_hash.rssrc/resolver/lib.rssrc/runtime/Cargo.tomlsrc/runtime/api/BunObject.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/SubprocessPipeReader.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create/SourceFileProjectGenerator.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/link_command.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/pm_version_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test/parallel/Worker.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/upgrade_command.rssrc/runtime/dispatch.rssrc/runtime/dispatch_js2native.rssrc/runtime/dns_jsc/dns.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/node.rssrc/runtime/node/StatFS.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/node_os.rssrc/runtime/node/signal_handle_windows.rssrc/runtime/node/types.rssrc/runtime/node/uv_signal_handle_windows.rssrc/runtime/node/win_watcher.rssrc/runtime/server/FileResponseStream.rssrc/runtime/server/FileRoute.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/IO.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/Yield.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Handlers.rssrc/runtime/socket/Listener.rssrc/runtime/socket/SSLConfig.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/mod.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/timer/mod.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/shell_parser/parse.rssrc/spawn/Cargo.tomlsrc/spawn/lib.rssrc/spawn/process.rssrc/spawn/static_pipe_writer.rssrc/spawn_sys/Cargo.tomlsrc/spawn_sys/lib.rssrc/spawn_sys/spawn_process.rssrc/symbols.defsrc/sys/Cargo.tomlsrc/sys/Error.rssrc/sys/PosixStat.rssrc/sys/dir.rssrc/sys/fd.rssrc/sys/lib.rssrc/sys/sys_uv.rssrc/sys/tmp.rssrc/sys/windows/fs.rssrc/sys/windows/mod.rssrc/sys/windows/quirks/addenda.mdsrc/sys/windows/quirks/fs-links-dir.mdsrc/sys/windows/quirks/fs-meta.mdsrc/sys/windows/quirks/fs-open-io.mdsrc/sys/windows/quirks/loop-core.mdsrc/sys/windows/quirks/misc-history.mdsrc/sys/windows/quirks/pipes.mdsrc/sys/windows/quirks/poll.mdsrc/sys/windows/quirks/process.mdsrc/sys/windows/quirks/signal-fsevent.mdsrc/sys/windows/quirks/sockets.mdsrc/sys/windows/quirks/status.tomlsrc/sys/windows/quirks/tty.mdsrc/sys/windows/quirks/util-os.mdsrc/sys_jsc/error_jsc.rssrc/sys_jsc/fd_jsc.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/InternalLoopData.rssrc/uws_sys/Loop.rssrc/uws_sys/Response.rssrc/uws_sys/lib.rssrc/uws_sys/socket.rssrc/windows_sys/externs.rssrc/windows_sys/lib.rssrc/winfs/Cargo.tomlsrc/winfs/fsio.rssrc/winfs/fslnk.rssrc/winfs/fsmisc.rssrc/winfs/lib.rssrc/winfs/readdir.rssrc/winfs/stat.rstest/js/bun/spawn/spawn.test.tstest/js/bun/sys/error-name-from-libuv.test.tstest/js/node/process/process.test.jstest/js/web/streams/streams.test.jstest/js/web/timers/setImmediate.test.jstest/js/web/timers/setTimeout.test.jstest/napi/uv-stub-stuff/good_plugin.ctest/napi/uv-stub-stuff/plugin.ctest/napi/uv-stub-stuff/uv_impl.ctest/napi/uv.test.tstest/napi/uv_stub.test.ts
💤 Files with no reviewable changes (10)
- packages/bun-usockets/src/internal/eventing/libuv.h
- src/errno/Cargo.toml
- packages/bun-usockets/src/eventing/libuv.c
- scripts/build/deps/index.ts
- src/bun_core/windows_sys.rs
- scripts/build/deps/libuv.ts
- src/install/windows-shim/main.rs
- packages/bun-usockets/src/loop.c
- packages/bun-usockets/src/internal/loop_data.h
- patches/libuv/win-poll-rearm-before-callback.patch
| for (const [label, env] of [ | ||
| ["default env", {}], | ||
| ["UV_THREADPOOL_SIZE=24", { UV_THREADPOOL_SIZE: "24" }], | ||
| ]) { | ||
| console.log(`--- ${label}`); | ||
| const r = spawnSync(process.execPath, [SELF, "--child"], { | ||
| stdio: "inherit", | ||
| env: { ...process.env, ...env }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear UV_THREADPOOL_SIZE for the baseline run.
The "default env" branch currently inherits any caller-provided UV_THREADPOOL_SIZE, so the supposed baseline can silently run with the same pool width as the comparison case and invalidate the numbers this script is meant to show.
Suggested fix
for (const [label, env] of [
- ["default env", {}],
+ ["default env", { UV_THREADPOOL_SIZE: undefined }],
["UV_THREADPOOL_SIZE=24", { UV_THREADPOOL_SIZE: "24" }],
]) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/libuv-removal/asyncfs-singlethread-knee.mjs` around lines 152 - 160,
The baseline run in the asyncfs benchmark still inherits UV_THREADPOOL_SIZE from
the parent process, so the “default env” case may not be a true baseline. Update
the spawnSync call in the benchmark loop so the default branch explicitly clears
UV_THREADPOOL_SIZE while keeping the comparison branch set to 24, using the
existing SELF, spawnSync, and env setup to locate the change.
| for (const wk of workers) wk.postMessage({ cmd: "bye" }); | ||
| await Promise.all(workers.map(wk => new Promise(r => wk.on("exit", r)))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Attach the exit waiters before posting bye.
A fast worker can terminate between postMessage({ cmd: "bye" }) and the later wk.on("exit", ...), which leaves the final Promise.all(...) pending forever and hangs the benchmark. Build the exit promises first, then send the shutdown message.
Suggested fix
- for (const wk of workers) wk.postMessage({ cmd: "bye" });
- await Promise.all(workers.map(wk => new Promise(r => wk.on("exit", r))));
+ const exits = workers.map(wk => new Promise(r => wk.once("exit", r)));
+ for (const wk of workers) wk.postMessage({ cmd: "bye" });
+ await Promise.all(exits);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const wk of workers) wk.postMessage({ cmd: "bye" }); | |
| await Promise.all(workers.map(wk => new Promise(r => wk.on("exit", r)))); | |
| const exits = workers.map(wk => new Promise(r => wk.once("exit", r))); | |
| for (const wk of workers) wk.postMessage({ cmd: "bye" }); | |
| await Promise.all(exits); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/libuv-removal/asyncfs-workers-global-pool.mjs` around lines 176 - 177,
The shutdown flow in the worker loop should create the `exit` waiters before
sending `bye`, because a worker can exit too quickly and miss the later listener
registration. In `asyncfs-workers-global-pool.mjs`, update the final `workers`
teardown so the `Promise.all(...)` of `wk.on("exit", ...)` promises is built
first, then call `postMessage({ cmd: "bye" })` on each worker, preserving the
same `workers` and `wk` logic.
| const [out, err, code] = await Promise.all([ | ||
| new Response(proc.stdout).text(), | ||
| new Response(proc.stderr).text(), | ||
| proc.exited, | ||
| ]); | ||
| if (code !== 0) return `EXIT ${code}\n${out}\n${err}`; | ||
| return out; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast when a benchmark child exits non-zero.
Returning EXIT ... as normal output lets pickSummary() filter it away, so a broken BEFORE/AFTER binary can still produce a successful wrapper run with misleading empty results.
Proposed fix
- if (code !== 0) return `EXIT ${code}\n${out}\n${err}`;
+ if (code !== 0) {
+ throw new Error(`benchmark child failed: ${bin} ${script}\nEXIT ${code}\n${out}\n${err}`);
+ }
return out;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [out, err, code] = await Promise.all([ | |
| new Response(proc.stdout).text(), | |
| new Response(proc.stderr).text(), | |
| proc.exited, | |
| ]); | |
| if (code !== 0) return `EXIT ${code}\n${out}\n${err}`; | |
| return out; | |
| const [out, err, code] = await Promise.all([ | |
| new Response(proc.stdout).text(), | |
| new Response(proc.stderr).text(), | |
| proc.exited, | |
| ]); | |
| if (code !== 0) { | |
| throw new Error(`benchmark child failed: ${bin} ${script}\nEXIT ${code}\n${out}\n${err}`); | |
| } | |
| return out; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/libuv-removal/compare-fs-after.ts` around lines 50 - 56, The benchmark
child handling in compare-fs-after.ts should fail the wrapper immediately
instead of returning an "EXIT ..." string when proc.exited is non-zero. Update
the logic around the Promise.all result in the compare function so a non-zero
code throws or rejects with the captured stdout/stderr, ensuring pickSummary()
cannot hide a broken BEFORE/AFTER binary. Use the proc.exited, stdout/stderr
reads, and the surrounding compare/pickSummary flow to locate the change.
| /// Queue this handle's endgame (idempotent). // quirk: LOOP-26 | ||
| pub fn want_endgame(&mut self) { | ||
| if self.flags & ENDGAME_QUEUED != 0 { | ||
| return; | ||
| } | ||
| self.flags |= ENDGAME_QUEUED; | ||
| let this: *mut HandleCore = self; | ||
| // SAFETY: `new`s contract — the loop outlives the handle. | ||
| unsafe { (*self.loop_).endgame_push(this) }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Hide want_endgame behind the closing invariant.
This method is public, but calling it before close() queues teardown for a live handle and can release active_handles incorrectly in run_endgame. Keep it private/internal and assert the CLOSING precondition at the queue site.
Proposed tightening
- pub fn want_endgame(&mut self) {
+ pub(crate) fn want_endgame(&mut self) {
+ debug_assert!(self.flags & CLOSING != 0, "endgame queued before close");
if self.flags & ENDGAME_QUEUED != 0 {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Queue this handle's endgame (idempotent). // quirk: LOOP-26 | |
| pub fn want_endgame(&mut self) { | |
| if self.flags & ENDGAME_QUEUED != 0 { | |
| return; | |
| } | |
| self.flags |= ENDGAME_QUEUED; | |
| let this: *mut HandleCore = self; | |
| // SAFETY: `new`s contract — the loop outlives the handle. | |
| unsafe { (*self.loop_).endgame_push(this) }; | |
| } | |
| /// Queue this handle's endgame (idempotent). // quirk: LOOP-26 | |
| pub(crate) fn want_endgame(&mut self) { | |
| debug_assert!(self.flags & CLOSING != 0, "endgame queued before close"); | |
| if self.flags & ENDGAME_QUEUED != 0 { | |
| return; | |
| } | |
| self.flags |= ENDGAME_QUEUED; | |
| let this: *mut HandleCore = self; | |
| // SAFETY: `new`s contract — the loop outlives the handle. | |
| unsafe { (*self.loop_).endgame_push(this) }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/iocp/handle.rs` around lines 191 - 200, Make want_endgame internal
instead of public, since it should only be reachable after close() has
established the CLOSING state. Update the HandleCore::want_endgame queueing path
to assert the CLOSING precondition before setting ENDGAME_QUEUED and calling
endgame_push, so live handles cannot be queued for teardown prematurely. Ensure
any call sites that currently use want_endgame are routed through the closing
flow rather than exposed externally.
| pub fn ensure_winsock() { | ||
| static ONCE: Once = Once::new(); | ||
| ONCE.call_once(|| { | ||
| if bun_windows_sys::user32::GetSystemMetrics(bun_windows_sys::user32::SM_CLEANBOOT) != 1 { | ||
| let mut wsa_data = core::mem::MaybeUninit::<WSADATA>::zeroed(); | ||
| // SAFETY: valid out-pointer; winsock 2.2 always available. | ||
| let r = unsafe { WSAStartup(0x0202, wsa_data.as_mut_ptr()) }; | ||
| assert_eq!(r, 0, "WSAStartup failed: {r}"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify every caller can either propagate or translate a fallible Winsock init result.
rg -n -C 3 '\bensure_winsock\b|Bun__ensure_winsock|WSAStartup' src packagesRepository: oven-sh/bun
Length of output: 29085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the local context around ensure_winsock and its callers.
sed -n '130,190p' src/iocp/init.rs
printf '\n--- callers ---\n'
sed -n '4170,4225p' src/sys/lib.rs
printf '\n--- dns caller ---\n'
sed -n '2568,2592p' src/runtime/dns_jsc/dns.rs
printf '\n--- bsd caller ---\n'
sed -n '680,696p' packages/bun-usockets/src/bsd.cRepository: oven-sh/bun
Length of output: 5297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect adjacent Windows init helpers and any existing fatal-init conventions.
sed -n '1,120p' src/iocp/init.rs
printf '\n--- windows mod wrapper ---\n'
sed -n '1,60p' src/sys/windows/mod.rs
printf '\n--- any other WSAStartup handling ---\n'
rg -n -C 2 'WSAStartup|assert_eq!\(r, 0|WSANOTINITIALISED|WSASYSNOTREADY|WSAVERNOTSUPPORTED' src packagesRepository: oven-sh/bun
Length of output: 30718
Propagate WSAStartup failures out of ensure_winsock.
assert_eq! turns a network-stack/OS error into a process abort on the first socket/DNS path that hits Windows networking. Thread a fallible result through ensure_winsock/Bun__ensure_winsock so callers can surface the underlying WSA* error instead of crashing Bun.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/iocp/init.rs` around lines 150 - 158, `ensure_winsock` currently aborts
via `assert_eq!` when `WSAStartup` fails, which turns a recoverable Windows
networking error into a process crash. Change `ensure_winsock` to return a
fallible result and propagate the `WSAStartup` status instead of asserting, then
update `Bun__ensure_winsock` and its callers to handle/report the `WSA*` error
so the underlying failure can be surfaced without aborting.
Source: Coding guidelines
| !ever_registered, | ||
| "windows FilePoll can never have registered" | ||
| ); | ||
| } | ||
|
|
||
| // Safe fn item: module-private thunk, only coerced to the C-ABI | ||
| // `OpaqueCallback` fn-pointer type — never callable by name outside | ||
| // `Store`. Body wraps its raw-ptr op explicitly. | ||
| extern "C" fn process_deferred_frees_thunk(ctx: *mut c_void) { | ||
| // SAFETY: `ctx` was set to `self as *mut Store` in `put` above. The thunk fires | ||
| // from the event loop's after-tick hook with no other `&mut Store` borrow live, | ||
| // so this is the unique accessor (safe-single-owner). | ||
| let this = unsafe { bun_ptr::callback_ctx::<Store>(ctx) }; | ||
| this.process_deferred_frees(); | ||
| } | ||
| } | ||
|
|
||
| pub struct Waker { | ||
| // `BackRef<WindowsLoop>`: `WindowsLoop::get()` hands out the shared | ||
| // process-global singleton; the pointee strictly outlives every `Waker`. | ||
| // Safe `Deref` only — `wait`/`wake` route the raw `as_ptr()` straight to | ||
| // the C entry points so no `&mut WindowsLoop` is ever materialised (a | ||
| // concurrent `wake()` from a worker thread cannot alias). | ||
| loop_: bun_ptr::BackRef<WindowsLoop>, | ||
| } | ||
| // SAFETY: `Waker::wake()` only forwards to `WindowsLoop::wakeup()`, which is | ||
| // the documented cross-thread wake path (uv_async_send under the hood). | ||
| unsafe impl Send for Waker {} | ||
| unsafe impl Sync for Waker {} | ||
|
|
||
| impl Waker { | ||
| // `Result` kept (despite being infallible here) for signature parity with | ||
| // the POSIX wakers, whose `init` can fail (eventfd / kqueue). | ||
| pub fn init() -> Result<Waker, bun_core::Error> { | ||
| Ok(Waker { | ||
| loop_: bun_ptr::BackRef::from( | ||
| ptr::NonNull::new(WindowsLoop::get()).expect("WindowsLoop::get() singleton"), | ||
| ), | ||
| }) | ||
| } | ||
|
|
||
| /// The libuv loop backing the process-global `WindowsLoop`. Exposed so | ||
| /// callers that need a bare `uv_loop_t*` (e.g. `BundleThread`'s keep-alive | ||
| /// timer) can wire libuv handles without holding a `&WindowsLoop` borrow | ||
| /// against the shared global. | ||
| #[inline] | ||
| pub fn uv_loop(&self) -> *mut uv::Loop { | ||
| // `BackRef` deref is safe (pointee outlives holder); `uv_loop` is a | ||
| // `Copy` field set once by `us_create_loop` and immutable for the | ||
| // process. | ||
| self.loop_.uv_loop | ||
| } | ||
|
|
||
| // `getFd`/`initWithFileDescriptor` must never be referenced on Windows, | ||
| // so they are simply not defined here — POSIX-only call sites are | ||
| // `cfg`-gated, so a stray Windows use fails the build. | ||
|
|
||
| pub fn wait(&self) { | ||
| // Do NOT go through `WindowsLoop::wait(&mut self)`: that would | ||
| // materialize a `&mut WindowsLoop` over the process-global singleton | ||
| // for the entire duration of `us_loop_run`/`uv_run`, and a concurrent | ||
| // `wake()` from a worker thread would alias it (two live `&mut T` to | ||
| // one allocation = UB under Stacked/Tree Borrows). Call the C entry | ||
| // point with the raw pointer directly — no exclusivity claimed. | ||
| // SAFETY: `loop_` is the live `WindowsLoop::get()` singleton. | ||
| unsafe { waker_c::us_loop_run(self.loop_.as_ptr()) }; | ||
| } | ||
|
|
||
| pub fn wake(&self) { | ||
| // See `wait()` — call the thread-safe C wake path with the raw pointer | ||
| // instead of forming a `&mut WindowsLoop` that would alias the | ||
| // event-loop thread's borrow held across `us_loop_run`. | ||
| // SAFETY: `loop_` is the live `WindowsLoop::get()` singleton; | ||
| // `us_wakeup_loop` → `uv_async_send` is documented thread-safe. | ||
| unsafe { waker_c::us_wakeup_loop(self.loop_.as_ptr()) }; | ||
| } | ||
| } | ||
|
|
||
| // Local extern shims for `Waker`: the canonical decls live in | ||
| // `bun_uws_sys::loop_::c` but that module is crate-private. Re-declaring the | ||
| // two symbols here lets `Waker::{wait,wake}` pass the raw `*mut WindowsLoop` | ||
| // without round-tripping through a `&mut self` receiver (see comments above). | ||
| mod waker_c { | ||
| use super::WindowsLoop; | ||
| unsafe extern "C" { | ||
| pub(super) fn us_loop_run(loop_: *mut WindowsLoop); | ||
| pub(super) fn us_wakeup_loop(loop_: *mut WindowsLoop); | ||
| let poll = poll.as_ptr(); | ||
| // SAFETY: `poll` is a fully-initialized hive slot; FilePoll has no | ||
| // drop glue, so `put` is a no-op drop + recycle. | ||
| unsafe { self.hive.put(poll) }; | ||
| } |
There was a problem hiding this comment.
🟡 The next_to_free: *mut FilePoll field on the Windows FilePoll (line 39) and its ptr::null_mut() initializer (line 78) are now dead — this PR deleted the Windows Store deferred-free list (pending_free_head/pending_free_tail/process_deferred_frees) that consumed it, and put() now recycles immediately. The POSIX FilePoll in posix_event_loop.rs still uses its own next_to_free, but that is a separate struct. Per CLAUDE.md ('Delete dead code in the same PR that makes it dead — fields nothing reads'), the field and initializer should be removed.
Extended reasoning...
What the bug is
The Windows FilePoll struct at src/io/windows_event_loop.rs:39 declares pub next_to_free: *mut FilePoll, and FilePoll::init() at line 78 initializes it to ptr::null_mut(). After this PR, that field is never read on Windows — it is write-once dead state (8 bytes per FilePoll).
Why this PR made it dead
The diff for windows_event_loop.rs deletes the entire Windows deferred-free mechanism from Store:
pending_free_head: *mut FilePoll/pending_free_tail: *mut FilePollfieldsprocess_deferred_frees()andprocess_deferred_frees_thunk()- The intrusive-list walk in
put()that read(*current).next_to_freeand wroteself.pending_free_tail = poll
The new Store::put() (lines 221–227) recycles the hive slot immediately, with the comment "Registration never happens on Windows (no register() here), so recycling is immediate — the POSIX deferred-free list has no Windows counterpart." The next_to_free field's only consumer was that deleted list walk.
Why the POSIX field doesn't cover it
Grep across src/ shows all remaining reads of next_to_free live in src/io/posix_event_loop.rs (lines 308, 321, 570, 1443–1476). That is a separate FilePoll struct definition — the POSIX one — with its own still-live deferred-free Store. The two structs share a name but nothing else; no code path reads the Windows struct's next_to_free.
Step-by-step proof
rg next_to_free src/io/windows_event_loop.rs→ exactly two hits: the declaration at :39 and the initializer at :78. No reads.rg next_to_free src/→ every other hit is inposix_event_loop.rs, operating on the POSIXFilePoll(declared at posix_event_loop.rs:308).- The PR diff removes
- pub fn process_deferred_frees(&mut self) {…- let mut next = self.pending_free_head;…(*current).next_to_freefrom the WindowsStore— the only Windows reader. - The new
Storestruct (windows_event_loop.rs:202–204) has one field,hive; no intrusive list.
Impact
None at runtime — it's an unused 8-byte pointer per Windows FilePoll plus one dead store in init(). This is purely a CLAUDE.md "Code style & idioms reviewers enforce" cleanup: "Delete dead code in the same PR that makes it dead — fields nothing reads."
Fix
Delete line 39 (pub next_to_free: *mut FilePoll,) and the next_to_free: ptr::null_mut(), line in init().
|
Reviewed the spawn slice of this PR on Windows (debug build at c678475, Windows Server 2019), with released bun 1.3.14 as the libuv baseline for every failure. Ran the spawn, child_process, shell, terminal, and process suites (about 1490 tests). There is exactly one real regression class, plus two code-level findings. 1.
|
| suite | pass | fail | notes |
|---|---|---|---|
test/js/bun/spawn/spawn.test.ts |
117 | 0 | |
test/js/bun/spawn/ (41 files) |
272 | 5 | 3 = finding 1, 2 = timing noise (below) |
test/js/node/child_process/ (8 files) |
80 | 1 | 1 = finding 1 (run-p) |
test/js/bun/shell/ (40 files) |
810 | 10 | all non-regressions (below) |
test/js/bun/terminal/terminal-spawn.test.ts |
13 | 0 | |
test/js/node/process/process.test.js |
97 | 0 | incl. the new process.kill(0) tests |
9 race-prone spawn files, --rerun-each=4 |
84 | 0 | no intermittent failures |
Why the other 12 failures are not regressions
spawn-maxbuf.test.ts "yes is killed" x2 (toBeLessThan(100), got 147-156 ms): the child is [bunExe(), "exec", "yes"], the debug build itself. A 100 ms budget does not fit a debug bun child's startup. The baseline's bunExe() is the fast release binary. This is a pre-existing tightness in the test's budget, not the engine; it will also trip any debug or ASAN Windows lane.
Shell suite, 10 failures, all accounted for:
- 5 x
tilde_expansion: the harness's expected string is literally"undefined"(it reads$HOME, unset on this box) while the shell correctly expanded~toC:\Users\azureuser. The same tests fail the same way on 1.3.14. - 4 x timeouts, all at the very edge of their budgets (e.g.
memleak_Blob_nothingpassed at 93.9 s,memleak_Blob_somethinghit the 100 s cap). Debug slowness. - 1 x
fd leak > #11816 > builtin(11 liveShellInterpreterobjects): pollution from the immediately preceding#11816 > externaltest, which has a 5 s budget in a file whose siblings legitimately take 24-93 s on debug. It timed out, left a dangling process and live interpreters, andbuiltin'sheapStats()then saw them.
Verified clean by hand
ProcessHandle::spawn's error paths (src/iocp/process.rs:1306-1548) are tight: hThread is closed exactly once on all three terminal branches, ChildStdio::Drop and AttrList::Drop cover every early return, stdio_verify matches libuv's uv__stdio_verify byte for byte, and a RegisterWaitForSingleObject failure kills and closes the child rather than reporting a spawn success with no exit watch. The close/kill/endgame protocol is race-free: reaped latches before exit_cb, and close()'s blocking UnregisterWaitEx followed by the exit_posted read is a correct happens-before handshake. The quirks docs are genuinely good; every non-obvious decision is tagged and cross-referenced.
Couldn't run the Rust engine unit tests standalone (cargo test -p bun_iocp fails to link outside the full build; it needs mimalloc/simdutf/usockets). Not a finding.
The lazy Winsock init call was added to the shared uv_inet_ntop crash stub, which compiles on every platform while the symbol only exists on Windows — FreeBSD/Linux links failed with an undefined reference. The stub throws before the call could matter anyway; move the init into the Windows-only uv_inet_ntop implementation (the one that actually calls InetNtopA), inside the OS(WINDOWS) section.
|
Second, deeper pass on the pieces my comment above covered only through the test suites, mostly 4. CRITICAL:
|
The new Windows code brought the workspace under clippy's strict lint set for the first time; this clears every error with behavior preserved or deliberately corrected: - Pool large path buffers instead of stack-allocating them: ~330 sites move from 64-98 KB stack frames to the TLS buffer pool, including the NodeFS error-path scratch (was a per-operation allocation after an earlier boxing attempt) and the lockfile tree iterator (one field change replaces 15 identical per-site lint allowances). - Restore behavior that machine-applied suggestions had silently broken: the JUnit reporter's hostname computation; st_size clamping on POSIX (negative sizes from hostile filesystems clamp to 0 via a shared bun_sys::stat_size helper instead of panicking or wrapping); a Default derive that dropped Fd::INVALID initialization. - Read the NoDefaultCurrentDirectoryInExePath binary-planting opt-out live via NeedCurrentDirectoryForExePathW (matching the process engine) instead of a startup-cached env read, so setting it mid-process takes effect. - Write real SAFETY comments at every flagged unsafe block (COM/WIC vtable contracts, PEB access, kernel out-params, shim pointer arithmetic); make double-free-hazard helpers honestly unsafe fn. - Recursive-delete entry helpers use a small stack buffer with an explicit ENAMETOOLONG instead of silent truncation. - Mechanical: &raw mut/const borrows, c-string literals, matches!, is_ascii_alphabetic, ok_or_else, ManuallyDrop over mem::forget, read_unaligned for getaddrinfo sockaddr copies.
…ngine races Windows fixes for issues found in review of the event loop rewrite: - Positional reads/writes on non-seekable fds (pipes, character devices, consoles) pass through to the kernel, which ignores the offset and reads sequentially, instead of failing with an unmapped error code. This restores process.stdin in every child spawned with the default (NUL-device) stdin. Explicit lseek still rejects, now as ESPIPE via a boundary mapping (the general errno table stays row-for-row libuv parity, which has no 132 row). read_at/write_at wait out ERROR_IO_PENDING with GetOverlappedResult so a handle opened overlapped can never orphan a kernel-armed stack OVERLAPPED. - ConPTY spawns no longer build the default NUL stdio trio, so bInheritHandles is FALSE and terminal children inherit nothing, closing a leak where every inheritable handle in the process (and racing spawns' pipe ends) leaked into the child. - The pipe read path guards the stash-delivery arm against double queuing the embedded read request (a second same-turn read_start is a retarget no-op), and the EOF grace timer is stopped in read_stop and re-armed only when a real kernel read is submitted, so it can neither fire while the reader is stopped nor misread an unsubmitted request and force-close over live stashed data. Deviation from libuv (which delivers EOF to stopped readers) is recorded in the quirk ledger. - Shell 2>&1 capture pipes are adopted onto the event loop like every other capture (worker-pool reads) instead of blocking synchronous reads on the JS thread; the now-unused BufferFd stdio result variant is deleted. - Every completion-packet post goes through a post_or_die helper: a lost post wedges a latched protocol forever, so failures are loud (matching the existing signal-path precedent), including the previously unchecked pipe connect-thread post. - KeepAlive's deferred-unref path releases its concurrent-origin accounting like the immediate path, preventing worker-teardown over-subtraction. - Shell redirect writers classify regular files by exact GetFileType match; the old bitmask classified every file type as pollable, which sent regular-file writes down the async path.
…sh/bun into claude/busy-vaughan-130bdf
|
All 10 findings addressed in f1fec2c. Each was re-verified against source before fixing; three fixes ended up deeper or different than suggested: 1 (pread on NUL/char devices) — confirmed, both halves. Fixed by opening 2 (ConPTY inheritance) — confirmed. Fixed at the source you pointed at: a pseudoconsole spawn builds an empty 3 + 6 (unchecked 4 (stash-arm double queue) — confirmed. The stash arm now has the same 5 (EOF timer manufacturing stash + read_pending) — confirmed, but the fix went one level deeper, and one correction to the writeup: libuv's 7 (dup2 server end overlapped) — confirmed, but the suggested non-overlapped pair turned out to trade an error for something worse: the 8 (tty 9 (KeepAlive next-tick accounting) — confirmed. 10 (pollable mask) — confirmed, including the |
| /// Whether a posted wakeup packet has not yet been dequeued. The async | ||
| /// close protocol must wait for in-flight packets before teardown. | ||
| /// // quirk: LOOP-32 | ||
| pub fn wakeup_in_flight(&self) -> bool { | ||
| self.wakeup_pending.load(Ordering::Acquire) | ||
| } | ||
|
|
||
| /// Dispatch arm for the wakeup packet. Cleared at dispatch time, BEFORE | ||
| /// any further processing: a `wake()` racing this (or sent from a | ||
| /// callback) must post a fresh packet, not coalesce into one being | ||
| /// consumed. // quirk: LOOP-31 | ||
| pub(crate) fn consume_wakeup(&mut self) { | ||
| self.wakeup_pending.store(false, Ordering::Release); | ||
| } | ||
|
|
||
| // ── pending queue ───────────────────────────────────────────────────── | ||
|
|
||
| /// Queue a locally-completed request for dispatch on the next | ||
| /// `process_pending`. Single error-delivery funnel: synchronous submit | ||
| /// failures call `req.set_error(..)` then this, so callers observe every | ||
| /// failure asynchronously, exactly like a kernel completion. | ||
| /// // quirk: POLL-28 | ||
| /// | ||
| /// # Safety | ||
| /// `req` must stay alive until dispatched; it must not already be queued. | ||
| pub unsafe fn insert_pending(&mut self, req: *mut Req) { | ||
| // SAFETY: caller guarantees `req` outlives its dispatch and is not |
There was a problem hiding this comment.
🔴 The fix commit f1fec2c made read_stop stop the EOF grace timer (pipe.rs:813) with the stated intent "re-arm in read_start" (:809), but the re-arm at :786-793 sits INSIDE the else if !self.read_pending branch — so when a real kernel read is carried across pause/resume (has_stash=false && read_pending=true, peer silent), neither arm at :771/:783 executes and the timer stays stopped. sock.end(); sock.pause(); sock.resume() on a Windows named pipe with a silent/dead peer now hangs indefinitely instead of force-closing after 50ms. Add a third arm else if self.eof_timer_active { eof_timer_start(lp, hp) }; the block at :786-793 is then redundant (submit_read :1512-1513 already re-arms). The fix-commit note verified peer-CLOSE (which stashes → has_stash path), not peer-SILENT. (Anchored here because pipe.rs has no diff line ranges in the tracker; the actual change is at src/iocp/pipe.rs:783-795.)
Extended reasoning...
What the bug is
Fix commit f1fec2c changed PipeHandle::read_stop (src/iocp/pipe.rs:806-813) to stop the EOF grace timer, with the comment at :807-810 stating the intent: "We stop it here and re-arm in read_start". But the re-arm in read_start (:786-793) is placed inside the else if !self.read_pending branch at :783. When a real kernel read is already in flight across pause/resume — has_stash == false && read_pending == true — neither the stash arm at :771 nor the submit arm at :783 executes, and read_start returns at :796 with the force-close timer still stopped.
The specific code path
read_start at pipe.rs:746-797 (post-f1fec2ca):
let has_stash = self.stashed.is_some(); // :759
...
if has_stash { // :771 — skipped (peer silent, nothing stashed)
if !self.read_pending { ... insert_pending ... }
} else if !self.read_pending { // :783 — skipped (kernel read still in flight)
unsafe { submit_read(lp, hp) };
if self.eof_timer_active && self.read_pending { // :786-793 — the re-arm, INSIDE this branch
unsafe { eof_timer_start(lp, hp) };
}
}
Ok(()) // :796 — timer still stoppedThe comment at :787-788 ("Resume during the EOF grace window with a real kernel read in flight") describes the intent correctly, but is misplaced — it sits inside the branch where read_pending was false on entry (a fresh read is being submitted). The case it describes — a real kernel read carried across pause — is exactly the case neither arm covers.
Why the re-arm at :786-793 is also redundant
submit_read at pipe.rs:1510-1514 already re-arms unconditionally when eof_timer_active:
// Every queued read during the post-shutdown grace re-arms the EOF window.
if (*h).eof_timer_active {
eof_timer_start(lp, h);
}So the block at :786-793 double-arms in the fresh-submit case (harmless — timer_start restarts) and does nothing at all for the carried-across-pause case that actually needs it.
Step-by-step proof
sock.end()→ shutdown completes →process_pipe_shutdown_reqpipe.rs:1917 setseof_timer_active = true; :1918-1919 arms the timer (a kernel read is pending,read_pending = true).sock.pause()→read_stop(): :805 setsreading = false; :806if self.eof_timer_active→ :813timer_stop(&mut self.eof_timer).read_pendingstays true (per :799-800 doc, the in-flight read is deliberately not cancelled).eof_timer_activestays true (only cleared at :1221 close and :1982 timer-cb).sock.resume()→read_start(): :759has_stash = false(peer sent nothing, no completion arrived while stopped). :771if has_stash→ false. :783else if !self.read_pending→ false (read still pending). Falls through to :796Ok(()). Timer stays stopped.- Peer stays silent (dead/firewalled). The kernel read never completes →
process_pipe_read_req:1566 never runs → the 50ms EOF force-close (EOF_TIMEOUT_MS, :1947) never fires → the pipe never emits EOF/close. Hang.
Why this is a regression introduced by f1fec2c
Before f1fec2c, read_stop did not stop the timer (it only set reading = false and called core.stop()), so — like libuv — the timer stayed armed through pause and force-closed after 50ms. The fix commit's own note says "Pause → peer-close → grace expiry → resume was exercised end to end" — that tests peer-CLOSE, which completes the kernel read, stashes it (:1604/:1619), and takes the has_stash path on resume. Peer-SILENT (kernel read never completes) is the uncovered gap.
There is a related sibling: pause() before shutdown-completion → :1918-1919 arms the timer with reading=false → timer fires → pipe_eof_timer_cb :1974-1976 debug_assert!(reading) trips (release: early-return at :1975) → resume() → same gap → same hang.
Impact
On a Windows named-pipe net.Socket, sock.end(); sock.pause(); sock.resume() (or any resume during the EOF grace window with the kernel read still parked) against a silent/dead peer hangs indefinitely instead of force-closing after 50ms. Before f1fec2c (and in libuv) the socket closed after the grace timeout. This defeats the entire purpose of the EOF grace mechanism (PIPE-51) for the silent-peer case whenever pause/resume is involved.
Fix
Add a third arm covering the missing case, and delete the now-redundant :786-793 block:
} else if !self.read_pending {
unsafe { submit_read(lp, hp) }; // internally re-arms at :1512-1513
} else if self.eof_timer_active {
// Real kernel read carried across pause/resume: re-arm the force-close
// timer read_stop stopped. has_stash is false here, so the req IS a
// primed kernel read and the timer's completed_volatile probe is safe.
unsafe { eof_timer_start(lp, hp) };
}(Anchored at event_loop.rs because the tracker has no diff line ranges for the new pipe.rs file; the actual change is at src/iocp/pipe.rs:783-795.)
| //! Node-visible uv errno protocol numbers. JS sees these in `err.errno` | ||
| //! (e.g. ENOENT = -4058 on Windows), so the NUMBERS are ABI — they survive | ||
| //! the libuv removal as pure protocol constants, owned here. | ||
|
|
There was a problem hiding this comment.
🔴 The Windows uv_os_setpriority polyfill at src/jsc/bindings/uv-posix-polyfills.c:404-413 uses <= where libuv uses strict < (plus a nested <= -20 that only matches -20), so every priority value BETWEEN the six os.constants.priority.* boundaries maps one Windows priority class too low — e.g. os.setPriority(0, 5) sets BELOW_NORMAL instead of NORMAL, and os.setPriority(0, -19) sets HIGH instead of REALTIME. This is reachable from os.setPriority() via set_process_priority (c-bindings.cpp:65) and is a regression vs the previously-linked real libuv; the repo's own quirk ledger OS-45 (util-os.md:319-321) says 'must-port … incl. the exact band edges' and cites libuv's <-14/<-7/<0/<10/<19 bands. Fix: change lines 404-413 to strict < per OS-45.
Extended reasoning...
What the bug is
The Windows uv_os_setpriority polyfill at src/jsc/bindings/uv-posix-polyfills.c:398-413 mistranscribes libuv's priority-band boundaries. libuv uses strict < comparisons; this port uses <=, and additionally combines the top two bands with a nested priority <= -20 check inside the first arm (which can only match exactly -20, since values below -20 are already rejected at line 402). The net effect is that every input value BETWEEN the six os.constants.priority.* boundary constants maps to a Windows priority class one bucket lower than libuv/Node.
libuv reference — the repo's own quirk ledger at src/sys/windows/quirks/util-os.md:319 (OS-45) documents libuv's util.c:1483-1497 bands verbatim:
set: bands
<-14→REALTIME, <-7→HIGH, <0→ABOVE_NORMAL, <10→NORMAL, <19→BELOW_NORMAL, else IDLE
and line 321 explicitly flags:
must-port (os.getPriority/setPriority parity incl. the exact band edges and ESRCH remap)
This port (lines 404-413 as read from source):
if (priority <= -14 /* UV_PRIORITY_HIGH..HIGHEST */)
cls = priority <= -20 ? REALTIME_PRIORITY_CLASS : HIGH_PRIORITY_CLASS;
else if (priority <= -7) cls = ABOVE_NORMAL_PRIORITY_CLASS;
else if (priority <= 0) cls = NORMAL_PRIORITY_CLASS;
else if (priority <= 10) cls = BELOW_NORMAL_PRIORITY_CLASS;
else cls = IDLE_PRIORITY_CLASS;The specific code path that triggers it
os.setPriority(pid, priority) on Windows routes through set_process_priority (src/jsc/bindings/c-bindings.cpp:62-69), which calls uv_os_setpriority(pid, priority) — this polyfill. So this is not just N-API surface for addons; it is Bun's own node:os implementation.
Why existing code doesn't prevent it
The six os.constants.priority.* values are exactly the intersection points where both mappings agree — each boundary sits at the top of its port bucket AND the bottom of its libuv bucket:
| priority | libuv (<) |
port (<=) |
|---|---|---|
| -20 (HIGHEST) | REALTIME | REALTIME ✓ |
| -14 (HIGH) | HIGH (falls to <-7) |
HIGH (<=-14, not <=-20) ✓ |
| -7 (ABOVE_NORMAL) | ABOVE_NORMAL | ABOVE_NORMAL ✓ |
| 0 (NORMAL) | NORMAL | NORMAL ✓ |
| 10 (BELOW_NORMAL) | BELOW_NORMAL | BELOW_NORMAL ✓ |
| 19 (LOW) | IDLE | IDLE ✓ |
So tests that only pass the constant boundary values pass — which is likely how this got through the suite.
Step-by-step proof
Band tables side by side:
| range | libuv (<-14/<-7/<0/<10/<19/else) |
port (<=-14 nested / <=-7 / <=0 / <=10 / else) |
|---|---|---|
| [-20, -15] | REALTIME | -20 → REALTIME; [-19,-15] → HIGH |
| [-14, -8] | HIGH | -14 → HIGH; [-13,-8] → ABOVE_NORMAL |
| [-7, -1] | ABOVE_NORMAL | -7 → ABOVE_NORMAL; [-6,-1] → NORMAL |
| [0, 9] | NORMAL | 0 → NORMAL; [1,9] → BELOW_NORMAL |
| [10, 18] | BELOW_NORMAL | 10 → BELOW_NORMAL; [11,18] → IDLE |
| [19] | IDLE | IDLE ✓ |
Concrete example: os.setPriority(0, 5).
set_process_priority(0, 5)→uv_os_setpriority(0, 5).- Line 402:
5is in[-20, 19], no EINVAL. - Line 404:
5 <= -14? No. Line 406:5 <= -7? No. Line 408:5 <= 0? No. Line 410:5 <= 10? Yes →cls = BELOW_NORMAL_PRIORITY_CLASS. SetPriorityClass(GetCurrentProcess(), BELOW_NORMAL_PRIORITY_CLASS).os.getPriority(0)→uv_os_getpriority(lines 372-395) →BELOW_NORMAL_PRIORITY_CLASS→ returns 10.
Under libuv/Node, step 3 evaluates 5 < 10 → NORMAL_PRIORITY_CLASS, and os.getPriority(0) returns 0. So the round-trip os.setPriority(0, 5); os.getPriority(0) diverges: Node returns 0, this port returns 10.
Other mismatching examples: -19 → libuv REALTIME / port HIGH; -8 → HIGH / ABOVE_NORMAL; -1 → ABOVE_NORMAL / NORMAL; 11 → BELOW_NORMAL / IDLE.
Impact
This is a regression: before this PR, real libuv was linked on Windows and uv_os_setpriority used the correct bands. Any user passing a non-boundary integer to os.setPriority() (e.g. os.setPriority(0, 5) for a mild deprioritization, or a value computed from POSIX nice arithmetic) gets a lower priority class than Node would set. os.setPriority(0, -19) — one below PRIORITY_HIGHEST — now sets HIGH instead of REALTIME. Blast radius is limited (most users pass the constants), but the divergence is concrete, user-reachable, and directly contradicts the ledger's own must-port note.
Fix
Rewrite lines 404-413 to strict < per OS-45 / libuv util.c:1483-1497:
if (priority < -14) cls = REALTIME_PRIORITY_CLASS;
else if (priority < -7) cls = HIGH_PRIORITY_CLASS;
else if (priority < 0) cls = ABOVE_NORMAL_PRIORITY_CLASS;
else if (priority < 10) cls = NORMAL_PRIORITY_CLASS;
else if (priority < 19) cls = BELOW_NORMAL_PRIORITY_CLASS;
else cls = IDLE_PRIORITY_CLASS;(This comment is anchored at src/errno/uv_numbers.rs because the tracker has no diff hunk for uv-posix-polyfills.c; the actual change is at that file, lines 404-413.)
| return 2; | ||
| } | ||
| panic!( | ||
| "Cast bun.FD.uv({}) makes closing impossible!\n\n\ | ||
| The supplier of fd FD should call 'FD.makeLibUVOwned',\n\ | ||
| "Cast bun.FD.js_fd({}) makes closing impossible!\n\n\ | ||
| The supplier of fd FD should call 'FD.make_table_owned',\n\ | ||
| probably where open() was called.", | ||
| self, | ||
| ); |
There was a problem hiding this comment.
🟡 Addendum to the stale-libuv-comments cleanup at loop_data.h:59 — three more locations this PR's renames orphaned that aren't listed there: (1) src/bun_core/util.rs:926 Full method set (close, makeLibUVOwned, …) — the method was renamed to make_table_owned in this PR (the panic message at :1064-1065 was updated but this section-header comment 261 lines up was missed; the camelCase spelling means a snake_case grep won't find it); (2) src/runtime/socket/Listener.rs:901-903 routing it through .uv() panics… (system→u64, uv→i32, posix→i32) — .uv() is now .js_fd(), the kind is Table; (3) src/runtime/socket/socket_body.rs:3255-3257, identical stale text. Update (not delete) — the socket ones document a real Windows panic hazard.
Extended reasoning...
What the bug is
Same class as the already-posted stale-libuv-comments finding anchored at packages/bun-usockets/src/internal/loop_data.h:59 (which covers loop_data.h / AnyEventLoop.rs / CLAUDE.md), but at three additional locations orphaned by this PR's own identifier renames that weren't listed there:
1. src/bun_core/util.rs:926 — the Fd section-header comment reads:
// Full method set (close, makeLibUVOwned, …) stays in bun_sys which re-exportsThis PR renames make_libuv_owned → make_table_owned (defined at util.rs:1187) and updates the panic message at util.rs:1064-1065 to say 'FD.make_table_owned', but this section-header comment 261 lines above still names the old method. It uses the camelCase spelling makeLibUVOwned, so a grep for the snake_case renamed function would not have found it — worth flagging explicitly.
2. src/runtime/socket/Listener.rs:901-903 — the get_fd comment reads:
// On Windows the listening socket fd is a system-kind SOCKET
// handle; routing it through `.uv()` panics for anything but
// stdio. The sys_jsc helper branches on kind
// (system→u64, uv→i32, posix→i32).This PR renames .uv() → .js_fd() (util.rs:1043-1047) and FdKind::Uv / DecodeWindows::Uv → Table (util.rs:1303). The comment still names the old accessor and kind.
3. src/runtime/socket/socket_body.rs:3255-3257 — identical stale text to #2.
Why this PR made them stale
The renames are all in this PR's util.rs diff: from_uv→from_js_fd, .uv()→.js_fd(), make_libuv_owned→make_table_owned, FdKind::Uv→FdKind::Table, DecodeWindows::Uv→DecodeWindows::Table. All three files are in this PR's changed-files list. The function bodies were updated to the new names (Listener.rs:905 and socket_body.rs:3259 call to_js_without_minting(), which internally uses the new API), but their doc comments were not.
Why existing findings don't cover it
The already-posted inline comment at loop_data.h:59 covers three DIFFERENT files (loop_data.h, AnyEventLoop.rs, CLAUDE.md) and targets deleted-mechanism references (quic_timer/uv_loop_t). These three locations reference renamed identifiers instead — same cleanup pass, distinct instances. All three verifiers confirmed this is not a duplicate.
Step-by-step proof
rg 'makeLibUVOwned' src/→ exactly one hit: util.rs:926.rg 'make_libuv_owned' src/→ zero hits (the function no longer exists under that name).rg 'fn make_table_owned' src/bun_core/util.rs→ line 1187 (the rename target).- util.rs:1064-1065:
"The supplier of fd FD should call 'FD.make_table_owned',\n"— the panic message was updated in this PR; only the section-header 261 lines above was missed. rg '\.uv\(\) panics' src/runtime/socket/→ Listener.rs:901 and socket_body.rs:3256.rg 'fn uv\(self\)' src/bun_core/util.rs→ zero hits;rg 'fn js_fd\(self\)' src/bun_core/util.rs→ the rename target.- util.rs:1303:
Table = 1,inenum FdKind—Uvno longer exists.
Impact
Zero runtime effect — pure comment staleness. Per CLAUDE.md "Comments carry only durable non-obvious content" and "docs invalidated by a change should be updated in the same PR", these should be updated in the same cleanup pass as the loop_data.h finding.
Fix
- util.rs:926:
makeLibUVOwned→make_table_owned(or drop the parenthetical example list entirely). - Listener.rs:901-903 / socket_body.rs:3255-3257:
.uv()→.js_fd(),uv→i32→table→i32. Do NOT delete these — they document a real Windows panic hazard (system-kind SOCKET fds have no numeric JS fd;.js_fd()on them panics per util.rs:1060-1068). Only the identifier names are stale.
| await bench(); | ||
| } else { | ||
| console.log("Bun.file().arrayBuffer() vs fs.promises.readFile(), same 64KB files, hot cache"); | ||
| console.log("(after libuv removal the removal, the Bun.file row should match/beat readFile)\n"); |
There was a problem hiding this comment.
🟡 The bench/libuv-removal/*.mjs files contain ~25 garbled phrases from a bulk find-and-replace of a plan-reference token with the literal string 'the removal'/'this removal' — producing 'the the removal', 'the removal of the removal plan', 'plan the removal', 'a the removal bug', etc. Several are in user-facing console.log output (this line, tcp-loopback-eventing.mjs:165), not just comments. Cosmetic (bench scripts are dev tooling), but worth a cleanup pass — grep rg 'the the removal|plan the removal|removal the removal|this removal\)' bench/libuv-removal/ finds them all.
Extended reasoning...
What the bug is
The bench/libuv-removal/*.mjs benchmark scripts and README.md contain ~25+ instances of nonsense text produced by a bulk find-and-replace of some plan-reference token (likely a phase marker like 'PHASE-N', '§N', or similar) with the literal string 'the removal' / 'this removal'. The substitution ran blind over comments, README table cells, and console.log output alike, producing doubled-article and self-referential gibberish.
Concrete instances
User-facing console.log output (developers see this when running the benchmarks):
asyncfs-bunfile-vs-readfile.mjs:138:'(after libuv removal the removal, the Bun.file row should match/beat readFile)'tcp-loopback-eventing.mjs:165-166:'Baseline for the the removal loop swap'…'treat a regression as a the removal bug'
Code comments:
tcp-loopback-eventing.mjs:39:'same bun script across the the removal migration'composite-test-parallel.mjs:7:'the removal of the removal plan replaces this'composite-test-parallel.mjs:17-18:'UPPER BOUND on the the removal win'…'the the removal REGRESSION GUARD'eventloop-settimeout-chain.mjs:18:'the the removal rewrite must not make this SLOWER'eventloop-timer-precision.mjs:5,37:'gauge for the the removal timer rewrite','a the removal design option'eventloop-worker-pingpong.mjs:20:'regression guard for the the removal wakeup rewire'fswatch-burst.mjs:'(plan the removal)', and comment section markers'// the removal: single-touch delivery latency','// the removal: burst while…'— these were clearly phase/section labelsdns-fs-interference.mjs:'(GetAddrInfoW, plan the removal)','After the removal plan: DNS via GetAddrInfoW on WorkPool (this removal), fs via WorkPool (this removal)'README.mdtier-1 table row 6:'this removal before/after metric'pipes-*.mjs,spawn-*.mjs,asyncfs-singlethread-knee.mjs,composite-startup-resolve.mjs,hrtime-callcost.mjs:'plan the removal','the libuv-removal work the removal', etc.
Why it's unambiguously a find-replace artifact
Doubled articles ('the the removal'), self-referential phrases ('the removal of the removal plan'), and grammatically impossible constructions ('a the removal bug', 'plan the removal' used as a parenthetical noun phrase) cannot have been written by hand. The pattern is consistent: wherever the original text had <token> in a context like 'the <token> rewrite' or 'plan <token>', the substitution produced 'the the removal rewrite' or 'plan the removal'. The fswatch-burst.mjs section-marker comments ('// the removal: single-touch…') make the original shape obvious — they were phase headings.
Impact
Purely cosmetic. bench/ is developer tooling, not shipped runtime code; nothing breaks functionally, no test can fail, no user sees this. But:
- Several instances are in
console.logoutput that developers read when running the benchmarks - The comments are the primary documentation for these scripts' claims and methodology (per the README: 'see script headers')
- CLAUDE.md explicitly calls out 'Audit every hit of bulk find-and-replace' as a review pattern
Step-by-step proof
rg -n 'the the removal' bench/libuv-removal/→ 8+ hits across composite-test-parallel.mjs, eventloop-*.mjs, tcp-loopback-eventing.mjs (impossible doubled article)rg -n 'the removal of the removal' bench/libuv-removal/→ composite-test-parallel.mjs:7 (self-referential)rg -n 'a the removal' bench/libuv-removal/→ tcp-loopback-eventing.mjs:166, eventloop-timer-precision.mjs:37 (article-before-article)- asyncfs-bunfile-vs-readfile.mjs:138 is inside a template literal passed to
console.log→ user-facing at runtime - tcp-loopback-eventing.mjs:165 is inside a template literal passed to
console.log→ user-facing at runtime
Fix
One cleanup pass: rg -l 'the the removal|plan the removal|removal the removal|this removal\)|a the removal|removal of the removal' bench/libuv-removal/ lists every affected file. Rewrite each occurrence to read coherently — the fswatch-burst.mjs section markers and README table cells suggest the original token was a phase/section reference that can simply be dropped or replaced with 'the migration' / 'phase N' as appropriate.
|
Third pass, on the IOCP event loop and AFD poll core, at 3bd989e on Windows Server 2019. This comment is the behavioral half and one confirmed regression your Windows CI is already red on; a second comment with the code-level engine review follows. First: all 10 findings from my two comments above are fixed at 3bd989e. Re-verified each on Windows: the pread-on-NUL minimal repro returns 0, Confirmed regression: an HTTP/3
|
| build | engine | profile | result |
|---|---|---|---|
PR head 3bd989e60 |
new IOCP | debug | 4/4 hang |
merge base 00a93bd93 |
libuv | debug | 4/4 pass, 46-161 ms |
canary 52a1ddf07 (recent main) |
libuv | release | 4/4 pass, 44-298 ms |
| 1.3.14 | libuv | release | 4/4 pass, 59-283 ms |
The merge-base row is the one that matters: same machine, same debug build profile, same test runner, identical command; the only variable is this PR's diff. Your own CI corroborates it at the release tier: http3 adversarial body=65536 > POST /echo (Uint8Array) times out at 90 seconds on all three Windows release test lanes in build 67507. 90 seconds on a release build for a 64 KB loopback echo is a hang, not slowness.
It is also clearly a hang rather than general HTTP/3 breakage. In the full-file run on the same debug build, body=4194304 > 8 concurrent POST /echo (32 MB of HTTP/3 traffic) completes in 1.1 s while a single 64 KB echo hangs for the full 5 s.
Two observations that narrowed the mechanism but did not close it, which I think are the useful handles:
-
It needs the
bun testcontext. The identical operation (Bun.serve({ http3: true, http1: false })plus afetchwithprotocol: "http3"and a 64 KBUint8Arraybody) as a standalonebun script.mjspasses in 33 ms on the PR build, three round-trips in a row. So this is a race the test runner's loop activity happens to trigger, not a dead code path. -
A
ReadableStreambody unsticks it. The file isdescribe.each(sizes)and the tests run sequentially. In the full-file run, only the first two fail:POST /echo (Uint8Array)andPOST /slow-echo(also aUint8Arraybody). The next two,POST /echo via pull ReadableStreamandvia type:direct stream, pass, and after them every subsequentUint8Array-body test passes, including8 concurrent POST /echoat all four sizes. With-t Uint8Array(so noReadableStream-body variants run at all), all four hang. Whatever the client does only on the streaming-body send path (an extra timer arm, an extra loop wakeup, an extra UDP flush) is also what the buffered-body path is waiting for.
I did not find the mechanism in the time I had. What I did rule out: the QUIC deadline fold into the poll timeout exists (src/iocp/usockets.rs:634-641) and its microsecond-to-millisecond conversion is correct; the quic.c per-connection virtual-poll liveness accounting is correct; and it is not a size threshold. Observation 2 smells like a missed first wakeup (the loop blocks before lsquic's first retransmission or ACK deadline is visible to it) rather than anything in the AFD re-arm path, but that is a guess.
Two more Windows CI failures are the same class and worth triaging together: test/js/web/fetch/body-stream.test.ts's body-stream over http/3 describe also times out at 90 s on all three lanes, with a different test failing on each lane, which is exactly what a cold-connection race looks like. And one to cross off your list: test/js/bun/http/serve-http3.test.ts's 8 MB POST body echoes byte-exact also fails on the libuv baseline, so that one is pre-existing and not this PR.
Behavioral sweep
Ran the loop/AFD-facing suites on the PR debug build, with every single failure chased to ground rather than reported as a count:
| suite | pass | fail | disposition |
|---|---|---|---|
test/js/web/timers/ + test/js/bun/net/ |
109 | 3 | 2 are object-count GC assertions that 3 faithful standalone repros on both engines could not reproduce (incl. the exact using path); 1 is a test whose budget the debug build cannot fit (the release baseline takes 54.7 s of a 90 s budget) |
test/js/node/net/ + serve.test.ts + test/js/web/websocket/ |
598 | 2 | both pass in isolation; the failures were interleaved-output and a 5 s budget racing 37 concurrent files on a debug build |
| 4 previously-failing spawn/child_process tests | 4 | 0 | confirms the fixes |
So outside HTTP/3: 707 tests, zero confirmed regressions, zero hangs. A systematic AFD re-arm failure would produce many hangs across Bun.serve and node:net; there were none. The engine is behaviorally solid.
The AFD completion dispatch, read by hand
I read process_poll_req (src/iocp/afd.rs:741-873) and submit_poll_req closely, since a re-arm bug there would be the single worst failure mode in the PR, and it is correct. For the record, because this is the part I would have been most worried about:
- It is libuv's two-slot fast-poll design: a mask change while an IRP is pending submits an exclusive replacement into the other slot (the kernel cancels the first), each with its own pinned
AFD_POLL_INFO, and the suppression-mask algebra (deliver = events_from_afd(afd) & (*h).events & !mask, line 801) prevents the replacement from double-reporting what the cancelled one already did. - Every arm of the dispatch funnels to the same post-callback guard (
!is_closing() && events & !(submitted_1 | submitted_2) != 0, line 868). Cancelled, kicked, and zero-handle completions set no error and no deliver and fall through to that guard, so they re-arm silently. A genuine error disarms before the callback so a restart from inside it cannot spin.AFD_POLL_LOCAL_CLOSEzeroes the subscription so a recycled handle is never re-armed. - There is even a documented, argued deviation from libuv at lines 776-779 (errors dispatch on a zero mask, "epoll parity, deliberate deviation from libuv's
events != 0error gate"), and the unit tests in the same file read like a checklist of the wepoll hazard list (irp_pending_during_callback,restart_inside_callback_no_double_fire,close_with_in_flight_irps_gates_endgame,external_cancel_is_swallowed_and_rearmed,slow_path_select_fallback).
No finding from that read.
|
Found the likely mechanism for the HTTP/3 hang above. It is an ordering bug in the new loop, and the code this PR deleted is the proof. The ordering
pub fn tick(&mut self, timeout_ms: Option<u64>) -> TickResult {
let mut result = TickResult::default();
self.run_hook(self.pre_hook); // 557
...
let mut wait = match (timeout_ms, timer_due) { // 566
(Some(t), Some(d)) => Some(t.min(d)),
...
};
...
result.dispatched = self.poll_once(wait); // 584Line 557's Line 566 then computes the wait from There are two places that do fold the QUIC deadline in, and both are upstream of the recomputation:
Both read the value Why libuv didn't have this bugYour own quic.c diff is the smoking gun. This is what the PR deleted from #ifdef LIBUS_USE_LIBUV
if (have_tick) {
if (!loop->data.quic_timer)
loop->data.quic_timer = us_create_timer(loop, 1, 0);
int ms = min_diff <= 0 ? 1 : (min_diff + 999) / 1000;
us_timer_set(loop->data.quic_timer, us_quic_on_timer, ms, 0);
}
#endifA real timer, armed inside the function that computes the deadline, therefore necessarily after the computation. Correct by construction. The replacement reads the deadline before the function that computes it runs, which is correct only when the value happens to be fresh enough, i.e. only when something else wakes the loop again soon. This explains all three clues at onceEvery observation from the previous comment says the same thing: any extra loop wakeup after the body is sent cures the hang, because the next iteration re-reads a fresh
And it explains why your CI only fails the first one or two tests in each HTTP/3 file, with a different test failing on each of the three Windows lanes: it is a race against the first idle tick after the send, and whichever test's send happens to land on a quiet loop loses. I did not build a fix to prove it, so I am calling this a strong hypothesis rather than confirmed, but it is the only mechanism I found that predicts all three observations, and the deleted timer is an independent reason to believe it. Two fix shapesEither restores the invariant the deleted code had:
If (2), note the POSIX backends would want the same audit: |
| // ERROR_DIRECTORY ("the directory name is invalid") is ENOENT in | ||
| // the contract table (libuv 162e57ba), not ENOTDIR. | ||
| W::DIRECTORY => SystemErrno::ENOENT, |
There was a problem hiding this comment.
🔴 fs.rmdirSync(file) on Windows now returns {code:'ENOTDIR', syscall:'open'} instead of Node/libuv's {code:'ENOENT', syscall:'rmdir'} — the use bun_sys::sys_uv as Syscall → use bun_sys as Syscall change (node_fs.rs:282) means NodeFS::rmdir now resolves to the top-level bun_sys::rmdir (lib.rs:8669 → DeleteFileBun(FILE_DIRECTORY_FILE)), not the libuv-parity port. This PR added the correct port at windows::fs::rmdir (fs.rs:434 → bun_winfs::unlink_rmdir → Win32Error::DIRECTORY) and changed W::DIRECTORY => ENOENT right here for exactly that path — but nothing calls it (windows_impl has no rmdir, and grep confirms zero callers). Matches CI: test-fs-rmdir-throws-on-file.js fails on all three Windows targets. Fix: route bun_sys::rmdir on Windows through windows::fs::rmdir — note that adding it to windows_impl alone won't shadow the explicit top-level fn at lib.rs:8669 (Rust glob-import shadowing rule); either #[cfg(not(windows))]-gate that fn or make its Windows body call w::fs::rmdir.
Extended reasoning...
What the bug is
This PR replaced use bun_sys::sys_uv as Syscall; with use bun_sys as Syscall; on Windows (node_fs.rs:282, now unconditional), so NodeFS::rmdir at node_fs.rs:7166 (#[cfg(windows)] block) now calls bun_sys::rmdir instead of the deleted sys_uv::rmdir. But windows_impl (sys/lib.rs:3593+) has no pub fn rmdir — unlike mkdir, unlink, rename, symlink, readlink, etc. — so pub use windows_impl::* (lib.rs:4285) doesn't shadow the pre-existing top-level pub fn rmdir at lib.rs:8669, which routes through rmdirat(Fd::cwd(), to) → unlinkat_with_flags(.., AT_REMOVEDIR) → DeleteFileBun, NOT the libuv-parity port.
The behavioral difference
DeleteFileBun (windows/mod.rs:3756+) with remove_dir: true calls NtCreateFile(.., FILE_DIRECTORY_FILE, ..). On a regular file that returns STATUS_NOT_A_DIRECTORY → errno_sys(rc, Tag::open) (mod.rs:3829) → translate_ntstatus_to_errno (windows_errno.rs:926: NTSTATUS::NOT_A_DIRECTORY => E::NOTDIR). Result: {code: 'ENOTDIR', syscall: 'open'}.
Before this PR, sys_uv::rmdir (deleted sys_uv.rs:309) → uv_fs_rmdir → libuv's fs__rmdir → fs__unlink_rmdir(is_rmdir=1) checked attributes and returned ERROR_DIRECTORY for a non-directory → UV_ENOENT. Result: {code: 'ENOENT', syscall: 'rmdir'}.
The dead libuv-parity port
This PR did add the correct implementation: windows::fs::rmdir at src/sys/windows/fs.rs:434 → bun_winfs::rmdir_path → unlink_rmdir(.., is_rmdir=true) (winfs/fslnk.rs:602-666), which checks attrs & FILE_ATTRIBUTE_DIRECTORY == 0 and returns Win32Error::DIRECTORY — and this PR also changed the errno-table row W::DIRECTORY => SystemErrno::ENOENT right here (windows_errno.rs:765-767, from ENOTDIR), with the comment "libuv 162e57ba, not ENOTDIR", specifically for that libuv-parity contract. But grep confirms windows::fs::rmdir has zero callers — it is dead code because nothing wires it up.
Step-by-step proof
- Windows:
fs.rmdirSync('some-file.txt')where the target is a regular file. NodeFS::rmdir(node_fs.rs:7164),args.recursiveis false → line 7166:Syscall::rmdir(path).Syscall=bun_sys(this PR, node_fs.rs:282).bun_sys::rmdir—windows_impl(sys/lib.rs:3593+) exports open/close/read/write/pread/pwrite/stat/fstat/lstat/mkdir/unlink/rename/symlink/readlink/fchmod/fchown/ftruncate/chmod/chown/link/fsync/fdatasync/openat/dup, but not rmdir — sopub use windows_impl::*at lib.rs:4285 doesn't shadow the top-levelpub fn rmdirat lib.rs:8669.- lib.rs:8669:
rmdirat(Fd::cwd(), to)→ dir.rs:306-308:unlinkat_with_flags(dirfd, path, AT_REMOVEDIR). - lib.rs:3771 (
windows_impl::unlinkat_with_flags):DeleteFileBun(wpath, DeleteFileOptions{remove_dir: true, ..}). - windows/mod.rs:3757-3758:
remove_dir=true→create_options_flags = FILE_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT. - windows/mod.rs:3800-3813:
NtCreateFile(.., FILE_DIRECTORY_FILE, ..)on a regular file →STATUS_NOT_A_DIRECTORY(0xC0000103). - windows/mod.rs:3829:
errno_sys(rc, Tag::open)→translate_nt_status_to_errno(NOT_A_DIRECTORY)→ windows_errno.rs:926:E::NOTDIR. - JS observes
{code: 'ENOTDIR', syscall: 'open'}; Node's test asserts{code: 'ENOENT'}on Windows → fails.
Impact / CI match
This is a user-visible Node.js compat regression on Windows. test/js/node/test/parallel/test-fs-rmdir-throws-on-file.js:9 — const code = common.isWindows ? 'ENOENT' : 'ENOTDIR'; — fails with code 1 on all three Windows targets in this PR's robobun CI comment (2019 x64, x64-baseline, 11 aarch64) and passes on every POSIX target. User code that catches err.code === 'ENOENT' for rmdir-on-a-file (the standard Node idiom) breaks.
Fix
Wire bun_sys::rmdir on Windows through the parity port. Two workable shapes:
#[cfg(not(windows))]-gate the top-levelpub fn rmdirat lib.rs:8669, and addpub fn rmdir(path: &ZStr) -> Maybe<()> { w::fs::rmdir(path) }towindows_implalongsidemkdir/unlink. (Note: adding it towindows_implalone is insufficient — Rust glob imports do not shadow explicitly-defined items in the same module, so the top-level fn at :8669 would still win.)- Or change the top-level fn's Windows body to call
crate::windows::fs::rmdir(to)directly.
Either way, this also fixes the syscall tag (rmdir instead of open) and makes the port at fs.rs:434 — and the W::DIRECTORY => ENOENT change right here — actually reachable.
(This comment is anchored at windows_errno.rs:765-767 — where this PR changed W::DIRECTORY from ENOTDIR to ENOENT for exactly this libuv-parity contract that's never reached — because sys/lib.rs and node_fs.rs have no diff line ranges in the tracker; the actual fix goes in src/sys/lib.rs.)
There was a problem hiding this comment.
The ENOTDIR→ENOENT remap at node_fs.rs:7171-7173 (7f8ca2f) fixes err.code, so test-fs-rmdir-throws-on-file.js now passes — but two of the three things this comment named are still open:
err.syscallis still'open'. The remap only rewriteserr.errno;err.syscallstill comes fromDeleteFileBun'serrno_sys(rc, Tag::open)(mod.rs:3838), and.with_path()preserves it (Error.rs:197). Node/libuv reportsyscall: 'rmdir'here.windows::fs::rmdir(fs.rs:434) is still dead code — grep confirms zero callers ofbun_winfs::rmdir_pathoutside winfs's own tests. It carriesTag::rmdirand producesWin32Error::DIRECTORY→ theW::DIRECTORY => ENOENTrow right here, so wiring it up fixes both the tag and the code without a caller-side patch.
Changing the top-level pub fn rmdir at lib.rs:8669 to call crate::windows::fs::rmdir(to) on #[cfg(windows)] (the second option in the original comment) gets both, and the node_fs.rs remap can then go — otherwise it's a per-caller patch that any other Windows bun_sys::rmdir caller misses.
| /// Gate for positioned (pread/pwrite-style) I/O. Seekable kinds | ||
| /// Every kind gets a ticket: the kernel ignores the OVERLAPPED offset | ||
| /// on non-seekable handles and reads/writes sequentially — libuv's | ||
| /// fs__read outcome. Explicit seeks still reject in `seek()`. | ||
| /// // quirk: FSIO-21 |
There was a problem hiding this comment.
🟡 The fix commit f1fec2c rewrote this doc but left the last two words of the old first line in place — 'Seekable kinds' now dangles with no verb before 'Every kind gets a ticket:'. Line 559 should end after 'I/O.' (the old sentence continued '…(File, Directory) get a ticket'; the replacement started at line 560).
Extended reasoning...
What the bug is
The doc comment on FdTable::positioned_io at src/fdtable/table.rs:559-563 currently reads:
/// Gate for positioned (pread/pwrite-style) I/O. Seekable kinds
/// Every kind gets a ticket: the kernel ignores the OVERLAPPED offset
/// on non-seekable handles and reads/writes sequentially — libuv's
/// fs__read outcome. Explicit seeks still reject in `seek()`.
Rendered: 'Gate for positioned (pread/pwrite-style) I/O. Seekable kinds Every kind gets a ticket: …' — 'Seekable kinds' is a two-word noun phrase with no verb, spliced onto the following sentence.
The specific code path
This is a fix-commit editing artifact. Before f1fec2c the comment was:
/// Gate for positioned (pread/pwrite-style) I/O. Seekable kinds
/// (`File`, `Directory`) get a ticket — positioned ops never touch the
/// fd's logical `pos`. Sequential-only kinds (`Pipe`, `Char`, `Tty`)
/// are the raw ESPIPE shape, ...
git show f1fec2ca -- src/fdtable/table.rs confirms line 559 is unchanged context (leading space in the diff), while the following lines were removed and replaced with the new 'Every kind gets a ticket: …' body. The first line's trailing 'Seekable kinds' was orphaned from its old '(File, Directory) get a ticket' continuation.
Why existing findings don't cover it
This is distinct from the two stale-comment findings already posted on this PR — the one at loop_data.h:59 (deleted-mechanism references: quic_timer/uv_loop_t/CLAUDE.md) and the one at util.rs:1068 (renamed-identifier references: makeLibUVOwned/.uv()). Those are about identifiers/mechanisms this PR removed or renamed; this is a sentence-splice introduced by the fix commit itself when rewriting a doc block.
Step-by-step proof
- Read src/fdtable/table.rs:559 →
/// Gate for positioned (pread/pwrite-style) I/O. Seekable kinds. - Read :560 →
/// Every kind gets a ticket: the kernel ignores the OVERLAPPED offset. - Concatenate as rustdoc will render it: '…I/O. Seekable kinds Every kind gets a ticket:…'. 'Seekable kinds' has no verb; 'Every kind' begins a new sentence.
git show f1fec2ca -- src/fdtable/table.rs: line 559 is a context line (unchanged); lines 560-565 are removed (the old '(File,Directory) get a ticket…' continuation); lines 560-563 are added (the new body). The old sentence's subject was left behind when its predicate was replaced.
Impact
Zero runtime effect. This is a pub fn's rustdoc on a new file, so it renders in the crate docs, but it's purely cosmetic — the second sentence still conveys the correct behavior.
Fix
Delete 'Seekable kinds' from the end of line 559 so it reads /// Gate for positioned (pread/pwrite-style) I/O. — the following 'Every kind gets a ticket:' sentence stands on its own.
|
Fourth pass: the fd table and the synchronous filesystem layer ( Method: I took the set of test files your Windows release CI lanes fail at both 67443 and 67507 (so they are stable, not flaky), ran every fs-layer one on my debug PR build, and A/B'd each against a libuv baseline (the canary, 1.
|
| suite | pass | fail | disposition |
|---|---|---|---|
test/js/node/fs/ + test/js/bun/io/ + test/js/bun/glob/ |
785 | 13 | 2 are findings 1 and 3; 11 are timeouts |
All 11 timeouts are recursive glob/readdir over a 2792-package node_modules on a debug build (e.g. readdirSync({recursive}) x 100 takes 36 s against a 10 s budget). None of them is on your release CI lanes, so I am confident they are debug slowness, not hangs. One thing I could not do cleanly: a release-build readdir performance comparison against libuv. The recursive glob tests pass on the release CI, so there is no evidence of a perf regression, but readdir is exactly the NtQueryDirectoryFile rewrite and I wanted to note that I did not measure it on release.
Still untriaged from your Windows CI set, for completeness: test-fs-rmdir-throws-on-file.js and test-fs-promises-writefile.js both passed on my debug build despite failing on the release lanes, so those two are release-specific or timing-sensitive and I could not reproduce them; and symbols.test.ts, uv.test.ts, uv_stub.test.ts, test-os-process-priority.js, 26411.test.ts, markdown-entrypoint.test.ts, test-child-process-detached.js, test-stdin-pause-resume.js, bun-pack.test.ts are outside the fs scope (the uv ABI stubs, tty, and one install test) and I have not looked at them.
…sh/bun into claude/busy-vaughan-130bdf
| Source::Tty(tty) => { | ||
| // WTF-8 → UTF-16 here (the engine takes UTF-16 and owns EOL | ||
| // normalization, chunking and the cross-write surrogate | ||
| // carry). Lone surrogates pass through so a pair split | ||
| // across two writes is still joined. // quirk: TTY-12, TTY-13 | ||
| let mut units: Vec<u16> = vec![0; buf.len().max(1)]; | ||
| let n = | ||
| match bun_core::strings::try_convert_utf8_to_utf16_in_buffer(&mut units, buf) { | ||
| Some(out) => out.len(), | ||
| // UTF-16 unit count never exceeds the WTF-8 byte count. | ||
| None => unreachable!("utf16 output larger than utf8 input"), | ||
| }; | ||
| // SAFETY: forwarded fn contract; `units` is consumed | ||
| // synchronously inside `write` (engine contract). | ||
| unsafe { | ||
| Self::tty_mut(tty) | ||
| .handle | ||
| .write(&units[..n], Some(tty_cb), data) | ||
| } |
There was a problem hiding this comment.
🔴 🔴 The Tty arm of Source::stream_write converts WTF-8→UTF-16 statelessly per call via try_convert_utf8_to_utf16_in_buffer — a UTF-8 sequence byte-split across two process.stdout.write() calls (e.g. [F0 9F 98] then [80] for 😀) emits U+FFFD instead of rejoining. This is a regression from pre-PR libuv, whose uv__tty_write_bufs carried per-handle utf8_bytes_left/utf8_codepoint state; the repo's own ledger TTY-12 (tty.md:87-92) marks it must-port ('stateful streaming UTF-8→UTF-16 across writes') and cites libuv 3016fbc4 which permanently DELETED bulk conversion for exactly this reason (libuv#1965 / JuliaLang/julia#27267). status.toml:2241 marks TTY-12 'implemented' citing write_transform_eol_chunk_surrogate_kats — but that test exercises transform_units(&[u16]), one layer below where the byte-split happens. Fix: add per-TtySource UTF-8 partial-byte carry (stash the trailing incomplete-sequence bytes and prepend to the next call), or push the byte-level decode into the engine alongside pending_high_surrogate.
Extended reasoning...
What the bug is
Source::stream_write's Tty arm at src/io/source.rs:475-493 converts each write's bytes to UTF-16 with a stateless bulk converter:
Source::Tty(tty) => {
// WTF-8 → UTF-16 here (the engine takes UTF-16 and owns EOL
// normalization, chunking and the cross-write surrogate
// carry). Lone surrogates pass through so a pair split
// across two writes is still joined. // quirk: TTY-12, TTY-13
let mut units: Vec<u16> = vec[0; buf.len().max(1)];
let n = match bun_core::strings::try_convert_utf8_to_utf16_in_buffer(&mut units, buf) {
...try_convert_utf8_to_utf16_in_buffer (src/bun_core/string/immutable.rs:2951) is bulk simdutf; on invalid UTF-8 it falls back to a scalar WTF-8 decoder whose decode_wtf8_one returns (0xFFFD, 1) for a truncated 4-byte lead (immutable.rs:3022-3023: if s.len() < 4 { return (0xFFFD, 1); }). No cross-call state is carried anywhere — grep for utf8_bytes_left|utf8_codepoint|pending_utf8|utf8_partial in src/iocp and src/io returns zero hits.
The comment at :478-479 ('Lone surrogates pass through so a pair split across two writes is still joined') describes only the WTF-8-encoded-lone-surrogate case (ED A0 xx / ED Bx xx as complete 3-byte sequences), not the byte-split case TTY-12 is about.
Why the engine's pending_high_surrogate doesn't help
TtyHandle at src/iocp/tty.rs:1326-1328 carries only pending_high_surrogate: u16 and previous_eol: u16, and transform_units (tty.rs:1150) takes &[u16] — it operates on already-converted UTF-16. That surrogate carry only engages when the CONVERTER outputs a lone high surrogate in call N and a lone low in call N+1. But for a byte-truncated F0 xx xx prefix, decode_wtf8_one emits U+FFFD (a BMP codepoint), never a lone high surrogate — so the carry never engages for the byte-split case.
The engine's own doc at tty.rs:14-22 explicitly says 'libuv's byte-at-a-time UTF-8 decoder (its TTY-12 state machine) is therefore not ported — its cross-write-statefulness requirement survives as the pending-high-surrogate carry'. That design claim is factually wrong for byte-splits.
Why this is a regression from pre-PR libuv
Before this PR, WindowsBufferedWriter/WindowsStreamingWriter cast the tty source to uv_stream_t* and called uv_write. For a uv_tty_t that routes to uv_tty_write → uv__tty_write_bufs, libuv's stateful per-byte UTF-8 decoder with utf8_bytes_left/utf8_codepoint state (src/jsc/bindings/libuv/uv/win.h:523-524, and per the ledger 'tty.c:1716-1717, 1734-1784, saved back at 2184-2187'). So a byte-split sequence rejoined correctly.
The repo's own quirk ledger TTY-12 (src/sys/windows/quirks/tty.md:87-92) documents this as must-port: 'the requirement: stateful streaming UTF-8→UTF-16 across writes'. The history section (tty.md:91) says: '58ccfd4c → 8cbabaa8 revert → 445e3a1f + c2f0e4f6 → final shape 3016fbc4 (libuv#1965, JuliaLang/julia#27267)' — 58ccfd4c added bulk conversion, 8cbabaa8 reverted it, and 3016fbc4 permanently DELETED the bulk fast path in favor of the incremental decoder even for VT-passthrough consoles. This PR reintroduces exactly the shape libuv reverted.
Why status.toml is wrong
status.toml:2241-2243 marks TTY-12 status = 'implemented' with test write_transform_eol_chunk_surrogate_kats. But that test exercises transform_units(&[u16]) — it takes UTF-16 units as input. It CANNOT test UTF-8 byte-splits, because the byte→u16 conversion happens one layer up in source.rs, before transform_units sees anything.
Step-by-step proof (concrete repro, Windows real console with process.stdout.isTTY)
process.stdout.write(Buffer.from([0xF0, 0x9F, 0x98])); // first 3 bytes of 😀 U+1F600
process.stdout.write(Buffer.from([0x80])); // last byteCall 1, buf = [F0 9F 98]:
- simdutf fails (invalid — truncated 4-byte sequence).
- Scalar fallback
decode_wtf8_one([F0 9F 98]): b0=0xF0 ≥ 0xF0,s.len() == 3 < 4→ returns(0xFFFD, 1). - Advance 1;
decode_wtf8_one([9F 98]): b0=0x9F < 0xC0 (bare continuation) →(0xFFFD, 1). - Advance 1;
decode_wtf8_one([98]): same →(0xFFFD, 1). - Output:
[0xFFFD, 0xFFFD, 0xFFFD]→ three � on the console.
Call 2, buf = [80]:
6. decode_wtf8_one([80]): b0=0x80 < 0xC0 → (0xFFFD, 1) → one more �.
Result: four U+FFFD instead of 😀. Pre-PR / libuv / Node: prints 😀 (libuv's utf8_bytes_left=1, utf8_codepoint=0x1F60 state carries across the two uv_write calls).
Impact / reachability
Realistic trigger: fs.createReadStream(utf8file).pipe(process.stdout) on a real Windows console where a highWaterMark chunk boundary lands mid-character — JuliaLang/julia#27267's exact scenario. Also any tool streaming arbitrary UTF-8 chunks (test reporters, loggers, CLI progress with emoji/CJK). process.stdout on Windows is force-sync (Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio, per FileSink.rs and bench/libuv-removal/stdout-throughput.mjs header), so each JS .write() reaches stream_write as a separate call — the buffered writer does not coalesce them. Callers pass raw byte buffers with no UTF-8 boundary awareness (PipeWriter.rs:1601 buffered, :2150 streaming).
Scoped to real-console output only (isTTY=true — pipes/files use the zero-copy Pipe/File arms, not this path). But it directly contradicts the PR description's 'Suites at parity or better on Windows: … tty' claim and the repo's own must-port ledger.
Fix
Two shapes; either works:
(a) Add per-TtySource UTF-8 partial-byte state in source.rs — stash the trailing incomplete-sequence bytes from each buf (find the last complete-sequence boundary via a backward scan of the last ≤3 bytes), prepend them to the next call's buf before conversion. Small, local to source.rs.
(b) Change TtyHandle::write to accept &[u8] and add utf8_bytes_left: u8 / utf8_codepoint: u32 state alongside pending_high_surrogate/previous_eol — libuv's shape, and what the ledger recommends ('Bun's Rust strings layer should supply the decoder').
Either way, add a KAT that feeds [F0 9F 98] + [80] through the byte-level entry point (not transform_units), and update status.toml's TTY-12 test citation.
| //! Node-visible uv errno protocol numbers. JS sees these in `err.errno` | ||
| //! (e.g. ENOENT = -4058 on Windows), so the NUMBERS are ABI — they survive | ||
| //! the libuv removal as pure protocol constants, owned here. | ||
|
|
There was a problem hiding this comment.
🟡 Two more uv-posix-polyfills.c parity nits (same class as the setpriority/cpus/homedir/process.title findings at this anchor): (1) uv_getrusage (:527-562) computes CPU time via FileTimeToSystemTime + wHour*3600 + wMinute*60 + wSecond, wrapping at 24h — the comment at :529-530 ('CPU time wraps at 24h upstream too') reads as intentional-for-parity, but the ledger OS-37 (util-os.md:265, added in this PR) says 'FIX the conversion (u64 QuadPart/10 → µs; no SYSTEMTIME) — do not inherit the 24h wrap'; either apply the trivial QuadPart / 10000000 fix or add // quirk: OS-37 so readers don't cargo-cult the wrap. (2) uv_kill (:341) hardcodes signum >= 32 /* NSIG */, but NSIG in scope (uv/win.h:95) is SIGWINCH+1 = 29, matching libuv and the Rust engine's kill_raw (iocp/process.rs:1683 with (0..NSIG), signal.rs:79 unit-tested at :665) — so signums 29-31 return UV_ENOSYS (falls to default: at :376) where libuv and Bun's own process.kill() return UV_EINVAL, and the PR description's 'uv_kill-polyfill signal semantics match the engine' is false for those values; one-token fix: 32 → NSIG. (Anchored here because uv-posix-polyfills.c has no diff hunk ranges; actual code at :527-562 and :341.)
Extended reasoning...
What these are
Two more small libuv-parity divergences in the hand-written Windows uv-posix-polyfills.c, of the same class as the setpriority (band-edge)/os.cpus (ProcessorNameString)/os.homedir (GetLastError)/process.title (cache-on-fail) findings already anchored at this file. Both are non-blocking; both are one-token or few-token fixes.
(1) uv_getrusage 24h CPU-time wrap — comment contradicts ledger OS-37
Code path. src/jsc/bindings/uv-posix-polyfills.c:527-562 implements the Windows uv_getrusage by calling GetProcessTimes, then FileTimeToSystemTime on both kernel and user times, then computing tv_sec = wHour*3600 + wMinute*60 + wSecond (:549, :552). SYSTEMTIME.wHour is 0-23 and excess days go into wDay, which is ignored — so a process with >24h accumulated CPU time reports time mod 24h. Both process.cpuUsage() and process.resourceUsage() route through this on Windows (BunProcess.cpp:3435, :3478 — the #else arm of #if !OS(WINDOWS)).
The comment/ledger contradiction. The comment at :529-530 reads "including its FileTimeToSystemTime conversion (CPU time wraps at 24h upstream too)" — framing the wrap as deliberate libuv parity. But the repo's own quirk ledger, added in this same PR, says the opposite at OS-37 (src/sys/windows/quirks/util-os.md:260-265): "must-port the field mapping/units for Node parity, but FIX the conversion (u64 QuadPart/10 → µs; no SYSTEMTIME) — do not inherit the 24h wrap. Target: process.resourceUsage / process.cpuUsage". OS-38 (getrusage_thread) has the identical disposition. status.toml:2534 tracks OS-37 as pending, so the PR's own tracking mechanism knows this is a TODO — the code comment just doesn't say so.
Why it's a nit. Not a regression: pre-PR real libuv (win/util.c:906-914) had the identical FileTimeToSystemTime bug, so a Windows process with >24h CPU time saw the same wrong answer before and after this PR. Reachability requires >24h accumulated CPU on a single Windows process. The actionable ask is small: since the PR is hand-writing this polyfill fresh anyway, either apply the ~4-line ledger-prescribed fix (ULARGE_INTEGER u = {.LowPart = user_time.dwLowDateTime, .HighPart = user_time.dwHighDateTime}; ru_utime.tv_sec = u.QuadPart / 10000000; ru_utime.tv_usec = (u.QuadPart % 10000000) / 10;, dropping both FileTimeToSystemTime calls), or change the comment to // quirk: OS-37 (pending) so the next reader doesn't cargo-cult the wrap as a parity requirement. Other quirk-tracked code in this PR uses exactly that // quirk: XX-NN marker convention.
(2) uv_kill NSIG=32 vs 29 — polyfill disagrees with the engine
Code path. src/jsc/bindings/uv-posix-polyfills.c:341 (inside the #elif OS(WINDOWS) block) has:
if (signum < 0 || signum >= 32 /* NSIG */)
return UV_EINVAL;But NSIG in scope (via #include <uv.h> → this repo's stub uv/win.h:88,95) is SIGWINCH + 1 = 29, with an #undef guard against the CRT's NSIG=23. And the Rust engine's kill_raw at src/iocp/process.rs:1683 uses if !(0..NSIG).contains(&signum) → KillError::InvalidSignal, with NSIG = SIGWINCH + 1 = 29 at src/iocp/signal.rs:79 and a unit test at signal.rs:665 asserting NSIG == 29. The polyfill's switch at :344-378 handles 0/SIGINT/SIGQUIT/SIGKILL/SIGTERM and default: at :376-377 returns UV_ENOSYS.
Step-by-step for signum=29. An N-API addon calls uv_kill(pid, 29). Line :341: 29 >= 32 is false → passes the range check. switch(29) matches no named case → default: → return UV_ENOSYS. Under real libuv (which uses NSIG=29 from the same uv/win.h) and under Bun's own process.kill(pid, 29) (→ kill_raw → !(0..29).contains(&29) → true → InvalidSignal → EINVAL), the same input returns UV_EINVAL. So the PR's two kill implementations disagree with each other on signums 29-31, and the polyfill diverges from the libuv its own :335 comment claims to port ('Port of libuv win/process.c uv_kill semantics'). The PR description's Compatibility Notes line "uv_kill-polyfill signal semantics match the engine" is false for these three values.
Why it's a nit. The /* NSIG */ comment right next to the literal 32 shows the author intended NSIG — this is a transcription slip (32 is the common POSIX value; libuv's Windows NSIG is 29). Reachability is very narrow: only N-API addons calling uv_kill with raw signums 29-31, which correspond to no named signal on any platform, and the observable difference is ENOSYS vs EINVAL for meaningless values. But it IS a genuine self-inconsistency between two implementations landed in the same PR, documented in the ledger at PROC-51 (process.md:357-362: '890eedaf (#1642) added EINVAL range checking (was ENOSYS for everything)'). One-token fix: 32 → NSIG (already in scope from <uv.h>).
Fix
- :529-530: either apply the OS-37 QuadPart conversion, or reword the comment to
// quirk: OS-37 (24h wrap — pending fix). - :341:
signum >= 32→signum >= NSIG.
(Anchored at src/errno/uv_numbers.rs because uv-posix-polyfills.c has no diff hunk ranges in the tracker; the actual code is at :527-562 and :341.)
…30bdf # Conflicts: # src/io/PipeReader.rs # src/js/builtins/ReadableStreamInternals.ts # src/jsc/bindings/BunProcess.cpp # src/runtime/node/node_fs.rs # src/runtime/node/win_watcher.rs
The merge auto-renamed EventType to WatchEventKind in imports and the emit() signature but the inline call site kept by the conflict resolution still used the old name.
…30bdf # Conflicts: # src/io/PipeWriter.rs # src/runtime/node/node_fs.rs # test/js/node/fs/fs.test.ts # test/napi/uv.test.ts
Three doc comments that still referenced the libuv plumbing after the merge: - node_process.rs: hasTitle doc mentioned uv_get_process_title - path_watcher.rs: orphaned Windows stub section header at EOF - repository.rs: uv_loop_t* reference in the spawn comment
| if (delay < 1) { | ||
| #ifdef LIBUS_USE_LIBUV | ||
| return -4071; //UV_EINVAL; | ||
| #ifdef _WIN32 | ||
| return WSAEINVAL; | ||
| #else | ||
| //TODO: revisit this when IOCP loop is implemented without libuv here | ||
| return 4071; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🟡 The inner #ifdef _WIN32 at :597 sits inside the #else arm of #ifndef _WIN32 (:550/:584), so _WIN32 is always defined here and the #else return 4071; at :599-601 is unreachable dead code. Before this PR the inner check was #ifdef LIBUS_USE_LIBUV (a distinct macro, with a "revisit when IOCP loop is implemented without libuv" TODO); the mechanical LIBUS_USE_LIBUV→_WIN32 replacement made it tautological. Zero runtime impact — flatten :597-601 to just return WSAEINVAL;.
Extended reasoning...
What the bug is
The PR mechanically replaced #ifdef LIBUS_USE_LIBUV with #ifdef _WIN32 across packages/bun-usockets/src/bsd.c. At bsd_socket_keepalive, this produced a nested #ifdef _WIN32 inside a block that only compiles when _WIN32 is already defined, making the inner #else branch dead code.
The preprocessor structure
548 int bsd_socket_keepalive(...) {
550 #ifndef _WIN32
... POSIX arm ...
584 #else // ← _WIN32 IS defined from here to :612
...
596 if (delay < 1) {
597 #ifdef _WIN32 // ← always true here
598 return WSAEINVAL;
599 #else // ← unreachable
600 return 4071;
601 #endif
602 }
...
612 #endifLine 550 opens #ifndef _WIN32; line 584 is its #else; line 612 closes it. Lines 585-611 compile only when _WIN32 is defined. So the inner #ifdef _WIN32 at :597 is tautologically true, and lines 599-601 (#else return 4071; #endif) can never compile into any target.
Why this PR made it dead
Before this PR, the inner check was #ifdef LIBUS_USE_LIBUV — an orthogonal macro. On Windows one could theoretically build with _WIN32 defined but LIBUS_USE_LIBUV undefined (a hypothetical non-libuv Windows build), so the #else return 4071; arm was reachable in principle, and it carried a //TODO: revisit when IOCP loop is implemented without libuv here comment naming exactly that case. This PR is that IOCP loop, so the TODO is now moot — but instead of collapsing the branch, the mechanical LIBUS_USE_LIBUV→_WIN32 replacement turned it into a nested check on the same macro as its enclosing gate.
Step-by-step proof
- Line 550:
#ifndef _WIN32— the POSIX arm. - Line 584:
#elseof that#ifndef— so for lines 585-611,_WIN32is defined by construction. - Line 597:
#ifdef _WIN32— inside the block from step 2, this is always true. - Line 598:
return WSAEINVAL;always compiles. - Lines 599-601:
#else return 4071; #endif— the#elseof an always-true condition. Never compiled, on any target.
Impact
Zero runtime impact. Windows correctly returns WSAEINVAL (10022) for delay < 1; POSIX takes the outer #ifndef _WIN32 arm and never reaches this code. The 4071 value (which was -UV_EINVAL stripped of its sign — itself a leftover oddity) is simply dead. This is purely a "delete dead code in the same PR that makes it dead" cleanup per CLAUDE.md.
Fix
Flatten lines 596-602 to:
if (delay < 1) {
return WSAEINVAL;
}deleting the inner #ifdef _WIN32 / #else / #endif scaffolding and the dead return 4071;.
|
Seventh and final pass: the remaining engine subsystems nobody had looked at (tty.rs 2491 lines, signal.rs 1532, fsevent.rs 1953, the DNS path, and the Downgraded from MAJOR to a hardening note: two more
|
| // `uv::UV_E*` protocol numbers live in `crate::uv_numbers` (node ABI); | ||
| // `Win32Error` / `NTSTATUS` / the NTSTATUS→errno mapper live locally in this | ||
| // module (their only external use is via `SystemErrno::init`, defined here). | ||
| pub use self::windows::{NTSTATUS, Win32Error, Win32ErrorExt}; | ||
| use bun_libuv_sys as uv; | ||
| use crate::uv_numbers as uv; |
There was a problem hiding this comment.
🟡 Addendum to the stale-libuv-comments cleanup (same class as the loop_data.h / util.rs:1068 / MiniEventLoop.rs threads) — six more bun_libuv_sys/libuv_sys references the HEAD cleanup commit 9d7baee missed, all naming the crate this PR deletes: src/sys/lib.rs:7824-7827 ('Route through bun_libuv_sys (not bun_windows_sys::ws2_32)' — line 7829 now does pub use bun_windows_sys::ws2_32::{...}, so the comment states the OPPOSITE of the code), :8090 ('ws2_32 via bun_libuv_sys'), :8264 ('[u8; 4] in bun_libuv_sys::sockaddr_in'), src/windows_sys/externs.rs:3 ('depends on nothing above libuv_sys'), :76-78 ('shared by bun_libuv_sys (uv/win.h embeds)… libuv embeds them in uv_req_s/uv_tty_s'), :1191-1193 ('must not depend on bun_libuv_sys'). Zero runtime impact. rg 'bun_libuv_sys|libuv_sys' src/ -g '*.rs' finds all six.
Extended reasoning...
What this is
Same stale-comment cleanup class as the three open threads on this PR (loop_data.h:59 / util.rs:1068 / MiniEventLoop.rs:486), at six additional locations that HEAD commit 9d7baee ('Clean up stale libuv references in doc comments') did not touch — git show --stat 9d7baee2 -- src/sys/lib.rs src/windows_sys/externs.rs is empty. All six reference the bun_libuv_sys crate this PR deletes from the workspace (Cargo.toml removes it from members; src/libuv_sys/ is gone; this PR's own windows_errno.rs:10 was updated bun_libuv_sys as uv → crate::uv_numbers as uv).
The six locations
src/sys/lib.rs:7824-7827 — the strongest one:
// Route through `bun_libuv_sys` (not `bun_windows_sys::ws2_32`) so types
// returned by libuv APIs (`uv_interface_address_t`, `uv_udp_*`) are the
// *same* nominal type callers see via `bun_sys::posix::sockaddr_*` — Rust
// doesn't structurally unify two identical-layout `#[repr(C)]` structs.
#[cfg(windows)]
pub use bun_windows_sys::ws2_32::{sockaddr, sockaddr_in, sockaddr_in6, sockaddr_storage};The comment says "route through X (not Y)"; the very next line routes through Y. It states the OPPOSITE of what the code does.
src/sys/lib.rs:8090 — 'ws2_32 via bun_libuv_sys on Windows'. Now via ws2_32 directly.
src/sys/lib.rs:8264-8265 — '[u8; 4] in bun_libuv_sys::sockaddr_in; reinterpret as raw octets so both shapes resolve'. That type no longer exists; ws2_32's sin_addr is in_addr { s_addr: u32 } on both POSIX and Windows now, so the 'both shapes' rationale is gone too.
src/windows_sys/externs.rs:3 — file-level doc: 'This crate is a tier-0 leaf: it depends on nothing above libuv_sys'. The crate no longer exists.
src/windows_sys/externs.rs:76-78 — 'Win32 POD structs shared by bun_libuv_sys (uv/win.h embeds)… All derive Clone+Copy: libuv embeds them in uv_req_s/uv_tty_s/'. The stated rationale for the Copy derives references deleted types.
src/windows_sys/externs.rs:1191-1193 — 'bun.windows.libuv is exposed from the higher-tier bun_sys::windows module… must not depend on bun_libuv_sys (would invert the tier ordering)'. Nothing left to invert.
Why this PR made them stale
Both files WERE modified by this PR (commit 1bebf7e 'Remove libuv on Windows': sys/lib.rs 512 lines changed, windows_sys/externs.rs 1773 insertions) — the changed-files list shows both. Line 7829 (pub use bun_windows_sys::ws2_32::{sockaddr, ...}) was introduced by that commit; the four-line comment above it was left as unchanged context.
Not a duplicate
The three open stale-comment threads cover DIFFERENT locations: loop_data.h:59 covers loop_data.h/AnyEventLoop.rs/CLAUDE.md; util.rs:1068 covers util.rs:926/Listener.rs/socket_body.rs (renamed identifiers: makeLibUVOwned/.uv()); MiniEventLoop.rs:486 covers a stale invariant guard. None of those six files overlap with sys/lib.rs or windows_sys/externs.rs. Same cleanup CLASS, distinct instances.
Step-by-step proof
rg 'bun_libuv_sys|libuv_sys' src/ -g '*.rs'→ exactly six hits at sys/lib.rs:7824,8090,8264 and windows_sys/externs.rs:3,76,1193.ls src/libuv_sys/→ No such file or directory (deleted).grep bun_libuv_sys Cargo.toml→ no matches (removed from workspace members and[workspace.dependencies]).git show --stat 9d7baee2 -- src/sys/lib.rs src/windows_sys/externs.rs→ empty (the cleanup commit didn't touch either file).- Read src/sys/lib.rs:7824-7829: comment at :7824 says 'Route through
bun_libuv_sys(notbun_windows_sys::ws2_32)'; :7829 doespub use bun_windows_sys::ws2_32::{...}. Comment ⟂ code.
Impact
Zero runtime impact — pure comment staleness. Per CLAUDE.md 'Comments carry only durable non-obvious content' and 'Delete dead code in the same PR that makes it dead', these belong in the same cleanup pass as the other three stale-comment threads. The sys/lib.rs:7824 one actively misleads (a reader following it would look for a nominal-type-unification constraint that no longer exists).
Fix
Rewrite or delete each. The sys/lib.rs:7824-7827 comment can drop entirely (or become 'ws2def.h layouts; nominal identity matters for callers that reborrow through raw-pointer casts' — matching the cares_sys/lib.rs comment this PR added). :8090 → 'ws2_32 on Windows'. :8264-8265 can drop entirely (both platforms are in_addr { s_addr: u32 } now). externs.rs:3 → 'depends on nothing' or drop the clause. externs.rs:76-78 → the Clone+Copy rationale can just say 'embedded by-value in engine structs'. externs.rs:1191-1193 can drop entirely.
(Anchored at src/errno/windows_errno.rs:9-13 — where this PR did the bun_libuv_sys as uv → crate::uv_numbers as uv migration — because sys/lib.rs and windows_sys/externs.rs have no diff hunk ranges in the tracker.)
…rror report - source.rs Bun__Tty__getWindowSize: report dwSize.X for width (the buffer width wrapping happens at), matching TtyHandle::get_winsize and Node/libuv (quirk TTY-48). Previously reported the viewport width. - fsevent.rs: a FILE_NOTIFY_INFORMATION record with an odd or oversized FileNameLength is malformed input, not the zero-length marker for a change on the watched directory itself. Abort the batch instead of setting dir_event. - fsevent.rs two_call_wide: set INSUFFICIENT_BUFFER before returning None on the contract-violation exit so callers reading Win32Error::get() receive a deterministic value instead of stale thread state.
Make it structurally impossible for Rust code to call a ws2_32 function before Winsock is initialized, while preserving lazy init. libuv's WSAStartup ran at loop creation, and every process created a loop at startup, so no ws2_32 call could precede init. The engine rewrite replaced that with lazy init at the first WSA consumer, which is a startup win for network-free invocations but created a whack-a-mole: every first-use site had to remember to call ensure_winsock(). Three separate review passes found three separate sites that forgot (bsd_create_listen_socket, LibcBackend::run, c-ares Channel::init). The raw ws2_32 externs move into a private `mod raw` inside `bun_windows_sys::ws2_32`. Every public ws2_32 function is now an #[inline] wrapper that calls ensure_winsock() first, so a caller cannot reach Winsock without the gate firing. The gate is a single acquire load after first use. WSAStartup is refcounted (multiple calls are permitted), so the rare cold-path race where two threads both miss the flag is harmless and no Once/spin is needed, which keeps the #![no_std] crate dependency-free. The SM_CLEANBOOT safe-mode skip is preserved. WSAStartup itself and WSAGetLastError/WSASetLastError (thread-local slot accessors with no init requirement) forward without gating. bun_iocp::init::ensure_winsock and bun_sys::windows::ensure_winsock become re-exports of the canonical gate. The four explicit call-site ensure_winsock() calls (afd.rs create_peer_socket, sys/lib.rs recv/send, dns.rs work_pool_callback) are removed as redundant. The Bun__ensure_winsock C-ABI export stays for the C-side callers (usockets bsd_create_socket, the uv polyfills), which call ws2_32 directly from C.
status.toml: - HIST-06 (lazy Winsock init) is implemented: every ws2_32 call gates at the bun_windows_sys::ws2_32 module boundary. - SIGEV-33: cite the fsevent test that actually exercises two_call_wide (the previous entry cited the unrelated win32_grow_probe test). test harness (not shipping code): - tty/tests.rs and signal.rs spawn_console_fixture: drain stdout/stderr on threads concurrent with the bounded try_wait, so a fixture that grows past the anonymous-pipe buffer cannot deadlock on write. The current fixtures are well under the buffer; this removes the latent footgun.
| //! Node-visible uv errno protocol numbers. JS sees these in `err.errno` | ||
| //! (e.g. ENOENT = -4058 on Windows), so the NUMBERS are ABI — they survive | ||
| //! the libuv removal as pure protocol constants, owned here. | ||
|
|
There was a problem hiding this comment.
🟡 The Windows uv_open_osfhandle polyfill was hand-written into src/jsc/bindings/uv-posix-stubs.c:1010-1026 — a file whose header says "GENERATED CODE - DO NOT MODIFY BY HAND" — while "uv_open_osfhandle" is still uncommented in the generator's symbols array (generate_uv_posix_stubs_constants.ts:177). Re-running bun run uv-posix-stubs will silently replace the polyfill with a plain crash stub, regressing the ea7a056 fix. Move the implementation to uv-posix-polyfills.c (alongside its inverse uv_get_osfhandle) and comment out constants.ts:177, matching every sibling polyfill in this PR.
Extended reasoning...
What the bug is
The Windows uv_open_osfhandle polyfill (added in ea7a056 for robobun finding #6) was hand-edited into src/jsc/bindings/uv-posix-stubs.c at lines 1010-1026 — a file whose line 1 reads // GENERATED CODE - DO NOT MODIFY BY HAND. Meanwhile, "uv_open_osfhandle" is still an uncommented entry in the symbols array at generate_uv_posix_stubs_constants.ts:177, and the generator (generate_uv_posix_stubs.ts:288-299) does Bun.write(join(..., "uv-posix-stubs.c"), final_contents) — a wholesale overwrite. So re-running bun run uv-posix-stubs will replace the hand-written #if OS(WINDOWS) ... Bun__FdTable__adoptHandle ... #else block with a plain __bun_throw_not_implemented("uv_open_osfhandle") crash stub, silently regressing the fix (N-API addons that adopt HANDLEs on Windows would abort again).
Why this is inconsistent with the PR's own convention
Every OTHER real polyfill this PR added follows the established pattern: comment the symbol out of the symbols array in generate_uv_posix_stubs_constants.ts and define it in the hand-maintained uv-posix-polyfills.c. Verified against source: // "uv_get_osfhandle", at :110, // "uv_getrusage", at :116, // "uv_guess_handle", at :120, // "uv_kill", at :151, // "uv_cpu_info", at :39, // "uv_os_setpriority", at :194. uv_open_osfhandle is the ONLY polyfill placed in the generated file — grep -n '#if OS' uv-posix-stubs.c returns exactly two hits: line 4 (the outer #if !(OS(...)) wrapper the generator itself emits) and line 1010 (this hand-edit).
Why the generator has no escape hatch
addStub() at generate_uv_posix_stubs.ts:246-252 emits a plain { __bun_throw_not_implemented(...); __builtin_unreachable(); } body for every symbol in the array, with no per-symbol or per-platform special-casing anywhere. Grep for open_osfhandle in the generator .ts file returns zero hits — the generator has no knowledge of this symbol beyond its presence in the array. There is no merge/preserve logic; the Bun.write overwrites the file wholesale.
Step-by-step proof
generate_uv_posix_stubs_constants.ts:177—"uv_open_osfhandle",(not commented; contrast :176// "uv_once",right above it).generate_uv_posix_stubs.ts:246-252—addStub()returns a plain crash-stub body for every symbol; zero grep hits foropen_osfhandlein the generator.generate_uv_posix_stubs.ts:288-299—Bun.write(join(..., "uv-posix-stubs.c"), final_contents)overwrites the file wholesale.- Current
uv-posix-stubs.c:1010-1026— the hand-added#if OS(WINDOWS) ... Bun__FdTable__adoptHandle ... #else ... crash stub ... #endif. - After re-running the generator:
uv-posix-stubs.c:~1010becomes the plain crash stub for BOTH platforms;Bun__FdTable__adoptHandleis never called; on Windows an addon callinguv_open_osfhandlegets a process abort.
Impact and why this is a nit
Nothing is broken at runtime today — the committed file works. The generator is a manual bun run uv-posix-stubs invocation, not part of the automatic build, so this only bites the next person who regenerates the stubs (e.g. when adding a new symbol). And test/napi/uv.test.ts would catch the regression on Windows CI (per dylan-conway's fix note: "The napi fixture round-trips it through a DuplicateHandle"). So this is a code-placement/durability landmine, not a merge-blocking defect. It violates the file's own DO-NOT-MODIFY header and diverges from every sibling polyfill's pattern in the same PR.
Fix
Move the implementation to src/jsc/bindings/uv-posix-polyfills.c — its inverse uv_get_osfhandle already lives there at :459-462, so they'd sit together. The POSIX crash stub moves with it (same shape as uv_free_interface_addresses at uv-posix-polyfills.c:192-197 in the POSIX block). Then comment out "uv_open_osfhandle" at generate_uv_posix_stubs_constants.ts:177 to match every sibling. Two-file change, no behavior difference.
(This is distinct from the existing table.rs:453 comment on the same polyfill — that's about its failure-path close semantics; this is about its placement making it non-durable across a generator run.)
(Anchored at src/errno/uv_numbers.rs because uv-posix-stubs.c and the generator .ts have no diff hunk ranges in the tracker; the actual code is at uv-posix-stubs.c:1010-1026 and generate_uv_posix_stubs_constants.ts:177.)
| /* Loop liveness is poll-count based on every backend; the socket's poll | ||
| * already counts, so there is nothing to add. Kept for API compatibility. */ | ||
| void us_socket_ref(struct us_socket_t *s) { | ||
| #ifdef LIBUS_USE_LIBUV | ||
| uv_ref((uv_handle_t *) s->p.uv_p); | ||
| #endif | ||
| // do nothing if not using libuv | ||
| (void) s; |
There was a problem hiding this comment.
🟡 Same stale-comment class as the loop_data.h / util.rs:1068 / MiniEventLoop.rs / windows_errno.rs:13 threads — this new comment says 'Loop liveness is poll-count based on every backend; the socket's poll already counts', but on the Windows engine this PR adds, polls explicitly do NOT count (bun_iocp.h:112: 'polls and the wakeup async never count'; usockets.rs:837 immediately .unref()s with 'uSockets polls never hold the loop open'; the hazard-7 test at usockets.rs:1716 asserts us_loop_active_count == 1 after starting a poll). Zero runtime impact — the no-op body is behaviorally correct (JS-layer KeepAlive via us_loop_add_active is what holds the loop on Windows); only the rationale is wrong. Fix: drop the first sentence, or say 'On POSIX the socket's poll counts via num_polls; on the Windows engine polls never count and JS-layer KeepAlive (us_loop_add_active) holds the loop. Kept for API compatibility.'
Extended reasoning...
What the bug is
The new comment on us_socket_ref at packages/bun-usockets/src/socket.c:688-689 (added by this PR to replace the deleted #ifdef LIBUS_USE_LIBUV uv_ref(...) body) reads:
/* Loop liveness is poll-count based on every backend; the socket's poll
* already counts, so there is nothing to add. Kept for API compatibility. */
void us_socket_ref(struct us_socket_t *s) {
(void) s;
}Both clauses of the first sentence — 'poll-count based on every backend' and 'the socket's poll already counts' — are false for the Windows IOCP backend this same PR introduces.
The evidence, from this PR's own code
Three independent sources in the diff contradict the comment:
- The normative contract at
packages/bun-usockets/src/internal/eventing/bun_iocp.h:110-114(theus_loop_add_activedoc): 'One coherent refcount: explicit units here + non-fallthrough timers; polls and the wakeup async never count.' - The implementation at
src/iocp/usockets.rs:837-839—us_poll_start_rcimmediately callswatcher.unref()with the comment 'uSockets polls never hold the loop open (hazard 7)'. Andsrc/iocp/afd.rs:600-603documents that fast AFD IRPs usereq_submitted_uncounted()so 'an unref'd watcher with a parked IRP must let the loop exit' — the poll's IRP doesn't count towardactive_reqseither. - The unit test at
src/iocp/usockets.rs:1716explicitly assertsus_loop_active_count(loop_) == 1with message"polls are always unref'd"immediately afterus_poll_start_rc.
On POSIX (epoll_kqueue.c num_polls — if (loop->num_polls == 0) return;) the claim IS true: every started poll increments num_polls. So the comment is correct for POSIX and false for Windows; 'on every backend' is the wrong part.
Step-by-step proof
- Read socket.c:688: comment claims 'the socket's poll already counts' toward loop liveness.
- On the Windows engine,
us_poll_start_rc(usockets.rs) creates anAfdPollwatcher and at :837-839 callswatcher.unref()— the poll handle contributes 0 toactive_handles. Loop::alive()at event_loop.rs:230-233 checksactive_handles > 0 || active_reqs > 0 || pending— no poll count.- The AFD IRP is submitted via
req_submitted_uncounted()(afd.rs:600-603), so it contributes 0 toactive_reqs. - Therefore on Windows a started socket poll contributes NOTHING to loop liveness. The unit test at usockets.rs:1716 pins this: after starting a poll,
us_loop_active_countis unchanged. - What actually holds the loop for a socket on Windows: the JS-layer
Async.KeepAlive→us_loop_add_active(bun_iocp.h:110-116, the contract this comment should have referenced).
Why the no-op body is still correct
The behavior is fine — this is comment-accuracy only. The old uv_ref/uv_unref bracket in HttpContext.h only spanned a single synchronous onData dispatch (loop liveness isn't checked mid-dispatch), so it was already a POSIX no-op and effectively a Windows-libuv no-op too. On the new Windows engine, sockets are held alive by the JS-layer KeepAlive → us_loop_add_active path, not by us_socket_ref. So the empty body is behaviorally correct on every backend; the comment just gives the wrong reason.
Impact and class
Zero runtime impact. Same stale/wrong-comment cleanup class as the four open threads on this PR — loop_data.h:59 (orphaned quic_timer doc + AnyEventLoop.rs + CLAUDE.md), util.rs:1068 (makeLibUVOwned/.uv() renames), MiniEventLoop.rs:486 (stale invariant guard), and windows_errno.rs:13 (six bun_libuv_sys references) — at a distinct location none of them cover (grep confirms zero hits for socket.c / us_socket_ref in any of those threads). Per CLAUDE.md 'Comments carry only durable non-obvious content', it belongs in the same cleanup pass.
Fix
Either drop the first sentence entirely (leaving just 'Kept for API compatibility.'), or state the actual per-backend mechanism:
/* On POSIX the socket's poll counts via num_polls; on the Windows engine
* polls never count and JS-layer KeepAlive (us_loop_add_active) holds the
* loop. Kept for API compatibility. */Same treatment for the sibling us_socket_unref comment at :730 ('See us_socket_ref.').
| //! Node-visible uv errno protocol numbers. JS sees these in `err.errno` | ||
| //! (e.g. ENOENT = -4058 on Windows), so the NUMBERS are ABI — they survive | ||
| //! the libuv removal as pure protocol constants, owned here. | ||
|
|
There was a problem hiding this comment.
🟡 The four std::thread::spawn calls added in HEAD commit 194d4c0 (src/iocp/signal.rs:1419,1426 and src/iocp/tty/tests.rs:1378,1385) lack #[allow(clippy::disallowed_methods)], so cargo clippy --all-targets on Windows fails with 4 denials. The pre-existing sibling at usockets.rs:1903 (same crate, also #[cfg(test)]) carries #[expect(clippy::disallowed_methods)] with the natively-linkable-test-binary rationale, and these SAME two functions already carry #[allow(clippy::disallowed_types)] on their std::process::Command uses (signal.rs:1395, tty/tests.rs:1354) — so the four new calls are inconsistent with the immediate surrounding convention. The GitHub Actions clippy job (ubuntu, no --all-targets) doesn't compile #[cfg(windows)] test code so it won't see this; only local/BuildKite Windows clippy runs bite. (Anchored here because signal.rs and tty/tests.rs are new files with no diff line ranges; actual code at signal.rs:1419,1426 and tty/tests.rs:1378,1385.)
Extended reasoning...
What the bug is
Commit 194d4c0 (HEAD, 'status.toml + test-harness follow-ups from the tty/signal/fsevent review') added concurrent stdout/stderr drain threads to two subprocess-spawning test helpers, using std::thread::spawn at four sites — src/iocp/signal.rs:1419, :1426 and src/iocp/tty/tests.rs:1378, :1385 — with no #[allow(clippy::disallowed_methods)] or #[expect(...)] attribute. The workspace's clippy.toml:13 lists std::thread::spawn as disallowed (message: 'use bun_threading::spawn_named or ThreadPool'), Cargo.toml:277 sets disallowed_methods = "deny", and src/iocp/Cargo.toml:9-10 has [lints] workspace = true. So cargo clippy --all-targets on a Windows target fails with four disallowed-method denials.
Why this is inconsistent with the immediate surrounding convention
Two levels of local convention make this the odd one out:
-
Same crate, same
#[cfg(test)]context:src/iocp/usockets.rs:1901-1904has a pre-existingstd::thread::spawnin test code carrying#[expect(clippy::disallowed_methods)]with the exact rationale — 'bun_threading would break this crate's natively-linkable test binary' (thebun_iocpcrate deliberately depends only onbun_windows_syspersrc/iocp/Cargo.toml's comment, so itscargo testbinary links without mimalloc/simdutf/crash-handler externs; pulling inbun_threadingwould break that). -
Same two functions: signal.rs:1395 and tty/tests.rs:1354 — inside the very same test helpers that gained the new spawn calls — already carry
#[allow(clippy::disallowed_types)]on theirstd::process::Commanduses. So the author was aware of the disallowed-item lints in these exact functions and suppressed the type-level one; the method-level one on the four new lines was simply missed.
Why the GitHub Actions clippy job doesn't see it
package.json:74 defines the clippy script as cargo clippy --workspace --no-deps --keep-going (no --all-targets), and the GH Actions clippy job runs on ubuntu. Without --all-targets, test code isn't compiled; and on a non-Windows host, everything under #[cfg(windows)] (both signal.rs and tty/tests.rs are Windows-gated) is skipped entirely. So the GH Actions lint lane stays green. This only bites cargo clippy --all-targets on a Windows target — local dev on Windows, or any BuildKite Windows clippy lane that passes --all-targets.
Step-by-step proof
clippy.toml:13—disallowed-methods = [ ..., { path = "std::thread::spawn", reason = "use bun_threading::spawn_named or ThreadPool" }, ... ].Cargo.toml:277—disallowed_methods = "deny"in[workspace.lints.clippy].src/iocp/Cargo.toml:9-10—[lints] workspace = true.git show 194d4c04 -- src/iocp/signal.rs src/iocp/tty/tests.rs— the fourstd::thread::spawn(move || { ... })calls are new in this commit ('drain stdout/stderr on threads concurrent with the bounded try_wait').rg 'allow\(clippy::disallowed_methods\)|expect\(clippy::disallowed_methods\)' src/iocp/— one hit: usockets.rs:1903. No module-level, function-level, or crate-level allow covers signal.rs or tty/tests.rs (verified: no#[allow(clippy::disallowed_methods)]in lib.rs, tty.rs, tty/tests.rs, or signal.rs).- On Windows:
cargo clippy --all-targets -p bun_iocp→ 4×error: use of a disallowed methodstd::thread::spawn`` at signal.rs:1419, signal.rs:1426, tty/tests.rs:1378, tty/tests.rs:1385.
Impact
Test-only code, zero runtime impact, and the GH Actions clippy job doesn't see it — hence nit rather than blocking. But it's a real cargo clippy --all-targets denial on Windows and inconsistent with the sibling convention in the same functions/crate.
Fix
Add #[allow(clippy::disallowed_methods)] on each of the four std::thread::spawn calls, matching usockets.rs:1903. Four one-line insertions.
…30bdf # Conflicts: # Cargo.lock # Cargo.toml # packages/bun-usockets/src/internal/loop_data.h # packages/bun-usockets/src/loop.c # src/bun_core/lib.rs # src/jsc/GarbageCollectionController.rs # src/jsc/web_worker.rs # src/libuv_sys/libuv.rs # src/runtime/Cargo.toml # src/runtime/api/bun/js_bun_spawn_bindings.rs # src/runtime/node/node_fs.rs # src/runtime/socket/Listener.rs # src/runtime/timer/mod.rs # src/sys/lib.rs # src/uws_sys/InternalLoopData.rs # src/windows_sys/externs.rs # test/js/bun/net/socket.test.ts
- externs.rs: main and PR both added FILE_ALL_INFORMATION / FS_INFORMATION_CLASS / FILE_FS_* types independently; auto-merge kept both. Removed main's copies (PR's have quirk tags and are what winfs consumes). - sys/windows/mod.rs: removed main's filetime_to_timespec / timespec_to_filetime (added for #33713's stat path; superseded by winfs's own definitions and now unused after the merge). - node_fs.rs mkdtemp: Windows arm now returns encode_path_result(out, encoding) to match the StringOrBuffer return type main introduced.
Main's #33359 gated the timer-based sweep path behind LIBUS_USE_LIBUV (vs the new folded-timeout path on epoll/kqueue). This branch removed libuv entirely, so LIBUS_USE_LIBUV is never defined and the guards silently took the POSIX #else branch on Windows, dropping sweep_timer and has_added_timer_to_event_loop from the structs and breaking the static_asserts in bun_iocp.h and the Rust mirrors. The IOCP backend uses the same timer-based sweep (us_create_timer / us_timer_set) that libuv did, so the guards become LIBUS_USE_BUN_IOCP. Also re-guards us_timer_close(sweep_timer) in loop_data_free, which the earlier merge had left unconditional (compile error on POSIX).
| free(loop->data.send_buf); | ||
|
|
||
| #ifdef LIBUS_USE_LIBUV | ||
| us_timer_close(loop->data.sweep_timer, 0); |
There was a problem hiding this comment.
🔴 The merge with main (11d868d) brought in #33359, which gates fields on #ifdef LIBUS_USE_LIBUV — a macro this PR removed entirely (replaced with LIBUS_USE_BUN_IOCP). Since LIBUS_USE_LIBUV is never defined, sweep_timer and has_added_timer_to_event_loop no longer exist on any platform, producing hard compile errors: loop.c:153 (us_timer_close(loop->data.sweep_timer, 0), all platforms), bun_iocp.h:62 (offsetof(..., sweep_timer), Windows), and internal.h:391 (_Static_assert(sizeof(us_internal_callback_t) == 64) fails at 48, Windows). Minimal fix: change every remaining #ifdef LIBUS_USE_LIBUV (loop_data.h:39, loop.c:49/123/384, internal.h:152/380) to #ifdef LIBUS_USE_BUN_IOCP. A third artifact from the same merge: gc_controller.deinit() is now called twice at web_worker.rs:1241 and :1307 — the second is an idempotent no-op with a factually-wrong comment; delete :1303-1308.
Extended reasoning...
What the bug is
Merge commit 11d868d brought in main's #33359 ('uws: drop us_timer_t on epoll/kqueue'), which gated two struct fields on #ifdef LIBUS_USE_LIBUV. But this PR removed LIBUS_USE_LIBUV entirely — libusockets.h:637 now defines only LIBUS_USE_BUN_IOCP. So on every platform the C preprocessor takes the #else arm of every #ifdef LIBUS_USE_LIBUV, and both fields disappear from their structs. Follow-up commit 1c39675 ('Fix silent auto-merge duplications') caught three Rust-side conflicts but did not touch loop_data.h / loop.c / internal.h / bun_iocp.h / usockets.rs / web_worker.rs.
The specific compile errors
All platforms — packages/bun-usockets/src/loop.c:153:
us_timer_close(loop->data.sweep_timer, 0);This line is ungated (the PR removed the #ifdef LIBUS_USE_LIBUV wrapper around it). But loop_data.h:39-45 now gates struct us_timer_t *sweep_timer on the never-defined LIBUS_USE_LIBUV, so the field doesn't exist — every platform gets long long sweep_next_tick_ns at offset 0 instead. Hard C compile error.
Windows — packages/bun-usockets/src/internal/eventing/bun_iocp.h:62:
BUN_IOCP_STATIC_ASSERT(offsetof(struct us_internal_loop_data_t, sweep_timer) == 0, ...)References the same nonexistent field.
Windows — packages/bun-usockets/src/internal/internal.h:391:
_Static_assert(sizeof(struct us_internal_callback_t) == 64, ...)internal.h:380-382 gates unsigned has_added_timer_to_event_loop; on #ifdef LIBUS_USE_LIBUV, so the field is gone. Without it, us_internal_callback_t on Windows is: alignas(16) us_poll_t (24) + loop* (8) + int (4) + int (4) + cb* (8) = 48 bytes. With the 4-byte field: 52 → padded to 64. So the assert fails at 48 ≠ 64.
Windows semantic breakage (post-compile-fix)
Even after fixing the compile errors by simply deleting the field references, the Windows engine would be broken:
- The Rust mirror
LoopDataat src/iocp/usockets.rs:71 still declarespub sweep_timer: *mut UsInternalCallbackat offset 0. C'sus_internal_loop_data_init(loop.c:126) writessweep_next_tick_ns = -1there. usockets.rs:632-634 reads it as a pointer, checks!is_null()(0xFFFF…FFFF is not null), then dereferences → segfault on the first tick. - The Rust mirror
UsInternalCallbackat usockets.rs:149-159 still declareshas_added_timer_to_event_loopand assertssize_of == 64at :240 and :1630; :1153 reads/writes the field. - The Windows sweep timer would be semantically dead:
us_internal_sweep_if_dueis only called from epoll_kqueue.c, never from the Windows engine, so socket idle timeouts (Bun.serveidleTimeout,net.Socket#setTimeout()) would never fire. - loop.c:77
clock_gettime(CLOCK_MONOTONIC, ...)would compile into the Windows arm.
Step-by-step proof
rg '#define LIBUS_USE_LIBUV'→ zero hits repo-wide. Only#define LIBUS_USE_BUN_IOCPat libusockets.h:639.rg '#if.*def LIBUS_USE_LIBUV'→ six orphaned gates: loop_data.h:39, loop.c:49/123/384, internal.h:152/380.- loop_data.h:39-45: with
LIBUS_USE_LIBUVnever defined, the struct haslong long sweep_next_tick_nsat offset 0 and nosweep_timerfield on any platform. - loop.c:153 references
loop->data.sweep_timerunconditionally → C compile error, all platforms. - bun_iocp.h:62 references
offsetof(..., sweep_timer)underLIBUS_USE_BUN_IOCP→ C compile error, Windows. - internal.h:380-382:
has_added_timer_to_event_loopis gone; sizeof(us_internal_callback_t) drops from 64 to 48; the_Static_assert(== 64)at :391 (added by this PR underLIBUS_USE_BUN_IOCP) fails.
The third merge artifact: web_worker.rs duplicate deinit
Same silent-auto-merge class, distinct location. #33359 on main both ADDED vm.gc_controller.deinit() at web_worker.rs:1241 (because it changed GC timers from uws handles to EventLoopTimer heap nodes) and REMOVED the pre-existing later block. But this PR had rewritten that later block's comment (from 'Must precede Loop.shutdown so uv_close isn't called twice' to 'Must precede on_thread_exit below — the GC timers live on the loop it frees'). Git's 3-way merge saw #33359's removal targeting the OLD comment text and kept both — the new call at :1241 AND the PR's rewritten block at :1303-1308.
The second call is a harmless no-op: GarbageCollectionController::deinit() is documented idempotent — it sets disabled = true (already true) and only removes timers where state == ACTIVE (already CANCELLED after the first call). But the comment at :1305-1306 ('the GC timers live on the loop it frees') is now factually wrong — after #33359 the GC timers are pairing-heap nodes on the VM, not engine handles on the uws loop. This is dead code with a misleading rationale that belongs in the same cleanup pass.
Fix
Minimal fix (restores what the Windows engine already implements and drives): change every remaining #ifdef LIBUS_USE_LIBUV / #ifndef LIBUS_USE_LIBUV to #ifdef LIBUS_USE_BUN_IOCP / #ifndef LIBUS_USE_BUN_IOCP:
- loop_data.h:39
- loop.c:49, 123, 384
- internal.h:152, 380
This gives Windows back the real us_timer_t *sweep_timer and has_added_timer_to_event_loop (which usockets.rs already implements at :1090+ and consumes at :632/:1153), and gives POSIX the ns-deadline model #33359 intended. Then delete web_worker.rs:1303-1308 (the duplicate gc_controller.deinit() block).
The alternative — porting the Windows engine to the ns-deadline model (fold sweep_next_tick_ns into Loop::tick, call us_internal_sweep_if_due, add a Windows monotonic-ns source, update the Rust mirrors + bun_iocp.h + usockets.rs) — matches main's design intent but is a larger change.
There was a problem hiding this comment.
The compile-error half is fixed (a4e9ac5 changed all six LIBUS_USE_LIBUV guards to LIBUS_USE_BUN_IOCP and re-gated us_timer_close(sweep_timer) at loop.c:153), so sweep_timer / has_added_timer_to_event_loop exist again on Windows and the static asserts hold.
The third merge artifact from this comment wasn't touched: gc_controller.deinit() is still called twice at web_worker.rs:1241 and :1307, and the block at :1303-1308 still carries the now-wrong 'the GC timers live on the loop it frees' comment (after #33359 they're pairing-heap nodes on the VM). Harmless no-op — deinit() is idempotent — but it's dead code with a misleading rationale; delete :1303-1308.
| //! Node-visible uv errno protocol numbers. JS sees these in `err.errno` | ||
| //! (e.g. ENOENT = -4058 on Windows), so the NUMBERS are ABI — they survive | ||
| //! the libuv removal as pure protocol constants, owned here. | ||
|
|
There was a problem hiding this comment.
🔴 The comment at src/windows_sys/externs.rs:2099 ("Ungated forwarders.", added in 8621334) trips test/internal/port-era-markers.test.ts, which bans /\bungated\b/i in Rust comments — this is failing on all platforms in CI build #71597 (🐧 x64-asan + all three 🪟 lanes). The word here means "not gated behind ensure_winsock()" (legitimate, not port-era jargon), but the lint doesn't distinguish. Reword to e.g. "Direct forwarders (no ensure_winsock gate)." (Anchored at uv_numbers.rs because externs.rs has no diff hunk in the tracker; the actual line is externs.rs:2099.)
Extended reasoning...
What the bug is
test/internal/port-era-markers.test.ts is a source-lint test that scans every src/**/*.rs file for banned port-era progress-narrative phrases in // comments. One of the banned patterns (line 41-42) is:
{ pattern: /\bungated\b/i, reason: "'ungated' is port-era progress narrative, not useful documentation" }Commit 8621334 in this PR ("ws2_32: gate every Winsock call at the module boundary") added a comment at src/windows_sys/externs.rs:2099:
// `WSAStartup` is the init itself; the error accessors are thread-local
// slot reads/writes with no init requirement. Ungated forwarders.The word "Ungated" matches /\bungated\b/i (word boundary before the U, case-insensitive), so the test fails.
The specific code path
The test at port-era-markers.test.ts:50 builds globAllSources().rust.filter(p => p.endsWith(".rs")), then for each file iterates lines and, for any line containing //, tests each banned regex against it. src/windows_sys/externs.rs is under src/ and ends in .rs, so it's in scope; line 2099 contains // and matches the ungated pattern.
Why existing code doesn't prevent it
The lint is intentionally broad — it catches port-era jargon like "ungated in phase 2" but has no way to distinguish that from a legitimate technical use of the word. Here "ungated" means "these three functions (WSAStartup, WSAGetLastError, WSASetLastError) forward directly to raw:: without the ensure_winsock() gate that every other ws2_32 wrapper in this module has" — a genuine architectural note about the lazy-winsock-init design, not port-progress narrative. But the test can't tell the difference, and it's the only /\bungated\b/i hit in src/ (verified with rg -n -i '\bungated\b' src/ --type rust).
Step-by-step proof
test/internal/port-era-markers.test.ts:41— banned pattern/\bungated\b/i.test/internal/port-era-markers.test.ts:50— scansglobAllSources().rustfiltered to.rs.src/windows_sys/externs.rs:2099— reads// slot reads/writes with no init requirement. Ungated forwarders.- Line contains
//;/\bungated\b/i.test("... Ungated forwarders.")→true(space beforeUis a word boundary; case-insensitive). git blame -L 2099,2099→ commit8621334f3, part of this PR (2026-07-07).rgconfirms this is the ONLY hit repo-wide insrc/**/*.rs.- The test is source-scanning (no runtime code), so it fails identically on every platform. robobun's CI comment for build #71597 (HEAD a4e9ac5) lists
test/internal/port-era-markers.test.tsfailing with code 1 on 🪟 2019 x64, 🪟 2019 x64-baseline, 🪟 11 aarch64, AND 🐧 13 x64-asan.
Impact
Hard CI test failure on every platform at the current PR HEAD — blocks merge until fixed. The fix is trivial (one-word comment rewrite), but without it CI is red on all lanes.
Fix
Reword the comment at src/windows_sys/externs.rs:2099 to avoid the banned token while keeping the meaning. For example:
// `WSAStartup` is the init itself; the error accessors are thread-local
// slot reads/writes with no init requirement. Direct forwarders (no ensure_winsock gate).or simply drop the last sentence (the preceding sentence already explains why these three don't need the gate).
(This comment is anchored at src/errno/uv_numbers.rs because src/windows_sys/externs.rs has no diff hunk in the tracker; the actual line is externs.rs:2099.)
|
I found a concrete Windows handle-inheritance case that appears directly relevant to the The following minimal reproduction is deterministic with both Bun import { spawn } from "node:child_process";
const server = Bun.serve({
hostname: "127.0.0.1",
port: 18131,
fetch() {
return new Response("ok");
},
});
const child = spawn(
process.execPath,
["-e", "setTimeout(() => {}, 30000)"],
{ detached: true, stdio: "ignore", windowsHide: true },
);
child.unref();
console.log(`parent=${process.pid} child=${child.pid}`);
await server.stop(true);
console.log("server.stop(true) resolved");
process.exit(0);After
Removing the child spawn is a clean control: the port is released normally on both runtimes. This also produced a user-visible service-lifecycle failure in OpenCodex #733: a long-lived Windows tray lineage inherited the live proxy's sockets, and a management restart could not reuse the configured port after the original proxy exited. Stopping only the tray immediately cleared the stale The behavior seems consistent with the current Windows spawn path allowing unrelated inheritable handles into the child, while this PR's explicit handle list should restrict inheritance to intended stdio handles. I have not tested this PR's build, so I am not claiming that it already fixes the reproduction. A focused Windows regression test could keep the spawned child alive, stop the server, and assert that the listen port can be rebound before the child exits. If this PR cannot land soon, the same test may also help evaluate whether the handle-list portion can be backported independently. |
3 participants
Bun no longer links libuv on any platform. Windows event-loop, file, pipe, process, tty, signal, and DNS plumbing now run on an in-tree IOCP/AFD engine (
src/iocp/), and JS-visible file descriptors are backed by Bun's own fd table (src/fdtable/) instead of the C runtime's lowio table.Why
libuv forced three costly designs on Windows:
uv_loop_tembedded next to the usockets loop with manual keep-alive bridging between the two refcount domains._open_osfhandle), so every fast path paid CRT lookups, and ownership (CRT_closevsCloseHandle) was per-callsite convention. The CRT table also caps at 8192 entries.Bun.file()reads went JS → uv_fs threadpool → uv completion → JS-thread bounce, wherefs.readFileused Bun's own worker pool with one hop. The measured cost is in the table below.What changed
us_loop_*contract. Sockets get readiness via AFD poll IOCTLs (the wepoll/libuv technique); pipes, files, processes, and wakeups deliver as completions on the same IOCP. The entireLIBUS_USE_LIBUVeventing backend is deleted from usockets, including the QUIC uv-timer arm — every backend now folds the QUIC deadline into its poll timeout. Completions posted for unref'd handles (e.g. an unref'd child's exit) are still collected during idle ticks, matchinguv_run(UV_RUN_NOWAIT)semantics.src/sys/windows/,src/winfs/) plus worker-pool async;readv/writev/preadv/pwritevandstatfsare implemented natively (previouslyENOSYSstubs whose errno constant collided withELOOP).lseekis fd-table-aware, so sequential I/O after a seek uses the logical position;O_SYNC/O_DSYNC/O_DIRECTreach the engine's write-through/no-buffering arms instead of being dropped at the open boundary.src/fdtable/): slot = HANDLE + kind + flags; bit-63 tagging keeps table indices and raw handles type-distinct; close-exactly-once enforced, including on adoption-failure paths. The CRTlpReserved2blob is still parsed/emitted at process boundaries, so fd passing to/from Node processes keeps working.ReadDirectoryChangesW.getaddrinfoon the shared worker pool (ws2_32), same cache and request coalescing as POSIX.uv__once_init(error mode, CRT invalid-parameter handler, suspend/resume loop wake, console-ctrl hook) lives in the engine'sprocess_init(). Winsock initializes at the first actual consumer — socket creation, AFD peer setup,getaddrinfo,send/recv— so network-free invocations never load the service-provider catalog.bun.exestill exports the uv symbol set (305 symbols). 25 are real implementations (mutex/once/hrtime, getpid/getppid, kill, priorities, cpu_info, interface_addresses, rusage, RSS, guess_handle, strerror, version, get_osfhandle, …); the rest are loud crash stubs that name the unsupported symbol — the same policy POSIX builds ship with. An addon that previously drove uv handles on the Bun loop on Windows now fails fast with a clear message instead of silently sharing a loop that no longer exists.process.versions.uvreports the emulated ABI level (1.51.0).ReadableStream:releaseLock()no longer assumes every native-stream source implements$resume; the lazy complete-buffer path produces a plain source without it (previously unreachable because uv opens never completed synchronously).Performance (Windows release builds, interleaved A/B runs)
Bun.file().arrayBuffer()N=32, 64KBfs.readFileN=32The
Bun.filegap was the uv chain: the same hardware serves 6.2 GB/s through both APIs now.Compatibility notes
tty_wrapsetRawModeerror returns are positive errnos (previously negative uv codes); in-tree consumers only truth-test the value.uv_kill-polyfill signal semantics match the engine: INT/QUIT/KILL/TERM terminate (exit code 1), 0 probes, othersENOSYS.listen({fd})were never supported on Windows; the rejection message changed shape.Testing
lpReserved2parsing and a differential battery against ucrt's lowio table (same op script driven through_open_osfhandle/_read/_lseeki64/_closeand through the table, asserting element-wise identical observations), uv-errno mapping specs (miri-clean), and IPC frame known-answer tests.