-
Notifications
You must be signed in to change notification settings - Fork 282
Ignore nested node_modules and .git in the workspace root watch #2041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
SawyerHood
merged 3 commits into
main
from
bb/fix-1779-recursive-workspace-watch-can-oom-the-h-thr_2dyz3mvb6z
Aug 20, 2026
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
276 changes: 276 additions & 0 deletions
276
packages/host-watcher/test/workspace-root-ignores.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,276 @@ | ||
| // Regression tests for get-bb/bb#1779: the workspace-root watch of an | ||
| // "umbrella" root (a directory with untracked nested checkouts that contain | ||
| // node_modules and .git) must not register an inotify watch on every nested | ||
| // directory, and must not report changes inside those trees. The Git-derived | ||
| // ignore list only covers the root's own top-level ignored directories, so the | ||
| // watcher has to add recursive glob ignores. | ||
| // | ||
| // The inotify tests are Linux only: they read the real watch count from | ||
| // /proc/self/fdinfo. The event test runs on every platform because parcel | ||
| // applies the globs during the crawl on Linux but per event on macOS/Windows. | ||
| import { execFile } from "node:child_process"; | ||
| import fsSync from "node:fs"; | ||
| import fs from "node:fs/promises"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { promisify } from "node:util"; | ||
| import parcelWatcher from "@parcel/watcher"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { watchWorkspaceStatus } from "../src/watch-status.js"; | ||
| import type { WorkspaceStatusChangeEvent } from "../src/watch-status-types.js"; | ||
|
|
||
| const execFileAsync = promisify(execFile); | ||
| const tempDirs: string[] = []; | ||
| // Bind once at module load so a spy left behind by a failed test can never | ||
| // become the "real" implementation of the next spy. | ||
| const realParcelSubscribe = parcelWatcher.subscribe.bind(parcelWatcher); | ||
|
|
||
| const NESTED_REPOS = 4; | ||
| const PACKAGES_PER_NESTED_REPO = 300; | ||
| const EVENT_TIMEOUT_MS = 5_000; | ||
| // The umbrella tree has thousands of directories; CI runners build it slowly. | ||
| const TEST_TIMEOUT_MS = 60_000; | ||
| // Root, apps/, apps/child-N and the root's own git-dir metadata watches. | ||
| const MAX_EXPECTED_WATCHES = 20; | ||
|
|
||
| async function git(cwd: string, ...args: string[]): Promise<void> { | ||
| await execFileAsync("git", args, { cwd, encoding: "utf8" }); | ||
| } | ||
|
|
||
| async function initRepo(dir: string): Promise<void> { | ||
| await fs.mkdir(dir, { recursive: true }); | ||
| await git(dir, "init", "-q", "-b", "main"); | ||
| await git(dir, "config", "user.name", "BB Tests"); | ||
| await git(dir, "config", "user.email", "bb@example.com"); | ||
| await fs.writeFile(path.join(dir, "README.md"), "hello\n"); | ||
| await git(dir, "add", "README.md"); | ||
| await git(dir, "commit", "-q", "-m", "init"); | ||
| } | ||
|
|
||
| async function buildUmbrellaRoot(args: { | ||
| gitRoot: boolean; | ||
| nestedRepos?: number; | ||
| packagesPerNestedRepo?: number; | ||
| }): Promise<{ | ||
| root: string; | ||
| nestedDirCount: number; | ||
| }> { | ||
| const root = await fs.mkdtemp(path.join(os.tmpdir(), "bb-1779-umbrella-")); | ||
| tempDirs.push(root); | ||
| if (args.gitRoot) { | ||
| await initRepo(root); | ||
| } | ||
| const nestedRepos = args.nestedRepos ?? NESTED_REPOS; | ||
| const packagesPerNestedRepo = | ||
| args.packagesPerNestedRepo ?? PACKAGES_PER_NESTED_REPO; | ||
| let nestedDirCount = 0; | ||
| for (let i = 0; i < nestedRepos; i += 1) { | ||
| const child = path.join(root, "apps", `child-${i}`); | ||
| await initRepo(child); | ||
| // The nested repo ignores its own node_modules, like every real project. | ||
| await fs.writeFile(path.join(child, ".gitignore"), "node_modules/\n"); | ||
| await git(child, "add", ".gitignore"); | ||
| await git(child, "commit", "-q", "-m", "ignore node_modules"); | ||
| await Promise.all( | ||
| Array.from({ length: packagesPerNestedRepo }, async (_unused, p) => { | ||
| const pkgLib = path.join(child, "node_modules", `pkg-${p}`, "lib"); | ||
| await fs.mkdir(pkgLib, { recursive: true }); | ||
| await fs.writeFile( | ||
| path.join(pkgLib, "index.js"), | ||
| "module.exports={}\n", | ||
| ); | ||
| }), | ||
| ); | ||
| nestedDirCount += packagesPerNestedRepo * 2; | ||
| } | ||
| return { root, nestedDirCount }; | ||
| } | ||
|
|
||
| function countInotifyWatches(): number { | ||
| const fdinfoDir = "/proc/self/fdinfo"; | ||
| let count = 0; | ||
| for (const fd of fsSync.readdirSync(fdinfoDir)) { | ||
| try { | ||
| const info = fsSync.readFileSync(path.join(fdinfoDir, fd), "utf8"); | ||
| count += info | ||
| .split("\n") | ||
| .filter((line) => line.startsWith("inotify wd:")).length; | ||
| } catch { | ||
| // fd closed between readdir and read | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
|
|
||
| async function measureWorkspaceRootWatch(root: string): Promise<{ | ||
| ignore: string[] | undefined; | ||
| watches: number; | ||
| }> { | ||
| const seenOptions: Array<{ dir: string; ignore: string[] | undefined }> = []; | ||
| vi.spyOn(parcelWatcher, "subscribe").mockImplementation( | ||
| async (dir, cb, opts) => { | ||
| seenOptions.push({ dir, ignore: opts?.ignore }); | ||
| return realParcelSubscribe(dir, cb, opts); | ||
| }, | ||
| ); | ||
| const baselineWatches = countInotifyWatches(); | ||
| let ready!: () => void; | ||
| const readyPromise = new Promise<void>((resolve) => { | ||
| ready = resolve; | ||
| }); | ||
| const stop = watchWorkspaceStatus(root, { | ||
| onChange: () => undefined, | ||
| onReady: () => ready(), | ||
| onWatchError: () => undefined, | ||
| }); | ||
| try { | ||
| await readyPromise; | ||
| // Settle the metadata (git-dir) subscriptions too. | ||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
| const realRoot = fsSync.realpathSync(root); | ||
| const workspaceRootSubscribe = seenOptions.find( | ||
| (o) => o.dir === root || o.dir === realRoot, | ||
| ); | ||
| return { | ||
| ignore: workspaceRootSubscribe?.ignore, | ||
| watches: countInotifyWatches() - baselineWatches, | ||
| }; | ||
| } finally { | ||
| await stop(); | ||
| } | ||
| } | ||
|
|
||
| async function waitFor( | ||
| predicate: () => boolean, | ||
| timeoutMs: number, | ||
| ): Promise<void> { | ||
| const deadline = Date.now() + timeoutMs; | ||
| while (!predicate()) { | ||
| if (Date.now() > deadline) { | ||
| throw new Error("Timed out waiting for workspace change events"); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| } | ||
| } | ||
|
|
||
| afterEach(async () => { | ||
| vi.restoreAllMocks(); | ||
| for (const dir of tempDirs.splice(0)) { | ||
| await fs.rm(dir, { force: true, recursive: true }); | ||
| } | ||
| }); | ||
|
|
||
| describe.skipIf(process.platform !== "linux")( | ||
| "workspace root watch ignores nested heavy directories (#1779)", | ||
| () => { | ||
| it( | ||
| "does not watch nested node_modules or .git under a git umbrella root", | ||
| async () => { | ||
| const { root, nestedDirCount } = await buildUmbrellaRoot({ | ||
| gitRoot: true, | ||
| }); | ||
| const { ignore, watches } = await measureWorkspaceRootWatch(root); | ||
| expect(nestedDirCount).toBeGreaterThan(MAX_EXPECTED_WATCHES); | ||
| expect(ignore).toContain(".git"); | ||
| expect(ignore).toContain("**/node_modules/**"); | ||
| expect(watches).toBeLessThan(MAX_EXPECTED_WATCHES); | ||
| }, | ||
| TEST_TIMEOUT_MS, | ||
| ); | ||
|
|
||
| it( | ||
| "does not watch nested node_modules or .git under a non-git root", | ||
| async () => { | ||
| const { root, nestedDirCount } = await buildUmbrellaRoot({ | ||
| gitRoot: false, | ||
| }); | ||
| const { ignore, watches } = await measureWorkspaceRootWatch(root); | ||
| expect(nestedDirCount).toBeGreaterThan(MAX_EXPECTED_WATCHES); | ||
| // `<root>/.git` must stay watchable so `git init` promotion still fires. | ||
| expect(ignore).not.toContain(".git"); | ||
| expect(ignore).toContain("**/node_modules/**"); | ||
| expect(watches).toBeLessThan(MAX_EXPECTED_WATCHES); | ||
| }, | ||
| TEST_TIMEOUT_MS, | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| describe("workspace root watch events inside nested heavy directories (#1779)", () => { | ||
| it( | ||
| "does not report changes inside nested node_modules or nested .git", | ||
| async () => { | ||
| const { root } = await buildUmbrellaRoot({ | ||
| gitRoot: true, | ||
| nestedRepos: 1, | ||
| packagesPerNestedRepo: 1, | ||
| }); | ||
| const realRoot = fsSync.realpathSync(root); | ||
| const nestedPackageFile = path.join( | ||
| realRoot, | ||
| "apps", | ||
| "child-0", | ||
| "node_modules", | ||
| "pkg-0", | ||
| "lib", | ||
| "index.js", | ||
| ); | ||
| const nestedGitFile = path.join( | ||
| realRoot, | ||
| "apps", | ||
| "child-0", | ||
| ".git", | ||
| "bb-marker", | ||
| ); | ||
| const visibleFile = path.join(realRoot, "apps", "child-0", "visible.txt"); | ||
| const events: WorkspaceStatusChangeEvent[] = []; | ||
| let ready!: () => void; | ||
| const readyPromise = new Promise<void>((resolve) => { | ||
| ready = resolve; | ||
| }); | ||
| const stop = watchWorkspaceStatus(root, { | ||
| onChange: (event) => { | ||
| events.push(event); | ||
| }, | ||
| onReady: () => ready(), | ||
| onWatchError: () => undefined, | ||
| }); | ||
| try { | ||
| await readyPromise; | ||
| // Let the initial crawl and the FSEvents stream settle. | ||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
|
|
||
| await fs.writeFile( | ||
| nestedPackageFile, | ||
| "module.exports={changed:true}\n", | ||
| ); | ||
| await fs.writeFile(nestedGitFile, "marker\n"); | ||
| // The visible write is the control: it proves the watch is live and | ||
| // delivers events, so the absence of the nested paths is meaningful. | ||
| await fs.writeFile(visibleFile, "visible\n"); | ||
| await waitFor( | ||
| () => | ||
| events.some((event) => event.changedPaths.includes(visibleFile)), | ||
| EVENT_TIMEOUT_MS, | ||
| ); | ||
| // Give any straggling nested events a chance to arrive. | ||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
|
|
||
| const changedPaths = events.flatMap((event) => event.changedPaths); | ||
| expect(changedPaths).toContain(visibleFile); | ||
| expect(changedPaths).not.toContain(nestedPackageFile); | ||
| expect(changedPaths).not.toContain(nestedGitFile); | ||
| expect( | ||
| changedPaths.filter( | ||
| (changedPath) => | ||
| changedPath.includes(`${path.sep}node_modules${path.sep}`) || | ||
| changedPath.includes(`${path.sep}.git${path.sep}`), | ||
| ), | ||
| ).toEqual([]); | ||
| } finally { | ||
| await stop(); | ||
| } | ||
| }, | ||
| TEST_TIMEOUT_MS, | ||
| ); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨
slopcop/review— Different same-root subscriptions make this fix depend on start order.Parcel keeps one directory tree for each root, even when subscriptions use different ignore sets. If a plugin watch starts first, this workspace watch reuses the full tree and can still create every Linux watch. If this watch starts first, a later plugin watch cannot see existing excluded paths. Isolate subscriptions with different ignore sets. Add a Linux test for both start orders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I measured both start orders on Linux with two real parcel subscriptions on the same umbrella root (one plain, one with the workspace globs):
So the claim "this workspace watch reuses the full tree and can still create every Linux watch" is not what happens:
InotifyBackend::watchDirreuses the same inotify fd, so the workspace watch adds zero watches when a full-tree subscription already exists. The watch count is set by the least restrictive subscription on that root, which in that scenario is the other subscription's ignore list, not this one.The reverse order is real, but it is parcel's per-root
DirTree::getCachedcache and it is not introduced by this PR: the existing.gitand Git-derived ignores (.turbo,coverage, …) already prune the shared tree the same way for any later same-root subscription. Parcel keeps the tree in a C++ singleton keyed by the resolved root, so separate subscriptions in one backend cannot be isolated from the JS side; that would need a parcel change or separate watcher processes. I am not changing it in this PR, which is scoped to the #1779 OOM. If we want to track the shared-tree limitation for same-root plugin watches, that belongs in its own issue.