From 3f1b9e7da3ed3eac968731e00f980dd05f90b6c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 08:37:39 +0200 Subject: [PATCH 1/2] feat(daemon): opt-in inactivity expiry for idle local device claims #2833: on a host shared by several agents, a session that never runs `close` keeps its host-global device claim until the daemon stops, and every other agent reads DEVICE_IN_USE with no way to tell active from abandoned. Set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to expire a claim-holding session with no remote lease and no capture running for it, once it has taken no attached commands for that long. The daemon that owns the session settles it under that session's own execution lock pair, releases the claim, and leaves a bounded marker so the next command answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the released device. Off by default; a positive window below one millisecond is the shortest window, not off. A settle that cannot confirm its claim gone holds the session record back and retries a window later: this daemon stays alive, so forgetting the record would strand a claim owned by a process that no longer knows what it holds, which `device release --stale` cannot reclaim. `clearDeviceClaim` therefore gains `unattributable` for a record that yields no attributable owner, because that says nothing about a successor having taken the device. The lock pair belongs to the release, not to the sweep's wait for it. A teardown budget bounds only that wait, so one stuck recorder cannot hang a sweep while a budget releasing the locks would let a retried `close` join a teardown in progress, or a late release delete a session a retried `open` just created and mark it expired. A sweep that finds a release in flight defers that address rather than re-arming at a deadline already past, and a release that lands after its budget still reports the session it freed. --- src/cli/commands/__tests__/daemon.test.ts | 48 +- src/cli/commands/daemon.ts | 29 +- src/commands/schema/cli-help-topics.test.ts | 1 + src/commands/schema/cli-help.ts | 1 + .../application-lifecycle-recovery.test.ts | 24 + .../__tests__/daemon-shutdown-report.test.ts | 36 +- .../filesystem-boundary-faults.test.ts | 2 +- .../request-router-idle-expired.test.ts | 147 ++++ .../__tests__/session-idle-activity.test.ts | 183 +++++ .../__tests__/session-idle-expiry-harness.ts | 178 +++++ .../__tests__/session-idle-expiry.test.ts | 176 +++++ src/daemon/__tests__/session-store.test.ts | 69 ++ src/daemon/application-lifecycle-recovery.ts | 15 +- src/daemon/daemon-shutdown-report.ts | 19 +- src/daemon/daemon-stop.ts | 9 + .../device/__tests__/device-claims.test.ts | 25 +- src/daemon/device/device-claims.ts | 20 +- src/daemon/request-binding.ts | 21 +- src/daemon/request-execution-locks.ts | 8 +- src/daemon/request-execution-scope.ts | 78 +- src/daemon/request-router.ts | 56 +- src/daemon/server/daemon-runtime.ts | 78 ++ ...mon-session-idle-expiry-scheduling.test.ts | 698 ++++++++++++++++++ .../server/daemon-session-idle-expiry.test.ts | 378 ++++++++++ .../server/daemon-session-idle-expiry.ts | 573 ++++++++++++++ .../server/daemon-shutdown-claims.test.ts | 90 ++- src/daemon/server/daemon-shutdown-claims.ts | 53 +- src/daemon/session-idle-expiry.ts | 184 +++++ src/daemon/session-idle-tombstone.test.ts | 37 + src/daemon/session-idle-tombstone.ts | 67 ++ src/daemon/session-state.ts | 9 + src/daemon/session-store.ts | 94 +++ 32 files changed, 3344 insertions(+), 62 deletions(-) create mode 100644 src/daemon/__tests__/request-router-idle-expired.test.ts create mode 100644 src/daemon/__tests__/session-idle-activity.test.ts create mode 100644 src/daemon/__tests__/session-idle-expiry-harness.ts create mode 100644 src/daemon/__tests__/session-idle-expiry.test.ts create mode 100644 src/daemon/server/daemon-session-idle-expiry-scheduling.test.ts create mode 100644 src/daemon/server/daemon-session-idle-expiry.test.ts create mode 100644 src/daemon/server/daemon-session-idle-expiry.ts create mode 100644 src/daemon/session-idle-expiry.ts create mode 100644 src/daemon/session-idle-tombstone.test.ts create mode 100644 src/daemon/session-idle-tombstone.ts 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)); } From 6eeab911b5182fa948a2f50a74c795789efda0e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 15:56:13 +0200 Subject: [PATCH 2/2] chore(gates): declare lastActivityAtMs as a store-owned SessionState field R7 holds every SessionState field to one declared writer. The #2833 request path reports activity through `SessionStore.noteSessionActivity` rather than mutating the record itself, so the store that owns the record is the only writer here and the field now says so. --- scripts/layering/session-state.ts | 3 +++ 1 file changed, 3 insertions(+) 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',