Fix deletion persistence when freshly delinked prims are removed

Previously, Scene.Inventory.DeRezObjects() forced the persistence of prims before deletion.
This is necessary so that freshly delinked prims can be deleted (otherwise they remain as parts of their old group and reappear on server restart).
However, DeRezObjects() deleted to user inventory, which is required by llDie() or direct region module unlink and deletion.
Therefore, forced persistence has been pushed down into Scene.UnlinkSceneObject() to be more general, this is still on the DeRezObjects() path.

Uncommented TestDelinkPersistence() since this now passes.
Tests required considerable elaboration of MockRegionDataPlugin to reflect underlying storing of parts.

10 files changed, 129 insertions, 57 deletions

diff --git a/OpenSim/Data/MySQL/MySQLLegacyRegionData.cs b/OpenSim/Data/MySQL/MySQLLegacyRegionData.cs
index 30253c3..a39e68d 100644
--- a/OpenSim/Data/MySQL/MySQLLegacyRegionData.cs
+++ b/OpenSim/Data/MySQL/MySQLLegacyRegionData.cs
@@ -247,6 +247,8 @@ namespace OpenSim.Data.MySQL
 
         public void RemoveObject(UUID obj, UUID regionUUID)
         {
+//            m_log.DebugFormat("[REGION DB]: Deleting scene object {0} from {1} in database", obj, regionUUID);
+            
             List<UUID> uuids = new List<UUID>();
 
             // Formerly, this used to check the region UUID.
diff --git a/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs b/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs
index 241cac0..916148b 100644
--- a/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs
+++ b/OpenSim/Region/Framework/Scenes/AsyncSceneObjectGroupDeleter.cs
@@ -111,11 +111,11 @@ namespace OpenSim.Region.Framework.Scenes
         
         private void InventoryRunDeleteTimer(object sender, ElapsedEventArgs e)
         {
-            m_log.Debug("[SCENE]: Starting send to inventory loop");
+            m_log.Debug("[ASYNC DELETER]: Starting send to inventory loop");
             
             while (InventoryDeQueueAndDelete())
             {
-                //m_log.Debug("[SCENE]: Sent item successfully to inventory, continuing...");
+                //m_log.Debug("[ASYNC DELETER]: Sent item successfully to inventory, continuing...");
             }
         }
 
@@ -137,7 +137,7 @@ namespace OpenSim.Region.Framework.Scenes
                         x = m_inventoryDeletes.Dequeue();
 
                         m_log.DebugFormat(
-                            "[SCENE]: Sending object to user's inventory, {0} item(s) remaining.", left);
+                            "[ASYNC DELETER]: Sending object to user's inventory, {0} item(s) remaining.", left);
                         
                         try
                         {
@@ -152,7 +152,8 @@ namespace OpenSim.Region.Framework.Scenes
                         }
                         catch (Exception e)
                         {
-                            m_log.DebugFormat("Exception background sending object: " + e);
+                            m_log.ErrorFormat(
+                                "[ASYNC DELETER]: Exception background sending object: {0}{1}", e.Message, e.StackTrace);
                         }
                         
                         return true;
@@ -164,12 +165,16 @@ namespace OpenSim.Region.Framework.Scenes
                 // We can't put the object group details in here since the root part may have disappeared (which is where these sit).
                 // FIXME: This needs to be fixed.
                 m_log.ErrorFormat(
-                    "[SCENE]: Queued sending of scene object to agent {0} {1} failed: {2}",
-                    (x != null ? x.remoteClient.Name : "unavailable"), (x != null ? x.remoteClient.AgentId.ToString() : "unavailable"), e.ToString());
+                    "[ASYNC DELETER]: Queued sending of scene object to agent {0} {1} failed: {2} {3}",
+                    (x != null ? x.remoteClient.Name : "unavailable"), 
+                    (x != null ? x.remoteClient.AgentId.ToString() : "unavailable"), 
+                    e.Message, 
+                    e.StackTrace);
             }
 
-            m_log.Debug("[SCENE]: No objects left in inventory send queue.");
+            m_log.Debug("[ASYNC DELETER]: No objects left in inventory send queue.");
+            
             return false;
         }
     }
-}
+}
\ No newline at end of file
diff --git a/OpenSim/Region/Framework/Scenes/EntityBase.cs b/OpenSim/Region/Framework/Scenes/EntityBase.cs
index e183f9d..6fd38e5 100644
--- a/OpenSim/Region/Framework/Scenes/EntityBase.cs
+++ b/OpenSim/Region/Framework/Scenes/EntityBase.cs
@@ -69,6 +69,7 @@ namespace OpenSim.Region.Framework.Scenes
         public bool IsDeleted
         {
             get { return m_isDeleted; }
+            set { m_isDeleted = value; }
         }
         protected bool m_isDeleted;
 
diff --git a/OpenSim/Region/Framework/Scenes/Scene.Inventory.cs b/OpenSim/Region/Framework/Scenes/Scene.Inventory.cs
index 4e871d9..c49386a 100644
--- a/OpenSim/Region/Framework/Scenes/Scene.Inventory.cs
+++ b/OpenSim/Region/Framework/Scenes/Scene.Inventory.cs
@@ -1720,7 +1720,7 @@ namespace OpenSim.Region.Framework.Scenes
         public virtual void DeRezObject(IClientAPI remoteClient, uint localID,
                 UUID groupID, DeRezAction action, UUID destinationID)
         {
-            DeRezObjects(remoteClient, new List<uint>() { localID} , groupID, action, destinationID);
+            DeRezObjects(remoteClient, new List<uint>() { localID }, groupID, action, destinationID);
         }
 
         public virtual void DeRezObjects(IClientAPI remoteClient, List<uint> localIDs,
@@ -1757,11 +1757,6 @@ namespace OpenSim.Region.Framework.Scenes
                 deleteIDs.Add(localID);
                 deleteGroups.Add(grp);
 
-                // Force a database backup/update on this SceneObjectGroup
-                // So that we know the database is upto date,
-                // for when deleting the object from it
-                ForceSceneObjectBackup(grp);
-
                 if (remoteClient == null)
                 {
                     // Autoreturn has a null client. Nothing else does. So
diff --git a/OpenSim/Region/Framework/Scenes/Scene.cs b/OpenSim/Region/Framework/Scenes/Scene.cs
index 183310d..82e7d76 100644
--- a/OpenSim/Region/Framework/Scenes/Scene.cs
+++ b/OpenSim/Region/Framework/Scenes/Scene.cs
@@ -2092,7 +2092,7 @@ namespace OpenSim.Region.Framework.Scenes
 //                rootPart.PhysActor = null;
 //            }
 
-            if (UnlinkSceneObject(group.UUID, false))
+            if (UnlinkSceneObject(group, false))
             {
                 EventManager.TriggerObjectBeingRemovedFromScene(group);
                 EventManager.TriggerParcelPrimCountTainted();
@@ -2107,18 +2107,25 @@ namespace OpenSim.Region.Framework.Scenes
         /// Unlink the given object from the scene.  Unlike delete, this just removes the record of the object - the
         /// object itself is not destroyed.
         /// </summary>
-        /// <param name="uuid">Id of object.</param>
+        /// <param name="so">The scene object.</param>
+        /// <param name="softDelete">If true, only deletes from scene, but keeps the object in the database.</param>
         /// <returns>true if the object was in the scene, false if it was not</returns>
-        /// <param name="softDelete">If true, only deletes from scene, but keeps object in database.</param>
-        public bool UnlinkSceneObject(UUID uuid, bool softDelete)
+        public bool UnlinkSceneObject(SceneObjectGroup so, bool softDelete)
         {
-            if (m_sceneGraph.DeleteSceneObject(uuid, softDelete))
+            if (m_sceneGraph.DeleteSceneObject(so.UUID, softDelete))
             {
                 if (!softDelete)
                 {
-                    m_storageManager.DataStore.RemoveObject(uuid,
-                                                            m_regInfo.RegionID);
+                    // Force a database update so that the scene object group ID is accurate.  It's possible that the
+                    // group has recently been delinked from another group but that this change has not been persisted
+                    // to the DB.
+                    ForceSceneObjectBackup(so);
+                    so.DetachFromBackup();                                
+                    m_storageManager.DataStore.RemoveObject(so.UUID, m_regInfo.RegionID);
                 }
+                                    
+                // We need to keep track of this state in case this group is still queued for further backup.
+                so.IsDeleted = true;                
 
                 return true;
             }
@@ -2149,7 +2156,7 @@ namespace OpenSim.Region.Framework.Scenes
                 }
                 catch (Exception)
                 {
-                    m_log.Warn("[DATABASE]: exception when trying to remove the prim that crossed the border.");
+                    m_log.Warn("[SCENE]: exception when trying to remove the prim that crossed the border.");
                 }
                 return;
             }
@@ -2166,7 +2173,7 @@ namespace OpenSim.Region.Framework.Scenes
                 }
                 catch (Exception)
                 {
-                    m_log.Warn("[DATABASE]: exception when trying to return the prim that crossed the border.");
+                    m_log.Warn("[SCENE]: exception when trying to return the prim that crossed the border.");
                 }
                 return;
             }
diff --git a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
index b8b3db5..09e3e0e 100644
--- a/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
+++ b/OpenSim/Region/Framework/Scenes/SceneObjectGroup.cs
@@ -1226,16 +1226,16 @@ namespace OpenSim.Region.Framework.Scenes
         }
 
         /// <summary>
-        /// Delete this group from its scene and tell all the scene presences about that deletion.
+        /// Delete this group from its scene.  
         /// </summary>
-        /// <param name="silent">Broadcast deletions to all clients.</param>
+        /// 
+        /// This only handles the in-world consequences of deletion (e.g. any avatars sitting on it are forcibly stood
+        /// up and all avatars receive notification of its removal.  Removal of the scene object from database backup
+        /// must be handled by the caller.
+        /// 
+        /// <param name="silent">If true then deletion is not broadcast to clients</param>
         public void DeleteGroup(bool silent)
         {
-            // We need to keep track of this state in case this group is still queued for backup.
-            m_isDeleted = true;
-
-            DetachFromBackup();
-
             lock (m_parts)
             {
                 foreach (SceneObjectPart part in m_parts.Values)
@@ -1381,10 +1381,18 @@ namespace OpenSim.Region.Framework.Scenes
         public virtual void ProcessBackup(IRegionDataStore datastore, bool forcedBackup)
         {
             if (!m_isBackedUp)
+            {
+//                m_log.DebugFormat(
+//                    "[WATER WARS]: Ignoring backup of {0} {1} since object is not marked to be backed up", Name, UUID);                
                 return;
+            }
 
             if (IsDeleted || UUID == UUID.Zero)
+            {
+//                m_log.DebugFormat(
+//                    "[WATER WARS]: Ignoring backup of {0} {1} since object is marked as already deleted", Name, UUID);
                 return;
+            }
 
             // Since this is the top of the section of call stack for backing up a particular scene object, don't let
             // any exception propogate upwards.            
@@ -1420,7 +1428,7 @@ namespace OpenSim.Region.Framework.Scenes
                 if (HasGroupChanged)
                 {
                     // don't backup while it's selected or you're asking for changes mid stream.
-                    if ((isTimeToPersist()) || (forcedBackup))
+                    if (isTimeToPersist() || forcedBackup)
                     {
                         m_log.DebugFormat(
                             "[SCENE]: Storing {0}, {1} in {2}",
@@ -1443,19 +1451,19 @@ namespace OpenSim.Region.Framework.Scenes
 
                         backup_group = null;
                     }
-    //                else
-    //                {
-    //                    m_log.DebugFormat(
-    //                        "[SCENE]: Did not update persistence of object {0} {1}, selected = {2}",
-    //                        Name, UUID, IsSelected);
-    //                }
+//                    else
+//                    {
+//                        m_log.DebugFormat(
+//                            "[SCENE]: Did not update persistence of object {0} {1}, selected = {2}",
+//                            Name, UUID, IsSelected);
+//                    }
                 }
             }
             catch (Exception e)
             {
                 m_log.ErrorFormat(
-                    "[SCENE]: Storing of {0}, {1} in {2} failed with exception {3}\n\t{4}", 
-                    Name, UUID, m_scene.RegionInfo.RegionName, e, e.StackTrace);
+                    "[SCENE]: Storing of {0}, {1} in {2} failed with exception {3}{4}", 
+                    Name, UUID, m_scene.RegionInfo.RegionName, e.Message, e.StackTrace);
             }
         }
 
@@ -2245,7 +2253,7 @@ namespace OpenSim.Region.Framework.Scenes
                 }
             }
 
-            m_scene.UnlinkSceneObject(objectGroup.UUID, true);
+            m_scene.UnlinkSceneObject(objectGroup, true);
             objectGroup.m_isDeleted = true;
             
             lock (objectGroup.m_parts)
diff --git a/OpenSim/Region/Framework/Scenes/Tests/SceneObjectLinkingTests.cs b/OpenSim/Region/Framework/Scenes/Tests/SceneObjectLinkingTests.cs
index 93409fa..62761fb 100644
--- a/OpenSim/Region/Framework/Scenes/Tests/SceneObjectLinkingTests.cs
+++ b/OpenSim/Region/Framework/Scenes/Tests/SceneObjectLinkingTests.cs
@@ -303,11 +303,11 @@ namespace OpenSim.Region.Framework.Scenes.Tests
         /// <summary>
         /// Test that a delink of a previously linked object is correctly persisted to the database
         /// </summary>
-        //[Test]
+        [Test]
         public void TestDelinkPersistence()
         {
             TestHelper.InMethod();
-            //log4net.Config.XmlConfigurator.Configure();
+            log4net.Config.XmlConfigurator.Configure();
             
             TestScene scene = SceneSetupHelpers.SetupScene();            
             
diff --git a/OpenSim/Tests/Common/Mock/MockRegionDataPlugin.cs b/OpenSim/Tests/Common/Mock/MockRegionDataPlugin.cs
index 1c139c5..2a055cc 100644
--- a/OpenSim/Tests/Common/Mock/MockRegionDataPlugin.cs
+++ b/OpenSim/Tests/Common/Mock/MockRegionDataPlugin.cs
@@ -44,7 +44,7 @@ namespace OpenSim.Data.Null
         private static readonly ILog m_log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);
         
         protected Dictionary<UUID, RegionSettings> m_regionSettings = new Dictionary<UUID, RegionSettings>();
-        protected Dictionary<UUID, SceneObjectGroup> m_sceneObjects = new Dictionary<UUID, SceneObjectGroup>();
+        protected Dictionary<UUID, SceneObjectPart> m_sceneObjectParts = new Dictionary<UUID, SceneObjectPart>();
         protected Dictionary<UUID, ICollection<TaskInventoryItem>> m_primItems 
             = new Dictionary<UUID, ICollection<TaskInventoryItem>>();
         protected Dictionary<UUID, double[,]> m_terrains = new Dictionary<UUID, double[,]>();
@@ -85,18 +85,33 @@ namespace OpenSim.Data.Null
 
         public void StoreObject(SceneObjectGroup obj, UUID regionUUID)
         {
-            m_log.DebugFormat(
-                "[MOCK REGION DATA PLUGIN]: Storing object {0} {1} in {2}", obj.Name, obj.UUID, regionUUID);
-            m_sceneObjects[obj.UUID] = obj;
+            // We can't simply store groups here because on delinking, OpenSim will not update the original group
+            // directly.  Rather, the newly delinked parts will be updated to be in their own scene object group
+            // Therefore, we need to store parts rather than groups.
+            foreach (SceneObjectPart prim in obj.Children.Values)
+            {
+                m_log.DebugFormat(
+                    "[MOCK REGION DATA PLUGIN]: Storing part {0} {1} in object {2} {3} in region {4}", 
+                    prim.Name, prim.UUID, obj.Name, obj.UUID, regionUUID);
+                            
+                m_sceneObjectParts[prim.UUID] = prim;
+            }
         }
 
         public void RemoveObject(UUID obj, UUID regionUUID)
-        {
-            m_log.DebugFormat(
-                "[MOCK REGION DATA PLUGIN]: Removing object {0} from {1}", obj, regionUUID);
-            
-            if (m_sceneObjects.ContainsKey(obj))
-                m_sceneObjects.Remove(obj);
+        {           
+            // All parts belonging to the object with the uuid are removed.
+            List<SceneObjectPart> parts = new List<SceneObjectPart>(m_sceneObjectParts.Values);
+            foreach (SceneObjectPart part in parts)
+            {
+                if (part.ParentGroup.UUID == obj)
+                {
+                    m_log.DebugFormat(
+                        "[MOCK REGION DATA PLUGIN]: Removing part {0} {1} as part of object {2} from {3}", 
+                        part.Name, part.UUID, obj, regionUUID);                    
+                    m_sceneObjectParts.Remove(part.UUID);
+                }
+            }
         }
 
         // see IRegionDatastore
@@ -107,10 +122,49 @@ namespace OpenSim.Data.Null
 
         public List<SceneObjectGroup> LoadObjects(UUID regionUUID)
         {
-            m_log.DebugFormat(
-                "[MOCK REGION DATA PLUGIN]: Loading objects from {0}", regionUUID);
+            Dictionary<UUID, SceneObjectGroup> objects = new Dictionary<UUID, SceneObjectGroup>();
+            
+            // Create all of the SOGs from the root prims first
+            foreach (SceneObjectPart prim in m_sceneObjectParts.Values)
+            {
+                if (prim.IsRoot)
+                {
+                    m_log.DebugFormat(
+                        "[MOCK REGION DATA PLUGIN]: Loading root part {0} {1} in {2}", prim.Name, prim.UUID, regionUUID);                               
+                    objects[prim.UUID] = new SceneObjectGroup(prim);
+                }
+            }
+
+            // Add all of the children objects to the SOGs
+            foreach (SceneObjectPart prim in m_sceneObjectParts.Values)
+            {
+                SceneObjectGroup sog;
+                if (prim.UUID != prim.ParentUUID)
+                {
+                    if (objects.TryGetValue(prim.ParentUUID, out sog))
+                    {
+                        int originalLinkNum = prim.LinkNum;
+
+                        sog.AddPart(prim);
+
+                        // SceneObjectGroup.AddPart() tries to be smart and automatically set the LinkNum.
+                        // We override that here
+                        if (originalLinkNum != 0)
+                            prim.LinkNum = originalLinkNum;
+                    }
+                    else
+                    {
+                        m_log.WarnFormat(
+                            "[MOCK REGION DATA PLUGIN]: Database contains an orphan child prim {0} {1} in region {2} pointing to missing parent {3}.  This prim will not be loaded.",
+                            prim.Name, prim.UUID, regionUUID, prim.ParentUUID);
+                    }
+                }
+            }
+            
+            // TODO: Load items.  This is assymetric - we store items as a separate method but don't retrieve them that
+            // way!
             
-            return new List<SceneObjectGroup>(m_sceneObjects.Values);
+            return new List<SceneObjectGroup>(objects.Values);
         }
 
         public void StoreTerrain(double[,] ter, UUID regionID)
diff --git a/OpenSim/Tests/Common/Mock/TestInventoryDataPlugin.cs b/OpenSim/Tests/Common/Mock/TestInventoryDataPlugin.cs
index b70b47d..b47ad5d 100644
--- a/OpenSim/Tests/Common/Mock/TestInventoryDataPlugin.cs
+++ b/OpenSim/Tests/Common/Mock/TestInventoryDataPlugin.cs
@@ -42,7 +42,7 @@ namespace OpenSim.Tests.Common.Mock
     /// </summary>
     public class TestInventoryDataPlugin : IInventoryDataPlugin
     {
-        private static readonly ILog m_log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);
+//        private static readonly ILog m_log = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);
         
         /// <value>
         /// Inventory folders
diff --git a/OpenSim/Tests/Common/Setup/UserInventoryTestUtils.cs b/OpenSim/Tests/Common/Setup/UserInventoryTestUtils.cs
index c57363a..915af7e 100644
--- a/OpenSim/Tests/Common/Setup/UserInventoryTestUtils.cs
+++ b/OpenSim/Tests/Common/Setup/UserInventoryTestUtils.cs
@@ -52,7 +52,7 @@ namespace OpenSim.Tests.Common
             InventoryFolderBase objsFolder = scene.InventoryService.GetFolderForType(userId, AssetType.Object);
             
             item.Folder = objsFolder.ID;
-            scene.AddInventoryItem(userId, item);            
+            scene.AddInventoryItem(item);            
             
             return item;
         }