Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip parsing Early Hints for known empty results #7561

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+183 −138
Open

Skip parsing Early Hints for known empty results #7561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a37943c
Remove now-unused asset preservation cache v1 from Pages
GregBrimble Dec 13, 2024
9f8cb27
Refactor ETag handling
GregBrimble Dec 13, 2024
e0e7703
Check if-none-match before fulfilling preservation cache
GregBrimble Dec 13, 2024
356e483
Skip parsing Early Hints for known empty results
GregBrimble Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fuzzy-nails-invent.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/pages-shared": minor
---

feat: Return a 304 Not Modified response when matching an asset preservation cache request if appropriate
5 changes: 5 additions & 0 deletions .changeset/green-socks-trade.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/pages-shared": patch
---

chore: Remove now-unused asset preservation cache (v1)
5 changes: 5 additions & 0 deletions .changeset/proud-rules-try.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
191 changes: 101 additions & 90 deletions packages/pages-shared/__tests__/asset-server/handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried:

  • a PNG with image/png (but we have a check earlier for the content type)
  • a PNG with text/html (but HTMLRewriter just skipped over it fine)
  • malformed HTML (but HTMLRewriter just skipped over it fine)

}
);

describe("should serve deleted assets from preservation cache", async () => {
beforeEach(() => {
vi.useFakeTimers();
Expand Down Expand Up @@ -572,128 +653,58 @@ 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"),
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(200);
expect(await response2.text()).toMatchInlineSnapshot('"hello world!"');
// Cached responses have the same headers with a few changes/additions:
expect(Object.fromEntries(response2.headers)).toStrictEqual({
...expectedHeaders,
"cache-control": "public, s-maxage=604800",
"x-robots-tag": "noindex",
"cf-cache-status": "HIT", // new header
});
expect(response2.status).toBe(304);
expect(await response2.text()).toMatchInlineSnapshot('""');

// Serve with a fresh cache and ensure we don't get a response
// 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,
fetchAsset: () =>
Promise.resolve(Object.assign(new Response("hello world!"))),
});
expect(response3.status).toBe(404);
expect(Object.fromEntries(response3.headers)).toMatchInlineSnapshot(`
{
"access-control-allow-origin": "*",
"cache-control": "no-store",
"referrer-policy": "strict-origin-when-cross-origin",
}
`);
});

test("preservationCacheV1 (fallback)", async () => {
vi.setSystemTime(new Date("2024-05-09")); // 1 day before fallback is disabled

const deploymentId = "deployment-" + Math.random();
const metadata = createMetadataObject({ deploymentId }) as Metadata;
const { caches } = createCacheStorage();

const preservationCacheV1 = await caches.open("assetPreservationCache");

// Write a response to the V1 cache and make sure it persists
await preservationCacheV1.put(
"https://example.com/foo",
new Response("preserved in V1 cache!", {
headers: {
"Cache-Control": "public, max-age=300",
},
})
);

const preservationRes = await preservationCacheV1.match(
"https://example.com/foo"
);

if (!preservationRes) {
throw new Error(
"Did not match preservation cache on https://example.com/foo"
);
}

expect(await preservationRes.text()).toMatchInlineSnapshot(
`"preserved in V1 cache!"`
);

// Delete the asset from the manifest and ensure it's served from V1 preservation cache
const findAssetEntryForPath = async (_path: string) => {
return null;
};
const { response, spies } = await getTestResponse({
request: new Request("https://example.com/foo"),
metadata,
findAssetEntryForPath,
caches,
fetchAsset: () =>
Promise.resolve(Object.assign(new Response("hello world!"))),
});
expect(response.status).toBe(200);
expect(await response.text()).toMatchInlineSnapshot(
`"preserved in V1 cache!"`
);
expect(Object.fromEntries(response.headers)).toMatchInlineSnapshot(`
{
"access-control-allow-origin": "*",
"cache-control": "public, max-age=300",
"cf-cache-status": "HIT",
"content-type": "text/plain;charset=UTF-8",
"referrer-policy": "strict-origin-when-cross-origin",
"x-content-type-options": "nosniff",
}
`);
// No cache or early hints writes
expect(spies.waitUntil.length).toBe(0);
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(response3.headers)).toStrictEqual({
...expectedHeaders,
"cache-control": "public, s-maxage=604800",
"x-robots-tag": "noindex",
"cf-cache-status": "HIT", // new header
});

// Should disable fallback starting may 10th
vi.setSystemTime(new Date("2024-05-10"));
const { response: response2, spies: spies2 } = await getTestResponse({
// Serve with a fresh cache and ensure we don't get a response
const { response: response4 } = await getTestResponse({
request: new Request("https://example.com/foo"),
metadata,
findAssetEntryForPath,
caches,
fetchAsset: () =>
Promise.resolve(Object.assign(new Response("hello world!"))),
});
expect(response2.status).toBe(404);
expect(Object.fromEntries(response2.headers)).toMatchInlineSnapshot(`
expect(response4.status).toBe(404);
expect(Object.fromEntries(response4.headers)).toMatchInlineSnapshot(`
{
"access-control-allow-origin": "*",
"cache-control": "no-store",
"referrer-policy": "strict-origin-when-cross-origin",
}
`);
// No cache or early hints writes
expect(spies2.waitUntil.length).toBe(0);
});
});

Expand Down
Loading
Loading