diff --git a/scripts/layering/session-state.ts b/scripts/layering/session-state.ts index 088628eab7..6b21c290f2 100644 --- a/scripts/layering/session-state.ts +++ b/scripts/layering/session-state.ts @@ -125,6 +125,9 @@ export const STORE_OWNED_SESSION_STATE_FIELDS: ReadonlySet = new Set([ 'audioProbe', 'createdAt', 'device', + // #2833: the request path reports session activity through `SessionStore.noteSessionActivity`, so + // the only writer of this field is the store that owns the record. + 'lastActivityAtMs', 'lastPerfProfile', 'name', 'recordOnlySession', diff --git a/src/cli/commands/__tests__/daemon.test.ts b/src/cli/commands/__tests__/daemon.test.ts index aab158d5f6..39ef7d8df4 100644 --- a/src/cli/commands/__tests__/daemon.test.ts +++ b/src/cli/commands/__tests__/daemon.test.ts @@ -33,6 +33,7 @@ const GRACEFUL_RESULT: DaemonStopResult = { claimsReleased: [], claimsOrphaned: [], claimsSuperseded: [], + claimsUnattributable: [], providerReleases: { status: 'completed', released: [], pending: [] }, warnings: [], }; @@ -77,7 +78,7 @@ test('merges a graceful shutdown report and cleans runner leases with the start- released: [{ leaseId: 'lease-1', provider: 'limrun' }], pending: [], }, - claims: { released: [claim], orphaned: [], superseded: [] }, + claims: { released: [claim], orphaned: [], superseded: [], unattributable: [] }, }); try { @@ -104,6 +105,7 @@ test('merges a graceful shutdown report and cleans runner leases with the start- claimsReleased: [claim], claimsOrphaned: [], claimsSuperseded: [], + claimsUnattributable: [], }), expect.any(Function), ); @@ -152,7 +154,7 @@ test('warns in text output when a graceful stop leaves an orphaned claim', async }; mocks.readDaemonShutdownReport.mockReturnValue({ providerReleases: { released: [], pending: [] }, - claims: { released: [], orphaned: [claim], superseded: [] }, + claims: { released: [], orphaned: [claim], superseded: [], unattributable: [] }, }); try { @@ -176,3 +178,45 @@ test('warns in text output when a graceful stop leaves an orphaned claim', async fs.rmSync(stateDir, { recursive: true, force: true }); } }); + +test('routes an unattributable claim to the default status view, never to --stale', async () => { + const stateDir = mkdtempForTestSync('agent-device-daemon-command-'); + mocks.readDaemonStopIdentity.mockReturnValue({ pid: 123, processStartTime: 'start-time' }); + mocks.stopDaemon.mockResolvedValue(GRACEFUL_RESULT); + const claim = { + deviceKey: 'local:android:none:emulator-5554', + session: 'default', + platform: 'android', + deviceId: 'emulator-5554', + }; + mocks.readDaemonShutdownReport.mockReturnValue({ + providerReleases: { released: [], pending: [] }, + claims: { released: [], orphaned: [], superseded: [], unattributable: [claim] }, + }); + + try { + await daemonCommand({ + positionals: ['stop'], + flags: { clean: false, help: false, json: false, stateDir, version: false }, + client: {} as never, + }); + + const [, data, renderHuman] = mocks.writeCommandOutput.mock.calls.at(-1) ?? []; + expect(data).toEqual( + expect.objectContaining({ + claimsUnattributable: [claim], + claimsOrphaned: [], + warnings: [expect.stringContaining('whose owner could not be read')], + }), + ); + const rendered = (renderHuman as () => string)(); + expect(rendered).toContain('Inspect with: agent-device device status.'); + // The whole reason this is its own bucket: both stale routes refuse a record with no recorded + // owner, so the warning may name them only to forbid them and must not end in a release. + expect(rendered).toContain('No device release route settles it'); + expect(rendered).not.toContain('then release with'); + expect(rendered).not.toContain('Inspect with: agent-device device status --stale'); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/src/cli/commands/daemon.ts b/src/cli/commands/daemon.ts index 75fea816b8..d21c515c26 100644 --- a/src/cli/commands/daemon.ts +++ b/src/cli/commands/daemon.ts @@ -46,10 +46,12 @@ function mergeShutdownReport( claimsReleased: report.claims.released, claimsOrphaned: report.claims.orphaned, claimsSuperseded: report.claims.superseded, + claimsUnattributable: report.claims.unattributable, warnings: [ ...stopped.warnings, ...supersededClaimWarnings(report.claims.superseded), ...orphanedClaimWarnings(report.claims.orphaned), + ...unattributableClaimWarnings(report.claims.unattributable), ], } : stopped; @@ -77,7 +79,11 @@ function supersededClaimWarnings(superseded: DaemonStopResult['claimsSuperseded' /** An orphaned claim keeps holding its device after the daemon is gone, and * only `device release --stale` or the next open settles it — say so instead - * of leaving the block discoverable through --json alone. */ + * of leaving the block discoverable through --json alone. + * + * A record whose owner could not be attributed is NOT reported here. Those are what + * `--stale` hides and refuses, so this sentence would send the operator to a route that cannot + * succeed; {@link unattributableClaimWarnings} names the view that does show them. */ function orphanedClaimWarnings(orphaned: DaemonStopResult['claimsOrphaned']): string[] { if (orphaned.length === 0) return []; const devices = orphaned.map((claim) => claim.deviceId).join(', '); @@ -86,6 +92,27 @@ function orphanedClaimWarnings(orphaned: DaemonStopResult['claimsOrphaned']): st ]; } +/** + * What the three provable buckets each get an exact remedy for, this one does not: settling a claim + * always rests on a proof about its owner, and this record names none. So the warning states only + * what is known, points at the view that lists these records — the default one, because `--stale` + * filters records without a decodable owner out — and does not promise a release that would clear + * them. `device release --stale` refuses every record that names no owner, and the next `open` refuses + * to overwrite one. The one principal that can clear an allocator-held record is the allocator that + * issued it, which is named on the refusal rather than guessed at here. + */ +function unattributableClaimWarnings( + unattributable: DaemonStopResult['claimsUnattributable'], +): string[] { + if (unattributable.length === 0) return []; + const devices = unattributable.map((claim) => claim.deviceId).join(', '); + return [ + `A claim for ${devices} remains whose owner could not be read, so this daemon cannot say whether those devices are free. ` + + 'No device release route settles it: --stale proves staleness from the recorded owner and refuses records that name none, ' + + 'and open refuses to overwrite them. Inspect with: agent-device device status.', + ]; +} + function renderDaemonStop( result: Pick & { clean: boolean; diff --git a/src/commands/schema/cli-help-topics.test.ts b/src/commands/schema/cli-help-topics.test.ts index 212235fd3e..7ac2d2c264 100644 --- a/src/commands/schema/cli-help-topics.test.ts +++ b/src/commands/schema/cli-help-topics.test.ts @@ -517,6 +517,7 @@ test('usageForCommand resolves physical-device help topic', async () => { ); assert.match(help, /AGENT_DEVICE_IOS_RUNNER_IDLE_STOP_MS/); assert.match(help, /AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS/); + assert.match(help, /AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS/); assert.match( help, /a stale iOS runner lease — its owner process dead, or its AGENT_DEVICE_STATE_DIR deleted — is reclaimed automatically/i, diff --git a/src/commands/schema/cli-help.ts b/src/commands/schema/cli-help.ts index c3fd791ac7..294f94a7a7 100644 --- a/src/commands/schema/cli-help.ts +++ b/src/commands/schema/cli-help.ts @@ -702,6 +702,7 @@ Runner and daemon lifecycle (applies to simulators too): No runner read launches a session app that is not running: snapshot, wait, is, get, a reading find, and an interaction's leading reads (a gesture's viewport read, the capture that resolves a selector click/fill) answer the retriable APP_NOT_RUNNING instead of bare-launching over a launch SpringBoard still holds behind its deep-link confirmation. Only open, activate, and a command that mutates without a leading read bring a stopped app up. close keeps a healthy iOS simulator XCTest runner warm by default so the next open on that simulator (same udid in the same simulator set) skips the runner build, unless --shutdown was requested, the session was recording, or the session held a device lease. A retained runner auto-stops after an idle window (default 5 minutes); set AGENT_DEVICE_IOS_RUNNER_IDLE_STOP_MS to override, or 0 to disable idle stop and retain until daemon exit. Each AGENT_DEVICE_STATE_DIR runs its own daemon. It self-exits after an idle window (default 5 minutes, matching the runner idle-stop default) once it has no open sessions, no in-flight requests, and no active recording; set AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS to override, or 0 to disable idle reap. + On a machine shared by several agents, set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to also expire an individual session that has taken a host-global device claim and then received no commands for that long; the claim is released and the next command on that session answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the device. Off by default, because on a shared host "idle" and "thinking" are indistinguishable: only set it where an abandoned claim blocking every other agent is the worse failure. Sessions holding a remote lease or any active capture (recording, logs, audio, performance, or trace) are never expired this way. A stale iOS runner lease — its owner process dead, or its AGENT_DEVICE_STATE_DIR deleted — is reclaimed automatically instead of failing with "is already owned by another agent-device daemon". A live owner's runner is also reclaimed when the requesting daemon holds the host-global device claim for that device: claims are exclusive, so holding one proves the runner's owner released the device and merely kept the runner warm. The error remains only for owners outside claim arbitration (a pre-claims build, or daemons pointed at different claim stores). For iOS SpringBoard, widget, or other system-UI surfaces, read agent-device help ios-system-ui.`, diff --git a/src/daemon/__tests__/application-lifecycle-recovery.test.ts b/src/daemon/__tests__/application-lifecycle-recovery.test.ts index db92d13f94..5f5fae6835 100644 --- a/src/daemon/__tests__/application-lifecycle-recovery.test.ts +++ b/src/daemon/__tests__/application-lifecycle-recovery.test.ts @@ -188,6 +188,30 @@ test('daemon lifecycle finalization admits facts once, binds once, and disposes expect(dispose).toHaveBeenCalledOnce(); }); +// #2833: an idle-session expiry is the one daemon-owned teardown with no successor to hand a healthy +// execution host to. Deferring runner termination to the gateway's shutdown phase would park the +// runner — and the device behind it — until process exit, which is the opposite of why the expiry ran. +test('a daemon that stays alive finalizes without the shutdown deferral', async () => { + const finalize = vi.fn(async () => {}); + const runtime = gateway({ finalize }); + + await finalizeDaemonSessionApplicationLifecycle({ + gateway: runtime.gateway, + scope: scope(), + session, + stateDir: '/state', + runtimeHints: {}, + daemonLeaving: false, + }); + + expect(finalize).toHaveBeenCalledWith({ + appBundleId: undefined, + surface: 'app', + retainRunner: false, + stateDir: '/state', + }); +}); + test.each([ { name: 'Android provider device', diff --git a/src/daemon/__tests__/daemon-shutdown-report.test.ts b/src/daemon/__tests__/daemon-shutdown-report.test.ts index 428fb642c0..c402122d0d 100644 --- a/src/daemon/__tests__/daemon-shutdown-report.test.ts +++ b/src/daemon/__tests__/daemon-shutdown-report.test.ts @@ -27,7 +27,7 @@ test('round-trips provider release and device claim records without lease creden try { writeDaemonShutdownReport(stateDir, { providerReleases: { released: [lease], pending: [lease] }, - claims: { released: [claim], orphaned: [], superseded: [claim] }, + claims: { released: [claim], orphaned: [], superseded: [claim], unattributable: [] }, }); expect(readDaemonShutdownReport(stateDir)).toEqual({ @@ -35,7 +35,7 @@ test('round-trips provider release and device claim records without lease creden released: [{ leaseId: lease.leaseId, provider: 'limrun' }], pending: [{ leaseId: lease.leaseId, provider: 'limrun' }], }, - claims: { released: [claim], orphaned: [], superseded: [claim] }, + claims: { released: [claim], orphaned: [], superseded: [claim], unattributable: [] }, }); } finally { fs.rmSync(stateDir, { recursive: true, force: true }); @@ -54,7 +54,7 @@ test('a report written before claim reporting still reads its provider releases' expect(readDaemonShutdownReport(stateDir)).toEqual({ providerReleases: { released: [], pending: [] }, - claims: { released: [], orphaned: [], superseded: [] }, + claims: { released: [], orphaned: [], superseded: [], unattributable: [] }, }); } finally { fs.rmSync(stateDir, { recursive: true, force: true }); @@ -82,3 +82,33 @@ test('ignores malformed shutdown reports and clear removes a prior report', () = fs.rmSync(stateDir, { recursive: true, force: true }); } }); + +test('a report written before unattributable claims were separated still reads its three buckets', () => { + const stateDir = mkdtempForTestSync('agent-device-shutdown-report-'); + const reportPath = path.join(stateDir, 'daemon-shutdown.json'); + const claim = { + deviceKey: 'local:android:none:emulator-5554', + session: 'default', + platform: 'android', + deviceId: 'emulator-5554', + }; + + try { + // The shape a previous daemon writes. `unattributable` is additive, so a reader of a report from + // before it existed must still get every bucket it can describe rather than dropping the section. + fs.writeFileSync( + reportPath, + JSON.stringify({ + providerReleases: { released: [], pending: [] }, + claims: { released: [claim], orphaned: [claim], superseded: [claim] }, + }), + ); + + expect(readDaemonShutdownReport(stateDir)).toEqual({ + providerReleases: { released: [], pending: [] }, + claims: { released: [claim], orphaned: [claim], superseded: [claim], unattributable: [] }, + }); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/src/daemon/__tests__/filesystem-boundary-faults.test.ts b/src/daemon/__tests__/filesystem-boundary-faults.test.ts index b6e31da3c4..73d06fe771 100644 --- a/src/daemon/__tests__/filesystem-boundary-faults.test.ts +++ b/src/daemon/__tests__/filesystem-boundary-faults.test.ts @@ -138,7 +138,7 @@ function createShutdownReportFixture(root: string): FilesystemBoundaryFixture { run: async () => writeDaemonShutdownReport(root, { providerReleases: { released: [], pending: [] }, - claims: { released: [], orphaned: [], superseded: [] }, + claims: { released: [], orphaned: [], superseded: [], unattributable: [] }, }), expected: 'return', }; diff --git a/src/daemon/__tests__/request-router-idle-expired.test.ts b/src/daemon/__tests__/request-router-idle-expired.test.ts new file mode 100644 index 0000000000..94c4f0e0ed --- /dev/null +++ b/src/daemon/__tests__/request-router-idle-expired.test.ts @@ -0,0 +1,147 @@ +import { createTestDeviceInventoryGateways } from '../../__tests__/test-utils/device-inventory-gateways.ts'; +/** + * #2833: when a command finds no session because this daemon already expired it for idleness, the + * answer stays `SESSION_NOT_FOUND` but carries the typed reason, the window it missed, and the device + * it released — never a bare "Run open first" with nothing to reason about. The repair tombstone is + * consulted first, so an abandoned repair transaction keeps its own, more specific guidance. + */ +import { test, expect } from 'vitest'; +import path from 'node:path'; +import fs from 'node:fs'; + +import { createRequestHandler } from './test-device-runtime-gateway.ts'; +import { buildIdleExpiryTombstone } from '../session-idle-expiry.ts'; +import type { DaemonRequest } from '../daemon-request.ts'; +import { LeaseRegistry } from '../lease-registry.ts'; +import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; +import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; + +function makeHandler(prefix: string) { + const sessionStore = makeSessionStore(prefix); + const handler = createRequestHandler({ + logPath: path.join(mkdtempForTestSync('daemon'), 'daemon.log'), + token: 'test-token', + sessionStore, + leaseRegistry: new LeaseRegistry(), + deviceInventoryGateways: createTestDeviceInventoryGateways(), + trackDownloadableArtifact: () => 'artifact-id', + }); + return { sessionStore, handler }; +} + +function closeRequest(session: string, tenant?: string): DaemonRequest { + return { + token: 'test-token', + session, + command: 'close', + positionals: [], + flags: {}, + ...(tenant ? { meta: { tenantId: tenant, sessionIsolation: 'tenant' as const } } : {}), + }; +} + +function writeIdleMarker(sessionStore: ReturnType, session: string) { + sessionStore.writeIdleExpiryTombstone( + session, + buildIdleExpiryTombstone(session, { + expiredAtMs: Date.now(), + idleExpiryMs: 1_500_000, + deviceKey: 'ios:sim-1', + }), + ); +} + +test('a command on an idle-expired session names the reason, window, and released device', async () => { + const { sessionStore, handler } = makeHandler('agent-device-router-idle-expired-'); + writeIdleMarker(sessionStore, 'idle-x'); + + const response = await handler(closeRequest('idle-x')); + + expect(response.ok).toBe(false); + if (response.ok) return; + // Still SESSION_NOT_FOUND: the session genuinely is gone. What changes is what it says why. + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + expect(response.error.details?.reason).toBe('SESSION_IDLE_EXPIRED'); + expect(response.error.details?.idleExpiryMs).toBe(1_500_000); + expect(response.error.details?.deviceKey).toBe('ios:sim-1'); + expect(response.error.hint).toMatch(/ios:sim-1/); +}); + +test('an expired session with no marker keeps the plain SESSION_NOT_FOUND', async () => { + const { handler } = makeHandler('agent-device-router-idle-no-marker-'); + const response = await handler(closeRequest('never-existed')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + expect(response.error.details?.reason).toBeUndefined(); +}); + +test('an expired marker that has aged out stops explaining the absence', async () => { + const { sessionStore, handler } = makeHandler('agent-device-router-idle-stale-'); + writeIdleMarker(sessionStore, 'idle-stale'); + const markerPath = path.join(sessionStore.resolveSessionDir('idle-stale'), 'idle-expiry.json'); + const marker = JSON.parse(fs.readFileSync(markerPath, 'utf8')) as { expiresAt: number }; + marker.expiresAt = Date.now() - 1; + fs.writeFileSync(markerPath, `${JSON.stringify(marker)}\n`); + + const response = await handler(closeRequest('idle-stale')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + expect(response.error.details?.reason).toBeUndefined(); +}); + +test('an abandoned repair transaction outranks the idle-expiry marker', async () => { + const { sessionStore, handler } = makeHandler('agent-device-router-idle-vs-repair-'); + writeIdleMarker(sessionStore, 'repair-x'); + sessionStore.writeRepairTombstone({ + name: 'repair-x', + device: { platform: 'apple', id: 'sim-1', name: 'iPhone', kind: 'simulator', booted: true }, + createdAt: Date.now(), + actions: [], + scriptPublication: { + kind: 'repair', + status: 'armed', + target: { kind: 'default', force: false }, + boundary: 0, + sourcePath: '/flows/login.ad', + }, + }); + + const response = await handler(closeRequest('repair-x')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('REPAIR_SESSION_EXPIRED'); +}); + +test('a tenant-isolated request reads the marker for its own scoped session', async () => { + const { sessionStore, handler } = makeHandler('agent-device-router-idle-tenant-'); + // The request names `idle-x` and the daemon owns it as `:idle-x`. Reading the raw name + // would miss the marker entirely and return a bare "Run open first". + writeIdleMarker(sessionStore, 'tenant-a:idle-x'); + + const response = await handler(closeRequest('idle-x', 'tenant-a')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + expect(response.error.details?.reason).toBe('SESSION_IDLE_EXPIRED'); + expect(response.error.details?.deviceKey).toBe('ios:sim-1'); +}); + +test('another tenant marker never explains this tenant absent session', async () => { + const { sessionStore, handler } = makeHandler('agent-device-router-idle-tenant-leak-'); + writeIdleMarker(sessionStore, 'tenant-b:idle-x'); + + const response = await handler(closeRequest('idle-x', 'tenant-a')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('SESSION_NOT_FOUND'); + // Reading the unscoped key would have reported tenant-b's device as the one tenant-a just lost. + expect(response.error.details?.reason).toBeUndefined(); + expect(response.error.details?.deviceKey).toBeUndefined(); +}); diff --git a/src/daemon/__tests__/session-idle-activity.test.ts b/src/daemon/__tests__/session-idle-activity.test.ts new file mode 100644 index 0000000000..02557fdbdf --- /dev/null +++ b/src/daemon/__tests__/session-idle-activity.test.ts @@ -0,0 +1,183 @@ +/** + * #2833: the request path reports session activity by finishing UNDER the session's execution lock — + * the same lock an expiry must take before settling. That is what makes the stamp and the verdict + * one clock: a session in use is never caught mid-command, and the deadline an expiry reads is the + * one the last finished command set. Commands that never take that lock (the registry's + * lock-exempt inventory surface) report nothing, so a bystander polling `devices` on a shared host + * cannot keep another agent's abandoned claim alive; and a request whose client hung up preserves + * nothing, exactly as a canceled request renews no lease (ADR 0007). + */ +import { afterAll, test, expect, vi } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { withDiagnosticsScope } from '@agent-device/host-kit/diagnostics'; +import { clearRequestCanceled, markRequestCanceled } from '@agent-device/host-kit/request'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; +import { LeaseRegistry } from '../lease-registry.ts'; +import { + existingSessionExecutionLockKeys, + resolveRequestExecutionLockPlan, +} from '../request-binding.ts'; +import { + createRequestExecutionScope, + getLeaseRegistryExecutionLocks, +} from '../request-execution-scope.ts'; +import { withKeyedLock } from '@agent-device/kernel/keyed-lock'; +import type { DaemonRequest } from '../daemon-request.ts'; +import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; + +const TEST_ROOT = mkdtempForTestSync('agent-device-session-idle-activity-'); +const LOG_PATH = path.join(TEST_ROOT, 'diagnostics.log'); + +afterAll(() => { + fs.rmSync(TEST_ROOT, { recursive: true, force: true }); +}); + +function makeRequest(overrides: Partial = {}): DaemonRequest { + return { + token: 'test-token', + session: 'default', + command: 'snapshot', + positionals: [], + ...overrides, + }; +} + +function storeWithSession() { + const sessionStore = makeSessionStore('agent-device-idle-activity-'); + sessionStore.set('default', makeIosSession('default')); + return sessionStore; +} + +test('a command that completes under the session lock stamps the session it ran on', async () => { + const sessionStore = storeWithSession(); + const scope = await withDiagnosticsScope({ command: 'snapshot', logPath: LOG_PATH }, () => + createRequestExecutionScope({ + req: makeRequest({ command: 'snapshot' }), + sessionStore, + leaseRegistry: new LeaseRegistry(), + }), + ); + + await scope.runLocked(async () => 'ran'); + + expect(sessionStore.get('default')?.lastActivityAtMs).toBeGreaterThan(0); +}); + +test('a lock-exempt inventory command reports no activity, even against a claim-holding session', async () => { + const sessionStore = storeWithSession(); + const scope = await withDiagnosticsScope({ command: 'devices', logPath: LOG_PATH }, () => + createRequestExecutionScope({ + req: makeRequest({ command: 'devices' }), + sessionStore, + leaseRegistry: new LeaseRegistry(), + }), + ); + + await scope.runLocked(async () => 'ran'); + + // `devices` resolves a session address only to locate its own artifacts. On the shared host this + // feature exists for, stamping there would let one agent's polling extend another's deadline. + expect(sessionStore.get('default')?.lastActivityAtMs).toBeUndefined(); +}); + +test('a command whose client hung up mid-flight preserves no activity', async () => { + const sessionStore = storeWithSession(); + const requestId = 'canceled-mid-command'; + const scope = await withDiagnosticsScope( + { command: 'snapshot', requestId, logPath: LOG_PATH }, + () => + createRequestExecutionScope({ + req: makeRequest({ command: 'snapshot', meta: { requestId } }), + sessionStore, + leaseRegistry: new LeaseRegistry(), + }), + ); + + // The request was admitted, then the client hung up while the handler worked. A handler that + // ignores its cancellation still reaches the stamp site — and must not renew anything there, the + // same rule that stops a canceled request renewing a remote lease (ADR 0007). An agent that + // timed out and left a hung handler is precisely what this deadline exists to catch. + try { + await scope.runLocked(async () => { + markRequestCanceled(requestId); + return 'ran to completion despite the disconnect'; + }); + expect(sessionStore.get('default')?.lastActivityAtMs).toBeUndefined(); + } finally { + clearRequestCanceled(requestId); + } +}); + +test('a command still running when the expiry arrives blocks it until the stamp lands', async () => { + const sessionStore = storeWithSession(); + const leaseRegistry = new LeaseRegistry(); + const locks = getLeaseRegistryExecutionLocks(leaseRegistry); + const scope = await withDiagnosticsScope({ command: 'snapshot', logPath: LOG_PATH }, () => + createRequestExecutionScope({ + req: makeRequest({ command: 'snapshot' }), + sessionStore, + leaseRegistry, + }), + ); + + let release!: () => void; + const running = new Promise((resolve) => { + release = resolve; + }); + const command = scope.runLocked(async () => { + await running; + return 'ran'; + }); + + // An expiry that starts while the command holds the lock cannot settle — and once the command + // finishes, its stamp is already visible to whoever finally holds the lock. + let settledFirst = true; + const expiry = withKeyedLock(locks, 'session:default', async () => { + settledFirst = sessionStore.get('default')?.lastActivityAtMs === undefined; + }); + + await new Promise((resolve) => setTimeout(resolve, 5)); + release(); + await Promise.all([command, expiry]); + + expect(settledFirst).toBe(false); +}); + +test('the request plan for an existing session keeps the canonical session-first lock pair', async () => { + const sessionStore = storeWithSession(); + const session = sessionStore.get('default')!; + const requestPlan = await resolveRequestExecutionLockPlan({ + req: makeRequest({ command: 'snapshot' }), + sessionName: 'default', + sessionStore, + }); + + // What this pins is the PLANNER: an existing session's request takes `existingSessionExecutionLockKeys` + // with the session first, which is the order a reaper must match to avoid deadlocking against it. The + // reaper's own pair is exercised against a live request in server/daemon-session-idle-expiry.test.ts; + // comparing this function with itself would prove nothing about that side. + expect(requestPlan.keys).toEqual(existingSessionExecutionLockKeys('default', session.device.id)); +}); + +test('the reaper never observes a mid-command session through the shared map', async () => { + const sessionStore = storeWithSession(); + const leaseRegistry = new LeaseRegistry(); + const scope = await withDiagnosticsScope({ command: 'snapshot', logPath: LOG_PATH }, () => + createRequestExecutionScope({ + req: makeRequest({ command: 'snapshot' }), + sessionStore, + leaseRegistry, + }), + ); + + const inside = vi.fn(); + await scope.runLocked(async () => { + inside(getLeaseRegistryExecutionLocks(leaseRegistry).has('session:default')); + return 'ran'; + }); + + expect(inside).toHaveBeenCalledWith(true); + expect(getLeaseRegistryExecutionLocks(leaseRegistry).has('session:default')).toBe(false); +}); diff --git a/src/daemon/__tests__/session-idle-expiry-harness.ts b/src/daemon/__tests__/session-idle-expiry-harness.ts new file mode 100644 index 0000000000..fbbd913eeb --- /dev/null +++ b/src/daemon/__tests__/session-idle-expiry-harness.ts @@ -0,0 +1,178 @@ +import { afterEach, assert } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { SessionStore } from '../session-store.ts'; +import type { SessionState } from '../session-state.ts'; +import type { createSessionIdleExpiry } from '../server/daemon-session-idle-expiry.ts'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import { + isolatedDeviceClaimStores, + retainOrphanedDeviceClaims, + type IsolatedDeviceClaimStore, +} from '../../__tests__/test-utils/device-claim-store.ts'; +import { acquireDeviceClaim, type DeviceClaimSessionOwnership } from '../device/device-claims.ts'; +import { resolveDeviceClaimPath } from '../device/device-claim-paths.ts'; +import { IOS_SIMULATOR } from '../../__tests__/test-utils/device-fixtures.ts'; +import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; + +/** The inactivity window every harness session is already past, except where a test says otherwise. */ +export const WINDOW_MS = 1_000; + +/** + * A wall-clock-scale base rather than a small synthetic one: the marker's own reader compares its + * `expiresAt` against the real clock, exactly as the repair tombstone reader does. + */ +export const NOW = Date.now(); + +/** A claim-shaped record for a session that only needs to be HOLDING something. */ +export const CLAIM = Object.freeze({ + deviceKey: 'ios:sim-1', + ownerToken: 'token-1', + ownerPid: 4242, + ownerStartTime: null, +}); + +export type IdleExpiryFixture = Readonly<{ + sessionStore: SessionStore; + /** This test's isolated claim store and daemon state dir, shared by the acquire and the sweep. */ + claims: IsolatedDeviceClaimStore; +}>; + +/** + * The claim and session scaffolding both idle-expiry test files need, with cleanup registered once. + * + * Claims are host-global by design, so reading and writing one has to be redirected away from the + * developer's `~/.agent-device`. Every sweep reaches `clearDeviceClaim`, so the store is redirected + * for every test rather than only the ones that inspect a claim. That path is resolved lazily, so + * the LAST store this harness hands out is the one the calling test reads and writes. + */ +export function createIdleExpiryHarness(): Readonly<{ + makeFixture: (prefix: string) => IdleExpiryFixture; + idleClaimedSession: (store: SessionStore, name?: string) => SessionState; + unclaimedExpiredSession: (name: string) => SessionState; + sessionWithLiveClaim: ( + fixture: IdleExpiryFixture, + name?: string, + ) => Promise>; + claimFileHeld: (deviceClaim: DeviceClaimSessionOwnership) => boolean; + /** + * Triggers a sweep and waits `ms` for nothing in particular. + * + * This is a BOUND on absence, not a wait for completion: use it only for the assertions that + * something did NOT happen, where there is no outcome to look for. Anything that asserts a settle + * landed goes through {@link waitFor} instead, because a fixed sleep here is a race the CI machine + * wins or loses, and this suite performs real filesystem work per settle. + */ + runUntilIdle: ( + controller: ReturnType, + ms: number, + ) => Promise; + /** + * Gives an expiry that is blocked on something a moment to produce an outcome it must NOT produce. + * + * A bounded wait on ABSENCE, which has no fact to poll for. It is honest only because the calling + * test then also awaits {@link createSweepBarrier.swept}: without that, the assertion below it would + * be reading a sweep that had simply never started. + */ + boundAbsence: (ms: number) => Promise; + /** Polls `probe` until it holds, so a test waits for the outcome it asserts rather than for time. */ + waitFor: (probe: () => boolean, description: string, timeoutMs?: number) => Promise; + /** + * A barrier on sweep COMPLETION, for the tests that configure no settle budget. + * + * The composition scope wraps a sweep and awaits it, and a sweep with no budget awaits every + * session's settle to the end, so a scope whose `run()` resolved is a sweep whose work is finished. + * Passing this as `withinDiagnosticsScope` and then awaiting {@link createSweepBarrier.swept} + * replaces a fixed sleep with the fact itself: the assertions that follow cannot be reading a + * half-finished settle, and `sweeps` says a sweep really ran rather than merely never settled. + */ + createSweepBarrier: () => Readonly<{ + withinDiagnosticsScope: (run: () => Promise) => Promise; + swept: (atLeast?: number) => Promise; + sweeps: () => number; + }>; +}> { + const claimStores = isolatedDeviceClaimStores('agent-device-idle-expiry-claim-'); + // Registers the cleanup hook at collection time; the per-test calls below share its root list. + claimStores(); + + const roots: string[] = []; + afterEach(() => { + for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); + }); + + return { + makeFixture: (prefix) => { + const root = mkdtempForTestSync(prefix); + roots.push(root); + return { sessionStore: new SessionStore(path.join(root, 'sessions')), claims: claimStores() }; + }, + idleClaimedSession: (store, name = 'default') => { + const session = makeIosSession(name, { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: { ...CLAIM }, + }); + store.set(name, session); + return session; + }, + // Past its window on time alone, and holding no claim — nothing another agent waits on. + unclaimedExpiredSession: (name) => makeIosSession(name, { createdAt: NOW - WINDOW_MS - 1 }), + // Stands a real, currently-held claim and returns the session sitting on it. A fabricated token + // would make the release a no-op that reports `ownership-changed`, which is precisely the outcome + // that must not read as a device freed. + sessionWithLiveClaim: async (fixture, name = 'default') => { + const { stateDir } = fixture.claims; + const acquired = await acquireDeviceClaim({ + device: IOS_SIMULATOR, + session: name, + workspace: stateDir, + stateDir, + reconcileOrphanedDeviceClaim: retainOrphanedDeviceClaims, + }); + if (acquired.status !== 'acquired') { + throw new Error(`expected an acquired claim, got ${acquired.status}`); + } + const session = makeIosSession(name, { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: acquired.ownership, + }); + fixture.sessionStore.set(name, session); + return { session, deviceClaim: acquired.ownership }; + }, + claimFileHeld: (deviceClaim) => fs.existsSync(resolveDeviceClaimPath(deviceClaim.deviceKey)), + runUntilIdle: (controller, ms) => { + controller.noteSessionsChanged(); + return new Promise((resolve) => setTimeout(resolve, ms)); + }, + createSweepBarrier: () => { + let completed = 0; + let wake: (() => void) | undefined; + return { + withinDiagnosticsScope: async (run) => { + await run(); + completed++; + wake?.(); + }, + swept: async (atLeast = 1) => { + while (completed < atLeast) { + await new Promise((resolve) => { + wake = resolve; + }); + } + }, + sweeps: () => completed, + }; + }, + boundAbsence: (ms) => new Promise((resolve) => setTimeout(resolve, ms)), + waitFor: async (probe, description, timeoutMs = 2_000) => { + const deadline = Date.now() + timeoutMs; + for (;;) { + if (probe()) return; + if (Date.now() >= deadline) { + assert.fail(`timed out after ${timeoutMs}ms waiting for ${description}`); + } + await new Promise((resolve) => setTimeout(resolve, 2)); + } + }, + }; +} diff --git a/src/daemon/__tests__/session-idle-expiry.test.ts b/src/daemon/__tests__/session-idle-expiry.test.ts new file mode 100644 index 0000000000..de013d4ffa --- /dev/null +++ b/src/daemon/__tests__/session-idle-expiry.test.ts @@ -0,0 +1,176 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { + SESSION_IDLE_EXPIRY_ENV, + buildIdleExpiryTombstone, + idleDeadlineExceeded, + isIdleExpirableSession, + isSessionIdleExpired, + lastActivityMs, + resolveSessionIdleExpiryMs, + sessionIdleDeadlineMs, + sessionIdleExpiredError, +} from '../session-idle-expiry.ts'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import { makeTestScreenRecordingResource } from '../../__tests__/test-utils/screen-recording-live-handle.ts'; +import { createDurableResourceEnvelope } from '@agent-device/capture-kit'; +import { localRuntimeOwner } from '@agent-device/contracts/platform-runtime'; +import { deviceIdentity } from '@agent-device/kernel/device'; +import type { DurableResourceEnvelope } from '@agent-device/contracts/durable-resource-envelope'; +import type { SessionState } from '../session-state.ts'; + +const CLAIM = { + deviceKey: 'ios:sim-1', + ownerToken: 'token-1', + ownerPid: 4242, + ownerStartTime: null, +} as const; + +function claimHoldingSession(overrides: Parameters[1] = {}) { + return makeIosSession('default', { deviceClaim: { ...CLAIM }, ...overrides }); +} + +test('resolveSessionIdleExpiryMs is off unless the env names a positive number', () => { + assert.equal(resolveSessionIdleExpiryMs({}), 0); + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: '' }), 0); + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: '0' }), 0); + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: '-1' }), 0); + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: 'not-a-number' }), 0); + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: '1500000' }), 1_500_000); + // A fractional window is a real reading, and flooring keeps it positive rather than NaN. + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: ' 1500.7 ' }), 1500); + // Flooring must never round a positive opt-in into `0`, which is the other thing this env means: + // a smaller unit than milliseconds is a request for the shortest window, not for off. + assert.equal(resolveSessionIdleExpiryMs({ [SESSION_IDLE_EXPIRY_ENV]: '0.4' }), 1); +}); + +test('lastActivityMs falls back to createdAt so an abandoned open still expires', () => { + const session = claimHoldingSession({ createdAt: 1_000 }); + assert.equal(lastActivityMs(session), 1_000); + session.lastActivityAtMs = 5_000; + assert.equal(lastActivityMs(session), 5_000); +}); + +test('only a claim-holding, unleased, unrecording session is idle-expirable', () => { + // No claim: the session holds nothing another agent waits on. + assert.equal(isIdleExpirableSession(makeIosSession('default')), false); + // A remote lease owns this session's ownership and already has its own inactivity TTL (ADR 0007). + assert.equal( + isIdleExpirableSession( + claimHoldingSession({ + lease: { leaseId: 'lease-1', tenantId: 'tenant-1', runId: 'run-1', expiresAt: 1 }, + }), + ), + false, + ); + // A live recording is use happening on the device with no command in flight, which is exactly the + // evidence the daemon-process reap also honors. + const recording = claimHoldingSession(); + recording.screenRecording = makeTestScreenRecordingResource(recording); + assert.equal(isIdleExpirableSession(recording), false); + assert.equal(isIdleExpirableSession(claimHoldingSession()), true); +}); + +test('idleDeadlineExceeded measures from the last activity and never fires while off', () => { + const session = claimHoldingSession({ createdAt: 0, lastActivityAtMs: 1_000 }); + assert.equal(idleDeadlineExceeded(session, 0, 10_000), false); + assert.equal(idleDeadlineExceeded(session, 5_000, 5_999), false); + assert.equal(idleDeadlineExceeded(session, 5_000, 6_000), true); +}); + +test('isSessionIdleExpired refuses a session the policy may not touch even past its deadline', () => { + const expiredInTime = claimHoldingSession({ createdAt: 0 }); + assert.equal(isSessionIdleExpired(expiredInTime, 1_000, 5_000), true); + assert.equal( + isSessionIdleExpired(makeIosSession('default', { createdAt: 0 }), 1_000, 5_000), + false, + ); +}); + +test('sessionIdleDeadlineMs arms nothing for a session that is not expirable', () => { + assert.equal(sessionIdleDeadlineMs(claimHoldingSession({ createdAt: 1_000 }), 5_000), 6_000); + assert.equal(sessionIdleDeadlineMs(makeIosSession('default'), 5_000), undefined); + assert.equal(sessionIdleDeadlineMs(claimHoldingSession(), 0), undefined); +}); + +test('the expiry error keeps SESSION_NOT_FOUND and carries the reason, window, and device', () => { + const tombstone = buildIdleExpiryTombstone('cwd:abc:default', { + expiredAtMs: 1_000, + idleExpiryMs: 1_500_000, + deviceKey: 'ios:sim-1', + }); + // The hour is asserted as a value, not imported from the module: the marker's life is a promise to + // the agent that comes next, and a test that re-derives it from the code would accept any TTL. + assert.equal(tombstone.expiresAt, 1_000 + 60 * 60_000); + + const error = sessionIdleExpiredError('cwd:abc:default', tombstone, 61_000); + assert.equal(error.code, 'SESSION_NOT_FOUND'); + assert.equal(error.details?.reason, 'SESSION_IDLE_EXPIRED'); + assert.equal(error.details?.idleExpiryMs, 1_500_000); + assert.equal(error.details?.deviceKey, 'ios:sim-1'); + assert.equal(error.details?.session, 'cwd:abc:default'); + // The hint names the device the caller can now re-claim, and how to stop this happening again. + assert.match(String(error.details?.hint), /ios:sim-1/); + assert.match(String(error.details?.hint), new RegExp(SESSION_IDLE_EXPIRY_ENV)); +}); + +test('an expiry marker without a device still explains itself without one', () => { + const tombstone = buildIdleExpiryTombstone('default', { + expiredAtMs: 1_000, + idleExpiryMs: 5_000, + }); + assert.equal('deviceKey' in tombstone, false); + const error = sessionIdleExpiredError('default', tombstone, 6_000); + assert.equal(error.details?.deviceKey, undefined); + assert.match(String(error.details?.hint), new RegExp(SESSION_IDLE_EXPIRY_ENV)); +}); + +/** + * A capture handle and its envelope, built once for every capture kind this policy has to respect. The + * idle-expiry policy asks only whether a capture is attached, so this handle never has to do anything + * and the three handle shapes are cast rather than reproduced; the envelope is real because the + * session field's type demands it. + */ +function makeTestCaptureResource( + session: SessionState, + resourceKind: K, +): { handle: never; envelope: DurableResourceEnvelope } { + const handle = { + inspect: () => ({}), + finish: async () => ({ status: 'completed' as const, result: {} }), + forceCleanup: async () => ({ status: 'cleaned' as const }), + setOutputPath: () => {}, + [Symbol.asyncDispose]: async () => {}, + }; + return { + handle: handle as never, + envelope: createDurableResourceEnvelope({ + resourceKind, + sessionId: session.name, + device: deviceIdentity(session.device), + owner: localRuntimeOwner(session.device.platform), + fence: { token: `test-${resourceKind}`, generation: 1 }, + lifecycle: 'open', + descriptor: { version: 1, body: { fixture: true } }, + }), + }; +} + +test('a session with any capture running is never idle-expired', () => { + // Every one of these stamps the session once and then goes silent while the capture runs, so a + // deadline measured from commands alone would call the session idle and destroy evidence its own + // workflow is still collecting. Deleting any single guard below must fail this test. + const running: Array<[string, (session: SessionState) => void]> = [ + ['screen recording', (s) => (s.screenRecording = makeTestScreenRecordingResource(s))], + ['app logs', (s) => (s.appLog = makeTestCaptureResource(s, 'app-log'))], + ['audio probe', (s) => (s.audioProbe = makeTestCaptureResource(s, 'audio-probe'))], + ['performance capture', (s) => (s.perfCapture = makeTestCaptureResource(s, 'perf-capture'))], + ['trace', (s) => (s.trace = { outPath: '/tmp/trace.trace', startedAt: 1 })], + ]; + for (const [name, attach] of running) { + const session = claimHoldingSession(); + attach(session); + assert.equal(isIdleExpirableSession(session), false, `${name} must hold the session back`); + } + assert.equal(isIdleExpirableSession(claimHoldingSession()), true); +}); diff --git a/src/daemon/__tests__/session-store.test.ts b/src/daemon/__tests__/session-store.test.ts index acc68faf14..05fe386324 100644 --- a/src/daemon/__tests__/session-store.test.ts +++ b/src/daemon/__tests__/session-store.test.ts @@ -880,3 +880,72 @@ test('BLOCKER 3: finalizeRepairTeardown auto-commit records a terminal close, pr const bareRefs = parsed.actions.flatMap((a) => a.positionals.filter((p) => p.startsWith('@'))); assert.deepEqual(bareRefs, []); }); + +// #2833: the store owns the activity signal the opt-in inactivity deadline is measured from, and the +// bounded marker an expired session leaves. Both are the store's, so the request path never becomes a +// `SessionState` writer to report either one. + +test('noteSessionActivity stamps the live record and ignores an unknown address', () => { + const { store, session } = makeFixture('agent-device-store-note-activity-'); + store.set('default', session); + assert.equal(session.lastActivityAtMs, undefined); + + store.noteSessionActivity('default', 5_000); + assert.equal(session.lastActivityAtMs, 5_000); + + // A session this very command is creating is not what the command was using when it started. + store.noteSessionActivity('absent', 9_000); +}); + +test('the idle-expiry marker round-trips, and a fresh open clears it', () => { + const { store } = makeFixture('agent-device-store-idle-marker-'); + const marker = { + owner: 'default', + expiredAtMs: Date.now(), + expiresAt: Date.now() + 60_000, + idleExpiryMs: 1_500_000, + deviceKey: 'ios:sim-1', + }; + + assert.equal(store.readIdleExpiryTombstone('default'), undefined); + store.writeIdleExpiryTombstone('default', marker); + assert.deepEqual(store.readIdleExpiryTombstone('default'), marker); + + store.clearIdleExpiryTombstone('default'); + assert.equal(store.readIdleExpiryTombstone('default'), undefined); + // Clearing a marker that was never there is not a failure: the caller is a successful `open`. + store.clearIdleExpiryTombstone('default'); +}); + +test('an idle-expiry marker for an unsafe session name is neither written nor read', () => { + const { store } = makeFixture('agent-device-store-idle-marker-unsafe-'); + store.writeIdleExpiryTombstone('..', { + owner: '..', + expiredAtMs: Date.now(), + expiresAt: Date.now() + 60_000, + idleExpiryMs: 1_000, + }); + // The read is total, so its own answer proves nothing about where the write went: it declines `..` + // before touching the filesystem either way. The thing being guarded is the DIRECTORY ABOVE the + // sessions tree, so that is the path that has to be asserted empty. + assert.equal(store.readIdleExpiryTombstone('..'), undefined); + assert.equal(fs.existsSync(path.join(store.resolveDaemonStateDir(), 'idle-expiry.json')), false); +}); + +test('an idle-expiry marker never explains a different session key sharing its directory', () => { + const { store } = makeFixture('agent-device-store-idle-marker-owner-'); + const marker = { + owner: 'ws/a', + expiredAtMs: Date.now(), + expiresAt: Date.now() + 60_000, + idleExpiryMs: 60_000, + deviceKey: 'ios:sim-shared', + }; + // `safeSessionName` maps both of these names onto one session directory — it is the same encoding + // every other session artifact shares — which is why the marker also records the key that wrote it. + // Without that check, expiring `ws/a` would tell `ws:a`'s next command that IT had been expired, + // quoting another session's window and another session's device as the one this caller just lost. + store.writeIdleExpiryTombstone('ws/a', marker); + assert.equal(store.readIdleExpiryTombstone('ws:a'), undefined); + assert.deepEqual(store.readIdleExpiryTombstone('ws/a'), marker); +}); diff --git a/src/daemon/application-lifecycle-recovery.ts b/src/daemon/application-lifecycle-recovery.ts index 402ba9e0fe..4a15e6addf 100644 --- a/src/daemon/application-lifecycle-recovery.ts +++ b/src/daemon/application-lifecycle-recovery.ts @@ -65,9 +65,15 @@ export async function finalizeBoundSessionApplicationLifecycle(params: { } /** - * Finish a persisted session's lifecycle-owned resources during daemon shutdown. The selected - * owner is admitted from eager facts, then bound through the provider-first gateway exactly once. - * A platform finalization failure remains primary over binding disposal failure. + * Finish a persisted session's lifecycle-owned resources when the daemon, not a request, owns the + * teardown. The selected owner is admitted from eager facts, then bound through the provider-first + * gateway exactly once. A platform finalization failure remains primary over binding disposal failure. + * + * `daemonLeaving` is the one axis that decides what happens to a healthy execution host, and it + * defaults to `true` because every existing caller is the daemon on its way out, which hands a + * healthy host to the gateway's final shutdown phase. A daemon that stays alive — the #2833 + * idle-session expiry — has no successor and passes `false`, taking the ordinary-close path that + * stops the host and releases its lease instead of parking it until process exit. */ export async function finalizeDaemonSessionApplicationLifecycle(params: { gateway: DeviceRuntimeGateway; @@ -75,6 +81,7 @@ export async function finalizeDaemonSessionApplicationLifecycle(params: { session: SessionState; stateDir: string; runtimeHints: RuntimeHintValues; + daemonLeaving?: boolean; }): Promise { const { gateway, scope, session, stateDir, runtimeHints } = params; // Bound lazily so an unsupported cell still fails on facts alone, without allocating an owner. @@ -89,7 +96,7 @@ export async function finalizeDaemonSessionApplicationLifecycle(params: { session, stateDir, runtimeHints, - daemonShutdown: true, + ...(params.daemonLeaving === false ? {} : { daemonShutdown: true }), }); } catch (operationError) { try { diff --git a/src/daemon/daemon-shutdown-report.ts b/src/daemon/daemon-shutdown-report.ts index 2ec19ce684..65b389ed1b 100644 --- a/src/daemon/daemon-shutdown-report.ts +++ b/src/daemon/daemon-shutdown-report.ts @@ -16,7 +16,9 @@ export type ProviderReleaseRecord = { * terminal state; `orphaned` means teardown left it in place, so the exiting * daemon's dead owner identity is what later proves it reclaimable; `superseded` * means another owner had already replaced it, so this daemon released nothing - * and left nothing to reconcile. + * and left nothing to reconcile; `unattributable` means a record is still there + * that names no owner at all, which neither of the other two describes — see + * `DaemonShutdownClaims.unattributable`. */ export type DeviceClaimRecord = { deviceKey: string; @@ -34,6 +36,7 @@ export type DaemonShutdownReport = { released: DeviceClaimRecord[]; orphaned: DeviceClaimRecord[]; superseded: DeviceClaimRecord[]; + unattributable: DeviceClaimRecord[]; }; }; @@ -45,6 +48,7 @@ export function writeDaemonShutdownReport( released: readonly DeviceClaimRecord[]; orphaned: readonly DeviceClaimRecord[]; superseded: readonly DeviceClaimRecord[]; + unattributable: readonly DeviceClaimRecord[]; }; }, ): void { @@ -57,6 +61,7 @@ export function writeDaemonShutdownReport( released: [...outcome.claims.released], orphaned: [...outcome.claims.orphaned], superseded: [...outcome.claims.superseded], + unattributable: [...outcome.claims.unattributable], }, }; const filePath = shutdownReportPath(stateDir); @@ -119,12 +124,20 @@ function isProviderReleaseReport( function readClaimSection(value: { claims?: unknown }): DaemonShutdownReport['claims'] { const claims = value.claims; - if (!claims || typeof claims !== 'object') return { released: [], orphaned: [], superseded: [] }; - const records = claims as { released?: unknown; orphaned?: unknown; superseded?: unknown }; + if (!claims || typeof claims !== 'object') { + return { released: [], orphaned: [], superseded: [], unattributable: [] }; + } + const records = claims as { + released?: unknown; + orphaned?: unknown; + superseded?: unknown; + unattributable?: unknown; + }; return { released: readClaimRecords(records.released), orphaned: readClaimRecords(records.orphaned), superseded: readClaimRecords(records.superseded), + unattributable: readClaimRecords(records.unattributable), }; } diff --git a/src/daemon/daemon-stop.ts b/src/daemon/daemon-stop.ts index 829a347717..7f2dbf7461 100644 --- a/src/daemon/daemon-stop.ts +++ b/src/daemon/daemon-stop.ts @@ -30,6 +30,12 @@ export type DaemonStopResult = { claimsOrphaned: DeviceClaimRecord[]; /** Claims another owner had already taken over; this daemon released nothing. */ claimsSuperseded: DeviceClaimRecord[]; + /** + * Claims whose on-disk record named no owner, so this daemon cannot say whether the device is + * still held. `device release --stale` cannot settle these — it proves staleness from a recorded + * owner — so they are reported apart from {@link DaemonStopResult.claimsOrphaned}. + */ + claimsUnattributable: DeviceClaimRecord[]; providerReleases: { status: 'completed' | 'unknown'; released: ProviderReleaseRecord[]; @@ -76,6 +82,7 @@ export async function stopDaemon(params: { claimsReleased: [], claimsOrphaned: [], claimsSuperseded: [], + claimsUnattributable: [], providerReleases: { status: 'completed', released: [], pending: [] }, warnings: [], }; @@ -99,6 +106,7 @@ export async function stopDaemon(params: { claimsReleased: [], claimsOrphaned: [], claimsSuperseded: [], + claimsUnattributable: [], providerReleases: { status: 'unknown', released: [], pending: null }, warnings: [ 'The daemon was force-killed before provider lease state could be finalized. Provider allocations may remain active.', @@ -139,6 +147,7 @@ function notRunningResult(): DaemonStopResult { claimsReleased: [], claimsOrphaned: [], claimsSuperseded: [], + claimsUnattributable: [], providerReleases: { status: 'completed', released: [], pending: [] }, warnings: [], }; diff --git a/src/daemon/device/__tests__/device-claims.test.ts b/src/daemon/device/__tests__/device-claims.test.ts index e14b03a99a..fdc6f07a25 100644 --- a/src/daemon/device/__tests__/device-claims.test.ts +++ b/src/daemon/device/__tests__/device-claims.test.ts @@ -109,6 +109,24 @@ test('reports the exact outcome of clearing an owned, missing, and unowned claim assert.equal(await clearDeviceClaim(undefined), 'absent'); }); +test('reports an undecodable claim at its own device key as unattributable, not released', async () => { + const root = useClaimsRoot(); + const ownership = { + deviceKey: canonicalLocalDeviceKey(device), + ownerToken: 'this-daemons-token', + ownerPid: process.pid, + ownerStartTime: null, + } as const; + // A record sitting exactly where this ownership's claim belongs, that decodes into nothing. The + // clear cannot remove it and cannot attribute it, which is NOT the same information as a successor + // having taken the device — a caller that forgets its session here would strand this record under a + // process that no longer knows it holds the device. + fs.writeFileSync(claimPath(root), '{bad json'); + + assert.equal(await clearDeviceClaim(ownership), 'unattributable'); + assert.equal(fs.readFileSync(claimPath(root), 'utf8'), '{bad json'); +}); + test('keeps corrupt records visible and classifies dead owners without reclaiming either', () => { const root = useClaimsRoot(); fs.writeFileSync(path.join(root, 'corrupt.json'), '{bad json'); @@ -752,7 +770,10 @@ test('an allocator-held claim conflicts with an ordinary acquire, is never recon assert.equal(fs.readFileSync(claimPath(root), 'utf8'), before); // Apple runner arbitration reads process ownership; an installation principal grants none. assert.equal(processOwnsActiveDeviceClaim(device), false); - // Nothing this daemon holds can clear it either: there is no ownership to match. + // Nothing this daemon holds can clear it either: there is no ownership to match. And the record + // left behind yields no session claim to attribute the device to, so a caller that is about to + // forget its own claim cannot read this as "a successor has it now" — `ownership-changed` would + // say the device moved, while this record says nothing about who holds the device. assert.equal( await clearDeviceClaim({ deviceKey: canonicalLocalDeviceKey(device), @@ -760,7 +781,7 @@ test('an allocator-held claim conflicts with an ordinary acquire, is never recon ownerPid: process.pid, ownerStartTime: null, }), - 'ownership-changed', + 'unattributable', ); assert.equal(fs.readFileSync(claimPath(root), 'utf8'), before); }); diff --git a/src/daemon/device/device-claims.ts b/src/daemon/device/device-claims.ts index 2326ba9db0..3a1b399436 100644 --- a/src/daemon/device/device-claims.ts +++ b/src/daemon/device/device-claims.ts @@ -333,10 +333,16 @@ function staleReleaseRefusalReason(classification: DeviceClaimClassification): s * * - `deleted` — the claim this ownership acquired was removed. * - `absent` — no claim remains for the device; nothing to remove. - * - `ownership-changed`— a claim remains, but it is not the one we acquired - * (a successor owner, or a record we cannot attribute). + * - `ownership-changed`— a decodable claim remains and it is not the one we + * acquired, so a successor owns the device now. + * - `unattributable` — a record remains that yields no attributable claim: + * unreadable, undecodable, or held by an allocator + * principal. This says nothing about who owns the + * device and is NOT evidence we released anything, so a + * caller that is about to forget this claim must hold it + * back and try again rather than read it as superseded. */ -export type DeviceClaimClearOutcome = 'deleted' | 'absent' | 'ownership-changed'; +export type DeviceClaimClearOutcome = 'deleted' | 'absent' | 'ownership-changed' | 'unattributable'; export async function clearDeviceClaim( ownership: DeviceClaimSessionOwnership | undefined, @@ -353,7 +359,13 @@ export async function clearDeviceClaim( } return 'deleted'; }, - (conflict) => (conflict ? 'ownership-changed' : 'absent'), + (conflict) => { + if (!conflict) return 'absent'; + // `claim` is optional by type precisely to keep a reader from matching against a record that + // never decoded into a process-owned claim. Present means a successor's record; absent means + // the file told us nothing we can attribute to either side. + return conflict.claim ? 'ownership-changed' : 'unattributable'; + }, ); } diff --git a/src/daemon/request-binding.ts b/src/daemon/request-binding.ts index 488d7514b4..92cffd5cf0 100644 --- a/src/daemon/request-binding.ts +++ b/src/daemon/request-binding.ts @@ -10,6 +10,22 @@ import type { SessionRef } from './session-state.ts'; export type RequestExecutionLockKey = `session:${string}` | `device:${string}`; +/** + * The lock pair an already-bound session's commands take, in acquisition order. The #2833 + * session-idle expiry settles a session through these same keys in this same order: settling a + * session while a device-lock-holding command still runs on its device would release a device + * under work, and any other order would deadlock against a request's pair. + */ +export function existingSessionExecutionLockKeys( + sessionName: string, + deviceId: string, +): RequestExecutionLockKey[] { + return orderRequestExecutionLockKeys([ + sessionExecutionLockKey(sessionName), + deviceExecutionLockKey(deviceId), + ]); +} + export type RequestExecutionLockPlan = { keys: RequestExecutionLockKey[]; /** @@ -35,10 +51,7 @@ export async function resolveRequestExecutionLockPlan(params: { const existingSession = sessionStore.get(sessionName); if (existingSession) { return { - keys: orderRequestExecutionLockKeys([ - sessionExecutionLockKey(sessionName), - deviceExecutionLockKey(existingSession.device.id), - ]), + keys: existingSessionExecutionLockKeys(sessionName, existingSession.device.id), deviceId: existingSession.device.id, }; } diff --git a/src/daemon/request-execution-locks.ts b/src/daemon/request-execution-locks.ts index b91c3b39a8..13adc70d3c 100644 --- a/src/daemon/request-execution-locks.ts +++ b/src/daemon/request-execution-locks.ts @@ -40,7 +40,7 @@ export function createRequestExecutionLocks(params: { } running = true; try { - return await withRequestExecutionLocks(locks, initialKeys, task); + return await withRequestExecutionLockKeys(locks, initialKeys, task); } finally { try { await releaseRetainedLocks(); @@ -66,9 +66,9 @@ export function createRequestExecutionLocks(params: { }; } -async function withRequestExecutionLocks( +export async function withRequestExecutionLockKeys( locks: Map>, - keys: RequestExecutionLockKey[], + keys: readonly RequestExecutionLockKey[], task: () => Promise, ): Promise { const [key, ...remainingKeys] = keys; @@ -76,7 +76,7 @@ async function withRequestExecutionLocks( return await withKeyedLock( locks, key, - async () => await withRequestExecutionLocks(locks, remainingKeys, task), + async () => await withRequestExecutionLockKeys(locks, remainingKeys, task), ); } diff --git a/src/daemon/request-execution-scope.ts b/src/daemon/request-execution-scope.ts index 193b2eaaa7..489c66c306 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -26,7 +26,7 @@ import { } from './request-binding.ts'; import { beginOpenDeviceWait, readOpenWaitBudgetMs } from './open-device-contention-wait.ts'; import { createRequestExecutionLocks } from './request-execution-locks.ts'; -import { throwIfRequestCanceled } from '@agent-device/host-kit/request'; +import { isRequestCanceled, throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { finalizeDaemonResponse } from './request-finalization.ts'; import { refreshRecordingHealth } from './request-recording-health.ts'; import { runAdmittedLeaseWork } from './request-lease-work.ts'; @@ -134,6 +134,10 @@ export async function createRequestExecutionScope(params: { const command = scopedReq.command; const startedAtMs = Date.now(); + // The one trait that says a request acts *through* a session rather than merely resolving one: + // it is what puts the session's execution lock in this request's plan, and therefore the only + // trait under which an activity stamp can be written while holding that lock. + const attachesToSession = shouldLockSessionExecution(command); const sessionName = resolveEffectiveSessionName(scopedReq, sessionStore, { // Inventory commands (`session list`, `devices`, `doctor`, …) route only to locate their own // artifacts and never act through a session, so they must keep resolving an address even when @@ -249,29 +253,47 @@ export async function createRequestExecutionScope(params: { throwIfCanceled: () => throwIfRequestCanceled(scopedReq.meta?.requestId), runAdmitted: async (task) => { throwIfRequestCanceled(scopedReq.meta?.requestId); - await cleanupExpiredLeasedSession({ - sessionName, - sessionStore, - leaseRegistry, - teardownSession: async (session, expiredSessionName) => - await teardownExpiredSession({ - session, - sessionName: expiredSessionName, - sessionStore, - inspectFacts: scope.inspectFacts, - bindDevice: scope.bindDevice, - platformCleanup: requirePlatformCleanup(params.platformResourceCleanup), - }), - }); - scopedReq = admitRequestLeaseForLockedScope({ - req: scopedReq, - sessionName, - sessionStore, - leaseRegistry, - providerAppCatalog: params.providerAppCatalog, - }); - scope.req = scopedReq; - return await runAdmittedLeaseWork({ leaseRegistry, req: scopedReq, task }); + try { + await cleanupExpiredLeasedSession({ + sessionName, + sessionStore, + leaseRegistry, + teardownSession: async (session, expiredSessionName) => + await teardownExpiredSession({ + session, + sessionName: expiredSessionName, + sessionStore, + inspectFacts: scope.inspectFacts, + bindDevice: scope.bindDevice, + platformCleanup: requirePlatformCleanup(params.platformResourceCleanup), + }), + }); + scopedReq = admitRequestLeaseForLockedScope({ + req: scopedReq, + sessionName, + sessionStore, + leaseRegistry, + providerAppCatalog: params.providerAppCatalog, + }); + scope.req = scopedReq; + return await runAdmittedLeaseWork({ leaseRegistry, req: scopedReq, task }); + } finally { + // The #2833 inactivity deadline is measured from the END of the last command that ATTACHED + // to this session, stamped here under the session's own execution lock. The lock is what + // makes this one stamp enough: an expiry has to acquire it too, so it can never catch a + // session mid-command, and one command slower than the window keeps the session it is + // working on — the same guarantee admitted work gives a remote lease (ADR 0007). + // + // Two exclusions carry that parity. Inventory commands (`devices`, `doctor`, `session list`) + // resolve a session address only to locate their own artifacts, so on a shared host they run + // against a session they never act through — stamping there would let a bystander agent's + // polling keep another agent's abandoned claim alive forever. And a request whose client + // hung up preserves nothing, exactly as a canceled request renews no lease: an agent that + // timed out is the behavior this feature exists to catch. + if (attachesToSession && !isRequestCanceled(scopedReq.meta?.requestId)) { + sessionStore.noteSessionActivity(sessionName); + } + } }, runLocked: async (task) => { throwIfRequestCanceled(scopedReq.meta?.requestId); @@ -554,7 +576,13 @@ function contextFromRequestFlags( }; } -function getLeaseRegistryExecutionLocks( +/** + * The per-`LeaseRegistry` execution-lock map a request's session and device locks are taken from. + * Exported because the #2833 session-idle reaper expires sessions through this same map: an expiry + * that did not wait on the session's execution lock could tear down a session mid-command, and a + * second map would make that race invisible rather than impossible. + */ +export function getLeaseRegistryExecutionLocks( leaseRegistry: LeaseRegistry, ): Map> { let locks = leaseRegistryExecutionLocks.get(leaseRegistry); diff --git a/src/daemon/request-router.ts b/src/daemon/request-router.ts index 5ccf01d9e0..cb8defc0de 100644 --- a/src/daemon/request-router.ts +++ b/src/daemon/request-router.ts @@ -57,6 +57,10 @@ import { isWebSession } from './web-session-names.ts'; import { inferFillText } from '@agent-device/ad-script'; import { createPlatformRequestScope } from './platform-request-scope.ts'; import { createOwnerScopedDeviceClaimReconciler } from './device/device-claim-owner-recovery.ts'; +import { scopeRequestSession } from './request-admission.ts'; +import { resolveEffectiveSessionName } from './session-routing.ts'; +import { sessionIdleExpiredError } from './session-idle-expiry.ts'; +import type { IdleSessionTombstone } from './session-idle-tombstone.ts'; import { createAppLogAdmissionLedger, type AppLogAdmissionLedger, @@ -184,8 +188,13 @@ export function createRequestHandler(deps: RequestRouterDeps): DaemonInvokeFn { if (!response.ok) { // ADR 0012 decision 6, R7 (C5a): a command that finds no session but // hits a live repair tombstone gets `REPAIR_SESSION_EXPIRED` with - // re-run guidance, never a bare SESSION_NOT_FOUND. - const error = repairExpiredIfTombstoned(req, response.error, sessionStore); + // re-run guidance, never a bare SESSION_NOT_FOUND. #2833 adds the + // sibling case: an idle-expired session explains itself the same way. + const error = idleExpiredIfTombstoned( + req, + repairExpiredIfTombstoned(req, response.error, sessionStore), + sessionStore, + ); return { ok: false, error: enrichDaemonError(error) }; } // Phase 4 (agent-cost) grafts on the success path. Runs inside the @@ -596,6 +605,49 @@ function repairExpiredIfTombstoned( ); } +/** + * #2833: when a command finds no session because this daemon already expired it for idleness, the + * answer stays `SESSION_NOT_FOUND` — the session genuinely is gone — but carries the typed reason, + * the window it missed, and the device it released. A bare "Run open first" tells an agent on a + * shared machine to retry an `open` that will collide with its own expired session's claim if the + * release is still in flight, and gives it nothing to reason about either way. + * + * Consulted after the repair tombstone so an abandoned repair transaction keeps its own, more + * specific recovery guidance. The address is resolved through the same routing the request took; a + * name that cannot be resolved has no marker rather than an error that would replace the caller's. + */ +function idleExpiredIfTombstoned( + req: DaemonRequest, + error: DaemonError, + sessionStore: SessionStore, +): DaemonError { + if (error.code !== 'SESSION_NOT_FOUND') return error; + const tombstone = readIdleExpiryTombstoneSafely(req, sessionStore); + if (!tombstone) return error; + return normalizeError(sessionIdleExpiredError(tombstone.owner, tombstone)); +} + +function readIdleExpiryTombstoneSafely( + req: DaemonRequest, + sessionStore: SessionStore, +): IdleSessionTombstone | undefined { + try { + // The address is resolved exactly as the request itself resolved it, tenant scope included: a + // tenant-isolated request keeps its sessions under `:`, so reading the raw name + // would miss this request's own marker and could instead surface another tenant's, reporting an + // unrelated device as the one this caller just lost. + const scopedReq = scopeRequestSession(req); + // `attachesToSession: false` is the inventory reading: it never refuses an ambiguous workspace, + // which is right here because this read is a question about an absent session, not a request to + // act through one. + return sessionStore.readIdleExpiryTombstone( + resolveEffectiveSessionName(scopedReq, sessionStore, { attachesToSession: false }), + ); + } catch { + return undefined; + } +} + // Phase 2 typed-error graft: add machine-readable signals to an error response. // Returns the error unchanged unless a signal applies, so the default wire shape // is preserved for the common codes. diff --git a/src/daemon/server/daemon-runtime.ts b/src/daemon/server/daemon-runtime.ts index aca7766de1..f9d9ba0a13 100644 --- a/src/daemon/server/daemon-runtime.ts +++ b/src/daemon/server/daemon-runtime.ts @@ -27,6 +27,7 @@ import { LeaseRegistry } from '../lease-registry.ts'; import { createExpiredProviderLeaseReleaser } from '../provider-lease-expiry.ts'; import { clearDaemonShutdownReport, writeDaemonShutdownReport } from '../daemon-shutdown-report.ts'; import { createRequestHandler } from '../request-router.ts'; +import { getLeaseRegistryExecutionLocks } from '../request-execution-scope.ts'; import { stopSessionAppLog, teardownSessionResources } from '../session-teardown.ts'; import { resolveDaemonSessionTeardownTimeoutMs } from '../session-teardown-budget.ts'; import { finalizeDaemonSessionApplicationLifecycle } from '../application-lifecycle-recovery.ts'; @@ -35,6 +36,8 @@ import { closeDaemonServers } from './server-shutdown.ts'; import type { DaemonInvokeFn } from '../daemon-request.ts'; import type { SessionState } from '../session-state.ts'; import { createDaemonIdleReap } from './daemon-idle-reap.ts'; +import { createSessionIdleExpiry } from './daemon-session-idle-expiry.ts'; +import { resolveSessionIdleExpiryMs } from '../session-idle-expiry.ts'; import { finalizeDaemonSessionLease } from './daemon-session-lease-finalizer.ts'; import { processOwnsActiveDeviceClaim, @@ -398,6 +401,73 @@ export async function startDaemonRuntime( await Promise.all(sessionsToStop.map(teardownDaemonSession)); }; + // #2833: settles the resources of a session this daemon expires for idleness. Deliberately NOT + // `teardownDaemonSession`: that one exists for a daemon that is leaving, so it hands a healthy + // execution host to its successor, finalizes a remote lease, and deletes the session whether the + // bounded teardown finished or not. An idle expiry is the opposite situation — the daemon is + // staying alive, there is no successor, and a session whose resources would not release has to + // survive so the next pass can retry rather than leave a claim owned by a process that no longer + // knows what it holds. So: resources, then the platform finalization that stops the execution host + // and releases its lease, then the claim, cleared last, by the reaper. + const settleIdleExpiredSession = async ( + session: SessionState, + sessionName: string, + ): Promise => { + await teardownSessionResources({ + appLog: 'run', + session, + sessionName, + sessionStore, + stateDir: baseDir, + platformCleanup: platformResourceCleanup, + }); + await finalizeDaemonSessionApplicationLifecycle({ + gateway: deviceRuntimeGateway, + scope: createDaemonRecoveryPlatformScope(), + session, + stateDir: baseDir, + runtimeHints: runtimeHintValues(sessionStore.getRuntimeHints(sessionName)), + // The one caller that must say so: this daemon is staying alive, so there is no shutdown + // phase a healthy runner could be deferred to. Taking the ordinary-close path stops the + // runner and releases its lease instead of parking it until process exit. + daemonLeaving: false, + }); + // ADR 0012 R7 binds this teardown too — the healed `.ad` is committed by + // `SessionStore.finalizeRepairTeardown` — but NOT from in here. That call publishes the script + // and stamps COMMITTED onto the record it is handed, which makes it part of ENDING the session + // rather than part of releasing its resources, and a settle that cannot confirm its claim gone + // holds the session back to retry. The reaper runs it once the expiry is committed to. + }; + + const sessionIdleExpiry = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: resolveSessionIdleExpiryMs(env), + executionLocks: getLeaseRegistryExecutionLocks(leaseRegistry), + settleSession: settleIdleExpiredSession, + // The same bounded teardown budget every other session teardown gets. It bounds only how long a + // sweep waits, never the settle itself, so a stuck recorder cannot make a sweep hang but also + // cannot make an expiry give up on a device that does come free. + settleBudgetMs: (session) => resolveDaemonSessionTeardownTimeoutMs(session), + // A sweep is out-of-request work, so it has no request scope and `emitDiagnostic` would drop + // everything it reports — including the record of what was reclaimed and why a reclaim failed. + withinDiagnosticsScope: async (run) => + await withDiagnosticsScope( + { command: 'daemon', session: 'daemon', logPath, debug: true }, + async () => { + try { + return await run(); + } finally { + flushDiagnosticsToSessionFile({ force: true }); + } + }, + ), + // An expiry can be the event that makes this daemon fully idle, and no request follows it to + // arm the process-level reap. + onSessionExpired: () => { + idleReap.noteActivity(); + }, + }); + // Reaps this daemon process when it sits fully idle (no open sessions, no // in-flight requests, no active recording) past AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS. // `shutdown` is defined below but only invoked asynchronously by the timer, @@ -420,6 +490,11 @@ export async function startDaemonRuntime( } finally { inFlightRequestCount--; idleReap.noteActivity(); + // One hook covers every way a deadline changes: an `open` has added one, a `close` has removed + // one, and any other command has just re-stamped the session it ran on. Reading the session set + // here — after the request's own session stamp landed inside its execution lock — is what keeps + // the reaper's view of a session's deadline identical to the lock-guarded one. + sessionIdleExpiry.noteSessionsChanged(); } }; @@ -563,6 +638,8 @@ export async function startDaemonRuntime( // Arms the initial idle-reap timer: a daemon that starts and never // receives a request must still be able to reap itself. idleReap.noteActivity(); + // The #2833 deadline needs no equivalent arm: the store is empty until a request puts a session + // in it, and that request's own completion re-arms the reaper. } catch (error) { const appErr = asAppError(error); stderr.write(`Daemon error: ${appErr.message}\n`); @@ -582,6 +659,7 @@ export async function startDaemonRuntime( let shuttingDown = false; const shutdown = async (shutdownOptions: { exitCode?: number; cause?: unknown } = {}) => { idleReap.cancel(); + sessionIdleExpiry.cancel(); if (shuttingDown) return; shuttingDown = true; if (shutdownOptions.cause) { diff --git a/src/daemon/server/daemon-session-idle-expiry-scheduling.test.ts b/src/daemon/server/daemon-session-idle-expiry-scheduling.test.ts new file mode 100644 index 0000000000..8aae5cd14b --- /dev/null +++ b/src/daemon/server/daemon-session-idle-expiry-scheduling.test.ts @@ -0,0 +1,698 @@ +import { test, vi } from 'vitest'; +import assert from 'node:assert/strict'; +import { withKeyedLock } from '@agent-device/kernel/keyed-lock'; +import { createSessionIdleExpiry } from './daemon-session-idle-expiry.ts'; +import { + createIdleExpiryHarness, + CLAIM, + NOW, + WINDOW_MS, +} from '../__tests__/session-idle-expiry-harness.ts'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import { IOS_SIMULATOR } from '../../__tests__/test-utils/device-fixtures.ts'; + +/** + * #2833: WHEN the reaper starts an expiry and how long it may take. The window that arms it, the + * ceiling on a delay a Node timer can express, the session's and device's execution locks it must + * wait behind, the budget on how long anyone waits for it, the deferral a failed or in-flight release + * owes before another attempt, and the daemon that begins to leave while a sweep is running. + * + * What an expiry that runs actually COMMITS is `daemon-session-idle-expiry.test.ts`. + */ +const harness = createIdleExpiryHarness(); +const { + makeFixture, + idleClaimedSession, + sessionWithLiveClaim, + claimFileHeld, + runUntilIdle, + waitFor, + boundAbsence, + createSweepBarrier, +} = harness; + +test('a session still inside its window survives a sweep that actually ran', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-active-'); + const session = idleClaimedSession(sessionStore); + session.lastActivityAtMs = NOW; + // A second session that IS past its window and holding a claim. Without it `nextDueMs` sees nothing + // due and arms no timer at all, so a sweep never runs and `settleCalls === 0` would pass even for a + // reaper that ignored the window entirely. This one gives the sweep a reason to run, which is what + // puts the fresh session in front of the deadline re-check this test is about. + idleClaimedSession(sessionStore, 'expired'); + const settledNames: string[] = []; + const sweeps = createSweepBarrier(); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async (_session, sessionName) => { + settledNames.push(sessionName); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + + assert.deepEqual( + settledNames, + ['expired'], + 'the sweep ran and reached both sessions, settling only the one past its window', + ); + assert.notEqual(sessionStore.get('default'), undefined); + controller.cancel(); +}); + +test('the policy being off arms nothing at all, even for a long-dead session', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-off-'); + idleClaimedSession(sessionStore); + let settleCalls = 0; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: 0, + executionLocks: new Map(), + settleSession: async () => { + settleCalls++; + }, + now: () => NOW, + }); + await runUntilIdle(controller, 20); + + assert.equal(controller.idleExpiryMs, 0); + assert.equal(settleCalls, 0); + assert.notEqual(sessionStore.get('default'), undefined); +}); + +// Node's own ceiling, not a knob the module exposes: above this a `setTimeout` delay is silently +// replaced by 1 ms. The two tests below assert against that platform fact. +const NODE_MAX_TIMER_DELAY_MS = 2_147_483_647; +// 30 days: past that ceiling, and a perfectly readable way for an operator to say "essentially +// never". `resolveSessionIdleExpiryMs` accepts it because off is spelled `0`, not `huge`. +const LONG_WINDOW_MS = 30 * 24 * 60 * 60_000; + +test('a window longer than a timer can express does not sweep a session that is inside it', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-long-window-quiet-'); + const session = idleClaimedSession(sessionStore); + session.lastActivityAtMs = NOW; + let sweeps = 0; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: LONG_WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => { + throw new Error('a 30-day window must not expire a session milliseconds after it opened'); + }, + // The scope wraps exactly one sweep, so it counts sweeps without a test-only seam. + withinDiagnosticsScope: async (run) => { + sweeps++; + await run(); + }, + now: () => NOW, + }); + await runUntilIdle(controller, 50); + + assert.equal(sweeps, 0, 'a long window waits; it does not poll every millisecond'); + assert.notEqual(sessionStore.get('default'), undefined); + controller.cancel(); +}); + +test('a window longer than a timer can express still expires once the window elapses', async () => { + vi.useFakeTimers(); + try { + const { sessionStore } = makeFixture('agent-device-idle-expiry-long-window-due-'); + // A ticking clock, because what this asserts happens after more than a day of waiting. + let clock = NOW; + let sweeps = 0; + const session = idleClaimedSession(sessionStore); + session.lastActivityAtMs = NOW; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: LONG_WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: async (run) => { + sweeps++; + await run(); + }, + now: () => clock, + }); + controller.noteSessionsChanged(); + + // The first piece of the wait ends with the deadline still ahead. + await vi.advanceTimersByTimeAsync(NODE_MAX_TIMER_DELAY_MS + 1_000); + assert.equal(sweeps, 0, 'a piece that ends early re-arms instead of sweeping'); + assert.notEqual(sessionStore.get('default'), undefined); + + clock = NOW + LONG_WINDOW_MS; + await vi.advanceTimersByTimeAsync(2 * NODE_MAX_TIMER_DELAY_MS + 5_000); + assert.equal(sweeps, 1, 'the sweep runs once the deadline is finally reached'); + assert.equal(sessionStore.get('default'), undefined); + controller.cancel(); + } finally { + vi.useRealTimers(); + } +}); + +test('an expiry waits for the session lock a command of its own holds', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-session-lock-'); + idleClaimedSession(sessionStore); + const locks = new Map>(); + let settleCalls = 0; + const sweeps = createSweepBarrier(); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: locks, + settleSession: async () => { + settleCalls++; + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + + let release!: () => void; + const held = new Promise((resolve) => { + release = resolve; + }); + const command = withKeyedLock(locks, 'session:default', async () => { + await held; + }); + + controller.noteSessionsChanged(); + await boundAbsence(40); + assert.equal(settleCalls, 0, 'a mid-command session must not be expired'); + assert.notEqual(sessionStore.get('default'), undefined); + + release(); + await command; + // No budget is configured, so the scope does not resolve until the settle is over. Reaching here is + // the proof that the sweep really ran — which is what makes the zero above mean "waited" rather than + // "never started". + await sweeps.swept(1); + assert.equal(settleCalls, 1, 'the same expiry settles once the lock is free'); + controller.cancel(); +}); + +test('an expiry waits for the DEVICE lock a command on another session holds', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-device-lock-'); + idleClaimedSession(sessionStore); + const locks = new Map>(); + let settleCalls = 0; + const sweeps = createSweepBarrier(); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: locks, + settleSession: async () => { + settleCalls++; + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + + // Another session's command on the SAME device: no session-key overlap, so only the device key + // can prove the expiry is not about to free a device that is under work. + let release!: () => void; + const held = new Promise((resolve) => { + release = resolve; + }); + const command = withKeyedLock(locks, 'device:sim-1', async () => { + await held; + }); + + controller.noteSessionsChanged(); + await boundAbsence(40); + assert.equal(settleCalls, 0); + assert.notEqual(sessionStore.get('default'), undefined); + + release(); + await command; + await sweeps.swept(1); + assert.equal(settleCalls, 1); + controller.cancel(); +}); + +test('a command that re-stamped activity while the expiry queued cancels it', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-restamp-'); + idleClaimedSession(sessionStore); + const locks = new Map>(); + let settleCalls = 0; + let commandFinished = false; + let sweptWhileHeld = false; + let sweepsCompleted = 0; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: locks, + settleSession: async () => { + settleCalls++; + }, + withinDiagnosticsScope: async (run) => { + // The sweep starting while the command still holds the lock is what makes the zero below mean + // "refused inside the lock" rather than "never got around to it". + sweptWhileHeld = sweptWhileHeld || !commandFinished; + await run(); + sweepsCompleted++; + }, + now: () => NOW, + }); + + let release!: () => void; + const held = new Promise((resolve) => { + release = resolve; + }); + const command = withKeyedLock(locks, 'session:default', async () => { + await held; + // The command finishes and re-stamps the session. The store hands out the live record, so this + // is a durable write; a sweep that trusted the reference it swept would not see it. + sessionStore.noteSessionActivity('default', NOW); + }); + + controller.noteSessionsChanged(); + await runUntilIdle(controller, 10); + release(); + await command; + commandFinished = true; + await waitFor(() => sweepsCompleted > 0, 'a sweep to complete'); + + assert.equal(sweptWhileHeld, true, 'the sweep began while the command held the lock'); + assert.equal(settleCalls, 0, 'the deadline is re-checked inside the lock, not at scheduling'); + assert.notEqual(sessionStore.get('default'), undefined); + controller.cancel(); +}); + +test('a session that moved to another device is fenced by that device, not the swept one', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-device-moved-'); + idleClaimedSession(sessionStore); + const locks = new Map>(); + const settledDeviceIds: string[] = []; + const sweeps = createSweepBarrier(); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: locks, + settleSession: async (session) => { + settledDeviceIds.push(session.device.id); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + + // The sweep picks its lock pair from the session it found. A command on that session can replace + // the record with one bound to a DIFFERENT device while the expiry is queued, and settling THAT + // session under the swept device's lock would free a device nothing here holds. + let releaseCommand!: () => void; + const commandHeld = new Promise((resolve) => { + releaseCommand = resolve; + }); + const command = withKeyedLock(locks, 'session:default', async () => { + await commandHeld; + sessionStore.set( + 'default', + makeIosSession('default', { + createdAt: NOW - WINDOW_MS - 1, + device: { ...IOS_SIMULATOR, id: 'sim-2', name: 'iPhone 17' }, + deviceClaim: { ...CLAIM, deviceKey: 'ios:sim-2' }, + }), + ); + }); + + // A command on the NEW session's device is in flight the whole time. + let releaseDevice!: () => void; + const deviceHeld = new Promise((resolve) => { + releaseDevice = resolve; + }); + const otherCommand = withKeyedLock(locks, 'device:sim-2', async () => { + await deviceHeld; + }); + + controller.noteSessionsChanged(); + await runUntilIdle(controller, 10); + releaseCommand(); + await command; + // Bounded, and then closed properly: the sweep that queued is still inside the device lock, so + // `swept` below is the fact that it eventually ran rather than a window in which nothing happened. + assert.deepEqual(settledDeviceIds, [], 'nothing settles while the settled device is held'); + + releaseDevice(); + await otherCommand; + // Not the barrier: the sweep that found the device held completed too, and it settled nothing. This + // waits for the settle itself, which no budget short-circuits. + await waitFor(() => settledDeviceIds.length > 0, 'the expiry to settle the moved session'); + + // And what it does settle is the session it now holds the device lock for. + assert.deepEqual(settledDeviceIds, ['sim-2']); + controller.cancel(); +}); + +test('a settle that outlives the budget is caught by the sweep rather than the process handler', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-budget-'); + idleClaimedSession(sessionStore); + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => { + throw new Error('teardown exceeded its budget'); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + // Reached here rather than an unhandled rejection, which would shut the daemon down. + assert.notEqual(sessionStore.get('default'), undefined); + controller.cancel(); +}); + +test('a sweep runs inside the composition diagnostics scope it is given', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-scope-'); + idleClaimedSession(sessionStore); + const scopes: string[] = []; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: async (run) => { + scopes.push('entered'); + await run(); + scopes.push('flushed'); + }, + now: () => NOW, + }); + controller.noteSessionsChanged(); + // `flushed` is written after the sweep resolves, so waiting for it waits for the whole sweep. + await waitFor(() => scopes.includes('flushed'), 'the scope to flush after the sweep'); + + assert.deepEqual(scopes, ['entered', 'flushed']); + controller.cancel(); +}); + +test('a settle that outruns its wait still finishes the release it started', async () => { + const fixture = makeFixture('agent-device-idle-expiry-over-budget-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + let releaseSettle!: () => void; + const settleHeld = new Promise((resolve) => { + releaseSettle = resolve; + }); + let settleCalls = 0; + // A clock that can move: the deferred retry is measured against this clock, so a frozen one would + // answer "may a second teardown join the first?" with "the retry isn't due yet" and prove nothing. + let clock = NOW; + + // A window short enough that the retry becomes reachable while the settle is still stuck. + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: 40, + executionLocks: new Map(), + settleSession: async () => { + settleCalls++; + await settleHeld; + }, + settleBudgetMs: () => 10, + now: () => clock, + }); + // Arrival is waited for; the bound afterwards is only what proves the second part — that no + // further sweep joined the stuck one. + await runUntilIdle(controller, 30); + await waitFor(() => settleCalls > 0, 'the first settle to start'); + + // The wait is what the budget bounds. Releasing the claim here would free a device that still has + // whatever the stuck teardown was holding, which is the failure this whole ordering exists to avoid. + assert.equal(settleCalls, 1); + assert.equal(claimFileHeld(deviceClaim), true, 'an unfinished settle must not free the device'); + assert.notEqual(sessionStore.get('default'), undefined); + + // The budget released the sweep's wait AND its execution lock, but not the work. Push the clock + // past the retry cadence so the next sweep is genuinely due: a second teardown would then stop + // resources the first one is still holding. + clock = NOW + 200; + controller.noteSessionsChanged(); + await runUntilIdle(controller, 60); + assert.equal(settleCalls, 1, 'one stuck settle must not be joined by a second teardown'); + + releaseSettle(); + await waitFor( + () => !claimFileHeld(deviceClaim) && sessionStore.get('default') === undefined, + 'the settle that was never abandoned to complete the release', + ); + controller.cancel(); +}); + +test('a daemon beginning to leave does not start settling the next session', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-closing-'); + sessionStore.set( + 'first', + makeIosSession('first', { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: { ...CLAIM, deviceKey: 'ios:sim-1' }, + }), + ); + sessionStore.set( + 'second', + makeIosSession('second', { + createdAt: NOW - WINDOW_MS - 1, + device: { ...IOS_SIMULATOR, id: 'sim-2' }, + deviceClaim: { ...CLAIM, deviceKey: 'ios:sim-2' }, + }), + ); + const settledNames: string[] = []; + let releaseFirst!: () => void; + const firstHeld = new Promise((resolve) => { + releaseFirst = resolve; + }); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async (_session, sessionName) => { + settledNames.push(sessionName); + if (sessionName === 'first') await firstHeld; + }, + now: () => NOW, + }); + controller.noteSessionsChanged(); + // Waits for the first settle to be entered, so the assertion below cannot be reading a sweep that + // simply had not reached the second session yet. + await waitFor(() => settledNames.length > 0, 'the first session to be settled'); + assert.deepEqual(settledNames, ['first']); + + controller.cancel(); + releaseFirst(); + // `cancel()` stops re-arming, so no second expiry can start; this bound is the only way to express + // "and it stays that way", and the first settle above proves the loop was live to begin with. + await runUntilIdle(controller, 30); + + assert.deepEqual( + settledNames, + ['first'], + 'shutdown is tearing the session set down; a second expiry would only race it', + ); + assert.notEqual(sessionStore.get('second'), undefined); +}); + +test('an over-budget expiry keeps the locks the release itself needs', async () => { + const fixture = makeFixture('agent-device-idle-expiry-lock-held-past-budget-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + const locks = new Map>(); + let releaseSettle!: () => void; + const settleHeld = new Promise((resolve) => { + releaseSettle = resolve; + }); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: locks, + settleSession: async () => { + await settleHeld; + }, + settleBudgetMs: () => 10, + now: () => NOW, + }); + await runUntilIdle(controller, 30); + + // The budget released the sweep's WAIT. It must not have released the pair: a `close` or a retried + // `open` that took these locks now would join a teardown mid-flight, and the release's own final + // steps — delete this record, write the marker — would then land on whatever that request created. + let acquiredWhileStuck = false; + const contender = withKeyedLock(locks, 'session:default', async () => { + acquiredWhileStuck = true; + }); + // Bounded by necessity: "did not acquire" is the absence being asserted, and there is no fact to + // poll for. The wait is not load-bearing on its own — the release below is what proves the pair was + // genuinely held rather than simply uncontended. + await runUntilIdle(controller, 20); + assert.equal( + acquiredWhileStuck, + false, + 'a request must not reach a session whose release is still running', + ); + assert.equal(claimFileHeld(deviceClaim), true); + + releaseSettle(); + await contender; + await waitFor( + () => !claimFileHeld(deviceClaim) && sessionStore.get('default') === undefined, + 'the release that kept the locks to finish', + ); + assert.equal(acquiredWhileStuck, true, 'the pair comes back when the release is over'); + controller.cancel(); +}); + +test('an expiry that lands after its budget still reports the session it released', async () => { + const fixture = makeFixture('agent-device-idle-expiry-late-notify-'); + const { sessionStore } = fixture; + await sessionWithLiveClaim(fixture); + let releaseSettle!: () => void; + const settleHeld = new Promise((resolve) => { + releaseSettle = resolve; + }); + const expired: string[] = []; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => { + await settleHeld; + }, + settleBudgetMs: () => 10, + onSessionExpired: (outcome) => { + expired.push(outcome.sessionName); + }, + now: () => NOW, + }); + await runUntilIdle(controller, 30); + // The composition arms the process-level reap from this callback. A release that outran the wait and + // went unreported would leave a fully idle daemon alive forever. + assert.deepEqual(expired, []); + + releaseSettle(); + // This controller runs with a settle budget, so the diagnostics scope resolves when the WAIT ends, + // not when the release does. Poll for the report itself. + await waitFor(() => expired.length > 0, 'the late release being reported'); + assert.deepEqual(expired, ['default'], 'a completed release is reported whenever it lands'); + controller.cancel(); +}); + +test('a stuck expiry does not leave the timer firing with no delay', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-stuck-loop-'); + idleClaimedSession(sessionStore); + let releaseSettle!: () => void; + const settleHeld = new Promise((resolve) => { + releaseSettle = resolve; + }); + let sweeps = 0; + let settles = 0; + // A clock that runs: the retry window only becomes reachable if time moves, and the loop this test + // guards against is a timer that fires the instant it is armed. + let clock = NOW; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: 20, + executionLocks: new Map(), + // One call per sweep, which is the quantity at issue: a settle skipped while another is in flight + // never reaches `settleSession`, so counting settles would rate a re-firing timer as healthy. + withinDiagnosticsScope: async (run) => { + sweeps++; + await run(); + }, + settleSession: async () => { + settles++; + await settleHeld; + }, + settleBudgetMs: () => 5, + now: () => clock, + }); + controller.noteSessionsChanged(); + await waitFor(() => settles > 0, 'the first expiry to start'); + assert.equal(settles, 1, 'the first expiry starts once'); + + // The budget released the sweep's wait ages ago and the retry window has elapsed too, while the + // release is still stuck holding the locks. A sweep that skips this address and re-arms at the same + // elapsed deadline re-enters this code on every turn of the event loop. + const sweepsBeforeSkipping = sweeps; + clock = NOW + 10_000; + for (let round = 0; round < 5; round++) { + controller.noteSessionsChanged(); + await runUntilIdle(controller, 10); + } + assert.equal(settles, 1, 'a stuck release must not be joined by a second teardown'); + // The bound is the point, not the exact count. A skip that leaves this deadline in the past re-arms + // at zero delay and sweeps again the instant it is armed, so these five nudges would produce dozens + // of sweeps — one per turn of the event loop — instead of at most one per nudge. + assert.ok( + sweeps - sweepsBeforeSkipping <= 5, + `five nudges must not sweep more than five times, swept ${sweeps - sweepsBeforeSkipping}`, + ); + + releaseSettle(); + await waitFor(() => sessionStore.get('default') === undefined, 'the stuck release to finish'); + controller.cancel(); +}); + +test('a settle that outlives its window is retried a full window after it ends, not after it started', async () => { + vi.useFakeTimers(); + try { + const { sessionStore } = makeFixture('agent-device-idle-expiry-long-settle-retry-'); + idleClaimedSession(sessionStore); + // No settle budget, so the failing settle writes the only deferral this session has: that is the + // one whose clock is in question. Fake timers because a deferral is a duration, and this measures + // one that is longer than the settle that owes it. + const WINDOW = 120; + let releaseSettle!: () => void; + const settleHeld = new Promise((resolve) => { + releaseSettle = resolve; + }); + let settles = 0; + let settledAt = 0; + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW, + executionLocks: new Map(), + settleSession: async () => { + settles++; + await settleHeld; + settledAt = Date.now(); + // The release itself fails: the session is held back for another attempt. + throw new Error('the recorder would not stop'); + }, + }); + controller.noteSessionsChanged(); + await vi.advanceTimersByTimeAsync(5); + assert.equal(settles, 1); + + // The release outlives the window it was started inside. A deferral measured from the START of + // that settle is already elapsed by the time it is written, so the reaper arms at zero delay and + // runs a second teardown of a session whose first one only just failed. + await vi.advanceTimersByTimeAsync(WINDOW * 2); + releaseSettle(); + await vi.advanceTimersByTimeAsync(5); + assert.ok(settledAt > 0, 'the settle reached its failure'); + assert.equal(settles, 1, 'a settle that just failed owes a full window, not zero delay'); + + await vi.advanceTimersByTimeAsync(WINDOW); + assert.equal(settles, 2, 'the window it was owed does come due'); + controller.cancel(); + } finally { + vi.useRealTimers(); + } +}); diff --git a/src/daemon/server/daemon-session-idle-expiry.test.ts b/src/daemon/server/daemon-session-idle-expiry.test.ts new file mode 100644 index 0000000000..402da7d219 --- /dev/null +++ b/src/daemon/server/daemon-session-idle-expiry.test.ts @@ -0,0 +1,378 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { + readIdleSessionTombstoneFile, + resolveIdleSessionTombstonePath, +} from '../session-idle-tombstone.ts'; +import { createSessionIdleExpiry } from './daemon-session-idle-expiry.ts'; +import { + createIdleExpiryHarness, + CLAIM, + NOW, + WINDOW_MS, +} from '../__tests__/session-idle-expiry-harness.ts'; +import { + makeIosSession, + makeRepairCompleteSession, +} from '../../__tests__/test-utils/session-factories.ts'; +import { resolveDeviceClaimPath } from '../device/device-claim-paths.ts'; + +/** + * #2833: what one expiry COMMITS, once it has run. Whether a session past its window is the kind that + * may be expired at all, and once it is: its claim released, its record deleted, its repair + * transaction committed, and a bounded marker naming the device left for the next command. Including + * whether a settle that could not confirm the release is allowed to have forgotten anything. + * + * WHEN the reaper starts an expiry and how long it may take is + * `daemon-session-idle-expiry-scheduling.test.ts`. + */ +const harness = createIdleExpiryHarness(); +const { + makeFixture, + idleClaimedSession, + unclaimedExpiredSession, + sessionWithLiveClaim, + claimFileHeld, + runUntilIdle, + waitFor, + createSweepBarrier, +} = harness; + +test('a session past its window releases its claim, is deleted, and leaves a marker naming its device', async () => { + const fixture = makeFixture('agent-device-idle-expiry-settle-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + assert.equal(claimFileHeld(deviceClaim), true, 'the fixture stands a claim that is really held'); + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + + // The device is what every other agent on the host waits for, so this — not the in-memory record — + // is the promise the feature makes. + assert.equal(claimFileHeld(deviceClaim), false, 'the expired claim must be released'); + assert.equal(sessionStore.get('default'), undefined); + const marker = readIdleSessionTombstoneFile( + resolveIdleSessionTombstonePath(sessionStore.resolveSessionDir('default')), + ); + assert.equal(marker?.owner, 'default'); + assert.equal(marker?.deviceKey, deviceClaim.deviceKey); + assert.equal(marker?.idleExpiryMs, WINDOW_MS); + controller.cancel(); +}); + +test('a settle that cannot confirm the claim gone keeps the session record standing', async () => { + const fixture = makeFixture('agent-device-idle-expiry-claim-unconfirmed-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + + // A claim store this process cannot write into: the clear reaches NO verdict, which is different + // information from a successor having taken the device. + const claimsDir = path.dirname(resolveDeviceClaimPath(deviceClaim.deviceKey)); + fs.chmodSync(claimsDir, 0o500); + try { + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + controller.cancel(); + } finally { + fs.chmodSync(claimsDir, 0o700); + } + + // Deleting the record here would leave a claim owned by a live daemon that no longer knows it holds + // it — un-reclaimable by `device release --stale`, which proves staleness from owner liveness. + assert.notEqual( + sessionStore.get('default'), + undefined, + 'an unconfirmed clear holds the record back for retry', + ); + assert.equal( + fs.existsSync(resolveIdleSessionTombstonePath(sessionStore.resolveSessionDir('default'))), + false, + 'a settle that did not release the device must not report the session as expired', + ); +}); + +test('a claim that yields no attributable owner keeps the session record standing', async () => { + const fixture = makeFixture('agent-device-idle-expiry-unattributable-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + // The record this daemon wrote is now unreadable. The clear therefore learns nothing about whether + // its claim is gone, which is a different answer from a successor owning the device — and only that + // successor case may justify forgetting a session whose daemon is still alive. + fs.writeFileSync(resolveDeviceClaimPath(deviceClaim.deviceKey), '{bad json'); + + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + controller.cancel(); + + assert.notEqual( + sessionStore.get('default'), + undefined, + 'an unattributable record is still a claim this daemon may be holding', + ); + assert.equal( + fs.existsSync(resolveIdleSessionTombstonePath(sessionStore.resolveSessionDir('default'))), + false, + 'a device that was not confirmed free must not be reported as released', + ); +}); + +test('a settle that fails keeps the session and its claim, and retries on the next window', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-retry-'); + idleClaimedSession(sessionStore); + let clock = NOW; + let settleCalls = 0; + + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => { + settleCalls++; + throw new Error('recorder would not finalize'); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => clock, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + + // The session survives: deleting it would leave a claim owned by a process that no longer knows + // what it holds — worse than not expiring at all. + assert.equal(settleCalls, 1); + assert.notEqual(sessionStore.get('default'), undefined); + assert.equal( + fs.existsSync(resolveIdleSessionTombstonePath(sessionStore.resolveSessionDir('default'))), + false, + 'a failed settle must not tell the next caller the session is gone', + ); + + // The retry does not spin: the next attempt waits a full window rather than refiring at zero. + // Bounded because the fact asserted is that nothing happened; the pass below waits for the outcome. + await runUntilIdle(controller, 10); + assert.equal(settleCalls, 1, 'the retry is deferred, not immediate'); + + clock = NOW + WINDOW_MS; + controller.noteSessionsChanged(); + await waitFor(() => settleCalls === 2, 'the retry becoming due'); + controller.cancel(); +}); + +test('a session shutdown finalized mid-settle is not also reported as expired', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-shutdown-race-'); + idleClaimedSession(sessionStore); + const sweeps = createSweepBarrier(); + + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + // Shutdown takes no execution lock: it tears the whole session set down directly, so it is the + // one remover that can finish while a settle is still running. + settleSession: async (_session, sessionName) => { + sessionStore.delete(sessionName); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + controller.cancel(); + + assert.equal( + fs.existsSync(resolveIdleSessionTombstonePath(sessionStore.resolveSessionDir('default'))), + false, + 'shutdown closed this session; a marker would blame idleness', + ); +}); + +test('a session closed by its owner stops the retry clock tracking it', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-closed-retry-'); + idleClaimedSession(sessionStore); + let settleCalls = 0; + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => { + settleCalls++; + throw new Error('cleanup failed'); + }, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + assert.equal(settleCalls, 1); + + sessionStore.delete('default'); + await runUntilIdle(controller, 10); + assert.equal(settleCalls, 1, 'nothing is left to retry once the session is gone'); + controller.cancel(); +}); + +test('a settled session of another kind is never touched by the sweep', async () => { + const { sessionStore } = makeFixture('agent-device-idle-expiry-kind-'); + // Past its deadline in time, but holding a remote lease: that lease owns the device. + sessionStore.set( + 'leased', + makeIosSession('leased', { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: { ...CLAIM }, + lease: { + leaseId: 'lease-1', + tenantId: 'tenant-1', + runId: 'run-1', + expiresAt: NOW + WINDOW_MS, + }, + }), + ); + // Past its deadline too, but holding no claim: there is no device for another agent to wait on, + // so there is nothing here an expiry could reclaim and nothing to release. + sessionStore.set('plain', unclaimedExpiredSession('plain')); + + const settledNames: string[] = []; + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + // Succeeds rather than throwing: a rejected settle leaves every record standing for retry, which + // would let this pass read as a correct refusal even if the sweep had torn both sessions down. + settleSession: async (_session, sessionName) => { + settledNames.push(sessionName); + }, + now: () => NOW, + }); + await runUntilIdle(controller, 10); + + assert.deepEqual(settledNames, [], 'neither session is eligible, so neither is ever settled'); + assert.notEqual(sessionStore.get('leased'), undefined); + assert.notEqual(sessionStore.get('plain'), undefined); + controller.cancel(); +}); + +test('a held-back settle leaves a repair transaction uncommitted, so a later pass can still publish it', async () => { + const fixture = makeFixture('agent-device-idle-expiry-repair-held-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + // A repair transaction that reached its last executable step: this is the state whose healed `.ad` + // a teardown commits, and a commit is ONE-WAY — writing onto an already-committed transaction is an + // idempotent no-op. Finalizing a settle that is about to be held back would therefore mark a + // still-live session's script as published and no later teardown would ever publish it. + const session = makeRepairCompleteSession('default', { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: { ...deviceClaim }, + }); + sessionStore.set('default', session); + + const claimsDir = path.dirname(resolveDeviceClaimPath(deviceClaim.deviceKey)); + const sweeps = createSweepBarrier(); + const settleParams = { + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + } as const; + + // Pass one: the claim store cannot be written into, so the clear reaches no verdict and the settle + // is held back. The barrier is what makes this a COMPLETED pass rather than a started one. + fs.chmodSync(claimsDir, 0o500); + const heldBack = createSessionIdleExpiry(settleParams); + try { + heldBack.noteSessionsChanged(); + await sweeps.swept(1); + } finally { + fs.chmodSync(claimsDir, 0o700); + } + heldBack.cancel(); + + assert.notEqual(sessionStore.get('default'), undefined); + assert.equal( + session.scriptPublication?.kind === 'repair' ? session.scriptPublication.status : undefined, + 'complete', + "a settle that released nothing must not publish this session's repair transaction", + ); + assert.equal( + fs.existsSync(path.join(sessionStore.resolveSessionDir('default'), 'repair-tombstone.json')), + false, + 'and must not leave a repair tombstone for a transaction that never ended', + ); + + // Pass two, over the SAME live record: the session is still there, so this is the continuity that + // matters. A first pass that had finalized while held back would have stamped COMMITTED onto this + // very object, and this pass's write would then be the idempotent no-op that loses the script. + const retried = createSessionIdleExpiry(settleParams); + retried.noteSessionsChanged(); + await sweeps.swept(2); + retried.cancel(); + + assert.equal(sessionStore.get('default'), undefined, 'the reclaim that was owed does land'); + assert.equal( + session.scriptPublication?.kind === 'repair' ? session.scriptPublication.status : undefined, + 'committed', + 'and the held-back transaction is published by the pass that finally ends the session', + ); +}); + +test('a committed expiry finalizes the repair transaction it ends', async () => { + const fixture = makeFixture('agent-device-idle-expiry-repair-committed-'); + const { sessionStore } = fixture; + const { deviceClaim } = await sessionWithLiveClaim(fixture); + const session = makeRepairCompleteSession('default', { + createdAt: NOW - WINDOW_MS - 1, + deviceClaim: { ...deviceClaim }, + }); + sessionStore.set('default', session); + + const sweeps = createSweepBarrier(); + const controller = createSessionIdleExpiry({ + sessionStore, + idleExpiryMs: WINDOW_MS, + executionLocks: new Map(), + settleSession: async () => {}, + withinDiagnosticsScope: sweeps.withinDiagnosticsScope, + now: () => NOW, + }); + controller.noteSessionsChanged(); + await sweeps.swept(); + controller.cancel(); + + assert.equal(sessionStore.get('default'), undefined); + // ADR 0012 R7 binds an expiry like every other teardown: the healed script is committed, not + // discarded with the record. + assert.equal( + session.scriptPublication?.kind === 'repair' ? session.scriptPublication.status : undefined, + 'committed', + ); +}); diff --git a/src/daemon/server/daemon-session-idle-expiry.ts b/src/daemon/server/daemon-session-idle-expiry.ts new file mode 100644 index 0000000000..0ffd0f33bf --- /dev/null +++ b/src/daemon/server/daemon-session-idle-expiry.ts @@ -0,0 +1,573 @@ +import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; +import { clearDeviceClaim, type DeviceClaimClearOutcome } from '../device/device-claims.ts'; +import { existingSessionExecutionLockKeys } from '../request-binding.ts'; +import { withRequestExecutionLockKeys } from '../request-execution-locks.ts'; +import type { RequestExecutionLockKey } from '../request-binding.ts'; +import { + buildIdleExpiryTombstone, + isIdleExpirableSession, + isSessionIdleExpired, + lastActivityMs, + sessionIdleDeadlineMs, + type SessionIdleExpiryOutcome, +} from '../session-idle-expiry.ts'; +import type { SessionState } from '../session-state.ts'; +import type { SessionStore } from '../session-store.ts'; + +/** Settles one expired session's owned resources. Supplied by the runtime, which owns the seams. */ +export type IdleSessionSettler = (session: SessionState, sessionName: string) => Promise; + +/** The one outcome this reaper invents: the clear threw, so nothing about the claim is known. */ +const CLAIM_CLEAR_FAILED = 'claim-clear-failed'; + +/** + * Whether an outcome confirms this daemon no longer holds the device. A table rather than a + * comparison so a member added to `DeviceClaimClearOutcome` cannot default its way into "go ahead + * and forget the session": a new outcome has to declare itself here. + */ +const CLAIM_GONE: Readonly> = Object.freeze({ + deleted: true, + absent: true, + 'ownership-changed': true, + unattributable: false, +}); + +/** + * The largest delay a Node `setTimeout` can express. Anything above it is silently replaced by a + * 1 ms timer rather than rejected, which would turn a long window into a busy loop. + */ +const MAX_TIMER_DELAY_MS = 2_147_483_647; + +export type SessionIdleExpiryController = Readonly<{ + /** + * Re-arms the pending expiry from the current session set. Called after anything that can start or + * end a deadline: a session opened, a session closed, or a command finishing on one. A no-op while + * the policy is off, so the default configuration schedules nothing at all. + */ + noteSessionsChanged: () => void; + /** + * Stops scheduling and tells a sweep already running to stop starting settles. The composition + * calls this when the daemon begins to leave; it does not wait for a settle that had already + * started, because a daemon on its way out must not be held hostage by a teardown that is stuck — + * the process ending takes the stuck work with it. + */ + cancel: () => void; + readonly idleExpiryMs: number; +}>; + +/** + * The #2833 deadline's clock: one timer, always pointing at the earliest deadline in the session set. + * + * A timer rather than a lazy check at the next command, because a claim only frees its device once the + * daemon that took it releases it, and the agent that abandoned the session is by definition the one + * that stopped sending commands. Another worktree cannot help either: a claim is owned by the daemon + * that wrote it, so no other process may clear one, and #1320 keeps it that way. + * + * Every expiry runs under that session's execution lock pair — the session key AND the device key it + * is bound to, in the same order a request takes them — and re-checks the deadline inside them. That + * is what makes the two rules of this feature hold together: an admitted command holds the locks and + * re-stamps activity when it finishes, so a session in use is never caught mid-command, and the + * deadline it is measured against is always the one its last finished command set. Settling without + * the device key would release a device under a command that still holds it, and taking the two keys + * in any other order would deadlock against a request's pair. + */ +export function createSessionIdleExpiry(params: { + sessionStore: SessionStore; + idleExpiryMs: number; + executionLocks: Map>; + settleSession: IdleSessionSettler; + /** + * How long a sweep WAITS on one expiry. Bounds the wait only, never the expiry: the expiry is what + * holds this session's execution locks, and a budget that took those from under it would let a + * retried `close` join a teardown in progress. See `expireIdleSession`. + */ + settleBudgetMs?: (session: SessionState) => number; + /** + * Runs one sweep inside whatever diagnostics scope the composition owns. A sweep is out-of-request + * work, so without it `emitDiagnostic` is a no-op and every reclaim — and every failed reclaim — + * is unrecordable, following ADR 0018's session-scoped teardown scope. + */ + withinDiagnosticsScope?: (run: () => Promise) => Promise; + /** + * Reports a release that actually landed. Called by the expiry rather than by the sweep's wait, so a + * release that arrives after its budget elapsed still counts: it freed the device and the session is + * gone, and a composition that never learns has an idle daemon it believes is still in use. + */ + onSessionExpired?: (outcome: SessionIdleExpiryOutcome) => void; + now?: () => number; +}): SessionIdleExpiryController { + const { sessionStore, idleExpiryMs } = params; + const now = params.now ?? Date.now; + let timer: ReturnType | undefined; + let sweeping = false; + // `cancel()` is the daemon beginning to leave. It stops future sweeps, and it also tells a sweep + // already running not to START settling another session: shutdown tears the whole session set down + // without taking any execution lock, so it is the one remover that can finalize a session out from + // under a settle. A sweep already inside a settle cannot be recalled, and settles there because + // stopping mid-teardown would strand a resource; what it must not do is write an idle-expiry + // marker over a shutdown's close, which `SessionStore.delete`'s answer detects. + let closing = false; + // Addresses with a settle in flight. A settle whose budget expired stopped being WAITED on, not + // stopped: it keeps holding the session's execution lock until it actually finishes. Without this + // the next sweep would take the lock the moment that budget released and run a second teardown of + // the same session concurrently with the first. + const settling = new Set(); + // Sessions whose last settle attempt could not finish, and when the next one may run. A failed + // settle deliberately leaves the session and its claim in place, so without this the earliest + // deadline would stay in the past and the timer would refire with zero delay forever. One full + // window is the retry cadence: it is the only interval this feature is configured to care about. + const retryNotBeforeMs = new Map(); + + const clearTimer = (): void => { + if (!timer) return; + clearTimeout(timer); + timer = undefined; + }; + + /** Called by the composition when this daemon begins to leave. */ + const cancel = (): void => { + closing = true; + clearTimer(); + }; + + const nextDueMs = (): number | undefined => { + let next: number | undefined; + const liveAddresses = new Set(); + for (const ref of sessionStore.listRefs()) { + liveAddresses.add(ref.address); + const deadline = sessionIdleDeadlineMs(ref.session, idleExpiryMs); + if (deadline === undefined) continue; + const notBefore = retryNotBeforeMs.get(ref.address); + const due = notBefore === undefined ? deadline : Math.max(deadline, notBefore); + if (next === undefined || due < next) next = due; + } + // A session that left the store has nothing left to retry, and `close` never reaches the expiry + // that would have cleared it. An address re-created by `open` can inherit a deferral its new + // occupant never earned, and that costs at most one window: a fresh record's own deadline is a + // full window ahead anyway, so an inherited deferral never reaches past the one it was set for. + for (const address of retryNotBeforeMs.keys()) { + if (!liveAddresses.has(address)) retryNotBeforeMs.delete(address); + } + return next; + }; + + const arm = (): void => { + clearTimer(); + if (closing || idleExpiryMs <= 0 || sweeping) return; + const atMs = now(); + const deadline = nextDueMs(); + if (deadline === undefined) return; + const waitMs = Math.max(0, deadline - atMs); + // A Node timer cannot express anything past a signed 32-bit int: the delay is silently replaced + // by 1 ms rather than rejected. A window longer than ~24.8 days — which `resolveSessionIdleExpiryMs` + // accepts, because off is spelled `0` and a huge value is a real reading of "essentially never" — + // would therefore arm a sweep every millisecond, and each sweep would find the session inside its + // window and re-arm at the same 1 ms. So a wait beyond the range is taken in pieces, and a piece + // that ends with the deadline still ahead simply re-arms instead of sweeping. + const deadlineStillAheadOfThisPiece = waitMs > MAX_TIMER_DELAY_MS; + timer = setTimeout( + () => { + timer = undefined; + if (deadlineStillAheadOfThisPiece) arm(); + else void runSweep(); + }, + Math.min(waitMs, MAX_TIMER_DELAY_MS), + ); + // An armed reaper must never be what keeps a daemon's event loop alive: the daemon has its own + // lifetime rules (idle reap, lock, signal handlers) and this only frees a device sooner. + timer.unref?.(); + }; + + const runSweep = async (): Promise => { + sweeping = true; + const sweep = async (): Promise => { + try { + await expireIdleSessions({ + ...params, + now, + retryNotBeforeMs, + settling, + closing: () => closing, + notifyExpired: params.onSessionExpired, + }); + } catch (error) { + // A sweep is never allowed to reject into the daemon's unhandled-rejection path: that path + // shuts the daemon down, and a failed reclaim must not cost every other session its daemon. + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_sweep_failed', + data: { idleExpiryMs, error: error instanceof Error ? error.message : String(error) }, + }); + } + }; + try { + await (params.withinDiagnosticsScope ? params.withinDiagnosticsScope(sweep) : sweep()); + } finally { + sweeping = false; + arm(); + } + }; + + return { noteSessionsChanged: arm, cancel, idleExpiryMs }; +} + +type IdleExpirySweepParams = { + sessionStore: SessionStore; + idleExpiryMs: number; + executionLocks: Map>; + settleSession: IdleSessionSettler; + /** + * How long a sweep waits for one expiry. Optional because the composition owns the teardown budget. + * Bounds the wait only, never the expiry itself — see `expireIdleSession`. + */ + settleBudgetMs?: (session: SessionState) => number; + /** + * Reports a release that actually landed. Called by the expiry rather than by the sweep's wait, so a + * release that arrives after its budget elapsed still counts: it freed the device and the session is + * gone, and a composition that never learns has an idle daemon it believes is still in use. + */ + notifyExpired?: (outcome: SessionIdleExpiryOutcome) => void; + now: () => number; + retryNotBeforeMs: Map; + settling: Set; + closing: () => boolean; +}; + +async function expireIdleSessions(params: IdleExpirySweepParams): Promise { + for (const ref of params.sessionStore.listRefs()) { + if (!isIdleExpirableSession(ref.session)) continue; + // Shutdown is tearing every session down without taking a single execution lock, so a settle + // started now would only race it. + if (params.closing()) return; + await expireIdleSession({ ...params, ref }); + } +} + +/** + * Starts one expiry and waits for it as long as the budget allows. + * + * The budget bounds only this WAIT, never the expiry. The expiry is what holds the session's and the + * device's execution locks and what may free the device; detaching the wait is what keeps one stuck + * recorder from hanging a sweep, while keeping the locks with the expiry is what stops a budget from + * letting a `close` tear the same session down twice or letting a late release delete a session a + * retried `open` just created. Reporting belongs to the expiry rather than to this wait, so a release + * that lands after the budget still counts. + */ +async function expireIdleSession( + params: IdleExpirySweepParams & { ref: { address: string; session: SessionState } }, +): Promise { + const { address } = params.ref; + // A release still in flight holds this session's locks, so queueing on them would have the sweep wait + // for a stuck teardown instead of getting on with the rest of the session set. Skipping is only + // correct because it defers this address in the same breath: the deadline is still in the past, and a + // sweep that re-arms at a past deadline fires again the instant it is armed — a stuck release would + // then spin the event loop for its whole duration. One window is the same cadence a failed settle + // already asks for, and the release in flight sets its own retry clock when it finishes. + if (params.settling.has(address)) { + params.retryNotBeforeMs.set(address, params.now() + params.idleExpiryMs); + return; + } + // The device this expiry fences is read inside the lock pair rather than from the swept reference: + // a session can only change device while its own session lock is free, which this pair holds first. + // The pair comes from the same planner a command on this session uses, so the device key is what + // stops an expiry releasing a device another session's command still holds, and the order is what + // stops it deadlocking against a request holding the other one. + const lockKeys = existingSessionExecutionLockKeys(address, params.ref.session.device.id); + const expiry = settleIdleSessionUnderLock({ ...params, lockKeys }); + const budgetMs = params.settleBudgetMs?.(params.ref.session); + if (budgetMs === undefined) return void (await expiry); + const budget = budgetElapsedAfter(budgetMs); + try { + if ((await Promise.race([expiry, budget.promise])) !== BUDGET_ELAPSED) return; + } finally { + budget.cancel(); + } + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_settle_timeout', + data: { + session: address, + idleExpiryMs: params.idleExpiryMs, + settleBudgetMs: budgetMs, + ...(params.ref.session.deviceClaim + ? { deviceKey: params.ref.session.deviceClaim.deviceKey } + : {}), + }, + }); + // Deferred rather than cleared: the release is still running and may yet succeed, in which case the + // session disappears and the next sweep prunes this entry along with it. Measured from here — the + // moment this sweep stopped waiting — because the release may have been running for most of the + // window already, and deferring from when it STARTED would schedule the next pass in the past. + params.retryNotBeforeMs.set(address, params.now() + params.idleExpiryMs); +} + +const BUDGET_ELAPSED = Symbol('session-idle-settle-budget-elapsed'); + +/** + * A wait the expiry can win. Unref'd and cancellable because a sweep that stopped waiting must leave + * nothing behind: a ref'd timer here is a daemon that cannot exit, and an armed one is a timer the + * event loop carries for the whole budget after the session it was watching was released. + */ +function budgetElapsedAfter(ms: number): Readonly<{ + promise: Promise; + cancel: () => void; +}> { + let timer: ReturnType | undefined; + const promise = new Promise((resolve) => { + timer = setTimeout(() => resolve(BUDGET_ELAPSED), ms); + timer.unref?.(); + }); + return { promise, cancel: () => clearTimeout(timer) }; +} + +/** + * Runs one expiry from start to finish INSIDE the session's and the device's execution lock pair, and + * reports its outcome whether or not anyone is still waiting for it. + * + * The lock pair outlives the sweep's wait on purpose. A budget that released these locks would let a + * retried `close` join a teardown already in progress, and let the expiry's own final steps — delete + * this record, write the marker — land on a session a fresh `open` had just created at this address, + * orphaning that new session's claim. The `settling` fence spans the same span so no second expiry + * queues behind the first either, and lifts only when the release is genuinely over. + */ +async function settleIdleSessionUnderLock( + params: IdleExpirySweepParams & { + ref: { address: string; session: SessionState }; + lockKeys: readonly RequestExecutionLockKey[]; + }, +): Promise { + const { address, session } = params.ref; + params.settling.add(address); + let attempt: IdleSettleAttempt; + try { + attempt = await withRequestExecutionLockKeys( + params.executionLocks, + params.lockKeys, + async () => { + // Re-checked inside the pair: a settle that took this lock before `cancel()` can still be + // finishing its resources while shutdown runs, and a queued one must not begin then. + if (params.closing()) return NOTHING_TO_RETRY; + // Re-read under the locks rather than trusting the swept reference: the device may have moved, + // and the lock keys were chosen from the pre-lock reading. + const settled = params.sessionStore.get(address); + if (!settled) return NOTHING_TO_RETRY; + if (settled.device.id !== session.device.id) return NOTHING_TO_RETRY; + // Re-clocked here too: a command that admitted while this expiry was queuing has finished and + // re-stamped the session by now, so a deadline seen outside the locks is a hint, not a verdict. + const atMs = params.now(); + if (!isSessionIdleExpired(settled, params.idleExpiryMs, atMs)) return NOTHING_TO_RETRY; + const outcome = await settleExpiredSession({ + sessionName: address, + session: settled, + idleExpiryMs: params.idleExpiryMs, + expiredAtMs: atMs, + sessionStore: params.sessionStore, + settleSession: params.settleSession, + }); + return { outcome, retry: true }; + }, + ); + if (attempt.retry) { + rememberRetry(params, address, attempt.outcome); + } else { + // A session that is gone, moved, or back inside its window earned no deferral: it is the next + // command or the next deadline that decides what happens here, not a clock this sweep set. + params.retryNotBeforeMs.delete(address); + } + } catch (error) { + // The rejection is owned here: a budget that stopped waiting would otherwise hand the failure to + // the daemon's `unhandledRejection` handler, which spends a live daemon on one stuck recorder. + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_settle_failed', + data: { + session: address, + idleExpiryMs: params.idleExpiryMs, + error: error instanceof Error ? error.message : String(error), + }, + }); + rememberRetry(params, address, undefined); + return undefined; + } finally { + params.settling.delete(address); + } + if (attempt.outcome) params.notifyExpired?.(attempt.outcome); + return attempt.outcome; +} + +/** Whether a pass under the locks has anything to retry, and what it achieved if it has. */ +type IdleSettleAttempt = Readonly<{ + outcome: SessionIdleExpiryOutcome | undefined; + retry: boolean; +}>; + +/** + * A pass that decided not to settle — the session closed, moved to another device, or is back inside + * its window. Only an expiry that actually ran and could not finish is owed a retry clock. + */ +const NOTHING_TO_RETRY: IdleSettleAttempt = Object.freeze({ outcome: undefined, retry: false }); + +/** + * Gives an expiry that ran and could not finish a full window before the next attempt, and takes the + * deferral away from one that succeeded. Measured from NOW, at the moment the attempt ends, rather + * than from when the sweep started it: a release is bounded only in how long anyone waits for it, so + * one that outlasts the window would otherwise write a deferral already in the past and be retried + * with no gap at all — and would overwrite the fresher deferral a sweep that found it in flight had + * just computed for the same address. + */ +function rememberRetry( + params: Pick, + address: string, + outcome: SessionIdleExpiryOutcome | undefined, +): void { + if (outcome) { + params.retryNotBeforeMs.delete(address); + return; + } + params.retryNotBeforeMs.set(address, params.now() + params.idleExpiryMs); +} + +/** + * Settles one expired session: its owned resources, then its claim, then the repair transaction it + * ends, then its record, then the bounded marker that tells the next command what happened. + * + * The order matters three times over. The claim has to outlive the resource teardown, because a claim + * whose device still has an attached helper or a live execution host describes an unfinished release + * rather than a free device — the same rule `close` follows when it withholds the clear after a failed + * teardown. Repair finalization has to wait for the claim, because committing is one-way and a settle + * that is held back must leave the transaction committable by the pass that succeeds. And the marker is + * written once the session record is gone, because it is a diagnostic for the agent that comes next, + * never a reason to skip cleanup. + * + * A failed settle returns `undefined` and leaves the session and its claim exactly as they were: this + * daemon is staying alive, so it can retry; deleting the session anyway would leave a claim owned by + * a live process that no longer knows what it holds — un-reclaimable by `--stale`, which proves + * staleness from the owner's liveness, and strictly worse than not expiring at all. That argument + * binds the + * clear itself as hard as it binds the teardown: a clear that learned nothing — it threw, or left a + * record no owner can be attributed to — leaves the same stranded claim, so it holds the record back + * too and the next pass tries again. A clear reporting `ownership-changed` is confirmation that a + * successor owns the device now, and never blocks the expiry. + */ +async function settleExpiredSession(params: { + sessionName: string; + session: SessionState; + idleExpiryMs: number; + expiredAtMs: number; + sessionStore: SessionStore; + settleSession: IdleSessionSettler; +}): Promise { + const { sessionName, session, idleExpiryMs, expiredAtMs } = params; + const deviceKey = session.deviceClaim?.deviceKey; + const identity = { session: sessionName, idleExpiryMs, ...(deviceKey ? { deviceKey } : {}) }; + if (!(await releaseExpiredSessionResources(params, identity))) return undefined; + const claim = await clearExpiredClaim(sessionName, session.deviceClaim); + if (claim === CLAIM_CLEAR_FAILED || !CLAIM_GONE[claim]) { + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_device_claim_unconfirmed', + data: identity, + }); + return undefined; + } + // ADR 0012 R7 binds an expiry like every other teardown: a repair transaction this expiry catches + // commits its healed `.ad` iff COMPLETE and otherwise leaves its own tombstone, which the router + // already prefers over the idle-expiry marker's guidance. It runs AFTER the gate above rather than + // inside the settler, because publishing is part of ending the session: `finalizeRepairTeardown` + // stamps COMMITTED onto the record, and a write onto an already-committed transaction is an + // idempotent no-op. Finalizing a settle that is being held back would therefore mark a still-live + // session's healed script as already published, and no later teardown would ever publish it. + params.sessionStore.finalizeRepairTeardown(session); + // `delete` reports whether a record was still here to remove. Every request-path remover — `close`, + // a replacing `open`, a lease-expiry teardown — removes a session from inside `runAdmitted`, which + // holds the same lock pair this settle holds, and a settle budget bounds only the sweep's WAIT and + // never these locks, so no request can reach this record while the release is running. Daemon + // shutdown is the one remover that takes no lock at all, and it is therefore the only way here. + // The session was ended by someone else, and that owner has already explained it; a marker written + // now would tell the next agent the session died of idleness when something else closed it. + // + // The finalize above already published this session's repair transaction, which is why "a failed + // settle changes nothing" is not this function's contract: publishing belongs to whoever ENDS the + // session, and shutdown ends it by finalizing the same live record, onto which this commit is an + // idempotent no-op. Deferring the finalize to below this guard would instead resolve the healed + // script's event-log directory by map identity, which a deleted record no longer answers correctly. + if (!params.sessionStore.delete(sessionName)) { + emitDiagnostic({ + level: 'info', + phase: 'session_idle_expiry_superseded', + data: identity, + }); + return undefined; + } + // The marker is keyed by the store address: the one string that both names this session's artifact + // directory correctly and resolves back to this same session for the next command. Keying it by + // `session.name` would let two worktrees' `default` sessions overwrite each other's marker. + params.sessionStore.writeIdleExpiryTombstone( + sessionName, + buildIdleExpiryTombstone(sessionName, { expiredAtMs, idleExpiryMs, deviceKey }), + ); + const idleForMs = expiredAtMs - lastActivityMs(session); + emitDiagnostic({ + level: 'info', + phase: 'session_idle_expired', + data: { ...identity, idleForMs, claim }, + }); + return { + sessionName, + idleExpiryMs, + idleForMs, + claim, + ...(deviceKey ? { deviceKey } : {}), + }; +} + +/** + * Releases one expired session's owned resources, reporting whether the settle may proceed to the + * claim. A failure here is reported and swallowed rather than propagated: the caller's answer is the + * same either way — hold the record back and retry — and the distinguishing information belongs to + * this step's diagnostic. + */ +async function releaseExpiredSessionResources( + params: { session: SessionState; sessionName: string; settleSession: IdleSessionSettler }, + identity: Readonly>, +): Promise { + try { + await params.settleSession(params.session, params.sessionName); + return true; + } catch (error) { + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_cleanup_failed', + data: { ...identity, error: error instanceof Error ? error.message : String(error) }, + }); + return false; + } +} + +/** + * Releases the expired session's own claim. Two results say this daemon learned nothing about + * whether its claim is gone — the clear threw, and `unattributable`, where a record is still on disk + * that yields no attributable owner. Both hold the session record back: forgetting it would leave a + * claim owned by a live process that no longer knows what it holds, which `device release --stale` + * cannot reclaim because it proves staleness from the owner's liveness. + */ +async function clearExpiredClaim( + sessionName: string, + ownership: SessionState['deviceClaim'], +): Promise { + if (!ownership) return 'absent'; + try { + return await clearDeviceClaim(ownership); + } catch (error) { + emitDiagnostic({ + level: 'warn', + phase: 'session_idle_expiry_device_claim_clear_failed', + data: { + session: sessionName, + deviceKey: ownership.deviceKey, + error: error instanceof Error ? error.message : String(error), + }, + }); + return CLAIM_CLEAR_FAILED; + } +} diff --git a/src/daemon/server/daemon-shutdown-claims.test.ts b/src/daemon/server/daemon-shutdown-claims.test.ts index e679e65a84..018ebab09f 100644 --- a/src/daemon/server/daemon-shutdown-claims.test.ts +++ b/src/daemon/server/daemon-shutdown-claims.test.ts @@ -1,4 +1,4 @@ -import { expect, test } from 'vitest'; +import { afterEach, expect, test, vi } from 'vitest'; import { ANDROID_EMULATOR } from '../../__tests__/test-utils/device-fixtures.ts'; import { isolatedDeviceClaimStores, @@ -6,11 +6,16 @@ import { } from '../../__tests__/test-utils/device-claim-store.ts'; import fs from 'node:fs'; import { acquireDeviceClaim } from '../device/device-claims.ts'; +import { acquireAllocatorHeldDeviceClaim } from '../device/device-claim-allocator.ts'; import { resolveDeviceClaimPath } from '../device/device-claim-paths.ts'; import { inspectDeviceClaims } from '../device/device-claim-inspection.ts'; import { createDaemonShutdownClaimLedger } from './daemon-shutdown-claims.ts'; import type { SessionState } from '../session-state.ts'; +afterEach(() => { + vi.restoreAllMocks(); +}); + const setup = isolatedDeviceClaimStores('agent-device-shutdown-claim-ledger-'); function claimRecord(session: SessionState, name: string) { @@ -53,6 +58,7 @@ test('a claim cleared after clean teardown is reported released', async () => { released: [claimRecord(session, 'default')], orphaned: [], superseded: [], + unattributable: [], }); expect(inspectDeviceClaims({})).toEqual([]); }); @@ -97,6 +103,81 @@ test('a claim replaced by a successor owner is reported superseded, never releas expect(inspectDeviceClaims({}).map((entry) => entry.claim?.session)).toEqual(['successor']); }); +test('an undecodable record is reported unattributable, never orphaned or superseded', async () => { + const session = await claimedSession('unreadable'); + const deviceKey = session.deviceClaim?.deviceKey ?? ''; + // The record this daemon's claim points at can no longer be decoded into a process-owned claim. + // That is neither fact at once: nothing proves a successor took the device, so it is not + // `superseded`; and it is not `orphaned` either, because that bucket's remedy is + // `device release --stale`, which proves staleness from the recorded owner this record does not + // have. Claiming the bucket would send the operator to a command that refuses this record. + fs.writeFileSync(resolveDeviceClaimPath(deviceKey), '{bad json'); + + const ledger = createDaemonShutdownClaimLedger(); + await ledger.releaseClaim(session); + ledger.finalize(session); + + expect(ledger.claims).toEqual({ + released: [], + orphaned: [], + superseded: [], + unattributable: [claimRecord(session, 'unreadable')], + }); +}); + +test('an allocator-held record is reported unattributable, not released', async () => { + const session = await claimedSession('allocator'); + const deviceKey = session.deviceClaim?.deviceKey ?? ''; + // A record owned by an allocator principal is attributable to that installation, but not to any + // process this daemon's teardown can prove dead, and only the allocator's own removal proof clears + // it. So this daemon released nothing and cannot reconcile the record either. + fs.rmSync(resolveDeviceClaimPath(deviceKey)); + const held = await acquireAllocatorHeldDeviceClaim({ + device: ANDROID_EMULATOR, + principal: { + stateDir: '/installations/allocator', + instanceId: 'allocator-1', + identityIncarnationId: 'incarnation-1', + }, + }); + expect(held.status).toBe('acquired'); + + const ledger = createDaemonShutdownClaimLedger(); + await ledger.releaseClaim(session); + ledger.finalize(session); + + expect(ledger.claims).toEqual({ + released: [], + orphaned: [], + superseded: [], + unattributable: [claimRecord(session, 'allocator')], + }); + // The allocator's grant survives a daemon that cannot settle it. + expect(inspectDeviceClaims({}).map((entry) => entry.classification)).toEqual(['allocator-held']); +}); + +test('a claim clear that throws is reported orphaned, the one bucket with a working remedy', async () => { + const session = await claimedSession('unrecorded'); + const ledger = createDaemonShutdownClaimLedger(); + // The clear never returned a verdict at all. This is the state the `orphaned` advice is written + // for: our own owner identity dies with the exiting daemon, which is exactly the proof + // `device release --stale` needs, and the record still names us. + const unlink = vi.spyOn(fs, 'unlinkSync').mockImplementation(() => { + throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); + }); + + await ledger.releaseClaim(session); + unlink.mockRestore(); + ledger.finalize(session); + + expect(ledger.claims).toEqual({ + released: [], + orphaned: [claimRecord(session, 'unrecorded')], + superseded: [], + unattributable: [], + }); +}); + test('a session that never held a claim contributes nothing', async () => { const ledger = createDaemonShutdownClaimLedger(); const session: SessionState = { @@ -109,5 +190,10 @@ test('a session that never held a claim contributes nothing', async () => { await ledger.releaseClaim(session); ledger.finalize(session); - expect(ledger.claims).toEqual({ released: [], orphaned: [], superseded: [] }); + expect(ledger.claims).toEqual({ + released: [], + orphaned: [], + superseded: [], + unattributable: [], + }); }); diff --git a/src/daemon/server/daemon-shutdown-claims.ts b/src/daemon/server/daemon-shutdown-claims.ts index 7452838ca2..9d068fe3c7 100644 --- a/src/daemon/server/daemon-shutdown-claims.ts +++ b/src/daemon/server/daemon-shutdown-claims.ts @@ -8,6 +8,14 @@ export type DaemonShutdownClaims = { released: DeviceClaimRecord[]; orphaned: DeviceClaimRecord[]; superseded: DeviceClaimRecord[]; + /** + * A record this daemon's claim pointed at that yielded nothing attributable, so the daemon cannot + * say whether it still holds the device. Neither of the two buckets above tells the truth about it: + * it is not `superseded`, because nothing proves a successor took the device, and it is not + * `orphaned`, because that bucket's advice is `device release --stale` and that route proves + * staleness from a recorded owner — which is exactly what a record that never decoded does not have. + */ + unattributable: DeviceClaimRecord[]; }; export type DaemonShutdownClaimLedger = Readonly<{ @@ -18,6 +26,13 @@ export type DaemonShutdownClaimLedger = Readonly<{ finalize(session: SessionState): void; }>; +/** + * The clear threw, so this ledger holds no verdict for the session. Its own sentinel rather than an + * absent map entry, so the classifying switch must account for it by name alongside every real + * outcome. + */ +const CLEAR_UNRECORDED = 'clear-unrecorded'; + /** * #1320 claim results for `daemon stop`, classified from what clearing actually * did rather than from whether it threw: @@ -31,10 +46,23 @@ export type DaemonShutdownClaimLedger = Readonly<{ * claim of ours remains to reconcile), so it gets its own * bucket instead of being folded into a list whose meaning it * would break. + * - `unattributable` — a record remains where our claim was, but it decoded to + * no owner at all, so this daemon cannot say whether it still + * holds the device. It is NOT `orphaned`, because that bucket + * tells the operator to run `device release --stale`, and that + * route proves staleness from a recorded owner this record does + * not have; it refuses here and `device status --stale` hides + * it. Nor is it `superseded`, which would assert a successor + * this verdict explicitly refuses to assume. */ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger { - const claims: DaemonShutdownClaims = { released: [], orphaned: [], superseded: [] }; - const outcomes = new Map(); + const claims: DaemonShutdownClaims = { + released: [], + orphaned: [], + superseded: [], + unattributable: [], + }; + const outcomes = new Map(); return { claims, releaseClaim: async (session) => { @@ -42,7 +70,7 @@ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger { try { outcomes.set(session.name, await clearDeviceClaim(session.deviceClaim)); } catch (error) { - // An unrecorded outcome stays orphaned: the claim may still be on disk. + outcomes.set(session.name, CLEAR_UNRECORDED); emitDiagnostic({ level: 'warn', phase: 'daemon_shutdown_device_claim_release_failed', @@ -63,7 +91,10 @@ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger { platform: publicPlatformString(session.device), deviceId: session.device.id, }; - switch (outcomes.get(session.name)) { + // Exhaustive rather than defaulted: a member added to `DeviceClaimClearOutcome` has to declare + // which bucket it belongs to here, instead of arriving in `orphaned` unnoticed. + const outcome = outcomes.get(session.name); + switch (outcome) { case 'deleted': case 'absent': claims.released.push(record); @@ -71,9 +102,21 @@ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger { case 'ownership-changed': claims.superseded.push(record); return; - default: + case 'unattributable': + claims.unattributable.push(record); + return; + case CLEAR_UNRECORDED: + case undefined: + // The clear never reported: the claim may still be on disk. claims.orphaned.push(record); + return; + default: + assertDeclaredClaimOutcome(outcome); } }, }; } + +function assertDeclaredClaimOutcome(outcome: never): never { + throw new Error(`Undeclared device-claim outcome: ${String(outcome)}`); +} diff --git a/src/daemon/session-idle-expiry.ts b/src/daemon/session-idle-expiry.ts new file mode 100644 index 0000000000..721a52449a --- /dev/null +++ b/src/daemon/session-idle-expiry.ts @@ -0,0 +1,184 @@ +import { AppError } from '@agent-device/kernel/errors'; +import type { DeviceClaimClearOutcome } from './device/device-claims.ts'; +import type { SessionState } from './session-state.ts'; +import type { IdleSessionTombstone } from './session-idle-tombstone.ts'; + +/** + * #2833: an opt-in inactivity deadline for a session holding a local device claim, off by default. + * + * Agents sharing one host routinely finish a workflow without running `close`, so the host-global + * claim their session took stays live until that daemon stops; every other agent on the machine then + * reads `DEVICE_IN_USE` and cannot tell an active session from an abandoned one. A remote lease + * already answers this with an inactivity TTL (ADR 0007) because the remote daemon is authoritative + * over it. A local claim has no such authority, so only the daemon that OWNS the session may end it: + * this is that daemon expiring its own session, which is why it never crosses #1320's rule that a + * verified live *foreign* owner is not reclaimed for looking idle. Nothing here reads or reconciles + * another daemon's claim — `device release --stale` and the startup sweep stay the only paths that + * touch a foreign claim, and both keep their fail-closed behavior. + * + * Scope is the sessions whose ownership this daemon can actually settle, and it is a rule rather + * than a heuristic: + * + * - `deviceClaim` present — the host-global claim is the resource the issue is about, and a session + * with no claim is holding nothing another agent waits on. + * - `lease` absent — a session with a remote lease is governed by that lease's own inactivity TTL, + * which is authoritative and already enforced at admission. Running two deadlines over one + * session would make the earlier one responsible for a device the other owns. + * + * Off unless `AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS` is a positive number, because a false expiry + * tears down a live session, and on a shared host "idle" and "thinking" are indistinguishable. + */ +export const SESSION_IDLE_EXPIRY_ENV = 'AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS'; + +/** The typed reason naming why an absent session is absent. Keyed behavior, never message text. */ +const SESSION_IDLE_EXPIRED_REASON = 'SESSION_IDLE_EXPIRED'; + +/** A marker lives long enough for the command that comes next, not for an unrelated future session. */ +const IDLE_EXPIRY_TOMBSTONE_TTL_MS = 60 * 60_000; + +/** + * The inactivity window in force for this daemon, or `0` when the feature is off. Unset, + * unparseable, and non-positive all mean off; `0` is the documented "run until closed". + * + * A positive value is never allowed to round down into off: `0.4` is an operator asking for the + * shortest window the unit supports, not for the feature being disabled, and silently disabling a + * feature someone just opted into is the worst possible reading of a typo. + */ +export function resolveSessionIdleExpiryMs(env: NodeJS.ProcessEnv = process.env): number { + const raw = env[SESSION_IDLE_EXPIRY_ENV]?.trim(); + if (!raw) return 0; + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed <= 0) return 0; + return Math.max(1, Math.floor(parsed)); +} + +/** + * The instant this session last did something that counts as using it: the end of the last command + * admitted against it, or its own creation when nothing has finished since. `createdAt` is the base + * so the exact abandonment this exists for — `open`, then silence — expires on its own. + * + * Measured from a command's END, matching what a lease does for admitted work (ADR 0007): one + * command that outlives the window must not be the reason its own session is taken away mid-flight. + */ +export function lastActivityMs(session: SessionState): number { + return session.lastActivityAtMs ?? session.createdAt; +} + +/** + * The sessions this daemon may expire for idleness: a claim to release, no remote lease owning the + * session's ownership instead, and no capture running that this daemon itself started. + * + * The capture exclusion is the recording rule, generalized. `record`, `logs start`, `audio start`, + * `perf start`, and `trace start` each stamp the session once and then go silent while the capture + * runs, so a deadline measured from commands alone would call that idle and destroy evidence the + * workflow is still collecting — the same evidence the daemon-process reap honors with + * `hasActiveRecording`. It is narrower than that reap needs (a reapable daemon has no open session + * left to hold) and exactly as wide as an expiring daemon must be, because here the session IS the + * thing under the deadline. An abandoned capture is reclaimed by daemon exit and the startup orphan + * reap, not here. + */ +export function isIdleExpirableSession(session: SessionState): boolean { + return ( + session.deviceClaim !== undefined && + session.lease === undefined && + session.screenRecording === undefined && + session.appLog === undefined && + session.audioProbe === undefined && + session.perfCapture === undefined && + session.trace === undefined + ); +} + +/** Whether the inactivity window has elapsed since this session was last used. */ +export function idleDeadlineExceeded( + session: SessionState, + idleExpiryMs: number, + now: number = Date.now(), +): boolean { + if (idleExpiryMs <= 0) return false; + return now - lastActivityMs(session) >= idleExpiryMs; +} + +/** + * The full expiry verdict for one session: expirable in kind AND past its deadline. This is the + * predicate the reaper re-checks under the session's execution lock, so the eligibility rule and the + * clock can never be applied by half at the moment that matters. + */ +export function isSessionIdleExpired( + session: SessionState, + idleExpiryMs: number, + now: number = Date.now(), +): boolean { + return isIdleExpirableSession(session) && idleDeadlineExceeded(session, idleExpiryMs, now); +} + +/** The instant this session's deadline falls due, or `undefined` when nothing is armed. */ +export function sessionIdleDeadlineMs( + session: SessionState, + idleExpiryMs: number, +): number | undefined { + if (!isIdleExpirableSession(session) || idleExpiryMs <= 0) return undefined; + return lastActivityMs(session) + idleExpiryMs; +} + +/** + * One expired session's claim result, classified the way `daemon stop` classifies claims: `deleted` + * or `absent` means the claim is confirmed gone, and `ownership-changed` means another owner already + * replaced it. A clear that could confirm nothing never reaches this record — it holds the expiry + * back so the next pass retries — which is why there is no failure member here. + */ +export type SessionIdleExpiryOutcome = Readonly<{ + sessionName: string; + idleExpiryMs: number; + idleForMs: number; + claim: DeviceClaimClearOutcome; + deviceKey?: string; +}>; + +export function buildIdleExpiryTombstone( + sessionName: string, + record: Readonly<{ expiredAtMs: number; idleExpiryMs: number; deviceKey?: string }>, +): IdleSessionTombstone { + return { + owner: sessionName, + expiredAtMs: record.expiredAtMs, + expiresAt: record.expiredAtMs + IDLE_EXPIRY_TOMBSTONE_TTL_MS, + idleExpiryMs: record.idleExpiryMs, + ...(record.deviceKey ? { deviceKey: record.deviceKey } : {}), + }; +} + +/** + * The answer a caller gets when it addresses a session an earlier expiry already settled: still + * `SESSION_NOT_FOUND`, because the session genuinely is gone, but carrying the reason, the window, + * and the device that was released instead of a bare "Run open first". + */ +export function sessionIdleExpiredError( + sessionName: string, + tombstone: IdleSessionTombstone, + now: number = Date.now(), +): AppError { + const sinceExpiredMs = Math.max(0, now - tombstone.expiredAtMs); + return new AppError( + 'SESSION_NOT_FOUND', + `Session "${tombstone.owner}" expired ${formatIdle(sinceExpiredMs)} ago for idleness (window ${formatIdle(tombstone.idleExpiryMs)}). Run open again to start a new session.`, + { + reason: SESSION_IDLE_EXPIRED_REASON, + session: sessionName, + idleExpiryMs: tombstone.idleExpiryMs, + ...(tombstone.deviceKey ? { deviceKey: tombstone.deviceKey } : {}), + hint: tombstone.deviceKey + ? `Run open again to claim ${tombstone.deviceKey} and start a new session. Set ${SESSION_IDLE_EXPIRY_ENV}=0 to keep sessions open indefinitely.` + : `Run open again to start a new session. Set ${SESSION_IDLE_EXPIRY_ENV}=0 to keep sessions open indefinitely.`, + }, + ); +} + +/** Human-scale durations for the message; the exact milliseconds ride in `details`. */ +function formatIdle(ms: number): string { + const seconds = ms / 1000; + if (seconds < 90) return `${Math.round(seconds)}s`; + const minutes = seconds / 60; + if (minutes < 90) return `${Math.round(minutes)}m`; + return `${Math.round(minutes / 60)}h`; +} diff --git a/src/daemon/session-idle-tombstone.test.ts b/src/daemon/session-idle-tombstone.test.ts new file mode 100644 index 0000000000..95683a22cb --- /dev/null +++ b/src/daemon/session-idle-tombstone.test.ts @@ -0,0 +1,37 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import { + readIdleSessionTombstoneFile, + resolveIdleSessionTombstonePath, +} from './session-idle-tombstone.ts'; +import { mkdtempForTestSync } from '../__tests__/test-utils/tmp-dir.ts'; + +test('a marker whose lifetime cannot be represented stops explaining the absence', () => { + const markerPath = resolveIdleSessionTombstonePath( + mkdtempForTestSync('agent-device-idle-tombstone-range-'), + ); + + // `JSON.parse` turns this literal into `Infinity`, which is greater than every clock reading. A + // reader that only asks "is it a number, and still ahead?" accepts it, and one damaged file then + // owns this session key forever. + fs.writeFileSync( + markerPath, + '{"owner":"default","expiredAtMs":1,"expiresAt":1e400,"idleExpiryMs":1000}\n', + ); + assert.equal(readIdleSessionTombstoneFile(markerPath), undefined); + + fs.writeFileSync( + markerPath, + '{"owner":"default","expiredAtMs":1,"expiresAt":9e15,"idleExpiryMs":1e400}\n', + ); + assert.equal(readIdleSessionTombstoneFile(markerPath), undefined); + + // The device key is quoted back to the agent as the device to re-claim, so a value the writer could + // not have produced must not travel into that sentence. + fs.writeFileSync( + markerPath, + '{"owner":"default","expiredAtMs":1,"expiresAt":9e15,"idleExpiryMs":1000,"deviceKey":7}\n', + ); + assert.equal(readIdleSessionTombstoneFile(markerPath), undefined); +}); diff --git a/src/daemon/session-idle-tombstone.ts b/src/daemon/session-idle-tombstone.ts new file mode 100644 index 0000000000..0cdb2c569a --- /dev/null +++ b/src/daemon/session-idle-tombstone.ts @@ -0,0 +1,67 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +/** + * #2833: the bounded marker an idle-expired session leaves behind, so the next command that finds no + * session on that key answers `SESSION_NOT_FOUND` with a typed `details.reason` naming why the + * session is gone and its device released, instead of the bare "Run open first" that reads like the + * agent never opened anything. + * + * Same shape and lifetime as the repair tombstone (`src/session-repair-tombstone.ts`): keyed by the + * session's own store key, and bounded by `expiresAt` so an old marker never shadows an unrelated + * future session that happens to reuse the name. + */ +export type IdleSessionTombstone = { + owner: string; + expiredAtMs: number; + expiresAt: number; + /** The inactivity window that was in force when the session was expired. */ + idleExpiryMs: number; + /** The device the expired session released, when it held a host-global claim. */ + deviceKey?: string; +}; + +const IDLE_SESSION_TOMBSTONE_FILENAME = 'idle-expiry.json'; + +/** The tombstone file inside one session directory. Single owner of the file name. */ +export function resolveIdleSessionTombstonePath(sessionDir: string): string { + return path.join(sessionDir, IDLE_SESSION_TOMBSTONE_FILENAME); +} + +/** + * Parses/validates a tombstone file at `tombstonePath`; `undefined` if missing, malformed, or expired. + * + * Every field is checked against the range the writer could have produced rather than just its type, + * because `JSON.parse` accepts numbers no clock can hold: `1e400` parses to `Infinity`, which is after + * every date and would let one damaged marker explain this session key's absences forever. The device + * key is checked for the same reason — it is quoted back to the agent as the device to re-claim, so a + * value the writer never produces must not reach that sentence. + */ +export function readIdleSessionTombstoneFile( + tombstonePath: string, +): IdleSessionTombstone | undefined { + let raw: string; + try { + raw = fs.readFileSync(tombstonePath, 'utf8'); + } catch { + return undefined; + } + let parsed: IdleSessionTombstone; + try { + parsed = JSON.parse(raw) as IdleSessionTombstone; + } catch { + return undefined; + } + if (typeof parsed?.expiresAt !== 'number' || !Number.isFinite(parsed.expiresAt)) return undefined; + if (parsed.expiresAt <= Date.now()) return undefined; + if (typeof parsed.owner !== 'string') return undefined; + if (!isFiniteTimestamp(parsed.expiredAtMs) || !isFiniteTimestamp(parsed.idleExpiryMs)) { + return undefined; + } + if (parsed.deviceKey !== undefined && typeof parsed.deviceKey !== 'string') return undefined; + return parsed; +} + +function isFiniteTimestamp(value: unknown): value is number { + return typeof value === 'number' && Number.isFinite(value) && value >= 0; +} diff --git a/src/daemon/session-state.ts b/src/daemon/session-state.ts index 354b928636..b829717e5a 100644 --- a/src/daemon/session-state.ts +++ b/src/daemon/session-state.ts @@ -137,6 +137,15 @@ export type SessionState = { clientId?: string; expiresAt?: number; }; + /** + * #2833: the instant a command that attaches to this session last finished, which is what the + * opt-in inactivity deadline for a claim-holding session is measured from. Absent until then, in + * which case the session's own `createdAt` is the base — so the abandonment this exists for + * (`open`, then silence) still expires on its own. Written only through `SessionStore + * .noteSessionActivity`, under the session's execution lock. A session with a remote lease is + * governed by that lease's own `expiresAt` instead and is never measured from this field. + */ + lastActivityAtMs?: number; /** Enforced host-global local-device claim owned by this session, if acquired. */ deviceClaim?: { deviceKey: string; diff --git a/src/daemon/session-store.ts b/src/daemon/session-store.ts index 3d43d61fda..1598dd9414 100644 --- a/src/daemon/session-store.ts +++ b/src/daemon/session-store.ts @@ -14,6 +14,11 @@ import { resolveRepairTombstonePath, type RepairSessionTombstone, } from '../session-repair-tombstone.ts'; +import { + readIdleSessionTombstoneFile, + resolveIdleSessionTombstonePath, + type IdleSessionTombstone, +} from './session-idle-tombstone.ts'; import { NO_SCRIPT_PUBLICATION, isRepairCommittable } from './session-script-publication-state.ts'; import { effectiveWriteForce } from './session-script-publication-capability.ts'; import { @@ -66,7 +71,14 @@ export class SessionStore { * documents intent rather than committing anything; a genuinely new record needs it. */ set(name: string, session: SessionState): void { + // A key with no record is a NEW occupant, and the previous occupant's idle-expiry marker must + // stop explaining this key's absences from now on. Clearing it here rather than at `open` covers + // every way a record arrives — `open`'s provisional record, a record-only `record` session — and + // cannot be forgotten by a future insertion path. A replacing `open` on a live session takes the + // other branch and keeps whatever marker that session will earn for itself. + const occupying = this.sessions.has(name); this.sessions.set(name, session); + if (!occupying) this.clearIdleExpiryTombstone(name); } delete(name: string): boolean { @@ -276,6 +288,88 @@ export class SessionStore { } catch {} } + /** + * #2833: records the instant a command that attaches to this session finished, which is the moving + * signal the opt-in inactivity deadline for a claim-holding session is measured from. The store + * owns the field, so the request path reports the event without becoming a `SessionState` writer. + * Callers hold the session's execution lock, which is what makes one plain assignment enough. + * + * An unknown address is ignored rather than fatal: the common one is an `open` whose session was + * never built, and its own `createdAt` already starts that session's deadline clock. + */ + noteSessionActivity(address: string, atMs: number = Date.now()): void { + const session = this.sessions.get(address); + if (!session) return; + session.lastActivityAtMs = atMs; + } + + /** + * #2833: drops the bounded marker an idle-expired session leaves, so the next command on that key + * learns why its session is gone instead of being told to run `open`. Best effort — the expiry + * already happened, and a marker that cannot be written must not undo it. + */ + writeIdleExpiryTombstone(sessionName: string, tombstone: IdleSessionTombstone): void { + try { + fs.mkdirSync(this.resolveSessionDir(sessionName), { recursive: true }); + fs.writeFileSync( + resolveIdleSessionTombstonePath(this.resolveSessionDir(sessionName)), + `${JSON.stringify(tombstone)}\n`, + ); + } catch (error) { + emitDiagnostic({ + level: 'warn', + phase: 'idle_expiry_tombstone_write_failed', + data: { + session: sessionName, + error: error instanceof Error ? error.message : String(error), + }, + }); + } + } + + /** + * #2833: the non-expired idle-expiry marker this exact session key left when it was expired, or + * `undefined`. Total by design: a name that cannot address a session directory has no marker rather + * than the `INVALID_ARGS` `resolveSessionDir` would raise, because this read runs on an error path + * where a throw would replace the caller's own failure with an internal one. + * + * The recorded owner has to be the key being asked about, not merely a session that shares its + * directory. A session name becomes a directory through `safeSessionName`, which is a many-to-one + * encoding — the same one every other session artifact shares, and one no marker may quietly fork + * from — so two distinct keys can land in one directory. Answering for either of them with the + * other's expiry would report the wrong window and, worse, the wrong device as the one this caller + * just lost. Absent rather than borrowed is the safe answer. + */ + readIdleExpiryTombstone(sessionName: string): IdleSessionTombstone | undefined { + if (!isSafeSessionSegment(sessionName)) return undefined; + const tombstone = readIdleSessionTombstoneFile( + resolveIdleSessionTombstonePath(this.resolveSessionDir(sessionName)), + ); + return tombstone?.owner === sessionName ? tombstone : undefined; + } + + /** + * #2833: a fresh `open` on this key clears the idle-expiry marker, so a later `SESSION_NOT_FOUND` + * for a DIFFERENT removal of this session (an explicit `close`, a lease expiry) can't borrow the + * old expiry's explanation. Best effort, like the write. + */ + clearIdleExpiryTombstone(sessionName: string): void { + try { + fs.rmSync(resolveIdleSessionTombstonePath(this.resolveSessionDir(sessionName)), { + force: true, + }); + } catch (error) { + emitDiagnostic({ + level: 'warn', + phase: 'idle_expiry_tombstone_clear_failed', + data: { + session: sessionName, + error: error instanceof Error ? error.message : String(error), + }, + }); + } + } + private repairTombstonePath(sessionName: string): string { return resolveRepairTombstonePath(this.resolveSessionDir(sessionName)); }