Repository navigation
Conversation
…o load
The server HMR runtime's registerUpdate started replaceModules and dropped
its promise. DevServer drops the promise registerUpdate returns. A module
that failed to load during a hot update was therefore an unhandled
rejection, and the dev server process exited with code 1. Two ways to get
there: a route module that throws at module scope when it is saved, and a
page saved while a stylesheet it imports has an error ("Failed to load
bundled module 'routes/s.css'").
registerUpdate now handles the rejection and logs the error, as the client
runtime does. The module keeps its failure, so the next request for the
route answers with the error page, and the next save of the file loads it
again.
|
Status: fix pushed, waiting for CI. How I reproduced it:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe HMR runtime now handles rejected module replacement asynchronously. Dev server tests cover module-scope route failures and invalid imported stylesheets, including 500 responses, unaffected routes, and recovery after source fixes. ChangesHMR error recovery
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The dev server now contains hot-update failures without leaving rejected updates unhandled; no merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue Resolution Update the CSS root discovery implementation to leave the failed stylesheet import record unchanged and exclude only the failed stylesheet from Full details: Out of Scope Changes checkExplanation The Resolution Remove the generic HMR rejection handling and route-evaluation test from this PR, or link them to a coding issue that requires server HMR failure handling. Keep stylesheet tests only when they verify the
Comment |
|
The review found no actionable comments. The two pre-merge warnings do not apply, so I made no change:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, targeted fix that mirrors the client runtime's existing pattern.
What was reviewed:
registerUpdatechange:.catch(console.error)matcheshmr-runtime-client.ts:101; error is logged, module keeps itsState.Error, and the route still serves 500 — nothing is silently swallowed.- Checked the not-awaited semantics against the surrounding manifest update loop and
DevServer.finalize_bundledropping the returned promise; the two-line comment states the non-obvious reason. - New
devTestcases inbundle.test.ts: use harnesswrite/patch(rebuild-awaited, no sleeps),{ errors: null }per harness signature, assert body before status, verify sibling route and recovery; appended to the existing file per convention.
Extended reasoning...
Overview
The PR touches two files. In src/runtime/bake/hmr-runtime-server.ts, the un-awaited replaceModules(modules) call inside registerUpdate gains a .catch(err => console.error(err)) so a server-side hot update whose module throws (or whose bundled dependency is missing, e.g. a broken CSS import) logs instead of becoming an unhandled rejection that exits the dev server. Two lines of comment explain why the call is intentionally not awaited (deferred requests read the manifest updated below) and why the rejection must be handled at this site (the Rust DevServer drops the returned promise). In test/bake/dev/bundle.test.ts, two new devTest cases cover a route module that throws at module scope on hot update and a route saved while its imported stylesheet has a syntax error; both assert the failing route serves 500 with the error text, a sibling route keeps responding, and a subsequent good save recovers.
Security risks
None. This is dev-server-only HMR glue in the bake runtime, not shipped to production request handling. No auth, crypto, filesystem, or untrusted-input parsing is touched. The change converts a process-terminating unhandled rejection into a logged error plus a 500 on the affected route — no failure is hidden from the user.
Level of scrutiny
Low-to-moderate. The behavioral change is a single expression that copies the exact shape already in hmr-runtime-client.ts:101-104, so it follows an established in-tree pattern rather than introducing a new error-handling approach. REVIEW.md's "never swallow a failure or signal success on one" was the main check: the error is console.error'd, hmr-module.ts records State.Error on the failed module so later requests re-throw it, and the tests confirm the route answers 500 — the failure remains visible in both the terminal and the browser. The comment satisfies "only comment what the code cannot say" (why not awaited, why caught here).
Other factors
Tests are placed in the existing test/bake/dev/bundle.test.ts alongside related HMR/bundle cases (per CLAUDE.md). They use the devTest harness whose write/patch await the dev server's rebuild acknowledgment, so there are no sleep/setTimeout waits; { errors: null } matches the harness write signature for expected-error builds. Assertions check response body text before status, verify a sibling route keeps working (process alive), and confirm recovery after a good save — including that the recovered stylesheet URL is unchanged and its content updated. No CODEOWNERS entry covers these paths (only *.d.ts is owned). Hunt exit reason was dry_streak with no findings and no ruled-out candidates. No prior reviewer objections on the timeline.
…6270) ### What does this PR do? Hardening follow-up to #46021. It fixes regressions from #46021 and #44618 that were never released, includes more of the open pull requests that fix a crash, and fixes a few crashes that had no pull request. #### Regressions fixed (none was released) - `spyOn(Bun, "$")`, and the same for `"sql"`, `"postgres"` and `"SQL"`, threw "does not support accessor properties yet". - Async `fs.read`, `fs.write` and `zlib.brotliCompress` on a view over a `WebAssembly.Memory` could lose data or copy far more than they needed. - `node:vm` with `vm.constants.DONT_CONTEXTIFY`: `this` in host accessors, prototype handling, `util.types.isProxy`, `Bun.deepEquals`, `assert.deepEqual` and `console.log` of a context. - `zlib` `stream.flush()` threw for arguments it used to accept, and streams had a new enumerable property. - `Bun.Transpiler` `exports.replace` with the name `"default"`. - `process.env.X = 3000` could store the number once the function was JIT-compiled. - `node:sqlite` and `bun:sqlite` could lose rows at exit. - Dev server: stylesheets shared between pages after a build error. - Shell `cp` of a file onto itself exited 1. - `error.stack`: the frame of `require()` reads `at anonymous (unknown)` again. - `import * as process from "process"` followed by `process.on(...)` threw at load. The EventEmitter methods that the module exports now act on `process`. - `delete res.statusCode; res.end()` in a `node:http` handler threw. It sends 200, as in Node. (from #46128) - `node:tls`, `node:https` and `node:http2`: connection teardown and write callbacks behave as in 1.4.3 and Node again. #### Crash fixes Bundler, parser and CSS: #42286, #46092, #46097, #46177, #46239, #36334, #43024, #40769. `bun test`, `--hot` and `--watch`: #30776, #38206, #38974. Shell: #43183, #37743, #46168, #46207, #34900. `node:http`, `node:http2`, `Bun.serve`, streams: #43449, #45380, #44457, #42197, #42485, #38082, #38124, #46074, #45999, #46045. `node:vm`, native addons, workers: #46129, #46198, #45615, #33412, #38031. Dev server: #42790, #42850, #39488. Other: #35991, #38030, #38586, #40526, #41830, #46029, #46078, #46114, and the bug of #42215. #### Behavior changes - `crypto.randomFill(view, cb)` reports `ERR_CRYPTO_OPERATION_FAILED` if it could not fill the view. It used to report success. - A function registered twice on `process` runs twice, as in Node. (#41830) - Under `--hot`, a reload after an uncaught exception cancels the timers that the failed run created. The new run's timers fire, and so do timers and `fs.watchFile` watchers that earlier runs kept on `globalThis`. The reload also resets the exit code to 0. (#38206) - The parent of a Worker no longer gets an `'error'` event for a failed `Bun.serve` request in that Worker. The error is printed, as on the main thread. - `hash.write()` after `digest()` drops the chunk, as in Node. (#35991) - `http2` over TLS with `server.emit("connection", socket)`: `session.socket` is a `TLSSocket` and a `'connection'` listener fires, as in Node. (#38124) - A `slice()` with no type of a body `Blob` has `type === ""`, as in Node. (#46114) - `util.types.isProxy(globalThis)` is false, as in Node. - A write on a `node:net` or `node:tls` socket that is still pending when the connection closes calls back with `ECANCELED`, as in Node. - `HTMLRewriter.on()` throws for a selector that is nested too deeply. - `Bun.Image`: `maxPixels` also bounds PNG metadata, `metadata()` of a GIF reports the size that gets decoded, and a path with a NUL byte throws. (#40526) - `bun build --no-bundle` with the `base64` or `dataurl` loader prints an error. (#36334) - Bundled output for a default-exported function or class expression changes to `var file_default = function name() {}`, as in esbuild. `.name` is unchanged. (#46097) - A Proxy in the prototype chain of a `node:vm` global is allowed. jsdom 30 with `runScripts` needs this. - `bun:sqlite` commits a write that `iterate()` left open when the main thread exits. An explicit `BEGIN` is still rolled back. - When a Worker stops while a native addon's async work is still running, the addon's finalizers run after that work is done, as in Node. - Shell: `mv -n` and `cp -n` don't overwrite, and `cp` reads `R`, `v` and `n` anywhere in a flag group. #### Performance - Ordinary `node:vm` contexts run at the same speed. With `DONT_CONTEXTIFY`, stores to globals and reads of inherited names in a hot loop cost 10 to 20 % more than on `main`. - A `process.env` write costs 20 to 35 % less than after #46021. - An async read into a `WebAssembly.Memory` view looks through the live memories: about 1.5 µs more with one memory. #### Not included - Taken in part, so these should stay open for the rest: #36334, #38974, #39488, #40526, #40769, #42485, #45615, #45999, #46029, #46092, #46114, #46168, #46207, #46239. - Needs a WebKit change: #34655 #39204 #40170 #40253 #41587 #41900 #42699 #42897 #42903 #43084 #43813 #44518 #44949 #45770 #45804 #45827. - #44723 (covered by #46097), #41675, #42753, #38762, #41475, #46016. ### How did you verify your code works? - Every item was reproduced first on release builds of `main`, of its parent and of 1.4.3, and compared with Node v26 where Node has the API. - Every fix has a test that fails on `main` and passes on a debug + ASAN build of this branch, except where that isn't practical (huge inputs, code that only runs on Windows or macOS). - The regressions were checked on grids of calls against the build before #46021 and against 1.4.3. - Bun's own test suites and Node's `test-vm-*`, `test-http2-*` and `test-zlib-*` files for the areas touched.
This branch has not been deployed
2 participants
Problem
error: Failed to load bundled module 'routes/s.css'.A route module that throws at module scope when it is saved gives the same exit.registerUpdate(src/runtime/bake/hmr-runtime-server.ts:162) callsreplaceModules(modules)and drops the promise.finalize_bundle(src/runtime/bake/DevServer.rs:4315) drops the promise thatregisterUpdatereturns. A rejection is unhandled, so Bun ends the process.Fix
registerUpdatehandles the rejection and logs the error. The client runtime already does this (hmr-runtime-client.ts:101).test/bake/dev/bundle.test.ts. Both fail on 1.4.3-canary and on a debug build of main, and pass with the fix. Also rancss,html,hot,esmintest/bake/dev/.Background
hmr-module.ts) in the server's JS VM. After each rebuild,finalize_bundlepasses the new module code toregisterUpdate.replaceModulesevaluates the changed modules again. It is async. It rejects when a module throws, or when a module lists a dependency that the runtime does not have.Notes
Found by a save-burst fuzzer against a framework fixture (fuzz ledger #52267). The minimal sequence:
routes/styled.tsdoesimport "./s.css". Load the page once (200).s.csswith a syntax error.routes/styled.ts. The server printserror: Failed to load bundled module 'routes/s.css'. This is not a dynamic import, and therefore is a bug in Bun's bundler.withregisterUpdate(bake://server-runtime.js) at the bottom of the stack, and exits with code 1.Reproduced here on 1.4.3-canary.1+09bb54630 with the ledger script: exit 1 with the page save, alive with only the stylesheet save or only the page save. The wider case needs no stylesheet: save a route module with
throw new Error("boom")at module scope and the server exits 1.The request for the route can still be answered before the exit, because Bun reports the unhandled rejection after the current event loop tick. The two tests therefore fail on the request that follows (
ConnectionRefused).State of the module after a failed hot update (unchanged loader code in
hmr-module.ts):finishLoadModuleAsyncstoresState.Errorand the error.loadModuleAsyncthrows it again for each later request, so the route answers 500 with that error. A later hot update for the file sets it toStaleand loads it again. The first test checks this (v1, 500 with the message, v3).throwNotFoundfromparseEsmDependencies): the module staysPendingwith its old exports. In the stylesheet sequence this does not show, because the route is in the build-failed state and answers the "Build Failed" page until the stylesheet is fixed, and the fix rebuilds the page. The second test checks this (v1, 500 "Build Failed", v2 with the same stylesheet URL, stylesheet served).The bundler side of the stylesheet trigger:
finish_from_bake_dev_server("Discover all CSS roots",src/bundler/bundle_v2.rs:5621) setsrecord.source_index = Index::INVALIDfor a stylesheet that failed.convert_stmts_for_chunk_for_dev_serverthen does not see a stylesheet import and emits it as a module dependency. #40092 also setsrecord.path.is_disabled = truethere, which drops the import. I applied that one line on top of this branch: the stylesheet sequence logs noFailed to load bundled module, and the two tests from the closed #37844 (HTML<link>tag, script that imports a stylesheet boots after recovery) pass. #39488 carries the sameregisterUpdatechange as this PR and the #37844 variant of the bundler change.The fuzzer saw two more shapes of the same exit (a module deleted together with the stylesheet error and created again, and a renamed module named in the message). They were not minimised. They go through the same unhandled rejection.
One gap stays open and has its own follow-up: when only the stylesheet is saved with an error (step 2), a new GET of the framework route still answers the old page with 200. The failure is recorded on the client graph, and failure indexing does not cross to the server-side importers of a stylesheet.
[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file