diff --git a/backend/__tests__/integration/faceExternalImportEnqueue.test.js b/backend/__tests__/integration/faceExternalImportEnqueue.test.js new file mode 100644 index 00000000..b6485430 --- /dev/null +++ b/backend/__tests__/integration/faceExternalImportEnqueue.test.js @@ -0,0 +1,198 @@ +/** + * External imports are queued for face scanning, in the right order (#1090). + * + * Managed uploads are enqueued by photoProcessor, which writes face_status + * 'pending' once a photo is processed (photoProcessor.js:573 — "the only + * correct place to enqueue"). External media never goes through photoProcessor: + * adminExternalMedia inserts rows directly, so they stayed NULL and were only + * ever picked up by a manual Re-scan. + * + * The ordering matters as much as the enqueue. events.external_path is written + * only AFTER the whole import loop, so marking rows 'pending' as they are + * inserted publishes claimable work while the event still points at the old + * directory — or none at all, on a first import. The face worker polls + * continuously, would resolve those photos against the wrong path, and mark + * them permanently 'failed', a state only an explicit Re-scan clears. + * + * This drives the real route rather than re-implementing it, so removing the + * enqueue fails the first test and moving it back onto the insert fails the + * second. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const express = require('express'); +const request = require('supertest'); + +describe('external import queues faces (#1090)', () => { + let tmpDir; let db; let app; let mediaRoot; + // Recorded from inside the per-photo thumbnail call, i.e. mid-loop. + let pendingSeenDuringLoop = 0; + let externalPathDuringLoop; + // When set to an event id, the mocked thumbnail call turns detection on + // mid-loop, standing in for an admin flipping the toggle during an import. + let flipFacesOnDuringLoop = null; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-extenq-')); + mediaRoot = path.join(tmpDir, 'media'); + await fs.promises.mkdir(path.join(mediaRoot, 'nas', 'individual'), { recursive: true }); + for (const name of ['a.jpg', 'b.jpg', 'c.jpg']) { + await fs.promises.writeFile(path.join(mediaRoot, 'nas', 'individual', name), 'not-a-real-jpeg'); + } + + process.env.NODE_ENV = 'test'; + process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'db.sqlite'); + await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true }); + process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); + process.env.EXTERNAL_MEDIA_ROOT = mediaRoot; + process.env.JWT_SECRET = process.env.JWT_SECRET || 'extenq-secret'; + + jest.resetModules(); + + jest.doMock('../../src/middleware/auth', () => ({ + adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester', roleName: 'admin' }; next(); }, + })); + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + })); + jest.doMock('../../src/middleware/ownership', () => ({ + requireEventOwnership: (_req, _res, next) => next(), + })); + + // Runs once per photo, inside the import loop — the only hook that can + // observe the intermediate state the ordering bug would expose. + jest.doMock('../../src/services/imageProcessor', () => ({ + generateThumbnail: jest.fn(async () => { + const { db: liveDb } = require('../../src/database/db'); + const rows = await liveDb('photos').where({ face_status: 'pending' }); + pendingSeenDuringLoop += rows.length; + const ev = await liveDb('events').first(); + externalPathDuringLoop = ev ? ev.external_path : undefined; + if (flipFacesOnDuringLoop) { + await liveDb('events').where({ id: flipFacesOnDuringLoop }) + .update({ face_recognition_enabled: true }); + } + return 'thumbnails/mock.jpg'; + }), + ensureThumbnail: jest.fn(), + })); + + jest.doMock('../../src/utils/logger', () => ({ + debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), + })); + + // bootCrmDb runs every migrations/core/*.up() directly — knex's Migrator + // deadlocks on 001_init's nested initializeDatabase() call. + ({ db } = await require('./helpers/crmDb').bootCrmDb()); + + app = express(); + app.use(express.json()); + app.use('/api/admin/external-media', require('../../src/routes/adminExternalMedia')); + }, 180000); + + afterAll(async () => { + if (db) await db.destroy?.(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + async function seedEvent({ facesEnabled, flagOn }) { + await db('feature_flags').insert({ key: 'faces', value: flagOn }) + .onConflict('key').merge() + .catch(async () => { await db('feature_flags').where({ key: 'faces' }).update({ value: flagOn }); }); + // The flag read is TTL-cached (requireFeatureFlag.js:26-34); production + // invalidates after every write, and so must this. + require('../../src/middleware/requireFeatureFlag').invalidateFeatureFlagCache(); + + await db('photos').del(); + await db('events').del(); + const [e] = await db('events').insert({ + slug: `extenq-${Math.random().toString(36).slice(2, 8)}`, + event_type: 'wedding', + event_name: 'extenq', + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `extenq-${Math.random()}`, + expires_at: new Date().toISOString(), + face_recognition_enabled: facesEnabled, + source_mode: 'reference', + }).returning('id'); + + pendingSeenDuringLoop = 0; + externalPathDuringLoop = undefined; + return typeof e === 'object' ? e.id : e; + } + + async function runImport(eventId) { + return request(app) + .post(`/api/admin/external-media/events/${eventId}/import-external`) + .send({ external_path: 'nas', recursive: true }); + } + + it('queues imported photos when detection is on', async () => { + const eventId = await seedEvent({ facesEnabled: true, flagOn: true }); + + const res = await runImport(eventId); + expect(res.status).toBe(200); + + const photos = await db('photos').where({ event_id: eventId }); + expect(photos.length).toBeGreaterThan(0); + // The regression: these stayed NULL and waited for a manual Re-scan. + expect(photos.every((p) => p.face_status === 'pending')).toBe(true); + }); + + it('does not publish claimable rows before events.external_path is written', async () => { + const eventId = await seedEvent({ facesEnabled: true, flagOn: true }); + + await runImport(eventId); + + // Observed from inside the loop: nothing may be claimable yet, because the + // event still resolves to the old (here: empty) directory. Marking rows on + // insert would make this non-zero and leave photos permanently 'failed'. + expect(pendingSeenDuringLoop).toBe(0); + expect(externalPathDuringLoop).toBeFalsy(); + + // ...and afterwards both are in place. + const ev = await db('events').where({ id: eventId }).first(); + expect(ev.external_path).toBe('nas'); + expect((await db('photos').where({ event_id: eventId, face_status: 'pending' })).length) + .toBe((await db('photos').where({ event_id: eventId })).length); + }); + + it('honours a toggle flipped DURING the import', async () => { + // The setting is read after the loop, not before: on a large library the + // loop runs for minutes, and the toggle endpoint only queues rows that + // already existed when it fired. Reading it up front would strand every + // photo imported after that moment at NULL forever. + const eventId = await seedEvent({ facesEnabled: false, flagOn: true }); + flipFacesOnDuringLoop = eventId; + + await runImport(eventId); + flipFacesOnDuringLoop = null; + + const photos = await db('photos').where({ event_id: eventId }); + expect(photos.length).toBeGreaterThan(0); + expect(photos.every((p) => p.face_status === 'pending')).toBe(true); + }); + + it('leaves face_status untouched when the per-event toggle is off', async () => { + const eventId = await seedEvent({ facesEnabled: false, flagOn: true }); + await runImport(eventId); + const photos = await db('photos').where({ event_id: eventId }); + expect(photos.length).toBeGreaterThan(0); + expect(photos.every((p) => p.face_status === null)).toBe(true); + }); + + it('leaves face_status untouched when the global flag is off', async () => { + // Installs without the feature must never accumulate face_status rows — + // the same invariant photoProcessor's guard protects. + const eventId = await seedEvent({ facesEnabled: true, flagOn: false }); + await runImport(eventId); + const photos = await db('photos').where({ event_id: eventId }); + expect(photos.length).toBeGreaterThan(0); + expect(photos.every((p) => p.face_status === null)).toBe(true); + }); +}); diff --git a/backend/__tests__/integration/faceExternalPhotos.test.js b/backend/__tests__/integration/faceExternalPhotos.test.js new file mode 100644 index 00000000..4a097ddb --- /dev/null +++ b/backend/__tests__/integration/faceExternalPhotos.test.js @@ -0,0 +1,148 @@ +/** + * External / reference photos are scannable (#1090). + * + * faceProcessor used to short-circuit every photo with source_origin + * 'external' or 'reference' to 'skipped', because resolvePhotoStorageKey + * returns null for anything outside managed storage and ensurePreviewImage + * could not build a preview for it. #1078 removed that limitation — + * ensurePreviewImage now reads externals straight off the mount and writes + * the preview into managed storage — but the guard stayed, so the whole + * feature was a no-op on external-media installs. The reporter's gallery sat + * at 0/3230 with every row 'skipped' and no error. + * + * These pin both halves: the guard is gone, and a photo whose source is + * genuinely missing still fails rather than being quietly skipped — the blanket + * skip used to absorb that case too, so a real breakage looked like an + * unsupported one. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-faceext-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'faceext-test-secret'; + +const sharp = require('sharp'); + +let mockPreviewBuffer; +// Set per-test: what ensurePreviewImage returns for the photo under test. +let previewKeyResult; // eslint-disable-line prefer-const +const mockEnsurePreviewImage = jest.fn(async () => previewKeyResult); +const mockDetectFaces = jest.fn(); + +jest.mock('../../src/services/imageProcessor', () => ({ + ...jest.requireActual('../../src/services/imageProcessor'), + ensurePreviewImage: (...args) => mockEnsurePreviewImage(...args), +})); + +jest.mock('../../src/services/storage', () => ({ + getStorage: () => ({ get: async () => mockPreviewBuffer }), +})); + +jest.mock('../../src/services/faceClient', () => ({ + detectFaces: (...args) => mockDetectFaces(...args), + SidecarUnavailableError: class extends Error {}, +})); + +const { bootCrmDb } = require('./helpers/crmDb'); + +let db; let cleanup; let faceProcessor; + +async function seedPhoto({ sourceOrigin = 'managed', sourceMode = 'managed' } = {}) { + const [e] = await db('events').insert({ + slug: `ext-${Math.random().toString(36).slice(2, 8)}`, + event_type: 'wedding', + event_name: 'ext', + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `ext-${Math.random()}`, + expires_at: new Date().toISOString(), + face_recognition_enabled: true, + source_mode: sourceMode, + }).returning('id'); + const eventId = typeof e === 'object' ? e.id : e; + + const [p] = await db('photos').insert({ + event_id: eventId, + filename: 'ext.jpg', + path: '/tmp/ext.jpg', + type: 'individual', + width: 1920, + height: 1440, + processing_status: 'complete', + face_status: 'processing', + source_origin: sourceOrigin, + external_relpath: sourceOrigin === 'managed' ? null : 'nas/ext.jpg', + }).returning('id'); + return { eventId, photoId: typeof p === 'object' ? p.id : p }; +} + +describe('face scanning of external/reference photos (#1090)', () => { + beforeAll(async () => { + mockPreviewBuffer = await sharp({ + create: { width: 1920, height: 1440, channels: 3, background: { r: 20, g: 40, b: 80 } }, + }).jpeg().toBuffer(); + + ({ db, cleanup } = await bootCrmDb()); + await db('feature_flags').insert({ key: 'faces', value: true }) + .onConflict('key').merge() + .catch(async () => { await db('feature_flags').where({ key: 'faces' }).update({ value: true }); }); + faceProcessor = require('../../src/services/faceProcessor'); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + beforeEach(() => { + mockEnsurePreviewImage.mockClear(); + mockDetectFaces.mockClear(); + previewKeyResult = 'previews/preview_ext.jpg'; + mockDetectFaces.mockResolvedValue({ + model_version: 'test-v1', + faces: [{ + bbox: [100, 100, 50, 50], + score: 0.99, + landmarks: [[0, 0], [0, 0], [0, 0], [0, 0], [0, 0]], + yaw: 0, pitch: 0, blur: 500, + embedding: Array.from({ length: 64 }, (_, i) => (i === 0 ? 1 : 0)), + }], + }); + }); + + it.each(['external', 'reference'])('scans a %s photo instead of skipping it', async (origin) => { + const { photoId } = await seedPhoto({ sourceOrigin: origin, sourceMode: 'reference' }); + + const result = await faceProcessor.processPhotoFaces(photoId); + + // The regression: this used to return 'skipped' without ever building a + // preview or contacting the sidecar. + expect(result.status).not.toBe('skipped'); + expect(mockEnsurePreviewImage).toHaveBeenCalled(); + expect(mockDetectFaces).toHaveBeenCalled(); + + const photo = await db('photos').where({ id: photoId }).first(); + expect(photo.face_status).toBe('done'); + expect(await db('photo_faces').where({ photo_id: photoId }).first()).toBeTruthy(); + }); + + it('fails, not skips, when the external source is genuinely gone', async () => { + // A missing file is a property of that photo, so it should be visible as a + // failure the admin can act on — not silently absorbed the way the old + // blanket skip did. + previewKeyResult = null; + const { photoId } = await seedPhoto({ sourceOrigin: 'external', sourceMode: 'reference' }); + + const result = await faceProcessor.processPhotoFaces(photoId); + + expect(result.status).toBe('failed'); + expect(mockDetectFaces).not.toHaveBeenCalled(); + const photo = await db('photos').where({ id: photoId }).first(); + expect(photo.face_status).toBe('failed'); + expect(photo.face_error).toMatch(/preview/i); + }); +}); diff --git a/backend/src/routes/adminExternalMedia.js b/backend/src/routes/adminExternalMedia.js index de9e6c06..8bebb442 100644 --- a/backend/src/routes/adminExternalMedia.js +++ b/backend/src/routes/adminExternalMedia.js @@ -98,6 +98,30 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. let thumbnailsGenerated = 0; let thumbnailsFailed = 0; + // Face detection (#1090). Managed uploads are enqueued by photoProcessor, + // which sets face_status 'pending' once a photo is processed + // (photoProcessor.js:573) — but external media never goes through it, it + // is inserted directly here. Before #1090 that was invisible, because + // faceProcessor skipped externals anyway; now that they are scannable, an + // import into an already-enabled event would still sit unscanned until + // someone pressed Re-scan. + // + // Ids are collected unconditionally and the setting is read at the END, + // not here: this loop can run for many minutes on a large library, and an + // admin who enables detection during it would otherwise leave every photo + // imported after that moment stuck at NULL forever — the toggle endpoint + // only queues rows that already existed when it fired. + // + // Collected during the loop and marked 'pending' only AFTER + // events.external_path is written below. Setting it on insert would publish + // claimable rows while the event still carries the old path — or none, on a + // first import: the face worker polls continuously, resolvePhotoFilePath + // would resolve against the wrong directory, and a multi-minute import + // would leave photos permanently 'failed', a state only an explicit + // Re-scan clears. No video guard needed either way: walkDir collects only + // jpg/jpeg/png/webp. + const importedPhotoIds = []; + // Insert photos for (const f of dedupeMap.values()) { // Infer type by subfolder names @@ -169,6 +193,7 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. } } + if (photoId != null) importedPhotoIds.push(photoId); imported += (inserted?.length ? 1 : 0); } catch (e) { skipped++; @@ -178,6 +203,32 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. // Update event fields await db('events').where('id', eventId).update({ source_mode: 'reference', external_path }); + // Only now does the event resolve to the new directory, so only now is it + // safe to open the queue. Guarded the same way photoProcessor guards it + // (both the global flag and the per-event toggle), so installs without the + // feature still never write a face_status. Re-read here rather than before + // the loop so a toggle flipped mid-import is honoured. + let queueFaces = false; + try { + const { isEnabledForEvent } = require('../services/faceSettings'); + const freshEvent = await db('events').where('id', eventId).first(); + queueFaces = await isEnabledForEvent(freshEvent); + } catch (err) { + // Never let the face feature break an import — the photos are the point. + logger.warn(`Could not resolve face settings for event ${eventId}: ${err.message}`); + } + + // Chunked because SQLite caps a statement at 999 bound parameters and an + // import can be far larger than that. + if (queueFaces && importedPhotoIds.length) { + for (let i = 0; i < importedPhotoIds.length; i += 500) { + await db('photos') + .whereIn('id', importedPhotoIds.slice(i, i + 500)) + .update({ face_status: 'pending' }); + } + logger.info(`Queued ${importedPhotoIds.length} imported external photo(s) for face scanning (event ${eventId})`); + } + await logActivity( 'external_import_completed', { event_id: eventId, imported, skipped, thumbnailsGenerated, thumbnailsFailed, external_path }, diff --git a/backend/src/services/faceProcessor.js b/backend/src/services/faceProcessor.js index 482dda54..92e6f4ed 100644 --- a/backend/src/services/faceProcessor.js +++ b/backend/src/services/faceProcessor.js @@ -75,18 +75,18 @@ async function processPhotoFaces(photoId) { return { status: 'skipped' }; } - // External / reference photos live outside managed storage, and - // resolvePhotoStorageKey returns null for them by design (photoResolver.js). - // ensurePreviewImage therefore cannot build a preview, so scanning them is - // unsupported rather than broken — 'skipped', not 'failed', so an external - // gallery does not report every photo as an error. - if (photo.source_origin === 'external' || photo.source_origin === 'reference') { - await db('photos').where({ id: photoId }).update({ - face_status: 'skipped', face_started_at: null, face_error: null, - }); - return { status: 'skipped' }; - } - + // No external/reference guard here on purpose (#1090). One used to skip + // them, because resolvePhotoStorageKey returns null for photos outside + // managed storage and ensurePreviewImage could not build a preview. #1078 + // removed that limitation: ensurePreviewImage now reads externals straight + // off the mount via resolvePhotoFilePath and writes the preview into + // managed storage, so the key below is readable like any other. Keeping the + // guard made the whole feature a no-op on external-media installs, where + // every photo in the library can be external. + // + // A photo whose source really is gone still returns null and lands in the + // 'failed' branch below, which is the honest outcome — that is a broken + // photo, not an unsupported one. const previewKey = await ensurePreviewImage(photo); if (!previewKey) { // No preview and no way to make one — the source is missing or corrupt.