From 1154c7b6b4753e132d54586d8783f12441bdd080 Mon Sep 17 00:00:00 2001
From: Greg Brimble <gbrimble@cloudflare.com>
Date: Fri, 13 Dec 2024 11:55:53 -0500
Subject: [PATCH] Check if-none-match before fulfilling preservation cache

---
 .changeset/fuzzy-nails-invent.md              |  5 ++++
 .../__tests__/asset-server/handler.test.ts    | 28 ++++++++++++++-----
 packages/pages-shared/asset-server/handler.ts | 18 +++++++++++-
 3 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100644 .changeset/fuzzy-nails-invent.md

diff --git a/.changeset/fuzzy-nails-invent.md b/.changeset/fuzzy-nails-invent.md
new file mode 100644
index 000000000000..e21d23648c10
--- /dev/null
+++ 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/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts
index b1ba50cb35ea..b97c1a8e91f0 100644
--- a/packages/pages-shared/__tests__/asset-server/handler.test.ts
+++ b/packages/pages-shared/__tests__/asset-server/handler.test.ts
@@ -572,11 +572,25 @@ describe("asset-server handler", () => {
 				'"asset-key-foo.html"'
 			);
 
-			// Delete the asset from the manifest and ensure it's served from preservation cache
+			// Delete the asset from the manifest and ensure it's served from preservation cache with a 304 when if-none-match is present
 			findAssetEntryForPath = async (_path: string) => {
 				return null;
 			};
 			const { response: response2 } = await getTestResponse({
+				request: new Request("https://example.com/foo", {
+					headers: { "if-none-match": expectedHeaders.etag },
+				}),
+				metadata,
+				findAssetEntryForPath,
+				caches,
+				fetchAsset: () =>
+					Promise.resolve(Object.assign(new Response("hello world!"))),
+			});
+			expect(response2.status).toBe(304);
+			expect(await response2.text()).toMatchInlineSnapshot('""');
+
+			// Ensure the asset is served from preservation cache with a 200 if if-none-match is not present
+			const { response: response3 } = await getTestResponse({
 				request: new Request("https://example.com/foo"),
 				metadata,
 				findAssetEntryForPath,
@@ -584,10 +598,10 @@ describe("asset-server handler", () => {
 				fetchAsset: () =>
 					Promise.resolve(Object.assign(new Response("hello world!"))),
 			});
-			expect(response2.status).toBe(200);
-			expect(await response2.text()).toMatchInlineSnapshot('"hello world!"');
+			expect(response3.status).toBe(200);
+			expect(await response3.text()).toMatchInlineSnapshot('"hello world!"');
 			// Cached responses have the same headers with a few changes/additions:
-			expect(Object.fromEntries(response2.headers)).toStrictEqual({
+			expect(Object.fromEntries(response3.headers)).toStrictEqual({
 				...expectedHeaders,
 				"cache-control": "public, s-maxage=604800",
 				"x-robots-tag": "noindex",
@@ -595,15 +609,15 @@ describe("asset-server handler", () => {
 			});
 
 			// Serve with a fresh cache and ensure we don't get a response
-			const { response: response3 } = await getTestResponse({
+			const { response: response4 } = await getTestResponse({
 				request: new Request("https://example.com/foo"),
 				metadata,
 				findAssetEntryForPath,
 				fetchAsset: () =>
 					Promise.resolve(Object.assign(new Response("hello world!"))),
 			});
-			expect(response3.status).toBe(404);
-			expect(Object.fromEntries(response3.headers)).toMatchInlineSnapshot(`
+			expect(response4.status).toBe(404);
+			expect(Object.fromEntries(response4.headers)).toMatchInlineSnapshot(`
 				{
 				  "access-control-allow-origin": "*",
 				  "cache-control": "no-store",
diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts
index 11bda07be216..8002158c1fce 100644
--- a/packages/pages-shared/asset-server/handler.ts
+++ b/packages/pages-shared/asset-server/handler.ts
@@ -95,7 +95,10 @@ type ServeAsset<AssetEntry> = (
 type CacheStatus = "hit" | "miss";
 type CacheResult<A extends string> = `${A}-${CacheStatus}`;
 export type HandlerMetrics = {
-	preservationCacheResult?: CacheResult<"checked"> | "disabled";
+	preservationCacheResult?:
+		| CacheResult<"checked">
+		| "not-modified"
+		| "disabled";
 	earlyHintsResult?: CacheResult<"used" | "notused"> | "disabled";
 };
 
@@ -663,6 +666,19 @@ export async function generateHandler<
 						return new Response(null, preservedResponse);
 					}
 					if (assetKey) {
+						const { strongETag, weakETag } = generateETagHeader(assetKey);
+						const isIfNoneMatch = checkIfNoneMatch(
+							request,
+							strongETag,
+							weakETag
+						);
+						if (isIfNoneMatch) {
+							if (setMetrics) {
+								setMetrics({ preservationCacheResult: "not-modified" });
+							}
+							return new NotModifiedResponse();
+						}
+
 						const asset = await fetchAsset(assetKey);
 
 						if (asset) {