Conversation
…no table `Output::err` took the syscall-error branch for every `bun_sys::Error`, and `Error::name()` returns the literal "UNKNOWN" when the errno is not in the table. So an errno the table does not declare (FUSE, odd drivers, Linux ENOTSUPP) printed as `UNKNOWN: <msg> (<syscall>)`. `SysErrInfo::tag_name` is now an `Option` that is `None` for such an errno. `err_with_body` then prints `error: <msg> (<syscall>, errno N)`: no fake code, and the number stays so it can be looked up. Adds the `sysErrorOutputErr` testing hook so a test can render an errno that no syscall in CI returns.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 2:14 PM PT - Aug 28th, 2026
❌ @robobun, your commit df514dc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40854That installs a local version of the PR into your bun-40854 --bun |
|
Status: ready for review. Reproduced with the new
A known errno is unchanged: CI on df514dc: the new tests pass on every lane. The one red job is Related: #40852 touches the same |
There was a problem hiding this comment.
LGTM — the assertion-ordering nit from the earlier review is addressed; tests now assert the combined { stderr, stdout, exitCode } object.
What was reviewed:
SysErrInfo.tag_nametype change: only one producer (impl ErrName for bun_sys::Error) and one consumer (err_with_body), both updated together.sys_error_output_errhost fn: arg validation before use,from_libuvis read on both cfg branches (early-return vs. struct field), no unused-binding warning.- New tests spawn via
bunExe()/bunEnvwith-e, drain both pipes concurrently, and aretest.concurrent.
Extended reasoning...
Overview
The PR changes how bun_core::output::err renders a bun_sys::Error whose errno is not in the SystemErrno table: instead of printing UNKNOWN: <msg> (<syscall>), it now prints the generic error: <msg> (<syscall>, errno N) form so the raw number is preserved. Mechanically, SysErrInfo.tag_name becomes Option<&'static str> (was &'static [u8]), the sole producer in src/sys/Error.rs populates it from get_error_code_tag_name() (which returns None for out-of-table errnos), and err_with_body gains an early branch for the None case. The known-errno paths just swap bstr::BStr::new(bytes) for direct &str display — same rendered output. A test-only host function sys_error_output_err is added and wired through dispatch_js2native.rs and internal-for-testing.ts, and two test.concurrent cases in test/js/bun/sys/error-name-from-libuv.test.ts cover both the known-errno (2 → ENOENT) and out-of-table (524) paths.
Security risks
None. This is CLI error-message formatting on a static SysErrInfo struct with &'static str fields. The new host function is confined to bun:internal-for-testing, validates its two arguments (is_number() / is_boolean() / u16::try_from) before use, and only writes to stderr. No user-controlled data reaches allocation, filesystem, or network paths.
Level of scrutiny
Low-to-moderate. The behavioral surface is a single formatted line in an error path that previously printed a less-useful literal. I confirmed via grep that SysErrInfo has exactly one construction site and one read site, both updated in this diff, so the field-type change cannot silently break another producer. The get_error_code_tag_name() helper already existed and is what Error::name() itself wraps, so the semantics of "None means not in the table" are established. Cross-platform: the from_libuv binding is consumed on both #[cfg] branches (non-Windows early-return, Windows struct field), so no dead-binding lint on either target.
Other factors
My earlier inline nit (assert exit code last) was addressed in a follow-up commit — the helper now returns { stderr, stdout, exitCode } and each test asserts the whole object with .toEqual, matching the CLAUDE.md/REVIEW.md convention. The tests follow harness patterns (bunExe(), bunEnv, -e for single-file spawn, concurrent pipe draining via Promise.all, test.concurrent). The change is ~60 net lines, self-contained, and the PR description accurately describes the before/after output. No outstanding third-party CHANGES_REQUESTED reviews in the timeline.
…al-field cells, Http2Response, and the WebView ObjC wrappers (#42816) ### Problem - Four native class members exist only for built-in JS, and nothing calls them: the SQL connection `connected` getter and `onconnect` accessor, `NodeHTTPResponse.ended`, and `crash_handler.getFeaturesAsVLQ`. - Thirteen `JSInternalFieldObjectImpl` subclasses carry a copied `static size_t allocationSize(Checked<size_t>)`. Each `create()` uses `allocateCell<T>(vm)`, which takes `sizeof(T)`. JavaScriptCore calls `allocationSize` only on its own classes. - Other leftovers have no reader: two WebCore members, eight `Http2Response` members and the `socketData` field behind one of them, two `ObjCRuntime` wrappers, `.typos.toml`, and `packages/bun-error/img/*`. ### Fix - Delete each item: 31 files, 154 lines and 4 images removed. The Notes list every symbol. - Correct because every removed name has zero references in `src/`, `packages/`, `scripts/`, `test/` and `build/debug/codegen/` outside its own definition. No removed member is documented or typed API. - Verified: `bun bd` and `bun run rust:check-all` (12 targets) pass. The sql, `node:http`, streams, `serve-http2`, node error-code, performance and crash-handler test files pass. `ObjCRuntime` compiles only on macOS, so CI is the compile check for those 13 lines. ### Background - A `*.classes.ts` file declares the members of a native class. The generator emits the C++ wrapper and the Rust glue. The generated thunk is `#[no_mangle]`, so neither rustc nor the linker reports a member that no JS reads. - `values: ["onconnect", ...]` in `sql.classes.ts` declares a GC-visited slot on the wrapper. Native code uses the slot through `onconnect_get_cached` / `onconnect_set_cached`. The removed accessor was a JS-facing route to the same slot. The slot stays. - 34 other dead-code pull requests are open. This one deletes nothing that they delete. Six files overlap on other lines (listed in the Notes). <details><summary>Notes</summary> No test is added. The change deletes code that has no user, so it has no behavior to cover, and REVIEW.md says not to add tests that check dead code stays dead. Overlap with open pull requests: - No deleted line is a line that one of the 34 open dead-code pull requests deletes (checked line by line against their diffs). - Six files also change in one of them, on other lines: `scripts/glob-sources.ts` (#40232), `src/jsc/bindings/ErrorCode.h` (#39929), `JSOneShotDirectSink.h` (#41385), `JSStreamAlgorithmContexts.h` (#41445), `src/runtime/api/crash_handler_jsc.rs` (#41385), `src/sql_jsc/postgres/PostgresSQLConnection.rs` (#40824). - The diffs of the 227 open pull requests (newest 2,500) that touch the `node:http`, sql, crash-handler, streams-header, webview or uws HTTP/2 files add no user of a removed name. Removed, one line each: - `PostgresSQLConnection.connected` / `MySQLConnection.connected` and both `get_connected`. Every `.connected` in `src/js/internal/sql/` and `src/js/bun/sql.ts` is `PooledConnectionState.connected`. `test/js/sql` mentions it in comments only. - `PostgresSQLConnection.onconnect` / `MySQLConnection.onconnect` accessor and the two `(get_on_connect, set_on_connect => ...)` lines of `cached_prop_hostfns!`. Every JS `onconnect` is on the options object (`connectionInfo.onconnect`, `ret.onconnect`). - `NodeHTTPResponse.ended` / `get_ended`. The `_http_*` modules read `handle.flags & NodeHTTPResponseFlags.ended`. The other `.ended` hits are `queued.ended`, `_readableState.ended`, and the JS handle in `http1_server_fallback.ts`. - `crash_handler.getFeaturesAsVLQ` / `js_get_features_as_vlq`, and the `BoundedArray` import that only it used. `getFeatureData` and the other entries have users in `test/`, `scripts/` or `packages/bun-release`. - `allocationSize` in `InternalModuleRegistry.h`, `ErrorCode.h` (`ErrorCodeCache`), `BunStreamSource.h`, `JSAsyncIteratorSourceOperation.h`, `JSDirectStreamSource.h`, `JSOneShotDirectSink.h`, `JSReadRequest.h` (2), `JSReadStreamIntoSinkOperation.h`, `JSReadableStreamIntoArrayOperation.h`, `JSStreamAlgorithmContexts.h`, `JSStreamTeeState.h`, `JSStreamPipeToOperation.h`. `rg allocationSize src packages build/debug/codegen` found the 14 definitions and no call. The debug objects contain no `allocationSize` symbol for a bun class. The fourteenth copy, in `JSCrossRealmTransformState.h`, stays because #41445 deletes that file. - #41268 lists `InternalModuleRegistry::allocationSize` under "Kept on purpose" because it corrects the base class version, which returns `sizeof(JSInternalFieldObjectImpl)`. That reason does not hold. Nothing calls either version for these classes, `allocateCell<T>(vm)` sizes the cell from `sizeof(T)`, and four other subclasses (`ModuleLoader.h`, `JSSocketHandlers.h`, `JSNextTickQueue.h`, `JSMockFunction.h`) never had the copy. - `JSPerformanceResourceTiming::protectedWrapped`. The callers of `protectedWrapped()` are on `JSDOMURL`, `JSAbortSignal` and the `CookieMap` wrapper. - `ResourceLoadTiming::isolatedCopy`. No caller. - `Http2Response::getWriteOffset`, `overrideWriteOffset`, `getSocketData`, `prepareForSendfile`, `uncork`, `isCorked`, `getNativeHandle`, `isConnectRequest` (`packages/bun-uws/src/Http2Context.h`), and `Http2ResponseData::socketData`, which only `getSocketData` read and nothing wrote. The callers of the last four names in `libuwsockets.cpp` are on `HttpResponse<SSL>` and `AsyncSocket<SSL>`. `libuwsockets_h2.cpp` uses none of the eight. #40294 removes the same-named members of `HttpResponse` and `Http3Response`. - `ObjCRuntime`: `struct NSObject` (only member: `describe()`), `Ref::s_description`, `WKWebView::isLoading()`, `WKWebView::s_isLoading`. Each name has only its declaration, definition and `sel(...)` initializer. - `.typos.toml`. Its runner, `.github/workflows/typos.yml`, was deleted in 126f468 (2025-11-05). - `packages/bun-error/img/{close.png,error.png,powered-by.png,powered-by.webp}`, last touched 2021-09-11. `bun-error.css` uses inline `data:` URIs. The `img/*` glob in `scripts/glob-sources.ts` goes with them. Tests run with the debug build: `test/js/sql/sql-onconnect-onclose-throw.test.ts` (9), `sql-connection-socket-uaf.test.ts` (2), `sql-mysql-clean-reentry.test.ts` (1), `sql-mysql.test.ts`, `sql.test.ts`, `test/js/node/http/node-http.test.ts` (159), `node-http-uaf.test.ts` (9), `test/js/web/streams/streams.test.js` (563), `test/js/bun/http/serve-http2.test.ts` (90), `test/js/web/timers/performance-entries.test.ts`, `test/cli/run/run-crash-handler.test.ts` (28), `test/js/node/errors/` (2 files), `test/js/bun/util/error-code-mirror.test.ts`. How the candidates were found: - A repo-wide identifier index (definitions with zero other mentions, comments stripped). - A compiler pass over a scratch copy of the workspace: every `pub` item that no other crate names becomes `pub(crate)`, then `cargo check --force-warn dead_code` runs for linux (dev and release), windows and macos. A loop restores `pub` at each definition that a privacy error points to. 131 leaf items came out of it. - A second compiler pass marks each of those 131 items `#[deprecated]` in an unmodified copy and checks all four configurations again. 88 had a real user. The first pass misses them because a private inherent method silently loses method resolution to a trait method of the same name. 43 had none. - Manual review dropped all 43: unit-test users (`CowSlice::init_dupe`, `clap::SliceIterator`, `Imports::ALL_SORTED`, the `h2::Connection` send path), names that `generate-classes.ts` emits (`host_fn_setter`, `host_fn_this_value`), a FreeBSD-only user (`O::EVTONLY`), the `__IsFreeze` autoref const, and errno or fault-injection tables that mirror C. - A mark-and-sweep over the relocations of the `-O0 -ffunction-sections` debug objects for C++. Almost every hit is in a function that one of the open pull requests already deletes. - A per-binding comparison of every native object and `*.classes.ts` member that built-in JS consumes against `src/js`, `packages/bun-types` and `test/`. Self-reviewed: three changes requested, all made. The other four `Http2Response` stubs and `socketData` go too, three more `allocationSize` copies go too, and the `sys/Error.rs` entry is back. Found, not deleted here: - `"sys/Error.rs"` in `rustIdentifierPaths` (`src/codegen/generate-js2native.ts`): no `$newRustFunction` call site on main since 2b3f660, but #40854 adds one. - `FrameworkFileSystemRouter.match` (the `bun:internal-for-testing` router binding, 67 lines with `route_to_json_inverse`): no user on main, but #40731 and #33232 add tests that call it. - `bun_core::strings::split_once`: no user, but #40824 deletes the adjacent `rsplit_once`. - The SQL `queries` getter is never read, so `queries_set_cached` never runs and `get_queries_array()` always returns `undefined`. The `if (queries)` branches in `onResolve*`/`onReject*` (`mysql.ts`, `postgres.ts`) and the `queries` argument of about 12 native call sites are dead. That is a refactor of the resolve path, not a deletion. - `NodeHTTPResponse.ref`: no JS caller, but `unref` has one, and a one-sided pair reads like a bug. - `MySQLConnection.ref`/`unref`: no caller, but `sql.classes.ts` generates both drivers from one loop. - 36 of the 48 `X_getter` functions that `WEBCORE_GENERATED_CONSTRUCTOR_GETTER` defines in `ZigGlobalObject.cpp` have no user. The macro also defines the live `XConstructorCallback`, so this needs a macro split. - `IDLByte`, `IDLShort` and `IDLLongLong` converters: never instantiated, but `src/codegen/bindgen.ts` can emit the type names. - `ERR_REDIS_INVALID_DATABASE` in `ErrorCode.ts`: no user, but three open pull requests edit the adjacent lines. - `internalBinding("quic")` constants that `quic.ts` does not read: the object mirrors Node's list and is returned whole. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts <!-- robobun:evidence:end -->
Problem
Output::errprints abun_sys::Errorwhose errno is not in theSystemErrnotable asUNKNOWN: <msg> (<syscall>). "UNKNOWN" is not an errno code. The plainerror: <msg>form, with the number, is more useful.impl ErrName for bun_sys::Erroralways returnsSome(SysErrInfo { tag_name: Error::name(self), .. })(src/sys/Error.rs:522), andError::name()falls back to the literal"UNKNOWN"(src/sys/Error.rs:319). Soerr_with_body(src/bun_core/output.rs:2400) always takes the syscall-error branch.Fix
SysErrInfo::tag_nameis nowOption<&'static str>. Thebun_sysimpl sets it fromget_error_code_tag_name(), so it isNonewhen the errno does not resolve.err_with_bodyprintserror: <msg> (<syscall>, errno N)for that case. The known-errno forms do not change:ENOENT: No such file or directory: <msg> (open).test/js/bun/sys/error-name-from-libuv.test.ts. Before the fix the new test getsUNKNOWN: sysErrorOutputErr (open). After:error: sysErrorOutputErr (open, errno 524). Alsotest/internal/sigaction-layout.test.tsandtest/internal/source-lints.Background
Output::err(name, fmt, args)is the CLI error printer. For abun_sys::Errorit renders<code>: <strerror>: <msg> (<syscall>). TheSysErrInfostruct is howbun_sys(a higher crate) hands the pieces tobun_corewithoutbun_corenaming the error type.SystemErrnois bun's per-OS errno enum. The kernel is not bound to it: FUSE filesystems and some drivers return values it does not declare, for example LinuxENOTSUPP(524).sysErrorOutputErr(errno, fromLibuv)hook inbun:internal-for-testing. It builds thebun_sys::Errorand callsOutput::erron it. The hook is the same one Output.err: print the errno description for libuv errors on Windows #40852 adds, so the two branches merge without a second copy.Notes
from_libuverrors on Windows in the sameas_sys_err_info. The two changes compose: that PR changes theerrnofield, this one changestag_name. Whichever lands second has a small conflict inas_sys_err_infoand in the test file.Error::name()keeps its"UNKNOWN"fallback. Other callers use it in free-form messages (failed to open: {name}), which this request does not cover.E::EUNKNOWN(viaE::from_raw) is in the table and still prints asEUNKNOWN: <msg> (<syscall>). The raw number is lost before theErroris built, so there is nothing better to print there.cargo fmt --check,cargo clippy -p bun_core -p bun_sys -p bun_sys_jsc, and prettier on the touched files.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/bun/sys/error-name-from-libuv.test.ts