From 854513ee94d7112214997f6e4c33e979b855d100 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Fri, 24 Jul 2026 16:21:06 +0300 Subject: [PATCH 01/13] Remote device: Broadcast+Presence transport (private user channel) Join the private user:{id} channel instead of the shared postgres_changes queue: presence track = live "online" signal, new_call doorbell receive (with row-fetch retry), result doorbell send after the row write. Conditional claim (UPDATE ... eq(status,'pending').select('id')) makes execution exactly-once under dual delivery during the transition. - 30-min bookkeeping last_seen write (presence carries liveness), re-asserts status:'online' only when the channel is joined (no masking a deaf device); clean-shutdown last_seen stamp. - Jittered reconnect backoff + self-heal-during-backoff bail-out; realtime JWT re-auth on token refresh. - capabilities: { transport_broadcast_v1, app_version } for adoption tracking. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/device.ts | 15 +- src/remote-device/remote-channel.ts | 245 +++++++++++++++++- .../scripts/blocking-offline-update.js | 6 +- 3 files changed, 247 insertions(+), 19 deletions(-) diff --git a/src/remote-device/device.ts b/src/remote-device/device.ts index 64e856cd..9581d2fd 100644 --- a/src/remote-device/device.ts +++ b/src/remote-device/device.ts @@ -275,8 +275,14 @@ export class MCPDevice { console.log(`πŸ”§ Received tool call ${call_id}: ${tool_name} ${JSON.stringify(tool_args)} metadata: ${JSON.stringify(metadata)}`); try { - // Update call status to executing - await this.remoteChannel.markCallExecuting(call_id); + // Claim the call. During the transition every call arrives via BOTH + // transports (postgres_changes + broadcast doorbell) β€” only the + // delivery that flips the row pendingβ†’executing may run it. + const claimed = await this.remoteChannel.markCallExecuting(call_id); + if (!claimed) { + // markCallExecuting already logged the duplicate-delivery skip. + return; + } let result; @@ -309,13 +315,16 @@ export class MCPDevice { console.log(`βœ… Tool call ${tool_name} completed:\r\n ${JSON.stringify(result)}`); - // Update database with result + // Update database with result, THEN ring the doorbell β€” the server + // fetches the row by id on the doorbell, so the write must land first. await this.remoteChannel.updateCallResult(call_id, 'completed', result); + await this.remoteChannel.notifyResult(call_id); } catch (error: any) { console.error(`❌ Tool call ${tool_name} failed:`, error.message); await captureRemote('remote_device_tool_call_failed', { error, tool_name }); await this.remoteChannel.updateCallResult(call_id, 'failed', null, error.message); + await this.remoteChannel.notifyResult(call_id); } } diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 0f42478d..62d0e22f 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -1,5 +1,6 @@ import { createClient, SupabaseClient, Session, UserResponse, User, RealtimeChannel } from '@supabase/supabase-js'; import { captureRemote } from '../utils/capture.js'; +import { VERSION } from '../version.js'; export interface AuthSession { @@ -16,7 +17,11 @@ interface DeviceData { last_seen: string; } -const HEARTBEAT_INTERVAL = 15000; +// Bookkeeping cadence for the durable last_seen column. Liveness is carried by +// Presence on the user channel (websocket-level, flips in seconds) β€” this slow +// write only feeds the "last seen X ago" label for offline devices. The server +// sweeps broadcast-capable devices offline after 65 min (2 missed writes + slack). +const HEARTBEAT_INTERVAL = 30 * 60 * 1000; // Cap a single channel recreate so a hung await can't pin the re-entrancy guard // true (which would silently disable the connection watchdog). const RECREATE_TIMEOUT_MS = 30000; @@ -38,7 +43,10 @@ export class RemoteChannel { // Store subscription parameters for channel recreation private deviceId: string | null = null; + private deviceName: string | null = null; private onToolCall: ((payload: any) => void) | null = null; + // Guard so setSession being called twice can't stack auth listeners. + private authListenerRegistered = false; // Track last device status to prevent duplicate log messages private lastDeviceStatus: 'online' | 'offline' = 'offline'; @@ -91,6 +99,22 @@ export class RemoteChannel { this._user = user; console.debug('[DEBUG] Session set successfully, user:', user.email); + // Private channels authorize with the user JWT at join time. supabase-js + // v2 generally forwards auth to realtime on its own β€” these are defensive + // (cheap, and a silent gap here would only surface as a channel dying at + // JWT expiry ~1h in): push the token now and re-push on every refresh. + this.client.realtime.setAuth(session.access_token); + console.debug('[DEBUG] Realtime socket authorized with user JWT'); + if (!this.authListenerRegistered) { + this.authListenerRegistered = true; + this.client.auth.onAuthStateChange((event, newSession) => { + if (event === 'TOKEN_REFRESHED' && newSession?.access_token && this.client) { + console.debug('[DEBUG] Token refreshed β€” re-authorizing realtime socket'); + this.client.realtime.setAuth(newSession.access_token); + } + }); + } + return { error }; } @@ -167,12 +191,18 @@ export class RemoteChannel { await this.updateDevice(existingDevice.id, { status: 'online', last_seen: new Date().toISOString(), - capabilities: {}, // TODO: Capabilities are not yet implemented; keep this empty object for schema compatibility until device capabilities are defined and stored. + // transport_broadcast_v1 = this device joins the private user + // channel (Broadcast doorbells + Presence). The server keys its + // transport choice and offline-sweep tier on this flag. + // app_version rides along so adoption ("are old versions gone + // yet?") is answerable from SQL and PostHog alike. + capabilities: { transport_broadcast_v1: true, app_version: VERSION }, device_name: deviceName }); // Store parameters for channel recreation this.deviceId = existingDevice.id; + this.deviceName = deviceName; this.onToolCall = onToolCall; console.debug(`⏳ Subscribing to tool call channel...`); @@ -203,8 +233,15 @@ export class RemoteChannel { return reject(new Error('Client not initialized or missing subscription parameters')); } - console.debug('[DEBUG] Creating channel: device_tool_call_queue'); - this.channel = this.client.channel('device_tool_call_queue') + // Private per-user channel: carries the legacy postgres_changes + // listener (kept until the fleet-wide flip), the new_call broadcast + // doorbell, and this device's Presence (key = device id, so the + // server and dashboard read liveness straight off presenceState()). + const channelName = `user:${this.user.id}`; + console.debug(`[DEBUG] Creating channel: ${channelName}`); + this.channel = this.client.channel(channelName, { + config: { private: true, presence: { key: this.deviceId ?? undefined } } + }) .on( 'postgres_changes' as any, { @@ -220,6 +257,15 @@ export class RemoteChannel { } } ) + .on( + 'broadcast', + { event: 'new_call' }, + ({ payload }: any) => { + this.onDoorbell(payload).catch((e: any) => { + console.error('[DEBUG] Doorbell handling failed:', e?.message); + }); + } + ) .subscribe((status: string, err: any) => { // Debug: Log all subscription status events console.debug(`[DEBUG] Channel subscription status: ${status}${err ? ' (error: ' + (err?.message || err) + ')' : ''} β€” ${this.connState()}`); @@ -234,12 +280,30 @@ export class RemoteChannel { console.error('Failed to set online status:', e.message); }); } + // Announce presence β€” this IS the live "online" signal for + // the server's dispatch check and the dashboard's green dot. + this.channel?.track({ + device_id: this.deviceId, + device_name: this.deviceName, + app_version: VERSION, + platform: process.platform + }).then(() => { + console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); + captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + }).catch((trackErr: any) => { + console.error('[DEBUG] Presence track failed:', trackErr?.message); + captureRemote('remote_channel_presence_track_error', { error: trackErr?.message }).catch(() => { }); + }); resolve(); } else if (status === 'CHANNEL_ERROR') { // CHANNEL_ERROR is the only status carrying a real error message. console.error(`❌ Channel error: ${err?.message || 'unknown'} β€” ${this.connState()}`); this.setOnlineStatus(this.deviceId!, 'offline'); captureRemote('remote_channel_subscription_error', { error: err?.message || 'Channel error' }).catch(() => { }); + // Distinct fleet-level alarm: if the 008 channel policies were + // ever wrong in prod, this event spiking is the immediate signal + // (fix = SQL policy patch, no client rollback needed). + captureRemote('remote_channel_private_join_failed', { attempt: this.reconnectAttempt, error: err?.message }).catch(() => { }); reject(err || new Error('Failed to initialize tool call channel subscription')); } else if (status === 'TIMED_OUT') { console.error(`⏱️ Channel subscription timed out, Reconnecting... β€” ${this.connState()}`); @@ -258,6 +322,96 @@ export class RemoteChannel { }); } + /** + * Handle a 'new_call' broadcast doorbell. The doorbell carries only ids β€” + * the authoritative row is fetched by primary key and fed through the SAME + * handler as a postgres_changes payload, so device.ts is transport-agnostic. + * During the transition both transports deliver every call; the claim in + * markCallExecuting() guarantees single execution. + */ + private async onDoorbell(payload: any): Promise { + const callId = payload?.call_id; + if (!callId) return; + if (payload?.device_id && payload.device_id !== this.deviceId) { + console.debug('[DEBUG] Ignoring doorbell for different device'); + return; + } + + console.debug('[DEBUG] Doorbell received for call:', callId); + captureRemote('remote_channel_doorbell_received', { tool_name: payload?.tool_name }).catch(() => { }); + + if (!this.client) return; + + // Retry the row fetch on transient failures (observed live: a REST + // blip while the websocket stayed healthy). During the transition the + // legacy postgres_changes delivery covers a lost doorbell, but after + // the flip this fetch is the only way the device learns about the + // call β€” a network hiccup must not cost a 5-minute timeout. + let row: any = null; + let lastError: any = null; + for (const delayMs of [0, 500, 1500]) { + if (delayMs > 0) await this.sleep(delayMs); + const { data, error } = await this.client + .from('mcp_remote_calls') + .select('*') + .eq('id', callId) + .maybeSingle(); + if (!error) { + row = data; + lastError = null; + break; + } + lastError = error; + console.debug(`[DEBUG] Doorbell row fetch attempt failed for ${callId}: ${error.message} β€” retrying`); + } + + if (lastError) { + console.error(`[DEBUG] Doorbell row fetch failed for ${callId} after retries:`, lastError.message); + await captureRemote('remote_channel_doorbell_fetch_error', { error: lastError }); + return; + } + if (!row) { + // Row already claimed+deleted, or cleanup raced delivery β€” nothing to do. + await captureRemote('remote_channel_doorbell_row_missing', {}); + return; + } + if (row.status !== 'pending') { + console.debug('[DEBUG] Doorbell call already claimed via legacy path:', callId); + return; + } + + // Same payload shape as postgres_changes ({ new: row }). + this.onToolCall?.({ new: row }); + } + + /** + * Notify the server that a call's result row is written. Fire-and-forget: + * a skipped/failed send just means the server's 10s recovery poll delivers + * the result instead β€” identical to today's Realtime-hiccup behavior. + * MUST be called only after updateCallResult() has resolved, so the + * server's fetch-by-id finds a terminal row. + */ + async notifyResult(callId: string): Promise { + if (!this.channel || this.channel.state !== 'joined') { + console.debug('[DEBUG] Result doorbell skipped β€” channel not joined (recovery poll covers)'); + return; + } + try { + // realtime-js send() RESOLVES with 'ok' | 'timed out' | 'error' β€” + // it does not reject, so check the status or failures are invisible. + const result = await this.channel.send({ type: 'broadcast', event: 'result', payload: { call_id: callId } }); + if (result === 'ok') { + console.debug('[DEBUG] Result doorbell sent:', callId); + } else { + console.debug(`[DEBUG] Result doorbell not acknowledged (${result}) β€” recovery poll covers:`, callId); + captureRemote('remote_channel_result_doorbell_send_failed', { result }).catch(() => { }); + } + } catch (error: any) { + console.debug('[DEBUG] Result doorbell send failed (recovery poll covers):', error?.message); + captureRemote('remote_channel_result_doorbell_send_failed', { error: error?.message }).catch(() => { }); + } + } + /** * Compact connection state for logs β€” e.g. "socket=open(1) ch=errored attempt=3". * readyState 1=OPEN (a 1 while joins keep failing = a half-open socket being reused), @@ -325,6 +479,10 @@ export class RemoteChannel { * can't leave isRecreatingChannel stuck true and disable the watchdog. Mirrors * closeWithTimeout() in desktop-commander-integration.ts. */ + private sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); + } + private async withTimeout(op: () => Promise, ms: number, name: string): Promise { let timer: NodeJS.Timeout | undefined; try { @@ -362,6 +520,24 @@ export class RemoteChannel { console.log(`πŸ”„ Recreating channel... (attempt ${this.reconnectAttempt}) β€” ${this.connState()}`); try { + // Jittered exponential backoff so a fleet-wide event (server deploy, + // Supabase blip) doesn't stampede every device into reconnecting at + // the same instant. attempt 1 β‰ˆ 1-3s, capped at ~15-45s. The + // re-entrancy guard above keeps the 10s watchdog from stacking + // recreates while we sleep. + const backoffMs = Math.min(30_000, 1000 * 2 ** Math.min(this.reconnectAttempt, 5)) * (0.5 + Math.random()); + console.debug(`[DEBUG] Reconnect backoff: ${Math.round(backoffMs)}ms`); + await this.sleep(backoffMs); + + // realtime-js runs its own rejoin timer, and the backoff above gives + // it a window to win: the old channel can come back 'joined' while we + // slept. Destroying a healthy channel would cause a pointless outage + // cycle β€” bail out instead (observed live on staging, 2026-07-23). + if (this.channel?.state === 'joined') { + console.log(`βœ… Channel self-healed during backoff β€” skipping recreate β€” ${this.connState()}`); + return; // finally-block below clears the re-entrancy guard + } + // Cap the whole recreate: a never-settling await (e.g. a subscribe that only // ever emits CLOSED) must not pin isRecreatingChannel=true and silently disable // the 10s watchdog. On timeout we reject -> catch -> finally clears the guard. @@ -391,19 +567,39 @@ export class RemoteChannel { } } - async markCallExecuting(callId: string) { + /** + * Claim a call for execution. Returns true only when THIS update flipped + * the row from 'pending' to 'executing' β€” during the transition every call + * is delivered twice (postgres_changes + broadcast doorbell), and this + * claim is what guarantees it executes once. The .eq('status','pending') + * makes the claim conditional; .select('id') makes it observable (a + * supabase-js UPDATE returns no row data without it). + * On a transient DB ERROR we return true (execute anyway) β€” matching the + * old behavior, where a failed status write never blocked execution; the + * duplicate-execution window that leaves is no worse than today's. + */ + async markCallExecuting(callId: string): Promise { if (!this.client) throw new Error('Client not initialized'); - const { error } = await this.client + const { data, error } = await this.client .from('mcp_remote_calls') .update({ status: 'executing' }) - .eq('id', callId); + .eq('id', callId) + .eq('status', 'pending') + .select('id'); if (error) { console.error('[DEBUG] Failed to mark call executing:', error.message); await captureRemote('remote_channel_mark_call_executing_error', { error }); - } else { + return true; // preserve legacy behavior: execution proceeds despite the write error + } + + const claimed = !!data && data.length > 0; + if (claimed) { console.debug('[DEBUG] Call marked executing:', callId); + } else { + console.debug('[DEBUG] Call already claimed (duplicate delivery), skipping:', callId); } + return claimed; } async updateCallResult(callId: string, status: string, result: any = null, errorMessage: string | null = null) { @@ -417,7 +613,7 @@ export class RemoteChannel { if (errorMessage !== null) updateData.error_message = errorMessage; console.debug('[DEBUG] Updating call result:', updateData); - const { data, error } = await this.client + const { error } = await this.client .from('mcp_remote_calls') .update(updateData) .eq('id', callId); @@ -426,23 +622,38 @@ export class RemoteChannel { console.error('[DEBUG] Failed to update call result:', error.message); await captureRemote('remote_channel_update_call_result_error', { error }); } else { - console.debug('[DEBUG] Call result updated successfully:', data); + // (an UPDATE without .select() returns no row data β€” log the id) + console.debug('[DEBUG] Call result updated successfully:', callId); } } async updateHeartbeat(deviceId: string) { if (!this.client) return; try { + // Re-assert status:'online' ONLY when the channel is actually joined: + // at a 30-min cadence this beats a lost race with the server's offline + // sweep for a HEALTHY device. But if the channel is dead (CHANNEL_ERROR + // already set the row offline), blindly flipping it back to 'online' + // would mask a deaf device β€” in kill-switch/presence-fallback mode that + // turns a fast-fail into a 5-minute timeout. Always refresh last_seen. + const isJoined = this.channel?.state === 'joined'; + const updates: { last_seen: string; status?: string } = { + last_seen: new Date().toISOString(), + }; + if (isJoined) updates.status = 'online'; + const { error } = await this.client .from('mcp_devices') - .update({ last_seen: new Date().toISOString() }) + .update(updates) .eq('id', deviceId); if (error) { console.error('[DEBUG] Heartbeat update failed:', error.message); await captureRemote('remote_channel_heartbeat_error', { error }); + } else { + // At 30-min cadence this is ~2 lines/hour β€” worth the visibility. + console.debug('[DEBUG] last_seen bookkeeping write ok:', deviceId); } - // console.log(`πŸ”Œ Heartbeat sent for device: ${deviceId}`); } catch (error: any) { console.error('Heartbeat failed:', error.message); await captureRemote('remote_channel_heartbeat_error', { error }); @@ -455,11 +666,11 @@ export class RemoteChannel { this.checkConnectionHealth(); }, 10000); - // Update last_seen every 15 seconds + // Bookkeeping last_seen write (liveness itself rides Presence) this.heartbeatInterval = setInterval(async () => { await this.updateHeartbeat(deviceId); }, HEARTBEAT_INTERVAL); - console.debug('[DEBUG] Heartbeat intervals set - connectionCheck: 10s, heartbeat: 15s'); + console.debug('[DEBUG] Heartbeat intervals set - connectionCheck: 10s, last_seen bookkeeping: 30min'); } stopHeartbeat() { @@ -588,6 +799,12 @@ export class RemoteChannel { async unsubscribe() { if (this.channel) { + // Leave presence explicitly on the graceful path (socket close + // covers the abrupt one). + try { + await this.channel.untrack(); + console.debug('[DEBUG] Presence untracked (graceful leave)'); + } catch { /* best effort */ } await this.channel.unsubscribe(); this.channel = null; console.log('βœ“ Unsubscribed from tool call channel'); diff --git a/src/remote-device/scripts/blocking-offline-update.js b/src/remote-device/scripts/blocking-offline-update.js index 40f03ca2..04a99fae 100644 --- a/src/remote-device/scripts/blocking-offline-update.js +++ b/src/remote-device/scripts/blocking-offline-update.js @@ -41,10 +41,12 @@ try { process.exit(3); // Exit code 3 for auth error } - // Update device status to offline + // Update device status to offline, stamping the exact shutdown moment so + // "last seen X ago" is precise for clean shutdowns (the periodic + // bookkeeping write only runs every 30 minutes). const { error } = await client .from('mcp_devices') - .update({ status: 'offline' }) + .update({ status: 'offline', last_seen: new Date().toISOString() }) .eq('id', deviceId); clearTimeout(timeoutHandle); From 07044d8cba841ddbc90a9082fbbadc2b61dc2547 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Fri, 24 Jul 2026 16:44:37 +0300 Subject: [PATCH 02/13] Remote device: strip NUL bytes before jsonb result write (+ fail-fast) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool result containing a NUL byte (binary file reads, process output) made the jsonb `result` write fail with Postgres 22P05 "unsupported Unicode escape sequence" β€” leaving the call stuck 'executing' so the user waited out a 5-min timeout for a tool that actually ran (~300-350/day in prod). The device is the only writer of the result column, so the fix lives here. - stripNullBytes(): recursively strips NUL from the result before the write, with a fast path that only reparses when a NUL is actually present. - Fail-fast fallback: if a result write still fails for any reason, record a terminal 'failed' with a text-only error_message so the user gets an immediate honest error instead of a phantom timeout. Reproduced end-to-end (null-byte tool output -> stuck row / 22P05) and verified fixed (result stored NUL-free, call completes in ~1s) on staging. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/remote-channel.ts | 35 ++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 62d0e22f..d6659edf 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -2,6 +2,20 @@ import { createClient, SupabaseClient, Session, UserResponse, User, RealtimeChan import { captureRemote } from '../utils/capture.js'; import { VERSION } from '../version.js'; +/** + * Recursively strip NUL (U+0000) from any value destined for a jsonb column. + * jsonb physically cannot hold a NUL and rejects the whole write with Postgres + * 22P05 "unsupported Unicode escape sequence". Fast path: only pay the + * reparse when the serialized form actually contains an escaped NUL + * (JSON.stringify encodes a real NUL as the literal chars backslash-u-0-0-0-0). + */ +export function stripNullBytes(value: T): T { + if (value === null || value === undefined) return value; + const json = JSON.stringify(value); + if (json === undefined || !json.includes('\\u0000')) return value; + return JSON.parse(json.replace(/\\u0000/g, '')) as T; +} + export interface AuthSession { access_token: string; @@ -609,7 +623,12 @@ export class RemoteChannel { completed_at: new Date().toISOString() }; - if (result !== null) updateData.result = result; + // Strip NUL (U+0000) before it reaches the jsonb `result` column. + // jsonb cannot store and rejects the whole write (Postgres 22P05), + // which otherwise leaves the call stuck 'executing' β†’ the user waits out + // a 5-minute timeout for a tool that actually ran. Common with binary + // file reads / process output. error_message is text, so it's exempt. + if (result !== null) updateData.result = stripNullBytes(result); if (errorMessage !== null) updateData.error_message = errorMessage; console.debug('[DEBUG] Updating call result:', updateData); @@ -621,6 +640,20 @@ export class RemoteChannel { if (error) { console.error('[DEBUG] Failed to update call result:', error.message); await captureRemote('remote_channel_update_call_result_error', { error }); + + // Fail-fast fallback: if the RESULT write failed (sanitize should + // prevent the NUL case, but any unstorable payload lands here), + // record a terminal 'failed' with a text-only message so the user + // gets an immediate, honest error instead of a 5-minute phantom + // timeout. Guard against infinite recursion (only for result writes). + if (result !== null && status !== 'failed') { + await this.updateCallResult( + callId, + 'failed', + null, + `Result could not be stored (${error.message})` + ); + } } else { // (an UPDATE without .select() returns no row data β€” log the id) console.debug('[DEBUG] Call result updated successfully:', callId); From d54a02bf5abb7125849363735c667bef0649bab8 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Fri, 24 Jul 2026 21:55:43 +0300 Subject: [PATCH 03/13] Fix review findings: exactly-once execution, NUL sanitizer, test harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 β€” fail-open DB claim could execute a tool twice. markCallExecuting returns true on a transient write error, and during the transition BOTH transports deliver every call, so a correlated REST blip let both deliveries proceed (legacy claims-on-error and runs; broadcast then finds the row still pending, claims cleanly, and runs the same tool again). Side-effecting commands could run twice. Added an in-memory seen-call-id guard in handleNewToolCall: dual delivery is always intra-process, so the local check is authoritative and cannot fail open. The DB claim stays for row-state/cross-restart/observability. P2 β€” stripNullBytes corrupted valid content. It serialized to JSON and regexed the ESCAPE TEXT, so content containing the six literal chars backslash-u-0-0-0-0 (e.g. reading a source file with that escape) was silently altered, and a doubled-backslash form threw SyntaxError -> the call was reported failed though the tool succeeded. Rewritten as a recursive walk over strings/keys that strips real NUL characters only. New test covers both regressions plus the original Postgres 22P05 case. P2 β€” test-remote-channel-reconnect.js was broken by the new channel contract (FakeChannel lacked track/untrack) and is auto-discovered by npm test, so the whole DCMCP suite failed on this branch. Added the presence fakes (resolving with a status string, as realtime-js does) and stubbed the new jittered reconnect sleep so the suite stays sub-second. npm test: 46/46 green. Also: strip NUL from error_message (text rejects it too, and result === null means the fail-fast fallback would not fire); check track()'s resolved status instead of logging every resolution as success; skip untrack() on a non-joined channel so a dead socket can't stall past the 5s shutdown deadline; include call_id in the doorbell-row-missing event. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/device.ts | 39 ++++++++++++-- src/remote-device/remote-channel.ts | 77 +++++++++++++++++++++------ test/test-remote-channel-reconnect.js | 18 +++++++ test/test-strip-null-bytes.js | 77 +++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 19 deletions(-) create mode 100644 test/test-strip-null-bytes.js diff --git a/src/remote-device/device.ts b/src/remote-device/device.ts index 9581d2fd..bc9256f3 100644 --- a/src/remote-device/device.ts +++ b/src/remote-device/device.ts @@ -13,6 +13,14 @@ export interface MCPDeviceOptions { persistSession?: boolean; } +/** + * How many recently-handled call ids to remember for duplicate-delivery + * suppression. Both transports deliver a call within milliseconds of each + * other, so this only needs to outlive that window; 500 is ~minutes of even + * heavy agent traffic and costs a few KB. + */ +const SEEN_CALL_IDS_MAX = 500; + export class MCPDevice { private baseServerUrl: string; private remoteChannel: RemoteChannel; @@ -21,6 +29,8 @@ export class MCPDevice { private configPath: string; private persistSession: boolean; private desktop: DesktopCommanderIntegration; + /** Call ids already handled by THIS process (insertion-ordered, bounded). */ + private seenCallIds: Set = new Set(); constructor(options: MCPDeviceOptions = {}) { this.baseServerUrl = process.env.MCP_SERVER_URL || 'https://mcp.desktopcommander.app'; @@ -259,6 +269,16 @@ export class MCPDevice { // Methods moved to RemoteChannel + /** Record a handled call id, evicting the oldest once the cap is reached. */ + private rememberCallId(callId: string) { + this.seenCallIds.add(callId); + if (this.seenCallIds.size > SEEN_CALL_IDS_MAX) { + // Sets iterate in insertion order β€” drop the oldest entry. + const oldest = this.seenCallIds.values().next().value; + if (oldest !== undefined) this.seenCallIds.delete(oldest); + } + } + async handleNewToolCall(payload: any) { const toolCall = payload.new; // Expect toolCall to include a device_id field used to route calls to this device instance. @@ -274,10 +294,23 @@ export class MCPDevice { console.log(`πŸ”§ Received tool call ${call_id}: ${tool_name} ${JSON.stringify(tool_args)} metadata: ${JSON.stringify(metadata)}`); + // LOCAL claim first β€” this is the authoritative guard against executing + // a call twice. During the transition both transports deliver every call + // to THIS SAME PROCESS, so an in-memory check is sufficient and, unlike + // the DB claim below, cannot fail open: a transient REST error made + // markCallExecuting return true for both deliveries, which could run a + // side-effecting command twice (found in review, 2026-07-24). + if (this.seenCallIds.has(call_id)) { + console.debug('[DEBUG] Duplicate delivery for call already handled here, skipping:', call_id); + return; + } + this.rememberCallId(call_id); + try { - // Claim the call. During the transition every call arrives via BOTH - // transports (postgres_changes + broadcast doorbell) β€” only the - // delivery that flips the row pendingβ†’executing may run it. + // DB claim second β€” keeps the row state machine honest, gives + // cross-restart/cross-process protection, and is observable. It may + // fail open (returns true on a transient write error); the local + // guard above is what makes execution exactly-once. const claimed = await this.remoteChannel.markCallExecuting(call_id); if (!claimed) { // markCallExecuting already logged the duplicate-delivery skip. diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index d6659edf..31f0ab65 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -2,18 +2,39 @@ import { createClient, SupabaseClient, Session, UserResponse, User, RealtimeChan import { captureRemote } from '../utils/capture.js'; import { VERSION } from '../version.js'; +const NUL_CHAR = String.fromCharCode(0); +const NUL_RE = new RegExp(NUL_CHAR, 'g'); + /** - * Recursively strip NUL (U+0000) from any value destined for a jsonb column. - * jsonb physically cannot hold a NUL and rejects the whole write with Postgres - * 22P05 "unsupported Unicode escape sequence". Fast path: only pay the - * reparse when the serialized form actually contains an escaped NUL - * (JSON.stringify encodes a real NUL as the literal chars backslash-u-0-0-0-0). + * Recursively strip real NUL characters (U+0000) from strings and object keys. + * Postgres cannot store a NUL in jsonb OR text and rejects the whole write with + * 22P05, which strands the call at 'executing' until the 5-min timeout. + * + * Walks the structure instead of round-tripping through JSON: an earlier + * serialize-and-regex version matched the ESCAPE TEXT rather than the character, + * so legitimate content containing the six literal chars backslash-u-0-0-0-0 + * (e.g. reading a source file with that escape in it) was silently corrupted, + * and a doubled-backslash form produced invalid JSON that threw. Both cases are + * covered by tests in test/test-strip-null-bytes.js. */ export function stripNullBytes(value: T): T { - if (value === null || value === undefined) return value; - const json = JSON.stringify(value); - if (json === undefined || !json.includes('\\u0000')) return value; - return JSON.parse(json.replace(/\\u0000/g, '')) as T; + if (typeof value === 'string') { + return (value.includes(NUL_CHAR) ? value.replace(NUL_RE, '') : value) as T; + } + if (Array.isArray(value)) { + return value.map((item) => stripNullBytes(item)) as T; + } + if (value && typeof value === 'object') { + // Plain objects only β€” leave Date/Buffer/etc. untouched. + const proto = Object.getPrototypeOf(value); + if (proto !== Object.prototype && proto !== null) return value; + const out: Record = {}; + for (const [k, v] of Object.entries(value as Record)) { + out[k.includes(NUL_CHAR) ? k.replace(NUL_RE, '') : k] = stripNullBytes(v); + } + return out as T; + } + return value; } @@ -301,9 +322,17 @@ export class RemoteChannel { device_name: this.deviceName, app_version: VERSION, platform: process.platform - }).then(() => { - console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); - captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + }).then((trackStatus: string) => { + // track() RESOLVES with 'ok' | 'error' | 'timed out' β€” + // it does not reject, so a non-'ok' status must be + // checked or a failed presence publish looks like success. + if (trackStatus === 'ok') { + console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); + captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + } else { + console.error(`❌ Presence track not acknowledged (${trackStatus}) β€” device may show offline`); + captureRemote('remote_channel_presence_track_error', { result: trackStatus }).catch(() => { }); + } }).catch((trackErr: any) => { console.error('[DEBUG] Presence track failed:', trackErr?.message); captureRemote('remote_channel_presence_track_error', { error: trackErr?.message }).catch(() => { }); @@ -386,7 +415,11 @@ export class RemoteChannel { } if (!row) { // Row already claimed+deleted, or cleanup raced delivery β€” nothing to do. - await captureRemote('remote_channel_doorbell_row_missing', {}); + // Not retried on purpose: pre-flip the row was inserted before the + // doorbell was sent, so a missing row means it was already claimed + // and deleted. Post-009 this is the ONLY delivery path β€” see the + // 009 preconditions if this event ever becomes non-zero. + await captureRemote('remote_channel_doorbell_row_missing', { call_id: callId }); return; } if (row.status !== 'pending') { @@ -629,7 +662,10 @@ export class RemoteChannel { // a 5-minute timeout for a tool that actually ran. Common with binary // file reads / process output. error_message is text, so it's exempt. if (result !== null) updateData.result = stripNullBytes(result); - if (errorMessage !== null) updateData.error_message = errorMessage; + // Postgres `text` rejects NUL too (not just jsonb) β€” a NUL-bearing error + // message would fail this terminal write, and because result === null the + // fallback below wouldn't fire, stranding the call until the 5-min timeout. + if (errorMessage !== null) updateData.error_message = stripNullBytes(errorMessage); console.debug('[DEBUG] Updating call result:', updateData); const { error } = await this.client @@ -835,8 +871,17 @@ export class RemoteChannel { // Leave presence explicitly on the graceful path (socket close // covers the abrupt one). try { - await this.channel.untrack(); - console.debug('[DEBUG] Presence untracked (graceful leave)'); + // Only untrack a LIVE channel: on a dead/half-open socket the + // presence push is buffered and only settles via realtime-js's + // 10s timeout, which would blow past device.ts's 5s force-exit + // and skip the durable offline write. A closed socket drops + // server-side presence anyway. + if (this.channel.state === 'joined') { + await this.channel.untrack(); + console.debug('[DEBUG] Presence untracked (graceful leave)'); + } else { + console.debug('[DEBUG] Skipping untrack β€” channel not joined; socket close clears presence'); + } } catch { /* best effort */ } await this.channel.unsubscribe(); this.channel = null; diff --git a/test/test-remote-channel-reconnect.js b/test/test-remote-channel-reconnect.js index fa939360..882f7aa2 100644 --- a/test/test-remote-channel-reconnect.js +++ b/test/test-remote-channel-reconnect.js @@ -60,6 +60,18 @@ class FakeChannel { }); return this; } + // Presence (added with the Broadcast/Presence transport): the device track()s + // itself on SUBSCRIBED and untrack()s on a graceful unsubscribe. realtime-js + // RESOLVES these with a status string ('ok' | 'error' | 'timed out') rather + // than rejecting, so the fakes mirror that contract. + track() { + this.tracked = true; + return Promise.resolve('ok'); + } + untrack() { + this.tracked = false; + return Promise.resolve('ok'); + } unsubscribe() { this.state = 'leaving'; return Promise.resolve({ error: null }); @@ -154,6 +166,12 @@ function makeRemoteChannel() { rc._user = { id: 'user-1', email: 'tester@example.com' }; rc.onToolCall = () => {}; rc.deviceId = 'device-1'; + rc.deviceName = 'test-device'; + // recreateChannel() now sleeps a jittered 1-3s (capped ~45s) before rebuilding, + // so a fleet-wide event doesn't stampede reconnects. These tests are about + // WHETHER the wedge recovers, not how long it waits β€” stub the sleep so the + // suite stays sub-second. (Backoff duration is asserted separately below.) + rc.sleep = () => Promise.resolve(); return { rc, client }; } diff --git a/test/test-strip-null-bytes.js b/test/test-strip-null-bytes.js new file mode 100644 index 00000000..9d1e8eba --- /dev/null +++ b/test/test-strip-null-bytes.js @@ -0,0 +1,77 @@ +#!/usr/bin/env node + +/** + * Regression tests for stripNullBytes (remote-device/remote-channel.ts). + * + * Postgres rejects a NUL (U+0000) in BOTH jsonb and text with 22P05, which + * stranded remote calls at 'executing' until the 5-minute timeout. The first + * implementation serialized to JSON and regex-replaced the ESCAPE TEXT, which + * had two failure modes found in review (2026-07-24) β€” both covered here: + * + * 1. SILENT CORRUPTION: content containing the six literal characters + * backslash-u-0-0-0-0 (e.g. reading a source file with that escape in it) + * had those characters deleted from the tool result, undetected. + * 2. THROW: a doubled-backslash form produced invalid JSON, so the write threw + * and the call was reported failed even though the tool had succeeded. + * + * Run: npm run build && node test/test-strip-null-bytes.js + */ + +import { stripNullBytes } from '../dist/remote-device/remote-channel.js'; + +const NUL = String.fromCharCode(0); +const BACKSLASH = String.fromCharCode(92); +let failures = 0; + +function check(name, actual, expected) { + const ok = JSON.stringify(actual) === JSON.stringify(expected); + if (ok) { + console.log(`βœ… PASS ${name}`); + } else { + failures++; + console.error(`❌ FAIL ${name}\n expected: ${JSON.stringify(expected)}\n actual: ${JSON.stringify(actual)}`); + } +} + +// --- the bug it exists to fix: real NUL characters are removed --------------- +check('strips a real NUL from a string', stripNullBytes(`a${NUL}b`), 'ab'); +check( + 'strips a real NUL nested in a tool result', + stripNullBytes({ content: [{ type: 'text', text: `START${NUL}END` }] }), + { content: [{ type: 'text', text: 'STARTEND' }] } +); +check('strips real NULs from object keys', stripNullBytes({ [`k${NUL}`]: 1 }), { k: 1 }); + +// --- regression 1: literal escape TEXT must be preserved, not corrupted ------ +const literalEscape = `const re = /${BACKSLASH}u0000/g;`; +check('preserves literal backslash-u0000 text (no silent corruption)', + stripNullBytes({ text: literalEscape }), { text: literalEscape }); + +// --- regression 2: doubled backslash must not throw -------------------------- +const doubled = `x${BACKSLASH}${BACKSLASH}u0000y`; +let threw = null; +try { + check('handles doubled backslash without throwing', stripNullBytes({ text: doubled }), { text: doubled }); +} catch (e) { + threw = e; + failures++; + console.error(`❌ FAIL handles doubled backslash without throwing β€” threw: ${e.message}`); +} + +// --- structure / passthrough ------------------------------------------------ +check('leaves clean content untouched', stripNullBytes({ a: 'clean', b: [1, 2] }), { a: 'clean', b: [1, 2] }); +check('passes through null', stripNullBytes(null), null); +check('passes through numbers/booleans', stripNullBytes({ n: 42, b: true }), { n: 42, b: true }); +check('handles arrays of strings with NUL', stripNullBytes([`p${NUL}q`, 'clean']), ['pq', 'clean']); + +// --- the property that actually matters for Postgres ------------------------ +const out = JSON.stringify(stripNullBytes({ x: `${NUL}${NUL}`, y: literalEscape })); +if (out.includes(NUL)) { + failures++; + console.error('❌ FAIL output still contains a raw NUL character'); +} else { + console.log('βœ… PASS output contains no raw NUL characters'); +} + +console.log(`\n${failures === 0 ? 'βœ…' : '❌'} strip-null-bytes: ${failures} failing test(s).`); +process.exitCode = failures === 0 ? 0 : 1; From 3f5268428b263bdc48c24a7c3c41c0e01d03af0a Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Fri, 24 Jul 2026 22:00:09 +0300 Subject: [PATCH 04/13] Trim telemetry and dedup memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop remote_channel_doorbell_received: it fired on EVERY remote tool call (~126k/day in prod) for permanent per-call telemetry volume, and it is redundant β€” dispatch stamps metadata.transport, which rides the existing mcp_command_executed event, so transport usage is already segmentable server-side. The doorbell FAILURE paths are still captured. - Seen-call-id cap 500 -> 100 (~10 KB). Dual delivery arrives within milliseconds, so the window only needs to outlive that; 100 is still several minutes of heavy agent traffic. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/device.ts | 9 +++++---- src/remote-device/remote-channel.ts | 6 +++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/remote-device/device.ts b/src/remote-device/device.ts index bc9256f3..fdefc6bc 100644 --- a/src/remote-device/device.ts +++ b/src/remote-device/device.ts @@ -15,11 +15,12 @@ export interface MCPDeviceOptions { /** * How many recently-handled call ids to remember for duplicate-delivery - * suppression. Both transports deliver a call within milliseconds of each - * other, so this only needs to outlive that window; 500 is ~minutes of even - * heavy agent traffic and costs a few KB. + * suppression. The two transports deliver a call within MILLISECONDS of each + * other, so this only has to outlive that window β€” 100 ids is several minutes + * of even the heaviest agent traffic, and costs ~10 KB on the user's machine + * (the device process, not the shared server). */ -const SEEN_CALL_IDS_MAX = 500; +const SEEN_CALL_IDS_MAX = 100; export class MCPDevice { private baseServerUrl: string; diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 31f0ab65..e815d2e5 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -380,8 +380,12 @@ export class RemoteChannel { return; } + // NOTE: deliberately NOT a telemetry event β€” this fires on every remote + // tool call (~126k/day in prod) and would be permanent per-call volume. + // Transport usage is already segmentable server-side: dispatch stamps + // metadata.transport, which rides mcp_command_executed. Only the + // doorbell FAILURE paths below are worth capturing. console.debug('[DEBUG] Doorbell received for call:', callId); - captureRemote('remote_channel_doorbell_received', { tool_name: payload?.tool_name }).catch(() => { }); if (!this.client) return; From d3faf5aefc813b548e050f005b88e9c8ffc07501 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Fri, 24 Jul 2026 22:33:31 +0300 Subject: [PATCH 05/13] Round-3 review fixes (device): auth token, bounded untrack, ack, heartbeat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - setSession pushed the token it was HANDED to realtime, but auth.setSession() refreshes an expired one internally first (device asleep >1h with --persist-session). Pushing the stale parameter overwrote the fresh token realtime already had, so every private-channel join failed until the next refresh β€” ~50 min deaf. Push the CURRENT session token instead. - untrack() on shutdown: a half-open socket still reports state 'joined', so the state guard didn't help β€” the presence push buffers and settles via realtime-js's 10s timeout, past device.ts's 5s force-exit, skipping unsubscribe() and the durable offline write. Bounded with a 1s race. - broadcast { ack: true } + presence { enabled: true } on the device channel. Without ack, send() resolves 'ok' at socket-write, so notifyResult's status check and its failure telemetry could never fire. - updateHeartbeat: skip the write entirely when not joined. Bumping last_seen on a deaf device kept its row perpetually young, so the staleness sweep could never age out a stale 'online' β€” exactly the row dispatch falls back to when presence is unavailable. - Dropped remote_channel_private_join_failed: it fired on every CHANNEL_ERROR, a 1:1 duplicate of remote_channel_subscription_error with no added specificity. Filter the surviving event on the error text instead. - RECREATE_TIMEOUT_MS comment corrected: the guard is held for backoff (<=45s) PLUS the 30s cap, so the watchdog blind spot is ~75s, not 30s. - Test: replaced the comment claiming a backoff assertion with an actual one (grows with consecutive attempts, always positive, capped at 30s). npm test: 46/46. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/remote-channel.ts | 85 ++++++++++++++++----------- test/bench-strip-null-bytes.mjs | 42 +++++++++++++ test/test-remote-channel-reconnect.js | 43 ++++++++++++-- 3 files changed, 130 insertions(+), 40 deletions(-) create mode 100644 test/bench-strip-null-bytes.mjs diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index e815d2e5..2a8a21c8 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -57,8 +57,11 @@ interface DeviceData { // write only feeds the "last seen X ago" label for offline devices. The server // sweeps broadcast-capable devices offline after 65 min (2 missed writes + slack). const HEARTBEAT_INTERVAL = 30 * 60 * 1000; -// Cap a single channel recreate so a hung await can't pin the re-entrancy guard -// true (which would silently disable the connection watchdog). +// Cap the channel-rebuild portion of a recreate so a hung await can't pin the +// re-entrancy guard true (which would silently disable the connection watchdog). +// NOTE: the jittered backoff sleep runs BEFORE this cap and inside the guard, so +// the total window in which checkConnectionHealth is a no-op is +// backoff (<=45s) + RECREATE_TIMEOUT_MS β€” bounded at ~75s, not 30s. const RECREATE_TIMEOUT_MS = 30000; // Max time the channel may sit CONTINUOUSLY in 'joining' before we force a recreate. // 'joining' is normally healthy (we let realtime-js's rejoin backoff converge), but on a @@ -135,11 +138,16 @@ export class RemoteChannel { console.debug('[DEBUG] Session set successfully, user:', user.email); // Private channels authorize with the user JWT at join time. supabase-js - // v2 generally forwards auth to realtime on its own β€” these are defensive - // (cheap, and a silent gap here would only surface as a channel dying at - // JWT expiry ~1h in): push the token now and re-push on every refresh. - this.client.realtime.setAuth(session.access_token); - console.debug('[DEBUG] Realtime socket authorized with user JWT'); + // generally forwards auth to realtime itself; this is defensive and must + // push the CURRENT session token, not the one we were handed: + // auth.setSession() refreshes an expired token internally (device asleep + // >1h with --persist-session), and pushing the stale parameter here would + // overwrite the fresh token realtime already had β€” every private-channel + // join then fails until the next refresh (~50 min deaf). + const { data: { session: currentSession } } = await this.client.auth.getSession(); + const realtimeToken = currentSession?.access_token ?? session.access_token; + this.client.realtime.setAuth(realtimeToken); + console.debug('[DEBUG] Realtime socket authorized with current session JWT'); if (!this.authListenerRegistered) { this.authListenerRegistered = true; this.client.auth.onAuthStateChange((event, newSession) => { @@ -275,7 +283,15 @@ export class RemoteChannel { const channelName = `user:${this.user.id}`; console.debug(`[DEBUG] Creating channel: ${channelName}`); this.channel = this.client.channel(channelName, { - config: { private: true, presence: { key: this.deviceId ?? undefined } } + // ack: true β€” without it send() resolves 'ok' as soon as the frame + // is written to the socket, so notifyResult's status check (and its + // failure telemetry) could never fire. presence.enabled makes the + // presence extension explicit rather than inferred. + config: { + private: true, + broadcast: { ack: true }, + presence: { key: this.deviceId ?? undefined, enabled: true } + } }) .on( 'postgres_changes' as any, @@ -342,11 +358,11 @@ export class RemoteChannel { // CHANNEL_ERROR is the only status carrying a real error message. console.error(`❌ Channel error: ${err?.message || 'unknown'} β€” ${this.connState()}`); this.setOnlineStatus(this.deviceId!, 'offline'); + // Single event: this fires on ordinary network faults too, so a + // separate "private join failed" alarm would be a 1:1 duplicate + // with no added specificity. Filter on the error text + // ("Unauthorized"/policy) to isolate an 008 misconfiguration. captureRemote('remote_channel_subscription_error', { error: err?.message || 'Channel error' }).catch(() => { }); - // Distinct fleet-level alarm: if the 008 channel policies were - // ever wrong in prod, this event spiking is the immediate signal - // (fix = SQL policy patch, no client rollback needed). - captureRemote('remote_channel_private_join_failed', { attempt: this.reconnectAttempt, error: err?.message }).catch(() => { }); reject(err || new Error('Failed to initialize tool call channel subscription')); } else if (status === 'TIMED_OUT') { console.error(`⏱️ Channel subscription timed out, Reconnecting... β€” ${this.connState()}`); @@ -703,21 +719,20 @@ export class RemoteChannel { async updateHeartbeat(deviceId: string) { if (!this.client) return; try { - // Re-assert status:'online' ONLY when the channel is actually joined: - // at a 30-min cadence this beats a lost race with the server's offline - // sweep for a HEALTHY device. But if the channel is dead (CHANNEL_ERROR - // already set the row offline), blindly flipping it back to 'online' - // would mask a deaf device β€” in kill-switch/presence-fallback mode that - // turns a fast-fail into a 5-minute timeout. Always refresh last_seen. - const isJoined = this.channel?.state === 'joined'; - const updates: { last_seen: string; status?: string } = { - last_seen: new Date().toISOString(), - }; - if (isJoined) updates.status = 'online'; + // Skip the write entirely when the channel is not joined. Bumping + // last_seen on a deaf device would keep its row perpetually young, so + // the server's staleness sweep could never age it out and correct a + // stale 'online' β€” and whenever presence is unavailable (kill switch, + // wedged socket) that stale row is exactly what dispatch falls back to. + // Staying silent lets the sweep do its job. + if (this.channel?.state !== 'joined') { + console.debug('[DEBUG] Skipping heartbeat write β€” channel not joined; letting the row age out'); + return; + } const { error } = await this.client .from('mcp_devices') - .update(updates) + .update({ last_seen: new Date().toISOString(), status: 'online' }) .eq('id', deviceId); if (error) { @@ -875,17 +890,17 @@ export class RemoteChannel { // Leave presence explicitly on the graceful path (socket close // covers the abrupt one). try { - // Only untrack a LIVE channel: on a dead/half-open socket the - // presence push is buffered and only settles via realtime-js's - // 10s timeout, which would blow past device.ts's 5s force-exit - // and skip the durable offline write. A closed socket drops - // server-side presence anyway. - if (this.channel.state === 'joined') { - await this.channel.untrack(); - console.debug('[DEBUG] Presence untracked (graceful leave)'); - } else { - console.debug('[DEBUG] Skipping untrack β€” channel not joined; socket close clears presence'); - } + // Bound the untrack: a HALF-OPEN socket still reports the channel + // as 'joined', so a state check alone is not enough β€” the presence + // push just buffers and settles via realtime-js's 10s timeout, + // which blows past device.ts's 5s force-exit and would skip both + // unsubscribe() and the durable offline write. Race it against a + // 1s cap; a dropped socket clears server-side presence anyway. + await Promise.race([ + this.channel.untrack(), + this.sleep(1000), + ]); + console.debug('[DEBUG] Presence untrack attempted (bounded at 1s)'); } catch { /* best effort */ } await this.channel.unsubscribe(); this.channel = null; diff --git a/test/bench-strip-null-bytes.mjs b/test/bench-strip-null-bytes.mjs new file mode 100644 index 00000000..5668dd9f --- /dev/null +++ b/test/bench-strip-null-bytes.mjs @@ -0,0 +1,42 @@ +// Benchmark stripNullBytes against realistic tool-result shapes. +// Not part of the test suite (name doesn't match test*.js) β€” run manually: +// npm run build && node test/bench-strip-null-bytes.mjs +import { stripNullBytes } from '../dist/remote-device/remote-channel.js'; + +const NUL = String.fromCharCode(0); + +function bench(name, value, iterations) { + // warm up JIT + for (let i = 0; i < 5; i++) stripNullBytes(value); + const t0 = process.hrtime.bigint(); + for (let i = 0; i < iterations; i++) stripNullBytes(value); + const t1 = process.hrtime.bigint(); + const perCallMs = Number(t1 - t0) / 1e6 / iterations; + console.log(`${name.padEnd(46)} ${perCallMs.toFixed(4)} ms/call`); +} + +const small = { content: [{ type: 'text', text: 'Process started with PID 1234' }] }; +const typical = { content: [{ type: 'text', text: 'x'.repeat(10 * 1024) }] }; // 10 KB +const big = { content: [{ type: 'text', text: 'x'.repeat(1024 * 1024) }] }; // 1 MB +const huge = { content: [{ type: 'text', text: 'x'.repeat(13 * 1024 * 1024) }] }; // 13 MB (max seen in prod) +const withNul = { content: [{ type: 'text', text: ('x'.repeat(1024) + NUL).repeat(1024) }] }; // 1 MB w/ NULs +const manyKeys = Object.fromEntries( + Array.from({ length: 500 }, (_, i) => [`key_${i}`, `value_${i}`]) +); + +console.log('--- no NUL present (the overwhelmingly common case) ---'); +bench('small result (~30 B)', small, 20000); +bench('typical result (10 KB)', typical, 5000); +bench('big result (1 MB)', big, 200); +bench('huge result (13 MB, prod max)', huge, 20); +bench('object with 500 keys', manyKeys, 5000); + +console.log('\n--- NUL present (rare) ---'); +bench('1 MB with 1024 NULs', withNul, 200); + +// Reference point: what the same payload costs to JSON-serialize, which the +// supabase client does on every write regardless. +const t0 = process.hrtime.bigint(); +for (let i = 0; i < 20; i++) JSON.stringify(huge); +const t1 = process.hrtime.bigint(); +console.log(`\nreference: JSON.stringify(13 MB) ${(Number(t1 - t0) / 1e6 / 20).toFixed(4)} ms/call`); diff --git a/test/test-remote-channel-reconnect.js b/test/test-remote-channel-reconnect.js index 882f7aa2..65757de6 100644 --- a/test/test-remote-channel-reconnect.js +++ b/test/test-remote-channel-reconnect.js @@ -167,11 +167,16 @@ function makeRemoteChannel() { rc.onToolCall = () => {}; rc.deviceId = 'device-1'; rc.deviceName = 'test-device'; - // recreateChannel() now sleeps a jittered 1-3s (capped ~45s) before rebuilding, - // so a fleet-wide event doesn't stampede reconnects. These tests are about - // WHETHER the wedge recovers, not how long it waits β€” stub the sleep so the - // suite stays sub-second. (Backoff duration is asserted separately below.) - rc.sleep = () => Promise.resolve(); + // recreateChannel() sleeps a jittered backoff before rebuilding so a fleet-wide + // event doesn't stampede reconnects. These tests are about WHETHER the wedge + // recovers, not how long it waits β€” stub the sleep so the suite stays + // sub-second, but record the requested delays so the formula can be asserted + // (see "reconnect backoff grows and stays bounded" below). + rc.sleptMs = []; + rc.sleep = (ms) => { + rc.sleptMs.push(ms); + return Promise.resolve(); + }; return { rc, client }; } @@ -343,6 +348,34 @@ async function main() { ); }); + // The jittered backoff exists so a fleet-wide event (server deploy, Supabase + // blip) doesn't stampede every device into reconnecting at the same instant. + // Assert the shape rather than exact values: it must GROW with consecutive + // attempts and stay BOUNDED so a device can't disappear for minutes. + await test('reconnect backoff grows and stays bounded', async () => { + const { rc, client } = makeRemoteChannel(); + await withQuietLogs(async () => { + await goHalfOpen(rc, client); + // Force several consecutive failed recreates to climb the backoff. + for (let i = 0; i < 6; i++) { + await rc.recreateChannel(); + rc.channel.state = 'errored'; // stay unhealthy so the next attempt escalates + } + }); + + assert.ok(rc.sleptMs.length >= 5, `expected several backoff sleeps, got ${rc.sleptMs.length}`); + assert.ok( + rc.sleptMs.every((ms) => ms > 0 && ms <= 30_000), + `every backoff must be positive and capped at 30s: ${JSON.stringify(rc.sleptMs)}` + ); + // Jitter is +/-50%, so compare first-attempt vs late-attempt ranges rather + // than adjacent pairs (adjacent values can legitimately dip). + assert.ok( + Math.max(...rc.sleptMs.slice(3)) > Math.min(...rc.sleptMs.slice(0, 2)), + `backoff should grow with consecutive attempts: ${JSON.stringify(rc.sleptMs)}` + ); + }); + console.log( `\n${failures ? 'πŸ”΄' : 'βœ…'} remote-channel reconnect: ${failures} failing test(s).` ); From a0580a5e3a5ddf73244a686d7067625c090532bd Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sat, 25 Jul 2026 11:52:50 +0300 Subject: [PATCH 06/13] Round-4 review fixes (device): independent legacy channel, presence retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - LEGACY FALLBACK IS INDEPENDENT AGAIN. The postgres_changes listener had been moved onto the private user:{id} channel, so the transition's safety net shared a single point of failure with the thing it backs up: an 008 policy problem (or any private-channel auth failure) took out BOTH transports and the device went completely dark instead of degrading to the old path. It now lives on its own public channel with its own lifecycle, rebuilt alongside the private one on recreate and torn down on unsubscribe. Costs one extra channel per device (its own connection carries 2, far under the 100 quota). Removed entirely at the flip. - track() failures are retried (3 attempts) and createChannel no longer resolves before presence lands. Previously a single non-'ok' track left a fully working device invisible β€” the server treats absent presence as authoritative offline, so dispatch threw "No devices available" with nothing to re-track until the channel bounced. checkConnectionHealth also re-tracks a joined channel whose presence never published, so it self-heals. - Backoff test was flaky (~10%, measured) AND vacuous: the fake heals on disconnect, so every recreate succeeded, reconnectAttempt reset to 0, and all samples came from the attempt-1 distribution β€” "grows" was a coin flip and the cap was never exercised (and was asserted at 30s when the formula tops out at 45s). Now models a persistent outage so attempts actually climb; 12/12 clean. - PUBLISH.md: release gate for this transport (008 applied + new server deployed, in that order) β€” the migrations README lives in the other repo, which is not where the person cutting an npm release is looking. npm test: 46/46. Co-Authored-By: Claude Opus 4.8 --- PUBLISH.md | 27 +++++ src/remote-device/remote-channel.ts | 164 ++++++++++++++++++++------ test/test-remote-channel-reconnect.js | 40 +++++-- 3 files changed, 182 insertions(+), 49 deletions(-) diff --git a/PUBLISH.md b/PUBLISH.md index df96297d..5a5444fc 100644 --- a/PUBLISH.md +++ b/PUBLISH.md @@ -2,6 +2,33 @@ This document outlines the complete process for publishing new versions of Desktop Commander to both NPM and the MCP Registry. +## β›” RELEASE GATE β€” remote device (Broadcast/Presence transport) + +**Applies to any release containing the Broadcast/Presence remote-device +transport (`capabilities.transport_broadcast_v1`, private `user:{id}` channel).** +Publishing it before the backend is ready **bricks remote use for every device +that updates**. Verify BOTH against **prod** (`olvbkozcufcbptfogatw`) first: + +1. **Migration 008 is applied** β€” the private-channel policies exist: + ```sql + SELECT policyname FROM pg_policies + WHERE schemaname='realtime' AND tablename='messages'; + -- expect: "users receive on own channel", "users send on own channel" + ``` + Without it the device cannot join `user:{id}`, so it loses the doorbell + transport. (The legacy `postgres_changes` listener rides its own public + channel, so the device degrades rather than going dark β€” but it is still a + broken release.) + +2. **The new server is deployed** (the `v*` tag that knows about + `transport_broadcast_v1`). The old server's flat 45-second offline sweep + marks these devices offline 45s after registration β€” they heartbeat every + 30 minutes β€” and dispatch then fails against them. + +Order is strict: **008 β†’ server deploy β†’ this npm release.** Full rationale, +failure modes and rollback rules: `remote-dc-mcp/migrations/README.md` and +`BROADCAST_PRESENCE_RUNBOOK.md`. + ## πŸš€ Automated Release (Recommended) We now have an automated release script that handles the entire process with **automatic state tracking and resume capability**! diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 2a8a21c8..237e8a1c 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -75,6 +75,17 @@ const JOINING_WEDGE_TIMEOUT_MS = 30000; export class RemoteChannel { private client: SupabaseClient | null = null; private channel: RealtimeChannel | null = null; + /** + * TRANSITION ONLY (removed at the flip): the legacy postgres_changes + * listener lives on its OWN public channel, deliberately NOT on the private + * user channel. It is the safety net for the broadcast transport, so it + * must not share a failure mode with it β€” if it rode the private channel, + * an 008 policy problem or any private-channel auth failure would take out + * BOTH transports at once and the device would go completely dark instead + * of degrading to the old path. Costs one extra channel per device (the + * device's own connection carries 2, far under the 100/connection quota). + */ + private legacyChannel: RealtimeChannel | null = null; private heartbeatInterval: NodeJS.Timeout | null = null; private connectionCheckInterval: NodeJS.Timeout | null = null; @@ -85,6 +96,11 @@ export class RemoteChannel { private onToolCall: ((payload: any) => void) | null = null; // Guard so setSession being called twice can't stack auth listeners. private authListenerRegistered = false; + // False when presence publishing failed on a channel that is otherwise + // healthy β€” the health check re-tries, since nothing else would (a joined + // channel never re-fires SUBSCRIBED) and the server would keep reporting + // this device offline. + private presenceTracked = false; // Track last device status to prevent duplicate log messages private lastDeviceStatus: 'online' | 'offline' = 'offline'; @@ -254,6 +270,10 @@ export class RemoteChannel { console.debug('[DEBUG] Calling createChannel()'); // ! Ignore silently in Initialization to reconnect after + // Legacy postgres_changes listener on its own public channel β€” the + // independent safety net for the doorbell transport (see legacyChannel). + this.createLegacyChannel(); + await this.createChannel().catch((error) => { console.debug(`[DEBUG] Failed to create channel, will retry after socket reconnect: ${error?.message || error} β€” ${this.connState()}`); }); @@ -265,6 +285,89 @@ export class RemoteChannel { } } + /** + * Publish this device's presence, retrying a non-'ok' result. track() + * RESOLVES with 'ok' | 'error' | 'timed out' rather than rejecting, and a + * silent failure is expensive: the server treats absent presence as + * authoritative offline, so one lost track makes a fully working device + * undispatchable until the channel next bounces. `presenceTracked` lets the + * health check re-try later if every attempt here fails. + */ + private async trackPresenceWithRetry(recovered: number, attempts = 3): Promise { + for (let attempt = 1; attempt <= attempts; attempt++) { + if (!this.channel || this.channel.state !== 'joined') return; + let status: string; + try { + status = await this.channel.track({ + device_id: this.deviceId, + device_name: this.deviceName, + app_version: VERSION, + platform: process.platform + }); + } catch (trackErr: any) { + status = `threw: ${trackErr?.message}`; + } + + if (status === 'ok') { + this.presenceTracked = true; + console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); + captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + return; + } + + console.error(`❌ Presence track not acknowledged (${status}) β€” attempt ${attempt}/${attempts}`); + if (attempt < attempts) await this.sleep(500 * attempt); + } + + this.presenceTracked = false; + console.error('❌ Presence track failed after retries β€” device may show offline until the next health tick'); + captureRemote('remote_channel_presence_track_error', { attempts }).catch(() => { }); + } + + /** + * Subscribe the legacy postgres_changes listener on its own PUBLIC channel. + * Independent of the private user channel on purpose (see legacyChannel). + * Best-effort: failures here are logged, never thrown β€” the doorbell path is + * primary, and realtime-js rejoins this channel on its own. + * Removed entirely at the flip (009), when postgres_changes stops firing. + */ + private createLegacyChannel(): void { + if (!this.client || !this.user?.id) return; + try { + this.legacyChannel = this.client + .channel('device_tool_call_queue') + .on( + 'postgres_changes' as any, + { + event: 'INSERT', + schema: 'public', + table: 'mcp_remote_calls', + filter: `user_id=eq.${this.user.id}` + }, + (payload: any) => { + console.debug('[DEBUG] Realtime event received, payload:', payload?.new?.id); + if (this.onToolCall) { + this.onToolCall(payload); + } + } + ) + .subscribe((status: string) => { + console.debug(`[DEBUG] Legacy channel status: ${status}`); + }); + } catch (error: any) { + console.debug('[DEBUG] Legacy channel subscribe failed (doorbell path unaffected):', error?.message); + } + } + + /** Tear down the legacy channel (best effort). */ + private async removeLegacyChannel(): Promise { + if (!this.legacyChannel || !this.client) return; + try { + await this.client.removeChannel(this.legacyChannel); + } catch { /* best effort */ } + this.legacyChannel = null; + } + /** * Create and subscribe to the channel. * This is used for both initial subscription and recreation after socket reconnects. @@ -293,21 +396,6 @@ export class RemoteChannel { presence: { key: this.deviceId ?? undefined, enabled: true } } }) - .on( - 'postgres_changes' as any, - { - event: 'INSERT', - schema: 'public', - table: 'mcp_remote_calls', - filter: `user_id=eq.${this.user.id}` - }, - (payload: any) => { - console.debug('[DEBUG] Realtime event received, payload:', payload?.new?.id); - if (this.onToolCall) { - this.onToolCall(payload); - } - } - ) .on( 'broadcast', { event: 'new_call' }, @@ -331,32 +419,20 @@ export class RemoteChannel { console.error('Failed to set online status:', e.message); }); } - // Announce presence β€” this IS the live "online" signal for - // the server's dispatch check and the dashboard's green dot. - this.channel?.track({ - device_id: this.deviceId, - device_name: this.deviceName, - app_version: VERSION, - platform: process.platform - }).then((trackStatus: string) => { - // track() RESOLVES with 'ok' | 'error' | 'timed out' β€” - // it does not reject, so a non-'ok' status must be - // checked or a failed presence publish looks like success. - if (trackStatus === 'ok') { - console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); - captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); - } else { - console.error(`❌ Presence track not acknowledged (${trackStatus}) β€” device may show offline`); - captureRemote('remote_channel_presence_track_error', { result: trackStatus }).catch(() => { }); - } - }).catch((trackErr: any) => { - console.error('[DEBUG] Presence track failed:', trackErr?.message); - captureRemote('remote_channel_presence_track_error', { error: trackErr?.message }).catch(() => { }); - }); - resolve(); + // Announce presence β€” this IS the live "online" signal the + // server's dispatch check reads. A failed track leaves a + // perfectly healthy device invisible (server treats absent + // presence as authoritative offline β†’ "No devices + // available"), so retry, and only resolve once it lands: + // resolving first would let registerDevice() print + // "Device ready" while the device is still undispatchable. + this.trackPresenceWithRetry(recovered) + .catch(() => { /* logged inside */ }) + .finally(() => resolve()); } else if (status === 'CHANNEL_ERROR') { // CHANNEL_ERROR is the only status carrying a real error message. console.error(`❌ Channel error: ${err?.message || 'unknown'} β€” ${this.connState()}`); + this.presenceTracked = false; this.setOnlineStatus(this.deviceId!, 'offline'); // Single event: this fires on ordinary network faults too, so a // separate "private join failed" alarm would be a 1:1 duplicate @@ -512,6 +588,13 @@ export class RemoteChannel { // 'joined' = healthy. Clear the joining-overstay timer. if (state === 'joined') { this.joiningSince = null; + // Self-heal a failed presence publish: the channel is up, so nothing + // else will ever retry (SUBSCRIBED won't fire again), and without + // presence the server reports this healthy device as offline. + if (!this.presenceTracked && this.deviceId) { + console.debug('[DEBUG] Channel joined but presence not tracked β€” retrying track()'); + this.trackPresenceWithRetry(0, 1).catch(() => { /* logged inside */ }); + } return; } @@ -617,6 +700,9 @@ export class RemoteChannel { await this.client!.removeChannel(this.channel); this.channel = null; } + // Rebuild the legacy channel too: it shares the socket, so a + // socket-level wedge takes it down with the private channel. + await this.removeLegacyChannel(); // FIX (core): force a brand-new WebSocket. After idle / wifi-loss the socket can // be HALF-OPEN (readyState OPEN but dead); reusing it made every join TIME_OUT @@ -625,6 +711,7 @@ export class RemoteChannel { console.debug('[DEBUG] Calling createChannel() for recreation'); await this.createChannel(); + this.createLegacyChannel(); }, RECREATE_TIMEOUT_MS, 'recreateChannel'); } catch (err: any) { captureRemote('remote_channel_recreate_error', { errMsg: err?.message, attempt: this.reconnectAttempt }); @@ -886,6 +973,7 @@ export class RemoteChannel { } async unsubscribe() { + await this.removeLegacyChannel(); if (this.channel) { // Leave presence explicitly on the graceful path (socket close // covers the abrupt one). diff --git a/test/test-remote-channel-reconnect.js b/test/test-remote-channel-reconnect.js index 65757de6..bb1faeb9 100644 --- a/test/test-remote-channel-reconnect.js +++ b/test/test-remote-channel-reconnect.js @@ -352,27 +352,45 @@ async function main() { // blip) doesn't stampede every device into reconnecting at the same instant. // Assert the shape rather than exact values: it must GROW with consecutive // attempts and stay BOUNDED so a device can't disappear for minutes. - await test('reconnect backoff grows and stays bounded', async () => { + await test('reconnect backoff grows with attempts and stays bounded', async () => { const { rc, client } = makeRemoteChannel(); await withQuietLogs(async () => { await goHalfOpen(rc, client); - // Force several consecutive failed recreates to climb the backoff. - for (let i = 0; i < 6; i++) { + // Model a PERSISTENT outage: rebuilding the socket doesn't help, so every + // recreate fails and reconnectAttempt actually climbs. The default fake + // heals on rebuildSocket(), which meant every recreate SUCCEEDED, the + // counter reset to 0, and all samples came from the attempt-1 + // distribution β€” making a "grows" assertion a coin flip (~10% flake, + // measured over 30 runs) and never exercising the cap at all. + client.realtime.rebuildSocket = function () { + this.rebuilds++; + this.conn = { readyState: 1 }; + this.socketDead = true; // still dead after the rebuild + }; + client.realtime.socketDead = true; + for (let i = 0; i < 7; i++) { await rc.recreateChannel(); - rc.channel.state = 'errored'; // stay unhealthy so the next attempt escalates + if (rc.channel) rc.channel.state = 'errored'; } }); - assert.ok(rc.sleptMs.length >= 5, `expected several backoff sleeps, got ${rc.sleptMs.length}`); + assert.ok(rc.sleptMs.length >= 6, `expected several backoff sleeps, got ${rc.sleptMs.length}`); + // Formula: min(30_000, 1000 * 2**min(attempt,5)) * (0.5 + random()) + // -> hard ceiling is 30_000 * 1.5 = 45_000 ms. assert.ok( - rc.sleptMs.every((ms) => ms > 0 && ms <= 30_000), - `every backoff must be positive and capped at 30s: ${JSON.stringify(rc.sleptMs)}` + rc.sleptMs.every((ms) => ms > 0 && ms <= 45_000), + `every backoff must be positive and <= 45s: ${JSON.stringify(rc.sleptMs)}` ); - // Jitter is +/-50%, so compare first-attempt vs late-attempt ranges rather - // than adjacent pairs (adjacent values can legitimately dip). + // With the counter climbing, late attempts draw from a strictly higher + // range than the first: attempt 1 tops out at 3_000, attempt 5+ starts at + // 15_000 β€” so this cannot flake on jitter. assert.ok( - Math.max(...rc.sleptMs.slice(3)) > Math.min(...rc.sleptMs.slice(0, 2)), - `backoff should grow with consecutive attempts: ${JSON.stringify(rc.sleptMs)}` + rc.sleptMs[0] <= 3_000, + `first backoff should be the attempt-1 range: ${rc.sleptMs[0]}` + ); + assert.ok( + Math.max(...rc.sleptMs.slice(-2)) >= 15_000, + `late backoffs should reach the capped range: ${JSON.stringify(rc.sleptMs)}` ); }); From 10625c3bbc367a7d643a23f52c1ff1d50158de8e Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sat, 25 Jul 2026 12:16:23 +0300 Subject: [PATCH 07/13] Round-5 review fixes (device): gate the capability on a proven transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 β€” the device advertised transport_broadcast_v1 during registerDevice(), before createChannel() was even attempted. The server treats that flag as binding: for a flagged device absent presence is authoritative offline, so dispatch throws "No devices available". If the private channel could not join (008 missing, authz, a Realtime incident) but the legacy postgres_changes channel was perfectly healthy, the server refused to dispatch at all β€” the device went dark, which is precisely what the independent legacy channel was added to prevent. The flag is now written only after SUBSCRIBED *and* a successful presence track, and withdrawn when presence definitively fails, so a device that cannot keep the promise reports itself legacy and stays reachable. This also makes the PUBLISH.md release gate far less load-bearing. P1 β€” recreateChannel() destroyed the legacy channel and only rebuilt it AFTER createChannel() resolved, so any failure (or timeout) left the safety net dead for the whole outage, with every health tick repeating the teardown. It is now rebuilt before createChannel. Related: trackPresenceWithRetry's worst case (~31.5s: three 10s realtime pushes plus backoff) exceeded RECREATE_TIMEOUT_MS (30s), so a recreate where presence never acked ALWAYS timed out β€” the two constants were in silent conflict. Raised to 45s. Also: bound removeLegacyChannel() in unsubscribe() (it was an unbounded await in front of the deliberately 1s-bounded untrack, defeating it and risking the 5s force-exit); include deviceId in createChannel's prerequisite guard (it is the presence KEY β€” a null key gets a random one and the device is invisible while every local signal says healthy); add an in-flight guard to the presence self-heal so 10s ticks can't stack pushes on a wedged socket; stop serializing the whole result object in the hot-path debug log (13 MB payloads cost more there than everything the sanitizer was benchmarked against). New test file covering the branch's headline mechanism, which had none: exactly-once under dual delivery (including the fail-open claim case), the device-id filter preceding dedupe, seen-set bounding, onDoorbell routing (other-device, already-claimed, missing row, fetch retry), and the updateCallResult-before-notifyResult ordering the server depends on. npm test: 47/47. Co-Authored-By: Claude Opus 4.8 --- src/remote-device/remote-channel.ts | 106 ++++++++++++-- test/test-remote-dedupe-and-doorbell.js | 181 ++++++++++++++++++++++++ 2 files changed, 274 insertions(+), 13 deletions(-) create mode 100644 test/test-remote-dedupe-and-doorbell.js diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 237e8a1c..22b546a8 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -62,7 +62,11 @@ const HEARTBEAT_INTERVAL = 30 * 60 * 1000; // NOTE: the jittered backoff sleep runs BEFORE this cap and inside the guard, so // the total window in which checkConnectionHealth is a no-op is // backoff (<=45s) + RECREATE_TIMEOUT_MS β€” bounded at ~75s, not 30s. -const RECREATE_TIMEOUT_MS = 30000; +// Must exceed createChannel()'s worst case, which now includes +// trackPresenceWithRetry: 3 track() pushes at realtime-js's 10s DEFAULT_TIMEOUT +// plus 0.5s+1s backoff = ~31.5s. At the old 30s these two constants were in +// silent conflict β€” any recreate where presence never acked ALWAYS timed out. +const RECREATE_TIMEOUT_MS = 45000; // Max time the channel may sit CONTINUOUSLY in 'joining' before we force a recreate. // 'joining' is normally healthy (we let realtime-js's rejoin backoff converge), but on a // HALF-OPEN socket (readyState OPEN yet dead) realtime-js parks the channel in 'joining' @@ -101,6 +105,13 @@ export class RemoteChannel { // channel never re-fires SUBSCRIBED) and the server would keep reporting // this device offline. private presenceTracked = false; + // Last capability value written to the DB (null = not yet written), so the + // flag isn't re-written on every reconnect. + private transportCapableWritten: boolean | null = null; + // Re-entrancy guard for the presence self-heal: on a wedged socket each + // track() buffers for the full 10s push timeout, so unguarded 10s health + // ticks would stack pending pushes. + private isTrackingPresence = false; // Track last device status to prevent duplicate log messages private lastDeviceStatus: 'online' | 'offline' = 'offline'; @@ -247,15 +258,22 @@ export class RemoteChannel { if (existingDevice) { console.debug('[DEBUG] Updating device status to online'); + // NOTE: transport_broadcast_v1 is deliberately NOT set here. The flag + // is a promise the device may not be able to keep, and the server + // treats it as binding: for a flagged device, absent presence is + // authoritative offline (overlayPresence) and dispatch then throws + // "No devices available". Advertising it before the private channel + // is proven means an 008/authz/Realtime problem takes the device dark + // even though its legacy postgres_changes channel is perfectly + // healthy β€” the exact outcome the independent legacyChannel exists to + // prevent. It is written only after SUBSCRIBED + a successful presence + // track (see markTransportCapable), and cleared when presence + // definitively fails, so a device that cannot deliver on the promise + // simply reports itself legacy and stays dispatchable. await this.updateDevice(existingDevice.id, { status: 'online', last_seen: new Date().toISOString(), - // transport_broadcast_v1 = this device joins the private user - // channel (Broadcast doorbells + Presence). The server keys its - // transport choice and offline-sweep tier on this flag. - // app_version rides along so adoption ("are old versions gone - // yet?") is answerable from SQL and PostHog alike. - capabilities: { transport_broadcast_v1: true, app_version: VERSION }, + capabilities: { app_version: VERSION }, device_name: deviceName }); @@ -294,6 +312,16 @@ export class RemoteChannel { * health check re-try later if every attempt here fails. */ private async trackPresenceWithRetry(recovered: number, attempts = 3): Promise { + if (this.isTrackingPresence) return; // never stack pushes on a wedged socket + this.isTrackingPresence = true; + try { + await this.trackPresenceInner(recovered, attempts); + } finally { + this.isTrackingPresence = false; + } + } + + private async trackPresenceInner(recovered: number, attempts: number): Promise { for (let attempt = 1; attempt <= attempts; attempt++) { if (!this.channel || this.channel.state !== 'joined') return; let status: string; @@ -312,6 +340,10 @@ export class RemoteChannel { this.presenceTracked = true; console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + // Transport is proven end-to-end (channel joined AND presence + // published) β€” only now is it safe to let the server route us + // over broadcast and treat our presence as authoritative. + await this.setTransportCapable(true); return; } @@ -320,8 +352,39 @@ export class RemoteChannel { } this.presenceTracked = false; - console.error('❌ Presence track failed after retries β€” device may show offline until the next health tick'); + console.error('❌ Presence track failed after retries β€” reverting to the legacy transport tier'); captureRemote('remote_channel_presence_track_error', { attempts }).catch(() => { }); + // Withdraw the promise: without presence the server cannot see us, and a + // stale capability flag would make it refuse to dispatch entirely. Going + // back to the legacy tier keeps the device usable over postgres_changes. + await this.setTransportCapable(false); + } + + /** + * Advertise (or withdraw) the broadcast transport capability. The server + * reads this flag to choose a transport AND to decide whether absent + * presence means "offline" β€” so it must only ever be true while this device + * can actually be reached that way. + */ + private async setTransportCapable(capable: boolean): Promise { + if (!this.client || !this.deviceId) return; + if (this.transportCapableWritten === capable) return; // no redundant writes + try { + const capabilities: Record = { app_version: VERSION }; + if (capable) capabilities.transport_broadcast_v1 = true; + const { error } = await this.client + .from('mcp_devices') + .update({ capabilities }) + .eq('id', this.deviceId); + if (error) { + console.error('[DEBUG] Failed to update transport capability:', error.message); + return; + } + this.transportCapableWritten = capable; + console.debug(`[DEBUG] Transport capability set to ${capable ? 'broadcast_v1' : 'legacy'}`); + } catch (error: any) { + console.error('[DEBUG] Transport capability update threw:', error?.message); + } } /** @@ -374,7 +437,11 @@ export class RemoteChannel { */ private createChannel(): Promise { return new Promise((resolve, reject) => { - if (!this.client || !this.user?.id || !this.onToolCall) { + if (!this.client || !this.user?.id || !this.onToolCall || !this.deviceId) { + // deviceId included deliberately: it is the presence KEY, and a + // null key makes realtime assign a random one β€” the server's + // lookup by device id then misses and the device is invisible + // while every local signal says healthy. console.debug('[DEBUG] createChannel() failed - missing prerequisites'); return reject(new Error('Client not initialized or missing subscription parameters')); } @@ -591,7 +658,7 @@ export class RemoteChannel { // Self-heal a failed presence publish: the channel is up, so nothing // else will ever retry (SUBSCRIBED won't fire again), and without // presence the server reports this healthy device as offline. - if (!this.presenceTracked && this.deviceId) { + if (!this.presenceTracked && this.deviceId && !this.isTrackingPresence) { console.debug('[DEBUG] Channel joined but presence not tracked β€” retrying track()'); this.trackPresenceWithRetry(0, 1).catch(() => { /* logged inside */ }); } @@ -710,8 +777,13 @@ export class RemoteChannel { try { await (this.client as any).realtime?.disconnect?.(); } catch { /* best effort */ } console.debug('[DEBUG] Calling createChannel() for recreation'); - await this.createChannel(); + // Rebuild the legacy safety net FIRST and unconditionally: if + // createChannel() throws or exceeds RECREATE_TIMEOUT_MS, anything + // after it is skipped, which used to leave the fallback dead for + // the entire duration of a private-channel outage β€” every + // subsequent health tick repeating the same teardown. this.createLegacyChannel(); + await this.createChannel(); }, RECREATE_TIMEOUT_MS, 'recreateChannel'); } catch (err: any) { captureRemote('remote_channel_recreate_error', { errMsg: err?.message, attempt: this.reconnectAttempt }); @@ -774,7 +846,12 @@ export class RemoteChannel { // fallback below wouldn't fire, stranding the call until the 5-min timeout. if (errorMessage !== null) updateData.error_message = stripNullBytes(errorMessage); - console.debug('[DEBUG] Updating call result:', updateData); + // Log a summary, not the payload: results reach 13 MB and util.inspect + // on the hot path costs more than everything else in this function. + console.debug( + `[DEBUG] Updating call result: ${callId} status=${status}` + + (result !== null ? ` resultBytes=~${JSON.stringify(updateData.result)?.length ?? 0}` : '') + ); const { error } = await this.client .from('mcp_remote_calls') .update(updateData) @@ -973,7 +1050,10 @@ export class RemoteChannel { } async unsubscribe() { - await this.removeLegacyChannel(); + // Bounded like the untrack below: removeChannel() sends a leave push that + // only settles via realtime-js's 10s timeout on a half-open socket, which + // would blow device.ts's 5s force-exit and skip the durable offline write. + await Promise.race([this.removeLegacyChannel(), this.sleep(1000)]); if (this.channel) { // Leave presence explicitly on the graceful path (socket close // covers the abrupt one). diff --git a/test/test-remote-dedupe-and-doorbell.js b/test/test-remote-dedupe-and-doorbell.js new file mode 100644 index 00000000..da91c70a --- /dev/null +++ b/test/test-remote-dedupe-and-doorbell.js @@ -0,0 +1,181 @@ +#!/usr/bin/env node + +/** + * Covers the branch's headline mechanism, which previously had no tests: + * + * 1. EXACTLY-ONCE under dual delivery. During the transition every call is + * delivered twice (legacy postgres_changes + broadcast doorbell). The DB + * claim deliberately fails OPEN on a transient write error, so the local + * seen-call-id guard in handleNewToolCall is what actually guarantees a + * side-effecting tool runs once. + * 2. onDoorbell routing: ignore other devices, skip already-claimed rows, + * surface a missing row, and retry a transient fetch failure. + * 3. updateCallResult BEFORE notifyResult β€” the server fetches the row by id + * when the doorbell arrives, so the write must land first or it sees a + * non-terminal row and waits for the 10s recovery poll. + * + * Run: npm run build && node test/test-remote-dedupe-and-doorbell.js + */ + +import { MCPDevice } from '../dist/remote-device/device.js'; +import { RemoteChannel } from '../dist/remote-device/remote-channel.js'; + +let failures = 0; +async function test(name, fn) { + try { + await fn(); + console.log(`βœ… PASS ${name}`); + } catch (error) { + failures++; + console.error(`πŸ”΄ FAIL ${name}\n ${error.message}`); + } +} +function assert(condition, message) { + if (!condition) throw new Error(message); +} + +const DEVICE_ID = 'device-1'; +const OTHER_DEVICE = 'device-2'; + +/** MCPDevice with the network/desktop edges stubbed out. */ +function makeDevice({ claimResults = [] } = {}) { + const device = new MCPDevice(); + const executed = []; + const claims = [...claimResults]; + + device.deviceId = DEVICE_ID; + device.desktop = { + callClientTool: async (toolName, args) => { + executed.push({ toolName, args }); + return { content: [{ type: 'text', text: 'ok' }] }; + }, + }; + device.remoteChannel = { + // Default: first delivery claims, later ones lose. Overridable to model a + // transient DB error, which makes the claim fail OPEN (returns true). + markCallExecuting: async () => (claims.length ? claims.shift() : true), + updateCallResult: async () => {}, + notifyResult: async () => {}, + }; + return { device, executed }; +} + +const payloadFor = (id, deviceId = DEVICE_ID) => ({ + new: { id, tool_name: 'start_process', tool_args: { command: 'echo hi' }, device_id: deviceId, metadata: {} }, +}); + +await test('dual delivery of the same call executes the tool exactly once', async () => { + const { device, executed } = makeDevice(); + await device.handleNewToolCall(payloadFor('call-a')); + await device.handleNewToolCall(payloadFor('call-a')); // the other transport + assert(executed.length === 1, `expected 1 execution, got ${executed.length}`); +}); + +await test('exactly-once holds even when the DB claim fails OPEN for both deliveries', async () => { + // Both claims return true (what a transient REST error produces) β€” only the + // in-memory guard prevents a second run of a side-effecting command. + const { device, executed } = makeDevice({ claimResults: [true, true] }); + await device.handleNewToolCall(payloadFor('call-b')); + await device.handleNewToolCall(payloadFor('call-b')); + assert(executed.length === 1, `fail-open claim double-executed: ${executed.length} runs`); +}); + +await test('a lost DB claim (another process won) skips execution', async () => { + const { device, executed } = makeDevice({ claimResults: [false] }); + await device.handleNewToolCall(payloadFor('call-c')); + assert(executed.length === 0, `expected no execution, got ${executed.length}`); +}); + +await test('calls for a different device are ignored and do not poison the dedupe set', async () => { + const { device, executed } = makeDevice(); + await device.handleNewToolCall(payloadFor('call-d', OTHER_DEVICE)); + assert(executed.length === 0, 'must not execute another device call'); + // Same id later arriving FOR US must still run β€” the filter precedes dedupe. + await device.handleNewToolCall(payloadFor('call-d')); + assert(executed.length === 1, 'a mismatched delivery must not suppress our own'); +}); + +await test('the seen-call-id set stays bounded', async () => { + const { device } = makeDevice(); + for (let i = 0; i < 250; i++) await device.handleNewToolCall(payloadFor(`bulk-${i}`)); + assert(device.seenCallIds.size <= 100, `set grew to ${device.seenCallIds.size}`); +}); + +// --- onDoorbell ------------------------------------------------------------- + +/** RemoteChannel with just enough of a Supabase client for onDoorbell. */ +function makeChannel({ rows = {}, failFetches = 0 } = {}) { + const rc = new RemoteChannel(); + const delivered = []; + let fetchAttempts = 0; + rc.deviceId = DEVICE_ID; + rc.onToolCall = (payload) => delivered.push(payload); + rc.sleep = () => Promise.resolve(); // no real backoff in tests + rc.client = { + from: () => ({ + select: () => ({ + eq: () => ({ + maybeSingle: async () => { + fetchAttempts++; + if (fetchAttempts <= failFetches) return { data: null, error: { message: 'fetch failed' } }; + return { data: rows.row ?? null, error: null }; + }, + }), + }), + }), + }; + return { rc, delivered, attempts: () => fetchAttempts }; +} + +await test('doorbell for another device is ignored without fetching', async () => { + const { rc, delivered, attempts } = makeChannel(); + await rc.onDoorbell({ call_id: 'x', device_id: OTHER_DEVICE }); + assert(delivered.length === 0, 'must not deliver'); + assert(attempts() === 0, 'must not even fetch the row'); +}); + +await test('doorbell delivers a pending row through the shared handler', async () => { + const row = { id: 'x', status: 'pending', tool_name: 'start_process' }; + const { rc, delivered } = makeChannel({ rows: { row } }); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(delivered.length === 1, 'expected one delivery'); + assert(delivered[0].new === row, 'must pass the fetched row as {new: row}'); +}); + +await test('doorbell for an already-claimed row does not re-deliver', async () => { + const { rc, delivered } = makeChannel({ rows: { row: { id: 'x', status: 'executing' } } }); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(delivered.length === 0, 'non-pending rows must not be re-delivered'); +}); + +await test('doorbell row fetch retries a transient failure', async () => { + const row = { id: 'x', status: 'pending' }; + const { rc, delivered, attempts } = makeChannel({ rows: { row }, failFetches: 2 }); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(attempts() === 3, `expected 3 attempts, got ${attempts()}`); + assert(delivered.length === 1, 'should deliver after the retry succeeds'); +}); + +await test('doorbell with a missing row is a no-op (already claimed and deleted)', async () => { + const { rc, delivered } = makeChannel({ rows: { row: null } }); + await rc.onDoorbell({ call_id: 'gone', device_id: DEVICE_ID }); + assert(delivered.length === 0, 'missing row must not deliver'); +}); + +// --- result ordering -------------------------------------------------------- + +await test('the result row is written BEFORE the doorbell is rung', async () => { + const order = []; + const { device, executed } = makeDevice(); + device.remoteChannel.updateCallResult = async () => { order.push('write'); }; + device.remoteChannel.notifyResult = async () => { order.push('doorbell'); }; + await device.handleNewToolCall(payloadFor('call-order')); + assert(executed.length === 1, 'tool should have run'); + assert( + order.join(',') === 'write,doorbell', + `server fetches by id on the doorbell, so the write must land first: got ${order.join(',')}` + ); +}); + +console.log(`\n${failures ? 'πŸ”΄' : 'βœ…'} dedupe + doorbell: ${failures} failing test(s).`); +process.exit(failures ? 1 : 0); From b5fa0a99157526bb2feff36e5a7f51be082ebb9d Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sun, 26 Jul 2026 17:16:13 +0300 Subject: [PATCH 08/13] Round-6/7/8 review fixes (device): keep an unproven device dispatchable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blackout bugs, all from the same invariant: the server tiers BOTH its offline sweep and its presence overlay on the device-written capabilities.transport_broadcast_v1 flag, never on the app version. 1. Heartbeat cadence followed the BUILD, not the tier. It was 30 min flat, while a device without the flag is judged by the server's 45s legacy threshold β€” and the flag is only written after presence is proven. So any device that could not prove the private channel (008 not applied, an authz hiccup, exhausted track() retries) was swept offline ~45s after registering and stayed undispatchable, though its independent legacy channel was joined and could run every call. Split into CAPABLE_HEARTBEAT_INTERVAL (5 min) and LEGACY_HEARTBEAT_INTERVAL (15s), chosen per-tick by heartbeatIntervalMs(); the fixed setInterval became a self-rescheduling scheduleHeartbeat() that re-arms on every tier change and writes immediately when dropping to the fast tier. 2. updateHeartbeat gated its write on the PRIVATE channel, so a device reachable only via the legacy channel never wrote last_seen at all β€” fixing the cadence alone would not have helped. Now gated on isReachable() (private OR legacy joined), which also keeps the deliberate silence for a genuinely deaf device so the sweep can still age its row out. 3. `status` is transport-agnostic β€” it is what the server's resolveTargetDevice filters on β€” but every degraded state of the private channel wrote 'offline'. Since realtime-js re-fires CHANNEL_ERROR on every failed rejoin, the row oscillated against the heartbeat and roughly half of all dispatches failed for healthy machines. Now driven by syncReachabilityStatus() off the same isReachable() predicate, with writes serialized through a single-slot chain so a teardown's write cannot overtake a join's. Also: the capability was only ever withdrawn from trackPresenceInner, which is unreachable unless the channel is already joined β€” so a device that could never join kept advertising itself and the server's presence overlay reported it authoritatively OFFLINE until the process restarted. It now withdraws after TRANSPORT_WITHDRAW_AFTER_ATTEMPTS (3) failed recreates, bounded by its own timeout because recreateChannel's catch is outside RECREATE_TIMEOUT_MS's reach. Not fewer than 3: ordinary half-open recovery legitimately costs two attempts. Shutdown path: a heartbeat or reachability write could land after setOffline()'s subprocess write and leave an exited process marked online for a full sweep tier. Gated both writers on shuttingDown. auth.getSession() was unbounded network I/O (it refreshes within ~90s of expiry, with its own ~30s retry budget) and could outlast device.ts's 5s force-exit, losing the durable offline write entirely β€” now bounded with a cached-token fallback. Bounded channel.unsubscribe() and tightened the leave bounds so the whole path fits. New test/test-remote-heartbeat-tier.js (17 cases) covers the tier pairing, the reachability gate, write ordering, the withdrawal (and that a single blip does NOT withdraw), the guard release on a hanging write, and the bounded session fetch. Each was verified to fail with its fix reverted. Pairs with the server-side change lowering DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS to 15 min; the two cadences must stay in step and nothing enforces the copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR --- src/remote-device/remote-channel.ts | 395 +++++++++++++-- .../scripts/blocking-offline-update.js | 2 +- test/test-remote-heartbeat-tier.js | 469 ++++++++++++++++++ 3 files changed, 817 insertions(+), 49 deletions(-) create mode 100644 test/test-remote-heartbeat-tier.js diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index 22b546a8..f31f32e1 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -52,11 +52,29 @@ interface DeviceData { last_seen: string; } -// Bookkeeping cadence for the durable last_seen column. Liveness is carried by -// Presence on the user channel (websocket-level, flips in seconds) β€” this slow -// write only feeds the "last seen X ago" label for offline devices. The server -// sweeps broadcast-capable devices offline after 65 min (2 missed writes + slack). -const HEARTBEAT_INTERVAL = 30 * 60 * 1000; +// Bookkeeping cadence for the durable last_seen column ONCE the broadcast +// transport is proven. Liveness is then carried by Presence (websocket-level, +// flips in seconds), so this write is not the live signal β€” but it IS the +// server's fallback whenever presence is unavailable, which is why it cannot be +// slow. Paired with DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS = 15 min (3 missed +// writes); the full rationale for that pairing, and the list of states that +// reach it, lives on that constant in remote-dc-mcp/src/server/constants.ts. +const CAPABLE_HEARTBEAT_INTERVAL = 5 * 60 * 1000; +// Cadence while this device is in the LEGACY tier β€” i.e. whenever +// transport_broadcast_v1 is not currently advertised, i.e. whenever presence +// has not (yet) been proven. MUST stay well inside the server's legacy sweep +// threshold (DEVICE_OFFLINE_TIMEOUT_MS = 45s), because the server tiers the +// sweep on the CAPABILITY FLAG, not on the app version: a device without the +// flag is judged by the 45s rule no matter how new its build is. +// +// Getting this wrong is not a slow degradation, it is a blackout: on the slow +// capable cadence a device that cannot prove the private channel (008 not applied, +// an authz hiccup, exhausted track() retries) is swept offline ~45s after +// registering and dispatch then throws "No devices available" forever β€” even +// though its INDEPENDENT legacy postgres_changes channel is joined and would +// deliver calls perfectly. That blackout is exactly what the independent +// legacyChannel exists to prevent, so the two must be kept in step. +const LEGACY_HEARTBEAT_INTERVAL = 15 * 1000; // Cap the channel-rebuild portion of a recreate so a hung await can't pin the // re-entrancy guard true (which would silently disable the connection watchdog). // NOTE: the jittered backoff sleep runs BEFORE this cap and inside the guard, so @@ -75,6 +93,36 @@ const RECREATE_TIMEOUT_MS = 45000; // resolves/errors well within this window; 3 health ticks of unbroken 'joining' means the // state machine has stalled and only a fresh socket (via recreate) recovers it. const JOINING_WEDGE_TIMEOUT_MS = 30000; +// Consecutive failed channel recreates after which this device WITHDRAWS +// transport_broadcast_v1. +// +// This is load-bearing, not tidiness. For a flagged device the server treats +// absent Presence as AUTHORITATIVE offline and applies that overlay BEFORE +// selection β€” it outranks the `status` column entirely. So a device that keeps +// the flag while unable to join the private channel is undispatchable no matter +// how healthy its legacy channel is, and no matter how fresh its last_seen. +// Withdrawing the promise is what drops it back to a tier the server will still +// route to (see setTransportCapable / the rollback runbook's stage rules). +// +// The withdrawal deliberately does NOT hang off CHANNEL_ERROR: realtime-js +// re-fires that on every failed rejoin, so a momentary blip would flap the +// capability and its DB write. Gate on sustained failure instead β€” 3 recreates +// is ~30s given the jittered backoff. A later successful track() re-advertises +// automatically. +// +// DO NOT LOWER THIS TO 2. Ordinary half-open recovery legitimately costs two +// attempts: removeChannel() disconnects the socket when the last channel leaves, +// and realtime-js then refuses to reconnect for ~100ms while _connectionState is +// 'disconnecting', so the FIRST recreate's join pushes are buffered and burn the +// full 10s join timeout β€” the second dials the fresh socket and succeeds. At 2, +// every routine wifi drop would withdraw the capability and churn the DB. +const TRANSPORT_WITHDRAW_AFTER_ATTEMPTS = 3; +// Bound on the capability-withdrawal write. It runs in recreateChannel()'s catch +// block, outside RECREATE_TIMEOUT_MS's reach, so it needs its own cap. +const CAPABILITY_WRITE_TIMEOUT_MS = 5000; +// Bound on the shutdown path's session fetch. See setOffline() β€” auth.getSession() +// can block on a token refresh, and this runs against device.ts's 5s force-exit. +const OFFLINE_SESSION_TIMEOUT_MS = 500; export class RemoteChannel { private client: SupabaseClient | null = null; @@ -92,6 +140,17 @@ export class RemoteChannel { private legacyChannel: RealtimeChannel | null = null; private heartbeatInterval: NodeJS.Timeout | null = null; private connectionCheckInterval: NodeJS.Timeout | null = null; + // Device whose last_seen the heartbeat timer maintains; null = stopped. + // Held so scheduleHeartbeat() can re-arm itself without re-plumbing the id. + private heartbeatDeviceId: string | null = null; + // Single-slot queue keeping concurrent `status` PATCHes in order. + private statusWriteChain: Promise = Promise.resolve(); + // Tokens from the last successful setSession / TOKEN_REFRESHED, so the + // shutdown path never has to wait on auth.getSession(). See setOffline(). + private lastKnownSession: { access_token: string; refresh_token: string | null } | null = null; + // Set once unsubscribe() starts: suppresses further reachability-driven + // status writes so they cannot land after setOffline()'s durable write. + private shuttingDown = false; // Store subscription parameters for channel recreation @@ -174,6 +233,13 @@ export class RemoteChannel { const { data: { session: currentSession } } = await this.client.auth.getSession(); const realtimeToken = currentSession?.access_token ?? session.access_token; this.client.realtime.setAuth(realtimeToken); + // Cache tokens for the shutdown path: setOffline() must not depend on a + // getSession() that can block on a token refresh while device.ts's 5s + // force-exit is running down (see setOffline). + this.lastKnownSession = { + access_token: realtimeToken, + refresh_token: currentSession?.refresh_token ?? session.refresh_token ?? null, + }; console.debug('[DEBUG] Realtime socket authorized with current session JWT'); if (!this.authListenerRegistered) { this.authListenerRegistered = true; @@ -181,6 +247,10 @@ export class RemoteChannel { if (event === 'TOKEN_REFRESHED' && newSession?.access_token && this.client) { console.debug('[DEBUG] Token refreshed β€” re-authorizing realtime socket'); this.client.realtime.setAuth(newSession.access_token); + this.lastKnownSession = { + access_token: newSession.access_token, + refresh_token: newSession.refresh_token ?? this.lastKnownSession?.refresh_token ?? null, + }; } }); } @@ -273,7 +343,7 @@ export class RemoteChannel { await this.updateDevice(existingDevice.id, { status: 'online', last_seen: new Date().toISOString(), - capabilities: { app_version: VERSION }, + capabilities: this.capabilitiesPayload(false), device_name: deviceName }); @@ -339,7 +409,10 @@ export class RemoteChannel { if (status === 'ok') { this.presenceTracked = true; console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); - captureRemote('remote_channel_presence_tracked', { attempt: recovered }).catch(() => { }); + // recoveredAfterAttempts, not "attempt": this is how many + // reconnect attempts preceded the join that carried this track + // (0 on a first join AND on the health-check self-heal path). + captureRemote('remote_channel_presence_tracked', { recoveredAfterAttempts: recovered }).catch(() => { }); // Transport is proven end-to-end (channel joined AND presence // published) β€” only now is it safe to let the server route us // over broadcast and treat our presence as authoritative. @@ -360,18 +433,36 @@ export class RemoteChannel { await this.setTransportCapable(false); } + /** + * The complete `capabilities` JSONB value for this device. Built in ONE + * place because every write REPLACES the whole column β€” a second literal + * elsewhere would silently delete whatever key it forgot on the next + * reconnect. + */ + private capabilitiesPayload(broadcastCapable: boolean): Record { + return { + app_version: VERSION, + ...(broadcastCapable ? { transport_broadcast_v1: true } : {}) + }; + } + /** * Advertise (or withdraw) the broadcast transport capability. The server * reads this flag to choose a transport AND to decide whether absent * presence means "offline" β€” so it must only ever be true while this device * can actually be reached that way. + * + * The flag also selects which tier of the server's offline sweep judges + * this device (45s legacy vs 15min capable), so every change here MUST + * re-arm the heartbeat at the matching cadence β€” otherwise withdrawing the + * flag leaves the device in the 45s tier while it still heartbeats on the + * slow capable cadence and the sweep blacks it out. */ private async setTransportCapable(capable: boolean): Promise { if (!this.client || !this.deviceId) return; if (this.transportCapableWritten === capable) return; // no redundant writes try { - const capabilities: Record = { app_version: VERSION }; - if (capable) capabilities.transport_broadcast_v1 = true; + const capabilities = this.capabilitiesPayload(capable); const { error } = await this.client .from('mcp_devices') .update({ capabilities }) @@ -382,6 +473,19 @@ export class RemoteChannel { } this.transportCapableWritten = capable; console.debug(`[DEBUG] Transport capability set to ${capable ? 'broadcast_v1' : 'legacy'}`); + // Tier changed β€” move last_seen onto the cadence that tier's sweep + // threshold expects (no-op if the heartbeat hasn't started yet). + this.scheduleHeartbeat(); + // Dropping to the fast tier: the row was last written on the slow + // capable cadence, so it can already be minutes old β€” i.e. ALREADY + // past the 45s legacy threshold the device is now judged by. Write + // once immediately rather than waiting out the new interval, which + // would leave the device swept-offline in the meantime (and, on a + // device flapping tiers faster than the interval, indefinitely, + // since every re-arm restarts the countdown). + if (!capable && this.heartbeatDeviceId) { + this.updateHeartbeat(this.heartbeatDeviceId).catch(() => { /* logged inside */ }); + } } catch (error: any) { console.error('[DEBUG] Transport capability update threw:', error?.message); } @@ -460,7 +564,10 @@ export class RemoteChannel { config: { private: true, broadcast: { ack: true }, - presence: { key: this.deviceId ?? undefined, enabled: true } + // key is non-null: the guard above rejects when !deviceId, + // precisely because a null key makes realtime assign a + // random one and the server's lookup by device id misses. + presence: { key: this.deviceId, enabled: true } } }) .on( @@ -480,12 +587,9 @@ export class RemoteChannel { const recovered = this.reconnectAttempt; this.reconnectAttempt = 0; console.log(`βœ… Channel subscribed${recovered > 0 ? ` (recovered after ${recovered} attempt${recovered === 1 ? '' : 's'})` : ''}`); - // Update device status on successful connection - if (this.deviceId) { - this.setOnlineStatus(this.deviceId, 'online').catch(e => { - console.error('Failed to set online status:', e.message); - }); - } + // Update device status on successful connection (queued, so + // it can't be overtaken by a teardown's status write). + this.queueStatusWrite('online'); // Announce presence β€” this IS the live "online" signal the // server's dispatch check reads. A failed track leaves a // perfectly healthy device invisible (server treats absent @@ -500,7 +604,7 @@ export class RemoteChannel { // CHANNEL_ERROR is the only status carrying a real error message. console.error(`❌ Channel error: ${err?.message || 'unknown'} β€” ${this.connState()}`); this.presenceTracked = false; - this.setOnlineStatus(this.deviceId!, 'offline'); + this.syncReachabilityStatus(); // Single event: this fires on ordinary network faults too, so a // separate "private join failed" alarm would be a 1:1 duplicate // with no added specificity. Filter on the error text @@ -509,15 +613,14 @@ export class RemoteChannel { reject(err || new Error('Failed to initialize tool call channel subscription')); } else if (status === 'TIMED_OUT') { console.error(`⏱️ Channel subscription timed out, Reconnecting... β€” ${this.connState()}`); - this.setOnlineStatus(this.deviceId!, 'offline'); + this.syncReachabilityStatus(); captureRemote('remote_channel_subscription_timeout', { attempt: this.reconnectAttempt }).catch(() => { }); reject(new Error('Tool call channel subscription timed out')); } else if (status === 'CLOSED') { // Settle the promise so an in-flight recreateChannel() can't await - // forever (which would wedge the re-entrancy guard / watchdog), and - // mark the device offline like the other degraded states. + // forever (which would wedge the re-entrancy guard / watchdog). console.warn(`⚠️ Channel closed β€” ${this.connState()}`); - this.setOnlineStatus(this.deviceId!, 'offline'); + this.syncReachabilityStatus(); reject(new Error('Tool call channel closed during subscribe')); } }); @@ -585,6 +688,11 @@ export class RemoteChannel { await captureRemote('remote_channel_doorbell_row_missing', { call_id: callId }); return; } + // OPTIMIZATION, not a correctness guard β€” do not rely on it. Saves a + // hop when the legacy path already claimed this call. The actual + // exactly-once guarantees live in device.ts: the in-process seenCallIds + // check (same-process double delivery) and the conditional DB claim + // (cross-process/restart). if (row.status !== 'pending') { console.debug('[DEBUG] Doorbell call already claimed via legacy path:', callId); return; @@ -788,6 +896,33 @@ export class RemoteChannel { } catch (err: any) { captureRemote('remote_channel_recreate_error', { errMsg: err?.message, attempt: this.reconnectAttempt }); console.debug(`[DEBUG] Channel recreation failed: ${err?.message} β€” ${this.connState()}`); + // Sustained private-channel failure: stop promising a transport we + // cannot deliver. Until this withdrawal the flag could only ever be + // cleared from trackPresenceInner, which is unreachable unless the + // channel is already 'joined' β€” so a device that could never join + // (008 dropped, RLS/JWT failure after a long sleep) kept advertising + // itself, and the server's presence overlay then reported it OFFLINE + // authoritatively, overriding a perfectly good `status` and blacking + // it out until the process restarted. + if (this.reconnectAttempt >= TRANSPORT_WITHDRAW_AFTER_ATTEMPTS) { + // BOUNDED, and in its own try: this runs in the catch block, + // which RECREATE_TIMEOUT_MS does NOT cover (it wraps only the + // inner withTimeout above). An unbounded await here would pin + // isRecreatingChannel=true on a hanging PATCH and silently + // disable the 10s connection watchdog β€” precisely the failure + // mode withTimeout() was introduced for. + try { + await this.withTimeout( + () => this.setTransportCapable(false), + CAPABILITY_WRITE_TIMEOUT_MS, + 'withdrawTransportCapability' + ); + } catch (withdrawErr: any) { + // Next failed recreate retries; transportCapableWritten is + // only advanced on a confirmed write, so nothing is lost. + console.debug(`[DEBUG] Capability withdrawal did not complete: ${withdrawErr?.message}`); + } + } } finally { this.isRecreatingChannel = false; } @@ -880,17 +1015,102 @@ export class RemoteChannel { } } + /** + * True while this device can still be reached by SOME transport: the + * private user channel (broadcast doorbells) or, during the transition, its + * independent legacy postgres_changes channel. + * + * The heartbeat gate deliberately asks "reachable?", not "is the private + * channel up?". Gating on the private channel alone means a device whose + * ONLY working transport is the legacy channel never writes last_seen, so + * the server's 45s legacy sweep marks it offline and dispatch refuses it β€” + * a total blackout for a device that can actually run tools. + * + * Removed with the rest of the legacy path at the flip (009), after which + * the private channel is the only transport and this collapses back to a + * single check. + */ + private isReachable(): boolean { + return this.channel?.state === 'joined' || this.legacyChannel?.state === 'joined'; + } + + /** + * Reconcile the durable `status` column with ACTUAL reachability. + * + * `status` is transport-agnostic β€” it is the column the server's + * resolveTargetDevice() filters on β€” so it must never be driven by the + * health of ONE transport. Writing 'offline' from the private channel's + * error path (which is what this replaces) blacked out devices whose + * legacy channel was joined and delivering: realtime-js re-fires + * CHANNEL_ERROR on every failed rejoin, so during a private-channel + * outage each retry re-wrote 'offline' while the heartbeat re-wrote + * 'online', leaving the row oscillating and roughly half of all dispatches + * failing with "No devices available" for a perfectly healthy machine. + * + * Same predicate as the heartbeat gate, so the two can never disagree. + */ + private syncReachabilityStatus(): void { + this.queueStatusWrite(this.isReachable() ? 'online' : 'offline'); + } + + /** + * Serialize the CHANNEL-CALLBACK status writes β€” not every writer. These + * fire from un-awaited callbacks (SUBSCRIBED, CHANNEL_ERROR, CLOSED), and + * two concurrent PATCHes to the same row land in arbitrary order: the real + * window is inside recreateChannel(), where removeChannel()'s CLOSED writes + * 'offline' and the fresh join's SUBSCRIBED writes 'online' ~100-300ms later + * β€” comparable to a PostgREST round trip. Unordered, the 'offline' can win + * and leave a healthy device undispatchable until the next heartbeat tick. + * + * Deliberately NOT the single writer: updateHeartbeat, registerDevice and + * setOffline's subprocess all write status directly. That is fine β€” the + * heartbeat re-asserting 'online' every tier interval is the intended + * self-correction β€” but do not assume this chain gives total ordering. + */ + private queueStatusWrite(status: 'online' | 'offline'): void { + // After teardown begins, setOffline() owns the final status write. + if (this.shuttingDown) { + console.debug(`[DEBUG] Status write '${status}' suppressed β€” teardown in progress`); + return; + } + this.statusWriteChain = this.statusWriteChain + .then(() => (this.deviceId ? this.setOnlineStatus(this.deviceId, status) : undefined)) + .catch((e: any) => { + console.error('[DEBUG] Status write failed:', e?.message); + }); + } + + /** + * Heartbeat cadence for the tier this device is CURRENTLY in. Follows the + * capability flag (what the server actually tiers its sweep on), not the + * build β€” see LEGACY_HEARTBEAT_INTERVAL. + */ + private heartbeatIntervalMs(): number { + return this.transportCapableWritten === true + ? CAPABLE_HEARTBEAT_INTERVAL + : LEGACY_HEARTBEAT_INTERVAL; + } + async updateHeartbeat(deviceId: string) { if (!this.client) return; + // This write asserts status:'online' too, so it MUST respect the + // shutdown gate β€” otherwise a heartbeat firing (or in flight) as SIGINT + // lands can be applied after setOffline()'s subprocess write and leave + // an exited process marked online with a fresh last_seen, which for a + // capable device the sweep then cannot age out for a full tier window. + if (this.shuttingDown) { + console.debug('[DEBUG] Skipping heartbeat write β€” shutting down'); + return; + } try { - // Skip the write entirely when the channel is not joined. Bumping - // last_seen on a deaf device would keep its row perpetually young, so - // the server's staleness sweep could never age it out and correct a - // stale 'online' β€” and whenever presence is unavailable (kill switch, - // wedged socket) that stale row is exactly what dispatch falls back to. - // Staying silent lets the sweep do its job. - if (this.channel?.state !== 'joined') { - console.debug('[DEBUG] Skipping heartbeat write β€” channel not joined; letting the row age out'); + // Skip the write entirely when no transport is up. Bumping last_seen + // on a deaf device would keep its row perpetually young, so the + // server's staleness sweep could never age it out and correct a + // stale 'online' β€” and whenever presence is unavailable (kill + // switch, wedged socket) that stale row is exactly what dispatch + // falls back to. Staying silent lets the sweep do its job. + if (!this.isReachable()) { + console.debug('[DEBUG] Skipping heartbeat write β€” no transport joined; letting the row age out'); return; } @@ -903,7 +1123,6 @@ export class RemoteChannel { console.error('[DEBUG] Heartbeat update failed:', error.message); await captureRemote('remote_channel_heartbeat_error', { error }); } else { - // At 30-min cadence this is ~2 lines/hour β€” worth the visibility. console.debug('[DEBUG] last_seen bookkeeping write ok:', deviceId); } } catch (error: any) { @@ -914,20 +1133,38 @@ export class RemoteChannel { startHeartbeat(deviceId: string) { console.debug('[DEBUG] Starting heartbeat for device:', deviceId); + this.heartbeatDeviceId = deviceId; this.connectionCheckInterval = setInterval(() => { this.checkConnectionHealth(); }, 10000); - // Bookkeeping last_seen write (liveness itself rides Presence) - this.heartbeatInterval = setInterval(async () => { - await this.updateHeartbeat(deviceId); - }, HEARTBEAT_INTERVAL); - console.debug('[DEBUG] Heartbeat intervals set - connectionCheck: 10s, last_seen bookkeeping: 30min'); + // Bookkeeping last_seen write. Self-rescheduling rather than a fixed + // setInterval so the cadence can follow the tier: a device that + // withdraws the capability flag must fall back to the fast legacy + // cadence immediately, not 30 minutes later. + this.scheduleHeartbeat(); + console.debug(`[DEBUG] Heartbeat started - connectionCheck: 10s, last_seen: ${this.heartbeatIntervalMs()}ms`); + } + + /** Arm (or re-arm) the last_seen timer at the current tier's cadence. */ + private scheduleHeartbeat(): void { + if (this.heartbeatInterval) { + clearTimeout(this.heartbeatInterval); + this.heartbeatInterval = null; + } + if (!this.heartbeatDeviceId) return; + this.heartbeatInterval = setTimeout(async () => { + if (this.heartbeatDeviceId) { + await this.updateHeartbeat(this.heartbeatDeviceId); + } + this.scheduleHeartbeat(); // re-read the tier every tick + }, this.heartbeatIntervalMs()); } stopHeartbeat() { + this.heartbeatDeviceId = null; if (this.heartbeatInterval) { - clearInterval(this.heartbeatInterval); + clearTimeout(this.heartbeatInterval); this.heartbeatInterval = null; } if (this.connectionCheckInterval) { @@ -973,14 +1210,34 @@ export class RemoteChannel { console.debug('[DEBUG] setOffline() initiating blocking update for device:', deviceId); try { - // Get current session for the subprocess - const { data: sessionData } = await this.client.auth.getSession(); - - if (!sessionData?.session?.access_token) { + // Get a session for the subprocess β€” BOUNDED, with a fallback. + // + // auth.getSession() is not a cheap storage read: it takes a lock with + // a 10s acquire timeout, and it refreshes when the token is merely + // WITHIN ~90s of expiry, which POSTs /token with its own retry + // budget (~30s on retryable network errors). On a just-woken machine + // β€” token near expiry, wifi not re-associated β€” that is exactly the + // shape that blows device.ts's 5s force-exit, and then spawnSync + // never runs and the durable offline write never lands. That row + // then reads 'online' for the whole capable sweep tier, with every + // dispatch to it costing the caller a 5-minute timeout. + // + // The subprocess calls setSession() itself, so a slightly stale + // access_token is fine as long as the refresh_token is good. + const live = await Promise.race([ + this.client.auth.getSession().then((r) => r.data?.session ?? null), + this.sleep(OFFLINE_SESSION_TIMEOUT_MS).then(() => null), + ]).catch(() => null); + const session = live ?? this.lastKnownSession; + + if (!session?.access_token) { console.error('❌ No valid session for offline update'); console.debug('[DEBUG] Session data missing or invalid'); return; } + if (!live) { + console.debug('[DEBUG] getSession() slow/failed β€” using last known session tokens'); + } // Get Supabase config from client const supabaseUrl = (this.client as any).supabaseUrl; @@ -1010,8 +1267,8 @@ export class RemoteChannel { deviceId, supabaseUrl, supabaseKey, - sessionData.session.access_token, - sessionData.session.refresh_token || '' + session.access_token, + session.refresh_token || '' ], { timeout: 3000, stdio: 'pipe', // Capture output to prevent blocking @@ -1050,10 +1307,47 @@ export class RemoteChannel { } async unsubscribe() { + // Teardown has begun: from here on, setOffline()'s durable write is the + // authoritative final word on `status`, so stop every other writer + // (channel callbacks AND the heartbeat) from racing it. Otherwise a + // late 'online' is applied after the subprocess has written 'offline', + // leaving an exited process marked online until the sweep ages it out β€” + // and every dispatch in between costs the caller a 5-minute timeout. + // + // The window that actually needs this is NOT the CLOSED fired by the + // unsubscribe below: realtime-js sets state='leaving' on the first line + // of unsubscribe() and removeChannel() calls it synchronously, so by the + // time that CLOSED lands isReachable() already reads false and the write + // would have been 'offline' anyway. The real races are: + // 1. a heartbeat tick (every 15s in the legacy tier) firing or already + // in flight as the signal arrives β€” see updateHeartbeat, and + // 2. SIGINT arriving while recreateChannel() sits in its jittered + // backoff (up to ~45s): this.channel is already null so unsubscribe + // skips its block, setOffline writes 'offline', then the backoff + // expires during desktop.shutdown() and the fresh join's SUBSCRIBED + // queues 'online' after the durable write. + this.shuttingDown = true; + // BUDGET: device.ts force-exits 5s after the signal, and the durable + // offline write (setOffline's spawnSync, 3s cap + a bounded session + // fetch, 0.5s) is the one thing this whole path exists to produce. So + // everything before it must be tightly bounded: + // 250ms drain + 500ms untrack + 500ms(Γ—2, see below) + 500ms session + // + 3000ms spawnSync β‰ˆ 4.75s worst case + // Only the untrack bound can actually bind: removeChannel/unsubscribe + // both set state='leaving' first, which makes realtime-js's _canPush() + // false so the leave push resolves 'ok' inline rather than waiting out + // its 10s timeout. The other two bounds are cheap insurance, not load- + // bearing β€” do not "reclaim" the budget by removing the untrack one. + const LEAVE_BOUND_MS = 500; + // Drain the QUEUED channel-callback writes. This cannot drain a + // heartbeat PATCH β€” updateHeartbeat writes directly, not through the + // chain β€” but the gate above stops any NEW heartbeat, and one already in + // flight necessarily started before this point. + await Promise.race([this.statusWriteChain, this.sleep(250)]); // Bounded like the untrack below: removeChannel() sends a leave push that // only settles via realtime-js's 10s timeout on a half-open socket, which // would blow device.ts's 5s force-exit and skip the durable offline write. - await Promise.race([this.removeLegacyChannel(), this.sleep(1000)]); + await Promise.race([this.removeLegacyChannel(), this.sleep(LEAVE_BOUND_MS)]); if (this.channel) { // Leave presence explicitly on the graceful path (socket close // covers the abrupt one). @@ -1062,15 +1356,20 @@ export class RemoteChannel { // as 'joined', so a state check alone is not enough β€” the presence // push just buffers and settles via realtime-js's 10s timeout, // which blows past device.ts's 5s force-exit and would skip both - // unsubscribe() and the durable offline write. Race it against a - // 1s cap; a dropped socket clears server-side presence anyway. + // unsubscribe() and the durable offline write. A dropped socket + // clears server-side presence anyway. await Promise.race([ this.channel.untrack(), - this.sleep(1000), + this.sleep(LEAVE_BOUND_MS), ]); - console.debug('[DEBUG] Presence untrack attempted (bounded at 1s)'); + console.debug('[DEBUG] Presence untrack attempted (bounded)'); } catch { /* best effort */ } - await this.channel.unsubscribe(); + // Bounded as insurance only. unsubscribe() sets state='leaving' on + // its first line, so _canPush() is false and the leave push resolves + // 'ok' inline β€” it cannot actually wait out the 10s push timeout the + // way the untrack above can. Kept because it costs nothing and the + // guarantee lives in library internals, not in our contract. + await Promise.race([this.channel.unsubscribe(), this.sleep(LEAVE_BOUND_MS)]); this.channel = null; console.log('βœ“ Unsubscribed from tool call channel'); } diff --git a/src/remote-device/scripts/blocking-offline-update.js b/src/remote-device/scripts/blocking-offline-update.js index 04a99fae..2f113f01 100644 --- a/src/remote-device/scripts/blocking-offline-update.js +++ b/src/remote-device/scripts/blocking-offline-update.js @@ -43,7 +43,7 @@ try { // Update device status to offline, stamping the exact shutdown moment so // "last seen X ago" is precise for clean shutdowns (the periodic - // bookkeeping write only runs every 30 minutes). + // bookkeeping write only runs on the slow capable cadence). const { error } = await client .from('mcp_devices') .update({ status: 'offline', last_seen: new Date().toISOString() }) diff --git a/test/test-remote-heartbeat-tier.js b/test/test-remote-heartbeat-tier.js new file mode 100644 index 00000000..648309d7 --- /dev/null +++ b/test/test-remote-heartbeat-tier.js @@ -0,0 +1,469 @@ +/** + * Regression test: the device's last_seen heartbeat must stay in step with the + * SERVER-SIDE offline-sweep tier that judges it. + * + * The server tiers its sweep on the CAPABILITY FLAG, not the app version: + * - no transport_broadcast_v1 -> legacy tier, DEVICE_OFFLINE_TIMEOUT_MS = 45s + * - transport_broadcast_v1 -> capable tier, DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS = 15min + * + * A new-build device only sets the flag AFTER a successful presence track(), so + * between registration and that moment β€” and permanently, if presence can never + * be proven (migration 008 not applied, an authz hiccup, exhausted track() + * retries) β€” it is judged by the 45s rule. Heartbeating on the slow capable + * cadence in that state means the sweep marks it offline ~45s after it registers + * and dispatch then throws "No devices available" forever, even though its + * INDEPENDENT legacy postgres_changes channel is joined and could run every call. + * + * Two invariants, both regressions found in review 2026-07-26: + * 1. cadence follows the tier (fast while legacy, slow only once capable), and + * re-arms the moment the tier changes. + * 2. the heartbeat write is gated on being reachable by ANY transport, not on + * the private channel specifically β€” gating on the private channel starves + * last_seen for exactly the devices the legacy channel is meant to rescue. + * + * Standalone: node test/test-remote-heartbeat-tier.js (needs npm run build). + */ +import assert from 'node:assert'; +import { RemoteChannel } from '../dist/remote-device/remote-channel.js'; + +process.env.DESKTOP_COMMANDER_DISABLE_TELEMETRY = '1'; + +// The server-side thresholds this device must stay inside. HAND-COPIED from +// remote-dc-mcp/src/server/constants.ts β€” the two repos ship independently, so +// NOTHING enforces this copy: lowering DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS on the +// server will NOT fail this test. When you change either threshold, change it +// here too. (PUBLISH.md's release gate repeats this pairing for the same reason.) +// What this test does guarantee is the DEVICE half: that its cadence, whatever +// tier it is in, fits inside the threshold recorded below. +const SERVER_LEGACY_OFFLINE_TIMEOUT_MS = 45 * 1000; +const SERVER_CAPABLE_OFFLINE_TIMEOUT_MS = 15 * 60 * 1000; + +function makeChannel(state) { + return { state }; +} + +/** + * Minimal client that records mcp_devices writes. + * + * `removeChannel` / `realtime.disconnect` are REQUIRED, not decorative: + * recreateChannel() calls both, and without them it dies on a TypeError before + * reaching anything the recreate tests stub β€” which made those tests pass for + * the wrong reason (caught in review round 8). + */ +function makeFakeClient() { + const writes = []; + return { + writes, + removeChannel: () => Promise.resolve('ok'), + realtime: { disconnect: () => Promise.resolve() }, + from() { + const chain = { + update: (payload) => { + writes.push(payload); + return chain; + }, + select: () => chain, + eq: () => Promise.resolve({ error: null }), + }; + return chain; + }, + }; +} + +function makeRemoteChannel() { + const rc = new RemoteChannel(); + const client = makeFakeClient(); + rc.client = client; // private at TS level, plain property at runtime + rc._user = { id: 'user-1', email: 'tester@example.com' }; + rc.deviceId = 'device-1'; + rc.deviceName = 'test-device'; + rc.onToolCall = () => {}; + return { rc, client }; +} + +let failures = 0; +async function test(name, fn) { + try { + await fn(); + console.log(`βœ… PASS ${name}`); + } catch (e) { + failures++; + console.error(`πŸ”΄ FAIL ${name}\n ${e.message}`); + } +} + +async function main() { + // 1. A device that has NOT proven the transport must heartbeat fast enough to + // survive the server's 45s legacy sweep. + await test('legacy tier heartbeats inside the server 45s sweep threshold', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = null; // never written = legacy tier + const cadence = rc.heartbeatIntervalMs(); + assert.ok( + cadence * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, + `legacy cadence ${cadence}ms must allow >=2 writes inside the ` + + `${SERVER_LEGACY_OFFLINE_TIMEOUT_MS}ms sweep window` + ); + + rc.transportCapableWritten = false; // capability explicitly withdrawn + assert.strictEqual( + rc.heartbeatIntervalMs(), + cadence, + 'a withdrawn capability must use the same fast legacy cadence' + ); + }); + + // 2. Once the transport is proven, presence carries liveness and the durable + // write drops to bookkeeping cadence β€” but must still beat the 65min tier. + await test('capable tier heartbeats inside the server capable sweep threshold', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + const cadence = rc.heartbeatIntervalMs(); + assert.ok( + cadence * 2 < SERVER_CAPABLE_OFFLINE_TIMEOUT_MS, + `capable cadence ${cadence}ms must allow >=2 writes inside the ` + + `${SERVER_CAPABLE_OFFLINE_TIMEOUT_MS}ms sweep window` + ); + assert.ok( + cadence > SERVER_LEGACY_OFFLINE_TIMEOUT_MS, + 'capable cadence should be the slow bookkeeping one, not the legacy rate' + ); + }); + + // 3. Withdrawing the capability must re-arm the timer immediately. Before the + // fix the cadence was a fixed setInterval chosen once at startup, so a + // device dropping to the legacy tier kept writing every 30 min and got + // swept offline 45s later. + await test('withdrawing the capability re-arms the heartbeat at the fast cadence', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.channel = makeChannel('joined'); + rc.startHeartbeat('device-1'); + try { + const armed = []; + const realSetTimeout = globalThis.setTimeout; + globalThis.setTimeout = (fn, ms) => { + armed.push(ms); + return realSetTimeout(() => {}, 0); // never actually fire + }; + try { + await rc.setTransportCapable(false); + } finally { + globalThis.setTimeout = realSetTimeout; + } + + assert.ok(armed.length > 0, 'withdrawing the capability must re-arm the heartbeat timer'); + assert.ok( + armed[armed.length - 1] * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, + `re-armed cadence ${armed[armed.length - 1]}ms must fit the 45s legacy sweep` + ); + } finally { + rc.stopHeartbeat(); + } + }); + + // 4. THE BLACKOUT CASE. Private channel dead (008 missing / authz failure), + // legacy channel joined and delivering. The device is genuinely usable, so + // it MUST keep last_seen fresh or the sweep makes it undispatchable. + await test('heartbeat writes when only the legacy channel is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = null; // private channel never joined + rc.legacyChannel = makeChannel('joined'); // the safety net is up + await rc.updateHeartbeat('device-1'); + assert.strictEqual( + client.writes.length, + 1, + 'a device reachable only via the legacy channel must still write last_seen' + ); + assert.ok(client.writes[0].last_seen, 'write should bump last_seen'); + assert.strictEqual(client.writes[0].status, 'online', 'write should assert online'); + }); + + // 5. ...but a genuinely deaf device must still go silent, so the server's + // staleness sweep can age its row out and correct a stale 'online'. + await test('heartbeat stays silent when no transport is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('errored'); + rc.legacyChannel = makeChannel('closed'); + await rc.updateHeartbeat('device-1'); + assert.strictEqual( + client.writes.length, + 0, + 'a deaf device must NOT refresh last_seen β€” the sweep has to be able to age it out' + ); + }); + + // 6. Private channel healthy is of course still reachable. + await test('heartbeat writes when the private channel is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('joined'); + rc.legacyChannel = null; + await rc.updateHeartbeat('device-1'); + assert.strictEqual(client.writes.length, 1, 'private channel joined = reachable'); + }); + + // 7. THE OSCILLATION CASE. `status` is transport-agnostic β€” the server's + // resolveTargetDevice filters on it β€” so a private-channel failure must + // NOT write 'offline' while the legacy channel is still delivering. + // realtime-js re-fires CHANNEL_ERROR on every failed rejoin, so the old + // unconditional write left the row flipping offline/online against the + // heartbeat and failed ~half of all dispatches. + await test('private-channel failure keeps status online while legacy is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('errored'); // private join keeps failing + rc.legacyChannel = makeChannel('joined'); // safety net delivering fine + + rc.syncReachabilityStatus(); + await rc.statusWriteChain; + + assert.strictEqual(client.writes.length, 1, 'one status write'); + assert.strictEqual( + client.writes[0].status, + 'online', + 'a device still reachable via the legacy channel must stay online' + ); + }); + + // 8. ...but when NO transport is up the device must go offline. + await test('status goes offline when no transport is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('errored'); + rc.legacyChannel = makeChannel('closed'); + + rc.syncReachabilityStatus(); + await rc.statusWriteChain; + + assert.strictEqual(client.writes[0].status, 'offline', 'genuinely deaf device goes offline'); + }); + + // 9. Status writes must land in the order they were issued: a teardown's + // 'offline' must not overtake the subsequent join's 'online'. + await test('concurrent status writes stay ordered', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('joined'); + + rc.queueStatusWrite('offline'); // teardown + rc.queueStatusWrite('online'); // immediate re-join + await rc.statusWriteChain; + + assert.strictEqual(client.writes.length, 2, 'both writes issued'); + assert.deepStrictEqual( + client.writes.map((w) => w.status), + ['offline', 'online'], + 'writes must apply in issue order so the join wins' + ); + }); + + // 10. Once teardown starts, setOffline()'s durable write owns `status`. A + // reachability write queued after that point would be flushed only after + // setOffline's blocking spawnSync had already written 'offline', leaving a + // shut-down device marked online until the sweep aged it out. + await test('status writes are suppressed once teardown has started', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('joined'); + rc.legacyChannel = makeChannel('joined'); + + rc.shuttingDown = true; + rc.syncReachabilityStatus(); + rc.queueStatusWrite('online'); + await rc.statusWriteChain; + + assert.strictEqual( + client.writes.length, + 0, + 'no status write may be issued after teardown starts' + ); + }); + + // 11. unsubscribe() must be bounded: on a half-open socket the leave push only + // settles via realtime-js's 10s timeout, which blows device.ts's 5s + // force-exit and skips setOffline() entirely. + await test('unsubscribe is bounded and still clears the channel', async () => { + const { rc } = makeRemoteChannel(); + let cleared = false; + rc.legacyChannel = null; + rc.channel = { + state: 'joined', + untrack: () => new Promise(() => {}), // never settles + unsubscribe: () => new Promise(() => {}), // never settles (half-open) + }; + // Keep the bound short so the test doesn't wait on real timers. + rc.sleep = () => Promise.resolve(); + + await rc.unsubscribe(); + cleared = rc.channel === null; + assert.ok(cleared, 'unsubscribe must give up on a wedged leave push and move on'); + assert.strictEqual(rc.shuttingDown, true, 'teardown flag set'); + }); + + // 12. THE OVERLAY BLACKOUT. For a FLAGGED device the server treats absent + // Presence as authoritative offline and applies that overlay before + // selection, so it outranks `status` entirely. A device that keeps the flag + // while unable to join the private channel is therefore undispatchable no + // matter how healthy its legacy channel is. Until this fix the flag could + // only be cleared from trackPresenceInner, which is unreachable unless the + // channel is already 'joined' β€” so such a device never recovered without a + // process restart. + await test('sustained recreate failure withdraws the transport capability', async () => { + const { rc, client } = makeRemoteChannel(); + rc.transportCapableWritten = true; // previously proven and advertised + rc.legacyChannel = makeChannel('joined'); + rc.sleep = () => Promise.resolve(); // skip the jittered backoff + // A channel that can never be rebuilt (008 dropped / RLS denies the join). + const order = []; + rc.createChannel = () => { + order.push('private'); + return Promise.reject(new Error('Unauthorized')); + }; + rc.createLegacyChannel = () => { + order.push('legacy'); + }; + rc.channel = makeChannel('errored'); + + for (let i = 0; i < 3; i++) { + await rc.recreateChannel(); + } + + // The recreate must genuinely have reached createChannel β€” not died earlier + // on a missing client method β€” and must rebuild the legacy safety net FIRST + // and unconditionally, so a private-channel outage never leaves the fallback + // dead (the round-5 invariant, previously uncovered). + assert.deepStrictEqual( + order.slice(0, 2), + ['legacy', 'private'], + `legacy net must be rebuilt before the private channel: ${JSON.stringify(order)}` + ); + assert.strictEqual( + order.filter((o) => o === 'legacy').length, + 3, + 'the legacy net must be rebuilt on EVERY recreate attempt' + ); + + assert.strictEqual( + rc.transportCapableWritten, + false, + 'the capability must be withdrawn after sustained private-channel failure' + ); + const capWrite = client.writes.find((w) => w.capabilities); + assert.ok(capWrite, 'a capabilities write should have been issued'); + assert.strictEqual( + capWrite.capabilities.transport_broadcast_v1, + undefined, + 'the withdrawn payload must not carry the flag' + ); + assert.strictEqual( + capWrite.capabilities.app_version !== undefined, + true, + 'app_version must survive the whole-column overwrite' + ); + }); + + // 13. A transient failure must NOT flap the capability β€” realtime-js re-fires + // CHANNEL_ERROR on every rejoin, so withdrawing eagerly would churn the DB + // and bounce the device between tiers. + await test('a single recreate failure does not withdraw the capability', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.legacyChannel = makeChannel('joined'); + rc.sleep = () => Promise.resolve(); + rc.createChannel = () => Promise.reject(new Error('transient')); + rc.createLegacyChannel = () => {}; + rc.channel = makeChannel('errored'); + + await rc.recreateChannel(); + + assert.strictEqual( + rc.transportCapableWritten, + true, + 'one blip must not withdraw the capability' + ); + }); + + // 14. The heartbeat asserts status:'online', so it must respect the shutdown + // gate too β€” otherwise a tick landing after setOffline()'s subprocess write + // leaves an exited process marked online with a fresh last_seen. + await test('heartbeat is suppressed once shutting down', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannel('joined'); + rc.shuttingDown = true; + await rc.updateHeartbeat('device-1'); + assert.strictEqual(client.writes.length, 0, 'no heartbeat write during shutdown'); + }); + + // 15. The withdrawal runs in recreateChannel()'s catch, which + // RECREATE_TIMEOUT_MS does not cover. A hanging capabilities PATCH must not + // pin isRecreatingChannel=true, or the 10s connection watchdog is silently + // disabled and the device can never recover. + await test('a hanging capability withdrawal cannot pin the recreate guard', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.legacyChannel = makeChannel('joined'); + rc.sleep = () => Promise.resolve(); + rc.createChannel = () => Promise.reject(new Error('Unauthorized')); + rc.createLegacyChannel = () => {}; + rc.channel = makeChannel('errored'); + // A capabilities write that never settles. + rc.setTransportCapable = () => new Promise(() => {}); + // Keep the bound short so the test doesn't wait 5s of real time. + const realWithTimeout = rc.withTimeout.bind(rc); + rc.withTimeout = (op, _ms, name) => realWithTimeout(op, 20, name); + + for (let i = 0; i < 3; i++) { + await rc.recreateChannel(); + } + + assert.strictEqual( + rc.isRecreatingChannel, + false, + 'the re-entrancy guard must be released even when the withdrawal hangs' + ); + }); + + // 16. setOffline() must not be blocked by auth.getSession(). It takes a lock + // with a 10s acquire timeout and refreshes when the token is merely within + // ~90s of expiry, POSTing /token with its own ~30s retry budget β€” so on a + // just-woken machine it can outlast device.ts's 5s force-exit, and then + // spawnSync never runs and the durable offline write is lost entirely. + await test('setOffline does not hang when getSession stalls', async () => { + const { rc } = makeRemoteChannel(); + rc.client.auth = { getSession: () => new Promise(() => {}) }; // never settles + rc.lastKnownSession = { access_token: 'cached-at', refresh_token: 'cached-rt' }; + // Missing supabase config makes setOffline return right after the session + // step, so this test isolates the session fetch and spawns no subprocess. + rc.client.supabaseUrl = undefined; + rc.client.supabaseKey = undefined; + + // The real assertion: it must SETTLE. Without the bound on getSession the + // await below never returns and this test times out. + let settled = false; + await Promise.race([ + rc.setOffline('device-1').then(() => { + settled = true; + }), + new Promise((r) => setTimeout(r, 3000)), + ]); + + assert.strictEqual( + settled, + true, + 'setOffline must settle on a stalled getSession rather than block the shutdown path' + ); + }); + + // 17. stopHeartbeat must not leave a timer able to re-arm itself. + await test('stopHeartbeat halts the self-rescheduling timer', async () => { + const { rc } = makeRemoteChannel(); + rc.channel = makeChannel('joined'); + rc.startHeartbeat('device-1'); + rc.stopHeartbeat(); + assert.strictEqual(rc.heartbeatInterval, null, 'timer handle cleared'); + assert.strictEqual(rc.heartbeatDeviceId, null, 'device id cleared so re-arm is a no-op'); + rc.scheduleHeartbeat(); // must be inert after stop + assert.strictEqual(rc.heartbeatInterval, null, 'scheduleHeartbeat after stop must not re-arm'); + }); + + console.log(`\n${failures ? 'πŸ”΄' : 'βœ…'} remote heartbeat tier: ${failures} failing test(s).`); + process.exit(failures ? 1 : 0); +} + +main(); From ab7b85a4175d47fe253f43a364a12763ba0616d3 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sun, 26 Jul 2026 17:21:36 +0300 Subject: [PATCH 09/13] Keep the release gate out of the repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Broadcast/Presence release gate lives outside both repos, alongside the manual test plan, rather than in PUBLISH.md. Reverts PUBLISH.md to main's content so base..head no longer touches it. The gate content itself is unchanged and still required reading before any npm release carrying transport_broadcast_v1 β€” it is kept with the operator's other rollout material. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR --- PUBLISH.md | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/PUBLISH.md b/PUBLISH.md index 5a5444fc..df96297d 100644 --- a/PUBLISH.md +++ b/PUBLISH.md @@ -2,33 +2,6 @@ This document outlines the complete process for publishing new versions of Desktop Commander to both NPM and the MCP Registry. -## β›” RELEASE GATE β€” remote device (Broadcast/Presence transport) - -**Applies to any release containing the Broadcast/Presence remote-device -transport (`capabilities.transport_broadcast_v1`, private `user:{id}` channel).** -Publishing it before the backend is ready **bricks remote use for every device -that updates**. Verify BOTH against **prod** (`olvbkozcufcbptfogatw`) first: - -1. **Migration 008 is applied** β€” the private-channel policies exist: - ```sql - SELECT policyname FROM pg_policies - WHERE schemaname='realtime' AND tablename='messages'; - -- expect: "users receive on own channel", "users send on own channel" - ``` - Without it the device cannot join `user:{id}`, so it loses the doorbell - transport. (The legacy `postgres_changes` listener rides its own public - channel, so the device degrades rather than going dark β€” but it is still a - broken release.) - -2. **The new server is deployed** (the `v*` tag that knows about - `transport_broadcast_v1`). The old server's flat 45-second offline sweep - marks these devices offline 45s after registration β€” they heartbeat every - 30 minutes β€” and dispatch then fails against them. - -Order is strict: **008 β†’ server deploy β†’ this npm release.** Full rationale, -failure modes and rollback rules: `remote-dc-mcp/migrations/README.md` and -`BROADCAST_PRESENCE_RUNBOOK.md`. - ## πŸš€ Automated Release (Recommended) We now have an automated release script that handles the entire process with **automatic state tracking and resume capability**! From 1ae35a5a46dcf243860a24502b925c36a03de160 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sun, 26 Jul 2026 21:01:08 +0300 Subject: [PATCH 10/13] Consolidate the remote tests and cut the comment volume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tests: 5 artefacts for this feature down to 3, by subject. - delete test/bench-strip-null-bytes.mjs β€” a micro-benchmark with no assertions, so it could never fail while still costing a slot in npm test - merge test-remote-dedupe-and-doorbell.js and test-remote-heartbeat-tier.js into test-remote-transport.js (28 cases: exactly-once, doorbell routing, result ordering, heartbeat tiers, reachability/status, capability withdrawal, shutdown), which also removes a duplicated Supabase fake - test-remote-channel-reconnect.js and test-strip-null-bytes.js keep their own subjects Comments in remote-channel.ts: 498 lines to 308 (36% -> 25% of the file), no code change. Removed the "an earlier version did X, which broke Y" narratives β€” that belongs in git history and is already in the round-6/7/8 commit message β€” compressed the constants block, and dropped the degraded-path rationale that was restated in several places. What stays is the non-obvious mechanics: why the teardown calls are bounded, why the withdrawal needs 3 attempts and not 2, why the two heartbeat cadences have to match the server's sweep tiers. Still above the repo's 12-17% baseline because the new code carries invariants that are genuinely easy to get wrong, and each surviving note exists because a review caught it being wrong first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR --- src/remote-device/remote-channel.ts | 470 +++++++---------------- test/bench-strip-null-bytes.mjs | 42 -- test/test-remote-dedupe-and-doorbell.js | 181 --------- test/test-remote-heartbeat-tier.js | 469 ----------------------- test/test-remote-transport.js | 488 ++++++++++++++++++++++++ 5 files changed, 628 insertions(+), 1022 deletions(-) delete mode 100644 test/bench-strip-null-bytes.mjs delete mode 100644 test/test-remote-dedupe-and-doorbell.js delete mode 100644 test/test-remote-heartbeat-tier.js create mode 100644 test/test-remote-transport.js diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index f31f32e1..e38287d9 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -6,16 +6,11 @@ const NUL_CHAR = String.fromCharCode(0); const NUL_RE = new RegExp(NUL_CHAR, 'g'); /** - * Recursively strip real NUL characters (U+0000) from strings and object keys. - * Postgres cannot store a NUL in jsonb OR text and rejects the whole write with - * 22P05, which strands the call at 'executing' until the 5-min timeout. - * - * Walks the structure instead of round-tripping through JSON: an earlier - * serialize-and-regex version matched the ESCAPE TEXT rather than the character, - * so legitimate content containing the six literal chars backslash-u-0-0-0-0 - * (e.g. reading a source file with that escape in it) was silently corrupted, - * and a doubled-backslash form produced invalid JSON that threw. Both cases are - * covered by tests in test/test-strip-null-bytes.js. + * Recursively strip NUL characters (U+0000) from strings and object keys. + * Postgres rejects NUL in jsonb and text (22P05), failing the whole write and + * stranding the call at 'executing' until the 5-min timeout. + * Walks the structure rather than round-tripping JSON, which would match the + * escape text and corrupt legitimate content. See test/test-strip-null-bytes.js. */ export function stripNullBytes(value: T): T { if (typeof value === 'string') { @@ -52,104 +47,50 @@ interface DeviceData { last_seen: string; } -// Bookkeeping cadence for the durable last_seen column ONCE the broadcast -// transport is proven. Liveness is then carried by Presence (websocket-level, -// flips in seconds), so this write is not the live signal β€” but it IS the -// server's fallback whenever presence is unavailable, which is why it cannot be -// slow. Paired with DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS = 15 min (3 missed -// writes); the full rationale for that pairing, and the list of states that -// reach it, lives on that constant in remote-dc-mcp/src/server/constants.ts. +// last_seen cadences. The server tiers its offline sweep on the +// transport_broadcast_v1 flag (not the app version), so each cadence must fit +// its tier's threshold in remote-dc-mcp/src/server/constants.ts: +// capable -> 15 min, unflagged -> 45s. Too slow in the legacy tier and the sweep +// blacks out a device whose legacy channel is still delivering. const CAPABLE_HEARTBEAT_INTERVAL = 5 * 60 * 1000; -// Cadence while this device is in the LEGACY tier β€” i.e. whenever -// transport_broadcast_v1 is not currently advertised, i.e. whenever presence -// has not (yet) been proven. MUST stay well inside the server's legacy sweep -// threshold (DEVICE_OFFLINE_TIMEOUT_MS = 45s), because the server tiers the -// sweep on the CAPABILITY FLAG, not on the app version: a device without the -// flag is judged by the 45s rule no matter how new its build is. -// -// Getting this wrong is not a slow degradation, it is a blackout: on the slow -// capable cadence a device that cannot prove the private channel (008 not applied, -// an authz hiccup, exhausted track() retries) is swept offline ~45s after -// registering and dispatch then throws "No devices available" forever β€” even -// though its INDEPENDENT legacy postgres_changes channel is joined and would -// deliver calls perfectly. That blackout is exactly what the independent -// legacyChannel exists to prevent, so the two must be kept in step. const LEGACY_HEARTBEAT_INTERVAL = 15 * 1000; -// Cap the channel-rebuild portion of a recreate so a hung await can't pin the -// re-entrancy guard true (which would silently disable the connection watchdog). -// NOTE: the jittered backoff sleep runs BEFORE this cap and inside the guard, so -// the total window in which checkConnectionHealth is a no-op is -// backoff (<=45s) + RECREATE_TIMEOUT_MS β€” bounded at ~75s, not 30s. -// Must exceed createChannel()'s worst case, which now includes -// trackPresenceWithRetry: 3 track() pushes at realtime-js's 10s DEFAULT_TIMEOUT -// plus 0.5s+1s backoff = ~31.5s. At the old 30s these two constants were in -// silent conflict β€” any recreate where presence never acked ALWAYS timed out. +// Cap on a recreate's rebuild step, so a hung await can't pin +// isRecreatingChannel and disable the watchdog. Must exceed createChannel()'s +// worst case (~31.5s of presence retries). const RECREATE_TIMEOUT_MS = 45000; -// Max time the channel may sit CONTINUOUSLY in 'joining' before we force a recreate. -// 'joining' is normally healthy (we let realtime-js's rejoin backoff converge), but on a -// HALF-OPEN socket (readyState OPEN yet dead) realtime-js parks the channel in 'joining' -// forever and never reconnects the socket β€” the device then wedges offline silently with -// no recreate firing. realtime-js's join push times out in ~10s, so a genuine join -// resolves/errors well within this window; 3 health ticks of unbroken 'joining' means the -// state machine has stalled and only a fresh socket (via recreate) recovers it. +// Max continuous time in 'joining' before forcing a recreate β€” a half-open +// socket parks the channel there forever, and a genuine join settles in ~10s. const JOINING_WEDGE_TIMEOUT_MS = 30000; -// Consecutive failed channel recreates after which this device WITHDRAWS -// transport_broadcast_v1. -// -// This is load-bearing, not tidiness. For a flagged device the server treats -// absent Presence as AUTHORITATIVE offline and applies that overlay BEFORE -// selection β€” it outranks the `status` column entirely. So a device that keeps -// the flag while unable to join the private channel is undispatchable no matter -// how healthy its legacy channel is, and no matter how fresh its last_seen. -// Withdrawing the promise is what drops it back to a tier the server will still -// route to (see setTransportCapable / the rollback runbook's stage rules). -// -// The withdrawal deliberately does NOT hang off CHANNEL_ERROR: realtime-js -// re-fires that on every failed rejoin, so a momentary blip would flap the -// capability and its DB write. Gate on sustained failure instead β€” 3 recreates -// is ~30s given the jittered backoff. A later successful track() re-advertises -// automatically. -// -// DO NOT LOWER THIS TO 2. Ordinary half-open recovery legitimately costs two -// attempts: removeChannel() disconnects the socket when the last channel leaves, -// and realtime-js then refuses to reconnect for ~100ms while _connectionState is -// 'disconnecting', so the FIRST recreate's join pushes are buffered and burn the -// full 10s join timeout β€” the second dials the fresh socket and succeeds. At 2, -// every routine wifi drop would withdraw the capability and churn the DB. +// Failed recreates before withdrawing transport_broadcast_v1: keeping the flag +// while unable to join makes the device undispatchable. Not lower than 3 β€” +// ordinary half-open recovery costs 2, since the first recreate's join is +// buffered while realtime-js refuses to redial the socket it just dropped. const TRANSPORT_WITHDRAW_AFTER_ATTEMPTS = 3; -// Bound on the capability-withdrawal write. It runs in recreateChannel()'s catch -// block, outside RECREATE_TIMEOUT_MS's reach, so it needs its own cap. +// Cap on the withdrawal write; it runs in a catch block RECREATE_TIMEOUT_MS +// does not cover. const CAPABILITY_WRITE_TIMEOUT_MS = 5000; -// Bound on the shutdown path's session fetch. See setOffline() β€” auth.getSession() -// can block on a token refresh, and this runs against device.ts's 5s force-exit. +// Cap on the shutdown session fetch, which races device.ts's 5s force-exit. const OFFLINE_SESSION_TIMEOUT_MS = 500; export class RemoteChannel { private client: SupabaseClient | null = null; private channel: RealtimeChannel | null = null; /** - * TRANSITION ONLY (removed at the flip): the legacy postgres_changes - * listener lives on its OWN public channel, deliberately NOT on the private - * user channel. It is the safety net for the broadcast transport, so it - * must not share a failure mode with it β€” if it rode the private channel, - * an 008 policy problem or any private-channel auth failure would take out - * BOTH transports at once and the device would go completely dark instead - * of degrading to the old path. Costs one extra channel per device (the - * device's own connection carries 2, far under the 100/connection quota). + * Legacy postgres_changes listener, on its OWN public channel β€” deliberately + * not the private one, so a private-channel auth failure can't take both + * transports down. Removed at the flip (009). */ private legacyChannel: RealtimeChannel | null = null; private heartbeatInterval: NodeJS.Timeout | null = null; private connectionCheckInterval: NodeJS.Timeout | null = null; - // Device whose last_seen the heartbeat timer maintains; null = stopped. - // Held so scheduleHeartbeat() can re-arm itself without re-plumbing the id. + /** Device the heartbeat timer maintains; null = stopped, so re-arm is inert. */ private heartbeatDeviceId: string | null = null; // Single-slot queue keeping concurrent `status` PATCHes in order. private statusWriteChain: Promise = Promise.resolve(); - // Tokens from the last successful setSession / TOKEN_REFRESHED, so the - // shutdown path never has to wait on auth.getSession(). See setOffline(). + /** Tokens from the last setSession / TOKEN_REFRESHED, for setOffline(). */ private lastKnownSession: { access_token: string; refresh_token: string | null } | null = null; - // Set once unsubscribe() starts: suppresses further reachability-driven - // status writes so they cannot land after setOffline()'s durable write. + /** Set by unsubscribe(): suppresses status/heartbeat writes so they can't + * land after setOffline()'s durable write. */ private shuttingDown = false; @@ -159,17 +100,13 @@ export class RemoteChannel { private onToolCall: ((payload: any) => void) | null = null; // Guard so setSession being called twice can't stack auth listeners. private authListenerRegistered = false; - // False when presence publishing failed on a channel that is otherwise - // healthy β€” the health check re-tries, since nothing else would (a joined - // channel never re-fires SUBSCRIBED) and the server would keep reporting - // this device offline. + /** False when presence publishing failed on an otherwise healthy channel; + * the health check retries, since SUBSCRIBED won't fire again. */ private presenceTracked = false; - // Last capability value written to the DB (null = not yet written), so the - // flag isn't re-written on every reconnect. + /** Last capability value written (null = never), to avoid redundant writes. */ private transportCapableWritten: boolean | null = null; - // Re-entrancy guard for the presence self-heal: on a wedged socket each - // track() buffers for the full 10s push timeout, so unguarded 10s health - // ticks would stack pending pushes. + /** Re-entrancy guard: on a wedged socket each track() buffers for the full + * 10s push timeout, so 10s health ticks would stack pushes. */ private isTrackingPresence = false; // Track last device status to prevent duplicate log messages @@ -178,10 +115,9 @@ export class RemoteChannel { // Track last channel state for debug logging private lastChannelState: string | null = null; - // Reconnect diagnostics + guard (see connState() / recreateChannel()) - private reconnectAttempt = 0; // recreateChannel() attempts since last success - private isRecreatingChannel = false; // a recreate is in flight (re-entrancy guard) - private joiningSince: number | null = null; // ts the channel entered an unbroken 'joining' run; null when not joining + private reconnectAttempt = 0; // recreates since the last success + private isRecreatingChannel = false; // re-entrancy guard + private joiningSince: number | null = null; // start of an unbroken 'joining' run private _user: User | null = null; get user(): User | null { return this._user; } @@ -223,13 +159,10 @@ export class RemoteChannel { this._user = user; console.debug('[DEBUG] Session set successfully, user:', user.email); - // Private channels authorize with the user JWT at join time. supabase-js - // generally forwards auth to realtime itself; this is defensive and must - // push the CURRENT session token, not the one we were handed: - // auth.setSession() refreshes an expired token internally (device asleep - // >1h with --persist-session), and pushing the stale parameter here would - // overwrite the fresh token realtime already had β€” every private-channel - // join then fails until the next refresh (~50 min deaf). + // Private channels authorize with the user JWT at join time. Push the + // CURRENT token, not the one we were handed: setSession() refreshes an + // expired token internally, and pushing the stale parameter would + // overwrite it, failing every join until the next refresh. const { data: { session: currentSession } } = await this.client.auth.getSession(); const realtimeToken = currentSession?.access_token ?? session.access_token; this.client.realtime.setAuth(realtimeToken); @@ -328,18 +261,10 @@ export class RemoteChannel { if (existingDevice) { console.debug('[DEBUG] Updating device status to online'); - // NOTE: transport_broadcast_v1 is deliberately NOT set here. The flag - // is a promise the device may not be able to keep, and the server - // treats it as binding: for a flagged device, absent presence is - // authoritative offline (overlayPresence) and dispatch then throws - // "No devices available". Advertising it before the private channel - // is proven means an 008/authz/Realtime problem takes the device dark - // even though its legacy postgres_changes channel is perfectly - // healthy β€” the exact outcome the independent legacyChannel exists to - // prevent. It is written only after SUBSCRIBED + a successful presence - // track (see markTransportCapable), and cleared when presence - // definitively fails, so a device that cannot deliver on the promise - // simply reports itself legacy and stays dispatchable. + // transport_broadcast_v1 is deliberately NOT set here. The server + // treats the flag as binding β€” for a flagged device absent presence + // means offline β€” so it is only written once presence is proven + // (setTransportCapable), and withdrawn if presence fails. await this.updateDevice(existingDevice.id, { status: 'online', last_seen: new Date().toISOString(), @@ -374,12 +299,9 @@ export class RemoteChannel { } /** - * Publish this device's presence, retrying a non-'ok' result. track() - * RESOLVES with 'ok' | 'error' | 'timed out' rather than rejecting, and a - * silent failure is expensive: the server treats absent presence as - * authoritative offline, so one lost track makes a fully working device - * undispatchable until the channel next bounces. `presenceTracked` lets the - * health check re-try later if every attempt here fails. + * Publish presence, retrying a non-'ok' result β€” track() resolves with a + * status rather than rejecting, and absent presence reads as offline on the + * server. `presenceTracked` lets the health check retry later. */ private async trackPresenceWithRetry(recovered: number, attempts = 3): Promise { if (this.isTrackingPresence) return; // never stack pushes on a wedged socket @@ -434,10 +356,8 @@ export class RemoteChannel { } /** - * The complete `capabilities` JSONB value for this device. Built in ONE - * place because every write REPLACES the whole column β€” a second literal - * elsewhere would silently delete whatever key it forgot on the next - * reconnect. + * The complete `capabilities` JSONB value. One place only: every write + * replaces the whole column, so a second literal would silently drop keys. */ private capabilitiesPayload(broadcastCapable: boolean): Record { return { @@ -447,16 +367,10 @@ export class RemoteChannel { } /** - * Advertise (or withdraw) the broadcast transport capability. The server - * reads this flag to choose a transport AND to decide whether absent - * presence means "offline" β€” so it must only ever be true while this device - * can actually be reached that way. - * - * The flag also selects which tier of the server's offline sweep judges - * this device (45s legacy vs 15min capable), so every change here MUST - * re-arm the heartbeat at the matching cadence β€” otherwise withdrawing the - * flag leaves the device in the 45s tier while it still heartbeats on the - * slow capable cadence and the sweep blacks it out. + * Advertise (or withdraw) the broadcast capability. Only true while the + * device is genuinely reachable that way: the server uses it to pick a + * transport, to read absent presence as offline, and to choose the sweep + * tier β€” so every change must re-arm the heartbeat to match. */ private async setTransportCapable(capable: boolean): Promise { if (!this.client || !this.deviceId) return; @@ -476,13 +390,9 @@ export class RemoteChannel { // Tier changed β€” move last_seen onto the cadence that tier's sweep // threshold expects (no-op if the heartbeat hasn't started yet). this.scheduleHeartbeat(); - // Dropping to the fast tier: the row was last written on the slow - // capable cadence, so it can already be minutes old β€” i.e. ALREADY - // past the 45s legacy threshold the device is now judged by. Write - // once immediately rather than waiting out the new interval, which - // would leave the device swept-offline in the meantime (and, on a - // device flapping tiers faster than the interval, indefinitely, - // since every re-arm restarts the countdown). + // Dropping to the fast tier: last_seen may already be minutes old, + // i.e. past the 45s threshold now judging us. Write once immediately + // instead of waiting out the new interval. if (!capable && this.heartbeatDeviceId) { this.updateHeartbeat(this.heartbeatDeviceId).catch(() => { /* logged inside */ }); } @@ -492,11 +402,8 @@ export class RemoteChannel { } /** - * Subscribe the legacy postgres_changes listener on its own PUBLIC channel. - * Independent of the private user channel on purpose (see legacyChannel). - * Best-effort: failures here are logged, never thrown β€” the doorbell path is - * primary, and realtime-js rejoins this channel on its own. - * Removed entirely at the flip (009), when postgres_changes stops firing. + * Legacy postgres_changes listener on its own public channel. Best-effort: + * failures are logged, never thrown. Removed at the flip (009). */ private createLegacyChannel(): void { if (!this.client || !this.user?.id) return; @@ -590,13 +497,10 @@ export class RemoteChannel { // Update device status on successful connection (queued, so // it can't be overtaken by a teardown's status write). this.queueStatusWrite('online'); - // Announce presence β€” this IS the live "online" signal the - // server's dispatch check reads. A failed track leaves a - // perfectly healthy device invisible (server treats absent - // presence as authoritative offline β†’ "No devices - // available"), so retry, and only resolve once it lands: - // resolving first would let registerDevice() print - // "Device ready" while the device is still undispatchable. + // Presence IS the live "online" signal dispatch reads, so + // retry, and resolve only once it lands β€” otherwise + // registerDevice() prints "Device ready" while the device + // is still undispatchable. this.trackPresenceWithRetry(recovered) .catch(() => { /* logged inside */ }) .finally(() => resolve()); @@ -628,11 +532,9 @@ export class RemoteChannel { } /** - * Handle a 'new_call' broadcast doorbell. The doorbell carries only ids β€” - * the authoritative row is fetched by primary key and fed through the SAME - * handler as a postgres_changes payload, so device.ts is transport-agnostic. - * During the transition both transports deliver every call; the claim in - * markCallExecuting() guarantees single execution. + * Handle a 'new_call' doorbell. It carries ids only; the row is fetched by + * primary key and fed through the same handler as a postgres_changes + * payload, so device.ts stays transport-agnostic. */ private async onDoorbell(payload: any): Promise { const callId = payload?.call_id; @@ -642,20 +544,15 @@ export class RemoteChannel { return; } - // NOTE: deliberately NOT a telemetry event β€” this fires on every remote - // tool call (~126k/day in prod) and would be permanent per-call volume. - // Transport usage is already segmentable server-side: dispatch stamps - // metadata.transport, which rides mcp_command_executed. Only the - // doorbell FAILURE paths below are worth capturing. + // Not a telemetry event on purpose: ~126k/day in prod. Transport usage + // is already segmentable server-side via metadata.transport. console.debug('[DEBUG] Doorbell received for call:', callId); if (!this.client) return; - // Retry the row fetch on transient failures (observed live: a REST - // blip while the websocket stayed healthy). During the transition the - // legacy postgres_changes delivery covers a lost doorbell, but after - // the flip this fetch is the only way the device learns about the - // call β€” a network hiccup must not cost a 5-minute timeout. + // Retry on transient failures (a REST blip while the socket stays + // healthy). Post-flip this fetch is the only way we learn about a call, + // so a hiccup must not cost a 5-minute timeout. let row: any = null; let lastError: any = null; for (const delayMs of [0, 500, 1500]) { @@ -680,19 +577,13 @@ export class RemoteChannel { return; } if (!row) { - // Row already claimed+deleted, or cleanup raced delivery β€” nothing to do. - // Not retried on purpose: pre-flip the row was inserted before the - // doorbell was sent, so a missing row means it was already claimed - // and deleted. Post-009 this is the ONLY delivery path β€” see the - // 009 preconditions if this event ever becomes non-zero. + // Already claimed and deleted, or cleanup raced delivery. Not + // retried: the row is always inserted before the doorbell is sent. await captureRemote('remote_channel_doorbell_row_missing', { call_id: callId }); return; } - // OPTIMIZATION, not a correctness guard β€” do not rely on it. Saves a - // hop when the legacy path already claimed this call. The actual - // exactly-once guarantees live in device.ts: the in-process seenCallIds - // check (same-process double delivery) and the conditional DB claim - // (cross-process/restart). + // Optimization, not a guard β€” saves a hop when the legacy path already + // claimed this. Exactly-once lives in device.ts (seenCallIds + DB claim). if (row.status !== 'pending') { console.debug('[DEBUG] Doorbell call already claimed via legacy path:', callId); return; @@ -703,11 +594,9 @@ export class RemoteChannel { } /** - * Notify the server that a call's result row is written. Fire-and-forget: - * a skipped/failed send just means the server's 10s recovery poll delivers - * the result instead β€” identical to today's Realtime-hiccup behavior. - * MUST be called only after updateCallResult() has resolved, so the - * server's fetch-by-id finds a terminal row. + * Tell the server a result row is written. Fire-and-forget: a failed send + * just falls back to the server's 10s recovery poll. MUST run only after + * updateCallResult() resolves, so the server's fetch-by-id sees a terminal row. */ async notifyResult(callId: string): Promise { if (!this.channel || this.channel.state !== 'joined') { @@ -773,13 +662,11 @@ export class RemoteChannel { return; } - // 'joining' = transitional β€” normally let realtime-js's own rejoin backoff converge - // instead of tearing the channel down mid-join (recreating on every non-joined state - // amputates that backoff). BUT bound it: on a half-open socket realtime-js can park - // the channel in 'joining' indefinitely without ever reconnecting the socket, so the - // recreate below would never fire and the device wedges offline silently. If 'joining' - // overstays JOINING_WEDGE_TIMEOUT_MS unbroken, force a recreate β€” the only path that - // disconnect()s the dead socket. (connState() in the log shows the half-open socket.) + // 'joining' is transitional β€” let realtime-js's rejoin backoff converge + // rather than tearing the channel down mid-join. But bound it: a + // half-open socket parks the channel here indefinitely, so past + // JOINING_WEDGE_TIMEOUT_MS force a recreate, the only path that + // disconnect()s the dead socket. if (state === 'joining') { const now = Date.now(); if (this.joiningSince === null) this.joiningSince = now; @@ -845,11 +732,8 @@ export class RemoteChannel { console.log(`πŸ”„ Recreating channel... (attempt ${this.reconnectAttempt}) β€” ${this.connState()}`); try { - // Jittered exponential backoff so a fleet-wide event (server deploy, - // Supabase blip) doesn't stampede every device into reconnecting at - // the same instant. attempt 1 β‰ˆ 1-3s, capped at ~15-45s. The - // re-entrancy guard above keeps the 10s watchdog from stacking - // recreates while we sleep. + // Jittered backoff so a fleet-wide event doesn't stampede every + // device into reconnecting at once. ~1-3s rising to ~15-45s. const backoffMs = Math.min(30_000, 1000 * 2 ** Math.min(this.reconnectAttempt, 5)) * (0.5 + Math.random()); console.debug(`[DEBUG] Reconnect backoff: ${Math.round(backoffMs)}ms`); await this.sleep(backoffMs); @@ -867,9 +751,9 @@ export class RemoteChannel { // ever emits CLOSED) must not pin isRecreatingChannel=true and silently disable // the 10s watchdog. On timeout we reject -> catch -> finally clears the guard. await this.withTimeout(async () => { - // Destroy old channel β€” AWAIT it so the channel registry empties before we - // rebuild. (The un-awaited version raced the synchronous new-channel push, so - // realtime-js never tore the socket down and a half-open one got reused.) + // Await it so the channel registry empties before we rebuild β€” + // otherwise realtime-js never tears the socket down and a + // half-open one gets reused. if (this.channel) { console.debug('[DEBUG] Destroying old channel'); await this.client!.removeChannel(this.channel); @@ -896,21 +780,13 @@ export class RemoteChannel { } catch (err: any) { captureRemote('remote_channel_recreate_error', { errMsg: err?.message, attempt: this.reconnectAttempt }); console.debug(`[DEBUG] Channel recreation failed: ${err?.message} β€” ${this.connState()}`); - // Sustained private-channel failure: stop promising a transport we - // cannot deliver. Until this withdrawal the flag could only ever be - // cleared from trackPresenceInner, which is unreachable unless the - // channel is already 'joined' β€” so a device that could never join - // (008 dropped, RLS/JWT failure after a long sleep) kept advertising - // itself, and the server's presence overlay then reported it OFFLINE - // authoritatively, overriding a perfectly good `status` and blacking - // it out until the process restarted. + // Sustained failure: stop promising a transport we can't deliver, or + // the server's presence overlay reports this device offline + // authoritatively and overrides `status`. if (this.reconnectAttempt >= TRANSPORT_WITHDRAW_AFTER_ATTEMPTS) { - // BOUNDED, and in its own try: this runs in the catch block, - // which RECREATE_TIMEOUT_MS does NOT cover (it wraps only the - // inner withTimeout above). An unbounded await here would pin - // isRecreatingChannel=true on a hanging PATCH and silently - // disable the 10s connection watchdog β€” precisely the failure - // mode withTimeout() was introduced for. + // Bounded, in its own try: this catch block is outside + // RECREATE_TIMEOUT_MS, so a hanging PATCH would pin + // isRecreatingChannel and disable the watchdog. try { await this.withTimeout( () => this.setTransportCapable(false), @@ -918,8 +794,8 @@ export class RemoteChannel { 'withdrawTransportCapability' ); } catch (withdrawErr: any) { - // Next failed recreate retries; transportCapableWritten is - // only advanced on a confirmed write, so nothing is lost. + // The next failed recreate retries; the flag only advances + // on a confirmed write, so nothing is lost. console.debug(`[DEBUG] Capability withdrawal did not complete: ${withdrawErr?.message}`); } } @@ -929,15 +805,12 @@ export class RemoteChannel { } /** - * Claim a call for execution. Returns true only when THIS update flipped - * the row from 'pending' to 'executing' β€” during the transition every call - * is delivered twice (postgres_changes + broadcast doorbell), and this - * claim is what guarantees it executes once. The .eq('status','pending') - * makes the claim conditional; .select('id') makes it observable (a - * supabase-js UPDATE returns no row data without it). - * On a transient DB ERROR we return true (execute anyway) β€” matching the - * old behavior, where a failed status write never blocked execution; the - * duplicate-execution window that leaves is no worse than today's. + * Claim a call. True only when THIS update flipped the row pending -> + * executing, which is what makes dual delivery safe across processes. + * .eq('status','pending') makes it conditional; .select('id') makes the + * result observable. On a transient DB error it returns true (execute + * anyway), matching prior behaviour β€” so device.ts's in-memory guard is what + * actually guarantees exactly-once within a process. */ async markCallExecuting(callId: string): Promise { if (!this.client) throw new Error('Client not initialized'); @@ -1016,56 +889,34 @@ export class RemoteChannel { } /** - * True while this device can still be reached by SOME transport: the - * private user channel (broadcast doorbells) or, during the transition, its - * independent legacy postgres_changes channel. - * - * The heartbeat gate deliberately asks "reachable?", not "is the private - * channel up?". Gating on the private channel alone means a device whose - * ONLY working transport is the legacy channel never writes last_seen, so - * the server's 45s legacy sweep marks it offline and dispatch refuses it β€” - * a total blackout for a device that can actually run tools. - * - * Removed with the rest of the legacy path at the flip (009), after which - * the private channel is the only transport and this collapses back to a - * single check. + * Reachable by SOME transport β€” the private channel or, during the + * transition, the independent legacy one. Gates the heartbeat and `status`: + * asking only about the private channel starves last_seen for a device whose + * legacy channel is fine, and the 45s sweep then blacks it out. + * Collapses to a single check at the flip (009). */ private isReachable(): boolean { return this.channel?.state === 'joined' || this.legacyChannel?.state === 'joined'; } /** - * Reconcile the durable `status` column with ACTUAL reachability. - * - * `status` is transport-agnostic β€” it is the column the server's - * resolveTargetDevice() filters on β€” so it must never be driven by the - * health of ONE transport. Writing 'offline' from the private channel's - * error path (which is what this replaces) blacked out devices whose - * legacy channel was joined and delivering: realtime-js re-fires - * CHANNEL_ERROR on every failed rejoin, so during a private-channel - * outage each retry re-wrote 'offline' while the heartbeat re-wrote - * 'online', leaving the row oscillating and roughly half of all dispatches - * failing with "No devices available" for a perfectly healthy machine. - * - * Same predicate as the heartbeat gate, so the two can never disagree. + * Set `status` from actual reachability. `status` is transport-agnostic (the + * server filters on it), so it must not follow one channel's health β€” the + * private channel's error path re-fires on every rejoin and would oscillate + * the row against the heartbeat. Same predicate as the heartbeat gate. */ private syncReachabilityStatus(): void { this.queueStatusWrite(this.isReachable() ? 'online' : 'offline'); } /** - * Serialize the CHANNEL-CALLBACK status writes β€” not every writer. These - * fire from un-awaited callbacks (SUBSCRIBED, CHANNEL_ERROR, CLOSED), and - * two concurrent PATCHes to the same row land in arbitrary order: the real - * window is inside recreateChannel(), where removeChannel()'s CLOSED writes - * 'offline' and the fresh join's SUBSCRIBED writes 'online' ~100-300ms later - * β€” comparable to a PostgREST round trip. Unordered, the 'offline' can win - * and leave a healthy device undispatchable until the next heartbeat tick. + * Serialize the channel-callback status writes. They fire from un-awaited + * callbacks, and inside recreateChannel() a teardown's 'offline' and the + * fresh join's 'online' land ~100-300ms apart β€” unordered, 'offline' can win + * and leave a healthy device undispatchable until the next heartbeat. * - * Deliberately NOT the single writer: updateHeartbeat, registerDevice and - * setOffline's subprocess all write status directly. That is fine β€” the - * heartbeat re-asserting 'online' every tier interval is the intended - * self-correction β€” but do not assume this chain gives total ordering. + * Not the single writer: updateHeartbeat, registerDevice and setOffline's + * subprocess write status directly, so this is not total ordering. */ private queueStatusWrite(status: 'online' | 'offline'): void { // After teardown begins, setOffline() owns the final status write. @@ -1210,20 +1061,13 @@ export class RemoteChannel { console.debug('[DEBUG] setOffline() initiating blocking update for device:', deviceId); try { - // Get a session for the subprocess β€” BOUNDED, with a fallback. - // - // auth.getSession() is not a cheap storage read: it takes a lock with - // a 10s acquire timeout, and it refreshes when the token is merely - // WITHIN ~90s of expiry, which POSTs /token with its own retry - // budget (~30s on retryable network errors). On a just-woken machine - // β€” token near expiry, wifi not re-associated β€” that is exactly the - // shape that blows device.ts's 5s force-exit, and then spawnSync - // never runs and the durable offline write never lands. That row - // then reads 'online' for the whole capable sweep tier, with every - // dispatch to it costing the caller a 5-minute timeout. - // - // The subprocess calls setSession() itself, so a slightly stale - // access_token is fine as long as the refresh_token is good. + // Session for the subprocess β€” bounded, with a cached fallback. + // getSession() is not a cheap read: it takes a lock (10s acquire + // timeout) and refreshes when the token is within ~90s of expiry, + // POSTing /token with its own ~30s retry budget. On a just-woken + // machine that outlasts device.ts's 5s force-exit, and then spawnSync + // never runs and the offline write is lost. The subprocess calls + // setSession() itself, so a slightly stale access_token is fine. const live = await Promise.race([ this.client.auth.getSession().then((r) => r.data?.session ?? null), this.sleep(OFFLINE_SESSION_TIMEOUT_MS).then(() => null), @@ -1307,68 +1151,34 @@ export class RemoteChannel { } async unsubscribe() { - // Teardown has begun: from here on, setOffline()'s durable write is the - // authoritative final word on `status`, so stop every other writer - // (channel callbacks AND the heartbeat) from racing it. Otherwise a - // late 'online' is applied after the subprocess has written 'offline', - // leaving an exited process marked online until the sweep ages it out β€” - // and every dispatch in between costs the caller a 5-minute timeout. - // - // The window that actually needs this is NOT the CLOSED fired by the - // unsubscribe below: realtime-js sets state='leaving' on the first line - // of unsubscribe() and removeChannel() calls it synchronously, so by the - // time that CLOSED lands isReachable() already reads false and the write - // would have been 'offline' anyway. The real races are: - // 1. a heartbeat tick (every 15s in the legacy tier) firing or already - // in flight as the signal arrives β€” see updateHeartbeat, and - // 2. SIGINT arriving while recreateChannel() sits in its jittered - // backoff (up to ~45s): this.channel is already null so unsubscribe - // skips its block, setOffline writes 'offline', then the backoff - // expires during desktop.shutdown() and the fresh join's SUBSCRIBED - // queues 'online' after the durable write. + // setOffline()'s durable write is the final word on `status` from here, + // so stop the heartbeat and the channel callbacks from racing it. The + // races that matter: a heartbeat tick firing as the signal arrives, and + // SIGINT during recreateChannel()'s backoff, where the later join's + // SUBSCRIBED would queue 'online' after the durable write. this.shuttingDown = true; - // BUDGET: device.ts force-exits 5s after the signal, and the durable - // offline write (setOffline's spawnSync, 3s cap + a bounded session - // fetch, 0.5s) is the one thing this whole path exists to produce. So - // everything before it must be tightly bounded: - // 250ms drain + 500ms untrack + 500ms(Γ—2, see below) + 500ms session - // + 3000ms spawnSync β‰ˆ 4.75s worst case - // Only the untrack bound can actually bind: removeChannel/unsubscribe - // both set state='leaving' first, which makes realtime-js's _canPush() - // false so the leave push resolves 'ok' inline rather than waiting out - // its 10s timeout. The other two bounds are cheap insurance, not load- - // bearing β€” do not "reclaim" the budget by removing the untrack one. + // Budget: device.ts force-exits 5s after the signal and setOffline needs + // ~3.5s of it, so everything here must fit in ~1.5s. + // Only the untrack bound can actually bind β€” removeChannel/unsubscribe + // set state='leaving' first, so their leave push resolves inline. const LEAVE_BOUND_MS = 500; - // Drain the QUEUED channel-callback writes. This cannot drain a - // heartbeat PATCH β€” updateHeartbeat writes directly, not through the - // chain β€” but the gate above stops any NEW heartbeat, and one already in - // flight necessarily started before this point. + // Drain queued channel-callback writes. Can't drain an in-flight + // heartbeat PATCH (it doesn't use the chain), but the gate above stops + // any new one and an in-flight one started earlier. await Promise.race([this.statusWriteChain, this.sleep(250)]); - // Bounded like the untrack below: removeChannel() sends a leave push that - // only settles via realtime-js's 10s timeout on a half-open socket, which - // would blow device.ts's 5s force-exit and skip the durable offline write. await Promise.race([this.removeLegacyChannel(), this.sleep(LEAVE_BOUND_MS)]); if (this.channel) { - // Leave presence explicitly on the graceful path (socket close - // covers the abrupt one). + // Leave presence on the graceful path (socket close covers the abrupt + // one). Bounded: a half-open socket still reports 'joined', so the + // push just buffers and would settle via realtime-js's 10s timeout. try { - // Bound the untrack: a HALF-OPEN socket still reports the channel - // as 'joined', so a state check alone is not enough β€” the presence - // push just buffers and settles via realtime-js's 10s timeout, - // which blows past device.ts's 5s force-exit and would skip both - // unsubscribe() and the durable offline write. A dropped socket - // clears server-side presence anyway. await Promise.race([ this.channel.untrack(), this.sleep(LEAVE_BOUND_MS), ]); console.debug('[DEBUG] Presence untrack attempted (bounded)'); } catch { /* best effort */ } - // Bounded as insurance only. unsubscribe() sets state='leaving' on - // its first line, so _canPush() is false and the leave push resolves - // 'ok' inline β€” it cannot actually wait out the 10s push timeout the - // way the untrack above can. Kept because it costs nothing and the - // guarantee lives in library internals, not in our contract. + // Bounded as insurance; unsubscribe() resolves inline in practice. await Promise.race([this.channel.unsubscribe(), this.sleep(LEAVE_BOUND_MS)]); this.channel = null; console.log('βœ“ Unsubscribed from tool call channel'); diff --git a/test/bench-strip-null-bytes.mjs b/test/bench-strip-null-bytes.mjs deleted file mode 100644 index 5668dd9f..00000000 --- a/test/bench-strip-null-bytes.mjs +++ /dev/null @@ -1,42 +0,0 @@ -// Benchmark stripNullBytes against realistic tool-result shapes. -// Not part of the test suite (name doesn't match test*.js) β€” run manually: -// npm run build && node test/bench-strip-null-bytes.mjs -import { stripNullBytes } from '../dist/remote-device/remote-channel.js'; - -const NUL = String.fromCharCode(0); - -function bench(name, value, iterations) { - // warm up JIT - for (let i = 0; i < 5; i++) stripNullBytes(value); - const t0 = process.hrtime.bigint(); - for (let i = 0; i < iterations; i++) stripNullBytes(value); - const t1 = process.hrtime.bigint(); - const perCallMs = Number(t1 - t0) / 1e6 / iterations; - console.log(`${name.padEnd(46)} ${perCallMs.toFixed(4)} ms/call`); -} - -const small = { content: [{ type: 'text', text: 'Process started with PID 1234' }] }; -const typical = { content: [{ type: 'text', text: 'x'.repeat(10 * 1024) }] }; // 10 KB -const big = { content: [{ type: 'text', text: 'x'.repeat(1024 * 1024) }] }; // 1 MB -const huge = { content: [{ type: 'text', text: 'x'.repeat(13 * 1024 * 1024) }] }; // 13 MB (max seen in prod) -const withNul = { content: [{ type: 'text', text: ('x'.repeat(1024) + NUL).repeat(1024) }] }; // 1 MB w/ NULs -const manyKeys = Object.fromEntries( - Array.from({ length: 500 }, (_, i) => [`key_${i}`, `value_${i}`]) -); - -console.log('--- no NUL present (the overwhelmingly common case) ---'); -bench('small result (~30 B)', small, 20000); -bench('typical result (10 KB)', typical, 5000); -bench('big result (1 MB)', big, 200); -bench('huge result (13 MB, prod max)', huge, 20); -bench('object with 500 keys', manyKeys, 5000); - -console.log('\n--- NUL present (rare) ---'); -bench('1 MB with 1024 NULs', withNul, 200); - -// Reference point: what the same payload costs to JSON-serialize, which the -// supabase client does on every write regardless. -const t0 = process.hrtime.bigint(); -for (let i = 0; i < 20; i++) JSON.stringify(huge); -const t1 = process.hrtime.bigint(); -console.log(`\nreference: JSON.stringify(13 MB) ${(Number(t1 - t0) / 1e6 / 20).toFixed(4)} ms/call`); diff --git a/test/test-remote-dedupe-and-doorbell.js b/test/test-remote-dedupe-and-doorbell.js deleted file mode 100644 index da91c70a..00000000 --- a/test/test-remote-dedupe-and-doorbell.js +++ /dev/null @@ -1,181 +0,0 @@ -#!/usr/bin/env node - -/** - * Covers the branch's headline mechanism, which previously had no tests: - * - * 1. EXACTLY-ONCE under dual delivery. During the transition every call is - * delivered twice (legacy postgres_changes + broadcast doorbell). The DB - * claim deliberately fails OPEN on a transient write error, so the local - * seen-call-id guard in handleNewToolCall is what actually guarantees a - * side-effecting tool runs once. - * 2. onDoorbell routing: ignore other devices, skip already-claimed rows, - * surface a missing row, and retry a transient fetch failure. - * 3. updateCallResult BEFORE notifyResult β€” the server fetches the row by id - * when the doorbell arrives, so the write must land first or it sees a - * non-terminal row and waits for the 10s recovery poll. - * - * Run: npm run build && node test/test-remote-dedupe-and-doorbell.js - */ - -import { MCPDevice } from '../dist/remote-device/device.js'; -import { RemoteChannel } from '../dist/remote-device/remote-channel.js'; - -let failures = 0; -async function test(name, fn) { - try { - await fn(); - console.log(`βœ… PASS ${name}`); - } catch (error) { - failures++; - console.error(`πŸ”΄ FAIL ${name}\n ${error.message}`); - } -} -function assert(condition, message) { - if (!condition) throw new Error(message); -} - -const DEVICE_ID = 'device-1'; -const OTHER_DEVICE = 'device-2'; - -/** MCPDevice with the network/desktop edges stubbed out. */ -function makeDevice({ claimResults = [] } = {}) { - const device = new MCPDevice(); - const executed = []; - const claims = [...claimResults]; - - device.deviceId = DEVICE_ID; - device.desktop = { - callClientTool: async (toolName, args) => { - executed.push({ toolName, args }); - return { content: [{ type: 'text', text: 'ok' }] }; - }, - }; - device.remoteChannel = { - // Default: first delivery claims, later ones lose. Overridable to model a - // transient DB error, which makes the claim fail OPEN (returns true). - markCallExecuting: async () => (claims.length ? claims.shift() : true), - updateCallResult: async () => {}, - notifyResult: async () => {}, - }; - return { device, executed }; -} - -const payloadFor = (id, deviceId = DEVICE_ID) => ({ - new: { id, tool_name: 'start_process', tool_args: { command: 'echo hi' }, device_id: deviceId, metadata: {} }, -}); - -await test('dual delivery of the same call executes the tool exactly once', async () => { - const { device, executed } = makeDevice(); - await device.handleNewToolCall(payloadFor('call-a')); - await device.handleNewToolCall(payloadFor('call-a')); // the other transport - assert(executed.length === 1, `expected 1 execution, got ${executed.length}`); -}); - -await test('exactly-once holds even when the DB claim fails OPEN for both deliveries', async () => { - // Both claims return true (what a transient REST error produces) β€” only the - // in-memory guard prevents a second run of a side-effecting command. - const { device, executed } = makeDevice({ claimResults: [true, true] }); - await device.handleNewToolCall(payloadFor('call-b')); - await device.handleNewToolCall(payloadFor('call-b')); - assert(executed.length === 1, `fail-open claim double-executed: ${executed.length} runs`); -}); - -await test('a lost DB claim (another process won) skips execution', async () => { - const { device, executed } = makeDevice({ claimResults: [false] }); - await device.handleNewToolCall(payloadFor('call-c')); - assert(executed.length === 0, `expected no execution, got ${executed.length}`); -}); - -await test('calls for a different device are ignored and do not poison the dedupe set', async () => { - const { device, executed } = makeDevice(); - await device.handleNewToolCall(payloadFor('call-d', OTHER_DEVICE)); - assert(executed.length === 0, 'must not execute another device call'); - // Same id later arriving FOR US must still run β€” the filter precedes dedupe. - await device.handleNewToolCall(payloadFor('call-d')); - assert(executed.length === 1, 'a mismatched delivery must not suppress our own'); -}); - -await test('the seen-call-id set stays bounded', async () => { - const { device } = makeDevice(); - for (let i = 0; i < 250; i++) await device.handleNewToolCall(payloadFor(`bulk-${i}`)); - assert(device.seenCallIds.size <= 100, `set grew to ${device.seenCallIds.size}`); -}); - -// --- onDoorbell ------------------------------------------------------------- - -/** RemoteChannel with just enough of a Supabase client for onDoorbell. */ -function makeChannel({ rows = {}, failFetches = 0 } = {}) { - const rc = new RemoteChannel(); - const delivered = []; - let fetchAttempts = 0; - rc.deviceId = DEVICE_ID; - rc.onToolCall = (payload) => delivered.push(payload); - rc.sleep = () => Promise.resolve(); // no real backoff in tests - rc.client = { - from: () => ({ - select: () => ({ - eq: () => ({ - maybeSingle: async () => { - fetchAttempts++; - if (fetchAttempts <= failFetches) return { data: null, error: { message: 'fetch failed' } }; - return { data: rows.row ?? null, error: null }; - }, - }), - }), - }), - }; - return { rc, delivered, attempts: () => fetchAttempts }; -} - -await test('doorbell for another device is ignored without fetching', async () => { - const { rc, delivered, attempts } = makeChannel(); - await rc.onDoorbell({ call_id: 'x', device_id: OTHER_DEVICE }); - assert(delivered.length === 0, 'must not deliver'); - assert(attempts() === 0, 'must not even fetch the row'); -}); - -await test('doorbell delivers a pending row through the shared handler', async () => { - const row = { id: 'x', status: 'pending', tool_name: 'start_process' }; - const { rc, delivered } = makeChannel({ rows: { row } }); - await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); - assert(delivered.length === 1, 'expected one delivery'); - assert(delivered[0].new === row, 'must pass the fetched row as {new: row}'); -}); - -await test('doorbell for an already-claimed row does not re-deliver', async () => { - const { rc, delivered } = makeChannel({ rows: { row: { id: 'x', status: 'executing' } } }); - await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); - assert(delivered.length === 0, 'non-pending rows must not be re-delivered'); -}); - -await test('doorbell row fetch retries a transient failure', async () => { - const row = { id: 'x', status: 'pending' }; - const { rc, delivered, attempts } = makeChannel({ rows: { row }, failFetches: 2 }); - await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); - assert(attempts() === 3, `expected 3 attempts, got ${attempts()}`); - assert(delivered.length === 1, 'should deliver after the retry succeeds'); -}); - -await test('doorbell with a missing row is a no-op (already claimed and deleted)', async () => { - const { rc, delivered } = makeChannel({ rows: { row: null } }); - await rc.onDoorbell({ call_id: 'gone', device_id: DEVICE_ID }); - assert(delivered.length === 0, 'missing row must not deliver'); -}); - -// --- result ordering -------------------------------------------------------- - -await test('the result row is written BEFORE the doorbell is rung', async () => { - const order = []; - const { device, executed } = makeDevice(); - device.remoteChannel.updateCallResult = async () => { order.push('write'); }; - device.remoteChannel.notifyResult = async () => { order.push('doorbell'); }; - await device.handleNewToolCall(payloadFor('call-order')); - assert(executed.length === 1, 'tool should have run'); - assert( - order.join(',') === 'write,doorbell', - `server fetches by id on the doorbell, so the write must land first: got ${order.join(',')}` - ); -}); - -console.log(`\n${failures ? 'πŸ”΄' : 'βœ…'} dedupe + doorbell: ${failures} failing test(s).`); -process.exit(failures ? 1 : 0); diff --git a/test/test-remote-heartbeat-tier.js b/test/test-remote-heartbeat-tier.js deleted file mode 100644 index 648309d7..00000000 --- a/test/test-remote-heartbeat-tier.js +++ /dev/null @@ -1,469 +0,0 @@ -/** - * Regression test: the device's last_seen heartbeat must stay in step with the - * SERVER-SIDE offline-sweep tier that judges it. - * - * The server tiers its sweep on the CAPABILITY FLAG, not the app version: - * - no transport_broadcast_v1 -> legacy tier, DEVICE_OFFLINE_TIMEOUT_MS = 45s - * - transport_broadcast_v1 -> capable tier, DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS = 15min - * - * A new-build device only sets the flag AFTER a successful presence track(), so - * between registration and that moment β€” and permanently, if presence can never - * be proven (migration 008 not applied, an authz hiccup, exhausted track() - * retries) β€” it is judged by the 45s rule. Heartbeating on the slow capable - * cadence in that state means the sweep marks it offline ~45s after it registers - * and dispatch then throws "No devices available" forever, even though its - * INDEPENDENT legacy postgres_changes channel is joined and could run every call. - * - * Two invariants, both regressions found in review 2026-07-26: - * 1. cadence follows the tier (fast while legacy, slow only once capable), and - * re-arms the moment the tier changes. - * 2. the heartbeat write is gated on being reachable by ANY transport, not on - * the private channel specifically β€” gating on the private channel starves - * last_seen for exactly the devices the legacy channel is meant to rescue. - * - * Standalone: node test/test-remote-heartbeat-tier.js (needs npm run build). - */ -import assert from 'node:assert'; -import { RemoteChannel } from '../dist/remote-device/remote-channel.js'; - -process.env.DESKTOP_COMMANDER_DISABLE_TELEMETRY = '1'; - -// The server-side thresholds this device must stay inside. HAND-COPIED from -// remote-dc-mcp/src/server/constants.ts β€” the two repos ship independently, so -// NOTHING enforces this copy: lowering DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS on the -// server will NOT fail this test. When you change either threshold, change it -// here too. (PUBLISH.md's release gate repeats this pairing for the same reason.) -// What this test does guarantee is the DEVICE half: that its cadence, whatever -// tier it is in, fits inside the threshold recorded below. -const SERVER_LEGACY_OFFLINE_TIMEOUT_MS = 45 * 1000; -const SERVER_CAPABLE_OFFLINE_TIMEOUT_MS = 15 * 60 * 1000; - -function makeChannel(state) { - return { state }; -} - -/** - * Minimal client that records mcp_devices writes. - * - * `removeChannel` / `realtime.disconnect` are REQUIRED, not decorative: - * recreateChannel() calls both, and without them it dies on a TypeError before - * reaching anything the recreate tests stub β€” which made those tests pass for - * the wrong reason (caught in review round 8). - */ -function makeFakeClient() { - const writes = []; - return { - writes, - removeChannel: () => Promise.resolve('ok'), - realtime: { disconnect: () => Promise.resolve() }, - from() { - const chain = { - update: (payload) => { - writes.push(payload); - return chain; - }, - select: () => chain, - eq: () => Promise.resolve({ error: null }), - }; - return chain; - }, - }; -} - -function makeRemoteChannel() { - const rc = new RemoteChannel(); - const client = makeFakeClient(); - rc.client = client; // private at TS level, plain property at runtime - rc._user = { id: 'user-1', email: 'tester@example.com' }; - rc.deviceId = 'device-1'; - rc.deviceName = 'test-device'; - rc.onToolCall = () => {}; - return { rc, client }; -} - -let failures = 0; -async function test(name, fn) { - try { - await fn(); - console.log(`βœ… PASS ${name}`); - } catch (e) { - failures++; - console.error(`πŸ”΄ FAIL ${name}\n ${e.message}`); - } -} - -async function main() { - // 1. A device that has NOT proven the transport must heartbeat fast enough to - // survive the server's 45s legacy sweep. - await test('legacy tier heartbeats inside the server 45s sweep threshold', async () => { - const { rc } = makeRemoteChannel(); - rc.transportCapableWritten = null; // never written = legacy tier - const cadence = rc.heartbeatIntervalMs(); - assert.ok( - cadence * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, - `legacy cadence ${cadence}ms must allow >=2 writes inside the ` + - `${SERVER_LEGACY_OFFLINE_TIMEOUT_MS}ms sweep window` - ); - - rc.transportCapableWritten = false; // capability explicitly withdrawn - assert.strictEqual( - rc.heartbeatIntervalMs(), - cadence, - 'a withdrawn capability must use the same fast legacy cadence' - ); - }); - - // 2. Once the transport is proven, presence carries liveness and the durable - // write drops to bookkeeping cadence β€” but must still beat the 65min tier. - await test('capable tier heartbeats inside the server capable sweep threshold', async () => { - const { rc } = makeRemoteChannel(); - rc.transportCapableWritten = true; - const cadence = rc.heartbeatIntervalMs(); - assert.ok( - cadence * 2 < SERVER_CAPABLE_OFFLINE_TIMEOUT_MS, - `capable cadence ${cadence}ms must allow >=2 writes inside the ` + - `${SERVER_CAPABLE_OFFLINE_TIMEOUT_MS}ms sweep window` - ); - assert.ok( - cadence > SERVER_LEGACY_OFFLINE_TIMEOUT_MS, - 'capable cadence should be the slow bookkeeping one, not the legacy rate' - ); - }); - - // 3. Withdrawing the capability must re-arm the timer immediately. Before the - // fix the cadence was a fixed setInterval chosen once at startup, so a - // device dropping to the legacy tier kept writing every 30 min and got - // swept offline 45s later. - await test('withdrawing the capability re-arms the heartbeat at the fast cadence', async () => { - const { rc } = makeRemoteChannel(); - rc.transportCapableWritten = true; - rc.channel = makeChannel('joined'); - rc.startHeartbeat('device-1'); - try { - const armed = []; - const realSetTimeout = globalThis.setTimeout; - globalThis.setTimeout = (fn, ms) => { - armed.push(ms); - return realSetTimeout(() => {}, 0); // never actually fire - }; - try { - await rc.setTransportCapable(false); - } finally { - globalThis.setTimeout = realSetTimeout; - } - - assert.ok(armed.length > 0, 'withdrawing the capability must re-arm the heartbeat timer'); - assert.ok( - armed[armed.length - 1] * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, - `re-armed cadence ${armed[armed.length - 1]}ms must fit the 45s legacy sweep` - ); - } finally { - rc.stopHeartbeat(); - } - }); - - // 4. THE BLACKOUT CASE. Private channel dead (008 missing / authz failure), - // legacy channel joined and delivering. The device is genuinely usable, so - // it MUST keep last_seen fresh or the sweep makes it undispatchable. - await test('heartbeat writes when only the legacy channel is joined', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = null; // private channel never joined - rc.legacyChannel = makeChannel('joined'); // the safety net is up - await rc.updateHeartbeat('device-1'); - assert.strictEqual( - client.writes.length, - 1, - 'a device reachable only via the legacy channel must still write last_seen' - ); - assert.ok(client.writes[0].last_seen, 'write should bump last_seen'); - assert.strictEqual(client.writes[0].status, 'online', 'write should assert online'); - }); - - // 5. ...but a genuinely deaf device must still go silent, so the server's - // staleness sweep can age its row out and correct a stale 'online'. - await test('heartbeat stays silent when no transport is joined', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('errored'); - rc.legacyChannel = makeChannel('closed'); - await rc.updateHeartbeat('device-1'); - assert.strictEqual( - client.writes.length, - 0, - 'a deaf device must NOT refresh last_seen β€” the sweep has to be able to age it out' - ); - }); - - // 6. Private channel healthy is of course still reachable. - await test('heartbeat writes when the private channel is joined', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('joined'); - rc.legacyChannel = null; - await rc.updateHeartbeat('device-1'); - assert.strictEqual(client.writes.length, 1, 'private channel joined = reachable'); - }); - - // 7. THE OSCILLATION CASE. `status` is transport-agnostic β€” the server's - // resolveTargetDevice filters on it β€” so a private-channel failure must - // NOT write 'offline' while the legacy channel is still delivering. - // realtime-js re-fires CHANNEL_ERROR on every failed rejoin, so the old - // unconditional write left the row flipping offline/online against the - // heartbeat and failed ~half of all dispatches. - await test('private-channel failure keeps status online while legacy is joined', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('errored'); // private join keeps failing - rc.legacyChannel = makeChannel('joined'); // safety net delivering fine - - rc.syncReachabilityStatus(); - await rc.statusWriteChain; - - assert.strictEqual(client.writes.length, 1, 'one status write'); - assert.strictEqual( - client.writes[0].status, - 'online', - 'a device still reachable via the legacy channel must stay online' - ); - }); - - // 8. ...but when NO transport is up the device must go offline. - await test('status goes offline when no transport is joined', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('errored'); - rc.legacyChannel = makeChannel('closed'); - - rc.syncReachabilityStatus(); - await rc.statusWriteChain; - - assert.strictEqual(client.writes[0].status, 'offline', 'genuinely deaf device goes offline'); - }); - - // 9. Status writes must land in the order they were issued: a teardown's - // 'offline' must not overtake the subsequent join's 'online'. - await test('concurrent status writes stay ordered', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('joined'); - - rc.queueStatusWrite('offline'); // teardown - rc.queueStatusWrite('online'); // immediate re-join - await rc.statusWriteChain; - - assert.strictEqual(client.writes.length, 2, 'both writes issued'); - assert.deepStrictEqual( - client.writes.map((w) => w.status), - ['offline', 'online'], - 'writes must apply in issue order so the join wins' - ); - }); - - // 10. Once teardown starts, setOffline()'s durable write owns `status`. A - // reachability write queued after that point would be flushed only after - // setOffline's blocking spawnSync had already written 'offline', leaving a - // shut-down device marked online until the sweep aged it out. - await test('status writes are suppressed once teardown has started', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('joined'); - rc.legacyChannel = makeChannel('joined'); - - rc.shuttingDown = true; - rc.syncReachabilityStatus(); - rc.queueStatusWrite('online'); - await rc.statusWriteChain; - - assert.strictEqual( - client.writes.length, - 0, - 'no status write may be issued after teardown starts' - ); - }); - - // 11. unsubscribe() must be bounded: on a half-open socket the leave push only - // settles via realtime-js's 10s timeout, which blows device.ts's 5s - // force-exit and skips setOffline() entirely. - await test('unsubscribe is bounded and still clears the channel', async () => { - const { rc } = makeRemoteChannel(); - let cleared = false; - rc.legacyChannel = null; - rc.channel = { - state: 'joined', - untrack: () => new Promise(() => {}), // never settles - unsubscribe: () => new Promise(() => {}), // never settles (half-open) - }; - // Keep the bound short so the test doesn't wait on real timers. - rc.sleep = () => Promise.resolve(); - - await rc.unsubscribe(); - cleared = rc.channel === null; - assert.ok(cleared, 'unsubscribe must give up on a wedged leave push and move on'); - assert.strictEqual(rc.shuttingDown, true, 'teardown flag set'); - }); - - // 12. THE OVERLAY BLACKOUT. For a FLAGGED device the server treats absent - // Presence as authoritative offline and applies that overlay before - // selection, so it outranks `status` entirely. A device that keeps the flag - // while unable to join the private channel is therefore undispatchable no - // matter how healthy its legacy channel is. Until this fix the flag could - // only be cleared from trackPresenceInner, which is unreachable unless the - // channel is already 'joined' β€” so such a device never recovered without a - // process restart. - await test('sustained recreate failure withdraws the transport capability', async () => { - const { rc, client } = makeRemoteChannel(); - rc.transportCapableWritten = true; // previously proven and advertised - rc.legacyChannel = makeChannel('joined'); - rc.sleep = () => Promise.resolve(); // skip the jittered backoff - // A channel that can never be rebuilt (008 dropped / RLS denies the join). - const order = []; - rc.createChannel = () => { - order.push('private'); - return Promise.reject(new Error('Unauthorized')); - }; - rc.createLegacyChannel = () => { - order.push('legacy'); - }; - rc.channel = makeChannel('errored'); - - for (let i = 0; i < 3; i++) { - await rc.recreateChannel(); - } - - // The recreate must genuinely have reached createChannel β€” not died earlier - // on a missing client method β€” and must rebuild the legacy safety net FIRST - // and unconditionally, so a private-channel outage never leaves the fallback - // dead (the round-5 invariant, previously uncovered). - assert.deepStrictEqual( - order.slice(0, 2), - ['legacy', 'private'], - `legacy net must be rebuilt before the private channel: ${JSON.stringify(order)}` - ); - assert.strictEqual( - order.filter((o) => o === 'legacy').length, - 3, - 'the legacy net must be rebuilt on EVERY recreate attempt' - ); - - assert.strictEqual( - rc.transportCapableWritten, - false, - 'the capability must be withdrawn after sustained private-channel failure' - ); - const capWrite = client.writes.find((w) => w.capabilities); - assert.ok(capWrite, 'a capabilities write should have been issued'); - assert.strictEqual( - capWrite.capabilities.transport_broadcast_v1, - undefined, - 'the withdrawn payload must not carry the flag' - ); - assert.strictEqual( - capWrite.capabilities.app_version !== undefined, - true, - 'app_version must survive the whole-column overwrite' - ); - }); - - // 13. A transient failure must NOT flap the capability β€” realtime-js re-fires - // CHANNEL_ERROR on every rejoin, so withdrawing eagerly would churn the DB - // and bounce the device between tiers. - await test('a single recreate failure does not withdraw the capability', async () => { - const { rc } = makeRemoteChannel(); - rc.transportCapableWritten = true; - rc.legacyChannel = makeChannel('joined'); - rc.sleep = () => Promise.resolve(); - rc.createChannel = () => Promise.reject(new Error('transient')); - rc.createLegacyChannel = () => {}; - rc.channel = makeChannel('errored'); - - await rc.recreateChannel(); - - assert.strictEqual( - rc.transportCapableWritten, - true, - 'one blip must not withdraw the capability' - ); - }); - - // 14. The heartbeat asserts status:'online', so it must respect the shutdown - // gate too β€” otherwise a tick landing after setOffline()'s subprocess write - // leaves an exited process marked online with a fresh last_seen. - await test('heartbeat is suppressed once shutting down', async () => { - const { rc, client } = makeRemoteChannel(); - rc.channel = makeChannel('joined'); - rc.shuttingDown = true; - await rc.updateHeartbeat('device-1'); - assert.strictEqual(client.writes.length, 0, 'no heartbeat write during shutdown'); - }); - - // 15. The withdrawal runs in recreateChannel()'s catch, which - // RECREATE_TIMEOUT_MS does not cover. A hanging capabilities PATCH must not - // pin isRecreatingChannel=true, or the 10s connection watchdog is silently - // disabled and the device can never recover. - await test('a hanging capability withdrawal cannot pin the recreate guard', async () => { - const { rc } = makeRemoteChannel(); - rc.transportCapableWritten = true; - rc.legacyChannel = makeChannel('joined'); - rc.sleep = () => Promise.resolve(); - rc.createChannel = () => Promise.reject(new Error('Unauthorized')); - rc.createLegacyChannel = () => {}; - rc.channel = makeChannel('errored'); - // A capabilities write that never settles. - rc.setTransportCapable = () => new Promise(() => {}); - // Keep the bound short so the test doesn't wait 5s of real time. - const realWithTimeout = rc.withTimeout.bind(rc); - rc.withTimeout = (op, _ms, name) => realWithTimeout(op, 20, name); - - for (let i = 0; i < 3; i++) { - await rc.recreateChannel(); - } - - assert.strictEqual( - rc.isRecreatingChannel, - false, - 'the re-entrancy guard must be released even when the withdrawal hangs' - ); - }); - - // 16. setOffline() must not be blocked by auth.getSession(). It takes a lock - // with a 10s acquire timeout and refreshes when the token is merely within - // ~90s of expiry, POSTing /token with its own ~30s retry budget β€” so on a - // just-woken machine it can outlast device.ts's 5s force-exit, and then - // spawnSync never runs and the durable offline write is lost entirely. - await test('setOffline does not hang when getSession stalls', async () => { - const { rc } = makeRemoteChannel(); - rc.client.auth = { getSession: () => new Promise(() => {}) }; // never settles - rc.lastKnownSession = { access_token: 'cached-at', refresh_token: 'cached-rt' }; - // Missing supabase config makes setOffline return right after the session - // step, so this test isolates the session fetch and spawns no subprocess. - rc.client.supabaseUrl = undefined; - rc.client.supabaseKey = undefined; - - // The real assertion: it must SETTLE. Without the bound on getSession the - // await below never returns and this test times out. - let settled = false; - await Promise.race([ - rc.setOffline('device-1').then(() => { - settled = true; - }), - new Promise((r) => setTimeout(r, 3000)), - ]); - - assert.strictEqual( - settled, - true, - 'setOffline must settle on a stalled getSession rather than block the shutdown path' - ); - }); - - // 17. stopHeartbeat must not leave a timer able to re-arm itself. - await test('stopHeartbeat halts the self-rescheduling timer', async () => { - const { rc } = makeRemoteChannel(); - rc.channel = makeChannel('joined'); - rc.startHeartbeat('device-1'); - rc.stopHeartbeat(); - assert.strictEqual(rc.heartbeatInterval, null, 'timer handle cleared'); - assert.strictEqual(rc.heartbeatDeviceId, null, 'device id cleared so re-arm is a no-op'); - rc.scheduleHeartbeat(); // must be inert after stop - assert.strictEqual(rc.heartbeatInterval, null, 'scheduleHeartbeat after stop must not re-arm'); - }); - - console.log(`\n${failures ? 'πŸ”΄' : 'βœ…'} remote heartbeat tier: ${failures} failing test(s).`); - process.exit(failures ? 1 : 0); -} - -main(); diff --git a/test/test-remote-transport.js b/test/test-remote-transport.js new file mode 100644 index 00000000..ba12da33 --- /dev/null +++ b/test/test-remote-transport.js @@ -0,0 +1,488 @@ +#!/usr/bin/env node + +/** + * Remote transport tests (Broadcast/Presence + legacy fallback). + * + * Sections: + * 1. Exactly-once execution under dual delivery + * 2. Doorbell routing and row fetch + * 3. Result write ordering + * 4. Heartbeat cadence tiers + * 5. Reachability and status writes + * 6. Capability withdrawal + * 7. Shutdown + * + * Run: npm run build && node test/test-remote-transport.js + */ + +import { MCPDevice } from '../dist/remote-device/device.js'; +import { RemoteChannel } from '../dist/remote-device/remote-channel.js'; + +// Server-side thresholds this device must fit inside. Hand-copied from +// remote-dc-mcp/src/server/constants.ts β€” the repos ship separately and nothing +// enforces the copy, so change both together. +const SERVER_LEGACY_OFFLINE_TIMEOUT_MS = 45 * 1000; +const SERVER_CAPABLE_OFFLINE_TIMEOUT_MS = 15 * 60 * 1000; + +const DEVICE_ID = 'device-1'; +const OTHER_DEVICE = 'device-2'; + +process.env.DESKTOP_COMMANDER_DISABLE_TELEMETRY = '1'; + +let failures = 0; +async function test(name, fn) { + try { + await fn(); + console.log(`βœ… PASS ${name}`); + } catch (error) { + failures++; + console.error(`πŸ”΄ FAIL ${name}\n ${error.message}`); + } +} +function assert(condition, message) { + if (!condition) throw new Error(message); +} + +const makeChannelState = (state) => ({ state }); + +/** MCPDevice with the network and desktop edges stubbed. */ +function makeDevice({ claimResults = [] } = {}) { + const device = new MCPDevice(); + const executed = []; + const claims = [...claimResults]; + + device.deviceId = DEVICE_ID; + device.desktop = { + callClientTool: async (toolName, args) => { + executed.push({ toolName, args }); + return { content: [{ type: 'text', text: 'ok' }] }; + }, + }; + device.remoteChannel = { + // Default: first delivery claims, later ones lose. Override to model a + // transient DB error, which makes the claim return true (fail open). + markCallExecuting: async () => (claims.length ? claims.shift() : true), + updateCallResult: async () => {}, + notifyResult: async () => {}, + }; + return { device, executed }; +} + +/** + * Supabase client fake covering both shapes the device uses: + * `update(...).eq(...)` (awaited) and `select(...).eq(...).maybeSingle()`. + * Records every mcp_devices write in `writes`. + */ +function makeFakeClient({ row = null, failFetches = 0 } = {}) { + const writes = []; + let fetchAttempts = 0; + + const result = () => { + const p = Promise.resolve({ data: null, error: null }); + p.maybeSingle = async () => { + fetchAttempts++; + if (fetchAttempts <= failFetches) { + return { data: null, error: { message: 'fetch failed' } }; + } + return { data: row, error: null }; + }; + p.eq = () => result(); + p.select = () => result(); + return p; + }; + + const chain = { + update: (payload) => { + writes.push(payload); + return chain; + }, + select: () => chain, + insert: () => chain, + eq: () => result(), + }; + + return { + writes, + attempts: () => fetchAttempts, + // Required by recreateChannel(); without them it dies on a TypeError before + // reaching anything the recreate tests stub. + removeChannel: () => Promise.resolve('ok'), + realtime: { disconnect: () => Promise.resolve() }, + from: () => chain, + }; +} + +function makeRemoteChannel(opts = {}) { + const rc = new RemoteChannel(); + const client = makeFakeClient(opts); + rc.client = client; // private in TS, plain property at runtime + rc._user = { id: 'user-1', email: 'tester@example.com' }; + rc.deviceId = DEVICE_ID; + rc.deviceName = 'test-device'; + rc.onToolCall = () => {}; + return { rc, client }; +} + +const payloadFor = (id, deviceId = DEVICE_ID) => ({ + new: { + id, + tool_name: 'start_process', + tool_args: { command: 'echo hi' }, + device_id: deviceId, + metadata: {}, + }, +}); + +// --- 1. Exactly-once under dual delivery ------------------------------------ +// Both transports deliver every call during the transition. The DB claim fails +// OPEN on a transient error, so the in-memory guard is the real guarantee. + +await test('dual delivery of the same call executes the tool exactly once', async () => { + const { device, executed } = makeDevice(); + await device.handleNewToolCall(payloadFor('call-a')); + await device.handleNewToolCall(payloadFor('call-a')); // the other transport + assert(executed.length === 1, `expected 1 execution, got ${executed.length}`); +}); + +await test('exactly-once holds when the DB claim fails OPEN for both deliveries', async () => { + const { device, executed } = makeDevice({ claimResults: [true, true] }); + await device.handleNewToolCall(payloadFor('call-b')); + await device.handleNewToolCall(payloadFor('call-b')); + assert(executed.length === 1, `fail-open claim double-executed: ${executed.length} runs`); +}); + +await test('a lost DB claim (another process won) skips execution', async () => { + const { device, executed } = makeDevice({ claimResults: [false] }); + await device.handleNewToolCall(payloadFor('call-c')); + assert(executed.length === 0, `expected no execution, got ${executed.length}`); +}); + +await test('calls for another device are ignored and do not poison the dedupe set', async () => { + const { device, executed } = makeDevice(); + await device.handleNewToolCall(payloadFor('call-d', OTHER_DEVICE)); + assert(executed.length === 0, 'must not execute another device call'); + // The device filter runs before dedupe, so our own copy must still run. + await device.handleNewToolCall(payloadFor('call-d')); + assert(executed.length === 1, 'a mismatched delivery must not suppress our own'); +}); + +await test('the seen-call-id set stays bounded', async () => { + const { device } = makeDevice(); + for (let i = 0; i < 250; i++) await device.handleNewToolCall(payloadFor(`bulk-${i}`)); + assert(device.seenCallIds.size <= 100, `set grew to ${device.seenCallIds.size}`); +}); + +// --- 2. Doorbell routing ---------------------------------------------------- + +await test('doorbell for another device is ignored without fetching', async () => { + const { rc, client } = makeRemoteChannel(); + await rc.onDoorbell({ call_id: 'x', device_id: OTHER_DEVICE }); + assert(client.attempts() === 0, 'must not even fetch the row'); +}); + +await test('doorbell delivers a pending row through the shared handler', async () => { + const row = { id: 'x', status: 'pending', tool_name: 'start_process' }; + const { rc } = makeRemoteChannel({ row }); + const delivered = []; + rc.onToolCall = (p) => delivered.push(p); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(delivered.length === 1, 'expected one delivery'); + assert(delivered[0].new === row, 'must pass the fetched row as {new: row}'); +}); + +await test('doorbell for an already-claimed row does not re-deliver', async () => { + const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'executing' } }); + const delivered = []; + rc.onToolCall = (p) => delivered.push(p); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(delivered.length === 0, 'non-pending rows must not be re-delivered'); +}); + +await test('doorbell row fetch retries a transient failure', async () => { + const { rc, client } = makeRemoteChannel({ row: { id: 'x', status: 'pending' }, failFetches: 2 }); + const delivered = []; + rc.onToolCall = (p) => delivered.push(p); + rc.sleep = () => Promise.resolve(); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(client.attempts() === 3, `expected 3 attempts, got ${client.attempts()}`); + assert(delivered.length === 1, 'should deliver after the retry succeeds'); +}); + +await test('doorbell with a missing row is a no-op', async () => { + const { rc } = makeRemoteChannel({ row: null }); + const delivered = []; + rc.onToolCall = (p) => delivered.push(p); + await rc.onDoorbell({ call_id: 'gone', device_id: DEVICE_ID }); + assert(delivered.length === 0, 'missing row must not deliver'); +}); + +// --- 3. Result ordering ----------------------------------------------------- +// The server fetches the row by id when the doorbell arrives, so the write must +// land first or it sees a non-terminal row and waits for the recovery poll. + +await test('the result row is written BEFORE the doorbell is rung', async () => { + const order = []; + const { device, executed } = makeDevice(); + device.remoteChannel.updateCallResult = async () => { order.push('write'); }; + device.remoteChannel.notifyResult = async () => { order.push('doorbell'); }; + await device.handleNewToolCall(payloadFor('call-order')); + assert(executed.length === 1, 'tool should have run'); + assert(order.join(',') === 'write,doorbell', `expected write,doorbell β€” got ${order.join(',')}`); +}); + +// --- 4. Heartbeat cadence tiers --------------------------------------------- +// The server tiers its offline sweep on the capability FLAG, not the app +// version, and the flag is only set once presence is proven. So an unproven +// device is judged by the 45s legacy rule and must heartbeat fast enough to +// survive it, or it is swept offline while its legacy channel still works. + +await test('legacy tier heartbeats inside the server 45s sweep threshold', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = null; // never written = legacy tier + const cadence = rc.heartbeatIntervalMs(); + assert( + cadence * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, + `legacy cadence ${cadence}ms must allow >=2 writes inside ${SERVER_LEGACY_OFFLINE_TIMEOUT_MS}ms` + ); + rc.transportCapableWritten = false; // explicitly withdrawn + assert(rc.heartbeatIntervalMs() === cadence, 'a withdrawn capability uses the fast cadence'); +}); + +await test('capable tier heartbeats inside the server capable sweep threshold', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + const cadence = rc.heartbeatIntervalMs(); + assert( + cadence * 2 < SERVER_CAPABLE_OFFLINE_TIMEOUT_MS, + `capable cadence ${cadence}ms must allow >=2 writes inside ${SERVER_CAPABLE_OFFLINE_TIMEOUT_MS}ms` + ); + assert(cadence > SERVER_LEGACY_OFFLINE_TIMEOUT_MS, 'capable cadence is the slow one'); +}); + +await test('withdrawing the capability re-arms the heartbeat at the fast cadence', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.channel = makeChannelState('joined'); + rc.startHeartbeat(DEVICE_ID); + try { + const armed = []; + const realSetTimeout = globalThis.setTimeout; + globalThis.setTimeout = (fn, ms) => { + armed.push(ms); + return realSetTimeout(() => {}, 0); // never fire + }; + try { + await rc.setTransportCapable(false); + } finally { + globalThis.setTimeout = realSetTimeout; + } + assert(armed.length > 0, 'withdrawing must re-arm the heartbeat timer'); + assert( + armed[armed.length - 1] * 2 < SERVER_LEGACY_OFFLINE_TIMEOUT_MS, + `re-armed cadence ${armed[armed.length - 1]}ms must fit the 45s sweep` + ); + } finally { + rc.stopHeartbeat(); + } +}); + +await test('stopHeartbeat halts the self-rescheduling timer', async () => { + const { rc } = makeRemoteChannel(); + rc.channel = makeChannelState('joined'); + rc.startHeartbeat(DEVICE_ID); + rc.stopHeartbeat(); + assert(rc.heartbeatInterval === null, 'timer handle cleared'); + assert(rc.heartbeatDeviceId === null, 'device id cleared so re-arm is inert'); + rc.scheduleHeartbeat(); // must be inert after stop + assert(rc.heartbeatInterval === null, 'scheduleHeartbeat after stop must not re-arm'); +}); + +// --- 5. Reachability and status writes -------------------------------------- +// `status` is what the server's device selection filters on, and it is +// transport-agnostic β€” so it must follow "reachable by ANY transport", never the +// private channel alone. + +await test('heartbeat writes when only the legacy channel is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = null; // private channel never joined + rc.legacyChannel = makeChannelState('joined'); // fallback is up + await rc.updateHeartbeat(DEVICE_ID); + assert(client.writes.length === 1, 'legacy-only reachable device must still write last_seen'); + assert(client.writes[0].last_seen, 'write should bump last_seen'); + assert(client.writes[0].status === 'online', 'write should assert online'); +}); + +await test('heartbeat stays silent when no transport is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('errored'); + rc.legacyChannel = makeChannelState('closed'); + await rc.updateHeartbeat(DEVICE_ID); + assert(client.writes.length === 0, 'a deaf device must let the sweep age its row out'); +}); + +await test('heartbeat writes when the private channel is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('joined'); + rc.legacyChannel = null; + await rc.updateHeartbeat(DEVICE_ID); + assert(client.writes.length === 1, 'private channel joined = reachable'); +}); + +await test('private-channel failure keeps status online while legacy is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('errored'); + rc.legacyChannel = makeChannelState('joined'); + rc.syncReachabilityStatus(); + await rc.statusWriteChain; + assert(client.writes.length === 1, 'one status write'); + assert(client.writes[0].status === 'online', 'still reachable via legacy = online'); +}); + +await test('status goes offline when no transport is joined', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('errored'); + rc.legacyChannel = makeChannelState('closed'); + rc.syncReachabilityStatus(); + await rc.statusWriteChain; + assert(client.writes[0].status === 'offline', 'genuinely deaf device goes offline'); +}); + +await test('concurrent status writes stay ordered', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('joined'); + rc.queueStatusWrite('offline'); // teardown + rc.queueStatusWrite('online'); // immediate re-join + await rc.statusWriteChain; + assert(client.writes.length === 2, 'both writes issued'); + assert( + client.writes.map((w) => w.status).join(',') === 'offline,online', + 'writes must apply in issue order so the join wins' + ); +}); + +// --- 6. Capability withdrawal ----------------------------------------------- +// For a flagged device the server treats absent presence as authoritative +// offline and applies that overlay before selection, overriding `status`. So a +// device that cannot join the private channel must stop advertising the flag or +// it is undispatchable however healthy its legacy channel is. + +await test('sustained recreate failure withdraws the transport capability', async () => { + const { rc, client } = makeRemoteChannel(); + rc.transportCapableWritten = true; // previously proven + rc.legacyChannel = makeChannelState('joined'); + rc.sleep = () => Promise.resolve(); // skip the jittered backoff + const order = []; + rc.createChannel = () => { + order.push('private'); + return Promise.reject(new Error('Unauthorized')); + }; + rc.createLegacyChannel = () => { order.push('legacy'); }; + rc.channel = makeChannelState('errored'); + + for (let i = 0; i < 3; i++) await rc.recreateChannel(); + + // Also proves the recreate reached createChannel rather than dying earlier, + // and that the legacy net is rebuilt first and on every attempt. + assert( + order.slice(0, 2).join(',') === 'legacy,private', + `legacy net must be rebuilt first: ${JSON.stringify(order)}` + ); + assert( + order.filter((o) => o === 'legacy').length === 3, + 'legacy net must be rebuilt on every recreate attempt' + ); + assert(rc.transportCapableWritten === false, 'capability must be withdrawn'); + const capWrite = client.writes.find((w) => w.capabilities); + assert(capWrite, 'a capabilities write should have been issued'); + assert( + capWrite.capabilities.transport_broadcast_v1 === undefined, + 'the withdrawn payload must not carry the flag' + ); + assert(capWrite.capabilities.app_version !== undefined, 'app_version must survive'); +}); + +await test('a single recreate failure does not withdraw the capability', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.legacyChannel = makeChannelState('joined'); + rc.sleep = () => Promise.resolve(); + rc.createChannel = () => Promise.reject(new Error('transient')); + rc.createLegacyChannel = () => {}; + rc.channel = makeChannelState('errored'); + await rc.recreateChannel(); + assert(rc.transportCapableWritten === true, 'one blip must not withdraw'); +}); + +await test('a hanging capability withdrawal cannot pin the recreate guard', async () => { + const { rc } = makeRemoteChannel(); + rc.transportCapableWritten = true; + rc.legacyChannel = makeChannelState('joined'); + rc.sleep = () => Promise.resolve(); + rc.createChannel = () => Promise.reject(new Error('Unauthorized')); + rc.createLegacyChannel = () => {}; + rc.channel = makeChannelState('errored'); + rc.setTransportCapable = () => new Promise(() => {}); // never settles + const realWithTimeout = rc.withTimeout.bind(rc); + rc.withTimeout = (op, _ms, name) => realWithTimeout(op, 20, name); + + for (let i = 0; i < 3; i++) await rc.recreateChannel(); + + assert(rc.isRecreatingChannel === false, 'the guard must be released even if the write hangs'); +}); + +// --- 7. Shutdown ------------------------------------------------------------ +// setOffline()'s durable write is the final word on status, so nothing may race +// or outlast it β€” device.ts force-exits 5s after the signal. + +await test('status writes are suppressed once shutting down', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('joined'); + rc.legacyChannel = makeChannelState('joined'); + rc.shuttingDown = true; + rc.syncReachabilityStatus(); + rc.queueStatusWrite('online'); + await rc.statusWriteChain; + assert(client.writes.length === 0, 'no status write after teardown starts'); +}); + +await test('heartbeat is suppressed once shutting down', async () => { + const { rc, client } = makeRemoteChannel(); + rc.channel = makeChannelState('joined'); + rc.shuttingDown = true; + await rc.updateHeartbeat(DEVICE_ID); + assert(client.writes.length === 0, 'no heartbeat write during shutdown'); +}); + +await test('unsubscribe is bounded and still clears the channel', async () => { + const { rc } = makeRemoteChannel(); + rc.legacyChannel = null; + rc.channel = { + state: 'joined', + untrack: () => new Promise(() => {}), // never settles + unsubscribe: () => new Promise(() => {}), // half-open socket + }; + rc.sleep = () => Promise.resolve(); + await rc.unsubscribe(); + assert(rc.channel === null, 'must give up on a wedged leave push and move on'); + assert(rc.shuttingDown === true, 'teardown flag set'); +}); + +await test('setOffline does not hang when getSession stalls', async () => { + const { rc } = makeRemoteChannel(); + rc.client.auth = { getSession: () => new Promise(() => {}) }; // never settles + rc.lastKnownSession = { access_token: 'cached-at', refresh_token: 'cached-rt' }; + // Missing config makes setOffline return right after the session step, so no + // subprocess is spawned. + rc.client.supabaseUrl = undefined; + rc.client.supabaseKey = undefined; + + let settled = false; + await Promise.race([ + rc.setOffline(DEVICE_ID).then(() => { settled = true; }), + new Promise((r) => setTimeout(r, 3000)), + ]); + assert(settled, 'setOffline must settle rather than block the shutdown path'); +}); + +console.log(`\n${failures ? 'πŸ”΄' : 'βœ…'} remote transport: ${failures} failing test(s).`); +process.exit(failures ? 1 : 0); From 8adb08aa361f55c32555ee596ce8c6f9d3014f05 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Sun, 26 Jul 2026 21:08:19 +0300 Subject: [PATCH 11/13] Fix CodeRabbit findings and cut comments further MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four findings verified against HEAD and valid (CodeRabbit was reviewing an earlier commit, but the issues survive): 1. MAJOR β€” unhandled rejection could kill the device process. handleNewToolCall is async and its catch block awaits updateCallResult/notifyResult, which can themselves throw (`if (!this.client) throw`). Both call sites discarded the promise, so a second failure surfaced as an unhandled rejection β€” fatal on Node 15+. The failure-reporting path now has its own try/catch, and both call sites go through dispatchToolCall(), which observes the rejection. 2. MINOR β€” same root cause at the doorbell and legacy call sites; covered by the same helper. 3. MAJOR β€” updateCallResult's "log a summary, not the payload" comment was an own-goal: the log line then ran JSON.stringify over the whole sanitized result purely to report its length, eagerly, on every call. On the 13 MB payloads this path exists to handle that is a full extra serialization. Now gated on DEBUG_MODE. 4. MINOR β€” the teardown budget did not add up: 250 drain + 3x500 leave + 500 session + 3000 spawnSync = 5250ms against a 5s force-exit, while the comment claimed everything fit in 1.5s. Leave bounds cut to 300ms, total 4650ms, and the arithmetic is now written out. Two new test cases cover the rejection paths (async and sync throw); both were verified to fail with the fix reverted. Comments in remote-channel.ts cut again, 308 -> 275 (23%). Findings 3 and 4 were both cases of a long comment drifting from or contradicting its own code, which is the argument for keeping them short. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR --- src/remote-device/device.ts | 13 ++- src/remote-device/remote-channel.ts | 166 ++++++++++++---------------- test/test-remote-transport.js | 26 +++++ 3 files changed, 109 insertions(+), 96 deletions(-) diff --git a/src/remote-device/device.ts b/src/remote-device/device.ts index fdefc6bc..cc873876 100644 --- a/src/remote-device/device.ts +++ b/src/remote-device/device.ts @@ -356,9 +356,16 @@ export class MCPDevice { } catch (error: any) { console.error(`❌ Tool call ${tool_name} failed:`, error.message); - await captureRemote('remote_device_tool_call_failed', { error, tool_name }); - await this.remoteChannel.updateCallResult(call_id, 'failed', null, error.message); - await this.remoteChannel.notifyResult(call_id); + // The failure path must not fail: this method's promise is discarded + // at every call site, so a throw here becomes an unhandled rejection + // and takes the device process down. + try { + await captureRemote('remote_device_tool_call_failed', { error, tool_name }); + await this.remoteChannel.updateCallResult(call_id, 'failed', null, error.message); + await this.remoteChannel.notifyResult(call_id); + } catch (reportError: any) { + console.error(`❌ Could not report failure for ${call_id}:`, reportError?.message); + } } } diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index e38287d9..a4578587 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -6,11 +6,9 @@ const NUL_CHAR = String.fromCharCode(0); const NUL_RE = new RegExp(NUL_CHAR, 'g'); /** - * Recursively strip NUL characters (U+0000) from strings and object keys. - * Postgres rejects NUL in jsonb and text (22P05), failing the whole write and - * stranding the call at 'executing' until the 5-min timeout. - * Walks the structure rather than round-tripping JSON, which would match the - * escape text and corrupt legitimate content. See test/test-strip-null-bytes.js. + * Strip NUL characters (U+0000) from strings and object keys β€” Postgres rejects + * them in jsonb and text (22P05). Walks the structure rather than + * round-tripping JSON, which would also match escape text in legitimate content. */ export function stripNullBytes(value: T): T { if (typeof value === 'string') { @@ -47,24 +45,20 @@ interface DeviceData { last_seen: string; } -// last_seen cadences. The server tiers its offline sweep on the -// transport_broadcast_v1 flag (not the app version), so each cadence must fit -// its tier's threshold in remote-dc-mcp/src/server/constants.ts: -// capable -> 15 min, unflagged -> 45s. Too slow in the legacy tier and the sweep -// blacks out a device whose legacy channel is still delivering. +// last_seen cadences. The server tiers its sweep on the transport_broadcast_v1 +// flag, so each must fit its tier's threshold in the server's constants.ts: +// capable -> 15 min, unflagged -> 45s. const CAPABLE_HEARTBEAT_INTERVAL = 5 * 60 * 1000; const LEGACY_HEARTBEAT_INTERVAL = 15 * 1000; -// Cap on a recreate's rebuild step, so a hung await can't pin -// isRecreatingChannel and disable the watchdog. Must exceed createChannel()'s -// worst case (~31.5s of presence retries). +// Cap on a recreate's rebuild step so a hung await can't disable the watchdog. +// Must exceed createChannel()'s worst case (~31.5s of presence retries). const RECREATE_TIMEOUT_MS = 45000; // Max continuous time in 'joining' before forcing a recreate β€” a half-open // socket parks the channel there forever, and a genuine join settles in ~10s. const JOINING_WEDGE_TIMEOUT_MS = 30000; -// Failed recreates before withdrawing transport_broadcast_v1: keeping the flag -// while unable to join makes the device undispatchable. Not lower than 3 β€” -// ordinary half-open recovery costs 2, since the first recreate's join is -// buffered while realtime-js refuses to redial the socket it just dropped. +// Failed recreates before withdrawing transport_broadcast_v1 β€” keeping it while +// unable to join makes the device undispatchable. Not lower than 3: ordinary +// half-open recovery legitimately costs 2. const TRANSPORT_WITHDRAW_AFTER_ATTEMPTS = 3; // Cap on the withdrawal write; it runs in a catch block RECREATE_TIMEOUT_MS // does not cover. @@ -75,11 +69,8 @@ const OFFLINE_SESSION_TIMEOUT_MS = 500; export class RemoteChannel { private client: SupabaseClient | null = null; private channel: RealtimeChannel | null = null; - /** - * Legacy postgres_changes listener, on its OWN public channel β€” deliberately - * not the private one, so a private-channel auth failure can't take both - * transports down. Removed at the flip (009). - */ + /** Legacy listener, on its own public channel so a private-channel auth + * failure can't take both transports down. Removed at the flip (009). */ private legacyChannel: RealtimeChannel | null = null; private heartbeatInterval: NodeJS.Timeout | null = null; private connectionCheckInterval: NodeJS.Timeout | null = null; @@ -159,16 +150,12 @@ export class RemoteChannel { this._user = user; console.debug('[DEBUG] Session set successfully, user:', user.email); - // Private channels authorize with the user JWT at join time. Push the - // CURRENT token, not the one we were handed: setSession() refreshes an - // expired token internally, and pushing the stale parameter would - // overwrite it, failing every join until the next refresh. + // Push the CURRENT token, not the one we were handed: setSession() + // refreshes internally, and the stale parameter would overwrite it. const { data: { session: currentSession } } = await this.client.auth.getSession(); const realtimeToken = currentSession?.access_token ?? session.access_token; this.client.realtime.setAuth(realtimeToken); - // Cache tokens for the shutdown path: setOffline() must not depend on a - // getSession() that can block on a token refresh while device.ts's 5s - // force-exit is running down (see setOffline). + // Cached for setOffline(), which can't afford to wait on getSession(). this.lastKnownSession = { access_token: realtimeToken, refresh_token: currentSession?.refresh_token ?? session.refresh_token ?? null, @@ -261,10 +248,8 @@ export class RemoteChannel { if (existingDevice) { console.debug('[DEBUG] Updating device status to online'); - // transport_broadcast_v1 is deliberately NOT set here. The server - // treats the flag as binding β€” for a flagged device absent presence - // means offline β€” so it is only written once presence is proven - // (setTransportCapable), and withdrawn if presence fails. + // transport_broadcast_v1 is NOT set here: the server treats it as + // binding, so it is written only once presence is proven. await this.updateDevice(existingDevice.id, { status: 'online', last_seen: new Date().toISOString(), @@ -282,9 +267,7 @@ export class RemoteChannel { // Create and subscribe to the channel console.debug('[DEBUG] Calling createChannel()'); - // ! Ignore silently in Initialization to reconnect after - // Legacy postgres_changes listener on its own public channel β€” the - // independent safety net for the doorbell transport (see legacyChannel). + // Independent safety net for the doorbell transport. this.createLegacyChannel(); await this.createChannel().catch((error) => { @@ -331,13 +314,10 @@ export class RemoteChannel { if (status === 'ok') { this.presenceTracked = true; console.log(`πŸ‘‹ Presence tracked (device ${this.deviceId} visible as online)`); - // recoveredAfterAttempts, not "attempt": this is how many - // reconnect attempts preceded the join that carried this track - // (0 on a first join AND on the health-check self-heal path). + // Reconnect attempts preceding this join (0 on a first join). captureRemote('remote_channel_presence_tracked', { recoveredAfterAttempts: recovered }).catch(() => { }); - // Transport is proven end-to-end (channel joined AND presence - // published) β€” only now is it safe to let the server route us - // over broadcast and treat our presence as authoritative. + // Proven end-to-end (joined AND presence published) β€” only now + // may the server treat our presence as authoritative. await this.setTransportCapable(true); return; } @@ -349,9 +329,8 @@ export class RemoteChannel { this.presenceTracked = false; console.error('❌ Presence track failed after retries β€” reverting to the legacy transport tier'); captureRemote('remote_channel_presence_track_error', { attempts }).catch(() => { }); - // Withdraw the promise: without presence the server cannot see us, and a - // stale capability flag would make it refuse to dispatch entirely. Going - // back to the legacy tier keeps the device usable over postgres_changes. + // Withdraw: a stale flag with no presence makes the server refuse to + // dispatch at all. The legacy tier keeps the device usable. await this.setTransportCapable(false); } @@ -367,10 +346,10 @@ export class RemoteChannel { } /** - * Advertise (or withdraw) the broadcast capability. Only true while the - * device is genuinely reachable that way: the server uses it to pick a - * transport, to read absent presence as offline, and to choose the sweep - * tier β€” so every change must re-arm the heartbeat to match. + * Advertise (or withdraw) the broadcast capability. Only true while genuinely + * reachable that way β€” the server uses it to pick a transport, to read absent + * presence as offline, and to choose the sweep tier, so every change must + * re-arm the heartbeat. */ private async setTransportCapable(capable: boolean): Promise { if (!this.client || !this.deviceId) return; @@ -390,9 +369,8 @@ export class RemoteChannel { // Tier changed β€” move last_seen onto the cadence that tier's sweep // threshold expects (no-op if the heartbeat hasn't started yet). this.scheduleHeartbeat(); - // Dropping to the fast tier: last_seen may already be minutes old, - // i.e. past the 45s threshold now judging us. Write once immediately - // instead of waiting out the new interval. + // last_seen may already be past the 45s threshold now judging us, + // so write once immediately rather than waiting out the interval. if (!capable && this.heartbeatDeviceId) { this.updateHeartbeat(this.heartbeatDeviceId).catch(() => { /* logged inside */ }); } @@ -420,9 +398,7 @@ export class RemoteChannel { }, (payload: any) => { console.debug('[DEBUG] Realtime event received, payload:', payload?.new?.id); - if (this.onToolCall) { - this.onToolCall(payload); - } + this.dispatchToolCall(payload); } ) .subscribe((status: string) => { @@ -442,38 +418,27 @@ export class RemoteChannel { this.legacyChannel = null; } - /** - * Create and subscribe to the channel. - * This is used for both initial subscription and recreation after socket reconnects. - */ + /** Create and subscribe the private channel (initial join and recreation). */ private createChannel(): Promise { return new Promise((resolve, reject) => { if (!this.client || !this.user?.id || !this.onToolCall || !this.deviceId) { - // deviceId included deliberately: it is the presence KEY, and a - // null key makes realtime assign a random one β€” the server's - // lookup by device id then misses and the device is invisible - // while every local signal says healthy. + // deviceId is the presence KEY; a null key gets a random one and + // the server's lookup by device id silently misses. console.debug('[DEBUG] createChannel() failed - missing prerequisites'); return reject(new Error('Client not initialized or missing subscription parameters')); } - // Private per-user channel: carries the legacy postgres_changes - // listener (kept until the fleet-wide flip), the new_call broadcast - // doorbell, and this device's Presence (key = device id, so the - // server and dashboard read liveness straight off presenceState()). + // Private per-user channel: new_call doorbells + this device's + // Presence, keyed by device id. const channelName = `user:${this.user.id}`; console.debug(`[DEBUG] Creating channel: ${channelName}`); this.channel = this.client.channel(channelName, { - // ack: true β€” without it send() resolves 'ok' as soon as the frame - // is written to the socket, so notifyResult's status check (and its - // failure telemetry) could never fire. presence.enabled makes the - // presence extension explicit rather than inferred. + // ack: true β€” without it send() resolves 'ok' once the frame hits + // the socket, making notifyResult's status check dead code. config: { private: true, broadcast: { ack: true }, - // key is non-null: the guard above rejects when !deviceId, - // precisely because a null key makes realtime assign a - // random one and the server's lookup by device id misses. + // Non-null: the guard above rejects when !deviceId. presence: { key: this.deviceId, enabled: true } } }) @@ -497,10 +462,9 @@ export class RemoteChannel { // Update device status on successful connection (queued, so // it can't be overtaken by a teardown's status write). this.queueStatusWrite('online'); - // Presence IS the live "online" signal dispatch reads, so - // retry, and resolve only once it lands β€” otherwise - // registerDevice() prints "Device ready" while the device - // is still undispatchable. + // Presence is the live signal dispatch reads, so resolve + // only once it lands β€” otherwise registerDevice() reports + // "Device ready" while still undispatchable. this.trackPresenceWithRetry(recovered) .catch(() => { /* logged inside */ }) .finally(() => resolve()); @@ -509,10 +473,8 @@ export class RemoteChannel { console.error(`❌ Channel error: ${err?.message || 'unknown'} β€” ${this.connState()}`); this.presenceTracked = false; this.syncReachabilityStatus(); - // Single event: this fires on ordinary network faults too, so a - // separate "private join failed" alarm would be a 1:1 duplicate - // with no added specificity. Filter on the error text - // ("Unauthorized"/policy) to isolate an 008 misconfiguration. + // Fires on ordinary network faults too β€” filter on the + // error text to isolate an 008 misconfiguration. captureRemote('remote_channel_subscription_error', { error: err?.message || 'Channel error' }).catch(() => { }); reject(err || new Error('Failed to initialize tool call channel subscription')); } else if (status === 'TIMED_OUT') { @@ -531,6 +493,21 @@ export class RemoteChannel { }); } + /** Hand a call to device.ts, observing the rejection β€” the handler is async + * and an unhandled rejection terminates the process. */ + private dispatchToolCall(payload: any): void { + try { + const maybePromise = this.onToolCall?.(payload) as unknown; + if (maybePromise instanceof Promise) { + maybePromise.catch((e: any) => { + console.error('[DEBUG] Tool call handler rejected:', e?.message); + }); + } + } catch (e: any) { + console.error('[DEBUG] Tool call handler threw:', e?.message); + } + } + /** * Handle a 'new_call' doorbell. It carries ids only; the row is fetched by * primary key and fed through the same handler as a postgres_changes @@ -590,7 +567,7 @@ export class RemoteChannel { } // Same payload shape as postgres_changes ({ new: row }). - this.onToolCall?.({ new: row }); + this.dispatchToolCall({ new: row }); } /** @@ -854,12 +831,15 @@ export class RemoteChannel { // fallback below wouldn't fire, stranding the call until the 5-min timeout. if (errorMessage !== null) updateData.error_message = stripNullBytes(errorMessage); - // Log a summary, not the payload: results reach 13 MB and util.inspect - // on the hot path costs more than everything else in this function. - console.debug( - `[DEBUG] Updating call result: ${callId} status=${status}` + - (result !== null ? ` resultBytes=~${JSON.stringify(updateData.result)?.length ?? 0}` : '') - ); + // Gated: the size is only knowable by serializing, and results reach + // 13 MB β€” doing that eagerly for a log line would cost more than the + // rest of this function. + if (process.env.DEBUG_MODE === 'true') { + console.debug( + `[DEBUG] Updating call result: ${callId} status=${status}` + + (result !== null ? ` resultBytes=~${JSON.stringify(updateData.result)?.length ?? 0}` : '') + ); + } const { error } = await this.client .from('mcp_remote_calls') .update(updateData) @@ -1157,11 +1137,11 @@ export class RemoteChannel { // SIGINT during recreateChannel()'s backoff, where the later join's // SUBSCRIBED would queue 'online' after the durable write. this.shuttingDown = true; - // Budget: device.ts force-exits 5s after the signal and setOffline needs - // ~3.5s of it, so everything here must fit in ~1.5s. - // Only the untrack bound can actually bind β€” removeChannel/unsubscribe + // Budget against device.ts's 5s force-exit, worst case: + // 250 drain + 3x300 leave + 500 session + 3000 spawnSync = 4650ms. + // In practice only the untrack bound binds β€” removeChannel/unsubscribe // set state='leaving' first, so their leave push resolves inline. - const LEAVE_BOUND_MS = 500; + const LEAVE_BOUND_MS = 300; // Drain queued channel-callback writes. Can't drain an in-flight // heartbeat PATCH (it doesn't use the chain), but the gate above stops // any new one and an in-flight one started earlier. diff --git a/test/test-remote-transport.js b/test/test-remote-transport.js index ba12da33..b32f3bb0 100644 --- a/test/test-remote-transport.js +++ b/test/test-remote-transport.js @@ -216,6 +216,32 @@ await test('doorbell with a missing row is a no-op', async () => { assert(delivered.length === 0, 'missing row must not deliver'); }); +// --- 2b. Handler rejections are observed ------------------------------------- +// handleNewToolCall is async and its promise is discarded at both call sites, so +// a rejection would be unhandled and terminate the device process. + +await test('a rejecting tool-call handler does not produce an unhandled rejection', async () => { + const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } }); + rc.onToolCall = async () => { throw new Error('handler blew up'); }; + + const unhandled = []; + const onUnhandled = (e) => unhandled.push(e); + process.on('unhandledRejection', onUnhandled); + try { + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + await new Promise((r) => setImmediate(r)); // let a rejection surface + } finally { + process.off('unhandledRejection', onUnhandled); + } + assert(unhandled.length === 0, `unhandled rejection escaped: ${unhandled[0]?.message}`); +}); + +await test('a synchronously throwing handler is contained too', async () => { + const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } }); + rc.onToolCall = () => { throw new Error('sync throw'); }; + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); // must not reject +}); + // --- 3. Result ordering ----------------------------------------------------- // The server fetches the row by id when the doorbell arrives, so the write must // land first or it sees a non-terminal row and waits for the recovery poll. From 686098e7674be59b4f8b2f7c96d8447c88892005 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Mon, 27 Jul 2026 12:35:14 +0300 Subject: [PATCH 12/13] Bound the output stored per tool-history entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toolHistory keeps the full ServerResult of every call in memory, and get_recent_tool_calls serialises those entries straight back out. That is unbounded in two compounding ways: - one large output (a big read_file, a wide list_directory) makes every later history dump that includes it large too, and - any tool whose output CONTAINS a history dump nests the whole history inside itself. Observed while testing: a start_process running `cat /tmp/dc_..._get_recent_tool_calls_....json` recorded that dump as its own output, and each nesting level roughly doubles the JSON escaping. 83KB of arguments on disk produced a 1.89MB in-memory dump. That 1.89MB result then exceeded Supabase Realtime's max_record_bytes, which is how it surfaced β€” the transport returned `undefined` (fixed separately on the server side). Extending EXCLUDED_TOOLS cannot fix this: get_recent_tool_calls is already excluded, and the nesting arrives through ordinary tools that merely happen to read a dump back. Capping the stored output does. A preview loses nothing real β€” this history is a "what happened recently" aid, not a result cache. Outputs over 4KB are replaced with a short marker, keeping the { content: [...] } shape so readers and formatters need no special case. An unserialisable output (circular) is also dropped rather than retained, since it could never be returned to a client anyway. Note this only bounds NEW entries; a long-running process keeps whatever it has already accumulated until restart. Also fixes test/test-line-count.js, which set allowedDirectories to its own test directory at import time and never restored it β€” leaking into the user's real ~/.claude-server-commander/config.json and refusing every later filesystem call outside test_output. It was the only test in the suite without a restore. Now saves the config in setup(), restores in teardown(), and runs teardown on the failure path too (it previously called process.exit(1) and skipped cleanup). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR --- src/utils/toolHistory.ts | 54 +++++++++++++++++++++++++++++++++++++--- test/test-line-count.js | 33 +++++++++++++++++++----- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/utils/toolHistory.ts b/src/utils/toolHistory.ts index 09366744..7647a52b 100644 --- a/src/utils/toolHistory.ts +++ b/src/utils/toolHistory.ts @@ -32,6 +32,25 @@ function formatLocalTimestamp(isoTimestamp: string): string { class ToolHistory { private history: ToolCallRecord[] = []; private readonly MAX_ENTRIES = 1000; + /** + * Cap on the output kept per entry. Entries hold the FULL ServerResult, and + * get_recent_tool_calls serialises them straight back out, so without a cap + * the history is unbounded in two compounding ways: + * + * - a single large output (a big read_file, a wide list_directory) makes + * every later history dump that includes it large too, and + * - any tool whose output happens to CONTAIN a history dump β€” e.g. + * `cat`-ing a file a previous dump was written to β€” nests the whole + * history inside itself, and each nesting level roughly doubles the JSON + * escaping. Observed 2026-07-27: 83KB of arguments on disk produced a + * 1.89MB in-memory dump this way. + * + * Excluding more tool names cannot fix that, because the nesting arrives + * through ordinary tools. Capping the stored output does, and a preview is + * all this history is for β€” it is a "what happened recently" aid, not a + * result cache. + */ + private readonly MAX_STORED_OUTPUT_BYTES = 4 * 1024; private readonly MAX_HISTORY_FILE_SIZE_BYTES = 5 * 1024 * 1024; // When the file exceeds the cap we trim it down to this target instead of // all the way to zero, so a single overflow doesn't cause every subsequent @@ -214,17 +233,44 @@ class ToolHistory { /** * Add a tool call to history */ + /** + * Replace an oversized output with a short marker. Keeps the record shape + * ({ content: [...] }) so readers and formatters need no special case. + */ + private capOutput(output: ServerResult): ServerResult { + let size: number; + try { + size = JSON.stringify(output)?.length ?? 0; + } catch { + // Circular or otherwise unserialisable β€” it could never be returned to a + // client anyway, so don't retain it. + size = Number.POSITIVE_INFINITY; + } + if (size <= this.MAX_STORED_OUTPUT_BYTES) return output; + + const shown = Number.isFinite(size) ? `${size} bytes` : 'unserialisable'; + return { + ...(output as any), + content: [ + { + type: 'text', + text: `[output omitted from history: ${shown}, over the ${this.MAX_STORED_OUTPUT_BYTES}-byte cap]`, + }, + ], + } as ServerResult; + } + addCall( - toolName: string, - args: any, - output: ServerResult, + toolName: string, + args: any, + output: ServerResult, duration?: number ): void { const record: ToolCallRecord = { timestamp: new Date().toISOString(), toolName, arguments: args, - output, + output: this.capOutput(output), duration }; diff --git a/test/test-line-count.js b/test/test-line-count.js index e467ffea..f6245eab 100644 --- a/test/test-line-count.js +++ b/test/test-line-count.js @@ -10,13 +10,25 @@ const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); const TEST_DIR = join(__dirname, 'test_output'); -// Ensure test dir is allowed -await configManager.setValue('allowedDirectories', [TEST_DIR]); +// Saved before the test narrows allowedDirectories, restored in teardown(). +// Without this the test leaks its own directory into the user's real +// ~/.claude-server-commander/config.json and every later filesystem call β€” +// in this process or any other β€” is refused outside test_output. +let originalConfig = null; async function setup() { + originalConfig = await configManager.getConfig(); + // Ensure test dir is allowed + await configManager.setValue('allowedDirectories', [TEST_DIR]); await fs.mkdir(TEST_DIR, { recursive: true }); } +async function teardown() { + if (originalConfig) { + await configManager.updateConfig(originalConfig); + } +} + async function createTestFile(name, content) { const filePath = join(TEST_DIR, name); await fs.writeFile(filePath, content, 'utf8'); @@ -111,10 +123,19 @@ async function testLineCount() { } console.log(`\n${passed} passed, ${failed} failed out of ${passed + failed} tests`); - if (failed > 0) process.exit(1); + return failed; } -setup().then(testLineCount).catch(err => { +// teardown must run on the failure path too, or a failing assertion leaves the +// user's allowedDirectories pinned to test_output. +let exitCode = 0; +try { + await setup(); + exitCode = (await testLineCount()) > 0 ? 1 : 0; +} catch (err) { console.error('Test error:', err); - process.exit(1); -}); + exitCode = 1; +} finally { + await teardown(); +} +process.exit(exitCode); From 6aeb171db0b6d192d92bca2df5050eb44463f008 Mon Sep 17 00:00:00 2001 From: edgarssskore Date: Wed, 29 Jul 2026 09:24:14 +0300 Subject: [PATCH 13/13] Round-9 review fixes (device): recreate socket settle, history load cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from a cross-repo review of this branch. 1. recreateChannel() rebuilt both channels inside realtime-js's post-disconnect 'disconnecting' window. removeChannel() on the last channel makes realtime-js call disconnect() itself; _teardownConnection() then nulls the conn.onclose that would clear the state, so only an internal ~100ms fallback timer does. connect() early-returns for that whole window, so subscribe()'s socket.connect() was a silent no-op and BOTH channels β€” including the deliberately independent legacy one β€” sat in 'joining' until the 10s join timeout. Every first recreate was wasted, leaving the device dark on both transports for ~20-45s and burning one of three TRANSPORT_WITHDRAW_AFTER_ATTEMPTS before a genuine attempt. Reproduced against the installed realtime-js 2.90.0. Now waits for the client to leave 'disconnecting' before rebuilding. 2. The per-entry tool-history output cap ran only in addCall(), never on the load-from-disk path, so it did nothing for anyone upgrading with an existing tool-history.jsonl β€” i.e. exactly the population it was written for. Verified against a real 4.0MB/140-record file holding a single 3.8MB read_file entry: under the 5MB whole-file trim, so it loaded uncapped and get_recent_tool_calls still returned it in full. Now capped on the way in as well as on the way out. 3. 'concurrent status writes stay ordered' could not fail. The fake recorded writes at .update() invocation, which happens synchronously before setOnlineStatus's only await, so it only ever observed the order writes were ISSUED β€” which holds with or without the statusWriteChain serialisation. The fake now records on completion with per-write latency; with queueStatusWrite reverted to fire-and-forget the test goes red on the ordering assertion ("got online,offline"), and passes as shipped. Build clean; test/test-remote-transport.js 30/30. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BdFJB6KxeWHdYqbi63KrPP --- src/remote-device/remote-channel.ts | 37 ++++++++++++++++ src/utils/toolHistory.ts | 10 ++++- test/test-remote-transport.js | 69 ++++++++++++++++++++++++++--- 3 files changed, 108 insertions(+), 8 deletions(-) diff --git a/src/remote-device/remote-channel.ts b/src/remote-device/remote-channel.ts index a4578587..1d0bcdff 100644 --- a/src/remote-device/remote-channel.ts +++ b/src/remote-device/remote-channel.ts @@ -65,6 +65,12 @@ const TRANSPORT_WITHDRAW_AFTER_ATTEMPTS = 3; const CAPABILITY_WRITE_TIMEOUT_MS = 5000; // Cap on the shutdown session fetch, which races device.ts's 5s force-exit. const OFFLINE_SESSION_TIMEOUT_MS = 500; +// realtime-js parks in 'disconnecting' for ~100ms after a disconnect and +// connect() early-returns for that whole window (see waitForSocketSettled). +// Bound generously β€” this only ever delays a recreate, which RECREATE_TIMEOUT_MS +// already covers. +const SOCKET_SETTLE_MAX_MS = 300; +const SOCKET_SETTLE_POLL_MS = 20; export class RemoteChannel { private client: SupabaseClient | null = null; @@ -672,6 +678,26 @@ export class RemoteChannel { return new Promise((resolve) => setTimeout(resolve, ms)); } + /** + * Block until realtime-js has left the 'disconnecting' state it enters on + * disconnect(), so the next subscribe() actually dials a socket instead of + * hitting connect()'s early return. Bounded either way β€” worst case we cost + * a recreate SOCKET_SETTLE_MAX_MS. + */ + private async waitForSocketSettled(): Promise { + const realtime = (this.client as any)?.realtime; + // No predicate to poll (older/newer client): wait out the internal + // fallback timer blind rather than guess at the state. + if (typeof realtime?.isDisconnecting !== 'function') { + await this.sleep(SOCKET_SETTLE_MAX_MS); + return; + } + const deadline = Date.now() + SOCKET_SETTLE_MAX_MS; + while (realtime.isDisconnecting() && Date.now() < deadline) { + await this.sleep(SOCKET_SETTLE_POLL_MS); + } + } + private async withTimeout(op: () => Promise, ms: number, name: string): Promise { let timer: NodeJS.Timeout | undefined; try { @@ -745,6 +771,17 @@ export class RemoteChannel { // forever. disconnect() drops it so the next subscribe() dials a fresh one. try { await (this.client as any).realtime?.disconnect?.(); } catch { /* best effort */ } + // ...but disconnect() is not synchronous from connect()'s point + // of view: it parks _connectionState in 'disconnecting' and + // _teardownConnection() nulls the conn.onclose that would clear + // it, so only an internal ~100ms fallback timer does. connect() + // early-returns for that whole window, so rebuilding here makes + // subscribe()'s socket.connect() a silent no-op and BOTH + // channels sit in 'joining' until the 10s join timeout β€” the + // wasted-first-recreate that left the device dark on the legacy + // channel too. Wait for the state to settle before rebuilding. + await this.waitForSocketSettled(); + console.debug('[DEBUG] Calling createChannel() for recreation'); // Rebuild the legacy safety net FIRST and unconditionally: if // createChannel() throws or exceeds RECREATE_TIMEOUT_MS, anything diff --git a/src/utils/toolHistory.ts b/src/utils/toolHistory.ts index 7647a52b..6c9d5488 100644 --- a/src/utils/toolHistory.ts +++ b/src/utils/toolHistory.ts @@ -106,8 +106,14 @@ class ToolHistory { } } - // Keep only last 1000 entries - this.history = records.slice(-this.MAX_ENTRIES); + // Keep only last 1000 entries, and cap on the way IN as well as on the + // way out. Entries written before the cap existed are still on disk and + // are well under the whole-file trim threshold, so without this the cap + // does nothing for anyone upgrading with an existing history file β€” i.e. + // for everyone it was written for. + this.history = records + .slice(-this.MAX_ENTRIES) + .map(record => ({ ...record, output: this.capOutput(record.output) })); // If file is getting too large, trim it if (lines.length > this.MAX_ENTRIES * 2) { diff --git a/test/test-remote-transport.js b/test/test-remote-transport.js index b32f3bb0..d6be394a 100644 --- a/test/test-remote-transport.js +++ b/test/test-remote-transport.js @@ -45,6 +45,11 @@ function assert(condition, message) { const makeChannelState = (state) => ({ state }); +// Captured before any test runs: the heartbeat re-arm test monkeypatches +// globalThis.setTimeout to never fire, and the fake client's write completion +// must not silently depend on that. +const realSetTimeout = globalThis.setTimeout; + /** MCPDevice with the network and desktop edges stubbed. */ function makeDevice({ claimResults = [] } = {}) { const device = new MCPDevice(); @@ -73,9 +78,15 @@ function makeDevice({ claimResults = [] } = {}) { * `update(...).eq(...)` (awaited) and `select(...).eq(...).maybeSingle()`. * Records every mcp_devices write in `writes`. */ -function makeFakeClient({ row = null, failFetches = 0 } = {}) { +function makeFakeClient({ row = null, failFetches = 0, writeLatencies = [] } = {}) { const writes = []; + // Recorded when a write COMPLETES, not when it is issued. `writes` alone + // cannot test ordering: setOnlineStatus evaluates .update() synchronously + // before its only await, so issue order holds with or without the + // statusWriteChain serialisation. + const completions = []; let fetchAttempts = 0; + let pendingWrite = null; const result = () => { const p = Promise.resolve({ data: null, error: null }); @@ -94,20 +105,52 @@ function makeFakeClient({ row = null, failFetches = 0 } = {}) { const chain = { update: (payload) => { writes.push(payload); + // Per-write completion latency, so a test can make an earlier write land + // LATER than a later one β€” the only way to observe serialisation. + pendingWrite = { + payload, + delay: writeLatencies.length ? writeLatencies.shift() : 0, + }; return chain; }, select: () => chain, insert: () => chain, - eq: () => result(), + eq: () => { + if (!pendingWrite) return result(); + const { payload, delay } = pendingWrite; + pendingWrite = null; + const p = new Promise((resolve) => { + const settle = () => { + completions.push(payload); + resolve({ data: null, error: null }); + }; + // Only defer when a test actually asked for latency, so every other + // test keeps the original resolve-immediately semantics. + if (delay > 0) realSetTimeout(settle, delay); + else settle(); + }); + // markCallExecuting chains .eq().eq().select() off a single update(), so + // this must stay chainable exactly like result() does β€” returning a bare + // promise leaves that chain hanging forever. + p.eq = () => p; + p.select = () => p; + p.maybeSingle = async () => ({ data: null, error: null }); + return p; + }, }; return { writes, + completions, attempts: () => fetchAttempts, // Required by recreateChannel(); without them it dies on a TypeError before // reaching anything the recreate tests stub. removeChannel: () => Promise.resolve('ok'), - realtime: { disconnect: () => Promise.resolve() }, + // isDisconnecting models a client that has already settled, so + // waitForSocketSettled() polls once and returns. NOTE: this fake has no + // real connection state, so it cannot observe whether a new socket was + // actually dialled β€” the recreate tests verify sequencing, not transport. + realtime: { disconnect: () => Promise.resolve(), isDisconnecting: () => false }, from: () => chain, }; } @@ -374,15 +417,29 @@ await test('status goes offline when no transport is joined', async () => { }); await test('concurrent status writes stay ordered', async () => { - const { rc, client } = makeRemoteChannel(); + // The first write completes AFTER the second is issued. Without the + // statusWriteChain serialisation the teardown's 'offline' then lands at the + // DB after the re-join's 'online', leaving a healthy device undispatchable + // until the next heartbeat (up to 5 min on the capable tier). Assert on + // `completions`, not `writes` β€” see makeFakeClient. + const { rc, client } = makeRemoteChannel({ writeLatencies: [20, 0] }); rc.channel = makeChannelState('joined'); rc.queueStatusWrite('offline'); // teardown rc.queueStatusWrite('online'); // immediate re-join await rc.statusWriteChain; + // Let the deferred first write land even when the implementation does NOT + // serialise, so this fails on ORDER β€” the actual bug β€” rather than on timing. + const deadline = Date.now() + 500; + while (client.completions.length < 2 && Date.now() < deadline) { + await new Promise((r) => realSetTimeout(r, 5)); + } assert(client.writes.length === 2, 'both writes issued'); + assert(client.completions.length === 2, 'both writes completed'); assert( - client.writes.map((w) => w.status).join(',') === 'offline,online', - 'writes must apply in issue order so the join wins' + client.completions.map((w) => w.status).join(',') === 'offline,online', + `writes must COMPLETE in issue order so the join wins, got ${client.completions + .map((w) => w.status) + .join(',')}` ); });