v2: sub-workflow node + parallel waves (batch fan-out phase 1) - #213
v2: sub-workflow node + parallel waves (batch fan-out phase 1)#213xBalbinus wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
The PR is well-structured and the implementation is largely correct, but there are two real correctness issues and one security-relevant gap worth flagging.
-
executeWavehas a race on thenextcounter — the pool pattern is not safe when workers are async. Two workers can read the sameidxbefore either increments it becausenext++is not atomic acrossawaitboundaries inside the same microtask loop. In practice Node.js is single-threaded so the read-and-increment of a plain JSletis synchronous and cannot interleave, but thewhile(true) { const idx = next++; ... await ... }shape looks racy and is worth a comment; more importantly, if a worker throws after incrementingnextbut before writingoutcomes[idx], that slot staysundefinedand therejectedcheck rethrows correctly — but the caller then gets a sparse array with at least one unset element at indexidx. Thethrow rejected.reasonpath is reached, so the drive loop does abort, but any partially-writtenoutcomesentries could leak if the caller ever inspected them. This is a latent correctness hole ifexecuteWave's contract is ever relaxed. -
settleFromChildskipsputIntentbeforecompleteCheckpointon the failure path — every other failure path in this file (thefail()helper, the depth guard, etc.) callsputIntentfirst to make the write idempotent. The failure branch insidesettleFromChild(lines ~150-165 ofworkflow-call.ts) callscompleteCheckpointdirectly without a precedingputIntent. On re-entry after a crash betweencompleteCheckpointand the return, the executor will re-run the wholesettleFromChildpath and callcompleteCheckpointagain. WhethercompleteCheckpointis idempotent on a second call with the same key is store-defined — if the in-memory store is idempotent it works, but this violates the intent→complete ordering the rest of the file enforces, and a stricter store implementation would reject it.packages/workflow/src/nodes/workflow-call.tsaround line 152 (thechild.outcome !== 'completed'branch) — addawait store.putIntent(...)beforestore.completeCheckpoint(...)to match the success path and thefail()helper.
-
computeChildOutputonly looks atiteration === 0checkpoints —byNodeis built withif (cp.iteration === 0). If the child workflow contains aforeachnode, its body checkpoints use higher iterations and the foreach's own completion checkpoint is at iteration 0 butcp.resultreflects the aggregate. That's probably fine. However, the stop-node output branch looks forresult.outputinside the checkpoint result object; whether the stop executor stores output under that key depends on convention not visible in this diff. If the stop executor stores the output differently,computeChildOutputsilently falls through to the leaf-node map. This is worth a test that actually asserts the stop-output path, not just the leaf-output path — the existing test (resumes with the child stop output) does cover this, but only for the in-memory store's exact shape. -
wakeParentRundoes an unconditionalgetRunon every settle —packages/workflow/src/interpreter.ts,wakeParentRun. This adds a store round-trip to everytwoPhaseSettleandresumeTerminalizecall, including for top-level runs that have no parent (the common case). The check isrun?.params.parentRunId !== undefined, which is fast once the row is fetched, butgetRunis called on a run that was just settled and is still in memory/cache. Low severity, but given this is on the hot path of every run settlement it is worth noting — passingparentRunIdthrough from the already-loadedWorkflowRunwould eliminate the extra fetch.
Created on behalf of Xiangan He xiangan@turnkey.io
|
Checked all four against the code. No change made — reasoning below, happy to be argued out of any of it. 1. The sparse-array half can't reach a caller either. A slot is only left unwritten when its executor rejects, and 2. Idempotency is also contract-defined rather than store-defined — 3. 4. |
Summary
Phase 1 of the batch fan-out design (spec included as the first commit): the dag/v1 interpreter cannot express "a small DAG per item" — a foreach body is one inline node, and even sibling tool nodes execute sequentially. This PR adds the two pieces that make the per-customer batch shape expressible.
Sub-workflow node. A new
workflownode type ({ workflowId, input }) starts a child run of the referenced definition and parks until it settles, valid at top level and as a foreach body. Each batch item becomes a real run: per-node status, reruns, and the runs list double as the batch tracker. The child recordsparentRunId/parentNodeId/parentIterationin itsRunParams(jsonb — no migration). Dispatch is replay-safe: the child run id derives from the checkpoint PK (sha256(runId:nodeId:iteration)), the intent checkpoint persists it before creation, andcreateRunis insert-if-absent. Guards: exact-owner match on the reference (team sharing waits for RBAC Phase B), nesting depth 1 enforced at dispatch and against the resolved definition, loud failure when the host wires noresolveWorkflow. A completed child's output is its stop node's declaredoutput, else a map of leaf-node results. Cancel propagates to parked children as the same durablecancelsignalterminate()writes; settle-time wake of the parent lives in the interpreter, with the host sweep as the lost-wake backstop (new{ kind: 'run' }wait condition).Parallel waves. The interpreter executes a wave's runnable nodes concurrently (bounded, 5) instead of a sequential
for...of await. Same-wave nodes never feed each other — the template context and runnable set are computed before the wave, and each executor writes only its own checkpoint row — so this changes wall-clock, not semantics. Outcomes still aggregate in definition order.Engine port change is minimal: one optional
resolveWorkflowmethod, implemented api-side overworkflow_definitions. Starting the child needs no engine call — a run starts by existing in the store with a requested wake, which is whatLocalRunHost.startdoes.Testing
packages/workflow/src/nodes/workflow-call.test.ts(13): drive-level lifecycle (start/park/settle/output/failure), replay convergence on the derived child id, depth + unresolved + unwired guards, cancel propagation, foreach fan-out over two child runs, a concurrency-overlap proof for parallel waves, and validator coverage.@valet/workflowsuite 272 passed (no regressions from the wave change); api workflow suites 150 passed; PG store conformance 59 passed; rootpnpm typecheckclean.e2e scorecard
18 passed, 3 red rows, all explained:
engine-unitis the documented Node-version trap (green under Node 22 — 538 passed);web-buildwas this change (the editor's exhaustive node-type maps require the new type) and is fixed in the second commit (web build green, 573 web tests pass);cliis a missingpackages/api/node_modules/.bin/tsxbin link present in the untouched checkout too — environmental, unrelated. Docker/API-key suites skipped (daemon and key absent in the shell).