576924fa57
* 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>
199 lines
8.5 KiB
JavaScript
199 lines
8.5 KiB
JavaScript
/**
|
|
* 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);
|
|
});
|
|
});
|