Fix encoder/game thread race; add ID fallback in event refs (#10561)

The encoder previously ran on the Netty I/O thread, reading TrackableObject
state concurrently with the game thread's set() calls. EnumMap.writeObject
would intermittently fail with AIOOBE, and because RemoteClient.write had
no failure listener, the dropped setGameView left the client tracker in an
inconsistent state. Move encoding onto the calling thread (which is also the
only mutator) and hand the resulting ByteBuf to the channel for async write,
preserving non-blocking sends.

- Drop EventPlayerRef and route PlayerView through IdRef in event mode.
  PlayerView is stable in the tracker for the lifetime of a game

- Convert IdRef and EventCardRef to records. Same semantics, less
  boilerplate, free toString() for net log output

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: MostCromulent

forge-gui/src/main/java/forge/gamemodes/net/CObjectOutputStream.java

@@ -24,6 +24,6 @@ protected void writeClassDescriptor(ObjectStreamClass desc) throws IOException {
 
     @Override
     protected Object replaceObject(Object obj) throws IOException {
-        return TrackableSerializer.replace(obj, null);
+        return TrackableSerializer.replace(obj, null, false);
     }
 }

forge-gui/src/main/java/forge/gamemodes/net/CompatibleObjectEncoder.java

@@ -5,6 +5,7 @@
 import forge.trackable.Tracker;
 import forge.util.IHasForgeLog;
 import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufAllocator;
 import io.netty.buffer.ByteBufOutputStream;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.MessageToByteEncoder;
@@ -29,6 +30,23 @@ public void setTracker(Tracker tracker) {
 
     @Override
     protected void encode(ChannelHandlerContext ctx, Serializable msg, ByteBuf out) throws Exception {
+        encodeInto(msg, out, this.tracker, this.byteTracker);
+    }
+
+    /** Caller passes the returned buffer to writeAndFlush, which takes ownership. */
+    public ByteBuf encodeToBuf(Serializable msg, ByteBufAllocator alloc) throws Exception {
+        ByteBuf out = alloc.buffer();
+        try {
+            encodeInto(msg, out, this.tracker, this.byteTracker);
+        } catch (Exception e) {
+            out.release();
+            throw e;
+        }
+        return out;
+    }
+
+    private static void encodeInto(Serializable msg, ByteBuf out, Tracker tracker,
+                                   NetworkByteTracker byteTracker) throws Exception {
         int startIdx = out.writerIndex();
         ByteBufOutputStream bout = new ByteBufOutputStream(out);
         ObjectOutputStream oout = null;
@@ -39,7 +57,7 @@ protected void encode(ChannelHandlerContext ctx, Serializable msg, ByteBuf out)
             bout.write(LENGTH_PLACEHOLDER);
             if (GuiBase.hasPropertyConfig()) {
                 oout = replace
-                        ? new TrackableSerializer.ReplacingOutputStream(new LZ4BlockOutputStream(bout), tracker)
+                        ? new TrackableSerializer.ReplacingOutputStream(new LZ4BlockOutputStream(bout), tracker, false)
                         : new ObjectOutputStream(new LZ4BlockOutputStream(bout));
             } else {
                 oout = new CObjectOutputStream(new LZ4BlockOutputStream(bout), replace);

forge-gui/src/main/java/forge/gamemodes/net/TrackableSerializer.java

@@ -33,38 +33,14 @@ public final class TrackableSerializer {
     static final byte TYPE_CARD_VIEW = 0;
     static final byte TYPE_PLAYER_VIEW = 1;
 
-    /**
-     * Lightweight serializable marker that replaces a TrackableObject reference.
-     */
-    static final class IdRef implements Serializable {
-        private static final long serialVersionUID = 1L;
-        final byte typeTag;
-        final int id;
-
-        IdRef(byte typeTag, int id) {
-            this.typeTag = typeTag;
-            this.id = id;
-        }
+    /** Marker for tracker-stable refs (top-level protocol method args, and PlayerView in events). */
+    record IdRef(byte typeTag, int id) implements Serializable {
+        @Serial private static final long serialVersionUID = 1L;
     }
 
-    /**
-     * Marker for a stale CardView reference — the object holds a previous
-     * incarnation of a card (same ID, different Java object) that has since
-     * been replaced in the tracker by a zone-change copy. Carries the
-     * image key and name so the decoder can construct a detached CardView
-     * with the correct display data.
-     */
-    static final class StaleCardRef implements Serializable {
-        private static final long serialVersionUID = 1L;
-        final int id;
-        final String imageKey;
-        final String name;
-
-        StaleCardRef(int id, String imageKey, String name) {
-            this.id = id;
-            this.imageKey = imageKey;
-            this.name = name;
-        }
+    /** CardView ref inside a wrapped event. {@code preserveSnapshot=true} forces fallback even if the tracker has the id. */
+    record EventCardRef(int id, String name, String imageKey, boolean preserveSnapshot) implements Serializable {
+        @Serial private static final long serialVersionUID = 1L;
     }
 
     static byte typeTagFor(TrackableObject obj) {
@@ -83,59 +59,66 @@ static TrackableType<?> trackableTypeFor(byte typeTag) {
 
     /**
      * Replaces TrackableObject references with {@link IdRef} markers, or
-     * {@link StaleCardRef} markers for CardViews whose tracker entry has
-     * been replaced by a zone-change copy. When {@code tracker} is null,
-     * stale detection is skipped (used by the client encoder, which has
-     * no game-state awareness).
+     * {@link EventCardRef} markers for CardViews inside wrapped events
+     * ({@code eventMode = true}). When the tracker holds a different object
+     * for the CardView's id (zone-change copy), {@code preserveSnapshot} is
+     * set so the receiver decodes a detached CardView from the carried name
+     * and image key. When {@code tracker} is null, the snapshot check is
+     * skipped (used by the client encoder, which has no game-state awareness).
      */
-    static Object replace(Object obj, Tracker tracker) {
+    static Object replace(Object obj, Tracker tracker, boolean eventMode) {
         if (obj instanceof TrackableObject trackable) {
             byte tag = typeTagFor(trackable);
-            if (tag >= 0) {
-                if (tracker != null) {
-                    TrackableType<?> type = trackableTypeFor(tag);
-                    if (type != null) {
-                        Object tracked = tracker.getObj(type, trackable.getId());
-                        if (tracked != trackable && tag == TYPE_CARD_VIEW && tracked != null) {
-                            // Stale reference: previous incarnation of this card
-                            CardView cv = (CardView) trackable;
-                            String imgKey = cv.getCurrentState() != null
-                                    ? cv.getCurrentState().getImageKey(null) : null;
-                            return new StaleCardRef(cv.getId(), imgKey, cv.getName());
-                        }
+            if (tag < 0) return obj;
+
+            if (!eventMode || tag == TYPE_PLAYER_VIEW) {
+                return new IdRef(tag, trackable.getId());
+            }
+
+            boolean preserveSnapshot = false;
+            if (tracker != null) {
+                TrackableType<?> type = trackableTypeFor(tag);
+                if (type != null) {
+                    Object tracked = tracker.getObj(type, trackable.getId());
+                    if (tracked != null && tracked != trackable) {
+                        preserveSnapshot = true;
                     }
                 }
-                return new IdRef(tag, trackable.getId());
             }
+            CardView cv = (CardView) trackable;
+            String imgKey = cv.getCurrentState() != null
+                    ? cv.getCurrentState().getImageKey(null) : null;
+            return new EventCardRef(trackable.getId(), cv.getName(), imgKey, preserveSnapshot);
         }
         return obj;
     }
 
     /**
-     * Resolves {@link IdRef} and {@link StaleCardRef} markers back to
+     * Resolves {@link IdRef} and {@link EventCardRef} markers back to
      * TrackableObjects from the given Tracker.
      */
     static Object resolve(Object obj, Tracker tracker) {
-        if (obj instanceof StaleCardRef ref) {
-            // Create a detached CardView with the correct image key.
-            // Not registered in the tracker — used only for display
-            // (game log thumbnail) so it won't affect live game state.
-            CardView detached = new CardView(ref.id, tracker);
-            if (ref.name != null) {
-                detached.set(TrackableProperty.Name, ref.name);
-                detached.getCurrentState().set(TrackableProperty.Name, ref.name);
+        if (obj instanceof EventCardRef ref) {
+            if (!ref.preserveSnapshot()) {
+                CardView fromTracker = tracker.getObj(TrackableTypes.CardViewType, ref.id());
+                if (fromTracker != null) return fromTracker;
+            }
+            CardView detached = new CardView(ref.id(), tracker);
+            if (ref.name() != null) {
+                detached.set(TrackableProperty.Name, ref.name());
+                detached.getCurrentState().set(TrackableProperty.Name, ref.name());
             }
-            if (ref.imageKey != null) {
-                detached.getCurrentState().set(TrackableProperty.ImageKey, ref.imageKey);
+            if (ref.imageKey() != null) {
+                detached.getCurrentState().set(TrackableProperty.ImageKey, ref.imageKey());
             }
             return detached;
         }
         if (obj instanceof IdRef ref) {
-            TrackableType<?> type = trackableTypeFor(ref.typeTag);
+            TrackableType<?> type = trackableTypeFor(ref.typeTag());
             if (type != null) {
-                Object resolved = tracker.getObj(type, ref.id);
+                Object resolved = tracker.getObj(type, ref.id());
                 if (resolved == null) {
-                    netLog.warn("Could not resolve IdRef(tag={}, id={}) from Tracker", ref.typeTag, ref.id);
+                    netLog.warn("Could not resolve IdRef(tag={}, id={}) from Tracker", ref.typeTag(), ref.id());
                 }
                 return resolved;
             }
@@ -152,7 +135,7 @@ public static int measureSize(Object obj, Tracker tracker) {
         try {
             ByteArrayOutputStream baos = new ByteArrayOutputStream();
             LZ4BlockOutputStream lz4Out = new LZ4BlockOutputStream(baos);
-            ObjectOutputStream oos = tracker == null ? new ObjectOutputStream(lz4Out) : new ReplacingOutputStream(lz4Out, tracker);
+            ObjectOutputStream oos = tracker == null ? new ObjectOutputStream(lz4Out) : new ReplacingOutputStream(lz4Out, tracker, false);
             oos.writeObject(obj);
             oos.close();
             return baos.size();
@@ -163,7 +146,7 @@ public static int measureSize(Object obj, Tracker tracker) {
 
     /**
      * Serializable wrapper for a GameEvent whose TrackableObject references
-     * have been replaced with IdRef/StaleCardRef markers. Stored in
+     * have been replaced with IdRef/EventCardRef markers. Stored in
      * DeltaPacket.events so events travel as compact byte arrays rather than
      * full object graphs. Unwrapped after delta state is applied, when the
      * client tracker is populated.
@@ -183,7 +166,7 @@ public static List<Object> wrapEvents(List<GameEvent> events, Tracker tracker) {
         for (GameEvent event : events) {
             try {
                 ByteArrayOutputStream baos = new ByteArrayOutputStream(256);
-                try (ReplacingOutputStream out = new ReplacingOutputStream(baos, tracker)) {
+                try (ReplacingOutputStream out = new ReplacingOutputStream(baos, tracker, true)) {
                     out.writeObject(event);
                 }
                 wrapped.add(new WrappedEvent(baos.toByteArray()));
@@ -219,16 +202,18 @@ public static List<GameEvent> unwrapEvents(List<Object> items, Tracker tracker)
 
     static class ReplacingOutputStream extends ObjectOutputStream {
         private final Tracker tracker;
+        private final boolean eventMode;
 
-        ReplacingOutputStream(OutputStream out, Tracker tracker) throws IOException {
+        ReplacingOutputStream(OutputStream out, Tracker tracker, boolean eventMode) throws IOException {
             super(out);
             this.tracker = tracker;
+            this.eventMode = eventMode;
             enableReplaceObject(true);
         }
 
         @Override
         protected Object replaceObject(Object obj) {
-            return replace(obj, tracker);
+            return replace(obj, tracker, eventMode);
         }
     }
 

forge-gui/src/main/java/forge/gamemodes/net/client/FGameClient.java

@@ -11,6 +11,7 @@
 import forge.gui.interfaces.IGuiGame;
 import forge.interfaces.ILobbyListener;
 import io.netty.bootstrap.Bootstrap;
+import io.netty.buffer.ByteBuf;
 import io.netty.channel.*;
 import io.netty.channel.nio.NioEventLoopGroup;
 import io.netty.channel.socket.SocketChannel;
@@ -105,7 +106,19 @@ public void send(final NetEvent event) {
             return;
         }
         netLog.info("Client sent {}", event);
-        channel.writeAndFlush(event);
+        final CompatibleObjectEncoder encoder = channel.pipeline().get(CompatibleObjectEncoder.class);
+        if (encoder == null) {
+            netLog.error("No encoder in client pipeline for {}", event);
+            return;
+        }
+        final ByteBuf encoded;
+        try {
+            encoded = encoder.encodeToBuf(event, channel.alloc());
+        } catch (Exception e) {
+            netLog.error(e, "Client encode error for {}", event);
+            return;
+        }
+        channel.writeAndFlush(encoded);
     }
 
     /**

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

@@ -7,7 +7,7 @@
 import forge.gamemodes.net.event.IdentifiableNetEvent;
 import forge.gamemodes.net.event.NetEvent;
 import forge.util.IHasForgeLog;
-
+import io.netty.buffer.ByteBuf;
 import io.netty.channel.Channel;
 
 import java.util.concurrent.atomic.AtomicInteger;
@@ -57,11 +57,31 @@ public boolean hasValidSlot() {
         return index >= 0;
     }
 
+    /** Encodes synchronously on the caller's thread. Returns null on failure (logged). */
+    private ByteBuf encodeOnCallingThread(final NetEvent event) {
+        final Channel ch = channel;
+        final CompatibleObjectEncoder encoder = ch.pipeline().get(CompatibleObjectEncoder.class);
+        if (encoder == null) {
+            netLog.error("No encoder in pipeline for {} (event: {})", username, event);
+            sendErrors.incrementAndGet();
+            return null;
+        }
+        try {
+            return encoder.encodeToBuf(event, ch.alloc());
+        } catch (Exception e) {
+            sendErrors.incrementAndGet();
+            netLog.error(e, "Network encode error for {} (event: {})", username, event);
+            return null;
+        }
+    }
+
     @Override
     public void send(final NetEvent event) {
         final Channel ch = channel;
         recordSendIfSaturated(ch);
-        ch.writeAndFlush(event).addListener(f -> {
+        final ByteBuf encoded = encodeOnCallingThread(event);
+        if (encoded == null) return;
+        ch.writeAndFlush(encoded).addListener(f -> {
             if (!f.isSuccess()) {
                 sendErrors.incrementAndGet();
                 Throwable c = f.cause();
@@ -79,15 +99,33 @@ public void send(final NetEvent event) {
     public void write(final NetEvent event) {
         final Channel ch = channel;
         recordSendIfSaturated(ch);
-        ch.write(event);
+        final ByteBuf encoded = encodeOnCallingThread(event);
+        if (encoded == null) return;
+        ch.write(encoded).addListener(f -> {
+            if (!f.isSuccess()) {
+                sendErrors.incrementAndGet();
+                Throwable c = f.cause();
+                if (c != null) {
+                    netLog.error(c, "Network write error for {} (event: {})", username, event);
+                } else {
+                    netLog.error("Network write error for {} (event: {}, cause: {})",
+                            username, event, f.isCancelled() ? "cancelled" : "no cause");
+                }
+            }
+        });
     }
 
     @Override
     public Object sendAndWait(final IdentifiableNetEvent event) {
         replies.initialize(event.getId());
         final Channel ch = channel;
         recordSendIfSaturated(ch);
-        ch.writeAndFlush(event).addListener(f -> {
+        final ByteBuf encoded = encodeOnCallingThread(event);
+        if (encoded == null) {
+            replies.complete(event.getId(), null);
+            return replies.get(event.getId());
+        }
+        ch.writeAndFlush(encoded).addListener(f -> {
             if (!f.isSuccess()) {
                 sendErrors.incrementAndGet();
                 Throwable c = f.cause();