Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions core/src/workflow/nodes/join_node.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {createEvent, Event} from '../../events/event.js';
import {BaseNode} from '../base_node.js';
import {NodeContext} from '../node_context.js';

/**
* A fan-in barrier node: it waits for ALL of its predecessors to complete, then
* emits the aggregated inputs (a map of predecessor name → output) as its
* output.
*
* Ported from `google/adk-python` `workflow/_join_node.py`.
*/
export class JoinNode extends BaseNode {
override get requiresAllPredecessors(): boolean {
return true;
}
Comment on lines +12 to +21

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit. Nothing reads requiresAllPredecessors, so this class is a barrier in name only — and there is a second flag for the same concept.

export class JoinNode extends BaseNode {
  override get requiresAllPredecessors(): boolean {
    return true;
  }

Grepping the tree at this head, requiresAllPredecessors appears exactly three times: the base getter (base_node.ts:110), this override, and parallel_worker_test.ts:93 asserting the getter returns true. There is no consumer anywhere. And waitForOutput — declared, assigned, never read at base_node.ts:41/71/95 — is documented as "the node only produces its output once all of its predecessors have triggered it (fan-in / join semantics)", which is the same sentence as this getter's doc. So there are two carriers for one concept with no stated precedence, and JoinNode sets one while leaving the other false: new JoinNode({name: 'j', waitForOutput: true}) is accepted today and means nothing. Please collapse them to one flag now, while there is no consumer to disagree with, and say in the PR body which part lands the reader.

Same file, lines 12-14: the doc says the node "emits the aggregated inputs (a map of predecessor name → output)", but runImpl yields input unchanged whatever its shape — the aggregation belongs to the engine and the engine side isn't here yet (the test hand-builds {a: 1, b: 2} and asserts it comes back). "Emits its input unchanged; the engine supplies the predecessor-name → output map" would describe the code that exists.

Since it came up on Part 1: I checked both carrier issues against this part and neither lands here — nothing in this diff reads Event.route/EventActions.route, and joinCompleted does not appear anywhere in the tree, so JoinNode is not writing it.


protected async *runImpl(
ctx: NodeContext,
input: unknown,
): AsyncGenerator<Event, void, void> {
yield createEvent({
author: this.name,
invocationId: ctx.invocationId,
branch: ctx.branch,
output: input,
});
}
}
113 changes: 113 additions & 0 deletions core/src/workflow/nodes/parallel_worker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {BaseNode} from '../base_node.js';
import {NodeContext} from '../node_context.js';
import {RetryConfig} from '../retry_config.js';
import {registerParallelWorkerFactory} from '../utils/workflow_graph_utils.js';

/** Options for a {@link ParallelWorker}. */
export interface ParallelWorkerConfig {
/** Maximum number of items processed concurrently. `undefined` = unlimited. */
maxParallelWorkers?: number;
Comment on lines +14 to +15

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit. The default is unlimited concurrency.

/** Maximum number of items processed concurrently. `undefined` = unlimited. */
maxParallelWorkers?: number;

Unset, poolSize at line 61 becomes items.length, so a node fanned out over a 500-element list issues 500 concurrent inner runs. When the inner node is an LLM or a remote tool that is a rate-limit and cost incident, and the list length is normally data-driven, not author-chosen — the person who gets burned is not the person who wrote the graph. A named default:

const DEFAULT_MAX_PARALLEL_WORKERS = 8;
...
const poolSize = Math.min(
  this.maxParallelWorkers ?? DEFAULT_MAX_PARALLEL_WORKERS,
  items.length,
);

Callers who really want unbounded can pass Infinity, which Math.min handles. If _parallel_worker.py is unbounded and you are matching it deliberately, please say so in the PR body — this is a case where diverging from the reference for a safe default seems worth it.

retryConfig?: RetryConfig;
timeout?: number;
}

/**
* A node that runs a wrapped node in parallel for each item of a list input,
* preserving order, bounded by `maxParallelWorkers`, cancelling on first error.
*
* Ported from `google/adk-python` `workflow/_parallel_worker.py`. A non-list
* input is treated as a single-element list. Each item runs via
* `ctx.runNode(inner, item, {useSubBranch: true})`; the node's output is the
* ordered list of the children's outputs.
*/
export class ParallelWorker extends BaseNode {
readonly maxParallelWorkers?: number;
private readonly inner: BaseNode;

constructor(inner: BaseNode, config: ParallelWorkerConfig = {}) {
super({
name: inner.name,
rerunOnResume: true,
retryConfig: config.retryConfig,
timeout: config.timeout,
});
Comment on lines +33 to +39

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit. The wrapper takes the same retryConfig and timeout as the node it wraps, so both levels apply them and the effects multiply.

super({
  name: inner.name,
  rerunOnResume: true,
  retryConfig: config.retryConfig,
  timeout: config.timeout,
});

buildNode hands one options bag to both sides: buildInnerNode(nodeLike, options) at workflow_graph_utils.ts:123 (which reaches new FunctionNode(name, handler, options) at function_node.ts:218, whose ctor does super({name, ...config})), and then the same retryConfig/timeout again to the factory at workflow_graph_utils.ts:135-139. Two consequences:

  • Retries compose. {maxAttempts: 3} gives up to 3 inner attempts per item and 3 outer attempts of the whole fan-out. Worse, results is rebuilt on every entry into runImpl, so an outer retry re-executes every item, including the ones that already succeeded. One flaky item in a 20-item LLM fan-out costs 20 more model calls per outer attempt.
  • The timeout means two different things. On the inner node it is a per-item budget; on the wrapper it is the budget for the entire fan-out. {maxParallelWorkers: 2, timeout: 30} over 10 items that each take ~20s can only ever hit the outer deadline.

Simplest fix: don't forward them to the wrapper — drop retryConfig/timeout from this super() call and from ParallelWorkerFactory's options, and leave them on the inner node. If the wrapper genuinely needs its own, they should be separate options so a caller can set the two independently. The buildNode half is Part 2 code, so either side can carry the fix. Nothing in parallel_worker_test.ts constructs a ParallelWorker with a retryConfig or a timeout, which is why this is invisible today.

if (
config.maxParallelWorkers !== undefined &&
config.maxParallelWorkers < 1
) {
throw new Error('maxParallelWorkers must be greater than or equal to 1.');
}
this.inner = inner;
this.maxParallelWorkers = config.maxParallelWorkers;
}

protected async *runImpl(
ctx: NodeContext,
input: unknown,
): AsyncGenerator<unknown, void, void> {
const items = Array.isArray(input) ? input : [input];
if (items.length === 0) {
yield [];
return;
}

const results = new Array<unknown>(items.length);
const poolSize = Math.min(
this.maxParallelWorkers ?? items.length,
items.length,
);

let nextIndex = 0;
let firstError: unknown;

const worker = async (): Promise<void> => {
for (;;) {
if (firstError !== undefined) {
return;
}
Comment on lines +70 to +73

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit. The loop never observes cancellation, so a timeout or an aborted invocation does not stop the fan-out.

for (;;) {
  if (firstError !== undefined) {
    return;
  }

node_context.ts:61-69 documents ctx.abortSignal as exactly the hook a node body is meant to poll, and node_runner.ts:192-193 sets it when the node declares a timeout (which this class accepts). On the deadline the runner stops consuming and fire-and-forgets iterator.return() (node_runner.ts:222) — but that request queues behind the in-flight await Promise.all(...), so the generator body runs to completion and every remaining item is still claimed and executed. Their events don't stop either: each child pushes into the shared channel through its own executeChildNode/runOnce, not through the parent's consume, so the runner's "nothing is pushed past the deadline" guarantee does not hold for this node.

for (;;) {
  if (firstError !== undefined || isAborted(ctx)) {
    return;
  }

with

const isAborted = (ctx: NodeContext): boolean =>
  ctx.abortSignal?.aborted === true ||
  ctx.invocationContext.abortSignal?.aborted === true;

Both checks are needed: ctx.abortSignal is only populated when the ParallelWorker itself declares a timeout (node_runner.ts:176), so on a plain invocation abort it is undefined. This only stops scheduling — already-started items still run to completion, because ctx.runNode has no way to take a signal. Worth stating that limit in the class doc rather than leaving it implied.

Smaller point on the same guard: firstError !== undefined doubles as the failure flag, so an item that rejects with undefined (a bare Promise.reject()) leaves it unset — that item is skipped, results keeps a hole, and the node resolves successfully with a silently wrong list. A separate let failed = false; next to firstError removes the ambiguity.

const i = nextIndex++;
if (i >= items.length) {
return;
}
try {
// Key each child run by its item index (not call order) so the
// run id -> item mapping is deterministic. On resume this lets each
// item fast-forward from its own cached run rather than being matched
// to a differently-ordered run id.
const child = await ctx.runNode(this.inner, items[i], {
useSubBranch: true,
runId: String(i),
});
Comment on lines +79 to +86

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. The runId is set but the node path is not, so outside the scheduler path the children are not actually distinguishable.

const child = await ctx.runNode(this.inner, items[i], {
  useSubBranch: true,
  runId: String(i),
});

executeChildNode computes nodePath as ${parent.nodePath}.${nodeName} (node_runner.ts:60-62) unless overrideNodePath is passed, and RunNodeOptions.overrideNodePath is documented as the field "used by the dynamic scheduler to embed the run id, e.g. wf.node@1, so distinct runs are distinguishable on resume" (node_runner.ts:36-41). With no scheduler set — the only path this PR's tests exercise — all N children emit events stamped with the same nodeInfo.path, so per-item attribution in the event stream is lost. The branch is unique here (useSubBranch + runId); the path is not.

The resume claim in the comment above also rests on ScheduleDynamicNode keying its runs map by (nodeName, runId), and only the interface is in this branch, so it can't be verified from here. Either pass overrideNodePath as well, or trim the comment to what this part actually guarantees and make the resume claim in the part that lands the scheduler.

results[i] = child.output;
} catch (err) {
if (firstError === undefined) {
firstError = err;
}
return;
}
}
};

await Promise.all(Array.from({length: poolSize}, () => worker()));

if (firstError !== undefined) {
throw firstError;
}
yield results;
Comment on lines +99 to +102

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit. One failing item throws away every sibling's completed output, and the caller never sees them.

if (firstError !== undefined) {
  throw firstError;
}
yield results;

At this point results holds the output of every item that finished, and all of it is discarded. With an LLM or remote-tool inner node those are results you have already paid for — and combined with the outer retry noted on the constructor, the next attempt pays for them again. Either emit a partial-result event before rethrowing, or state in the class doc that ParallelWorker is all-or-nothing so a graph author knows to make individual items failure-tolerant. What matters is that it is a deliberate choice; right now it reads as a side effect of the control flow.

Related, at line 22 the class doc says "cancelling on first error" and nothing is cancelled — the loop stops claiming new items while in-flight items run to completion (and, per the abort note, keep running past an abort too). "stopping on first error" is what the code does.

}
}

/**
* Registers the factory the engine uses to wrap a built node in a
* {@link ParallelWorker} when `buildNode(..., {parallelWorker: true})` is
* requested — keeping the engine core free of a static import of this module.
*/
registerParallelWorkerFactory(
(inner, options) => new ParallelWorker(inner, options),
);
95 changes: 95 additions & 0 deletions core/test/workflow/parallel_worker_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {describe, expect, it} from 'vitest';
import {FunctionNode} from '../../src/workflow/nodes/function_node.js';
import {JoinNode} from '../../src/workflow/nodes/join_node.js';
import {ParallelWorker} from '../../src/workflow/nodes/parallel_worker.js';
import {buildNode} from '../../src/workflow/utils/workflow_graph_utils.js';
import {driveNode} from './test_helpers.js';

describe('ParallelWorker', () => {
it('maps a list input through the inner node, preserving order', async () => {
const inner = new FunctionNode('double', (_c, n: number) => n * 2);
const {output} = await driveNode(new ParallelWorker(inner), [1, 2, 3, 4]);
expect(output).toEqual([2, 4, 6, 8]);
});

it('treats a non-list input as a single-element list', async () => {
const inner = new FunctionNode('double', (_c, n: number) => n * 2);
const {output} = await driveNode(new ParallelWorker(inner), 5);
expect(output).toEqual([10]);
});

it('yields an empty list for an empty input', async () => {
const inner = new FunctionNode('id', (_c, x) => x);
const {output} = await driveNode(new ParallelWorker(inner), []);
expect(output).toEqual([]);
});

it('bounds concurrency by maxParallelWorkers', async () => {
let active = 0;
let peak = 0;
const inner = new FunctionNode('track', async (_c, n: number) => {
active++;
peak = Math.max(peak, active);
await new Promise((r) => setTimeout(r, 5));
active--;
return n;
});
const {output} = await driveNode(
new ParallelWorker(inner, {maxParallelWorkers: 2}),
[1, 2, 3, 4, 5],
);
expect(output).toEqual([1, 2, 3, 4, 5]);
expect(peak).toBeLessThanOrEqual(2);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. This assertion passes on an implementation that isn't parallel at all.

expect(peak).toBeLessThanOrEqual(2);

peak would be 1 if the pool loop regressed to running items one at a time, and this test would stay green — so the only test of the class's headline feature can't fail in the direction that matters. expect(peak).toBe(2) pins both halves (never more than 2, and actually 2 at some point). It shouldn't be flaky the way timing assertions usually are: both workers are started before either item awaits, and the 5ms body means the first active-- is many microtask turns away.

});

it('rejects maxParallelWorkers < 1', () => {
const inner = new FunctionNode('x', (_c, v) => v);
expect(() => new ParallelWorker(inner, {maxParallelWorkers: 0})).toThrow(
/greater than or equal to 1/,
);
});

it('propagates the first error from a failing item', async () => {
const inner = new FunctionNode('boom', (_c, n: number) => {
if (n === 3) {
throw new Error('boom at 3');
}
return n;
});
await expect(
driveNode(new ParallelWorker(inner), [1, 2, 3, 4]),
).rejects.toThrow('boom at 3');
});
});

describe('ParallelWorker registry factory', () => {
it('buildNode wraps the built node when parallelWorker is requested', () => {
const node = buildNode((_c: unknown, n: number) => n, {
name: 'w',
parallelWorker: true,
});
expect(node).toBeInstanceOf(ParallelWorker);
});

it('rejects maxParallelWorkers without parallelWorker', () => {
expect(() =>
buildNode(() => {}, {name: 'x', maxParallelWorkers: 2}),
).toThrow(/maxParallelWorkers can only be set/);
});
});

describe('JoinNode', () => {
it('emits its aggregated input as output and requires all predecessors', async () => {
const join = new JoinNode({name: 'join'});
const aggregated = {a: 1, b: 2};
const {output} = await driveNode(join, aggregated);
expect(output).toEqual(aggregated);
expect(join.requiresAllPredecessors).toBe(true);
});
});
Loading