From 4daea941bfeeb426886f2d048aed7f3cb2dbf97b Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:51:23 +0800 Subject: [PATCH] fix(cli): retry retained browser scenario cleanup Retry retained browser scenario cleanup before the one-shot CLI exits. Preserve primary failures, partial observations and completed Evidence when shutdown fails. Closes #1771. --- src/cliBrowserScenarioCommands.ts | 41 ++- .../cli/browserScenarioCleanup.test.ts | 302 ++++++++++++++++++ 2 files changed, 338 insertions(+), 5 deletions(-) create mode 100644 tests/boundary/cli/browserScenarioCleanup.test.ts diff --git a/src/cliBrowserScenarioCommands.ts b/src/cliBrowserScenarioCommands.ts index a1a1f86e1..c42d4e25d 100644 --- a/src/cliBrowserScenarioCommands.ts +++ b/src/cliBrowserScenarioCommands.ts @@ -6,7 +6,9 @@ import { CLI_COMMANDS } from "./cliCommandNames.js"; import { parseCliJsonInput, resolveCliJsonPaths } from "./cliJsonInput.js"; import { logCliCommand } from "./cliLogging.js"; import { AnalysisInputError } from "./domain/analysisErrorCore.js"; +import { AnalysisError } from "./domain/analysisErrorBase.js"; import { projectAnalysisError } from "./domain/analysisErrorProjection.js"; +import { BrowserObservationError } from "./domain/browserObservationError.js"; import { projectInputIssues } from "./domain/inputIssueProjection.js"; import { browserScenarioSchema } from "./domain/browserScenario.js"; import type { JsonValue } from "./domain/jsonValue.js"; @@ -46,11 +48,40 @@ export const registerBrowserScenarioCommands = ( // Load Playwright only for this command, not during CLI startup. const { createBrowserScenarioProvider } = await import("./composition/browserScenario.js"); - const result = await captureBrowserScenario( - createBrowserScenarioProvider(environment), - scenario.data, - ); - return result.ok ? result.value : cliError(result.error); + const provider = createBrowserScenarioProvider(environment); + let result: + | Awaited> + | undefined; + try { + result = await captureBrowserScenario(provider, scenario.data); + return result.ok ? result.value : cliError(result.error); + } finally { + try { + await provider?.close?.(); + } catch (cause: unknown) { + // Failed captures already retain their primary error and partial + // observations. A cleanup retry must not replace that outcome. + if (result?.ok) + throw new BrowserObservationError(OPERATION, "cleanup_failed", { + cause, + cleanup: + cause instanceof AnalysisError && cause.cleanup !== undefined + ? cause.cleanup + : { + reason: + cause instanceof Error + ? cause.message + : String(cause), + resources: + cause instanceof AnalysisError && + cause.cleanupResources.length > 0 + ? cause.cleanupResources + : ["browser_transport"], + }, + partialObservation: result.value, + }); + } + } }), }); }; diff --git a/tests/boundary/cli/browserScenarioCleanup.test.ts b/tests/boundary/cli/browserScenarioCleanup.test.ts new file mode 100644 index 000000000..45df1b49b --- /dev/null +++ b/tests/boundary/cli/browserScenarioCleanup.test.ts @@ -0,0 +1,302 @@ +import { Cli } from "incur"; +import { expect, it, vi } from "vitest"; + +import type { BrowserScenarioCapturePort } from "../../../src/application/BrowserScenarioCapturePort.js"; +import { createBrowserScenarioEvidence } from "../../../src/application/BrowserScenarioEvidence.js"; +import type { BrowserScenarioSessionPort } from "../../../src/browser/BrowserScenarioSessionPort.js"; +import { PlaywrightBrowserScenarioProvider } from "../../../src/browser/PlaywrightBrowserScenarioProvider.js"; +import { PlaywrightScenarioBrowserCleanupOwner } from "../../../src/browser/PlaywrightScenarioBrowser.js"; +import { registerBrowserScenarioCommands } from "../../../src/cliBrowserScenarioCommands.js"; +import { sanitizeBrowserUrl } from "../../../src/domain/browserObservation.js"; +import { + browserScenarioSchema, + type BrowserScenario, +} from "../../../src/domain/browserScenario.js"; +import type { BrowserScenarioAction } from "../../../src/domain/browserScenarioValues.js"; +import { browserStepArtifactsSchema } from "../../../src/domain/browserScenarioCaptureValues.js"; +import { silentLogger } from "../../../src/logger.js"; +import { + AnalysisCancelledError, + AnalysisTimeoutError, +} from "../../../src/domain/analysisErrorCore.js"; +import { BrowserObservationError } from "../../../src/domain/browserObservationError.js"; +import { parseEvidence } from "../../../src/domain/evidence.js"; +import { jsonObjectSchema } from "../../../src/domain/jsonValue.js"; +import { ok } from "../../../src/domain/result.js"; + +const state = vi.hoisted(() => ({ + provider: undefined as BrowserScenarioCapturePort | undefined, +})); +vi.mock("../../../src/composition/browserScenario.js", () => ({ + createBrowserScenarioProvider: () => state.provider, +})); + +const scenario = browserScenarioSchema.parse({ + browser: { mode: "launch", executable_path: "/fixture/chromium" }, + start_url: { url: "https://app.example.test/" }, + actions: [{ step_id: "wait", action: "wait_for_timeout", duration_ms: 1 }], + storage: {}, + capture: { after_each_step: ["url"], at_end: [], events: [] }, +}); + +// Controlled session failures exercise the real provider and cleanup owner; +// this fixture opens no browser process or profile directory. +class RetrySession implements BrowserScenarioSessionPort { + readonly mode = "launch"; + readonly processOwnership = "provider-owned"; + readonly product = "Fixture Chromium"; + readonly version = "1"; + readonly initialUrl = "https://app.example.test/"; + closeBrowserCalls = 0; + profileRemovalCalls = 0; + readonly cleanup = new PlaywrightScenarioBrowserCleanupOwner({ + closeBrowser: async () => { + this.closeBrowserCalls += 1; + if (this.closeBrowserCalls <= this.closeFailures) + throw new Error(`injected close failure ${this.closeBrowserCalls}`); + }, + removeProfile: async () => { + expect(this.cleanup.browserClosed).toBe(true); + this.profileRemovalCalls += 1; + }, + }); + + constructor(private readonly closeFailures = 1) {} + + currentUrl() { + return this.initialUrl; + } + sanitizeUrl(value: string) { + return sanitizeBrowserUrl(value); + } + setStep(_index: number) {} + nextEventSequence() { + return 1; + } + lastEventSequence() { + return 0; + } + events() { + return { retained: 0, dropped: 0, items: [] }; + } + async perform(_action: BrowserScenarioAction) {} + async capture( + requested: ReadonlySet< + BrowserScenario["capture"]["after_each_step"][number] + >, + ) { + const artifact = ( + kind: BrowserScenario["capture"]["after_each_step"][number], + ) => + !requested.has(kind) + ? { state: "not_requested" } + : kind === "url" + ? { state: "captured", value: this.sanitizeUrl(this.initialUrl) } + : { state: "missing", reason: "fixture unavailable" }; + return browserStepArtifactsSchema.parse({ + screenshot: artifact("screenshot"), + dom: artifact("dom"), + accessibility: artifact("accessibility"), + url: artifact("url"), + history: artifact("history"), + storage: artifact("storage"), + }); + } + async close() { + await this.cleanup.close(); + return "terminated-owned-process" as const; + } + redactError(error: unknown) { + return error instanceof Error ? error.message : "fixture failure"; + } +} + +const serveScenario = async () => { + const cli = Cli.create("browser-cleanup-test", { sync: false }); + registerBrowserScenarioCommands(cli, silentLogger, {}); + let stdout = ""; + let exitCode = 0; + await cli.serve( + ["capture-browser-scenario", JSON.stringify(scenario), "--json"], + { + env: {}, + exit: (code) => { + exitCode = code; + }, + stdout: (text) => { + stdout += text; + }, + }, + ); + return { + output: jsonObjectSchema.parse(JSON.parse(stdout)), + exitCode, + processExitCode: process.exitCode, + }; +}; + +it("retries retained browser cleanup before the one-shot CLI releases its provider", async () => { + const savedExitCode = process.exitCode; + const session = new RetrySession(); + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + const close = vi.spyOn(provider, "close"); + state.provider = provider; + try { + const result = await serveScenario(); + expect(result.processExitCode).toBe(1); + expect(result.output).toMatchObject({ + code: "cleanup_incomplete", + category: "execution_failure", + details: { + cleanup: "incomplete", + resources: ["browser_transport", "browser_profile"], + partial_observation: { + capture: { browser: { cleanup: "incomplete" } }, + }, + }, + }); + const details = jsonObjectSchema.parse(result.output.details); + const partial = jsonObjectSchema.parse(details.partial_observation); + expect(jsonObjectSchema.parse(partial.capture).steps).toHaveLength(2); + expect(close).toHaveBeenCalledTimes(1); + expect(session.closeBrowserCalls).toBe(2); + expect(session.profileRemovalCalls).toBe(1); + } finally { + // The red baseline must also release this controlled owner. + await provider.close(); + state.provider = undefined; + process.exitCode = savedExitCode; + } +}); + +it("preserves the initial partial observation when the provider cleanup retry also fails", async () => { + const savedExitCode = process.exitCode; + const session = new RetrySession(2); + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + const close = vi.spyOn(provider, "close"); + state.provider = provider; + try { + const result = await serveScenario(); + expect(result.output).toMatchObject({ code: "cleanup_incomplete" }); + expect(JSON.stringify(result.output)).toContain("injected close failure 1"); + expect(JSON.stringify(result.output)).not.toContain( + "injected close failure 2", + ); + expect(close).toHaveBeenCalledTimes(1); + expect(session.closeBrowserCalls).toBe(2); + expect(session.profileRemovalCalls).toBe(0); + // Retained ownership remains retryable after the command's failed retry. + await provider.close(); + expect(session.closeBrowserCalls).toBe(3); + expect(session.profileRemovalCalls).toBe(1); + } finally { + await provider.close(); + state.provider = undefined; + process.exitCode = savedExitCode; + } +}); + +it("retains completed Evidence when provider shutdown fails after a successful capture", async () => { + const savedExitCode = process.exitCode; + const provider = new PlaywrightBrowserScenarioProvider( + async () => new RetrySession(0), + ); + const captured = await provider.captureScenario(scenario); + if (!captured.ok) throw captured.error; + const evidence = createBrowserScenarioEvidence( + scenario, + captured.value, + provider.identity(), + ); + const close = vi.fn().mockRejectedValue( + new BrowserObservationError("capture_browser_scenario", "cleanup_failed", { + cleanup: { + reason: "injected shutdown failure", + resources: ["browser_profile"], + }, + }), + ); + state.provider = { + identity: () => provider.identity(), + captureScenario: async () => ok(captured.value), + close, + }; + try { + const result = await serveScenario(); + expect(result.processExitCode).toBe(1); + expect(result.output).toMatchObject({ + code: "cleanup_incomplete", + details: { resources: ["browser_profile"] }, + }); + const details = jsonObjectSchema.parse(result.output.details); + expect(parseEvidence(details.partial_observation)).toEqual(evidence); + expect(close).toHaveBeenCalledTimes(1); + } finally { + await provider.close(); + state.provider = undefined; + process.exitCode = savedExitCode; + } +}); + +it("preserves cancellation diagnostics while retrying retained cleanup", async () => { + const savedExitCode = process.exitCode; + const session = new RetrySession(); + vi.spyOn(session, "capture").mockRejectedValue( + new AnalysisCancelledError("capture_browser_scenario"), + ); + const provider = new PlaywrightBrowserScenarioProvider(async () => session); + state.provider = provider; + try { + const result = await serveScenario(); + expect(result.output).toMatchObject({ + code: "cleanup_incomplete", + category: "execution_failure", + details: { + cleanup: "incomplete", + diagnostics: { + primary_error: { code: "cancelled", category: "cancelled" }, + }, + partial_observation: { kind: "browser-scenario-observation" }, + }, + }); + expect(session.closeBrowserCalls).toBe(2); + expect(session.profileRemovalCalls).toBe(1); + } finally { + await provider.close(); + state.provider = undefined; + process.exitCode = savedExitCode; + } +}); + +it("preserves a thrown capture error when provider shutdown also rejects", async () => { + const savedExitCode = process.exitCode; + const provider = new PlaywrightBrowserScenarioProvider( + async () => new RetrySession(0), + ); + const primary = new AnalysisTimeoutError("capture_browser_scenario", 123); + const close = vi + .fn() + .mockRejectedValue(new Error("secondary shutdown failure")); + state.provider = { + identity: () => provider.identity(), + captureScenario: async () => { + throw primary; + }, + close, + }; + try { + const result = await serveScenario(); + expect(result.output).toMatchObject({ + code: "provider_timeout", + category: "timeout", + }); + expect(JSON.stringify(result.output)).not.toContain( + "secondary shutdown failure", + ); + expect(close).toHaveBeenCalledTimes(1); + } finally { + await provider.close(); + state.provider = undefined; + process.exitCode = savedExitCode; + } +});