From bdafbc1494511b83b4ff628d836befe9d8ce2e8d Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Tue, 2 Sep 2025 10:03:00 -0700 Subject: [PATCH 1/2] FoundationEssentials: try to make the file atomic behaviour more robust (#1485) We have observed `ERROR_ACCESS_DENIED` in CI on `SetRenameInformationFile`. Try to make the path more robust by first performing a kernel based rename with POSIX semantics. This requires Windows 10 1809+. If that is unsuccessful, verify that attributes are not to blame. A failure may still occur if the file is on a different volume. In such a case, fallback to the `MoveFileExW` operation to perform a copy + delete operation. Hopefully this should make the implementation more robust to failures. --- .../Data/Data+Writing.swift | 94 ++++++++++++++----- .../WinSDK+Extensions.swift | 13 +++ .../DataIOTests.swift | 18 ++++ 3 files changed, 100 insertions(+), 25 deletions(-) diff --git a/Sources/FoundationEssentials/Data/Data+Writing.swift b/Sources/FoundationEssentials/Data/Data+Writing.swift index 51b54ed5e..720923064 100644 --- a/Sources/FoundationEssentials/Data/Data+Writing.swift +++ b/Sources/FoundationEssentials/Data/Data+Writing.swift @@ -358,44 +358,88 @@ private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPoint // TODO: Somehow avoid copying back and forth to a String to hold the path #if os(Windows) - try inPath.path.withNTPathRepresentation { pwszPath in - var (fd, auxPath, temporaryDirectoryPath) = try createProtectedTemporaryFile(at: inPath.path, inPath: inPath, options: options, variant: "Folder") + var (fd, auxPath, temporaryDirectoryPath) = try createProtectedTemporaryFile(at: inPath.path, inPath: inPath, options: options, variant: "Folder") - // Cleanup temporary directory - defer { cleanupTemporaryDirectory(at: temporaryDirectoryPath) } + // Cleanup temporary directory + defer { cleanupTemporaryDirectory(at: temporaryDirectoryPath) } - guard fd >= 0 else { + guard fd >= 0 else { + throw CocoaError.errorWithFilePath(inPath, errno: errno, reading: false) + } + + defer { if fd >= 0 { _close(fd) } } + + let callback = (reportProgress && Progress.current() != nil) ? Progress(totalUnitCount: Int64(buffer.count)) : nil + + do { + try write(buffer: buffer, toFileDescriptor: fd, path: inPath, parentProgress: callback) + } catch { + try auxPath.withNTPathRepresentation { pwszAuxPath in + _ = DeleteFileW(pwszAuxPath) + } + + if callback?.isCancelled ?? false { + throw CocoaError(.userCancelled) + } else { throw CocoaError.errorWithFilePath(inPath, errno: errno, reading: false) } + } - defer { if fd >= 0 { _close(fd) } } + writeExtendedAttributes(fd: fd, attributes: attributes) - let callback = (reportProgress && Progress.current() != nil) ? Progress(totalUnitCount: Int64(buffer.count)) : nil + _close(fd) + fd = -1 - do { - try write(buffer: buffer, toFileDescriptor: fd, path: inPath, parentProgress: callback) - } catch { - try auxPath.withNTPathRepresentation { pwszAuxPath in - _ = DeleteFileW(pwszAuxPath) - } + try auxPath.withNTPathRepresentation { pwszAuxiliaryPath in + defer { _ = DeleteFileW(pwszAuxiliaryPath) } - if callback?.isCancelled ?? false { - throw CocoaError(.userCancelled) - } else { - throw CocoaError.errorWithFilePath(inPath, errno: errno, reading: false) + var hFile = CreateFileW(pwszAuxiliaryPath, DELETE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nil, OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, + nil) + if hFile == INVALID_HANDLE_VALUE { + throw CocoaError.errorWithFilePath(inPath, win32: GetLastError(), reading: false) + } + + defer { + switch hFile { + case INVALID_HANDLE_VALUE: + break + default: + _ = CloseHandle(hFile) } } - writeExtendedAttributes(fd: fd, attributes: attributes) + try inPath.path.withNTPathRepresentation { pwszPath in + let cchLength = wcslen(pwszPath) + let cbSize = cchLength * MemoryLayout.size + let dwSize = DWORD(MemoryLayout.size + cbSize + MemoryLayout.size) + try withUnsafeTemporaryAllocation(byteCount: Int(dwSize), + alignment: MemoryLayout.alignment) { pBuffer in + var pInfo = pBuffer.baseAddress?.bindMemory(to: FILE_RENAME_INFO.self, capacity: 1) + pInfo?.pointee.Flags = FILE_RENAME_FLAG_POSIX_SEMANTICS | FILE_RENAME_FLAG_REPLACE_IF_EXISTS + pInfo?.pointee.RootDirectory = nil + pInfo?.pointee.FileNameLength = DWORD(cbSize) + pBuffer.baseAddress?.advanced(by: MemoryLayout.offset(of: \.FileName)!) + .withMemoryRebound(to: WCHAR.self, capacity: cchLength + 1) { + wcscpy_s($0, cchLength + 1, pwszPath) + } - _close(fd) - fd = -1 + if !SetFileInformationByHandle(hFile, FileRenameInfoEx, pInfo, dwSize) { + let dwError = GetLastError() + guard dwError == ERROR_NOT_SAME_DEVICE else { + throw CocoaError.errorWithFilePath(inPath, win32: dwError, reading: false) + } - try auxPath.withNTPathRepresentation { pwszAuxiliaryPath in - guard MoveFileExW(pwszAuxiliaryPath, pwszPath, MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) else { - let dwError = GetLastError() - _ = DeleteFileW(pwszAuxiliaryPath) - throw CocoaError.errorWithFilePath(inPath, win32: dwError, reading: false) + _ = CloseHandle(hFile) + hFile = INVALID_HANDLE_VALUE + + // The move is across volumes. + guard MoveFileExW(pwszAuxiliaryPath, pwszPath, MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING) else { + throw CocoaError.errorWithFilePath(inPath, win32: GetLastError(), reading: false) + } + } } } } diff --git a/Sources/FoundationEssentials/WinSDK+Extensions.swift b/Sources/FoundationEssentials/WinSDK+Extensions.swift index 54f3cb78b..6322c4c86 100644 --- a/Sources/FoundationEssentials/WinSDK+Extensions.swift +++ b/Sources/FoundationEssentials/WinSDK+Extensions.swift @@ -41,6 +41,10 @@ package var CREATE_NEW: DWORD { DWORD(WinSDK.CREATE_NEW) } +package var DELETE: DWORD { + DWORD(WinSDK.DELETE) +} + package var ERROR_ACCESS_DENIED: DWORD { DWORD(WinSDK.ERROR_ACCESS_DENIED) } @@ -133,6 +137,7 @@ package var FILE_ATTRIBUTE_READONLY: DWORD { DWORD(WinSDK.FILE_ATTRIBUTE_READONLY) } + package var FILE_ATTRIBUTE_REPARSE_POINT: DWORD { DWORD(WinSDK.FILE_ATTRIBUTE_REPARSE_POINT) } @@ -153,6 +158,14 @@ package var FILE_NAME_NORMALIZED: DWORD { DWORD(WinSDK.FILE_NAME_NORMALIZED) } +package var FILE_RENAME_FLAG_POSIX_SEMANTICS: DWORD { + DWORD(WinSDK.FILE_RENAME_FLAG_POSIX_SEMANTICS) +} + +package var FILE_RENAME_FLAG_REPLACE_IF_EXISTS: DWORD { + DWORD(WinSDK.FILE_RENAME_FLAG_REPLACE_IF_EXISTS) +} + package var FILE_SHARE_DELETE: DWORD { DWORD(WinSDK.FILE_SHARE_DELETE) } diff --git a/Tests/FoundationEssentialsTests/DataIOTests.swift b/Tests/FoundationEssentialsTests/DataIOTests.swift index e9d2d8e15..4ebca2557 100644 --- a/Tests/FoundationEssentialsTests/DataIOTests.swift +++ b/Tests/FoundationEssentialsTests/DataIOTests.swift @@ -193,6 +193,24 @@ private final class DataIOTests { let maps = try String(contentsOfFile: "/proc/self/maps", encoding: .utf8) #expect(!maps.isEmpty) } + + @Test + func atomicWrite() async throws { + let data = generateTestData() + + await withThrowingTaskGroup(of: Void.self) { group in + for _ in 0 ..< 8 { + group.addTask { [url] in + #expect(throws: Never.self) { + try data.write(to: url, options: [.atomic]) + } + } + } + } + + let readData = try Data(contentsOf: url, options: []) + #expect(readData == data) + } } extension LargeDataTests { From 5eeffaad18e3e91c7a36af1651bb919c694b0265 Mon Sep 17 00:00:00 2001 From: Jeremy Schonfeld Date: Tue, 2 Sep 2025 11:08:42 -0700 Subject: [PATCH 2/2] Update package dependency branches --- CMakeLists.txt | 2 +- Package.swift | 4 ++-- Sources/FoundationMacros/CMakeLists.txt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6e1f9ffba..abc92833e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,7 +75,7 @@ else() message(STATUS "_SwiftFoundationICU_SourceDIR not provided, checking out local copy of swift-foundation-icu") FetchContent_Declare(SwiftFoundationICU GIT_REPOSITORY https://github.com/apple/swift-foundation-icu.git - GIT_TAG main) + GIT_TAG release/6.2.1) endif() if (_SwiftCollections_SourceDIR) diff --git a/Package.swift b/Package.swift index 4d123c0f7..b659e3684 100644 --- a/Package.swift +++ b/Package.swift @@ -60,10 +60,10 @@ var dependencies: [Package.Dependency] { from: "1.1.0"), .package( url: "https://github.com/apple/swift-foundation-icu", - branch: "main"), + branch: "release/6.2.1"), .package( url: "https://github.com/swiftlang/swift-syntax", - branch: "main") + branch: "release/6.2.1") ] } } diff --git a/Sources/FoundationMacros/CMakeLists.txt b/Sources/FoundationMacros/CMakeLists.txt index b0cbed634..d16f0e4ae 100644 --- a/Sources/FoundationMacros/CMakeLists.txt +++ b/Sources/FoundationMacros/CMakeLists.txt @@ -43,7 +43,7 @@ if(NOT SwiftSyntax_FOUND) # If building at desk, check out and link against the SwiftSyntax repo's targets FetchContent_Declare(SwiftSyntax GIT_REPOSITORY https://github.com/swiftlang/swift-syntax.git - GIT_TAG main) + GIT_TAG release/6.2.1) FetchContent_MakeAvailable(SwiftSyntax) else() message(STATUS "SwiftSyntax_DIR provided, using swift-syntax from ${SwiftSyntax_DIR}")