fix(gateway): fail fast + cache tool lookup on WS upgrade (+ upstream-failure logging)#1255
fix(gateway): fail fast + cache tool lookup on WS upgrade (+ upstream-failure logging)#1255mikhailm-coder wants to merge 5 commits into
Conversation
The tool-agent / tool-api WebSocket routes resolve their upstream via a per-upgrade reactive Mongo lookup (toolRepository). On any lookup error or timeout the filter had no error handler, so the upgrade hung with no response until the gateway response-timeout: the agent saw "No HTTP response" and retried forever (mesh offline), while the static /ws/nats route was unaffected (no lookup). - onErrorResume: write a real status (404 / 503) and complete, so a failed lookup fails fast and visibly instead of hanging the upgrade. - timeout(5s) on the lookup so a stalled Mongo query can't hang to the response-timeout. - Caffeine cache (60s, positive-only) so an agent reconnect storm doesn't put a Mongo lookup on every WS upgrade. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ProxySessionCleanupWebSocketClient only logged after a successful upstream connect (inside the handler). An upstream connect failure/timeout to a dead upstream (meshcentral or NATS) was invisible: the inbound upgrade never completed and the agent just saw "No HTTP response". Add a warn on upstream connect/relay error (and a debug on connect for /ws/ paths) so a "forwarded but upstream dead" case is distinguishable from a tool-lookup failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The upstream WS URI can carry forwarded query params (e.g. an agent key); log host+path only, matching the existing path-only logging in this class. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughTwo WebSocket proxy classes are updated. ChangesWebSocket Proxy Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java (1)
109-111: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a structured cache key instead of delimiter concatenation.
tenantId + "::" + toolIdcan collide if either value can contain::. A small record key avoids cross-tenant/tool cache pollution.Suggested local refactor
- private final Cache<String, IntegratedTool> toolCache = Caffeine.newBuilder() + private record ToolCacheKey(String tenantId, String toolId) {} + + private final Cache<ToolCacheKey, IntegratedTool> toolCache = Caffeine.newBuilder() .maximumSize(10_000) .expireAfterWrite(Duration.ofSeconds(60)) .build();- String cacheKey = tenantRoutingEnabled - ? (TenantRoutingHeaders.tenantId(request) + "::" + toolId) - : toolId; + ToolCacheKey cacheKey = new ToolCacheKey( + tenantRoutingEnabled ? TenantRoutingHeaders.tenantId(request) : null, + toolId);🤖 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/ws/ToolWebSocketProxyUrlFilter.java` around lines 109 - 111, The cache key in ToolWebSocketProxyUrlFilter is built by concatenating tenantId and toolId with "::", which can collide if either value contains that delimiter. Replace the string concatenation with a structured key type (for example, a small local record or equivalent key object) and use that object as the cache key in the tenantRoutingEnabled branch so cache lookups stay unique per tenant/tool combination.
🤖 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/ws/ToolWebSocketProxyUrlFilter.java`:
- Around line 43-46: The ToolWebSocketProxyUrlFilter cache currently keeps
IntegratedTool entries for 60 seconds without any invalidation, so a tool that
has been disabled or deleted can still pass through via the cached value. Update
the caching behavior around the tool lookup and isEnabled() gate in
ToolWebSocketProxyUrlFilter so revocation changes are reflected immediately,
either by invalidating toolCache on tool update/delete events or by
removing/reducing caching for this access check.
- Around line 85-95: The abort path in
ToolWebSocketProxyUrlFilter.abortUpgrade() should skip writing a status once the
ServerHttpResponse is already committed, since setStatusCode() cannot take
effect then. Update the onErrorResume flow to check response.isCommitted() (or
the boolean result of setStatusCode()) before calling setComplete(), and if the
response can no longer be modified, let the error propagate instead of returning
a successful Mono<Void>. Keep the existing abortUpgrade(ServerWebExchange,
String, Throwable) and abortUpgrade(...) logging/status handling as the main
place to make this change.
---
Nitpick comments:
In
`@openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java`:
- Around line 109-111: The cache key in ToolWebSocketProxyUrlFilter is built by
concatenating tenantId and toolId with "::", which can collide if either value
contains that delimiter. Replace the string concatenation with a structured key
type (for example, a small local record or equivalent key object) and use that
object as the cache key in the tenantRoutingEnabled branch so cache lookups stay
unique per tenant/tool combination.
🪄 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: a33fb42c-ac1f-4ed4-8b8a-b6abecaf0b0a
📒 Files selected for processing (2)
openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ProxySessionCleanupWebSocketClient.javaopenframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java
| private final Cache<String, IntegratedTool> toolCache = Caffeine.newBuilder() | ||
| .maximumSize(10_000) | ||
| .expireAfterWrite(Duration.ofSeconds(60)) | ||
| .build(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid caching revocation state without invalidation.
Once an enabled tool is cached, later upgrades bypass the repository and the isEnabled() check for up to 60 seconds. If disabling/deleting a tool is an access-revocation action, add cache invalidation on tool updates or reduce/remove caching for that gate.
Also applies to: 112-132
🤖 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/ws/ToolWebSocketProxyUrlFilter.java`
around lines 43 - 46, The ToolWebSocketProxyUrlFilter cache currently keeps
IntegratedTool entries for 60 seconds without any invalidation, so a tool that
has been disabled or deleted can still pass through via the cached value. Update
the caching behavior around the tool lookup and isEnabled() gate in
ToolWebSocketProxyUrlFilter so revocation changes are reflected immediately,
either by invalidating toolCache on tool update/delete events or by
removing/reducing caching for this access check.
| }) | ||
| // Without this, a lookup error/timeout left the WS upgrade hanging with no response written, so the agent saw "No HTTP response" and retried forever; fail fast with a real status instead. | ||
| .onErrorResume(ex -> abortUpgrade(exchange, toolId, ex)); | ||
| } | ||
|
|
||
| private Mono<Void> abortUpgrade(ServerWebExchange exchange, String toolId, Throwable ex) { | ||
| HttpStatusCode status = (ex instanceof ResponseStatusException rse) ? rse.getStatusCode() : HttpStatus.SERVICE_UNAVAILABLE; | ||
| log.warn("Tool websocket upgrade aborted (tool={}, status={}): {}", toolId, status, ex.toString()); | ||
| ServerHttpResponse response = exchange.getResponse(); | ||
| response.setStatusCode(status); | ||
| return response.setComplete(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Spring WebFlux ServerHttpResponse setStatusCode return value when response is already committed
💡 Result:
When you call setStatusCode(HttpStatusCode) on a Spring WebFlux ServerHttpResponse, the method returns a boolean value indicating whether the status code was successfully applied [1]. The return value behavior is as follows: - Returns true if the status code was successfully set [1]. - Returns false if the status code change could not be processed because the HTTP response is already committed [1]. A response is considered "committed" once the headers and the initial part of the response body have been sent to the client [2][3]. Once the response is in this state, it is no longer possible to modify the status code or headers, as they have already been written to the underlying network stream [1][4]. In practice, developers often use the boolean return value to conditionally handle errors or log status changes, ensuring that they do not attempt to set a status code when it is too late to do so [5][6]. Because attempts to change the status after commitment are ignored (resulting in a false return value), application logic should be designed to handle potential failures or status changes before any part of the response body is emitted [3]. If you need to perform actions or set statuses precisely before the response is committed, you can use the beforeCommit(Supplier) hook provided by the ServerHttpResponse [1][4].
Citations:
- 1: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/server/reactive/AbstractServerHttpResponse.html
- 2: WebFlux with Tomcat, intermittent timeout when creating response spring-projects/spring-framework#23096
- 3: WebFlux controller successfully completes response if Flux emits error [SPR-16051] spring-projects/spring-framework#20600
- 4: https://stackoverflow.com/questions/50101750/how-to-modify-the-response-body-in-spring-cloud-gateway-just-before-the-commit
- 5: https://wang-ray.github.io/java/2018/04/03/%E5%AE%9E%E8%B7%B5Spring-Webflux/
- 6: https://www.seaxiang.com/blog/372646556b594f3f8ec31219226ee4b8
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the surrounding implementation.
git ls-files 'openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java'
wc -l openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java
sed -n '1,220p' openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.javaRepository: flamingo-stack/openframe-oss-lib
Length of output: 7260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the filter implementation and related WebSocket upgrade flow.
sed -n '1,220p' openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws/ToolWebSocketProxyUrlFilter.java
printf '\n--- SEARCH ---\n'
rg -n "chain\.filter|onErrorResume|setStatusCode|isCommitted|upgrade|WebSocket" openframe-gateway-service-core/src/main/java/com/openframe/gateway/config/ws -g '*.java'Repository: flamingo-stack/openframe-oss-lib
Length of output: 19048
Skip abortUpgrade(...) once the response is committed. setStatusCode(...) is a no-op after commit, so this onErrorResume can still complete successfully while the client gets no usable HTTP status. Guard with response.isCommitted() or the setStatusCode(...) return value and let the error propagate when the response can no longer be modified.
🤖 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/ws/ToolWebSocketProxyUrlFilter.java`
around lines 85 - 95, The abort path in
ToolWebSocketProxyUrlFilter.abortUpgrade() should skip writing a status once the
ServerHttpResponse is already committed, since setStatusCode() cannot take
effect then. Update the onErrorResume flow to check response.isCommitted() (or
the boolean result of setStatusCode()) before calling setComplete(), and if the
response can no longer be modified, let the error propagate instead of returning
a successful Mono<Void>. Keep the existing abortUpgrade(ServerWebExchange,
String, Throwable) and abortUpgrade(...) logging/status handling as the main
place to make this change.
1 participant
(Recreated from #1250 after the branch was renamed
fix/→hotfix/.)Problem
On
mikart.openframe.ai~half the fleet shows mesh offline since ~Jun-18: the agent's mesh WS upgrade gets no HTTP response (never a 101), retrying forever — while NATS/HTTPS on the same box work. The NATS route cleanly 502s (and recovers) but the mesh route never returns any HTTP code.Root cause
/ws/tools/agent/{tool}/**resolves its upstream via a per-upgrade reactive Mongo lookup inToolWebSocketProxyUrlFilter. The filter had no error handler: on lookup error/timeout the error propagated with no response written, so the upgrade hung until the gateway response-timeout./ws/natsuses a static upstream (no lookup), so it's unaffected.Change
onErrorResume: write a real status (404/503) and complete — fail fast instead of hanging.timeout(5s)on the lookup so a stalled Mongo query can't hang to the response-timeout.ProxySessionCleanupWebSocketClient: log upstream connect/relay failures (host+path, no query secrets) — captures "forwarded but upstream dead" + NATS upstream failures, previously invisible.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes