-
Notifications
You must be signed in to change notification settings - Fork 34
Websockets support #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Websockets support #162
Conversation
🦋 Changeset detectedLatest commit: 39f6dd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Claude Code ReviewWebSocket Support Implementation - Looks Good The PR successfully adds WebSocket support via the Issues Found1. Incomplete Error Handling in Test Worker (packages/sandbox/src/sandbox.ts:206-214)The WebSocket routing in the test worker lacks error handling for unknown paths: if (upgradeHeader?.toLowerCase() === 'websocket') {
if (url.pathname === '/ws/echo') {
return await connect(sandbox, request, 8080);
}
if (url.pathname === '/ws/code') {
return await connect(sandbox, request, 8081);
}
if (url.pathname === '/ws/terminal') {
return await connect(sandbox, request, 8082);
}
}Issue: If a WebSocket request hits an unrecognized path, it falls through to the end of the handler without returning a proper error response. Fix: Add a catch-all for unrecognized WebSocket paths: if (upgradeHeader?.toLowerCase() === 'websocket') {
if (url.pathname === '/ws/echo') return await connect(sandbox, request, 8080);
if (url.pathname === '/ws/code') return await connect(sandbox, request, 8081);
if (url.pathname === '/ws/terminal') return await connect(sandbox, request, 8082);
// Unknown WebSocket endpoint
return new Response('WebSocket endpoint not found', { status: 404 });
}2. Race Condition in E2E Tests (tests/e2e/websocket-connect.test.ts:46-47)await new Promise(resolve => setTimeout(resolve, 2000));Issue: Hard-coded 2-second sleep assumes server startup completes. On slower CI runners or during cold starts, this could cause flaky tests. Better approach: Poll the server health or add a readiness check: // Wait for server to be ready (with retry)
for (let i = 0; i < 10; i++) {
const processes = await fetch(`${workerUrl}/api/processes/list`, { headers }).then(r => r.json());
if (processes.processes?.some(p => p.id === 'ws-echo-8080' && p.status === 'running')) {
await new Promise(resolve => setTimeout(resolve, 500)); // Grace period
break;
}
await new Promise(resolve => setTimeout(resolve, 500));
}3. Container.fetch() Type Safety (packages/sandbox/src/sandbox.ts:321)return await super.fetch(request);The Suggestion: Enhance the comment: // WebSocket path: Let parent Container class handle WebSocket proxying
// This bypasses containerFetch() which uses JSRPC and cannot handle WebSocket upgrades.
// The Container base class proxies directly to the container HTTP server, preserving
// the WebSocket upgrade handshake that would be broken by JSRPC serialization.Minor Observations
Testing Coverage✅ Unit tests cover WebSocket detection logic VerdictApprove with minor improvements suggested. The WebSocket implementation is architecturally sound and well-tested. The identified issues are minor (test robustness and error handling) but should be addressed to prevent flaky tests and improve error visibility. |
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-162-b2c49a7Version: You can use this Docker image with the preview package from this PR. |
The examples/basic directory was primarily used for integration testing rather than as a standalone example. This moves it to tests/integration to reflect its actual purpose. Merged WebSocket example functionality into tests/integration since it shared the same testing infrastructure. Simplified WebSocket E2E tests by removing redundant test coverage and focusing on core transport-level functionality.
This reverts commit b915e85.
adds WebSocket support to the Sandbox SDK. we added connect(sandbox, request, port) to route client WebSocket connections directly to WebSocket servers in containers.
How it works:
Adding PR to docs repo for this: cloudflare/cloudflare-docs#26133