Conversation
… node On Windows, bun_sys::Error derives the JS-visible err.code (and the message prefix) from the SystemErrno variant name, so errors that resolve to the EUNKNOWN variant (UV_UNKNOWN itself, or any Win32/uv code the tables do not map) surfaced as code "EUNKNOWN". libuv's uv_err_name(UV_UNKNOWN) is "UNKNOWN", which is what node reports and what util.getSystemErrorName(-4094) already returns in bun. Give the variant a strum serialize name of "UNKNOWN". Nothing parses SystemErrno from a string, so only the rendered name changes. The standalone-graph copy failure message formatted the errno with Debug, which bypasses the strum name; use Display there.
|
Warning Review limit reached
Next review available in: 28 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: reproduced on windows-x64 and windows-aarch64 with the released bun ( |
There was a problem hiding this comment.
LGTM — one-line strum serialize override making the Windows unknown-errno fallback render as UNKNOWN (matching libuv/node), plus consistent formatting and Windows-only regression tests. The inline nit about child_process.ts:1168 is pre-existing and on a different mechanism; not blocking.
Extended reasoning...
Overview
The fixing change is one attribute: #[strum(serialize = "UNKNOWN")] on SystemErrno::EUNKNOWN in src/errno/windows_errno.rs. This overrides the string IntoStaticStr/Display/EnumString produce for that variant without renaming the Rust identifier, so every existing SystemErrno::EUNKNOWN / E::EUNKNOWN match arm compiles unchanged. The pattern is already used in the same enum for E::_2BIG → "2BIG". The other source changes are: comment-only rewording in Listener.rs (behavior unchanged — the code already routed EUNKNOWN to a generic error and hardcoded "UNKNOWN" in the fallback), and {:?} → {} in StandaloneModuleGraph.rs so a Windows-only CopyFileW error message renders through the strum name instead of the Rust identifier. Three test files gain Windows-gated coverage (it.if(isWindows) / test.skipIf(!isWindows)).
Security risks
None. This changes only the string rendered for an already-surfaced error variant on Windows. No parsing, validation, auth, or privileged path is touched.
Level of scrutiny
Low-to-medium. It is a targeted Node compat fix with one behavioral line and mechanical follow-ups. I verified: Display for SystemErrno at src/errno/lib.rs:329 delegates to <&'static str>::from (the IntoStaticStr impl), so the {} change in StandaloneModuleGraph.rs compiles and picks up the override; no callers of SystemErrno::from_str/.parse() exist in src/, so the EnumString side of the attribute has no effect; the macro-appended UV_UNKNOWN tail variant serializes as "UV_UNKNOWN", so there is no strum name collision; no remaining "EUNKNOWN" string literals in test/, and the only one in src/ is the pre-existing child_process.ts:1168 site the inline nit already covers.
Other factors
The PR description documents USE_SYSTEM_BUN=1 fail / bun bd pass on windows-x64, cross-checks the bad.exe fixture on both Windows arches, and lists the adjacent test files re-run green. The tests assert exact {code, errno, syscall, message} shapes against node v26 output, cover both Bun.spawn/spawnSync and child_process.spawn/spawnSync, and add a self-consistency check against util.getSystemErrorName(-4094). The one existing assertion changed (error-name-from-libuv.test.ts line 20) is the direct assertion on the previously-wrong name, correctly updated. The inline nit is non-blocking: it is a different field (err.errno as a string), a different mechanism (JS SystemError constructor, not the strum table), and its correct fix is orthogonal (numeric errno + getSystemErrorName).
|
Heads up on an interaction with #39345, which makes |
…NOENT (#40602) ### Problem - On Windows, user-visible messages spell the errno without the `E` prefix. `bun install` with `"workspaces": ["C:/missing/*"]` prints `Failed to run workspace pattern C:/missing/* due to error NOENT`. POSIX prints `ENOENT`. - Cause: on Windows `bun_errno::E` is its own enum with unprefixed variant names (`NOENT`), and its `strum::IntoStaticStr` derive made `<&str>::from(e)` return that name (`src/errno/windows_errno.rs:95`). On POSIX `E` is `type E = SystemErrno`, so the same expression returns `ENOENT`. Nine messages built their name with `<&'static str>::from(err.get_errno())`. ### Fix - Replace the derive with `impl From<E> for &'static str` that returns `SystemErrno`'s name for the same discriminant. The two enums share one discriminant set (`SystemErrno::to_e` casts the other way, and all 138 dense variants match). Every `<&str>::from(E)` on Windows now prints `ENOENT`. The unused `strum::EnumString` derive goes too. - The nine sites switch to `bun_sys::Error::name()` (`src/sys/Error.rs:321`), the accessor the rest of the tree uses. It also translates libuv-sourced errnos and reports an unmapped value as `UNKNOWN`, which `get_errno()` cannot. - The two `bun:internal-for-testing` hooks that print an `E` (`src/sys_jsc/error_jsc.rs`) keep doing so. Their Windows-only tests now expect `EPERM`, `ENOENT`, `EUNKNOWN`. - Verified on windows-x64: `bad-workspace.test.ts` (new test), `translate-uv-error-windows.test.ts` and `rm-windows-ntstatus.test.ts` fail with bun 1.4.1 and pass with the debug build. Also `error-name-from-libuv.test.ts`, and `cron.test.ts` on Linux. ### Background - `bun_sys::Error::get_errno()` returns the errno as `E` for `match` arms. `name()` returns the user-visible code. - On Windows, `E` (bare names) and `SystemErrno` (E-prefixed names) are two `#[repr(u16)]` enums with the same discriminants. `pub const ENOENT: E = E::NOENT` aliases let cross-platform code compile. POSIX has no second enum. - strum's `IntoStaticStr` derive turns an enum value into its Rust variant name. <details><summary>Notes</summary> - Repro with the released bun 1.4.1 on windows-x64: ``` > echo { "name": "root", "workspaces": ["C:/nonexistent-dir-xyz/*"] } > package.json > bun install error: Failed to run workspace pattern C:/nonexistent-dir-xyz/* due to error NOENT ``` With this change the same command prints `due to error ENOENT`, as Linux does. - The nine sites: `src/install/lockfile/Package/WorkspaceMap.rs:456,472,488`, `src/install/lifecycle_script_runner.rs:389`, `src/runtime/api/cron.rs:192,396,1128`, `src/runtime/cli/test/parallel/Coordinator.rs:934`, `src/runtime/api/bun/spawn/stdio.rs:175`. A tenth, `src/jsc/NodeCompileCache.rs:162`, is left as is: the `From<E>` impl corrects its output, and #40600 switches it to `get_error_code_tag_name()`. - `stdio.rs:175` is inside `#[cfg(linux)]`, so its output was already right. It is changed for consistency with the other sites. - `EUNKNOWN` in the hook tests is `SystemErrno::EUNKNOWN`'s current name, the same string `err.code` carries today. #39340 renames it to `UNKNOWN` and should update these assertions with the rest. - `cargo test -p bun_errno` gains `e_spells_like_system_errno`. It is trivially true on POSIX and checks the new impl on a Windows host. `cargo test` for one crate does not link on Windows today (bun_core needs the C++ symbols), so the hook tests are the Windows coverage in CI. - The bug is Windows-only. On Linux every test in this PR passes with and without the change. - Edge cases where `name()` and the old expression differed on Windows: an error with `from_libuv` set printed `UV_ENOENT`, `name()` prints `ENOENT`. An unmapped discriminant printed `SUCCESS`, `name()` prints `UNKNOWN`. - `bun-install-lifecycle-scripts.test.ts` on Linux: 119 pass, 3 fail because `node` is not on PATH in the container (`node: command not found`). Unrelated to this change. - A first version of this PR added a source lint that banned `str>::from(...get_errno())`. A review pointed out that the spelling is a property of the type and that the lint had no positive control and missed the let-bound form, so the type-level fix above replaced it. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/translate-uv-error-windows.test.ts, test/js/node/fs/rm-windows-ntstatus.test.ts, test/cli/install/bad-workspace.test.ts <!-- robobun:evidence:end -->
…ansmuting (#40720) ### Problem - `SystemErrno::from_raw` (`src/errno/lib.rs`) and the Linux raw-syscall `impl GetErrno for usize` (`src/errno/linux_errno.rs`) built the `#[repr(u16)]` enum with `transmute::<u16, E>`. The only guard was a `debug_assert!(n < MAX)` or an `n < 4096` range check. Linux declares 134 variants, so any larger code is an invalid enum value in a release build (a `match` on it can jump anywhere). A debug build dies with `panic: assertion failed: (n as usize) < (Self::MAX as usize)`. - Such codes reach userspace. FUSE filesystems pass through any errno below 512. Driver-internal codes such as `ENOTSUPP` (524) and `ERESTARTSYS` (512) leak out. Every libc `get_errno(rc)` path (`impl_get_errno_libc!`) and `E::from_raw` caller decoded them this way. ### Fix - Add an `EUNKNOWN` variant to the Linux (134), Darwin (107) and FreeBSD (98) enums, one past the last kernel errno. The Windows enum already has `EUNKNOWN = 134`. Derive `strum::FromRepr` on the POSIX enums. - `from_raw` is now a total function: `from_repr(n)` for a declared discriminant, `EUNKNOWN` for anything else. `init` decodes through `from_repr` too. The errno crate has no transmute left. The Windows `E::from_raw` gets the same shape, with `UNKNOWN` as the fallback. - `Error::get_errno` and `e_from_negated` fell back to `SUCCESS` for a code they could not decode. An `Error` is a failure, and the shell builtins turn that `SUCCESS` into exit code 0. They fall back to `EUNKNOWN` now. `from_errno` and `to_zig_err` keep their `EIO` fallback. - Verified: `test/js/node/fs/fs-stat-seccomp-linux.test.ts`, new block "node:fs errno outside the SystemErrno table" (a seccomp filter makes `fsync(2)` fail with 524, `fs.fsyncSync` must report `code: "EUNKNOWN"`). Also `cargo test -p bun_errno`, `cargo check --workspace` for the linux, darwin, windows, freebsd and android targets, `test/js/node/fs/fs.test.ts`, `test/js/node/test/parallel/test-uv-errno.js`. ### Background - `SystemErrno` (alias `E` on POSIX) is the per-OS errno enum. Rust requires a `#[repr(u16)]` enum to hold a declared discriminant at all times. A value that is not one is undefined behaviour at the point of construction, whether or not a `match` ever runs. - `strum::FromRepr` generates `const fn from_repr(u16) -> Option<Self>`, an exhaustive match over the declared variants. It is the checked constructor the Windows enums already used. - `get_errno(rc)` turns a syscall return value into an `E`. On libc targets it reads the thread-local errno after a `-1`. On Linux raw syscalls the kernel returns `-errno` in the result register, and any value in `-4095..=-1` is an error, a range wider than the table. - `bun_sys::Error` stores the errno as a plain `u16` and decodes it on read. A code outside the table already reported as name `UNKNOWN` with the real negative number on that path. `get_errno(rc)` has no room for the raw number because `E` is an exhaustive enum, so the new variant is what it reports. JS sees `code: "EUNKNOWN"` and `errno: -134` on Linux. <details><summary>Notes</summary> Reproduction, both on the release binary and on a debug build. The seccomp helper from the test makes `fsync(2)` return 524, then runs `fs.fsyncSync(fd)`: ``` release (1.4.1-canary.1): {"errno":-524,"syscall":"fsync","message":"Unknown Error, fsync"} (the invalid enum value happened to survive as its bit pattern; no `code`) debug: panic: assertion failed: (n as usize) < (Self::MAX as usize) <bun_errno::linux_errno::SystemErrno>::from_raw src/errno/lib.rs:290 <i32 as bun_errno::GetErrno>::get_errno src/errno/lib.rs:20 ...::MaybeSysResultExt<()>>::errno_sys::<i32> src/runtime/node/node_fs.rs:70 <bun_runtime::node::fs::NodeFS>::fsync src/runtime/node/node_fs.rs:5244 with this change: {"errno":-134,"code":"EUNKNOWN","syscall":"fsync","message":"EUNKNOWN: unknown error, fsync"} ``` Design choice: the fallback is a variant, not `EIO`. Windows already models unmapped errors this way, the result round-trips through `init` (`init(134)` is `Some(EUNKNOWN)`, so a stored `EUNKNOWN` never reads back as `SUCCESS`), and it keeps the user-visible name honest. Node on Linux reports `errno: -524, code: "Unknown system error -524"` for the same call. The raw number is not preserved here because `get_errno(rc)` returns the enum. The `check!`-style wrappers in `bun_sys` (`Error::from_code_int`) store the raw number and are unchanged. Placement: `EUNKNOWN` sits at the old `MAX`, and `MAX` grows by one, so the enums stay dense and `enum_map::Enum`, `LIBUV_ERROR_MAP`, `COREUTILS_ERROR_MAP` and `system_errno_max_dense()` keep working without changes. Both maps fill the new slot with "unknown error". `os.constants.errno` and `util.getSystemErrorMap()` are built from other tables and do not change. `impl_get_errno_libc!` now converts the `c_int` errno with `u16::try_from` instead of `as u16`, so a value that does not fit cannot wrap onto another variant. Related open PRs. #38914 fixes the same transmute as half of a larger change (release-mode `assert!` in the `ZStr`/`WStr` constructors). It makes `from_raw` panic on an undeclared code and routes OS codes to `EIO`. This PR takes the variant approach and touches only the errno path. The two conflict in `src/errno`. #30924 makes `from_raw` panic on OS input. #39340 renames the Windows `EUNKNOWN` strum name to `UNKNOWN`. If it lands, the three POSIX variants added here want the same `#[strum(serialize = "UNKNOWN")]`. Other suites run: `test/js/bun/shell/commands/rm.test.ts` (its "replaced by a symlink during deletion" case runs 4.7 to 5.1 s against the 5 s default in this ASAN container and times out on some runs, with and without the change), `test/js/bun/sys/error-name-from-libuv.test.ts`, `cargo clippy -p bun_errno -p bun_sys`, `cargo fmt --check`. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/fs-stat-seccomp-linux.test.ts <!-- robobun:evidence:end -->
|
Heads-up from the errno dedupe: #40720 is on main (bd630c1). It adds an So "POSIX is unaffected" in the description no longer holds. If this rename is the chosen spelling, the three POSIX variants want the same |
Problem
err.code === "EUNKNOWN"with a message startingEUNKNOWN: unknown error, .... Example: spawning a file CreateProcess refuses (libuv reportsUV_UNKNOWN, -4094) throughBun.spawn,Bun.spawnSync,child_process.spawnorchild_process.spawnSync.code: "UNKNOWN"for the same file (output below), because libuv's name forUV_UNKNOWNisUNKNOWN(uv_err_name,src/uv-common.c);EUNKNOWNexists nowhere in libuv or node. Bun's ownutil.getSystemErrorName(-4094)(src/errno/lib.rs:52) andprocess.binding("uv").errname(-4094)(src/jsc/bindings/ProcessBindingUV.cpp:94) also sayUNKNOWN, so bun disagreed with itself.bun_sys::Error::get_error_code_tag_name(src/sys/Error.rs:329) uses the strum name of the resolvedSystemErrnovariant as the code, andto_system_errorputs the same string into the message. The Windows enum names that variantEUNKNOWN(src/errno/windows_errno.rs:554). Every other variant's Rust name is already the node spelling (ENOENT,ECHARSET,EOF,EFTYPE, ...); this one is the only mismatch. It is reached byUV_UNKNOWNitself, bytranslate_uv_error_to_e's fallback for uv codes with noErow, and by theunwrap_or(EUNKNOWN)fallbacks insrc/sys/windows/mod.rsandsrc/sys/lib.rs.Fix
#[strum(serialize = "UNKNOWN")]onSystemErrno::EUNKNOWN(the fixing line). The Rust identifier stays, so the existingSystemErrno::EUNKNOWN/E::EUNKNOWNuses are unchanged; only the rendered name changes, and it changes in one place for every path that renders it:err.code, the message prefix,Error::name(),Display for SystemErrno, and the per-crateError::name()impls.E::_2BIG(serialize = "2BIG"). Nothing parses aSystemErrnofrom a string (nofrom_str/parseusers of theEnumStringderive), so the serialize attribute has no other effect.Error::name()already returned the literal"UNKNOWN"when the errno cannot be resolved at all, so the two fallbacks now agree too. POSIX is unaffected: itsSystemErrnohas no such variant.StandaloneModuleGraph.rsformatted this errno with{:?}, which prints the Rust identifier and would have kept sayingEUNKNOWN; it now uses{}, which goes through the same strum name as everything else.Listener.rsintentionally keeps routingEUNKNOWNthrough its generic "Failed to listen" error (that variant is also the fallback for unmapped codes, so it does not identify the failure); only its comments changed, since they justified the exclusion by the old spelling.src/js/node/child_process.ts:1168(#handleOnExit,exitCode < 0branch) is intentionally left alone: that literal is a JS-sideerr.errnovalue (itscodeisERR_CHILD_PROCESS_UNKNOWN_ERROR), not a rendering of this variant, and the native side never passes a negative exit code (Subprocess::get_exit_codereturns theu8ornull; aStatus::Errarrives as theerrargument), so the branch is not reachable today. Reshaping it to node'serrnoExceptionform is a separate change.src/js/internal/fs/cp.tsandcp-sync.tsalready special-caseerr.code === "UNKNOWN"(ported from node's Windows readlink workaround); that branch becomes reachable on bun now.it.if(isWindows)/test.skipIf(!isWindows); they fail on the unfixed build and pass with it, checked on windows-x64 withUSE_SYSTEM_BUN=1vsbun bd):test/js/bun/sys/error-name-from-libuv.test.ts: the existing4094assertion updated from"EUNKNOWN"to"UNKNOWN", plus a test thatError.name()agrees withutil.getSystemErrorName(-4094)and that a uv code with no constant (4000) folds to the same name.test/js/bun/spawn/spawn.test.ts:Bun.spawnSyncandBun.spawnon a non-PEbad.exethrow{ code: "UNKNOWN", errno: -4094, message: "UNKNOWN: unknown error, uv_spawn" }.test/js/node/child_process/child_process.test.ts:spawnthrows andspawnSyncreturns{ code: "UNKNOWN", errno: -4094 }with node'ssyscallvalues; the expected values are node v26.3.0's output for the same file.bad.exeyields-4094on both windows-x64 and windows-aarch64 (checked with the released bun on both), so the precondition holds on both Windows CI lanes.spawn.test.tsandchild_process.test.tsfiles,test/js/node/fs/translate-uv-error-windows.test.ts,rm-windows-ntstatus.test.ts,test/js/bun/net/named-pipe-listen-error.test.ts,test/js/node/util/util.test.js.path(POSIX ones and node do); that is a separate defect inspawn_process_windowsand is tracked separately, not changed here.Background
bun_sys::Errorstores an errno as anEdiscriminant. On Windows there is no host errno, so bun keeps two parallel#[repr(u16)]enums with identical discriminants:E(bare names,UNKNOWN = 134) for matching in Rust code, andSystemErrno(E-prefixed names,EUNKNOWN = 134) whose variant names double as the node-style error codes shown to users. Win32 and libuv error codes are folded into these tables; anything without a row lands on discriminant 134.IntoStaticStrderive turns a variant into its name as a&'static str;#[strum(serialize = "...")]overrides that string for one variant without renaming it.UV_UNKNOWN(-4094) is libuv's own "no errno for this" code. node'serr.errnois the libuv code anderr.codeisuv_err_name()of it, which is whereUNKNOWNcomes from; bun already reportserrno: -4094here, only the name was off.Repro output (windows-x64)
Fixture: 11 bytes that are not a PE image, written to
C:/tmp/bad.exe.node v26.3.0:
bun 1.4.0 canary (before):
this branch (
bun bd):