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 e2d8c331..914486b9 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -29,6 +29,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 { getEventShareToken, resolveShareIdentifier, buildShareLinkVariants } = require('../services/shareLinkService'); const { handleAsync, errorResponse } = require('../utils/routeHelpers'); @@ -1044,7 +1045,7 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async 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 db('access_logs').insert({ @@ -1505,7 +1506,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, @@ -1517,7 +1518,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; } @@ -1551,7 +1552,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); @@ -1600,7 +1601,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); @@ -1695,7 +1696,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'); @@ -1781,7 +1782,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:', { @@ -1877,7 +1878,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/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 };