Przeglądaj źródła

fix: a loop where every item failed no longer reports the run as successful

Continue On Error is a per-item policy - one unreadable image should not
abandon the other forty - and it was being read as a per-run policy too. A pass
in which every single item failed still finished as completed.

That is how an outage stayed invisible. Ollama began answering 403 (a past-due
subscription), so every image failed vision analysis; the sub-workflow counted
31 consecutive failures while the parent's counter sat at zero, because the
parent kept reporting success. No threshold could ever have tripped on that,
and the only signal reaching anyone was the error text itself, five times a
run, every fifteen minutes.

The loop now counts its passes and how many failed. All failed, and the run
fails - partial failures behave exactly as before. The run's error leads with
the systemic case:

  All 3 items failed, so the run failed even though the loop tolerates errors.
  Loop body node "Vision Analyse" failed on item 2: ...

"failed on item 2" alone reads like one bad item; it is the difference between
a bad photo and a dead dependency, and it decides whether anyone goes and looks.
fszontagh 1 miesiąc temu
rodzic
commit
fa54616ae6
2 zmienionych plików z 47 dodań i 1 usunięć
  1. 35 1
      src/runner/workflow_engine.cpp
  2. 12 0
      src/runner/workflow_engine.hpp

+ 35 - 1
src/runner/workflow_engine.cpp

@@ -1075,7 +1075,21 @@ Result<ExecutionResult> WorkflowEngine::execute(const Workflow& workflow,
                     });
                 }
 
-                if (!loop_success && !loop_ctx.continue_on_error) {
+                // Tolerating failures item by item is the point of
+                // continue_on_error, but a pass in which nothing at all
+                // succeeded is not a tolerated failure - it is the run failing.
+                // Reporting it as success is what let a dead dependency spam
+                // errors for hours while the workflow looked healthy.
+                const bool nothing_succeeded = loop_ctx.iterations_run > 0 &&
+                                               loop_ctx.iterations_failed ==
+                                                   loop_ctx.iterations_run;
+                if (nothing_succeeded && loop_ctx.continue_on_error) {
+                    LOG_WARN("Loop {} tolerates failures, but all {} of its iterations failed - "
+                             "failing the run rather than reporting success",
+                             node_id, loop_ctx.iterations_run);
+                }
+
+                if (!loop_success && (!loop_ctx.continue_on_error || nothing_succeeded)) {
                     result.status = ExecutionStatus::Failed;
 
                     // "Loop iteration failed" on its own says nothing, and it is
@@ -1106,6 +1120,17 @@ Result<ExecutionResult> WorkflowEngine::execute(const Workflow& workflow,
                     }
 
                     result.error = detail.empty() ? "Loop iteration failed" : detail;
+
+                    // "failed on item 2" reads like one bad item among many.
+                    // When every item failed, that is the headline - it is the
+                    // difference between a bad photo and a dead dependency, and
+                    // it is what tells whoever is reading to go and fix
+                    // something rather than ignore it.
+                    if (nothing_succeeded) {
+                        result.error = "All " + std::to_string(loop_ctx.iterations_run) +
+                                       " items failed, so the run failed even though the loop "
+                                       "tolerates errors. " + result.error;
+                    }
                     break;
                 }
 
@@ -3103,6 +3128,8 @@ bool WorkflowEngine::executeLoopBody(
     // Mutable copy of loop context for iteration tracking
     LoopContext ctx = loop_ctx;
     bool all_succeeded = true;
+    int iterations_run = 0;
+    int iterations_failed = 0;
 
     // Testing one node from the editor: the node is asked for once, so the body
     // runs for a single item and stops at the node under test. Without this the
@@ -3722,6 +3749,11 @@ bool WorkflowEngine::executeLoopBody(
             ctx.results.push_back(ctx.items[i]);
         }
 
+        ++iterations_run;
+        if (iteration_failed) {
+            ++iterations_failed;
+        }
+
         if (callback) {
             callback("loop.iteration.end", {
                 {"executionId", result.execution_id},
@@ -3756,6 +3788,8 @@ bool WorkflowEngine::executeLoopBody(
 
     // Copy results back (ctx was passed by const ref, but we used a mutable copy)
     const_cast<LoopContext&>(loop_ctx).results = ctx.results;
+    const_cast<LoopContext&>(loop_ctx).iterations_run = iterations_run;
+    const_cast<LoopContext&>(loop_ctx).iterations_failed = iterations_failed;
 
     // Store a lookup mirror for every body node under its own plain, never-
     // prefixed id - never as one of the caller's own results (an expression

+ 12 - 0
src/runner/workflow_engine.hpp

@@ -463,6 +463,18 @@ private:
         bool continue_on_error;
         std::vector<nlohmann::json> results;
         size_t current_index = 0;
+
+        // How many body passes ran, and how many of those failed. Filled in by
+        // executeLoopBody on the way out.
+        //
+        // continue_on_error is a per-item policy: one unreadable image should
+        // not abandon the other forty. It was being read as a per-run policy
+        // too, so a run in which every single item failed still reported
+        // success - during an outage the pipeline logged failure after failure
+        // while the workflow's own failure counter sat at zero and no
+        // threshold could ever trip.
+        int iterations_run = 0;
+        int iterations_failed = 0;
     };
 
     // target_node_id and cached_outputs are set only when a single node is