Repository navigation
feat(workflow): end hooks A — finally on every flowable and the flow, errors and finally on the loops - #759
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… safety divergence) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…en against Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…se finally is refused for #9 The model reads `finally:` on Sequential, Parallel, If, Switch, Dag, Loop, LoopUntil, WorkingDirectory and Pause, and `errors:` on Loop, LoopUntil and WorkingDirectory; children()/error_children() and their _mut twins walk them. pause_gap refuses a Pause `finally` (unsupported_pause_property, names #9). The keys walker descends the new lists and property-treatment.json is regenerated (finally/errors ignored -> honoured on 12 lines). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Plan::finally and PlanTask::finally hold the new lists, appended after every index the plan held before (D3); AllowFailure keeps its finally inline. The graph draws a finally for every kind that has one. A flowable finally and a Loop errors are covered for children_mut and publish resolution, and a non-list finally is a parse error naming the key. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…x stale plan comments Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…stra's list rule flowable::current_list is Kestra's findTaskDependingFlowState with its third list (Current::Finally): tasks, then errors on a failure for good, then finally, one child at a time also under Parallel and Dag. Sequential, Parallel, Dag, If, Switch, AllowFailure and the flow's root call pass their finally; a block's and the execution's state stays the aggregate of tasks alone. A flowable with nothing to run (empty If branch, all children disabled) ends without its finally. A Dag with errors keeps its dependsOn gate (it used to be handed to parallel::drive). LoopUntil and WorkingDirectory finally stay A5's. Ratchet 301/72/72/0 -> 301/68/68/0: finally-dag, finally-parallel, finally-sequential and finally-sequential-error bank. The five flow-level finally entries stay baselined: worker::claim::leaf does not walk flow.finally_, so their leaves fail NotALeaf. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Five private walks turned a stored run's task id back into its model task, each over a different subset: claim::leaf missed the flow's finally (so its leaves failed "not a leaf"), spawn::find_task missed every errors list below the root (the tick aborted for a Loop there), and working_dir's block_of and parent_block_of saw flow.tasks alone. All five now call spec::path::find_task, the every_task tree without paths, pinned against every planned task of every corpus flow. Ratchet 301/68/68/0 -> 301/63/63/0: finally-flow, finally-flow-error, finally-flow-error-first, finally-sequential-error-first and restart-with-finally bank. task-runif-taskswithstate stays baselined on Pebble's `empty` test (a renderer cause, now a handler-surface blocker). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…inally LoopUntil (D6): each iteration walks tasks, then errors on a failure, then finally, before the condition is asked; condition_is_asked gates the render on the same walk. current_list no longer counts a run RestartLoop threw away as started, so a later iteration does not resume the earlier finally. WorkingDirectory (D7, Q4): after the finish claim, errors (on failure) and finally (always) run as ordinary children outside the directory. Only a child in the block's tasks is inside it: flowable::start submits a hook leaf, the claim joins the block only while PREPARED (a hook child gets no block_id or snapshot), and carry_of refuses a tasks child whose block is not prepared. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sub_flow_source makes the Loop's own errors/finally the iteration's (the parent flow's are not inherited; an iteration has no afterExecution). Ratchet 301/63/63/0 -> 301/60/60/0: invalids/loop-switch-failed, valids/finally-loop and valids/finally-loop-parallel bank. The census constants and handler-surface.txt move with them (dated). Also: restore the refusal message's line continuation in working_dir.rs and assert it exactly, fix the block_snapshot doc, and pin that a failed finish claim runs neither errors nor finally of a WorkingDirectory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Drop the dead finally/errors entries from silent() and the TWO cross-check exemption, replace the old 'draws nothing' test with path-pinned typo tests, correct stale notes on sub_flow_source and the oracle README/baseline, test nested Loops in sub_flow_source, and pin the V3 failure reason. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…; pin Pause finally Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The planner asks pause_plan_gap, which ignores finally:, so an execution started before #2a and re-planned through plan_stored keeps the Pause a TaskKind::Pause at the same index (D4). spec::validate still asks pause_gap, so publish and new starts refuse finally: naming #9. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A run a LoopUntil restart emptied and the next iteration never rebuilt is not live, so a Parallel entering finally after success no longer persists a ghost FAILED for it (review M3). The end-hooks drive harness now records only terminal execution states, and every drive test asserts the terminal state it expects, so a hang cannot pass (review M4). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The end-hooks spec records that a stored execution keeps running a Pause with finally: as before (D4, D14), and a deploy note: stop old workers first, plus the two known deploy-time edge cases (malformed lists and duplicate ids stored before #2a). The VI mirrors of the roadmap, the programme spec and the no-silent-drops spec get the 2026-10-27 notes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The plan-layout fixture regenerator reads this variable directly; the env allow-list test failed CI on it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workflow parity roadmap package #2a, "End hooks, Part A":
finally:now runs on every flowable and on the flow, andLoop,LoopUntilandWorkingDirectoryrun their ownerrors:andfinally:, following Kestra's list rule (tasks, thenerrorsonly on failure, thenfinally; a block's and the execution's state come from the main tasks only, so a failingfinallychanges nothing).afterExecution:and the Rule 2 sweep fix are #2b.Pausefinally:moves to #9 and is refused naming it.Closes #
Changes
finally:and the loops'errors:; the plan appends the new lists after today's layout, so every storedtask_runs.task_ixstays valid (a frozen layout fixture and a prefix test guard it).current_list) in the executor;advancestays pure. Parallel/Dag/If/Switch/AllowFailure/Sequential/LoopUntil/WorkingDirectory and the flow use it. A Dag witherrors:keeps itsdependsOngate (it used to ignore it).Loopiteration runs the Loop's ownerrors/finally. AWorkingDirectory's hooks run after the block, outside the directory (no block id, no snapshot access).spec::path::find_taskreplaces five walkers that missed lists. This also fixes two pre-existing gaps (aLooporWorkingDirectoryinside anerrors:list).Pausewithfinally:still plans as a Pause; publish and new starts refuse it naming Phase 3b-1 — Query Engine (async execution, REST polling) #9.task-runif-taskswithstatestays baselined (rendereremptytest, package Phase 0b — Frontend skeleton (dual-portal SPA, sparklabx design system) #5).Test plan
cargo test -p egret-workflow --no-fail-faston a private Postgres/Valkey before the final rebase: only the 7sql_starrocks_livetests fail (need a StarRocks engine, as on main). After the rebase onto fix(catalog): classifications follow a renamed table or column #757: lib 1821 pass, executor ratchet 61/61 pass.finally) is fixed and pinned.Deploy note
Stop old workers before deploying (a re-plan must not interleave with an old binary). Two edge cases are documented, not fixed: a malformed
finally:stored before this PR stalls an in-flight execution of that flow until it is killed, and a task id duplicated between a pre-existingfinally:and another task resolves to the first match for executions already running.Documentation
Spec, plan and roadmap notes are in this PR. The VI mirror of the new spec and plan lands with #2b (task B8). The platform user docs live in
egret-data/docs: a follow-up is needed (afinally:now runs;Pausefinally:is refused).Checklist
🤖 Generated with Claude Code