Repository navigation
dev server: fix an index out of bounds panic when a file with an unresolved import is imported twice - #44340
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe incremental graph now grows stale-bit capacity and marks newly inserted stale files. A dev-server regression test covers an unresolved import that is later fixed, followed by a reload and delivery of the imported value. ChangesStale-file tracking
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to This fixes a dev-server panic that occurred when a file with an unresolved import was imported twice. The change is small and has regression coverage, so no merge-blocking risk is evident. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/bake/dev_server/incremental_graph.rs— A file added through insert_empty between bundles can be reported as cached with no content. insert_empty at incremental_graph.rs:1395 grows with true but never sets its own bit; after DevServer.rs:4224 grew the set with false, indices in [count, aligned) are already covered and false, so the grow is a no-op and the new file is non-stale. is_file_cached (DevServer.rs:5116) then returns Some for it with kind Unknown. The diff fixed this exact shape in insert_stale_extra by adding an explicit set at :1324 but left the sibling. Fix: set the bit for a newly inserted file in insert_empty (and insert_css_file_on_server) so stale state never depends on which fill the last resize used.Why this was flagged
Trigger: after any finalize_bundle, DevServer.rs:4224 calls ensure_stale_bit_capacity(false), so bits for indices between bundled_files.count() and the 512-aligned bit_length are false. Then assets.rs:97 (a new asset registered) or mod.rs:1270 (track_resolution_failure for an importer) calls insert_empty for a path not yet in the graph. incremental_graph.rs:1393-1395 pushes an edge list and calls ensure_stale_bit_capacity(true), but want = max(count, bit_length) = bit_length, so resize is a no-op and the file's bit stays false. Nothing sets it. is_file_cached at DevServer.rs:5116 reads !is_set and returns a CacheEntry with file_kind() Unknown for a file that has no bundled content. This is the same 'relies on the fill value instead of setting the bit' pattern the diff fixed at :1324; the PR's 'fix the whole class' obligation leaves insert_empty and insert_css_file_on_server (:1406, no growth at all) untouched. Pre-existing, but the diff establishes the explicit-set pattern and should apply it to every insert.
Verification: Pre-existing (identical on the base) but same class as the PR's own fix.
insert_empty(incremental_graph.rs:1393-1396) does onlyself.ensure_stale_bit_capacity(true)?with noset(idx): after DevServer.rs:4224-4225 callensure_stale_bit_capacity(false), the resize is a no-op and the file stays non-stale. The asset path (assets.rs:97) is the live trigger, in any bundle after the first. -
🟣
src/runtime/bake/dev_server/incremental_graph.rs— Files bundled for the first time can end up marked stale after a bundle that also had a resolution failure, so the next rebuild redoes them. receive_chunk keeps the bit_length guard at incremental_graph.rs:596 that this PR removed from insert_stale_extra. The dismissal assumed the ensure(false) at DevServer.rs:4224 fixes uncovered files, but insert_failure runs first (DevServer.rs:4179 -> incremental_graph.rs:1452) and fills them with true. Fix: replace the guard with ensure_stale_bit_capacity(false)? followed by an unconditional unset, so every insert site grows the set itself. [also at: src/runtime/bake/dev_server/incremental_graph.rs:1452 - Dev-server users whose first bundle adds more than 512 files and hits any resolution failure get every file past the aligned capacity re-parsed on the next hot update.]Why this was flagged
Trigger: a bundle whose Pass 1 adds fresh files with indices >= stale_files.bit_length and whose current_bundle.resolution_failure_entries is non-empty (any unresolved import). At incremental_graph.rs:596 receive_chunk only unsets when bit_length > index, leaving fresh files uncovered. The dismissing finder relied on DevServer.rs:4224-4225 growing with false, but between Pass 1 and that line DevServer.rs:4179 calls prepare_and_log_resolution_failures, and insert_failure at incremental_graph.rs:1452 calls ensure_stale_bit_capacity(true), which covers those indices with true. After that, 4224 is a no-op for them. Result versus a correct graph: is_file_cached (DevServer.rs:5116) treats bundled files as not cached, so the next incremental bundle re-parses and re-emits them. The PR fixed the identical guard pattern for insert_stale_extra at :1323 but left this sibling; REVIEW.md requires sibling sites be fixed together. Remedy: unconditional ensure_stale_bit_capacity(false)? plus unset in receive_chunk.
Verification: Pre-existing; the base fails the same way. Trigger: receive_chunk adds files with index >= stale_files.bit_length and resolution_failure_entries is non-empty. The guard at incremental_graph.rs:596-598 leaves them uncovered; insert_failure (DevServer.rs:4179 -> incremental_graph.rs:1452) fills them with true before ensure_stale_bit_capacity(false) at 4224, so the next bundle re-parses them.
|
Updated 7:05 PM PT - Sep 30th, 2026
@dylan-conway, your commit bf6397b is building: |
What does this PR do?
Fixes
panic: index out of bounds: the len is 0 but the index is 0in the dev server (DevServer::is_file_cached, Sentry BUN-4QTH and BUN-54KE, 16 reports on 1.4.2 and later).Trigger
An HTML route whose first bundle has a file with an import that does not resolve, and a second file that imports that file:
Cause
Every file in
IncrementalGraph::bundled_filesis meant to have a bit instale_files.insert_emptyandinsert_failuregrow the bit set when they add a file.insert_stale_extradid not: it set the bit only if the set happened to be long enough already, and left growing it to its callers.get_log_for_resolution_failurescalls it in the middle of a bundle, for the file whose import failed. With only HTML routes the bit set is still empty during the first bundle, so when the bundler then resolves another import of that file,is_file_cachedfinds it inbundled_filesand reads a bit that is not there.Fix
insert_stale_extragrows the bit set and sets the bit, with the same two lines asinsert_failure.The other two inserts,
receive_chunkandinsert_css_file_on_server, only run while a bundle is finalized, after all resolution, andfinalize_bundlegrows the set right after them.How did you verify your code works?
A new test in
test/bake/dev/html.test.ts. It also fixes the broken file afterwards and expects the page to reload and run.The same files outside the test harness panic in 20 of 20 runs on canary
7fe13e1b9, with 2, 5 and 13 hops between the entry point and the second import.html.test.tsandbundle.test.ts: 35 pass on the debug build.