fix(edge): release the WebSocket admission permit, add a budget key - #126
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughAdds configurable admission and WebSocket relay caps, validates and wires them into gateway runtime behavior, fixes permit release across WebSocket lifecycle paths, adds regression integration coverage, and re-enables the ChangesEdge hardening configuration and runtime
Integration regression coverage
Benchmark and documentation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
PR Reviewer Guide 🔍
|
![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 (2)
integration-tests/scripts/start-integration-container.sh (1)
218-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe gateway-instance set is re-enumerated by hand in two scripts. The authoritative definition is the
api-sheriff*services (and their:9000publications) inintegration-tests/docker-compose.yml; both scripts copy that set into a literal list guarded only by a "keep in lockstep" comment, so a newly added instance is silently unwaited and undiagnosable.
integration-tests/scripts/start-integration-container.sh#L218-L220: derive the service names and management ports from compose (e.g.docker compose ps --servicesfiltered on^api-sheriffplusdocker compose port <svc> 9000) instead of the literalservice:portpairs.integration-tests/scripts/dump-keycloak-logs.sh#L54-L58: derive the container names from the running compose project (e.g.docker ps --format '{{.Names}}' | grep api-sheriff) instead of the literalintegration-tests-api-sheriff-*-1list.🤖 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 `@integration-tests/scripts/start-integration-container.sh` around lines 218 - 220, The gateway instance lists are duplicated instead of being derived from the running Compose configuration. In integration-tests/scripts/start-integration-container.sh:218-220, replace the literal service/port pairs in the gateway_instance loop with api-sheriff services discovered via docker compose and management ports resolved from each service’s published 9000 port. In integration-tests/scripts/dump-keycloak-logs.sh:54-58, replace the literal container-name list with api-sheriff container names discovered from the running Compose project.Source: Path instructions
integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java (1)
67-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth constants mirror authoritative values elsewhere without deriving them.
WS_ADMISSION_PORT_PREFIXmust track thetest.ws.admission.portfailsafe property inintegration-tests/pom.xml, andSEQUENTIAL_UPGRADESmust trackWebSocketProxyIT.LOW_CAP_SEQUENTIAL_UPGRADES. Both are copied by hand, so a change on either authoritative side leaves this guard asserting the wrong thing — and the upgrade count is precisely what makes cap drift detectable, so its divergence silently weakens the check. Reading the port fromSystem.getProperty("test.ws.admission.port")and referencing the IT's public constant (or hosting one shared constant) closes both.As per path instructions: "Treat a hardcoded list that must mirror a set defined elsewhere … as a defect unless it is derived from that source at build or run time. Name the authoritative definition and the drift it permits."
🤖 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 `@integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java` around lines 67 - 78, Replace the duplicated WS_ADMISSION_PORT_PREFIX value in WsAdmissionActivationWiringTest with the authoritative System.getProperty("test.ws.admission.port") value from integration-tests/pom.xml, adapting it to the existing prefix usage. Replace SEQUENTIAL_UPGRADES with WebSocketProxyIT.LOW_CAP_SEQUENTIAL_UPGRADES, or move both values to a shared authoritative constant, so changes to either source cannot silently weaken the regression check.Source: Path instructions
🤖 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 @.github/workflows/benchmark.yml:
- Line 154: Replace the hardcoded expected benchmark names in the workflow’s
validation step with values derived at build or run time from the authoritative
run-k6-* executions or manifest in benchmarks/pom.xml. Ensure the generated set
is used for summary comparison so additions, removals, and renames remain
synchronized automatically.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/EdgeHardeningConfig.java`:
- Around line 82-87: Update EdgeHardeningConfig.effectiveWebsocketRelayCap() so
an omitted relay cap defaults to the effective admission cap when that is lower
than DEFAULT_WEBSOCKET_RELAY_CAP, while preserving explicitly declared relay
caps. Add a regression test covering edge_hardening with only admission_cap set
and verify ConfigValidator accepts it.
---
Nitpick comments:
In `@integration-tests/scripts/start-integration-container.sh`:
- Around line 218-220: The gateway instance lists are duplicated instead of
being derived from the running Compose configuration. In
integration-tests/scripts/start-integration-container.sh:218-220, replace the
literal service/port pairs in the gateway_instance loop with api-sheriff
services discovered via docker compose and management ports resolved from each
service’s published 9000 port. In
integration-tests/scripts/dump-keycloak-logs.sh:54-58, replace the literal
container-name list with api-sheriff container names discovered from the running
Compose project.
In
`@integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java`:
- Around line 67-78: Replace the duplicated WS_ADMISSION_PORT_PREFIX value in
WsAdmissionActivationWiringTest with the authoritative
System.getProperty("test.ws.admission.port") value from
integration-tests/pom.xml, adapting it to the existing prefix usage. Replace
SEQUENTIAL_UPGRADES with WebSocketProxyIT.LOW_CAP_SEQUENTIAL_UPGRADES, or move
both values to a shared authoritative constant, so changes to either source
cannot silently weaken the regression check.
🪄 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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 52c21658-caaa-4590-8f1f-999b0cce0047
📒 Files selected for processing (29)
.github/workflows/benchmark.ymlapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/EdgeHardeningConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/GatewayConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigModelReflection.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.javaapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/SheriffMetricsTest.javabenchmarks/README.adocbenchmarks/pom.xmldoc/configuration.adocdoc/development/protocol-processors.adocdoc/user/protocol-routes.adocintegration-tests/docker-compose.ymlintegration-tests/pom.xmlintegration-tests/scripts/dump-keycloak-logs.shintegration-tests/scripts/start-integration-container.shintegration-tests/src/main/docker/sheriff-config-ws-admission/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WebSocketProxyIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java
Triage dispositionsIn reply to comment_id:
|
GatewayEdgeRoute.handle registers its sole admission release inside ctx.addEndHandler. A completed WebSocket upgrade takes the connection over, so the HTTP response never ends, the end handler never fires, and one of the gateway's admission permits is stranded for the process lifetime — a sustained WebSocket workload degrades the gateway to blanket 503s. Stash the existing releaseAdmission CAS guard on the RoutingContext under a new key (the same idiom as ROUTE_KEY, and the seam that already crosses the virtual-thread hop), read it back in dispatchWebSocket, and hand WebSocketRelayStage a release callback. The relay invokes it on both teardown paths: the established relay's single idempotent RelaySession.closeBoth funnel, and the client-upgrade-failure branch that constructs no RelaySession at all. It is never invoked at upgrade completion, which would under-count concurrent relays. The upstream-dial-failure path is unchanged — it ends the response, so the end handler still releases through the same guard, and the shared CAS guard keeps any overlap from double-releasing.
…t relay sub-cap A WebSocket relay now holds its admission permit for the connection's whole lifetime, so a modest number of long-lived relays could consume the general pool and starve HTTP — trading the permit leak for a slow squeeze. Bound them separately. Adds the operator-facing edge_hardening block: admission_cap (previously the hard-coded 2048 in EdgeHardeningOptions) and websocket_relay_cap, a sub-budget acquired in addition to the general permit rather than instead of it. Wired through the whole config chain — the EdgeHardeningConfig record, the GatewayConfig component, the JSON schema (whose root additionalProperties:false would otherwise refuse the block at boot), native reflection registration, and a boot validator requiring both caps >= 1 with the relay cap within the pool it draws from. EdgeHardeningOptions is now supplied by a ConfigProducer producer method behind the same buildOnce() guard, so the caps come from the single boot-time assembly; an omitted block resolves to the documented defaults, preserving today's behaviour. GatewayEdgeRoute acquires the sub-permit before the relay hand-off, refuses 503 beyond it, and releases both permits together through the deliverable-1 CAS guard so neither a refusal nor a dial failure can strand one.
Seven passing WebSocket tests coexisted with a permit leak on every established relay, because each performs at most one handshake and asserts only that one response — three leaked permits against a 2048-permit pool is invisible by construction. A per-request correctness assertion cannot observe cumulative resource exhaustion. Adds the assertion of the shape that can: N sequential relays, then a claim about the edge's remaining capacity. It needs a budget small enough to exhaust, and the admission budget is a property of the whole gateway process, so the caps live on a dedicated low-cap instance (api-sheriff-ws-admission, port 10447, admission_cap 8 / websocket_relay_cap 2) rather than on the primary instance that serves the rest of the suite. Ten sequential upgrades exceed both caps, so a leak of either permit fails the run; a following plain HTTP request proves the general permit came back too. The relay releases both permits before closing either leg, so awaiting the close frame makes each iteration race-free rather than timed. WsAdmissionActivationWiringTest is the guard against that regression quietly stopping: it is a no-Docker descriptor assertion that the overlay is mounted, the instance is defined, and both caps stay below the ten upgrades actually driven — because against the shipped defaults the exhaustion test passes whether or not the permits are ever returned, and no black-box assertion inside the IT can tell the difference. Also records the blind-spot analysis in the suite's own javadoc, and corrects the benchmark lane's stack-startup budget comment, which still claimed four gateway instances.
…k is fixed The goal was suppressed because a sustained WebSocket load run exhausted the edge admission pool and left the gateway 503-ing all traffic until restart — which would also have poisoned every goal sequenced after it. The relay now releases its permit from its own teardown funnel, so the aspect measures relay throughput instead of the exhaustion path. Flips skip.benchmark.websocket.echo to false. The property is kept rather than deleted so the goal stays individually suppressible without editing the execution, and the goal keeps its defense-in-depth position at the end of the profile. Re-derives every count and narrative the flip invalidates rather than only the property: the CI lane now expects ELEVEN of the twelve declared goals to produce a result, so websocketEcho joins the coverage step's expected list — without which the goal could vanish from a run and the job would still pass — and leaves the "Deliberately skipped" table, where only uploadLarge remains. The goal-execution budget term and the README's coverage and trend-series passages move with it. Every uploadLarge / skip.benchmark.upload.large row is left byte-unchanged; that goal is still blocked on its own gateway defect.
…ayers A configuration key that exists only in the schema is a key operators cannot find, so the new block is documented at each of the three layers that answer a different question about it. doc/configuration.adoc is the reference: the gateway.yaml sample gains the block with both keys commented, and the "Edge hardening defaults" section is rewritten around the distinction the block introduces — the framing and drain bounds remain fixed in code, while the two admission budgets are now operator-owned, because their sizing depends on the deployment's traffic shape rather than on protocol safety. That section previously claimed the whole set was applied in code "not by gateway.yaml"; the claim is now false and is gone. A Field Reference entry and two Validation Rules entries (both caps >= 1, and the relay cap within the pool it draws from) complete the reference surface. doc/user/protocol-routes.adoc answers the operator's actual question — what to set it to. A relay holds its permit for the connection's lifetime rather than a round-trip, so relays accumulate against the pool HTTP draws from; size the sub-cap to peak concurrent relays plus reconnect-storm headroom, and read admission_cap minus websocket_relay_cap as the request concurrency guaranteed to survive a saturated relay pool. doc/development/protocol-processors.adoc records the permit lifecycle as a contract, because this is the one dispatch path where the edge's ordinary release site does not work: a completed upgrade takes the connection over, so ctx.addEndHandler never fires. Names all three release sites and the reason both permits must be returned together.
…ources Formatter and import-sort output the quality gate produced against the files the two preceding commits touched: import-group separation in EdgeHardeningOptions and EdgeHardeningTest, continuation-indent normalisation in the two WebSocket test helpers. No behavioural change; committed so the tree the gate produces and the tree in the branch agree.
Sonar java:S7467 - the ExecutionException binding in connectWhenAdmitted is never read, so replace it with the Java 25 unnamed pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UV4pXNUtk5CKceRLWxvapz
…admission cap A partially declared edge_hardening block self-rejected at boot: with admission_cap lowered below 512 and websocket_relay_cap omitted, the relay cap resolved to the fixed DEFAULT_WEBSOCKET_RELAY_CAP (512) while ConfigValidator compares the resolved pair, so the block was refused with 'websocket_relay_cap 512 must not exceed admission_cap 64' - a configuration the class Javadoc documents as valid. effectiveWebsocketRelayCap() now resolves an absent member to Math.max(1, effectiveAdmissionCap() / 4) instead of the fixed constant. That preserves the documented three-quarters-for-HTTP reservation at every admission cap (a min-clamp against 512 would resolve relay == admission below 512, letting relays consume the whole general pool) and is byte-identical at the shipped default, since 2048 / 4 == 512. An explicitly declared relay cap is untouched, so an explicit cap above admission_cap is still refused at boot. Javadoc, doc/configuration.adoc and doc/user/protocol-routes.adoc updated to describe the derived default. EdgeHardeningTest.fallsBackPerOmittedMember asserted the old fixed-constant contract at admission_cap 4096 and now asserts the derived 1024; the four default-cap assertions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UV4pXNUtk5CKceRLWxvapz
Promoted from a recurring finalize disposition: review-bot suggestions to derive the benchmark coverage expected= list from benchmarks/pom.xml were declined twice in this plan, because independence from the pom is the property the check relies on. Co-Authored-By: Claude <noreply@anthropic.com>
Records why a WebSocket relay holds its general admission permit for the connection lifetime and additionally acquires a derived websocket_relay_cap sub-budget, and why the omitted relay cap derives proportionally from the admission cap rather than being clamped to a constant. Co-Authored-By: Claude <noreply@anthropic.com>
PR #125 landed ADR-0018 and ADR-0019 while this branch was queued, so the number this ADR was authored under is taken. The rebase was conflict-free because the filenames differ, which is exactly why the collision had to be caught by inspection rather than by git. Co-Authored-By: Claude <noreply@anthropic.com>
2b69abf to
27866fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.plan/project-architecture/benchmarks/enriched.json:
- Around line 3-5: Replace the hand-maintained benchmark goal list described in
the insights entry with an automated consistency mechanism tied to the
authoritative run-k6-* goals in benchmarks/pom.xml. Ensure the workflow coverage
oracle detects both newly added or un-skipped goals and removed goals, rather
than silently accepting divergence between the Maven definitions and
benchmark.yml.
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.java`:
- Around line 433-446: Extend shouldAcceptLoweredAdmissionCapWithoutRelayCap or
add a focused test covering an omitted websocket_relay_cap with admission_cap
set to 1, and assert effectiveWebsocketRelayCap() returns 1 rather than a
quarter-rounded value of 0. Preserve the existing validation-success assertion
and use the existing EdgeHardeningConfig and gateway setup patterns.
In
`@doc/adr/0020-WebSocket_relays_hold_their_admission_permit_for_the_connection_lifetime_and_are_bounded_by_a_derived_sub-budget.adoc`:
- Around line 74-90: Ensure the configuration validation and defaulting
described in the ADR enforce a strict websocket_relay_cap < admission_cap
relationship, including the derived default when admission_cap is 1. Update the
related guarantees and examples in the affected sections to state the required
minimum admission cap or qualify HTTP-headroom claims when separation is not
guaranteed; keep all references consistent.
In `@integration-tests/scripts/start-integration-container.sh`:
- Around line 218-220: Replace the hardcoded gateway_instance inventory in the
startup loop with entries derived from the authoritative gateway service and
management-port mappings in integration-tests/docker-compose.yml, or consume a
shared/generated manifest. Preserve the existing GATEWAY_SERVICE and
GATEWAY_MGMT_PORT parsing while ensuring future Compose additions or port
changes are automatically included.
- Around line 221-223: Update the readiness polling loop around the gateway curl
check to request the Quarkus readiness endpoint /q/health/ready instead of
/q/health/live. Keep the existing retry and success behavior unchanged so
startup waits for GatewayReadinessCheck validation to complete.
🪄 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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 632c9457-0b56-4847-b345-c60aeed4414c
📒 Files selected for processing (31)
.github/workflows/benchmark.yml.plan/project-architecture/benchmarks/enriched.jsonapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/EdgeHardeningConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/GatewayConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigModelReflection.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.javaapi-sheriff/src/main/resources/schema/gateway.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/SheriffMetricsTest.javabenchmarks/README.adocbenchmarks/pom.xmldoc/adr/0020-WebSocket_relays_hold_their_admission_permit_for_the_connection_lifetime_and_are_bounded_by_a_derived_sub-budget.adocdoc/configuration.adocdoc/development/protocol-processors.adocdoc/user/protocol-routes.adocintegration-tests/docker-compose.ymlintegration-tests/pom.xmlintegration-tests/scripts/dump-keycloak-logs.shintegration-tests/scripts/start-integration-container.shintegration-tests/src/main/docker/sheriff-config-ws-admission/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WebSocketProxyIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java
🚧 Files skipped from review as they are similar to previous changes (27)
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigModelReflection.java
- doc/development/protocol-processors.adoc
- doc/user/protocol-routes.adoc
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/GatewayConfig.java
- api-sheriff/src/main/resources/schema/gateway.schema.json
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/SheriffMetricsTest.java
- integration-tests/scripts/dump-keycloak-logs.sh
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/EdgeHardeningConfig.java
- doc/configuration.adoc
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.java
- integration-tests/docker-compose.yml
- benchmarks/pom.xml
- integration-tests/src/main/docker/sheriff-config-ws-admission/gateway.yaml
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java
- .github/workflows/benchmark.yml
- integration-tests/pom.xml
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.java
- benchmarks/README.adoc
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WebSocketProxyIT.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java
Triage dispositionsIn reply to comment_id:
|
1 participant
Summary
Fixes a remotely-reachable denial-of-service defect in the API Sheriff edge: every WebSocket upgrade
permanently leaked one admission permit from the gateway's single admission semaphore, so a sustained
WebSocket workload could exhaust the pool and the gateway would then reject all traffic — HTTP
included — with 503 until restarted. This PR releases the permit correctly, promotes the admission
budget to operator-facing configuration with a dedicated WebSocket-relay budget key, adds a
containerised regression test that reproduces and proves the fix, records why the existing IT suite
was blind to the leak, and re-enables the previously-quarantined
websocketEchobenchmark goal.Changes
Leak fix (D1a) — the admission permit is now released exactly once via the existing
releaseAdmission(AtomicBoolean)CAS guard, held for the whole relay lifecycle instead of beingdropped at upgrade time. Both the established-relay teardown funnel (
RelaySession.closeBoth) andthe client-upgrade-failure path now release correctly:
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaOperator-facing config surface (D1b) — the admission cap is no longer a hard-coded constant; it
is now a configurable, WebSocket-relay-specific budget key threaded through the full config chain:
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/EdgeHardeningConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/GatewayConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigModelReflection.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.javaapi-sheriff/src/main/resources/schema/gateway.schema.jsonContainerised regression test (D2) — a dedicated compose service reusing the same native image
with an overlaid, low-admission-cap
gateway.yaml(mirroring the existingapi-sheriff-mtls/api-sheriff-cookieidiom) so the shared primary IT instance is untouched:integration-tests/src/main/docker/sheriff-config-ws-admission/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WebSocketProxyIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/WsAdmissionActivationWiringTest.javaintegration-tests/docker-compose.ymlintegration-tests/pom.xmlintegration-tests/scripts/dump-keycloak-logs.shintegration-tests/scripts/start-integration-container.shBlind-spot record (D3) —
WebSocketProxyITnow documents why the prior suite (7 tests, onehandshake each, per-request status assertions only) could never exhaust a large admission pool and
therefore never observed the leak.
Benchmark re-activation (D4) —
websocketEchois re-enabled end to end: the skip flag defaultsto
false, the goal moves from the skipped table into the coverage step's expected list, and stalegoal-count prose is updated:
benchmarks/pom.xml.github/workflows/benchmark.ymlbenchmarks/README.adocThree-layer documentation (D5) — the new operator-visible configuration key is documented across
the authoritative reference, the user layer, and the development layer:
doc/configuration.adocdoc/user/protocol-routes.adocdoc/development/protocol-processors.adocRegression coverage for the touched production classes:
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/SheriffMetricsTest.javaTest Plan
verify -Ppre-commit) passedverify) passedverify -Pintegration-tests -pl integration-tests -am), including thenew
WebSocketProxyITregression case, passedwebsocketEchogoal produces asummary document (tracked separately, not a pre-merge gate)
Related Issues
None.
Generated by plan-finalize skill
Summary by CodeRabbit
edge_hardeningadmission limits:admission_capandwebsocket_relay_cap, including defaults and derived relay-cap behavior.websocket_relay_capare rejected with HTTP 503.