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