Repository navigation
Conversation
… before it was parked defer_request writes a 413 or 400 response and parks nothing when prepare_and_save_js_request_context returns None. ensure_route_is_bundled still queued the route, set it to Bundling, and started a bundle that finalize_bundle could never settle. The next request for the route then panicked with "infallible: bundle active". on_defer now reports whether the request was parked. When it was answered, the route stays Unqueued and no bundle starts. Fixes #42838
|
Updated 8:28 PM PT - Sep 15th, 2026
✅ @robobun, your commit 9ad375714ab4183ae21c53a19cefd3494067d8ea passed in 🧪 To try this PR locally: bunx bun-pr 42850That installs a local version of the PR into your bun-42850 --bun |
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesDeferral outcome handling
Priority: ⚪ Pending latest changes Merge Risk: ⚪ Minimal · up to The change keeps answered requests out of bundling while preserving recovery for later requests; no concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether moving on_defer ahead of route_queue.put changes the bundleNewRoute (promise) path — it does not, since PromiseEnsureRouteBundledCtx::on_defer already does the same route_queue.put for the route before the outer put runs, and always returns Parked. The Answered early return in defer_request also leaves the claimed deferred_request_pool slot released without drop_in_place, matching the pre-existing None path.
Extended reasoning...
Findings were reported inline, so this body only records what else was examined. The reorder in the three Unqueued sub-paths is behavior-preserving for the promise context because that context's on_defer itself puts the route into route_queue (idempotent second put afterwards) and registers the promise; the request context's Answered path returns before any state write, and the pool slot claimed in defer_request is dropped uninitialized exactly as on the old None path. The remaining concerns (test hang wiring, test flakiness against the unfixed build, uncovered sibling sites, and the still-live waiter-less advancement paths in start_next_bundle_if_present/on_plugins_rejected) are covered by the inline comments and are the reason a human should look.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/bake/DevServer.rs— Dev server users whose deferred route has nothing left to bundle still get thepanic: infallible: bundle activeabort this PR's title promises to remove.start_next_bundle_if_presentmarks every queued routeBundlingat DevServer.rs:4961, then skipsstart_async_bundleat DevServer.rs:4966 whenentry_pointsis empty, so the route staysBundlingwithcurrent_bundleNone and its parked request is never answered. Fix: when no bundle starts, settle the queued routes (mark themLoadedorPossibleBundlingFailuresand replaynext_bundle.requests) instead of leaving themBundling; the referenced #42830 is not in this checkout. [also at: src/runtime/bake/DevServer.rs:4961 - Clients whose request was parked for a queued route are never answered when the next bundle has nothing to compile, and the route is leftBundlingwith no bundle.]Extended reasoning...
The dismissal relied on the PR text saying #42830 covers this. git log --all --grep=42830 finds nothing in this checkout, so after merge the path is live. Trigger: bundle A is active for route /a whose page module imports /b's page module. A request for /b arrives, hits the Unqueued arm at DevServer.rs:1939, on_defer returns Parked, /b is put in route_queue and set DeferredToNextBundle. Bundle A finishes and bundles /b's files as imports. finalize_bundle_cleanup calls start_next_bundle_if_present at DevServer.rs:3799. next_bundle.requests.first is non-null, so the loop at 4958 sets /b to Bundling and append_route_entry_points_if_not_stale adds nothing because every file is already in bundled_files. entry_points.set is empty, start_async_bundle is skipped at 4966, route_queue is cleared at 4971. /b is Bundling with current_bundle None; the parked request stays in next_bundle.requests. The next request for /b enters the Bundling arm at DevServer.rs:2068, on_defer(CurrentBundle) hits .expect("infallible: bundle active") at line…
Verification: pre-existing (acknowledged in diff: the PR description defers this path to "#42830", which is not in this checkout —
git log --all --grep=42830is empty; the note that it is "a different entry into the same panic" is accurate). Triggering condition: a route is parked asDeferredToNextBundlewhile another bundle is active, and that bundle happens to bundle all of the deferred route's… -
🟣
src/runtime/bake/DevServer.rs— After a plugin load fails, every later request for a route that was queued behind the plugins hangs forever, and debug builds trip an assertion.on_plugins_rejectedat DevServer.rs:6229-6238 aborts the parked requests and clearsroute_queuebut never resets those routes'server_state, so they stayDeferredToNextBundle. The arm at DevServer.rs:2056-2064 then parks each new request innext_bundle.requestswith nothing scheduled to start a bundle, and thedebug_assertat 2057 fires. Fix: any path that drops waiters without a bundle (on_plugins_rejected, the empty-entry-points skip) must also return the affected routes toUnqueued, so the next request re-enters the plugin-error or fresh-bundle path.Extended reasoning...
The finder filed this as pre_existing because the diff does not touch these entries. The PR's own thesis is that a route must never be advanced without a waiter, and this PR keeps that invariant only at three sites while the sibling site that removes waiters is left inconsistent. Trigger: cold server, first request for /a. Unqueued arm, plugin_state Unknown, get_or_load_plugins returns Pending (DevServer.rs:1983), plugin_state becomes Pending, the request is parked at 2003 and /a is put in route_queue and set DeferredToNextBundle at 2007. The user's plugin setup throws. on_plugins_rejected runs: plugin_state = Err, the parked request is aborted with an error page, route_queue is cleared at 6238. /a's server_state is still DeferredToNextBundle. The user fixes nothing yet and reloads /a. ensure_route_is_bundled reads DeferredToNextBundle, debug_assert at 2057 fails in debug builds; in release, on_defer(NextBundle) parks the request in next_bundle.requests and returns. No current bundle exists, plugins are Err, nothing calls start_next_bundle_if_present, so the browser spins until the idle…
Verification: pre-existing (the base already fails by the same route; this diff touches the Pending arm that produces the state but does not introduce or widen the defect; PR description explicitly notes
on_plugins_rejected"can still advance a route without a waiter" and leaves it out of scope). Trigger: a server with asyncserve_pluginswhose plugin load rejects after at least one framework-route…
…ixed code A request for a second route waits until the stray bundle ends, so the next request for the first route always finds the route stuck. Reject the raw-socket promise on close so that a dropped connection fails fast.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review follow-up, as of 9ad3757:
|
|
A second branch reached the same fix (the same three guards) before this PR existed. That branch is dropped, and no second PR will be opened. It had two tests that this PR does not have. They cover the two guards that the test here does not reach:
Both tests are in one test-only commit on top of this PR's head (9ad3757): 8991d95 on git fetch origin robobun/5946ccf5/dev-server-413-more-tests && git merge --ff-only FETCH_HEADChecked on a debug build with the
|
…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.
Problem
Bun.serve({ app: { framework } }), behind the bake feature flag). The first request for a cold route is rejected with 413 because itsContent-Lengthis overmaxRequestBodySize. The next request for that route aborts the process withpanic: infallible: bundle active(release) orassertion failed: dev.current_bundle.is_some()(debug).DevServer::defer_request(src/runtime/bake/DevServer.rs:2116) returns without parking the request whenprepare_and_save_js_request_contextreturnsNone: the response is already written.ensure_route_is_bundled(line 1927) did not know that. It still queued the route, set it toBundling, and started a bundle with nothing parked.finalize_bundlesettles a route only through its parked requests and promise, so the route stayedBundlingafter the bundle ended.Fix
on_deferanddefer_requestreturn a newDeferralenum:ParkedorAnswered.ensure_route_is_bundledcallon_deferfirst. OnAnsweredthey return beforeroute_queue.put, the state write, andstart_async_bundle. The route staysUnqueued, so the next request bundles it as usual.bundleNewRoute) always returnsParked. Its behavior does not change.test/bake/dev/bundle.test.ts(new test, fails on main with the panic above). Alsotest/bake/framework-router.test.ts,test/bake/dev/plugins.test.ts,test/bake/dev/html.test.ts.Background
server_state:Unqueued,DeferredToNextBundle,Bundling,PossibleBundlingFailures, orLoaded.ensure_route_is_bundleddrives a request through these states.Loadedis parked:defer_requestsaves it incurrent_bundle.requestsornext_bundle.requests.finalize_bundlewalks that list, marks each waiting routeLoaded, and replays the requests.prepare_and_save_js_request_context(src/runtime/server/server_body.rs) can answer the request itself before it is saved: 413 for a body over the limit, 400 for HTTP/3 withtransfer-encoding. That is theAnsweredcase.Notes
HandlerKind::ServerHandler(framework routes underBun.serve({ app })). HTML bundle routes useBundledHtmlPageand never take theAnsweredpath. The report (dev server: a request rejected before it is parked (413) leaves its framework route inBundling, and the next request panics #42838) came from a code read, not from a user.DeferredToNextBundleinroute_queuewith nothing parked. The first site in theUnqueuedarm covers that case too.BundlingandDeferredToNextBundlearms also callon_defer. OnAnsweredthey already return without a state change, so they need no edit.append_route_entry_points_if_not_staleruns beforeon_deferin the last site. It only reads the graphs, so the early return leaves no partial state.start_next_bundle_if_presentandon_plugins_rejectedcan still advance a route without a waiter. Those are separate paths and stay out of this diff.bun repro413.mjsprintsPOST -> 413,GET /about -> connection ECONNRESET,server DIED SIGABRTon main. With this diff:GET /about -> 200 <body>about</body>,server alive.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The dev server moved a cold framework route into the
Bundlingstate before it knew whether the request could actually be parked as a waiter, so a request answered early (for example with 413 for an oversizedContent-Length) left the route marked as bundling with no bundle running and nothing to settle it. Becausefinalize_bundleonly resolves a route through its waiters, the next request for that route foundBundlingwith no current bundle and hit the infallible assertion. The fix makes the deferral path report whether the request wasParkedorAnswered, and when it was answered…