feat(assets): Use entity-tags to revalidate cached remote images #12426

oliverlynch · 2024-11-14T07:56:41Z

Changes

Store the entity tag of cached remote images, and use them to revalidate the cache when it goes stale to prevent re-downloading. Improves build time and bandwidth usage for sites with stale cached assets.

Build with fully stale cache:

Build with fully stale cache and etag revalidation:

Testing

Tested with the astro base template with a single remote image with 3 densities added, and successfully ran pnpm --filter astro run test.

Docs

Current caching behaviour does not seem to be documented, so no documentation to update. This being said I think a new section in the astro asset docs outlining the behaviour of the asset cache would be useful.

…load

changeset-bot · 2024-11-14T07:56:48Z

🦋 Changeset detected

Latest commit: c63f48d

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

packages/astro/src/assets/build/generate.ts

ematipico · 2024-11-18T09:33:50Z

packages/astro/src/assets/build/generate.ts

+		JSON.stringify({
+			data: Buffer.from(resultData.data).toString('base64'),
+			expires: resultData.expires,
+			etag: resultData.etag,
+		}),


JSON.stringify and Buffer.from can be both fail at runtime, we should handle the errors somehow

I've wrapped this in a try catch block which will print a warning and no-op, rather than throwing an Error. The asset should just skip the cache in this case.

packages/astro/src/assets/build/remote.ts

ematipico · 2024-11-18T09:37:57Z

packages/astro/src/assets/build/remote.ts

+
+	if (!res.ok && res.status != 304) {
+		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}))`,


This error isn't actionable. If a user sees that, they don't know what to do in order to fix it. We should provide a better error that gives the user an action.

I agree that this is a bad error, however this is handled the same way in the existing loadRemoteImage function I based the revalidateRemoteImage function off of, which provides similarly un-actionable errors.

I can potentially handle common cases like 404 with custom messages although there are many status codes where it would be difficult to give any actionable advice, like 5xx errors.

Thank you for providing more context. Looking at the current use case, we don't need to throw an error because we are attempting to revalidate the image, so I assume we already have the image.

Maybe we could log a warning instead—for any non-200 status code—and inform the user that Astro couldn't revalidate the image and will use the existing one (we could add more info e.g. status code). What do you think?

Yeah a warning would be a better solution. I've added the status text (e.g. NOT FOUND/FORBIDDEN) to the error, which should make it more understandable without needing to look up the status code first.

I've added a try catch block in generate.ts to handle this error and errors from Request itself, which should fall back to using the stale cache and print a warning in the log.

Example warning:

[WARN] An error was encountered while revalidating a cached remote asset. Proceeding with stale cache. Error: Failed to revalidate cached remote image https://<domain>/test.jpg. The request did not return a 200 OK / 304 NOT MODIFIED response. (received 403 Forbidden) ▶ /_astro/test_Z2qHSPG.webp (reused cache entry) (+295ms) (31/33)

codspeed-hq · 2024-11-19T07:51:07Z

CodSpeed Performance Report

Merging #12426 will not alter performance

_{Comparing oliverlynch:remote-assets-use-etag (c63f48d) with main (b140a3f)}

Summary

✅ 6 untouched benchmarks

packages/astro/src/assets/build/remote.ts

ematipico · 2024-11-19T13:28:41Z

packages/astro/src/assets/build/remote.ts

+
+	if (!res.ok && res.status != 304) {
+		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}))`,


Thank you for providing more context. Looking at the current use case, we don't need to throw an error because we are attempting to revalidate the image, so I assume we already have the image.

Maybe we could log a warning instead—for any non-200 status code—and inform the user that Astro couldn't revalidate the image and will use the existing one (we could add more info e.g. status code). What do you think?

packages/astro/src/assets/build/remote.ts

.changeset/red-poems-pay.md

ascorbic · 2024-11-21T10:33:48Z

packages/astro/src/assets/build/remote.ts

+ * @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.
+ */
+export async function revalidateRemoteImage(src: string, etag: string) {
+	const req = new Request(src, { headers: { 'If-None-Match': etag } });


It would be worth also supporting If-Modified-Since, and storing the Last-Modified or Date for image responses that don't include an etag.

ascorbic · 2024-11-21T10:37:01Z

packages/astro/src/assets/build/generate.ts

+		return await fs.promises.writeFile(
+			cachedFileURL,
+			JSON.stringify({
+				data: Buffer.from(resultData.data).toString('base64'),


I realise that this is what we were already doing, but it does seem a bit wasteful to be saving images as JSON-encoded base64 strings, and this could be a good time to fix it. I think it would be better to store the image in a binary, and then use a separate JSON file for metadata, probably with the same filename alongside it but with something like an appended .json

Princesseuh · 2024-11-21T11:37:03Z

packages/astro/src/assets/build/generate.ts

-		const remoteImage = await loadRemoteImage(path);
-		return {
-			data: remoteImage.data,
-			expires: remoteImage.expires,
-		};


I have no idea what happened here, but thank you for cleaning it, ha

Princesseuh

I didn't try locally or anything, but the logic makes sense to me in the code. I agree with Matt's suggestions, but I'd be okay with those to be tackled separately personally.

oliverlynch · 2024-11-21T12:50:01Z

I agree, the base64 encoding does seem inefficient. I would be happy to implement the sidecar file approach but I think it would be better off as a separate PR.

I would also be happy to implement If-Modified-Since, either in this PR or a separate one. The main changes to revalidate assets are written already so adding another header shouldn't be too much effort.

ascorbic · 2024-11-21T13:07:38Z

I think the best bet is to add the if-modified to this one as it's related and not much more code, and then you or someone else can follow up later with the other change

oliverlynch · 2024-11-25T08:33:04Z

Added revalidation with If-Modified-Since, and also a tweak to revalidateRemoteImage to use the stored etag/last-modified header if the server returns 304 Not Modified but does not include the headers.

oliverlynch added 4 commits November 11, 2024 17:57

feat(assets): Store etag to refresh cached images without a full down…

2e2fc9d

…load

Merge branch 'withastro:main' into remote-assets-use-etag

e844a93

Seperate loading and revalidating functions

6955f3f

Add changeset

9981da8

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 14, 2024

Merge branch 'main' into remote-assets-use-etag

83d007d

github-actions bot added the semver: minor Change triggers a `minor` release label Nov 18, 2024

github-actions bot requested changes Nov 18, 2024

View reviewed changes

ematipico reviewed Nov 18, 2024

View reviewed changes

Merge branch 'withastro:main' into remote-assets-use-etag

c3e60f3

Updates based on requested changes

f7a614a

ematipico reviewed Nov 19, 2024

View reviewed changes

Wording changes, use stale cache on failure to revalidate

2ebfd63

ascorbic reviewed Nov 21, 2024

View reviewed changes

Princesseuh reviewed Nov 21, 2024

View reviewed changes

Princesseuh approved these changes Nov 21, 2024

View reviewed changes

oliverlynch added 2 commits November 25, 2024 15:53

Merge branch 'withastro:main' into remote-assets-use-etag

a806f5c

Add If-Modified-Since as cache revalidation method

c63f48d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(assets): Use entity-tags to revalidate cached remote images #12426

feat(assets): Use entity-tags to revalidate cached remote images #12426

oliverlynch commented Nov 14, 2024

changeset-bot bot commented Nov 14, 2024 •

edited

Loading

github-actions bot left a comment

ematipico Nov 18, 2024

oliverlynch Nov 19, 2024

ematipico Nov 18, 2024

oliverlynch Nov 19, 2024

ematipico Nov 19, 2024

oliverlynch Nov 20, 2024 •

edited

Loading

codspeed-hq bot commented Nov 19, 2024 •

edited

Loading

ematipico Nov 19, 2024

ascorbic Nov 21, 2024

ascorbic Nov 21, 2024 •

edited

Loading

Princesseuh Nov 21, 2024

Princesseuh left a comment

oliverlynch commented Nov 21, 2024

ascorbic commented Nov 21, 2024

oliverlynch commented Nov 25, 2024

feat(assets): Use entity-tags to revalidate cached remote images #12426

Are you sure you want to change the base?

feat(assets): Use entity-tags to revalidate cached remote images #12426

Conversation

oliverlynch commented Nov 14, 2024

Changes

Testing

Docs

changeset-bot bot commented Nov 14, 2024 • edited Loading

🦋 Changeset detected

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverlynch Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

codspeed-hq bot commented Nov 19, 2024 • edited Loading

Merging #12426 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascorbic Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Princesseuh left a comment

Choose a reason for hiding this comment

oliverlynch commented Nov 21, 2024

ascorbic commented Nov 21, 2024

oliverlynch commented Nov 25, 2024

changeset-bot bot commented Nov 14, 2024 •

edited

Loading

oliverlynch Nov 20, 2024 •

edited

Loading

codspeed-hq bot commented Nov 19, 2024 •

edited

Loading

ascorbic Nov 21, 2024 •

edited

Loading