From 2e67d57d0fbdd8b0131623fbb23107df584ed2c1 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:15:15 +0800 Subject: [PATCH] fix(browser): distinguish shutdown admission from failed cleanup (#10038) --- src/browser/CdpTargetSessionOwner.ts | 22 ++++---- .../PlaywrightBrowserScenarioProvider.test.ts | 54 +++++++++++++++++++ .../PlaywrightBrowserScenarioProvider.ts | 14 +++-- .../browser/cdpTargetSessionOwner.test.ts | 35 +++++++++++- 4 files changed, 109 insertions(+), 16 deletions(-) diff --git a/src/browser/CdpTargetSessionOwner.ts b/src/browser/CdpTargetSessionOwner.ts index 50bb08653f..190560b0aa 100644 --- a/src/browser/CdpTargetSessionOwner.ts +++ b/src/browser/CdpTargetSessionOwner.ts @@ -8,7 +8,10 @@ import { AnalysisError } from "../domain/analysisErrorBase.js"; import { BrowserObservationError } from "../domain/browserObservationError.js"; import type { BrowserObservationOperation } from "../domain/browserObservationErrors.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; -import { AnalysisCancelledError } from "../domain/analysisErrorCore.js"; +import { + AnalysisCancelledError, + AnalysisCapabilityUnavailableError, +} from "../domain/analysisErrorCore.js"; import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; import { CdpConnection } from "./CdpConnection.js"; import type { ExecutionOptions } from "../application/AnalysisProvider.js"; @@ -119,7 +122,7 @@ export class CdpTargetSessionOwner { options: ExecutionOptions, limits: BrowserSocketLimits, ): Promise<{ readonly connection: CdpConnection; readonly reused: boolean }> { - this.#assertOpen(operation, "browser-endpoint"); + this.#assertOpen(operation); await this.retryPending(); const leased = await this.#leaseExistingBrowserSocket( url, @@ -140,7 +143,7 @@ export class CdpTargetSessionOwner { { readonly connection: CdpConnection; readonly reused: boolean } | undefined > { while (true) { - this.#assertOpen(operation, "browser-endpoint"); + this.#assertOpen(operation); if (options.signal?.aborted === true) throw new AnalysisCancelledError(operation, { cause: options.signal.reason, @@ -427,9 +430,9 @@ export class CdpTargetSessionOwner { readonly handshakeTimeoutMs?: number; }, ): Promise { - this.#assertOpen(operation, resource); + this.#assertOpen(operation); await this.retryPending(); - this.#assertOpen(operation, resource); + this.#assertOpen(operation); if (options.signal?.aborted === true) throw new AnalysisCancelledError(operation, { cause: options.signal.reason, @@ -576,13 +579,12 @@ export class CdpTargetSessionOwner { if (failures.length > 1) throw aggregateCleanupFailures(failures); } - #assertOpen(operation: BrowserObservationOperation, resource: string): void { + #assertOpen(operation: BrowserObservationOperation): void { if (!this.#closing) return; - throw new ProviderCleanupError( + throw new AnalysisCapabilityUnavailableError( "rea-cdp-browser", - [resource], - { reason: "CDP provider is closing and cannot admit another session." }, - { operation }, + operation, + "CDP provider is closing and cannot admit another session.", ); } diff --git a/src/browser/PlaywrightBrowserScenarioProvider.test.ts b/src/browser/PlaywrightBrowserScenarioProvider.test.ts index 8b4e72b67f..7cb0b4506f 100644 --- a/src/browser/PlaywrightBrowserScenarioProvider.test.ts +++ b/src/browser/PlaywrightBrowserScenarioProvider.test.ts @@ -1429,3 +1429,57 @@ it.each([false, true])( expect(screenshot).not.toHaveBeenCalled(); }, ); + +it.each(["clean", "pending", "queued"] as const)( + "refuses scenario admission during %s shutdown without claiming cleanup failed", + async (stage) => { + let opened = 0; + let finish: () => void = () => {}; + let started: () => void = () => {}; + const release = new Promise((resolve) => { + finish = resolve; + }); + const closingStarted = new Promise((resolve) => { + started = resolve; + }); + const session = new FakeSession("launch"); + session.close = async () => { + started(); + await release; + return "terminated-owned-process"; + }; + const provider = new PlaywrightBrowserScenarioProvider(async () => { + opened += 1; + return session; + }); + let running: ReturnType | undefined; + let closing: Promise | undefined; + try { + if (stage !== "clean") { + running = provider.captureScenario(scenario()); + await closingStarted; + } + const queued = + stage === "queued" ? provider.captureScenario(scenario()) : undefined; + closing = provider.close(); + if (stage === "clean") await closing; + const refusal = queued ?? provider.captureScenario(scenario()); + if (stage === "queued") finish(); + const result = await refusal; + expect(result.ok).toBe(false); + if (result.ok) + throw new Error("Expected closed provider admission refusal"); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "capability_unavailable", + details: { operation: "capture_browser_scenario" }, + }); + expect(opened).toBe(stage === "clean" ? 0 : 1); + } finally { + finish(); + await running; + await closing; + await provider.close(); + } + }, +); diff --git a/src/browser/PlaywrightBrowserScenarioProvider.ts b/src/browser/PlaywrightBrowserScenarioProvider.ts index 9180132df4..b5d77c5106 100644 --- a/src/browser/PlaywrightBrowserScenarioProvider.ts +++ b/src/browser/PlaywrightBrowserScenarioProvider.ts @@ -34,7 +34,10 @@ import { type BrowserStepArtifacts, } from "../domain/browserScenarioCaptureValues.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; -import { AnalysisCancelledError } from "../domain/analysisErrorCore.js"; +import { + AnalysisCancelledError, + AnalysisCapabilityUnavailableError, +} from "../domain/analysisErrorCore.js"; import { BrowserObservationError, browserCleanupObservation, @@ -496,10 +499,11 @@ export class PlaywrightBrowserScenarioProvider implements BrowserScenarioCapture } #closedError(): AnalysisError { - return new BrowserObservationError(OPERATION, "cleanup_failed", { - detail: "Browser scenario provider is closing.", - cleanup: { reason: "Provider is closing", resources: [] }, - }); + return new AnalysisCapabilityUnavailableError( + PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id, + OPERATION, + "Browser scenario provider is closing.", + ); } #bindCleanup( diff --git a/tests/boundary/browser/cdpTargetSessionOwner.test.ts b/tests/boundary/browser/cdpTargetSessionOwner.test.ts index 8abf21802f..9b97b1e0ab 100644 --- a/tests/boundary/browser/cdpTargetSessionOwner.test.ts +++ b/tests/boundary/browser/cdpTargetSessionOwner.test.ts @@ -9,11 +9,44 @@ import { CdpConnection } from "../../../src/browser/CdpConnection.js"; import { CdpBrowserProvider } from "../../../src/browser/CdpBrowserProvider.js"; import { inspectWebPageInputSchema } from "../../../src/domain/browserObservation.js"; import { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; -import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js"; +import { + AnalysisCancelledError, + AnalysisCapabilityUnavailableError, +} from "../../../src/domain/analysisErrorCore.js"; import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; import { startFakeCdpBrowser } from "../../fixtures/fakeCdpBrowser.js"; import { awaitAcknowledgement } from "../../fixtures/lifecycleAcknowledgement.js"; +it.each(["connect", "acquireBrowserSocket"] as const)( + "refuses %s after clean shutdown without inventing retained cleanup", + async (method) => { + const owner = new CdpTargetSessionOwner(); + await owner.close(); + const endpoint = "ws://127.0.0.1:9/devtools/browser/closed"; + const failure = await ( + method === "connect" + ? owner.connect(endpoint, "inspect_web_page", {}, "not-acquired") + : owner.acquireBrowserSocket( + endpoint, + "inspect_web_page", + {}, + { + commandTimeoutMs: 100, + handshakeTimeoutMs: 100, + }, + ) + ).catch((cause: unknown) => cause); + expect(failure).toBeInstanceOf(AnalysisCapabilityUnavailableError); + expect(failure).toMatchObject({ + providerId: "rea-cdp-browser", + operation: "inspect_web_page", + cleanupIncomplete: false, + cleanupResources: [], + }); + await owner.close(); + }, +); + it("joins a late owned handshake and waits for its active operation", async () => { const http = createServer(); const webSockets = new WebSocketServer({ noServer: true });