fix(multiplayer): stop a doomed formation from wedging the matcher

Closes the 'innocent-ticket restoration' gap noted in §8.20 and found by re-examining §8.16's matcher worker. domain.FormFromQueue's anchor is always the single oldest candidate, deterministically. If domain.PrepareProposal then rejected that exact formation for a reason specific to those particular players — mismatched protocol, incomplete ranked identity metadata, a duplicate-SteamID pair, ranked admission generally — RunOnce returned immediately and the next matcher interval reproduced the identical formation and failed again. Forever: nothing in the queue ever changes, so the same doomed anchor group would be retried every single pass, permanently head-of-line- blocking every other waiting player behind it too, not just the players actually at fault. This is worse than the already-fixed no-common-region crash-loop (§8.16) — that one killed the process; this one fails silently and just never matches anyone again. Two changes, both required together: 1. RunOnce now excludes a failed formation's players and retries with the remaining candidate pool, bounded to 8 attempts per pass. A batch with no viable formation at all (the pre-existing no-common-region case) still returns immediately, since retrying that can't help. 2. That fix was inert without a second one: RunOnce was asking Source for exactly w.Size candidates, so after excluding one failed formation's players there was nothing left to retry against. domain.SelectCandidates was always designed to search a larger pool (anchor plus an arbitrary remainder, widening through it) — the call site just never gave it one. RunOnce now requests up to 10x w.Size, capped at 200. Verified: go build/vet/test -race clean across every server package. Three new matcher tests cover the exclusion retry (an 8-candidate batch whose permanently-doomed oldest 4 still lets the remaining 4 form and claim, correctly excluding the doomed players from the claimed ticket set), that exhausting every attempt surfaces the last real error rather than a silent false/nil, and that Source is actually asked for more than w.Size candidates — a regression guard for exactly the companion bug above. All six pre-existing worker tests still pass unmodified, confirming the fix preserves every prior guarantee (mixed-playlist/duplicate-identity rejection, incomplete batch handling, durable claim failure propagation, Run's existing per-pass-error survival).
2026-09-10 16:04:04 +00:00 · 2026-09-04 17:33:00 +01:00
parent f09ef7da8f
commit 5190cded56
3 changed files with 186 additions and 12 deletions
@@ -90,30 +90,100 @@ func (w Worker) RunOnce(ctx context.Context) (bool, error) {
 		return false, ErrInvalidMatcherSize
 	}
 	now := w.Now()
-	candidates, err := w.Source(ctx, now, w.Playlist, w.Size)
+	// Request headroom beyond exactly w.Size: the exclusion-retry loop below
+	// needs other candidates to fall back to when the oldest-anchor formation
+	// fails, and a pool capped at exactly w.Size leaves nothing to retry with
+	// -- silently reintroducing the same head-of-line wedge the loop exists
+	// to fix. Bounded (not unlimited) so a large playlist backlog doesn't turn
+	// every pass into an expensive scan.
+	candidates, err := w.Source(ctx, now, w.Playlist, w.candidatePoolSize())
 	if err != nil {
 		return false, err
 	}
 	if len(candidates) < w.Size {
 		return false, nil
 	}
-	queue := domain.NewQueue()
+	// A first full pass over every candidate validates playlist and identity
+	// shape once, exactly as before -- these are hard input-format errors, not
+	// "this particular formation didn't work out", so they still fail the pass
+	// immediately rather than being retried below.
 	for _, candidate := range candidates {
 		if candidate.Playlist != w.Playlist {
 			return false, fmt.Errorf("candidate playlist does not match worker")
 		}
-		if _, err := queue.Create(candidate.PlayerID, candidate.TicketID, "matcher-"+candidate.TicketID, candidate, now); err != nil {
+	}
+
+	// FormFromQueue's anchor is always the oldest candidate, deterministically.
+	// If domain.PrepareProposal then rejects that exact formation (mismatched
+	// protocol, incomplete ranked identity metadata, a duplicate-SteamID pair,
+	// etc.), retrying next interval reproduces the identical formation and
+	// fails again -- forever, permanently head-of-line-blocking every other
+	// waiting player behind that anchor, not just the players actually at
+	// fault. Excluding the failed formation's players and retrying with the
+	// remainder, bounded within this one pass, means one bad combination can
+	// no longer wedge the whole playlist; Worker.Run's existing non-fatal
+	// per-pass-error handling still applies if every attempt is exhausted.
+	remaining := candidates
+	var lastErr error
+	for attempt := 0; attempt < maxFormationAttemptsPerPass && len(remaining) >= w.Size; attempt++ {
+		queue := domain.NewQueue()
+		for _, candidate := range remaining {
+			if _, err := queue.Create(candidate.PlayerID, candidate.TicketID, "matcher-"+candidate.TicketID, candidate, now); err != nil {
+				return false, err
+			}
+		}
+		formation, err := domain.FormFromQueue(queue, w.Size, now)
+		if err != nil {
+			// No compatible batch exists at all within what's left of the pool
+			// (e.g. no shared region) -- not specific to one formation, so
+			// retrying within this pass cannot help either.
 			return false, err
 		}
+		prepared, err := w.Prepare(w.NextID(), w.Playlist, formation, now)
+		if err != nil {
+			lastErr = err
+			excluded := make(map[string]bool, len(formation.Selection.Players))
+			for _, player := range formation.Selection.Players {
+				excluded[player.PlayerID] = true
+			}
+			next := make([]domain.Candidate, 0, len(remaining))
+			for _, candidate := range remaining {
+				if !excluded[candidate.PlayerID] {
+					next = append(next, candidate)
+				}
+			}
+			remaining = next
+			continue
+		}
+		return w.claim(ctx, formation, prepared, now)
 	}
-	formation, err := domain.FormFromQueue(queue, w.Size, now)
-	if err != nil {
-		return false, err
-	}
-	prepared, err := w.Prepare(w.NextID(), w.Playlist, formation, now)
-	if err != nil {
-		return false, err
+	return false, lastErr
+}
+
+// maxFormationAttemptsPerPass bounds how many distinct formations RunOnce
+// will try excluding prior failures before deferring to the next interval.
+// Each attempt is pure in-memory work (no durable claim happens until
+// Prepare succeeds), so this is cheap; it exists to keep one pass bounded
+// rather than to conserve resources.
+const maxFormationAttemptsPerPass = 8
+
+const (
+	candidatePoolMultiplier = 10
+	maxCandidatePoolSize    = 200
+)
+
+// candidatePoolSize is how many candidates RunOnce asks Source for. It is
+// deliberately larger than w.Size (see RunOnce) and bounded independently of
+// the playlist's actual backlog size.
+func (w Worker) candidatePoolSize() int {
+	poolSize := w.Size * candidatePoolMultiplier
+	if poolSize > maxCandidatePoolSize {
+		return maxCandidatePoolSize
 	}
+	return poolSize
+}
+
+func (w Worker) claim(ctx context.Context, formation domain.MatchFormation, prepared domain.PreparedProposal, now time.Time) (bool, error) {
 	ticketIDs := make(map[string]string, len(prepared.Proposal.Participants))
 	for _, participant := range prepared.Proposal.Participants {
 		for _, candidate := range formation.Selection.Players {
@@ -3,6 +3,7 @@ package matcher
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sync"
 	"testing"
 	"time"
@@ -163,6 +164,109 @@ func TestRunSurvivesPerPassErrorsAndKeepsRetrying(t *testing.T) {
 	}
 }

+// TestRunOnceRequestsMoreCandidatesThanASingleFormationNeeds guards the
+// companion half of the wedge fix below: excluding a failed formation and
+// retrying is a no-op if Source was only ever asked for exactly w.Size
+// candidates in the first place, since nothing is left afterward. RunOnce
+// must ask Source for headroom beyond one formation's worth.
+func TestRunOnceRequestsMoreCandidatesThanASingleFormationNeeds(t *testing.T) {
+	var requestedLimit int
+	worker := workerFor(func(_ context.Context, _ time.Time, _ domain.Playlist, limit int) ([]domain.Candidate, error) {
+		requestedLimit = limit
+		return candidates(), nil
+	}, &creatorSpy{})
+	if _, err := worker.RunOnce(context.Background()); err != nil {
+		t.Fatalf("RunOnce: %v", err)
+	}
+	if requestedLimit <= worker.Size {
+		t.Fatalf("Source was asked for limit=%d, want more than worker.Size=%d so a failed formation has a remainder to retry against", requestedLimit, worker.Size)
+	}
+}
+
+// TestRunOnceExcludesAFailingFormationAndTriesTheRemainingCandidates covers
+// a wedge distinct from the no-common-region crash-loop above:
+// domain.FormFromQueue's anchor is always the oldest candidate, so if
+// domain.PrepareProposal rejects that exact formation (ranked admission,
+// mismatched protocol, incomplete identity metadata -- anything formation-
+// specific rather than "no batch exists at all"), retrying next interval
+// reproduces the identical formation and fails again forever, permanently
+// head-of-line-blocking every other waiting player behind that anchor too,
+// not just the players actually at fault. RunOnce must exclude the failed
+// formation's players and try the remaining pool within the same pass.
+func TestRunOnceExcludesAFailingFormationAndTriesTheRemainingCandidates(t *testing.T) {
+	now := time.Unix(1000, 0).UTC()
+	batch := make([]domain.Candidate, 8)
+	for i := range batch {
+		batch[i] = domain.Candidate{TicketID: fmt.Sprintf("ticket-%d", i), PlayerID: fmt.Sprintf("player-%d", i), Playlist: domain.Casual, ProtocolVersion: 1, EnqueuedAt: now.Add(time.Duration(i) * time.Second), PredictedRTT: map[string]float64{"EU": 20}}
+	}
+	creator := &creatorSpy{}
+	prepareCalls := 0
+	worker := Worker{
+		Source:   func(context.Context, time.Time, domain.Playlist, int) ([]domain.Candidate, error) { return batch, nil },
+		Creator:  creator,
+		Playlist: domain.Casual,
+		Size:     4,
+		Now:      func() time.Time { return now },
+		NextID:   func() string { return "proposal-1234567890123456" },
+		Prepare: func(id string, playlist domain.Playlist, formation domain.MatchFormation, at time.Time) (domain.PreparedProposal, error) {
+			prepareCalls++
+			for _, player := range formation.Selection.Players {
+				// The oldest four players (the deterministic anchor group) are
+				// the "doomed" combination -- always reject them, every time.
+				if player.PlayerID == "player-0" {
+					return domain.PreparedProposal{}, errors.New("simulated formation-specific rejection")
+				}
+			}
+			return domain.PrepareProposal(id, playlist, formation, nil, domain.RankedArena{}, at)
+		},
+	}
+	formed, err := worker.RunOnce(context.Background())
+	if err != nil || !formed {
+		t.Fatalf("formed=%v err=%v, want the second (players 4-7) formation to succeed", formed, err)
+	}
+	if prepareCalls != 2 {
+		t.Fatalf("Prepare calls=%d, want exactly 2 (the doomed anchor group, then the remainder)", prepareCalls)
+	}
+	if creator.calls != 1 {
+		t.Fatalf("creator calls=%d, want exactly 1", creator.calls)
+	}
+	for _, doomed := range []string{"player-0", "player-1", "player-2", "player-3"} {
+		if _, claimed := creator.ids[doomed]; claimed {
+			t.Fatalf("doomed player %s must not have been claimed by the surviving proposal: %+v", doomed, creator.ids)
+		}
+	}
+	if len(creator.ids) != 4 {
+		t.Fatalf("claimed ticket count=%d, want 4", len(creator.ids))
+	}
+}
+
+// TestRunOnceReturnsTheLastFormationErrorWhenEveryAttemptFails proves the
+// exclusion loop is bounded and still surfaces a real error to Run's
+// existing non-fatal per-pass handling, rather than silently reporting
+// formed=false,err=nil when nothing could ever have worked this pass.
+func TestRunOnceReturnsTheLastFormationErrorWhenEveryAttemptFails(t *testing.T) {
+	now := time.Unix(1000, 0).UTC()
+	batch := make([]domain.Candidate, 8)
+	for i := range batch {
+		batch[i] = domain.Candidate{TicketID: fmt.Sprintf("ticket-%d", i), PlayerID: fmt.Sprintf("player-%d", i), Playlist: domain.Casual, ProtocolVersion: 1, EnqueuedAt: now.Add(time.Duration(i) * time.Second), PredictedRTT: map[string]float64{"EU": 20}}
+	}
+	worker := Worker{
+		Source:   func(context.Context, time.Time, domain.Playlist, int) ([]domain.Candidate, error) { return batch, nil },
+		Creator:  &creatorSpy{},
+		Playlist: domain.Casual,
+		Size:     4,
+		Now:      func() time.Time { return now },
+		NextID:   func() string { return "proposal-1234567890123456" },
+		Prepare: func(string, domain.Playlist, domain.MatchFormation, time.Time) (domain.PreparedProposal, error) {
+			return domain.PreparedProposal{}, errors.New("every formation is doomed")
+		},
+	}
+	formed, err := worker.RunOnce(context.Background())
+	if formed || err == nil || err.Error() != "every formation is doomed" {
+		t.Fatalf("formed=%v err=%v, want the last formation-specific error surfaced", formed, err)
+	}
+}
+
 // TestRunStopsImmediatelyOnConfigurationErrors is the other half of the
 // fix: a genuinely static misconfiguration (true on every future pass, not
 // just this one) must still stop the worker rather than spin forever.