From 05f7e30e6f3c4d4041787bf6b9eff8d72b2adf91 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 5 Aug 2026 15:09:35 +0100 Subject: [PATCH 1/3] [miniflare] Reconcile dev registry entries instead of deleting and recreating them Applying options called `unregisterWorkers()` unconditionally, unlinking every entry this instance had registered, before `register()` wrote them all back. Peers discover Workers by watching that directory, so every config update published a window in which this session appeared to have no Workers at all. `updateRegistryPath()` now only clears entries when the registry path actually changes, since those entries live in the directory being left behind, and `register()` reconciles the set: retained Workers are overwritten in place and only genuinely removed ones are unlinked. --- .changeset/lucky-pandas-wander.md | 9 ++ packages/miniflare/src/index.ts | 3 + packages/miniflare/src/shared/dev-registry.ts | 21 +++- packages/miniflare/test/dev-registry.spec.ts | 107 ++++++++++++++++++ 4 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 .changeset/lucky-pandas-wander.md diff --git a/.changeset/lucky-pandas-wander.md b/.changeset/lucky-pandas-wander.md new file mode 100644 index 0000000000..0cdf692ebe --- /dev/null +++ b/.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. diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts index 87314a6b27..eab29cc888 100644 --- a/packages/miniflare/src/index.ts +++ b/packages/miniflare/src/index.ts @@ -2684,6 +2684,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.register({}); return; } const debugPortAddress = `127.0.0.1:${debugPort}`; diff --git a/packages/miniflare/src/shared/dev-registry.ts b/packages/miniflare/src/shared/dev-registry.ts index c74995ae68..4fdb6c0cc8 100644 --- a/packages/miniflare/src/shared/dev-registry.ts +++ b/packages/miniflare/src/shared/dev-registry.ts @@ -137,11 +137,17 @@ export class DevRegistry { registryPath: string | undefined, onUpdate?: (registry: WorkerRegistry) => void ): Promise { - // 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 +165,17 @@ 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]) { + if (!(name in workers)) { + 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); diff --git a/packages/miniflare/test/dev-registry.spec.ts b/packages/miniflare/test/dev-registry.spec.ts index 5f7714e07f..e169908fb5 100644 --- a/packages/miniflare/test/dev-registry.spec.ts +++ b/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,108 @@ 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 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 } + ); + }); +}); From c5f46c8a99fb7522e0ea29668cd8eb14a33a2fe2 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 5 Aug 2026 15:56:32 +0100 Subject: [PATCH 2/3] Address review: use Object.hasOwn and expose unregisterWorkers `name in workers` walks the prototype chain, so a Worker legitimately named after an inherited property (`constructor`, `toString`) was reported as still configured and its entry was never withdrawn. Use `Object.hasOwn`, with a test covering it. Also call `devRegistry.unregisterWorkers()` directly on the path where the debug port is unavailable, rather than expressing "withdraw everything" as `register({})`. --- packages/miniflare/src/index.ts | 2 +- packages/miniflare/src/shared/dev-registry.ts | 10 ++++- packages/miniflare/test/dev-registry.spec.ts | 42 +++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts index eab29cc888..2356b80107 100644 --- a/packages/miniflare/src/index.ts +++ b/packages/miniflare/src/index.ts @@ -2686,7 +2686,7 @@ export class Miniflare { ); // Nothing we advertised before is reachable, so withdraw it rather than // leaving peers pointed at an address we can no longer serve. - this.#devRegistry.register({}); + this.#devRegistry.unregisterWorkers(); return; } const debugPortAddress = `127.0.0.1:${debugPort}`; diff --git a/packages/miniflare/src/shared/dev-registry.ts b/packages/miniflare/src/shared/dev-registry.ts index 4fdb6c0cc8..547984a8f3 100644 --- a/packages/miniflare/src/shared/dev-registry.ts +++ b/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); } @@ -170,7 +173,10 @@ export class DevRegistry { // 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]) { - if (!(name in workers)) { + // `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); } diff --git a/packages/miniflare/test/dev-registry.spec.ts b/packages/miniflare/test/dev-registry.spec.ts index e169908fb5..d7cdf46b90 100644 --- a/packages/miniflare/test/dev-registry.spec.ts +++ b/packages/miniflare/test/dev-registry.spec.ts @@ -1857,6 +1857,48 @@ describe("registry churn across config updates", () => { ).toBeDefined(); }); + 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, }) => { From 13b527384c7ef5dc92edec46279c97a8d1616a2c Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 5 Aug 2026 17:58:05 +0100 Subject: [PATCH 3/3] Withdraw registry entries when a config update leaves no runtime Dropping the unconditional unregister also dropped the safety net it provided when a reload never completes. `#runtime.updateConfig()` stops the existing workerd before starting its replacement and throws if the replacement fails, so `#registerWorkers()` is never reached and the entries written for the previous run stay on disk advertising a debug port nothing is listening on. Their 30s heartbeats keep touching the files, so the 5 minute stale sweep never reclaims them either. Withdraw this instance's entries when a config update or a crash restart fails to produce a running runtime, mirroring the missing-debug-port case. --- packages/miniflare/src/index.ts | 16 +++++++- packages/miniflare/test/dev-registry.spec.ts | 42 ++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts index 2356b80107..1b7c4c9a10 100644 --- a/packages/miniflare/src/index.ts +++ b/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( @@ -2837,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 { diff --git a/packages/miniflare/test/dev-registry.spec.ts b/packages/miniflare/test/dev-registry.spec.ts index d7cdf46b90..a9f083a77d 100644 --- a/packages/miniflare/test/dev-registry.spec.ts +++ b/packages/miniflare/test/dev-registry.spec.ts @@ -1857,6 +1857,48 @@ describe("registry churn across config updates", () => { ).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, }) => {