dev server: re-bundle a failed route once per file change and close the descriptors the watcher declines

src/bundler/ParseTask.rs

@@ -68,7 +68,14 @@ mod EventLoop {
 #[derive(bun_core::EnumTag)]
 #[enum_tag(existing = ContentsOrFdTag)]
 pub enum ContentsOrFd {
-    Fd { dir: Fd, file: Fd },
+    Fd {
+        dir: Fd,
+        file: Fd,
+        /// `file` was opened by this task's read rather than borrowed from the
+        /// resolver's entry cache. It is kept open for the watcher; if the
+        /// watcher does not adopt it, the bundle thread closes it.
+        owns_file: bool,
+    },
     // The `'static` is ownership-erased: contents may be arena-owned,
     // plugin-owned, or truly static (runtime source). The producer keeps the
     // backing allocation alive for the duration of the bundle pass.
@@ -161,13 +168,17 @@ impl ResultValue {
 pub(crate) struct WatcherData {
     pub(crate) fd: Fd,
     pub(crate) dir_fd: Fd,
+    /// See [`ContentsOrFd::Fd::owns_file`]. When set and the watcher does not
+    /// adopt `fd`, the bundle thread closes it.
+    pub(crate) owns_fd: bool,
 }
 
 impl WatcherData {
     /// When no files to watch, this encoding is used.
     pub(crate) const NONE: WatcherData = WatcherData {
         fd: Fd::INVALID,
         dir_fd: Fd::INVALID,
+        owns_fd: false,
     };
 }
 
@@ -263,6 +274,7 @@ impl ParseTask {
             contents_or_fd: ContentsOrFd::Fd {
                 dir: resolve_result.dirname_fd,
                 file: resolve_result.file_fd,
+                owns_file: false,
             },
             side_effects: resolve_result.primary_side_effects_data,
             // D042: resolver-side and bundler-side `jsx::Pragma` are the SAME
@@ -1393,7 +1405,7 @@ pub mod parse_worker {
         _loader: Loader,
     ) -> core::result::Result<CacheEntry, AnyError> {
         match &task.contents_or_fd {
-            ContentsOrFd::Fd { dir, file } => 'brk: {
+            ContentsOrFd::Fd { dir, file, .. } => 'brk: {
                 let contents_dir = *dir;
                 let contents_file = *file;
                 let _trace = perf::trace("Bundler.readFile");
@@ -2326,20 +2338,21 @@ pub mod parse_worker {
         // there); closing it leaves a stale fd for the next in-process build.
         let opened_own_fd =
             matches!(task.contents_or_fd, ContentsOrFd::Fd { file, .. } if !file.is_valid());
-        let will_close_file_descriptor = opened_own_fd
-            && entry.fd.is_valid()
-            && entry.fd.stdio_tag().is_none()
-            && worker_ctx.bun_watcher.is_none();
-        if will_close_file_descriptor {
+        let owns_fd = opened_own_fd && entry.fd.is_valid() && entry.fd.stdio_tag().is_none();
+        if owns_fd && worker_ctx.bun_watcher.is_none() {
             let _ = entry.close_fd();
             task.contents_or_fd = ContentsOrFd::Fd {
                 file: Fd::INVALID,
                 dir: Fd::INVALID,
+                owns_file: false,
             };
         } else if matches!(task.contents_or_fd, ContentsOrFd::Fd { .. }) {
+            // With a watcher, the descriptor stays open and `on_parse_task_complete`
+            // either hands it to the watcher or closes it.
             task.contents_or_fd = ContentsOrFd::Fd {
                 file: entry.fd,
                 dir: Fd::INVALID,
+                owns_file: owns_fd,
             };
         }
         *step = Step::Parse;
@@ -2825,9 +2838,14 @@ pub mod parse_worker {
             // doesn't derive `Copy`, so move it out (task is consumed here).
             external: core::mem::take(&mut this.external_free_function),
             watcher_data: match this.contents_or_fd {
-                ContentsOrFd::Fd { file, dir } => WatcherData {
+                ContentsOrFd::Fd {
+                    file,
+                    dir,
+                    owns_file,
+                } => WatcherData {
                     fd: file,
                     dir_fd: dir,
+                    owns_fd: owns_file,
                 },
                 ContentsOrFd::Contents(_) => WatcherData::NONE,
             },

src/bundler/bundle_v2.rs

@@ -4750,6 +4750,7 @@ pub mod bv2_impl {
                                 contents_or_fd: parse_task::ContentsOrFd::Fd {
                                     dir: bun_sys::Fd::INVALID,
                                     file: bun_sys::Fd::INVALID,
+                                    owns_file: false,
                                 },
                                 side_effects: bun_ast::SideEffects::HasSideEffects,
                                 jsx: this
@@ -6905,28 +6906,34 @@ pub mod bv2_impl {
             }
 
             // To minimize contention, watchers are appended on the bundle thread.
-            if this.bun_watcher.is_some() {
-                if parse_result.watcher_data.fd != bun_sys::Fd::INVALID {
-                    let source_index = parse_result.value.source_index();
-                    // borrowck — read the source path before
-                    // `should_add_watcher(&self)` so the column borrow is released.
-                    let source_path = this.graph.input_files.items_source()[source_index as usize]
-                        .path
-                        .text;
-                    if this.should_add_watcher(source_path) {
+            let watcher_data = &parse_result.watcher_data;
+            if this.bun_watcher.is_some() && watcher_data.fd.is_valid() {
+                let source_index = parse_result.value.source_index();
+                // borrowck — read the source path before
+                // `should_add_watcher(&self)` so the column borrow is released.
+                let source_path = this.graph.input_files.items_source()[source_index as usize]
+                    .path
+                    .text;
+                let adopted = this.should_add_watcher(source_path)
+                    && matches!(
                         // const generic `CLONE_FILE_PATH = isWindows`
                         // matches `cfg!(windows)` at compile time.
-                        let _ = this
-                            .bun_watcher_mut()
+                        this.bun_watcher_mut()
                             .unwrap()
                             .add_file::<{ cfg!(windows) }>(
-                                parse_result.watcher_data.fd,
+                                watcher_data.fd,
                                 source_path,
                                 bun_wyhash::hash(source_path) as u32,
-                                parse_result.watcher_data.dir_fd,
+                                watcher_data.dir_fd,
                                 None,
-                            );
-                    }
+                            ),
+                        Ok(bun_watcher::FdOwnership::Watcher)
+                    );
+                // The file is already watched (every re-parse under the dev
+                // server) or excluded from watching: nothing else will close
+                // the descriptor this parse opened.
+                if !adopted && watcher_data.owns_fd {
+                    let _ = bun_sys::close(watcher_data.fd);
                 }
             }
 

src/runtime/bake/DevServer.rs

@@ -423,6 +423,16 @@ pub struct DevServer {
     pub(crate) emit_memory_visualizer_events: u32,
     pub(crate) memory_visualizer_timer: EventLoopTimer,
 
+    /// Perfect incremental bundling would mean the code that bundles, watches
+    /// and rebuilds routes has no bugs, so a route's stored failures are always
+    /// accurate. Since it is not perfect, when this is false a request for a
+    /// route with failures re-bundles everything the route imports before
+    /// showing them (`check_route_failures`). This happens once per route per
+    /// file change (`RouteBundle::rebundled_for_failures`); otherwise a page
+    /// sitting on an error page would rebuild the route on every request.
+    ///
+    /// Disabled in release builds, enabled in debug builds; override with
+    /// `BUN_ASSUME_PERFECT_INCREMENTAL`.
     pub(crate) assume_perfect_incremental_bundling: bool,
 
     /// If true, console logs from the browser will be echoed to the server console.
@@ -2259,6 +2269,9 @@ fn check_route_failures(
     resp: DevResponse,
 ) -> crate::Result<CheckResult> {
     let mut gts = dev.init_graph_trace_state(0)?;
+    // Still holds whatever the last bundle added; the trace below must only
+    // collect the failures this route can reach.
+    dev.incremental_result.failures_added.clear();
     // Note: erase to a raw pointer so the deferred cleanup only fires on
     // scope exit when no other borrow of `dev` is live.
     let dev_ptr = std::ptr::from_mut::<DevServer>(dev);
@@ -2278,8 +2291,11 @@ fn check_route_failures(
         TraceImportGoal::FindErrors,
     )?;
     if !dev.incremental_result.failures_added.is_empty() {
-        // See comment on this field for information
-        if !dev.assume_perfect_incremental_bundling {
+        let assume_perfect_incremental_bundling = dev.assume_perfect_incremental_bundling;
+        let rb = dev.route_bundle_ptr(route_bundle_index);
+        // See `assume_perfect_incremental_bundling`.
+        if !assume_perfect_incremental_bundling && !rb.rebundled_for_failures {
+            rb.rebundled_for_failures = true;
             // Cache bust EVERYTHING reachable
             {
                 let mut it = gts.client_bits.iterator::<true, true>();
@@ -3147,6 +3163,12 @@ impl DevServer {
         debug_assert!(!entry_points.set.is_empty());
         self.log.clear_and_free();
 
+        if had_reload_event {
+            for route_bundle in &mut self.route_bundles {
+                route_bundle.rebundled_for_failures = false;
+            }
+        }
+
         // Notify inspector about bundle start
         if let Some(agent) = self.inspector() {
             let mut trigger_files: Vec<BunString> = Vec::with_capacity(entry_points.set.len());
@@ -5253,6 +5275,7 @@ impl DevServer {
             server_state: route_bundle::State::Unqueued,
             client_bundle: None,
             active_viewers: 0,
+            rebundled_for_failures: false,
         });
         // SAFETY: index_location still valid (route_bundles is a separate field)
         unsafe { *index_location = Some(bundle_index) };

src/runtime/bake/dev_server/route_bundle.rs

@@ -157,6 +157,12 @@ pub struct RouteBundle {
     pub(crate) client_bundle: Option<StaticRouteRef>,
     pub(crate) client_script_generation: u32,
     pub(crate) active_viewers: u32,
+    /// A request already re-bundled everything this route imports because
+    /// the route had bundling failures (see
+    /// `DevServer::assume_perfect_incremental_bundling`), and no file has
+    /// changed since. Further requests serve the stored failures instead of
+    /// bundling again; cleared whenever a hot-reload bundle starts.
+    pub(crate) rebundled_for_failures: bool,
 }
 
 impl RouteBundle {

test/bake/bake-harness.ts

@@ -279,6 +279,49 @@ export class Dev extends EventEmitter {
     });
   }
 
+  /**
+   * Resolves once the dev server finishes its next bundle, whether it
+   * succeeded or failed. Call this before triggering the bundle.
+   */
+  waitForBundle(): Promise<void> {
+    return new Promise<void>(resolve => {
+      const handle = (kind: WatchSynchronization) => {
+        if (
+          kind === WatchSynchronization.AnyBuildFinished ||
+          kind === WatchSynchronization.AnyBuildFinishedWaitForWebSockets
+        ) {
+          this.off("watch_synchronization", handle);
+          resolve();
+        }
+      };
+      this.on("watch_synchronization", handle);
+    });
+  }
+
+  /**
+   * Returns how many bundles (successful or failed) the dev server finished
+   * while `fn` ran. `dev.write` and friends wait for the bundle they trigger,
+   * so those are counted too.
+   */
+  async countBundles(fn: () => Promise<void>): Promise<number> {
+    let count = 0;
+    const handle = (kind: WatchSynchronization) => {
+      if (
+        kind === WatchSynchronization.AnyBuildFinished ||
+        kind === WatchSynchronization.AnyBuildFinishedWaitForWebSockets
+      ) {
+        count++;
+      }
+    };
+    this.on("watch_synchronization", handle);
+    try {
+      await fn();
+    } finally {
+      this.off("watch_synchronization", handle);
+    }
+    return count;
+  }
+
   async batchChanges(options: { errors?: null | ErrorSpec[]; snapshot?: string } = {}) {
     if (this.batchingChanges) {
       this.batchingChanges.write?.();

test/bake/dev/bundle.test.ts

@@ -1,5 +1,6 @@
 // Bundle tests are tests concerning bundling bugs that only occur in DevServer.
 import { expect } from "bun:test";
+import { readdirSync, readlinkSync } from "node:fs";
 import { devTest, emptyHtmlFile, minimalFramework } from "../bake-harness";
 
 devTest("import identifier doesnt get renamed", {
@@ -274,6 +275,96 @@ devTest("deleting imported file shows error then recovers", {
     });
   },
 });
+devTest("requests for a route with build errors do not re-bundle it every time", {
+  files: {
+    "index.html": emptyHtmlFile({
+      scripts: ["index.ts"],
+    }),
+    "index.ts": `
+      import { value } from "./other";
+      console.log(value);
+    `,
+    "other.ts": `
+      export const value = 1;
+    `,
+  },
+  async test(dev) {
+    expect((await dev.fetch("/")).status).toBe(200);
+
+    const expectErrorPage = async () => {
+      const response = await dev.fetch("/");
+      expect(await response.text()).toContain("Build Failed");
+      expect(response.status).toBe(500);
+    };
+
+    for (const broken of ["export const value = <<<SYNTAX_ERROR>>>;", "export const value = <<<ANOTHER_ONE>>>;"]) {
+      await dev.write("other.ts", broken);
+
+      // The first request after a file change re-bundles the route once, in
+      // case the incremental build got the failures wrong.
+      const rebundled = dev.waitForBundle();
+      await expectErrorPage();
+      await rebundled;
+
+      // Until another file changes, requests serve the failures that bundle
+      // produced. The fixed server never starts a bundle here, so any count
+      // observed by the time the responses arrive is a regression.
+      expect(
+        await dev.countBundles(async () => {
+          for (let i = 0; i < 5; i++) {
+            await expectErrorPage();
+          }
+        }),
+      ).toBe(0);
+    }
+
+    await dev.write("other.ts", "export const value = 2;");
+    expect((await dev.fetch("/")).status).toBe(200);
+  },
+});
+devTest("re-bundling a watched file does not leak a file descriptor", {
+  // Counts the dev server's descriptors through /proc.
+  skip: ["win32", "darwin"],
+  files: {
+    "index.html": emptyHtmlFile({
+      scripts: ["index.ts"],
+    }),
+    "index.ts": `
+      import { value } from "./other";
+      console.log(value);
+    `,
+    "other.ts": `
+      export const value = 0;
+    `,
+  },
+  async test(dev) {
+    expect((await dev.fetch("/")).status).toBe(200);
+
+    const other = dev.join("other.ts");
+    const fdDir = `/proc/${dev.devProcess.pid}/fd`;
+    const descriptorsForOther = () => {
+      let count = 0;
+      for (const fd of readdirSync(fdDir)) {
+        let target: string;
+        try {
+          target = readlinkSync(`${fdDir}/${fd}`);
+        } catch {
+          continue;
+        }
+        if (target === other) count++;
+      }
+      return count;
+    };
+
+    // The watcher keeps one descriptor for the file. Each re-bundle opens
+    // another one to read it, which must be closed once the watcher declines it.
+    const before = descriptorsForOther();
+    for (let i = 1; i <= 5; i++) {
+      await dev.write("other.ts", `export const value = ${i};`);
+    }
+    expect(descriptorsForOther()).toBe(before);
+  },
+});
 // Regression test: DirectoryWatchStore.Dep.source_file_path borrows the key
 // string from IncrementalGraph.bundled_files. When a client-component boundary
 // is demoted (its "use client" directive is removed) the server graph calls