A dismount during a pending mount load no longer crashes (#2390)

Dismounting while the mount model was still pending despawned that child, but the same attach pass could still visit it afterwards. Its plain inserts then landed on a despawned entity, and Bevy's default command handler panicked.

The attach pass now records the children it drops and skips them for the rest of the pass, so the outcome no longer depends on visit order. The despawns there became `try_despawn`.

The player build also installs a command error handler. A failed command now logs a warning naming the command and the entity, instead of ending the session. Every other error still panics, and the tests keep the panicking default, so our own bugs still fail there.

Tests: a dropped pending child is skipped whatever the visit order, for both the dismount and the stale display; the handler logs a failed command and still panics on other errors.

Fixes #2377

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/benilla-app/src/crash.rs

@@ -14,6 +14,8 @@ use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH};
 
+use bevy::ecs::error::{BevyError, ErrorContext};
+
 use crate::BuildId;
 
 /// Chains the crash-report hook after the current one. Called once as the client launches
@@ -28,6 +30,20 @@ pub(crate) fn install(build: BuildId) {
     }));
 }
 
+/// The client's default error handler, installed by [`crate::run`] on its `App`: a command that
+/// fails (an entity a same-frame despawn already removed) is logged at `warn` with the command and
+/// the entity, and the session goes on. Every other error stays a panic, as Bevy's default, and
+/// reaches the crash report: a missing resource or a failed system is a bug to see, not to skip.
+/// Only the real launch installs it. Tests build their own `App`, which keeps Bevy's panicking
+/// default, so a command that fails in a test still fails the test.
+pub(crate) fn command_error_handler(error: BevyError, ctx: ErrorContext) {
+    if matches!(ctx, ErrorContext::Command { .. }) {
+        bevy::log::warn!("{ctx}: {error}");
+    } else {
+        bevy::ecs::error::panic(error, ctx);
+    }
+}
+
 // The injected test panic (`WOW_CRASH_INJECT=<at_secs>`) lives in `perf::crash_inject`, behind
 // the `dev` feature.
 
@@ -190,4 +206,70 @@ mod tests {
         assert_eq!(std::fs::read_to_string(&path).unwrap(), text);
         let _ = std::fs::remove_dir_all(&dir);
     }
+
+    /// A failed command is logged with the command and the entity and the frame goes on; any
+    /// other error is still a panic.
+    #[test]
+    fn a_failed_command_logs_and_other_errors_still_panic() {
+        use std::sync::{Arc, Mutex};
+
+        use bevy::prelude::{App, Name};
+
+        use bevy::log::tracing::field::{Field, Visit};
+        use bevy::log::tracing::{Event, Subscriber};
+        use bevy::log::tracing_subscriber::layer::{Context, SubscriberExt};
+        use bevy::log::tracing_subscriber::Layer;
+
+        struct Capture(Arc<Mutex<Vec<String>>>);
+        struct Message(String);
+        impl Visit for Message {
+            fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) {
+                if field.name() == "message" {
+                    self.0 = format!("{value:?}");
+                }
+            }
+        }
+        impl<S: Subscriber> Layer<S> for Capture {
+            fn on_event(&self, event: &Event<'_>, _: Context<'_, S>) {
+                let mut m = Message(String::new());
+                event.record(&mut m);
+                self.0
+                    .lock()
+                    .unwrap()
+                    .push(format!("{} {}", event.metadata().level(), m.0));
+            }
+        }
+
+        let mut app = App::new();
+        app.set_error_handler(command_error_handler);
+        let gone = app.world_mut().spawn_empty().id();
+        app.world_mut().despawn(gone);
+
+        let lines = Arc::new(Mutex::new(Vec::new()));
+        let subscriber = bevy::log::tracing_subscriber::registry().with(Capture(lines.clone()));
+        bevy::log::tracing::subscriber::with_default(subscriber, || {
+            app.world_mut()
+                .commands()
+                .entity(gone)
+                .insert(Name::new("x"));
+            app.world_mut().flush();
+        });
+        let lines = lines.lock().unwrap();
+        assert_eq!(lines.len(), 1, "{lines:?}");
+        assert!(lines[0].starts_with("WARN "), "{lines:?}");
+        assert!(lines[0].contains("Command"), "names the command: {lines:?}");
+        assert!(
+            lines[0].contains(&gone.to_string())
+                || lines[0].contains(&format!("{}v", gone.index())),
+            "names the entity: {lines:?}"
+        );
+
+        let system = ErrorContext::System {
+            name: bevy::utils::prelude::DebugName::borrowed("s"),
+            last_run: bevy::ecs::change_detection::Tick::new(0),
+        };
+        let panicked =
+            std::panic::catch_unwind(|| command_error_handler(BevyError::from("boom"), system));
+        assert!(panicked.is_err(), "a system error stays a panic");
+    }
 }

crates/benilla-app/src/entities/attach/mod.rs

@@ -289,6 +289,10 @@ pub(super) fn attach_entity_visuals(
         mut images,
     ) = skin_build;
     let now = time.elapsed_secs();
+    // Mount children this pass has despawned for a dismounted rider. The query was read before
+    // the despawn applies, so a child visited after its rider is still in it: skipping it here
+    // keeps every write off an entity the pass itself removed, whatever the iteration order.
+    let mut dropped: Vec<Entity> = Vec::new();
     for (
         entity,
         net,
@@ -301,6 +305,9 @@ pub(super) fn attach_entity_visuals(
         shown_rebuild,
     ) in &pending
     {
+        if dropped.contains(&entity) {
+            continue;
+        }
         // The appear ramp's origin: the unit's arrival, however long its visual waited to build.
         let arrived = arrival.map_or(now, |a| a.0);
         // A player attaches once its equipment settles, so it never flashes naked.
@@ -403,6 +410,7 @@ pub(super) fn attach_entity_visuals(
             if mount_display != 0 {
                 match super::mount::seat_or_spawn_mount(
                     &mut commands,
+                    &mut dropped,
                     &mount_children,
                     &mut built_poses,
                     creatures.as_deref(),
@@ -420,9 +428,8 @@ pub(super) fn attach_entity_visuals(
                 }
             } else if let Some(&super::mount::MountChild(child)) = mount_child {
                 // Dismounted while pending: drop the mount it ordered.
-                if let Ok(mut ec) = commands.get_entity(child) {
-                    ec.despawn();
-                }
+                commands.entity(child).try_despawn();
+                dropped.push(child);
                 commands.entity(entity).remove::<super::mount::MountChild>();
             }
             let kind = match net.kind {
@@ -1082,4 +1089,104 @@ mod tests {
             "a live unit still builds"
         );
     }
+
+    /// A rider that drops its still-pending mount child, dismounted or mounted on another display,
+    /// has the pass skip that child wherever the iteration reaches it: its writes would otherwise
+    /// apply to an entity the same pass despawned, which the default command error handler
+    /// panics on.
+    #[test]
+    fn a_dropped_pending_mount_child_is_skipped_whatever_the_visit_order() {
+        use super::super::mount::{MountBody, MountChild};
+
+        for (child_table_first, stale_display) in
+            [(true, false), (false, false), (true, true), (false, true)]
+        {
+            let mut app = App::new();
+            app.add_plugins((MinimalPlugins, AssetPlugin::default()))
+                .init_asset::<Image>()
+                .init_asset::<Mesh>()
+                .init_asset::<benilla_assets::materials::WowModelMaterial>()
+                .init_resource::<benilla_world::model_render::ModelMaterials>()
+                .init_resource::<super::super::corpse::BonesModels>()
+                .init_resource::<SkinComposites>()
+                .init_resource::<merge::MergedFormsCache>()
+                .init_resource::<benilla_world::doodad_anim::UvAnimMaterials>()
+                .init_resource::<benilla_world::doodad_anim::TintAnimMaterials>()
+                .init_resource::<benilla_world::mat_anim_table::MatAnimTable>()
+                .init_resource::<benilla_world::rig_palette::RigPalettes>()
+                .init_resource::<benilla_world::collision::ColliderEpoch>()
+                .insert_resource(CubeAssets {
+                    mesh: Handle::default(),
+                    player_mesh: Handle::default(),
+                    player_mat: Handle::default(),
+                    npc_mat: Handle::default(),
+                })
+                // A loaded model that names a file and draws nothing: the build runs, so the
+                // rider's dismount branch and the child's writes are reached.
+                .insert_resource(Creatures {
+                    catalog: Default::default(),
+                    models: std::collections::HashMap::from([(
+                        1u32,
+                        DisplayModel {
+                            handle: ModelHandle::M2(Handle::default()),
+                            parts: Some(Vec::new()),
+                            ..super::super::display::empty_shell()
+                        },
+                    )]),
+                });
+            let net = || NetEntity {
+                kind: EntityKind::Unit,
+                display_id: Some(1),
+                scale: 1.0,
+            };
+            let world = app.world_mut();
+            // The mount display the rider's field names: none (dismounted), or one the child was
+            // not built for.
+            let store = ObjectStore(benilla_protocol::ObjectFields::from_pairs(&[(
+                benilla_protocol::field::FIELD_UNIT_MOUNTDISPLAYID,
+                if stale_display { 2 } else { 0 },
+            )]));
+            // The two sit in different tables, and which one the query walks first follows the
+            // order they were created in, so each layout is run: the rider is visited before its
+            // child in one of them (measured, not assumed).
+            let (rider, child) = if child_table_first {
+                let child = world
+                    .spawn((
+                        net(),
+                        MountBody {
+                            host: Entity::PLACEHOLDER,
+                        },
+                    ))
+                    .id();
+                let rider = world.spawn((net(), MountChild(child), store)).id();
+                world.entity_mut(child).insert(MountBody { host: rider });
+                (rider, child)
+            } else {
+                let child = world.spawn_empty().id();
+                let rider = world.spawn((net(), MountChild(child), store)).id();
+                world.entity_mut(child).insert((
+                    net(),
+                    MountBody { host: rider },
+                    Transform::default(),
+                ));
+                (rider, child)
+            };
+
+            let mut build = IntoSystem::into_system(attach_entity_visuals);
+            build.initialize(world);
+            build
+                .run_without_applying_deferred((), world)
+                .expect("the build system runs");
+            build.apply_deferred(world);
+
+            assert!(
+                world.get_entity(child).is_err(),
+                "the pending mount is gone (child_table_first={child_table_first}, stale={stale_display})"
+            );
+            let rider = world.entity(rider);
+            assert!(!rider.contains::<MountChild>());
+            // A stale mount waits for the next pass to spawn the right one; a dismounted unit builds.
+            assert_eq!(rider.contains::<VisualAttached>(), !stale_display);
+        }
+    }
 }

crates/benilla-app/src/entities/mount.rs

@@ -115,9 +115,12 @@ pub(super) type MountChildren<'w, 's> = Query<
 >;
 
 /// The seat leg of the first build and the transition: spawn the mount child when missing, drop
-/// one built for a display the field has left, and resolve the seat once its rig is up.
+/// one built for a display the field has left, and resolve the seat once its rig is up. A child it
+/// despawns is pushed on `dropped`, which a caller still iterating children must skip: its query
+/// predates the despawn.
 pub(super) fn seat_or_spawn_mount(
     commands: &mut Commands,
+    dropped: &mut Vec<Entity>,
     children: &MountChildren,
     poses: &mut Query<&mut RigPose>,
     creatures: Option<&super::Creatures>,
@@ -145,9 +148,8 @@ pub(super) fn seat_or_spawn_mount(
     // Built for a display the field has left, or gone: drop it and start over next pass.
     let built = children.get(child).map(|(_, _, n)| n.display_id).ok();
     if built != Some(Some(mount_display)) {
-        if let Ok(mut ec) = commands.get_entity(child) {
-            ec.despawn();
-        }
+        commands.entity(child).try_despawn();
+        dropped.push(child);
         commands.entity(unit).remove::<MountChild>();
         return Seat::Wait;
     }
@@ -220,9 +222,7 @@ fn reseat_rig(commands: &mut Commands, rig: &mut RigPose, frame: Entity, conform
     rig.pose_dirty = true;
     // A conform node the rig just left is empty, so it goes; a seat anchor dies with its mount.
     if conform {
-        if let Ok(mut ec) = commands.get_entity(old) {
-            ec.despawn();
-        }
+        commands.entity(old).try_despawn();
     }
 }
 
@@ -269,9 +269,7 @@ pub(super) fn reseat_mounts(
         // sweep any child, which the settled test would otherwise re-fire on every frame.
         let Ok(seated_root) = poses.get(entity).map(|r| r.joints_root) else {
             if let Some(&MountChild(child)) = child {
-                if let Ok(mut ec) = commands.get_entity(child) {
-                    ec.despawn();
-                }
+                commands.entity(child).try_despawn();
                 commands.entity(entity).remove::<MountChild>();
             }
             commands.entity(entity).insert(AppliedMount(live));
@@ -291,9 +289,7 @@ pub(super) fn reseat_mounts(
                 }
             }
             if let Some(child) = child {
-                if let Ok(mut ec) = commands.get_entity(child) {
-                    ec.despawn();
-                }
+                commands.entity(child).try_despawn();
             }
             commands
                 .entity(entity)
@@ -304,6 +300,8 @@ pub(super) fn reseat_mounts(
         // ── Leg 2 · `0x607a00`: build the mount model and attach the body to its seat.
         match seat_or_spawn_mount(
             &mut commands,
+            // This system walks riders, never children, so nothing here revisits a despawned one.
+            &mut Vec::new(),
             &children,
             &mut poses,
             creatures.as_deref(),

crates/benilla-app/src/lib.rs

@@ -282,6 +282,8 @@ fn launch(build: BuildId, extend: Option<Extension<'_>>) -> AppExit {
     };
 
     let mut app = App::new();
+    // A command on an entity a same-frame despawn removed logs instead of aborting the session.
+    app.set_error_handler(crash::command_error_handler);
     // The panel footer and the build banner read the stamp back.
     app.insert_resource(build);
     // Static-scene transform tracking pinned on: the default threshold re-decides every frame with