fix(browser): let attached workers exit after CDP use (#122103)

* fix(browser): retire attached runtime Playwright CDP adapter on disposal (#122065)

* fix(browser): use optional chaining on refresh in CDP adapter retirement

* refactor(browser): simplify attached adapter disposal

---------

Co-authored-by: Peter Steinberger <steipete@gmail.com>

extensions/browser/src/attached-browser-tool-runtime.test.ts

@@ -7,6 +7,7 @@ const mocks = vi.hoisted(() => ({
   createBrowserTool: vi.fn(),
   startBrowserBridgeServer: vi.fn(),
   stopBrowserBridgeServer: vi.fn(),
+  closePlaywrightBrowserConnection: vi.fn(),
 }));
 
 vi.mock("./browser-tool.js", () => ({
@@ -18,11 +19,16 @@ vi.mock("./browser/bridge-server.js", () => ({
   stopBrowserBridgeServer: mocks.stopBrowserBridgeServer,
 }));
 
+vi.mock("./browser/pw-session.js", () => ({
+  closePlaywrightBrowserConnection: mocks.closePlaywrightBrowserConnection,
+}));
+
 import { createAttachedBrowserToolRuntime } from "./attached-browser-tool-runtime.js";
 
 describe("attached Browser tool runtime", () => {
   beforeEach(() => {
     vi.clearAllMocks();
+    mocks.closePlaywrightBrowserConnection.mockResolvedValue(undefined);
     mocks.createBrowserTool.mockReturnValue({ name: "browser" });
     mocks.startBrowserBridgeServer.mockResolvedValue({
       baseUrl: "http://127.0.0.1:18443",
@@ -80,11 +86,13 @@ describe("attached Browser tool runtime", () => {
     const sourcePath = path.join(workspaceDir, "source.png");
     await fs.writeFile(sourcePath, "screenshot-bytes");
     const artifactPath = await toolParams.persistScreenshot({ sourcePath, type: "png" });
-    expect(artifactPath).toMatch(
+    expect(artifactPath.replaceAll("\\", "/")).toMatch(
       /\.artifacts\/cloud-worker-browser\/screenshot-[a-f0-9]{16}\.png$/u,
     );
     expect(await fs.readFile(artifactPath, "utf8")).toBe("screenshot-bytes");
-    expect((await fs.stat(artifactPath)).mode & 0o777).toBe(0o600);
+    if (process.platform !== "win32") {
+      expect((await fs.stat(artifactPath)).mode & 0o777).toBe(0o600);
+    }
     const filesBeforeFailure = await fs.readdir(path.dirname(artifactPath));
     await expect(
       toolParams.persistScreenshot({
@@ -97,10 +105,39 @@ describe("attached Browser tool runtime", () => {
 
     await runtime.dispose();
     expect(mocks.stopBrowserBridgeServer).toHaveBeenCalledWith({ marker: "bridge-server" });
+    expect(mocks.closePlaywrightBrowserConnection).toHaveBeenCalledWith({
+      cdpUrl: "http://127.0.0.1:9222",
+    });
     expect(ensureAttachTarget).toHaveBeenCalledOnce();
     await fs.rm(workspaceDir, { recursive: true, force: true });
   });
 
+  it("retries exact Playwright CDP adapter disposal after a failed disconnect", async () => {
+    const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "attached-browser-workspace-"));
+    const runtime = await createAttachedBrowserToolRuntime({
+      cdpUrl: "http://127.0.0.1:9222",
+      ensureAttachTarget: async () => {},
+      workspaceDir,
+    });
+    mocks.closePlaywrightBrowserConnection
+      .mockRejectedValueOnce(new Error("disconnect failed"))
+      .mockResolvedValueOnce(undefined);
+
+    await expect(runtime.dispose()).rejects.toThrow("disconnect failed");
+    await expect(runtime.dispose()).resolves.toBeUndefined();
+
+    expect(mocks.stopBrowserBridgeServer).toHaveBeenCalledTimes(2);
+    expect(mocks.closePlaywrightBrowserConnection).toHaveBeenCalledTimes(2);
+    expect(mocks.closePlaywrightBrowserConnection).toHaveBeenNthCalledWith(1, {
+      cdpUrl: "http://127.0.0.1:9222",
+    });
+    expect(mocks.closePlaywrightBrowserConnection).toHaveBeenNthCalledWith(2, {
+      cdpUrl: "http://127.0.0.1:9222",
+    });
+
+    await fs.rm(workspaceDir, { recursive: true, force: true });
+  });
+
   it.runIf(process.platform !== "win32")(
     "rejects screenshot artifact symlink escapes",
     async () => {

extensions/browser/src/attached-browser-tool-runtime.ts

@@ -11,6 +11,7 @@ import { createBrowserTool } from "./browser-tool.js";
 import type { AnyAgentTool } from "./browser-tool.runtime.js";
 import { startBrowserBridgeServer, stopBrowserBridgeServer } from "./browser/bridge-server.js";
 import { resolveBrowserConfig } from "./browser/config.js";
+import { closePlaywrightBrowserConnection } from "./browser/pw-session.js";
 import { writeExternalFileWithinRoot } from "./sdk-security-runtime.js";
 
 const ATTACHED_PROFILE_NAME = "worker";
@@ -113,6 +114,13 @@ export async function createAttachedBrowserToolRuntime(
     authToken: randomBytes(32).toString("base64url"),
     onEnsureAttachTarget: async () => await params.ensureAttachTarget(),
   });
+  const dispose = async () => {
+    try {
+      await stopBrowserBridgeServer(bridge.server);
+    } finally {
+      await closePlaywrightBrowserConnection({ cdpUrl });
+    }
+  };
   try {
     const tool = createBrowserTool({
       sandboxBridgeUrl: bridge.baseUrl,
@@ -132,10 +140,10 @@ export async function createAttachedBrowserToolRuntime(
     });
     return {
       tool,
-      dispose: async () => await stopBrowserBridgeServer(bridge.server),
+      dispose,
     };
   } catch (error) {
-    await stopBrowserBridgeServer(bridge.server);
+    await dispose();
     throw error;
   }
 }