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.
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.
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).
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.
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.
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.
Josh Creek