Drop the trailing period from the node-shaped AbortError message - #39277
Conversation
Node's AbortError defaults to "The operation was aborted" (no period); the period belongs to the DOMException default, which has its own literal. The shared common string is only used by $makeAbortError and Bun__wrapAbortError, so it now carries node's text.
|
Warning Review limit reached
Next review available in: 10 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 (9)
Comment |
|
Status: merged as 8c5296a. The follow-up self-review pass finished after the merge and raised nothing that needs a change here. Reproduced on bun 1.4.0 and main ( Noticed while here, deliberately not part of this PR (tracked separately): |
There was a problem hiding this comment.
LGTM — one-character literal change bringing node's AbortError message in line with upstream, with thorough test coverage.
What was reviewed:
- Confirmed
OperationWasAbortedis only read byjsFunctionMakeAbortError/Bun__wrapAbortError(ErrorCode.cpp:1748-1772); the DOMException path at :1784 and DOMException.cpp:53 use their own literals and keep the period. - Grepped remaining
"The operation was aborted."assertions intest/— all are DOMException-shaped (fetch, web/abort, serve, undici, callbackfs.watchviasignal.reason, and the newsignal.reason.messagecheck) and are unaffected. - Test edits also strengthen previously-vacuous assertions (the un-awaited IIFE in the
events.onAbortController test;expect-in-catchinfs.promises.watch).
Extended reasoning...
Overview
The runtime change is a single-character deletion in src/jsc/bindings/BunCommonStrings.h: the OperationWasAborted common string drops its trailing period so that node-compat APIs that build node's AbortError (code: 'ABORT_ERR', cause: signal.reason) match Node's exact default message. Eight test files are updated: one upstream node parallel test restored to its original text, five existing test files updated to assert the new message (several also strengthening previously-weak assertions), and two files gaining new coverage for the $makeAbortError() no-arg path and the timers/promises variant matrix.
Security risks
None. This is a one-character change to a user-visible error message string. No auth, crypto, parsing, or memory-safety surface is touched.
Level of scrutiny
Low. The only production change is a string literal. I verified via grep that the only readers of this common string are the two node-shaped AbortError constructors in ErrorCode.cpp (lines 1748, 1751, 1769, 1772), and that the WebIDL DOMException path (CommonAbortReason::UserAbort → createDOMException at line 1784, and DOMException.cpp:53) uses its own separate literal that retains the period. I also confirmed the callback fs.watch abort path emits signal.reason (the DOMException) directly via emit_if_aborted → s.js_reason() in node_fs_watcher.rs, so the three unchanged period-asserting tests in fs.watch.test.ts are correct as-is.
Other factors
The PR description is unusually thorough — it traces the regression to #17179, cites the Node source line, and documents USE_SYSTEM_BUN=1 verification for every changed test. The test edits go beyond a mechanical string swap: the events.on AbortController test previously ran its assertion in a detached un-awaited IIFE (so a failure would have been an unhandled rejection, not a test failure) and now awaits it; the two fs.promises.watch abort tests previously used expect inside a catch (vacuous if the promise resolved) and now use rejects.toThrow. New coverage in timers.promises.test.ts and node-stream.test.js exercises both the with-cause and no-cause $makeAbortError branches and explicitly asserts that signal.reason.message still has its period, guarding against accidentally changing the DOMException text.
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
Problem
AbortError(name: "AbortError",code: "ABORT_ERR",cause: signal.reason) uses the message"The operation was aborted.". Node's message is"The operation was aborted", with no period (lib/internal/errors.js,class AbortError, v26.3.0 L979-L988).timers/promises,fs/fs.promises{ signal },fs.promises.watch,events.once/on, streams (addAbortSignal,finished,Symbol.asyncDispose, ...),child_process,net,http2client requests, andBun.spawn({ signal }), since they all build the error through$makeAbortErrororBun__wrapAbortError.src/jsc/bindings/ErrorCode.cpp:1748,:1751,:1769,:1772) take their default message from theOperationWasAbortedcommon string, defined with the period insrc/jsc/bindings/BunCommonStrings.h:40.$makeAbortErrordefaulted to the period-less text. That PR introduced the common string (with the period) because it was also used for theCommonAbortReason::UserAborterror, whose WebIDL text does end in a period, and patched the two tests that noticed. TheCommonAbortReasonpath has since moved tocreateDOMExceptionwith its own literals (ErrorCode.cpp:1781-1787,DOMException.cpp:53), so the common string is now only read by the two node-shaped paths.Fix
BunCommonStrings.h: theOperationWasAbortedliteral loses its period; that is the whole runtime change. Both consumers ($makeAbortErrorwith and without arguments,Bun__wrapAbortErrorwith and without a cause) read this one string, so every caller listed above changes together.AbortError, and for those node's observed text is the spec. The DOMException defaults (signal.reason,fetchaborts,AbortSignal.timeout) are separate literals and keep their period; the tests asserting them (test/js/web/abort,test/js/web/fetch, the callbackfs.watchtests,undici,serve) are unchanged and still pass.test/js/node/test/parallel/test-timers-promises-scheduler.js: restored to the upstream text byte for byte (blob7caf92fdf6, the pre-Avoid creating temporary strings when throwing errors #17179 version).test/js/node/fs/promises.test.js(Bun__wrapAbortErrorvianode_fs: pre-aborted, in flight, default and custom reasons, callback API).test/js/node/events/event-emitter.test.ts: theon()abort test now awaits its iteration and asserts name/code/message/cause; previously the assertion ran in a detached async function.test/js/node/watch/fs.watch.test.ts(fs.promises.watch): the two assertions now userejects.toThrowinstead of anexpectinside acatch.test/js/node/timers.promises/timers.promises.test.ts: new block for the repro above plussetImmediate/setInterval, in-flight abort, custom reason, and a check thatsignal.reason.messagestill has its period.test/js/node/stream/node-stream.test.js:Readable/Writable[Symbol.asyncDispose], the$makeAbortError()no-argument path (node: no period, nocause).test/js/node/http2/node-http2.test.jsandtest/js/bun/spawn/spawn-signal.test.ts: the existing abort assertions gain the message (Bun__wrapAbortErrorvia the http2 parser and via spawn).bun bd teston the seven.test.*files above passes;bun bd test/js/node/test/parallel/test-timers-promises-scheduler.jsandtest-child-process-exec-abortcontroller-promisified.jsexit 0;USE_SYSTEM_BUN=1(1.4.0) fails each modified file on the message only.git grep "operation was aborted" testleaves only DOMException assertions, all unchanged.Background
DOMExceptionwithname === "AbortError"and numericcode === 20; it is whatAbortController.abort()stores insignal.reason, and WebIDL defines its default message as"The operation was aborted.". Node's is a plainErrorsubclass with stringcode === "ABORT_ERR"; node's abortable APIs reject with it and attachsignal.reasonascause, and its default message has no period.$makeAbortErroris the private global bun's built-in JS modules call to construct node'sAbortError(implemented byjsFunctionMakeAbortErrorinErrorCode.cpp);Bun__wrapAbortErroris the same constructor exported to Rust (AbortSignal::node_abort_error_if_aborted, the http2 parser).BunCommonStrings.hlists string literals that are created once per global object and reused, so an error can be built without allocating its message each time; changing the literal changes every consumer.