Repository navigation
Conversation
… is added to the incremental graph Every file added to bundled_files now gets a stale bit at once, with fresh bits cleared. ensure_stale_bit_capacity and its fill flag are removed. A css asset or a failed import registered during the first bundle no longer marks the modules received before it as stale.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![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 the direct stale_files.set(...) callers that no longer have a capacity grow ahead of them (bundle.rs:551, ensure_route.rs:406/412, and the InsertFailureKey::Index arm plus the invalidate paths in incremental_graph.rs): add_node is now the only get_or_put on bundled_files and nothing shrinks stale_files, so every index those sites use already has a bit.
Extended reasoning...
The change collapses five file-insertion sites in the Bake dev server's IncrementalGraph into one add_node helper that grows the stale bitset with cleared bits, and deletes the ensure_stale_bit_capacity grows in finalize.rs and lifecycle.rs. It touches no security-sensitive surface. The inline findings already flag the insert_empty polarity change on existing files, so the body only records the sibling-site bounds check that was ruled out.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- 🔴
src/runtime/bake/dev_server/incremental_graph.rs:1445—Dev-server users can get an unchanged stylesheet or asset re-bundled and re-sent on a later hot update, which the base… - Also unresolved: 2 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
The StaleBit::SetIfNew change in 319eb3a restores the base contract for insert_empty on an existing file, and this pass found no new issues; since this consolidates every insertion path of a core dev-server data structure and deliberately changes when a server css node starts cached, a human look is still worthwhile.
What was reviewed:
- Traced all five
add_nodecall sites against the old stale-bit behavior:receive_chunk,insert_stale,insert_emptyandinsert_failure(AbsPath)are equivalent; onlyinsert_css_file_on_serverchanges (new node born cached), which the third hot.test.ts case pins. - Checked the
InsertFailureKey::Indexarm: both callers in failures.rs pass indices of files already inbundled_files, so the directsetis in bounds under the new invariant. - Checked the deleted grows in lifecycle.rs/finalize.rs: at startup only
insert_staleinserts and sets its own bit, and the finalize grow used a false fill, so no bit value is lost. - Grepped for
get_or_put/edge_lists.push/stale_files.resize/ensure_stale_bit_capacityoutsideadd_node: none remain.
Extended reasoning...
The PR touches the Bake dev server's IncrementalGraph (src/runtime/bake/dev_server/incremental_graph.rs) plus two call-site deletions in finalize.rs and lifecycle.rs, replacing the stale-bitset growth helper with a single add_node helper that owns every file insertion, and adds four dev-server tests. It touches no security-sensitive surface. The latest commit addressed the one blocking finding from the prior review; the two remaining inline threads are pre-existing behaviors kept by the refactor, not regressions. Defer rather than approve because the change rewrites all insertion paths of a central dev-server data structure with one intentional behavior change, which merits a maintainer's judgment despite the test coverage; no CODEOWNERS entry covers these paths.
|
#45455 fixed the same bug with a smaller change in One of its tests is not ported here: |
Fixes #48078
Problem
gen/h.tsin the issue). In a large React app this re-runs hundreds of modules and can force a full reload.receive_chunk(incremental_graph.rs:614) cleared a file's stale bit only when the bit set already covered the index. A css asset registered later in the same bundle calledensure_stale_bit_capacity(true), which grew the set with every new bit set. Every file already received at index >= 512 was now stale, and the next bundle re-parsed and re-sent it.Fix
add_nodeonIncrementalGraphnow owns every file insertion: it interns the key, pushes theEdgeListsentry, growsstale_fileswith fresh bits cleared, and sets, clears, or keeps the new file's own bit (StaleBit::{Set, Clear, Keep}). The five formerget_or_putsites call it.ensure_stale_bit_capacityand its fill flag are deleted, with the two end-of-bundle grows infinalize.rsand the two startup grows inlifecycle.rs. No code can write a bit for a file other than the one it inserts, so the bug class is gone for every current and future inserter on both graphs.truefill only ever reached files thatreceive_chunkorinsert_css_file_on_serverhad added, and those are never stale. This completes the invariant dev server: fix an index out of bounds panic when a file with an unresolved import is imported twice #44340 states: every file inbundled_fileshas a bit instale_files.test/bake/dev/css.test.ts(700 modules + css, two edits each ship onlygen/last.ts) and three cases intest/bake/dev/hot.test.ts. All four fail on a debug build of main. Also all oftest/bake/dev.Background
IncrementalGraphkeeps one record per file inbundled_files, with two parallel containers indexed by the sameFileIndex:edge_lists(import edges) andstale_files(a bit set, set = re-parse on the next bundle).is_file_cached(bundle.rs:99) reads the bit to decide whether the bundler may skip a file.receive_chunkruns once per bundled file during finalize.insert_empty,insert_failureandinsert_stale_extrarun in the same finalize for css assets, failed imports and route files, so they interleave with it.insert_css_file_on_server) is now born cached instead of inheriting whatever fill the next grow used. The third hot.test.ts case pins it: editing the route no longer re-parses the unchanged stylesheet on the server.receive_chunk). It leaves thetruefill in place, still back-fills server css nodes, and callsresizeonce per received file past the set (about 97 calls instead of 6 on a 700-file bundle). This PR supersedes dev server: clear the stale bit of a module that is bundled past the end of the stale bit set #45455, whose "merge dev server: leave out an html link to a cached json module instead of panicking, and mark a new placeholder file stale #41157 first" condition was met by Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021, and ports two of its tests. Related to Bun dev server:K.onDispose is not iterableduring HMR when index.html links a stylesheet + entry has both accept() and dispose() #29418: it removes the dev-server trigger of that report, theonDisposethrow itself is inhmr-module.ts.Downsides
get_or_put, same push, same compare of the index against the bit length, plus one call intoadd_node(not inlined in release).stale_filesresize calls on the 700-module fixture (debug counter at the call site): first bundle 4 -> 2, each later bundle 2 -> 0, startup 2 -> 0. Reallocs stay at 2..text: 140289307 -> 140287259 bytes (-2048).incremental_graphsymbols: 35384 -> 34074 bytes (nm -Son bun-profile, main 7ca0042 vs this branch). No callgrind: valgrind is not installed in the build container, so no instruction count per call.Notes
Mechanism on main, 700 modules + css, first bundle.
ensure_route.rs:506inserts the html route and grows the client set to 512 bits.receive_chunkadds 700 files, clearing bits 0..511 and skipping 512..699.assets.rs:96registers the css asset throughinsert_empty, which grows the set to 1024 with bits 512..1023 set.finalize.rs:628then grows withfalse, which touches no existing bit. On the next editis_file_cachedreturnsNonefor every file in 512..699 that the changed file reaches, sogen/h.tsis re-parsed and shipped.receive_chunkclears it then, so the second edit is clean. Without css nothing grows the set betweenreceive_chunkand the final grow. Below 512 files every index fits the initial set. A framework with no client entry and no react refresh starts with an empty client set, so there the first bundle hit this at any size (second hot.test.ts case).Siblings fixed by the same change.
insert_failureafter more than 512 received files (finalize.rs:299/397,failures.rs),insert_emptyfor css@importchildren and directory placeholders (incremental_graph.rs:1025,directory_watch_store.rs:203),insert_stale_extraon the server graph (failures.rs:392,lifecycle.rs:465), andinsert_css_file_on_server(finalize.rs:414/489), which never touched the set at all.Behaviour table per site.
receive_chunk:Clear(was: clear if covered).insert_stale_extra,insert_failure(AbsPath):Set(was: grow withtrue, then set).insert_empty:SetIfNew(was: grow withtrueand set only for a new file).insert_failure(Index):set(i)directly, always in bounds now.insert_css_file_on_server:Keep(was: no write, value came from the next grow). Theis_routewrite on an existing server route moved fromgop.value_ptrtovalues_mut()[idx].Review. One bot finding fixed after the first push:
insert_emptyset the bit of an existing file, the base did not (SetIfNewrestores that). Two pre-existing findings noted in their threads. Self-review before the push: 9 concerns raised, 9 addressed: the three should-fix items were one coverage gap around the server css node (the third hot.test.ts case), the rest were PR body wording, the STEP back-link comment, and the#45455/#29418links above.Measurements. Debug builds of main and this branch with an
eprintln!at the resize call site (not committed), 700-module css fixture, first load plus two edits. Release builds of main7ca0042d3band this branch on linux x64,sizeandnm -S. New tests under debug+ASAN, median of 3 isolated runs: css 3351 ms, hot failed-import 4128 ms, hot client component 1599 ms, hot server css 1886 ms (1 run).Suites run.
bun bd test test/bake/dev/(191 pass, 1 fail:production > works with sourcemaps - error thrown in React componenttimed out under load and passes alone; it runs a production build, not the dev server).test/bake/dev/css.test.ts,hot.test.ts,incremental-graph-edge-deletion.test.tsseparately.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/hot.test.ts