inter-process race condition when writing builtin.zig file

[andrewrk]

Zig Version

0.11.0-dev.2150+68c7261e1

Steps to Reproduce and Observed Behavior

zig build test

2023-03-17T02:35:33.7612383Z run-translated-c division of floating literals build-exe: error: the following command failed with 1 compilation errors:
2023-03-17T02:35:33.7616740Z C:\actions-runner\_work\zig\zig\build-release\stage3-release\bin\zig.exe build-exe C:\actions-runner\_work\zig\zig\build-release\zig-local-cache\o\c00ed4aa0da85b3d6f53e1b3a4de5d8e\source.zig -lc --cache-dir C:\actions-runner\_work\zig\zig\build-release\zig-local-cache --global-cache-dir C:\actions-runner\_work\zig\zig\build-release\zig-global-cache --name translated_c -L C:\actions-runner\_work\zig\zig\..\zig+llvm+lld+clang-aarch64-windows-gnu-0.11.0-dev.670+f7fea080b\lib -I C:\actions-runner\_work\zig\zig\..\zig+llvm+lld+clang-aarch64-windows-gnu-0.11.0-dev.670+f7fea080b\include --zig-lib-dir C:\actions-runner\_work\zig\zig\lib --enable-cache --listen=- 
2023-03-17T02:41:34.5577258Z Build Summary: 4011/4162 steps succeeded; 147 skipped; 1 failed; 47994/50636 tests passed; 2642 skipped (disable with -fno-summary)
2023-03-17T02:41:34.6550307Z |  +- run-translated-c division of floating literals run transitive failure
2023-03-17T02:41:34.6550500Z |  |  +- run-translated-c division of floating literals build-exe 1 errors
2023-03-17T02:41:34.6550999Z |  |     +- run-translated-c division of floating literals translate-c success 69ms
2023-03-17T02:41:34.6551249Z |  |        +- WriteFile source.c success
2023-03-17T02:41:34.6653922Z error: unable to write builtin.zig to C:\actions-runner\_work\zig\zig\build-release\zig-local-cache\tmp\da8554bcb2aed65d: AccessDenied

Expected Behavior

Expected the tests to pass.

The relevant code is here:

zig/src/Module.zig

Lines 3985 to 4015 in 68c7261

	if (builtin_pkg.root_src_directory.handle.statFile(builtin_pkg.root_src_path)) |stat| {
	if (stat.size != file.source.len) {
	const full_path = try builtin_pkg.root_src_directory.join(gpa, &.{
	builtin_pkg.root_src_path,
	});
	defer gpa.free(full_path);
	log.warn(
	"the cached file '{s}' had the wrong size. Expected {d}, found {d}. " ++
	"Overwriting with correct file contents now",
	.{ full_path, file.source.len, stat.size },
	);
	try writeBuiltinFile(file, builtin_pkg);
	} else {
	file.stat = .{
	.size = stat.size,
	.inode = stat.inode,
	.mtime = stat.mtime,
	};
	}
	} else |err| switch (err) {
	error.BadPathName => unreachable, // it's always "builtin.zig"
	error.NameTooLong => unreachable, // it's always "builtin.zig"
	error.PipeBusy => unreachable, // it's not a pipe
	error.WouldBlock => unreachable, // not asking for non-blocking I/O
	error.FileNotFound => try writeBuiltinFile(file, builtin_pkg),
	else => |e| return e,
	}

[jacobly0]

This happens on windows if std.fs.AtomicFile.finish is called simultaneously to overwrite the same file. The error should reproduce by adding a sleep after NtSetInformationFile in std.os.renameatW and launching two processes using the std.fs.AtomicFile API.

diff --git a/lib/std/os.zig b/lib/std/os.zig
index 5554e6e31..634c53f6d 100644
--- a/lib/std/os.zig
+++ b/lib/std/os.zig
@@ -2697,6 +2697,8 @@ pub fn renameatW(
         .FileRenameInformation,
     );
 
+    std.time.sleep(std.time.ns_per_s);
+
     switch (rc) {
         .SUCCESS => return,
         .INVALID_HANDLE => unreachable,

const std = @import("std");
pub fn main() !void {
    var af = try std.fs.cwd().atomicFile("builtin.zig", .{});
    defer af.deinit();
    try af.finish();
}

[matu3ba]

I think the correct fix is to 1. use FILE_RENAME_POSIX_SEMANTICS as described in https://stackoverflow.com/a/51737582/9306292 and 2. as fallback before the Win10 update (like for deleting files with posix semantics) https://stackoverflow.com/a/57358387/9306292 SetFileInformationByHandle with FILE_RENAME_INFO.ReplaceIfExists.

Without supporting early Win10 versions, we can get away with the saner option 1.

Appendum: Alternative options in the comment section of 2.
Funny part: Accepted answer is wrong and was never updated.

[jacobly0]

2. is already status quo and is what causes the spurious error.

This specific issue is not caused by a lack of atomicity, but rather by an unanticipated (and badly documented) error condition.

[matu3ba]

Mhm, that's true. MS docs mentions "If FILE_RENAME_REPLACE_IF_EXISTS is also specified, allow replacing a file even if there are existing handles to it." https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information

Not really sure, but the problem could be the same as in DeleteFile without posix semantics, where other process handles must be closed first or worse only in some (error) cases.

[jacobly0]

Yes, if you look carefully at where I placed the sleep, it is in between NtSetInformationFile and CloseHandle. It has already been colloquially shown using my repro that adding FILE_RENAME_POSIX_SEMANTICS avoids the error. The error happens in the second process after a successful rename in the first process, but before the first process closes the original handle.