feat(#2772): branch on outcome in the graph, not inside the node
Splits what was one `Cancelled` outcome into two, because they were two
different facts wearing one name:
- `Skipped` — the node's own edges ruled it out. Expected; the failure
branch of a run that succeeded is `Skipped`. A parent's roll-up
**ignores** it.
- `Cancelled` — the work was dropped before it could start. Still
not-success for the roll-up, as before.
Without that split, branching on outcome defeats itself: exactly one
branch is always ruled out, `any_child_failed` counted it, and every DAG
containing a branch would have rolled up failed no matter how the run
went. Caught in review before it was written, not after.
`AFTER_ANY` becomes `{Done, Failed, Skipped}` — "anything except the work
being dropped". That is what it always meant; it only swept in
cancellation because cancellation wasn't distinguishable from
elimination. Audited every user rather than assuming, which is how the
one regression in my own proposal surfaced: `{Done, Failed}` would have
refused to run rebuild's recovery `Reconcile` after a failed `MetaSync`
(that eliminates `Prebuild`, so the tail's dep is `Skipped`, not
`Failed`) and left the container down.
With that, the templates stop computing outcomes and let the graph pick:
- `ResolveApproval { approval_id, outcome }` — one tail per outcome, each
edged to accept only its own, so exactly one is ever runnable.
- `EmitRebuilt { agent, ok }` — a pair. `ok` is not derived, it is which
of the two the graph let run.
Edges are conjunctive, so "any of these roots failed" is not directly
sayable. The composition: the success branch is `AFTER_OK` on every root
(so it is itself eliminated the moment one doesn't succeed), and the
failure branch keys off *that* elimination. The failure branch also
waits on every root — without it, a failed `Prebuild` eliminates the
success branch immediately and the failure would be announced while the
recovery `Reconcile` was still running. The tests caught that one.
Deletes, all of them #2770's host-side debt:
- `Claim.deps`, `DepOutcome`, `Claim::deps_state`, `Claim::deps_error`
and the dep-snapshotting loop in `claim_ready`. Executors read their
own variant now; nothing inspects anything.
- `NodeKind::is_tail()` and the `cancel` exemption built on it. Sparing
is derived from the edges: `cancel` keeps a node iff one of its edges
accepts `Cancelled`. An approval tail names it and survives to resolve
the row; `Reconcile` doesn't and is cancelled with the rest. My earlier
claim that this couldn't dissolve was only true while `AFTER_ANY`
accepted cancellation.
`resolve_approval_dag` / `deploy_terminal_tag` now take `TerminalState`
rather than the wire `State`, so both matches are exhaustive instead of
ending in a catch-all.
Skipped nodes are filtered off the wire alongside `Done` ones. That costs
some dashboard detail on a failed rebuild — which steps were skipped —
and the tests say so with a pointer to the follow-up. Surfacing them as
`Cancelled` instead would be worse: the client roll-up ranks `Cancelled`
above `Running`, so a successful DAG with a not-taken branch would read
as cancelled.
This commit is contained in:
parent
affedecaa5
commit
07078b76ef
8 changed files with 365 additions and 385 deletions
|
|
@ -507,20 +507,18 @@ async fn run_approval_schedule_prompt(
|
|||
/// so the work's terminal state is the authoritative outcome and there's no
|
||||
/// in-node resolution to skip around.
|
||||
///
|
||||
/// `state` / `error` come from the tail node's own dependency roll-up
|
||||
/// ([`Claim::deps_state`] / [`Claim::deps_error`]), so this runs on the success,
|
||||
/// failure **and cancel** paths alike.
|
||||
/// `outcome` is the one the calling node was built to report — a template emits
|
||||
/// one tail per outcome, so success, failure and cancel each arrive here from
|
||||
/// their own node rather than from one node branching.
|
||||
///
|
||||
/// [`NodeKind::ResolveApproval`]: crate::job_queue::NodeKind::ResolveApproval
|
||||
/// [`Claim::deps_state`]: crate::job_queue::Claim::deps_state
|
||||
/// [`Claim::deps_error`]: crate::job_queue::Claim::deps_error
|
||||
pub(crate) async fn resolve_approval_dag(
|
||||
coord: &Arc<Coordinator>,
|
||||
approval_id: i64,
|
||||
state: crate::job_queue::State,
|
||||
outcome: crate::job_queue::TerminalState,
|
||||
error: Option<&str>,
|
||||
) {
|
||||
use crate::job_queue::State;
|
||||
use crate::job_queue::TerminalState;
|
||||
let approval = match coord.approvals.get(approval_id) {
|
||||
Ok(Some(a)) => a,
|
||||
Ok(None) => {
|
||||
|
|
@ -532,10 +530,14 @@ pub(crate) async fn resolve_approval_dag(
|
|||
return;
|
||||
}
|
||||
};
|
||||
let result: Result<()> = match state {
|
||||
State::Done => Ok(()),
|
||||
State::Cancelled => Err(anyhow::anyhow!("cancelled before completion")),
|
||||
_ => Err(anyhow::anyhow!("{}", error.unwrap_or("job dag failed"))),
|
||||
let result: Result<()> = match outcome {
|
||||
TerminalState::Done => Ok(()),
|
||||
// `Skipped` never reaches here — no template emits a tail for it — but it
|
||||
// reads the same to an operator either way: the work did not happen.
|
||||
TerminalState::Cancelled | TerminalState::Skipped => {
|
||||
Err(anyhow::anyhow!("cancelled before completion"))
|
||||
}
|
||||
TerminalState::Failed => Err(anyhow::anyhow!("{}", error.unwrap_or("job dag failed"))),
|
||||
};
|
||||
let mut terminal_tag = None;
|
||||
match approval.kind {
|
||||
|
|
@ -551,7 +553,7 @@ pub(crate) async fn resolve_approval_dag(
|
|||
}
|
||||
}
|
||||
ApprovalKind::MergeConfigPr => {
|
||||
terminal_tag = deploy_terminal_tag(approval.agent.as_str(), approval_id, state).await;
|
||||
terminal_tag = deploy_terminal_tag(approval.agent.as_str(), approval_id, outcome).await;
|
||||
// On a failed deploy, surface the failing build log back onto the
|
||||
// PR so the manager sees why it was rejected without leaving the
|
||||
// forge. Posted here rather than inside a node because this is the
|
||||
|
|
@ -577,13 +579,14 @@ pub(crate) async fn resolve_approval_dag(
|
|||
async fn deploy_terminal_tag(
|
||||
agent: &str,
|
||||
approval_id: i64,
|
||||
state: crate::job_queue::State,
|
||||
outcome: crate::job_queue::TerminalState,
|
||||
) -> Option<String> {
|
||||
use crate::job_queue::State;
|
||||
let candidate = match state {
|
||||
State::Done => format!("deployed/{approval_id}"),
|
||||
State::Cancelled => return None,
|
||||
_ => format!("failed/{approval_id}"),
|
||||
use crate::job_queue::TerminalState;
|
||||
let candidate = match outcome {
|
||||
TerminalState::Done => format!("deployed/{approval_id}"),
|
||||
// Nothing ran, so nothing was planted.
|
||||
TerminalState::Cancelled | TerminalState::Skipped => return None,
|
||||
TerminalState::Failed => format!("failed/{approval_id}"),
|
||||
};
|
||||
lifecycle::git_rev_parse(&crate::paths::applied_dir(agent), &candidate)
|
||||
.await
|
||||
|
|
|
|||
Loading…
Reference in a new issue