fix(sdk): remove REST tunnel bridge copy#985
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:
📝 WalkthroughWalkthroughThe tunnel client now returns the upgraded HTTP connection through ChangesTunnel connection flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ApiClient
participant TunnelTestServer
participant TokioIoUpgraded
ApiClient->>TunnelTestServer: Send CONNECT request
TunnelTestServer-->>ApiClient: Return upgraded connection
ApiClient->>TokioIoUpgraded: Write ping and shut down writer
TokioIoUpgraded->>TunnelTestServer: Forward ping and EOF
TunnelTestServer-->>TokioIoUpgraded: Return pong
TokioIoUpgraded-->>ApiClient: Read pong
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Closing this draft because POL-103 is an API-layer semantics/documentation question, not a code bug to fix in this form. |
b17470c to
49472db
Compare
49472db to
2d8a476
Compare
2d8a476 to
718ea19
Compare
7d92b55 to
55823e9
Compare
📦 BoxLite review — couldn't completepowered by BoxLite |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/boxlite/src/rest/client.rs (1)
358-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the upgraded-stream ownership and half-close contract.
This crate-visible API now exposes the raw upgraded I/O directly, but provides no usage contract for callers.
Proposed documentation
+ /// Establish an HTTP CONNECT tunnel to `uri`. + /// + /// Returns the upgraded bidirectional I/O stream. Callers own the stream + /// and may shut down its write half while continuing to read peer data. pub(crate) async fn connect_box_network_tunnel(As per coding guidelines, “Write comprehensive docstrings for all public functions and classes.”
🤖 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` around lines 358 - 361, Document connect_box_network_tunnel, including that it returns ownership of the upgraded raw I/O stream and callers are responsible for its lifecycle; specify the expected half-close behavior and any required shutdown handling when either side finishes communicating.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/boxlite/src/rest/client.rs`:
- Around line 358-361: Document connect_box_network_tunnel, including that it
returns ownership of the upgraded raw I/O stream and callers are responsible for
its lifecycle; specify the expected half-close behavior and any required
shutdown handling when either side finishes communicating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7142f45-7817-473c-ae80-b0c6761ecca8
📒 Files selected for processing (2)
sdks/go/tunnel.gosrc/boxlite/src/rest/client.rs
7ef7474 to
10a8b58
Compare
10a8b58 to
2ca233e
Compare
2ca233e to
889729e
Compare
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 `@scripts/test/e2e/cases/test_node_tunnel.py`:
- Line 83: Remove the strict xfail from the half-close coverage in
scripts/test/e2e/cases/test_node_tunnel.py:83-83 and
scripts/test/e2e/cases/test_sdk_tunnel.py:229-229, or condition each xfail
exclusively on NLB deployments; preserve the tests as required E2E validation
for other tunnel configurations.
🪄 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: 239f1cbd-4bae-4200-ba55-6aa6f1d3f542
📒 Files selected for processing (2)
scripts/test/e2e/cases/test_node_tunnel.pyscripts/test/e2e/cases/test_sdk_tunnel.py
| @pytest.mark.xfail( | ||
| strict=True, | ||
| reason="TCP half-close currently drops the guest response", | ||
| reason="cloud tunnel path through NLB TLS termination does not guarantee TCP half-close", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not retain strict xfails for the fixed half-close coverage. Both tests will fail as XPASS when the tunnel correctly preserves half-close responses, defeating the PR’s required E2E validation.
scripts/test/e2e/cases/test_node_tunnel.py#L83-L83: remove the strict xfail or scope it only to NLB deployments.scripts/test/e2e/cases/test_sdk_tunnel.py#L229-L229: remove the strict xfail or scope it only to NLB deployments.
📍 Affects 2 files
scripts/test/e2e/cases/test_node_tunnel.py#L83-L83(this comment)scripts/test/e2e/cases/test_sdk_tunnel.py#L229-L229
🤖 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 `@scripts/test/e2e/cases/test_node_tunnel.py` at line 83, Remove the strict
xfail from the half-close coverage in
scripts/test/e2e/cases/test_node_tunnel.py:83-83 and
scripts/test/e2e/cases/test_sdk_tunnel.py:229-229, or condition each xfail
exclusively on NLB deployments; preserve the tests as required E2E validation
for other tunnel configurations.
Return the REST CONNECT upgraded stream directly from the Rust SDK instead of inserting an internal duplex copy bridge.
Test plan:
PATH="$HOME/.cargo/bin:$PATH" cargo test -p boxlite --lib connect_box_network_tunnel_uses_connect_and_streams_both_directions --features rest -- --nocapturepython3 -m py_compile scripts/test/e2e/cases/test_sdk_tunnel.py scripts/test/e2e/cases/test_node_tunnel.pygit diff --checkpython -m pytest sdks/python/tests/test_tunnel.py -qgo test ./pkg/proxy -run TestProxyBidirectionalStreamPreservesHalfCloseResponse -vfromapps/common-gogo test ./pkg/proxy -run "TestBufferedConnForwardsCloseWrite|TestTunnelTarget" -vfromapps/proxySummary by CodeRabbit
Bug Fixes
Tests