Repository navigation
Conversation
Signed-off-by: Shiju <shiju@nvidia.com>
Consolidate overlapping forwarding cases through the complete relay and retain distinct failure boundaries. Remove relay implementation details from the network policy guide. Signed-off-by: Shiju <shiju@nvidia.com>
EmilienM
left a comment
There was a problem hiding this comment.
Another iteration needed: 1 blocking, 4 major, 7 minor (inline, 5 below)
The tunnel REST/GraphQL/passthrough loops are covered and tested, but the JSON-RPC/MCP loop and the forward-proxy entry path don't get the fix, and response_started can still suppress the 502 with nothing delivered to the client.
Outside the diff:
- Blocking: the PR has no area label, so Review Readiness fails;
area:supervisor(orarea:runtime) fits. - Major:
crates/openshell-supervisor-network/src/proxy.rs:6836, the forward-proxy error boundary downcasts the sibling typed relay errors but has no arm for the newUpstreamHttpError, so plain forward requests still close silently with no 502 and the allow-event for an allowed request is now skipped. - Major: issue #5322's first acceptance criterion (replaying a request sent on a reused connection when the upstream closed before any response byte) isn't implemented; since the PR closes the issue, either implement it for idempotent methods or update the issue so the descope is recorded.
- Minor:
relay.rs:2355, an upstream failure now ends the websocket middleware session asCancellationwhile the sibling arms useUpstreamFailure. - Minor: the PR body says the connection behavior is documented, but the second commit removed the docs paragraph; restore a trimmed note or drop the claim.
How this was reviewed
- Commit
e5fe0357, round 1 - Standard review, because the change is code in
crates/openshell-supervisor-network/src/l7/relay.rs,l7/rest.rs, andl7/rest/http_response.rs - Passes: architecture, performance, security, scalability, rust, supply-chain, then correctness
- Independent skeptics checked each candidate finding (2 per blocking or major, 1 per minor); 11 of 11 held up, 2 of them after re-checking the code
- Repo rules read from
main: AGENTS.md, CLAUDE.md, CONTRIBUTING.md, docs/CONTRIBUTING.mdx - Not covered: nothing skipped
Cover JSON-RPC/MCP idle closure and forward HTTP failures. Track response commitment at client writes, retain upstream causes, and distinguish truncated framing from normal completion and policy reload. Preserve request correlation and upstream/session cleanup. Signed-off-by: Shiju <shiju@nvidia.com>
Integrate audit trace correlation while preserving upstream HTTP failure handling. Signed-off-by: Shiju <shiju@nvidia.com>
|
All five HTTP loops now watch idle upstream connections, including JSON-RPC/MCP. Forward HTTP uses the same upstream-error handling and preserves allowed-request telemetry. Response commitment tracks successful client writes, so an incomplete buffered header can still produce a 502. Original I/O causes are preserved, and truncated bodies are distinguished from policy or middleware failures after normal body completion. The shared handler preserves the request ID, shuts down upstream, and records a 502 only after delivery succeeds. Session cleanup uses UpstreamFailure. The tests now cover inspected REST, resets and the forward-handler path. The stale docs claim is removed, and |
Record the response request ID in the HTTP upstream-failure event so operators can match client errors to structured and shorthand logs. Cover supplied and absent IDs in the existing event schema test. Signed-off-by: Shiju <shiju@nvidia.com>
Summary
Retire HTTP connections promptly when the upstream closes while idle, including JSON-RPC and MCP. For relay-owned upstream failures, return a correlated
502 Bad GatewaywithConnection: closebefore any response bytes are written to the client; close without appending another status once a response has started. Apply the same error handling to plain forward HTTP and CONNECT tunnels.Related Issue
Closes #5322
Scope follows the issue discussion: automatic reconnect/replay is deliberately excluded. The original replay acceptance criterion still needs the issue author's correction to reflect the agreed idle-close and explicit-error scope.
Changes
Release Notes
HTTP clients receive idle upstream closure promptly. Relay-owned failures before a response starts return an explicit gateway error with a request ID.
Testing
Local validation used Rust 1.95.0 on macOS. Full Linux sandbox E2E and the reporter's EKS/Istio/Bun deployment were not run; hosted CI owns broad verification after this push.
cargo clippy --offline --locked -p openshell-supervisor-network --lib --tests -- -D warnings: passed. Rust formatting, diff whitespace and SPDX checks passed.Checklist