diff --git a/src/collections/multi_array_list.rs b/src/collections/multi_array_list.rs index 4476c01129eb..96c882b8ff77 100644 --- a/src/collections/multi_array_list.rs +++ b/src/collections/multi_array_list.rs @@ -990,29 +990,33 @@ 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) { + /// retain list ordering. Returns the removed element, like [`pop`](Self::pop). + 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) { + /// 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, "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 +1426,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 / 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 { + name: Box<[u8]>, + n: u32, + } + + // Under Miri this also checks that every `name` is freed exactly once. + #[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..22a5043e32cf 100644 --- a/src/watcher/Watcher.rs +++ b/src/watcher/Watcher.rs @@ -438,7 +438,8 @@ impl Watcher { if item == last_item || self.watchlist.len() <= item as usize { continue; } - self.watchlist.swap_remove(item as usize); + // 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 // carries its old `udata` (= pre-swap index). Rewrite it so subsequent kevents @@ -1051,6 +1052,7 @@ impl fmt::Display for Op { // ─── WatchItem ──────────────────────────────────────────────────────────── pub struct WatchItem { + /// 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, diff --git a/test/cli/hot/watch-many-dirs.test.ts b/test/cli/hot/watch-many-dirs.test.ts index 547226a7b3e4..991ef6c3b72f 100644 --- a/test/cli/hot/watch-many-dirs.test.ts +++ b/test/cli/hot/watch-many-dirs.test.ts @@ -182,4 +182,67 @@ 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, 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("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