Skip to content

Commit f92d1fc

Browse files
petebacondarwindhruv7539
petebacondarwin
and
dhruv7539
authored
[wrangler] Serialize custom build watcher events to avoid concurrent runs (#14936)
Co-authored-by: dhruv7539 <dhruvbhanderi7@gmail.com>
1 parent 83a37e3 commit f92d1fc

10 files changed

Lines changed: 761 additions & 167 deletions

File tree

‎.changeset/custom-build-jsx-fragment.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"wrangler": patch
3+
---
4+
5+
Fix `jsx_fragment` being ignored when `wrangler dev` runs a custom build
6+
7+
If your project uses a custom build and sets both `jsx_factory` and `jsx_fragment`, `wrangler dev` used your `jsx_factory` value for JSX fragments as well, so fragments compiled incorrectly. Your `jsx_fragment` value is now used.

‎.changeset/no-work-after-dev-teardown.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"wrangler": patch
3+
---
4+
5+
Stop `wrangler dev` starting new work after you stop it or it reloads
6+
7+
Stopping `wrangler dev`, or having it reload after a configuration change, could still leave it starting work for the state it had just left behind: your custom build command could run once more after dev had stopped, a change to a file in your assets directory could be reported against configuration that had already been replaced, and in some cases the process could stay alive instead of exiting.
8+
9+
That work is now discarded, so stopping or reloading `wrangler dev` leaves nothing running behind it.

‎.changeset/serialize-custom-builds.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"wrangler": patch
3+
---
4+
5+
Stop `wrangler dev` from running custom builds concurrently
6+
7+
When several watched files changed at once — for example during a `git pull` or a "save all" — `wrangler dev` started a custom build for every file that changed, so multiple copies of your build command ran at the same time and fought over the same output files.
8+
9+
A burst of file changes now results in a single build, and a build only starts once the previous one has finished.

‎packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts‎

Lines changed: 190 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { existsSync } from "node:fs";
1+
import { existsSync, readdirSync } from "node:fs";
22
import path from "node:path";
3+
import { setTimeout } from "node:timers/promises";
34
import {
45
normalizeString,
56
runInTempDir,
@@ -20,10 +21,20 @@ function findSourceFile(source: string, name: string): string {
2021
return source.slice(startIndex, endIndex);
2122
}
2223

24+
/**
25+
* The temporary build directories created under the project root, each of which
26+
* also registers a process exit listener to clean itself up.
27+
*/
28+
function wranglerTmpDirs(): string[] {
29+
const tmpRoot = path.resolve(".wrangler/tmp");
30+
return existsSync(tmpRoot) ? readdirSync(tmpRoot) : [];
31+
}
32+
2333
function configDefaults(
2434
config: Partial<
25-
Omit<StartDevWorkerOptions, "build"> & {
35+
Omit<StartDevWorkerOptions, "build" | "dev"> & {
2636
build: Partial<StartDevWorkerOptions["build"]>;
37+
dev: Partial<StartDevWorkerOptions["dev"]>;
2738
}
2839
>
2940
): StartDevWorkerOptions {
@@ -34,8 +45,11 @@ function configDefaults(
3445
entrypoint: path.resolve("src/index.ts"),
3546
projectRoot: path.resolve("src"),
3647
legacy: {},
37-
dev: { persist },
3848
...config,
49+
dev: {
50+
persist,
51+
...config.dev,
52+
},
3953
build: {
4054
additionalModules: [],
4155
processEntrypoint: false,
@@ -319,6 +333,112 @@ describe("BundleController", { retry: 5, timeout: 10_000 }, () => {
319333
);
320334
});
321335

336+
test("a burst of watched file changes does not run custom builds concurrently", async ({
337+
expect,
338+
}) => {
339+
await seed({
340+
// Records an overlap in `overlap.txt` if another custom build process is
341+
// still running when this one starts.
342+
//
343+
// A build that is superseded part way through is killed, leaving
344+
// `build.lock` behind, so the recorded pid is checked for liveness
345+
// rather than treating the presence of the lock as an overlap.
346+
"build.js": dedent /* javascript */ `
347+
const fs = require("node:fs");
348+
349+
if (fs.existsSync("build.lock")) {
350+
const pid = Number(fs.readFileSync("build.lock", "utf8"));
351+
let running = true;
352+
try {
353+
process.kill(pid, 0);
354+
} catch {
355+
running = false;
356+
}
357+
if (running) {
358+
fs.writeFileSync("overlap.txt", String(pid));
359+
process.exit(1);
360+
}
361+
}
362+
363+
fs.writeFileSync("build.lock", String(process.pid));
364+
setTimeout(() => {
365+
fs.cpSync("custom_build_dir/index.ts", "out.ts");
366+
fs.rmSync("build.lock");
367+
}, 300);
368+
`,
369+
"custom_build_dir/index.ts": dedent /* javascript */ `
370+
export default {
371+
fetch() {
372+
return new Response("hello custom build")
373+
}
374+
} satisfies ExportedHandler
375+
`,
376+
});
377+
const config = configDefaults({
378+
entrypoint: path.resolve("out.ts"),
379+
projectRoot: path.resolve("."),
380+
build: {
381+
custom: {
382+
command: "node build.js",
383+
watch: "custom_build_dir",
384+
},
385+
moduleRoot: path.resolve("."),
386+
},
387+
});
388+
389+
const firstBuild = bus.waitFor("bundleComplete");
390+
controller.onConfigUpdate({ type: "configUpdate", config });
391+
await firstBuild;
392+
393+
const bundleStartCount = () =>
394+
bus.events.filter((event) => event.type === "bundleStart").length;
395+
const buildsBeforeChanges = bundleStartCount();
396+
397+
// Simulate the burst of watcher events produced by something like a
398+
// `git pull` touching several files at once.
399+
await seed(
400+
Object.fromEntries(
401+
Array.from({ length: 5 }, (_, i) => [
402+
`custom_build_dir/change-${i}.txt`,
403+
String(i),
404+
])
405+
)
406+
);
407+
408+
// Wait for the builds triggered by the burst to settle, which we treat as
409+
// two consecutive polls seeing the same number of builds. We deliberately
410+
// don't wait for `bundleComplete`: overlapping builds make the last build
411+
// fail, and the assertions below report that far more usefully than the
412+
// test timing out.
413+
let previousCount = -1;
414+
await vi.waitFor(
415+
() => {
416+
const count = bundleStartCount();
417+
const settled =
418+
count > buildsBeforeChanges && count === previousCount;
419+
previousCount = count;
420+
if (!settled) {
421+
throw new Error(
422+
`Custom builds have not settled yet (${count - buildsBeforeChanges} started since the file changes)`
423+
);
424+
}
425+
},
426+
{ timeout: 8_000, interval: 500 }
427+
);
428+
429+
expect(existsSync("overlap.txt")).toBe(false);
430+
expect(
431+
bus.events.filter(
432+
(event) =>
433+
event.type === "error" &&
434+
event.source === "BundlerController" &&
435+
event.reason === "Custom build failed"
436+
)
437+
).toEqual([]);
438+
// The burst is debounced, so it must not produce a build per changed file
439+
expect(bundleStartCount() - buildsBeforeChanges).toBeLessThan(5);
440+
});
441+
322442
test("teardown aborts an in-flight watched custom build", async ({
323443
expect,
324444
}) => {
@@ -382,6 +502,73 @@ describe("BundleController", { retry: 5, timeout: 10_000 }, () => {
382502
)
383503
).toBe(false);
384504
});
505+
506+
// `DevEnv` tears its controllers down concurrently, and `ConfigController` can
507+
// dispatch a config-file change that was delivered while its own watcher was
508+
// closing, so a config update can reach the bundler after it has torn down.
509+
// Acting on one leaks watchers, an esbuild watch build and a temp directory
510+
// that nothing will ever clean up, and can keep the process alive.
511+
const postTeardownCases = [
512+
{
513+
name: "watched custom build",
514+
build: {
515+
custom: { command: "node build.js", watch: "custom_build_dir" },
516+
},
517+
},
518+
{
519+
name: "unwatched custom build",
520+
dev: { watch: false },
521+
build: {
522+
custom: { command: "node build.js", watch: "custom_build_dir" },
523+
},
524+
},
525+
{
526+
name: "esbuild bundler",
527+
entrypoint: path.resolve("custom_build_dir/index.ts"),
528+
},
529+
];
530+
531+
for (const testCase of postTeardownCases) {
532+
test(`a config update after teardown is ignored (${testCase.name})`, async ({
533+
expect,
534+
}) => {
535+
await seed({
536+
"build.js": dedent /* javascript */ `
537+
const fs = require("node:fs");
538+
fs.writeFileSync("built.txt", "yes");
539+
fs.cpSync("custom_build_dir/index.ts", "out.ts");
540+
`,
541+
"custom_build_dir/index.ts": dedent /* javascript */ `
542+
export default {
543+
fetch() {
544+
return new Response("initial")
545+
}
546+
}
547+
`,
548+
});
549+
const config = configDefaults({
550+
entrypoint: path.resolve("out.ts"),
551+
projectRoot: path.resolve("."),
552+
...testCase,
553+
build: { moduleRoot: path.resolve("."), ...testCase.build },
554+
});
555+
556+
await controller.teardown();
557+
controller.onConfigUpdate({ type: "configUpdate", config });
558+
await setTimeout(500);
559+
560+
// Nothing may run the user's build command or report to the torn-down bus
561+
expect(existsSync("built.txt")).toBe(false);
562+
expect(
563+
bus.events.filter(
564+
(event) =>
565+
event.type === "bundleStart" || event.type === "bundleComplete"
566+
)
567+
).toEqual([]);
568+
// ...nor create resources that nothing is left to clean up
569+
expect(wranglerTmpDirs()).toEqual([]);
570+
});
571+
}
385572
});
386573

387574
test("module aliasing", async ({ expect }) => {

0 commit comments

Comments
 (0)