core: keep forwarded messages in order when a batch is full (#8058)

* tests: order of forwarded messages when a batch is full

* core: keep forwarded messages in order when a batch is full

src/Simplex/Chat/Messages/Batch.hs

@@ -82,17 +82,18 @@ batchMessages mode maxLen = addBatch . foldr addToBatch ([], [], [], 0, 0)
 -- | Batches delivery tasks into (batch if any task was accepted, accepted, large).
 -- Always uses binary batch format for relay groups.
 batchDeliveryTasks1 :: VersionRangeChat -> Int -> NonEmpty MessageDeliveryTask -> (Maybe ByteString, [MessageDeliveryTask], [MessageDeliveryTask])
-batchDeliveryTasks1 _vr maxLen = toResult . foldl' addToBatch ([], [], [], 0, 0) . L.toList
+batchDeliveryTasks1 _vr maxLen = toResult . addToBatch ([], [], [], 0, 0) . L.toList
   where
-    addToBatch :: ([ByteString], [MessageDeliveryTask], [MessageDeliveryTask], Int, Int) -> MessageDeliveryTask -> ([ByteString], [MessageDeliveryTask], [MessageDeliveryTask], Int, Int)
-    addToBatch (msgBodies, accepted, large, len, n) task
+    addToBatch :: ([ByteString], [MessageDeliveryTask], [MessageDeliveryTask], Int, Int) -> [MessageDeliveryTask] -> ([ByteString], [MessageDeliveryTask], [MessageDeliveryTask], Int, Int)
+    addToBatch acc [] = acc
+    addToBatch acc@(msgBodies, accepted, large, len, n) (task : tasks)
       -- element can't fit even a singleton batch (4-byte binary-batch framing)
-      | msgLen + 4 > maxLen = (msgBodies, accepted, task : large, len, n)
+      | msgLen + 4 > maxLen = addToBatch (msgBodies, accepted, task : large, len, n) tasks
       -- fits: include in batch
       -- batch overhead: '=' + count (2) + 2-byte length prefix per element
-      | n + 1 <= maxBatchElementCount && len' + (n + 1) * 2 + 2 <= maxLen = (msgBody : msgBodies, task : accepted, large, len', n + 1)
+      | n + 1 <= maxBatchElementCount && len' + (n + 1) * 2 + 2 <= maxLen = addToBatch (msgBody : msgBodies, task : accepted, large, len', n + 1) tasks
       -- doesn't fit: stop adding further messages
-      | otherwise = (msgBodies, accepted, large, len, n)
+      | otherwise = acc
       where
         MessageDeliveryTask {fwdSender, brokerTs = fwdBrokerTs, verifiedMsg} = task
         msgBody = encodeFwdElement GrpMsgForward {fwdSender, fwdBrokerTs} verifiedMsg

tests/MessageBatching.hs

@@ -50,6 +50,7 @@ batchingTests = describe "message batching tests" $ do
   it "splits a batch that exceeds the element count limit" testBatchElementCountLimit
   it "does not create a relay delivery body when every task is oversized" testRelayBatchAllLarge
   it "classifies a task that fits raw but not as a framed singleton as large" testRelayBatchSingletonOverflow
+  it "stops a relay delivery batch at the first task that does not fit" testRelayBatchKeepsOrder
   it "shortens forwarded member names" testFwdMemberName
 
 instance IsString SndMessage where
@@ -186,12 +187,26 @@ deliveryTask taskId text =
 testRelayBatchSingletonOverflow :: IO ()
 testRelayBatchSingletonOverflow = do
   let task = deliveryTask 1 "overflow"
-      elemLen = B.length $ encodeFwdElement (GrpMsgForward (fwdSender task) (brokerTs task)) (verifiedMsg task)
+      elemLen = B.length $ fwdElement task
       (body_, accepted, large) = batchDeliveryTasks1 chatInitialVRange (elemLen + 2) (task :| [])
   body_ `shouldBe` Nothing
   map deliveryTaskId accepted `shouldBe` []
   map deliveryTaskId large `shouldBe` [1]
 
+testRelayBatchKeepsOrder :: IO ()
+testRelayBatchKeepsOrder = do
+  let task1 = deliveryTask 1 "one"
+      task2 = deliveryTask 2 "two is longer"
+      task3 = deliveryTask 3 "three"
+      maxLen = B.length $ encodeBinaryBatch [fwdElement task1, fwdElement task3]
+      (body_, accepted, large) = batchDeliveryTasks1 chatInitialVRange maxLen (task1 :| [task2, task3])
+  map deliveryTaskId accepted `shouldBe` [1]
+  map deliveryTaskId large `shouldBe` []
+  body_ `shouldBe` Just (encodeBinaryBatch [fwdElement task1])
+
+fwdElement :: MessageDeliveryTask -> ByteString
+fwdElement MessageDeliveryTask {fwdSender, brokerTs, verifiedMsg} = encodeFwdElement (GrpMsgForward fwdSender brokerTs) verifiedMsg
+
 testFwdMemberName :: IO ()
 testFwdMemberName = do
   fwdMemberName "sixteen_chars_ab" `shouldBe` "sixteen_chars_ab"