diff --git a/.changeset/assets-watcher-ebadf-macos.md b/.changeset/assets-watcher-ebadf-macos.md new file mode 100644 index 0000000000..964ce084d4 --- /dev/null +++ b/.changeset/assets-watcher-ebadf-macos.md @@ -0,0 +1,7 @@ +--- +"wrangler": patch +--- + +Fix `wrangler dev` crashing with `spawn EBADF` on macOS when the assets directory contains many files + +On macOS, chokidar 4 opens one `fs.watch` handle per asset file. Past the process file-descriptor limit this makes child-process spawns fail with `EBADF`, so `wrangler dev` exits. `wrangler dev` now watches the assets directory with a single recursive `fs.watch` on macOS and Windows, which uses one native handle for the whole tree. diff --git a/packages/wrangler/src/__tests__/api/startDevWorker/BundlerController.assets-watcher.test.ts b/packages/wrangler/src/__tests__/api/startDevWorker/BundlerController.assets-watcher.test.ts index 24d63983c4..b39672e60f 100644 --- a/packages/wrangler/src/__tests__/api/startDevWorker/BundlerController.assets-watcher.test.ts +++ b/packages/wrangler/src/__tests__/api/startDevWorker/BundlerController.assets-watcher.test.ts @@ -1,5 +1,6 @@ import assert from "node:assert"; import { EventEmitter } from "node:events"; +import * as fs from "node:fs"; import path from "node:path"; import { setTimeout } from "node:timers/promises"; import { runInTempDir, seed } from "@cloudflare/workers-utils/test-helpers"; @@ -17,11 +18,22 @@ import { FakeBus } from "../../helpers/fake-bus"; import { mockConsoleMethods } from "../../helpers/mock-console"; import type { StartDevWorkerOptions } from "../../../api"; import type { FSWatcher, Matcher } from "chokidar"; +import type { WatchOptions } from "node:fs"; // Mock chokidar so we can drive watcher events and errors directly, without // depending on the filesystem delivering them. vi.mock("chokidar"); +// Replace only `watch` so darwin/win32 tests can drive a recursive native +// watcher. Other fs methods stay real — this suite seeds a temp directory. +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + watch: vi.fn(actual.watch), + }; +}); + function configDefaults( overrides: Partial = {} ): StartDevWorkerOptions { @@ -129,6 +141,64 @@ function mockWatchers(expect: ExpectStatic, { closeDelayMs = 0 } = {}) { }; } +type FsWatchCall = { + watchPath: string; + options: WatchOptions | undefined; +}; + +/** + * Make `fs.watch()` hand the controller native watchers whose events we drive + * ourselves, recording every call so darwin/win32 tests can assert a single + * recursive watch and close them. + */ +function mockFsWatch(expect: ExpectStatic, { closeDelayMs = 0 } = {}) { + const created: FakeWatcher[] = []; + const calls: FsWatchCall[] = []; + vi.mocked(fs.watch).mockImplementation((watchPath, options) => { + const watchOptions = + typeof options === "object" && options !== null + ? (options as WatchOptions) + : undefined; + calls.push({ watchPath: String(watchPath), options: watchOptions }); + const fakeWatcher = new EventEmitter() as FakeWatcher; + let closing: Promise | undefined; + // Repeated `close()` calls return the same promise, matching the + // chokidar fake, so reload-during-close tests can delay teardown. + fakeWatcher.close = vi.fn(() => (closing ??= setTimeout(closeDelayMs))); + created.push(fakeWatcher); + return fakeWatcher as unknown as fs.FSWatcher; + }); + return { + created, + calls, + /** Wait for the watcher at `index` to be created, then return it. */ + async at(index: number) { + await vi.waitFor(() => expect(created.length).toBeGreaterThan(index)); + const watcher = created[index]; + assert(watcher); + return watcher; + }, + }; +} + +function stubPlatform(platform: NodeJS.Platform) { + const original = process.platform; + Object.defineProperty(process, "platform", { + value: platform, + configurable: true, + }); + return () => { + Object.defineProperty(process, "platform", { + value: original, + configurable: true, + }); + }; +} + +function bundleCompleteCount(bus: FakeBus) { + return bus.events.filter((event) => event.type === "bundleComplete").length; +} + describe("BundlerController — assets watcher", () => { const std = mockConsoleMethods(); runInTempDir(); @@ -371,4 +441,263 @@ describe("BundlerController — assets watcher", () => { expect(watcher.close).toHaveBeenCalled(); } }); + + test("does not use recursive fs.watch on linux", async ({ expect }) => { + vi.mocked(fs.watch).mockClear(); + const watchers = mockWatchers(expect); + + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + await firstBundle; + await watchers.at(0); + + expect(fs.watch).not.toHaveBeenCalled(); + }); }); + +describe.each(["darwin", "win32"] as const)( + "BundlerController — assets watcher (%s recursive fs.watch)", + (platform) => { + const std = mockConsoleMethods(); + runInTempDir(); + + let bus: FakeBus; + let controller: BundlerController; + let restorePlatform: () => void; + + beforeEach(async () => { + restorePlatform = stubPlatform(platform); + vi.mocked(fs.watch).mockReset(); + vi.mocked(watch).mockReset(); + bus = new FakeBus(); + controller = new BundlerController(bus); + + await seed({ + "src/index.ts": `export default { fetch() { return new Response("ok"); } }`, + "assets/placeholder.txt": "hello", + }); + }); + + afterEach(async () => { + await controller.teardown(); + restorePlatform(); + }); + + test("watches the assets directory with a single recursive fs.watch", async ({ + expect, + }) => { + const fsWatchers = mockFsWatch(expect); + const assetsDir = path.resolve("assets"); + + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + await firstBundle; + await fsWatchers.at(0); + + expect(fsWatchers.calls).toEqual([ + { + watchPath: assetsDir, + options: expect.objectContaining({ + persistent: true, + recursive: true, + }), + }, + ]); + expect(vi.mocked(watch)).not.toHaveBeenCalled(); + }); + + test("an asset change refreshes the current bundle", async ({ expect }) => { + const fsWatchers = mockFsWatch(expect); + + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + await firstBundle; + const fakeWatcher = await fsWatchers.at(0); + + const secondBundle = bus.waitFor("bundleComplete"); + fakeWatcher.emit("change", "change", "placeholder.txt"); + await secondBundle; + + expect(bundleCompleteCount(bus)).toBe(2); + }); + + test("ignores changes under the dev session write directories", async ({ + expect, + }) => { + const fsWatchers = mockFsWatch(expect); + + const projectRoot = path.resolve("."); + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsAt(projectRoot), + }); + await firstBundle; + const fakeWatcher = await fsWatchers.at(0); + + fakeWatcher.emit( + "change", + "change", + path.join(".wrangler", "tmp", "built.js") + ); + await setTimeout(500); + expect(bundleCompleteCount(bus)).toBe(1); + + const secondBundle = bus.waitFor("bundleComplete"); + fakeWatcher.emit("change", "change", "index.html"); + await secondBundle; + expect(bundleCompleteCount(bus)).toBe(2); + }); + + test("ignores a persist directory inside the assets tree", async ({ + expect, + }) => { + const fsWatchers = mockFsWatch(expect); + + const projectRoot = path.resolve("."); + const persist = path.join(projectRoot, "persist-state"); + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsAt(projectRoot, persist), + }); + await firstBundle; + const fakeWatcher = await fsWatchers.at(0); + + fakeWatcher.emit( + "change", + "change", + path.join("persist-state", "kv.sqlite") + ); + await setTimeout(500); + expect(bundleCompleteCount(bus)).toBe(1); + + const secondBundle = bus.waitFor("bundleComplete"); + fakeWatcher.emit("change", "change", "index.html"); + await secondBundle; + expect(bundleCompleteCount(bus)).toBe(2); + }); + + test("keeps watching when the persist path is the assets directory", async ({ + expect, + }) => { + const fsWatchers = mockFsWatch(expect); + + const assetsDir = path.resolve("public"); + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsAt(assetsDir, assetsDir), + }); + await firstBundle; + const fakeWatcher = await fsWatchers.at(0); + + const secondBundle = bus.waitFor("bundleComplete"); + fakeWatcher.emit("change", "change", "index.html"); + await secondBundle; + expect(bundleCompleteCount(bus)).toBe(2); + }); + + test("a pending assets refresh is discarded on teardown", async ({ + expect, + }) => { + const fsWatchers = mockFsWatch(expect); + + const firstBundle = bus.waitFor("bundleComplete"); + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + await firstBundle; + const fakeWatcher = await fsWatchers.at(0); + + fakeWatcher.emit("change", "change", "placeholder.txt"); + await controller.teardown(); + + const eventsAtTeardown = bus.events.length; + await setTimeout(500); + expect(bus.events.slice(eventsAtTeardown)).toEqual([]); + }); + + test( + "logs a warning and disables the watcher when the native watcher emits EMFILE", + { timeout: 5_000 }, + async ({ expect }) => { + const fsWatchers = mockFsWatch(expect); + + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + const fakeWatcher = await fsWatchers.at(0); + + const emfileError = Object.assign( + new Error("EMFILE: too many open files, watch"), + { code: "EMFILE" } + ); + fakeWatcher.emit("error", emfileError); + await setTimeout(0); + + expect(std.warn).toContain("platform limit"); + expect(std.warn).toContain("flattening"); + expect(fakeWatcher.close).toHaveBeenCalled(); + } + ); + + test( + "logs a warning and closes the watcher for non-EMFILE watcher errors", + { timeout: 5_000 }, + async ({ expect }) => { + const fsWatchers = mockFsWatch(expect); + + controller.onConfigUpdate({ + type: "configUpdate", + config: assetsConfig(), + }); + const fakeWatcher = await fsWatchers.at(0); + + const genericError = new Error("EACCES: permission denied"); + fakeWatcher.emit("error", genericError); + await setTimeout(0); + + expect(std.warn).toContain( + "encountered an error and has been disabled" + ); + expect(std.warn).toContain("EACCES: permission denied"); + expect(fakeWatcher.close).toHaveBeenCalled(); + } + ); + + test("no watchers are created by a config reload that outlives teardown", async ({ + expect, + }) => { + const watchers = mockWatchers(expect, { closeDelayMs: 200 }); + const fsWatchers = mockFsWatch(expect, { closeDelayMs: 200 }); + + const config = customBuildAndAssetsConfig(); + controller.onConfigUpdate({ type: "configUpdate", config }); + await watchers.at(0); + await fsWatchers.at(0); + + controller.onConfigUpdate({ + type: "configUpdate", + config: { ...config, name: "replacement-worker" }, + }); + await controller.teardown(); + + expect(watchers.created).toHaveLength(1); + expect(fsWatchers.created).toHaveLength(1); + expect(watchers.created[0]?.close).toHaveBeenCalled(); + expect(fsWatchers.created[0]?.close).toHaveBeenCalled(); + }); + } +); diff --git a/packages/wrangler/src/api/startDevWorker/BundlerController.ts b/packages/wrangler/src/api/startDevWorker/BundlerController.ts index 693d25ef6e..482403a897 100644 --- a/packages/wrangler/src/api/startDevWorker/BundlerController.ts +++ b/packages/wrangler/src/api/startDevWorker/BundlerController.ts @@ -25,10 +25,12 @@ import { debounce } from "../../utils/debounce"; import { isAbortError } from "../../utils/isAbortError"; import { Controller } from "./BaseController"; import { castErrorCause } from "./events"; +import { watchAssetsDirectory } from "./watch-assets-directory"; import type { BundleResult } from "../../deployment-bundle/bundle"; import type { EsbuildBundle } from "../../dev/use-esbuild"; import type { ConfigUpdateEvent } from "./events"; import type { StartDevWorkerOptions } from "./types"; +import type { AssetsWatcher } from "./watch-assets-directory"; import type { EphemeralDirectory, Entry } from "@cloudflare/workers-utils"; type PendingCustomBuild = { @@ -532,7 +534,7 @@ export class BundlerController extends Controller { ); } - #assetsWatcher?: ReturnType; + #assetsWatcher?: AssetsWatcher; #debouncedRefreshBundle?: ReturnType; async #ensureWatchingAssets(config: StartDevWorkerOptions) { @@ -566,18 +568,13 @@ export class BundlerController extends Controller { if (config.dev.watch !== false && config.assets?.directory) { const assetsDir = path.resolve(config.assets.directory); - const watcher = watch(assetsDir, { - persistent: true, - ignoreInitial: true, - // Chokidar does not walk an ignored directory, so writes under - // `.wrangler` and `dev.persist` do not reload. Drop a write directory - // only when it is the assets directory, or the watcher would ignore - // the tree it was created to watch. - ignored: getDevSessionWriteDirs(config).filter( - (dir) => path.resolve(dir) !== assetsDir - ), - }) - .on("all", async (eventName, filePath) => { + // Drop a write directory only when it is the assets directory, or + // the watcher would ignore the tree it was created to watch. + const ignoredDirs = getDevSessionWriteDirs(config).filter( + (dir) => path.resolve(dir) !== assetsDir + ); + const watcher = watchAssetsDirectory(assetsDir, ignoredDirs) + .on("all", (eventName, filePath) => { const message = getAssetChangeMessage(eventName, filePath); logger.debug(`🌀 ${message}...`); debouncedRefreshBundle(); diff --git a/packages/wrangler/src/api/startDevWorker/watch-assets-directory.ts b/packages/wrangler/src/api/startDevWorker/watch-assets-directory.ts new file mode 100644 index 0000000000..f08a8831ec --- /dev/null +++ b/packages/wrangler/src/api/startDevWorker/watch-assets-directory.ts @@ -0,0 +1,130 @@ +import * as fs from "node:fs"; +import path from "node:path"; +import { watch } from "chokidar"; +import type { EventName } from "chokidar/handler.js"; + +/** + * A minimal watcher surface shared by the chokidar and recursive `fs.watch` + * backends so `#ensureWatchingAssets` can treat them the same. + */ +export type AssetsWatcher = { + close(): void | Promise; + on( + event: "all", + listener: (eventName: EventName, filePath: string) => void | Promise + ): AssetsWatcher; + on(event: "error", listener: (error: Error) => void): AssetsWatcher; +}; + +/** + * Watch an assets directory for changes that should trigger a local-dev reload. + * + * On macOS and Windows this uses a single recursive `fs.watch`, which is one + * FSEvents / ReadDirectoryChangesW handle for the whole tree. Chokidar 4 + * otherwise opens one `fs.watch` (kqueue) per file on those platforms, and + * past macOS `OPEN_MAX` (10240) child-process spawns fail with `EBADF` + * without ever raising `EMFILE`. + * + * Linux keeps chokidar: its inotify backend watches directories rather than + * each file, so a large assets tree does not exhaust descriptors the same way. + * + * @param assetsDir Absolute path of the assets directory to watch. + * @param ignoredDirs Directories whose writes must not reload (for example + * `.wrangler` and the persist directory). Callers must already omit the + * assets directory itself when it would otherwise appear in this list. + * @returns A watcher with chokidar-like `on("all" | "error")` and `close()`. + */ +export function watchAssetsDirectory( + assetsDir: string, + ignoredDirs: string[] +): AssetsWatcher { + if (process.platform === "darwin" || process.platform === "win32") { + return watchAssetsDirectoryRecursive(assetsDir, ignoredDirs); + } + return watchAssetsDirectoryChokidar(assetsDir, ignoredDirs); +} + +function watchAssetsDirectoryChokidar( + assetsDir: string, + ignoredDirs: string[] +): AssetsWatcher { + return watch(assetsDir, { + persistent: true, + ignoreInitial: true, + // Chokidar does not walk an ignored directory, so writes under + // `.wrangler` and `dev.persist` do not reload. + ignored: ignoredDirs, + }); +} + +function watchAssetsDirectoryRecursive( + assetsDir: string, + ignoredDirs: string[] +): AssetsWatcher { + const nativeWatcher = fs.watch(assetsDir, { + persistent: true, + recursive: true, + encoding: "utf8", + }); + + const allListeners: Array< + (eventName: EventName, filePath: string) => void | Promise + > = []; + const errorListeners: Array<(error: Error) => void> = []; + + nativeWatcher.on("change", (_eventType, filename) => { + const filePath = + filename == null || filename === "" + ? assetsDir + : path.resolve(assetsDir, filename.toString()); + if (ignoredDirs.some((dir) => isPathInsideDir(filePath, dir))) { + return; + } + for (const listener of allListeners) { + void listener("change", filePath); + } + }); + + nativeWatcher.on("error", (error: Error) => { + for (const listener of errorListeners) { + listener(error); + } + }); + + const watcher: AssetsWatcher = { + close() { + // Real `fs.watch` closes synchronously (`void`). Tests delay close + // with a Promise, which `#ensureWatchingAssets` / `teardown` await. + return nativeWatcher.close() as unknown as void | Promise; + }, + on(event, listener) { + if (event === "all") { + allListeners.push( + listener as ( + eventName: EventName, + filePath: string + ) => void | Promise + ); + } else { + errorListeners.push(listener as (error: Error) => void); + } + return watcher; + }, + }; + return watcher; +} + +/** + * Whether `target` is `dir` or a path inside it. + * + * A prefix check that requires a path separator after `dir` so that + * `.wrangler-backup` is not treated as inside `.wrangler`. + */ +function isPathInsideDir(target: string, dir: string): boolean { + const resolvedDir = path.resolve(dir); + const resolvedTarget = path.resolve(target); + const prefix = resolvedDir.endsWith(path.sep) + ? resolvedDir + : resolvedDir + path.sep; + return resolvedTarget === resolvedDir || resolvedTarget.startsWith(prefix); +}