From 44a8340702adba520be20afd7dbf4819103fbea8 Mon Sep 17 00:00:00 2001
From: Brendan Coll <bcoll@cloudflare.com>
Date: Wed, 8 Nov 2023 11:23:25 +0000
Subject: [PATCH] fix: cleanup temporary directory after shutting down
 `workerd`

---
 .changeset/tricky-masks-accept.md     | 12 +++++++
 packages/miniflare/src/index.ts       | 24 ++++++++-----
 packages/miniflare/test/index.spec.ts | 52 ++++++++++++++++++++++++++-
 3 files changed, 78 insertions(+), 10 deletions(-)
 create mode 100644 .changeset/tricky-masks-accept.md

diff --git a/.changeset/tricky-masks-accept.md b/.changeset/tricky-masks-accept.md
new file mode 100644
index 000000000000..fe7d3b413208
--- /dev/null
+++ b/.changeset/tricky-masks-accept.md
@@ -0,0 +1,12 @@
+---
+"miniflare": patch
+---
+
+fix: cleanup temporary directory after shutting down `workerd`
+
+Previously on exit, Miniflare would attempt to remove its temporary directory
+before shutting down `workerd`. This could lead to `EBUSY` errors on Windows.
+This change ensures we shutdown `workerd` before removing the directory.
+Since we can only clean up on a best effort basis when exiting, it also catches
+any errors thrown when removing the directory, in case the runtime doesn't
+shutdown fast enough.
diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts
index d0712b225f4d..602c750b8dbf 100644
--- a/packages/miniflare/src/index.ts
+++ b/packages/miniflare/src/index.ts
@@ -563,7 +563,7 @@ export class Miniflare {
 	#log: Log;
 
 	readonly #runtime?: Runtime;
-	readonly #removeRuntimeExitHook?: () => void;
+	readonly #removeExitHook?: () => void;
 	#runtimeEntryURL?: URL;
 	#socketPorts?: SocketPorts;
 	#runtimeDispatcher?: Dispatcher;
@@ -573,7 +573,6 @@ export class Miniflare {
 	// Object storage. Note this may not exist, it's up to the consumers to
 	// create this if needed. Deleted on `dispose()`.
 	readonly #tmpPath: string;
-	readonly #removeTmpPathExitHook: () => void;
 
 	// Mutual exclusion lock for runtime operations (i.e. initialisation and
 	// updating config). This essentially puts initialisation and future updates
@@ -637,13 +636,21 @@ export class Miniflare {
 			os.tmpdir(),
 			`miniflare-${crypto.randomBytes(16).toString("hex")}`
 		);
-		this.#removeTmpPathExitHook = exitHook(() => {
-			fs.rmSync(this.#tmpPath, { force: true, recursive: true });
-		});
 
 		// Setup runtime
 		this.#runtime = new Runtime();
-		this.#removeRuntimeExitHook = exitHook(() => void this.#runtime?.dispose());
+		this.#removeExitHook = exitHook(() => {
+			void this.#runtime?.dispose();
+			try {
+				fs.rmSync(this.#tmpPath, { force: true, recursive: true });
+			} catch (e) {
+				// `rmSync` may fail on Windows with `EBUSY` if `workerd` is still
+				// running. `Runtime#dispose()` should kill the runtime immediately.
+				// `exitHook`s must be synchronous, so we can only clean up on a best
+				// effort basis.
+				this.#log.debug(`Unable to remove temporary directory: ${String(e)}`);
+			}
+		});
 
 		this.#disposeController = new AbortController();
 		this.#runtimeMutex = new Mutex();
@@ -1506,9 +1513,8 @@ export class Miniflare {
 		try {
 			await this.#waitForReady(/* disposing */ true);
 		} finally {
-			// Remove exit hooks, we're cleaning up what they would've cleaned up now
-			this.#removeTmpPathExitHook();
-			this.#removeRuntimeExitHook?.();
+			// Remove exit hook, we're cleaning up what they would've cleaned up now
+			this.#removeExitHook?.();
 
 			// Cleanup as much as possible even if `#init()` threw
 			await this.#proxyClient?.dispose();
diff --git a/packages/miniflare/test/index.spec.ts b/packages/miniflare/test/index.spec.ts
index f01f23da165b..3635303ce18b 100644
--- a/packages/miniflare/test/index.spec.ts
+++ b/packages/miniflare/test/index.spec.ts
@@ -1,12 +1,14 @@
 // noinspection TypeScriptValidateJSTypes
 
 import assert from "assert";
+import childProcess from "child_process";
+import { once } from "events";
 import fs from "fs/promises";
 import http from "http";
 import { AddressInfo } from "net";
 import path from "path";
 import { Writable } from "stream";
-import { json } from "stream/consumers";
+import { json, text } from "stream/consumers";
 import util from "util";
 import {
 	D1Database,
@@ -1088,6 +1090,54 @@ unixSerialTest(
 	}
 );
 
+test("Miniflare: exits cleanly", async (t) => {
+	const miniflarePath = require.resolve("miniflare");
+	const result = childProcess.spawn(
+		process.execPath,
+		[
+			"--no-warnings", // Hide experimental warnings
+			"-e",
+			`
+      const { Miniflare, Log, LogLevel } = require(${JSON.stringify(
+				miniflarePath
+			)});
+      const mf = new Miniflare({
+        verbose: true,
+        modules: true,
+        script: \`export default {
+          fetch() {
+            return new Response("body");
+          }
+        }\`
+      });
+      (async () => {
+        const res = await mf.dispatchFetch("http://placeholder/");
+        const text = await res.text();
+        process.send(text);
+        process.disconnect();
+      })();
+      `,
+		],
+		{
+			stdio: [/* in */ "ignore", /* out */ "pipe", /* error */ "pipe", "ipc"],
+		}
+	);
+
+	// Make sure workerd started
+	const [message] = await once(result, "message");
+	t.is(message, "body");
+
+	// Check exit doesn't output anything
+	const closePromise = once(result, "close");
+	result.kill("SIGINT");
+	assert(result.stdout !== null && result.stderr !== null);
+	const stdout = await text(result.stdout);
+	const stderr = await text(result.stderr);
+	await closePromise;
+	t.is(stdout, "");
+	t.is(stderr, "");
+});
+
 test("Miniflare: allows the use of unsafe eval bindings", async (t) => {
 	const log = new TestLog(t);