Skip to content

Report EBADF instead of UV_EBADF for async libuv fs errors on Windows - #37465

Closed
robobun wants to merge 1 commit into
mainfrom
farm/26028479/uv-rc64-errno-translate
Closed

robobun wants to merge 1 commit into
mainfrom
farm/26028479/uv-rc64-errno-translate

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Repro (Windows only)

const fs = require("fs");
const fd = fs.openSync("existing.txt", "r");
Bun.write(Bun.file(fd), "abc").catch(e => console.log(e.code, "|", e.message));
Windows:  UV_EBADF | UV_EBADF: unknown error, write
POSIX:    EBADF    | EBADF: bad file descriptor, write
Node:     EBADF    | EBADF: bad file descriptor, write   (errno -4083 on Windows, same as Bun)

Bun.write(Bun.file(fd), new Uint8Array(3)), Bun.write(Bun.file(fd), Bun.stdin) with a piped stdin, Bun.file(fd).writer() and Bun.file(fd).stream() on a write-only fd all report the same UV_E* code on Windows. node:fs on the same fd, and Bun.file(fd).text(), already report EBADF. User code checking err.code === "EBADF" (or any other errno name) therefore fails on Windows for these Bun APIs only.

Cause

libuv hands back an error either as the c_int return value of a synchronous call (ReturnCode) or, for asynchronous requests, in uv_fs_t.result (ReturnCodeI64). ReturnCode::errno() maps the negative UV_E* code to the POSIX E discriminant that bun_sys::Error.errno stores; ReturnCodeI64::errno() returned the raw |UV_E*| magnitude (4083 for UV_EBADF) instead. Every consumer treats the two the same way and stores the value with from_libuv unset: Error::from_uv_rc64 (behind ReturnCodeI64::to_error, used by CopyFileWindows's read/write loop and the PipeWriter/PipeReader file completions that back FileSink and Bun.file().stream()), and WriteFileWindows::on_write_complete directly.

Nothing catches this because on Windows the E enum also declares the sparse UV_E* tail variants at exactly those magnitudes, so Error::resolve_system_errno turns 4083 into E::UV_EBADF: strum names it UV_EBADF, and the libuv message table only has labels for the E* variants, hence "unknown error". Rust-side comparisons such as err.get_errno() == E::EPIPE (for example the shell's broken-pipe bookkeeping in IOWriter::fail_pending_writers) also never matched errors produced by these paths on Windows. The asymmetry between the two return-code types predates the Rust port.

Fix

ReturnCodeI64::errno() translates the code exactly like ReturnCode::errno(); err_enum() is now an alias of it, as it already was on ReturnCode. The translation is shared through one helper that folds codes missing from the table to E::UNKNOWN (the fallback translate_uv_error_to_e already uses), so a negative result can never come back as None and be mistaken for success. That matters for the async callers, which go on to try_from(rc.int()).expect(..) on the "success" path; every code in libuv's errno.h is in the table today, so this only changes what would happen on a future libuv addition. A compile-time block pins both types to the same contract.

Fixing the producer rather than teaching resolve_system_errno to fold UV_E* variants keeps Error.errno holding one kind of value for from_libuv == false, which is what get_errno()/is_retry() assume. The node:fs completion handlers, which store the raw magnitude with from_libuv: true, are a separate convention and are left alone; #32066 is reworking those and independently adds the same unmapped-code fallback to ReturnCode::errno(), while explicitly leaving this mislabeling out of scope, so the two overlap only in that hunk.

Verification

New tests cover each affected producer: Bun.write(Bun.file(fd), string | Uint8Array) and Bun.write(Bun.file(fd), Bun.stdin) in test/js/bun/io/bun-write.test.js, Bun.file(fd).writer() in test/js/bun/util/filesink.test.ts, Bun.file(fd).stream() in test/js/bun/util/bun-file.test.ts. The write tests assert the same code/errno/message on every platform (errno is -4083 on Windows and -9 elsewhere, matching Node); the pipe-source and stream tests are Windows only because POSIX takes different paths there (a pipe source is rejected earlier, and a failing stream() read does not settle yet).

On Windows x64, all five cases fail with bun 1.4.0 ("code": "UV_EBADF", "message": "UV_EBADF: unknown error, write") and pass with this branch; bun-write.test.js, filesink.test.ts, bun-file.test.ts, spawn.test.ts, translate-uv-error-windows.test.ts and error-name-from-libuv.test.ts pass on the debug build. On POSIX the new tests pass before and after, since the changed code is #[cfg(windows)]. cargo check and clippy are clean for bun_libuv_sys/bun_sys on x86_64-pc-windows-msvc.

ReturnCode::errno() maps a negative libuv code to the POSIX E discriminant
that bun_sys::Error.errno stores, but ReturnCodeI64::errno() (the type of
uv_fs_t.result, so every async fs completion) returned the raw |UV_E*|
magnitude. Callers store both the same way with from_libuv unset, and on
Windows E also declares the sparse UV_E* tail variants at exactly those
magnitudes, so the error resolved to E::UV_EBADF and reached JS as
code "UV_EBADF" with message "UV_EBADF: unknown error, write" instead of
"EBADF: bad file descriptor, write".

Affected: Bun.write to a fd (WriteFileWindows), the Bun.write read/write
copy loop (CopyFileWindows), FileSink writes and Bun.file().stream() reads
on Windows (PipeWriter/PipeReader file completions via Error::from_uv_rc64).

Both return-code types now share one translation; codes missing from the
table fold to E::UNKNOWN so a failed call can never read as success.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 36d13e1b-d656-4f64-847e-f9c001975297

📥 Commits

Reviewing files that changed from the base of the PR and between 54d6d16 and f722611.

📒 Files selected for processing (5)
  • src/libuv_sys/libuv.rs
  • src/sys/Error.rs
  • test/js/bun/io/bun-write.test.js
  • test/js/bun/util/bun-file.test.ts
  • test/js/bun/util/filesink.test.ts

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

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on Windows Server 2019 x64 with bun 1.4.0 (Bun.write(Bun.file(readOnlyFd), "abc") rejects with code: "UV_EBADF", message UV_EBADF: unknown error, write; same for Bun.file(fd).writer(), Bun.write(Bun.file(fd), Bun.stdin) and Bun.file(fd).stream()). The five new test cases fail there on 1.4.0 and pass with this branch's debug build; on POSIX the changed code is not compiled, so the tests pass both before and after there.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — aligns ReturnCodeI64::errno() with ReturnCode::errno() so async libuv fs errors on Windows report EBADF instead of UV_EBADF.

What was reviewed:

  • Traced consumers of ReturnCodeI64::errno() (WriteFileWindows::on_write_complete, CopyFileWindows read/write loop, PipeWriter/PipeReader file completions via from_uv_rc64) — all store into Error.errno with from_libuv=false and expect the translated discriminant.
  • Confirmed node_fs handlers using from_libuv: true compute (-rc) as _ directly and do not go through errno(), so they're unaffected.
  • Verified E_UNKNOWN_DISCRIMINANT = 134 matches E::UNKNOWN in windows_errno.rs; the new Some(UNKNOWN) fallback for unmapped negative codes prevents the usize::try_from(rc.int()).expect(..) panic at write_file.rs:1087 on the fall-through path.
Extended reasoning...

Overview

This PR fixes a Windows-only error-code mislabeling in src/libuv_sys/libuv.rs. ReturnCodeI64::errno() (used for async uv_fs_t.result) previously returned the raw |UV_E*| magnitude (e.g. 4083 for UV_EBADF) instead of the translated POSIX E discriminant (9 for EBADF), unlike its sibling ReturnCode::errno(). Consumers stored this into bun_sys::Error.errno with from_libuv=false, so resolve_system_errno matched the sparse E::UV_EBADF tail variant and JS saw code: "UV_EBADF" / "unknown error". The fix routes both return-code types through a shared uv_err_to_e_discriminant_lossy helper (folding unmapped codes to E::UNKNOWN), aliases err_enum() to errno(), and adds a const _: () = { ... } block asserting the contract at compile time. src/sys/Error.rs gets a doc-comment update only. Five new tests cover each affected producer.

Security risks

None. This is pure error-code translation for user-visible diagnostics; no auth, crypto, path handling, or untrusted-input parsing is involved.

Level of scrutiny

Medium-low. The changed code is #[cfg(windows)]-gated and small (~40 net Rust lines). The behavior change is a strict bug fix (aligning two nominally-identical accessors), and I traced every consumer of ReturnCodeI64::errno() / from_uv_rc64 / .to_error() on io_request.result to confirm they all expect the translated discriminant. The one semantic widening — ReturnCode::errno() now returns Some(E::UNKNOWN) instead of None for negative codes missing from the table — is defensive: no libuv code is missing today, and returning None would let write_file.rs:1087's usize::try_from(rc.int()).expect(..) panic on a negative fall-through. The compile-time assertions pin UV_EBADF → 9, 0/i64::MAX → None, and unmapped/overflow → E_UNKNOWN_DISCRIMINANT on both types.

Other factors

  • E_UNKNOWN_DISCRIMINANT = 134 matches E::UNKNOWN = 134 in src/errno/windows_errno.rs, and the UV_UNKNOWN => 134 table row it references.
  • The node:fs completion handlers that store raw magnitudes with from_libuv: true (src/runtime/node/node_fs.rs) compute (-rc) as _ directly and never call ReturnCodeI64::errno(), so they are untouched — matching the PR description's scoping note.
  • Tests follow harness conventions: tempDir, bunEnv/bunExe, concurrent pipe draining, exact-shape assertions on {code, errno, syscall, message}, negative-contract check that the target file is unmodified, and it.skipIf(!isWindows) with a stated reason for the two Windows-only paths.
  • No prior human reviews or outstanding comments on the timeline.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:33 AM PT - Aug 11th, 2026

✅ @robobun, your commit f722611c4bc79d52d0ba709add59ea66f8a55e08 passed in Build #92153! 🎉


🧪   To try this PR locally:

bunx bun-pr 37465

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

bun-37465 --bun

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Note for whoever rebases or merges this: the compile-time assertion block this adds to src/libuv_sys/libuv.rs calls ReturnCode::from_raw and ReturnCodeI64::init. Both constructors were deleted from main by #39249 (a42889a), so on top of current main this branch no longer compiles for Windows targets (libuv.rs is #![cfg(windows)]), even though GitHub still reports it as mergeable because the hunks do not overlap textually. Both newtypes keep their tuple field pub(crate), so the block can build the values directly (ReturnCode(UV_EBADF), ReturnCodeI64(UV_EBADF as i64)). The rest of the diff applies as is.

A separate report of the same mislabeling reached me, described from the Error::from_uv_rc64 side (the ReturnCodeI64::to_error calls in src/io/PipeReader.rs and src/io/PipeWriter.rs, which surface through FileSink and Bun.file().stream()). Since from_uv_rc64 stores ReturnCodeI64::errno(), the change here covers those paths too, so no second PR is being opened for it.

@robobun

robobun commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #40860. It removed the from_libuv flag and the raw u16 ReturnCode::errno(). ReturnCodeI64::errno() now translates the libuv code through translate_uv_error_to_e, the same way ReturnCode::errno() does, so async fs errors on Windows carry the errno name.

Verified on a Windows x64 debug build of main (b49398c). The repro prints EBADF | -4083 | EBADF: bad file descriptor, write. The five tests from this PR, applied on top of main without the src/ changes, pass. The same tests fail on the pre-#40860 canary with code: "UV_EBADF". The test hunks still apply cleanly on main if someone wants them as regression coverage.

Closing.

@robobun robobun closed this Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant