diff --git a/.changeset/clean-caches-settle.md b/.changeset/clean-caches-settle.md new file mode 100644 index 0000000..a845151 --- /dev/null +++ b/.changeset/clean-caches-settle.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/test/workerScreenshotCache.test.ts b/test/workerScreenshotCache.test.ts index aaa19bd..a9412fe 100644 --- a/test/workerScreenshotCache.test.ts +++ b/test/workerScreenshotCache.test.ts @@ -199,6 +199,63 @@ test("malformed hits and unsafe capture responses never escape into shared cache } }); +test("Browser Rendering errors stay errors and never enter the edge cache", async () => { + const request = new Request(cardUrl()); + const { url, plan: screenshot } = plan(cardUrl()); + const { cache, entries } = memoryCache(); + const deferred: Promise[] = []; + + const response = await servePostScreenshot({ + request, + requestUrl: url, + postId: "post_1", + plan: screenshot, + rendererGeneration: GENERATION, + clientCacheControl: "private, max-age=300", + defer: (promise) => deferred.push(promise), + authorize: async () => new Response("renderable"), + capture: async () => + new Response('{"error":"rate limited"}', { + status: 429, + headers: { "content-type": "application/json" }, + }), + cache, + }); + await Promise.all(deferred); + + assert.equal(response.status, 429); + assert.equal(response.headers.get("content-type"), "application/json"); + assert.equal(response.headers.get("cache-control"), "no-store"); + assert.equal(await response.text(), '{"error":"rate limited"}'); + assert.equal(entries.size, 0); +}); + +test("a non-PNG capture never gains a client cache policy", async () => { + const request = new Request(cardUrl()); + const { url, plan: screenshot } = plan(cardUrl()); + const { cache, entries } = memoryCache(); + + const response = await servePostScreenshot({ + request, + requestUrl: url, + postId: "post_1", + plan: screenshot, + rendererGeneration: GENERATION, + clientCacheControl: "public, max-age=300", + defer: () => {}, + authorize: async () => new Response("renderable"), + capture: async () => + new Response("not an image", { headers: { "content-type": "text/plain" } }), + cache, + }); + + assert.equal(response.status, 200); + assert.equal(response.headers.get("content-type"), "text/plain"); + assert.equal(response.headers.get("cache-control"), "no-store"); + assert.equal(await response.text(), "not an image"); + assert.equal(entries.size, 0); +}); + test("orchestration revalidates before hits and applies the current access policy", async () => { const request = () => new Request(cardUrl()); const { url, plan: screenshot } = plan(cardUrl()); diff --git a/workers/screenshotCache.ts b/workers/screenshotCache.ts index 5886e11..171c488 100644 --- a/workers/screenshotCache.ts +++ b/workers/screenshotCache.ts @@ -203,6 +203,10 @@ export async function servePostScreenshot({ defer, async () => { const screenshot = await capture(); + // Browser Rendering returns JSON error responses for failures. Preserve an + // unsafe response rather than relabeling its body as a successful PNG: + // doing otherwise would let the cache admit an hour-long error payload. + if (screenshot.status !== 200 || !isPng(screenshot)) return screenshot; return new Response(await screenshot.arrayBuffer(), { headers: { "content-type": "image/png", "cache-control": clientCacheControl }, }); @@ -214,6 +218,12 @@ export async function servePostScreenshot({ // above uses the CURRENT policy, so its client directive must also win over // the policy recorded with an older internal entry. const out = new Response(response.body, response); - out.headers.set("cache-control", clientCacheControl); + // A rejected Browser Rendering response must not acquire the normal positive + // client TTL: an intermediary could otherwise cache a transient error even + // though the Worker did not put it in the edge cache. + out.headers.set( + "cache-control", + response.status === 200 && isPng(response) ? clientCacheControl : "no-store", + ); return out; }