From 1852b18a43c823847a9e14cd9ecb8a4f1b7b214b Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Wed, 28 Oct 2026 15:35:24 +0000 Subject: [PATCH 1/5] feat: dispose session-owned platform drivers on teardown --- CHANGELOG.md | 8 ++ README.md | 10 ++ SKILL.md | 2 + .../mobile-platform-driver.dispose.test.ts | 46 ++++++++++ src/platform/mobile-platform-driver.ts | 16 ++++ src/platform/types.ts | 3 + src/server/cleanup-session.test.ts | 92 +++++++++++++++++++ src/server/cleanup-session.ts | 53 +++++++++++ src/server/create-server.test.ts | 22 +++++ src/server/create-server.ts | 5 +- src/server/session-manager.ts | 5 + src/tools/cleanup.test.ts | 9 ++ src/tools/cleanup.ts | 3 +- src/tools/launch.test.ts | 32 +++++++ src/tools/launch.ts | 15 ++- vitest.config.mts | 8 +- 16 files changed, 320 insertions(+), 9 deletions(-) create mode 100644 src/platform/mobile-platform-driver.dispose.test.ts create mode 100644 src/server/cleanup-session.test.ts create mode 100644 src/server/cleanup-session.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 16befe2..b865e90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add optional platform-driver disposal and forward mobile disposal to lifecycle-capable device-mcp backends. + +### Fixed + +- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals for retry. + ## [0.12.0] ### Added diff --git a/README.md b/README.md index 844ce5d..f31df05 100644 --- a/README.md +++ b/README.md @@ -237,6 +237,16 @@ type ISessionManager = { }; ``` +### Session-scoped device teardown + +Core captures `sessionManager.getPlatformDriver?.()` before calling the consumer's `cleanup()`, then calls the driver's optional `dispose()` hook. Disposal runs even when consumer cleanup fails. This applies to `mm cleanup`, forced relaunch, failed launch, daemon shutdown, and server startup rollback. A failed disposal is retained for a later cleanup attempt. + +`MobilePlatformDriver.dispose()` delegates to its backend's optional `dispose()` hook. With a device-mcp version that implements private IDB companions, this stops only that backend's companion and recording. Older device-mcp versions remain compatible, but do not gain companion teardown. Adopt the device-mcp lifecycle release in the consumer after it is published; this change does not update the core dependency range. + +Consumers must expose a session-owned driver through `getPlatformDriver()` and create a fresh driver/backend for each launch. If launch fails before the driver is exposed, the consumer must dispose that locally held driver in its own failure path. Direct calls to a consumer's `cleanup()` also remain the consumer's responsibility. + +`mm cleanup --shutdown` additionally stops the worktree's daemon. Driver disposal does not shut down a simulator, stop Metro, or terminate shared IDB companions. Use separate worktrees/daemons and explicit device IDs for independent agent sessions. Separate companions isolate connection teardown even for the same simulator; app state on that simulator remains shared. + ### Workflow Context & Capabilities The `WorkflowContext` aggregates optional capabilities that consumers inject through the `contextFactory`. The tool system checks for capabilities at runtime — tools that depend on missing capabilities return clear errors. diff --git a/SKILL.md b/SKILL.md index 68e918b..8ba6767 100644 --- a/SKILL.md +++ b/SKILL.md @@ -14,6 +14,8 @@ mm describe-screen # 4. Re-describe after every action to get fresh refs mm cleanup --shutdown # 5. Clean up when done ``` +Cleanup also disposes the session driver's owned resources after consumer app/session cleanup, including when cleanup fails. With a lifecycle-capable device-mcp backend, this releases its private companion and recording. `--shutdown` stops the worktree's daemon; it does not shut down a simulator or stop Metro. Independent agents should use separate worktrees/daemons and explicit device IDs. + **Critical rules:** - **Always `describe-screen` before interacting.** Refs like `e1`, `e2` are ephemeral — they change after every action. diff --git a/src/platform/mobile-platform-driver.dispose.test.ts b/src/platform/mobile-platform-driver.dispose.test.ts new file mode 100644 index 0000000..d7875ee --- /dev/null +++ b/src/platform/mobile-platform-driver.dispose.test.ts @@ -0,0 +1,46 @@ +import type { DeviceBackend } from '@metamask/device-mcp'; +import { describe, expect, it, vi } from 'vitest'; + +import { MobilePlatformDriver } from './mobile-platform-driver.js'; + +describe('MobilePlatformDriver disposal', () => { + it('disposes only its own backend once across concurrent and repeated calls', async () => { + const disposeA = vi.fn().mockResolvedValue(undefined); + const disposeB = vi.fn().mockResolvedValue(undefined); + const a = new MobilePlatformDriver({ + platform: 'ios', + dispose: disposeA, + } as unknown as DeviceBackend); + const b = new MobilePlatformDriver({ + platform: 'ios', + dispose: disposeB, + } as unknown as DeviceBackend); + await Promise.all([a.dispose(), a.dispose()]); + await a.dispose(); + expect(disposeA).toHaveBeenCalledTimes(1); + expect(disposeB).not.toHaveBeenCalled(); + await b.dispose(); + expect(disposeB).toHaveBeenCalledTimes(1); + }); + + it('supports device-mcp versions without disposal', async () => { + const driver = new MobilePlatformDriver({ + platform: 'ios', + } as DeviceBackend); + expect(await driver.dispose()).toBeUndefined(); + }); + + it('allows a retry after failed disposal', async () => { + const dispose = vi + .fn() + .mockRejectedValueOnce(new Error('failed')) + .mockResolvedValue(undefined); + const driver = new MobilePlatformDriver({ + platform: 'ios', + dispose, + } as unknown as DeviceBackend); + await expect(driver.dispose()).rejects.toThrowError('failed'); + await driver.dispose(); + expect(dispose).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/platform/mobile-platform-driver.ts b/src/platform/mobile-platform-driver.ts index bc97f8f..172643c 100644 --- a/src/platform/mobile-platform-driver.ts +++ b/src/platform/mobile-platform-driver.ts @@ -72,6 +72,8 @@ export class MobilePlatformDriver implements IPlatformDriver { // across mobile sessions (e.g. a relaunch against a different simulator). readonly #hermesSession: HermesSession; + #disposing: Promise | undefined; + /** * @param backend - The device-mcp backend to delegate to. * @param bundleId - The app bundle ID for getAppState calls. @@ -82,6 +84,20 @@ export class MobilePlatformDriver implements IPlatformDriver { this.#hermesSession = new HermesSession({ platform: backend.platform }); } + /** Release only this backend's resources; compatible with older device-mcp releases. */ + async dispose(): Promise { + const backend = this.#backend as DeviceBackend & { + dispose?(): Promise; + }; + this.#disposing ??= Promise.resolve() + .then(async () => backend.dispose?.()) + .catch((error: unknown) => { + this.#disposing = undefined; + throw error; + }); + await this.#disposing; + } + /** * @param targetType - Target identifier type. * @param targetValue - Target value for element lookup. diff --git a/src/platform/types.ts b/src/platform/types.ts index 529e6ed..3b6862b 100644 --- a/src/platform/types.ts +++ b/src/platform/types.ts @@ -49,6 +49,9 @@ export type WithinScope = { }; export type IPlatformDriver = { + /** Release resources owned by this driver. Idempotent; does not shut down the device. */ + dispose?(): Promise; + click( targetType: TargetType, targetValue: string, diff --git a/src/server/cleanup-session.test.ts b/src/server/cleanup-session.test.ts new file mode 100644 index 0000000..eead2b9 --- /dev/null +++ b/src/server/cleanup-session.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { cleanupSession } from './cleanup-session.js'; +import type { IPlatformDriver } from '../platform/types.js'; +import { createMockSessionManager } from '../tools/test-utils/mock-factories.js'; + +describe('cleanupSession', () => { + it('captures the driver before consumer cleanup clears it and disposes afterward', async () => { + const dispose = vi.fn().mockResolvedValue(undefined); + let driver = { dispose } as unknown as IPlatformDriver | undefined; + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => driver; + vi.spyOn(manager, 'cleanup').mockImplementation(async () => { + expect(dispose).not.toHaveBeenCalled(); + driver = undefined; + return true; + }); + + expect(await cleanupSession(manager)).toBe(true); + expect(dispose).toHaveBeenCalledTimes(1); + }); + + it('disposes the driver even when consumer cleanup fails', async () => { + const dispose = vi.fn().mockResolvedValue(undefined); + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => + ({ dispose }) as unknown as IPlatformDriver; + vi.spyOn(manager, 'cleanup').mockRejectedValue( + new Error('app close failed'), + ); + + await expect(cleanupSession(manager)).rejects.toThrowError( + 'app close failed', + ); + expect(dispose).toHaveBeenCalledTimes(1); + }); + + it('retries failed disposal even after the consumer has cleared the driver', async () => { + const dispose = vi + .fn() + .mockRejectedValueOnce(new Error('dispose failed')) + .mockResolvedValue(undefined); + let driver = { dispose } as unknown as IPlatformDriver | undefined; + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => driver; + vi.spyOn(manager, 'cleanup').mockImplementation(async () => { + driver = undefined; + return true; + }); + + await expect(cleanupSession(manager)).rejects.toThrowError( + 'dispose failed', + ); + expect(await cleanupSession(manager)).toBe(true); + expect(dispose).toHaveBeenCalledTimes(2); + }); + + it('does not dispose another session manager’s driver', async () => { + const a = createMockSessionManager(); + const b = createMockSessionManager(); + const disposeA = vi.fn().mockResolvedValue(undefined); + const disposeB = vi.fn().mockResolvedValue(undefined); + a.getPlatformDriver = () => + ({ dispose: disposeA }) as unknown as IPlatformDriver; + b.getPlatformDriver = () => + ({ dispose: disposeB }) as unknown as IPlatformDriver; + + await cleanupSession(a); + expect(disposeA).toHaveBeenCalledTimes(1); + expect(disposeB).not.toHaveBeenCalled(); + }); + + it('supports managers and drivers without disposal hooks', async () => { + const manager = createMockSessionManager(); + manager.getPlatformDriver = undefined; + expect(await cleanupSession(manager)).toBe(true); + }); + + it('reports both failures while still attempting every pending disposal', async () => { + const dispose = vi.fn().mockRejectedValue(new Error('dispose failed')); + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => + ({ dispose }) as unknown as IPlatformDriver; + vi.spyOn(manager, 'cleanup').mockRejectedValue(new Error('session failed')); + await expect(cleanupSession(manager)).rejects.toMatchObject({ + errors: [ + expect.objectContaining({ message: 'session failed' }), + expect.objectContaining({ message: 'dispose failed' }), + ], + }); + }); +}); diff --git a/src/server/cleanup-session.ts b/src/server/cleanup-session.ts new file mode 100644 index 0000000..b961585 --- /dev/null +++ b/src/server/cleanup-session.ts @@ -0,0 +1,53 @@ +import type { ISessionManager } from './session-manager.js'; +import type { IPlatformDriver } from '../platform/types.js'; + +// A consumer can clear its driver during cleanup before disposal fails. Retain +// that driver for a subsequent cleanup attempt rather than leaking its resources. +const pendingDrivers = new WeakMap>(); + +/** + * Close the app/session before releasing the driver resources it may still use. + * Disposal also runs when consumer cleanup fails, including partial launches. + * + * @param manager - The session manager whose resources should be released. + * @returns Whether the consumer cleaned up a session. + */ +export async function cleanupSession( + manager: ISessionManager, +): Promise { + const drivers = pendingDrivers.get(manager) ?? new Set(); + const driver = manager.getPlatformDriver?.(); + if (driver?.dispose) { + drivers.add(driver); + } + pendingDrivers.set(manager, drivers); + + const errors: unknown[] = []; + let cleanedUp = false; + try { + cleanedUp = await manager.cleanup(); + } catch (error) { + errors.push(error); + } + for (const pending of drivers) { + try { + await pending.dispose?.(); + drivers.delete(pending); + } catch (error) { + errors.push(error); + } + } + if (drivers.size === 0) { + pendingDrivers.delete(manager); + } + if (errors.length === 1) { + throw errors[0]; + } + if (errors.length > 1) { + throw new AggregateError( + errors, + 'Session cleanup and driver disposal failed', + ); + } + return cleanedUp; +} diff --git a/src/server/create-server.test.ts b/src/server/create-server.test.ts index 32f5f0b..9e9358c 100644 --- a/src/server/create-server.test.ts +++ b/src/server/create-server.test.ts @@ -735,6 +735,8 @@ describe('createServer integration', () => { await server.stop(); const mockSM = createMockSessionManager(); + const dispose = vi.fn().mockResolvedValue(undefined); + mockSM.getPlatformDriver.mockReturnValue({ dispose }); const customServer = createServer( buildConfig({ sessionManager: mockSM as unknown as ServerConfig['sessionManager'], @@ -748,6 +750,7 @@ describe('createServer integration', () => { try { await expect(customServer.start()).rejects.toThrowError(/EACCES/u); expect(mockSM.cleanup).toHaveBeenCalled(); + expect(dispose).toHaveBeenCalledTimes(1); } finally { await fs.chmod(tmpDir, 0o755).catch(() => {}); } @@ -846,6 +849,25 @@ describe('createServer integration', () => { expect(await server.stop()).toBeUndefined(); }); + it('disposes the session driver on daemon stop even when consumer cleanup fails', async () => { + await server.stop(); + const mockSM = createMockSessionManager(); + const dispose = vi.fn().mockResolvedValue(undefined); + mockSM.getPlatformDriver.mockReturnValue({ dispose }); + mockSM.cleanup.mockRejectedValue(new Error('app close failed')); + const customServer = createServer( + buildConfig({ + sessionManager: mockSM as unknown as ServerConfig['sessionManager'], + }), + ); + await customServer.start(); + await customServer.stop(); + expect(dispose).toHaveBeenCalledTimes(1); + expect(mockSM.cleanup.mock.invocationCallOrder[0]).toBeLessThan( + dispose.mock.invocationCallOrder[0], + ); + }); + describe('POST /tool/:name input validation', () => { it('returns 400 for missing required field', async () => { const res = await httpRequest( diff --git a/src/server/create-server.ts b/src/server/create-server.ts index b00fe1a..5ab4d90 100644 --- a/src/server/create-server.ts +++ b/src/server/create-server.ts @@ -4,6 +4,7 @@ import { randomUUID } from 'node:crypto'; import * as fs from 'node:fs/promises'; import * as http from 'node:http'; +import { cleanupSession } from './cleanup-session.js'; import { writeDaemonState, removeDaemonState } from './daemon-state.js'; import { compactObservation } from './observation-compaction.js'; import { RequestQueue } from './request-queue.js'; @@ -845,7 +846,7 @@ export function createServer(config: ServerConfig): ServerInstance { }); } try { - await config.sessionManager.cleanup(); + await cleanupSession(config.sessionManager); } catch { // Swallow — we're already propagating startupError. } @@ -897,7 +898,7 @@ export function createServer(config: ServerConfig): ServerInstance { // 4. Clean up session try { - await config.sessionManager.cleanup(); + await cleanupSession(config.sessionManager); } catch (error) { appendLog( config.logFilePath, diff --git a/src/server/session-manager.ts b/src/server/session-manager.ts index b700d1b..b209519 100644 --- a/src/server/session-manager.ts +++ b/src/server/session-manager.ts @@ -332,6 +332,11 @@ export type ISessionManager = { // Platform Driver (Optional — consumers that support mobile implement these) // ----------------------------------------------------------------------------- + /** + * Return the session-owned driver, including during partial launch when possible. + * Core captures it before cleanup and disposes its resources afterward. Consumers + * must dispose drivers that fail before they can be exposed here. + */ getPlatformDriver?(): IPlatformDriver | undefined; setPlatformDriver?(driver: IPlatformDriver): void; diff --git a/src/tools/cleanup.test.ts b/src/tools/cleanup.test.ts index 5348703..4640d93 100644 --- a/src/tools/cleanup.test.ts +++ b/src/tools/cleanup.test.ts @@ -71,4 +71,13 @@ describe('cleanupTool', () => { expect(context.sessionManager.cleanup).toHaveBeenCalledTimes(2); }); + + it('disposes the active platform driver through the cleanup tool', async () => { + const context = createMockContext(true); + const dispose = vi.fn().mockResolvedValue(undefined); + context.sessionManager.getPlatformDriver = () => + ({ dispose }) as unknown as NonNullable; + await cleanupTool({}, context); + expect(dispose).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/tools/cleanup.ts b/src/tools/cleanup.ts index cae4b36..68f3c30 100644 --- a/src/tools/cleanup.ts +++ b/src/tools/cleanup.ts @@ -1,5 +1,6 @@ import type { CleanupInput, CleanupResult } from './types'; import { createToolSuccess } from './utils.js'; +import { cleanupSession } from '../server/cleanup-session.js'; import type { ToolContext, ToolResponse } from '../types/http.js'; /** @@ -13,7 +14,7 @@ export async function cleanupTool( _input: CleanupInput, context: ToolContext, ): Promise> { - const cleanedUp = await context.sessionManager.cleanup(); + const cleanedUp = await cleanupSession(context.sessionManager); return createToolSuccess({ cleanedUp }); } diff --git a/src/tools/launch.test.ts b/src/tools/launch.test.ts index 3f9151f..eb36237 100644 --- a/src/tools/launch.test.ts +++ b/src/tools/launch.test.ts @@ -216,6 +216,9 @@ describe('launchTool', () => { it('cleans up and relaunches when force is true', async () => { const context = createMockContext({ hasActive: true }); vi.spyOn(context.sessionManager, 'cleanup').mockResolvedValue(true); + const dispose = vi.fn().mockResolvedValue(undefined); + context.sessionManager.getPlatformDriver = () => + ({ dispose }) as unknown as NonNullable; const result = await launchTool( { stateMode: 'default', force: true }, @@ -224,11 +227,40 @@ describe('launchTool', () => { expect(result.ok).toBe(true); expect(context.sessionManager.cleanup).toHaveBeenCalled(); + expect(dispose).toHaveBeenCalledTimes(1); + expect(dispose.mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(context.sessionManager.launch).mock.invocationCallOrder[0], + ); expect(context.sessionManager.launch).toHaveBeenCalled(); }); }); describe('launch failures', () => { + it('disposes an exposed partial-launch driver even if consumer cleanup fails', async () => { + const context = createMockContext({ hasActive: false }); + const dispose = vi.fn().mockResolvedValue(undefined); + context.sessionManager.getPlatformDriver = () => + ({ dispose }) as unknown as NonNullable; + vi.spyOn(context.sessionManager, 'launch').mockRejectedValue( + Object.assign(new Error('device failed'), { + code: ErrorCodes.MM_DEPENDENCIES_MISSING, + }), + ); + vi.spyOn(context.sessionManager, 'cleanup').mockRejectedValue( + new Error('cleanup failed'), + ); + + const result = await launchTool({ stateMode: 'default' }, context); + expect(dispose).toHaveBeenCalledTimes(1); + expect(result).toMatchObject({ + ok: false, + error: { + code: ErrorCodes.MM_DEPENDENCIES_MISSING, + message: 'device failed', + }, + }); + }); + it('returns port conflict error for EADDRINUSE', async () => { const context = createMockContext({ hasActive: false }); vi.spyOn(context.sessionManager, 'launch').mockRejectedValue( diff --git a/src/tools/launch.ts b/src/tools/launch.ts index b8b8e04..5025ab2 100644 --- a/src/tools/launch.ts +++ b/src/tools/launch.ts @@ -2,6 +2,7 @@ import type { LaunchInput, LaunchPrerequisite, LaunchResult } from './types'; import { ErrorCodes } from './types'; import type { ErrorCode } from './types/errors.js'; import { createToolError, createToolSuccess } from './utils.js'; +import { cleanupSession } from '../server/cleanup-session.js'; import type { ToolContext, ToolResponse } from '../types/http.js'; import { extractErrorMessage } from '../utils'; @@ -39,7 +40,7 @@ export async function launchTool( try { if (sessionManager.hasActiveSession()) { if (input.force) { - await sessionManager.cleanup(); + await cleanupSession(sessionManager); } else { return createToolError( ErrorCodes.MM_SESSION_ALREADY_RUNNING, @@ -52,7 +53,17 @@ export async function launchTool( sessionManager.setContext(input.context); } - const result = await sessionManager.launch(input); + const result = await sessionManager + .launch(input) + .catch(async (error: unknown) => { + // Preserve the launch error after best-effort cleanup of a partial session. + try { + await cleanupSession(sessionManager); + } catch { + // The original launch error is the actionable failure. + } + throw error; + }); const isProdMode = sessionManager.getEnvironmentMode() === 'prod'; return createToolSuccess({ diff --git a/vitest.config.mts b/vitest.config.mts index b7cf922..5803445 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -35,10 +35,10 @@ export default defineConfig({ // Auto-update the coverage thresholds when running locally. // Disabled in CI to prevent non-deterministic config changes. autoUpdate: !process.env.CI, - branches: 91.16, - functions: 93.64, - lines: 96.35, - statements: 96.11, + branches: 91.68, + functions: 94.76, + lines: 96.67, + statements: 96.44, }, }, From ae94d39ae65aba736edcbc2b2b1a8f73c059b766 Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Wed, 28 Oct 2026 18:57:05 +0000 Subject: [PATCH 2/5] fix: keep driver disposal retries in a server-owned lifecycle Failed disposals were kept in a module-level WeakMap keyed by the session manager object a caller passed. When a tool receives a per-request wrapper of the manager (a Proxy on the core stack), a failed disposal is stored under that wrapper and is lost after the request. The consumer already cleared its driver, so the companion leaks, and stop() and startup rollback never see it. - Add DriverLifecycle, which owns the pending set. createServer creates one and uses it for stop(), startup rollback and the tools, through a new optional ToolContext.cleanupSession(). - Keep cleanupSession(manager, lifecycle?) with a per-manager fallback for contexts built outside the daemon. - Add createServer tests: a failing /tool/cleanup is retried by the next /tool/cleanup and by stop(). --- README.md | 2 +- src/server/cleanup-session.test.ts | 41 ++++++++++- src/server/cleanup-session.ts | 112 +++++++++++++++++++---------- src/server/create-server.test.ts | 69 ++++++++++++++++++ src/server/create-server.ts | 11 ++- src/tools/cleanup.test.ts | 12 ++++ src/tools/cleanup.ts | 3 +- src/tools/launch.test.ts | 22 ++++++ src/tools/launch.ts | 6 +- src/types/http.ts | 6 ++ vitest.config.mts | 6 +- 11 files changed, 241 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index f31df05..a5ef8d9 100644 --- a/README.md +++ b/README.md @@ -239,7 +239,7 @@ type ISessionManager = { ### Session-scoped device teardown -Core captures `sessionManager.getPlatformDriver?.()` before calling the consumer's `cleanup()`, then calls the driver's optional `dispose()` hook. Disposal runs even when consumer cleanup fails. This applies to `mm cleanup`, forced relaunch, failed launch, daemon shutdown, and server startup rollback. A failed disposal is retained for a later cleanup attempt. +Core captures `sessionManager.getPlatformDriver?.()` before calling the consumer's `cleanup()`, then calls the driver's optional `dispose()` hook. Disposal runs even when consumer cleanup fails. This applies to `mm cleanup`, forced relaunch, failed launch, daemon shutdown, and server startup rollback. A failed disposal is retained for a later cleanup attempt. The daemon owns that retention (one `DriverLifecycle` per `createServer`), so a later `mm cleanup`, forced relaunch, or daemon stop retries it, whatever wrapper of the session manager a tool receives. Tools reach it through `ToolContext.cleanupSession()`; without it (a context built outside the daemon), the tools fall back to retention per session-manager object. `MobilePlatformDriver.dispose()` delegates to its backend's optional `dispose()` hook. With a device-mcp version that implements private IDB companions, this stops only that backend's companion and recording. Older device-mcp versions remain compatible, but do not gain companion teardown. Adopt the device-mcp lifecycle release in the consumer after it is published; this change does not update the core dependency range. diff --git a/src/server/cleanup-session.test.ts b/src/server/cleanup-session.test.ts index eead2b9..61a50e3 100644 --- a/src/server/cleanup-session.test.ts +++ b/src/server/cleanup-session.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; -import { cleanupSession } from './cleanup-session.js'; +import { cleanupSession, DriverLifecycle } from './cleanup-session.js'; import type { IPlatformDriver } from '../platform/types.js'; import { createMockSessionManager } from '../tools/test-utils/mock-factories.js'; @@ -89,4 +89,43 @@ describe('cleanupSession', () => { ], }); }); + + it('retains failed disposals across per-request wrappers of one manager', async () => { + // The daemon hands each request a new wrapper (a Proxy on the stack) of + // the same session manager. Retention must not depend on that identity. + const lifecycle = new DriverLifecycle(); + const dispose = vi + .fn() + .mockRejectedValueOnce(new Error('dispose failed')) + .mockResolvedValue(undefined); + let driver = { dispose } as unknown as IPlatformDriver | undefined; + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => driver; + vi.spyOn(manager, 'cleanup').mockImplementation(async () => { + driver = undefined; + return true; + }); + const wrap = (): typeof manager => new Proxy(manager, {}); + + await expect(lifecycle.cleanupSession(wrap())).rejects.toThrowError( + 'dispose failed', + ); + expect(lifecycle.pendingCount).toBe(1); + expect(await lifecycle.cleanupSession(wrap())).toBe(true); + expect(dispose).toHaveBeenCalledTimes(2); + expect(lifecycle.pendingCount).toBe(0); + }); + + it('uses the given lifecycle instead of the per-manager fallback', async () => { + const lifecycle = new DriverLifecycle(); + const dispose = vi.fn().mockRejectedValueOnce(new Error('dispose failed')); + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => + ({ dispose }) as unknown as IPlatformDriver; + + await expect(cleanupSession(manager, lifecycle)).rejects.toThrowError( + 'dispose failed', + ); + expect(lifecycle.pendingCount).toBe(1); + }); }); diff --git a/src/server/cleanup-session.ts b/src/server/cleanup-session.ts index b961585..99202f4 100644 --- a/src/server/cleanup-session.ts +++ b/src/server/cleanup-session.ts @@ -1,53 +1,89 @@ import type { ISessionManager } from './session-manager.js'; import type { IPlatformDriver } from '../platform/types.js'; -// A consumer can clear its driver during cleanup before disposal fails. Retain -// that driver for a subsequent cleanup attempt rather than leaking its resources. -const pendingDrivers = new WeakMap>(); - /** - * Close the app/session before releasing the driver resources it may still use. - * Disposal also runs when consumer cleanup fails, including partial launches. + * Owns the drivers whose disposal failed, so a later cleanup can retry them. * - * @param manager - The session manager whose resources should be released. - * @returns Whether the consumer cleaned up a session. + * A consumer can clear its driver during cleanup before disposal fails; the + * driver is then reachable only from here. Retention is keyed by this + * instance, not by the session manager object, because callers can see the + * same manager through different wrappers (for example a per-request proxy). + * `createServer` owns one instance and uses it for the cleanup tool, forced + * and failed launches, daemon stop and startup rollback. */ -export async function cleanupSession( - manager: ISessionManager, -): Promise { - const drivers = pendingDrivers.get(manager) ?? new Set(); - const driver = manager.getPlatformDriver?.(); - if (driver?.dispose) { - drivers.add(driver); - } - pendingDrivers.set(manager, drivers); +export class DriverLifecycle { + readonly #pending = new Set(); - const errors: unknown[] = []; - let cleanedUp = false; - try { - cleanedUp = await manager.cleanup(); - } catch (error) { - errors.push(error); + /** + * @returns How many drivers still wait for a successful disposal. + */ + get pendingCount(): number { + return this.#pending.size; } - for (const pending of drivers) { + + /** + * Close the app/session before releasing the driver resources it may still + * use. Disposal also runs when consumer cleanup fails, including partial + * launches. Previously failed disposals are retried. + * + * @param manager - The session manager whose resources should be released. + * @returns Whether the consumer cleaned up a session. + */ + async cleanupSession(manager: ISessionManager): Promise { + const driver = manager.getPlatformDriver?.(); + if (driver?.dispose) { + this.#pending.add(driver); + } + + const errors: unknown[] = []; + let cleanedUp = false; try { - await pending.dispose?.(); - drivers.delete(pending); + cleanedUp = await manager.cleanup(); } catch (error) { errors.push(error); } + for (const pending of [...this.#pending]) { + try { + await pending.dispose?.(); + this.#pending.delete(pending); + } catch (error) { + errors.push(error); + } + } + if (errors.length === 1) { + throw errors[0]; + } + if (errors.length > 1) { + throw new AggregateError( + errors, + 'Session cleanup and driver disposal failed', + ); + } + return cleanedUp; } - if (drivers.size === 0) { - pendingDrivers.delete(manager); - } - if (errors.length === 1) { - throw errors[0]; - } - if (errors.length > 1) { - throw new AggregateError( - errors, - 'Session cleanup and driver disposal failed', - ); +} + +// Fallback for callers without a server-owned lifecycle (for example a +// ToolContext built outside createServer). +const fallbackLifecycles = new WeakMap(); + +/** + * Clean up a session and dispose its driver. Prefer passing the server-owned + * {@link DriverLifecycle}; without one, failed disposals are retained per + * session manager object. + * + * @param manager - The session manager whose resources should be released. + * @param lifecycle - The lifecycle that retains failed disposals. + * @returns Whether the consumer cleaned up a session. + */ +export async function cleanupSession( + manager: ISessionManager, + lifecycle?: DriverLifecycle, +): Promise { + let owner = lifecycle ?? fallbackLifecycles.get(manager); + if (!owner) { + owner = new DriverLifecycle(); + fallbackLifecycles.set(manager, owner); } - return cleanedUp; + return owner.cleanupSession(manager); } diff --git a/src/server/create-server.test.ts b/src/server/create-server.test.ts index 9e9358c..27e6a40 100644 --- a/src/server/create-server.test.ts +++ b/src/server/create-server.test.ts @@ -868,6 +868,75 @@ describe('createServer integration', () => { ); }); + describe('driver disposal retries (server-owned)', () => { + function flakyDriverSession(): { + mockSM: ReturnType; + dispose: ReturnType; + } { + const mockSM = createMockSessionManager(); + const dispose = vi + .fn() + .mockRejectedValueOnce(new Error('companion stop timed out')) + .mockResolvedValue(undefined); + let driver: + | { dispose: typeof dispose; getPlatform: () => 'ios' } + | undefined = { dispose, getPlatform: () => 'ios' }; + mockSM.hasActiveSession.mockImplementation(() => driver !== undefined); + mockSM.getPlatformDriver.mockImplementation(() => driver); + mockSM.cleanup.mockImplementation(async () => { + // The consumer resets its session state, dropping the driver. + driver = undefined; + return true; + }); + return { mockSM, dispose }; + } + + async function postCleanup(port: number): Promise { + const res = await httpRequest(`http://127.0.0.1:${port}/tool/cleanup`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + await res.json(); + return res.status; + } + + it('retries a failed disposal on the next /tool/cleanup', async () => { + await server.stop(); + const { mockSM, dispose } = flakyDriverSession(); + const customServer = createServer( + buildConfig({ + sessionManager: mockSM as unknown as ServerConfig['sessionManager'], + }), + ); + const started = await customServer.start(); + try { + expect(await postCleanup(started.port)).toBe(500); + expect(dispose).toHaveBeenCalledTimes(1); + expect(await postCleanup(started.port)).toBe(200); + expect(dispose).toHaveBeenCalledTimes(2); + } finally { + await customServer.stop(); + } + expect(dispose).toHaveBeenCalledTimes(2); + }); + + it('retries a failed /tool/cleanup disposal on daemon stop', async () => { + await server.stop(); + const { mockSM, dispose } = flakyDriverSession(); + const customServer = createServer( + buildConfig({ + sessionManager: mockSM as unknown as ServerConfig['sessionManager'], + }), + ); + const started = await customServer.start(); + expect(await postCleanup(started.port)).toBe(500); + expect(dispose).toHaveBeenCalledTimes(1); + await customServer.stop(); + expect(dispose).toHaveBeenCalledTimes(2); + }); + }); + describe('POST /tool/:name input validation', () => { it('returns 400 for missing required field', async () => { const res = await httpRequest( diff --git a/src/server/create-server.ts b/src/server/create-server.ts index 5ab4d90..4a36937 100644 --- a/src/server/create-server.ts +++ b/src/server/create-server.ts @@ -4,7 +4,7 @@ import { randomUUID } from 'node:crypto'; import * as fs from 'node:fs/promises'; import * as http from 'node:http'; -import { cleanupSession } from './cleanup-session.js'; +import { DriverLifecycle } from './cleanup-session.js'; import { writeDaemonState, removeDaemonState } from './daemon-state.js'; import { compactObservation } from './observation-compaction.js'; import { RequestQueue } from './request-queue.js'; @@ -259,6 +259,9 @@ export function createServer(config: ServerConfig): ServerInstance { const queue = new RequestQueue(config.requestTimeoutMs); const nonce = randomUUID(); const knowledgeStore = config.knowledgeStore ?? new KnowledgeStore(); + // One owner for driver disposal retries across tools, stop() and rollback. + // Keyed by this server, not by the session manager object a caller sees. + const driverLifecycle = new DriverLifecycle(); let httpServer: http.Server | null = null; let worktreeRoot = ''; @@ -341,6 +344,8 @@ export function createServer(config: ServerConfig): ServerInstance { knowledgeStore, toolRegistry, driver, + cleanupSession: async () => + driverLifecycle.cleanupSession(config.sessionManager), }; } @@ -846,7 +851,7 @@ export function createServer(config: ServerConfig): ServerInstance { }); } try { - await cleanupSession(config.sessionManager); + await driverLifecycle.cleanupSession(config.sessionManager); } catch { // Swallow — we're already propagating startupError. } @@ -898,7 +903,7 @@ export function createServer(config: ServerConfig): ServerInstance { // 4. Clean up session try { - await cleanupSession(config.sessionManager); + await driverLifecycle.cleanupSession(config.sessionManager); } catch (error) { appendLog( config.logFilePath, diff --git a/src/tools/cleanup.test.ts b/src/tools/cleanup.test.ts index 4640d93..6ca0e33 100644 --- a/src/tools/cleanup.test.ts +++ b/src/tools/cleanup.test.ts @@ -80,4 +80,16 @@ describe('cleanupTool', () => { await cleanupTool({}, context); expect(dispose).toHaveBeenCalledTimes(1); }); + + it('uses the server-owned cleanup when the context provides one', async () => { + const context = createMockContext(true); + const serverCleanup = vi.fn().mockResolvedValue(true); + const result = await cleanupTool( + {}, + { ...context, cleanupSession: serverCleanup }, + ); + expect(serverCleanup).toHaveBeenCalledTimes(1); + expect(context.sessionManager.cleanup).not.toHaveBeenCalled(); + expect(result).toStrictEqual({ ok: true, result: { cleanedUp: true } }); + }); }); diff --git a/src/tools/cleanup.ts b/src/tools/cleanup.ts index 68f3c30..8a43b4e 100644 --- a/src/tools/cleanup.ts +++ b/src/tools/cleanup.ts @@ -14,7 +14,8 @@ export async function cleanupTool( _input: CleanupInput, context: ToolContext, ): Promise> { - const cleanedUp = await cleanupSession(context.sessionManager); + const cleanedUp = await (context.cleanupSession?.() ?? + cleanupSession(context.sessionManager)); return createToolSuccess({ cleanedUp }); } diff --git a/src/tools/launch.test.ts b/src/tools/launch.test.ts index eb36237..7ea31a8 100644 --- a/src/tools/launch.test.ts +++ b/src/tools/launch.test.ts @@ -261,6 +261,28 @@ describe('launchTool', () => { }); }); + it('uses the server-owned cleanup for forced and failed launches', async () => { + const serverCleanup = vi.fn().mockResolvedValue(true); + const forced = createMockContext({ hasActive: true }); + await launchTool( + { stateMode: 'default', force: true }, + { ...forced, cleanupSession: serverCleanup }, + ); + expect(serverCleanup).toHaveBeenCalledTimes(1); + expect(forced.sessionManager.cleanup).not.toHaveBeenCalled(); + + const failed = createMockContext({ hasActive: false }); + vi.spyOn(failed.sessionManager, 'launch').mockRejectedValue( + new Error('device failed'), + ); + const result = await launchTool( + { stateMode: 'default' }, + { ...failed, cleanupSession: serverCleanup }, + ); + expect(result.ok).toBe(false); + expect(serverCleanup).toHaveBeenCalledTimes(2); + }); + it('returns port conflict error for EADDRINUSE', async () => { const context = createMockContext({ hasActive: false }); vi.spyOn(context.sessionManager, 'launch').mockRejectedValue( diff --git a/src/tools/launch.ts b/src/tools/launch.ts index 5025ab2..4ad5a67 100644 --- a/src/tools/launch.ts +++ b/src/tools/launch.ts @@ -36,11 +36,13 @@ export async function launchTool( context: ToolContext, ): Promise> { const { sessionManager } = context; + const cleanup = async (): Promise => + context.cleanupSession?.() ?? cleanupSession(sessionManager); try { if (sessionManager.hasActiveSession()) { if (input.force) { - await cleanupSession(sessionManager); + await cleanup(); } else { return createToolError( ErrorCodes.MM_SESSION_ALREADY_RUNNING, @@ -58,7 +60,7 @@ export async function launchTool( .catch(async (error: unknown) => { // Preserve the launch error after best-effort cleanup of a partial session. try { - await cleanupSession(sessionManager); + await cleanup(); } catch { // The original launch error is the actionable failure. } diff --git a/src/types/http.ts b/src/types/http.ts index 604f99d..a1d33ca 100644 --- a/src/types/http.ts +++ b/src/types/http.ts @@ -32,6 +32,12 @@ export type ToolContext = { toolRegistry: Map>; /** Platform driver for cross-platform element interaction (browser, iOS, Android) */ driver?: IPlatformDriver; + /** + * Cleans up the session and disposes its driver through the server-owned + * lifecycle, so failed disposals are retried by later cleanups and by + * daemon stop. Absent when the context was not built by the daemon. + */ + cleanupSession?: () => Promise; }; /** diff --git a/vitest.config.mts b/vitest.config.mts index 5803445..2743c70 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -35,9 +35,9 @@ export default defineConfig({ // Auto-update the coverage thresholds when running locally. // Disabled in CI to prevent non-deterministic config changes. autoUpdate: !process.env.CI, - branches: 91.68, - functions: 94.76, - lines: 96.67, + branches: 91.7, + functions: 94.81, + lines: 96.68, statements: 96.44, }, }, From 05455cc0ef4ddcd129c572b6d1e9e1c40b74f51b Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Wed, 28 Oct 2026 22:44:01 +0000 Subject: [PATCH 3/5] chore: update changelog for server-owned disposal retries --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b865e90..0059e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals for retry. +- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. ## [0.12.0] From 30de39e64276bdc2c2481e7856ca6a8e48833a7a Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 13:16:38 +0000 Subject: [PATCH 4/5] fix: keep stale driver disposals from failing the current session --- CHANGELOG.md | 2 +- README.md | 2 +- src/server/cleanup-session.test.ts | 160 ++++++++++++++++++++++++- src/server/cleanup-session.ts | 98 +++++++++++++-- src/server/create-server.test.ts | 32 +++++ src/server/create-server.ts | 10 +- src/tools/cleanup.test.ts | 27 +++++ src/tools/launch.test.ts | 30 +++++ src/tools/launch.ts | 5 +- src/tools/test-utils/mock-factories.ts | 8 ++ vitest.config.mts | 8 +- 11 files changed, 360 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0059e2a..b5c193c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. +- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. Only the current session's cleanup and disposal can fail a call; drivers retained from earlier sessions are retried best-effort in parallel, logged when they fail again, and never fail the current cleanup or launch. ## [0.12.0] diff --git a/README.md b/README.md index a5ef8d9..080adb7 100644 --- a/README.md +++ b/README.md @@ -239,7 +239,7 @@ type ISessionManager = { ### Session-scoped device teardown -Core captures `sessionManager.getPlatformDriver?.()` before calling the consumer's `cleanup()`, then calls the driver's optional `dispose()` hook. Disposal runs even when consumer cleanup fails. This applies to `mm cleanup`, forced relaunch, failed launch, daemon shutdown, and server startup rollback. A failed disposal is retained for a later cleanup attempt. The daemon owns that retention (one `DriverLifecycle` per `createServer`), so a later `mm cleanup`, forced relaunch, or daemon stop retries it, whatever wrapper of the session manager a tool receives. Tools reach it through `ToolContext.cleanupSession()`; without it (a context built outside the daemon), the tools fall back to retention per session-manager object. +Core captures `sessionManager.getPlatformDriver?.()` before calling the consumer's `cleanup()`, then calls the driver's optional `dispose()` hook. Disposal runs even when consumer cleanup fails. This applies to `mm cleanup`, forced relaunch, failed launch, daemon shutdown, and server startup rollback. A failed disposal fails that cleanup call and is retained for a later attempt. The daemon owns that retention (one per `createServer` instance), so a later `mm cleanup`, forced relaunch, or daemon stop retries it, whatever wrapper of the session manager a tool receives. Only the current session's cleanup and disposal can fail a call. Drivers retained from earlier sessions are retried best-effort and in parallel. When they fail again, the daemon writes a `[WARN]` line to its log, and the current cleanup or launch continues. Tools reach this retention through `ToolContext.cleanupSession()`; without it (a context built outside the daemon), the tools fall back to retention per session-manager object. `MobilePlatformDriver.dispose()` delegates to its backend's optional `dispose()` hook. With a device-mcp version that implements private IDB companions, this stops only that backend's companion and recording. Older device-mcp versions remain compatible, but do not gain companion teardown. Adopt the device-mcp lifecycle release in the consumer after it is published; this change does not update the core dependency range. diff --git a/src/server/cleanup-session.test.ts b/src/server/cleanup-session.test.ts index 61a50e3..a5904c5 100644 --- a/src/server/cleanup-session.test.ts +++ b/src/server/cleanup-session.test.ts @@ -55,7 +55,8 @@ describe('cleanupSession', () => { expect(dispose).toHaveBeenCalledTimes(2); }); - it('does not dispose another session manager’s driver', async () => { + it('does not dispose another session manager’s live driver through a shared lifecycle', async () => { + const lifecycle = new DriverLifecycle(); const a = createMockSessionManager(); const b = createMockSessionManager(); const disposeA = vi.fn().mockResolvedValue(undefined); @@ -65,9 +66,15 @@ describe('cleanupSession', () => { b.getPlatformDriver = () => ({ dispose: disposeB }) as unknown as IPlatformDriver; - await cleanupSession(a); + expect(await lifecycle.cleanupSession(a)).toBe(true); expect(disposeA).toHaveBeenCalledTimes(1); expect(disposeB).not.toHaveBeenCalled(); + expect(b.cleanup).not.toHaveBeenCalled(); + + expect(await lifecycle.cleanupSession(b)).toBe(true); + expect(disposeA).toHaveBeenCalledTimes(1); + expect(disposeB).toHaveBeenCalledTimes(1); + expect(lifecycle.pendingCount).toBe(0); }); it('supports managers and drivers without disposal hooks', async () => { @@ -83,6 +90,8 @@ describe('cleanupSession', () => { ({ dispose }) as unknown as IPlatformDriver; vi.spyOn(manager, 'cleanup').mockRejectedValue(new Error('session failed')); await expect(cleanupSession(manager)).rejects.toMatchObject({ + message: + 'Session cleanup and driver disposal failed: session failed; dispose failed', errors: [ expect.objectContaining({ message: 'session failed' }), expect.objectContaining({ message: 'dispose failed' }), @@ -90,6 +99,153 @@ describe('cleanupSession', () => { }); }); + describe('drivers retained from earlier sessions', () => { + /** + * Build a manager whose driver is replaced by each test, and whose + * cleanup clears the driver like a real consumer does. + * + * @returns The manager and a setter for its current driver. + */ + function sessionWithSwappableDriver(): { + manager: ReturnType; + setDriver: (dispose: () => Promise) => void; + } { + let driver: IPlatformDriver | undefined; + const manager = createMockSessionManager(); + manager.getPlatformDriver = () => driver; + vi.spyOn(manager, 'cleanup').mockImplementation(async () => { + driver = undefined; + return true; + }); + return { + manager, + setDriver: (dispose) => { + driver = { dispose } as unknown as IPlatformDriver; + }, + }; + } + + it('retries a stuck older driver without failing later cleanups', async () => { + const onStaleDisposalError = vi.fn(); + const lifecycle = new DriverLifecycle({ onStaleDisposalError }); + const { manager, setDriver } = sessionWithSwappableDriver(); + const stuck = vi.fn().mockRejectedValue(new Error('stuck companion')); + setDriver(stuck); + + // The session that owns the stuck driver still reports its failure. + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + 'stuck companion', + ); + expect(lifecycle.pendingCount).toBe(1); + expect(onStaleDisposalError).not.toHaveBeenCalled(); + + for (let attempt = 1; attempt <= 2; attempt++) { + const healthy = vi.fn().mockResolvedValue(undefined); + setDriver(healthy); + expect(await lifecycle.cleanupSession(manager)).toBe(true); + expect(healthy).toHaveBeenCalledTimes(1); + expect(stuck).toHaveBeenCalledTimes(1 + attempt); + expect(onStaleDisposalError).toHaveBeenCalledTimes(attempt); + expect(lifecycle.pendingCount).toBe(1); + } + expect(onStaleDisposalError).toHaveBeenLastCalledWith( + expect.objectContaining({ message: 'stuck companion' }), + ); + }); + + it('drops an older driver from pending once its retry succeeds', async () => { + const lifecycle = new DriverLifecycle(); + const { manager, setDriver } = sessionWithSwappableDriver(); + const flaky = vi + .fn() + .mockRejectedValueOnce(new Error('stuck companion')) + .mockResolvedValue(undefined); + setDriver(flaky); + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + 'stuck companion', + ); + + setDriver(vi.fn().mockResolvedValue(undefined)); + expect(await lifecycle.cleanupSession(manager)).toBe(true); + expect(flaky).toHaveBeenCalledTimes(2); + expect(lifecycle.pendingCount).toBe(0); + + expect(await lifecycle.cleanupSession(manager)).toBe(true); + expect(flaky).toHaveBeenCalledTimes(2); + }); + + it('still fails the call for the current driver when an older driver is stuck', async () => { + const lifecycle = new DriverLifecycle(); + const { manager, setDriver } = sessionWithSwappableDriver(); + setDriver(vi.fn().mockRejectedValue(new Error('stuck companion'))); + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + 'stuck companion', + ); + + setDriver(vi.fn().mockRejectedValue(new Error('current dispose failed'))); + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + new Error('current dispose failed'), + ); + expect(lifecycle.pendingCount).toBe(2); + }); + + it('disposes a current driver that is also pending only once per call', async () => { + const lifecycle = new DriverLifecycle(); + const dispose = vi + .fn() + .mockRejectedValueOnce(new Error('dispose failed')) + .mockResolvedValue(undefined); + const driver = { dispose } as unknown as IPlatformDriver; + const manager = createMockSessionManager(); + // The consumer keeps exposing the same driver after a failed attempt. + manager.getPlatformDriver = () => driver; + + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + 'dispose failed', + ); + expect(await lifecycle.cleanupSession(manager)).toBe(true); + expect(dispose).toHaveBeenCalledTimes(2); + expect(lifecycle.pendingCount).toBe(0); + }); + + it('retries older drivers in parallel', async () => { + const lifecycle = new DriverLifecycle(); + const { manager, setDriver } = sessionWithSwappableDriver(); + const resolvers: (() => void)[] = []; + let deferRetries = false; + const olderDisposals = [1, 2].map(() => + vi.fn(async () => { + if (!deferRetries) { + throw new Error('stuck companion'); + } + await new Promise((resolve) => { + resolvers.push(resolve); + }); + }), + ); + for (const dispose of olderDisposals) { + setDriver(dispose); + await expect(lifecycle.cleanupSession(manager)).rejects.toThrowError( + 'stuck companion', + ); + } + expect(lifecycle.pendingCount).toBe(2); + olderDisposals.forEach((dispose) => dispose.mockClear()); + + deferRetries = true; + setDriver(vi.fn().mockResolvedValue(undefined)); + const pending = lifecycle.cleanupSession(manager); + await vi.waitFor(() => expect(resolvers).toHaveLength(2)); + // Both retries started before either one settled. + for (const dispose of olderDisposals) { + expect(dispose).toHaveBeenCalledTimes(1); + } + resolvers.forEach((resolve) => resolve()); + expect(await pending).toBe(true); + expect(lifecycle.pendingCount).toBe(0); + }); + }); + it('retains failed disposals across per-request wrappers of one manager', async () => { // The daemon hands each request a new wrapper (a Proxy on the stack) of // the same session manager. Retention must not depend on that identity. diff --git a/src/server/cleanup-session.ts b/src/server/cleanup-session.ts index 99202f4..87b51b5 100644 --- a/src/server/cleanup-session.ts +++ b/src/server/cleanup-session.ts @@ -1,5 +1,18 @@ import type { ISessionManager } from './session-manager.js'; import type { IPlatformDriver } from '../platform/types.js'; +import { debugWarn, extractErrorMessage } from '../utils'; + +/** + * Options for a {@link DriverLifecycle}. + */ +export type DriverLifecycleOptions = { + /** + * Called for each driver from an earlier session whose retried disposal + * fails again. These failures never fail the current call, so use this + * hook to make them visible (for example in the daemon log). + */ + onStaleDisposalError?: (error: unknown) => void; +}; /** * Owns the drivers whose disposal failed, so a later cleanup can retry them. @@ -10,10 +23,24 @@ import type { IPlatformDriver } from '../platform/types.js'; * same manager through different wrappers (for example a per-request proxy). * `createServer` owns one instance and uses it for the cleanup tool, forced * and failed launches, daemon stop and startup rollback. + * + * Only the current session decides whether a call fails. Drivers retained + * from earlier sessions are retried best-effort on every call: their failures + * are reported to `onStaleDisposalError` and never fail the current cleanup + * or launch. */ export class DriverLifecycle { readonly #pending = new Set(); + readonly #onStaleDisposalError: ((error: unknown) => void) | undefined; + + /** + * @param options - Hooks for retries of drivers from earlier sessions. + */ + constructor(options: DriverLifecycleOptions = {}) { + this.#onStaleDisposalError = options.onStaleDisposalError; + } + /** * @returns How many drivers still wait for a successful disposal. */ @@ -24,15 +51,22 @@ export class DriverLifecycle { /** * Close the app/session before releasing the driver resources it may still * use. Disposal also runs when consumer cleanup fails, including partial - * launches. Previously failed disposals are retried. + * launches. Drivers retained from earlier sessions are retried in parallel, + * best-effort. * * @param manager - The session manager whose resources should be released. * @returns Whether the consumer cleaned up a session. + * @throws The consumer cleanup error or the current driver's disposal + * error; an `AggregateError` when both fail. */ async cleanupSession(manager: ISessionManager): Promise { + // Read the driver first: consumers may clear it during cleanup. const driver = manager.getPlatformDriver?.(); - if (driver?.dispose) { - this.#pending.add(driver); + const current = driver?.dispose ? driver : undefined; + if (current) { + // If it is already pending from an earlier failed attempt, it is + // treated as current and is not retried again as a stale driver. + this.#pending.add(current); } const errors: unknown[] = []; @@ -42,25 +76,67 @@ export class DriverLifecycle { } catch (error) { errors.push(error); } - for (const pending of [...this.#pending]) { - try { - await pending.dispose?.(); - this.#pending.delete(pending); - } catch (error) { - errors.push(error); - } + + const stale = [...this.#pending].filter((pending) => pending !== current); + const [currentFailure] = await Promise.all([ + current ? this.#dispose(current) : undefined, + this.#retryStale(stale), + ]); + if (currentFailure) { + errors.push(currentFailure.error); } + if (errors.length === 1) { throw errors[0]; } if (errors.length > 1) { throw new AggregateError( errors, - 'Session cleanup and driver disposal failed', + `Session cleanup and driver disposal failed: ${errors + .map(extractErrorMessage) + .join('; ')}`, ); } return cleanedUp; } + + /** + * Dispose one driver and drop it from the pending set on success. + * + * @param driver - The driver to dispose. + * @returns The failure, or undefined when disposal succeeded. + */ + async #dispose( + driver: IPlatformDriver, + ): Promise<{ error: unknown } | undefined> { + try { + await driver.dispose?.(); + this.#pending.delete(driver); + return undefined; + } catch (error) { + return { error }; + } + } + + /** + * Retry drivers from earlier sessions in parallel. Failures are logged and + * reported, but never thrown. + * + * @param drivers - The retained drivers to retry. + */ + async #retryStale(drivers: IPlatformDriver[]): Promise { + const results = await Promise.allSettled( + drivers.map(async (driver) => driver.dispose?.()), + ); + results.forEach((result, index) => { + if (result.status === 'fulfilled') { + this.#pending.delete(drivers[index]); + return; + } + debugWarn('driver-lifecycle.retry', result.reason); + this.#onStaleDisposalError?.(result.reason); + }); + } } // Fallback for callers without a server-owned lifecycle (for example a diff --git a/src/server/create-server.test.ts b/src/server/create-server.test.ts index 27e6a40..91beb36 100644 --- a/src/server/create-server.test.ts +++ b/src/server/create-server.test.ts @@ -935,6 +935,38 @@ describe('createServer integration', () => { await customServer.stop(); expect(dispose).toHaveBeenCalledTimes(2); }); + + it('keeps /tool/cleanup healthy and logs on stop while an older driver stays stuck', async () => { + await server.stop(); + const { mockSM, dispose } = flakyDriverSession(); + dispose.mockReset(); + dispose.mockRejectedValue(new Error('stuck companion')); + const logPath = path.join(tmpDir, 'stuck-driver.log'); + const customServer = createServer( + buildConfig({ + sessionManager: mockSM as unknown as ServerConfig['sessionManager'], + logFilePath: logPath, + }), + ); + const started = await customServer.start(); + expect(await postCleanup(started.port)).toBe(500); + expect(await postCleanup(started.port)).toBe(200); + expect(dispose).toHaveBeenCalledTimes(2); + + await customServer.stop(); + expect(dispose).toHaveBeenCalledTimes(3); + await vi.waitFor(async () => { + const log = await fs.readFile(logPath, 'utf-8'); + expect(log).toContain( + '[WARN] Retained driver disposal failed again: stuck companion', + ); + expect(log).toContain('[INFO] Daemon stopped'); + }); + const log = await fs.readFile(logPath, 'utf-8'); + // One warning per retry: the second /tool/cleanup and stop(). + expect(log.match(/\[WARN\] Retained driver disposal/gu)).toHaveLength(2); + expect(log).not.toContain('[ERROR] Cleanup failed'); + }); }); describe('POST /tool/:name input validation', () => { diff --git a/src/server/create-server.ts b/src/server/create-server.ts index 4a36937..2449bcf 100644 --- a/src/server/create-server.ts +++ b/src/server/create-server.ts @@ -261,7 +261,15 @@ export function createServer(config: ServerConfig): ServerInstance { const knowledgeStore = config.knowledgeStore ?? new KnowledgeStore(); // One owner for driver disposal retries across tools, stop() and rollback. // Keyed by this server, not by the session manager object a caller sees. - const driverLifecycle = new DriverLifecycle(); + // Retries of drivers from earlier sessions never fail the current call, so + // log them here to keep repeated failures visible. + const driverLifecycle = new DriverLifecycle({ + onStaleDisposalError: (error): void => + appendLog( + config.logFilePath, + `[WARN] Retained driver disposal failed again: ${extractErrorMessage(error)}`, + ), + }); let httpServer: http.Server | null = null; let worktreeRoot = ''; diff --git a/src/tools/cleanup.test.ts b/src/tools/cleanup.test.ts index 6ca0e33..f614a86 100644 --- a/src/tools/cleanup.test.ts +++ b/src/tools/cleanup.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { cleanupTool } from './cleanup.js'; import { createMockSessionManager } from './test-utils/mock-factories.js'; +import { DriverLifecycle } from '../server/cleanup-session.js'; import type { ToolContext } from '../types/http.js'; function createMockContext(hasActive = false): ToolContext { @@ -92,4 +93,30 @@ describe('cleanupTool', () => { expect(context.sessionManager.cleanup).not.toHaveBeenCalled(); expect(result).toStrictEqual({ ok: true, result: { cleanedUp: true } }); }); + + it('succeeds while a stuck driver from an earlier session keeps failing', async () => { + const context = createMockContext(true); + const lifecycle = new DriverLifecycle(); + const { sessionManager } = context; + const stuck = vi.fn().mockRejectedValue(new Error('stuck companion')); + sessionManager.getPlatformDriver = () => + ({ dispose: stuck }) as unknown as NonNullable; + const serverContext: ToolContext = { + ...context, + cleanupSession: async () => lifecycle.cleanupSession(sessionManager), + }; + await expect(cleanupTool({}, serverContext)).rejects.toThrowError( + 'stuck companion', + ); + + const healthy = vi.fn().mockResolvedValue(undefined); + sessionManager.getPlatformDriver = () => + ({ dispose: healthy }) as unknown as NonNullable; + const result = await cleanupTool({}, serverContext); + + expect(result).toStrictEqual({ ok: true, result: { cleanedUp: true } }); + expect(healthy).toHaveBeenCalledTimes(1); + expect(stuck).toHaveBeenCalledTimes(2); + expect(lifecycle.pendingCount).toBe(1); + }); }); diff --git a/src/tools/launch.test.ts b/src/tools/launch.test.ts index 7ea31a8..714a56c 100644 --- a/src/tools/launch.test.ts +++ b/src/tools/launch.test.ts @@ -9,6 +9,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { launchTool } from './launch.js'; import type { LaunchInput } from './types'; import type { ExtensionState } from '../capabilities/types.js'; +import { DriverLifecycle } from '../server/cleanup-session.js'; import type { SessionLaunchResult } from '../server/session-manager.js'; import { createMockSessionManager } from './test-utils/mock-factories.js'; import { ErrorCodes } from './types/errors.js'; @@ -233,6 +234,35 @@ describe('launchTool', () => { ); expect(context.sessionManager.launch).toHaveBeenCalled(); }); + + it('relaunches with force while a stuck driver from an earlier session keeps failing', async () => { + const context = createMockContext({ hasActive: true }); + const lifecycle = new DriverLifecycle(); + const { sessionManager } = context; + const stuck = vi.fn().mockRejectedValue(new Error('stuck companion')); + sessionManager.getPlatformDriver = () => + ({ dispose: stuck }) as unknown as NonNullable; + const serverContext: ToolContext = { + ...context, + cleanupSession: async () => lifecycle.cleanupSession(sessionManager), + }; + await expect( + lifecycle.cleanupSession(sessionManager), + ).rejects.toThrowError('stuck companion'); + + const healthy = vi.fn().mockResolvedValue(undefined); + sessionManager.getPlatformDriver = () => + ({ dispose: healthy }) as unknown as NonNullable; + const result = await launchTool( + { stateMode: 'default', force: true }, + serverContext, + ); + + expect(result.ok).toBe(true); + expect(sessionManager.launch).toHaveBeenCalledTimes(1); + expect(healthy).toHaveBeenCalledTimes(1); + expect(stuck).toHaveBeenCalledTimes(2); + }); }); describe('launch failures', () => { diff --git a/src/tools/launch.ts b/src/tools/launch.ts index 4ad5a67..7c0e2ed 100644 --- a/src/tools/launch.ts +++ b/src/tools/launch.ts @@ -4,7 +4,7 @@ import type { ErrorCode } from './types/errors.js'; import { createToolError, createToolSuccess } from './utils.js'; import { cleanupSession } from '../server/cleanup-session.js'; import type { ToolContext, ToolResponse } from '../types/http.js'; -import { extractErrorMessage } from '../utils'; +import { debugWarn, extractErrorMessage } from '../utils'; const PROD_MODE_PREREQUISITES: LaunchPrerequisite[] = [ { @@ -61,8 +61,9 @@ export async function launchTool( // Preserve the launch error after best-effort cleanup of a partial session. try { await cleanup(); - } catch { + } catch (rollbackError) { // The original launch error is the actionable failure. + debugWarn('launch.rollback', rollbackError); } throw error; }); diff --git a/src/tools/test-utils/mock-factories.ts b/src/tools/test-utils/mock-factories.ts index f19ee1c..54a8015 100644 --- a/src/tools/test-utils/mock-factories.ts +++ b/src/tools/test-utils/mock-factories.ts @@ -12,6 +12,7 @@ import { vi } from 'vitest'; import type { ExtensionState } from '../../capabilities/types.js'; import type { KnowledgeStore } from '../../knowledge-store/knowledge-store.js'; +import type { IPlatformDriver } from '../../platform/types.js'; import type { TrackedPage, SessionLaunchResult, @@ -132,6 +133,13 @@ export function createMockSessionManager( getContractSeedingCapability: vi.fn().mockReturnValue(undefined), getStateSnapshotCapability: vi.fn().mockReturnValue(undefined), + // Platform Driver + getPlatformDriver: vi + .fn<() => IPlatformDriver | undefined>() + .mockReturnValue(undefined) as + | (() => IPlatformDriver | undefined) + | undefined, + // Environment setWorkflowContext: vi.fn(), getEnvironmentMode: vi diff --git a/vitest.config.mts b/vitest.config.mts index 2743c70..b2465fd 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -35,10 +35,10 @@ export default defineConfig({ // Auto-update the coverage thresholds when running locally. // Disabled in CI to prevent non-deterministic config changes. autoUpdate: !process.env.CI, - branches: 91.7, - functions: 94.81, - lines: 96.68, - statements: 96.44, + branches: 91.73, + functions: 94.86, + lines: 96.69, + statements: 96.46, }, }, From f2cea9b646915497988a31b7cee3c12c79c259da Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 13:30:35 +0000 Subject: [PATCH 5/5] chore: revert changelog update --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5c193c..0059e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. Only the current session's cleanup and disposal can fail a call; drivers retained from earlier sessions are retried best-effort in parallel, logged when they fail again, and never fail the current cleanup or launch. +- Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. ## [0.12.0]