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

fix(thumbnails): regenerate external photos instead of dropping their tiers (#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, so every one failed and was counted as an error — and
because the tier deletion runs first, the endpoint dropped every ?w= tier and
rebuilt nothing, leaving the library worse than before it ran. The UI reported
success either way.

Now routed through ensureThumbnail, which resolves both source kinds, uses the
per-photo ext<id>_ output name, and writes thumbnail_path back itself.

Review rounds also removed both destructive deletes in generateThumbnail: the
pre-delete ran before sharp opened the source, so an unreadable source left the
previous rendition gone and the database pointing at it — across a bulk run,
the whole gallery. Neither delete was needed, since put stages to a temp file
and renames atomically and is the last statement in the try.

Videos are filtered out, and the superseded rendition is removed only when the
storage key actually moved, compared through the same canonicalisation the
backends apply so a legacy backslash path is not mistaken for a different
object.

Reported by @BraynArts, who also identified the fix.
This commit is contained in:
Paul Nothaft
2026-08-22 21:37:49 +02:00
committed by GitHub
parent f735d26422
commit 97d92f8428
4 changed files with 439 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/adminThumbnails.js
+74 -37
View File
@@ -3,12 +3,29 @@ 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 `thumbnails\thumb_x.jpg`, `./thumbnails/thumb_x.jpg`
* and `thumbnails/thumb_x.jpg` are all one file. A legacy thumbnail_path in
* any of those shapes would compare unequal to the freshly generated POSIX
* key — and the "the key moved, delete the old one" branch below would then
* delete the thumbnail that had just been written, leaving every regenerated
* photo pointing at nothing.
*/
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,15 +142,26 @@ router.post('/regenerate', adminAuth, requirePermission('photos.edit'), async (r
try {
const { eventId } = req.body; // Optional: regenerate for specific event only
// source_origin/external_relpath/filename are selected for
// deleteThumbnailTiers below — it derives the tier keys from the same
// fields ensureThumbnailAtWidth used to write them.
// source_origin/external_relpath/filename feed BOTH deleteThumbnailTiers
// (which derives the tier keys from the same fields ensureThumbnailAtWidth
// wrote them with) and ensureThumbnail, which branches on them 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', 'source_origin', 'external_relpath', 'filename');
.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, matching /regenerate-previews. Their thumbnail is a poster
// frame from videoProcessor, so handing the container file to Sharp here
// only ever produced an error per video row.
query = query.where(function() {
this.whereNull('media_type').orWhere('media_type', '!=', 'video');
});
const photos = await query;
if (photos.length === 0) {
@@ -153,44 +181,53 @@ 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);
// Drop the responsive tiers first (#1095), same as the preview
// endpoint below. They are cached by width outside thumbnail_path
// and their key carries no settings version, so regenerating only
// the canonical rendition leaves phones served the old fit, quality
// or format indefinitely — which is exactly what this endpoint is
// invoked to undo after a settings change.
//
// Above the fs.access below, not after it: that check only passes
// for managed photos on a local filesystem. On S3, and for external
// or reference rows, it fails and skips the photo — so invalidating
// after it would leave stale tiers on precisely the deployments
// where they are hardest to notice.
await require('../services/imageProcessor').deleteThumbnailTiers(photo);
// 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;
}
// Through ensureThumbnail, not a hand-rolled path (#1129). This
// route used to resolve the 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.
// Every such photo failed the check and was counted as an error, so
// on a reference install the endpoint dropped every tier and
// rebuilt nothing, while the UI reported success (the response is
// sent before this loop starts).
//
// ensureThumbnail already resolves both source kinds via
// resolvePhotoFilePath/resolvePhotoStorageKey, 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 — the same trick /regenerate-previews uses.
const newThumbnailPath = await ensureThumbnail({ ...photo, thumbnail_path: null });
// 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()
if (newThumbnailPath) {
// Drop the superseded canonical rendition when the key MOVED.
//
// For a managed photo on S3, ensureThumbnail downloads the source
// to a randomly-named temp file, and for non-RAW input
// withProcessableImage passes no outputBasename — so the key is
// derived from that random name and differs on every run. Nulling
// thumbnail_path above hides the old key from everything that
// would otherwise clean it up, so without this each regeneration
// strands a full thumbnail in the bucket, once per photo per run.
//
// Guarded on the key actually changing: on local storage it is
// stable, and deleting the equal key would delete the file that
// was just written.
if (photo.thumbnail_path && !sameStorageKey(photo.thumbnail_path, newThumbnailPath)) {
await getStorage().delete(photo.thumbnail_path).catch((err) => {
// Losing the old object is untidy, not a failed regeneration.
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
+22 -7
View File
@@ -230,10 +230,21 @@ async function generateThumbnail(imagePath, options = {}) {
const thumbnailFilename = `thumb_${widthTag}${outputBasename}`;
const thumbnailRelKey = path.posix.join('thumbnails', thumbnailFilename);
// 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 and
// leave the whole library serving 404s.
//
// 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. So the write replaces the old rendition either way —
// the only thing the delete added was a window in which there was no
// thumbnail at all.
try {
// First, verify the source image is complete and valid
@@ -294,9 +305,13 @@ 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 as above (#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 itself stages to a temp
// file and only renames on success. So the only object this delete could
// ever have removed is the PREVIOUS, perfectly good rendition — which is
// exactly the thumbnail a failed regeneration must leave alone.
return null;
}
}