Paul Nothaft
18a3bb101d
PR 1402 fixes the cached-zip builder. The same unguarded pattern — one storage read appended per photo, archiver draining one at a time, nothing destroying the rest — is still present in three other places, two of which a gallery guest reaches with no admin credentials: - routes/gallery/downloads.js, download-all and download-selected - services/downloadJobService.js, the custom-resolution job builder, which is worse in one respect: its per-photo catch skips a bad source without ever destroying the stream it had already opened, so every skipped photo leaked a socket for the life of the process An unread S3 response body holds its socket open indefinitely — 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 — and archiver's abort() does not touch its source streams. That is the mechanism behind the incident described in PR 1402: 43 of 50 pooled sockets held with unread bytes, uploads and gallery reads starved behind them, a process restart the only way out. utils/archiveStreamGuard.js caps reads in flight at 2 and destroys whatever is still open on every exit: an error, a failed append, and — for the two guest routes — the client closing the tab mid-download, which previously left every appended-but-undrained read parked forever. Deliberately does not touch downloadZipService.js, so there is no conflict with 1402. Once that lands, its inline equivalent can move onto this helper. Local-filesystem installs are unaffected either way: they take archiver's archive.file(path) branch and open no sockets. Relates to issue 1399
95 lines
3.5 KiB
JavaScript
95 lines
3.5 KiB
JavaScript
/**
|
|
* 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('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();
|
|
});
|
|
});
|