fix(faces): scan external/reference photos instead of skipping them (#1090) (#1091)

* fix(faces): scan external/reference photos instead of skipping them (#1090)

faceProcessor short-circuited every photo with source_origin 'external'
or 'reference' straight to 'skipped', before the sidecar was ever
contacted. On an external-media install that is the entire library — the
reporter's gallery sat at 0/3230 with every row skipped and no error, and
a rescan changed nothing.

The guard was correct when written: resolvePhotoStorageKey returns null
for anything outside managed storage, so ensurePreviewImage could not
build a preview and there was nothing to send. #1078 removed that
limitation one release earlier — ensurePreviewImage now reads externals
straight off the mount via resolvePhotoFilePath and writes the preview
into managed storage, so the key faceProcessor already fetches through
getStorage() is readable like any other. The guard outlived its reason.

Photos whose source is genuinely gone still return a null preview key
and land in the existing 'failed' branch, which is the honest outcome:
that is a broken photo, not an unsupported one. The blanket skip was
absorbing those too.

No migration or manual reset needed — enqueueEvent already re-queues
rows with face_status in (NULL, 'failed', 'skipped'), so previously
skipped photos get picked up on the next scan.

* fix(faces): queue external imports for scanning (#1090)

The other half of the same bug, found by external review — and my first
counter-argument against it was wrong.

Managed uploads are enqueued by photoProcessor, which writes face_status
'pending' once a photo is processed (photoProcessor.js:573, commented as
"the only correct place to enqueue"). External media never goes through
photoProcessor at all: adminExternalMedia inserts rows directly, leaving
face_status NULL.

faceQueue.claimNextPhoto only claims 'pending' (faceQueue.js:64), so an
import into an already-enabled event produced nothing until someone
pressed Re-scan. Lifting the skip guard alone made external photos
scannable but still not scanned — which looks like a complete fix right
up until you import a photo.

Resolved once per import rather than per file, since it is a per-event
setting and the loop can run to a thousand files, and guarded on both
the global flag and the per-event toggle exactly as photoProcessor
guards it, so installs without the feature still never write a
face_status. A failure to read the setting logs and imports anyway — the
photos are the point.

No video guard: walkDir only collects jpg/jpeg/png/webp, so nothing
faceProcessor would skip as video can arrive through this route.

* fix(faces): enqueue imports only after the event path is written

External review caught a race I introduced in the previous commit.

Marking rows 'pending' as they were inserted published claimable work
while events.external_path still held the old value — or none at all, on
a first import, since the route only writes it after the entire
thumbnail loop. The face worker polls continuously, so on any import
long enough to matter (the loop is ~100-300ms per photo, and the
reporter's library is 6500+) it would claim those rows, resolve them
against the wrong directory and mark them permanently 'failed' — a state
only an explicit Re-scan clears. That is strictly worse than the
unscanned photos this set out to fix.

Ids are now collected during the loop and marked pending in one pass
after the event path is written, chunked at 500 because SQLite caps a
statement at 999 bound parameters.

The test now drives the real route instead of re-implementing its logic,
and observes the mid-loop state from inside the per-photo thumbnail
call — the only hook that can see the window the race lived in. Verified
it discriminates: deleting the enqueue fails two tests, and moving it
back onto the insert fails the ordering test specifically.

* fix(faces): read the face setting after the import, not before

Third external-review round. The setting was captured before a loop that
runs for many minutes on a large library, so an admin who enabled
detection during an import left every photo imported after that moment
at NULL forever — the toggle endpoint only queues rows that already
existed when it fired.

Ids are now collected unconditionally and the setting is evaluated
immediately before the queue update, off a freshly read event row. The
guard is unchanged in substance: both the global flag and the per-event
toggle, so installs without the feature still never write a face_status.

Test flips the toggle from inside the mocked per-photo thumbnail call,
which is the same mid-loop hook the ordering test uses.

---------

Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>

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);
+  });
+});

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);
+  });
+});

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 },

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.