paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(thumbnails): regenerate external photos, and stop destroying good ones (#1129)

Browse Source 
POST /admin/thumbnails/regenerate resolved every source as
storage/events/active/<photo.path> and fs.access'd it. External and reference
rows are not there — their originals live under events.external_path — so every
one failed the check and was counted as an error, while the UI reported success
because the response is sent before the background loop starts.

It now goes through ensureThumbnail, which resolves both source kinds and writes
thumbnail_path back itself. Nulling thumbnail_path stops it short-circuiting on
isThumbnailValid, which matters because the old thumbnail is normally still
readable at exactly the moment someone presses regenerate.

And the half that destroys data: generateThumbnail deleted the target BEFORE
sharp had opened the source, and again in its catch. A source that could not be
read — a NAS mount that blipped — left the previous rendition gone and the
database pointing at it. Across a bulk regenerate that is the whole gallery.
Neither delete was needed: put stages to a temp file and renames atomically, and
put is the last statement in the try so no partial object can exist.

Also: videos filtered out, and the superseded rendition removed only when the
storage key actually moved, compared through the same canonicalisation the
backends apply.

Stable twin of #1134.
This commit is contained in:
Paul Nothaft
2026-08-22 21:36:19 +02:00
committed by GitHub
parent 7598e20f55
commit dc9e3cdc5e
4 changed files with 416 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/adminThumbnails.js
+62 -28
View File
@@ -3,12 +3,27 @@ const router = express.Router();
const { db } = require('../database/db');
const { adminAuth } = require('../middleware/auth');
const { requirePermission } = require('../middleware/permissions');
const { generateThumbnail, ensurePreviewImage } = require('../services/imageProcessor');
const path = require('path');
const fs = require('fs').promises;
const { ensureThumbnail, ensurePreviewImage } = require('../services/imageProcessor');
const { getStorage } = require('../services/storage');
const logger = require('../utils/logger');
const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage');
/**
* Do these two stored paths address the same object?
*
* Compared the way the storage backends do, not as raw strings.
* LocalFsStorage._resolve and S3StorageBackend._key both fold `\` to `/` and
* strip a leading `./`, so a legacy thumbnail_path in any of those shapes is
* the SAME file as the freshly generated POSIX key while comparing unequal —
* and the "the key moved, delete the old one" branch below would then delete
* the thumbnail that had just been written.
*/
function sameStorageKey(a, b) {
const canonical = (key) => String(key)
.replace(/\\/g, '/')
.replace(/^\.?\/+/, '')
.replace(/\/+/g, '/');
return canonical(a) === canonical(b);
}
// Parse JSON-encoded setting values
function parseSettingValue(value) {
@@ -125,10 +140,21 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r
try {
const { eventId } = req.body; // Optional: regenerate for specific event only
let query = db('photos').select('id', 'event_id', 'path');
// source_origin/external_relpath/filename are what ensureThumbnail branches
// on to resolve an external source off its mount instead of under
// events/active. thumbnail_path is selected so it can be nulled — see below.
let query = db('photos').select(
'id', 'event_id', 'path', 'media_type', 'mime_type', 'thumbnail_path',
'source_origin', 'external_relpath', 'filename'
);
if (eventId) {
query = query.where('event_id', eventId);
}
// Skip videos: their thumbnail is a poster frame from videoProcessor, so
// handing the container file to Sharp only ever produced an error per row.
query = query.where(function() {
this.whereNull('media_type').orWhere('media_type', '!=', 'video');
});
const photos = await query;
@@ -149,30 +175,38 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r
for (const photo of photos) {
try {
const storagePath = getStoragePath();
const originalPath = path.join(storagePath, 'events/active', photo.path);
// Check if original file exists
try {
await fs.access(originalPath);
} catch (err) {
logger.warn(`Original file not found for photo ${photo.id}: ${originalPath}`);
errorCount++;
continue;
}
// Regenerate thumbnail
const thumbnailPath = await generateThumbnail(originalPath, { regenerate: true });
if (thumbnailPath) {
// Update database with new thumbnail path
await db('photos')
.where({ id: photo.id })
.update({
thumbnail_path: thumbnailPath,
updated_at: db.fn.now()
// Through ensureThumbnail, not a hand-rolled path (#1129). This route
// used to resolve every source as `storage/events/active/<path>` and
// fs.access it — a location that does not exist for external or
// reference rows, whose originals live under events.external_path. So
// every one of them failed the check and was counted as an error: on a
// reference install the endpoint rebuilt nothing while the UI reported
// success, because the response is sent before this loop starts.
//
// ensureThumbnail already resolves both source kinds, uses the
// per-photo ext<id>_ output name so two events referencing one NAS
// basename cannot clobber each other, and writes thumbnail_path back
// itself. Nulling thumbnail_path is what stops it short-circuiting on
// isThumbnailValid — necessary rather than cosmetic, because the old
// thumbnail is normally still readable at exactly the moment someone
// presses regenerate.
const newThumbnailPath = await ensureThumbnail({ ...photo, thumbnail_path: null });
if (newThumbnailPath) {
// Drop the superseded rendition when the key MOVED. On S3 the source
// is downloaded to a randomly-named temp file and, for non-RAW input,
// the key is derived from that name — so it differs every run, and
// nulling thumbnail_path hides the old key from everything that would
// otherwise clean it up. Guarded on the key actually changing: local
// storage is stable, and deleting the equal key would delete the file
// just written.
if (photo.thumbnail_path && !sameStorageKey(photo.thumbnail_path, newThumbnailPath)) {
await getStorage().delete(photo.thumbnail_path).catch((err) => {
logger.warn(
`Could not remove superseded thumbnail ${photo.thumbnail_path} for photo ${photo.id}: ${err.message}`
);
});
}
successCount++;
logger.info(`Regenerated thumbnail for photo ${photo.id}`);
} else {
backend/src/services/imageProcessor.js
+18 -7
View File
@@ -133,10 +133,18 @@ async function generateThumbnail(imagePath, options = {}) {
// Get thumbnail settings
const settings = await getThumbnailSettings();
// Force regeneration: drop the existing object before writing the new one
if (options.regenerate) {
await storage.delete(thumbnailRelKey).catch(() => {});
}
// `options.regenerate` deliberately does NOT delete the existing object first
// (#1129).
//
// It used to, and the delete ran BEFORE sharp had even opened the source — so
// a source that could not be read (a NAS mount that blipped, a corrupt file)
// left the old thumbnail already gone and returned null, with the database
// still pointing at it. One bulk regeneration during a mount outage could
// therefore strip every canonical thumbnail in a reference gallery.
//
// Nothing is lost by dropping it: LocalFsStorage.put stages to a temp file and
// renames over the target, which replaces atomically, and an S3 put overwrites
// by key. The delete only added a window with no thumbnail at all.
try {
// First, verify the source image is complete and valid
@@ -192,9 +200,12 @@ async function generateThumbnail(imagePath, options = {}) {
const msg = (error && error.message) ? error.message : String(error);
logger.error(`Failed to generate thumbnail for ${sourceBasename}: ${msg}`);
// Clean up any partially uploaded object
await storage.delete(thumbnailRelKey).catch(() => {});
// No cleanup delete here either, for the same reason (#1129). This was
// "clean up any partially uploaded object", but there cannot be one:
// storage.put is the LAST statement in the try, every throw above it
// happens before anything is written, and put unlinks its own temp file on
// failure. The only object this could remove is the PREVIOUS, valid
// rendition — exactly the thumbnail a failed regeneration must leave alone.
return null;
}
}