Skip to content

Commit 184fa92

Browse files
metis-sprockmetis-sprock
committed
Merge fix/party-separation (#392)
2 parents a5c3bfc + 1c28f2f commit 184fa92

4 files changed

Lines changed: 274 additions & 28 deletions

File tree

‎server/evr_lobby_configure_party_test.go‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
package server
2+
3+
import (
4+
"context"
5+
"sync/atomic"
6+
"testing"
7+
8+
"github.com/gofrs/uuid/v5"
9+
"github.com/heroiclabs/nakama/v3/server/evr"
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
uatomic "go.uber.org/atomic"
13+
)
14+
15+
// kickTrackingStreamManager wraps testStreamManager and records UserLeave calls
16+
// so tests can assert whether StreamUserKick was invoked.
17+
type kickTrackingStreamManager struct {
18+
testStreamManager
19+
kickCount atomic.Int32
20+
}
21+
22+
func (m *kickTrackingStreamManager) UserLeave(stream PresenceStream, userID, sessionID uuid.UUID) error {
23+
m.kickCount.Add(1)
24+
return nil
25+
}
26+
27+
// TestConfigureParty_FollowerNotOnMatchmakingStream_NotKicked verifies that
28+
// when the leader re-queues for matchmaking, followers who are party members
29+
// but not yet on the matchmaking stream (e.g. transitioning between queue
30+
// cycles) are NOT kicked from the stream.
31+
//
32+
// Reproduces the production bug where:
33+
// 1. Leader's matchmaking times out
34+
// 2. Leader re-queues, calling configureParty
35+
// 3. Follower's matchmaking stream was cleaned up during the timeout
36+
// 4. configureParty sees follower not on stream → kicks them
37+
// 5. Kick cancels follower's context → follower ends up in solo lobby
38+
func TestConfigureParty_FollowerNotOnMatchmakingStream_NotKicked(t *testing.T) {
39+
logger := loggerForTest(t)
40+
tracker := newMockMatchmakingTracker()
41+
mm, mmCleanup := createLightMatchmaker(t, logger)
42+
defer mmCleanup()
43+
44+
ksm := &kickTrackingStreamManager{}
45+
dmr := &DummyMessageRouter{}
46+
pr := NewLocalPartyRegistry(logger, cfg, mm, tracker, ksm, dmr, "testnode")
47+
48+
groupName := "testgroup"
49+
50+
// Create the leader session.
51+
leaderSession := newTestSessionForParty(t, "leader", tracker, pr)
52+
53+
// Create the follower session.
54+
followerSession := newTestSessionForParty(t, "follower", tracker, pr)
55+
56+
// Both join the same party group (leader joins first, becomes leader).
57+
_, leaderIsLeader, err := JoinPartyGroup(leaderSession, groupName, MatchID{})
58+
require.NoError(t, err)
59+
require.True(t, leaderIsLeader)
60+
61+
_, followerIsLeader, err := JoinPartyGroup(followerSession, groupName, MatchID{})
62+
require.NoError(t, err)
63+
require.False(t, followerIsLeader)
64+
65+
// Set up the EvrPipeline with tracker and stream manager so that
66+
// nk.StreamUserGet and nk.StreamUserKick work.
67+
pipeline := &EvrPipeline{
68+
node: "testnode",
69+
nk: &RuntimeGoNakamaModule{
70+
tracker: tracker,
71+
streamManager: ksm,
72+
},
73+
}
74+
75+
groupID := uuid.Must(uuid.NewV4())
76+
lobbyParams := &LobbySessionParameters{
77+
PartyGroupName: groupName,
78+
GroupID: groupID,
79+
PartySize: uatomic.NewInt64(1),
80+
}
81+
82+
// Leader IS on the matchmaking stream (they just re-queued).
83+
tracker.Track(context.Background(), leaderSession.id,
84+
PresenceStream{Mode: StreamModeMatchmaking, Subject: groupID},
85+
leaderSession.userID,
86+
PresenceMeta{Status: "matchmaking"})
87+
88+
// Follower is NOT on the matchmaking stream. This simulates the
89+
// re-queue race: the follower's stream was cleaned up when the
90+
// previous matchmaking cycle ended, and they haven't re-joined yet.
91+
// (Deliberately not tracking follower on matchmaking stream.)
92+
93+
// Call configureParty as the leader.
94+
lobbyGroup, memberSessionIDs, isLeader, err := pipeline.configureParty(
95+
context.Background(), logger, leaderSession, lobbyParams)
96+
require.NoError(t, err)
97+
require.True(t, isLeader)
98+
99+
// The follower should still be in the party group.
100+
assert.Equal(t, 2, lobbyGroup.Size(),
101+
"party should still have 2 members (leader + follower)")
102+
103+
// The follower's session ID should be in the returned member list.
104+
assert.Contains(t, memberSessionIDs, followerSession.id,
105+
"follower should be included in memberSessionIDs")
106+
107+
// CRITICAL: StreamUserKick should NOT have been called.
108+
// The current buggy code DOES call it, which cancels the follower's
109+
// matchmaking context and causes them to end up in a solo lobby.
110+
assert.Equal(t, int32(0), ksm.kickCount.Load(),
111+
"follower should NOT be kicked from matchmaking stream — "+
112+
"they are a party member transitioning between queue cycles")
113+
}
114+
115+
// TestLeavePartyStream_RemovesPartyStreamPresence verifies that
116+
// LeavePartyStream removes the player's party stream tracking from the
117+
// tracker. This documents why LeavePartyStream must NOT be called on
118+
// matchmaking errors — it destroys the party stream presence.
119+
func TestLeavePartyStream_RemovesPartyStreamPresence(t *testing.T) {
120+
tracker := newMockMatchmakingTracker()
121+
122+
sessionID := uuid.Must(uuid.NewV4())
123+
userID := uuid.Must(uuid.NewV4())
124+
partyID := uuid.Must(uuid.NewV4())
125+
126+
partyStream := PresenceStream{Mode: StreamModeParty, Subject: partyID, Label: "testnode"}
127+
128+
// Manually track the session on the party stream.
129+
tracker.Track(context.Background(), sessionID, partyStream, userID, PresenceMeta{})
130+
require.True(t, tracker.hasPresence(sessionID, partyStream, userID),
131+
"setup: session should be on party stream")
132+
133+
// Create a minimal session with the tracker.
134+
s := &sessionWS{}
135+
s.id = sessionID
136+
s.userID = userID
137+
s.tracker = tracker
138+
139+
// LeavePartyStream should remove the party stream presence.
140+
LeavePartyStream(s)
141+
142+
assert.False(t, tracker.hasPresence(sessionID, partyStream, userID),
143+
"LeavePartyStream should remove party stream presence")
144+
}
145+
146+
// TestHandleMatchmakingError_PreservesPartyStream calls handleMatchmakingError
147+
// directly and verifies the party stream is NOT destroyed.
148+
//
149+
// The production bug: matchmaking error → handleMatchmakingError calls
150+
// LeavePartyStream → party destroyed → player re-queues as solo leader.
151+
func TestHandleMatchmakingError_PreservesPartyStream(t *testing.T) {
152+
tracker := newMockMatchmakingTracker()
153+
154+
sessionID := uuid.Must(uuid.NewV4())
155+
userID := uuid.Must(uuid.NewV4())
156+
partyID := uuid.Must(uuid.NewV4())
157+
158+
partyStream := PresenceStream{Mode: StreamModeParty, Subject: partyID, Label: "testnode"}
159+
160+
// Player is on the party stream (in a party).
161+
tracker.Track(context.Background(), sessionID, partyStream, userID, PresenceMeta{})
162+
163+
s := &sessionWS{}
164+
s.id = sessionID
165+
s.userID = userID
166+
s.tracker = tracker
167+
168+
groupID := uuid.Must(uuid.NewV4())
169+
lobbyParams := &LobbySessionParameters{
170+
GroupID: groupID,
171+
PartySize: uatomic.NewInt64(1),
172+
}
173+
lobbyParams.Mode = evr.ModeArenaPublic
174+
175+
// Call handleMatchmakingError with a generic lobby error.
176+
someError := NewLobbyError(InternalError, "test error")
177+
_ = handleMatchmakingError(loggerForTest(t), s, lobbyParams, &testMetrics{}, someError)
178+
179+
// Party stream must survive — players don't leave parties on matchmaking errors.
180+
assert.True(t, tracker.hasPresence(sessionID, partyStream, userID),
181+
"party stream should survive matchmaking error — "+
182+
"players don't leave parties just because matchmaking failed")
183+
}
184+
185+
// TestConfigureParty_AllFollowersOnStream_NoKick is a baseline test:
186+
// when all followers ARE on the matchmaking stream, no kicks should occur.
187+
func TestConfigureParty_AllFollowersOnStream_NoKick(t *testing.T) {
188+
logger := loggerForTest(t)
189+
tracker := newMockMatchmakingTracker()
190+
mm, mmCleanup := createLightMatchmaker(t, logger)
191+
defer mmCleanup()
192+
193+
ksm := &kickTrackingStreamManager{}
194+
dmr := &DummyMessageRouter{}
195+
pr := NewLocalPartyRegistry(logger, cfg, mm, tracker, ksm, dmr, "testnode")
196+
197+
groupName := "testgroup"
198+
199+
leaderSession := newTestSessionForParty(t, "leader", tracker, pr)
200+
followerSession := newTestSessionForParty(t, "follower", tracker, pr)
201+
202+
_, _, err := JoinPartyGroup(leaderSession, groupName, MatchID{})
203+
require.NoError(t, err)
204+
_, _, err = JoinPartyGroup(followerSession, groupName, MatchID{})
205+
require.NoError(t, err)
206+
207+
pipeline := &EvrPipeline{
208+
node: "testnode",
209+
nk: &RuntimeGoNakamaModule{
210+
tracker: tracker,
211+
streamManager: ksm,
212+
},
213+
}
214+
215+
groupID := uuid.Must(uuid.NewV4())
216+
lobbyParams := &LobbySessionParameters{
217+
PartyGroupName: groupName,
218+
GroupID: groupID,
219+
PartySize: uatomic.NewInt64(1),
220+
}
221+
222+
// Both leader and follower are on the matchmaking stream.
223+
tracker.Track(context.Background(), leaderSession.id,
224+
PresenceStream{Mode: StreamModeMatchmaking, Subject: groupID},
225+
leaderSession.userID,
226+
PresenceMeta{Status: "matchmaking"})
227+
228+
tracker.Track(context.Background(), followerSession.id,
229+
PresenceStream{Mode: StreamModeMatchmaking, Subject: groupID},
230+
followerSession.userID,
231+
PresenceMeta{Status: "matchmaking"})
232+
233+
lobbyGroup, memberSessionIDs, isLeader, err := pipeline.configureParty(
234+
context.Background(), logger, leaderSession, lobbyParams)
235+
require.NoError(t, err)
236+
require.True(t, isLeader)
237+
238+
assert.Equal(t, 2, lobbyGroup.Size())
239+
assert.Contains(t, memberSessionIDs, followerSession.id)
240+
assert.Equal(t, int32(0), ksm.kickCount.Load(),
241+
"no kicks should occur when all followers are on the stream")
242+
}

‎server/evr_lobby_find.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ func (p *EvrPipeline) configureParty(ctx context.Context, logger *zap.Logger, se
268268
}
269269
// Populate PartyID from the registry-assigned party (random UUID, not derived from group name).
270270
lobbyParams.PartyID = lobbyGroup.ID()
271-
logger.Debug("Joined party group", zap.String("partyID", lobbyGroup.IDStr()))
271+
logger.Debug("Joined party group", zap.String("partyID", lobbyGroup.IDStr()), zap.String("partyGroupName", lobbyParams.PartyGroupName))
272272

273273
// If this is the leader, then set the presence status to the current match ID.
274274
if isLeader {

‎server/evr_lobby_group_split_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ func newTestSessionForParty(t *testing.T, username string, tracker Tracker, part
2626
s.logger = loggerForTest(t)
2727
s.format = SessionFormatProtobuf
2828
s.outgoingCh = make(chan []byte, 16)
29+
s.tracker = tracker
2930
s.pipeline = &Pipeline{node: "testnode"}
3031
s.pipeline.tracker = tracker
3132
s.pipeline.partyRegistry = partyRegistry

‎server/evr_lobby_session.go‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -74,33 +74,7 @@ func (p *EvrPipeline) handleLobbySessionRequest(ctx context.Context, logger *zap
7474
if err == nil {
7575
return nil
7676
}
77-
code := InternalError
78-
79-
if errors.Is(err, context.Canceled) {
80-
logger.Debug("Matchmaking context canceled")
81-
return nil
82-
}
83-
if errors.Is(err, ErrMatchmakingTimeout) {
84-
logger.Warn("Matchmaking timed out", zap.String("mode", lobbyParams.Mode.String()), zap.Error(err))
85-
err = NewLobbyError(Timeout, "matchmaking timed out")
86-
} else {
87-
lobbyErr := &LobbyError{}
88-
if errors.As(err, &lobbyErr) {
89-
code = lobbyErr.code
90-
} else {
91-
logger.Warn("Unexpected error while finding match", zap.Error(err))
92-
code = InternalError
93-
}
94-
95-
tags := lobbyParams.MetricsTags()
96-
tags["error_code"] = strconv.Itoa(int(code))
97-
tags["error_str"] = code.String()
98-
99-
p.nk.metrics.CustomCounter("lobby_find_match_error", tags, int64(lobbyParams.GetPartySize()))
100-
// On error, leave any party the user might be a member of.
101-
LeavePartyStream(session)
102-
}
103-
return err
77+
return handleMatchmakingError(logger, session, lobbyParams, p.nk.metrics, err)
10478
}
10579
return nil
10680

@@ -134,6 +108,35 @@ func (p *EvrPipeline) handleLobbySessionRequest(ctx context.Context, logger *zap
134108
return nil
135109
}
136110

111+
// handleMatchmakingError classifies a matchmaking error and performs cleanup.
112+
// Returns nil if the error is a context cancellation (no error to report),
113+
// or a wrapped LobbyError for all other cases.
114+
func handleMatchmakingError(logger *zap.Logger, session *sessionWS, lobbyParams *LobbySessionParameters, metrics Metrics, err error) error {
115+
if errors.Is(err, context.Canceled) {
116+
logger.Debug("Matchmaking context canceled")
117+
return nil
118+
}
119+
if errors.Is(err, ErrMatchmakingTimeout) {
120+
logger.Warn("Matchmaking timed out", zap.String("mode", lobbyParams.Mode.String()), zap.Error(err))
121+
return NewLobbyError(Timeout, "matchmaking timed out")
122+
}
123+
124+
code := InternalError
125+
lobbyErr := &LobbyError{}
126+
if errors.As(err, &lobbyErr) {
127+
code = lobbyErr.code
128+
} else {
129+
logger.Warn("Unexpected error while finding match", zap.Error(err))
130+
}
131+
132+
tags := lobbyParams.MetricsTags()
133+
tags["error_code"] = strconv.Itoa(int(code))
134+
tags["error_str"] = code.String()
135+
136+
metrics.CustomCounter("lobby_find_match_error", tags, int64(lobbyParams.GetPartySize()))
137+
return err
138+
}
139+
137140
func LobbyPrepareSession(ctx context.Context, nk runtime.NakamaModule, matchID MatchID, settings *MatchSettings) (*MatchLabel, error) {
138141

139142
response, err := SignalMatch(ctx, nk, matchID, SignalPrepareSession, settings)

0 commit comments

Comments
 (0)