Repository navigation
Conversation
…reload() server.reload() builds a fresh Route for every html import in the new config and replaces the server's static route list. The js and css chunk routes that the old route's bundle appended lived only on that list, so after a reload the chunk urls in a document a client already held fell through to fetch() until some client requested the page again. The Route now records the static routes its bundle appended. On reload, a new Route for the same HTMLBundle takes those routes from the old one and re-appends them to the new config. The page itself still bundles again on the next request, as before. The chunks of the previous build stay served until the next build replaces them.
|
Updated 8:56 AM PT - Sep 6th, 2026
❌ @autofix-ci[bot], your commit 3aa6256 has 3 failures in
The baseline build contains instructions not available on Static scan violations |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughChangesHTML routes now record assets from successful bundles and reuse matching route assets during reloads. Reload tests cover preserved JavaScript and CSS chunks before and after rebuilds in development and HMR-disabled modes. HTML route reload
Suggested reviewers: Merge Risk: ⚪ Minimal · up to HTML bundle chunk URLs remain available across server reloads while the page rebuilds from disk on its next request. Current coverage includes the affected route and configuration variants, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Shortened the three doc comments the comment check flagged (77f9735). No code change. |
|
CI status for 77f9735 (build 111076): the new tests pass on every lane. The red lanes are not from this diff.
The other failures passed on retry. Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nits, I also checked whether recording the built page at its bundler output path in assets (HTMLBundle.rs:673) means adopt_assets would shadow the html route and skip re-bundling when the user's mount path equals that output path — the html route is registered after the appended static routes on reload and wins the match, so re-bundling still happens. Also traced the RefPtr<StaticRoute> clones through adopt_assets → append_static_route: each clone is moved into an AnyRoute::Static and the vec is dropped, so the counts balance.
Extended reasoning...
Two minor findings are posted inline (stale doc-comment method name; memory_cost() not accounting for the new assets field). Beyond those, the review looked at whether carrying the page-at-output-path entry across reload could suppress the intended re-bundle and at the refcount balance of the cloned RefPtr<StaticRoute> values flowing through adopt_assets into new_config.append_static_route; both were ruled out. One further verified finding was pruned from posting, so approval is withheld.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/server/HTMLBundle.rs— nit:Route::memory_cost()was not updated for the newassetsfield, so the heap bytes it now owns (theVeccapacity plus eachBox<[u8]>path) are not reported to GC pressure accounting after a bundle completes or a reload adopts chunks. Fix: add theassetsvec's element size × len and the summed path lengths tomemory_cost(), matching howpending_responsesis accounted just above (do not add theStaticRoutebodies — those are already counted via the config'sstatic_routeslist).Extended reasoning...
Route::memory_cost()at HTMLBundle.rs:191-197 sumssize_of::<Route>(),pending_responses.len() * size_of::<PendingResponse>(), andstate.memory_cost(). The struct gainedassets: JsCell<Vec<(Box<[u8]>, RefPtr<StaticRoute>)>>(line 165), populated byappend_asseton every bundle output and byadopt_assetson reload. REVIEW.md requires every consumer of a struct's fields be updated atomically with a new field;memory_cost()is such a consumer (StaticRouteEntry::memory_cost at ServerConfig.rs:209 shows the pattern of addingpath.len()). On the base branch there was noassetsfield so nothing was under-reported; after this change a built route owning several chunk paths under-reports its retained memory. Minor correctness gap, not a leak — refs are still balanced viaDrop.Verification: nit —
Route::memory_cost()at src/runtime/server/HTMLBundle.rs:191-197 was not updated for the newassets: JsCell<Vec<(Box<[u8]>, RefPtr<StaticRoute>)>>field added at line 165. The function sums onlysize_of::<Route>()(which covers the Vec header inline in the struct, not its heap buffer),pending_responses.get().len() * size_of::<PendingResponse>(), andstate.get().memory_cost().…
…d do not alias it over an html route Only the chunk routes are assets. The page at its bundler output path (/index.html) is registered as before but not recorded, so a reload drops it and the new html route bundles again. When an html route is mounted at that same path, the alias replaced the html route in the static route list after the first build, so the page never bundled again and a reload could not find the route to take the chunks from. Skip the alias in that case.
|
Addressed the reviews:
|
|
CI for 3aa6256 (build 111276): the PR's tests pass on every lane. The red lanes are not from this diff.
The other failures passed on retry. Ready for review. |
There was a problem hiding this comment.
Re-reviewed after d84c4fa/756bbe0e — the html page is no longer pushed into assets, and the output-path alias now skips paths that already carry an Html route, so the /index.html carry-over I flagged no longer applies; the new route: "/index.html" matrix rows exercise exactly that. No further issues found. Given this touches RefPtr<StaticRoute> lifetime across server.reload(), a human pass on the ownership shape is still worth having.
What was reviewed
on_complete: html entry now bypassesappend_assetand is guarded byhtml_path_has_html_route; chunk outputs alone populateassets.adopt_assets: pointer-identity + empty-destination guard means the same dedupedRouteseen at multiple paths adopts once and returnsVec::new()thereafter.- Test matrix now covers
{development: false | {hmr:false}} × {"/", "/index.html"}and assertssameChunk: false+v2: trueafter an on-disk edit + reload.
Extended reasoning...
Overview
Incremental commits since the last review (d84c4fa, 756bbe0, 3aa6256) narrow Route.assets to chunk outputs only: the built html page is registered via server.append_static_route directly instead of append_asset, and is additionally skipped when an AnyRoute::Html entry already sits at the bundler's html output path. The test matrix in test/js/bun/http/bun-serve-html.test.ts was widened from two development values to four {development, route} combinations, adding route: "/index.html" — the case where the user's mount path equals the bundler's un-hashed entry-point output path.
Security risks
None. The change is server-internal route bookkeeping across server.reload(); no new input parsing, no auth/crypto/permission surface, no untrusted-size arithmetic.
Level of scrutiny
Medium-high. The mechanics are contained (one new JsCell<Vec<...>> field, two small methods, a ~20-line reload-path loop), but they own RefPtr<StaticRoute> values whose lifetime now extends across a config swap, and the O(new_html × old_html) loop plus the .any() scan over static_routes sit on the reload/bundle-complete paths. REVIEW.md calls out refcount balance on every terminal path and state stored on the object whose lifetime matches it — both are in play here and merit a human eye even though nothing looked wrong on inspection (RefPtr is cloned into containers, assets.set(Vec::new()) at build start drops the previous generation, memory_cost accounts for the field).
Other factors
The prior inline finding about the html page being carried over is addressed in code and now covered by an automated test that would fail on the earlier draft (the /index.html rebuild case asserts sameChunk: false and v2: true). The adopt_assets guard (!self.assets.get().is_empty()) makes the nested reload loop idempotent when the same deduped Route appears under multiple paths, matching the PR notes. No CODEOWNERS gate was found for these paths. Deferring rather than approving because the change is not mechanical and sits in refcounted server-lifetime code.
Problem
server.reload(), the hashed/chunk-*.jsand/chunk-*.cssurls of an html import route answer 404 (they fall through tofetch) until some client requests the html page again. A client that holds the document, or that races a reload between the document and its subresources, loses the app's script and style. Seen withdevelopment: falseand{ hmr: false }on 1.3.14 through 1.4.3 and main.on_reload_from_zig(src/runtime/server/server_body.rs:2178) replacesconfig.static_routeswith the new config's list, andhtml_route_from_jsgives the new config a freshRouteinState::Pending. The chunk routes that the old route's bundle appended inon_complete(src/runtime/server/HTMLBundle.rs) lived only on the replaced list.Fix
html_bundle::Routerecords the static routes its bundle appends (assets).on_completegoes throughappend_assetfor every output and resets the list at the start of each successful build.HTMLBundletakes the old route's chunk routes (adopt_assets) and the reload appends them to the new config before it replaces the list.test/js/bun/http/bun-serve-html.test.ts, two new cases fordevelopment: falseand{ hmr: false }, each at/and at/index.html(eight tests, all fail on 1.4.3). Also the otherbun-serve-html-*files andbun-serve-routes.test.ts.Background
html_bundle::Route. The first request to it runs the bundler.on_completeturns each output into aStaticRoute, appends it toconfig.static_routes, and re-registers every route on the uws app.config.static_routesis input state.server.reload()replaces it with the list parsed from the new config. Function routes and html routes come from that new config. The chunk routes have no source in it.development: true) keys its route bundles on the html file and is not affected.Notes
server.reload().HTMLBundlepointer identity. A reload with a differentHTMLBundleobject for the same file (for example afterbun --hotre-evaluates the import) does not adopt. The next page request bundles and registers the chunks as before.adopt_assetsadopts once per new route. The same route can appear under several paths in one config (thededupe_html_bundle_mapinhtml_route_from_js), so the loop inon_reload_from_zigsees it more than once./index.html). It is not an asset: a reload drops it so the page bundles again. When an html route is mounted at that same path, the alias replaced the html route in the list after the first build (normalize_static_routes_listkeeps the later entry), so the page never bundled again and a reload had no old route to take the chunks from. The alias is now skipped in that case.HTMLBundlenever bundled again. That is a semantic change the docs do not describe (docs/runtime/http/routing.mdxsaysserver.reload()is how static routes are refreshed). The draft was narrowed to carry only the chunk routes.reload({ fetch })without arouteskey drops static and html routes but keeps function routes. A failed bundle answers 500 once, then 200 with an empty body (theState::Errarm ofon_any_request).[human-review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
HTML routes only registered their bundled JS and CSS chunk paths as static routes from the bundle-completion path triggered by a request to the HTML route, and
server.reload()rebuilt the static route list from the new config, so the previously appended chunk routes were dropped until the next request to the HTML path re-ran the cached completion. The fix makes each HTML route record its built chunk assets on the route object itself, and on reload any matching HTML route in the new config adopts those assets and re-appends them to the static route list so the chunk URLs keep serving witho…