From 83fa2fc5713088e914458e074d8706efb50873ca Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 25 Aug 2026 22:40:27 -0700 Subject: [PATCH] fix(browser): enforce download deadlines through atomic save (#129899) --- .../src/browser/pw-download-capture.test.ts | 167 ++++++++++++++++++ .../src/browser/pw-download-capture.ts | 30 ++-- 2 files changed, 185 insertions(+), 12 deletions(-) diff --git a/extensions/browser/src/browser/pw-download-capture.test.ts b/extensions/browser/src/browser/pw-download-capture.test.ts index 2794517cf8d0..27b6d8ab533a 100644 --- a/extensions/browser/src/browser/pw-download-capture.test.ts +++ b/extensions/browser/src/browser/pw-download-capture.test.ts @@ -7,6 +7,61 @@ import { describe, expect, it, vi } from "vitest"; import { createDownloadCaptureForPage } from "./pw-download-capture.js"; describe("Playwright download capture cancellation", () => { + it.each(["explicit", "passive"] as const)( + "enforces the %s download deadline after its event arrives", + async (mode) => { + vi.useFakeTimers(); + const page = new EventEmitter(); + const state = { downloadWaiterDepth: 0 }; + const controller = new AbortController(); + const validation = createDeferred(); + const saveAs = vi.fn(async () => {}); + const cancel = vi.fn(async () => {}); + const timeoutMessage = + mode === "passive" + ? "Timeout waiting for navigation download" + : "Timeout waiting for download"; + const capture = createDownloadCaptureForPage(page, state, 25, { + mode, + signal: controller.signal, + timeoutMessage, + beforeSave: async () => { + await validation.promise; + }, + }); + const outcome = capture.promise.then( + () => "resolved" as const, + (error: unknown) => error, + ); + + try { + page.emit("download", { + url: () => "https://example.com/export.csv", + suggestedFilename: () => "export.csv", + saveAs, + cancel, + }); + await vi.advanceTimersByTimeAsync(25); + + await expect(Promise.race([outcome, Promise.resolve("pending")])).resolves.toMatchObject({ + message: timeoutMessage, + }); + expect(cancel).toHaveBeenCalledOnce(); + validation.resolve(); + await vi.advanceTimersByTimeAsync(0); + + expect(saveAs).not.toHaveBeenCalled(); + expect(state.downloadWaiterDepth).toBe(0); + expect(page.listenerCount("download")).toBe(0); + } finally { + controller.abort(new Error("download test cleanup")); + validation.resolve(); + await outcome; + vi.useRealTimers(); + } + }, + ); + it("cancels a captured download before delayed validation can save bytes", async () => { const page = new EventEmitter(); const state = { downloadWaiterDepth: 0 }; @@ -114,6 +169,58 @@ describe("Playwright download capture cancellation", () => { } }); + it("cancels a timed-out in-progress download without publishing staged output", async () => { + const outputRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-download-timeout-")); + const outputPath = path.join(outputRoot, "timed-out.bin"); + vi.useFakeTimers(); + const page = new EventEmitter(); + const state = { downloadWaiterDepth: 0 }; + const saveGate = createDeferred(); + const saveStarted = createDeferred(); + const saveAs = vi.fn(async (tempPath: string) => { + await fs.writeFile(tempPath, "timed-out partial contents", "utf8"); + saveStarted.resolve(tempPath); + await saveGate.promise; + }); + const cancel = vi.fn(async () => { + saveGate.resolve(); + }); + const capture = createDownloadCaptureForPage(page, state, 25, { + mode: "explicit", + outputPath, + outputRoot, + }); + const outcome = capture.promise.then( + () => "resolved" as const, + (error: unknown) => error, + ); + + try { + page.emit("download", { + url: () => "https://example.com/timed-out.bin", + suggestedFilename: () => "timed-out.bin", + saveAs, + cancel, + }); + const partialPath = await saveStarted.promise; + await vi.advanceTimersByTimeAsync(25); + + await expect(Promise.race([outcome, Promise.resolve("pending")])).resolves.toMatchObject({ + message: "Timeout waiting for download", + }); + expect(cancel).toHaveBeenCalledOnce(); + await vi.waitFor(async () => { + await expect(fs.access(partialPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + await expect(fs.access(outputPath)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + saveGate.resolve(); + await outcome; + vi.useRealTimers(); + await fs.rm(outputRoot, { recursive: true, force: true }); + } + }); + it("finishes atomic publication when cancellation arrives after its commit boundary", async () => { const outputRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-download-publish-")); const outputPath = path.join(outputRoot, "published.bin"); @@ -178,4 +285,64 @@ describe("Playwright download capture cancellation", () => { await fs.rm(outputRoot, { recursive: true, force: true }); } }); + + it("finishes atomic publication after its download deadline retires", async () => { + const outputRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-download-deadline-")); + const outputPath = path.join(outputRoot, "published.bin"); + vi.useFakeTimers(); + const renameStarted = createDeferred(); + const releaseRename = createDeferred(); + const renameFinished = createDeferred(); + const originalRename = fs.rename.bind(fs); + const rename = vi.spyOn(fs, "rename").mockImplementation(async (source, destination) => { + if (String(destination).endsWith(`${path.sep}published.bin`)) { + renameStarted.resolve(); + await releaseRename.promise; + } + try { + await originalRename(source, destination); + } finally { + renameFinished.resolve(); + } + }); + const page = new EventEmitter(); + const state = { downloadWaiterDepth: 0 }; + const cancel = vi.fn(async () => {}); + const capture = createDownloadCaptureForPage(page, state, 25, { + mode: "explicit", + outputPath, + outputRoot, + }); + const outcome = capture.promise.then( + (result) => result, + (error: unknown) => error, + ); + + try { + page.emit("download", { + url: () => "https://example.com/published.bin", + suggestedFilename: () => "published.bin", + saveAs: async (tempPath: string) => { + await fs.writeFile(tempPath, "completed download", "utf8"); + }, + cancel, + }); + await renameStarted.promise; + await vi.advanceTimersByTimeAsync(25); + + expect(await Promise.race([outcome, Promise.resolve("pending")])).toBe("pending"); + expect(cancel).not.toHaveBeenCalled(); + releaseRename.resolve(); + + await expect(capture.promise).resolves.toMatchObject({ path: outputPath }); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("completed download"); + } finally { + releaseRename.resolve(); + await renameFinished.promise; + await outcome; + rename.mockRestore(); + vi.useRealTimers(); + await fs.rm(outputRoot, { recursive: true, force: true }); + } + }); }); diff --git a/extensions/browser/src/browser/pw-download-capture.ts b/extensions/browser/src/browser/pw-download-capture.ts index ebbae48ab198..ade46b3c8534 100644 --- a/extensions/browser/src/browser/pw-download-capture.ts +++ b/extensions/browser/src/browser/pw-download-capture.ts @@ -91,6 +91,7 @@ export function createDownloadCaptureForPage( } state.downloadWaiterDepth += 1; + const operation = new AbortController(); let done = false; let timer: NodeJS.Timeout | undefined; let handler: ((download: unknown) => void) | undefined; @@ -103,6 +104,9 @@ export function createDownloadCaptureForPage( page.off("download", handler); handler = undefined; } + }; + + const retireDeadline = () => { if (timer) { clearTimeout(timer); timer = undefined; @@ -112,20 +116,31 @@ export function createDownloadCaptureForPage( const cleanup = () => { done = true; releaseWaiter(); + retireDeadline(); opts.signal?.removeEventListener("abort", abort); }; const promise = new Promise((resolve, reject) => { + const rejectCapture = (reason: Error) => { + if (done) { + return; + } + operation.abort(reason); + cleanup(); + void activeDownload?.cancel?.().catch(() => {}); + reject(reason); + }; handler = (download: unknown) => { if (done) { return; } activeDownload = download as PlaywrightDownload; releaseWaiter(); - void saveBrowserDownload(activeDownload, opts, () => { + void saveBrowserDownload(activeDownload, { ...opts, signal: operation.signal }, () => { // Atomic publication cannot be revoked, so a later abort must not // report cancellation while its completed file is being published. opts.signal?.removeEventListener("abort", abort); + retireDeadline(); }) .finally(cleanup) .then(resolve, reject); @@ -133,23 +148,14 @@ export function createDownloadCaptureForPage( page.on("download", handler); timer = setTimeout( () => { - if (done) { - return; - } - cleanup(); - reject(new Error(opts.timeoutMessage ?? "Timeout waiting for download")); + rejectCapture(new Error(opts.timeoutMessage ?? "Timeout waiting for download")); }, Math.max(1, timeoutMs), ); timer.unref?.(); abort = () => { - if (done) { - return; - } - cleanup(); - void activeDownload?.cancel?.().catch(() => {}); const reason = opts.signal?.reason; - reject(reason instanceof Error ? reason : new Error("Download wait was cancelled")); + rejectCapture(reason instanceof Error ? reason : new Error("Download wait was cancelled")); }; opts.signal?.addEventListener("abort", abort, { once: true }); if (opts.signal?.aborted) {