From ec51c14a41cdfb0e4b68db7cdbed271f0471a4b7 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:16:10 -0600 Subject: [PATCH] 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 | 75 ----------------------------- plugin/src/ServeSession.lua | 1 + src/snapshot/tree.rs | 23 +++++++++ src/web/api.rs | 11 +++-- tests/tests/scripts_only_pruning.rs | 2 +- 5 files changed, 33 insertions(+), 79 deletions(-) diff --git a/TODO.txt b/TODO.txt index 0e3c593f..0db1c29a 100644 --- a/TODO.txt +++ b/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 - 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 - 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,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,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 - diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 198aaeee..c253849c 100644 --- a/plugin/src/ServeSession.lua +++ b/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() diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 5626cce1..169b94a4 100644 --- a/src/snapshot/tree.rs +++ b/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()); diff --git a/src/web/api.rs b/src/web/api.rs index 06f63e24..33969959 100644 --- a/src/web/api.rs +++ b/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>( diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs index 0cc668c6..f4a93158 100644 --- a/tests/tests/scripts_only_pruning.rs +++ b/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"