diff --git a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt index 778609ff3b..7897268e17 100644 --- a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt +++ b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt @@ -7,6 +7,24 @@ internal object AndroidDeviceAgent { const val NAME = "androidDevice" const val CHANNEL_NAME = "agent:$NAME" const val SCHEMA_ASSET = "typeagent/androidDeviceSchema.ts" + + /** + * Methods this agent answers on its RPC channel, sent as `agentInterface` + * at registration. + * + * The server builds its proxy from this list and, when several devices host + * `androidDevice`, rejects one whose list differs from the others. So it has + * to describe what `handleAndroidDeviceInvoke` really dispatches: declaring + * a method the device cannot answer fails only later, at the call. Keeping + * one list for both the declaration and the dispatch guard is what stops the + * two from drifting - nothing else checks them against each other, and no CI + * job builds this module. + */ + val SUPPORTED_METHODS = listOf("executeAction") + + /** Whether [SUPPORTED_METHODS] covers an incoming RPC method. */ + fun supports(methodName: String): Boolean = SUPPORTED_METHODS.contains(methodName) + private const val AGENT_DESCRIPTION = "Acts on this Android device: sets alarms and countdown timers, shows the " + "alarm and timer lists, searches for nearby places, shows a place on the " + @@ -34,11 +52,14 @@ internal object AndroidDeviceAgent { .put("actionDefaultEnabled", true) .put("schema", schema) + val agentInterface = JSONArray() + SUPPORTED_METHODS.forEach { agentInterface.put(it) } + return JSONObject() .put("name", NAME) .put("conversationId", conversationId) .put("manifest", manifest) - .put("agentInterface", JSONArray().put("executeAction")) + .put("agentInterface", agentInterface) // Identifies this device so several devices can share one // `androidDevice` agent, and so a reconnect replaces this device // instead of adding another. `multiInstance` is the opt-in: without diff --git a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt index 3756de3e2d..4e90c5f8c0 100644 --- a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt +++ b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt @@ -748,7 +748,10 @@ class WebSocketManager internal constructor( Log.e(TAG, "Android agent invocation is missing callId.") return } - if (methodName != "executeAction") { + // The same list registration declares as agentInterface, so the guard + // and the declaration cannot drift apart. It has one entry today; a + // second would need its own dispatch below, not just a line in the list. + if (!AndroidDeviceAgent.supports(methodName)) { sendRpcError( channelName, callId, diff --git a/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt b/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt index ed6f955fe3..4ab89b97ac 100644 --- a/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt +++ b/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt @@ -23,10 +23,16 @@ class AndroidDeviceAgentTest { assertEquals("instance-1", registration.getString("instanceId")) assertEquals("Pixel 8", registration.getString("displayName")) assertEquals(true, registration.getBoolean("multiInstance")) - assertEquals( - "executeAction", - registration.getJSONArray("agentInterface").getString(0) - ) + // The declared set must be exactly what the RPC dispatcher answers: the + // server builds its proxy from this and, with several devices hosting + // the agent, rejects one that declares a different set. + val declared = registration.getJSONArray("agentInterface") + assertEquals(AndroidDeviceAgent.SUPPORTED_METHODS.size, declared.length()) + for (index in AndroidDeviceAgent.SUPPORTED_METHODS.indices) { + assertEquals(AndroidDeviceAgent.SUPPORTED_METHODS[index], declared.getString(index)) + } + assertTrue(AndroidDeviceAgent.supports("executeAction")) + assertFalse(AndroidDeviceAgent.supports("getDynamicDisplay")) assertEquals( "export type AndroidDeviceAction = never;", registration diff --git a/ts/packages/agentRpc/src/client.ts b/ts/packages/agentRpc/src/client.ts index 1e42e0791c..e42a6e31ad 100644 --- a/ts/packages/agentRpc/src/client.ts +++ b/ts/packages/agentRpc/src/client.ts @@ -46,8 +46,22 @@ import { getActiveTypeAgentSpanAttributes } from "@typeagent/telemetry/traceCont export type AgentRpcOptions = { trustedContextPropagation?: boolean; logger?: RpcStructuredLogger; + channelName?: string; }; +function getAgentChannelName(name: string, options?: AgentRpcOptions): string { + return options?.channelName ?? `agent:${name}`; +} + +function getOptionsChannelName( + name: string, + options?: AgentRpcOptions, +): string { + return options?.channelName === undefined + ? `options:${name}` + : `${options.channelName}:options`; +} + /** * Race a promise against an AbortSignal. If the signal fires before the * promise settles, throw an AbortError immediately (the underlying work @@ -164,7 +178,9 @@ function createOptionsRpc( name: string, options?: AgentRpcOptions, ) { - const channel = channelProvider.createChannel(`options:${name}`); + const channel = channelProvider.createChannel( + getOptionsChannelName(name, options), + ); const optionsMap = createObjectMap(); return { optionsMap, @@ -209,7 +225,9 @@ export async function createAgentRpcClient( agentInterface: AgentInterfaceFunctionName[], options?: AgentRpcOptions, ) { - const channel = channelProvider.createChannel(`agent:${name}`); + const channel = channelProvider.createChannel( + getAgentChannelName(name, options), + ); const contextMap = createObjectMap>(); // Tracks port registration handles returned by sessionContext.registerPort // so the out-of-process agent can release them via the regId we sent back. @@ -869,7 +887,7 @@ export async function createAgentRpcClient( // Options are agent-scoped (created once per initializeAgentContext call) // so they can be released when the context is torn down. if (optionsRpc !== undefined) { - channelProvider.deleteChannel(`options:${name}`); + channelProvider.deleteChannel(getOptionsChannelName(name, options)); optionsRpc = undefined; } return result; diff --git a/ts/packages/agentRpc/src/server.ts b/ts/packages/agentRpc/src/server.ts index 38b7a54c59..8a19d4110c 100644 --- a/ts/packages/agentRpc/src/server.ts +++ b/ts/packages/agentRpc/src/server.ts @@ -43,8 +43,25 @@ import { export type AgentRpcServerOptions = { trustedContextPropagation?: boolean; logger?: RpcStructuredLogger; + channelName?: string; }; +function getAgentChannelName( + name: string, + options?: AgentRpcServerOptions, +): string { + return options?.channelName ?? `agent:${name}`; +} + +function getOptionsChannelName( + name: string, + options?: AgentRpcServerOptions, +): string { + return options?.channelName === undefined + ? `options:${name}` + : `${options.channelName}:options`; +} + function getTrustedRpcOptions( options: AgentRpcServerOptions | undefined, ): RpcOptions | undefined { @@ -73,7 +90,7 @@ function createOptionsRpc( options?: AgentRpcServerOptions, ) { const optionsChannel: RpcChannel = channelProvider.createChannel( - `options:${name}`, + getOptionsChannelName(name, options), ); return createRpc( name, @@ -107,7 +124,7 @@ export function createAgentRpcServer( channelProvider: ChannelProvider, options?: AgentRpcServerOptions, ) { - const channelName = `agent:${name}`; + const channelName = getAgentChannelName(name, options); const channel = channelProvider.createChannel(channelName); let optionsRpc: ReturnType | undefined; diff --git a/ts/packages/agentRpc/test/actionContext.spec.ts b/ts/packages/agentRpc/test/actionContext.spec.ts index 85c3349728..4c642f8094 100644 --- a/ts/packages/agentRpc/test/actionContext.spec.ts +++ b/ts/packages/agentRpc/test/actionContext.spec.ts @@ -44,11 +44,13 @@ describe("agent action context RPC", () => { "test", serverAgent, serverProvider, + { channelName: "agent:test:registration-1" }, ); const clientAgent = await createAgentRpcClient( "test", clientProvider, server.agentInterface, + { channelName: "agent:test:registration-1" }, ); try { diff --git a/ts/packages/agentServer/client/src/agentServerClient.ts b/ts/packages/agentServer/client/src/agentServerClient.ts index 580d6fd990..3f42da0c24 100644 --- a/ts/packages/agentServer/client/src/agentServerClient.ts +++ b/ts/packages/agentServer/client/src/agentServerClient.ts @@ -335,10 +335,40 @@ export function createAgentServerConnection( { dispatcher: Dispatcher; connectionId: string } >(); - // Client-hosted agents registered on the server, name → agent-rpc server - // closeFn. Used to tear down the local rpc server when unregistering, - // re-registering, or closing the connection. - const clientAgentServers = new Map void>(); + // Client-hosted agents registered on the server. Registration details let + // re-registration remove the previous agent while its RPC endpoint is + // still alive, so dispatcher lifecycle teardown can reach it. + const clientAgentServers = new Map< + string, + { + closeFn: () => void; + conversationId: string; + instanceId?: string | undefined; + } + >(); + let nextClientAgentRegistrationId = 0; + + function resolveClientAgentConversationId(conversationId?: string): string { + if (conversationId !== undefined) { + if (!joinedConversations.has(conversationId)) { + throw new Error( + `Not joined to conversation: ${conversationId}`, + ); + } + return conversationId; + } + if (joinedConversations.size === 1) { + return joinedConversations.keys().next().value as string; + } + if (joinedConversations.size === 0) { + throw new Error( + "Cannot register client agent: no conversation joined", + ); + } + throw new Error( + "Cannot register client agent: multiple conversations joined; specify conversationId", + ); + } let closed = false; @@ -624,22 +654,25 @@ export function createAgentServerConnection( conversationId?: string, identity?: ClientAgentIdentity, ): Promise { - // Drop any previous rpc server for this name (e.g. re-registering - // after a reconnect, where the old server sat on a stale channel). - clientAgentServers.get(name)?.(); - clientAgentServers.delete(name); + const resolvedConversationId = + resolveClientAgentConversationId(conversationId); + const previous = clientAgentServers.get(name); + const registrationId = String(++nextClientAgentRegistrationId); + const channelName = `agent:${name}:${registrationId}`; const { closeFn, agentInterface } = createAgentRpcServer( name, agent, currentChannel, + { channelName }, ); try { await rpc.invoke("registerClientAgent", { name, manifest, agentInterface, - ...(conversationId !== undefined ? { conversationId } : {}), + conversationId: resolvedConversationId, + registrationId, ...(identity?.instanceId !== undefined ? { instanceId: identity.instanceId } : {}), @@ -654,7 +687,44 @@ export function createAgentServerConnection( closeFn(); throw e; } - clientAgentServers.set(name, closeFn); + if ( + previous !== undefined && + previous.conversationId !== resolvedConversationId + ) { + try { + await rpc.invoke("unregisterClientAgent", { + name, + conversationId: previous.conversationId, + ...(previous.instanceId !== undefined + ? { instanceId: previous.instanceId } + : {}), + }); + } catch (e) { + try { + await rpc.invoke("unregisterClientAgent", { + name, + conversationId: resolvedConversationId, + ...(identity?.instanceId !== undefined + ? { instanceId: identity.instanceId } + : {}), + }); + } catch (rollbackError) { + closeFn(); + throw new AggregateError( + [e, rollbackError], + `Failed to move client agent '${name}' and roll back the new registration`, + ); + } + closeFn(); + throw e; + } + } + previous?.closeFn(); + clientAgentServers.set(name, { + closeFn, + conversationId: resolvedConversationId, + instanceId: identity?.instanceId, + }); }, async unregisterClientAgent( @@ -669,7 +739,7 @@ export function createAgentServerConnection( ...(instanceId !== undefined ? { instanceId } : {}), }); } finally { - clientAgentServers.get(name)?.(); + clientAgentServers.get(name)?.closeFn(); clientAgentServers.delete(name); } }, @@ -689,7 +759,7 @@ export function createAgentServerConnection( joinedConversations.clear(); // Client-agent rpc servers were bound to the old channel; drop them // so the caller re-registers them on the new channel after re-join. - for (const closeFn of clientAgentServers.values()) { + for (const { closeFn } of clientAgentServers.values()) { closeFn(); } clientAgentServers.clear(); @@ -702,7 +772,7 @@ export function createAgentServerConnection( } closed = true; debug("Closing agent server connection"); - for (const closeFn of clientAgentServers.values()) { + for (const { closeFn } of clientAgentServers.values()) { closeFn(); } clientAgentServers.clear(); diff --git a/ts/packages/agentServer/protocol/src/protocol.ts b/ts/packages/agentServer/protocol/src/protocol.ts index 26f60ef215..1f8a3fb350 100644 --- a/ts/packages/agentServer/protocol/src/protocol.ts +++ b/ts/packages/agentServer/protocol/src/protocol.ts @@ -280,12 +280,13 @@ export type AgentServerInvokeFunctions = { * The agent is removed automatically when the connection drops or leaves * the conversation. * - * The client must create its agent-rpc server on the `agent:` - * channel (via createAgentRpcServer over the connection channel provider) - * before calling this. A client that opts in with `multiInstance` may share - * the agent name with other clients carrying the same schema: the server - * keeps one registration with an instance per client, and routes each - * action to one of them. Without it, a second client is rejected as before. + * The client must create its agent-rpc server before calling this. Current + * clients pass a `registrationId` and host it on the derived unique channel; + * older clients use `agent:`. A client that opts in with + * `multiInstance` may share the agent name with other clients carrying the + * same schema: the server keeps one registration with an instance per + * client, and routes each action to one of them. Without it, a second client + * is rejected as before. */ registerClientAgent: (param: RegisterClientAgentParams) => Promise; /** Unregister a previously registered client-hosted agent. */ @@ -298,6 +299,12 @@ export type RegisterClientAgentParams = { name: string; manifest: AppAgentManifest; agentInterface: AgentInterfaceFunctionName[]; + /** + * Identifies this RPC endpoint on the connection. When supplied, the + * endpoint is hosted on `agent::`, allowing a + * replacement to be validated while the previous endpoint remains live. + */ + registrationId?: string; /** * Target conversation. If omitted, the server uses the connection's single * joined conversation (and errors if the connection has joined none or diff --git a/ts/packages/agentServer/server/src/clientAgentRegistry.ts b/ts/packages/agentServer/server/src/clientAgentRegistry.ts index 9f2a5b26ed..695d4360c6 100644 --- a/ts/packages/agentServer/server/src/clientAgentRegistry.ts +++ b/ts/packages/agentServer/server/src/clientAgentRegistry.ts @@ -7,6 +7,7 @@ import { AppAgentManifest, SessionContext, } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import { createHash } from "node:crypto"; import { createLimiter } from "@typeagent/common-utils"; import registerDebug from "debug"; @@ -36,6 +37,11 @@ export type ClientAgentGroup = { manifest: AppAgentManifest; /** Hash of the schema source; instances must agree on it. See {@link getManifestKey}. */ manifestKey: string; + /** + * Normalized `agentInterface` of the instances currently in the group. See + * {@link getAgentInterfaceKey}. + */ + agentInterfaceKey: string; /** * Whether the client that created this group opted in to sharing the name. * Off means a second, different client is rejected exactly as it was before @@ -53,6 +59,13 @@ export type ClientAgentRegistration = { connectionId: string; appAgent: AppAgent; manifest: AppAgentManifest; + /** + * Methods the client implements, which the caller already used to build + * {@link ClientAgentRegistration.appAgent}. Required: `registerClientAgent` + * takes it as a required field and `createAgentRpcClient` cannot build a + * proxy without it, so a registration that reaches here always has one. + */ + agentInterface: readonly AgentInterfaceFunctionName[]; /** See {@link ClientAgentGroup.multiInstance}. Only read on the first registration. */ multiInstance?: boolean; }; @@ -135,6 +148,27 @@ export function schemaMismatchMessage(name: string): string { return `Client agent '${name}' is already registered on this conversation with a different schema version. Update the app to the same version as the other device(s), or disconnect them first.`; } +/** + * Normalized `agentInterface`, order-insensitive and de-duplicated so key order + * cannot cause a false mismatch (the same trap {@link getManifestKey} avoids + * for Android's `org.json.JSONObject`). + * + * The mux is built once, from the first instance's proxy, and + * {@link getManifestKey} only covers schema text -- two app versions can share + * a schema and still implement different methods. Without this, a device with a + * narrower interface joins a group created by a richer one and silently appears + * to support methods it does not; the call only fails once someone makes it. + */ +export function getAgentInterfaceKey( + agentInterface: readonly AgentInterfaceFunctionName[], +): string { + return [...new Set(agentInterface)].sort().join("\u0000"); +} + +export function interfaceMismatchMessage(name: string): string { + return `Client agent '${name}' is already registered on this conversation by a device that implements a different set of methods. Update the app to the same version as the other device(s), or disconnect them first.`; +} + /** * Names the user can tell apart. Two phones of the same model both report * "Pixel 8", so duplicates get a numeric suffix in registration order. @@ -478,6 +512,15 @@ function createMux(group: ClientAgentGroup, template: AppAgent): AppAgent { return fn.apply(instance.appAgent, rebind(instance)); }; } + if (mux.startBackgroundTasks === undefined) { + // The dispatcher calls this after creating its SessionContext. Keep the + // context even when no device implements the hook so an init-only + // instance can still receive closeAgentContext during replacement. + mux.startBackgroundTasks = async (...args: unknown[]) => { + getInternals(group).sessionContext = + args[0] as SessionContext; + }; + } return mux as unknown as AppAgent; } @@ -485,40 +528,130 @@ function createMux(group: ClientAgentGroup, template: AppAgent): AppAgent { * Bring a device that joined late up to the state the others are in. Failures * are traced, not thrown: one device must not fail another's registration. */ -async function initializeInstance( +async function initializeInstanceState( group: ClientAgentGroup, instance: ClientAgentInstance, ): Promise { const state = getInternals(group); const contexts = getContexts(group, instance.instanceId); const appAgent = methodsOf(instance.appAgent); + if (appAgent.initializeAgentContext !== undefined) { + contexts.agentContext = await appAgent.initializeAgentContext(); + contexts.agentContextSet = true; + } + const sessionContext = state.sessionContext; + if (sessionContext !== undefined) { + const view = viewSessionContext(contexts, sessionContext); + await appAgent.startBackgroundTasks?.(view); + for (const schemaName of state.enabledSchemas) { + await appAgent.updateAgentContext?.(true, view, schemaName); + } + } +} + +async function initializeInstance( + group: ClientAgentGroup, + instance: ClientAgentInstance, +): Promise { try { - if (appAgent.initializeAgentContext !== undefined) { - contexts.agentContext = await appAgent.initializeAgentContext(); - contexts.agentContextSet = true; + await initializeInstanceState(group, instance); + } catch (e) { + debugGroup( + `${group.name}: failed to initialize late-joining instance ${instance.instanceId}: ${ + e instanceof Error ? e.message : String(e) + }`, + ); + } +} + +async function closeInstance( + group: ClientAgentGroup, + instance: ClientAgentInstance, +): Promise { + const state = getInternals(group); + const contexts = state.contexts.get(instance.instanceId); + const sessionContext = state.sessionContext; + if (contexts === undefined || sessionContext === undefined) { + return; + } + const appAgent = methodsOf(instance.appAgent); + const view = viewSessionContext(contexts, sessionContext); + await appAgent.stopBackgroundTasks?.(view); + for (const schemaName of state.enabledSchemas) { + await appAgent.updateAgentContext?.(false, view, schemaName); + } + await appAgent.closeAgentContext?.(view); +} + +async function closeInstanceForReplacement( + group: ClientAgentGroup, + instance: ClientAgentInstance, +): Promise { + const state = getInternals(group); + const contexts = state.contexts.get(instance.instanceId); + const sessionContext = state.sessionContext; + if (contexts === undefined || sessionContext === undefined) { + return; + } + const appAgent = methodsOf(instance.appAgent); + const view = viewSessionContext(contexts, sessionContext); + let backgroundStopped = false; + const disabledSchemas: string[] = []; + try { + if (appAgent.stopBackgroundTasks !== undefined) { + await appAgent.stopBackgroundTasks(view); + backgroundStopped = true; } - const sessionContext = state.sessionContext; - if (sessionContext !== undefined) { - const view = viewSessionContext(contexts, sessionContext); - await appAgent.startBackgroundTasks?.(view); - for (const schemaName of state.enabledSchemas) { + for (const schemaName of state.enabledSchemas) { + await appAgent.updateAgentContext?.(false, view, schemaName); + disabledSchemas.push(schemaName); + } + await appAgent.closeAgentContext?.(view); + } catch (e) { + const rollbackErrors: unknown[] = []; + for (const schemaName of disabledSchemas.reverse()) { + try { await appAgent.updateAgentContext?.(true, view, schemaName); + } catch (rollbackError) { + rollbackErrors.push(rollbackError); + } + } + if (backgroundStopped) { + try { + await appAgent.startBackgroundTasks?.(view); + } catch (rollbackError) { + rollbackErrors.push(rollbackError); } } + if (rollbackErrors.length !== 0) { + throw new AggregateError( + [e, ...rollbackErrors], + `Failed to close and restore client agent '${group.name}' instance '${instance.instanceId}'`, + ); + } + throw e; + } +} + +async function closeInstanceBestEffort( + group: ClientAgentGroup, + instance: ClientAgentInstance, +): Promise { + try { + await closeInstance(group, instance); } catch (e) { debugGroup( - `${group.name}: failed to initialize late-joining instance ${instance.instanceId}: ${ + `${group.name}: failed to close instance ${instance.instanceId}: ${ e instanceof Error ? e.message : String(e) }`, ); } } -export function createClientAgentGroup( - name: string, +function createClientAgentInstance( registration: ClientAgentRegistration, -): ClientAgentGroup { - const instance: ClientAgentInstance = { +): ClientAgentInstance { + return { instanceId: registration.instanceId, displayName: registration.displayName, connectionId: registration.connectionId, @@ -526,10 +659,18 @@ export function createClientAgentGroup( registeredAt: Date.now(), lastUsed: Date.now(), }; +} + +export function createClientAgentGroup( + name: string, + registration: ClientAgentRegistration, +): ClientAgentGroup { + const instance = createClientAgentInstance(registration); const group: ClientAgentGroup = { name, manifest: registration.manifest, manifestKey: getManifestKey(registration.manifest), + agentInterfaceKey: getAgentInterfaceKey(registration.agentInterface), multiInstance: registration.multiInstance === true, instances: new Map([[instance.instanceId, instance]]), mux: undefined as unknown as AppAgent, @@ -546,6 +687,78 @@ export function createClientAgentGroup( return group; } +function replacementInstanceId( + group: ClientAgentGroup, + registration: ClientAgentRegistration, +): string | undefined { + return group.instances.has(registration.instanceId) + ? registration.instanceId + : findInstanceIdForConnection(group, registration.connectionId); +} + +async function restoreInstance( + group: ClientAgentGroup, + instance: ClientAgentInstance, + wasInitialized: boolean, +): Promise { + group.instances.set(instance.instanceId, instance); + if (wasInitialized) { + await initializeInstanceState(group, instance); + } +} + +async function replaceClientAgentInstance( + group: ClientAgentGroup, + previousId: string, + registration: ClientAgentRegistration, +): Promise { + const state = getInternals(group); + const previous = group.instances.get(previousId)!; + const wasInitialized = state.contexts.has(previousId); + if (wasInitialized) { + await closeInstanceForReplacement(group, previous); + } + + group.instances.delete(previousId); + state.contexts.delete(previousId); + const replacement = createClientAgentInstance(registration); + group.instances.set(replacement.instanceId, replacement); + try { + if (wasInitialized) { + await initializeInstanceState(group, replacement); + } + } catch (e) { + let cleanupError: unknown; + try { + await closeInstance(group, replacement); + } catch (closeError) { + cleanupError = closeError; + } + group.instances.delete(replacement.instanceId); + state.contexts.delete(replacement.instanceId); + try { + await restoreInstance(group, previous, wasInitialized); + } catch (rollbackError) { + throw new AggregateError( + cleanupError === undefined + ? [e, rollbackError] + : [e, cleanupError, rollbackError], + `Failed to initialize replacement and restore client agent '${group.name}' instance '${previousId}'`, + ); + } + if (cleanupError !== undefined) { + throw new AggregateError( + [e, cleanupError], + `Failed to initialize and close replacement client agent '${group.name}' instance '${replacement.instanceId}'`, + ); + } + throw e; + } + debugGroup( + `${group.name}: replaced instance ${previousId} with ${replacement.instanceId} (${replacement.displayName}) on connection ${replacement.connectionId}, instances: ${group.instances.size}`, + ); +} + /** * Add a device, or replace its proxy if the same `instanceId` is already * there. Replacing in place is what makes a reconnect work: the device keeps @@ -561,37 +774,18 @@ export async function joinClientAgentGroup( throw new Error(schemaMismatchMessage(group.name)); } - const existing = group.instances.get(registration.instanceId); - if (existing !== undefined) { - existing.appAgent = registration.appAgent; - existing.connectionId = registration.connectionId; - existing.displayName = registration.displayName; - existing.lastUsed = Date.now(); - debugGroup( - `${group.name}: replaced instance ${existing.instanceId} (${existing.displayName}) on connection ${existing.connectionId}, instances: ${group.instances.size}`, - ); + const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); + if (agentInterfaceKey !== group.agentInterfaceKey) { + throw new Error(interfaceMismatchMessage(group.name)); + } + + const previousId = replacementInstanceId(group, registration); + if (previousId !== undefined) { + await replaceClientAgentInstance(group, previousId, registration); return false; } - // A connection hosts one instance per agent name: both would sit on the - // single agent: channel, so an instance already on this connection - // is the same client coming back under a new id, and its proxy died when - // the new registration claimed that channel. Retire it and hand its slot - // over. Leaving it would strand it past the connection's disconnect, and - // requester routing scans in insertion order, so it would be picked ahead - // of the live one. This is a replacement rather than a second device, so - // it does not need the sharing opt-in below. - const superseded = findInstanceIdForConnection( - group, - registration.connectionId, - ); - if (superseded !== undefined) { - group.instances.delete(superseded); - getInternals(group).contexts.delete(superseded); - debugGroup( - `${group.name}: retired instance ${superseded}; connection ${registration.connectionId} re-registered as ${registration.instanceId}`, - ); - } else if (!group.multiInstance) { + if (!group.multiInstance) { // Sharing is opt-in, and the group's creator decides. A client that // opts in cannot join a group whose creator did not, so no client can // widen another client's agent. @@ -601,20 +795,13 @@ export async function joinClientAgentGroup( throw new Error(agentAlreadyExistsMessage(group.name)); } - const instance: ClientAgentInstance = { - instanceId: registration.instanceId, - displayName: registration.displayName, - connectionId: registration.connectionId, - appAgent: registration.appAgent, - registeredAt: Date.now(), - lastUsed: Date.now(), - }; + const instance = createClientAgentInstance(registration); group.instances.set(instance.instanceId, instance); debugGroup( `${group.name}: added instance ${instance.instanceId} (${instance.displayName}) on connection ${instance.connectionId}, instances: ${group.instances.size}`, ); await initializeInstance(group, instance); - return superseded === undefined; + return true; } /** The instance this connection owns in the group, if any. */ @@ -674,6 +861,13 @@ export type ClientAgentHost = { manifest: AppAgentManifest, appAgent: AppAgent, ): Promise; + replaceDynamicAgent( + name: string, + currentManifest: AppAgentManifest, + currentAppAgent: AppAgent, + nextManifest: AppAgentManifest, + nextAppAgent: AppAgent, + ): Promise; removeDynamicAgent(name: string): Promise; }; @@ -715,6 +909,38 @@ export function createClientAgentRegistry(): ClientAgentRegistry { return lock(async () => { const existing = groups.get(name); if (existing !== undefined) { + const manifestKey = getManifestKey(registration.manifest); + if (manifestKey !== existing.manifestKey) { + throw new Error(schemaMismatchMessage(name)); + } + const agentInterfaceKey = getAgentInterfaceKey( + registration.agentInterface, + ); + if (agentInterfaceKey !== existing.agentInterfaceKey) { + if ( + existing.instances.size !== 1 || + replacementInstanceId(existing, registration) === + undefined + ) { + throw new Error(interfaceMismatchMessage(name)); + } + const replacement = createClientAgentGroup(name, { + ...registration, + multiInstance: existing.multiInstance, + }); + await host.replaceDynamicAgent( + name, + existing.manifest, + existing.mux, + registration.manifest, + replacement.mux, + ); + groups.set(name, replacement); + debugGroup( + `${name}: sole instance ${registration.instanceId} changed the method set; dynamic agent replaced`, + ); + return; + } await joinClientAgentGroup(existing, registration); return; } @@ -733,6 +959,17 @@ export function createClientAgentRegistry(): ClientAgentRegistry { if (group === undefined) { return false; } + const instance = group.instances.get(instanceId); + if ( + instance === undefined || + (options?.ownerConnectionId !== undefined && + instance.connectionId !== options.ownerConnectionId) + ) { + return false; + } + if (group.instances.size > 1) { + await closeInstanceBestEffort(group, instance); + } if (!removeClientAgentInstance(group, instanceId, options)) { return false; } diff --git a/ts/packages/agentServer/server/src/connectionHandler.ts b/ts/packages/agentServer/server/src/connectionHandler.ts index 115259a451..3985364d7c 100644 --- a/ts/packages/agentServer/server/src/connectionHandler.ts +++ b/ts/packages/agentServer/server/src/connectionHandler.ts @@ -24,6 +24,13 @@ import type { PortRegistrar } from "agent-dispatcher"; import type { ConversationManager } from "./conversationManager.js"; import { resolveTunnelUrlForDiscovery } from "./tunnelResolver.js"; import { getSpeechToken } from "./speechToken.js"; +import registerDebug from "debug"; + +// Disconnect cleanup is best effort, so a failure cannot be surfaced to anyone: +// the socket it would be reported on is already gone. Without a trace, a client +// agent left behind on the shared dispatcher only shows up much later as a +// routing failure with nothing pointing back at the cause. +const debugError = registerDebug("agent-server:connection:error"); /** * Per-connection handler signature expected by transports (the WebSocket @@ -244,10 +251,12 @@ export function createAgentServerConnectionHandler( >(); // Client-hosted agents this connection registered, per conversation. - // conversationId → (agent name → instanceId). Keyed by instance, not - // just by name, so tearing this connection down removes only its own - // instances and leaves other devices on the same agent alone. - const clientAgents = new Map>(); + // Keyed by instance so disconnect removes only this connection's + // devices; the channel name lets replacements overlap until commit. + const clientAgents = new Map< + string, + Map + >(); // Resolve the conversation a client-agent operation targets. When no id // is given, use the single joined conversation; error if there are zero @@ -583,23 +592,41 @@ export function createAgentServerConnectionHandler( param.displayName, name, ); + const registrationId = + param.registrationId === undefined + ? undefined + : checkIdentityField( + "registrationId", + param.registrationId, + "", + ); + const channelName = + registrationId === undefined + ? `agent:${name}` + : `agent:${name}:${registrationId}`; const registered = clientAgents.get(conversationId); - if (registered?.has(name)) { - // This connection is re-registering the same name (the - // client rebuilt its rpc server). Drop the stale channel so - // the new proxy can claim it. - channelProvider.deleteChannel(`agent:${name}`); + const previous = registered?.get(name); + const replacedExistingChannel = + previous?.channelName === channelName; + if (replacedExistingChannel) { + // Legacy clients reuse agent:. The current client + // uses a unique registration channel, so its old proxy can + // stay live until the replacement commits. + channelProvider.deleteChannel(channelName); } - // Build the rpc proxy on the connection's own channel provider - // (the client hosts the real agent via createAgentRpcServer on - // the matching agent: channel). - const appAgent = await createAgentRpcClient( - name, - channelProvider, - agentInterface, - ); + let proxyCreated = false; try { + // Build the rpc proxy on the connection's own channel + // provider. The client hosts the real agent on the matching + // registration channel. + const appAgent = await createAgentRpcClient( + name, + channelProvider, + agentInterface, + { channelName }, + ); + proxyCreated = true; await conversationManager.addClientAgent( conversationId, name, @@ -609,17 +636,45 @@ export function createAgentServerConnectionHandler( displayName, connectionId, param.multiInstance === true, + agentInterface, ); } catch (e) { - channelProvider.deleteChannel(`agent:${name}`); + if (proxyCreated || replacedExistingChannel) { + channelProvider.deleteChannel(channelName); + } + if ( + replacedExistingChannel && + previous !== undefined && + registered !== undefined + ) { + // The client and server have both replaced the old RPC + // channel by this point. If validation rejected the new + // proxy, remove the now-unreachable previous instance. + const removed = + await conversationManager.removeClientAgent( + conversationId, + name, + previous.instanceId, + { ownerConnectionId: connectionId }, + ); + if (removed) { + registered.delete(name); + } + } throw e; } + if ( + previous !== undefined && + previous.channelName !== channelName + ) { + channelProvider.deleteChannel(previous.channelName); + } let map = clientAgents.get(conversationId); if (map === undefined) { map = new Map(); clientAgents.set(conversationId, map); } - map.set(name, instanceId); + map.set(name, { instanceId, channelName }); }, unregisterClientAgent: async (param) => { const conversationId = resolveClientAgentConversation( @@ -635,7 +690,7 @@ export function createAgentServerConnectionHandler( // someone else's agent. const instanceId = param.instanceId ?? - tracked ?? + tracked?.instanceId ?? conversationManager.findClientAgentInstance( conversationId, name, @@ -657,7 +712,9 @@ export function createAgentServerConnectionHandler( // past disconnect. With nothing tracked there is nothing to // protect, so any dangling channel can go. if (removed || tracked === undefined) { - channelProvider.deleteChannel(`agent:${name}`); + channelProvider.deleteChannel( + tracked?.channelName ?? `agent:${name}`, + ); clientAgents.get(conversationId)?.delete(name); } }, @@ -678,14 +735,30 @@ export function createAgentServerConnectionHandler( for (const [conversationId, agents] of clientAgents.entries()) { const connectionId = joinedConversations.get(conversationId)?.connectionId; - for (const [name, instanceId] of agents.entries()) { + for (const [ + name, + { instanceId, channelName }, + ] of agents.entries()) { conversationManager .removeClientAgent(conversationId, name, instanceId, { ownerConnectionId: connectionId, }) - .catch(() => { - // Best effort on disconnect - }); + .catch((e) => { + // Best effort on disconnect, but not silent: this + // failing is how a client agent leaks onto the + // shared dispatcher. Not retried on purpose -- + // removal is idempotent and ownership-checked, so a + // second attempt could only race a reconnect that + // has legitimately reclaimed the instance. + debugError( + `Failed to remove client agent "${name}" instance ${instanceId} (connection ${connectionId}) from conversation ${conversationId} on disconnect: ${ + e instanceof Error ? e.message : String(e) + }`, + ); + }) + .finally(() => + channelProvider.deleteChannel(channelName), + ); } } clientAgents.clear(); @@ -695,8 +768,15 @@ export function createAgentServerConnectionHandler( ] of joinedConversations.entries()) { conversationManager .leaveConversation(conversationId, connectionId) - .catch(() => { - // Best effort on disconnect + .catch((e) => { + // Best effort on disconnect, but traced: a conversation + // this connection never leaves keeps its dispatcher + // alive and its idle timer from ever starting. + debugError( + `Failed to leave conversation ${conversationId} for connection ${connectionId} on disconnect: ${ + e instanceof Error ? e.message : String(e) + }`, + ); }); } joinedConversations.clear(); diff --git a/ts/packages/agentServer/server/src/conversationManager.ts b/ts/packages/agentServer/server/src/conversationManager.ts index 40d90598a7..087b738b68 100644 --- a/ts/packages/agentServer/server/src/conversationManager.ts +++ b/ts/packages/agentServer/server/src/conversationManager.ts @@ -24,6 +24,7 @@ import { ConversationSummaryResult, } from "agent-dispatcher"; import type { AppAgent, AppAgentManifest } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import type { DisplayLogEntry, PendingInteractionRequest, @@ -253,8 +254,9 @@ export type ConversationManager = { * same schema: the dynamic agent is added once and each client becomes an * instance behind it. Re-registering the same `instanceId` replaces its * proxy in place, which is how a reconnect recovers. Rejects when the - * schema differs, or when the instance is new and multi-instance support - * is switched off. + * schema differs, when the `agentInterface` differs from what the other + * devices implement, or when the instance is new and multi-instance + * support is switched off. */ addClientAgent( conversationId: string, @@ -265,6 +267,7 @@ export type ConversationManager = { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise; /** * Remove one instance added via {@link addClientAgent}. The dynamic agent @@ -1220,6 +1223,7 @@ export async function createConversationManager( displayName: string, connectionId: string, multiInstance: boolean, + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise { const record = conversations.get(conversationId); if (record === undefined) { @@ -1232,6 +1236,7 @@ export async function createConversationManager( connectionId, appAgent, manifest, + agentInterface, multiInstance, }); debugConversation( diff --git a/ts/packages/agentServer/server/src/sharedDispatcher.ts b/ts/packages/agentServer/server/src/sharedDispatcher.ts index 87acb539dd..9abc00a32a 100644 --- a/ts/packages/agentServer/server/src/sharedDispatcher.ts +++ b/ts/packages/agentServer/server/src/sharedDispatcher.ts @@ -28,6 +28,7 @@ import { closeCommandHandlerContext, initializeCommandHandlerContext, createDispatcherFromContext, + getAppAgentName, prewarmReasoning as prewarmDispatcherReasoning, } from "agent-dispatcher/internal"; import { PendingInteractionManager } from "agent-dispatcher/internal"; @@ -48,6 +49,29 @@ function errMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } +function throwAgentStateFailures( + name: string, + result: { + failed: { + schemas: [string, boolean, Error][]; + actions: [string, boolean, Error][]; + commands: [string, boolean, Error][]; + }; + }, +): void { + const failures = [ + ...result.failed.schemas, + ...result.failed.actions, + ...result.failed.commands, + ].filter(([failedName]) => getAppAgentName(failedName) === name); + if (failures.length !== 0) { + throw new AggregateError( + failures.map(([, , error]) => error), + `Failed to enable dynamic agent '${name}'`, + ); + } +} + type ClientRecord = { clientIO: ClientIO; filter: boolean; @@ -946,6 +970,55 @@ export async function createSharedDispatcher( ); }); }, + async replaceDynamicAgent( + name: string, + currentManifest: AppAgentManifest, + currentAppAgent: AppAgent, + nextManifest: AppAgentManifest, + nextAppAgent: AppAgent, + ): Promise { + await context.commandLock(async () => { + await context.agents.removeAgent( + name, + context.agentCache.grammarStore, + ); + try { + await context.agents.addDynamicAgent( + name, + nextManifest, + nextAppAgent, + ); + const result = await context.agents.setState( + context, + context.session.getConfig(), + ); + throwAgentStateFailures(name, result); + } catch (e) { + try { + await context.agents.removeAgent( + name, + context.agentCache.grammarStore, + ); + await context.agents.addDynamicAgent( + name, + currentManifest, + currentAppAgent, + ); + const rollbackResult = await context.agents.setState( + context, + context.session.getConfig(), + ); + throwAgentStateFailures(name, rollbackResult); + } catch (rollbackError) { + throw new AggregateError( + [e, rollbackError], + `Failed to replace dynamic agent '${name}' and restore the previous registration`, + ); + } + throw e; + } + }); + }, async removeDynamicAgent(name: string): Promise { await context.commandLock(async () => { await context.agents.removeAgent( @@ -1011,6 +1084,14 @@ export type SharedDispatcher = { manifest: AppAgentManifest, appAgent: AppAgent, ): Promise; + /** Replace a dynamic agent and restore the previous one if setup fails. */ + replaceDynamicAgent( + name: string, + currentManifest: AppAgentManifest, + currentAppAgent: AppAgent, + nextManifest: AppAgentManifest, + nextAppAgent: AppAgent, + ): Promise; /** Remove a previously added dynamic agent. No-op if it doesn't exist. */ removeDynamicAgent(name: string): Promise; /** @internal Test-only: tighten the no-clients grace window. */ diff --git a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts index 69559bc47b..a6929b3cd7 100644 --- a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts @@ -16,6 +16,7 @@ import { } from "@typeagent/agent-rpc/channel"; import type { AppAgent, AppAgentManifest } from "@typeagent/agent-sdk"; import type { TypeAgentAction } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import type { ClientIO } from "@typeagent/dispatcher-rpc/types"; import type { MacroManager } from "@typeagent/copilot-macros"; import { @@ -52,7 +53,11 @@ const manifest: AppAgentManifest = { type TestServer = { registry: ClientAgentRegistry; - host: ClientAgentHost & { added: string[]; removed: string[] }; + host: ClientAgentHost & { + added: string[]; + replaced: string[]; + removed: string[]; + }; connect(): TestClient; }; @@ -66,13 +71,18 @@ type TestClient = { function createTestServer(): TestServer { const registry = createClientAgentRegistry(); const added: string[] = []; + const replaced: string[] = []; const removed: string[] = []; const host = { added, + replaced, removed, async addDynamicAgent(name: string) { added.push(name); }, + async replaceDynamicAgent(name: string) { + replaced.push(name); + }, async removeDynamicAgent(name: string) { removed.push(name); }, @@ -102,6 +112,7 @@ function createTestServer(): TestServer { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface: readonly AgentInterfaceFunctionName[], ) { await registry.add(host, name, { instanceId, @@ -109,6 +120,7 @@ function createTestServer(): TestServer { connectionId, appAgent, manifest: agentManifest, + agentInterface, multiInstance, }); }, @@ -190,6 +202,15 @@ function makeAgent(executed: TypeAgentAction[]): AppAgent { }; } +function makeAgentWithDynamicDisplay(executed: TypeAgentAction[]): AppAgent { + return { + ...makeAgent(executed), + async getDynamicDisplay() { + return { content: "test", nextRefreshMs: -1 }; + }, + }; +} + async function executeVia( server: TestServer, connectionId: string | undefined, @@ -330,8 +351,8 @@ describe("client agent multi-instance integration", () => { const group = server.registry.groups.get(AGENT_NAME)!; expect([...group.instances.keys()]).toEqual(["device-new"]); - // The slot was handed over, not torn down and rebuilt. expect(server.host.added).toEqual([AGENT_NAME]); + expect(server.host.replaced).toEqual([]); expect(server.host.removed).toEqual([]); // The requester routes to the live proxy, not the retired one. @@ -346,6 +367,105 @@ describe("client agent multi-instance integration", () => { expect(server.host.removed).toEqual([AGENT_NAME]); }); + test("a rejected re-registration preserves the previous instance", async () => { + const server = createTestServer(); + + const executedA: TypeAgentAction[] = []; + const clientA = server.connect(); + await clientA.join(); + await clientA.connection.registerClientAgent( + AGENT_NAME, + manifest, + makeAgent(executedA), + CONVERSATION_ID, + { + instanceId: "device-a", + displayName: "Pixel 8", + multiInstance: true, + }, + ); + + const executedB: TypeAgentAction[] = []; + const clientB = server.connect(); + await clientB.join(); + await clientB.connection.registerClientAgent( + AGENT_NAME, + manifest, + makeAgent(executedB), + CONVERSATION_ID, + { + instanceId: "device-b", + displayName: "Galaxy Tab", + multiInstance: true, + }, + ); + + await expect( + clientA.connection.registerClientAgent( + AGENT_NAME, + manifest, + makeAgentWithDynamicDisplay([]), + CONVERSATION_ID, + { + instanceId: "device-a", + displayName: "Pixel 8", + multiInstance: true, + }, + ), + ).rejects.toThrow(/different set of methods/i); + + expect([ + ...server.registry.groups.get(AGENT_NAME)!.instances.keys(), + ]).toEqual(["device-a", "device-b"]); + await executeVia(server, clientB.connectionId); + expect(executedB).toHaveLength(1); + await executeVia(server, clientA.connectionId); + expect(executedA).toHaveLength(1); + + clientA.disconnect(); + await new Promise((resolve) => setImmediate(resolve)); + expect([ + ...server.registry.groups.get(AGENT_NAME)!.instances.keys(), + ]).toEqual(["device-b"]); + }); + + test("a lone client changing methods replaces the dynamic agent", async () => { + const server = createTestServer(); + + const client = server.connect(); + await client.join(); + await client.connection.registerClientAgent( + AGENT_NAME, + manifest, + makeAgent([]), + CONVERSATION_ID, + { + instanceId: "device-a", + displayName: "Pixel 8", + multiInstance: true, + }, + ); + + await client.connection.registerClientAgent( + AGENT_NAME, + manifest, + makeAgentWithDynamicDisplay([]), + CONVERSATION_ID, + { + instanceId: "device-a", + displayName: "Pixel 8", + multiInstance: true, + }, + ); + + expect(server.host.added).toEqual([AGENT_NAME]); + expect(server.host.replaced).toEqual([AGENT_NAME]); + expect(server.host.removed).toEqual([]); + expect( + server.registry.groups.get(AGENT_NAME)!.mux.getDynamicDisplay, + ).toBeDefined(); + }); + // Unregister only ever removes an instance the caller owns. Naming someone // else's must leave the caller's own bookkeeping alone, or the caller's // instance outlives its connection and lingers as an unreachable device. diff --git a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts index 0a56a053c4..1b4f461799 100644 --- a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts @@ -17,6 +17,7 @@ import { type ClientAgentHost, type ClientAgentRegistry, } from "../src/clientAgentRegistry.js"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; const AGENT_NAME = "androidDevice"; const SCHEMA = @@ -44,33 +45,62 @@ function makeManifest( type FakeDevice = { appAgent: AppAgent; executed: TypeAgentAction[]; + dynamicDisplays: string[]; }; -function makeDevice(): FakeDevice { +/** What a device implements unless a test asks for something else. */ +const DEFAULT_INTERFACE: AgentInterfaceFunctionName[] = ["executeAction"]; + +/** + * A device whose proxy carries exactly the methods it declares. The interface + * checks are about a device advertising methods it cannot answer, so a fake + * that always implements the same one would not show the difference. + * `getDynamicDisplay` is the optional method those tests move in and out. + */ +function makeDevice( + agentInterface: readonly AgentInterfaceFunctionName[] = DEFAULT_INTERFACE, +): FakeDevice { const executed: TypeAgentAction[] = []; + const dynamicDisplays: string[] = []; + const available: Record = { + async executeAction(action: TypeAgentAction) { + executed.push(action); + return undefined; + }, + async getDynamicDisplay(_type: string, displayId: string) { + dynamicDisplays.push(displayId); + return { type: "text", content: displayId }; + }, + }; + const appAgent: Record = {}; + for (const method of agentInterface) { + if (available[method] === undefined) { + throw new Error(`makeDevice has no fake for '${method}'`); + } + appAgent[method] = available[method]; + } return { executed, - appAgent: { - async executeAction(action: TypeAgentAction) { - executed.push(action); - return undefined; - }, - }, + dynamicDisplays, + appAgent: appAgent as unknown as AppAgent, }; } type FakeHost = ClientAgentHost & { added: string[]; + replaced: string[]; removed: string[]; registered: Map; }; function makeHost(): FakeHost { const added: string[] = []; + const replaced: string[] = []; const removed: string[] = []; const registered = new Map(); return { added, + replaced, removed, registered, async addDynamicAgent(name, _manifest, appAgent) { @@ -80,6 +110,17 @@ function makeHost(): FakeHost { added.push(name); registered.set(name, appAgent); }, + async replaceDynamicAgent( + name, + _currentManifest, + currentAppAgent, + _nextManifest, + nextAppAgent, + ) { + expect(registered.get(name)).toBe(currentAppAgent); + replaced.push(name); + registered.set(name, nextAppAgent); + }, async removeDynamicAgent(name) { removed.push(name); registered.delete(name); @@ -122,6 +163,7 @@ async function register( connectionId: string; appAgent: AppAgent; manifest?: AppAgentManifest; + agentInterface?: readonly AgentInterfaceFunctionName[]; multiInstance?: boolean; }, ): Promise { @@ -131,6 +173,13 @@ async function register( connectionId: options.connectionId, appAgent: options.appAgent, manifest: options.manifest ?? makeManifest(), + // Default to what the proxy actually implements, which is what the + // real client sends: createAgentRpcServer derives agentInterface from + // the agent object. A test that passes one explicitly is deliberately + // making the two disagree. + agentInterface: + options.agentInterface ?? + (Object.keys(options.appAgent) as AgentInterfaceFunctionName[]), // Devices opt in; the tests that pin single-host behaviour pass // false explicitly. multiInstance: options.multiInstance ?? true, @@ -257,6 +306,520 @@ describe("clientAgentRegistry registration", () => { expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); + test("a second device implementing fewer methods is rejected", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const a = makeDevice(["executeAction", "getDynamicDisplay"]); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: a.appAgent, + }); + // The mux is built from A's proxy, so the dynamic agent the dispatcher + // holds offers getDynamicDisplay. + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + + // B is an older build: same schema, but no getDynamicDisplay. Without + // the check it would join, and the first getDynamicDisplay that routed + // to B would fail at call time. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(["executeAction"]).appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + }); + + test("a second device implementing extra methods is rejected", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // The other direction: B's extra method would be silently unreachable, + // since the mux only carries what A's proxy had. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + }); + + test("the same method set in another order is accepted", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + agentInterface: ["getDynamicDisplay", "executeAction"], + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("an empty method set is compared like any other", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // Nothing in common with the group, so it is a mismatch rather than an + // opt-out: the key for [] is the empty string, not undefined. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice([]).appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + }); + + test("a device reconnecting with the same method set keeps its slot", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }); + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + expect(host.added).toEqual([AGENT_NAME]); + }); + + test("a lone device that upgrades its app changes the group's method set", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + const dispatcherMux = host.registered.get(AGENT_NAME)!; + expect(dispatcherMux.getDynamicDisplay).toBeUndefined(); + + const upgraded = makeDevice(["executeAction", "getDynamicDisplay"]); + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: upgraded.appAgent, + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + const reloadedMux = host.registered.get(AGENT_NAME)!; + expect(reloadedMux).not.toBe(dispatcherMux); + expect(reloadedMux).toBe(getMux(registry)); + expect(host.added).toEqual([AGENT_NAME]); + expect(host.replaced).toEqual([AGENT_NAME]); + expect(host.removed).toEqual([]); + const { context } = makeSessionContext("conn-a2"); + expect(reloadedMux.getDynamicDisplay).toBeDefined(); + await reloadedMux.getDynamicDisplay!("html", "display-1", context); + expect(upgraded.dynamicDisplays).toEqual(["display-1"]); + }); + + test("a lone device that downgrades loses the method from the mux", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }); + const dispatcherMux = host.registered.get(AGENT_NAME)!; + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // Leaving it on the mux would advertise a method no device can answer. + const reloadedMux = host.registered.get(AGENT_NAME)!; + expect(reloadedMux).not.toBe(dispatcherMux); + expect(reloadedMux).toBe(getMux(registry)); + expect(host.added).toEqual([AGENT_NAME]); + expect(host.replaced).toEqual([AGENT_NAME]); + expect(host.removed).toEqual([]); + expect(reloadedMux.getDynamicDisplay).toBeUndefined(); + expect(reloadedMux.executeAction).toBeDefined(); + }); + + test("a reconnecting device cannot change a shared group's method set", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const shared: AgentInterfaceFunctionName[] = [ + "executeAction", + "getDynamicDisplay", + ]; + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(shared).appAgent, + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(shared).appAgent, + }); + + // Replacing in place keeps the mux built from the original proxy, so + // the check has to cover a replacement too, not just a new instance. + // B is still there and still expects getDynamicDisplay to work. + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction"]).appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + }); + + test("replacing one shared instance refreshes its lifecycle", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const calls: string[] = []; + const makeLifecycleDevice = (label: string): AppAgent => ({ + async initializeAgentContext() { + calls.push(`${label}:initialize`); + return { label }; + }, + async startBackgroundTasks() { + calls.push(`${label}:start`); + }, + async updateAgentContext(enable) { + calls.push(`${label}:update:${enable}`); + }, + async stopBackgroundTasks() { + calls.push(`${label}:stop`); + }, + async closeAgentContext() { + calls.push(`${label}:close`); + }, + async executeAction() { + return undefined; + }, + }); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeLifecycleDevice("old-a"), + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeLifecycleDevice("b"), + }); + + const mux = getMux(registry); + await mux.initializeAgentContext!(); + const { context } = makeSessionContext(undefined); + await mux.startBackgroundTasks!(context); + await mux.updateAgentContext!(true, context, "androidDevice"); + calls.length = 0; + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeLifecycleDevice("new-a"), + }); + + expect(calls).toEqual([ + "old-a:stop", + "old-a:update:false", + "old-a:close", + "new-a:initialize", + "new-a:start", + "new-a:update:true", + ]); + expect(getMux(registry)).toBe(mux); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("a departing shared instance is removed when cleanup fails", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const makeLifecycleDevice = (stopFails: boolean): AppAgent => ({ + async initializeAgentContext() { + return {}; + }, + async startBackgroundTasks() {}, + async stopBackgroundTasks() { + if (stopFails) { + throw new Error("stop failed"); + } + }, + async executeAction() { + return undefined; + }, + }); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeLifecycleDevice(true), + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeLifecycleDevice(false), + }); + const { context } = makeSessionContext(undefined); + await getMux(registry).initializeAgentContext!(); + await getMux(registry).startBackgroundTasks!(context); + + await expect( + registry.remove(host, AGENT_NAME, "a", { + ownerConnectionId: "conn-a", + }), + ).resolves.toBe(true); + expect([...registry.groups.get(AGENT_NAME)!.instances.keys()]).toEqual([ + "b", + ]); + }); + + test("an init-only shared instance is closed when replaced", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const calls: string[] = []; + const makeInitOnlyDevice = (label: string): AppAgent => ({ + async initializeAgentContext() { + calls.push(`${label}:initialize`); + return { label }; + }, + async closeAgentContext() { + calls.push(`${label}:close`); + }, + async executeAction() { + return undefined; + }, + }); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeInitOnlyDevice("old-a"), + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeInitOnlyDevice("b"), + }); + const mux = getMux(registry); + await mux.initializeAgentContext!(); + const { context } = makeSessionContext(undefined); + await mux.startBackgroundTasks!(context); + calls.length = 0; + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeInitOnlyDevice("new-a"), + }); + + expect(calls).toEqual(["old-a:close", "new-a:initialize"]); + }); + + test("a partially initialized replacement is closed before rollback", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const calls: string[] = []; + const makeLifecycleDevice = ( + label: string, + failEnable = false, + ): AppAgent => ({ + async initializeAgentContext() { + calls.push(`${label}:initialize`); + return { label }; + }, + async startBackgroundTasks() { + calls.push(`${label}:start`); + }, + async updateAgentContext(enable) { + calls.push(`${label}:update:${enable}`); + if (enable && failEnable) { + throw new Error("enable failed"); + } + }, + async stopBackgroundTasks() { + calls.push(`${label}:stop`); + }, + async closeAgentContext() { + calls.push(`${label}:close`); + }, + async executeAction() { + return undefined; + }, + }); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeLifecycleDevice("old-a"), + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeLifecycleDevice("b"), + }); + const mux = getMux(registry); + await mux.initializeAgentContext!(); + const { context } = makeSessionContext(undefined); + await mux.startBackgroundTasks!(context); + await mux.updateAgentContext!(true, context, "androidDevice"); + calls.length = 0; + + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeLifecycleDevice("new-a", true), + }), + ).rejects.toThrow("enable failed"); + + expect(calls).toEqual([ + "old-a:stop", + "old-a:update:false", + "old-a:close", + "new-a:initialize", + "new-a:start", + "new-a:update:true", + "new-a:stop", + "new-a:update:false", + "new-a:close", + "old-a:initialize", + "old-a:start", + "old-a:update:true", + ]); + expect( + registry.groups.get(AGENT_NAME)!.instances.get("a")!.connectionId, + ).toBe("conn-a"); + }); + + test("a teardown failure does not reinitialize the previous instance", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const calls: string[] = []; + const makeLifecycleDevice = ( + label: string, + stopFails = false, + ): AppAgent => ({ + async initializeAgentContext() { + calls.push(`${label}:initialize`); + return { label }; + }, + async startBackgroundTasks() { + calls.push(`${label}:start`); + }, + async stopBackgroundTasks() { + calls.push(`${label}:stop`); + if (stopFails) { + throw new Error("stop failed"); + } + }, + async executeAction() { + return undefined; + }, + }); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeLifecycleDevice("old-a", true), + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeLifecycleDevice("b"), + }); + const mux = getMux(registry); + await mux.initializeAgentContext!(); + const { context } = makeSessionContext(undefined); + await mux.startBackgroundTasks!(context); + calls.length = 0; + + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeLifecycleDevice("new-a"), + }), + ).rejects.toThrow("stop failed"); + + expect(calls).toEqual(["old-a:stop"]); + expect( + registry.groups.get(AGENT_NAME)!.instances.get("a")!.connectionId, + ).toBe("conn-a"); + }); + + test("a failed dynamic-agent replacement keeps the previous group", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + const previousGroup = registry.groups.get(AGENT_NAME)!; + const previousMux = host.registered.get(AGENT_NAME)!; + host.replaceDynamicAgent = async () => { + throw new Error("replacement failed"); + }; + + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }), + ).rejects.toThrow("replacement failed"); + + expect(registry.groups.get(AGENT_NAME)).toBe(previousGroup); + expect(host.registered.get(AGENT_NAME)).toBe(previousMux); + expect(previousMux.getDynamicDisplay).toBeUndefined(); + }); + // Case 12 test("a client that does not opt in stays the only host of its agent", async () => { const registry = createClientAgentRegistry();