Skip to content

Commit 31b0490

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 43e06e7 commit 31b0490

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
@@ -849,7 +849,15 @@ export class Client extends Protocol<ClientContext> {
849849
...(config.resources && { resourcesListChanged: true as const })
850850
};
851851
if (Object.keys(filter).length > 0) {
852-
this._autoOpenedSubscription = await this.listen(filter, options);
852+
// A failed auto-open MUST NOT fail connect: the modern
853+
// connection is fully usable without a listen stream (the
854+
// server may not support it, or refuse on capacity). Surface
855+
// via onerror; the consumer can call listen() later.
856+
try {
857+
this._autoOpenedSubscription = await this.listen(filter, options);
858+
} catch (error) {
859+
this.onerror?.(error instanceof Error ? error : new Error(String(error)));
860+
}
853861
}
854862
}
855863
}
@@ -1210,13 +1218,20 @@ export class Client extends Protocol<ClientContext> {
12101218
{ method: 'subscriptions/listen', protocolVersion: negotiated }
12111219
);
12121220
}
1221+
// Connectivity is checked here so the rejection is delivered as the
1222+
// returned promise (no setup or ack timer is started) — `_parkRequest`
1223+
// would otherwise throw NotConnected from inside the executor below
1224+
// after the timer is armed.
1225+
if (this.transport === undefined) {
1226+
throw new SdkError(SdkErrorCode.NotConnected, 'Not connected');
1227+
}
12131228

12141229
if (this._onParkedNotification === undefined) {
12151230
this._onParkedNotification = raw => this._listenFirstLook(raw);
12161231
}
12171232

12181233
const requestAbort = new AbortController();
1219-
const transportKind = this.transport === undefined ? 'http' : detectProbeTransportKind(this.transport);
1234+
const transportKind = detectProbeTransportKind(this.transport);
12201235

12211236
let closed = false;
12221237
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)