fix(rerouting): check that the new route is different (#8648)

* fix(rerouting): check that the new route is different

* add tests

* changeset grammar

.changeset/wild-bees-sneeze.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixed an issue where a response with status code 404 led to an endless loop of implicit rerouting in dev mode.

packages/astro/src/vite-plugin-astro-server/route.ts

@@ -220,6 +220,7 @@ export async function handleRoute({
 	let response = await pipeline.renderRoute(renderContext, mod);
 	if (response.status === 404 && has404Route(manifestData)) {
 		const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
+		if (fourOhFourRoute?.route !== options.route)
 		return handleRoute({
 			...options,
 			matchedRoute: fourOhFourRoute,
@@ -342,6 +343,6 @@ function getStatus(matchedRoute?: MatchedRoute): 404 | 500 | undefined {
 	if (matchedRoute.route.route === '/500') return 500;
 }
 
-function has404Route(manifest: ManifestData): RouteData | undefined {
-	return manifest.routes.find((route) => route.route === '/404');
+function has404Route(manifest: ManifestData): boolean {
+	return manifest.routes.some((route) => route.route === '/404');
 }

packages/astro/test/custom-404-implicit-rerouting.test.js

@@ -0,0 +1,57 @@
+import { expect } from 'chai';
+import { loadFixture } from './test-utils.js';
+
+for (const caseNumber of [ 1, 2, 3, 4 ]) {
+	describe(`Custom 404 with implicit rerouting - Case #${caseNumber}`, () => {
+		/** @type Awaited<ReturnType<typeof loadFixture>> */ 
+		let fixture;
+		/** @type Awaited<ReturnType<typeof fixture['startDevServer']>> */ 
+		let devServer
+
+		before(async () => {
+			fixture = await loadFixture({
+				root: `./fixtures/custom-404-loop-case-${caseNumber}/`,
+				site: 'http://example.com'
+			});
+
+			devServer = await fixture.startDevServer();
+		});
+
+		// sanity check
+		it('dev server handles normal requests', async () => {
+			const resPromise = fixture.fetch('/');
+			const result = await withTimeout(resPromise, 1000);
+			expect(result).to.not.equal(timeout);
+			expect(result.status).to.equal(200);
+		});
+
+		it('dev server stays responsive', async () => {
+			const resPromise = fixture.fetch('/alvsibdlvjks');
+			const result = await withTimeout(resPromise, 1000);
+			expect(result).to.not.equal(timeout);
+			expect(result.status).to.equal(404);
+		});
+
+		after(async () => {
+			await devServer.stop();
+		});
+	});
+}
+
+
+/***** UTILITY FUNCTIONS *****/
+
+const timeout = Symbol("timeout")
+
+/** @template Res */
+function withTimeout(
+	/** @type Promise<Res> */
+	responsePromise,
+	/** @type number */
+	timeLimit
+) {
+	/** @type Promise<typeof timeout> */
+	const timeoutPromise = new Promise(resolve => setTimeout(() => resolve(timeout), timeLimit))
+
+	return Promise.race([ responsePromise, timeoutPromise ]);
+}

packages/astro/test/fixtures/custom-404-loop-case-1/astro.config.mjs

@@ -0,0 +1 @@
+export default {}

packages/astro/test/fixtures/custom-404-loop-case-1/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/custom-404-loop-case-1",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/custom-404-loop-case-1/src/pages/404.astro

@@ -0,0 +1,4 @@
+---
+Astro.response.status = 404
+---
+<p>four oh four route</p>

packages/astro/test/fixtures/custom-404-loop-case-1/src/pages/index.astro

@@ -0,0 +1 @@
+<p>all good here... or is it?</p>

packages/astro/test/fixtures/custom-404-loop-case-2/astro.config.mjs

@@ -0,0 +1 @@
+export default {}

packages/astro/test/fixtures/custom-404-loop-case-2/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/custom-404-loop-case-2",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/custom-404-loop-case-2/src/pages/404.astro

@@ -0,0 +1,4 @@
+---
+return new Response(null, { status: 404 })
+---
+<p>four oh four route</p>

packages/astro/test/fixtures/custom-404-loop-case-2/src/pages/index.astro

@@ -0,0 +1 @@
+<p>all good here... or is it?</p>

packages/astro/test/fixtures/custom-404-loop-case-3/astro.config.mjs

@@ -0,0 +1 @@
+export default {}

packages/astro/test/fixtures/custom-404-loop-case-3/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/custom-404-loop-case-3",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/custom-404-loop-case-3/src/middleware.js

@@ -0,0 +1,7 @@
+export async function onRequest(ctx, next) {
+    if (ctx.url.pathname !== '/') {
+        const response = await next()
+        return new Response(response.body, { ...response, status: 404 })
+    }
+    return next();
+}

packages/astro/test/fixtures/custom-404-loop-case-3/src/pages/404.astro

@@ -0,0 +1 @@
+<p>four oh four route</p>

packages/astro/test/fixtures/custom-404-loop-case-3/src/pages/index.astro

@@ -0,0 +1 @@
+<p>all good here... or is it?</p>

packages/astro/test/fixtures/custom-404-loop-case-4/astro.config.mjs

@@ -0,0 +1 @@
+export default {}

packages/astro/test/fixtures/custom-404-loop-case-4/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/custom-404-loop-case-4",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/custom-404-loop-case-4/src/middleware.js

@@ -0,0 +1,6 @@
+export function onRequest(ctx, next) {
+    if (ctx.url.pathname !== '/') {
+        return new Response(null, { status: 404 });
+    }
+    return next();
+}

packages/astro/test/fixtures/custom-404-loop-case-4/src/pages/404.astro

@@ -0,0 +1 @@
+<p>four oh four route</p>

packages/astro/test/fixtures/custom-404-loop-case-4/src/pages/index.astro

@@ -0,0 +1 @@
+<p>all good here... or is it?</p>