eden cli: Retry rmdir after killing processes (on Windows) (redo)

Summary:
Now that we have the ability to kill processes from the previous diff, this one adds the logic to retry if the process kill is successful.

(note: Replacing a previous diff that was giving me some rebase pain)

Reviewed By: MichaelCuevas

Differential Revision: D54070610

fbshipit-source-id: 7a085c7eccba2bf1544de55c3b5dca7a53533ef3

eden/fs/cli/doctor/test/lib/fake_fs_util.py

@@ -24,3 +24,6 @@ class FakeFsUtil(FsUtil):
         # A made up filesystem with 50% free, but with other fields
         # defaulted from an EdenFS mount on Linux.
         return shutil._ntuple_diskusage(self.total, self.used, self.free)
+
+    def rmdir(self, path: str, keep_root: bool) -> bool:
+        return True

eden/fs/cli/file_handler_tools.py

@@ -23,14 +23,14 @@ class FileHandlerReleaser(ABC):
         pass
 
     @abstractmethod
-    def check_handle(self, mount: Path) -> None:
+    def check_handle(self, mount: Path) -> bool:
         """Displays processes keeping an open handle to files and if possible, offers to terminate them."""
-        pass
+        return False
 
     @abstractmethod
-    def try_release(self, mount: Path) -> None:
+    def try_release(self, mount: Path) -> bool:
         """If a handle tool exist, use it to display info to the user with check handle."""
-        pass
+        return False
 
 
 if sys.platform == "win32":
@@ -40,18 +40,15 @@ if sys.platform == "win32":
 
     class WinFileHandlerReleaser(FileHandlerReleaser):
         def get_handle_path(self) -> Optional[Path]:
-            return None
-
             handle = shutil.which(WINDOWS_HANDLE_BIN)
             if handle:
                 return Path(handle)
             return None
 
-        def check_handle(self, mount: Path) -> None:
+        def check_handle(self, mount: Path) -> bool:
             handle = self.get_handle_path()
-
             if not handle:
-                return
+                return False
 
             print(
                 f"Checking handle.exe for processes using '{mount}'. This can take a while..."
@@ -72,7 +69,7 @@ if sys.platform == "win32":
                     "If you want to find out which process is still using the repo, run:"
                 )
                 print(f"    handle.exe {mount}\n")
-                return
+                return False
             parsed = [
                 line.split()
                 for line in output.decode(errors="ignore").splitlines()
@@ -87,7 +84,10 @@ if sys.platform == "win32":
             if not non_edenfs_process or not parsed or len(parsed[0]) == 4:
                 # Nothing other than edenfs.exe is holding handles to files from
                 # the repo, we can proceed with the removal
-                return
+                print(
+                    "No processes found. They may be running under a different user.\n"
+                )
+                return False
 
             print("The following processes are still using the repo.\n")
 
@@ -103,18 +103,21 @@ if sys.platform == "win32":
                     try:
                         proc = psutil.Process(pid)
                         proc.kill()
+                        proc.wait()
                     except Exception as e:
                         print(f"Failed to kill process {pid}: {e}")
+                        return False
             else:
                 print(
                     f"Once you have exited those processes, delete {mount} manually.\n"
                 )
+                return False
             print()
-            return
+            return True
 
-        def try_release(self, mount: Path) -> None:
+        def try_release(self, mount: Path) -> bool:
             if self.get_handle_path():
-                self.check_handle(mount)
+                return self.check_handle(mount)
             else:
                 print(
                     f"""\
@@ -129,3 +132,4 @@ if sys.platform == "win32":
                     f"After terminating the processes, please manually delete {mount}.\n"
                 )
                 print()
+                return False

eden/fs/cli/filesystem.py

@@ -9,6 +9,7 @@
 import abc
 import os
 import shutil
+from pathlib import Path
 
 from . import util
 
@@ -22,6 +23,10 @@ class FsUtil(abc.ABC):
     def disk_usage(self, path: str) -> shutil._ntuple_diskusage:
         """Calls os.statvfs on the mount"""
 
+    @abc.abstractmethod
+    def rmdir(self, path: str, keep_root: bool) -> bool:
+        """Removes a directory recursively. Raises exception on failure, otherwise completes normally."""
+
 
 class RealFsUtil(FsUtil):
     def mkdir_p(self, path: str) -> str:
@@ -30,6 +35,18 @@ class RealFsUtil(FsUtil):
     def disk_usage(self, path: str) -> shutil._ntuple_diskusage:
         return shutil.disk_usage(path)
 
+    def rmdir(self, path: str, keep_root: bool) -> bool:
+        dir: Path = Path(path)
+        dir.chmod(0o755)
+        for child in dir.iterdir():
+            if child.is_dir():
+                shutil.rmtree(child)
+            else:
+                child.unlink()
+        if not keep_root:
+            dir.rmdir()
+        return True
+
 
 def new() -> FsUtil:
     return RealFsUtil()

eden/fs/cli/main.py

@@ -51,6 +51,7 @@ from . import (
     daemon_util,
     debug as debug_mod,
     doctor as doctor_mod,
+    filesystem as fs_mod,
     hg_util,
     mtab,
     prefetch as prefetch_mod,
@@ -1477,10 +1478,13 @@ class MountCmd(Subcmd):
 # its configuration.
 # * CLEANUP_ONLY: removing an unknown directory, it might be an old EdenFS
 # mount that failed to clean up. We try to clean it up again in this case.
+# NON_EDEN: removing a non-EdenFS directory or file using eden rm - in this
+# case we want to skip the Eden housekeeping after removing it
 class RemoveType(enum.Enum):
     ACTIVE_MOUNT = 0
     INACTIVE_MOUNT = 1
     CLEANUP_ONLY = 2
+    NON_EDEN = 3
 
 
 @subcmd("remove", "Remove an EdenFS checkout", aliases=["rm"])
@@ -1566,6 +1570,7 @@ Do you still want to delete {path}?"""
                 mount_path = util.get_eden_mount_name(path)
                 remove_type = RemoveType.ACTIVE_MOUNT
             except util.NotAnEdenMountError as ex:
+                remove_type = RemoveType.NON_EDEN
                 # This is not an active mount point.
                 # Check for it by name in the config file anyway, in case it is
                 # listed in the config file but not currently mounted.
@@ -1587,15 +1592,7 @@ Do you still want to delete {path}?"""
                             return 2
                         else:
                             try:
-                                path = Path(path)
+                                fs_mod.new().rmdir(path, args.preserve_mount_point)
-                                path.chmod(0o755)
-                                for child in path.iterdir():
-                                    if child.is_dir():
-                                        shutil.rmtree(child)
-                                    else:
-                                        child.unlink()
-                                if not args.preserve_mount_point:
-                                    path.rmdir()
                                 return 0
                             except Exception as ex:
                                 if sys.platform != "win32":
@@ -1605,8 +1602,18 @@ Do you still want to delete {path}?"""
                                     return 1
                                 else:
                                     winhr = WinFileHandlerReleaser()
-                                    winhr.try_release(path)
+                                    maybe_succeeded = winhr.try_release(path)
-                                    return 0
+                                    try:
+                                        # Try again after try_release
+                                        if maybe_succeeded:
+                                            fs_mod.new().rmdir(
+                                                path, args.preserve_mount_point
+                                            )
+                                    except Exception as ex:
+                                        print(
+                                            f"Error: cannot remove contents of {path} even after trying to kill processes holding resources: {ex}"
+                                        )
+                                        return 1
                     else:
                         # We can't ask the user what their true intentions are,
                         # so let's fail by default.
@@ -1623,10 +1630,11 @@ Do you still want to delete {path}?"""
                     f"{mount_path}, not deleting"
                 )
                 return 1
-            mounts.append((mount_path, remove_type))
+            if remove_type != RemoveType.NON_EDEN:
+                mounts.append((mount_path, remove_type))
 
         # Warn the user since this operation permanently destroys data
-        if args.prompt and sys.stdin.isatty():
+        if args.prompt and sys.stdin.isatty() and len(mounts) > 0:
             mounts_list = "\n  ".join(path for path, _ in mounts)
             print(
                 f"""\