fix(browser): release attached emulation overrides (#3778)

* fix(browser): release attached emulation overrides

* fix(browser): preserve unowned environment state

* fix(browser): coordinate attached environment cleanup

src/browser/PlaywrightScenarioBrowser.test.ts

@@ -1,5 +1,10 @@
 import { afterEach, expect, it, vi } from "vitest";
-import type { Browser } from "playwright-core";
+import type {
+  Browser,
+  BrowserContext,
+  CDPSession,
+  Page,
+} from "playwright-core";
 
 import { browserScenarioSchema } from "../domain/browserScenario.js";
 import {
@@ -157,11 +162,12 @@ it("keeps attached-target ownership after the cleanup response timeout", async (
 
 it("does not configure a late attached connection after cancellation", async () => {
   const connection =
-    deferred<Pick<Browser, "contexts" | "close" | "version">>();
+    deferred<Pick<Browser, "contexts" | "close" | "isConnected" | "version">>();
   const connectionStarted = deferred<void>();
   const connectedBrowser = {
     close: vi.fn(async () => undefined),
     contexts: vi.fn(() => []),
+    isConnected: vi.fn(() => true),
     version: () => "fixture",
   };
   const controller = new AbortController();
@@ -211,6 +217,7 @@ it("closes an attached connection when cancellation starts target discovery", as
       controller.abort();
       return [];
     }),
+    isConnected: vi.fn(() => true),
     version: () => "fixture",
   };
   const launcher = {
@@ -245,6 +252,265 @@ it("closes an attached connection when cancellation starts target discovery", as
   expect(connectedBrowser.close).toHaveBeenCalledOnce();
 });
 
+const attachedBrowserFixture = (
+  callbacks: {
+    readonly send?: (method: string, parameters?: unknown) => Promise<unknown>;
+    readonly emulateMedia?: (
+      options: Parameters<Page["emulateMedia"]>[0],
+    ) => Promise<void>;
+    readonly detach?: () => Promise<void>;
+    readonly closeBrowser?: () => Promise<void>;
+    readonly isTargetClosed?: () => boolean;
+    readonly isBrowserConnected?: () => boolean;
+  } = {},
+) => {
+  const discoverySession = {
+    send: vi.fn(async () => ({ targetInfo: { targetId: "page-1" } })),
+    detach: vi.fn(async () => undefined),
+  } as unknown as CDPSession;
+  const environmentSession = {
+    send: vi.fn(callbacks.send ?? (async () => undefined)),
+    detach: vi.fn(callbacks.detach ?? (async () => undefined)),
+  } as unknown as CDPSession;
+  const page = {
+    emulateMedia: vi.fn(callbacks.emulateMedia ?? (async () => undefined)),
+    isClosed: vi.fn(callbacks.isTargetClosed ?? (() => false)),
+  } as unknown as Page;
+  const context = {
+    pages: () => [page],
+    newCDPSession: vi
+      .fn()
+      .mockResolvedValueOnce(discoverySession)
+      .mockResolvedValueOnce(environmentSession),
+  } as unknown as BrowserContext;
+  const browser = {
+    close: vi.fn(callbacks.closeBrowser ?? (async () => undefined)),
+    contexts: () => [context],
+    isConnected: vi.fn(callbacks.isBrowserConnected ?? (() => true)),
+    version: () => "fixture",
+  } as unknown as Pick<
+    Browser,
+    "contexts" | "close" | "isConnected" | "version"
+  >;
+  const scenario = browserScenarioSchema.parse({
+    browser: {
+      mode: "connect",
+      cdp_endpoint: "http://127.0.0.1:9222",
+      target_id: "page-1",
+    },
+    start_url: { url: "https://example.test" },
+    actions: [{ step_id: "wait", action: "wait_for_timeout", duration_ms: 1 }],
+  });
+  return {
+    browser,
+    environmentSession,
+    page,
+    open: (signal?: AbortSignal) =>
+      openPlaywrightScenarioBrowser(
+        scenario,
+        {},
+        {
+          launcher: {
+            connectOverCDP: async () => browser,
+            launchPersistentContext: async () => {
+              throw new Error("Unexpected launch");
+            },
+          },
+          signal,
+        },
+      ),
+  };
+};
+
+it("restores its environment overrides before releasing an attached browser", async () => {
+  const cleanupOrder: string[] = [];
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        method === "Emulation.setTimezoneOverride" &&
+        (parameters as { timezoneId?: string }).timezoneId === ""
+      )
+        cleanupOrder.push("timezone");
+      if (
+        method === "Emulation.setLocaleOverride" &&
+        (parameters as { locale?: string }).locale === ""
+      )
+        cleanupOrder.push("locale");
+      if (method === "Emulation.clearDeviceMetricsOverride")
+        cleanupOrder.push("device-metrics");
+    },
+    emulateMedia: async (options) => {
+      if (options?.colorScheme === null) cleanupOrder.push("media");
+    },
+    detach: async () => {
+      cleanupOrder.push("session");
+    },
+    closeBrowser: async () => {
+      cleanupOrder.push("browser");
+    },
+  });
+  const opened = await fixture.open();
+  await opened.cleanup.close();
+
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.setLocaleOverride",
+    { locale: "en-US" },
+  );
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.setLocaleOverride",
+    { locale: "" },
+  );
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.setTimezoneOverride",
+    { timezoneId: "" },
+  );
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.clearDeviceMetricsOverride",
+  );
+  expect(fixture.page.emulateMedia).toHaveBeenLastCalledWith({
+    colorScheme: null,
+    reducedMotion: null,
+  });
+  expect(fixture.environmentSession.detach).toHaveBeenCalledOnce();
+  expect(fixture.browser.close).toHaveBeenCalledOnce();
+  expect(cleanupOrder).toEqual([
+    "timezone",
+    "locale",
+    "device-metrics",
+    "media",
+    "session",
+    "browser",
+  ]);
+});
+
+it("retains attached environment ownership until a failed reset can be retried", async () => {
+  let localeResetAttempts = 0;
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        method === "Emulation.setLocaleOverride" &&
+        (parameters as { locale?: string }).locale === ""
+      ) {
+        localeResetAttempts += 1;
+        if (localeResetAttempts === 1) throw new Error("reset interrupted");
+      }
+    },
+  });
+  const opened = await fixture.open();
+
+  await expect(opened.cleanup.close()).rejects.toMatchObject({
+    reason: "cleanup_failed",
+    cleanup: { reason: "reset interrupted" },
+  });
+  expect(fixture.browser.close).not.toHaveBeenCalled();
+  expect(fixture.environmentSession.detach).not.toHaveBeenCalled();
+
+  await opened.cleanup.close();
+  expect(localeResetAttempts).toBe(2);
+  expect(fixture.environmentSession.detach).toHaveBeenCalledOnce();
+  expect(fixture.browser.close).toHaveBeenCalledOnce();
+});
+
+it("does not clear an attached environment override it failed to acquire", async () => {
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        method === "Emulation.setLocaleOverride" &&
+        (parameters as { locale?: string }).locale === "en-US"
+      )
+        throw new Error("caller already owns locale");
+    },
+  });
+
+  await expect(fixture.open()).rejects.toThrow("caller already owns locale");
+  expect(fixture.environmentSession.send).not.toHaveBeenCalledWith(
+    "Emulation.setLocaleOverride",
+    { locale: "" },
+  );
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.clearDeviceMetricsOverride",
+  );
+  expect(fixture.environmentSession.detach).toHaveBeenCalledOnce();
+  expect(fixture.browser.close).toHaveBeenCalledOnce();
+});
+
+it("waits for a cancelled environment mutation before restoring it", async () => {
+  const localeStarted = deferred<void>();
+  const localeResult = deferred<void>();
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        method === "Emulation.setLocaleOverride" &&
+        (parameters as { locale?: string }).locale === "en-US"
+      ) {
+        localeStarted.resolve(undefined);
+        await localeResult.promise;
+      }
+    },
+  });
+  const controller = new AbortController();
+  const opening = fixture.open(controller.signal);
+  await localeStarted.promise;
+  const cancelled = expect(opening).rejects.toMatchObject({
+    _tag: "AnalysisCancelledError",
+  });
+  controller.abort();
+  await new Promise((resolve) => setImmediate(resolve));
+  expect(fixture.environmentSession.send).not.toHaveBeenCalledWith(
+    "Emulation.setLocaleOverride",
+    { locale: "" },
+  );
+
+  localeResult.resolve(undefined);
+  await cancelled;
+  expect(fixture.environmentSession.send).toHaveBeenCalledWith(
+    "Emulation.setLocaleOverride",
+    { locale: "" },
+  );
+  expect(fixture.environmentSession.detach).toHaveBeenCalledOnce();
+  expect(fixture.browser.close).toHaveBeenCalledOnce();
+});
+
+it("releases cleanup ownership when the attached target is destroyed", async () => {
+  let targetClosed = false;
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        targetClosed &&
+        method === "Emulation.setTimezoneOverride" &&
+        (parameters as { timezoneId?: string }).timezoneId === ""
+      )
+        throw new Error("Target closed");
+    },
+    isTargetClosed: () => targetClosed,
+  });
+  const opened = await fixture.open();
+  targetClosed = true;
+
+  await opened.cleanup.close();
+  expect(fixture.browser.close).toHaveBeenCalledOnce();
+});
+
+it("releases cleanup ownership when the attached browser disconnects", async () => {
+  let connected = true;
+  const fixture = attachedBrowserFixture({
+    send: async (method, parameters) => {
+      if (
+        !connected &&
+        method === "Emulation.setTimezoneOverride" &&
+        (parameters as { timezoneId?: string }).timezoneId === ""
+      )
+        throw new Error("Browser disconnected");
+    },
+    isBrowserConnected: () => connected,
+  });
+  const opened = await fixture.open();
+  connected = false;
+
+  await opened.cleanup.close();
+  expect(fixture.browser.close).not.toHaveBeenCalled();
+});
+
 it("cancels same-target queued connection before acquiring a browser", async () => {
   const makeScenario = (targetId: string) =>
     browserScenarioSchema.parse({
@@ -350,7 +616,7 @@ it("serializes concurrent attach across loopback spellings of one browser", asyn
       ],
     });
   const firstConnection =
-    deferred<Pick<Browser, "contexts" | "close" | "version">>();
+    deferred<Pick<Browser, "contexts" | "close" | "isConnected" | "version">>();
   let rejectFirstConnection: ((cause: Error) => void) | undefined;
   const failingConnection = new Promise<never>((_resolve, reject) => {
     rejectFirstConnection = reject;
@@ -399,6 +665,7 @@ it("serializes concurrent attach across loopback spellings of one browser", asyn
   firstConnection.resolve({
     close: vi.fn(async () => undefined),
     contexts: vi.fn(() => []),
+    isConnected: vi.fn(() => true),
     version: () => "fixture",
   });
   await expect(first).rejects.toThrow();

src/browser/PlaywrightScenarioBrowser.ts

@@ -2,7 +2,13 @@ import { chmod, mkdtemp, rm } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join, resolve } from "node:path";
 
-import type { Browser, BrowserContext, Page, chromium } from "playwright-core";
+import type {
+  Browser,
+  BrowserContext,
+  CDPSession,
+  Page,
+  chromium,
+} from "playwright-core";
 
 import type { BrowserScenario } from "../domain/browserScenario.js";
 import { AnalysisError } from "../domain/analysisErrorBase.js";
@@ -314,39 +320,148 @@ const findConnectedPage = async (
   throw new BrowserObservationError(OPERATION, "target_not_found");
 };
 
-const configureAttachedEnvironment = async (
-  context: BrowserContext,
-  page: Page,
-  environment: BrowserScenario["environment"],
-  signal: AbortSignal | undefined,
-): Promise<void> => {
-  // setViewportSize also resizes the external browser window. The scenario's
-  // CDP metrics override below supplies the viewport and expires on disconnect.
-  await page.emulateMedia({
-    colorScheme: environment.color_scheme,
-    reducedMotion: environment.reduced_motion,
-  });
-  throwIfScenarioCancelled(signal);
-  const session = await context.newCDPSession(page);
-  throwIfScenarioCancelled(signal);
-  // Chromium resets these overrides when the CDP session detaches. Keep it on
-  // the scenario transport until cleanup disconnects the attached browser.
-  await session.send("Emulation.setDeviceMetricsOverride", {
-    width: environment.viewport.width,
-    height: environment.viewport.height,
-    deviceScaleFactor: environment.viewport.device_scale_factor,
-    mobile: false,
-    dontSetVisibleSize: true,
-  });
-  throwIfScenarioCancelled(signal);
-  await session.send("Emulation.setLocaleOverride", {
-    locale: environment.locale,
-  });
-  throwIfScenarioCancelled(signal);
-  await session.send("Emulation.setTimezoneOverride", {
-    timezoneId: environment.timezone,
-  });
-};
+/** Owns the target state changed while one scenario uses an attached page. */
+class AttachedScenarioEnvironmentOwner {
+  #mediaOverride = false;
+  #deviceMetricsOverride = false;
+  #localeOverride = false;
+  #timezoneOverride = false;
+  #detached = false;
+  #application: Promise<void> | undefined;
+
+  private constructor(
+    private readonly page: Page,
+    private readonly session: CDPSession,
+  ) {}
+
+  static async create(
+    context: BrowserContext,
+    page: Page,
+  ): Promise<AttachedScenarioEnvironmentOwner> {
+    return new AttachedScenarioEnvironmentOwner(
+      page,
+      await context.newCDPSession(page),
+    );
+  }
+
+  get restored(): boolean {
+    return (
+      !this.#mediaOverride &&
+      !this.#deviceMetricsOverride &&
+      !this.#localeOverride &&
+      !this.#timezoneOverride
+    );
+  }
+
+  get targetClosed(): boolean {
+    return this.page.isClosed();
+  }
+
+  apply(
+    environment: BrowserScenario["environment"],
+    signal: AbortSignal | undefined,
+  ): Promise<void> {
+    const application =
+      this.#application ?? this.#applyEnvironment(environment, signal);
+    this.#application = application;
+    return application;
+  }
+
+  async #applyEnvironment(
+    environment: BrowserScenario["environment"],
+    signal: AbortSignal | undefined,
+  ): Promise<void> {
+    await this.page.emulateMedia({
+      colorScheme: environment.color_scheme,
+      reducedMotion: environment.reduced_motion,
+    });
+    this.#mediaOverride = true;
+    throwIfScenarioCancelled(signal);
+
+    // setViewportSize also resizes the external browser window. CDP metrics
+    // provide the scenario viewport without changing that caller-owned window.
+    await this.session.send("Emulation.setDeviceMetricsOverride", {
+      width: environment.viewport.width,
+      height: environment.viewport.height,
+      deviceScaleFactor: environment.viewport.device_scale_factor,
+      mobile: false,
+      dontSetVisibleSize: true,
+    });
+    this.#deviceMetricsOverride = true;
+    throwIfScenarioCancelled(signal);
+
+    await this.session.send("Emulation.setLocaleOverride", {
+      locale: environment.locale,
+    });
+    this.#localeOverride = true;
+    throwIfScenarioCancelled(signal);
+
+    await this.session.send("Emulation.setTimezoneOverride", {
+      timezoneId: environment.timezone,
+    });
+    this.#timezoneOverride = true;
+    throwIfScenarioCancelled(signal);
+  }
+
+  async close(): Promise<void> {
+    if (this.#detached) return;
+    // The execution boundary can report cancellation before a pending CDP
+    // command settles. Wait for that producer so every successful mutation is
+    // recorded before deciding which state this owner must restore.
+    try {
+      await this.#application;
+    } catch {
+      // The operation path reports application failures. Cleanup still owns
+      // and restores every mutation that completed before the failure.
+    }
+    const failures: unknown[] = [];
+    if (this.#timezoneOverride) {
+      try {
+        await this.session.send("Emulation.setTimezoneOverride", {
+          timezoneId: "",
+        });
+        this.#timezoneOverride = false;
+      } catch (cause: unknown) {
+        failures.push(cause);
+      }
+    }
+    if (this.#localeOverride) {
+      try {
+        await this.session.send("Emulation.setLocaleOverride", { locale: "" });
+        this.#localeOverride = false;
+      } catch (cause: unknown) {
+        failures.push(cause);
+      }
+    }
+    if (this.#deviceMetricsOverride) {
+      try {
+        await this.session.send("Emulation.clearDeviceMetricsOverride");
+        this.#deviceMetricsOverride = false;
+      } catch (cause: unknown) {
+        failures.push(cause);
+      }
+    }
+    if (this.#mediaOverride) {
+      try {
+        await this.page.emulateMedia({
+          colorScheme: null,
+          reducedMotion: null,
+        });
+        this.#mediaOverride = false;
+      } catch (cause: unknown) {
+        failures.push(cause);
+      }
+    }
+    if (failures.length === 1) throw failures[0];
+    if (failures.length > 1)
+      throw new AggregateError(
+        failures,
+        "Attached browser environment restoration failed",
+      );
+    await this.session.detach();
+    this.#detached = true;
+  }
+}
 
 /** Preserve an operation failure while reporting any incomplete cleanup. */
 export const failBrowserScenarioOperation = async (
@@ -389,7 +504,7 @@ type ScenarioBrowserLauncher = Pick<
 > & {
   readonly connectOverCDP: (
     endpoint: string,
-  ) => Promise<Pick<Browser, "contexts" | "close" | "version">>;
+  ) => Promise<Pick<Browser, "contexts" | "close" | "isConnected" | "version">>;
 };
 
 type AttachedBrowserRequest = Extract<
@@ -406,6 +521,7 @@ const openAttachedScenarioBrowser = async (
 ): Promise<OpenedScenarioBrowser> => {
   const lease = await acquireAttachedScenario(browserRequest, signal);
   let cleanupOwner: PlaywrightScenarioBrowserCleanupOwner | undefined;
+  let environmentOwner: AttachedScenarioEnvironmentOwner | undefined;
   try {
     const priorOwner = lease.admission.cleanupOwner;
     if (priorOwner !== undefined) await priorOwner.close();
@@ -414,7 +530,43 @@ const openAttachedScenarioBrowser = async (
     const browser = await launcher.connectOverCDP(browserRequest.cdp_endpoint);
     const cleanup: PlaywrightScenarioBrowserCleanupOwner =
       new PlaywrightScenarioBrowserCleanupOwner({
-        closeBrowser: () => browser.close(),
+        closeBrowser: async () => {
+          let environmentFailure: unknown;
+          const ownedEnvironment = environmentOwner;
+          if (ownedEnvironment !== undefined) {
+            try {
+              await ownedEnvironment.close();
+              environmentOwner = undefined;
+            } catch (cause: unknown) {
+              environmentFailure = cause;
+              const browserConnected = browser.isConnected();
+              if (ownedEnvironment.targetClosed || !browserConnected) {
+                // A destroyed target has no remaining emulation state. A lost
+                // transport cannot be retained or retried; release its local
+                // owner so later discovery can establish current target state.
+                environmentOwner = undefined;
+                if (!browserConnected) return;
+              } else if (!ownedEnvironment.restored) {
+                // Keep the live transport and target session for a cleanup
+                // retry while any target override remains.
+                throw cause;
+              }
+            }
+          }
+          try {
+            await browser.close();
+            // Once target state is restored, disconnecting the transport is a
+            // definitive fallback for an explicit session detach failure.
+            environmentOwner = undefined;
+          } catch (browserFailure: unknown) {
+            if (environmentFailure !== undefined)
+              throw new AggregateError(
+                [environmentFailure, browserFailure],
+                "Attached browser environment cleanup failed",
+              );
+            throw browserFailure;
+          }
+        },
         onSettled: (browserClosed) => {
           if (browserClosed && lease.admission.cleanupOwner === cleanup)
             lease.admission.cleanupOwner = undefined;
@@ -431,14 +583,15 @@ const openAttachedScenarioBrowser = async (
       signal,
     );
     throwIfScenarioCancelled(signal);
-    await withPlaywrightExecutionBoundary(
+    const configuredEnvironment = await withPlaywrightExecutionBoundary(
       () =>
-        configureAttachedEnvironment(
-          target.context,
-          target.page,
-          environment,
-          signal,
-        ),
+        AttachedScenarioEnvironmentOwner.create(target.context, target.page),
+      undefined,
+      signal,
+    );
+    environmentOwner = configuredEnvironment;
+    await withPlaywrightExecutionBoundary(
+      () => configuredEnvironment.apply(environment, signal),
       undefined,
       signal,
     );