diff --git a/extensions/browser/src/browser/server-context.availability.ts b/extensions/browser/src/browser/server-context.availability.ts index 94b4db2c3cc..2c1454fe875 100644 --- a/extensions/browser/src/browser/server-context.availability.ts +++ b/extensions/browser/src/browser/server-context.availability.ts @@ -26,6 +26,10 @@ import { CDP_READY_AFTER_LAUNCH_POLL_MS, CDP_READY_AFTER_LAUNCH_WINDOW_MS, } from "./server-context.constants.js"; +import { + closePlaywrightBrowserConnectionForProfile, + resolveIdleProfileStopOutcome, +} from "./server-context.lifecycle.js"; import type { BrowserServerState, ContextOptions, @@ -100,15 +104,6 @@ export function createProfileAvailability({ }); }; - const closePlaywrightBrowserConnectionForProfile = async (cdpUrl?: string): Promise => { - try { - const mod = await import("./pw-ai.js"); - await mod.closePlaywrightBrowserConnection(cdpUrl ? { cdpUrl } : undefined); - } catch { - // ignore - } - }; - const reconcileProfileRuntime = async (): Promise => { const profileState = getProfileState(); const reconcile = profileState.reconcile; @@ -277,15 +272,13 @@ export function createProfileAvailability({ } const profileState = getProfileState(); if (!profileState.running) { - const remoteCdp = capabilities.isRemote; - if (profile.attachOnly || remoteCdp) { - // No process was launched for attachOnly/remote profiles, but a Playwright - // CDP connection may still be active. Close it so emulation overrides - // (prefers-color-scheme, etc.) are released. + const idleStop = resolveIdleProfileStopOutcome(profile); + if (idleStop.closePlaywright) { + // No process was launched for attachOnly/remote profiles, but a cached + // Playwright CDP connection may still be active and holding emulation state. await closePlaywrightBrowserConnectionForProfile(profile.cdpUrl); - return { stopped: true }; } - return { stopped: false }; + return { stopped: idleStop.stopped }; } await stopOpenClawChrome(profileState.running); setProfileRunning(null); diff --git a/extensions/browser/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts b/extensions/browser/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts index e1476dd7de4..0b739b75f78 100644 --- a/extensions/browser/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts +++ b/extensions/browser/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts @@ -1,5 +1,3 @@ -import type { ChildProcessWithoutNullStreams } from "node:child_process"; -import { EventEmitter } from "node:events"; import { afterEach, describe, expect, it, vi } from "vitest"; import "./server-context.chrome-test-harness.js"; import { @@ -7,55 +5,8 @@ import { PROFILE_HTTP_REACHABILITY_TIMEOUT_MS, } from "./cdp-timeouts.js"; import * as chromeModule from "./chrome.js"; -import type { RunningChrome } from "./chrome.js"; -import type { BrowserServerState } from "./server-context.js"; import { createBrowserRouteContext } from "./server-context.js"; - -function makeBrowserState(): BrowserServerState { - return { - // oxlint-disable-next-line typescript/no-explicit-any - server: null as any, - port: 0, - resolved: { - enabled: true, - controlPort: 18791, - cdpProtocol: "http", - cdpHost: "127.0.0.1", - cdpIsLoopback: true, - cdpPortRangeStart: 18800, - cdpPortRangeEnd: 18810, - evaluateEnabled: false, - remoteCdpTimeoutMs: 1500, - remoteCdpHandshakeTimeoutMs: 3000, - extraArgs: [], - color: "#FF4500", - headless: true, - noSandbox: false, - attachOnly: false, - ssrfPolicy: { allowPrivateNetwork: true }, - defaultProfile: "openclaw", - profiles: { - openclaw: { cdpPort: 18800, color: "#FF4500" }, - }, - }, - profiles: new Map(), - }; -} - -function mockLaunchedChrome( - launchOpenClawChrome: { mockResolvedValue: (value: RunningChrome) => unknown }, - pid: number, -) { - const proc = new EventEmitter() as unknown as ChildProcessWithoutNullStreams; - launchOpenClawChrome.mockResolvedValue({ - pid, - exe: { kind: "chromium", path: "/usr/bin/chromium" }, - userDataDir: "/tmp/openclaw-test", - cdpPort: 18800, - startedAt: Date.now(), - proc, - }); -} +import { makeBrowserServerState, mockLaunchedChrome } from "./server-context.test-harness.js"; function setupEnsureBrowserAvailableHarness() { vi.useFakeTimers(); @@ -66,7 +17,7 @@ function setupEnsureBrowserAvailableHarness() { const isChromeCdpReady = vi.mocked(chromeModule.isChromeCdpReady); isChromeReachable.mockResolvedValue(false); - const state = makeBrowserState(); + const state = makeBrowserServerState(); const ctx = createBrowserRouteContext({ getState: () => state }); const profile = ctx.forProfile("openclaw"); diff --git a/extensions/browser/src/browser/server-context.lifecycle.test.ts b/extensions/browser/src/browser/server-context.lifecycle.test.ts new file mode 100644 index 00000000000..179bdb58704 --- /dev/null +++ b/extensions/browser/src/browser/server-context.lifecycle.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; +import { resolveIdleProfileStopOutcome } from "./server-context.lifecycle.js"; +import { makeBrowserProfile } from "./server-context.test-harness.js"; + +describe("resolveIdleProfileStopOutcome", () => { + it("treats attachOnly profiles as stopped via Playwright cleanup", () => { + expect(resolveIdleProfileStopOutcome(makeBrowserProfile({ attachOnly: true }))).toEqual({ + stopped: true, + closePlaywright: true, + }); + }); + + it("treats remote CDP profiles as stopped via Playwright cleanup", () => { + expect( + resolveIdleProfileStopOutcome( + makeBrowserProfile({ + cdpUrl: "http://10.0.0.5:9222", + cdpHost: "10.0.0.5", + cdpIsLoopback: false, + cdpPort: 9222, + }), + ), + ).toEqual({ + stopped: true, + closePlaywright: true, + }); + }); + + it("keeps never-started managed profiles as not stopped", () => { + expect(resolveIdleProfileStopOutcome(makeBrowserProfile())).toEqual({ + stopped: false, + closePlaywright: false, + }); + }); +}); diff --git a/extensions/browser/src/browser/server-context.lifecycle.ts b/extensions/browser/src/browser/server-context.lifecycle.ts new file mode 100644 index 00000000000..17af693d73f --- /dev/null +++ b/extensions/browser/src/browser/server-context.lifecycle.ts @@ -0,0 +1,28 @@ +import type { ResolvedBrowserProfile } from "./config.js"; +import { getBrowserProfileCapabilities } from "./profile-capabilities.js"; + +export function resolveIdleProfileStopOutcome(profile: ResolvedBrowserProfile): { + stopped: boolean; + closePlaywright: boolean; +} { + const capabilities = getBrowserProfileCapabilities(profile); + if (profile.attachOnly || capabilities.isRemote) { + return { + stopped: true, + closePlaywright: true, + }; + } + return { + stopped: false, + closePlaywright: false, + }; +} + +export async function closePlaywrightBrowserConnectionForProfile(cdpUrl?: string): Promise { + try { + const mod = await import("./pw-ai.js"); + await mod.closePlaywrightBrowserConnection(cdpUrl ? { cdpUrl } : undefined); + } catch { + // ignore + } +} diff --git a/extensions/browser/src/browser/server-context.stop-running-browser.test.ts b/extensions/browser/src/browser/server-context.stop-running-browser.test.ts index 9cb114e4ac4..cf313dc65f9 100644 --- a/extensions/browser/src/browser/server-context.stop-running-browser.test.ts +++ b/extensions/browser/src/browser/server-context.stop-running-browser.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { createProfileAvailability } from "./server-context.availability.js"; -import type { BrowserServerState, ProfileRuntimeState } from "./server-context.types.js"; +import { createBrowserRouteContext } from "./server-context.js"; +import { makeBrowserProfile, makeBrowserServerState } from "./server-context.test-harness.js"; const pwAiMocks = vi.hoisted(() => ({ closePlaywrightBrowserConnection: vi.fn(async () => {}), @@ -13,6 +13,7 @@ vi.mock("./chrome.js", () => ({ launchOpenClawChrome: vi.fn(async () => { throw new Error("unexpected launch"); }), + resolveOpenClawUserDataDir: vi.fn(() => "/tmp/openclaw-test"), stopOpenClawChrome: vi.fn(async () => {}), })); vi.mock("./chrome-mcp.js", () => ({ @@ -25,108 +26,48 @@ afterEach(() => { vi.clearAllMocks(); }); -function makeProfile( - overrides: Partial[0]["profile"]> = {}, -): Parameters[0]["profile"] { - return { - name: "openclaw", - cdpUrl: "http://127.0.0.1:18800", - cdpHost: "127.0.0.1", - cdpIsLoopback: true, - cdpPort: 18800, - color: "#f60", - driver: "openclaw", - attachOnly: false, - ...overrides, - }; -} - -function makeState( - profile: Parameters[0]["profile"], -): BrowserServerState { - return { - server: null, - port: 0, - resolved: { - enabled: true, - evaluateEnabled: false, - controlPort: 18791, - cdpProtocol: "http", - cdpHost: profile.cdpHost, - cdpIsLoopback: profile.cdpIsLoopback, - cdpPortRangeStart: 18800, - cdpPortRangeEnd: 18810, - remoteCdpTimeoutMs: 1500, - remoteCdpHandshakeTimeoutMs: 3000, - extraArgs: [], - color: profile.color, - headless: true, - noSandbox: false, - attachOnly: false, +function createStopHarness(profile: ReturnType) { + const state = makeBrowserServerState({ + profile, + resolvedOverrides: { ssrfPolicy: { dangerouslyAllowPrivateNetwork: true }, - defaultProfile: profile.name, - profiles: { - [profile.name]: profile, - }, - }, - profiles: new Map(), - }; -} - -function createStopHarness(profile: Parameters[0]["profile"]) { - const state = makeState(profile); - const runtimeState: ProfileRuntimeState = { - profile, - running: null, - lastTargetId: null, - reconcile: null, - }; - state.profiles.set(profile.name, runtimeState); - - const ops = createProfileAvailability({ - opts: { getState: () => state }, - profile, - state: () => state, - getProfileState: () => runtimeState, - setProfileRunning: (running) => { - runtimeState.running = running; }, }); - - return { ops }; + const ctx = createBrowserRouteContext({ getState: () => state }); + return { profileCtx: ctx.forProfile(profile.name) }; } describe("createProfileAvailability.stopRunningBrowser", () => { it("disconnects attachOnly loopback profiles without an owned process", async () => { - const profile = makeProfile({ attachOnly: true }); - const { ops } = createStopHarness(profile); + const profile = makeBrowserProfile({ attachOnly: true }); + const { profileCtx } = createStopHarness(profile); - await expect(ops.stopRunningBrowser()).resolves.toEqual({ stopped: true }); + await expect(profileCtx.stopRunningBrowser()).resolves.toEqual({ stopped: true }); expect(pwAiMocks.closePlaywrightBrowserConnection).toHaveBeenCalledWith({ cdpUrl: "http://127.0.0.1:18800", }); }); it("disconnects remote CDP profiles without an owned process", async () => { - const profile = makeProfile({ + const profile = makeBrowserProfile({ cdpUrl: "http://10.0.0.5:9222", cdpHost: "10.0.0.5", cdpIsLoopback: false, cdpPort: 9222, }); - const { ops } = createStopHarness(profile); + const { profileCtx } = createStopHarness(profile); - await expect(ops.stopRunningBrowser()).resolves.toEqual({ stopped: true }); + await expect(profileCtx.stopRunningBrowser()).resolves.toEqual({ stopped: true }); expect(pwAiMocks.closePlaywrightBrowserConnection).toHaveBeenCalledWith({ cdpUrl: "http://10.0.0.5:9222", }); }); it("keeps never-started local managed profiles as not stopped", async () => { - const profile = makeProfile(); - const { ops } = createStopHarness(profile); + const profile = makeBrowserProfile(); + const { profileCtx } = createStopHarness(profile); - await expect(ops.stopRunningBrowser()).resolves.toEqual({ stopped: false }); + await expect(profileCtx.stopRunningBrowser()).resolves.toEqual({ stopped: false }); expect(pwAiMocks.closePlaywrightBrowserConnection).not.toHaveBeenCalled(); }); }); diff --git a/extensions/browser/src/browser/server-context.test-harness.ts b/extensions/browser/src/browser/server-context.test-harness.ts new file mode 100644 index 00000000000..75d36b95e7d --- /dev/null +++ b/extensions/browser/src/browser/server-context.test-harness.ts @@ -0,0 +1,72 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { EventEmitter } from "node:events"; +import type { RunningChrome } from "./chrome.js"; +import type { ResolvedBrowserProfile } from "./config.js"; +import type { BrowserServerState } from "./server-context.js"; + +export function makeBrowserProfile( + overrides: Partial = {}, +): ResolvedBrowserProfile { + return { + name: "openclaw", + cdpUrl: "http://127.0.0.1:18800", + cdpHost: "127.0.0.1", + cdpIsLoopback: true, + cdpPort: 18800, + color: "#FF4500", + driver: "openclaw", + attachOnly: false, + ...overrides, + }; +} + +export function makeBrowserServerState(params?: { + profile?: ResolvedBrowserProfile; + resolvedOverrides?: Partial; +}): BrowserServerState { + const profile = params?.profile ?? makeBrowserProfile(); + return { + // oxlint-disable-next-line typescript/no-explicit-any + server: null as any, + port: 0, + resolved: { + enabled: true, + controlPort: 18791, + cdpProtocol: "http", + cdpHost: profile.cdpHost, + cdpIsLoopback: profile.cdpIsLoopback, + cdpPortRangeStart: 18800, + cdpPortRangeEnd: 18810, + evaluateEnabled: false, + remoteCdpTimeoutMs: 1500, + remoteCdpHandshakeTimeoutMs: 3000, + extraArgs: [], + color: profile.color, + headless: true, + noSandbox: false, + attachOnly: false, + ssrfPolicy: { allowPrivateNetwork: true }, + defaultProfile: profile.name, + profiles: { + [profile.name]: profile, + }, + ...params?.resolvedOverrides, + }, + profiles: new Map(), + }; +} + +export function mockLaunchedChrome( + launchOpenClawChrome: { mockResolvedValue: (value: RunningChrome) => unknown }, + pid: number, +) { + const proc = new EventEmitter() as unknown as ChildProcessWithoutNullStreams; + launchOpenClawChrome.mockResolvedValue({ + pid, + exe: { kind: "chromium", path: "/usr/bin/chromium" }, + userDataDir: "/tmp/openclaw-test", + cdpPort: 18800, + startedAt: Date.now(), + proc, + }); +} diff --git a/scripts/changelog-add-unreleased.ts b/scripts/changelog-add-unreleased.ts new file mode 100644 index 00000000000..d1d51760952 --- /dev/null +++ b/scripts/changelog-add-unreleased.ts @@ -0,0 +1,65 @@ +import { readFileSync, writeFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { appendUnreleasedChangelogEntry } from "../src/infra/changelog-unreleased.js"; + +type SectionArg = "breaking" | "changes" | "fixes"; + +function parseArgs(argv: string[]): { + changelogPath: string; + section: "Breaking" | "Changes" | "Fixes"; + entry: string; +} { + let changelogPath = resolve("CHANGELOG.md"); + let section: SectionArg | undefined; + const entryParts: string[] = []; + + for (let index = 0; index < argv.length; index += 1) { + const arg = argv[index]; + if (arg === "--file") { + const next = argv[index + 1]; + if (!next) { + throw new Error("Missing value for --file."); + } + changelogPath = resolve(next); + index += 1; + continue; + } + if (arg === "--section") { + const next = argv[index + 1] as SectionArg | undefined; + if (!next || !["breaking", "changes", "fixes"].includes(next)) { + throw new Error("Missing or invalid value for --section."); + } + section = next; + index += 1; + continue; + } + entryParts.push(arg); + } + + if (!section) { + throw new Error("Missing required --section ."); + } + const entry = entryParts.join(" ").trim(); + if (!entry) { + throw new Error("Missing changelog entry text."); + } + + return { + changelogPath, + section: section === "breaking" ? "Breaking" : section === "changes" ? "Changes" : "Fixes", + entry, + }; +} + +if (import.meta.main) { + const { changelogPath, section, entry } = parseArgs(process.argv.slice(2)); + const content = readFileSync(changelogPath, "utf8"); + const next = appendUnreleasedChangelogEntry(content, { + section, + entry, + }); + if (next !== content) { + writeFileSync(changelogPath, next); + } + console.log(`Updated ${changelogPath} (${section}).`); +} diff --git a/src/agents/subagent-registry-lifecycle.test.ts b/src/agents/subagent-registry-lifecycle.test.ts index 832f2400e27..d847ea9c672 100644 --- a/src/agents/subagent-registry-lifecycle.test.ts +++ b/src/agents/subagent-registry-lifecycle.test.ts @@ -17,8 +17,8 @@ const lifecycleEventMocks = vi.hoisted(() => ({ emitSessionLifecycleEvent: vi.fn(), })); -const browserMaintenanceMocks = vi.hoisted(() => ({ - closeTrackedBrowserTabsForSessions: vi.fn(async () => 0), +const browserLifecycleCleanupMocks = vi.hoisted(() => ({ + cleanupBrowserSessionsForLifecycleEnd: vi.fn(async () => {}), })); vi.mock("../tasks/task-executor.js", () => ({ @@ -31,8 +31,9 @@ vi.mock("../sessions/session-lifecycle-events.js", () => ({ emitSessionLifecycleEvent: lifecycleEventMocks.emitSessionLifecycleEvent, })); -vi.mock("../plugin-sdk/browser-maintenance.js", () => ({ - closeTrackedBrowserTabsForSessions: browserMaintenanceMocks.closeTrackedBrowserTabsForSessions, +vi.mock("../browser-lifecycle-cleanup.js", () => ({ + cleanupBrowserSessionsForLifecycleEnd: + browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd, })); vi.mock("./subagent-registry-helpers.js", async () => { @@ -66,7 +67,7 @@ describe("subagent registry lifecycle hardening", () => { beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); - browserMaintenanceMocks.closeTrackedBrowserTabsForSessions.mockClear(); + browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd.mockClear(); mod = await import("./subagent-registry-lifecycle.js"); }); @@ -208,10 +209,12 @@ describe("subagent registry lifecycle hardening", () => { }), ).resolves.toBeUndefined(); - expect(browserMaintenanceMocks.closeTrackedBrowserTabsForSessions).toHaveBeenCalledWith({ - sessionKeys: [entry.childSessionKey], - onWarn: expect.any(Function), - }); + expect(browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd).toHaveBeenCalledWith( + { + sessionKeys: [entry.childSessionKey], + onWarn: expect.any(Function), + }, + ); expect(runSubagentAnnounceFlow).toHaveBeenCalledWith( expect.objectContaining({ childSessionKey: entry.childSessionKey, @@ -252,7 +255,9 @@ describe("subagent registry lifecycle hardening", () => { }), ).resolves.toBeUndefined(); - expect(browserMaintenanceMocks.closeTrackedBrowserTabsForSessions).not.toHaveBeenCalled(); + expect( + browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd, + ).not.toHaveBeenCalled(); expect(runSubagentAnnounceFlow).not.toHaveBeenCalled(); }); }); diff --git a/src/agents/subagent-registry-lifecycle.ts b/src/agents/subagent-registry-lifecycle.ts index 3360cfd2ac5..b837c1dd2d4 100644 --- a/src/agents/subagent-registry-lifecycle.ts +++ b/src/agents/subagent-registry-lifecycle.ts @@ -1,6 +1,6 @@ import { isSilentReplyText, SILENT_REPLY_TOKEN } from "../auto-reply/tokens.js"; +import { cleanupBrowserSessionsForLifecycleEnd } from "../browser-lifecycle-cleanup.js"; import { formatErrorMessage, readErrorName } from "../infra/errors.js"; -import { closeTrackedBrowserTabsForSessions } from "../plugin-sdk/browser-maintenance.js"; import { defaultRuntime } from "../runtime.js"; import { emitSessionLifecycleEvent } from "../sessions/session-lifecycle-events.js"; import { @@ -606,17 +606,10 @@ export function createSubagentRegistryLifecycleController(params: { return; } - // Clean up browser tabs/processes opened by this subagent session. - // Without this, browser processes become orphaned after the subagent - // exits. See #60104. - try { - await closeTrackedBrowserTabsForSessions({ - sessionKeys: [entry.childSessionKey], - onWarn: (msg) => params.warn(msg, { runId: entry.runId }), - }); - } catch { - // Best-effort cleanup. - } + await cleanupBrowserSessionsForLifecycleEnd({ + sessionKeys: [entry.childSessionKey], + onWarn: (msg) => params.warn(msg, { runId: entry.runId }), + }); startSubagentAnnounceCleanupFlow(completeParams.runId, entry); }; diff --git a/src/browser-lifecycle-cleanup.test.ts b/src/browser-lifecycle-cleanup.test.ts new file mode 100644 index 00000000000..a56b1d2352b --- /dev/null +++ b/src/browser-lifecycle-cleanup.test.ts @@ -0,0 +1,48 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const closeTrackedBrowserTabsForSessions = vi.hoisted(() => vi.fn(async () => 0)); + +vi.mock("./plugin-sdk/browser-maintenance.js", () => ({ + closeTrackedBrowserTabsForSessions, +})); + +describe("cleanupBrowserSessionsForLifecycleEnd", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("normalizes session keys before closing browser sessions", async () => { + const { cleanupBrowserSessionsForLifecycleEnd } = + await import("./browser-lifecycle-cleanup.js"); + const onWarn = vi.fn(); + + await expect( + cleanupBrowserSessionsForLifecycleEnd({ + sessionKeys: ["", " session-a ", "session-a", "session-b"], + onWarn, + }), + ).resolves.toBeUndefined(); + + expect(closeTrackedBrowserTabsForSessions).toHaveBeenCalledWith({ + sessionKeys: ["session-a", "session-b"], + onWarn, + }); + }); + + it("swallows browser cleanup failures", async () => { + const { cleanupBrowserSessionsForLifecycleEnd } = + await import("./browser-lifecycle-cleanup.js"); + const onError = vi.fn(); + const error = new Error("cleanup failed"); + closeTrackedBrowserTabsForSessions.mockRejectedValueOnce(error); + + await expect( + cleanupBrowserSessionsForLifecycleEnd({ + sessionKeys: ["session-a"], + onError, + }), + ).resolves.toBeUndefined(); + + expect(onError).toHaveBeenCalledWith(error); + }); +}); diff --git a/src/browser-lifecycle-cleanup.ts b/src/browser-lifecycle-cleanup.ts new file mode 100644 index 00000000000..bc934318c40 --- /dev/null +++ b/src/browser-lifecycle-cleanup.ts @@ -0,0 +1,33 @@ +import { runBestEffortCleanup } from "./infra/non-fatal-cleanup.js"; +import { closeTrackedBrowserTabsForSessions } from "./plugin-sdk/browser-maintenance.js"; + +function normalizeSessionKeys(sessionKeys: string[]): string[] { + const keys = new Set(); + for (const sessionKey of sessionKeys) { + const normalized = sessionKey.trim(); + if (normalized) { + keys.add(normalized); + } + } + return [...keys]; +} + +export async function cleanupBrowserSessionsForLifecycleEnd(params: { + sessionKeys: string[]; + onWarn?: (message: string) => void; + onError?: (error: unknown) => void; +}): Promise { + const sessionKeys = normalizeSessionKeys(params.sessionKeys); + if (sessionKeys.length === 0) { + return; + } + await runBestEffortCleanup({ + cleanup: async () => { + await closeTrackedBrowserTabsForSessions({ + sessionKeys, + onWarn: params.onWarn, + }); + }, + onError: params.onError, + }); +} diff --git a/src/gateway/server-cron.test.ts b/src/gateway/server-cron.test.ts index 091117f97bc..e612bac1bf0 100644 --- a/src/gateway/server-cron.test.ts +++ b/src/gateway/server-cron.test.ts @@ -12,14 +12,14 @@ const { loadConfigMock, fetchWithSsrFGuardMock, runCronIsolatedAgentTurnMock, - closeTrackedBrowserTabsForSessionsMock, + cleanupBrowserSessionsForLifecycleEndMock, } = vi.hoisted(() => ({ enqueueSystemEventMock: vi.fn(), requestHeartbeatNowMock: vi.fn(), loadConfigMock: vi.fn(), fetchWithSsrFGuardMock: vi.fn(), runCronIsolatedAgentTurnMock: vi.fn(async () => ({ status: "ok" as const, summary: "ok" })), - closeTrackedBrowserTabsForSessionsMock: vi.fn(async () => 0), + cleanupBrowserSessionsForLifecycleEndMock: vi.fn(async () => {}), })); function enqueueSystemEvent(...args: unknown[]) { @@ -61,8 +61,8 @@ vi.mock("../cron/isolated-agent.js", () => ({ runCronIsolatedAgentTurn: runCronIsolatedAgentTurnMock, })); -vi.mock("../plugin-sdk/browser-maintenance.js", () => ({ - closeTrackedBrowserTabsForSessions: closeTrackedBrowserTabsForSessionsMock, +vi.mock("../browser-lifecycle-cleanup.js", () => ({ + cleanupBrowserSessionsForLifecycleEnd: cleanupBrowserSessionsForLifecycleEndMock, })); import { buildGatewayCronService } from "./server-cron.js"; @@ -86,7 +86,7 @@ describe("buildGatewayCronService", () => { loadConfigMock.mockClear(); fetchWithSsrFGuardMock.mockClear(); runCronIsolatedAgentTurnMock.mockClear(); - closeTrackedBrowserTabsForSessionsMock.mockClear(); + cleanupBrowserSessionsForLifecycleEndMock.mockClear(); }); it("routes main-target jobs to the scoped session for enqueue + wake", async () => { @@ -207,7 +207,7 @@ describe("buildGatewayCronService", () => { sessionKey: "project-alpha-monitor", }), ); - expect(closeTrackedBrowserTabsForSessionsMock).toHaveBeenCalledWith({ + expect(cleanupBrowserSessionsForLifecycleEndMock).toHaveBeenCalledWith({ sessionKeys: ["project-alpha-monitor"], onWarn: expect.any(Function), }); diff --git a/src/gateway/server-cron.ts b/src/gateway/server-cron.ts index 20e174a0ee9..43be88cc7b7 100644 --- a/src/gateway/server-cron.ts +++ b/src/gateway/server-cron.ts @@ -1,4 +1,5 @@ import { resolveDefaultAgentId } from "../agents/agent-scope.js"; +import { cleanupBrowserSessionsForLifecycleEnd } from "../browser-lifecycle-cleanup.js"; import type { CliDeps } from "../cli/deps.js"; import { createOutboundSendDeps } from "../cli/outbound-send-deps.js"; import { loadConfig } from "../config/config.js"; @@ -11,7 +12,6 @@ import { resolveStorePath } from "../config/sessions/paths.js"; import { resolveFailureDestination, sendFailureNotificationAnnounce } from "../cron/delivery.js"; import { runCronIsolatedAgentTurn } from "../cron/isolated-agent.js"; import { resolveDeliveryTarget } from "../cron/isolated-agent/delivery-target.js"; -import { closeTrackedBrowserTabsForSessions } from "../plugin-sdk/browser-maintenance.js"; import { appendCronRunLog, resolveCronRunLogPath, @@ -302,18 +302,10 @@ export function buildGatewayCronService(params: { lane: "cron", }); } finally { - // Clean up browser tabs/processes opened during this cron run. - // Without this, browser processes become orphaned (PPID=1) after - // the cron task completes. See #60104. - try { - await closeTrackedBrowserTabsForSessions({ - sessionKeys: [sessionKey], - onWarn: (msg) => cronLogger.warn({ jobId: job.id }, msg), - }); - } catch { - // Best-effort cleanup — do not let browser cleanup failures - // mask the actual cron run result. - } + await cleanupBrowserSessionsForLifecycleEnd({ + sessionKeys: [sessionKey], + onWarn: (msg) => cronLogger.warn({ jobId: job.id }, msg), + }); } }, sendCronFailureAlert: async ({ job, text, channel, to, mode, accountId }) => { diff --git a/src/infra/changelog-unreleased.test.ts b/src/infra/changelog-unreleased.test.ts new file mode 100644 index 00000000000..150b2d94970 --- /dev/null +++ b/src/infra/changelog-unreleased.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { appendUnreleasedChangelogEntry } from "./changelog-unreleased.js"; + +const baseChangelog = `# Changelog + +## Unreleased + +### Breaking + +- Existing breaking entry. + +### Changes + +- Existing change. + +### Fixes + +- Existing fix. + +## 2026.4.4 +`; + +describe("appendUnreleasedChangelogEntry", () => { + it("appends to the end of the requested unreleased section", () => { + const next = appendUnreleasedChangelogEntry(baseChangelog, { + section: "Fixes", + entry: "New fix entry.", + }); + + expect(next).toContain(`### Fixes + +- Existing fix. +- New fix entry. + +## 2026.4.4`); + }); + + it("avoids duplicating an existing entry", () => { + const next = appendUnreleasedChangelogEntry(baseChangelog, { + section: "Changes", + entry: "- Existing change.", + }); + + expect(next).toBe(baseChangelog); + }); + + it("throws when the unreleased section is missing", () => { + expect(() => + appendUnreleasedChangelogEntry("# Changelog\n", { + section: "Fixes", + entry: "New fix entry.", + }), + ).toThrow("## Unreleased"); + }); +}); diff --git a/src/infra/changelog-unreleased.ts b/src/infra/changelog-unreleased.ts new file mode 100644 index 00000000000..ae30bd900ef --- /dev/null +++ b/src/infra/changelog-unreleased.ts @@ -0,0 +1,68 @@ +type UnreleasedSection = "Breaking" | "Changes" | "Fixes"; + +function findSectionRange( + lines: string[], + section: UnreleasedSection, +): { + start: number; + insertAt: number; +} { + const unreleasedIndex = lines.findIndex((line) => line.trim() === "## Unreleased"); + if (unreleasedIndex === -1) { + throw new Error("CHANGELOG.md is missing the '## Unreleased' heading."); + } + + const sectionHeading = `### ${section}`; + let sectionIndex = -1; + for (let index = unreleasedIndex + 1; index < lines.length; index += 1) { + const line = lines[index]; + if (line.startsWith("## ")) { + break; + } + if (line.trim() === sectionHeading) { + sectionIndex = index; + break; + } + } + if (sectionIndex === -1) { + throw new Error(`CHANGELOG.md is missing the '${sectionHeading}' section under Unreleased.`); + } + + let insertAt = lines.length; + for (let index = sectionIndex + 1; index < lines.length; index += 1) { + const line = lines[index]; + if (line.startsWith("### ") || line.startsWith("## ")) { + insertAt = index; + break; + } + } + + while (insertAt > sectionIndex + 1 && lines[insertAt - 1]?.trim() === "") { + insertAt -= 1; + } + + return { start: sectionIndex, insertAt }; +} + +export function appendUnreleasedChangelogEntry( + content: string, + params: { + section: UnreleasedSection; + entry: string; + }, +): string { + const entry = params.entry.trim(); + if (!entry) { + throw new Error("Changelog entry must not be empty."); + } + + const lines = content.split("\n"); + const bullet = entry.startsWith("- ") ? entry : `- ${entry}`; + if (lines.some((line) => line.trim() === bullet)) { + return content; + } + + const { insertAt } = findSectionRange(lines, params.section); + lines.splice(insertAt, 0, bullet, ""); + return lines.join("\n"); +} diff --git a/src/infra/non-fatal-cleanup.test.ts b/src/infra/non-fatal-cleanup.test.ts new file mode 100644 index 00000000000..101bb2a7d3c --- /dev/null +++ b/src/infra/non-fatal-cleanup.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it, vi } from "vitest"; +import { runBestEffortCleanup } from "./non-fatal-cleanup.js"; + +describe("runBestEffortCleanup", () => { + it("returns the cleanup result when the cleanup succeeds", async () => { + await expect( + runBestEffortCleanup({ + cleanup: async () => 7, + }), + ).resolves.toBe(7); + }); + + it("swallows cleanup failures and reports them through onError", async () => { + const onError = vi.fn(); + const error = new Error("cleanup failed"); + + await expect( + runBestEffortCleanup({ + cleanup: async () => { + throw error; + }, + onError, + }), + ).resolves.toBeUndefined(); + + expect(onError).toHaveBeenCalledWith(error); + }); +}); diff --git a/src/infra/non-fatal-cleanup.ts b/src/infra/non-fatal-cleanup.ts new file mode 100644 index 00000000000..712198ebd33 --- /dev/null +++ b/src/infra/non-fatal-cleanup.ts @@ -0,0 +1,11 @@ +export async function runBestEffortCleanup(params: { + cleanup: () => Promise; + onError?: (error: unknown) => void; +}): Promise { + try { + return await params.cleanup(); + } catch (error) { + params.onError?.(error); + return undefined; + } +}