Repository navigation
Fall back to SSH when Cloud VM attach endpoint is unavailable - #6079
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesFreestyleProvider SSH Fallback Eligibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e258674228
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async (span) => { | ||
| recordSpanError(span, err); |
There was a problem hiding this comment.
Keep fallback split attaches on SSH
When this fallback branch is reached for a VM that still has a PTY websocket but no daemon, cmux vm new creates an SSH fallback workspace, and runSSHWithOptions stores cmux vm ssh-attach --id … as the reusable terminal command (CLI/cmux.swift:8223-8227). That command calls vm.attach_info without require_daemon and then parses the response as SSH (CLI/cmux.swift:9691-9693), so the next split on that workspace can receive the PTY websocket instead of SSH and fail. Force the reusable attach path to request an SSH endpoint, or otherwise make the fallback decision sticky for the workspace.
Useful? React with 👍 / 👎.
Greptile SummaryNarrows the Freestyle
Confidence Score: 5/5The fallback narrowing is well-scoped: only the three specific error substrings trigger SSH, and the predicate correctly inspects both the ProviderError message and its cause. All three fallback paths (missing daemon, health-check non-200, fetch failure) are structurally correct and the predicate correctly handles the ProviderError+cause wrapping that production openWebSocketPty always produces. The test coverage gap does not affect production correctness. web/tests/vm-freestyle-provider.test.ts — the TestFreestyleProvider override throws plain Errors rather than ProviderError-wrapped ones, so the regression tests exercise a slightly different code path than production. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant openAttach
participant openWebSocketPty
participant ensureWSHealthy as ensureFreestyleWebSocketHealthy
participant shouldFallback as shouldFallbackAttachToSSH
participant openSSH
Caller->>openAttach: "openAttach(vmId, {requireDaemon})"
openAttach->>openWebSocketPty: openWebSocketPty(vmId)
openWebSocketPty->>ensureWSHealthy: fetch healthz (10s timeout)
alt fetch fails / non-200
ensureWSHealthy-->>openWebSocketPty: Error(health check failed/returned)
openWebSocketPty-->>openAttach: "ProviderError(cause=healthCheckError)"
openAttach->>shouldFallback: shouldFallbackAttachToSSH(err)
shouldFallback-->>openAttach: true
openAttach->>openSSH: openSSH(vmId)
openSSH-->>Caller: SSHEndpoint
else WebSocket ok, no daemon
openWebSocketPty-->>openAttach: WebSocketPtyEndpoint (no daemon)
openAttach->>openAttach: throw ProviderError(requires a cmuxd RPC endpoint)
openAttach->>shouldFallback: shouldFallbackAttachToSSH(err)
shouldFallback-->>openAttach: true
openAttach->>openSSH: openSSH(vmId)
openSSH-->>Caller: SSHEndpoint
else WebSocket ok, daemon present
openWebSocketPty-->>openAttach: WebSocketPtyEndpoint (with daemon)
openAttach-->>Caller: WebSocketPtyEndpoint
else unexpected error
openWebSocketPty-->>openAttach: "ProviderError(cause=401 error)"
openAttach->>shouldFallback: shouldFallbackAttachToSSH(err)
shouldFallback-->>openAttach: false
openAttach-->>Caller: throws ProviderError
end
Reviews (2): Last reviewed commit: "Handle VM attach health check timeouts" | Re-trigger Greptile |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 76b2d58. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81c5f355c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return messages.some((message) => | ||
| message.includes("requires a cmuxd RPC endpoint") | ||
| || message.includes("Freestyle cmuxd websocket health check returned") | ||
| || message.includes("fetch failed") | ||
| ); |
There was a problem hiding this comment.
Treat health-check timeouts as SSH-fallback eligible
When the cmuxd /healthz request hangs instead of returning a non-200 response, ensureFreestyleWebSocketHealthy aborts via AbortSignal.timeout(10_000), which rejects as TimeoutError: The operation timed out. in Bun rather than matching any of these substrings. In that common “attach endpoint unavailable” case, openAttach(..., { requireDaemon: true }) still throws instead of falling back to SSH, so the fallback added here only handles returned 5xx/network failures but not hung endpoints.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/tests/vm-freestyle-provider.test.ts`:
- Around line 55-65: The current test in the "falls back to SSH when the
WebSocket health check times out" test validates a raw Error message, but
production code receives a ProviderError with a cause property. Add a second
test case that wraps the websocket timeout error as a cause within a
ProviderError object to verify the fallback to SSH still works when the error is
wrapped. This ensures the actual production path through openAttach (which
receives ProviderError from openWebSocketPty) is properly tested and protected
by assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b099eee8-d693-4fc7-a7f1-3fb63b562e77
📒 Files selected for processing (2)
web/services/vms/drivers/freestyle.tsweb/tests/vm-freestyle-provider.test.ts
| test("falls back to SSH when the WebSocket health check times out", async () => { | ||
| const provider = new TestFreestyleProvider(); | ||
| provider.websocketResult = new Error( | ||
| "Freestyle cmuxd websocket health check failed: The operation was aborted", | ||
| ); | ||
|
|
||
| const endpoint = await provider.openAttach("vm-1", { requireDaemon: true }); | ||
|
|
||
| expect(endpoint).toEqual(sshEndpoint); | ||
| expect(provider.sshCalls).toBe(1); | ||
| }); |
There was a problem hiding this comment.
Add coverage for wrapped ProviderError.cause fallback path.
At Line 55, this test validates message matching on a raw Error, but production openAttach usually receives a ProviderError from openWebSocketPty and relies on inspecting cause. Please add one case that throws a wrapped ProviderError so the real branch is protected.
Proposed test addition
+import { ProviderError } from "../services/vms/drivers/types";
import type {
SSHEndpoint,
WebSocketPtyEndpoint,
} from "../services/vms/drivers/types";
@@
test("falls back to SSH when the WebSocket health check times out", async () => {
@@
expect(provider.sshCalls).toBe(1);
});
+
+ test("falls back to SSH when health check failure is wrapped in ProviderError cause", async () => {
+ const provider = new TestFreestyleProvider();
+ provider.websocketResult = new ProviderError(
+ "freestyle",
+ "openWebSocketPty(vm-1)",
+ new Error("Freestyle cmuxd websocket health check failed: The operation was aborted"),
+ );
+
+ const endpoint = await provider.openAttach("vm-1", { requireDaemon: true });
+
+ expect(endpoint).toEqual(sshEndpoint);
+ expect(provider.sshCalls).toBe(1);
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/tests/vm-freestyle-provider.test.ts` around lines 55 - 65, The current
test in the "falls back to SSH when the WebSocket health check times out" test
validates a raw Error message, but production code receives a ProviderError with
a cause property. Add a second test case that wraps the websocket timeout error
as a cause within a ProviderError object to verify the fallback to SSH still
works when the error is wrapped. This ensures the actual production path through
openAttach (which receives ProviderError from openWebSocketPty) is properly
tested and protected by assertions.
Summary
cmux vm newcreate idempotency immediately after create succeeds so attach failures do not poison the next createBenchmark
cmux vm new --detachagainst production: clean samples 2.9s, 9.8s, 2.6s, 2.1s, median 2.8snpx freestyle@latest vm create --snapshot sh-17agfasevrc18c8f15nn: clean samples 1.0s, 4.3s, 2.7s, median 2.7snpx freestyle@latest ... --jsoncurrently creates VMs but fails withConverting circular structure to JSONinfreestyle@0.1.63Verification
bun test tests/vm-freestyle-provider.test.tsbunx tsc --noEmit./scripts/reload-cloud.sh --tag vmnew/,/handler/sign-in,/handler/after-sign-inwith final HTTP 200CMUX_TAG=vmnew scripts/cmux-debug-cli.sh vm newopened an Ubuntu shell via SSH fallback; temporary VM and workspaces cleaned upProduction observation
cmux rpc vm.attach_info {"id":"99neulfwov26elo7oci6","require_daemon":true}still reproduces HTTP 502operation: openAttachbefore this PR is deployedNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes VM attach behavior and when short-lived SSH identities are minted; fallback is narrowed to specific failures but wrong classification could still attach over SSH or block valid paths.
Overview
Freestyle
openAttachno longer treatsrequireDaemon: trueas the only reason to try SSH after WebSocket attach fails. It now falls back only when the error looks recoverable—missing cmuxd RPC metadata, cmuxd/healthzfailures, orfetch failed—and rethrows everything else (e.g. API 401) without minting SSH credentials.The SSH fallback span records
cmux.vm.attach.require_daemonfor telemetry. HelpersshouldFallbackAttachToSSHanderrorMessageinspect the error andProviderErrorcause messages to decide eligibility.Adds
vm-freestyle-provider.test.tscovering health-check/missing-daemon fallback, happy-path WebSocket+daemon, and no-SSH on unexpected errors.Reviewed by Cursor Bugbot for commit 81c5f35. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Falls back to SSH for VM attach when the Freestyle WebSocket/RPC path is unhealthy or missing daemon metadata, including health-check timeouts, so required-daemon attaches don’t fail hard. Preserves VM create retry idempotency by clearing it only after a successful create, so a failed attach doesn’t affect the next create.
openAttach: SSH fallback only for specific errors (missing daemon metadata, cmuxd WebSocket health-check failures or timeouts); no fallback on unrelated errors (e.g., 401). Addscmux.vm.attach.require_daemonspan attribute.cmuxCLI: clear create idempotency immediately after create succeeds to preserve retry idempotency. Adds regression tests, including health-check timeout fallback.Written for commit 580bd15. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests