diff --git a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts index 7c0a9f91b..20ada7e21 100644 --- a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts +++ b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts @@ -69,6 +69,9 @@ import { stopAndroidSnapshotHelperSessionForDevice } from '../../../platforms/an import { stopIosRunnerSession } from '../../../platforms/apple/core/runner/runner-client.ts'; import { WEB_DESKTOP_DEVICE } from '../../../__tests__/test-utils/index.ts'; import { setActiveProviderDeviceRuntimes } from '../../../provider-device-runtime.ts'; +import { acquireAdvisoryDeviceClaim } from '../../device-claims.ts'; +import { inspectDeviceClaims } from '../../device-claim-inspection.ts'; +import { flushDiagnosticsToSessionFile, withDiagnosticsScope } from '../../../utils/diagnostics.ts'; const mockShutdownSimulator = vi.mocked(shutdownSimulator); const mockRunCmd = vi.mocked(runCmd); @@ -1077,6 +1080,273 @@ test('targeted close preserves the platform-close AppError and still runs later expect(sessionStore.get(sessionName)).toBeUndefined(); }); +// #1478-adjacent (device-claim retention observability): a failed platform close deliberately +// keeps the advisory device claim (handing an unconfirmed device to the next session would be +// worse), but the session record is still deleted on the very next line. Before this pair of +// tests, nothing said so — the claim just quietly named a session `session list` no longer +// reported. These two tests pin both branches of that decision so a future refactor cannot +// silently invert either one. +test('a failed platform close retains the device claim and reports it', async () => { + const claimsRoot = mkdtempForTestSync('agent-device-session-close-claim-retained-'); + const previousClaimsDir = process.env.AGENT_DEVICE_CLAIMS_DIR; + process.env.AGENT_DEVICE_CLAIMS_DIR = claimsRoot; + try { + const sessionStore = makeSessionStore(); + const sessionName = 'targeted-close-claim-retained-session'; + const device = { + platform: 'apple' as const, + id: 'sim-udid-close-claim-retained', + name: 'iPhone 15', + kind: 'simulator' as const, + booted: true, + }; + const acquired = await acquireAdvisoryDeviceClaim({ + device, + session: sessionName, + workspace: process.cwd(), + stateDir: sessionStore.resolveDaemonStateDir(), + }); + if (!acquired.ownership) { + throw new Error('expected the test session to acquire a device claim'); + } + const session = { + ...makeSession(sessionName, device), + // Recording defeats runner retention so this mirrors the platform-close-error test above + // rather than exercising a different code path. + recording: { outPath: '/tmp/recording.mp4' }, + deviceClaim: acquired.ownership, + } as unknown as SessionState; + sessionStore.set(sessionName, session); + + const platformCloseError = new AppError('DEVICE_UNAVAILABLE', 'platform close failed', { + reason: 'device_disconnected', + hint: 'Reconnect the device and retry close.', + }); + mockDispatchCommand.mockRejectedValueOnce(platformCloseError); + + const diagnosticsLogPath = path.join(claimsRoot, 'diagnostics.ndjson'); + const thrown = await withDiagnosticsScope( + { session: sessionName, command: 'close', logPath: diagnosticsLogPath }, + async () => { + let caught: unknown; + try { + await handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: ['com.example.app'], + flags: {}, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }); + } catch (error) { + caught = error; + } + flushDiagnosticsToSessionFile({ force: true }); + return caught; + }, + ); + + expect(thrown).toBe(platformCloseError); + // The session is still deleted (deliberate, pre-existing behavior) even though the claim + // could not be confirmed released. + expect(sessionStore.get(sessionName)).toBeUndefined(); + + // The claim itself was NOT cleared: it is still live, still naming the deleted session. + const claimState = inspectDeviceClaims({ serial: device.id })[0]; + expect(claimState?.classification).toBe('live'); + expect(claimState?.claim?.session).toBe(sessionName); + + // A warn diagnostic names the retained claim's device key and owning session so the retention + // is observable instead of silent. + const rows = fs + .readFileSync(diagnosticsLogPath, 'utf8') + .trim() + .split('\n') + .map((line) => JSON.parse(line)); + expect(rows).toContainEqual( + expect.objectContaining({ + level: 'warn', + phase: 'device_claim_close_effects_unconfirmed', + data: { deviceKey: acquired.ownership.deviceKey, session: sessionName }, + }), + ); + } finally { + if (previousClaimsDir === undefined) delete process.env.AGENT_DEVICE_CLAIMS_DIR; + else process.env.AGENT_DEVICE_CLAIMS_DIR = previousClaimsDir; + fs.rmSync(claimsRoot, { recursive: true, force: true }); + } +}); + +// The retention decision has TWO inputs — `platformCloseError ?? cleanupAggregate` — and the test +// above only drives the first. A best-effort cleanup failure is the branch operators actually hit +// more often (a wedged perfetto stop, a dead helper), and it reaches the same retention through a +// different value, so it needs its own pin: making the aggregate stop blocking the claim would +// leave the test above green. +test('a failing best-effort cleanup also retains the device claim and reports it', async () => { + const claimsRoot = mkdtempForTestSync('agent-device-session-close-claim-cleanup-failure-'); + const previousClaimsDir = process.env.AGENT_DEVICE_CLAIMS_DIR; + process.env.AGENT_DEVICE_CLAIMS_DIR = claimsRoot; + try { + const sessionStore = makeSessionStore(); + const sessionName = 'close-claim-cleanup-failure-session'; + const device = { + platform: 'android' as const, + id: 'emulator-5556', + name: 'Pixel', + kind: 'emulator' as const, + booted: true, + }; + const acquired = await acquireAdvisoryDeviceClaim({ + device, + session: sessionName, + workspace: process.cwd(), + stateDir: sessionStore.resolveDaemonStateDir(), + }); + if (!acquired.ownership) { + throw new Error('expected the test session to acquire a device claim'); + } + const session = { + ...makeSession(sessionName, device), + appBundleId: 'com.example.app', + nativePerf: { + android: { + type: 'trace', + kind: 'perfetto', + packageName: 'com.example.app', + appPid: '1234', + profilerPid: '5678', + remotePath: '/data/misc/perfetto-traces/app.perfetto-trace', + outPath: '/tmp/app.perfetto-trace', + startedAt: Date.now(), + state: 'running', + }, + }, + deviceClaim: acquired.ownership, + } as unknown as SessionState; + sessionStore.set(sessionName, session); + + // The platform close itself succeeds; only the best-effort cleanup step fails, so the + // blocking error arrives as the cleanup aggregate rather than as platformCloseError. + mockCleanupAndroidNativePerfSession.mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'perfetto stop failed'), + ); + + const diagnosticsLogPath = path.join(claimsRoot, 'diagnostics.ndjson'); + const thrown = await withDiagnosticsScope( + { session: sessionName, command: 'close', logPath: diagnosticsLogPath }, + async () => { + let caught: unknown; + try { + await handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: {}, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }); + } catch (error) { + caught = error; + } + flushDiagnosticsToSessionFile({ force: true }); + return caught; + }, + ); + + expect(thrown).toMatchObject({ + details: expect.objectContaining({ + reason: 'session_cleanup_incomplete', + failedSteps: ['android_native_perf'], + }), + }); + expect(sessionStore.get(sessionName)).toBeUndefined(); + + const claimState = inspectDeviceClaims({ serial: device.id })[0]; + expect(claimState?.classification).toBe('live'); + expect(claimState?.claim?.session).toBe(sessionName); + + const rows = fs + .readFileSync(diagnosticsLogPath, 'utf8') + .trim() + .split('\n') + .map((line) => JSON.parse(line)); + expect(rows).toContainEqual( + expect.objectContaining({ + level: 'warn', + phase: 'device_claim_close_effects_unconfirmed', + data: { deviceKey: acquired.ownership.deviceKey, session: sessionName }, + }), + ); + } finally { + if (previousClaimsDir === undefined) delete process.env.AGENT_DEVICE_CLAIMS_DIR; + else process.env.AGENT_DEVICE_CLAIMS_DIR = previousClaimsDir; + fs.rmSync(claimsRoot, { recursive: true, force: true }); + } +}); + +test('a successful close clears the device claim', async () => { + const claimsRoot = mkdtempForTestSync('agent-device-session-close-claim-cleared-'); + const previousClaimsDir = process.env.AGENT_DEVICE_CLAIMS_DIR; + process.env.AGENT_DEVICE_CLAIMS_DIR = claimsRoot; + try { + const sessionStore = makeSessionStore(); + const sessionName = 'targeted-close-claim-cleared-session'; + const device = { + platform: 'android' as const, + id: 'emulator-5554', + name: 'Pixel', + kind: 'emulator' as const, + booted: true, + }; + const acquired = await acquireAdvisoryDeviceClaim({ + device, + session: sessionName, + workspace: process.cwd(), + stateDir: sessionStore.resolveDaemonStateDir(), + }); + if (!acquired.ownership) { + throw new Error('expected the test session to acquire a device claim'); + } + const session = { + ...makeSession(sessionName, device), + deviceClaim: acquired.ownership, + }; + sessionStore.set(sessionName, session); + + const response = await handleSessionCommands({ + req: { + token: 't', + session: sessionName, + command: 'close', + positionals: [], + flags: {}, + }, + sessionName, + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore, + invoke: noopInvoke, + }); + + expect(response?.ok).toBe(true); + expect(sessionStore.get(sessionName)).toBeUndefined(); + expect(inspectDeviceClaims({ serial: device.id })).toEqual([]); + } finally { + if (previousClaimsDir === undefined) delete process.env.AGENT_DEVICE_CLAIMS_DIR; + else process.env.AGENT_DEVICE_CLAIMS_DIR = previousClaimsDir; + fs.rmSync(claimsRoot, { recursive: true, force: true }); + } +}); + test('targeted close skips platform dispatch and preserves the error when the required pre-close runner stop fails', async () => { const sessionStore = makeSessionStore(); const sessionName = 'targeted-close-preclose-failure-session'; diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index f652fb844..8fe47a71c 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -406,7 +406,18 @@ async function runCloseTeardownAndRelease(params: { failures: cleanupFailures, }); const deviceClaimBlockingError = platformCloseError ?? cleanupAggregate; - if (!deviceClaimBlockingError) { + if (deviceClaimBlockingError) { + if (session.deviceClaim) { + emitDiagnostic({ + level: 'warn', + phase: 'device_claim_close_effects_unconfirmed', + data: { + deviceKey: session.deviceClaim.deviceKey, + session: sessionName, + }, + }); + } + } else { await clearAdvisoryDeviceClaim(session.deviceClaim); } sessionStore.delete(sessionName);