Skip to content

Commit d0234a0

Browse files
thymikeethymikee
committed
fix(limrun): keep the owner's state on attached instances
Address review on #3173: attached Android teardown removes only the port reverse mappings this session created; app-log reconnect routes by the descriptor's instance id, so a created instance still reattaches through the organization API while another instance is attached; the attached reconnect honors its abort signal; the Android verification hint names the ADB URL variable; connect tests unstub env between tests.
1 parent 62dd146 commit d0234a0

10 files changed

Lines changed: 193 additions & 89 deletions

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import type {
2121
LimrunRuntimeDependencies,
2222
} from './runtime-dependencies.ts';
2323
import type { AndroidAdbInvocation } from '@agent-device/platform-android/mechanics';
24+
import { isAttachedLimrunInstanceId } from './instance-access.ts';
2425
import { normalizeOptionalString } from './strings.ts';
2526
import {
2627
awaitLimrunDeploymentOperation,
@@ -165,9 +166,10 @@ async function cleanupAndroidPortReverse(session: LimrunAndroidSession): Promise
165166
const mappings = await reverse.list().catch(() => []);
166167
const owners = new Set<string>();
167168
const unownedLocals: LimrunPortReverseEndpoint[] = [];
169+
const sharedDevice = isAttachedLimrunInstanceId(session.instanceId);
168170
for (const mapping of mappings) {
169171
if (mapping.ownerId) owners.add(mapping.ownerId);
170-
else unownedLocals.push(mapping.local);
172+
else if (!sharedDevice) unownedLocals.push(mapping.local);
171173
}
172174
await Promise.allSettled([
173175
...[...owners].map(async (ownerId) => await reverse.removeAllOwned(ownerId)),

‎packages/provider-limrun/src/app-log-reconnect.test.ts‎

Lines changed: 95 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,7 @@ import {
3030
serializeAndroidAdbInvocation,
3131
type AndroidAdbInvocation,
3232
} from '@agent-device/platform-android/mechanics';
33-
import {
34-
reconnectAttachedLimrunAppLogReader,
35-
reconnectLimrunAppLogReader,
36-
} from './app-log-reconnect.ts';
33+
import { reconnectLimrunAppLogReader } from './app-log-reconnect.ts';
3734
import { attachedLimrunInstanceId } from './instance-access.ts';
3835
import { createInstanceClient as createIosInstanceClient } from '@limrun/api/ios-client';
3936
import type {
@@ -49,11 +46,12 @@ test('reattaches an owned Limrun instance without persisting credentials', async
4946
}));
5047
const signal = new AbortController().signal;
5148
const outcome = await reconnectLimrunAppLogReader({
52-
limrun: {
53-
iosInstances: {
54-
get,
55-
},
56-
} as never,
49+
orgClient: () =>
50+
({
51+
iosInstances: {
52+
get,
53+
},
54+
}) as never,
5755
descriptor: {
5856
transport: 'limrun-log-poller',
5957
platform: 'ios',
@@ -75,14 +73,15 @@ test('reattaches an owned Limrun instance without persisting credentials', async
7573

7674
test('fails closed when the instance labels do not match the descriptor lease', async () => {
7775
const outcome = await reconnectLimrunAppLogReader({
78-
limrun: {
79-
iosInstances: {
80-
get: vi.fn(async () => ({
81-
metadata: { labels: { provider: 'limrun', leaseId: 'another-lease' } },
82-
status: { state: 'ready', apiUrl: 'https://instance', token: 'secret' },
83-
})),
84-
},
85-
} as never,
76+
orgClient: () =>
77+
({
78+
iosInstances: {
79+
get: vi.fn(async () => ({
80+
metadata: { labels: { provider: 'limrun', leaseId: 'another-lease' } },
81+
status: { state: 'ready', apiUrl: 'https://instance', token: 'secret' },
82+
})),
83+
},
84+
}) as never,
8685
descriptor: {
8786
transport: 'limrun-log-poller',
8887
platform: 'ios',
@@ -100,19 +99,20 @@ test('disconnects the Android instance client when tunnel acquisition fails', as
10099
androidClient.disconnect.mockClear();
101100
await expect(
102101
reconnectLimrunAppLogReader({
103-
limrun: {
104-
androidInstances: {
105-
get: vi.fn(async () => ({
106-
metadata: { labels: { provider: 'limrun', leaseId: 'lease-a' } },
107-
status: {
108-
state: 'ready',
109-
apiUrl: 'https://instance',
110-
adbWebSocketUrl: 'wss://adb',
111-
token: 'secret',
112-
},
113-
})),
114-
},
115-
} as never,
102+
orgClient: () =>
103+
({
104+
androidInstances: {
105+
get: vi.fn(async () => ({
106+
metadata: { labels: { provider: 'limrun', leaseId: 'lease-a' } },
107+
status: {
108+
state: 'ready',
109+
apiUrl: 'https://instance',
110+
adbWebSocketUrl: 'wss://adb',
111+
token: 'secret',
112+
},
113+
})),
114+
},
115+
}) as never,
116116
descriptor: {
117117
transport: 'limrun-log-poller',
118118
platform: 'android',
@@ -141,19 +141,20 @@ test('addresses app-log adb traffic at the tunnel serial and hands cleanup a com
141141
}));
142142
androidClient.disconnect.mockClear();
143143
const outcome = await reconnectLimrunAppLogReader({
144-
limrun: {
145-
androidInstances: {
146-
get: vi.fn(async () => ({
147-
metadata: { labels: { provider: 'limrun', leaseId: 'lease-a' } },
148-
status: {
149-
state: 'ready',
150-
apiUrl: 'https://instance',
151-
adbWebSocketUrl: 'wss://adb',
152-
token: 'secret',
153-
},
154-
})),
155-
},
156-
} as never,
144+
orgClient: () =>
145+
({
146+
androidInstances: {
147+
get: vi.fn(async () => ({
148+
metadata: { labels: { provider: 'limrun', leaseId: 'lease-a' } },
149+
status: {
150+
state: 'ready',
151+
apiUrl: 'https://instance',
152+
adbWebSocketUrl: 'wss://adb',
153+
token: 'secret',
154+
},
155+
})),
156+
},
157+
}) as never,
157158
descriptor: {
158159
transport: 'limrun-log-poller',
159160
platform: 'android',
@@ -215,32 +216,72 @@ test('addresses app-log adb traffic at the tunnel serial and hands cleanup a com
215216
expect(androidClient.disconnect).toHaveBeenCalledOnce();
216217
});
217218

218-
test('reattaches app logs to the attached instance the descriptor names', async () => {
219-
const access = { apiUrl: 'https://attached.example/api', token: 'instance-token' };
220-
const descriptor = {
219+
const ATTACHED_ACCESS = { apiUrl: 'https://attached.example/api', token: 'instance-token' };
220+
221+
function attachedDescriptor(instanceId = attachedLimrunInstanceId(ATTACHED_ACCESS.apiUrl)) {
222+
return {
221223
transport: 'limrun-log-poller',
222224
platform: 'ios',
223225
leaseId: 'lease-a',
224-
instanceId: attachedLimrunInstanceId(access.apiUrl),
226+
instanceId,
225227
appBundleId: 'com.example.app',
226228
outputPath: '/sessions/one/app.log',
227229
} as const;
230+
}
231+
232+
function unavailableOrgClient(): never {
233+
throw new Error('the organization API must not be used');
234+
}
228235

229-
const outcome = await reconnectAttachedLimrunAppLogReader({
230-
instances: { ios: access },
231-
descriptor,
236+
test('reattaches app logs to the attached instance the descriptor names', async () => {
237+
const outcome = await reconnectLimrunAppLogReader({
238+
orgClient: unavailableOrgClient,
239+
instances: { ios: ATTACHED_ACCESS },
240+
descriptor: attachedDescriptor(),
232241
dependencies: {} as LimrunRuntimeDependencies,
233242
});
234243
expect(vi.mocked(createIosInstanceClient)).toHaveBeenLastCalledWith({
235-
...access,
244+
...ATTACHED_ACCESS,
236245
logLevel: 'warn',
237246
});
238247
expect(outcome.status).toBe('opened');
239248

240-
const replaced = await reconnectAttachedLimrunAppLogReader({
241-
instances: { ios: { ...access, apiUrl: 'https://another-instance.example/api' } },
242-
descriptor,
249+
const replaced = await reconnectLimrunAppLogReader({
250+
orgClient: unavailableOrgClient,
251+
instances: { ios: { ...ATTACHED_ACCESS, apiUrl: 'https://another-instance.example/api' } },
252+
descriptor: attachedDescriptor(),
243253
dependencies: {} as LimrunRuntimeDependencies,
244254
});
245255
expect(replaced).toEqual({ status: 'ownership-lost' });
246256
});
257+
258+
test('reattaches a created instance through the organization API while another is attached', async () => {
259+
const get = vi.fn(async () => ({
260+
metadata: { labels: { provider: 'limrun', leaseId: 'lease-a' } },
261+
status: { state: 'ready', apiUrl: 'https://created.example/api', token: 'created-token' },
262+
}));
263+
const outcome = await reconnectLimrunAppLogReader({
264+
orgClient: () => ({ iosInstances: { get } }) as never,
265+
instances: { ios: ATTACHED_ACCESS },
266+
descriptor: attachedDescriptor('ios_created_instance'),
267+
dependencies: {} as LimrunRuntimeDependencies,
268+
});
269+
expect(get).toHaveBeenCalledOnce();
270+
expect(outcome.status).toBe('opened');
271+
});
272+
273+
test('does not connect once the reconnect request is aborted', async () => {
274+
const controller = new AbortController();
275+
controller.abort();
276+
vi.mocked(createIosInstanceClient).mockClear();
277+
await expect(
278+
reconnectLimrunAppLogReader({
279+
orgClient: unavailableOrgClient,
280+
instances: { ios: ATTACHED_ACCESS },
281+
descriptor: attachedDescriptor(),
282+
dependencies: {} as LimrunRuntimeDependencies,
283+
signal: controller.signal,
284+
}),
285+
).rejects.toThrow();
286+
expect(vi.mocked(createIosInstanceClient)).not.toHaveBeenCalled();
287+
});

‎packages/provider-limrun/src/app-log-reconnect.ts‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,42 +8,55 @@ import type { LimrunAppLogReader } from './app-log-poller.ts';
88
import type { LimrunAppLogReconnectOutcome } from './app-log-runtime.ts';
99
import {
1010
attachedLimrunInstanceId,
11+
isAttachedLimrunInstanceId,
1112
type LimrunAndroidInstanceAccess,
1213
type LimrunInstanceAccess,
1314
type LimrunIosInstanceAccess,
1415
} from './instance-access.ts';
1516
import type { LimrunRuntimeDependencies } from './runtime-dependencies.ts';
1617

18+
/**
19+
* Reopens the reader a descriptor names. An attached instance reconnects with its own access; an
20+
* instance this runtime created is looked up through the organization API.
21+
*/
1722
export async function reconnectLimrunAppLogReader(options: {
18-
limrun: Limrun;
23+
orgClient: () => Limrun;
24+
instances?: LimrunInstanceAccess;
1925
descriptor: LimrunAppLogDescriptor;
2026
dependencies: LimrunRuntimeDependencies;
2127
signal?: AbortSignal;
2228
}): Promise<LimrunAppLogReconnectOutcome> {
29+
options.signal?.throwIfAborted();
30+
if (isAttachedLimrunInstanceId(options.descriptor.instanceId)) {
31+
return await reconnectAttached(
32+
options.instances ?? {},
33+
options.descriptor,
34+
options.dependencies,
35+
);
36+
}
37+
const limrun = options.orgClient();
2338
try {
2439
return options.descriptor.platform === 'ios'
25-
? await reconnectIos(options)
26-
: await reconnectAndroid(options);
40+
? await reconnectIos({ ...options, limrun })
41+
: await reconnectAndroid({ ...options, limrun });
2742
} catch (error) {
2843
if (error instanceof NotFoundError) return { status: 'missing' };
2944
throw error;
3045
}
3146
}
3247

33-
/** Reopens a reader on the attached instance, if it is still the one the descriptor names. */
34-
export async function reconnectAttachedLimrunAppLogReader(options: {
35-
instances: LimrunInstanceAccess;
36-
descriptor: LimrunAppLogDescriptor;
37-
dependencies: LimrunRuntimeDependencies;
38-
}): Promise<LimrunAppLogReconnectOutcome> {
39-
const { descriptor, instances } = options;
48+
async function reconnectAttached(
49+
instances: LimrunInstanceAccess,
50+
descriptor: LimrunAppLogDescriptor,
51+
dependencies: LimrunRuntimeDependencies,
52+
): Promise<LimrunAppLogReconnectOutcome> {
4053
if (descriptor.platform === 'ios') {
4154
return isDescribedInstance(instances.ios, descriptor)
4255
? await openIosReader(instances.ios, descriptor)
4356
: { status: 'ownership-lost' };
4457
}
4558
return isDescribedInstance(instances.android, descriptor)
46-
? await openAndroidReader(instances.android, descriptor, options.dependencies)
59+
? await openAndroidReader(instances.android, descriptor, dependencies)
4760
: { status: 'ownership-lost' };
4861
}
4962

‎packages/provider-limrun/src/connection-verification.test.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,28 @@ test('Limrun verification reports a rejected instance token with the variables t
115115
}),
116116
(error: unknown) => {
117117
assert.equal((error as { code?: string }).code, 'COMMAND_FAILED');
118-
assert.match(JSON.stringify(error), /LIM_IOS_INSTANCE_URL and LIM_IOS_INSTANCE_TOKEN/);
118+
assert.match(JSON.stringify(error), /LIM_IOS_INSTANCE_URL and LIM_IOS_INSTANCE_TOKEN,/);
119119
assert.doesNotMatch(JSON.stringify(error), /lim_st_secret/);
120120
return true;
121121
},
122122
);
123123
});
124+
125+
test('Limrun Android verification failure names the ADB URL variable too', async () => {
126+
instanceClients.createAndroid.mockRejectedValueOnce(new Error('adb endpoint refused'));
127+
128+
await assert.rejects(
129+
verifyLimrunConnection({
130+
instances: { android: { apiUrl: 'https://attached/api', token: 't', adbUrl: 'wss://adb' } },
131+
clientVersion: '1.2.3',
132+
platform: 'android',
133+
}),
134+
(error: unknown) => {
135+
assert.match(
136+
JSON.stringify(error),
137+
/LIM_ANDROID_INSTANCE_TOKEN, and LIM_ANDROID_INSTANCE_ADB_URL/,
138+
);
139+
return true;
140+
},
141+
);
142+
});

‎packages/provider-limrun/src/connection-verification.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,10 @@ async function verifyAttachedInstance(
117117
connect: InstanceConnector,
118118
): Promise<LimrunConnectionVerification> {
119119
const platformName = platform === 'android' ? 'Android' : 'iOS';
120-
const envPrefix = platform === 'android' ? 'LIM_ANDROID' : 'LIM_IOS';
120+
const instanceVars =
121+
platform === 'android'
122+
? 'LIM_ANDROID_INSTANCE_URL, LIM_ANDROID_INSTANCE_TOKEN, and LIM_ANDROID_INSTANCE_ADB_URL'
123+
: 'LIM_IOS_INSTANCE_URL and LIM_IOS_INSTANCE_TOKEN';
121124
try {
122125
const client = await connect();
123126
client.disconnect();
@@ -126,7 +129,7 @@ async function verifyAttachedInstance(
126129
'COMMAND_FAILED',
127130
`Limrun ${platformName} instance access failed.`,
128131
{
129-
hint: `Check ${envPrefix}_INSTANCE_URL and ${envPrefix}_INSTANCE_TOKEN, and that the instance is still running.`,
132+
hint: `Check ${instanceVars}, and that the instance is still running.`,
130133
},
131134
error,
132135
);

‎packages/provider-limrun/src/instance-access.test.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
import { expect, test } from 'vitest';
2-
import { attachedLimrunInstanceId } from './instance-access.ts';
2+
import { attachedLimrunInstanceId, isAttachedLimrunInstanceId } from './instance-access.ts';
33

44
test('names an attached instance stably by its API URL', () => {
55
const apiUrl = 'https://region.limrun.example/v1/ios_attached/api';
66
expect(attachedLimrunInstanceId(apiUrl)).toBe(attachedLimrunInstanceId(apiUrl));
77
expect(attachedLimrunInstanceId(apiUrl)).toMatch(/^attached-[a-f0-9]{12}$/);
88
expect(attachedLimrunInstanceId(`${apiUrl}/other`)).not.toBe(attachedLimrunInstanceId(apiUrl));
99
});
10+
11+
test('tells attached instance ids from ids the control plane assigned', () => {
12+
expect(isAttachedLimrunInstanceId(attachedLimrunInstanceId('https://x/api'))).toBe(true);
13+
expect(isAttachedLimrunInstanceId('ios_euna_01m414fmjtfzb82wshkbcaa09c')).toBe(false);
14+
});

‎packages/provider-limrun/src/instance-access.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,17 @@ export type LimrunInstanceAccess = Readonly<{
1919
android?: LimrunAndroidInstanceAccess;
2020
}>;
2121

22+
const ATTACHED_INSTANCE_ID_PREFIX = 'attached-';
23+
2224
/**
2325
* A stable local id for an attached instance, derived from the API URL that identifies it, so the
2426
* same instance keeps the same id across daemon restarts.
2527
*/
2628
export function attachedLimrunInstanceId(apiUrl: string): string {
27-
return `attached-${createHash('sha256').update(apiUrl).digest('hex').slice(0, 12)}`;
29+
return `${ATTACHED_INSTANCE_ID_PREFIX}${createHash('sha256').update(apiUrl).digest('hex').slice(0, 12)}`;
30+
}
31+
32+
/** Whether an instance id names an attached instance, which someone else created and owns. */
33+
export function isAttachedLimrunInstanceId(instanceId: string): boolean {
34+
return instanceId.startsWith(ATTACHED_INSTANCE_ID_PREFIX);
2835
}

0 commit comments

Comments
 (0)