The just-landed signed workload token embedded (allocation_id, match_id,
server_id) as claims. That doesn't actually work for its intended delivery
channel: the token is meant to be requested as a GameServerAllocation
annotation in the SAME request that asks Agones to pick a server, so at mint
time the allocator knows allocation_id (it generates it) but not yet which
server_id Agones will return -- server_id only exists in Agones's response,
after the annotation request has already been sent. Embedding it was simply
not possible for the real caller this was built for; only the (allocator ->
signed_token) unit tests and hand-constructed integration tests happened to
supply it directly, masking the gap.
Fixes it by having the token bind only allocation_id (the one identifier
actually known at mint time) plus expiry. match_id/server_id are resolved at
verify time from the durable allocations table via the new
store.AllocationBindingByAllocationID, keyed by allocation_id -- which the
allocator already records immediately after Agones responds. This is
strictly stronger, not just a workaround: a caller can no longer claim any
match/server pairing at all, even one that happens to be internally
consistent -- the binding returned is entirely durable-record-derived.
Verified: server/workload's unit tests updated for the new two-field claim
shape; server/api's Postgres integration suite gains
TestWorkloadVerifierFromSignedTokenNeverTrustsCallerSuppliedBinding (two
distinct real allocations each resolve to their own, and only their own,
match/server pairing) replacing the now-inapplicable mismatched-triple test.
Full `go build ./... && go vet ./... && gofmt -l . && go test ./... -race`
and `go test -tags integration ./... -race` both clean; the api integration
suite re-run 3x clean against a live postgres:17-alpine container.
WorkloadVerify (api.Service.WorkloadVerify) was permanently unwired: both
/v1/servers/{id}/register and /v1/servers/{id}/result always 503, because
the only design considered so far was verifying a Kubernetes-projected
service-account JWT (server/workload/jwt.go), which needs a live cluster's
TokenReview/JWKS endpoint to validate against safely -- something this
sandbox cannot do without guessing at a trust boundary.
The API layer doesn't actually require that specific mechanism. serverMutation
only compares WorkloadBinding.ServerID and .MatchID (server/api/service.go);
AdvanceServerRegistration only uses .MatchID/.ServerID/.AllocationID. Nothing
downstream needs Namespace/ServiceAcct/PodUID/GameServerUID populated.
This adds a self-contained alternative: a short-lived, HMAC-signed token the
control plane mints and verifies with a secret only it holds (server/workload/
signed_token.go), the same trust model domain.SessionStore already uses for
player sessions elsewhere in this codebase. It needs no cluster to verify --
signature + expiry is fully self-contained and unit-testable.
The design's soundness rests on the delivery channel, not the crypto: the
token is meant to reach the allocated GameServer via the same Agones
GameServerAllocation annotation channel allocation.go already uses for
match-id/allocation-id, readable only by that pod's own local SDK sidecar. A
caller presenting this token has already proven, via that channel, that it is
the pod Agones allocated. (Wiring the actual annotation delivery -- extending
agones.Client.Allocate and the supervisor's token source -- is a separate,
follow-up change; this commit lands the verification core it depends on.)
store.AllocationBindingStillValid adds defense-in-depth on top of signature
and expiry: it cross-checks the token's claims against the durable
allocations table (append-only, never leaves 'ALLOCATED'), so a validly-signed
token naming an allocation that was never recorded -- or a real allocation id
paired with a mismatched match/server -- is still rejected.
api.WorkloadVerifierFromSignedToken wires the two together and is now plugged
into cmd/control-plane (new --workload-secret / COSMIC_CLASH_WORKLOAD_SECRET
flag; a startup warning is logged if it's left unset, since the route then
stays 503 exactly as before) and cmd/testkit-api (fixed test secret, since
that binary is test-only already).
Verified: new unit tests in server/workload (signature tamper, wrong secret,
expiry boundary, malformed input) and a new Postgres integration suite in
server/api (real allocation row, real signed token, acceptance / unknown-
allocation rejection / mismatched-triple rejection / the previously-503
Service.WorkloadVerify field itself) -- both run clean with -race across
multiple passes against a live postgres:17-alpine container. Full
`go build ./... && go vet ./... && gofmt -l . && go test ./... -race` and
`go test -tags integration ./... -race` both clean.
The last two commits fixed the severe stranding bug in decline and
timeout, but left a real responsiveness gap: cancelling a ticket
directly while it's part of an OPEN proposal used to leave the OTHER
participant waiting out the full response window for something the
system already knew couldn't happen -- their proposal partner just
abandoned the queue. ProposalExpireRequeueSQL eventually rescues them,
but only after the full window elapses, not immediately.
CascadeCancelToOpenProposal runs inside the same transaction as the
cancel itself: if the cancelled ticket belonged to a currently-OPEN
proposal, decline that proposal right now and requeue every other
participant immediately via the same ProposalDeclineRequeueSQL the
decline path already uses. The cancelling player's own ticket
correctly stays CANCELLED, not swept back into the requeue meant for
everyone else (ProposalDeclineRequeueSQL only touches tickets still at
PROPOSED).
Covered by a real PostgreSQL integration test: cancelling one
participant's ticket mid-proposal immediately declines the proposal
and requeues the other participant with a refreshed expiry, while the
cancelling player's own ticket stays CANCELLED. First draft used a
stale expected revision (0) for the cancel call -- CreateProposal's
own QueueTicketProposeSQL already bumps a ticket's revision to 1 when
forming the proposal, caught immediately by actually running the test
against real Postgres rather than assuming. Clean across 5 runs after
the fix, plus the full integration and unit suites.
The timeout sibling of the previous commit's decline fix: a proposal
that simply times out (the 10s window elapses with no unanimous
response) hits ProposalExpireSQL/ProposalParticipantExpireSQL, and
neither of those -- same as the decline path -- ever touched
queue_tickets. Same severe consequence: every participant still
holding a PROPOSED ticket, response pending or already accepted, is
left stranded (invisible to the matcher, blocking a fresh
queue_create, renewable forever by heartbeat) with no automatic way
back into matchmaking. This path is reached from both GetProposal
(the recovery/read boundary -- a client that missed the expiry event
entirely) and RespondToProposal (a response arriving after the
window), so both needed the fix.
ProposalExpireRequeueSQL mirrors ProposalDeclineRequeueSQL, guarded on
state = 'EXPIRED' so it's safe to call unconditionally right after
ProposalExpireSQL: a no-op on a proposal that's still OPEN, and a
no-op on a proposal that was already EXPIRED on a prior pass (nothing
left at PROPOSED to requeue a second time).
Covered by a real PostgreSQL integration test via GetProposal (nobody
ever responds; recovering the proposal well after its window expires
it and must requeue both participants), confirming both tickets land
back at QUEUED with a refreshed expiry and are visible again to
ListQueuedCandidates. Clean across 5 runs, plus the full integration
and unit suites.
Found by reading the code, not a failing test: no path anywhere
transitioned a queue ticket from PROPOSED back to QUEUED after a
proposal was declined. A stranded PROPOSED ticket is invisible to the
matcher (ListQueuedCandidates only ever reads state='QUEUED'), still
counts as that player's one active ticket (blocking a fresh
queue_create), and is renewable forever by an ordinary heartbeat --
a player proposed a match with someone who then declines had no way
back into matchmaking without realising, on their own, that they
needed to manually cancel first. This affects every participant, not
just the decliner: an uninvolved player who never even responded was
left stuck by someone else's decision.
ProposalDeclineRequeueSQL requeues every participant's ticket,
including the decliner's own -- nothing yet enforces the decline
cooldown task 8.17 documents as a separate, not-yet-built feature, so
leaving anyone behind at PROPOSED today isn't "cooldown behaviour",
it's just broken. Once that cooldown exists it can exempt the
decliner from this immediate requeue; today nothing does.
Covered by a real PostgreSQL integration test: after one player
declines, both the decliner's and an uninvolved participant's tickets
land back at QUEUED with a refreshed expiry, and -- the actual
end-to-end regression -- both are visible again to
ListQueuedCandidates, the same query the matcher itself uses. Clean
across 5 runs, plus the full integration and unit suites.
Same discovery pattern as ResultSubmitter/SessionIssuer, one level
deeper: api.Service.rankedProfile and .profile both only ever read
from an in-memory RankedProfiles map with no durable-store equivalent
at all -- not "adapter exists but unwired" this time, there was no
adapter. Every real request to GET /v1/profile/ranked or /api/v1/profile
always 404'd regardless of a player's actual rating.
Add RankedProfileProvider (an interface, not a struct-literal adapter
this time) and store.PostgresRankedProfiles reading the ratings table;
Service.rankedProfileFor prefers it when set and falls back to the
map otherwise, so every existing test/direct Service literal keeps
compiling and passing unchanged. A missing ratings row maps to the
exact same (zero value, false, nil) the map lookup already produced,
preserving existing not-found semantics rather than reinterpreting
them. LastSeasonID/SeasonHistory are deliberately left unset -- the
ratings table has no season pointer, and reconstructing history needs
its own query and display semantics, not bundled in here speculatively.
Wired into both cmd/control-plane and cmd/testkit-api. Verified
against real PostgreSQL via curl: a fresh identity's ranked profile
correctly 404s through the real adapter (same behavior as before,
now for a real reason instead of an empty map).
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.
Josh Creek