|
|
@@ -0,0 +1,693 @@
|
|
|
+# configSchema defaults - write time and read time
|
|
|
+
|
|
|
+Branch: `config-defaults` (off `main`)
|
|
|
+Commit history: `3bd17d0698ca6fed6f643b5e1be57d5983c32222` (round 1) ->
|
|
|
+`de2b70b77d4480c56522e7cb9fa1d9af7d91f1fc` (round 1 fix) ->
|
|
|
+`ccdc442fc19fe235b8a1a4a46e985ed31f898c4c` (round 2 fix, below - current HEAD)
|
|
|
+
|
|
|
+## What was built
|
|
|
+
|
|
|
+- `lib/common/config_defaults.hpp` / `.cpp` (namespace `smartbotic::common`):
|
|
|
+ `nlohmann::json applyConfigDefaults(const nlohmann::json& config, const nlohmann::json& config_schema)`.
|
|
|
+ Fills in missing top-level `configSchema.properties[*].default` values. A
|
|
|
+ key already present in `config` - including an explicit `false`, `0`,
|
|
|
+ `null`, or `""` - is never touched. Tolerates a null/non-object schema or
|
|
|
+ one with no `properties`. Added to `CMakeLists.txt` under the
|
|
|
+ `smartbotic_common` target, alongside the other `lib/common/*.cpp` files.
|
|
|
+- Read-time call sites (the guarantee):
|
|
|
+ - `src/runner/workflow_engine.cpp`, just before
|
|
|
+ `evaluateExpressions(node->config, ...)` (~line 651). Looks up the node
|
|
|
+ definition via `registry_.getNode(node->type)` and applies defaults to
|
|
|
+ `node->config` before expression evaluation, so a defaulted value still
|
|
|
+ goes through expression evaluation like any other value.
|
|
|
+ - `src/webserver/webserver_service.cpp`, `loadScheduledWorkflows()`
|
|
|
+ (~line 318). Applies defaults to the trigger's stored config, using the
|
|
|
+ node definition already fetched from `node_store_`, before reading
|
|
|
+ `pollInterval`.
|
|
|
+ - `src/webserver/api/workflow_controller.cpp`,
|
|
|
+ `updateScheduledTriggers()` (~line 413-414) and
|
|
|
+ `getScheduledInterval()` (~line 440-441). Same pattern, using
|
|
|
+ `node_store_.get(node_type)`.
|
|
|
+- Write-time call sites: `src/webserver/api/workflow_controller.cpp`,
|
|
|
+ new private method `materializeNodeConfigDefaults(nlohmann::json& body)`,
|
|
|
+ called from `createWorkflow()` (after the name-required check, before
|
|
|
+ `storage_.insert`) and `updateWorkflow()` (before `storage_.update`).
|
|
|
+ Walks `body["nodes"]`, looks up each node's definition in `node_store_`,
|
|
|
+ and replaces each node's `config` with
|
|
|
+ `applyConfigDefaults(config, node_def.config_schema)`, so the saved
|
|
|
+ workflow document is self-describing.
|
|
|
+
|
|
|
+## Build
|
|
|
+
|
|
|
+Command:
|
|
|
+
|
|
|
+```
|
|
|
+cmake --build build -j$(nproc)
|
|
|
+```
|
|
|
+
|
|
|
+Full real output (tail):
|
|
|
+
|
|
|
+```
|
|
|
+[0/1] Re-running CMake...
|
|
|
+CMake Warning (dev) at /usr/share/cmake-4.2/Modules/FetchContent.cmake:1963 (message):
|
|
|
+ Calling FetchContent_Populate(bcrypt) is deprecated, call
|
|
|
+ FetchContent_MakeAvailable(bcrypt) instead. Policy CMP0169 can be set to
|
|
|
+ OLD to allow FetchContent_Populate(bcrypt) to be called directly for now,
|
|
|
+ but the ability to call it with declared details will be removed completely
|
|
|
+ in a future version.
|
|
|
+Call Stack (most recent call first):
|
|
|
+ cmake/Dependencies.cmake:54 (FetchContent_Populate)
|
|
|
+ CMakeLists.txt:13 (include)
|
|
|
+This warning is for project developers. Use -Wno-dev to suppress it.
|
|
|
+
|
|
|
+-- Found RE2 via pkg-config.
|
|
|
+-- Found MariaDB client library
|
|
|
+-- Found PostgreSQL client library (libpq)
|
|
|
+-- Configuring done (1.3s)
|
|
|
+-- Generating done (0.1s)
|
|
|
+-- Build files have been written to: /data/smartbotic/build
|
|
|
+[1/8] Building CXX object CMakeFiles/smartbotic_common.dir/lib/common/config_defaults.cpp.o
|
|
|
+[2/8] Linking CXX static library libsmartbotic_common.a
|
|
|
+[3/8] Building CXX object CMakeFiles/smartbotic-runner.dir/src/runner/workflow_engine.cpp.o
|
|
|
+[4/8] Building CXX object CMakeFiles/smartbotic-webserver.dir/src/webserver/api/workflow_controller.cpp.o
|
|
|
+/data/smartbotic/src/webserver/api/workflow_controller.cpp: In member function 'void smartbotic::webserver::api::WorkflowController::executeWorkflow(const httplib::Request&, httplib::Response&, const smartbotic::webserver::auth::AuthContext&)':
|
|
|
+/data/smartbotic/src/webserver/api/workflow_controller.cpp:317:11: warning: unused variable 'workflow' [-Wunused-variable]
|
|
|
+ 317 | auto& workflow = workflow_result.value();
|
|
|
+ | ^~~~~~~~
|
|
|
+[5/8] Building CXX object CMakeFiles/smartbotic-webserver.dir/src/webserver/webserver_service.cpp.o
|
|
|
+[6/8] Linking CXX executable smartbotic-webserver
|
|
|
+lto-wrapper: warning: using serial compilation of 33 LTRANS jobs
|
|
|
+lto-wrapper: note: see the 'flto' option documentation for more information
|
|
|
+[7/8] Linking CXX executable smartbotic-runner
|
|
|
+lto-wrapper: warning: using serial compilation of 44 LTRANS jobs
|
|
|
+lto-wrapper: note: see the 'flto' option documentation for more information
|
|
|
+```
|
|
|
+
|
|
|
+That single warning (`executeWorkflow`, unused `workflow` variable) is
|
|
|
+pre-existing and in code this change did not touch - confirmed by reading
|
|
|
+`executeWorkflow` before making any edits; it is untouched by this diff.
|
|
|
+No new warnings were introduced.
|
|
|
+
|
|
|
+## Scheduled-workflow count: before and after
|
|
|
+
|
|
|
+Both services were already running from a previous session, launched
|
|
|
+directly (not under systemd), logging to `/tmp/webserver.log` and
|
|
|
+`/tmp/runner.log`.
|
|
|
+
|
|
|
+Before-fix count, read from the existing log of the currently-running
|
|
|
+(pre-fix binary) webserver process:
|
|
|
+
|
|
|
+```
|
|
|
+$ grep -n "Loaded.*scheduled workflows" /tmp/webserver.log
|
|
|
+19:[2026-08-05 12:39:10.808] [webserver] [info] [7500] Loaded 1 scheduled workflows
|
|
|
+```
|
|
|
+
|
|
|
+Restart sequence (webserver first, then runner, each in its own bash call):
|
|
|
+
|
|
|
+```
|
|
|
+$ kill 7500 7571
|
|
|
+$ sleep 2; ps aux | grep -E "smartbotic-webserver|smartbotic-runner" | grep -v grep
|
|
|
+(no output - both terminated cleanly, no stale process needed a kill -9)
|
|
|
+$ ss -ltnp | grep -E "8090|9011|9012"
|
|
|
+(no output - ports free)
|
|
|
+$ rm -f /tmp/webserver.log && nohup ./build/smartbotic-webserver > /tmp/webserver.log 2>&1 &
|
|
|
+$ sleep 3; tail -30 /tmp/webserver.log
|
|
|
+```
|
|
|
+
|
|
|
+After-fix output (full relevant excerpt from the new webserver log):
|
|
|
+
|
|
|
+```
|
|
|
+[2026-08-05 13:41:13.573] [webserver] [info] [18313] Loading scheduled workflows from database...
|
|
|
+[2026-08-05 13:41:13.596] [webserver] [info] [18313] Scheduled workflow '35photo2anime' (wf_11784226-b91e-4e3e-8272-de6e314054d6) with schedule-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+[2026-08-05 13:41:13.604] [webserver] [info] [18313] Scheduled workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) with imap-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+[2026-08-05 13:41:13.631] [webserver] [info] [18313] Loaded 2 scheduled workflows
|
|
|
+```
|
|
|
+
|
|
|
+**Before: 1. After: 2.** The second workflow registered is exactly
|
|
|
+`wf_520f6f05-3256-4419-a8a5-42d7e7c3830f` ("Email OCR - Attachment to Text
|
|
|
+Reply"), the imap-trigger workflow whose stored config has no
|
|
|
+`pollInterval` key. It was not touched or re-saved; the fix made the
|
|
|
+already-stored, incomplete config register correctly on load.
|
|
|
+
|
|
|
+Runner restarted after confirming port 9011 was free:
|
|
|
+
|
|
|
+```
|
|
|
+$ ss -ltnp | grep 9011
|
|
|
+(no output - free)
|
|
|
+$ rm -f /tmp/runner.log && nohup ./build/smartbotic-runner > /tmp/runner.log 2>&1 &
|
|
|
+$ sleep 3; tail -40 /tmp/runner.log
|
|
|
+...
|
|
|
+[2026-08-05 13:41:22.216] [runner] [info] [18386] Loaded 56 node definitions from webserver
|
|
|
+[2026-08-05 13:41:22.217] [runner] [info] [18386] Node registry sync started with localhost:9012
|
|
|
+[2026-08-05 13:41:22.218] [runner] [info] [18386] Runner gRPC server listening on port 9011
|
|
|
+[2026-08-05 13:41:22.224] [runner] [info] [18386] Runner registered with webserver
|
|
|
+[2026-08-05 13:41:22.224] [runner] [info] [18386] Runner service runner-1 started
|
|
|
+```
|
|
|
+
|
|
|
+## Tests
|
|
|
+
|
|
|
+Two new fixtures added under `tests/nodes/`, both using the existing
|
|
|
+`code` node (which has a non-empty string `default` for its required
|
|
|
+`code` config property, and no defensive `||` fallback around it - so it
|
|
|
+directly exercises the runner's read-time defaulting):
|
|
|
+
|
|
|
+- `tests/nodes/config-defaults-fill.json` - a `code` node with `config: {}`
|
|
|
+ (no `code` key at all). Expects `status: completed` and
|
|
|
+ `output.result.processed === true`, which only happens if the runner
|
|
|
+ filled in the schema's default code text before execution. Without the
|
|
|
+ fix, `code.js`'s `if (!code ...) throw new Error('No code provided')`
|
|
|
+ would fail this node instead.
|
|
|
+- `tests/nodes/config-defaults-preserve-falsy.json` - a `code` node with
|
|
|
+ `config: {"code": ""}` (explicit falsy value for a defaulted key).
|
|
|
+ Expects `status: failed` with `errorContains: "No code provided"` -
|
|
|
+ proving `applyConfigDefaults` left the explicit empty string alone
|
|
|
+ instead of overwriting it with the non-empty default, which is exactly
|
|
|
+ the truthiness bug the design explicitly warned against.
|
|
|
+
|
|
|
+Individual runs:
|
|
|
+
|
|
|
+```
|
|
|
+$ python3 scripts/verify-node.py tests/nodes/config-defaults-fill.json
|
|
|
+case: verify-config-defaults-fill execution: exec_4ea0c625-8e9c-4f57-a2b9-91513068b653 status: completed
|
|
|
+ n1 completed {...}
|
|
|
+ n2 completed {"executionTime": 0, "result": {"data": {...}, "processed": true, "timestamp": ...}}
|
|
|
+
|
|
|
+PASS
|
|
|
+
|
|
|
+$ python3 scripts/verify-node.py tests/nodes/config-defaults-preserve-falsy.json
|
|
|
+case: verify-config-defaults-preserve-falsy execution: exec_3bef0971-35e2-4ad4-a533-8d0e513cf4fe status: failed
|
|
|
+ n1 completed {...}
|
|
|
+ n2 failed null
|
|
|
+
|
|
|
+PASS
|
|
|
+```
|
|
|
+
|
|
|
+Full fixture suite (all files under `tests/nodes/*.json`, one
|
|
|
+`verify-node.py` invocation per file):
|
|
|
+
|
|
|
+```
|
|
|
+$ for f in tests/nodes/*.json; do python3 scripts/verify-node.py "$f"; done
|
|
|
+```
|
|
|
+
|
|
|
+Result: **39/39 passing** - the 37 pre-existing fixtures plus the 2 new
|
|
|
+ones above. Every pre-existing fixture still reports `PASS` with the same
|
|
|
+node statuses as before this change; no node's behaviour changed once
|
|
|
+defaults started being applied at read time and write time.
|
|
|
+
|
|
|
+## Nodes whose behaviour changed
|
|
|
+
|
|
|
+None observed. Every fixture that exercised a node with a `configSchema`
|
|
|
+default (`filter`, `set-fields`, `sort-limit-dedupe`, `datetime`,
|
|
|
+`switch`, `respond-to-webhook`, `wait-for-approval`, etc.) passed
|
|
|
+unchanged. Spot-checking the node source under `nodes/` turned up a
|
|
|
+recurring pattern: most nodes already defend themselves with
|
|
|
+`config.field || <fallback>` or explicit `=== undefined` checks that
|
|
|
+happen to match the schema's declared default, which is presumably why
|
|
|
+none of the 37 pre-existing fixtures moved. The `code` node is the
|
|
|
+exception - its required `code` property has no JS-side fallback at all,
|
|
|
+which is exactly why it was the clean way to prove the read-time path
|
|
|
+does something real.
|
|
|
+
|
|
|
+## Concerns / follow-ups
|
|
|
+
|
|
|
+- Not fixed here, out of scope per the task: `executeWorkflow()` in
|
|
|
+ `workflow_controller.cpp` does not currently apply config defaults
|
|
|
+ before dispatching a manual/API-triggered execution to a runner over
|
|
|
+ gRPC - only `loadScheduledWorkflows` and `updateScheduledTriggers` read
|
|
|
+ trigger config directly on the webserver side, and the runner itself
|
|
|
+ applies defaults for every node right before execution (including
|
|
|
+ triggers passed through `executeWorkflow`), so this path is covered
|
|
|
+ transitively through the runner, not duplicated on the webserver side.
|
|
|
+- `applyConfigDefaults` only fills top-level `configSchema.properties`
|
|
|
+ entries; nested object/array-item defaults are intentionally not
|
|
|
+ recursed into, documented in the header. If a future node relies on a
|
|
|
+ default nested inside an object-typed config property, it will need
|
|
|
+ either a schema restructure or a deliberate extension of this helper -
|
|
|
+ not a silent gap someone will trip over unknowingly.
|
|
|
+
|
|
|
+### Correction to the "negative fixture" claim above
|
|
|
+
|
|
|
+`tests/nodes/config-defaults-preserve-falsy.json` does not actually
|
|
|
+discriminate between the fixed and unfixed code. Against the unfixed
|
|
|
+`workflow_engine.cpp` (no `applyConfigDefaults` call at all),
|
|
|
+`config: {"code": ""}` still reaches `code.js`'s
|
|
|
+`if (!code ...) throw new Error('No code provided')` and fails the same
|
|
|
+way, because nothing was ever overwriting it to begin with - there was no
|
|
|
+defaulting code present to get the truthiness check wrong. The fixture
|
|
|
+proves the correct behaviour today and stands as a guard against a future
|
|
|
+truthiness regression in `applyConfigDefaults` (e.g. someone "simplifying"
|
|
|
+`!result.contains(key)` into a truthiness check), but it is not proof that
|
|
|
+the empty string survives some prior broken state, because no such broken
|
|
|
+state existed for this fixture to distinguish from. The positive fixture,
|
|
|
+`config-defaults-fill.json`, does discriminate correctly: it fails against
|
|
|
+the unfixed code and passes against the fixed code.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+# Fix round 1
|
|
|
+
|
|
|
+Commit: `de2b70b77d4480c56522e7cb9fa1d9af7d91f1fc`
|
|
|
+
|
|
|
+## Findings addressed
|
|
|
+
|
|
|
+**Finding 1 (blocking) - loop bodies got no defaults.**
|
|
|
+`executeLoopBody` (`src/runner/workflow_engine.cpp`, ~line 2050) is a
|
|
|
+separate re-implementation of the main node walk and called
|
|
|
+`evaluateExpressions(body_node->config, ...)` directly, with no defaults
|
|
|
+applied - so a `code` node with `config: {}` inside a loop body failed with
|
|
|
+"No code provided" on every iteration, while the identical node outside
|
|
|
+the loop worked. Fixed by applying `applyConfigDefaults` to
|
|
|
+`body_node->config` before `evaluateExpressions`, using
|
|
|
+`registry_.getNode(body_node->type)`, mirroring the main walk's handling
|
|
|
+exactly (a failed lookup falls through and `executeNode` reports "Node
|
|
|
+type not found" as before, since an empty `std::optional` is passed
|
|
|
+through).
|
|
|
+
|
|
|
+**Finding 2 (non-blocking) - the added lookup doubled `NodeDefinition`
|
|
|
+copies (which carry the full JS source).**
|
|
|
+Chose: **hoist the lookup and reuse it**, rather than adding a
|
|
|
+by-reference accessor to `NodeRegistry`. `executeNode` now takes an
|
|
|
+optional fifth parameter, `const std::optional<NodeDefinition>&
|
|
|
+prefetched_node_def = std::nullopt` (declared in
|
|
|
+`src/runner/workflow_engine.hpp`). Both of the two call sites - the main
|
|
|
+walk (~line 669) and `executeLoopBody` (~line 2057) - already look up the
|
|
|
+node definition to apply defaults, so they now pass that same
|
|
|
+`std::optional<NodeDefinition>` straight into `executeNode`, which uses it
|
|
|
+if present instead of calling `registry_.getNode()` again. This was the
|
|
|
+smaller change: `NodeRegistry`'s `getNode()` already returns by value and
|
|
|
+is used that way from several other call sites in this file (lines 420,
|
|
|
+435, 546, 1867), so adding a second accessor would have meant two ways to
|
|
|
+fetch the same data; reusing what the caller already fetched keeps a
|
|
|
+single lookup path and drops the added lookup back down to one per node
|
|
|
+execution (previously two - one at the defaulting call site, one inside
|
|
|
+`executeNode` - and would have been three per loop iteration without this
|
|
|
+change).
|
|
|
+
|
|
|
+**Finding 3 (non-blocking, documentation).**
|
|
|
+- `lib/common/config_defaults.hpp`: added a paragraph recording that
|
|
|
+ write-time materialisation is permanent - once a default is baked into a
|
|
|
+ stored config, a later schema default change will never reach that
|
|
|
+ workflow, because the read path correctly leaves a present key alone.
|
|
|
+- `docs/nodes.md`: added a paragraph under "Configuration Schema" warning
|
|
|
+ that a `default:` containing `{{ }}` is evaluated as an expression, since
|
|
|
+ defaults flow through `evaluateExpressions` like any stored config value.
|
|
|
+
|
|
|
+## Build
|
|
|
+
|
|
|
+Command and full real output:
|
|
|
+
|
|
|
+```
|
|
|
+$ cmake --build build -j$(nproc)
|
|
|
+[1/9] Building CXX object CMakeFiles/smartbotic_common.dir/lib/common/config_defaults.cpp.o
|
|
|
+[2/9] Linking CXX static library libsmartbotic_common.a
|
|
|
+[3/9] Building CXX object CMakeFiles/smartbotic-runner.dir/src/runner/main.cpp.o
|
|
|
+[4/9] Building CXX object CMakeFiles/smartbotic-runner.dir/src/runner/runner_service.cpp.o
|
|
|
+[5/9] Building CXX object CMakeFiles/smartbotic-runner.dir/src/runner/workflow_engine.cpp.o
|
|
|
+[6/9] Building CXX object CMakeFiles/smartbotic-webserver.dir/src/webserver/api/workflow_controller.cpp.o
|
|
|
+/data/smartbotic/src/webserver/api/workflow_controller.cpp: In member function 'void smartbotic::webserver::api::WorkflowController::executeWorkflow(const httplib::Request&, httplib::Response&, const smartbotic::webserver::auth::AuthContext&)':
|
|
|
+/data/smartbotic/src/webserver/api/workflow_controller.cpp:317:11: warning: unused variable 'workflow' [-Wunused-variable]
|
|
|
+ 317 | auto& workflow = workflow_result.value();
|
|
|
+ | ^~~~~~~~
|
|
|
+[7/9] Building CXX object CMakeFiles/smartbotic-webserver.dir/src/webserver/webserver_service.cpp.o
|
|
|
+[8/9] Linking CXX executable smartbotic-webserver
|
|
|
+lto-wrapper: warning: using serial compilation of 33 LTRANS jobs
|
|
|
+lto-wrapper: note: see the '-flto' option documentation for more information
|
|
|
+[9/9] Linking CXX executable smartbotic-runner
|
|
|
+lto-wrapper: warning: using serial compilation of 43 LTRANS jobs
|
|
|
+lto-wrapper: note: see the '-flto' option documentation for more information
|
|
|
+```
|
|
|
+
|
|
|
+Same single pre-existing warning as round 1 (unrelated `executeWorkflow`
|
|
|
+unused variable, code this change did not touch). No new warnings.
|
|
|
+
|
|
|
+## Restart
|
|
|
+
|
|
|
+```
|
|
|
+$ ps aux | grep -E "smartbotic-webserver|smartbotic-runner" | grep -v grep
|
|
|
+fszonta+ 18313 ... ./build/smartbotic-webserver
|
|
|
+fszonta+ 18386 ... ./build/smartbotic-runner
|
|
|
+$ kill 18313 18386
|
|
|
+$ sleep 2; ps aux | grep -E "smartbotic-webserver|smartbotic-runner" | grep -v grep
|
|
|
+fszonta+ 18386 ... ./build/smartbotic-runner
|
|
|
+```
|
|
|
+
|
|
|
+The runner survived the plain `kill` (webserver did not). Confirmed and
|
|
|
+force-killed by PID:
|
|
|
+
|
|
|
+```
|
|
|
+$ kill -9 18386
|
|
|
+$ sleep 1; ps aux | grep -E "smartbotic-webserver|smartbotic-runner" | grep -v grep
|
|
|
+$ ss -ltnp | grep -E "8090|9011|9012"
|
|
|
+(no output - both processes gone, all three ports free)
|
|
|
+```
|
|
|
+
|
|
|
+Webserver started first:
|
|
|
+
|
|
|
+```
|
|
|
+$ rm -f /tmp/webserver.log && nohup /data/smartbotic/build/smartbotic-webserver > /tmp/webserver.log 2>&1 &
|
|
|
+$ sleep 3; tail -30 /tmp/webserver.log
|
|
|
+...
|
|
|
+[2026-08-05 13:57:59.374] [webserver] [info] [23060] Loading scheduled workflows from database...
|
|
|
+[2026-08-05 13:57:59.403] [webserver] [info] [23060] Scheduled workflow '35photo2anime' (wf_11784226-b91e-4e3e-8272-de6e314054d6) with schedule-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+[2026-08-05 13:57:59.411] [webserver] [info] [23060] Scheduled workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) with imap-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+[2026-08-05 13:57:59.440] [webserver] [info] [23060] Loaded 2 scheduled workflows
|
|
|
+...
|
|
|
+[2026-08-05 13:57:59.451] [webserver] [info] [23060] WebServer service started on port 8090
|
|
|
+```
|
|
|
+
|
|
|
+Port 9011 confirmed free before starting the runner:
|
|
|
+
|
|
|
+```
|
|
|
+$ ss -ltnp | grep 9011
|
|
|
+(no output)
|
|
|
+$ rm -f /tmp/runner.log && nohup /data/smartbotic/build/smartbotic-runner > /tmp/runner.log 2>&1 &
|
|
|
+$ sleep 3; tail -30 /tmp/runner.log
|
|
|
+[2026-08-05 13:58:08.404] [runner] [info] [23126] SmartBotic Runner starting...
|
|
|
+...
|
|
|
+[2026-08-05 13:58:08.487] [runner] [info] [23126] Loaded 56 node definitions from webserver
|
|
|
+...
|
|
|
+[2026-08-05 13:58:08.489] [runner] [info] [23126] Runner gRPC server listening on port 9011
|
|
|
+[2026-08-05 13:58:08.495] [runner] [info] [23126] Runner registered with webserver
|
|
|
+[2026-08-05 13:58:08.495] [runner] [info] [23126] Runner service runner-1 started
|
|
|
+```
|
|
|
+
|
|
|
+## Tests
|
|
|
+
|
|
|
+New fixture: `tests/nodes/config-defaults-loop-body.json` - a `code` node
|
|
|
+with an empty config placed as the sole body node of a `loop` iterating
|
|
|
+over `[1, 2]`. Its individual run:
|
|
|
+
|
|
|
+```
|
|
|
+$ python3 scripts/verify-node.py tests/nodes/config-defaults-loop-body.json
|
|
|
+case: verify-config-defaults-loop-body execution: exec_05da8c05-ce25-4350-abd1-79c7efd9a7f8 status: completed
|
|
|
+ body completed {"executionTime": 1, "result": {"data": 1, "processed": true, "timestamp": 1785931104318}}
|
|
|
+ items completed {"executionTime": 0, "result": {"items": [1, 2]}}
|
|
|
+ loop completed {"_activeBranch": "done", "_continueOnError": true, "_indexVariable": "index", "_isLoop": true, "_itemVariable": "item",
|
|
|
+ n1 completed {"executionId": "exec_05da8c05-ce25-4350-abd1-79c7efd9a7f8", "timestamp": 1785931104304, "triggeredBy": "manual"}
|
|
|
+
|
|
|
+PASS
|
|
|
+```
|
|
|
+
|
|
|
+Full fixture suite, one `verify-node.py` invocation per file under
|
|
|
+`tests/nodes/*.json`:
|
|
|
+
|
|
|
+```
|
|
|
+$ for f in tests/nodes/*.json; do
|
|
|
+ if python3 scripts/verify-node.py "$f" > /tmp/verify_out_$(basename "$f").txt 2>&1; then
|
|
|
+ pass=$((pass+1))
|
|
|
+ else
|
|
|
+ fail=$((fail+1)); failed_list="$failed_list $f"
|
|
|
+ fi
|
|
|
+ done
|
|
|
+ echo "PASS=$pass FAIL=$fail"
|
|
|
+PASS=40 FAIL=0
|
|
|
+FAILED:
|
|
|
+```
|
|
|
+
|
|
|
+**40/40 passing** - the 39 from round 1 plus this loop-body fixture.
|
|
|
+
|
|
|
+## Scheduled-workflow count: re-confirmed after restart
|
|
|
+
|
|
|
+```
|
|
|
+$ grep -n "Loaded.*scheduled workflows\|wf_520f6f05" /tmp/webserver.log
|
|
|
+19:[2026-08-05 13:57:59.411] [webserver] [info] [23060] Scheduled workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) with imap-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+20:[2026-08-05 13:57:59.440] [webserver] [info] [23060] Loaded 2 scheduled workflows
|
|
|
+```
|
|
|
+
|
|
|
+Still 2, `wf_520f6f05` still registered, still untouched.
|
|
|
+
|
|
|
+## Commit
|
|
|
+
|
|
|
+```
|
|
|
+$ git add -- docs/nodes.md lib/common/config_defaults.hpp src/runner/workflow_engine.cpp src/runner/workflow_engine.hpp tests/nodes/config-defaults-loop-body.json
|
|
|
+$ git commit -m "fix: apply config defaults inside loop bodies too, avoid extra source copies" ...
|
|
|
+[config-defaults de2b70b] fix: apply config defaults inside loop bodies too, avoid extra source copies
|
|
|
+ 5 files changed, 71 insertions(+), 8 deletions(-)
|
|
|
+ create mode 100644 tests/nodes/config-defaults-loop-body.json
|
|
|
+$ git log -1 --format="%H %G?"
|
|
|
+de2b70b77d4480c56522e7cb9fa1d9af7d91f1fc G
|
|
|
+```
|
|
|
+
|
|
|
+Signed (`G`), no pinentry issue.
|
|
|
+
|
|
|
+## Concerns
|
|
|
+
|
|
|
+- Named ports, `_webhookResponse`, `_pause` (twice), and now config
|
|
|
+ defaults have each had to be fixed twice - once in the main walk, once
|
|
|
+ in `executeLoopBody` - because the two are separate implementations.
|
|
|
+ Collapsing them is recorded as follow-up work and was explicitly not
|
|
|
+ this task's scope, but it remains the structural fix that would stop
|
|
|
+ this class of miss from recurring a sixth time.
|
|
|
+- The Finding 2 fix only threads the prefetched definition through the two
|
|
|
+ existing `executeNode` call sites. If a third call site is ever added
|
|
|
+ without also being told about `prefetched_node_def`, it will silently
|
|
|
+ fall back to `executeNode`'s own lookup - correct, just not optimal -
|
|
|
+ rather than fail loudly, so it is worth a second pair of eyes if
|
|
|
+ `executeNode` grows a new caller.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+# Fix round 2 (final) - the TTL decision
|
|
|
+
|
|
|
+Commit: `ccdc442fc19fe235b8a1a4a46e985ed31f898c4c`
|
|
|
+
|
|
|
+## Background
|
|
|
+
|
|
|
+`nodes/core/http-request.js` read `config.downloadTtlHours || 0` and
|
|
|
+`nodes/imap/imap-extract-attachments.js` read `config.storageTtlHours || 0`,
|
|
|
+while both schemas declare `default: 24`. Before this branch, a workflow
|
|
|
+whose config omitted the key stored downloads/attachments forever - the
|
|
|
+`||` fallback won and the schema default never arrived. With defaults now
|
|
|
+applied by this branch, those same already-saved workflows get 24 and the
|
|
|
+data starts expiring after a day. The project owner decided to KEEP the
|
|
|
+24-hour expiry: the schema is the intended behaviour, and unbounded
|
|
|
+storage growth is what the default was written to prevent in the first
|
|
|
+place. This round makes the code agree with that decision.
|
|
|
+
|
|
|
+## Changes
|
|
|
+
|
|
|
+- `nodes/core/http-request.js` line ~338:
|
|
|
+ `config.downloadTtlHours || 0` -> `config.downloadTtlHours ?? 24`.
|
|
|
+- `nodes/imap/imap-extract-attachments.js` line ~442:
|
|
|
+ `config.storageTtlHours || 0` -> `config.storageTtlHours ?? 24`.
|
|
|
+- Nullish coalescing, not `||`, so an explicit `0` - documented in both
|
|
|
+ schemas as "never expire" - still survives instead of being treated as
|
|
|
+ falsy and overridden.
|
|
|
+- Checked both files for other reads of the same config key:
|
|
|
+ `grep -n "downloadTtlHours\|storageTtlHours"` against each file showed
|
|
|
+ exactly one schema declaration and one read site per file. Nothing else
|
|
|
+ to make consistent.
|
|
|
+- Field descriptions updated in both schemas to state the default and the
|
|
|
+ 0-means-never behaviour plainly:
|
|
|
+ - `http-request.js` `downloadTtlHours.description`: "Auto-delete stored
|
|
|
+ file after this many hours. Default is 24 hours; set to 0 to keep it
|
|
|
+ forever."
|
|
|
+ - `imap-extract-attachments.js` `storageTtlHours.description`:
|
|
|
+ "Auto-delete stored files after this many hours. Default is 24 hours;
|
|
|
+ set to 0 to keep them forever."
|
|
|
+- `docs/nodes.md`, under "Storage (Database)", gained a paragraph stating
|
|
|
+ that `http-request` (Store Download) and `imap-extract-attachments`
|
|
|
+ (Store in Database) both default their TTL to 24 hours, so stored
|
|
|
+ downloads and extracted attachments now expire a day after they're
|
|
|
+ stored unless the workflow explicitly sets the TTL field to `0`, and
|
|
|
+ that any workflow relying on permanent storage under an unset TTL field
|
|
|
+ needs that `0` set explicitly or it will start losing data a day later.
|
|
|
+
|
|
|
+## Whether the explicit-zero fixtures were possible, and what they actually prove
|
|
|
+
|
|
|
+Both fixtures were possible and were written - `http-request`'s
|
|
|
+`storeDownload` path was exercised against a real local HTTP endpoint
|
|
|
+(`http://localhost:8090/index.html`, served by the already-running
|
|
|
+webserver's static file mount), and `imap-extract-attachments`'s
|
|
|
+`storeInDatabase` path was exercised by feeding a hand-built raw MIME
|
|
|
+multipart email through `emailSource`, with no live IMAP mailbox needed
|
|
|
+since that node accepts raw email text as config/input.
|
|
|
+
|
|
|
+What they can and cannot prove was checked directly, not assumed. Before
|
|
|
+writing them, I probed whether `smartbotic.storage.insert(..., ttlMs)`'s
|
|
|
+TTL is readable back through the JS API available to nodes:
|
|
|
+
|
|
|
+```
|
|
|
+$ python3 -c "... insert with ttlMs=5000, then storage.get() on the same id ..."
|
|
|
+```
|
|
|
+
|
|
|
+Full real output of the returned document:
|
|
|
+
|
|
|
+```json
|
|
|
+{
|
|
|
+ "collection": "ttl_probe_test",
|
|
|
+ "document": {
|
|
|
+ "_created_at": 1785932269775728600,
|
|
|
+ "_created_by": "",
|
|
|
+ "_id": "019fd1dbc8cfcd70ef3a1be5b435",
|
|
|
+ "_updated_at": 1785932269775728600,
|
|
|
+ "_updated_by": "",
|
|
|
+ "_version": 1,
|
|
|
+ "probe": true
|
|
|
+ },
|
|
|
+ "found": true,
|
|
|
+ "id": "019fd1dbc8cfcd70ef3a1be5b435"
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+No TTL or expiry field is present. `lib/storage/storage_client.hpp` /
|
|
|
+`.cpp` and the QuickJS binding in
|
|
|
+`src/runner/engine/script_engine.cpp` (`storage.insert`, `storage.get`,
|
|
|
+`storage.query`) confirm there is no accessor that returns a document's
|
|
|
+TTL or expiry timestamp back to a node - `insert()` takes `ttl_ms` and
|
|
|
+converts it to seconds for the upstream client, one-way. So:
|
|
|
+
|
|
|
+- The two fixtures **do** prove that `smartbotic.storage.insert` is
|
|
|
+ reached and completes successfully (returns `success: true`, the node
|
|
|
+ returns a `storage: {collection, id}` object) when the TTL field is
|
|
|
+ explicitly `0` - i.e. the storage path is genuinely exercised, not
|
|
|
+ skipped or thrown on.
|
|
|
+- The two fixtures **cannot** prove that the TTL value that reached
|
|
|
+ `storage.insert` was actually `0` rather than the pre-`??`-fix `24`
|
|
|
+ (in hours) or any other value - there is no way to read that back
|
|
|
+ through the harness, and waiting out a real 24-hour expiry to observe
|
|
|
+ the difference empirically is not practical for this suite. That part
|
|
|
+ of the guarantee rests on the `?? 24` change being correct at the two
|
|
|
+ read sites, which is a one-line, directly-readable diff in each file
|
|
|
+ (confirmed there is exactly one read site per file, above) rather than
|
|
|
+ something a black-box fixture can independently verify.
|
|
|
+
|
|
|
+Fixture files:
|
|
|
+- `tests/nodes/config-defaults-ttl-zero-http-request.json`
|
|
|
+- `tests/nodes/config-defaults-ttl-zero-imap-extract-attachments.json`
|
|
|
+
|
|
|
+Individual runs, full real output:
|
|
|
+
|
|
|
+```
|
|
|
+$ python3 scripts/verify-node.py tests/nodes/config-defaults-ttl-zero-http-request.json
|
|
|
+case: verify-config-defaults-ttl-zero-http-request execution: exec_1fa17e8a-127f-4dea-ac3e-4608cb1a7eb0 status: completed
|
|
|
+ dl completed {"body": null, "checksum": "7f09ea737005b3abf0fa01db11929491a119ac59ed433d2635c980c0c9473caf", "file": {"checksum": "7f0
|
|
|
+ n1 completed {"executionId": "exec_1fa17e8a-127f-4dea-ac3e-4608cb1a7eb0", "timestamp": 1785932459567, "triggeredBy": "manual"}
|
|
|
+
|
|
|
+PASS
|
|
|
+
|
|
|
+$ python3 scripts/verify-node.py tests/nodes/config-defaults-ttl-zero-imap-extract-attachments.json
|
|
|
+case: verify-config-defaults-ttl-zero-imap-extract-attachments execution: exec_7f7f2770-dc43-4797-ab27-50581569fb68 status: completed
|
|
|
+ extract completed {"attachments": [{"contentId": null, "deduplicated": false, "filePath": "./data/attachments/2026-08/66f6263d-78ce-4e30-a
|
|
|
+ mail completed {"executionTime": 1, "result": {"raw": "Content-Type: multipart/mixed; boundary=\"BOUNDARY123\"\n\n--BOUNDARY123\nConten
|
|
|
+ n1 completed {"executionId": "exec_7f7f2770-dc43-4797-ab27-50581569fb68", "timestamp": 1785932461151, "triggeredBy": "manual"}
|
|
|
+
|
|
|
+PASS
|
|
|
+```
|
|
|
+
|
|
|
+## Full fixture suite
|
|
|
+
|
|
|
+```
|
|
|
+$ for f in tests/nodes/*.json; do
|
|
|
+ if python3 scripts/verify-node.py "$f" > /tmp/verify_out_$(basename "$f").txt 2>&1; then
|
|
|
+ pass=$((pass+1))
|
|
|
+ else
|
|
|
+ fail=$((fail+1)); failed_list="$failed_list $f"
|
|
|
+ fi
|
|
|
+ done
|
|
|
+ echo "PASS=$pass FAIL=$fail"
|
|
|
+PASS=42 FAIL=0
|
|
|
+FAILED:
|
|
|
+```
|
|
|
+
|
|
|
+**42/42 passing** - the 40 from round 1 plus these 2 TTL fixtures.
|
|
|
+
|
|
|
+## Scheduled-workflow count: re-confirmed
|
|
|
+
|
|
|
+No rebuild or restart was performed or needed for this round - both
|
|
|
+changed files are hot-reloaded JavaScript node source, not C++. The
|
|
|
+webserver and runner from round 1's restart were still running.
|
|
|
+
|
|
|
+```
|
|
|
+$ grep -n "Loaded.*scheduled workflows\|wf_520f6f05" /tmp/webserver.log
|
|
|
+19:[2026-08-05 13:57:59.411] [webserver] [info] [23060] Scheduled workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) with imap-trigger trigger, interval: 5 minutes, overlap: skip, maxConcurrent: 1
|
|
|
+20:[2026-08-05 13:57:59.440] [webserver] [info] [23060] Loaded 2 scheduled workflows
|
|
|
+594:[2026-08-05 14:03:39.401] [webserver] [info] [23089] Scheduler executing workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) - imap-trigger trigger
|
|
|
+598:[2026-08-05 14:03:40.226] [webserver] [info] [23089] Scheduled workflow wf_520f6f05-3256-4419-a8a5-42d7e7c3830f execution started: exec_606211f9-fba8-4805-9adb-84bf2f565056 on runner runner-1
|
|
|
+759:[2026-08-05 14:09:10.255] [webserver] [info] [23089] Scheduler executing workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) - imap-trigger trigger
|
|
|
+779:[2026-08-05 14:09:10.997] [webserver] [info] [23089] Scheduled workflow wf_520f6f05-3256-4419-a8a5-42d7e7c3830f execution started: exec_58d3c9f8-705a-4384-8422-db4d5b047ed6 on runner runner-1
|
|
|
+925:[2026-08-05 14:14:40.625] [webserver] [info] [23089] Scheduler executing workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) - imap-trigger trigger
|
|
|
+929:[2026-08-05 14:14:41.395] [webserver] [info] [23089] Scheduled workflow wf_520f6f05-3256-4419-a8a5-42d7e7c3830f execution started: exec_4dc881fc-986e-40d2-863a-05820a4deb80 on runner runner-1
|
|
|
+1024:[2026-08-05 14:20:11.422] [webserver] [info] [23089] Scheduler executing workflow 'Email OCR - Attachment to Text Reply' (wf_520f6f05-3256-4419-a8a5-42d7e7c3830f) - imap-trigger trigger
|
|
|
+1027:[2026-08-05 14:20:12.165] [webserver] [info] [23089] Scheduled workflow wf_520f6f05-3256-4419-a8a5-42d7e7c3830f execution started: exec_01e9a248-93d8-4838-b1d2-fcd36693d7f8 on runner runner-1
|
|
|
+```
|
|
|
+
|
|
|
+Still 2, `wf_520f6f05` still registered and unmodified - and, beyond just
|
|
|
+being registered, its imap-trigger has been firing on schedule
|
|
|
+(4 scheduled executions logged between 13:58 and 14:20) throughout this
|
|
|
+whole review round, which is the round-1 fix holding up under real,
|
|
|
+continued operation rather than a one-time startup check.
|
|
|
+
|
|
|
+## Commit
|
|
|
+
|
|
|
+```
|
|
|
+$ git add -- docs/nodes.md nodes/core/http-request.js nodes/imap/imap-extract-attachments.js tests/nodes/config-defaults-ttl-zero-http-request.json tests/nodes/config-defaults-ttl-zero-imap-extract-attachments.json
|
|
|
+$ git commit -m "fix: make http-request and imap-extract-attachments TTL fallbacks agree with their schema defaults" ...
|
|
|
+[config-defaults ccdc442] fix: make http-request and imap-extract-attachments TTL fallbacks agree with their schema defaults
|
|
|
+ 5 files changed, 56 insertions(+), 4 deletions(-)
|
|
|
+ create mode 100644 tests/nodes/config-defaults-ttl-zero-http-request.json
|
|
|
+ create mode 100644 tests/nodes/config-defaults-ttl-zero-imap-extract-attachments.json
|
|
|
+$ git log -1 --format="%H %G?"
|
|
|
+ccdc442fc19fe235b8a1a4a46e985ed31f898c4c G
|
|
|
+```
|
|
|
+
|
|
|
+Signed (`G`), no pinentry issue.
|
|
|
+
|
|
|
+## Which nodes now behave differently from before this branch, and how
|
|
|
+
|
|
|
+This is the accumulated, user-visible behaviour change across all three
|
|
|
+rounds on this branch, for anyone reading this report to understand
|
|
|
+impact:
|
|
|
+
|
|
|
+- **Every node with a `configSchema` default**, across both the runner
|
|
|
+ (including inside loop bodies) and the webserver scheduler, now
|
|
|
+ receives that default when its stored config omits the key - on
|
|
|
+ already-saved workflows, not just newly-saved ones. Spot-checked
|
|
|
+ against the 37 pre-existing fixtures plus manual review of node
|
|
|
+ source: no other node's fixture-observable behaviour changed, because
|
|
|
+ most nodes already defended themselves with a JS-level fallback that
|
|
|
+ happened to match the schema default. `nodes/core/code.js` is the one
|
|
|
+ node in the fixture suite with no such defensive fallback for its
|
|
|
+ required `code` field, which is why it was used to prove the mechanism
|
|
|
+ works at all (`config-defaults-fill.json`,
|
|
|
+ `config-defaults-preserve-falsy.json`,
|
|
|
+ `config-defaults-loop-body.json`).
|
|
|
+- **`nodes/core/http-request.js`** (Store Download) and
|
|
|
+ **`nodes/imap/imap-extract-attachments.js`** (Store in Database) are
|
|
|
+ the two nodes whose behaviour has materially and deliberately changed
|
|
|
+ for real, already-running workflows. Previously `|| 0` meant a config
|
|
|
+ that omitted `downloadTtlHours` / `storageTtlHours` stored data forever
|
|
|
+ (the schema's `default: 24` never reached the node). Now, with defaults
|
|
|
+ applied, an omitted key resolves to `24` and data stored by these two
|
|
|
+ nodes is auto-deleted 24 hours after creation, unless the workflow's
|
|
|
+ config explicitly sets the TTL field to `0`. This is an intentional,
|
|
|
+ owner-approved change in behaviour, not a bug - the report warns of it
|
|
|
+ here and in `docs/nodes.md` because it is the one change on this branch
|
|
|
+ that can cause silent data loss for an existing workflow that nobody
|
|
|
+ told to expect it.
|
|
|
+- **`wf_520f6f05-3256-4419-a8a5-42d7e7c3830f`** is the confirmed
|
|
|
+ real-world example of the positive side of this same mechanism: its
|
|
|
+ imap-trigger's missing `pollInterval` now resolves to the schema's
|
|
|
+ `default: 5`, and the workflow is scheduled and firing again after
|
|
|
+ being silently dead since it was saved.
|
|
|
+
|
|
|
+## Concerns
|
|
|
+
|
|
|
+- The data-loss risk flagged above is real and immediate: any production
|
|
|
+ workflow using `http-request` with Store Download enabled, or
|
|
|
+ `imap-extract-attachments` with Store in Database enabled, and no
|
|
|
+ explicit TTL value in its saved config, will start deleting that stored
|
|
|
+ data 24 hours after each item is stored, starting from whenever this
|
|
|
+ branch reaches production. `docs/nodes.md` and both field descriptions
|
|
|
+ now say so, but nothing in the system will proactively surface this to
|
|
|
+ an existing workflow's owner - it is documentation, not a migration or
|
|
|
+ a warning banner. Whether some active-workflow scan or one-time
|
|
|
+ notification is warranted is a product decision beyond this branch's
|
|
|
+ scope, but worth raising explicitly since the owner's decision was to
|
|
|
+ accept the new expiry rather than the old unbounded growth.
|
|
|
+- The two new TTL fixtures are deliberately scoped to what the harness
|
|
|
+ can prove (the storage call succeeds with an explicit 0) and explicitly
|
|
|
+ cannot prove the numeric TTL value used. If `storage.insert`'s TTL ever
|
|
|
+ becomes introspectable through the JS API, these fixtures should be
|
|
|
+ strengthened to assert on the actual value rather than just successful
|
|
|
+ completion.
|