implementations/Crafter.Build-Platform.cpp

@@ -27,9 +27,6 @@ module;
 #include <unistd.h>
 #include <dlfcn.h>
 #include <sys/wait.h>
-#include <sys/file.h>
-#include <fcntl.h>
-#include <errno.h>
 #endif
 module Crafter.Build:Platform_impl;
 import std;
@@ -40,57 +37,39 @@ namespace fs = std::filesystem;
 using namespace Crafter;
 
 namespace {
-    // Exclusive advisory lock over one PCM cache dir (`<cache>/<target>-<march>/`),
-    // held for the duration of a (re)build. Concurrent crafter-build invocations —
-    // and the worker threads inside a single invocation — that share a cache
-    // serialize their builds through it: the first builder to acquire the lock
-    // precompiles std.pcm / the Crafter.Build-*.pcm modules while everyone else
-    // blocks, then re-checks freshness and reuses the finished file instead of
-    // re-running clang and racing to overwrite the same path. Each builder must
-    // pass through the lock before it reaches the compile steps that *read* those
-    // PCMs, so a reader never observes a half-written file.
-    //
-    // The lock lives in the OS, keyed on a `.lock` file in the cache dir, and is
-    // released automatically when the descriptor/handle closes — including on
-    // process death — so an interrupted build can never leave a stale lock. The
-    // descriptor is close-on-exec / non-inheritable so spawned clang processes
-    // don't keep the lock alive past our own release.
-    class CacheLock {
+    // Used to make per-process temp filenames so concurrent crafter-build
+    // invocations sharing the host cache dir don't collide on the same
+    // partially-written .pcm. Cross-process uniqueness comes from the PID;
+    // intra-process uniqueness from the counter (multiple host PCMs precompile
+    // back-to-back, and tests can call BuildStdPcm from several threads).
+    unsigned long CurrentProcessId() {
 #ifdef CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_linux_gnu
-        int fd_ = -1;
-    public:
-        explicit CacheLock(const fs::path& cacheDir) {
-            fd_ = ::open((cacheDir / ".lock").c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644);
-            if (fd_ >= 0) {
-                while (::flock(fd_, LOCK_EX) != 0 && errno == EINTR) {}
-            }
-        }
-        ~CacheLock() {
-            if (fd_ >= 0) {
-                ::close(fd_);  // closing the descriptor drops the flock
-            }
-        }
+        return static_cast<unsigned long>(getpid());
 #else
-        HANDLE handle_ = INVALID_HANDLE_VALUE;
-    public:
-        explicit CacheLock(const fs::path& cacheDir) {
-            handle_ = CreateFileW((cacheDir / ".lock").wstring().c_str(),
-                GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE,
-                nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
-            if (handle_ != INVALID_HANDLE_VALUE) {
-                OVERLAPPED ov{};
-                LockFileEx(handle_, LOCKFILE_EXCLUSIVE_LOCK, 0, MAXDWORD, MAXDWORD, &ov);
-            }
-        }
-        ~CacheLock() {
-            if (handle_ != INVALID_HANDLE_VALUE) {
-                CloseHandle(handle_);  // closing the handle drops the lock
-            }
-        }
+        return static_cast<unsigned long>(GetCurrentProcessId());
 #endif
-        CacheLock(const CacheLock&) = delete;
-        CacheLock& operator=(const CacheLock&) = delete;
-    };
+    }
+
+    fs::path MakeTempPcmPath(const fs::path& finalPath) {
+        static std::atomic<unsigned> seq{0};
+        return fs::path(finalPath.string() + std::format(".tmp.{}.{}",
+            CurrentProcessId(), seq.fetch_add(1, std::memory_order_relaxed)));
+    }
+
+    // Atomically swap `tmpPath` into `finalPath`. If a parallel crafter-build
+    // committed its own equivalent build first (identical .cppm + flags →
+    // identical .pcm), `fs::rename` may fail on some filesystems; accept the
+    // outcome as long as the destination now exists.
+    std::string CommitPcm(const fs::path& tmpPath, const fs::path& finalPath) {
+        std::error_code ec;
+        fs::rename(tmpPath, finalPath, ec);
+        if (!ec) return "";
+        std::error_code rmEc;
+        fs::remove(tmpPath, rmEc);
+        if (fs::exists(finalPath)) return "";
+        return std::format("rename {} -> {}: {}",
+            tmpPath.string(), finalPath.string(), ec.message());
+    }
 }
 
 fs::path Crafter::GetCrafterBuildHome() {
@@ -306,9 +285,15 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
     std::string libcxx = std::getenv("LIBCXX_DIR");
     std::string stdcppm = std::format("{}\\modules\\c++\\v1\\std.cppm", libcxx);
 
-    CacheLock lock(stdPcm.parent_path());
     if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdcppm)) {
-        return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, stdPcm.string()));
+        fs::path tmpPcm = MakeTempPcmPath(stdPcm);
+        std::string out = RunCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, tmpPcm.string()));
+        if (!out.empty()) {
+            std::error_code ec;
+            fs::remove(tmpPcm, ec);
+            return out;
+        }
+        return CommitPcm(tmpPcm, stdPcm);
     }
     return "";
 }
@@ -319,7 +304,6 @@ std::string Crafter::GetBaseCommand(const Configuration& config) {
 
 namespace {
     void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) {
-        CacheLock lock(cacheDir);
         for (std::string_view name : kCrafterBuildModules) {
             fs::path cppmPath = sourceDir / (std::string(name) + ".cppm");
             fs::path pcmPath = cacheDir / (std::string(name) + ".pcm");
@@ -329,6 +313,7 @@ namespace {
             if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) {
                 continue;
             }
+            fs::path tmpPcm = MakeTempPcmPath(pcmPath);
             std::string cmd = std::format(
                 "clang++ --target=x86_64-pc-windows-msvc -march=native -mtune=native "
                 "-std=c++26 -O3 -D CRAFTER_BUILD_DLL_IMPORT "
@@ -336,11 +321,16 @@ namespace {
                 "-Wno-reserved-identifier -Wno-reserved-module-identifier "
                 "-fprebuilt-module-path={} "
                 "--precompile {} -o {}",
-                cacheDir.string(), cppmPath.string(), pcmPath.string());
+                cacheDir.string(), cppmPath.string(), tmpPcm.string());
             CommandResult r = Crafter::RunCommandChecked(cmd);
             if (r.exitCode != 0) {
+                std::error_code ec;
+                fs::remove(tmpPcm, ec);
                 throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output));
             }
+            if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) {
+                throw std::runtime_error(std::format("Failed to commit {} pcm: {}", name, err));
+            }
         }
     }
 }
@@ -459,7 +449,6 @@ namespace {
 }
 
 std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
-    CacheLock lock(stdPcm.parent_path());
     if (config.target == "x86_64-pc-windows-msvc") {
         // MSVC target on mingw host: same MSVC libc++ logic as the
         // native-MSVC LoadProject path. User must have LIBCXX_DIR pointing
@@ -472,11 +461,18 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
         if (fs::exists(stdPcm) && fs::last_write_time(stdPcm) >= fs::last_write_time(stdcppm)) {
             return "";
         }
-        return RunCommand(std::format(
+        fs::path tmpPcm = MakeTempPcmPath(stdPcm);
+        std::string out = RunCommand(std::format(
             "clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 "
             "-nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier "
             "--precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}",
-            config.target, config.march, config.mtune, stdPcm.string()));
+            config.target, config.march, config.mtune, tmpPcm.string()));
+        if (!out.empty()) {
+            std::error_code ec;
+            fs::remove(tmpPcm, ec);
+            return out;
+        }
+        return CommitPcm(tmpPcm, stdPcm);
     }
 
     // Default: mingw target. Look up mingw-w64 libstdc++ via the msys2 prefix.
@@ -490,15 +486,16 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
     }
     // Copy std.cc → std.cppm in C++ rather than via cmd's `copy /Y` because
     // `copy` always prints "1 file(s) copied." to stdout and RunCommand
-    // treats any output as an error. Held under the cache lock, so a plain
-    // filename is safe — no other builder writes this path concurrently.
-    fs::path stdCppm = stdPcm.parent_path() / "std.cppm";
+    // treats any output as an error. Per-PID filename so concurrent
+    // crafter-build invocations don't see each other's half-written copies.
+    fs::path stdCppm = stdPcm.parent_path() / std::format("std.cppm.{}", CurrentProcessId());
     std::error_code ec;
     fs::copy_file(stdCc, stdCppm, fs::copy_options::overwrite_existing, ec);
     if (ec) {
         return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), ec.message());
     }
-    return RunCommand(std::format(
+    fs::path tmpPcm = MakeTempPcmPath(stdPcm);
+    std::string out = RunCommand(std::format(
         "clang++ --target={} -march={} -mtune={} "
         "--sysroot=\"{}\" -femulated-tls "
         "-O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier "
@@ -506,7 +503,14 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
         config.target, config.march, config.mtune,
         prefix.string(),
         stdCppm.string(),
-        stdPcm.string()));
+        tmpPcm.string()));
+    std::error_code rmEc;
+    fs::remove(stdCppm, rmEc);
+    if (!out.empty()) {
+        fs::remove(tmpPcm, rmEc);
+        return out;
+    }
+    return CommitPcm(tmpPcm, stdPcm);
 }
 
 std::string Crafter::GetBaseCommand(const Configuration& config) {
@@ -518,7 +522,6 @@ std::string Crafter::GetBaseCommand(const Configuration& config) {
 
 namespace {
     void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) {
-        CacheLock lock(cacheDir);
         fs::path prefix = MingwPrefix();
         for (std::string_view name : kCrafterBuildModules) {
             fs::path cppmPath = sourceDir / (std::string(name) + ".cppm");
@@ -531,6 +534,7 @@ namespace {
             if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) {
                 continue;
             }
+            fs::path tmpPcm = MakeTempPcmPath(pcmPath);
             std::string cmd = std::format(
                 "clang++ --target=x86_64-w64-mingw32 -march=native -mtune=native "
                 "--sysroot=\"{}\" -femulated-tls "
@@ -538,12 +542,18 @@ namespace {
                 "-Wno-reserved-identifier -Wno-reserved-module-identifier "
                 "-fprebuilt-module-path={} "
                 "--precompile {} -o {}",
-                prefix.string(), cacheDir.string(), cppmPath.string(), pcmPath.string());
+                prefix.string(), cacheDir.string(), cppmPath.string(), tmpPcm.string());
             CommandResult r = Crafter::RunCommandChecked(cmd);
             if (r.exitCode != 0) {
+                std::error_code ec;
+                fs::remove(tmpPcm, ec);
                 throw std::runtime_error(std::format(
                     "Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output));
             }
+            if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) {
+                throw std::runtime_error(std::format(
+                    "Failed to commit {} pcm: {}", name, err));
+            }
         }
     }
 }
@@ -720,7 +730,6 @@ CommandResult Crafter::RunCommandWithTimeout(std::string_view cmd, std::chrono::
 }
 
 std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
-    CacheLock lock(stdPcm.parent_path());
     if(config.target == "x86_64-w64-mingw32") {
         std::vector<std::string> folders;
 
@@ -743,15 +752,24 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
         if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdCc)) {
             // -femulated-tls keeps PCM TLS access matching libstdc++'s emutls
             // definitions; mismatch surfaces as undefined std::__once_callable.
-            // The std.cc → std.cppm copy uses a plain filename: held under the
-            // cache lock, no other builder writes this path concurrently.
-            fs::path stdCppm = stdPcm.parent_path() / "std.cppm";
+            // Per-PID std.cppm and a tmp .pcm + atomic rename so concurrent
+            // crafter-build invocations sharing the cache dir don't read each
+            // other's half-written files.
+            fs::path stdCppm = stdPcm.parent_path() / std::format("std.cppm.{}", CurrentProcessId());
             std::error_code copyEc;
             fs::copy_file(stdCc, stdCppm, fs::copy_options::overwrite_existing, copyEc);
             if (copyEc) {
                 return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), copyEc.message());
             }
-            return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), stdPcm.string()));
+            fs::path tmpPcm = MakeTempPcmPath(stdPcm);
+            std::string out = RunCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), tmpPcm.string()));
+            std::error_code rmEc;
+            fs::remove(stdCppm, rmEc);
+            if (!out.empty()) {
+                fs::remove(tmpPcm, rmEc);
+                return out;
+            }
+            return CommitPcm(tmpPcm, stdPcm);
         } else {
             return "";
         }
@@ -779,7 +797,14 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
             ? std::string(" -fno-exceptions -msimd128 -fno-c++-static-destructors -mllvm -wasm-enable-sjlj -D_WASI_EMULATED_SIGNAL")
             : std::format(" -march={} -mtune={}", config.march, config.mtune);
         if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdCppm)) {
-            return RunCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, stdPcm.string()));
+            fs::path tmpPcm = MakeTempPcmPath(stdPcm);
+            std::string out = RunCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, tmpPcm.string()));
+            if (!out.empty()) {
+                std::error_code ec;
+                fs::remove(tmpPcm, ec);
+                return out;
+            }
+            return CommitPcm(tmpPcm, stdPcm);
         } else {
             return "";
         }
@@ -823,7 +848,6 @@ namespace {
     };
 
     void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) {
-        CacheLock lock(cacheDir);
         for (std::string_view name : kCrafterBuildModules) {
             fs::path cppmPath = sourceDir / (std::string(name) + ".cppm");
             fs::path pcmPath = cacheDir / (std::string(name) + ".pcm");
@@ -833,17 +857,23 @@ namespace {
             if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) {
                 continue;
             }
+            fs::path tmpPcm = MakeTempPcmPath(pcmPath);
             std::string cmd = std::format(
                 "clang++ --target=x86_64-pc-linux-gnu -march=native -mtune=native "
                 "-std=c++26 -stdlib=libc++ -O3 "
                 "-Wno-reserved-identifier -Wno-reserved-module-identifier "
                 "-fprebuilt-module-path={} "
                 "--precompile {} -o {}",
-                cacheDir.string(), cppmPath.string(), pcmPath.string());
+                cacheDir.string(), cppmPath.string(), tmpPcm.string());
             CommandResult r = Crafter::RunCommandChecked(cmd);
             if (r.exitCode != 0) {
+                std::error_code ec;
+                fs::remove(tmpPcm, ec);
                 throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output));
             }
+            if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) {
+                throw std::runtime_error(std::format("Failed to commit {} pcm: {}", name, err));
+            }
         }
     }
 }

tests/ConcurrentCacheRace/main.cpp

@@ -9,10 +9,9 @@ using namespace Crafter;
 // clobber each other's writes to `<cache>/<target>-<march>/std.pcm` and the
 // `Crafter.Build-*.pcm` host modules. The loser's clang would then read a
 // torn file via -fprebuilt-module-path and die with "malformed or corrupted
-// precompiled file: 'can't skip to bit X from Y'". The fix serializes PCM
-// (re)builds through an exclusive advisory lock on the cache dir: only one
-// builder precompiles while the rest wait and then reuse the finished file, so
-// no two writers ever touch the same path and a reader never sees a torn one.
+// precompiled file: 'can't skip to bit X from Y'". The fix routes every PCM
+// write through a per-process temp + atomic rename so a reader always sees
+// either the old or the new file, never a half-written one.
 //
 // LoadProject() is the right surface to test because it runs the full host
 // bootstrap (BuildStdPcm + EnsureCrafterBuildPcms × the Crafter.Build-*