fix(images): probe and clean up preview tiers under the extension the encoder actually wrote (#1355)

generatePreviewImage rewrites the output extension to match the encoding it chose, .jpg or .webp for alpha and multi-frame sources. The tier lookup in ensurePreviewImageAtWidth and the cleanup list in previewTierKeys kept the SOURCE extension instead, so for anything but a lowercase .jpg source the stat never matched: every tier request for a .png, .JPG, .jpeg, .heic or RAW photo re-ran Sharp, and cleanup never found the files it left behind, which accumulated for the life of the install. Both now derive every key the tier can live under: the .jpg and .webp candidates, plus the source-extension key last so tiers written before the rewrite are still found by lookup and by cleanup. Follow-up to issue 1020, where the mismatch was identified during review. Co-authored-by: Paul Nothaft <[email protected]>
2026-09-08 08:41:53 +02:00
parent c97341e454
commit acb25a9a1c
3 changed files with 208 additions and 20 deletions
@@ -129,8 +129,15 @@ describe('preview tiers (#1095)', () => {
    it('derives every non-default tier key for cleanup', () => {
      // Tiers live outside preview_path, so delete/archive/regenerate have no
      // other way to find them. 1920 is excluded because that IS preview_path.
+      // Two candidates per width — the encoder picks `.jpg` or `.webp` and the
+      // cleanup list cannot know which without probing the source.
      const keys = imageProcessor.previewTierKeys({ id: 5, path: 'e/a.jpg', source_origin: 'managed' });
-      expect(keys).toHaveLength(imageProcessor.PREVIEW_WIDTHS.length - 1);
+      const widths = imageProcessor.PREVIEW_WIDTHS.filter((w) => w !== 1920);
+      expect(keys).toHaveLength(widths.length * 2);
+      for (const w of widths) {
+        expect(keys).toContain(`previews/preview_w${w}_p5_a.jpg`);
+        expect(keys).toContain(`previews/preview_w${w}_p5_a.webp`);
+      }
      expect(keys.some((k) => k.includes('w1920'))).toBe(false);
      expect(keys.every((k) => k.includes('p5_'))).toBe(true);
    });
@@ -0,0 +1,148 @@
+/**
+ * Preview tier lookup and cleanup must agree with what the generator writes
+ * (#1020 follow-up).
+ *
+ * generatePreviewImage rewrites the extension to match the encoding (`.jpg`,
+ * or `.webp` for alpha / multi-frame sources). ensurePreviewImageAtWidth and
+ * previewTierKeys kept the SOURCE extension, so for a `.png`, `.JPG`, `.heic`
+ * or RAW source the tier was generated on every single request — the stat
+ * never matched — and cleanup never found the files, which piled up in
+ * storage for the life of the install.
+ *
+ * Driven against real Sharp output, because the whole question is which
+ * extension the encoder actually chose.
+ */
+const path = require('path');
+const fs = require('fs').promises;
+const os = require('os');
+const sharp = require('sharp');
+
+jest.mock('../../src/database/db', () => {
+  const state = { event: null };
+  const api = (table) => {
+    if (table === 'events') return { where: () => ({ first: async () => state.event }) };
+    if (table === 'photos') return { where: () => ({ update: async () => 1 }) };
+    if (table === 'app_settings') return { whereIn: () => ({ select: async () => [] }) };
+    throw new Error(`unexpected table in test: ${table}`);
+  };
+  api.__state = state;
+  return { db: api };
+});
+
+const LocalFsStorage = require('../../src/services/storage/LocalFsStorage');
+const storageModule = require('../../src/services/storage');
+const { db } = require('../../src/database/db');
+
+const EVENT = { id: 21, slug: 'fmt-ev', source_mode: 'managed' };
+let nextId = 5000;
+
+describe('preview tier keys follow the encoded extension', () => {
+  let storage; let storageRoot; let imageProcessor; let puts;
+
+  beforeAll(async () => {
+    storageRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-tierkeys-'));
+    storage = new LocalFsStorage({ root: storageRoot });
+    await storage.init();
+    const origPut = storage.put.bind(storage);
+    storage.put = async (key, ...rest) => { puts.push(key); return origPut(key, ...rest); };
+    storageModule.setStorageForTesting(storage);
+    delete require.cache[require.resolve('../../src/services/imageProcessor')];
+    imageProcessor = require('../../src/services/imageProcessor');
+  }, 30000);
+
+  beforeEach(() => { puts = []; db.__state.event = EVENT; });
+
+  afterAll(async () => {
+    storageModule.resetStorage();
+    await fs.rm(storageRoot, { recursive: true, force: true }).catch(() => {});
+  });
+
+  async function photoWith(name, { alpha = false } = {}) {
+    const id = nextId++;
+    const rel = `${EVENT.slug}/${name}`;
+    const abs = path.join(storageRoot, 'events/active', rel);
+    await fs.mkdir(path.dirname(abs), { recursive: true });
+    const pipeline = sharp({
+      create: {
+        width: 2000, height: 1400, channels: alpha ? 4 : 3,
+        background: alpha ? { r: 10, g: 20, b: 30, alpha: 0.5 } : { r: 10, g: 20, b: 30 },
+      },
+    });
+    if (/\.png$/i.test(name)) await pipeline.png().toFile(abs);
+    else if (/\.webp$/i.test(name)) await pipeline.webp().toFile(abs);
+    else await pipeline.jpeg().toFile(abs);
+    return { id, event_id: EVENT.id, source_origin: 'managed', path: rel, filename: name };
+  }
+
+  it.each([
+    ['opaque PNG', 'photo.png', false, '.jpg'],
+    ['transparent PNG', 'photo-alpha.png', true, '.webp'],
+    ['uppercase JPG', 'IMG_0001.JPG', false, '.jpg'],
+    ['jpeg spelled out', 'photo.jpeg', false, '.jpg'],
+    ['lowercase jpg', 'photo.jpg', false, '.jpg'],
+  ])('%s: the second request is a cache hit, not a second generation', async (_label, name, alpha, ext) => {
+    const photo = await photoWith(name, { alpha });
+
+    const first = await imageProcessor.ensurePreviewImageAtWidth(photo, 640);
+    expect(first).toBe(`previews/preview_w640_p${photo.id}_${name.replace(/\.[^.]+$/, '')}${ext}`);
+    expect(puts).toEqual([first]);
+
+    const second = await imageProcessor.ensurePreviewImageAtWidth(photo, 640);
+    expect(second).toBe(first);
+    expect(puts).toHaveLength(1);
+  });
+
+  it.each([
+    ['opaque PNG', 'cleanup.png', false],
+    ['transparent PNG', 'cleanup-alpha.png', true],
+    ['uppercase JPG', 'CLEANUP.JPG', false],
+  ])('%s: previewTierKeys covers the generated key, so deletePreviewTiers removes it', async (_label, name, alpha) => {
+    const photo = await photoWith(name, { alpha });
+    const k640 = await imageProcessor.ensurePreviewImageAtWidth(photo, 640);
+    const k1280 = await imageProcessor.ensurePreviewImageAtWidth(photo, 1280);
+    expect(await storage.stat(k640)).toBeTruthy();
+    expect(await storage.stat(k1280)).toBeTruthy();
+
+    const keys = imageProcessor.previewTierKeys(photo);
+    expect(keys).toEqual(expect.arrayContaining([k640, k1280]));
+
+    await imageProcessor.deletePreviewTiers(photo);
+    expect(await storage.stat(k640)).toBeNull();
+    expect(await storage.stat(k1280)).toBeNull();
+  });
+
+  it('a tier written before the extension rewrite is still found, by lookup and by cleanup', async () => {
+    // JPEG bytes under the source's `.png` name — what generatePreviewImage
+    // produced before it started rewriting the extension. Served as JPEG by
+    // the route (the Content-Type comes from the `.webp` suffix only).
+    const photo = await photoWith('legacy.png');
+    const legacyKey = `previews/preview_w640_p${photo.id}_legacy.png`;
+    await storage.put(legacyKey, await sharp({ create: { width: 640, height: 448, channels: 3, background: '#123' } }).jpeg().toBuffer());
+    puts = [];
+
+    expect(await imageProcessor.ensurePreviewImageAtWidth(photo, 640)).toBe(legacyKey);
+    expect(puts).toHaveLength(0);
+
+    expect(imageProcessor.previewTierKeys(photo)).toContain(legacyKey);
+    await imageProcessor.deletePreviewTiers(photo);
+    expect(await storage.stat(legacyKey)).toBeNull();
+  });
+
+  it('never lists the canonical 1920 rendition, which preview_path owns', () => {
+    const keys = imageProcessor.previewTierKeys({ id: 9, path: 'e/a.png', source_origin: 'managed' });
+    expect(keys.some((k) => k.includes('w1920'))).toBe(false);
+    expect(keys.every((k) => k.includes('p9_'))).toBe(true);
+    // Both encodings plus the legacy source-extension key, per width.
+    expect(keys).toEqual([
+      'previews/preview_w640_p9_a.jpg', 'previews/preview_w640_p9_a.webp', 'previews/preview_w640_p9_a.png',
+      'previews/preview_w1280_p9_a.jpg', 'previews/preview_w1280_p9_a.webp', 'previews/preview_w1280_p9_a.png',
+    ]);
+  });
+
+  it('does not duplicate the legacy key when the source already is a lowercase .jpg', () => {
+    const keys = imageProcessor.previewTierKeys({ id: 9, path: 'e/a.jpg', source_origin: 'managed' });
+    expect(keys.filter((k) => k.includes('w640'))).toEqual([
+      'previews/preview_w640_p9_a.jpg', 'previews/preview_w640_p9_a.webp',
+    ]);
+  });
+});
@@ -1021,14 +1021,53 @@ async function isPreviewValid(previewPath) {
 */
 function previewTierKeys(photo) {
  if (!photo) return [];
+  return PREVIEW_WIDTHS
+    .filter((w) => w !== DEFAULT_PREVIEW_LONG_EDGE)
+    .flatMap((w) => previewTierKeyCandidates(photo, w));
+}
+
+/**
+ * The output basename every preview tier of a photo is written under.
+ *
+ * ALWAYS scoped by photo id, managed rows included. Basenames are not unique
+ * across events — two galleries can each hold an IMG_0001.jpg — and because a
+ * tier is served straight from a cache hit without re-reading the source, a
+ * collision hands one gallery's photo to another. Scoping by id is what makes
+ * the cache safe to trust; it is not a tidiness choice.
+ */
+function previewTierBasename(photo) {
  const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
  const sourceBasename = path.basename(
    (isExternal ? (photo.external_relpath || photo.filename) : photo.path) || `photo-${photo.id}`
  );
-  const outputBasename = `p${photo.id}_${sourceBasename}`;
-  return PREVIEW_WIDTHS
-    .filter((w) => w !== DEFAULT_PREVIEW_LONG_EDGE)
-    .map((w) => path.posix.join('previews', `preview_w${w}_${outputBasename}`));
+  return `p${photo.id}_${sourceBasename}`;
+}
+
+/**
+ * Every storage key one preview tier of a photo can live under, most likely
+ * first.
+ *
+ * generatePreviewImage rewrites the extension to match the encoding it chose
+ * — `.jpg`, or `.webp` for a source with alpha or more than one frame — and
+ * which one that is cannot be known without probing the source, which is the
+ * work the cache exists to skip. The lookup used to probe a single key that
+ * kept the SOURCE extension, so for anything but a lowercase `.jpg` source
+ * (`.png`, `.JPG`, `.heic`, RAW) it never matched what had been written: every
+ * tier request re-ran Sharp, and cleanup, deriving the same key, never found
+ * the files it left behind.
+ *
+ * The source-extension key stays in the list, last: previews written before
+ * the extension rewrite carry it (JPEG bytes under a `.png` name, still
+ * served as JPEG), and they have to be found by cleanup as well as lookup.
+ */
+function previewTierKeyCandidates(photo, width) {
+  const outputBasename = previewTierBasename(photo);
+  const stem = `preview_w${width}_`;
+  const base = outputBasename.replace(/\.[^./\\]+$/, '');
+  const keys = [`${stem}${base}.jpg`, `${stem}${base}.webp`];
+  const legacy = `${stem}${outputBasename}`;
+  if (!keys.includes(legacy)) keys.push(legacy);
+  return keys.map((k) => path.posix.join('previews', k));
 }

 /** Best-effort removal of every responsive tier for a photo. */
@@ -1196,23 +1235,17 @@ async function ensurePreviewImageAtWidthUnguarded(photo, width) {
  if (!event) return null;

  const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
-  const sourceBasename = path.basename(
-    (isExternal ? (photo.external_relpath || photo.filename) : photo.path) || `photo-${photo.id}`
-  );
-  // ALWAYS scoped by photo id, managed rows included. Basenames are not unique
-  // across events — two galleries can each hold an IMG_0001.jpg — and because a
-  // tier is served straight from a cache hit without re-reading the source, a
-  // collision hands one gallery's photo to another. Scoping by id is what makes
-  // the cache safe to trust; it is not a tidiness choice.
-  const outputBasename = `p${photo.id}_${sourceBasename}`;
-  const key = path.posix.join('previews', `preview_w${width}_${outputBasename}`);
+  const outputBasename = previewTierBasename(photo);

  // Cache hit: nothing to do. This is the common path once a gallery has been
-  // browsed at a given size.
-  try {
-    if (await storage.stat(key)) return key;
-  } catch (e) {
-    // fall through and regenerate
+  // browsed at a given size. Every key the tier can have been written under
+  // is probed — see previewTierKeyCandidates for why there is more than one.
+  for (const key of previewTierKeyCandidates(photo, width)) {
+    try {
+      if (await storage.stat(key)) return key;
+    } catch (e) {
+      // fall through to the next candidate, then regenerate
+    }
  }

  try {