mirror of
https://github.com/jcreek/CosmicClash.git
synced 2026-09-10 16:04:04 +00:00
fix(deploy): supply Steam credentials to the control plane, refresh stale status
The Steam adapter took --steam-publisher-key/--steam-app-id and the matching env vars, but no manifest supplied them, so a deployed control plane would have kept sign-in returning 503 even once the App ID from #15 arrived -- that issue would have unblocked nothing on landing. Mount them from a new cosmic-clash-steam Secret, into the control-plane Deployment alone: the publisher key is issued to us, never to a client, and no other workload (least of all a game server) has any use for it. A manifest test asserts both the wiring and that the Secret appears in no other manifest; verified it fails in both directions. Both keys are optional, so the Deployment still rolls out before the App ID exists and sign-in simply stays 503. Also correct task rows this branch made stale: 7.4 (durable ban storage landed), 8.7 (adapter, bans and secret store landed), 8.39 (cross-replica fan-out landed), and 8.5's migration range, which stopped at 0013. Move the branch review into docs/ with a header marking it a point-in-time artefact -- all thirteen findings are addressed, and its present tense would otherwise read as current behaviour. Record gotcha 52: the integration scripts use `docker run --rm`, which reclaims the container but not its anonymous volume. Sixty-four of them, ~4 GB, accumulated during this session until PostgreSQL stopped starting -- surfacing only as the script's own readiness timeout, not as a disk error. That is the real cause behind the "Docker storage exhausted locally" notes those rows carried.
This commit is contained in:
@@ -0,0 +1,298 @@
|
||||
# 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
|
||||
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.
|
||||
Reference in New Issue
Block a user