diff --git a/server/matcher/worker.go b/server/matcher/worker.go index 46259ea1..13d037d1 100644 --- a/server/matcher/worker.go +++ b/server/matcher/worker.go @@ -4,12 +4,27 @@ package matcher import ( "context" + "errors" "fmt" "time" "github.com/cosmic-clash/cosmic-clash/server/domain" ) +// These three are the only RunOnce failures Run treats as fatal to the whole +// worker: they're static misconfiguration, true on every future pass just as +// much as this one, so retrying cannot help. Every other RunOnce error -- +// a source read hiccup, no common region among the current candidate pool, +// a losing race against another matcher replica, an incomplete batch -- is a +// single pass's worth of "no match formed this time," a routine and +// expected steady state that must not take matching down for every other +// player still waiting behind it. +var ( + ErrWorkerNotConfigured = errors.New("matcher worker is not configured") + ErrUnsupportedPlaylist = errors.New("unsupported matcher playlist") + ErrInvalidMatcherSize = errors.New("invalid matcher size") +) + type CandidateSource func(context.Context, time.Time, domain.Playlist, int) ([]domain.Candidate, error) type ProposalCreator interface { @@ -42,7 +57,10 @@ func (w Worker) Run(ctx context.Context, interval time.Duration) error { } for { if _, err := w.RunOnce(ctx); err != nil { - return err + if errors.Is(err, ErrWorkerNotConfigured) || errors.Is(err, ErrUnsupportedPlaylist) || errors.Is(err, ErrInvalidMatcherSize) { + return err + } + // Not fatal -- fall through and retry next interval. } timer := time.NewTimer(interval) select { @@ -59,13 +77,13 @@ func (w Worker) Run(ctx context.Context, interval time.Duration) error { // a stale cache therefore fails safely and can be retried on the next pass. func (w Worker) RunOnce(ctx context.Context) (bool, error) { if w.Source == nil || w.Creator == nil || w.Now == nil || w.NextID == nil || w.Prepare == nil { - return false, fmt.Errorf("matcher worker is not configured") + return false, ErrWorkerNotConfigured } if w.Playlist != domain.Casual && w.Playlist != domain.Ranked { - return false, fmt.Errorf("unsupported matcher playlist") + return false, ErrUnsupportedPlaylist } if w.Size < 2 || w.Size > 6 { - return false, fmt.Errorf("invalid matcher size") + return false, ErrInvalidMatcherSize } now := w.Now() candidates, err := w.Source(ctx, now, w.Playlist, w.Size) diff --git a/server/matcher/worker_test.go b/server/matcher/worker_test.go index ea6a030a..1a5bea46 100644 --- a/server/matcher/worker_test.go +++ b/server/matcher/worker_test.go @@ -3,6 +3,7 @@ package matcher import ( "context" "errors" + "sync" "testing" "time" @@ -104,3 +105,76 @@ func TestRunOnceRejectsMixedPlaylistAndDuplicateIdentityBatches(t *testing.T) { t.Fatalf("creator calls=%d", creator.calls) } } + +// TestRunSurvivesPerPassErrorsAndKeepsRetrying reproduces a real production +// bug found via a live integration test (multiplayer-next.md 8.40): two real +// players queued with no verified common region formed exactly this +// "source succeeds, formation fails" shape, and the matcher process died +// entirely rather than waiting for a compatible batch -- silently taking +// matchmaking down for every other player behind them too, not just the +// incompatible pair. Run must survive a per-pass RunOnce error and try +// again next interval rather than returning immediately. +func TestRunSurvivesPerPassErrorsAndKeepsRetrying(t *testing.T) { + var mu sync.Mutex + attempts := 0 + creatorCalls := 0 + worker := workerFor(func(context.Context, time.Time, domain.Playlist, int) ([]domain.Candidate, error) { + mu.Lock() + defer mu.Unlock() + attempts++ + if attempts == 1 { + // Same failure shape as domain.FormFromQueue's "no compatible + // candidates" -- RunOnce still returns a non-nil error here, only + // Run's handling of it is what this test is about. + return nil, errors.New("no common region") + } + return candidates(), nil + }, ProposalCreatorFunc(func(context.Context, domain.Proposal, map[string]string, time.Time) error { + mu.Lock() + defer mu.Unlock() + creatorCalls++ + return nil + })) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + done := make(chan error, 1) + go func() { done <- worker.Run(ctx, 5*time.Millisecond) }() + deadline := time.After(1 * time.Second) + for { + mu.Lock() + calls := creatorCalls + mu.Unlock() + if calls > 0 { + break + } + select { + case err := <-done: + t.Fatalf("Run returned early on a per-pass error instead of retrying: %v", err) + case <-deadline: + t.Fatal("Run never recovered from the first pass's error") + default: + time.Sleep(time.Millisecond) + } + } + mu.Lock() + defer mu.Unlock() + if creatorCalls != 1 { + t.Fatalf("creator calls=%d, want exactly 1 once formation finally succeeded", creatorCalls) + } +} + +// 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. +func TestRunStopsImmediatelyOnConfigurationErrors(t *testing.T) { + worker := workerFor(func(context.Context, time.Time, domain.Playlist, int) ([]domain.Candidate, error) { + return candidates(), nil + }, &creatorSpy{}) + worker.Playlist = domain.Playlist("invalid") + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err := worker.Run(ctx, 5*time.Millisecond) + if !errors.Is(err, ErrUnsupportedPlaylist) { + t.Fatalf("Run() error = %v, want ErrUnsupportedPlaylist", err) + } +}