From a97e063f9ce11529fa12c2e5090c6bc275fd4b22 Mon Sep 17 00:00:00 2001 From: Sam Sung <51686767+samwhosung@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:55:00 +0700 Subject: [PATCH] A dismount during a pending mount load no longer crashes (#2390) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit ce5503ca538dcd98424efe136d12cdfe7259643a) # Conflicts: # crates/benilla-app/src/entities/attach/mod.rs # crates/benilla-app/src/lib.rs Absorbed into the octo fork. Merged surgically rather than taken: upstream's hunk also restores the 'a player attaches once its equipment settles' gate, which this fork deliberately removed โ€” gating on Equipment::settled withheld a body for good on any chain that never answers a visible-item query, which in a battleground crowd is every other player. Our gate and comment stand; what is absorbed is the crash fix itself (the dropped-children list, the skip at the top of the loop, try_despawn) plus the command error handler in lib.rs. The arrival/shown_rebuild query legs belong to other upstream commits and are left out. attach 28 green, crash 2 green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/crash.rs | 82 +++++++++++++ crates/benilla-app/src/entities/attach/mod.rs | 113 +++++++++++++++++- crates/benilla-app/src/entities/mount.rs | 24 ++-- crates/benilla-app/src/lib.rs | 2 + 4 files changed, 205 insertions(+), 16 deletions(-) diff --git a/crates/benilla-app/src/crash.rs b/crates/benilla-app/src/crash.rs index eb17c7ff..723d6579 100644 --- a/crates/benilla-app/src/crash.rs +++ b/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 from [`crate::run`], before @@ -27,6 +29,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=`) lives in `perf::crash_inject`, behind // the `dev` feature. @@ -180,4 +196,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>>); + 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 Layer 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"); + } } diff --git a/crates/benilla-app/src/entities/attach/mod.rs b/crates/benilla-app/src/entities/attach/mod.rs index 16f4ddbc..e041e74b 100644 --- a/crates/benilla-app/src/entities/attach/mod.rs +++ b/crates/benilla-app/src/entities/attach/mod.rs @@ -295,7 +295,14 @@ pub(super) fn attach_entity_visuals( mut anim_table, ) = 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 = Vec::new(); for (entity, net, equipment, reattached, mount_child, mount_body, anchored) in &pending { + if dropped.contains(&entity) { + continue; + } // A player attaches as soon as its display model is ready. 1.12.1 withholds a unit only // until its first composite (`0x477860`) is done, never until every `PLAYER_VISIBLE_ITEM` // template is answered: the compositor paints the armour it knows and re-dresses when a @@ -360,6 +367,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(), @@ -376,9 +384,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) .try_remove::(); @@ -1041,4 +1048,104 @@ mod tests { ); world.insert_resource(palettes); } + + /// 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::() + .init_asset::() + .init_asset::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .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::()); + // A stale mount waits for the next pass to spawn the right one; a dismounted unit builds. + assert_eq!(rider.contains::(), !stale_display); + } + } } diff --git a/crates/benilla-app/src/entities/mount.rs b/crates/benilla-app/src/entities/mount.rs index b016f0a9..b913b9d4 100644 --- a/crates/benilla-app/src/entities/mount.rs +++ b/crates/benilla-app/src/entities/mount.rs @@ -110,9 +110,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, children: &MountChildren, poses: &mut Query<&mut RigPose>, creatures: Option<&super::Creatures>, @@ -132,9 +135,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::(); return Seat::Wait; } @@ -207,9 +209,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(); } } @@ -251,9 +251,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::(); } commands.entity(entity).insert(AppliedMount(live)); @@ -273,9 +271,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) @@ -286,6 +282,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(), diff --git a/crates/benilla-app/src/lib.rs b/crates/benilla-app/src/lib.rs index b46e56df..39cc4cb2 100644 --- a/crates/benilla-app/src/lib.rs +++ b/crates/benilla-app/src/lib.rs @@ -258,6 +258,8 @@ pub fn run(build: BuildId) -> 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 preflight banner read the stamp back. app.insert_resource(build); // Static-scene transform tracking pinned on: the default threshold re-decides every frame with