From 6b6bc0134334e090e45064f6d6349194b5abddf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 13:13:27 +0200 Subject: [PATCH 1/2] fix(daemon): report the device claim retained by a failed close When close cannot confirm the device was released, it deliberately keeps the advisory claim (handing an unconfirmed device to the next session would be worse) but deletes the session record on the next line regardless, leaving a claim naming a session the daemon no longer tracks with no trace. Emit a warn diagnostic naming the device key and session, mirroring the open path's existing rollbackNewSessionClaim handling. Retention policy is unchanged. --- .../__tests__/session-close-shutdown.test.ts | 158 ++++++++++++++++++ src/daemon/handlers/session-close.ts | 13 +- 2 files changed, 170 insertions(+), 1 deletion(-) diff --git a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts index 7c0a9f91b..7bca26a84 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,161 @@ 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 }); + } +}); + +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); From 037d4dc2b2cc252ef0053ea07c8bf5e4c9cad146 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 19:00:06 +0000 Subject: [PATCH 2/2] test(daemon): pin claim retention on the cleanup-failure branch too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retention decision reads `platformCloseError ?? cleanupAggregate`, and the existing pair only drove the first input. A best-effort cleanup failure — a wedged perfetto stop, a dead helper — is the branch operators hit more often and reaches the same retention through a different value, so narrowing the diagnostic to the platform-close branch left every existing test green. Verified red against exactly that: gating the emit on `platformCloseError` fails this test alone, 28 others unaffected. Live evidence for the device-facing path is still outstanding; this closes the untested residual the review named, not that requirement. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017Rva4YGtSCAKJqH5PbpcCU --- .../__tests__/session-close-shutdown.test.ts | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts index 7bca26a84..20ada7e21 100644 --- a/src/daemon/handlers/__tests__/session-close-shutdown.test.ts +++ b/src/daemon/handlers/__tests__/session-close-shutdown.test.ts @@ -1182,6 +1182,118 @@ test('a failed platform close retains the device claim and reports it', async () } }); +// 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;