diff --git a/src/application/BrowserScenarioCaptureService.test.ts b/src/application/BrowserScenarioCaptureService.test.ts index 9645358d6a..df187682a4 100644 --- a/src/application/BrowserScenarioCaptureService.test.ts +++ b/src/application/BrowserScenarioCaptureService.test.ts @@ -76,3 +76,30 @@ it("converts a raw scenario capture retained after cleanup failure", async () => }); } }); + +it.each([ + null, + { request: "opaque cancellation" }, + new BrowserObservationError("capture_browser_scenario", "protocol_error"), +])( + "preserves the exact pre-admission caller reason without dispatch: %j", + async (reason) => { + const caller = new AbortController(); + caller.abort(reason); + let calls = 0; + const provider: BrowserScenarioCapturePort = { + identity: () => ({ id: "fixture", name: "fixture", version: "1" }), + captureScenario: async () => { + calls++; + throw new Error("Unexpected dispatch"); + }, + }; + const result = await captureBrowserScenario(provider, scenario, { + signal: caller.signal, + }); + if (result.ok) throw new Error("Expected cancellation"); + expect(result.error).toBeInstanceOf(AnalysisCancelledError); + expect(result.error.cause).toBe(reason); + expect(calls).toBe(0); + }, +); diff --git a/src/application/BrowserScenarioCaptureService.ts b/src/application/BrowserScenarioCaptureService.ts index 9573c53fd4..0a5bc26019 100644 --- a/src/application/BrowserScenarioCaptureService.ts +++ b/src/application/BrowserScenarioCaptureService.ts @@ -31,7 +31,9 @@ export const captureBrowserScenario = async ( ), ); if (options.signal?.aborted) - return err(new AnalysisCancelledError(OPERATION)); + return err( + new AnalysisCancelledError(OPERATION, { cause: options.signal.reason }), + ); const captured = await provider.captureScenario(scenario, options); if (!captured.ok) { const partial = captured.error.partialObservation; diff --git a/src/browser/BrowserScenarioPartialObservation.ts b/src/browser/BrowserScenarioPartialObservation.ts index cc83ed1552..42d933d53e 100644 --- a/src/browser/BrowserScenarioPartialObservation.ts +++ b/src/browser/BrowserScenarioPartialObservation.ts @@ -10,12 +10,8 @@ import type { } from "../domain/browserScenarioCaptureValues.js"; import type { BrowserScenarioSessionPort } from "./BrowserScenarioSessionPort.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; -import { - AnalysisCancelledError, - AnalysisTimeoutError, -} from "../domain/analysisErrorCore.js"; -import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; -import { BrowserObservationError } from "../domain/browserObservationError.js"; +import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { ProviderAdapterError } from "../domain/providerAdapterError.js"; import { PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY } from "./providerIdentities.js"; @@ -136,68 +132,22 @@ export const browserScenarioOperationFailure = ( OPERATION, { cause }, ); - const primaryProjection = projectAnalysisError(primary); - const cleanupCause = - cleanupFailure === undefined - ? undefined - : cleanupFailure.cause instanceof AnalysisError - ? cleanupFailure.cause - : new ProviderAdapterError( - PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, - "close_browser_scenario", - { cause: cleanupFailure.cause }, - ); - const cleanupObservation = - cleanupFailure === undefined - ? undefined - : browserScenarioCleanupObservation(cleanupFailure.cause); - const retainedOptions = { - cause: primary, - partialObservation, - ...(primary.capturedOutput === undefined - ? {} - : { capturedOutput: primary.capturedOutput }), - ...(primary.cleanup === undefined ? {} : { cleanup: primary.cleanup }), - }; - if (cleanupCause === undefined) { - if (primary instanceof AnalysisCancelledError) - return new AnalysisCancelledError(primary.operation, retainedOptions); - if (primary instanceof AnalysisTimeoutError) - return new AnalysisTimeoutError( - primary.operation, - primary.timeoutMs, - retainedOptions, - ); - if (primary instanceof BrowserObservationError) - return new BrowserObservationError(primary.operation, primary.reason, { - ...retainedOptions, - ...(primary.userMessage === undefined - ? {} - : { detail: primary.userMessage }), - }); - } - const cleanup = cleanupObservation ?? primary.cleanup; - return new ProviderAdapterError( - PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + primary.retainPartialObservation(partialObservation); + if (cleanupFailure === undefined) return primary; + const observation = browserScenarioCleanupObservation(cleanupFailure.cause); + const cleanup = + cleanupFailure.cause instanceof AnalysisError && + cleanupFailure.cause.cleanupIncomplete + ? cleanupFailure.cause + : new ProviderCleanupError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + observation.resources, + { reason: observation.reason }, + { operation: "close_browser_scenario", cause: cleanupFailure.cause }, + ); + return analysisErrorWithCleanupFailure( + primary, + cleanup, OPERATION, - { - ...retainedOptions, - cause: - cleanupCause === undefined - ? primary - : new AggregateError( - [primary, cleanupCause], - "Browser scenario operation and cleanup both failed", - { cause: primary }, - ), - userMessage: primaryProjection.message, - ...(cleanup === undefined ? {} : { cleanup }), - diagnostics: { - primary_error: primaryProjection, - ...(cleanupCause === undefined - ? {} - : { cleanup_error: projectAnalysisError(cleanupCause) }), - }, - }, - ); + ).bindCleanupDependencies([primary, cleanup]); }; diff --git a/src/browser/BrowserScenarioSessionPort.ts b/src/browser/BrowserScenarioSessionPort.ts index d0cf7f3a78..9079149e5e 100644 --- a/src/browser/BrowserScenarioSessionPort.ts +++ b/src/browser/BrowserScenarioSessionPort.ts @@ -1,3 +1,4 @@ +import type { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import type { BrowserScenario } from "../domain/browserScenario.js"; import type { BrowserScenarioAction } from "../domain/browserScenarioValues.js"; import type { @@ -33,6 +34,7 @@ export interface BrowserScenarioSessionPort { requested: ReadonlySet, signal?: AbortSignal, ): Promise; + readonly cleanupLease: AnalysisCleanupLease; close(): Promise<"terminated-owned-process" | "disconnected-external">; redactError(error: unknown): string; } @@ -42,6 +44,6 @@ export type BrowserScenarioSessionOpener = ( scenario: BrowserScenario, options: { readonly signal?: AbortSignal; - readonly retainCleanup?: (close: () => Promise) => void; + readonly retainCleanup?: (lease: AnalysisCleanupLease) => void; }, ) => Promise; diff --git a/src/browser/PlaywrightBrowserScenarioProvider.test.ts b/src/browser/PlaywrightBrowserScenarioProvider.test.ts index 97ed96da05..5a976a565c 100644 --- a/src/browser/PlaywrightBrowserScenarioProvider.test.ts +++ b/src/browser/PlaywrightBrowserScenarioProvider.test.ts @@ -1,3 +1,5 @@ +import { failBrowserScenarioOperation } from "./PlaywrightScenarioBrowser.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; // Fake-backed composition coverage; real browser verification lives in // `npm run verify:browser`. import { describe, expect, it, vi } from "vitest"; @@ -117,6 +119,9 @@ class FakeSession implements BrowserScenarioSessionPort { readonly version = "1"; readonly initialUrl = "about:blank"; readonly processOwnership: "provider-owned" | "external"; + cleanupLease = new AnalysisCleanupLease(async () => { + await this.close(); + }, ["browser_transport"]); closeCalls = 0; performCalls = 0; captureCalls = 0; @@ -575,18 +580,24 @@ describe("PlaywrightBrowserScenarioProvider operation failures", () => { expect(projection).toMatchObject({ code: "cleanup_incomplete", details: { - diagnostics: { - primary_error: { - code: "cleanup_incomplete", - details: { - operation: "capture_browser_scenario", - reason: "step capture failed", - }, - }, - ...(cleanupFails - ? { cleanup_error: { code: "execution_failure" } } - : {}), - }, + ...(cleanupFails + ? { + diagnostics: { + primary_error: { + code: "cleanup_incomplete", + details: { + operation: "capture_browser_scenario", + diagnostics: { + primary_error: { + details: { reason: "step capture failed" }, + }, + }, + }, + }, + cleanup_error: { code: "cleanup_incomplete" }, + }, + } + : { reason: "step capture failed" }), partial_observation: { kind: "browser-scenario-observation", capture: { @@ -617,17 +628,16 @@ describe("PlaywrightBrowserScenarioProvider operation failures", () => { stderr: "", truncated: false, }); - expect(result.error.cleanup).toEqual( + expect(result.error.cleanupResources).toEqual( cleanupFails - ? { - reason: "browser close failed", - resources: ["browser_transport"], - } - : { - reason: "capture cleanup is uncertain", - resources: ["capture_handle"], - }, + ? ["capture_handle", "browser_transport"] + : ["capture_handle"], ); + if (!cleanupFails) + expect(result.error.cleanup).toEqual({ + reason: "capture cleanup is uncertain", + resources: ["capture_handle"], + }); expect(session.closeCalls).toBe(1); }, ); @@ -755,7 +765,7 @@ describe("PlaywrightBrowserScenarioProvider request cancellation", () => { expect(result.error.cause).toBe(cancellationReason); }); - it("keeps cleanup failure primary when cancellation arrives during a failed close", async () => { + it("retains final-close cancellation alongside failed cleanup", async () => { const controller = new AbortController(); const session = new FakeSession("launch", { close: new Error("fixture close failed"), @@ -772,8 +782,8 @@ describe("PlaywrightBrowserScenarioProvider request cancellation", () => { expect(result).toMatchObject({ ok: false, error: { - _tag: "BrowserObservationError", - reason: "cleanup_failed", + _tag: "ProviderAdapterError", + cleanupIncomplete: true, partialObservation: { kind: "browser-scenario-observation", capture: { @@ -783,6 +793,13 @@ describe("PlaywrightBrowserScenarioProvider request cancellation", () => { }, }, }); + if (result.ok) + throw new Error("Expected final-close cancellation with cleanup"); + let primary = result.error.cleanupPrimary; + while (primary?.cleanupPrimary !== undefined) + primary = primary.cleanupPrimary; + expect(primary).toMatchObject({ _tag: "AnalysisCancelledError" }); + expect(primary?.cause).toBe(controller.signal.reason); await expect(provider.close()).rejects.toMatchObject({ _tag: "ProviderAdapterError", operation: "capture_browser_scenario", @@ -893,3 +910,320 @@ it.each(["launch", "connect"] as const)( } }, ); + +it.each([false, true])( + "follows the browser owner through provider recovery (operation failed: %s)", + async (operationFailed) => { + const reason = { caller: "authored capture cancellation" }; + const primary = new AnalysisCancelledError("capture_browser_scenario", { + cause: reason, + }); + const session = new FakeSession( + "launch", + operationFailed ? { capture: { call: 1, error: primary } } : {}, + ); + let denied = true; + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser: async () => {}, + removeProfile: async () => { + if (denied) throw new Error("profile removal denied"); + }, + }); + session.cleanupLease = owner.cleanupLease; + session.close = async () => { + await owner.close(); + return "terminated-owned-process"; + }; + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + try { + const result = await provider.captureScenario(scenario()); + if (result.ok) throw new Error("Expected retained cleanup"); + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toEqual(["browser_profile"]); + const partial = result.error.partialObservation; + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + denied = false; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.cleanupResources).toEqual([]); + expect(result.error.partialObservation).toBe(partial); + if (operationFailed) { + let restored = result.error.cleanupPrimary; + while (restored?.cleanupPrimary !== undefined) + restored = restored.cleanupPrimary; + expect(restored).toBe(primary); + expect(primary.cause).toBe(reason); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cancelled", + }); + } + } finally { + denied = false; + await provider.close(); + } + }, +); + +it("resolves the returned failure after retrying a plain session cleanup capability", async () => { + const session = new FakeSession("launch"); + let denied = true; + session.close = async () => { + if (denied) throw new Error("authored session cleanup denial"); + return "terminated-owned-process"; + }; + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + try { + const result = await provider.captureScenario(scenario()); + if (result.ok) throw new Error("Expected cleanup failure"); + expect(result.error.cleanupIncomplete).toBe(true); + const failed: unknown = await provider + .close() + .catch((cause: unknown) => cause); + expect(failed).toMatchObject({ cleanupIncomplete: true }); + denied = false; + await provider.close(); + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(failed).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + } finally { + denied = false; + await provider.close(); + } +}); + +it("retries every retained opening cleanup and preserves the original failure through mixed recovery", async () => { + let denied = true; + let healthyCloses = 0; + const physical = new AnalysisOutputError( + "capture_browser_scenario", + "authored open failure", + ); + const provider = new PlaywrightBrowserScenarioProvider( + async (_scenario, options) => { + options.retainCleanup?.( + new AnalysisCleanupLease(async () => { + if (denied) throw new Error("first cleanup denied"); + }, ["first cleanup"]), + ); + options.retainCleanup?.( + new AnalysisCleanupLease(async () => { + healthyCloses++; + }, ["healthy cleanup"]), + ); + throw physical; + }, + ); + try { + const result = await provider.captureScenario(scenario()); + if (result.ok) throw new Error("Expected opening failure"); + const failed: unknown = await provider + .close() + .catch((cause: unknown) => cause); + expect(failed).toMatchObject({ cleanupIncomplete: true }); + expect(result.error.cleanupIncomplete).toBe(true); + expect(healthyCloses).toBe(1); + denied = false; + await provider.close(); + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(failed).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(result.error.cleanupPrimary).toBe(physical); + expect(healthyCloses).toBe(1); + } finally { + denied = false; + await provider.close(); + } +}); + +it("keeps a cancelled opening incomplete until its retained late cleanup finishes", async () => { + let finish: () => void = () => {}; + const late = new Promise((resolve) => { + finish = resolve; + }); + const reason = { caller: "cancelled opening before late handshake" }; + const primary = new AnalysisCancelledError("capture_browser_scenario", { + cause: reason, + }); + const provider = new PlaywrightBrowserScenarioProvider( + async (_scenario, options) => { + options.retainCleanup?.( + new AnalysisCleanupLease(() => late, ["late opening"]), + ); + throw primary; + }, + ); + try { + const result = await provider.captureScenario(scenario()); + if (result.ok) throw new Error("Expected cancelled opening"); + expect(result.error.cleanupIncomplete).toBe(true); + const closing = provider.close(); + await Promise.resolve(); + expect(result.error.cleanupIncomplete).toBe(true); + finish(); + await closing; + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(result.error.cleanupPrimary).toBe(primary); + expect(primary.cause).toBe(reason); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cancelled", + }); + } finally { + finish(); + await provider.close(); + } +}); + +it.each([false, true])( + "follows late browser acknowledgement without another retry (cancelled during close: %s)", + async (cancelled) => { + vi.useFakeTimers(); + let acknowledge: () => void = () => {}; + const acknowledgement = new Promise((resolve) => { + acknowledge = resolve; + }); + const caller = new AbortController(); + const reason = { caller: "during final browser close" }; + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser: () => acknowledgement, + removeProfile: undefined, + }); + const session = new FakeSession("launch"); + session.cleanupLease = owner.cleanupLease; + session.close = async () => { + if (cancelled) caller.abort(reason); + await owner.close(); + return "terminated-owned-process"; + }; + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + try { + const pending = provider.captureScenario(scenario(), { + signal: caller.signal, + }); + await vi.advanceTimersByTimeAsync(30_000); + const result = await pending; + if (result.ok) throw new Error("Expected timed out cleanup response"); + expect(result.error.cleanupIncomplete).toBe(true); + const partial = result.error.partialObservation; + acknowledge(); + await owner.cleanupLease.release(); + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(result.error.partialObservation).toBe(partial); + if (cancelled) { + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cancelled", + }); + let primary = result.error.cleanupPrimary; + while (primary?.cleanupPrimary !== undefined) + primary = primary.cleanupPrimary; + expect(primary?.cause).toBe(reason); + } + } finally { + acknowledge(); + await provider.close(); + vi.useRealTimers(); + } + }, +); + +it("retains a profile-only opening lease without adding transport labels or waiting for another retry", async () => { + let denied = true; + let releases = 0; + const primary = new AnalysisOutputError( + "capture_browser_scenario", + "authored opening failure", + ); + const lease = new AnalysisCleanupLease(async () => { + releases++; + if (denied) throw new Error("profile removal denied"); + }, ["browser_profile"]); + const provider = new PlaywrightBrowserScenarioProvider( + async (_scenario, options) => { + try { + return await failBrowserScenarioOperation(lease, primary); + } catch (cause: unknown) { + options.retainCleanup?.(lease); + throw cause; + } + }, + ); + try { + const result = await provider.captureScenario(scenario()); + if (result.ok) throw new Error("Expected opening failure"); + expect(result.error).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_profile"], + }); + denied = false; + await lease.release(); + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "unreadable_output", + }); + await provider.close(); + expect(releases).toBe(2); + } finally { + denied = false; + await provider.close(); + } +}); + +it("keeps an earlier physical failure primary when final cleanup also observes cancellation", async () => { + const caller = new AbortController(); + const physical = new Error("physical capture failure"); + const primary = new AnalysisOutputError( + "capture_browser_scenario", + physical.message, + { cause: physical }, + ); + const session = new FakeSession("launch", { + capture: { call: 1, error: primary }, + }); + let denied = true; + session.close = async () => { + caller.abort({ caller: "later during final cleanup" }); + if (denied) throw new Error("cleanup denied"); + return "terminated-owned-process"; + }; + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + try { + const result = await provider.captureScenario(scenario(), { + signal: caller.signal, + }); + if (result.ok) throw new Error("Expected physical failure"); + denied = false; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "unreadable_output", + }); + let restored = result.error.cleanupPrimary; + while (restored?.cleanupPrimary !== undefined) + restored = restored.cleanupPrimary; + expect(restored).toBe(primary); + expect(primary.cause).toBe(physical); + } finally { + denied = false; + await provider.close(); + } +}); diff --git a/src/browser/PlaywrightBrowserScenarioProvider.ts b/src/browser/PlaywrightBrowserScenarioProvider.ts index a4150263e1..65e8fb67cc 100644 --- a/src/browser/PlaywrightBrowserScenarioProvider.ts +++ b/src/browser/PlaywrightBrowserScenarioProvider.ts @@ -1,3 +1,13 @@ +import { + withPlaywrightExecutionBoundary, + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, +} from "./PlaywrightExecutionBoundary.js"; +import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import type { BrowserScenarioCapturePort } from "../application/BrowserScenarioCapturePort.js"; import type { ExecutionOptions, @@ -233,10 +243,12 @@ const runScenario = async ( openSession: BrowserScenarioSessionOpener, scenario: BrowserScenario, options: ExecutionOptions, - retainCleanup: (close: () => Promise) => void, + retainCleanup: (lease: AnalysisCleanupLease) => void, ): Promise => { if (options.signal?.aborted) - throw new BrowserObservationError(OPERATION, "cancelled"); + throw new BrowserObservationError(OPERATION, "cancelled", { + cause: options.signal.reason, + }); const startedAt = performance.now(); const session = await openSession(scenario, { ...(options.signal === undefined ? {} : { signal: options.signal }), @@ -283,10 +295,23 @@ const runScenario = async ( try { cleanupOutcome = { ok: true, cleanup: await session.close() }; } catch (cause: unknown) { - cleanupOutcome = { ok: false, cause }; - retainCleanup(() => session.close()); + const cleanup = browserScenarioCleanupObservation(cause); + const failure = new BrowserObservationError(OPERATION, "cleanup_failed", { + cause, + cleanup, + }); + retainCleanup(session.cleanupLease); + new AnalysisCleanupLeaseGroup(failure, [session.cleanupLease]); + cleanupOutcome = { ok: false, cause: failure }; } if (!cleanupOutcome.ok) { + if (operationOutcome.ok && options.signal?.aborted) + operationOutcome = { + ok: false, + cause: new AnalysisCancelledError(OPERATION, { + cause: options.signal.reason, + }), + }; const cleanup = browserScenarioCleanupObservation(cleanupOutcome.cause); const partialObservation: BrowserScenarioPartialObservation = { kind: "browser-scenario-observation", @@ -307,12 +332,18 @@ const runScenario = async ( partialObservation, { cause: cleanupOutcome.cause }, ); - throw new BrowserObservationError(OPERATION, "cleanup_failed", { + const failure = new BrowserObservationError(OPERATION, "cleanup_failed", { cause: cleanupOutcome.cause, cleanup, partialObservation, detail: `Browser scenario cleanup could not be confirmed: ${cleanup.reason}. Collected steps and events are retained in partial_observation. Review the reported resources before retrying.`, }); + if ( + cleanupOutcome.cause instanceof AnalysisError && + cleanupOutcome.cause.cleanupIncomplete + ) + failure.bindCleanupDependencies([cleanupOutcome.cause]); + throw failure; } if (!operationOutcome.ok) throw browserScenarioOperationFailure(operationOutcome.cause, { @@ -361,7 +392,7 @@ export class PlaywrightBrowserScenarioProvider implements BrowserScenarioCapture #tail: Promise = Promise.resolve(); #closing = false; #closePromise: Promise | undefined; - readonly #pendingCleanup = new Set<() => Promise>(); + readonly #pendingCleanup = new Set(); constructor(private readonly openSession: BrowserScenarioSessionOpener) {} @@ -392,20 +423,37 @@ export class PlaywrightBrowserScenarioProvider implements BrowserScenarioCapture ): Promise> { const pending = await this.#retryCleanup(); if (pending !== undefined) return err(pending); + const retained: AnalysisCleanupLease[] = []; try { return ok( - await runScenario(this.openSession, scenario, options, (close) => { - this.#pendingCleanup.add(close); + await runScenario(this.openSession, scenario, options, (lease) => { + this.#pendingCleanup.add(lease); + retained.push(lease); }), ); } catch (cause: unknown) { - if (cause instanceof AnalysisError) return err(cause); + const primary = + cause instanceof AnalysisError + ? cause + : new ProviderAdapterError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + OPERATION, + { cause }, + ); + if (retained.length === 0) return err(primary); + const cleanup = new ProviderCleanupError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + retained.flatMap((lease) => lease.cleanupResources ?? []), + { reason: "Browser scenario cleanup capabilities remain retained." }, + { operation: OPERATION, cause }, + ); + new AnalysisCleanupLeaseGroup(cleanup, retained); return err( - new ProviderAdapterError( - PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + analysisErrorWithCleanupFailure( + primary, + cleanup, OPERATION, - { cause }, - ), + ).bindCleanupDependencies([primary, cleanup]), ); } } @@ -432,20 +480,35 @@ export class PlaywrightBrowserScenarioProvider implements BrowserScenarioCapture } async #retryCleanup(): Promise { + const failures: AnalysisError[] = []; for (const pending of this.#pendingCleanup) { try { - await pending(); + await withPlaywrightExecutionBoundary( + () => pending.release(), + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, + ); this.#pendingCleanup.delete(pending); } catch (cause: unknown) { - return cause instanceof AnalysisError - ? cause - : new ProviderAdapterError( - PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, - OPERATION, - { cause }, - ); + const failure = new ProviderCleanupError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + pending.cleanupResources ?? [], + { reason: cause instanceof Error ? cause.message : String(cause) }, + { operation: OPERATION, cause }, + ); + new AnalysisCleanupLeaseGroup(failure, [pending]); + failures.push(failure); } } - return undefined; + if (failures.length === 0) return undefined; + if (failures.length === 1) return failures[0]; + return new ProviderCleanupError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + failures.flatMap((failure) => failure.cleanupResources), + { reason: "One or more browser scenario resources remain owned." }, + { + operation: OPERATION, + cause: new AggregateError(failures, "Browser scenario cleanup failed"), + }, + ).bindCleanupDependencies(failures); } } diff --git a/src/browser/PlaywrightExecutionBoundary.test.ts b/src/browser/PlaywrightExecutionBoundary.test.ts index 51df817012..4c15a06779 100644 --- a/src/browser/PlaywrightExecutionBoundary.test.ts +++ b/src/browser/PlaywrightExecutionBoundary.test.ts @@ -51,3 +51,23 @@ describe("withPlaywrightExecutionBoundary", () => { ).resolves.toBe("done"); }); }); + +it.each(["before", "during"] as const)( + "keeps typed caller reasons opaque when cancelled %s work", + async (phase) => { + const caller = new AbortController(); + const reason = new AnalysisTimeoutError("capture_browser_scenario", 123); + if (phase === "before") caller.abort(reason); + const pending = withPlaywrightExecutionBoundary( + () => new Promise(() => {}), + undefined, + caller.signal, + ); + if (phase === "during") caller.abort(reason); + const error: unknown = await pending.catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(AnalysisCancelledError); + if (!(error instanceof AnalysisCancelledError)) + throw new Error("Expected typed cancellation"); + expect(error.cause).toBe(reason); + }, +); diff --git a/src/browser/PlaywrightExecutionBoundary.ts b/src/browser/PlaywrightExecutionBoundary.ts index 759fcd4847..b500573f9a 100644 --- a/src/browser/PlaywrightExecutionBoundary.ts +++ b/src/browser/PlaywrightExecutionBoundary.ts @@ -5,6 +5,9 @@ import { const OPERATION = "capture_browser_scenario" as const; +// Chrome shutdown can take several seconds; expiry bounds the response, not ownership. +export const PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS = 30_000; + export const withPlaywrightExecutionBoundary = ( work: () => Promise, timeoutMs: number | undefined, @@ -13,7 +16,9 @@ export const withPlaywrightExecutionBoundary = ( if (timeoutMs !== undefined && timeoutMs <= 0) return Promise.reject(new AnalysisTimeoutError(OPERATION, timeoutMs)); if (signal?.aborted === true) - return Promise.reject(new AnalysisCancelledError(OPERATION)); + return Promise.reject( + new AnalysisCancelledError(OPERATION, { cause: signal.reason }), + ); return new Promise((resolve, reject) => { let settled = false; @@ -47,7 +52,7 @@ export const withPlaywrightExecutionBoundary = ( reject(error); }; const onAbort = (): void => { - fail(new AnalysisCancelledError(OPERATION)); + fail(new AnalysisCancelledError(OPERATION, { cause: signal?.reason })); }; scheduleTimeout(); signal?.addEventListener("abort", onAbort, { once: true }); diff --git a/src/browser/PlaywrightScenarioBrowser.test.ts b/src/browser/PlaywrightScenarioBrowser.test.ts index 36b9e753e1..cd2b954809 100644 --- a/src/browser/PlaywrightScenarioBrowser.test.ts +++ b/src/browser/PlaywrightScenarioBrowser.test.ts @@ -1,4 +1,11 @@ +import { AnalysisError } from "../domain/analysisErrorBase.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; +import { AnalysisCancelledError } from "../domain/analysisErrorCore.js"; +import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { afterEach, expect, it, vi } from "vitest"; +import { mkdir, writeFile, rm, access } from "node:fs/promises"; +import { join } from "node:path"; +import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; import type { Browser, BrowserContext, @@ -9,6 +16,7 @@ import type { import { browserScenarioSchema } from "../domain/browserScenario.js"; import { attachedScenarioLeaseKey, + failBrowserScenarioOperation, openPlaywrightScenarioBrowser, PlaywrightScenarioBrowserCleanupOwner, } from "./PlaywrightScenarioBrowser.js"; @@ -135,6 +143,10 @@ it("keeps attached-target ownership after the cleanup response timeout", async ( onSettled: (browserClosed) => settled.push(browserClosed), }); const timedOutClose = cleanup.close(); + let timedOutFailure: unknown; + void timedOutClose.catch((cause: unknown) => { + timedOutFailure = cause; + }); const timeoutExpectation = expect(timedOutClose).rejects.toMatchObject({ reason: "cleanup_failed", cleanup: { reason: expect.stringContaining("timed out") }, @@ -155,6 +167,10 @@ it("keeps attached-target ownership after the cleanup response timeout", async ( closeResult.resolve(undefined); await cleanup.close(); expect(settled).toEqual([true]); + expect(timedOutFailure).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); } finally { vi.useRealTimers(); } @@ -675,3 +691,93 @@ it("serializes concurrent attach across loopback spellings of one browser", asyn await expect(second).rejects.toThrow("second connection failed"); expect(endpoints).toEqual(["http://127.0.0.1:9222", "http://[::1]:9222"]); }); + +it("tracks only pending browser cleanup resources and resolves prior errors after exact recovery", async () => { + const root = await createTestTempDirectory("rea-browser-cleanup-recovery-"); + const profile = join(root, "profile"); + await mkdir(profile); + await writeFile(join(profile, "authored-data"), "profile control"); + let browserDenied = true; + let profileDenied = true; + let browserCloses = 0; + const owner = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser: async () => { + browserCloses++; + if (browserDenied) throw new Error("controlled transport close failure"); + }, + removeProfile: async () => { + if (profileDenied) throw new Error("controlled profile removal failure"); + await rm(profile, { recursive: true }); + }, + }); + try { + const first: unknown = await owner.close().catch((cause: unknown) => cause); + expect(first).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_transport", "browser_profile"], + }); + browserDenied = false; + const second: unknown = await owner + .close() + .catch((cause: unknown) => cause); + expect(owner.browserClosed).toBe(true); + for (const error of [first, second]) + expect(error).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_profile"], + }); + await access(profile); + profileDenied = false; + await owner.close(); + await expect(access(profile)).rejects.toMatchObject({ code: "ENOENT" }); + for (const error of [first, second]) + expect(error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(browserCloses).toBe(2); + } finally { + browserDenied = false; + profileDenied = false; + await owner.close(); + } +}); + +it("bounds an opening failure response while retaining the same late cleanup lease", async () => { + vi.useFakeTimers(); + let acknowledge: () => void = () => {}; + const late = new Promise((resolve) => { + acknowledge = resolve; + }); + const lease = new AnalysisCleanupLease(() => late, ["browser_profile"]); + const reason = { caller: "cancelled opening" }; + const primary = new AnalysisCancelledError("capture_browser_scenario", { + cause: reason, + }); + try { + const opening = failBrowserScenarioOperation(lease, primary).catch( + (cause: unknown) => cause, + ); + await vi.advanceTimersByTimeAsync(30_000); + const failure = await opening; + expect(failure).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_profile"], + }); + expect(lease.released).toBe(false); + acknowledge(); + await lease.release(); + expect(failure).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + if (!(failure instanceof AnalysisError)) + throw new Error("Expected typed cleanup failure"); + expect(projectAnalysisError(failure)).toMatchObject({ code: "cancelled" }); + expect(primary.cause).toBe(reason); + } finally { + acknowledge(); + await lease.release(); + vi.useRealTimers(); + } +}); diff --git a/src/browser/PlaywrightScenarioBrowser.ts b/src/browser/PlaywrightScenarioBrowser.ts index fe151d7a96..c261553070 100644 --- a/src/browser/PlaywrightScenarioBrowser.ts +++ b/src/browser/PlaywrightScenarioBrowser.ts @@ -1,3 +1,10 @@ +import { PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY } from "./providerIdentities.js"; +import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; +import { ProviderAdapterError } from "../domain/providerAdapterError.js"; +import { + AnalysisCleanupLeaseGroup, + AnalysisCleanupLease, +} from "../artifacts/AnalysisCleanupLease.js"; import { chmod, mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; @@ -14,16 +21,18 @@ import type { BrowserScenario } from "../domain/browserScenario.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; import { isLiteralLoopbackHostname } from "../domain/browserObservation.js"; import { BrowserObservationError } from "../domain/browserObservationError.js"; -import { withPlaywrightExecutionBoundary } from "./PlaywrightExecutionBoundary.js"; +import { + withPlaywrightExecutionBoundary, + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, +} from "./PlaywrightExecutionBoundary.js"; const OPERATION = "capture_browser_scenario" as const; -// Closing a real Chrome persistent context can take several seconds even when -// it succeeds. The shorter operation deadline must not misreport cleanup. -const CLEANUP_CONFIRMATION_TIMEOUT_MS = 30_000; const throwIfScenarioCancelled = (signal: AbortSignal | undefined): void => { if (signal?.aborted === true) - throw new BrowserObservationError(OPERATION, "cancelled"); + throw new BrowserObservationError(OPERATION, "cancelled", { + cause: signal?.reason, + }); }; export interface OpenedScenarioBrowser { @@ -45,63 +54,75 @@ interface PlaywrightScenarioCleanupResources { export class PlaywrightScenarioBrowserCleanupOwner { #browserClosed = false; #profileRemoved: boolean; - #closePromise: Promise | undefined; #eventFinalizationPromise: Promise | undefined; + #eventsFinalized = false; + readonly cleanupLease: AnalysisCleanupLease; /** Keep the narrow provider actions needed to release one opened browser. */ constructor(private readonly resources: PlaywrightScenarioCleanupResources) { this.#profileRemoved = resources.removeProfile === undefined; + this.cleanupLease = new AnalysisCleanupLease( + () => + this.#closeResources().finally(() => + this.resources.onSettled?.(this.#browserClosed), + ), + this, + ); } get browserClosed(): boolean { return this.#browserClosed; } + get cleanupResources(): readonly string[] { + return [ + ...(!this.#eventsFinalized ? ["browser_events"] : []), + ...(!this.#browserClosed ? ["browser_transport"] : []), + ...(!this.#profileRemoved ? ["browser_profile"] : []), + ]; + } + + /** Await actual resource release, including a late browser acknowledgement. */ + release(finishEvents?: () => Promise): Promise { + this.#prepareEvents(finishEvents); + return this.cleanupLease.release(); + } + close(finishEvents?: () => Promise): Promise { - const cleanup = this.#closePromise ?? this.#closeResources(finishEvents); - if (this.#closePromise === undefined) { - this.#closePromise = cleanup; - void cleanup.then( - () => this.resources.onSettled?.(this.#browserClosed), - () => { - if (this.#closePromise === cleanup) this.#closePromise = undefined; - this.resources.onSettled?.(this.#browserClosed); - }, - ); - } - return this.#awaitCleanup(cleanup); + return this.#awaitCleanup(this.release(finishEvents)); + } + + #prepareEvents(finishEvents?: () => Promise): void { + this.#eventFinalizationPromise ??= Promise.resolve().then(async () => { + await finishEvents?.(); + this.#eventsFinalized = true; + }); } async #awaitCleanup(cleanup: Promise): Promise { try { await withPlaywrightExecutionBoundary( () => cleanup, - CLEANUP_CONFIRMATION_TIMEOUT_MS, + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, ); } catch (cause: unknown) { - throw new BrowserObservationError(OPERATION, "cleanup_failed", { + const failure = new BrowserObservationError(OPERATION, "cleanup_failed", { cause, cleanup: { reason: cause instanceof Error ? cause.message : String(cause), - resources: [ - "browser_transport", - ...(this.resources.removeProfile === undefined - ? [] - : ["browser_profile"]), - ], + resources: this.cleanupResources, }, }); + new AnalysisCleanupLeaseGroup(failure, [this.cleanupLease]); + throw failure; } } - async #closeResources(finishEvents?: () => Promise): Promise { + async #closeResources(): Promise { const failures: unknown[] = []; - const eventFinalization = (this.#eventFinalizationPromise ??= - Promise.resolve().then(async () => { - await finishEvents?.(); - })); + this.#prepareEvents(); try { - await eventFinalization; + await this.#eventFinalizationPromise; } catch (cause: unknown) { // A failed finalization stays failed: the memoized attempt rejects on // every retry so cleanup can never report success for unfinalized @@ -172,7 +193,11 @@ const releaseAttachedScenario = ( if (waiter.onAbort !== undefined) waiter.signal?.removeEventListener("abort", waiter.onAbort); if (waiter.signal?.aborted === true) { - waiter.reject(new BrowserObservationError(OPERATION, "cancelled")); + waiter.reject( + new BrowserObservationError(OPERATION, "cancelled", { + cause: waiter.signal.reason, + }), + ); releaseAttachedScenario(key, admission); return; } @@ -209,7 +234,11 @@ const acquireAttachedScenario = ( signal: AbortSignal | undefined, ): Promise => { if (signal?.aborted === true) - return Promise.reject(new BrowserObservationError(OPERATION, "cancelled")); + return Promise.reject( + new BrowserObservationError(OPERATION, "cancelled", { + cause: signal?.reason, + }), + ); const key = attachedScenarioLeaseKey( scenario.cdp_endpoint, @@ -246,7 +275,11 @@ const acquireAttachedScenario = ( if (index < 0) return; admission.waiters.splice(index, 1); signal.removeEventListener("abort", onAbort); - reject(new BrowserObservationError(OPERATION, "cancelled")); + reject( + new BrowserObservationError(OPERATION, "cancelled", { + cause: signal?.reason, + }), + ); if (!admission.active && admission.waiters.length === 0) releaseAttachedScenario(key, admission); }; @@ -465,35 +498,38 @@ class AttachedScenarioEnvironmentOwner { /** Preserve an operation failure while reporting any incomplete cleanup. */ export const failBrowserScenarioOperation = async ( - cleanup: () => Promise, + cleanup: AnalysisCleanupLease, primaryFailure: unknown, - fallbackCleanupResources: readonly string[] = ["browser_transport"], ): Promise => { try { - await cleanup(); - } catch (cleanupFailure: unknown) { - const cleanupDetails = - cleanupFailure instanceof AnalysisError && - cleanupFailure.cleanup !== undefined - ? cleanupFailure.cleanup - : { - reason: - cleanupFailure instanceof Error - ? cleanupFailure.message - : String(cleanupFailure), - resources: - cleanupFailure instanceof AnalysisError - ? cleanupFailure.cleanupResources - : fallbackCleanupResources, - }; - throw new BrowserObservationError(OPERATION, "cleanup_failed", { - cause: new AggregateError( - [primaryFailure, cleanupFailure], - "Browser scenario operation and cleanup both failed", - { cause: primaryFailure }, - ), - cleanup: cleanupDetails, + await withPlaywrightExecutionBoundary( + () => cleanup.release(), + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, + ); + } catch (cause: unknown) { + const primary = + primaryFailure instanceof AnalysisError + ? primaryFailure + : new ProviderAdapterError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + OPERATION, + { + cause: primaryFailure, + }, + ); + const failure = new BrowserObservationError(OPERATION, "cleanup_failed", { + cause, + cleanup: { + reason: cause instanceof Error ? cause.message : String(cause), + resources: cleanup.cleanupResources ?? [], + }, }); + new AnalysisCleanupLeaseGroup(failure, [cleanup]); + throw analysisErrorWithCleanupFailure( + primary, + failure, + OPERATION, + ).bindCleanupDependencies([primary, failure]); } throw primaryFailure; }; @@ -517,7 +553,7 @@ const openAttachedScenarioBrowser = async ( environment: BrowserScenario["environment"], launcher: ScenarioBrowserLauncher, signal: AbortSignal | undefined, - retainCleanup: ((close: () => Promise) => void) | undefined, + retainCleanup: ((lease: AnalysisCleanupLease) => void) | undefined, ): Promise => { const lease = await acquireAttachedScenario(browserRequest, signal); let cleanupOwner: PlaywrightScenarioBrowserCleanupOwner | undefined; @@ -607,11 +643,11 @@ const openAttachedScenarioBrowser = async ( throw cause; } const cleanup = cleanupOwner; - const close = () => cleanup.close(); + const recovery = cleanup.cleanupLease; try { - return await failBrowserScenarioOperation(close, cause); + return await failBrowserScenarioOperation(recovery, cause); } catch (failure: unknown) { - if (failure !== cause) retainCleanup?.(close); + if (failure !== cause) retainCleanup?.(recovery); throw failure; } } @@ -624,7 +660,7 @@ export const openPlaywrightScenarioBrowser = async ( options: { readonly launcher?: ScenarioBrowserLauncher; readonly signal?: AbortSignal | undefined; - readonly retainCleanup?: (close: () => Promise) => void; + readonly retainCleanup?: (lease: AnalysisCleanupLease) => void; } = {}, ): Promise => { // Load Playwright only when a scenario opens; MCP startup stays off its module graph. @@ -671,12 +707,12 @@ export const openPlaywrightScenarioBrowser = async ( : [], }); } catch (cause: unknown) { - const cleanup = () => - rm(profilePath, { recursive: true, force: true, maxRetries: 3 }); + const cleanup = new AnalysisCleanupLease( + () => rm(profilePath, { recursive: true, force: true, maxRetries: 3 }), + ["browser_profile"], + ); try { - return await failBrowserScenarioOperation(cleanup, cause, [ - "browser_profile", - ]); + return await failBrowserScenarioOperation(cleanup, cause); } catch (failure: unknown) { if (failure !== cause) options.retainCleanup?.(cleanup); throw failure; @@ -700,11 +736,11 @@ export const openPlaywrightScenarioBrowser = async ( cleanup, }; } catch (cause: unknown) { - const close = () => cleanup.close(); + const recovery = cleanup.cleanupLease; try { - return await failBrowserScenarioOperation(close, cause); + return await failBrowserScenarioOperation(recovery, cause); } catch (failure: unknown) { - if (failure !== cause) options.retainCleanup?.(close); + if (failure !== cause) options.retainCleanup?.(recovery); throw failure; } } diff --git a/src/browser/PlaywrightScenarioSession.test.ts b/src/browser/PlaywrightScenarioSession.test.ts index ae73b95b91..b482d681c4 100644 --- a/src/browser/PlaywrightScenarioSession.test.ts +++ b/src/browser/PlaywrightScenarioSession.test.ts @@ -1,3 +1,7 @@ +import { AnalysisCleanupLeaseGroup } from "../artifacts/AnalysisCleanupLease.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { PlaywrightBrowserScenarioProvider } from "./PlaywrightBrowserScenarioProvider.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import { EventEmitter } from "node:events"; import { beforeEach, expect, it, vi } from "vitest"; @@ -40,11 +44,11 @@ vi.mock("./CdpTargetSession.js", () => ({ vi.mock("./PlaywrightScenarioBrowser.js", () => ({ openPlaywrightScenarioBrowser: mocks.openBrowser, failBrowserScenarioOperation: async ( - cleanup: () => Promise, + cleanup: { release(): Promise }, cause: unknown, ) => { try { - await cleanup(); + await cleanup.release(); } catch (cleanupCause: unknown) { throw new AggregateError([cause, cleanupCause]); } @@ -139,6 +143,20 @@ const fixture = () => { }); const cleanup = { close, + release: close, + cleanupLease: new AnalysisCleanupLease( + async () => { + await close(); + }, + { + get cleanupResources() { + return browserClosed ? [] : ["browser_transport"]; + }, + }, + ), + get cleanupResources() { + return browserClosed ? [] : ["browser_transport"]; + }, get browserClosed() { return browserClosed; }, @@ -194,7 +212,10 @@ it("stops attached navigation without storage and retries rejected policy cleanu const session = await PlaywrightScenarioSession.open(attached, {}, {}); const failure = new Error("stopLoading rejected"); mocks.policySend.mockRejectedValueOnce(failure); - await expect(session.close(true)).rejects.toBe(failure); + await expect(session.close(true)).rejects.toMatchObject({ + cause: failure, + cleanupIncomplete: true, + }); expect(cleanup.close).not.toHaveBeenCalled(); await expect(session.close(true)).resolves.toBe("disconnected-external"); expect( @@ -246,7 +267,8 @@ it("transfers attached navigation stopping after storage cleanup fails", async ( mocks.policySend.mockRejectedValueOnce(policyFailure); await expect(session.close(true)).rejects.toMatchObject({ - errors: [policyFailure, storageFailure], + cause: { errors: [policyFailure, storageFailure] }, + cleanupIncomplete: true, }); expect(cleanup.close).not.toHaveBeenCalled(); await expect(session.close()).resolves.toBe("disconnected-external"); @@ -289,7 +311,10 @@ it("transfers a late stop request after storage has already released", async () return policyResponse(method); }); - await expect(session.close(false)).rejects.toBe(restorationFailure); + await expect(session.close(false)).rejects.toMatchObject({ + cause: restorationFailure, + cleanupIncomplete: true, + }); expect(cleanup.close).not.toHaveBeenCalled(); cancellation.abort(); await expect(session.close()).resolves.toBe("disconnected-external"); @@ -320,7 +345,7 @@ it("retains transferred navigation stopping after setup cleanup fails", async () } return policyResponse(method); }); - let retainedCleanup: (() => Promise) | undefined; + let retainedCleanup: AnalysisCleanupLease | undefined; const attached = browserScenarioSchema.parse({ ...scenario, browser: { @@ -346,7 +371,7 @@ it("retains transferred navigation stopping after setup cleanup fails", async () expect(cleanup.close).not.toHaveBeenCalled(); if (retainedCleanup === undefined) throw new Error("failed cleanup was not retained"); - await expect(retainedCleanup()).resolves.toBeUndefined(); + await expect(retainedCleanup.release()).resolves.toBeUndefined(); expect( mocks.policySend.mock.calls.filter( ([method]) => method === "Page.stopLoading", @@ -571,7 +596,10 @@ it("retains an owned script identifier after rejected removal", async () => { const session = await PlaywrightScenarioSession.open(attached, {}, {}); const failure = new Error("removeScript rejected"); mocks.policySend.mockRejectedValueOnce(failure); - await expect(session.close()).rejects.toBe(failure); + await expect(session.close()).rejects.toMatchObject({ + cause: failure, + cleanupIncomplete: true, + }); expect(cleanup.close).not.toHaveBeenCalled(); await expect(session.close()).resolves.toBe("disconnected-external"); const removals = mocks.policySend.mock.calls.filter( @@ -597,7 +625,10 @@ it("retries transport close without detaching an already released policy session const session = await PlaywrightScenarioSession.open(attached, {}, {}); const failure = new Error("transport close rejected"); mocks.policyTransportClose.mockRejectedValueOnce(failure); - await expect(session.close()).rejects.toBe(failure); + await expect(session.close()).rejects.toMatchObject({ + cause: failure, + cleanupIncomplete: true, + }); await expect(session.close()).resolves.toBe("disconnected-external"); expect( mocks.policySend.mock.calls.filter( @@ -668,9 +699,14 @@ it("retains a detached policy connection until the main realm is restored", asyn .mockResolvedValueOnce("policy_owned") .mockResolvedValueOnce(null); - await expect(session.close(true)).rejects.toThrow( - "service-worker policy restoration could not be confirmed", - ); + await expect(session.close(true)).rejects.toMatchObject({ + cause: { + message: expect.stringContaining( + "service-worker policy restoration could not be confirmed", + ), + }, + cleanupIncomplete: true, + }); expect(cleanup.close).not.toHaveBeenCalled(); expect(mocks.policyTransportClose).not.toHaveBeenCalled(); await expect(session.close()).resolves.toBe("disconnected-external"); @@ -776,7 +812,10 @@ it("retains the attached connection when registration restoration fails", async } return policyResponse(method); }); - await expect(session.close(true)).rejects.toBe(failure); + await expect(session.close(true)).rejects.toMatchObject({ + cause: failure, + cleanupIncomplete: true, + }); expect(cleanup.close).not.toHaveBeenCalled(); await expect(session.close()).resolves.toBe("disconnected-external"); expect( @@ -928,8 +967,10 @@ it("preserves both cleanup failures and retries retained owners", async () => { } catch (cause: unknown) { failure = cause; } - expect(failure).toBeInstanceOf(AggregateError); - expect((failure as AggregateError).errors).toEqual([ + expect(failure).toMatchObject({ cleanupIncomplete: true }); + const physical = (failure as Error).cause; + expect(physical).toBeInstanceOf(AggregateError); + expect((physical as AggregateError).errors).toEqual([ detachFailure, transportFailure, ]); @@ -937,6 +978,116 @@ it("preserves both cleanup failures and retries retained owners", async () => { expect(cleanup.close).toHaveBeenCalledTimes(1); await expect(session.close()).resolves.toBe("terminated-owned-process"); + expect(failure).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); expect(storage.close).toHaveBeenCalledTimes(2); expect(cleanup.close).toHaveBeenCalledTimes(2); }); + +it("keeps the session's exact cleanup lease pending after response timeout until browser acknowledgement", async () => { + vi.useFakeTimers(); + const { cleanup } = fixture(); + const session = await PlaywrightScenarioSession.open(scenario, {}, {}); + let acknowledge: () => void = () => {}; + const late = new Promise((resolve) => { + acknowledge = resolve; + }); + cleanup.release = vi.fn(async (finishEvents?: () => Promise) => { + await late; + await cleanup.close(finishEvents); + }); + try { + const pending = session.close().catch((cause: unknown) => cause); + await vi.advanceTimersByTimeAsync(30_000); + const failure = await pending; + expect(failure).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_transport"], + }); + expect(session.cleanupLease.released).toBe(false); + acknowledge(); + await session.cleanupLease.release(); + expect(failure).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(cleanup.close).toHaveBeenCalledTimes(1); + } finally { + acknowledge(); + await session.cleanupLease.release(); + vi.useRealTimers(); + } +}); + +it("retains late opening failure cleanup after cancellation until the exact profile lease recovers", async () => { + let finishOpening: () => void = () => {}; + const terminal = new Promise((resolve) => { + finishOpening = resolve; + }); + let profileStarted: () => void = () => {}; + const profileAttempt = new Promise((resolve) => { + profileStarted = resolve; + }); + let denied = true; + const lease = new AnalysisCleanupLease(async () => { + profileStarted(); + if (denied) throw new Error("authored profile removal denial"); + }, ["browser_profile"]); + const failure = new ProviderCleanupError("fixture", ["browser_profile"], { + reason: "late opening profile cleanup", + }); + new AnalysisCleanupLeaseGroup(failure, [lease]); + let launchStarted: () => void = () => {}; + const started = new Promise((resolve) => { + launchStarted = resolve; + }); + mocks.openBrowser.mockImplementation( + async ( + _scenario: unknown, + _env: unknown, + options: { retainCleanup?: (lease: AnalysisCleanupLease) => void }, + ) => { + launchStarted(); + await terminal; + options.retainCleanup?.(lease); + throw failure; + }, + ); + const provider = new PlaywrightBrowserScenarioProvider((input, options) => + PlaywrightScenarioSession.open(input, {}, options), + ); + const caller = new AbortController(); + const reason = { caller: "before late launch rejection" }; + try { + const pending = provider.captureScenario(scenario, { + signal: caller.signal, + }); + await started; + caller.abort(reason); + const result = await pending; + if (result.ok) throw new Error("Expected cancelled opening"); + expect(result.error.cleanupIncomplete).toBe(true); + finishOpening(); + await profileAttempt; + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + expect(result.error).toMatchObject({ + cleanupIncomplete: true, + cleanupResources: ["browser_profile"], + }); + denied = false; + await provider.close(); + expect(result.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); + expect(failure.cleanupIncomplete).toBe(false); + } finally { + denied = false; + finishOpening(); + await provider.close(); + } +}); diff --git a/src/browser/PlaywrightScenarioSession.ts b/src/browser/PlaywrightScenarioSession.ts index 63557da7be..dca2896280 100644 --- a/src/browser/PlaywrightScenarioSession.ts +++ b/src/browser/PlaywrightScenarioSession.ts @@ -1,7 +1,12 @@ +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../artifacts/AnalysisCleanupLease.js"; import type { BrowserContext } from "playwright-core"; import type { BrowserScenario } from "../domain/browserScenario.js"; import type { BrowserScenarioAction } from "../domain/browserScenarioValues.js"; +import { AnalysisError } from "../domain/analysisErrorBase.js"; import { BrowserObservationError } from "../domain/browserObservationError.js"; import type { BrowserScenarioSessionPort } from "./BrowserScenarioSessionPort.js"; import { BrowserScenarioSecrets } from "./BrowserScenarioSecrets.js"; @@ -15,7 +20,10 @@ import { capturePlaywrightStepArtifacts } from "./PlaywrightScenarioArtifacts.js import { PlaywrightScenarioEvents } from "./PlaywrightScenarioEvents.js"; import { PlaywrightScenarioStorage } from "./PlaywrightScenarioStorage.js"; import { PlaywrightScenarioServiceWorkers } from "./PlaywrightScenarioServiceWorkers.js"; -import { withPlaywrightExecutionBoundary } from "./PlaywrightExecutionBoundary.js"; +import { + withPlaywrightExecutionBoundary, + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, +} from "./PlaywrightExecutionBoundary.js"; import { browserScenarioCaptureData, browserScenarioOperationFailure, @@ -25,7 +33,9 @@ const OPERATION = "capture_browser_scenario" as const; const throwIfScenarioCancelled = (signal: AbortSignal | undefined): void => { if (signal?.aborted === true) - throw new BrowserObservationError(OPERATION, "cancelled"); + throw new BrowserObservationError(OPERATION, "cancelled", { + cause: signal?.reason, + }); }; const throwCleanupFailures = (failures: readonly unknown[]): void => { @@ -73,6 +83,8 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { private readonly serviceWorkers: PlaywrightScenarioServiceWorkers | undefined; private readonly signal: AbortSignal | undefined; #storageReleased = false; + #stopLoading = false; + readonly cleanupLease: AnalysisCleanupLease; #policyStopsLoading = false; private constructor( @@ -96,6 +108,16 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { this.signal = options.signal; this.version = opened.browser.version(); this.initialUrl = opened.page.url(); + this.cleanupLease = new AnalysisCleanupLease( + async () => { + await this.#closeResources(); + }, + { + get cleanupResources() { + return opened.cleanup.cleanupResources; + }, + }, + ); } static async open( @@ -103,7 +125,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { environment: Readonly>, options: { readonly signal?: AbortSignal; - readonly retainCleanup?: (close: () => Promise) => void; + readonly retainCleanup?: (lease: AnalysisCleanupLease) => void; }, ): Promise { throwIfScenarioCancelled(options.signal); @@ -111,11 +133,13 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { if (secrets === undefined) throw new BrowserObservationError(OPERATION, "secret_unavailable"); const startedAt = performance.now(); + const openingCleanup = new Set(); const opening = openPlaywrightScenarioBrowser(scenario, environment, { signal: options.signal, - ...(options.retainCleanup === undefined - ? {} - : { retainCleanup: options.retainCleanup }), + retainCleanup: (lease) => { + openingCleanup.add(lease); + options.retainCleanup?.(lease); + }, }); let opened: OpenedScenarioBrowser; try { @@ -125,18 +149,48 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { options.signal, ); } catch (cause: unknown) { - const lateCleanup = () => - opening.then( - (lateOpened) => lateOpened.cleanup.close(), - () => undefined, - ); - options.retainCleanup?.(() => - withPlaywrightExecutionBoundary(lateCleanup, 1_000), + let lateOpened: OpenedScenarioBrowser | undefined; + let openingFailure: AnalysisError | undefined; + const cleanup = new AnalysisCleanupLease( + async () => { + try { + lateOpened = await opening; + } catch (cause: unknown) { + openingFailure = + cause instanceof AnalysisError && + !( + options.signal?.aborted === true && + cause === options.signal.reason + ) + ? cause + : undefined; + const results = await Promise.allSettled( + [...openingCleanup].map((lease) => lease.release()), + ); + const failures = results.flatMap((result) => + result.status === "rejected" ? [result.reason as unknown] : [], + ); + throwCleanupFailures(failures); + if (openingFailure?.cleanupIncomplete === true) + throw openingFailure; + return; + } + await lateOpened.cleanup.release(); + }, + { + get cleanupResources() { + return ( + lateOpened?.cleanup.cleanupResources ?? + openingFailure?.cleanupResources + ); + }, + }, ); - void lateCleanup().catch((cleanupCause: unknown) => { - // best-effort cleanup: retainCleanup keeps the retry capability. - void cleanupCause; - }); + options.retainCleanup?.(cleanup); + void withPlaywrightExecutionBoundary( + () => cleanup.release(), + 1_000, + ).catch(() => undefined); throw cause; } const session = await this.initializeSession( @@ -177,7 +231,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } catch (failure: unknown) { cleanup = "incomplete"; cleanupFailure = { cause: failure }; - options.retainCleanup?.(() => session.close()); + options.retainCleanup?.(session.cleanupLease); } throw browserScenarioOperationFailure( cause, @@ -205,7 +259,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { secrets: BrowserScenarioSecrets, options: { readonly signal?: AbortSignal; - readonly retainCleanup?: (close: () => Promise) => void; + readonly retainCleanup?: (lease: AnalysisCleanupLease) => void; }, ): Promise { let events: PlaywrightScenarioEvents | undefined; @@ -289,7 +343,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } if (policyFailure === undefined) { try { - await opened.cleanup.close( + await opened.cleanup.release( events === undefined ? undefined : () => events?.finish() ?? Promise.resolve(), @@ -313,10 +367,11 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { ...(browserFailure === undefined ? [] : [browserFailure.cause]), ]); }; + const recovery = new AnalysisCleanupLease(cleanup, opened.cleanup); try { - return await failBrowserScenarioOperation(cleanup, cause); + return await failBrowserScenarioOperation(recovery, cause); } catch (failure: unknown) { - if (failure !== cause) options.retainCleanup?.(cleanup); + if (failure !== cause) options.retainCleanup?.(recovery); throw failure; } } @@ -402,6 +457,30 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } async close(stopLoading = this.signal?.aborted === true) { + this.#stopLoading ||= stopLoading; + try { + await withPlaywrightExecutionBoundary( + () => this.cleanupLease.release(), + PLAYWRIGHT_CLEANUP_CONFIRMATION_TIMEOUT_MS, + ); + } catch (cause: unknown) { + const error = new BrowserObservationError(OPERATION, "cleanup_failed", { + cause, + cleanup: { + reason: cause instanceof Error ? cause.message : String(cause), + resources: this.cleanupLease.cleanupResources ?? [], + }, + }); + new AnalysisCleanupLeaseGroup(error, [this.cleanupLease]); + throw error; + } + return this.mode === "launch" + ? ("terminated-owned-process" as const) + : ("disconnected-external" as const); + } + + async #closeResources(): Promise { + const stopLoading = this.#stopLoading || this.signal?.aborted === true; let storageFailure: { readonly cause: unknown } | undefined; let policyFailure: { readonly cause: unknown } | undefined; let browserFailure: { readonly cause: unknown } | undefined; @@ -425,7 +504,7 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { } if (policyFailure === undefined) { try { - await this.opened.cleanup.close(() => this.eventCapture.finish()); + await this.opened.cleanup.release(() => this.eventCapture.finish()); } catch (failure: unknown) { browserFailure = { cause: failure }; } @@ -444,9 +523,6 @@ export class PlaywrightScenarioSession implements BrowserScenarioSessionPort { : [storageFailure.cause]), ...(browserFailure === undefined ? [] : [browserFailure.cause]), ]); - return this.mode === "launch" - ? ("terminated-owned-process" as const) - : ("disconnected-external" as const); } redactError(error: unknown): string { diff --git a/src/browser/PlaywrightScenarioStorage.ts b/src/browser/PlaywrightScenarioStorage.ts index b0190c5f43..677b4c9609 100644 --- a/src/browser/PlaywrightScenarioStorage.ts +++ b/src/browser/PlaywrightScenarioStorage.ts @@ -1,3 +1,4 @@ +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import { randomUUID } from "node:crypto"; import { z } from "zod"; @@ -103,7 +104,7 @@ type PendingReply = { }; type StorageInstallationOptions = { readonly signal?: AbortSignal; - readonly retainCleanup?: (close: () => Promise) => void; + readonly retainCleanup?: (lease: AnalysisCleanupLease) => void; }; type StoragePage = { readonly context: BrowserContext; readonly page: Page }; export type PlaywrightScenarioStorageCleanup = { @@ -176,9 +177,12 @@ export class PlaywrightScenarioStorage { detached = true; } }; - retainCleanup?.(lateCleanup); + const cleanup = new AnalysisCleanupLease(lateCleanup, [ + "browser_storage_transport", + ]); + retainCleanup?.(cleanup); void withPlaywrightExecutionBoundary( - lateCleanup, + () => cleanup.release(), STORAGE_CLEANUP_TIMEOUT_MS, ).catch(() => undefined); } diff --git a/tests/fixtures/browserScenarioCancellation.mjs b/tests/fixtures/browserScenarioCancellation.mjs index ecb85e608c..d6eb22795f 100644 --- a/tests/fixtures/browserScenarioCancellation.mjs +++ b/tests/fixtures/browserScenarioCancellation.mjs @@ -34,6 +34,9 @@ class ControlledSession { process.stderr.write("REA_CONTROLLED_PROFILE_CALLBACK\n"); }, }); + get cleanupLease() { + return this.cleanup.cleanupLease; + } currentUrl() { return this.initialUrl; } @@ -66,7 +69,11 @@ class ControlledSession { await new Promise((resolveWait, reject) => { timer = setTimeout(resolveWait, 60000); abort = () => - reject(new AnalysisCancelledError("capture_browser_scenario")); + reject( + new AnalysisCancelledError("capture_browser_scenario", { + cause: signal?.reason, + }), + ); signal?.addEventListener("abort", abort, { once: true }); process.stderr.write( "REA_CONTROLLED_BROWSER_READY signal_present=" + diff --git a/tests/process-global/browserScenarioCleanup.test.ts b/tests/process-global/browserScenarioCleanup.test.ts index 638d188ad0..5c1c0f2e20 100644 --- a/tests/process-global/browserScenarioCleanup.test.ts +++ b/tests/process-global/browserScenarioCleanup.test.ts @@ -75,6 +75,9 @@ class RetrySession implements BrowserScenarioSessionPort { }, }); + get cleanupLease() { + return this.cleanup.cleanupLease; + } constructor(private readonly closeFailures = 1) {} currentUrl() { @@ -317,7 +320,12 @@ it("retries a late Playwright open after cancellation before accepting another c cancellation.abort(); const cancelled = await firstCapture; if (cancelled.ok) throw new Error("Expected the first capture to cancel."); - expect(cancelled.error).toBeInstanceOf(AnalysisCancelledError); + expect(cancelled.error.cleanupIncomplete).toBe(true); + let primary = cancelled.error.cleanupPrimary; + while (primary?.cleanupPrimary !== undefined) + primary = primary.cleanupPrimary; + expect(primary).toBeInstanceOf(AnalysisCancelledError); + expect(primary?.cause).toBe(cancellation.signal.reason); finishLateOpen(makeContext()); await closeStarted; @@ -331,6 +339,10 @@ it("retries a late Playwright open after cancellation before accepting another c expect(retried.ok).toBe(true); if (retried.ok) expect(retried.value.steps).toHaveLength(2); expect(firstCloseAttempts).toBe(2); + expect(cancelled.error).toMatchObject({ + cleanupIncomplete: false, + cleanupResources: [], + }); expect(launchCount).toBe(2); await provider.close(); expect(launchCount).toBe(2); @@ -431,7 +443,14 @@ it("preserves cancellation diagnostics while retrying retained cleanup", async ( details: { cleanup: "incomplete", diagnostics: { - primary_error: { code: "cancelled", category: "cancelled" }, + primary_error: { + code: "cleanup_incomplete", + details: { + diagnostics: { + primary_error: { code: "cancelled", category: "cancelled" }, + }, + }, + }, }, partial_observation: { kind: "browser-scenario-observation" }, },