Fix/remote supervision hardening - #603
Open
edgarsskore wants to merge 3 commits into
Open
Conversation
…-flight calls fail fast The SDK chains transport onclose/onerror handlers that exist BEFORE client.connect() (its wrapper calls ours, then its own close handling, which rejects in-flight requests with "Connection closed" immediately). Assigned after connect() they REPLACED the SDK's handler, so a tool call in flight when the local child died hung for the full 60s request timeout instead of failing in ~0.1s as it did before supervision was added. Measured: 60.0s "Request timed out" -> 1.0s "Connection closed". Also guard callClientTool against the child dying between ensureReady() resolving and callTool() dispatching: surface a descriptive Error instead of a TypeError on a nulled client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…from both health signals handleLocalMcpLoss restarted the child instantly and gave up after one failed attempt. A crash-looping child therefore cycled every ~0.35s, PATCHing mcp_devices twice per cycle (~5-6 writes/sec per device, indefinitely) — the same write pattern as the 2026-07-18 prod DB incident — while a single transient restart failure (files mid-upgrade, resource exhaustion) left the device offline until a human restarted the process, since offline devices receive no calls to trigger the lazy restart. Recovery is now a serialized retry loop with exponential backoff: 2s base doubling to a 5min ceiling, 15% jitter so fleet-wide triggers don't synchronize, never gives up, telemetry per failed attempt, and the ladder resets after 60s of stable child uptime so a fresh one-off crash still recovers in ~2-3s. Test knobs: DC_LOCAL_RESTART_BACKOFF_BASE_MS / DC_LOCAL_RESTART_STABLE_UPTIME_MS. Because backoff widens dead-child windows from ~0.35s to minutes, the channel's unconditional 'online' write on every SUBSCRIBED became a real hazard: a routine resubscribe during recovery would advertise a device whose child is dead. New DeviceStatusArbiter is the single mcp_devices status writer: online iff BOTH channel and child are healthy, transition-only writes (a reconnect storm re-reporting the same state produces zero extra PATCHes), sync() after registration. RemoteChannel reports health through it, with a legacy direct-write fallback when unwired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five cases in test-remote-device-supervision.js, each verified failing on the unfixed #598 head before the fixes landed (red -> green): - in-flight call fails fast on child death (was 60.0s, now ~1s) - restart backoff escalates (was constant ~330ms recovery cadence) - restart retries through transient failures (was give-up-after-one) - callClientTool surfaces a descriptive error, not a TypeError - status arbiter: channel-up + child-dead must not write 'online', transition-only writes Real MCPDevice/DesktopCommanderIntegration with only setOnlineStatus stubbed; optional case-name filter arg for single-case runs. Also rework the "bogus executable" case in test-spawn-error-no-crash.js: a falsy shell argument is coerced to the configured default shell, so the case ran through a shell and could never produce a spawn 'error' event — it now honestly asserts the exit-127 command-not-found path (spawn-error coverage remains with the bogus-shell case). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardening on top of #598 (which correctly fixes both bugs — verified). Three things found while verifying it:
1. Regression: in-flight calls hung 60s on child death (882f0b0)
transport.onclosewas assigned afterclient.connect(), replacing the SDK's handler that fails in-flight calls fast. Measured 60.0s → fixed to ~1s by moving the assignment aboveconnect().2. Restart policy (4488b2b)
Restarts were instant-forever (crash-looping child = ~0.35s cycles = ~5–6
mcp_devicesPATCHes/sec, the 2026-07-18 incident pattern) or once-never (one failed restart = offline until manually restarted). Now: exponential backoff 2s → 5min ceiling, jittered, never gives up, resets after 60s stable uptime.Since recovery windows are now minutes, added
DeviceStatusArbiter: single status writer, online iff channel AND child healthy, transition-only writes — a channel resubscribe can no longer mark a dead-child device online.