diff --git a/backend/__tests__/integration/maintenanceJobState.test.js b/backend/__tests__/integration/maintenanceJobState.test.js index 5c17f937..9268ecc9 100644 --- a/backend/__tests__/integration/maintenanceJobState.test.js +++ b/backend/__tests__/integration/maintenanceJobState.test.js @@ -71,7 +71,10 @@ describe('maintenance job state (#1181)', () => { test('the migration seeds a row for each job', async () => { const names = await db('maintenance_jobs').pluck('job_name'); - expect(names.sort()).toEqual(['photo_capture_date_backfill', 'photo_dimension_repair']); + // 190 seeds the orientation backfill alongside 189's two (#1198). + expect(names.sort()).toEqual([ + 'photo_capture_date_backfill', 'photo_dimension_repair', 'photo_orientation_backfill', + ]); }); test('a second claim is refused while the first is alive', async () => { diff --git a/backend/__tests__/integration/orientationBackfill.test.js b/backend/__tests__/integration/orientationBackfill.test.js new file mode 100644 index 00000000..0c304a3a --- /dev/null +++ b/backend/__tests__/integration/orientationBackfill.test.js @@ -0,0 +1,361 @@ +/** + * Backfilling orientation for a library that predates #1185 (#1198). + * + * The orientation fix corrected the generators and every ingest path, but left + * existing rows describing the raw sensor order. Those rows end up worse than + * untouched ones: before the fix a rotated photo was consistently wrong — a + * sideways image in a matching tile — and afterwards the thumbnail is right + * while the stored aspect ratio is not. + * + * A first attempt at this was reverted from #1194 after review. These tests + * pin the five things that went wrong with it: + * + * 1. requeueing faces without clearing the cached preview, so the rescan + * re-read unrotated pixels; + * 2. reading originals in a way that cannot see S3 or RAW; + * 3. deciding "did this change" from a dimension delta, which never fires + * for orientations 2, 3 and 4; + * 4. walking archived events whose originals no longer exist; + * 5. writing dimensions and invalidation non-atomically. + */ + +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const express = require('express'); +const request = require('supertest'); +const sharp = require('sharp'); + +describe('orientation backfill (#1198)', () => { + let tmpDir; let db; let app; let storageRoot; + + const status = () => request(app).get('/api/admin/photos/repair-orientation/status'); + const run = () => request(app).post('/api/admin/photos/repair-orientation'); + const settle = async () => { + for (let i = 0; i < 100; i++) { + await new Promise((r) => setTimeout(r, 50)); + const s = await status(); + if (!s.body.isRunning) return s; + } + throw new Error('backfill did not settle'); + }; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-orientbf-')); + 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.JWT_SECRET = process.env.JWT_SECRET || 'orientbf-secret'; + + jest.resetModules(); + jest.doMock('../../src/middleware/auth', () => ({ + adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); }, + })); + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + })); + jest.doMock('../../src/utils/logger', () => ({ + debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), + })); + + ({ db } = await require('./helpers/crmDb').bootCrmDb()); + // bootCrmDb owns STORAGE_PATH; fixtures must live where the app resolves. + storageRoot = process.env.STORAGE_PATH; + await fs.promises.mkdir(path.join(storageRoot, 'events/active/orientbf'), { recursive: true }); + + app = express(); + app.use(express.json()); + app.use('/api/admin/photos', require('../../src/routes/adminPhotoDimensions')); + }, 180000); + + afterAll(async () => { + if (db) await db.destroy?.(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + async function seed({ + orientation, storedWidth, storedHeight, faceStatus = null, + previewPath = 'previews/prev_orientbf.jpg', archived = false, filename = 'p.jpg', + thumbnailPath = 'thumbnails/thumb_orientbf.jpg', heroPath = 'heroes/hero_orientbf.jpg', + watermarkPath = 'watermarks/wm_orientbf.jpg', checkedAt = null, + }) { + await db('photos').del(); + await db('events').del(); + const [e] = await db('events').insert({ + slug: 'orientbf', event_type: 'wedding', event_name: 'orientbf', event_date: '2026-01-01', + host_email: 'h@example.com', admin_email: 'a@example.com', password_hash: 'x', + share_link: `orientbf-${Math.random()}`, expires_at: new Date().toISOString(), + is_archived: archived, + }).returning('id'); + const eventId = typeof e === 'object' ? e.id : e; + + const img = sharp({ create: { width: 400, height: 200, channels: 3, background: { r: 7, g: 7, b: 7 } } }); + await (orientation ? img.withMetadata({ orientation }) : img) + .jpeg().toFile(path.join(storageRoot, 'events/active/orientbf', filename)); + + const [p] = await db('photos').insert({ + event_id: eventId, filename, path: `orientbf/${filename}`, type: 'individual', + width: storedWidth, height: storedHeight, face_status: faceStatus, + preview_path: previewPath, thumbnail_path: thumbnailPath, hero_path: heroPath, + watermark_path: watermarkPath, orientation_checked_at: checkedAt, + uploaded_at: new Date().toISOString(), + }).returning('id'); + return { eventId, photoId: typeof p === 'object' ? p.id : p }; + } + + it('corrects a row whose dimensions are transposed', async () => { + // The case the dimension repair can never reach: both values present, + // just in the raw sensor order. + const { photoId } = await seed({ orientation: 6, storedWidth: 400, storedHeight: 200 }); + + expect((await run()).body.count).toBe(1); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.width).toBe(200); + expect(row.height).toBe(400); + expect(done.body.lastResult.corrected).toBe(1); + }); + + it('clears the cached preview before requeueing, not after', async () => { + // The reverted attempt's own-goal: ensurePreviewImage hands back a cached + // preview whenever it is still a valid image, so a rescan against the + // pre-fix preview produced boxes in the old coordinate system and scaled + // them by the corrected dimensions. + const { photoId } = await seed({ + orientation: 6, storedWidth: 400, storedHeight: 200, faceStatus: 'done', + }); + + await run(); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.preview_path).toBeNull(); + expect(row.face_status).toBe('pending'); + expect(done.body.lastResult.requeuedFaces).toBe(1); + }); + + it('clears the thumbnail and hero too, not just the preview', async () => { + // The miss that mattered most: ensureThumbnail and ensureHeroImage return + // their cached file whenever it is merely VALID, and a pre-fix sideways + // thumbnail is perfectly valid. Clearing only the preview fixed the face + // data and left the gallery rendering the old sideways image inside a + // newly-corrected portrait tile. + const { photoId } = await seed({ orientation: 6, storedWidth: 400, storedHeight: 200 }); + + await run(); + await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.preview_path).toBeNull(); + expect(row.thumbnail_path).toBeNull(); + expect(row.hero_path).toBeNull(); + }); + + it('leaves the renditions of an untransformed photo alone', async () => { + // Nothing moved, so nothing cached is stale — clearing them would make a + // routine run regenerate the whole library for no reason. + const { photoId } = await seed({ orientation: null, storedWidth: 400, storedHeight: 200 }); + + await run(); + await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.thumbnail_path).toBe('thumbnails/thumb_orientbf.jpg'); + expect(row.hero_path).toBe('heroes/hero_orientbf.jpg'); + expect(row.preview_path).toBe('previews/prev_orientbf.jpg'); + }); + + it('does not touch a row whose file was replaced while it was reading', async () => { + // replacePhoto swaps a new file under an existing row and rewrites + // path/filename (reachable from replace_by_name). The writes are fenced on + // the identity that was measured, so a replacement that lands mid-run is + // left entirely alone rather than being given the previous file's + // dimensions and having its fresh renditions cleared. + const { photoId } = await seed({ + orientation: 6, storedWidth: 400, storedHeight: 200, faceStatus: 'done', + }); + + const res = await run(); + expect(res.body.count).toBe(1); + // Simulate the replacement landing before the loop writes. + await db('photos').where({ id: photoId }) + .update({ path: 'orientbf/replaced.jpg', filename: 'replaced.jpg' }); + await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.width).toBe(400); // untouched + expect(row.face_status).toBe('done'); // not requeued + expect(row.thumbnail_path).toBe('thumbnails/thumb_orientbf.jpg'); + }); + + it('is idempotent — a second run finds nothing left to do', async () => { + // The trigger is the EXIF tag on the ORIGINAL, which correcting a photo + // never changes. Without a marker every re-run would throw away the + // renditions it had just regenerated and requeue every completed face + // scan — on a face-enabled install, re-detecting the whole library. + await seed({ orientation: 6, storedWidth: 400, storedHeight: 200, faceStatus: 'done' }); + + expect((await run()).body.count).toBe(1); + const first = await settle(); + expect(first.body.lastResult.corrected).toBe(1); + + expect((await run()).body.count).toBe(0); + }); + + it('force revisits rows it has already checked', async () => { + await seed({ + orientation: 6, storedWidth: 200, storedHeight: 400, + checkedAt: new Date().toISOString(), + }); + + expect((await run()).body.count).toBe(0); + const forced = await request(app).post('/api/admin/photos/repair-orientation').send({ force: true }); + expect(forced.body.count).toBe(1); + await settle(); + }); + + it('clears the watermarked rendition, which is what a guest actually sees', async () => { + // gallery.js serves watermark_path ahead of the original when branding + // watermarking is on. + const { photoId } = await seed({ orientation: 6, storedWidth: 400, storedHeight: 200 }); + + await run(); + await settle(); + + expect((await db('photos').where({ id: photoId }).first()).watermark_path).toBeNull(); + }); + + it('does not report stale tiers after a clean run', async () => { + // storage.stat() RESOLVES with null for a missing key rather than + // rejecting, so counting "the promise settled" marked every deleted and + // never-created tier as a survivor and told the operator to re-run. + await seed({ orientation: 6, storedWidth: 400, storedHeight: 200 }); + + await run(); + const done = await settle(); + + expect(done.body.lastResult.staleTiers).toBe(0); + }); + + it('requeues faces when only the dimensions were wrong', async () => { + // No rotation involved: boxes are scaled by photo.width at read time, so + // any change to the stored dimensions invalidates them. + const { photoId } = await seed({ + orientation: null, storedWidth: 999, storedHeight: 111, faceStatus: 'done', + }); + + await run(); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.width).toBe(400); + expect(row.face_status).toBe('pending'); + expect(done.body.lastResult.requeuedFaces).toBe(1); + }); + + it('requeues an orientation that moves pixels without moving dimensions', async () => { + // Orientation 3 is a 180° turn: every pixel moves, width and height do + // not. A dimension-delta check sees nothing and skips exactly this row. + const { photoId } = await seed({ + orientation: 3, storedWidth: 400, storedHeight: 200, faceStatus: 'done', + }); + + await run(); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.width).toBe(400); // unchanged, correctly + expect(row.face_status).toBe('pending'); + expect(row.preview_path).toBeNull(); + expect(done.body.lastResult.requeuedFaces).toBe(1); + expect(done.body.lastResult.corrected).toBe(0); + }); + + it('leaves a post-fix import alone, renditions and all', async () => { + // A 5-8 rotation changes the dimensions, so a tagged photo whose stored + // dimensions are already oriented must have been ingested after #1185. + // Re-clearing its renditions would delete valid files and rescan a + // completed face detection for nothing. + const { photoId } = await seed({ + orientation: 6, storedWidth: 200, storedHeight: 400, faceStatus: 'done', + }); + + await run(); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.thumbnail_path).toBe('thumbnails/thumb_orientbf.jpg'); + expect(row.face_status).toBe('done'); + expect(done.body.lastResult).toMatchObject({ corrected: 0, requeuedFaces: 0 }); + // ...and it is marked, so it is not re-read next time either. + expect(row.orientation_checked_at).toBeTruthy(); + }); + + it('still invalidates a 180-degree rotation, which carries no such evidence', async () => { + // Orientation 3 leaves the dimensions identical whether or not it has been + // processed, so there is nothing to infer from and it must be done once. + const { photoId } = await seed({ + orientation: 3, storedWidth: 400, storedHeight: 200, faceStatus: 'done', + }); + + await run(); + await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.thumbnail_path).toBeNull(); + expect(row.face_status).toBe('pending'); + }); + + it('leaves an untagged photo completely alone', async () => { + const { photoId } = await seed({ + orientation: null, storedWidth: 400, storedHeight: 200, faceStatus: 'done', + }); + + await run(); + const done = await settle(); + + const row = await db('photos').where({ id: photoId }).first(); + expect(row.width).toBe(400); + expect(row.face_status).toBe('done'); + expect(row.preview_path).toBe('previews/prev_orientbf.jpg'); + expect(done.body.lastResult).toMatchObject({ corrected: 0, requeuedFaces: 0, failed: 0 }); + }); + + it('does not start face scanning on an install that never enabled it', async () => { + const { photoId } = await seed({ + orientation: 6, storedWidth: 400, storedHeight: 200, faceStatus: null, + }); + + await run(); + const done = await settle(); + + expect((await db('photos').where({ id: photoId }).first()).face_status).toBeNull(); + expect(done.body.lastResult.requeuedFaces).toBe(0); + // ...but the dimensions are still corrected. + expect(done.body.lastResult.corrected).toBe(1); + }); + + it('skips archived events, whose originals were deleted on archive', async () => { + await seed({ orientation: 6, storedWidth: 400, storedHeight: 200, archived: true }); + + const res = await run(); + expect(res.body.count).toBe(0); + }); + + it('refuses a second run while one is in flight', async () => { + // Shares the maintenance-lease plumbing, on its own job row so it neither + // blocks nor is blocked by the dimension repair. + const jobs = require('../../src/services/maintenanceJobState'); + await seed({ orientation: 6, storedWidth: 400, storedHeight: 200 }); + + const claim = await jobs.claim(jobs.JOB_ORIENTATION_BACKFILL); + expect(claim).toEqual(expect.any(String)); + + expect((await run()).status).toBe(409); + + // The dimension repair is a different job and is unaffected. + expect((await request(app).post('/api/admin/photos/repair-dimensions')).status).toBe(200); + await jobs.release(jobs.JOB_ORIENTATION_BACKFILL, claim); + }); +}); diff --git a/backend/migrations/core/190_orientation_backfill_job.js b/backend/migrations/core/190_orientation_backfill_job.js new file mode 100644 index 00000000..e831586f --- /dev/null +++ b/backend/migrations/core/190_orientation_backfill_job.js @@ -0,0 +1,43 @@ +/** + * Migration 190: a maintenance-job row for the orientation backfill (#1198). + * + * The backfill is its own job rather than a mode of the dimension repair. They + * look similar — both walk photos and write width/height — but they are not + * the same operation and must not share a lease: + * + * - the dimension repair FILLS rows that have none, and touches nothing else; + * - this one RECOMPUTES rows that already have dimensions and, where the EXIF + * transform means the stored pixels have moved, invalidates the derived + * images and face data that were generated against the old orientation. + * + * Sharing one row would mean an operator filling in missing dimensions blocks + * a colleague correcting a rotated library, and the two would report into the + * same lastResult with different shapes. + * + * Seeded here for the same reason as 189: the claim is a plain conditional + * UPDATE, and a row that has to be created on demand puts an insert race + * behind the very thing that exists to prevent races. + */ + +const JOB_NAME = 'photo_orientation_backfill'; + +exports.up = async function (knex) { + if (!(await knex.schema.hasTable('maintenance_jobs'))) { + // 189 creates it. If it is missing the install has not reached that + // migration yet, and 189 seeds its own rows when it runs. + console.log('190: maintenance_jobs missing, skipping seed'); + return; + } + + const existing = await knex('maintenance_jobs').where({ job_name: JOB_NAME }).first(); + if (!existing) { + await knex('maintenance_jobs').insert({ job_name: JOB_NAME, is_running: false }); + console.log(`190: seeded job row ${JOB_NAME}`); + } +}; + +exports.down = async function (knex) { + if (await knex.schema.hasTable('maintenance_jobs')) { + await knex('maintenance_jobs').where({ job_name: JOB_NAME }).del(); + } +}; diff --git a/backend/migrations/core/191_photo_orientation_checked.js b/backend/migrations/core/191_photo_orientation_checked.js new file mode 100644 index 00000000..961647ce --- /dev/null +++ b/backend/migrations/core/191_photo_orientation_checked.js @@ -0,0 +1,48 @@ +/** + * Migration 191: remember which photos the orientation backfill has already + * looked at (#1198). + * + * Without this the job is not idempotent, and the way it fails is expensive. + * Its trigger is the EXIF orientation tag on the ORIGINAL, which the backfill + * never changes — correcting the derived data does not untag the source. So on + * a second run every orientation-tagged photo still reads as "transformed", + * gets its freshly-regenerated thumbnail, preview and hero thrown away again, + * and has a completed face scan requeued. On an install with face detection on, + * running the job twice means re-detecting the whole library for nothing. + * + * The same applies to photos imported AFTER #1185, which are already correct + * but still carry their tag. + * + * A timestamp rather than a boolean so a future fix to the orientation + * handling can re-open the rows it needs by comparing against its own release + * date, instead of needing another column. + */ + +exports.up = async function (knex) { + if (!(await knex.schema.hasColumn('photos', 'orientation_checked_at'))) { + await knex.schema.alterTable('photos', (table) => { + table.timestamp('orientation_checked_at').nullable(); + }); + console.log('191: added photos.orientation_checked_at'); + } + + // Outside the column guard, for the same reason as 185: a run that died + // between the two statements would leave the column present and the index + // missing, and the re-run would skip both. The backfill's candidate query + // filters on this column across the whole photos table, so it wants one. + await knex.raw( + 'CREATE INDEX IF NOT EXISTS photos_orientation_checked_idx ' + + 'ON photos (orientation_checked_at)' + ); +}; + +exports.down = async function (knex) { + // Index first and unconditionally: SQLite rebuilds the table on dropColumn, + // and an index over the dropped column makes that rebuild fail. + await knex.raw('DROP INDEX IF EXISTS photos_orientation_checked_idx'); + if (await knex.schema.hasColumn('photos', 'orientation_checked_at')) { + await knex.schema.alterTable('photos', (table) => { + table.dropColumn('orientation_checked_at'); + }); + } +}; diff --git a/backend/src/routes/adminPhotoDimensions.js b/backend/src/routes/adminPhotoDimensions.js index ee8e47a9..9dcfcafc 100644 --- a/backend/src/routes/adminPhotoDimensions.js +++ b/backend/src/routes/adminPhotoDimensions.js @@ -19,7 +19,7 @@ const maintenanceJobs = require('../services/maintenanceJobState'); // Two separate rows, for the same reason the two objects were separate: the // jobs walk the same photos but read different things out of them, and one // running must not block or report for the other. -const { JOB_DIMENSION_REPAIR, JOB_CAPTURE_DATE_BACKFILL } = maintenanceJobs; +const { JOB_DIMENSION_REPAIR, JOB_CAPTURE_DATE_BACKFILL, JOB_ORIENTATION_BACKFILL } = maintenanceJobs; const { HEARTBEAT_INTERVAL_MS } = maintenanceJobs; @@ -519,4 +519,347 @@ router.get('/repair-capture-dates/status', adminAuth, requirePermission('system. } }); +/** + * Backfill orientation for a library that predates #1185 (#1198). + * + * The orientation fix corrected the generators and every ingest path, but did + * nothing for photos already in the database. Those rows are worse off than + * untouched ones in one specific way: before the fix a rotated photo was + * CONSISTENTLY wrong — a sideways image in a tile shaped to match. Afterwards + * the regenerated thumbnail is correct while photos.width/height still + * describe the raw sensor order, so masonry and justified size a portrait + * photo with a landscape ratio. + * + * The dimension repair above cannot reach them: it only selects rows with a + * NULL dimension, and an affected row has both — just transposed. + * + * Its own job rather than a mode of that one, because it does strictly more: + * where the EXIF transform means the pixels have moved, the derived images and + * face data generated against the old orientation are no longer valid and have + * to be invalidated with the write. + */ +router.post('/repair-orientation', adminAuth, requirePermission('system.manage'), async (req, res) => { + try { + // The trigger is the EXIF tag on the original, and correcting a photo does + // not untag it — so without a marker every re-run would throw away the + // renditions it just regenerated and requeue every completed face scan. + // `force` is the escape hatch for an interrupted run, or for a future fix + // that needs to revisit rows this one already cleared. + const force = req.body?.force === true || req.query?.force === 'true'; + + const token = await maintenanceJobs.claim(JOB_ORIENTATION_BACKFILL); + if (!token) { + return res.status(409).json({ error: 'Orientation backfill is already running' }); + } + const lease = startLeaseKeeper(JOB_ORIENTATION_BACKFILL, token); + + let photos; + try { + photos = await db('photos') + .join('events', 'photos.event_id', 'events.id') + .where(function () { + this.where('photos.media_type', '!=', 'video').orWhereNull('photos.media_type'); + }) + .where(function () { + this.where('photos.type', '!=', 'video').orWhereNull('photos.type'); + }) + .where(function () { + this.whereNull('photos.mime_type').orWhere('photos.mime_type', 'not like', 'video/%'); + }) + // Archiving deletes the originals and keeps the rows, so every archived + // photo would fail its read and add nothing but noise to a run that + // already walks the whole library. + .where(function () { + this.where('events.is_archived', false).orWhereNull('events.is_archived'); + }) + .modify((q) => { + if (!force) q.whereNull('photos.orientation_checked_at'); + }) + .select( + 'photos.id', 'photos.path', 'photos.filename', + 'photos.source_origin', 'photos.external_relpath', 'photos.event_id', + 'photos.width', 'photos.height', 'photos.face_status', + // Every rendition the invalidation below deletes. Selecting only + // preview_path left thumbnail_path and hero_path undefined, so their + // database pointers were cleared while the objects stayed in storage. + 'photos.preview_path', 'photos.thumbnail_path', 'photos.hero_path', + 'photos.watermark_path', + 'events.source_mode', 'events.external_path', 'events.slug' + ); + } catch (err) { + lease.stop(); + await maintenanceJobs.release(JOB_ORIENTATION_BACKFILL, token); + throw err; + } + + if (photos.length === 0) { + lease.stop(); + await maintenanceJobs.release(JOB_ORIENTATION_BACKFILL, token); + return res.json({ message: 'No photos to check', count: 0 }); + } + + res.json({ message: `Checking orientation for ${photos.length} photos`, count: photos.length }); + + setImmediate(async () => { + const sharp = require('sharp'); + const { + orientedDimensions, hasOrientationTransform, withLocalCopy, withProcessableImage, + deletePreviewTiers, deleteThumbnailTiers, previewTierKeys, thumbnailTierKeys, + } = require('../services/imageProcessor'); + const { getStorage } = require('../services/storage'); + const { resolvePhotoStorageKey } = require('../services/photoResolver'); + + // Fenced on the identity that was measured, not just the id: replacePhoto + // — reachable from the replace_by_name upload path (adminPhotos.js) — + // swaps a new file under an existing row and rewrites path/filename, so a + // replacement landing mid-run would otherwise be given the previous + // file's dimensions and have its fresh renditions cleared. + const fenceOf = (photo) => ({ id: photo.id, path: photo.path, filename: photo.filename }); + const nowIso = () => new Date().toISOString(); + + let checked = 0; + let corrected = 0; + let requeuedFaces = 0; + let staleTiers = 0; + let errorCount = 0; + let lostClaim = false; + + try { + for (const photo of photos) { + if (lease.lost()) { lostClaim = true; break; } + + try { + const event = { + source_mode: photo.source_mode, + external_path: photo.external_path, + slug: photo.slug, + }; + const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; + + // Read the metadata the same way every other maintenance path + // does. The dimension repair reads with resolvePhotoFilePath and + // plain sharp, which means it does nothing at all on an S3 install + // and rejects RAW/DNG — this job walks the WHOLE library, so both + // of those stop being edge cases. + let metadata; + if (isExternal) { + const fullPath = resolvePhotoFilePath(event, photo); + await fs.access(fullPath); + const proc = await withProcessableImage(fullPath, photo.filename); + try { + metadata = await sharp(proc.path).metadata(); + } finally { + await proc.cleanup(); + } + } else { + const sourceKey = resolvePhotoStorageKey(event, photo); + metadata = await withLocalCopy(sourceKey, async (localPath) => { + await fs.access(localPath); + const proc = await withProcessableImage(localPath, photo.filename); + try { + return await sharp(proc.path).metadata(); + } finally { + await proc.cleanup(); + } + }); + } + + checked++; + + const dims = orientedDimensions(metadata); + if (!dims.width || !dims.height) continue; + + const dimsWrong = photo.width !== dims.width || photo.height !== dims.height; + // Face boxes live in ORIGINAL pixel space and are scaled by + // photo.width at read time, so ANY change to the stored dimensions + // invalidates them — not only one caused by rotation. + // A 5-8 rotation changes the dimensions, so a tagged photo whose + // stored dimensions are ALREADY oriented must have been ingested + // after #1185 — its renditions are correct and re-clearing them + // would delete valid files and rescan faces for nothing. 2, 3 and + // 4 leave dimensions untouched, so they carry no such evidence and + // are invalidated once; the marker stops it happening twice. + // A square image is the exception within 5-8: the rotation is real + // but the dimensions come out identical, so it carries no evidence + // either and has to be treated like 2/3/4. + const swapsDimensions = metadata.orientation >= 5 && metadata.orientation <= 8 + && metadata.width !== metadata.height; + const cannotTell = hasOrientationTransform(metadata) && !swapsDimensions; + const facesStale = dimsWrong || cannotTell; + // NOT the same question as "did the dimensions change". Orientation + + if (!dimsWrong && !facesStale) { + // Nothing to change, but record that it was looked at so a + // re-run does not pay for reading it again. + await db('photos').where(fenceOf(photo)).update({ orientation_checked_at: nowIso() }); + continue; + } + + // Every write is fenced on the identity we measured, not just the + // id. replacePhoto — reachable from the replace_by_name upload path + // (adminPhotos.js) — swaps a new file under an existing row and + // rewrites path/filename, so a replacement landing while this job + // read the old original would otherwise get the previous file's + // dimensions written over it and its fresh renditions cleared. + // Matching path and filename too means the update affects no rows + // instead. + const fence = fenceOf(photo); + + // One transaction. If the dimension write commits and the + // invalidation does not, the row keeps stale face boxes AND a + // retry computes "already correct" — so nothing would ever fix it. + let dimsWritten = 0; + let invalidated = 0; + let markerPending = false; + await db.transaction(async (trx) => { + if (dimsWrong) { + dimsWritten = await trx('photos').where(fence) + .update({ width: dims.width, height: dims.height }); + } + + if (facesStale) { + // Every cached rendition, not just the preview. All three are + // regenerated lazily and all three short-circuit on a file + // that is merely VALID — and a pre-fix sideways thumbnail is + // perfectly valid. Clearing only the preview fixed the face + // data while leaving the gallery showing the old sideways + // image inside a newly-corrected portrait tile, which is worse + // than not having run at all. + // + // The preview specifically must go before faces are requeued: + // ensurePreviewImage would otherwise hand the rescan the old + // unrotated pixels, whose boxes then get scaled by the + // corrected dimensions. + invalidated = await trx('photos').where(fence).update({ + preview_path: null, + thumbnail_path: null, + hero_path: null, + // gallery.js serves watermark_path ahead of the original when + // branding watermarking is on, so a stale one is the single + // most visible rendition of all. + watermark_path: null, + }); + + // whereNotNull: face_status NULL means this photo was never + // scanned, and an install that never enabled the feature must + // not start scanning because of a dimension repair. + requeuedFaces += await trx('photos') + .where(fence) + .whereNotNull('face_status') + .whereNot('face_status', 'pending') + .update({ face_status: 'pending' }); + } + + // Same transaction as the work it records: a marker written + // separately could survive a rolled-back correction and hide the + // row from every future run. Withheld below if the storage + // cleanup then fails, so the row stays eligible for a retry. + markerPending = true; + }); + + // Outside the transaction on purpose: these delete files, and a + // storage error must not roll back a correct database write. A + // rolled-back write is silent corruption; a leftover object is not. + // + // For the canonical renditions a failed delete is harmless — their + // keys are deterministic, so regeneration overwrites in place. The + // responsive TIERS are the exception: ensurePreviewImageAtWidth + // treats storage.stat(key) as a cache hit, so a tier that survived + // deletion keeps being served unrotated and never regenerates. The + // tier helpers swallow their own errors, so the keys are re-checked + // and anything still standing is counted — a run that could not + // clear them should not report itself as clean. + // Only when a fenced write actually landed. If the file was + // replaced mid-run every update matched zero rows, and deleting + // now would destroy renditions belonging to the REPLACEMENT — + // watermarks especially, which are keyed by photo id and so alias + // straight onto the new file. + if (facesStale && invalidated > 0) { + const storage = getStorage(); + for (const key of [photo.preview_path, photo.thumbnail_path, photo.hero_path, photo.watermark_path]) { + if (key) await storage.delete(key).catch(() => {}); + } + await deletePreviewTiers(photo).catch(() => {}); + await deleteThumbnailTiers(photo).catch(() => {}); + + // stat() RESOLVES with null for a missing key rather than + // rejecting, so testing only that the promise settled counted + // every deleted — and every never-created — tier as a survivor, + // and told the operator to re-run after a perfectly clean pass. + // A rejection is a real storage error, which is also not proof + // the object is gone, so it counts as stuck. + const survivors = await Promise.all( + [...previewTierKeys(photo), ...thumbnailTierKeys(photo)] + .map((k) => storage.stat(k).then((st) => (st ? k : null)).catch(() => k)) + ); + const stuck = survivors.filter(Boolean).length; + if (stuck) { + staleTiers += stuck; + // Leave the row unmarked so the ordinary (non-force) re-run + // the UI recommends actually finds it again. Marking it here + // would make that advice impossible to follow. + markerPending = false; + logger.warn( + `Orientation backfill: ${stuck} tier(s) survived deletion for photo ${photo.id} — ` + + 'they will keep serving unrotated until storage is writable and this is re-run' + ); + } + } + + if (markerPending) { + await db('photos').where(fence).update({ orientation_checked_at: nowIso() }); + } + + // From the affected-row count, not the intent: if the fence + // rejected the write because the file was replaced mid-run, the + // photo was not corrected and must not be reported as such. + if (dimsWritten > 0) corrected++; + + if ((corrected + requeuedFaces) % 50 === 0 && (corrected + requeuedFaces) > 0) { + logger.info(`Orientation backfill progress: ${corrected} corrected...`); + } + } catch (error) { + logger.error(`Error backfilling orientation for photo ${photo.id}:`, error); + errorCount++; + } + } + + if (lostClaim) { + logger.warn(`Orientation backfill stopped: claim taken over after ${corrected} corrected`); + return; + } + await maintenanceJobs.release(JOB_ORIENTATION_BACKFILL, token, { + checked, corrected, requeuedFaces, staleTiers, failed: errorCount, + }); + logger.info( + `Orientation backfill complete: ${checked} checked, ${corrected} corrected, ` + + `${requeuedFaces} requeued for face scanning, ${errorCount} errors` + ); + } catch (err) { + logger.error('Orientation backfill aborted:', err); + await maintenanceJobs + .release(JOB_ORIENTATION_BACKFILL, token, { + checked, corrected, requeuedFaces, staleTiers, failed: errorCount, error: err.message, + }) + .catch(() => {}); + } finally { + lease.stop(); + } + }); + } catch (error) { + logger.error('Error starting orientation backfill:', error); + res.status(500).json({ error: 'Failed to start orientation backfill' }); + } +}); + +router.get('/repair-orientation/status', adminAuth, requirePermission('system.manage'), async (req, res) => { + try { + const state = await maintenanceJobs.read(JOB_ORIENTATION_BACKFILL); + res.json({ isRunning: state.isRunning, lastResult: state.lastResult }); + } catch (error) { + logger.error('Error fetching orientation backfill status:', error); + res.status(500).json({ error: 'Failed to fetch orientation backfill status' }); + } +}); + module.exports = router; diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 82e3b3d0..8822f847 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -2353,7 +2353,16 @@ router.get('/:slug/photo/:photoId', const watermarkHash = watermarkSettings?.enabled ? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}` : '-nowm'; - const etag = `"${photoId}-${mtimeMs}${watermarkHash}"`; + // orientation_checked_at participates because the backfill (#1198) can + // change these bytes without touching either of the other two inputs: + // it rewrites the derived renditions while the ORIGINAL's mtime and the + // watermark settings both stay exactly as they were. Without it a guest + // holding a pre-fix ETag keeps getting 304 and keeps their cached + // sideways image, however many times the backfill succeeds. + const orientationVersion = photo.orientation_checked_at + ? `-o${new Date(photo.orientation_checked_at).getTime()}` + : ''; + const etag = `"${photoId}-${mtimeMs}${watermarkHash}${orientationVersion}"`; if (req.headers['if-none-match'] === etag) { return res.status(304).end(); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index a4b1749d..2e9bdb3b 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -370,6 +370,28 @@ async function isThumbnailValid(thumbnailPath) { * @param {Object} metadata - a sharp metadata object * @returns {{ width: number|null, height: number|null }} */ +/** + * Does this image's EXIF orientation mean `.rotate()` will move its pixels? + * (#1198) + * + * Distinct from orientedDimensions, and the distinction matters. Orientations + * 2, 3 and 4 are a mirror, a 180° turn and a mirrored 180° turn: every pixel + * moves, but width and height are unchanged. A square image with 5-8 is the + * same story. So "did the dimensions change?" is not the same question as "was + * this image transformed", and anything keyed to derived data — face bounding + * boxes, cached previews — has to ask the second one or it silently skips + * exactly those cases. + * + * 1 means no transform. Absent means no tag, which is also no transform. + * + * @param {Object} metadata - a sharp metadata object + * @returns {boolean} + */ +function hasOrientationTransform(metadata) { + const o = metadata && metadata.orientation; + return typeof o === 'number' && o >= 2 && o <= 8; +} + function orientedDimensions(metadata) { if (!metadata || !metadata.width || !metadata.height) return { width: null, height: null }; const swap = metadata.orientation >= 5 && metadata.orientation <= 8; @@ -1305,6 +1327,7 @@ async function resizeToBox(inputBuffer, box, options = {}) { module.exports = { orientedDimensions, + hasOrientationTransform, ensurePreviewImageAtWidth, ensureThumbnailAtWidth, thumbnailTierKeys, diff --git a/backend/src/services/maintenanceJobState.js b/backend/src/services/maintenanceJobState.js index 9f1881b7..1d69c962 100644 --- a/backend/src/services/maintenanceJobState.js +++ b/backend/src/services/maintenanceJobState.js @@ -43,6 +43,7 @@ const logger = require('../utils/logger'); const JOB_DIMENSION_REPAIR = 'photo_dimension_repair'; const JOB_CAPTURE_DATE_BACKFILL = 'photo_capture_date_backfill'; +const JOB_ORIENTATION_BACKFILL = 'photo_orientation_backfill'; // How long a run may go without renewing its lease before another replica is // allowed to take it over. Generous on purpose: these jobs walk the whole @@ -173,6 +174,7 @@ module.exports = { read, JOB_DIMENSION_REPAIR, JOB_CAPTURE_DATE_BACKFILL, + JOB_ORIENTATION_BACKFILL, DEFAULT_STALE_MS, HEARTBEAT_INTERVAL_MS, }; diff --git a/frontend/src/features/settings/tabs/StatusTab.tsx b/frontend/src/features/settings/tabs/StatusTab.tsx index 59cc5125..1f018bd8 100644 --- a/frontend/src/features/settings/tabs/StatusTab.tsx +++ b/frontend/src/features/settings/tabs/StatusTab.tsx @@ -9,6 +9,7 @@ import { Activity, Ruler, CalendarClock, + RotateCw, } from 'lucide-react'; import { Button, Card, Input } from '../../../components/common'; import { useTranslation } from 'react-i18next'; @@ -113,6 +114,30 @@ export const StatusTab: React.FC = ({ refetchInterval: 10000, }); + // Orientation backfill (#1198). No backlog counter of its own: unlike the + // other two it cannot know how many rows need it without re-reading every + // original, which is the job itself. So the button is always available and + // the result line is what tells the operator whether it found anything. + const { data: orientationStatus } = useQuery({ + queryKey: ['photo-orientation-status'], + queryFn: async () => { + const res = await api.get('/admin/photos/repair-orientation/status'); + return res.data; + }, + enabled: isActive && canManageSystem, + refetchInterval: 10000, + }); + + const orientationMutation = useMutation({ + mutationFn: async () => { + const res = await api.post('/admin/photos/repair-orientation'); + return res.data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['photo-orientation-status'] }); + }, + }); + const captureDateMutation = useMutation({ mutationFn: async () => { const res = await api.post('/admin/photos/repair-capture-dates'); @@ -731,6 +756,54 @@ export const StatusTab: React.FC = ({ )} + {orientationStatus && canManageSystem && ( + +

+ + {t('settings.orientationBackfill.title', 'Photo Orientation')} +

+ +

+ {t('settings.orientationBackfill.description', 'Re-read EXIF orientation for photos imported before rotation was applied, correct their stored dimensions, and clear the thumbnails, previews and hero images generated from the unrotated originals. Only photos whose orientation actually changed are touched.')} +

+ + {orientationStatus.lastResult && ( +

+ {t('settings.orientationBackfill.resultSuccess', { + checked: orientationStatus.lastResult.checked, + corrected: orientationStatus.lastResult.corrected, + requeued: orientationStatus.lastResult.requeuedFaces, + failed: orientationStatus.lastResult.failed, + defaultValue: 'Last run: {{checked}} checked, {{corrected}} corrected, {{requeued}} requeued for face scanning, {{failed}} unreachable', + })} + {Number(orientationStatus.lastResult.staleTiers) > 0 && ( + + {t('settings.orientationBackfill.staleTiers', { + count: orientationStatus.lastResult.staleTiers, + defaultValue: '{{count}} cached size(s) could not be deleted and will keep serving the old orientation — re-run once storage is writable.', + })} + + )} +

+ )} + +
+ +
+
+ )} + {/* Update Notification Settings */} diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 9bae0f41..6e33e3d9 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -2363,6 +2363,14 @@ "noneToFill": "Alle Fotos haben bereits ein Aufnahmedatum", "resultSuccess": "Letzter Lauf: {{success}} aktualisiert, {{noExif}} ohne gefundenes Datum, {{failed}} nicht erreichbar", "description": "Trägt „Aufnahmedatum\" aus den EXIF-Daten nach, für Fotos die vor dieser Auswertung importiert wurden. Externe Importe haben nie eines gespeichert, dadurch sortieren diese Galerien nach Importreihenfolge statt nach Aufnahmezeit." + }, + "orientationBackfill": { + "title": "Fotoausrichtung", + "description": "Liest die EXIF-Ausrichtung für Fotos neu ein, die vor der Drehungskorrektur importiert wurden, korrigiert ihre gespeicherten Maße und verwirft Thumbnails, Vorschauen und Hero-Bilder, die aus den ungedrehten Originalen erzeugt wurden. Es werden nur Fotos angefasst, deren Ausrichtung sich tatsächlich ändert.", + "button": "Fotoausrichtung korrigieren", + "running": "Ausrichtung wird geprüft...", + "resultSuccess": "Letzter Lauf: {{checked}} geprüft, {{corrected}} korrigiert, {{requeued}} erneut zur Gesichtserkennung eingereiht, {{failed}} nicht erreichbar", + "staleTiers": "{{count}} zwischengespeicherte Größe(n) konnten nicht gelöscht werden und liefern weiterhin die alte Ausrichtung — bitte erneut ausführen, sobald der Speicher beschreibbar ist." } }, "branding": { diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index f54dfa7b..847ff941 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -1904,6 +1904,14 @@ "noneToFill": "All photos already have a capture date", "resultSuccess": "Last run: {{success}} updated, {{noExif}} with no date found, {{failed}} unreachable", "description": "Backfill \"Date Taken\" from EXIF for photos imported before capture dates were read. External/reference imports never recorded one, so their galleries sort by import order instead of when the photos were taken." + }, + "orientationBackfill": { + "title": "Photo Orientation", + "description": "Re-read EXIF orientation for photos imported before rotation was applied, correct their stored dimensions, and clear the thumbnails, previews and hero images generated from the unrotated originals. Only photos whose orientation actually changed are touched.", + "button": "Fix Photo Orientation", + "running": "Checking orientation...", + "resultSuccess": "Last run: {{checked}} checked, {{corrected}} corrected, {{requeued}} requeued for face scanning, {{failed}} unreachable", + "staleTiers": "{{count}} cached size(s) could not be deleted and will keep serving the old orientation — re-run once storage is writable." } }, "analytics": { diff --git a/frontend/src/i18n/locales/fr.json b/frontend/src/i18n/locales/fr.json index 8e6b0a08..3ecb386f 100644 --- a/frontend/src/i18n/locales/fr.json +++ b/frontend/src/i18n/locales/fr.json @@ -1212,6 +1212,14 @@ "noneToFill": "Toutes les photos ont déjà une date de prise de vue", "resultSuccess": "Dernier passage : {{success}} mises à jour, {{noExif}} sans date trouvée, {{failed}} inaccessibles", "description": "Complète la « date de prise de vue » depuis les EXIF pour les photos importées avant sa lecture. Les imports externes n'en enregistraient aucune, si bien que ces galeries se trient par ordre d'import plutôt que par date de prise de vue." + }, + "orientationBackfill": { + "title": "Orientation des photos", + "description": "Relit l'orientation EXIF des photos importées avant l'application de la rotation, corrige leurs dimensions enregistrées et supprime les miniatures, aperçus et images héro générés à partir des originaux non pivotés. Seules les photos dont l'orientation change réellement sont modifiées.", + "button": "Corriger l'orientation", + "running": "Vérification de l'orientation...", + "resultSuccess": "Dernier passage : {{checked}} vérifiées, {{corrected}} corrigées, {{requeued}} remises en file pour la détection de visages, {{failed}} inaccessibles", + "staleTiers": "{{count}} taille(s) en cache n'ont pas pu être supprimées et continueront de servir l'ancienne orientation — relancez une fois le stockage accessible en écriture." } }, "analytics": { diff --git a/frontend/src/i18n/locales/sl.json b/frontend/src/i18n/locales/sl.json index ecbfe6ef..603d4c90 100644 --- a/frontend/src/i18n/locales/sl.json +++ b/frontend/src/i18n/locales/sl.json @@ -1212,6 +1212,14 @@ "noneToFill": "Vse fotografije že imajo datum zajema", "resultSuccess": "Zadnji zagon: {{success}} posodobljenih, {{noExif}} brez najdenega datuma, {{failed}} nedosegljivih", "description": "Dopolni »datum zajema« iz EXIF za fotografije, uvožene pred njegovim branjem. Zunanji uvozi ga niso zabeležili, zato se te galerije razvrščajo po vrstnem redu uvoza namesto po času zajema." + }, + "orientationBackfill": { + "title": "Usmerjenost fotografij", + "description": "Ponovno prebere zapis EXIF o usmerjenosti za fotografije, uvožene pred popravkom vrtenja, popravi shranjene mere ter odstrani sličice, predoglede in glavne slike, ustvarjene iz nezavrtenih izvirnikov. Spremenjene so samo fotografije, katerih usmerjenost se dejansko spremeni.", + "button": "Popravi usmerjenost", + "running": "Preverjanje usmerjenosti...", + "resultSuccess": "Zadnji zagon: {{checked}} preverjenih, {{corrected}} popravljenih, {{requeued}} znova uvrščenih v prepoznavo obrazov, {{failed}} nedosegljivih", + "staleTiers": "{{count}} predpomnjenih velikosti ni bilo mogoče izbrisati in bodo še naprej prikazovale staro usmerjenost — ponovite, ko bo shramba zapisljiva." } }, "analytics": {