From 43a895740b0c9a92e0c83c977a4adf4d1ed6a5d8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:56:40 +0000 Subject: [PATCH 1/5] watcher: free the owned path of evicted watchlist entries MultiArrayList::swap_remove and ordered_remove copied the remaining rows over the removed one without running its destructor, so Watcher::flush_evictions leaked the Cow::Owned file_path of every evicted entry that had been added with CLONE_FILE_PATH (every module watched by --hot, plugin-loaded files, dev server directory watches, everything on Windows). Under --hot each save of a watched file evicts and re-adds its entry, leaking one path per save. Both removal functions now return the removed element, transferring ownership to the caller the same way pop does, and flush_evictions drops it. The only other caller (HTTP header entries, a Copy type) is unaffected. --- src/collections/multi_array_list.rs | 60 ++++++++++++++++++++-- src/watcher/Watcher.rs | 7 ++- test/cli/hot/watch-many-dirs.test.ts | 77 ++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 5 deletions(-) diff --git a/src/collections/multi_array_list.rs b/src/collections/multi_array_list.rs index 4476c01129eb..5bca5ded1f20 100644 --- a/src/collections/multi_array_list.rs +++ b/src/collections/multi_array_list.rs @@ -991,28 +991,40 @@ impl MultiArrayList { /// Remove the specified item from the list, swapping the last /// item in the list into its position. Fast, but does not /// retain list ordering. - pub fn swap_remove(&mut self, index: usize) { + /// + /// Returns the removed element, whose fields the caller now owns (as with + /// [`pop`](Self::pop)). Nothing else ever runs a removed row's destructor: + /// `Drop` for this list is slab-only and + /// [`drop_elements`](Self::drop_elements) only sees rows still in the list. + pub fn swap_remove(&mut self, index: usize) -> T { assert!( index < self.len, "MultiArrayList::swap_remove: index out of bounds" ); let last = self.len - 1; let mut s = self.slice(); + let removed = s.gather(index); s.copy_rows_within(last, index, 1); self.len -= 1; + removed } /// Remove the specified item from the list, shifting items /// after it to preserve order. - pub fn ordered_remove(&mut self, index: usize) { + /// + /// Returns the removed element; ownership semantics as in + /// [`swap_remove`](Self::swap_remove). + pub fn ordered_remove(&mut self, index: usize) -> T { assert!( index < self.len, "MultiArrayList::ordered_remove: index out of bounds" ); let tail = self.len - 1 - index; let mut s = self.slice(); + let removed = s.gather(index); s.copy_rows_within(index + 1, index, tail); self.len -= 1; + removed } /// Attempt to reduce allocated capacity to `new_len`. @@ -1422,10 +1434,50 @@ mod tests { .unwrap(); } assert_eq!(list.items::<"a", u32>(), &[0, 1, 2, 3, 4, 5]); - list.ordered_remove(2); + assert_eq!(list.ordered_remove(2), Foo { a: 2, b: 2, c: 2 }); assert_eq!(list.items::<"a", u32>(), &[0, 1, 3, 4, 5]); - list.swap_remove(1); + assert_eq!(list.swap_remove(1), Foo { a: 1, b: 1, c: 1 }); assert_eq!(list.items::<"a", u32>(), &[0, 5, 3, 4]); + assert_eq!(list.items::<"c", u64>(), &[0, 5, 3, 4]); + // Removing the last row swaps it with itself. + assert_eq!(list.swap_remove(3), Foo { a: 4, b: 4, c: 4 }); + assert_eq!(list.items::<"a", u32>(), &[0, 5, 3]); + } + + struct Owning { + name: Box<[u8]>, + n: u32, + } + + // Under Miri (`bun run rust:miri`) this also checks that a removed row's + // heap payload is freed exactly once: by the caller dropping the returned + // element, not again by `drop_elements`. + #[test] + fn remove_returns_owned_element() { + let mut list = MultiArrayList::::default(); + for i in 0..4u32 { + list.push(Owning { + name: vec![b'a' + i as u8; 3].into_boxed_slice(), + n: i, + }) + .unwrap(); + } + + let removed = list.swap_remove(1); + assert_eq!(&*removed.name, b"bbb"); + assert_eq!(removed.n, 1); + assert_eq!(list.items::<"n", u32>(), &[0, 3, 2]); + drop(removed); + + let removed = list.ordered_remove(0); + assert_eq!(&*removed.name, b"aaa"); + assert_eq!(list.items::<"n", u32>(), &[3, 2]); + assert_eq!(&*list.items::<"name", Box<[u8]>>()[0], b"ddd"); + assert_eq!(&*list.items::<"name", Box<[u8]>>()[1], b"ccc"); + drop(removed); + + list.drop_elements(); + assert_eq!(list.len(), 0); } #[test] diff --git a/src/watcher/Watcher.rs b/src/watcher/Watcher.rs index 796f0b07b17b..3f1277f9e97a 100644 --- a/src/watcher/Watcher.rs +++ b/src/watcher/Watcher.rs @@ -438,7 +438,9 @@ impl Watcher { if item == last_item || self.watchlist.len() <= item as usize { continue; } - self.watchlist.swap_remove(item as usize); + // Dropping the removed row frees the `file_path` that + // `CLONE_FILE_PATH` entries own; the fd was already closed above. + drop(self.watchlist.swap_remove(item as usize)); // swapRemove put a different entry at `item`, but its kqueue registration still // carries its old `udata` (= pre-swap index). Rewrite it so subsequent kevents @@ -1051,6 +1053,9 @@ impl fmt::Display for Op { // ─── WatchItem ──────────────────────────────────────────────────────────── pub struct WatchItem { + /// `Borrowed` from an interned, process-lifetime path, or `Owned` when the + /// entry was added with `CLONE_FILE_PATH`; an owned path lives until + /// `flush_evictions` removes the entry. pub file_path: Cow<'static, [u8]>, // filepath hash for quick comparison pub hash: u32, diff --git a/test/cli/hot/watch-many-dirs.test.ts b/test/cli/hot/watch-many-dirs.test.ts index 547226a7b3e4..0237160706da 100644 --- a/test/cli/hot/watch-many-dirs.test.ts +++ b/test/cli/hot/watch-many-dirs.test.ts @@ -182,4 +182,81 @@ if (globalThis.reloaded++ >= ${maxCount}) process.exit(0); }, 60000, ); + + // Editing dep.js raises an inotify event on lib/, which makes the reloader + // evict dep's watchlist entry; the reload then re-adds it with a fresh heap + // copy of the path. Eviction used to discard the evicted entry's copy + // without freeing it, so every edit leaked one path. The leak is only + // observable through LSan (ASAN builds); logLevel=debug makes the reloader + // print "Removing file" per eviction, which guards against a reload cycle + // that stopped evicting passing this vacuously. + test.skipIf(!isLinux || !isASAN)( + "evicting watchlist entries does not leak their paths", + async () => { + const edits = 3; + await using dir = tempDir("hot-evict-leak", { + "bunfig.toml": `logLevel = "debug"\n`, + "lib/dep.js": `export const value = 0;`, + "entry.js": ` + import { lsanDoLeakCheck } from "bun:internal-for-testing"; + import { value } from "./lib/dep.js"; + console.log("RELOAD", value); + if (value === ${edits}) { + lsanDoLeakCheck(); + console.log("LEAKCHECK"); + } + `, + }); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "entry.js"], + cwd: String(dir), + env: { + ...bunEnv, + // Bun's built-in ASAN defaults disable LSan. entry.js runs the check + // itself once the edits are done; the process is killed afterwards, + // so the at-exit check is not needed. + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + // verbosity=1 makes the check announce itself on stderr. + LSAN_OPTIONS: "leak_check_at_exit=0:malloc_context_size=30:verbosity=1", + }, + stdout: "pipe", + stderr: "pipe", + }); + const stderrText = proc.stderr.text(); + + const iter = forEachLine(proc.stdout); + const waitForLine = async (expected: string) => { + while (true) { + const { value: line, done } = await iter.next(); + if (done) throw new Error(`--hot exited before printing "${expected}" (exit ${proc.exitCode})`); + if (line === expected) return; + } + }; + + await waitForLine("RELOAD 0"); + for (let i = 1; i <= edits; i++) { + writeFileSync(join(dir, "lib", "dep.js"), `export const value = ${i};`); + await waitForLine(`RELOAD ${i}`); + } + await waitForLine("LEAKCHECK"); + proc.kill(); + const stderr = await stderrText; + + expect(stderr).toContain("LeakSanitizer: checking for leaks"); + // One edit can produce more than one directory event, so this is a lower bound. + const evictions = stderr.split("\n").filter(line => line.includes("Removing file:")).length; + expect(evictions).toBeGreaterThanOrEqual(edits); + + // LSan prints one blank-line-separated block per leaking allocation + // stack. A leaked watchlist path is allocated by bun_watcher's append_* + // functions; leaks from elsewhere are not this test's concern. + const watcherLeaks = stderr + .split(/\n\s*\n/) + .filter(block => /^(?:Direct|Indirect) leak of /.test(block) && block.includes("bun_watcher::")); + expect(watcherLeaks).toEqual([]); + }, + // Debug + ASAN: three reloads plus symbolizing the LSan report take a few seconds. + 30_000, + ); }); From 98005f662f5b4941b9314c0c65df5ceee536df09 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 02:58:43 +0000 Subject: [PATCH 2/5] test: check the eviction leak with LSan at exit, run LSan on bake css tests The --hot test now lets the child exit and relies on the exit-time leak check instead of an on-demand check filtered by symbol name, which does not depend on which frames survive inlining and needs no symbolization when clean. test/bake/dev/css.test.ts leaked the same way through the dev server's directory watches (css-13 and css-14 fail under LSan without the fix) and is clean with it, so it no longer opts out of LSan in CI. --- src/watcher/Watcher.rs | 6 +- test/cli/hot/watch-many-dirs.test.ts | 130 ++++++++++++--------------- test/no-validate-leaksan.txt | 1 - 3 files changed, 61 insertions(+), 76 deletions(-) diff --git a/src/watcher/Watcher.rs b/src/watcher/Watcher.rs index 3f1277f9e97a..729c4a38940a 100644 --- a/src/watcher/Watcher.rs +++ b/src/watcher/Watcher.rs @@ -1053,9 +1053,9 @@ impl fmt::Display for Op { // ─── WatchItem ──────────────────────────────────────────────────────────── pub struct WatchItem { - /// `Borrowed` from an interned, process-lifetime path, or `Owned` when the - /// entry was added with `CLONE_FILE_PATH`; an owned path lives until - /// `flush_evictions` removes the entry. + /// `Owned` when the entry was added with `CLONE_FILE_PATH`, and freed when + /// `flush_evictions` removes the entry. Otherwise `Borrowed`, and the caller + /// must keep the bytes alive for as long as the entry exists. pub file_path: Cow<'static, [u8]>, // filepath hash for quick comparison pub hash: u32, diff --git a/test/cli/hot/watch-many-dirs.test.ts b/test/cli/hot/watch-many-dirs.test.ts index 0237160706da..991ef6c3b72f 100644 --- a/test/cli/hot/watch-many-dirs.test.ts +++ b/test/cli/hot/watch-many-dirs.test.ts @@ -186,77 +186,63 @@ if (globalThis.reloaded++ >= ${maxCount}) process.exit(0); // Editing dep.js raises an inotify event on lib/, which makes the reloader // evict dep's watchlist entry; the reload then re-adds it with a fresh heap // copy of the path. Eviction used to discard the evicted entry's copy - // without freeing it, so every edit leaked one path. The leak is only - // observable through LSan (ASAN builds); logLevel=debug makes the reloader - // print "Removing file" per eviction, which guards against a reload cycle - // that stopped evicting passing this vacuously. - test.skipIf(!isLinux || !isASAN)( - "evicting watchlist entries does not leak their paths", - async () => { - const edits = 3; - await using dir = tempDir("hot-evict-leak", { - "bunfig.toml": `logLevel = "debug"\n`, - "lib/dep.js": `export const value = 0;`, - "entry.js": ` - import { lsanDoLeakCheck } from "bun:internal-for-testing"; - import { value } from "./lib/dep.js"; - console.log("RELOAD", value); - if (value === ${edits}) { - lsanDoLeakCheck(); - console.log("LEAKCHECK"); - } - `, - }); - - await using proc = spawn({ - cmd: [bunExe(), "--hot", "entry.js"], - cwd: String(dir), - env: { - ...bunEnv, - // Bun's built-in ASAN defaults disable LSan. entry.js runs the check - // itself once the edits are done; the process is killed afterwards, - // so the at-exit check is not needed. - ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), - // verbosity=1 makes the check announce itself on stderr. - LSAN_OPTIONS: "leak_check_at_exit=0:malloc_context_size=30:verbosity=1", - }, - stdout: "pipe", - stderr: "pipe", - }); - const stderrText = proc.stderr.text(); - - const iter = forEachLine(proc.stdout); - const waitForLine = async (expected: string) => { - while (true) { - const { value: line, done } = await iter.next(); - if (done) throw new Error(`--hot exited before printing "${expected}" (exit ${proc.exitCode})`); - if (line === expected) return; - } - }; - - await waitForLine("RELOAD 0"); - for (let i = 1; i <= edits; i++) { - writeFileSync(join(dir, "lib", "dep.js"), `export const value = ${i};`); - await waitForLine(`RELOAD ${i}`); + // without freeing it, so every edit leaked one path, which LSan reports when + // the process exits (the same check CI's ASAN lane applies to every test + // process). logLevel=debug makes the reloader log each eviction, so a reload + // cycle that stopped evicting cannot pass this vacuously. + test.skipIf(!isLinux || !isASAN)("evicting watchlist entries does not leak their paths", async () => { + const edits = 3; + await using dir = tempDir("hot-evict-leak", { + "bunfig.toml": `logLevel = "debug"\n`, + "lib/dep.js": `export const value = 0;`, + "entry.js": ` + import { value } from "./lib/dep.js"; + console.log("RELOAD", value); + if (value === ${edits}) process.exit(0); + `, + }); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "entry.js"], + cwd: String(dir), + env: { + ...bunEnv, + // Bun's built-in ASAN defaults turn LSan off. Destructing the VM on exit + // frees what JS still referenced, so only lost allocations get reported. + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + // verbosity=1 makes the exit-time check announce itself on stderr. + LSAN_OPTIONS: [bunEnv.LSAN_OPTIONS, "verbosity=1"].filter(Boolean).join(":"), + BUN_DESTRUCT_VM_ON_EXIT: "1", + }, + stdout: "pipe", + stderr: "pipe", + }); + const stderrText = proc.stderr.text(); + + const iter = forEachLine(proc.stdout); + const waitForLine = async (expected: string) => { + while (true) { + const { value: line, done } = await iter.next(); + if (done) throw new Error(`--hot exited before printing "${expected}" (exit ${proc.exitCode})`); + if (line === expected) return; } - await waitForLine("LEAKCHECK"); - proc.kill(); - const stderr = await stderrText; - - expect(stderr).toContain("LeakSanitizer: checking for leaks"); - // One edit can produce more than one directory event, so this is a lower bound. - const evictions = stderr.split("\n").filter(line => line.includes("Removing file:")).length; - expect(evictions).toBeGreaterThanOrEqual(edits); - - // LSan prints one blank-line-separated block per leaking allocation - // stack. A leaked watchlist path is allocated by bun_watcher's append_* - // functions; leaks from elsewhere are not this test's concern. - const watcherLeaks = stderr - .split(/\n\s*\n/) - .filter(block => /^(?:Direct|Indirect) leak of /.test(block) && block.includes("bun_watcher::")); - expect(watcherLeaks).toEqual([]); - }, - // Debug + ASAN: three reloads plus symbolizing the LSan report take a few seconds. - 30_000, - ); + }; + + await waitForLine("RELOAD 0"); + for (let i = 1; i <= edits; i++) { + writeFileSync(join(dir, "lib", "dep.js"), `export const value = ${i};`); + await waitForLine(`RELOAD ${i}`); + } + const [stderr, exitCode] = await Promise.all([stderrText, proc.exited]); + + // One edit can produce more than one directory event, so this is a lower bound. + const evictions = stderr.split("\n").filter(line => line.includes("Removing file:")).length; + expect(evictions).toBeGreaterThanOrEqual(edits); + expect(stderr).toContain("LeakSanitizer: checking for leaks"); + // LSan prints one blank-line-separated block per leaking allocation stack + // (each naming its allocation site) and then fails the exit. + const leaks = stderr.split(/\n\s*\n/).filter(block => /^(?:Direct|Indirect) leak of /.test(block)); + expect(leaks).toEqual([]); + expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); + }); }); diff --git a/test/no-validate-leaksan.txt b/test/no-validate-leaksan.txt index e4f53b8bfa48..d8277ea05ea3 100644 --- a/test/no-validate-leaksan.txt +++ b/test/no-validate-leaksan.txt @@ -363,7 +363,6 @@ test/bake/dev/server-sourcemap.test.ts # Watcher Thread test/bake/dev-and-prod.test.ts test/bake/dev/bundle.test.ts -test/bake/dev/css.test.ts test/bake/dev/esm.test.ts test/bake/dev/hot.test.ts test/bake/dev/react-spa.test.ts From 723d0acb85d379449b8e9843ff3dbe6e0474811f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 03:02:29 +0000 Subject: [PATCH 3/5] watcher, collections: shorten the comments on the removal path --- src/collections/multi_array_list.rs | 16 +++------------- src/watcher/Watcher.rs | 8 +++----- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/src/collections/multi_array_list.rs b/src/collections/multi_array_list.rs index 5bca5ded1f20..7d11c3d7c93b 100644 --- a/src/collections/multi_array_list.rs +++ b/src/collections/multi_array_list.rs @@ -990,12 +990,7 @@ impl MultiArrayList { /// Remove the specified item from the list, swapping the last /// item in the list into its position. Fast, but does not - /// retain list ordering. - /// - /// Returns the removed element, whose fields the caller now owns (as with - /// [`pop`](Self::pop)). Nothing else ever runs a removed row's destructor: - /// `Drop` for this list is slab-only and - /// [`drop_elements`](Self::drop_elements) only sees rows still in the list. + /// retain list ordering. Returns the removed element, like [`pop`](Self::pop). pub fn swap_remove(&mut self, index: usize) -> T { assert!( index < self.len, @@ -1010,10 +1005,7 @@ impl MultiArrayList { } /// Remove the specified item from the list, shifting items - /// after it to preserve order. - /// - /// Returns the removed element; ownership semantics as in - /// [`swap_remove`](Self::swap_remove). + /// after it to preserve order. Returns the removed element, like [`pop`](Self::pop). pub fn ordered_remove(&mut self, index: usize) -> T { assert!( index < self.len, @@ -1449,9 +1441,7 @@ mod tests { n: u32, } - // Under Miri (`bun run rust:miri`) this also checks that a removed row's - // heap payload is freed exactly once: by the caller dropping the returned - // element, not again by `drop_elements`. + // Under Miri this also checks that every `name` is freed exactly once. #[test] fn remove_returns_owned_element() { let mut list = MultiArrayList::::default(); diff --git a/src/watcher/Watcher.rs b/src/watcher/Watcher.rs index 729c4a38940a..97f40bdeac8e 100644 --- a/src/watcher/Watcher.rs +++ b/src/watcher/Watcher.rs @@ -438,8 +438,7 @@ impl Watcher { if item == last_item || self.watchlist.len() <= item as usize { continue; } - // Dropping the removed row frees the `file_path` that - // `CLONE_FILE_PATH` entries own; the fd was already closed above. + // Frees an owned `file_path`; the fd was closed in the first pass. drop(self.watchlist.swap_remove(item as usize)); // swapRemove put a different entry at `item`, but its kqueue registration still @@ -1053,9 +1052,8 @@ impl fmt::Display for Op { // ─── WatchItem ──────────────────────────────────────────────────────────── pub struct WatchItem { - /// `Owned` when the entry was added with `CLONE_FILE_PATH`, and freed when - /// `flush_evictions` removes the entry. Otherwise `Borrowed`, and the caller - /// must keep the bytes alive for as long as the entry exists. + /// `Owned` for `CLONE_FILE_PATH` entries (freed by `flush_evictions`); + /// otherwise the caller keeps the bytes alive for as long as the entry exists. pub file_path: Cow<'static, [u8]>, // filepath hash for quick comparison pub hash: u32, From 96d34fb5dbcad713067575b46685eaab44d0b719 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 03:03:53 +0000 Subject: [PATCH 4/5] watcher: one-line doc for WatchItem.file_path --- src/watcher/Watcher.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/watcher/Watcher.rs b/src/watcher/Watcher.rs index 97f40bdeac8e..22a5043e32cf 100644 --- a/src/watcher/Watcher.rs +++ b/src/watcher/Watcher.rs @@ -1052,8 +1052,7 @@ impl fmt::Display for Op { // ─── WatchItem ──────────────────────────────────────────────────────────── pub struct WatchItem { - /// `Owned` for `CLONE_FILE_PATH` entries (freed by `flush_evictions`); - /// otherwise the caller keeps the bytes alive for as long as the entry exists. + /// Freed by `flush_evictions` when `Owned`; borrowed bytes must outlive the entry. pub file_path: Cow<'static, [u8]>, // filepath hash for quick comparison pub hash: u32, From 4163a489ed7308cd605b419266aaeea7976263c0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 03:08:31 +0000 Subject: [PATCH 5/5] collections: cover ordered_remove of the last row in the unit test --- src/collections/multi_array_list.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/collections/multi_array_list.rs b/src/collections/multi_array_list.rs index 7d11c3d7c93b..96c882b8ff77 100644 --- a/src/collections/multi_array_list.rs +++ b/src/collections/multi_array_list.rs @@ -1431,9 +1431,11 @@ mod tests { assert_eq!(list.swap_remove(1), Foo { a: 1, b: 1, c: 1 }); assert_eq!(list.items::<"a", u32>(), &[0, 5, 3, 4]); assert_eq!(list.items::<"c", u64>(), &[0, 5, 3, 4]); - // Removing the last row swaps it with itself. + // Removing the last row swaps it with itself / shifts nothing. assert_eq!(list.swap_remove(3), Foo { a: 4, b: 4, c: 4 }); assert_eq!(list.items::<"a", u32>(), &[0, 5, 3]); + assert_eq!(list.ordered_remove(2), Foo { a: 3, b: 3, c: 3 }); + assert_eq!(list.items::<"a", u32>(), &[0, 5]); } struct Owning {