From 004767ed7e9bf0cf0a050e7a647a805056d17859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 25 Sep 2026 22:55:07 +0200 Subject: [PATCH 01/21] fix(lease): renew a heartbeat with no ttl for the window the lease already carries `heartbeatLease` resolved an absent `ttlMs` through the registry's default resolver, so a caller that heartbeats without repeating its allocation TTL silently shortened the lease to the daemon default. Every renewal that is not asked to change the window now renews for the window the lease is living on, which is the rule `refreshProtectedLease` already applied to protected work; that arithmetic moves to `leaseOwnTtlMs` beside the other lease-scope rules so both callers read one definition. The caller that heartbeats without a TTL is not asking for the default, it is asking for the same lease to keep going. An admitted request does exactly that, and this is half of why a long upload expired the lease paying for its own device (#2946). --- src/daemon/__tests__/lease-registry.test.ts | 53 +++++++++++++++++++++ src/daemon/lease-registry-scope.ts | 12 +++++ src/daemon/lease-registry.ts | 17 ++++--- 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/daemon/__tests__/lease-registry.test.ts b/src/daemon/__tests__/lease-registry.test.ts index fc3d576d5e..33852bfe44 100644 --- a/src/daemon/__tests__/lease-registry.test.ts +++ b/src/daemon/__tests__/lease-registry.test.ts @@ -646,6 +646,59 @@ test('releasing a lease drops the work claims recorded against it', () => { assert.equal(registry.listActiveLeases().length, 0, 'a released lease must not come back'); }); +// #2946: a caller that heartbeats without repeating its allocation TTL is asking for the same lease +// to keep going, not for the registry default. Resolving an absent `ttlMs` to that default silently +// shortened every lease allocated above it, which is how an upload expired the lease paying for the +// device it was uploading to. +test('a heartbeat with no ttlMs renews the lease for the window it already carries', () => { + let now = 1_000; + const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 10_000 }); + const lease = registry.allocateLease({ tenantId: 'tenant-a', runId: 'run-1', ttlMs: 60_000 }); + + now = 2_000; + const renewed = registry.heartbeatLease({ leaseId: lease.leaseId }); + assert.equal(renewed.heartbeatAt, 2_000); + assert.equal(renewed.expiresAt, 62_000, 'the allocated 60s window, not the 10s default'); +}); + +test('a heartbeat with no ttlMs keeps a long-TTL lease outliving the default TTL', () => { + let now = 0; + const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 60_000 }); + const lease = registry.allocateLease({ + tenantId: 'tenant-a', + runId: 'run-1', + leaseProvider: 'proxy', + deviceKey: 'android:mobile:emulator-5554', + ttlMs: 5 * 60_000, + }); + + // The 1m47s upload from the issue, beaten every 20s by the client and admitted after it lands. + for (const elapsed of [20_000, 40_000, 60_000, 80_000, 100_000, 107_000]) { + now = elapsed; + registry.heartbeatLease({ + leaseId: lease.leaseId, + tenantId: 'tenant-a', + runId: 'run-1', + leaseProvider: 'proxy', + deviceKey: 'android:mobile:emulator-5554', + }); + } + + const active = registry.listActiveLeases().find((entry) => entry.leaseId === lease.leaseId); + assert.ok(active, 'the lease is still active at the end of the upload'); + assert.equal(active.expiresAt - active.heartbeatAt, 5 * 60_000); +}); + +test('a heartbeat with an explicit ttlMs still sets that window', () => { + let now = 1_000; + const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 10_000 }); + const lease = registry.allocateLease({ tenantId: 'tenant-a', runId: 'run-1', ttlMs: 60_000 }); + + now = 2_000; + const renewed = registry.heartbeatLease({ leaseId: lease.leaseId, ttlMs: 5_000 }); + assert.equal(renewed.expiresAt, 7_000, 'an explicit window is the caller asking to change it'); +}); + function inFlightClaimKeys(registry: LeaseRegistry): string[] { const work = ( registry as unknown as { diff --git a/src/daemon/lease-registry-scope.ts b/src/daemon/lease-registry-scope.ts index 290bf86729..a228a9ec7f 100644 --- a/src/daemon/lease-registry-scope.ts +++ b/src/daemon/lease-registry-scope.ts @@ -328,6 +328,18 @@ export function createDeviceLease( }; } +/** + * The inactivity window a lease is currently living on: whatever TTL it was last renewed for. + * + * Every renewal that is not asked to change the window uses this, so a renewal never quietly + * re-decides it. Reading the registry default instead would shorten a lease allocated with a longer + * one — and a caller that heartbeats without repeating the allocation's TTL is not asking for the + * default, it is asking for the same lease to keep going. + */ +export function leaseOwnTtlMs(lease: Pick): number { + return lease.expiresAt - lease.heartbeatAt; +} + export function deviceLeaseBusyError(activeLease: DeviceLease): AppError { return new AppError('DEVICE_IN_USE', 'Device is already leased', { reason: 'DEVICE_LEASE_BUSY', diff --git a/src/daemon/lease-registry.ts b/src/daemon/lease-registry.ts index af8683d417..e0204ff6e1 100644 --- a/src/daemon/lease-registry.ts +++ b/src/daemon/lease-registry.ts @@ -23,6 +23,7 @@ import { assertLeaseOwnerScope, assertLeaseScopeMatch, leaseDeviceBindingKey, + leaseOwnTtlMs, leaseRunBindingKey, } from './lease-registry-scope.ts'; import { DeviceMutationDrain } from './device/device-mutation-drain.ts'; @@ -107,13 +108,21 @@ export class LeaseRegistry { return this.refreshLease(existingLease, leaseTtlMs); } + /** + * Extends a lease's life. A request naming a `ttlMs` asks for that inactivity window; one naming + * none renews for the window the lease already carries, the way protected work renews for its + * existing one (ADR 0007). Resolving an absent `ttlMs` to the registry default instead would + * shorten a lease every time a caller heartbeats without repeating its allocation TTL — which is + * what an admitted request does, and what expired the lease paying for a device mid-upload (#2946). + */ heartbeatLease(request: HeartbeatLeaseRequest): DeviceLease { const leaseId = normalizeRequiredLeaseId(request.leaseId); this.cleanupExpiredLeases(); const lease = this.getActiveLease(leaseId); assertLeaseOwnerScope(lease, request); assertLeaseScopeMatch(lease, request); - const leaseTtlMs = this.resolveLeaseTtlMs(request.ttlMs); + const leaseTtlMs = + request.ttlMs === undefined ? leaseOwnTtlMs(lease) : this.resolveLeaseTtlMs(request.ttlMs); return this.refreshLease(lease, leaseTtlMs); } @@ -427,11 +436,7 @@ export class LeaseRegistry { private refreshProtectedLease(leaseId: string | undefined, at: number): void { const lease = leaseId ? this.leases.get(leaseId) : undefined; if (lease) { - this.refreshLease( - lease, - lease.expiresAt - lease.heartbeatAt, - Math.max(at, lease.heartbeatAt), - ); + this.refreshLease(lease, leaseOwnTtlMs(lease), Math.max(at, lease.heartbeatAt)); } } From 84abb6639ce1deef514a1e13ffe0fa16a79baad2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 25 Sep 2026 22:55:15 +0200 Subject: [PATCH 02/21] fix(remote): renew the lease while an artifact upload runs on the caller's side A remote install uploaded the artifact from the caller before the install request was admitted, so nothing renewed the lease while the bytes moved: the daemon protects a lease while admitted work runs on it (#2509, ADR 0007), and an upload is the mirror image of that. A 449 MB APK that took 1m47s to upload against the one-minute default TTL expired the lease paying for the device it was uploading to, and the install then failed `Lease is not active` (#2946). `sendToDaemon` now brackets the upload phase with beats over the same transport the command uses. A beat names the command's lease scope, its own request id, and nothing else: the scope is what the daemon needs, and reusing the install request would send the upload's own payload once per beat. Each beat gets a fresh id because a beat that times out is canceled under its own. Only a remote daemon uploads and only a remote daemon holds a billed device, so a local command and a command that names no lease get no timer at all, and the interval starts rather than fires immediately, so an install that beats never sends no extra request. A beat that finds the lease gone ends the upload with that lease error rather than finishing bytes to a device nobody owns; a beat that fails for any other reason is reported through diagnostics and survived, since a later beat covers one lost request. ADR 0007 gains the rule this closes: what protects work that happens before admission. --- docs/adr/0007-remote-device-leases.md | 18 + .../daemon-client-upload-lease.test.ts | 515 ++++++++++++++++++ src/daemon-client/daemon-client.ts | 212 ++++++- 3 files changed, 744 insertions(+), 1 deletion(-) create mode 100644 src/daemon-client/__tests__/daemon-client-upload-lease.test.ts diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index af0e02d6ac..2a849c6211 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -67,6 +67,24 @@ one minute, while a cloud WebDriver connection profile asks for ten. A single co longer than its own lease is therefore ordinary on the default and only reachable through a profile on the longer one. +## Client-side work that precedes admission + +Protecting admitted work covers nothing that happens before a request is admitted. Installing an +artifact uploads it from the caller while the request that will consume it has not been admitted yet, +so an upload slower than the lease's inactivity window expired the lease paying for the device the +bytes were going to (#2946). The caller therefore beats the lease over the ordinary transport for as +long as that phase runs, naming the lease scope exactly as the command named it and no payload of its +own. An install names no window, so its beats renew for the window the lease already carries; a +caller that did name one keeps renewing on it. Resolving an absent window to the registry default +instead — which is what a heartbeat used to do — quietly shortened every lease allocated above that +default, which is the other half of why the upload could not survive. + +A beat is a fresh request each time, never the protected request rewritten: a request identity is +what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat +that finds the lease gone ends the phase with that lease error rather than finishing the upload +against a device nobody owns. A beat that fails for any other reason is reported and survived, because +a later beat covers one lost request. + ## Human control Human-control holds coexist with an open remote session. They belong to `LeaseRegistry` and use diff --git a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts new file mode 100644 index 0000000000..422af7920f --- /dev/null +++ b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts @@ -0,0 +1,515 @@ +import { afterEach, describe, test, vi } from 'vitest'; +import assert from 'node:assert/strict'; +import net from 'node:net'; +import { AppError } from '@agent-device/kernel/errors'; +import { closeLoopbackServer, listenOnLoopback } from '../../__tests__/test-utils/loopback.ts'; +import { resolveDaemonPaths } from '../../daemon-resolution.ts'; +import { + buildLeaseHeartbeatRequest, + buildUploadLeaseHeartbeat, + createLeaseRenewalBeat, + LEASE_HEARTBEAT_INTERVAL_MS, + leaseScopeForHeartbeat, + runProtectedLeaseWork, +} from '../daemon-client.ts'; +import type { DaemonRequest } from '../../daemon/daemon-request.ts'; + +function lostLeaseError(reason: string): AppError { + return new AppError('UNAUTHORIZED', 'Lease is not active', { reason }); +} + +function deferred(): { + promise: Promise; + resolve: (value: T) => void; + reject: (error: unknown) => void; +} { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('runProtectedLeaseWork', () => { + test('runs the task untouched when there is no lease to protect', async () => { + const heartbeat = vi.fn(); + assert.equal( + await runProtectedLeaseWork({ heartbeat: undefined, task: async () => 'ok' }), + 'ok', + ); + assert.equal(heartbeat.mock.calls.length, 0); + }); + + test('beats on the interval while a slow task runs, and stops when it ends', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => ({})); + const upload = deferred(); + + const running = runProtectedLeaseWork({ + heartbeat, + task: () => upload.promise, + }); + + await vi.advanceTimersByTimeAsync(LEASE_HEARTBEAT_INTERVAL_MS * 3); + // #2946: a 1m47s upload beat zero times and the lease died. Three intervals, three beats. + assert.equal(heartbeat.mock.calls.length, 3); + + upload.resolve('installed'); + assert.equal(await running, 'installed'); + + const before = heartbeat.mock.calls.length; + await vi.advanceTimersByTimeAsync(LEASE_HEARTBEAT_INTERVAL_MS * 3); + assert.equal(heartbeat.mock.calls.length, before, 'no beat outlives the phase'); + }); + + test('never overlaps beats', async () => { + vi.useFakeTimers(); + let inFlight = 0; + let maxConcurrent = 0; + const gates: ReturnType>[] = []; + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat: async () => { + const gate = deferred(); + gates.push(gate); + inFlight += 1; + maxConcurrent = Math.max(maxConcurrent, inFlight); + await gate.promise; + inFlight -= 1; + }, + }); + + await vi.advanceTimersByTimeAsync(10); + assert.equal(gates.length, 1); + + // Three more intervals pass with the first beat still outstanding: none of them may start a + // second one, or a slow beat would pile up behind a stalled transport. + await vi.advanceTimersByTimeAsync(30); + assert.equal(gates.length, 1, 'an outstanding beat holds the next one off'); + + gates[0]!.resolve(); + await vi.advanceTimersByTimeAsync(10); + assert.equal(gates.length, 2, 'the next beat starts once the previous one lands'); + + gates[1]!.resolve(); + upload.resolve(); + await vi.advanceTimersByTimeAsync(0); + await running; + assert.equal(maxConcurrent, 1); + }); + + test('a beat that fails for a reason other than the lease still running is survived', async () => { + vi.useFakeTimers(); + const failing: () => Promise = async () => { + // A reason from the same registry that is not a lost lease: contention says nothing about + // whether this lease is still ours, so the upload keeps going and the next beat asks again. + throw new AppError('DEVICE_IN_USE', 'Device is already leased', { + reason: 'DEVICE_LEASE_BUSY', + }); + }; + const heartbeat = vi.fn(failing); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat, + }); + + await vi.advanceTimersByTimeAsync(35); + assert.equal(heartbeat.mock.calls.length, 3, 'one lost beat does not stop the others'); + + heartbeat.mockImplementation(async () => ({})); + await vi.advanceTimersByTimeAsync(10); + upload.resolve('installed'); + assert.equal(await running, 'installed'); + }); + + for (const reason of [ + 'LEASE_NOT_FOUND', + 'LEASE_EXPIRED', + 'LEASE_REVOKED', + 'LEASE_SESSION_MISMATCH', + ]) { + test(`a beat that finds the lease ${reason} ends the phase with that error`, async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => { + throw lostLeaseError(reason); + }); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat, + }); + + // #2946 asked for an upload longer than the TTL to still succeed; when the lease really is + // gone the honest answer is the lease error, delivered before the bytes finish. The + // expectation rides the promise before the clock moves, so the rejection is never unobserved. + const rejected = assert.rejects( + running, + (error: unknown) => + error instanceof AppError && + error.code === 'UNAUTHORIZED' && + error.details?.reason === reason, + ); + await vi.advanceTimersByTimeAsync(15); + await rejected; + upload.resolve('too late'); + }); + } + + test('an upload that lands before the first beat reports success', async () => { + vi.useFakeTimers(); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat: async () => { + throw lostLeaseError('LEASE_NOT_FOUND'); + }, + }); + + upload.resolve('installed'); + await vi.advanceTimersByTimeAsync(0); + assert.equal(await running, 'installed'); + }); + + test('a phase that throws synchronously still stops the beats', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => ({})); + await assert.rejects( + (async () => + await runProtectedLeaseWork({ + intervalMs: 10, + task: () => { + throw new AppError('INVALID_ARGS', 'artifact vanished'); + }, + heartbeat, + }))(), + /artifact vanished/, + ); + + await vi.advanceTimersByTimeAsync(100); + assert.equal(heartbeat.mock.calls.length, 0, 'no timer outlived a phase that never started'); + }); + + test('a lost lease surfaces as a rejection even when the task finishes first', async () => { + vi.useFakeTimers(); + + let beat: (() => void) | undefined; + const first = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => first.promise, + heartbeat: () => + new Promise((_, reject) => { + beat = () => reject(lostLeaseError('LEASE_NOT_FOUND')); + }), + }); + + const rejected = assert.rejects( + running, + (error: unknown) => error instanceof AppError && error.details?.reason === 'LEASE_NOT_FOUND', + ); + await vi.advanceTimersByTimeAsync(10); + assert.ok(beat, 'a beat started'); + first.resolve('installed'); + beat?.(); + await vi.advanceTimersByTimeAsync(0); + await rejected; + }); + + test('a task rejection propagates and still stops the beats', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => ({})); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: async () => { + throw new AppError('COMMAND_FAILED', 'upload failed'); + }, + heartbeat, + }); + + await assert.rejects((async () => await running)(), /upload failed/); + const before = heartbeat.mock.calls.length; + await vi.advanceTimersByTimeAsync(100); + assert.equal(heartbeat.mock.calls.length, before); + }); + + test('an in-flight beat is awaited on the way out', async () => { + vi.useFakeTimers(); + const beat = deferred(); + const upload = deferred(); + let started = false; + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat: async () => { + started = true; + await beat.promise; + }, + }); + + await vi.advanceTimersByTimeAsync(10); + assert.equal(started, true, 'a beat is in flight while the upload finishes'); + + upload.resolve('installed'); + await vi.advanceTimersByTimeAsync(0); + let settled = false; + void running.then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(0); + assert.equal(settled, false, 'the phase waits for the renewal it started'); + + beat.resolve(); + assert.equal(await running, 'installed'); + }); +}); + +describe('leaseScopeForHeartbeat', () => { + test('names no lease for a request that carries none', () => { + assert.equal(leaseScopeForHeartbeat({ flags: {}, meta: undefined }), undefined); + }); + + test('reads the lease scope from the request meta, then from the flags', () => { + assert.equal( + leaseScopeForHeartbeat({ meta: { leaseId: 'lease-meta', tenantId: 'acme' } })?.leaseId, + 'lease-meta', + ); + assert.equal( + leaseScopeForHeartbeat({ flags: { leaseId: 'lease-flag' } })?.leaseId, + 'lease-flag', + ); + }); +}); + +describe('buildLeaseHeartbeatRequest', () => { + test('carries the lease scope and nothing that belongs to the request it is protecting', () => { + const beat = buildLeaseHeartbeatRequest( + { + leaseId: 'lease-1', + tenantId: 'acme', + runId: 'run-1', + leaseBackend: 'android-instance', + leaseProvider: 'proxy', + deviceKey: 'android:mobile:emulator-5554', + clientId: 'client-1', + }, + { + session: 'adc-android', + sessionIsolation: 'tenant', + requestId: 'beat-1', + token: 'daemon-token', + }, + ); + + assert.equal(beat.command, 'lease_heartbeat'); + assert.deepEqual(beat.positionals, []); + assert.equal(beat.session, 'adc-android'); + assert.equal(beat.token, 'daemon-token'); + assert.deepEqual(beat.meta, { + leaseId: 'lease-1', + tenantId: 'acme', + runId: 'run-1', + leaseBackend: 'android-instance', + leaseProvider: 'proxy', + deviceKey: 'android:mobile:emulator-5554', + clientId: 'client-1', + sessionIsolation: 'tenant', + requestId: 'beat-1', + }); + // The socket transport serializes the whole request, so a beat that rode along with the install + // would re-send a 449 MB artifact every interval. + assert.equal(beat.flags, undefined); + assert.equal('installSource' in (beat.meta ?? {}), false); + assert.equal(beat.internal, undefined); + }); + + test('sends no ttl for an install, whose scope never carried one, so the lease keeps its own window', () => { + // connection-runtime passes the TTL to `leases.allocate` only, so an install request's scope has + // none. A beat that invented one would shorten a lease allocated longer. + const installRequest: Pick = { + flags: { leaseId: 'lease-1', platform: 'android' }, + meta: { leaseId: 'lease-1', tenantId: 'acme' }, + }; + const scope = leaseScopeForHeartbeat(installRequest)!; + const beat = buildLeaseHeartbeatRequest(scope, { + session: 'default', + requestId: 'beat-1', + token: 't', + }); + assert.equal(beat.meta?.leaseTtlMs, undefined); + }); + + test('a caller that did name a ttl keeps renewing on it', () => { + const scope = leaseScopeForHeartbeat({ + flags: { leaseId: 'lease-1' }, + meta: { leaseId: 'lease-1', leaseTtlMs: 600_000 }, + })!; + const beat = buildLeaseHeartbeatRequest(scope, { + session: 'default', + requestId: 'beat-1', + token: 't', + }); + assert.equal(beat.meta?.leaseTtlMs, 600_000); + }); +}); + +describe('createLeaseRenewalBeat', () => { + const scope = { + leaseId: 'lease-1', + tenantId: 'acme', + runId: 'run-1', + leaseBackend: 'android-instance', + } as const; + + function beatContext(send: (request: DaemonRequest) => Promise) { + return { + session: 'adc-android', + sessionIsolation: 'tenant' as const, + token: 'daemon-token', + send, + }; + } + + test('sends one lease_heartbeat naming the lease it is protecting', async () => { + const sent: DaemonRequest[] = []; + await createLeaseRenewalBeat( + scope, + beatContext(async (request) => void sent.push(request)), + )(); + + assert.equal(sent.length, 1); + assert.equal(sent[0]!.command, 'lease_heartbeat'); + assert.equal(sent[0]!.meta?.leaseId, 'lease-1'); + assert.equal(sent[0]!.meta?.tenantId, 'acme'); + assert.equal(sent[0]!.meta?.runId, 'run-1'); + assert.equal(sent[0]!.meta?.sessionIsolation, 'tenant'); + assert.equal(sent[0]!.session, 'adc-android'); + assert.equal(sent[0]!.token, 'daemon-token'); + }); + + test('every beat is a distinct request, so a timed-out beat cannot cancel the next one', async () => { + const sent: DaemonRequest[] = []; + const beat = createLeaseRenewalBeat( + scope, + beatContext(async (request) => void sent.push(request)), + ); + await beat(); + await beat(); + await beat(); + + const ids = sent.map((request) => request.meta?.requestId); + assert.equal(new Set(ids).size, 3, 'three beats, three request ids'); + assert.ok(ids.every((id) => typeof id === 'string' && id.length > 0)); + }); + + test('a transport failure propagates to the caller that survives it', async () => { + const beat = createLeaseRenewalBeat( + scope, + beatContext(async () => { + throw new AppError('COMMAND_FAILED', 'connection reset'); + }), + ); + await assert.rejects((async () => await beat())(), /connection reset/); + }); +}); + +describe('buildUploadLeaseHeartbeat', () => { + const installRequest = { + command: 'install', + positionals: ['/tmp/app.apk'], + session: 'adc-android', + flags: { + leaseId: 'lease-1', + tenantId: 'acme', + runId: 'run-1', + deviceKey: 'android:mobile:emulator-5554', + platform: 'android' as const, + }, + meta: { leaseId: 'lease-1', tenantId: 'acme', runId: 'run-1' }, + }; + + const settings = { + paths: resolveDaemonPaths('/tmp/agent-device-upload-lease'), + transportPreference: 'socket' as const, + serverMode: 'socket' as const, + }; + + test('no beat for a local daemon, which never uploads and holds no billed device', () => { + assert.equal( + buildUploadLeaseHeartbeat( + { port: 1, token: 't', pid: process.pid }, + settings, + installRequest, + ), + undefined, + ); + }); + + test('the beat reaches a remote daemon over its HTTP endpoint', async () => { + const requests: { method?: string; path?: string; body: string }[] = []; + const server = net.createServer((socket) => { + let body = ''; + socket.on('data', (chunk) => { + body += chunk.toString('utf8'); + const headerEnd = body.indexOf('\r\n\r\n'); + if (headerEnd < 0) return; + const head = body.slice(0, headerEnd); + const [requestLine] = head.split('\r\n'); + const [method, path] = requestLine?.split(' ') ?? []; + requests.push({ method, path, body: body.slice(headerEnd + 4) }); + const payload = JSON.stringify({ + jsonrpc: '2.0', + id: 'x', + result: { ok: true }, + }); + socket.write( + `HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: ${String( + payload.length, + )}\r\n\r\n${payload}`, + ); + socket.end(); + }); + }); + const port = await listenOnLoopback(server); + + try { + const beat = buildUploadLeaseHeartbeat( + { baseUrl: `http://127.0.0.1:${String(port)}/agent-device`, token: 'remote-token', pid: 1 }, + { ...settings, transportPreference: 'auto' }, + installRequest, + ); + assert.ok(beat); + await beat!(); + } finally { + await closeLoopbackServer(server); + } + + const posted = requests.find((request) => request.method === 'POST'); + assert.ok(posted, 'the beat POSTs to the remote daemon'); + const payload = JSON.parse(posted!.body) as { + method: string; + params: Record; + }; + assert.equal(posted!.path, '/agent-device/rpc'); + assert.equal(payload.method, 'agent_device.lease.heartbeat'); + assert.equal(payload.params.leaseId, 'lease-1'); + assert.equal(payload.params.tenantId, 'acme'); + assert.equal(payload.params.runId, 'run-1'); + assert.equal(payload.params.deviceKey, 'android:mobile:emulator-5554'); + + // A beat asks for the same lease to keep going: it names no window, so the daemon renews the one + // the lease already carries. + assert.equal('ttlMs' in payload.params, false); + }); +}); diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index 95a4ad4728..ccacd5b96b 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -14,6 +14,12 @@ import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '@agent-device/command-regist import { resolveCommandTimeoutPolicy } from '@agent-device/command-registry/registry'; import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/timeout-policy'; import { prepareRemoteRequestArtifacts } from '../remote/daemon-artifacts.ts'; +import { isRemoteDaemon } from './daemon-client-metadata.ts'; +import { + leaseScopeFromRequest, + leaseScopeToRequestMeta, + type LeaseScope, +} from '@agent-device/contracts/lease-scope'; import { attachActiveSessionAddressHint, attachRepairSessionAddressHint, @@ -61,7 +67,10 @@ export async function sendToDaemon( { requestId, session: req.session }, ); const info = daemon.info; - const preparedRemoteRequest = await prepareRemoteRequestArtifacts(requestWithoutAuthFlag, info); + const preparedRemoteRequest = await runProtectedLeaseWork({ + heartbeat: buildUploadLeaseHeartbeat(info, settings, requestWithoutAuthFlag), + task: () => prepareRemoteRequestArtifacts(requestWithoutAuthFlag, info), + }); writeInstallInProgressNotice(requestWithoutAuthFlag.command); const request = buildTransportRequest( @@ -267,6 +276,207 @@ function withActiveSessionAddressHint( ); } +/** + * How often a long client-side phase renews the lease it is waiting under. + * + * A third of the daemon's one-minute default inactivity TTL: a beat always lands while two thirds of + * the window it is protecting is still open, so one slow or lost beat cannot cost the lease. + */ +export const LEASE_HEARTBEAT_INTERVAL_MS = 20_000; + +/** Why a beat stopped protecting the lease: the lease is gone, so nothing else is worth waiting for. */ +const FATAL_LEASE_BEAT_REASONS: ReadonlySet = new Set([ + 'LEASE_NOT_FOUND', + 'LEASE_EXPIRED', + 'LEASE_REVOKED', + 'LEASE_SESSION_MISMATCH', +]); + +/** + * Runs one client-side phase under a lease it does not own the clock of. + * + * A lease renews when a request is admitted, and the daemon protects a lease while ADMITTED work + * runs on it (#2509, ADR 0007). An artifact upload is the mirror image: it happens on the caller's + * side, before the install request that will consume it is admitted, so nothing renews the lease + * while it runs and a large enough artifact expired the lease that was paying for the device it was + * being uploaded to (#2946). + * + * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and + * the phase runs untouched. A beat that fails for a reason other than the lease being gone is + * reported and ignored — one lost request must not fail an upload that a later beat will cover. A + * beat that finds the lease gone ends the phase immediately with that error: the device is no + * longer ours, and the only honest outcome is to say so before the bytes finish. + * + * Beats never overlap, and an in-flight beat is awaited on the way out so a renewal cannot land + * after the phase it was protecting. + */ +export async function runProtectedLeaseWork( + options: Readonly<{ + heartbeat: (() => Promise) | undefined; + intervalMs?: number; + task: () => Promise; + }>, +): Promise { + const { heartbeat } = options; + if (!heartbeat) return await options.task(); + const intervalMs = options.intervalMs ?? LEASE_HEARTBEAT_INTERVAL_MS; + + let inFlight: Promise | undefined; + let lostLease: unknown; + let reportLoss: ((error: unknown) => void) | undefined; + const lost = new Promise((_, reject) => { + reportLoss = reject; + }); + + const beat = () => { + if (inFlight) return; + inFlight = (async () => { + try { + await heartbeat(); + } catch (error) { + if (!isLostLeaseError(error)) { + emitDiagnostic({ + level: 'warn', + phase: 'lease_heartbeat_failed', + data: { message: error instanceof Error ? error.message : String(error) }, + }); + return; + } + lostLease = error; + reportLoss?.(error); + } finally { + inFlight = undefined; + } + })(); + }; + + const timer = setInterval(beat, intervalMs); + // A beat already in flight when the phase settles can still report a lost lease; the caller reads + // it from `lostLease`, so nothing may be left racing on this rejection by then. + void lost.catch(() => undefined); + const phase = await captureOutcome( + // The async boundary also turns a synchronous throw from the phase into a rejection, so the + // timer below is always cleared. + (async () => await Promise.race([options.task(), lost]))(), + ); + clearInterval(timer); + await inFlight; + // A beat that found the lease gone outranks a phase that settled meanwhile, from either side: the + // device is no longer ours, and the lease error is the reason the phase was not worth finishing. + if (lostLease !== undefined) throw lostLease; + if (!phase.ok) throw phase.error; + return phase.value; +} + +type Outcome = { ok: true; value: T } | { ok: false; error: unknown }; + +async function captureOutcome(promise: Promise): Promise> { + try { + return { ok: true, value: await promise }; + } catch (error) { + return { ok: false, error }; + } +} + +function isLostLeaseError(error: unknown): boolean { + return error instanceof AppError && FATAL_LEASE_BEAT_REASONS.has(error.details?.reason); +} + +/** + * The beat that keeps a remote lease alive across a long client-side phase. + * + * `send` is the caller's transport. The beat goes over it directly rather than through the client's + * `leases.heartbeat`, because the client would come back through the upload path it protects. + */ +export function createLeaseRenewalBeat( + leaseScope: LeaseScope, + context: Readonly<{ + session: string; + sessionIsolation?: NonNullable['sessionIsolation']; + token: string; + send: (request: DaemonRequest) => Promise; + }>, +): () => Promise { + return async () => + await context.send( + buildLeaseHeartbeatRequest(leaseScope, { + session: context.session, + sessionIsolation: context.sessionIsolation, + requestId: createRequestId(), + token: context.token, + }), + ); +} + +/** + * The request one beat sends: the command's lease scope, its own id, and nothing else. + * + * It is a fresh request rather than the install rewritten, so nothing about the upload — its source, + * its positional, its own request id — can be mistaken for the lease's state. The scope rides along + * as the command named it, so a beat renews for whatever window that command asked for, and for the + * lease's own window when it named none, which is the ordinary case for an install. Each beat gets a + * fresh id because a beat that times out is canceled under its own, and a shared id would let a later + * beat inherit an earlier cancellation. + */ +export function buildLeaseHeartbeatRequest( + leaseScope: LeaseScope, + context: Readonly<{ + session: string; + sessionIsolation?: NonNullable['sessionIsolation']; + requestId: string; + token: string; + }>, +): DaemonRequest { + return { + command: INTERNAL_COMMANDS.leaseHeartbeat, + positionals: [], + session: context.session, + token: context.token, + meta: { + ...leaseScopeToRequestMeta(leaseScope), + sessionIsolation: context.sessionIsolation, + requestId: context.requestId, + }, + }; +} + +/** The lease a request is running under, when it names one. */ +export function leaseScopeForHeartbeat( + request: Pick, +): LeaseScope | undefined { + const scope = leaseScopeFromRequest(request); + return scope.leaseId ? scope : undefined; +} + +/** + * The beat that renews a remote lease across an artifact upload, or `undefined` when there is no + * lease to protect: only a remote daemon uploads, so only one can be waiting on a billed device, and + * a command that names no lease has nothing to renew. + * + * Exported for its own coverage: `sendToDaemon` calls it once per upload, and the beat cadence is + * far too slow for an end-to-end test to reach it at real transport speed. + */ +export function buildUploadLeaseHeartbeat( + info: EnsuredDaemon['info'], + settings: DaemonClientSettings, + request: Omit, +): (() => Promise) | undefined { + if (!isRemoteDaemon(info)) return undefined; + const leaseScope = leaseScopeForHeartbeat(request); + if (!leaseScope) return undefined; + const timeoutMs = resolveCommandRequestTimeoutMs( + resolveCommandTimeoutPolicy(INTERNAL_COMMANDS.leaseHeartbeat), + { positionals: [] }, + ); + return createLeaseRenewalBeat(leaseScope, { + session: request.session, + sessionIsolation: request.meta?.sessionIsolation, + token: info.token, + send: async (beat) => + await sendRequest(info, beat, settings.transportPreference, settings.paths, timeoutMs), + }); +} + function writeInstallInProgressNotice(command: string | undefined): void { if (!isInstallLikeCommand(command) || process.stderr.isTTY !== true || process.env.CI) return; process.stderr.write( From f52305c4eb56b2385008a5056def0afccc53e240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:21:06 +0200 Subject: [PATCH 03/21] fix(lease): renew an admitted request for the window its lease carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Admission named its own proxy default on every request, so a client that rented a device for longer than that default lost the window on the next command — the lease-side twin of the heartbeat bug in #2946, which the client fix could not reach because the shortening happened at admission. The window a lease carries is the one its client named when it allocated; only a request naming its own window changes it now, which retires DEFAULT_PROXY_LEASE_TTL_MS with no producer left. --- packages/contracts/src/lease-scope.ts | 1 - src/daemon/__tests__/lease-lifecycle.test.ts | 4 +- .../__tests__/request-admission.test.ts | 76 +++++++++++++++++++ .../__tests__/request-execution-scope.test.ts | 4 +- .../__tests__/request-router-open.test.ts | 2 +- src/daemon/lease-context.ts | 3 +- src/daemon/request-admission.ts | 13 +--- .../remote-proxy-parity.test.ts | 33 ++++---- 8 files changed, 106 insertions(+), 30 deletions(-) diff --git a/packages/contracts/src/lease-scope.ts b/packages/contracts/src/lease-scope.ts index 57a2b480f9..1f18c16d26 100644 --- a/packages/contracts/src/lease-scope.ts +++ b/packages/contracts/src/lease-scope.ts @@ -4,7 +4,6 @@ import type { CloudProviderProfileFields } from './remote-config-fields.ts'; import type { CommandFlags } from './command-flags.ts'; const PROXY_LEASE_PROVIDER = 'proxy'; -export const DEFAULT_PROXY_LEASE_TTL_MS = 300_000; const REQUIRED_PROXY_LEASE_FIELDS = [ 'leaseId', diff --git a/src/daemon/__tests__/lease-lifecycle.test.ts b/src/daemon/__tests__/lease-lifecycle.test.ts index e6c3f6751c..8fce339a62 100644 --- a/src/daemon/__tests__/lease-lifecycle.test.ts +++ b/src/daemon/__tests__/lease-lifecycle.test.ts @@ -48,7 +48,9 @@ test('admitRequestLeaseForLockedScope heartbeats and stores admitted lease on th expect(req.internal?.admittedLease?.leaseId).toBe(lease.leaseId); expect(req.internal?.admittedLease?.heartbeatAt).toBe(2_000); - expect(sessionStore.get('default')?.lease?.expiresAt).toBe(302_000); + // Renewal is for the window the lease carries — the registry default here, since this client + // named none — not the proxy-specific default admission used to name on every request (#2946). + expect(sessionStore.get('default')?.lease?.expiresAt).toBe(62_000); }); test('cleanupExpiredLeasedSession consumes expired lease and deletes the session after teardown', async () => { diff --git a/src/daemon/__tests__/request-admission.test.ts b/src/daemon/__tests__/request-admission.test.ts index e4bfca61d0..c8719b9d8c 100644 --- a/src/daemon/__tests__/request-admission.test.ts +++ b/src/daemon/__tests__/request-admission.test.ts @@ -164,6 +164,82 @@ test('sessionless apps admits a provider declared by the runtime app catalog', ( assert.equal(result, undefined); }); +// #2946 (the other half): a proxy client that allocated a lease with a window longer than the old +// synthetic admission default used to lose that window on the next admitted command — admission +// re-renewed every proxy lease at its own default instead of the window the lease carries. Admission +// now renews for the lease's window (ADR 0007); only a request that names a window changes it. +test('admitting a command does not shorten a proxy lease allocated above the old admission default', () => { + let now = 1_000; + const registry = new LeaseRegistry({ now: () => now }); + const lease = registry.allocateLease({ + tenantId: 'tenant-a', + runId: 'run-1', + leaseProvider: 'proxy', + deviceKey: 'ios:mobile:SIM-001', + clientId: 'client-a', + ttlMs: 600_000, + }); + now = 2_000; + + const result = assertRequestLeaseAdmission( + makeRequest({ + command: 'snapshot', + meta: { + tenantId: 'tenant-a', + runId: 'run-1', + sessionIsolation: 'tenant', + leaseId: lease.leaseId, + leaseProvider: 'proxy', + deviceKey: 'ios:mobile:SIM-001', + clientId: 'client-a', + }, + }), + registry, + undefined, + ); + + assert.equal(result?.leaseId, lease.leaseId); + assert.equal( + result!.expiresAt - result!.heartbeatAt, + 600_000, + 'the window the lease carries, not a smaller default', + ); +}); + +test('a command that names its own window renews the lease onto it', () => { + let now = 1_000; + const registry = new LeaseRegistry({ now: () => now }); + const lease = registry.allocateLease({ + tenantId: 'tenant-a', + runId: 'run-1', + leaseProvider: 'proxy', + deviceKey: 'ios:mobile:SIM-001', + clientId: 'client-a', + ttlMs: 600_000, + }); + now = 2_000; + + const result = assertRequestLeaseAdmission( + makeRequest({ + command: 'snapshot', + meta: { + tenantId: 'tenant-a', + runId: 'run-1', + sessionIsolation: 'tenant', + leaseId: lease.leaseId, + leaseProvider: 'proxy', + deviceKey: 'ios:mobile:SIM-001', + clientId: 'client-a', + leaseTtlMs: 90_000, + }, + }), + registry, + undefined, + ); + + assert.equal(result!.expiresAt - result!.heartbeatAt, 90_000); +}); + test('close still admits and heartbeats a real active lease', () => { let now = 1_000; const registry = new LeaseRegistry({ now: () => now }); diff --git a/src/daemon/__tests__/request-execution-scope.test.ts b/src/daemon/__tests__/request-execution-scope.test.ts index 4790083b8e..5da8da19d9 100644 --- a/src/daemon/__tests__/request-execution-scope.test.ts +++ b/src/daemon/__tests__/request-execution-scope.test.ts @@ -224,8 +224,8 @@ test('leased session admission uses stored lease metadata and heartbeats', async expect(scope.sessionName).toBe('default'); const activeLease = leaseRegistry.listActiveLeases()[0]; expect(activeLease?.heartbeatAt).toBe(2_000); - expect(activeLease?.expiresAt).toBe(302_000); - expect(sessionStore.get('default')?.lease?.expiresAt).toBe(302_000); + expect(activeLease?.expiresAt).toBe(62_000); + expect(sessionStore.get('default')?.lease?.expiresAt).toBe(62_000); }); test('leased session heartbeat is serialized with the request execution lock', async () => { diff --git a/src/daemon/__tests__/request-router-open.test.ts b/src/daemon/__tests__/request-router-open.test.ts index 344f7ba6b1..ec084f7c75 100644 --- a/src/daemon/__tests__/request-router-open.test.ts +++ b/src/daemon/__tests__/request-router-open.test.ts @@ -368,7 +368,7 @@ test('open stores admitted lease metadata on the session', async () => { leaseProvider: 'proxy', clientId: 'client-a', deviceKey: 'ios:SIM-LEASED', - expiresAt: 301_000, + expiresAt: 61_000, }); }); diff --git a/src/daemon/lease-context.ts b/src/daemon/lease-context.ts index 73d6c229f0..a07acb3d71 100644 --- a/src/daemon/lease-context.ts +++ b/src/daemon/lease-context.ts @@ -4,14 +4,13 @@ import type { DeviceLease } from '@agent-device/contracts/device'; import type { RunnerLogicalLeaseContext } from '@agent-device/contracts/runner-lease-context'; import { stripUndefined } from '@agent-device/kernel/record'; import { - DEFAULT_PROXY_LEASE_TTL_MS, findMissingProxyLeaseFields, isProxyLeaseScope, leaseScopeFromRequest, type LeaseScope, } from '@agent-device/contracts/lease-scope'; -export { DEFAULT_PROXY_LEASE_TTL_MS, findMissingProxyLeaseFields, isProxyLeaseScope }; +export { findMissingProxyLeaseFields, isProxyLeaseScope }; export type { LeaseScope }; export type SessionLease = { diff --git a/src/daemon/request-admission.ts b/src/daemon/request-admission.ts index 12e8117b91..1d0bfd4490 100644 --- a/src/daemon/request-admission.ts +++ b/src/daemon/request-admission.ts @@ -8,9 +8,7 @@ import { } from './daemon-command-registry.ts'; import type { DeviceLease, ProviderAppCatalog } from '@agent-device/contracts/device'; import { - DEFAULT_PROXY_LEASE_TTL_MS, findMissingProxyLeaseFields, - isProxyLeaseScope, resolveLeaseScope, resolveRequestOrSessionLeaseScope, } from './lease-context.ts'; @@ -94,14 +92,11 @@ export function assertRequestLeaseAdmission( } assertRequestSessionLeaseMatches(requestLeaseScope, sessionLease); const leaseScope = resolveRequestOrSessionLeaseScope(req, session); - const heartbeatLeaseScope = { - ...leaseScope, - leaseTtlMs: - leaseScope.leaseTtlMs ?? - (isProxyLeaseScope(leaseScope) ? DEFAULT_PROXY_LEASE_TTL_MS : undefined), - }; leaseRegistry.assertLeaseAdmission(leaseScopeToHeartbeatRequest(leaseScope)); - const lease = leaseRegistry.heartbeatLease(leaseScopeToHeartbeatRequest(heartbeatLeaseScope)); + // Admission renews for the window the lease already carries, or the window this request named. + // Naming a proxy-specific default here used to shorten every lease allocated above it — a client + // that rented a device for longer than the default lost it on the next admitted command (#2946). + const lease = leaseRegistry.heartbeatLease(leaseScopeToHeartbeatRequest(leaseScope)); if (isHumanControlMutation(req)) leaseRegistry.assertHumanControlAdmission(lease); return lease; } diff --git a/test/integration/provider-scenarios/remote-proxy-parity.test.ts b/test/integration/provider-scenarios/remote-proxy-parity.test.ts index 0cd8b69191..f0b99530c2 100644 --- a/test/integration/provider-scenarios/remote-proxy-parity.test.ts +++ b/test/integration/provider-scenarios/remote-proxy-parity.test.ts @@ -4,7 +4,6 @@ import http from 'node:http'; import os from 'node:os'; import path from 'node:path'; import { test } from 'vitest'; -import { DEFAULT_PROXY_LEASE_TTL_MS } from '@agent-device/contracts/lease-scope'; import { sendToDaemon } from '../../../src/daemon-client/daemon-client.ts'; import { LeaseRegistry } from '../../../src/daemon/lease-registry.ts'; import { createDaemonHttpServer } from '../../../src/daemon/server/http-server.ts'; @@ -549,7 +548,7 @@ test( async ({ world, proxied }) => { const session = 'leased'; const flags = { platform: 'ios', udid: SIM.id } as const; - const allocate = async (): Promise => { + const allocate = async (): Promise<{ leaseId: string; expiresAt: number }> => { const response = await proxied({ session, command: 'lease_allocate', @@ -558,9 +557,13 @@ test( meta: LEASE_SCOPE, }); assert.equal(response.ok, true, JSON.stringify(response)); - const leaseId = (response.ok ? response.data : {})?.lease as { leaseId?: string }; - assert.equal(typeof leaseId?.leaseId, 'string'); - return leaseId.leaseId!; + const lease = (response.ok ? response.data : {})?.lease as { + leaseId?: string; + expiresAt?: number; + }; + assert.equal(typeof lease?.leaseId, 'string'); + assert.equal(typeof lease?.expiresAt, 'number'); + return { leaseId: lease.leaseId!, expiresAt: lease.expiresAt! }; }; const run = async ( command: string, @@ -577,22 +580,24 @@ test( }); const firstLease = await allocate(); - assert.equal((await run('open', [APP], firstLease)).ok, true); + assert.equal((await run('open', [APP], firstLease.leaseId)).ok, true); assert.equal( - (await run('snapshot', [], firstLease, { snapshotInteractiveOnly: true })).ok, + (await run('snapshot', [], firstLease.leaseId, { snapshotInteractiveOnly: true })).ok, true, ); assert.equal( baselineInitialized( - await run('diff', ['snapshot'], firstLease, { snapshotInteractiveOnly: true }), + await run('diff', ['snapshot'], firstLease.leaseId, { snapshotInteractiveOnly: true }), ), false, 'the leased session holds comparison state before it expires', ); // The lease lapses without a heartbeat; the next request through the proxy finds it expired. - now += DEFAULT_PROXY_LEASE_TTL_MS + 1; - const expired = await run('diff', ['snapshot'], firstLease, { + // The jump is the window the lease itself was allocated with — a client that names no ttl + // gets the registry default, which is the only window the daemon promises it. + now = firstLease.expiresAt + 1; + const expired = await run('diff', ['snapshot'], firstLease.leaseId, { snapshotInteractiveOnly: true, }); assert.equal(expired.ok, false); @@ -603,16 +608,16 @@ test( assert.equal(world.daemon.session(session), undefined, 'expiry tears the session down'); const secondLease = await allocate(); - assert.notEqual(secondLease, firstLease); - assert.equal((await run('open', [APP], secondLease)).ok, true); + assert.notEqual(secondLease.leaseId, firstLease.leaseId); + assert.equal((await run('open', [APP], secondLease.leaseId)).ok, true); assert.equal( baselineInitialized( - await run('diff', ['snapshot'], secondLease, { snapshotInteractiveOnly: true }), + await run('diff', ['snapshot'], secondLease.leaseId, { snapshotInteractiveOnly: true }), ), true, 'a reacquired lease must not compare against the expired session tree', ); - assert.equal((await run('close', [], secondLease, {})).ok, true); + assert.equal((await run('close', [], secondLease.leaseId, {})).ok, true); }, { leaseRegistry }, ); From 1c149b6cb319e330b408d6006a7605b9586cde95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:21:55 +0200 Subject: [PATCH 04/21] fix(remote): beat to the lease's own window and stop the upload with it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upload beat waited a fixed twenty seconds for its first renewal, so a lease admitted with a shorter window lapsed while a perfectly valid upload ran — the #2946 symptom one seam earlier. A beat answers with the window it just renewed, so the phase now beats immediately and then a third of that window after each beat lands: a beat slower than the cadence delays its successor instead of replacing the schedule or silencing every beat behind it, and a floor keeps a pathological window from becoming a request loop. A beat refused for a missing or mismatched owner scope says this request can never renew the lease, so retrying it only spent the upload against a lease that had stopped renewing. Those reasons now end the phase beside the lost-lease ones, and the phase hands its task an abort signal the upload chain honours: preflight, finalize, the direct PUT, and the legacy stream all run under the caller's cancellation combined with their own timeout, and a canceled upload stops asking the daemon for a fresh ticket. node:http's own request signal is what stops bytes already piped at a device nobody holds. --- docs/adr/0007-remote-device-leases.md | 19 +- .../upload-client-cancellation.test.ts | 194 +++++++++++++++ .../daemon-client-upload-lease.test.ts | 226 ++++++++++++++---- src/daemon-client/daemon-client.ts | 147 ++++++++---- .../daemon-artifacts-save-script.test.ts | 13 +- .../daemon-artifacts-test-command.test.ts | 10 +- src/remote/daemon-artifacts.ts | 8 +- src/remote/upload-client.ts | 98 ++++++-- src/remote/upload-stream.ts | 6 + .../remote-daemon-client.test.ts | 5 + 10 files changed, 608 insertions(+), 118 deletions(-) create mode 100644 src/__tests__/upload-client-cancellation.test.ts diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index 2a849c6211..604d63c50b 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -77,13 +77,24 @@ long as that phase runs, naming the lease scope exactly as the command named it own. An install names no window, so its beats renew for the window the lease already carries; a caller that did name one keeps renewing on it. Resolving an absent window to the registry default instead — which is what a heartbeat used to do — quietly shortened every lease allocated above that -default, which is the other half of why the upload could not survive. +default, which is the other half of why the upload could not survive. Request admission had its own +copy of that mistake: it named a proxy-specific default for every admitted request, so a lease +allocated longer than the default lost its window on the next command. Admission renews on the lease's +window too; the window a lease carries is the one its client named when it allocated. + +The first beat is fired when the phase starts, not one cadence in, because a beat is what proves the +lease the upload is spending time on is still alive — a lease shorter than any fixed cadence would +otherwise lapse before anything renewed it, and a lease already gone is worth learning that before any +bytes move. Each beat answers with the window it just renewed, and the next one is armed a third of +that window after the beat lands: beats never overlap, and one slower than the cadence delays its +successor instead of replacing the schedule or suppressing every beat behind it. A beat is a fresh request each time, never the protected request rewritten: a request identity is what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat -that finds the lease gone ends the phase with that lease error rather than finishing the upload -against a device nobody owns. A beat that fails for any other reason is reported and survived, because -a later beat covers one lost request. +that finds the lease gone, or finds that this request can never renew it because its scope is missing +or belongs to another lease, ends the phase with that error and cancels the upload rather than +finishing bytes against a device nobody owns or a lease that will stop renewing. A beat that fails for +any other reason is reported and survived, because a later beat covers one lost request. ## Human control diff --git a/src/__tests__/upload-client-cancellation.test.ts b/src/__tests__/upload-client-cancellation.test.ts new file mode 100644 index 0000000000..ec1240da6f --- /dev/null +++ b/src/__tests__/upload-client-cancellation.test.ts @@ -0,0 +1,194 @@ +// Ending an artifact upload when the work it serves is over (#2946). +// +// A beat that finds the device's lease gone aborts the upload protecting that lease. An upload is a +// piped `node:http` request, so the only way to stop bytes already in flight is the request's own +// abort signal — a rejection the caller swallows would keep streaming a full app bundle to a device +// nobody owns. Kept out of `upload-client.test.ts`, which is already over the test-file size +// tripwire and may not grow (docs/agents/testing.md). + +import { afterEach, test } from 'vitest'; +import assert from 'node:assert/strict'; +import { createHash } from 'node:crypto'; +import fs from 'node:fs'; +import http, { type IncomingMessage, type ServerResponse } from 'node:http'; +import path from 'node:path'; +import { once } from 'node:events'; +import { uploadArtifact } from '../remote/upload-client.ts'; +import { mkdtempForTestSync } from './test-utils/tmp-dir.ts'; + +const TEST_TOKEN = 'agent-device-upload-cancel-token'; +const tempDirs: string[] = []; + +afterEach(async () => { + for (const dir of tempDirs) { + await fs.promises.rm(dir, { recursive: true, force: true }).catch(() => {}); + } + tempDirs.length = 0; +}); + +test('an aborted signal ends a legacy upload mid-stream instead of finishing the bytes', async () => { + const content = Buffer.alloc(8 * 1024 * 1024, 'x'); + const artifactPath = createTempFile('app.apk', content); + const control = new AbortController(); + let sawBytes = 0; + let connectionEnded: () => void; + const ended = new Promise((resolve) => { + connectionEnded = resolve; + }); + + // Preflight reports itself unsupported so the upload takes the legacy stream, the one path that + // pipes a file at the daemon and keeps going for as long as the daemon drains it. + const server = await startServer(async (req, res) => { + if (req.method === 'POST' && req.url === '/upload/preflight') { + await readRequestBody(req); + res.statusCode = 404; + res.end('not found'); + return; + } + if (req.method === 'POST' && req.url === '/upload') { + req.on('data', (chunk: Buffer) => { + sawBytes += chunk.length; + // The lease dies once bytes are genuinely in flight — not before the request starts. + // Stopping the read applies backpressure so the rest of the file cannot race through + // loopback and make "stopped early" a matter of timing. + req.pause(); + if (!control.signal.aborted) control.abort(); + }); + req.on('close', connectionEnded); + return; + } + res.statusCode = 404; + res.end('not found'); + }); + + try { + await assert.rejects( + async () => + await uploadArtifact({ + localPath: artifactPath, + baseUrl: server.baseUrl, + token: TEST_TOKEN, + signal: control.signal, + }), + ); + assert.ok(sawBytes > 0, 'the upload had started streaming before the abort'); + assert.ok(sawBytes < content.length, `the stream stopped early, at ${sawBytes} bytes`); + await ended; + } finally { + await server.close(); + } +}); + +test('an aborted signal before preflight refuses to ask the daemon for a ticket', async () => { + const artifactPath = createTempFile('app.apk', 'payload'); + const control = new AbortController(); + control.abort(); + const requests: string[] = []; + + const server = await startServer(async (req, res) => { + requests.push(`${req.method} ${req.url}`); + res.statusCode = 404; + res.end('not found'); + }); + + try { + await assert.rejects( + async () => + await uploadArtifact({ + localPath: artifactPath, + baseUrl: server.baseUrl, + token: TEST_TOKEN, + signal: control.signal, + }), + ); + assert.deepEqual(requests, [], 'an upload nobody waits for never reaches the daemon'); + } finally { + await server.close(); + } +}); + +test('an upload with no signal behaves exactly as before', async () => { + const content = 'unprotected-payload'; + const artifactPath = createTempFile('app.apk', content); + const expectedHash = createHash('sha256').update(content).digest('hex'); + + const server = await startServer(async (req, res) => { + if (req.method === 'POST' && req.url === '/upload/preflight') { + await readRequestBody(req); + res.statusCode = 404; + res.end('not found'); + return; + } + if (req.method === 'POST' && req.url === '/upload') { + assert.equal(req.headers['x-artifact-hash'], expectedHash); + await readRequestBody(req); + sendJson(res, { ok: true, uploadId: 'upload-uncancelled' }); + return; + } + res.statusCode = 404; + res.end('not found'); + }); + + try { + const uploadId = await uploadArtifact({ + localPath: artifactPath, + baseUrl: server.baseUrl, + token: TEST_TOKEN, + }); + assert.equal(uploadId, 'upload-uncancelled'); + } finally { + await server.close(); + } +}); + +function createTempFile(filename: string, content: string | Buffer): string { + const dir = mkdtempForTestSync('agent-device-upload-cancel-'); + tempDirs.push(dir); + const filePath = path.join(dir, filename); + fs.writeFileSync(filePath, content); + return filePath; +} + +async function startServer( + handler: (req: IncomingMessage, res: ServerResponse) => Promise, +): Promise<{ baseUrl: string; close: () => Promise }> { + const server = http.createServer((req, res) => { + void handler(req, res).catch(() => { + res.statusCode = 500; + res.end(); + }); + }); + // A canceled upload leaves its socket in a state `closeAllConnections` can race; without a bound + // idle keep-alive, `close` would then wait out Node's five-second default. + server.keepAliveTimeout = 100; + server.listen(0, '127.0.0.1'); + server.unref(); + await once(server, 'listening'); + const address = server.address(); + assert.ok(address && typeof address === 'object'); + return { + baseUrl: `http://127.0.0.1:${address.port}`, + close: async () => { + // A canceled upload leaves its socket half-open and a pooled keep-alive one behind; without + // dropping them, `close` would wait out the server's five-second keep-alive timeout. + server.closeAllConnections(); + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); + }, + }; +} + +async function readRequestBody(req: IncomingMessage): Promise { + const chunks: Buffer[] = []; + for await (const chunk of req) { + chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + } + return Buffer.concat(chunks); +} + +function sendJson(res: ServerResponse, body: unknown): void { + res.statusCode = 200; + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify(body)); +} diff --git a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts index 422af7920f..824110c5fa 100644 --- a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts +++ b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts @@ -8,7 +8,6 @@ import { buildLeaseHeartbeatRequest, buildUploadLeaseHeartbeat, createLeaseRenewalBeat, - LEASE_HEARTBEAT_INTERVAL_MS, leaseScopeForHeartbeat, runProtectedLeaseWork, } from '../daemon-client.ts'; @@ -36,39 +35,118 @@ afterEach(() => { vi.useRealTimers(); }); +function renewedLeaseResponse(windowMs: number): { + ok: true; + data: { lease: { heartbeatAt: number; expiresAt: number } }; +} { + return { ok: true, data: { lease: { heartbeatAt: 1_000_000, expiresAt: 1_000_000 + windowMs } } }; +} + describe('runProtectedLeaseWork', () => { test('runs the task untouched when there is no lease to protect', async () => { const heartbeat = vi.fn(); - assert.equal( - await runProtectedLeaseWork({ heartbeat: undefined, task: async () => 'ok' }), - 'ok', - ); + const phase = await runProtectedLeaseWork({ heartbeat: undefined, task: async () => 'ok' }); + assert.equal(phase, 'ok'); assert.equal(heartbeat.mock.calls.length, 0); }); - test('beats on the interval while a slow task runs, and stops when it ends', async () => { + test('a fast upload that lands before the first beat reports success', async () => { vi.useFakeTimers(); - const heartbeat = vi.fn(async () => ({})); - const upload = deferred(); + const heartbeat = vi.fn(async () => renewedLeaseResponse(30_000)); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: async (signal) => { + assert.ok(!signal.aborted, 'a lease still held does not cancel the upload'); + return 'installed'; + }, + heartbeat, + }); + assert.equal(await running, 'installed', 'the beat is armed but the upload is faster'); + }); + + test('the first beat fires immediately, not one interval into the upload', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => renewedLeaseResponse(30_000)); + const upload = deferred(); const running = runProtectedLeaseWork({ + intervalMs: 20_000, + task: () => upload.promise, heartbeat, + }); + + // A lease admitted with a short window used to be able to lapse before the first beat: the + // upload paid for the device with a lease nobody renewed yet. + await vi.advanceTimersByTimeAsync(0); + assert.equal(heartbeat.mock.calls.length, 1, 'a beat is out before the interval elapses'); + + upload.resolve('installed'); + assert.equal(await running, 'installed'); + }); + + test('beats follow the window the lease reports, not the fallback interval', async () => { + vi.useFakeTimers(); + // The daemon says it just extended the lease by 15s; the next beat must land at a third of + // that, whatever the caller's fallback cadence was. + const heartbeat = vi.fn(async () => renewedLeaseResponse(15_000)); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 60_000, task: () => upload.promise, + heartbeat, }); - await vi.advanceTimersByTimeAsync(LEASE_HEARTBEAT_INTERVAL_MS * 3); - // #2946: a 1m47s upload beat zero times and the lease died. Three intervals, three beats. + await vi.advanceTimersByTimeAsync(5_000); + assert.equal(heartbeat.mock.calls.length, 2, 'first beat at once, second one third of 15s in'); + await vi.advanceTimersByTimeAsync(5_000); assert.equal(heartbeat.mock.calls.length, 3); upload.resolve('installed'); assert.equal(await running, 'installed'); - const before = heartbeat.mock.calls.length; - await vi.advanceTimersByTimeAsync(LEASE_HEARTBEAT_INTERVAL_MS * 3); + await vi.advanceTimersByTimeAsync(60_000); assert.equal(heartbeat.mock.calls.length, before, 'no beat outlives the phase'); }); - test('never overlaps beats', async () => { + test('a beat shorter than the floor still beats no faster than the floor', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => renewedLeaseResponse(1_500)); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 60_000, + task: () => upload.promise, + heartbeat, + }); + + await vi.advanceTimersByTimeAsync(999); + assert.equal(heartbeat.mock.calls.length, 1, 'the floor holds a pathological window off'); + await vi.advanceTimersByTimeAsync(1); + assert.equal(heartbeat.mock.calls.length, 2); + + upload.resolve('installed'); + assert.equal(await running, 'installed'); + }); + + test('a response that names no window keeps the fallback cadence', async () => { + vi.useFakeTimers(); + const heartbeat = vi.fn(async () => ({ ok: true, data: {} })); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat, + }); + + await vi.advanceTimersByTimeAsync(10); + assert.equal(heartbeat.mock.calls.length, 2, 'immediate first, then the fallback interval'); + await vi.advanceTimersByTimeAsync(10); + assert.equal(heartbeat.mock.calls.length, 3); + + upload.resolve('installed'); + assert.equal(await running, 'installed'); + }); + + test('never overlaps beats, and a slow beat delays its successor instead of replacing the schedule', async () => { vi.useFakeTimers(); let inFlight = 0; let maxConcurrent = 0; @@ -87,14 +165,16 @@ describe('runProtectedLeaseWork', () => { }, }); - await vi.advanceTimersByTimeAsync(10); + await vi.advanceTimersByTimeAsync(0); assert.equal(gates.length, 1); - // Three more intervals pass with the first beat still outstanding: none of them may start a - // second one, or a slow beat would pile up behind a stalled transport. + // The fallback intervals pass with the first beat still outstanding: none of them may start a + // second one, or a stalled transport would pile beats up behind it. await vi.advanceTimersByTimeAsync(30); assert.equal(gates.length, 1, 'an outstanding beat holds the next one off'); + // The successor is armed when the slow beat lands — the cadence comes from completions, so a + // beat slower than the interval shifts the schedule instead of silently killing every later one. gates[0]!.resolve(); await vi.advanceTimersByTimeAsync(10); assert.equal(gates.length, 2, 'the next beat starts once the previous one lands'); @@ -106,16 +186,15 @@ describe('runProtectedLeaseWork', () => { assert.equal(maxConcurrent, 1); }); - test('a beat that fails for a reason other than the lease still running is survived', async () => { + test('a beat that fails for a transient reason is survived and re-armed', async () => { vi.useFakeTimers(); - const failing: () => Promise = async () => { - // A reason from the same registry that is not a lost lease: contention says nothing about - // whether this lease is still ours, so the upload keeps going and the next beat asks again. + // A reason from the same registry that is not a lost lease: contention says nothing about + // whether this lease is still ours, so the upload keeps going and the next beat asks again. + const heartbeat = vi.fn<() => Promise>(async () => { throw new AppError('DEVICE_IN_USE', 'Device is already leased', { reason: 'DEVICE_LEASE_BUSY', }); - }; - const heartbeat = vi.fn(failing); + }); const upload = deferred(); const running = runProtectedLeaseWork({ intervalMs: 10, @@ -123,10 +202,10 @@ describe('runProtectedLeaseWork', () => { heartbeat, }); - await vi.advanceTimersByTimeAsync(35); - assert.equal(heartbeat.mock.calls.length, 3, 'one lost beat does not stop the others'); + await vi.advanceTimersByTimeAsync(25); + assert.equal(heartbeat.mock.calls.length, 3, 'one failed beat does not stop the others'); - heartbeat.mockImplementation(async () => ({})); + heartbeat.mockImplementation(async () => ({ ok: true })); await vi.advanceTimersByTimeAsync(10); upload.resolve('installed'); assert.equal(await running, 'installed'); @@ -160,31 +239,73 @@ describe('runProtectedLeaseWork', () => { error.code === 'UNAUTHORIZED' && error.details?.reason === reason, ); - await vi.advanceTimersByTimeAsync(15); + await vi.advanceTimersByTimeAsync(0); await rejected; upload.resolve('too late'); }); } - test('an upload that lands before the first beat reports success', async () => { + for (const reason of ['LEASE_SCOPE_REQUIRED', 'LEASE_SCOPE_MISMATCH']) { + test(`a beat refused ${reason} ends the phase instead of beating to the lease's death`, async () => { + vi.useFakeTimers(); + // Both say this request can never renew the lease — the scope it names is missing or belongs + // to someone else. Surviving would spend the whole upload on a lease that stops renewing: + // the #2946 symptom recreated on the client's own side. + const heartbeat = vi.fn(async () => { + throw new AppError('UNAUTHORIZED', "Lease scope is not this request's", { reason }); + }); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat, + }); + + const rejected = assert.rejects( + running, + (error: unknown) => + error instanceof AppError && + error.code === 'UNAUTHORIZED' && + error.details?.reason === reason, + ); + await vi.advanceTimersByTimeAsync(50); + assert.equal( + heartbeat.mock.calls.length, + 1, + 'a doomed renewal is not retried for the window', + ); + await rejected; + upload.resolve('too late'); + }); + } + + test('a beat that ends the protection cancels the upload the phase is running', async () => { vi.useFakeTimers(); - const upload = deferred(); + const heartbeat = vi.fn(async () => { + throw lostLeaseError('LEASE_NOT_FOUND'); + }); + let sawAbort = false; const running = runProtectedLeaseWork({ intervalMs: 10, - task: () => upload.promise, - heartbeat: async () => { - throw lostLeaseError('LEASE_NOT_FOUND'); - }, + task: (signal) => + new Promise((_resolve, reject) => { + signal.addEventListener('abort', () => { + sawAbort = true; + reject(signal.reason); + }); + }), + heartbeat, }); - upload.resolve('installed'); + const rejected = assert.rejects(running); await vi.advanceTimersByTimeAsync(0); - assert.equal(await running, 'installed'); + await rejected; + assert.equal(sawAbort, true, 'the upload is told to stop before the bytes finish'); }); test('a phase that throws synchronously still stops the beats', async () => { vi.useFakeTimers(); - const heartbeat = vi.fn(async () => ({})); + const heartbeat = vi.fn(async () => ({ ok: true })); await assert.rejects( (async () => await runProtectedLeaseWork({ @@ -219,7 +340,7 @@ describe('runProtectedLeaseWork', () => { running, (error: unknown) => error instanceof AppError && error.details?.reason === 'LEASE_NOT_FOUND', ); - await vi.advanceTimersByTimeAsync(10); + await vi.advanceTimersByTimeAsync(0); assert.ok(beat, 'a beat started'); first.resolve('installed'); beat?.(); @@ -229,7 +350,7 @@ describe('runProtectedLeaseWork', () => { test('a task rejection propagates and still stops the beats', async () => { vi.useFakeTimers(); - const heartbeat = vi.fn(async () => ({})); + const heartbeat = vi.fn(async () => ({ ok: true })); const running = runProtectedLeaseWork({ intervalMs: 10, task: async () => { @@ -244,22 +365,22 @@ describe('runProtectedLeaseWork', () => { assert.equal(heartbeat.mock.calls.length, before); }); - test('an in-flight beat is awaited on the way out', async () => { + test('an in-flight beat is awaited on the way out and arms nothing after it', async () => { vi.useFakeTimers(); const beat = deferred(); const upload = deferred(); - let started = false; + const heartbeat = vi.fn(async () => { + await beat.promise; + return renewedLeaseResponse(30_000); + }); const running = runProtectedLeaseWork({ intervalMs: 10, task: () => upload.promise, - heartbeat: async () => { - started = true; - await beat.promise; - }, + heartbeat, }); - await vi.advanceTimersByTimeAsync(10); - assert.equal(started, true, 'a beat is in flight while the upload finishes'); + await vi.advanceTimersByTimeAsync(0); + assert.equal(heartbeat.mock.calls.length, 1, 'a beat is in flight while the upload finishes'); upload.resolve('installed'); await vi.advanceTimersByTimeAsync(0); @@ -272,6 +393,8 @@ describe('runProtectedLeaseWork', () => { beat.resolve(); assert.equal(await running, 'installed'); + await vi.advanceTimersByTimeAsync(60_000); + assert.equal(heartbeat.mock.calls.length, 1, 'the renewal that landed last arms no successor'); }); }); @@ -445,6 +568,19 @@ describe('buildUploadLeaseHeartbeat', () => { serverMode: 'socket' as const, }; + test('no beat for a remote request that names no lease, which has nothing to renew', () => { + // The lease-less path is the one an unleased install takes; a timer there would beat a lease + // that does not exist and keep a request alive that owns no device. + assert.equal( + buildUploadLeaseHeartbeat( + { baseUrl: 'http://remote.example.test/agent-device', token: 't', pid: 1 }, + settings, + { ...installRequest, flags: {}, meta: undefined }, + ), + undefined, + ); + }); + test('no beat for a local daemon, which never uploads and holds no billed device', () => { assert.equal( buildUploadLeaseHeartbeat( diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index ccacd5b96b..437d3d0100 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -69,7 +69,7 @@ export async function sendToDaemon( const info = daemon.info; const preparedRemoteRequest = await runProtectedLeaseWork({ heartbeat: buildUploadLeaseHeartbeat(info, settings, requestWithoutAuthFlag), - task: () => prepareRemoteRequestArtifacts(requestWithoutAuthFlag, info), + task: (signal) => prepareRemoteRequestArtifacts(requestWithoutAuthFlag, info, signal), }); writeInstallInProgressNotice(requestWithoutAuthFlag.command); @@ -277,21 +277,44 @@ function withActiveSessionAddressHint( } /** - * How often a long client-side phase renews the lease it is waiting under. + * The cadence a long client-side phase beats on before it has learned the lease's own window. * - * A third of the daemon's one-minute default inactivity TTL: a beat always lands while two thirds of - * the window it is protecting is still open, so one slow or lost beat cannot cost the lease. + * The first beat answers with the window the daemon just renewed, and from then on the phase beats a + * third of that window. This constant only covers the gap until that answer lands, and the phase + * when the caller overrides it. */ -export const LEASE_HEARTBEAT_INTERVAL_MS = 20_000; +const LEASE_HEARTBEAT_INTERVAL_MS = 20_000; + +/** + * The floor for a window-derived cadence. + * + * A lease admitted with the registry's minimum five-second window beats every 1.6s at one third of + * its window; the floor keeps a misreported or pathologically short window from turning the beat + * into a request loop against the daemon it is trying to stay admitted to. + */ +const MIN_LEASE_BEAT_INTERVAL_MS = 1_000; /** Why a beat stopped protecting the lease: the lease is gone, so nothing else is worth waiting for. */ -const FATAL_LEASE_BEAT_REASONS: ReadonlySet = new Set([ +const LOST_LEASE_BEAT_REASONS: ReadonlySet = new Set([ 'LEASE_NOT_FOUND', 'LEASE_EXPIRED', 'LEASE_REVOKED', 'LEASE_SESSION_MISMATCH', ]); +/** + * Why a beat stopped protecting the lease even though the lease may live on: this client's request + * will never be the one that renews it. + * + * A beat refused for a missing or mismatched owner scope is a fact about the request, not the + * lease, so every successor is refused identically. Surviving it would only spend the upload against + * a lease that stops renewing — the #2946 failure with extra steps. + */ +const UNRENEWABLE_LEASE_BEAT_REASONS: ReadonlySet = new Set([ + 'LEASE_SCOPE_REQUIRED', + 'LEASE_SCOPE_MISMATCH', +]); + /** * Runs one client-side phase under a lease it does not own the clock of. * @@ -302,72 +325,114 @@ const FATAL_LEASE_BEAT_REASONS: ReadonlySet = new Set([ * being uploaded to (#2946). * * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and - * the phase runs untouched. A beat that fails for a reason other than the lease being gone is - * reported and ignored — one lost request must not fail an upload that a later beat will cover. A - * beat that finds the lease gone ends the phase immediately with that error: the device is no - * longer ours, and the only honest outcome is to say so before the bytes finish. + * the phase runs untouched. The first beat is fired immediately rather than one interval in, so a + * lease shorter than that interval is renewed before it can lapse — and a lease already gone is + * found before any bytes move. Each beat answers with the window it just renewed, and the next one + * is armed for a third of that window from the beat that landed: beats never overlap, and a slow + * one delays its successor instead of suppressing every beat after it. * - * Beats never overlap, and an in-flight beat is awaited on the way out so a renewal cannot land - * after the phase it was protecting. + * A beat that fails for a reason that says nothing about this lease is reported and survived — one + * lost request must not fail an upload that a later beat will cover. A beat that finds the lease + * gone, or finds this client can never renew it, ends the phase with that error and aborts the + * signal the phase runs under: the device is no longer ours (or was never reachable through this + * request), and the only honest outcome is to say so before the bytes finish. */ export async function runProtectedLeaseWork( options: Readonly<{ heartbeat: (() => Promise) | undefined; intervalMs?: number; - task: () => Promise; + task: (signal: AbortSignal) => Promise; }>, ): Promise { const { heartbeat } = options; - if (!heartbeat) return await options.task(); - const intervalMs = options.intervalMs ?? LEASE_HEARTBEAT_INTERVAL_MS; + if (!heartbeat) return await options.task(new AbortController().signal); + const fallbackIntervalMs = options.intervalMs ?? LEASE_HEARTBEAT_INTERVAL_MS; + const control = new AbortController(); + let intervalMs = fallbackIntervalMs; + let timer: ReturnType | undefined; + let stopped = false; let inFlight: Promise | undefined; - let lostLease: unknown; - let reportLoss: ((error: unknown) => void) | undefined; - const lost = new Promise((_, reject) => { - reportLoss = reject; + let terminalError: unknown; + let reportTerminal: ((error: unknown) => void) | undefined; + const terminal = new Promise((_, reject) => { + reportTerminal = reject; }); - const beat = () => { - if (inFlight) return; + const runBeat = (): void => { inFlight = (async () => { try { - await heartbeat(); + const renewed = leaseWindowFromHeartbeatResponse(await heartbeat()); + if (renewed !== undefined) { + intervalMs = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); + } } catch (error) { - if (!isLostLeaseError(error)) { - emitDiagnostic({ - level: 'warn', - phase: 'lease_heartbeat_failed', - data: { message: error instanceof Error ? error.message : String(error) }, - }); + if (isTerminalLeaseBeatError(error)) { + terminalError = error; + // The upload is the only thing still consuming this phase's time, and it is pointed at a + // device this client can no longer renew. Stop it rather than finish bytes nobody owns. + control.abort(); + reportTerminal?.(error); return; } - lostLease = error; - reportLoss?.(error); - } finally { - inFlight = undefined; + emitDiagnostic({ + level: 'warn', + phase: 'lease_heartbeat_failed', + data: { message: error instanceof Error ? error.message : String(error) }, + }); } + // A beat that lands after the phase settled must not arm a successor: nothing is left to + // protect, and a beat without a phase to stop it would renew the lease forever. + if (!stopped) timer = setTimeout(runBeat, intervalMs); })(); }; - const timer = setInterval(beat, intervalMs); + // Armed before the phase starts, not one interval in: a beat is what proves the lease the upload + // is spending its time on is still alive. + timer = setTimeout(runBeat, 0); // A beat already in flight when the phase settles can still report a lost lease; the caller reads - // it from `lostLease`, so nothing may be left racing on this rejection by then. - void lost.catch(() => undefined); + // it from `terminalError`, so nothing may be left racing on this rejection by then. + void terminal.catch(() => undefined); const phase = await captureOutcome( // The async boundary also turns a synchronous throw from the phase into a rejection, so the // timer below is always cleared. - (async () => await Promise.race([options.task(), lost]))(), + (async () => await Promise.race([options.task(control.signal), terminal]))(), ); - clearInterval(timer); + stopped = true; + if (timer) clearTimeout(timer); await inFlight; - // A beat that found the lease gone outranks a phase that settled meanwhile, from either side: the + // A beat that ended the protection outranks a phase that settled meanwhile, from either side: the // device is no longer ours, and the lease error is the reason the phase was not worth finishing. - if (lostLease !== undefined) throw lostLease; + if (terminalError !== undefined) throw terminalError; if (!phase.ok) throw phase.error; return phase.value; } +/** + * The inactivity window a beat just renewed, read from the lease its response carries. + * + * `heartbeatLease` answers with the lease, whose `expiresAt - heartbeatAt` is exactly the window it + * extended — the same pair `leaseOwnTtlMs` renews on. Anything unrecognizable leaves the caller on + * the fallback cadence rather than guessing one. + */ +function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined { + const lease = ( + response as Readonly<{ data?: Readonly<{ lease?: Readonly> }> }> + )?.data?.lease; + const expiresAt = lease?.expiresAt; + const heartbeatAt = lease?.heartbeatAt; + if (typeof expiresAt !== 'number' || typeof heartbeatAt !== 'number') return undefined; + return expiresAt > heartbeatAt ? expiresAt - heartbeatAt : undefined; +} + +function isTerminalLeaseBeatError(error: unknown): boolean { + return ( + error instanceof AppError && + (LOST_LEASE_BEAT_REASONS.has(error.details?.reason) || + UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason)) + ); +} + type Outcome = { ok: true; value: T } | { ok: false; error: unknown }; async function captureOutcome(promise: Promise): Promise> { @@ -378,10 +443,6 @@ async function captureOutcome(promise: Promise): Promise> { } } -function isLostLeaseError(error: unknown): boolean { - return error instanceof AppError && FATAL_LEASE_BEAT_REASONS.has(error.details?.reason); -} - /** * The beat that keeps a remote lease alive across a long client-side phase. * diff --git a/src/remote/__tests__/daemon-artifacts-save-script.test.ts b/src/remote/__tests__/daemon-artifacts-save-script.test.ts index 9ecfd13c0e..137217d2ba 100644 --- a/src/remote/__tests__/daemon-artifacts-save-script.test.ts +++ b/src/remote/__tests__/daemon-artifacts-save-script.test.ts @@ -14,6 +14,7 @@ import { prepareRemoteRequestArtifacts } from '../daemon-artifacts.ts'; const REMOTE = { baseUrl: 'http://remote-mac.example.test:7777/agent-device', token: 'secret' }; const LOCAL = { token: 'secret' }; +const NO_CANCELLATION = new AbortController().signal; function replayRequest(saveScript?: boolean | string) { return { @@ -27,7 +28,7 @@ function replayRequest(saveScript?: boolean | string) { test('a remote daemon refuses --save-script, naming the caller/daemon split', async () => { await assert.rejects( - async () => await prepareRemoteRequestArtifacts(replayRequest(true), REMOTE), + async () => await prepareRemoteRequestArtifacts(replayRequest(true), REMOTE, NO_CANCELLATION), (error: unknown) => error instanceof AppError && error.code === 'INVALID_ARGS' && @@ -38,20 +39,24 @@ test('a remote daemon refuses --save-script, naming the caller/daemon split', as test('a remote daemon refuses an explicit --save-script output path too', async () => { await assert.rejects( async () => - await prepareRemoteRequestArtifacts(replayRequest('./flows/login.healed.ad'), REMOTE), + await prepareRemoteRequestArtifacts( + replayRequest('./flows/login.healed.ad'), + REMOTE, + NO_CANCELLATION, + ), (error: unknown) => error instanceof AppError && error.code === 'INVALID_ARGS', ); }); test('a local daemon leaves --save-script untouched', async () => { - const prepared = await prepareRemoteRequestArtifacts(replayRequest(true), LOCAL); + const prepared = await prepareRemoteRequestArtifacts(replayRequest(true), LOCAL, NO_CANCELLATION); assert.equal(prepared.flags?.saveScript, true); assert.deepEqual(prepared.positionals, ['./flows/login.ad']); }); test('a remote replay without --save-script passes through unchanged', async () => { - const prepared = await prepareRemoteRequestArtifacts(replayRequest(), REMOTE); + const prepared = await prepareRemoteRequestArtifacts(replayRequest(), REMOTE, NO_CANCELLATION); assert.deepEqual(prepared.positionals, ['./flows/login.ad']); assert.equal(prepared.uploadedArtifactId, undefined); diff --git a/src/remote/__tests__/daemon-artifacts-test-command.test.ts b/src/remote/__tests__/daemon-artifacts-test-command.test.ts index b804135b5c..898ecc3543 100644 --- a/src/remote/__tests__/daemon-artifacts-test-command.test.ts +++ b/src/remote/__tests__/daemon-artifacts-test-command.test.ts @@ -12,6 +12,7 @@ import { prepareRemoteRequestArtifacts } from '../daemon-artifacts.ts'; const REMOTE = { baseUrl: 'http://remote-mac.example.test:7777/agent-device', token: 'secret' }; const LOCAL = { token: 'secret' }; +const NO_CANCELLATION = new AbortController().signal; function testRequest(artifactsDir: string | undefined, cwd = '/repo') { return { @@ -27,6 +28,7 @@ test('a remote daemon redirects an explicit --artifacts-dir to a temp path it ow const prepared = await prepareRemoteRequestArtifacts( testRequest('remote-device-artifacts/ad-test'), REMOTE, + NO_CANCELLATION, ); const redirected = (prepared.flags as Record | undefined)?.artifactsDir; @@ -39,7 +41,11 @@ test('a remote daemon redirects an explicit --artifacts-dir to a temp path it ow }); test('a remote daemon redirects the default artifacts directory too', async () => { - const prepared = await prepareRemoteRequestArtifacts(testRequest(undefined), REMOTE); + const prepared = await prepareRemoteRequestArtifacts( + testRequest(undefined), + REMOTE, + NO_CANCELLATION, + ); const redirected = (prepared.flags as Record | undefined)?.artifactsDir; assert.equal(typeof redirected, 'string'); @@ -54,6 +60,7 @@ test('a remote daemon leaves an already-absolute --artifacts-dir as the download const prepared = await prepareRemoteRequestArtifacts( testRequest('/ci/artifacts/ad-test'), REMOTE, + NO_CANCELLATION, ); const redirected = (prepared.flags as Record | undefined)?.artifactsDir; @@ -65,6 +72,7 @@ test('a local daemon leaves --artifacts-dir untouched', async () => { const prepared = await prepareRemoteRequestArtifacts( testRequest('remote-device-artifacts/ad-test'), LOCAL, + NO_CANCELLATION, ); assert.equal( diff --git a/src/remote/daemon-artifacts.ts b/src/remote/daemon-artifacts.ts index c99a848f7b..4a56d088f7 100644 --- a/src/remote/daemon-artifacts.ts +++ b/src/remote/daemon-artifacts.ts @@ -31,6 +31,7 @@ type PreparedRemoteRequest = { export async function prepareRemoteRequestArtifacts( req: Omit, info: DaemonArtifactEndpoint, + signal: AbortSignal, ): Promise { const positionals = [...(req.positionals ?? [])]; let flags = req.flags ? { ...req.flags } : undefined; @@ -51,7 +52,7 @@ export async function prepareRemoteRequestArtifacts( assertRemoteDaemonSupportsSaveScript(req); flags = applyRemoteArtifactCommand(req, positionals, flags, clientArtifactPaths); - const remoteInstallSource = await prepareRemoteInstallSource(req, info, uploadProgress); + const remoteInstallSource = await prepareRemoteInstallSource(req, info, uploadProgress, signal); if (remoteInstallSource) { installSource = remoteInstallSource.installSource; uploadedArtifactId = remoteInstallSource.uploadedArtifactId ?? uploadedArtifactId; @@ -72,6 +73,7 @@ export async function prepareRemoteRequestArtifacts( info, positionals, uploadProgress, + signal, ); uploadedArtifactId = installPackageResult ?? uploadedArtifactId; return baseResult(); @@ -103,6 +105,7 @@ async function prepareRemoteInstallPackage( info: DaemonArtifactEndpoint, positionals: string[], onProgress: UploadProgressSink | undefined, + signal: AbortSignal, ): Promise { const pathIndex = positionals.length === 1 ? 0 : 1; const rawPath = positionals[pathIndex]; @@ -121,6 +124,7 @@ async function prepareRemoteInstallPackage( token: info.token, platform: req.flags?.platform, onProgress, + signal, }); } @@ -178,6 +182,7 @@ async function prepareRemoteInstallSource( req: Omit, info: DaemonArtifactEndpoint, onProgress: UploadProgressSink | undefined, + signal: AbortSignal, ): Promise<{ installSource: NonNullable['installSource']; uploadedArtifactId?: string; @@ -218,6 +223,7 @@ async function prepareRemoteInstallSource( token: info.token, platform: req.flags?.platform, onProgress, + signal, }); return { installSource: { diff --git a/src/remote/upload-client.ts b/src/remote/upload-client.ts index 25232764d6..3f0fe10c63 100644 --- a/src/remote/upload-client.ts +++ b/src/remote/upload-client.ts @@ -14,6 +14,11 @@ type UploadArtifactOptions = { token: string; platform?: string; onProgress?: UploadProgressSink; + /** + * Ends the upload when the thing it is uploading for stops being worth finishing — today, a beat + * that found the device's lease gone. Aborted requests reject with the signal's own reason. + */ + signal?: AbortSignal; }; type UploadResponse = { @@ -54,6 +59,7 @@ export async function uploadArtifact(options: UploadArtifactOptions): Promise; uploadAttemptId: string; onProgress?: UploadProgressSink; + signal?: AbortSignal; }): Promise { const uploadOnce = async ( preflight: Extract, ): Promise => { - await uploadDirectArtifact(options.artifact, preflight, options.onProgress); + await uploadDirectArtifact(options.artifact, preflight, options.onProgress, options.signal); return await finalizeDirectUpload({ normalizedBase: options.normalizedBase, token: options.token, uploadId: preflight.uploadId, + signal: options.signal, }); }; try { return await uploadOnce(options.preflight); } catch (error) { + // A canceled upload resumes for no reason: the thing it was uploaded for is already gone, and + // re-preflighting would ask the daemon for a fresh ticket to send bytes nobody is waiting on. + if (options.signal?.aborted) throw error; if (!shouldRetryDirectUpload(error)) return undefined; - const retryPreflight = await requestUploadPreflight({ - normalizedBase: options.normalizedBase, - token: options.token, - artifact: options.artifact, - uploadAttemptId: options.uploadAttemptId, - }); - if (retryPreflight?.kind === 'cache-hit') { - return retryPreflight.uploadId; - } - if (retryPreflight?.kind === 'direct-upload') { - try { - return await uploadOnce(retryPreflight); - } catch { - return undefined; - } + return await retryDirectUpload(options, uploadOnce); + } +} + +/** + * One fresh attempt after a retryable stream failure, asked for under the same attempt id. + * + * Anything short of a usable ticket — a cache hit excepted, which is itself a finished upload — + * hands the caller back to the legacy path. A second failure is not retried again: a ticket that + * failed twice is a transport or ticket problem the legacy route may still survive. + */ +async function retryDirectUpload( + options: { + normalizedBase: string; + token: string; + artifact: PreparedUploadArtifact; + uploadAttemptId: string; + onProgress?: UploadProgressSink; + signal?: AbortSignal; + }, + uploadOnce: ( + preflight: Extract, + ) => Promise, +): Promise { + const retryPreflight = await requestUploadPreflight({ + normalizedBase: options.normalizedBase, + token: options.token, + artifact: options.artifact, + uploadAttemptId: options.uploadAttemptId, + signal: options.signal, + }); + if (retryPreflight?.kind === 'cache-hit') { + return retryPreflight.uploadId; + } + if (retryPreflight?.kind === 'direct-upload') { + try { + return await uploadOnce(retryPreflight); + } catch { + return undefined; } - return undefined; } + return undefined; } function shouldRetryDirectUpload(error: unknown): boolean { @@ -147,6 +185,7 @@ async function uploadLegacyArtifact(options: { token: string; artifact: PreparedUploadArtifact; onProgress?: UploadProgressSink; + signal?: AbortSignal; }): Promise { const { normalizedBase, token, artifact } = options; const uploadUrl = new URL('upload', normalizedBase); @@ -170,6 +209,7 @@ async function uploadLegacyArtifact(options: { timeoutHint: 'The upload to the remote daemon exceeded the 5-minute timeout.', errorMessage: 'Failed to upload artifact to remote daemon', errorHint: 'Verify the remote daemon is reachable and supports artifact uploads.', + signal: options.signal, progress: { stage: 'legacy', fileName: artifact.fileName, @@ -194,6 +234,7 @@ async function requestUploadPreflight(options: { token: string; artifact: PreparedUploadArtifact; uploadAttemptId: string; + signal?: AbortSignal; }): Promise { const preflightUrl = new URL('upload/preflight', options.normalizedBase); const headers: Record = { @@ -204,7 +245,7 @@ async function requestUploadPreflight(options: { const response = await fetch(preflightUrl, { method: 'POST', headers, - signal: AbortSignal.timeout(UPLOAD_PREFLIGHT_TIMEOUT_MS), + signal: combineUploadSignals(options.signal, UPLOAD_PREFLIGHT_TIMEOUT_MS), body: JSON.stringify({ uploadAttemptId: options.uploadAttemptId, sha256: options.artifact.sha256, @@ -214,7 +255,12 @@ async function requestUploadPreflight(options: { ...(options.artifact.platform ? { platform: options.artifact.platform } : {}), contentType: options.artifact.contentType, }), - }).catch(() => undefined); + }).catch((error: unknown) => { + // A failed preflight is ordinary here — the caller falls back to the legacy upload. A canceled + // one is not: there is nothing to fall back to when the lease that paid for this upload is gone. + if (options.signal?.aborted) throw error; + return undefined; + }); if (!response?.ok) { return undefined; @@ -223,6 +269,15 @@ async function requestUploadPreflight(options: { return parseUploadPreflightResult(await response.json().catch(() => undefined)); } +/** + * The signal one upload request runs under: the caller's cancellation and the request's own + * timeout, whichever fires first. + */ +function combineUploadSignals(signal: AbortSignal | undefined, timeoutMs: number): AbortSignal { + const timeout = AbortSignal.timeout(timeoutMs); + return signal ? AbortSignal.any([signal, timeout]) : timeout; +} + function parseUploadPreflightResult(value: unknown): UploadPreflightResult | undefined { if (!value || typeof value !== 'object') { return undefined; @@ -265,6 +320,7 @@ async function uploadDirectArtifact( artifact: PreparedUploadArtifact, ticket: Extract, onProgress: UploadProgressSink | undefined, + signal: AbortSignal | undefined, ): Promise { const response = await streamFileToHttpRequest({ url: new URL(ticket.url), @@ -275,6 +331,7 @@ async function uploadDirectArtifact( timeoutHint: 'The direct upload ticket did not accept the artifact within the timeout.', errorMessage: 'Failed to upload artifact with direct upload ticket', retryable: true, + signal, progress: { stage: 'direct', fileName: artifact.fileName, @@ -294,6 +351,7 @@ async function finalizeDirectUpload(options: { normalizedBase: string; token: string; uploadId: string; + signal?: AbortSignal; }): Promise { const finalizeUrl = new URL('upload/finalize', options.normalizedBase); const headers: Record = { @@ -304,7 +362,7 @@ async function finalizeDirectUpload(options: { const response = await fetch(finalizeUrl, { method: 'POST', headers, - signal: AbortSignal.timeout(UPLOAD_PREFLIGHT_TIMEOUT_MS), + signal: combineUploadSignals(options.signal, UPLOAD_PREFLIGHT_TIMEOUT_MS), body: JSON.stringify({ uploadId: options.uploadId }), }).catch((error) => { throw new AppError('COMMAND_FAILED', 'Failed to finalize direct artifact upload', {}, error); diff --git a/src/remote/upload-stream.ts b/src/remote/upload-stream.ts index 608b48bdd8..db640bddd7 100644 --- a/src/remote/upload-stream.ts +++ b/src/remote/upload-stream.ts @@ -36,6 +36,8 @@ export async function streamFileToHttpRequest(options: { errorMessage: string; errorHint?: string; retryable?: boolean; + /** Ends this request when the work the upload serves is over; see `uploadArtifact`. */ + signal?: AbortSignal; progress?: UploadStreamProgressOptions; }): Promise { return await streamFileToHttpRequestAttempt({ @@ -60,6 +62,7 @@ async function streamFileToHttpRequestAttempt(options: { errorMessage: string; errorHint?: string; retryable?: boolean; + signal?: AbortSignal; redirectCount: number; startOffset: number; progress?: UploadStreamProgressOptions; @@ -82,6 +85,9 @@ async function streamFileToHttpRequestAttempt(options: { method: options.method, path: options.url.pathname + options.url.search, headers, + // Aborting destroys the request mid-stream, which is the only way to stop bytes that are + // already piped at a device this client no longer holds the lease on. + ...(options.signal ? { signal: options.signal } : {}), }, (res) => { responseReceived = true; diff --git a/test/integration/provider-scenarios/remote-daemon-client.test.ts b/test/integration/provider-scenarios/remote-daemon-client.test.ts index 1b1ae2cb70..c80c5df38e 100644 --- a/test/integration/provider-scenarios/remote-daemon-client.test.ts +++ b/test/integration/provider-scenarios/remote-daemon-client.test.ts @@ -14,6 +14,8 @@ import { skipWhenLoopbackUnavailable, } from '../../../src/__tests__/test-utils/loopback.ts'; +const NO_CANCELLATION = new AbortController().signal; + type RemoteRpcRequest = { id: unknown; method?: string; @@ -678,6 +680,7 @@ test('remote web recording defaults client and daemon artifact paths to WebM', a meta: { cwd: '/tmp/project' }, }, { baseUrl: 'http://127.0.0.1:1', token: 'remote-token' }, + NO_CANCELLATION, ); assert.equal(prepared.positionals[0], 'start'); @@ -698,6 +701,7 @@ test('remote web recording appends WebM extension to extensionless client paths' meta: { cwd: '/tmp/project' }, }, { baseUrl: 'http://127.0.0.1:1', token: 'remote-token' }, + NO_CANCELLATION, ); assert.equal(prepared.positionals[0], 'start'); @@ -715,6 +719,7 @@ test('remote recording without platform or requested path lets daemon choose ses meta: { cwd: '/tmp/project' }, }, { baseUrl: 'http://127.0.0.1:1', token: 'remote-token' }, + NO_CANCELLATION, ); assert.deepEqual(prepared.positionals, ['start']); From f3253cccd13344aac46d4f2761cc293ce3f79eeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:21:58 +0200 Subject: [PATCH 05/21] chore(gates): ack the upload wire digests the abort signal moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every moved declaration is a client-side optional parameter — the request bytes an older daemon parses are unchanged, and an aborted request is the client disconnect it already handles — so each gets its own digest-keyed compatibleChanges entry rather than a protocol bump. AbortSignal joins TS_GLOBALS: a lib global with no declaration site to digest, like URL. --- test/wire-compat/ledger.json | 49 +++++++++++++++++++++++---- test/wire-compat/module-resolution.ts | 1 + 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/test/wire-compat/ledger.json b/test/wire-compat/ledger.json index 181e04ba82..e107c65d42 100644 --- a/test/wire-compat/ledger.json +++ b/test/wire-compat/ledger.json @@ -161,14 +161,14 @@ "src/remote/upload-client.ts#UploadPreflightResponse": "sha256:196a1d9ef41a1aec192589fa7ccb30cf71d971a005349d5d73eed42e6ab21327", "src/remote/upload-client.ts#UploadPreflightResult": "sha256:bd8fc6d617c0630325455fd64d0fe319daaced198ee6745e4673a8ace3df460d", "src/remote/upload-client.ts#UploadResponse": "sha256:499e01466b5e0e03413f8d3f1986f5af6bc120704c50a3747d84de422b3f0314", - "src/remote/upload-client.ts#finalizeDirectUpload": "sha256:5e32c87214f9f6140535b48d05de8565cb121370fdc334c05f7f0ad476b73a07", + "src/remote/upload-client.ts#finalizeDirectUpload": "sha256:04e18bdf91b9930a04d43f1661fe3f31ac2e6018154f8a8cb14656b9bc2b6140", "src/remote/upload-client.ts#isStringRecord": "sha256:b6d6b571b90baf0976d9c347c4a45a4363a4df5ad184ea1087cb3b9febd4b624", "src/remote/upload-client.ts#parseUploadPreflightResult": "sha256:7ce3f43b45c03df8b7f2690432beedc7a537c1bba540961716862f8a632d5623", - "src/remote/upload-client.ts#requestUploadPreflight": "sha256:df42c0eaef3a54f4388c99d74bf6030885239780c2ddc78117151dbd391afedd", + "src/remote/upload-client.ts#requestUploadPreflight": "sha256:4c9863bce8757f1dc0f01ec7374019e752efc8e786cd070200f516a1d72d09e1", "src/remote/upload-client.ts#shouldRetryDirectUpload": "sha256:29059f076ff7f2b9540ad15408be48792b2d63a601ef2b9fb2d54f5576d49bbd", - "src/remote/upload-client.ts#tryDirectUploadWithResume": "sha256:0f57cf9bb1eefa3b988e5d723b2eebf2c4a622c3f7c808f35eae070771acafa6", - "src/remote/upload-client.ts#uploadDirectArtifact": "sha256:5f26f8f7b6ada2fe2089a214fb8350e3a9f65725212952c17f6787b3ca8fe232", - "src/remote/upload-client.ts#uploadLegacyArtifact": "sha256:f3ccab9c5ba18fc2f2000ea8e2a4bbb3cc97f7b50f6111192e513013f635cdd7", + "src/remote/upload-client.ts#tryDirectUploadWithResume": "sha256:67596a2ff2b8a5402566c3b7b99a6fa1baaf9d5de3d647bf35398185cc45eea6", + "src/remote/upload-client.ts#uploadDirectArtifact": "sha256:c16cb5dd74895931b8e41e16629a0519e5601340563c976b7f800c6de8d158b2", + "src/remote/upload-client.ts#uploadLegacyArtifact": "sha256:a7a82d6eb2925d6f510b9cc00f01f9ce865638368e0bec503b700f0bc59871bc", "src/remote/upload-stream.ts#MAX_UPLOAD_REDIRECTS": "sha256:12ff46dfe33a1dce95bdb79c90c17a3ba4f3919e7dc4b3a1b04590fff430f79f", "src/remote/upload-stream.ts#UploadStreamResponse": "sha256:724e284c2a6bd7b4b3a49b1d8c61c8923d6cc24777c3d76224a69d190e56adce", "src/remote/upload-stream.ts#buildUploadRequestHeaders": "sha256:827576b13a9f3e6cea445119a6a29b2ac5eaa23b66436cf810b2d09765c82a4c", @@ -177,8 +177,8 @@ "src/remote/upload-stream.ts#isUploadResumeStatus": "sha256:9f730ccd1c3401c326f8df67d1fd8243fef757357a38a1f53736df2b90bd8fdc", "src/remote/upload-stream.ts#parseNonNegativeIntegerHeader": "sha256:5cbe9c6994459d673d988a51407e65556e28175b187ce0a9d27cbb6331962834", "src/remote/upload-stream.ts#parseUploadResumeOffset": "sha256:297184ea22a6dc1cda9aee17034122ae0ea2f43d0355647f6fcf02f3f4b5ca0e", - "src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:eaf2ea49957034f6a7e17d34092dc6cb1998812bddfd68381a0ac34d8a2de58c", - "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:da39a79fa7c1f81e55caf613eedc347c0f3a9a9711b265a4db185677532d9552" + "src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:ce07ea33a275e06e4cceaf0c75a079bd0c7938f36df6817407dbb1cc8c4ff900", + "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:bc636083b00c2eef73a65a3446491e831510cdf934ff2545dd12e6b6ba13160d" }, "compatibleChanges": [ { @@ -300,6 +300,41 @@ "declaration": "packages/kernel/src/contracts.ts#daemonRuntimeSchema", "digest": "sha256:3b99926ce9deb5d66186a55b1168011dc1ae4d010ada5ac5d79c4c7225a1f80f", "rationale": "#2266 broadens runtime-hint validation to accept the additive HarmonyOS platform value; existing protocol-2 runtime hints remain valid and unchanged." + }, + { + "declaration": "src/remote/upload-client.ts#requestUploadPreflight", + "digest": "sha256:4c9863bce8757f1dc0f01ec7374019e752efc8e786cd070200f516a1d72d09e1", + "rationale": "#2946 runs the preflight fetch under the caller's abort signal in addition to its own timeout. Route, headers, and JSON body are unchanged, so a protocol-2 daemon parses an uncancelled preflight exactly as before; an aborted one is the client disconnect it already handles." + }, + { + "declaration": "src/remote/upload-client.ts#uploadDirectArtifact", + "digest": "sha256:c16cb5dd74895931b8e41e16629a0519e5601340563c976b7f800c6de8d158b2", + "rationale": "#2946 threads an optional abort signal into the PUT so a lost lease can stop the stream. Method, ticket url and headers, and body bytes are unchanged for an uncancelled upload; an abort truncates the request, which a protocol-2 daemon sees as the client disconnect it already handles." + }, + { + "declaration": "src/remote/upload-client.ts#tryDirectUploadWithResume", + "digest": "sha256:67596a2ff2b8a5402566c3b7b99a6fa1baaf9d5de3d647bf35398185cc45eea6", + "rationale": "#2946 stops a canceled upload from re-preflighting and lifts the retry leg into its own function. The request sequence a daemon sees — preflight, direct PUT, finalize, or the legacy fallback after a retryable failure — is unchanged." + }, + { + "declaration": "src/remote/upload-client.ts#finalizeDirectUpload", + "digest": "sha256:04e18bdf91b9930a04d43f1661fe3f31ac2e6018154f8a8cb14656b9bc2b6140", + "rationale": "#2946 adds the caller's abort signal to finalize's existing timeout. Route and body keys are unchanged, so a protocol-2 daemon parses an uncancelled finalize exactly as before." + }, + { + "declaration": "src/remote/upload-client.ts#uploadLegacyArtifact", + "digest": "sha256:a7a82d6eb2925d6f510b9cc00f01f9ce865638368e0bec503b700f0bc59871bc", + "rationale": "#2946 threads an optional abort signal into the legacy upload stream. Method, route, artifact headers, and chunked body are unchanged for an uncancelled upload; an abort truncates the request, which a protocol-2 daemon sees as the client disconnect it already handles." + }, + { + "declaration": "src/remote/upload-stream.ts#streamFileToHttpRequest", + "digest": "sha256:ce07ea33a275e06e4cceaf0c75a079bd0c7938f36df6817407dbb1cc8c4ff900", + "rationale": "#2946 widens the options with an optional abort signal handed to node:http.request. The request line, headers built by buildUploadRequestHeaders, and piped payload are unchanged, so an older daemon's 200/308 handling is unaffected." + }, + { + "declaration": "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt", + "digest": "sha256:bc636083b00c2eef73a65a3446491e831510cdf934ff2545dd12e6b6ba13160d", + "rationale": "#2946 passes the optional abort signal to the request. The released 308 resume contract is untouched: offset headers, resume start offsets, redirects, and payload bytes are computed exactly as before." } ] } diff --git a/test/wire-compat/module-resolution.ts b/test/wire-compat/module-resolution.ts index f281315690..605ebaba0f 100644 --- a/test/wire-compat/module-resolution.ts +++ b/test/wire-compat/module-resolution.ts @@ -35,6 +35,7 @@ export type ResolvedOrigin = * `DaemonRequest`), so treating the constructor as a leaf loses nothing. */ const TS_GLOBALS = new Set([ + 'AbortSignal', 'Array', 'Awaited', 'Date', From ad1355e9c407812bae5c2898b79be7795b1fc6eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:27:02 +0200 Subject: [PATCH 06/21] fix(remote): treat a beat the daemon rejects on its own terms as terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The beat's scope and window are fixed where it is built, so a daemon that refuses them — a ttl outside [minLeaseTtlMs, maxLeaseTtlMs], an unusable lease id — refuses every successor identically. Such a refusal carries no reason to key on, so the code is the signal: end the phase the way a lost lease does instead of spending the upload against a lease that stopped renewing on the first beat. --- docs/adr/0007-remote-device-leases.md | 9 ++++--- .../daemon-client-upload-lease.test.ts | 25 +++++++++++++++++++ src/daemon-client/daemon-client.ts | 23 +++++++++++------ 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index 604d63c50b..a51c178731 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -91,10 +91,11 @@ successor instead of replacing the schedule or suppressing every beat behind it. A beat is a fresh request each time, never the protected request rewritten: a request identity is what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat -that finds the lease gone, or finds that this request can never renew it because its scope is missing -or belongs to another lease, ends the phase with that error and cancels the upload rather than -finishing bytes against a device nobody owns or a lease that will stop renewing. A beat that fails for -any other reason is reported and survived, because a later beat covers one lost request. +that finds the lease gone, or finds that this request can never renew it — its scope is missing or +belongs to another lease, or the daemon rejects the scope and window the beat itself was built with, +which no successor will ask differently — ends the phase with that error and cancels the upload rather +than finishing bytes against a device nobody owns or a lease that will stop renewing. A beat that fails +for any other reason is reported and survived, because a later beat covers one lost request. ## Human control diff --git a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts index 824110c5fa..0ffeb1ae3c 100644 --- a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts +++ b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts @@ -279,6 +279,31 @@ describe('runProtectedLeaseWork', () => { }); } + test('a beat refused INVALID_ARGS ends the phase, because the beat asks the same thing forever', async () => { + vi.useFakeTimers(); + // The beat's scope and ttl are fixed when it is built, so a daemon that rejects them — a ttl + // outside [minLeaseTtlMs, maxLeaseTtlMs] — rejects every successor identically. It carries no + // reason to key on, so the code is the signal; waiting it out only spends the upload. + const heartbeat = vi.fn(async () => { + throw new AppError('INVALID_ARGS', 'Lease ttlMs must be between 5000 and 3600000.'); + }); + const upload = deferred(); + const running = runProtectedLeaseWork({ + intervalMs: 10, + task: () => upload.promise, + heartbeat, + }); + + const rejected = assert.rejects( + running, + (error: unknown) => error instanceof AppError && error.code === 'INVALID_ARGS', + ); + await vi.advanceTimersByTimeAsync(50); + assert.equal(heartbeat.mock.calls.length, 1, 'a refusal the beat cannot fix is not retried'); + await rejected; + upload.resolve('too late'); + }); + test('a beat that ends the protection cancels the upload the phase is running', async () => { vi.useFakeTimers(); const heartbeat = vi.fn(async () => { diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index 437d3d0100..185fd2c08b 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -315,6 +315,21 @@ const UNRENEWABLE_LEASE_BEAT_REASONS: ReadonlySet = new Set([ 'LEASE_SCOPE_MISMATCH', ]); +/** + * Whether a beat failed for a reason every successor will repeat: the lease is gone, or the daemon + * refused a fact baked into the beat itself. A beat's scope and ttl never change across the phase, + * so an `INVALID_ARGS` refusal — an out-of-range ttl, an unusable lease id — is terminal without + * waiting out the window it can no longer renew. + */ +function isTerminalLeaseBeatError(error: unknown): boolean { + return ( + error instanceof AppError && + (LOST_LEASE_BEAT_REASONS.has(error.details?.reason) || + UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason) || + error.code === 'INVALID_ARGS') + ); +} + /** * Runs one client-side phase under a lease it does not own the clock of. * @@ -425,14 +440,6 @@ function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined return expiresAt > heartbeatAt ? expiresAt - heartbeatAt : undefined; } -function isTerminalLeaseBeatError(error: unknown): boolean { - return ( - error instanceof AppError && - (LOST_LEASE_BEAT_REASONS.has(error.details?.reason) || - UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason)) - ); -} - type Outcome = { ok: true; value: T } | { ok: false; error: unknown }; async function captureOutcome(promise: Promise): Promise> { From 0f12bf26b6449aee4f95985e5b415cbd05866da3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:39:00 +0200 Subject: [PATCH 07/21] test(remote): drop the connection-close wait that hung the cancellation test The byte counts are what prove the stream stopped early; waiting for the server to see the socket close added a second promise that only loopback timing could settle, and under a loaded coverage lane it never did inside the test timeout. A smaller payload keeps the same pause-and-abort scenario off the CPU. --- src/__tests__/upload-client-cancellation.test.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/__tests__/upload-client-cancellation.test.ts b/src/__tests__/upload-client-cancellation.test.ts index ec1240da6f..2dd620252e 100644 --- a/src/__tests__/upload-client-cancellation.test.ts +++ b/src/__tests__/upload-client-cancellation.test.ts @@ -27,14 +27,12 @@ afterEach(async () => { }); test('an aborted signal ends a legacy upload mid-stream instead of finishing the bytes', async () => { - const content = Buffer.alloc(8 * 1024 * 1024, 'x'); + // Two megabytes is well past what a paused read lets through: the assertions below are about the + // stream stopping, and a smaller payload keeps that off the CPU in a loaded lane. + const content = Buffer.alloc(2 * 1024 * 1024, 'x'); const artifactPath = createTempFile('app.apk', content); const control = new AbortController(); let sawBytes = 0; - let connectionEnded: () => void; - const ended = new Promise((resolve) => { - connectionEnded = resolve; - }); // Preflight reports itself unsupported so the upload takes the legacy stream, the one path that // pipes a file at the daemon and keeps going for as long as the daemon drains it. @@ -54,7 +52,6 @@ test('an aborted signal ends a legacy upload mid-stream instead of finishing the req.pause(); if (!control.signal.aborted) control.abort(); }); - req.on('close', connectionEnded); return; } res.statusCode = 404; @@ -73,7 +70,6 @@ test('an aborted signal ends a legacy upload mid-stream instead of finishing the ); assert.ok(sawBytes > 0, 'the upload had started streaming before the abort'); assert.ok(sawBytes < content.length, `the stream stopped early, at ${sawBytes} bytes`); - await ended; } finally { await server.close(); } From 4e4a38925ba0f14b2dc742d2a9bf26c52973a004 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:58:20 +0200 Subject: [PATCH 08/21] fix(remote): reject an aborted upload with the reason it was aborted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract on the upload signal is that an aborted upload rejects with the signal's own reason. The preflight leg honoured it and the stream leg did not: node:http reports an aborted request as a transport error, and the shared handler wrapped that into COMMAND_FAILED. That is not just the wrong message — a wrapped cancellation is indistinguishable from a broken transport, so the direct-upload retry policy is one refactor away from re-preflighting for a fresh ticket after the caller asked for none. Finalize wrapped the same way. Both tests now pin the rejection instead of accepting anything, and the admission test pins heartbeatAt so the window assertion cannot pass on a lease that was never renewed. --- src/__tests__/upload-client-cancellation.test.ts | 9 +++++++++ src/daemon/__tests__/request-admission.test.ts | 3 +++ src/remote/upload-client.ts | 3 +++ src/remote/upload-stream.ts | 6 ++++++ 4 files changed, 21 insertions(+) diff --git a/src/__tests__/upload-client-cancellation.test.ts b/src/__tests__/upload-client-cancellation.test.ts index 2dd620252e..0313feff53 100644 --- a/src/__tests__/upload-client-cancellation.test.ts +++ b/src/__tests__/upload-client-cancellation.test.ts @@ -67,6 +67,7 @@ test('an aborted signal ends a legacy upload mid-stream instead of finishing the token: TEST_TOKEN, signal: control.signal, }), + isAbortError, ); assert.ok(sawBytes > 0, 'the upload had started streaming before the abort'); assert.ok(sawBytes < content.length, `the stream stopped early, at ${sawBytes} bytes`); @@ -96,6 +97,7 @@ test('an aborted signal before preflight refuses to ask the daemon for a ticket' token: TEST_TOKEN, signal: control.signal, }), + isAbortError, ); assert.deepEqual(requests, [], 'an upload nobody waits for never reaches the daemon'); } finally { @@ -137,6 +139,13 @@ test('an upload with no signal behaves exactly as before', async () => { } }); +function isAbortError(error: unknown): boolean { + // The contract on `signal` is that an aborted upload rejects with the signal's own reason. Any + // other rejection — a transport failure, the server-error fallback — means the upload stopped for + // a reason this test is not about and would let the abort path regress while staying green. + return error instanceof DOMException && error.name === 'AbortError'; +} + function createTempFile(filename: string, content: string | Buffer): string { const dir = mkdtempForTestSync('agent-device-upload-cancel-'); tempDirs.push(dir); diff --git a/src/daemon/__tests__/request-admission.test.ts b/src/daemon/__tests__/request-admission.test.ts index c8719b9d8c..c3c1a58e2e 100644 --- a/src/daemon/__tests__/request-admission.test.ts +++ b/src/daemon/__tests__/request-admission.test.ts @@ -199,6 +199,9 @@ test('admitting a command does not shorten a proxy lease allocated above the old ); assert.equal(result?.leaseId, lease.leaseId); + // Without this, a lease handed back untouched would pass: allocation already leaves a 600_000 + // difference between these two stamps, so the delta alone cannot prove admission renewed anything. + assert.equal(result!.heartbeatAt, 2_000, 'admission renewed at admission time'); assert.equal( result!.expiresAt - result!.heartbeatAt, 600_000, diff --git a/src/remote/upload-client.ts b/src/remote/upload-client.ts index 3f0fe10c63..c0a1be29b1 100644 --- a/src/remote/upload-client.ts +++ b/src/remote/upload-client.ts @@ -365,6 +365,9 @@ async function finalizeDirectUpload(options: { signal: combineUploadSignals(options.signal, UPLOAD_PREFLIGHT_TIMEOUT_MS), body: JSON.stringify({ uploadId: options.uploadId }), }).catch((error) => { + // As with preflight: a canceled finalize is not a transport failure to report, it is the caller + // saying this upload is no longer worth finishing. + if (options.signal?.aborted) throw error; throw new AppError('COMMAND_FAILED', 'Failed to finalize direct artifact upload', {}, error); }); diff --git a/src/remote/upload-stream.ts b/src/remote/upload-stream.ts index db640bddd7..fedb2c332d 100644 --- a/src/remote/upload-stream.ts +++ b/src/remote/upload-stream.ts @@ -168,6 +168,12 @@ async function streamFileToHttpRequestAttempt(options: { req.on('error', (err) => { if (responseReceived) return; clearTimeout(timeout); + // A caller that canceled owns this outcome: the signal's reason is the answer, and wrapping it + // as an ordinary transport failure would let a retry path treat "we stopped" as "it broke". + if (options.signal?.aborted) { + reject(options.signal.reason); + return; + } reject( new AppError( 'COMMAND_FAILED', From e2a6dcf347676cc1c45a61b9d0118337386f0c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 10:58:20 +0200 Subject: [PATCH 09/21] chore(gates): ack the two upload digests the abort-reason fix moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finalizeDirectUpload and streamFileToHttpRequestAttempt changed shape; both edits are client-side only — the bytes a protocol-2 daemon parses for an uncancelled request are unchanged, and a canceled request is the disconnect it already handles. --- test/wire-compat/ledger.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/wire-compat/ledger.json b/test/wire-compat/ledger.json index e107c65d42..b61d956aaf 100644 --- a/test/wire-compat/ledger.json +++ b/test/wire-compat/ledger.json @@ -161,7 +161,7 @@ "src/remote/upload-client.ts#UploadPreflightResponse": "sha256:196a1d9ef41a1aec192589fa7ccb30cf71d971a005349d5d73eed42e6ab21327", "src/remote/upload-client.ts#UploadPreflightResult": "sha256:bd8fc6d617c0630325455fd64d0fe319daaced198ee6745e4673a8ace3df460d", "src/remote/upload-client.ts#UploadResponse": "sha256:499e01466b5e0e03413f8d3f1986f5af6bc120704c50a3747d84de422b3f0314", - "src/remote/upload-client.ts#finalizeDirectUpload": "sha256:04e18bdf91b9930a04d43f1661fe3f31ac2e6018154f8a8cb14656b9bc2b6140", + "src/remote/upload-client.ts#finalizeDirectUpload": "sha256:5d283353df5f089a6ab26b7be96c44997e6b9905b329cb993d300c615479d715", "src/remote/upload-client.ts#isStringRecord": "sha256:b6d6b571b90baf0976d9c347c4a45a4363a4df5ad184ea1087cb3b9febd4b624", "src/remote/upload-client.ts#parseUploadPreflightResult": "sha256:7ce3f43b45c03df8b7f2690432beedc7a537c1bba540961716862f8a632d5623", "src/remote/upload-client.ts#requestUploadPreflight": "sha256:4c9863bce8757f1dc0f01ec7374019e752efc8e786cd070200f516a1d72d09e1", @@ -178,7 +178,7 @@ "src/remote/upload-stream.ts#parseNonNegativeIntegerHeader": "sha256:5cbe9c6994459d673d988a51407e65556e28175b187ce0a9d27cbb6331962834", "src/remote/upload-stream.ts#parseUploadResumeOffset": "sha256:297184ea22a6dc1cda9aee17034122ae0ea2f43d0355647f6fcf02f3f4b5ca0e", "src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:ce07ea33a275e06e4cceaf0c75a079bd0c7938f36df6817407dbb1cc8c4ff900", - "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:bc636083b00c2eef73a65a3446491e831510cdf934ff2545dd12e6b6ba13160d" + "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8" }, "compatibleChanges": [ { @@ -318,8 +318,8 @@ }, { "declaration": "src/remote/upload-client.ts#finalizeDirectUpload", - "digest": "sha256:04e18bdf91b9930a04d43f1661fe3f31ac2e6018154f8a8cb14656b9bc2b6140", - "rationale": "#2946 adds the caller's abort signal to finalize's existing timeout. Route and body keys are unchanged, so a protocol-2 daemon parses an uncancelled finalize exactly as before." + "digest": "sha256:5d283353df5f089a6ab26b7be96c44997e6b9905b329cb993d300c615479d715", + "rationale": "#2946 adds the caller's abort signal to finalize's existing timeout and lets a canceled one reject with the signal's own reason instead of a wrapped transport error. Route and body keys are unchanged, so a protocol-2 daemon parses an uncancelled finalize exactly as before." }, { "declaration": "src/remote/upload-client.ts#uploadLegacyArtifact", @@ -333,8 +333,8 @@ }, { "declaration": "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt", - "digest": "sha256:bc636083b00c2eef73a65a3446491e831510cdf934ff2545dd12e6b6ba13160d", - "rationale": "#2946 passes the optional abort signal to the request. The released 308 resume contract is untouched: offset headers, resume start offsets, redirects, and payload bytes are computed exactly as before." + "digest": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8", + "rationale": "#2946 passes the optional abort signal to the request and rejects with that signal's reason when it is what ended the stream. The released 308 resume contract is untouched: offset headers, resume start offsets, redirects, and payload bytes are computed exactly as before." } ] } From c4dc0056e4811d5c6ecea8516357388dc307c25c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 16:03:25 +0200 Subject: [PATCH 10/21] fix(remote): give each lease beat a budget and prove the beat on the real route A beat was sent under the lease_heartbeat command policy's 90s timeout and its successor was armed only after it settled, so one stalled round trip stopped the renewals for up to 90s while the lease's 60s window lapsed underneath the upload and the install RPC waited behind it. Each beat now carries a budget no longer than the cadence it started on and arms its successor at beat start, so a beat that never settles is abandoned on schedule; nothing awaits an outstanding beat when the phase settles. The beat's cadence is gone with it: the loop beats at the registry's floor until a beat names the window, and an answer that names no window keeps the cadence rather than slowing down on the absence of evidence. The lease-lost reasons move to contracts as one taxonomy shared with the connection runtime, and LEASE_SESSION_MISMATCH leaves the terminal set: only request admission raises it and lease_heartbeat is admission-exempt. Two suites back it. The beat loop is pinned against fake timers for the stalled beat, its budget, and an abandoned beat that answers too late to matter. remote-upload-lease-beat.test.ts drives an install through sendToDaemon against a fake remote daemon, so the wiring between the loop, the beat it is handed, and the upload is covered, not just each of them alone: the first holds the upload until a second beat names the lease and asserts a renewed artifact lands on a live lease before the install RPC; the second answers a beat with LEASE_NOT_FOUND and asserts the artifact is destroyed mid-flight, is not left to drain once the daemon stops applying backpressure, and no install RPC goes out. Killing either half of that wiring fails them: dropping the heartbeat shows no beat, and dropping the signal hands the daemon a drained artifact. --- .../src/__tests__/lease-scope.test.ts | 27 ++ packages/contracts/src/lease-scope.ts | 26 ++ src/cli/commands/connection-runtime.ts | 10 +- .../daemon-client-upload-lease.test.ts | 218 +++++++---- src/daemon-client/daemon-client.ts | 150 +++++--- .../remote-upload-lease-beat.test.ts | 353 ++++++++++++++++++ 6 files changed, 637 insertions(+), 147 deletions(-) create mode 100644 test/integration/provider-scenarios/remote-upload-lease-beat.test.ts diff --git a/packages/contracts/src/__tests__/lease-scope.test.ts b/packages/contracts/src/__tests__/lease-scope.test.ts index daad373eb2..3da34f2be7 100644 --- a/packages/contracts/src/__tests__/lease-scope.test.ts +++ b/packages/contracts/src/__tests__/lease-scope.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; import { findMissingProxyLeaseFields, + isInactiveLeaseError, leaseScopeFromOptions, leaseScopeFromRequest, leaseScopeToCommandFlags, @@ -215,3 +216,29 @@ test('readLeaseAllocateProviderFlags carries the provider-allocation flags and d ); assert.deepEqual(readLeaseAllocateProviderFlags(undefined), {}); }); + +test('isInactiveLeaseError recognizes the lease being gone, and refuses a request mismatch', () => { + for (const reason of ['LEASE_NOT_FOUND', 'LEASE_EXPIRED', 'LEASE_REVOKED']) { + assert.equal( + isInactiveLeaseError({ code: 'UNAUTHORIZED', details: { reason } }), + true, + `${reason} says the lease is no longer usable`, + ); + } + // A mismatch is about the request that asked, not the lease: the lease may be perfectly alive and + // held by this same client under another request, so a beat must keep renewing on this answer. + assert.equal( + isInactiveLeaseError({ code: 'UNAUTHORIZED', details: { reason: 'LEASE_SCOPE_MISMATCH' } }), + false, + 'a scope mismatch is not the lease being gone', + ); + // Both halves are required: the code alone is any unauthorized call, and the reason alone could + // ride on an error the client has no business acting on. + assert.equal(isInactiveLeaseError({ code: 'UNAUTHORIZED', details: {} }), false); + assert.equal( + isInactiveLeaseError({ code: 'COMMAND_FAILED', details: { reason: 'LEASE_NOT_FOUND' } }), + false, + ); + assert.equal(isInactiveLeaseError(new Error('LEASE_NOT_FOUND')), false); + assert.equal(isInactiveLeaseError(undefined), false); +}); diff --git a/packages/contracts/src/lease-scope.ts b/packages/contracts/src/lease-scope.ts index 1f18c16d26..d03b7cf6d7 100644 --- a/packages/contracts/src/lease-scope.ts +++ b/packages/contracts/src/lease-scope.ts @@ -295,6 +295,32 @@ export function findMissingProxyLeaseFields(scope: LeaseScope): string[] { return REQUIRED_PROXY_LEASE_FIELDS.filter((field) => !scope[field]); } +/** + * Why a lease stopped being ours: it is gone, spent, or taken back. + * + * This is the whole taxonomy of "the lease is no longer usable" as the daemon reports it, and both + * readers ask the same question — a client deciding whether a connection still owns a device, and a + * lease beat deciding whether an upload is still worth finishing. A reason naming a mismatch between + * a request and a lease is not in here: that says something about the request, not the lease. + */ +const INACTIVE_LEASE_REASONS: ReadonlySet = new Set([ + 'LEASE_NOT_FOUND', + 'LEASE_EXPIRED', + 'LEASE_REVOKED', +]); + +/** Whether `error` says the lease is gone rather than merely unavailable to this request. */ +export function isInactiveLeaseError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + (error as Readonly<{ code?: unknown }>).code === 'UNAUTHORIZED' && + INACTIVE_LEASE_REASONS.has( + (error as Readonly<{ details?: Readonly<{ reason?: unknown }> }>).details?.reason, + ) + ); +} + function leaseScopeToScopedRequest(scope: LeaseScope): LeaseScopedRequestScope { return stripUndefined({ leaseId: scope.leaseId ?? '', diff --git a/src/cli/commands/connection-runtime.ts b/src/cli/commands/connection-runtime.ts index 961560e226..23579c540d 100644 --- a/src/cli/commands/connection-runtime.ts +++ b/src/cli/commands/connection-runtime.ts @@ -15,6 +15,7 @@ import { type DeviceInfo, } from '@agent-device/kernel/device'; import { shouldAgentCdpUseRemoteBridgeUrl } from './agent-cdp.ts'; +import { isInactiveLeaseError } from '@agent-device/contracts/lease-scope'; import { buildRemoteConnectionDaemonState, buildRemoteConnectionRequestMetadata, @@ -991,12 +992,3 @@ async function heartbeatOrAllocateLease( throw error; } } - -function isInactiveLeaseError(error: unknown): boolean { - if (!(error instanceof AppError) || error.code !== 'UNAUTHORIZED') return false; - return ( - error.details?.reason === 'LEASE_NOT_FOUND' || - error.details?.reason === 'LEASE_EXPIRED' || - error.details?.reason === 'LEASE_REVOKED' - ); -} diff --git a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts index 0ffeb1ae3c..696974b797 100644 --- a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts +++ b/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts @@ -54,7 +54,6 @@ describe('runProtectedLeaseWork', () => { vi.useFakeTimers(); const heartbeat = vi.fn(async () => renewedLeaseResponse(30_000)); const running = runProtectedLeaseWork({ - intervalMs: 10, task: async (signal) => { assert.ok(!signal.aborted, 'a lease still held does not cancel the upload'); return 'installed'; @@ -70,7 +69,6 @@ describe('runProtectedLeaseWork', () => { const heartbeat = vi.fn(async () => renewedLeaseResponse(30_000)); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 20_000, task: () => upload.promise, heartbeat, }); @@ -78,26 +76,25 @@ describe('runProtectedLeaseWork', () => { // A lease admitted with a short window used to be able to lapse before the first beat: the // upload paid for the device with a lease nobody renewed yet. await vi.advanceTimersByTimeAsync(0); - assert.equal(heartbeat.mock.calls.length, 1, 'a beat is out before the interval elapses'); + assert.equal(heartbeat.mock.calls.length, 1, 'a beat is out before any interval elapses'); upload.resolve('installed'); assert.equal(await running, 'installed'); }); - test('beats follow the window the lease reports, not the fallback interval', async () => { + test('beats follow the window the lease reports, not the assumed floor', async () => { vi.useFakeTimers(); - // The daemon says it just extended the lease by 15s; the next beat must land at a third of - // that, whatever the caller's fallback cadence was. + // The daemon says it just extended the lease by 15s; the next beat must land a third of that + // after the answer, whatever the loop assumed beforehand. const heartbeat = vi.fn(async () => renewedLeaseResponse(15_000)); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 60_000, task: () => upload.promise, heartbeat, }); await vi.advanceTimersByTimeAsync(5_000); - assert.equal(heartbeat.mock.calls.length, 2, 'first beat at once, second one third of 15s in'); + assert.equal(heartbeat.mock.calls.length, 2, 'first beat at once, second a third of 15s in'); await vi.advanceTimersByTimeAsync(5_000); assert.equal(heartbeat.mock.calls.length, 3); @@ -113,7 +110,6 @@ describe('runProtectedLeaseWork', () => { const heartbeat = vi.fn(async () => renewedLeaseResponse(1_500)); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 60_000, task: () => upload.promise, heartbeat, }); @@ -127,63 +123,120 @@ describe('runProtectedLeaseWork', () => { assert.equal(await running, 'installed'); }); - test('a response that names no window keeps the fallback cadence', async () => { + test('an answer that names no window keeps the cadence the beat was asked at', async () => { vi.useFakeTimers(); + // Slowing down here would have to rest on evidence about the lease, and an unreadable answer is + // the absence of evidence: an older daemon renews happily without describing the window. const heartbeat = vi.fn(async () => ({ ok: true, data: {} })); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); - await vi.advanceTimersByTimeAsync(10); - assert.equal(heartbeat.mock.calls.length, 2, 'immediate first, then the fallback interval'); - await vi.advanceTimersByTimeAsync(10); + await vi.advanceTimersByTimeAsync(1_000); + assert.equal(heartbeat.mock.calls.length, 2, 'immediate first, then the same cadence again'); + await vi.advanceTimersByTimeAsync(1_000); assert.equal(heartbeat.mock.calls.length, 3); upload.resolve('installed'); assert.equal(await running, 'installed'); }); - test('never overlaps beats, and a slow beat delays its successor instead of replacing the schedule', async () => { + test('each beat is given a budget no longer than the cadence it starts on', async () => { vi.useFakeTimers(); - let inFlight = 0; - let maxConcurrent = 0; - const gates: ReturnType>[] = []; - const upload = deferred(); + const budgets: number[] = []; + const heartbeat = vi.fn(async (budgetMs: number) => { + budgets.push(budgetMs); + return renewedLeaseResponse(60_000); + }); + const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, - heartbeat: async () => { - const gate = deferred(); - gates.push(gate); - inFlight += 1; - maxConcurrent = Math.max(maxConcurrent, inFlight); - await gate.promise; - inFlight -= 1; - }, + heartbeat, }); + await vi.advanceTimersByTimeAsync(40_000); + // Before the first answer the loop assumes the shortest window the daemon accepts, so the beat + // that has to prove that lease is alive cannot itself take longer than a fifth of it. Once the + // window is known the budget is the cadence: a stalled beat dies inside one, not the 90s the + // command's own heartbeat policy would allow. + assert.deepEqual(budgets, [1_000, 20_000, 20_000]); + + upload.resolve('installed'); + assert.equal(await running, 'installed'); + }); + + test('a beat that never settles is abandoned on schedule and holds no successor off', async () => { + vi.useFakeTimers(); + // The #2946 failure this loop exists to prevent, in its remaining shape: a beat stuck on a + // half-open connection. Overlapping is the point — a stalled beat must not take the schedule + // with it, or the lease dies at 60s while the beat waits out its transport timeout. + const started: number[] = []; + const stalled = deferred(); + const heartbeat = vi.fn(async () => { + started.push(Date.now()); + await stalled.promise; + return renewedLeaseResponse(5_000); + }); + const upload = deferred(); + const running = runProtectedLeaseWork({ + task: () => upload.promise, + heartbeat, + }); + + // A lease on the registry's five-second minimum window, with every beat stalled. + await vi.advanceTimersByTimeAsync(4_000); + assert.ok( + heartbeat.mock.calls.length >= 3, + `beats keep coming through a stall, got ${heartbeat.mock.calls.length}`, + ); + + upload.resolve('installed'); + let outcome: string | undefined; + void running.then((value) => { + outcome = value; + }); await vi.advanceTimersByTimeAsync(0); - assert.equal(gates.length, 1); + assert.equal(outcome, 'installed', 'a finished upload does not wait on a stalled beat'); + }); + + test('a late lost-lease answer from an abandoned beat is logged and does not rewrite the outcome', async () => { + vi.useFakeTimers(); + let reportLost: ((error: unknown) => void) | undefined; + const heartbeat = vi.fn( + () => + new Promise((_resolve, reject) => { + reportLost = reject; + }), + ); + const upload = deferred(); + const running = runProtectedLeaseWork({ + task: () => upload.promise, + heartbeat, + }); - // The fallback intervals pass with the first beat still outstanding: none of them may start a - // second one, or a stalled transport would pile beats up behind it. - await vi.advanceTimersByTimeAsync(30); - assert.equal(gates.length, 1, 'an outstanding beat holds the next one off'); + await vi.advanceTimersByTimeAsync(3_000); + assert.equal(heartbeat.mock.calls.length, 4, 'the stalled beats were abandoned, not awaited'); - // The successor is armed when the slow beat lands — the cadence comes from completions, so a - // beat slower than the interval shifts the schedule instead of silently killing every later one. - gates[0]!.resolve(); - await vi.advanceTimersByTimeAsync(10); - assert.equal(gates.length, 2, 'the next beat starts once the previous one lands'); + upload.resolve('installed'); + let outcome: string | undefined; + void running.then( + () => { + outcome = 'resolved'; + }, + (error: unknown) => { + outcome = error instanceof AppError ? String(error.details?.reason) : 'rejected'; + }, + ); + await vi.advanceTimersByTimeAsync(0); + assert.equal(outcome, 'resolved', 'the upload had already finished when the beat was dropped'); - gates[1]!.resolve(); - upload.resolve(); + // The abandoned beat is still listened to: its late answer is a fact about the lease the caller + // should be able to find in the log even though its own outcome already stands. + reportLost?.(lostLeaseError('LEASE_EXPIRED')); await vi.advanceTimersByTimeAsync(0); - await running; - assert.equal(maxConcurrent, 1); + assert.equal(outcome, 'resolved'); }); test('a beat that fails for a transient reason is survived and re-armed', async () => { @@ -197,26 +250,20 @@ describe('runProtectedLeaseWork', () => { }); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); - await vi.advanceTimersByTimeAsync(25); + await vi.advanceTimersByTimeAsync(2_500); assert.equal(heartbeat.mock.calls.length, 3, 'one failed beat does not stop the others'); - heartbeat.mockImplementation(async () => ({ ok: true })); - await vi.advanceTimersByTimeAsync(10); + heartbeat.mockImplementation(async () => renewedLeaseResponse(30_000)); + await vi.advanceTimersByTimeAsync(1_000); upload.resolve('installed'); assert.equal(await running, 'installed'); }); - for (const reason of [ - 'LEASE_NOT_FOUND', - 'LEASE_EXPIRED', - 'LEASE_REVOKED', - 'LEASE_SESSION_MISMATCH', - ]) { + for (const reason of ['LEASE_NOT_FOUND', 'LEASE_EXPIRED', 'LEASE_REVOKED']) { test(`a beat that finds the lease ${reason} ends the phase with that error`, async () => { vi.useFakeTimers(); const heartbeat = vi.fn(async () => { @@ -224,7 +271,6 @@ describe('runProtectedLeaseWork', () => { }); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); @@ -256,7 +302,6 @@ describe('runProtectedLeaseWork', () => { }); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); @@ -268,7 +313,7 @@ describe('runProtectedLeaseWork', () => { error.code === 'UNAUTHORIZED' && error.details?.reason === reason, ); - await vi.advanceTimersByTimeAsync(50); + await vi.advanceTimersByTimeAsync(5_000); assert.equal( heartbeat.mock.calls.length, 1, @@ -289,7 +334,6 @@ describe('runProtectedLeaseWork', () => { }); const upload = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); @@ -298,7 +342,7 @@ describe('runProtectedLeaseWork', () => { running, (error: unknown) => error instanceof AppError && error.code === 'INVALID_ARGS', ); - await vi.advanceTimersByTimeAsync(50); + await vi.advanceTimersByTimeAsync(5_000); assert.equal(heartbeat.mock.calls.length, 1, 'a refusal the beat cannot fix is not retried'); await rejected; upload.resolve('too late'); @@ -311,7 +355,6 @@ describe('runProtectedLeaseWork', () => { }); let sawAbort = false; const running = runProtectedLeaseWork({ - intervalMs: 10, task: (signal) => new Promise((_resolve, reject) => { signal.addEventListener('abort', () => { @@ -334,7 +377,6 @@ describe('runProtectedLeaseWork', () => { await assert.rejects( (async () => await runProtectedLeaseWork({ - intervalMs: 10, task: () => { throw new AppError('INVALID_ARGS', 'artifact vanished'); }, @@ -343,7 +385,7 @@ describe('runProtectedLeaseWork', () => { /artifact vanished/, ); - await vi.advanceTimersByTimeAsync(100); + await vi.advanceTimersByTimeAsync(10_000); assert.equal(heartbeat.mock.calls.length, 0, 'no timer outlived a phase that never started'); }); @@ -353,7 +395,6 @@ describe('runProtectedLeaseWork', () => { let beat: (() => void) | undefined; const first = deferred(); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => first.promise, heartbeat: () => new Promise((_, reject) => { @@ -377,7 +418,6 @@ describe('runProtectedLeaseWork', () => { vi.useFakeTimers(); const heartbeat = vi.fn(async () => ({ ok: true })); const running = runProtectedLeaseWork({ - intervalMs: 10, task: async () => { throw new AppError('COMMAND_FAILED', 'upload failed'); }, @@ -386,11 +426,11 @@ describe('runProtectedLeaseWork', () => { await assert.rejects((async () => await running)(), /upload failed/); const before = heartbeat.mock.calls.length; - await vi.advanceTimersByTimeAsync(100); + await vi.advanceTimersByTimeAsync(10_000); assert.equal(heartbeat.mock.calls.length, before); }); - test('an in-flight beat is awaited on the way out and arms nothing after it', async () => { + test('an abandoned beat arms no successor once the phase has settled', async () => { vi.useFakeTimers(); const beat = deferred(); const upload = deferred(); @@ -399,7 +439,6 @@ describe('runProtectedLeaseWork', () => { return renewedLeaseResponse(30_000); }); const running = runProtectedLeaseWork({ - intervalMs: 10, task: () => upload.promise, heartbeat, }); @@ -408,17 +447,10 @@ describe('runProtectedLeaseWork', () => { assert.equal(heartbeat.mock.calls.length, 1, 'a beat is in flight while the upload finishes'); upload.resolve('installed'); - await vi.advanceTimersByTimeAsync(0); - let settled = false; - void running.then(() => { - settled = true; - }); - await vi.advanceTimersByTimeAsync(0); - assert.equal(settled, false, 'the phase waits for the renewal it started'); + assert.equal(await running, 'installed', 'the phase does not wait on a beat it gave up on'); beat.resolve(); - assert.equal(await running, 'installed'); - await vi.advanceTimersByTimeAsync(60_000); + await vi.advanceTimersByTimeAsync(120_000); assert.equal(heartbeat.mock.calls.length, 1, 'the renewal that landed last arms no successor'); }); }); @@ -520,7 +552,7 @@ describe('createLeaseRenewalBeat', () => { leaseBackend: 'android-instance', } as const; - function beatContext(send: (request: DaemonRequest) => Promise) { + function beatContext(send: (request: DaemonRequest, budgetMs: number) => Promise) { return { session: 'adc-android', sessionIsolation: 'tenant' as const, @@ -534,7 +566,7 @@ describe('createLeaseRenewalBeat', () => { await createLeaseRenewalBeat( scope, beatContext(async (request) => void sent.push(request)), - )(); + )(1_000); assert.equal(sent.length, 1); assert.equal(sent[0]!.command, 'lease_heartbeat'); @@ -552,9 +584,9 @@ describe('createLeaseRenewalBeat', () => { scope, beatContext(async (request) => void sent.push(request)), ); - await beat(); - await beat(); - await beat(); + await beat(1_000); + await beat(1_000); + await beat(1_000); const ids = sent.map((request) => request.meta?.requestId); assert.equal(new Set(ids).size, 3, 'three beats, three request ids'); @@ -568,7 +600,7 @@ describe('createLeaseRenewalBeat', () => { throw new AppError('COMMAND_FAILED', 'connection reset'); }), ); - await assert.rejects((async () => await beat())(), /connection reset/); + await assert.rejects((async () => await beat(1_000))(), /connection reset/); }); }); @@ -651,7 +683,7 @@ describe('buildUploadLeaseHeartbeat', () => { installRequest, ); assert.ok(beat); - await beat!(); + await beat!(5_000); } finally { await closeLoopbackServer(server); } @@ -673,4 +705,28 @@ describe('buildUploadLeaseHeartbeat', () => { // the lease already carries. assert.equal('ttlMs' in payload.params, false); }); + + test('a beat that never answers dies at its budget, not at the heartbeat policy', async () => { + vi.useFakeTimers(); + // The daemon accepts the connection and never answers — a half-open pipe. The beat exists to + // notice the lease inside its window, so the transport must cut it at the budget the loop + // handed it (here well under the command's 90s lease_heartbeat policy) and destroy the + // socket, instead of holding one half-open for the full policy. + const server = net.createServer(() => {}); + const port = await listenOnLoopback(server); + try { + const beat = buildUploadLeaseHeartbeat( + { baseUrl: `http://127.0.0.1:${String(port)}/agent-device`, token: 'remote-token', pid: 1 }, + { ...settings, transportPreference: 'auto' }, + installRequest, + ); + assert.ok(beat); + const rejected = assert.rejects((async () => await beat!(1_000))(), /timed out/i); + await vi.advanceTimersByTimeAsync(1_000); + await rejected; + } finally { + vi.useRealTimers(); + await closeLoopbackServer(server); + } + }); }); diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index 185fd2c08b..925c6b2082 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -16,6 +16,7 @@ import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/t import { prepareRemoteRequestArtifacts } from '../remote/daemon-artifacts.ts'; import { isRemoteDaemon } from './daemon-client-metadata.ts'; import { + isInactiveLeaseError, leaseScopeFromRequest, leaseScopeToRequestMeta, type LeaseScope, @@ -277,31 +278,17 @@ function withActiveSessionAddressHint( } /** - * The cadence a long client-side phase beats on before it has learned the lease's own window. + * The fastest cadence a phase beats at, and the budget each beat gets until the window is known. * - * The first beat answers with the window the daemon just renewed, and from then on the phase beats a - * third of that window. This constant only covers the gap until that answer lands, and the phase - * when the caller overrides it. - */ -const LEASE_HEARTBEAT_INTERVAL_MS = 20_000; - -/** - * The floor for a window-derived cadence. - * - * A lease admitted with the registry's minimum five-second window beats every 1.6s at one third of - * its window; the floor keeps a misreported or pathologically short window from turning the beat - * into a request loop against the daemon it is trying to stay admitted to. + * Before the first beat answers, the window is unknown and the worst legal case is the registry's + * five-second minimum: a beat that stalls must be abandoned and retried inside that window, or the + * thing the beat exists to prevent happens while it waits. One second is a fifth of that minimum, + * and no window-derived cadence is ever allowed below it, so it is also the loop's floor: a + * misreported or pathologically short window cannot turn the beat into a request loop faster than + * this. */ const MIN_LEASE_BEAT_INTERVAL_MS = 1_000; -/** Why a beat stopped protecting the lease: the lease is gone, so nothing else is worth waiting for. */ -const LOST_LEASE_BEAT_REASONS: ReadonlySet = new Set([ - 'LEASE_NOT_FOUND', - 'LEASE_EXPIRED', - 'LEASE_REVOKED', - 'LEASE_SESSION_MISMATCH', -]); - /** * Why a beat stopped protecting the lease even though the lease may live on: this client's request * will never be the one that renews it. @@ -316,17 +303,20 @@ const UNRENEWABLE_LEASE_BEAT_REASONS: ReadonlySet = new Set([ ]); /** - * Whether a beat failed for a reason every successor will repeat: the lease is gone, or the daemon - * refused a fact baked into the beat itself. A beat's scope and ttl never change across the phase, - * so an `INVALID_ARGS` refusal — an out-of-range ttl, an unusable lease id — is terminal without - * waiting out the window it can no longer renew. + * Whether a beat failed for a reason every successor will repeat: the lease is gone (the shared + * taxonomy), the daemon refused a fact baked into the beat itself, or this request can never renew + * it. A beat's scope and ttl never change across the phase, so an `INVALID_ARGS` refusal — an + * out-of-range ttl, an unusable lease id — is terminal without waiting out a window it can no + * longer renew. + * + * `LEASE_SESSION_MISMATCH` is deliberately absent: only request admission raises it, and + * `lease_heartbeat` is admission-exempt, so a beat can never receive it. */ function isTerminalLeaseBeatError(error: unknown): boolean { return ( - error instanceof AppError && - (LOST_LEASE_BEAT_REASONS.has(error.details?.reason) || - UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason) || - error.code === 'INVALID_ARGS') + isInactiveLeaseError(error) || + (error instanceof AppError && + (UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason) || error.code === 'INVALID_ARGS')) ); } @@ -342,9 +332,15 @@ function isTerminalLeaseBeatError(error: unknown): boolean { * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and * the phase runs untouched. The first beat is fired immediately rather than one interval in, so a * lease shorter than that interval is renewed before it can lapse — and a lease already gone is - * found before any bytes move. Each beat answers with the window it just renewed, and the next one - * is armed for a third of that window from the beat that landed: beats never overlap, and a slow - * one delays its successor instead of suppressing every beat after it. + * found while the upload is still hashing. Each beat answers with the window it just renewed, and + * the cadence becomes a third of that window. + * + * A beat's budget is the cadence it started on, and its successor is armed when the beat starts + * rather than when it settles. A beat that never returns — a half-open connection, a daemon wedged + * before it admits anything — is therefore abandoned on schedule instead of holding the schedule: + * the lease is still beaten at window/3, and the abandoned round trip is cut off by its own budget + * in the transport rather than by the command's 90-second heartbeat policy. An abandoned beat is + * still listened to, because the answer it eventually gives can be a lost lease. * * A beat that fails for a reason that says nothing about this lease is reported and survived — one * lost request must not fail an upload that a later beat will cover. A beat that finds the lease @@ -354,20 +350,23 @@ function isTerminalLeaseBeatError(error: unknown): boolean { */ export async function runProtectedLeaseWork( options: Readonly<{ - heartbeat: (() => Promise) | undefined; - intervalMs?: number; + /** + * One renewal. `budgetMs` is how long this beat may take before the loop abandons it. Absent + * when the request names no lease to renew, which is the ordinary unleased install. + */ + heartbeat?: ((budgetMs: number) => Promise) | undefined; task: (signal: AbortSignal) => Promise; }>, ): Promise { const { heartbeat } = options; if (!heartbeat) return await options.task(new AbortController().signal); - const fallbackIntervalMs = options.intervalMs ?? LEASE_HEARTBEAT_INTERVAL_MS; const control = new AbortController(); - let intervalMs = fallbackIntervalMs; + // Until a beat names the window, the loop assumes the shortest window the daemon will accept: the + // budget of the beat that has to prove a short lease is alive cannot itself be longer than it. + let intervalMs = MIN_LEASE_BEAT_INTERVAL_MS; let timer: ReturnType | undefined; let stopped = false; - let inFlight: Promise | undefined; let terminalError: unknown; let reportTerminal: ((error: unknown) => void) | undefined; const terminal = new Promise((_, reject) => { @@ -375,15 +374,39 @@ export async function runProtectedLeaseWork( }); const runBeat = (): void => { - inFlight = (async () => { + // Armed while this beat is still outstanding: a beat that never settles is abandoned on + // schedule rather than taking the schedule with it. The beat may re-arm it on the way out. + const arm = (delayMs: number): void => { + if (stopped) return; + if (timer) clearTimeout(timer); + timer = setTimeout(runBeat, delayMs); + }; + arm(intervalMs); + const settle = (async () => { + const budgetMs = intervalMs; try { - const renewed = leaseWindowFromHeartbeatResponse(await heartbeat()); - if (renewed !== undefined) { - intervalMs = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); - } + const renewed = leaseWindowFromHeartbeatResponse(await heartbeat(budgetMs)); + // An answer that names no window keeps the cadence it was asked at: the loop only ever + // slows down on evidence of how long the lease is good for, and never on the absence of it. + if (renewed === undefined) return; + const cadence = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); + if (cadence === intervalMs) return; + intervalMs = cadence; + // The window just moved, so the next beat is due one cadence from this answer. + arm(cadence); } catch (error) { if (isTerminalLeaseBeatError(error)) { terminalError = error; + if (stopped) { + // The phase settled first; the outcome it returned already stands, but a lease this + // client just learned is gone is worth one diagnostic on the way out. + emitDiagnostic({ + level: 'warn', + phase: 'lease_lost_after_phase', + data: { message: error instanceof Error ? error.message : String(error) }, + }); + return; + } // The upload is the only thing still consuming this phase's time, and it is pointed at a // device this client can no longer renew. Stop it rather than finish bytes nobody owns. control.abort(); @@ -396,10 +419,10 @@ export async function runProtectedLeaseWork( data: { message: error instanceof Error ? error.message : String(error) }, }); } - // A beat that lands after the phase settled must not arm a successor: nothing is left to - // protect, and a beat without a phase to stop it would renew the lease forever. - if (!stopped) timer = setTimeout(runBeat, intervalMs); })(); + // A beat the loop has moved on from is still listened to, and nothing awaits it: its outcome is + // swallowed here so a beat nobody is waiting on cannot surface as an unhandled rejection. + void settle.catch(() => undefined); }; // Armed before the phase starts, not one interval in: a beat is what proves the lease the upload @@ -415,7 +438,8 @@ export async function runProtectedLeaseWork( ); stopped = true; if (timer) clearTimeout(timer); - await inFlight; + // No outstanding beat is awaited here: a beat on a half-open connection would hold a finished + // upload behind its own budget for no decision the phase still has to make. // A beat that ended the protection outranks a phase that settled meanwhile, from either side: the // device is no longer ours, and the lease error is the reason the phase was not worth finishing. if (terminalError !== undefined) throw terminalError; @@ -453,8 +477,10 @@ async function captureOutcome(promise: Promise): Promise> { /** * The beat that keeps a remote lease alive across a long client-side phase. * - * `send` is the caller's transport. The beat goes over it directly rather than through the client's - * `leases.heartbeat`, because the client would come back through the upload path it protects. + * `send` is the caller's transport and `budgetMs` is how long this beat may take: the beat's answer + * only matters before the next one is due, so a stalled round trip is cut off at the cadence rather + * than at the command's own 90-second heartbeat policy. A beat is only ever sent to a remote daemon, + * where a transport timeout performs no local cleanup. */ export function createLeaseRenewalBeat( leaseScope: LeaseScope, @@ -462,10 +488,10 @@ export function createLeaseRenewalBeat( session: string; sessionIsolation?: NonNullable['sessionIsolation']; token: string; - send: (request: DaemonRequest) => Promise; + send: (request: DaemonRequest, budgetMs: number) => Promise; }>, -): () => Promise { - return async () => +): (budgetMs: number) => Promise { + return async (budgetMs) => await context.send( buildLeaseHeartbeatRequest(leaseScope, { session: context.session, @@ -473,6 +499,7 @@ export function createLeaseRenewalBeat( requestId: createRequestId(), token: context.token, }), + budgetMs, ); } @@ -521,18 +548,19 @@ export function leaseScopeForHeartbeat( * lease to protect: only a remote daemon uploads, so only one can be waiting on a billed device, and * a command that names no lease has nothing to renew. * - * Exported for its own coverage: `sendToDaemon` calls it once per upload, and the beat cadence is - * far too slow for an end-to-end test to reach it at real transport speed. + * Each beat's transport timeout is its budget, capped by the command's heartbeat policy: the answer + * is worthless once the next beat is due, so a stalled round trip is cut off and destroyed at the + * cadence instead of holding a socket for 90 seconds. `sendToDaemon` wires this once per upload. */ export function buildUploadLeaseHeartbeat( info: EnsuredDaemon['info'], settings: DaemonClientSettings, request: Omit, -): (() => Promise) | undefined { +): ((budgetMs: number) => Promise) | undefined { if (!isRemoteDaemon(info)) return undefined; const leaseScope = leaseScopeForHeartbeat(request); if (!leaseScope) return undefined; - const timeoutMs = resolveCommandRequestTimeoutMs( + const policyTimeoutMs = resolveCommandRequestTimeoutMs( resolveCommandTimeoutPolicy(INTERNAL_COMMANDS.leaseHeartbeat), { positionals: [] }, ); @@ -540,8 +568,16 @@ export function buildUploadLeaseHeartbeat( session: request.session, sessionIsolation: request.meta?.sessionIsolation, token: info.token, - send: async (beat) => - await sendRequest(info, beat, settings.transportPreference, settings.paths, timeoutMs), + send: async (beat, budgetMs) => + await sendRequest( + info, + beat, + settings.transportPreference, + settings.paths, + // The beat's own budget governs; the command's heartbeat policy only ever caps it, and an + // unbounded policy leaves the budget standing on its own. + policyTimeoutMs === undefined ? budgetMs : Math.min(policyTimeoutMs, budgetMs), + ), }); } diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts new file mode 100644 index 0000000000..4395ed91fb --- /dev/null +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -0,0 +1,353 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import http from 'node:http'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { sendToDaemon } from '../../../src/daemon-client/daemon-client.ts'; +import type { DaemonRequest } from '../../../src/daemon/daemon-request.ts'; +import { + listenOnLoopback, + skipWhenLoopbackUnavailable, +} from '../../../src/__tests__/test-utils/loopback.ts'; + +const LEASE_ID = 'lease-upload-beat'; +const TOKEN = 'upload-beat-token'; +const APK_BYTES = 4 * 1024 * 1024; +/** Renewed window the fake daemon reports; a third of it is the cadence the client beats on. */ +const LEASE_WINDOW_MS = 3_000; +/** How long a stalled upload gets to observe its own cancellation before the daemon drains it. */ +const CANCEL_NOTICE_MS = 150; + +/** + * #2946's route end to end: `sendToDaemon` uploads an artifact for a remote install before the + * install request is admitted, and the beat is what keeps the lease alive across that gap. The unit + * suite covers the beat loop, the request a beat sends, and the upload client each in isolation; + * none of them would notice the wiring between the three being dropped. + */ + +type FakeDaemon = { + baseUrl: string; + /** Beats and commands the daemon saw, in arrival order. */ + seen: string[]; + uploadBytesDelivered(): number; + /** + * How the upload ended, as the daemon observed it: the artifact drained, or the request carrying + * it was destroyed. Waits for the first of the two, because neither is instantaneous. + */ + uploadOutcome(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; + close(): Promise; +}; + +type UploadBehaviour = 'complete' | 'backpressure'; + +/** + * A remote daemon that only answers a beat, and treats the upload as the long phase it is: + * + * - `complete` drains the artifact and withholds the upload response until a second beat has + * arrived, so "the lease was renewed while the artifact was still uploading" is a fact of the + * test rather than a race it happens to win. + * - `backpressure` stops reading after the first chunk, so the artifact is still in flight when the + * next beat reports the lease gone — the state the abort exists for. + */ +async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise { + const seen: string[] = []; + let uploadBytesDelivered = 0; + let resolveOutcome!: (outcome: 'drained' | 'canceled') => void; + const settled = new Promise<'drained' | 'canceled'>((resolve) => { + resolveOutcome = resolve; + }); + let stopBackpressure: (() => void) | undefined; + let beatsAnswered = 0; + let leaseDeclaredLost = false; + let writeUploadResponse: (() => void) | undefined; + + const server = http.createServer((req, res) => { + if (req.method === 'GET' && (req.url ?? '').startsWith('/health')) { + writeJson(res, 200, { ok: true }); + return; + } + if (req.method !== 'POST') { + res.writeHead(404); + res.end('not found'); + return; + } + if (req.url === '/upload/preflight') { + // Draining before the 404 keeps the connection usable; an early end would make the client's + // fallback to the legacy upload route a matter of socket recycling rather than the protocol. + readJsonBody(req, () => { + res.writeHead(404); + res.end('not found'); + }); + return; + } + if (req.url === '/upload') { + handleUpload(req, res, behaviour, { + onBytes: (length) => { + uploadBytesDelivered += length; + }, + onBodyArrived: () => { + resolveOutcome('drained'); + }, + onCanceled: () => { + resolveOutcome('canceled'); + }, + holdResponse: (write) => { + writeUploadResponse = write; + if (!leaseDeclaredLost && beatsAnswered >= 2) write(); + }, + releaseBackpressure: (resume) => { + stopBackpressure = resume; + }, + }); + return; + } + if (req.url !== '/rpc') { + res.writeHead(404); + res.end('not found'); + return; + } + readJsonBody(req, (payload) => { + if (payload.method === 'agent_device.lease.heartbeat') { + beatsAnswered += 1; + seen.push('lease_heartbeat'); + assert.equal(payload.params?.leaseId, LEASE_ID, 'a beat names the lease it protects'); + // Only the beat that is not the first can say something about the upload: the loop fires + // one at t=0, while the artifact is still being hashed. + const leaseGone = behaviour === 'backpressure' && beatsAnswered >= 2; + if (leaseGone) { + leaseDeclaredLost = true; + // A writer stalled on backpressure and a writer that was just canceled look identical from + // here, so the daemon releases the pressure and lets the difference show: the canceled + // request is already destroyed and stops short, while one nobody canceled drains. + // Deferring it is what keeps that a causal gap rather than a race for the same tick. + setTimeout(() => stopBackpressure?.(), CANCEL_NOTICE_MS).unref(); + } + if (leaseGone) { + writeLeaseLostError(res, payload.id); + } else { + const now = Date.now(); + writeJson(res, 200, { + jsonrpc: '2.0', + id: payload.id, + result: { + ok: true, + data: { lease: { heartbeatAt: now, expiresAt: now + LEASE_WINDOW_MS } }, + }, + }); + } + // A beat that reports the lease gone must not also complete the upload it is meant to + // stop: the artifact's fate is decided by the abort, not by a response from here. + if (!leaseGone) writeUploadResponse?.(); + return; + } + seen.push(String(payload.params?.command ?? payload.method)); + writeJson(res, 200, { + jsonrpc: '2.0', + id: payload.id, + result: { ok: true, data: { package: 'com.example.demo' } }, + }); + }); + }); + server.keepAliveTimeout = 100; + + const port = await listenOnLoopback(server); + return { + baseUrl: `http://127.0.0.1:${String(port)}`, + seen, + uploadBytesDelivered: () => uploadBytesDelivered, + async uploadOutcome(graceMs = 1_000) { + const raced = await Promise.race([ + settled, + new Promise<'unresolved'>((resolve) => { + setTimeout(() => resolve('unresolved'), graceMs).unref(); + }), + ]); + return raced; + }, + async close() { + server.closeAllConnections(); + await new Promise((resolve) => { + server.close(() => resolve()); + }); + }, + }; +} + +function handleUpload( + req: http.IncomingMessage, + res: http.ServerResponse, + behaviour: UploadBehaviour, + hooks: Readonly<{ + onBytes(length: number): void; + onBodyArrived(): void; + onCanceled(): void; + holdResponse(write: () => void): void; + releaseBackpressure(resume: () => void): void; + }>, +): void { + let answered = false; + const respond = (): void => { + if (answered) return; + answered = true; + writeJson(res, 200, { ok: true, uploadId: 'upload-demo.apk' }); + }; + const requestSettled = (): void => { + if (answered) return; + hooks.onCanceled(); + }; + req.on('aborted', requestSettled); + res.on('close', requestSettled); + let stalled = false; + req.on('data', (chunk: Buffer) => { + hooks.onBytes(chunk.length); + // Stopping the read applies backpressure, so the artifact stays in flight instead of racing + // through loopback and making "stopped early" a matter of timing. + // Pausing once, rather than on every chunk: releasing the pressure has to actually let the + // artifact through, otherwise a stalled upload and a canceled one are the same observation. + if (behaviour === 'backpressure' && !stalled) { + req.pause(); + stalled = true; + } + }); + req.on('end', () => { + hooks.onBodyArrived(); + hooks.holdResponse(respond); + }); + hooks.releaseBackpressure(() => { + if (stalled) req.resume(); + }); +} + +function readJsonBody( + req: http.IncomingMessage, + done: (payload: { id: unknown; method: string; params?: Record }) => void, +): void { + let body = ''; + req.setEncoding('utf8'); + req.on('data', (chunk) => { + body += chunk; + }); + req.on('end', () => { + done(JSON.parse(body) as { id: unknown; method: string; params?: Record }); + }); +} + +function writeJson(res: http.ServerResponse, statusCode: number, payload: unknown): void { + res.writeHead(statusCode, { 'content-type': 'application/json' }); + res.end(JSON.stringify(payload)); +} + +function writeLeaseLostError(res: http.ServerResponse, id: unknown): void { + writeJson(res, 400, { + jsonrpc: '2.0', + id, + error: { + code: -32000, + message: 'Lease is not active', + data: { + code: 'UNAUTHORIZED', + message: 'Lease is not active', + details: { reason: 'LEASE_NOT_FOUND' }, + }, + }, + }); +} + +function installRequest( + baseUrl: string, + apkPath: string, + stateDir: string, +): Omit { + return { + session: 'upload-beat', + command: 'install', + positionals: [apkPath], + flags: { + platform: 'android', + daemonBaseUrl: baseUrl, + stateDir, + leaseId: LEASE_ID, + tenant: 'acme', + runId: 'run-1', + leaseProvider: 'proxy', + deviceKey: 'android:mobile:emulator-5554', + clientId: 'client-a', + }, + meta: { cwd: path.dirname(apkPath) }, + }; +} + +const TRANSPORT = { authToken: TOKEN } as const; + +async function withUploadFixture( + t: { skip(reason?: string): void }, + behaviour: UploadBehaviour, + run: (daemon: FakeDaemon, apkPath: string) => Promise, +): Promise { + if (await skipWhenLoopbackUnavailable(t)) return undefined; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-upload-beat-')); + const apkPath = path.join(dir, 'demo.apk'); + fs.writeFileSync(apkPath, Buffer.alloc(APK_BYTES, 'x')); + const daemon = await startFakeRemoteDaemon(behaviour); + try { + return await run(daemon, apkPath); + } finally { + await daemon.close(); + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +test('an install beats the lease while its artifact uploads, before the install RPC', async (t) => { + await withUploadFixture(t, 'complete', async (daemon, apkPath) => { + const response = await sendToDaemon( + installRequest(daemon.baseUrl, apkPath, path.dirname(apkPath)), + TRANSPORT, + ); + + assert.equal(response.ok, true); + assert.ok( + daemon.seen.filter((entry) => entry === 'lease_heartbeat').length >= 2, + `a beat has to land while the upload is held open, daemon saw: ${daemon.seen.join(', ')}`, + ); + assert.equal( + await daemon.uploadOutcome(), + 'drained', + 'the artifact arrived whole on a lease that was being renewed under it', + ); + assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); + }); +}); + +test('a lease lost mid-upload aborts the upload and no install request goes out', async (t) => { + await withUploadFixture(t, 'backpressure', async (daemon, apkPath) => { + await assert.rejects( + async () => + await sendToDaemon( + installRequest(daemon.baseUrl, apkPath, path.dirname(apkPath)), + TRANSPORT, + ), + (error: unknown) => + error instanceof AppError && + error.code === 'UNAUTHORIZED' && + error.details?.reason === 'LEASE_NOT_FOUND', + ); + + assert.ok( + daemon.uploadBytesDelivered() > 0, + 'bytes were already in flight, which is what had to be stopped', + ); + // The daemon released its backpressure after the beat that lost the lease, so an upload nobody + // canceled would have drained to the end. A destroyed request is the only other way this ends. + assert.equal( + await daemon.uploadOutcome(), + 'canceled', + 'the upload was stopped, not left to finish on a lease nobody held', + ); + assert.ok( + !daemon.seen.includes('install'), + `nothing was asked of a device no longer ours, daemon saw: ${daemon.seen.join(', ')}`, + ); + }); +}); From 4f739a499e8377d0c617f4d46341f210374447cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 16:07:03 +0200 Subject: [PATCH 11/21] refactor(remote): move the lease beat to its own module runProtectedLeaseWork, the beat that keeps a remote lease alive across a client-side phase, and the request it sends were three of daemon-client.ts's four exports and existed there only so their tests could reach them. They move to daemon-client-lease-beat.ts, which leaves the transport entry module 287 lines and knowing nothing about cadence, budgets, or which lease reasons are terminal. The test moves with them and is renamed to match. The beat family is a pure move: no behaviour change, and the beat's transport is still the send it is handed, so the module stays free of transport decisions. --- ...st.ts => daemon-client-lease-beat.test.ts} | 2 +- src/daemon-client/daemon-client-lease-beat.ts | 328 ++++++++++++++++++ src/daemon-client/daemon-client.ts | 312 +---------------- .../remote-upload-lease-beat.test.ts | 71 ++-- 4 files changed, 364 insertions(+), 349 deletions(-) rename src/daemon-client/__tests__/{daemon-client-upload-lease.test.ts => daemon-client-lease-beat.test.ts} (99%) create mode 100644 src/daemon-client/daemon-client-lease-beat.ts diff --git a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts similarity index 99% rename from src/daemon-client/__tests__/daemon-client-upload-lease.test.ts rename to src/daemon-client/__tests__/daemon-client-lease-beat.test.ts index 696974b797..cc6551c274 100644 --- a/src/daemon-client/__tests__/daemon-client-upload-lease.test.ts +++ b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts @@ -10,7 +10,7 @@ import { createLeaseRenewalBeat, leaseScopeForHeartbeat, runProtectedLeaseWork, -} from '../daemon-client.ts'; +} from '../daemon-client-lease-beat.ts'; import type { DaemonRequest } from '../../daemon/daemon-request.ts'; function lostLeaseError(reason: string): AppError { diff --git a/src/daemon-client/daemon-client-lease-beat.ts b/src/daemon-client/daemon-client-lease-beat.ts new file mode 100644 index 0000000000..4debd67f32 --- /dev/null +++ b/src/daemon-client/daemon-client-lease-beat.ts @@ -0,0 +1,328 @@ +import type { DaemonRequest } from '../daemon/daemon-request.ts'; +import { AppError } from '@agent-device/kernel/errors'; +import { createRequestId, emitDiagnostic } from '@agent-device/host-kit/diagnostics'; +import { INTERNAL_COMMANDS } from '@agent-device/command-registry/catalog'; +import { resolveCommandTimeoutPolicy } from '@agent-device/command-registry/registry'; +import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/timeout-policy'; +import { + isInactiveLeaseError, + leaseScopeFromRequest, + leaseScopeToRequestMeta, + type LeaseScope, +} from '@agent-device/contracts/lease-scope'; +import type { DaemonClientSettings } from './daemon-client-lifecycle.ts'; +import { isRemoteDaemon, type DaemonInfo } from './daemon-client-metadata.ts'; +import { sendRequest } from './daemon-client-transport.ts'; + +// The lease beat: what keeps a remote lease alive across a client-side phase the daemon never sees. +// +// A lease renews when a request is admitted, and the daemon protects a lease while admitted work runs +// on it (#2509, ADR 0007). An artifact upload is the mirror image: it runs on the caller's side, +// before the install that consumes it is admitted, so nothing renews the lease while it runs and a +// large enough artifact expired the lease that was paying for the device it was being uploaded to +// (#2946). This module owns the answer — a beat with a budget that ends before the window does — and +// knows nothing about transports beyond the `send` it is handed. + +/** + * The fastest cadence a phase beats at, and the budget each beat gets until the window is known. + * + * Before the first beat answers, the window is unknown and the worst legal case is the registry's + * five-second minimum: a beat that stalls must be abandoned and retried inside that window, or the + * thing the beat exists to prevent happens while it waits. One second is a fifth of that minimum, + * and no window-derived cadence is ever allowed below it, so it is also the loop's floor: a + * misreported or pathologically short window cannot turn the beat into a request loop faster than + * this. + */ +const MIN_LEASE_BEAT_INTERVAL_MS = 1_000; + +/** + * Why a beat stopped protecting the lease even though the lease may live on: this client's request + * will never be the one that renews it. + * + * A beat refused for a missing or mismatched owner scope is a fact about the request, not the + * lease, so every successor is refused identically. Surviving it would only spend the upload against + * a lease that stops renewing — the #2946 failure with extra steps. + */ +const UNRENEWABLE_LEASE_BEAT_REASONS: ReadonlySet = new Set([ + 'LEASE_SCOPE_REQUIRED', + 'LEASE_SCOPE_MISMATCH', +]); + +/** + * Whether a beat failed for a reason every successor will repeat: the lease is gone (the shared + * taxonomy), the daemon refused a fact baked into the beat itself, or this request can never renew + * it. A beat's scope and ttl never change across the phase, so an `INVALID_ARGS` refusal — an + * out-of-range ttl, an unusable lease id — is terminal without waiting out a window it can no + * longer renew. + * + * `LEASE_SESSION_MISMATCH` is deliberately absent: only request admission raises it, and + * `lease_heartbeat` is admission-exempt, so a beat can never receive it. + */ +function isTerminalLeaseBeatError(error: unknown): boolean { + return ( + isInactiveLeaseError(error) || + (error instanceof AppError && + (UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason) || error.code === 'INVALID_ARGS')) + ); +} + +/** + * Runs one client-side phase under a lease it does not own the clock of. + * + * A lease renews when a request is admitted, and the daemon protects a lease while ADMITTED work + * runs on it (#2509, ADR 0007). An artifact upload is the mirror image: it happens on the caller's + * side, before the install request that will consume it is admitted, so nothing renews the lease + * while it runs and a large enough artifact expired the lease that was paying for the device it was + * being uploaded to (#2946). + * + * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and + * the phase runs untouched. The first beat is fired immediately rather than one interval in, so a + * lease shorter than that interval is renewed before it can lapse — and a lease already gone is + * found while the upload is still hashing. Each beat answers with the window it just renewed, and + * the cadence becomes a third of that window. + * + * A beat's budget is the cadence it started on, and its successor is armed when the beat starts + * rather than when it settles. A beat that never returns — a half-open connection, a daemon wedged + * before it admits anything — is therefore abandoned on schedule instead of holding the schedule: + * the lease is still beaten at window/3, and the abandoned round trip is cut off by its own budget + * in the transport rather than by the command's 90-second heartbeat policy. An abandoned beat is + * still listened to, because the answer it eventually gives can be a lost lease. + * + * A beat that fails for a reason that says nothing about this lease is reported and survived — one + * lost request must not fail an upload that a later beat will cover. A beat that finds the lease + * gone, or finds this client can never renew it, ends the phase with that error and aborts the + * signal the phase runs under: the device is no longer ours (or was never reachable through this + * request), and the only honest outcome is to say so before the bytes finish. + */ +export async function runProtectedLeaseWork( + options: Readonly<{ + /** + * One renewal. `budgetMs` is how long this beat may take before the loop abandons it. Absent + * when the request names no lease to renew, which is the ordinary unleased install. + */ + heartbeat?: ((budgetMs: number) => Promise) | undefined; + task: (signal: AbortSignal) => Promise; + }>, +): Promise { + const { heartbeat } = options; + if (!heartbeat) return await options.task(new AbortController().signal); + + const control = new AbortController(); + // Until a beat names the window, the loop assumes the shortest window the daemon will accept: the + // budget of the beat that has to prove a short lease is alive cannot itself be longer than it. + let intervalMs = MIN_LEASE_BEAT_INTERVAL_MS; + let timer: ReturnType | undefined; + let stopped = false; + let terminalError: unknown; + let reportTerminal: ((error: unknown) => void) | undefined; + const terminal = new Promise((_, reject) => { + reportTerminal = reject; + }); + + const runBeat = (): void => { + // Armed while this beat is still outstanding: a beat that never settles is abandoned on + // schedule rather than taking the schedule with it. The beat may re-arm it on the way out. + const arm = (delayMs: number): void => { + if (stopped) return; + if (timer) clearTimeout(timer); + timer = setTimeout(runBeat, delayMs); + }; + arm(intervalMs); + const settle = (async () => { + const budgetMs = intervalMs; + try { + const renewed = leaseWindowFromHeartbeatResponse(await heartbeat(budgetMs)); + // An answer that names no window keeps the cadence it was asked at: the loop only ever + // slows down on evidence of how long the lease is good for, and never on the absence of it. + if (renewed === undefined) return; + const cadence = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); + if (cadence === intervalMs) return; + intervalMs = cadence; + // The window just moved, so the next beat is due one cadence from this answer. + arm(cadence); + } catch (error) { + if (isTerminalLeaseBeatError(error)) { + terminalError = error; + if (stopped) { + // The phase settled first; the outcome it returned already stands, but a lease this + // client just learned is gone is worth one diagnostic on the way out. + emitDiagnostic({ + level: 'warn', + phase: 'lease_lost_after_phase', + data: { message: error instanceof Error ? error.message : String(error) }, + }); + return; + } + // The upload is the only thing still consuming this phase's time, and it is pointed at a + // device this client can no longer renew. Stop it rather than finish bytes nobody owns. + control.abort(); + reportTerminal?.(error); + return; + } + emitDiagnostic({ + level: 'warn', + phase: 'lease_heartbeat_failed', + data: { message: error instanceof Error ? error.message : String(error) }, + }); + } + })(); + // A beat the loop has moved on from is still listened to, and nothing awaits it: its outcome is + // swallowed here so a beat nobody is waiting on cannot surface as an unhandled rejection. + void settle.catch(() => undefined); + }; + + // Armed before the phase starts, not one interval in: a beat is what proves the lease the upload + // is spending its time on is still alive. + timer = setTimeout(runBeat, 0); + // A beat already in flight when the phase settles can still report a lost lease; the caller reads + // it from `terminalError`, so nothing may be left racing on this rejection by then. + void terminal.catch(() => undefined); + const phase = await captureOutcome( + // The async boundary also turns a synchronous throw from the phase into a rejection, so the + // timer below is always cleared. + (async () => await Promise.race([options.task(control.signal), terminal]))(), + ); + stopped = true; + if (timer) clearTimeout(timer); + // No outstanding beat is awaited here: a beat on a half-open connection would hold a finished + // upload behind its own budget for no decision the phase still has to make. + // A beat that ended the protection outranks a phase that settled meanwhile, from either side: the + // device is no longer ours, and the lease error is the reason the phase was not worth finishing. + if (terminalError !== undefined) throw terminalError; + if (!phase.ok) throw phase.error; + return phase.value; +} + +/** + * The inactivity window a beat just renewed, read from the lease its response carries. + * + * `heartbeatLease` answers with the lease, whose `expiresAt - heartbeatAt` is exactly the window it + * extended — the same pair `leaseOwnTtlMs` renews on. Anything unrecognizable leaves the caller on + * the fallback cadence rather than guessing one. + */ +function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined { + const lease = ( + response as Readonly<{ data?: Readonly<{ lease?: Readonly> }> }> + )?.data?.lease; + const expiresAt = lease?.expiresAt; + const heartbeatAt = lease?.heartbeatAt; + if (typeof expiresAt !== 'number' || typeof heartbeatAt !== 'number') return undefined; + return expiresAt > heartbeatAt ? expiresAt - heartbeatAt : undefined; +} + +type Outcome = { ok: true; value: T } | { ok: false; error: unknown }; + +async function captureOutcome(promise: Promise): Promise> { + try { + return { ok: true, value: await promise }; + } catch (error) { + return { ok: false, error }; + } +} + +/** + * The beat that keeps a remote lease alive across a long client-side phase. + * + * `send` is the caller's transport and `budgetMs` is how long this beat may take: the beat's answer + * only matters before the next one is due, so a stalled round trip is cut off at the cadence rather + * than at the command's own 90-second heartbeat policy. A beat is only ever sent to a remote daemon, + * where a transport timeout performs no local cleanup. + */ +export function createLeaseRenewalBeat( + leaseScope: LeaseScope, + context: Readonly<{ + session: string; + sessionIsolation?: NonNullable['sessionIsolation']; + token: string; + send: (request: DaemonRequest, budgetMs: number) => Promise; + }>, +): (budgetMs: number) => Promise { + return async (budgetMs) => + await context.send( + buildLeaseHeartbeatRequest(leaseScope, { + session: context.session, + sessionIsolation: context.sessionIsolation, + requestId: createRequestId(), + token: context.token, + }), + budgetMs, + ); +} + +/** + * The request one beat sends: the command's lease scope, its own id, and nothing else. + * + * It is a fresh request rather than the install rewritten, so nothing about the upload — its source, + * its positional, its own request id — can be mistaken for the lease's state. The scope rides along + * as the command named it, so a beat renews for whatever window that command asked for, and for the + * lease's own window when it named none, which is the ordinary case for an install. Each beat gets a + * fresh id because a beat that times out is canceled under its own, and a shared id would let a later + * beat inherit an earlier cancellation. + */ +export function buildLeaseHeartbeatRequest( + leaseScope: LeaseScope, + context: Readonly<{ + session: string; + sessionIsolation?: NonNullable['sessionIsolation']; + requestId: string; + token: string; + }>, +): DaemonRequest { + return { + command: INTERNAL_COMMANDS.leaseHeartbeat, + positionals: [], + session: context.session, + token: context.token, + meta: { + ...leaseScopeToRequestMeta(leaseScope), + sessionIsolation: context.sessionIsolation, + requestId: context.requestId, + }, + }; +} + +/** The lease a request is running under, when it names one. */ +export function leaseScopeForHeartbeat( + request: Pick, +): LeaseScope | undefined { + const scope = leaseScopeFromRequest(request); + return scope.leaseId ? scope : undefined; +} + +/** + * The beat that renews a remote lease across an artifact upload, or `undefined` when there is no + * lease to protect: only a remote daemon uploads, so only one can be waiting on a billed device, and + * a command that names no lease has nothing to renew. + * + * Each beat's transport timeout is its budget, capped by the command's heartbeat policy: the answer + * is worthless once the next beat is due, so a stalled round trip is cut off and destroyed at the + * cadence instead of holding a socket for 90 seconds. `sendToDaemon` wires this once per upload. + */ +export function buildUploadLeaseHeartbeat( + info: DaemonInfo, + settings: DaemonClientSettings, + request: Omit, +): ((budgetMs: number) => Promise) | undefined { + if (!isRemoteDaemon(info)) return undefined; + const leaseScope = leaseScopeForHeartbeat(request); + if (!leaseScope) return undefined; + const policyTimeoutMs = resolveCommandRequestTimeoutMs( + resolveCommandTimeoutPolicy(INTERNAL_COMMANDS.leaseHeartbeat), + { positionals: [] }, + ); + return createLeaseRenewalBeat(leaseScope, { + session: request.session, + sessionIsolation: request.meta?.sessionIsolation, + token: info.token, + send: async (beat, budgetMs) => + await sendRequest( + info, + beat, + settings.transportPreference, + settings.paths, + // The beat's own budget governs; the command's heartbeat policy only ever caps it, and an + // unbounded policy leaves the budget standing on its own. + policyTimeoutMs === undefined ? budgetMs : Math.min(policyTimeoutMs, budgetMs), + ), + }); +} diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index 925c6b2082..13610507eb 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -14,13 +14,6 @@ import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '@agent-device/command-regist import { resolveCommandTimeoutPolicy } from '@agent-device/command-registry/registry'; import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/timeout-policy'; import { prepareRemoteRequestArtifacts } from '../remote/daemon-artifacts.ts'; -import { isRemoteDaemon } from './daemon-client-metadata.ts'; -import { - isInactiveLeaseError, - leaseScopeFromRequest, - leaseScopeToRequestMeta, - type LeaseScope, -} from '@agent-device/contracts/lease-scope'; import { attachActiveSessionAddressHint, attachRepairSessionAddressHint, @@ -33,6 +26,7 @@ import { type EnsuredDaemon, } from './daemon-client-lifecycle.ts'; import { sendRequest } from './daemon-client-transport.ts'; +import { buildUploadLeaseHeartbeat, runProtectedLeaseWork } from './daemon-client-lease-beat.ts'; export type DaemonRequest = SharedDaemonRequest; export type DaemonResponse = SharedDaemonResponse; @@ -277,310 +271,6 @@ function withActiveSessionAddressHint( ); } -/** - * The fastest cadence a phase beats at, and the budget each beat gets until the window is known. - * - * Before the first beat answers, the window is unknown and the worst legal case is the registry's - * five-second minimum: a beat that stalls must be abandoned and retried inside that window, or the - * thing the beat exists to prevent happens while it waits. One second is a fifth of that minimum, - * and no window-derived cadence is ever allowed below it, so it is also the loop's floor: a - * misreported or pathologically short window cannot turn the beat into a request loop faster than - * this. - */ -const MIN_LEASE_BEAT_INTERVAL_MS = 1_000; - -/** - * Why a beat stopped protecting the lease even though the lease may live on: this client's request - * will never be the one that renews it. - * - * A beat refused for a missing or mismatched owner scope is a fact about the request, not the - * lease, so every successor is refused identically. Surviving it would only spend the upload against - * a lease that stops renewing — the #2946 failure with extra steps. - */ -const UNRENEWABLE_LEASE_BEAT_REASONS: ReadonlySet = new Set([ - 'LEASE_SCOPE_REQUIRED', - 'LEASE_SCOPE_MISMATCH', -]); - -/** - * Whether a beat failed for a reason every successor will repeat: the lease is gone (the shared - * taxonomy), the daemon refused a fact baked into the beat itself, or this request can never renew - * it. A beat's scope and ttl never change across the phase, so an `INVALID_ARGS` refusal — an - * out-of-range ttl, an unusable lease id — is terminal without waiting out a window it can no - * longer renew. - * - * `LEASE_SESSION_MISMATCH` is deliberately absent: only request admission raises it, and - * `lease_heartbeat` is admission-exempt, so a beat can never receive it. - */ -function isTerminalLeaseBeatError(error: unknown): boolean { - return ( - isInactiveLeaseError(error) || - (error instanceof AppError && - (UNRENEWABLE_LEASE_BEAT_REASONS.has(error.details?.reason) || error.code === 'INVALID_ARGS')) - ); -} - -/** - * Runs one client-side phase under a lease it does not own the clock of. - * - * A lease renews when a request is admitted, and the daemon protects a lease while ADMITTED work - * runs on it (#2509, ADR 0007). An artifact upload is the mirror image: it happens on the caller's - * side, before the install request that will consume it is admitted, so nothing renews the lease - * while it runs and a large enough artifact expired the lease that was paying for the device it was - * being uploaded to (#2946). - * - * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and - * the phase runs untouched. The first beat is fired immediately rather than one interval in, so a - * lease shorter than that interval is renewed before it can lapse — and a lease already gone is - * found while the upload is still hashing. Each beat answers with the window it just renewed, and - * the cadence becomes a third of that window. - * - * A beat's budget is the cadence it started on, and its successor is armed when the beat starts - * rather than when it settles. A beat that never returns — a half-open connection, a daemon wedged - * before it admits anything — is therefore abandoned on schedule instead of holding the schedule: - * the lease is still beaten at window/3, and the abandoned round trip is cut off by its own budget - * in the transport rather than by the command's 90-second heartbeat policy. An abandoned beat is - * still listened to, because the answer it eventually gives can be a lost lease. - * - * A beat that fails for a reason that says nothing about this lease is reported and survived — one - * lost request must not fail an upload that a later beat will cover. A beat that finds the lease - * gone, or finds this client can never renew it, ends the phase with that error and aborts the - * signal the phase runs under: the device is no longer ours (or was never reachable through this - * request), and the only honest outcome is to say so before the bytes finish. - */ -export async function runProtectedLeaseWork( - options: Readonly<{ - /** - * One renewal. `budgetMs` is how long this beat may take before the loop abandons it. Absent - * when the request names no lease to renew, which is the ordinary unleased install. - */ - heartbeat?: ((budgetMs: number) => Promise) | undefined; - task: (signal: AbortSignal) => Promise; - }>, -): Promise { - const { heartbeat } = options; - if (!heartbeat) return await options.task(new AbortController().signal); - - const control = new AbortController(); - // Until a beat names the window, the loop assumes the shortest window the daemon will accept: the - // budget of the beat that has to prove a short lease is alive cannot itself be longer than it. - let intervalMs = MIN_LEASE_BEAT_INTERVAL_MS; - let timer: ReturnType | undefined; - let stopped = false; - let terminalError: unknown; - let reportTerminal: ((error: unknown) => void) | undefined; - const terminal = new Promise((_, reject) => { - reportTerminal = reject; - }); - - const runBeat = (): void => { - // Armed while this beat is still outstanding: a beat that never settles is abandoned on - // schedule rather than taking the schedule with it. The beat may re-arm it on the way out. - const arm = (delayMs: number): void => { - if (stopped) return; - if (timer) clearTimeout(timer); - timer = setTimeout(runBeat, delayMs); - }; - arm(intervalMs); - const settle = (async () => { - const budgetMs = intervalMs; - try { - const renewed = leaseWindowFromHeartbeatResponse(await heartbeat(budgetMs)); - // An answer that names no window keeps the cadence it was asked at: the loop only ever - // slows down on evidence of how long the lease is good for, and never on the absence of it. - if (renewed === undefined) return; - const cadence = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); - if (cadence === intervalMs) return; - intervalMs = cadence; - // The window just moved, so the next beat is due one cadence from this answer. - arm(cadence); - } catch (error) { - if (isTerminalLeaseBeatError(error)) { - terminalError = error; - if (stopped) { - // The phase settled first; the outcome it returned already stands, but a lease this - // client just learned is gone is worth one diagnostic on the way out. - emitDiagnostic({ - level: 'warn', - phase: 'lease_lost_after_phase', - data: { message: error instanceof Error ? error.message : String(error) }, - }); - return; - } - // The upload is the only thing still consuming this phase's time, and it is pointed at a - // device this client can no longer renew. Stop it rather than finish bytes nobody owns. - control.abort(); - reportTerminal?.(error); - return; - } - emitDiagnostic({ - level: 'warn', - phase: 'lease_heartbeat_failed', - data: { message: error instanceof Error ? error.message : String(error) }, - }); - } - })(); - // A beat the loop has moved on from is still listened to, and nothing awaits it: its outcome is - // swallowed here so a beat nobody is waiting on cannot surface as an unhandled rejection. - void settle.catch(() => undefined); - }; - - // Armed before the phase starts, not one interval in: a beat is what proves the lease the upload - // is spending its time on is still alive. - timer = setTimeout(runBeat, 0); - // A beat already in flight when the phase settles can still report a lost lease; the caller reads - // it from `terminalError`, so nothing may be left racing on this rejection by then. - void terminal.catch(() => undefined); - const phase = await captureOutcome( - // The async boundary also turns a synchronous throw from the phase into a rejection, so the - // timer below is always cleared. - (async () => await Promise.race([options.task(control.signal), terminal]))(), - ); - stopped = true; - if (timer) clearTimeout(timer); - // No outstanding beat is awaited here: a beat on a half-open connection would hold a finished - // upload behind its own budget for no decision the phase still has to make. - // A beat that ended the protection outranks a phase that settled meanwhile, from either side: the - // device is no longer ours, and the lease error is the reason the phase was not worth finishing. - if (terminalError !== undefined) throw terminalError; - if (!phase.ok) throw phase.error; - return phase.value; -} - -/** - * The inactivity window a beat just renewed, read from the lease its response carries. - * - * `heartbeatLease` answers with the lease, whose `expiresAt - heartbeatAt` is exactly the window it - * extended — the same pair `leaseOwnTtlMs` renews on. Anything unrecognizable leaves the caller on - * the fallback cadence rather than guessing one. - */ -function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined { - const lease = ( - response as Readonly<{ data?: Readonly<{ lease?: Readonly> }> }> - )?.data?.lease; - const expiresAt = lease?.expiresAt; - const heartbeatAt = lease?.heartbeatAt; - if (typeof expiresAt !== 'number' || typeof heartbeatAt !== 'number') return undefined; - return expiresAt > heartbeatAt ? expiresAt - heartbeatAt : undefined; -} - -type Outcome = { ok: true; value: T } | { ok: false; error: unknown }; - -async function captureOutcome(promise: Promise): Promise> { - try { - return { ok: true, value: await promise }; - } catch (error) { - return { ok: false, error }; - } -} - -/** - * The beat that keeps a remote lease alive across a long client-side phase. - * - * `send` is the caller's transport and `budgetMs` is how long this beat may take: the beat's answer - * only matters before the next one is due, so a stalled round trip is cut off at the cadence rather - * than at the command's own 90-second heartbeat policy. A beat is only ever sent to a remote daemon, - * where a transport timeout performs no local cleanup. - */ -export function createLeaseRenewalBeat( - leaseScope: LeaseScope, - context: Readonly<{ - session: string; - sessionIsolation?: NonNullable['sessionIsolation']; - token: string; - send: (request: DaemonRequest, budgetMs: number) => Promise; - }>, -): (budgetMs: number) => Promise { - return async (budgetMs) => - await context.send( - buildLeaseHeartbeatRequest(leaseScope, { - session: context.session, - sessionIsolation: context.sessionIsolation, - requestId: createRequestId(), - token: context.token, - }), - budgetMs, - ); -} - -/** - * The request one beat sends: the command's lease scope, its own id, and nothing else. - * - * It is a fresh request rather than the install rewritten, so nothing about the upload — its source, - * its positional, its own request id — can be mistaken for the lease's state. The scope rides along - * as the command named it, so a beat renews for whatever window that command asked for, and for the - * lease's own window when it named none, which is the ordinary case for an install. Each beat gets a - * fresh id because a beat that times out is canceled under its own, and a shared id would let a later - * beat inherit an earlier cancellation. - */ -export function buildLeaseHeartbeatRequest( - leaseScope: LeaseScope, - context: Readonly<{ - session: string; - sessionIsolation?: NonNullable['sessionIsolation']; - requestId: string; - token: string; - }>, -): DaemonRequest { - return { - command: INTERNAL_COMMANDS.leaseHeartbeat, - positionals: [], - session: context.session, - token: context.token, - meta: { - ...leaseScopeToRequestMeta(leaseScope), - sessionIsolation: context.sessionIsolation, - requestId: context.requestId, - }, - }; -} - -/** The lease a request is running under, when it names one. */ -export function leaseScopeForHeartbeat( - request: Pick, -): LeaseScope | undefined { - const scope = leaseScopeFromRequest(request); - return scope.leaseId ? scope : undefined; -} - -/** - * The beat that renews a remote lease across an artifact upload, or `undefined` when there is no - * lease to protect: only a remote daemon uploads, so only one can be waiting on a billed device, and - * a command that names no lease has nothing to renew. - * - * Each beat's transport timeout is its budget, capped by the command's heartbeat policy: the answer - * is worthless once the next beat is due, so a stalled round trip is cut off and destroyed at the - * cadence instead of holding a socket for 90 seconds. `sendToDaemon` wires this once per upload. - */ -export function buildUploadLeaseHeartbeat( - info: EnsuredDaemon['info'], - settings: DaemonClientSettings, - request: Omit, -): ((budgetMs: number) => Promise) | undefined { - if (!isRemoteDaemon(info)) return undefined; - const leaseScope = leaseScopeForHeartbeat(request); - if (!leaseScope) return undefined; - const policyTimeoutMs = resolveCommandRequestTimeoutMs( - resolveCommandTimeoutPolicy(INTERNAL_COMMANDS.leaseHeartbeat), - { positionals: [] }, - ); - return createLeaseRenewalBeat(leaseScope, { - session: request.session, - sessionIsolation: request.meta?.sessionIsolation, - token: info.token, - send: async (beat, budgetMs) => - await sendRequest( - info, - beat, - settings.transportPreference, - settings.paths, - // The beat's own budget governs; the command's heartbeat policy only ever caps it, and an - // unbounded policy leaves the budget standing on its own. - policyTimeoutMs === undefined ? budgetMs : Math.min(policyTimeoutMs, budgetMs), - ), - }); -} - function writeInstallInProgressNotice(command: string | undefined): void { if (!isInstallLikeCommand(command) || process.stderr.isTTY !== true || process.env.CI) return; process.stderr.write( diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 4395ed91fb..22f30e7723 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -110,36 +110,7 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise { if (payload.method === 'agent_device.lease.heartbeat') { - beatsAnswered += 1; - seen.push('lease_heartbeat'); - assert.equal(payload.params?.leaseId, LEASE_ID, 'a beat names the lease it protects'); - // Only the beat that is not the first can say something about the upload: the loop fires - // one at t=0, while the artifact is still being hashed. - const leaseGone = behaviour === 'backpressure' && beatsAnswered >= 2; - if (leaseGone) { - leaseDeclaredLost = true; - // A writer stalled on backpressure and a writer that was just canceled look identical from - // here, so the daemon releases the pressure and lets the difference show: the canceled - // request is already destroyed and stops short, while one nobody canceled drains. - // Deferring it is what keeps that a causal gap rather than a race for the same tick. - setTimeout(() => stopBackpressure?.(), CANCEL_NOTICE_MS).unref(); - } - if (leaseGone) { - writeLeaseLostError(res, payload.id); - } else { - const now = Date.now(); - writeJson(res, 200, { - jsonrpc: '2.0', - id: payload.id, - result: { - ok: true, - data: { lease: { heartbeatAt: now, expiresAt: now + LEASE_WINDOW_MS } }, - }, - }); - } - // A beat that reports the lease gone must not also complete the upload it is meant to - // stop: the artifact's fate is decided by the abort, not by a response from here. - if (!leaseGone) writeUploadResponse?.(); + answerBeat(res, payload); return; } seen.push(String(payload.params?.command ?? payload.method)); @@ -150,6 +121,34 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise= 2; + if (leaseGone) { + leaseDeclaredLost = true; + // A writer stalled on backpressure and a writer that was just canceled look identical from + // here, so the daemon releases the pressure and lets the difference show: the canceled + // request is already destroyed and stops short, while one nobody canceled drains. Deferring + // it is what keeps that a causal gap rather than a race for the same tick. + setTimeout(() => stopBackpressure?.(), CANCEL_NOTICE_MS).unref(); + writeLeaseLostError(res, payload.id); + // A beat that reports the lease gone must not also complete the upload it is meant to stop: + // the artifact's fate is decided by the abort, not by a response from here. + return; + } + const now = Date.now(); + writeJson(res, 200, { + jsonrpc: '2.0', + id: payload.id, + result: { ok: true, data: { lease: { heartbeatAt: now, expiresAt: now + LEASE_WINDOW_MS } } }, + }); + writeUploadResponse?.(); + } server.keepAliveTimeout = 100; const port = await listenOnLoopback(server); @@ -158,13 +157,12 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise uploadBytesDelivered, async uploadOutcome(graceMs = 1_000) { - const raced = await Promise.race([ + return await Promise.race([ settled, new Promise<'unresolved'>((resolve) => { setTimeout(() => resolve('unresolved'), graceMs).unref(); }), ]); - return raced; }, async close() { server.closeAllConnections(); @@ -220,17 +218,16 @@ function handleUpload( }); } -function readJsonBody( - req: http.IncomingMessage, - done: (payload: { id: unknown; method: string; params?: Record }) => void, -): void { +type RpcPayload = Readonly<{ id: unknown; method: string; params?: Record }>; + +function readJsonBody(req: http.IncomingMessage, done: (payload: RpcPayload) => void): void { let body = ''; req.setEncoding('utf8'); req.on('data', (chunk) => { body += chunk; }); req.on('end', () => { - done(JSON.parse(body) as { id: unknown; method: string; params?: Record }); + done(JSON.parse(body) as RpcPayload); }); } From ffabb5e1a36767b2c605d0453bf51829e457022b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 16:08:36 +0200 Subject: [PATCH 12/21] chore(gates): record the lease-beat module's type-only read of the wire request R78 names measured edges, never a directory, so the extracted module owes its own entry: a type-only import of the same DaemonRequest vocabulary the four sibling client modules already record. --- scripts/layering/daemon-client-entry.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/layering/daemon-client-entry.ts b/scripts/layering/daemon-client-entry.ts index 3c40ddce4a..1c7ced3664 100644 --- a/scripts/layering/daemon-client-entry.ts +++ b/scripts/layering/daemon-client-entry.ts @@ -43,6 +43,11 @@ export const DAEMON_CLIENT_ENTRY_EDGES: readonly DaemonClientEntryEdge[] = [ target: 'src/daemon/daemon-request.ts', rationale: REQUEST_RATIONALE, }, + { + file: 'src/daemon-client/daemon-client-lease-beat.ts', + target: 'src/daemon/daemon-request.ts', + rationale: REQUEST_RATIONALE, + }, { file: 'src/daemon-client/daemon-client-progress.ts', target: 'src/daemon/daemon-request.ts', From 612909b2590d9ce9a09932289525b51a35df8edd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 16:11:13 +0200 Subject: [PATCH 13/21] docs(leases): state what the first beat actually buys, and what an absent ttlMs keeps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0007 and the loop's JSDoc both claimed a missing lease is caught before any bytes move. Hashing, the preflight, and the start of the stream can all run while the opening beat is still outstanding, so the claim was stronger than the behaviour: the first beat buys learning the loss during the upload, not before it. The same section described the successor as armed a third of a window after the beat lands, which the budget change just superseded; it now says the successor is armed at beat start and the budget is capped at the cadence, and the survived- failure paragraph says that promise covers a beat abandoned at its budget, not only one that fails fast — the distinction the review found in the old shape. remote-proxy.md promises five minutes of proxy lease inactivity for CLI connect. That holds because `open` names the window; a lease allocated over the RPC with no ttlMs keeps the daemon's one-minute default, so the sentence now says which one a reader gets. --- docs/adr/0007-remote-device-leases.md | 16 +++++++++++----- src/daemon-client/daemon-client-lease-beat.ts | 7 ++++--- website/docs/docs/remote-proxy.md | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index a51c178731..eaf4b8a6ab 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -84,10 +84,14 @@ window too; the window a lease carries is the one its client named when it alloc The first beat is fired when the phase starts, not one cadence in, because a beat is what proves the lease the upload is spending time on is still alive — a lease shorter than any fixed cadence would -otherwise lapse before anything renewed it, and a lease already gone is worth learning that before any -bytes move. Each beat answers with the window it just renewed, and the next one is armed a third of -that window after the beat lands: beats never overlap, and one slower than the cadence delays its -successor instead of replacing the schedule or suppressing every beat behind it. +otherwise lapse before anything renewed it. A lease already gone is caught early, but not before the +phase begins: hashing, the preflight, and the start of the stream can all run while the first beat is +still outstanding, so what the first beat buys is that the loss is learned during the upload rather +than after it. Each beat answers with the window it just renewed, and the loop arms its successor when +the beat starts rather than when it settles, so a beat that never answers is abandoned on schedule +instead of taking the schedule with it; the beat's own budget is capped at the cadence it started on, +which is what keeps one stalled round trip from outliving the window it exists to protect. A phase +that settles does not wait on a beat still in flight. A beat is a fresh request each time, never the protected request rewritten: a request identity is what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat @@ -95,7 +99,9 @@ that finds the lease gone, or finds that this request can never renew it — its belongs to another lease, or the daemon rejects the scope and window the beat itself was built with, which no successor will ask differently — ends the phase with that error and cancels the upload rather than finishing bytes against a device nobody owns or a lease that will stop renewing. A beat that fails -for any other reason is reported and survived, because a later beat covers one lost request. +for any other reason is reported and survived, because a later beat covers one lost request — including +one that was abandoned at its budget rather than answered, which is what makes that promise hold for a +stalled round trip and not only for one that fails fast. ## Human control diff --git a/src/daemon-client/daemon-client-lease-beat.ts b/src/daemon-client/daemon-client-lease-beat.ts index 4debd67f32..992edfaac7 100644 --- a/src/daemon-client/daemon-client-lease-beat.ts +++ b/src/daemon-client/daemon-client-lease-beat.ts @@ -77,9 +77,10 @@ function isTerminalLeaseBeatError(error: unknown): boolean { * * `heartbeat` is the caller's transport decision; `undefined` means there is no lease to protect and * the phase runs untouched. The first beat is fired immediately rather than one interval in, so a - * lease shorter than that interval is renewed before it can lapse — and a lease already gone is - * found while the upload is still hashing. Each beat answers with the window it just renewed, and - * the cadence becomes a third of that window. + * lease shorter than that interval is renewed before it can lapse. A lease already gone is caught + * early rather than at the end of the phase, though not before the phase begins: hashing, preflight, + * and the first bytes can all happen while the opening beat is still outstanding. Each beat answers + * with the window it just renewed, and the cadence becomes a third of that window. * * A beat's budget is the cadence it started on, and its successor is armed when the beat starts * rather than when it settles. A beat that never returns — a half-open connection, a daemon wedged diff --git a/website/docs/docs/remote-proxy.md b/website/docs/docs/remote-proxy.md index 593a2a8ae0..1668f1716c 100644 --- a/website/docs/docs/remote-proxy.md +++ b/website/docs/docs/remote-proxy.md @@ -45,7 +45,7 @@ agent-device disconnect Passing `--daemon-auth-token ` instead of exporting the environment variable also works, but only authenticates the single command it is passed to; subsequent commands need the token again through the env var, a `daemonAuthToken` entry in your remote config profile, or a repeated `--daemon-auth-token` flag. -`connect proxy` stores the proxy profile and client identity. Device leases are automatic on `open` and expire after five minutes without commands. `close` releases the active session and device lease; `disconnect` clears local connection state. +`connect proxy` stores the proxy profile and client identity. Device leases are automatic on `open` and expire after five minutes without commands. That five minutes is the window `open` asks for; a lease allocated directly over the RPC without `ttlMs` keeps the daemon's one-minute inactivity default instead. `close` releases the active session and device lease; `disconnect` clears local connection state. Multiple agents can share one proxy when each uses the normal `connect proxy`, `open`, commands, `close`, and `disconnect` flow. A busy device error means another agent owns the device until it closes or its inactivity lease expires. From d9b7af7fa43a7c6481e08feb4747720fe5a0e01e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 16:52:07 +0200 Subject: [PATCH 14/21] test(remote): let the caller release the stalled upload, so the outcome is causal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lost-lease case waited on whichever of two events reached the fake daemon first, with a grace window deciding an inconclusive answer. A client blocked in a kernel write cannot observe its own cancellation until the peer drains, so that window had to be longer than the drain — timing the test had to win rather than a fact it could state. The fake daemon now holds the artifact until the test releases it, and only releases it after sendToDaemon has already rejected. An upload nobody canceled has nothing left that could stop it by then and drains; one that was destroyed cannot. The success case reads the delivered byte count instead of racing the same oracle. Killing either half of the wiring still fails: dropping the heartbeat leaves no beat and both cases time out, dropping the signal hands the daemon a drained artifact. --- .../remote-upload-lease-beat.test.ts | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 22f30e7723..5c39cb45c5 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -17,8 +17,6 @@ const TOKEN = 'upload-beat-token'; const APK_BYTES = 4 * 1024 * 1024; /** Renewed window the fake daemon reports; a third of it is the cadence the client beats on. */ const LEASE_WINDOW_MS = 3_000; -/** How long a stalled upload gets to observe its own cancellation before the daemon drains it. */ -const CANCEL_NOTICE_MS = 150; /** * #2946's route end to end: `sendToDaemon` uploads an artifact for a remote install before the @@ -33,10 +31,14 @@ type FakeDaemon = { seen: string[]; uploadBytesDelivered(): number; /** - * How the upload ended, as the daemon observed it: the artifact drained, or the request carrying - * it was destroyed. Waits for the first of the two, because neither is instantaneous. + * Lets a stalled artifact through, and reports how the upload ended: `drained` if the whole + * artifact arrived, `canceled` if the request carrying it was destroyed first. + * + * The caller releases it rather than a beat answering, so the answer is causal: the client has + * already been told the lease is gone by then, and an upload it did not cancel has nothing left + * that could stop it. */ - uploadOutcome(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; + releaseUploadAndObserveOutcome(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; close(): Promise; }; @@ -131,11 +133,6 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise= 2; if (leaseGone) { leaseDeclaredLost = true; - // A writer stalled on backpressure and a writer that was just canceled look identical from - // here, so the daemon releases the pressure and lets the difference show: the canceled - // request is already destroyed and stops short, while one nobody canceled drains. Deferring - // it is what keeps that a causal gap rather than a race for the same tick. - setTimeout(() => stopBackpressure?.(), CANCEL_NOTICE_MS).unref(); writeLeaseLostError(res, payload.id); // A beat that reports the lease gone must not also complete the upload it is meant to stop: // the artifact's fate is decided by the abort, not by a response from here. @@ -156,7 +153,8 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise uploadBytesDelivered, - async uploadOutcome(graceMs = 1_000) { + async releaseUploadAndObserveOutcome(graceMs = 2_000) { + stopBackpressure?.(); return await Promise.race([ settled, new Promise<'unresolved'>((resolve) => { @@ -309,8 +307,8 @@ test('an install beats the lease while its artifact uploads, before the install `a beat has to land while the upload is held open, daemon saw: ${daemon.seen.join(', ')}`, ); assert.equal( - await daemon.uploadOutcome(), - 'drained', + daemon.uploadBytesDelivered(), + APK_BYTES, 'the artifact arrived whole on a lease that was being renewed under it', ); assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); @@ -332,13 +330,16 @@ test('a lease lost mid-upload aborts the upload and no install request goes out' ); assert.ok( - daemon.uploadBytesDelivered() > 0, - 'bytes were already in flight, which is what had to be stopped', + daemon.uploadBytesDelivered() > 0 && daemon.uploadBytesDelivered() < APK_BYTES, + `bytes were in flight and the daemon stopped reading at ${String( + daemon.uploadBytesDelivered(), + )} of ${String(APK_BYTES)}, which is what had to be stopped`, ); - // The daemon released its backpressure after the beat that lost the lease, so an upload nobody - // canceled would have drained to the end. A destroyed request is the only other way this ends. + // The daemon never read past the first chunk, so this is the only way the artifact can stop + // short: the client destroyed the request. An upload nobody canceled drains once the pressure + // comes off, and by now the client has long since been told the lease is gone. assert.equal( - await daemon.uploadOutcome(), + await daemon.releaseUploadAndObserveOutcome(), 'canceled', 'the upload was stopped, not left to finish on a lease nobody held', ); From 8beb7fb2ad70a53025daa622fe9436831dc790eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 11:29:24 +0200 Subject: [PATCH 15/21] refactor(remote): fold the beat's thin wrappers into the one factory that needs them collapse createLeaseRenewalBeat and leaseScopeForHeartbeat into buildUploadLeaseHeartbeat, which was their only caller, and let the beat tests read the scope through the contracts helper the factory itself uses. the per-beat request id is now pinned over the real transport, where a shared id is observable, instead of against an injected send. --- .../daemon-client-lease-beat.test.ts | 168 ++++--------- src/daemon-client/daemon-client-lease-beat.ts | 71 ++---- .../remote-upload-lease-beat.test.ts | 232 +++++++----------- 3 files changed, 156 insertions(+), 315 deletions(-) diff --git a/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts index cc6551c274..2039d9af48 100644 --- a/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts +++ b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts @@ -7,10 +7,9 @@ import { resolveDaemonPaths } from '../../daemon-resolution.ts'; import { buildLeaseHeartbeatRequest, buildUploadLeaseHeartbeat, - createLeaseRenewalBeat, - leaseScopeForHeartbeat, runProtectedLeaseWork, } from '../daemon-client-lease-beat.ts'; +import { leaseScopeFromRequest } from '@agent-device/contracts/lease-scope'; import type { DaemonRequest } from '../../daemon/daemon-request.ts'; function lostLeaseError(reason: string): AppError { @@ -455,23 +454,6 @@ describe('runProtectedLeaseWork', () => { }); }); -describe('leaseScopeForHeartbeat', () => { - test('names no lease for a request that carries none', () => { - assert.equal(leaseScopeForHeartbeat({ flags: {}, meta: undefined }), undefined); - }); - - test('reads the lease scope from the request meta, then from the flags', () => { - assert.equal( - leaseScopeForHeartbeat({ meta: { leaseId: 'lease-meta', tenantId: 'acme' } })?.leaseId, - 'lease-meta', - ); - assert.equal( - leaseScopeForHeartbeat({ flags: { leaseId: 'lease-flag' } })?.leaseId, - 'lease-flag', - ); - }); -}); - describe('buildLeaseHeartbeatRequest', () => { test('carries the lease scope and nothing that belongs to the request it is protecting', () => { const beat = buildLeaseHeartbeatRequest( @@ -521,7 +503,7 @@ describe('buildLeaseHeartbeatRequest', () => { flags: { leaseId: 'lease-1', platform: 'android' }, meta: { leaseId: 'lease-1', tenantId: 'acme' }, }; - const scope = leaseScopeForHeartbeat(installRequest)!; + const scope = leaseScopeFromRequest(installRequest); const beat = buildLeaseHeartbeatRequest(scope, { session: 'default', requestId: 'beat-1', @@ -531,10 +513,10 @@ describe('buildLeaseHeartbeatRequest', () => { }); test('a caller that did name a ttl keeps renewing on it', () => { - const scope = leaseScopeForHeartbeat({ + const scope = leaseScopeFromRequest({ flags: { leaseId: 'lease-1' }, meta: { leaseId: 'lease-1', leaseTtlMs: 600_000 }, - })!; + }); const beat = buildLeaseHeartbeatRequest(scope, { session: 'default', requestId: 'beat-1', @@ -544,66 +526,6 @@ describe('buildLeaseHeartbeatRequest', () => { }); }); -describe('createLeaseRenewalBeat', () => { - const scope = { - leaseId: 'lease-1', - tenantId: 'acme', - runId: 'run-1', - leaseBackend: 'android-instance', - } as const; - - function beatContext(send: (request: DaemonRequest, budgetMs: number) => Promise) { - return { - session: 'adc-android', - sessionIsolation: 'tenant' as const, - token: 'daemon-token', - send, - }; - } - - test('sends one lease_heartbeat naming the lease it is protecting', async () => { - const sent: DaemonRequest[] = []; - await createLeaseRenewalBeat( - scope, - beatContext(async (request) => void sent.push(request)), - )(1_000); - - assert.equal(sent.length, 1); - assert.equal(sent[0]!.command, 'lease_heartbeat'); - assert.equal(sent[0]!.meta?.leaseId, 'lease-1'); - assert.equal(sent[0]!.meta?.tenantId, 'acme'); - assert.equal(sent[0]!.meta?.runId, 'run-1'); - assert.equal(sent[0]!.meta?.sessionIsolation, 'tenant'); - assert.equal(sent[0]!.session, 'adc-android'); - assert.equal(sent[0]!.token, 'daemon-token'); - }); - - test('every beat is a distinct request, so a timed-out beat cannot cancel the next one', async () => { - const sent: DaemonRequest[] = []; - const beat = createLeaseRenewalBeat( - scope, - beatContext(async (request) => void sent.push(request)), - ); - await beat(1_000); - await beat(1_000); - await beat(1_000); - - const ids = sent.map((request) => request.meta?.requestId); - assert.equal(new Set(ids).size, 3, 'three beats, three request ids'); - assert.ok(ids.every((id) => typeof id === 'string' && id.length > 0)); - }); - - test('a transport failure propagates to the caller that survives it', async () => { - const beat = createLeaseRenewalBeat( - scope, - beatContext(async () => { - throw new AppError('COMMAND_FAILED', 'connection reset'); - }), - ); - await assert.rejects((async () => await beat(1_000))(), /connection reset/); - }); -}); - describe('buildUploadLeaseHeartbeat', () => { const installRequest = { command: 'install', @@ -651,27 +573,37 @@ describe('buildUploadLeaseHeartbeat', () => { test('the beat reaches a remote daemon over its HTTP endpoint', async () => { const requests: { method?: string; path?: string; body: string }[] = []; + const connections: net.Socket[] = []; const server = net.createServer((socket) => { - let body = ''; + connections.push(socket); + // Answers every request the connection carries, and stays open: the beat is sent repeatedly + // and the transport keeps its socket, so closing after the first answer would surface the + // second beat as a reset rather than as a beat. + let buffered = ''; socket.on('data', (chunk) => { - body += chunk.toString('utf8'); - const headerEnd = body.indexOf('\r\n\r\n'); - if (headerEnd < 0) return; - const head = body.slice(0, headerEnd); - const [requestLine] = head.split('\r\n'); - const [method, path] = requestLine?.split(' ') ?? []; - requests.push({ method, path, body: body.slice(headerEnd + 4) }); - const payload = JSON.stringify({ - jsonrpc: '2.0', - id: 'x', - result: { ok: true }, - }); - socket.write( - `HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: ${String( - payload.length, - )}\r\n\r\n${payload}`, - ); - socket.end(); + buffered += chunk.toString('utf8'); + for (;;) { + const headerEnd = buffered.indexOf('\r\n\r\n'); + if (headerEnd < 0) return; + const head = buffered.slice(0, headerEnd); + const [requestLine] = head.split('\r\n'); + const [method, path] = requestLine?.split(' ') ?? []; + const declaredLength = Number(head.match(/content-length: (\d+)/i)?.[1] ?? '0'); + if (buffered.length < headerEnd + 4 + declaredLength) return; + const body = buffered.slice(headerEnd + 4, headerEnd + 4 + declaredLength); + buffered = buffered.slice(headerEnd + 4 + declaredLength); + requests.push({ method, path, body }); + const payload = JSON.stringify({ + jsonrpc: '2.0', + id: (JSON.parse(body) as { id: string }).id, + result: { ok: true }, + }); + socket.write( + `HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: ${String( + payload.length, + )}\r\n\r\n${payload}`, + ); + } }); }); const port = await listenOnLoopback(server); @@ -684,26 +616,34 @@ describe('buildUploadLeaseHeartbeat', () => { ); assert.ok(beat); await beat!(5_000); + // Two beats, because a beat that times out is canceled under its own id: sharing one would + // let a later beat inherit an earlier cancellation and stop renewing a live lease. + await beat!(5_000); } finally { + // The server keeps the connection open, and `close()` waits for it. + for (const connection of connections) connection.destroy(); await closeLoopbackServer(server); } - const posted = requests.find((request) => request.method === 'POST'); - assert.ok(posted, 'the beat POSTs to the remote daemon'); - const payload = JSON.parse(posted!.body) as { - method: string; - params: Record; - }; - assert.equal(posted!.path, '/agent-device/rpc'); - assert.equal(payload.method, 'agent_device.lease.heartbeat'); - assert.equal(payload.params.leaseId, 'lease-1'); - assert.equal(payload.params.tenantId, 'acme'); - assert.equal(payload.params.runId, 'run-1'); - assert.equal(payload.params.deviceKey, 'android:mobile:emulator-5554'); + const posted = requests.filter((request) => request.method === 'POST'); + assert.equal(posted.length, 2, 'every beat is its own request'); + const payloads = posted.map( + (request) => + JSON.parse(request.body) as { id: string; method: string; params: Record }, + ); + assert.equal(new Set(payloads.map((payload) => payload.id)).size, 2); + + const [first] = payloads; + assert.equal(posted[0]!.path, '/agent-device/rpc'); + assert.equal(first!.method, 'agent_device.lease.heartbeat'); + assert.equal(first!.params.leaseId, 'lease-1'); + assert.equal(first!.params.tenantId, 'acme'); + assert.equal(first!.params.runId, 'run-1'); + assert.equal(first!.params.deviceKey, 'android:mobile:emulator-5554'); // A beat asks for the same lease to keep going: it names no window, so the daemon renews the one // the lease already carries. - assert.equal('ttlMs' in payload.params, false); + assert.equal('ttlMs' in first!.params, false); }); test('a beat that never answers dies at its budget, not at the heartbeat policy', async () => { diff --git a/src/daemon-client/daemon-client-lease-beat.ts b/src/daemon-client/daemon-client-lease-beat.ts index 992edfaac7..f5e0d55ff3 100644 --- a/src/daemon-client/daemon-client-lease-beat.ts +++ b/src/daemon-client/daemon-client-lease-beat.ts @@ -221,35 +221,6 @@ async function captureOutcome(promise: Promise): Promise> { } } -/** - * The beat that keeps a remote lease alive across a long client-side phase. - * - * `send` is the caller's transport and `budgetMs` is how long this beat may take: the beat's answer - * only matters before the next one is due, so a stalled round trip is cut off at the cadence rather - * than at the command's own 90-second heartbeat policy. A beat is only ever sent to a remote daemon, - * where a transport timeout performs no local cleanup. - */ -export function createLeaseRenewalBeat( - leaseScope: LeaseScope, - context: Readonly<{ - session: string; - sessionIsolation?: NonNullable['sessionIsolation']; - token: string; - send: (request: DaemonRequest, budgetMs: number) => Promise; - }>, -): (budgetMs: number) => Promise { - return async (budgetMs) => - await context.send( - buildLeaseHeartbeatRequest(leaseScope, { - session: context.session, - sessionIsolation: context.sessionIsolation, - requestId: createRequestId(), - token: context.token, - }), - budgetMs, - ); -} - /** * The request one beat sends: the command's lease scope, its own id, and nothing else. * @@ -282,14 +253,6 @@ export function buildLeaseHeartbeatRequest( }; } -/** The lease a request is running under, when it names one. */ -export function leaseScopeForHeartbeat( - request: Pick, -): LeaseScope | undefined { - const scope = leaseScopeFromRequest(request); - return scope.leaseId ? scope : undefined; -} - /** * The beat that renews a remote lease across an artifact upload, or `undefined` when there is no * lease to protect: only a remote daemon uploads, so only one can be waiting on a billed device, and @@ -305,25 +268,25 @@ export function buildUploadLeaseHeartbeat( request: Omit, ): ((budgetMs: number) => Promise) | undefined { if (!isRemoteDaemon(info)) return undefined; - const leaseScope = leaseScopeForHeartbeat(request); - if (!leaseScope) return undefined; + const leaseScope = leaseScopeFromRequest(request); + if (!leaseScope.leaseId) return undefined; const policyTimeoutMs = resolveCommandRequestTimeoutMs( resolveCommandTimeoutPolicy(INTERNAL_COMMANDS.leaseHeartbeat), { positionals: [] }, ); - return createLeaseRenewalBeat(leaseScope, { - session: request.session, - sessionIsolation: request.meta?.sessionIsolation, - token: info.token, - send: async (beat, budgetMs) => - await sendRequest( - info, - beat, - settings.transportPreference, - settings.paths, - // The beat's own budget governs; the command's heartbeat policy only ever caps it, and an - // unbounded policy leaves the budget standing on its own. - policyTimeoutMs === undefined ? budgetMs : Math.min(policyTimeoutMs, budgetMs), - ), - }); + return async (budgetMs) => + await sendRequest( + info, + buildLeaseHeartbeatRequest(leaseScope, { + session: request.session, + sessionIsolation: request.meta?.sessionIsolation, + requestId: createRequestId(), + token: info.token, + }), + settings.transportPreference, + settings.paths, + // The beat's own budget governs; the command's heartbeat policy only ever caps it, and an + // unbounded policy leaves the budget standing on its own. + policyTimeoutMs === undefined ? budgetMs : Math.min(policyTimeoutMs, budgetMs), + ); } diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 5c39cb45c5..1b62f915d2 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -25,59 +25,47 @@ const LEASE_WINDOW_MS = 3_000; * none of them would notice the wiring between the three being dropped. */ -type FakeDaemon = { +type FakeDaemon = Readonly<{ baseUrl: string; /** Beats and commands the daemon saw, in arrival order. */ - seen: string[]; - uploadBytesDelivered(): number; + seen: readonly string[]; + /** Bytes of the artifact the daemon actually read. */ + uploadBytes(): number; /** - * Lets a stalled artifact through, and reports how the upload ended: `drained` if the whole - * artifact arrived, `canceled` if the request carrying it was destroyed first. - * - * The caller releases it rather than a beat answering, so the answer is causal: the client has - * already been told the lease is gone by then, and an upload it did not cancel has nothing left - * that could stop it. + * Stops withholding the artifact and reports how the upload ended. The caller releases it after + * `sendToDaemon` settles, so the answer is causal rather than timed: an upload the client did not + * cancel has nothing left that could stop it by then, and one it did cannot arrive. */ - releaseUploadAndObserveOutcome(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; + releaseUpload(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; close(): Promise; -}; - -type UploadBehaviour = 'complete' | 'backpressure'; +}>; /** - * A remote daemon that only answers a beat, and treats the upload as the long phase it is: + * A remote daemon that answers a beat and treats the upload as the long phase it is. * - * - `complete` drains the artifact and withholds the upload response until a second beat has - * arrived, so "the lease was renewed while the artifact was still uploading" is a fact of the - * test rather than a race it happens to win. - * - `backpressure` stops reading after the first chunk, so the artifact is still in flight when the - * next beat reports the lease gone — the state the abort exists for. + * `stallUpload` decides how the artifact is held: withheld while it drains until a second beat has + * landed, or stopped in flight after the first chunk. Without that hold, "the lease was renewed + * during the upload" would be a race the test happens to win rather than something it establishes. */ -async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise { +async function startFakeRemoteDaemon(stallUpload: boolean): Promise { const seen: string[] = []; - let uploadBytesDelivered = 0; + let uploadBytes = 0; + let beatsAnswered = 0; + let leaseDeclaredLost = false; + let resumeUpload: (() => void) | undefined; let resolveOutcome!: (outcome: 'drained' | 'canceled') => void; - const settled = new Promise<'drained' | 'canceled'>((resolve) => { + const outcome = new Promise<'drained' | 'canceled'>((resolve) => { resolveOutcome = resolve; }); - let stopBackpressure: (() => void) | undefined; - let beatsAnswered = 0; - let leaseDeclaredLost = false; - let writeUploadResponse: (() => void) | undefined; const server = http.createServer((req, res) => { if (req.method === 'GET' && (req.url ?? '').startsWith('/health')) { writeJson(res, 200, { ok: true }); return; } - if (req.method !== 'POST') { - res.writeHead(404); - res.end('not found'); - return; - } if (req.url === '/upload/preflight') { - // Draining before the 404 keeps the connection usable; an early end would make the client's - // fallback to the legacy upload route a matter of socket recycling rather than the protocol. + // Drained before the 404, so the fallback to the legacy upload route is the protocol's doing + // and not a matter of socket recycling. readJsonBody(req, () => { res.writeHead(404); res.end('not found'); @@ -85,27 +73,10 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise { - uploadBytesDelivered += length; - }, - onBodyArrived: () => { - resolveOutcome('drained'); - }, - onCanceled: () => { - resolveOutcome('canceled'); - }, - holdResponse: (write) => { - writeUploadResponse = write; - if (!leaseDeclaredLost && beatsAnswered >= 2) write(); - }, - releaseBackpressure: (resume) => { - stopBackpressure = resume; - }, - }); + handleUpload(req, res); return; } - if (req.url !== '/rpc') { + if (req.method !== 'POST' || req.url !== '/rpc') { res.writeHead(404); res.end('not found'); return; @@ -124,18 +95,54 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise { + if (answered || !bodyArrived || leaseDeclaredLost || beatsAnswered < 2) return; + answered = true; + writeJson(res, 200, { ok: true, uploadId: 'upload-demo.apk' }); + }; + resumeUpload = () => { + if (stalled) req.resume(); + answerUpload(); + }; + req.on('data', (chunk: Buffer) => { + uploadBytes += chunk.length; + // Pausing once, not per chunk: releasing the pressure has to let the artifact through, or a + // stalled upload and a canceled one are the same observation from here. + if (stallUpload && !stalled) { + req.pause(); + stalled = true; + } + }); + req.on('end', () => { + bodyArrived = true; + resolveOutcome('drained'); + answerUpload(); + }); + req.on('aborted', () => { + if (!answered) resolveOutcome('canceled'); + }); + res.on('close', () => { + if (!answered) resolveOutcome('canceled'); + }); + } + function answerBeat(res: http.ServerResponse, payload: RpcPayload): void { beatsAnswered += 1; seen.push('lease_heartbeat'); assert.equal(payload.params?.leaseId, LEASE_ID, 'a beat names the lease it protects'); // Only a beat after the first can say anything about the upload: the loop fires one at t=0, // while the artifact is still being hashed. - const leaseGone = behaviour === 'backpressure' && beatsAnswered >= 2; - if (leaseGone) { + if (stallUpload && beatsAnswered >= 2) { leaseDeclaredLost = true; writeLeaseLostError(res, payload.id); - // A beat that reports the lease gone must not also complete the upload it is meant to stop: - // the artifact's fate is decided by the abort, not by a response from here. return; } const now = Date.now(); @@ -144,7 +151,7 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise uploadBytesDelivered, - async releaseUploadAndObserveOutcome(graceMs = 2_000) { - stopBackpressure?.(); + uploadBytes: () => uploadBytes, + async releaseUpload(graceMs = 2_000) { + resumeUpload?.(); return await Promise.race([ - settled, + outcome, new Promise<'unresolved'>((resolve) => { setTimeout(() => resolve('unresolved'), graceMs).unref(); }), ]); }, - async close() { + close: async () => { server.closeAllConnections(); await new Promise((resolve) => { server.close(() => resolve()); @@ -171,51 +178,6 @@ async function startFakeRemoteDaemon(behaviour: UploadBehaviour): Promise void): void; - releaseBackpressure(resume: () => void): void; - }>, -): void { - let answered = false; - const respond = (): void => { - if (answered) return; - answered = true; - writeJson(res, 200, { ok: true, uploadId: 'upload-demo.apk' }); - }; - const requestSettled = (): void => { - if (answered) return; - hooks.onCanceled(); - }; - req.on('aborted', requestSettled); - res.on('close', requestSettled); - let stalled = false; - req.on('data', (chunk: Buffer) => { - hooks.onBytes(chunk.length); - // Stopping the read applies backpressure, so the artifact stays in flight instead of racing - // through loopback and making "stopped early" a matter of timing. - // Pausing once, rather than on every chunk: releasing the pressure has to actually let the - // artifact through, otherwise a stalled upload and a canceled one are the same observation. - if (behaviour === 'backpressure' && !stalled) { - req.pause(); - stalled = true; - } - }); - req.on('end', () => { - hooks.onBodyArrived(); - hooks.holdResponse(respond); - }); - hooks.releaseBackpressure(() => { - if (stalled) req.resume(); - }); -} - type RpcPayload = Readonly<{ id: unknown; method: string; params?: Record }>; function readJsonBody(req: http.IncomingMessage, done: (payload: RpcPayload) => void): void { @@ -250,11 +212,8 @@ function writeLeaseLostError(res: http.ServerResponse, id: unknown): void { }); } -function installRequest( - baseUrl: string, - apkPath: string, - stateDir: string, -): Omit { +/** An install of `apkPath` against a remote daemon, under the lease the beat has to renew. */ +function installRequest(baseUrl: string, apkPath: string): Omit { return { session: 'upload-beat', command: 'install', @@ -262,7 +221,7 @@ function installRequest( flags: { platform: 'android', daemonBaseUrl: baseUrl, - stateDir, + stateDir: path.dirname(apkPath), leaseId: LEASE_ID, tenant: 'acme', runId: 'run-1', @@ -276,16 +235,16 @@ function installRequest( const TRANSPORT = { authToken: TOKEN } as const; -async function withUploadFixture( +async function withUploadedArtifact( t: { skip(reason?: string): void }, - behaviour: UploadBehaviour, + stallUpload: boolean, run: (daemon: FakeDaemon, apkPath: string) => Promise, ): Promise { if (await skipWhenLoopbackUnavailable(t)) return undefined; const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-upload-beat-')); const apkPath = path.join(dir, 'demo.apk'); fs.writeFileSync(apkPath, Buffer.alloc(APK_BYTES, 'x')); - const daemon = await startFakeRemoteDaemon(behaviour); + const daemon = await startFakeRemoteDaemon(stallUpload); try { return await run(daemon, apkPath); } finally { @@ -295,53 +254,32 @@ async function withUploadFixture( } test('an install beats the lease while its artifact uploads, before the install RPC', async (t) => { - await withUploadFixture(t, 'complete', async (daemon, apkPath) => { - const response = await sendToDaemon( - installRequest(daemon.baseUrl, apkPath, path.dirname(apkPath)), - TRANSPORT, - ); + await withUploadedArtifact(t, false, async (daemon, apkPath) => { + const response = await sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); assert.equal(response.ok, true); - assert.ok( - daemon.seen.filter((entry) => entry === 'lease_heartbeat').length >= 2, - `a beat has to land while the upload is held open, daemon saw: ${daemon.seen.join(', ')}`, - ); - assert.equal( - daemon.uploadBytesDelivered(), - APK_BYTES, - 'the artifact arrived whole on a lease that was being renewed under it', - ); + assert.equal(daemon.uploadBytes(), APK_BYTES, 'the artifact arrived whole'); + // The response was withheld until the second beat, so that beat proves the lease was renewed + // while the install was still mid-flight. assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); }); }); test('a lease lost mid-upload aborts the upload and no install request goes out', async (t) => { - await withUploadFixture(t, 'backpressure', async (daemon, apkPath) => { + await withUploadedArtifact(t, true, async (daemon, apkPath) => { await assert.rejects( - async () => - await sendToDaemon( - installRequest(daemon.baseUrl, apkPath, path.dirname(apkPath)), - TRANSPORT, - ), + async () => await sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT), (error: unknown) => error instanceof AppError && error.code === 'UNAUTHORIZED' && error.details?.reason === 'LEASE_NOT_FOUND', ); - assert.ok( - daemon.uploadBytesDelivered() > 0 && daemon.uploadBytesDelivered() < APK_BYTES, - `bytes were in flight and the daemon stopped reading at ${String( - daemon.uploadBytesDelivered(), - )} of ${String(APK_BYTES)}, which is what had to be stopped`, - ); - // The daemon never read past the first chunk, so this is the only way the artifact can stop - // short: the client destroyed the request. An upload nobody canceled drains once the pressure - // comes off, and by now the client has long since been told the lease is gone. + assert.ok(daemon.uploadBytes() < APK_BYTES, 'the artifact stopped short of the end'); assert.equal( - await daemon.releaseUploadAndObserveOutcome(), + await daemon.releaseUpload(), 'canceled', - 'the upload was stopped, not left to finish on a lease nobody held', + 'the request was destroyed, not left to finish on a lease nobody held', ); assert.ok( !daemon.seen.includes('install'), From b391a4af553ad8305e8bfc6cc13c933f724e67d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 11:54:42 +0200 Subject: [PATCH 16/21] fix(remote): budget each beat from the window it protects, not the cadence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Budgeting a beat at the cadence it started on made one number do two jobs. Until a beat answers, the cadence sits at the 1s floor, so `sendRequest` got a 1s budget, destroyed the request, and emitted a `daemon_request_timeout`. That answer is non-terminal, so the cadence never left the floor: on any link where a round trip takes over a second — a tunneled proxy, a fresh TLS connect per beat because every timeout tore the socket down, an uplink the upload was saturating — no beat ever got a window in which to answer, and the lease could lapse on exactly the slow uploads #2946 exists to protect, with a diagnostic pair every second. Cadence and budget are two numbers now. A beat is armed every third of the window and allowed the whole window to answer in, so a slow first answer sets the cadence instead of being cut off before it can say anything. The window the loop assumes before the first answer is the registry's five-second minimum, which is now shared with the client as `MIN_LEASE_WINDOW_MS` rather than assumed twice: planning against a longer window than the daemon would ever grant is the same mistake pointed the other way. The slow-link case is covered through `sendToDaemon`, which no loopback unit test travels: the fake daemon answers a beat after 3s — longer than the assumed cadence, shorter than the assumed window, so a beat budgeted at the cadence is cut off and one budgeted at the window is not. It asserts the beat stops arriving once the window is known and counts no `daemon_request_timeout` inside the upload, in a diagnostics scope because outside one every count reads zero for the wrong reason. Verified against the previous beat loop: five arrivals there, none of them answered in time. --- docs/adr/0007-remote-device-leases.md | 11 +- packages/contracts/src/lease-scope.ts | 9 ++ .../daemon-client-lease-beat.test.ts | 27 ++-- src/daemon-client/daemon-client-lease-beat.ts | 56 +++++--- src/daemon/lease-registry-scope.ts | 4 +- .../remote-upload-lease-beat.test.ts | 125 +++++++++++++++--- 6 files changed, 179 insertions(+), 53 deletions(-) diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index eaf4b8a6ab..648bd554df 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -89,9 +89,14 @@ phase begins: hashing, the preflight, and the start of the stream can all run wh still outstanding, so what the first beat buys is that the loss is learned during the upload rather than after it. Each beat answers with the window it just renewed, and the loop arms its successor when the beat starts rather than when it settles, so a beat that never answers is abandoned on schedule -instead of taking the schedule with it; the beat's own budget is capped at the cadence it started on, -which is what keeps one stalled round trip from outliving the window it exists to protect. A phase -that settles does not wait on a beat still in flight. +instead of taking the schedule with it. A phase that settles does not wait on a beat still in flight. + +Cadence and budget are deliberately two numbers. A beat is armed every third of the window, but it is +allowed the whole window to answer in. Budgeting a beat at its cadence instead would give a heartbeat +that needs more than a third of a window for its round trip — a tunneled proxy, an uplink the upload +itself is saturating — no way to answer at all, and every beat would re-time-out on exactly the slow +links the beat exists to protect. The window, not the cadence, is the interval in which being alive +still matters, so it is also the longest a beat's answer is worth waiting for. A beat is a fresh request each time, never the protected request rewritten: a request identity is what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat diff --git a/packages/contracts/src/lease-scope.ts b/packages/contracts/src/lease-scope.ts index d03b7cf6d7..51bb05550c 100644 --- a/packages/contracts/src/lease-scope.ts +++ b/packages/contracts/src/lease-scope.ts @@ -295,6 +295,15 @@ export function findMissingProxyLeaseFields(scope: LeaseScope): string[] { return REQUIRED_PROXY_LEASE_FIELDS.filter((field) => !scope[field]); } +/** + * The shortest inactivity window a lease can hold by default. + * + * The registry clamps every ttl to this floor, so it is also the worst case a client plans against + * before it has seen a lease answer: work that has to finish inside a lease window it does not know + * yet must assume this one, or it can be planning for a window the daemon would never grant. + */ +export const MIN_LEASE_WINDOW_MS = 5_000; + /** * Why a lease stopped being ours: it is gone, spent, or taken back. * diff --git a/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts index 2039d9af48..858feae913 100644 --- a/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts +++ b/src/daemon-client/__tests__/daemon-client-lease-beat.test.ts @@ -133,17 +133,23 @@ describe('runProtectedLeaseWork', () => { heartbeat, }); - await vi.advanceTimersByTimeAsync(1_000); + // No window named, so the loop stays on the cadence it assumed: a third of the shortest window + // the daemon will accept. + await vi.advanceTimersByTimeAsync(1_666); assert.equal(heartbeat.mock.calls.length, 2, 'immediate first, then the same cadence again'); - await vi.advanceTimersByTimeAsync(1_000); + await vi.advanceTimersByTimeAsync(1_666); assert.equal(heartbeat.mock.calls.length, 3); upload.resolve('installed'); assert.equal(await running, 'installed'); }); - test('each beat is given a budget no longer than the cadence it starts on', async () => { + test('each beat is given the window it protects as its budget, not the cadence', async () => { vi.useFakeTimers(); + // Cadence and budget are separate numbers. A beat is armed every third of the window, but it is + // allowed the whole window to answer in: budgeting it at the cadence would leave a heartbeat + // that needs more than a third of a window for its round trip with no chance to answer, on + // exactly the slow links the beat exists to protect. const budgets: number[] = []; const heartbeat = vi.fn(async (budgetMs: number) => { budgets.push(budgetMs); @@ -156,11 +162,9 @@ describe('runProtectedLeaseWork', () => { }); await vi.advanceTimersByTimeAsync(40_000); - // Before the first answer the loop assumes the shortest window the daemon accepts, so the beat - // that has to prove that lease is alive cannot itself take longer than a fifth of it. Once the - // window is known the budget is the cadence: a stalled beat dies inside one, not the 90s the - // command's own heartbeat policy would allow. - assert.deepEqual(budgets, [1_000, 20_000, 20_000]); + // Before the first answer the loop assumes the shortest window the daemon will accept; once the + // lease names its own, the budget is that window, not the 90s the heartbeat policy would allow. + assert.deepEqual(budgets, [5_000, 60_000, 60_000]); upload.resolve('installed'); assert.equal(await running, 'installed'); @@ -215,7 +219,7 @@ describe('runProtectedLeaseWork', () => { heartbeat, }); - await vi.advanceTimersByTimeAsync(3_000); + await vi.advanceTimersByTimeAsync(5_000); assert.equal(heartbeat.mock.calls.length, 4, 'the stalled beats were abandoned, not awaited'); upload.resolve('installed'); @@ -253,11 +257,12 @@ describe('runProtectedLeaseWork', () => { heartbeat, }); - await vi.advanceTimersByTimeAsync(2_500); + await vi.advanceTimersByTimeAsync(3_332); assert.equal(heartbeat.mock.calls.length, 3, 'one failed beat does not stop the others'); heartbeat.mockImplementation(async () => renewedLeaseResponse(30_000)); - await vi.advanceTimersByTimeAsync(1_000); + await vi.advanceTimersByTimeAsync(10_000); + assert.equal(heartbeat.mock.calls.length, 4, 'the window the beat reported sets the cadence'); upload.resolve('installed'); assert.equal(await running, 'installed'); }); diff --git a/src/daemon-client/daemon-client-lease-beat.ts b/src/daemon-client/daemon-client-lease-beat.ts index f5e0d55ff3..457bb51805 100644 --- a/src/daemon-client/daemon-client-lease-beat.ts +++ b/src/daemon-client/daemon-client-lease-beat.ts @@ -5,6 +5,7 @@ import { INTERNAL_COMMANDS } from '@agent-device/command-registry/catalog'; import { resolveCommandTimeoutPolicy } from '@agent-device/command-registry/registry'; import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/timeout-policy'; import { + MIN_LEASE_WINDOW_MS, isInactiveLeaseError, leaseScopeFromRequest, leaseScopeToRequestMeta, @@ -24,14 +25,12 @@ import { sendRequest } from './daemon-client-transport.ts'; // knows nothing about transports beyond the `send` it is handed. /** - * The fastest cadence a phase beats at, and the budget each beat gets until the window is known. + * The fastest cadence a phase beats at. * - * Before the first beat answers, the window is unknown and the worst legal case is the registry's - * five-second minimum: a beat that stalls must be abandoned and retried inside that window, or the - * thing the beat exists to prevent happens while it waits. One second is a fifth of that minimum, - * and no window-derived cadence is ever allowed below it, so it is also the loop's floor: a - * misreported or pathologically short window cannot turn the beat into a request loop faster than - * this. + * A cadence is a third of a window, and a window can be as short as the registry's five-second + * minimum, so without a floor a short lease would turn the beat into a request loop. One second is + * a fifth of that minimum: it keeps the shortest legal window beaten five times over, and no + * window-derived cadence is ever allowed below it. */ const MIN_LEASE_BEAT_INTERVAL_MS = 1_000; @@ -82,12 +81,16 @@ function isTerminalLeaseBeatError(error: unknown): boolean { * and the first bytes can all happen while the opening beat is still outstanding. Each beat answers * with the window it just renewed, and the cadence becomes a third of that window. * - * A beat's budget is the cadence it started on, and its successor is armed when the beat starts - * rather than when it settles. A beat that never returns — a half-open connection, a daemon wedged - * before it admits anything — is therefore abandoned on schedule instead of holding the schedule: - * the lease is still beaten at window/3, and the abandoned round trip is cut off by its own budget - * in the transport rather than by the command's 90-second heartbeat policy. An abandoned beat is - * still listened to, because the answer it eventually gives can be a lost lease. + * The cadence and the budget are two numbers, and only one of them is the cadence. A beat is armed + * every third of the window, and its budget is the whole window it protects: a heartbeat that needs + * more than a cadence to come back — a tunneled proxy, a slow uplink the upload itself is + * saturating — still answers inside the lease it is renewing, and the cadence it proves becomes the + * loop's. Budgeting a beat at the cadence instead would hand a slow link no window in which to + * answer at all and re-time-out every beat forever. Successors are armed when a beat starts rather + * than when it settles, so a beat that never returns at all is abandoned on schedule instead of + * taking the schedule with it. A stalled beat is never awaited, so the outstanding ones are bounded + * by the budget divided by the cadence rather than by the phase's length. An abandoned beat is still + * listened to, because the answer it eventually gives can be a lost lease. * * A beat that fails for a reason that says nothing about this lease is reported and survived — one * lost request must not fail an upload that a later beat will cover. A beat that finds the lease @@ -98,8 +101,10 @@ function isTerminalLeaseBeatError(error: unknown): boolean { export async function runProtectedLeaseWork( options: Readonly<{ /** - * One renewal. `budgetMs` is how long this beat may take before the loop abandons it. Absent - * when the request names no lease to renew, which is the ordinary unleased install. + * One renewal. `budgetMs` is how long this beat may take before the loop abandons it: the window + * the beat protects, so a slow round trip still gets a chance to answer inside the lease it is + * renewing. Absent when the request names no lease to renew, which is the ordinary unleased + * install. */ heartbeat?: ((budgetMs: number) => Promise) | undefined; task: (signal: AbortSignal) => Promise; @@ -109,9 +114,10 @@ export async function runProtectedLeaseWork( if (!heartbeat) return await options.task(new AbortController().signal); const control = new AbortController(); - // Until a beat names the window, the loop assumes the shortest window the daemon will accept: the - // budget of the beat that has to prove a short lease is alive cannot itself be longer than it. - let intervalMs = MIN_LEASE_BEAT_INTERVAL_MS; + // Until a beat names the window, the loop assumes the shortest window the daemon will accept: a + // beat that budgets itself on a longer window than the lease actually has would outlive it. + let windowMs = MIN_LEASE_WINDOW_MS; + let intervalMs = leaseBeatIntervalMs(windowMs); let timer: ReturnType | undefined; let stopped = false; let terminalError: unknown; @@ -130,14 +136,15 @@ export async function runProtectedLeaseWork( }; arm(intervalMs); const settle = (async () => { - const budgetMs = intervalMs; + const budgetMs = windowMs; try { const renewed = leaseWindowFromHeartbeatResponse(await heartbeat(budgetMs)); // An answer that names no window keeps the cadence it was asked at: the loop only ever - // slows down on evidence of how long the lease is good for, and never on the absence of it. + // moves on evidence of how long the lease is good for, and never on the absence of it. if (renewed === undefined) return; - const cadence = Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(renewed / 3)); - if (cadence === intervalMs) return; + const cadence = leaseBeatIntervalMs(renewed); + if (renewed === windowMs) return; + windowMs = renewed; intervalMs = cadence; // The window just moved, so the next beat is due one cadence from this answer. arm(cadence); @@ -201,6 +208,11 @@ export async function runProtectedLeaseWork( * extended — the same pair `leaseOwnTtlMs` renews on. Anything unrecognizable leaves the caller on * the fallback cadence rather than guessing one. */ +/** The cadence a window of `windowMs` beats at: a third of it, never faster than the floor. */ +function leaseBeatIntervalMs(windowMs: number): number { + return Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(windowMs / 3)); +} + function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined { const lease = ( response as Readonly<{ data?: Readonly<{ lease?: Readonly> }> }> diff --git a/src/daemon/lease-registry-scope.ts b/src/daemon/lease-registry-scope.ts index a228a9ec7f..fd8cdb285e 100644 --- a/src/daemon/lease-registry-scope.ts +++ b/src/daemon/lease-registry-scope.ts @@ -1,5 +1,6 @@ import crypto from 'node:crypto'; import type { DeviceLease } from '@agent-device/contracts/device'; +import { MIN_LEASE_WINDOW_MS } from '@agent-device/contracts/lease-scope'; import type { LeaseBackend } from '@agent-device/kernel/contracts'; import { AppError } from '@agent-device/kernel/errors'; import { normalizeTenantId } from './config.ts'; @@ -84,7 +85,6 @@ export type NormalizedAllocateLeaseRequest = { }; const DEFAULT_LEASE_TTL_MS = 60_000; -const MIN_LEASE_TTL_MS = 5_000; const MAX_LEASE_TTL_MS = 10 * 60_000; const DEFAULT_LEASE_PROVIDER = 'default'; @@ -94,7 +94,7 @@ export function createLeaseTtlResolver(options: LeaseRegistryOptions) { : DEFAULT_LEASE_TTL_MS; const minTtl = Number.isInteger(options.minLeaseTtlMs) ? Math.max(1, Number(options.minLeaseTtlMs)) - : MIN_LEASE_TTL_MS; + : MIN_LEASE_WINDOW_MS; const maxTtl = Number.isInteger(options.maxLeaseTtlMs) ? Math.max(minTtl, Number(options.maxLeaseTtlMs)) : MAX_LEASE_TTL_MS; diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 1b62f915d2..2a76f3133f 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -5,6 +5,10 @@ import os from 'node:os'; import path from 'node:path'; import { test } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; +import { + countDiagnosticEventsByPhase, + withDiagnosticsScope, +} from '@agent-device/host-kit/diagnostics'; import { sendToDaemon } from '../../../src/daemon-client/daemon-client.ts'; import type { DaemonRequest } from '../../../src/daemon/daemon-request.ts'; import { @@ -17,6 +21,29 @@ const TOKEN = 'upload-beat-token'; const APK_BYTES = 4 * 1024 * 1024; /** Renewed window the fake daemon reports; a third of it is the cadence the client beats on. */ const LEASE_WINDOW_MS = 3_000; +/** + * A heartbeat round trip on a slow link. + * + * The number has to land between the two budgets the loop can be holding a beat to: longer than the + * cadence it assumes before a window is known (a third of the registry's five-second minimum), and + * shorter than that assumed window itself. Anything shorter and a beat budgeted at the cadence still + * answers in time, so the test would pass on a budget sized to the wrong thing. + */ +const SLOW_BEAT_MS = 3_000; +/** The window the fake daemon reports on the slow-link case, whose third is its cadence. */ +const SLOW_BEAT_LEASE_WINDOW_MS = 30_000; +/** How long the slow-link case watches the beat before letting the artifact finish. */ +const SLOW_BEAT_OBSERVATION_MS = 4_500; + +/** + * How the fake daemon makes the upload the long phase it is. + * + * - `renewed`: withhold the artifact until a second beat has renewed the lease mid-upload. + * - `lost`: stop the artifact in flight and have the second beat report the lease gone. + * - `slow`: answer every beat late, which is what tells a budget sized to the window from one sized + * to the cadence. + */ +type UploadMode = 'renewed' | 'lost' | 'slow'; /** * #2946's route end to end: `sendToDaemon` uploads an artifact for a remote install before the @@ -31,6 +58,8 @@ type FakeDaemon = Readonly<{ seen: readonly string[]; /** Bytes of the artifact the daemon actually read. */ uploadBytes(): number; + /** When each beat reached the daemon, as `process.hrtime.bigint()` readings. */ + readonly beatArrivals: readonly bigint[]; /** * Stops withholding the artifact and reports how the upload ended. The caller releases it after * `sendToDaemon` settles, so the answer is causal rather than timed: an upload the client did not @@ -47,10 +76,11 @@ type FakeDaemon = Readonly<{ * landed, or stopped in flight after the first chunk. Without that hold, "the lease was renewed * during the upload" would be a race the test happens to win rather than something it establishes. */ -async function startFakeRemoteDaemon(stallUpload: boolean): Promise { +async function startFakeRemoteDaemon(mode: UploadMode): Promise { const seen: string[] = []; let uploadBytes = 0; let beatsAnswered = 0; + const beatArrivals: bigint[] = []; let leaseDeclaredLost = false; let resumeUpload: (() => void) | undefined; let resolveOutcome!: (outcome: 'drained' | 'canceled') => void; @@ -99,12 +129,16 @@ async function startFakeRemoteDaemon(stallUpload: boolean): Promise let answered = false; let bodyArrived = false; let stalled = false; + // The slow-link case holds the artifact against one beat: a beat answering there only proves the + // lease was renewed, and must not also be what ends the phase being measured. + const holdUntilBeats = mode === 'slow' ? 1 : 2; // The artifact's response is withheld until a beat has renewed the lease mid-upload, so the // first case proves the renewal landed while the install was still in flight. A beat that // reports the lease gone must not also complete the upload it exists to stop: that artifact's // fate belongs to the abort, not to a response from here. const answerUpload = (): void => { - if (answered || !bodyArrived || leaseDeclaredLost || beatsAnswered < 2) return; + if (answered || !bodyArrived || leaseDeclaredLost) return; + if (beatsAnswered < holdUntilBeats) return; answered = true; writeJson(res, 200, { ok: true, uploadId: 'upload-demo.apk' }); }; @@ -112,11 +146,12 @@ async function startFakeRemoteDaemon(stallUpload: boolean): Promise if (stalled) req.resume(); answerUpload(); }; + releaseArtifact = resumeUpload; req.on('data', (chunk: Buffer) => { uploadBytes += chunk.length; // Pausing once, not per chunk: releasing the pressure has to let the artifact through, or a // stalled upload and a canceled one are the same observation from here. - if (stallUpload && !stalled) { + if (!stalled) { req.pause(); stalled = true; } @@ -136,22 +171,35 @@ async function startFakeRemoteDaemon(stallUpload: boolean): Promise function answerBeat(res: http.ServerResponse, payload: RpcPayload): void { beatsAnswered += 1; + beatArrivals.push(process.hrtime.bigint()); seen.push('lease_heartbeat'); assert.equal(payload.params?.leaseId, LEASE_ID, 'a beat names the lease it protects'); // Only a beat after the first can say anything about the upload: the loop fires one at t=0, // while the artifact is still being hashed. - if (stallUpload && beatsAnswered >= 2) { + if (mode === 'lost' && beatsAnswered >= 2) { leaseDeclaredLost = true; writeLeaseLostError(res, payload.id); return; } - const now = Date.now(); - writeJson(res, 200, { - jsonrpc: '2.0', - id: payload.id, - result: { ok: true, data: { lease: { heartbeatAt: now, expiresAt: now + LEASE_WINDOW_MS } } }, - }); - resumeUpload?.(); + const windowMs = mode === 'slow' ? SLOW_BEAT_LEASE_WINDOW_MS : LEASE_WINDOW_MS; + writeBeatAnswer(res, payload.id, windowMs); + if (mode !== 'slow') resumeUpload?.(); + } + + function writeBeatAnswer(res: http.ServerResponse, id: unknown, windowMs: number): void { + const answer = (): void => { + const now = Date.now(); + writeJson(res, 200, { + jsonrpc: '2.0', + id, + result: { ok: true, data: { lease: { heartbeatAt: now, expiresAt: now + windowMs } } }, + }); + }; + if (mode !== 'slow') { + answer(); + return; + } + setTimeout(answer, SLOW_BEAT_MS).unref(); } server.keepAliveTimeout = 100; @@ -160,6 +208,7 @@ async function startFakeRemoteDaemon(stallUpload: boolean): Promise baseUrl: `http://127.0.0.1:${String(port)}`, seen, uploadBytes: () => uploadBytes, + beatArrivals, async releaseUpload(graceMs = 2_000) { resumeUpload?.(); return await Promise.race([ @@ -191,6 +240,11 @@ function readJsonBody(req: http.IncomingMessage, done: (payload: RpcPayload) => }); } +/** Lets a withheld artifact finish, for the case that measures the phase before ending it. */ +function resumeUploadNow(): void { + releaseArtifact(); +} + function writeJson(res: http.ServerResponse, statusCode: number, payload: unknown): void { res.writeHead(statusCode, { 'content-type': 'application/json' }); res.end(JSON.stringify(payload)); @@ -235,16 +289,18 @@ function installRequest(baseUrl: string, apkPath: string): Omit void; + async function withUploadedArtifact( t: { skip(reason?: string): void }, - stallUpload: boolean, + mode: UploadMode, run: (daemon: FakeDaemon, apkPath: string) => Promise, ): Promise { if (await skipWhenLoopbackUnavailable(t)) return undefined; const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-upload-beat-')); const apkPath = path.join(dir, 'demo.apk'); fs.writeFileSync(apkPath, Buffer.alloc(APK_BYTES, 'x')); - const daemon = await startFakeRemoteDaemon(stallUpload); + const daemon = await startFakeRemoteDaemon(mode); try { return await run(daemon, apkPath); } finally { @@ -254,7 +310,7 @@ async function withUploadedArtifact( } test('an install beats the lease while its artifact uploads, before the install RPC', async (t) => { - await withUploadedArtifact(t, false, async (daemon, apkPath) => { + await withUploadedArtifact(t, 'renewed', async (daemon, apkPath) => { const response = await sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); assert.equal(response.ok, true); @@ -266,7 +322,7 @@ test('an install beats the lease while its artifact uploads, before the install }); test('a lease lost mid-upload aborts the upload and no install request goes out', async (t) => { - await withUploadedArtifact(t, true, async (daemon, apkPath) => { + await withUploadedArtifact(t, 'lost', async (daemon, apkPath) => { await assert.rejects( async () => await sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT), (error: unknown) => @@ -287,3 +343,42 @@ test('a lease lost mid-upload aborts the upload and no install request goes out' ); }); }); + +test('a beat that takes over a second to answer sets the cadence instead of timing out', async (t) => { + await withUploadedArtifact(t, 'slow', async (daemon, apkPath) => { + let timeoutsDuringUpload = -1; + let beatsDuringUpload = -1; + let response: Awaited> | undefined; + + // The request has to run inside a diagnostics scope for the absence of a timeout to mean + // anything: outside one, `emitDiagnostic` records nothing at all and every count reads zero. + await withDiagnosticsScope({ session: 'upload-beat', command: 'install' }, async () => { + const running = sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); + await new Promise((resolve) => { + setTimeout(resolve, SLOW_BEAT_OBSERVATION_MS).unref(); + }); + timeoutsDuringUpload = countDiagnosticEventsByPhase(['daemon_request_timeout']); + beatsDuringUpload = daemon.beatArrivals.length; + // Only now does the artifact get the rest of its way in, so everything above was measured + // while the upload was genuinely still running. + resumeUploadNow(); + response = await running; + }); + + assert.equal(response!.ok, true); + // Two beats, not three: the opening one, and the successor the loop had already armed at the + // cadence it assumes before any window is known. The first beat's late answer then moves the + // loop to a third of the 30s window it just renewed, so nothing else is due inside the + // observation. A beat budgeted at its cadence is cut off before that answer lands, never learns + // the window, and keeps arriving every assumed cadence with a `daemon_request_timeout` behind it + // — which is #2946's slow link wearing the beat down instead of protecting it. + assert.equal(beatsDuringUpload, 2, `beats seen: ${String(beatsDuringUpload)}`); + assert.equal( + timeoutsDuringUpload, + 0, + 'a heartbeat the transport cut off is a timeout, not a slow answer', + ); + + assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); + }); +}); From 4c74a43bfbe8f3d86bcd8eec3bdf46a068f3c8cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 11:54:47 +0200 Subject: [PATCH 17/21] fix(remote): load the lease beat only for the request that can use it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The extraction that moved the beat out of daemon-client.ts put it in cli.ts's eager closure, which every command pays for, and took it from 295 modules to 296 (#2990's coverage failure). The beat is only ever useful to one kind of request: one heading to a remote daemon that names a lease. Both guards are cheap and local — the lease scope comes off the request the caller already built — so the module now loads behind them and the closure is back to 295. Also folds the beat's thin wrappers away from the entry surface, which is what the four test-only exports in daemon-client.ts had become: createLeaseRenewalBeat and leaseScopeForHeartbeat existed only for tests, the scope precedence they pinned is already covered where it lives in contracts, and the one invariant that wasn't covered anywhere — a beat per request id, so a timed-out beat cannot cancel its successor — is now pinned over the real transport, where a shared id is actually observable. --- src/daemon-client/daemon-client.ts | 43 +++++++++++++++++++++++++----- src/remote/daemon-artifacts.ts | 2 +- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/daemon-client/daemon-client.ts b/src/daemon-client/daemon-client.ts index 13610507eb..ae08c62a15 100644 --- a/src/daemon-client/daemon-client.ts +++ b/src/daemon-client/daemon-client.ts @@ -13,7 +13,10 @@ import { import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '@agent-device/command-registry/catalog'; import { resolveCommandTimeoutPolicy } from '@agent-device/command-registry/registry'; import { resolveCommandRequestTimeoutMs } from '@agent-device/command-registry/timeout-policy'; -import { prepareRemoteRequestArtifacts } from '../remote/daemon-artifacts.ts'; +import { + prepareRemoteRequestArtifacts, + type PreparedRemoteRequest, +} from '../remote/daemon-artifacts.ts'; import { attachActiveSessionAddressHint, attachRepairSessionAddressHint, @@ -26,7 +29,8 @@ import { type EnsuredDaemon, } from './daemon-client-lifecycle.ts'; import { sendRequest } from './daemon-client-transport.ts'; -import { buildUploadLeaseHeartbeat, runProtectedLeaseWork } from './daemon-client-lease-beat.ts'; +import { isRemoteDaemon, type DaemonInfo } from './daemon-client-metadata.ts'; +import { leaseScopeFromRequest } from '@agent-device/contracts/lease-scope'; export type DaemonRequest = SharedDaemonRequest; export type DaemonResponse = SharedDaemonResponse; @@ -62,10 +66,11 @@ export async function sendToDaemon( { requestId, session: req.session }, ); const info = daemon.info; - const preparedRemoteRequest = await runProtectedLeaseWork({ - heartbeat: buildUploadLeaseHeartbeat(info, settings, requestWithoutAuthFlag), - task: (signal) => prepareRemoteRequestArtifacts(requestWithoutAuthFlag, info, signal), - }); + const preparedRemoteRequest = await protectArtifactUploadWithLeaseBeats( + info, + settings, + requestWithoutAuthFlag, + ); writeInstallInProgressNotice(requestWithoutAuthFlag.command); const request = buildTransportRequest( @@ -285,3 +290,29 @@ function isInstallLikeCommand(command: string | undefined): boolean { command === INTERNAL_COMMANDS.installSource ); } + +/** + * Uploads a remote request's artifact under a lease beat, so a large artifact cannot outlive the + * lease paying for the device it is going to (#2946). + * + * Only a remote daemon uploads, and a request that names no lease has nothing to renew, so those two + * guards answer for the overwhelming majority of requests — and they are what let the beat module + * stay out of `cli.ts`'s eager closure, which every command pays for. Both are cheap and local: the + * lease scope is read from the request the caller already built. + */ +async function protectArtifactUploadWithLeaseBeats( + info: DaemonInfo, + settings: DaemonClientSettings, + request: Omit, +): Promise { + const leaseScope = leaseScopeFromRequest(request); + if (!isRemoteDaemon(info) || !leaseScope.leaseId) { + return await prepareRemoteRequestArtifacts(request, info, new AbortController().signal); + } + const { buildUploadLeaseHeartbeat, runProtectedLeaseWork } = + await import('./daemon-client-lease-beat.ts'); + return await runProtectedLeaseWork({ + heartbeat: buildUploadLeaseHeartbeat(info, settings, request), + task: (signal) => prepareRemoteRequestArtifacts(request, info, signal), + }); +} diff --git a/src/remote/daemon-artifacts.ts b/src/remote/daemon-artifacts.ts index 4a56d088f7..30a9cb3a1d 100644 --- a/src/remote/daemon-artifacts.ts +++ b/src/remote/daemon-artifacts.ts @@ -20,7 +20,7 @@ export type DaemonArtifactEndpoint = { token: string; }; -type PreparedRemoteRequest = { +export type PreparedRemoteRequest = { positionals: string[]; flags?: DaemonRequest['flags']; installSource?: NonNullable['installSource']; From 0c9cc8410c0f2aa4864b9a487f8f1b71d5c12ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 12:46:02 +0200 Subject: [PATCH 18/21] test(remote): give the slow-beat scenario the lane's parallel-scenario timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provider-integration project sets no `testTimeout`, so the scenario ran under Vitest's 5s default and the Coverage job's full-suite run — heavier than a lane-local one — tripped it: the slow-link case spends 4.5s watching a real lease window, and that body has to sit past the assumed cadence so the opening beat's late answer can move the loop off it. Under the documented parallel import tail the worst case approached the default, and CI drew first. Uses the project's own PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS, which exists for scenarios that build a real in-process daemon path and get timed out by setup contention rather than by their own work. The observation window is untouched: shortening it would cut the margin between the cadence-budgeted beat (three arrivals, each timed out) and the window-budgeted one (the answer lands), which is the only thing this test reads. --- .../remote-upload-lease-beat.test.ts | 73 ++++++++++--------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 2a76f3133f..e929bb1fd9 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -15,6 +15,7 @@ import { listenOnLoopback, skipWhenLoopbackUnavailable, } from '../../../src/__tests__/test-utils/loopback.ts'; +import { PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS } from './test-timeouts.ts'; const LEASE_ID = 'lease-upload-beat'; const TOKEN = 'upload-beat-token'; @@ -344,41 +345,45 @@ test('a lease lost mid-upload aborts the upload and no install request goes out' }); }); -test('a beat that takes over a second to answer sets the cadence instead of timing out', async (t) => { - await withUploadedArtifact(t, 'slow', async (daemon, apkPath) => { - let timeoutsDuringUpload = -1; - let beatsDuringUpload = -1; - let response: Awaited> | undefined; +test( + 'a beat that takes over a second to answer sets the cadence instead of timing out', + async (t) => { + await withUploadedArtifact(t, 'slow', async (daemon, apkPath) => { + let timeoutsDuringUpload = -1; + let beatsDuringUpload = -1; + let response: Awaited> | undefined; - // The request has to run inside a diagnostics scope for the absence of a timeout to mean - // anything: outside one, `emitDiagnostic` records nothing at all and every count reads zero. - await withDiagnosticsScope({ session: 'upload-beat', command: 'install' }, async () => { - const running = sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); - await new Promise((resolve) => { - setTimeout(resolve, SLOW_BEAT_OBSERVATION_MS).unref(); + // The request has to run inside a diagnostics scope for the absence of a timeout to mean + // anything: outside one, `emitDiagnostic` records nothing at all and every count reads zero. + await withDiagnosticsScope({ session: 'upload-beat', command: 'install' }, async () => { + const running = sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); + await new Promise((resolve) => { + setTimeout(resolve, SLOW_BEAT_OBSERVATION_MS).unref(); + }); + timeoutsDuringUpload = countDiagnosticEventsByPhase(['daemon_request_timeout']); + beatsDuringUpload = daemon.beatArrivals.length; + // Only now does the artifact get the rest of its way in, so everything above was measured + // while the upload was genuinely still running. + resumeUploadNow(); + response = await running; }); - timeoutsDuringUpload = countDiagnosticEventsByPhase(['daemon_request_timeout']); - beatsDuringUpload = daemon.beatArrivals.length; - // Only now does the artifact get the rest of its way in, so everything above was measured - // while the upload was genuinely still running. - resumeUploadNow(); - response = await running; - }); - assert.equal(response!.ok, true); - // Two beats, not three: the opening one, and the successor the loop had already armed at the - // cadence it assumes before any window is known. The first beat's late answer then moves the - // loop to a third of the 30s window it just renewed, so nothing else is due inside the - // observation. A beat budgeted at its cadence is cut off before that answer lands, never learns - // the window, and keeps arriving every assumed cadence with a `daemon_request_timeout` behind it - // — which is #2946's slow link wearing the beat down instead of protecting it. - assert.equal(beatsDuringUpload, 2, `beats seen: ${String(beatsDuringUpload)}`); - assert.equal( - timeoutsDuringUpload, - 0, - 'a heartbeat the transport cut off is a timeout, not a slow answer', - ); + assert.equal(response!.ok, true); + // Two beats, not three: the opening one, and the successor the loop had already armed at the + // cadence it assumes before any window is known. The first beat's late answer then moves the + // loop to a third of the 30s window it just renewed, so nothing else is due inside the + // observation. A beat budgeted at its cadence is cut off before that answer lands, never learns + // the window, and keeps arriving every assumed cadence with a `daemon_request_timeout` behind it + // — which is #2946's slow link wearing the beat down instead of protecting it. + assert.equal(beatsDuringUpload, 2, `beats seen: ${String(beatsDuringUpload)}`); + assert.equal( + timeoutsDuringUpload, + 0, + 'a heartbeat the transport cut off is a timeout, not a slow answer', + ); - assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); - }); -}); + assert.deepEqual(daemon.seen, ['lease_heartbeat', 'lease_heartbeat', 'install']); + }); + }, + PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS, +); From 58a6110675f4ee812b4ad291e18d9e74d388782a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 13:11:45 +0200 Subject: [PATCH 19/21] test(remote): center the slow-beat number and un-detach the window doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking notes from the 4c74a43 pass. SLOW_BEAT_MS only discriminates the window-budgeted beat from the cadence-budgeted one inside a narrow band: slower than the pre-window cadence (1666ms) so a beat held to that cadence is cut off before the answer lands, and fast enough to answer before that cadence arms a third beat (3332ms). At 3000ms the late answer had ~332ms of margin to the buggy third arrival — enough to flake on a loaded runner. 2500ms is the center of the band; verified by mutation, budgeting the beat at its cadence now trips "beats seen: 3". Moved the leaseWindowFromHeartbeatResponse doc off leaseBeatIntervalMs and onto the function it describes, and reordered the renewed-window guard to match the undefined guard above it. No behavior change. --- src/daemon-client/daemon-client-lease-beat.ts | 12 ++++++------ .../remote-upload-lease-beat.test.ts | 11 +++++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/daemon-client/daemon-client-lease-beat.ts b/src/daemon-client/daemon-client-lease-beat.ts index 457bb51805..adfd4c9f07 100644 --- a/src/daemon-client/daemon-client-lease-beat.ts +++ b/src/daemon-client/daemon-client-lease-beat.ts @@ -142,8 +142,8 @@ export async function runProtectedLeaseWork( // An answer that names no window keeps the cadence it was asked at: the loop only ever // moves on evidence of how long the lease is good for, and never on the absence of it. if (renewed === undefined) return; - const cadence = leaseBeatIntervalMs(renewed); if (renewed === windowMs) return; + const cadence = leaseBeatIntervalMs(renewed); windowMs = renewed; intervalMs = cadence; // The window just moved, so the next beat is due one cadence from this answer. @@ -201,6 +201,11 @@ export async function runProtectedLeaseWork( return phase.value; } +/** The cadence a window of `windowMs` beats at: a third of it, never faster than the floor. */ +function leaseBeatIntervalMs(windowMs: number): number { + return Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(windowMs / 3)); +} + /** * The inactivity window a beat just renewed, read from the lease its response carries. * @@ -208,11 +213,6 @@ export async function runProtectedLeaseWork( * extended — the same pair `leaseOwnTtlMs` renews on. Anything unrecognizable leaves the caller on * the fallback cadence rather than guessing one. */ -/** The cadence a window of `windowMs` beats at: a third of it, never faster than the floor. */ -function leaseBeatIntervalMs(windowMs: number): number { - return Math.max(MIN_LEASE_BEAT_INTERVAL_MS, Math.floor(windowMs / 3)); -} - function leaseWindowFromHeartbeatResponse(response: unknown): number | undefined { const lease = ( response as Readonly<{ data?: Readonly<{ lease?: Readonly> }> }> diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index e929bb1fd9..dac79b9785 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -26,11 +26,14 @@ const LEASE_WINDOW_MS = 3_000; * A heartbeat round trip on a slow link. * * The number has to land between the two budgets the loop can be holding a beat to: longer than the - * cadence it assumes before a window is known (a third of the registry's five-second minimum), and - * shorter than that assumed window itself. Anything shorter and a beat budgeted at the cadence still - * answers in time, so the test would pass on a budget sized to the wrong thing. + * cadence it assumes before a window is known (1666ms, a third of the registry's five-second + * minimum), and short enough to answer before that assumed cadence arms a third beat (3332ms). + * Anything slower than the cadence and a beat budgeted at it is cut off before the answer lands, so + * the test would pass on a budget sized to the wrong thing; anything past the third arrival and the + * correct loop is caught with an extra beat too. This sits at the middle of that window so a loaded + * runner has room on both sides. */ -const SLOW_BEAT_MS = 3_000; +const SLOW_BEAT_MS = 2_500; /** The window the fake daemon reports on the slow-link case, whose third is its cadence. */ const SLOW_BEAT_LEASE_WINDOW_MS = 30_000; /** How long the slow-link case watches the beat before letting the artifact finish. */ From 9ab78fb31c6e7f13663f7aefe72a5990a721fa82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 13:44:53 +0200 Subject: [PATCH 20/21] test(remote): state the beat's transport cap, and fail the slow case on its cause The ADR told slow links they get the whole window as a beat's answer deadline. The implementation takes the shorter of the window and the heartbeat's request policy, so a ten-minute lease caps at ninety seconds and the paragraph promised a budget the code never grants. Documented rather than removed: the daemon renews when it handles the request, before it answers, so a beat abandoned at the cap still extended the lease when the request landed, and when it didn't a saturated link is not served by a socket held for the rest of a multi-minute window. Shorter windows are unchanged. The slow-link case released its withheld artifact through a module-level global assigned inside the current test's upload handler. A client regression that stops the artifact from ever arriving left the previous test's closure in place, so the call was a no-op and the test hung on `await running` until the lane timeout instead of naming the cause. It now calls the fake daemon's own resumeUpload, which throws when no artifact reached that daemon. Verified by pointing the install at a path that is never uploaded: all three cases fail on that message rather than timing out. --- docs/adr/0007-remote-device-leases.md | 9 ++++++ .../remote-upload-lease-beat.test.ts | 29 ++++++++++--------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/adr/0007-remote-device-leases.md b/docs/adr/0007-remote-device-leases.md index 648bd554df..d64f08b6ae 100644 --- a/docs/adr/0007-remote-device-leases.md +++ b/docs/adr/0007-remote-device-leases.md @@ -98,6 +98,15 @@ itself is saturating — no way to answer at all, and every beat would re-time-o links the beat exists to protect. The window, not the cadence, is the interval in which being alive still matters, so it is also the longest a beat's answer is worth waiting for. +That ceiling is the shorter of the window and the heartbeat's own request policy, currently ninety +seconds. A ten-minute lease therefore gives a beat ninety seconds rather than the full window, which +costs nothing the beat exists to buy: the daemon renews the lease when it handles the request, before +it answers, so a round trip abandoned at the cap has still extended the lease if the request ever +landed — and if it never landed, a transport that has stopped carrying traffic is not served by holding +the socket open for the rest of a window measured in minutes. What the cap refuses is exactly that +idle hold. It binds only on windows longer than the policy, and on those the beat already has the +longest answer deadline any lease gets; shorter windows are bounded by their own window, as above. + A beat is a fresh request each time, never the protected request rewritten: a request identity is what a timed-out beat is canceled under, and beats must not inherit each other's cancellation. A beat that finds the lease gone, or finds that this request can never renew it — its scope is missing or diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index dac79b9785..02a1429dbd 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -70,6 +70,11 @@ type FakeDaemon = Readonly<{ * cancel has nothing left that could stop it by then, and one it did cannot arrive. */ releaseUpload(graceMs?: number): Promise<'drained' | 'canceled' | 'unresolved'>; + /** + * Stops withholding the artifact without waiting for it. Fails when no upload reached this daemon, + * so a caller that expected one learns that now instead of awaiting a response that never comes. + */ + resumeUpload(): void; close(): Promise; }>; @@ -86,7 +91,7 @@ async function startFakeRemoteDaemon(mode: UploadMode): Promise { let beatsAnswered = 0; const beatArrivals: bigint[] = []; let leaseDeclaredLost = false; - let resumeUpload: (() => void) | undefined; + let releaseWithheldUpload: (() => void) | undefined; let resolveOutcome!: (outcome: 'drained' | 'canceled') => void; const outcome = new Promise<'drained' | 'canceled'>((resolve) => { resolveOutcome = resolve; @@ -146,11 +151,10 @@ async function startFakeRemoteDaemon(mode: UploadMode): Promise { answered = true; writeJson(res, 200, { ok: true, uploadId: 'upload-demo.apk' }); }; - resumeUpload = () => { + releaseWithheldUpload = () => { if (stalled) req.resume(); answerUpload(); }; - releaseArtifact = resumeUpload; req.on('data', (chunk: Buffer) => { uploadBytes += chunk.length; // Pausing once, not per chunk: releasing the pressure has to let the artifact through, or a @@ -187,7 +191,7 @@ async function startFakeRemoteDaemon(mode: UploadMode): Promise { } const windowMs = mode === 'slow' ? SLOW_BEAT_LEASE_WINDOW_MS : LEASE_WINDOW_MS; writeBeatAnswer(res, payload.id, windowMs); - if (mode !== 'slow') resumeUpload?.(); + if (mode !== 'slow') releaseWithheldUpload?.(); } function writeBeatAnswer(res: http.ServerResponse, id: unknown, windowMs: number): void { @@ -214,7 +218,7 @@ async function startFakeRemoteDaemon(mode: UploadMode): Promise { uploadBytes: () => uploadBytes, beatArrivals, async releaseUpload(graceMs = 2_000) { - resumeUpload?.(); + releaseWithheldUpload?.(); return await Promise.race([ outcome, new Promise<'unresolved'>((resolve) => { @@ -222,6 +226,12 @@ async function startFakeRemoteDaemon(mode: UploadMode): Promise { }), ]); }, + resumeUpload() { + if (releaseWithheldUpload === undefined) { + throw new Error('no artifact ever reached this daemon, so nothing was being withheld'); + } + releaseWithheldUpload(); + }, close: async () => { server.closeAllConnections(); await new Promise((resolve) => { @@ -244,11 +254,6 @@ function readJsonBody(req: http.IncomingMessage, done: (payload: RpcPayload) => }); } -/** Lets a withheld artifact finish, for the case that measures the phase before ending it. */ -function resumeUploadNow(): void { - releaseArtifact(); -} - function writeJson(res: http.ServerResponse, statusCode: number, payload: unknown): void { res.writeHead(statusCode, { 'content-type': 'application/json' }); res.end(JSON.stringify(payload)); @@ -293,8 +298,6 @@ function installRequest(baseUrl: string, apkPath: string): Omit void; - async function withUploadedArtifact( t: { skip(reason?: string): void }, mode: UploadMode, @@ -367,7 +370,7 @@ test( beatsDuringUpload = daemon.beatArrivals.length; // Only now does the artifact get the rest of its way in, so everything above was measured // while the upload was genuinely still running. - resumeUploadNow(); + daemon.resumeUpload(); response = await running; }); From c10e3a8657c182a30304033991c19174c52f36c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 27 Sep 2026 14:04:06 +0200 Subject: [PATCH 21/21] test(remote): settle the slow-beat count on arrivals instead of a wall clock The slow-link case sampled `beatArrivals.length` at a fixed 4.5s and asserted 2. Both directions of that number depended on event-loop timing: a stall over the margin between the first answer landing and the assumed cadence arming a third beat read as three beats, and a stall long enough to keep the second beat out of the sample read as one. Neither says anything about the loop under test. The count now waits for the two beats it expects, then keeps watching through a quiet window of two assumed cadences and returns what arrived. A loop that budgets a beat at its cadence is still arriving throughout that window and is counted, not awaited; a loop that moved to the window it just renewed is silent until ten seconds in. A runner stall delays the read instead of moving a beat across it. Verified by mutation both ways: budgeting the beat at its interval reports 4 beats, and arming the opening beat late fails with "only 0 of 2 beats arrived" rather than hanging to the lane timeout. The assumed cadence is derived from MIN_LEASE_WINDOW_MS instead of restated, so it cannot drift from what the loop actually assumes. --- .../remote-upload-lease-beat.test.ts | 63 +++++++++++++++---- 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts index 02a1429dbd..cf1637b56b 100644 --- a/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts +++ b/test/integration/provider-scenarios/remote-upload-lease-beat.test.ts @@ -5,6 +5,7 @@ import os from 'node:os'; import path from 'node:path'; import { test } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; +import { MIN_LEASE_WINDOW_MS } from '@agent-device/contracts/lease-scope'; import { countDiagnosticEventsByPhase, withDiagnosticsScope, @@ -36,8 +37,16 @@ const LEASE_WINDOW_MS = 3_000; const SLOW_BEAT_MS = 2_500; /** The window the fake daemon reports on the slow-link case, whose third is its cadence. */ const SLOW_BEAT_LEASE_WINDOW_MS = 30_000; -/** How long the slow-link case watches the beat before letting the artifact finish. */ -const SLOW_BEAT_OBSERVATION_MS = 4_500; +/** The cadence the loop assumes before any window is known: a third of the registry minimum. */ +const ASSUMED_BEAT_INTERVAL_MS = Math.floor(MIN_LEASE_WINDOW_MS / 3); +/** + * How long the slow-link case watches for an extra beat after the second one lands. Two assumed + * cadences: a loop that budgets a beat at its cadence re-sends every one of them and is caught + * inside this window, while the correct loop has moved to a third of the 30s window by then and is + * quiet until ten seconds in. Sampling on arrivals plus a quiet window — rather than a fixed wall + * clock — keeps a runner stall from reading as a missing beat. + */ +const SLOW_BEAT_QUIET_WINDOW_MS = ASSUMED_BEAT_INTERVAL_MS * 2; /** * How the fake daemon makes the upload the long phase it is. @@ -298,6 +307,37 @@ function installRequest(baseUrl: string, apkPath: string): Omit { + return new Promise((resolve) => { + setTimeout(resolve, ms).unref(); + }); +} + +/** + * How many beats arrived, once the loop has been seen to go quiet. + * + * Waits for `expected` arrivals, then keeps watching through one quiet window and returns the count. + * A loop that keeps beating at the cadence it assumed is still arriving inside that window, so it is + * counted rather than awaited; a loop that moved to the window it just renewed is silent through it. + * Settling on arrivals instead of a fixed wall clock means a runner stall delays the sample instead + * of dropping a beat from it. The deadline bounds the wait so a beat that never comes fails on that + * fact rather than on the lane's timeout. + */ +async function settleBeatArrivals(daemon: FakeDaemon, expected: number): Promise { + const deadline = Date.now() + ASSUMED_BEAT_INTERVAL_MS * 3; + while (daemon.beatArrivals.length < expected) { + if (Date.now() > deadline) { + throw new Error( + `only ${String(daemon.beatArrivals.length)} of ${String(expected)} beats arrived; ` + + `daemon saw: ${daemon.seen.join(', ') || 'nothing'}`, + ); + } + await sleep(25); + } + await sleep(SLOW_BEAT_QUIET_WINDOW_MS); + return daemon.beatArrivals.length; +} + async function withUploadedArtifact( t: { skip(reason?: string): void }, mode: UploadMode, @@ -363,11 +403,16 @@ test( // anything: outside one, `emitDiagnostic` records nothing at all and every count reads zero. await withDiagnosticsScope({ session: 'upload-beat', command: 'install' }, async () => { const running = sendToDaemon(installRequest(daemon.baseUrl, apkPath), TRANSPORT); - await new Promise((resolve) => { - setTimeout(resolve, SLOW_BEAT_OBSERVATION_MS).unref(); - }); + // Two beats, not three: the opening one, and the successor the loop had already armed at + // the cadence it assumes before any window is known. The first beat's late answer then + // moves the loop to a third of the 30s window it just renewed, so nothing else is due for + // ten seconds. A beat budgeted at its cadence is cut off before that answer lands, never + // learns the window, and keeps arriving every assumed cadence with a `daemon_request_timeout` + // behind it — which is #2946's slow link wearing the beat down instead of protecting it. + // The count settles on arrivals going quiet rather than on a wall-clock sample, so a loaded + // runner delays the read instead of moving a beat across it. + beatsDuringUpload = await settleBeatArrivals(daemon, 2); timeoutsDuringUpload = countDiagnosticEventsByPhase(['daemon_request_timeout']); - beatsDuringUpload = daemon.beatArrivals.length; // Only now does the artifact get the rest of its way in, so everything above was measured // while the upload was genuinely still running. daemon.resumeUpload(); @@ -375,12 +420,6 @@ test( }); assert.equal(response!.ok, true); - // Two beats, not three: the opening one, and the successor the loop had already armed at the - // cadence it assumes before any window is known. The first beat's late answer then moves the - // loop to a third of the 30s window it just renewed, so nothing else is due inside the - // observation. A beat budgeted at its cadence is cut off before that answer lands, never learns - // the window, and keeps arriving every assumed cadence with a `daemon_request_timeout` behind it - // — which is #2946's slow link wearing the beat down instead of protecting it. assert.equal(beatsDuringUpload, 2, `beats seen: ${String(beatsDuringUpload)}`); assert.equal( timeoutsDuringUpload,