From 5d07c86f665a161e82ea64a67aa5e9be874c61f4 Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 11:45:33 -0700 Subject: [PATCH 1/4] Trace client agent cleanup failures and pin the group's method set Two follow-ups to #2914, both on the client-hosted agent path. Disconnect cleanup swallowed every error. `removeClientAgent` failing is how a client agent leaks onto the shared dispatcher, and `leaveConversation` failing keeps a dispatcher alive with its idle timer never starting -- both then surface much later with nothing pointing back at the cause. Trace them on `agent-server:connection:error`. Not retried: removal is idempotent and ownership-checked, so a second attempt could only race a reconnect that has legitimately reclaimed the instance. `createMux` builds its method set from whichever proxy created the group, and `getManifestKey` hashes schema text only, so two builds can share a schema and still implement different methods. A device with a different `agentInterface` joined and appeared to support methods it does not, and the call only failed once someone made it. Compare the interface at join, alongside the schema, and for a replacement too, since replacing in place keeps the original mux. Only compared when both sides declared one, so a client that sends none is unaffected. Tests: 4 cases (115 passed, 111 before). Mutation-checked -- disabling the interface check fails both rejection cases and leaves both acceptance cases passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../server/src/clientAgentRegistry.ts | 47 ++++++++++ .../server/src/connectionHandler.ts | 33 ++++++- .../server/src/conversationManager.ts | 7 +- .../test/clientAgentIntegration.spec.ts | 2 + .../server/test/clientAgentRegistry.spec.ts | 89 +++++++++++++++++++ 5 files changed, 172 insertions(+), 6 deletions(-) diff --git a/ts/packages/agentServer/server/src/clientAgentRegistry.ts b/ts/packages/agentServer/server/src/clientAgentRegistry.ts index 9f2a5b26ed..bb33e446d7 100644 --- a/ts/packages/agentServer/server/src/clientAgentRegistry.ts +++ b/ts/packages/agentServer/server/src/clientAgentRegistry.ts @@ -36,6 +36,11 @@ export type ClientAgentGroup = { manifest: AppAgentManifest; /** Hash of the schema source; instances must agree on it. See {@link getManifestKey}. */ manifestKey: string; + /** + * Normalized `agentInterface` the group was created with, or undefined when + * the creator did not declare one. See {@link getAgentInterfaceKey}. + */ + agentInterfaceKey: string | undefined; /** * 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 +58,11 @@ export type ClientAgentRegistration = { connectionId: string; appAgent: AppAgent; manifest: AppAgentManifest; + /** + * Methods the client implements. Optional so a client that does not send + * one keeps working; when present it must match the group's. + */ + agentInterface?: readonly string[] | undefined; /** See {@link ClientAgentGroup.multiInstance}. Only read on the first registration. */ multiInstance?: boolean; }; @@ -135,6 +145,28 @@ 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`, or undefined when the client did not declare + * one. + * + * 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 string[] | undefined, +): string | undefined { + return agentInterface === undefined + ? undefined + : [...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. @@ -530,6 +562,7 @@ export function createClientAgentGroup( 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, @@ -561,6 +594,20 @@ export async function joinClientAgentGroup( throw new Error(schemaMismatchMessage(group.name)); } + // Checked alongside the schema, and for a replacement too: the mux was + // built from the method set of whichever proxy created the group, so an + // instance that arrives with a different one would be routed calls it + // cannot answer. Only compared when both sides declared an interface, so a + // client that sends none keeps working. + const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); + if ( + agentInterfaceKey !== undefined && + group.agentInterfaceKey !== undefined && + agentInterfaceKey !== group.agentInterfaceKey + ) { + throw new Error(interfaceMismatchMessage(group.name)); + } + const existing = group.instances.get(registration.instanceId); if (existing !== undefined) { existing.appAgent = registration.appAgent; diff --git a/ts/packages/agentServer/server/src/connectionHandler.ts b/ts/packages/agentServer/server/src/connectionHandler.ts index 115259a451..4dad7ba972 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 @@ -609,6 +616,7 @@ export function createAgentServerConnectionHandler( displayName, connectionId, param.multiInstance === true, + agentInterface, ); } catch (e) { channelProvider.deleteChannel(`agent:${name}`); @@ -683,8 +691,18 @@ export function createAgentServerConnectionHandler( .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) + }`, + ); }); } } @@ -695,8 +713,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..5d1a49af20 100644 --- a/ts/packages/agentServer/server/src/conversationManager.ts +++ b/ts/packages/agentServer/server/src/conversationManager.ts @@ -253,8 +253,8 @@ 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 or the `agentInterface` differs, or when the instance is new and + * multi-instance support is switched off. */ addClientAgent( conversationId: string, @@ -265,6 +265,7 @@ export type ConversationManager = { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ): Promise; /** * Remove one instance added via {@link addClientAgent}. The dynamic agent @@ -1220,6 +1221,7 @@ export async function createConversationManager( displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ): Promise { const record = conversations.get(conversationId); if (record === undefined) { @@ -1232,6 +1234,7 @@ export async function createConversationManager( connectionId, appAgent, manifest, + agentInterface, multiInstance, }); debugConversation( diff --git a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts index 69559bc47b..5e4e7007b2 100644 --- a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts @@ -102,6 +102,7 @@ function createTestServer(): TestServer { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ) { await registry.add(host, name, { instanceId, @@ -109,6 +110,7 @@ function createTestServer(): TestServer { connectionId, appAgent, manifest: agentManifest, + agentInterface, multiInstance, }); }, diff --git a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts index 0a56a053c4..b317d7b072 100644 --- a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts @@ -122,6 +122,7 @@ async function register( connectionId: string; appAgent: AppAgent; manifest?: AppAgentManifest; + agentInterface?: readonly string[]; multiInstance?: boolean; }, ): Promise { @@ -131,6 +132,7 @@ async function register( connectionId: options.connectionId, appAgent: options.appAgent, manifest: options.manifest ?? makeManifest(), + agentInterface: options.agentInterface, // Devices opt in; the tests that pin single-host behaviour pass // false explicitly. multiInstance: options.multiInstance ?? true, @@ -257,6 +259,93 @@ describe("clientAgentRegistry registration", () => { expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); + test("a device implementing a different method set is rejected", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + + // Same schema, older build: it cannot answer getDynamicDisplay. The mux + // was built from A's proxy, so without the check B would be routed + // calls it has no method for. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }), + ).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().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + agentInterface: ["getDynamicDisplay", "executeAction"], + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("a client that declares no method set is unaffected by the check", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("a reconnecting instance cannot change the group's method set", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + + // 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. + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }), + ).rejects.toThrow(/different set of methods/i); + }); + // Case 12 test("a client that does not opt in stays the only host of its agent", async () => { const registry = createClientAgentRegistry(); From c971f56ca7c0e5980fe28e1786c14444faf45b4f Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 15:22:54 -0700 Subject: [PATCH 2/4] Require agentInterface, and let a lone device change its method set Follow-ups to the review of the method-set check. agentInterface was optional, so the check skipped whenever it was absent. It cannot be absent: registerClientAgent declares it required and createAgentRpcClient dereferences it to build the proxy, so a registration that reaches the registry always has one. The optional branch was dead code that only weakened the check. It is now required and typed AgentInterfaceFunctionName[] rather than string[], and the two undefined guards are gone. The check also rejected a lone device that upgraded its app: same schema, one more method, and the reconnect failed while its stale instance was still in the group - told to disconnect devices that do not exist. When the registration takes over the group's only instance it now adopts the new set instead. The mux has to be updated in place because the dispatcher keeps the object addDynamicAgent handed it and checks optional methods on it at call time, so replacing group.mux would leave the dispatcher on the old one. Tests: makeDevice now builds a proxy carrying exactly the methods it declares, so the cases exercise the actual misroute instead of just the string comparison. Covers both directions, the empty set, a plain reconnect, and a lone device gaining and losing a method. Mutation-checked: disabling rebuildMux fails only the two mux cases; disabling the rejection fails only the four rejection cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../server/src/clientAgentRegistry.ts | 91 ++++++-- .../server/src/conversationManager.ts | 10 +- .../test/clientAgentIntegration.spec.ts | 3 +- .../server/test/clientAgentRegistry.spec.ts | 207 +++++++++++++++--- 4 files changed, 254 insertions(+), 57 deletions(-) diff --git a/ts/packages/agentServer/server/src/clientAgentRegistry.ts b/ts/packages/agentServer/server/src/clientAgentRegistry.ts index bb33e446d7..f8d028b833 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"; @@ -37,10 +38,10 @@ export type ClientAgentGroup = { /** Hash of the schema source; instances must agree on it. See {@link getManifestKey}. */ manifestKey: string; /** - * Normalized `agentInterface` the group was created with, or undefined when - * the creator did not declare one. See {@link getAgentInterfaceKey}. + * Normalized `agentInterface` of the instances currently in the group. See + * {@link getAgentInterfaceKey}. */ - agentInterfaceKey: string | undefined; + 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 @@ -59,10 +60,12 @@ export type ClientAgentRegistration = { appAgent: AppAgent; manifest: AppAgentManifest; /** - * Methods the client implements. Optional so a client that does not send - * one keeps working; when present it must match the group's. + * 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 string[] | undefined; + agentInterface: readonly AgentInterfaceFunctionName[]; /** See {@link ClientAgentGroup.multiInstance}. Only read on the first registration. */ multiInstance?: boolean; }; @@ -146,8 +149,9 @@ export function schemaMismatchMessage(name: string): string { } /** - * Normalized `agentInterface`, or undefined when the client did not declare - * one. + * 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 @@ -156,11 +160,9 @@ export function schemaMismatchMessage(name: string): string { * to support methods it does not; the call only fails once someone makes it. */ export function getAgentInterfaceKey( - agentInterface: readonly string[] | undefined, -): string | undefined { - return agentInterface === undefined - ? undefined - : [...new Set(agentInterface)].sort().join("\u0000"); + agentInterface: readonly AgentInterfaceFunctionName[], +): string { + return [...new Set(agentInterface)].sort().join("\u0000"); } export function interfaceMismatchMessage(name: string): string { @@ -513,6 +515,27 @@ function createMux(group: ClientAgentGroup, template: AppAgent): AppAgent { return mux as unknown as AppAgent; } +/** + * Point the group's existing mux at a new method set, in place. + * + * The dispatcher was handed this exact object by `addDynamicAgent` and keeps + * that reference, checking each optional method on it at call time. Assigning a + * fresh object to `group.mux` would therefore leave the dispatcher on the old + * one, so the methods have to be swapped onto the object it already holds. + */ +function rebuildMux(group: ClientAgentGroup, template: AppAgent): void { + const next = methodsOf(createMux(group, template)); + const current = methodsOf(group.mux); + for (const method of Object.keys(current)) { + if (next[method] === undefined) { + delete current[method]; + } + } + for (const method of Object.keys(next)) { + current[method] = next[method]; + } +} + /** * 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. @@ -579,6 +602,25 @@ export function createClientAgentGroup( return group; } +/** + * True when this registration takes over the group's only instance: either the + * same `instanceId` coming back, or the same connection re-registering under a + * new one (its old proxy died when the new one claimed the `agent:` + * channel). Either way no other device is in the group, so the method set is + * this device's alone to change. + */ +function replacesSoleInstance( + group: ClientAgentGroup, + registration: ClientAgentRegistration, +): boolean { + return ( + group.instances.size === 1 && + (group.instances.has(registration.instanceId) || + findInstanceIdForConnection(group, registration.connectionId) !== + undefined) + ); +} + /** * 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 @@ -597,15 +639,22 @@ export async function joinClientAgentGroup( // Checked alongside the schema, and for a replacement too: the mux was // built from the method set of whichever proxy created the group, so an // instance that arrives with a different one would be routed calls it - // cannot answer. Only compared when both sides declared an interface, so a - // client that sends none keeps working. + // cannot answer. const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); - if ( - agentInterfaceKey !== undefined && - group.agentInterfaceKey !== undefined && - agentInterfaceKey !== group.agentInterfaceKey - ) { - throw new Error(interfaceMismatchMessage(group.name)); + if (agentInterfaceKey !== group.agentInterfaceKey) { + // Unless this device is the whole group. A lone device that upgrades + // its app keeps its schema but can gain or lose methods, and rejecting + // that would break a reconnect that used to work - with a message + // telling the user to disconnect devices that do not exist. Nobody else + // is sharing the name, so adopt the new set and bring the mux with it. + if (!replacesSoleInstance(group, registration)) { + throw new Error(interfaceMismatchMessage(group.name)); + } + group.agentInterfaceKey = agentInterfaceKey; + rebuildMux(group, registration.appAgent); + debugGroup( + `${group.name}: sole instance ${registration.instanceId} changed the method set; mux rebuilt`, + ); } const existing = group.instances.get(registration.instanceId); diff --git a/ts/packages/agentServer/server/src/conversationManager.ts b/ts/packages/agentServer/server/src/conversationManager.ts index 5d1a49af20..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 or the `agentInterface` 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,7 +267,7 @@ export type ConversationManager = { displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise; /** * Remove one instance added via {@link addClientAgent}. The dynamic agent @@ -1221,7 +1223,7 @@ export async function createConversationManager( displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise { const record = conversations.get(conversationId); if (record === undefined) { diff --git a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts index 5e4e7007b2..76e7cba95a 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 { @@ -102,7 +103,7 @@ function createTestServer(): TestServer { displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ) { await registry.add(host, name, { instanceId, diff --git a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts index b317d7b072..42bcd51309 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,18 +45,44 @@ 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, }; } @@ -122,7 +149,7 @@ async function register( connectionId: string; appAgent: AppAgent; manifest?: AppAgentManifest; - agentInterface?: readonly string[]; + agentInterface?: readonly AgentInterfaceFunctionName[]; multiInstance?: boolean; }, ): Promise { @@ -132,7 +159,13 @@ async function register( connectionId: options.connectionId, appAgent: options.appAgent, manifest: options.manifest ?? makeManifest(), - agentInterface: options.agentInterface, + // 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, @@ -259,26 +292,52 @@ describe("clientAgentRegistry registration", () => { expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); - test("a device implementing a different method set is rejected", async () => { + 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: makeDevice().appAgent, - agentInterface: ["executeAction"], + 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(); - // Same schema, older build: it cannot answer getDynamicDisplay. The mux - // was built from A's proxy, so without the check B would be routed - // calls it has no method for. + // 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().appAgent, - agentInterface: ["executeAction", "getDynamicDisplay"], + 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); @@ -291,59 +350,145 @@ describe("clientAgentRegistry registration", () => { await register(registry, host, { instanceId: "a", connectionId: "conn-a", - appAgent: makeDevice().appAgent, + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, agentInterface: ["executeAction", "getDynamicDisplay"], }); await register(registry, host, { instanceId: "b", connectionId: "conn-b", - appAgent: makeDevice().appAgent, + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, agentInterface: ["getDynamicDisplay", "executeAction"], }); expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); - test("a client that declares no method set is unaffected by the check", async () => { + 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().appAgent, - agentInterface: ["executeAction"], + 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: "b", - connectionId: "conn-b", - appAgent: makeDevice().appAgent, + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, }); - expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + expect(host.added).toEqual([AGENT_NAME]); }); - test("a reconnecting instance cannot change the group's method set", async () => { + 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().appAgent, - agentInterface: ["executeAction"], + appAgent: makeDevice(["executeAction"]).appAgent, + }); + expect(getMux(registry).getDynamicDisplay).toBeUndefined(); + + // Same device, same schema, new build that implements one more method. + // Nobody else is in the group, so there is no other device to conflict + // with and nothing for the user to disconnect. + 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); + // The dispatcher still holds the object it was handed, so the new + // method has to show up on that same mux and route to the device. + const { context } = makeSessionContext("conn-a2"); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + await getMux(registry).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, + }); + 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. + expect(getMux(registry).getDynamicDisplay).toBeUndefined(); + expect(getMux(registry).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().appAgent, - agentInterface: ["executeAction", "getDynamicDisplay"], + appAgent: makeDevice(["executeAction"]).appAgent, }), ).rejects.toThrow(/different set of methods/i); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); }); // Case 12 From b65af8eac7a592172f4bd05c73bf354ddea782db Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 16:23:32 -0700 Subject: [PATCH 3/4] Derive the Android agentInterface from the methods it dispatches The Android client hardcoded its agentInterface as ["executeAction"] while handleAndroidDeviceInvoke separately hardcoded the same string as its guard. Nothing tied the two together, and no CI job builds this module, so adding a method to one and not the other would go unnoticed. That drift is exactly what the server now rejects at join time, and it fails in the worse direction: a device that declares a method it cannot answer is routed the call and fails only when someone makes it. Both now come from AndroidDeviceAgent.SUPPORTED_METHODS. The test asserts the declared array against that list rather than a literal, and pins that an unimplemented method is not claimed - widening the list without adding dispatch fails the test. Verified by hand, since nothing in CI covers android/: gradlew assembleDebug and testDebugUnitTest both pass (171 tests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../typeagentchat/AndroidDeviceAgent.kt | 23 ++++++++++++++++++- .../example/typeagentchat/WebSocketManager.kt | 5 +++- .../typeagentchat/AndroidDeviceAgentTest.kt | 14 +++++++---- 3 files changed, 36 insertions(+), 6 deletions(-) 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 From e7633d368916abea701422e4d127b09f0725e98f Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Thu, 10 Sep 2026 12:29:51 -0700 Subject: [PATCH 4/4] Make client agent replacement atomic Keep old and new RPC endpoints distinct during re-registration so replacements can be validated before the previous channel is retired. Refresh or replace lifecycle state with rollback, clean up partial initialization, and strengthen the mux and failure-path coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ts/packages/agentRpc/src/client.ts | 24 +- ts/packages/agentRpc/src/server.ts | 21 +- .../agentRpc/test/actionContext.spec.ts | 2 + .../client/src/agentServerClient.ts | 96 ++++- .../agentServer/protocol/src/protocol.ts | 19 +- .../server/src/clientAgentRegistry.ts | 343 ++++++++++++----- .../server/src/connectionHandler.ts | 101 +++-- .../server/src/sharedDispatcher.ts | 81 ++++ .../test/clientAgentIntegration.spec.ts | 121 +++++- .../server/test/clientAgentRegistry.spec.ts | 349 +++++++++++++++++- 10 files changed, 997 insertions(+), 160 deletions(-) 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 f8d028b833..695d4360c6 100644 --- a/ts/packages/agentServer/server/src/clientAgentRegistry.ts +++ b/ts/packages/agentServer/server/src/clientAgentRegistry.ts @@ -512,68 +512,146 @@ 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; } /** - * Point the group's existing mux at a new method set, in place. - * - * The dispatcher was handed this exact object by `addDynamicAgent` and keeps - * that reference, checking each optional method on it at call time. Assigning a - * fresh object to `group.mux` would therefore leave the dispatcher on the old - * one, so the methods have to be swapped onto the object it already holds. + * 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. */ -function rebuildMux(group: ClientAgentGroup, template: AppAgent): void { - const next = methodsOf(createMux(group, template)); - const current = methodsOf(group.mux); - for (const method of Object.keys(current)) { - if (next[method] === undefined) { - delete current[method]; - } +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; } - for (const method of Object.keys(next)) { - current[method] = next[method]; + 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); + } } } -/** - * 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( group: ClientAgentGroup, instance: ClientAgentInstance, +): Promise { + try { + 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 = getContexts(group, instance.instanceId); + 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.initializeAgentContext !== undefined) { - contexts.agentContext = await appAgent.initializeAgentContext(); - contexts.agentContextSet = true; + if (appAgent.stopBackgroundTasks !== undefined) { + await appAgent.stopBackgroundTasks(view); + backgroundStopped = true; + } + for (const schemaName of state.enabledSchemas) { + await appAgent.updateAgentContext?.(false, view, schemaName); + disabledSchemas.push(schemaName); } - const sessionContext = state.sessionContext; - if (sessionContext !== undefined) { - const view = viewSessionContext(contexts, sessionContext); - await appAgent.startBackgroundTasks?.(view); - for (const schemaName of state.enabledSchemas) { + 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, @@ -581,6 +659,13 @@ 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, @@ -602,22 +687,75 @@ export function createClientAgentGroup( return group; } -/** - * True when this registration takes over the group's only instance: either the - * same `instanceId` coming back, or the same connection re-registering under a - * new one (its old proxy died when the new one claimed the `agent:` - * channel). Either way no other device is in the group, so the method set is - * this device's alone to change. - */ -function replacesSoleInstance( +function replacementInstanceId( group: ClientAgentGroup, registration: ClientAgentRegistration, -): boolean { - return ( - group.instances.size === 1 && - (group.instances.has(registration.instanceId) || - findInstanceIdForConnection(group, registration.connectionId) !== - undefined) +): 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}`, ); } @@ -636,58 +774,18 @@ export async function joinClientAgentGroup( throw new Error(schemaMismatchMessage(group.name)); } - // Checked alongside the schema, and for a replacement too: the mux was - // built from the method set of whichever proxy created the group, so an - // instance that arrives with a different one would be routed calls it - // cannot answer. const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); if (agentInterfaceKey !== group.agentInterfaceKey) { - // Unless this device is the whole group. A lone device that upgrades - // its app keeps its schema but can gain or lose methods, and rejecting - // that would break a reconnect that used to work - with a message - // telling the user to disconnect devices that do not exist. Nobody else - // is sharing the name, so adopt the new set and bring the mux with it. - if (!replacesSoleInstance(group, registration)) { - throw new Error(interfaceMismatchMessage(group.name)); - } - group.agentInterfaceKey = agentInterfaceKey; - rebuildMux(group, registration.appAgent); - debugGroup( - `${group.name}: sole instance ${registration.instanceId} changed the method set; mux rebuilt`, - ); + throw new Error(interfaceMismatchMessage(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 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. @@ -697,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. */ @@ -770,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; }; @@ -811,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; } @@ -829,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 4dad7ba972..3985364d7c 100644 --- a/ts/packages/agentServer/server/src/connectionHandler.ts +++ b/ts/packages/agentServer/server/src/connectionHandler.ts @@ -251,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 @@ -590,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, @@ -619,15 +639,42 @@ export function createAgentServerConnectionHandler( 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( @@ -643,7 +690,7 @@ export function createAgentServerConnectionHandler( // someone else's agent. const instanceId = param.instanceId ?? - tracked ?? + tracked?.instanceId ?? conversationManager.findClientAgentInstance( conversationId, name, @@ -665,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); } }, @@ -686,7 +735,10 @@ 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, @@ -703,7 +755,10 @@ export function createAgentServerConnectionHandler( e instanceof Error ? e.message : String(e) }`, ); - }); + }) + .finally(() => + channelProvider.deleteChannel(channelName), + ); } } clientAgents.clear(); 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 76e7cba95a..a6929b3cd7 100644 --- a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts @@ -53,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; }; @@ -67,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); }, @@ -193,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, @@ -333,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. @@ -349,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 42bcd51309..1b4f461799 100644 --- a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts @@ -88,16 +88,19 @@ function makeDevice( 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) { @@ -107,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); @@ -417,11 +431,9 @@ describe("clientAgentRegistry registration", () => { connectionId: "conn-a", appAgent: makeDevice(["executeAction"]).appAgent, }); - expect(getMux(registry).getDynamicDisplay).toBeUndefined(); + const dispatcherMux = host.registered.get(AGENT_NAME)!; + expect(dispatcherMux.getDynamicDisplay).toBeUndefined(); - // Same device, same schema, new build that implements one more method. - // Nobody else is in the group, so there is no other device to conflict - // with and nothing for the user to disconnect. const upgraded = makeDevice(["executeAction", "getDynamicDisplay"]); await register(registry, host, { instanceId: "a", @@ -430,11 +442,15 @@ describe("clientAgentRegistry registration", () => { }); expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); - // The dispatcher still holds the object it was handed, so the new - // method has to show up on that same mux and route to the device. + 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(getMux(registry).getDynamicDisplay).toBeDefined(); - await getMux(registry).getDynamicDisplay!("html", "display-1", context); + expect(reloadedMux.getDynamicDisplay).toBeDefined(); + await reloadedMux.getDynamicDisplay!("html", "display-1", context); expect(upgraded.dynamicDisplays).toEqual(["display-1"]); }); @@ -448,6 +464,7 @@ describe("clientAgentRegistry registration", () => { appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) .appAgent, }); + const dispatcherMux = host.registered.get(AGENT_NAME)!; await register(registry, host, { instanceId: "a", connectionId: "conn-a2", @@ -455,8 +472,14 @@ describe("clientAgentRegistry registration", () => { }); // Leaving it on the mux would advertise a method no device can answer. - expect(getMux(registry).getDynamicDisplay).toBeUndefined(); - expect(getMux(registry).executeAction).toBeDefined(); + 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 () => { @@ -491,6 +514,312 @@ describe("clientAgentRegistry registration", () => { 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();