fix: PreviousServer is always nil (minekube#541)

* fix: PreviousServer is always nil

fixes minekube#539
closes minekube#540

- Added a `previousServer` field to `ServerPreConnectEvent` and `serverConnection` to track the server a player was previously connected to.
- Updated constructors and methods to accommodate the new `previousServer` parameter, improving connection request handling and event firing.
- Enhanced `createConnectionRequest` to include the previous connection for better context during server transitions.

* fix: Prevent storing typed nil pointers for previousServer

- Updated the handling of the previousServer field in ServerConnectedEvent and ServerPostConnectEvent to ensure it is only assigned when non-nil, preventing incorrect nil pointer storage.
- This change enhances the integrity of event data during server transitions, ensuring accurate tracking of previous connections.

Author: robinbraemer

pkg/edition/java/proxy/events.go

@@ -442,17 +442,18 @@ func (e *PlayerChooseInitialServerEvent) SetInitialServer(server RegisteredServe
 
 // ServerPreConnectEvent is fired before the player connects to a server.
 type ServerPreConnectEvent struct {
-	player   Player
-	original RegisteredServer
-
-	server RegisteredServer
+	player         Player
+	original       RegisteredServer
+	server         RegisteredServer
+	previousServer RegisteredServer // nil-able
 }
 
-func newServerPreConnectEvent(player Player, server RegisteredServer) *ServerPreConnectEvent {
+func newServerPreConnectEvent(player Player, server RegisteredServer, previousServer RegisteredServer) *ServerPreConnectEvent {
 	return &ServerPreConnectEvent{
-		player:   player,
-		original: server,
-		server:   server,
+		player:         player,
+		original:       server,
+		server:         server,
+		previousServer: previousServer,
 	}
 }
 
@@ -489,6 +490,12 @@ func (e *ServerPreConnectEvent) Server() RegisteredServer {
 	return e.server
 }
 
+// PreviousServer returns the server the player was previously connected to.
+// May return nil if there was none!
+func (e *ServerPreConnectEvent) PreviousServer() RegisteredServer {
+	return e.previousServer
+}
+
 //
 //
 //

pkg/edition/java/proxy/server.go

@@ -219,6 +219,8 @@ type serverConnection struct {
 	player *connectedPlayer
 	log    logr.Logger
 
+	previousServer *registeredServer // nil-able
+
 	completedJoin      atomic.Bool
 	gracefulDisconnect atomic.Bool
 	pendingPings       *lru.SyncCache[int64, time.Time]
@@ -228,11 +230,12 @@ type serverConnection struct {
 	connPhase  phase.BackendConnectionPhase
 }
 
-func newServerConnection(server *registeredServer, player *connectedPlayer) *serverConnection {
+func newServerConnection(server *registeredServer, previousServer *registeredServer, player *connectedPlayer) *serverConnection {
 	return &serverConnection{
-		server:       server,
-		player:       player,
-		pendingPings: lru.NewSync[int64, time.Time](lru.WithCapacity(5)),
+		server:         server,
+		player:         player,
+		previousServer: previousServer,
+		pendingPings:   lru.NewSync[int64, time.Time](lru.WithCapacity(5)),
 		log: player.log.WithName("serverConn").WithValues(
 			"serverName", server.info.Name(),
 			"serverAddr", server.info.Addr()),

pkg/edition/java/proxy/session_backend_transition.go

@@ -163,6 +163,7 @@ func (b *backendTransitionSessionHandler) handleJoinGame(pc *proto.PacketContext
 	if !ok {
 		return
 	}
+	previousServer := b.serverConn.previousServer
 
 	failResult := func(format string, a ...any) {
 		err := fmt.Errorf(format, a...)
@@ -173,9 +174,7 @@ func (b *backendTransitionSessionHandler) handleJoinGame(pc *proto.PacketContext
 
 	b.serverConn.player.mu.Lock()
 	existingConn := b.serverConn.player.connectedServer_
-	var previousServer RegisteredServer
 	if existingConn != nil {
-		previousServer = existingConn.server
 		// Shut down the existing server connection.
 		b.serverConn.player.connectedServer_ = nil
 		b.serverConn.player.mu.Unlock()
@@ -202,9 +201,14 @@ func (b *backendTransitionSessionHandler) handleJoinGame(pc *proto.PacketContext
 	connectedEvent := &ServerConnectedEvent{
 		player:         b.serverConn.player,
 		server:         b.serverConn.server,
-		previousServer: previousServer, // nil-able
 		entityID:       p.EntityID,
 	}
+	// Assign previousServer only if non-nil to prevent storing a typed nil pointer,
+	// which would incorrectly make connectedEvent.previousServer not equal to nil,
+	// as the previousServer field in ServerConnectedEvent is an interface type.
+	if previousServer != nil {
+		connectedEvent.previousServer = previousServer
+	}
 	// Fire event in same goroutine as we don't want to read
 	// more incoming packets while we process the JoinGame!
 	b.eventMgr.Fire(connectedEvent)
@@ -294,7 +298,13 @@ func (b *backendTransitionSessionHandler) handleJoinGame(pc *proto.PacketContext
 	}
 
 	// We're done!
-	postConnectEvent := newServerPostConnectEvent(b.serverConn.player, previousServer)
+	postConnectEvent := newServerPostConnectEvent(b.serverConn.player, nil)
+	// Assign previousServer only if non-nil to prevent storing a typed nil pointer,
+	// which would incorrectly make postConnectEvent.previousServer not equal to nil,
+	// as the previousServer field in ServerPostConnectEvent is an interface type.
+	if previousServer != nil {
+		postConnectEvent.previousServer = previousServer
+	}
 	b.eventMgr.Fire(postConnectEvent)
 	b.requestCtx.result(plainConnectionResult(SuccessConnectionStatus, b.serverConn.server), nil)
 }

pkg/edition/java/proxy/switch.go

@@ -97,12 +97,20 @@ func (p *connectedPlayer) CreateConnectionRequest(server RegisteredServer) Conne
 }
 
 func (p *connectedPlayer) createConnectionRequest(server RegisteredServer) *connectionRequest {
-	return &connectionRequest{server: server, player: p}
+	return p.createConnectionRequestWith(server, p.connectedServer())
+}
+func (p *connectedPlayer) createConnectionRequestWith(server RegisteredServer, previousConn *serverConnection) *connectionRequest {
+	var previousServer *registeredServer
+	if previousConn != nil {
+		previousServer = previousConn.server
+	}
+	return &connectionRequest{server: server, player: p, previousServer: previousServer}
 }
 
 type connectionRequest struct {
-	server RegisteredServer // the target server to connect to
-	player *connectedPlayer // the player to connect to the server
+	server         RegisteredServer  // the target server to connect to
+	player         *connectedPlayer  // the player to connect to the server
+	previousServer *registeredServer // nil-able
 }
 
 func (c *connectionRequest) connect(ctx context.Context) (*connectionResult, error) {
@@ -225,7 +233,7 @@ func (p *connectedPlayer) handleKickEvent(e *KickedFromServerEvent, friendlyReas
 
 	// Make sure we clear the current connected server as the connection is invalid.
 	p.mu.Lock()
-	previouslyConnected := p.connectedServer_ != nil
+	previousConnection := p.connectedServer_
 	if kickedFromCurrent {
 		p.connectedServer_ = nil
 	}
@@ -242,7 +250,7 @@ func (p *connectedPlayer) handleKickEvent(e *KickedFromServerEvent, friendlyReas
 	case *RedirectPlayerKickResult:
 		ctx, cancel := context.WithTimeout(context.Background(), time.Duration(p.config().ConnectionTimeout)*time.Millisecond)
 		defer cancel()
-		redirect, err := p.createConnectionRequest(result.Server).connect(ctx)
+		redirect, err := p.createConnectionRequestWith(result.Server, previousConnection).connect(ctx)
 		if err != nil {
 			p.handleConnectionErr(result.Server, err, true)
 			return
@@ -275,7 +283,7 @@ func (p *connectedPlayer) handleKickEvent(e *KickedFromServerEvent, friendlyReas
 			_ = p.SendMessage(requestedMessage)
 		}
 	case *NotifyKickResult:
-		if e.KickedDuringServerConnect() && previouslyConnected {
+		if e.KickedDuringServerConnect() && previousConnection != nil {
 			_ = p.SendMessage(result.Message)
 		} else {
 			p.Disconnect(result.Message)
@@ -367,7 +375,7 @@ func (c *connectionRequest) internalConnect(ctx context.Context) (result *connec
 		return plainConnectionResult(status, c.server), nil
 	}
 
-	connectEvent := newServerPreConnectEvent(c.player, c.server)
+	connectEvent := newServerPreConnectEvent(c.player, c.server, c.previousServer)
 	c.event().Fire(connectEvent)
 	if !connectEvent.Allowed() {
 		return plainConnectionResult(CanceledConnectionStatus, c.server), nil
@@ -387,7 +395,7 @@ func (c *connectionRequest) internalConnect(ctx context.Context) (result *connec
 		return plainConnectionResult(CanceledConnectionStatus, newDest), nil
 	}
 
-	conn := newServerConnection(server, c.player)
+	conn := newServerConnection(server, c.previousServer, c.player)
 	c.player.setInFlightConnection(conn)
 	defer c.resetIfInFlightIs(conn)
 	return conn.connect(ctx)