Enhance error handling and improve script tracking in RojoTree and ServeSession

- Added a test to verify the functionality of script tracking updates in RojoTree, ensuring proper addition and removal of script references.
- Improved error handling in ServeSession by clearing temporary script IDs upon disconnection to prevent potential memory leaks.
- Updated API logic to filter out empty properties and children during instance addition, enhancing the integrity of the synchronization process.

These changes aim to optimize script management and enhance the robustness of the session handling and tree operations.

TODO.txt

@@ -355,78 +355,3 @@ this needs to be slugified when the name exactly matches init, ignoring case [fo
 
 [one shot mode change]
 new changes that come in / out during the review menu on the plugin is really disorienting.
-
-[error]
-[INFO  librojo::change_processor] Two-way sync: Removing instance Ref(Some(11172145805454499324864790020248300740)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VisalizationInstancesV2\Visualize\VisualizeV2\init.luau)
-[INFO  librojo::change_processor] Two-way sync: Removing instance Ref(Some(250896002424020262431971720464633879491)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura\BillboardGui\Script.client.luau)
-[INFO  librojo::change_processor] Two-way sync: Removing instance Ref(Some(180523706273884596638310961780081579965)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX)
-[INFO  librojo::change_processor] Two-way sync: Removing instance Ref(Some(255012798697385670746832788816389180140)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura)
-[INFO  librojo::change_processor] Two-way sync: Removing instance Ref(Some(327927689482872520713513681823365738785)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup)
-[INFO  librojo::change_processor] Two-way sync: Writing Source to d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\Disco\init.luau
-[ERROR atlas] Rojo crashed! You are running Rojo 8.4.1.
-[ERROR atlas] This is probably a Rojo bug.
-[ERROR atlas] 
-[ERROR atlas] Please consider filing an issue: https://github.com/UserGeneratedLLC/rojo/issues
-[ERROR atlas] 
-[ERROR atlas] Details: called `Option::unwrap()` on a `None` value
-[ERROR atlas] in file src\snapshot\tree.rs on line 471
-   0: backtrace::backtrace::win64::trace
-             at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\win64.rs:85
-      backtrace::backtrace::trace_unsynchronized
-             at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:66
-      backtrace::backtrace::trace<backtrace::capture::impl$4::create::closure_env$0>
-             at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:53
-   1: backtrace::capture::Backtrace::create
-             at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:294
-   2: backtrace::capture::Backtrace::new
-             at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:259
-   3: atlas::main::closure$0
-             at D:\UserGenerated\rojo\src\main.rs:48
-   4: std::panicking::panic_with_hook
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:833
-   5: std::panicking::panic_handler::closure$0
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:691
-   6: std::sys::backtrace::__rust_end_short_backtrace<std::panicking::panic_handler::closure_env$0,never$>
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\backtrace.rs:176
-   7: std::panicking::panic_handler
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:689
-   8: core::panicking::panic_fmt
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:80
-   9: core::panicking::panic
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:150
-  10: core::option::unwrap_failed
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\option.rs:2175
-  11: librojo::snapshot::tree::RojoTree::remove_metadata
-             at D:\UserGenerated\rojo\src\snapshot\tree.rs:471
-      librojo::snapshot::tree::RojoTree::remove
-             at D:\UserGenerated\rojo\src\snapshot\tree.rs:165
-  12: librojo::snapshot::patch_apply::apply_remove_instance
-             at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:208
-      librojo::snapshot::patch_apply::apply_patch_set
-             at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:32
-  13: librojo::change_processor::JobThreadContext::handle_tree_event
-             at D:\UserGenerated\rojo\src\change_processor.rs:1858
-  14: librojo::change_processor::impl$1::start::closure$0
-             at D:\UserGenerated\rojo\src\change_processor.rs:174
-      std::sys::backtrace::__rust_begin_short_backtrace<librojo::change_processor::impl$1::start::closure_env$0,enum2$<core::result::Result<tuple$<>,crossbeam_channel::err::RecvError> > >
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\backtrace.rs:160
-  15: std::thread::lifecycle::spawn_unchecked::closure$1::closure$0
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:92
-      core::panic::unwind_safe::impl$25::call_once
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\panic\unwind_safe.rs:274
-      std::panicking::catch_unwind::do_call
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:581
-      std::panicking::catch_unwind
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:544
-      std::panic::catch_unwind
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:359
-      std::thread::lifecycle::spawn_unchecked::closure$1
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:90
-      core::ops::function::FnOnce::call_once<std::thread::lifecycle::spawn_unchecked::closure_env$1<librojo::change_processor::impl$1::start::closure_env$0,enum2$<core::result::Result<tuple$<>,crossbeam_channel::err::RecvError> > >,tuple$<> >
-             at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250
-  16: std::sys::thread::windows::impl$0::new::thread_start
-             at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\thread\windows.rs:59
-  17: BaseThreadInitThunk
-  18: RtlUserThreadStart
-Process exited with code 1
-

plugin/src/ServeSession.lua

@@ -1020,6 +1020,7 @@ end
 function ServeSession:__stopInternal(err)
 	self:__setStatus(Status.Disconnected, err)
 	self.__apiContext:disconnect()
+	table.clear(self.__instanceMap.scriptsOnlyTempIds)
 	self.__instanceMap:stop()
 	self.__changeBatcher:stop()
 

src/snapshot/tree.rs

@@ -625,6 +625,29 @@ mod test {
 
     use super::RojoTree;
 
+    #[test]
+    fn update_script_tracking_adds_and_removes() {
+        let snapshot = InstanceSnapshot::new().name("Root").class_name("DataModel");
+        let child = InstanceSnapshot::new()
+            .name("TestFolder")
+            .class_name("Folder");
+
+        let mut tree = RojoTree::new(snapshot);
+        let root = tree.get_root_id();
+        let folder_id = tree.insert_instance(root, child);
+
+        assert!(!tree.script_refs().contains(&folder_id));
+
+        tree.update_script_tracking(folder_id, "Folder", "ModuleScript");
+        assert!(tree.script_refs().contains(&folder_id));
+
+        tree.update_script_tracking(folder_id, "ModuleScript", "Script");
+        assert!(tree.script_refs().contains(&folder_id));
+
+        tree.update_script_tracking(folder_id, "Script", "Folder");
+        assert!(!tree.script_refs().contains(&folder_id));
+    }
+
     #[test]
     fn swap_duped_specified_ids() {
         let custom_ref = RojoRef::new("MyCoolRef".into());

src/web/api.rs

@@ -23,7 +23,8 @@ use rbx_dom_weak::{
 use crate::{
     serve_session::ServeSession,
     snapshot::{
-        InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet, PatchUpdate,
+        is_script_class, InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet,
+        PatchUpdate,
     },
     syncback::{slugify_name, VISIBLE_SERVICES},
     web::{
@@ -559,6 +560,9 @@ impl ApiService {
                         if temp_to_real.contains_key(guid) {
                             continue;
                         }
+                        if !added.properties.is_empty() || !added.children.is_empty() {
+                            continue;
+                        }
                         let parent = added.parent.unwrap_or(Ref::none());
                         let real_parent = temp_to_real.get(&parent).copied().unwrap_or(parent);
                         if tree.get_instance(real_parent).is_none() {
@@ -593,6 +597,9 @@ impl ApiService {
                     for guid in temp_to_real.keys() {
                         updated_in_place.insert(*guid);
                     }
+                    request
+                        .added
+                        .retain(|guid, _| !temp_to_real.contains_key(guid));
                 }
             }
 
@@ -3604,8 +3611,6 @@ fn parent_requirements(class: &str) -> Option<&str> {
     })
 }
 
-use crate::snapshot::is_script_class;
-
 /// Filters a SubscribeMessage to only include scripts and their ancestors.
 /// Non-script ancestors get ignoreUnknownInstances: true.
 fn filter_subscribe_message_for_scripts<'a>(

tests/tests/scripts_only_pruning.rs

@@ -73,7 +73,7 @@ fn scripts_only_read_ancestors_have_no_properties() {
         let root_id = info.root_instance_id;
         let read_response = session.get_api_read(root_id).unwrap();
 
-        for (_, inst) in &read_response.instances {
+        for inst in read_response.instances.values() {
             let is_script = matches!(
                 inst.class_name.as_str(),
                 "Script" | "LocalScript" | "ModuleScript"