diff --git a/backend/__tests__/integration/faceMergeSuggestions.test.js b/backend/__tests__/integration/faceMergeSuggestions.test.js new file mode 100644 index 00000000..2bff6316 --- /dev/null +++ b/backend/__tests__/integration/faceMergeSuggestions.test.js @@ -0,0 +1,452 @@ +/** + * Automatic consolidation reporting and the suggestion band (#1107). + * + * Centroids are built to an EXACT cosine similarity rather than jittered + * towards one, because every assertion here is about which side of a threshold + * a pair falls on. `pairAtSimilarity` returns two unit vectors whose dot + * product is the requested number to floating-point precision, and each pair + * is built on its own orthogonal basis so two different pairs are never + * accidentally similar to each other. + */ + +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-facesuggest-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'facesuggest-test-secret'; + +const { bootCrmDb } = require('./helpers/crmDb'); + +let db; let cleanup; let clustering; + +// Mirrors the service: merge at match + 0.08, so with a 0.60 floor the +// suggestion band is [0.60, 0.68). +const THRESHOLDS = { + face_match_threshold: 0.6, + face_quality_min_score: 0.7, + face_quality_min_px: 40, +}; + +const DIM = 64; + +/** Two unit vectors whose dot product is exactly `target`, on basis (i, i+1). */ +function pairAtSimilarity(target, basis) { + const a = new Float32Array(DIM); + const b = new Float32Array(DIM); + const orth = Math.sqrt(1 - target * target); + a[basis] = 1; + b[basis] = target; + b[basis + 1] = orth; + return [a, b]; +} + +async function seedEvent(slug) { + const [row] = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: slug, + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `${slug}-share`, + expires_at: new Date().toISOString(), + }).returning('id'); + return typeof row === 'object' ? row.id : row; +} + +async function insertPerson(eventId, centroid, overrides = {}) { + const [row] = await db('event_people').insert({ + event_id: eventId, + centroid: clustering.packEmbedding(centroid), + face_count_total: 5, + model_version: 'test-v1', + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + ...overrides, + }).returning('id'); + return typeof row === 'object' ? row.id : row; +} + +/** One person with one real face, so merge/split have something to move. */ +async function insertPersonWithFace(eventId, centroid, overrides = {}) { + const personId = await insertPerson(eventId, centroid, overrides); + const [p] = await db('photos').insert({ + event_id: eventId, + filename: `${Math.random()}.jpg`, + path: '/tmp/x.jpg', + type: 'individual', + }).returning('id'); + const photoId = typeof p === 'object' ? p.id : p; + await db('photo_faces').insert({ + photo_id: photoId, + event_id: eventId, + person_id: personId, + bbox_x: 0, bbox_y: 0, bbox_w: 200, bbox_h: 200, + det_score: 0.99, + embedding: clustering.packEmbedding(centroid), + model_version: 'test-v1', + created_at: new Date().toISOString(), + }); + return personId; +} + +/** An additional face on an existing person, so a split has something to move. */ +async function addFaceTo(eventId, personId, centroid) { + const [p] = await db('photos').insert({ + event_id: eventId, + filename: `${Math.random()}.jpg`, + path: '/tmp/x.jpg', + type: 'individual', + }).returning('id'); + const photoId = typeof p === 'object' ? p.id : p; + const [f] = await db('photo_faces').insert({ + photo_id: photoId, + event_id: eventId, + person_id: personId, + bbox_x: 0, bbox_y: 0, bbox_w: 200, bbox_h: 200, + det_score: 0.99, + embedding: clustering.packEmbedding(centroid), + model_version: 'test-v1', + created_at: new Date().toISOString(), + }).returning('id'); + return typeof f === 'object' ? f.id : f; +} + +const suggest = (eventId) => clustering.suggestMerges(eventId, { thresholds: THRESHOLDS }); + +describe('face merge suggestions (#1107)', () => { + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + clustering = require('../../src/services/faceClustering'); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + describe('the band', () => { + it('suggests a pair between the match and auto-merge thresholds', async () => { + const eventId = await seedEvent('band-inside'); + const [a, b] = pairAtSimilarity(0.64, 0); + const idA = await insertPerson(eventId, a); + const idB = await insertPerson(eventId, b); + + const out = await suggest(eventId); + + expect(out).toHaveLength(1); + expect([out[0].person_a_id, out[0].person_b_id].sort()).toEqual([idA, idB].sort()); + expect(out[0].score).toBeCloseTo(0.64, 4); + }); + + it('stays silent above the auto-merge threshold — consolidate() owns that pair', async () => { + const eventId = await seedEvent('band-above'); + const [a, b] = pairAtSimilarity(0.75, 0); + await insertPerson(eventId, a); + await insertPerson(eventId, b); + + expect(await suggest(eventId)).toEqual([]); + }); + + it('stays silent below the match threshold — further apart than one face would join', async () => { + const eventId = await seedEvent('band-below'); + const [a, b] = pairAtSimilarity(0.5, 0); + await insertPerson(eventId, a); + await insertPerson(eventId, b); + + expect(await suggest(eventId)).toEqual([]); + }); + }); + + describe('what it refuses to ask about', () => { + it('never questions two people the photographer named differently', async () => { + const eventId = await seedEvent('named-apart'); + const [a, b] = pairAtSimilarity(0.64, 0); + await insertPerson(eventId, a, { label: 'Anna' }); + await insertPerson(eventId, b, { label: 'Beatrix' }); + + expect(await suggest(eventId)).toEqual([]); + }); + + it('still asks when only one of the two is named', async () => { + const eventId = await seedEvent('one-named'); + const [a, b] = pairAtSimilarity(0.64, 0); + await insertPerson(eventId, a, { label: 'Anna' }); + await insertPerson(eventId, b); + + expect(await suggest(eventId)).toHaveLength(1); + }); + + it('skips a person marked "not a real person" — that answer was already given', async () => { + const eventId = await seedEvent('ignored'); + const [a, b] = pairAtSimilarity(0.64, 0); + await insertPerson(eventId, a); + await insertPerson(eventId, b, { is_ignored: true }); + + expect(await suggest(eventId)).toEqual([]); + }); + + it('never crosses embedding spaces', async () => { + const eventId = await seedEvent('model-skew'); + const [a, b] = pairAtSimilarity(0.64, 0); + await insertPerson(eventId, a); + await insertPerson(eventId, b, { model_version: 'test-v2' }); + + expect(await suggest(eventId)).toEqual([]); + }); + }); + + describe('dismissal', () => { + it('stops suggesting a pair the photographer rejected, and survives a repeat', async () => { + const eventId = await seedEvent('dismissal'); + const [a, b] = pairAtSimilarity(0.64, 0); + const idA = await insertPerson(eventId, a); + const idB = await insertPerson(eventId, b); + + expect(await suggest(eventId)).toHaveLength(1); + + await clustering.dismissMergeSuggestion(eventId, idB, idA); // reversed on purpose + expect(await suggest(eventId)).toEqual([]); + + // A second dismissal hits the UNIQUE constraint. Dismissing twice is a + // double-click, not an error. + await expect(clustering.dismissMergeSuggestion(eventId, idA, idB)).resolves.toEqual({ + dismissed: true, + }); + expect(await suggest(eventId)).toEqual([]); + }); + + /** + * The swallow-the-duplicate branch has to discriminate, because the failure + * it must NOT swallow looks identical to the caller: returning + * "kept separate" for a decision that was never written means the pair + * silently comes back after the next scan. + * + * Tested on the predicate directly — provoking a read-only database or a + * dropped table mid-suite would corrupt the shared fixture for every other + * case in this file. + */ + it.each([ + ['postgres unique violation', { code: '23505', message: 'duplicate key value violates unique constraint' }, true], + ['sqlite3 unique violation', { code: 'SQLITE_CONSTRAINT', message: 'UNIQUE constraint failed: event_people_merge_dismissals.event_id' }, true], + ['better-sqlite3 unique violation', { code: 'SQLITE_CONSTRAINT_UNIQUE', message: 'UNIQUE constraint failed' }, true], + ['sqlite foreign-key violation', { code: 'SQLITE_CONSTRAINT', message: 'FOREIGN KEY constraint failed' }, false], + ['sqlite busy', { code: 'SQLITE_BUSY', message: 'database is locked' }, false], + ['missing table', { code: 'SQLITE_ERROR', message: 'no such table: event_people_merge_dismissals' }, false], + ['postgres read-only transaction', { code: '25006', message: 'cannot execute INSERT in a read-only transaction' }, false], + ['no error at all', null, false], + ])('%s → swallowed: %s', (_name, err, expected) => { + expect(clustering.isUniqueViolation(err)).toBe(expected); + }); + + /** + * The dismissal read is the only thing standing between the automatic pass + * and a pair the photographer explicitly separated. If it fails open, a + * timeout silently restores the merge that "Not the same" was supposed to + * prevent — so anything other than a missing table must stop the pass. + */ + it('refuses to consolidate when the dismissal list cannot be read', async () => { + const eventId = await seedEvent('dismissals-unreadable'); + // Well above the auto-merge threshold, so only a refusal keeps them apart. + const [a, b] = pairAtSimilarity(0.97, 0); + await insertPersonWithFace(eventId, a); + await insertPersonWithFace(eventId, b); + + // Break the read for real rather than mocking knex: dropping a selected + // column makes the query fail with something that is NOT "missing + // table", which is exactly the class that must not fail open. + await db.schema.alterTable('event_people_merge_dismissals', (t) => t.dropColumn('person_b_id')); + try { + await expect(clustering.consolidate(eventId, { thresholds: THRESHOLDS })) + .rejects.toThrow(); + + // Nothing merged: the pass gave up rather than overriding a decision + // it could not read. + expect(await db('event_people').where({ event_id: eventId })).toHaveLength(2); + } finally { + await db.schema.alterTable('event_people_merge_dismissals', (t) => { + t.integer('person_b_id').notNullable().defaultTo(0); + }); + } + }); + + it.each([ + ['postgres undefined_table', { code: '42P01', message: 'relation "x" does not exist' }, true], + ['sqlite missing table', { code: 'SQLITE_ERROR', message: 'no such table: x' }, true], + // The one that matters: a missing COLUMN is a broken query, not a + // pre-migration install, and must NOT be allowed to fail open. + ['postgres undefined_column', { code: '42703', message: 'column "x" does not exist' }, false], + ['sqlite missing column', { code: 'SQLITE_ERROR', message: 'no such column: x' }, false], + ['statement timeout', { code: '57014', message: 'canceling statement due to statement timeout' }, false], + ])('missing-table check — %s → %s', (_name, err, expected) => { + expect(clustering.isMissingTable(err)).toBe(expected); + }); + + it('normalizes the pair so one row covers both orderings', async () => { + const eventId = await seedEvent('dismissal-normalized'); + const [a, b] = pairAtSimilarity(0.64, 0); + const idA = await insertPerson(eventId, a); + const idB = await insertPerson(eventId, b); + + await clustering.dismissMergeSuggestion(eventId, idB, idA); + const rows = await db('event_people_merge_dismissals').where({ event_id: eventId }); + + expect(rows).toHaveLength(1); + expect(rows[0].person_a_id).toBe(Math.min(idA, idB)); + expect(rows[0].person_b_id).toBe(Math.max(idA, idB)); + }); + }); + + describe('one suggestion per person per round', () => { + it('does not offer A-B, A-C and B-C for a three-way fragment', async () => { + const eventId = await seedEvent('three-way'); + // Three mutually similar centroids, all inside the band. + const base = new Float32Array(DIM); base[0] = 1; + const people = []; + for (let k = 0; k < 3; k++) { + const v = new Float32Array(DIM); + v[0] = 0.9; + v[1 + k] = Math.sqrt(1 - 0.81); + people.push(await insertPerson(eventId, v)); + } + await insertPerson(eventId, base); + + const out = await suggest(eventId); + + // Every returned pair must name people not already spoken for: accepting + // the first suggestion must never leave a second one pointing at a person + // that the merge just deleted. + const seen = new Set(); + for (const s of out) { + expect(seen.has(s.person_a_id)).toBe(false); + expect(seen.has(s.person_b_id)).toBe(false); + seen.add(s.person_a_id); + seen.add(s.person_b_id); + } + }); + + it('offers the most similar pair first', async () => { + const eventId = await seedEvent('ordering'); + const [a1, b1] = pairAtSimilarity(0.62, 0); + const [a2, b2] = pairAtSimilarity(0.67, 10); + await insertPerson(eventId, a1); + await insertPerson(eventId, b1); + await insertPerson(eventId, a2); + await insertPerson(eventId, b2); + + const out = await suggest(eventId); + + expect(out).toHaveLength(2); + expect(out[0].score).toBeGreaterThan(out[1].score); + }); + }); + + describe('manual splits survive the automatic pass', () => { + /** + * The regression that matters most once consolidation runs on every scan: + * a photographer splitting a wrongly-merged cluster produces two people + * who are look-alikes BY CONSTRUCTION, so their centroids sit above the + * merge threshold and the very next scan would put them straight back. + */ + it('records a split as a separation, so consolidation leaves it alone', async () => { + const eventId = await seedEvent('split-protected'); + const base = new Float32Array(DIM); base[0] = 1; + + // One cluster holding two near-identical faces. + const personId = await insertPersonWithFace(eventId, base); + const extraFaceId = await addFaceTo(eventId, personId, base); + + const newPersonId = await clustering.splitPerson(eventId, personId, [extraFaceId]); + expect(newPersonId).toBeTruthy(); + + const rows = await db('event_people_merge_dismissals').where({ event_id: eventId }); + expect(rows).toHaveLength(1); + expect([rows[0].person_a_id, rows[0].person_b_id].sort()) + .toEqual([personId, newPersonId].sort()); + + // Identical centroids — nothing but the recorded separation can stop + // this merge. + const merged = await clustering.consolidate(eventId, { thresholds: THRESHOLDS }); + expect(merged).toEqual([]); + expect(await db('event_people').where({ event_id: eventId })).toHaveLength(2); + }); + }); + + describe('consolidation reporting', () => { + it('records what an automatic pass merged, so it is not silent', async () => { + const eventId = await seedEvent('report-merged'); + // 0.97 is above the 0.68 auto-merge threshold — consolidate() acts. + const [a, b] = pairAtSimilarity(0.97, 0); + await insertPersonWithFace(eventId, a); + await insertPersonWithFace(eventId, b); + + const merged = await clustering.consolidate(eventId, { thresholds: THRESHOLDS }); + expect(merged).toHaveLength(1); + + const event = await db('events').where({ id: eventId }).first(); + expect(Number(event.faces_last_consolidated_count)).toBe(1); + expect(event.faces_last_consolidated_at).toBeTruthy(); + }); + + it('never absorbs an ignored cluster — that would mark a real person ignored', async () => { + const eventId = await seedEvent('consolidate-ignored'); + // Well above the auto-merge threshold: only the is_ignored flag can + // stop this pair. + const [a, b] = pairAtSimilarity(0.97, 0); + const real = await insertPersonWithFace(eventId, a); + const junk = await insertPersonWithFace(eventId, b, { is_ignored: true }); + + const merged = await clustering.consolidate(eventId, { thresholds: THRESHOLDS }); + + expect(merged).toEqual([]); + // Both still standing, and the real person is still guest-visible — + // mergePeople ORs is_ignored onto the survivor, so absorbing the junk + // cluster would have hidden a real person from the gallery. + const survivors = await db('event_people').where({ event_id: eventId }).select('id', 'is_ignored'); + expect(survivors.map((p) => p.id).sort()).toEqual([real, junk].sort()); + const realRow = survivors.find((p) => p.id === real); + expect(realRow.is_ignored === true || realRow.is_ignored === 1).toBe(false); + }); + + it('never merges a pair the photographer said was not the same person', async () => { + const eventId = await seedEvent('consolidate-dismissed'); + // Also above the auto-merge threshold: the dismissal is the only thing + // standing between these two, which is the point — a human "no" has to + // outrank the automatic pass, not just the suggestion list. + const [a, b] = pairAtSimilarity(0.97, 0); + const idA = await insertPersonWithFace(eventId, a); + const idB = await insertPersonWithFace(eventId, b); + + await clustering.dismissMergeSuggestion(eventId, idA, idB); + const merged = await clustering.consolidate(eventId, { thresholds: THRESHOLDS }); + + expect(merged).toEqual([]); + expect(await db('event_people').where({ event_id: eventId }).count({ c: '*' }).first()) + .toEqual(expect.objectContaining({ c: 2 })); + }); + + // NOT covered by a test: reporting what a pass merged before it died + // partway. `consolidate` calls `mergePeople` through the module-local + // binding, so a spy on the export cannot intercept it, and no realistic + // database failure lands on the second merge only. The recording therefore + // sits in a `finally` — each mergePeople is its own transaction, so a pass + // that throws has still committed what it did, and the alternative is a + // real merge going unreported. Verified by reading, not by assertion. + + it('clears a previous count when a later pass merges nothing', async () => { + const eventId = await seedEvent('report-cleared'); + await db('events').where({ id: eventId }).update({ faces_last_consolidated_count: 7 }); + + const [a, b] = pairAtSimilarity(0.5, 0); + await insertPersonWithFace(eventId, a); + await insertPersonWithFace(eventId, b); + + await clustering.consolidate(eventId, { thresholds: THRESHOLDS }); + + const event = await db('events').where({ id: eventId }).first(); + expect(Number(event.faces_last_consolidated_count)).toBe(0); + }); + }); +}); diff --git a/backend/__tests__/integration/faceQueueDrainConsolidation.test.js b/backend/__tests__/integration/faceQueueDrainConsolidation.test.js new file mode 100644 index 00000000..5eae7ed6 --- /dev/null +++ b/backend/__tests__/integration/faceQueueDrainConsolidation.test.js @@ -0,0 +1,219 @@ +/** + * "The scan finished" is not a thing this queue is told (#1107). + * + * It claims photos one at a time, so a backfill is just a lot of independent + * claims and the only available signal is a worker finding nothing left. That + * signal is NOT sufficient on its own — with concurrency above one the other + * workers may still be busy, and a photo released back to `pending` by a down + * sidecar is still owed — so the drain is tested against the queue directly. + * + * These are the cases that decide whether consolidation runs too early (a + * wasted pass over half-formed clusters) or never (the feature silently does + * nothing, which is the state #1107 was filed about). + */ + +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-facedrain-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'facedrain-test-secret'; + +const { bootCrmDb } = require('./helpers/crmDb'); + +let db; let cleanup; let faceQueue; let clustering; + +async function seedEvent(slug) { + const [row] = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: slug, + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `${slug}-share`, + expires_at: new Date().toISOString(), + // The drain rechecks this before consolidating, so the fixture has to be + // a gallery that actually has detection on. + face_recognition_enabled: true, + }).returning('id'); + return typeof row === 'object' ? row.id : row; +} + +/** Both halves of the "two deliberate actions" rule have to be on. */ +async function enableFacesGlobally() { + const existing = await db('feature_flags').where({ key: 'faces' }).first(); + if (existing) await db('feature_flags').where({ key: 'faces' }).update({ value: true }); + else await db('feature_flags').insert({ key: 'faces', value: true }); +} + +async function insertPhoto(eventId, faceStatus) { + const [row] = await db('photos').insert({ + event_id: eventId, + filename: `${Math.random()}.jpg`, + path: '/tmp/x.jpg', + type: 'individual', + face_status: faceStatus, + }).returning('id'); + return typeof row === 'object' ? row.id : row; +} + +describe('faceQueue drain consolidation (#1107)', () => { + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + faceQueue = require('../../src/services/faceQueue'); + clustering = require('../../src/services/faceClustering'); + await enableFacesGlobally(); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + beforeEach(() => { + faceQueue.touchedEvents.clear(); + faceQueue.consolidationRetryAt.clear(); + faceQueue.inFlightByEvent.clear(); + jest.restoreAllMocks(); + }); + + it('does nothing at all when no photo has been scanned', async () => { + const spy = jest.spyOn(clustering, 'consolidate'); + await faceQueue.drainConsolidation(); + expect(spy).not.toHaveBeenCalled(); + }); + + it('waits while the event still has photos queued', async () => { + const eventId = await seedEvent('drain-pending'); + await insertPhoto(eventId, 'done'); + await insertPhoto(eventId, 'pending'); + faceQueue.touchedEvents.add(eventId); + + const spy = jest.spyOn(clustering, 'consolidate'); + await faceQueue.drainConsolidation(); + + expect(spy).not.toHaveBeenCalled(); + // Still owed, so it must keep its place for the next idle tick — dropping + // it here would mean the gallery never consolidates at all. + expect(faceQueue.touchedEvents.has(eventId)).toBe(true); + }); + + it('waits while a photo is still being processed by another worker', async () => { + const eventId = await seedEvent('drain-processing'); + await insertPhoto(eventId, 'done'); + await insertPhoto(eventId, 'processing'); + faceQueue.touchedEvents.add(eventId); + + const spy = jest.spyOn(clustering, 'consolidate'); + await faceQueue.drainConsolidation(); + + expect(spy).not.toHaveBeenCalled(); + expect(faceQueue.touchedEvents.has(eventId)).toBe(true); + }); + + it('consolidates once the queue is empty, and does not repeat itself', async () => { + const eventId = await seedEvent('drain-empty'); + await insertPhoto(eventId, 'done'); + await insertPhoto(eventId, 'failed'); + await insertPhoto(eventId, 'skipped'); + faceQueue.touchedEvents.add(eventId); + + const spy = jest.spyOn(clustering, 'consolidate').mockResolvedValue([]); + await faceQueue.drainConsolidation(); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(eventId); + // Drained and handled, so a second idle tick must not pay for it again. + expect(faceQueue.touchedEvents.has(eventId)).toBe(false); + + await faceQueue.drainConsolidation(); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('a failing consolidation never propagates into the worker loop, and is retried', async () => { + const eventId = await seedEvent('drain-throws'); + await insertPhoto(eventId, 'done'); + faceQueue.touchedEvents.add(eventId); + + const spy = jest.spyOn(clustering, 'consolidate').mockRejectedValue(new Error('boom')); + + await expect(faceQueue.drainConsolidation()).resolves.toBeUndefined(); + + // A transient database error must not cost the gallery its consolidation + // outright — the event keeps its place so a later tick retries. + expect(faceQueue.touchedEvents.has(eventId)).toBe(true); + + // ...but not on the very next tick. The worker idles every couple of + // seconds, so an immediate retry would hot-loop a permanently broken event + // and warn every time. + expect(faceQueue.consolidationRetryAt.get(eventId)).toBeGreaterThan(Date.now()); + const callsBefore = spy.mock.calls.length; + await faceQueue.drainConsolidation(); + expect(spy).toHaveBeenCalledTimes(callsBefore); + + // Once the backoff elapses it really does try again, and succeeds. + faceQueue.consolidationRetryAt.set(eventId, Date.now() - 1); + spy.mockResolvedValue([]); + await faceQueue.drainConsolidation(); + expect(faceQueue.touchedEvents.has(eventId)).toBe(false); + expect(faceQueue.consolidationRetryAt.has(eventId)).toBe(false); + }); + + it('waits while another worker is still inside processPhotoFaces', async () => { + const eventId = await seedEvent('drain-inflight'); + // Every row already reads as drained: the last photo is committed 'done' + // inside the transaction, and auto-categorisation runs afterwards. Only + // the in-flight count knows a worker is still there. + await insertPhoto(eventId, 'done'); + faceQueue.touchedEvents.add(eventId); + faceQueue.inFlightByEvent.set(eventId, 1); + + const spy = jest.spyOn(clustering, 'consolidate').mockResolvedValue([]); + await faceQueue.drainConsolidation(); + + // Consolidating here would record its count, and the busy worker would + // then re-mark the event — the next pass merges nothing and overwrites the + // real number with zero. + expect(spy).not.toHaveBeenCalled(); + expect(faceQueue.touchedEvents.has(eventId)).toBe(true); + + faceQueue.inFlightByEvent.delete(eventId); + await faceQueue.drainConsolidation(); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('does not consolidate an event whose detection was switched off mid-drain', async () => { + const eventId = await seedEvent('drain-disabled'); + await insertPhoto(eventId, 'done'); + await db('events').where({ id: eventId }).update({ face_recognition_enabled: false }); + faceQueue.touchedEvents.add(eventId); + + const spy = jest.spyOn(clustering, 'consolidate').mockResolvedValue([]); + await faceQueue.drainConsolidation(); + + // An earlier photo legitimately marked the event before the toggle went + // off. Merging someone's clusters just after they disabled the feature is + // not a thing to do quietly. + expect(spy).not.toHaveBeenCalled(); + // Dropped rather than retried — it is not coming back on its own. + expect(faceQueue.touchedEvents.has(eventId)).toBe(false); + }); + + it('treats events independently — a busy gallery does not hold up a finished one', async () => { + const busy = await seedEvent('drain-busy'); + const done = await seedEvent('drain-done'); + await insertPhoto(busy, 'pending'); + await insertPhoto(done, 'done'); + faceQueue.touchedEvents.add(busy); + faceQueue.touchedEvents.add(done); + + const spy = jest.spyOn(clustering, 'consolidate').mockResolvedValue([]); + await faceQueue.drainConsolidation(); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(done); + expect(faceQueue.touchedEvents.has(busy)).toBe(true); + }); +}); diff --git a/backend/migrations/core/180_face_merge_suggestions.js b/backend/migrations/core/180_face_merge_suggestions.js new file mode 100644 index 00000000..f9cb16e7 --- /dev/null +++ b/backend/migrations/core/180_face_merge_suggestions.js @@ -0,0 +1,73 @@ +/** + * Automatic consolidation after a scan, and the suggestion band below it + * (#1107). + * + * `faceClustering.consolidate()` has existed since #1074 but only ever ran from + * `recluster()`, i.e. when an admin pressed "Re-group people". After a normal + * background scan nobody looked, so a gallery settled with "Anna in daylight" + * and "Anna at the party" as separate people even though their centroids had + * long since converged. + * + * Two things get stored here. + * + * 1. `events.faces_last_consolidated_*` — merging biometric clusters silently + * is the wrong default even at high confidence, so a pass that merged + * anything has to be able to say so afterwards. The count is per-scan: the + * next consolidation overwrites it, which is the intended lifetime. + * + * 2. `event_people_merge_dismissals` — the band BELOW the auto-merge threshold + * is surfaced as a suggestion rather than merged, and a suggestion the + * photographer has rejected must stay rejected. Without this the same "are + * these the same person?" pair returns after every scan. + * + * The dismissal rows reference people that merge, split and recluster all + * delete. That is deliberately NOT enforced with a foreign key: a dangling + * dismissal simply stops matching anything, which is the correct outcome, and + * an FK would either block those operations or need cascade rules on a table + * whose whole purpose is to be advisory. `pruneDismissals` is not needed for + * correctness — the rows are tiny and harmless. + */ + +exports.up = async function up(knex) { + if (await knex.schema.hasTable('events')) { + const hasCount = await knex.schema.hasColumn('events', 'faces_last_consolidated_count'); + const hasAt = await knex.schema.hasColumn('events', 'faces_last_consolidated_at'); + if (!hasCount || !hasAt) { + await knex.schema.alterTable('events', (table) => { + if (!hasCount) table.integer('faces_last_consolidated_count').defaultTo(0); + if (!hasAt) table.timestamp('faces_last_consolidated_at').nullable(); + }); + } + } + + if (!(await knex.schema.hasTable('event_people_merge_dismissals'))) { + await knex.schema.createTable('event_people_merge_dismissals', (table) => { + table.increments('id').primary(); + table.integer('event_id').notNullable(); + // Stored with person_a_id < person_b_id so a pair has exactly one row + // regardless of which order the comparison produced it in. + table.integer('person_a_id').notNullable(); + table.integer('person_b_id').notNullable(); + table.timestamp('created_at').defaultTo(knex.fn.now()); + + table.unique(['event_id', 'person_a_id', 'person_b_id']); + table.index(['event_id']); + }); + } +}; + +exports.down = async function down(knex) { + if (await knex.schema.hasTable('event_people_merge_dismissals')) { + await knex.schema.dropTable('event_people_merge_dismissals'); + } + if (await knex.schema.hasTable('events')) { + const hasCount = await knex.schema.hasColumn('events', 'faces_last_consolidated_count'); + const hasAt = await knex.schema.hasColumn('events', 'faces_last_consolidated_at'); + if (hasCount || hasAt) { + await knex.schema.alterTable('events', (table) => { + if (hasCount) table.dropColumn('faces_last_consolidated_count'); + if (hasAt) table.dropColumn('faces_last_consolidated_at'); + }); + } + } +}; diff --git a/backend/src/routes/adminEvents/faces.js b/backend/src/routes/adminEvents/faces.js index e594331d..42aa7281 100644 --- a/backend/src/routes/adminEvents/faces.js +++ b/backend/src/routes/adminEvents/faces.js @@ -99,6 +99,14 @@ module.exports = (router) => { enabled: event.face_recognition_enabled === true || event.face_recognition_enabled === 1, visible_to_guests: faceSettings.areFacesVisibleToGuests(event), last_scan_at: event.faces_last_scan_at || null, + // What the automatic consolidation pass did when this event last + // drained (#1107). Surfaced because merging biometric clusters + // without saying so is the wrong default, however confident the + // similarity was. + consolidation: { + merged: Number(event.faces_last_consolidated_count || 0), + at: event.faces_last_consolidated_at || null, + }, status, }); } catch (error) { @@ -189,6 +197,71 @@ module.exports = (router) => { } }); + /** + * Look-alike pairs in the band BELOW the auto-merge threshold (#1107). + * + * Registered before the '/:id/people/:personId' family so the literal + * 'suggestions' segment can never be read as a person id — see ./index.js on + * why registration order is load-bearing here. It would not collide today + * (that route has no GET), but relying on that is one refactor away from a + * person id of "suggestions" reaching the database. + * + * Returns ids and a score only. Every caller already holds the people list + * with its covers, so re-sending face crops here would duplicate a payload + * the modal has open in front of it. + */ + router.get('/:id/people/suggestions', + adminAuth, requirePermission('events.view'), requireFaces, requireEventOwnership, + async (req, res) => { + try { + const event = await loadOwnedEvent(req); + if (!event) return res.status(404).json({ error: 'Event not found' }); + + const suggestions = await faceClustering.suggestMerges(event.id); + res.json({ suggestions }); + } catch (error) { + errorResponse(res, error, 500, 'Failed to fetch merge suggestions'); + } + }); + + /** + * "These two are not the same person." Sticky, so the pair stops coming back + * after every scan. + */ + router.post('/:id/people/suggestions/dismiss', + adminAuth, + requirePermission('events.edit'), + requireFaces, + requireEventOwnership, + [body('person_a_id').isInt(), body('person_b_id').isInt()], + async (req, res) => { + const errors = validationResult(req); + if (!errors.isEmpty()) return res.status(400).json({ errors: errors.array() }); + + try { + const event = await loadOwnedEvent(req); + if (!event) return res.status(404).json({ error: 'Event not found' }); + + // Both ids must belong to this event. Without this an admin could + // write dismissal rows naming people in someone else's gallery — + // harmless on its own, but it is other tenants' data in our table. + const ids = [Number(req.body.person_a_id), Number(req.body.person_b_id)]; + if (ids[0] === ids[1]) { + return res.status(400).json({ error: 'A person cannot be dismissed against itself' }); + } + const owned = await db('event_people') + .where({ event_id: event.id }).whereIn('id', ids).pluck('id'); + if (owned.length !== 2) { + return res.status(400).json({ error: 'One or more people do not belong to this event' }); + } + + const result = await faceClustering.dismissMergeSuggestion(event.id, ids[0], ids[1]); + res.json({ success: true, ...result }); + } catch (error) { + errorResponse(res, error, 500, 'Failed to dismiss suggestion'); + } + }); + /** * Rename / hide / ignore / set cover. */ diff --git a/backend/src/services/databaseBackup.js b/backend/src/services/databaseBackup.js index 63883f5f..99f34dfe 100644 --- a/backend/src/services/databaseBackup.js +++ b/backend/src/services/databaseBackup.js @@ -28,7 +28,7 @@ const PROGRESS_INTERVAL = 100; // Report progress every 100 rows // SQLite cannot filter at all — `sqlite3 .backup` is a whole-file binary copy // — so the rows are deleted from the temp copy before it is finalised. See // createSQLiteBackup below. -const FACE_TABLES = ['photo_faces', 'event_people']; +const FACE_TABLES = ['photo_faces', 'event_people', 'event_people_merge_dismissals']; /** * Database Backup Service diff --git a/backend/src/services/faceClustering.js b/backend/src/services/faceClustering.js index 5f10997f..73e238b0 100644 --- a/backend/src/services/faceClustering.js +++ b/backend/src/services/faceClustering.js @@ -247,6 +247,60 @@ async function assignFacesLocked(eventId, faceRows, thresholds, trx) { return assignments; } +/** + * Does this error mean the table isn't there yet, as opposed to the query + * failing? Postgres reports SQLSTATE 42P01; SQLite says so in the message. + * The distinction decides whether a dismissal read may fail open. + */ +function isMissingTable(err) { + if (!err) return false; + // Postgres: undefined_table. Deliberately NOT matched on the message — its + // "does not exist" wording also covers a missing COLUMN, which is a broken + // query rather than a pre-migration install and must not fail open. + if (err.code === '42P01') return true; + return /no such table/i.test(err.message || ''); +} + +/** + * One identity for a pair regardless of which order it was produced in. + * Rows are stored the same way, so the two always agree. + */ +function pairKey(a, b) { + return a < b ? `${a}:${b}` : `${b}:${a}`; +} + +/** + * Pairs the photographer has explicitly kept apart (#1107). + * + * Read by BOTH the automatic pass and the suggestion list: "not the same + * person" has to bind the thing that acts on its own even more than it binds + * the thing that asks. Missing table (pre-migration) reads as "nothing + * dismissed" rather than failing the merge that called this. + */ +async function loadDismissedPairs(eventId) { + try { + const rows = await db('event_people_merge_dismissals') + .where({ event_id: eventId }) + .select('person_a_id', 'person_b_id'); + return new Set(rows.map((d) => pairKey(d.person_a_id, d.person_b_id))); + } catch (err) { + // ONLY a missing table reads as "nothing dismissed" — that is a + // pre-migration install, where by definition nothing has been dismissed. + // + // Everything else FAILS CLOSED. Treating a timeout or a permission error + // as an empty set would let the automatic pass merge pairs the + // photographer explicitly separated, which is precisely the decision this + // set exists to protect. The caller defers instead: drainConsolidation + // backs the event off and retries. + if (!isMissingTable(err)) throw err; + logger.warn( + `faceClustering: merge-dismissal table absent for event ${eventId} — treating as none`, + { error: err.message } + ); + return new Set(); + } +} + /** * Merge people whose centroids have drifted together. * @@ -264,40 +318,214 @@ async function consolidate(eventId, options = {}) { const people = await db('event_people') .where({ event_id: eventId }) - .select('id', 'centroid', 'face_count_total', 'model_version', 'label'); + .select('id', 'centroid', 'face_count_total', 'model_version', 'label', 'is_ignored'); const state = people + // "Not a real person" must never be merged INTO one. mergePeople ORs + // is_ignored onto the survivor, so absorbing a false-positive cluster + // would mark a real person ignored and drop them out of the guest-facing + // strip entirely. Cheap to skip, expensive to discover. + .filter((p) => !(p.is_ignored === true || p.is_ignored === 1)) .map((p) => ({ ...p, vec: unpackEmbedding(p.centroid) })) .filter((p) => p.vec); + // A pair the photographer answered "not the same" about stays not the same, + // however far the centroids drift afterwards. Without this the automatic + // pass silently overturns an explicit human decision the moment new faces + // push the pair over the threshold — or the moment someone tunes it. + const dismissed = await loadDismissedPairs(eventId); + const merged = []; const absorbed = new Set(); + // Each mergePeople is its own transaction, so a pass that dies halfway has + // still committed what it did. Reporting has to survive that: recorded in a + // finally, or a failure on the second pair would leave the first one merged + // and unreported — silent, which is the one thing this must never be. The + // retry then re-reports against whatever is left. + try { + for (let i = 0; i < state.length; i++) { + if (absorbed.has(state[i].id)) continue; + for (let j = i + 1; j < state.length; j++) { + if (absorbed.has(state[j].id)) continue; + const a = state[i]; + const b = state[j]; + if (a.model_version !== b.model_version) continue; + + // Never silently merge two people the photographer has NAMED + // differently — that is a human assertion this heuristic does not get + // to overrule. + if (a.label && b.label && a.label !== b.label) continue; + + if (dismissed.has(pairKey(a.id, b.id))) continue; + + if (dot(a.vec, b.vec) >= mergeThreshold) { + // Re-read this one pair immediately before acting. The set above was + // loaded once for the whole pass, and a photographer pressing "Not the + // same" during it would otherwise be overruled by a decision that was + // already stale when it was made. Narrows the window to a single + // statement rather than the length of the pass; one extra query per + // pair that is actually about to merge, which is rare. + const justDismissed = await db('event_people_merge_dismissals') + .where({ + event_id: eventId, + person_a_id: Math.min(a.id, b.id), + person_b_id: Math.max(a.id, b.id), + }) + .first() + .catch((err) => { + if (isMissingTable(err)) return null; + throw err; + }); + if (justDismissed) continue; + + await mergePeople(eventId, [b.id], a.id); + absorbed.add(b.id); + merged.push({ from: b.id, into: a.id }); + } + } + } + + } finally { + if (merged.length) { + logger.info(`faceClustering: consolidated ${merged.length} person pair(s) in event ${eventId}`); + } + + // Record the outcome even when it is zero. Merging biometric clusters + // silently is the wrong default however confident the maths is (#1107), so + // the admin card reports what this pass did — and a run that merged nothing + // has to clear a previous run's count rather than leave it standing. + await db('events').where({ id: eventId }).update({ + faces_last_consolidated_count: merged.length, + faces_last_consolidated_at: new Date().toISOString(), + }).catch((err) => { + // Pre-migration installs simply do not report. Never let bookkeeping fail + // a merge that already happened. + logger.warn(`faceClustering: could not record consolidation for event ${eventId}`, { + error: err.message, + }); + }); + } + + return merged; +} + +/** + * Pairs that look like the same person but not confidently enough to merge + * automatically (#1107). + * + * The band is [match_threshold, merge_threshold): above the top of it + * `consolidate()` has already merged the pair, and below the bottom the two + * centroids are further apart than the distance at which a single face would + * have joined the cluster at all — which is not a claim worth putting in front + * of anyone. + * + * This is the "with a warning" half of the request. An over-eager merge is much + * harder to unpick than a missed one, so the uncertain band never merges by + * itself; it asks. + */ +async function suggestMerges(eventId, options = {}) { + const thresholds = options.thresholds || (await getThresholds()); + const mergeThreshold = Math.min(0.95, thresholds.face_match_threshold + 0.08); + const floor = thresholds.face_match_threshold; + const limit = options.limit || 20; + + const people = await db('event_people') + .where({ event_id: eventId }) + .select('id', 'centroid', 'face_count_total', 'model_version', 'label', 'is_ignored'); + + const state = people + // "Not a real person" is an answer already given — never ask about it again. + .filter((p) => !(p.is_ignored === true || p.is_ignored === 1)) + .map((p) => ({ ...p, vec: unpackEmbedding(p.centroid) })) + .filter((p) => p.vec); + + if (state.length < 2) return []; + + const dismissed = await loadDismissedPairs(eventId); + + const pairs = []; for (let i = 0; i < state.length; i++) { - if (absorbed.has(state[i].id)) continue; for (let j = i + 1; j < state.length; j++) { - if (absorbed.has(state[j].id)) continue; const a = state[i]; const b = state[j]; if (a.model_version !== b.model_version) continue; - - // Never silently merge two people the photographer has NAMED - // differently — that is a human assertion this heuristic does not get - // to overrule. + // Same rule consolidate() applies: two different names is a human + // assertion, not a question. if (a.label && b.label && a.label !== b.label) continue; - if (dot(a.vec, b.vec) >= mergeThreshold) { - await mergePeople(eventId, [b.id], a.id); - absorbed.add(b.id); - merged.push({ from: b.id, into: a.id }); - } + if (dismissed.has(pairKey(a.id, b.id))) continue; + + const score = dot(a.vec, b.vec); + if (score < floor || score >= mergeThreshold) continue; + + pairs.push({ + person_a_id: Math.min(a.id, b.id), + person_b_id: Math.max(a.id, b.id), + score, + }); } } - if (merged.length) { - logger.info(`faceClustering: consolidated ${merged.length} person pair(s) in event ${eventId}`); + // Most-similar first: the strongest suggestion is the one most likely to be + // accepted, and a photographer working down the list should meet it first. + pairs.sort((x, y) => y.score - x.score); + + // One suggestion per person per round. Without this a cluster that genuinely + // has three fragments produces A-B, A-C and B-C, and accepting A-B leaves two + // suggestions pointing at a person that no longer exists. + const used = new Set(); + const result = []; + for (const pair of pairs) { + if (used.has(pair.person_a_id) || used.has(pair.person_b_id)) continue; + used.add(pair.person_a_id); + used.add(pair.person_b_id); + result.push(pair); + if (result.length >= limit) break; } - return merged; + return result; +} + +/** + * Is this the UNIQUE constraint firing, as opposed to a real write failure? + * + * Both engines have to be recognised: Postgres reports SQLSTATE 23505, and + * sqlite3 reports SQLITE_CONSTRAINT with the specific constraint named in the + * message (better-sqlite3 narrows the code itself). Matching too broadly here + * would put us back to swallowing genuine failures. + */ +function isUniqueViolation(err) { + if (!err) return false; + if (err.code === '23505') return true; + if (typeof err.code === 'string' && err.code.startsWith('SQLITE_CONSTRAINT')) { + return /unique/i.test(err.message || ''); + } + return false; +} + +/** + * Remember that these two are NOT the same person, so the pair stops being + * suggested. Normalized to (lower id, higher id) so the pair has one identity. + */ +async function dismissMergeSuggestion(eventId, personAId, personBId) { + const lo = Math.min(personAId, personBId); + const hi = Math.max(personAId, personBId); + try { + await db('event_people_merge_dismissals').insert({ + event_id: eventId, + person_a_id: lo, + person_b_id: hi, + created_at: new Date().toISOString(), + }); + } catch (err) { + // ONLY the UNIQUE constraint doing its job — dismissing twice is a + // double-click, not an error. Anything else (missing table on a + // pre-migration install, read-only database) must reach the caller: + // reporting "kept separate" for a decision that was never stored is worse + // than an error, because the pair silently comes back next scan. + if (!isUniqueViolation(err)) throw err; + } + return { dismissed: true }; } /** @@ -380,6 +608,22 @@ async function splitPerson(eventId, personId, faceIds) { .whereIn('id', faces.map((f) => f.id)) .update({ person_id: newPersonId }); + // A split IS a "these are not the same person" decision, and it has to be + // recorded as one (#1107). Consolidation now runs automatically after + // every scan, and two clusters a photographer pulled apart are look-alikes + // by construction — their centroids usually sit above the merge threshold, + // so the very next scan would put them straight back together and the + // manual correction would look like it never happened. + await trx('event_people_merge_dismissals').insert({ + event_id: eventId, + person_a_id: Math.min(personId, newPersonId), + person_b_id: Math.max(personId, newPersonId), + created_at: new Date().toISOString(), + }).catch((err) => { + // Pre-migration install, or the pair was already separated once before. + if (!isMissingTable(err) && !isUniqueViolation(err)) throw err; + }); + await recomputeCentroid(newPersonId, trx); await recomputeCentroid(personId, trx); return newPersonId; @@ -557,6 +801,12 @@ module.exports = { meetsQualityFloor, assignFaces, consolidate, + suggestMerges, + dismissMergeSuggestion, + // Exported for the tests that pin which failures may be swallowed and which + // must stop the pass. + isUniqueViolation, + isMissingTable, mergePeople, splitPerson, recomputeCentroid, diff --git a/backend/src/services/faceProcessor.js b/backend/src/services/faceProcessor.js index 05df9a07..ee3f5c7e 100644 --- a/backend/src/services/faceProcessor.js +++ b/backend/src/services/faceProcessor.js @@ -371,6 +371,18 @@ async function purgeEvent(eventId) { await trx('photos').where({ event_id: eventId }).update({ face_status: null, face_count: null, face_started_at: null, face_error: null, }); + + // Everything the erasure was about is gone, so the records ABOUT that + // grouping go too (#1107): dismissal rows name people that no longer + // exist, and a consolidation count left standing would have the card + // reporting merges beside an empty people list. Inside purgeEvent rather + // than the route so archival gets the same treatment. + await trx('event_people_merge_dismissals').where({ event_id: eventId }).del() + .catch(() => { /* pre-migration install — nothing to clear */ }); + await trx('events').where({ id: eventId }).update({ + faces_last_consolidated_count: 0, + faces_last_consolidated_at: null, + }).catch(() => { /* pre-migration install */ }); logger.info(`faceProcessor: purged ${faces} face(s) and ${people} person(s) from event ${eventId}`); return { faces, people }; }); diff --git a/backend/src/services/faceQueue.js b/backend/src/services/faceQueue.js index 433d512a..190e1454 100644 --- a/backend/src/services/faceQueue.js +++ b/backend/src/services/faceQueue.js @@ -31,7 +31,7 @@ const logger = require('../utils/logger'); const { processPhotoFaces } = require('./faceProcessor'); const { SidecarUnavailableError } = require('./faceClient'); const { TransientSourceError } = require('./faceProcessor'); -const { isFeatureEnabled } = require('./faceSettings'); +const { isFeatureEnabled, isEnabledForEvent } = require('./faceSettings'); const POLL_INTERVAL_MS = parseInt(process.env.FACE_PROCESSOR_POLL_MS || '2000', 10); const CONCURRENCY = Math.max(1, parseInt(process.env.FACE_PROCESSOR_CONCURRENCY || '1', 10)); @@ -101,6 +101,121 @@ function logUnreachableSource(message) { ); } +/** + * Events that have had a photo scanned since their last consolidation pass + * (#1107). + * + * There is no "scan finished" event to hook: the queue is per-photo, and a + * backfill is just a lot of independent claims. So a worker that finds nothing + * left to claim asks whether the events it touched have actually drained, and + * consolidates the ones that have — greedy assignment leaves "Anna in daylight" + * and "Anna at the party" as two clusters whose centroids have since converged, + * and until now nothing looked unless an admin pressed "Re-group people". + * + * In-memory, like `deferredEvents`: a restart loses at most a consolidation + * pass, and the next scan of that event schedules another one. + */ +const touchedEvents = new Set(); + +// A consolidation that fails keeps its place so a transient database error +// does not cost the gallery its pass — but the worker goes idle every +// POLL_INTERVAL_MS, so retrying immediately would hot-loop a permanently +// broken event and emit a warning every couple of seconds. Same shape as +// `deferredEvents` above: back it off, then try again. +const CONSOLIDATE_RETRY_MS = parseInt(process.env.FACE_CONSOLIDATE_RETRY_MS || '60000', 10); +const consolidationRetryAt = new Map(); + +// eventId -> how many workers are currently inside processPhotoFaces for it. +// +// `face_status` alone cannot answer "is anyone still working on this event": +// the photo is committed 'done' inside the transaction, and auto-categorisation +// then runs before the call returns. During that window the row looks drained +// to every other worker. Counting the callers closes it. +const inFlightByEvent = new Map(); + +function markInFlight(eventId, delta) { + if (eventId == null) return; + const next = (inFlightByEvent.get(eventId) || 0) + delta; + if (next > 0) inFlightByEvent.set(eventId, next); + else inFlightByEvent.delete(eventId); +} + +/** + * Consolidate every touched event that has genuinely drained. + * + * "A worker went idle" is not the same as "the scan is done" — with + * FACE_PROCESSOR_CONCURRENCY > 1 the others may still be working, and photos + * released back to `pending` by a down sidecar are still owed. So the drain is + * tested directly against the queue rather than inferred, and an event that is + * still busy simply stays in the set for the next idle tick. + * + * Across multiple pods two workers can consolidate the same event at once. + * That is safe rather than coordinated: `mergePeople` is transactional, and a + * pair whose source was already absorbed merges nothing. + */ +async function drainConsolidation() { + if (!touchedEvents.size) return; + + for (const eventId of [...touchedEvents]) { + const retryAt = consolidationRetryAt.get(eventId); + if (retryAt && Date.now() < retryAt) continue; + + try { + const outstanding = await db('photos') + .where({ event_id: eventId }) + .whereIn('face_status', ['pending', 'processing']) + .count({ c: '*' }) + .first(); + + if (Number(outstanding?.c ?? 0) > 0) continue; + + // A worker may still be inside processPhotoFaces for this event: the + // photo is committed 'done' before auto-categorisation runs, so the row + // stops counting as outstanding while the call is still going. Without + // this, an idle worker consolidates and records its count, the busy one + // then re-marks the event, and the next pass overwrites the real number + // with zero — losing exactly the report this feature exists to give. + if ((inFlightByEvent.get(eventId) || 0) > 0) continue; + + // Detection may have been switched off mid-drain, after an earlier + // photo already marked this event. Merging someone's clusters just + // after they turned the feature off is not a thing to do quietly. + const event = await db('events').where({ id: eventId }).first(); + if (!(await isEnabledForEvent(event))) { + touchedEvents.delete(eventId); + consolidationRetryAt.delete(eventId); + continue; + } + + // Required here rather than at module load, matching faceProcessor's + // call into faceAutoCategories: the binding stays late, which keeps the + // seam this is tested through honest. + const { consolidate } = require('./faceClustering'); + const merged = await consolidate(eventId); + // Dropped only once it has actually run. Removing it first meant a + // transient database error lost the pass entirely — no retry until the + // gallery happened to be scanned again. + touchedEvents.delete(eventId); + consolidationRetryAt.delete(eventId); + if (merged.length) { + logger.info( + `faceQueue: scan of event ${eventId} drained — consolidated ${merged.length} look-alike pair(s)` + ); + } + } catch (e) { + // Never let a consolidation failure stop the queue. The event keeps its + // place so a transient error is retried, but not before the backoff — + // otherwise a permanently failing event warns on every idle tick. + consolidationRetryAt.set(eventId, Date.now() + CONSOLIDATE_RETRY_MS); + logger.warn( + `faceQueue: consolidation failed for event ${eventId}, retrying in ` + + `${Math.round(CONSOLIDATE_RETRY_MS / 1000)}s`, + { error: e.message } + ); + } + } +} + let running = false; let workerHandles = []; let janitorHandle = null; @@ -184,12 +299,27 @@ async function workerLoop(workerIdx) { } if (!claimed) { + // Nothing left to claim is the only signal this queue has that a scan + // may have finished. Cheap when idle: no-ops unless work happened. + await drainConsolidation(); await sleep(POLL_INTERVAL_MS); continue; } + markInFlight(claimed.event_id, +1); try { - await processPhotoFaces(claimed.id); + const outcome = await processPhotoFaces(claimed.id); + // Only a photo that actually went through detection can have moved this + // event's clusters. 'skipped' covers videos, a purge that raced the + // scan, and — the one that matters — detection being switched OFF + // mid-drain: consolidating there would merge clusters moments after the + // admin turned the feature off. 'failed' produced no faces either. + // + // A photo containing no faces still reports 'done', so an event whose + // photos are all empty still gets its (harmless) pass. + if (outcome?.status === 'done' && claimed.event_id != null) { + touchedEvents.add(claimed.event_id); + } } catch (err) { // The sidecar being down stops EVERY photo, so returning this one to // 'pending' and backing off costs nothing — there is no other work to @@ -237,6 +367,11 @@ async function workerLoop(workerIdx) { error: updateErr.message, }); } + } finally { + // In a finally because the catch above returns to the loop via + // `continue` on two paths — a leaked count would block this event's + // consolidation for the lifetime of the process. + markInFlight(claimed.event_id, -1); } } } @@ -255,6 +390,18 @@ async function janitorLoop() { } catch (e) { logger.warn('faceQueue: janitor error', { error: e.message }); } + + // The worker only reaches its drain when it can claim NOTHING, anywhere. + // With the default single worker that means one gallery finishing during a + // large backfill waits for every other gallery — and under continuous + // ingestion it could wait indefinitely. Running the drain here too makes + // consolidation depend on the event being finished rather than the whole + // install being idle. It is per-event guarded, so this is a no-op for + // anything still in flight. + await drainConsolidation().catch((e) => + logger.warn('faceQueue: janitor drain error', { error: e.message }) + ); + await sleep(JANITOR_INTERVAL_MS); } } @@ -293,4 +440,11 @@ async function stop() { janitorHandle = null; } -module.exports = { start, stop, claimNextPhoto }; +// drainConsolidation, touchedEvents and consolidationRetryAt are exported for +// the same reason claimNextPhoto is: the drain condition and its failure +// backoff are the subtle parts of #1107 and are worth pinning directly, rather +// than through a running worker loop. +module.exports = { + start, stop, claimNextPhoto, drainConsolidation, + touchedEvents, consolidationRetryAt, inFlightByEvent, +}; diff --git a/backend/src/services/picpeakExportService.js b/backend/src/services/picpeakExportService.js index 4595b581..50c1cdaa 100644 --- a/backend/src/services/picpeakExportService.js +++ b/backend/src/services/picpeakExportService.js @@ -42,6 +42,11 @@ const EXCLUDED_TABLES = new Set([ 'knex_migrations_lock', 'photo_faces', 'event_people', + // Follows event_people out of the export (#1107): these rows are nothing but + // references to person ids the target will never receive. Carried across, + // they would attach to whatever ids the target's own re-scan happens to + // mint, silently suppressing merge suggestions in unrelated galleries. + 'event_people_merge_dismissals', ]); // Storage subdirs holding non-recalculable blobs — always included. diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index ee9c38ef..fa3beab7 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -358,7 +358,7 @@ async function replaceAllTables(tables, dataDir, currentAdmin, roleSnapshot, { c // orphans can end up attached to reused photo/event ids from the incoming // archive: one instance's biometric data silently adopted by another's // galleries. Purge them explicitly. - for (const faceTable of ['photo_faces', 'event_people']) { + for (const faceTable of ['photo_faces', 'event_people', 'event_people_merge_dismissals']) { try { await trx(faceTable).del(); } catch (err) { diff --git a/frontend/src/components/admin/FaceRecognitionCard.tsx b/frontend/src/components/admin/FaceRecognitionCard.tsx index bc793178..ee854894 100644 --- a/frontend/src/components/admin/FaceRecognitionCard.tsx +++ b/frontend/src/components/admin/FaceRecognitionCard.tsx @@ -29,6 +29,12 @@ interface FacesPayload { enabled: boolean; visible_to_guests: boolean; last_scan_at: string | null; + // What the automatic consolidation pass merged when this gallery last + // finished scanning (#1107). + consolidation?: { + merged: number; + at: string | null; + }; status: { scanned: number; total: number; @@ -156,6 +162,29 @@ export const FaceRecognitionCard: React.FC = ({ eventI // eslint-disable-next-line react-hooks/exhaustive-deps }, [healthAt, shouldProbe, scanRunning, health]); + // Consolidation happens AFTER the last photo is marked done: the worker + // records the event, goes idle, then drains. So the poll that first sees + // `in_progress: false` also stops polling, and it read the consolidation + // count a moment too early — leaving the card silent about merges that did + // happen until the admin navigates back. + // + // Two catch-up refetches rather than one, because a consolidation that hits + // a transient error is retried by the queue a minute later + // (FACE_CONSOLIDATE_RETRY_MS): the first covers the normal case, the second + // covers one retry. Deliberately a fixed pair and not a poll — an event + // whose photos all failed never consolidates at all, and a condition-based + // poll would spin on it forever. + const wasScanning = useRef(false); + useEffect(() => { + const scanning = !!data?.status?.in_progress; + const justFinished = wasScanning.current && !scanning; + wasScanning.current = scanning; + if (!justFinished) return; + + const timers = [8000, 70000].map((ms) => setTimeout(() => { refetch(); }, ms)); + return () => timers.forEach(clearTimeout); + }, [data?.status?.in_progress, refetch]); + useEffect(() => { if (!data?.enabled) return; api.get('/admin/events/faces/auto-categories') @@ -451,6 +480,24 @@ export const FaceRecognitionCard: React.FC = ({ eventI misconfiguration or from corrupt images at some earlier point. Attributing them properly would mean reading stored face_error rows — worth doing, but a bigger change than this. */} + {/* Automatic consolidation (#1107). Clustering merged look-alike + groups on its own once the scan drained, and doing that to + biometric clusters without saying so is the wrong default — + even at the stricter-than-assignment threshold it uses. Points + at the tool for undoing it rather than claiming an undo we do + not have: Split is how a wrong merge gets unpicked. */} + {!status.in_progress && (data.consolidation?.merged ?? 0) > 0 && ( +

+ + + {t('admin.faces.consolidated', { + count: data.consolidation!.merged, + defaultValue_one: 'Grouping merged {{count}} look-alike pair automatically after the last scan. Open Manage people to check it — anything merged wrongly can be separated again with Split.', + defaultValue_other: 'Grouping merged {{count}} look-alike pairs automatically after the last scan. Open Manage people to check them — anything merged wrongly can be separated again with Split.', + })} + +

+ )} {!status.in_progress && sidecarNotice && (

diff --git a/frontend/src/components/admin/PeopleManagerModal.tsx b/frontend/src/components/admin/PeopleManagerModal.tsx index cfce275c..eed94102 100644 --- a/frontend/src/components/admin/PeopleManagerModal.tsx +++ b/frontend/src/components/admin/PeopleManagerModal.tsx @@ -56,6 +56,13 @@ interface PersonFace { blur: number | null; } +/** A pair scoring between the assignment threshold and the auto-merge one. */ +interface MergeSuggestion { + person_a_id: number; + person_b_id: number; + score: number; +} + interface PeopleManagerModalProps { eventId: number; open: boolean; @@ -199,6 +206,14 @@ export const PeopleManagerModal: React.FC = ({ // differs. Keyed by person id so switching between them reuses the cache. const facesFor = splitting || coverFor || viewing?.person || null; + // Look-alike pairs the automatic pass deliberately did NOT merge (#1107): + // similar enough to ask about, not similar enough to act on unasked. + const { data: suggestionData } = useQuery<{ suggestions: MergeSuggestion[] }>({ + queryKey: ['admin-people-suggestions', eventId], + queryFn: async () => (await api.get(`/admin/events/${eventId}/people/suggestions`)).data, + enabled: open, + }); + const { data: faceData, isLoading: facesLoading } = useQuery<{ faces: PersonFace[] }>({ queryKey: ['admin-person-faces', eventId, facesFor?.id], queryFn: async () => @@ -208,12 +223,53 @@ export const PeopleManagerModal: React.FC = ({ const people = useMemo(() => data?.people || [], [data]); + /** + * Suggestions resolved against the loaded people. + * + * The endpoint returns ids and a score only — the covers are already here. + * Pairs whose people are missing are dropped rather than rendered blank: the + * two queries are invalidated together, but a suggestion computed just before + * a merge can name a person that no longer exists. + */ + const suggestionPairs = useMemo(() => { + const byId = new Map(people.map((p) => [p.id, p])); + return (suggestionData?.suggestions || []) + .map((s) => ({ + a: byId.get(s.person_a_id), + b: byId.get(s.person_b_id), + score: s.score, + })) + .filter((p): p is { a: AdminPerson; b: AdminPerson; score: number } => !!p.a && !!p.b); + }, [suggestionData, people]); + + /** + * Names already used in THIS gallery, for the rename input's datalist + * (#1107). Naming a wedding means typing the same surname into a dozen + * fresh empty inputs; the second occurrence should be a keystroke. + * + * Deliberately event-scoped. Names from other events would be more useful — + * the same family recurs across shoots — but that would surface client names + * from galleries the current admin may not be allowed to open, which is a + * permissions decision (#743), not an implementation detail. + * + * No fetch: the people list already in front of the user IS the source. + */ + const knownNames = useMemo(() => { + const names = people + .map((p) => (p.label || '').trim()) + .filter(Boolean); + return [...new Set(names)].sort((a, b) => a.localeCompare(b)); + }, [people]); + const after = async (message: string) => { // The face list is cached per person and split/merge move faces between // them. Until the cover picker landed, the only reader closed itself after // acting so nobody saw the stale copy; now a second surface reads the same // key and would offer faces that are no longer this person's. await queryClient.invalidateQueries({ queryKey: ['admin-person-faces'] }); + // Merging or splitting changes which pairs are still worth suggesting, and + // a suggestion naming a person that no longer exists is worse than none. + await queryClient.invalidateQueries({ queryKey: ['admin-people-suggestions'] }); await refetch(); onChanged?.(); setSelected([]); @@ -262,6 +318,36 @@ export const PeopleManagerModal: React.FC = ({ ); }; + /** + * Accept a suggestion. The named side is the target so the name survives — + * the same rule doMerge applies, but here the order is ours to choose rather + * than the click order's, so it can be chosen correctly. Falling back to the + * larger cluster keeps the bigger centroid as the survivor. + */ + const acceptSuggestion = (a: AdminPerson, b: AdminPerson) => { + const target = a.label ? a : b.label ? b + : ((a.total_face_count ?? a.face_count) >= (b.total_face_count ?? b.face_count) ? a : b); + const source = target.id === a.id ? b : a; + run( + async () => { + await api.post(`/admin/events/${eventId}/people/merge`, { + source_ids: [source.id], target_id: target.id, + }); + }, + t('admin.people.merged', { count: 1, defaultValue: 'People merged' }) + ); + }; + + const dismissSuggestion = (a: AdminPerson, b: AdminPerson) => + run( + async () => { + await api.post(`/admin/events/${eventId}/people/suggestions/dismiss`, { + person_a_id: a.id, person_b_id: b.id, + }); + }, + t('admin.people.suggestionDismissed', { defaultValue: 'Kept separate' }) + ); + const doSplit = () => { if (!splitting || !splitFaceIds.length) return; const personId = splitting.id; @@ -307,6 +393,13 @@ export const PeopleManagerModal: React.FC = ({ aria-modal="true" className="relative bg-white dark:bg-neutral-900 text-neutral-900 dark:text-neutral-100 rounded-xl shadow-xl w-full max-w-4xl max-h-[88vh] flex flex-col" > + {/* One datalist for every row's rename input — a per-row copy would + duplicate the whole name list once per person. */} + {knownNames.length > 0 && ( + + {knownNames.map((name) => + )}

@@ -539,6 +632,71 @@ export const PeopleManagerModal: React.FC = ({

) : (
+ {/* --- merge suggestions (#1107) ------------------------- + The band below the auto-merge threshold. These are asked + rather than done: an over-eager merge of two people is + much harder to unpick than a missed one, and this is + biometric grouping, so the uncertain cases get a human. + Dismissal is sticky — a pair told "not the same" does not + come back after the next scan. */} + {suggestionPairs.length > 0 && ( +
+

+ {t('admin.people.suggestionsHeading', { + count: suggestionPairs.length, + defaultValue: 'These might be the same person. Grouping was not confident enough to merge them on its own.', + })} +

+
+ {suggestionPairs.map(({ a, b, score }) => ( +
+
+ {[a, b].map((person) => ( +
+ + + {person.label || t('admin.people.photoCount', { + count: person.total_face_count ?? person.face_count, + defaultValue: `${person.total_face_count ?? person.face_count} photos`, + })} + +
+ ))} +
+ + {t('admin.people.suggestionScore', { + percent: Math.round(score * 100), + defaultValue: `${Math.round(score * 100)}% alike`, + })} + +
+ + +
+
+ ))} +
+
+ )} + {people.map((person) => { const isSelected = selected.includes(person.id); return ( @@ -577,6 +735,7 @@ export const PeopleManagerModal: React.FC = ({ if (e.key === 'Escape') setRenaming(null); }} placeholder={t('admin.people.namePlaceholder', { defaultValue: 'Add a name' })} + list={knownNames.length ? 'picpeak-people-names' : undefined} className="w-full max-w-xs px-2 py-1 text-sm border border-neutral-300 dark:border-neutral-600 bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 rounded focus:outline-none focus:ring-2 focus:ring-primary-500" /> ) : ( diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 0c655351..55623955 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -3150,7 +3150,14 @@ "splitHelp": "Wählen Sie die Fotos aus, die NICHT diese Person zeigen. Sie werden zu einem neuen Eintrag, alles andere bleibt.", "splitSelected": "{{count}} ausgewählt", "doSplit": "Abtrennen", - "empty": "Noch keine Personen erkannt." + "empty": "Noch keine Personen erkannt.", + "photoCount_one": "{{count}} Foto", + "photoCount_other": "{{count}} Fotos", + "suggestionsHeading": "Das könnte dieselbe Person sein. Die Gruppierung war sich nicht sicher genug, um sie von selbst zusammenzuführen.", + "suggestionScore": "{{percent}}% ähnlich", + "suggestionAccept": "Dieselbe Person", + "suggestionReject": "Nicht dieselbe", + "suggestionDismissed": "Getrennt gelassen" }, "faces": { "manage": "Personen verwalten", @@ -3180,7 +3187,9 @@ "sidecarUnreachable": "Der Gesichtserkennungs-Dienst unter {{url}} ist nicht erreichbar, deshalb werden die {{pending}} eingereihten Fotos nicht verarbeitet. Es geht nichts verloren — der Scan läuft von selbst weiter, sobald der Dienst wieder da ist. Starte ihn mit `docker compose --profile faces up -d`; er beendet sich sofort, wenn FACE_ML_TOKEN nicht auf denselben Wert wie im Backend gesetzt ist — einen Standardwert gibt es nicht.", "sidecarUnauthorized": "Der Gesichtserkennungs-Dienst unter {{url}} weist unser Token zurück; Fotos werden als fehlgeschlagen markiert statt erneut versucht. Setze FACE_ML_TOKEN im Backend und im picpeak-ml-Container identisch, starte beide neu und nutze dann Neu scannen — das Token allein zu korrigieren verarbeitet die bereits fehlgeschlagenen Fotos nicht erneut.", "sidecarRejected": "{{url}} antwortet, aber nicht wie der Gesichtserkennungs-Dienst — Fotos werden als fehlgeschlagen markiert statt erneut versucht. Prüfe, ob FACE_ML_URL auf den picpeak-ml-Container zeigt und kein Proxy dazwischenliegt, und nutze dann Neu scannen für die bereits fehlgeschlagenen Fotos.", - "sidecarStateNow": "Aktueller Zustand des Dienstes — einige der Fehler oben können eine andere Ursache haben, aber ein erneuter Scan wird erst nach der Behebung erfolgreich sein:" + "sidecarStateNow": "Aktueller Zustand des Dienstes — einige der Fehler oben können eine andere Ursache haben, aber ein erneuter Scan wird erst nach der Behebung erfolgreich sein:", + "consolidated_one": "Beim letzten Scan wurde {{count}} ähnliches Paar automatisch zusammengeführt. Prüfen Sie es unter „Personen verwalten“ — falsch Zusammengeführtes lässt sich mit „Trennen“ wieder aufteilen.", + "consolidated_other": "Beim letzten Scan wurden {{count}} ähnliche Paare automatisch zusammengeführt. Prüfen Sie sie unter „Personen verwalten“ — falsch Zusammengeführtes lässt sich mit „Trennen“ wieder aufteilen." } }, "acceptInvitation": { diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 8e35f594..0f36b611 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2722,7 +2722,14 @@ "splitHelp": "Pick the photos that are NOT this person. They become a new entry, and everything else stays.", "splitSelected": "{{count}} selected", "doSplit": "Split out", - "empty": "No people detected yet." + "empty": "No people detected yet.", + "photoCount_one": "{{count}} photo", + "photoCount_other": "{{count}} photos", + "suggestionsHeading": "These might be the same person. Grouping was not confident enough to merge them on its own.", + "suggestionScore": "{{percent}}% alike", + "suggestionAccept": "Same person", + "suggestionReject": "Not the same", + "suggestionDismissed": "Kept separate" }, "faces": { "manage": "Manage people", @@ -2752,7 +2759,9 @@ "sidecarUnreachable": "Can't reach the face-detection service at {{url}}, so the {{pending}} queued photos aren't being processed. Nothing is lost — the scan resumes on its own once the service is up. Start it with `docker compose --profile faces up -d`, and note it exits immediately unless FACE_ML_TOKEN is set to the same value as the backend — there is no default.", "sidecarUnauthorized": "The face-detection service at {{url}} is rejecting our token, and photos are being marked failed rather than retried. Make FACE_ML_TOKEN identical on the backend and the picpeak-ml container, restart both, then use Re-scan — fixing the token alone will not reprocess the photos that already failed.", "sidecarRejected": "{{url}} answered, but not like the face-detection service — photos are being marked failed rather than retried. Check FACE_ML_URL points at the picpeak-ml container and that nothing is proxying that address, then use Re-scan for the photos that already failed.", - "sidecarStateNow": "Service state right now — some of the failures above may have a different cause, but a re-scan will not succeed until this is fixed:" + "sidecarStateNow": "Service state right now — some of the failures above may have a different cause, but a re-scan will not succeed until this is fixed:", + "consolidated_one": "Grouping merged {{count}} look-alike pair automatically after the last scan. Open Manage people to check it — anything merged wrongly can be separated again with Split.", + "consolidated_other": "Grouping merged {{count}} look-alike pairs automatically after the last scan. Open Manage people to check them — anything merged wrongly can be separated again with Split." } }, "acceptInvitation": {