fix(room_manager): only sort rooms removal if major version diff (#640)

We only want to apply sorting by version if the room has a major version of
difference compared to the active scheduler version. This prevents minor
updates forcing the sort, which would cause occupied rooms to be deleted
first (from a prior minor version) when we don't want that behavior. Thus,
check if it has a major version diff between room and active scheduler

internal/core/entities/scheduler.go

@@ -26,6 +26,7 @@ import (
 	"errors"
 	"time"
 
+	"github.com/Masterminds/semver"
 	"github.com/topfreegames/maestro/internal/core/entities/autoscaling"
 	"github.com/topfreegames/maestro/internal/core/entities/port"
 
@@ -202,3 +203,17 @@ func (s *Scheduler) HasValidPortRangeConfiguration() error {
 
 	return nil
 }
+
+func (s *Scheduler) IsSameMajorVersion(otherVersion string) bool {
+	otherVer, err := semver.NewVersion(otherVersion)
+	if err != nil {
+		return false
+	}
+
+	schedulerVer, err := semver.NewVersion(s.Spec.Version)
+	if err != nil {
+		return false
+	}
+
+	return otherVer.Major() == schedulerVer.Major()
+}

internal/core/entities/scheduler_test.go

@@ -399,3 +399,31 @@ func TestHasValidPortRangeConfiguration(t *testing.T) {
 		})
 	}
 }
+
+func TestIsSameMajorVersion(t *testing.T) {
+	s := &entities.Scheduler{}
+	s.Spec.Version = "v10.5.0"
+
+	testCases := []struct {
+		name         string
+		otherVersion string
+		expected     bool
+	}{
+		{"Both versions are the same", "v10.5.0", true},
+		{"Only minor version difference", "v10.6.0", true},
+		{"Only bugfix version difference", "v10.5.1", true},
+		{"otherVersion without v prefix", "10.0.0", true},
+		{"Major version difference", "v11.0.0", false},
+		{"Empty otherVersion", "", false},
+		{"otherVersion not semantic", "version10", false},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := s.IsSameMajorVersion(tc.otherVersion)
+			if result != tc.expected {
+				t.Errorf("Test %s failed: expected %v, got %v", tc.name, tc.expected, result)
+			}
+		})
+	}
+}

internal/core/operations/rooms/remove/executor.go

@@ -123,7 +123,7 @@ func (e *Executor) removeRoomsByAmount(ctx context.Context, logger *zap.Logger,
 	}
 
 	logger.Info("removing rooms by amount sorting by version",
-		zap.Array("originalRoomsOrder", zapcore.ArrayMarshalerFunc(func(enc zapcore.ArrayEncoder) error {
+		zap.Array("rooms:", zapcore.ArrayMarshalerFunc(func(enc zapcore.ArrayEncoder) error {
 			for _, room := range rooms {
 				enc.AppendString(fmt.Sprintf("%s-%s-%s", room.ID, room.Version, room.Status.String()))
 			}

internal/core/services/rooms/room_manager.go

@@ -247,7 +247,7 @@ func (m *RoomManager) ListRoomsWithDeletionPriority(ctx context.Context, activeS
 				return nil, fmt.Errorf("failed to fetch room information: %w", err)
 			}
 
-			room = &game_room.GameRoom{ID: roomID, SchedulerID: activeScheduler.Name, Status: game_room.GameStatusError}
+			room = &game_room.GameRoom{ID: roomID, SchedulerID: activeScheduler.Name, Status: game_room.GameStatusError, Version: activeScheduler.Spec.Version}
 		}
 
 		// Select Terminating rooms to be re-deleted. This is useful for fixing any desync state.
@@ -257,7 +257,7 @@ func (m *RoomManager) ListRoomsWithDeletionPriority(ctx context.Context, activeS
 		}
 
 		isRoomActive := room.Status == game_room.GameStatusOccupied || room.Status == game_room.GameStatusReady || room.Status == game_room.GameStatusPending
-		if isRoomActive && room.Version == activeScheduler.Spec.Version {
+		if isRoomActive && activeScheduler.IsSameMajorVersion(room.Version) {
 			activeVersionRoomPool = append(activeVersionRoomPool, room)
 		} else {
 			toDeleteRooms = append(toDeleteRooms, room)

internal/core/services/rooms/room_manager_test.go

@@ -580,24 +580,29 @@ func TestRoomManager_ListRoomsWithDeletionPriority(t *testing.T) {
 		ctx := context.Background()
 		scheduler := newValidScheduler()
 		availableRooms := []*game_room.GameRoom{
-			{ID: "first-room", SchedulerID: scheduler.Name, Status: game_room.GameStatusReady},
+			{ID: "first-room", SchedulerID: scheduler.Name, Status: game_room.GameStatusReady, Version: scheduler.Spec.Version},
 		}
 
 		notFoundRoomID := "second-room"
 
-		roomStorage.EXPECT().GetRoomIDsByStatus(ctx, scheduler.Name, gomock.Any()).Return([]string{availableRooms[0].ID}, nil).AnyTimes()
+		roomStorage.EXPECT().GetRoomIDsByStatus(ctx, scheduler.Name, game_room.GameStatusError).Return([]string{}, nil).Times(1)
+		roomStorage.EXPECT().GetRoomIDsByStatus(ctx, scheduler.Name, game_room.GameStatusPending).Return([]string{}, nil).Times(1)
+		roomStorage.EXPECT().GetRoomIDsByStatus(ctx, scheduler.Name, game_room.GameStatusReady).Return([]string{availableRooms[0].ID}, nil).Times(1)
+		roomStorage.EXPECT().GetRoomIDsByStatus(ctx, scheduler.Name, game_room.GameStatusOccupied).Return([]string{}, nil).Times(1)
 		roomStorage.EXPECT().GetRoomIDsByLastPing(ctx, scheduler.Name, gomock.Any()).Return([]string{notFoundRoomID}, nil)
-
-		roomStorage.EXPECT().GetRoom(ctx, scheduler.Name, availableRooms[0].ID).Return(availableRooms[0], nil)
-
 		getRoomErr := porterrors.NewErrNotFound("failed to get")
 		roomStorage.EXPECT().GetRoom(ctx, scheduler.Name, notFoundRoomID).Return(nil, getRoomErr)
+		roomStorage.EXPECT().GetRoom(ctx, scheduler.Name, availableRooms[0].ID).Return(availableRooms[0], nil)
 
 		rooms, err := roomManager.ListRoomsWithDeletionPriority(ctx, scheduler, 2)
 		require.NoError(t, err)
 
-		availableRooms = append(availableRooms, &game_room.GameRoom{ID: notFoundRoomID, SchedulerID: scheduler.Name, Status: game_room.GameStatusError})
-		require.Equal(t, rooms, availableRooms)
+		expectedRooms := []*game_room.GameRoom{
+			{ID: notFoundRoomID, SchedulerID: scheduler.Name, Status: game_room.GameStatusError, Version: scheduler.Spec.Version},
+			{ID: "first-room", SchedulerID: scheduler.Name, Status: game_room.GameStatusReady, Version: scheduler.Spec.Version},
+		}
+
+		require.Equal(t, rooms, expectedRooms)
 	})
 
 	t.Run("when error happens while fetch a room it returns error", func(t *testing.T) {