From a35451f5b91e693ef05cbc80dcc94c75518c6e85 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:37:42 -0400 Subject: [PATCH] fix(relay): stop self-closing the control socket on unknown messages (#18400) * fix(relay): stop self-closing the control socket on unknown messages The desktop control client tore its own relay control WebSocket down with code 4401 "unknown control message" for any well-formed control frame it did not recognize. handleMessage() funneled everything that was not ping / conn-open / drain / a tracked request reply into failProtocol('unknown control message'), which closes the socket and orphans the origin. Three real frames hit that branch: - A relay reply that arrives after the desktop's 10s request deadline already deleted the pending entry. Relay control operations run DB transactions that can exceed 10s under load, so resolveMessage() finds no waiter and returns false. - A control-error carrying no reqId (or an unknown one), including the relay's own 'unknown_control_message' reply to a host command it could not route. - A newer relay's opcode that this build predates. Fleet telemetry shows ~15 of these closes per day across app versions 1.4.175..1.4.197, so it is version-agnostic. The self-close was also far more costly than the message that caused it: the relay session dropped to 'orphaned' and answered the phone with HOST_OFFLINE (4404) for the orphan grace window, then the desktop had to re-register through the director's 503 reconnect throttle, stretching a single stray frame into minutes of mobile downtime. Per docs/reference/remote-wire-compatibility.md Rule 2, an unknown but well-formed control frame must be dropped, not treated as fatal. Log and ignore it; malformed JSON, binary frames, and messages before activation still close as protocol violations. Adds unit tests for the unknown-opcode drop, the timed-out-reply drop, and the preserved malformed-frame teardown. * docs(relay): correct the ignore rationale, drop the Rule 2 misattribution Rule 2 of remote-wire-compatibility governs the SENDER of a new terminal- stream opcode and treats the receiver's silent drop as a hazard, not a mandate. Reframe the comment around the actual justification: the decoder convention of dropping unknown frames, the control channel's lack of an opcode negotiation step, and the incident cost asymmetry. --- .../relay/relay-control-client.test.ts | 45 +++++++++++++++++++ .../runtime/relay/relay-control-client.ts | 13 +++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/main/runtime/relay/relay-control-client.test.ts b/src/main/runtime/relay/relay-control-client.test.ts index 2975b67e631..d235f0ebacd 100644 --- a/src/main/runtime/relay/relay-control-client.test.ts +++ b/src/main/runtime/relay/relay-control-client.test.ts @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import nacl from 'tweetnacl' import { WebSocketServer, type WebSocket } from 'ws' import type { E2EEKeypair } from '../e2ee-keypair' +import { MOBILE_RELAY_CLOSE_CODE } from '../../../shared/mobile-relay-close-codes' import { RelayControlClient } from './relay-control-client' const encoder = new TextEncoder() @@ -550,4 +551,48 @@ describe('RelayControlClient scripted-socket lifecycle', () => { vi.advanceTimersByTime(91_000) expect(client.isLive()).toBe(false) }) + + it('ignores an unrecognized control message without closing the active control', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { client, socket, onClose } = scriptedControl() + await client.connect() + expect(client.isLive()).toBe(true) + + // A newer relay opcode the desktop schema does not know. Rule 2 of + // remote-wire-compatibility: an unknown-but-well-formed frame is dropped, + // never fatal to a live control. + socket.deliver({ type: 'relay-hint', v: 2, hint: 'future-feature' }) + + expect(client.isLive()).toBe(true) + expect(socket.readyState).toBe(1) + expect(onClose).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('ignores a reply whose request already timed out instead of self-closing', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { client, socket, onClose } = scriptedControl() + await client.connect() + + // A relay control-error carrying a reqId with no live waiter — e.g. a late + // reply that arrived after the desktop's request deadline deleted it, or the + // relay's no-op error for a command it could not route. Must not be fatal. + socket.deliver({ type: 'control-error', reqId: 'expired-req', code: 'unknown_control_message' }) + + expect(client.isLive()).toBe(true) + expect(socket.readyState).toBe(1) + expect(onClose).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('still tears down a malformed (non-JSON) control frame', async () => { + const { client, socket, onClose } = scriptedControl() + await client.connect() + + socket.emit('message', 'not-json{', false) + + expect(client.isLive()).toBe(false) + expect(socket.readyState).toBe(3) + expect(onClose).toHaveBeenCalledWith(MOBILE_RELAY_CLOSE_CODE.BAD_OUTER_CREDENTIAL) + }) }) diff --git a/src/main/runtime/relay/relay-control-client.ts b/src/main/runtime/relay/relay-control-client.ts index 76816c81a3a..7e742173f72 100644 --- a/src/main/runtime/relay/relay-control-client.ts +++ b/src/main/runtime/relay/relay-control-client.ts @@ -234,7 +234,18 @@ export class RelayControlClient { if (this.requests.resolveMessage(message)) { return } - this.failProtocol('unknown control message') + // Drop a well-formed control message we do not recognize, matching how every + // other Orca decoder treats an unknown frame (see the silent-drop convention + // in docs/reference/remote-wire-compatibility.md). The control channel has no + // opcode negotiation step, so this reaches either a newer relay's message + // this build predates, or a reply whose request already timed out and has no + // waiter (relay control ops run DB transactions that can exceed the request + // deadline under load). Self-closing here was strictly worse than ignoring: + // it orphaned the relay session, which answered the phone with HOST_OFFLINE + // for the orphan-grace window plus the director's reconnect throttle — minutes + // of outage from a single stray frame. + const messageType = typeof message.type === 'string' ? message.type : 'unknown' + console.warn(`[relay] ignoring unrecognized control message type=${messageType}`) } private handleProofMessage(message: Record): void {