[miniflare] Reconcile dev registry entries instead of deleting and recreating them

.changeset/lucky-pandas-wander.md

@@ -0,0 +1,9 @@
+---
+"miniflare": patch
+---
+
+Stop deleting and recreating every dev registry entry on each config update
+
+Applying options rewrote this instance's dev registry entries by removing them and putting them straight back. Other dev sessions find Workers by watching that directory, so each update briefly looked to them like every Worker in the session had gone away — and a session that had already resolved one of those Workers could be left acting on that, up to and including tearing down a binding to a Worker that never actually stopped running.
+
+Entries are now reconciled instead: Workers that are still present are updated in place, and only the ones that have genuinely gone are removed. Switching to a different registry path still clears the entries from the directory being left behind.

packages/miniflare/src/index.ts

@@ -2361,6 +2361,10 @@ export class Miniflare {
 				try {
 					await this.#assembleAndUpdateConfig(true);
 				} catch (error) {
+					// Same reasoning as the failed-update path in `#setOptions()`: there
+					// is no longer a runtime behind what we advertised, so withdraw it
+					// instead of leaving peers pointed at a dead debug port.
+					this.#devRegistry.unregisterWorkers();
 					const cause =
 						error instanceof Error ? error : new Error(String(error));
 					this.#runtimeRestartError = new MiniflareCoreError(
@@ -2684,6 +2688,9 @@ export class Miniflare {
 			this.#log.warn(
 				"Debug port not available — skipping dev registry registration"
 			);
+			// Nothing we advertised before is reachable, so withdraw it rather than
+			// leaving peers pointed at an address we can no longer serve.
+			this.#devRegistry.unregisterWorkers();
 			return;
 		}
 		const debugPortAddress = `127.0.0.1:${debugPort}`;
@@ -2834,7 +2841,17 @@ export class Miniflare {
 			}
 		);
 		// Send to runtime and wait for updates to process
-		await this.#assembleAndUpdateConfig();
+		try {
+			await this.#assembleAndUpdateConfig();
+		} catch (error) {
+			// The runtime this instance was reachable on has already been stopped by
+			// this point, and we never got as far as advertising its replacement. Our
+			// entries would otherwise sit in the registry pointing at a dead debug
+			// port, kept fresh by their heartbeats so the stale-entry sweep never
+			// reclaims them.
+			this.#devRegistry.unregisterWorkers();
+			throw error;
+		}
 	}
 
 	setOptions(opts: MiniflareOptions): Promise<void> {

packages/miniflare/src/shared/dev-registry.ts

@@ -89,7 +89,10 @@ export class DevRegistry {
 		});
 	}
 
-	private unregisterWorkers() {
+	/**
+	 * Withdraw every entry this instance has registered.
+	 */
+	public unregisterWorkers() {
 		for (const worker of this.registeredWorkers) {
 			this.unregister(worker);
 		}
@@ -137,11 +140,17 @@ export class DevRegistry {
 		registryPath: string | undefined,
 		onUpdate?: (registry: WorkerRegistry) => void
 	): Promise<void> {
-		// Unregister all registered workers
-		this.unregisterWorkers();
 		this.onUpdate = onUpdate;
 
 		if (registryPath !== this.registryPath) {
+			// Our entries live in the directory we are leaving, so they have to be
+			// removed from there before we switch. When the path is unchanged we
+			// deliberately keep them: `register()` reconciles the set instead, so a
+			// config update no longer deletes and recreates every entry. Other dev
+			// sessions watch this directory, and a deletion is visible to them even
+			// if we put the file straight back.
+			this.unregisterWorkers();
+
 			// Close the existing watcher if it exists.
 			// It will watch the new path if there is any dependent services in a later step
 			await this.watcher?.close();
@@ -159,6 +168,20 @@ export class DevRegistry {
 		// Make sure the registry path exists
 		mkdirSync(this.registryPath, { recursive: true });
 
+		// Drop the entries for Workers this instance no longer has. Workers that
+		// remain are overwritten in place below instead of being deleted and
+		// recreated, so a peer never observes one of its service binding or
+		// `tail_consumers` targets disappearing during a routine config update.
+		for (const name of [...this.registeredWorkers]) {
+			// `hasOwn` rather than `in`: a Worker may legitimately be named after an
+			// inherited property such as `constructor`, and `in` would report it as
+			// still present and leave its entry behind.
+			if (!Object.hasOwn(workers, name)) {
+				this.unregister(name);
+				this.registeredWorkers.delete(name);
+			}
+		}
+
 		for (const [name, definition] of Object.entries(workers)) {
 			const definitionPath = path.join(this.registryPath, name);
 			const existingHeartbeat = this.heartbeats.get(name);

packages/miniflare/test/dev-registry.spec.ts

@@ -1,3 +1,5 @@
+import path from "node:path";
+import { watch } from "chokidar";
 import { getWorkerRegistry, Miniflare } from "miniflare";
 import { describe, onTestFinished, test, vi } from "vitest";
 import { useDispose, useTmp } from "./test-shared";
@@ -1788,3 +1790,192 @@ describe.sequential("DevRegistry", () => {
 		);
 	});
 });
+
+describe("registry churn across config updates", () => {
+	const script = (body: string) =>
+		`export default { async fetch() { return new Response("${body}"); } }`;
+
+	test("does not withdraw a Worker's entry during an unrelated config update", async ({
+		expect,
+	}) => {
+		const unsafeDevRegistryPath = await useTmp();
+		const mf = new Miniflare({
+			name: "stable-worker",
+			unsafeDevRegistryPath,
+			modules: true,
+			script: script("before"),
+		});
+		useDispose(mf);
+		await mf.ready;
+
+		await vi.waitFor(
+			() => {
+				expect(
+					getWorkerRegistry(unsafeDevRegistryPath)["stable-worker"]
+				).toBeDefined();
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+
+		// Other dev sessions learn about us by watching this directory, so watch it
+		// the same way and record what a peer would actually see.
+		const events: string[] = [];
+		const watcher = watch(unsafeDevRegistryPath, {
+			ignoreInitial: true,
+		});
+		onTestFinished(() => watcher.close());
+		await new Promise((resolve) => watcher.once("ready", resolve));
+		watcher.on("unlink", (file) =>
+			events.push(`unlink:${path.basename(file)}`)
+		);
+		watcher.on("add", (file) => events.push(`add:${path.basename(file)}`));
+		watcher.on("change", (file) =>
+			events.push(`change:${path.basename(file)}`)
+		);
+
+		await mf.setOptions({
+			name: "stable-worker",
+			unsafeDevRegistryPath,
+			modules: true,
+			script: script("after"),
+		});
+
+		// Wait until the update has definitely reached the directory, so that an
+		// empty event list can't be mistaken for a passing assertion.
+		await vi.waitFor(
+			() => {
+				expect(events.some((event) => event.endsWith(":stable-worker"))).toBe(
+					true
+				);
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+
+		expect(events).not.toContain("unlink:stable-worker");
+		expect(
+			getWorkerRegistry(unsafeDevRegistryPath)["stable-worker"]
+		).toBeDefined();
+	});
+
+	test("withdraws its entries when a config update fails to start a runtime", async ({
+		expect,
+	}) => {
+		const unsafeDevRegistryPath = await useTmp();
+		const mf = new Miniflare({
+			name: "doomed-worker",
+			unsafeDevRegistryPath,
+			modules: true,
+			script: script("before"),
+		});
+		useDispose(mf);
+		await mf.ready;
+
+		await vi.waitFor(
+			() => {
+				expect(
+					getWorkerRegistry(unsafeDevRegistryPath)["doomed-worker"]
+				).toBeDefined();
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+
+		// A flag `workerd` rejects. The previous runtime is stopped before the
+		// replacement is started, so this update leaves no runtime behind it.
+		await expect(
+			mf.setOptions({
+				name: "doomed-worker",
+				unsafeDevRegistryPath,
+				modules: true,
+				script: script("after"),
+				compatibilityFlags: ["definitely_not_a_real_compatibility_flag"],
+			})
+		).rejects.toThrow(/runtime failed to start/i);
+
+		// Nothing serves the advertised debug port now, and the entry's heartbeat
+		// would keep it looking fresh, so it has to be withdrawn rather than left
+		// for the stale-entry sweep.
+		expect(
+			getWorkerRegistry(unsafeDevRegistryPath)["doomed-worker"]
+		).toBeUndefined();
+	});
+
+	test("withdraws a removed Worker named after an inherited property", async ({
+		expect,
+	}) => {
+		const unsafeDevRegistryPath = await useTmp();
+		const mf = new Miniflare({
+			unsafeDevRegistryPath,
+			workers: [
+				{ name: "kept-worker", modules: true, script: script("kept") },
+				// A name that exists on `Object.prototype`, so a membership test that
+				// walks the prototype chain would report it as still configured.
+				{ name: "constructor", modules: true, script: script("dropped") },
+			],
+		});
+		useDispose(mf);
+		await mf.ready;
+
+		// `hasOwn` throughout: a plain object resolves `registry["constructor"]`
+		// through its prototype, so a plain lookup would assert nothing here.
+		await vi.waitFor(
+			() => {
+				expect(
+					Object.hasOwn(getWorkerRegistry(unsafeDevRegistryPath), "constructor")
+				).toBe(true);
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+
+		await mf.setOptions({
+			unsafeDevRegistryPath,
+			workers: [{ name: "kept-worker", modules: true, script: script("kept") }],
+		});
+
+		await vi.waitFor(
+			() => {
+				const registry = getWorkerRegistry(unsafeDevRegistryPath);
+				expect(registry["kept-worker"]).toBeDefined();
+				expect(Object.hasOwn(registry, "constructor")).toBe(false);
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+	});
+
+	test("withdraws the entry for a Worker removed from the config", async ({
+		expect,
+	}) => {
+		const unsafeDevRegistryPath = await useTmp();
+		const mf = new Miniflare({
+			unsafeDevRegistryPath,
+			workers: [
+				{ name: "kept-worker", modules: true, script: script("kept") },
+				{ name: "dropped-worker", modules: true, script: script("dropped") },
+			],
+		});
+		useDispose(mf);
+		await mf.ready;
+
+		await vi.waitFor(
+			() => {
+				const registry = getWorkerRegistry(unsafeDevRegistryPath);
+				expect(registry["kept-worker"]).toBeDefined();
+				expect(registry["dropped-worker"]).toBeDefined();
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+
+		await mf.setOptions({
+			unsafeDevRegistryPath,
+			workers: [{ name: "kept-worker", modules: true, script: script("kept") }],
+		});
+
+		await vi.waitFor(
+			() => {
+				const registry = getWorkerRegistry(unsafeDevRegistryPath);
+				expect(registry["kept-worker"]).toBeDefined();
+				expect(registry["dropped-worker"]).toBeUndefined();
+			},
+			{ timeout: 10_000, interval: 100 }
+		);
+	});
+});