diff --git a/deploy/k8s/base/control-plane-deployment.yaml b/deploy/k8s/base/control-plane-deployment.yaml index 5225cc61..aa59a6c2 100644 --- a/deploy/k8s/base/control-plane-deployment.yaml +++ b/deploy/k8s/base/control-plane-deployment.yaml @@ -95,3 +95,22 @@ spec: secretKeyRef: name: cosmic-clash-workload key: secret + # Player sign-in. The publisher key is the credential Valve issues + # to us, never to a client, so it is mounted only here -- no other + # workload and no game server ever sees it. Both values must be + # present or POST /v1/session/steam keeps returning 503: silently + # accepting an unverified ticket would be worse than refusing to + # authenticate. Optional until the App ID exists (issue #15), so the + # Deployment still rolls out without the Secret. + - name: COSMIC_CLASH_STEAM_PUBLISHER_KEY + valueFrom: + secretKeyRef: + name: cosmic-clash-steam + key: publisher-key + optional: true + - name: COSMIC_CLASH_STEAM_APP_ID + valueFrom: + secretKeyRef: + name: cosmic-clash-steam + key: app-id + optional: true diff --git a/review-findings.md b/docs/REVIEW-2026-09-feat-multiplayer.md similarity index 92% rename from review-findings.md rename to docs/REVIEW-2026-09-feat-multiplayer.md index 4eb5c35c..77364ae9 100644 --- a/review-findings.md +++ b/docs/REVIEW-2026-09-feat-multiplayer.md @@ -1,4 +1,22 @@ -# Branch review findings +# Branch review findings — `feat/multiplayer`, September 2026 + +> **Point-in-time artefact, not living documentation.** This records the state +> of the branch at `089c127c`. **All thirteen findings below have since been +> addressed** — every one was verified against the code first, and each fix +> carries a test confirmed to fail against the defect it covers. Do not read +> the present tense here as describing current behaviour. +> +> For what is actually outstanding, see [`multiplayer-next.md`](../multiplayer-next.md) +> §0 and §7. For the design the fixes implement, see +> [`MATCHMAKING.md`](MATCHMAKING.md). It is kept because the reasoning about +> *why* each defect mattered is worth preserving, and because several fixes are +> only intelligible alongside the failure they close. +> +> Two things the review did not cover, found while fixing it and recorded in +> `multiplayer-next.md` rather than here: `predicted_rtt` was persisted as a +> JSONB scalar `null` (so `RecordProbe` could never have worked even once the +> probe endpoint was wired), and the ranked-rating gap existed on the Redis +> path too, via the candidate built at enqueue rather than the candidate query. Review scope: `feat/multiplayer` at `089c127c`, compared with merge-base `3aa0f5b9` (`origin/master`). This is a second, stricter adversarial pass over diff --git a/multiplayer-next.md b/multiplayer-next.md index fbf4743f..8056b743 100644 --- a/multiplayer-next.md +++ b/multiplayer-next.md @@ -150,7 +150,7 @@ retrofitting one. | 7.1 `[D:1.2]` | GodotSteam integration and custom export templates, client *and* headless server | Awaiting the custom binaries/SDK access | | 7.2 `[D:7.1]` | `NetTransport` Steam implementation (`SteamMultiplayerPeer`, SDR) | Server advertising waits for `ISteamGameServer` work | | 7.3 `[D:7.2]` `[P]` | Server-browser UI and `ISteamMatchmakingServers` adapter | Unimplemented until real Steam SDK/API access is available; ENet direct-IP remains the supported browser-free path meanwhile | -| 7.4 `[D:7.2]` `[P]` | Auth tickets in `hello` → `BeginAuthSession`, Steam identity in the roster, persistent ban list | Real GodotSteam auth integration, server-side VAC state, durable ban storage remain. **Fixes known defect C** for direct/community servers once landed | +| 7.4 `[D:7.2]` `[P]` | Auth tickets in `hello` → `BeginAuthSession`, Steam identity in the roster, persistent ban list | Durable ban storage landed: `identities.banned_until`/`ban_reason` are enforced on both session issuance and every authenticated request, and `ApplyIdentityBan` revokes an identity's sessions in the same transaction. Real GodotSteam auth integration and server-side VAC state remain (VAC state is read at login by the Web API adapter, but is not yet re-checked mid-session). **Fixes known defect C** for direct/community servers once landed | | 7.5 `[D:7.2]` `[P]` | `SteamBootstrap` gating (stock builds keep ENet, explicit Steam selection fails closed) | Custom Steam client/server export smoke remains blocked on pinned GodotSteam binaries | | 7.6 `[D:7.4]` | Backend `AuthCoordinator`, session persistence, `ControlPlaneClient.login_steam()`, real `ISteamUserAuth/AuthenticateUserTicket` adapter (`server/steam`), client web-API ticket acquisition, sign-in before matchmaking | Needs a real App ID and publisher key ([#15](https://github.com/jcreek/CosmicClash/issues/15)) and a custom GodotSteam build ([#16](https://github.com/jcreek/CosmicClash/issues/16)) to exercise live; sign-in is config-gated and returns 503 until both are set | | 7.7 `[D:7.1]` `[P]` | Obtain the production App ID, publisher key, SDR coordinator SDK/signing approval, certificates and hosted-data-centre support from Valve | Not started | @@ -191,14 +191,14 @@ are done; everything below is what's left on the tasks still open. | # | Task | Remaining | |---|---|---| -| 8.5 `[D:8.4]` | PostgreSQL migrations 0001–0013 (idempotency, queue fencing, identities, ratings, matches, results, audits, outbox, allocator registry, proposal plans, leases, quotas) | New validations await a live database rerun — Docker storage exhausted locally | +| 8.5 `[D:8.4]` | PostgreSQL migrations 0001–0017 (idempotency, queue fencing, identities, ratings, matches, results, audits, outbox, allocator registry, proposal plans, leases, quotas, outbox dead-letter, retention indexes, allocation endpoints, probe challenges) | Verified against a live PostgreSQL; migrations now run to 0017. The local Docker storage exhaustion is a recurring symptom, not a one-off — see §9 gotcha on the integration scripts leaking anonymous volumes | | 8.6 `[D:8.3,8.4]` | Allocated-mode `ServerConfig` fields | Signed-authorisation admission, dynamic endpoint wiring, full manifest/runtime tests remain | #### 8B — Authentication and secure control plane | # | Task | Remaining | |---|---|---| -| 8.7 `[D:7.6,8.3]` | Ticket policy binding expected App ID/identity | Real `AuthenticateUserTicket` backend adapter, bans, publisher secret store, real Steam verification remain | +| 8.7 `[D:7.6,8.3]` | Ticket policy binding expected App ID/identity | Adapter, bans and secret store landed: `server/steam` calls `ISteamUserAuth/AuthenticateUserTicket`, rejects family-shared and banned accounts, and separates a Valve outage (503) from a bad ticket (401); the publisher key is mounted into the control-plane Deployment alone from the `cosmic-clash-steam` Secret, asserted by a manifest test. Only verification against real Valve remains, which needs the App ID and key ([#15](https://github.com/jcreek/CosmicClash/issues/15)) | | 8.8 `[D:8.7]` | Session policy (opaque tokens, digests, revocation) | Distributed revocation coordination, live Steam/session integration remain | | 8.9 `[D:8.4,8.7]` | Join policy, durable reconnect leases | Live PostgreSQL/Godot process-restart and outage recovery verification remains | | 8.10 `[D:8.5,8.31]` | Workload credential policy (signed tokens, not Kubernetes JWTs), delivery channel, conflict alerting | Never run against a real Agones cluster; alert validated only statically, not against live Prometheus/Alertmanager traffic | @@ -219,7 +219,7 @@ are done; everything below is what's left on the tasks still open. | 8.21 `[D:8.5,8.20]` | Rating core (Glicko-2, weights, transactional updates) | Live maintenance/DB execution remains | | 8.22 `[D:8.21]` | Ranked profile (provisional games, tiers) | Persisted tier policy, client UI, reconnect transport remain | | 8.23 `[D:8.21]` | Ranked season policy (compression, rollover) | Live maintenance/DB execution remains | -| 8.24 `[D:8.9,8.20,8.21]` | Ranked connection policy, reconnect lease, abandon ladder | Live PostgreSQL/process-restart/outage execution remains, blocked by Docker storage | +| 8.24 `[D:8.9,8.20,8.21]` | Ranked connection policy, reconnect lease, abandon ladder | Live PostgreSQL execution now verified (`make verify-phase6` and every integration script run clean). Process-restart and outage execution remain | | 8.25 `[D:8.10,8.24]` | Result policy (workload-bound, idempotent, transactional) | Production credentials, Agones annotation persistence/reconciliation, integrity-evidence adapters remain | #### 8D — Agones, allocation and regional scaling @@ -244,7 +244,7 @@ are done; everything below is what's left on the tasks still open. | # | Task | Remaining | |---|---|---| -| 8.39 `[D:8.3,8.14,8.17]` | `MatchmakingState`/`ControlPlaneClient`, queue/proposal UI, targeted revisioned events | Live PostgreSQL-backed dispatcher/fan-out verification remains | +| 8.39 `[D:8.3,8.14,8.17]` | `MatchmakingState`/`ControlPlaneClient`, queue/proposal UI, targeted revisioned events | Cross-replica fan-out landed: committed outbox events are published through PostgreSQL LISTEN/NOTIFY so the replica owning a subscriber's WebSocket delivers it, rather than whichever replica happened to drain the row. Verified against real PostgreSQL with two listeners. Live multi-replica verification under load remains | | 8.40 `[D:8.3,8.14]` | Revisioned event stream, REST resync, outbox dispatcher | Allocator and Redis fan-out live verification remain | | 8.41 `[D:7.8,8.9,8.31,8.40]` | Player-scoped assignment API, `connect_to_assignment()` wiring, join-authorisation verification in `MatchNet` | SDR relay-ticket installation and live Agones cluster integration remain | | 8.42 `[D:8.22,8.23,8.24,8.40]` | `RankedProfileState`, backend-authoritative rating/tier display | Committed revision after reconnect, abandon status, season countdown remain dependent on live auth/backend events and Godot runtime verification | @@ -259,7 +259,7 @@ are done; everything below is what's left on the tasks still open. | 8.46 `[D:8.5,8.7,8.9,8.10,8.14,8.18,8.21,8.23,8.25]` | Go unit/race/fuzz coverage, local verification gate | Live matcher-worker-under-load-during-failover integration remains | | 8.47 `[D:8.7,8.30]` | Offline testkit (fake Steam, fake allocation) | Live exhaustive matrix and production Steam remain | | 8.48 `[D:8.10,8.14,8.17,8.18,8.27,8.31,8.35,8.47]` | Allocated Compose end-to-end (queue → proposal → allocation → assignment → result) | **Local complete; production gate open** — real Agones/kind and production evidence remain open | -| 8.49 `[D:8.25,8.26,8.28,8.29,8.30,8.31,8.35,8.36]` | Disposable kind+Agones cluster runner | CI/live evidence for production control-plane registration, roster/no-show, both readiness stages, races, multi-match node, result-pending reconciliation, drain, rollback remains open. Blocked locally on Docker storage/kind/Helm availability | +| 8.49 `[D:8.25,8.26,8.28,8.29,8.30,8.31,8.35,8.36]` | Disposable kind+Agones cluster runner | CI/live evidence for production control-plane registration, roster/no-show, both readiness stages, races, multi-match node, result-pending reconciliation, drain, rollback remains open. Blocked locally on kind/Helm availability | | 8.50 `[D:8.25,8.37,8.43,8.49]` | Chaos recovery (stale allocation, no-penalty requeue) | **Local complete; production gate open** — 100 ms RTT/jitter/loss, matcher/client restart, game-pod death, node drain, Redis failover, control-plane loss, live chaos evidence remain | | 8.51 `[D:8.17,8.18,8.30,8.31,8.45]` | 10,000-client API load gate | **Local complete; production gate open** — PostgreSQL saturation, durable matcher fencing under load, forecast launch concurrency ×2, replica scaling remain live infrastructure gates | | 8.52 `[D:8.32,8.34,8.45,8.51]` | Per-replica + shared regional allocator quota | Real image digest/secrets, measured regional cost model, threshold tuning, denial-of-wallet rehearsal remain | @@ -348,6 +348,7 @@ single-player one. 49. **Advancing a consumer cursor past data that has not arrived is not a lossy shortcut — it is permanent, because the producer-side filter then rejects the very data being waited for.** **Only give up on an expected item once strictly newer data proves it lost**; "it hasn't arrived yet" and "it will never arrive" are different states and must not share a code path. 50. **A metric that stops sampling during a failure will report that failure as healthy.** Every rate-shaped assertion needs a companion assertion on the **denominator**, or an outage silently becomes an absence of evidence and then evidence of absence. 51. **An architectural blocker inherited from a previous session is a claim to verify, not a premise to build on.** Reconstruct the failing invariant from the code and reproduce it against a control before accepting a scope estimate attached to it — especially when the recommendation arrives without the cheaper alternative recorded as tested. +52. **`docker run --rm` reclaims the container, not its anonymous volumes.** Every run of `scripts/run_*_integration.sh` leaves a throwaway PostgreSQL/Redis data volume behind. They accumulate invisibly — 64 of them, ~4 GB, after one working session — until the Docker VM disk fills and the next container silently fails to start, surfacing only as the script's own `PostgreSQL did not become ready` timeout rather than as a disk error. This is the actual cause behind the "Docker storage exhausted locally" notes elsewhere in this document. `docker system df` shows it (`Local Volumes … 100% reclaimable`); `docker volume prune` clears it. Worth checking first whenever an integration script starts timing out on a machine where it previously worked. --- diff --git a/server/security/test_kubernetes_policies.py b/server/security/test_kubernetes_policies.py index a606918d..49faada0 100644 --- a/server/security/test_kubernetes_policies.py +++ b/server/security/test_kubernetes_policies.py @@ -228,6 +228,30 @@ class KubernetesPolicyTest(unittest.TestCase): # The matcher never calls the control plane's API. self.assertNotIn("port: 8080", matcher) + def test_steam_publisher_credentials_reach_only_the_control_plane(self): + # The adapter and flags existed but no manifest supplied them, so a + # deployed control plane would have kept sign-in disabled even once the + # App ID landed -- making issue #15 unblock nothing on arrival. + deployment = self.read("control-plane-deployment.yaml") + for required in ( + "name: COSMIC_CLASH_STEAM_PUBLISHER_KEY", + "name: COSMIC_CLASH_STEAM_APP_ID", + "name: cosmic-clash-steam", + "key: publisher-key", + "key: app-id", + ): + self.assertIn(required, deployment) + # Optional until the App ID exists, so the Deployment still rolls out + # without the Secret and sign-in simply stays 503. + self.assertIn("optional: true", deployment) + + # The publisher key is issued to us, never to a client. No other + # workload -- and above all no game server -- may mount it. + for name in sorted(BASE.glob("*.yaml")): + if name.name == "control-plane-deployment.yaml": + continue + self.assertNotIn("cosmic-clash-steam", name.read_text(), name.name) + if __name__ == "__main__": unittest.main()