🐛 Fixed email rendering to use cached image dimensions (#26295)

ref HKG-1583

- Email rendering was bypassing the image dimension cache — it used the
raw `ImageSize.getImageSizeFromUrl()` which makes a fresh HTTP
request/probe on every call, even though the frontend metadata path
already cached these via `CachedImageSizeFromUrl`
- Updated the caching logic into the existing
`getCachedImageSizeFromUrl()` — it now returns shallow copies to prevent
cache corruption when consumers mutate dimensions in-place, only caches
404s permanently with a `notFound` marker so they can be distinguished
from stale transient errors (transient errors are not cached, allowing
retry), and retries fetch when the cache holds a stale error entry
without dimensions
- Wired the email renderer to use `cachedImageSizeFromUrl` directly as
the `imageSize` dependency, replacing the uncached `ImageSize` instance
- Added a guard in `limitImageWidth` for missing dimensions — since
`getCachedImageSizeFromUrl` never throws (returns `{url}` on error), it
checks `!size.width` and returns defaults, so the renderer gracefully
handles failed fetches without a behavior change
- The other consumer — `image-dimensions.js` (frontend metadata) — did
not need any changes since it already calls `getCachedImageSizeFromUrl`
and handles missing dimensions gracefully. The shallow copy fix and
improved caching strategy benefit it automatically
- Updated unit tests for `getCachedImageSizeFromUrl` (transient errors
not cached, 404s cached, stale entry retry, cache corruption prevention)
and added tests verifying the email renderer wiring end-to-end (cache
hit, cache miss with write-back, stale error entry retry, fetch failure
fallback)

Author: vershwal

ghost/core/core/server/lib/image/cached-image-size-from-url.js

@@ -14,6 +14,7 @@ const logging = require('@tryghost/logging');
  * @property {string} url image url
  * @property {number} height image height
  * @property {number} width image width
+ * @property {boolean} notFound true if the image is not found
  */
 
 class CachedImageSizeFromUrl {
@@ -30,46 +31,57 @@ class CachedImageSizeFromUrl {
 
     /**
      * Get cached image size from URL
-     * Always returns {object} imageSizeCache
+     * Returns null when dimensions are unavailable (invalid URL, 404, transient
+     * errors) so consumers can gracefully skip images with missing dimensions.
+     *
+     * Caching strategy:
+     * - Successful fetches are cached
+     * - NotFoundError (404) is cached permanently with a marker
+     * - Transient errors (timeouts, 500s) are NOT cached, allowing retry on next call
+     * - Stale error entries (cached without dimensions) trigger a retry
+     *
      * @param {string} url
      * @returns {Promise<ImageSizeCache>}
-     * @description Takes a url and returns image width and height from cache if available.
-     * If not in cache, `getImageSizeFromUrl` is called and returns the dimensions in a Promise.
      */
     async getCachedImageSizeFromUrl(url) {
         if (!url || url === undefined || url === null) {
-            return;
+            return null;
         }
 
         const cachedImageSize = await this.cache.get(url);
 
-        if (cachedImageSize) {
+        // Check for cachedImageSize.width to handle legacy cache entries
+        // that were stored as {url} without dimensions or a notFound marker.
+        // These stale entries fall through to trigger a re-fetch and self-heal.
+        if (cachedImageSize && cachedImageSize.width) {
             debug('Read image from cache:', url);
+            return {...cachedImageSize};
+        }
 
-            return cachedImageSize;
-        } else {
-            try {
-                const res = await this.getImageSizeFromUrl(url);
-                await this.cache.set(url, res);
-
-                debug('Cached image:', url);
+        // 404s are cached permanently — don't retry
+        if (cachedImageSize && cachedImageSize.notFound) {
+            debug('Read image from cache (not found):', url);
+            return null;
+        }
 
-                return this.cache.get(url);
-            } catch (err) {
-                if (err instanceof errors.NotFoundError) {
-                    debug('Cached image (not found):', url);
-                } else {
-                    debug('Cached image (error):', url);
-                    logging.error(err);
-                }
+        try {
+            const res = await this.getImageSizeFromUrl(url);
+            await this.cache.set(url, {...res});
 
-                // in case of error we just attach the url
-                await this.cache.set(url, {
-                    url
-                });
+            debug('Cached image:', url);
 
-                return this.cache.get(url);
+            return res;
+        } catch (err) {
+            if (err instanceof errors.NotFoundError) {
+                debug('Cached image (not found):', url);
+                // Cache 404s with a marker
+                await this.cache.set(url, {url, notFound: true});
+            } else {
+                debug('Image fetch error (not cached):', url);
+                logging.error(err);
             }
+
+            return null;
         }
     }
 }

ghost/core/core/server/services/email-service/email-renderer.js

@@ -148,7 +148,7 @@ class EmailRenderer {
      * @param {object} dependencies.renderers
      * @param {{render(object, options): string}} dependencies.renderers.lexical
      * @param {{render(object, options): string}} dependencies.renderers.mobiledoc
-     * @param {{getImageSizeFromUrl(url: string): Promise<{width: number, height: number}>}} dependencies.imageSize
+     * @param {{getCachedImageSizeFromUrl(url: string): Promise<{url: string, width: number, height: number} | null>}} dependencies.imageSize
      * @param {{urlFor(type: string, optionsOrAbsolute, absolute): string, isSiteUrl(url, context): boolean}} dependencies.urlUtils
      * @param {{isLocalImage(url: string): boolean}} dependencies.storageUtils
      * @param {(post: Post) => string} dependencies.getPostUrl
@@ -1402,7 +1402,11 @@ class EmailRenderer {
             };
         } else {
             try {
-                const size = await this.#imageSize.getImageSizeFromUrl(href);
+                const size = await this.#imageSize.getCachedImageSizeFromUrl(href);
+
+                if (!size || !size.width) {
+                    return {href, width: 0, height: null};
+                }
 
                 if (size.width >= visibleWidth) {
                     if (!visibleHeight) {

ghost/core/core/server/services/email-service/email-service-wrapper.js

@@ -47,7 +47,7 @@ class EmailServiceWrapper {
         const audienceFeedback = require('../audience-feedback');
         const storageUtils = require('../../adapters/storage/utils');
         const emailAnalyticsJobs = require('../email-analytics/jobs');
-        const {imageSize} = require('../../lib/image');
+        const {cachedImageSizeFromUrl} = require('../../lib/image');
 
         // capture errors from mailgun client and log them in sentry
         const errorHandler = (error) => {
@@ -79,7 +79,7 @@ class EmailServiceWrapper {
                 mobiledoc: mobiledocLib,
                 lexical: lexicalLib
             },
-            imageSize,
+            imageSize: cachedImageSizeFromUrl,
             urlUtils,
             storageUtils,
             getPostUrl: this.getPostUrl,

ghost/core/test/unit/frontend/helpers/ghost-head.test.js

@@ -378,7 +378,7 @@ describe('{{ghost_head}} helper', function () {
 
         // @TODO: this is a LOT of mocking :/
         routingRegistryGetRssUrlStub = sinon.stub(routing.registry, 'getRssUrl').returns('http://localhost:65530/rss/');
-        sinon.stub(imageLib.imageSize, 'getImageSizeFromUrl').resolves();
+        sinon.stub(imageLib.cachedImageSizeFromUrl, 'getCachedImageSizeFromUrl').resolves();
         getStub = sinon.stub(settingsCache, 'get');
 
         getStub.withArgs('title').returns('Ghost');
@@ -418,7 +418,7 @@ describe('{{ghost_head}} helper', function () {
                     safeVersion: '0.3'
                 }
             }));
-            sinon.assert.calledOnce(loggingErrorStub);
+            sinon.assert.notCalled(loggingErrorStub);
         });
 
         it('returns structured data on first index page', async function () {

ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js

@@ -62,7 +62,7 @@ describe('lib/image: image size cache', function () {
         assert.equal(image2.height, 50);
     });
 
-    it('can handle generic image-size errors', async function () {
+    it('should not cache transient errors, allowing retry on next call', async function () {
         const url = 'http://mysite.com/content/image/mypostcoverimage.jpg';
 
         sizeOfStub.rejects('error');
@@ -74,17 +74,26 @@ describe('lib/image: image size cache', function () {
         });
 
         const loggingStub = sinon.stub(logging, 'error');
-        await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        const result = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
 
-        cacheStore.get(url).should.not.be.undefined;
-        const image = cacheStore.get(url);
-        assert.equal(image.url, 'http://mysite.com/content/image/mypostcoverimage.jpg');
-        assert.equal(image.width, undefined);
-        assert.equal(image.height, undefined);
+        assert.equal(result, null);
+
+        // Transient errors should NOT be cached
+        assert.equal(cacheStore.get(url), undefined);
         sinon.assert.calledOnce(loggingStub);
+        assert.equal(sizeOfStub.calledOnce, true);
+
+        // Second call should retry the fetch since nothing was cached
+        const result2 = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+
+        assert.equal(result2, null);
+
+        // Cache should still be empty after the second transient error
+        assert.equal(cacheStore.get(url), undefined);
+        assert.equal(sizeOfStub.calledTwice, true);
     });
 
-    it('can handle NotFoundError error', async function () {
+    it('should cache NotFoundError permanently and not refetch on subsequent calls', async function () {
         const url = 'http://mysite.com/content/image/mypostcoverimage.jpg';
 
         sizeOfStub.rejects(new errors.NotFoundError('it iz gone mate!'));
@@ -95,13 +104,71 @@ describe('lib/image: image size cache', function () {
             cache: cacheStore
         });
 
-        await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        const result = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        assert.equal(result, null);
 
+        // Verify 404 was cached with notFound marker
         cacheStore.get(url).should.not.be.undefined;
         const image = cacheStore.get(url);
-        assert.equal(image.url, 'http://mysite.com/content/image/mypostcoverimage.jpg');
-        assert.equal(image.width, undefined);
-        assert.equal(image.height, undefined);
+        assert.equal(image.url, url);
+        assert.equal(image.notFound, true);
+
+        // Second call should NOT refetch — 404 is permanent
+        const secondResult = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        assert.equal(secondResult, null);
+        assert.equal(sizeOfStub.calledOnce, true);
+    });
+
+    it('should retry fetch when cache has a stale error entry (no dimensions)', async function () {
+        const url = 'http://mysite.com/content/image/photo.jpg';
+
+        sizeOfStub.resolves({width: 500, height: 400, type: 'jpg'});
+
+        const cacheStore = new InMemoryCache();
+        // Pre-populate cache with an error entry (no width/height)
+        cacheStore.set(url, {url});
+
+        const cachedImageSizeFromUrl = new CachedImageSizeFromUrl({
+            getImageSizeFromUrl: sizeOfStub,
+            cache: cacheStore
+        });
+
+        const result = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+
+        assert.equal(result.width, 500);
+        assert.equal(result.height, 400);
+        assert.equal(sizeOfStub.calledOnce, true);
+
+        // Verify cache was overwritten with valid dimensions
+        const cached = cacheStore.get(url);
+        assert.equal(cached.width, 500);
+        assert.equal(cached.height, 400);
+    });
+
+    it('should not corrupt cache when caller mutates the returned object', async function () {
+        const url = 'http://mysite.com/content/image/mypostcoverimage.jpg';
+
+        sizeOfStub.resolves({width: 2000, height: 1000, type: 'jpg'});
+
+        const cacheStore = new InMemoryCache();
+        const cachedImageSizeFromUrl = new CachedImageSizeFromUrl({
+            getImageSizeFromUrl: sizeOfStub,
+            cache: cacheStore
+        });
+
+        const result = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        result.width = 1200;
+        result.height = 600;
+
+        // Cache should still hold the original dimensions
+        const cached = cacheStore.get(url);
+        assert.equal(cached.width, 2000);
+        assert.equal(cached.height, 1000);
+
+        // A subsequent call should also return original dimensions
+        const secondResult = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
+        assert.equal(secondResult.width, 2000);
+        assert.equal(secondResult.height, 1000);
     });
 
     it('should return null if url is null', async function () {
@@ -114,6 +181,6 @@ describe('lib/image: image size cache', function () {
 
         result = await cachedImageSizeFromUrl.getCachedImageSizeFromUrl(url);
 
-        assert.equal(result, undefined);
+        assert.equal(result, null);
     });
 });