Ws transport #253
Ws transport #253
Conversation
|
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-253-be15028Version: You can use this Docker image with the preview package from this PR. |
Documents the new WebSocket transport feature that enables multiplexing SDK operations over a single WebSocket connection to reduce sub-request count in Workers. Changes: - Add comprehensive WebSocket transport concept page - Document useWebSocket option in sandbox configuration - Update architecture overview to mention transport options Related PR: cloudflare/sandbox-sdk#253 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This PR adds WebSocket transport support to enable multiplexing HTTP requests over a single connection, reducing sub-request count in Workers/DOs. The architecture is sound and integrates cleanly with existing patterns, but several issues need addressing.
Critical Issues
1. package-lock.json Changes
Extensive unrelated dependency modifications (30+ packages with peer flag changes). This appears to be an accidental lockfile regeneration. Please verify these changes are intentional or revert and regenerate with only WebSocket-related dependencies.
Important Issues
2. SSE Parsing Has Data Loss Bug (packages/sandbox-container/src/handlers/ws-handler.ts:267-305)
The parseSSEEvents() method uses buffer.indexOf(line, processedIndex) where line === '', which returns processedIndex instead of the actual newline position. This causes incorrect buffer trimming and potential data loss. The parser also doesn't handle: multiple data lines, comment lines, or SSE spec fields like id: and retry:.
3. Module-Level Mutable State (packages/sandbox-container/src/index.ts:16-17)
wsHandler is initialized after createApplication() but used in serve() config. While technically safe, this pattern is fragile. Move WebSocket handler into the application context return value to eliminate module-level mutation.
4. Unsafe Type Casting (packages/sandbox-container/src/index.ts:78, 82, 88)
Using as unknown as ServerWebSocket<WSData> bypasses type safety. Define proper type interfaces that match Bun's actual types instead of forcing casts.
5. Missing Error Handling for Send Failures (packages/sandbox-container/src/handlers/ws-handler.ts:310-319)
When ws.send() fails, the error is logged but the connection stays open, causing hung requests on the client side. Close the connection on send failure and return a status indicator.
Strengths
- Clean integration with existing Router/middleware patterns
- Proper logger injection via constructor
- Comprehensive test coverage (unit + E2E)
- Type definitions properly shared via @repo/shared
- Protocol design is sound
Verdict
Needs fixes before merge. The SSE parsing bug and package-lock.json changes are the most critical. The architecture is solid once these issues are addressed.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
| const line = lines[i]; | ||
|
|
||
| // Check if we have a complete event (empty line after data) | ||
| if (line === '' && currentEvent.data.length > 0) { |
There was a problem hiding this comment.
Bug: SSE parsing has data loss issue
buffer.indexOf(line, processedIndex) where line === '' returns processedIndex, not the actual newline position. This breaks buffer tracking.
Recommend rewriting to use buffer.split(/\n\n/) to find complete events, process the complete parts, and keep the last incomplete part as remaining.
| const app = await createApplication(); | ||
|
|
||
| // Initialize WebSocket handler with the router | ||
| wsHandler = new WebSocketHandler(app.router, logger); |
There was a problem hiding this comment.
Issue: Module-level mutable state
wsHandler is initialized here but declared at module level (line 16). This creates fragile initialization order. Consider moving the handler into the createApplication() return value:
return {
fetch: ...,
websocketHandlers: {
open: (ws) => wsHandler.onOpen(ws),
close: (ws, code, reason) => wsHandler.onClose(ws, code, reason),
message: (ws, msg) => wsHandler.onMessage(ws, msg)
}
};| async message() { | ||
| // WebSocket functionality can be added here in the future | ||
| open(ws) { | ||
| wsHandler?.onOpen(ws as unknown as ServerWebSocket<WSData>); |
There was a problem hiding this comment.
Issue: Unsafe type casting
as unknown as ServerWebSocket<WSData> bypasses TypeScript's type safety. Define a proper type interface that matches Bun's actual WebSocket type instead of forcing the cast.
| */ | ||
| private send(ws: ServerWebSocket<WSData>, message: WSServerMessage): void { | ||
| try { | ||
| ws.send(JSON.stringify(message)); |
There was a problem hiding this comment.
Issue: Missing error handling for send failures
When ws.send() fails, the error is logged but the connection remains open. This causes hung requests on the client. Recommend:
- Close the connection on send failure
- Return a boolean status so callers know the send failed
- Abort further processing for that request
| "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", | ||
| "license": "MIT", | ||
| "peer": true, | ||
| "engines": { |
There was a problem hiding this comment.
Critical: Unrelated lockfile changes
This PR modifies 30+ unrelated dependencies (removing/adding "peer": true flags). This suggests an accidental npm install regenerated the lockfile. Please verify these changes are intentional or revert and only include WebSocket-related dependency changes.
1 participant
No description provided.