diff --git a/src/browser/PlaywrightBrowserScenarioProvider.test.ts b/src/browser/PlaywrightBrowserScenarioProvider.test.ts index 5a976a565c..88fe1f97d8 100644 --- a/src/browser/PlaywrightBrowserScenarioProvider.test.ts +++ b/src/browser/PlaywrightBrowserScenarioProvider.test.ts @@ -821,7 +821,8 @@ it("shares concurrent close and retries browser cleanup after failure", async () .mockResolvedValue(undefined); const cleanupSettlements: boolean[] = []; const cleanup = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: browserClose, + closeBrowser: (disconnectAtBoundary) => + disconnectAtBoundary(() => browserClose()), removeProfile, onSettled: (browserClosed) => cleanupSettlements.push(browserClosed), }); @@ -851,7 +852,8 @@ it("keeps failed event finalization failed across cleanup retries", async () => .fn<() => Promise>() .mockResolvedValue(undefined); const cleanup = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser, + closeBrowser: (disconnectAtBoundary) => + disconnectAtBoundary(() => closeBrowser()), removeProfile: undefined, }); @@ -924,7 +926,8 @@ it.each([false, true])( ); let denied = true; const owner = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: async () => {}, + closeBrowser: (disconnectAtBoundary) => + disconnectAtBoundary(async () => undefined), removeProfile: async () => { if (denied) throw new Error("profile removal denied"); }, @@ -1099,7 +1102,8 @@ it.each([false, true])( const caller = new AbortController(); const reason = { caller: "during final browser close" }; const owner = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: () => acknowledgement, + closeBrowser: (disconnectAtBoundary) => + disconnectAtBoundary(() => acknowledgement), removeProfile: undefined, }); const session = new FakeSession("launch"); diff --git a/src/browser/PlaywrightScenarioBrowser.test.ts b/src/browser/PlaywrightScenarioBrowser.test.ts index cd2b954809..d067674ec7 100644 --- a/src/browser/PlaywrightScenarioBrowser.test.ts +++ b/src/browser/PlaywrightScenarioBrowser.test.ts @@ -135,9 +135,9 @@ it("keeps attached-target ownership after the cleanup response timeout", async ( const closeResult = deferred(); const settled: boolean[] = []; const cleanup = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: () => { + closeBrowser: async (disconnectAtBoundary) => { closeStarted.resolve(undefined); - return closeResult.promise; + await disconnectAtBoundary(() => closeResult.promise); }, removeProfile: undefined, onSettled: (browserClosed) => settled.push(browserClosed), @@ -399,6 +399,50 @@ it("restores its environment overrides before releasing an attached browser", as ]); }); +it("checks a late stop after attached restoration and before transport disconnect", async () => { + const restorationStarted = deferred(); + const finishRestoration = deferred(); + const order: string[] = []; + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + method === "Emulation.setTimezoneOverride" && + (parameters as { timezoneId?: string }).timezoneId === "" + ) { + order.push("restore-started"); + restorationStarted.resolve(undefined); + await finishRestoration.promise; + order.push("restore-finished"); + } + }, + closeBrowser: async () => { + order.push("browser-disconnected"); + }, + }); + const opened = await fixture.open(); + let revision = 0; + const release = opened.cleanup.release(undefined, { + get revision() { + return revision; + }, + async run() { + order.push(`gate:${revision}`); + }, + }); + + await restorationStarted.promise; + revision += 1; + finishRestoration.resolve(undefined); + await release; + + expect(order).toEqual([ + "restore-started", + "restore-finished", + "gate:1", + "browser-disconnected", + ]); +}); + it("retains attached environment ownership until a failed reset can be retried", async () => { let localeResetAttempts = 0; const fixture = attachedBrowserFixture({ @@ -701,9 +745,12 @@ it("tracks only pending browser cleanup resources and resolves prior errors afte let profileDenied = true; let browserCloses = 0; const owner = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: async () => { + closeBrowser: async (disconnectAtBoundary) => { browserCloses++; - if (browserDenied) throw new Error("controlled transport close failure"); + await disconnectAtBoundary(async () => { + if (browserDenied) + throw new Error("controlled transport close failure"); + }); }, removeProfile: async () => { if (profileDenied) throw new Error("controlled profile removal failure"); @@ -743,6 +790,145 @@ it("tracks only pending browser cleanup resources and resolves prior errors afte } }); +it("rechecks the browser release gate after finalization changes its revision", async () => { + const eventsStarted = deferred(); + const finishEvents = deferred(); + const gateStarted = deferred(); + const finishGate = deferred(); + const closeBrowser = vi.fn( + async ( + disconnectAtBoundary: (disconnect: () => Promise) => Promise, + ) => disconnectAtBoundary(async () => undefined), + ); + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser, + removeProfile: undefined, + }); + let revision = 0; + const gateRuns: number[] = []; + const release = owner.release( + async () => { + eventsStarted.resolve(undefined); + await finishEvents.promise; + }, + { + get revision() { + return revision; + }, + async run() { + gateRuns.push(revision); + if (gateRuns.length === 1) { + gateStarted.resolve(undefined); + await finishGate.promise; + } + }, + }, + ); + + await eventsStarted.promise; + finishEvents.resolve(undefined); + await gateStarted.promise; + revision += 1; + finishGate.resolve(undefined); + await release; + + expect(gateRuns).toEqual([0, 1]); + expect(closeBrowser).toHaveBeenCalledTimes(1); + expect(owner.browserClosed).toBe(true); +}); + +it("runs the latest stop gate after attached restoration and before disconnect", async () => { + const restorationStarted = deferred(); + const finishRestoration = deferred(); + const gateRevisions: number[] = []; + let revision = 0; + let disconnected = false; + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser: async (disconnectAtBoundary) => { + restorationStarted.resolve(undefined); + await finishRestoration.promise; + await disconnectAtBoundary(async () => { + disconnected = true; + }); + }, + removeProfile: undefined, + }); + const release = owner.release(undefined, { + get revision() { + return revision; + }, + async run() { + gateRevisions.push(revision); + }, + }); + + await restorationStarted.promise; + revision += 1; + finishRestoration.resolve(undefined); + await release; + + expect(gateRevisions).toEqual([1]); + expect(disconnected).toBe(true); + expect(owner.browserClosed).toBe(true); +}); + +it.each([false, true])( + "retains a failed release gate when its revision changes: %s", + async (changeRevision) => { + let transportClosed = false; + const closeBrowser = vi.fn( + async ( + disconnectAtBoundary: ( + disconnect: () => Promise, + ) => Promise, + ) => + disconnectAtBoundary(async () => { + transportClosed = true; + }), + ); + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser, + removeProfile: undefined, + }); + const failure = new Error("navigation stop was not confirmed"); + const gateStarted = deferred(); + const finishGate = deferred(); + let revision = 0; + let failGate = true; + const gate = { + get revision() { + return revision; + }, + async run() { + if (failGate) { + failGate = false; + gateStarted.resolve(undefined); + await finishGate.promise; + throw failure; + } + }, + }; + + const release = owner.release(undefined, gate).then( + () => ({ ok: true as const }), + (cause: unknown) => ({ ok: false as const, cause }), + ); + await gateStarted.promise; + if (changeRevision) revision += 1; + finishGate.resolve(undefined); + await expect(release).resolves.toEqual({ ok: false, cause: failure }); + expect(closeBrowser).toHaveBeenCalledTimes(1); + expect(transportClosed).toBe(false); + expect(owner.browserClosed).toBe(false); + expect(owner.cleanupResources).toContain("browser_transport"); + + await owner.release(undefined, gate); + expect(closeBrowser).toHaveBeenCalledTimes(2); + expect(transportClosed).toBe(true); + expect(owner.browserClosed).toBe(true); + }, +); + it("bounds an opening failure response while retaining the same late cleanup lease", async () => { vi.useFakeTimers(); let acknowledge: () => void = () => {}; diff --git a/src/browser/PlaywrightScenarioBrowser.ts b/src/browser/PlaywrightScenarioBrowser.ts index c261553070..bc4f4db26e 100644 --- a/src/browser/PlaywrightScenarioBrowser.ts +++ b/src/browser/PlaywrightScenarioBrowser.ts @@ -43,9 +43,17 @@ export interface OpenedScenarioBrowser { readonly cleanup: PlaywrightScenarioBrowserCleanupOwner; } +/** A retryable obligation that must hold before the browser transport closes. */ +export interface PlaywrightScenarioBrowserReleaseGate { + readonly revision: number; + run(): Promise; +} + /** Provider actions retained until the owned browser and profile are released. */ interface PlaywrightScenarioCleanupResources { - readonly closeBrowser: () => Promise; + readonly closeBrowser: ( + disconnectAtBoundary: (disconnect: () => Promise) => Promise, + ) => Promise; readonly removeProfile: (() => Promise) | undefined; readonly onSettled?: (browserClosed: boolean) => void; } @@ -56,6 +64,7 @@ export class PlaywrightScenarioBrowserCleanupOwner { #profileRemoved: boolean; #eventFinalizationPromise: Promise | undefined; #eventsFinalized = false; + #releaseGate: PlaywrightScenarioBrowserReleaseGate | undefined; readonly cleanupLease: AnalysisCleanupLease; /** Keep the narrow provider actions needed to release one opened browser. */ @@ -83,13 +92,20 @@ export class PlaywrightScenarioBrowserCleanupOwner { } /** Await actual resource release, including a late browser acknowledgement. */ - release(finishEvents?: () => Promise): Promise { + release( + finishEvents?: () => Promise, + releaseGate?: PlaywrightScenarioBrowserReleaseGate, + ): Promise { this.#prepareEvents(finishEvents); + if (releaseGate !== undefined) this.#releaseGate = releaseGate; return this.cleanupLease.release(); } - close(finishEvents?: () => Promise): Promise { - return this.#awaitCleanup(this.release(finishEvents)); + close( + finishEvents?: () => Promise, + releaseGate?: PlaywrightScenarioBrowserReleaseGate, + ): Promise { + return this.#awaitCleanup(this.release(finishEvents, releaseGate)); } #prepareEvents(finishEvents?: () => Promise): void { @@ -131,7 +147,9 @@ export class PlaywrightScenarioBrowserCleanupOwner { } if (!this.#browserClosed) { try { - await this.resources.closeBrowser(); + await this.resources.closeBrowser((disconnect) => + this.#runReleaseGate(disconnect), + ); this.#browserClosed = true; } catch (cause: unknown) { failures.push(cause); @@ -153,6 +171,25 @@ export class PlaywrightScenarioBrowserCleanupOwner { if (failures.length > 1) throw new AggregateError(failures, "Browser session cleanup failed"); } + + async #runReleaseGate(disconnect: () => Promise): Promise { + for (;;) { + const gate = this.#releaseGate; + if (gate === undefined) { + await disconnect(); + return; + } + const revision = gate.revision; + await gate.run(); + if (this.#releaseGate === gate && gate.revision === revision) { + // Invoke the physical disconnect in the same continuation that + // observes a stable stop revision; no await separates the check and + // the operation start. + await disconnect(); + return; + } + } + } } interface AttachedScenarioWaiter { @@ -566,7 +603,7 @@ const openAttachedScenarioBrowser = async ( const browser = await launcher.connectOverCDP(browserRequest.cdp_endpoint); const cleanup: PlaywrightScenarioBrowserCleanupOwner = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: async () => { + closeBrowser: async (disconnectAtBoundary) => { let environmentFailure: unknown; const ownedEnvironment = environmentOwner; if (ownedEnvironment !== undefined) { @@ -590,7 +627,7 @@ const openAttachedScenarioBrowser = async ( } } try { - await browser.close(); + await disconnectAtBoundary(() => browser.close()); // Once target state is restored, disconnecting the transport is a // definitive fallback for an explicit session detach failure. environmentOwner = undefined; @@ -719,7 +756,9 @@ export const openPlaywrightScenarioBrowser = async ( } } const cleanup = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: () => context.close(), + closeBrowser: async (disconnectAtBoundary) => { + await disconnectAtBoundary(() => context.close()); + }, removeProfile: () => rm(profilePath, { recursive: true, force: true, maxRetries: 3 }), }); diff --git a/src/browser/PlaywrightScenarioServiceWorkers.ts b/src/browser/PlaywrightScenarioServiceWorkers.ts index 40b79dfc24..46ffc0579e 100644 --- a/src/browser/PlaywrightScenarioServiceWorkers.ts +++ b/src/browser/PlaywrightScenarioServiceWorkers.ts @@ -51,6 +51,8 @@ export class PlaywrightScenarioServiceWorkers { #released = false; #detached = false; #loadingStopped = false; + #stopRequested = false; + #stopPromise: Promise | undefined; #removeListener: (() => void) | undefined; #setup = new AbortController(); @@ -276,9 +278,11 @@ export class PlaywrightScenarioServiceWorkers { private async stopMainFrame(frame: Frame): Promise { try { await frame.evaluate("globalThis.stop()"); - this.#loadingStopped = true; } catch (cause: unknown) { - if (!this.page.isClosed() && !frame.isDetached()) throw cause; + // A closed page no longer has an active navigation to stop. On a live + // page, including a detached frame, preserve failure so retry can use a + // fresh main-frame reference. + if (!this.page.isClosed()) throw cause; } } @@ -313,9 +317,10 @@ export class PlaywrightScenarioServiceWorkers { close(stopLoading = false): Promise { this.#closed = true; + this.#stopRequested ||= stopLoading; // Abort discovery/handshakes and reads, never an already-sent page mutation. this.#setup.abort(); - const restoration = (this.#restoration ??= this.restore(stopLoading)); + const restoration = (this.#restoration ??= this.restore()); void restoration.then( () => { if (this.#restoration === restoration) this.#restoration = undefined; @@ -324,13 +329,48 @@ export class PlaywrightScenarioServiceWorkers { if (this.#restoration === restoration) this.#restoration = undefined; }, ); + // Restoration completion does not discharge a stop request that arrived + // after that restoration began. Both paths share the owner's stop latch. + const completion = restoration.then(() => this.fulfillStopRequest()); return withPlaywrightExecutionBoundary( - () => restoration, + () => completion, POLICY_CLEANUP_CONFIRMATION_TIMEOUT_MS, ); } - private async restore(stopLoading: boolean): Promise { + private fulfillStopRequest(): Promise { + if (!this.#stopRequested || this.#loadingStopped) return Promise.resolve(); + if (this.#stopPromise !== undefined) return this.#stopPromise; + const stopping = (async () => { + if (this.page.isClosed()) { + this.#loadingStopped = true; + return; + } + const session = this.#session; + if ( + !this.#released && + !this.#detached && + session?.sessionId !== undefined + ) { + await this.send(session.sessionId, "Page.stopLoading"); + } else { + await this.stopMainFrame(this.page.mainFrame()); + } + this.#loadingStopped = true; + })(); + this.#stopPromise = stopping; + void stopping.then( + () => { + if (this.#stopPromise === stopping) this.#stopPromise = undefined; + }, + () => { + if (this.#stopPromise === stopping) this.#stopPromise = undefined; + }, + ); + return stopping; + } + + private async restore(): Promise { await this.#initialization?.catch(() => undefined); if (this.#released) return; const session = this.#session; @@ -339,14 +379,7 @@ export class PlaywrightScenarioServiceWorkers { const mainFrame = this.page.mainFrame(); // This owner exists even with empty storage seeds. Stopped navigation is // independent of storage and restoration never waits for a load event. - if (stopLoading && !this.#loadingStopped) { - if (!this.#detached && session.sessionId !== undefined) { - await this.send(session.sessionId, "Page.stopLoading"); - this.#loadingStopped = true; - } else { - await this.stopMainFrame(mainFrame); - } - } + await this.fulfillStopRequest(); await Promise.all( [...this.#targets.values()].map((target) => this.settleTargetForRestore(target), diff --git a/src/browser/PlaywrightScenarioSession.test.ts b/src/browser/PlaywrightScenarioSession.test.ts index b482d681c4..aa099ec109 100644 --- a/src/browser/PlaywrightScenarioSession.test.ts +++ b/src/browser/PlaywrightScenarioSession.test.ts @@ -65,7 +65,7 @@ vi.mock("./PlaywrightScenarioStorage.js", () => ({ vi.mock("./PlaywrightScenarioEvents.js", () => ({ PlaywrightScenarioEvents: class { finish() { - mocks.finishEvents(); + return mocks.finishEvents(); } result() { @@ -137,10 +137,22 @@ const fixture = () => { version: () => "Chrome 154", }); let browserClosed = false; - const close = vi.fn(async (finishEvents?: () => Promise) => { - await finishEvents?.(); - browserClosed = true; - }); + const close = vi.fn( + async ( + finishEvents?: () => Promise, + releaseGate?: { readonly revision: number; run(): Promise }, + ) => { + await finishEvents?.(); + if (releaseGate !== undefined) { + for (;;) { + const revision = releaseGate.revision; + await releaseGate.run(); + if (releaseGate.revision === revision) break; + } + } + browserClosed = true; + }, + ); const cleanup = { close, release: close, @@ -226,6 +238,41 @@ it("stops attached navigation without storage and retries rejected policy cleanu expect(cleanup.close).toHaveBeenCalledTimes(1); }); +it("stops attached navigation when cancellation precedes the first close", async () => { + const { cleanup, page } = fixture(); + mocks.storage = undefined; + const cancellation = new AbortController(); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + storage: {}, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + cancellation.abort(); + + await expect(session.close()).resolves.toBe("disconnected-external"); + expect( + mocks.policySend.mock.calls.filter( + ([method]) => method === "Page.stopLoading", + ), + ).toHaveLength(1); + expect( + page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ), + ).toHaveLength(0); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + it("assigns attached navigation stopping to the storage owner once", async () => { const { cleanup, storage } = fixture(); const attached = browserScenarioSchema.parse({ @@ -318,7 +365,7 @@ it("transfers a late stop request after storage has already released", async () expect(cleanup.close).not.toHaveBeenCalled(); cancellation.abort(); await expect(session.close()).resolves.toBe("disconnected-external"); - expect(storage?.close.mock.calls).toEqual([[false], [true]]); + expect(storage?.close.mock.calls).toEqual([[false]]); expect( mocks.policySend.mock.calls.filter( ([method]) => method === "Page.stopLoading", @@ -327,6 +374,614 @@ it("transfers a late stop request after storage has already released", async () expect(cleanup.close).toHaveBeenCalledTimes(1); }); +it.each(["pending", "settled"] as const)( + "honors a late stop after policy restoration is %s", + async (phase) => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishRestoration: () => void = () => undefined; + let restorationStarted: () => void = () => undefined; + const held = new Promise((resolve) => { + finishRestoration = resolve; + }); + const started = new Promise((resolve) => { + restorationStarted = resolve; + }); + let signalTransportClosed: () => void = () => undefined; + const transportClosed = new Promise((resolve) => { + signalTransportClosed = resolve; + }); + mocks.policyTransportClose.mockImplementation(async () => { + signalTransportClosed(); + }); + mocks.policySend.mockImplementation(async (method: string) => { + if (method === "Runtime.evaluate") { + restorationStarted(); + await held; + } + return policyResponse(method); + }); + + vi.useFakeTimers(); + try { + const firstClose = session.close(false).then( + () => ({ ok: true as const }), + (cause: unknown) => ({ ok: false as const, cause }), + ); + await started; + await vi.advanceTimersByTimeAsync(5_001); + await expect(firstClose).resolves.toMatchObject({ + ok: false, + cause: { cleanupIncomplete: true }, + }); + expect(cleanup.close).not.toHaveBeenCalled(); + + if (phase === "settled") { + finishRestoration(); + await transportClosed; + await vi.advanceTimersByTimeAsync(0); + } + cancellation.abort(); + const retry = session.close(); + if (phase === "pending") { + finishRestoration(); + await transportClosed; + await vi.advanceTimersByTimeAsync(0); + } + await expect(retry).resolves.toBe("disconnected-external"); + } finally { + vi.useRealTimers(); + } + + expect(storage?.close.mock.calls).toEqual([[false]]); + const policyStops = mocks.policySend.mock.calls.filter( + ([method]) => method === "Page.stopLoading", + ).length; + const frameStops = page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ).length; + expect(policyStops + frameStops).toBe(1); + expect(cleanup.close).toHaveBeenCalledTimes(1); + expect(mocks.policyTransportClose).toHaveBeenCalledTimes(1); + }, +); + +it("transfers cancellation to a no-stop cleanup already in progress", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishRestoration: () => void = () => undefined; + let restorationStarted: () => void = () => undefined; + const held = new Promise((resolve) => { + finishRestoration = resolve; + }); + const started = new Promise((resolve) => { + restorationStarted = resolve; + }); + let browserWasOpenAtStop = false; + page.evaluate.mockImplementation(async (script: unknown) => { + if (script === "globalThis.stop()") + browserWasOpenAtStop = cleanup.browserClosed === false; + return null; + }); + mocks.policySend.mockImplementation(async (method: string) => { + if (method === "Runtime.evaluate") { + restorationStarted(); + await held; + } + if (method === "Page.stopLoading") + browserWasOpenAtStop = cleanup.browserClosed === false; + return policyResponse(method); + }); + + const firstClose = session.close(false); + await started; + cancellation.abort(); + const retry = session.close(); + finishRestoration(); + await expect(Promise.all([firstClose, retry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + + const policyStops = mocks.policySend.mock.calls.filter( + ([method]) => method === "Page.stopLoading", + ).length; + const frameStops = page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ).length; + expect(policyStops + frameStops).toBe(1); + expect(cleanup.close).toHaveBeenCalledTimes(1); + expect(browserWasOpenAtStop).toBe(true); +}); + +it("does not repeat a stop confirmed by storage during a late retry", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: true }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishRestoration: () => void = () => undefined; + let restorationStarted: () => void = () => undefined; + const held = new Promise((resolve) => { + finishRestoration = resolve; + }); + const started = new Promise((resolve) => { + restorationStarted = resolve; + }); + mocks.policySend.mockImplementation(async (method: string) => { + if (method === "Runtime.evaluate") { + restorationStarted(); + await held; + } + return policyResponse(method); + }); + + const firstClose = session.close(false); + await started; + cancellation.abort(); + const retry = session.close(); + finishRestoration(); + await expect(Promise.all([firstClose, retry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + + expect(storage?.close).toHaveBeenCalledWith(false); + expect( + mocks.policySend.mock.calls.filter( + ([method]) => method === "Page.stopLoading", + ), + ).toHaveLength(0); + expect( + page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ), + ).toHaveLength(0); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + +it("transfers cancellation arriving during final event cleanup", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishEvents: () => void = () => undefined; + let finishStarted: () => void = () => undefined; + const heldEvents = new Promise((resolve) => { + finishEvents = resolve; + }); + const eventsStarted = new Promise((resolve) => { + finishStarted = resolve; + }); + mocks.finishEvents.mockImplementation(async () => { + finishStarted(); + await heldEvents; + }); + let browserWasOpenAtStop = false; + page.evaluate.mockImplementation(async (script: unknown) => { + if (script === "globalThis.stop()") + browserWasOpenAtStop = cleanup.browserClosed === false; + return null; + }); + + const firstClose = session.close(false); + await eventsStarted; + cancellation.abort(); + const retry = session.close(); + finishEvents(); + await expect(Promise.all([firstClose, retry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + + expect(page.evaluate).toHaveBeenCalledWith("globalThis.stop()"); + expect(browserWasOpenAtStop).toBe(true); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + +it("stops a connected page when service workers are allowed", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "allow" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishEvents: () => void = () => undefined; + let finishStarted: () => void = () => undefined; + const heldEvents = new Promise((resolve) => { + finishEvents = resolve; + }); + const eventsStarted = new Promise((resolve) => { + finishStarted = resolve; + }); + mocks.finishEvents.mockImplementation(async () => { + finishStarted(); + await heldEvents; + }); + let browserWasOpenAtStop = false; + page.evaluate.mockImplementation(async (script: unknown) => { + if (script === "globalThis.stop()") + browserWasOpenAtStop = cleanup.browserClosed === false; + return null; + }); + + const firstClose = session.close(false); + await eventsStarted; + cancellation.abort(); + const retry = session.close(); + finishEvents(); + await expect(Promise.all([firstClose, retry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + + expect(page.evaluate).toHaveBeenCalledTimes(1); + expect(browserWasOpenAtStop).toBe(true); + expect(storage?.close).toHaveBeenCalledWith(false); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + +it("stops a connected page without storage or a service-worker policy owner", async () => { + const { cleanup, page } = fixture(); + mocks.storage = undefined; + const cancellation = new AbortController(); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "allow" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishEvents: () => void = () => undefined; + let finishStarted: () => void = () => undefined; + const heldEvents = new Promise((resolve) => { + finishEvents = resolve; + }); + const eventsStarted = new Promise((resolve) => { + finishStarted = resolve; + }); + mocks.finishEvents.mockImplementation(async () => { + finishStarted(); + await heldEvents; + }); + + const firstClose = session.close(false); + await eventsStarted; + cancellation.abort(); + const retry = session.close(); + finishEvents(); + await expect(Promise.all([firstClose, retry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + + expect(page.evaluate).toHaveBeenCalledWith("globalThis.stop()"); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + +it("retries a failed connected-page stop when service workers are allowed", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "allow" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishEvents: () => void = () => undefined; + let finishStarted: () => void = () => undefined; + const heldEvents = new Promise((resolve) => { + finishEvents = resolve; + }); + const eventsStarted = new Promise((resolve) => { + finishStarted = resolve; + }); + mocks.finishEvents.mockImplementation(async () => { + finishStarted(); + await heldEvents; + }); + const failedFrame = { + isDetached: () => false, + evaluate: vi.fn(async () => { + throw new Error("main frame stop failed"); + }), + } as unknown as ReturnType; + const freshFrame = { + isDetached: () => false, + evaluate: vi.fn(async () => undefined), + } as unknown as ReturnType; + vi.spyOn(page, "mainFrame") + .mockReturnValueOnce(failedFrame) + .mockReturnValue(freshFrame); + + const firstClose = session.close(false); + await eventsStarted; + cancellation.abort(); + const concurrentClose = session.close(); + finishEvents(); + await expect( + Promise.allSettled([firstClose, concurrentClose]), + ).resolves.toEqual( + expect.arrayContaining([ + expect.objectContaining({ status: "rejected" }), + expect.objectContaining({ status: "rejected" }), + ]), + ); + + expect(cleanup.browserClosed).toBe(false); + await expect(session.close()).resolves.toBe("disconnected-external"); + expect(failedFrame.evaluate).toHaveBeenCalledTimes(1); + expect(freshFrame.evaluate).toHaveBeenCalledTimes(1); + expect(storage?.close).toHaveBeenCalledTimes(1); + expect(cleanup.close).toHaveBeenCalledTimes(2); +}); + +it("retains a late fallback stop failure for retry", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishRestoration: () => void = () => undefined; + let restorationStarted: () => void = () => undefined; + const held = new Promise((resolve) => { + finishRestoration = resolve; + }); + const started = new Promise((resolve) => { + restorationStarted = resolve; + }); + let signalTransportClosed: () => void = () => undefined; + const transportClosed = new Promise((resolve) => { + signalTransportClosed = resolve; + }); + mocks.policyTransportClose.mockImplementation(async () => { + signalTransportClosed(); + }); + mocks.policySend.mockImplementation(async (method: string) => { + if (method === "Runtime.evaluate") { + restorationStarted(); + await held; + } + return policyResponse(method); + }); + + vi.useFakeTimers(); + try { + const firstClose = session.close(false).then( + () => ({ ok: true as const }), + (cause: unknown) => ({ ok: false as const, cause }), + ); + await started; + await vi.advanceTimersByTimeAsync(5_001); + await expect(firstClose).resolves.toMatchObject({ ok: false }); + finishRestoration(); + await transportClosed; + await vi.advanceTimersByTimeAsync(0); + + cancellation.abort(); + const stopFailure = new Error("late main-frame stop failed"); + const detachedFrame = { + evaluate: vi.fn(async () => { + throw stopFailure; + }), + isDetached: () => true, + } as unknown as ReturnType; + const freshFrame = { + evaluate: vi.fn(async () => null), + isDetached: () => false, + } as unknown as ReturnType; + vi.spyOn(page, "mainFrame") + .mockReturnValueOnce(detachedFrame) + .mockReturnValue(freshFrame); + const failedRetry = session.close().then( + () => ({ ok: true as const }), + (cause: unknown) => ({ ok: false as const, cause }), + ); + await expect(failedRetry).resolves.toMatchObject({ + ok: false, + cause: { cleanupIncomplete: true }, + }); + expect(cleanup.close).not.toHaveBeenCalled(); + + await expect(session.close()).resolves.toBe("disconnected-external"); + expect(detachedFrame.evaluate).toHaveBeenCalledWith("globalThis.stop()"); + expect(freshFrame.evaluate).toHaveBeenCalledWith("globalThis.stop()"); + } finally { + vi.useRealTimers(); + } + + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + +it("coalesces concurrent late-stop close retries", async () => { + const { cleanup, page, storage } = fixture(); + const cancellation = new AbortController(); + storage?.close.mockResolvedValue({ loadingStopped: false }); + const attached = browserScenarioSchema.parse({ + ...scenario, + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "target", + }, + environment: { service_workers: "block" }, + }); + const session = await PlaywrightScenarioSession.open( + attached, + {}, + { signal: cancellation.signal }, + ); + let finishRestoration: () => void = () => undefined; + let restorationStarted: () => void = () => undefined; + const held = new Promise((resolve) => { + finishRestoration = resolve; + }); + const started = new Promise((resolve) => { + restorationStarted = resolve; + }); + let signalTransportClosed: () => void = () => undefined; + const transportClosed = new Promise((resolve) => { + signalTransportClosed = resolve; + }); + mocks.policyTransportClose.mockImplementation(async () => { + signalTransportClosed(); + }); + mocks.policySend.mockImplementation(async (method: string) => { + if (method === "Runtime.evaluate") { + restorationStarted(); + await held; + } + return policyResponse(method); + }); + + vi.useFakeTimers(); + try { + const firstClose = session.close(false).then( + () => ({ ok: true as const }), + (cause: unknown) => ({ ok: false as const, cause }), + ); + await started; + await vi.advanceTimersByTimeAsync(5_001); + await expect(firstClose).resolves.toMatchObject({ ok: false }); + finishRestoration(); + await transportClosed; + await vi.advanceTimersByTimeAsync(0); + + cancellation.abort(); + let finishStop: () => void = () => undefined; + let stopStarted: () => void = () => undefined; + const heldStop = new Promise((resolve) => { + finishStop = resolve; + }); + const startedStop = new Promise((resolve) => { + stopStarted = resolve; + }); + page.evaluate.mockImplementation(async (script: unknown) => { + if (script === "globalThis.stop()") { + stopStarted(); + await heldStop; + } + return null; + }); + const firstRetry = session.close(); + await startedStop; + const secondRetry = session.close(); + expect( + page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ), + ).toHaveLength(1); + finishStop(); + await expect(Promise.all([firstRetry, secondRetry])).resolves.toEqual([ + "disconnected-external", + "disconnected-external", + ]); + } finally { + vi.useRealTimers(); + } + + expect( + page.evaluate.mock.calls.filter( + ([script]) => script === "globalThis.stop()", + ), + ).toHaveLength(1); + expect(cleanup.close).toHaveBeenCalledTimes(1); +}); + it("retains transferred navigation stopping after setup cleanup fails", async () => { const { cleanup, storage } = fixture(); const cancellation = new AbortController(); diff --git a/src/browser/PlaywrightScenarioSession.ts b/src/browser/PlaywrightScenarioSession.ts index dca2896280..031d5b9372 100644 --- a/src/browser/PlaywrightScenarioSession.ts +++ b/src/browser/PlaywrightScenarioSession.ts @@ -13,6 +13,7 @@ import { BrowserScenarioSecrets } from "./BrowserScenarioSecrets.js"; import { failBrowserScenarioOperation, openPlaywrightScenarioBrowser, + type PlaywrightScenarioBrowserReleaseGate, type OpenedScenarioBrowser, } from "./PlaywrightScenarioBrowser.js"; import { performPlaywrightScenarioAction } from "./PlaywrightScenarioActions.js"; @@ -83,9 +84,14 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { private readonly serviceWorkers: PlaywrightScenarioServiceWorkers | undefined; private readonly signal: AbortSignal | undefined; #storageReleased = false; + #storageCloseCompleted = false; + #storageStopConfirmed = false; #stopLoading = false; + #stopRevision = 0; readonly cleanupLease: AnalysisCleanupLease; - #policyStopsLoading = false; + #navigationStopConfirmed = false; + readonly #browserReleaseGate: PlaywrightScenarioBrowserReleaseGate; + readonly #onAbort = (): void => this.#recordStopRequest(); private constructor( private readonly opened: OpenedScenarioBrowser, @@ -106,6 +112,15 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { this.storage = options.storage; this.serviceWorkers = options.serviceWorkers; this.signal = options.signal; + const session = this; + this.#browserReleaseGate = { + get revision() { + return session.#stopRevision; + }, + run: () => this.#fulfillNavigationStopRequest(), + }; + if (this.signal?.aborted === true) this.#recordStopRequest(); + else this.signal?.addEventListener("abort", this.#onAbort, { once: true }); this.version = opened.browser.version(); this.initialUrl = opened.page.url(); this.cleanupLease = new AnalysisCleanupLease( @@ -457,7 +472,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } async close(stopLoading = this.signal?.aborted === true) { - this.#stopLoading ||= stopLoading; + if (stopLoading) this.#recordStopRequest(); try { await withPlaywrightExecutionBoundary( () => this.cleanupLease.release(), @@ -480,31 +495,46 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } async #closeResources(): Promise { - const stopLoading = this.#stopLoading || this.signal?.aborted === true; + const stopLoading = this.#stopLoading; let storageFailure: { readonly cause: unknown } | undefined; let policyFailure: { readonly cause: unknown } | undefined; let browserFailure: { readonly cause: unknown } | undefined; - if (stopLoading && this.storage === undefined) - this.#policyStopsLoading = true; - if (!this.#storageReleased) { + if (!this.#storageCloseCompleted) { try { const result = await this.storage?.close(stopLoading); - if (stopLoading && result?.loadingStopped !== true) - this.#policyStopsLoading = true; + this.#storageStopConfirmed = result?.loadingStopped === true; + this.#storageCloseCompleted = true; } catch (failure: unknown) { storageFailure = { cause: failure }; - if (stopLoading) this.#policyStopsLoading = true; } } try { if (!this.opened.cleanup.browserClosed) - await this.serviceWorkers?.close(this.#policyStopsLoading); + await this.serviceWorkers?.close( + stopLoading && !this.#storageStopConfirmed, + ); + if ( + stopLoading && + !this.#storageStopConfirmed && + this.serviceWorkers !== undefined + ) + this.#navigationStopConfirmed = true; } catch (failure: unknown) { policyFailure = { cause: failure }; } if (policyFailure === undefined) { try { - await this.opened.cleanup.release(() => this.eventCapture.finish()); + await this.#fulfillNavigationStopRequest(); + } catch (failure: unknown) { + policyFailure = { cause: failure }; + } + } + if (policyFailure === undefined) { + try { + await this.opened.cleanup.release( + () => this.eventCapture.finish(), + this.#browserReleaseGate, + ); } catch (failure: unknown) { browserFailure = { cause: failure }; } @@ -515,7 +545,10 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { browserFailure = { cause: failure }; } } - if (this.opened.cleanup.browserClosed) this.#storageReleased = true; + if (this.opened.cleanup.browserClosed) { + this.#storageReleased = true; + this.signal?.removeEventListener("abort", this.#onAbort); + } throwCleanupFailures([ ...(policyFailure === undefined ? [] : [policyFailure.cause]), ...(this.#storageReleased || storageFailure === undefined @@ -525,6 +558,33 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { ]); } + #recordStopRequest(): void { + if (!this.#stopLoading) this.#stopRevision += 1; + this.#stopLoading = true; + } + + async #fulfillNavigationStopRequest(): Promise { + if ( + !this.#stopLoading || + this.#storageStopConfirmed || + this.#navigationStopConfirmed || + this.mode === "launch" + ) + return; + if (this.serviceWorkers !== undefined) { + await this.serviceWorkers.close(true); + } else if (this.opened.page.isClosed()) { + this.#navigationStopConfirmed = true; + return; + } else { + const frame = this.opened.page.mainFrame(); + if (frame.isDetached()) + throw new Error("Cannot stop navigation through a detached main frame"); + await frame.evaluate("globalThis.stop()"); + } + this.#navigationStopConfirmed = true; + } + redactError(error: unknown): string { return this.secrets.redact( error instanceof Error ? error.message : "browser action failed",