fix(gallery): cap how many cached zips rebuild at once in the background (#1418)

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 <[email protected]>
2026-09-11 13:36:27 +02:00
parent 2a04137a75
commit 98d25601b4
2 changed files with 174 additions and 1 deletions
@@ -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);
+  });
+});