fix(sdk): preserve tunnel half-close responses#996
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds box service-port tunneling across the API, proxy, runner, core runtime, C/Go/Node/Python SDKs, CLI, OpenAPI contracts, infrastructure, and end-to-end tests. It also updates preview URL encoding, forwarding headers, WebSocket path matching, and public-box defaults. ChangesNetwork tunneling and service previews
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicProxy
participant RunnerAPI
participant BoxNetwork
participant GuestService
Client->>PublicProxy: CONNECT preview host
PublicProxy->>RunnerAPI: CONNECT /network/tunnel?port=port
RunnerAPI->>BoxNetwork: Dial guest port
BoxNetwork->>GuestService: Open service connection
GuestService-->>BoxNetwork: Bidirectional stream
BoxNetwork-->>RunnerAPI: Tunnel stream
RunnerAPI-->>PublicProxy: 200 Connection Established
PublicProxy-->>Client: Relay tunnel bytes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
8246d7d to
e83feda
Compare
|
Does the current implementation establish a new runner-to-box connection for every HTTP request? Does each request require re-authentication? Is the user base characterized by low concurrency? Is the performance acceptable? |
G4614
changed the title
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/net/mod.rs (1)
651-673: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comments after removing
into_fd(). The test no longer recovers an fd, but lines 651–653 and the trailing line 672 still describe fd recoverability / "SDK fd-bridge relies on it," which no longer holds. Trim these so the test intent stays accurate.🤖 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 `@src/boxlite/src/net/mod.rs` around lines 651 - 673, Remove the stale fd-recovery and SDK fd-bridge claims from the test comments surrounding BoxInternalTunnel::from_local. Keep comments describing the Unix socketpair, bidirectional AsyncRead/AsyncWrite behavior, and peer handling, but delete the trailing comment about recovering an owned OS fd.
🧹 Nitpick comments (6)
src/cli/src/commands/tunnel.rs (1)
22-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNo test coverage for the new
tunnelcommand.
executehas no accompanying unit test in this batch (unlike the port-validation checks mirrored in the SDKtunnel()bindings, which do have tests). Consider adding at least a test for theport == 0rejection and theBoxEndpoint::UnixSocketerror path.🤖 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 `@src/cli/src/commands/tunnel.rs` around lines 22 - 50, Add unit tests for the tunnel command’s execute flow, covering port == 0 rejection and the BoxEndpoint::UnixSocket error path. Reuse existing CLI test patterns and fixtures/mocks around execute, asserting the validation error and remote REST profile error respectively without changing production behavior.sdks/python/tests/test_tunnel.py (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd comprehensive docstrings to public test functions.
As per coding guidelines, comprehensive docstrings must be written for all public functions and classes. While these are test functions, they are public and should include a brief docstring explaining the test's purpose.
sdks/python/tests/test_tunnel.py#L38-L38: Add a docstring totest_endpoint_returns_stable_unix_socket_path.sdks/python/tests/test_tunnel.py#L49-L49: Add a docstring totest_connect_opens_fresh_sockets.sdks/python/tests/test_tunnel.py#L73-L73: Add a docstring totest_tunnel_requires_a_started_box.sdks/python/tests/test_dev_tunnel_e2e.py#L136-L136: Add a docstring totest_local_box_tunnel_endpoint_and_repeated_connects.sdks/python/tests/test_dev_tunnel_e2e.py#L152-L152: Add a docstring totest_dev_cloud_tunnel_endpoint_and_repeated_connects.🤖 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 `@sdks/python/tests/test_tunnel.py` at line 38, Add brief purpose-focused docstrings to test_endpoint_returns_stable_unix_socket_path, test_connect_opens_fresh_sockets, and test_tunnel_requires_a_started_box in sdks/python/tests/test_tunnel.py at lines 38-38, 49-49, and 73-73, respectively. Also document test_local_box_tunnel_endpoint_and_repeated_connects and test_dev_cloud_tunnel_endpoint_and_repeated_connects in sdks/python/tests/test_dev_tunnel_e2e.py at lines 136-136 and 152-152, describing the behavior each test verifies.Source: Coding guidelines
sdks/python/boxlite/simplebox.py (1)
216-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant attribute check.
Since
self._networkis unconditionally initialized in__init__(line 138), thehasattrcheck here is redundant and can be simplified.♻️ Proposed refactor
`@property` def network(self) -> _SimpleBoxNetwork: """Get the box-scoped network handle.""" - if not hasattr(self, "_network"): - self._network = _SimpleBoxNetwork(self) return self._network🤖 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 `@sdks/python/boxlite/simplebox.py` around lines 216 - 222, Update the network property to return the already-initialized self._network directly, removing the redundant hasattr check and lazy _SimpleBoxNetwork construction while preserving the existing _SimpleBoxNetwork handle.sdks/python/boxlite/sync_api/_network.py (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type hints for
connectandendpointreturn values.To match the
BoxTunnelinterface insimplebox.py, consider adding explicit return type hints (socket.socketandstr) to these methods.Add the
socketimport at the top of the file:import socket♻️ Proposed refactor
- def connect(self): + def connect(self) -> socket.socket: """Open a blocking socket to the target service.""" tunnel = self._box._sync(self._tunnel.connect()) tunnel.setblocking(True) return tunnel - def endpoint(self): + def endpoint(self) -> str: """Return the cloud URL or local Unix socket path.""" return self._box._sync(self._tunnel.endpoint())🤖 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 `@sdks/python/boxlite/sync_api/_network.py` around lines 18 - 26, Update the synchronous tunnel methods connect and endpoint with explicit return annotations of socket.socket and str, respectively, and import socket in the module so the annotations resolve.apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (1)
195-208: 🚀 Performance & Scalability | 🔵 TrivialRe: reviewer question on per-request runner connections/re-auth.
Confirming from this code: yes — every octet-stream tunnel request flowing through
proxyNetworkTunnel→proxyToRunnerperforms a freshboxService.findOneByIdOrNameDB lookup, a freshrunnerService.findOne, and instantiates a brand-newcreateProxyMiddleware(...)(with its own outgoing connection to the runner) per HTTP request. This mirrors the pre-existing pattern used byproxyExec/proxyFiles/proxyMetrics, so it isn't a regression from this PR, but it does mean concurrency/performance for guest-port tunneling is bounded by this per-request overhead (DB round-trip + new outbound connection) rather than a persistently reused runner connection. Worth keeping in mind if/when the expected request rate per tunnel grows, since (unlike the WS proxy inboxlite-ws-proxy.service.ts, which builds onecreateProxyMiddlewareinstance in the constructor) this path builds a new middleware instance on every call.Also applies to: 242-290
🤖 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 `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts` around lines 195 - 208, Review the per-request connection behavior in proxyNetworkTunnel and proxyToRunner, including the related logic around the alternate tunnel path, and avoid creating a fresh createProxyMiddleware instance and runner lookup for every streaming request. Reuse a persistent proxy middleware/runner connection pattern comparable to boxlite-ws-proxy.service.ts while preserving authentication, request routing, and octet-stream validation.apps/runner/pkg/api/controllers/proxy.go (1)
263-264: 🚀 Performance & Scalability | 🔵 TrivialPer-request transport prevents connection reuse.
NewGuestPortTransportbuilds a fresh transport that dials a new guest tunnel per request, anddefer transport.CloseIdleConnections()discards any keep-alive connection once the handler returns, so nothing is reused across requests. This is fine for the expected low-concurrency preview traffic called out in the PR discussion, but if request volume grows, consider caching a transport per(boxId, port)to amortize tunnel setup.🤖 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 `@apps/runner/pkg/api/controllers/proxy.go` around lines 263 - 264, Update the proxy handler around NewGuestPortTransport to reuse transports across requests by caching them per (boxId, port) pair instead of creating and closing one for every request. Ensure cached transports remain available for connection reuse and are safely managed when boxes or ports are no longer needed.
🤖 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 `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 182-212: Update proxyNetworkTunnel to await this.startHint(boxId,
authContext) before either the streaming proxyToRunner path or the
getPortPreviewUrl path, matching the startup handling in proxyExec, proxyFiles,
and proxyMetrics. Ensure the notification occurs after validating the port and
before any guest-facing request can autostart the box.
In `@sdks/python/boxlite/sync_api/_network.py`:
- Around line 35-38: Update SyncBoxTunnel's tunnel method to validate port
before calling _owner._create_tunnel, ensuring it is a valid integer port within
the supported range and raising the established input-validation error for
invalid values. Preserve the existing tunnel creation and SyncBoxTunnel return
flow for valid ports, matching SimpleBox._create_tunnel behavior.
In `@sdks/python/boxlite/sync_api/_simplebox.py`:
- Around line 258-264: Update the docstring of SyncSimpleBox.tunnel to describe
returning a lazy SyncBoxTunnel handle, not immediately opening a socket; mention
that callers must explicitly invoke .connect() to obtain the socket, while
leaving the method behavior unchanged.
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 75-90: Update the listener accept loop around
listener.accept().await so transient accept errors are logged and the loop
continues instead of breaking and terminating the accept task. Preserve the
existing client-handling spawn and tunnel error logging, while reserving loop
termination only for explicitly unrecoverable listener states if such handling
already exists.
---
Outside diff comments:
In `@src/boxlite/src/net/mod.rs`:
- Around line 651-673: Remove the stale fd-recovery and SDK fd-bridge claims
from the test comments surrounding BoxInternalTunnel::from_local. Keep comments
describing the Unix socketpair, bidirectional AsyncRead/AsyncWrite behavior, and
peer handling, but delete the trailing comment about recovering an owned OS fd.
---
Nitpick comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 195-208: Review the per-request connection behavior in
proxyNetworkTunnel and proxyToRunner, including the related logic around the
alternate tunnel path, and avoid creating a fresh createProxyMiddleware instance
and runner lookup for every streaming request. Reuse a persistent proxy
middleware/runner connection pattern comparable to boxlite-ws-proxy.service.ts
while preserving authentication, request routing, and octet-stream validation.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 263-264: Update the proxy handler around NewGuestPortTransport to
reuse transports across requests by caching them per (boxId, port) pair instead
of creating and closing one for every request. Ensure cached transports remain
available for connection reuse and are safely managed when boxes or ports are no
longer needed.
In `@sdks/python/boxlite/simplebox.py`:
- Around line 216-222: Update the network property to return the
already-initialized self._network directly, removing the redundant hasattr check
and lazy _SimpleBoxNetwork construction while preserving the existing
_SimpleBoxNetwork handle.
In `@sdks/python/boxlite/sync_api/_network.py`:
- Around line 18-26: Update the synchronous tunnel methods connect and endpoint
with explicit return annotations of socket.socket and str, respectively, and
import socket in the module so the annotations resolve.
In `@sdks/python/tests/test_tunnel.py`:
- Line 38: Add brief purpose-focused docstrings to
test_endpoint_returns_stable_unix_socket_path, test_connect_opens_fresh_sockets,
and test_tunnel_requires_a_started_box in sdks/python/tests/test_tunnel.py at
lines 38-38, 49-49, and 73-73, respectively. Also document
test_local_box_tunnel_endpoint_and_repeated_connects and
test_dev_cloud_tunnel_endpoint_and_repeated_connects in
sdks/python/tests/test_dev_tunnel_e2e.py at lines 136-136 and 152-152,
describing the behavior each test verifies.
In `@src/cli/src/commands/tunnel.rs`:
- Around line 22-50: Add unit tests for the tunnel command’s execute flow,
covering port == 0 rejection and the BoxEndpoint::UnixSocket error path. Reuse
existing CLI test patterns and fixtures/mocks around execute, asserting the
validation error and remote REST profile error respectively without changing
production behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56686a1b-9c8c-4da7-9b22-b8e5d3ba672d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/api/src/boxlite-rest/boxlite-rest-routing.spec.tsapps/api/src/boxlite-rest/boxlite-ws-proxy.service.tsapps/api/src/main.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/infra/sst.config.tsapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_integration_test.goapps/runner/pkg/api/controllers/proxy_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/guest_port_tunnel.goopenapi/box.openapi.yamlscripts/build/build-runtime.shscripts/test/e2e/cases/test_node_tunnel.pyscripts/test/e2e/cases/test_sdk_tunnel.pyscripts/test/e2e/sdks/node/e2e_tunnel.tssdks/c/Cargo.tomlsdks/c/include/boxlite.hsdks/c/src/lib.rssdks/c/src/network.rssdks/c/src/tests.rssdks/go/constants.gosdks/go/tunnel.gosdks/node/Cargo.tomlsdks/node/lib/index.tssdks/node/lib/native-contracts.tssdks/node/lib/simplebox.tssdks/node/src/box_handle.rssdks/node/src/lib.rssdks/node/src/network.rssdks/node/tests/tunnel.test.tssdks/python/Cargo.tomlsdks/python/boxlite/__init__.pysdks/python/boxlite/simplebox.pysdks/python/boxlite/sync_api/__init__.pysdks/python/boxlite/sync_api/_box.pysdks/python/boxlite/sync_api/_network.pysdks/python/boxlite/sync_api/_simplebox.pysdks/python/pytest.inisdks/python/src/box_handle.rssdks/python/src/lib.rssdks/python/src/network.rssdks/python/tests/test_dev_tunnel_e2e.pysdks/python/tests/test_tunnel.pysrc/boxlite/Cargo.tomlsrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/network.rssrc/boxlite/src/net/gvproxy/services.rssrc/boxlite/src/net/mod.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/runtime.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/tests/gvproxy_backend.rssrc/cli/src/cli.rssrc/cli/src/commands/mod.rssrc/cli/src/commands/tunnel.rssrc/cli/src/main.rs
💤 Files with no reviewable changes (2)
- src/boxlite/src/net/gvproxy/services.rs
- src/boxlite/tests/gvproxy_backend.rs
G4614
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (1)
186-205:⚠️ Potential issue | 🟠 MajorMissing
startHintautostart notification.
proxyNetworkTunnelis missing thestartHintautostart notification. Every other guest-facing proxy method (proxyExec,proxyFiles,proxyMetrics) callsawait this.startHint(boxId, authContext)before proxying. WithoutstartHint, tunneling into a guest port on a stopped box will trigger an auto-start/re-stop race because the control plane'ssync-stateswill promptly stop the box when it seesdesiredState=STOPPED.🐛 Proposed fix
if (port < 1 || port > 65535) { return res.status(400).json({ error: 'port must be between 1 and 65535' }) } + await this.startHint(boxId, authContext) + const { url: uri } = await this.boxService.getPortPreviewUrl(boxId, authContext.organizationId, port)🤖 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 `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts` around lines 186 - 205, Update proxyNetworkTunnel to await this.startHint(boxId, authContext) before requesting the preview URL or issuing the tunnel ticket, matching the existing proxyExec, proxyFiles, and proxyMetrics flows so stopped boxes receive the autostart notification first.
🧹 Nitpick comments (2)
openapi/box.openapi.yaml (1)
1298-1313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
snake_casefor API response properties.The
BoxServiceEndpointschema introducesconnectUriincamelCase, which is inconsistent with thesnake_caseconvention used across the rest of the API (e.g.,box_id,created_at,disk_size_gb).
openapi/box.openapi.yaml#L1298-L1313: RenameconnectUritoconnect_uri.apps/api/src/boxlite-rest/boxlite-proxy.controller.ts#L186-L205: Returnconnect_uriinstead ofconnectUriin the JSON response payload.apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts#L78-L99: Update the test assertion to expectconnect_uri.🤖 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 `@openapi/box.openapi.yaml` around lines 1298 - 1313, Rename the BoxServiceEndpoint schema property from connectUri to connect_uri in openapi/box.openapi.yaml (1298-1313), update the boxlite-proxy controller response to return connect_uri in apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (186-205), and change the corresponding assertion in apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts (78-99) to expect connect_uri.apps/runner/pkg/api/controllers/proxy_test.go (1)
19-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify
proxyBidirectionalStreamwaits for both directions to finish.The test runs
proxyBidirectionalStreamviagoand never synchronizes on its return, so it can't detect the single-<-done-receive bug (seeproxy.golines 159-171): the function could return well beforeguest's pong is actually delivered and the test would still pass. Consider running it synchronously (or with a completion channel) and asserting it returns only after both writes/reads complete.🤖 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 `@apps/runner/pkg/api/controllers/proxy_test.go` around lines 19 - 46, The test currently does not synchronize with proxyBidirectionalStream, so it cannot verify completion of both relay directions. Update TestProxyBidirectionalStreamRelaysBothDirections to use a completion channel or equivalent synchronization, wait for proxyBidirectionalStream to return after both ping and pong exchanges, and assert completion occurs only after both writes and reads finish.
🤖 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 `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 200-204: Update the http.Server initialization in the proxy setup
to add a suitable ReadTimeout alongside ReadHeaderTimeout, so routed non-CONNECT
requests have an overall read deadline. Keep the existing Addr, Handler, and
header-timeout configuration unchanged.
In `@apps/proxy/pkg/proxy/tunnel.go`:
- Around line 94-98: Update the TLS configuration in the HTTPS branch of the
tunnel connection logic to explicitly set MinVersion to tls.VersionTLS12 (or the
project’s approved TLS 1.3 minimum) when constructing tls.Config. Keep the
existing ServerName behavior and non-HTTPS dialing path unchanged.
- Around line 130-137: Add a CloseWrite method to bufferedConn that detects
whether the embedded net.Conn supports CloseWrite() error and forwards the call,
returning the underlying result; otherwise return an appropriate
unsupported-operation error. This ensures proxyTunnelStreams can detect and
half-close buffered connections.
- Around line 78-101: Update dialRunnerTunnel to enforce an explicit bounded
timeout for the runner-tunnel connection and TLS handshake, rather than relying
solely on the incoming request context. Configure the net.Dialer used by both
plain and TLS dialing with the established tunnel setup timeout, preserving
context cancellation while ensuring unreachable or slow runners cannot block
indefinitely.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 159-171: Both bidirectional tunnel helpers return after only one
copy direction completes. In apps/runner/pkg/api/controllers/proxy.go lines
159-171, update proxyBidirectionalStream to wait for both done signals before
returning; apply the identical change in apps/proxy/pkg/proxy/tunnel.go lines
139-151 for proxyTunnelStreams. Preserve the existing two-goroutine streaming
behavior and deferred connection cleanup.
---
Duplicate comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 186-205: Update proxyNetworkTunnel to await this.startHint(boxId,
authContext) before requesting the preview URL or issuing the tunnel ticket,
matching the existing proxyExec, proxyFiles, and proxyMetrics flows so stopped
boxes receive the autostart notification first.
---
Nitpick comments:
In `@apps/runner/pkg/api/controllers/proxy_test.go`:
- Around line 19-46: The test currently does not synchronize with
proxyBidirectionalStream, so it cannot verify completion of both relay
directions. Update TestProxyBidirectionalStreamRelaysBothDirections to use a
completion channel or equivalent synchronization, wait for
proxyBidirectionalStream to return after both ping and pong exchanges, and
assert completion occurs only after both writes and reads finish.
In `@openapi/box.openapi.yaml`:
- Around line 1298-1313: Rename the BoxServiceEndpoint schema property from
connectUri to connect_uri in openapi/box.openapi.yaml (1298-1313), update the
boxlite-proxy controller response to return connect_uri in
apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (186-205), and change the
corresponding assertion in
apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts (78-99) to expect
connect_uri.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d7ebc8b-d917-4dde-a7e8-9664c51c7fa8
📒 Files selected for processing (25)
apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/api/src/boxlite-rest/boxlite-rest.module.tsapps/api/src/boxlite-rest/boxlite-tunnel-ticket.service.spec.tsapps/api/src/boxlite-rest/boxlite-tunnel-ticket.service.tsapps/api/src/config/configuration.tsapps/common-go/pkg/cache/redis_cache.goapps/infra/sst.config.tsapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_test.goapps/runner/pkg/api/server.goopenapi/box.openapi.yamlsdks/c/include/boxlite.hsdks/c/src/network.rssdks/go/tunnel.gosdks/node/src/network.rssdks/python/src/network.rssrc/boxlite/Cargo.tomlsrc/boxlite/src/litebox/network.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/cli/src/commands/tunnel.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/runner/pkg/api/server.go
- src/cli/src/commands/tunnel.rs
- src/boxlite/Cargo.toml
- src/boxlite/src/rest/litebox.rs
- sdks/node/src/network.rs
- sdks/c/include/boxlite.h
- sdks/c/src/network.rs
- sdks/go/tunnel.go
- sdks/python/src/network.rs
- src/boxlite/src/litebox/network.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/proxy/pkg/proxy/proxy.go (1)
103-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize
tunnelTicketswhen Redis is disabled.The new cache is assigned only in the
config.Redis != nilbranch, whileconnectAwareHandlerroutes CONNECT requests in both modes. In a no-Redis configuration, tunnel handling can therefore dereference a nil cache or reject every ticket.} else { proxy.boxRunnerCache = common_cache.NewMapCache[RunnerInfo](ctx) proxy.runnerCache = common_cache.NewMapCache[RunnerInfo](ctx) proxy.boxPublicCache = common_cache.NewMapCache[bool](ctx) proxy.boxAuthKeyValidCache = common_cache.NewMapCache[bool](ctx) proxy.boxLastActivityUpdateCache = common_cache.NewMapCache[bool](ctx) + proxy.tunnelTickets = common_cache.NewMapCache[TunnelTicket](ctx) }🤖 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 `@apps/proxy/pkg/proxy/proxy.go` around lines 103 - 113, Initialize proxy.tunnelTickets in the config.Redis == nil branch using the in-memory cache implementation, alongside the other MapCache fields. Ensure connectAwareHandler can access a non-nil tunnelTickets cache in both Redis-enabled and no-Redis configurations.
♻️ Duplicate comments (2)
apps/proxy/pkg/proxy/proxy.go (1)
200-203: 🩺 Stability & Availability | 🟠 MajorAdd read deadlines to the HTTP server.
This remains unresolved from the previous review: the server has neither
ReadHeaderTimeoutnorReadTimeout. Non-CONNECT requests delegated toroutercan hold connections open with slow request bodies, exhausting proxy resources. Choose values compatible with supported request sizes and CONNECT handshakes.🤖 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 `@apps/proxy/pkg/proxy/proxy.go` around lines 200 - 203, Update the http.Server initialization in the proxy handler setup to configure both ReadHeaderTimeout and ReadTimeout with finite values. Choose timeouts that allow supported request sizes and CONNECT handshakes while preventing slow non-CONNECT requests routed through router from holding connections indefinitely.Source: Linters/SAST tools
apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (1)
186-204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
proxyNetworkTunnelis missing thestartHintautostart notification.Every other guest-facing proxy method (
proxyExec,proxyFiles,proxyMetrics) callsawait this.startHint(boxId, authContext)before proxying, because a proxied call into a stopped box's runtime auto-starts the VM (Box::live_state()) without the control-plane DB knowing — withoutstartHint,sync-stateswill seedesiredState=STOPPEDand re-stop the box shortly after tunnel traffic arrives.proxyNetworkTunnelskips this call, so tunneling into a guest port on a stopped box is likely to trigger the exact auto-start/re-stop race theensureStartedForProxydocstring describes.🐛 Proposed fix
if (port < 1 || port > 65535) { return res.status(400).json({ error: 'port must be between 1 and 65535' }) } + await this.startHint(boxId, authContext) + const { url: uri } = await this.boxService.getPortPreviewUrl(boxId, authContext.organizationId, port)🤖 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 `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts` around lines 186 - 204, Update proxyNetworkTunnel to await startHint(boxId, authContext) before requesting the port preview URL or issuing the tunnel ticket, matching the startup notification flow used by proxyExec, proxyFiles, and proxyMetrics.
🧹 Nitpick comments (2)
sdks/python/boxlite/sync_api/_simplebox.py (1)
258-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public networking API contract completely.
The lazy-tunnel wording is now correct, but both public APIs should document their arguments/return values and the
RuntimeErrorraised before startup, as required by the SDK guidelines.📝 Suggested documentation
def tunnel(self, port: int): - """Return a lazy tunnel handle; call ``connect()`` to open its socket.""" + """Return a lazy tunnel handle for a guest port. + + Args: + port: Guest port to expose. + + Returns: + A tunnel handle; call ``connect()`` to open its socket. + + Raises: + RuntimeError: If the box has not been started. + """ `@property` def network(self): - """Get the box-scoped network handle.""" + """Return the box-scoped network handle. + + Raises: + RuntimeError: If the box has not been started. + """🤖 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 `@sdks/python/boxlite/sync_api/_simplebox.py` around lines 258 - 273, Complete the docstrings for the public SyncSimpleBox.tunnel and SyncSimpleBox.network APIs, documenting the tunnel port argument, returned handle/value, and the RuntimeError raised when the box has not started. Preserve the existing lazy tunnel behavior and startup validation.Source: Coding guidelines
src/boxlite/src/rest/client.rs (1)
346-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTower
Service::callrequirespoll_ready.Calling
.call()directly on atower::Servicewithout first ensuring.poll_ready()returnsReadyviolates the Tower service contract. Althoughhyper_rustls::HttpsConnectorhappens to always be ready without blocking, it's safer and more idiomatic to usetower::ServiceExt::oneshot.🛠️ Proposed fix
Import
ServiceExtat the use-site and apply.oneshot():- let io = tokio::time::timeout(TUNNEL_SETUP_TIMEOUT, connector.call(uri.clone())) + use tower::ServiceExt; + let io = tokio::time::timeout(TUNNEL_SETUP_TIMEOUT, connector.oneshot(uri.clone()))🤖 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 `@src/boxlite/src/rest/client.rs` at line 346, Update the tunnel setup flow around the connector service to import and use Tower’s ServiceExt::oneshot instead of calling connector.call directly, ensuring readiness is polled before dispatch while preserving the existing timeout and URI handling.
🤖 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 `@apps/proxy/pkg/proxy/proxy.go`:
- Line 202: Update the CONNECT handling around proxy.handleTunnelConnect and
connectAwareHandler so established hijacked tunnels are registered with
shutdownWg before being served and released when they close, ensuring graceful
shutdown waits for active CONNECT streams. Preserve the existing handler routing
while covering all CONNECT tunnel lifetimes.
---
Outside diff comments:
In `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 103-113: Initialize proxy.tunnelTickets in the config.Redis == nil
branch using the in-memory cache implementation, alongside the other MapCache
fields. Ensure connectAwareHandler can access a non-nil tunnelTickets cache in
both Redis-enabled and no-Redis configurations.
---
Duplicate comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 186-204: Update proxyNetworkTunnel to await startHint(boxId,
authContext) before requesting the port preview URL or issuing the tunnel
ticket, matching the startup notification flow used by proxyExec, proxyFiles,
and proxyMetrics.
In `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 200-203: Update the http.Server initialization in the proxy
handler setup to configure both ReadHeaderTimeout and ReadTimeout with finite
values. Choose timeouts that allow supported request sizes and CONNECT
handshakes while preventing slow non-CONNECT requests routed through router from
holding connections indefinitely.
---
Nitpick comments:
In `@sdks/python/boxlite/sync_api/_simplebox.py`:
- Around line 258-273: Complete the docstrings for the public
SyncSimpleBox.tunnel and SyncSimpleBox.network APIs, documenting the tunnel port
argument, returned handle/value, and the RuntimeError raised when the box has
not started. Preserve the existing lazy tunnel behavior and startup validation.
In `@src/boxlite/src/rest/client.rs`:
- Line 346: Update the tunnel setup flow around the connector service to import
and use Tower’s ServiceExt::oneshot instead of calling connector.call directly,
ensuring readiness is polled before dispatch while preserving the existing
timeout and URI handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e5dd83d-bc5d-41b0-a7e8-ece7c7072532
📒 Files selected for processing (14)
apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_test.goopenapi/box.openapi.yamlsdks/python/boxlite/sync_api/_network.pysdks/python/boxlite/sync_api/_simplebox.pysdks/python/tests/test_tunnel.pysrc/boxlite/Cargo.tomlsrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/rest/client.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/proxy/pkg/proxy/tunnel_test.go
- apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts
- sdks/python/boxlite/sync_api/_network.py
- openapi/box.openapi.yaml
- src/boxlite/src/litebox/box_impl.rs
- apps/proxy/pkg/proxy/tunnel.go
- src/boxlite/Cargo.toml
- apps/runner/pkg/api/controllers/proxy_test.go
- apps/runner/pkg/api/controllers/proxy.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/proxy/pkg/proxy/tunnel.go (1)
117-123: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPath is stripped from
CONNECTrequests, causing tunnel failure.Go's
http.Request.Writemethod specifically handles theCONNECTmethod by usingreq.URL.Hostas the Request-URI and completely ignoring the path. Becausereqis created with"http://"+host+path,req.URL.Hostis populated, andreq.Write(conn)will outputCONNECT {host} HTTP/1.1. The runner will never receive the/v1/boxes/...path and will likely return a 404 Not Found.To preserve the path in the Request-URI while maintaining the
Hostheader, initialize the request with just thepath.🐛 Proposed fix
- path := fmt.Sprintf("/v1/boxes/%s/network/tunnel?port=%d", url.PathEscape(boxID), port) - req, err := http.NewRequestWithContext(ctx, http.MethodConnect, "http://"+host+path, nil) + path := fmt.Sprintf("/v1/boxes/%s/network/tunnel?port=%d", url.PathEscape(boxID), port) + req, err := http.NewRequestWithContext(ctx, http.MethodConnect, path, nil) if err != nil { conn.Close() return nil, err } req.Host = host🤖 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 `@apps/proxy/pkg/proxy/tunnel.go` around lines 117 - 123, Update the CONNECT request construction near req.Host so http.NewRequestWithContext receives only path rather than "http://"+host+path, then retain host in req.Host. Preserve the existing context, method, error cleanup, and escaped tunnel path so req.Write sends the full path as the Request-URI.
🤖 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 `@apps/proxy/pkg/proxy/tunnel.go`:
- Around line 28-37: Fix handleTunnelConnect and tunnelTarget so a plain box ID
from the Host header cannot authorize a tunnel: require and validate a securely
signed preview token or restored Proxy-Authorization/ticket credential, reject
decoding or validation failures, and verify the requested port is marked public
before calling getBoxRunnerInfo or dialing the runner.
- Line 63: Update the tunnel setup around proxyTunnelStreams to wrap clientConn
with bufferedConn, matching the existing runner connection handling, and pass
the wrapped client connection into proxyTunnelStreams so bytes already held by
hijacker.Hijack’s buffered.Reader are forwarded.
---
Outside diff comments:
In `@apps/proxy/pkg/proxy/tunnel.go`:
- Around line 117-123: Update the CONNECT request construction near req.Host so
http.NewRequestWithContext receives only path rather than "http://"+host+path,
then retain host in req.Host. Preserve the existing context, method, error
cleanup, and escaped tunnel path so req.Write sends the full path as the
Request-URI.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18693219-6bf9-4a71-8f2e-fb66e2643ec1
📒 Files selected for processing (9)
apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/infra/sst.config.tsapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goopenapi/box.openapi.yamlsrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rs
💤 Files with no reviewable changes (2)
- apps/proxy/pkg/proxy/proxy.go
- apps/infra/sst.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/boxlite/src/rest/litebox.rs
Pull request was closed
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
894039b to
8f50bcd
Compare
8f50bcd to
75751a9
Compare
75751a9 to
b1ba38a
Compare
1182eb8 to
ae6a01e
Compare
G4614
changed the title
2 participants
Moved the half-close fix into #985 so this PR no longer carries a separate change.
Test plan: