Investigating the fleet.yaml wiring task found something more
fundamental than a manifest problem: cmd/control-plane/main.go never
wires WorkloadVerify, and store.PostgresResults (a ready-made,
already-correct ResultSubmitter adapter matching the interface
exactly) was referenced from nowhere outside its own file -- not even
a test. Both server-authenticated routes this session built
(/v1/servers/{id}/register and the pre-existing /result) are
completely unreachable in the actual running control-plane binary
today: Service.serverMutation treats a nil WorkloadVerify as fatal
for both routes regardless of ServerRegistrar/ResultSubmitter being
present, so every real request 503s.
Wire the safe, obviously-correct half: ResultSubmitter now uses
store.PostgresResults{DB: db}, same pattern as ServerRegistrar.
Deliberately NOT attempting a WorkloadVerify implementation here.
server/workload/jwt.go's ParseAndValidate needs a pre-known "expected"
WorkloadBinding to construct its policy against (itself needing a
durable per-allocation lookup that doesn't exist yet) plus a real
cryptographic SignatureVerifier -- which for a Kubernetes projected
service account token means either fetching/caching the cluster's own
JWKS or delegating to the API server's TokenReview endpoint, a
different verification model that doesn't fit ParseAndValidate's
signature-callback shape at all and would need its own domain-level
adapter. This is authentication-critical code with no existing
wiring example anywhere in the codebase to follow, and the actual
trust boundary (a live cluster's key material) can't be validated
from this sandbox regardless of how carefully the client code is
written. Building it fast under this session's already-heavy pace
risked a subtle, dangerous mistake far more costly than leaving the
gap named precisely, which is what this commit does instead.
Added TestServerRoutesRequireWorkloadVerifyToBeWired: pins the
current 503-on-every-request behavior as an explicit, visible
regression trip-wire rather than a silent gap -- it's designed to
start failing (and be updated, not deleted) the day WorkloadVerify is
actually wired.
Closes the last named item on task 8.28 (Health-reclaim): nothing
currently detects or cleans up a match stuck in
ALLOCATING/PROCESS_READY/ASSIGNMENT_READY forever because its server
crashed or was reclaimed by Agones as unhealthy before ever
registering -- players would wait indefinitely for a match that was
never coming.
The design question this was blocked on -- does an abandoned match
auto-requeue its players, or fail and make them re-queue -- isn't
actually open: task 8.50's own stated acceptance criterion already
answers it ("infrastructure-caused cases cannot penalise affected
players"). A server-side crash/reclaim is exactly that, not player
behaviour, so store.ExpireStalledAllocations fails the match but
requeues every participant's ticket to QUEUED with a fresh expiry
(matching the ordinary 30s queue window), releases their
match_participants row (participation_active = false, so they're
matchable again immediately), all inside one FOR UPDATE SKIP LOCKED
pass so a second maintenance replica continues past whatever a
concurrent one is already reclaiming.
Wired into cmd/maintenance alongside the existing season-rollover
sweep: --stalled-allocation-deadline (default 2m) and
--stalled-allocation-batch (default 100).
Covered by a SQL-fragment test and a real PostgreSQL integration test:
two matches (one genuinely stalled, one recent), confirming the
deadline boundary is respected (recent match untouched), both
stranded participants' tickets requeue with a refreshed expiry, the
match_participants row releases, and a second pass doesn't reprocess
an already-FAILED match. Verified clean across 5 runs, plus the full
integration and unit suites.
Godot's stdout is fully (block) buffered whenever it isn't attached to
a TTY -- true of every real deployment path this repo documents:
'docker run -d' (Docker's log driver presents a pipe), a plain
'docker run' without -d, and systemd's journal capture (also a pipe).
Confirmed directly, not from the existing gotcha note alone: a real
'docker run -d' container sat for 20+ seconds with 'docker logs'
showing nothing at all -- not even the startup line -- while the
process was confirmed alive and running (ps aux inside the container).
'docker stop' then killed it via SIGTERM (Godot has no SIGTERM hook)
without ever flushing that buffered output, losing it permanently
rather than merely delaying it.
This affects the already-shipped community server path today, not
just the not-yet-built Agones fleet path multiplayer-next.md's task
8.28 gotcha originally flagged this for -- SERVER.md's Docker AND
native-systemd instructions both route through this exact launcher
script, and journald's capture has the same non-TTY-pipe buffering
problem docker logs does.
Wrap the exec in 'stdbuf -oL -eL' (LD_PRELOAD-based line buffering,
touches no binary) when available, falling back to the unwrapped exec
otherwise so a minimal image without GNU coreutils still starts.
Re-verified the same failing scenario against the actual launcher
script in a real image: the startup line now appears within 3s of a
genuinely detached 'docker run -d'. Re-ran the full make verify-phase6
gate end to end afterward to confirm no regression: both arenas
rotated, both clients observed both goals, clean teardown.
Closes the second blocker named last commit. Re-traced the actual code
path rather than trusting the earlier assumption: server_boot.gd
verifies its mounted roster file synchronously in _ready(), before
NetworkManager.host() runs and before ServerControl.set_process_ready
is ever called -- so by the time the loopback /ready probe (and thus
Agones Ready, and thus process-ready registration) succeeds, Godot has
already verified its own roster. And the API's ASSIGNMENT_READY gate
(AdvanceServerRegistrationSQL) checks only durable `assignments` rows
server-side, nothing Godot reports. No new Godot-side state was needed
-- the earlier 'needs Godot's own roster-verification state exposed'
claim was overcautious and is corrected here.
The supervisor now calls registerControlPlane(ctx, true) right after
process-ready succeeds, with a bounded retry (default 5 attempts, 2s
apart, both configurable) rather than a single attempt: the durable
`assignments` rows the server-side gate checks may not have propagated
by the first attempt, and that is expected, not fatal. Unlike a
process-ready registration failure, a persistent assignment-ready
failure does NOT kill the child -- the process is already legitimately
listening and usable, and killing a healthy process over a lagging
control-plane read would be actively harmful; it's logged to stderr
instead.
Covered by two tests: the full process-ready-then-assignment-ready
sequence and body shapes, and a retry test that fails the assignment-
ready call twice with 409 (simulating the real gate not yet
satisfied) before succeeding on the third attempt, asserting Start()
still succeeds and the child is never killed.
An earlier edit this session (commit 4278c04d) had appended new
content to the row's evidence column using the same duplicated-prefix
text twice with an extra '|' between them, silently turning a 3-column
markdown table row into 4 columns -- caught only by literally counting
pipe characters, not by reading the rendered text. Rewritten as a
clean 3-column row: task description, then evidence, deduplicated.
Also records the game-server Docker image, the allocation-annotation
match-ID channel, and the dead ubuntu digest fix from this session's
commits.
New additive targets, the existing server/exporter/smoke-client/enet-test
targets are byte-for-byte unchanged (make verify-phase6 re-run clean
after this, see previous commit):
- supervisor-build: builds server/cmd/game-server-supervisor with a
pinned golang:1.23-alpine (matching go.mod's go 1.23)
- game-server: the same dedicated-server export as `server`, plus the
supervisor binary, with the supervisor as ENTRYPOINT instead of the
direct launcher script -- this is what makes the process-ready/
assignment-ready control-plane registration from the last two
commits actually reachable in a real deployment.
Verified with a real docker build --target game-server, not just by
reading the file: both binaries land at the expected paths with
correct permissions, and the supervisor prints its usage text when
run directly.
deploy/k8s/base/fleet.yaml does not reference this image or invoke the
supervisor's flags yet -- documented inline and in
multiplayer-next.md; that's the next concrete step, not attempted here
since the exact flag values (control-plane URL, workload-token mount
path) are deployment-environment decisions this sandbox can't make.
The server stage's ubuntu@sha256:571c2ab1... no longer resolves from
Docker Hub (docker pull by that exact digest returns 'not found',
verified directly, independent of anything in this Dockerfile) --
make verify-phase6 was currently broken for anyone building from a
clean cache. Found while adding a new build stage below it and
actually running the build rather than just editing the file.
Re-pinned to a digest verified to pull, and re-ran the full
make verify-phase6 gate end to end to confirm: both arenas rotated,
both headless clients observed both authoritative goals, clean
teardown.
Investigated the Fleet-manifest wiring task flagged last commit and
found a deeper, previously-undesigned gap: Kubernetes env vars are
fixed at pod creation, but Agones allocates a match to an already-
running Ready pod well after it starts -- so there was no channel at
all for match-specific data (match ID) to reach that pod's processes.
Close it using the Agones GameServerAllocation API's documented
spec.metadata.annotations field, which Agones applies to the allocated
GameServer's own object_meta on success: server/agones.Client.Allocate
now requests cosmic-clash.io/match-id and cosmic-clash.io/allocation-id
annotations, and the supervisor reads them back from the same
/gameserver SDK call it already makes for the assigned port/address
(GameServer.ObjectMeta.Annotations), falling back to them for its own
control-plane registration only when MatchID isn't explicitly
configured -- an explicit value always wins, and a match ID resolvable
from neither source fails Start() closed before any HTTP call.
The exact object_meta vs objectMeta JSON key from a live Agones SDK
sidecar is not independently verified from this sandbox; documented
inline, and the fallback degrades safely (empty annotations map, same
as before this change) if it turns out to be wrong.
Covered by two new tests: the annotation actually flowing through to
the registration body, and fail-closed with neither config nor
annotation supplying a match ID (registerCalled stays false, not just
that Start() errors).
Also records the concrete gap this surfaced: the Fleet manifest never
actually invokes game-server-supervisor today, so the new capability
has no deployment wiring yet -- and names exactly what's needed
(entrypoint/sidecar decision, token volume, Downward API env vars)
rather than leaving it as an unspecified 'remains'.
Add opt-in control-plane registration to server/supervisor: once Agones
Ready succeeds, POST /v1/servers/{id}/register (assignment_ready=false)
using a workload token read fresh from disk each call -- matching how a
Kubernetes projected service account token is rotated in place by
kubelet, unlike a cached/env-var secret. ControlPlaneURL empty (the
default) is a total no-op, so direct/Compose mode and allocated-without-
control-plane mode are both byte-for-byte unaffected; New() rejects a
half-configured registration (URL set without token path/server/match/
digest) rather than silently skipping it.
ServerID/MatchID/ImageDigest are read from env vars named by CLI flags
(--server-id-env, --match-id-env, --image-digest-env), matching the
existing --drain-token-env convention in this same binary, rather than
parsed out of the Agones SDK's own GameServer JSON -- that shape isn't
independently verifiable from here, whereas the Kubernetes Downward API
(fieldRef: metadata.name) populating an env var is a standard, safe
pattern already used elsewhere in this codebase for exactly this class
of secret.
A registration failure now kills the child (matching the existing
waitReady failure path) rather than leaving Agones-Ready-but-
control-plane-unregistered process running -- a real gap the second new
test (TestControlPlaneRegistrationFailureKillsChildRatherThanRunningUnregistered)
had to be corrected to actually exercise: its first draft omitted
ReadyURL and was failing at waitReady, before ever reaching the code
path it claimed to test.
Races 5 concurrent HeartbeatQueueTicket calls, all at the same expected
revision, against a real PostgreSQL instance -- a genuinely plausible
client scenario (slow-response retry, duplicate tab/process), and one
the existing sequential stale-revision test can't exercise since it
only calls the second heartbeat after the first has already
committed. Asserts exactly one wins, the durable revision ends at
exactly 1 (not higher, which a stale winner slipping through would
produce), and every loser fails cleanly rather than hanging or
returning a raw serialization error. Stable across 6 runs with -race,
plus the full integration and unit suites.
Every existing RedisCandidateIndex test runs against miniredis -- a
from-scratch Go reimplementation of the Redis command set, not real
Redis's own float64 score encoding, TTL/expiry, or RESP behavior.
Add an opt-in integration suite (mirroring postgres_integration_test.go's
pattern: //go:build integration, COSMIC_CLASH_REDIS_ADDR-gated) plus
scripts/run_redis_integration.sh against a disposable redis:7-alpine
container, covering:
- upsert/snapshot/remove against a real server
- a real TTL actually waited out (not miniredis's manual
FastForward), proving expiry really happens on the wire
- the documented 'Redis restart or lost keyspace' repair path
exercised against an actual FLUSHALL, not a simulated empty map,
including that the repair actually persists back to Redis (a
second snapshot reads it without a second durable-source call)
Verified stable across 3 runs with -race against a real container.
Races 5 concurrent identical CompleteResultWithResult calls for the
same RESULT_PENDING match against a real PostgreSQL instance -- the
scenario behind 8.25's 'identical duplicates idempotent' claim, which
every existing result test only exercised sequentially. All five must
succeed (idempotent replay, not conflict), and the rating update must
apply exactly once: asserted by computing the expected post-match
rating independently via the same domain.CasualOpponents/UpdateRating
functions and requiring an exact match, since a doubled application
would compound the rating further away from baseline rather than
merely producing 'some' change that a looser inequality check would
miss.
First draft asserted ranked_games == 1, which doesn't hold for a
casual result (rankedIncrement is unconditionally 0 for casual by
design) -- caught by actually running it, not by inspection. Verified
clean across 6 runs with -race, plus the full integration and unit
suites.
Fires more concurrent ClaimAllocation calls than there is Ready
capacity at a real PostgreSQL instance and asserts: exactly as many
win as there was capacity, every winner gets a distinct server (no
double-booking), every loser gets ErrNoCapacity rather than a raw
serialization error or a hang, and the durable game_servers.state
count matches. This is the cross-allocator-replica race 8.30 calls
out as untested -- the existing capacity test in this file claims
strictly one request at a time. Verified clean across 6 runs with
-race, plus the full integration and unit suites.
Every existing Postgres integration test runs strictly one transaction
at a time, so none of them exercise the SERIALIZABLE retry-and-fence
path CreateProposal actually depends on for correctness under real
matcher-replica contention -- only concurrent goroutines against a
real connection can. Add a test that races two goroutines each
proposing a formation that shares one contested ticket (a realistic
scenario: nothing stops two matcher replicas reading the same QUEUED
ticket in the same poll window), and asserts exactly one proposal
commits, the loser's proposal and participant rows are fully rolled
back, the contested ticket ends up claimed by the winner, and -- the
part a single-threaded test can't show -- the loser's OWN uncontested
ticket also rolls back to QUEUED rather than being left stranded as
PROPOSED with no surviving proposal.
Adversarial review of my own first draft: it initially failed
deterministically (5/5 runs), but the failure was in the test itself
-- the winner/loser branch picking the loser's uncontested ticket had
the two branches swapped, so it was checking the WINNER's ticket
against the QUEUED expectation. Fixed and re-verified clean across 8
runs with -race, plus the full integration suite.
Wire the same Service.Log hook added for the server register/result
routes into queue create/heartbeat/cancel and proposal accept/decline:
log the resulting state on success (queue_create, queue_heartbeat,
queue_cancel, proposal_response) or 'rejected' on a domain error,
using only the ticket/proposal ID and outcome -- never the domain
error text itself, which isn't documented as credential-free.
Read-only routes (queue GET, proposal GET, assignment fetch) and the
early availability/not-found rejections that return before reaching
the domain call are deliberately not logged in this pass.
Covered by a new end-to-end test driving real create/heartbeat/cancel
and an accept followed by a stale-revision accept (fenced for real by
the domain layer behind proposalBackendSpy, unlike the dumb queue
spy), asserting the exact sequence of events logged.
server/observability existed fully unit-tested but was imported by
nothing outside its own package -- no HTTP handler ever called it, so
its credential redaction protected zero real log output. Wire it into
Service via an optional Log field (nil-safe, so every existing Service
literal keeps compiling unchanged) and call it from the two
workload-authenticated server routes -- register and result -- at
every outcome: unauthorized, rejected, conflict and success. Wire
cmd/control-plane to actually emit those events as JSON lines on
stderr.
Add a secret canary test that drives both routes end to end with
realistic bearer-token and result-nonce values and asserts neither
literal secret appears anywhere in what Service.Log actually received
-- a stronger claim than the existing observability unit test, which
only proves redact() strips a synthetic value under a denylisted key
name. redact() is still key-name-based, not content-based: a future
call site that logs a secret under an unlisted key name would not be
caught by this test or by redact() itself, only by the same discipline
applied here of never putting raw request/token bytes into Fields.
Queue, proposal and assignment mutation routes are not wired yet.
Tasks 8.5, 8.14, 8.18 and 8.23 all claimed opt-in PostgreSQL execution
already covered their durable paths. It existed, but per this session's
run had apparently never actually been exercised clean: it surfaced
three real bugs (queue ticket insert param-count mismatch, an
unconditional proposal-participant timeout, a missing seasons row in
one integration test) that a passing unit-test suite could not have
caught, since the unit tests mock the driver. Record what was found,
fixed and re-verified live, and that the new migration rollback runner
is now in place.
ranked_season_rollovers.season_id has a foreign key into seasons, but
the integration test never inserted a seasons row for 'season-1' --
ApplyRankedSeasonRollover failed on the FK constraint before the
rollover logic itself ran at all. Insert a matching seasons row,
mirroring how a real 12-week season would already exist when
maintenance's rollover sweep runs. Verified against a real
PostgreSQL instance.
ProposalParticipantExpireSQL marked every PENDING participant on a
proposal TIMED_OUT unconditionally -- it took a proposal_id and 'now'
but never actually compared 'now' against the proposal's expires_at,
unlike its sibling ProposalExpireSQL (which does gate on
'expires_at <= $2'). Both GetProposal and RespondToProposal run this
statement on every call as a recovery step, so the very first
RespondToProposal for any proposal timed out every participant
(including the one about to respond) before checking their response,
then rejected the real accept/decline with ErrConflict. Add the same
expiry gate via an EXISTS against proposals.expires_at, matching
ProposalExpireSQL's own condition, and update the SQL-fragment test to
assert the gate is present. Verified end to end against a real
PostgreSQL instance: TestPostgreSQLProposalClaimAndResponseAreAtomic
now passes a two-participant accept/accept sequence that previously
failed on the first response.
CreateQueueTicket passed 9 arguments to QueueTicketInsertSQL, which
only has 8 placeholders (state is a hardcoded 'QUEUED' literal in the
SQL, not $4) -- every real queue-ticket creation against PostgreSQL
failed with 'mismatched param and argument count'. Found by actually
running the opt-in Postgres integration suite (previously never
exercised locally, per its own gating) rather than trusting the unit
tests, which mock the driver and can't catch a placeholder-count
mismatch. Verified fixed against a real postgres:17-alpine container.
Add migrations.Rollback(ctx, db, dir, steps): reverses the N most
recently applied migrations, newest first, each in its own committed
transaction under the same advisory lock Apply uses. Down SQL lives in
migrations/down/<version>.sql (a subdirectory, so Apply's *.sql glob
over the main directory is untouched); a missing down file for a
migration being rolled back is a hard error rather than a silent
partial reversal. Wire it into cmd/migrate as --rollback=N.
Add down files for all six existing migrations, each dropping objects
in FK-safe reverse dependency order.
Adversarial review: could not run the new integration test
(TestPostgreSQLMigrationsRollBackAndReapplyCleanly, gated behind
COSMIC_CLASH_POSTGRES_DSN / scripts/run_postgres_integration.sh)
against a real database in this sandbox - Docker Desktop's own
overlayfs ran out of space pulling postgres:17-alpine, unrelated to
this change. Verified instead by hand-tracing every DROP against its
forward migration's FK graph, confirming Apply's directory glob does
not pick up the down/ subdirectory, and a clean go build/vet/test
-tags integration. Worth an explicit real run before this is trusted
in CI.
Add POST /v1/servers/{id}/register (and its /api/v1 contract alias),
authenticated by the same workload binding as the result route. A
game server reports its protocol version and image digest and asks
to advance ALLOCATING -> PROCESS_READY -> ASSIGNMENT_READY; the store
boundary (AdvanceServerRegistration) does this as one idempotent
SERIALIZABLE transaction that also advances every participant's queue
ticket, and gates the final transition on every participant having a
live, unexpired assignment.
Adversarial review of the surrounding routing turned up a pre-existing
bug: contractServerMutation rejected any path containing '/', so the
already-documented /api/v1/servers/{id}/result route (and this new
/register route) 404'd for every real caller despite being declared
in the OpenAPI contract. Fix it to delegate shape validation to
serverMutation, matching how contractQueueMutation handles its own
two-segment paths, and add a regression test covering both contract
routes end to end.
multiplayer-todo.md and multiplayer-next.md tracked overlapping
information in two places. Fold everything into multiplayer-next.md
(architecture decisions, wire format, task breakdown with checkboxes,
gotchas list, testing notes) and delete multiplayer-todo.md. Section
numbers are unchanged, so existing code comments citing them by
section/task number still resolve; update every such reference to
point at the new filename.
Josh Creek