Skip to content

Commit 6a8a41d

Browse files
fix(client): guard listen() connectivity before park; auto-open failure surfaces via onerror
Two related hardening fixes: - listen() now checks this.transport before any setup. _parkRequest throws NotConnected synchronously, and it was called inside the ack-promise executor AFTER the ack timer was armed — so a not-connected listen() leaked a timer whose callback then dereferenced an unassigned park handle. The guard returns the rejection as the listen() promise with no setup started. - ClientOptions.listChanged auto-open during _connectNegotiated no longer fails connect when listen() rejects (server refuses on capacity, does not support listen, etc.). The modern connection is fully usable without a listen stream; the failure surfaces via onerror and autoOpenedSubscription stays undefined. Tests added for both.
1 parent a4416a9 commit 6a8a41d

2 files changed

Lines changed: 67 additions & 2 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -799,7 +799,15 @@ export class Client extends Protocol<ClientContext> {
799799
...(config.resources && { resourcesListChanged: true as const })
800800
};
801801
if (Object.keys(filter).length > 0) {
802-
this._autoOpenedSubscription = await this.listen(filter, options);
802+
// A failed auto-open MUST NOT fail connect: the modern
803+
// connection is fully usable without a listen stream (the
804+
// server may not support it, or refuse on capacity). Surface
805+
// via onerror; the consumer can call listen() later.
806+
try {
807+
this._autoOpenedSubscription = await this.listen(filter, options);
808+
} catch (error) {
809+
this.onerror?.(error instanceof Error ? error : new Error(String(error)));
810+
}
803811
}
804812
}
805813
}
@@ -1147,13 +1155,20 @@ export class Client extends Protocol<ClientContext> {
11471155
{ method: 'subscriptions/listen', protocolVersion: negotiated }
11481156
);
11491157
}
1158+
// Connectivity is checked here so the rejection is delivered as the
1159+
// returned promise (no setup or ack timer is started) — `_parkRequest`
1160+
// would otherwise throw NotConnected from inside the executor below
1161+
// after the timer is armed.
1162+
if (this.transport === undefined) {
1163+
throw new SdkError(SdkErrorCode.NotConnected, 'Not connected');
1164+
}
11501165

11511166
if (this._onParkedNotification === undefined) {
11521167
this._onParkedNotification = raw => this._listenFirstLook(raw);
11531168
}
11541169

11551170
const requestAbort = new AbortController();
1156-
const transportKind = this.transport === undefined ? 'http' : detectProbeTransportKind(this.transport);
1171+
const transportKind = detectProbeTransportKind(this.transport);
11571172

11581173
let closed = false;
11591174
let parked!: ReturnType<typeof this._parkRequest>;

packages/client/test/client/listen.test.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,19 @@ describe('Client.listen()', () => {
166166
await client.close();
167167
});
168168

169+
it('rejects with NotConnected (as a rejected promise, no setup) when no transport is connected', async () => {
170+
const { clientTx } = await scriptedModern();
171+
const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } });
172+
await client.connect(clientTx);
173+
await client.close();
174+
// listen() is async, so a pre-send guard throw is delivered as the
175+
// returned promise's rejection (no ack timer started, no park state).
176+
const pending = client.listen({ toolsListChanged: true });
177+
const error = await pending.catch(e => e as SdkError);
178+
expect(error).toBeInstanceOf(SdkError);
179+
expect((error as SdkError).code).toBe(SdkErrorCode.NotConnected);
180+
});
181+
169182
it('ClientOptions.listChanged auto-opens a listen stream on a modern connection (filter derived from sub-options)', async () => {
170183
const filters: unknown[] = [];
171184
const { clientTx } = await scriptedModern((_id, filter) => filters.push(filter));
@@ -181,4 +194,41 @@ describe('Client.listen()', () => {
181194
await client.autoOpenedSubscription!.close();
182195
await client.close();
183196
});
197+
198+
it('a failed auto-open surfaces via onerror and does NOT fail connect', async () => {
199+
const [clientTx, serverTx] = InMemoryTransport.createLinkedPair();
200+
serverTx.onmessage = m => {
201+
const req = m as { id?: number | string; method?: string };
202+
if (req.method === 'server/discover' && req.id !== undefined) {
203+
void serverTx.send({
204+
jsonrpc: '2.0',
205+
id: req.id,
206+
result: {
207+
resultType: 'complete',
208+
supportedVersions: [MODERN],
209+
capabilities: { tools: { listChanged: true } },
210+
serverInfo: { name: 's', version: '1' }
211+
}
212+
});
213+
}
214+
if (req.method === 'subscriptions/listen' && req.id !== undefined) {
215+
// Server refuses listen (capacity guard / not supported).
216+
void serverTx.send({ jsonrpc: '2.0', id: req.id, error: { code: -32_603, message: 'Subscription limit reached' } });
217+
}
218+
};
219+
await serverTx.start();
220+
const onChanged = () => {};
221+
const client = new Client(
222+
{ name: 'c', version: '1' },
223+
{ versionNegotiation: { mode: 'auto' }, listChanged: { tools: { onChanged } } }
224+
);
225+
const errors: Error[] = [];
226+
client.onerror = e => errors.push(e);
227+
// connect MUST resolve: the modern connection is usable without listen.
228+
await client.connect(clientTx);
229+
expect(client.autoOpenedSubscription).toBeUndefined();
230+
expect(errors).toHaveLength(1);
231+
expect((errors[0] as { code?: number }).code).toBe(-32_603);
232+
await client.close();
233+
});
184234
});

0 commit comments

Comments
 (0)