CosmicClash/review-findings.md

Branch review findings

Review scope: feat/multiplayer at 089c127c, compared with merge-base 3aa0f5b9 (origin/master). This is a second, stricter adversarial pass over the complete branch.

[P0] Ship runnable control-plane and matcher workloads

Location: Dockerfile:51-100, deploy/k8s/base/kustomization.yaml:3-18, deploy/k8s/base/control-plane-deployment.yaml:48-50

The Kubernetes base deploys a control-plane image, but the Dockerfile neither builds cmd/control-plane nor defines a control-plane target. Conversely, the Dockerfile does build a matcher image, but the Kubernetes base contains no matcher Deployment at all. Applying the checked-in base therefore cannot produce the advertised production topology: there is no repository-defined artifact for one required workload, and no running process that consumes queued tickets for the other. Tickets can be created but can never become proposals.

Add a production control-plane image target (not the fake-login testkit-api target), add separately configured casual and ranked matcher Deployments plus their network policies/health checks, and make the release pipeline build and pin every referenced target. Add a rendered-manifest test that asserts every required role is present and every image maps to a real Docker target.

[P0] Wire production Steam authentication and the client sign-in flow

Location: server/cmd/control-plane/main.go:129-157, server/api/service.go:321-340, Game/scripts/control_plane_client.gd:15-23, Game/scripts/control_plane_client.gd:157-168, Game/scripts/control_plane_client.gd:218-221, Game/scripts/main_menu.gd:194-195

newAPIService never supplies SteamLogin, so the production POST /v1/session/steam handler always returns 503 auth_unavailable. On the other side, the game starts with an empty token and a localhost base URL; it has configure and login_steam methods, but no production code calls either one and the menu enters matchmaking directly. All matchmaking HTTP operations then fail locally with ERR_UNAUTHORIZED. Only cmd/testkit-api supplies an authentication provider, so the passing integration path is not a deployable or secure player path.

Implement and configure the real Steam ticket adapter, expose explicit control-plane endpoint configuration for release builds, obtain a Steam Web API ticket through the platform integration, and complete login before enabling Find Match. Add an end-to-end test using the production binary wiring (with the external Steam boundary stubbed), rather than the testkit service.

[P0] Populate server-derived RTT or every queued candidate is invalid

Location: server/cmd/control-plane/main.go:134-154, server/api/service.go:1143-1168, server/store/queue_sql.go:134-181, server/store/queue_sql.go:208-227, server/domain/matcher.go:159-168, Game/scripts/control_plane_client.gd:205-221, server/api/service.go:1175-1181

Queue creation persists an empty predicted_rtt map, while validCandidate rejects every candidate whose map remains empty. The production control plane sets ProbeRecorder but never sets the Probe provider, so the probe endpoint always returns 503 probe_unavailable; the Godot client also implements no probe request at all. As a result, even if a matcher Deployment is added, no real client-created ticket can participate in a formation. There is a second cache-coherency failure behind that blocker: a successful probe updates only PostgreSQL and does not refresh CandidateIndex, leaving a previously inserted Redis candidate with its empty RTT map. In a busy shared keyspace whose TTL is continually refreshed, that stale candidate need not repair itself.

Wire regional probe adapters into the production service and have the client complete authenticated probe collection for supported regions after queuing (or before making a candidate visible to the matcher), and update/invalidate the Redis projection after probe persistence. Add a full production-wiring test proving a newly logged-in client can acquire RTT evidence and be selected through both the PostgreSQL and Redis paths without direct database seeding.

[P0] Publish signed assignment rosters before starting allocated servers

Location: server/allocator/worker.go:34-79, server/cmd/allocator/main.go:81-93, server/allocator/service.go:84-91, server/store/assignment_sql.go:178-331, server/supervisor/supervisor.go:198-224, server/supervisor/supervisor.go:313-388

The worker stops after binding the provider allocation. Although Service.PublishRoster and SaveVerifiedAssignmentRoster exist, the production allocator configures no roster store/signing key and never calls them. The allocated supervisor fetches a non-empty roster before it launches the game child, so every real allocation fails at that fetch and can never reach assignment-ready or accept a player. Existing tests seed assignments directly and therefore bypass the missing production hand-off.

Define the signing-key ownership and rotation model, build one signed join authorisation per participant, persist the assignment and roster atomically with the allocation transition, and make retries idempotent. Exercise the real allocator worker through supervisor startup without fixture-seeding the assignment tables.

[P0] Allow both game traffic and workload callbacks through NetworkPolicy

Location: deploy/k8s/base/network-policies.yaml:1-92, deploy/k8s/base/fleet.yaml:54-83

The namespace-wide policy selects every pod and denies ingress and egress. No ingress policy allows UDP/7777 to game-server pods, so public players cannot reach an allocated ENet server. Independently, game-server egress permits TCP 8080 to the control plane, but control-plane ingress permits only pods labelled edge-gateway; the game-server source is not allowed. Consequently roster fetch, registration, connection receipts, shutdown, and result submission are all blocked even inside the cluster.

Add narrowly scoped game-server UDP ingress for the chosen Agones/public relay source and control-plane TCP ingress from the game-server pod selector. Keep the default deny and add policy tests for both directions, including a real NetworkPolicy-enforcing cluster smoke test.

[P1] Emit a valid initial-connect outbox envelope so one row cannot poison the queue

Location: server/store/initial_connect_sql.go:155-164, server/api/outbox.go:95-106, server/api/outbox.go:168-195, server/store/outbox.go:46-51

ApplyInitialConnectPlan writes state_changed payloads containing only match_id, state, and action. The state dispatcher requires event, revision, resource_id, occurred_at, and a non-empty player_ids list, so delivery always rejects that row. Dispatch stops on the first error and the row is never acknowledged; because reads are ordered oldest-first, the malformed row is retried forever and can prevent all later state events in the batch from being delivered.

Construct the same complete envelope used by the other lifecycle writers (or centralize envelope creation), include the authoritative participant list, and add a store-to-dispatch integration test for both LIVE and CANCELLED initial- connect outcomes. Also isolate/dead-letter permanently invalid rows so one bad event cannot globally head-of-line block publication.

[P1] Load authoritative ratings into ranked matcher candidates

Location: server/store/queue_sql.go:61-66, server/store/queue_sql.go:105-131, server/domain/matcher.go:171-186, server/domain/matcher.go:220-239, server/domain/teams.go:59-93

The production candidate query does not join or otherwise read the ratings table, and its scan never sets domain.Candidate.Rating. All PostgreSQL- sourced ranked candidates therefore have the Go zero value. Rating tolerance, selection scoring, and team partitioning all consume that field, so ranked matchmaking treats every player as identically rated regardless of their authoritative profile. Unit tests mask the defect by constructing candidates with ratings directly.

Populate ranked candidates from the authoritative rating row (with an explicit default for a genuinely new profile), carry it through Redis, and add store- backed matcher tests with deliberately distant ratings and a team-balancing assertion. Never accept a client-supplied rating.

[P1] Partition and bound Redis snapshots before filtering by playlist

Location: server/store/redis_candidates.go:80-85, server/store/redis_candidates.go:141-188, server/cmd/matcher/main.go:67-93

Both playlists share one Redis hash/sorted set. Snapshot performs an unbounded ZRANGEBYSCORE and HMGET, materializes and decodes the whole queue, then the matcher truncates to its candidate limit before filtering by playlist. A large casual prefix can therefore make the ranked worker see zero candidates indefinitely even when ranked tickets exist later in the set. A repair is worse: each matcher captures only its selected playlist as the durable source, but Rebuild replaces the shared keys, so a casual repair can erase ranked projections and vice versa. The unbounded read also makes each one-second poll allocate and transfer data proportional to total queue depth.

Use playlist-specific keys and make the snapshot API accept a hard limit that is applied by Redis (LIMIT 0 N) before transfer. Rebuild only the matching playlist namespace. Add mixed-playlist and large-backlog tests proving neither worker can erase/starve the other and that Redis never receives an unbounded range/HMGET.

[P1] Enforce durable identity bans during session issuance and authentication

Location: server/migrations/0001_initial.sql:5-10, server/store/session_sql.go:16-22, server/store/session_sql.go:49-63, server/domain/auth.go:166-197

The durable schema has banned_until and ban_reason, but production session authentication reads only the sessions row and no production store code reads either ban column. The only ban check is an in-memory TicketVerifier used by domain tests. Once real Steam login is wired, a banned identity can continue using every existing session until expiry and, unless the future adapter independently duplicates this policy, can receive new sessions too. This defeats the server-authoritative anti-abuse boundary.

Make ban state part of the durable authentication transaction: refuse session issuance for an active ban and join/check identities on every authenticated request (or revoke all sessions atomically when applying a ban). Add tests for immediate enforcement across two control-plane replicas and for expiry/unban semantics.

[P1] Fan out outbox events to every control-plane replica

Location: deploy/k8s/base/control-plane-deployment.yaml:8-14, server/api/events.go:55-117, server/api/events.go:217-230, server/api/outbox.go:69-90, server/store/outbox.go:46-60

The Deployment runs two replicas, but WebSocket subscribers live only in each process's in-memory hub. Every replica races to read the same global unpublished outbox rows, and publishing succeeds even when the winning replica has no matching local subscriber; that replica then sets the single global published_at. A client connected to the other replica never receives the event. The REST recovery polls eventually converge, but WebSocket delivery degrades as replicas are added and short-lived proposal transitions can be observed late.

Publish committed events through a shared fan-out transport, or maintain a durable per-replica/consumer-group cursor so every connection-owning replica sees them. Do not globally acknowledge merely because a local hub accepted an event for zero subscribers. Add a two-replica integration test with the client connected to the non-consuming replica.

[P1] Add retention for high-volume idempotency and outbox records

Location: server/migrations/0001_initial.sql:13-29, server/migrations/0001_initial.sql:147-177, Game/scripts/matchmaking.gd:38-52, Game/scripts/control_plane_client.gd:794-795, server/store/queue_sql.go:262-320, server/cmd/maintenance/main.go:57-104

Each ten-second queue heartbeat gets a fresh idempotency key and permanently inserts a new row. Published outbox rows and expired/revoked sessions are also never purged; the maintenance role performs lifecycle reconciliation only. At 10,000 queued players, heartbeats alone add roughly 60,000 durable rows per minute, causing unbounded table/index growth, vacuum pressure, backup growth, and progressively slower recovery on a service intended to scale horizontally.

Define retention windows longer than every supported retry/recovery horizon, index cleanup predicates, and delete/archive in bounded SKIP LOCKED batches. Expose deletion lag/row-count metrics and load-test sustained heartbeat volume to verify that steady-state storage remains bounded.

[P2] Make the observability verifier test reach its intended assertion

Location: server/security/test_observability_manifests.py:20-32, scripts/verify_observability_manifests.py:16-22

test_checker_rejects_wrong_namespace_and_broad_scrape copies only the control-plane ServiceMonitor and rules into its temporary directory. The verifier first requires kustomization.yaml and the allocator ServiceMonitor, so the test fails on a missing file before it examines the mutated namespace or scrape path. The security suite is red and the stated regression case is not covered.

Copy the complete minimum fixture (including kustomization and allocator ServiceMonitor), then assert the namespace and /metrics mutations separately so either defect produces the intended diagnostic.

[P2] Synchronize the contract test with the renamed connection operation

Location: server/contracts/v1/test_contracts.py:21-28, server/contracts/v1/openapi.json:54

The OpenAPI document calls the endpoint claimPlayerConnection, while the structural test still requires recordPlayerConnected. The checked-in contract suite therefore fails despite the endpoint being present, making the gate noisy and capable of obscuring real compatibility regressions.

Choose the intended public operation ID and update the test or document. If the rename is intentional, document the generated-client compatibility impact and assert claimPlayerConnection consistently.

Verification notes

• go test ./...: passed.
• go test -race ./...: passed.
• go vet ./...: passed.
• Godot unit suite: 220 tests passed with the project-compatible headless renderer flags.
• Training unit suite: 16 focused generation/evaluation tests passed in training/.venv; the reviewed training changes keep new distributions and team reward sharing opt-in, so no training-regression finding was raised.
• Contract suite: one failure, recorded above.
• Security manifest suite: one failure, recorded above.
• Script verifier unit suite: 10 tests passed.