fix: emote play-timeout watchdog never fires, stranding emote intents

CharacterEmoteIntent.UpdatePlayTimeout had an operator-precedence bug
(`playTimeout?.ElapsedTime ?? 0 + dt` parses as `?? (0 + dt)`) that froze
ElapsedTime at the first frame's dt, so the #6531 60s unstuck timeout never
fired; the tick was also only reachable on one parked sub-path inside
CharacterEmoteSystem.ConsumeEmoteIntent. A scene emote that parks (breaks
mid-cinematic) and is then evicted from the memory-pressure cache sweep
(frequent on Mac) leaves a permanent CharacterEmoteIntent that silently
blocks every user emote until client restart.

Fixes #9485
Related: #6531

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: eordano

Explorer/Assets/DCL/AvatarRendering/Emotes/Components/CharacterEmoteIntent.cs

@@ -31,7 +31,7 @@ public void UpdateRemoteId(URN emoteId)
         public bool UpdatePlayTimeout(float dt)
         {
             // Timeout access returns a temporary value. We need to reassign the field or we lose the changes
-            playTimeout = new LoadTimeout(playTimeout?.Timeout ?? StreamableLoadingDefaults.TIMEOUT, playTimeout?.ElapsedTime ?? 0 + dt);
+            playTimeout = new LoadTimeout(playTimeout?.Timeout ?? StreamableLoadingDefaults.TIMEOUT, (playTimeout?.ElapsedTime ?? 0) + dt);
             bool result = playTimeout.Value.IsTimeout;
             return result;
         }

Explorer/Assets/DCL/AvatarRendering/Emotes/Systems/Play/CharacterEmoteSystem.cs

@@ -280,6 +280,17 @@ private void ConsumeEmoteIntent([Data] float dt, Entity entity,
             // it's very important to catch any exception here to avoid not consuming the emote intent, so we don't infinitely create props
             try
             {
+                // Fixes https://github.com/decentraland/unity-explorer/issues/6531
+                // Rarely happens for an unknown reason that emote.AssetResults[bodyShape] is null, provoking the emote intent to never finish,
+                // thus props of the previous emote cannot be disposed either.
+                // By setting a timeout we force unstuck the process
+                if (emoteIntent.UpdatePlayTimeout(dt))
+                {
+                    ReportHub.LogError(GetReportData(), $"Cant play emote {emoteId} timeout reached.");
+                    World.Remove<CharacterEmoteIntent>(entity);
+                    return;
+                }
+
                 // we wait until the avatar finishes moving to trigger the emote,
                 // avoid the case where: you stop moving, trigger the emote, the emote gets triggered and next frame it gets cancelled because inertia keeps moving the avatar
                 // We also avoid triggering the emote while the character is jumping or landing, as the landing animation breaks the emote flow if they have props
@@ -312,17 +323,6 @@ private void ConsumeEmoteIntent([Data] float dt, Entity entity,
                         return;
                     }
 
-                    // Fixes https://github.com/decentraland/unity-explorer/issues/6531
-                    // Rarely happens for an unknown reason that emote.AssetResults[bodyShape] is null, provoking the emote intent to never finish,
-                    // thus props of the previous emote cannot be disposed either.
-                    // By setting a timeout we force unstuck the process
-                    if (emoteIntent.UpdatePlayTimeout(dt))
-                    {
-                        ReportHub.LogError(GetReportData(), $"Cant play emote {emoteId} timeout reached.");
-                        World.Remove<CharacterEmoteIntent>(entity);
-                        return;
-                    }
-
                     BodyShape bodyShape = avatarShapeComponent.BodyShape;
                     StreamableLoadingResult<AttachmentRegularAsset>? assetResult = emote.AssetResults[bodyShape];
 

Explorer/Assets/DCL/AvatarRendering/Emotes/Tests/CharacterEmoteIntentShould.cs

@@ -0,0 +1,43 @@
+using ECS.StreamableLoading;
+using NUnit.Framework;
+
+namespace DCL.AvatarRendering.Emotes.Tests
+{
+    public class CharacterEmoteIntentShould
+    {
+        [Test]
+        public void UpdatePlayTimeout_AccumulateElapsedTimeAcrossCalls_ReturnTrueOnceTotalReachesTimeout()
+        {
+            var intent = new CharacterEmoteIntent();
+
+            // One call per simulated second, mirroring how ConsumeEmoteIntent ticks this every frame with that frame's dt.
+            var timedOutBeforeLastSecond = false;
+
+            for (var second = 0; second < StreamableLoadingDefaults.TIMEOUT - 1; second++)
+                timedOutBeforeLastSecond |= intent.UpdatePlayTimeout(1f);
+
+            Assert.IsFalse(timedOutBeforeLastSecond,
+                "Must not report a timeout before StreamableLoadingDefaults.TIMEOUT seconds of elapsed time have accumulated.");
+
+            var timedOutAtTimeoutSecond = intent.UpdatePlayTimeout(1f);
+
+            Assert.IsTrue(timedOutAtTimeoutSecond,
+                "Elapsed time must accumulate across calls (each call's dt added to the running total) so IsTimeout fires " +
+                "once the total reaches StreamableLoadingDefaults.TIMEOUT seconds. This is the #6531 unstuck watchdog for a " +
+                "stranded CharacterEmoteIntent (see emote-lock-after-fish-catch). Fails at the pin because " +
+                "`playTimeout?.ElapsedTime ?? 0 + dt` parses as `?? (0 + dt)`: once playTimeout is non-null the right side " +
+                "of `??` is never evaluated, so ElapsedTime is reassigned to itself and freezes at the first call's dt " +
+                "forever, and IsTimeout never fires.");
+        }
+
+        [Test]
+        public void UpdatePlayTimeout_ReturnFalse_OnFirstCallBeforeTimeoutElapsed()
+        {
+            var intent = new CharacterEmoteIntent();
+
+            var result = intent.UpdatePlayTimeout(1f);
+
+            Assert.IsFalse(result);
+        }
+    }
+}

Explorer/Assets/DCL/AvatarRendering/Emotes/Tests/CharacterEmoteIntentShould.cs.meta

@@ -1,16 +1,24 @@
 using Arch.Core;
+using CommunicationData.URLHelpers;
+using DCL.AvatarRendering.AvatarShape.Components;
 using DCL.AvatarRendering.AvatarShape.UnityInterface;
 using DCL.AvatarRendering.Emotes.Play;
+using DCL.AvatarRendering.Loading.Assets;
+using DCL.AvatarRendering.Loading.Components;
 using DCL.Character.Components;
 using DCL.DebugUtilities;
 using DCL.Diagnostics;
+using DCL.ECSComponents;
 using DCL.Multiplayer.Emotes;
 using ECS.SceneLifeCycle;
+using ECS.StreamableLoading;
+using ECS.StreamableLoading.Common.Components;
 using ECS.TestSuite;
 using NSubstitute;
 using NUnit.Framework;
 using SceneRunner.Scene;
 using UnityEngine;
+using Utility.Animations;
 using Object = UnityEngine.Object;
 
 namespace DCL.AvatarRendering.Emotes.Tests
@@ -22,6 +30,7 @@ public class CharacterEmoteSystemShould : UnitySystemTestBase<CharacterEmoteSyst
 
         private ScenesCache scenesCache = null!;
         private IEmotesMessageBus messageBus = null!;
+        private IEmoteStorage emoteStorage = null!;
         private IAvatarView avatarView = null!;
         private GameObject poolRoot = null!;
         private GameObject audioSourcePrefab = null!;
@@ -38,8 +47,9 @@ public void Setup()
 
             scenesCache = new ScenesCache();
             messageBus = Substitute.For<IEmotesMessageBus>();
+            emoteStorage = Substitute.For<IEmoteStorage>();
 
-            system = new CharacterEmoteSystem(world, Substitute.For<IEmoteStorage>(), messageBus, emotePlayer,
+            system = new CharacterEmoteSystem(world, emoteStorage, messageBus, emotePlayer,
                 Substitute.For<IDebugContainerBuilder>(), localSceneDevelopment: false, scenesCache);
 
             avatarView = Substitute.For<IAvatarView>();
@@ -91,6 +101,70 @@ public void StopSceneEmoteWhenItsSceneIsNoLongerLoaded()
             messageBus.Received().SendStop();
         }
 
+        /// <summary>
+        /// Regression for the "emote-lock-after-fish-catch" bug: a scene emote whose asset never resolves
+        /// (e.g. evicted from the memory-pressure cache sweep before it ever played, as happens with the
+        /// Genesis Plaza fishing catch/reveal emotes when the cinematic breaks) leaves a stranded
+        /// CharacterEmoteIntent on the player entity. UpdateEmoteInputSystem.TriggerEmote is
+        /// [None(typeof(CharacterEmoteIntent))], so a stranded intent silently blocks every user emote
+        /// (slot shortcuts and the wheel) until the #6531 play-timeout watchdog removes it.
+        /// </summary>
+        [Test]
+        public void RemoveStrandedCharacterEmoteIntentAfterPlayTimeoutElapses()
+        {
+            // The fixed watchdog path legitimately emits "[Error] Cant play emote ... timeout reached."
+            // (pre-existing ReportHub.LogError relocated by potential-fix.patch); without this the
+            // Unity Test Framework fails the test on the unhandled error log. Set inside the body:
+            // the framework resets LogAssert state after [SetUp], before the test body runs.
+            UnityEngine.TestTools.LogAssert.ignoreFailingMessages = true;
+
+            // Arrange: a full-body scene emote resolved in the storage but whose per-body-shape asset
+            // never arrived (AssetResults entry stays null), so ConsumeEmoteIntent keeps parking on the
+            // "Loading not complete" branch every frame instead of playing or failing outright.
+            IAvatarView strandedAvatarView = Substitute.For<IAvatarView>();
+
+            // Grounded and not jumping/moving: bypasses the animator park gate so the query reaches the
+            // emote-storage branch below instead of parking earlier for an unrelated reason.
+            strandedAvatarView.GetAnimatorBool(AnimationHashes.GROUNDED).Returns(true);
+
+            IEmote emote = Substitute.For<IEmote>();
+            emote.IsLoading.Returns(false);
+
+            // Empty results: the asset is not resident (e.g. evicted with a zero refcount before it ever
+            // played), so ConsumeEmoteIntent takes the "assetResult == null" return every frame.
+            emote.AssetResults.Returns(new StreamableLoadingResult<AttachmentRegularAsset>?[BodyShape.COUNT]);
+
+            emoteStorage.TryGetElement(Arg.Any<URN>(), out Arg.Any<IEmote>())
+                        .Returns(call =>
+                         {
+                             call[1] = emote;
+                             return true;
+                         });
+
+            Entity strandedEntity = world.Create(
+                new CharacterEmoteComponent(),
+                new CharacterEmoteIntent
+                {
+                    EmoteId = new URN("urn:decentraland:off-chain:scene-emote:pond-fishing_catch-false"),
+                    Mask = AvatarEmoteMask.AemFullBody,
+                },
+                strandedAvatarView,
+                new AvatarShapeComponent { BodyShape = BodyShape.MALE });
+
+            // Act: simulate more than StreamableLoadingDefaults.TIMEOUT seconds of frames, one second of
+            // dt at a time. A single call with a large dt would not reproduce the pin's bug (the very
+            // first call is unaffected by the precedence bug — see CharacterEmoteIntentShould), so the
+            // repeated small-dt calls are load-bearing for the regression.
+            for (var second = 0; second < StreamableLoadingDefaults.TIMEOUT + 1; second++)
+                system!.Update(1f);
+
+            // Assert
+            Assert.IsFalse(world.Has<CharacterEmoteIntent>(strandedEntity),
+                "A CharacterEmoteIntent whose asset never resolves must expire after StreamableLoadingDefaults.TIMEOUT " +
+                "seconds via the #6531 watchdog, instead of permanently blocking UpdateEmoteInputSystem.TriggerEmote's " +
+                "[None(CharacterEmoteIntent)] gate (the restart-only emote lock from emote-lock-after-fish-catch).");
+        }
+
         private static ISceneFacade NewSceneFacadeWithName(string name)
         {
             ISceneData sceneData = Substitute.For<ISceneData>();