From 4823b236a7958fb0c697feb36e5ab24dd1c8af85 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 15 Aug 2026 20:44:31 -0700 Subject: [PATCH] fix(browser): stop cancelled downloads from capturing later files (#124382) * fix(browser): cancel abandoned download waits * fix(browser): cancel in-flight download clicks --- .../browser/src/browser-tool.actions.ts | 3 + extensions/browser/src/browser-tool.ts | 1 + .../src/browser/client-actions-core.ts | 6 + .../pw-download-cancel.chromium.test.ts | 271 ++++++++++++++++++ .../src/browser/pw-download-capture.ts | 16 ++ .../src/browser/pw-tools-core.downloads.ts | 26 +- ...-core.waits-next-download-saves-it.test.ts | 37 +++ .../src/browser/routes/agent.act.download.ts | 6 +- ...-contract-form-layout-act-commands.test.ts | 34 +++ 9 files changed, 385 insertions(+), 15 deletions(-) create mode 100644 extensions/browser/src/browser/pw-download-cancel.chromium.test.ts diff --git a/extensions/browser/src/browser-tool.actions.ts b/extensions/browser/src/browser-tool.actions.ts index 89ae99c456f5..1bb6788dc5cb 100644 --- a/extensions/browser/src/browser-tool.actions.ts +++ b/extensions/browser/src/browser-tool.actions.ts @@ -395,6 +395,7 @@ export async function executeDownloadAction(params: { baseUrl?: string; profile?: string; proxyRequest: BrowserProxyRequest | null; + signal?: AbortSignal; onTabActivity?: (targetId: string | undefined) => void; }): Promise> { const { action, input, baseUrl, profile, proxyRequest } = params; @@ -419,12 +420,14 @@ export async function executeDownloadAction(params: { targetId, timeoutMs, profile, + signal: params.signal, }) : await browserToolActionDeps.browserWaitForDownload(baseUrl, { path: request.path, targetId, timeoutMs, profile, + signal: params.signal, }); params.onTabActivity?.(readStringValue((result as { targetId?: unknown }).targetId) ?? targetId); return formatBrowserExternalToolResult({ kind: "download", payload: result }); diff --git a/extensions/browser/src/browser-tool.ts b/extensions/browser/src/browser-tool.ts index a2cde3cea553..6bffa7beb28e 100644 --- a/extensions/browser/src/browser-tool.ts +++ b/extensions/browser/src/browser-tool.ts @@ -932,6 +932,7 @@ export function createBrowserTool(opts?: { baseUrl, profile, proxyRequest, + signal, onTabActivity: sessionTabs.touch, }); case "upload": { diff --git a/extensions/browser/src/browser/client-actions-core.ts b/extensions/browser/src/browser/client-actions-core.ts index 052ef3ab186b..9c2278f22eea 100644 --- a/extensions/browser/src/browser/client-actions-core.ts +++ b/extensions/browser/src/browser/client-actions-core.ts @@ -64,6 +64,7 @@ async function postDownloadRequest( body: Record, profile?: string, timeoutMs?: number, + signal?: AbortSignal, ): Promise { const q = buildProfileQuery(profile); return await fetchBrowserJson(withBaseUrl(baseUrl, `${route}${q}`), { @@ -71,6 +72,7 @@ async function postDownloadRequest( headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), timeoutMs: resolveBrowserOperationRequestTimeoutMs(timeoutMs), + signal, }); } @@ -160,6 +162,7 @@ export async function browserWaitForDownload( targetId?: string; timeoutMs?: number; profile?: string; + signal?: AbortSignal; }, ): Promise { return await postDownloadRequest( @@ -172,6 +175,7 @@ export async function browserWaitForDownload( }, opts.profile, opts.timeoutMs, + opts.signal, ); } @@ -184,6 +188,7 @@ export async function browserDownload( targetId?: string; timeoutMs?: number; profile?: string; + signal?: AbortSignal; }, ): Promise { return await postDownloadRequest( @@ -197,6 +202,7 @@ export async function browserDownload( }, opts.profile, opts.timeoutMs, + opts.signal, ); } diff --git a/extensions/browser/src/browser/pw-download-cancel.chromium.test.ts b/extensions/browser/src/browser/pw-download-cancel.chromium.test.ts new file mode 100644 index 000000000000..ca8c86a89657 --- /dev/null +++ b/extensions/browser/src/browser/pw-download-cancel.chromium.test.ts @@ -0,0 +1,271 @@ +import fs from "node:fs/promises"; +import { createServer, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test-support.js"; +import { playwrightCore } from "./playwright-core.runtime.js"; +import { ensurePageState } from "./pw-session-state.js"; +import { closePlaywrightBrowserConnection, getPageForTargetId } from "./pw-session.js"; +import { downloadViaPlaywright, waitForDownloadViaPlaywright } from "./pw-tools-core.downloads.js"; +import { getFreePort } from "./test-port.js"; + +const runChromiumProof = process.env.OPENCLAW_BROWSER_DOWNLOAD_E2E === "1"; +const tempDirs = useAutoCleanupTempDirTracker(afterEach); + +function listen(server: Server): Promise { + return new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + resolve((server.address() as AddressInfo).port); + }); + }); +} + +function closeServer(server: Server): Promise { + return new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); +} + +async function readTargetId(page: import("playwright-core").Page): Promise { + const session = await page.context().newCDPSession(page); + try { + const { targetInfo } = await session.send("Target.getTargetInfo"); + return targetInfo.targetId; + } finally { + await session.detach(); + } +} + +describe.runIf(runChromiumProof)("managed Chromium download cancellation", () => { + const cleanup: Array<() => Promise> = []; + + afterEach(async () => { + for (const dispose of cleanup.splice(0).toReversed()) { + await dispose(); + } + }); + + it("does not let a cancelled waiter capture and write a later download", async () => { + const rootDir = tempDirs.make("openclaw-download-cancel-"); + cleanup.push(async () => await fs.rm(rootDir, { recursive: true, force: true })); + + const abandonedPayload = Buffer.from("abandoned-click-download\n"); + const successorPayload = Buffer.from("successor-download\n"); + const downloadServer = createServer((request, response) => { + if (request.url === "/late.txt") { + response.writeHead(200, { + "content-disposition": 'attachment; filename="duplicate.txt"', + "content-length": String(abandonedPayload.byteLength), + "content-type": "text/plain", + }); + response.end(abandonedPayload); + return; + } + if (request.url === "/successor.txt") { + response.writeHead(200, { + "content-disposition": 'attachment; filename="duplicate.txt"', + "content-length": String(successorPayload.byteLength), + "content-type": "text/plain", + }); + response.end(successorPayload); + return; + } + response.writeHead(200, { "content-type": "text/html; charset=utf-8" }); + response.end( + '' + + 'Download', + ); + }); + const downloadPort = await listen(downloadServer); + cleanup.push(async () => await closeServer(downloadServer)); + + const cdpPort = await getFreePort(); + const profileDir = path.join(rootDir, "profile"); + const context = await playwrightCore.chromium.launchPersistentContext(profileDir, { + headless: true, + executablePath: process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, + args: [`--remote-debugging-port=${cdpPort}`], + }); + cleanup.push(async () => await context.close()); + const page = context.pages()[0] ?? (await context.newPage()); + await page.goto(`http://127.0.0.1:${downloadPort}/`); + + const cdpUrl = `http://127.0.0.1:${cdpPort}`; + const targetId = await readTargetId(page); + cleanup.push(async () => await closePlaywrightBrowserConnection({ cdpUrl })); + const outputRoot = path.join(rootDir, "downloads"); + const outputPath = path.join(outputRoot, "cancelled.txt"); + const controller = new AbortController(); + const wait = waitForDownloadViaPlaywright({ + cdpUrl, + targetId, + path: outputPath, + rootDir: outputRoot, + timeoutMs: 5_000, + signal: controller.signal, + }); + const outcome = wait.then( + (download) => ({ kind: "resolved" as const, download }), + (error: unknown) => ({ + kind: "rejected" as const, + message: error instanceof Error ? error.message : String(error), + }), + ); + + const controlledPage = await getPageForTargetId({ cdpUrl, targetId }); + await expect.poll(() => ensurePageState(controlledPage).downloadWaiterDepth).toBe(1); + controller.abort(new Error("request aborted")); + const afterAbort = await Promise.race([ + outcome, + new Promise<{ kind: "pending" }>((resolve) => { + setTimeout(() => resolve({ kind: "pending" }), 200); + }), + ]); + + const successorPath = path.join(outputRoot, "successor.txt"); + const successor = waitForDownloadViaPlaywright({ + cdpUrl, + targetId, + path: successorPath, + rootDir: outputRoot, + timeoutMs: 5_000, + }); + await expect.poll(() => ensurePageState(controlledPage).downloadWaiterDepth).toBe(1); + await page.locator("#download").click(); + const finalOutcome = await outcome; + const written = await fs.readFile(outputPath).catch(() => undefined); + const successorResult = await successor; + + expect({ + afterAbort, + finalOutcome, + written: written?.toString("utf8"), + successor: { + bytes: await fs.readFile(successorResult.path, "utf8"), + suggestedFilename: successorResult.suggestedFilename, + }, + }).toEqual({ + afterAbort: { kind: "rejected", message: "request aborted" }, + finalOutcome: { kind: "rejected", message: "request aborted" }, + written: undefined, + successor: { + bytes: successorPayload.toString("utf8"), + suggestedFilename: "duplicate.txt", + }, + }); + + const pageState = ensurePageState(controlledPage); + pageState.roleRefs = { e1: { role: "button", name: "Delayed Download" } }; + pageState.roleRefsMode = "role"; + await page.evaluate(() => { + setTimeout(() => { + const delayed = document.querySelector("#delayed"); + if (delayed) { + delayed.disabled = false; + } + }, 500); + }); + const sharedPath = path.join(outputRoot, "shared.txt"); + const clickController = new AbortController(); + const cancelledClick = downloadViaPlaywright({ + cdpUrl, + targetId, + ref: "e1", + path: sharedPath, + rootDir: outputRoot, + timeoutMs: 5_000, + signal: clickController.signal, + }); + const clickOutcome = cancelledClick.then( + () => ({ kind: "resolved" as const }), + (error: unknown) => ({ + kind: "rejected" as const, + message: error instanceof Error ? error.message : String(error), + }), + ); + await expect.poll(() => pageState.downloadWaiterDepth).toBe(1); + clickController.abort(new Error("click request aborted")); + const clickAfterAbort = await Promise.race([ + clickOutcome, + new Promise<{ kind: "pending" }>((resolve) => { + setTimeout(() => resolve({ kind: "pending" }), 200); + }), + ]); + expect(clickAfterAbort).toEqual({ kind: "rejected", message: "click request aborted" }); + expect(pageState.downloadWaiterDepth).toBe(0); + + const clickSuccessor = waitForDownloadViaPlaywright({ + cdpUrl, + targetId, + path: sharedPath, + rootDir: outputRoot, + timeoutMs: 5_000, + }); + await expect.poll(() => pageState.downloadWaiterDepth).toBe(1); + const beforeSuccessorClick = await Promise.race([ + clickSuccessor.then(() => "resolved" as const), + new Promise<"pending">((resolve) => { + setTimeout(() => resolve("pending"), 700); + }), + ]); + expect(beforeSuccessorClick).toBe("pending"); + await expect(fs.access(sharedPath)).rejects.toMatchObject({ code: "ENOENT" }); + await page.locator("#download").click(); + const clickSuccessorResult = await clickSuccessor; + await expect(fs.readFile(clickSuccessorResult.path, "utf8")).resolves.toBe( + successorPayload.toString("utf8"), + ); + + await closePlaywrightBrowserConnection({ cdpUrl }); + await context.close(); + + const restartedCdpPort = await getFreePort(); + const restartedContext = await playwrightCore.chromium.launchPersistentContext(profileDir, { + headless: true, + executablePath: process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, + args: [`--remote-debugging-port=${restartedCdpPort}`], + }); + cleanup.push(async () => await restartedContext.close()); + const restartedPage = restartedContext.pages()[0] ?? (await restartedContext.newPage()); + await restartedPage.goto(`http://127.0.0.1:${downloadPort}/`); + const restartedCdpUrl = `http://127.0.0.1:${restartedCdpPort}`; + const restartedTargetId = await readTargetId(restartedPage); + cleanup.push(async () => await closePlaywrightBrowserConnection({ cdpUrl: restartedCdpUrl })); + + const downloadAfterRestart = async () => { + const pending = waitForDownloadViaPlaywright({ + cdpUrl: restartedCdpUrl, + targetId: restartedTargetId, + rootDir: outputRoot, + timeoutMs: 5_000, + }); + const connected = await getPageForTargetId({ + cdpUrl: restartedCdpUrl, + targetId: restartedTargetId, + }); + await expect.poll(() => ensurePageState(connected).downloadWaiterDepth).toBe(1); + await restartedPage.locator("#download").click(); + return await pending; + }; + const firstDuplicate = await downloadAfterRestart(); + const secondDuplicate = await downloadAfterRestart(); + + expect(firstDuplicate.suggestedFilename).toBe("duplicate.txt"); + expect(secondDuplicate.suggestedFilename).toBe("duplicate.txt"); + expect(firstDuplicate.path).not.toBe(secondDuplicate.path); + await expect(fs.readFile(firstDuplicate.path, "utf8")).resolves.toBe( + successorPayload.toString("utf8"), + ); + await expect(fs.readFile(secondDuplicate.path, "utf8")).resolves.toBe( + successorPayload.toString("utf8"), + ); + expect((await fs.readdir(outputRoot)).some((name) => name.endsWith(".part"))).toBe(false); + + await closePlaywrightBrowserConnection({ cdpUrl: restartedCdpUrl }); + await restartedContext.close(); + await fs.rm(rootDir, { recursive: true, force: true }); + await expect(fs.access(rootDir)).rejects.toMatchObject({ code: "ENOENT" }); + }, 30_000); +}); diff --git a/extensions/browser/src/browser/pw-download-capture.ts b/extensions/browser/src/browser/pw-download-capture.ts index 83707a7d333e..cf31302127e9 100644 --- a/extensions/browser/src/browser/pw-download-capture.ts +++ b/extensions/browser/src/browser/pw-download-capture.ts @@ -16,6 +16,7 @@ export type BrowserDownloadCaptureOptions = { mode?: "passive" | "explicit"; outputPath?: string; outputRoot?: string; + signal?: AbortSignal; timeoutMessage?: string; }; @@ -85,6 +86,7 @@ export function createDownloadCaptureForPage( let depthReleased = false; let timer: NodeJS.Timeout | undefined; let handler: ((download: unknown) => void) | undefined; + let abort = () => {}; const cleanup = () => { if (!depthReleased) { @@ -99,6 +101,7 @@ export function createDownloadCaptureForPage( page.off("download", handler as never); handler = undefined; } + opts.signal?.removeEventListener("abort", abort); }; const promise = new Promise((resolve, reject) => { @@ -123,6 +126,19 @@ export function createDownloadCaptureForPage( Math.max(1, timeoutMs), ); timer.unref?.(); + abort = () => { + if (done) { + return; + } + done = true; + cleanup(); + const reason = opts.signal?.reason; + reject(reason instanceof Error ? reason : new Error("Download wait was cancelled")); + }; + opts.signal?.addEventListener("abort", abort, { once: true }); + if (opts.signal?.aborted) { + abort(); + } }); return { diff --git a/extensions/browser/src/browser/pw-tools-core.downloads.ts b/extensions/browser/src/browser/pw-tools-core.downloads.ts index 36fd86d30cb0..e11262ec48bb 100644 --- a/extensions/browser/src/browser/pw-tools-core.downloads.ts +++ b/extensions/browser/src/browser/pw-tools-core.downloads.ts @@ -49,6 +49,7 @@ function createExplicitDownloadCapture(params: { timeoutMs: number; outPath?: string; rootDir?: string; + signal?: AbortSignal; }) { params.state.armIdDownload = bumpDownloadArmId(); const armId = params.state.armIdDownload; @@ -56,6 +57,7 @@ function createExplicitDownloadCapture(params: { mode: "explicit", outputPath: params.outPath, outputRoot: params.rootDir, + signal: params.signal, beforeSave: () => { if (params.state.armIdDownload !== armId) { throw new Error("Download was superseded by another waiter"); @@ -374,6 +376,7 @@ export async function waitForDownloadViaPlaywright(opts: { targetId?: string; path?: string; rootDir?: string; + signal?: AbortSignal; timeoutMs?: number; }): Promise { const page = await getPageForTargetId(opts); @@ -386,13 +389,9 @@ export async function waitForDownloadViaPlaywright(opts: { timeoutMs: timeout, outPath: opts.path, rootDir: opts.path?.trim() ? opts.rootDir : (opts.rootDir ?? resolveImplicitDownloadRoot()), + signal: opts.signal, }); - try { - return await capture.promise; - } catch (err) { - capture.cancel(); - throw err; - } + return await capture.promise; } /** Clicks an element ref and saves the download triggered by that click. */ @@ -402,6 +401,7 @@ export async function downloadViaPlaywright(opts: { ref: string; path: string; rootDir?: string; + signal?: AbortSignal; timeoutMs?: number; }): Promise { const page = await getPageForTargetId(opts); @@ -421,17 +421,17 @@ export async function downloadViaPlaywright(opts: { timeoutMs: timeout, outPath, rootDir: opts.rootDir, + signal: opts.signal, }); + void capture.promise.catch(() => {}); try { const locator = refLocator(page, ref); - try { - await locator.click({ timeout }); - } catch (err) { - throw toAIFriendlyError(err, ref); - } - return await capture.promise; + await locator.click({ timeout, signal: opts.signal }); } catch (err) { capture.cancel(); - throw err; + throw opts.signal?.aborted && opts.signal.reason instanceof Error + ? opts.signal.reason + : toAIFriendlyError(err, ref); } + return await capture.promise; } diff --git a/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index 2f15315ce808..3ed838d2111d 100644 --- a/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -318,6 +318,43 @@ describe("pw-tools-core", () => { expect(harness.activeHandlerCount()).toBe(0); }); + it("releases a cancelled waiter before the next download", async () => { + const harness = createDownloadEventHarness(); + const state = sessionMocks.ensurePageState(); + const controller = new AbortController(); + const cancelled = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + timeoutMs: 1000, + signal: controller.signal, + }); + + await Promise.resolve(); + expect(state.downloadWaiterDepth).toBe(1); + controller.abort(new Error("request aborted")); + await expect(cancelled).rejects.toThrow("request aborted"); + expect(state.downloadWaiterDepth).toBe(0); + expect(harness.activeHandlerCount()).toBe(0); + + const successor = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + timeoutMs: 1000, + }); + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "successor-content", "utf8"); + }); + await Promise.resolve(); + harness.trigger({ + url: () => "https://example.com/successor.bin", + suggestedFilename: () => "successor.bin", + saveAs, + }); + + await expect(successor).resolves.toMatchObject({ suggestedFilename: "successor.bin" }); + expect(saveAs).toHaveBeenCalledOnce(); + }); + it("lets only the latest overlapping explicit waiter save the download", async () => { const harness = createDownloadEventHarness(); const state = sessionMocks.ensurePageState(); diff --git a/extensions/browser/src/browser/routes/agent.act.download.ts b/extensions/browser/src/browser/routes/agent.act.download.ts index b4cd8129d776..3b200341ff48 100644 --- a/extensions/browser/src/browser/routes/agent.act.download.ts +++ b/extensions/browser/src/browser/routes/agent.act.download.ts @@ -50,7 +50,7 @@ export function registerBrowserAgentActDownloadRoutes( ctx, targetId, enforceCurrentUrlAllowed: true, - run: async ({ profileCtx, cdpUrl, tab }) => { + run: async ({ profileCtx, cdpUrl, tab, signal }) => { if (getBrowserProfileCapabilities(profileCtx.profile).usesChromeMcp) { return jsonError(res, 501, EXISTING_SESSION_LIMITS.download.waitUnsupported); } @@ -77,6 +77,7 @@ export function registerBrowserAgentActDownloadRoutes( ...requestBase, path: downloadPath, rootDir: DEFAULT_DOWNLOAD_DIR, + signal, }); res.json({ ok: true, targetId: tab.targetId, download: result }); }, @@ -107,7 +108,7 @@ export function registerBrowserAgentActDownloadRoutes( ctx, targetId, enforceCurrentUrlAllowed: true, - run: async ({ profileCtx, cdpUrl, tab }) => { + run: async ({ profileCtx, cdpUrl, tab, signal }) => { if (getBrowserProfileCapabilities(profileCtx.profile).usesChromeMcp) { return jsonError(res, 501, EXISTING_SESSION_LIMITS.download.downloadUnsupported); } @@ -131,6 +132,7 @@ export function registerBrowserAgentActDownloadRoutes( ref, path: downloadPath, rootDir: DEFAULT_DOWNLOAD_DIR, + signal, }); res.json({ ok: true, targetId: tab.targetId, download: result }); }, diff --git a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts index a8cbd6abcd90..d4f7058675ea 100644 --- a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -841,9 +841,42 @@ describe("browser control server", () => { expectRecordFields(waitCall, "wait download call", { targetId: "abcd1234", }); + expect(waitCall.signal).toBeInstanceOf(AbortSignal); expect(String(waitCall.path)).toContain("safe-wait.pdf"); }); + it("cancels wait/download when its HTTP caller disconnects", async () => { + const base = await startServerAndBase(); + let operationSignal: AbortSignal | undefined; + requirePwMock("waitForDownloadViaPlaywright").mockImplementationOnce(async (value) => { + const options = value as { signal?: AbortSignal }; + operationSignal = options.signal; + await new Promise((_resolve, reject) => { + options.signal?.addEventListener( + "abort", + () => { + const reason = options.signal?.reason; + reject(reason instanceof Error ? reason : new Error("request aborted")); + }, + { once: true }, + ); + }); + throw new Error("unreachable"); + }); + const controller = new AbortController(); + const response = realFetch(`${base}/wait/download`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ path: "cancelled-wait.pdf" }), + signal: controller.signal, + }); + + await vi.waitFor(() => expect(operationSignal).toBeInstanceOf(AbortSignal)); + controller.abort(new Error("caller disconnected")); + await expect(response).rejects.toThrow(); + await vi.waitFor(() => expect(operationSignal?.aborted).toBe(true)); + }); + it("download accepts in-root relative output path", async () => { const base = await startServerAndBase(); const res = await postJson<{ ok?: boolean; download?: { path?: string } }>(`${base}/download`, { @@ -857,6 +890,7 @@ describe("browser control server", () => { targetId: "abcd1234", ref: "e12", }); + expect(downloadCall.signal).toBeInstanceOf(AbortSignal); expect(String(downloadCall.path)).toContain("safe-download.pdf"); }); });