fix(gateway): use unpooled connection for WebSocket upstreams#1324
fix(gateway): use unpooled connection for WebSocket upstreams#1324mikhailm-coder wants to merge 2 commits into
Conversation
The WS upstream client (reactorNettyWebSocketClient) used HttpClient.create() — Reactor Netty's shared pool with no idle eviction (no maxIdleTime / evictInBackground / maxLifeTime). MeshCentral (~150s agent idle) and NATS close idle connections server-side, so the pool hands back a socket the upstream already closed; the first relayed frame then fails with "AbortedException: Connection has been closed BEFORE send operation". Combined with SO_LINGER=0 (abortive RST) this surfaced on agents as repeated "No HTTP response" and drove a fleet-wide reconnect loop. WebSocket upstreams are long-lived and dedicated per session, so pooling gives no benefit and only introduces staleness. Use a fresh connection per handshake (ConnectionProvider.newConnection()) and drop SO_LINGER=0 so the upstream closes gracefully. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe ChangesWebSocket HttpClient connection provider update
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
HttpClient.newConnection()instead ofHttpClient.create(ConnectionProvider.newConnection()).Reactor Netty exposes
HttpClient.newConnection()as a direct shorthand for "an unpooled HttpClient", equivalent toHttpClient.create(ConnectionProvider.newConnection()). Using it removes the need for theConnectionProviderimport and is more idiomatic for this use case.♻️ Proposed simplification
-import reactor.netty.resources.ConnectionProvider; +// ConnectionProvider import no longer needed`@Bean`("reactorNettyWebSocketClient") public WebSocketClient reactorNettyWebSocketClient() { // Unpooled + graceful close: WS upstreams are long-lived per session; a pooled, non-evicted idle connection can be handed out already closed (AbortedException before send), and SO_LINGER=0 would RST it. - HttpClient httpClient = HttpClient.create(ConnectionProvider.newConnection()) + HttpClient httpClient = HttpClient.newConnection() .option(ChannelOption.TCP_NODELAY, true); return new ReactorNettyWebSocketClient(httpClient); }Also applies to: 37-39
🤖 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 `@openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java` at line 13, In NettySocketConfig, replace the unpooled client construction in the HttpClient setup with the direct HttpClient.newConnection() shorthand instead of using HttpClient.create(ConnectionProvider.newConnection()). Update the related code path in the configuration to use this idiomatic API and remove the now-unused ConnectionProvider import.
37-39: 🩺 Stability & Availability | 🔵 TrivialNote: dropping
SO_LINGER=0trades RST avoidance for potentialTIME_WAITbuildup on this client.The prior
SO_LINGER=0was specifically there to avoidTIME_WAITaccumulation; removing it for graceful close is the right call given theAbortedException/RST issue described, but under high reconnect churn (the exact symptom being fixed) this WS client could now accumulateTIME_WAITsockets. Worth keeping an eye on ephemeral port/TIME_WAIT metrics post-deploy given the connection volumes cited in the PR (~4882 MeshCentral upgrade attempts/12h).🤖 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 `@openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java` around lines 37 - 39, Keep the SO_LINGER=0 removal in NettySocketConfig.HttpClient setup, but make the TIME_WAIT tradeoff explicit by adding a brief note near HttpClient.create(ConnectionProvider.newConnection()) and ensuring the WebSocket client has operational visibility for reconnect churn. If there is an existing metric/health hook in this client path, wire it to report ephemeral-port/TIME_WAIT pressure so this ConnectionProvider/TCP_NODELAY configuration can be monitored after deploy.
🤖 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
`@openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java`:
- Around line 37-39: Update the NettySocketConfig documentation to match the
current WebSocket client bean behavior. In the section describing socket tuning
for the three beans, remove or qualify the claim that SO_LINGER=0 is applied
across all of them, and explicitly note that the WebSocket client in
NettySocketConfig omits it intentionally. Keep the wording aligned with the
HttpClient setup so future readers can distinguish the shared options from the
bean-specific exception.
---
Nitpick comments:
In
`@openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java`:
- Line 13: In NettySocketConfig, replace the unpooled client construction in the
HttpClient setup with the direct HttpClient.newConnection() shorthand instead of
using HttpClient.create(ConnectionProvider.newConnection()). Update the related
code path in the configuration to use this idiomatic API and remove the
now-unused ConnectionProvider import.
- Around line 37-39: Keep the SO_LINGER=0 removal in
NettySocketConfig.HttpClient setup, but make the TIME_WAIT tradeoff explicit by
adding a brief note near HttpClient.create(ConnectionProvider.newConnection())
and ensuring the WebSocket client has operational visibility for reconnect
churn. If there is an existing metric/health hook in this client path, wire it
to report ephemeral-port/TIME_WAIT pressure so this
ConnectionProvider/TCP_NODELAY configuration can be monitored after deploy.
🪄 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: cdb8b034-26f8-4080-a32a-63d352e11f8f
📒 Files selected for processing (1)
openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/NettySocketConfig.java
Address CodeRabbit: .NettySocketConfig.md claimed SO_LINGER=0 applies to all three beans, but the WebSocket client now intentionally omits it and uses an unpooled connection provider. Scope the socket-option notes accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Problem
Agents (MeshCentral meshagent) and NATS clients across multiple tenants are stuck in a WebSocket reconnect loop. Client logs show
Connection FAILED: No HTTP response/Network timeoutat ~once/minute for hours; REST auth on the same clients succeeds (agent_auth_service: Successfully authenticated using refresh token), so it is WebSocket-specific, not connectivity or credentials.Gateway logs (12h, tenant
tenant-010-15) show the WS upstream bridge failing:Debug ws upstream connection FAILED url=ws://meshcentral…:8383/…/agent.ashx : reactor.netty.channel.AbortedException: Connection has been closed BEFORE send operationws://nats…Cannot enable websocket if headers have already been sentMeshCentral received 4882 upgrades but only 2746 delivered a first frame — connections upgrade then die immediately.
Root cause (confirmed in config)
NettySocketConfig.reactorNettyWebSocketClient()built the WS upstream client with:HttpClient.create()uses Reactor Netty's default pool withmaxIdleTime/maxLifeTime/evictInBackgroundall unset → idle connections are never evicted. MeshCentral (agentidletimeout≈ 150s) and NATS close idle connections server-side, so the pool hands out a socket the upstream already closed →AbortedException: Connection has been closed BEFORE send operationon the first relayed frame.spring.cloud.gateway.httpclienttuning (that only applies to the SCG-managed client), so the WS path can't be fixed via yaml.SO_LINGER=0makes every close an abortive RST, which the agent surfaces asNo HTTP response(reset, no HTTP reply) rather than a clean close.REST survives the same pool because WebClient/Reactor Netty retry idempotent requests on a reset; the WS retry (
FluxRetryWhen) instead re-runs after the client upgrade has started →Cannot enable websocket if headers have already been sent.Fix
WebSocket upstreams are long-lived and dedicated per session — pooling gives no benefit and is the sole source of the staleness. A fresh connection per handshake eliminates the stale-reuse
AbortedException. DroppingSO_LINGER=0lets the upstream close gracefully (FIN) instead of RST.Alternative considered
Keep pooling but add a dedicated
ConnectionProviderwithmaxIdleTime(< MeshCentral's ~150s) +evictInBackground. Rejected as more complex for zero pooling benefit on WS — happy to switch if preferred.Open question for reviewers (the
SO_LINGERreview)This PR only removes
SO_LINGER=0from the WS client. The same option is also set on the inbound server child channel (nettyServerCustomizer) and the REST client customizer (gatewayHttpClientCustomizer). Removing it from the inbound server side would make agent-facing closes graceful FINs instead of RSTs — arguably cleaner, but broader blast radius, so I left those untouched here. Let me know if we want a follow-up.Testing
AbortedException: Connection has been closed BEFORE send operation(should drop to ~0) and MeshCentralfirst-frame / upgraderatio (should approach 1.0) for tenanttenant-010-15.🤖 Generated with Claude Code
Summary by CodeRabbit