diff --git a/backend/__tests__/integration/externalImportConcurrency.test.js b/backend/__tests__/integration/externalImportConcurrency.test.js index 6bd72ab7..2cd340a0 100644 --- a/backend/__tests__/integration/externalImportConcurrency.test.js +++ b/backend/__tests__/integration/externalImportConcurrency.test.js @@ -173,14 +173,16 @@ describe('concurrent external imports (#1162)', () => { path: 'x/a.jpg', type: 'individual', source_origin: 'external', - external_relpath: path.join('individual', 'a.jpg'), + // Root-relative, as the route now writes it (#1163) — the competing + // writer has to target the same value for the race to be real. + external_relpath: path.join('nas', '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); + expect(counts.get(path.join('nas', '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); @@ -194,7 +196,9 @@ describe('concurrent external imports (#1162)', () => { path: 'x/a.jpg', type: 'individual', source_origin: 'external', - external_relpath: path.join('individual', 'a.jpg'), + // Root-relative, as the route now writes it (#1163) — the competing + // writer has to target the same value for the race to be real. + external_relpath: path.join('nas', 'individual', 'a.jpg'), }; await runImport(eventId); diff --git a/backend/__tests__/integration/externalImportSecondFolder.test.js b/backend/__tests__/integration/externalImportSecondFolder.test.js new file mode 100644 index 00000000..181d7369 --- /dev/null +++ b/backend/__tests__/integration/externalImportSecondFolder.test.js @@ -0,0 +1,175 @@ +/** + * Importing a second folder must not move the photos already in the event (#1163). + * + * events.external_path is overwritten by every import, and external_relpath + * used to be stored relative to it — so a second import silently rebased every + * existing row onto the new folder. The reporter had 7547 of 8004 originals + * pointing at files that do not exist, and nothing said so: thumbnails are + * written to local storage during the import while the base path is still + * correct, so the grid carries on rendering. + * + * Driven through the real route and the real resolver, against a real + * directory tree — the failure is entirely about whether a file is where the + * app looks for it. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const express = require('express'); +const request = require('supertest'); + +describe('a second external import (#1163)', () => { + let tmpDir; let db; let app; let mediaRoot; let resolvePhotoFilePath; + + const touch = async (rel) => { + const full = path.join(mediaRoot, rel); + await fs.promises.mkdir(path.dirname(full), { recursive: true }); + await fs.promises.writeFile(full, 'not-a-real-jpeg'); + }; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-ext2nd-')); + mediaRoot = path.join(tmpDir, 'media'); + await fs.promises.mkdir(mediaRoot, { recursive: true }); + + 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 || 'ext2nd-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(), + })); + jest.doMock('sharp', () => () => ({ metadata: async () => ({ width: 100, height: 200 }) })); + jest.doMock('../../src/services/imageProcessor', () => ({ + generateThumbnail: jest.fn(async () => '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()); + ({ resolvePhotoFilePath } = require('../../src/services/photoResolver')); + + 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(); + await fs.promises.rm(mediaRoot, { recursive: true, force: true }); + await fs.promises.mkdir(mediaRoot, { recursive: true }); + const [e] = await db('events').insert({ + slug: `ext2nd-${Math.random().toString(36).slice(2, 8)}`, + event_type: 'wedding', + event_name: 'ext2nd', + event_date: '2026-01-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_link: `ext2nd-${Math.random()}`, + expires_at: new Date().toISOString(), + source_mode: 'reference', + }).returning('id'); + return typeof e === 'object' ? e.id : e; + } + + const runImport = (eventId, external_path) => request(app) + .post(`/api/admin/external-media/events/${eventId}/import-external`) + .send({ external_path, recursive: true }); + + /** Where the app would go looking for this photo's original, right now. */ + async function resolved(eventId, filename) { + const event = await db('events').where({ id: eventId }).first(); + const photo = await db('photos').where({ event_id: eventId, filename }).first(); + return resolvePhotoFilePath(event, photo); + } + + it('stores paths relative to the media root, not to the imported folder', async () => { + const eventId = await seedEvent(); + await touch('Trip/Leknes/old.jpg'); + + await runImport(eventId, 'Trip'); + + const photo = await db('photos').where({ event_id: eventId }).first(); + expect(photo.external_relpath).toBe(path.join('Trip', 'Leknes', 'old.jpg')); + }); + + it('leaves the first folder’s originals reachable after a second import', async () => { + const eventId = await seedEvent(); + await touch('Trip/Leknes/old.jpg'); + await touch('Trip/Sub/new.jpg'); + + await runImport(eventId, 'Trip'); + const before = await resolved(eventId, 'old.jpg'); + await runImport(eventId, 'Trip/Sub'); + const after = await resolved(eventId, 'old.jpg'); + + // The regression: `after` used to be /Trip/Sub/Leknes/old.jpg. + expect(after).toBe(before); + expect(fs.existsSync(after)).toBe(true); + }); + + it('every original in the event is still on disk afterwards', async () => { + const eventId = await seedEvent(); + await touch('Trip/Leknes/a.jpg'); + await touch('Trip/Leknes/b.jpg'); + await touch('Trip/Sub/c.jpg'); + + await runImport(eventId, 'Trip'); + await runImport(eventId, 'Trip/Sub'); + + const event = await db('events').where({ id: eventId }).first(); + const photos = await db('photos').where({ event_id: eventId }); + expect(photos).toHaveLength(3); + for (const photo of photos) { + expect(fs.existsSync(resolvePhotoFilePath(event, photo))).toBe(true); + } + }); + + it('does not re-insert a file the first import already took', async () => { + // The dedupe check compares stored paths, so it has to be comparing the + // same shape the insert writes. + const eventId = await seedEvent(); + await touch('Trip/Sub/c.jpg'); + + await runImport(eventId, 'Trip'); + const second = await runImport(eventId, 'Trip/Sub'); + + expect(second.body.imported).toBe(0); + expect(second.body.skipped).toBe(1); + expect(await db('photos').where({ event_id: eventId }).count('* as c').first()).toEqual({ c: 1 }); + }); + + it('resolves a subfolder that repeats its parent’s name', async () => { + // The old resolver stripped the relpath's first segment when it matched the + // base path's last one, which broke exactly this layout. + const eventId = await seedEvent(); + await touch('Trip/Trip/x.jpg'); + + await runImport(eventId, 'Trip'); + + const event = await db('events').where({ id: eventId }).first(); + const photo = await db('photos').where({ event_id: eventId }).first(); + expect(resolvePhotoFilePath(event, photo)).toBe(path.join(mediaRoot, 'Trip', 'Trip', 'x.jpg')); + }); +}); diff --git a/backend/__tests__/integration/externalRelpathFoldPg.test.js b/backend/__tests__/integration/externalRelpathFoldPg.test.js new file mode 100644 index 00000000..da6713d1 --- /dev/null +++ b/backend/__tests__/integration/externalRelpathFoldPg.test.js @@ -0,0 +1,121 @@ +/** + * PostgreSQL integration test for the external-path fold (#1163). + * + * Gated the same way as picpeakRestorePg: runs only when PICPEAK_PG_TEST_URL + * points at a throwaway Postgres DB, e.g. + * PICPEAK_PG_TEST_URL="postgres://picpeak:picpeak_secure_pass_2024@127.0.0.1:7102/picpeak_fold_test" \ + * npx jest __tests__/integration/externalRelpathFoldPg.test.js + * + * This exists because of a defect SQLite could not have caught. The two-pass + * rewrite parks each row on a temporary value, and that value was first written + * with a leading NUL. SQLite stores NUL in TEXT without complaint; Postgres + * rejects it outright ("invalid byte sequence for encoding UTF8"), so migration + * 187 would have rolled back on exactly the installs needing the repair — and + * only on the engine most of them run. + * + * The staging value is therefore an engine-level contract, not an + * implementation detail, and it is pinned here on the engine that constrains it. + */ + +const knex = require('knex'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const PG_URL = process.env.PICPEAK_PG_TEST_URL; +const maybe = PG_URL ? describe : describe.skip; + +maybe('external relpath fold on Postgres', () => { + let pgDb; let mediaRoot; let fold; + + const touch = async (rel, bytes) => { + const full = path.join(mediaRoot, rel); + await fs.promises.mkdir(path.dirname(full), { recursive: true }); + await fs.promises.writeFile(full, Buffer.alloc(bytes)); + return bytes; + }; + + beforeAll(async () => { + mediaRoot = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-foldpg-')); + process.env.EXTERNAL_MEDIA_ROOT = mediaRoot; + jest.resetModules(); + ({ foldExternalRelpaths: fold } = require('../../src/services/externalRelpathFold')); + + pgDb = knex({ client: 'pg', connection: PG_URL }); + }, 60000); + + afterAll(async () => { + if (pgDb) await pgDb.destroy(); + await fs.promises.rm(mediaRoot, { recursive: true, force: true }).catch(() => {}); + delete process.env.EXTERNAL_MEDIA_ROOT; + }); + + beforeEach(async () => { + await pgDb.raw('DROP TABLE IF EXISTS photos, events, app_settings CASCADE'); + await pgDb.schema.createTable('events', (t) => { + t.increments('id'); + t.text('external_path'); + }); + await pgDb.schema.createTable('photos', (t) => { + t.increments('id'); + t.integer('event_id'); + t.text('external_relpath'); + t.bigInteger('size_bytes'); + t.string('source_origin').defaultTo('managed'); + }); + await pgDb.schema.createTable('app_settings', (t) => { + t.increments('id'); + t.string('setting_key'); + t.text('setting_value'); + t.string('setting_type'); + t.string('updated_at'); + }); + await fs.promises.rm(mediaRoot, { recursive: true, force: true }); + await fs.promises.mkdir(mediaRoot, { recursive: true }); + }); + + const relpaths = async () => + (await pgDb('photos').orderBy('id').select('external_relpath')).map((r) => r.external_relpath); + + it('completes the two-pass repair that a NUL staging value would abort', async () => { + // The exact shape that forces staging: `photo.jpg` repairs up to + // `Trip/photo.jpg`, while the row already holding `Trip/photo.jpg` folds + // deeper. Every final value is distinct, but a final value equals another + // row's current one, so the rewrite has to park first. + const a = await touch('Trip/photo.jpg', 11); + const b = await touch('Trip/Sub/Trip/photo.jpg', 22); + await pgDb('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await pgDb('photos').insert([ + { event_id: 1, external_relpath: 'photo.jpg', size_bytes: a, source_origin: 'external' }, + { event_id: 1, external_relpath: 'Trip/photo.jpg', size_bytes: b, source_origin: 'external' }, + ]); + + await fold(pgDb); + + expect(await relpaths()).toEqual(['Trip/photo.jpg', 'Trip/Sub/Trip/photo.jpg']); + }); + + it('leaves no staging value behind', async () => { + await touch('Trip/a.jpg', 8); + await pgDb('events').insert({ id: 1, external_path: 'Trip' }); + await pgDb('photos').insert({ event_id: 1, external_relpath: 'a.jpg', size_bytes: 8, source_origin: 'external' }); + + await fold(pgDb); + + const rows = await relpaths(); + expect(rows).toEqual(['Trip/a.jpg']); + expect(rows.some((r) => r.includes('staging'))).toBe(false); + }); + + it('folds and marks in one transaction', async () => { + await touch('Trip/a.jpg', 8); + await pgDb('events').insert({ id: 1, external_path: 'Trip' }); + await pgDb('photos').insert({ event_id: 1, external_relpath: 'a.jpg', size_bytes: 8, source_origin: 'external' }); + + await fold(pgDb); + // Second run is a no-op: the marker committed with the rewrites. + await fold(pgDb); + + expect(await relpaths()).toEqual(['Trip/a.jpg']); + }); +}); diff --git a/backend/__tests__/integration/faceTransientSource.test.js b/backend/__tests__/integration/faceTransientSource.test.js index 503ef524..bae17639 100644 --- a/backend/__tests__/integration/faceTransientSource.test.js +++ b/backend/__tests__/integration/faceTransientSource.test.js @@ -72,7 +72,10 @@ async function seedExternalPhoto({ externalPath, relpath = 'individual/a.jpg' }) processing_status: 'complete', face_status: 'processing', source_origin: 'external', - external_relpath: relpath, + // Stored relative to EXTERNAL_MEDIA_ROOT (#1163), which is what the import + // route writes — `relpath` above is expressed relative to the event's + // folder only because that reads better at the call sites. + external_relpath: path.join(externalPath, relpath), }).returning('id'); return { eventId, photoId: typeof p === 'object' ? p.id : p }; } diff --git a/backend/__tests__/integration/regenerateThumbnailsScript.test.js b/backend/__tests__/integration/regenerateThumbnailsScript.test.js index 7e435568..a90b984b 100644 --- a/backend/__tests__/integration/regenerateThumbnailsScript.test.js +++ b/backend/__tests__/integration/regenerateThumbnailsScript.test.js @@ -31,8 +31,9 @@ describe('regenerate-thumbnails script (#1148)', () => { process.env.NODE_ENV = 'test'; process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'test.db'); process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); - // External sources are sandboxed under EXTERNAL_MEDIA_ROOT; event - // external_path is relative to it, exactly as on a real install. + // External sources are sandboxed under EXTERNAL_MEDIA_ROOT. Rows carry a + // path relative to that root (#1163), so the 'wedding/' prefix on each + // external_relpath below is the event folder, not decoration. process.env.EXTERNAL_MEDIA_ROOT = path.join(tmpDir, 'media'); externalRoot = path.join(process.env.EXTERNAL_MEDIA_ROOT, 'wedding'); @@ -72,7 +73,7 @@ describe('regenerate-thumbnails script (#1148)', () => { path: 'regen-script-event/shot.jpg', type: 'individual', source_origin: 'external', - external_relpath: 'shot.jpg', + external_relpath: 'wedding/shot.jpg', uploaded_at: new Date().toISOString(), }).returning('id'); externalPhotoId = typeof p === 'object' ? p.id : p; @@ -85,7 +86,7 @@ describe('regenerate-thumbnails script (#1148)', () => { media_type: 'video', mime_type: 'video/mp4', source_origin: 'external', - external_relpath: 'clip.mp4', + external_relpath: 'wedding/clip.mp4', uploaded_at: new Date().toISOString(), }).returning('id'); videoPhotoId = typeof v === 'object' ? v.id : v; @@ -108,7 +109,7 @@ describe('regenerate-thumbnails script (#1148)', () => { type: 'video', mime_type: 'video/mp4', source_origin: 'external', - external_relpath: 'watched.mp4', + external_relpath: 'wedding/watched.mp4', uploaded_at: new Date().toISOString(), }).returning('id'); watcherVideoId = typeof wv === 'object' ? wv.id : wv; @@ -125,7 +126,7 @@ describe('regenerate-thumbnails script (#1148)', () => { type: 'individual', thumbnail_path: 'thumbnails/thumb_ext_missing_repair.jpg', source_origin: 'external', - external_relpath: 'repair.jpg', + external_relpath: 'wedding/repair.jpg', uploaded_at: new Date().toISOString(), }).returning('id'); repairPhotoId = typeof rp === 'object' ? rp.id : rp; @@ -142,7 +143,7 @@ describe('regenerate-thumbnails script (#1148)', () => { path: 'regen-script-event/vanishing.jpg', type: 'individual', source_origin: 'external', - external_relpath: 'vanishing.jpg', + external_relpath: 'wedding/vanishing.jpg', uploaded_at: new Date().toISOString(), }).returning('id'); vanishingPhotoId = typeof vp === 'object' ? vp.id : vp; diff --git a/backend/__tests__/migrations/187_external_relpath_from_root.test.js b/backend/__tests__/migrations/187_external_relpath_from_root.test.js new file mode 100644 index 00000000..3ec35866 --- /dev/null +++ b/backend/__tests__/migrations/187_external_relpath_from_root.test.js @@ -0,0 +1,364 @@ +/** + * Folding the event's base path into every external row (#1163). + * + * Two things can go wrong and both are silent, which is why they are pinned + * here rather than left to review: folding a path that was ALREADY folded + * (every original moves), and "repairing" a healthy install because the media + * root happened to be unmounted when the migration ran (every original moves). + * + * The repair itself is driven against a real temp directory tree, because the + * whole mechanism is "is this file actually there" and a mocked fs would only + * be testing the mock. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +describe('migration 187 — external_relpath from the media root (#1163)', () => { + let knex; let tmpDir; let mediaRoot; let migration; + + /** Writes `bytes` bytes and returns the size, so fixtures can record it the + * way an import would have. */ + const touch = async (rel, bytes = 8) => { + const full = path.join(mediaRoot, rel); + await fs.promises.mkdir(path.dirname(full), { recursive: true }); + await fs.promises.writeFile(full, Buffer.alloc(bytes)); + return bytes; + }; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig187-')); + mediaRoot = path.join(tmpDir, 'media'); + await fs.promises.mkdir(mediaRoot, { recursive: true }); + process.env.EXTERNAL_MEDIA_ROOT = mediaRoot; + + // The service caches the root on first call, so it must not have been + // resolved before EXTERNAL_MEDIA_ROOT was set above. + jest.resetModules(); + migration = require('../../migrations/core/187_external_relpath_from_root'); + + 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(() => {}); + delete process.env.EXTERNAL_MEDIA_ROOT; + }); + + beforeEach(async () => { + await knex.schema.dropTableIfExists('photos'); + await knex.schema.dropTableIfExists('events'); + await knex.schema.dropTableIfExists('app_settings'); + await knex.schema.createTable('events', (t) => { + t.increments('id').primary(); + t.string('external_path'); + }); + await knex.schema.createTable('photos', (t) => { + t.increments('id').primary(); + t.integer('event_id'); + t.string('external_relpath'); + t.integer('size_bytes'); + t.string('source_origin').defaultTo('managed'); + }); + await knex.schema.createTable('app_settings', (t) => { + t.increments('id').primary(); + t.string('setting_key'); + t.text('setting_value'); + t.string('setting_type'); + t.string('updated_at'); + }); + await fs.promises.rm(mediaRoot, { recursive: true, force: true }); + await fs.promises.mkdir(mediaRoot, { recursive: true }); + }); + + const relpaths = async () => + (await knex('photos').orderBy('id', 'asc').select('external_relpath')) + .map((r) => r.external_relpath); + + it('folds the base path into every row of a healthy event', async () => { + await touch('Trip/Leknes/a.jpg'); + await touch('Trip/Leknes/b.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'Leknes/a.jpg', source_origin: 'external' }, + { event_id: 1, external_relpath: 'Leknes/b.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Leknes/a.jpg', 'Trip/Leknes/b.jpg']); + }); + + it('repairs rows an earlier import had rebased', async () => { + // The reported shape: a parent imported first, a child imported second, so + // events.external_path is the child and the parent's rows resolve into a + // path that does not exist. + const oldSize = await touch('Trip/Leknes/old.jpg', 11); // from the first import + const newSize = await touch('Trip/Sub/new.jpg', 22); // from the second + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'Leknes/old.jpg', size_bytes: oldSize, source_origin: 'external' }, + { event_id: 1, external_relpath: 'new.jpg', size_bytes: newSize, source_origin: 'external' }, + ]); + + await migration.up(knex); + + // The old row is placed where the file actually is; the new one keeps + // resolving exactly where it resolved before. + expect(await relpaths()).toEqual(['Trip/Leknes/old.jpg', 'Trip/Sub/new.jpg']); + }); + + it('refuses an ancestor whose file is a different size', async () => { + // The dangerous case: the row's own file was simply deleted, and an + // UNRELATED file one directory up happens to share its name. Adopting it + // would make downloads serve the wrong original — worse than a dead link. + await touch('Trip/photo.jpg', 999); + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert({ + event_id: 1, external_relpath: 'photo.jpg', size_bytes: 42, source_origin: 'external', + }); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Sub/photo.jpg']); + }); + + it('refuses an ancestor when the row records no size to check against', async () => { + // Nothing to verify provenance with, so the row stays where it resolves + // today rather than adopting a same-named stranger. + await touch('Trip/photo.jpg', 100); + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert({ + event_id: 1, external_relpath: 'photo.jpg', size_bytes: null, source_origin: 'external', + }); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Sub/photo.jpg']); + }); + + it('leaves nothing folded when a rewrite fails partway', async () => { + // Without a transaction, a crash between the first event's UPDATE and the + // marker leaves mixed formats behind — and the next run folds the already + // folded rows a second time, putting every original one directory deeper. + await touch('A/one.jpg'); + await touch('B/two.jpg'); + await knex('events').insert([ + { id: 1, external_path: 'A' }, + { id: 2, external_path: 'B' }, + ]); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'one.jpg', source_origin: 'external' }, + { event_id: 2, external_relpath: 'two.jpg', source_origin: 'external' }, + ]); + // app_settings is written last, in the same transaction as the rewrites. + await knex.schema.dropTableIfExists('app_settings_backup'); + await knex.raw('CREATE TRIGGER fail_marker BEFORE INSERT ON app_settings ' + + "BEGIN SELECT RAISE(ABORT, 'boom'); END"); + + await expect(migration.up(knex)).rejects.toThrow(/boom/); + + await knex.raw('DROP TRIGGER fail_marker'); + // Every row still base-relative, and no marker — so a retry is correct. + expect(await relpaths()).toEqual(['one.jpg', 'two.jpg']); + expect(await knex('app_settings').where('setting_key', 'external_relpath_root_relative').first()) + .toBeUndefined(); + }); + + it('removes the losing row when two paths converge, instead of stranding it', async () => { + // Trip/Sub/c.jpg imported once via `Trip` (as `Sub/c.jpg`) and once via + // `Trip/Sub` (as `c.jpg`). Both fold to the same path. Skipping the loser + // would leave it base-relative under a root-only resolver — pointing at + // /c.jpg — with the marker claiming the conversion is complete. + const size = await touch('Trip/Sub/c.jpg', 33); + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'Sub/c.jpg', size_bytes: size, source_origin: 'external' }, + { event_id: 1, external_relpath: 'c.jpg', size_bytes: size, source_origin: 'external' }, + ]); + + await migration.up(knex); + + const rows = await knex('photos').select('external_relpath'); + expect(rows).toHaveLength(1); + expect(rows[0].external_relpath).toBe('Trip/Sub/c.jpg'); + }); + + it('survives a final path that equals another row\'s current path', async () => { + // `photo.jpg` repairs to `Trip/photo.jpg` while the row already holding + // `Trip/photo.jpg` folds to `Trip/Sub/Trip/photo.jpg`. Every FINAL value is + // distinct, but a one-pass rewrite collides halfway through — and on + // Postgres that 23505 is misread by the migration runner as "already + // applied", leaving everything unconverted. + const a = await touch('Trip/photo.jpg', 11); + const b = await touch('Trip/Sub/Trip/photo.jpg', 22); + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'photo.jpg', size_bytes: a, source_origin: 'external' }, + { event_id: 1, external_relpath: 'Trip/photo.jpg', size_bytes: b, source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/photo.jpg', 'Trip/Sub/Trip/photo.jpg']); + }); + + it('does not re-prefix a row inserted while the probe was running', async () => { + // Phase 1 runs outside the transaction and can take minutes on a cold + // mount. An import finishing in that window writes an already + // root-relative row, which a `where event_id` bulk update would prefix a + // second time with the stale base. + await touch('Trip/a.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }); + + const { foldExternalRelpaths } = require('../../src/services/externalRelpathFold'); + const realStat = fs.promises.stat; + let injected = false; + jest.spyOn(fs.promises, 'access').mockImplementation(async (...args) => { + if (!injected) { + injected = true; + await knex('photos').insert({ + event_id: 1, external_relpath: 'Trip/late.jpg', source_origin: 'external', + }); + } + return realStat(args[0]).then(() => undefined); + }); + + await foldExternalRelpaths(knex); + fs.promises.access.mockRestore(); + + expect((await relpaths()).sort()).toEqual(['Trip/a.jpg', 'Trip/late.jpg']); + }); + + it('leaves a row it cannot place resolving where it resolves today', async () => { + // Never guess below current behaviour: a file that is genuinely gone must + // not have its path rewritten to some other file that happens to exist. + await touch('Trip/Sub/present.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'present.jpg', source_origin: 'external' }, + { event_id: 1, external_relpath: 'vanished.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Sub/present.jpg', 'Trip/Sub/vanished.jpg']); + }); + + it('folds without repairing when the media root is unmounted', async () => { + // An unmounted share leaves the mountpoint as an empty directory, so every + // file looks missing. Repairing off that signal would move every original + // on a perfectly healthy install. + await knex('events').insert({ id: 1, external_path: 'Trip/Sub' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'Leknes/a.jpg', source_origin: 'external' }, + ]); + // mediaRoot is empty — see beforeEach. + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Sub/Leknes/a.jpg']); + }); + + it('leaves managed rows alone', async () => { + await touch('Trip/a.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert([ + { event_id: 1, external_relpath: null, source_origin: 'managed' }, + { event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await relpaths()).toEqual([null, 'Trip/a.jpg']); + }); + + it('leaves an event with no base path alone — its rows are already root-relative', async () => { + await touch('a.jpg'); + await knex('events').insert({ id: 1, external_path: null }); + await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['a.jpg']); + }); + + it('folds each event with its own base', async () => { + await touch('A/one.jpg'); + await touch('B/two.jpg'); + await knex('events').insert([ + { id: 1, external_path: 'A' }, + { id: 2, external_path: 'B' }, + ]); + await knex('photos').insert([ + { event_id: 1, external_relpath: 'one.jpg', source_origin: 'external' }, + { event_id: 2, external_relpath: 'two.jpg', source_origin: 'external' }, + ]); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['A/one.jpg', 'B/two.jpg']); + }); + + it('tolerates a base path with stray slashes', async () => { + await touch('Trip/a.jpg'); + await knex('events').insert({ id: 1, external_path: '/Trip/' }); + await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/a.jpg']); + }); + + it('does not fold twice when run again', async () => { + // The failure this guards is total: every original on the install moves one + // directory deeper, and there is no undo. + await touch('Trip/a.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }); + + await migration.up(knex); + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/a.jpg']); + }); + + it('does not fold twice when the base repeats in the relpath', async () => { + // The inference this migration deliberately does NOT use: `Trip/x.jpg` + // under base `Trip` already "starts with the base", but has not been + // folded — it is a subfolder that shares its parent's name. + await touch('Trip/Trip/x.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert({ event_id: 1, external_relpath: 'Trip/x.jpg', source_origin: 'external' }); + + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/Trip/x.jpg']); + }); + + it('rollback does not clear the marker, so a re-run cannot double-fold', async () => { + await touch('Trip/a.jpg'); + await knex('events').insert({ id: 1, external_path: 'Trip' }); + await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' }); + + await migration.up(knex); + await migration.down(knex); + await migration.up(knex); + + expect(await relpaths()).toEqual(['Trip/a.jpg']); + }); + + 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/ensurePreviewImage.external.test.js b/backend/__tests__/services/ensurePreviewImage.external.test.js index 7252dd01..4813361a 100644 --- a/backend/__tests__/services/ensurePreviewImage.external.test.js +++ b/backend/__tests__/services/ensurePreviewImage.external.test.js @@ -91,15 +91,16 @@ describe('ensurePreviewImage — external/reference sources (#1078)', () => { it.each(['external', 'reference'])( 'generates a downscaled preview for a %s photo off the media mount', async (sourceOrigin) => { - const relpath = `${sourceOrigin}-shot.jpg`; - await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + const name = `${sourceOrigin}-shot.jpg`; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, name)); const photo = { id: sourceOrigin === 'external' ? 101 : 102, event_id: EVENT.id, source_origin: sourceOrigin, - external_relpath: relpath, - filename: relpath, + // Relative to the media ROOT, not to event.external_path (#1163). + external_relpath: path.join(EVENT.external_path, name), + filename: name, preview_path: null, }; @@ -107,7 +108,7 @@ describe('ensurePreviewImage — external/reference sources (#1078)', () => { // Per-photo basename so two events referencing the same NAS filename // can't clobber each other's preview. - expect(key).toBe(`previews/preview_ext${photo.id}_${relpath}`); + expect(key).toBe(`previews/preview_ext${photo.id}_${name}`); expect(await storage.exists(key)).toBe(true); const meta = await sharp(storage.resolveLocalPath(key)).metadata(); @@ -124,14 +125,14 @@ describe('ensurePreviewImage — external/reference sources (#1078)', () => { ); it('short-circuits on an existing valid preview instead of regenerating', async () => { - const relpath = 'already-previewed.jpg'; - await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + const name = 'already-previewed.jpg'; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, name)); const photo = { id: 103, event_id: EVENT.id, source_origin: 'external', - external_relpath: relpath, - filename: relpath, + external_relpath: path.join(EVENT.external_path, name), + filename: name, preview_path: null, }; @@ -179,19 +180,19 @@ describe('ensurePreviewImage — external/reference sources (#1078)', () => { // Pins why the /regenerate-previews caller must select source_origin: // an external row missing that column takes the managed path, where // resolvePhotoStorageKey yields null and generation is skipped. - const relpath = 'column-starved.jpg'; - await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, relpath)); + const name = 'column-starved.jpg'; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, name)); const starved = { id: 106, event_id: EVENT.id, - external_relpath: relpath, + external_relpath: path.join(EVENT.external_path, name), preview_path: null, }; await expect(imageProcessor.ensurePreviewImage(starved)).resolves.toBeNull(); await expect( - imageProcessor.ensurePreviewImage({ ...starved, source_origin: 'external', filename: relpath }) - ).resolves.toBe(`previews/preview_ext106_${relpath}`); + imageProcessor.ensurePreviewImage({ ...starved, source_origin: 'external', filename: name }) + ).resolves.toBe(`previews/preview_ext106_${name}`); }); it('still routes managed photos through the storage backend', async () => { diff --git a/backend/__tests__/services/photoResolver.test.js b/backend/__tests__/services/photoResolver.test.js index 0d0b0f79..161b0949 100644 --- a/backend/__tests__/services/photoResolver.test.js +++ b/backend/__tests__/services/photoResolver.test.js @@ -2,10 +2,10 @@ const path = require('path'); const mockPath = path; jest.mock('../../src/services/externalMediaService', () => ({ - resolveExternalPath: jest.fn((event, relPath) => mockPath.join('/mock/external', event.external_path || '', relPath || '')), + resolveExternalPhotoPath: jest.fn((photo) => mockPath.join('/mock/external', photo.external_relpath || '')), })); -const { resolveExternalPath } = require('../../src/services/externalMediaService'); +const { resolveExternalPhotoPath } = require('../../src/services/externalMediaService'); const { resolvePhotoFilePath } = require('../../src/services/photoResolver'); describe('resolvePhotoFilePath', () => { @@ -46,24 +46,34 @@ describe('resolvePhotoFilePath', () => { expect(result).toBe(path.join(backendRoot, 'storage', 'events/active', 'wedding-party', 'hero.jpg')); }); - it('delegates external photos to external media resolver', () => { + it('resolves an external photo from the media root, ignoring the event', () => { const event = { slug: 'fashion-show', source_mode: 'reference', external_path: 'picsum-demo' }; - const photo = { source_origin: 'external', external_relpath: 'individual/look-01.jpg' }; + const photo = { source_origin: 'external', external_relpath: 'picsum-demo/individual/look-01.jpg' }; const result = resolvePhotoFilePath(event, photo); - expect(resolveExternalPath).toHaveBeenCalledWith(event, 'individual/look-01.jpg'); + expect(resolveExternalPhotoPath).toHaveBeenCalledWith(photo); expect(result).toBe(path.join('/mock/external', 'picsum-demo', 'individual', 'look-01.jpg')); }); - it('deduplicates folder names when event external path already ends with segment', () => { - const event = { slug: 'fashion-show', source_mode: 'reference', external_path: 'picsum-demo/individual' }; - const photo = { source_origin: 'external', external_relpath: 'individual/look-02.jpg' }; + it('does not move a photo when the event is repointed at another folder (#1163)', () => { + // The regression. Both events below hold the SAME row; only + // events.external_path differs, which is what a second import overwrites. + const photo = { source_origin: 'external', external_relpath: 'Trip/Leknes/_DSC0818.JPG' }; + const before = { slug: 'trip', source_mode: 'reference', external_path: 'Trip' }; + const after = { slug: 'trip', source_mode: 'reference', external_path: 'Trip/Subfolder' }; - const result = resolvePhotoFilePath(event, photo); + expect(resolvePhotoFilePath(before, photo)).toBe(resolvePhotoFilePath(after, photo)); + }); - expect(resolveExternalPath).toHaveBeenCalledWith(event, 'look-02.jpg'); - expect(result).toBe(path.join('/mock/external', 'picsum-demo/individual', 'look-02.jpg')); + it('keeps a first segment that repeats the base path', () => { + // The old duplicate-leaf-segment normalisation stripped this, which is + // corruption once the relpath is root-relative: `Trip/Trip/x.jpg` is a real + // layout, and the file is not at `Trip/x.jpg`. + const event = { slug: 'trip', source_mode: 'reference', external_path: 'Trip' }; + const photo = { source_origin: 'external', external_relpath: 'Trip/Trip/x.jpg' }; + + expect(resolvePhotoFilePath(event, photo)).toBe(path.join('/mock/external', 'Trip', 'Trip', 'x.jpg')); }); it('falls back to managed storage when external metadata is missing', () => { @@ -72,7 +82,7 @@ describe('resolvePhotoFilePath', () => { const result = resolvePhotoFilePath(event, photo); - expect(resolveExternalPath).not.toHaveBeenCalled(); + expect(resolveExternalPhotoPath).not.toHaveBeenCalled(); expect(result).toBe(path.join(backendRoot, 'storage', 'events/active', 'fashion-show', 'new-upload.jpg')); }); diff --git a/backend/migrations/core/187_external_relpath_from_root.js b/backend/migrations/core/187_external_relpath_from_root.js new file mode 100644 index 00000000..9e590e9f --- /dev/null +++ b/backend/migrations/core/187_external_relpath_from_root.js @@ -0,0 +1,52 @@ +/** + * Migration 187: external_relpath becomes relative to EXTERNAL_MEDIA_ROOT (#1163). + * + * It used to be relative to events.external_path, which every import + * overwrites — so importing a second folder into an event rebased every photo + * already in it onto the new folder. Nothing errored. Thumbnails are written to + * local storage during the import while the base path is still correct, so the + * grid kept rendering and only the things that need the ORIGINAL broke: preview + * generation, the lightbox, downloads. The reporter had 7547 of 8004 rows + * resolving to files that do not exist, and spent a while chasing it as a CPU + * problem. + * + * The work — including the on-disk repair of events that have already been + * rebased, and why it refuses to guess below current behaviour — lives in + * services/externalRelpathFold.js, because a .picpeak restore has to run it + * too: knex_migrations is excluded from the archive, so a pre-#1163 backup + * lands base-relative rows on an instance that has already migrated. + */ + +const { foldExternalRelpaths } = require('../../src/services/externalRelpathFold'); + +exports.up = async function(knex) { + try { + await foldExternalRelpaths(knex, (msg) => console.log(`187_external_relpath_from_root: ${msg}`)); + } catch (err) { + // Re-thrown WITHOUT the driver's error code. run-migrations-safe.js treats + // 23505 / 42P07 / 42701 / 42710 as "schema already exists" and marks the + // migration applied (run-migrations-safe.js:138) — so a unique-violation + // rolling this fold back would be recorded as a success, leaving every + // external path in the old format under a resolver that reads them + // differently, with nothing to trigger a retry. + throw new Error( + `187_external_relpath_from_root failed and was rolled back: ${err.message}. ` + + 'External photo paths are unchanged; resolve the cause and re-run the migration.' + ); + } +}; + +/** + * Irreversible by design, and a deliberate no-op rather than a partial undo. + * + * The base each row was folded with is not recorded anywhere: + * events.external_path holds whatever the LAST import set, which for a + * repaired row is the wrong answer and is exactly what broke these installs. + * Stripping it back off would re-break them. + * + * The idempotency marker stays for the same reason — clearing it would let + * up() run a second time and fold every path twice. + */ +exports.down = async function() { + console.log('187_external_relpath_from_root: rollback is a no-op (see header)'); +}; diff --git a/backend/src/routes/adminBackup.js b/backend/src/routes/adminBackup.js index 2954f8e8..e8ac7e42 100644 --- a/backend/src/routes/adminBackup.js +++ b/backend/src/routes/adminBackup.js @@ -249,6 +249,11 @@ router.post('/picpeak/import', adminAuth, requirePermission('backup.restore'), p filesRestored: result.filesRestored, usesExternalMedia: result.usesExternalMedia, crossEngine: result.crossEngine, + // False when the pre-#1163 external-path conversion failed. The rows and + // files are in place, but no external original resolves until it is + // retried — the UI must say so rather than showing a plain success. + externalPathsConverted: result.externalPathsConverted !== false, + externalPathError: result.externalPathError || null, sessionInvalidated: true, }); } catch (error) { diff --git a/backend/src/routes/adminExternalMedia.js b/backend/src/routes/adminExternalMedia.js index 98a60be2..c78bd037 100644 --- a/backend/src/routes/adminExternalMedia.js +++ b/backend/src/routes/adminExternalMedia.js @@ -81,6 +81,15 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. if (!event) return res.status(404).json({ error: 'Event not found' }); const baseAbs = resolveExternalPath({ external_path }, ''); + + // What gets STORED on the row (#1163). `f.rel` stays relative to the + // imported folder because the type inference below reads its first segment + // ('individual' / 'collages'); external_relpath is written relative to + // EXTERNAL_MEDIA_ROOT so the row does not depend on a column this very + // handler is about to overwrite. + const basePrefix = String(external_path).replace(/^\/+|\/+$/g, ''); + const toRootRelative = (rel) => (basePrefix ? path.join(basePrefix, rel) : rel); + // Collect files const files = recursive ? await walkDir(baseAbs, baseAbs) : (await fs.readdir(baseAbs, { withFileTypes: true })) .filter(e => e.isFile()) @@ -131,15 +140,12 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. // :52) and is NOT NULL defaulting to 'managed', so flipping source_mode // does not touch them. // - // It does NOT isolate existing EXTERNAL rows — resolveExternalPath prefixes - // every one of them with event.external_path - // (externalMediaService.js:97-102), so importing folder B into an event - // that already references folder A rebases the A rows onto B. That is - // pre-existing: the update always did this, just after the loop instead of - // before it, so moving it changes when the window opens and not whether it - // exists. The underlying limitation is that an event carries a single - // external base path, which makes importing a second folder into it - // incoherent either way — worth its own fix, not this one. + // Safe for existing EXTERNAL rows too, as of #1163. It was not: relpaths + // were stored relative to external_path, so overwriting the column here + // rebased every row already in the event onto the new folder — quietly, + // because their thumbnails were already on local disk and the grid carried + // on rendering. Rows now carry a root-relative path and this write cannot + // reach them. await db('events').where('id', eventId).update({ source_mode: 'reference', external_path }); let imported = 0; @@ -175,6 +181,8 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. if (segs[0] === map.collages) type = 'collage'; if (segs[0] === map.individual) type = 'individual'; + const relFromRoot = toRootRelative(f.rel); + try { // 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 @@ -184,7 +192,7 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. // 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 }) + .where({ event_id: eventId, external_relpath: relFromRoot }) .first(); if (exists) { skipped++; continue; } const stats = await fs.stat(f.full); @@ -214,7 +222,7 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. width, height, source_origin: 'external', - external_relpath: f.rel + external_relpath: relFromRoot }) .returning('id'); } catch (insertErr) { diff --git a/backend/src/services/externalMediaService.js b/backend/src/services/externalMediaService.js index f696ae7e..0ad9048f 100644 --- a/backend/src/services/externalMediaService.js +++ b/backend/src/services/externalMediaService.js @@ -94,6 +94,14 @@ async function list(relativePath = '') { return { path: relFromRoot, entries, canNavigateUp }; } +/** + * A path under an EVENT's configured base directory. + * + * Still the right resolver for anything that means "the folder this event was + * imported from" — the import walk, and the mount-health probe in + * faceProcessor. It is NOT how a photo's original is found any more: see + * resolveExternalPhotoPath (#1163). + */ function resolveExternalPath(event, relpath) { const root = getExternalMediaRoot(); const base = event?.external_path ? path.join(event.external_path) : ''; @@ -101,9 +109,29 @@ function resolveExternalPath(event, relpath) { return safePathJoin(root, combined); } +/** + * A photo's original, from the root (#1163). + * + * photos.external_relpath used to be stored relative to events.external_path, + * which made a row's meaning depend on a column on another table that every + * import overwrites. Importing a second folder into an event therefore rebased + * every row already in it — silently, because thumbnails are written to local + * storage during the import and the grid keeps rendering. One reporter had + * 7547 of 8004 rows resolving to files that did not exist. + * + * Root-relative makes a row self-describing: nothing an admin does to the event + * afterwards can move an already-imported photo. Migration 187 rewrote the + * existing rows. + */ +function resolveExternalPhotoPath(photo) { + const root = getExternalMediaRoot(); + return safePathJoin(root, photo?.external_relpath || ''); +} + module.exports = { getExternalMediaRoot, isUnderRoot, list, resolveExternalPath, + resolveExternalPhotoPath, }; diff --git a/backend/src/services/externalRelpathFold.js b/backend/src/services/externalRelpathFold.js new file mode 100644 index 00000000..12fb43e3 --- /dev/null +++ b/backend/src/services/externalRelpathFold.js @@ -0,0 +1,304 @@ +/** + * Fold each event's base path into its external photo rows (#1163). + * + * photos.external_relpath used to be stored relative to events.external_path — + * a column every import overwrites — so importing a second folder into an + * event rebased every photo already in it. Root-relative paths make a row + * self-describing. + * + * This lives in a service rather than inside migration 187 because it has two + * callers. The migration is one. The other is a .picpeak restore: knex_migrations + * is excluded from the archive, so restoring a pre-#1163 backup onto an + * already-migrated instance drops base-relative rows into a schema that no + * longer folds them, and every original in the restored library becomes + * unreachable with nothing logged. + * + * REPAIR, and its limits. For a healthy event the correct new value is just + * `external_path + relpath` — that is what the app resolves today, so folding + * it in changes nothing and can break nothing. For an event that has ALREADY + * been rebased, that same rule would bake the broken path in permanently, so + * where the current resolution does not exist on disk this walks up the base + * path looking for an ancestor under which the file IS there (the shape the + * bug produces: a parent imported first, a child second). A row it cannot + * place is left resolving exactly where it resolves today — preserving current + * behaviour is the floor, never guess below it. + * + * Existence alone is NOT enough to accept an ancestor. A row whose file an + * admin simply deleted would otherwise adopt any same-named file further up — + * base `Trip/Sub`, relpath `photo.jpg`, an unrelated `Trip/photo.jpg` — and + * downloads would then serve the WRONG original, which is worse than a broken + * link. So an ancestor candidate must also match photos.size_bytes, recorded + * by the import from the very file the row describes. Rows carrying no size + * are never repaired from an ancestor. + * + * ATOMICITY. Probing is read-only and runs first; every rewrite and the marker + * are then committed in ONE transaction. Split across commits, a process + * killed mid-fold would leave converted and unconverted rows behind with no + * marker, and the next run would fold the converted ones a second time — + * putting every original one directory deeper, permanently. + * + * The probe is skipped entirely when the media root is unreachable or empty: + * an unmounted share makes every file look missing, and "repairing" off that + * signal would move every original on a healthy install. When it does run it + * is one access() per photo, base path first, so a healthy install pays one + * stat per row and then a single UPDATE per event. + * + * Idempotency is recorded explicitly in app_settings rather than inferred from + * the data. The tempting inference — "does the relpath already start with the + * base path?" — is wrong for any event with a subfolder named after its parent + * (base 'Trip', row 'Trip/x.jpg'), and being wrong there corrupts a path in an + * operation that has no undo. + */ + +const path = require('path'); +const fsp = require('fs').promises; +const { deleteDuplicatePhotos } = require('./externalPhotoDedupe'); + +const MARKER = 'external_relpath_root_relative'; +// Per-row parking value for the two-pass rewrite below. +// +// NOT a NUL-prefixed string, which is what this was first written as: Postgres +// rejects U+0000 in a `text` column outright ("invalid byte sequence for +// encoding UTF8"), so the rewrite would abort on exactly the installs that +// need the two-pass repair — and only on Postgres, which SQLite-only tests +// cannot see. The prefix below is ordinary text, cannot collide with a real +// relative path (no import writes a leading dot-segment like this), and stays +// obviously wrong if a crash ever leaves one behind. +const STAGING_PREFIX = '.picpeak-fold-staging/'; +const CHUNK = 400; // SQLite caps a statement at 999 bound parameters. +const chunk = (arr) => { + const out = []; + for (let i = 0; i < arr.length; i += CHUNK) out.push(arr.slice(i, i + CHUNK)); + return out; +}; + +function normalizeBase(externalPath) { + return String(externalPath || '').replace(/\\/g, '/').replace(/^\/+|\/+$/g, ''); +} + +/** Every prefix of `base`, longest first, then '' (the root itself). */ +function ancestorPrefixes(base) { + const segs = base.split('/').filter(Boolean); + const out = []; + for (let i = segs.length; i > 0; i--) out.push(segs.slice(0, i).join('/')); + out.push(''); + return out; +} + +async function exists(p) { + try { await fsp.access(p); return true; } catch { return false; } +} + +/** + * Is `p` plausibly the file this row was imported from? + * + * Size is the provenance signal available without re-reading every original: + * photos.size_bytes was written by the import from the file the row describes. + * Returns false when it cannot be verified, so an unverifiable candidate is + * never adopted from somewhere the row did not previously point. + */ +async function fileMatchesSize(p, expectedSize) { + if (expectedSize == null || Number(expectedSize) <= 0) return false; + try { + const stats = await fsp.stat(p); + return stats.isFile() && stats.size === Number(expectedSize); + } catch { + return false; + } +} + +/** + * Joined without the safePathJoin the app serves through: this is reading, and + * a stored path that escapes the root is damage worth detecting rather than + * throwing on. + */ +const under = (root, ...parts) => path.join(root, ...parts.filter(Boolean)); + +async function rootUsable(root) { + if (!root) return false; + try { + // An unmounted NFS/SMB share usually leaves the mountpoint behind as an + // ordinary empty directory, so readdir succeeds on storage that is gone. + return (await fsp.readdir(root)).length > 0; + } catch { + return false; + } +} + +/** + * @param {import('knex')} knex a knex instance, or a transaction from a caller + * that is already inside one (the restore). + * @param {(msg: string) => void} [log] + * @returns {Promise<{skipped?: string, folded?: number, repaired?: number, stranded?: number, collided?: number}>} + */ +async function foldExternalRelpaths(knex, log = () => {}) { + if (!(await knex.schema.hasTable('photos'))) return { skipped: 'no photos table' }; + if (!(await knex.schema.hasColumn('photos', 'external_relpath'))) return { skipped: 'no external_relpath column' }; + if (!(await knex.schema.hasTable('events'))) return { skipped: 'no events table' }; + if (!(await knex.schema.hasTable('app_settings'))) return { skipped: 'no app_settings table' }; + + if (await knex('app_settings').where('setting_key', MARKER).first()) { + return { skipped: 'already folded' }; + } + + const events = await knex('events').select('id', 'external_path'); + const byId = new Map(events.map((e) => [e.id, normalizeBase(e.external_path)])); + const eventRows = await knex('photos').whereNotNull('external_relpath').distinct('event_id'); + + let root = null; + try { + root = require('./externalMediaService').getExternalMediaRoot(); + } catch (e) { + log(`media root unavailable (${e.message}) — folding without on-disk repair`); + } + const canProbe = await rootUsable(root); + if (!canProbe) log('media root unreachable or empty — folding base paths in without on-disk repair'); + + // ---- Phase 1: decide, writing nothing. ------------------------------- + // Read-only, so the transaction below stays short. Probing a cold NAS can + // take minutes and holding a write transaction open for that would block the + // app for the duration. + let folded = 0; let repaired = 0; let stranded = 0; let collided = 0; + const plan = []; + + for (const { event_id: eventId } of eventRows) { + // No base path means the rows are already relative to the root. + const base = byId.get(eventId); + if (!base) continue; + + const rows = await knex('photos') + .where('event_id', eventId) + .whereNotNull('external_relpath') + .select('id', 'external_relpath', 'size_bytes'); + + // Deciding health from a SAMPLE was the tempting shortcut and it is not + // safe: a rebased event whose first few rows happen to come from the most + // recent import reads as healthy, and every older row is baked in wrong. + const prefixes = ancestorPrefixes(base); + const placements = []; + let allUnderBase = true; + + for (const row of rows) { + if (!canProbe) { placements.push([row, base]); continue; } + + let chosen = null; + // The current base first, on existence alone — nothing is inferred + // there, it is where the row already resolves. + if (await exists(under(root, base, row.external_relpath))) { + chosen = base; + } else { + // Anywhere else has to prove itself: the name AND the size the import + // recorded. Without that a row whose file an admin deleted would adopt + // an unrelated same-named file one directory up, and downloads would + // serve the wrong original. + for (const prefix of prefixes) { + if (prefix === base) continue; + if (await fileMatchesSize(under(root, prefix, row.external_relpath), row.size_bytes)) { + chosen = prefix; + break; + } + } + } + + if (chosen === null) { chosen = base; stranded++; } + if (chosen !== base) allUnderBase = false; + placements.push([row, chosen]); + } + + if (allUnderBase) { + folded += rows.length; + plan.push({ eventId, base, bulk: true, rows: [], ids: rows.map((r) => r.id) }); + continue; + } + + // Two rows can now target the same path — the same file imported under two + // different bases really is one file. Resolved HERE rather than by letting + // the write fail: a caught write error cannot tell a genuine duplicate from + // a lock or I/O fault, and continuing past one would certify a partial + // conversion by writing the marker anyway. + // + // The loser is DELETED, not skipped. Skipping leaves it holding a + // base-relative path that the root-only resolver then reads as + // `/` — permanently pointing at the wrong place, or + // nowhere, with the marker saying the conversion is done. And it is a + // duplicate by construction: two rows that resolve to one file is exactly + // what migration 186 removes, so it goes through the same helper, which + // reparents the feedback and marks and reconciles the face clusters. + const claimed = new Map(); + const resolved = []; + const losers = new Map(); + for (const [row, chosen] of placements) { + const next = chosen ? `${chosen}/${row.external_relpath}` : row.external_relpath; + const winner = claimed.get(next); + if (winner != null) { losers.set(row.id, winner); collided++; continue; } + claimed.set(next, row.id); + if (chosen === base) folded++; else repaired++; + resolved.push([row.id, next]); + } + plan.push({ eventId, base, bulk: false, rows: resolved, losers }); + } + + // ---- Phase 2: write, all or nothing. --------------------------------- + // The marker rides in the same transaction as the rewrites, so there is no + // window where some rows are folded, the marker is absent, and a second run + // folds them again — which would put every original one directory deeper, + // permanently. + const apply = async (trx) => { + for (const step of plan) { + if (step.bulk) { + // BY ID, not by event. Phase 1 runs outside the transaction and can + // take minutes probing a cold mount; an import completing in that + // window inserts an already root-relative row, and `where event_id` + // would prefix it a second time with the stale base. + for (const ids of chunk(step.ids)) { + await trx('photos') + .whereIn('id', ids) + .whereNotNull('external_relpath') + .update({ external_relpath: trx.raw('? || external_relpath', [`${step.base}/`]) }); + } + continue; + } + // Losers first: while they still hold their old path, the survivor has + // not taken the value they would collide with. + if (step.losers && step.losers.size) { + await deleteDuplicatePhotos(trx, step.losers); + } + + // Two passes, through a per-row temporary value. The FINAL values are + // all distinct, but a final value can equal another row's CURRENT one — + // `photo.jpg` repairing to `Trip/photo.jpg` while the existing + // `Trip/photo.jpg` is still waiting to fold — so a single pass violates + // migration 186's unique index halfway through. And on Postgres that + // surfaces as 23505, which run-migrations-safe.js mistakes for "schema + // already exists" and records the migration as applied after the + // rollback, leaving every path unconverted with no retry. + for (const [id] of step.rows) { + await trx('photos').where('id', id).update({ external_relpath: `${STAGING_PREFIX}${id}` }); + } + for (const [id, next] of step.rows) { + // No catch: collisions were resolved above, so anything failing here is + // a real fault and must roll the whole fold back rather than leave a + // half-converted table certified by the marker. + await trx('photos').where('id', id).update({ external_relpath: next }); + } + } + + await trx('app_settings').insert({ + setting_key: MARKER, + setting_value: JSON.stringify(true), + setting_type: 'system', + updated_at: new Date().toISOString(), + }); + }; + + // A caller already inside a transaction (the restore) passes its trx in as + // `knex`; opening a nested one would deadlock SQLite. + if (knex.isTransaction) await apply(knex); + else await knex.transaction(apply); + + log(`${folded} folded, ${repaired} repaired, ${stranded} left unresolved, ${collided} skipped as duplicates`); + return { folded, repaired, stranded, collided }; +} + + +module.exports = { foldExternalRelpaths, MARKER }; diff --git a/backend/src/services/photoResolver.js b/backend/src/services/photoResolver.js index 91f0a76f..53c17f2c 100644 --- a/backend/src/services/photoResolver.js +++ b/backend/src/services/photoResolver.js @@ -1,5 +1,5 @@ const path = require('path'); -const { resolveExternalPath } = require('./externalMediaService'); +const { resolveExternalPhotoPath } = require('./externalMediaService'); const { safePathJoin } = require('../utils/fileSecurityUtils'); const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); @@ -40,7 +40,7 @@ function resolvePhotoStorageKey(event, photo) { /** * Resolve absolute photo file path based on event + photo origin * Managed: storage/events/active + photo.path (legacy variants supported) - * External reference: EXTERNAL_MEDIA_ROOT + event.external_path + photo.external_relpath + * External reference: EXTERNAL_MEDIA_ROOT + photo.external_relpath */ function resolvePhotoFilePath(event, photo) { if (!event || !photo) throw new Error('resolvePhotoFilePath requires event and photo'); @@ -61,20 +61,18 @@ function resolvePhotoFilePath(event, photo) { } throw new Error('Missing external_relpath for external photo'); } - // Normalize duplicate leaf segments (e.g., event.external_path ends with 'individual' - // and external_relpath starts with 'individual/') to avoid double segment like - // '/external-media/.../individual/individual/file.jpg' - let rel = photo.external_relpath; - try { - const lastSeg = path.basename(event.external_path || ''); - const firstSeg = rel.split(path.sep)[0]; - if (lastSeg && firstSeg && lastSeg === firstSeg) { - rel = rel.split(path.sep).slice(1).join(path.sep) || ''; - } - } catch (_) { - // ignore normalization errors - } - return resolveExternalPath(event, rel); + // external_relpath is relative to EXTERNAL_MEDIA_ROOT, so the event is not + // consulted at all (#1163). It used to be relative to event.external_path, + // which meant importing a second folder into an event silently moved every + // photo already in it. + // + // The duplicate-leaf-segment normalisation that used to live here went with + // it. It stripped the first segment of the relpath when it matched the last + // segment of event.external_path — a guess that papered over the + // double-prefixing this class of bug produced, and one that actively + // corrupts a root-relative path whose first segment legitimately repeats + // (external_path 'Trip', relpath 'Trip/x.jpg'). + return resolveExternalPhotoPath(photo); } const storagePath = getStoragePath(); diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index 32f27c97..3dfd1843 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -541,6 +541,38 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) { const filesRestored = await restoreFiles(staging); + // External media paths (#1163). knex_migrations is excluded from the + // archive, so migration 187 does not re-run after a restore — a pre-#1163 + // backup would otherwise drop base-relative rows onto an instance that + // resolves them from the media root, and every original in the restored + // library would be unreachable with nothing logged. The fold is a no-op + // when the restored app_settings already carries the marker. + // + // BEFORE the face requeue below, and for the same reason that requeue sits + // after restoreFiles: the worker is live throughout. Queued first, it can + // claim an external row while the row is still base-relative, resolve it + // against the wrong path with the root-only resolver, and mark the photo + // failed — a state only an explicit Re-scan clears, and one the fold does + // not undo. Probing a cold mount takes long enough for that to be likely + // rather than theoretical. + let externalPathsConverted = true; + let externalPathError = null; + try { + const { foldExternalRelpaths } = require('./externalRelpathFold'); + const result = await foldExternalRelpaths(db, (msg) => logger.info(`picpeakImport: external paths — ${msg}`)); + if (result.folded || result.repaired) { + logger.info(`picpeakImport: folded ${result.folded} external path(s), repaired ${result.repaired}`); + } + } catch (err) { + // NOT swallowed as a footnote. The fold is transactional, so a failure + // leaves every external path in the pre-#1163 format while the running + // resolver reads from the media root — meaning every original in the + // restored library is unreachable. Reporting that as a clean restore + // sends the admin away believing it worked. + externalPathsConverted = false; + externalPathError = err.message; + logger.error(`picpeakImport: external path conversion FAILED — originals will not resolve until this is retried: ${err.message}`); + } // Face data (#1074): queue ONLY once the files are on disk. The archive // carries no face rows and the export blanked photos.face_status, but the // event toggles come across enabled, so the "enable" transition that @@ -551,23 +583,44 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) { // instance's files or marks photos failed for originals that are not // there yet — and nothing re-queues them afterwards. try { - const requeued = await db('photos') - .whereIn('event_id', db('events').select('id').where('face_recognition_enabled', true)) - .update({ - face_status: 'pending', face_count: null, face_started_at: null, face_error: null, - }); - if (requeued > 0) { - logger.info(`picpeakImport: queued ${requeued} photo(s) for face detection after import`); + if (!externalPathsConverted) { + // Queueing now would hand the live worker rows whose paths the + // resolver cannot follow, and it would mark them 'failed' — a state + // only an explicit Re-scan clears. Leave them unqueued; the operator + // re-runs the conversion and then re-scans. + logger.warn('picpeakImport: skipping face requeue — external paths are unconverted'); + } else { + const requeued = await db('photos') + .whereIn('event_id', db('events').select('id').where('face_recognition_enabled', true)) + .update({ + face_status: 'pending', face_count: null, face_started_at: null, face_error: null, + }); + if (requeued > 0) { + logger.info(`picpeakImport: queued ${requeued} photo(s) for face detection after import`); + } } } catch (err) { logger.debug?.(`picpeakImport: face requeue skipped: ${err.message}`); } + const usesExternalMedia = await detectExternalMedia(); logger.info( `[picpeak-import] restored ${tables.length} tables, ${filesRestored} files (externalMedia=${usesExternalMedia}, crossEngine=${crossEngine})` ); - return { restored: true, tables: tables.length, filesRestored, usesExternalMedia, crossEngine, manifest }; + return { + restored: true, + tables: tables.length, + filesRestored, + usesExternalMedia, + crossEngine, + manifest, + // Surfaced so the caller can warn rather than report an unqualified + // success: the rows and files are in place, but the external originals + // do not resolve until the conversion is retried (#1163). + externalPathsConverted, + externalPathError, + }; } finally { await fsp.rm(staging, { recursive: true, force: true }).catch(() => {}); } diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index ebba330e..692730a6 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -209,18 +209,20 @@ class RestoreService { // Step 6: Perform the actual restore based on type let restoreResult; + // Set by the full/database branches; acted on after step 7c so the + // schema — and any data conversion those migrations perform — is in + // place before the live face worker can claim a row. + let needsFaceRequeue = false; + let migrationsApplied = true; switch (options.restoreType) { case 'full': restoreResult = await this.performFullRestore(localBackupPath, manifest, options); - // After the FILES too — see requeueFaceScans on why the ordering - // matters with a live worker. - await this.requeueFaceScans(); + // Deferred to after step 7c — see the requeue there. + needsFaceRequeue = true; break; case 'database': restoreResult = await this.performDatabaseRestore(localBackupPath, manifest, options); - // Database-only restore: the existing files stay in place, so there - // is nothing to race. - await this.requeueFaceScans(); + needsFaceRequeue = true; break; case 'files': restoreResult = await this.performFilesRestore(localBackupPath, manifest, options); @@ -326,11 +328,31 @@ class RestoreService { } this.log('info', 'Post-restore migrations applied'); } catch (migErr) { + // Also gates the face requeue below: a pre-#1163 backup whose + // migration 187 did not run still holds event-relative external + // paths, and queueing those hands the live worker rows it will + // resolve from the media root and mark 'failed' — a state the later + // retry does not clear. + migrationsApplied = false; this.log('warn', 'Post-restore migrate:safe failed — restore data is in place but the schema may lag the running image. ' + `A container restart will retry via wait-for-db.sh. Error: ${migErr.message}`); } + // Faces last (#1163). This used to run in step 6, before the migrations + // above. On a backup predating migration 187 that meant queueing rows + // whose external_relpath was still relative to events.external_path + // while the running code resolves from the media root — so the live + // worker resolved them against the wrong path and marked them 'failed', + // a state the later fold does not clear and only an explicit Re-scan + // does. The files are already in place by step 6, so deferring costs + // nothing and closes that window. + if (needsFaceRequeue && migrationsApplied) { + await this.requeueFaceScans(); + } else if (needsFaceRequeue) { + this.log('warn', 'Skipping face requeue — post-restore migrations did not complete, so photo paths may be unconverted'); + } + // Step 8: Clean up temporary files if (localBackupPath !== options.source) { await fs.unlink(localBackupPath).catch(err => diff --git a/frontend/src/components/admin/PicpeakBackupCard.tsx b/frontend/src/components/admin/PicpeakBackupCard.tsx index d6bdc6a6..35793427 100644 --- a/frontend/src/components/admin/PicpeakBackupCard.tsx +++ b/frontend/src/components/admin/PicpeakBackupCard.tsx @@ -18,6 +18,11 @@ interface RestoreResult { usesExternalMedia: boolean; crossEngine?: boolean; sessionInvalidated?: boolean; + // False when the pre-#1163 external-path conversion failed. Rows and files + // are in place, but no external original resolves until it is retried — so + // this must not be presented as a plain success. + externalPathsConverted?: boolean; + externalPathError?: string | null; } // ── Download half (Dashboard) ──────────────────────────────────────────────── @@ -115,7 +120,12 @@ export const PicpeakRestoreCard: React.FC = () => { const res = await api.post('/admin/backup/picpeak/import', fd); setResult(res.data); setPendingFile(null); - toast.success(t('backup.picpeak.restoreDone', 'Backup restored.')); + if (res.data?.externalPathsConverted === false) { + toast.error(t('backup.picpeak.externalPathsFailed', + 'Backup restored, but external photo paths could not be converted — those originals will not load until this is retried.')); + } else { + toast.success(t('backup.picpeak.restoreDone', 'Backup restored.')); + } // The restore rewrote admin_users and the backend revoked our session // (ids may have shifted). Send the operator to a fresh login rather than // letting the now-stale token resolve to a different restored account. @@ -169,6 +179,16 @@ export const PicpeakRestoreCard: React.FC = () => { {t('backup.picpeak.crossEngineNote', 'Cross-engine restore: a SQLite backup was converted onto this PostgreSQL instance.')}

)} + {result.externalPathsConverted === false && ( +

+ + + {t('backup.picpeak.externalPathsFailedDetail', + 'External photo paths were not converted, so every referenced original is currently unreachable. Re-run the restore or the pending migrations once the cause is resolved.')} + {result.externalPathError ? ` (${result.externalPathError})` : ''} + +

+ )} {result.usesExternalMedia && (