Ensure that the client clears pending trades on its side when exiting to

the character screen (and similar actions). When any entity is
deleted cancel any existing trades associated with it.

client/src/lib.rs

@@ -2065,6 +2065,8 @@ impl Client {
                             PresenceKind::Possessor => PresenceKind::Possessor,
                         });
                     }
+                    // Clear pending trade
+                    self.pending_trade = None;
                 } else {
                     return Err(Error::Other("Failed to find entity from uid.".into()));
                 }
@@ -2543,6 +2545,9 @@ impl Client {
 
     /// Clean client ECS state
     fn clean_state(&mut self) {
+        // Clear pending trade
+        self.pending_trade = None;
+
         let client_uid = self
             .uid()
             .map(|u| u.into())

server/src/cmd.rs

@@ -1851,7 +1851,7 @@ fn handle_kill_npcs(
     let count = to_kill.len();
     for entity in to_kill {
         // Directly remove entities instead of modifying health to avoid loot drops.
-        if let Err(e) server.state.delete_entity_recorded(entity) {
+        if let Err(e) = server.state.delete_entity_recorded(entity) {
             error!(?e, ?entity, "Failed to delete entity");
         }
     }

server/src/events/mod.rs

@@ -25,9 +25,10 @@ use inventory_manip::handle_inventory;
 use invite::{handle_invite, handle_invite_response};
 use player::{handle_client_disconnect, handle_exit_ingame, handle_possess};
 use specs::{Builder, Entity as EcsEntity, WorldExt};
-use trade::{cancel_trade_for, handle_process_trade_action};
+use trade::handle_process_trade_action;
 
 pub use group_manip::update_map_markers;
+pub(crate) use trade::cancel_trades_for;
 
 mod entity_creation;
 mod entity_manipulation;
@@ -165,7 +166,6 @@ impl Server {
                     handle_loaded_character_data(self, entity, components);
                 },
                 ServerEvent::ExitIngame { entity } => {
-                    cancel_trade_for(self, entity);
                     handle_exit_ingame(self, entity);
                 },
                 ServerEvent::CreateNpc {

server/src/events/player.rs

@@ -1,6 +1,6 @@
 use super::Event;
 use crate::{
-    client::Client, events::trade::cancel_trade_for, metrics::PlayerMetrics,
+    client::Client, metrics::PlayerMetrics,
     persistence::character_updater::CharacterUpdater, presence::Presence, state_ext::StateExt,
     BattleModeBuffer, Server,
 };
@@ -24,9 +24,16 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity) {
     let entity = persist_entity(state, entity);
 
     // Create new entity with just `Client`, `Uid`, `Player`, and `...Stream`
-    // components Easier than checking and removing all other known components
+    // components.
+    //
+    // Easier than checking and removing all other known components.
+    //
     // Note: If other `ServerEvent`s are referring to this entity they will be
-    // disrupted
+    // disrupted.
+
+    // Since we remove `Uid` below, any trades won't be cancelled by `delete_entity_recorded`. So
+    // we cancel the trade here. (maybe the trade key could be switched from `Uid` to `Entity`)
+    super::cancel_trades_for(state, entity);
 
     let maybe_admin = state.ecs().write_storage::<comp::Admin>().remove(entity);
     let maybe_group = state
@@ -115,7 +122,6 @@ pub fn handle_client_disconnect(
     skip_persistence: bool,
 ) -> Event {
     span!(_guard, "handle_client_disconnect");
-    cancel_trade_for(server, entity);
     if let Some(client) = server
         .state()
         .ecs()

server/src/events/trade.rs

@@ -55,7 +55,7 @@ fn notify_agent_prices(
 }
 
 /// Invoked when the trade UI is up, handling item changes, accepts, etc
-pub fn handle_process_trade_action(
+pub(super) fn handle_process_trade_action(
     server: &mut Server,
     entity: EcsEntity,
     trade_id: TradeId,
@@ -164,29 +164,34 @@ pub fn handle_process_trade_action(
     }
 }
 
-//Cancel all trades registered for a given UID.
-pub fn cancel_trade_for(server: &mut Server, entity: EcsEntity) {
-    if let Some(uid) = server.state().ecs().uid_from_entity(entity) {
-        let mut trades = server.state.ecs().write_resource::<Trades>();
+/// Cancel all trades registered for a given UID.
+///
+/// Note: This doesn't send any notification to the provided entity (only other
+/// participants in the trade). It is assumed that the supplied entity either no
+/// longer exists or is awareof this cancellation through other means (e.g.
+/// client getting ExitInGameSuccess message knows that it should clear any
+/// trades).
+pub(crate) fn cancel_trades_for(state: &mut common_state::State, entity: EcsEntity) {
+    let ecs = state.ecs();
+    if let Some(uid) = ecs.uid_from_entity(entity) {
+        let mut trades = ecs.write_resource::<Trades>();
 
         let active_trade = match trades.entity_trades.get(&uid) {
             Some(n) => *n,
-            None => {
-                return;
-            },
+            None => return,
         };
 
         let to_notify = trades.decline_trade(active_trade, uid);
-        to_notify
-            .and_then(|u| server.state.ecs().entity_from_uid(u.0))
-            .map(|e| {
-                server.notify_client(e, ServerGeneral::FinishedTrade(TradeResult::Declined));
-                notify_agent_simple(
-                    server.state.ecs().write_storage::<Agent>(),
-                    e,
-                    AgentEvent::FinishedTrade(TradeResult::Declined),
-                );
-            });
+        to_notify.and_then(|u| ecs.entity_from_uid(u.0)).map(|e| {
+            if let Some(c) = ecs.read_storage::<crate::Client>().get(e) {
+                c.send_fallible(ServerGeneral::FinishedTrade(TradeResult::Declined));
+            }
+            notify_agent_simple(
+                ecs.write_storage::<Agent>(),
+                e,
+                AgentEvent::FinishedTrade(TradeResult::Declined),
+            );
+        });
     }
 }
 

server/src/state_ext.rs

@@ -1,7 +1,7 @@
 use crate::{
     automod::AutoMod,
     client::Client,
-    events::update_map_markers,
+    events::{self, update_map_markers},
     persistence::PersistedComponents,
     pet::restore_pet,
     presence::{Presence, RepositionOnChunkLoad},
@@ -1000,10 +1000,14 @@ impl StateExt for State {
             );
         }
 
+        // Cancel extant trades
+        events::cancel_trades_for(self, entity);
+
         let (maybe_uid, maybe_pos) = (
             self.ecs().read_storage::<Uid>().get(entity).copied(),
             self.ecs().read_storage::<comp::Pos>().get(entity).copied(),
         );
+
         let res = self.ecs_mut().delete_entity(entity);
         if res.is_ok() {
             if let (Some(uid), Some(pos)) = (maybe_uid, maybe_pos) {