Merge pull request #297 from lichess-org/bestmove-as-uci

Refactor BestMove from String to Uci

app/src/main/scala/Executor.scala

@@ -69,15 +69,12 @@ object Executor:
           case GetTaskResult.NotFound              => state -> logNotFound(workId, key)
           case GetTaskResult.AcquiredByOther(task) => state -> logNotAcquired(task, key)
           case GetTaskResult.Found(task) =>
-            response.uci match
-              case Some(uci) =>
-                state.remove(task.id) -> (monitor.success(task) >>
-                  client.send(Lila.Response(task.request.id, task.request.moves, uci)))
-              case _ =>
-                val (newState, maybeGivenUp) = state.unassignOrGiveUp(task)
-                val logs = maybeGivenUp.traverse_(t => warn"Give up an invalid move $response by $key for $t")
+            state.remove(task.id) -> client
+              .send(Lila.Response(task.request.id, task.request.moves, response.value))
+              .handleErrorWith: e =>
+                error"Failed to send move ${task.id} for ${task.request.id} by $key: $e"
                   *> failure(task, key)
-                newState -> logs
+            *> monitor.success(task)
 
     private def invalidate(workId: WorkId, key: ClientKey): IO[Unit] =
       ref.flatModify: state =>

app/src/main/scala/model.scala

@@ -15,14 +15,12 @@ object ClientKey:
   given Decoder[ClientKey]                    = decodeString
   extension (ck: ClientKey) def value: String = ck
 
-opaque type BestMove = String
+opaque type BestMove = Uci
 object BestMove:
-  def apply(value: String): BestMove = value
-  given Encoder[BestMove]            = encodeString
-  given Decoder[BestMove]            = decodeString
-  extension (bm: BestMove)
-    def value: String    = bm
-    def uci: Option[Uci] = Uci(bm)
+  def apply(value: Uci): BestMove         = value
+  given Encoder[BestMove]                 = encodeString.contramap(_.uci)
+  given Decoder[BestMove]                 = decodeString.emap(Uci.apply(_).toRight("Invalid Uci"))
+  extension (bm: BestMove) def value: Uci = bm
 
 opaque type WorkId = String
 object WorkId:

app/src/test/scala/ExecutorTest.scala

@@ -2,6 +2,7 @@ package lila.fishnet
 
 import cats.effect.{ IO, Ref }
 import cats.syntax.all.*
+import chess.format.Uci
 import org.typelevel.log4cats.Logger
 import org.typelevel.log4cats.noop.NoOpLogger
 import weaver.*
@@ -26,8 +27,7 @@ object ExecutorTest extends SimpleIOSuite:
   val key  = ClientKey("key")
   val key2 = ClientKey("key2")
 
-  val validMove   = BestMove("e2e4")
-  val invalidMove = BestMove("2e4")
+  val validMove = BestMove(Uci("e2e4").get)
 
   test("acquire when there is no work should return none"):
     for
@@ -93,27 +93,6 @@ object ExecutorTest extends SimpleIOSuite:
       response <- ref.get.map(_.head)
     yield expect.same(response, Lila.Response(request.id, request.moves, chess.format.Uci.Move("e2e4").get))
 
-  test("post an invalid move should not send move"):
-    for
-      ref      <- emptyMovesRef
-      executor <- createExecutor(ref)(ExecutorConfig(2))
-      _        <- executor.add(request)
-      acquired <- executor.acquire(key)
-      _        <- executor.move(acquired.get.id, key, invalidMove.some)
-      moves    <- ref.get
-    yield assert(moves.isEmpty)
-
-  test("after post an invalid move, acquire again should return work.some"):
-    for
-      executor <- createExecutor(ExecutorConfig(2))
-      _        <- executor.add(request)
-      acquired <- executor.acquire(key)
-      workId = acquired.get.id
-      _              <- executor.move(workId, key, invalidMove.some)
-      acquiredOption <- executor.acquire(key)
-      acquired = acquiredOption.get
-    yield expect.same(acquired.request, request)
-
   test("post null move should remove the task"):
     for
       ref      <- emptyMovesRef
@@ -129,38 +108,27 @@ object ExecutorTest extends SimpleIOSuite:
     for
       executor <- createExecutor(ExecutorConfig(2))
       _        <- executor.add(request)
-      _ <- (executor.acquire(key).flatMap(x => executor.move(x.get.id, key, invalidMove.some))).replicateA_(2)
+      _ <- (executor.acquire(key).flatMap(x => executor.clean(Instant.now.plusSeconds(17)))).replicateA_(2)
       acquired <- executor.acquire(key)
     yield assert(acquired.isDefined)
 
   test("should give up after 3 tries"):
     for
       executor <- createExecutor(ExecutorConfig(2))
       _        <- executor.add(request)
-      _ <- (executor.acquire(key).flatMap(x => executor.move(x.get.id, key, invalidMove.some))).replicateA_(3)
+      _ <- (executor.acquire(key).flatMap(x => executor.clean(Instant.now.plusSeconds(17)))).replicateA_(3)
       acquired <- executor.acquire(key)
     yield assert(acquired.isEmpty)
 
-  test("give up due to invalid move should reduce task's size"):
-    for
-      executor <- createExecutor(ExecutorConfig(2))
-      _        <- executor.add(request)
-      _        <- executor.add(request.copy(id = GameId("2")))
-      _ <- (executor.acquire(key).flatMap(x => executor.move(x.get.id, key, invalidMove.some))).replicateA_(3)
-      _ <- executor.add(request.copy(id = GameId("3")))
-      a1 <- executor.acquire(key)
-      a2 <- executor.acquire(key2)
-    yield assert(a1.isDefined && a2.isDefined)
-
   test("give up due to cleaning should reduce task's size"):
     for
       executor <- createExecutor(ExecutorConfig(2))
       _        <- executor.add(request)
       _        <- executor.add(request.copy(id = GameId("2")))
-      _ <- (executor.acquire(key).flatMap(x => executor.move(x.get.id, key, invalidMove.some))).replicateA_(2)
-      _ <- executor.acquire(key)
-      _ <- executor.clean(Instant.now.plusSeconds(37))
-      _ <- executor.add(request.copy(id = GameId("3")))
+      _  <- (executor.acquire(key).flatMap(x => executor.clean(Instant.now.plusSeconds(17)))).replicateA_(2)
+      _  <- executor.acquire(key)
+      _  <- executor.clean(Instant.now.plusSeconds(37))
+      _  <- executor.add(request.copy(id = GameId("3")))
       a1 <- executor.acquire(key)
       a2 <- executor.acquire(key2)
     yield assert(a1.isDefined && a2.isDefined)

app/src/test/scala/IntegrationTest.scala

@@ -2,6 +2,7 @@ package lila.fishnet
 
 import cats.effect.{ IO, Ref, Resource }
 import cats.syntax.all.*
+import chess.format.Uci
 import com.comcast.ip4s.*
 import com.dimafeng.testcontainers.GenericContainer
 import io.chrisdavenport.rediculous.RedisPubSub
@@ -53,7 +54,7 @@ object IntegrationTest extends IOSuite:
   test("let's play a game"): res =>
     val fishnet               = Fishnet("2.7.2", ClientKey("secret-key"))
     val fishnetAcquireRequest = Acquire(fishnet)
-    val bestMoves             = List("e7e6", "d7d5", "d8d6")
+    val bestMoves             = List("e7e6", "d7d5", "d8d6").traverse(Uci.apply).get
     val postMoves             = bestMoves.map(m => PostMove(fishnet, Move(BestMove(m).some)))
 
     val gameId = "CPzkP0tq"