Skip to content

Commit e5ebf76

Browse files
committed
fix(limrun): answer for a joining caller and never end one lease twice
Six ways a lease could be torn down wrongly, or a caller handed a session it was never entitled to, each behind a test that fails without its fix. A joiner with profile flags this provider reads no field for is now refused by the create it joins, not handed that create's session: validation runs before the join for every lease this runtime already holds, while a new lease keeps its validation as the first step of its own create so publication stays free of any await. A late end joins the delete already in flight. The owner records the request it has with Limrun, so the continuation of a create that answered after its disposal gave up waits on that request instead of sending a second one for the same instance. The route an ADB teardown ends its serial through is scoped to that teardown. The reverse manager dispatches through it, and a caller command that reaches the same route while the teardown is mid-request carries no marker: it is refused by the session's closed public route, so nothing gets mapped behind the teardown's one listing. A disposal reports every step it stopped waiting for. A rejected local teardown is reported rather than thrown; an expired capture record contributes its own failure; and a daemon close names why each lease it could not end is still held, not only the first. Two disposal routes awaited a capture end with no deadline at all, and a repeat allocation joined a stranger's destination-tunnel handshake unbounded; all three now answer within a budget. Both continuations that outlive every budget carry rejection handlers, so work this runtime deliberately stopped waiting for cannot reach the process as an unhandled rejection.
1 parent c91a2b9 commit e5ebf76

10 files changed

Lines changed: 392 additions & 108 deletions

‎packages/provider-limrun/src/android.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,48 @@ test('a command that arrives while cleanup holds its serial is refused', async (
5858
expect(tunnels.start).toHaveBeenCalledOnce();
5959
});
6060

61+
/**
62+
* The serial a cleanup is ending is reached only by the cleanup's own dispatches. A reverse command that
63+
* arrives while that cleanup is mid-request goes back through the session's closed public route, so nothing
64+
* gets mapped at a device that is being disconnected, behind the cleanup's one listing.
65+
*/
66+
test('a reverse command that arrives during cleanup is refused, not dispatched at the ending serial', async () => {
67+
const runs: string[][] = [];
68+
let reportList!: () => void;
69+
reverse.listGate = new Promise<void>((resolve) => {
70+
reportList = resolve;
71+
});
72+
const deps = androidDependencies(runs);
73+
// The route the reverse manager dispatches through, which is what production hands it.
74+
let reverseRoute!: (args: readonly string[]) => Promise<unknown>;
75+
deps.android.createPortReverse = async (adb) => {
76+
reverseRoute = adb;
77+
return reverse;
78+
};
79+
const session = await createLimrunAndroidSession(ANDROID_SESSION_OPTIONS, deps);
80+
// The first command raises the tunnel this session owns, giving cleanup a serial to end.
81+
await session.adbProvider.exec(['shell', 'echo', 'ready']);
82+
const dispatched = runs.length;
83+
try {
84+
// Cleanup holds its listing open: its teardown executor is published at the serial it is ending.
85+
const cleanup = cleanupLimrunAndroidAdbTunnel(session);
86+
await vi.waitFor(() => expect(reverse.list).toHaveBeenCalledOnce());
87+
88+
await expect(reverseRoute(['reverse', 'tcp:9001', 'tcp:8001'])).rejects.toMatchObject({
89+
code: 'SESSION_NOT_FOUND',
90+
details: { reason: 'limrun_adb_tunnel_cleaned_up' },
91+
});
92+
93+
reportList();
94+
await cleanup;
95+
} finally {
96+
reverse.listGate = undefined;
97+
}
98+
// Nothing mapped an endpoint behind the cleanup's back: no reverse command was dispatched at all, because
99+
// the listing and the removals are stubbed and the caller's command never reached the device.
100+
expect(runs.slice(dispatched).filter((argv) => argv.includes('reverse'))).toEqual([]);
101+
});
102+
61103
const tunnels = vi.hoisted(() => ({
62104
/**
63105
* Held by a case that has to keep a tunnel in flight, which is the window a session's cleanup has to

‎packages/provider-limrun/src/android.ts‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import path from 'node:path';
2+
import { AsyncLocalStorage } from 'node:async_hooks';
23
import type Limrun from '@limrun/api';
34
import {
45
createInstanceClient as createAndroidInstanceClient,
@@ -184,7 +185,12 @@ export async function cleanupLimrunAndroidAdbTunnel(session: LimrunAndroidSessio
184185
// ask for.
185186
session.adbReverseExecutor = async (args, options) =>
186187
await runLimrunAndroidAdbAtSerial(session, serial, args, options);
187-
await cleanupAndroidPortReverse(session);
188+
// The teardown's marker covers this route and every dispatch this cleanup starts from it, including a
189+
// reverse manager that answers a later request of its own inside the same await chain. A caller command
190+
// that reaches this route through any other path carries no marker.
191+
await limrunAdbTeardownScope.run(true, async () => {
192+
await cleanupAndroidPortReverse(session);
193+
});
188194
await session.dependencies.host
189195
.runAdb(session.dependencies.android.hostAdbInvocation(['disconnect', serial]), {
190196
allowFailure: true,
@@ -239,14 +245,25 @@ async function runLimrunAndroidAdb(
239245
/**
240246
* The reverse manager's own route to the device. It is the session's route while the session is live, and
241247
* cleanup's route of its own once the session is retired: clearing reverse mappings means reaching the device
242-
* that is about to be disconnected, and that has to stay possible after every caller of this session was refused.
248+
* that is about to be disconnected, and that has to stay possible after every caller of this session was
249+
* refused. Only the cleanup's own dispatches carry its teardown scope, so no caller command is ever
250+
* dispatched at the serial the teardown is ending.
243251
*/
252+
/**
253+
* Whether one dispatch belongs to a session's teardown: true only for the dispatches that teardown itself
254+
* started. The reverse manager is given this predicate as its route, and a provider that hands a caller
255+
* command to it — a reverse command queued inside an operation of its own — carries no marker, so the
256+
* manager cannot have a caller's mapping bound at a serial that is on its way out. A marker set by one
257+
* session never authorises another session's route.
258+
*/
259+
const limrunAdbTeardownScope = new AsyncLocalStorage<boolean>();
260+
244261
async function runLimrunAndroidReverseAdb(
245262
session: LimrunAndroidAdbSession,
246263
args: readonly string[],
247264
options?: LimrunAdbCommandOptions,
248265
): Promise<LimrunAdbCommandResult> {
249-
const atSerial = session.adbReverseExecutor;
266+
const atSerial = limrunAdbTeardownScope.getStore() ? session.adbReverseExecutor : undefined;
250267
if (atSerial !== undefined) return await atSerial(args, options);
251268
return await runLimrunAndroidAdb(session, args, options);
252269
}
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import { test, vi } from 'vitest';
2+
import assert from 'node:assert/strict';
3+
import type { DeviceLease } from '@agent-device/contracts/device';
4+
import { LimrunCaptureLifecycle, type LimrunCaptureHost } from './capture-lifecycle.ts';
5+
import type { LimrunLeaseOwner } from './lease-ownership.ts';
6+
import type { NetworkCapture } from './network-har.ts';
7+
8+
/** Longer than the lifecycle's budget for a destination tunnel that is still being opened. */
9+
const CAPTURE_OPEN_WAIT_MS = 25_000;
10+
11+
/**
12+
* A repeat allocation joins the start its owner is already running, within the budget a read or a teardown
13+
* gives that same handshake.
14+
*/
15+
test('a repeat allocation stops waiting on a destination tunnel that never answers', async () => {
16+
vi.useFakeTimers();
17+
const lifecycle = new LimrunCaptureLifecycle(captureHost());
18+
const owner = ownerWithCapture();
19+
let joined = false;
20+
const joining = lifecycle.start(owner, undefined).then(() => {
21+
joined = true;
22+
});
23+
try {
24+
await vi.advanceTimersByTimeAsync(CAPTURE_OPEN_WAIT_MS);
25+
await joining;
26+
} finally {
27+
vi.useRealTimers();
28+
}
29+
30+
assert.ok(joined, 'expected the joining allocation to stop waiting on the stalled tunnel');
31+
// What the stalled start goes on to open stays the owner's to publish, and its teardown's to end: the
32+
// joiner leaves the capture where the allocation that started it put it.
33+
assert.equal(owner.capture?.phase, 'starting');
34+
});
35+
36+
function captureHost(held: readonly LimrunLeaseOwner[] = []): LimrunCaptureHost {
37+
return {
38+
harDir: undefined,
39+
liveSessionOf: () => undefined,
40+
heldOwners: () => held,
41+
};
42+
}
43+
44+
/** An owner whose lease has a destination tunnel that has not answered, and never will. */
45+
function ownerWithCapture(): LimrunLeaseOwner {
46+
const lease = {
47+
leaseId: 'lease-capture-join',
48+
tenantId: 'team-a',
49+
runId: 'run-a',
50+
backend: 'android-instance',
51+
leaseProvider: 'limrun',
52+
createdAt: 1,
53+
heartbeatAt: 1,
54+
expiresAt: 60_001,
55+
} satisfies DeviceLease;
56+
const opening = new Promise<NetworkCapture>(() => {});
57+
return {
58+
lease,
59+
platform: 'android',
60+
creation: Promise.resolve({ status: 'ready', session: undefined, appAlias: undefined }),
61+
state: { phase: 'live' },
62+
rentedInstanceId: undefined,
63+
session: undefined,
64+
capture: { phase: 'starting', opening },
65+
appAlias: undefined,
66+
deletedInstanceId: undefined,
67+
} as unknown as LimrunLeaseOwner;
68+
}

‎packages/provider-limrun/src/capture-lifecycle.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,11 @@ export class LimrunCaptureLifecycle {
5050
async start(owner: LimrunLeaseOwner, hostList: unknown): Promise<void> {
5151
const held = owner.capture;
5252
if (held?.phase === 'starting') {
53-
await held.opening;
53+
// Joining another allocation's destination tunnel is waited for within the budget a read or a teardown
54+
// gives that same handshake, and its outcome changes nothing here: the start keeps its own owner to
55+
// publish to, and this lease's teardown is the route that ends whatever tunnel it opens. So a handshake
56+
// that stops answering costs this caller its wait, and nothing else.
57+
await awaitLimrunWithinBudget(held.opening, CAPTURE_OPEN_BUDGET_MS);
5458
return;
5559
}
5660
const harDir = this.host.harDir;
@@ -107,9 +111,14 @@ export class LimrunCaptureLifecycle {
107111
// written and ended by the continuation below all the same, so it is never left without an owner.
108112
const opened = await awaitLimrunWithinBudget(state.opening, CAPTURE_OPEN_BUDGET_MS);
109113
if (opened.ok) return await endCapture(leaseId, opened.value, this.record);
110-
void state.opening.then(async (late) => {
111-
await endCapture(leaseId, late, this.record);
112-
});
114+
void state.opening.then(
115+
async (late) => {
116+
await endCapture(leaseId, late, this.record);
117+
},
118+
// Observed: this continuation outlives its budget, so a late failure of it must not reach the process
119+
// as an unhandled rejection. The tunnel it could not end stays the host's, reported by nothing else.
120+
() => undefined,
121+
);
113122
return this.settled.get(leaseId);
114123
}
115124

‎packages/provider-limrun/src/client-options.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ export function limrunUserAgent(clientVersion: string): string {
1212
* Asks the SDK for one attempt at a request whose failure this runtime cannot tell apart from Limrun having
1313
* done the work: the SDK retries a lost answer by POSTing again, and a repeated instance create rents a
1414
* second instance. The SDK defaults to two retries, so a request that must not repeat carries this.
15+
*
16+
* The option is the SDK's request-scoped `maxRetries` (`RequestOptions`, @limrun/api 0.64.1), which every
17+
* resource method takes as its second argument. `session-allocation.test.ts` pins that this is what the
18+
* create honours: it drives the real SDK against a local server that drops the answer and asserts one POST.
1519
*/
1620
export const LIMRUN_SINGLE_ATTEMPT = Object.freeze({ maxRetries: 0 });
1721

‎packages/provider-limrun/src/lease-ownership.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ export type LimrunLeaseOwner = {
5757
appAlias: LimrunAppAlias | undefined;
5858
/** The instance Limrun has already confirmed deleted, so a retried disposal never bills a second delete. */
5959
deletedInstanceId: string | undefined;
60+
/**
61+
* The delete this owner has in flight: the instance it names, and the request Limrun has not answered. A
62+
* route that owes the end of that instance waits on this request rather than sending Limrun a second one —
63+
* a delete whose answer this disposal stopped waiting for is still the delete that instance is getting.
64+
*/
65+
pendingDelete:
66+
| Readonly<{ instanceId: string; requested: Promise<string | undefined> }>
67+
| undefined;
6068
};
6169

6270
/** What one lease has live in this runtime, whatever its platform. */
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import { test, vi, expect } from 'vitest';
2+
import type { DeviceLease } from '@agent-device/contracts/device';
3+
import { disposeLimrunLeaseOwner, type LimrunDisposalHost } from './lease-release.ts';
4+
import type { LimrunCreateOutcome, LimrunLeaseOwner } from './lease-ownership.ts';
5+
6+
/*
7+
* Each wait is longer than the budgets it steps past: a disposal's create wait, and everything a disposal
8+
* left without a create answer may still wait for — one delete, the local teardown, one capture record.
9+
*/
10+
const CREATE_ANSWER_WAIT_MS = 130_000;
11+
const DISPOSAL_SETTLE_WAIT_MS = 110_000;
12+
13+
/**
14+
* A create that answers after its disposal stopped waiting owes its end to the disposal already over. If that
15+
* disposal's delete is still with Limrun, the late end joins it: one instance is deleted by one request.
16+
*/
17+
test('a create that answers late joins the delete its disposal already started', async () => {
18+
// Loaded before the clock is replaced, so the disposal's own budget timers are fake timers.
19+
await import('./lifecycle-budget.ts');
20+
let reportDelete!: () => void;
21+
const deleteHost = vi.fn<LimrunDisposalHost['deleteInstance']>(async () => {
22+
await new Promise<void>((resolve) => {
23+
reportDelete = resolve;
24+
});
25+
return undefined;
26+
});
27+
const host: LimrunDisposalHost = {
28+
deleteInstance: deleteHost,
29+
endCapture: async () => undefined,
30+
};
31+
let reportCreate!: () => void;
32+
const creation = new Promise<LimrunCreateOutcome>((resolve) => {
33+
// A create that rented the instance and then failed, after its disposal had given up on it.
34+
reportCreate = () => resolve({ status: 'failed', error: new Error('instance boot failed') });
35+
});
36+
const owner = ownerWithRentedInstance(creation);
37+
38+
vi.useFakeTimers();
39+
try {
40+
const disposal = disposeLimrunLeaseOwner(host, owner);
41+
// The disposal stops waiting on the create and asks Limrun once to end the instance it did rent.
42+
await vi.advanceTimersByTimeAsync(CREATE_ANSWER_WAIT_MS);
43+
expect(deleteHost).toHaveBeenCalledOnce();
44+
// Its own delete then runs out its deadline, and the disposal refuses its caller.
45+
await vi.advanceTimersByTimeAsync(DISPOSAL_SETTLE_WAIT_MS);
46+
await expect(disposal).resolves.toMatchObject({ cleanup: 'uncertain' });
47+
48+
// The create answers now. The end it owes is this disposal's, and it joins the request Limrun already
49+
// has for this instance rather than sending a second one.
50+
reportCreate();
51+
await vi.advanceTimersByTimeAsync(DISPOSAL_SETTLE_WAIT_MS);
52+
expect(deleteHost).toHaveBeenCalledOnce();
53+
54+
// The one request confirming is enough: the joined end records the instance as deleted.
55+
reportDelete();
56+
await vi.advanceTimersByTimeAsync(DISPOSAL_SETTLE_WAIT_MS);
57+
} finally {
58+
vi.useRealTimers();
59+
}
60+
expect(owner.deletedInstanceId).toBe('android-instance-1');
61+
});
62+
63+
/** An owner whose create has rented an instance but has not answered, taken by a disposal. */
64+
function ownerWithRentedInstance(creation: Promise<LimrunCreateOutcome>): LimrunLeaseOwner {
65+
const lease = {
66+
leaseId: 'lease-release-join',
67+
tenantId: 'team-a',
68+
runId: 'run-a',
69+
backend: 'android-instance',
70+
leaseProvider: 'limrun',
71+
createdAt: 1,
72+
heartbeatAt: 1,
73+
expiresAt: 60_001,
74+
} satisfies DeviceLease;
75+
return {
76+
lease,
77+
platform: 'android',
78+
creation,
79+
state: { phase: 'releasing', by: 'release' },
80+
rentedInstanceId: 'android-instance-1',
81+
session: undefined,
82+
localRelease: undefined,
83+
keepAlive: undefined,
84+
capture: undefined,
85+
appAlias: undefined,
86+
deletedInstanceId: undefined,
87+
pendingDelete: undefined,
88+
} as unknown as LimrunLeaseOwner;
89+
}

0 commit comments

Comments
 (0)