Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/cli/commands/__tests__/daemon.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const GRACEFUL_RESULT: DaemonStopResult = {
cleanupConfidence: 'known',
claimsReleased: [],
claimsOrphaned: [],
claimsSuperseded: [],
providerReleases: { status: 'completed', released: [], pending: [] },
warnings: [],
};
Expand Down Expand Up @@ -68,11 +69,18 @@ test('merges a graceful shutdown report and cleans runner leases with the start-
const stateDir = mkdtempForTestSync('agent-device-daemon-command-');
mocks.readDaemonStopIdentity.mockReturnValue({ pid: 123, processStartTime: 'start-time' });
mocks.stopDaemon.mockResolvedValue(GRACEFUL_RESULT);
const claim = {
deviceKey: 'local:android:none:emulator-5554',
session: 'default',
platform: 'android',
deviceId: 'emulator-5554',
};
mocks.readDaemonShutdownReport.mockReturnValue({
providerReleases: {
released: [{ leaseId: 'lease-1', provider: 'limrun' }],
pending: [],
},
claims: { released: [claim], orphaned: [], superseded: [] },
});

try {
Expand All @@ -95,6 +103,10 @@ test('merges a graceful shutdown report and cleans runner leases with the start-
released: [{ leaseId: 'lease-1', provider: 'limrun' }],
pending: [],
},
// #1799: a graceful stop reports the claims it actually released.
claimsReleased: [claim],
claimsOrphaned: [],
claimsSuperseded: [],
}),
expect.any(Function),
);
Expand Down
19 changes: 18 additions & 1 deletion src/cli/commands/daemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,14 @@ function mergeShutdownReport(
): DaemonStopResult {
if (stopped.mode !== 'graceful' || report) {
return report
? { ...stopped, providerReleases: { status: 'completed', ...report.providerReleases } }
? {
...stopped,
providerReleases: { status: 'completed', ...report.providerReleases },
claimsReleased: report.claims.released,
claimsOrphaned: report.claims.orphaned,
claimsSuperseded: report.claims.superseded,
warnings: [...stopped.warnings, ...supersededClaimWarnings(report.claims.superseded)],
}
: stopped;
}
return {
Expand All @@ -55,6 +62,16 @@ function mergeShutdownReport(
};
}

/** A superseded claim is not a failure to report as one, but the operator's
* device is now owned elsewhere, so it must not pass silently. */
function supersededClaimWarnings(superseded: DaemonStopResult['claimsSuperseded']): string[] {
if (superseded.length === 0) return [];
const devices = superseded.map((claim) => claim.deviceId).join(', ');
return [
`Another owner had already claimed ${devices} before this daemon released it, so those devices are now owned elsewhere.`,
];
}

function renderDaemonStop(
result: Pick<DaemonStopResult, 'stopped' | 'mode' | 'warnings'> & {
clean: boolean;
Expand Down
34 changes: 32 additions & 2 deletions src/daemon/__tests__/daemon-shutdown-report.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ import {
import { LeaseRegistry } from '../lease-registry.ts';
import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';

test('round-trips provider release records without persisting lease credentials', () => {
const claim = {
deviceKey: 'local:android:none:emulator-5554',
session: 'default',
platform: 'android',
deviceId: 'emulator-5554',
};

test('round-trips provider release and device claim records without lease credentials', () => {
const stateDir = mkdtempForTestSync('agent-device-shutdown-report-');
const lease = new LeaseRegistry().allocateLease({
tenantId: 'tenant-a',
Expand All @@ -18,13 +25,36 @@ test('round-trips provider release records without persisting lease credentials'
});

try {
writeDaemonShutdownReport(stateDir, { released: [lease], pending: [lease] });
writeDaemonShutdownReport(stateDir, {
providerReleases: { released: [lease], pending: [lease] },
claims: { released: [claim], orphaned: [], superseded: [claim] },
});

expect(readDaemonShutdownReport(stateDir)).toEqual({
providerReleases: {
released: [{ leaseId: lease.leaseId, provider: 'limrun' }],
pending: [{ leaseId: lease.leaseId, provider: 'limrun' }],
},
claims: { released: [claim], orphaned: [], superseded: [claim] },
});
} finally {
fs.rmSync(stateDir, { recursive: true, force: true });
}
});

test('a report written before claim reporting still reads its provider releases', () => {
const stateDir = mkdtempForTestSync('agent-device-shutdown-report-');
const reportPath = path.join(stateDir, 'daemon-shutdown.json');

try {
fs.writeFileSync(
reportPath,
JSON.stringify({ providerReleases: { released: [], pending: [] } }),
);

expect(readDaemonShutdownReport(stateDir)).toEqual({
providerReleases: { released: [], pending: [] },
claims: { released: [], orphaned: [], superseded: [] },
});
} finally {
fs.rmSync(stateDir, { recursive: true, force: true });
Expand Down
20 changes: 19 additions & 1 deletion src/daemon/__tests__/device-claims.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,28 @@ test('clears only the exact owner token and identity, never a successor claim',
claimPath(root),
JSON.stringify({ ...stored, ownerToken: 'successor-token', session: 'second' }),
);
await clearDeviceClaim(acquired.ownership);
// Resolving is not releasing: the outcome is what a caller reporting
// ownership must read, since the successor's claim is deliberately kept.
assert.equal(await clearDeviceClaim(acquired.ownership), 'ownership-changed');
assert.equal(inspectDeviceClaims({ serial: device.id })[0]?.claim?.session, 'second');
});

test('reports the exact outcome of clearing an owned, missing, and unowned claim', async () => {
const root = useClaimsRoot();
const acquired = await acquireDeviceClaim({
device,
session: 'owner',
workspace: '/worktrees/owner',
stateDir: root,
});
assert.equal(acquired.status, 'acquired');
if (acquired.status !== 'acquired') return;

assert.equal(await clearDeviceClaim(acquired.ownership), 'deleted');
assert.equal(await clearDeviceClaim(acquired.ownership), 'absent');
assert.equal(await clearDeviceClaim(undefined), 'absent');
});

test('keeps corrupt records visible and classifies dead owners without reclaiming either', () => {
const root = useClaimsRoot();
fs.writeFileSync(path.join(root, 'corrupt.json'), '{bad json');
Expand Down
73 changes: 68 additions & 5 deletions src/daemon/daemon-shutdown-report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,53 @@ export type ProviderReleaseRecord = {
provider?: string;
};

/**
* #1320: what happened to one session's device claim during graceful teardown.
* `released` means the claim was confirmed gone after the session reached a safe
* terminal state; `orphaned` means teardown left it in place, so the exiting
* daemon's dead owner identity is what later proves it reclaimable; `superseded`
* means another owner had already replaced it, so this daemon released nothing
* and left nothing to reconcile.
*/
export type DeviceClaimRecord = {
deviceKey: string;
session: string;
platform: string;
deviceId: string;
};

export type DaemonShutdownReport = {
providerReleases: {
released: ProviderReleaseRecord[];
pending: ProviderReleaseRecord[];
};
claims: {
released: DeviceClaimRecord[];
orphaned: DeviceClaimRecord[];
superseded: DeviceClaimRecord[];
};
};

export function writeDaemonShutdownReport(
stateDir: string,
providerReleases: { released: readonly DeviceLease[]; pending: readonly DeviceLease[] },
outcome: {
providerReleases: { released: readonly DeviceLease[]; pending: readonly DeviceLease[] };
claims: {
released: readonly DeviceClaimRecord[];
orphaned: readonly DeviceClaimRecord[];
superseded: readonly DeviceClaimRecord[];
};
},
): void {
const report: DaemonShutdownReport = {
providerReleases: {
released: providerReleases.released.map(toProviderReleaseRecord),
pending: providerReleases.pending.map(toProviderReleaseRecord),
released: outcome.providerReleases.released.map(toProviderReleaseRecord),
pending: outcome.providerReleases.pending.map(toProviderReleaseRecord),
},
claims: {
released: [...outcome.claims.released],
orphaned: [...outcome.claims.orphaned],
superseded: [...outcome.claims.superseded],
},
};
const filePath = shutdownReportPath(stateDir);
Expand All @@ -42,7 +74,10 @@ export function writeDaemonShutdownReport(
export function readDaemonShutdownReport(stateDir: string): DaemonShutdownReport | null {
try {
const parsed = JSON.parse(fs.readFileSync(shutdownReportPath(stateDir), 'utf8')) as unknown;
return isDaemonShutdownReport(parsed) ? parsed : null;
if (!isProviderReleaseReport(parsed)) return null;
// A report left behind by a daemon that predates claim reporting still
// describes its provider releases honestly; it just knows nothing of claims.
return { ...parsed, claims: readClaimSection(parsed) };
} catch {
return null;
}
Expand All @@ -65,7 +100,9 @@ function toProviderReleaseRecord(lease: DeviceLease): ProviderReleaseRecord {
};
}

function isDaemonShutdownReport(value: unknown): value is DaemonShutdownReport {
function isProviderReleaseReport(
value: unknown,
): value is Omit<DaemonShutdownReport, 'claims'> & { claims?: unknown } {
if (!value || typeof value !== 'object') return false;
const releases = (value as { providerReleases?: unknown }).providerReleases;
if (!releases || typeof releases !== 'object') return false;
Expand All @@ -78,6 +115,32 @@ function isDaemonShutdownReport(value: unknown): value is DaemonShutdownReport {
);
}

function readClaimSection(value: { claims?: unknown }): DaemonShutdownReport['claims'] {
const claims = value.claims;
if (!claims || typeof claims !== 'object') return { released: [], orphaned: [], superseded: [] };
const records = claims as { released?: unknown; orphaned?: unknown; superseded?: unknown };
return {
released: readClaimRecords(records.released),
orphaned: readClaimRecords(records.orphaned),
superseded: readClaimRecords(records.superseded),
};
}

function readClaimRecords(value: unknown): DeviceClaimRecord[] {
return Array.isArray(value) ? value.filter(isDeviceClaimRecord) : [];
}

function isDeviceClaimRecord(value: unknown): value is DeviceClaimRecord {
if (!value || typeof value !== 'object') return false;
const record = value as Partial<Record<keyof DeviceClaimRecord, unknown>>;
return (
typeof record.deviceKey === 'string' &&
typeof record.session === 'string' &&
typeof record.platform === 'string' &&
typeof record.deviceId === 'string'
);
}

function isProviderReleaseRecord(value: unknown): value is ProviderReleaseRecord {
if (!value || typeof value !== 'object') return false;
const record = value as { leaseId?: unknown; provider?: unknown };
Expand Down
16 changes: 13 additions & 3 deletions src/daemon/daemon-stop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { isAgentDeviceDaemonProcess, trySignalProcess } from './daemon-process.t
import { isProcessAlive, waitForProcessExit } from '../utils/host-process.ts';
import { sleep } from '../utils/timeouts.ts';
import type { DaemonPaths } from './config.ts';
import type { ProviderReleaseRecord } from './daemon-shutdown-report.ts';
import type { DeviceClaimRecord, ProviderReleaseRecord } from './daemon-shutdown-report.ts';

const DAEMON_STOP_GRACE_TIMEOUT_MS = 10_000;
const DAEMON_STOP_KILL_TIMEOUT_MS = 2_000;
Expand All @@ -19,8 +19,15 @@ export type DaemonStopResult = {
stopped: boolean;
mode: 'graceful' | 'forced' | 'not-running';
cleanupConfidence: 'known' | 'unknown';
claimsReleased: [];
claimsOrphaned: [];
/**
* #1320 claim results. Only a graceful stop can carry values: they come from
* the shutdown report the exiting daemon wrote, so a forced kill or a daemon
* that was not running reports none rather than claiming certainty.
*/
claimsReleased: DeviceClaimRecord[];
claimsOrphaned: DeviceClaimRecord[];
/** Claims another owner had already taken over; this daemon released nothing. */
claimsSuperseded: DeviceClaimRecord[];
providerReleases: {
status: 'completed' | 'unknown';
released: ProviderReleaseRecord[];
Expand Down Expand Up @@ -66,6 +73,7 @@ export async function stopDaemon(params: {
cleanupConfidence: 'known',
claimsReleased: [],
claimsOrphaned: [],
claimsSuperseded: [],
providerReleases: { status: 'completed', released: [], pending: [] },
warnings: [],
};
Expand All @@ -89,6 +97,7 @@ export async function stopDaemon(params: {
cleanupConfidence: 'unknown',
claimsReleased: [],
claimsOrphaned: [],
claimsSuperseded: [],
providerReleases: { status: 'unknown', released: [], pending: null },
warnings: [
'The daemon was force-killed before provider lease state could be finalized. Provider allocations may remain active.',
Expand Down Expand Up @@ -148,6 +157,7 @@ function notRunningResult(): DaemonStopResult {
cleanupConfidence: 'known',
claimsReleased: [],
claimsOrphaned: [],
claimsSuperseded: [],
providerReleases: { status: 'completed', released: [], pending: [] },
warnings: [],
};
Expand Down
30 changes: 23 additions & 7 deletions src/daemon/device-claims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,28 +231,44 @@ function isCurrentClaimOwner(
);
}

/**
* What releasing a claim actually did. Resolving is not the same as releasing:
* clearing deliberately leaves a claim it does not own in place, so a caller
* that reports ownership must read this rather than the absence of a throw.
*
* - `deleted` — the claim this ownership acquired was removed.
* - `absent` — no claim remains for the device; nothing to remove.
* - `ownership-changed`— a claim remains, but it is not the one we acquired
* (a successor owner, or a record we cannot attribute).
*/
export type DeviceClaimClearOutcome = 'deleted' | 'absent' | 'ownership-changed';

export async function clearDeviceClaim(
ownership: DeviceClaimSessionOwnership | undefined,
): Promise<void> {
if (!ownership) return;
await withDeviceClaimLock(ownership.deviceKey, async () => {
const inspected = inspectDeviceClaimFile(resolveDeviceClaimPath(ownership.deviceKey));
if (!inspected?.claim) return;
): Promise<DeviceClaimClearOutcome> {
if (!ownership) return 'absent';
return await withDeviceClaimLock(ownership.deviceKey, async () => {
const claimPath = resolveDeviceClaimPath(ownership.deviceKey);
const inspected = inspectDeviceClaimFile(claimPath);
if (!inspected) return 'absent';
const claim = inspected.claim;
if (
!claim ||
claim.ownerToken !== ownership.ownerToken ||
!ownerIdentityMatches(
{ pid: claim.ownerPid, startTime: claim.ownerStartTime },
{ pid: ownership.ownerPid, startTime: ownership.ownerStartTime },
)
) {
return;
return 'ownership-changed';
}
try {
fs.unlinkSync(resolveDeviceClaimPath(ownership.deviceKey));
fs.unlinkSync(claimPath);
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error;
return 'absent';
}
return 'deleted';
});
}

Expand Down
Loading
Loading