Optimize file manager move operations on same filesystem#2429
Conversation
WalkthroughAdds rsync-based transfer/move logic with persistent PID and empty-dir state, new helpers for ETA/formatting/parsing/escaping, JSON progress publishing via an Nchan /pub/filemanager channel, UI updates to parse JSON progress and preserve icons, and removes the mv-feasibility branch. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileMgr as FileManager
participant Rsync
participant FS as Filesystem
participant Nchan
participant UI as Browser/UI
User->>FileMgr: request move/copy (sources[], target)
FileMgr->>FileMgr: read/write `$pid_file`, ensure `$empty_dir`
FileMgr->>Rsync: spawn rsync (escaped components, quoted includes)
Rsync-->>FileMgr: stdout/stderr progress lines
FileMgr->>FileMgr: parse_rsync_progress -> calculate_eta -> format JSON status
FileMgr->>Nchan: publish JSON progress (/pub/filemanager)
Nchan-->>UI: push JSON messages
UI->>UI: dfm_showProgress parses JSON, preserves icon, updates footer via dfm_footer
FileMgr->>FS: handle empty_dir lifecycle, cleanup, remove `$pid_file` on finish/kill
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/nchan/file_manager (3)
15-19: Harden the empty_dir path definition (security + clarity).Good to keep a trailing slash. Also ensure we only ever use a directory path (not a preexisting file/symlink). Follow-up fix is in the creation block below.
115-127: Rsync include patterns: add safeguards and ensure directory includes traverse correctly.
- Guard against invalid/empty paths.
- For directories, include both “/name/” and “/name/***” to ensure traversal.
- Deduplicate to keep the CLI short.
Apply:
function quoted_rsync_include($paths) { - // note: this function is never called with invalid names because of "if (!$valid_source_path)" - $result = []; - foreach ($paths as $path) { - $valid_path = validname($path); - if (is_dir($valid_path)) { - $result[] = "--include=".escapeshellarg("/".basename($valid_path)."/***"); - } else { - $result[] = "--include=".escapeshellarg("/".basename($valid_path)); - } - } - return implode(' ', $result); + $result = []; + foreach ($paths as $path) { + $valid_path = validname($path); + if (!$valid_path) continue; + $base = '/'.basename(rtrim($valid_path,'/')); + if (is_dir($valid_path)) { + // include the dir itself and its contents + $result[] = "--include=".escapeshellarg("$base/"); + $result[] = "--include=".escapeshellarg("$base/***"); + } else { + $result[] = "--include=".escapeshellarg($base); + } + } + // keep CLI lean + $result = array_values(array_unique($result)); + return implode(' ', $result); }
266-269: Fallback rsync path: retain existing behavior, but consider timeouts and error surfacing.Optional: add a short rsync I/O timeout (e.g., --timeout=600) and exit-code logging for better UX when a remote/unusual FS stalls.
I can wire a minimal timeout and capture rsync’s exit code into $status/$error without changing current UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(3 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix/nchan/file_manager (2)
243-251: Conflict check when ignore-existing is set: also guard against preexisting directories.Top-level existence check is good; make it explicit for dirs too (merging is not supported with this trick).
if (!empty($exist)) { // would add "--ignore-existing" to rsync $target_item = rtrim($target, '/') . '/' . basename($valid_source_path); - if (file_exists($target_item)) { + if (is_dir($valid_source_path) ? is_dir($target_item) : file_exists($target_item)) { $use_rsync_rename = false; break; } }Would you also want to fail fast if any child of a source dir exists under the target (deep conflicts)? I can add an optional preflight scan that bails early for large trees.
260-269: Rsync-rename command: small safety tweaks.
- Keep exclude in the helper or here, but ensure it’s the final rule.
- Consider -a for consistent traversal behavior (no metadata transfer cost occurs).
- Explicitly pass -- in front of paths.
-$cmd = "rsync -r --out-format=%f --info=flist0,misc0,stats0,name1,progress2 --delete --backup --backup-dir=".escapeshellarg($target)." ".quoted_rsync_include($source)." --exclude='*' ".escapeshellarg($empty_dir)." ".escapeshellarg($parent_dir)." 1>$status 2>$error & echo \$!"; +$cmd = "rsync -a --out-format=%f --info=flist0,misc0,stats0,name1,progress2 --delete --backup " + . "--backup-dir=".escapeshellarg($target)." " + . quoted_rsync_include($source)." --exclude='*' " + . "-- ".escapeshellarg($empty_dir)." ".escapeshellarg($parent_dir) + ." 1>$status 2>$error & echo \$!";If you intentionally chose -r over -a for performance or semantics, keep it; the rename path shouldn’t copy metadata anyway. Please confirm.
… from stat commands
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
129-133: Acknowledged prior thread on empty_dir creation.Leaving as-is per earlier discussion; no further action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(3 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix/nchan/file_manager (5)
15-19: LGTM on new constants.Clear naming and trailing slash comment for $empty_dir prevents later surprises.
243-247: Good guard: ensure target is a directory before rsync-rename.Prevents file-to-file moves from hitting backup-dir errors and cleanly falls back to copy-delete.
249-256: Confirm “ignore-existing” semantics vs UI label.Code disqualifies rsync-rename when $exist is set (i.e., adding --ignore-existing). That aligns with “do not overwrite existing files.” Please confirm the UI text maps to this (to avoid operator confusion). If the UI says “Overwrite existing files,” consider flipping the predicate/comment for clarity.
266-270: Rename path command LGTM; inherits metadata via rename(2).Using -r (not -a) is correct here since data isn’t copied; backup-dir receives renames, preserving attributes. Include/exclude list comes from the new helper.
188-191: Resolved — Browse.page no longer references mv actions (5, 10)
Verified: emhttp/plugins/dynamix/Browse.page invokes doAction(4) and doAction(9) for Move (rsync); there are no references to actions 5 or 10 in Browse.page. Note: emhttp/plugins/dynamix/nchan/file_manager still defines cases 5 and 10 (legacy mv handlers) — they are currently unreachable from the UI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/nchan/file_manager (2)
115-127: Critical: escape rsync filter meta-chars and handle symlinks correctly.This function was flagged in a previous review but the issue remains unresolved. Basenames containing rsync filter meta-characters (
*,?,[,],\) will be interpreted by rsync's filter engine, potentially matching unintended files. For example, a file namedtest*.txtwould generate the pattern/test*.txt, which rsync interprets as matchingtest+ any characters +.txt, causing wrong files to be moved.Additionally,
is_dir()follows symlinks, so a symlinked directory would incorrectly receive the/***suffix and be treated as a real directory rather than a single entry.Apply this diff to fix both issues:
+// Escape rsync filter meta characters in a basename +function rsync_escape_component($s) { + // Escape: *, ?, [, ], \ + return preg_replace('/([*?\[\]\\\\])/', '\\\\$1', $s); +} + function quoted_rsync_include($paths) { // note: this function is never called with invalid names because of "if (!$valid_source_path)" $result = []; foreach ($paths as $path) { $valid_path = validname($path); - if (is_dir($valid_path)) { - $result[] = "--include=".escapeshellarg("/".basename($valid_path)."/***"); + $base = rsync_escape_component(basename($valid_path)); + if (is_dir($valid_path) && !is_link($valid_path)) { + $result[] = "--include=".escapeshellarg("/{$base}/***"); } else { - $result[] = "--include=".escapeshellarg("/".basename($valid_path)); + $result[] = "--include=".escapeshellarg("/{$base}"); } } return implode(' ', $result); }
216-216: Check first element of target_device_id for consistency.Line 216 checks
!empty($target_device_id)but line 231 explicitly checks$source_device_id[0]. For consistency and to guard against edge cases wherestatmight succeed but return an empty device ID, verify the first element is non-empty.Apply this diff:
- if (!empty($target_device_id)) { + if (!empty($target_device_id) && !empty($target_device_id[0])) { $use_rsync_rename = true; // assume we can use it, then check for disqualifying conditions
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
244-247: Move target directory check outside the loop.This check validates the same
$targeton every iteration through the sources array. Since the target doesn't change between iterations, move this check outside theforeachloop (e.g., right after line 217) to avoid redundant filesystem calls.Apply this diff:
if (!empty($target_device_id) && !empty($target_device_id[0])) { $use_rsync_rename = true; // assume we can use it, then check for disqualifying conditions + + // target must be a directory + if (!is_dir(rtrim($target,'/'))) { + $use_rsync_rename = false; + } foreach ($source as $source_path) { ... - // target must be a directory - if (!is_dir(rtrim($target,'/'))) { - $use_rsync_rename = false; - break; - }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
243-254: Optional: Move target directory check outside the loop for efficiency.The
is_dir($target)check on line 251 is repeated for every source item but the target doesn't change between iterations.Apply this diff to improve efficiency:
$use_rsync_rename = false; $last_dirname = ''; $target_device_id = []; // for filesystem check, find first existing parent directory $target_for_stat = $target; while (!file_exists($target_for_stat) && $target_for_stat != '/') { $target_for_stat = dirname($target_for_stat); } exec("stat -c %d -- ".escapeshellarg($target_for_stat)." 2>/dev/null", $target_device_id); // check all source paths if (!empty($target_device_id)) { $use_rsync_rename = true; // assume we can use it, then check for disqualifying conditions + + // target must be an existing directory for the backup-dir optimization + if (!is_dir(rtrim($target,'/'))) { + $use_rsync_rename = false; + } foreach ($source as $source_path) { // source path must be valid $valid_source_path = validname($source_path); if (!$valid_source_path) { $use_rsync_rename = false; break; } // filesystem (device) of source and target must be equal $source_device_id = []; exec("stat -c %d -- ".escapeshellarg($valid_source_path)." 2>/dev/null", $source_device_id); if (empty($source_device_id) || $source_device_id[0] != $target_device_id[0]) { $use_rsync_rename = false; break; } - // parent directory of all source paths must be equal (not sure if this is really required, but keeping for now) + // parent directory of all source paths must be equal (required: line 274 uses first source's parent) if (!empty($last_dirname) && $last_dirname != dirname($valid_source_path) ) { $use_rsync_rename = false; break; } $last_dirname = dirname($valid_source_path); - - // target must be a directory - if (!is_dir(rtrim($target,'/'))) { - $use_rsync_rename = false; - break; - } // selected source files and directories must not exist on target when "Overwrite existing files" is not setThis also clarifies why the parent directory check is necessary (it's used on line 274) and makes the "existing directory" requirement more explicit in the comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(3 hunks)
🔇 Additional comments (3)
emhttp/plugins/dynamix/nchan/file_manager (3)
115-134: LGTM! Helper functions correctly implement rsync filter pattern generation.The implementation properly escapes rsync meta-characters and handles symlinked directories as single entities rather than recursing into them. This prevents incorrect file selection during the optimized move operation.
136-139: Initialization is sufficient for the daemon's single-instance execution model.The straightforward directory creation is appropriate given that this daemon runs as a single instance with exclusive access to the path.
206-283: Excellent implementation of the rsync-based rename optimization!The logic correctly validates all preconditions for using rename(2):
- Same filesystem via device ID comparison
- Common parent directory for all sources
- Target is an existing directory
- No conflicts in ignore-existing mode
The optimization path uses
rsync --backup-dirto trigger rename(2) syscalls, while the fallback gracefully handles cross-filesystem and conflict scenarios. All critical issues from past reviews have been properly addressed.
|
A possible alternative to the rsync-rename trick: easy: advanced:
If this is the prefered method, I could rewrite the patch accordingly. But finally I would still remove the actions 5 and 10 from the front end as all relevant checks could - and I even think: should - be done in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/nchan/file_manager (2)
136-139: Consider adding basic safety check for empty_dir creation.While the simplified approach was previously discussed, adding a minimal check to ensure the path is actually a directory (not a file or symlink) would improve robustness without significant complexity:
// create empty directory for optimized move if not exists -if (!file_exists($empty_dir)) { - mkdir($empty_dir); +if (file_exists($empty_dir)) { + if (!is_dir($empty_dir) || is_link($empty_dir)) { + @unlink($empty_dir); + mkdir($empty_dir, 0755); + } +} else { + mkdir($empty_dir, 0755); }This prevents potential failures if the path was created as a file by another process.
195-197: Track frontend cleanup for actions 5 and 10.The comments note that actions 5 and 10 should be removed from Browse.page. Consider opening a follow-up issue to track this frontend cleanup, ensuring the transition to rsync-based moves is complete.
Would you like me to help draft an issue description for tracking the Browse.page changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(4 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
206-283: Well-implemented rsync-rename optimization with thorough validation.The validation logic correctly ensures the optimization is only used when safe:
- Same filesystem check using device IDs
- Common parent directory verification
- Target directory validation
- Conflict detection when --ignore-existing is set
The fallback to standard rsync copy-delete preserves existing behavior. Past critical issues have been properly addressed.
|
I fixed an additional bug in How to reproduce the bug:
if you repeat this process you can spawn an infinite amount of The root cause is that EDIT: realized it based on pid-file: |
Replace complex INI serialization with simple separate .pid file: - Avoids escaping issues (semicolons, hashes, newlines, etc.) - No risk of INI file corruption - Consistent with existing pattern (.active, .status, .error, .pid) - Simpler code with fewer error sources - Atomic writes (single number to file)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
137-141: Optional: harden empty_dir creation/validation.Not required for correctness here, but you can defensively ensure the path is a directory and empty before use. This was discussed earlier; leaving as optional.
-// create empty directory for optimized move if not exists -if (!file_exists($empty_dir)) { - mkdir($empty_dir); -} +// create/validate empty directory for optimized move +if (file_exists($empty_dir) && !is_dir($empty_dir)) { + @unlink($empty_dir); +} +if (!is_dir($empty_dir)) { + @mkdir($empty_dir, 0700, true); +} else { + $it = @scandir($empty_dir); + if ($it !== false && count($it) > 2) { + exec("rm -rf -- " . escapeshellarg(rtrim($empty_dir, '/'))); + @mkdir($empty_dir, 0700, true); + } +}
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/nchan/file_manager (3)
122-135: Escape logic looks good; de‑dupe patterns to avoid redundant --include args.quoted_rsync_include() is correct and safe. Minor improvement: remove duplicates to keep command lines short on bulk selections.
-function quoted_rsync_include($paths) { +function quoted_rsync_include($paths) { // note: this function is never called with invalid names because of "if (!$valid_source_path)" $result = []; - foreach ($paths as $path) { + foreach (array_unique($paths) as $path) { $valid_path = validname($path); $base = rsync_escape_component(basename($valid_path)); if (is_dir($valid_path) && !is_link($valid_path)) { $result[] = "--include=" . escapeshellarg("/{$base}/***"); } else { $result[] = "--include=" . escapeshellarg("/{$base}"); } } return implode(' ', $result); }
214-233: Small robustness tweaks: check stat result element; allow non‑existent dir targets when intent is directory.
- Check element 0 for stat output before enabling the optimization.
- Current target-dir guard disables rsync‑rename when the target directory doesn’t exist yet (even if the user provided a trailing slash). Allow it when the parent exists and the path is directory‑intent.
- if (!empty($target_device_id)) { + if (!empty($target_device_id) && !empty($target_device_id[0])) { $use_rsync_rename = true; // assume we can use it, then check for disqualifying conditions + + // validate directory semantics for target once (supports non-existent dir with trailing slash) + $target_is_dir = is_dir(rtrim($target,'/')); + if (!$target_is_dir && isdir($target) && is_dir(dirname(rtrim($target,'/')))) { + $target_is_dir = true; // directory-intent and parent exists + } + if (!$target_is_dir) { + $use_rsync_rename = false; + }- // target must be a directory - if (!is_dir(rtrim($target,'/'))) { - $use_rsync_rename = false; - break; - } + // target-dir check evaluated once above + if (!$use_rsync_rename) break;Also applies to: 243-263
147-151: PID persistence: add LOCK_EX and basic validation.Use LOCK_EX to avoid write races and ignore malformed PID content.
if (file_exists($pid_file)) { - $pid = trim(file_get_contents($pid_file)); + $pid = trim(@file_get_contents($pid_file)); + if ($pid !== '' && ctype_digit($pid)) { + $pid = (int)$pid; + } else { + unset($pid); + } }- if ($pid !== false) { - file_put_contents($pid_file, $pid); + if ($pid !== false) { + @file_put_contents($pid_file, (string)$pid, LOCK_EX); }Also applies to: 328-331
|
Ok, the bug is still present. Needs more testing... EDIT: Sorry I was wrong. I forgot updating the PR Test Plugin 🙈 The bug is fixed with the persisted PID file! 🎉 proof: devy@3f0eda67c5e1:~$ # opening file manager gui ...
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l file_manager"
3222146 file_manager
devy@3f0eda67c5e1:~$ # transfer files from disk1 to disk2
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l rsync"
3205570 rsync
3205575 rsync
3205578 rsync
devy@3f0eda67c5e1:~$ # closing webgui ...
devy@3f0eda67c5e1:~$ sleep 30
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l file_manager"
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l rsync"
3205570 rsync
3205575 rsync
3205578 rsync
devy@3f0eda67c5e1:~$ # opening file manager gui ...
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l file_manager"
3222146 file_manager
devy@3f0eda67c5e1:~$ ssh root@horus "pgrep -l rsync"
3205570 rsync
3205575 rsync
3205578 rsync
devy@3f0eda67c5e1:~$ ssh root@horus "cat /var/tmp/file.manager.pid"
3205570devy@3f0eda67c5e1:~$ # still only three rsync processes, bug is fixed |
- Initialize outside the main loop to ensure correct state handling for move/cleanup phases - If file_manager restarts and PID is loaded from file and move operation is still active, enable to cleanup empty directories afterwards - Clarify comment for INI job parameters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
264-268: Move target directory check outside the source loop for efficiency.The target directory validation at Line 265 is executed once per source item but doesn't depend on any source. Moving it before the
foreachloop would eliminate redundant checks.Apply this diff to optimize the check placement:
exec("stat -c %d -- ".escapeshellarg($target_for_stat)." 2>/dev/null", $target_device_id); // check all source paths if (!empty($target_device_id)) { $use_rsync_rename = true; // assume we can use it, then check for disqualifying conditions + + // target must be a directory + if (!is_dir(rtrim($target,'/'))) { + $use_rsync_rename = false; + } foreach ($source as $source_path) { // source path must be valid $valid_source_path = validname($source_path); if (!$valid_source_path) { $use_rsync_rename = false; break; } // filesystem (device) of source and target must be equal $source_device_id = []; exec("stat -c %d -- ".escapeshellarg($valid_source_path)." 2>/dev/null", $source_device_id); if (empty($source_device_id) || $source_device_id[0] != $target_device_id[0]) { $use_rsync_rename = false; break; } // parent directory of all source paths must be equal (not sure if this is really required, but keeping for now) if (!empty($last_dirname) && $last_dirname != dirname($valid_source_path) ) { $use_rsync_rename = false; break; } $last_dirname = dirname($valid_source_path); - - // target must be a directory - if (!is_dir(rtrim($target,'/'))) { - $use_rsync_rename = false; - break; - } // selected source files and directories must not exist on target when "Overwrite existing files" is not set
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(5 hunks)
🔇 Additional comments (7)
emhttp/plugins/dynamix/nchan/file_manager (7)
15-20: LGTM: Clear variable declarations with good documentation.The new global variables are well-named and appropriately placed in
/var/tmp. The comment about the trailing slash requirement for rsync is helpful.
116-135: LGTM: Proper rsync filter escaping and symlink handling.These functions correctly address the critical issues raised in past reviews by:
- Escaping rsync filter meta-characters (
*,?,[,],\) to prevent unintended pattern matching- Treating symlinked directories as single entries (not recursively included with
/***)
151-154: LGTM: Correct PID persistence for restart resilience.The logic correctly reads the persisted PID only when needed (
!$pid), enabling file_manager to resume operations after restarts. Thefile_existscheck prevents errors on fresh starts.
210-213: LGTM: Resume state correctly preserves cleanup behavior.The addition of
if ($move === null) $move = true;ensures that empty-directory cleanup runs after job completion even when file_manager is restarted mid-operation. This addresses the concern raised in past reviews.
282-296: LGTM: Correct implementation of both optimization and fallback paths.The rsync-rename optimization cleverly uses
--backup-dirwith an empty source to trigger rename(2) syscalls, while the fallback path uses standard rsync copy-delete. Both commands are properly escaped and handle their respective use cases correctly. The explanatory comment is helpful.
334-337: LGTM: PID persistence correctly integrated.The PID is properly written to the file after spawning any background process, enabling restart resilience. While there's no explicit error handling for
file_put_contents, this is acceptable since:
- PID persistence is a best-effort optimization
- Stale PIDs are handled by
pgrep()validation at Line 332- The cleanup paths correctly remove the PID file
326-326: Also applies to: 361-361
|
deleted wrong assumption Everything should be fine now. EDIT: Nah, still a bug present:
Result: This differs from the behaviour of the original file_manager and I wonder why. While using the original version, the status bar is empty as long the file_manager is not running. Really strange... |
|
Ok, found the main reason for the different behaviors. One server gets this response from /sub/filemanager: and the other: As you can see the output is reversed (last transfer speed and current transferred file). I think this happens, when really small files are transferred. Then the order can be different. And because of the different order, this code does not work as expected: text = text[1].split(/\s+/);But even if this bug is solved, we will still see outdated messages in the footer, so I changed the The next step is to enhance the |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
etc/rc.d/rc.nginx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
PR: unraid/webgui#2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
etc/rc.d/rc.nginx
…ager location block
|
Note: A different solution to d26390c would be to start EDIT: Ok, waited 2 minutes before re-opening the webgui and the footer message keeps empty. I tend to reduce the timeout to 30s. By that we should never see outdated messages as long |
|
I changed the PR state to Draft as long this bug is not solved: Still working on that... |
Multiple improvements to file_manager reliability and status handling:
1. Fix rsync output parsing bug
- rsync returns filename and progress lines in unpredictable order
- Old JS parsing: text[1].split() assumed fixed line order
- New approach: Simplified shell parsing (tail + tr) in PHP
- parse_rsync_progress() helper function for consistent handling
2. Migrate status updates to JSON format
- Structure: {action: int, text: [file_text, progress_text?]}
- PHP: mb_strimhalf() for UTF-8-safe middle truncation
- PHP: htmlspecialchars() escaping, JS adds icons client-side
- Minimizes WebSocket overhead (no HTML in payload)
- Universal handler in BrowseButton.page (6 lines)
3. Fix stale status messages (Nchan caching issue)
- rc.nginx: nchan_message_timeout 30s for /pub/filemanager
- Status expires automatically after 30s
- Prevents displaying outdated progress status in footer
4. Remove obsolete move operation variants (case 5/10)
- file_manager: Remove case 5/10 comments (mv-based move)
- Browse.page: Remove action++ logic from doAction/doActions
- BrowseButton.page: Remove case 5/10 from dfm_makeDialog()
- All moves now use case 4/9 with automatic rsync-rename optimization
Stop after first match to avoid processing entire file
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
PR: unraid/webgui#2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🔇 Additional comments (15)
emhttp/plugins/dynamix/nchan/file_manager (15)
15-20: LGTM: Global variable declarations are appropriate.The use of
/var/tmpfor temporary state files is correct, and the trailing slash requirement for$empty_diris properly documented.
64-76: LGTM: Time conversion functions are correct.The regex validation and null return for invalid input in
time_to_secondsis appropriate. Both functions use correct time arithmetic.
78-128: LGTM: ETA calculation with hysteresis is well-implemented.The function correctly handles:
- Unit conversion for both transferred size and speed
- Edge case of 0% with fallback to last known ETA
- Hysteresis blending (70% last ETA, 30% calculated) to smooth fluctuations
- Division-by-zero protection with the
$percent_val > 0guard
130-177: LGTM: Progress parsing is robust and well-structured.The function correctly:
- Distinguishes between rsync's ETA and elapsed-time output formats
- Applies hysteresis by storing and reusing the last rsync ETA
- Extracts filenames while filtering out progress lines and empty lines
- Properly escapes HTML output with
htmlspecialcharsandENT_QUOTES
238-256: LGTM: Rsync filter escaping and symlink handling are correct.The functions properly:
- Escape rsync filter metacharacters (*, ?, [, ], ) before shell escaping
- Distinguish real directories from symlinked directories using
!is_link()- Apply
/***recursion only to real directories, treating symlinks as single entries
258-262: Acceptable: Empty directory creation is safe in this controlled context.While defensive checks (verifying it's not a symlink/file, explicit permissions, emptiness validation) would be more robust, the current approach is acceptable because:
/var/tmpis root-controlledfile_managerruns as root with exclusive access to this path- The risk of interference is minimal in this environment
273-279: LGTM: PID restoration logic enables proper resume after restarts.Reading the PID from disk when
!$pidallows the file_manager to resume monitoring background processes after a restart, preventing duplicate operations.
303-313: LGTM: Delete operation now returns structured JSON status.The JSON response format
{action, text: [string]}is consistent with the new progress reporting pattern, and proper HTML escaping is applied.
327-344: LGTM: Copy operation properly structured with JSON progress reporting.The command construction is correct:
- Process substitution
>(...)is safe (Unraid uses bash as/bin/sh)- Properly backgrounded with
& echo $!for PID capture- Uses
stdbuf -o0for unbuffered output- JSON response includes parsed rsync progress
Based on learnings.
350-372: LGTM: Move operation status handling correctly distinguishes cleanup from transfer phase.The state management properly:
- Sets
$delete_empty_dirs = trueon resume to handle the rsync copy-delete case- Returns simple status during cleanup (when
false)- Returns parsed progress during the transfer phase
- Acknowledges that find errors for rsync-rename are benign (commented on lines 353-356)
380-448: LGTM: Rsync-rename validation logic is comprehensive and correct.The validation properly checks all requirements:
- Same filesystem using device ID (
stat -c %d) rather than mount point- Same parent directory for all sources
- Target is a directory (not a file)
- No conflicts when "ignore existing" is set
- Target is not a subdirectory of any source
All critical issues from previous reviews have been addressed.
455-468: LGTM: Both rsync commands are correctly constructed and backgrounded.The commands properly:
- Use process substitution for output handling (safe with bash)
- Background with
& echo $!for PID capture- Set
$delete_empty_dirs = trueonly for copy-delete (which needs cleanup)- Use
--no-inc-recursiveto prevent temporary root:root 0700 permissionsThe rsync-rename optimization uses the empty-dir trick with
--delete --backup --backup-dirto trigger rename(2) syscalls.
505-510: LGTM: PID persistence correctly implemented.The
pgrep()validation ensures only active PIDs are stored, andfile_put_contents()writes the PID to disk for restoration after restart.
512-537: LGTM: Cleanup phase properly handles post-move directory removal.The logic correctly:
- Runs
find -type d -empty -print -deleteonly when needed (rsync copy-delete)- Suppresses errors with
2>$null(handles rsync-rename case where dirs already removed)- Transitions state to
falseto prevent re-running cleanup- Cleans up all state files including the new
$pid_file- Resets
$delete_empty_dirstonullfor the next operation
496-501: LGTM: Kill handler properly cleans up PID file and state.The cleanup is consistent with normal completion, ensuring the PID file and
$delete_empty_dirsstate are reset when an operation is cancelled.
|
Found a Bug: 
This should not happen as both directories are not a subdirectory of each other. This check is wrong: dirname($valid_source_path)Should only be done if source is a file path and not a directory path. Will fix this... |
|
Meh... "Cannot move directory into its own subdirectory" bug is still NOT fixed! EDIT: Ah... last time forgot to push 🙈 |
The subdirectory check was incorrectly using dirname() on source directories, causing false positives. For example, moving /mnt/disk7/Backups/Smartphone to /mnt/disk7/Backups/Marc/ would fail because dirname() reduced the source to /mnt/disk7/Backups, making it appear that Marc was a subdirectory of the source's parent. Now correctly checks: - For directories: use the directory path itself - For files: use the containing directory (dirname) This ensures the check properly prevents moving a directory into its own subdirectories while allowing valid moves to sibling directories.
mb_strimhalf: - Fixed substring calculation for right side (was -1-$half, now -$half) - Properly account for marker length in width calculation to ensure output never exceeds specified width calculate_eta: - Add minimum threshold check (> 1.0 instead of > 0) to avoid unrealistically large ETAs from very small speed values
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
80-130: Optional: Add input validation to calculate_eta for robustness.The function assumes well-formed inputs from rsync progress output. While the current implementation handles malformed inputs reasonably (intval/floatval return 0 for invalid strings, triggering fallback logic), explicit validation would make the function more robust and easier to debug.
Consider adding input validation at the function start:
function calculate_eta($transferred, $percent, $speed, $last_rsync_eta_seconds = null) { // Validate inputs if (empty($transferred) || empty($percent) || empty($speed)) { return "N/A"; } // Convert transferred size to bytes // ... rest of function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(7 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ljm42
PR: unraid/webgui#2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
PR: unraid/webgui#2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
PR: unraid/webgui#2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-02-10T20:28:41.294Z
Learnt from: ljm42
PR: unraid/webgui#2011
File: etc/rc.d/rc.S:0-0
Timestamp: 2025-02-10T20:28:41.294Z
Learning: When modifying files that are managed outside the repository (like `/etc/php.d/errors-php.ini`), always include a file existence check to ensure the script doesn't fail if the file is missing.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
PR: unraid/webgui#2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
PR: unraid/webgui#2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:13:53.283Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
PR: unraid/webgui#2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🔇 Additional comments (6)
emhttp/plugins/dynamix/nchan/file_manager (6)
382-450: LGTM: rsync-rename optimization logic is well-designed.The decision tree for enabling the rsync-rename optimization is comprehensive and safe:
- ✓ Device ID comparison (not mount paths) ensures same-filesystem
- ✓ Parent directory consistency check prevents edge cases
- ✓ Target-is-directory check prevents file-to-file rename issues
- ✓ Conflict detection when
--ignore-existingis set- ✓ Subdirectory relationship check prevents rsync
--backup-dirissuesThe fallback to rsync copy-delete ensures robustness when any condition fails. Past review concerns have all been addressed.
354-359: Acceptable trade-off: empty directory cleanup errors suppressed rather than state persisted.The
$delete_empty_dirsstate management handles the rsync copy-delete cleanup phase:
- Set to
trueonly for rsync copy-delete (line 464)- Fallback to
trueon restart ifnull(line 359)finderrors suppressed via2>$null(line 516)Edge case: If file_manager restarts after a fast rsync-rename completes, the fallback will trigger cleanup on non-existent directories, but errors are suppressed. Given rename(2) operations typically complete in <10 seconds and the narrow restart window, this trade-off (simple error suppression vs. complex state persistence) is reasonable and maintainable.
Also applies to: 464-464, 515-517
307-310: LGTM: JSON-encoded status responses are well-structured and secure.The transition from plain text to structured JSON responses provides:
- ✓ Consistent schema with
actionandtextfields- ✓ XSS protection via
htmlspecialchars(..., ENT_QUOTES, 'UTF-8')- ✓ Rich progress information via
parse_rsync_progress()- ✓ Frontend can distinguish action types and handle accordingly
The implementation is clean and maintains backwards compatibility by keeping the status messages user-friendly.
Also applies to: 331-334, 363-366, 370-373
57-64: LGTM: Helper functions are well-designed and address past review concerns.mb_strimhalf (lines 57-64): Correctly truncates strings in the middle, keeping both ends visible. The calculation
mb_substr($text, -($available - $half))properly extracts the right portion.parse_rsync_progress (lines 132-179): Sophisticated progress parsing with ETA hysteresis smoothing. The use of
tac --beforeandgrep -m1efficiently extracts the most recent progress line without loading the entire status file into PHP. The hysteresis blend (70% last rsync ETA, 30% calculated) prevents ETA fluctuations.rsync_escape_component/quoted_rsync_include (lines 240-258): Properly escapes rsync filter metacharacters (
*,?,[,],\) and handles symlinked directories as single entries (not with/***). Addresses the critical filtering issue from past reviews.Also applies to: 132-179, 240-258
278-281: LGTM: PID persistence via separate file is a clean solution.The separate PID file approach (lines 278-281 read, lines 509-512 write) elegantly solves the restart-survival problem:
- ✓ Atomic write of a simple value
- ✓ No INI serialization complexity
- ✓ Consistent with
.status,.errorfile pattern- ✓ Properly cleaned up on kill (line 500) and completion (line 536)
This addressed the fragile INI serialization concerns from past reviews.
Also applies to: 509-512
276-276: I need to search more specifically for where these variables are set and how the INI file is written. Let me look for the specific variables and the serialization method:I need to find the complete picture of how Control.php writes the INI file and where the specific variables come from. Let me search more comprehensively:Based on the available evidence, I can now consolidate my findings and generate the final review response.
Verify Control.php INI file writing implementation.
The original concern about potential command injection through INI variables is partially supported, but the verification is inconclusive. The script shows Control.php extracts POST parameters and places them into a
$data[]array, but the output was cut off before showing how this array is written to the INI file.Key uncertainties:
- Whether values are properly quoted and escaped when written to INI (critical for safety)
- The origin of
$H,$sparse,$exist,$mkpathvariables—they're absent from the Control.php snippet and may be system-generated rsync flags rather than user-controlled- Whether the htmlspecialchars_decode(rawurldecode()) treatment is sufficient for INI context
Check how the
$data[]array is serialized to/var/tmp/file.manager.activein Control.php's 'file' case: confirm values are quoted and that special characters (;,#,=, quotes, backslashes) are properly escaped before being written.
The rsync rename operation uses --delete flag which outputs 'deleting filename/' in the status. This could cause user confusion as it looks like files are being deleted rather than moved. Filter out the 'deleting ' prefix with sed before writing to status file to show a cleaner 'Moving... filename/' message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
80-130: Minor: Handle 100% completion case explicitly.When
$percent_valreaches exactly 100, the function returns "N/A" (line 129) instead of "0:00:00". Consider handling this explicitly:if ($percent_val == 0) { if ($last_rsync_eta_seconds !== null && $last_rsync_eta_seconds > 0) { return seconds_to_time($last_rsync_eta_seconds); } return "N/A"; } + + if ($percent_val >= 100) { + return "0:00:00"; + } - if ($percent_val > 0 && $percent_val < 100 && $speed_bytes > 1.0) { + if ($speed_bytes > 1.0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(7 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ljm42
PR: unraid/webgui#2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
PR: unraid/webgui#2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
PR: unraid/webgui#2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-02-10T20:28:41.294Z
Learnt from: ljm42
PR: unraid/webgui#2011
File: etc/rc.d/rc.S:0-0
Timestamp: 2025-02-10T20:28:41.294Z
Learning: When modifying files that are managed outside the repository (like `/etc/php.d/errors-php.ini`), always include a file existence check to ensure the script doesn't fail if the file is missing.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
PR: unraid/webgui#2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
PR: unraid/webgui#2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:13:53.283Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
PR: unraid/webgui#2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🔇 Additional comments (14)
emhttp/plugins/dynamix/nchan/file_manager (14)
57-64: LGTM: Correct middle-truncation implementation.The substring length calculation correctly ensures the result never exceeds
$width. The fix to use-($available - $half)instead of the original-1 - $halfresolves the past review concern.
66-78: LGTM: Clean time conversion utilities.Both functions handle their conversions correctly with proper validation and formatting.
132-179: LGTM: Well-designed progress parser with XSS protection.The function correctly:
- Uses
tacwith--beforeto avoid line concatenation issues- Uses
grep -m1to capture only the most recent line (addressing past review feedback)- Distinguishes between rsync ETA and elapsed time lines
- Applies hysteresis to stabilize ETA
- Protects filename output with
htmlspecialcharsagainst XSS
240-258: LGTM: Proper rsync filter escaping and symlink handling.The implementation correctly:
- Escapes rsync meta-characters (*, ?, [, ], ) to prevent unintended filter matches
- Treats symlinked directories as single entries (not recursive)
- Wraps patterns in
escapeshellargfor shell safetyThis addresses the critical security issue flagged in past reviews.
278-281: LGTM: PID persistence enables restart resilience.Reading the PID from disk allows the file_manager to resume monitoring background operations after a restart, addressing the duplicate-rsync bug mentioned in PR objectives.
304-315: LGTM: Structured JSON status with XSS protection.The JSON format provides better structure for frontend parsing, and
htmlspecialcharsproperly sanitizes the filename output.
328-346: LGTM: Proper backgrounding and progress streaming.The rsync command correctly:
- Uses process substitution for real-time progress (safe per line 271 comment)
- Backgrounds with
& echo $!to capture PID (fixing past review issue)- Uses
stdbuf -o0to disable buffering for immediate progress updates- Converts
\rto\nfor line-based parsing
354-374: Acceptable state management with documented trade-off.Setting
$delete_empty_dirs = trueon resume (line 359) enables the cleanup phase even for rsync-rename operations that don't need it. The error suppression on line 517 (2>$null) prevents user-visible errors, and the speed of rename(2) operations makes the race window minimal.This pragmatic approach was discussed in past reviews and is acceptable given the performance characteristics.
382-450: LGTM: Comprehensive validation for rsync-rename optimization.The validation logic correctly checks all requirements:
- ✓ Finds first existing parent for target stat
- ✓ Uses device ID (
%d) for same-filesystem check (not mount path)- ✓ Validates all source paths
- ✓ Checks source and target on same device
- ✓ Ensures all sources share same parent directory
- ✓ Disqualifies file-to-file moves (target must be directory)
- ✓ Checks for conflicts when overwrite is disabled
- ✓ Prevents moving into subdirectory of source
All critical issues from past reviews have been addressed.
452-461: LGTM: Clever rsync-rename implementation using --delete and --backup-dir.The command correctly leverages rsync semantics to trigger rename(2) syscalls:
- Syncs empty directory to parent directory
--deleteremoves items from parent_dir (selected by include filters)--backup --backup-dir=$targetmoves deleted items to target instead- Result: same-filesystem rename, supporting merge and overwrite
The
sed 's/^deleting //'appropriately changes "deleting" to not confuse users.
463-471: LGTM: Proper rsync copy-delete with permission fix.The fallback path correctly:
- Sets
$delete_empty_dirs = trueto trigger cleanup after transfer- Uses
--remove-source-filesto delete sources after copy- Includes
--no-inc-recursiveto prevent premature directory creation with temporary root:root 0700 permissions (which would break SMB access)- Properly backgrounds the command
The trade-offs of
--no-inc-recursive(higher RAM, initial delay) are acceptable to avoid breaking share access during transfer.
501-503: LGTM: Complete cleanup on kill.Including
$pid_filein the cleanup ensures no stale PID persists after termination, and resetting$delete_empty_dirsto null returns to the initial state.
510-513: LGTM: Simple and effective PID persistence.Writing the PID after each loop iteration ensures it survives file_manager restarts, enabling the daemon to resume monitoring background operations.
516-540: LGTM: Correct cleanup phase and completion handling.The logic correctly:
- Triggers empty directory cleanup after rsync copy-delete (line 516-519)
- Suppresses find errors with
2>$nullfor rsync-rename edge cases- Transitions through cleanup phase (
delete_empty_dirs = false)- Performs final cleanup including
$pid_fileremoval- Resets all state variables
The state machine is properly implemented.
- Prevents rsync from creating directory structure with temporary root:root permissions - Ensures proper access for unraid users through /mnt/user/sharename - Applied to both copy and move operations for consistency
|
I added |
|
My PR changes the popup size of the editor. Will check that: EDIT: Ok, this was not related to this PR. It happened because I installed the PR Plugin in Unraid 7.1.4, while the PR plugin contained much newer .page files. After updating my dev server to Unraid 7.2 the popup dimensions are as expected: |
|
Sorry, I accidentally closed the PR. Will push a small commit to trigger the check again. |
- clarify that timestamps represent ETA vs elapsed time - improve filename vs progress line distinction in regex comment - remove unnecessary empty line
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
304-314: Consider error handling for parse failures in JSON status generation.The delete operation generates JSON status but doesn't handle potential failures in
tailorexec(). While unlikely to fail, adding a try-catch or validation would make the status reporting more robust.Example approach:
if (!empty($pid)) { - $reply['status'] = json_encode([ - 'action' => $action, - 'text' => [htmlspecialchars(mb_strimhalf(exec("tail -1 $status"), 70, '...'), ENT_QUOTES, 'UTF-8')] - ]); + $last_line = @exec("tail -1 $status") ?: _('Processing...'); + $reply['status'] = json_encode([ + 'action' => $action, + 'text' => [htmlspecialchars(mb_strimhalf($last_line, 70, '...'), ENT_QUOTES, 'UTF-8')] + ]);The same pattern applies to lines 330-333, 365-368, and 372-375 for consistency across all JSON status outputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/file_manager(7 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-02-10T20:28:41.294Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2011
File: etc/rc.d/rc.S:0-0
Timestamp: 2025-02-10T20:28:41.294Z
Learning: When modifying files that are managed outside the repository (like `/etc/php.d/errors-php.ini`), always include a file existence check to ensure the script doesn't fail if the file is missing.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-08-13T03:13:53.283Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🔇 Additional comments (1)
emhttp/plugins/dynamix/nchan/file_manager (1)
15-552: Excellent implementation—critical issues from past reviews have been addressed.The rsync-rename optimization is well-implemented with proper safety checks:
- ✅ Device ID comparison (%d) for same-filesystem detection
- ✅ Filter meta-character escaping in
rsync_escape_component()- ✅ Commands properly backgrounded with
& echo $!- ✅ Persistent PID tracking via
$pid_file- ✅ Target validation (directory check, subdirectory prevention)
- ✅ Process substitution documented as safe on Unraid (bash as /bin/sh)
- ✅ Error suppression in cleanup phase (
2>$null)The helper functions (
parse_rsync_progress,calculate_eta,mb_strimhalf) are well-designed with hysteresis for stable ETA reporting and proper multibyte string handling. The JSON status outputs provide structured progress data.Your pragmatic decisions on edge cases (empty_dir lifecycle, delete_empty_dirs race window) are reasonable given the extensive testing documented in the PR objectives.
|
Feedback: This is the final state of this PR: As mentioned I will continue with a new PR after this has been merged. |
|
Hey @mgutt! Thanks so much for this contribution. We're going to get this PR merged for 7.3! I am also tracking a couple of other bugs that we may put a bug bounty on with the file manager, if you're interested. I'll try to get those into our tracking system later this week - you'll be able to find them here: Unraid OS Feature & Bug Bounties |
|
@elibosley |
|
thank you! |
…ations This PR completes the migration from HTML string responses to a unified JSON API format for all file_manager operations, building on the work started in PR unraid#2429. Backend changes (nchan/file_manager): - Migrate remaining actions to JSON format: * case 0 (create folder): {action: 0, text: ['Creating...']} * case 2/7 (rename): {action: 2/7, text: ['Renaming...']} * case 11 (chown): {action: 11, text: ['Updating... filename']} * case 12 (chmod): {action: 12, text: ['Updating... filename']} * case 15 (search): {action: 15, text: ['Searching... count']} * completion: {action: X, text: ['Done']} for all non-search operations - Convert search results to JSON structure: * {action: 15, results: [{location, path}, ...]} * Replaces legacy #cat# string format with structured data - Convert cat() function to return array of objects instead of string - Send errors as plain text (not HTML) for safer frontend handling Frontend changes (BrowseButton.page): - Implement unified JSON parser for all status updates - Show footer only for progress-tracking operations (delete, copy, move, search) - No footer for quick operations (create, rename, chown, chmod) - Parse search results from JSON structure: * Display as properly paired dt/dd elements * Clear previous results before showing new ones * Show 'No files found' for empty results - Remove all legacy string parsing code - Use .text() with white-space:pre-line for error display (prevents XSS) - Add error logging for JSON parse failures Frontend changes (Browse.page): - Add try-catch for Control.php JSON parsing - Handle empty responses gracefully All file_manager operations now use a consistent, type-safe JSON API with proper error handling and no legacy code paths remaining.
4 participants
Description
This pull request replaces
mvwith an optimizedrsyncthat usesrename(2)syscalls for same-filesystem moves.Problems
mvcan't mergeYou can't move files on the same disk when the destination directory already exists and contains files, even with "Overwrite existing files" enabled.
Test Case
Then try to move
/mnt/disk1/sharename/footo/mnt/disk1/sharename/barwhile enablingOverwrite existing files.Although
lorem.txtdoes not exist in/mnt/disk1/sharename/bar/foo, it won't move the file. Insteadmvreturns this error:Existing bug in
mvdecision logicIn
Browse.pagethere is a bug in the decision logic to usemvinstead ofrsync:The Problem: If source[0] is /boot/config/folder/, the path becomes:
Initial:
['boot', 'config', 'folder', ''](length: 4)After
path.pop() || path.pop(): ['boot', 'config'](length: 2)Condition
path.length > 2fails: usesrsyncinstead ofmvThis causes same-disk moves to incorrectly use
rsynccopy-delete instead ofmv.Solution: Optimized
rsyncwithrename(2)Instead of fixing the JavaScript bug and keeping
mv's limitations, this PR implements an optimizedrsyncapproach that:rename(2)syscalls when moving files on the same filesystemmvdoes not supportrsyncoptionsBrowse.pageas themvactions (5 and 10) are still handled (theaction++related code inBrowse.pagecan be removed now or later)How it works
The optimization uses
rsync's--backupand--deleteoptions with an empty source directory to trigger rename operations instead of copy-delete:Safety checks
The optimized
rsynconly activates when:When these conditions aren't met, it falls back to standard
rsynccopy-delete.Summary by CodeRabbit
Refactor
New Features
Bug Fixes / UX