From 98d25601b465ecc44d0875b52891073720fabbbc Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:36:27 +0200 Subject: [PATCH] fix(gallery): cap how many cached zips rebuild at once in the background (#1418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit invalidateAll() invalidates every event holding a cached zip, and each invalidate() arms its own debounce timer in the same tick — so they all fired together and every one of them started building at once. Each build opens its own storage reads, so flipping a global setting across 25 events was enough to exhaust the S3 agent pool and stall uploads, thumbnails and gallery reads until the burst drained. Nothing capped it. Background rebuilds now run two at a time. The cap is deliberately only on that path: a foreground generateZip — a guest actually waiting on a download — is never queued behind a settings-change burst, which would trade one stall for another. stop() drains anything parked for a slot, so shutdown cannot hang on a queue that will never move. This is the second half of the problem. The first half — an individual build opening one unbounded read per photo — is the storage-read guard that already landed for the guest download routes and the job builder; the cached-zip builder's own copy is still in an open PR. Relates to issue 1399 Co-authored-by: Paul Nothaft --- .../downloadZipRegenConcurrency.test.js | 128 ++++++++++++++++++ backend/src/services/downloadZipService.js | 47 ++++++- 2 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/services/downloadZipRegenConcurrency.test.js diff --git a/backend/__tests__/services/downloadZipRegenConcurrency.test.js b/backend/__tests__/services/downloadZipRegenConcurrency.test.js new file mode 100644 index 00000000..be8b46e1 --- /dev/null +++ b/backend/__tests__/services/downloadZipRegenConcurrency.test.js @@ -0,0 +1,128 @@ +/** + * Background zip rebuilds are capped (#1399). + * + * invalidateAll() invalidates every event holding a cached zip, and each + * invalidate() arms its own debounce timer in the same tick — so they all fire + * together. Every build opens its own storage reads, so a settings change + * across 25 events was enough to exhaust the S3 agent pool and stall uploads, + * thumbnails and gallery reads until the burst drained. + * + * The cap is on the BACKGROUND path only: a guest waiting on a download must + * not be queued behind a settings-change burst. + */ +jest.mock('../../src/database/db', () => ({ db: jest.fn() })); +jest.mock('../../src/utils/logger', () => ({ + info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(), +})); + +const { db } = require('../../src/database/db'); +const service = require('../../src/services/downloadZipService'); + +const flush = () => new Promise((r) => setImmediate(r)); + +describe('downloadZipService background regen concurrency (#1399)', () => { + let peak; + let inFlight; + let release; + + beforeEach(() => { + // setImmediate must stay real: the flush() helper below rides on it, and + // jest's modern fake timers mock it too. + jest.useFakeTimers({ doNotFake: ['setImmediate'] }); + peak = 0; + inFlight = 0; + release = []; + service.stopped = false; + service.regenActive = 0; + service.regenWaiters = []; + service.debounceTimers.clear(); + service.activeBuilds.clear(); + + jest.spyOn(service, 'generateZip').mockImplementation(() => { + inFlight += 1; + peak = Math.max(peak, inFlight); + return new Promise((resolve) => { + release.push(() => { inFlight -= 1; resolve(); }); + }); + }); + jest.spyOn(service, '_cleanup').mockResolvedValue(undefined); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.restoreAllMocks(); + }); + + it('never runs more than two rebuilds at once, however many fire together', async () => { + const rows = Array.from({ length: 12 }, (_, i) => ({ id: i + 1 })); + db.mockReturnValue({ + whereNotNull: () => ({ select: () => Promise.resolve(rows) }), + }); + + await service.invalidateAll(); + // Every debounce timer was armed in the same tick — fire them all. + jest.runAllTimers(); + await flush(); + + expect(peak).toBe(2); + expect(service.generateZip).toHaveBeenCalledTimes(2); + }); + + it('starts the next rebuild as each one finishes', async () => { + const rows = Array.from({ length: 5 }, (_, i) => ({ id: i + 1 })); + db.mockReturnValue({ + whereNotNull: () => ({ select: () => Promise.resolve(rows) }), + }); + + await service.invalidateAll(); + jest.runAllTimers(); + await flush(); + expect(service.generateZip).toHaveBeenCalledTimes(2); + + release.shift()(); + await flush(); + expect(service.generateZip).toHaveBeenCalledTimes(3); + expect(peak).toBe(2); + + while (release.length) { release.shift()(); await flush(); } + expect(service.generateZip).toHaveBeenCalledTimes(5); + expect(peak).toBe(2); + }); + + it('does not queue a foreground download behind the burst', async () => { + const rows = Array.from({ length: 6 }, (_, i) => ({ id: i + 1 })); + db.mockReturnValue({ + whereNotNull: () => ({ select: () => Promise.resolve(rows) }), + }); + + await service.invalidateAll(); + jest.runAllTimers(); + await flush(); + expect(service.generateZip).toHaveBeenCalledTimes(2); + + // A guest asking for a zip right now calls generateZip directly. It must + // not park behind the two rebuilds already holding the slots. + service.generateZip(999); + await flush(); + expect(service.generateZip).toHaveBeenCalledWith(999); + expect(inFlight).toBe(3); + }); + + it('releases anything parked for a slot on shutdown', async () => { + const rows = Array.from({ length: 6 }, (_, i) => ({ id: i + 1 })); + db.mockReturnValue({ + whereNotNull: () => ({ select: () => Promise.resolve(rows) }), + }); + + await service.invalidateAll(); + jest.runAllTimers(); + await flush(); + expect(service.regenWaiters.length).toBeGreaterThan(0); + + // stop() must not hang on a queue that will never drain. + const stopping = service.stop(); + release.forEach((fn) => fn()); + await expect(stopping).resolves.toBeUndefined(); + expect(service.regenWaiters).toHaveLength(0); + }); +}); diff --git a/backend/src/services/downloadZipService.js b/backend/src/services/downloadZipService.js index cd5d7e31..b798a3dc 100644 --- a/backend/src/services/downloadZipService.js +++ b/backend/src/services/downloadZipService.js @@ -38,6 +38,18 @@ const DEBOUNCE_MS = 5000; // Two keeps the next photo's round trip overlapped with the current write // without ever leaving more than one socket idle. const MAX_INFLIGHT_READS = 2; +// How many cached zips may be REBUILT at once in the background (#1399). +// +// invalidateAll() invalidates every event that has a cached zip, and each +// invalidate() arms its own debounce timer in the same tick — so they all fire +// together and, before this, every one of them started building at once. Each +// build opens its own storage reads, so 25 events was enough to exhaust the S3 +// agent pool and stall uploads, thumbnails and gallery reads until the burst +// finished. +// +// This caps the BACKGROUND path only. A foreground generateZip() — a guest +// actually waiting for a download — is never queued behind a rebuild. +const MAX_CONCURRENT_REGENS = 2; class DownloadZipService { constructor() { @@ -45,11 +57,42 @@ class DownloadZipService { this.debounceTimers = new Map(); // eventId -> setTimeout handle this.versions = new Map(); // eventId -> generation counter this.buildCancellers = new Map(); // eventId -> abort the in-flight build + this.regenActive = 0; // background rebuilds running right now + this.regenWaiters = []; // resolvers parked waiting for a slot + this.stopped = false; + } + + /** + * Run a BACKGROUND rebuild under the concurrency cap (#1399). Foreground + * callers deliberately do not go through here: someone is waiting on that + * response, and making them queue behind a settings-change burst would trade + * one stall for another. + */ + async _withRegenSlot(fn) { + if (this.stopped) return undefined; + if (this.regenActive >= MAX_CONCURRENT_REGENS) { + await new Promise((resolve) => this.regenWaiters.push(resolve)); + // Shutdown can drain the queue while we were parked. + if (this.stopped) return undefined; + } + this.regenActive += 1; + try { + return await fn(); + } finally { + this.regenActive -= 1; + const next = this.regenWaiters.shift(); + if (next) next(); + } } async stop() { + this.stopped = true; for (const timer of this.debounceTimers.values()) clearTimeout(timer); this.debounceTimers.clear(); + // Release anything parked for a slot so shutdown can't hang on a queue + // that will never drain — they check `stopped` and return without building. + const waiters = this.regenWaiters.splice(0); + for (const resume of waiters) resume(); await Promise.allSettled([...this.activeBuilds.values()].map(build => build.promise)); this.versions.clear(); this.buildCancellers.clear(); @@ -364,7 +407,9 @@ class DownloadZipService { // Debounce regeneration const newTimer = setTimeout(() => { this.debounceTimers.delete(eventId); - this.generateZip(eventId).catch(err => + // Through the cap (#1399): invalidateAll arms every one of these in the + // same tick, so without it they all start building together. + this._withRegenSlot(() => this.generateZip(eventId)).catch(err => logger.warn('downloadZipService debounced regen error', { eventId, error: err.message }) ); }, DEBOUNCE_MS);