Merge pull request #17798 from fmarcac/fix/syncplay-ping-delay-units

Fix unit mismatch in the SyncPlay resume delay floor

3 files changed, 158 insertions, 3 deletions

diff --git a/Emby.Server.Implementations/SyncPlay/Group.cs b/Emby.Server.Implementations/SyncPlay/Group.cs
index 38a0018a70..923bfc67aa 100644
--- a/Emby.Server.Implementations/SyncPlay/Group.cs
+++ b/Emby.Server.Implementations/SyncPlay/Group.cs
@@ -91,6 +91,18 @@ namespace Emby.Server.Implementations.SyncPlay
         public long DefaultPing { get; } = 500;
 
         /// <summary>
+        /// Gets the maximum ping, in milliseconds, accepted from a session.
+        /// </summary>
+        /// <remarks>
+        /// Pings are reported by clients and are scaled into the delays used to schedule playback,
+        /// so an unbounded value lets a single session push the whole group's resume point
+        /// arbitrarily far out, or overflow the arithmetic entirely. Anything above this is not a
+        /// usable measurement for synchronisation.
+        /// </remarks>
+        /// <value>The maximum ping.</value>
+        public long MaxPing { get; } = 10000;
+
+        /// <summary>
         /// Gets the maximum time offset error accepted for dates reported by clients, in milliseconds.
         /// </summary>
         /// <value>The maximum time offset error.</value>
@@ -438,7 +450,7 @@ namespace Emby.Server.Implementations.SyncPlay
         {
             if (_participants.TryGetValue(session.Id, out GroupMember value))
             {
-                value.Ping = ping;
+                value.Ping = Math.Clamp(ping, 0, MaxPing);
             }
         }
 
@@ -451,7 +463,9 @@ namespace Emby.Server.Implementations.SyncPlay
                 max = Math.Max(max, session.Ping);
             }
 
-            return max;
+            // A group with no participants has no ping to report. Returning long.MinValue would
+            // overflow the callers that scale this value into ticks, so fall back to the default.
+            return max == long.MinValue ? DefaultPing : max;
         }
 
         /// <inheritdoc />
diff --git a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
index eb38eeb503..f4fab29800 100644
--- a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
+++ b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
@@ -501,7 +501,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
                     {
                         // Client, that was buffering, resumed playback but did not update others in time.
                         delayTicks = context.GetHighestPing() * 2 * TimeSpan.TicksPerMillisecond;
-                        delayTicks = Math.Max(delayTicks, context.DefaultPing);
+                        delayTicks = Math.Max(delayTicks, TimeSpan.FromMilliseconds(context.DefaultPing).Ticks);
 
                         context.LastActivity = currentTime.AddTicks(delayTicks);
 
diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
new file mode 100644
index 0000000000..0cccd5d4ca
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
@@ -0,0 +1,141 @@
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Jellyfin.Database.Implementations.Entities;
+using MediaBrowser.Controller.Entities;
+using MediaBrowser.Controller.Library;
+using MediaBrowser.Controller.Session;
+using MediaBrowser.Controller.SyncPlay.GroupStates;
+using MediaBrowser.Controller.SyncPlay.PlaybackRequests;
+using MediaBrowser.Controller.SyncPlay.Requests;
+using MediaBrowser.Model.SyncPlay;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+using SyncPlayGroup = Emby.Server.Implementations.SyncPlay.Group;
+
+namespace Jellyfin.Server.Implementations.Tests.SyncPlay;
+
+public class WaitingGroupStateTests
+{
+    [Fact]
+    public void Ready_ClientResumedWithLowPing_AppliesTheDefaultPingFloorInMilliseconds()
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        // Both members report a ping well under the default, so the floor is what decides the delay.
+        group.UpdatePing(harness.First, 10);
+        group.UpdatePing(harness.Second, 10);
+
+        group.PositionTicks = TimeSpan.FromMinutes(5).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+        group.SetBuffering(harness.First, true);
+        group.SetBuffering(harness.Second, false);
+
+        var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true };
+
+        var before = DateTime.UtcNow;
+        state.HandleRequest(
+            new ReadyGroupRequest(DateTime.UtcNow, group.PositionTicks, true, harness.PlaylistItemId),
+            group,
+            GroupStateType.Waiting,
+            harness.First,
+            CancellationToken.None);
+
+        // DefaultPing is expressed in milliseconds, so the floor must be converted before being
+        // compared against a tick count. Without the conversion the floor is 500 ticks (0.05 ms)
+        // and never applies.
+        var scheduledDelay = group.LastActivity - before;
+        Assert.True(
+            scheduledDelay >= TimeSpan.FromMilliseconds(group.DefaultPing),
+            $"expected a resume delay of at least {group.DefaultPing} ms, got {scheduledDelay.TotalMilliseconds} ms");
+    }
+
+    [Theory]
+    [InlineData(4_000_000_000L)]
+    [InlineData(1_000_000_000_000_000L)]
+    [InlineData(long.MaxValue)]
+    [InlineData(-1L)]
+    public void UpdatePing_ClientReportsAnUnusablePing_IsClampedAndCannotStallTheGroup(long reportedPing)
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        group.UpdatePing(harness.First, reportedPing);
+
+        Assert.InRange(group.GetHighestPing(), 0, group.MaxPing);
+
+        // The reported ping is scaled into the group's resume point, so an unclamped value either
+        // pushes playback months out or overflows the arithmetic outright.
+        var state = new PlayingGroupState(NullLoggerFactory.Instance);
+        var before = DateTime.UtcNow;
+        state.HandleRequest(
+            new UnpauseGroupRequest(),
+            group,
+            GroupStateType.Paused,
+            harness.First,
+            CancellationToken.None);
+
+        Assert.InRange(group.LastActivity - before, TimeSpan.Zero, TimeSpan.FromMinutes(1));
+    }
+
+    private sealed class GroupHarness
+    {
+        public GroupHarness()
+        {
+            var userManager = new Mock<IUserManager>();
+            var sessionManager = new Mock<ISessionManager>();
+            var libraryManager = new Mock<ILibraryManager>();
+
+            var user = new User("tester", "auth-provider", "pwdreset-provider");
+            userManager.Setup(m => m.GetUserById(It.IsAny<Guid>())).Returns(user);
+
+            var item = new Mock<BaseItem>();
+            item.Setup(i => i.IsVisibleStandalone(It.IsAny<User>())).Returns(true);
+            item.Object.RunTimeTicks = TimeSpan.FromHours(2).Ticks;
+            libraryManager.Setup(m => m.GetItemById(It.IsAny<Guid>())).Returns(item.Object);
+
+            sessionManager
+                .Setup(m => m.SendSyncPlayCommand(It.IsAny<string>(), It.IsAny<SendCommand>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.CompletedTask);
+
+            sessionManager
+                .Setup(m => m.SendSyncPlayGroupUpdate(It.IsAny<string>(), It.IsAny<GroupUpdate<GroupStateUpdate>>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.CompletedTask);
+
+            Group = new SyncPlayGroup(
+                NullLoggerFactory.Instance,
+                userManager.Object,
+                sessionManager.Object,
+                libraryManager.Object);
+
+            First = new SessionInfo(sessionManager.Object, NullLogger.Instance)
+            {
+                Id = "first",
+                UserId = user.Id,
+                UserName = "first"
+            };
+            Second = new SessionInfo(sessionManager.Object, NullLogger.Instance)
+            {
+                Id = "second",
+                UserId = user.Id,
+                UserName = "second"
+            };
+
+            Group.CreateGroup(First, new NewGroupRequest("group"), CancellationToken.None);
+            Group.SessionJoin(Second, new JoinGroupRequest(Group.GroupId), CancellationToken.None);
+            Group.SetPlayQueue(new List<Guid> { Guid.NewGuid() }, 0, 0);
+            PlaylistItemId = Group.PlayQueue.GetPlayingItemPlaylistId();
+        }
+
+        public SyncPlayGroup Group { get; }
+
+        public SessionInfo First { get; }
+
+        public SessionInfo Second { get; }
+
+        public Guid PlaylistItemId { get; }
+    }
+}