From f3b0b0027ed41939e3eaeab760777ec4f085aac3 Mon Sep 17 00:00:00 2001
From: boatbomber <zack@boatbomber.com>
Date: Wed, 31 Jan 2024 17:07:01 -0800
Subject: [PATCH] Catch more sync failures (#845)

- Catch removal failures
- Catch name change failures
- Don't remove IDs for instances if they weren't actually destroyed
---
 plugin/src/InstanceMap.lua           | 22 ++++++++++++----------
 plugin/src/PatchTree.lua             | 11 ++++++++---
 plugin/src/Reconciler/applyPatch.lua | 21 ++++++++++++++++-----
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua
index 91a1d4cdc..b035d423c 100644
--- a/plugin/src/InstanceMap.lua
+++ b/plugin/src/InstanceMap.lua
@@ -113,27 +113,29 @@ end
 function InstanceMap:destroyInstance(instance)
 	local id = self.fromInstances[instance]
 
+	local descendants = instance:GetDescendants()
+	instance:Destroy()
+
+	-- After the instance is successfully destroyed,
+	-- we can remove all the id mappings
+
 	if id ~= nil then
 		self:removeId(id)
 	end
 
-	for _, descendantInstance in ipairs(instance:GetDescendants()) do
+	for _, descendantInstance in descendants do
 		self:removeInstance(descendantInstance)
 	end
-
-	instance:Destroy()
 end
 
 function InstanceMap:destroyId(id)
 	local instance = self.fromIds[id]
-	self:removeId(id)
-
 	if instance ~= nil then
-		for _, descendantInstance in ipairs(instance:GetDescendants()) do
-			self:removeInstance(descendantInstance)
-		end
-
-		instance:Destroy()
+		self:destroyInstance(instance)
+	else
+		-- There is no instance with this id, so we can just remove the id
+		-- without worrying about instance destruction
+		self:removeId(id)
 	end
 end
 
diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua
index d68bdcd70..e469e2112 100644
--- a/plugin/src/PatchTree.lua
+++ b/plugin/src/PatchTree.lua
@@ -437,8 +437,13 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch)
 			continue
 		end
 		for _, change in node.changeList do
-			if not failedChange.changedProperties[change[1]] then
-				-- This change didn't fail
+			local property = change[1]
+			local propertyFailedToApply = if property == "Name"
+				then failedChange.changedName ~= nil -- Name is not in changedProperties, so it needs a special case
+				else failedChange.changedProperties[property] ~= nil
+
+			if not propertyFailedToApply then
+				-- This change didn't fail, no need to mark
 				continue
 			end
 			if change[4] == nil then
@@ -446,7 +451,7 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch)
 			else
 				change[4].isWarning = true
 			end
-			Log.trace("  Marked property as warning: {}.{}", node.name, change[1])
+			Log.trace("  Marked property as warning: {}.{}", node.name, property)
 		end
 	end
 	for failedAdditionId in unappliedPatch.added do
diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua
index 4faa73105..a66be3a21 100644
--- a/plugin/src/Reconciler/applyPatch.lua
+++ b/plugin/src/Reconciler/applyPatch.lua
@@ -25,10 +25,15 @@ local function applyPatch(instanceMap, patch)
 	local unappliedPatch = PatchSet.newEmpty()
 
 	for _, removedIdOrInstance in ipairs(patch.removed) do
-		if Types.RbxId(removedIdOrInstance) then
-			instanceMap:destroyId(removedIdOrInstance)
-		else
-			instanceMap:destroyInstance(removedIdOrInstance)
+		local ok = pcall(function()
+			if Types.RbxId(removedIdOrInstance) then
+				instanceMap:destroyId(removedIdOrInstance)
+			else
+				instanceMap:destroyInstance(removedIdOrInstance)
+			end
+		end)
+		if not ok then
+			table.insert(unappliedPatch.removed, removedIdOrInstance)
 		end
 	end
 
@@ -170,7 +175,13 @@ local function applyPatch(instanceMap, patch)
 		end
 
 		if update.changedName ~= nil then
-			instance.Name = update.changedName
+			local ok = pcall(function()
+				instance.Name = update.changedName
+			end)
+			if not ok then
+				unappliedUpdate.changedName = update.changedName
+				partiallyApplied = true
+			end
 		end
 
 		if update.changedMetadata ~= nil then