fix(images): backfill orientation for libraries that predate the fix (#1199)

* fix(images): backfill orientation for libraries that predate the fix (#1198)

#1194 corrected the generators and every ingest path, but did nothing for
photos already in the database. Those rows end up worse than untouched ones:
before the fix a rotated photo was CONSISTENTLY wrong — a sideways image in a
tile shaped to match — and 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
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. They look alike but are not the
same operation: the repair FILLS missing values and touches nothing else,
while this RECOMPUTES and invalidates the derived data generated against the
old orientation. Sharing a lease would also mean one blocks the other.

A first attempt at this was reverted from #1194 after review found five
problems. All five are addressed here:

- Originals are read through resolvePhotoStorageKey + withLocalCopy +
  withProcessableImage, so the job works on S3 installs and on RAW/DNG. The
  dimension repair's direct fs read does neither, which stops being an edge
  case in a job that walks the whole library.
- The canonical preview is cleared BEFORE faces are requeued.
  ensurePreviewImage returns a cached preview whenever it is still a valid
  image, and a pre-fix unrotated one is perfectly valid — so requeueing alone
  made the rescan read unrotated pixels and scale those boxes by the corrected
  dimensions, which is worse than leaving the data alone.
- Invalidation keys off the EXIF transform, not a dimension delta. Orientations
  2, 3 and 4 move every pixel while leaving width and height unchanged, as does
  5-8 on a square image; a delta check skips exactly those rows.
- Archived events are excluded — archiving deletes the originals and keeps the
  rows, so every one of them would fail its read.
- The dimension write and the invalidation share a transaction. Split, a
  failure between them leaves stale face data that no retry can fix, because
  the retry computes "already correct".

Tier deletion stays outside the transaction on purpose: it touches storage, and
a failed object delete must not roll back a correct database write. A leftover
tier regenerates on next read; a rolled-back write is silent corruption.

* fix(images): invalidate every stale rendition, fence the writes, and give the job a button (#1198)

Three things from review, one of which mattered a lot.

The invalidation was too narrow. Clearing only preview_path fixed the face
data and left the gallery worse off: ensureThumbnail and ensureHeroImage
return their cached file whenever it is merely VALID, and a pre-fix sideways
thumbnail is perfectly valid — so a corrected row rendered the old sideways
image inside a newly-corrected portrait tile. All three canonical renditions
are cleared now, their stored objects deleted, and both responsive tier sets
with them.

The responsive tiers also needed handling rather than a hopeful catch. Their
helpers swallow delete errors, and ensurePreviewImageAtWidth treats
storage.stat(key) as a cache hit — so a tier that survived deletion keeps
serving unrotated forever and never regenerates. The keys are re-checked after
deletion and survivors are counted into the result, so a run that could not
clear them does not report itself as clean.

Writes are fenced on the identity that was measured, not just the id.
replacePhoto swaps a new file under an existing row and rewrites
path/filename, and it IS reachable — from the replace_by_name upload path in
adminPhotos.js. A replacement landing while this job read the old original
would otherwise have had the previous file's dimensions written over it and
its fresh renditions cleared.

And the job had no way to start it: the endpoint existed with no caller, so an
upgrade would have left every affected library untouched unless an operator
found the API themselves. It gets a Status card like its two neighbours, with
strings in en/de/fr/sl. No backlog counter, because unlike the other two it
cannot know how many rows need it without doing the work.

* fix(images): make the backfill idempotent, and stop it lying about what it did (#1198)

Six things from review round 2.

The job was not idempotent, and the way it failed was expensive. Its trigger is
the EXIF tag on the ORIGINAL, which correcting a photo never changes — so every
re-run threw away the renditions it had just regenerated and requeued every
completed face scan. On a face-enabled install, running it twice meant
re-detecting the whole library for nothing. Migration 191 adds
photos.orientation_checked_at, written in the same transaction as the work it
records, with `force` as the escape hatch for an interrupted run.

The candidate query selected preview_path but not thumbnail_path or hero_path,
which the deletion loop reads — so those two pointers were cleared in the
database while the objects stayed in storage, still reachable through
previously issued URLs.

watermark_path was missed entirely. gallery.js serves it ahead of the original
when branding watermarking is on, which makes it the most visible rendition of
the lot. (Its generator needed rotating too — that went into #1185, where the
other three live.)

storage.stat() RESOLVES with null for a missing key rather than rejecting, so
counting "the promise settled" marked every deleted — and every never-created —
tier as a survivor. A perfectly clean run told the operator to re-run. Now a
null means gone, and a rejection counts as stuck, since a storage error is not
proof the object went away.

Face data is invalidated whenever the stored dimensions change, not only when
the change came from rotation: boxes are scaled by photo.width at read time, so
any dimension change strands them.

And `corrected` now comes from the affected-row count. If the fence rejected the
write because the file was replaced mid-run, the photo was not corrected and
the run must not claim it was.

* fix(images): stop the backfill doing unnecessary work, and make its retry advice true (#1198)

Round 3, four points, all narrower than the last two rounds.

It re-processed photos that were already correct. 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 fine and clearing them
deletes valid files and rescans a completed face detection for nothing. Those
are now skipped and simply marked. Orientations 2, 3 and 4 (and 5-8 on a square
image) leave the dimensions identical either way, so they carry no such
evidence and are still done once.

The retry advice was impossible to follow. When a responsive tier could not be
deleted the row was still marked, so the ordinary re-run the UI recommends
found nothing and the stale tier kept serving unrotated forever. The marker is
withheld when a tier survives, which is what makes that message honest.

Storage cleanup now only runs when a fenced write actually landed. If the file
was replaced mid-run every update matched zero rows, but the deletion went
ahead anyway and could destroy renditions belonging to the REPLACEMENT —
watermarks especially, which are keyed by photo id and alias straight onto the
new file.

And the full-photo ETag includes the backfill's timestamp. It was built from
the ORIGINAL's mtime plus the watermark settings hash, neither of which this
job touches — so a guest holding a pre-fix ETag would go on getting 304 and
their cached sideways image no matter how many times the backfill succeeded.

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-26 21:01:51 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent c18f54ede0
commit edef4d7365
13 changed files with 940 additions and 3 deletions
+344 -1
View File
@@ -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;
+10 -1
View File
@@ -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();
+23
View File
@@ -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,
@@ -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,
};