From 2e804ca776ae66248b762a8c30f704b74028ec24 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 18:12:07 +0200 Subject: [PATCH 1/9] feat(velocity): support static server discovery --- README.md | 11 ++ .../2026-08-23-static-server-discovery.md | 126 ++++++++++++++++++ ...26-08-23-static-server-discovery-design.md | 27 ++++ .../gg/grounds/discovery/DiscoveryConfig.kt | 35 +++++ .../gg/grounds/discovery/DiscoveryService.kt | 126 +++++++++++------- .../grounds/discovery/DiscoveryConfigTest.kt | 80 +++++++++++ .../grounds/discovery/ServerOwnershipTest.kt | 20 +++ 7 files changed, 380 insertions(+), 45 deletions(-) create mode 100644 docs/superpowers/plans/2026-08-23-static-server-discovery.md create mode 100644 docs/superpowers/specs/2026-08-23-static-server-discovery-design.md create mode 100644 velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt diff --git a/README.md b/README.md index 3a6c5ec..8068ec0 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,17 @@ deployments do not need to set anything. | `GROUNDS_AGONES_POLL_INTERVAL` | `2s` | Accepts `Ns`, `Nm`, `Nh` | | `GROUNDS_AGONES_ADDRESS_TYPE` | `PodIP` | Which entry of `status.addresses` to dial (`PodIP`, `ExternalIP`, …) | | `GROUNDS_AGONES_PORT` | `25565` | TCP port on the GameServer | +| `GROUNDS_STATIC_SERVERS` | _(none)_ | Comma-separated `name=host:port` static Velocity backends | + +`GROUNDS_STATIC_SERVERS` is validated strictly: names and hosts must be non-empty, ports must be +between `1` and `65535`, and names must be unique. For example, Stage proxies can use: + +```text +GROUNDS_STATIC_SERVERS=buildserver=buildserver:25565 +``` + +This only registers the backend with Velocity; configuring the backend itself for Velocity +forwarding remains a separate deployment concern. Typical Helm chart wiring uses a `ConfigMap` consumed via `envFrom`, plus `POD_NAMESPACE` from the Downward API for clusters where the proxy should diff --git a/docs/superpowers/plans/2026-08-23-static-server-discovery.md b/docs/superpowers/plans/2026-08-23-static-server-discovery.md new file mode 100644 index 0000000..163e414 --- /dev/null +++ b/docs/superpowers/plans/2026-08-23-static-server-discovery.md @@ -0,0 +1,126 @@ +# Static Server Discovery Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add explicitly configured static Velocity backends without allowing Agones reconciliation to delete them. + +**Architecture:** Parse a strict optional `GROUNDS_STATIC_SERVERS` value into typed `StaticServer` entries in `DiscoveryConfig`. `DiscoveryService` registers those entries independently at startup and tracks only Agones-owned names for later removal. + +**Tech Stack:** Kotlin, Velocity API, JUnit 5, Gradle + +**Spec:** `docs/superpowers/specs/2026-08-23-static-server-discovery-design.md` + +## Global Constraints + +- The environment variable is exactly `GROUNDS_STATIC_SERVERS`. +- Its format is a comma-separated list of `name=host:port` entries. +- Absent or blank configuration preserves current behavior and yields no static servers. +- Names and hosts are non-empty, ports are in `1..65535`, duplicate names are rejected, and malformed entries fail configuration. +- Static servers have role `static` and are never added to the lobby set. +- Agones cleanup may unregister only names previously registered and tracked by Agones discovery. +- A configured static server wins a name collision with an Agones GameServer. +- Static registration remains available if Kubernetes client initialization fails. + +--- + +### Task 1: Static configuration and ownership-aware registration + +**Files:** +- Modify: `velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt` +- Modify: `velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt` +- Modify: `velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt` +- Create: `velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt` +- Modify: `README.md` + +**Interfaces:** +- Produces: `data class StaticServer(val name: String, val host: String, val port: Int)`. +- Produces: `DiscoveryConfig.staticServers: List` sourced from `GROUNDS_STATIC_SERVERS`. +- Produces: an internal pure ownership helper used by `DiscoveryService` that returns stale names from the Agones-managed set only. + +- [ ] **Step 1: Write failing configuration tests** + +Add tests demonstrating these literal outcomes: + +```kotlin +assertEquals(emptyList(), DiscoveryConfig.fromEnv(emptyMap()).staticServers) +assertEquals( + listOf( + StaticServer("buildserver", "buildserver", 25565), + StaticServer("metrics", "metrics.stage.svc.cluster.local", 25566), + ), + DiscoveryConfig.fromEnv( + mapOf( + "GROUNDS_STATIC_SERVERS" to + " buildserver=buildserver:25565, metrics=metrics.stage.svc.cluster.local:25566 " + ) + ).staticServers, +) +``` + +Add separate `assertThrows` cases for a missing `=`, empty name, empty host, port `0`, port `65536`, a non-numeric port, and duplicate names. + +- [ ] **Step 2: Run the configuration tests and verify RED** + +Run: + +```bash +./gradlew --no-daemon :velocity:test --tests gg.grounds.discovery.DiscoveryConfigTest +``` + +Expected: compilation or assertion failure because `StaticServer` and `staticServers` do not exist. + +- [ ] **Step 3: Implement strict static-server parsing** + +Add `StaticServer`, add `staticServers` to `DiscoveryConfig`, document `GROUNDS_STATIC_SERVERS`, and parse the optional value. Blank input returns an empty list. Split entries on commas, split each entry once on `=`, split the address at the final `:`, trim fields, validate all constraints, and throw `IllegalArgumentException` with the offending entry but without secrets or unrelated environment values. + +- [ ] **Step 4: Run the configuration tests and verify GREEN** + +Run the command from Step 2. Expected: PASS. + +- [ ] **Step 5: Write failing ownership tests** + +Create `ServerOwnershipTest` for the internal pure helper. It must prove: + +```kotlin +assertEquals(setOf("old-game"), staleManagedServerNames(setOf("live-game"), setOf("live-game", "old-game"))) +assertEquals(emptySet(), staleManagedServerNames(emptySet(), emptySet())) +``` + +The test names must state that only missing Agones-owned servers become stale and an unowned static server cannot enter the removal result. + +- [ ] **Step 6: Run the ownership test and verify RED** + +Run: + +```bash +./gradlew --no-daemon :velocity:test --tests gg.grounds.discovery.ServerOwnershipTest +``` + +Expected: compilation failure because the helper does not exist. + +- [ ] **Step 7: Implement static registration and Agones ownership tracking** + +Change startup order to remove image placeholders, register all configured static servers with `ServerInfo(name, InetSocketAddress.createUnresolved(host, port))`, assign role `static`, and register listeners before attempting Kubernetes initialization. Only initialize `coreApi` and schedule polling when the Kubernetes client is available. + +Track successful/current Agones registrations in a concurrent set. During each poll, skip an Agones server whose name belongs to a configured static server and log the collision. Cleanup must call the tested helper with running Agones names and the tracked Agones-owned set, unregister only the returned names, and clear their lobby/role/ownership state. It must not iterate over all Velocity registrations as removal candidates. + +- [ ] **Step 8: Document the environment variable** + +Add `GROUNDS_STATIC_SERVERS`, its `name=host:port` syntax, strict validation, and the Stage example to `README.md` without claiming that the backend itself is automatically configured for Velocity forwarding. + +- [ ] **Step 9: Run formatting and the complete Velocity test suite** + +Run: + +```bash +./gradlew --no-daemon spotlessApply :velocity:test +``` + +Expected: `BUILD SUCCESSFUL`. + +- [ ] **Step 10: Commit** + +```bash +git add README.md docs velocity/src +git commit -m "feat(velocity): support static server discovery" +``` diff --git a/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md b/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md new file mode 100644 index 0000000..bd3bf3e --- /dev/null +++ b/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md @@ -0,0 +1,27 @@ +# Static Server Discovery Design + +## Goal + +Allow Velocity proxies running `plugin-agones` to register a small set of explicitly configured, non-Agones backends such as the temporary Stage buildserver. + +## Configuration + +`GROUNDS_STATIC_SERVERS` is optional. Its value is a comma-separated list of `name=host:port` entries, for example: + +```text +buildserver=buildserver:25565 +``` + +Whitespace around entries is ignored. Names and hosts must be non-empty, ports must be in `1..65535`, and duplicate names or malformed entries fail plugin configuration rather than being silently ignored. An absent or blank value means no static servers and preserves current production behavior. + +## Ownership and reconciliation + +At startup the plugin continues removing the placeholder backends baked into the Velocity image, then registers the configured static servers. Static servers use the role `static`; they are never lobby candidates. + +Agones polling owns only servers it registered from Agones. Reconciliation may remove a previously managed Agones server that is no longer running, but must never remove a configured static server or a backend owned by another plugin. A static name takes precedence over an Agones GameServer with the same name and the collision is logged. + +Static registration must still work when the Kubernetes client cannot initialize. Agones polling may remain disabled in that case. + +## Deployment path + +The Stage proxies will receive `GROUNDS_STATIC_SERVERS=buildserver=buildserver:25565` after the plugin is released and bundled into a Velocity image. The buildserver will then be switched from public online-mode access to a ClusterIP backend using modern Velocity forwarding. That deployment work is intentionally separate from this plugin change. diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt index 0134cbb..bbd2d02 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt @@ -19,7 +19,11 @@ import java.time.Duration * - `GROUNDS_AGONES_ADDRESS_TYPE` — Which `status.addresses` entry to use (`PodIP`, `ExternalIP`, * `InternalIP`, `Hostname`). * - `GROUNDS_AGONES_PORT` — TCP port to dial on the discovered GameServer. + * - `GROUNDS_STATIC_SERVERS` — Comma-separated `name=host:port` Velocity backends that are + * registered without Agones discovery. */ +data class StaticServer(val name: String, val host: String, val port: Int) + data class DiscoveryConfig( val namespace: String, val labelSelector: String, @@ -29,6 +33,7 @@ data class DiscoveryConfig( val pollInterval: Duration, val addressType: String, val port: Int, + val staticServers: List, ) { companion object { const val DEFAULT_NAMESPACE = "games" @@ -59,6 +64,7 @@ data class DiscoveryConfig( ?: DEFAULT_POLL_INTERVAL, addressType = env["GROUNDS_AGONES_ADDRESS_TYPE"] ?: DEFAULT_ADDRESS_TYPE, port = env["GROUNDS_AGONES_PORT"]?.toIntOrNull() ?: DEFAULT_PORT, + staticServers = parseStaticServers(env["GROUNDS_STATIC_SERVERS"]), ) private val DURATION_PATTERN = Regex("""^(\d+)\s*(s|m|h)$""") @@ -77,5 +83,34 @@ data class DiscoveryConfig( else -> error("unreachable") } } + + private fun parseStaticServers(raw: String?): List { + if (raw.isNullOrBlank()) return emptyList() + + val names = mutableSetOf() + return raw.split(",").map { entry -> + val trimmedEntry = entry.trim() + val separator = trimmedEntry.indexOf('=') + require(separator >= 0) { "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry'" } + + val name = trimmedEntry.substring(0, separator).trim() + val address = trimmedEntry.substring(separator + 1).trim() + val portSeparator = address.lastIndexOf(':') + require(name.isNotEmpty() && portSeparator >= 0) { + "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry'" + } + + val host = address.substring(0, portSeparator).trim() + val port = address.substring(portSeparator + 1).trim().toIntOrNull() + require(host.isNotEmpty() && port != null && port in 1..65535) { + "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry'" + } + require(names.add(name)) { + "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry': duplicate name '$name'" + } + + StaticServer(name, host, port) + } + } } } diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt index 3f9921b..43a4e60 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt @@ -16,6 +16,11 @@ import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit import org.slf4j.Logger +internal fun staleManagedServerNames( + runningAgonesServerNames: Set, + agonesManagedServerNames: Set, +): Set = agonesManagedServerNames - runningAgonesServerNames + class DiscoveryService( private val plugin: Any, private val proxyServer: ProxyServer, @@ -28,15 +33,18 @@ class DiscoveryService( private lateinit var pollTask: ScheduledTask private val lobbyServers: MutableSet = ConcurrentHashMap.newKeySet() private val serverRoles: MutableMap = ConcurrentHashMap() + private val agonesManagedServers: MutableSet = ConcurrentHashMap.newKeySet() + private val staticServerNames = config.staticServers.mapTo(mutableSetOf()) { it.name } @Volatile private var countsSnapshot: Pair?>? = null fun start() { + unregisterPreconfiguredServers() + registerStaticServers() + registerListeners() + customObjectsApi = createCustomObjectsApi() ?: return // Same client, already configured as the default above. coreApi = CoreV1Api() - - unregisterPreconfiguredServers() - registerListeners() schedulePolling() } @@ -75,6 +83,24 @@ class DiscoveryService( } } + private fun registerStaticServers() { + for (server in config.staticServers) { + proxyServer.registerServer( + ServerInfo( + server.name, + InetSocketAddress.createUnresolved(server.host, server.port), + ) + ) + serverRoles[server.name] = STATIC_SERVER_ROLE + logger.info( + "Registered static proxy server successfully (serverName={}, host={}, port={})", + server.name, + server.host, + server.port, + ) + } + } + private fun registerListeners() { proxyServer.eventManager.register( plugin, @@ -173,47 +199,58 @@ class DiscoveryService( continue } - val serverType = resolveServerType(metadata.labels) ?: continue - serverRoles[serverName] = serverType - - if (serverType == config.lobbyValue) { - lobbyServers.add(serverName) - } else { - lobbyServers.remove(serverName) + if (serverName in staticServerNames) { + logger.warn( + "Skipping Agones GameServer because its name collides with a static server (serverName={})", + serverName, + ) + continue } - if (serverName in currentServers) continue + val serverType = resolveServerType(metadata.labels) ?: continue + + if (serverName !in currentServers) { + // Agones does not always publish the pod's address in the GameServer's + // status. The bundle's lobby fleets carry Hostname, InternalIP AND + // PodIP; the fleets forge renders for a pushed gamemode carry only the + // first two. A proxy that insists on PodIP therefore throws away every + // pushed gamemode — the server runs, is Ready, and no player can ever + // reach it, which looks exactly like a broken game. + // + // The pod is the source of that address anyway, and Agones names it + // after the GameServer, so fall back to reading it directly. Never the + // node's InternalIP: that would route players to a machine instead of + // to their server. + val address = + gameServer.status + ?.addresses + ?.firstOrNull { it.type == config.addressType } + ?.address ?: podIp(serverName) + if (address == null) { + logger.error( + "Failed to register Agones GameServer (serverName={}, reason=missing_address, addressType={})", + serverName, + config.addressType, + ) + continue + } - // Agones does not always publish the pod's address in the GameServer's - // status. The bundle's lobby fleets carry Hostname, InternalIP AND - // PodIP; the fleets forge renders for a pushed gamemode carry only the - // first two. A proxy that insists on PodIP therefore throws away every - // pushed gamemode — the server runs, is Ready, and no player can ever - // reach it, which looks exactly like a broken game. - // - // The pod is the source of that address anyway, and Agones names it - // after the GameServer, so fall back to reading it directly. Never the - // node's InternalIP: that would route players to a machine instead of - // to their server. - val address = - gameServer.status?.addresses?.firstOrNull { it.type == config.addressType }?.address - ?: podIp(serverName) - if (address == null) { - logger.error( - "Failed to register Agones GameServer (serverName={}, reason=missing_address, addressType={})", + val serverInfo = ServerInfo(serverName, InetSocketAddress(address, config.port)) + proxyServer.registerServer(serverInfo) + agonesManagedServers.add(serverName) + logger.info( + "Registered proxy server successfully (serverName={}, serverType={})", serverName, - config.addressType, + serverType, ) - continue } - val serverInfo = ServerInfo(serverName, InetSocketAddress(address, config.port)) - proxyServer.registerServer(serverInfo) - logger.info( - "Registered proxy server successfully (serverName={}, serverType={})", - serverName, - serverType, - ) + serverRoles[serverName] = serverType + if (serverType == config.lobbyValue) { + lobbyServers.add(serverName) + } else { + lobbyServers.remove(serverName) + } } } @@ -234,16 +271,14 @@ class DiscoveryService( ) { val runningServerNames = runningGameServers.mapNotNull { it.metadata?.name }.toSet() - for (server in currentServers.values) { - if (server.serverInfo.name !in runningServerNames) { + for (serverName in staleManagedServerNames(runningServerNames, agonesManagedServers)) { + currentServers[serverName]?.let { server -> proxyServer.unregisterServer(server.serverInfo) - lobbyServers.remove(server.serverInfo.name) - serverRoles.remove(server.serverInfo.name) - logger.info( - "Unregistered proxy server successfully (serverName={})", - server.serverInfo.name, - ) + logger.info("Unregistered proxy server successfully (serverName={})", serverName) } + lobbyServers.remove(serverName) + serverRoles.remove(serverName) + agonesManagedServers.remove(serverName) } } @@ -251,6 +286,7 @@ class DiscoveryService( private const val GROUP = "agones.dev" private const val VERSION = "v1" private const val PLURAL = "gameservers" + private const val STATIC_SERVER_ROLE = "static" private val COUNTS_TTL_NANOS = TimeUnit.SECONDS.toNanos(2) } } diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt index 4de0bc1..385d5c8 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt @@ -19,6 +19,7 @@ class DiscoveryConfigTest { assertEquals(Duration.ofSeconds(2), cfg.pollInterval) assertEquals("PodIP", cfg.addressType) assertEquals(25565, cfg.port) + assertEquals(emptyList(), cfg.staticServers) } @Test @@ -121,4 +122,83 @@ class DiscoveryConfigTest { assertEquals(25565, cfg.port) assertEquals(Duration.ofSeconds(2), cfg.pollInterval) } + + @Test + fun `static servers parse comma separated name host and port entries`() { + assertEquals( + listOf( + StaticServer("buildserver", "buildserver", 25565), + StaticServer("metrics", "metrics.stage.svc.cluster.local", 25566), + ), + DiscoveryConfig.fromEnv( + env = + mapOf( + "GROUNDS_STATIC_SERVERS" to + " buildserver=buildserver:25565, metrics=metrics.stage.svc.cluster.local:25566 " + ) + ) + .staticServers, + ) + } + + @Test + fun `static servers reject entries without a name address separator`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv(env = mapOf("GROUNDS_STATIC_SERVERS" to "buildserver:25565")) + } + } + + @Test + fun `static servers reject entries with an empty name`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv(env = mapOf("GROUNDS_STATIC_SERVERS" to "=buildserver:25565")) + } + } + + @Test + fun `static servers reject entries with an empty host`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv(env = mapOf("GROUNDS_STATIC_SERVERS" to "buildserver=:25565")) + } + } + + @Test + fun `static servers reject entries with port zero`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv( + env = mapOf("GROUNDS_STATIC_SERVERS" to "buildserver=buildserver:0") + ) + } + } + + @Test + fun `static servers reject entries with ports above 65535`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv( + env = mapOf("GROUNDS_STATIC_SERVERS" to "buildserver=buildserver:65536") + ) + } + } + + @Test + fun `static servers reject entries with non numeric ports`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv( + env = mapOf("GROUNDS_STATIC_SERVERS" to "buildserver=buildserver:abc") + ) + } + } + + @Test + fun `static servers reject duplicate names`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv( + env = + mapOf( + "GROUNDS_STATIC_SERVERS" to + "buildserver=buildserver:25565,buildserver=other:25566" + ) + ) + } + } } diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt new file mode 100644 index 0000000..f0ed7db --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt @@ -0,0 +1,20 @@ +package gg.grounds.discovery + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class ServerOwnershipTest { + + @Test + fun `only missing Agones owned servers become stale`() { + assertEquals( + setOf("old-game"), + staleManagedServerNames(setOf("live-game"), setOf("live-game", "old-game")), + ) + } + + @Test + fun `an unowned static server cannot enter the removal result`() { + assertEquals(emptySet(), staleManagedServerNames(emptySet(), emptySet())) + } +} From 31301c87bc6efe6ed5b01caeff97122869d1b279 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 18:16:53 +0200 Subject: [PATCH 2/9] fix(velocity): preserve external server ownership --- .../gg/grounds/discovery/DiscoveryService.kt | 9 +++++++++ .../grounds/discovery/ServerOwnershipTest.kt | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt index 43a4e60..e833198 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt @@ -21,6 +21,12 @@ internal fun staleManagedServerNames( agonesManagedServerNames: Set, ): Set = agonesManagedServerNames - runningAgonesServerNames +internal fun shouldApplyAgonesState( + serverName: String, + currentServerNames: Set, + agonesManagedServerNames: Set, +): Boolean = serverName !in currentServerNames || serverName in agonesManagedServerNames + class DiscoveryService( private val plugin: Any, private val proxyServer: ProxyServer, @@ -208,6 +214,9 @@ class DiscoveryService( } val serverType = resolveServerType(metadata.labels) ?: continue + if (!shouldApplyAgonesState(serverName, currentServers.keys, agonesManagedServers)) { + continue + } if (serverName !in currentServers) { // Agones does not always publish the pod's address in the GameServer's diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt index f0ed7db..29b9f47 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt @@ -1,6 +1,8 @@ package gg.grounds.discovery import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test class ServerOwnershipTest { @@ -17,4 +19,22 @@ class ServerOwnershipTest { fun `an unowned static server cannot enter the removal result`() { assertEquals(emptySet(), staleManagedServerNames(emptySet(), emptySet())) } + + @Test + fun `an unowned current Velocity registration does not receive an Agones role`() { + assertFalse( + shouldApplyAgonesState( + serverName = "external-server", + currentServerNames = setOf("external-server"), + agonesManagedServerNames = emptySet(), + ) + ) + assertTrue( + shouldApplyAgonesState( + serverName = "agones-server", + currentServerNames = setOf("agones-server"), + agonesManagedServerNames = setOf("agones-server"), + ) + ) + } } From 98c8ccdf998226efdb82ea8e2e0b6cad0ffbaacc Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 18:29:13 +0200 Subject: [PATCH 3/9] fix(velocity): preserve registered server ownership --- .../gg/grounds/discovery/DiscoveryConfig.kt | 5 +- .../gg/grounds/discovery/DiscoveryService.kt | 126 ++++++++------ .../grounds/discovery/DiscoveryConfigTest.kt | 13 ++ .../grounds/discovery/DiscoveryServiceTest.kt | 163 ++++++++++++++++++ .../grounds/discovery/ServerOwnershipTest.kt | 20 --- 5 files changed, 257 insertions(+), 70 deletions(-) create mode 100644 velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryServiceTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt index bbd2d02..2238227 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt @@ -1,6 +1,9 @@ package gg.grounds.discovery import java.time.Duration +import java.util.Locale + +internal fun canonicalServerName(name: String): String = name.lowercase(Locale.ROOT) /** * Discovery configuration sourced from environment variables. All keys are optional; the defaults @@ -105,7 +108,7 @@ data class DiscoveryConfig( require(host.isNotEmpty() && port != null && port in 1..65535) { "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry'" } - require(names.add(name)) { + require(names.add(canonicalServerName(name))) { "Invalid GROUNDS_STATIC_SERVERS entry '$trimmedEntry': duplicate name '$name'" } diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt index e833198..a0e6d11 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt @@ -21,17 +21,29 @@ internal fun staleManagedServerNames( agonesManagedServerNames: Set, ): Set = agonesManagedServerNames - runningAgonesServerNames -internal fun shouldApplyAgonesState( - serverName: String, - currentServerNames: Set, - agonesManagedServerNames: Set, -): Boolean = serverName !in currentServerNames || serverName in agonesManagedServerNames +private fun createCustomObjectsApi(config: DiscoveryConfig, logger: Logger): CustomObjectsApi? = + try { + val client = Config.defaultClient() + Configuration.setDefaultApiClient(client) + CustomObjectsApi(client) + } catch (error: Throwable) { + logger.warn( + "Failed to initialize Agones discovery client (namespace={}, labelSelector={})", + config.namespace, + config.labelSelector, + error, + ) + null + } class DiscoveryService( private val plugin: Any, private val proxyServer: ProxyServer, private val logger: Logger, private val config: DiscoveryConfig = DiscoveryConfig.fromEnv(), + private val kubernetesClientFactory: () -> CustomObjectsApi? = { + createCustomObjectsApi(config, logger) + }, ) { private val gson = Gson() private lateinit var customObjectsApi: CustomObjectsApi @@ -39,16 +51,17 @@ class DiscoveryService( private lateinit var pollTask: ScheduledTask private val lobbyServers: MutableSet = ConcurrentHashMap.newKeySet() private val serverRoles: MutableMap = ConcurrentHashMap() - private val agonesManagedServers: MutableSet = ConcurrentHashMap.newKeySet() - private val staticServerNames = config.staticServers.mapTo(mutableSetOf()) { it.name } + private val agonesManagedServers: MutableMap = ConcurrentHashMap() + private val staticServerNames = + config.staticServers.mapTo(mutableSetOf()) { canonicalServerName(it.name) } @Volatile private var countsSnapshot: Pair?>? = null fun start() { - unregisterPreconfiguredServers() + unregisterBakedInPlaceholders() registerStaticServers() registerListeners() - customObjectsApi = createCustomObjectsApi() ?: return + customObjectsApi = kubernetesClientFactory() ?: return // Same client, already configured as the default above. coreApi = CoreV1Api() schedulePolling() @@ -60,35 +73,27 @@ class DiscoveryService( } } - fun getServerRole(serverName: String): String? = serverRoles[serverName] + fun getServerRole(serverName: String): String? = serverRoles[canonicalServerName(serverName)] - private fun createCustomObjectsApi(): CustomObjectsApi? { - return try { - val client = Config.defaultClient() - Configuration.setDefaultApiClient(client) - CustomObjectsApi(client) - } catch (error: Throwable) { - logger.warn( - "Failed to initialize Agones discovery client (namespace={}, labelSelector={})", - config.namespace, - config.labelSelector, - error, - ) - null - } - } - - private fun unregisterPreconfiguredServers() { - val configuredServers = proxyServer.allServers.toList() - for (server in configuredServers) { + private fun unregisterBakedInPlaceholders() { + for (server in proxyServer.allServers.filter(::isBakedInPlaceholder)) { proxyServer.unregisterServer(server.serverInfo) logger.info( - "Removed pre-configured server successfully (serverName={})", + "Removed baked-in placeholder server successfully (serverName={})", server.serverInfo.name, ) } } + private fun isBakedInPlaceholder(server: RegisteredServer): Boolean { + val serverInfo = server.serverInfo + return BAKED_IN_PLACEHOLDERS.any { placeholder -> + canonicalServerName(placeholder.name) == canonicalServerName(serverInfo.name) && + placeholder.host == serverInfo.address.hostString && + placeholder.port == serverInfo.address.port + } + } + private fun registerStaticServers() { for (server in config.staticServers) { proxyServer.registerServer( @@ -97,7 +102,7 @@ class DiscoveryService( InetSocketAddress.createUnresolved(server.host, server.port), ) ) - serverRoles[server.name] = STATIC_SERVER_ROLE + serverRoles[canonicalServerName(server.name)] = STATIC_SERVER_ROLE logger.info( "Registered static proxy server successfully (serverName={}, host={}, port={})", server.name, @@ -141,7 +146,8 @@ class DiscoveryService( private fun updateRegisteredGameServers() { val runningGameServers = fetchRunningGameServers() - val currentServers = proxyServer.allServers.associateBy { it.serverInfo.name } + val currentServers = + proxyServer.allServers.associateBy { canonicalServerName(it.serverInfo.name) } registerRunningServers(runningGameServers, currentServers) unregisterServersThatAreNoLongerRunning(runningGameServers, currentServers) @@ -188,7 +194,7 @@ class DiscoveryService( } } - private fun registerRunningServers( + internal fun registerRunningServers( runningGameServers: List, currentServers: Map, ) { @@ -205,7 +211,8 @@ class DiscoveryService( continue } - if (serverName in staticServerNames) { + val canonicalName = canonicalServerName(serverName) + if (canonicalName in staticServerNames) { logger.warn( "Skipping Agones GameServer because its name collides with a static server (serverName={})", serverName, @@ -214,11 +221,16 @@ class DiscoveryService( } val serverType = resolveServerType(metadata.labels) ?: continue - if (!shouldApplyAgonesState(serverName, currentServers.keys, agonesManagedServers)) { + val currentServer = currentServers[canonicalName] + if (currentServer != null && agonesManagedServers[canonicalName] !== currentServer) { + agonesManagedServers.remove(canonicalName)?.let { ownedServer -> + lobbyServers.remove(ownedServer.serverInfo.name) + serverRoles.remove(canonicalName) + } continue } - if (serverName !in currentServers) { + if (currentServer == null) { // Agones does not always publish the pod's address in the GameServer's // status. The bundle's lobby fleets carry Hostname, InternalIP AND // PodIP; the fleets forge renders for a pushed gamemode carry only the @@ -245,8 +257,8 @@ class DiscoveryService( } val serverInfo = ServerInfo(serverName, InetSocketAddress(address, config.port)) - proxyServer.registerServer(serverInfo) - agonesManagedServers.add(serverName) + val registeredServer = proxyServer.registerServer(serverInfo) + agonesManagedServers[canonicalName] = registeredServer logger.info( "Registered proxy server successfully (serverName={}, serverType={})", serverName, @@ -254,7 +266,7 @@ class DiscoveryService( ) } - serverRoles[serverName] = serverType + serverRoles[canonicalName] = serverType if (serverType == config.lobbyValue) { lobbyServers.add(serverName) } else { @@ -274,20 +286,30 @@ class DiscoveryService( else -> labels[config.lobbyLabel] } - private fun unregisterServersThatAreNoLongerRunning( + internal fun unregisterServersThatAreNoLongerRunning( runningGameServers: List, currentServers: Map, ) { - val runningServerNames = runningGameServers.mapNotNull { it.metadata?.name }.toSet() + val runningServerNames = + runningGameServers + .mapNotNull { it.metadata?.name } + .mapTo(mutableSetOf(), ::canonicalServerName) - for (serverName in staleManagedServerNames(runningServerNames, agonesManagedServers)) { - currentServers[serverName]?.let { server -> - proxyServer.unregisterServer(server.serverInfo) - logger.info("Unregistered proxy server successfully (serverName={})", serverName) - } - lobbyServers.remove(serverName) - serverRoles.remove(serverName) - agonesManagedServers.remove(serverName) + for (canonicalName in + staleManagedServerNames(runningServerNames, agonesManagedServers.keys)) { + val ownedServer = agonesManagedServers[canonicalName] ?: continue + currentServers[canonicalName] + ?.takeIf { it === ownedServer } + ?.let { server -> + proxyServer.unregisterServer(server.serverInfo) + logger.info( + "Unregistered proxy server successfully (serverName={})", + ownedServer.serverInfo.name, + ) + } + lobbyServers.remove(ownedServer.serverInfo.name) + serverRoles.remove(canonicalName) + agonesManagedServers.remove(canonicalName, ownedServer) } } @@ -296,6 +318,12 @@ class DiscoveryService( private const val VERSION = "v1" private const val PLURAL = "gameservers" private const val STATIC_SERVER_ROLE = "static" + private val BAKED_IN_PLACEHOLDERS = + setOf( + StaticServer("lobby", "127.0.0.1", 30066), + StaticServer("factions", "127.0.0.1", 30067), + StaticServer("minigames", "127.0.0.1", 30068), + ) private val COUNTS_TTL_NANOS = TimeUnit.SECONDS.toNanos(2) } } diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt index 385d5c8..fa2f54f 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt @@ -201,4 +201,17 @@ class DiscoveryConfigTest { ) } } + + @Test + fun `static servers reject duplicate names that differ only by case`() { + assertThrows(IllegalArgumentException::class.java) { + DiscoveryConfig.fromEnv( + env = + mapOf( + "GROUNDS_STATIC_SERVERS" to + "BuildServer=buildserver:25565,buildserver=other:25566" + ) + ) + } + } } diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryServiceTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryServiceTest.kt new file mode 100644 index 0000000..eebcb3d --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryServiceTest.kt @@ -0,0 +1,163 @@ +package gg.grounds.discovery + +import com.velocitypowered.api.event.EventManager +import com.velocitypowered.api.proxy.ProxyServer +import com.velocitypowered.api.proxy.server.RegisteredServer +import com.velocitypowered.api.proxy.server.ServerInfo +import java.lang.reflect.Proxy +import java.net.InetSocketAddress +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import org.slf4j.LoggerFactory + +class DiscoveryServiceTest { + + @Test + fun `startup removes only the exact baked image placeholders`() { + val removed = mutableListOf() + val placeholder = registeredServer("lobby", "127.0.0.1", 30066) + val sameNameDifferentAddress = registeredServer("lobby", "10.0.0.10", 25565) + val external = registeredServer("external", "10.0.0.11", 25565) + val service = + DiscoveryService( + plugin = Any(), + proxyServer = + proxyServer(listOf(placeholder, sameNameDifferentAddress, external), removed), + logger = LoggerFactory.getLogger(javaClass), + config = DiscoveryConfig.fromEnv(emptyMap()), + kubernetesClientFactory = { null }, + ) + + service.start() + + assertEquals(listOf(placeholder.serverInfo), removed) + } + + @Test + fun `static servers register when Kubernetes initialization fails`() { + val registrations = mutableListOf() + val service = + DiscoveryService( + plugin = Any(), + proxyServer = proxyServer(emptyList(), registrations = registrations), + logger = LoggerFactory.getLogger(javaClass), + config = + DiscoveryConfig.fromEnv( + mapOf("GROUNDS_STATIC_SERVERS" to "buildserver=buildserver:25565") + ), + kubernetesClientFactory = { null }, + ) + + service.start() + + assertEquals( + listOf( + ServerInfo("buildserver", InetSocketAddress.createUnresolved("buildserver", 25565)) + ), + registrations, + ) + assertEquals("static", service.getServerRole("buildserver")) + } + + @Test + fun `replacement of an Agones registration is never unregistered as owned`() { + val agonesRegistration = registeredServer("game", "10.0.0.1", 25565) + val externalReplacement = registeredServer("game", "10.0.0.2", 25565) + val removed = mutableListOf() + val service = + DiscoveryService( + plugin = Any(), + proxyServer = + proxyServer( + emptyList(), + removed, + registrations = mutableListOf(), + registered = agonesRegistration, + ), + logger = LoggerFactory.getLogger(javaClass), + config = DiscoveryConfig.fromEnv(emptyMap()), + kubernetesClientFactory = { null }, + ) + val gameServer = + GameServer( + metadata = + Metadata(name = "game", labels = mapOf("grounds/server-type" to "lobby")), + status = + Status( + state = "Ready", + addresses = listOf(GameServerAddress("10.0.0.1", "PodIP")), + ), + ) + + service.registerRunningServers(listOf(gameServer), emptyMap()) + service.registerRunningServers( + listOf( + gameServer.copy( + metadata = + Metadata(name = "game", labels = mapOf("grounds/server-type" to "game")) + ) + ), + mapOf("game" to externalReplacement), + ) + assertEquals(null, service.getServerRole("game")) + service.unregisterServersThatAreNoLongerRunning( + emptyList(), + mapOf("game" to externalReplacement), + ) + + assertTrue(removed.isEmpty()) + } + + private fun proxyServer( + servers: List, + removed: MutableList = mutableListOf(), + registrations: MutableList = mutableListOf(), + registered: RegisteredServer? = null, + ): ProxyServer = + proxy( + mapOf( + "getAllServers" to servers, + "getEventManager" to proxy(), + "unregisterServer" to + { args: Array -> + removed.add(args.single() as ServerInfo) + }, + "registerServer" to + { args: Array -> + val serverInfo = args.single() as ServerInfo + registrations.add(serverInfo) + registered + ?: registeredServer( + serverInfo.name, + serverInfo.address.hostString, + serverInfo.address.port, + ) + }, + ) + ) + + private fun registeredServer(name: String, host: String, port: Int): RegisteredServer = + proxy( + mapOf( + "getServerInfo" to ServerInfo(name, InetSocketAddress.createUnresolved(host, port)) + ) + ) + + @Suppress("UNCHECKED_CAST") + private inline fun proxy(responses: Map = emptyMap()): T = + Proxy.newProxyInstance(T::class.java.classLoader, arrayOf(T::class.java)) { _, method, args + -> + val response = responses[method.name] + when { + response is Function1<*, *> -> + (response as (Array) -> Any?)(args.orEmpty()) + response != null -> response + method.returnType == Boolean::class.javaPrimitiveType -> false + method.returnType == Int::class.javaPrimitiveType -> 0 + method.returnType == Long::class.javaPrimitiveType -> 0L + method.returnType == Void.TYPE -> null + else -> null + } + } as T +} diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt index 29b9f47..f0ed7db 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt @@ -1,8 +1,6 @@ package gg.grounds.discovery import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertFalse -import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test class ServerOwnershipTest { @@ -19,22 +17,4 @@ class ServerOwnershipTest { fun `an unowned static server cannot enter the removal result`() { assertEquals(emptySet(), staleManagedServerNames(emptySet(), emptySet())) } - - @Test - fun `an unowned current Velocity registration does not receive an Agones role`() { - assertFalse( - shouldApplyAgonesState( - serverName = "external-server", - currentServerNames = setOf("external-server"), - agonesManagedServerNames = emptySet(), - ) - ) - assertTrue( - shouldApplyAgonesState( - serverName = "agones-server", - currentServerNames = setOf("agones-server"), - agonesManagedServerNames = setOf("agones-server"), - ) - ) - } } From d99b49592da06e022b0abdfa7ab0160f3bd2e710 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 18:35:59 +0200 Subject: [PATCH 4/9] chore: remove implementation notes --- .../2026-08-23-static-server-discovery.md | 126 ------------------ ...26-08-23-static-server-discovery-design.md | 27 ---- 2 files changed, 153 deletions(-) delete mode 100644 docs/superpowers/plans/2026-08-23-static-server-discovery.md delete mode 100644 docs/superpowers/specs/2026-08-23-static-server-discovery-design.md diff --git a/docs/superpowers/plans/2026-08-23-static-server-discovery.md b/docs/superpowers/plans/2026-08-23-static-server-discovery.md deleted file mode 100644 index 163e414..0000000 --- a/docs/superpowers/plans/2026-08-23-static-server-discovery.md +++ /dev/null @@ -1,126 +0,0 @@ -# Static Server Discovery Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Add explicitly configured static Velocity backends without allowing Agones reconciliation to delete them. - -**Architecture:** Parse a strict optional `GROUNDS_STATIC_SERVERS` value into typed `StaticServer` entries in `DiscoveryConfig`. `DiscoveryService` registers those entries independently at startup and tracks only Agones-owned names for later removal. - -**Tech Stack:** Kotlin, Velocity API, JUnit 5, Gradle - -**Spec:** `docs/superpowers/specs/2026-08-23-static-server-discovery-design.md` - -## Global Constraints - -- The environment variable is exactly `GROUNDS_STATIC_SERVERS`. -- Its format is a comma-separated list of `name=host:port` entries. -- Absent or blank configuration preserves current behavior and yields no static servers. -- Names and hosts are non-empty, ports are in `1..65535`, duplicate names are rejected, and malformed entries fail configuration. -- Static servers have role `static` and are never added to the lobby set. -- Agones cleanup may unregister only names previously registered and tracked by Agones discovery. -- A configured static server wins a name collision with an Agones GameServer. -- Static registration remains available if Kubernetes client initialization fails. - ---- - -### Task 1: Static configuration and ownership-aware registration - -**Files:** -- Modify: `velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryConfig.kt` -- Modify: `velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt` -- Modify: `velocity/src/test/kotlin/gg/grounds/discovery/DiscoveryConfigTest.kt` -- Create: `velocity/src/test/kotlin/gg/grounds/discovery/ServerOwnershipTest.kt` -- Modify: `README.md` - -**Interfaces:** -- Produces: `data class StaticServer(val name: String, val host: String, val port: Int)`. -- Produces: `DiscoveryConfig.staticServers: List` sourced from `GROUNDS_STATIC_SERVERS`. -- Produces: an internal pure ownership helper used by `DiscoveryService` that returns stale names from the Agones-managed set only. - -- [ ] **Step 1: Write failing configuration tests** - -Add tests demonstrating these literal outcomes: - -```kotlin -assertEquals(emptyList(), DiscoveryConfig.fromEnv(emptyMap()).staticServers) -assertEquals( - listOf( - StaticServer("buildserver", "buildserver", 25565), - StaticServer("metrics", "metrics.stage.svc.cluster.local", 25566), - ), - DiscoveryConfig.fromEnv( - mapOf( - "GROUNDS_STATIC_SERVERS" to - " buildserver=buildserver:25565, metrics=metrics.stage.svc.cluster.local:25566 " - ) - ).staticServers, -) -``` - -Add separate `assertThrows` cases for a missing `=`, empty name, empty host, port `0`, port `65536`, a non-numeric port, and duplicate names. - -- [ ] **Step 2: Run the configuration tests and verify RED** - -Run: - -```bash -./gradlew --no-daemon :velocity:test --tests gg.grounds.discovery.DiscoveryConfigTest -``` - -Expected: compilation or assertion failure because `StaticServer` and `staticServers` do not exist. - -- [ ] **Step 3: Implement strict static-server parsing** - -Add `StaticServer`, add `staticServers` to `DiscoveryConfig`, document `GROUNDS_STATIC_SERVERS`, and parse the optional value. Blank input returns an empty list. Split entries on commas, split each entry once on `=`, split the address at the final `:`, trim fields, validate all constraints, and throw `IllegalArgumentException` with the offending entry but without secrets or unrelated environment values. - -- [ ] **Step 4: Run the configuration tests and verify GREEN** - -Run the command from Step 2. Expected: PASS. - -- [ ] **Step 5: Write failing ownership tests** - -Create `ServerOwnershipTest` for the internal pure helper. It must prove: - -```kotlin -assertEquals(setOf("old-game"), staleManagedServerNames(setOf("live-game"), setOf("live-game", "old-game"))) -assertEquals(emptySet(), staleManagedServerNames(emptySet(), emptySet())) -``` - -The test names must state that only missing Agones-owned servers become stale and an unowned static server cannot enter the removal result. - -- [ ] **Step 6: Run the ownership test and verify RED** - -Run: - -```bash -./gradlew --no-daemon :velocity:test --tests gg.grounds.discovery.ServerOwnershipTest -``` - -Expected: compilation failure because the helper does not exist. - -- [ ] **Step 7: Implement static registration and Agones ownership tracking** - -Change startup order to remove image placeholders, register all configured static servers with `ServerInfo(name, InetSocketAddress.createUnresolved(host, port))`, assign role `static`, and register listeners before attempting Kubernetes initialization. Only initialize `coreApi` and schedule polling when the Kubernetes client is available. - -Track successful/current Agones registrations in a concurrent set. During each poll, skip an Agones server whose name belongs to a configured static server and log the collision. Cleanup must call the tested helper with running Agones names and the tracked Agones-owned set, unregister only the returned names, and clear their lobby/role/ownership state. It must not iterate over all Velocity registrations as removal candidates. - -- [ ] **Step 8: Document the environment variable** - -Add `GROUNDS_STATIC_SERVERS`, its `name=host:port` syntax, strict validation, and the Stage example to `README.md` without claiming that the backend itself is automatically configured for Velocity forwarding. - -- [ ] **Step 9: Run formatting and the complete Velocity test suite** - -Run: - -```bash -./gradlew --no-daemon spotlessApply :velocity:test -``` - -Expected: `BUILD SUCCESSFUL`. - -- [ ] **Step 10: Commit** - -```bash -git add README.md docs velocity/src -git commit -m "feat(velocity): support static server discovery" -``` diff --git a/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md b/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md deleted file mode 100644 index bd3bf3e..0000000 --- a/docs/superpowers/specs/2026-08-23-static-server-discovery-design.md +++ /dev/null @@ -1,27 +0,0 @@ -# Static Server Discovery Design - -## Goal - -Allow Velocity proxies running `plugin-agones` to register a small set of explicitly configured, non-Agones backends such as the temporary Stage buildserver. - -## Configuration - -`GROUNDS_STATIC_SERVERS` is optional. Its value is a comma-separated list of `name=host:port` entries, for example: - -```text -buildserver=buildserver:25565 -``` - -Whitespace around entries is ignored. Names and hosts must be non-empty, ports must be in `1..65535`, and duplicate names or malformed entries fail plugin configuration rather than being silently ignored. An absent or blank value means no static servers and preserves current production behavior. - -## Ownership and reconciliation - -At startup the plugin continues removing the placeholder backends baked into the Velocity image, then registers the configured static servers. Static servers use the role `static`; they are never lobby candidates. - -Agones polling owns only servers it registered from Agones. Reconciliation may remove a previously managed Agones server that is no longer running, but must never remove a configured static server or a backend owned by another plugin. A static name takes precedence over an Agones GameServer with the same name and the collision is logged. - -Static registration must still work when the Kubernetes client cannot initialize. Agones polling may remain disabled in that case. - -## Deployment path - -The Stage proxies will receive `GROUNDS_STATIC_SERVERS=buildserver=buildserver:25565` after the plugin is released and bundled into a Velocity image. The buildserver will then be switched from public online-mode access to a ClusterIP backend using modern Velocity forwarding. That deployment work is intentionally separate from this plugin change. From b3be8b083154f4f5f2fbdd15aaecbe9b6eba3e46 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 19:07:02 +0200 Subject: [PATCH 5/9] feat(velocity): preserve static backend on drain --- .../discovery/DiscoveryPlayerListener.kt | 100 +++++++++++++++++- .../gg/grounds/discovery/DiscoveryService.kt | 8 +- .../kotlin/gg/grounds/drain/DrainManager.kt | 34 ++++-- .../gg/grounds/drain/DrainTransferCookie.kt | 53 ++++++++++ .../DrainStaticServerSelectionTest.kt | 45 ++++++++ .../gg/grounds/drain/DrainDecisionTest.kt | 14 ++- .../grounds/drain/DrainTransferCookieTest.kt | 38 +++++++ 7 files changed, 275 insertions(+), 17 deletions(-) create mode 100644 velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt create mode 100644 velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt create mode 100644 velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt index d4147a8..2b33dcb 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt @@ -1,23 +1,48 @@ package gg.grounds.discovery +import com.velocitypowered.api.event.Continuation +import com.velocitypowered.api.event.EventTask import com.velocitypowered.api.event.ResultedEvent import com.velocitypowered.api.event.Subscribe import com.velocitypowered.api.event.connection.LoginEvent +import com.velocitypowered.api.event.player.CookieReceiveEvent import com.velocitypowered.api.event.player.PlayerChooseInitialServerEvent +import com.velocitypowered.api.network.ProtocolVersion import com.velocitypowered.api.proxy.ProxyServer import com.velocitypowered.api.proxy.server.RegisteredServer +import com.velocitypowered.api.scheduler.ScheduledTask +import gg.grounds.drain.DrainTransferCookie +import java.util.UUID +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.TimeUnit import net.kyori.adventure.text.Component +internal fun selectDrainStaticServer( + serverName: String, + servers: Collection, + serverRole: (String) -> String?, +): RegisteredServer? = + servers.firstOrNull { server -> + server.serverInfo.name == serverName && + serverRole(server.serverInfo.name) == STATIC_SERVER_ROLE + } + +private const val STATIC_SERVER_ROLE = "static" + class DiscoveryPlayerListener( + private val plugin: Any, private val proxyServer: ProxyServer, private val lobbyServers: Set, + private val serverRole: (String) -> String?, /** * Network-wide players per backend server, or null when the network cannot be asked. Null falls * back to this proxy's own view — on a single proxy that is the same number, and with several * it still spreads, just per proxy rather than per network. */ private val networkCounts: () -> Map?, + private val drainTransferCookie: DrainTransferCookie = DrainTransferCookie(), ) { + private val pendingCookies = ConcurrentHashMap() @Subscribe fun onLogin(event: LoginEvent) { @@ -32,13 +57,58 @@ class DiscoveryPlayerListener( } @Subscribe - fun onPlayerChooseInitialServer(event: PlayerChooseInitialServerEvent) { - if (event.initialServer.isPresent) return + fun onPlayerChooseInitialServer(event: PlayerChooseInitialServerEvent): EventTask = + EventTask.withContinuation { continuation -> + if (event.initialServer.isPresent) { + continuation.resume() + return@withContinuation + } + val player = event.player + if (player.protocolVersion < ProtocolVersion.MINECRAFT_1_20_5) { + chooseServer(event, null) + continuation.resume() + return@withContinuation + } - val lobby = findLobbyServer() - if (lobby != null) { - event.setInitialServer(lobby) + val pending = PendingCookieRequest(event, continuation) + pendingCookies.put(player.uniqueId, pending)?.complete(null) + try { + player.requestCookie(DrainTransferCookie.KEY) + pending.timeoutTask = + proxyServer.scheduler + .buildTask( + plugin, + Runnable { + if (pendingCookies.remove(player.uniqueId, pending)) + pending.complete(null) + }, + ) + .delay(COOKIE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS) + .schedule() + } catch (error: IllegalArgumentException) { + if (pendingCookies.remove(player.uniqueId, pending)) pending.complete(null) + } } + + @Subscribe + fun onCookieReceive(event: CookieReceiveEvent) { + if (event.originalKey != DrainTransferCookie.KEY) return + event.result = CookieReceiveEvent.ForwardResult.handled() + pendingCookies.remove(event.player.uniqueId)?.complete(event.originalData) + } + + private fun chooseServer(event: PlayerChooseInitialServerEvent, payload: ByteArray?) { + if (event.initialServer.isPresent) return + val preferred = + drainTransferCookie.decode(payload)?.let { serverName -> + selectDrainStaticServer(serverName, proxyServer.allServers, serverRole) + } + if (preferred != null) { + event.player.storeCookie(DrainTransferCookie.KEY, byteArrayOf()) + event.setInitialServer(preferred) + return + } + findLobbyServer()?.let(event::setInitialServer) } private fun findLobbyServer(): RegisteredServer? { @@ -57,4 +127,24 @@ class DiscoveryPlayerListener( val chosen = LobbySelection.pick(candidates) ?: return null return lobbies.firstOrNull { it.serverInfo.name == chosen } } + + private inner class PendingCookieRequest( + private val event: PlayerChooseInitialServerEvent, + private val continuation: Continuation, + ) { + var timeoutTask: ScheduledTask? = null + + fun complete(payload: ByteArray?) { + timeoutTask?.cancel() + try { + chooseServer(event, payload) + } finally { + continuation.resume() + } + } + } + + private companion object { + private const val COOKIE_TIMEOUT_MILLIS = 250L + } } diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt index a0e6d11..a968ee8 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt @@ -115,7 +115,13 @@ class DiscoveryService( private fun registerListeners() { proxyServer.eventManager.register( plugin, - DiscoveryPlayerListener(proxyServer, lobbyServers, this::networkCountsCached), + DiscoveryPlayerListener( + plugin, + proxyServer, + lobbyServers, + this::getServerRole, + this::networkCountsCached, + ), ) } diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt index 45e9cd5..28e8117 100644 --- a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt @@ -21,8 +21,8 @@ import org.slf4j.Logger * becomes the transfer), or the pod's termination ends the session. A transfer would end the * round just as surely, only earlier. * - * "Inside a round" is decided by the server's `grounds/server-type` role: anything that is not the - * lobby role defers the transfer. A server that discovery has no role for cannot be a protected + * "Inside a round" is decided by the server's `grounds/server-type` role: only the `game` and + * `match` roles defer the transfer. A server that discovery has no role for cannot be a protected * round. */ class DrainManager( @@ -32,6 +32,7 @@ class DrainManager( private val config: DrainConfig, private val serverRole: (String) -> String?, private val lobbyValue: String, + private val drainTransferCookie: DrainTransferCookie = DrainTransferCookie(), ) { @Volatile var isDraining: Boolean = false @@ -102,16 +103,17 @@ class DrainManager( */ private fun transferOut(player: Player, force: Boolean): Boolean { val host = config.transferHost - val transferable = - host != null && player.protocolVersion >= ProtocolVersion.MINECRAFT_1_20_5 - if (transferable) { + if (host != null && player.protocolVersion >= ProtocolVersion.MINECRAFT_1_20_5) { + currentStaticServerName(player)?.let { serverName -> + player.storeCookie(DrainTransferCookie.KEY, drainTransferCookie.encode(serverName)) + } logger.info( "Draining player via transfer (player={}, target={}:{})", player.username, host, config.transferPort, ) - player.transferToHost(InetSocketAddress.createUnresolved(host!!, config.transferPort)) + player.transferToHost(InetSocketAddress.createUnresolved(host, config.transferPort)) return true } if (force) { @@ -124,16 +126,28 @@ class DrainManager( private fun roleOf(player: Player): String? = player.currentServer.map { it.serverInfo.name }.orElse(null)?.let(serverRole) + private fun currentStaticServerName(player: Player): String? = + player.currentServer + .map { it.serverInfo.name } + .orElse(null) + ?.takeIf { serverName -> shouldPreserveStaticBackend(serverRole(serverName)) } + companion object { val RESTART_MESSAGE: Component = Component.text("This proxy is restarting — please reconnect.") /** - * A transfer is deferred only for players on a server whose role is a real, non-lobby role: - * that is where a round can be running. No server or no role means nothing to protect. + * A transfer is deferred only for players on a real round server. No server, an unknown + * role, a lobby, or a static server means nothing to protect. */ @JvmStatic - fun shouldDefer(role: String?, lobbyValue: String): Boolean = - role != null && role != lobbyValue + @Suppress("UNUSED_PARAMETER") + fun shouldDefer(role: String?, lobbyValue: String): Boolean = role in ROUND_ROLES + + @JvmStatic + fun shouldPreserveStaticBackend(role: String?): Boolean = role == STATIC_SERVER_ROLE + + private val ROUND_ROLES = setOf("game", "match") + private const val STATIC_SERVER_ROLE = "static" } } diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt new file mode 100644 index 0000000..7b0e067 --- /dev/null +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt @@ -0,0 +1,53 @@ +package gg.grounds.drain + +import java.nio.ByteBuffer +import java.nio.charset.StandardCharsets +import java.time.Clock +import net.kyori.adventure.key.Key + +/** A short-lived destination hint written only before an automatic proxy drain transfer. */ +class DrainTransferCookie(private val clock: Clock = Clock.systemUTC()) { + + fun encode( + serverName: String, + expiresAtMillis: Long = clock.millis() + LIFETIME_MILLIS, + ): ByteArray { + val name = serverName.toByteArray(StandardCharsets.UTF_8) + require(name.isNotEmpty() && name.size <= MAX_SERVER_NAME_BYTES) { + "Static server name must be between 1 and $MAX_SERVER_NAME_BYTES UTF-8 bytes" + } + return ByteBuffer.allocate(HEADER_BYTES + name.size) + .put(VERSION) + .putLong(expiresAtMillis) + .put(name) + .array() + } + + /** Returns null for expired, malformed, or unsupported client-controlled payloads. */ + fun decode(payload: ByteArray?): String? { + if (payload == null || payload.size !in (HEADER_BYTES + 1)..MAX_PAYLOAD_BYTES) return null + + val bytes = ByteBuffer.wrap(payload) + if (bytes.get() != VERSION) return null + if (bytes.long <= clock.millis()) return null + + val name = ByteArray(bytes.remaining()) + bytes.get(name) + val serverName = name.toString(StandardCharsets.UTF_8) + if ( + serverName.isBlank() || serverName.toByteArray(StandardCharsets.UTF_8).size != name.size + ) { + return null + } + return serverName + } + + companion object { + val KEY: Key = Key.key("grounds", "drain-static-server") + private const val VERSION: Byte = 1 + private const val HEADER_BYTES = 1 + Long.SIZE_BYTES + private const val MAX_SERVER_NAME_BYTES = 64 + private const val MAX_PAYLOAD_BYTES = HEADER_BYTES + MAX_SERVER_NAME_BYTES + private const val LIFETIME_MILLIS = 30_000L + } +} diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt new file mode 100644 index 0000000..9ca89cb --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt @@ -0,0 +1,45 @@ +package gg.grounds.discovery + +import com.velocitypowered.api.proxy.server.RegisteredServer +import com.velocitypowered.api.proxy.server.ServerInfo +import java.lang.reflect.Proxy +import java.net.InetSocketAddress +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertSame +import org.junit.jupiter.api.Test + +class DrainStaticServerSelectionTest { + + @Test + fun `selects a registered static server named by the drain cookie`() { + val buildserver = registeredServer("buildserver") + + val selected = + selectDrainStaticServer("buildserver", listOf(buildserver)) { name -> + if (name == "buildserver") "static" else null + } + + assertSame(buildserver, selected) + } + + @Test + fun `does not select an Agones round server named by the drain cookie`() { + val game = registeredServer("game-7") + + val selected = selectDrainStaticServer("game-7", listOf(game)) { "game" } + + assertNull(selected) + } + + private fun registeredServer(name: String): RegisteredServer = + Proxy.newProxyInstance( + RegisteredServer::class.java.classLoader, + arrayOf(RegisteredServer::class.java), + ) { _, method, _ -> + when (method.name) { + "getServerInfo" -> + ServerInfo(name, InetSocketAddress.createUnresolved("$name.internal", 25565)) + else -> null + } + } as RegisteredServer +} diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainDecisionTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainDecisionTest.kt index b5fadc2..ce04d52 100644 --- a/velocity/src/test/kotlin/gg/grounds/drain/DrainDecisionTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainDecisionTest.kt @@ -19,8 +19,20 @@ class DrainDecisionTest { } @Test - fun `no server or unknown role is nothing to protect`() { + fun `only real round roles defer a drain`() { + assertFalse(DrainManager.shouldDefer(role = "static", lobbyValue = "lobby")) + assertFalse(DrainManager.shouldDefer(role = "lobby", lobbyValue = "lobby")) assertFalse(DrainManager.shouldDefer(role = null, lobbyValue = "lobby")) + assertFalse(DrainManager.shouldDefer(role = "unknown", lobbyValue = "lobby")) + } + + @Test + fun `only static backends are preserved across an automatic drain transfer`() { + assertTrue(DrainManager.shouldPreserveStaticBackend("static")) + assertFalse(DrainManager.shouldPreserveStaticBackend("lobby")) + assertFalse(DrainManager.shouldPreserveStaticBackend("game")) + assertFalse(DrainManager.shouldPreserveStaticBackend("match")) + assertFalse(DrainManager.shouldPreserveStaticBackend(null)) } @Test diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt new file mode 100644 index 0000000..20356f1 --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt @@ -0,0 +1,38 @@ +package gg.grounds.drain + +import java.time.Clock +import java.time.Instant +import java.time.ZoneOffset +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Test + +class DrainTransferCookieTest { + + private val now = Instant.parse("2026-08-23T16:00:00Z") + private val clock = Clock.fixed(now, ZoneOffset.UTC) + + @Test + fun `round trips a static server name before its expiry`() { + val cookie = DrainTransferCookie(clock) + + val payload = cookie.encode("buildserver") + + assertEquals("buildserver", cookie.decode(payload)) + } + + @Test + fun `rejects a cookie at its expiry`() { + val cookie = DrainTransferCookie(clock) + val payload = cookie.encode("buildserver", now.toEpochMilli()) + + assertNull(cookie.decode(payload)) + } + + @Test + fun `rejects malformed client payloads`() { + val cookie = DrainTransferCookie(clock) + + assertNull(cookie.decode(byteArrayOf(1, 2, 3))) + } +} From b2291a25ee4d871b52f2ae4ff64df55a6bfe30a1 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 19:11:13 +0200 Subject: [PATCH 6/9] fix(velocity): consume drain transfer cookies --- .../discovery/DiscoveryPlayerListener.kt | 19 ++++++++++++---- .../DrainStaticServerSelectionTest.kt | 9 ++++++++ .../DrainTransferCookieConsumptionTest.kt | 22 +++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 velocity/src/test/kotlin/gg/grounds/discovery/DrainTransferCookieConsumptionTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt index 2b33dcb..0311cf3 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt @@ -23,12 +23,20 @@ internal fun selectDrainStaticServer( serverRole: (String) -> String?, ): RegisteredServer? = servers.firstOrNull { server -> - server.serverInfo.name == serverName && + canonicalServerName(server.serverInfo.name) == canonicalServerName(serverName) && serverRole(server.serverInfo.name) == STATIC_SERVER_ROLE } private const val STATIC_SERVER_ROLE = "static" +internal fun consumeDrainTransferCookie(clearCookie: () -> Unit) { + try { + clearCookie() + } catch (_: Exception) { + // The destination choice and its continuation must not depend on client cookie storage. + } +} + class DiscoveryPlayerListener( private val plugin: Any, private val proxyServer: ProxyServer, @@ -94,7 +102,11 @@ class DiscoveryPlayerListener( fun onCookieReceive(event: CookieReceiveEvent) { if (event.originalKey != DrainTransferCookie.KEY) return event.result = CookieReceiveEvent.ForwardResult.handled() - pendingCookies.remove(event.player.uniqueId)?.complete(event.originalData) + val payload = event.originalData + consumeDrainTransferCookie { + event.player.storeCookie(DrainTransferCookie.KEY, byteArrayOf()) + } + pendingCookies.remove(event.player.uniqueId)?.complete(payload) } private fun chooseServer(event: PlayerChooseInitialServerEvent, payload: ByteArray?) { @@ -104,7 +116,6 @@ class DiscoveryPlayerListener( selectDrainStaticServer(serverName, proxyServer.allServers, serverRole) } if (preferred != null) { - event.player.storeCookie(DrainTransferCookie.KEY, byteArrayOf()) event.setInitialServer(preferred) return } @@ -145,6 +156,6 @@ class DiscoveryPlayerListener( } private companion object { - private const val COOKIE_TIMEOUT_MILLIS = 250L + private const val COOKIE_TIMEOUT_MILLIS = 1_000L } } diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt index 9ca89cb..6da3070 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt @@ -22,6 +22,15 @@ class DrainStaticServerSelectionTest { assertSame(buildserver, selected) } + @Test + fun `matches a static server name from the drain cookie case insensitively`() { + val buildserver = registeredServer("BuildServer") + + val selected = selectDrainStaticServer("buildserver", listOf(buildserver)) { "static" } + + assertSame(buildserver, selected) + } + @Test fun `does not select an Agones round server named by the drain cookie`() { val game = registeredServer("game-7") diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DrainTransferCookieConsumptionTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DrainTransferCookieConsumptionTest.kt new file mode 100644 index 0000000..992660b --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DrainTransferCookieConsumptionTest.kt @@ -0,0 +1,22 @@ +package gg.grounds.discovery + +import org.junit.jupiter.api.Assertions.assertDoesNotThrow +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class DrainTransferCookieConsumptionTest { + + @Test + fun `attempts to clear a received drain cookie even when the client clear fails`() { + var attempts = 0 + + assertDoesNotThrow { + consumeDrainTransferCookie { + attempts++ + throw IllegalStateException("client disconnected") + } + } + + assertEquals(1, attempts) + } +} From f6ef99dae4adb98342dbc07475509485ecd5b697 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 19:27:07 +0200 Subject: [PATCH 7/9] fix(velocity): confirm drain cookie before transfer --- .../kotlin/gg/grounds/GroundsPluginAgones.kt | 16 ++++- .../discovery/DiscoveryPlayerListener.kt | 20 +++++- .../gg/grounds/discovery/DiscoveryService.kt | 5 ++ .../kotlin/gg/grounds/drain/DrainListener.kt | 9 +++ .../kotlin/gg/grounds/drain/DrainManager.kt | 41 ++++++++--- .../gg/grounds/drain/DrainTransferCookie.kt | 72 +++++++++++++------ .../gg/grounds/drain/DrainTransferStager.kt | 50 +++++++++++++ .../DrainStaticServerSelectionTest.kt | 13 ++++ .../grounds/drain/DrainTransferCookieTest.kt | 30 +++++++- .../grounds/drain/DrainTransferStagerTest.kt | 31 ++++++++ 10 files changed, 251 insertions(+), 36 deletions(-) create mode 100644 velocity/src/main/kotlin/gg/grounds/drain/DrainTransferStager.kt create mode 100644 velocity/src/test/kotlin/gg/grounds/drain/DrainTransferStagerTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/GroundsPluginAgones.kt b/velocity/src/main/kotlin/gg/grounds/GroundsPluginAgones.kt index 1c8090e..ddf9aaa 100644 --- a/velocity/src/main/kotlin/gg/grounds/GroundsPluginAgones.kt +++ b/velocity/src/main/kotlin/gg/grounds/GroundsPluginAgones.kt @@ -13,6 +13,7 @@ import gg.grounds.drain.DrainConfig import gg.grounds.drain.DrainHttpServer import gg.grounds.drain.DrainListener import gg.grounds.drain.DrainManager +import gg.grounds.drain.DrainTransferCookie import gg.grounds.gameserver.GameServerStateManager import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -53,8 +54,18 @@ constructor(private val proxyServer: ProxyServer, private val logger: Logger) { stateManager = GameServerStateManager(this, proxyServer, logger, coroutineScope).also { it.start() } + val drainCookie = DrainTransferCookie(System.getenv(DrainTransferCookie.SECRET_ENV)) + lateinit var drainManager: DrainManager discoveryService = - DiscoveryService(this, proxyServer, logger, discoveryConfig).also { it.start() } + DiscoveryService( + this, + proxyServer, + logger, + discoveryConfig, + drainTransferCookie = drainCookie, + sourceCookiePending = { playerId -> drainManager.isCookiePending(playerId) }, + ) + .also { it.start() } proxyServer.commandManager.register( proxyServer.commandManager.metaBuilder("agones").build(), @@ -62,7 +73,7 @@ constructor(private val proxyServer: ProxyServer, private val logger: Logger) { ) val drainConfig = DrainConfig.fromEnv() - val drainManager = + drainManager = DrainManager( this, proxyServer, @@ -70,6 +81,7 @@ constructor(private val proxyServer: ProxyServer, private val logger: Logger) { drainConfig, { serverName -> discoveryService.getServerRole(serverName) }, discoveryConfig.lobbyValue, + drainCookie, ) proxyServer.eventManager.register(this, DrainListener(drainManager)) drainHttpServer = diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt index 0311cf3..63f0b54 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt @@ -29,6 +29,12 @@ internal fun selectDrainStaticServer( private const val STATIC_SERVER_ROLE = "static" +internal fun shouldDenyInitialLogin( + hasLobby: Boolean, + hasStatic: Boolean, + protocolVersion: ProtocolVersion, +): Boolean = !hasLobby && !(hasStatic && protocolVersion >= ProtocolVersion.MINECRAFT_1_20_5) + internal fun consumeDrainTransferCookie(clearCookie: () -> Unit) { try { clearCookie() @@ -49,12 +55,23 @@ class DiscoveryPlayerListener( */ private val networkCounts: () -> Map?, private val drainTransferCookie: DrainTransferCookie = DrainTransferCookie(), + private val sourceCookiePending: (String) -> Boolean = { false }, ) { private val pendingCookies = ConcurrentHashMap() @Subscribe fun onLogin(event: LoginEvent) { - if (findLobbyServer() != null) return + if ( + !shouldDenyInitialLogin( + findLobbyServer() != null, + proxyServer.allServers.any { server -> + serverRole(server.serverInfo.name) == STATIC_SERVER_ROLE + }, + event.player.protocolVersion, + ) + ) { + return + } event.result = ResultedEvent.ComponentResult.denied( @@ -101,6 +118,7 @@ class DiscoveryPlayerListener( @Subscribe fun onCookieReceive(event: CookieReceiveEvent) { if (event.originalKey != DrainTransferCookie.KEY) return + if (sourceCookiePending(event.player.uniqueId.toString())) return event.result = CookieReceiveEvent.ForwardResult.handled() val payload = event.originalData consumeDrainTransferCookie { diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt index a968ee8..1274603 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryService.kt @@ -44,6 +44,9 @@ class DiscoveryService( private val kubernetesClientFactory: () -> CustomObjectsApi? = { createCustomObjectsApi(config, logger) }, + private val drainTransferCookie: gg.grounds.drain.DrainTransferCookie = + gg.grounds.drain.DrainTransferCookie(), + private val sourceCookiePending: (String) -> Boolean = { false }, ) { private val gson = Gson() private lateinit var customObjectsApi: CustomObjectsApi @@ -121,6 +124,8 @@ class DiscoveryService( lobbyServers, this::getServerRole, this::networkCountsCached, + drainTransferCookie, + sourceCookiePending, ), ) } diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainListener.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainListener.kt index a5787f4..ab4c609 100644 --- a/velocity/src/main/kotlin/gg/grounds/drain/DrainListener.kt +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainListener.kt @@ -3,6 +3,7 @@ package gg.grounds.drain import com.velocitypowered.api.event.ResultedEvent import com.velocitypowered.api.event.Subscribe import com.velocitypowered.api.event.connection.LoginEvent +import com.velocitypowered.api.event.player.CookieReceiveEvent import com.velocitypowered.api.event.player.ServerPreConnectEvent class DrainListener(private val drainManager: DrainManager) { @@ -30,4 +31,12 @@ class DrainListener(private val drainManager: DrainManager) { event.result = ServerPreConnectEvent.ServerResult.denied() } } + + @Subscribe + fun onCookieReceive(event: CookieReceiveEvent) { + if (event.originalKey != DrainTransferCookie.KEY) return + if (drainManager.handleCookie(event.player, event.originalData)) { + event.result = CookieReceiveEvent.ForwardResult.handled() + } + } } diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt index 28e8117..47fac70 100644 --- a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt @@ -33,6 +33,9 @@ class DrainManager( private val serverRole: (String) -> String?, private val lobbyValue: String, private val drainTransferCookie: DrainTransferCookie = DrainTransferCookie(), + private val transferStager: DrainTransferStager = DrainTransferStager { action -> + proxy.scheduler.buildTask(plugin, Runnable(action)).delay(1, TimeUnit.SECONDS).schedule() + }, ) { @Volatile var isDraining: Boolean = false @@ -104,16 +107,27 @@ class DrainManager( private fun transferOut(player: Player, force: Boolean): Boolean { val host = config.transferHost if (host != null && player.protocolVersion >= ProtocolVersion.MINECRAFT_1_20_5) { - currentStaticServerName(player)?.let { serverName -> - player.storeCookie(DrainTransferCookie.KEY, drainTransferCookie.encode(serverName)) + val transfer = { + logger.info( + "Draining player via transfer (player={}, target={}:{})", + player.username, + host, + config.transferPort, + ) + player.transferToHost(InetSocketAddress.createUnresolved(host, config.transferPort)) + } + val payload = currentStaticServerName(player)?.let(drainTransferCookie::encode) + if (payload != null) { + transferStager.stage( + player.uniqueId.toString(), + payload, + { player.storeCookie(DrainTransferCookie.KEY, payload) }, + { player.requestCookie(DrainTransferCookie.KEY) }, + transfer, + ) + } else { + transfer() } - logger.info( - "Draining player via transfer (player={}, target={}:{})", - player.username, - host, - config.transferPort, - ) - player.transferToHost(InetSocketAddress.createUnresolved(host, config.transferPort)) return true } if (force) { @@ -132,6 +146,15 @@ class DrainManager( .orElse(null) ?.takeIf { serverName -> shouldPreserveStaticBackend(serverRole(serverName)) } + fun handleCookie(player: Player, payload: ByteArray?): Boolean { + val playerId = player.uniqueId.toString() + if (!transferStager.isPending(playerId)) return false + transferStager.onCookie(playerId, payload) + return true + } + + fun isCookiePending(playerId: String): Boolean = transferStager.isPending(playerId) + companion object { val RESTART_MESSAGE: Component = Component.text("This proxy is restarting — please reconnect.") diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt index 7b0e067..c89be71 100644 --- a/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferCookie.kt @@ -2,52 +2,82 @@ package gg.grounds.drain import java.nio.ByteBuffer import java.nio.charset.StandardCharsets +import java.security.MessageDigest import java.time.Clock +import javax.crypto.Mac +import javax.crypto.spec.SecretKeySpec import net.kyori.adventure.key.Key -/** A short-lived destination hint written only before an automatic proxy drain transfer. */ -class DrainTransferCookie(private val clock: Clock = Clock.systemUTC()) { +/** Signed, short-lived destination hint used only by an automatic proxy drain transfer. */ +class DrainTransferCookie(secret: String? = null, private val clock: Clock = Clock.systemUTC()) { + private val secret = secret?.takeIf { it.isNotBlank() }?.toByteArray(StandardCharsets.UTF_8) fun encode( serverName: String, expiresAtMillis: Long = clock.millis() + LIFETIME_MILLIS, - ): ByteArray { + ): ByteArray? { + val key = secret ?: return null val name = serverName.toByteArray(StandardCharsets.UTF_8) - require(name.isNotEmpty() && name.size <= MAX_SERVER_NAME_BYTES) { - "Static server name must be between 1 and $MAX_SERVER_NAME_BYTES UTF-8 bytes" - } - return ByteBuffer.allocate(HEADER_BYTES + name.size) - .put(VERSION) - .putLong(expiresAtMillis) - .put(name) - .array() + if (name.isEmpty() || name.size > MAX_SERVER_NAME_BYTES) return null + val issuedAtMillis = clock.millis() + val body = + ByteBuffer.allocate(HEADER_BYTES + name.size) + .put(VERSION) + .putLong(issuedAtMillis) + .putLong(expiresAtMillis) + .put(name) + .array() + return body + sign(body, key) } /** Returns null for expired, malformed, or unsupported client-controlled payloads. */ fun decode(payload: ByteArray?): String? { - if (payload == null || payload.size !in (HEADER_BYTES + 1)..MAX_PAYLOAD_BYTES) return null + val key = secret ?: return null + if (payload == null || payload.size !in (HEADER_BYTES + MAC_BYTES + 1)..MAX_PAYLOAD_BYTES) { + return null + } + val body = payload.copyOfRange(0, payload.size - MAC_BYTES) + val signature = payload.copyOfRange(payload.size - MAC_BYTES, payload.size) + if (!MessageDigest.isEqual(sign(body, key), signature)) return null - val bytes = ByteBuffer.wrap(payload) + val bytes = ByteBuffer.wrap(body) if (bytes.get() != VERSION) return null - if (bytes.long <= clock.millis()) return null + val issuedAtMillis = bytes.long + val expiresAtMillis = bytes.long + val now = clock.millis() + if ( + issuedAtMillis > now + CLOCK_SKEW_MILLIS || + expiresAtMillis <= now || + expiresAtMillis > now + LIFETIME_MILLIS + CLOCK_SKEW_MILLIS || + expiresAtMillis <= issuedAtMillis || + expiresAtMillis > issuedAtMillis + LIFETIME_MILLIS + CLOCK_SKEW_MILLIS + ) { + return null + } val name = ByteArray(bytes.remaining()) bytes.get(name) val serverName = name.toString(StandardCharsets.UTF_8) - if ( - serverName.isBlank() || serverName.toByteArray(StandardCharsets.UTF_8).size != name.size - ) { - return null + return serverName.takeIf { + it.isNotBlank() && it.toByteArray(StandardCharsets.UTF_8).size == name.size } - return serverName } + private fun sign(body: ByteArray, key: ByteArray): ByteArray = + Mac.getInstance("HmacSHA256").run { + init(SecretKeySpec(key, algorithm)) + doFinal(body) + } + companion object { val KEY: Key = Key.key("grounds", "drain-static-server") + const val SECRET_ENV = "VELOCITY_FORWARDING_SECRET" private const val VERSION: Byte = 1 - private const val HEADER_BYTES = 1 + Long.SIZE_BYTES + private const val HEADER_BYTES = 1 + Long.SIZE_BYTES + Long.SIZE_BYTES + private const val MAC_BYTES = 32 private const val MAX_SERVER_NAME_BYTES = 64 - private const val MAX_PAYLOAD_BYTES = HEADER_BYTES + MAX_SERVER_NAME_BYTES + private const val MAX_PAYLOAD_BYTES = HEADER_BYTES + MAX_SERVER_NAME_BYTES + MAC_BYTES private const val LIFETIME_MILLIS = 30_000L + private const val CLOCK_SKEW_MILLIS = 5_000L } } diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferStager.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferStager.kt new file mode 100644 index 0000000..dd27c43 --- /dev/null +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainTransferStager.kt @@ -0,0 +1,50 @@ +package gg.grounds.drain + +import java.security.MessageDigest +import java.util.concurrent.ConcurrentHashMap + +/** + * Coordinates store/request/echo before a drain transfer, with a bounded normal-transfer fallback. + */ +class DrainTransferStager(private val scheduleTimeout: ((() -> Unit) -> Unit)) { + private val pending = ConcurrentHashMap() + + fun stage( + playerId: String, + payload: ByteArray, + store: () -> Unit, + request: () -> Unit, + transfer: () -> Unit, + ) { + val stage = Pending(payload, request, transfer) + if (pending.putIfAbsent(playerId, stage) != null) return + try { + store() + request() + scheduleTimeout { complete(playerId, stage) } + } catch (_: Exception) { + complete(playerId, stage) + } + } + + fun onCookie(playerId: String, payload: ByteArray?) { + val stage = pending[playerId] ?: return + if (payload != null && MessageDigest.isEqual(stage.payload, payload)) { + complete(playerId, stage) + } else { + try { + stage.request() + } catch (_: Exception) { + complete(playerId, stage) + } + } + } + + fun isPending(playerId: String): Boolean = pending.containsKey(playerId) + + private fun complete(playerId: String, stage: Pending) { + if (pending.remove(playerId, stage)) stage.transfer() + } + + private class Pending(val payload: ByteArray, val request: () -> Unit, val transfer: () -> Unit) +} diff --git a/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt index 6da3070..1e7f030 100644 --- a/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/discovery/DrainStaticServerSelectionTest.kt @@ -1,9 +1,11 @@ package gg.grounds.discovery +import com.velocitypowered.api.network.ProtocolVersion import com.velocitypowered.api.proxy.server.RegisteredServer import com.velocitypowered.api.proxy.server.ServerInfo import java.lang.reflect.Proxy import java.net.InetSocketAddress +import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Assertions.assertSame import org.junit.jupiter.api.Test @@ -40,6 +42,17 @@ class DrainStaticServerSelectionTest { assertNull(selected) } + @Test + fun `allows a cookie-capable login to reach static selection without a lobby`() { + assertFalse( + shouldDenyInitialLogin( + hasLobby = false, + hasStatic = true, + protocolVersion = ProtocolVersion.MINECRAFT_1_20_5, + ) + ) + } + private fun registeredServer(name: String): RegisteredServer = Proxy.newProxyInstance( RegisteredServer::class.java.classLoader, diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt index 20356f1..131edc2 100644 --- a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt @@ -14,7 +14,7 @@ class DrainTransferCookieTest { @Test fun `round trips a static server name before its expiry`() { - val cookie = DrainTransferCookie(clock) + val cookie = DrainTransferCookie("secret", clock) val payload = cookie.encode("buildserver") @@ -23,7 +23,7 @@ class DrainTransferCookieTest { @Test fun `rejects a cookie at its expiry`() { - val cookie = DrainTransferCookie(clock) + val cookie = DrainTransferCookie("secret", clock) val payload = cookie.encode("buildserver", now.toEpochMilli()) assertNull(cookie.decode(payload)) @@ -31,8 +31,32 @@ class DrainTransferCookieTest { @Test fun `rejects malformed client payloads`() { - val cookie = DrainTransferCookie(clock) + val cookie = DrainTransferCookie("secret", clock) assertNull(cookie.decode(byteArrayOf(1, 2, 3))) } + + @Test + fun `rejects a cookie signed with another secret`() { + val payload = DrainTransferCookie("source-secret", clock).encode("buildserver") + + assertNull(DrainTransferCookie("target-secret", clock).decode(payload)) + } + + @Test + fun `rejects a forged cookie signature`() { + val cookie = DrainTransferCookie("secret", clock) + val payload = cookie.encode("buildserver")!! + payload[10] = (payload[10].toInt() xor 1).toByte() + + assertNull(cookie.decode(payload)) + } + + @Test + fun `rejects an expiry beyond the allowed lifetime`() { + val cookie = DrainTransferCookie("secret", clock) + val payload = cookie.encode("buildserver", now.plusSeconds(600).toEpochMilli()) + + assertNull(cookie.decode(payload)) + } } diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferStagerTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferStagerTest.kt new file mode 100644 index 0000000..ffe18bd --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferStagerTest.kt @@ -0,0 +1,31 @@ +package gg.grounds.drain + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class DrainTransferStagerTest { + @Test + fun `transfers only after the stored cookie echo is confirmed`() { + val actions = mutableListOf() + val stager = DrainTransferStager { actions += "retry" } + stager.stage("player", byteArrayOf(1), { actions += "store" }, { actions += "request" }) { + actions += "transfer" + } + stager.onCookie("player", byteArrayOf(1)) + assertEquals(listOf("store", "request", "retry", "transfer"), actions) + } + + @Test + fun `mismatch retries and timeout transfers once`() { + val actions = mutableListOf() + lateinit var timeout: () -> Unit + val stager = DrainTransferStager { timeout = it } + stager.stage("player", byteArrayOf(1), { actions += "store" }, { actions += "request" }) { + actions += "transfer" + } + stager.onCookie("player", byteArrayOf(2)) + timeout() + timeout() + assertEquals(listOf("store", "request", "request", "transfer"), actions) + } +} From dec3074ffbe23e00ec2e7e754ae8119e7b860bad Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 19:30:13 +0200 Subject: [PATCH 8/9] fix(velocity): keep drain deadline after transfer failure --- .../kotlin/gg/grounds/drain/DrainManager.kt | 39 +++++++++++++++---- .../grounds/drain/DrainTransferCookieTest.kt | 6 +++ .../grounds/drain/DrainTransferSafetyTest.kt | 25 ++++++++++++ 3 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 velocity/src/test/kotlin/gg/grounds/drain/DrainTransferSafetyTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt index 47fac70..90dd74b 100644 --- a/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt +++ b/velocity/src/main/kotlin/gg/grounds/drain/DrainManager.kt @@ -8,6 +8,20 @@ import java.util.concurrent.TimeUnit import net.kyori.adventure.text.Component import org.slf4j.Logger +internal fun transferAllSafely( + players: Iterable, + transfer: (T) -> Unit, + onFailure: (T, Exception) -> Unit, +) { + players.forEach { player -> + try { + transfer(player) + } catch (error: Exception) { + onFailure(player, error) + } + } +} + /** * Moves players off this proxy before it shuts down, instead of letting Velocity kick them. * @@ -55,16 +69,21 @@ class DrainManager( config.transferHost?.let { "$it:${config.transferPort}" } ?: "", ) - proxy.allPlayers.forEach { player -> - if (!shouldDefer(roleOf(player), lobbyValue)) { - transferOut(player, force = false) - } - } - proxy.scheduler .buildTask(plugin, Runnable { onDeadline() }) .delay(deadlineSeconds, TimeUnit.SECONDS) .schedule() + transferAllSafely( + proxy.allPlayers.filter { !shouldDefer(roleOf(it), lobbyValue) }, + { player -> transferOut(player, force = false) }, + { player, error -> + logger.warn( + "Failed to transfer draining player {}; leaving for deadline", + player.username, + error, + ) + }, + ) return true } @@ -87,7 +106,13 @@ class DrainManager( "Drain deadline reached; transferring {} players not inside a round", drainable.size, ) - drainable.forEach { transferOut(it, force = true) } + transferAllSafely( + drainable, + { player -> transferOut(player, force = true) }, + { player, error -> + logger.warn("Failed to transfer draining player {}", player.username, error) + }, + ) } if (inRound.isNotEmpty()) { logger.warn( diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt index 131edc2..166db8f 100644 --- a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferCookieTest.kt @@ -59,4 +59,10 @@ class DrainTransferCookieTest { assertNull(cookie.decode(payload)) } + + @Test + fun `blank or missing secrets disable cookie preservation`() { + assertNull(DrainTransferCookie(null, clock).encode("buildserver")) + assertNull(DrainTransferCookie(" ", clock).encode("buildserver")) + } } diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferSafetyTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferSafetyTest.kt new file mode 100644 index 0000000..67a6b0c --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainTransferSafetyTest.kt @@ -0,0 +1,25 @@ +package gg.grounds.drain + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class DrainTransferSafetyTest { + @Test + fun `a failed transfer does not stop later drain transfers`() { + val attempted = mutableListOf() + val failures = mutableListOf() + + transferAllSafely( + listOf("first", "second"), + { player -> + attempted += player + if (player == "first") error("connection closed") + }, + ) { player, _ -> + failures += player + } + + assertEquals(listOf("first", "second"), attempted) + assertEquals(listOf("first"), failures) + } +} From 8d685e188e1dba99e0487339681009f208488388 Mon Sep 17 00:00:00 2001 From: Lukas Jost Date: Sun, 23 Aug 2026 19:38:43 +0200 Subject: [PATCH 9/9] fix(velocity): order drain cookie listeners --- .../discovery/DiscoveryPlayerListener.kt | 5 ++- .../drain/DrainCookieListenerOrderingTest.kt | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 velocity/src/test/kotlin/gg/grounds/drain/DrainCookieListenerOrderingTest.kt diff --git a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt index 63f0b54..33be4ec 100644 --- a/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt +++ b/velocity/src/main/kotlin/gg/grounds/discovery/DiscoveryPlayerListener.kt @@ -115,7 +115,9 @@ class DiscoveryPlayerListener( } } - @Subscribe + // This must run before DrainListener: source-proxy cookie echoes belong to its stager, not to + // initial-server selection. Velocity invokes higher priorities first. + @Subscribe(priority = DRAIN_COOKIE_SOURCE_SUPPRESSION_PRIORITY) fun onCookieReceive(event: CookieReceiveEvent) { if (event.originalKey != DrainTransferCookie.KEY) return if (sourceCookiePending(event.player.uniqueId.toString())) return @@ -175,5 +177,6 @@ class DiscoveryPlayerListener( private companion object { private const val COOKIE_TIMEOUT_MILLIS = 1_000L + private const val DRAIN_COOKIE_SOURCE_SUPPRESSION_PRIORITY: Short = 100 } } diff --git a/velocity/src/test/kotlin/gg/grounds/drain/DrainCookieListenerOrderingTest.kt b/velocity/src/test/kotlin/gg/grounds/drain/DrainCookieListenerOrderingTest.kt new file mode 100644 index 0000000..cb6082c --- /dev/null +++ b/velocity/src/test/kotlin/gg/grounds/drain/DrainCookieListenerOrderingTest.kt @@ -0,0 +1,31 @@ +package gg.grounds.drain + +import com.velocitypowered.api.event.Subscribe +import com.velocitypowered.api.event.player.CookieReceiveEvent +import gg.grounds.discovery.DiscoveryPlayerListener +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +class DrainCookieListenerOrderingTest { + + @Test + fun `source cookie suppression runs before drain cookie completion`() { + val discoveryPriority = + DiscoveryPlayerListener::class + .java + .getDeclaredMethod("onCookieReceive", CookieReceiveEvent::class.java) + .getAnnotation(Subscribe::class.java) + .priority + val drainPriority = + DrainListener::class + .java + .getDeclaredMethod("onCookieReceive", CookieReceiveEvent::class.java) + .getAnnotation(Subscribe::class.java) + .priority + + assertTrue( + discoveryPriority > drainPriority, + "Discovery must suppress source drain cookies before DrainListener completes them", + ) + } +}