Browse Source

feat: an RSS reader can skip a feed it cannot read, and saving keeps a workflow on

Reading several feeds only survives one being down if the reader is allowed
to come back empty. rss-reader gains skipOnError: it returns success false
with the reason and no items, shaped like a successful read of an empty feed
so a merge or a loop downstream does not have to know the difference. Off by
default, so nothing that reads one feed changes behaviour - checked, not
assumed.

Both 35photo2anime readers now have it. Verified: with one feed pointed at an
unreachable address, the other feed's items still arrived through the merge
and the run completed.

That created a quieter failure than the one it removed. With both feeds
skippable, a run where neither answered ends having done nothing, which looks
exactly like a quiet day - and the loop's done branch went nowhere, so there
was nothing to notice it. It now leads to a check on the merged count and a
message when nothing came back at all. Verified by pointing both feeds at an
unreachable address: the run completed, the merge reported 0, and the message
was sent.

Separately, and worse: PUT /workflows/{id} is a replace, so a body that did
not mention "active" switched the workflow off. Saving a change to a node was
therefore a way to stop a scheduled workflow, silently and with nothing
recorded to say why - which is exactly the unexplained "Inactive" the failure
counter was written to avoid. I did it to 35photo2anime while editing it. An
update that says nothing about active now leaves it alone, and the same for
the two fields that explain a workflow switching itself off, so an ordinary
save cannot erase the explanation.

65 passed, two new fixtures: one feed down and the others carry on, and the
default still failing.
fszontagh 1 tháng trước cách đây
mục cha
commit
17b8607a95

+ 36 - 1
nodes/rss/rss-reader.js

@@ -54,6 +54,10 @@ const configSchema = {
             type: 'number', title: 'Max Retry Delay (ms)', default: 30000,
             description: 'Upper bound for the backoff'
         },
+        skipOnError: {
+            type: 'boolean', title: 'Skip On Error', default: false,
+            description: 'Return success false and no items instead of failing the workflow. For a workflow reading several feeds, this is what lets the others carry on when one is unreachable - without it the first bad feed ends the run'
+        },
         detectNewItems: {
             type: 'boolean',
             title: 'Detect New Items',
@@ -101,6 +105,8 @@ const inputSchema = {
 const outputSchema = {
     type: 'object',
     properties: {
+        success: { type: 'boolean', description: 'False when the feed could not be read and Skip On Error let the run carry on' },
+        error: { type: 'string', description: 'Why it could not be read, when it could not' },
         feedTitle: { type: 'string', description: 'Title of the feed' },
         feedLink: { type: 'string', description: 'Link to the feed homepage' },
         feedDescription: { type: 'string', description: 'Description of the feed' },
@@ -699,7 +705,7 @@ function filterNewItems(items, seenIds) {
     return newItems;
 }
 
-async function execute(config, input) {
+async function readFeed(config, input) {
     var url = input.url || config.url;
     var filterKeyword = input.filterKeyword || config.filterKeyword;
     var filterNewerThan = input.filterNewerThan || config.filterNewerThan;
@@ -863,4 +869,33 @@ async function execute(config, input) {
     return result;
 }
 
+async function execute(config, input) {
+    try {
+        var result = await readFeed(config, input);
+        result.success = true;
+        result.error = '';
+        return result;
+    } catch (err) {
+        if (config.skipOnError !== true) {
+            throw err;
+        }
+        var message = (err && err.message) ? err.message : String(err);
+        smartbotic.log.warn('RSS Reader: ' + (config.url || '') + ' could not be read, skipping: ' + message);
+        // Shaped like a successful read with nothing in it, so a merge or a
+        // loop downstream does not have to know the difference - and carrying
+        // the reason means a run that produced nothing can still say why.
+        return {
+            success: false,
+            error: message,
+            feedTitle: '',
+            feedLink: '',
+            feedDescription: '',
+            itemCount: 0,
+            items: [],
+            newItemCount: 0,
+            totalFeedItemCount: 0
+        };
+    }
+}
+
 module.exports = { configSchema, inputSchema, outputSchema, execute };

+ 20 - 0
src/webserver/api/workflow_controller.cpp

@@ -250,6 +250,26 @@ void WorkflowController::updateWorkflow(const httplib::Request& req, httplib::Re
             was_active = before.value().value("active", false);
         }
 
+        // This is a replace, not a merge, so a body that does not mention
+        // "active" would drop it - and a workflow that was running quietly
+        // stops, with nothing recorded to say why. Saving a change to a node
+        // must not be a way to switch a workflow off; that is what
+        // activate/deactivate are for, and what the failure counter records a
+        // reason for.
+        if (!body.contains("active") && before.ok()) {
+            body["active"] = was_active;
+        }
+        // The same for the two fields that explain a workflow switching itself
+        // off, so an ordinary save does not erase the explanation.
+        if (!body.contains("consecutiveFailures") && before.ok() &&
+            before.value().contains("consecutiveFailures")) {
+            body["consecutiveFailures"] = before.value()["consecutiveFailures"];
+        }
+        if (!body.contains("deactivatedReason") && before.ok() &&
+            before.value().contains("deactivatedReason")) {
+            body["deactivatedReason"] = before.value()["deactivatedReason"];
+        }
+
         materializeNodeConfigDefaults(body);
 
         auto result = storage_.update("workflows", id, body, 0, true);

+ 28 - 0
tests/nodes/rss-skip-on-error.json

@@ -0,0 +1,28 @@
+{
+  "name": "verify-rss-skip-on-error",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "dead", "name": "Dead Feed", "type": "rss-reader", "position": {"x": -150, "y": 100},
+     "config": {"url": "http://10.255.255.1/feed.xml", "timeout": 1000, "retries": 0,
+                "skipOnError": true, "detectNewItems": false, "maxItems": 0}},
+    {"id": "alive", "name": "Live Feed", "type": "code", "position": {"x": 150, "y": 100},
+     "config": {"code": "return { items: [ { title: 'one', enclosure: { url: 'https://cdn/1.jpg' } }, { title: 'two', enclosure: { url: 'https://cdn/2.jpg' } } ] };"}},
+    {"id": "merge", "name": "Merge", "type": "merge", "position": {"x": 0, "y": 220},
+     "config": {"mode": "combine-by-key", "key": "enclosure.url", "path1": "items", "path2": "result.items"}},
+    {"id": "check", "name": "Check", "type": "code", "position": {"x": 0, "y": 340},
+     "config": {"code": "const d = input.data || input; return { survived: (d.merged || []).length };"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "dead", "targetInput": "data"},
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "alive", "targetInput": "data"},
+    {"sourceNodeId": "dead", "sourceOutput": "main", "targetNodeId": "merge", "targetInput": "input1"},
+    {"sourceNodeId": "alive", "sourceOutput": "main", "targetNodeId": "merge", "targetInput": "input2"},
+    {"sourceNodeId": "merge", "sourceOutput": "main", "targetNodeId": "check", "targetInput": "data"}
+  ],
+  "expectStatus": "completed",
+  "expect": {
+    "dead": {"status": "completed", "output": {"success": false, "itemCount": 0}},
+    "merge": {"status": "completed", "output": {"count": 2}},
+    "check": {"status": "completed", "output": {"result": {"survived": 2}}}
+  }
+}