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

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,36 @@ 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.
+        /// The retry limit is per-directory.
+        ///
+        /// Note: A sufficiently adversarial process could still cause an effectively
+        /// infinite loop by doing something like continually creating deeply nested directory
+        /// structures within the to-be-deleted tree as it is being deleted.
+        max_retries: usize = 3,
+
+        /// 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 +2148,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 +2186,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 +2235,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 +2246,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 +2312,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 +2321,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 +2414,25 @@ 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,
                     };
+                    // Reset retries after successfully deleting a directory so that the
+                    // retries are effectively per-directory.
+                    retries_remaining = options.max_retries;
                     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 +2873,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,

tools/update_cpu_features.zig

@@ -1198,7 +1198,7 @@ fn processOneTarget(job: Job) anyerror!void {
 
     if (all_features.items.len == 0) {
         // We represent this with an empty file.
-        try target_dir.deleteTree(zig_code_basename);
+        try target_dir.deleteTree(zig_code_basename, .{});
         return;
     }