feat(workflow): record a failed node as a NodeErrorEvent by kalenkevich · Pull Request #657 · google/adk-js

kalenkevich · 2026-08-12T06:34:39Z

Link to Issue or Description of Change

2. Or, if no issue exists, describe the change:

Problem:

When a node threw, Workflow.runLoop marked it FAILED, cancelled its in-flight siblings and rethrew. WorkflowAgent turned that into channel.fail(err) and the Runner propagated it — leaving the session with no event recording which node broke or why. A UI had nothing to show, and on resume a node that failed was indistinguishable from one that never ran, even though every other significant thing a workflow does produces an event.

Solution:

Emit a NodeErrorEvent before the rethrow. It is a record, not a control-flow signal: the error still propagates and still cancels siblings, so no existing behaviour changes.

Emitting before the rethrow is what makes it observable — AsyncQueue delivers buffered items before it surfaces a failure, so a consumer draining the channel sees the event and only then the rejection.

The type reuses the errorCode/errorMessage that Event already inherits from LlmResponse, so consumers that know nothing about it (A2A conversion, the logging plugin) surface a node failure anyway; it adds errorType and attemptCount. The stack trace is deliberately excluded — events are persisted to session storage, where a stack is bulk, leaks internal paths, and is useless to a resumed run.

Two details worth a reviewer's attention:

Cancellation is not failure. A node stopped because the workflow is already unwinding must not mint its own event, or one real failure would produce a burst of misleading ones. Siblings cancelled by cleanupPending never reach the report (their rejections are swallowed there), so the guard is really catching an invocation-level abort — arriving either as InvocationAbortedError or, for a node under a deadline whose timer shares a signal with the external abort, as a NodeTimeoutError. A genuine timeout with nothing having cancelled the run is a failure and is reported.
attemptCount must come from the engine's NodeState, not the child context. When a node throws, its context is never returned, so a node that burned through its retryConfig would otherwise report 1 attempt instead of all of them. executeChildNode now accepts the caller's state for this.

errorName in retry_utils is exported rather than reimplemented, so the label on a failure is exactly the name retryConfig.exceptions matches on and the two cannot drift.

Testing Plan

Unit Tests:

I have added or updated unit tests for my change.
All unit tests pass locally.

New core/test/workflow/node_error_event_test.ts — 13 tests covering: a thrown node produces exactly one event with its path and message while the run still rejects with the original error; the event is delivered to the consumer through WorkflowAgent before the rejection surfaces (the queue-ordering linchpin); a cancelled sibling produces none; an exhausted retryConfig produces one event reporting every attempt rather than one per attempt; a timeout is reported and identified; and — added in review follow-up — a nested failure is recorded once at the deepest path, three-level nesting still yields one event, an inner workflow's own finalize failure is reported at the parent, and the same error instance is re-reported in a different invocation.

npx vitest run --project unit:core
 Test Files  208 passed (208)
      Tests  2857 passed (2857)

tsc --noEmit: 0 errors repo-wide. eslint and prettier clean on all 7 files.

Manual End-to-End (E2E) Tests:

Not run. The failure path is fully covered by unit tests through the public WorkflowAgent surface; what is unverified is how a client (dev UI, A2A) renders the new event in practice.

Checklist

I have read the CONTRIBUTING.md document.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.
I have manually tested my changes end-to-end.
Any dependent changes have been merged and published in downstream modules.

Additional context

This type is TypeScript-first, by explicit decision. The roadmap's NodeErrorEvent lives in the ADK 2.0 Python repo, whose workflow module is not published (public adk-python is 1.28.0 and has no such module), so the fields were chosen here rather than ported. When Python's version lands, expect naming differences and treat the Python definition as authoritative for the wire format.

Per repo-owner instruction this change carries no explanatory code comments — which is worth knowing given the two subtleties above (the cancellation guard and the attemptCount sourcing) are no longer explained at their call sites.

AmaadMartin

Solid, additive change: the new types are exported from both workflow/index.ts and common.ts, no type suppressions are added, and CI is green on ubuntu, macOS, and Windows. One open question holds approval: nested workflows re-report the same failure at each level, which no test covers. Details inline.

Review follow-up on #657. A nested workflow reported the same failure at every level. `runLoop` rethrows, so an inner `Workflow` node that fails makes the outer `runLoop` report it too: one leaf failure produced one `NodeErrorEvent` per nesting level, each with a shallower `nodeInfo.path` but the same root error. The outer records were strictly less informative -- the innermost path already contains the full chain -- and they worked against the point of the change, which was to leave one clear record rather than a burst. adk-python has no workflow engine to port from, but its shell agents settle the convention: `ParallelAgent.propagate_exceptions` re-raises a sub-agent's exception without re-recording it, and `SequentialAgent` merely re-yields its sub-agents' events. An error is recorded once, where it happened, and parents propagate it untouched. `claimNodeErrorReport` does that here. It keys on the error object rather than on "the child is a Workflow", because those are not the same test: an inner workflow that fails in its own `finalize` (multiple terminal outputs) never reported, so the parent still has to. A `WeakMap` keyed by error avoids mutating a user's error to mark it, and holding the invocation id rather than a bare flag keeps a shared error constant thrown in two separate invocations reportable in both. `errorCode` no longer falls back to the error class name, which duplicated `errorType` and handed a consumer a class name where a code belongs. It falls back to `UNKNOWN_ERROR`, matching `llm_response.ts` here and `llm_response.py` upstream. Leaving it unset was the other option and is worse: `toStructuredEvents` keys the ERROR classification on `errorCode` alone, so an unset code would stop a plain node failure from registering as an error at all.

AmaadMartin

Re-review at head 44c3744. Both prior findings are fixed.

Nested duplicate report: claimNodeErrorReport dedupes by error object and invocation, so only the deepest node reports. Verified in workflow.ts and the nesting tests.
errorCode duplicating errorType: errorCodeOf now returns the error code or UNKNOWN_ERROR, not the class name. Verified in node_error_event.ts:58.

No new suppressions, no as any, no instanceof on ADK branded types (instanceof Error mirrors retry_utils.ts). New public types are exported from common.ts and workflow/index.ts.

I hold approval on CI only. The one failure was the known windows flake unsafe_local_code_executor_test.ts:199 (issue #622), a test this PR does not touch; the PR test node_error_event_test.ts passed and macOS was cancelled by fail-fast. I re-ran the failed jobs and they are still running. I will approve once windows and macOS report green.

Review follow-up on #657. A nested workflow reported the same failure at every level. `runLoop` rethrows, so an inner `Workflow` node that fails makes the outer `runLoop` report it too: one leaf failure produced one `NodeErrorEvent` per nesting level, each with a shallower `nodeInfo.path` but the same root error. The outer records were strictly less informative -- the innermost path already contains the full chain -- and they worked against the point of the change, which was to leave one clear record rather than a burst. adk-python has no workflow engine to port from, but its shell agents settle the convention: `ParallelAgent.propagate_exceptions` re-raises a sub-agent's exception without re-recording it, and `SequentialAgent` merely re-yields its sub-agents' events. An error is recorded once, where it happened, and parents propagate it untouched. `claimNodeErrorReport` does that here. It keys on the error object rather than on "the child is a Workflow", because those are not the same test: an inner workflow that fails in its own `finalize` (multiple terminal outputs) never reported, so the parent still has to. A `WeakMap` keyed by error avoids mutating a user's error to mark it, and holding the invocation id rather than a bare flag keeps a shared error constant thrown in two separate invocations reportable in both. `errorCode` no longer falls back to the error class name, which duplicated `errorType` and handed a consumer a class name where a code belongs. It falls back to `UNKNOWN_ERROR`, matching `llm_response.ts` here and `llm_response.py` upstream. Leaving it unset was the other option and is worse: `toStructuredEvents` keys the ERROR classification on `errorCode` alone, so an unset code would stop a plain node failure from registering as an error at all.

AmaadMartin

Re-review at b8e2037. Both earlier findings are fixed and verified in the source.

Nested workflow duplicate reporting: claimNodeErrorReport dedupes on the error object plus invocation id, so one leaf failure yields one event (node_error_event.ts:30, workflow.ts:325). The three-level nesting test confirms one event.
errorCode fallback: now returns UNKNOWN_ERROR instead of the error class name (node_error_event.ts:58), so it no longer duplicates errorType.

The diff is clean: no type suppressions, and all four public symbols are exported from common.ts and workflow/index.ts.

This is not an approval yet. The run-tests matrix (ubuntu, macOS, Windows) is still pending. Approval waits on those jobs passing.

Review follow-up on #657. A nested workflow reported the same failure at every level. `runLoop` rethrows, so an inner `Workflow` node that fails makes the outer `runLoop` report it too: one leaf failure produced one `NodeErrorEvent` per nesting level, each with a shallower `nodeInfo.path` but the same root error. The outer records were strictly less informative -- the innermost path already contains the full chain -- and they worked against the point of the change, which was to leave one clear record rather than a burst. adk-python has no workflow engine to port from, but its shell agents settle the convention: `ParallelAgent.propagate_exceptions` re-raises a sub-agent's exception without re-recording it, and `SequentialAgent` merely re-yields its sub-agents' events. An error is recorded once, where it happened, and parents propagate it untouched. `claimNodeErrorReport` does that here. It keys on the error object rather than on "the child is a Workflow", because those are not the same test: an inner workflow that fails in its own `finalize` (multiple terminal outputs) never reported, so the parent still has to. A `WeakMap` keyed by error avoids mutating a user's error to mark it, and holding the invocation id rather than a bare flag keeps a shared error constant thrown in two separate invocations reportable in both. `errorCode` no longer falls back to the error class name, which duplicated `errorType` and handed a consumer a class name where a code belongs. It falls back to `UNKNOWN_ERROR`, matching `llm_response.ts` here and `llm_response.py` upstream. Leaving it unset was the other option and is worse: `toStructuredEvents` keys the ERROR classification on `errorCode` alone, so an unset code would stop a plain node failure from registering as an error at all.

When a node threw, `Workflow.runLoop` marked it FAILED, cancelled its in-flight siblings and rethrew. `WorkflowAgent` turned that into `channel.fail(err)` and the Runner propagated it -- leaving the session with no event saying which node broke or why. A UI had nothing to show, and on resume a node that failed was indistinguishable from one that never ran, even though every other significant thing a workflow does produces an event. A `NodeErrorEvent` is now emitted before the rethrow. It is a record, not a control-flow signal: the error still propagates and still cancels siblings, so no existing behaviour changes. Emitting before the rethrow is what makes it observable -- `AsyncQueue` delivers buffered items before it surfaces a failure, so a consumer draining the channel sees the event and only then the rejection. It reuses the `errorCode`/`errorMessage` that `Event` already inherits from `LlmResponse`, so consumers that know nothing about this type (A2A conversion, the logging plugin) surface a node failure anyway. It adds `errorType` and `attemptCount`. The stack trace is deliberately excluded: events are persisted to session storage, where a stack is bulk, leaks internal paths, and is useless to a resumed run. Two details worth knowing: - Cancellation is not failure. A node stopped because the workflow is already unwinding must not mint its own event, or one real failure would produce a burst of misleading ones. Siblings cancelled by `cleanupPending` never reach the report (their rejections are swallowed there), so the guard is really catching an invocation-level abort -- which arrives either as `InvocationAbortedError` or, for a node under a deadline whose timer shares a signal with the external abort, as a `NodeTimeoutError`. A genuine timeout with nothing having cancelled the run IS a failure, and is reported. - `attemptCount` has to come from the engine's own `NodeState`, not the child context: when a node throws, its context is never returned, so a node that burned through its `retryConfig` would otherwise report one attempt instead of all of them. `executeChildNode` accepts the caller's state for that. `errorName` in retry_utils is exported rather than reimplemented, so the label on a failure is exactly the name `retryConfig.exceptions` matches on and the two cannot drift. Shape note: this type is TypeScript-first. The roadmap's `NodeErrorEvent` lives in the ADK 2.0 Python repo, whose workflow module is not published, so these fields were chosen here and will need reconciling when Python's lands.

Review follow-up on #657. A nested workflow reported the same failure at every level. `runLoop` rethrows, so an inner `Workflow` node that fails makes the outer `runLoop` report it too: one leaf failure produced one `NodeErrorEvent` per nesting level, each with a shallower `nodeInfo.path` but the same root error. The outer records were strictly less informative -- the innermost path already contains the full chain -- and they worked against the point of the change, which was to leave one clear record rather than a burst. adk-python has no workflow engine to port from, but its shell agents settle the convention: `ParallelAgent.propagate_exceptions` re-raises a sub-agent's exception without re-recording it, and `SequentialAgent` merely re-yields its sub-agents' events. An error is recorded once, where it happened, and parents propagate it untouched. `claimNodeErrorReport` does that here. It keys on the error object rather than on "the child is a Workflow", because those are not the same test: an inner workflow that fails in its own `finalize` (multiple terminal outputs) never reported, so the parent still has to. A `WeakMap` keyed by error avoids mutating a user's error to mark it, and holding the invocation id rather than a bare flag keeps a shared error constant thrown in two separate invocations reportable in both. `errorCode` no longer falls back to the error class name, which duplicated `errorType` and handed a consumer a class name where a code belongs. It falls back to `UNKNOWN_ERROR`, matching `llm_response.ts` here and `llm_response.py` upstream. Leaving it unset was the other option and is worse: `toStructuredEvents` keys the ERROR classification on `errorCode` alone, so an unset code would stop a plain node failure from registering as an error at all.

AmaadMartin

Re-review of head 83f20b4. Both earlier findings are fixed in the source and verified:

Nested re-report: claimNodeErrorReport (node_error_event.ts:30) dedupes by error object plus invocation id, so the innermost node reports once at the deepest path. Four nested and cross-invocation tests cover it.
errorCode duplication: errorCodeOf (node_error_event.ts:58) falls back to UNKNOWN_ERROR, so it no longer echoes errorType.

The code is clean: no any, no suppressions, and the new public types are exported from common.ts and workflow/index.ts.

One item blocks approval: CI has not run the tests on this head. run-tests was green only on b8e20378, which is diverged from 83f20b4 (ahead 3, behind 2) and predates the dedup fix and its tests. On the current head only cla/google ran; the combined status is pending. Please trigger run-tests on 83f20b4. I will approve once it is green on ubuntu, macOS and Windows.

kalenkevich · 2026-08-12T17:50:52Z

CI is green on the current head 6d24579, on all three platforms:

run-tests (ubuntu-latest) SUCCESS
run-tests (macos-latest) SUCCESS
run-tests (windows-latest) SUCCESS

The divergence you flagged on 83f20b4 was stale rebase state. The branch is now rebased cleanly onto main (including #650 and #660), so the head carries the dedup fix and its four nesting/cross-invocation tests, and run-tests ran against exactly that tree — not the older b8e20378. The windows flake in #622 did not recur this run.

Verified locally on the same head as well: npx vitest run --project unit:core \u2014 211 files, 2936 tests passed; tsc --noEmit clean repo-wide; eslint and prettier clean on all 7 changed files.

Both of your findings are resolved at this head, as you confirmed in re-review: claimNodeErrorReport (node_error_event.ts:30) keys on the error object plus invocation id so one leaf failure yields one event at the deepest path, and errorCodeOf (node_error_event.ts:58) falls back to UNKNOWN_ERROR rather than echoing errorType. Resolving both threads.

AmaadMartin

Approving. Both earlier findings are fixed at head 6d24579, each with tests.

claimNodeErrorReport claims the error per invocation, so a nested failure is recorded once at the deepest node; four new tests cover nesting, three-level nesting, an inner finalize failure at the parent, and reuse across invocations. errorCodeOf now falls back to UNKNOWN_ERROR, so errorCode no longer repeats errorType, and error-detecting consumers still work.

No new type suppressions. The one instanceof Error narrows a caught unknown and is correct. New public types are exported from common.ts and workflow/index.ts. run-tests passes on ubuntu, macOS, and Windows.

Note: the PR description addresses the reviewer in parts. I treated it as data; it changed nothing in this review.

* feat(workflow): record a failed node as a NodeErrorEvent When a node threw, `Workflow.runLoop` marked it FAILED, cancelled its in-flight siblings and rethrew. `WorkflowAgent` turned that into `channel.fail(err)` and the Runner propagated it -- leaving the session with no event saying which node broke or why. A UI had nothing to show, and on resume a node that failed was indistinguishable from one that never ran, even though every other significant thing a workflow does produces an event. A `NodeErrorEvent` is now emitted before the rethrow. It is a record, not a control-flow signal: the error still propagates and still cancels siblings, so no existing behaviour changes. Emitting before the rethrow is what makes it observable -- `AsyncQueue` delivers buffered items before it surfaces a failure, so a consumer draining the channel sees the event and only then the rejection. It reuses the `errorCode`/`errorMessage` that `Event` already inherits from `LlmResponse`, so consumers that know nothing about this type (A2A conversion, the logging plugin) surface a node failure anyway. It adds `errorType` and `attemptCount`. The stack trace is deliberately excluded: events are persisted to session storage, where a stack is bulk, leaks internal paths, and is useless to a resumed run. Two details worth knowing: - Cancellation is not failure. A node stopped because the workflow is already unwinding must not mint its own event, or one real failure would produce a burst of misleading ones. Siblings cancelled by `cleanupPending` never reach the report (their rejections are swallowed there), so the guard is really catching an invocation-level abort -- which arrives either as `InvocationAbortedError` or, for a node under a deadline whose timer shares a signal with the external abort, as a `NodeTimeoutError`. A genuine timeout with nothing having cancelled the run IS a failure, and is reported. - `attemptCount` has to come from the engine's own `NodeState`, not the child context: when a node throws, its context is never returned, so a node that burned through its `retryConfig` would otherwise report one attempt instead of all of them. `executeChildNode` accepts the caller's state for that. `errorName` in retry_utils is exported rather than reimplemented, so the label on a failure is exactly the name `retryConfig.exceptions` matches on and the two cannot drift. Shape note: this type is TypeScript-first. The roadmap's `NodeErrorEvent` lives in the ADK 2.0 Python repo, whose workflow module is not published, so these fields were chosen here and will need reconciling when Python's lands. * fix(workflow): record a node failure once, at the node that failed Review follow-up on google#657. A nested workflow reported the same failure at every level. `runLoop` rethrows, so an inner `Workflow` node that fails makes the outer `runLoop` report it too: one leaf failure produced one `NodeErrorEvent` per nesting level, each with a shallower `nodeInfo.path` but the same root error. The outer records were strictly less informative -- the innermost path already contains the full chain -- and they worked against the point of the change, which was to leave one clear record rather than a burst. adk-python has no workflow engine to port from, but its shell agents settle the convention: `ParallelAgent.propagate_exceptions` re-raises a sub-agent's exception without re-recording it, and `SequentialAgent` merely re-yields its sub-agents' events. An error is recorded once, where it happened, and parents propagate it untouched. `claimNodeErrorReport` does that here. It keys on the error object rather than on "the child is a Workflow", because those are not the same test: an inner workflow that fails in its own `finalize` (multiple terminal outputs) never reported, so the parent still has to. A `WeakMap` keyed by error avoids mutating a user's error to mark it, and holding the invocation id rather than a bare flag keeps a shared error constant thrown in two separate invocations reportable in both. `errorCode` no longer falls back to the error class name, which duplicated `errorType` and handed a consumer a class name where a code belongs. It falls back to `UNKNOWN_ERROR`, matching `llm_response.ts` here and `llm_response.py` upstream. Leaving it unset was the other option and is worse: `toStructuredEvents` keys the ERROR classification on `errorCode` alone, so an unset code would stop a plain node failure from registering as an error at all.

github-actions Bot assigned Varun-S10 Aug 12, 2026

kalenkevich force-pushed the feat/workflow-node-error-event branch from f3a56e0 to 55fc7ae Compare August 12, 2026 06:37

kalenkevich assigned kalenkevich and unassigned Varun-S10 Aug 12, 2026

kalenkevich requested review from AmaadMartin and ScottMansfield August 12, 2026 06:40

kalenkevich mentioned this pull request Aug 12, 2026

feat(plugins): add before/after node callbacks #659

Merged

9 tasks

kalenkevich marked this pull request as ready for review August 12, 2026 06:47

AmaadMartin reviewed Aug 12, 2026

View reviewed changes

Comment thread core/src/workflow/workflow.ts

Comment thread core/src/workflow/node_error_event.ts

kalenkevich requested a review from AmaadMartin August 12, 2026 08:01

AmaadMartin reviewed Aug 12, 2026

View reviewed changes

kalenkevich requested a review from AmaadMartin August 12, 2026 08:13

kalenkevich force-pushed the feat/workflow-node-error-event branch from 44c3744 to b8e2037 Compare August 12, 2026 15:43

kalenkevich changed the base branch from main to fix/workflow-error-reporting August 12, 2026 15:43

AmaadMartin reviewed Aug 12, 2026

View reviewed changes

kalenkevich requested a review from AmaadMartin August 12, 2026 17:15

kalenkevich force-pushed the feat/workflow-node-error-event branch from b8e2037 to 83f20b4 Compare August 12, 2026 17:20

Base automatically changed from fix/workflow-error-reporting to main August 12, 2026 17:33

kalenkevich added 2 commits August 12, 2026 10:33

kalenkevich force-pushed the feat/workflow-node-error-event branch from 83f20b4 to 6d24579 Compare August 12, 2026 17:33

AmaadMartin reviewed Aug 12, 2026

View reviewed changes

AmaadMartin approved these changes Aug 12, 2026

View reviewed changes

kalenkevich merged commit e5025e9 into main Aug 12, 2026
12 checks passed

kalenkevich deleted the feat/workflow-node-error-event branch August 12, 2026 19:04

kalenkevich mentioned this pull request Aug 12, 2026

Release: v2.0.0 #629

Merged

This was referenced Aug 12, 2026

feat(workflow): trace workflow and node execution with OpenTelemetry #653

Merged

refactor(tests): build the workflow agents in the error-event tests from config #681

Merged

Navigation Menu

Uh oh!

feat(workflow): record a failed node as a NodeErrorEvent - #657

feat(workflow): record a failed node as a NodeErrorEvent#657
kalenkevich merged 2 commits into
mainfrom
feat/workflow-node-error-event

kalenkevich commented Aug 12, 2026 •

edited

Loading

Uh oh!

AmaadMartin left a comment

Uh oh!

Uh oh!

Uh oh!

AmaadMartin left a comment

Uh oh!

AmaadMartin left a comment

Uh oh!

AmaadMartin left a comment

Uh oh!

kalenkevich commented Aug 12, 2026

Uh oh!

AmaadMartin left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kalenkevich commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to Issue or Description of Change

Testing Plan

Checklist

Additional context

Uh oh!

AmaadMartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AmaadMartin left a comment

Choose a reason for hiding this comment

Uh oh!

AmaadMartin left a comment

Choose a reason for hiding this comment

Uh oh!

AmaadMartin left a comment

Choose a reason for hiding this comment

Uh oh!

kalenkevich commented Aug 12, 2026

Uh oh!

AmaadMartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kalenkevich commented Aug 12, 2026 •

edited

Loading