From 72163f3c082196f922dcc9bd3f56c57b1df2e3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=8B=E5=BB=BA=E5=86=9B?= Date: Tue, 6 Oct 2026 14:51:13 +0800 Subject: [PATCH 1/2] fix: enforce the documented 20-PDF download limit per session The limit check ran before the current download joined the in-flight set, so `> 20` admitted a 21st saved PDF (and two concurrent 20th downloads), contradicting the user-facing '20 PDF download limit' message. Use `>=` like the other session/profile caps. Adds a unit test for capturePdfDownload's limit and happy paths; the per-session boundary itself needs the Chromium fixture suite. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/worker/src/browser.ts | 4 +- apps/worker/tests/downloads.test.ts | 64 +++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 apps/worker/tests/downloads.test.ts diff --git a/apps/worker/src/browser.ts b/apps/worker/src/browser.ts index 6c4f03044..9a44f1cd4 100644 --- a/apps/worker/src/browser.ts +++ b/apps/worker/src/browser.ts @@ -241,12 +241,14 @@ export async function createBrowserManager( void dialog.dismiss(); }); page.on("download", (download) => { + // The current download is not in pending yet, so reaching 20 saved or + // in-flight downloads means this one exceeds the documented 20-PDF limit. const pending = downloads(id).then((saved) => capturePdfDownload({ directory: directory(id), tempDirectory, download, - limitReached: saved.length + instance.pending.size > 20, + limitReached: saved.length + instance.pending.size >= 20, }), ); instance.pending.add(pending); diff --git a/apps/worker/tests/downloads.test.ts b/apps/worker/tests/downloads.test.ts new file mode 100644 index 000000000..9e7ee7cf7 --- /dev/null +++ b/apps/worker/tests/downloads.test.ts @@ -0,0 +1,64 @@ +import assert from "node:assert/strict"; +import { mkdtemp, readdir, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Readable } from "node:stream"; +import test from "node:test"; +import { capturePdfDownload } from "../src/downloads.ts"; + +test("a reached download limit cancels the PDF and records DOWNLOAD_LIMIT", async (t) => { + const directory = await mkdtemp(join(tmpdir(), "openmuse-downloads-")); + t.after(() => rm(directory, { recursive: true, force: true })); + let cancelled = false; + await capturePdfDownload({ + directory, + tempDirectory: join(directory, "tmp"), + limitReached: true, + download: { + suggestedFilename: () => "report.pdf", + createReadStream: () => { + throw new Error("a limited download must not be read"); + }, + cancel: async () => { + cancelled = true; + }, + delete: async () => {}, + }, + }); + assert.equal(cancelled, true, "the download is cancelled, not captured"); + const outcomes = join(directory, "download-outcomes"); + const files = await readdir(outcomes); + assert.equal(files.length, 1); + const outcome = JSON.parse(await readFile(join(outcomes, files[0]), "utf8")); + assert.equal(outcome.code, "DOWNLOAD_LIMIT"); + assert.match(outcome.message, /20 PDF download limit/); +}); + +test("a download under the limit is captured instead of cancelled", async (t) => { + const directory = await mkdtemp(join(tmpdir(), "openmuse-downloads-")); + t.after(() => rm(directory, { recursive: true, force: true })); + let cancelled = false; + await capturePdfDownload({ + directory, + tempDirectory: await mkdtemp(join(tmpdir(), "openmuse-tmp-")), + limitReached: false, + download: { + suggestedFilename: () => "report.pdf", + createReadStream: async () => Readable.from(Buffer.from("%PDF-1.4 fake")), + cancel: async () => { + cancelled = true; + }, + delete: async () => {}, + }, + }); + assert.equal(cancelled, false, "an allowed download must not be cancelled"); + const saved = await readdir(join(directory, "downloads")); + const metadata = JSON.parse( + await readFile( + join(directory, "downloads", saved.find((file) => file.endsWith(".json")) ?? ""), + "utf8", + ), + ); + assert.equal(metadata.name, "report.pdf"); + assert.equal(metadata.mimeType, "application/pdf"); +}); From 0d8a422fa49c61637a0e6808ca5e0b9387be365d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=8B=E5=BB=BA=E5=86=9B?= Date: Wed, 7 Oct 2026 20:43:01 +0800 Subject: [PATCH 2/2] fix: keep the 20th PDF allowed and cover the handler boundary The review's Chromium reproduction was right: the decision runs after the current download joins the pending set, so the original > 20 accounting already admits exactly 20. My >= change reduced the cap to 19. Restore > 20 behind an explicit downloadLimitReached helper and pin the boundary: twentieth allowed, twenty-first blocked, and concurrent downloads counting each other in the pending set. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/worker/src/browser.ts | 7 ++++--- apps/worker/src/downloads.ts | 8 ++++++++ apps/worker/tests/downloads.test.ts | 19 ++++++++++++++++++- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/apps/worker/src/browser.ts b/apps/worker/src/browser.ts index 9a44f1cd4..c10841706 100644 --- a/apps/worker/src/browser.ts +++ b/apps/worker/src/browser.ts @@ -3,6 +3,7 @@ import { join } from "node:path"; import type { BrowserContext, Page } from "playwright"; import { capturePdfDownload, + downloadLimitReached, MAX_DOWNLOAD_BYTES, type PdfDownload, readDownloadFailures, @@ -241,14 +242,14 @@ export async function createBrowserManager( void dialog.dismiss(); }); page.on("download", (download) => { - // The current download is not in pending yet, so reaching 20 saved or - // in-flight downloads means this one exceeds the documented 20-PDF limit. + // The current download is already in instance.pending when this runs, + // so the cap admits the 20th PDF and blocks the 21st. const pending = downloads(id).then((saved) => capturePdfDownload({ directory: directory(id), tempDirectory, download, - limitReached: saved.length + instance.pending.size >= 20, + limitReached: downloadLimitReached(saved.length, instance.pending.size), }), ); instance.pending.add(pending); diff --git a/apps/worker/src/downloads.ts b/apps/worker/src/downloads.ts index 134838d30..d9f2f0ad3 100644 --- a/apps/worker/src/downloads.ts +++ b/apps/worker/src/downloads.ts @@ -60,6 +60,14 @@ export async function readDownloadFailures(directory: string, recoverInterrupted return failures.sort((a, b) => b.createdAt.localeCompare(a.createdAt)); } +/** + * Whether a new download exceeds the documented 20-PDF per-session cap. + * pendingCount must include the download being decided: the session handler + * adds the current transfer to its pending set before this is evaluated. + */ +export function downloadLimitReached(savedCount: number, pendingCount: number): boolean { + return savedCount + pendingCount > 20; +} export async function capturePdfDownload(options: { directory: string; tempDirectory: string; diff --git a/apps/worker/tests/downloads.test.ts b/apps/worker/tests/downloads.test.ts index 9e7ee7cf7..89a6bdd65 100644 --- a/apps/worker/tests/downloads.test.ts +++ b/apps/worker/tests/downloads.test.ts @@ -4,7 +4,24 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { Readable } from "node:stream"; import test from "node:test"; -import { capturePdfDownload } from "../src/downloads.ts"; +import { capturePdfDownload, downloadLimitReached } from "../src/downloads.ts"; + +test("the session cap admits the twentieth PDF and blocks the twenty-first", () => { + // pendingCount includes the download being decided, matching the handler, + // which adds the current transfer to its pending set before evaluating. + assert.equal(downloadLimitReached(19, 1), false, "the twentieth PDF is allowed"); + assert.equal(downloadLimitReached(20, 1), true, "the twenty-first PDF is blocked"); + assert.equal(downloadLimitReached(0, 1), false); + assert.equal(downloadLimitReached(20, 0), false, "no pending download decides nothing"); +}); + +test("concurrent downloads at the boundary keep the cap at 20", () => { + // Both handlers add to the pending set before either decision evaluates, so + // a racing pair counts each other: at 19 saved both see 21 and are blocked. + assert.equal(downloadLimitReached(18, 2), false, "two racing at 18 saved: both admitted"); + assert.equal(downloadLimitReached(19, 2), true, "two racing at 19 saved: both blocked"); + assert.equal(downloadLimitReached(18, 3), true, "three racing at 18 saved: all blocked"); +}); test("a reached download limit cancels the PDF and records DOWNLOAD_LIMIT", async (t) => { const directory = await mkdtemp(join(tmpdir(), "openmuse-downloads-"));