fix(scheduler): preserve stages on transition errors - #2215
Conversation
Build replacement stages before updating the execution graph so failed plan rewrites leave the original stage intact. Add regression coverage for resolve and rollback transitions. Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
sandugood
left a comment
sandugood
left a comment
There was a problem hiding this comment.
Nicely done, thanks @QuakeWang. Added a comment
| stage_id, | ||
| ExecutionStage::UnResolved(stage.to_unresolved(failure_reasons)?), | ||
| ); | ||
| let unresolved_stage = stage.to_unresolved(failure_reasons)?; |
There was a problem hiding this comment.
When this errors, the running_tasks was already collected, but it is now discarded. Better than a vanished stage, but it's a distinct bad state. Maybe add a line to "Are there any user facing changes"?
There was a problem hiding this comment.
Good point. The running-task list was also discarded on this error path before this PR, but preserving the stage makes the resulting behavior worth stating explicitly: the stage remains Running, its task tracking is retained, and the error propagates before a CancelTasks event can be emitted. I have clarified this in the user-facing changes section. Changing cancellation or recovery policy would be outside the failure-atomic transition scope of this PR.
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
thanks @QuakeWang this make sense to me
3 participants
Which issue does this PR close?
Closes #2214
Rationale for this change
StaticExecutionGraphremoved a stage before running fallible plan transformations. If a transformation failed, the error returned before the stage was reinserted, leaving the execution graph without the original stage.What changes are included in this PR?
Build replacement stages before updating the stage map, so failed resolve and rollback transitions preserve the original stage. Add regression tests for all three affected transitions.
Are there any user-facing changes?
No API changes. On a failed running-stage rollback, the original stage and its running-task tracking are preserved, but the error still propagates before a
CancelTasksevent can be produced.Testing
ci/scripts/rust_fmt.sh