Fix postponed group/member state race

2 files changed, 20 insertions, 6 deletions

diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx
index 8ee6d47..6743907 100644
--- a/build2/algorithm.cxx
+++ b/build2/algorithm.cxx
@@ -1790,15 +1790,29 @@ namespace build2
     //
     const target& g (*t.group);
 
-    if (execute (a, g) == target_state::busy)
+    target_state gs (execute (a, g));
+
+    if (gs == target_state::busy)
       sched.wait (target::count_executed (),
                   g[a].task_count,
                   scheduler::work_none);
 
-    // Indicate to execute() that this target's state comes from the group
-    // (which, BTW, can be failed).
+    // Return target_state::group to signal to execute() that this target's
+    // state comes from the group (which, BTW, can be failed).
+    //
+    // There is just one small problem: if the returned group state is
+    // postponed, then this means the group hasn't been executed yet. And if
+    // we return target_state::group, then this means any state queries (see
+    // executed_state()) will be directed to the target which might still not
+    // be executed or, worse, is being executed as we query.
+    //
+    // So in this case we return target_state::postponed (which will result in
+    // the member being treated as unchanged). This is how it is done for
+    // prerequisites and seeing that we've been acting as if the group is our
+    // prerequisite, there is no reason to deviate (see the recipe return
+    // value documentation for details).
     //
-    return target_state::group;
+    return gs != target_state::postponed ? target_state::group : gs;
   }
 
   target_state
diff --git a/build2/target.ixx b/build2/target.ixx
index ea91b6e..88bc530 100644
--- a/build2/target.ixx
+++ b/build2/target.ixx
@@ -94,14 +94,14 @@ namespace build2
   {
     // We go an extra step and short-circuit to the target state even if the
     // raw state is not group provided the recipe is group_recipe and the
-    // state is not failed.
+    // state is unknown (see mtime() for some ideas on why we do it).
     //
     const opstate& s (state[a]);
 
     if (s.state == target_state::group)
       return true;
 
-    if (s.state != target_state::failed && group != nullptr)
+    if (s.state == target_state::unknown && group != nullptr)
     {
       if (recipe_function* const* f = s.recipe.target<recipe_function*> ())
         return *f == &group_action;