Skip to content

bake: retract a file's bundling failure when the file is deleted - #37888

Open
robobun wants to merge 1 commit into
mainfrom
farm/72f824a9/bake-retract-failure-on-delete
Open

bake: retract a file's bundling failure when the file is deleted#37888
robobun wants to merge 1 commit into
mainfrom
farm/72f824a9/bake-retract-failure-on-delete

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Dev server: break a file so it fails to bundle (other.ts:1:22: error: Unexpected ;), then delete it. The importer's Could not resolve: "./other" error is added, but the deleted file's error is never retracted: the overlay and every fresh "Build Failed" page list both, and the error page never reloads once the importer drops the import. Reproduces on the released build.
  • Cause: deleting a node disconnects it but leaves its failure recorded and publishes no removal. A failure is only cleared when the file is bundled again, and a deleted file never is, so the entry lives for the rest of the process.
  • Predates the Rust port; the Zig code did the same.

Fix

  • Deleting a node now does what a rebundle does for a node that stops failing: clear its failed flag, drop its entry from the failure map, and queue that entry as a removal.
  • Property to check: the flag and the map entry move together. Other paths look the entry up whenever the flag is set, so clearing one alone leaks the entry or panics when the file is recreated.
  • The removal is queued mid-bundle, as a replaced failure already is, so it goes out in the same errors packet as the importers' new failures. The node is kept, as before, so a recreated file reuses it.
  • Deletion counterpart of bake: retract a client component's bundling failure when it is demoted #37862 (same omission for demoted client components); the changes do not overlap.
  • Verification: two new dev-server tests, client graph and server graph, fail on the released build at the delete step with the deleted file's error still listed, and pass with the change on a debug/ASAN build.

Background

  • The dev server (bake) keeps an incremental graph per side, client and server: one node per bundled file plus importer edges, so an edit re-bundles only that file and its importers.
  • A node carries a failed flag; the failure text lives in one process-wide map keyed by (side, node index), which overlays and fresh "Build Failed" pages are rendered from.
  • After each bundle the dev server sends one errors packet of failures added and removed. The overlay and error page update from those packets; the error page reloads only when no failure is left.
Original description

Problem

The remaining Batch::from(..) sites that hand an intrusive task to a thread pool projected the *mut Task out of a reference to the object instead of out of the object's pointer:

site shape
PatchTask::schedule(&mut self, ..) (src/install/patch_install.rs) Batch::from(&raw mut self.task)
AsyncHTTP::schedule(&mut self, ..) and preconnect (src/http/AsyncHTTP.rs) Batch::from(addr_of_mut!(self.task)), addr_of_mut!(async_http.task) with async_http: &mut AsyncHTTP
isolated-install Installer::start_task (src/install/isolated_install/Installer.rs) Batch::from(&raw mut task.task) with task = &mut self.tasks[i]
ThreadPool::each / each_ptr (src/threading/ThreadPool.rs) Batch::from(addr_of_mut!(runner_task.task)) from tasks.iter_mut()
compute_data_for_source_map (src/bundler/LinkerContext.rs) Batch::from(&raw mut line_offset.thread_task) from iter_mut(), twice
generate_chunks_in_parallel (src/bundler/linker_context/generateChunksInParallel.rs) push(..); Batch::from(&raw mut tasks.last_mut().unwrap().task) per element, four loops

In every case the pool calls back with exactly that pointer and the callback recovers the object from it with container-of and then uses the rest of the object: PatchTask::from_task_ptr (run_from_thread_pool), from_field_ptr!(Task, ..) (isolated install, which also writes result and links next), from_field_ptr!(RunnerTask, ..) (each, reads i and ctx), from_field_ptr!(SourceMapDataTask, ..), from_field_ptr!(PrepareCssAstTask, ..) and pending_part_range_prologue. For AsyncHTTP it happens before the task is even queued: HTTPThread::schedule container-ofs every task in the batch on the calling thread, links the object into queued_tasks through the result, and start_queued_task later ptr::reads the whole struct through it. bun_core::container_of documents that this requires a pointer derived from the object's pointer ("a &mut field reborrow does not suffice"), and WorkPool::schedule_owned projects out of the Box::into_raw pointer for that reason. The other Batch::from sites in the tree already do this, either inline (&raw mut (*parse_task).task, addr_of_mut!((*task).task)) or by passing a pointer a helper projected that way (&raw mut (*task).threadpool_task in the package manager's enqueue helpers); WorkPool::schedule forwards its argument, and the one site that narrows through an accessor is #37865's.

The reference-based spelling does not meet that contract. rustc retags the result of &raw mut <place> unless the place is based on a raw pointer, and under Stacked Borrows that retag covers the projected field only, so the callback's first sibling-field access is out of range. The push + last_mut() loops in generate_chunks_in_parallel have a second problem: every last_mut() goes through Vec::deref_mut, which reborrows the whole buffer and invalidates the pointers already in the batch, so Batch::push itself trips when it links the next task onto the previous one (projecting through ptr::from_mut(last_mut()) is not enough there; the batch has to be built after the Vec is filled). Reduction below. Tree Borrows, which is what bun run rust:miri checks, accepts all of these shapes, none of the crates involved are in its crate set, and no crash is known or expected from today's codegen: the addresses are right, and this only matters to the aliasing model. It is the same contract violation as #37768 (the WorkPool::schedule population, which deliberately leaves Batch::from out of its lint) and #37865 (NewTaskQueue::push, the one Batch::from site that narrows through an accessor; not touched here), in the one spelling neither of them covers.

Reduction under Miri (miri 0.1.0 9f36de775b), one mode per shape

ref is the &raw mut task.task / &raw mut self.task shape, iter_mut the addr_of_mut!(runner_task.task) shape, last_mut the chunk loops; last_mut_from_mut is the chunk loop with only the projection fixed; from_mut, as_mut_ptr and raw are the three shapes this PR converts to. The pool side is container_of followed by a sibling-field read and write, run on another thread. The trailing comments on the --> lines name the statement at each location.

=== Stacked Borrows: ref
error: Undefined Behavior: attempting a read access using <2295> at alloc929[0x10], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:72:25                                   let i = (*job).i;          // the callback
help: <2295> was created by a SharedReadWrite retag at offsets [0x8..0x10]
  --> src/main.rs:87:16                                   batch.push(&raw mut task.task);
=== Stacked Borrows: iter_mut
error: Undefined Behavior: attempting a read access using <2079> at alloc842[0x10], but that tag does not exist in the borrow stack for this location
help: <2079> was created by a SharedReadWrite retag at offsets [0x8..0x10]
  --> src/main.rs:123:28                                  batch.push(std::ptr::addr_of_mut!(j.task));
=== Stacked Borrows: last_mut
error: Undefined Behavior: attempting a write access using <1941> at alloc836[0x8], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:48:22                                   (*self.tail).node.next = task   // Batch::push, calling thread
help: <1941> was created by a SharedReadWrite retag at offsets [0x8..0x10]
help: <1941> was later invalidated at offsets [0x0..0x30] by a Unique retag
  --> src/main.rs:133:41                                  jobs.last_mut()                 // next iteration
=== Stacked Borrows: last_mut_from_mut
error: Undefined Behavior: attempting a write access using <1958> at alloc840[0x8], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:48:22                                   (*self.tail).node.next = task
help: <1958> was created by a SharedReadWrite retag at offsets [0x0..0x18]
help: <1958> was later invalidated at offsets [0x0..0x30] by a Unique retag
  --> src/main.rs:135:48                                  jobs.last_mut()
=== Stacked Borrows: from_mut
from_mut: ok [1, 2, 3]
=== Stacked Borrows: as_mut_ptr
as_mut_ptr: ok [1, 2, 3]
=== Stacked Borrows: raw
raw: ok [1, 2, 3]

Tree Borrows (-Zmiri-tree-borrows): all seven modes ok

Fix

Same pointer values as before at every site, so no behaviour change; what changes is which pointer they are projected from.

  • PatchTask::schedule becomes unsafe fn schedule(this: *mut Self, batch) and projects out of this. Its only caller, flush_patch_task_queue, already holds the *mut PatchTask it popped from the fifo and now reads the callback kind through it instead of forming a &mut PatchTask.
  • AsyncHTTP::schedule keeps &mut self (its callers, send_sync, NetworkTask, FetchTasklet, S3, hold the object by reference or value, and HTTPThread::schedule recovers an AsyncHTTP, which a &mut AsyncHTTP does cover) and projects through ptr::from_mut(self), a reborrow of the whole object. preconnect goes through it instead of projecting by hand, so the http crate has one projection site.
  • Installer::start_task and compute_data_for_source_map project through ptr::from_mut of the slot they just wrote.
  • ThreadPool::each_impl and the two batches in generate_chunks_in_parallel fill their Vec first and then build the batch in one loop from as_mut_ptr().add(i); the three copies of the last_mut() push in the chunk loop collapse into that one loop, and the with-capacity comments that justified taking pointers mid-fill go away with the pattern.

Verification

test/internal/source-lints/thread-pool-batch-projection.test.ts bans &raw mut / &raw const / addr_of_mut! / addr_of! of a field path rooted at a binding as the argument of any Batch::from( call (however the type is spelled, including the PoolBatch / ThreadPoolBatch aliases), with a self-test over the spellings of every site in the tree and the converted shapes. Against main it reports exactly the eleven sites above:

src/bundler/linker_context/generateChunksInParallel.rs:117, :203, :226, :248
src/bundler/LinkerContext.rs:589, :590
src/http/AsyncHTTP.rs:391, :530
src/install/isolated_install/Installer.rs:171
src/install/patch_install.rs:711
src/threading/ThreadPool.rs:560

and passes on this branch with no allowlist. It is deliberately disjoint from #37865's lint, which bans the reference / accessor / from_mut shapes in the same argument and reports only src/install/PackageInstall.rs:475 both on main and on this branch (checked by running it here), so the two can land in either order without sharing an allowlist; #37768's lint covers WorkPool::schedule, and both of those also report nothing on this branch.

The converted paths are exercised by existing tests, all run against the debug build of this branch: test/cli/install/bun-install-patch.test.ts (18 pass) and bun-patch.test.ts (31 pass) for PatchTask::schedule and the registry downloads that go through NetworkTaskAsyncHTTP::schedule; test/cli/install/isolated-install.test.ts (62 pass) for start_task; test/cli/install/bun-audit.test.ts (17 pass) for the send_sync path; test/js/web/fetch/fetch-redirect.test.ts (30 pass) and client-fetch.test.ts (34 pass) for the fetch path; test/bundler/bundler_html.test.ts (22 pass; JS, CSS and HTML part ranges plus the CSS AST batch), test/bundler/css/css-modules.test.ts (6 pass), test/bundler/bundler_splitting.test.ts (11 pass; many chunks through each_ptr), test/bundler/bun-build-api.test.ts (52 pass; source maps) and test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts (22 pass). fetch.preconnect was checked directly with a local listener (the preconnect opens the connection before the first fetch); test/js/web/fetch/fetch-preconnect.test.ts itself cannot run in this container because localhost resolves to ::1 first here, which makes it fail identically on the released binary, and that is tracked separately. cargo clippy --no-deps on bun_threading, bun_http, bun_install and bun_bundler is clean and cargo fmt is clean.

IncrementalGraph::on_file_deleted disconnected the node's imports and
re-enqueued its importers, but left the node's `failed` flag set and its
SerializedFailure in dev.bundling_failures, and never pushed it onto
incremental_result.failures_removed. Only receive_chunk clears a node's
failure, and a deleted file is never received again, so the failure
stayed registered for the rest of the process unless the file came back:
error overlays kept showing it next to the importers' new resolution
errors, every later "Build Failed" page listed it, and an error page
never reloaded once the importers were fixed because one entry remained.

Clear the flag and move the entry onto failures_removed in
on_file_deleted; index_failures publishes it at the end of the same
bundle, alongside the failures the re-bundled importers report.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 30f3d824-8ac0-49bf-bd2f-00e3d0e5ea29

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 1a64af5.

📒 Files selected for processing (2)
  • src/runtime/bake/dev_server/incremental_graph.rs
  • test/bake/dev/bundle.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:05 PM PT - Aug 12th, 2026

@robobun, your commit 1a64af5 has 4 failures in Build #93393 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37888

That installs a local version of the PR into your bun-37888 executable, so you can run:

bun-37888 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, well-tested fix that mirrors the existing failure-retraction pattern from receive_chunk.

What was reviewed:

  • The new block in on_file_deleted is byte-for-byte the same bookkeeping receive_chunk does on both sides (clear failed, fetch_swap_remove from bundling_failures, push to failures_removed), so the failed ⇔ map-entry invariant is preserved and the panic guard matches.
  • Confirmed on_file_deleted runs mid-bundle via handle_parse_task_failure, and index_failures drains failures_removed at the end of that bundle — the removal ships in the same errors packet as the importer's new failure.
  • Checked that the sibling-projection helpers (dev_bundling_failures, dev_incremental_result) are the same ones already used from receive_chunk/insert_failure; no new unsafe.
  • Tests cover both client and server graphs, the incremental errors packet, a fresh page load, error-page reload once the last failure clears, and node reuse on recreation; win32 skip matches the neighboring deletion test.
Extended reasoning...

Overview

The PR adds 13 lines to IncrementalGraph::on_file_deleted in src/runtime/bake/dev_server/incremental_graph.rs and two dev-server tests in test/bake/dev/bundle.test.ts. When a file that previously failed to bundle is deleted, its stale SerializedFailure is now retracted from dev.bundling_failures and pushed onto incremental_result.failures_removed, so the error overlay and the "Build Failed" page stop listing it and the error page can reload once the remaining failures clear.

Security risks

None. This is dev-server error-overlay bookkeeping; no user input parsing, no auth, no filesystem or network surface changes.

Level of scrutiny

Low-to-medium. The dev server's incremental graph is a stateful subsystem, but the change is a verbatim copy of the failure-clearing block that already runs in receive_chunk (both the Side::Client and Side::Server branches do exactly this: clear failed, fetch_swap_remove the OwnerPacked(SIDE, index) entry, push it onto failures_removed, panic if the invariant is broken). The failed flag and the bundling_failures entry always move together everywhere else in the file (insert_failure, receive_chunk, trace_imports), and this change keeps that invariant. The call site (handle_parse_task_failure → ENOENT branch) is inside a bundle, and index_failures drains failures_removed at the end of that bundle, so the removal is published exactly as the PR describes.

Other factors

  • The two new tests are placed next to the existing deleting imported file shows error then recovers test, share its win32 skip and its fixture shape, and exercise both the client graph (HTML route) and the server graph (minimalFramework). They assert the overlay contents after the delete, a fresh page load's failure list, that the error page reloads once the last failure is fixed, and that recreating the file (which reuses the node) recovers cleanly — the last check guards against clearing only the flag or only the map entry.
  • The PR description states both tests fail against the released build at the delete step with the stale error still listed, and pass on a debug/ASAN build with the fix; the rest of bundle.test.ts (23 tests) still passes.
  • The related sibling path (disconnect_and_delete_file for demoted client components, #37862) is explicitly called out as independent; the diffs do not overlap.
  • No new unsafe, no allocation or lifetime changes, no cross-graph access — the sibling-field projections used are the existing dev_bundling_failures() / dev_incremental_result() helpers already exercised from the same file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant