Eliminate deep recursion risk in cross-disk directory copy (cross_copy_single) - #8
Merged
koraytaylan merged 1 commit intoMay 19, 2026
Conversation
…s_copy_single) (#3) - Rewrite cross_copy_single to use explicit VecDeque work queue (BFS) instead of recursion. This removes the stack growth per directory level that could cause overflow on deep trees (even with Box::pin). - Update documentation for the function. - Add regression test 'cross_copy_single_deep_directory_tree' that builds and successfully copies a 60-level nested directory tree using MemoryBackend. - Verified: cargo check, all cross_* tests (incl. run loops), clippy -D warnings pass. - Behavior for callers, cancellation (outer loop), and progress unchanged. Closes #3
There was a problem hiding this comment.
Pull request overview
This PR refactors cross_copy_single (used by cross-backend copy/move) to avoid unbounded recursion when copying directory trees, preventing potential stack overflow on extremely deep directory hierarchies.
Changes:
- Replaced recursive directory traversal in
cross_copy_singlewith an iterativeVecDequework queue. - Updated function-level documentation to explain the recursion-safety motivation and approach.
- Added a regression test that copies a 60-level deep directory tree between two
MemoryBackends.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+212
to
224
| while let Some((current_src, current_dst)) = pending.pop_front() { | ||
| let stat = src.stat(¤t_src).await?; | ||
| if stat.is_dir { | ||
| dst.create_dir(¤t_dst).await?; | ||
| let children = src.list(¤t_src).await?; | ||
| for child in children { | ||
| let child_dst = format!("{}/{}", current_dst.trim_end_matches('/'), child.name); | ||
| pending.push_back((child.path, child_dst)); | ||
| } | ||
| } else { | ||
| let data = src.read(¤t_src).await?; | ||
| dst.write(¤t_dst, &data).await?; | ||
| } |
Comment on lines
+209
to
+220
| let mut pending: VecDeque<(String, String)> = VecDeque::new(); | ||
| pending.push_back((src_path.to_string(), dst_path.to_string())); | ||
|
|
||
| while let Some((current_src, current_dst)) = pending.pop_front() { | ||
| let stat = src.stat(¤t_src).await?; | ||
| if stat.is_dir { | ||
| dst.create_dir(¤t_dst).await?; | ||
| let children = src.list(¤t_src).await?; | ||
| for child in children { | ||
| let child_dst = format!("{}/{}", current_dst.trim_end_matches('/'), child.name); | ||
| pending.push_back((child.path, child_dst)); | ||
| } |
Comment on lines
+1109
to
+1112
| /// Regression test for deep directory trees (prevents stack overflow in cross-copy). | ||
| /// Creates a 60-level deep nested directory tree with a file at the bottom | ||
| /// and verifies that iterative copy succeeds where recursion would have overflowed. | ||
| #[tokio::test] |
This was referenced May 19, 2026
koraytaylan
added a commit
that referenced
this pull request
May 19, 2026
) (#12) - Pass cancel_flag (as Option<&AtomicBool>) into cross_copy_single and check it inside the DFS work loop for finer-grained cooperative cancellation during deep/wide cross-disk trees. - Update run_cross_copy_loop / run_cross_move_loop to pass the flag and re-check after the call so mid-tree cancels (incl. last top-level item) correctly transition the job to Cancelled instead of Completed/Failed. - Switch work queue from VecDeque (BFS) to Vec (explicit DFS/LIFO stack) to address memory concerns on wide directories; update docs explaining the improved peak-mem characteristics (drop full Entry metadata early, only retain path strings for pending siblings). - Reword deep-tree test as regression guard + bump from 60 to 1000 levels (now credibly exercises the 2k+ risk range from original #3). - Update module docs for the improved cancellation granularity. - All callers/tests updated; no behavior change for normal cases. - cargo test + clippy -D warnings clean. Addresses all three points from Copilot review on PR #8 (follow-up to #3). Closes #11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the deep recursion risk in
cross_copy_single(used by cross-disk copy/move) as described in #3.The Problem
The original implementation used unbounded recursion (with
Box::pin) for directory trees. Each nesting level added a stack frame, risking stack overflow (and app crash) on deep directory structures (e.g. 1000s of levels from backups or generated data).The Fix
VecDeque-based iterative work queue (BFS traversal) insidecross_copy_single.MemoryBackends (would have overflowed before).Verification
cross_copy_single_*,run_cross_copy_loop_*,run_cross_move_loop_*).cargo clippy -- -D warningsclean.Closes #3
Files changed
backend/src/commands/file.rs: core fix + testThis meets all acceptance criteria from the issue.