fix(gallery): serve thumbnails / photos / hero via storage abstraction (#432)

Three gallery serving routes bypassed the getStorage() abstraction and
used fs.* directly against local paths. Worked in local-fs mode, 500'd
in S3 mode because the files only exist in the bucket. Reported by
@w1ll-i-code with a precise root-cause pointer at gallery.js:1138.

The admin photo serving route (adminPhotos.js) had already been
converted to use storage.stat + storage.get; the gallery side hadn't.
This PR brings the gallery routes in line.

Changes:

- Add getRange(relPath, start, end) to the StorageBackend interface +
  LocalFsStorage (fs.createReadStream with start/end) + S3StorageBackend
  (downloadStream with Range header). Needed for video range requests
  on S3 — previously the photo route did fs.createReadStream(filePath,
  {start, end}) which is local-only.

- /:slug/thumbnail/:photoId — read mtime via storage.stat, stream bytes
  via storage.get. Watermark application path materializes the source
  via withLocalCopy (no-op in local mode, downloads to a tmp file then
  cleans up in S3 mode) so applyWatermark's sharp + fs.readFile still
  works.

- /:slug/photo/:photoId — branches on source_origin: external/reference
  photos still use the local fs path (NAS mounts are local), managed
  photos use the storage abstraction. Video range requests pass through
  to storage.getRange. Pre-generated watermarks served via storage too.
  On-the-fly watermark generation uses withLocalCopy for managed photos.

- /:slug/hero/:photoId — hero images are always managed-storage keys
  (imageProcessor.generateHeroImage writes via the storage abstraction),
  so this just switches to storage.stat + storage.get. Watermark via
  withLocalCopy.

Verified end-to-end against minio in dev:
  POST /api/admin/photos/N/upload         → photo + thumbnail land in S3
  GET /api/gallery/<slug>/thumbnail/<id>  → 200, JPEG 300x300 ✓
  GET /api/gallery/<slug>/photo/<id>      → 200, JPEG 1200x800 ✓
  GET /api/gallery/<slug>/hero/<id>       → 200, JPEG 1920x1080 ✓
  ETag round-trip (If-None-Match)         → 304 ✓
  Backend logs                            → no errors

LocalFs regression: 13/13 smoke tests pass.

Closes #432.
This commit is contained in:
Paul Nothaft
2026-05-09 20:56:20 +02:00
parent ed37caf3d8
commit 83d79f4d39
4 changed files with 173 additions and 101 deletions
+127 -65
View File
@@ -13,7 +13,7 @@ const { resolvePhotoFilePath } = require('../services/photoResolver');
const { getEventShareToken, resolveShareIdentifier, buildShareLinkVariants } = require('../services/shareLinkService'); const { getEventShareToken, resolveShareIdentifier, buildShareLinkVariants } = require('../services/shareLinkService');
const { handleAsync } = require('../utils/routeHelpers'); const { handleAsync } = require('../utils/routeHelpers');
const { NotFoundError } = require('../utils/errors'); const { NotFoundError } = require('../utils/errors');
const { ensureThumbnail, ensureHeroImage } = require('../services/imageProcessor'); const { ensureThumbnail, ensureHeroImage, withLocalCopy } = require('../services/imageProcessor');
const downloadZipService = require('../services/downloadZipService'); const downloadZipService = require('../services/downloadZipService');
const { getStorage } = require('../services/storage'); const { getStorage } = require('../services/storage');
const fs = require('fs'); const fs = require('fs');
@@ -937,11 +937,47 @@ router.get('/:slug/photo/:photoId',
}); });
} }
// Resolve the absolute file path for this photo, supporting both managed and external reference modes // Resolve where to read the photo bytes from. For external/reference
const { resolvePhotoFilePath } = require('../services/photoResolver'); // photos the source is always a local mount path. For managed photos
const fs = require('fs'); // we go through the storage abstraction so S3 deployments work too
// (#432 — previously this route did fs.* directly and 500'd in S3
// mode because the file wasn't on the container's local fs).
const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('../services/photoResolver');
const storage = getStorage();
const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
const useStorageBackend = !isExternal;
let filePath; let filePath = null; // Local fs path (external photos OR LocalFs storage)
let storageKey = null; // Relative storage key (managed photos via storage abstraction)
let stat;
let fileSize;
if (useStorageBackend) {
try {
storageKey = resolvePhotoStorageKey(req.event, photo);
} catch (resolveError) {
logger.error('Failed to resolve photo storage key', {
slug: req.params.slug,
photoId,
eventId: req.event.id,
error: resolveError.message,
photoPath: photo.path,
photoFilename: photo.filename
});
return res.status(404).json({ error: 'Photo file not found' });
}
stat = await storage.stat(storageKey);
if (!stat) {
logger.error('Photo not found in storage backend', {
slug: req.params.slug,
photoId,
eventId: req.event.id,
storageKey
});
return res.status(404).json({ error: 'Photo file not found' });
}
fileSize = stat.size;
} else {
try { try {
filePath = resolvePhotoFilePath(req.event, photo); filePath = resolvePhotoFilePath(req.event, photo);
} catch (resolveError) { } catch (resolveError) {
@@ -955,8 +991,6 @@ router.get('/:slug/photo/:photoId',
}); });
return res.status(404).json({ error: 'Photo file not found' }); return res.status(404).json({ error: 'Photo file not found' });
} }
// Verify file exists before attempting to serve
if (!fs.existsSync(filePath)) { if (!fs.existsSync(filePath)) {
logger.error('Photo file does not exist at resolved path', { logger.error('Photo file does not exist at resolved path', {
slug: req.params.slug, slug: req.params.slug,
@@ -967,28 +1001,19 @@ router.get('/:slug/photo/:photoId',
}); });
return res.status(404).json({ error: 'Photo file not found' }); return res.status(404).json({ error: 'Photo file not found' });
} }
stat = fs.statSync(filePath);
// Log access - temporarily disabled for debugging fileSize = stat.size;
// await secureImageService.logImageAccess( }
// photoId,
// req.event.id,
// req.clientInfo,
// 'view_basic'
// );
// Handle video streaming with range requests // Handle video streaming with range requests
if (isVideo) { if (isVideo) {
const stat = fs.statSync(filePath);
const fileSize = stat.size;
const range = req.headers.range; const range = req.headers.range;
if (range) { if (range) {
// Parse range header
const parts = range.replace(/bytes=/, '').split('-'); const parts = range.replace(/bytes=/, '').split('-');
const start = parseInt(parts[0], 10); const start = parseInt(parts[0], 10);
const end = parts[1] ? parseInt(parts[1], 10) : fileSize - 1; const end = parts[1] ? parseInt(parts[1], 10) : fileSize - 1;
const chunksize = (end - start) + 1; const chunksize = (end - start) + 1;
const file = fs.createReadStream(filePath, { start, end });
res.writeHead(206, { res.writeHead(206, {
'Content-Range': `bytes ${start}-${end}/${fileSize}`, 'Content-Range': `bytes ${start}-${end}/${fileSize}`,
@@ -999,9 +1024,11 @@ router.get('/:slug/photo/:photoId',
'X-Protection-Level': 'basic' 'X-Protection-Level': 'basic'
}); });
const file = useStorageBackend
? await storage.getRange(storageKey, start, end)
: fs.createReadStream(filePath, { start, end });
file.pipe(res); file.pipe(res);
} else { } else {
// No range request, send entire file
res.writeHead(200, { res.writeHead(200, {
'Content-Length': fileSize, 'Content-Length': fileSize,
'Content-Type': photo.mime_type || 'video/mp4', 'Content-Type': photo.mime_type || 'video/mp4',
@@ -1009,35 +1036,47 @@ router.get('/:slug/photo/:photoId',
'Cache-Control': 'private, max-age=1800', 'Cache-Control': 'private, max-age=1800',
'X-Protection-Level': 'basic' 'X-Protection-Level': 'basic'
}); });
const file = useStorageBackend
fs.createReadStream(filePath).pipe(res); ? await storage.get(storageKey)
: fs.createReadStream(filePath);
file.pipe(res);
} }
return; return;
} }
// Handle images (existing logic) // Image path
// Get watermark settings
const watermarkSettings = await watermarkService.getWatermarkSettings(); const watermarkSettings = await watermarkService.getWatermarkSettings();
// Generate ETag based on photo id, modification time, and watermark settings const mtimeMs = stat.mtime ? stat.mtime.getTime() : 0;
// This ensures cache invalidation when watermark settings change
const stat = fs.statSync(filePath);
const watermarkHash = watermarkSettings?.enabled const watermarkHash = watermarkSettings?.enabled
? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}` ? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}`
: '-nowm'; : '-nowm';
const etag = `"${photoId}-${stat.mtime.getTime()}${watermarkHash}"`; const etag = `"${photoId}-${mtimeMs}${watermarkHash}"`;
// Check if client has valid cached version
if (req.headers['if-none-match'] === etag) { if (req.headers['if-none-match'] === etag) {
return res.status(304).end(); return res.status(304).end();
} }
if (watermarkSettings && watermarkSettings.enabled) { if (watermarkSettings && watermarkSettings.enabled) {
// Try to serve pre-generated watermarked file for instant loading // Pre-generated watermarked file: served via the storage backend
// (managed) or directly from local fs (external).
if (photo.watermark_path) { if (photo.watermark_path) {
const watermarkFilePath = path.join(getStoragePath(), photo.watermark_path);
try { try {
// Check if pre-generated watermark file exists if (useStorageBackend) {
const wmStat = await storage.stat(photo.watermark_path);
if (wmStat) {
res.set({
'Content-Type': photo.mime_type || 'image/jpeg',
'Content-Length': wmStat.size,
'Cache-Control': 'private, max-age=1800',
'ETag': etag,
'X-Protection-Level': 'basic'
});
const wmStream = await storage.get(photo.watermark_path);
return wmStream.pipe(res);
}
} else {
const watermarkFilePath = path.join(getStoragePath(), photo.watermark_path);
if (fs.existsSync(watermarkFilePath)) { if (fs.existsSync(watermarkFilePath)) {
res.set({ res.set({
'Content-Type': photo.mime_type || 'image/jpeg', 'Content-Type': photo.mime_type || 'image/jpeg',
@@ -1047,15 +1086,19 @@ router.get('/:slug/photo/:photoId',
}); });
return res.sendFile(watermarkFilePath); return res.sendFile(watermarkFilePath);
} }
}
} catch (err) { } catch (err) {
// File doesn't exist or error, fall through to on-the-fly generation
logger.warn(`Pre-generated watermark not found for photo ${photoId}, falling back to on-the-fly`); logger.warn(`Pre-generated watermark not found for photo ${photoId}, falling back to on-the-fly`);
} }
} }
// Fallback: Apply watermark on-the-fly (slower, but ensures image is served) // Fallback: apply watermark on-the-fly. applyWatermark needs a
// Also queue regeneration for next time // local file path (sharp + fs.readFile) — for managed photos in
const watermarkedBuffer = await watermarkService.applyWatermark(filePath, watermarkSettings); // S3 mode, withLocalCopy materializes to a tmp file and cleans up.
const watermarkedBuffer = useStorageBackend
? await withLocalCopy(storageKey, (localPath) =>
watermarkService.applyWatermark(localPath, watermarkSettings))
: await watermarkService.applyWatermark(filePath, watermarkSettings);
// Queue watermark generation in background for next request // Queue watermark generation in background for next request
watermarkGeneratorService.generateForPhoto(photo.id) watermarkGeneratorService.generateForPhoto(photo.id)
@@ -1063,23 +1106,28 @@ router.get('/:slug/photo/:photoId',
res.set({ res.set({
'Content-Type': photo.mime_type || 'image/jpeg', 'Content-Type': photo.mime_type || 'image/jpeg',
'Cache-Control': 'private, max-age=1800', // Cache for 30 minutes 'Cache-Control': 'private, max-age=1800',
'ETag': etag, 'ETag': etag,
'X-Protection-Level': 'basic' 'X-Protection-Level': 'basic'
}); });
res.send(watermarkedBuffer); res.send(watermarkedBuffer);
} else { } else {
// Send original file with basic protection headers
res.set({ res.set({
'Cache-Control': 'private, max-age=1800', 'Cache-Control': 'private, max-age=1800',
'ETag': etag, 'ETag': etag,
'X-Protection-Level': 'basic' 'X-Protection-Level': 'basic'
}); });
// Ensure absolute path for res.sendFile if (useStorageBackend) {
res.set('Content-Length', stat.size);
if (photo.mime_type) res.set('Content-Type', photo.mime_type);
const stream = await storage.get(storageKey);
stream.pipe(res);
} else {
const absolutePath = path.isAbsolute(filePath) ? filePath : path.resolve(filePath); const absolutePath = path.isAbsolute(filePath) ? filePath : path.resolve(filePath);
res.sendFile(absolutePath); res.sendFile(absolutePath);
} }
}
} catch (error) { } catch (error) {
logger.error('Error serving photo:', { logger.error('Error serving photo:', {
error: error.message, error: error.message,
@@ -1120,7 +1168,16 @@ router.get('/:slug/thumbnail/:photoId',
return res.status(404).json({ error: 'Thumbnail generation failed' }); return res.status(404).json({ error: 'Thumbnail generation failed' });
} }
const thumbPath = path.join(getStoragePath(), thumbnailPath); // Read thumbnail metadata via the storage abstraction so we work in
// both LocalFs and S3 modes (#432). The previous fs.statSync on the
// resolved local path 500'd in S3 deployments because the thumbnail
// only exists in the bucket, not on the container's local fs.
const storage = getStorage();
const stat = await storage.stat(thumbnailPath);
if (!stat) {
logger.error(`Thumbnail not found in storage backend for photo ${photoId}`, { thumbnailPath });
return res.status(404).json({ error: 'Thumbnail not found' });
}
// Log thumbnail access // Log thumbnail access
await secureImageService.logImageAccess( await secureImageService.logImageAccess(
@@ -1133,13 +1190,12 @@ router.get('/:slug/thumbnail/:photoId',
// Check if watermarks are enabled and apply to thumbnail // Check if watermarks are enabled and apply to thumbnail
const watermarkSettings = await watermarkService.getWatermarkSettings(); const watermarkSettings = await watermarkService.getWatermarkSettings();
// Generate ETag based on photo id, thumbnail modification time, and watermark settings // ETag uses storage stat mtime + photo id + watermark hash.
const fs = require('fs'); const mtimeMs = stat.mtime ? stat.mtime.getTime() : 0;
const stat = fs.statSync(thumbPath);
const watermarkHash = watermarkSettings?.enabled const watermarkHash = watermarkSettings?.enabled
? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}` ? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}`
: '-nowm'; : '-nowm';
const etag = `"thumb-${photoId}-${stat.mtime.getTime()}${watermarkHash}"`; const etag = `"thumb-${photoId}-${mtimeMs}${watermarkHash}"`;
// Check if client has valid cached version // Check if client has valid cached version
if (req.headers['if-none-match'] === etag) { if (req.headers['if-none-match'] === etag) {
@@ -1157,12 +1213,17 @@ router.get('/:slug/thumbnail/:photoId',
}); });
if (watermarkSettings && watermarkSettings.enabled) { if (watermarkSettings && watermarkSettings.enabled) {
// Apply watermark to thumbnail // Watermarking needs a local file path (sharp + fs.readFile).
const watermarkedBuffer = await watermarkService.applyWatermark(thumbPath, watermarkSettings); // Materialize via withLocalCopy — no-op in local mode, downloads
// to a tmp file then cleans up in S3 mode.
const watermarkedBuffer = await withLocalCopy(thumbnailPath, (localPath) =>
watermarkService.applyWatermark(localPath, watermarkSettings)
);
res.send(watermarkedBuffer); res.send(watermarkedBuffer);
} else { } else {
// Send file without watermark res.setHeader('Content-Length', stat.size);
res.sendFile(path.resolve(thumbPath)); const stream = await storage.get(thumbnailPath);
stream.pipe(res);
} }
} catch (error) { } catch (error) {
logger.error('Error serving thumbnail:', { logger.error('Error serving thumbnail:', {
@@ -1211,30 +1272,27 @@ router.get('/:slug/hero/:photoId',
return res.redirect(`/api/gallery/${req.params.slug}/photo/${photoId}`); return res.redirect(`/api/gallery/${req.params.slug}/photo/${photoId}`);
} }
const heroFullPath = path.join(getStoragePath(), heroPath); // Hero images are always written via the storage abstraction (see
const fs = require('fs'); // imageProcessor.generateHeroImage), so they're a managed-storage
// key in both LocalFs and S3 modes (#432). Read via storage.
// Verify file exists before attempting to serve const storage = getStorage();
if (!fs.existsSync(heroFullPath)) { const stat = await storage.stat(heroPath);
logger.error('Hero image file does not exist at resolved path', { if (!stat) {
logger.error('Hero image file does not exist in storage backend', {
slug: req.params.slug, slug: req.params.slug,
photoId, photoId,
eventId: req.event.id, eventId: req.event.id,
resolvedPath: heroFullPath heroPath
}); });
return res.redirect(`/api/gallery/${req.params.slug}/photo/${photoId}`); return res.redirect(`/api/gallery/${req.params.slug}/photo/${photoId}`);
} }
// Get file stats for ETag const mtimeMs = stat.mtime ? stat.mtime.getTime() : 0;
const stat = fs.statSync(heroFullPath); const etag = `"hero-${photoId}-${mtimeMs}"`;
const etag = `"hero-${photoId}-${stat.mtime.getTime()}"`;
// Check if client has valid cached version
if (req.headers['if-none-match'] === etag) { if (req.headers['if-none-match'] === etag) {
return res.status(304).end(); return res.status(304).end();
} }
// Check if watermarks should be applied
const watermarkSettings = await watermarkService.getWatermarkSettings(); const watermarkSettings = await watermarkService.getWatermarkSettings();
res.set({ res.set({
@@ -1247,12 +1305,16 @@ router.get('/:slug/hero/:photoId',
}); });
if (watermarkSettings && watermarkSettings.enabled) { if (watermarkSettings && watermarkSettings.enabled) {
// Apply watermark to hero image // applyWatermark needs a local file path; materialize via
const watermarkedBuffer = await watermarkService.applyWatermark(heroFullPath, watermarkSettings); // withLocalCopy so this works in S3 mode too.
const watermarkedBuffer = await withLocalCopy(heroPath, (localPath) =>
watermarkService.applyWatermark(localPath, watermarkSettings)
);
res.send(watermarkedBuffer); res.send(watermarkedBuffer);
} else { } else {
// Send hero image without watermark res.setHeader('Content-Length', stat.size);
res.sendFile(path.resolve(heroFullPath)); const stream = await storage.get(heroPath);
stream.pipe(res);
} }
} catch (error) { } catch (error) {
logger.error('Error serving hero image:', { logger.error('Error serving hero image:', {
@@ -76,6 +76,11 @@ class LocalFsStorage {
return fs.createReadStream(abs); return fs.createReadStream(abs);
} }
async getRange(relPath, start, end) {
const abs = this._resolve(relPath);
return fs.createReadStream(abs, { start, end });
}
async getToFile(relPath, localPath) { async getToFile(relPath, localPath) {
const abs = this._resolve(relPath); const abs = this._resolve(relPath);
await fsp.mkdir(path.dirname(localPath), { recursive: true }); await fsp.mkdir(path.dirname(localPath), { recursive: true });
@@ -80,6 +80,10 @@ class S3StorageBackend {
return this.adapter.downloadStream(this._key(relPath)); return this.adapter.downloadStream(this._key(relPath));
} }
async getRange(relPath, start, end) {
return this.adapter.downloadStream(this._key(relPath), { range: `bytes=${start}-${end}` });
}
async getToFile(relPath, localPath) { async getToFile(relPath, localPath) {
await fsp.mkdir(path.dirname(localPath), { recursive: true }); await fsp.mkdir(path.dirname(localPath), { recursive: true });
await this.adapter.download(this._key(relPath), localPath); await this.adapter.download(this._key(relPath), localPath);
@@ -29,6 +29,7 @@
* @property {(relPath: string, body: NodeJS.ReadableStream | Buffer, options?: PutOptions) => Promise<void>} put * @property {(relPath: string, body: NodeJS.ReadableStream | Buffer, options?: PutOptions) => Promise<void>} put
* @property {(relPath: string, localPath: string, options?: PutOptions) => Promise<void>} putFromFile * @property {(relPath: string, localPath: string, options?: PutOptions) => Promise<void>} putFromFile
* @property {(relPath: string) => Promise<NodeJS.ReadableStream>} get - Returns a readable stream of the object body. * @property {(relPath: string) => Promise<NodeJS.ReadableStream>} get - Returns a readable stream of the object body.
* @property {(relPath: string, start: number, end: number) => Promise<NodeJS.ReadableStream>} getRange - Returns a readable stream of the object body for the inclusive byte range [start, end]. Used by video range-request handlers.
* @property {(relPath: string, localPath: string) => Promise<void>} getToFile - Streams the object to a local path (creates parent dirs). * @property {(relPath: string, localPath: string) => Promise<void>} getToFile - Streams the object to a local path (creates parent dirs).
* @property {(relPath: string) => Promise<boolean>} exists * @property {(relPath: string) => Promise<boolean>} exists
* @property {(relPath: string) => Promise<StatResult|null>} stat - Null if missing. * @property {(relPath: string) => Promise<StatResult|null>} stat - Null if missing.