Skip to content

Commit 664888a

Browse files
thymikeethymikee
committed
fix(daemon): keep lease deferral and claim verdicts identity-safe
Two accounting bugs keyed state on a value that is not unique per owner. LeaseInFlightWorkRegistry deleted a lease's whole entry set when the pass that released last had an empty set, even when the registry had since issued a new set for that lease id after pruning a cancelled pass. That removed a live request's deferral, letting a leased device's lease expire under the command still running on it (#2509). Delete the key only while it still holds the set the pass was issued out of. The daemon shutdown claim ledger recorded clear outcomes under SessionState.name, which two cwd-scoped sessions share as 'default'. One session's verdict was read as the other's, so a claim that was never cleared could be reported as released, hiding the orphan 'daemon stop' exists to surface. Key outcomes by the claim's per-acquisition owner token.
1 parent ff8c228 commit 664888a

5 files changed

Lines changed: 112 additions & 12 deletions

File tree

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,24 @@ test('one wanted pass keeps deferral while an abandoned sibling releases', () =>
3737
expect(work.isDeferred('lease-a')).toBe(false);
3838
});
3939

40+
// Pruning a cancelled pass empties its lease's claim set, and the next request on that lease id
41+
// gets a new one. The cancelled work still holds the set it was issued, so releasing it has to
42+
// leave whatever set the registry now holds alone: dropping it there stops the live request's pass
43+
// from deferring its lease at all, which is the lease expiring under a command (#2509).
44+
test('releasing a cancelled pass keeps a later pass deferring its lease', () => {
45+
const work = new LeaseInFlightWorkRegistry();
46+
let canceledWanted = true;
47+
const canceled = work.retain('lease-a', () => canceledWanted);
48+
canceledWanted = false;
49+
50+
expect(work.isDeferred('lease-a')).toBe(false);
51+
const running = work.retain('lease-a', () => true);
52+
53+
expect(canceled.release()).toBe(false);
54+
expect(work.isDeferred('lease-a')).toBe(true);
55+
expect(running.release()).toBe(true);
56+
});
57+
4058
test('releasing a pass twice renews nothing twice', () => {
4159
const work = new LeaseInFlightWorkRegistry();
4260
const pass = work.retain('lease-a', () => true);

‎packages/daemon/src/resources/__tests__/lease-registry.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,6 +855,38 @@ test('abandoned work renews nothing while work still wanted holds the lease', as
855855
assert.equal(registry.listActiveLeases()[0]?.expiresAt, 14_000);
856856
});
857857

858+
// The same accounting error one level up, where it costs a device. Read-only inspection prunes a
859+
// cancelled request's work without consuming the lease, so the next admitted request on that same
860+
// lease starts a fresh claim set. When the cancelled handler finally unwinds it must not take that
861+
// set with it, or the next sweep sees no deferral at all and drops the lease under the command
862+
// still running (#2509).
863+
test('a cancelled request finishing late keeps a later request deferring its lease', () => {
864+
let now = 0;
865+
const registry = new LeaseRegistry({ now: () => now, defaultLeaseTtlMs: 5_000 });
866+
const lease = registry.allocateLease(HUMAN_CONTROL_LEASE_REQUEST);
867+
let canceledWanted = true;
868+
const canceled = registry.retainLeaseWork(lease, () => canceledWanted);
869+
870+
canceledWanted = false;
871+
now = 6_000;
872+
assert.equal(
873+
registry.inspectLeaseBackend(lease.leaseId),
874+
undefined,
875+
'work nobody waited for defers nothing',
876+
);
877+
878+
const running = registry.retainLeaseWork(lease, () => true);
879+
assert.equal(registry.inspectLeaseBackend(lease.leaseId), lease.backend);
880+
881+
canceled.release();
882+
assert.deepEqual(
883+
registry.consumeExpiredLeases().map((entry) => entry.leaseId),
884+
[],
885+
'a request still working on the lease must keep it alive past its own window',
886+
);
887+
running.release();
888+
});
889+
858890
// The ordinary release path, not expiry: a released lease is never read by the
859891
// expiry sweep again, so a work claim left against it would never be cleaned.
860892
test('releasing a lease drops the work claims recorded against it', () => {

‎packages/daemon/src/resources/lease-in-flight-work.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,13 @@ export class LeaseInFlightWorkRegistry {
7878
if (entry.released) return false;
7979
entry.released = true;
8080
entries.delete(entry);
81-
if (entries.size === 0) this.entriesByLeaseId.delete(leaseId);
81+
// Only the set this pass was issued out of still accounts for this lease. Pruning a cancelled
82+
// pass can leave that set empty while a later pass already claimed the lease through a new one
83+
// (#2509), so dropping the key here would delete that request's claim and let the lease expire
84+
// under the command still running on it.
85+
if (entries.size === 0 && this.entriesByLeaseId.get(leaseId) === entries) {
86+
this.entriesByLeaseId.delete(leaseId);
87+
}
8288
return entry.wanted();
8389
}
8490
}

‎packages/daemon/src/server/http/daemon-shutdown-claims.test.ts‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
1-
import { ANDROID_EMULATOR, deviceClaimFilePath, bindTestCommandHost } from '@agent-device/test-kit';
1+
import {
2+
ANDROID_DEVICE,
3+
ANDROID_EMULATOR,
4+
deviceClaimFilePath,
5+
bindTestCommandHost,
6+
} from '@agent-device/test-kit';
7+
import type { DeviceInfo } from '@agent-device/kernel/device';
28
import { afterEach, expect, test, vi } from 'vitest';
39
import {
410
isolatedDeviceClaimStores,
511
retainOrphanedDeviceClaims,
612
} from '../../__tests__/support/device-claim-store.ts';
713
import fs from 'node:fs';
14+
import path from 'node:path';
815
import {
916
acquireDeviceClaim,
1017
inspectDeviceClaims,
@@ -25,15 +32,21 @@ function claimRecord(session: SessionState, name: string) {
2532
deviceKey: session.deviceClaim?.deviceKey,
2633
session: name,
2734
platform: 'android',
28-
deviceId: 'emulator-5554',
35+
deviceId: session.device.id,
2936
};
3037
}
3138

32-
async function claimedSession(name: string): Promise<SessionState & { stateDir: string }> {
33-
const { stateDir } = setup();
39+
async function claimedSession(
40+
name: string,
41+
device: DeviceInfo = ANDROID_EMULATOR,
42+
store = setup(),
43+
slot = 'session',
44+
): Promise<SessionState & { stateDir: string }> {
45+
const stateDir = path.join(store.stateDir, slot);
46+
fs.mkdirSync(stateDir, { recursive: true });
3447
const acquired = await acquireDeviceClaim({
3548
env: process.env,
36-
device: ANDROID_EMULATOR,
49+
device,
3750
session: name,
3851
workspace: stateDir,
3952
stateDir,
@@ -43,7 +56,7 @@ async function claimedSession(name: string): Promise<SessionState & { stateDir:
4356
return {
4457
stateDir,
4558
name,
46-
device: ANDROID_EMULATOR,
59+
device,
4760
deviceClaim: acquired.ownership,
4861
createdAt: Date.now(),
4962
actions: [],
@@ -155,6 +168,33 @@ test('a claim clear that throws is reported orphaned, the one bucket with a work
155168
});
156169
});
157170

171+
// Two cwd-scoped sessions are both named `default` and addressed apart (#2031/#1394), and the
172+
// daemon tears them down in one concurrent pass. A verdict recorded under the display name is one
173+
// session's outcome read as the other's: the session that never cleared its claim gets reported as
174+
// released while that claim is still on disk, which hides the one orphan `daemon stop` exists to
175+
// surface.
176+
test('two sessions named default with different teardown outcomes stay classified apart', async () => {
177+
const bothDefault = setup();
178+
const hiddenOrphan = await claimedSession('default', ANDROID_EMULATOR, bothDefault, 'scope-a');
179+
const cleanlyReleased = await claimedSession('default', ANDROID_DEVICE, bothDefault, 'scope-b');
180+
const ledger = createDaemonShutdownClaimLedger(process.env);
181+
182+
// B's teardown reached a safe terminal state and cleared its claim; A's teardown failed first, so
183+
// its clear never ran. Both finalize once their own teardown is over.
184+
await ledger.releaseClaim(cleanlyReleased);
185+
ledger.finalize(cleanlyReleased);
186+
ledger.finalize(hiddenOrphan);
187+
188+
expect(ledger.claims).toEqual({
189+
released: [claimRecord(cleanlyReleased, 'default')],
190+
orphaned: [claimRecord(hiddenOrphan, 'default')],
191+
superseded: [],
192+
unattributable: [],
193+
});
194+
// The classification has to match what is on disk: this claim was never removed.
195+
expect(fs.existsSync(deviceClaimFilePath(hiddenOrphan.deviceClaim!.deviceKey))).toBe(true);
196+
});
197+
158198
test('a session that never held a claim contributes nothing', async () => {
159199
const ledger = createDaemonShutdownClaimLedger(process.env);
160200
const session: SessionState = {

‎packages/daemon/src/server/http/daemon-shutdown-claims.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,21 +68,25 @@ export function createDaemonShutdownClaimLedger(
6868
superseded: [],
6969
unattributable: [],
7070
};
71+
// Keyed by the claim's owner token, which is minted per acquisition. A session name cannot carry
72+
// this: two cwd-scoped sessions are both named `default`, and one of them reading the other's
73+
// verdict turns an orphan into a report of a clean release.
7174
const outcomes = new Map<string, DeviceClaimClearOutcome | typeof CLEAR_UNRECORDED>();
7275
return {
7376
claims,
7477
releaseClaim: async (session) => {
75-
if (!session.deviceClaim) return;
78+
const claim = session.deviceClaim;
79+
if (!claim) return;
7680
try {
77-
outcomes.set(session.name, await clearDeviceClaim(session.deviceClaim, env));
81+
outcomes.set(claim.ownerToken, await clearDeviceClaim(claim, env));
7882
} catch (error) {
79-
outcomes.set(session.name, CLEAR_UNRECORDED);
83+
outcomes.set(claim.ownerToken, CLEAR_UNRECORDED);
8084
emitDiagnostic({
8185
level: 'warn',
8286
phase: 'daemon_shutdown_device_claim_release_failed',
8387
data: {
8488
session: session.name,
85-
deviceKey: session.deviceClaim.deviceKey,
89+
deviceKey: claim.deviceKey,
8690
error: error instanceof Error ? error.message : String(error),
8791
},
8892
});
@@ -99,7 +103,7 @@ export function createDaemonShutdownClaimLedger(
99103
};
100104
// Exhaustive rather than defaulted: a member added to `DeviceClaimClearOutcome` has to declare
101105
// which bucket it belongs to here, instead of arriving in `orphaned` unnoticed.
102-
const outcome = outcomes.get(session.name);
106+
const outcome = outcomes.get(claim.ownerToken);
103107
switch (outcome) {
104108
case 'deleted':
105109
case 'absent':

0 commit comments

Comments
 (0)