feat(workflow): ParallelWorker and JoinNode (Part 4) - #591
Conversation
fc27f1f to
980ee86
Compare
980ee86 to
013670a
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Order is deterministic and that part is right: results[i] is keyed by item index rather than completion order, and the shared nextIndex claim can't double-issue an item, so the output list always matches the input list. The things I'd want resolved before this lands are cancellation (the worker loop never checks ctx.abortSignal, so a timeout or abort stops nothing), the unlimited default concurrency, retryConfig/timeout being applied to both the wrapper and the node it wraps, and a failing item discarding every sibling's completed output. Separately, JoinNode's barrier flag has no reader anywhere in the tree at this head, and waitForOutput is a second unread flag for the same idea. Static review only — I did not run anything.
013670a to
604fc8f
Compare
604fc8f to
4b1f0c9
Compare
4b1f0c9 to
ce9e51c
Compare
f58b976 to
d77e067
Compare
Part 4/9 of the feature/workflows split, stacked on the built-in nodes.
- nodes/parallel_worker: runs a wrapped node once per item of a list input,
order-preserving, bounded by maxParallelWorkers, cancelling on first error
(a non-list input is treated as a single-element list). Registers a factory
with the engine (registerParallelWorkerFactory) so
buildNode(..., {parallelWorker: true}) works without a static import.
- nodes/join_node: a fan-in barrier that requires all predecessors and emits
the aggregated predecessor outputs as its output.
Tests (9): ParallelWorker mapping/order, single-item + empty-list handling,
concurrency bounding, first-error propagation, the registry factory
(buildNode + parallelWorker / maxParallelWorkers guard), and JoinNode
passthrough — all driven directly against a NodeContext. The graph-level
parallel/fan-in integration tests land in Part 6 with the runner. Full core
suite green (2384 tests).
Follow the registry removal: PARALLEL_WORKER_FACTORY is set in node_builders.ts instead of parallel_worker.ts calling registerParallelWorkerFactory at import time. Update the engine-util test now that the factory is present (parallelWorker wraps in a ParallelWorker).
…ounds, retry ParallelWorker: - Don't apply retryConfig/timeout to the wrapper — they belong on the inner node (per item), so the two levels no longer compose. Dropped from ParallelWorkerConfig, the ParallelWorkerFactory options, and buildNode's factory call. - Bound default concurrency (DEFAULT_MAX_PARALLEL_WORKERS = 8) instead of unlimited; pass Infinity for unbounded. - Observe cancellation: the worker loop now stops claiming items when ctx.abortSignal or the invocation's abort signal fires (documented as stops-scheduling only — in-flight items still finish), and doesn't emit a partial list on abort. - Track failure with a dedicated `failed` flag so an item that rejects with `undefined` still fails instead of leaving a silent hole. - Give each child a distinct node path (overrideNodePath) so its events are attributable, not just a distinct branch/runId. - Doc: "stopping on first error" (nothing is cancelled), and state the all-or-nothing semantics explicitly. JoinNode: doc now says it emits its input unchanged (the engine supplies the predecessor-name -> output map); the barrier is enforced by the orchestrator via requiresAllPredecessors in a later part. Tests: pin the concurrency peak (toBe), add default-bound / undefined-reject / abort-stops-scheduling cases.
Replace the infinite worker loop with while(!failed && !isAborted()) so the termination conditions live in the loop header instead of an infinite loop with internal breaks.
967675b to
e6e00d6
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at e6e00d60. All seven findings from the last round are resolved, and the two I cared most about were resolved the right way rather than papered over.
Unbounded fan-out is fixed properly. DEFAULT_MAX_PARALLEL_WORKERS = 8 with the rationale stated at the constant (a data-driven list length can't burst into concurrent LLM/remote calls), Infinity as the explicit opt-out, and a constructor guard rejecting < 1. The default is now safe and the unsafe case is opt-in, which is the right direction.
Cancellation is handled honestly. isAborted() checks both ctx.abortSignal and the invocation-level signal, the claim loop re-checks each iteration, and the post-pool guard suppresses a partial results list that would otherwise have holes. Crucially the class doc states the residual limit out loud — items already in flight run to completion because ctx.runNode can't forward a signal into a child. I'd rather have a documented boundary than a fix that pretends to cancel.
All-or-nothing is now a stated contract, not an accident. The behaviour is unchanged — first error rethrown, sibling outputs discarded — but it's declared in the class doc with guidance to make items failure-tolerant if partial results matter. That was exactly the ask: decide deliberately and say so. The separate failed flag is a nice catch beyond what I raised — a bare Promise.reject() now counts as a failure instead of leaving a silent hole in results and resolving successfully.
Also resolved: retryConfig/timeout no longer double-apply (the wrapper carries neither, and the doc says so); each child now gets overrideNodePath so its events are attributable instead of all sharing the inner node's path.
Two I verified rather than took on trust:
requiresAllPredecessorsstill has no reader in this part, but the doc says the orchestrator consumes it in a later one — and it does: #592 hasif (targetNode.requiresAllPredecessors). Declaring the contract where the node type lives and consuming it where the scheduler lives is the right split for a stacked series, and the consumer genuinely exists.- The concurrency test no longer passes on a serial implementation. It tracks peak in-flight and asserts
toBe(2)andtoBe(8)exactly, with a comment noting a>=form would stay green at peak=1 if the pool regressed. That's the specific weakness I flagged, fixed at the assertion rather than by adding more cases around it.
No new type suppressions, no instanceof, no stray logging in the added lines. CI green on ubuntu/macOS/Windows — checked the individual job list, not the rollup. LGTM.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
Continuing the stacked split of the large
feature/workflowsbranch. With the engine core (Part 2) and the built-in Function/Tool nodes (Part 3) in place, the engine needs its concurrency primitives.Solution:
This is Part 4 of 9 — ParallelWorker and JoinNode — stacked on Part 3.
Stacked on: #part3_pr_number (Part 3 — Function/Tool nodes). Please merge Part 3 first.
Included:
nodes/parallel_worker.ts— runs a wrapped node once per item of a list input; order-preserving, bounded bymaxParallelWorkers, cancelling on the first error (a non-list input is treated as a single-element list). It registers a factory with the engine (registerParallelWorkerFactory) sobuildNode(..., {parallelWorker: true})works without a static import.nodes/join_node.ts— a fan-in barrier that requires all predecessors and emits the aggregated predecessor outputs as its output.Intentionally deferred: the graph-level parallel / fan-in integration tests (which use the runner) land in Part 6. Dynamic scheduling (Part 5), LLM-as-node (Part 7), and HITL (Part 8) follow.
Testing Plan
Unit Tests:
Bundled tests (9):
workflow/parallel_worker_test.ts— ParallelWorker mapping/order, single-item and empty-list handling, concurrency bounding (maxParallelWorkers), first-error propagation, the registry factory (buildNode+parallelWorker, and themaxParallelWorkers-without-parallelWorkerguard), and JoinNode passthrough — all driven directly against aNodeContext.Full core suite green (2384 tests). Typecheck clean:
npx tsc --noEmit -p core/tsconfig.json.Manual End-to-End (E2E) Tests:
N/A — node-level units; graph/runner E2E coverage lands in Part 6.
Checklist
Additional context
Stacked split — merge in order (…Part 3 → Part 4 → Part 5 → …). Diff: 3 files, +242.