fs.Dir.deleteTree: Add configurable max_retries to avoid infinite loops

Previously, it was possible for deleteTree to get into a state where it would continually retry deleting the same directory over and over infinitely, since it would think all of the children were successfully deleted but when deleting the directory it would hit `error.DirNotEmpty`. Now, there is a (user-configurable) limit to the number of times it will retry deleting directories, and return `error.TooManyRetries` if the limit is exceeded.

Closes #15465

Author: squeek502

doc/docgen.zig

@@ -99,7 +99,7 @@ pub fn main() !void {
     var toc = try genToc(allocator, &tokenizer);
 
     try fs.cwd().makePath(tmp_dir_name);
-    defer fs.cwd().deleteTree(tmp_dir_name) catch {};
+    defer fs.cwd().deleteTree(tmp_dir_name, .{}) catch {};
 
     try genHtml(allocator, &tokenizer, &toc, buffered_writer.writer(), zig_exe, opt_zig_lib_dir, do_code_tests);
     try buffered_writer.flush();

lib/std/Build.zig

@@ -776,7 +776,7 @@ fn makeUninstall(uninstall_step: *Step, prog_node: *std.Progress.Node) anyerror!
         if (self.verbose) {
             log.info("rm {s}", .{full_path});
         }
-        fs.cwd().deleteTree(full_path) catch {};
+        fs.cwd().deleteTree(full_path, .{}) catch {};
     }
 
     // TODO remove empty directories

lib/std/Build/RemoveDirStep.zig

@@ -28,7 +28,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
     const b = step.owner;
     const self = @fieldParentPtr(RemoveDirStep, "step", step);
 
-    b.build_root.handle.deleteTree(self.dir_path) catch |err| {
+    b.build_root.handle.deleteTree(self.dir_path, .{}) catch |err| {
         if (b.build_root.path) |base| {
             return step.fail("unable to recursively delete path '{s}/{s}': {s}", .{
                 base, self.dir_path, @errorName(err),

lib/std/fs.zig

@@ -2089,6 +2089,10 @@ pub const Dir = struct {
         FileBusy,
         DeviceBusy,
 
+        /// The number of retries to delete thought-to-be-empty directories
+        /// exceeded `max_retries`.
+        TooManyRetries,
+
         /// One of the path components was not a directory.
         /// This error is unreachable if `sub_path` does not contain a path separator.
         NotDir,
@@ -2101,17 +2105,33 @@ pub const Dir = struct {
         BadPathName,
     } || os.UnexpectedError;
 
+    pub const DeleteTreeOptions = struct {
+        /// After successfully deleting all of a directories children, it is possible for
+        /// the directory to not actually be empty (either from a new child being added while
+        /// iterating or a child being in a `DELETE_PENDING` state on Windows). In this scenario,
+        /// the directory will be iterated again and then will be attempted to be deleted again.
+        /// This provides a limit to the total number of such retries.
+        /// In `deleteTree`, the retry limit is per-directory.
+        /// In `deleteTreeMinStackSize`, the retry limit is function-wide.
+        max_retries: usize = 4,
+
+        /// When initially trying to delete `sub_path`, it will first try and delete it as
+        /// the provided kind.
+        kind_hint: File.Kind = .File,
+    };
+
     /// Whether `full_path` describes a symlink, file, or directory, this function
     /// removes it. If it cannot be removed because it is a non-empty directory,
     /// this function recursively removes its entries and then tries again.
     /// This operation is not atomic on most file systems.
-    pub fn deleteTree(self: Dir, sub_path: []const u8) DeleteTreeError!void {
-        var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, .File)) orelse return;
+    pub fn deleteTree(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void {
+        var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return;
 
         const StackItem = struct {
             name: []const u8,
             parent_dir: Dir,
             iter: IterableDir.Iterator,
+            retries_remaining: usize,
         };
 
         var stack = std.BoundedArray(StackItem, 16){};
@@ -2125,6 +2145,7 @@ pub const Dir = struct {
             .name = sub_path,
             .parent_dir = self,
             .iter = initial_iterable_dir.iterateAssumeFirstIteration(),
+            .retries_remaining = options.max_retries,
         });
 
         process_stack: while (stack.len != 0) {
@@ -2162,10 +2183,14 @@ pub const Dir = struct {
                                 .name = entry.name,
                                 .parent_dir = top.iter.dir,
                                 .iter = iterable_dir.iterateAssumeFirstIteration(),
+                                .retries_remaining = options.max_retries,
                             });
                             continue :process_stack;
                         } else |_| {
-                            try top.iter.dir.deleteTreeMinStackSizeWithKindHint(entry.name, entry.kind);
+                            try top.iter.dir.deleteTreeMinStackSize(entry.name, .{
+                                .max_retries = options.max_retries,
+                                .kind_hint = entry.kind,
+                            });
                             break :handle_entry;
                         }
                     } else {
@@ -2207,6 +2232,7 @@ pub const Dir = struct {
             // pop the value from the stack.
             const parent_dir = top.parent_dir;
             const name = top.name;
+            const retries_remaining = top.retries_remaining;
             _ = stack.pop();
 
             var need_to_retry: bool = false;
@@ -2217,6 +2243,7 @@ pub const Dir = struct {
             };
 
             if (need_to_retry) {
+                if (retries_remaining == 0) return error.TooManyRetries;
                 // Since we closed the handle that the previous iterator used, we
                 // need to re-open the dir and re-create the iterator.
                 var iterable_dir = iterable_dir: {
@@ -2282,6 +2309,7 @@ pub const Dir = struct {
                     .name = name,
                     .parent_dir = parent_dir,
                     .iter = iterable_dir.iterateAssumeFirstIteration(),
+                    .retries_remaining = retries_remaining - 1,
                 });
                 continue :process_stack;
             }
@@ -2290,13 +2318,10 @@ pub const Dir = struct {
 
     /// Like `deleteTree`, but only keeps one `Iterator` active at a time to minimize the function's stack size.
     /// This is slower than `deleteTree` but uses less stack space.
-    pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8) DeleteTreeError!void {
-        return self.deleteTreeMinStackSizeWithKindHint(sub_path, .File);
-    }
-
-    fn deleteTreeMinStackSizeWithKindHint(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void {
+    pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void {
+        var retries_remaining = options.max_retries;
         start_over: while (true) {
-            var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, kind_hint)) orelse return;
+            var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return;
             var cleanup_dir_parent: ?IterableDir = null;
             defer if (cleanup_dir_parent) |*d| d.close();
 
@@ -2386,14 +2411,22 @@ pub const Dir = struct {
                 if (cleanup_dir_parent) |d| {
                     d.dir.deleteDir(dir_name) catch |err| switch (err) {
                         // These two things can happen due to file system race conditions.
-                        error.FileNotFound, error.DirNotEmpty => continue :start_over,
+                        error.FileNotFound, error.DirNotEmpty => {
+                            if (retries_remaining == 0) return error.TooManyRetries;
+                            retries_remaining -= 1;
+                            continue :start_over;
+                        },
                         else => |e| return e,
                     };
                     continue :start_over;
                 } else {
                     self.deleteDir(sub_path) catch |err| switch (err) {
                         error.FileNotFound => return,
-                        error.DirNotEmpty => continue :start_over,
+                        error.DirNotEmpty => {
+                            if (retries_remaining == 0) return error.TooManyRetries;
+                            retries_remaining -= 1;
+                            continue :start_over;
+                        },
                         else => |e| return e,
                     };
                     return;
@@ -2834,7 +2867,7 @@ pub fn deleteTreeAbsolute(absolute_path: []const u8) !void {
     var dir = try cwd().openDir(dirname, .{});
     defer dir.close();
 
-    return dir.deleteTree(path.basename(absolute_path));
+    return dir.deleteTree(path.basename(absolute_path), .{});
 }
 
 /// Same as `Dir.readLink`, except it asserts the path is absolute.

lib/std/fs/test.zig

@@ -304,7 +304,7 @@ test "Dir.Iterator but dir is deleted during iteration" {
     // This is a contrived reproduction, but this could happen outside of the program, in another thread, etc.
     // If we get an error while trying to delete, we can skip this test (this will happen on platforms
     // like Windows which will give FileBusy if the directory is currently open for iteration).
-    tmp.dir.deleteTree("subdir") catch return error.SkipZigTest;
+    tmp.dir.deleteTree("subdir", .{}) catch return error.SkipZigTest;
 
     // Now, when we try to iterate, the next call should return null immediately.
     const entry = try iterator.next();
@@ -696,7 +696,7 @@ test "makePath, put some files in it, deleteTree" {
     try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c");
     try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense");
     try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah");
-    try tmp.dir.deleteTree("os_test_tmp");
+    try tmp.dir.deleteTree("os_test_tmp", .{});
     if (tmp.dir.openDir("os_test_tmp", .{})) |dir| {
         _ = dir;
         @panic("expected error");
@@ -712,7 +712,7 @@ test "makePath, put some files in it, deleteTreeMinStackSize" {
     try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c");
     try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense");
     try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah");
-    try tmp.dir.deleteTreeMinStackSize("os_test_tmp");
+    try tmp.dir.deleteTreeMinStackSize("os_test_tmp", .{});
     if (tmp.dir.openDir("os_test_tmp", .{})) |dir| {
         _ = dir;
         @panic("expected error");
@@ -721,12 +721,48 @@ test "makePath, put some files in it, deleteTreeMinStackSize" {
     }
 }
 
+test "deleteTree max retries" {
+    // While TooManyRetries is relevant for non-Windows platforms, it is trickier to
+    // reproduce the necessary conditions, as getting DirNotEmpty when deleting a directory
+    // that `deleteTree` thinks is now empty would likely mean something like a file/dir being
+    // created while the directory is being iterated and its children deleted.
+    if (builtin.os.tag != .windows) return error.SkipZigTest;
+
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+
+    // Make a subdirectory with a file in it and keep the file handle open.
+    var leaked_handle = blk: {
+        var dir = try tmp.dir.makeOpenPath("dir", .{});
+        defer dir.close();
+
+        var file = try dir.createFile("neverclose", .{});
+        break :blk file;
+    };
+
+    {
+        // We always want to close the file handle even on test failure, but we want it to be
+        // open for these expectError calls.
+        defer leaked_handle.close();
+
+        // Since the file handle was never closed, it will enter into a `DELETE_PENDING`
+        // state which will cause deleteTree to continually delete the file and directory,
+        // getting 'success' from the file delete but then DirNotEmpty from the directory
+        // delete.
+        try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTree("dir", .{}));
+        try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTreeMinStackSize("dir", .{}));
+    }
+
+    // Now that the leaked handle has been closed, deleteTree should succeed.
+    try tmp.dir.deleteTree("dir", .{});
+}
+
 test "makePath in a directory that no longer exists" {
     if (builtin.os.tag == .windows) return error.SkipZigTest; // Windows returns FileBusy if attempting to remove an open dir
 
     var tmp = tmpDir(.{});
     defer tmp.cleanup();
-    try tmp.parent_dir.deleteTree(&tmp.sub_path);
+    try tmp.parent_dir.deleteTree(&tmp.sub_path, .{});
 
     try testing.expectError(error.FileNotFound, tmp.dir.makePath("sub-path"));
 }
@@ -751,7 +787,7 @@ fn testFilenameLimits(iterable_dir: IterableDir, maxed_filename: []const u8) !vo
     }
 
     // ensure that we can delete the tree
-    try iterable_dir.dir.deleteTree(maxed_filename);
+    try iterable_dir.dir.deleteTree(maxed_filename, .{});
 }
 
 test "max file name component lengths" {
@@ -874,15 +910,15 @@ test "access file" {
 
     try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", "");
     try tmp.dir.access("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", .{});
-    try tmp.dir.deleteTree("os_test_tmp");
+    try tmp.dir.deleteTree("os_test_tmp", .{});
 }
 
 test "sendfile" {
     var tmp = tmpDir(.{});
     defer tmp.cleanup();
 
     try tmp.dir.makePath("os_test_tmp");
-    defer tmp.dir.deleteTree("os_test_tmp") catch {};
+    defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {};
 
     var dir = try tmp.dir.openDir("os_test_tmp", .{});
     defer dir.close();
@@ -947,7 +983,7 @@ test "copyRangeAll" {
     defer tmp.cleanup();
 
     try tmp.dir.makePath("os_test_tmp");
-    defer tmp.dir.deleteTree("os_test_tmp") catch {};
+    defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {};
 
     var dir = try tmp.dir.openDir("os_test_tmp", .{});
     defer dir.close();

lib/std/fs/watch.zig

@@ -643,7 +643,7 @@ test "write a file, watch it, write it again, delete it" {
     if (builtin.single_threaded) return error.SkipZigTest;
 
     try std.fs.cwd().makePath(test_tmp_dir);
-    defer std.fs.cwd().deleteTree(test_tmp_dir) catch {};
+    defer std.fs.cwd().deleteTree(test_tmp_dir, .{}) catch {};
 
     return testWriteWatchWriteDelete(std.testing.allocator);
 }

lib/std/testing.zig

@@ -519,7 +519,7 @@ pub const TmpDir = struct {
 
     pub fn cleanup(self: *TmpDir) void {
         self.dir.close();
-        self.parent_dir.deleteTree(&self.sub_path) catch {};
+        self.parent_dir.deleteTree(&self.sub_path, .{}) catch {};
         self.parent_dir.close();
         self.* = undefined;
     }
@@ -535,7 +535,7 @@ pub const TmpIterableDir = struct {
 
     pub fn cleanup(self: *TmpIterableDir) void {
         self.iterable_dir.close();
-        self.parent_dir.deleteTree(&self.sub_path) catch {};
+        self.parent_dir.deleteTree(&self.sub_path, .{}) catch {};
         self.parent_dir.close();
         self.* = undefined;
     }

src/Package.zig

@@ -719,7 +719,7 @@ fn renameTmpIntoCache(
             },
             error.PathAlreadyExists, error.AccessDenied => {
                 // Package has been already downloaded and may already be in use on the system.
-                cache_dir.deleteTree(tmp_dir_sub_path) catch |del_err| {
+                cache_dir.deleteTree(tmp_dir_sub_path, .{}) catch |del_err| {
                     std.log.warn("unable to delete temp directory: {s}", .{@errorName(del_err)});
                 };
             },

src/link.zig

@@ -937,7 +937,7 @@ pub const File = struct {
                 // Work around windows `renameW` can't fail with `PathAlreadyExists`
                 // See https://github.com/ziglang/zig/issues/8362
                 if (cache_directory.handle.access(o_sub_path, .{})) |_| {
-                    try cache_directory.handle.deleteTree(o_sub_path);
+                    try cache_directory.handle.deleteTree(o_sub_path, .{});
                     continue;
                 } else |err| switch (err) {
                     error.FileNotFound => {},
@@ -961,7 +961,7 @@ pub const File = struct {
                     o_sub_path,
                 ) catch |err| switch (err) {
                     error.PathAlreadyExists => {
-                        try cache_directory.handle.deleteTree(o_sub_path);
+                        try cache_directory.handle.deleteTree(o_sub_path, .{});
                         continue;
                     },
                     else => |e| return e,