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