fix(workflow-orchestration): correct resume and failure-branch routing in WorkflowRunner
- Resume no longer infinite-loops when a crash occurs between append_step_result() and update_run() for a BranchStep: the recorded branch target is now read from the persisted result instead of recomputed as None. - Resume no longer silently discards a recorded step failure that happened right before the crash; the run correctly ends failed. - A failed step's own next_step_id (e.g. routing to a BranchStep that evaluates step_result_success) is now honored instead of unconditionally forcing the run to failed, per spec. openspec: workflow-orchestration capability, archived change workflow-orchestration-runtime
This commit is contained in:
@@ -14,6 +14,7 @@ from workflow.models import (
|
||||
SkillInvocationStep,
|
||||
WaitForConditionStep,
|
||||
WorkflowDefinition,
|
||||
WorkflowStepResult,
|
||||
)
|
||||
from workflow.runner import WorkflowRunner
|
||||
from workflow.store import WorkflowStore
|
||||
@@ -266,6 +267,145 @@ def test_workflow_runner_resume_skips_completed_steps_after_restart(tmp_path) ->
|
||||
assert calls == ["one", "two", "three"]
|
||||
|
||||
|
||||
def test_workflow_runner_resume_after_branch_crash_advances_to_recorded_target(
|
||||
tmp_path,
|
||||
) -> None:
|
||||
"""Simulates a crash between append_step_result() and update_run() for a
|
||||
BranchStep: the step's result is persisted but the checkpoint never
|
||||
happened, so current_step_id still points at the branch step itself.
|
||||
Resuming must recompute the next step from the *recorded* branch
|
||||
target instead of losing it (and must not loop forever re-processing
|
||||
the branch step).
|
||||
"""
|
||||
calls: list[str] = []
|
||||
store = _store(tmp_path)
|
||||
definition = WorkflowDefinition(
|
||||
name="branch-resume",
|
||||
entry_step_id="branch",
|
||||
steps=[
|
||||
BranchStep(
|
||||
"branch",
|
||||
ConditionSpec("world_variable_equals", {"name": "ready", "value": True}),
|
||||
on_true="true-step",
|
||||
on_false="false-step",
|
||||
),
|
||||
PlannedGoalStep("false-step", "false path"),
|
||||
PlannedGoalStep("true-step", "true path"),
|
||||
],
|
||||
)
|
||||
store.save_definition(definition)
|
||||
run = store.create_run(definition.id, {"ready": True}, device_id="phone")
|
||||
store.append_step_result(
|
||||
run.id,
|
||||
WorkflowStepResult(
|
||||
step_id="branch",
|
||||
kind="branch",
|
||||
success=True,
|
||||
detail={"condition_result": True, "next_step_id": "true-step"},
|
||||
),
|
||||
)
|
||||
|
||||
runner = WorkflowRunner(
|
||||
store,
|
||||
task_runner_factory=lambda: FakeTaskRunner(calls),
|
||||
)
|
||||
resumed = runner.resume(run.id)
|
||||
|
||||
assert resumed.status == "completed"
|
||||
assert calls == ["true path"]
|
||||
assert [result.step_id for result in resumed.step_results] == [
|
||||
"branch",
|
||||
"true-step",
|
||||
]
|
||||
|
||||
|
||||
def test_workflow_runner_resume_after_failed_step_crash_ends_failed(tmp_path) -> None:
|
||||
"""Simulates a crash between append_step_result(success=False) and
|
||||
update_run(): the failed result is persisted but current_step_id still
|
||||
points at the failed step. Resuming must recognize the recorded
|
||||
failure and end the run as failed rather than silently continuing (or
|
||||
looping forever re-processing the same step).
|
||||
"""
|
||||
store = _store(tmp_path)
|
||||
definition = WorkflowDefinition(
|
||||
name="fail-resume",
|
||||
entry_step_id="only",
|
||||
steps=[PlannedGoalStep("only", "risky")],
|
||||
)
|
||||
store.save_definition(definition)
|
||||
run = store.create_run(definition.id, {}, device_id="phone")
|
||||
store.append_step_result(
|
||||
run.id,
|
||||
WorkflowStepResult(
|
||||
step_id="only",
|
||||
kind="planned_goal",
|
||||
success=False,
|
||||
detail={"task_status": "failed", "failure_reason": "boom"},
|
||||
),
|
||||
)
|
||||
|
||||
runner = WorkflowRunner(store)
|
||||
resumed = runner.resume(run.id)
|
||||
|
||||
assert resumed.status == "failed"
|
||||
assert resumed.current_step_id is None
|
||||
|
||||
|
||||
def test_workflow_runner_failed_step_routes_to_recovery_branch(tmp_path) -> None:
|
||||
"""A planned-goal step that fails still routes to its declared
|
||||
next_step_id (a branch step evaluating step_result_success). Live
|
||||
execution must reach and execute the on_false recovery path rather
|
||||
than immediately terminating the run as failed.
|
||||
"""
|
||||
calls: list[str] = []
|
||||
|
||||
class ScriptedTaskRunner:
|
||||
def __init__(self, calls: list[str], failing_goals: set[str]) -> None:
|
||||
self.calls = calls
|
||||
self.failing_goals = failing_goals
|
||||
|
||||
def run(self, task: Task) -> Task:
|
||||
self.calls.append(task.goal)
|
||||
if task.goal in self.failing_goals:
|
||||
task.status = "failed" # type: ignore[assignment]
|
||||
task.failure_reason = "boom"
|
||||
else:
|
||||
task.status = "completed" # type: ignore[assignment]
|
||||
task.failure_reason = None
|
||||
return task
|
||||
|
||||
definition = WorkflowDefinition(
|
||||
name="failure-recovery",
|
||||
entry_step_id="goal",
|
||||
steps=[
|
||||
PlannedGoalStep("goal", "risky goal", next_step_id="check"),
|
||||
BranchStep(
|
||||
"check",
|
||||
ConditionSpec("step_result_success", {"step_id": "goal"}),
|
||||
on_true="happy",
|
||||
on_false="recover",
|
||||
),
|
||||
PlannedGoalStep("happy", "happy path"),
|
||||
PlannedGoalStep("recover", "recover path"),
|
||||
],
|
||||
)
|
||||
runner = WorkflowRunner(
|
||||
_store(tmp_path),
|
||||
task_runner_factory=lambda: ScriptedTaskRunner(calls, {"risky goal"}),
|
||||
)
|
||||
|
||||
run = runner.run(definition, "phone")
|
||||
|
||||
assert run.status == "completed"
|
||||
assert calls == ["risky goal", "recover path"]
|
||||
assert [result.step_id for result in run.step_results] == [
|
||||
"goal",
|
||||
"check",
|
||||
"recover",
|
||||
]
|
||||
assert run.step_results[0].success is False
|
||||
|
||||
|
||||
def test_workflow_run_persists_variables_and_ordered_results(tmp_path) -> None:
|
||||
store = _store(tmp_path)
|
||||
definition = WorkflowDefinition(
|
||||
|
||||
Reference in New Issue
Block a user