Skip parsing Early Hints for known empty results (#7561)

* Remove now-unused asset preservation cache v1 from Pages * Refactor ETag handling * Check if-none-match before fulfilling preservation cache * Skip parsing Early Hints for known empty results
cloudflare · Jan 10, 2025 · bcd2020 · bcd2020
1 parent 9db94a9
commit bcd2020
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 9 deletions.
diff --git a/.changeset/fuzzy-nails-invent.md b/.changeset/fuzzy-nails-invent.md
@@ -0,0 +1,5 @@
+---
+"@cloudflare/pages-shared": minor
+---
+
+feat: Return a 304 Not Modified response when matching an asset preservation cache request if appropriate
diff --git a/.changeset/green-socks-trade.md b/.changeset/green-socks-trade.md
@@ -0,0 +1,5 @@
+---
+"@cloudflare/pages-shared": patch
+---
+
+chore: Remove now-unused asset preservation cache (v1)
diff --git a/.changeset/proud-rules-try.md b/.changeset/proud-rules-try.md
@@ -0,0 +1,5 @@
+---
+"@cloudflare/pages-shared": patch
+---
+
+fix: Store an empty result when Early Hints parsing returns nothing or errors. Previously, we weren't storing anything which resulted in Early Hints being parsed on every request.
diff --git a/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts
@@ -510,6 +510,87 @@ describe("asset-server handler", () => {
 		);
 	});
 
+	test("early hints should cache empty link headers", async () => {
+		const deploymentId = "deployment-" + Math.random();
+		const metadata = createMetadataObject({ deploymentId }) as Metadata;
+
+		const findAssetEntryForPath = async (path: string) => {
+			if (path === "/index.html") {
+				return "index.html";
+			}
+
+			return null;
+		};
+
+		// Create cache storage to reuse between requests
+		const { caches } = createCacheStorage();
+
+		const getResponse = async () =>
+			getTestResponse({
+				request: new Request("https://example.com/"),
+				metadata,
+				findAssetEntryForPath,
+				caches,
+				fetchAsset: () =>
+					Promise.resolve(
+						Object.assign(
+							new Response(`
+							<!DOCTYPE html>
+							<html>
+								<body>
+									<h1>I'm a teapot</h1>
+								</body>
+							</html>`),
+							{ contentType: "text/html" }
+						)
+					),
+			});
+
+		const { response, spies } = await getResponse();
+		expect(response.status).toBe(200);
+		// waitUntil should be called twice: once for asset-preservation, once for early hints
+		expect(spies.waitUntil.length).toBe(2);
+
+		await Promise.all(spies.waitUntil);
+
+		const earlyHintsCache = await caches.open(`eh:${deploymentId}`);
+		const earlyHintsRes = await earlyHintsCache.match("https://example.com/");
+
+		if (!earlyHintsRes) {
+			throw new Error(
+				"Did not match early hints cache on https://example.com/"
+			);
+		}
+
+		expect(earlyHintsRes.headers.get("link")).toBeNull();
+
+		// Do it again, but this time ensure that we didn't write to cache again
+		const { response: response2, spies: spies2 } = await getResponse();
+
+		expect(response2.status).toBe(200);
+		// waitUntil should only be called for asset-preservation
+		expect(spies2.waitUntil.length).toBe(1);
+
+		await Promise.all(spies2.waitUntil);
+
+		const earlyHintsRes2 = await earlyHintsCache.match("https://example.com/");
+
+		if (!earlyHintsRes2) {
+			throw new Error(
+				"Did not match early hints cache on https://example.com/"
+			);
+		}
+
+		expect(earlyHintsRes2.headers.get("link")).toBeNull();
+	});
+
+	test.todo(
+		"early hints should temporarily cache failures to parse links",
+		async () => {
+			// I couldn't figure out a way to make HTMLRewriter error out
+		}
+	);
+
 	describe("should serve deleted assets from preservation cache", async () => {
 		beforeEach(() => {
 			vi.useFakeTimers();

diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts
@@ -436,22 +436,30 @@ export async function generateHandler<
 								});
 
 								const linkHeader = preEarlyHintsHeaders.get("Link");
+								const earlyHintsHeaders = new Headers({
+									"Cache-Control": "max-age=2592000", // 30 days
+								});
 								if (linkHeader) {
-									await earlyHintsCache.put(
-										earlyHintsCacheKey,
-										new Response(null, {
-											headers: {
-												Link: linkHeader,
-												"Cache-Control": "max-age=2592000", // 30 days
-											},
-										})
-									);
+									earlyHintsHeaders.append("Link", linkHeader);
 								}
+								await earlyHintsCache.put(
+									earlyHintsCacheKey,
+									new Response(null, { headers: earlyHintsHeaders })
+								);
 							} catch (err) {
 								// Nbd if we fail here in the deferred 'waitUntil' work. We're probably trying to parse a malformed page or something.
 								// Totally fine to skip over any errors.
 								// If we need to debug something, you can uncomment the following:
 								// logError(err)
+								// In any case, let's not bother checking again for another day.
+								await earlyHintsCache.put(
+									earlyHintsCacheKey,
+									new Response(null, {
+										headers: {
+											"Cache-Control": "max-age=86400", // 1 day
+										},
+									})
+								);
 							}
 						})()
 					);