diff --git a/.changeset/ten-jobs-bathe.md b/.changeset/ten-jobs-bathe.md new file mode 100644 index 000000000000..a7956e09b4e6 --- /dev/null +++ b/.changeset/ten-jobs-bathe.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes cached assets failing to revalidate due to redirect check mishandling Not Modified responses. diff --git a/packages/astro/src/assets/build/generate.ts b/packages/astro/src/assets/build/generate.ts index eacbe4ff0c79..82717a397a15 100644 --- a/packages/astro/src/assets/build/generate.ts +++ b/packages/astro/src/assets/build/generate.ts @@ -176,13 +176,11 @@ export async function generateImagesForPath( } else { const JSONData = JSON.parse(readFileSync(cachedMetaFileURL, 'utf-8')) as RemoteCacheEntry; - if (!JSONData.expires) { - try { - await fs.promises.unlink(cachedFileURL); - } catch { - /* Old caches may not have a separate image binary, no-op */ - } - await fs.promises.unlink(cachedMetaFileURL); + if (typeof JSONData.expires !== 'number') { + await Promise.allSettled([ + fs.promises.unlink(cachedFileURL), + fs.promises.unlink(cachedMetaFileURL), + ]); throw new Error( `Malformed cache entry for ${filepath}, cache will be regenerated for this file.`, @@ -203,9 +201,7 @@ export async function generateImagesForPath( if (JSONData.expires > Date.now()) { await fs.promises.copyFile(cachedFileURL, finalFileURL, fs.constants.COPYFILE_FICLONE); - return { - cached: 'hit', - }; + return { cached: 'hit' }; } // Try to revalidate the cache @@ -216,18 +212,16 @@ export async function generateImagesForPath( lastModified: JSONData.lastModified, }); - if (revalidatedData.data.length) { + if (revalidatedData.data !== null) { // Image cache was stale, update original image to avoid redownload - originalImage = revalidatedData; + originalImage = revalidatedData as ImageData; } else { - // Freshen cache on disk - await writeCacheMetaFile(cachedMetaFileURL, revalidatedData, env); - - await fs.promises.copyFile( - cachedFileURL, - finalFileURL, - fs.constants.COPYFILE_FICLONE, - ); + // Freshen cache on disk and output cached image + await Promise.all([ + writeCacheMetaFile(cachedMetaFileURL, revalidatedData, env), + fs.promises.copyFile(cachedFileURL, finalFileURL, fs.constants.COPYFILE_FICLONE), + ]); + return { cached: 'revalidated' }; } } catch (e) { @@ -242,8 +236,10 @@ export async function generateImagesForPath( } } - await fs.promises.unlink(cachedFileURL); - await fs.promises.unlink(cachedMetaFileURL); + await Promise.allSettled([ + fs.promises.unlink(cachedFileURL), + fs.promises.unlink(cachedMetaFileURL), + ]); } } catch (e: any) { if (e.code !== 'ENOENT') { @@ -339,6 +335,7 @@ async function writeCacheMetaFile( etag: resultData.etag, lastModified: resultData.lastModified, }), + 'utf-8', ); } catch (e) { env.logger.warn( diff --git a/packages/astro/src/assets/build/remote.ts b/packages/astro/src/assets/build/remote.ts index 12f6ed8ae75a..eae8e85f3a3b 100644 --- a/packages/astro/src/assets/build/remote.ts +++ b/packages/astro/src/assets/build/remote.ts @@ -7,9 +7,9 @@ export type RemoteCacheEntry = { lastModified?: string; }; -export async function loadRemoteImage(src: string) { +export async function loadRemoteImage(src: string, fetchFn: typeof fetch = globalThis.fetch) { const req = new Request(src); - const res = await fetch(req, { redirect: 'manual' }); + const res = await fetchFn(req, { redirect: 'manual' }); if (res.status >= 300 && res.status < 400) { throw new Error(`Failed to load remote image ${src}. The request was redirected.`); @@ -40,25 +40,27 @@ export async function loadRemoteImage(src: string) { * The remote server may respond that the cached asset is still up-to-date if the entity-tag or modification time matches (304 Not Modified), or respond with an updated asset (200 OK) * @param src - url to remote asset * @param revalidationData - an object containing the stored Entity-Tag of the cached asset and/or the Last Modified time - * @returns An ImageData object containing the asset data, a new expiry time, and the asset's etag. The data buffer will be empty if the asset was not modified. + * @returns An object containing the refreshed expiry time and cache headers. `data` will be a `Buffer` of the new image if the asset was modified (200 OK), or `null` if the cached version is still valid (304 Not Modified). */ export async function revalidateRemoteImage( src: string, revalidationData: { etag?: string; lastModified?: string }, + fetchFn: typeof fetch = globalThis.fetch, ) { const headers = { ...(revalidationData.etag && { 'If-None-Match': revalidationData.etag }), ...(revalidationData.lastModified && { 'If-Modified-Since': revalidationData.lastModified }), }; const req = new Request(src, { headers, cache: 'no-cache' }); - const res = await fetch(req, { redirect: 'manual' }); + const res = await fetchFn(req, { redirect: 'manual' }); - if (res.status >= 300 && res.status < 400) { - throw new Error(`Failed to revalidate cached remote image ${src}. The request was redirected.`); - } - - // Asset not modified: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/304 + // Allow 304 Not Modified: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/304 if (!res.ok && res.status !== 304) { + if (res.status >= 300 && res.status < 400) { + throw new Error( + `Failed to revalidate cached remote image ${src}. The request was redirected.`, + ); + } throw new Error( `Failed to revalidate cached remote image ${src}. The request did not return a 200 OK / 304 NOT MODIFIED response. (received ${res.status} ${res.statusText})`, ); @@ -68,7 +70,7 @@ export async function revalidateRemoteImage( if (res.ok && !data.length) { // Server did not include body but indicated cache was stale - return await loadRemoteImage(src); + return await loadRemoteImage(src, fetchFn); } // calculate an expiration date based on the response's TTL @@ -76,12 +78,12 @@ export async function revalidateRemoteImage( webToCachePolicyRequest(req), webToCachePolicyResponse( res.ok ? res : new Response(null, { status: 200, headers: res.headers }), - ), // 304 responses themselves are not cacheable, so just pretend to get the refreshed TTL + ), // 304 responses are not cacheable, so just use its headers to get the refreshed TTL ); const expires = policy.storable() ? policy.timeToLive() : 0; return { - data, + data: res.ok ? data : null, expires: Date.now() + expires, // While servers should respond with the same headers as a 200 response, if they don't we should reuse the stored value etag: res.headers.get('Etag') ?? (res.ok ? undefined : revalidationData.etag), diff --git a/packages/astro/test/units/assets/remote.test.js b/packages/astro/test/units/assets/remote.test.js new file mode 100644 index 000000000000..0688a0abd140 --- /dev/null +++ b/packages/astro/test/units/assets/remote.test.js @@ -0,0 +1,85 @@ +// @ts-check +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { revalidateRemoteImage } from '../../../dist/assets/build/remote.js'; + +/** + * + * @param {number} status + * @param {Record} headerInit + * @param {ArrayBuffer} body + * @returns {() => Promise} + */ +function makeFetchMock(status, headerInit = {}, body = new ArrayBuffer(0)) { + const headers = new Headers(headerInit); + return async () => + /** @type {Response} */ ( + /** @type {unknown} */ ({ + status, + ok: status >= 200 && status < 300, + headers, + arrayBuffer: async () => body, + }) + ); +} + +describe('revalidateRemoteImage', () => { + it('returns null data on 304 Not Modified (cache still valid)', async () => { + const result = await revalidateRemoteImage( + 'https://example.com/img.jpg', + { etag: '"abc123"' }, + makeFetchMock(304, { 'Cache-Control': 'max-age=3600', Etag: '"abc123"' }), + ); + + // null signals "cache is still valid, use the file on disk" + assert.equal(result.data, null); + // etag should be preserved from the stored revalidation data + assert.equal(result.etag, '"abc123"'); + // expires should be a future timestamp + assert.ok(result.expires > Date.now()); + }); + + it('preserves lastModified from stored data on 304 when server omits the header', async () => { + const result = await revalidateRemoteImage( + 'https://example.com/img.jpg', + { lastModified: 'Wed, 01 Jan 2025 00:00:00 GMT' }, + makeFetchMock(304, { 'Cache-Control': 'max-age=600' }), + ); + + assert.equal(result.data, null); + assert.equal(result.lastModified, 'Wed, 01 Jan 2025 00:00:00 GMT'); + }); + + it('returns image data on 200 OK (cache was stale)', async () => { + const imageBytes = new Uint8Array([1, 2, 3, 4]).buffer; + const result = await revalidateRemoteImage( + 'https://example.com/img.jpg', + { etag: '"oldtag"' }, + makeFetchMock(200, { 'Cache-Control': 'max-age=3600', Etag: '"newetag"' }, imageBytes), + ); + + assert.ok(result.data !== null); + assert.ok(result.data.length > 0); + assert.equal(result.etag, '"newetag"'); + }); + + it('throws on redirect responses (e.g. 301)', async () => { + await assert.rejects( + () => + revalidateRemoteImage( + 'https://example.com/img.jpg', + { etag: '"abc"' }, + makeFetchMock(301, { Location: 'https://example.com/new.jpg' }), + ), + /redirected/, + ); + }); + + it('throws on server error responses (e.g. 500)', async () => { + await assert.rejects( + () => + revalidateRemoteImage('https://example.com/img.jpg', { etag: '"abc"' }, makeFetchMock(500)), + /500/, + ); + }); +});