0b886ed942
* fix(faces): defer on unreachable storage, and commit the import path first The two follow-ups left open by #1091, both consequences of external photos becoming scannable at all. **A dropped mount no longer burns the gallery.** ensurePreviewImage returns null for "this JPEG is corrupt" and "the NFS share is gone" alike, and faceProcessor marked both 'failed'. Nothing re-queues a failure automatically and the queue only ever claims 'pending', so a mount that blinked mid-scan cost the whole event a manual Re-scan — on external libraries, where network storage drops far more often than local disk, that is the common case rather than the corner one. faceProcessor now probes the containing DIRECTORY before failing, and throws TransientSourceError when it cannot be reached; faceQueue treats that exactly like SidecarUnavailableError — release to pending, back off, retry — with the warning rate-limited to one per five minutes, since an outage hits every photo in the event. The directory rather than the file is the whole point: a missing file inside a healthy directory is a broken photo and should still fail, and it still does. Anything that goes wrong deciding which case it is falls through to 'failed', because guessing 'transient' on an unknown condition would retry forever. **The import commits its path before inserting rows.** enqueueEvent accepts processing_status NULL (faceProcessor.js:243-246), which these inserts leave unset, so an admin hitting the toggle or Re-scan during a long import could queue partial rows while the event still resolved against the old directory — and burn them to 'failed'. Moving events.external_path ahead of the loop closes that, and fixes a pre-existing bug on the same line: an import that died at photo 500 of 1000 used to leave those 500 rows pointing into the new tree while the event still resolved against the old one, making every one of them unreadable. Safe to do first because the path is already validated above, and existing photos are unaffected — photo.source_origin takes precedence over event.source_mode in both resolvers and is NOT NULL defaulting to 'managed'. The #1090 test that asserted a missing source fails needed its setup corrected rather than its intent: it created no directory at all, which is now (correctly) a dropped mount. It now creates one, so it tests the case it always meant to — healthy storage, dead photo. Verified both fixes discriminate: removing the probe fails the defer test, and moving the event update back after the loop fails the ordering test. Face suite 59 passed across 8 suites; the full backend suite fails the same 10 pre-existing suites as unmodified main, no more. * fix(faces): stop a dead mount stalling the whole queue External review, and the first finding is one my own change created. Deferring by returning the row to 'pending' was a trap: claimNextPhoto orders by id ascending and the queue defaults to a single worker, so the same unreachable row becomes the oldest pending one after every backoff and the worker never reaches a higher id. One dead mount would have stalled face scanning for the entire install — unrelated events, fresh uploads, everything. Strictly worse than the permanent 'failed' this set out to replace. The row is now left parked in 'processing' with face_started_at intact. It is not claimable, so the worker moves straight on; the janitor that already exists returns it to 'pending' past STUCK_TIMEOUT_MS, which is the retry. No new column and no new timer. The sidecar branch still releases, because a down sidecar blocks every photo anyway — there is no other work to get on with. Second: probing existence was not enough. Unmounting an NFS or SMB share usually leaves the mountpoint behind as an ordinary empty directory, so fs.access succeeded on storage that was entirely gone and the photo was failed anyway — the exact case this was written for. An empty directory where the photo should live now counts as unreachable. The trade is deliberate and documented: a directory an admin genuinely emptied is retried rather than failed, which now costs one attempt per janitor sweep and nothing else. Third: a comment in adminExternalMedia claimed source_origin isolates existing photos from the early external_path update. That is true of managed rows and false of external ones — resolveExternalPath prefixes every external row with event.external_path, so importing folder B into an event referencing folder A rebases the A rows. Pre-existing rather than introduced here (the update always did this, just later), but the comment asserted otherwise, so it now says what actually happens and names the underlying single-base-path limitation. The deferral test initially passed against the blocking version too — database state alone cannot tell the fix from the bug. It now inspects the branch directly, the way the #596 contract tests do, and fails when releaseToPending is put back or the two branches are merged. * fix(faces): back off per event, and stop clobbering concurrent scans Round two of external review. Parking a row in 'processing' fixed the head-of-line block but not the cost: every janitor sweep handed the whole dead gallery back, and the worker walked all of it again — one stat per photo against storage that may be hard-mounted and slow to time out — before reaching any healthy event. Every one of those attempts also went through generatePreviewImage first, which logs an error per photo, so a down mount produced a recurring flood that the rate-limited warning did nothing about. So the backoff is now per EVENT and separate from the janitor: TransientSourceError carries the event id, the queue records a cooldown, and claimNextPhoto excludes those events while it lasts. The janitor keeps doing its own job, which is rescuing rows a crashed worker abandoned. Cooldown is in memory on purpose — a restart is usually what follows fixing a mount, so it should retry at once. Second: committing the event path before the loop means a toggle or Re-scan firing mid-import can now genuinely queue and finish some of those rows. The final bulk update was unconditional, so it dragged 'done' rows back to 'pending' for a duplicate sidecar scan and knocked 'processing' rows out from under the worker. It is now whereNull — only rows nothing has touched are ours to queue. Also corrected a comment of mine that had gone stale in the same file: it still described the enqueue as happening after the event path was written "below", which stopped being true when that update moved above the loop. * fix(faces): judge the mount, the path and the file separately Round three of external review. The probe was too coarse in both directions. It read any ENOENT on the photo's own directory as a mount-wide outage, so a deleted or renamed subfolder — individual/ gone while collages/ is healthy — deferred the entire event and starved every sibling folder, renewing the cooldown on each retry. It now judges the EVENT ROOT for that verdict: root missing, or present-but-empty, is an outage; anything below a populated root is a broken path and fails. And it read a listable directory as proof the photo was at fault, so EACCES on a reconnected share, EIO, or the classic NFS ESTALE handle were burnt as permanent failures. Only ENOENT now means genuinely gone; any other error opening the file defers. The event-wide backoff was also too broad. A reference event can hold managed uploads alongside imported external ones, and those live in local storage that is fine — excluding the whole event id left them unscanned for as long as external rows kept renewing the cooldown, which during a real outage is indefinitely. The exclusion is now scoped to external and reference rows. One of my own tests had modelled the unmount wrongly: it emptied the photo's subdirectory rather than the event root, which under the corrected logic is a populated mount with a missing folder — a failure, not an outage. It now empties the root, which is what an unmount actually leaves behind. Dropped the path require the first version of this probe needed; the event-root form does not. All three fixes mutation-checked: reverting each one fails the test written for it. Face suite 69 passed across 9 suites; full backend suite fails the same 10 pre-existing suites as main, no more. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
227 lines
9.3 KiB
JavaScript
227 lines
9.3 KiB
JavaScript
/**
|
|
* A deferred photo must not stall the queue.
|
|
*
|
|
* claimNextPhoto orders by id ascending, and the queue defaults to a single
|
|
* worker. So returning an unreachable photo to 'pending' — the obvious way to
|
|
* say "try again later" — makes that same row the oldest pending one forever:
|
|
* the worker reclaims it after every backoff and never reaches a higher id.
|
|
* One dead mount would stall face scanning for the entire install, including
|
|
* unrelated events and fresh uploads.
|
|
*
|
|
* The row is instead left parked in 'processing' with its face_started_at
|
|
* intact. It is not claimable, so the worker advances; the existing janitor
|
|
* returns it to 'pending' after STUCK_TIMEOUT_MS, which is the retry.
|
|
*/
|
|
|
|
const path = require('path');
|
|
const fs = require('fs');
|
|
const os = require('os');
|
|
|
|
process.env.NODE_ENV = 'test';
|
|
const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-defer-'));
|
|
process.env.TEST_DATABASE_PATH = path.join(tmpRoot, 'db.sqlite');
|
|
process.env.JWT_SECRET = process.env.JWT_SECRET || 'defer-test-secret';
|
|
|
|
const { bootCrmDb } = require('./helpers/crmDb');
|
|
|
|
let db; let cleanup; let faceQueue; let faceProcessor;
|
|
|
|
describe('deferred photos do not block the queue', () => {
|
|
beforeAll(async () => {
|
|
({ db, cleanup } = await bootCrmDb());
|
|
faceQueue = require('../../src/services/faceQueue');
|
|
faceProcessor = require('../../src/services/faceProcessor');
|
|
}, 120000);
|
|
|
|
afterAll(async () => {
|
|
if (cleanup) await cleanup();
|
|
await fs.promises.rm(tmpRoot, { recursive: true, force: true }).catch(() => {});
|
|
});
|
|
|
|
it('exports TransientSourceError for the queue to branch on', () => {
|
|
// The queue imports this from faceProcessor; if the export is dropped the
|
|
// instanceof check silently becomes false and every deferral turns back
|
|
// into a permanent failure.
|
|
expect(typeof faceProcessor.TransientSourceError).toBe('function');
|
|
expect(new faceProcessor.TransientSourceError(1, 'x'))
|
|
.toBeInstanceOf(Error);
|
|
});
|
|
|
|
it('does NOT return a deferred row to pending', () => {
|
|
// Source inspection, deliberately. workerLoop is an unexported infinite
|
|
// loop, so the branch cannot be driven directly, and asserting on database
|
|
// state alone does not distinguish the fix from the bug — a version that
|
|
// re-queues the row passes every state assertion in this file. What
|
|
// actually matters is that this one branch does not call releaseToPending,
|
|
// so that is what is pinned. Same approach as the contract tests added for
|
|
// #596.
|
|
const src = fs.readFileSync(
|
|
path.join(__dirname, '..', '..', 'src', 'services', 'faceQueue.js'), 'utf8'
|
|
);
|
|
|
|
const marker = 'if (err instanceof TransientSourceError) {';
|
|
const start = src.indexOf(marker);
|
|
expect(start).toBeGreaterThan(-1);
|
|
|
|
// The branch body, up to its closing brace.
|
|
const body = src.slice(start, src.indexOf('\n }', start));
|
|
expect(body).not.toMatch(/releaseToPending/);
|
|
expect(body).toMatch(/continue/);
|
|
|
|
// And the sidecar branch, which SHOULD still release, so this test fails
|
|
// if the two branches are ever collapsed back together.
|
|
const sideStart = src.indexOf('if (err instanceof SidecarUnavailableError) {');
|
|
expect(sideStart).toBeGreaterThan(-1);
|
|
const sideBody = src.slice(sideStart, src.indexOf('\n }', sideStart));
|
|
expect(sideBody).toMatch(/releaseToPending/);
|
|
});
|
|
|
|
it('leaves a deferred row claimable-later, not claimable-now', async () => {
|
|
// A row parked in 'processing' is invisible to claimNextPhoto, which only
|
|
// ever selects face_status='pending' — that is what lets the worker move
|
|
// past it instead of spinning on it.
|
|
const [e] = await db('events').insert({
|
|
slug: `defer-${Math.random().toString(36).slice(2, 8)}`,
|
|
event_type: 'wedding',
|
|
event_name: 'defer',
|
|
event_date: '2026-01-01',
|
|
host_email: 'h@example.com',
|
|
admin_email: 'a@example.com',
|
|
password_hash: 'x',
|
|
share_link: `defer-${Math.random()}`,
|
|
expires_at: new Date().toISOString(),
|
|
face_recognition_enabled: true,
|
|
}).returning('id');
|
|
const eventId = typeof e === 'object' ? e.id : e;
|
|
|
|
const [stuck] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'stuck.jpg',
|
|
path: 'd/stuck.jpg',
|
|
type: 'individual',
|
|
processing_status: 'complete',
|
|
face_status: 'processing',
|
|
face_started_at: new Date().toISOString(),
|
|
source_origin: 'external',
|
|
}).returning('id');
|
|
const stuckId = typeof stuck === 'object' ? stuck.id : stuck;
|
|
|
|
const parked = await db('photos')
|
|
.where({ id: stuckId, face_status: 'pending' })
|
|
.first();
|
|
expect(parked).toBeUndefined(); // not claimable while parked
|
|
|
|
// The janitor's contract is what turns the park into a retry: it resets
|
|
// 'processing' rows whose face_started_at is older than the stuck timeout.
|
|
// Backdate past it and the row becomes claimable again.
|
|
const longAgo = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString();
|
|
await db('photos').where({ id: stuckId }).update({ face_started_at: longAgo });
|
|
|
|
const cutoff = new Date(Date.now() - 600000).toISOString();
|
|
const reset = await db('photos')
|
|
.where('face_status', 'processing')
|
|
.where('face_started_at', '<', cutoff)
|
|
.update({ face_status: 'pending', face_started_at: null });
|
|
|
|
expect(reset).toBeGreaterThan(0);
|
|
const after = await db('photos').where({ id: stuckId }).first();
|
|
expect(after.face_status).toBe('pending');
|
|
});
|
|
|
|
it('claimNextPhoto skips events inside their backoff window', async () => {
|
|
// The per-event cooldown is what stops the janitor handing a whole dead
|
|
// gallery back every sweep. Without the exclusion the worker walks all of
|
|
// it again — one slow stat per photo against a possibly hard-mounted
|
|
// share — before reaching any healthy event.
|
|
const mk = async (name) => {
|
|
const [e] = await db('events').insert({
|
|
slug: `cd-${name}-${Math.random().toString(36).slice(2, 8)}`,
|
|
event_type: 'wedding',
|
|
event_name: name,
|
|
event_date: '2026-01-01',
|
|
host_email: 'h@example.com',
|
|
admin_email: 'a@example.com',
|
|
password_hash: 'x',
|
|
share_link: `cd-${name}-${Math.random()}`,
|
|
expires_at: new Date().toISOString(),
|
|
face_recognition_enabled: true,
|
|
}).returning('id');
|
|
const eventId = typeof e === 'object' ? e.id : e;
|
|
const [p2] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: `${name}.jpg`,
|
|
path: `cd/${name}.jpg`,
|
|
type: 'individual',
|
|
processing_status: 'complete',
|
|
face_status: 'pending',
|
|
source_origin: 'external',
|
|
}).returning('id');
|
|
return { eventId, photoId: typeof p2 === 'object' ? p2.id : p2 };
|
|
};
|
|
|
|
await db('photos').del();
|
|
const dead = await mk('dead'); // lower id -> would win the FIFO
|
|
const healthy = await mk('healthy');
|
|
|
|
// Without exclusion the dead event's row is claimed first...
|
|
const first = await faceQueue.claimNextPhoto([]);
|
|
expect(first.id).toBe(dead.photoId);
|
|
await db('photos').where({ id: dead.photoId }).update({ face_status: 'pending' });
|
|
|
|
// ...and with it, the worker reaches the healthy event instead.
|
|
const second = await faceQueue.claimNextPhoto([dead.eventId]);
|
|
expect(second.id).toBe(healthy.photoId);
|
|
});
|
|
|
|
it('backoff spares managed rows in a mixed-source event', async () => {
|
|
// A reference event can hold managed uploads alongside imported external
|
|
// ones. Excluding the whole event id would leave those unscanned for as
|
|
// long as external rows keep renewing the cooldown — indefinitely, during
|
|
// a real outage — even though their local source is fine.
|
|
await db('photos').del();
|
|
const [e] = await db('events').insert({
|
|
slug: `mix-${Math.random().toString(36).slice(2, 8)}`,
|
|
event_type: 'wedding',
|
|
event_name: 'mix',
|
|
event_date: '2026-01-01',
|
|
host_email: 'h@example.com',
|
|
admin_email: 'a@example.com',
|
|
password_hash: 'x',
|
|
share_link: `mix-${Math.random()}`,
|
|
expires_at: new Date().toISOString(),
|
|
face_recognition_enabled: true,
|
|
source_mode: 'reference',
|
|
}).returning('id');
|
|
const eventId = typeof e === 'object' ? e.id : e;
|
|
|
|
const add = async (origin, name) => {
|
|
const [p2] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: name,
|
|
path: `mix/${name}`,
|
|
type: 'individual',
|
|
processing_status: 'complete',
|
|
face_status: 'pending',
|
|
source_origin: origin,
|
|
}).returning('id');
|
|
return typeof p2 === 'object' ? p2.id : p2;
|
|
};
|
|
await add('external', 'ext.jpg'); // lower id, would win the FIFO
|
|
const managedId = await add('managed', 'man.jpg');
|
|
|
|
// Event is in backoff: the external row is skipped, the managed one is not.
|
|
const claimed = await faceQueue.claimNextPhoto([eventId]);
|
|
expect(claimed).toBeTruthy();
|
|
expect(claimed.id).toBe(managedId);
|
|
});
|
|
|
|
it('startQueue is exported and does not throw on import', () => {
|
|
// faceQueue requires faceProcessor for TransientSourceError while
|
|
// faceProcessor is itself required by the routes — a circular require here
|
|
// would surface as an undefined export rather than a crash, so assert the
|
|
// module actually loaded something usable.
|
|
expect(faceQueue).toBeTruthy();
|
|
expect(Object.keys(faceQueue).length).toBeGreaterThan(0);
|
|
});
|
|
});
|