Skip to content

Commit 2abb77f

Browse files
willwashburnclaude
andcommitted
Address review: settle registration on reconnect exhaustion; null-safe reply
- All three clients reject the registration waiter when reconnects are exhausted before registering, so serve()/whenRegistered() no longer hang forever. - TS onRegistered tolerates a null/undefined reply data (treats it as an invalid reply hard-fail) instead of throwing a TypeError that misroutes to reconnect. - Guard the Python reconnect tests against a race reading the re-register frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8afe631 commit 2abb77f

5 files changed

Lines changed: 35 additions & 6 deletions

File tree

packages/sdk-python/src/relay_sdk/node.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -421,9 +421,13 @@ async def _reader_loop(self, conn: NodeConnection) -> None:
421421

422422
async def _backoff_sleep(self) -> None:
423423
if self._max_reconnect_attempts is not None and self._reconnect_attempt >= self._max_reconnect_attempts:
424-
self._report_error(NodeProviderError("node provider exhausted reconnect attempts"))
424+
err = NodeProviderError("node provider exhausted reconnect attempts")
425+
self._report_error(err)
426+
# If registration never completed, reject its waiter too — otherwise
427+
# wait_registered() would hang forever after reconnects are exhausted.
428+
self._reject_registered(err)
425429
self._stopped = True
426-
return
430+
raise err
427431
self._reconnect_attempt += 1
428432
exp = min(self._reconnect_base_delay * (2 ** (self._reconnect_attempt - 1)), self._reconnect_max_delay)
429433
delay = exp * (0.5 + random.random()) if self._reconnect_jitter else exp

packages/sdk-python/tests/test_node.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,7 @@ async def test_reconnects_with_new_instance_id_after_unexpected_drop():
357357
first.drop()
358358
second = await server.next_connection()
359359
assert second is not first
360+
await wait_until(lambda: len(second.sent_of_type("node.register")) >= 1)
360361
second_register = second.last_register()
361362
assert second_register["name"] == "alpha"
362363
assert second_register["provider"]["name"] == "py"
@@ -384,6 +385,7 @@ async def test_reconnects_when_dropped_during_register_handshake():
384385

385386
second = await server.next_connection()
386387
assert second is not first
388+
await wait_until(lambda: len(second.sent_of_type("node.register")) >= 1)
387389
second.push(accept_all(second.last_register()))
388390
await asyncio.wait_for(node.wait_registered(), timeout=1.0)
389391
assert node.connected is True

packages/sdk-swift/Sources/Relaycast/NodeProvider.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,10 @@ public actor NodeProvider {
434434
let error = NodeProviderError.reconnectAttemptsExhausted
435435
stopped = true
436436
reportError(error)
437+
// If registration never completed, fail its waiter too —
438+
// otherwise waitRegistered() would hang forever after
439+
// reconnects are exhausted.
440+
settleRegistration(.failure(error))
437441
throw error
438442
}
439443
reconnectAttempt += 1

packages/sdk-typescript/src/__tests__/node-provider.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,20 @@ describe('NodeProviderClient', () => {
279279
expect(node.connected).toBe(true);
280280
});
281281

282+
it('rejects whenRegistered when reconnects are exhausted before registering', async () => {
283+
const node = new NodeProviderClient({ ...baseOptions, maxReconnectAttempts: 1 });
284+
node.serve().catch(() => {});
285+
const first = newSocket();
286+
first.open();
287+
first.drop(); // dropped mid-handshake -> reconnect
288+
await vi.advanceTimersByTimeAsync(50);
289+
const second = newSocket();
290+
second.open();
291+
second.drop(); // second attempt drops -> reconnects exhausted
292+
await vi.advanceTimersByTimeAsync(50);
293+
await expect(node.whenRegistered()).rejects.toThrow(/exhausted/i);
294+
});
295+
282296
it('deregisters the provider on graceful stop', async () => {
283297
const node = new NodeProviderClient(baseOptions);
284298
node.serve();

packages/sdk-typescript/src/node-provider.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -351,10 +351,10 @@ export class NodeProviderClient {
351351

352352
private onRegistered(data: NodeRegisterReplyData): void {
353353
// The reply must carry a well-formed acceptance list (the engine always
354-
// does). A missing/corrupt list means acceptance can't be confirmed, so
355-
// fail registration rather than assume success; an entry missing `accepted`
356-
// counts as rejected (fail-safe).
357-
const list = data.accepted_capabilities;
354+
// does). A missing/corrupt reply (including a null `data`) means acceptance
355+
// can't be confirmed, so fail registration rather than assume success or
356+
// throw; an entry missing `accepted` counts as rejected (fail-safe).
357+
const list = (data ?? ({} as NodeRegisterReplyData)).accepted_capabilities;
358358
if (!Array.isArray(list)) {
359359
this.onRegisterFailed(new NodeRegistrationError(
360360
'Registration reply is missing accepted_capabilities',
@@ -546,6 +546,11 @@ export class NodeProviderClient {
546546
const err = new Error('node provider exhausted reconnect attempts');
547547
this.reportError(err);
548548
this.stopped = true;
549+
// If registration never completed, reject its waiter too — otherwise
550+
// whenRegistered() would hang forever after reconnects are exhausted.
551+
this.rejectRegister?.(err);
552+
this.rejectRegister = null;
553+
this.resolveRegister = null;
549554
this.settleServe(err);
550555
return;
551556
}

0 commit comments

Comments
 (0)