diff --git a/.changeset/miniflare-loopback-bind-error.md b/.changeset/miniflare-loopback-bind-error.md new file mode 100644 index 00000000000..e4281a20e2a --- /dev/null +++ b/.changeset/miniflare-loopback-bind-error.md @@ -0,0 +1,7 @@ +--- +"miniflare": patch +--- + +Reject loopback server bind failures during Miniflare startup instead of leaving `ready` and `dispose()` hanging + +`#startLoopbackServer` now attaches an `error` listener before `listen`, matching the inspector proxy. When the configured host cannot be bound (e.g. `192.0.2.1`), `ready` rejects and `dispose()` still settles even if the loopback server never started. diff --git a/.changeset/tidy-miniflare-listeners.md b/.changeset/tidy-miniflare-listeners.md new file mode 100644 index 00000000000..2dafe737d4c --- /dev/null +++ b/.changeset/tidy-miniflare-listeners.md @@ -0,0 +1,7 @@ +--- +"miniflare": patch +--- + +Clean up Miniflare listener startup error handlers + +Loopback and inspector servers now remove startup-only error handlers after binding and close the server after bind failures. diff --git a/fixtures/additional-modules/test/index.test.ts b/fixtures/additional-modules/test/index.test.ts index 33495fd739e..98c210dc397 100644 --- a/fixtures/additional-modules/test/index.test.ts +++ b/fixtures/additional-modules/test/index.test.ts @@ -19,6 +19,11 @@ function get(worker: WranglerDev, pathname: string) { return worker.fetch(url, { headers: { "MF-Disable-Pretty-Error": "true" } }); } +function waitForReload(callback: () => Promise) { + // File watching and Wrangler reloads can exceed `waitFor`'s one-second default under Windows load + return vi.waitFor(callback, { timeout: 5_000 }); +} + describe("find_additional_modules dev", () => { let tmpDir: string; let worker: WranglerDev; @@ -70,7 +75,8 @@ describe("find_additional_modules dev", () => { expect(await res.text()).toBe("hello"); }); - test("watches additional modules", async ({ expect }) => { + // This test mutates shared fixture state and cannot be safely retried. + test("watches additional modules", { retry: 0 }, async ({ expect }) => { const srcDir = path.join(tmpDir, "src"); // Update dynamically imported file @@ -78,7 +84,7 @@ describe("find_additional_modules dev", () => { path.join(srcDir, "dynamic.js"), 'export default "new dynamic";' ); - await vi.waitFor(async () => { + await waitForReload(async () => { const res = await get(worker, "/dynamic"); assert.strictEqual(await res.text(), "new dynamic"); }); @@ -86,7 +92,7 @@ describe("find_additional_modules dev", () => { // Delete dynamically imported file await fs.rm(path.join(srcDir, "lang", "en.js")); - await vi.waitFor(async () => { + await waitForReload(async () => { await expect(get(worker, "/lang/en")).rejects.toThrow( 'No such module "lang/en.js".' ); @@ -98,7 +104,7 @@ describe("find_additional_modules dev", () => { path.join(srcDir, "lang", "en", "us.js"), 'export default { hello: "hey" };' ); - await vi.waitFor(async () => { + await waitForReload(async () => { const res = await get(worker, "/lang/en/us"); assert.strictEqual(await res.text(), "hey"); }); @@ -108,7 +114,7 @@ describe("find_additional_modules dev", () => { path.join(srcDir, "lang", "en", "us.js"), 'export default { hello: "bye" };' ); - await vi.waitFor(async () => { + await waitForReload(async () => { const res = await get(worker, "/lang/en/us"); assert.strictEqual(await res.text(), "bye"); }); diff --git a/fixtures/multi-worker/tests/multi-worker.test.ts b/fixtures/multi-worker/tests/multi-worker.test.ts index f2a82232466..3a584392115 100644 --- a/fixtures/multi-worker/tests/multi-worker.test.ts +++ b/fixtures/multi-worker/tests/multi-worker.test.ts @@ -9,11 +9,15 @@ describe("Multi Worker", () => { ["-c=workers/sentry/wrangler.jsonc", "-c=workers/default/wrangler.jsonc"] ); try { - await vi.waitFor(async () => { - const response = await fetch(`http://${ip}:${port}/`); - const text = await response.text(); - expect(text).toBe(`Hello World!`); - }); + await vi.waitFor( + async () => { + const response = await fetch(`http://${ip}:${port}/`); + const text = await response.text(); + expect(text).toBe(`Hello World!`); + }, + // The TCP listener can be ready before the multi-worker runtime and Sentry integration can serve requests under Windows load + { timeout: 10_000, interval: 100 } + ); } finally { await stop(); } diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts index 9dabb8e400e..4ba4205b159 100644 --- a/packages/miniflare/src/index.ts +++ b/packages/miniflare/src/index.ts @@ -1930,7 +1930,7 @@ export class Miniflare { hostname = "::"; } - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const server = stoppable( http.createServer(this.#handleLoopback), /* grace */ 0 @@ -1944,14 +1944,33 @@ export class Miniflare { // already disable their timeouts. server.keepAliveTimeout = 0; server.on("upgrade", this.#handleLoopbackUpgrade); - server.listen(0, hostname, () => resolve(server)); + const onError = (error: Error) => { + server.close(); + reject(error); + }; + server.once("error", onError); + server.listen(0, hostname, () => { + server.off("error", onError); + resolve(server); + }); }); } #stopLoopbackServer(): Promise { + const loopbackServer = this.#loopbackServer; + if (loopbackServer === undefined) { + return Promise.resolve(); + } return new Promise((resolve, reject) => { - assert(this.#loopbackServer !== undefined); - this.#loopbackServer.stop((err) => (err ? reject(err) : resolve())); + loopbackServer.stop((err) => { + if (err) { + reject(err); + return; + } + this.#loopbackServer = undefined; + this.#loopbackHost = undefined; + resolve(); + }); }); } diff --git a/packages/miniflare/src/plugins/core/inspector-proxy/inspector-proxy-controller.ts b/packages/miniflare/src/plugins/core/inspector-proxy/inspector-proxy-controller.ts index 32d879fd65a..71b6b751b38 100644 --- a/packages/miniflare/src/plugins/core/inspector-proxy/inspector-proxy-controller.ts +++ b/packages/miniflare/src/plugins/core/inspector-proxy/inspector-proxy-controller.ts @@ -61,9 +61,8 @@ export class InspectorProxyController { res.end(null); }); - this.#initializeWebSocketServer(server); - await this.#startListening(server); + this.#initializeWebSocketServer(server); return server; } @@ -84,12 +83,15 @@ export class InspectorProxyController { `Trying to listen on ${this.inspectorHostOption}:${this.inspectorPortOption}` ); return new Promise((resolve, reject) => { - server.once("error", reject); - server.listen( - this.inspectorPortOption, - this.inspectorHostOption, - resolve - ); + const onError = (error: Error) => { + server.close(); + reject(error); + }; + server.prependOnceListener("error", onError); + server.listen(this.inspectorPortOption, this.inspectorHostOption, () => { + server.off("error", onError); + resolve(); + }); }); } @@ -108,6 +110,7 @@ export class InspectorProxyController { #initializeWebSocketServer(server: Server) { const devtoolsWebSocketServer = new WebSocketServer({ server }); + devtoolsWebSocketServer.on("error", (error) => this.log.error(error)); devtoolsWebSocketServer.on("connection", (devtoolsWs, upgradeRequest) => { const validationError = diff --git a/packages/miniflare/test/dev-registry.spec.ts b/packages/miniflare/test/dev-registry.spec.ts index 07183e91624..8b269286b40 100644 --- a/packages/miniflare/test/dev-registry.spec.ts +++ b/packages/miniflare/test/dev-registry.spec.ts @@ -2445,7 +2445,7 @@ describe.sequential("DevRegistry", () => { await remote.ready; const logs: string[] = []; - const local = new Miniflare({ + const localOptions: MiniflareOptions = { unsafeDevRegistryPath, handleStructuredLogs: ({ message }) => void logs.push(message), workers: [ @@ -2467,7 +2467,8 @@ describe.sequential("DevRegistry", () => { }, }, ], - }); + }; + const local = new Miniflare(localOptions); useDispose(local); await local.ready; @@ -2480,11 +2481,20 @@ describe.sequential("DevRegistry", () => { { timeout: 10_000, interval: 100 } ); - // Drop the peer without letting it deregister, so `local` keeps a registry - // entry pointing at a debug port that is no longer accepting connections. + const remoteDefinitionPath = path.join( + unsafeDevRegistryPath, + "remote-worker" + ); + const remoteDefinition = await fs.readFile(remoteDefinitionPath, "utf8"); + + // Restore the registry entry removed by disposal to model a peer that exited + // without cleaning up. This leaves `local` pointing at a debug port that is + // no longer accepting connections, regardless of watcher timing. // The forwarding RPC now rejects; that rejection must be reported rather // than escaping as an unhandled rejection. await remote.dispose(); + await fs.writeFile(remoteDefinitionPath, remoteDefinition); + await local.setOptions(localOptions); logs.length = 0; await vi.waitFor( diff --git a/packages/miniflare/test/index.spec.ts b/packages/miniflare/test/index.spec.ts index 0dc08bfe8a5..5197822b077 100644 --- a/packages/miniflare/test/index.spec.ts +++ b/packages/miniflare/test/index.spec.ts @@ -429,6 +429,99 @@ test("Miniflare: can use localhost as host", async ({ expect }) => { expect(await res.text()).toBe("body"); }); +test("Miniflare: removes loopback startup error handler after listening", async ({ + expect, + onTestFinished, +}) => { + const createServer = vi.spyOn(http, "createServer"); + onTestFinished(() => createServer.mockRestore()); + + const mf = new Miniflare({ + workers: [ + { + config: { + type: "worker", + name: "", + compatibilityDate: "2025-05-01", + manifest: singleModuleManifest( + `export default { fetch() { return new Response("ok"); } }` + ), + }, + }, + ], + }); + useDispose(mf); + + await mf.ready; + + expect(createServer).toHaveBeenCalledOnce(); + const server = createServer.mock.results[0].value; + expect(server.listenerCount("error")).toBe(0); +}); + +test("Miniflare: rejects ready when loopback server cannot bind", async ({ + expect, + onTestFinished, +}) => { + const createServer = vi.spyOn(http, "createServer"); + const close = vi.spyOn(http.Server.prototype, "close"); + onTestFinished(() => { + createServer.mockRestore(); + close.mockRestore(); + }); + + const mf = new Miniflare({ + host: "192.0.2.1", + workers: [ + { + config: { + type: "worker", + name: "", + compatibilityDate: "2025-05-01", + manifest: singleModuleManifest( + `export default { fetch() { return new Response("ok"); } }` + ), + }, + }, + ], + }); + + await expect(mf.ready).rejects.toMatchObject({ code: "EADDRNOTAVAIL" }); + expect(createServer).toHaveBeenCalledOnce(); + const server = createServer.mock.results[0].value; + expect(close.mock.instances).toContain(server); + expect(server.listening).toBe(false); + await expect(mf.dispose()).rejects.toMatchObject({ code: "EADDRNOTAVAIL" }); +}); + +test("Miniflare: setOptions: recovers after loopback bind failure", async ({ + expect, +}) => { + const worker = { + config: { + type: "worker" as const, + name: "", + compatibilityDate: "2025-05-01", + manifest: singleModuleManifest( + `export default { fetch() { return new Response("ok"); } }` + ), + }, + }; + const mf = new Miniflare({ host: "127.0.0.1", workers: [worker] }); + useDispose(mf); + + await mf.ready; + + await expect( + mf.setOptions({ host: "192.0.2.1", workers: [worker] }) + ).rejects.toMatchObject({ code: "EADDRNOTAVAIL" }); + + await mf.setOptions({ host: "127.0.0.1", workers: [worker] }); + + const res = await mf.dispatchFetch("https://example.com"); + expect(await res.text()).toBe("ok"); +}); + test("Miniflare: can use IPv6 loopback as host", async ({ expect }) => { const mf = new Miniflare({ host: "::1", diff --git a/packages/miniflare/test/plugins/browser/process.spec.ts b/packages/miniflare/test/plugins/browser/process.spec.ts index 3fac676b3a4..1f211c6d989 100644 --- a/packages/miniflare/test/plugins/browser/process.spec.ts +++ b/packages/miniflare/test/plugins/browser/process.spec.ts @@ -34,7 +34,8 @@ test("gracefully closes Chrome over CDP", async ({ expect }) => { await closeBrowserProcess( browserProcess, `ws://127.0.0.1:${address.port}`, - 100 + // Leave enough time for the WebSocket handshake when the suite is busy under macOS load + 1_000 ); expect(browserProcess.kill).not.toHaveBeenCalled(); diff --git a/packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts b/packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts index b4f6f6690d6..632a7144a21 100644 --- a/packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts +++ b/packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts @@ -1,10 +1,16 @@ import events from "node:events"; +import http from "node:http"; import { setTimeout } from "node:timers/promises"; import getPort from "get-port"; import { fetch, Miniflare, MiniflareCoreError } from "miniflare"; import { beforeAll, test, vi } from "vitest"; import WebSocket from "ws"; -import { singleModuleManifest, useDispose } from "../../../test-shared"; +import { InspectorProxyController } from "../../../../src/plugins/core/inspector-proxy"; +import { + singleModuleManifest, + TestLog, + useDispose, +} from "../../../test-shared"; import type { MiniflareOptions } from "miniflare"; const nullScript = @@ -17,6 +23,58 @@ beforeAll(() => { process.env.MINIFLARE_ASSERT_BODIES_CONSUMED = undefined; }); +test("InspectorProxy: removes startup error handler after listening", async ({ + expect, + onTestFinished, +}) => { + const off = vi.spyOn(http.Server.prototype, "off"); + const log = new TestLog(); + const logError = vi.spyOn(log, "error").mockImplementation(() => {}); + const controller = new InspectorProxyController( + 0, + "127.0.0.1", + log, + new Set() + ); + onTestFinished(async () => { + await controller.dispose(); + off.mockRestore(); + logError.mockRestore(); + }); + + await controller.getInspectorURL(); + + expect(off).toHaveBeenCalledWith("error", expect.any(Function)); + const errorListenerRemovalIndex = off.mock.calls.findIndex( + ([event]) => event === "error" + ); + const server = off.mock.instances[errorListenerRemovalIndex] as http.Server; + expect(server.listenerCount("error")).toBe(1); + + const error = new Error("test error"); + server.emit("error", error); + expect(logError).toHaveBeenCalledWith(error); +}); + +test("InspectorProxy: closes server after bind failure", async ({ + expect, + onTestFinished, +}) => { + const close = vi.spyOn(http.Server.prototype, "close"); + onTestFinished(() => close.mockRestore()); + const controller = new InspectorProxyController( + 0, + "192.0.2.1", + new TestLog(), + new Set() + ); + + await expect(controller.getInspectorURL()).rejects.toMatchObject({ + code: "EADDRNOTAVAIL", + }); + expect(close).toHaveBeenCalledOnce(); +}); + test("InspectorProxy: /json/version should provide details about the inspector version", async ({ expect, }) => {