ensure that container builds don't disrupt dev hotkey handling (#9833)

Author: dario-piotrowicz

.changeset/fine-seals-mate.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/containers-shared": minor
+---
+
+extend `prepareContainerImagesForDev` to allow aborting a container's build process

.changeset/sweet-turtles-stay.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: ensure that container builds don't disrupt dev hotkey handling
+
+currently container builds run during local development (via `wrangler dev` or `startWorker`) prevent the standard hotkeys not to be recognized (most noticeably `ctrl+c`, preventing developers from existing the process), the changes here ensure that hotkeys are instead correctly handled as expected

fixtures/container-app/package.json

@@ -6,7 +6,9 @@
 		"container:build": "wrangler containers build ./ -t container-fixture",
 		"deploy": "wrangler deploy",
 		"dev": "wrangler dev",
-		"start": "wrangler dev"
+		"dev:registry": "wrangler dev -c ./wrangler.registry.jsonc",
+		"start": "wrangler dev",
+		"start:registry": "wrangler dev -c ./wrangler.registry.jsonc"
 	},
 	"devDependencies": {
 		"@cloudflare/workers-tsconfig": "workspace:*",

fixtures/interactive-dev-tests/container-app/DockerfileWithLongSleep

@@ -0,0 +1,6 @@
+FROM node:22-alpine
+
+WORKDIR /usr/src/app
+
+RUN echo 'This (no-op) build takes forever...'
+RUN sleep 50000

fixtures/interactive-dev-tests/container-app/src/tsconfig.json

@@ -0,0 +1,12 @@
+{
+	"compilerOptions": {
+		"target": "ES2020",
+		"module": "ESNext",
+		"lib": ["ES2020"],
+		"types": ["@cloudflare/workers-types"],
+		"moduleResolution": "node",
+		"noEmit": true,
+		"skipLibCheck": true
+	},
+	"include": ["**/*.ts"]
+}

fixtures/interactive-dev-tests/container-app/src/worker-configuration.d.ts

@@ -0,0 +1,6 @@
+declare namespace Cloudflare {
+	interface Env {
+		CONTAINER: DurableObjectNamespace<import("./index").FixtureTestContainer>;
+	}
+}
+interface Env extends Cloudflare.Env {}

fixtures/interactive-dev-tests/tests/index.test.ts

@@ -5,6 +5,7 @@ import { tmpdir } from "node:os";
 import path from "node:path";
 import rl from "node:readline";
 import stream from "node:stream";
+import { setTimeout } from "node:timers/promises";
 import stripAnsi from "strip-ansi";
 import { fetch } from "undici";
 import {
@@ -435,6 +436,131 @@ baseDescribe.skipIf(process.platform !== "linux" && process.env.CI === "true")(
 	}
 );
 
+baseDescribe.skipIf(process.platform !== "linux" && process.env.CI === "true")(
+	"container dev where image build takes a long time",
+	{ retry: 0, timeout: 90000 },
+	() => {
+		let tmpDir: string;
+		beforeAll(async () => {
+			tmpDir = fs.mkdtempSync(path.join(tmpdir(), "wrangler-container-sleep-"));
+			fs.cpSync(
+				path.resolve(__dirname, "../", "container-app"),
+				path.join(tmpDir),
+				{
+					recursive: true,
+				}
+			);
+			const tmpDockerFilePath = path.join(tmpDir, "Dockerfile");
+			fs.rmSync(tmpDockerFilePath);
+			fs.renameSync(
+				path.join(tmpDir, "DockerfileWithLongSleep"),
+				tmpDockerFilePath
+			);
+
+			const ids = getContainerIds();
+			if (ids.length > 0) {
+				execSync("docker rm -f " + ids.join(" "), {
+					encoding: "utf8",
+				});
+			}
+		});
+
+		afterEach(async () => {
+			const ids = getContainerIds();
+			if (ids.length > 0) {
+				execSync("docker rm -f " + ids.join(" "), {
+					encoding: "utf8",
+				});
+			}
+		});
+
+		afterAll(async () => {
+			try {
+				fs.rmSync(tmpDir, { recursive: true, force: true });
+			} catch (e) {
+				// It seems that Windows doesn't let us delete this, with errors like:
+				//
+				// Error: EBUSY: resource busy or locked, rmdir 'C:\Users\RUNNER~1\AppData\Local\Temp\wrangler-modules-pKJ7OQ'
+				// ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
+				// Serialized Error: {
+				// 	"code": "EBUSY",
+				// 	"errno": -4082,
+				// 	"path": "C:\Users\RUNNER~1\AppData\Local\Temp\wrangler-modules-pKJ7OQ",
+				// 	"syscall": "rmdir",
+				// }
+				console.error(e);
+			}
+		});
+
+		it("should allow quitting while the image is building", async () => {
+			const wrangler = await startWranglerDev([
+				"dev",
+				"-c",
+				path.join(tmpDir, "wrangler.jsonc"),
+			]);
+
+			const waitForOptions = {
+				timeout: 10_000,
+				interval: WAITFOR_OPTIONS.interval,
+			};
+
+			// wait for long sleep instruction to start
+			await vi.waitFor(async () => {
+				expect(wrangler.stdout).toContain("RUN sleep 50000");
+			}, waitForOptions);
+
+			wrangler.pty.write("q");
+
+			await vi.waitFor(async () => {
+				expect(wrangler.stdout).toMatch(/CANCELED \[.*?\] RUN sleep 50000/);
+			}, waitForOptions);
+		});
+
+		it("should rebuilding while the image is building", async () => {
+			const wrangler = await startWranglerDev([
+				"dev",
+				"-c",
+				path.join(tmpDir, "wrangler.jsonc"),
+			]);
+
+			const waitForOptions = {
+				timeout: 15_000,
+				interval: 1_500,
+			};
+
+			// wait for long sleep instruction to start
+			await vi.waitFor(async () => {
+				expect(wrangler.stdout).toContain("RUN sleep 50000");
+			}, waitForOptions);
+
+			let logOccurrencesBefore =
+				wrangler.stdout.match(/This \(no-op\) build takes forever.../g)
+					?.length ?? 0;
+
+			wrangler.pty.write("r");
+
+			await vi.waitFor(async () => {
+				const logOccurrences =
+					wrangler.stdout.match(/This \(no-op\) build takes forever.../g)
+						?.length ?? 0;
+				expect(logOccurrences).toBeGreaterThan(1);
+				logOccurrencesBefore = logOccurrences;
+			}, waitForOptions);
+
+			await vi.waitFor(async () => {
+				await setTimeout(700);
+				wrangler.pty.write("r");
+				const logOccurrences =
+					wrangler.stdout.match(/This \(no-op\) build takes forever.../g)
+						?.length ?? 0;
+				expect(logOccurrences).toBeGreaterThan(logOccurrencesBefore);
+			}, waitForOptions);
+
+			wrangler.pty.kill();
+		});
+	}
+);
+
 /** gets any containers that were created by running this fixture */
 const getContainerIds = () => {
 	// note the -a to include stopped containers

packages/containers-shared/src/build.ts

@@ -43,32 +43,51 @@ export function dockerBuild(
 		buildCmd: string[];
 		dockerfile: string;
 	}
-): Promise<void> {
+): { abort: () => void; ready: Promise<void> } {
 	let errorHandled = false;
-	return new Promise((resolve, reject) => {
-		const child = spawn(dockerPath, options.buildCmd, {
-			stdio: ["pipe", "inherit", "inherit"],
-		});
-		if (child.stdin !== null) {
-			child.stdin.write(options.dockerfile);
-			child.stdin.end();
-		}
+	let resolve: () => void;
+	let reject: (err: unknown) => void;
+	const ready = new Promise<void>((res, rej) => {
+		resolve = res;
+		reject = rej;
+	});
 
-		child.on("exit", (code) => {
-			if (code === 0) {
-				resolve();
-			} else if (!errorHandled) {
-				errorHandled = true;
-				reject(new Error(`Build exited with code: ${code}`));
-			}
-		});
-		child.on("error", (err) => {
-			if (!errorHandled) {
-				errorHandled = true;
-				reject(err);
-			}
-		});
+	const child = spawn(dockerPath, options.buildCmd, {
+		stdio: ["pipe", "inherit", "inherit"],
+		// We need to set detached to true so that the child process
+		// will control all of its child processed and we can kill
+		// all of them in case we need to abort the build process
+		detached: true,
+	});
+	if (child.stdin !== null) {
+		child.stdin.write(options.dockerfile);
+		child.stdin.end();
+	}
+
+	child.on("exit", (code) => {
+		if (code === 0) {
+			resolve();
+		} else if (!errorHandled) {
+			errorHandled = true;
+			reject(new Error(`Build exited with code: ${code}`));
+		}
+	});
+	child.on("error", (err) => {
+		if (!errorHandled) {
+			errorHandled = true;
+			reject(err);
+		}
 	});
+	return {
+		abort: () => {
+			child.unref();
+			if (child.pid !== undefined) {
+				// kill run on the negative PID kills the whole group controlled by the child process
+				process.kill(-child.pid);
+			}
+		},
+		ready,
+	};
 }
 
 export async function buildImage(
@@ -88,5 +107,5 @@ export async function buildImage(
 		configPath
 	);
 
-	await dockerBuild(dockerPath, { buildCmd, dockerfile });
+	return dockerBuild(dockerPath, { buildCmd, dockerfile });
 }

packages/containers-shared/src/images.ts

@@ -15,17 +15,28 @@ import {
 export async function pullImage(
 	dockerPath: string,
 	options: ContainerDevOptions
-) {
+): Promise<{ abort: () => void; ready: Promise<void> }> {
 	await dockerLoginManagedRegistry(dockerPath);
-	await runDockerCmd(dockerPath, [
+	const pull = runDockerCmd(dockerPath, [
 		"pull",
 		options.image,
 		// All containers running on our platform need to be built for amd64 architecture, but by default docker pull seems to look for an image matching the host system, so we need to specify this here
 		"--platform",
 		"linux/amd64",
 	]);
-	// re-tag image with the expected dev-formatted image tag for consistency
-	await runDockerCmd(dockerPath, ["tag", options.image, options.imageTag]);
+	const ready = pull.ready.then(async ({ aborted }: { aborted: boolean }) => {
+		if (!aborted) {
+			// re-tag image with the expected dev-formatted image tag for consistency
+			await runDockerCmd(dockerPath, ["tag", options.image, options.imageTag]);
+		}
+	});
+
+	return {
+		abort: () => {
+			pull.abort();
+		},
+		ready,
+	};
 }
 
 /**
@@ -41,8 +52,16 @@ export async function pullImage(
 export async function prepareContainerImagesForDev(
 	dockerPath: string,
 	containerOptions: ContainerDevOptions[],
-	configPath: string | undefined
+	configPath: string | undefined,
+	onContainerImagePreparationStart: (args: {
+		containerOptions: ContainerDevOptions;
+		abort: () => void;
+	}) => void,
+	onContainerImagePreparationEnd: (args: {
+		containerOptions: ContainerDevOptions;
+	}) => void
 ) {
+	let aborted = false;
 	if (process.platform === "win32") {
 		throw new Error(
 			"Local development with containers is currently not supported on Windows. You should use WSL instead. You can also set `enable_containers` to false if you do not need to develop the container part of your application."
@@ -51,17 +70,41 @@ export async function prepareContainerImagesForDev(
 	await verifyDockerInstalled(dockerPath);
 	for (const options of containerOptions) {
 		if (isDockerfile(options.image, configPath)) {
-			await buildImage(dockerPath, options, configPath);
+			const build = await buildImage(dockerPath, options, configPath);
+			onContainerImagePreparationStart({
+				containerOptions: options,
+				abort: () => {
+					aborted = true;
+					build.abort();
+				},
+			});
+			await build.ready;
+			onContainerImagePreparationEnd({
+				containerOptions: options,
+			});
 		} else {
 			if (!isCloudflareRegistryLink(options.image)) {
 				throw new Error(
 					`Image "${options.image}" is a registry link but does not point to the Cloudflare container registry.\n` +
 						`To use an existing image from another repository, see https://developers.cloudflare.com/containers/image-management/#using-existing-images`
 				);
 			}
-			await pullImage(dockerPath, options);
+			const pull = await pullImage(dockerPath, options);
+			onContainerImagePreparationStart({
+				containerOptions: options,
+				abort: () => {
+					aborted = true;
+					pull.abort();
+				},
+			});
+			await pull.ready;
+			onContainerImagePreparationEnd({
+				containerOptions: options,
+			});
+		}
+		if (!aborted) {
+			await checkExposedPorts(dockerPath, options);
 		}
-		await checkExposedPorts(dockerPath, options);
 	}
 }