tjscience / RoboSharp

RoboSharp is a .NET wrapper for the awesome Robocopy windows application.
MIT License
216 stars 67 forks source link

MoveFilesAndFolders - "Cannot delete source Directory" error #151

Open RFBomb opened 2 years ago

RFBomb commented 2 years ago

My project is taking ALL files and folders EXCEPT 2 folders from the Root of a USB stick, and stuffing them into a backup folder on the root of the USB.

For example:

Source: E:\ Destination: $"E:\USB_BACKUP\{DateTime.Now.ToString("yyyy-MM-dd_hh_m_tt")};"

E:\FolderIgnore1
E:\USB_BACKUP
E:\SomeOtherFolderWithFiles
E:\SomeFile

turns into

E:\FolderIgnore1
E:\USB_BACKUP
E:\USB_BACKUP\CurrentDateTimeFolderName
E:\USB_BACKUP\CurrentDateTimeFolderName\SomeOtherFolderWithFiles
E:\USB_BACKUP\CurrentDateTimeFolderName\SomeFile

All that works, but RoboCopy reports an error that it cannot delete the source directory, which is typically done during the 'MoveFilesAndFolders'. I know WHY its erroring - It can't delete the source from the drive because the source IS the root of the drive.

Any idea how to prevent that error being generated?

RFBomb commented 2 years ago

Also, it apparently moved the 'System Volume Information' folder o.0

PCAssistSoftware commented 2 years ago

Can you upload a job file so I can see exactly what settings you are using, and a example directory structure so I can try and replicate it here?

As for "System Volume Information" then yes I would expect it to move that as well if the source is the root of a drive and you haven't added it as an excluded directory, also happens with $RECYCLE.BIN

RFBomb commented 2 years ago

BootVersion.zip JOB.zip

( I already added the SVI folder to ignored list) Unzip 'Bootversion' onto the root of a USB drive. After backup is complete, it should exist within the 'USB_BACKUP' folder in a subfolder. Unzip BootVersion onto the root again, run the backup again and it goes into a new subfolder (Obviously you have to change the folder name, but my code does this automatically via DateTime.Now as the folder name)

PCAssistSoftware commented 2 years ago

I will investigate further tomorrow as getting late here now, but I see what you mean, I also see that get the exact same error running from command line using RoboCopy, so at least we know it is a RoboCopy issue rather than a RoboSharp issue. Pretty certain I have seen something similar in the past when using RoboCopy for server data migrations.

image

RFBomb commented 2 years ago

Two potential solutions:

https://serverfault.com/questions/167723/robocopy-how-to-move-the-content-of-a-directory-but-keep-the-directory/167748

If this is a know issue, and really the only time it occurs is when using the /MOVE switch, we could sanitize it automatically. Do a check for that switch, and if it exists check if the source is the root of a drive or the destination is a sub folder of the source (both scenarios would result in the error), the amend the command to avoid the error altogether.

PCAssistSoftware commented 2 years ago

Neither solution works when using RoboCopy from command line - not yet tried in RoboSharp

PCAssistSoftware commented 2 years ago

What type of file system does the USB drive have?

The one I was testing with and having the same error as you with was formatted as FAT32?

I converted it to NTFS and it now works fine without any errors at all :)

convert e: /fs:ntfs

RFBomb commented 2 years ago

2GB Fat32 disks. These are boot software for a system, so reformatting to a different format won't work.

I'll play around and see if I can't resolve it

PCAssistSoftware commented 2 years ago

Ahh okay, thats a shame - definitely a permission issue relating to ACL on root of a drive hence working on NTFS and not on FAT32 - unfortunately RoboCopy works so much better on NTFS as half of its features are designed for that type of file system.

Let me know how you get on though

PCAssistSoftware commented 2 years ago

P.S. Just out of interest it seems for some switches such as /MIR that System Volume Information is automatically excluded according to the logs - never knew this

named (-1) - E:\System Volume Information\

RFBomb commented 2 years ago

Ok, so heres what I think is happening. I took a single job file and performed it over cmd prompt multiple times, with a subfolder of the C drive as the source and destination being a subfolder of the source. Not once did I get the error for deleting the source directory.

I think RoboCopy is using DirectoryInfo.Delete(false), which ONLY removes empty directories. BUT, if you don't have delete permission, I think gives the same alarm because ( I think) it checks for delete permission prior to attempting to delete.

So, when moving to a fat USB, that is the cause of the error, while in ntfs it doesn't display anything because it succeeds the permisson check, but fails due to the destination folder residing inside of it.

Interestingly, excluding the source directory does nothing. (I excluded E:\, all files and folders still copied, still received the error).

I even put an empty file in the folder, excluded it, and STILL received the error. So this makes me think my guess about order of operations above is accurate. It detects it doesn't have permission to delete and errors out prior to realizing it shouldn't be trying to delete it anyway.

It could also have to do with the fact that its the root of a drive, and nothing to do with the format of the drive. Either way, I think the solution for this is relatively easy if we wanted to 'fix' the bug in robocopy by just preventing showing it in robosharp.

image That bit of code prevents the OnError event showing up when this fault occurs (Deleting root of a drive - all other paths will still raise the event) It will still show in the logs though, and since robocopy writes to log files outside of robosharp's control, I didn't prevent it being added to these loglines either.

PCAssistSoftware commented 2 years ago

Interestingly, excluding the source directory does nothing. (I excluded E:, all files and folders still copied, still received the error).

Yes I had that same thought and tried it as well.

I think your logic above makes sense and I agree with why you think it is happening.

In my opinion perhaps rather than stopping it showing in OnError perhaps just get it to show a better explanation of why the error occurred e.g. append a "friendly" error message explaining what we know to it?

RFBomb commented 2 years ago

The check could be done after the description 'Access is denied' is added to it, and we append " - Safe to ignore: /MOVE with root of drive as source causes this fault (cannot delete root of drive)"

Or something. Not really sure how to describe it for public consumption.

Since this is going to be an issue, and my app currently throws a pop up showing the error and suspends the copy process, I'm going to filter it out manually in my event handler.

(I don't think it's worth it for a single error like this, but if more 'known non-error errors' occur, maybe worthwhile to add a static class with filters to detect and ignore them raising the event/append friendly description, obviously on basis.

Defaults would obviously be show them with friendly, with bool toggle to just not raise event.

PCAssistSoftware commented 2 years ago

Yes that would make sense and certainly help new users understand when an error is not really an error so to speak.

It is hard to know how to describe it for public consumption - you and I understand it as we have discussed it, but new people coming to it may not, perhaps as you say just perhaps prevent it showing for now then but with plenty of comments in the code and perhaps a link back to this issue maybe?

RFBomb commented 2 years ago

Yea, linking to this thread is easy enough to do, with a concise summary of what it prevents raising as well.

Let's get @tjscience opinion on that code screenshot before I submit a PR with it.

I think that unless we notice more 'nonsense errors' like that, creating a whole class with toggles and descriptions wouldn't make sense. As far as I'm aware, this is the only one that exists, and it's a quirk of using FAT32 root as source and with the move tag, so fairly niche I would think. (A network path root as source would also likely introduce the issue, but either way erroring due to inability to delete a drive root is likely an oversight by robocopy devs)

PCAssistSoftware commented 2 years ago

Yea, linking to this thread is easy enough to do, with a concise summary of what it prevents raising as well.

Let's get @tjscience opinion on that code screenshot before I submit a PR with it.

I think that unless we notice more 'nonsense errors' like that, creating a whole class with toggles and descriptions wouldn't make sense. As far as I'm aware, this is the only one that exists, and it's a quirk of using FAT32 root as source and with the move tag, so fairly niche I would think. (A network path root as source would also likely introduce the issue, but either way erroring due to inability to delete a drive root is likely an oversight by robocopy devs)

Completely agreed on both counts :)