Quellcode durchsuchen

fix: Stop interrupts the node that is running, instead of waiting for it

Cancellation was only looked at between nodes, so pressing Stop during a long
one did nothing until that node finished. Measured before: a Stop 6 seconds
into a 60-second Wait ended the run 54 seconds later. On a render or a slow
request it is minutes, which is indistinguishable from Stop being broken.

The mechanism was already there and never connected. The script engine has a
cancel flag that QuickJS's interrupt handler reads, but nothing told the
engine running the node that its execution had been cancelled. The workflow
engine now tracks which script engine is running which execution and cancels
it directly.

The interrupt handler only stops JavaScript between operations, and the long
waits are not JavaScript - they are C++ blocking calls that never yield to it.
So utils.sleep and the HTTP retry backoff now wait in slices and give up when
cancelled, which is what makes a Wait node stoppable at all.

A cancelled node reported as failed, which was wrong twice over: it put a red
mark on the list for something the user asked for, and it would set off the
error workflow for it. A node interrupted while the run is cancelled now ends
the run as cancelled. In a loop body it also breaks out immediately rather
than letting continue-on-error march through every remaining node failing
each one the same way.

Measured after: the same Stop ends the run in 0.5 seconds, as cancelled.

The engine is registered under a lock that also re-checks the flag, so a
cancel landing between acquiring the engine and starting the node is not
missed - that window would otherwise run the node in full.

62/62.
fszontagh vor 1 Monat
Ursprung
Commit
ad955dad1f

+ 35 - 2
src/runner/engine/script_engine.cpp

@@ -926,6 +926,29 @@ static CurlResponse performHttpRequest(
 }
 
 
+// Has this run been cancelled? Callable from any built-in: the interrupt
+// handler stops JavaScript between operations, but a built-in that blocks in
+// C++ never yields to it, and those are exactly the long ones.
+static bool executionCancelled(JSContext* ctx) {
+    auto* engine = static_cast<ScriptEngine*>(JS_GetRuntimeOpaque(JS_GetRuntime(ctx)));
+    return engine && engine->isCancelled();
+}
+
+// Sleep, but wake up if the run is cancelled. Pressing Stop during a Wait node
+// used to do nothing until the wait was over, which for a long one is
+// indistinguishable from Stop being broken.
+static bool sleepUnlessCancelled(JSContext* ctx, int64_t ms) {
+    const int64_t slice = 50;
+    for (int64_t waited = 0; waited < ms; waited += slice) {
+        if (executionCancelled(ctx)) {
+            return false;
+        }
+        const int64_t chunk = (ms - waited) < slice ? (ms - waited) : slice;
+        std::this_thread::sleep_for(std::chrono::milliseconds(chunk));
+    }
+    return !executionCancelled(ctx);
+}
+
 // Which failures are worth trying again. A timeout, a refused connection or a
 // name that would not resolve are the network being briefly unwell; a 500 or a
 // 503 is the far end being briefly unwell; a 429 is being told to slow down.
@@ -1260,7 +1283,9 @@ static JSValue js_http_request(JSContext* ctx, JSValue this_val, int argc, JSVal
                          ? ("HTTP " + std::to_string(response.status_code))
                          : last_transport_error,
                      wait_ms, attempt + 1, attempts);
-            std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
+            if (!sleepUnlessCancelled(ctx, wait_ms)) {
+                throw std::runtime_error("Execution cancelled while waiting to retry");
+            }
         }
 
         // Create response object
@@ -1339,6 +1364,12 @@ void ScriptEngine::initRuntime() {
     // Set interrupt handler for cancellation support
     JS_SetInterruptHandler(runtime_, interruptHandler, this);
 
+    // The same engine, reachable from any built-in that has a context. The
+    // interrupt handler covers running JavaScript; anything that blocks in C++
+    // - a sleep, a retry backoff - has to check for itself, and this is how it
+    // finds out.
+    JS_SetRuntimeOpaque(runtime_, this);
+
     // Create context
     context_ = JS_NewContext(runtime_);
     if (!context_) {
@@ -1474,7 +1505,9 @@ void ScriptEngine::setupBuiltinAPIs() {
             ms = 300000;
         }
 
-        std::this_thread::sleep_for(std::chrono::milliseconds(ms));
+        if (!sleepUnlessCancelled(ctx, ms)) {
+            return JS_ThrowInternalError(ctx, "Execution cancelled while waiting");
+        }
         return JS_UNDEFINED;
     }, "sleep", 1));
 

+ 78 - 3
src/runner/workflow_engine.cpp

@@ -828,6 +828,21 @@ Result<ExecutionResult> WorkflowEngine::execute(const Workflow& workflow,
 
             // Check for failure
             if (node_result.status == NodeStatus::Failed) {
+                // A node interrupted because someone pressed Stop did not fail -
+                // it was stopped. Recording that as a failure would put a red
+                // mark on the list for something the user asked for, and would
+                // set off the error workflow for it.
+                bool was_cancelled = false;
+                {
+                    std::lock_guard<std::mutex> lock(mutex_);
+                    was_cancelled = cancelled_executions_.contains(result.execution_id);
+                }
+                if (was_cancelled) {
+                    result.status = ExecutionStatus::Cancelled;
+                    result.error = "Execution cancelled";
+                    break;
+                }
+
                 bool should_continue = workflow.settings.value("continueOnError", false);
                 if (!should_continue) {
                     result.status = ExecutionStatus::Failed;
@@ -1074,8 +1089,23 @@ void WorkflowEngine::cancelExecution(const std::string& execution_id) {
         }
     }
 
-    LOG_INFO("Cancellation requested for execution {} ({} execution(s) marked)",
-             execution_id, cancelled);
+    // Interrupt whatever is running right now. Without this the flag is only
+    // noticed between nodes, and a node that takes minutes keeps the whole run
+    // going for minutes after Stop was pressed.
+    size_t interrupted = 0;
+    for (const auto& id : cancelled_executions_) {
+        auto engines = running_engines_.find(id);
+        if (engines == running_engines_.end()) {
+            continue;
+        }
+        for (auto* running : engines->second) {
+            running->cancel();
+            ++interrupted;
+        }
+    }
+
+    LOG_INFO("Cancellation requested for execution {} ({} execution(s) marked, {} interrupted)",
+             execution_id, cancelled, interrupted);
 }
 
 int WorkflowEngine::getActiveExecutionCount() const {
@@ -1534,7 +1564,42 @@ NodeExecutionResult WorkflowEngine::executeNode(const WorkflowNode& node,
         result.retry_count = attempt;
 
         auto* script_engine = script_pool_->acquire();
+
+        // Registered before it runs, so a Stop arriving mid-node reaches the
+        // engine actually doing the work. A cancel that landed between the
+        // acquire and here would otherwise be missed entirely, so the flag is
+        // checked once more under the same lock.
+        bool already_cancelled = false;
+        {
+            std::lock_guard<std::mutex> lock(mutex_);
+            if (cancelled_executions_.contains(execution_id)) {
+                already_cancelled = true;
+            } else {
+                running_engines_[execution_id].insert(script_engine);
+            }
+        }
+
+        if (already_cancelled) {
+            script_pool_->release(script_engine);
+            result.status = NodeStatus::Failed;
+            result.error = "Execution cancelled";
+            result.finished_at = TimeUtils::nowMs();
+            return result;
+        }
+
         auto script_result = script_engine->execute(node_def->code, ctx);
+
+        {
+            std::lock_guard<std::mutex> lock(mutex_);
+            auto it = running_engines_.find(execution_id);
+            if (it != running_engines_.end()) {
+                it->second.erase(script_engine);
+                if (it->second.empty()) {
+                    running_engines_.erase(it);
+                }
+            }
+        }
+
         script_pool_->release(script_engine);
 
         if (script_result.success) {
@@ -2656,7 +2721,17 @@ bool WorkflowEngine::executeLoopBody(
             if (body_result.status == NodeStatus::Failed) {
                 iteration_failed = true;
                 all_succeeded = false;
-                if (!ctx.continue_on_error) {
+
+                // A node interrupted by Stop is not a failure to carry on past.
+                // Continue-on-error would otherwise march through every
+                // remaining body node, failing each one the same way, before
+                // the between-iteration check finally noticed.
+                bool was_cancelled = false;
+                {
+                    std::lock_guard<std::mutex> lock(mutex_);
+                    was_cancelled = cancelled_executions_.contains(result.execution_id);
+                }
+                if (was_cancelled || !ctx.continue_on_error) {
                     break;
                 }
             }

+ 6 - 0
src/runner/workflow_engine.hpp

@@ -361,6 +361,12 @@ private:
     // nothing about it - and the caller is sitting inside that call, unable to
     // notice its own cancellation until the call returns.
     std::unordered_map<std::string, std::unordered_set<std::string>> child_executions_;
+
+    // Which script engines are running which executions right now. Cancelling
+    // used to be noticed only between nodes, so a Stop during a long node - a
+    // wait, a generation, a slow request - did nothing until that node finished.
+    // Telling the engine directly is what makes Stop mean now.
+    std::unordered_map<std::string, std::unordered_set<engine::ScriptEngine*>> running_engines_;
     mutable std::mutex mutex_;
     std::atomic<int> active_count_{0};
 };