Skip to content

Commit ab89147

Browse files
alexyakuninclaude
andcommitted
fix(rpc): addClient extends the shared proxy instead of dropping methods (F8)
The F8 client-proxy cache in RpcHub.addClient was keyed by (peer, service name) only, so a second addClient for the same service name returned the first proxy verbatim. When two call sites register different defs under one name — as ts/e2e/ts-dotnet-perf.ts does, splitting ITypeScriptTestComputeService into a call-methods def (Add/GetValue) and a stream def (StreamInt32) — the later def's methods were silently missing from the returned proxy, failing the .NET-driven StreamPerformance test with "svc.StreamInt32 is not a function". addClient now merges each def into a single shared proxy per (peer, name): methods absent from the proxy are added (superset extends it), preserving F8's singleton-proxy guarantee (same proxy object, one ComputeFunction per method, one shared computed/invalidation stream). A genuinely conflicting re-registration (same method name + argCount, different wire signature) throws a clear error, matching C#'s ServiceNameConflict/MethodNameConflict spirit — the API fails loudly rather than serving a method-less proxy silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SDiBQM5rnhyBG1asuP1kDi
1 parent d9f6acd commit ab89147

3 files changed

Lines changed: 155 additions & 50 deletions

File tree

docs/plans/ts-port-audit.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,7 @@ Confidence: confirmed.
905905

906906
### F8. No deduplication of remote computeds across proxies — each `addClient` mints a fresh key space
907907

908-
Status: **closed** — fixed 2026-07-15 (batch fusionrpc3; proxy cached per `(peer, service name)` in the base `RpcHub.addClient`).
908+
Status: **closed** — fixed 2026-07-15 (batch fusionrpc3; proxy cached per `(peer, service name)` in the base `RpcHub.addClient`). Refined same day (batch e2efix) after the .NET-driven e2e caught a regression: the cache silently dropped methods when a second, different def used the same service name (`ts-dotnet-perf.ts` registers disjoint defs) — `addClient` now merges defs into one shared proxy (existing methods and their `ComputeFunction`s preserved; different arities coexist as overloads, wire-name parity) and throws on genuinely conflicting signatures, validate-first so a rejected def changes nothing.
909909

910910
Confidence: confirmed.
911911

ts/packages/fusion-rpc/tests/fusion-rpc-glue-fixes.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,64 @@ describe('Fusion-over-RPC glue fixes', () => {
176176
clientConn.close();
177177
});
178178

179+
// F8 — a later def registered under the same service name must extend the
180+
// shared proxy, not silently return the first (method-less-for-the-caller)
181+
// one. Mirrors ts-dotnet-perf.ts splitting ITypeScriptTestComputeService
182+
// into a call-methods def and a stream def.
183+
it('extends the shared proxy when a later def adds methods under the same service name', () => {
184+
const clientPeer = new RpcClientPeer(clientHub, 'ws://test');
185+
clientHub.addPeer(clientPeer);
186+
187+
const callsDef = defineComputeService('SplitService', {
188+
Add: { args: [0, 0], callTypeId: 0 },
189+
GetValue: { args: [0] },
190+
});
191+
const streamDef = defineRpcService('SplitService', {
192+
StreamInt32: { args: [0], returns: RpcType.stream, wireArgCount: 1 },
193+
});
194+
195+
const svc = clientHub.addClient<Record<string, unknown>>(clientPeer, callsDef);
196+
const streamSvc = clientHub.addClient<Record<string, unknown>>(clientPeer, streamDef);
197+
198+
expect(streamSvc).toBe(svc);
199+
expect(typeof svc['Add']).toBe('function');
200+
expect(typeof svc['GetValue']).toBe('function');
201+
expect(typeof svc['StreamInt32']).toBe('function');
202+
expect(typeof streamSvc['StreamInt32']).toBe('function');
203+
});
204+
205+
it('throws on a conflicting re-registration of a method under the same service name', () => {
206+
const clientPeer = new RpcClientPeer(clientHub, 'ws://test');
207+
clientHub.addPeer(clientPeer);
208+
209+
const computeDef = defineComputeService('ConflictService', { Value: { args: [0] } });
210+
const rpcDef = defineRpcService('ConflictService', { Value: { args: [0] } });
211+
212+
clientHub.addClient(clientPeer, computeDef);
213+
expect(() => clientHub.addClient(clientPeer, rpcDef)).toThrow(/conflicting definition/);
214+
});
215+
216+
it('leaves the shared proxy unchanged when a partially conflicting def is rejected', () => {
217+
const clientPeer = new RpcClientPeer(clientHub, 'ws://test');
218+
clientHub.addPeer(clientPeer);
219+
220+
const baseDef = defineComputeService('PartialService', { Value: { args: [0] } });
221+
const badDef = defineRpcService('PartialService', {
222+
Extra: { args: [0] },
223+
Value: { args: [0] },
224+
});
225+
const goodDef = defineRpcService('PartialService', { Extra: { args: [0] } });
226+
227+
const svc = clientHub.addClient<Record<string, unknown>>(clientPeer, baseDef);
228+
expect(() => clientHub.addClient(clientPeer, badDef)).toThrow(/conflicting definition/);
229+
expect(svc['Extra']).toBeUndefined();
230+
231+
const extended = clientHub.addClient<Record<string, unknown>>(clientPeer, goodDef);
232+
expect(extended).toBe(svc);
233+
expect(typeof svc['Extra']).toBe('function');
234+
expect(typeof svc['Value']).toBe('function');
235+
});
236+
179237
it('mints distinct proxies for different peers', async () => {
180238
const [clientConnA, serverConnA] = createMessageChannelPair();
181239
const [clientConnB, serverConnB] = createMessageChannelPair();

ts/packages/rpc/src/rpc-hub.ts

Lines changed: 96 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,18 @@ const { warnLog } = getLogs('RpcHub');
5757
* `getServerPeer` cast the result, so the factory returns RpcPeer. */
5858
export type RpcPeerFactory = (hub: RpcHub, ref: string) => RpcPeer;
5959

60+
type RpcClientFn = (...args: unknown[]) => unknown;
61+
62+
/** A live client proxy plus the mutable maps it reads from, so a later def can
63+
* add methods to the already-handed-out proxy. `methods` is the map the Proxy's
64+
* get-trap resolves against; `overloads` tracks each method's per-argCount defs
65+
* and functions for overload resolution and conflict detection. */
66+
interface RpcClientProxy {
67+
readonly proxy: object;
68+
readonly methods: Map<string, RpcClientFn>;
69+
readonly overloads: Map<string, Map<number, { def: RpcMethodDef; fn: RpcClientFn }>>;
70+
}
71+
6072
/** Central RPC coordinator — manages peers, services, and configuration. */
6173
export class RpcHub {
6274
readonly hubId: string;
@@ -76,8 +88,11 @@ export class RpcHub {
7688
/** Cache of client proxies keyed by peer, then by service name — so
7789
* repeated {@link addClient} calls for the same service+peer return one
7890
* proxy. All consumers then share a single computed/call/invalidation
79-
* stream per logical value, matching .NET's singleton client proxies (F8). */
80-
private readonly _clientProxies = new WeakMap<RpcPeer, Map<string, object>>();
91+
* stream per logical value, matching .NET's singleton client proxies (F8).
92+
* A later def registered under the same name extends the existing proxy
93+
* with its extra methods (superset), so no consumer ever receives a proxy
94+
* missing methods the def declares. */
95+
private readonly _clientProxies = new WeakMap<RpcPeer, Map<string, RpcClientProxy>>();
8196

8297
/** Connection-lifecycle timing limits. Peers constructed against this hub
8398
* read their initial `*Ms` field values from this instance; later
@@ -183,65 +198,29 @@ export class RpcHub {
183198
this.registry.registerService(def.name, def.methods);
184199
}
185200

186-
/** Create a typed client proxy for a service on a remote peer. */
201+
/** Create a typed client proxy for a service on a remote peer. Repeated calls
202+
* for the same (peer, service name) return one shared proxy; a def carrying
203+
* extra methods extends it (F8). */
187204
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters -- T is used for caller-specified proxy type
188205
addClient<T extends object>(
189206
peer: RpcPeer,
190207
defOrContract: RpcServiceDef | (AnyConstructor)
191208
): T {
192209
const def = this._resolveServiceDef(defOrContract);
210+
this.registry.registerService(def.name, def.methods);
193211

194212
let byService = this._clientProxies.get(peer);
195213
if (byService === undefined) {
196-
byService = new Map<string, object>();
214+
byService = new Map<string, RpcClientProxy>();
197215
this._clientProxies.set(peer, byService);
198216
}
199-
const cached = byService.get(def.name);
200-
if (cached !== undefined)
201-
return cached as T;
202-
203-
this.registry.registerService(def.name, def.methods);
204-
205-
type RpcClientFn = (...args: unknown[]) => unknown;
206-
207-
// Group methods by clean name, indexed by argCount for overload resolution
208-
const overloads = new Map<string, Map<number, RpcClientFn>>();
209-
for (const methodDef of def.methods.values()) {
210-
let byArgCount = overloads.get(methodDef.name);
211-
if (!byArgCount) {
212-
byArgCount = new Map<number, RpcClientFn>();
213-
overloads.set(methodDef.name, byArgCount);
214-
}
215-
byArgCount.set(
216-
methodDef.argCount,
217-
this._createClientMethod(peer, methodDef)
218-
);
219-
}
220-
221-
// Build final proxy methods — single overload: use directly; multiple: resolve by args.length
222-
const methods = new Map<string, RpcClientFn>();
223-
for (const [name, byArgCount] of overloads) {
224-
if (byArgCount.size === 1) {
225-
const [[, singleFn]] = byArgCount;
226-
methods.set(name, singleFn);
227-
} else {
228-
methods.set(name, (...args: unknown[]) => {
229-
const fn = byArgCount.get(args.length);
230-
if (!fn)
231-
throw new Error(
232-
`No overload of ${name} accepts ${args.length} arguments`
233-
);
234-
return fn(...args);
235-
});
236-
}
217+
let entry = byService.get(def.name);
218+
if (entry === undefined) {
219+
entry = this._createClientProxy();
220+
byService.set(def.name, entry);
237221
}
238-
239-
const proxy = new Proxy({} as T, {
240-
get: (_target, prop) =>
241-
typeof prop === 'string' ? methods.get(prop) : undefined,
242-
});
243-
byService.set(def.name, proxy);
244-
return proxy;
222+
this._extendClientProxy(peer, entry, def);
223+
return entry.proxy as T;
245224
}
246225

247226
close(): void {
@@ -357,4 +336,72 @@ export class RpcHub {
357336
return resolveStreamRefs(result, peer);
358337
};
359338
}
339+
340+
// Private methods
341+
342+
private _createClientProxy(): RpcClientProxy {
343+
const methods = new Map<string, RpcClientFn>();
344+
const proxy = new Proxy({}, {
345+
get: (_target, prop) =>
346+
typeof prop === 'string' ? methods.get(prop) : undefined,
347+
});
348+
return { proxy, methods, overloads: new Map() };
349+
}
350+
351+
private _extendClientProxy(peer: RpcPeer, entry: RpcClientProxy, def: RpcServiceDef): void {
352+
// Validate the whole def before mutating anything, so a conflict
353+
// leaves the shared proxy exactly as it was.
354+
for (const methodDef of def.methods.values()) {
355+
const existing = entry.overloads.get(methodDef.name)?.get(methodDef.argCount);
356+
if (existing !== undefined && !isSameWireMethod(existing.def, methodDef))
357+
throw new Error(
358+
`RpcHub.addClient: conflicting definition of '${wireMethodName(methodDef)}' ` +
359+
`under service '${def.name}' — a different signature is already registered.`);
360+
}
361+
362+
const changed = new Set<string>();
363+
for (const methodDef of def.methods.values()) {
364+
let byArgCount = entry.overloads.get(methodDef.name);
365+
if (byArgCount === undefined) {
366+
byArgCount = new Map();
367+
entry.overloads.set(methodDef.name, byArgCount);
368+
}
369+
if (byArgCount.has(methodDef.argCount))
370+
continue;
371+
372+
byArgCount.set(methodDef.argCount, {
373+
def: methodDef,
374+
fn: this._createClientMethod(peer, methodDef),
375+
});
376+
changed.add(methodDef.name);
377+
}
378+
for (const name of changed) {
379+
const byArgCount = entry.overloads.get(name)!;
380+
entry.methods.set(name, buildOverloadFn(name, byArgCount));
381+
}
382+
}
383+
}
384+
385+
function buildOverloadFn(
386+
name: string,
387+
byArgCount: Map<number, { def: RpcMethodDef; fn: RpcClientFn }>
388+
): RpcClientFn {
389+
if (byArgCount.size === 1) {
390+
const [[, only]] = byArgCount;
391+
return only.fn;
392+
}
393+
return (...args: unknown[]) => {
394+
const overload = byArgCount.get(args.length);
395+
if (overload === undefined)
396+
throw new Error(`No overload of ${name} accepts ${args.length} arguments`);
397+
return overload.fn(...args);
398+
};
399+
}
400+
401+
function isSameWireMethod(a: RpcMethodDef, b: RpcMethodDef): boolean {
402+
return a.wireArgCount === b.wireArgCount
403+
&& a.callTypeId === b.callTypeId
404+
&& a.stream === b.stream
405+
&& a.noWait === b.noWait
406+
&& a.remoteExecutionMode === b.remoteExecutionMode;
360407
}

0 commit comments

Comments
 (0)