diff --git a/backend/__tests__/integration/adminThumbnails.regenerate.test.js b/backend/__tests__/integration/adminThumbnails.regenerate.test.js index 7a9f6e6e..012f69b0 100644 --- a/backend/__tests__/integration/adminThumbnails.regenerate.test.js +++ b/backend/__tests__/integration/adminThumbnails.regenerate.test.js @@ -23,7 +23,7 @@ const express = require('express'); const request = require('supertest'); describe('admin thumbnail regeneration (#1129)', () => { - let tmpDir; let db; let cleanup; let app; let imageProcessor; let storage; + let tmpDir; let db; let cleanup; let app; let imageProcessor; let storage; let logInfo; beforeAll(async () => { tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-regen-')); @@ -54,6 +54,10 @@ describe('admin thumbnail regeneration (#1129)', () => { deletePreviewTiers: jest.fn().mockResolvedValue(undefined), })); + // Same module registry as the route, so the spy sees its calls. The + // completion line is what drain() below waits for. + logInfo = jest.spyOn(require('../../src/utils/logger'), 'info'); + // bootCrmDb, not run-migrations: the latter calls process.exit(0) on // success, which ends the jest worker mid-suite. ({ db, cleanup } = await require('./helpers/crmDb').bootCrmDb()); @@ -94,8 +98,20 @@ describe('admin thumbnail regeneration (#1129)', () => { return typeof row === 'object' ? row.id : row; } - /** The work runs in setImmediate; give it room to finish. */ - const drain = () => new Promise((resolve) => setTimeout(resolve, 150)); + /** + * The work runs in setImmediate, after the response. Wait for the loop's + * "regeneration complete" log line rather than a fixed 150 ms: under a + * loaded machine (fifteen suites in parallel, each booting a migrated + * SQLite) the loop occasionally took longer than that, and the assertions + * then ran against a half-finished mock call list. + */ + const drain = async () => { + const deadline = Date.now() + 10000; + const done = () => logInfo.mock.calls.some((c) => /regeneration complete/.test(String(c[0]))); + while (!done() && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + }; it('rebuilds the canonical thumbnail for an external photo instead of erroring', async () => { const eventId = await seedEvent(); diff --git a/backend/__tests__/services/imageProcessor.singleFlight.test.js b/backend/__tests__/services/imageProcessor.singleFlight.test.js new file mode 100644 index 00000000..f2f41396 --- /dev/null +++ b/backend/__tests__/services/imageProcessor.singleFlight.test.js @@ -0,0 +1,497 @@ +/** + * Lazy rendition generation is single-flight per photo and rendition (#1020). + * + * ensureThumbnail / ensureHeroImage / ensurePreviewImage and the tier + * variants are check-then-generate, and the check reads the path off the row + * the caller already fetched. N concurrent cold requests for one photo all + * missed and all ran the same Sharp pass; worse, the hero and preview + * generators deleted the existing object before writing its replacement, so + * a reader landing between B's delete and B's write was redirected to the + * full original, and a regeneration whose source could not be read left the + * old rendition gone with the row still pointing at it. + * + * Driven against real Sharp output and the real LocalFsStorage, plus an + * in-memory backend with the S3 contract (no local paths, download on read), + * because the single-flight sits around withLocalCopy and the difference + * between one download and eight is the whole point. + */ +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); +const sharp = require('sharp'); + +const EXTERNAL_ROOT = path.join(os.tmpdir(), `picpeak-sf-ext-${process.pid}`); +process.env.EXTERNAL_MEDIA_ROOT = EXTERNAL_ROOT; + +jest.mock('../../src/database/db', () => { + const state = { events: {}, updates: [] }; + const api = (table) => { + if (table === 'events') { + return { where: (_col, id) => ({ first: async () => state.events[id] || null }) }; + } + if (table === 'photos') { + return { + where: (criteria) => ({ + update: async (values) => { state.updates.push({ criteria, values }); return 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 MANAGED_EVENT = { id: 11, slug: 'managed-ev', source_mode: 'managed' }; +const EXTERNAL_EVENT = { id: 7, slug: 'nas-ev', source_mode: 'reference', external_path: 'weddings/sf' }; + +let nextId = 1000; + +async function writeJpeg(absPath, { width = 2400, height = 1600 } = {}) { + await fs.mkdir(path.dirname(absPath), { recursive: true }); + await sharp({ create: { width, height, channels: 3, background: { r: 30, g: 120, b: 200 } } }) + .jpeg({ quality: 85 }).toFile(absPath); +} + +async function jpegBuffer({ width = 2400, height = 1600 } = {}) { + return sharp({ create: { width, height, channels: 3, background: { r: 200, g: 60, b: 30 } } }) + .jpeg({ quality: 85 }).toBuffer(); +} + +/** + * The S3 contract as imageProcessor sees it: kind() !== 'local', so validity + * is a stat only, and withLocalCopy has to download the source through + * getToFile before Sharp can open it. + */ +class MemoryObjectStore { + constructor() { this.objects = new Map(); this.puts = []; this.downloads = []; this.failNextPut = false; } + kind() { return 's3'; } + async init() {} + async put(key, body) { + if (this.failNextPut) { this.failNextPut = false; throw new Error('simulated upload failure'); } + this.puts.push(key); + this.objects.set(key, Buffer.from(body)); + } + async stat(key) { + const b = this.objects.get(key); + return b ? { size: b.length, mtime: new Date() } : null; + } + async exists(key) { return this.objects.has(key); } + async getToFile(key, localPath) { + this.downloads.push(key); + const b = this.objects.get(key); + if (!b) throw new Error(`NoSuchKey: ${key}`); + await fs.mkdir(path.dirname(localPath), { recursive: true }); + await fs.writeFile(localPath, b); + } + async delete(key) { this.objects.delete(key); } +} + +describe('single-flight rendition generation (#1020)', () => { + let imageProcessor; + + beforeAll(() => { + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + }); + + beforeEach(() => { + db.__state.events = { [MANAGED_EVENT.id]: MANAGED_EVENT, [EXTERNAL_EVENT.id]: EXTERNAL_EVENT }; + db.__state.updates = []; + }); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(EXTERNAL_ROOT, { recursive: true, force: true }).catch(() => {}); + }); + + describe('local storage', () => { + let storage; let storageRoot; let puts; + + beforeAll(async () => { + storageRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-sf-store-')); + storage = new LocalFsStorage({ root: storageRoot }); + await storage.init(); + const origPut = storage.put.bind(storage); + storage.put = async (key, ...rest) => { + if (storage.failNextPut) { storage.failNextPut = false; throw new Error('simulated write failure'); } + if (storage.holdNextPut) { const gate = storage.holdNextPut; storage.holdNextPut = null; await gate; } + puts.push(key); + return origPut(key, ...rest); + }; + storageModule.setStorageForTesting(storage); + }, 30000); + + beforeEach(() => { puts = []; storage.failNextPut = false; storage.holdNextPut = null; }); + + afterAll(async () => { + await fs.rm(storageRoot, { recursive: true, force: true }).catch(() => {}); + }); + + async function managedPhoto() { + const id = nextId++; + const name = `managed-${id}.jpg`; + const rel = `${MANAGED_EVENT.slug}/${name}`; + await writeJpeg(path.join(storageRoot, 'events/active', rel)); + return { id, event_id: MANAGED_EVENT.id, source_origin: 'managed', path: rel, filename: name }; + } + + async function externalPhoto({ write = true } = {}) { + const id = nextId++; + const name = `external-${id}.jpg`; + const relpath = path.join(EXTERNAL_EVENT.external_path, name); + if (write) await writeJpeg(path.join(EXTERNAL_ROOT, relpath)); + return { id, event_id: EXTERNAL_EVENT.id, source_origin: 'external', external_relpath: relpath, filename: name }; + } + + const putsUnder = (prefix) => puts.filter((k) => k.startsWith(prefix)); + + it('ensurePreviewImage: eight concurrent cold requests share one generation', async () => { + const photo = await managedPhoto(); + const results = await Promise.all(Array.from({ length: 8 }, () => imageProcessor.ensurePreviewImage(photo))); + + expect(results[0]).toMatch(/^previews\/preview_/); + expect(new Set(results).size).toBe(1); + expect(putsUnder('previews/')).toHaveLength(1); + // One flight, one row write — not eight identical updates. + expect(db.__state.updates).toHaveLength(1); + expect(await storage.stat(results[0])).toBeTruthy(); + }); + + it('ensureHeroImage: eight concurrent cold requests share one generation', async () => { + const photo = await managedPhoto(); + const results = await Promise.all(Array.from({ length: 8 }, () => imageProcessor.ensureHeroImage(photo))); + + expect(results[0]).toMatch(/^heroes\/hero_/); + expect(new Set(results).size).toBe(1); + expect(putsUnder('heroes/')).toHaveLength(1); + expect(db.__state.updates).toHaveLength(1); + }); + + it('ensureThumbnail: eight concurrent cold requests for an external photo share one generation', async () => { + const photo = await externalPhoto(); + const results = await Promise.all(Array.from({ length: 8 }, () => imageProcessor.ensureThumbnail(photo))); + + expect(results[0]).toBe(`thumbnails/thumb_ext${photo.id}_${photo.filename}`); + expect(new Set(results).size).toBe(1); + expect(putsUnder('thumbnails/')).toHaveLength(1); + expect(db.__state.updates).toHaveLength(1); + }); + + it('a tier request that resolves to the canonical thumbnail shares the canonical flight', async () => { + // ensureThumbnailAtWidth hands the canonical width, and every video, to + // ensureThumbnail. That used to be the one unguarded path a guarded + // request could fall through into. + const photo = await managedPhoto(); + const canonical = 300; // DEFAULT_THUMBNAIL_WIDTH; the settings mock returns no override + const results = await Promise.all([ + imageProcessor.ensureThumbnailAtWidth(photo, canonical), + imageProcessor.ensureThumbnailAtWidth(photo, canonical), + imageProcessor.ensureThumbnail(photo), + imageProcessor.ensureThumbnail(photo), + ]); + + expect(results[0]).toMatch(/^thumbnails\/thumb_/); + expect(new Set(results).size).toBe(1); + expect(putsUnder('thumbnails/')).toHaveLength(1); + }); + + it.each([ + ['ensureThumbnail', 'thumbnail_path', 'thumbnails/'], + ['ensureHeroImage', 'hero_path', 'heroes/'], + ['ensurePreviewImage', 'preview_path', 'previews/'], + ])('%s: a forced rebuild is never satisfied by joining a viewer\'s hot-path check', async (fn, column, prefix) => { + // adminThumbnails.js forces a rebuild by passing the row with the path + // nulled. If the validity check ran inside the flight, that call could + // join a viewer's flight for the same photo — one that was merely + // stat-ing an already good rendition — and be handed back the very + // file it was asked to replace, with the endpoint counting a success. + const photo = await managedPhoto(); + const existing = await imageProcessor[fn](photo); + expect(existing).toMatch(new RegExp(`^${prefix}`)); + expect(putsUnder(prefix)).toHaveLength(1); + + const [viewer, forced] = await Promise.all([ + imageProcessor[fn]({ ...photo, [column]: existing }), + imageProcessor[fn]({ ...photo, [column]: null }), + ]); + + expect(viewer).toBe(existing); + expect(forced).toBe(existing); + // The forced call wrote a fresh rendition; the viewer's did not. + expect(putsUnder(prefix)).toHaveLength(2); + }); + + it.each([ + ['ensureThumbnail', 'thumbnail_path', 'thumbnails/'], + ['ensurePreviewImage', 'preview_path', 'previews/'], + ])('%s: a forced rebuild runs after a lazy generation already in flight instead of adopting it', async (fn, column, prefix) => { + // The lazy flight read the settings when it started; after a settings + // change it is producing exactly what the admin's regenerate exists to + // replace. Joining it would count a success and leave the old size + // cached — the validity check only asks whether the file parses. + const photo = await managedPhoto(); + let release; + storage.holdNextPut = new Promise((r) => { release = r; }); + + const lazy = imageProcessor[fn](photo); // blocks inside put + const forced = imageProcessor[fn]({ ...photo, [column]: null }, { force: true }); + const joiner = imageProcessor[fn](photo); // lazy miss after the forced call + let forcedSettled = false; + forced.then(() => { forcedSettled = true; }); + + await new Promise((r) => setTimeout(r, 60)); + expect(putsUnder(prefix)).toHaveLength(0); + expect(forcedSettled).toBe(false); + + release(); + const results = await Promise.all([lazy, forced, joiner]); + expect(results.every((k) => k === results[0])).toBe(true); + expect(results[0]).toMatch(new RegExp(`^${prefix}`)); + // Lazy wrote once, the forced rebuild wrote once more after it; the + // later lazy miss joined the forced flight rather than starting a third. + expect(putsUnder(prefix)).toHaveLength(2); + }); + + it('a replaced photo (same id, new path) does not join a flight still rendering the old source', async () => { + // replacePhoto keeps the id and changes path/filename. Keyed by id and + // width alone, a request carrying the replacement row would join the + // old flight, be handed the old image, and the gallery would cache it. + const before = await managedPhoto(); + const after = await managedPhoto(); + const replacement = { ...after, id: before.id }; + + let release; + storage.holdNextPut = new Promise((r) => { release = r; }); + const stale = imageProcessor.ensureThumbnailAtWidth(before, 600); // blocks inside put + const fresh = imageProcessor.ensureThumbnailAtWidth(replacement, 600); + release(); + + const [oldKey, newKey] = await Promise.all([stale, fresh]); + expect(oldKey).toBe(`thumbnails/thumb_w600_p${before.id}_${before.filename}`); + expect(newKey).toBe(`thumbnails/thumb_w600_p${before.id}_${after.filename}`); + expect(putsUnder('thumbnails/').sort()).toEqual([oldKey, newKey].sort()); + + // Same shape for the canonical preview, which had no guard at all on + // base and must not gain a cross-source one now. + storage.holdNextPut = new Promise((r) => { release = r; }); + const staleP = imageProcessor.ensurePreviewImage(before); + const freshP = imageProcessor.ensurePreviewImage(replacement); + release(); + const [oldP, newP] = await Promise.all([staleP, freshP]); + expect(oldP).not.toBe(newP); + expect(putsUnder('previews/')).toHaveLength(2); + }); + + it('different photos and different widths are separate flights', async () => { + const a = await managedPhoto(); + const b = await externalPhoto(); + const calls = [ + ...Array.from({ length: 4 }, () => imageProcessor.ensurePreviewImageAtWidth(a, 640)), + ...Array.from({ length: 4 }, () => imageProcessor.ensurePreviewImageAtWidth(a, 1280)), + ...Array.from({ length: 4 }, () => imageProcessor.ensurePreviewImageAtWidth(b, 640)), + ...Array.from({ length: 4 }, () => imageProcessor.ensureThumbnailAtWidth(a, 600)), + ...Array.from({ length: 4 }, () => imageProcessor.ensureThumbnailAtWidth(b, 600)), + ]; + const results = await Promise.all(calls); + + expect(results.every(Boolean)).toBe(true); + expect(new Set(results).size).toBe(5); + expect(results.slice(0, 4).every((k) => k === results[0])).toBe(true); + expect(results.slice(4, 8).every((k) => k === results[4])).toBe(true); + expect(putsUnder('previews/')).toHaveLength(3); + expect(putsUnder('thumbnails/')).toHaveLength(2); + // Tiers are pure cache: never written to the row. + expect(db.__state.updates).toHaveLength(0); + }); + + it('a warm tier is served from storage without a second generation', async () => { + const photo = await managedPhoto(); + const first = await imageProcessor.ensurePreviewImageAtWidth(photo, 640); + const again = await Promise.all([ + imageProcessor.ensurePreviewImageAtWidth(photo, 640), + imageProcessor.ensurePreviewImageAtWidth(photo, 640), + ]); + + expect(again).toEqual([first, first]); + expect(putsUnder('previews/')).toHaveLength(1); + }); + + it('a failed flight is cleared so the next request retries instead of adopting the failure', async () => { + const photo = await externalPhoto({ write: false }); + + const cold = await Promise.all(Array.from({ length: 4 }, () => imageProcessor.ensurePreviewImage(photo))); + expect(cold).toEqual([null, null, null, null]); + expect(putsUnder('previews/')).toHaveLength(0); + + // The source appears (mount came back, file finished copying). + await writeJpeg(path.join(EXTERNAL_ROOT, photo.external_relpath)); + const warm = await imageProcessor.ensurePreviewImage(photo); + expect(warm).toBe(`previews/preview_ext${photo.id}_external-${photo.id}.jpg`); + expect(putsUnder('previews/')).toHaveLength(1); + }); + + it('a flight that returns null is shared by every waiter and cleared afterwards', async () => { + const photo = await managedPhoto(); + db.__state.events = {}; // the event lookup inside the flight finds nothing + const results = await Promise.all(Array.from({ length: 3 }, () => imageProcessor.ensureThumbnail(photo))); + expect(results).toEqual([null, null, null]); + + db.__state.events = { [MANAGED_EVENT.id]: MANAGED_EVENT }; + const key = await imageProcessor.ensureThumbnail(photo); + expect(key).toMatch(/^thumbnails\/thumb_/); + expect(putsUnder('thumbnails/')).toHaveLength(1); + }); + + it('a flight that throws rejects every waiter identically and is cleared afterwards', async () => { + const photo = await managedPhoto(); + const boom = new Error('db down'); + const realEvents = db.__state.events; + db.__state.events = new Proxy({}, { get: () => { throw boom; } }); + + // ensureThumbnail's event lookup is not wrapped in try/catch, so this + // propagates — to every caller of the shared flight, not just the first. + const settled = await Promise.allSettled(Array.from({ length: 3 }, () => imageProcessor.ensureThumbnail(photo))); + expect(settled.map((s) => s.status)).toEqual(['rejected', 'rejected', 'rejected']); + expect(settled.every((s) => s.reason === boom)).toBe(true); + + db.__state.events = realEvents; + const key = await imageProcessor.ensureThumbnail(photo); + expect(key).toMatch(/^thumbnails\/thumb_/); + }); + + it('the existing hero survives a regeneration whose source cannot be read', async () => { + // generateHeroImage used to delete the target before Sharp had opened + // the source, so a corrupt file or a blipped mount stripped the old + // hero and returned null with the row still pointing at it. + const src = path.join(storageRoot, 'events/active', MANAGED_EVENT.slug, 'hero-src.jpg'); + await writeJpeg(src); + const key = await imageProcessor.generateHeroImage(src, { outputBasename: 'survive.jpg' }); + expect(key).toBe('heroes/hero_survive.jpg'); + const before = await storage.stat(key); + + const junk = path.join(storageRoot, 'events/active', MANAGED_EVENT.slug, 'hero-junk.jpg'); + await fs.writeFile(junk, Buffer.from('this is not a jpeg')); + const result = await imageProcessor.generateHeroImage(junk, { regenerate: true, outputBasename: 'survive.jpg' }); + + expect(result).toBeNull(); + const after = await storage.stat(key); + expect(after).toBeTruthy(); + expect(after.size).toBe(before.size); + await expect(sharp(storage.resolveLocalPath(key)).metadata()).resolves.toMatchObject({ width: 1920 }); + }); + + it('the existing preview survives a regeneration whose write fails', async () => { + // Probe succeeds, the pipeline runs, the put throws: the catch used to + // delete the key, which by then only ever held the PREVIOUS good file. + const src = path.join(storageRoot, 'events/active', MANAGED_EVENT.slug, 'preview-src.jpg'); + await writeJpeg(src); + const key = await imageProcessor.generatePreviewImage(src, { outputBasename: 'survive.jpg' }); + expect(key).toBe('previews/preview_survive.jpg'); + const before = await storage.stat(key); + + storage.failNextPut = true; + const result = await imageProcessor.generatePreviewImage(src, { regenerate: true, outputBasename: 'survive.jpg' }); + + expect(result).toBeNull(); + const after = await storage.stat(key); + expect(after).toBeTruthy(); + expect(after.size).toBe(before.size); + }); + + it('the existing hero survives a regeneration whose write fails', async () => { + const src = path.join(storageRoot, 'events/active', MANAGED_EVENT.slug, 'hero-src2.jpg'); + await writeJpeg(src); + const key = await imageProcessor.generateHeroImage(src, { outputBasename: 'survive2.jpg' }); + const before = await storage.stat(key); + + storage.failNextPut = true; + const result = await imageProcessor.generateHeroImage(src, { regenerate: true, outputBasename: 'survive2.jpg' }); + + expect(result).toBeNull(); + expect((await storage.stat(key)).size).toBe(before.size); + }); + }); + + describe('S3 contract', () => { + let store; + + beforeAll(() => { + store = new MemoryObjectStore(); + storageModule.setStorageForTesting(store); + }); + + beforeEach(() => { store.puts = []; store.downloads = []; store.failNextPut = false; }); + + async function managedPhoto() { + const id = nextId++; + const name = `s3-${id}.jpg`; + const rel = `${MANAGED_EVENT.slug}/${name}`; + store.objects.set(`events/active/${rel}`, await jpegBuffer()); + return { id, event_id: MANAGED_EVENT.id, source_origin: 'managed', path: rel, filename: name }; + } + + it('ensurePreviewImage: concurrent cold requests download the source once and upload once', async () => { + const photo = await managedPhoto(); + const results = await Promise.all(Array.from({ length: 6 }, () => imageProcessor.ensurePreviewImage(photo))); + + expect(new Set(results).size).toBe(1); + expect(results[0]).toMatch(/^previews\/preview_/); + expect(store.downloads).toEqual([`events/active/${photo.path}`]); + expect(store.puts).toHaveLength(1); + expect(await store.stat(results[0])).toBeTruthy(); + expect(db.__state.updates).toHaveLength(1); + }); + + it('ensureHeroImage: concurrent cold requests download the source once and upload once', async () => { + const photo = await managedPhoto(); + const results = await Promise.all(Array.from({ length: 6 }, () => imageProcessor.ensureHeroImage(photo))); + + expect(new Set(results).size).toBe(1); + expect(store.downloads).toHaveLength(1); + expect(store.puts).toHaveLength(1); + }); + + it('ensureThumbnailAtWidth: concurrent cold tier requests download the source once and upload once', async () => { + const photo = await managedPhoto(); + const results = await Promise.all(Array.from({ length: 6 }, () => imageProcessor.ensureThumbnailAtWidth(photo, 600))); + + expect(new Set(results).size).toBe(1); + expect(results[0]).toBe(`thumbnails/thumb_w600_p${photo.id}_${photo.filename}`); + expect(store.downloads).toHaveLength(1); + expect(store.puts).toEqual([results[0]]); + }); + + it('the existing preview object survives a regeneration whose upload fails', async () => { + // Fixed outputBasename, as the external and RAW branches pass: the key + // is the same on both runs, so the pre-fix delete would have hit the + // good object. (Through withLocalCopy the basename carries a random + // temp prefix and the two runs never share a key, which is why this + // drives the generator directly.) + const srcDir = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-sf-s3src-')); + const src = path.join(srcDir, 'src.jpg'); + await writeJpeg(src); + try { + const key = await imageProcessor.generatePreviewImage(src, { outputBasename: 's3-survive.jpg' }); + expect(key).toBe('previews/preview_s3-survive.jpg'); + const before = store.objects.get(key); + expect(before).toBeTruthy(); + + store.failNextPut = true; + const result = await imageProcessor.generatePreviewImage(src, { regenerate: true, outputBasename: 's3-survive.jpg' }); + + expect(result).toBeNull(); + expect(store.objects.get(key)).toBe(before); + } finally { + await fs.rm(srcDir, { recursive: true, force: true }).catch(() => {}); + } + }); + }); +}); diff --git a/backend/src/routes/adminThumbnails.js b/backend/src/routes/adminThumbnails.js index 8ad5c7fd..3743fdf6 100644 --- a/backend/src/routes/adminThumbnails.js +++ b/backend/src/routes/adminThumbnails.js @@ -204,7 +204,9 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r // cannot clobber each other, and writes thumbnail_path back itself. // Nulling thumbnail_path is what stops it short-circuiting on // isThumbnailValid — the same trick /regenerate-previews uses. - const newThumbnailPath = await ensureThumbnail({ ...photo, thumbnail_path: null }); + // `force` is what stops it joining a lazy generation that is still + // running under the OLD settings and adopting that result (#1020). + const newThumbnailPath = await ensureThumbnail({ ...photo, thumbnail_path: null }, { force: true }); if (newThumbnailPath) { // Drop the superseded canonical rendition when the key MOVED. @@ -293,7 +295,8 @@ router.post('/regenerate-previews', adminAuth, requirePermission('photos.edit'), // is precisely the case this endpoint exists for (a replaced // reference source, or a corrupted rendition). await require('../services/imageProcessor').deletePreviewTiers(photo); - const newPreviewPath = await ensurePreviewImage({ ...photo, preview_path: null }); + // `force`: never adopt a lazy generation already in flight (#1020). + const newPreviewPath = await ensurePreviewImage({ ...photo, preview_path: null }, { force: true }); if (newPreviewPath) { successCount++; } else { diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 471e0587..26a0039e 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -443,6 +443,84 @@ async function withLocalCopy(sourceKey, fn) { } } +/** + * Process-local single-flight for lazy rendition generation (#1020). + * + * Every ensure* function below is check-then-generate: look for the + * rendition, run Sharp if it is missing or invalid. The check reads the path + * off the photo row the caller already fetched, so N simultaneous requests + * for a cold photo — several viewers opening the same lightbox slide, two + * kiosks starting the same slideshow (#1018), a grid mounting one tile per + * photo — all hold a snapshot where the path is still null, all miss, and + * all run the same resize. The output key is deterministic, so they leave no + * orphans; they multiply CPU, memory and source reads (a full download on + * S3, a full read off the NAS for a reference photo) at exactly the moment + * the system is already cold. + * + * Only a MISS enters the flight. The validity check on the caller's own + * snapshot runs outside it, lock-free, so a request that already has a good + * rendition never joins anything — and, the other way round, a forced rebuild + * (the admin regenerate endpoints pass a row with the path nulled) can never + * be satisfied by joining a viewer's hot-path flight and being handed the + * very rendition it was asked to replace. Inside the flight, everything is + * a regeneration. + * + * A forced rebuild (`force: true`) goes one step further: if a flight is + * already GENERATING for the key it does not join that either, it runs after + * it. The older flight read the thumbnail settings when it started, so after + * a settings change it is producing exactly the rendition the admin's + * regenerate was invoked to replace — adopting its result would count a + * success while the old size stays cached, and the validity check never + * notices because it only asks whether the file parses. Lazy misses that + * arrive while the forced flight is pending join it, so the map always + * points at the newest work. + * + * One map for every rendition, keyed by rendition, photo id AND source + * rather than by storage key: a preview's key is only known after the + * source has been probed, and the canonical thumbnail ensureThumbnailAtWidth + * falls back to must be guarded by the same mechanism as the tier it missed. + * The source is part of the key because replacePhoto keeps the id and + * changes the path — a request carrying the replacement row must not join a + * flight still rendering the file it replaced and cache that for 30 minutes. The entry is + * cleared in a finally, on success and failure alike, so a rejection cannot + * poison the key for the lifetime of the process — the next request + * re-attempts rather than adopting a failure. + * + * Deliberately no re-read of the photo row inside the flight. A request + * whose snapshot was taken while a previous flight was generating, and that + * reaches the map only after that flight has cleared, generates once more: + * one extra pass, not N. A re-read would close even that, but the admin + * regenerate endpoints force a rebuild precisely by passing a row with the + * path nulled (adminThumbnails.js), and a re-read would find the persisted + * rendition valid and hand it back untouched. + * + * Per-process only. Two replicas still generate independently, which is + * harmless: LocalFsStorage.put renames atomically and an S3 put overwrites + * by key, so they converge on the same output. Cross-replica coordination + * would need a storage-level lock and is not justified by the impact. + */ +const inFlightRenditions = new Map(); + +function flightKey(rendition, photo, width) { + const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; + const source = (isExternal ? (photo.external_relpath || photo.filename) : photo.path) || ''; + return `${rendition}:${photo.id}:${source}${width ? `:w${width}` : ''}`; +} + +function singleFlight(key, fn, { force = false } = {}) { + const pending = inFlightRenditions.get(key); + if (pending && !force) return pending; + // Forced: start once the older flight has settled, whichever way it went. + const start = pending ? pending.then(fn, fn) : Promise.resolve().then(fn); + const work = start.finally(() => { + // Only drop our own entry: an older flight settling later than the forced + // one that superseded it must not evict the newer work from the map. + if (inFlightRenditions.get(key) === work) inFlightRenditions.delete(key); + }); + inFlightRenditions.set(key, work); + return work; +} + /** * Regenerate thumbnail if it's broken or missing. * @@ -453,7 +531,20 @@ async function withLocalCopy(sourceKey, fn) { * to fall back to streaming the full original on every tile — minutes of * load time for a 100-photo NAS-mounted gallery. */ -async function ensureThumbnail(photo) { +async function ensureThumbnail(photo, { force = false } = {}) { + // Check if thumbnail exists and is valid (works for any source). + if (!force && photo.thumbnail_path) { + const isValid = await isThumbnailValid(photo.thumbnail_path); + if (isValid) { + return photo.thumbnail_path; + } + logger.warn(`Invalid thumbnail detected for photo ${photo.id}, regenerating...`); + } + + return singleFlight(flightKey('thumbnail', photo), () => regenerateThumbnail(photo), { force }); +} + +async function regenerateThumbnail(photo) { const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); const event = await db('events').where('id', photo.event_id).first(); @@ -462,15 +553,6 @@ async function ensureThumbnail(photo) { return null; } - // Check if thumbnail exists and is valid (works for any source). - if (photo.thumbnail_path) { - const isValid = await isThumbnailValid(photo.thumbnail_path); - if (isValid) { - return photo.thumbnail_path; - } - logger.warn(`Invalid thumbnail detected for photo ${photo.id}, regenerating...`); - } - const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; let newThumbnailPath; @@ -578,9 +660,15 @@ async function generateHeroImage(imagePath, options = {}) { const heroRelKey = path.posix.join('heroes', heroFilename); const storage = getStorage(); - if (options.regenerate) { - await storage.delete(heroRelKey).catch(() => {}); - } + // `options.regenerate` does not delete the existing object first, and the + // catch below does not clean up either — same reasoning as generateThumbnail + // (#1129, #1020). `storage.put` is the last statement in the try, so nothing + // partial can exist for the catch to remove; LocalFsStorage.put stages and + // renames atomically and an S3 put overwrites by key, so the write replaces + // the old rendition on its own. All the delete added was a window with no + // hero at all — in which a concurrent reader was redirected to the full + // original — and a source that could not be read left the old hero gone + // with the row still pointing at it. try { const metadata = await sharp(imagePath).metadata(); @@ -631,7 +719,6 @@ async function generateHeroImage(imagePath, options = {}) { } catch (error) { const msg = (error && error.message) ? error.message : String(error); logger.error(`Failed to generate hero image for ${filename}: ${msg}`); - await storage.delete(heroRelKey).catch(() => {}); return null; } } @@ -660,6 +747,18 @@ async function isHeroValid(heroPath) { * Ensure a hero image exists for a photo, regenerate if needed */ async function ensureHeroImage(photo) { + if (photo.hero_path) { + const isValid = await isHeroValid(photo.hero_path); + if (isValid) { + return photo.hero_path; + } + logger.warn(`Invalid hero image detected for photo ${photo.id}, regenerating...`); + } + + return singleFlight(flightKey('hero', photo), () => regenerateHeroImage(photo)); +} + +async function regenerateHeroImage(photo) { const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); let event; @@ -670,14 +769,6 @@ async function ensureHeroImage(photo) { return null; } - if (photo.hero_path) { - const isValid = await isHeroValid(photo.hero_path); - if (isValid) { - return photo.hero_path; - } - logger.warn(`Invalid hero image detected for photo ${photo.id}, regenerating...`); - } - // External sources never reach the managed backend, so resolvePhotoStorageKey // returns null for them by design — and this function used to feed that null // straight to withLocalCopy, which throws, so the hero route fell back to @@ -800,9 +891,8 @@ async function generatePreviewImage(imagePath, options = {}) { const previewFilename = `preview_${widthTag}${base}.${needsWebp ? 'webp' : 'jpg'}`; const previewRelKey = path.posix.join('previews', previewFilename); - if (options.regenerate) { - await storage.delete(previewRelKey).catch(() => {}); - } + // No delete on `options.regenerate` and none in the catch below — see + // generateHeroImage; the reasoning (#1129, #1020) is identical. try { const metadata = probe; @@ -869,7 +959,6 @@ async function generatePreviewImage(imagePath, options = {}) { } catch (error) { const msg = (error && error.message) ? error.message : String(error); logger.error(`Failed to generate preview image for ${filename}: ${msg}`); - await storage.delete(previewRelKey).catch(() => {}); return null; } } @@ -985,12 +1074,6 @@ async function deleteThumbnailTiers(photo) { * served from a cache hit without re-reading the source, so an unscoped key * would hand one gallery's photo to another. */ -/** - * Tier storage key -> the in-flight generation for it (#1128). Module scope so - * every concurrent request for one tile shares a single Sharp pass. - */ -const inFlightThumbnailTiers = new Map(); - async function ensureThumbnailAtWidth(photo, width) { if (!width) return ensureThumbnail(photo); @@ -1011,6 +1094,24 @@ async function ensureThumbnailAtWidth(photo, width) { return ensureThumbnail(photo); } + // One generation per tier, however many tiles ask for it (#1128, #1020). + // + // A grid issues one request per tile simultaneously, and on a cold gallery + // every one of them misses the stat inside. Without this each would run its + // own Sharp pass over the same source — and for an external photo, re-read + // the whole original off the NFS mount to do it. 79 tiles meant 79 decodes + // of the same file, which is also what made the delete race easy to hit. + // + // The stat lives INSIDE the flight so a request that arrives just as the + // previous flight clears finds the freshly written tier instead of missing + // on a stale probe and starting another pass. + return singleFlight( + flightKey('thumbnail', photo, width), + () => ensureThumbnailTierUnguarded(photo, width, settings, canonicalWidth) + ); +} + +async function ensureThumbnailTierUnguarded(photo, width, settings, canonicalWidth) { const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); const storage = getStorage(); @@ -1042,66 +1143,47 @@ async function ensureThumbnailAtWidth(photo, width) { // would visibly reframe as the tile size changes. const height = Math.round(width * (settings.height / canonicalWidth)); - // One generation per tier key, however many tiles ask for it (#1128). - // - // A grid issues one request per tile simultaneously, and on a cold gallery - // every one of them misses the stat above. Without this each would run its - // own Sharp pass over the same source — and for an external photo, re-read - // the whole original off the NFS mount to do it. 79 tiles meant 79 decodes - // of the same file, which is also what made the delete race easy to hit. - // - // Per-process only. Two pods still generate independently, which is - // harmless: the write ends in an atomic rename, so they converge on - // byte-identical output. - const pending = inFlightThumbnailTiers.get(key); - if (pending) return pending; - - const work = (async () => { - try { - // NOT `regenerate: true` (#1128). This path is only reached on a cache - // MISS, so there is nothing to regenerate — but that flag makes - // generateThumbnail open by DELETING the target. Request A publishes the - // tier, B stats it and heads for storage.get(), and C — still inside - // generation from its own earlier miss — unlinks the file B is about to - // open. B's lazy ReadStream then raised an ENOENT nothing was listening - // for and Node exited. - // - // Without the flag the write is a plain put: LocalFsStorage stages to a - // temp file and renames, which is atomic, so a concurrent reader sees - // either the old file or the new one and never a hole. - if (isExternal) { - const localPath = resolvePhotoFilePath(event, photo); - return await generateThumbnail(localPath, { outputBasename, width, height }); - } - const sourceKey = resolvePhotoStorageKey(event, photo); - if (!sourceKey) return null; - return await withLocalCopy(sourceKey, async (localPath) => { - const proc = await withProcessableImage(localPath, sourceKey); - try { - return await generateThumbnail(proc.path, { outputBasename, width, height }); - } finally { - proc.cleanup(); - } - }); - } catch (e) { - logger.warn(`Thumbnail tier w${width} failed for photo ${photo.id}: ${e.message}`); - return null; - } - })(); - - inFlightThumbnailTiers.set(key, work); try { - return await work; - } finally { - // In a finally so a rejection cannot poison the key for the process - // lifetime — the next request re-attempts rather than adopting a failure. - inFlightThumbnailTiers.delete(key); + // NOT `regenerate: true` (#1128). This path is only reached on a cache + // MISS, so there is nothing to regenerate — but that flag used to make + // generateThumbnail open by DELETING the target. Request A publishes the + // tier, B stats it and heads for storage.get(), and C — still inside + // generation from its own earlier miss — unlinks the file B is about to + // open. B's lazy ReadStream then raised an ENOENT nothing was listening + // for and Node exited. + // + // Without the flag the write is a plain put: LocalFsStorage stages to a + // temp file and renames, which is atomic, so a concurrent reader sees + // either the old file or the new one and never a hole. + if (isExternal) { + const localPath = resolvePhotoFilePath(event, photo); + return await generateThumbnail(localPath, { outputBasename, width, height }); + } + const sourceKey = resolvePhotoStorageKey(event, photo); + if (!sourceKey) return null; + return await withLocalCopy(sourceKey, async (localPath) => { + const proc = await withProcessableImage(localPath, sourceKey); + try { + return await generateThumbnail(proc.path, { outputBasename, width, height }); + } finally { + proc.cleanup(); + } + }); + } catch (e) { + logger.warn(`Thumbnail tier w${width} failed for photo ${photo.id}: ${e.message}`); + return null; } } async function ensurePreviewImageAtWidth(photo, width) { if (!width || width === DEFAULT_PREVIEW_LONG_EDGE) return ensurePreviewImage(photo); + return singleFlight( + flightKey('preview', photo, width), + () => ensurePreviewImageAtWidthUnguarded(photo, width) + ); +} +async function ensurePreviewImageAtWidthUnguarded(photo, width) { const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); const storage = getStorage(); @@ -1163,7 +1245,17 @@ async function ensurePreviewImageAtWidth(photo, width) { } } -async function ensurePreviewImage(photo) { +async function ensurePreviewImage(photo, { force = false } = {}) { + if (!force && photo.preview_path) { + const ok = await isPreviewValid(photo.preview_path); + if (ok) return photo.preview_path; + logger.warn(`Invalid preview detected for photo ${photo.id}, regenerating…`); + } + + return singleFlight(flightKey('preview', photo), () => regeneratePreviewImage(photo), { force }); +} + +async function regeneratePreviewImage(photo) { const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); let event; @@ -1179,12 +1271,6 @@ async function ensurePreviewImage(photo) { return null; } - if (photo.preview_path) { - const ok = await isPreviewValid(photo.preview_path); - if (ok) return photo.preview_path; - logger.warn(`Invalid preview detected for photo ${photo.id}, regenerating…`); - } - const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; let newPreviewPath;