fix(daemon): let admitted work keep the lease it is working on (#2517)

* fix(daemon): let admitted work keep the lease it is working on

A lease renewed only at admission, so a command slower than its own inactivity TTL
expired the lease that was paying for the device it was using. Expiry then tore the
provider session down underneath a client still waiting for that same command's
result, and every later command on the session reported a lease that was no longer
active. The session's own work was the thing that killed it.

Admitted work now preserves its lease the way a human-control hold does: while the
request is still wanted it defers expiry, and finishing while still wanted renews
the lease for its existing TTL from the moment the work ended. Work whose client
hung up preserves nothing — it neither defers expiry past that cancellation nor
renews the lease when it finally lands — so a handler that ignores its cancellation
cannot hold a rented device open.

Found while investigating #2509. Not its reported mechanism: a cloud WebDriver
connection profile asks for a ten-minute lease, so a one-minute hang cannot starve
it. This reaches leases on the daemon's one-minute default.

* fix(daemon): drop a released lease's work claims instead of leaving them empty

A completed pass emptied its set but left the key behind, and only the expiry sweep
removed keys — which never reads a released lease again. Every connect-and-close that
ran a command on a leased device left another permanent entry in the daemon.

Releasing the last pass now removes its lease's entry, and releasing the lease drops
its claims outright and disarms the passes still running on them, so work that
outlives its own lease renews nothing.

* docs(daemon): state the lease invariant without borrowing #2509's cause

Four comments told the report's story as though it were this mechanism. A cloud
WebDriver connection profile asks for a ten-minute lease, so the reported one-minute
hang cannot have expired anything. The invariant stands on its own; where it came from
and which leases it reaches belong in ADR 0007 and the commit, not in each test.

Author: thymikee

docs/adr/0007-remote-device-leases.md

@@ -51,6 +51,22 @@ owning lease expiry.
 Backend-only leases remain valid for older remote clients, while provider-aware
 clients get device-level contention and clearer recovery.
 
+## Admitted request work
+
+A lease renews when a request is admitted and never again while that request runs, so an admitted
+command slower than the inactivity TTL used to expire the lease that was paying for its own device
+and tear the session down underneath the client still waiting for its result. Admitted work
+therefore preserves its lease the way a human-control hold does: while the request is still wanted
+it defers expiry, and finishing while still wanted renews the lease for its existing inactivity TTL
+from the moment the work ended. A request whose client hung up preserves nothing — it neither defers
+expiry past that cancellation nor renews the lease when it finally lands — so a handler that ignores
+its cancellation cannot hold a rented device open.
+
+Which leases this reaches depends on the inactivity TTL the client asked for: the daemon default is
+one minute, while a cloud WebDriver connection profile asks for ten. A single command that runs
+longer than its own lease is therefore ordinary on the default and only reachable through a profile
+on the longer one.
+
 ## Human control
 
 Human-control holds coexist with an open remote session. They belong to `LeaseRegistry` and use

src/daemon/__tests__/lease-in-flight-work.test.ts

@@ -0,0 +1,85 @@
+import { test, expect } from 'vitest';
+import { LeaseInFlightWorkRegistry } from '../lease-in-flight-work.ts';
+
+test('a pass defers its lease until it is released', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  const pass = work.retain('lease-a', () => true);
+
+  expect(work.isDeferred('lease-a')).toBe(true);
+  expect(pass.release()).toBe(true);
+  expect(work.isDeferred('lease-a')).toBe(false);
+});
+
+// The deferral is a claim that somebody is still waiting. The client hanging up
+// ends the claim immediately, without waiting for the abandoned work to unwind.
+test('a pass whose request was cancelled stops deferring unreleased', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  let wanted = true;
+  const pass = work.retain('lease-a', () => wanted);
+  wanted = false;
+
+  expect(work.isDeferred('lease-a')).toBe(false);
+  expect(pass.release()).toBe(false);
+});
+
+// Two requests can work one leased device. Only work still wanted defers, and one
+// release must not disturb a pass that outlives it.
+test('one wanted pass keeps deferral while an abandoned sibling releases', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  let abandonedWanted = true;
+  const abandoned = work.retain('lease-a', () => abandonedWanted);
+  const wanted = work.retain('lease-a', () => true);
+  abandonedWanted = false;
+
+  expect(abandoned.release()).toBe(false);
+  expect(work.isDeferred('lease-a')).toBe(true);
+  expect(wanted.release()).toBe(true);
+  expect(work.isDeferred('lease-a')).toBe(false);
+});
+
+test('releasing a pass twice renews nothing twice', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  const pass = work.retain('lease-a', () => true);
+
+  expect(pass.release()).toBe(true);
+  expect(pass.release()).toBe(false);
+});
+
+test('passes on different leases defer independently', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  const other = work.retain('lease-b', () => true);
+
+  expect(work.isDeferred('lease-a')).toBe(false);
+  expect(work.isDeferred('lease-b')).toBe(true);
+  other.release();
+});
+
+// Leases churn with every connect and close, and a released lease is never read by
+// the expiry sweep again. A key left behind per released lease is a permanent claim
+// on memory the daemon can never reclaim.
+test('releasing the last pass leaves no claim recorded for its lease', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  const first = work.retain('lease-a', () => true);
+  const second = work.retain('lease-b', () => true);
+
+  first.release();
+  second.release();
+
+  expect(claimedLeaseIds(work)).toEqual([]);
+});
+
+// Work that outlives its own lease renews nothing, and holds no key either.
+test('forgetting a lease releases its claims and disarms the passes running on it', () => {
+  const work = new LeaseInFlightWorkRegistry();
+  const pass = work.retain('lease-a', () => true);
+
+  work.forget('lease-a');
+
+  expect(claimedLeaseIds(work)).toEqual([]);
+  expect(pass.release()).toBe(false);
+});
+
+function claimedLeaseIds(work: LeaseInFlightWorkRegistry): string[] {
+  const claims = (work as unknown as { entriesByLeaseId: Map<string, unknown> }).entriesByLeaseId;
+  return [...claims.keys()];
+}

src/daemon/__tests__/lease-registry.test.ts

@@ -564,3 +564,93 @@ test('canceling a superseded activation cannot remove its successor or another h
   );
   assert.equal(registry.listHumanControlHolds(authority)[0]?.reason, 'successor');
 });
+
+// Nothing heartbeats a lease while its request works, so an admitted capture
+// that legitimately outruns the lease TTL expires its own lease and loses the
+// device it was still holding. In-flight work defers expiry and renews on
+// completion, exactly as a human-control hold does.
+test('in-flight request work defers expiry and renews the lease when it finishes', async () => {
+  let now = 0;
+  const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
+  const lease = registry.allocateLease(HUMAN_CONTROL_LEASE_REQUEST);
+  const pass = registry.retainLeaseWork(lease, () => true);
+
+  now = 9_000;
+  assert.deepEqual(registry.consumeExpiredLeases(), []);
+  assert.equal(registry.consumeExpiredLease(lease.leaseId), undefined);
+  assert.equal(registry.listActiveLeases()[0]?.leaseId, lease.leaseId);
+
+  pass.release();
+  assert.equal(registry.listActiveLeases()[0]?.expiresAt, 14_000);
+  now = 14_000;
+  assert.equal(registry.consumeExpiredLeases()[0]?.leaseId, lease.leaseId);
+});
+
+// The deferral belongs to work somebody is still waiting for. A request whose
+// client hung up stops deferring the moment it is cancelled, and re-earns nothing
+// when it finally lands, so a handler that ignores cancellation cannot pin a
+// rented device open forever.
+test('a cancelled request stops deferring expiry and cannot revive its lease', async () => {
+  let now = 0;
+  const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
+  const lease = registry.allocateLease(HUMAN_CONTROL_LEASE_REQUEST);
+  let wanted = true;
+  const pass = registry.retainLeaseWork(lease, () => wanted);
+
+  wanted = false;
+  now = 9_000;
+  assert.deepEqual(
+    registry.consumeExpiredLeases().map((entry) => entry.leaseId),
+    [lease.leaseId],
+  );
+  pass.release();
+  assert.deepEqual(registry.listActiveLeases(), []);
+});
+
+// The closest negative: two requests on one device, one abandoned. Releasing the
+// abandoned work must renew nothing while the wanted work still defers expiry.
+test('abandoned work renews nothing while work still wanted holds the lease', async () => {
+  let now = 0;
+  const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
+  const lease = registry.allocateLease(HUMAN_CONTROL_LEASE_REQUEST);
+  let abandonedWanted = true;
+  const abandoned = registry.retainLeaseWork(lease, () => abandonedWanted);
+  const wanted = registry.retainLeaseWork(lease, () => true);
+
+  abandonedWanted = false;
+  now = 9_000;
+  assert.equal(registry.listActiveLeases()[0]?.leaseId, lease.leaseId);
+
+  abandoned.release();
+  assert.equal(
+    registry.listActiveLeases()[0]?.expiresAt,
+    5_000,
+    'abandoned work must not renew the lease it was sitting on',
+  );
+
+  wanted.release();
+  assert.equal(registry.listActiveLeases()[0]?.expiresAt, 14_000);
+});
+
+// The ordinary release path, not expiry: a released lease is never read by the
+// expiry sweep again, so a work claim left against it would never be cleaned.
+test('releasing a lease drops the work claims recorded against it', () => {
+  const registry = new LeaseRegistry({ defaultLeaseTtlMs: 5_000 });
+  const lease = registry.allocateLease({ tenantId: 'tenant-a', runId: 'run-9' });
+  const pass = registry.retainLeaseWork(lease, () => true);
+
+  registry.releaseLease({ leaseId: lease.leaseId });
+  pass.release();
+
+  assert.deepEqual(inFlightClaimKeys(registry), []);
+  assert.equal(registry.listActiveLeases().length, 0, 'a released lease must not come back');
+});
+
+function inFlightClaimKeys(registry: LeaseRegistry): string[] {
+  const work = (
+    registry as unknown as {
+      inFlightWork: { entriesByLeaseId: Map<string, unknown> };
+    }
+  ).inFlightWork;
+  return [...work.entriesByLeaseId.keys()];
+}

src/daemon/__tests__/request-execution-scope.test.ts

@@ -509,8 +509,54 @@ test('expired leases remove owned sessions before the next command and free capa
   expect(nextLease.tenantId).toBe('tenant-b');
 });
 
+// A lease renewed only at admission lets one command slower than its inactivity TTL
+// expire the lease paying for the device it is using, and expiry then tears the
+// provider session down under the client still waiting for that same command. Found
+// while investigating #2509, whose cloud session ran on a ten-minute lease and so
+// lost its session some other way.
+test('an admitted request that outlives the lease TTL keeps its lease and session', async () => {
+  let now = 1_000;
+  const sessionStore = makeSessionStore('agent-device-request-scope-');
+  const leaseRegistry = new LeaseRegistry({
+    defaultLeaseTtlMs: 10,
+    minLeaseTtlMs: 1,
+    now: () => now,
+  });
+  const lease = leaseRegistry.allocateLease({ tenantId: 'tenant-a', runId: 'run-1' });
+  sessionStore.set(
+    'default',
+    makeIosSession('default', {
+      lease: {
+        leaseId: lease.leaseId,
+        tenantId: lease.tenantId,
+        runId: lease.runId,
+        leaseBackend: lease.backend,
+        expiresAt: lease.expiresAt,
+      },
+    }),
+  );
+
+  const slow = await createRequestExecutionScope({
+    req: makeRequest({ command: 'snapshot' }),
+    sessionStore,
+    leaseRegistry,
+  });
+  // The capture is still being waited on when it crosses the TTL, as a cloud
+  // page-source read does on a screen that never goes idle.
+  expect(await slow.runLocked(async () => (now = 1_011))).toBe(1_011);
+
+  const next = await createRequestExecutionScope({
+    req: makeRequest({ command: 'screenshot' }),
+    sessionStore,
+    leaseRegistry,
+  });
+  expect(await next.runLocked(async () => 'ran')).toBe('ran');
+  expect(sessionStore.get('default')).toBeDefined();
+});
+
 test('expired leased session cleanup waits for the request execution lock', async () => {
   let now = 1_000;
+  const requestId = 'request-scope-holds-execution-lock';
   const sessionStore = makeSessionStore('agent-device-request-scope-');
   const leaseRegistry = new LeaseRegistry({
     defaultLeaseTtlMs: 10,
@@ -531,7 +577,7 @@ test('expired leased session cleanup waits for the request execution lock', asyn
     }),
   );
   const first = await createRequestExecutionScope({
-    req: makeRequest({ command: 'click' }),
+    req: makeRequest({ command: 'click', meta: { requestId } }),
     sessionStore,
     leaseRegistry,
   });
@@ -554,16 +600,24 @@ test('expired leased session cleanup waits for the request execution lock', asyn
       }),
   );
   await firstEnteredPromise;
+  // The client walked away from this request. Abandoned work no longer defers the
+  // expiry it is sitting on, which is what makes this case about the lock and not
+  // about in-flight lease liveness.
+  markRequestCanceled(requestId);
 
   now = 1_011;
   const secondRun = second.runLocked(async () => 'second');
   await new Promise((resolve) => setTimeout(resolve, 20));
   expect(sessionStore.get('default')).toBeDefined();
 
-  releaseFirst();
-  await firstRun;
-  await expect(secondRun).resolves.toBe('second');
-  expect(sessionStore.get('default')).toBeUndefined();
+  try {
+    releaseFirst();
+    await firstRun;
+    await expect(secondRun).resolves.toBe('second');
+    expect(sessionStore.get('default')).toBeUndefined();
+  } finally {
+    clearRequestCanceled(requestId);
+  }
 });
 
 test('tenant lease rejection flushes diagnostics into the effective session request log', async () => {

src/daemon/__tests__/request-lease-work.test.ts

@@ -0,0 +1,84 @@
+import { test, expect } from 'vitest';
+import {
+  clearRequestAbortRegistration,
+  markRequestCanceled,
+  registerRequestAbort,
+} from '@agent-device/host-kit/request';
+import type { DaemonRequest } from '../daemon-request.ts';
+import { LeaseRegistry } from '../lease-registry.ts';
+import { runAdmittedLeaseWork } from '../request-lease-work.ts';
+import { HUMAN_CONTROL_LEASE_REQUEST } from './human-control-fixtures.ts';
+
+function admittedRequest(
+  leaseRegistry: LeaseRegistry,
+  overrides: Partial<DaemonRequest> = {},
+): DaemonRequest {
+  return {
+    token: 'test-token',
+    session: 'default',
+    command: 'snapshot',
+    positionals: [],
+    internal: { admittedLease: leaseRegistry.allocateLease(HUMAN_CONTROL_LEASE_REQUEST) },
+    ...overrides,
+  };
+}
+
+// A capture that runs past the lease TTL used to expire the lease paying for the
+// device, so the session died underneath the client still waiting for it.
+test('admitted work that outlives the lease TTL keeps the lease it is working on', async () => {
+  let now = 0;
+  const leaseRegistry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
+  const req = admittedRequest(leaseRegistry);
+
+  const worked = await runAdmittedLeaseWork({
+    leaseRegistry,
+    req,
+    task: async () => {
+      now = 9_000;
+      expect(leaseRegistry.listActiveLeases()).toHaveLength(1);
+      return 'captured';
+    },
+  });
+
+  expect(worked).toBe('captured');
+  expect(leaseRegistry.listActiveLeases()[0]?.expiresAt).toBe(14_000);
+});
+
+// The pass protects work somebody is still waiting for, and nothing else. Once the
+// client hangs up, work that lands later re-earns no lease: a handler that ignores
+// its cancellation cannot pin a rented device open.
+test('work nobody waited for renews nothing once its lease fell due', async () => {
+  let now = 0;
+  const leaseRegistry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
+  const requestId = 'request-2509-abandoned';
+  const registration = registerRequestAbort(requestId);
+  try {
+    const req = admittedRequest(leaseRegistry, { meta: { requestId } });
+    const worked = await runAdmittedLeaseWork({
+      leaseRegistry,
+      req,
+      task: async () => {
+        now = 9_000;
+        markRequestCanceled(requestId);
+        return 'late';
+      },
+    });
+
+    expect(worked).toBe('late');
+    expect(leaseRegistry.listActiveLeases()).toEqual([]);
+  } finally {
+    clearRequestAbortRegistration(registration);
+  }
+});
+
+test('a request admitted without a lease runs its work untouched', async () => {
+  const leaseRegistry = new LeaseRegistry();
+  const result = await runAdmittedLeaseWork({
+    leaseRegistry,
+    req: { token: 'test-token', session: 'default', command: 'status', positionals: [] },
+    task: async () => 'ran',
+  });
+
+  expect(result).toBe('ran');
+  expect(leaseRegistry.listActiveLeases()).toEqual([]);
+});