Merge pull request 'fix: scope per-build module-state reset to the config being built (#16)' (#17) from claude/issue-16 into master

Reviewed-on: #17

implementations/Crafter.Build-Clang.cpp

@@ -215,13 +215,24 @@ void Configuration::GetInterfacesAndImplementations(std::span<fs::path> interfac
 BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, std::shared_future<BuildResult>>& depResults, std::mutex& depMutex) {
     // Reset per-build cached state on every Module/ModulePartition so that
     // successive Build() calls on the same Configuration re-evaluate mtimes
-    // (incremental-rebuild test scenarios). Walks cfg.dependencies recursively
-    // with a seen-set so diamond deps don't loop.
-    {
-        std::unordered_set<Configuration*> resetSeen;
-        std::function<void(Configuration*)> reset = [&](Configuration* c) {
-            if (!resetSeen.insert(c).second) return;
-            for (auto& iface : c->interfaces) {
+    // (incremental-rebuild test scenarios).
+    //
+    // Reset ONLY this configuration's own modules — never recurse into
+    // dependencies. Configuration objects are shared across the dependency DAG
+    // (diamond deps point at the same Configuration*), and every config in the
+    // tree gets its own Build() call: the per-PcmDir builder registered in
+    // depResults resets its own state here, at the top of its own Build(),
+    // sequenced before that config's compile threads spawn. Recursing into
+    // dependencies from here would re-clear a shared dependency's `compiled`
+    // atomic from a parent/sibling thread *while that dependency's own Build()
+    // is concurrently compiling it* — after its module-compile thread set the
+    // flag true and exited but before its impl/partition waiter ran
+    // compiled.wait(false). The waiter would then block forever on a flag
+    // nothing re-signals: the build freezes mid-compile, idle, with no compiler
+    // process alive (issue #16). Cross-config module state is consulted only via
+    // PCM file mtimes and the depResults futures, never via these flags, so the
+    // narrower reset is correct.
+    for (auto& iface : config.interfaces) {
         iface->checked = false;
         iface->compiled.store(false);
         for (auto& part : iface->partitions) {
@@ -229,12 +240,6 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
             part->compiled.store(false);
         }
     }
-            for (Configuration* dep : c->dependencies) {
-                reset(dep);
-            }
-        };
-        reset(&config);
-    }
 
     // Auto-detect the WASI sysroot before any compile step runs so BuildStdPcm
     // and the main compile command see the same value. Linux-only — Windows

project.cpp

@@ -113,6 +113,7 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> a
         // for project.so.
         cfg.AddTest("ConcurrentCacheRace").Dependencies({ crafterBuildLib.get() })
             .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl");
+        cfg.AddTest("ConcurrentDependencyReset").Dependencies({ crafterBuildLib.get() });
     }
 
     return cfg;

tests/ConcurrentDependencyReset/fixture/app/main.cpp

@@ -0,0 +1,7 @@
+import std;
+import DepMod;
+
+int main() {
+    std::print("{}", dep_value() * 2);
+    return 0;
+}

tests/ConcurrentDependencyReset/fixture/deplib/DepMod-impl.cpp

@@ -0,0 +1,7 @@
+module DepMod;
+
+// Implementation unit of the DepMod interface. Because it declares
+// `module DepMod;` it gains an intra-config module dependency on the DepMod
+// interface, so its compile waits on DepMod's `compiled` flag — the exact
+// flag a stale recursive reset used to clobber (issue #16).
+int dep_value() { return 21; }

tests/ConcurrentDependencyReset/fixture/deplib/DepMod.cppm

@@ -0,0 +1,2 @@
+export module DepMod;
+export int dep_value();

tests/ConcurrentDependencyReset/main.cpp

@@ -0,0 +1,126 @@
+import std;
+import Crafter.Build;
+namespace fs = std::filesystem;
+using namespace Crafter;
+
+// Regression for the concurrent dependency-reset deadlock (issue #16).
+//
+// Build() resets each Module/ModulePartition's per-build `compiled` /`checked`
+// flags so a reused Configuration re-evaluates mtimes. That reset MUST stay
+// scoped to the configuration being built. It used to recurse into
+// cfg.dependencies — but dependency Configurations are shared across the build
+// DAG and each is compiled concurrently by its own Build() call. A parent's
+// recursive reset could therefore clear a dependency's module `compiled` flag
+// *after* that dependency's module-compile thread had set it true and exited,
+// but before an intra-config waiter (its impl / a dependent partition) ran
+// `compiled.wait(false)`. The waiter then blocked forever on a flag nothing
+// re-signalled: the build froze mid-compile, idle, with no compiler process
+// alive.
+//
+// This test pins the invariant deterministically, without depending on thread
+// timing: build a static-lib dependency fully (its module ends up compiled),
+// then build a consumer that depends on it while the dependency is already
+// cached in depResults (so it is NOT rebuilt). Before the fix, the consumer's
+// recursive reset flipped the dependency's module `compiled` flag back to false
+// and, because the dependency never rebuilds, it stayed false. After the fix
+// the consumer resets only its own modules, so the dependency's flag is left
+// intact. We assert it is still set once the consumer build completes.
+int main() {
+    fs::path fixtureRoot = fs::current_path() / "tests" / "ConcurrentDependencyReset" / "fixture";
+
+    // Cold output: a stale archive/PCM from a previous run would let the
+    // dependency build short-circuit and skip the module compile we rely on.
+    std::error_code ec;
+    fs::remove_all(fixtureRoot / "deplib" / "build", ec);
+    fs::remove_all(fixtureRoot / "deplib" / "bin", ec);
+    fs::remove_all(fixtureRoot / "app" / "build", ec);
+    fs::remove_all(fixtureRoot / "app" / "bin", ec);
+
+    Configuration dep;
+    dep.path = fixtureRoot / "deplib";
+    dep.name = "depmod";
+    dep.outputName = "depmod";
+    dep.target = HostTarget();
+    dep.type = ConfigurationType::LibraryStatic;
+    {
+        std::array<fs::path, 1> ifaces = { "DepMod" };
+        std::array<fs::path, 1> impls = { "DepMod-impl" };
+        dep.GetInterfacesAndImplementations(ifaces, impls);
+    }
+
+    if (dep.interfaces.size() != 1) {
+        std::println(std::cerr, "expected 1 dependency interface, got {}", dep.interfaces.size());
+        return 1;
+    }
+
+    {
+        std::unordered_map<fs::path, std::shared_future<BuildResult>> depResults;
+        std::mutex depMutex;
+        BuildResult r = Build(dep, depResults, depMutex);
+        if (!r.result.empty()) {
+            std::println(std::cerr, "dependency build failed: {}", r.result);
+            return 1;
+        }
+        if (!dep.interfaces[0]->compiled.load()) {
+            std::println(std::cerr, "dependency module not marked compiled after its own build");
+            return 1;
+        }
+        // Seed a consumer-facing cache so the consumer reuses this build instead
+        // of rebuilding the dependency — mirrors how a shared dep is built once
+        // and consumed by multiple parents through depResults.
+        std::promise<BuildResult> promise;
+        promise.set_value(std::move(r));
+        std::shared_future<BuildResult> ready = promise.get_future().share();
+
+        std::unordered_map<fs::path, std::shared_future<BuildResult>> consumerDeps;
+        consumerDeps.emplace(dep.PcmDir(), ready);
+
+        Configuration app;
+        app.path = fixtureRoot / "app";
+        app.name = "resetapp";
+        app.outputName = "resetapp";
+        app.target = HostTarget();
+        app.type = ConfigurationType::Executable;
+        app.dependencies = { &dep };
+        {
+            std::array<fs::path, 0> appIfaces = {};
+            std::array<fs::path, 1> appImpls = { "main" };
+            app.GetInterfacesAndImplementations(appIfaces, appImpls);
+        }
+
+        BuildResult ar = Build(app, consumerDeps, depMutex);
+        if (!ar.result.empty()) {
+            std::println(std::cerr, "consumer build failed: {}", ar.result);
+            return 1;
+        }
+
+        // The core regression assertion: building the consumer must not have
+        // reset the (cached, never-rebuilt) dependency's module flag. Before the
+        // fix this was false and an intra-config waiter would have deadlocked.
+        if (!dep.interfaces[0]->compiled.load()) {
+            std::println(std::cerr,
+                "FAIL: consumer build reset a cached dependency's module 'compiled' "
+                "flag (issue #16 regression) — this is the state that deadlocks a "
+                "concurrent build");
+            return 1;
+        }
+
+        fs::path bin = app.BinDir() / "resetapp";
+        if (!fs::exists(bin)) {
+            std::println(std::cerr, "consumer binary not produced at {}", bin.string());
+            return 1;
+        }
+        auto run = RunCommandWithTimeout(bin.string(), std::chrono::seconds(10));
+        if (run.exitCode != 0 || run.timedOut || run.crashed) {
+            std::println(std::cerr, "consumer exe did not exit cleanly: exit={} output={}",
+                run.exitCode, run.output);
+            return 1;
+        }
+        if (run.output != "42") {
+            std::println(std::cerr, "expected '42', got '{}'", run.output);
+            return 1;
+        }
+    }
+
+    return 0;
+}