diff --git a/backend/__tests__/integration/externalImportConcurrency.test.js b/backend/__tests__/integration/externalImportConcurrency.test.js new file mode 100644 index 00000000..6bd72ab7 --- /dev/null +++ b/backend/__tests__/integration/externalImportConcurrency.test.js @@ -0,0 +1,205 @@ +/** + * Two overlapping external imports insert every file twice (#1162). + * + * The route checked for an existing external_relpath and then inserted, with + * an fs.stat and a `sharp().metadata()` read sitting in between. A reporter + * double-clicked a slow import of a 6012-file tree and got 8004 rows. + * + * Both halves of the fix are driven here through the real route: + * + * - the in-flight guard, which turns the second click into a 409 instead of + * a second full walk of the tree; + * - convergence when the guard cannot help (another replica, another + * process), which is the unique index from migration 186 firing and the + * loop counting a skip rather than dying or duplicating. + * + * The second is exercised by inserting a competing row from inside the mocked + * `sharp().metadata()` call — literally inside the window the bug lived in. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const express = require('express'); +const request = require('supertest'); + +describe('concurrent external imports (#1162)', () => { + let tmpDir; let db; let app; let mediaRoot; + // When set, the mocked sharp metadata read inserts this row first — the + // other run winning the race between our SELECT and our INSERT. + let stealDuringMetadata = null; + let thumbnailDelayMs = 0; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-extdup-')); + mediaRoot = path.join(tmpDir, 'media'); + await fs.promises.mkdir(path.join(mediaRoot, 'nas', 'individual'), { recursive: true }); + for (const name of ['a.jpg', 'b.jpg', 'c.jpg']) { + await fs.promises.writeFile(path.join(mediaRoot, 'nas', 'individual', name), 'not-a-real-jpeg'); + } + + process.env.NODE_ENV = 'test'; + process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'db.sqlite'); + await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true }); + process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); + process.env.EXTERNAL_MEDIA_ROOT = mediaRoot; + process.env.JWT_SECRET = process.env.JWT_SECRET || 'extdup-secret'; + + jest.resetModules(); + + jest.doMock('../../src/middleware/auth', () => ({ + adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester', roleName: 'admin' }; next(); }, + })); + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + })); + jest.doMock('../../src/middleware/ownership', () => ({ + requireEventOwnership: (_req, _res, next) => next(), + })); + + // The window. In production this is a real decode of a NAS-hosted file — + // hundreds of milliseconds during which the row we just proved absent can + // appear. Standing in for the other run here makes that deterministic. + jest.doMock('sharp', () => () => ({ + metadata: async () => { + if (stealDuringMetadata) { + const { db: liveDb } = require('../../src/database/db'); + await liveDb('photos').insert(stealDuringMetadata); + stealDuringMetadata = null; + } + return { width: 100, height: 200 }; + }, + })); + + jest.doMock('../../src/services/imageProcessor', () => ({ + generateThumbnail: jest.fn(async () => { + if (thumbnailDelayMs) await new Promise((r) => setTimeout(r, thumbnailDelayMs)); + return 'thumbnails/mock.jpg'; + }), + ensureThumbnail: jest.fn(), + })); + + jest.doMock('../../src/utils/logger', () => ({ + debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), + })); + + ({ db } = await require('./helpers/crmDb').bootCrmDb()); + + app = express(); + app.use(express.json()); + app.use('/api/admin/external-media', require('../../src/routes/adminExternalMedia')); + }, 180000); + + afterAll(async () => { + if (db) await db.destroy?.(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + async function seedEvent() { + await db('photos').del(); + await db('events').del(); + stealDuringMetadata = null; + thumbnailDelayMs = 0; + const [e] = await db('events').insert({ + slug: `extdup-${Math.random().toString(36).slice(2, 8)}`, + event_type: 'wedding', + event_name: 'extdup', + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `extdup-${Math.random()}`, + expires_at: new Date().toISOString(), + source_mode: 'reference', + }).returning('id'); + return typeof e === 'object' ? e.id : e; + } + + const runImport = (eventId) => request(app) + .post(`/api/admin/external-media/events/${eventId}/import-external`) + .send({ external_path: 'nas', recursive: true }); + + async function relpathCounts(eventId) { + const rows = await db('photos').where({ event_id: eventId }).select('external_relpath'); + const counts = new Map(); + for (const r of rows) counts.set(r.external_relpath, (counts.get(r.external_relpath) || 0) + 1); + return counts; + } + + it('rejects a second import while the first is still running', async () => { + const eventId = await seedEvent(); + // Enough to keep the first request inside its loop while the second + // arrives — the "slow import looks hung, so I clicked again" case. + thumbnailDelayMs = 20; + + const [first, second] = await Promise.all([runImport(eventId), runImport(eventId)]); + + const statuses = [first.status, second.status].sort(); + expect(statuses).toEqual([200, 409]); + const rejected = first.status === 409 ? first : second; + expect(rejected.body.error).toMatch(/already running/i); + }); + + it('leaves exactly one row per file after both runs', async () => { + const eventId = await seedEvent(); + thumbnailDelayMs = 20; + + await Promise.all([runImport(eventId), runImport(eventId)]); + + const counts = await relpathCounts(eventId); + expect(counts.size).toBe(3); + expect([...counts.values()]).toEqual([1, 1, 1]); + }); + + it('releases the event once the import finishes, so a re-import still works', async () => { + const eventId = await seedEvent(); + + expect((await runImport(eventId)).status).toBe(200); + // Not 409 — the guard is per run, not a permanent lock on the event. + const second = await runImport(eventId); + expect(second.status).toBe(200); + expect(second.body.imported).toBe(0); + expect(second.body.skipped).toBe(3); + }); + + it('converges when another writer wins the race mid-file', async () => { + // The guard is in-process, so it cannot see a second replica. This is what + // the unique index is for: the insert bounces, and the file is counted as + // skipped rather than duplicated or lost to a 500. + const eventId = await seedEvent(); + stealDuringMetadata = { + event_id: eventId, + filename: 'a.jpg', + path: 'x/a.jpg', + type: 'individual', + source_origin: 'external', + external_relpath: path.join('individual', 'a.jpg'), + }; + + const res = await runImport(eventId); + + expect(res.status).toBe(200); + const counts = await relpathCounts(eventId); + expect(counts.get(path.join('individual', 'a.jpg'))).toBe(1); + // Two imported by us, one lost to the other writer and reported honestly. + expect(res.body.imported).toBe(2); + expect(res.body.skipped).toBe(1); + }); + + it('does not let one contended file abort the rest of the import', async () => { + const eventId = await seedEvent(); + stealDuringMetadata = { + event_id: eventId, + filename: 'a.jpg', + path: 'x/a.jpg', + type: 'individual', + source_origin: 'external', + external_relpath: path.join('individual', 'a.jpg'), + }; + + await runImport(eventId); + + // All three files present — the contended one via the other writer's row. + expect((await relpathCounts(eventId)).size).toBe(3); + }); +}); diff --git a/backend/__tests__/migrations/186_external_relpath_unique.test.js b/backend/__tests__/migrations/186_external_relpath_unique.test.js new file mode 100644 index 00000000..76bc584f --- /dev/null +++ b/backend/__tests__/migrations/186_external_relpath_unique.test.js @@ -0,0 +1,554 @@ +/** + * One row per external file per event (#1162). + * + * The migration has two halves and they fail differently: the cleanup can take + * out the wrong row of a pair (losing a thumbnail, orphaning an event's hero), + * and the index can fail to be created at all — leaving an install that looks + * migrated and is still racing. Both are pinned here. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const migration = require('../../migrations/core/186_external_relpath_unique'); + +describe('migration 186 — unique (event_id, external_relpath) (#1162)', () => { + let knex; let tmpDir; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig186-')); + knex = require('knex')({ + client: 'sqlite3', + connection: { filename: path.join(tmpDir, 'db.sqlite') }, + useNullAsDefault: true, + }); + }); + + afterAll(async () => { + if (knex) await knex.destroy(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + for (const table of [ + 'photos', 'events', 'photo_categories', 'photo_feedback', + 'photo_admin_marks', 'photo_faces', 'image_access_logs', 'transfer_files', + 'event_people', 'event_people_merge_dismissals', + ]) { + await knex.schema.dropTableIfExists(table); + } + await knex.schema.createTable('events', (t) => { + t.increments('id').primary(); + t.integer('hero_photo_id'); + t.string('download_zip_path'); + t.string('download_zip_generated_at'); + }); + await knex.schema.createTable('photo_categories', (t) => { + t.increments('id').primary(); + t.integer('hero_photo_id'); + }); + await knex.schema.createTable('photos', (t) => { + t.increments('id').primary(); + t.integer('event_id'); + t.string('external_relpath'); + t.string('thumbnail_path'); + t.string('source_origin').defaultTo('managed'); + t.integer('feedback_count').defaultTo(0); + t.integer('like_count').defaultTo(0); + t.decimal('average_rating', 3, 2).defaultTo(0); + t.integer('favorite_count').defaultTo(0); + t.integer('reaction_count').defaultTo(0); + t.integer('color_label_count').defaultTo(0); + t.string('face_status'); + t.integer('view_count').defaultTo(0); + t.integer('download_count').defaultTo(0); + t.integer('face_count'); + t.string('face_started_at'); + t.text('face_error'); + }); + // Declared exactly as the real schema declares them — CASCADE and all. + // The point of these tables here is that SQLite does NOT enforce any of + // it (PicPeak never sets `PRAGMA foreign_keys = ON`), so a bare delete of + // the photo row leaves every one of them dangling. + await knex.schema.createTable('photo_feedback', (t) => { + t.increments('id').primary(); + t.integer('photo_id').references('id').inTable('photos').onDelete('CASCADE'); + t.integer('event_id'); + t.string('feedback_type'); + t.text('comment_text'); + t.string('guest_identifier'); + // Per-person guest identity (migration 078). Nullable: galleries without + // guest identity leave it NULL and fall back to guest_identifier. + t.integer('guest_id'); + t.integer('rating'); + t.boolean('is_hidden').defaultTo(false); + t.boolean('is_approved').defaultTo(true); + }); + await knex.schema.createTable('photo_admin_marks', (t) => { + t.increments('id').primary(); + t.integer('photo_id').notNullable().references('id').inTable('photos').onDelete('CASCADE'); + t.integer('event_id'); + t.integer('admin_id'); + t.integer('rating'); + // Independently writable alongside rating, per photoAdminMarksService. + t.string('color_label', 16); + t.unique(['photo_id', 'admin_id'], 'photo_admin_marks_photo_admin_uniq'); + }); + await knex.schema.createTable('photo_faces', (t) => { + t.increments('id').primary(); + t.integer('photo_id').references('id').inTable('photos').onDelete('CASCADE'); + t.integer('event_id'); + // purgePhotoFaces rebuilds the people that lose members, so the cluster + // link and the vectors recomputeCentroid reads have to be here for this + // to exercise the real path rather than a stub. + t.integer('person_id'); + t.binary('embedding'); + t.float('det_score'); + }); + await knex.schema.createTable('event_people', (t) => { + t.increments('id').primary(); + t.integer('event_id'); + t.binary('centroid'); + t.integer('face_count').defaultTo(0); + }); + await knex.schema.createTable('event_people_merge_dismissals', (t) => { + t.increments('id').primary(); + t.integer('event_id'); + t.binary('centroid_a'); + t.binary('centroid_b'); + }); + await knex.schema.createTable('image_access_logs', (t) => { + t.increments('id').primary(); + t.integer('photo_id'); + }); + await knex.schema.createTable('transfer_files', (t) => { + t.increments('id').primary(); + t.integer('transfer_id'); + t.integer('photo_id'); + t.unique(['transfer_id', 'photo_id'], 'transfer_files_unique'); + }); + }); + + /** Two duplicate rows for the same file: id 1 survives, id 2 is doomed. */ + const seedPair = async () => { + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + ]); + }; + + const rows = () => knex('photos').orderBy('id', 'asc').select('*'); + + it('collapses a duplicated pair to one row and leaves distinct paths alone', async () => { + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't1', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't2', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/y.jpg', thumbnail_path: 't3', source_origin: 'external' }, + ]); + + await migration.up(knex); + + const after = await rows(); + expect(after.map((r) => r.external_relpath)).toEqual(['a/x.jpg', 'a/y.jpg']); + // Lowest id survives when both sides are equally complete. + expect(after[0].id).toBe(1); + }); + + it('does not collapse the same path across different events', async () => { + // The constraint is per event. Two events referencing the same NAS folder + // is a supported setup, and treating those as duplicates would delete one + // event's entire library. + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }, + { event_id: 2, external_relpath: 'a/x.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await knex('photos').count('* as c').first()).toEqual({ c: 2 }); + }); + + it('never touches managed rows, however many carry NULL', async () => { + // Every managed photo has external_relpath NULL. Grouping on it without + // the NOT NULL filter would make them all one enormous "duplicate" group + // and delete the entire library bar one row. + await knex('photos').insert([ + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + ]); + + await migration.up(knex); + + expect(await knex('photos').count('* as c').first()).toEqual({ c: 3 }); + }); + + it('keeps the row that has a thumbnail, not merely the lowest id', async () => { + // An import killed mid-flight leaves rows without a thumbnail. Dropping + // the completed one would blank a tile in the grid for no reason. + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: null, source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 'thumb.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + const after = await rows(); + expect(after).toHaveLength(1); + expect(after[0].thumbnail_path).toBe('thumb.jpg'); + }); + + it('repoints a hero that pointed at the row being removed', async () => { + // events.hero_photo_id is ON DELETE SET NULL, so without this the cleanup + // silently strips the event's hero image — a visible regression caused + // entirely by the fix. + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + ]); + await knex('events').insert({ id: 1, hero_photo_id: 2 }); + await knex('photo_categories').insert({ id: 1, hero_photo_id: 2 }); + + await migration.up(knex); + + expect((await knex('events').where({ id: 1 }).first()).hero_photo_id).toBe(1); + expect((await knex('photo_categories').where({ id: 1 }).first()).hero_photo_id).toBe(1); + }); + + it('leaves a hero that pointed at the survivor untouched', async () => { + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', thumbnail_path: 't', source_origin: 'external' }, + ]); + await knex('events').insert({ id: 1, hero_photo_id: 1 }); + + await migration.up(knex); + + expect((await knex('events').where({ id: 1 }).first()).hero_photo_id).toBe(1); + }); + + it('makes a second insert of the same path impossible afterwards', async () => { + // The whole point. Without this the route is still racing, and the + // migration is recorded as applied. + await knex('photos').insert({ event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }); + + await migration.up(knex); + + await expect( + knex('photos').insert({ event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }) + ).rejects.toThrow(/unique/i); + }); + + it('still admits managed rows once the index exists', async () => { + await migration.up(knex); + + await knex('photos').insert([ + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + ]); + + expect(await knex('photos').count('* as c').first()).toEqual({ c: 2 }); + }); + + it('leaves nothing dangling behind the deleted row', async () => { + // SQLite never enforces the ON DELETE CASCADE these tables declare, so a + // bare delete strands biometric embeddings, feedback and marks pointing at + // a photo id that no longer exists — on every SQLite install. + await seedPair(); + await knex('photo_faces').insert({ photo_id: 2, event_id: 1 }); + await knex('image_access_logs').insert({ photo_id: 2 }); + + await migration.up(knex); + + expect(await knex('photo_faces').where('photo_id', 2).first()).toBeUndefined(); + expect(await knex('image_access_logs').where('photo_id', 2).first()).toBeUndefined(); + }); + + it('does not carry the duplicate\'s faces over to the survivor', async () => { + // Both rows were scanned independently, so the survivor already holds its + // own embeddings. Moving these would fabricate a second copy of every face + // and split the person clusters built from them. + await seedPair(); + await knex('photo_faces').insert([{ photo_id: 1, event_id: 1 }, { photo_id: 2, event_id: 1 }]); + + await migration.up(knex); + + expect(await knex('photo_faces').count('* as c').first()).toEqual({ c: 1 }); + }); + + it('moves a guest comment to the survivor rather than deleting it', async () => { + // The duplicates were separate tiles in the grid, so a guest could have + // commented on either. Silently dropping that inside a fix for silent data + // loss would be its own bug. + await seedPair(); + await knex('photo_feedback').insert({ + photo_id: 2, event_id: 1, feedback_type: 'comment', + comment_text: 'lovely shot', guest_identifier: 'guest-a', + }); + + await migration.up(knex); + + const rows = await knex('photo_feedback'); + expect(rows).toHaveLength(1); + expect(rows[0].photo_id).toBe(1); + expect(rows[0].comment_text).toBe('lovely shot'); + }); + + it('keeps both comments when the same guest commented on both tiles', async () => { + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 1, event_id: 1, feedback_type: 'comment', comment_text: 'one', guest_identifier: 'g' }, + { photo_id: 2, event_id: 1, feedback_type: 'comment', comment_text: 'two', guest_identifier: 'g' }, + ]); + + await migration.up(knex); + + const rows = await knex('photo_feedback').orderBy('id'); + expect(rows.map((r) => r.comment_text)).toEqual(['one', 'two']); + expect(rows.every((r) => r.photo_id === 1)).toBe(true); + }); + + it('does not double-count a like the same guest left on both tiles', async () => { + // Unlike comments, a like is a per-guest toggle: moving it would show two + // likes from one person. + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 1, event_id: 1, feedback_type: 'like', guest_identifier: 'g' }, + { photo_id: 2, event_id: 1, feedback_type: 'like', guest_identifier: 'g' }, + ]); + + await migration.up(knex); + + expect(await knex('photo_feedback').count('* as c').first()).toEqual({ c: 1 }); + }); + + it('moves a like from a guest the survivor has never seen', async () => { + await seedPair(); + await knex('photo_feedback').insert({ + photo_id: 2, event_id: 1, feedback_type: 'like', guest_identifier: 'other', + }); + + await migration.up(knex); + + const rows = await knex('photo_feedback'); + expect(rows).toHaveLength(1); + expect(rows[0].photo_id).toBe(1); + }); + + it('moves an admin mark, and drops it when that admin already marked the survivor', async () => { + // photo_admin_marks is UNIQUE(photo_id, admin_id), so a blind move would + // throw and abort the migration. + await seedPair(); + await knex('photo_admin_marks').insert([ + { photo_id: 1, event_id: 1, admin_id: 7, rating: 5 }, + { photo_id: 2, event_id: 1, admin_id: 7, rating: 2 }, + { photo_id: 2, event_id: 1, admin_id: 9, rating: 4 }, + ]); + + await migration.up(knex); + + const rows = await knex('photo_admin_marks').orderBy('admin_id'); + expect(rows.map((r) => [r.admin_id, r.rating])).toEqual([[7, 5], [9, 4]]); + expect(rows.every((r) => r.photo_id === 1)).toBe(true); + }); + + it('respects the transfer_files uniqueness when moving membership', async () => { + await seedPair(); + await knex('transfer_files').insert([ + { transfer_id: 3, photo_id: 1 }, + { transfer_id: 3, photo_id: 2 }, + { transfer_id: 4, photo_id: 2 }, + ]); + + await migration.up(knex); + + const rows = await knex('transfer_files').orderBy('transfer_id'); + expect(rows.map((r) => r.transfer_id)).toEqual([3, 4]); + expect(rows.every((r) => r.photo_id === 1)).toBe(true); + }); + + it('recomputes the survivor\'s feedback totals after reparenting rows', async () => { + // photos carries denormalized counters (migration 033). A survivor that + // now OWNS the feedback but still renders zero is the visible half of + // getting this wrong. + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 2, event_id: 1, feedback_type: 'like', guest_identifier: 'g1' }, + { photo_id: 2, event_id: 1, feedback_type: 'rating', rating: 4, guest_identifier: 'g1' }, + ]); + + await migration.up(knex); + + const survivor = await knex('photos').where('id', 1).first(); + expect(survivor.like_count).toBe(1); + expect(Number(survivor.average_rating)).toBe(4); + expect(survivor.feedback_count).toBe(1); + }); + + it('keeps two people who share a device apart', async () => { + // guest_identifier is per-device; guest_id is per-person (migration 078), + // and feedbackService scopes by guest_id when it is present. Keying on the + // identifier alone would read these as one person and delete a rating. + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 1, event_id: 1, feedback_type: 'rating', rating: 5, guest_identifier: 'shared', guest_id: 10 }, + { photo_id: 2, event_id: 1, feedback_type: 'rating', rating: 2, guest_identifier: 'shared', guest_id: 11 }, + ]); + + await migration.up(knex); + + const rows = await knex('photo_feedback').orderBy('guest_id'); + expect(rows.map((r) => [r.guest_id, r.rating])).toEqual([[10, 5], [11, 2]]); + }); + + it('still dedupes one person voting on both tiles', async () => { + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 1, event_id: 1, feedback_type: 'like', guest_identifier: 'shared', guest_id: 10 }, + { photo_id: 2, event_id: 1, feedback_type: 'like', guest_identifier: 'shared', guest_id: 10 }, + ]); + + await migration.up(knex); + + expect(await knex('photo_feedback').count('* as c').first()).toEqual({ c: 1 }); + }); + + it('rebuilds the people that lose members, rather than deleting faces raw', async () => { + // purgePhotoFaces is "called from every photo-deletion path" precisely + // because event_people counts and centroids are derived from the rows + // being removed. A bare delete leaves a ghost person behind. + await seedPair(); + await knex('event_people').insert({ id: 5, event_id: 1, face_count: 1 }); + await knex('photo_faces').insert({ photo_id: 2, event_id: 1, person_id: 5 }); + + await migration.up(knex); + + expect(await knex('photo_faces').count('* as c').first()).toEqual({ c: 0 }); + // The person had exactly one member and loses it, so it goes with it. + expect(await knex('event_people').where('id', 5).first()).toBeUndefined(); + }); + + it('keeps a hidden moderation record from swallowing the visible replacement', async () => { + // feedbackService lets both coexist and counts only the visible one. + await seedPair(); + await knex('photo_feedback').insert([ + { photo_id: 1, event_id: 1, feedback_type: 'like', guest_identifier: 'g', is_hidden: true }, + { photo_id: 2, event_id: 1, feedback_type: 'like', guest_identifier: 'g', is_hidden: false }, + ]); + + await migration.up(knex); + + expect(await knex('photo_feedback').count('* as c').first()).toEqual({ c: 2 }); + }); + + it('merges the independent halves of one admin\'s mark', async () => { + // rating and color_label are written independently, so the same admin can + // have rated one tile and coloured the other. + await seedPair(); + await knex('photo_admin_marks').insert([ + { photo_id: 1, event_id: 1, admin_id: 7, rating: 5, color_label: null }, + { photo_id: 2, event_id: 1, admin_id: 7, rating: null, color_label: 'red' }, + ]); + + await migration.up(knex); + + const rows = await knex('photo_admin_marks'); + expect(rows).toHaveLength(1); + expect([rows[0].rating, rows[0].color_label]).toEqual([5, 'red']); + }); + + it('requeues the survivor when the duplicate held the only scan', async () => { + // Otherwise the sole embeddings go with the purge and nothing re-queues: + // the photo just silently stops having a face. + await seedPair(); + await knex('photo_faces').insert({ photo_id: 2, event_id: 1 }); + + await migration.up(knex); + + expect((await knex('photos').where('id', 1).first()).face_status).toBe('pending'); + }); + + it('carries the duplicate\'s views and downloads over', async () => { + await seedPair(); + await knex('photos').where('id', 1).update({ view_count: 2, download_count: 1 }); + await knex('photos').where('id', 2).update({ view_count: 5, download_count: 3 }); + + await migration.up(knex); + + const survivor = await knex('photos').where('id', 1).first(); + expect([survivor.view_count, survivor.download_count]).toEqual([7, 4]); + }); + + it('fails loudly rather than recording itself applied without the index', async () => { + // Swallowing a failed CREATE INDEX would leave the install permanently + // racy — the in-flight guard only covers one process — with nothing to + // trigger a retry. Driven through the helper the migration calls, against + // a table that still holds duplicates — i.e. what it would face if the + // dedupe above had not achieved uniqueness. + await seedPair(); + const { createExternalRelpathIndex } = require('../../src/services/externalPhotoDedupe'); + + await expect(createExternalRelpathIndex(knex)).rejects.toThrow(/unique/i); + }); + + it('invalidates the pre-built download zip for the affected event', async () => { + // The cached archive still contains the rows just removed, and every + // ordinary photo-deletion path invalidates it for exactly that reason. + // getZipInfo treats a cleared record as a miss and rebuilds on request. + await seedPair(); + await knex('events').insert({ + id: 1, download_zip_path: 'events/active/x/.download-cache/all.zip', + download_zip_generated_at: '2026-01-01', + }); + + await migration.up(knex); + + const ev = await knex('events').where('id', 1).first(); + expect(ev.download_zip_path).toBeNull(); + expect(ev.download_zip_generated_at).toBeNull(); + }); + + it('leaves an untouched event\'s zip alone', async () => { + await seedPair(); + await knex('events').insert([ + { id: 1, download_zip_path: 'a.zip', download_zip_generated_at: '2026-01-01' }, + { id: 2, download_zip_path: 'b.zip', download_zip_generated_at: '2026-01-01' }, + ]); + + await migration.up(knex); + + expect((await knex('events').where('id', 2).first()).download_zip_path).toBe('b.zip'); + }); + + it('is idempotent', async () => { + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + const once = await rows(); + await migration.up(knex); + + expect(await rows()).toEqual(once); + }); + + it('rolls back to an unconstrained table', async () => { + await migration.up(knex); + await migration.down(knex); + + await knex('photos').insert([ + { event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }, + { event_id: 1, external_relpath: 'a/x.jpg', source_origin: 'external' }, + ]); + expect(await knex('photos').count('* as c').first()).toEqual({ c: 2 }); + }); + + it('no-ops before 041 has added the column', async () => { + await knex.schema.dropTableIfExists('photos'); + await knex.schema.createTable('photos', (t) => { t.increments('id').primary(); }); + + await expect(migration.up(knex)).resolves.toBeUndefined(); + }); +}); diff --git a/backend/__tests__/services/externalPhotoDedupe.restore.test.js b/backend/__tests__/services/externalPhotoDedupe.restore.test.js new file mode 100644 index 00000000..2fca042d --- /dev/null +++ b/backend/__tests__/services/externalPhotoDedupe.restore.test.js @@ -0,0 +1,92 @@ +/** + * A pre-#1162 backup must still restore (#1162 review). + * + * `replaceAllTables` suspends FOREIGN KEY enforcement for the load — Postgres + * via `session_replication_role = replica`, SQLite via `defer_foreign_keys` — + * but neither of those suspends a UNIQUE index. An archive taken before + * migration 186 carries exactly the duplicate photo rows that migration + * removes, so the batchInsert would hit the new index and roll the entire + * restore back, after every table had already been emptied. + * + * These pin the drop → load → dedupe → recreate sequence the restore now + * performs, and the failure it exists to prevent. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const { + dedupeExternalPhotos, + createExternalRelpathIndex, + dropExternalRelpathIndex, +} = require('../../src/services/externalPhotoDedupe'); + +describe('restoring an archive that predates the unique index (#1162)', () => { + let knex; let tmpDir; + + // What a pre-186 archive's photos.ndjson holds for a racing import: the same + // file twice, sub-millisecond apart. + const ARCHIVE_ROWS = [ + { id: 1, event_id: 1, external_relpath: 'Trip/a.jpg', source_origin: 'external' }, + { id: 2, event_id: 1, external_relpath: 'Trip/a.jpg', source_origin: 'external' }, + { id: 3, event_id: 1, external_relpath: 'Trip/b.jpg', source_origin: 'external' }, + ]; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-restore-dedupe-')); + knex = require('knex')({ + client: 'sqlite3', + connection: { filename: path.join(tmpDir, 'db.sqlite') }, + useNullAsDefault: true, + }); + }); + + afterAll(async () => { + if (knex) await knex.destroy(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + await knex.schema.dropTableIfExists('photos'); + await knex.schema.createTable('photos', (t) => { + t.integer('id').primary(); + t.integer('event_id'); + t.string('external_relpath'); + t.string('thumbnail_path'); + t.string('source_origin').defaultTo('managed'); + }); + await createExternalRelpathIndex(knex); + }); + + it('would abort the whole restore without the drop', async () => { + // The regression, stated directly: this is what the target instance does + // today when handed a legacy archive. + await expect(knex.batchInsert('photos', ARCHIVE_ROWS, 100)).rejects.toThrow(/unique/i); + }); + + it('loads, dedupes and comes back constrained', async () => { + await dropExternalRelpathIndex(knex); + await knex.batchInsert('photos', ARCHIVE_ROWS, 100); + + const removed = await dedupeExternalPhotos(knex); + await createExternalRelpathIndex(knex); + + expect(removed).toBe(1); + expect((await knex('photos').orderBy('id')).map((r) => r.external_relpath)) + .toEqual(['Trip/a.jpg', 'Trip/b.jpg']); + // The target must not be left unprotected by the restore that dropped it. + await expect( + knex('photos').insert({ id: 9, event_id: 1, external_relpath: 'Trip/b.jpg', source_origin: 'external' }) + ).rejects.toThrow(/unique/i); + }); + + it('is a no-op for an archive that has no duplicates', async () => { + await dropExternalRelpathIndex(knex); + await knex.batchInsert('photos', ARCHIVE_ROWS.slice(1), 100); + + expect(await dedupeExternalPhotos(knex)).toBe(0); + await expect(createExternalRelpathIndex(knex)).resolves.toBeUndefined(); + expect(await knex('photos').count('* as c').first()).toEqual({ c: 2 }); + }); +}); diff --git a/backend/migrations/core/186_external_relpath_unique.js b/backend/migrations/core/186_external_relpath_unique.js new file mode 100644 index 00000000..112c5f66 --- /dev/null +++ b/backend/migrations/core/186_external_relpath_unique.js @@ -0,0 +1,60 @@ +/** + * Migration 186: one row per external file per event (#1162). + * + * The import route checked for an existing external_relpath and then inserted, + * with an fs.stat and a sharp().metadata() call sitting in between — a window + * wide enough that two overlapping imports of the same folder each see "not + * there" and both insert. Nothing at the storage layer stopped them: 041 + * created only a NON-unique (event_id, source_origin) index. A reporter's + * event ended up holding 8004 rows for 6012 distinct paths. + * + * So this does two things: clear the duplicates that already exist, and add + * the constraint that makes the race unwinnable from here on. + * + * The work — which row survives, what happens to the guest feedback and admin + * marks hanging off the loser, and why the dependent rows are deleted by hand + * rather than left to ON DELETE CASCADE — lives in + * services/externalPhotoDedupe.js, because a .picpeak restore has to run it + * too: the archive carries the photos table verbatim, so a pre-#1162 backup + * would otherwise hit the unique index mid-restore and roll the whole thing + * back. + * + * Irreversible by design: down() drops the index but cannot resurrect the + * deleted rows. They were never distinct data — the same file counted twice. + * + * What it does NOT do is delete the duplicates' thumbnail files. Those are + * `ext_` keys under the thumbnail root, and a migration is the wrong + * place to reach into storage — the backend may be pointed at S3, and a failed + * object delete must not fail the schema change. They are left behind as + * unreferenced bytes; the storage figures on the dashboard count them, which + * is the correct answer to "what is on the disk". + */ + +const { + dedupeExternalPhotos, + createExternalRelpathIndex, + dropExternalRelpathIndex, +} = require('../../src/services/externalPhotoDedupe'); + +exports.up = async function(knex) { + if (!(await knex.schema.hasTable('photos'))) return; + if (!(await knex.schema.hasColumn('photos', 'external_relpath'))) return; + + const removed = await dedupeExternalPhotos(knex); + if (removed) { + console.log(`186_external_relpath_unique: removed ${removed} duplicate external photo row(s)`); + } + + // Deliberately unguarded. Recording this migration as applied without the + // index would leave the install permanently racy — the in-flight set only + // covers one process, and the route's unique-violation path cannot converge + // without a constraint to violate — with nothing to trigger a retry. A + // failure here means the dedupe above did not achieve uniqueness, which is + // worth stopping the upgrade for. + await createExternalRelpathIndex(knex); +}; + +exports.down = async function(knex) { + if (!(await knex.schema.hasTable('photos'))) return; + await dropExternalRelpathIndex(knex); +}; diff --git a/backend/src/routes/adminExternalMedia.js b/backend/src/routes/adminExternalMedia.js index d4507ed6..98a60be2 100644 --- a/backend/src/routes/adminExternalMedia.js +++ b/backend/src/routes/adminExternalMedia.js @@ -9,9 +9,24 @@ const { db, logActivity } = require('../database/db'); const sharp = require('sharp'); const logger = require('../utils/logger'); const { generateThumbnail } = require('../services/imageProcessor'); +const { isUniqueViolation } = require('../utils/dbErrors'); const router = express.Router(); +// Events with an import running in THIS process (#1162). +// +// The second line of defence, not the first: migration 186 puts a unique index +// on (event_id, external_relpath), and that is what actually makes a duplicate +// impossible — it holds across replicas, across restarts, and against anything +// that inserts external rows without going through this route. +// +// This set exists for the reason the duplicates got filed in the first place: +// a large tree takes long enough that the run LOOKS hung, so admins click +// again. Letting that second run walk the whole tree only to have every insert +// bounce off the index wastes minutes of CPU and reports a nonsense +// `skipped: 6012` back. Failing it immediately with 409 says what happened. +const importsInFlight = new Set(); + // GET /api/admin/external-media/list?path=relative/dir router.get('/list', adminAuth, requirePermission('photos.view'), async (req, res) => { try { @@ -50,8 +65,14 @@ async function walkDir(dir, baseDir) { // POST /api/admin/events/:id/import-external // Body: { external_path: string, recursive?: boolean, map?: { individual?: string, collages?: string } } router.post('/events/:id/import-external', adminAuth, requirePermission('photos.upload'), requireEventOwnership, async (req, res) => { + const eventId = parseInt(req.params.id); + if (importsInFlight.has(eventId)) { + return res.status(409).json({ + error: 'An import is already running for this event. Wait for it to finish before starting another.' + }); + } + importsInFlight.add(eventId); try { - const eventId = parseInt(req.params.id); const { external_path, recursive = true, map = { individual: 'individual', collages: 'collages' } } = req.body || {}; if (!external_path) return res.status(400).json({ error: 'external_path is required' }); @@ -155,7 +176,13 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. if (segs[0] === map.individual) type = 'individual'; try { - // Check if already exists (by external_relpath) + // Fast path only. This SELECT settles the common case — a re-import of + // a folder already in the event — without paying for a stat and a + // Sharp metadata read per file. It is NOT the guard: those two calls + // sit between here and the INSERT below, which is exactly the window + // two overlapping imports both walked through (#1162). The unique + // index from migration 186 is the guard, and the catch below is how + // this loop converges when it fires. const exists = await db('photos') .where({ event_id: eventId, external_relpath: f.rel }) .first(); @@ -173,21 +200,33 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. logger.warn(`Could not extract dimensions for ${f.rel}: ${dimErr.message}`); } - const inserted = await db('photos') - .insert({ - event_id: eventId, - filename: f.name, - // Keep path as a hint for legacy code but not used for resolution in external mode - path: path.join(event.slug, f.name), - thumbnail_path: null, - type, - size_bytes: stats.size, - width, - height, - source_origin: 'external', - external_relpath: f.rel - }) - .returning('id'); + let inserted; + try { + inserted = await db('photos') + .insert({ + event_id: eventId, + filename: f.name, + // Keep path as a hint for legacy code but not used for resolution in external mode + path: path.join(event.slug, f.name), + thumbnail_path: null, + type, + size_bytes: stats.size, + width, + height, + source_origin: 'external', + external_relpath: f.rel + }) + .returning('id'); + } catch (insertErr) { + // Another writer inserted this exact path while we were reading + // metadata. That is the outcome the index exists to produce, and it + // is a skip rather than a failure — the row is there, it just isn't + // ours. Counting it as `skipped` keeps the reported totals honest; + // before the index this landed in the outer catch as a nameless + // failure, or (more often) never fired at all and duplicated the row. + if (isUniqueViolation(insertErr)) { skipped++; continue; } + throw insertErr; + } const photoId = Array.isArray(inserted) && inserted.length ? (typeof inserted[0] === 'object' ? inserted[0].id : inserted[0]) @@ -275,6 +314,11 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. error: error.message }); res.status(500).json({ error: 'Failed to import external media' }); + } finally { + // In `finally` and not at the end of `try`: an import that throws must + // still release the event, or a single failure locks out every retry + // until the process restarts. + importsInFlight.delete(eventId); } }); diff --git a/backend/src/services/externalPhotoDedupe.js b/backend/src/services/externalPhotoDedupe.js new file mode 100644 index 00000000..8d1a8e51 --- /dev/null +++ b/backend/src/services/externalPhotoDedupe.js @@ -0,0 +1,389 @@ +/** + * One row per external file per event (#1162). + * + * The import route used to check for an existing external_relpath and then + * insert, with an fs.stat and a Sharp decode in between — wide enough that two + * overlapping imports both walked through it. A reporter's event held 8004 rows + * for 6012 distinct paths. + * + * This lives in a service rather than inside migration 186 because it has two + * callers. The migration is one. The other is a .picpeak restore: the archive + * carries the photos table verbatim, so a backup taken before this fix lands + * duplicate rows into a schema that now has a unique index on them — and + * neither Postgres' `session_replication_role = replica` nor SQLite's + * `defer_foreign_keys` disables a UNIQUE index, so batchInsert would abort the + * whole restore after every table had already been emptied. + * + * DELETING dependent rows explicitly, rather than trusting ON DELETE CASCADE, + * is the load-bearing part. Every FK into photos declares CASCADE, but PicPeak + * does not set `PRAGMA foreign_keys = ON` — the codebase says so in as many + * words where it deletes an event (adminEvents/helpers.js:245-249) — so on + * every SQLite install the cascade is inert and a bare delete would leave + * dangling face embeddings, feedback and marks behind. The same reason + * `hero_photo_id` is repointed by hand: its SET NULL is inert there too, so + * without it a SQLite install keeps a hero pointing at a row that is gone. + * + * Guest and admin state is MOVED to the survivor where it can be, not + * discarded. The duplicates were separate tiles in the grid, so a guest's + * comment or an admin's rating could legitimately be attached to either, and + * silently deleting it inside a fix for silent data loss would be its own bug. + * Where the target already holds an equivalent row — the same guest's like on + * the same photo, the same admin's mark, the same transfer's entry — the loser + * is dropped instead, because those tables mean "one per (photo, actor)" and + * moving would either violate a unique constraint or double-count. + * + * photo_faces is the deliberate exception: both rows were scanned + * independently, so the survivor already has its own embeddings and moving the + * duplicate's would fabricate a second copy of every face and split the + * person clusters built from them. + */ + +const { isUniqueViolation } = require('../utils/dbErrors'); + +const CHUNK = 400; // SQLite caps a statement at 999 bound parameters. +// Joins the parts of an equivalence key. Escaped, not a literal: a raw NUL in +// the source makes git classify this whole file as binary and hide its diffs. +const KEY_SEP = '\u0000'; +const INDEX_NAME = 'photos_event_external_relpath_uniq'; + +const chunked = (arr) => { + const out = []; + for (let i = 0; i < arr.length; i += CHUNK) out.push(arr.slice(i, i + CHUNK)); + return out; +}; + +/** Pure log rows — nothing is lost by dropping them with the duplicate. */ +const LOG_TABLES = [ + ['image_access_logs', 'photo_id'], + ['transfer_downloads', 'photo_id'], +]; + +/** + * Tables holding one row per (photo, actor). `keys` is what makes two rows + * equivalent, so a move that would collide becomes a delete instead. + */ +const MOVE_TABLES = [ + { + table: 'photo_feedback', + // Guest identity, the way feedbackService defines it: guest_id when the + // gallery uses per-person guests (migration 078), guest_identifier + // otherwise — the same COALESCE its own duplicate-check and stats + // aggregate use (feedbackService.js:208, :559). Keying on + // guest_identifier alone would treat two DIFFERENT people sharing a + // device as one and delete one of their ratings. + identity: (row) => (row.guest_id != null ? `id:${row.guest_id}` : `anon:${row.guest_identifier}`), + // is_hidden is part of the identity, not noise: feedbackService lets a + // moderator-hidden row coexist with the guest's visible replacement and + // excludes hidden rows from the counts. Without it the visible row is + // dropped as redundant against the hidden one. + keys: ['feedback_type', 'is_hidden'], + // A comment is distinct content, never a per-guest toggle: two comments + // from one guest are two comments, so they always move. + alwaysMove: (row) => row.feedback_type === 'comment', + }, + { + table: 'photo_admin_marks', + keys: ['admin_id'], + // rating and color_label are independently writable, so the same admin can + // have rated one tile and colour-labelled the other. Dropping the loser + // outright would lose a half the survivor's row has no value for. + mergeFields: ['rating', 'color_label'], + }, + { table: 'transfer_files', keys: ['transfer_id'] }, +]; + +const equivalenceKey = (spec, row) => [ + spec.identity ? spec.identity(row) : '', + ...spec.keys.map((k) => row[k]), +].join(KEY_SEP); + +async function repointColumn(knex, table, column, doomedToSurvivor) { + if (!(await knex.schema.hasTable(table))) return; + if (!(await knex.schema.hasColumn(table, column))) return; + for (const [doomed, survivor] of doomedToSurvivor) { + await knex(table).where(column, doomed).update({ [column]: survivor }); + } +} + +async function moveOrDrop(knex, spec, doomedToSurvivor, touched) { + if (!(await knex.schema.hasTable(spec.table))) return; + + for (const [doomed, survivor] of doomedToSurvivor) { + const rows = await knex(spec.table).where('photo_id', doomed); + if (!rows.length) continue; + if (touched) touched.add(survivor); + + const existing = await knex(spec.table).where('photo_id', survivor); + const taken = new Set(existing.map((r) => equivalenceKey(spec, r))); + + for (const row of rows) { + const key = equivalenceKey(spec, row); + const move = (spec.alwaysMove && spec.alwaysMove(row)) || !taken.has(key); + if (!move) { + // Before dropping the loser, hand over any field the winner has no + // value for — otherwise an independently-set half goes with it. + if (spec.mergeFields) { + const winner = existing.find((r) => equivalenceKey(spec, r) === key); + const fill = {}; + for (const field of spec.mergeFields) { + if (winner && winner[field] == null && row[field] != null) fill[field] = row[field]; + } + if (winner && Object.keys(fill).length) { + await knex(spec.table).where('id', winner.id).update(fill); + Object.assign(winner, fill); + } + } + await knex(spec.table).where('id', row.id).del(); + continue; + } + try { + await knex(spec.table).where('id', row.id).update({ photo_id: survivor }); + taken.add(key); + } catch (err) { + // A unique constraint we did not model. The row is redundant with one + // the survivor already has, so dropping it is correct — but anything + // else must surface rather than leave a dangling photo_id behind. + if (!isUniqueViolation(err)) throw err; + await knex(spec.table).where('id', row.id).del(); + } + } + } +} + +/** + * Remove every photo row in `doomedToSurvivor`, moving or dropping the state + * that hangs off it first. Safe on both engines and on schemas that predate + * any of the dependent tables. + */ +async function deleteDuplicatePhotos(knex, doomedToSurvivor) { + const doomed = [...doomedToSurvivor.keys()]; + if (!doomed.length) return 0; + + // SET NULL is as inert as CASCADE on SQLite, so an event whose hero happened + // to be the duplicate would silently lose its hero image. + await repointColumn(knex, 'events', 'hero_photo_id', doomedToSurvivor); + await repointColumn(knex, 'photo_categories', 'hero_photo_id', doomedToSurvivor); + + const feedbackTouched = new Set(); + for (const spec of MOVE_TABLES) { + await moveOrDrop(knex, spec, doomedToSurvivor, spec.table === 'photo_feedback' ? feedbackTouched : null); + } + + // photos carries denormalized feedback totals (migration 033: + // feedback_count, like_count, average_rating, favorite_count, and later + // reaction/colour counts). Reparenting rows without recomputing leaves a + // survivor that now OWNS feedback still rendering zero. + if (feedbackTouched.size && await knex.schema.hasColumn('photos', 'feedback_count')) { + const feedbackService = require('./feedbackService'); + for (const survivor of feedbackTouched) { + await feedbackService.updatePhotoFeedbackStats(survivor, knex); + } + } + + for (const [table, column] of LOG_TABLES) { + if (!(await knex.schema.hasTable(table))) continue; + for (const ids of chunked(doomed)) await knex(table).whereIn(column, ids).del(); + } + + // Both rows were scanned, so the survivor has its own faces; moving the + // duplicate's would double every embedding and split the person clusters. + // + // Through purgePhotoFaces, not a raw delete: deleting the rows is only half + // of it. event_people counts and centroids are derived from the faces being + // removed, and #1132's separation snapshots hold a COPY of each side's + // centroid — so a bare delete leaves ghost or inflated people and vectors + // built from photos that no longer exist. faceProcessor says as much: it is + // "called from every photo-deletion path". + if (await knex.schema.hasTable('photo_faces')) { + // If the ONLY completed scan of this file belonged to the duplicate, the + // purge below takes the sole embeddings with it and nothing re-queues the + // survivor — it just silently stops having a face. Mark those for a + // rescan; the worker picks up 'pending' on its own. + const needsRescan = []; + for (const [doomedId, survivorId] of doomedToSurvivor) { + if (!(await knex('photo_faces').where('photo_id', doomedId).first())) continue; + if (!(await knex('photo_faces').where('photo_id', survivorId).first())) needsRescan.push(survivorId); + } + + let purgePhotoFaces = null; + try { + ({ purgePhotoFaces } = require('./faceProcessor')); + } catch (err) { + // Face detection is optional; an install without it still needs the rows + // gone so nothing dangles on SQLite. + purgePhotoFaces = null; + } + if (purgePhotoFaces) { + for (const id of doomed) await purgePhotoFaces(id, knex); + } else { + for (const ids of chunked(doomed)) await knex('photo_faces').whereIn('photo_id', ids).del(); + } + + if (needsRescan.length && await knex.schema.hasColumn('photos', 'face_status')) { + for (const ids of chunked(needsRescan)) { + await knex('photos').whereIn('id', ids).update({ face_status: 'pending' }); + } + } + } + + // Real interactions, recorded per row. Deleting the duplicate would quietly + // lower the engagement the admin grid shows for a photo people did view and + // download. + if (await knex.schema.hasColumn('photos', 'view_count')) { + for (const [doomedId, survivorId] of doomedToSurvivor) { + const from = await knex('photos').where('id', doomedId) + .select('view_count', 'download_count').first(); + if (!from) continue; + const add = {}; + if (from.view_count) add.view_count = knex.raw('COALESCE(view_count, 0) + ?', [from.view_count]); + if (from.download_count) add.download_count = knex.raw('COALESCE(download_count, 0) + ?', [from.download_count]); + if (Object.keys(add).length) await knex('photos').where('id', survivorId).update(add); + } + } + + for (const ids of chunked(doomed)) await knex('photos').whereIn('id', ids).del(); + + // The pre-built "download everything" zip still contains the rows just + // removed. Every ordinary photo-deletion path calls + // downloadZipService.invalidate for this reason (adminPhotos.js:450 and + // friends) — but that service carries debounce timers and a regeneration + // queue, which is not something a migration should be starting. Clearing the + // columns is the durable half of what invalidate does: getZipInfo already + // treats a missing or absent record as a cache miss and rebuilds on the next + // request, so guests stop receiving an archive containing deleted duplicates. + // + // The stale object itself is left for the same reason the duplicates' + // thumbnails are — a migration is the wrong place to reach into storage, + // which may be S3. + if (await knex.schema.hasColumn('events', 'download_zip_path')) { + const affected = [...new Set([...doomedToSurvivor.values()])]; + const eventIds = affected.length + ? (await knex('photos').whereIn('id', affected).distinct('event_id')).map((r) => r.event_id) + : []; + for (const ids of chunked(eventIds)) { + await knex('events').whereIn('id', ids).update({ + download_zip_path: null, + download_zip_generated_at: null, + }); + } + } + + return doomed.length; +} + +/** + * Which rows are duplicates, and which one survives. + * + * Survivor: the lowest id that has a thumbnail_path, else the lowest id. + * Thumbnails are generated per row during import, so on a duplicated pair both + * usually have one and the tie-break never fires — but an import killed + * mid-flight leaves rows without, and dropping the one that HAS the thumbnail + * would blank a grid tile for no reason. + */ +async function planDedupe(knex) { + const dupKeys = await knex('photos') + .whereNotNull('external_relpath') + .select('event_id') + .count('* as c') + .groupBy('event_id', 'external_relpath') + .havingRaw('count(*) > 1'); + + const doomedToSurvivor = new Map(); + + for (const eventId of new Set(dupKeys.map((r) => r.event_id))) { + const rows = await knex('photos') + .where('event_id', eventId) + .whereNotNull('external_relpath') + .select('id', 'external_relpath', 'thumbnail_path') + .orderBy('id', 'asc'); + + const byPath = new Map(); + for (const row of rows) { + const group = byPath.get(row.external_relpath); + if (group) group.push(row); + else byPath.set(row.external_relpath, [row]); + } + + for (const group of byPath.values()) { + if (group.length < 2) continue; + const survivor = group.find((r) => r.thumbnail_path) || group[0]; + for (const row of group) { + if (row.id !== survivor.id) doomedToSurvivor.set(row.id, survivor.id); + } + } + } + + return doomedToSurvivor; +} + +/** @returns {Promise} how many duplicate rows were removed. */ +async function dedupeExternalPhotos(knex) { + if (!(await knex.schema.hasTable('photos'))) return 0; + if (!(await knex.schema.hasColumn('photos', 'external_relpath'))) return 0; + return deleteDuplicatePhotos(knex, await planDedupe(knex)); +} + +/** Is the index actually there? Asked of the catalog, not inferred. */ +async function externalRelpathIndexExists(knex) { + const isPg = knex.client && knex.client.config && knex.client.config.client === 'pg'; + const row = isPg + ? await knex('pg_indexes').where('indexname', INDEX_NAME).first() + : await knex('sqlite_master').where({ type: 'index', name: INDEX_NAME }).first(); + return !!row; +} + +/** + * The error a failed index MUST raise. + * + * Deliberately carries no `code`. run-migrations-safe.js treats 23505, 42P07, + * 42701 and 42710 as "schema already exists" and marks the migration applied + * (run-migrations-safe.js:138) — and a CREATE UNIQUE INDEX that finds + * duplicate rows raises exactly 23505 on Postgres. Letting the driver's error + * through would therefore record 186 as done on an install that never got the + * index, with nothing to trigger a retry: the precise outcome the throw + * exists to prevent. + */ +function indexFailure(detail) { + return new Error( + `Could not create ${INDEX_NAME}: ${detail}. The photos table still holds ` + + 'duplicate (event_id, external_relpath) rows — most likely inserted by a ' + + 'concurrent import while this migration ran. Stop other writers and re-run.' + ); +} + +/** + * Partial, so the managed rows — which all carry NULL — are not indexed at + * all. Both engines treat NULLs as distinct in a unique index, so a plain one + * would also be correct, but it would carry every managed photo for no query + * that ever uses it. + */ +async function createExternalRelpathIndex(knex) { + try { + await knex.raw( + `CREATE UNIQUE INDEX IF NOT EXISTS ${INDEX_NAME} ` + + 'ON photos (event_id, external_relpath) WHERE external_relpath IS NOT NULL' + ); + } catch (err) { + throw indexFailure(err.message); + } + // IF NOT EXISTS makes the statement itself a poor witness, and a replica + // inserting a duplicate between the dedupe and this lock is a real rolling- + // deploy shape. Ask the catalog. + if (!(await externalRelpathIndexExists(knex))) { + throw indexFailure('the index is absent afterwards'); + } +} + +async function dropExternalRelpathIndex(knex) { + await knex.raw(`DROP INDEX IF EXISTS ${INDEX_NAME}`); +} + +module.exports = { + dedupeExternalPhotos, + externalRelpathIndexExists, + deleteDuplicatePhotos, + planDedupe, + createExternalRelpathIndex, + dropExternalRelpathIndex, + INDEX_NAME, +}; diff --git a/backend/src/services/feedbackService.js b/backend/src/services/feedbackService.js index e983cf37..6a28be1e 100644 --- a/backend/src/services/feedbackService.js +++ b/backend/src/services/feedbackService.js @@ -541,27 +541,32 @@ class FeedbackService { } /** - * Update photo feedback statistics + * Update photo feedback statistics. + * + * `trx` so a caller running outside the request path — the duplicate-photo + * dedupe (#1162), which reparents feedback rows and must leave the + * survivor's denormalized totals correct — can recompute on its own + * connection. */ - async updatePhotoFeedbackStats(photoId) { + async updatePhotoFeedbackStats(photoId, trx = db) { try { // Get aggregated stats - const stats = await db('photo_feedback') + const stats = await trx('photo_feedback') .where('photo_id', photoId) .where('is_hidden', false) .select( - db.raw('COUNT(CASE WHEN feedback_type = ? AND is_approved = ? THEN 1 END) as comment_count', ['comment', formatBoolean(true)]), - db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as like_count', ['like']), - db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as favorite_count', ['favorite']), - db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as reaction_count', ['reaction']), - db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as color_label_count', ['color_label']), - db.raw('AVG(CASE WHEN feedback_type = ? THEN rating END) as average_rating', ['rating']), - db.raw('COUNT(DISTINCT COALESCE(CAST(guest_id AS VARCHAR), guest_identifier)) as feedback_count') + trx.raw('COUNT(CASE WHEN feedback_type = ? AND is_approved = ? THEN 1 END) as comment_count', ['comment', formatBoolean(true)]), + trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as like_count', ['like']), + trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as favorite_count', ['favorite']), + trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as reaction_count', ['reaction']), + trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as color_label_count', ['color_label']), + trx.raw('AVG(CASE WHEN feedback_type = ? THEN rating END) as average_rating', ['rating']), + trx.raw('COUNT(DISTINCT COALESCE(CAST(guest_id AS VARCHAR), guest_identifier)) as feedback_count') ) .first(); // Update photo table - await db('photos') + await trx('photos') .where('id', photoId) .update({ feedback_count: stats.feedback_count || 0, diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index fa3beab7..32f27c97 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -27,6 +27,11 @@ const { hasColumnCached } = require('../utils/schemaCache'); const { setSessionsValidAfter } = require('../utils/sessionCutoff'); const logger = require('../utils/logger'); const { PICPEAK_FORMAT_VERSION, EXCLUDED_TABLES, listDataTables } = require('./picpeakExportService'); +const { + dedupeExternalPhotos, + createExternalRelpathIndex, + dropExternalRelpathIndex, +} = require('./externalPhotoDedupe'); const isPostgres = () => knexConfig.client === 'pg'; @@ -348,6 +353,19 @@ async function replaceAllTables(tables, dataDir, currentAdmin, roleSnapshot, { c await trx.raw('PRAGMA defer_foreign_keys = ON'); } + // Suspending FK enforcement does not suspend UNIQUE indexes on either + // engine (#1162). A backup taken before migration 186 carries the + // duplicate photo rows that migration exists to remove, so batchInsert + // below would hit photos_event_external_relpath_uniq and roll the whole + // restore back — after every table had already been emptied. Drop it for + // the load and rebuild it once the rows are deduped, which is the same + // repair the migration performs. + let hadRelpathIndex = false; + if (await trx.schema.hasColumn('photos', 'external_relpath')) { + hadRelpathIndex = true; + await dropExternalRelpathIndex(trx); + } + for (const table of tables) { await trx(table).del(); } @@ -384,6 +402,18 @@ async function replaceAllTables(tables, dataDir, currentAdmin, roleSnapshot, { c await trx.batchInsert(table, prepared, 100); } + // Restore the constraint the load ran without. Deduping first because the + // incoming rows may be exactly the duplicates migration 186 removes; the + // index creation then also proves the repair worked, inside the same + // transaction that would otherwise leave the target unprotected. + if (hadRelpathIndex) { + const removed = await dedupeExternalPhotos(trx); + if (removed) { + logger.info(`picpeakImport: removed ${removed} duplicate external photo row(s) from the archive (#1162)`); + } + await createExternalRelpathIndex(trx); + } + const operatorId = await reinjectCurrentAdmin(trx, currentAdmin); if (operatorId && roleSnapshot) { await preserveOperatorRole(trx, operatorId, roleSnapshot);