diff --git a/backend/__tests__/integration/adminThumbnails.regenerate.test.js b/backend/__tests__/integration/adminThumbnails.regenerate.test.js new file mode 100644 index 00000000..0fce55ca --- /dev/null +++ b/backend/__tests__/integration/adminThumbnails.regenerate.test.js @@ -0,0 +1,246 @@ +/** + * POST /admin/thumbnails/regenerate for external/reference photos (#1129). + * + * STABLE TWIN. Diverges from the main version in one place: stable has no + * responsive ?w= tiers (#1095/#1109), so there is no deleteThumbnailTiers call + * to assert and the "drops the tiers first" test is absent here. Everything + * else — the external rebuild, the thumbnail_path:null contract, video + * skipping, per-event scoping and the superseded-key deletion — is identical. + * + * The route used to resolve every source as `storage/events/active/` and + * `fs.access` it. External and reference rows do not live there — their + * originals sit under `events.external_path` — so every one of them failed the + * check and was counted as an error. + * + * That alone would be inert. What made it destructive is that the tier + * deletion runs FIRST (deliberately, so S3 and external rows are not skipped): + * on a reference install the button dropped every ?w= tier and rebuilt + * nothing, while the UI reported success — the response is sent before the + * background loop starts. + * + * The background work is fired with setImmediate, so every assertion here has + * to wait for it to drain rather than trusting the response. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +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; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-regen-')); + process.env.NODE_ENV = 'test'; + process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'test.db'); + process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); + await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true }); + await fs.promises.mkdir(process.env.STORAGE_PATH, { recursive: true }); + + jest.resetModules(); + + jest.doMock('../../src/middleware/auth', () => ({ + adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); }, + })); + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + })); + // One instance, not a fresh object per call — the route and the + // assertions have to be looking at the same mock. + jest.doMock('../../src/services/storage', () => { + const instance = { delete: jest.fn().mockResolvedValue(undefined) }; + return { getStorage: () => instance }; + }); + jest.doMock('../../src/services/imageProcessor', () => ({ + ensureThumbnail: jest.fn().mockResolvedValue('thumbnails/thumb_ext1_shot.jpg'), + ensurePreviewImage: jest.fn().mockResolvedValue('previews/p.jpg'), + deletePreviewTiers: jest.fn().mockResolvedValue(undefined), + })); + + // 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()); + + imageProcessor = require('../../src/services/imageProcessor'); + storage = require('../../src/services/storage').getStorage(); + app = express(); + app.use(express.json()); + app.use('/admin/thumbnails', require('../../src/routes/adminThumbnails')); + }, 180000); + + afterAll(async () => { + if (cleanup) await cleanup(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + await db('photos').del(); + await db('events').del(); + }); + + async function seedEvent() { + const [row] = await db('events').insert({ + slug: 'nas-wedding', event_type: 'wedding', event_name: 'nas', + event_date: '2026-01-01', host_email: 'h@example.com', admin_email: 'a@example.com', + password_hash: 'x', share_link: 'nas-share', expires_at: new Date().toISOString(), + source_mode: 'reference', external_path: 'weddings/2026-08', + }).returning('id'); + return typeof row === 'object' ? row.id : row; + } + + async function seedPhoto(eventId, overrides = {}) { + const [row] = await db('photos').insert({ + event_id: eventId, filename: 'shot.jpg', path: 'nas-wedding/shot.jpg', + type: 'individual', ...overrides, + }).returning('id'); + 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)); + + it('rebuilds the canonical thumbnail for an external photo instead of erroring', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { + source_origin: 'external', + external_relpath: 'shot.jpg', + thumbnail_path: 'thumbnails/stale.jpg', + }); + + const res = await request(app).post('/admin/thumbnails/regenerate').send({}); + expect(res.status).toBe(200); + await drain(); + + // The whole bug: this used to be zero calls and one logged + // "Original file not found" per photo. + expect(imageProcessor.ensureThumbnail).toHaveBeenCalledTimes(1); + }); + + it('nulls thumbnail_path so the valid-thumbnail short-circuit cannot skip the rebuild', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { + source_origin: 'external', + external_relpath: 'shot.jpg', + thumbnail_path: 'thumbnails/still-on-disk.jpg', + }); + + await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + // Without this the endpoint is a no-op whenever the OLD thumbnail is still + // readable — which is the normal case after a settings change, and exactly + // when the admin pressed the button. + const [photoArg] = imageProcessor.ensureThumbnail.mock.calls[0]; + expect(photoArg.thumbnail_path).toBeNull(); + expect(photoArg.source_origin).toBe('external'); + // Carried through so ensureThumbnail can resolve off the mount rather than + // under events/active. + expect(photoArg.external_relpath).toBe('shot.jpg'); + }); + + it('leaves videos alone rather than handing a container file to Sharp', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { source_origin: 'managed', media_type: 'video', filename: 'clip.mp4' }); + await seedPhoto(eventId, { source_origin: 'managed', filename: 'still.jpg' }); + + const res = await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + expect(res.body.count).toBe(1); + expect(imageProcessor.ensureThumbnail).toHaveBeenCalledTimes(1); + expect(imageProcessor.ensureThumbnail.mock.calls[0][0].filename).toBe('still.jpg'); + }); + + /** + * On S3, ensureThumbnail downloads the source to a randomly-named temp file, + * and for non-RAW input withProcessableImage passes no outputBasename — so + * generateThumbnail derives the key from that random name and it differs on + * every run. Nulling thumbnail_path hides the old key from everything that + * would otherwise clean it up, so each regeneration would strand a full + * thumbnail in the bucket, once per photo per run. + */ + describe('superseded canonical renditions', () => { + it('removes the old thumbnail when the key moved', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { + source_origin: 'managed', + thumbnail_path: 'thumbnails/thumb_OLDRANDOM_shot.jpg', + }); + imageProcessor.ensureThumbnail.mockResolvedValueOnce('thumbnails/thumb_NEWRANDOM_shot.jpg'); + + await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + expect(storage.delete).toHaveBeenCalledWith('thumbnails/thumb_OLDRANDOM_shot.jpg'); + }); + + it('does NOT delete when the key is unchanged — that is the new file', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { + source_origin: 'managed', + thumbnail_path: 'thumbnails/thumb_stable.jpg', + }); + // Local storage resolves to a stable path, so the key is identical. + imageProcessor.ensureThumbnail.mockResolvedValueOnce('thumbnails/thumb_stable.jpg'); + + await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + expect(storage.delete).not.toHaveBeenCalled(); + }); + + it.each([ + ['a Windows-style legacy path', 'thumbnails\\thumb_ext1_shot.jpg'], + ['a leading ./', './thumbnails/thumb_ext1_shot.jpg'], + ['a doubled separator', 'thumbnails//thumb_ext1_shot.jpg'], + ])('does not delete the file it just wrote when the old path is %s', async (_name, stored) => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { source_origin: 'managed', thumbnail_path: stored }); + // Both storage backends fold these to the same key, so this is the SAME + // object — deleting it would remove the freshly generated thumbnail and + // leave the row pointing at nothing. + imageProcessor.ensureThumbnail.mockResolvedValueOnce('thumbnails/thumb_ext1_shot.jpg'); + + await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + expect(storage.delete).not.toHaveBeenCalled(); + }); + + it('counts the photo as regenerated even if the old object cannot be removed', async () => { + const eventId = await seedEvent(); + await seedPhoto(eventId, { + source_origin: 'managed', + thumbnail_path: 'thumbnails/thumb_OLD.jpg', + }); + imageProcessor.ensureThumbnail.mockResolvedValueOnce('thumbnails/thumb_NEW.jpg'); + storage.delete.mockRejectedValueOnce(new Error('bucket said no')); + + await request(app).post('/admin/thumbnails/regenerate').send({}); + await drain(); + + // Losing the old object is untidy; the regeneration itself succeeded. + expect(imageProcessor.ensureThumbnail).toHaveBeenCalledTimes(1); + }); + }); + + it('scopes to one event when asked', async () => { + const a = await seedEvent(); + await seedPhoto(a, { source_origin: 'external', external_relpath: 'a.jpg' }); + const [b] = await db('events').insert({ + slug: 'other', event_type: 'wedding', event_name: 'other', event_date: '2026-01-01', + host_email: 'h@example.com', admin_email: 'a@example.com', password_hash: 'x', + share_link: 'other-share', expires_at: new Date().toISOString(), + }).returning('id'); + await seedPhoto(typeof b === 'object' ? b.id : b, { source_origin: 'managed' }); + + const res = await request(app).post('/admin/thumbnails/regenerate').send({ eventId: a }); + await drain(); + + expect(res.body.count).toBe(1); + expect(imageProcessor.ensureThumbnail).toHaveBeenCalledTimes(1); + }); +}); diff --git a/backend/__tests__/services/generateThumbnail.regenerate.test.js b/backend/__tests__/services/generateThumbnail.regenerate.test.js new file mode 100644 index 00000000..558bad2e --- /dev/null +++ b/backend/__tests__/services/generateThumbnail.regenerate.test.js @@ -0,0 +1,90 @@ +/** + * Regeneration must not destroy a good thumbnail when the source is + * unreadable (#1129). + * + * The old code deleted the target BEFORE sharp opened the source, so a NAS + * mount that blipped mid-run left the previous rendition gone and the database + * still pointing at it. Across a bulk regenerate that is the whole gallery, + * and it is precisely the "worse than before you pressed it" outcome #1129 is + * about. + */ + +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); +const sharp = require('sharp'); + +jest.mock('../../src/database/db', () => ({ + db: () => ({ where: () => ({ first: async () => null, update: async () => 1 }) }), +})); + +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); +const storageModule = require('../../src/services/storage'); + +describe('generateThumbnail — regenerate is non-destructive (#1129)', () => { + let storage; let root; let imageProcessor; let srcDir; + + beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-regen-store-')); + srcDir = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-regen-src-')); + storage = new LocalFsStorage({ root }); + await storage.init(); + storageModule.setStorageForTesting(storage); + + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(root, { recursive: true, force: true }).catch(() => {}); + await fs.rm(srcDir, { recursive: true, force: true }).catch(() => {}); + }); + + async function writeSource(name, size = 400) { + const p = path.join(srcDir, name); + await sharp({ create: { width: size, height: size, channels: 3, background: { r: 1, g: 2, b: 3 } } }) + .jpeg().toFile(p); + return p; + } + + it('keeps the existing thumbnail when the source cannot be read', async () => { + const src = await writeSource('present.jpg'); + const key = await imageProcessor.generateThumbnail(src, { regenerate: true }); + expect(key).toBeTruthy(); + expect(await storage.exists(key)).toBe(true); + const before = await storage.get(key).then((s) => new Promise((res) => { + const c = []; s.on('data', (d) => c.push(d)); s.on('end', () => res(Buffer.concat(c))); + })); + + // The mount goes away between runs. + await fs.unlink(src); + const second = await imageProcessor.generateThumbnail(src, { regenerate: true }) + .catch(() => null); + + expect(second).toBeFalsy(); + // The old rendition is still there and still serves. Previously it had + // been deleted before sharp ever looked at the source. + expect(await storage.exists(key)).toBe(true); + const after = await storage.get(key).then((s) => new Promise((res) => { + const c = []; s.on('data', (d) => c.push(d)); s.on('end', () => res(Buffer.concat(c))); + })); + expect(after.equals(before)).toBe(true); + }); + + it('still replaces the thumbnail when the source IS readable', async () => { + const src = await writeSource('replaceme.jpg', 400); + const key = await imageProcessor.generateThumbnail(src, { regenerate: true }); + const firstSize = (await storage.stat(key)).size; + + // Same key, different source content — the atomic rename in put() is what + // makes the pre-delete unnecessary. + await fs.rm(src); + await sharp({ create: { width: 400, height: 400, channels: 3, background: { r: 250, g: 40, b: 9 } } }) + .jpeg().toFile(src); + const again = await imageProcessor.generateThumbnail(src, { regenerate: true }); + + expect(again).toBe(key); + expect((await storage.stat(key)).size).not.toBe(firstSize); + }); +}); diff --git a/backend/src/routes/adminThumbnails.js b/backend/src/routes/adminThumbnails.js index c7d1e789..3447e712 100644 --- a/backend/src/routes/adminThumbnails.js +++ b/backend/src/routes/adminThumbnails.js @@ -3,12 +3,27 @@ const router = express.Router(); const { db } = require('../database/db'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); -const { generateThumbnail, ensurePreviewImage } = require('../services/imageProcessor'); -const path = require('path'); -const fs = require('fs').promises; +const { ensureThumbnail, ensurePreviewImage } = require('../services/imageProcessor'); +const { getStorage } = require('../services/storage'); const logger = require('../utils/logger'); -const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); +/** + * Do these two stored paths address the same object? + * + * Compared the way the storage backends do, not as raw strings. + * LocalFsStorage._resolve and S3StorageBackend._key both fold `\` to `/` and + * strip a leading `./`, so a legacy thumbnail_path in any of those shapes is + * the SAME file as the freshly generated POSIX key while comparing unequal — + * and the "the key moved, delete the old one" branch below would then delete + * the thumbnail that had just been written. + */ +function sameStorageKey(a, b) { + const canonical = (key) => String(key) + .replace(/\\/g, '/') + .replace(/^\.?\/+/, '') + .replace(/\/+/g, '/'); + return canonical(a) === canonical(b); +} // Parse JSON-encoded setting values function parseSettingValue(value) { @@ -125,10 +140,21 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r try { const { eventId } = req.body; // Optional: regenerate for specific event only - let query = db('photos').select('id', 'event_id', 'path'); + // source_origin/external_relpath/filename are what ensureThumbnail branches + // on to resolve an external source off its mount instead of under + // events/active. thumbnail_path is selected so it can be nulled — see below. + let query = db('photos').select( + 'id', 'event_id', 'path', 'media_type', 'mime_type', 'thumbnail_path', + 'source_origin', 'external_relpath', 'filename' + ); if (eventId) { query = query.where('event_id', eventId); } + // Skip videos: their thumbnail is a poster frame from videoProcessor, so + // handing the container file to Sharp only ever produced an error per row. + query = query.where(function() { + this.whereNull('media_type').orWhere('media_type', '!=', 'video'); + }); const photos = await query; @@ -149,30 +175,38 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r for (const photo of photos) { try { - const storagePath = getStoragePath(); - const originalPath = path.join(storagePath, 'events/active', photo.path); - - // Check if original file exists - try { - await fs.access(originalPath); - } catch (err) { - logger.warn(`Original file not found for photo ${photo.id}: ${originalPath}`); - errorCount++; - continue; - } - - // Regenerate thumbnail - const thumbnailPath = await generateThumbnail(originalPath, { regenerate: true }); - - if (thumbnailPath) { - // Update database with new thumbnail path - await db('photos') - .where({ id: photo.id }) - .update({ - thumbnail_path: thumbnailPath, - updated_at: db.fn.now() + // Through ensureThumbnail, not a hand-rolled path (#1129). This route + // used to resolve every source as `storage/events/active/` and + // fs.access it — a location that does not exist for external or + // reference rows, whose originals live under events.external_path. So + // every one of them failed the check and was counted as an error: on a + // reference install the endpoint rebuilt nothing while the UI reported + // success, because the response is sent before this loop starts. + // + // ensureThumbnail already resolves both source kinds, uses the + // per-photo ext_ output name so two events referencing one NAS + // basename cannot clobber each other, and writes thumbnail_path back + // itself. Nulling thumbnail_path is what stops it short-circuiting on + // isThumbnailValid — necessary rather than cosmetic, because the old + // thumbnail is normally still readable at exactly the moment someone + // presses regenerate. + const newThumbnailPath = await ensureThumbnail({ ...photo, thumbnail_path: null }); + + if (newThumbnailPath) { + // Drop the superseded rendition when the key MOVED. On S3 the source + // is downloaded to a randomly-named temp file and, for non-RAW input, + // the key is derived from that name — so it differs every run, and + // nulling thumbnail_path hides the old key from everything that would + // otherwise clean it up. Guarded on the key actually changing: local + // storage is stable, and deleting the equal key would delete the file + // just written. + if (photo.thumbnail_path && !sameStorageKey(photo.thumbnail_path, newThumbnailPath)) { + await getStorage().delete(photo.thumbnail_path).catch((err) => { + logger.warn( + `Could not remove superseded thumbnail ${photo.thumbnail_path} for photo ${photo.id}: ${err.message}` + ); }); - + } successCount++; logger.info(`Regenerated thumbnail for photo ${photo.id}`); } else { diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 0f764a36..618718e5 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -133,10 +133,18 @@ async function generateThumbnail(imagePath, options = {}) { // Get thumbnail settings const settings = await getThumbnailSettings(); - // Force regeneration: drop the existing object before writing the new one - if (options.regenerate) { - await storage.delete(thumbnailRelKey).catch(() => {}); - } + // `options.regenerate` deliberately does NOT delete the existing object first + // (#1129). + // + // It used to, and the delete ran BEFORE sharp had even opened the source — so + // a source that could not be read (a NAS mount that blipped, a corrupt file) + // left the old thumbnail already gone and returned null, with the database + // still pointing at it. One bulk regeneration during a mount outage could + // therefore strip every canonical thumbnail in a reference gallery. + // + // Nothing is lost by dropping it: LocalFsStorage.put stages to a temp file and + // renames over the target, which replaces atomically, and an S3 put overwrites + // by key. The delete only added a window with no thumbnail at all. try { // First, verify the source image is complete and valid @@ -192,9 +200,12 @@ async function generateThumbnail(imagePath, options = {}) { const msg = (error && error.message) ? error.message : String(error); logger.error(`Failed to generate thumbnail for ${sourceBasename}: ${msg}`); - // Clean up any partially uploaded object - await storage.delete(thumbnailRelKey).catch(() => {}); - + // No cleanup delete here either, for the same reason (#1129). This was + // "clean up any partially uploaded object", but there cannot be one: + // storage.put is the LAST statement in the try, every throw above it + // happens before anything is written, and put unlinks its own temp file on + // failure. The only object this could remove is the PREVIOUS, valid + // rendition — exactly the thumbnail a failed regeneration must leave alone. return null; } }