fix(assets): allow 304 Not Modified status to bypass redirect check while revalidating cached assets

.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.

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(

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,20 +70,20 @@ 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
 	const policy = new CachePolicy(
 		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),

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<string, string>} headerInit 
+ * @param {ArrayBuffer} body 
+ * @returns {() => Promise<Response>}
+ */
+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/,
+		);
+	});
+});