diff --git a/backend/__tests__/utils/archiveStreamGuard.test.js b/backend/__tests__/utils/archiveStreamGuard.test.js new file mode 100644 index 00000000..719a7f4e --- /dev/null +++ b/backend/__tests__/utils/archiveStreamGuard.test.js @@ -0,0 +1,122 @@ +/** + * Bounded, reclaimable storage reads for archiver downloads (#1399 follow-up). + * + * archiver drains the sources it is handed one at a time, so appending a + * storage read per photo opens N and drains one. Every other read parks its + * socket holding unread bytes, and nothing reclaims them: archiver's abort() + * does not touch source streams, and the S3 SDK clears its socket timeout as + * soon as response headers land. That is the mechanism behind the incident in + * PR #1402 — 43 of 50 pooled sockets held, uploads starved, restart required. + * + * #1402 fixes the cached-zip builder. These are the guarantees the same guard + * has to give the three remaining call sites, two of which need no admin + * credentials to reach. + */ +const { Readable } = require('stream'); +const { createArchiveStreamGuard } = require('../../src/utils/archiveStreamGuard'); + +const makeStream = () => new Readable({ read() {} }); + +describe('archiveStreamGuard (#1399 follow-up)', () => { + it('lets the configured number of reads run at once', async () => { + const guard = createArchiveStreamGuard({ maxInFlight: 2 }); + expect(await guard.acquire()).toBe(true); + guard.track(makeStream()); + expect(await guard.acquire()).toBe(true); + guard.track(makeStream()); + expect(guard.openCount).toBe(2); + }); + + it('parks the next acquire until a read finishes', async () => { + const guard = createArchiveStreamGuard({ maxInFlight: 1 }); + await guard.acquire(); + const first = guard.track(makeStream()); + + let resumed = false; + const pending = guard.acquire().then((ok) => { resumed = ok; }); + + await new Promise((r) => setImmediate(r)); + expect(resumed).toBe(false); // still parked — this is the cap doing its job + + first.push(null); + first.resume(); + await pending; + expect(resumed).toBe(true); + }); + + it('releases a slot when a read errors, not just when it ends', async () => { + const guard = createArchiveStreamGuard({ maxInFlight: 1 }); + await guard.acquire(); + const stream = guard.track(makeStream()); + stream.on('error', () => {}); + stream.destroy(new Error('socket died')); + // Without the error listener the slot would never come back and the next + // photo would park forever. + expect(await guard.acquire()).toBe(true); + }); + + it('reports a failed read so the caller can abort the archive', async () => { + // A stream that errors while still QUEUED has no archiver listener on it + // yet. Releasing its slot and saying nothing leaves a dead stream in the + // queue, and the archive hangs when it reaches it. + const seen = []; + const guard = createArchiveStreamGuard({ maxInFlight: 2, onFatalError: (e) => seen.push(e) }); + await guard.acquire(); + const queued = guard.track(makeStream()); + queued.on('error', () => {}); + queued.destroy(new Error('socket died')); + await new Promise((r) => setImmediate(r)); // 'error' lands on the next tick + expect(seen).toHaveLength(1); + expect(seen[0].message).toBe('socket died'); + }); + + it('stays quiet about reads it destroyed itself', async () => { + // destroyAll is the caller's own teardown; reporting those back as fatal + // would re-enter the abort path it is already running. + const seen = []; + const guard = createArchiveStreamGuard({ onFatalError: (e) => seen.push(e) }); + await guard.acquire(); + const s1 = guard.track(makeStream()); + s1.on('error', () => {}); + guard.destroyAll(); + await new Promise((r) => setImmediate(r)); + expect(seen).toHaveLength(0); + }); + + it('destroys every read still holding bytes', async () => { + const guard = createArchiveStreamGuard({ maxInFlight: 5 }); + const streams = [makeStream(), makeStream(), makeStream()]; + for (const s of streams) { await guard.acquire(); guard.track(s); } + expect(guard.openCount).toBe(3); + + guard.destroyAll(); + expect(streams.every((s) => s.destroyed)).toBe(true); + expect(guard.openCount).toBe(0); + }); + + it('wakes a parked acquire on destroyAll so the loop can exit', async () => { + const guard = createArchiveStreamGuard({ maxInFlight: 1 }); + await guard.acquire(); + guard.track(makeStream()); + + const pending = guard.acquire(); + guard.destroyAll(); + // false, so the caller breaks out instead of appending to a dead archive. + expect(await pending).toBe(false); + }); + + it('destroys a stream tracked after shutdown rather than leaking it', () => { + const guard = createArchiveStreamGuard(); + guard.destroyAll(); + const late = guard.track(makeStream()); + expect(late.destroyed).toBe(true); + expect(guard.openCount).toBe(0); + }); + + it('tolerates destroyAll twice — exit paths overlap', () => { + const guard = createArchiveStreamGuard(); + guard.track(makeStream()); + guard.destroyAll(); + expect(() => guard.destroyAll()).not.toThrow(); + }); +}); diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 30bf0fe1..82b4c1e7 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -46,6 +46,7 @@ const { } = require('../services/downloadFilenameService'); const { buildContentDisposition } = require('../utils/filenameSanitizer'); const { getStorage } = require('../services/storage'); +const { createArchiveStreamGuard } = require('../utils/archiveStreamGuard'); const { setGalleryAuthCookies } = require('../utils/tokenUtils'); // Read globals from app_settings (the real table) — settingsService.getSetting // queries a non-existent `settings` table and throws. @@ -1326,6 +1327,10 @@ async function bumpEventDownloadCounts(eventId) { } router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async (req, res) => { + // Hoisted so the catch can reclaim reads opened before the failure, and so a + // cancelled download never reaches finalize() (see the close handler below). + let guard = null; + let cancelled = false; try { // Check if downloads are allowed for this event if (!parseBooleanInput(req.event.allow_downloads, true)) { @@ -1440,6 +1445,22 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async throw err; }); + // Bound and reclaim the storage reads. archiver drains its sources one at + // a time, so appending one read per photo parks an S3 socket per photo + // holding unread bytes, and nothing reclaims them — archiver's abort() + // does not touch source streams, and the SDK clears its socket timeout as + // soon as response headers land. + guard = createArchiveStreamGuard({ + onFatalError: () => { cancelled = true; guard.destroyAll(); archive.abort(); }, + }); + res.on('close', () => { + if (!res.writableFinished) { + cancelled = true; + guard.destroyAll(); + archive.abort(); + } + }); + archive.pipe(res); // Get watermark settings - apply if global setting OR event-level setting is enabled @@ -1508,8 +1529,9 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async archive.append(watermarkedBuffer, { name: archiveName }); } else if (storageKey) { + if (!await guard.acquire()) break; const stream = await storage.get(storageKey); - archive.append(stream, { name: archiveName }); + archive.append(guard.track(stream), { name: archiveName }); } else { archive.file(resolvePhotoFilePath(req.event, photo), { name: archiveName }); } @@ -1524,6 +1546,7 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async } } + if (cancelled) return; await archive.finalize(); // Log bulk download @@ -1540,12 +1563,19 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async .increment('download_count', 1).catch(() => {}); } } catch (error) { + if (guard) guard.destroyAll(); + // The client already left and the ZIP headers are gone; sending JSON here + // throws ERR_HTTP_HEADERS_SENT out of an async handler with nothing to + // catch it. + if (cancelled || res.headersSent) return; errorResponse(res, error, 500, 'Failed to create download archive'); } }); // Download selected photos as ZIP router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken, async (req, res) => { + let selectedGuard = null; + let selectedCancelled = false; try { // Check if downloads are allowed for this event if (!parseBooleanInput(req.event.allow_downloads, true)) { @@ -1605,6 +1635,19 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken, // ignore double-send errors } }); + + selectedGuard = createArchiveStreamGuard({ + onFatalError: () => { selectedCancelled = true; selectedGuard.destroyAll(); archive.abort(); }, + }); + archive.on('error', () => selectedGuard.destroyAll()); + res.on('close', () => { + if (!res.writableFinished) { + selectedCancelled = true; + selectedGuard.destroyAll(); + archive.abort(); + } + }); + archive.pipe(res); // Check watermark settings - apply if global setting OR event-level setting is enabled @@ -1651,8 +1694,9 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken, : await watermarkService.applyWatermark(resolvePhotoFilePath(req.event, photo), effectiveSettings); archive.append(buf, { name }); } else if (storageKey) { + if (!await selectedGuard.acquire()) break; const stream = await selectedStorage.get(storageKey); - archive.append(stream, { name }); + archive.append(selectedGuard.track(stream), { name }); } else { archive.file(resolvePhotoFilePath(req.event, photo), { name }); } @@ -1667,6 +1711,7 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken, } } + if (selectedCancelled) return; await archive.finalize(); await db('access_logs').insert({ @@ -1682,6 +1727,8 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken, .increment('download_count', 1).catch(() => {}); } } catch (error) { + if (selectedGuard) selectedGuard.destroyAll(); + if (selectedCancelled || res.headersSent) return; errorResponse(res, error, 500, 'Failed to download selected photos'); } }); diff --git a/backend/src/utils/archiveStreamGuard.js b/backend/src/utils/archiveStreamGuard.js new file mode 100644 index 00000000..9e3af50e --- /dev/null +++ b/backend/src/utils/archiveStreamGuard.js @@ -0,0 +1,100 @@ +/** + * Bounded, reclaimable storage reads for archiver-based downloads. + * + * archiver consumes the sources it is handed one at a time. Appending a + * storage read per photo in a tight loop therefore opens N reads and drains + * one, and every other one parks an S3 socket holding megabytes of unread + * body. Nothing reclaims them on its own: archiver's abort() does not touch + * its source streams, and the SDK arms its socket timeout on a 3s delay and + * clears it the moment response headers land, so a fast response never gets + * one at all. + * + * That is the shape of the incident in PR #1402 — 43 of 50 pooled sockets + * ESTABLISHED with unread bytes, uploads and gallery reads starved behind + * them, a process restart the only way out. #1402 fixes the cached-zip + * builder. This is the same guard for the other three call sites, two of + * which a gallery guest can reach with no admin credentials at all. + * + * Local-filesystem installs are unaffected — they take archiver's + * `archive.file(path)` branch and open no sockets — which is most likely why + * this went unnoticed for so long. + */ + +// Two in flight: one being drained, one ready to go. Enough to keep archiver +// fed, few enough that a build cannot monopolise the agent pool. +const DEFAULT_MAX_IN_FLIGHT = 2; + +function createArchiveStreamGuard({ maxInFlight = DEFAULT_MAX_IN_FLIGHT, onFatalError } = {}) { + const openReads = new Set(); + let waiter = null; + let closed = false; + + const wake = () => { + if (!waiter) return; + const resume = waiter; + waiter = null; + resume(); + }; + + const release = (stream) => { + openReads.delete(stream); + wake(); + }; + + return { + /** Park until a read slot frees up. Returns false once destroyAll ran. */ + async acquire() { + while (!closed && openReads.size >= maxInFlight) { + await new Promise((resolve) => { waiter = resolve; }); + } + return !closed; + }, + + /** Register a stream and hand it straight back, for inline use. */ + track(stream) { + if (closed) { + stream.destroy(); + return stream; + } + openReads.add(stream); + stream.once('end', () => release(stream)); + stream.once('close', () => release(stream)); + stream.once('error', (err) => { + release(stream); + // A stream that errors while still QUEUED behind another has no + // archiver listener on it yet, so archiver never learns it failed. + // Absorbing the error here and leaving the dead stream in the queue + // makes the archive hang forever when it reaches it — and in + // downloadJobService the build keeps its slot with it. Hand the + // failure to the caller, which aborts the archive. + if (!closed && typeof onFatalError === 'function') { + onFatalError(err); + } + }); + return stream; + }, + + /** + * Destroy every read still holding bytes. Safe to call more than once — + * the exit paths overlap (client disconnect and an error can both fire). + */ + destroyAll() { + closed = true; + for (const stream of openReads) { + try { + stream.destroy(); + } catch { + // Already gone; nothing to reclaim. + } + } + openReads.clear(); + wake(); + }, + + get openCount() { + return openReads.size; + }, + }; +} + +module.exports = { createArchiveStreamGuard, DEFAULT_MAX_IN_FLIGHT };