Fix attachment sync review findings

crates/horizon-client/src/game/attachment_sync.rs

@@ -31,9 +31,8 @@ fn recipients(hub: &Hub, holder: Holder) -> impl Iterator<Item = &Guest> {
 
 impl Game<'_> {
     /// The messages that give a player `holder`'s synced attachments whole: its
-    /// own, and for a chunk those of its block entities. A player holder with
-    /// none still gives one, to drop what the client holds; other holders
-    /// with none give no message.
+    /// own, and for a chunk those of its block entities; a holder with none
+    /// gives no message.
     fn attachment_snapshot(&mut self, holder: Holder) -> Vec<Message> {
         let changes = match holder {
             // Read in place: a chunk lent to a mod is copied and saved.
@@ -68,7 +67,7 @@ impl Game<'_> {
         }
         holders
             .into_iter()
-            .filter(|(holder, changes)| !changes.is_empty() || matches!(holder, Holder::Player(_)))
+            .filter(|(_, changes)| !changes.is_empty())
             .map(|(holder, changes)| Message {
                 holder,
                 replace: true,

crates/horizon-client/src/game/remote.rs

@@ -2284,7 +2284,9 @@ impl Game<'_> {
         }
     }
     /// Keep the renderable block entities (chests, furnaces) in step with blocks.
-    /// `previous`: the block's state before, when it changed.
+    /// `previous`: the block's state before, when it changed. A column sent again has
+    /// none, and keeps its block entities' synced attachments: the server does not send
+    /// them again (`remote_chunk` drops those of block entities the column lost).
     pub(super) fn sync_block_entity(
         &mut self,
         pos: (i32, i32, i32),
@@ -2302,12 +2304,16 @@ impl Game<'_> {
                     e.id() == entity.id() && !(other_block && matches!(e, BlockEntity::Mod(_)))
                 });
                 if !same {
-                    self.forget_block_entity_attachments(pos);
+                    if previous.is_some() {
+                        self.forget_block_entity_attachments(pos);
+                    }
                     self.world.block_entities.insert(pos, entity);
                 }
             }
             None => {
-                self.forget_block_entity_attachments(pos);
+                if previous.is_some() {
+                    self.forget_block_entity_attachments(pos);
+                }
                 self.world.block_entities.remove(&pos);
             }
         }

crates/horizon-client/src/game/remote_chunks.rs

@@ -13,6 +13,7 @@ use horizon_protocol::{
     packets::play::{self, Clientbound as C},
     registry::DimensionType,
 };
+use horizon_world::attachments::Holder;
 use std::sync::{
     Arc,
     mpsc::{self, TryRecvError},
@@ -307,6 +308,15 @@ impl Game<'_> {
             // LevelChunk.replaceWithPacketData: the block entity loads its update tag.
             self.remote_block_entity_data([pos.0, pos.1, pos.2], info.kind, &info.tag);
         }
+        if let Some(r) = self.remote.as_mut() {
+            r.attachments.retain(|holder, _| match *holder {
+                Holder::BlockEntity([bx, by, bz]) => {
+                    (bx >> 4, bz >> 4) != (x, z)
+                        || self.world.block_entities.contains_key(&(bx, by, bz))
+                }
+                _ => true,
+            });
+        }
     }
 
     /// `ClientPacketListener.handleChunksBiomes`: each loaded column takes

crates/horizon-client/src/lan/guest.rs

@@ -502,7 +502,8 @@ impl Guest {
     pub fn reads_attachment_sync(&self) -> bool {
         self.placed && self.brand == "horizon"
     }
-    /// A holder's synced attachments, if the guest reads them.
+    /// A holder's synced attachments, if the guest reads them; a message too big
+    /// for one payload goes in halves.
     pub fn send_attachment_sync(&self, message: &attachment_sync::Message) {
         if !self.reads_attachment_sync() {
             return;
@@ -512,6 +513,21 @@ impl Guest {
                 channel: attachment_sync::CHANNEL.into(),
                 data,
             })),
+            // One change is the unit that cannot be sent; the others still can.
+            Err(e) if message.changes.len() > 1 => {
+                let (first, rest) = message.changes.split_at(message.changes.len() / 2);
+                self.send_attachment_sync(&attachment_sync::Message {
+                    holder: message.holder,
+                    replace: message.replace,
+                    changes: first.to_vec(),
+                });
+                self.send_attachment_sync(&attachment_sync::Message {
+                    holder: message.holder,
+                    replace: false,
+                    changes: rest.to_vec(),
+                });
+                log::debug!("Horizon: attachment sync split: {e}");
+            }
             Err(e) => log::debug!("Horizon: attachment sync not sent: {e}"),
         }
     }

crates/horizon-client/src/lan/hub.rs

@@ -224,6 +224,23 @@ pub struct AttachmentSnapshot {
     pub holder: Holder,
 }
 
+/// Queues `holders` as snapshots for `player`; with no mod loaded there is
+/// nothing to sync, so none are queued.
+fn queue_snapshots(
+    snapshots: &mut Vec<AttachmentSnapshot>,
+    player: u128,
+    holders: impl IntoIterator<Item = Holder>,
+) {
+    if crate::mod_api::mods().is_empty() {
+        return;
+    }
+    snapshots.extend(
+        holders
+            .into_iter()
+            .map(|holder| AttachmentSnapshot { player, holder }),
+    );
+}
+
 /// Moves a guest's tracking events into `out`, one batch, and the entities it
 /// started tracking into `snapshots`.
 fn take_tracking(
@@ -234,10 +251,11 @@ fn take_tracking(
     let events = g.tracker.take_events();
     if !events.is_empty() {
         let started = events.iter().filter(|e| e.start);
-        snapshots.extend(started.map(|e| AttachmentSnapshot {
-            player: g.uuid,
-            holder: Holder::Entity(e.entity as u32),
-        }));
+        queue_snapshots(
+            snapshots,
+            g.uuid,
+            started.map(|e| Holder::Entity(e.entity as u32)),
+        );
         out.push(TrackingBatch {
             player: g.uuid,
             name: g.name.clone(),
@@ -1367,16 +1385,11 @@ impl Hub {
     fn send_level_info(&mut self, i: usize, ctx: &TickCtx, load_start: bool) {
         let spawn = ctx.spawn;
         let g = &self.guests[i];
-        self.attachment_snapshots.extend([
-            AttachmentSnapshot {
-                player: g.uuid,
-                holder: Holder::Level,
-            },
-            AttachmentSnapshot {
-                player: g.uuid,
-                holder: Holder::Player(g.uuid),
-            },
-        ]);
+        queue_snapshots(
+            &mut self.attachment_snapshots,
+            g.uuid,
+            [Holder::Level, Holder::Player(g.uuid)],
+        );
         g.send(&C::InitializeBorder(super::border::initialize(ctx.border)));
         g.send(&self.time_packet(ctx.world, ctx.clock_rate));
         g.send(&self.default_spawn(spawn));
@@ -1887,18 +1900,18 @@ impl Hub {
                 .rev()
                 .map(|idx| g.chunk_queue.remove(idx))
                 .collect();
+            // Far columns are terrain only: no snapshot.
+            queue_snapshots(
+                &mut self.attachment_snapshots,
+                g.uuid,
+                seen.iter().map(|&(x, z)| Holder::Chunk([x, z])),
+            );
             // A far chunk the save doesn't have leaves the queue unsent.
             seen.extend(far.into_iter().rev().filter_map(|(idx, sent)| {
                 let key = g.far_queue.remove(idx);
                 sent.then_some(key)
             }));
-            for key in seen {
-                self.attachment_snapshots.push(AttachmentSnapshot {
-                    player: g.uuid,
-                    holder: Holder::Chunk([key.0, key.1]),
-                });
-                g.sent_chunks.insert(key);
-            }
+            g.sent_chunks.extend(seen);
         }
     }
 

crates/horizon-protocol/src/attachment_sync.rs

@@ -8,7 +8,7 @@
 //! removal.
 use horizon_world::{
     attachments::{AttachmentSync, Holder},
-    nbt::{JavaString, Tag},
+    nbt::{self, Tag},
 };
 
 use crate::{
@@ -34,9 +34,9 @@ pub struct Message {
     pub changes: Vec<AttachmentSync>,
 }
 
-/// A message's payload; an error for a holder it cannot write. A change with a
-/// name past [`MAX_NAME`], or one that would take the payload past
-/// [`MAX_PAYLOAD`], is left out, so it cannot block the holder's others.
+/// A message's payload; an error for a holder it cannot write, a name past
+/// [`MAX_NAME`], a value that is not an encodable compound, or a payload past
+/// [`MAX_PAYLOAD`] (the sender splits the changes and tries again).
 pub fn encode(message: &Message) -> Result<Vec<u8>> {
     let mut w = Writer::new();
     match message.holder {
@@ -60,34 +60,30 @@ pub fn encode(message: &Message) -> Result<Vec<u8>> {
         _ => return Err(Error::new("attachment sync: unknown holder")),
     }
     w.bool(message.replace);
-    let mut body = Writer::new();
-    let mut count = 0;
+    w.varint(message.changes.len() as i32);
     for change in &message.changes {
-        // A name that is not UTF-16 or is too long leaves out only this change.
-        let name = |name: &JavaString| {
-            name.to_string()
-                .ok()
-                .filter(|n| n.encode_utf16().count() <= MAX_NAME)
-        };
-        let (Some(namespace), Some(name)) = (name(&change.namespace), name(&change.name)) else {
-            continue;
-        };
-        let mut entry = Writer::new();
-        entry.string(&namespace);
-        entry.string(&name);
+        for name in [&change.namespace, &change.name] {
+            let name = name
+                .to_string()
+                .map_err(|_| Error::new("attachment sync: name is not UTF-16"))?;
+            if name.encode_utf16().count() > MAX_NAME {
+                return Err(Error::new(format!("attachment sync: name {name} is too long")));
+            }
+            w.string(&name);
+        }
         match &change.value {
-            Some(value) => entry.nbt(&Tag::Compound(value.clone())),
-            None => entry.nbt(&Tag::End),
+            Some(value) => {
+                let tag = Tag::Compound(value.clone());
+                let bytes = nbt::write_network(&tag, nbt::Limits::default())
+                    .map_err(|e| Error::new(format!("attachment sync: value: {e}")))?;
+                w.bytes(&bytes);
+            }
+            None => w.bytes(&[0]),
         }
-        // The count's varint is at most five bytes.
-        if w.data.len() + 5 + body.data.len() + entry.data.len() > MAX_PAYLOAD {
-            continue;
-        }
-        body.data.extend(entry.data);
-        count += 1;
     }
-    w.varint(count);
-    w.data.extend(body.data);
+    if w.data.len() > MAX_PAYLOAD {
+        return Err(Error::new("attachment sync: payload too big"));
+    }
     Ok(w.data)
 }
 
@@ -187,7 +183,7 @@ mod tests {
     }
 
     #[test]
-    fn encode_leaves_out_a_name_decode_would_refuse() {
+    fn encode_refuses_a_change_the_client_would_not_read_as_sent() {
         let mut message = Message {
             holder: Holder::Level,
             replace: false,
@@ -196,7 +192,19 @@ mod tests {
         message.changes[0].name = "n".repeat(MAX_NAME).as_str().into();
         assert!(decode(&encode(&message).unwrap()).is_ok());
         message.changes[0].name = "n".repeat(MAX_NAME + 1).as_str().into();
-        let sent = decode(&encode(&message).unwrap()).unwrap();
-        assert_eq!(sent.changes, changes()[1..]);
+        assert!(encode(&message).is_err(), "name");
+        message.changes[0].name = "pool".into();
+        let mut deep = Tag::Int(0);
+        for _ in 0..600 {
+            deep = Tag::Compound([("v".into(), deep)].into());
+        }
+        let Tag::Compound(deep) = deep else {
+            unreachable!()
+        };
+        message.changes[0].value = Some(deep);
+        assert!(encode(&message).is_err(), "unwritable value");
+        let big = Tag::ByteArray(vec![0; MAX_PAYLOAD]);
+        message.changes[0].value = Some([("big".into(), big)].into());
+        assert!(encode(&message).is_err(), "payload");
     }
 }

crates/horizon-world/src/attachments.rs

@@ -593,7 +593,10 @@ mod tests {
         let mut map = Attachments::default();
         assert_eq!(map.get(&broken), Err(AttachmentError::InvalidSyncPolicy));
         assert_eq!(map.set(&broken, 1), Err(AttachmentError::InvalidSyncPolicy));
-        assert_eq!(map.mark_sync(&broken), Err(AttachmentError::InvalidSyncPolicy));
+        assert_eq!(
+            map.mark_sync(&broken),
+            Err(AttachmentError::InvalidSyncPolicy)
+        );
         assert!(map.take_sync_changes().is_empty());
     }
     #[test]

docs/native-mods.md

@@ -2197,14 +2197,18 @@ players who track the holder: guests who were sent a chunk for a chunk or block
 who track an entity for an entity, the player and those tracking them for a player, and
 everyone in the level for the level. A client also gets all of a holder's synced values
 when it starts seeing it: a chunk sent (with its block entities), a level joined or
-re-entered, an entity tracked, and its own player. The wire form is the codec's compound,
-so `sync` requires a codec. On a client, `Host::attachments` returns a copy of what the
-server synced, which `get` decodes; `set` and `remove` change only that copy, which is
-dropped when the chunk, entity or player goes. Only Horizon clients are sent values. A change whose namespace or name is over 256
-characters, or that would take a message past 1 MiB, is left out of it; the holder's others
-are still sent.
-A client keeps no copy for the level hosts (`LevelRandomHost`): they stay server-only
-there, though their changes on a server are synced.
+re-entered, an entity tracked, and its own player. Far columns get none. With no native
+mod loaded nothing is sent. The wire form is the codec's compound, so `sync` requires
+a codec.
+
+Only Horizon clients are sent values. A change whose namespace or name is over 256
+characters, whose value cannot be written or that alone takes a message past 1 MiB
+is not sent; the holder's others are, in more messages when they do not fit in one.
+
+On a client, `Host::attachments` returns a copy of what the server synced, which `get`
+decodes; `set` and `remove` change only that copy, which is dropped when the chunk,
+entity or player goes. A client keeps no copy for the level hosts (`LevelRandomHost`):
+they stay server-only there, though their changes on a server are synced.
 
 `copy_on_death` requires a codec. Player respawn copies opted-in entries via their
 encoded representation, including raw entries of an absent mod; other entries are