Przeglądaj źródła

fix: a loop back-edge now depends on which branch was actually taken

back_edge_sources recorded which nodes were wired back to the loop but not
which port the edge left from, and a node that decides a branch completes
whichever way it decides. So an if-condition inside a loop body with only its
true port wired back kept the loop going when it chose false - the one case
its author wrote it to stop on.

Reproduced first, on a three-item loop whose first item took the false branch:
the run went on to process the other two. It now stops after the first, and
the nodes after the loop still run, which is what leaving a branch unwired has
always meant.

The rule matches the main walk's: an unnamed or "main" port is not a branch and
always carries, otherwise the port has to be the one the node chose. A node
that has no _activeBranch at all is unaffected, so a loop wired back through
ordinary nodes iterates exactly as before - checked, along with the same
workflow when every item takes the true branch: all three items, as expected.

Full node suite: 92 passed, 0 failed.
fszontagh 1 miesiąc temu
rodzic
commit
b67eb3b56d
1 zmienionych plików z 37 dodań i 6 usunięć
  1. 37 6
      src/runner/workflow_engine.cpp

+ 37 - 6
src/runner/workflow_engine.cpp

@@ -2824,18 +2824,49 @@ bool WorkflowEngine::executeLoopBody(
     // incoming edge carrying its data, and that edge targets the loop node too -
     // counting it would make every loop in existence look explicitly wired back
     // and stop them all after one item.
-    std::unordered_set<std::string> back_edge_sources;
+    //
+    // Which port the edge leaves from is kept, not just which node it leaves.
+    // A node that decides a branch completes whichever way it decides, so
+    // matching on the node alone said "wired back" for a run that took the
+    // other branch: an if-condition inside a body with only its true port
+    // wired back kept the loop going on false, which is the one case the
+    // author wrote it to stop. Verified before fixing, on a three-item loop
+    // whose first item took the false branch and which went on to process the
+    // other two.
+    std::unordered_map<std::string, std::vector<std::string>> back_edge_ports;
     for (const auto& conn : workflow.connections) {
         if (conn.target_node_id == loop_node_id &&
             conn.source_node_id != loop_node_id &&
             body_nodes_set.contains(conn.source_node_id)) {
-            back_edge_sources.insert(conn.source_node_id);
+            back_edge_ports[conn.source_node_id].push_back(conn.source_output);
         }
     }
-    const bool explicit_loop_back = !back_edge_sources.empty();
+    const bool explicit_loop_back = !back_edge_ports.empty();
+
+    // Whether this node's result actually travelled down an edge back to the
+    // loop. The branch rule is the main walk's: an unnamed or "main" port is
+    // not a branch and always carries, otherwise the port has to be the one
+    // the node chose.
+    auto reachedLoopBack = [&back_edge_ports](const std::string& node_id,
+                                              const nlohmann::json& output) {
+        auto found = back_edge_ports.find(node_id);
+        if (found == back_edge_ports.end()) {
+            return false;
+        }
+        if (!output.contains("_activeBranch") || !output["_activeBranch"].is_string()) {
+            return true;
+        }
+        const std::string active = output["_activeBranch"].get<std::string>();
+        for (const auto& port : found->second) {
+            if (port.empty() || port == "main" || port == active) {
+                return true;
+            }
+        }
+        return false;
+    };
     if (explicit_loop_back) {
         LOG_DEBUG("Loop {} is wired back explicitly by {} node(s); an iteration continues "
-                  "only when one of them runs", loop_node_id, back_edge_sources.size());
+                  "only when one of them runs", loop_node_id, back_edge_ports.size());
     }
 
     // Set when an iteration ended without reaching a node wired back to the
@@ -2967,7 +2998,7 @@ bool WorkflowEngine::executeLoopBody(
                 // A disabled node that decides a branch is different: it has no
                 // answer to give, so the path really does end there.
                 if (!disabled_result.output.contains("_activeBranch") &&
-                    back_edge_sources.contains(body_node_id)) {
+                    back_edge_ports.contains(body_node_id)) {
                     reached_back_edge = true;
                 }
                 continue;
@@ -3285,7 +3316,7 @@ bool WorkflowEngine::executeLoopBody(
             // A disabled node never reaches here - it is handled above, where
             // the same rule about transparency is applied.
             if (body_result.status == NodeStatus::Completed &&
-                back_edge_sources.contains(body_node_id)) {
+                reachedLoopBack(body_node_id, body_result.output)) {
                 reached_back_edge = true;
             }