From f735d26422ddd7e4f83cdbaf6fa10c2120dc82a4 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sat, 22 Aug 2026 21:37:15 +0200 Subject: [PATCH] fix(gallery): a missing thumbnail tier must not take the backend down (#1128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first load of a gallery whose ?w= tiers do not exist yet could exit the Node process — not 500 one tile, kill the backend. Two defects stacked. The reader: LocalFsStorage.get() returns a lazy fs.createReadStream, so an ENOENT arrives after the await returned and outside the route's try/catch. An unhandled 'error' event is a process-level throw. pipeStreamToResponse attaches the handler the routes were missing — 404 for a vanished source, connection destroyed if bytes are already on the wire, file headers cleared so the JSON error is not served as image/jpeg or cached as a broken tile for an hour. Applied to all nine streaming responses in gallery.js. The writer: ensureThumbnailAtWidth passed regenerate:true, whose first act is to DELETE the target — on a path only reached when the tier is absent. A grid fires one request per tile, so one request unlinked the file another had just published and handed to a reader. Without the flag the write is an atomic rename. Generation is now also deduped per tier key: 8 concurrent requests ran 5 Sharp passes before, 1 after. Reported with a full diagnosis by @BraynArts. --- .../integration/previewTiers.test.js | 83 +++++++++ .../__tests__/utils/streamResponse.test.js | 160 ++++++++++++++++++ backend/src/routes/gallery.js | 19 ++- backend/src/services/imageProcessor.js | 80 ++++++--- backend/src/utils/streamResponse.js | 81 +++++++++ 5 files changed, 393 insertions(+), 30 deletions(-) create mode 100644 backend/__tests__/utils/streamResponse.test.js create mode 100644 backend/src/utils/streamResponse.js diff --git a/backend/__tests__/integration/previewTiers.test.js b/backend/__tests__/integration/previewTiers.test.js index 8364aceb..97bfb511 100644 --- a/backend/__tests__/integration/previewTiers.test.js +++ b/backend/__tests__/integration/previewTiers.test.js @@ -371,5 +371,88 @@ describe('preview tiers (#1095)', () => { await imageProcessor.deleteThumbnailTiers(await db('photos').where({ id: photo.id }).first()); expect(fs.existsSync(abs)).toBe(false); }); + + /** + * The crash in #1128 needed two things: a tier that disappears, and a + * reader that dies on it. The reader is fixed in streamResponse; this is + * the half that stops the file disappearing in the first place. + */ + describe('concurrent generation (#1128)', () => { + it('never leaves the tier absent once it has been published', async () => { + const photo = await seedThumbPhoto(); + const key = imageProcessor.thumbnailTierKeys(photo).find((k) => k.includes('_w600_')); + const abs = path.join(process.env.STORAGE_PATH, key); + + // A grid fires one request per tile at once, and on a cold gallery + // every one of them misses the cache. Previously each carried + // `regenerate: true`, whose first act is to DELETE the target — so a + // later arrival unlinked the file an earlier one had already published + // and handed to a reader. + const watcher = []; + const poll = setInterval(() => watcher.push(fs.existsSync(abs)), 1); + + const results = await Promise.all( + Array.from({ length: 12 }, () => imageProcessor.ensureThumbnailAtWidth(photo, 600)) + ); + clearInterval(poll); + + expect(results.every((r) => r === key)).toBe(true); + expect(fs.existsSync(abs)).toBe(true); + + // Once true, never false again: no window where a validated file is gone. + const firstSeen = watcher.indexOf(true); + if (firstSeen !== -1) { + expect(watcher.slice(firstSeen).every(Boolean)).toBe(true); + } + }); + + it('runs one generation for a burst of requests, not one per request', async () => { + const photo = await seedThumbPhoto(); + const thumbDir = path.join(process.env.STORAGE_PATH, 'thumbnails'); + await fs.promises.mkdir(thumbDir, { recursive: true }); + + // Counted through the staging files LocalFsStorage writes: + // `.tmp..`, one per put, each a distinct random suffix. + // So distinct temp names == distinct generations, which is the thing + // the dedupe is supposed to collapse. (Spying on generateThumbnail + // would not work — ensureThumbnailAtWidth calls it through the + // module-local binding, so an export spy never sees it.) + const seen = new Set(); + const poll = setInterval(() => { + for (const f of fs.readdirSync(thumbDir)) { + if (f.includes('_w900_') && f.includes('.tmp.')) seen.add(f); + } + }, 1); + + const results = await Promise.all( + Array.from({ length: 8 }, () => imageProcessor.ensureThumbnailAtWidth(photo, 900)) + ); + clearInterval(poll); + + const abs = path.join( + process.env.STORAGE_PATH, + imageProcessor.thumbnailTierKeys(photo).find((k) => k.includes('_w900_')) + ); + expect(fs.existsSync(abs)).toBe(true); + expect(new Set(results).size).toBe(1); + // 8 requests, at most one Sharp pass. Before the dedupe this was 8 — + // and on an external photo, 8 full reads of the original. + expect(seen.size).toBeLessThanOrEqual(1); + }); + + it('does not cache a failure — a later request retries', async () => { + const photo = await seedThumbPhoto(); + // Source removed underneath: generation fails and must not poison the + // key for the lifetime of the process. + const src = path.join(process.env.STORAGE_PATH, 'events/active', photo.path); + const saved = await fs.promises.readFile(src); + await fs.promises.unlink(src); + + expect(await imageProcessor.ensureThumbnailAtWidth(photo, 600)).toBeNull(); + + await fs.promises.writeFile(src, saved); + expect(await imageProcessor.ensureThumbnailAtWidth(photo, 600)).toContain('_w600_'); + }); + }); }); }); diff --git a/backend/__tests__/utils/streamResponse.test.js b/backend/__tests__/utils/streamResponse.test.js new file mode 100644 index 00000000..52e5644e --- /dev/null +++ b/backend/__tests__/utils/streamResponse.test.js @@ -0,0 +1,160 @@ +/** + * The contract that matters here is negative: a source that disappears must + * NOT be able to end the process (#1128). + * + * `fs.createReadStream` is lazy, so its ENOENT lands on a later tick, outside + * the route's try/catch. An EventEmitter emitting 'error' with no listener + * throws, and an uncaught throw from an I/O callback exits Node — which is how + * one missing thumbnail tier took every gallery on the install down. + * + * These use a REAL fs stream over a real missing path rather than a fake + * emitter: the point under test is the lazy-open timing, and a hand-rolled + * mock that emits synchronously would pass while proving nothing. + */ + +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { Readable } = require('stream'); +const { EventEmitter } = require('events'); + +const { pipeStreamToResponse } = require('../../src/utils/streamResponse'); + +jest.mock('../../src/utils/logger', () => ({ + warn: jest.fn(), error: jest.fn(), info: jest.fn(), debug: jest.fn(), +})); + +/** Minimal Express-ish response that records what happened to it. */ +function makeRes() { + const res = new EventEmitter(); + res.headers = { 'Content-Length': '1234', ETag: '"x"' }; + res.statusCode = 200; + res.headersSent = false; + res.writableEnded = false; + res.body = null; + res.destroyed = false; + res.removeHeader = (h) => { delete res.headers[h]; }; + res.setHeader = (h, v) => { res.headers[h] = v; }; + res.status = (code) => { res.statusCode = code; return res; }; + res.json = (payload) => { res.body = payload; res.writableEnded = true; return res; }; + res.destroy = () => { res.destroyed = true; }; + // pipe() target surface + res.write = () => true; + res.end = () => { res.writableEnded = true; }; + res.on = EventEmitter.prototype.on.bind(res); + res.emit = EventEmitter.prototype.emit.bind(res); + return res; +} + +const settle = () => new Promise((resolve) => setTimeout(resolve, 50)); + +describe('pipeStreamToResponse (#1128)', () => { + it('turns a missing file into a 404 instead of an unhandled error', async () => { + const missing = path.join(os.tmpdir(), `picpeak-not-here-${Date.now()}.jpg`); + const res = makeRes(); + + pipeStreamToResponse(stream_(missing), res, { context: 'thumbnail for photo 1' }); + await settle(); + + expect(res.statusCode).toBe(404); + expect(res.body).toEqual({ error: 'File not found' }); + }); + + // How this test discriminates, since the failure mode is a process-level + // one: replacing the call above with a bare `stream.pipe(res)` — what the + // thumbnail route did — makes jest fail this suite on the unhandled 'error' + // event before either assertion runs. Verified by doing exactly that. + // Catching the throw with a process.on('uncaughtException') listener does + // NOT work here and would be theatre: the runner installs its own handling, + // so such a listener never sees it and the assertion could never fail. + function stream_(p) { return fs.createReadStream(p); } + + it('strips every header that described the file it can no longer send', async () => { + const res = makeRes(); + // What the image and zip routes actually stage before streaming. + res.headers = { + 'Content-Length': '1234', + ETag: '"x"', + 'Content-Type': 'image/jpeg', + 'Content-Disposition': 'attachment; filename="gallery.zip"', + 'Cache-Control': 'private, max-age=1800', + }; + const stream = fs.createReadStream(path.join(os.tmpdir(), `gone-${Date.now()}.jpg`)); + + pipeStreamToResponse(stream, res); + await settle(); + + expect(res.headers['Content-Length']).toBeUndefined(); + expect(res.headers.ETag).toBeUndefined(); + // Express does NOT overwrite an existing Content-Type, so leaving it makes + // res.json() emit JSON labelled image/jpeg — or a corrupt .zip download. + expect(res.headers['Content-Type']).toBeUndefined(); + expect(res.headers['Content-Disposition']).toBeUndefined(); + }); + + it('does not let a transient 404 be cached as a broken tile', async () => { + const res = makeRes(); + // The thumbnail route stages 30 minutes; the hero route an hour. + res.headers = { 'Cache-Control': 'private, max-age=1800' }; + const stream = fs.createReadStream(path.join(os.tmpdir(), `gone3-${Date.now()}.jpg`)); + + pipeStreamToResponse(stream, res); + await settle(); + + // The regeneration race is transient by definition: the tier exists moments + // later. Caching this 404 would keep the tile broken long after the file is + // back — the opposite of what this helper is for. + expect(res.headers['Cache-Control']).toBe('no-store'); + }); + + it('honours a caller that wants a different missing-status', async () => { + const res = makeRes(); + const stream = fs.createReadStream(path.join(os.tmpdir(), `gone2-${Date.now()}.zip`)); + + pipeStreamToResponse(stream, res, { missingStatus: 410 }); + await settle(); + + expect(res.statusCode).toBe(410); + }); + + it('destroys the response instead of rewriting a status that is already sent', async () => { + const res = makeRes(); + res.headersSent = true; + + const stream = new Readable({ read() {} }); + pipeStreamToResponse(stream, res, { context: 'photo 9' }); + stream.emit('error', Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + await settle(); + + // Once bytes are on the wire a 404 is not available; a truncated image the + // client would cache is worse than a broken connection. + expect(res.destroyed).toBe(true); + expect(res.statusCode).toBe(200); + expect(res.body).toBeNull(); + }); + + it('reports a non-ENOENT failure as a 500 rather than a 404', async () => { + const res = makeRes(); + const stream = new Readable({ read() {} }); + + pipeStreamToResponse(stream, res); + stream.emit('error', Object.assign(new Error('disk exploded'), { code: 'EIO' })); + await settle(); + + expect(res.statusCode).toBe(500); + expect(res.body).toEqual({ error: 'Failed to serve file' }); + }); + + it('releases the source when the client hangs up mid-download', async () => { + const res = makeRes(); + let destroyed = false; + const stream = new Readable({ read() {}, destroy(err, cb) { destroyed = true; cb(err); } }); + + pipeStreamToResponse(stream, res); + res.emit('close'); + await settle(); + + // Otherwise an abandoned grid leaks one open fd per tile. + expect(destroyed).toBe(true); + }); +}); diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index b9977288..d66734ab 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -33,6 +33,7 @@ const { resolveGuest } = require('../middleware/guestAuth'); const { generateGuestIdentifier } = require('../middleware/feedbackRateLimit'); const secureImageService = require('../services/secureImageService'); const logger = require('../utils/logger'); +const { pipeStreamToResponse } = require('../utils/streamResponse'); const { resolvePhotoFilePath } = require('../services/photoResolver'); const { getEventCategoriesOrdered } = require('../utils/categoryOrder'); const { getEventShareToken, resolveShareIdentifier, buildShareLinkVariants } = require('../services/shareLinkService'); @@ -1462,7 +1463,7 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block res.setHeader('Content-Length', zipInfo.size); res.setHeader('Content-Disposition', `attachment; filename="${req.event.slug}.zip"`); const stream = await storage.get(zipInfo.key); - stream.pipe(res); + pipeStreamToResponse(stream, res, { context: `prepared zip for event ${req.event.id}`, missingStatus: 410 }); // Log bulk download (admin preview #868 excluded — stats stay client-only). if (!req.isAdminPreview) { @@ -1955,7 +1956,7 @@ router.get('/:slug/download-jobs/:token/file', verifyGalleryAccess, denySlidesho res.setHeader('Content-Length', stat.size); res.setHeader('Content-Disposition', `attachment; filename="${req.event.slug}-${suffix}.zip"`); const stream = await storage.get(job.zip_path); - stream.pipe(res); + pipeStreamToResponse(stream, res, { context: `download job ${job.id}`, missingStatus: 410 }); } catch (error) { errorResponse(res, error, 500, 'Failed to serve prepared download'); } @@ -2122,7 +2123,7 @@ router.get('/:slug/photo/:photoId', const file = useStorageBackend ? await storage.getRange(storageKey, start, end) : fs.createReadStream(filePath, { start, end }); - file.pipe(res); + pipeStreamToResponse(file, res, { context: `video range for photo ${photo.id}` }); } else { res.writeHead(200, { 'Content-Length': fileSize, @@ -2134,7 +2135,7 @@ router.get('/:slug/photo/:photoId', const file = useStorageBackend ? await storage.get(storageKey) : fs.createReadStream(filePath); - file.pipe(res); + pipeStreamToResponse(file, res, { context: `video for photo ${photo.id}` }); } return; } @@ -2168,7 +2169,7 @@ router.get('/:slug/photo/:photoId', 'X-Protection-Level': 'basic' }); const wmStream = await storage.get(photo.watermark_path); - return wmStream.pipe(res); + return pipeStreamToResponse(wmStream, res, { context: `watermarked photo ${photo.id}` }); } } else { const watermarkFilePath = path.join(getStoragePath(), photo.watermark_path); @@ -2217,7 +2218,7 @@ router.get('/:slug/photo/:photoId', 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); + pipeStreamToResponse(stream, res, { context: `photo ${photo.id}` }); } else { const absolutePath = path.isAbsolute(filePath) ? filePath : path.resolve(filePath); res.sendFile(absolutePath); @@ -2335,7 +2336,7 @@ router.get('/:slug/thumbnail/:photoId', } else { res.setHeader('Content-Length', stat.size); const stream = await storage.get(thumbnailPath); - stream.pipe(res); + pipeStreamToResponse(stream, res, { context: `thumbnail for photo ${photoId}` }); } } catch (error) { errorResponse(res, error, 500, 'Failed to serve thumbnail'); @@ -2424,7 +2425,7 @@ router.get('/:slug/hero/:photoId', } else { res.setHeader('Content-Length', stat.size); const stream = await storage.get(heroPath); - stream.pipe(res); + pipeStreamToResponse(stream, res, { context: `hero for photo ${photoId}` }); } } catch (error) { logger.error('Error serving hero image:', { @@ -2534,7 +2535,7 @@ router.get('/:slug/preview/:photoId', } else { res.setHeader('Content-Length', stat.size); const stream = await storage.get(previewPath); - stream.pipe(res); + pipeStreamToResponse(stream, res, { context: `preview for photo ${photoId}` }); } } catch (error) { logger.error('Error serving preview image:', { diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 9e8a8842..c19b25b4 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -784,6 +784,12 @@ async function deleteThumbnailTiers(photo) { * served from a cache hit without re-reading the source, so an unscoped key * would hand one gallery's photo to another. */ +/** + * Tier storage key -> the in-flight generation for it (#1128). Module scope so + * every concurrent request for one tile shares a single Sharp pass. + */ +const inFlightThumbnailTiers = new Map(); + async function ensureThumbnailAtWidth(photo, width) { if (!width) return ensureThumbnail(photo); @@ -835,28 +841,60 @@ async function ensureThumbnailAtWidth(photo, width) { // would visibly reframe as the tile size changes. const height = Math.round(width * (settings.height / canonicalWidth)); - try { - if (isExternal) { - const localPath = resolvePhotoFilePath(event, photo); - return await generateThumbnail(localPath, { - regenerate: true, outputBasename, width, height, - }); - } - const sourceKey = resolvePhotoStorageKey(event, photo); - if (!sourceKey) return null; - return await withLocalCopy(sourceKey, async (localPath) => { - const proc = await withProcessableImage(localPath, sourceKey); - try { - return await generateThumbnail(proc.path, { - regenerate: true, outputBasename, width, height, - }); - } finally { - proc.cleanup(); + // One generation per tier key, however many tiles ask for it (#1128). + // + // A grid issues one request per tile simultaneously, and on a cold gallery + // every one of them misses the stat above. Without this each would run its + // own Sharp pass over the same source — and for an external photo, re-read + // the whole original off the NFS mount to do it. 79 tiles meant 79 decodes + // of the same file, which is also what made the delete race easy to hit. + // + // Per-process only. Two pods still generate independently, which is + // harmless: the write ends in an atomic rename, so they converge on + // byte-identical output. + const pending = inFlightThumbnailTiers.get(key); + if (pending) return pending; + + const work = (async () => { + try { + // NOT `regenerate: true` (#1128). This path is only reached on a cache + // MISS, so there is nothing to regenerate — but that flag makes + // generateThumbnail open by DELETING the target. Request A publishes the + // tier, B stats it and heads for storage.get(), and C — still inside + // generation from its own earlier miss — unlinks the file B is about to + // open. B's lazy ReadStream then raised an ENOENT nothing was listening + // for and Node exited. + // + // Without the flag the write is a plain put: LocalFsStorage stages to a + // temp file and renames, which is atomic, so a concurrent reader sees + // either the old file or the new one and never a hole. + if (isExternal) { + const localPath = resolvePhotoFilePath(event, photo); + return await generateThumbnail(localPath, { outputBasename, width, height }); } - }); - } catch (e) { - logger.warn(`Thumbnail tier w${width} failed for photo ${photo.id}: ${e.message}`); - return null; + const sourceKey = resolvePhotoStorageKey(event, photo); + if (!sourceKey) return null; + return await withLocalCopy(sourceKey, async (localPath) => { + const proc = await withProcessableImage(localPath, sourceKey); + try { + return await generateThumbnail(proc.path, { outputBasename, width, height }); + } finally { + proc.cleanup(); + } + }); + } catch (e) { + logger.warn(`Thumbnail tier w${width} failed for photo ${photo.id}: ${e.message}`); + return null; + } + })(); + + inFlightThumbnailTiers.set(key, work); + try { + return await work; + } finally { + // In a finally so a rejection cannot poison the key for the process + // lifetime — the next request re-attempts rather than adopting a failure. + inFlightThumbnailTiers.delete(key); } } diff --git a/backend/src/utils/streamResponse.js b/backend/src/utils/streamResponse.js new file mode 100644 index 00000000..2db6b528 --- /dev/null +++ b/backend/src/utils/streamResponse.js @@ -0,0 +1,81 @@ +/** + * Pipe a file/storage stream to an Express response without betting the + * process on the source still being there (#1128). + * + * `fs.createReadStream` — what LocalFsStorage.get() returns — is LAZY. It + * resolves immediately and only opens the file on a later tick, so an ENOENT + * arrives AFTER the `await` returned and outside the route's try/catch. An + * EventEmitter that emits 'error' with no listener throws, and an uncaught + * throw from an I/O callback is not something Express can catch: Node exits. + * + * That is how one missing thumbnail tier took down every gallery on the + * install — the process died on the first grid load and only came back + * because Docker restarted it. + * + * The window is real and cannot be closed by a stat() beforehand: between the + * stat and the open, another request regenerating the same derivative can + * unlink it. So the handler is the fix, not the preflight. + */ + +const logger = require('./logger'); + +/** + * @param {import('stream').Readable} stream source, already opened or lazy + * @param {import('express').Response} res + * @param {object} [options] + * @param {string} [options.context] what was being served, for the log line + * @param {number} [options.missingStatus=404] status when the source is gone + */ +function pipeStreamToResponse(stream, res, options = {}) { + const { context = 'file', missingStatus = 404 } = options; + + stream.on('error', (err) => { + const gone = err && (err.code === 'ENOENT' || err.code === 'EISDIR'); + + // Once bytes are on the wire the status line is spent — there is no way to + // turn this into a 404. Destroy the response so the client sees a broken + // connection rather than a silently truncated image it would cache. + if (res.headersSent) { + logger.warn(`Stream failed mid-response for ${context}: ${err.message}`); + res.destroy(err); + return; + } + + // Every header staged for the FILE now describes a body that will never + // be sent. They are cleared rather than left to Express, which does not + // overwrite a Content-Type that is already set — so without this the JSON + // error goes out as `image/jpeg`, or as an `application/zip` attachment + // that saves to disk as a corrupt download. + // + // Cache-Control matters most. The image routes stage `max-age=1800` (the + // hero route 3600), so a 404 from the regeneration race — the transient + // case this whole helper exists for — would be cached as a broken tile for + // up to an hour after the tier finished generating. + res.removeHeader('Content-Length'); + res.removeHeader('ETag'); + res.removeHeader('Content-Type'); + res.removeHeader('Content-Disposition'); + res.setHeader('Cache-Control', 'no-store'); + + if (gone) { + // Expected under the regeneration race — the tier existed at stat time + // and was replaced before the open. One broken tile, not an outage. + logger.warn(`Source vanished while serving ${context}: ${err.message}`); + res.status(missingStatus).json({ error: 'File not found' }); + return; + } + + logger.error(`Failed to stream ${context}`, { error: err.message, code: err.code }); + res.status(500).json({ error: 'Failed to serve file' }); + }); + + // A client that navigates away mid-download leaves the source handle open + // otherwise; on a gallery grid that is one leaked fd per abandoned tile. + res.on('close', () => { + if (!res.writableEnded) stream.destroy(); + }); + + stream.pipe(res); +} + +module.exports = { pipeStreamToResponse };