diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript01.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript01.luau new file mode 100644 index 00000000..06e0c946 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript01.luau @@ -0,0 +1,2 @@ +local BatchScript01 = {} +return BatchScript01 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript02.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript02.luau new file mode 100644 index 00000000..909a59bd --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript02.luau @@ -0,0 +1,2 @@ +local BatchScript02 = {} +return BatchScript02 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript03.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript03.luau new file mode 100644 index 00000000..94d767ad --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript03.luau @@ -0,0 +1,2 @@ +local BatchScript03 = {} +return BatchScript03 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript04.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript04.luau new file mode 100644 index 00000000..77f95530 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript04.luau @@ -0,0 +1,2 @@ +local BatchScript04 = {} +return BatchScript04 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript05.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript05.luau new file mode 100644 index 00000000..e9cf8932 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript05.luau @@ -0,0 +1,2 @@ +local BatchScript05 = {} +return BatchScript05 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript06.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript06.luau new file mode 100644 index 00000000..96e56c10 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript06.luau @@ -0,0 +1,2 @@ +local BatchScript06 = {} +return BatchScript06 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript07.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript07.luau new file mode 100644 index 00000000..89fc6ec8 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript07.luau @@ -0,0 +1,2 @@ +local BatchScript07 = {} +return BatchScript07 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript08.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript08.luau new file mode 100644 index 00000000..026fa1f2 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript08.luau @@ -0,0 +1,2 @@ +local BatchScript08 = {} +return BatchScript08 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript09.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript09.luau new file mode 100644 index 00000000..57ccd37e --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript09.luau @@ -0,0 +1,2 @@ +local BatchScript09 = {} +return BatchScript09 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript10.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript10.luau new file mode 100644 index 00000000..9a2dd32d --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript10.luau @@ -0,0 +1,2 @@ +local BatchScript10 = {} +return BatchScript10 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript11.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript11.luau new file mode 100644 index 00000000..8ea7033c --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript11.luau @@ -0,0 +1,2 @@ +local BatchScript11 = {} +return BatchScript11 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript12.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript12.luau new file mode 100644 index 00000000..0d25ddc1 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript12.luau @@ -0,0 +1,2 @@ +local BatchScript12 = {} +return BatchScript12 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript13.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript13.luau new file mode 100644 index 00000000..1fb0d90e --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript13.luau @@ -0,0 +1,2 @@ +local BatchScript13 = {} +return BatchScript13 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript14.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript14.luau new file mode 100644 index 00000000..de006942 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript14.luau @@ -0,0 +1,2 @@ +local BatchScript14 = {} +return BatchScript14 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript15.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript15.luau new file mode 100644 index 00000000..06a8adea --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript15.luau @@ -0,0 +1,2 @@ +local BatchScript15 = {} +return BatchScript15 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript16.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript16.luau new file mode 100644 index 00000000..9d7a9548 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript16.luau @@ -0,0 +1,2 @@ +local BatchScript16 = {} +return BatchScript16 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript17.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript17.luau new file mode 100644 index 00000000..203a88d4 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript17.luau @@ -0,0 +1,2 @@ +local BatchScript17 = {} +return BatchScript17 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript18.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript18.luau new file mode 100644 index 00000000..687ff57e --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript18.luau @@ -0,0 +1,2 @@ +local BatchScript18 = {} +return BatchScript18 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript19.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript19.luau new file mode 100644 index 00000000..c7ba3de9 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript19.luau @@ -0,0 +1,2 @@ +local BatchScript19 = {} +return BatchScript19 diff --git a/rojo-test/serve-tests/git_sync_defaults/src/BatchScript20.luau b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript20.luau new file mode 100644 index 00000000..f4a27c68 --- /dev/null +++ b/rojo-test/serve-tests/git_sync_defaults/src/BatchScript20.luau @@ -0,0 +1,2 @@ +local BatchScript20 = {} +return BatchScript20 diff --git a/src/change_processor.rs b/src/change_processor.rs index 4a27b907..c04fc14d 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1013,6 +1013,7 @@ impl JobThreadContext { removed_instances: patch_set.removed_instances, updated_instances: Vec::new(), stage_ids: HashSet::new(), + stage_paths: Vec::new(), }; let applied = apply_patch_set(&mut tree, structural_patch); @@ -1027,7 +1028,7 @@ impl JobThreadContext { ); } - fn handle_tree_event(&self, patch_set: PatchSet) { + fn handle_tree_event(&self, mut patch_set: PatchSet) { // Log incoming patch summary at debug level log::debug!( "Processing client patch: {} removed, {} added, {} updated", @@ -1308,6 +1309,10 @@ impl JobThreadContext { // Applied after the PatchSet to keep metadata in sync. let mut metadata_updates: Vec<(Ref, PathBuf)> = Vec::new(); + // Paths to stage via git add after all writes complete. + // Starts with pre-resolved paths from api.rs, then Source writes are appended. + let mut pending_stage_paths = std::mem::take(&mut patch_set.stage_paths); + for update in &patch_set.updated_instances { let id = update.id; @@ -1821,12 +1826,7 @@ impl JobThreadContext { err ); } else if patch_set.stage_ids.contains(&id) { - if let Some(ref repo_root) = self.git_repo_root { - crate::git::git_add( - repo_root, - std::slice::from_ref(write_path), - ); - } + pending_stage_paths.push(write_path.clone()); } } else { log::warn!("Cannot change Source to non-string value."); @@ -1898,6 +1898,14 @@ impl JobThreadContext { } } + // Consolidated git staging: one git_add call for all paths + // (pre-resolved from api.rs + Source writes from this function). + if !pending_stage_paths.is_empty() { + if let Some(ref repo_root) = self.git_repo_root { + crate::git::git_add(repo_root, &pending_stage_paths); + } + } + applied }; diff --git a/src/snapshot/patch.rs b/src/snapshot/patch.rs index 7a4195ea..235ed12d 100644 --- a/src/snapshot/patch.rs +++ b/src/snapshot/patch.rs @@ -24,6 +24,12 @@ pub struct PatchSet { /// Populated from WriteRequest.stage_ids, consumed by change_processor. #[serde(default, skip)] pub stage_ids: HashSet, + /// Pre-resolved filesystem paths to stage via git add. + /// Populated by api.rs (for files already on disk), consumed by change_processor + /// together with any Source write paths from stage_ids. + /// All staging is consolidated into a single git_add call to avoid index.lock races. + #[serde(default, skip)] + pub stage_paths: Vec, } impl PatchSet { @@ -33,6 +39,7 @@ impl PatchSet { added_instances: Vec::new(), updated_instances: Vec::new(), stage_ids: HashSet::new(), + stage_paths: Vec::new(), } } } diff --git a/src/web/api.rs b/src/web/api.rs index 0f1ed75a..380873f5 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -764,15 +764,10 @@ impl ApiService { added_instances, updated_instances, stage_ids, + stage_paths: paths_to_stage, }) .unwrap(); - if let Some(repo_root) = self.serve_session.git_repo_root() { - if !paths_to_stage.is_empty() { - crate::git::git_add(repo_root, &paths_to_stage); - } - } - msgpack_ok(WriteResponse { session_id }) } diff --git a/tests/tests/git_sync_defaults.rs b/tests/tests/git_sync_defaults.rs index c5f2e92f..fd37c11e 100644 --- a/tests/tests/git_sync_defaults.rs +++ b/tests/tests/git_sync_defaults.rs @@ -690,3 +690,212 @@ fn init_style_script_staging_targets_init_file() { ); } } + +// =========================================================================== +// Batch staging: many files in a single write request +// =========================================================================== + +const BATCH_SCRIPT_COUNT: usize = 20; + +fn batch_script_name(i: usize) -> String { + format!("BatchScript{:02}", i) +} + +fn batch_script_rel_path(i: usize) -> String { + format!("src/{}.luau", batch_script_name(i)) +} + +#[test] +fn batch_stage_20_files_no_index_lock_race() { + let mut session = TestServeSession::new_with_git("git_sync_defaults", |path| { + git_commit_all(path, "initial commit"); + for i in 1..=BATCH_SCRIPT_COUNT { + fs::write( + path.join(batch_script_rel_path(i)), + format!("-- modified {}\nreturn {{}}\n", i), + ) + .unwrap(); + } + }); + let info = session.wait_to_come_online(); + let read = session.get_api_read(info.root_instance_id).unwrap(); + + let mut stage_ids = Vec::new(); + for i in 1..=BATCH_SCRIPT_COUNT { + if let Some((id, _)) = find_instance_by_name(&read.instances, &batch_script_name(i)) { + stage_ids.push(id); + } + } + assert!( + stage_ids.len() >= BATCH_SCRIPT_COUNT, + "Should find all {} batch scripts, found {}", + BATCH_SCRIPT_COUNT, + stage_ids.len() + ); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: HashMap::new(), + updated: vec![], + stage_ids, + }; + session.post_api_write(&write_request).unwrap(); + thread::sleep(Duration::from_millis(1500)); + + for i in 1..=BATCH_SCRIPT_COUNT { + assert!( + git_is_staged(session.path(), &batch_script_rel_path(i)), + "{} should be staged", + batch_script_name(i) + ); + } +} + +#[test] +fn batch_source_write_20_files_all_staged() { + let mut session = TestServeSession::new_with_git("git_sync_defaults", |path| { + git_commit_all(path, "initial commit"); + }); + let info = session.wait_to_come_online(); + let read = session.get_api_read(info.root_instance_id).unwrap(); + + let mut updates = Vec::new(); + let mut stage_ids = Vec::new(); + for i in 1..=BATCH_SCRIPT_COUNT { + if let Some((id, _)) = find_instance_by_name(&read.instances, &batch_script_name(i)) { + let mut props = UstrMap::default(); + props.insert( + ustr("Source"), + Some(Variant::String(format!( + "-- batch pull {}\nreturn {{}}\n", + i + ))), + ); + updates.push(InstanceUpdate { + id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }); + stage_ids.push(id); + } + } + assert!( + updates.len() >= BATCH_SCRIPT_COUNT, + "Should find all {} batch scripts, found {}", + BATCH_SCRIPT_COUNT, + updates.len() + ); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: HashMap::new(), + updated: updates, + stage_ids, + }; + session.post_api_write(&write_request).unwrap(); + thread::sleep(Duration::from_millis(2000)); + + for i in 1..=BATCH_SCRIPT_COUNT { + let rel = batch_script_rel_path(i); + assert!( + git_is_staged(session.path(), &rel), + "{} should be staged after batch Source write", + batch_script_name(i) + ); + let content = fs::read_to_string(session.path().join(&rel)).unwrap(); + assert!( + content.contains(&format!("batch pull {}", i)), + "{} should have new content on disk", + batch_script_name(i) + ); + } +} + +#[test] +fn batch_mixed_stage_and_source_write() { + let mut session = TestServeSession::new_with_git("git_sync_defaults", |path| { + git_commit_all(path, "initial commit"); + // Modify the first 10 files on disk (push-accepted: already on disk, just stage) + for i in 1..=10 { + fs::write( + path.join(batch_script_rel_path(i)), + format!("-- push modified {}\nreturn {{}}\n", i), + ) + .unwrap(); + } + }); + let info = session.wait_to_come_online(); + let read = session.get_api_read(info.root_instance_id).unwrap(); + + let mut updates = Vec::new(); + let mut stage_ids = Vec::new(); + + // Files 1-10: push-accepted (already modified on disk, just stage) + for i in 1..=10 { + if let Some((id, _)) = find_instance_by_name(&read.instances, &batch_script_name(i)) { + stage_ids.push(id); + } + } + + // Files 11-20: pull-accepted (Source write + stage) + for i in 11..=BATCH_SCRIPT_COUNT { + if let Some((id, _)) = find_instance_by_name(&read.instances, &batch_script_name(i)) { + let mut props = UstrMap::default(); + props.insert( + ustr("Source"), + Some(Variant::String(format!( + "-- pull source {}\nreturn {{}}\n", + i + ))), + ); + updates.push(InstanceUpdate { + id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }); + stage_ids.push(id); + } + } + + assert!( + stage_ids.len() >= BATCH_SCRIPT_COUNT, + "Should have {} stage_ids, got {}", + BATCH_SCRIPT_COUNT, + stage_ids.len() + ); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: HashMap::new(), + updated: updates, + stage_ids, + }; + session.post_api_write(&write_request).unwrap(); + thread::sleep(Duration::from_millis(2000)); + + // All 20 files should be staged + for i in 1..=BATCH_SCRIPT_COUNT { + assert!( + git_is_staged(session.path(), &batch_script_rel_path(i)), + "{} should be staged in mixed batch", + batch_script_name(i) + ); + } + + // Files 11-20 should have the new pull content + for i in 11..=BATCH_SCRIPT_COUNT { + let content = fs::read_to_string(session.path().join(batch_script_rel_path(i))).unwrap(); + assert!( + content.contains(&format!("pull source {}", i)), + "{} should have pull content on disk", + batch_script_name(i) + ); + } +}