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);