startDevWorker Milestone 1 - Reboot (#4413)

* Revert "Revert "startDevWorker - Milestone 1" (#4171)"

This reverts commit 88f15f6.

* fix: don't show logs to ProxyWorker(s)
unless log level is debug

* fix: show console.log's in remote mode
remote inspector websocket upgrade request required auth headers
so use `fetch` with `Upgrade: websocket` header instead of `new WebSocket`

* use miniflare verbose mode only if debug log level

* use single Miniflare instance
for (Inspector)ProxyWorker

* port: clear remote runtime logs
upon UserWorker restarts

* default unstable_dev inspectorPort to 0

* parallelise cleanup to minimise chance of hanging
previously, sequential cleanups fail to fully cleanup if earlier steps in the sequence fail

* ensure InspectorProxyWorker unsafeDirectPort is set

* don't use file-system for (Inspector)ProxyWorker DOs

* prevent eviction of the Durable Objects
with (Inspector)ProxyWorker

* remove miniflare workaround for parallel requests

* considerations for race between control messages and user fetches

* use port: undefined vs 0 for UserWorker
to force different port across reloads to workaround workerd bug on Windows

* Don't try to parse `node-internal:* import specifiers

* improve InspectorProxyWorker debug logs

* only proxy consoleAPICalled events in remote mode

* enable consoleAPICalled events proxying if local mode AND service-worker format

* fix userWorkerInnerUrlOverrides host/hostname/port
mainly, base innerUrl off of request.url not userWorkerUrl

* use ProxyWorker ip/port for DEV_SERVER_READY event
instead of UserWorker ip/port

* always disable the UserWorker miniflare pretty error
using the MF-Disable-Pretty-Error header on the UserWorker request

the ProxyWorker will still interpret the json error response
depending on its own MF-Disable-Pretty-Error header

* recover from 'address in use' errors
by trying to start on a random port

* run unit tests in parallel again
by removing --runInBand flag for jest

* add handleRuntimeStdio option to ProxyWorker miniflare instance

* expand containsHexStack check for windows

* logger.debug runtime websocket errors from InspectorProxyWorker
+ remove miniflare log.error overrides no longer needed

* log workerd warnings with logger.warn
not logger.error/info

* enable Cloudflare Access auth for remote previews

* only send Runtime.discardConsoleEntries if currently connected to runtime

---------

Co-authored-by: Samuel Macleod <smacleod@cloudflare.com>
Co-authored-by: MrBBot <bcoll@cloudflare.com>

.changeset/serious-toys-repeat.md

@@ -0,0 +1,12 @@
+---
+"wrangler": minor
+---
+
+Reintroduces some internal refactorings of wrangler dev servers (including `wrangler dev`, `wrangler dev --remote`, and `unstable_dev()`).
+
+These changes were released in 3.13.0 and reverted in 3.13.1 -- we believe the changes are now more stable and ready for release again.
+
+There are no changes required for developers to opt-in. Improvements include:
+
+- fewer 'address in use' errors upon reloads
+- upon config/source file changes, requests are buffered to guarantee the response is from the new version of the Worker

fixtures/dev-env/package.json

@@ -16,7 +16,7 @@
 		"@types/ws": "^8.5.7",
 		"@cloudflare/workers-tsconfig": "workspace:^",
 		"get-port": "^7.0.0",
-		"miniflare": "3.20231002.1",
+		"miniflare": "3.20231025.1",
 		"undici": "^5.23.0",
 		"wrangler": "workspace:*",
 		"ws": "^8.14.2"

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

@@ -18,18 +18,20 @@ const fakeBundle = {} as EsbuildBundle;
 
 let devEnv: DevEnv;
 let mf: Miniflare | undefined;
-let res: MiniflareResponse | undici.Response | undefined;
+let res: MiniflareResponse | undici.Response;
 let ws: WebSocket | undefined;
+let fireAndForgetPromises: Promise<any>[] = [];
 
 type OptionalKeys<T, K extends keyof T> = Omit<T, K> & Partial<Pick<T, K>>;
 
 beforeEach(() => {
 	devEnv = new DevEnv();
 	mf = undefined;
-	res = undefined;
+	res = undefined as any;
 	ws = undefined;
 });
 afterEach(async () => {
+	await Promise.allSettled(fireAndForgetPromises);
 	await devEnv?.teardown();
 	await mf?.dispose();
 	await ws?.close();
@@ -50,7 +52,7 @@ async function fakeStartUserWorker(options: {
 	};
 	const mfOpts: MiniflareOptions = Object.assign(
 		{
-			port: 0,
+			port: undefined,
 			inspectorPort: 0,
 			modules: true,
 			compatibilityDate: "2023-08-01",
@@ -67,10 +69,11 @@ async function fakeStartUserWorker(options: {
 	fakeReloadStart(config);
 
 	const worker = devEnv.startWorker(config);
-	const { proxyWorker, inspectorProxyWorker } = await devEnv.proxy.ready
-		.promise;
+	const { proxyWorker } = await devEnv.proxy.ready.promise;
 	const proxyWorkerUrl = await proxyWorker.ready;
-	const inspectorProxyWorkerUrl = await inspectorProxyWorker.ready;
+	const inspectorProxyWorkerUrl = await proxyWorker.unsafeGetDirectURL(
+		"InspectorProxyWorker"
+	);
 
 	mf = new Miniflare(mfOpts);
 
@@ -135,7 +138,8 @@ function fireAndForgetFakeUserWorkerChanges(
 	...args: Parameters<typeof fakeUserWorkerChanges>
 ) {
 	// fire and forget the reload -- this let's us test request buffering
-	void fakeUserWorkerChanges(...args);
+	const promise = fakeUserWorkerChanges(...args);
+	fireAndForgetPromises.push(promise);
 }
 
 function fakeConfigUpdate(config: StartDevWorkerOptions) {

packages/edge-preview-authenticated-proxy/tests/index.test.ts

@@ -150,10 +150,10 @@ compatibility_date = "2023-01-01"
 		// eslint-disable-next-line @typescript-eslint/no-explicit-any -- ignoring this test type error for sake of turborepo PR
 		const json = (await resp.json()) as any;
 
-		expect(
-			json.headers.find(([h]: [string]) => h === "cf-workers-preview-token")[1]
-		).toBe(token);
-		expect(json.url).toMatchInlineSnapshot('"http://127.0.0.1:6756/"');
+		expect(json).toMatchObject({
+			url: `http://127.0.0.1:${remote.port}/`,
+			headers: expect.arrayContaining([["cf-workers-preview-token", token]]),
+		});
 	});
 	it("should be redirected with cookie", async () => {
 		const resp = await worker.fetch(
@@ -192,10 +192,12 @@ compatibility_date = "2023-01-01"
 		);
 
 		const json = (await resp.json()) as { headers: string[][]; url: string };
-		expect(Object.fromEntries([...json.headers])).toMatchObject({
-			"cf-workers-preview-token": "TEST_TOKEN",
+		expect(json).toMatchObject({
+			url: `http://127.0.0.1:${remote.port}/`,
+			headers: expect.arrayContaining([
+				["cf-workers-preview-token", "TEST_TOKEN"],
+			]),
 		});
-		expect(json.url).toMatchInlineSnapshot('"http://127.0.0.1:6756/"');
 	});
 	it("should not follow redirects", async () => {
 		const resp = await worker.fetch(

packages/wrangler/package.json

@@ -60,11 +60,11 @@
 		"emit-types": "tsc -p tsconfig.emit.json && node -r esbuild-register scripts/emit-types.ts",
 		"prepublishOnly": "SOURCEMAPS=false npm run build",
 		"start": "pnpm run bundle && cross-env NODE_OPTIONS=--enable-source-maps ./bin/wrangler.js",
-		"test": "pnpm run assert-git-version && jest --runInBand",
+		"test": "pnpm run assert-git-version && jest",
 		"test:ci": "pnpm run test --coverage",
 		"test:debug": "pnpm run test --silent=false --verbose=true",
 		"test:e2e": "vitest --test-timeout 240000 --single-thread --dir ./e2e --retry 2 run",
-		"test:watch": "pnpm run test --runInBand --testTimeout=50000 --watch",
+		"test:watch": "pnpm run test --testTimeout=50000 --watch",
 		"type:tests": "tsc -p ./src/__tests__/tsconfig.json && tsc -p ./e2e/tsconfig.json"
 	},
 	"jest": {

packages/wrangler/src/api/dev.ts

@@ -169,7 +169,7 @@ export async function unstable_dev(
 		compatibilityDate: options?.compatibilityDate,
 		compatibilityFlags: options?.compatibilityFlags,
 		ip: options?.ip,
-		inspectorPort: options?.inspectorPort,
+		inspectorPort: options?.inspectorPort ?? 0,
 		v: undefined,
 		localProtocol: options?.localProtocol,
 		assets: options?.assets,

packages/wrangler/src/api/startDevWorker/DevEnv.ts

@@ -1,5 +1,5 @@
+import assert from "node:assert";
 import { EventEmitter } from "node:events";
-import { fetch, Request, type RequestInit } from "miniflare";
 import { logger } from "../../logger";
 import { BundlerController } from "./BundlerController";
 import { ConfigController } from "./ConfigController";
@@ -47,8 +47,14 @@ export class DevEnv extends EventEmitter {
 			controller.on("error", (event) => this.emitErrorEvent(event))
 		);
 
-		this.on("error", (event) => {
-			logger.error(event);
+		this.on("error", (event: ErrorEvent) => {
+			// TODO: when we're are comfortable with StartDevWorker/DevEnv stability,
+			//       we can remove this handler and let the user handle the unknowable errors
+			//       or let the process crash. For now, log them to stderr
+			//       so we can identify knowable vs unknowable error candidates
+
+			logger.error(`Error in ${event.source}: ${event.reason}\n`, event.cause);
+			logger.debug("=> Error contextual data:", event.data);
 		});
 
 		config.on("configUpdate", (event) => {
@@ -99,8 +105,48 @@ export class DevEnv extends EventEmitter {
 		]);
 	}
 
-	emitErrorEvent(data: ErrorEvent) {
-		this.emit("error", data);
+	emitErrorEvent(ev: ErrorEvent) {
+		if (
+			ev.source === "ProxyController" &&
+			ev.reason === "Failed to start ProxyWorker or InspectorProxyWorker"
+		) {
+			assert(ev.data.config); // we must already have a `config` if we've already tried (and failed) to instantiate the ProxyWorker(s)
+
+			const { config } = ev.data;
+			const port = config.dev?.server?.port;
+			const inspectorPort = config.dev?.inspector?.port;
+			const randomPorts = [0, undefined];
+
+			// console.log({ port, inspectorPort, ev });
+			if (!randomPorts.includes(port) || !randomPorts.includes(inspectorPort)) {
+				// emit the event here while the ConfigController is unimplemented
+				// this will cause the ProxyController to try reinstantiating the ProxyWorker(s)
+				// TODO: change this to `this.config.updateOptions({ dev: { server: { port: 0 }, inspector: { port: 0 } } });` when the ConfigController is implemented
+				this.config.emitConfigUpdateEvent({
+					type: "configUpdate",
+					config: {
+						...config,
+						dev: {
+							...config.dev,
+							server: { ...config.dev?.server, port: 0 }, // override port
+							inspector: { ...config.dev?.inspector, port: 0 }, // override port
+						},
+					},
+				});
+			}
+		} else if (
+			ev.source === "ProxyController" &&
+			(ev.reason.startsWith("Failed to send message to") ||
+				ev.reason.startsWith("Could not connect to InspectorProxyWorker"))
+		) {
+			logger.debug(`Error in ${ev.source}: ${ev.reason}\n`, ev.cause);
+			logger.debug("=> Error contextual data:", ev.data);
+		}
+		// if other knowable + recoverable errors occur, handle them here
+		else {
+			// otherwise, re-emit the unknowable errors to the top-level
+			this.emit("error", ev);
+		}
 	}
 }
 
@@ -120,18 +166,9 @@ export function createWorkerObject(devEnv: DevEnv): DevWorker {
 		},
 		async fetch(...args) {
 			const { proxyWorker } = await devEnv.proxy.ready.promise;
-			// return proxyWorker.dispatchFetch(...args);
-			// ^ bug: Miniflare#dispatchFetch uses one HTTP/1.1 connection, preventing parallel requests (pause/play requests + buffered eyeball requests)
-			// workaround: use undici.fetch
-			const proxyWorkerUrl = await proxyWorker.ready;
-			const req = new Request(...args);
-			const url = new URL(req.url);
-			url.protocol = proxyWorkerUrl.protocol;
-			url.hostname = proxyWorkerUrl.hostname;
-			url.port = proxyWorkerUrl.port;
-			// /workaround
-
-			return fetch(url, req as RequestInit);
+			await devEnv.proxy.runtimeMessageMutex.drained();
+
+			return proxyWorker.dispatchFetch(...args);
 		},
 		async queue(..._args) {
 			// const { worker } = await devEnv.proxy.ready;