Fix multiplayer join visibility snapshots

.github/workflows/ci.yml

@@ -54,6 +54,31 @@ jobs:
           make -C pkg/enet/enet clean
           make -C pkg/enet/enet
 
+      - name: Run relay regression tests
+        run: |
+          set -o pipefail
+          go test ./pkg/gameserver/relay -count=3 2>&1 | tee /tmp/relay-regression-test.log
+
+      - name: Run gameserver multiplayer regression tests
+        run: |
+          set -o pipefail
+          go test -vet=off ./pkg/gameserver -count=3 2>&1 | tee /tmp/gameserver-regression-test.log
+
+      - name: Run relay race tests
+        run: |
+          set -o pipefail
+          go test -race ./pkg/gameserver/relay -count=1 2>&1 | tee /tmp/relay-race-test.log
+
+      - name: Upload regression test logs on failure
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: void-arena-test-logs-${{ github.sha }}
+          path: /tmp/*test.log
+          if-no-files-found: ignore
+          retention-days: 7
+          compression-level: 0
+
       - name: Build game assets
         env:
           ASSET_MAPS: "complex dust2 turbine"

pkg/gameserver/clients.go

@@ -187,6 +187,46 @@ func (cm *ClientManager) Relay(from *Client, messages ...P.Message) {
 	cm.broadcast(exclude(from), messages...)
 }
 
+func appendClientIdentity(messages []P.Message, client *Client) []P.Message {
+	if client.IsBot {
+		ownerCN := int32(-1)
+		if client.Owner != nil {
+			ownerCN = int32(client.Owner.CN)
+		}
+		return append(messages, P.InitAI{
+			Aiclientnum:    int32(client.CN),
+			Ownerclientnum: ownerCN,
+			Aitype:         1,
+			Aiskill:        client.BotSkill,
+			Playermodel:    int32(client.Model),
+			Name:           client.Name,
+			Team:           client.Team.Name,
+		})
+	}
+
+	return append(messages, P.InitClient{
+		int32(client.CN), client.Name, client.Team.Name, int32(client.Model),
+	})
+}
+
+func appendClientVisibleState(messages []P.Message, client *Client) []P.Message {
+	if client.State == playerstate.Spectator {
+		return append(messages, P.Spectator{
+			Client:     int32(client.CN),
+			Spectating: true,
+		})
+	}
+
+	if client.State == playerstate.Alive || client.State == playerstate.Editing {
+		return append(messages, P.SpawnState{
+			Client:      int32(client.CN),
+			EntityState: client.ToWire(),
+		})
+	}
+
+	return messages
+}
+
 // Sends 'welcome' information to a newly joined client like map, mode, time left, other players, etc.
 func (s *Server) SendWelcome(c *Client) {
 	messages := []P.Message{
@@ -233,6 +273,16 @@ func (s *Server) SendWelcome(c *Client) {
 		Reason: -1,
 	})
 
+	// Identity packets must arrive before resume/spawn state. Sending state first
+	// can leave late joiners with an entity state for an unknown client number,
+	// which is one of the intermittent invisible-player failure modes.
+	for _, client := range s.Clients.clients {
+		if client == c {
+			continue
+		}
+		messages = appendClientIdentity(messages, client)
+	}
+
 	// tell the client how to spawn (what health, what armour, what weapons, what ammo, etc.)
 	if c.State == playerstate.Spectator {
 		messages = append(messages, P.Spectator{
@@ -267,32 +317,6 @@ func (s *Server) SendWelcome(c *Client) {
 	}
 	messages = append(messages, resume)
 
-	// send other client's state (name, team, playermodel)
-	for _, client := range s.Clients.clients {
-		if client == c {
-			continue
-		}
-		if client.IsBot {
-			ownerCN := int32(-1)
-			if client.Owner != nil {
-				ownerCN = int32(client.Owner.CN)
-			}
-			messages = append(messages, P.InitAI{
-				Aiclientnum:    int32(client.CN),
-				Ownerclientnum: ownerCN,
-				Aitype:         1,
-				Aiskill:        client.BotSkill,
-				Playermodel:    int32(client.Model),
-				Name:           client.Name,
-				Team:           client.Team.Name,
-			})
-			continue
-		}
-		messages = append(messages, P.InitClient{
-			int32(client.CN), client.Name, client.Team.Name, int32(client.Model),
-		})
-	}
-
 	c.Send(messages...)
 }
 
@@ -325,31 +349,15 @@ func (cm *ClientManager) Disconnect(c *Client, reason disconnectreason.ID) {
 
 // Informs all other clients that a client joined the game.
 func (cm *ClientManager) InformOthersOfJoin(c *Client) {
+	messages := appendClientIdentity(nil, c)
+	messages = appendClientVisibleState(messages, c)
+
 	if c.IsBot {
-		ownerCN := int32(-1)
-		if c.Owner != nil {
-			ownerCN = int32(c.Owner.CN)
-		}
-		cm.Broadcast(P.InitAI{
-			Aiclientnum:    int32(c.CN),
-			Ownerclientnum: ownerCN,
-			Aitype:         1,
-			Aiskill:        c.BotSkill,
-			Playermodel:    int32(c.Model),
-			Name:           c.Name,
-			Team:           c.Team.Name,
-		})
+		cm.Broadcast(messages...)
 		return
 	}
-	cm.Relay(c, P.InitClient{
-		int32(c.CN), c.Name, c.Team.Name, int32(c.Model),
-	})
 
-	if c.State == playerstate.Spectator {
-		cm.Relay(c, P.Spectator{
-			int32(c.CN), true,
-		})
-	}
+	cm.Relay(c, messages...)
 }
 
 func (s *Server) MapChange() {

pkg/gameserver/multiplayer_sync_test.go

@@ -0,0 +1,160 @@
+package gameserver
+
+import (
+	"testing"
+	"time"
+
+	P "github.com/cfoust/sour/pkg/game/protocol"
+	servergame "github.com/cfoust/sour/pkg/gameserver/game"
+	"github.com/cfoust/sour/pkg/gameserver/protocol/gamemode"
+	"github.com/cfoust/sour/pkg/gameserver/protocol/mastermode"
+	"github.com/cfoust/sour/pkg/gameserver/protocol/playerstate"
+	"github.com/cfoust/sour/pkg/gameserver/timer"
+	"github.com/cfoust/sour/pkg/utils"
+)
+
+type syncTestMode struct{}
+
+func (syncTestMode) ID() gamemode.ID { return gamemode.FFA }
+func (syncTestMode) NeedsMapInfo() bool { return false }
+func (syncTestMode) Leave(*servergame.Player) {}
+func (syncTestMode) CanSpawn(*servergame.Player) bool { return true }
+func (syncTestMode) Spawn(*servergame.PlayerState) {}
+func (syncTestMode) HandleFrag(*servergame.Player, *servergame.Player) {}
+func (syncTestMode) Pause() {}
+func (syncTestMode) Resume() {}
+func (syncTestMode) CleanUp() {}
+
+type syncTestClock struct{}
+
+func (syncTestClock) Start() {}
+func (syncTestClock) Pause(*servergame.Player) {}
+func (syncTestClock) Paused() bool { return false }
+func (syncTestClock) Resume(*servergame.Player) {}
+func (syncTestClock) Stop() {}
+func (syncTestClock) Ended() bool { return false }
+func (syncTestClock) TimeLeft() time.Duration { return 10 * time.Minute }
+func (syncTestClock) SetTimeLeft(time.Duration) {}
+func (syncTestClock) Leave(*servergame.Player) {}
+func (syncTestClock) CleanUp() {}
+
+func newSyncTestClient(cn uint32, session uint32, out chan ServerPacket) *Client {
+	client := NewClient(cn, session, out)
+	client.Name = "player"
+	client.Model = 1
+	client.Joined = true
+	client.State = playerstate.Alive
+	client.LifeSequence = int32(cn + 1)
+	client.QuadTimer = timer.NewTimer(0)
+	return client
+}
+
+func TestWelcomeInitializesPlayersBeforeResume(t *testing.T) {
+	joinOut := make(chan ServerPacket, 4)
+	existingOut := make(chan ServerPacket, 4)
+
+	existing := newSyncTestClient(0, 100, existingOut)
+	existing.Name = "existing"
+	joining := newSyncTestClient(1, 101, joinOut)
+	joining.Name = "joining"
+
+	clients := &ClientManager{
+		clients:    []*Client{existing, joining},
+		broadcasts: utils.NewTopic[[]P.Message](),
+	}
+
+	server := &Server{
+		Config: &Config{},
+		State: &State{
+			Clock:      syncTestClock{},
+			GameMode:   syncTestMode{},
+			Map:        "complex",
+			MasterMode: mastermode.Auth,
+		},
+		Clients: clients,
+	}
+
+	server.SendWelcome(joining)
+
+	packet := <-joinOut
+	initIndex := -1
+	resumeIndex := -1
+	for index, message := range packet.Messages {
+		switch message.(type) {
+		case P.InitClient:
+			if initIndex < 0 {
+				initIndex = index
+			}
+		case P.Resume:
+			resumeIndex = index
+		}
+	}
+
+	if initIndex < 0 {
+		t.Fatal("welcome snapshot did not include existing player's InitClient")
+	}
+	if resumeIndex < 0 {
+		t.Fatal("welcome snapshot did not include Resume state")
+	}
+	if initIndex > resumeIndex {
+		t.Fatalf("InitClient must be sent before Resume so late joiners can bind entity state: init=%d resume=%d", initIndex, resumeIndex)
+	}
+}
+
+func TestInformOthersOfJoinSendsVisibleHumanSnapshot(t *testing.T) {
+	existingOut := make(chan ServerPacket, 4)
+	joiningOut := make(chan ServerPacket, 4)
+
+	existing := newSyncTestClient(0, 100, existingOut)
+	joining := newSyncTestClient(1, 101, joiningOut)
+
+	clients := &ClientManager{
+		clients:    []*Client{existing, joining},
+		broadcasts: utils.NewTopic[[]P.Message](),
+	}
+
+	clients.InformOthersOfJoin(joining)
+
+	packet := <-existingOut
+	if len(packet.Messages) < 2 {
+		t.Fatalf("expected InitClient and SpawnState, got %d messages", len(packet.Messages))
+	}
+	if _, ok := packet.Messages[0].(P.InitClient); !ok {
+		t.Fatalf("first join packet must identify the player, got %T", packet.Messages[0])
+	}
+	if _, ok := packet.Messages[1].(P.SpawnState); !ok {
+		t.Fatalf("second join packet must make the player visible, got %T", packet.Messages[1])
+	}
+}
+
+func TestInformOthersOfJoinSendsVisibleBotSnapshotAbove127(t *testing.T) {
+	ownerOut := make(chan ServerPacket, 4)
+	otherOut := make(chan ServerPacket, 4)
+
+	owner := newSyncTestClient(0, 100, ownerOut)
+	other := newSyncTestClient(1, 101, otherOut)
+	bot := NewBot(128, owner, 70, 2, "Vector")
+	bot.State = playerstate.Alive
+	bot.LifeSequence = 5
+	bot.QuadTimer = timer.NewTimer(0)
+
+	clients := &ClientManager{
+		clients:    []*Client{owner, other, bot},
+		broadcasts: utils.NewTopic[[]P.Message](),
+	}
+
+	clients.InformOthersOfJoin(bot)
+
+	for name, out := range map[string]chan ServerPacket{"owner": ownerOut, "other": otherOut} {
+		packet := <-out
+		if len(packet.Messages) < 2 {
+			t.Fatalf("%s expected InitAI and SpawnState, got %d messages", name, len(packet.Messages))
+		}
+		if _, ok := packet.Messages[0].(P.InitAI); !ok {
+			t.Fatalf("%s first bot packet must identify the AI, got %T", name, packet.Messages[0])
+		}
+		if _, ok := packet.Messages[1].(P.SpawnState); !ok {
+			t.Fatalf("%s second bot packet must make the AI visible, got %T", name, packet.Messages[1])
+		}
+	}
+}

pkg/gameserver/relay/relay_test.go

@@ -14,17 +14,17 @@ type delivered struct {
 
 func TestSingleClientPacketsAreNotReplayedToLateJoiner(t *testing.T) {
 	r := New()
-	first, _ := r.AddClient(1, func(uint8, []protocol.Message) {})
+	_, firstPackets := r.AddClient(1, func(uint8, []protocol.Message) {})
 
-	first.Publish(protocol.Text{Text: "stale"})
+	firstPackets.Publish(protocol.Text{Text: "stale"})
 	time.Sleep(40 * time.Millisecond)
 
 	received := make(chan delivered, 4)
 	_, _ = r.AddClient(2, func(channel uint8, payload []protocol.Message) {
 		received <- delivered{channel: channel, payload: payload}
 	})
 
-	first.Publish(protocol.Text{Text: "fresh"})
+	firstPackets.Publish(protocol.Text{Text: "fresh"})
 
 	select {
 	case packet := <-received:
@@ -130,4 +130,4 @@ func TestBotSourceDoesNotEchoToOwner(t *testing.T) {
 	case <-time.After(time.Second):
 		t.Fatal("bot packet was not delivered to another player")
 	}
-}
+}