From d3a457d8d03c4b85f268956b040e8226bf5d0cb7 Mon Sep 17 00:00:00 2001
From: Josh Creek <8179928+jcreek@users.noreply.github.com>
Date: Tue, 1 Sep 2026 21:39:18 +0100
Subject: [PATCH] fix(multiplayer): avoid quota double charge on recovery

---
 multiplayer-next.md              |  2 ++
 server/allocator/service.go      | 11 +++--------
 server/allocator/service_test.go | 24 +++++++++++++++++++++---
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/multiplayer-next.md b/multiplayer-next.md
index a9898d9d..03bfe01c 100644
--- a/multiplayer-next.md
+++ b/multiplayer-next.md
@@ -1457,6 +1457,8 @@ Season rollover now computes from the row locked inside its serializable transac
 
 The production ranked-profile adapter now projects the active ranked season ID from the durable `seasons` table while keeping rollover history separate; the API prefers that current-season value and retains the legacy in-memory fallback for existing callers.
 
+Allocator quota accounting now charges only fresh provider attempts; recovery of a provider result after an ambiguous durable write does not consume the same regional quota a second time.
+
 Ranked proposal admission no longer trusts the matcher’s `--ranked-random-arena` boolean. The Go domain now owns a named allowlist for the three floor-goal `ArenaRegistry` entries, and rejects unknown and elevated IDs before any proposal is created.
 
 The arena hand-off is now durable: the matcher deterministically selects an eligible floor-goal arena from the proposal ID, migration 0008 stores that path on proposals and matches and enforces it for new direct SQL writes, migration 0009 retains it on provider allocations, domain/store/provider boundaries and recovery lookups recheck the same allowlist, allocation claims and idempotency digests retain it, Agones applies it as a match-scoped annotation, and the supervisor overlays the allocated child’s `--arena-path`. Godot accepts only the same floor-goal `ArenaRegistry` paths and requires one for allocated ranked matches, so a stale Fleet default, an elevated variant, or an altered retry cannot substitute a ranked arena. The recovery worker also rejects a provider-recovered allocation whose arena differs from the durable request before recording or binding it.
diff --git a/server/allocator/service.go b/server/allocator/service.go
index ab771fbb..b2d0bbfa 100644
--- a/server/allocator/service.go
+++ b/server/allocator/service.go
@@ -138,14 +138,9 @@ func (s Service) RecordProviderAllocation(ctx context.Context, result agones.All
 	if s.Durable == nil || result.Allocation.State != domain.ServerAllocated || result.Endpoint == "" {
 		return domain.Allocation{}, domain.ErrAllocationInput
 	}
-	if s.Quota != nil {
-		if err := s.Quota.Consume(ctx, result.Allocation.Region, now); err != nil {
-			if s.Metrics != nil {
-				s.Metrics.ObserveDenied(result.Allocation.Region)
-			}
-			return domain.Allocation{}, err
-		}
-	}
+	// Quota is consumed by Allocate before a fresh provider request. This
+	// method only reconciles an already-issued provider result after an
+	// ambiguous write, so consuming here would charge one allocation twice.
 	allocation, err := s.Durable.RecordProviderAllocation(ctx, result.Allocation, now)
 	if s.Metrics != nil {
 		if err != nil {
diff --git a/server/allocator/service_test.go b/server/allocator/service_test.go
index 323613fd..50ee0e87 100644
--- a/server/allocator/service_test.go
+++ b/server/allocator/service_test.go
@@ -97,7 +97,7 @@ func TestServiceConsumesSharedQuotaBeforeFreshProviderCall(t *testing.T) {
 	}
 }
 
-func TestServiceConsumesSharedQuotaOnceWhenReconcilingProviderResult(t *testing.T) {
+func TestServiceDoesNotConsumeSharedQuotaWhenReconcilingProviderResult(t *testing.T) {
 	quota := &quotaSpy{}
 	durable := &durableSpy{}
 	service := Service{Durable: durable, Quota: quota, Now: func() time.Time { return time.Unix(1000, 0) }}
@@ -105,8 +105,26 @@ func TestServiceConsumesSharedQuotaOnceWhenReconcilingProviderResult(t *testing.
 	if _, err := service.RecordProviderAllocation(context.Background(), result, time.Unix(1000, 0)); err != nil {
 		t.Fatalf("reconciliation failed: %v", err)
 	}
-	if quota.calls != 1 || durable.calls != 1 {
-		t.Fatalf("quota/durable calls = %d/%d, want 1/1", quota.calls, durable.calls)
+	if quota.calls != 0 || durable.calls != 1 {
+		t.Fatalf("quota/durable calls = %d/%d, want 0/1", quota.calls, durable.calls)
+	}
+}
+
+func TestServiceDoesNotDoubleChargeQuotaAfterProviderResultRecovery(t *testing.T) {
+	quota := &quotaSpy{}
+	durable := &durableSpy{err: errors.New("recording unavailable")}
+	provider := &providerSpy{result: agones.AllocatedServer{Allocation: domain.Allocation{AllocationID: "a", MatchID: "m", Region: "EU", State: domain.ServerAllocated}, Endpoint: "127.0.0.1:7777"}}
+	service := Service{Provider: provider, Durable: durable, Quota: quota, Now: func() time.Time { return time.Unix(1000, 0) }}
+	request := domain.AllocationRequest{AllocationID: "a", MatchID: "m", Region: "EU", Build: "b", Protocol: 1, Transport: "enet"}
+	if _, err := service.Allocate(context.Background(), request, nil); err == nil {
+		t.Fatal("durable recording failure was ignored")
+	}
+	durable.err = nil
+	if _, err := service.RecordProviderAllocation(context.Background(), provider.result, time.Unix(1001, 0)); err != nil {
+		t.Fatalf("provider recovery failed: %v", err)
+	}
+	if quota.calls != 1 || durable.calls != 2 {
+		t.Fatalf("quota/durable calls = %d/%d, want 1/2", quota.calls, durable.calls)
 	}
 }