Fix ConcurrentModificationException in netplay delta sync (Tracker delayed props)

forge-game/src/main/java/forge/trackable/Tracker.java

@@ -26,9 +26,12 @@
  * engine effect into a single coherent post-effect snapshot. {@link #flush()} drains the
  * queue without leaving the frozen state.
  *
- * <p><b>Thread safety.</b> Not thread-safe — game thread only. The {@code unfreeze}
- * replay walks TrackableObjects and triggers consumer notifications; running it from
- * another thread corrupts consumer dirty-bit state.
+ * <p><b>Thread safety.</b> Owned by the game thread — freezing, unfreezing and object
+ * lookups must stay there. The {@code unfreeze} replay walks TrackableObjects and
+ * triggers consumer notifications; running it from another thread corrupts consumer
+ * dirty-bit state. The delayed-prop queue itself is synchronized internally, because
+ * the netplay delta sync reads it through {@link #getDelayedPropsFor} from outside the
+ * game thread (see issue #11535).
  */
 public class Tracker {
     private int freezeCounter = 0;
@@ -60,14 +63,21 @@ public <T> void putObj(TrackableType<T> type, Integer id, T val) {
     }
 
     public void unfreeze() {
-        if (!isFrozen() || --freezeCounter > 0 || delayedPropChanges.isEmpty()) {
+        if (!isFrozen() || --freezeCounter > 0) {
             return;
         }
+        final List<DelayedPropChange> toApply;
+        synchronized (delayedPropChanges) {
+            if (delayedPropChanges.isEmpty()) {
+                return;
+            }
+            toApply = Lists.newArrayList(delayedPropChanges);
+            delayedPropChanges.clear();
+        }
         //after being unfrozen, ensure all changes delayed during freeze are now applied
-        for (final DelayedPropChange change : delayedPropChanges) {
+        for (final DelayedPropChange change : toApply) {
             change.object.set(change.prop, change.value);
         }
-        delayedPropChanges.clear();
     }
 
     public void flush() {
@@ -80,27 +90,34 @@ public void flush() {
     }
 
     public void addDelayedPropChange(final TrackableObject object, final TrackableProperty prop, final Object value) {
-        delayedPropChanges.add(new DelayedPropChange(object, prop, value));
+        synchronized (delayedPropChanges) {
+            delayedPropChanges.add(new DelayedPropChange(object, prop, value));
+        }
     }
 
     public void clearDelayed() {
-        delayedPropChanges.clear();
+        synchronized (delayedPropChanges) {
+            delayedPropChanges.clear();
+        }
     }
 
     /**
      * Read-only peek at delayed property changes queued for a specific object.
+     * Safe to call from outside the game thread (see issue #11535).
      */
     public Map<TrackableProperty, Object> getDelayedPropsFor(TrackableObject obj) {
-        if (delayedPropChanges.isEmpty()) {
-            return Collections.emptyMap();
-        }
-        Map<TrackableProperty, Object> result = new EnumMap<>(TrackableProperty.class);
-        for (DelayedPropChange change : delayedPropChanges) {
-            if (change.object == obj) {
-                result.put(change.prop, change.value);
+        synchronized (delayedPropChanges) {
+            if (delayedPropChanges.isEmpty()) {
+                return Collections.emptyMap();
+            }
+            Map<TrackableProperty, Object> result = new EnumMap<>(TrackableProperty.class);
+            for (DelayedPropChange change : delayedPropChanges) {
+                if (change.object == obj) {
+                    result.put(change.prop, change.value);
+                }
             }
+            return result;
         }
-        return result;
     }
 
     private class DelayedPropChange {

forge-game/src/test/java/forge/trackable/TrackerConcurrencyTest.java

@@ -0,0 +1,81 @@
+package forge.trackable;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.testng.AssertJUnit;
+import org.testng.annotations.Test;
+
+/**
+ * Regression test for https://github.com/Card-Forge/forge/issues/11535.
+ *
+ * <p>The netplay delta sync reads a tracker's delayed prop changes via
+ * {@link Tracker#getDelayedPropsFor} from outside the game thread while the
+ * game thread mutates the queue through {@link Tracker#addDelayedPropChange}
+ * and {@link Tracker#clearDelayed}. Before the queue accessors were
+ * synchronized, this threw ConcurrentModificationException and killed the
+ * host's Event Dispatch Thread mid-game.
+ */
+public class TrackerConcurrencyTest {
+
+    private static final class DummyObject extends TrackableObject {
+        DummyObject(final int id, final Tracker tracker) {
+            super(id, tracker);
+        }
+    }
+
+    @Test
+    public void testGetDelayedPropsForSafeDuringConcurrentMutation() throws InterruptedException {
+        final Tracker tracker = new Tracker();
+        final TrackableObject obj = new DummyObject(1, tracker);
+        final AtomicReference<Throwable> failure = new AtomicReference<>();
+        final AtomicBoolean writerDone = new AtomicBoolean(false);
+        final CountDownLatch start = new CountDownLatch(2);
+
+        // The crash happened inside a freeze bracket — delta sync only reads
+        // delayed props while the tracker is frozen.
+        tracker.freeze();
+
+        final Thread writer = new Thread(() -> {
+            try {
+                start.countDown();
+                start.await();
+                final long deadline = System.nanoTime() + 1_500_000_000L;
+                long i = 0;
+                while (System.nanoTime() < deadline) {
+                    tracker.addDelayedPropChange(obj, TrackableProperty.Life, (int) (i++ % 40));
+                    if (i % 25 == 0) {
+                        tracker.clearDelayed();
+                    }
+                }
+            } catch (final Throwable t) {
+                failure.compareAndSet(null, t);
+            } finally {
+                writerDone.set(true);
+            }
+        }, "game-thread");
+
+        final Thread reader = new Thread(() -> {
+            try {
+                start.countDown();
+                start.await();
+                while (!writerDone.get()) {
+                    tracker.getDelayedPropsFor(obj);
+                }
+            } catch (final Throwable t) {
+                failure.compareAndSet(null, t);
+            }
+        }, "sync-thread");
+
+        writer.start();
+        reader.start();
+        writer.join(10_000);
+        reader.join(10_000);
+
+        final Throwable thrown = failure.get();
+        if (thrown != null) {
+            AssertJUnit.fail("Concurrent delayed-prop access failed: " + thrown);
+        }
+    }
+}

forge-gui/src/main/java/forge/gamemodes/net/server/DeltaSyncManager.java

@@ -101,9 +101,12 @@ public int getConsumerId() {
      * New objects are registered with this consumer and sent in full.
      * Existing objects only send properties dirty for THIS consumer.
      *
-     * <p>Must be called on the game thread. All delta collection and checksum
-     * computation runs single-threaded — no locks, snapshots, or volatile
-     * barriers needed.
+     * <p>Intended to run on the game thread, but in practice also entered from
+     * the EDT (reveal/rollback dialogs via RemoteClientGuiGame.syncAndSend) and
+     * from netty threads (reconnect handshake). Reads of the tracker's
+     * delayed-prop queue are therefore synchronized (see issue #11535); the
+     * rest of the walk is unsynchronized and still assumes game state is not
+     * mutated mid-collection.
      */
     public DeltaPacket collectDeltas(GameView gameView) {
         Map<Integer, Map<TrackableProperty, Object>> objectDeltas = new HashMap<>();
@@ -302,8 +305,9 @@ private Map<TrackableProperty, Object> buildPropertyMap(TrackableObject obj, Set
      * to the props map or marked dirty while frozen, but network
      * clients need them in the same delta as their accompanying events.
      *
-     * This is safe because speculative freeze brackets (which call clearDelayed()) and real freeze brackets are disjoint
-     * — speculative brackets always start from freezeCounter == 0 and complete before any sync point where delta collection occurs.
+     * getDelayedPropsFor takes a synchronized snapshot: delta collection can run on
+     * the EDT concurrently with game-thread freeze brackets mutating the queue
+     * (issue #11535 — this used to throw ConcurrentModificationException here).
      */
     private void mergeDelayedProps(TrackableObject obj, Map<TrackableProperty, Object> delta, Set<TrackableProperty> dirtyProps) {
         Tracker tracker = obj.getTracker();