Skip to content

Commit 8db2ace

Browse files
DeanChensjcopybara-github
authored andcommitted
fix(workflow): Process all completed tasks in batch before exiting on error
Fixes a fail-fast bug where sibling nodes that completed in the same event loop tick as a failed node had their completions dropped. Now, all done tasks in the batch are processed to record their outputs and advance the sequence barrier before the workflow shuts down. Co-authored-by: Shangjie Chen <deanchen@google.com> PiperOrigin-RevId: 944711374
1 parent 989af4d commit 8db2ace

2 files changed

Lines changed: 99 additions & 7 deletions

File tree

src/google/adk/workflow/_workflow.py

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,7 @@ def get_recovered_sequence_index(t):
351351

352352
sorted_done = sorted(ordered_done, key=get_recovered_sequence_index)
353353

354+
error_to_raise = None
354355
for task in sorted_done:
355356
name = self._pop_completed_task(loop_state, task)
356357

@@ -365,14 +366,17 @@ def get_recovered_sequence_index(t):
365366
node_state = loop_state.nodes[name]
366367
node_state.status = NodeStatus.FAILED
367368

368-
ctx._error = child_ctx.error
369-
ctx._error_node_path = child_ctx.error_node_path
370-
371-
loop_state.error_shut_down = True
372-
logger.debug('node %s execute loop end.', ctx.node_path)
373-
return
369+
if not error_to_raise:
370+
ctx._error = child_ctx.error
371+
ctx._error_node_path = child_ctx.error_node_path
372+
error_to_raise = child_ctx.error
373+
else:
374+
self._handle_completion(loop_state, name, node, child_ctx)
374375

375-
self._handle_completion(loop_state, name, node, child_ctx)
376+
if error_to_raise:
377+
loop_state.error_shut_down = True
378+
logger.debug('node %s execute loop end.', ctx.node_path)
379+
return
376380

377381
# Await fire-and-forget dynamic tasks.
378382
# TODO: Handle dynamic task failures and interrupts here.

tests/unittests/workflow/test_workflow_failures.py

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,3 +1068,91 @@ def failing_node(ctx: Context):
10681068
and e.node_info.path == 'test_error_workflow@1'
10691069
]
10701070
assert len(workflow_error_events) == 0
1071+
1072+
1073+
@pytest.mark.asyncio
1074+
async def test_fail_fast_preserves_completed_siblings(
1075+
request: pytest.FixtureRequest,
1076+
):
1077+
"""Tests that when one node fails, other sibling nodes completed in the same tick still have their outputs preserved."""
1078+
node_success_started = False
1079+
node_success_completed = False
1080+
1081+
@node()
1082+
async def succeeding_node(ctx: Context):
1083+
nonlocal node_success_started, node_success_completed
1084+
node_success_started = True
1085+
await asyncio.sleep(0.1)
1086+
node_success_completed = True
1087+
return 'success_output'
1088+
1089+
@node()
1090+
async def failing_node(ctx: Context):
1091+
await asyncio.sleep(0.1)
1092+
raise ValueError('Fail')
1093+
1094+
wf = Workflow(
1095+
name='test_fail_fast_workflow',
1096+
edges=[
1097+
(START, failing_node),
1098+
(START, succeeding_node),
1099+
],
1100+
)
1101+
1102+
original_handle_completion = Workflow._handle_completion
1103+
handle_completion_calls = []
1104+
1105+
def spy_handle_completion(self, loop_state, node_name, node_obj, child_ctx):
1106+
handle_completion_calls.append(node_name)
1107+
return original_handle_completion(
1108+
self, loop_state, node_name, node_obj, child_ctx
1109+
)
1110+
1111+
app = App(name=request.function.__name__, root_agent=wf)
1112+
runner = testing_utils.InMemoryRunner(app=app)
1113+
1114+
with mock.patch.object(
1115+
Workflow, '_handle_completion', new=spy_handle_completion
1116+
):
1117+
with pytest.raises(ValueError, match='Fail'):
1118+
await runner.run_async(testing_utils.get_user_content('start'))
1119+
1120+
# The succeeding_node should have successfully completed.
1121+
assert node_success_started is True
1122+
assert node_success_completed is True
1123+
1124+
# Under the bug, succeeding_node's completion handler was skipped.
1125+
# With the fix, succeeding_node's completion is handled.
1126+
assert 'failing_node' not in handle_completion_calls
1127+
assert 'succeeding_node' in handle_completion_calls
1128+
1129+
1130+
@pytest.mark.asyncio
1131+
async def test_multiple_failures_first_error_wins(
1132+
request: pytest.FixtureRequest,
1133+
):
1134+
"""Tests that when multiple parallel nodes fail in the same tick, the first error is preserved."""
1135+
1136+
@node()
1137+
async def failing_node_1(ctx: Context):
1138+
await asyncio.sleep(0.1)
1139+
raise ValueError('Fail 1')
1140+
1141+
@node()
1142+
async def failing_node_2(ctx: Context):
1143+
await asyncio.sleep(0.1)
1144+
raise ValueError('Fail 2')
1145+
1146+
wf = Workflow(
1147+
name='test_multiple_failures_workflow',
1148+
edges=[
1149+
(START, failing_node_1),
1150+
(START, failing_node_2),
1151+
],
1152+
)
1153+
1154+
app = App(name=request.function.__name__, root_agent=wf)
1155+
runner = testing_utils.InMemoryRunner(app=app)
1156+
1157+
with pytest.raises(ValueError, match='Fail 1'):
1158+
await runner.run_async(testing_utils.get_user_content('start'))

0 commit comments

Comments
 (0)