From b2fad362f2b37f81dc9a6f784f4521e4ca24d41e Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Thu, 22 Oct 2026 08:52:56 +0800 Subject: [PATCH] fix(browser): stop expired work and release deadline resources --- .../PlaywrightExecutionBoundary.test.ts | 52 +++++++++++++++++++ src/browser/PlaywrightExecutionBoundary.ts | 13 +++-- .../PlaywrightScenarioServiceWorkers.ts | 50 ++---------------- .../browser/cdpTargetDeadlines.test.ts | 4 ++ 4 files changed, 70 insertions(+), 49 deletions(-) diff --git a/src/browser/PlaywrightExecutionBoundary.test.ts b/src/browser/PlaywrightExecutionBoundary.test.ts index 4c15a06779..741e5a9126 100644 --- a/src/browser/PlaywrightExecutionBoundary.test.ts +++ b/src/browser/PlaywrightExecutionBoundary.test.ts @@ -1,3 +1,5 @@ +import { getEventListeners } from "node:events"; + import { afterEach, describe, expect, it, vi } from "vitest"; import { withPlaywrightExecutionBoundary } from "./PlaywrightExecutionBoundary.js"; @@ -24,6 +26,56 @@ describe("withPlaywrightExecutionBoundary", () => { await rejection; }); + it("uses the owning policy's timeout error when its deadline expires", async () => { + vi.useFakeTimers(); + const timeout = new AnalysisTimeoutError("capture_browser_scenario", 25, { + userMessage: "Fixed service-worker policy deadline.", + }); + const result = withPlaywrightExecutionBoundary( + () => new Promise(() => undefined), + 25, + undefined, + () => timeout, + ); + const rejection = expect(result).rejects.toBe(timeout); + await vi.advanceTimersByTimeAsync(25); + await rejection; + }); + + it("does not admit work or attach cancellation after synchronous expiry", async () => { + vi.spyOn(performance, "now") + .mockReturnValueOnce(1_000) + .mockReturnValueOnce(1_001); + const controller = new AbortController(); + const addListener = vi.spyOn(controller.signal, "addEventListener"); + const work = vi.fn(async () => "unreachable"); + + await expect( + withPlaywrightExecutionBoundary(work, 0.5, controller.signal), + ).rejects.toBeInstanceOf(AnalysisTimeoutError); + expect(work).not.toHaveBeenCalled(); + expect(addListener).not.toHaveBeenCalled(); + }); + + it("cleans up when admitted work throws synchronously", async () => { + vi.useFakeTimers(); + const cause = { opaque: true }; + const controller = new AbortController(); + const work = vi.fn((): Promise => { + throw cause; + }); + const pending = withPlaywrightExecutionBoundary( + work, + 1_000, + controller.signal, + ); + + await expect(pending).rejects.toBe(cause); + expect(work).toHaveBeenCalledOnce(); + expect(getEventListeners(controller.signal, "abort")).toHaveLength(0); + expect(vi.getTimerCount()).toBe(0); + }); + it("rejects when the caller cancels active work", async () => { const controller = new AbortController(); const result = withPlaywrightExecutionBoundary( diff --git a/src/browser/PlaywrightExecutionBoundary.ts b/src/browser/PlaywrightExecutionBoundary.ts index b500573f9a..2f6921a14a 100644 --- a/src/browser/PlaywrightExecutionBoundary.ts +++ b/src/browser/PlaywrightExecutionBoundary.ts @@ -12,9 +12,11 @@ export const withPlaywrightExecutionBoundary = ( work: () => Promise, timeoutMs: number | undefined, signal?: AbortSignal, + timeoutError: () => AnalysisTimeoutError = () => + new AnalysisTimeoutError(OPERATION, timeoutMs ?? 0), ): Promise => { if (timeoutMs !== undefined && timeoutMs <= 0) - return Promise.reject(new AnalysisTimeoutError(OPERATION, timeoutMs)); + return Promise.reject(timeoutError()); if (signal?.aborted === true) return Promise.reject( new AnalysisCancelledError(OPERATION, { cause: signal.reason }), @@ -29,7 +31,7 @@ export const withPlaywrightExecutionBoundary = ( if (deadline === undefined) return; const remaining = deadline - performance.now(); if (remaining <= 0) { - fail(new AnalysisTimeoutError(OPERATION, timeoutMs ?? 0)); + fail(timeoutError()); return; } timer = setTimeout(scheduleTimeout, Math.min(remaining, 2_147_483_647)); @@ -55,11 +57,16 @@ export const withPlaywrightExecutionBoundary = ( fail(new AnalysisCancelledError(OPERATION, { cause: signal?.reason })); }; scheduleTimeout(); + if (settled) return; signal?.addEventListener("abort", onAbort, { once: true }); if (signal?.aborted === true) { onAbort(); return; } - void work().then(succeed, fail); + try { + void work().then(succeed, fail); + } catch (cause: unknown) { + fail(cause); + } }); }; diff --git a/src/browser/PlaywrightScenarioServiceWorkers.ts b/src/browser/PlaywrightScenarioServiceWorkers.ts index 6deb4996e2..e8b559b5de 100644 --- a/src/browser/PlaywrightScenarioServiceWorkers.ts +++ b/src/browser/PlaywrightScenarioServiceWorkers.ts @@ -8,10 +8,7 @@ import { CDP_DISCOVERY_RESPONSE_TIMEOUT_MS, selectedCdpCommandTimeoutMs, } from "../domain/browserDeadline.js"; -import { - AnalysisCancelledError, - AnalysisTimeoutError, -} from "../domain/analysisErrorCore.js"; +import { AnalysisTimeoutError } from "../domain/analysisErrorCore.js"; import { browserObservationEndpointSchema } from "../domain/browserObservation.js"; import { BrowserObservationError } from "../domain/browserObservationError.js"; import { CdpConnection } from "./CdpConnection.js"; @@ -44,47 +41,6 @@ const serviceWorkerPolicyCheckTimeout = (): AnalysisTimeoutError => { }); }; -const withServiceWorkerPolicyDeadline = ( - work: () => Promise, - signal: AbortSignal, -): Promise => { - if (signal.aborted) - return Promise.reject( - new AnalysisCancelledError(OPERATION, { cause: signal.reason }), - ); - return new Promise((resolve, reject) => { - let settled = false; - const timer = setTimeout(() => { - fail(serviceWorkerPolicyCheckTimeout()); - }, SERVICE_WORKER_POLICY_CHECK_TIMEOUT_MS); - timer.unref(); - const cleanup = (): void => { - clearTimeout(timer); - signal.removeEventListener("abort", onAbort); - }; - const succeed = (value: Value): void => { - if (settled) return; - settled = true; - cleanup(); - resolve(value); - }; - const fail = (error: unknown): void => { - if (settled) return; - settled = true; - cleanup(); - reject(error); - }; - const onAbort = (): void => { - fail(new AnalysisCancelledError(OPERATION, { cause: signal.reason })); - }; - signal.addEventListener("abort", onAbort, { once: true }); - if (signal.aborted) { - onAbort(); - return; - } - void work().then(succeed, fail); - }); -}; const scriptResponse = z.object({ identifier: z.string() }); const restorationResponse = z.object({ result: z.object({ value: z.literal("policy_owned").nullable() }), @@ -136,9 +92,11 @@ export class PlaywrightScenarioServiceWorkers { } private async install(): Promise { - await withServiceWorkerPolicyDeadline( + await withPlaywrightExecutionBoundary( () => this.assertBlocked(), + SERVICE_WORKER_POLICY_CHECK_TIMEOUT_MS, this.#setup.signal, + serviceWorkerPolicyCheckTimeout, ); // Discovery stays on its fixed deadline. The following handshake and // Target.attachToTarget each use command_timeout_ms. diff --git a/tests/boundary/browser/cdpTargetDeadlines.test.ts b/tests/boundary/browser/cdpTargetDeadlines.test.ts index d705f5ab07..acdbc7aa2a 100644 --- a/tests/boundary/browser/cdpTargetDeadlines.test.ts +++ b/tests/boundary/browser/cdpTargetDeadlines.test.ts @@ -401,6 +401,8 @@ it("stops a stalled blocked-service-worker check before discovery", async () => "allowed-page", commandTimeoutMs, ); + // Keep the monotonic deadline and its explicitly expired timer in sync. + const clock = vi.spyOn(performance, "now").mockReturnValue(1_000); const pending = workers.initialize(); try { await vi.waitFor(() => @@ -411,6 +413,7 @@ it("stops a stalled blocked-service-worker check before discovery", async () => expect(browser.httpRequests).toHaveLength(0); expect(browser.commands).toHaveLength(0); expect(activeTimers(commandTimeoutMs)).toHaveLength(0); + clock.mockReturnValue(1_000 + SERVICE_WORKER_POLICY_CHECK_TIMEOUT_MS); expireTimer(SERVICE_WORKER_POLICY_CHECK_TIMEOUT_MS); const failure = await pending.catch((cause: unknown) => cause); expect(failure).toBeInstanceOf(AnalysisTimeoutError); @@ -424,6 +427,7 @@ it("stops a stalled blocked-service-worker check before discovery", async () => expect(browser.httpRequests).toHaveLength(0); expect(activeTimers(commandTimeoutMs)).toHaveLength(0); } finally { + clock.mockRestore(); const closing = workers.close().catch(() => undefined); await browser.close(); await closing;