fix(gallery): a missing thumbnail tier must not take the backend down (#1128)
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.
This commit is contained in:
Paul Nothaft
@@ -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:
|
||||
// `<key>.tmp.<pid>.<hex>`, 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_');
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user