From e43de9b77b0686890487bc4c9e1f970f48b263ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Sun, 19 Jul 2026 12:29:37 -0300 Subject: [PATCH 1/2] test(session): characterize typed reaper teardown --- packages/android/test/reaper.test.ts | 8 ++++++++ packages/browser/test/session.test.ts | 7 +++++++ packages/desktop-linux/test/destroy.test.ts | 21 ++++++++++++--------- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/android/test/reaper.test.ts b/packages/android/test/reaper.test.ts index 7ec2638..5d6860e 100644 --- a/packages/android/test/reaper.test.ts +++ b/packages/android/test/reaper.test.ts @@ -21,6 +21,7 @@ vi.mock("../src/emulator.js", async (importOriginal) => { import { REAPER_CLEANUP_PENDING_META_KEY, + beginEvidenceRun, getSession, isPidAlive, reapDeadRunningSessions, @@ -80,6 +81,8 @@ describe("android reaper tracking", () => { bootTimeoutMs: 5_000, }); + const { run } = await beginEvidenceRun(projectDir, session.id); + forceStopFailure = true; try { await expect( @@ -103,5 +106,10 @@ describe("android reaper tracking", () => { expect(reaped.map((record) => record.id)).toContain(session.id); expect(await getSession(session.id, registryEnv)).toBeUndefined(); expect(isPidAlive(session.emulatorPid)).toBe(false); + expect( + JSON.parse( + await fs.promises.readFile(path.join(run.dir, "manifest.json"), "utf8"), + ), + ).toMatchObject({ status: "failed" }); }, 20_000); }); diff --git a/packages/browser/test/session.test.ts b/packages/browser/test/session.test.ts index fb87bc9..022cfee 100644 --- a/packages/browser/test/session.test.ts +++ b/packages/browser/test/session.test.ts @@ -1066,6 +1066,13 @@ describe("browser record inspection (no live processes)", () => { ); // The confinement guard must not have deleted the out-of-tree directory. expect(fs.existsSync(outside)).toBe(true); + expect((await getSession(rec.id, registryEnv))?.meta?.reaperCleanupPending).toBe( + true, + ); + + expect(await reapDeadRunningSessions(registryEnv)).toEqual([]); + expect(await getSession(rec.id, registryEnv)).toBeDefined(); + expect(fs.existsSync(outside)).toBe(true); }); it("refuses a symlinked profile and never removes its outside target", async () => { diff --git a/packages/desktop-linux/test/destroy.test.ts b/packages/desktop-linux/test/destroy.test.ts index 65c9ec8..2f6bb88 100644 --- a/packages/desktop-linux/test/destroy.test.ts +++ b/packages/desktop-linux/test/destroy.test.ts @@ -5,6 +5,7 @@ import { afterAll, describe, expect, it, vi } from "vitest"; const FAILING_PID = 424_242; const STUCK_PID = 535_353; +let allowFailingPidStop = false; vi.mock("@pickforge/picklab-core", async (importOriginal) => { const actual = @@ -13,7 +14,7 @@ vi.mock("@pickforge/picklab-core", async (importOriginal) => { ...actual, processIdentityMatches: vi.fn(() => true), stopPid: vi.fn(async (pid: number) => { - if (pid === FAILING_PID) { + if (pid === FAILING_PID && !allowFailingPidStop) { throw new Error(`kill EPERM (pid ${pid})`); } if (pid === STUCK_PID) { @@ -23,7 +24,7 @@ vi.mock("@pickforge/picklab-core", async (importOriginal) => { }), stopProcessGroupVerified: vi.fn( async ({ pid }: { pid: number; startTicks: number }) => { - if (pid === FAILING_PID) { + if (pid === FAILING_PID && !allowFailingPidStop) { throw new Error(`kill EPERM (pid ${pid})`); } return { @@ -39,6 +40,7 @@ import { REAPER_CLEANUP_PENDING_META_KEY, createSession, getSession, + reapDeadRunningSessions, stopPid, stopProcessGroupVerified, updateSession, @@ -101,13 +103,14 @@ describe("destroyDesktopSession exception safety", () => { xvfbPid: FAILING_PID, vncPid: FAILING_PID, }); - vi.mocked(stopPid).mockResolvedValueOnce(true); - vi.mocked(stopProcessGroupVerified).mockResolvedValueOnce({ - outcome: "terminated", - signaled: true, - }); - await destroyDesktopSession(id, registryEnv); - expect(await getSession(id, registryEnv)).toBeUndefined(); + allowFailingPidStop = true; + try { + const reaped = await reapDeadRunningSessions(registryEnv); + expect(reaped.map((record) => record.id)).toEqual([id]); + expect(await getSession(id, registryEnv)).toBeUndefined(); + } finally { + allowFailingPidStop = false; + } }); it("still stops xvfb when stopping vnc throws", async () => { From 21a1641cac36d3a3482d0dc1b63fc55e20c328b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Sun, 19 Jul 2026 13:17:52 -0300 Subject: [PATCH 2/2] refactor(session): route reaper through typed teardown --- docs/releases/UNRELEASED.md | 7 + packages/android/src/emulator.ts | 3 + packages/android/src/index.ts | 1 + packages/android/src/session.ts | 32 ++++- packages/android/test/emulator.test.ts | 21 +++ packages/android/test/reaper.test.ts | 18 ++- packages/browser/src/index.ts | 1 + packages/browser/src/session.ts | 24 +++- packages/browser/test/session.test.ts | 18 ++- packages/cli/src/commands/session.ts | 21 +++ packages/core/src/index.ts | 5 +- packages/core/src/session-lifecycle.ts | 113 ++++++++++++++- packages/core/src/session.ts | 143 +------------------ packages/core/test/session-lifecycle.test.ts | 30 ++++ packages/core/test/session.test.ts | 32 ++++- packages/desktop-linux/src/index.ts | 1 + packages/desktop-linux/src/session.ts | 24 +++- packages/desktop-linux/test/destroy.test.ts | 12 +- packages/mcp-server/src/tools/session.ts | 23 +++ 19 files changed, 361 insertions(+), 168 deletions(-) diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 6520bff..0678d90 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -48,6 +48,11 @@ then reset this file. ## Internal/release changes +- Routed dead-session reaping through the desktop, Android, and browser typed + teardown owners. Core now owns only registry/liveness/retry/evidence policy, + eliminating its duplicate PID and browser-profile stop implementation. CLI + and MCP creation compose all owners; direct package creation reaps its own + runtime type. - Stabilized the browser Xvfb cancellation regression test by waiting for the fake process's complete PID/display marker instead of treating file creation as publication. This removes the full-suite/coverage race that could abandon @@ -260,6 +265,8 @@ then reset this file. browser + desktop-linux suite iterations pass with a deliberately split fake startup-marker publication; the browser session suite (25 tests) and `bun run typecheck` pass. +- Typed reaper teardown: core plus desktop-linux, Android, and browser suites; + CLI/MCP session tests; `bun run typecheck`; and `bun run build` pass. - Evidence storage foundation: `bun run typecheck`, `bun run build`, and the full suite pass (73 files, 895 passed / 2 skipped). New core coverage diff --git a/packages/android/src/emulator.ts b/packages/android/src/emulator.ts index e723b2e..6573cdc 100644 --- a/packages/android/src/emulator.ts +++ b/packages/android/src/emulator.ts @@ -377,6 +377,9 @@ async function stopEmulatorProcess( opts: StopEmulatorOptions, ): Promise { const timeoutMs = opts.timeoutMs ?? EMU_KILL_TIMEOUT_MS; + if (opts.pid !== undefined && !isPidAlive(opts.pid)) { + return true; + } let adbPath: string | null = null; try { adbPath = resolveAdb(opts); diff --git a/packages/android/src/index.ts b/packages/android/src/index.ts index 433fe38..94a8153 100644 --- a/packages/android/src/index.ts +++ b/packages/android/src/index.ts @@ -100,6 +100,7 @@ export { createAndroidSession, destroyAndroidSession, getAndroidSessionStatus, + teardownAndroidSession, type AndroidSessionHandle, type AndroidSessionOpOptions, type AndroidSessionStatus, diff --git a/packages/android/src/session.ts b/packages/android/src/session.ts index d3ad01c..a1bc3d9 100644 --- a/packages/android/src/session.ts +++ b/packages/android/src/session.ts @@ -10,6 +10,7 @@ import { updateSession, type AndroidSessionInfo, type EnvLike, + type LocalSessionTeardownFinalizer, type SessionRecord, } from "@pickforge/picklab-core"; import { listDevices } from "./adb.js"; @@ -64,7 +65,17 @@ export async function createAndroidSession( ): Promise { const registryEnv = opts.registryEnv ?? process.env; const avdName = opts.avdName ?? DEFAULT_AVD_NAME; - await reapDeadRunningSessions(registryEnv); + await reapDeadRunningSessions(registryEnv, { + android: { + teardown: (id, finalize) => + teardownAndroidSession( + id, + registryEnv, + { sdk: opts.sdk, env: opts.env }, + finalize, + ), + }, + }); const record = await createSession( { type: "android", projectDir: opts.projectDir, android: { avdName } }, registryEnv, @@ -144,10 +155,11 @@ export async function createAndroidSession( } } -export async function destroyAndroidSession( +export async function teardownAndroidSession( id: string, - registryEnv: EnvLike = process.env, - opts: AndroidSessionOpOptions = {}, + registryEnv: EnvLike, + opts: AndroidSessionOpOptions, + finalize: LocalSessionTeardownFinalizer, ): Promise { const record = await getSession(id, registryEnv); if (record === undefined) { @@ -189,7 +201,17 @@ export async function destroyAndroidSession( ); } } - await destroySessionRecord(id, registryEnv); + await finalize(); +} + +export async function destroyAndroidSession( + id: string, + registryEnv: EnvLike = process.env, + opts: AndroidSessionOpOptions = {}, +): Promise { + await teardownAndroidSession(id, registryEnv, opts, () => + destroySessionRecord(id, registryEnv), + ); } export async function getAndroidSessionStatus( diff --git a/packages/android/test/emulator.test.ts b/packages/android/test/emulator.test.ts index 70183b4..7abdf92 100644 --- a/packages/android/test/emulator.test.ts +++ b/packages/android/test/emulator.test.ts @@ -251,6 +251,27 @@ describe("sdk auto-detection in the execution layer", () => { }); describe("stopEmulator confirmation", () => { + it("does not send adb kill when the recorded pid is already dead", async () => { + const marker = path.join(tmpRoot, `adb-kill-${sdkCounter + 1}.txt`); + const sdk = makeFakeSdk(`printf '%s\\n' "$*" >> ${JSON.stringify(marker)}`); + const registryEnv = makeRegistryEnv(); + const pid = await deadPid(); + expect(tryReserveConsolePort(5568, registryEnv, pid)).toBe(true); + + expect( + await stopEmulator({ + serial: "emulator-5568", + pid, + sdk, + env: { PATH: "" }, + registryEnv, + timeoutMs: 300, + }), + ).toBe(true); + expect(fs.existsSync(marker)).toBe(false); + expect(fs.existsSync(consolePortLockPath(5568, registryEnv))).toBe(false); + }); + it("returns false when adb devices cannot confirm the shutdown", async () => { const sdk = makeFakeSdk( [ diff --git a/packages/android/test/reaper.test.ts b/packages/android/test/reaper.test.ts index 5d6860e..30c43ae 100644 --- a/packages/android/test/reaper.test.ts +++ b/packages/android/test/reaper.test.ts @@ -27,7 +27,11 @@ import { reapDeadRunningSessions, type EnvLike, } from "@pickforge/picklab-core"; -import { createAndroidSession, destroyAndroidSession } from "../src/index.js"; +import { + createAndroidSession, + destroyAndroidSession, + teardownAndroidSession, +} from "../src/index.js"; const tmpRoot = fs.mkdtempSync( path.join(os.tmpdir(), "picklab-android-reaper-"), @@ -102,7 +106,17 @@ describe("android reaper tracking", () => { forceStopFailure = false; } - const reaped = await reapDeadRunningSessions(registryEnv); + const reaped = await reapDeadRunningSessions(registryEnv, { + android: { + teardown: (id, finalize) => + teardownAndroidSession( + id, + registryEnv, + { sdk, env: { PATH: "" }, timeoutMs: 300 }, + finalize, + ), + }, + }); expect(reaped.map((record) => record.id)).toContain(session.id); expect(await getSession(session.id, registryEnv)).toBeUndefined(); expect(isPidAlive(session.emulatorPid)).toBe(false); diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 3298b6a..4680ce6 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -35,6 +35,7 @@ export { createBrowserSession, destroyBrowserSession, getBrowserSessionStatus, + teardownBrowserSession, type BrowserSessionHandle, type BrowserSessionStatus, type CreateBrowserSessionOptions, diff --git a/packages/browser/src/session.ts b/packages/browser/src/session.ts index 716cf65..f6139d6 100644 --- a/packages/browser/src/session.ts +++ b/packages/browser/src/session.ts @@ -18,6 +18,7 @@ import { type BrowserSessionInfo, type DesktopSessionInfo, type EnvLike, + type LocalSessionTeardownFinalizer, type OwnedDaemonHandle, type ProcessIdentity, type SessionRecord, @@ -218,7 +219,12 @@ export async function createBrowserSession( ...(opts.binaryPath !== undefined ? { binaryPath: opts.binaryPath } : {}), }); - await reapDeadRunningSessions(registryEnv); + await reapDeadRunningSessions(registryEnv, { + browser: { + teardown: (id, finalize) => + teardownBrowserSession(id, registryEnv, finalize), + }, + }); assertNotAborted(opts.signal); const record = await createSession( { type: "browser", projectDir: opts.projectDir }, @@ -489,9 +495,10 @@ export async function getBrowserSessionStatus( * into one error * and leave the record in `error` state for inspection. */ -export async function destroyBrowserSession( +export async function teardownBrowserSession( id: string, - registryEnv: EnvLike = process.env, + registryEnv: EnvLike, + finalize: LocalSessionTeardownFinalizer, ): Promise { const initial = await getSession(id, registryEnv); if (initial === undefined) { @@ -624,6 +631,15 @@ export async function destroyBrowserSession( `Failed to fully destroy browser session ${id}`, ); } - await destroySessionRecord(id, registryEnv); + await finalize(); }); } + +export async function destroyBrowserSession( + id: string, + registryEnv: EnvLike = process.env, +): Promise { + await teardownBrowserSession(id, registryEnv, () => + destroySessionRecord(id, registryEnv), + ); +} diff --git a/packages/browser/test/session.test.ts b/packages/browser/test/session.test.ts index 022cfee..b71e3a8 100644 --- a/packages/browser/test/session.test.ts +++ b/packages/browser/test/session.test.ts @@ -28,6 +28,7 @@ import { destroyBrowserSession, getBrowserSessionStatus, resolveLiveBrowserSession, + teardownBrowserSession, type BrowserSessionHandle, } from "../src/index.js"; import { fakePath, writeExecutable, writeFakeChrome } from "./fakes.js"; @@ -58,6 +59,15 @@ afterEach(() => { fs.rmSync(tmp, { recursive: true, force: true }); }); +function reapBrowserSessions() { + return reapDeadRunningSessions(registryEnv, { + browser: { + teardown: (id, finalize) => + teardownBrowserSession(id, registryEnv, finalize), + }, + }); +} + function spawnEnvFor( mode: FakeChromeMode, extra: EnvLike = {}, @@ -538,9 +548,7 @@ describe.skipIf(!hasXvfb)("destroyBrowserSession (fake binaries)", () => { registryEnv, ); expect( - (await reapDeadRunningSessions(registryEnv)).map( - (reaped) => reaped.id, - ), + (await reapBrowserSessions()).map((reaped) => reaped.id), ).toEqual([session.id]); expect(await getSession(session.id, registryEnv)).toBeUndefined(); expect(fs.existsSync(session.profileDir)).toBe(false); @@ -891,7 +899,7 @@ describe.skipIf(!hasXvfb)("partial-failure cleanup (fake binaries)", () => { await stopPid(childPid, { timeoutMs: 1000 }); expect( - (await reapDeadRunningSessions(registryEnv)).map((record) => record.id), + (await reapBrowserSessions()).map((record) => record.id), ).toEqual([id]); expect(await getSession(id, registryEnv)).toBeUndefined(); expect(isPidAlive(failedXvfbPid)).toBe(false); @@ -1070,7 +1078,7 @@ describe("browser record inspection (no live processes)", () => { true, ); - expect(await reapDeadRunningSessions(registryEnv)).toEqual([]); + expect(await reapBrowserSessions()).toEqual([]); expect(await getSession(rec.id, registryEnv)).toBeDefined(); expect(fs.existsSync(outside)).toBe(true); }); diff --git a/packages/cli/src/commands/session.ts b/packages/cli/src/commands/session.ts index a46c536..15d168b 100644 --- a/packages/cli/src/commands/session.ts +++ b/packages/cli/src/commands/session.ts @@ -2,11 +2,13 @@ import { createAndroidSession, destroyAndroidSession, getAndroidSessionStatus, + teardownAndroidSession, } from "@pickforge/picklab-android"; import { createBrowserSession, destroyBrowserSession, getBrowserSessionStatus, + teardownBrowserSession, } from "@pickforge/picklab-browser"; import { createLocalSessions, @@ -15,18 +17,21 @@ import { listSessions, loadConfig, localSessionStatusEntry, + reapDeadRunningSessions, type LocalSessionCreateRuntime, type LocalSessionDestroyRuntime, type LocalSessionRecipe, type LocalSessionStatusEntry, type LocalSessionStatusRuntime, type LocalSessionSummary, + type LocalSessionTeardownRuntime, type SessionRecord, } from "@pickforge/picklab-core"; import { createDesktopSession, destroyDesktopSession, getDesktopSessionStatus, + teardownDesktopSession, } from "@pickforge/picklab-desktop-linux"; import { parseIntArg, @@ -104,6 +109,21 @@ const destroyRuntime: LocalSessionDestroyRuntime = { android: { destroy: (id) => destroyAndroidSession(id) }, }; +const reaperRuntime: LocalSessionTeardownRuntime = { + desktop: { + teardown: (id, finalize) => + teardownDesktopSession(id, process.env, finalize), + }, + browser: { + teardown: (id, finalize) => + teardownBrowserSession(id, process.env, finalize), + }, + android: { + teardown: (id, finalize) => + teardownAndroidSession(id, process.env, {}, finalize), + }, +}; + function describeCreated(summary: LocalSessionSummary): string { if (summary.type === "desktop") { const vnc = @@ -153,6 +173,7 @@ export async function runSessionCreate( (viewerMode === "auto" && (!createsWritableDesktop || opts.vncControl !== true))); + await reapDeadRunningSessions(process.env, reaperRuntime); const sessions = await createLocalSessions(opts.type, createRuntime(opts)); const lines = sessions.map(describeCreated); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index cb8a877..17fb944 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -136,6 +136,8 @@ export { destroyLocalSession, destroyLocalSessions, localSessionStatusEntry, + reapDeadRunningSessions, + teardownLocalSession, type AndroidLegHandle, type AndroidLiveStatus, type BrowserLegHandle, @@ -151,6 +153,8 @@ export { type LocalSessionStatusEntry, type LocalSessionStatusRuntime, type LocalSessionSummary, + type LocalSessionTeardownFinalizer, + type LocalSessionTeardownRuntime, } from "./session-lifecycle.js"; export { @@ -161,7 +165,6 @@ export { isDisplaySocketAlive, isSessionProcessAlive, listSessions, - reapDeadRunningSessions, updateSession, type AndroidSessionInfo, type BrowserSessionInfo, diff --git a/packages/core/src/session-lifecycle.ts b/packages/core/src/session-lifecycle.ts index 3a2a354..0762a74 100644 --- a/packages/core/src/session-lifecycle.ts +++ b/packages/core/src/session-lifecycle.ts @@ -1,4 +1,14 @@ -import type { SessionRecord, SessionType } from "./session.js"; +import type { EnvLike } from "./paths.js"; +import { + REAPER_CLEANUP_PENDING_META_KEY, + destroySessionRecord, + isSessionProcessAlive, + listSessions, + updateSession, + type SessionLivenessCheck, + type SessionRecord, + type SessionType, +} from "./session.js"; export type LocalSessionRecipe = SessionType; @@ -91,6 +101,29 @@ export interface LocalSessionDestroyRuntime { }; } +export type LocalSessionTeardownFinalizer = () => Promise; + +export interface LocalSessionTeardownRuntime { + desktop?: { + teardown: ( + id: string, + finalize: LocalSessionTeardownFinalizer, + ) => Promise; + }; + android?: { + teardown: ( + id: string, + finalize: LocalSessionTeardownFinalizer, + ) => Promise; + }; + browser?: { + teardown: ( + id: string, + finalize: LocalSessionTeardownFinalizer, + ) => Promise; + }; +} + export interface LocalSessionStatusEntry extends Record { id: string; type: SessionType; @@ -275,6 +308,84 @@ export async function localSessionStatusEntry( return entry; } +function canTeardownLocalSession( + record: SessionRecord, + runtime: LocalSessionTeardownRuntime, +): boolean { + if (record.type === "desktop") return runtime.desktop !== undefined; + if (record.type === "android") return runtime.android !== undefined; + if (record.type === "browser") return runtime.browser !== undefined; + return runtime.desktop !== undefined && runtime.android !== undefined; +} + +export async function teardownLocalSession( + record: SessionRecord, + runtime: LocalSessionTeardownRuntime, + finalize: LocalSessionTeardownFinalizer, +): Promise { + if (record.type === "desktop" && runtime.desktop !== undefined) { + await runtime.desktop.teardown(record.id, finalize); + return; + } + if (record.type === "android" && runtime.android !== undefined) { + await runtime.android.teardown(record.id, finalize); + return; + } + if (record.type === "browser" && runtime.browser !== undefined) { + await runtime.browser.teardown(record.id, finalize); + return; + } + if ( + record.type === "desktop+android" && + runtime.desktop !== undefined && + runtime.android !== undefined + ) { + await runtime.android.teardown(record.id, async () => {}); + await runtime.desktop.teardown(record.id, async () => {}); + await finalize(); + return; + } + throw new Error( + `No typed teardown runtime is available for session ${record.id} of type "${record.type}"`, + ); +} + +export async function reapDeadRunningSessions( + env: EnvLike, + runtime: LocalSessionTeardownRuntime, + isAlive: SessionLivenessCheck = isSessionProcessAlive, +): Promise { + const reaped: SessionRecord[] = []; + for (const record of await listSessions(env)) { + const retryPending = + record.status === "error" && + record.meta?.[REAPER_CLEANUP_PENDING_META_KEY] === true; + if (record.status !== "running" && !retryPending) continue; + if (!canTeardownLocalSession(record, runtime)) continue; + if (!retryPending && (await isAlive(record))) continue; + try { + await teardownLocalSession(record, runtime, () => + destroySessionRecord(record.id, env, "failed"), + ); + } catch { + await updateSession( + record.id, + { + status: "error", + meta: { + ...record.meta, + [REAPER_CLEANUP_PENDING_META_KEY]: true, + }, + }, + env, + ).catch(() => {}); + continue; + } + reaped.push(record); + } + return reaped; +} + export async function destroyLocalSession( record: SessionRecord, runtime: LocalSessionDestroyRuntime, diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index b23ca29..710765f 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -5,18 +5,12 @@ import { finalizeActiveEvidenceRun } from "./evidence.js"; import { writeEvidenceReport } from "./evidence-render.js"; import { ensureDir, - isProfileConfined, sessionsDir, runsDir, writeFileAtomic, type EnvLike, } from "./paths.js"; -import { - isPidAlive, - processIdentityMatches, - stopPid, - stopProcessGroupVerified, -} from "./proc.js"; +import { isPidAlive, processIdentityMatches } from "./proc.js"; export type SessionType = "desktop" | "android" | "desktop+android" | "browser"; export type SessionStatus = "starting" | "running" | "stopped" | "error"; @@ -273,141 +267,6 @@ export function isSessionProcessAlive(record: SessionRecord): boolean { export const REAPER_CLEANUP_PENDING_META_KEY = "reaperCleanupPending"; -export async function reapDeadRunningSessions( - env: EnvLike = process.env, - isAlive: SessionLivenessCheck = isSessionProcessAlive, -): Promise { - const reaped: SessionRecord[] = []; - for (const record of await listSessions(env)) { - const retryPending = - record.status === "error" && - record.meta?.[REAPER_CLEANUP_PENDING_META_KEY] === true; - if (record.status !== "running" && !retryPending) continue; - if (!retryPending && (await isAlive(record))) continue; - if (!(await stopRecordedPids(record, env))) { - await updateSession( - record.id, - { - status: "error", - meta: { - ...record.meta, - [REAPER_CLEANUP_PENDING_META_KEY]: true, - }, - }, - env, - ).catch(() => {}); - continue; - } - try { - await destroySessionRecord(record.id, env, "failed"); - } catch { - await updateSession( - record.id, - { - status: "error", - meta: { - ...record.meta, - [REAPER_CLEANUP_PENDING_META_KEY]: true, - }, - }, - env, - ).catch(() => {}); - continue; - } - reaped.push(record); - } - return reaped; -} - -async function stopRecordedGroup( - pid: number, - startTicks: number, -): Promise { - try { - const result = await stopProcessGroupVerified({ pid, startTicks }); - return ( - result.outcome === "terminated" || result.outcome === "already-dead" - ); - } catch { - return false; - } -} - -async function stopRecordedPids( - record: SessionRecord, - env: EnvLike, -): Promise { - const browser = record.browser; - if ( - browser !== undefined && - !(await stopRecordedGroup( - browser.browserPid, - browser.browserStartTimeTicks, - )) - ) { - return false; - } - - const desktop = record.desktop; - const vncPid = desktop?.vncPid; - const vncStartTimeTicks = desktop?.vncStartTimeTicks; - if (vncPid !== undefined && isPidAlive(vncPid)) { - if ( - vncStartTimeTicks === undefined || - !processIdentityMatches({ - pid: vncPid, - startTicks: vncStartTimeTicks, - }) - ) { - return false; - } - try { - if (!(await stopPid(vncPid))) return false; - } catch { - return false; - } - } - - const emulatorPid = record.android?.emulatorPid; - if (emulatorPid !== undefined && isPidAlive(emulatorPid)) { - try { - if (!(await stopPid(emulatorPid))) return false; - } catch { - return false; - } - } - - if (desktop?.xvfbPid !== undefined) { - if (desktop.xvfbStartTimeTicks === undefined) { - if (isPidAlive(desktop.xvfbPid)) return false; - } else if ( - !(await stopRecordedGroup( - desktop.xvfbPid, - desktop.xvfbStartTimeTicks, - )) - ) { - return false; - } - } - - const pendingBrowserCleanup = - record.type === "browser" && - record.meta?.[REAPER_CLEANUP_PENDING_META_KEY] === true; - if (browser?.profileMode === "ephemeral" || pendingBrowserCleanup) { - const sessionDir = path.join(sessionsDir(env), record.id); - const profileDir = browser?.profileDir ?? path.join(sessionDir, "profile"); - if (!(await isProfileConfined(sessionDir, profileDir))) { - return false; - } - try { - await fs.promises.rm(sessionDir, { recursive: true, force: true }); - } catch { - return false; - } - } - return true; -} - export async function updateSession( id: string, patch: Partial>, diff --git a/packages/core/test/session-lifecycle.test.ts b/packages/core/test/session-lifecycle.test.ts index 3a54bf8..b412ae6 100644 --- a/packages/core/test/session-lifecycle.test.ts +++ b/packages/core/test/session-lifecycle.test.ts @@ -3,10 +3,12 @@ import { createLocalSessions, destroyLocalSessions, localSessionStatusEntry, + teardownLocalSession, type LocalSessionCreateRuntime, type LocalSessionDestroyRuntime, type LocalSessionRecipe, type LocalSessionStatusRuntime, + type LocalSessionTeardownRuntime, type SessionRecord, } from "../src/index.js"; @@ -247,6 +249,34 @@ describe("local session lifecycle", () => { }); }); + it("combines typed desktop and android teardown before finalizing a legacy record", async () => { + const calls: string[] = []; + const runtime: LocalSessionTeardownRuntime = { + desktop: { + teardown: vi.fn(async (_id, finalize) => { + calls.push("desktop"); + await finalize(); + }), + }, + android: { + teardown: vi.fn(async (_id, finalize) => { + calls.push("android"); + await finalize(); + }), + }, + }; + + await teardownLocalSession( + record({ id: "duo-123456", type: "desktop+android" }), + runtime, + async () => { + calls.push("finalize"); + }, + ); + + expect(calls).toEqual(["android", "desktop", "finalize"]); + }); + it("dispatches typed destroy and continues after individual failures", async () => { const desktopDestroy = vi.fn(async () => { throw new Error("desktop stuck"); diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index a545088..8fd2fc0 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -5,6 +5,9 @@ import os from "node:os"; import path from "node:path"; import { once } from "node:events"; import { setTimeout as delay } from "node:timers/promises"; +import { teardownAndroidSession } from "../../android/src/session.js"; +import { teardownBrowserSession } from "../../browser/src/session.js"; +import { teardownDesktopSession } from "../../desktop-linux/src/session.js"; import { isPidAlive, readProcessIdentity, stopPid } from "../src/proc.js"; import { activePointerPath, @@ -18,13 +21,38 @@ import { getSession, isSessionProcessAlive, listSessions, - reapDeadRunningSessions, updateSession, + type SessionLivenessCheck, } from "../src/session.js"; +import { reapDeadRunningSessions as reapWithTypedRuntime } from "../src/session-lifecycle.js"; let home: string; let env: { PICKLAB_HOME: string }; +function reapDeadRunningSessions( + registryEnv: { PICKLAB_HOME: string }, + isAlive?: SessionLivenessCheck, +) { + return reapWithTypedRuntime( + registryEnv, + { + desktop: { + teardown: (id, finalize) => + teardownDesktopSession(id, registryEnv, finalize), + }, + android: { + teardown: (id, finalize) => + teardownAndroidSession(id, registryEnv, {}, finalize), + }, + browser: { + teardown: (id, finalize) => + teardownBrowserSession(id, registryEnv, finalize), + }, + }, + isAlive, + ); +} + beforeEach(async () => { home = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-sess-")); env = { PICKLAB_HOME: home }; @@ -853,7 +881,7 @@ describe("session registry", () => { const failed = await getSession(stale.id, env); expect(failed?.status).toBe("error"); expect(failed?.meta?.reaperCleanupPending).toBe(true); - expect(fs.existsSync(profileDir)).toBe(true); + expect(fs.existsSync(profileDir)).toBe(false); failRemoval = false; expect( diff --git a/packages/desktop-linux/src/index.ts b/packages/desktop-linux/src/index.ts index a22782b..117fcf9 100644 --- a/packages/desktop-linux/src/index.ts +++ b/packages/desktop-linux/src/index.ts @@ -93,6 +93,7 @@ export { ensureSessionVnc, getDesktopSessionStatus, stopOwnedSessionVnc, + teardownDesktopSession, withSessionVncLock, type CreateDesktopSessionOptions, type DesktopSessionHandle, diff --git a/packages/desktop-linux/src/session.ts b/packages/desktop-linux/src/session.ts index 75144d6..2fd2acd 100644 --- a/packages/desktop-linux/src/session.ts +++ b/packages/desktop-linux/src/session.ts @@ -16,6 +16,7 @@ import { updateSession, type DesktopSessionInfo, type EnvLike, + type LocalSessionTeardownFinalizer, type SessionRecord, } from "@pickforge/picklab-core"; import { @@ -86,7 +87,12 @@ export async function createDesktopSession( "VNC was requested but x11vnc was not found on PATH; install x11vnc to enable it", ); } - await reapDeadRunningSessions(registryEnv); + await reapDeadRunningSessions(registryEnv, { + desktop: { + teardown: (id, finalize) => + teardownDesktopSession(id, registryEnv, finalize), + }, + }); const record = await createSession( { type: "desktop", projectDir: opts.projectDir }, registryEnv, @@ -491,9 +497,10 @@ export async function ensureSessionVnc( }); } -export async function destroyDesktopSession( +export async function teardownDesktopSession( id: string, - registryEnv: EnvLike = process.env, + registryEnv: EnvLike, + finalize: LocalSessionTeardownFinalizer, ): Promise { if ((await getSession(id, registryEnv)) === undefined) { throw new Error(`Desktop session not found: ${id}`); @@ -562,10 +569,19 @@ export async function destroyDesktopSession( `Failed to stop ${failures.length} process(es) of desktop session ${id}`, ); } - await destroySessionRecord(id, registryEnv); + await finalize(); }); } +export async function destroyDesktopSession( + id: string, + registryEnv: EnvLike = process.env, +): Promise { + await teardownDesktopSession(id, registryEnv, () => + destroySessionRecord(id, registryEnv), + ); +} + export async function getDesktopSessionStatus( id: string, registryEnv: EnvLike = process.env, diff --git a/packages/desktop-linux/test/destroy.test.ts b/packages/desktop-linux/test/destroy.test.ts index 2f6bb88..1ccacec 100644 --- a/packages/desktop-linux/test/destroy.test.ts +++ b/packages/desktop-linux/test/destroy.test.ts @@ -46,7 +46,10 @@ import { updateSession, type EnvLike, } from "@pickforge/picklab-core"; -import { destroyDesktopSession } from "../src/session.js"; +import { + destroyDesktopSession, + teardownDesktopSession, +} from "../src/session.js"; const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-destroy-test-")); const registryEnv: EnvLike = { @@ -105,7 +108,12 @@ describe("destroyDesktopSession exception safety", () => { }); allowFailingPidStop = true; try { - const reaped = await reapDeadRunningSessions(registryEnv); + const reaped = await reapDeadRunningSessions(registryEnv, { + desktop: { + teardown: (sessionId, finalize) => + teardownDesktopSession(sessionId, registryEnv, finalize), + }, + }); expect(reaped.map((record) => record.id)).toEqual([id]); expect(await getSession(id, registryEnv)).toBeUndefined(); } finally { diff --git a/packages/mcp-server/src/tools/session.ts b/packages/mcp-server/src/tools/session.ts index f2da8ca..43f6dfc 100644 --- a/packages/mcp-server/src/tools/session.ts +++ b/packages/mcp-server/src/tools/session.ts @@ -9,11 +9,13 @@ import { createAndroidSession, destroyAndroidSession, getAndroidSessionStatus, + teardownAndroidSession, } from "@pickforge/picklab-android"; import { createBrowserSession, destroyBrowserSession, getBrowserSessionStatus, + teardownBrowserSession, } from "@pickforge/picklab-browser"; import { createLocalSessions, @@ -22,18 +24,21 @@ import { listSessions, loadConfig, localSessionStatusEntry, + reapDeadRunningSessions, type LocalSessionCreateRuntime, type LocalSessionDestroyRuntime, type LocalSessionLifecycle, type LocalSessionStatusEntry, type LocalSessionStatusRuntime, type LocalSessionSummary, + type LocalSessionTeardownRuntime, type SessionRecord, } from "@pickforge/picklab-core"; import { createDesktopSession, destroyDesktopSession, getDesktopSessionStatus, + teardownDesktopSession, } from "@pickforge/picklab-desktop-linux"; import { runTool, type ServerContext } from "../context.js"; import { withMcpEvidence } from "../evidence.js"; @@ -134,6 +139,23 @@ function destroyRuntime(ctx: ServerContext): LocalSessionDestroyRuntime { }; } +function reaperRuntime(ctx: ServerContext): LocalSessionTeardownRuntime { + return { + desktop: { + teardown: (id, finalize) => + teardownDesktopSession(id, ctx.env, finalize), + }, + browser: { + teardown: (id, finalize) => + teardownBrowserSession(id, ctx.env, finalize), + }, + android: { + teardown: (id, finalize) => + teardownAndroidSession(id, ctx.env, { env: ctx.env }, finalize), + }, + }; +} + export async function createSessions( ctx: ServerContext, args: { @@ -146,6 +168,7 @@ export async function createSessions( }, lifecycle: SessionLifecycle = {}, ): Promise { + await reapDeadRunningSessions(ctx.env, reaperRuntime(ctx)); return createLocalSessions( args.type ?? "desktop", createRuntime(ctx, args, lifecycle),