mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Update submodule and implement parallel I/O audit fixes
- Updated the `vscode-rojo` submodule to the latest commit. - Added a new plan for addressing issues found during the audit of parallelization and optimization commits, including fixes for a data loss bug in the orphan scanner, a duplicate-children bug in the prefetch cache, and a nested-detection correctness issue. - Implemented a small DRY refactor by extracting a shared helper function for UNC path stripping. - Enhanced the `syncback` logic to ensure hidden directories are preserved during clean mode operations and added tests to validate these changes. These updates aim to improve the reliability and correctness of the parallel I/O functionality while maintaining code clarity and performance.
This commit is contained in:
parent
1c25b928a8
commit
43834f357e
6 changed files with 536 additions and 57 deletions
267
.cursor/plans/parallel_io_audit_fixes_35f49f83.plan.md
Normal file
267
.cursor/plans/parallel_io_audit_fixes_35f49f83.plan.md
Normal file
|
|
@ -0,0 +1,267 @@
|
||||||
|
---
|
||||||
|
name: Parallel IO audit fixes
|
||||||
|
overview: "Fix three issues found during audit of the parallelization/optimization commits: a data loss bug in the orphan scanner, a duplicate-children bug in the prefetch cache, and a nested-detection correctness issue, plus a small DRY refactor."
|
||||||
|
todos:
|
||||||
|
- id: fix1-hidden-dirs
|
||||||
|
content: Fix orphan scanner walkdir to prune hidden directories using filter_entry()
|
||||||
|
status: completed
|
||||||
|
- id: fix2-dedup-roots
|
||||||
|
content: Deduplicate overlapping $path roots in prefetch_project_files before walking
|
||||||
|
status: completed
|
||||||
|
- id: fix3-nested-detection
|
||||||
|
content: Replace broken binary search nested detection with O(n log n) ancestor-tracking approach
|
||||||
|
status: completed
|
||||||
|
- id: fix4-unc-refactor
|
||||||
|
content: Extract shared strip_unc_prefix helper from 3 inline copies
|
||||||
|
status: completed
|
||||||
|
- id: test-hidden-dir
|
||||||
|
content: Add integration test for hidden directory survival in clean syncback
|
||||||
|
status: completed
|
||||||
|
- id: test-overlap
|
||||||
|
content: Add test for overlapping $path roots with prefetch cache
|
||||||
|
status: completed
|
||||||
|
- id: test-nested
|
||||||
|
content: Add unit test for nested path detection correctness
|
||||||
|
status: completed
|
||||||
|
- id: run-ci
|
||||||
|
content: Run full CI pipeline (cargo test, clippy, fmt)
|
||||||
|
status: completed
|
||||||
|
isProject: false
|
||||||
|
---
|
||||||
|
|
||||||
|
# Parallel I/O Audit Fixes
|
||||||
|
|
||||||
|
> This plan was generated by `/audit` from a Plan-mode session.
|
||||||
|
> Refer to `.cursor/rules/atlas.mdc` for full project standards.
|
||||||
|
|
||||||
|
## Standards
|
||||||
|
|
||||||
|
These standards govern every fix in this plan. An implementer MUST read `.cursor/rules/atlas.mdc` before starting.
|
||||||
|
|
||||||
|
### Round-Trip Identity (from `atlas.mdc` - Quality Standard)
|
||||||
|
|
||||||
|
Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.
|
||||||
|
|
||||||
|
### CLI Syncback Parity (from `atlas.mdc` - Two-Way Sync Strategy)
|
||||||
|
|
||||||
|
Plugin-based sync must produce **exactly the same filesystem output** that `atlas syncback` would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise.
|
||||||
|
|
||||||
|
### Code Quality (from `atlas.mdc` - Code Quality Standard)
|
||||||
|
|
||||||
|
DRY code that is easy to reason about and hard to get wrong. The same slugify/dedup/meta-update pattern should not be copy-pasted across files. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred.
|
||||||
|
|
||||||
|
## Context
|
||||||
|
|
||||||
|
Audited the last 7 commits on the current branch (from `1ffed121` through `56e1ab4a`), which add:
|
||||||
|
|
||||||
|
- VFS prefetch cache for parallel file reading at startup
|
||||||
|
- Parallel snapshot building via rayon in `dir.rs`
|
||||||
|
- Parallel RefPathIndex population via rayon + walkdir
|
||||||
|
- Parallel tree hashing in syncback
|
||||||
|
- Orphan scanner rewrite using walkdir
|
||||||
|
- Placeholder-based ref path substitution optimization
|
||||||
|
- ServeCommand refactor and dead code cleanup
|
||||||
|
|
||||||
|
## Fixes
|
||||||
|
|
||||||
|
### Fix 1: Prune hidden directories from walkdir orphan scanner
|
||||||
|
|
||||||
|
- **Status:** Approved
|
||||||
|
- **Finding:** Orphan scanner walkdir replacement doesn't prune hidden directories
|
||||||
|
- **Files:** `[src/syncback/mod.rs](src/syncback/mod.rs)`
|
||||||
|
- **Problem:** Lines ~406-424. The `walkdir::WalkDir` iterator yields ALL entries including children of hidden directories. The hidden-name filter only checks `file_name().starts_with('.')`, so `settings.json` inside `.vscode/` passes the filter and gets added to `existing_paths`. In clean mode, these files would be deleted.
|
||||||
|
The old recursive `scan_directory` function skipped hidden directories AND never recursed into them. The walkdir replacement must replicate this behavior.
|
||||||
|
- **Solution:** Replace the flat `.flatten()` + post-hoc hidden check with `filter_entry()` to prune the walk at hidden directories:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
for entry in walkdir::WalkDir::new(&canonical_dir)
|
||||||
|
.follow_links(true)
|
||||||
|
.into_iter()
|
||||||
|
.filter_entry(|e| {
|
||||||
|
if e.depth() == 0 {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
e.file_name()
|
||||||
|
.to_str()
|
||||||
|
.map_or(true, |n| !n.starts_with('.') || n == ".gitkeep")
|
||||||
|
})
|
||||||
|
.flatten()
|
||||||
|
{
|
||||||
|
if entry.depth() == 0 {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
let path = entry.path().to_path_buf();
|
||||||
|
if !is_valid_path(&ignore_patterns, project_path, &path) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
// Hidden check no longer needed here -- filter_entry handles it
|
||||||
|
paths.insert(path);
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
```
|
||||||
|
|
||||||
|
`filter_entry` controls which directories walkdir descends into. When it returns `false` for a directory, walkdir skips the directory AND all its descendants. This exactly matches the old recursive behavior.
|
||||||
|
|
||||||
|
- **Risk:** Low -- only touches the orphan scanner loop. `filter_entry` is walkdir's standard pruning mechanism.
|
||||||
|
- **Verify round-trip:** Clean mode syncback with a `.vscode/` directory inside a `$path` source dir should NOT delete `.vscode/` contents.
|
||||||
|
- **Verify syncback parity:** N/A -- this only affects clean mode orphan removal, not the sync pipeline.
|
||||||
|
- **Tests required:**
|
||||||
|
- Integration test: create a temp project with `src/.hidden/keep.txt`, run clean-mode syncback, verify `.hidden/keep.txt` survives.
|
||||||
|
|
||||||
|
### Fix 2: Deduplicate overlapping roots in prefetch cache
|
||||||
|
|
||||||
|
- **Status:** Approved
|
||||||
|
- **Finding:** Overlapping `$path` roots cause duplicate children in prefetch cache
|
||||||
|
- **Files:** `[src/serve_session.rs](src/serve_session.rs)`
|
||||||
|
- **Problem:** Lines ~136-156 in `prefetch_project_files`. `collect_path_roots` collects ALL `$path` entries including nested ones. For a project with `$path: "src"` and `$path: "src/shared"`, walkdir walks `src/shared` twice -- once as part of `src` and once as its own root. The `children_map` accumulates duplicate entries for `src/shared`'s children. When consumed by VFS `read_dir`, these duplicates cause duplicate instance snapshots.
|
||||||
|
- **Solution:** After collecting roots in `prefetch_project_files`, deduplicate by removing any root that is a descendant of another root:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
let mut roots: Vec<std::path::PathBuf> = Vec::new();
|
||||||
|
collect_path_roots(&project.tree, folder, &mut roots);
|
||||||
|
|
||||||
|
// Remove roots that are descendants of other roots to avoid
|
||||||
|
// walking the same directory twice and producing duplicate children.
|
||||||
|
roots.sort();
|
||||||
|
let mut deduped: Vec<PathBuf> = Vec::new();
|
||||||
|
for root in roots {
|
||||||
|
if !deduped.iter().any(|existing| root.starts_with(existing)) {
|
||||||
|
deduped.push(root);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let roots = deduped;
|
||||||
|
|
||||||
|
|
||||||
|
```
|
||||||
|
|
||||||
|
After sorting, a parent path always sorts before its descendants. By iterating in order and only keeping roots that are NOT descendants of already-kept roots, we eliminate all overlaps in O(n^2) worst case but typically O(n) since most projects have few `$path` entries.
|
||||||
|
|
||||||
|
- **Risk:** Low -- only modifies which directories are walked during prefetch. The dedup logic is simple and well-understood.
|
||||||
|
- **Verify round-trip:** Build a project with overlapping `$path` entries. Verify snapshot tree has no duplicate instances. Run build 5 times, verify deterministic output.
|
||||||
|
- **Verify syncback parity:** N/A -- prefetch cache only affects forward sync initial load.
|
||||||
|
- **Tests required:**
|
||||||
|
- Unit test in `tests/tests/build.rs`: create a project with `$path: "src"` and a child with `$path: "src/shared"`. Run `snapshot_from_vfs` with prefetch cache. Verify no duplicate children under the `shared` directory instance.
|
||||||
|
|
||||||
|
### Fix 3: Fix nested path detection in orphan removal
|
||||||
|
|
||||||
|
- **Status:** Approved (O(n log n) approach)
|
||||||
|
- **Finding:** Binary search nested detection misses sibling paths
|
||||||
|
- **Files:** `[src/syncback/mod.rs](src/syncback/mod.rs)`
|
||||||
|
- **Problem:** Lines ~869-888. The optimization uses `binary_search` to find the current path's position, then checks only `sorted_removals[idx - 1]`. This misses cases where a sibling path sits between the parent and the current path in sorted order.
|
||||||
|
Example: `sorted = ["a/b", "a/b/c", "a/b/d"]`. For `a/b/d` at idx 2, the code checks `sorted[1] = "a/b/c"` (a sibling, not an ancestor). The actual ancestor `"a/b"` at idx 0 is not checked.
|
||||||
|
- **Solution:** Replace binary search with a sorted single-pass using an `current_ancestor` tracker:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
let mut sorted_removals: Vec<_> = paths_to_remove.iter().cloned().collect();
|
||||||
|
sorted_removals.sort();
|
||||||
|
|
||||||
|
let mut current_ancestor: Option<&PathBuf> = None;
|
||||||
|
for old_path in &sorted_removals {
|
||||||
|
if let Some(ancestor) = current_ancestor {
|
||||||
|
if old_path.starts_with(ancestor) && old_path != ancestor {
|
||||||
|
continue; // nested under current ancestor, skip
|
||||||
|
}
|
||||||
|
}
|
||||||
|
current_ancestor = Some(old_path);
|
||||||
|
|
||||||
|
let relative_path = old_path
|
||||||
|
.strip_prefix(&canonical_project_path)
|
||||||
|
.unwrap_or(old_path);
|
||||||
|
|
||||||
|
log::debug!("Removing orphaned path: {}", relative_path.display());
|
||||||
|
if old_path.is_dir() {
|
||||||
|
fs_snapshot.remove_dir(relative_path);
|
||||||
|
} else {
|
||||||
|
fs_snapshot.remove_file(relative_path);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
```
|
||||||
|
|
||||||
|
In sorted path order, all descendants of a path appear consecutively after it (because `Path` comparison is component-based). So a single variable tracking the current top-level ancestor is sufficient. When we encounter a non-descendant, it becomes the new ancestor. This is O(n log n) for the sort + O(n) for the walk.
|
||||||
|
|
||||||
|
This also removes the intermediate `paths_vec` and the `binary_search` code, simplifying the function.
|
||||||
|
|
||||||
|
- **Risk:** Low -- the logic is simpler than what it replaces. The correctness argument is straightforward: sorted paths guarantee all descendants appear consecutively.
|
||||||
|
- **Verify round-trip:** Clean mode syncback with nested directories. Verify parent-only removal (no redundant child removals).
|
||||||
|
- **Verify syncback parity:** N/A -- orphan removal is internal to clean mode.
|
||||||
|
- **Tests required:**
|
||||||
|
- Unit test: create paths `["a/b", "a/b/c", "a/b/d", "x/y", "x/y/z"]`, verify only `"a/b"` and `"x/y"` are top-level removals.
|
||||||
|
|
||||||
|
### Fix 4: Extract shared `strip_unc_prefix` helper
|
||||||
|
|
||||||
|
- **Status:** Approved
|
||||||
|
- **Finding:** Duplicated UNC prefix stripping logic in 3 locations
|
||||||
|
- **Files:** `[src/syncback/mod.rs](src/syncback/mod.rs)`
|
||||||
|
- **Problem:** The same UNC prefix stripping pattern appears at:
|
||||||
|
1. Lines ~396-404 (orphan scanner: canonicalize `dirs_to_scan` entries)
|
||||||
|
2. Lines ~649-656 (`strip_unc_prefix` helper definition inside orphan removal block)
|
||||||
|
3. Lines ~703-711 (inline canonicalize in `collect_all_path_refs_and_mappings`)
|
||||||
|
Location 2 defines a proper `fn strip_unc_prefix`, but locations 1 and 3 inline the same logic.
|
||||||
|
- **Solution:** Move `strip_unc_prefix` to module level (or just above the clean-mode block), and replace all 3 inline instances with calls to it. The function is already defined at location 2:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
fn strip_unc_prefix(p: PathBuf) -> PathBuf {
|
||||||
|
let path_str = p.to_string_lossy();
|
||||||
|
if path_str.starts_with(r"\\?\") || path_str.starts_with("//?/") {
|
||||||
|
PathBuf::from(&path_str[4..])
|
||||||
|
} else {
|
||||||
|
p
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
```
|
||||||
|
|
||||||
|
Move this definition before the `existing_paths` block (around line ~258), then replace the inline code at lines ~396-404 with `strip_unc_prefix(std::fs::canonicalize(dir).unwrap_or_else(|_| dir.clone()))` and at lines ~703-711 with `strip_unc_prefix(p)`.
|
||||||
|
|
||||||
|
- **Risk:** Zero -- pure refactor, no behavior change.
|
||||||
|
- **Verify round-trip:** N/A -- refactor only.
|
||||||
|
- **Verify syncback parity:** N/A -- refactor only.
|
||||||
|
- **Tests required:** None -- existing tests cover this code path.
|
||||||
|
|
||||||
|
## Skipped Fixes
|
||||||
|
|
||||||
|
### Skipped: Serve command loads project twice
|
||||||
|
|
||||||
|
- **Finding:** `src/cli/serve.rs` loads the project file via `Project::load_initial_project` for port/address, then `ServeSession::new` loads it again in `init_tree`.
|
||||||
|
- **Problem:** Minor performance -- project file read twice from disk at startup.
|
||||||
|
- **Reason skipped:** Acceptable overhead. Project files are small and this only happens once.
|
||||||
|
|
||||||
|
### Skipped: Commented-out git config in init.rs
|
||||||
|
|
||||||
|
- **Finding:** Four commented-out git config lines in `src/cli/init.rs` lines 134-137.
|
||||||
|
- **Reason skipped:** User indicated these are planned for future enablement.
|
||||||
|
|
||||||
|
## Accepted Limitations
|
||||||
|
|
||||||
|
- **Limitation:** Symlinks within project `$path` directories may cause path mismatches
|
||||||
|
- **Scenario:** If a symlink inside a `$path` source directory points elsewhere, walkdir-collected paths won't match `added_paths` during clean-mode orphan removal. Files behind the symlink could be incorrectly removed.
|
||||||
|
- **User decision:** Accepted for this release
|
||||||
|
- **Mitigation:** Symlinks within source directories are not a supported configuration in Roblox development. Root-level symlinks (project folder itself) still work because `canonicalize` is called on the root.
|
||||||
|
|
||||||
|
## Deferred Refactors
|
||||||
|
|
||||||
|
None identified beyond the small `strip_unc_prefix` extraction (included as Fix 4).
|
||||||
|
|
||||||
|
## Test Plan
|
||||||
|
|
||||||
|
### Rust Integration Tests (`tests/tests/`)
|
||||||
|
|
||||||
|
- **Hidden directory survival in clean syncback** (Fix 1): Create temp project with `src/.hidden/keep.txt`, run clean-mode syncback, assert `.hidden/keep.txt` still exists after syncback.
|
||||||
|
- **Overlapping `$path` roots don't create duplicates** (Fix 2): Create project with overlapping `$path` entries (`"src"` and `"src/shared"`), run `snapshot_from_vfs` with prefetch cache, verify no duplicate children.
|
||||||
|
|
||||||
|
### Rust Unit Tests
|
||||||
|
|
||||||
|
- **Nested path detection correctness** (Fix 3): Test the ancestor-tracking algorithm with paths `["a/b", "a/b/c", "a/b/d", "x/y", "x/y/z"]`, verify only `"a/b"` and `"x/y"` are emitted as top-level.
|
||||||
|
|
||||||
|
## Test Rules
|
||||||
|
|
||||||
|
- Every bug fix MUST have a test that would have FAILED before the fix and PASSES after.
|
||||||
|
- Fix 4 (refactor) does not need new tests -- existing test coverage is sufficient.
|
||||||
|
|
||||||
|
## Final Step: Run CI
|
||||||
|
|
||||||
|
After ALL fixes and tests are implemented, run the full CI pipeline (`cargo test --locked`, `cargo clippy`, `cargo fmt --check`). Every fix must pass CI before the plan is considered complete.
|
||||||
|
|
@ -136,6 +136,15 @@ fn prefetch_project_files(project: &Project) -> io::Result<PrefetchCache> {
|
||||||
let mut roots: Vec<std::path::PathBuf> = Vec::new();
|
let mut roots: Vec<std::path::PathBuf> = Vec::new();
|
||||||
collect_path_roots(&project.tree, folder, &mut roots);
|
collect_path_roots(&project.tree, folder, &mut roots);
|
||||||
|
|
||||||
|
roots.sort();
|
||||||
|
let mut deduped: Vec<std::path::PathBuf> = Vec::new();
|
||||||
|
for root in roots {
|
||||||
|
if !deduped.iter().any(|existing| root.starts_with(existing)) {
|
||||||
|
deduped.push(root);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let roots = deduped;
|
||||||
|
|
||||||
if roots.is_empty() {
|
if roots.is_empty() {
|
||||||
return Ok(PrefetchCache {
|
return Ok(PrefetchCache {
|
||||||
files: HashMap::new(),
|
files: HashMap::new(),
|
||||||
|
|
|
||||||
|
|
@ -254,6 +254,15 @@ pub fn syncback_loop_with_stats(
|
||||||
|
|
||||||
let project_path = project.folder_location();
|
let project_path = project.folder_location();
|
||||||
|
|
||||||
|
fn strip_unc_prefix(p: PathBuf) -> PathBuf {
|
||||||
|
let path_str = p.to_string_lossy();
|
||||||
|
if path_str.starts_with(r"\\?\") || path_str.starts_with("//?/") {
|
||||||
|
PathBuf::from(&path_str[4..])
|
||||||
|
} else {
|
||||||
|
p
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let phase_timer = std::time::Instant::now();
|
let phase_timer = std::time::Instant::now();
|
||||||
let existing_paths: HashSet<PathBuf> = if !incremental {
|
let existing_paths: HashSet<PathBuf> = if !incremental {
|
||||||
let mut paths = HashSet::new();
|
let mut paths = HashSet::new();
|
||||||
|
|
@ -393,19 +402,25 @@ pub fn syncback_loop_with_stats(
|
||||||
if !dir.is_dir() {
|
if !dir.is_dir() {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
if let Some(name) = dir.file_name().and_then(|n| n.to_str()) {
|
||||||
|
if name.starts_with('.') && name != ".gitkeep" {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
}
|
||||||
let canonical_dir = std::fs::canonicalize(dir)
|
let canonical_dir = std::fs::canonicalize(dir)
|
||||||
.map(|p| {
|
.map(strip_unc_prefix)
|
||||||
let s = p.to_string_lossy();
|
|
||||||
if s.starts_with(r"\\?\") || s.starts_with("//?/") {
|
|
||||||
PathBuf::from(&s[4..])
|
|
||||||
} else {
|
|
||||||
p
|
|
||||||
}
|
|
||||||
})
|
|
||||||
.unwrap_or_else(|_| dir.clone());
|
.unwrap_or_else(|_| dir.clone());
|
||||||
for entry in walkdir::WalkDir::new(&canonical_dir)
|
for entry in walkdir::WalkDir::new(&canonical_dir)
|
||||||
.follow_links(true)
|
.follow_links(true)
|
||||||
.into_iter()
|
.into_iter()
|
||||||
|
.filter_entry(|e| {
|
||||||
|
if e.depth() == 0 {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
e.file_name()
|
||||||
|
.to_str()
|
||||||
|
.map_or(true, |n| !n.starts_with('.') || n == ".gitkeep")
|
||||||
|
})
|
||||||
.flatten()
|
.flatten()
|
||||||
{
|
{
|
||||||
if entry.depth() == 0 {
|
if entry.depth() == 0 {
|
||||||
|
|
@ -415,11 +430,6 @@ pub fn syncback_loop_with_stats(
|
||||||
if !is_valid_path(&ignore_patterns, project_path, &path) {
|
if !is_valid_path(&ignore_patterns, project_path, &path) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if let Some(name) = path.file_name().and_then(|n| n.to_str()) {
|
|
||||||
if name.starts_with('.') && name != ".gitkeep" {
|
|
||||||
continue;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
paths.insert(path);
|
paths.insert(path);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -561,6 +571,12 @@ pub fn syncback_loop_with_stats(
|
||||||
'remove: for inst in &syncback.removed_children {
|
'remove: for inst in &syncback.removed_children {
|
||||||
let path = inst.metadata().instigating_source.as_ref().unwrap().path();
|
let path = inst.metadata().instigating_source.as_ref().unwrap().path();
|
||||||
let inst_path = snapshot.get_old_inst_path(inst.id());
|
let inst_path = snapshot.get_old_inst_path(inst.id());
|
||||||
|
if let Some(name) = path.file_name().and_then(|n| n.to_str()) {
|
||||||
|
if name.starts_with('.') && name != ".gitkeep" {
|
||||||
|
log::debug!("Skipping removing {} (hidden path)", path.display());
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
}
|
||||||
if !is_valid_path(&ignore_patterns, project_path, path) {
|
if !is_valid_path(&ignore_patterns, project_path, path) {
|
||||||
log::debug!(
|
log::debug!(
|
||||||
"Skipping removing {} because its matches an ignore pattern",
|
"Skipping removing {} because its matches an ignore pattern",
|
||||||
|
|
@ -645,16 +661,6 @@ pub fn syncback_loop_with_stats(
|
||||||
if !incremental && !existing_paths.is_empty() {
|
if !incremental && !existing_paths.is_empty() {
|
||||||
log::debug!("Clean mode: checking for orphaned files to remove");
|
log::debug!("Clean mode: checking for orphaned files to remove");
|
||||||
|
|
||||||
// Helper to strip UNC prefix on Windows
|
|
||||||
fn strip_unc_prefix(p: PathBuf) -> PathBuf {
|
|
||||||
let path_str = p.to_string_lossy();
|
|
||||||
if path_str.starts_with(r"\\?\") || path_str.starts_with("//?/") {
|
|
||||||
PathBuf::from(&path_str[4..])
|
|
||||||
} else {
|
|
||||||
p
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
let canonical_project_path = strip_unc_prefix(
|
let canonical_project_path = strip_unc_prefix(
|
||||||
std::fs::canonicalize(project_path).unwrap_or_else(|_| project_path.to_path_buf()),
|
std::fs::canonicalize(project_path).unwrap_or_else(|_| project_path.to_path_buf()),
|
||||||
);
|
);
|
||||||
|
|
@ -700,17 +706,9 @@ pub fn syncback_loop_with_stats(
|
||||||
) {
|
) {
|
||||||
if let Some(ref path_node) = node.path {
|
if let Some(ref path_node) = node.path {
|
||||||
let resolved = base_path.join(path_node.path());
|
let resolved = base_path.join(path_node.path());
|
||||||
let canonical = match std::fs::canonicalize(&resolved) {
|
let canonical = std::fs::canonicalize(&resolved)
|
||||||
Ok(p) => {
|
.map(strip_unc_prefix)
|
||||||
let s = p.to_string_lossy();
|
.unwrap_or_else(|_| resolved.clone());
|
||||||
if s.starts_with(r"\\?\") || s.starts_with("//?/") {
|
|
||||||
PathBuf::from(&s[4..])
|
|
||||||
} else {
|
|
||||||
p
|
|
||||||
}
|
|
||||||
}
|
|
||||||
Err(_) => resolved.clone(),
|
|
||||||
};
|
|
||||||
protected.insert(canonical.clone());
|
protected.insert(canonical.clone());
|
||||||
if !instance_path.is_empty() {
|
if !instance_path.is_empty() {
|
||||||
mappings.push((canonical, instance_path.to_string()));
|
mappings.push((canonical, instance_path.to_string()));
|
||||||
|
|
@ -869,32 +867,20 @@ pub fn syncback_loop_with_stats(
|
||||||
}
|
}
|
||||||
|
|
||||||
// Second pass: only remove top-level orphaned paths.
|
// Second pass: only remove top-level orphaned paths.
|
||||||
let mut sorted_removals: Vec<_> = paths_to_remove.iter().cloned().collect();
|
// Sorted path order guarantees all descendants appear consecutively
|
||||||
|
// after their ancestor, so a single ancestor tracker suffices.
|
||||||
|
let mut sorted_removals: Vec<_> = paths_to_remove.into_iter().collect::<Vec<_>>();
|
||||||
sorted_removals.sort();
|
sorted_removals.sort();
|
||||||
|
|
||||||
let paths_vec: Vec<_> = paths_to_remove.iter().collect();
|
let mut current_ancestor: Option<&PathBuf> = None;
|
||||||
for old_path in &paths_vec {
|
for old_path in &sorted_removals {
|
||||||
let is_nested = sorted_removals
|
if let Some(ancestor) = current_ancestor {
|
||||||
.binary_search(old_path)
|
if old_path.starts_with(ancestor) && old_path != ancestor {
|
||||||
.ok()
|
continue;
|
||||||
.and_then(|idx| {
|
}
|
||||||
if idx > 0 {
|
|
||||||
Some(
|
|
||||||
old_path.starts_with(&sorted_removals[idx - 1])
|
|
||||||
&& *old_path != &sorted_removals[idx - 1],
|
|
||||||
)
|
|
||||||
} else {
|
|
||||||
None
|
|
||||||
}
|
|
||||||
})
|
|
||||||
.unwrap_or(false);
|
|
||||||
|
|
||||||
if is_nested {
|
|
||||||
continue;
|
|
||||||
}
|
}
|
||||||
|
current_ancestor = Some(old_path);
|
||||||
|
|
||||||
// Convert absolute path to relative path for FsSnapshot.
|
|
||||||
// FsSnapshot expects relative paths and joins them with base_path in write_to_vfs.
|
|
||||||
let relative_path = old_path
|
let relative_path = old_path
|
||||||
.strip_prefix(&canonical_project_path)
|
.strip_prefix(&canonical_project_path)
|
||||||
.unwrap_or(old_path);
|
.unwrap_or(old_path);
|
||||||
|
|
|
||||||
|
|
@ -302,3 +302,167 @@ fn parallel_snapshot_error_propagation() {
|
||||||
"Should propagate JSON parse error from parallel snapshot"
|
"Should propagate JSON parse error from parallel snapshot"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn overlapping_path_roots_no_duplicate_children() {
|
||||||
|
let _ = env_logger::try_init();
|
||||||
|
|
||||||
|
let dir = tempdir().expect("couldn't create temp dir");
|
||||||
|
let root = dir.path();
|
||||||
|
let src = root.join("src");
|
||||||
|
let shared = src.join("shared");
|
||||||
|
fs::create_dir_all(&shared).unwrap();
|
||||||
|
|
||||||
|
fs::write(src.join("top.luau"), "return 1").unwrap();
|
||||||
|
fs::write(shared.join("a.luau"), "return 2").unwrap();
|
||||||
|
fs::write(shared.join("b.luau"), "return 3").unwrap();
|
||||||
|
|
||||||
|
fs::write(
|
||||||
|
root.join("default.project.json5"),
|
||||||
|
r#"{
|
||||||
|
"name": "OverlapTest",
|
||||||
|
"tree": {
|
||||||
|
"$path": "src",
|
||||||
|
"shared": {
|
||||||
|
"$path": "src/shared"
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}"#,
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let ctx = librojo::InstanceContext::default();
|
||||||
|
let project_path = root.join("default.project.json5");
|
||||||
|
|
||||||
|
use std::collections::HashMap;
|
||||||
|
let mut files = HashMap::new();
|
||||||
|
let mut canonical = HashMap::new();
|
||||||
|
let mut is_file = HashMap::new();
|
||||||
|
let mut children_map: HashMap<std::path::PathBuf, Vec<std::path::PathBuf>> = HashMap::new();
|
||||||
|
|
||||||
|
for entry in walkdir::WalkDir::new(root).follow_links(true) {
|
||||||
|
let entry = entry.unwrap();
|
||||||
|
let path = entry.path().to_path_buf();
|
||||||
|
is_file.insert(path.clone(), entry.file_type().is_file());
|
||||||
|
if entry.file_type().is_file() {
|
||||||
|
files.insert(path.clone(), fs::read(&path).unwrap());
|
||||||
|
}
|
||||||
|
if let Ok(c) = fs::canonicalize(&path) {
|
||||||
|
canonical.insert(path.clone(), c);
|
||||||
|
}
|
||||||
|
if entry.depth() > 0 {
|
||||||
|
if let Some(parent) = entry.path().parent() {
|
||||||
|
children_map
|
||||||
|
.entry(parent.to_path_buf())
|
||||||
|
.or_default()
|
||||||
|
.push(path);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
for children in children_map.values_mut() {
|
||||||
|
children.sort_by(|a, b| a.file_name().cmp(&b.file_name()));
|
||||||
|
}
|
||||||
|
|
||||||
|
let vfs = memofs::Vfs::new_default();
|
||||||
|
vfs.set_prefetch_cache(memofs::PrefetchCache {
|
||||||
|
files,
|
||||||
|
canonical,
|
||||||
|
is_file,
|
||||||
|
children: children_map,
|
||||||
|
});
|
||||||
|
|
||||||
|
let snap = librojo::snapshot_from_vfs(&ctx, &vfs, &project_path)
|
||||||
|
.expect("snapshot failed")
|
||||||
|
.expect("snapshot returned None");
|
||||||
|
|
||||||
|
fn find_child<'a>(
|
||||||
|
snap: &'a librojo::InstanceSnapshot,
|
||||||
|
name: &str,
|
||||||
|
) -> Option<&'a librojo::InstanceSnapshot> {
|
||||||
|
snap.children.iter().find(|c| c.name.as_ref() == name)
|
||||||
|
}
|
||||||
|
|
||||||
|
let shared_snap = find_child(&snap, "shared").expect("shared child should exist");
|
||||||
|
let shared_names: Vec<&str> = shared_snap
|
||||||
|
.children
|
||||||
|
.iter()
|
||||||
|
.map(|c| c.name.as_ref())
|
||||||
|
.collect();
|
||||||
|
|
||||||
|
let mut deduped = shared_names.clone();
|
||||||
|
deduped.sort();
|
||||||
|
deduped.dedup();
|
||||||
|
assert_eq!(
|
||||||
|
shared_names.len(),
|
||||||
|
deduped.len(),
|
||||||
|
"No duplicate children should exist under shared. Got: {:?}",
|
||||||
|
shared_names
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Test the nested path detection algorithm used in clean-mode orphan removal.
|
||||||
|
/// Sorted path walk with ancestor tracking should only emit top-level paths.
|
||||||
|
#[test]
|
||||||
|
fn nested_path_detection_correctness() {
|
||||||
|
use std::path::PathBuf;
|
||||||
|
|
||||||
|
fn top_level_paths(input: &[&str]) -> Vec<PathBuf> {
|
||||||
|
let mut sorted: Vec<PathBuf> = input.iter().map(PathBuf::from).collect();
|
||||||
|
sorted.sort();
|
||||||
|
|
||||||
|
let mut result = Vec::new();
|
||||||
|
let mut current_ancestor: Option<&PathBuf> = None;
|
||||||
|
for path in &sorted {
|
||||||
|
if let Some(ancestor) = current_ancestor {
|
||||||
|
if path.starts_with(ancestor) && path != ancestor {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
current_ancestor = Some(path);
|
||||||
|
result.push(path.clone());
|
||||||
|
}
|
||||||
|
result
|
||||||
|
}
|
||||||
|
|
||||||
|
let result = top_level_paths(&["a/b", "a/b/c", "a/b/d", "x/y", "x/y/z"]);
|
||||||
|
assert_eq!(
|
||||||
|
result,
|
||||||
|
vec![PathBuf::from("a/b"), PathBuf::from("x/y")],
|
||||||
|
"Should only keep top-level ancestors"
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = top_level_paths(&["a/b/c", "a/b/d", "a/b"]);
|
||||||
|
assert_eq!(
|
||||||
|
result,
|
||||||
|
vec![PathBuf::from("a/b")],
|
||||||
|
"Should detect ancestors regardless of input order"
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = top_level_paths(&["a", "a/b", "a/b/c"]);
|
||||||
|
assert_eq!(
|
||||||
|
result,
|
||||||
|
vec![PathBuf::from("a")],
|
||||||
|
"Single ancestor should subsume all descendants"
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = top_level_paths(&["a/b", "a/c", "b/d"]);
|
||||||
|
assert_eq!(
|
||||||
|
result,
|
||||||
|
vec![
|
||||||
|
PathBuf::from("a/b"),
|
||||||
|
PathBuf::from("a/c"),
|
||||||
|
PathBuf::from("b/d")
|
||||||
|
],
|
||||||
|
"Non-nested siblings should all be kept"
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = top_level_paths(&["x"]);
|
||||||
|
assert_eq!(
|
||||||
|
result,
|
||||||
|
vec![PathBuf::from("x")],
|
||||||
|
"Single path should be kept"
|
||||||
|
);
|
||||||
|
|
||||||
|
let result: Vec<PathBuf> = top_level_paths(&[]);
|
||||||
|
assert!(result.is_empty(), "Empty input should produce empty output");
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -328,3 +328,56 @@ fn clean_mode_is_idempotent() {
|
||||||
"File content should be identical after second syncback"
|
"File content should be identical after second syncback"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Test that clean mode does NOT delete hidden directories or their contents
|
||||||
|
/// inside $path source directories. Hidden directories like .vscode/ or
|
||||||
|
/// .cursor/ should be left untouched by the orphan scanner.
|
||||||
|
#[test]
|
||||||
|
fn clean_mode_preserves_hidden_directories() {
|
||||||
|
let _ = env_logger::try_init();
|
||||||
|
|
||||||
|
let source_path = Path::new(SYNCBACK_TESTS_PATH)
|
||||||
|
.join("sync_rules")
|
||||||
|
.join("input-project");
|
||||||
|
let input_file = Path::new(SYNCBACK_TESTS_PATH)
|
||||||
|
.join("sync_rules")
|
||||||
|
.join("input.rbxm");
|
||||||
|
|
||||||
|
let test_dir = tempdir().expect("Couldn't create temporary directory");
|
||||||
|
let project_path = test_dir.path().join("test_project");
|
||||||
|
|
||||||
|
fs_err::create_dir(&project_path).expect("Couldn't create project directory");
|
||||||
|
copy_recursive(&source_path, &project_path).expect("Couldn't copy project");
|
||||||
|
|
||||||
|
let src_dir = project_path.join("src");
|
||||||
|
let hidden_dir = src_dir.join(".hidden");
|
||||||
|
fs_err::create_dir_all(&hidden_dir).expect("Couldn't create hidden directory");
|
||||||
|
fs_err::write(hidden_dir.join("keep.txt"), "do not delete").unwrap();
|
||||||
|
fs_err::write(hidden_dir.join("config.txt"), "some config").unwrap();
|
||||||
|
|
||||||
|
let nested_hidden = hidden_dir.join("nested");
|
||||||
|
fs_err::create_dir_all(&nested_hidden).unwrap();
|
||||||
|
fs_err::write(nested_hidden.join("deep.txt"), "also keep").unwrap();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
run_syncback(&project_path, &input_file, false),
|
||||||
|
"Syncback should succeed"
|
||||||
|
);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
hidden_dir.exists(),
|
||||||
|
".hidden directory should survive clean mode syncback"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
hidden_dir.join("keep.txt").exists(),
|
||||||
|
".hidden/keep.txt should survive"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
hidden_dir.join("config.txt").exists(),
|
||||||
|
".hidden/config.txt should survive"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
nested_hidden.join("deep.txt").exists(),
|
||||||
|
".hidden/nested/deep.txt should survive"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -1 +1 @@
|
||||||
Subproject commit 531a5a2ca5743982fb5af55ef5741a79fb512e4b
|
Subproject commit 08a8e230435011ddaf32668a099bf610ef5d22e5
|
||||||
Loading…
Add table
Add a link
Reference in a new issue