Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a WebSocket-to-TCP bridge in the DAP subsystem so remote debugging can connect via ws:///wss:// to the runner’s DAP server (e.g., through Dev Tunnels using GitHub auth), while the internal DAP server continues to speak DAP-over-TCP.
Changes:
- Add
WebSocketDapBridgeto translate WebSocket text frames ↔ DAP TCP (Content-Length framed) messages. - Update
DapDebuggerstartup/shutdown to expose the tunnel port via the WebSocket bridge and move the internal DAP listener to an ephemeral local port. - Add L0 coverage for the bridge and for DapDebugger WebSocket connectivity (including “pre-upgraded” WebSocket streams).
Show a summary per file
| File | Description |
|---|---|
| src/Runner.Worker/Dap/WebSocketDapBridge.cs | New bridge implementation handling upgrade detection, handshake, and bidirectional message pumping. |
| src/Runner.Worker/Dap/DapDebugger.cs | Starts/stops the bridge and uses an internal ephemeral TCP port for the actual DAP server when the bridge is enabled. |
| src/Test/L0/Worker/WebSocketDapBridgeL0.cs | New L0 tests validating forwarding, rejection paths, message size limits, and disposal behavior. |
| src/Test/L0/Worker/DapDebuggerL0.cs | Extends L0 tests to validate DapDebugger can accept DAP initialize via WebSocket and pre-upgraded WebSocket streams. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 7
| return null; | ||
| } | ||
|
|
||
| var webSocketKey = headers["Sec-WebSocket-Key"]; |
There was a problem hiding this comment.
Sec-WebSocket-Key like this is just for now right, cause at the moment its' untrusted right?
| if (messageStream.Length + result.Count > MaxInboundMessageSize) | ||
| { | ||
| Trace.Warning($"WebSocket message exceeds maximum allowed size of {MaxInboundMessageSize} bytes, closing connection"); | ||
| await source.CloseAsync( |
There was a problem hiding this comment.
Could we cancel sessionCts before initiating the close, or signal the session teardown path in HandleClientAsync to handle the graceful close instead?
| } | ||
|
|
||
| var prefixKind = ClassifyIncomingStreamPrefix(initialBytes); | ||
| if (prefixKind == IncomingStreamPrefixKind.PreUpgradedWebSocket) |
There was a problem hiding this comment.
this probs for dev tunnels right? jsut thinking something could easily skip e.g. by sending like 0x81 0x85 ..., maybe something like a flag allowPreUpgraded: true to Configure(), and the
bridge only accepts that byte-sniff path when the flag, that still has a limitation though if the relay is active but a bit more secure at least. I think tho if doing secrets or diff ports in the future it would address this anyway
|
|
||
| try | ||
| { | ||
| await completedTask; |
There was a problem hiding this comment.
can we skip the re-await of completedTask and go straight to Task.WhenAll after sessionCts.Cancel(), currently if completedTask faults with something other than OperationCanceledException (e.g., IOException from a peer disconnect), this rethrows and we never reach the Task.WhenAll below?
This adds a bridge converting messages to/from TCP <-> WS so that we can connect to the DAP server through
wss://directly and using GitHub credentials out of the box with Dev Tunnels.https://github.com/github/c2c-actions/issues/9831