From 40ca3861d1120addd409b114f86a47faafa482db Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 13 Jul 2026 21:42:42 +0200 Subject: [PATCH] spec(075-orchestration-node-control-parity): add specification Formalizes per-task run_timeout enforcement (spawned + RunInline) and Mode-1 terminal-failure recovery (state_injection substitute-and-continue) in zeph-orchestration, per a three-round architect/critic design review (final verdict: minor/approved). route_to reroute-to-alternate recovery (Mode 2) is explicitly deferred: its dependency-based dormancy mechanism was found to be inverted (the fallback would dispatch on the source task's success, not its failure) and requires a TaskStatus::Dormant or on-failure-edge redesign that is out of scope for this spec. Idle-timeout progress-signal plumbing is likewise deferred pending a non-evicting progress channel design. Follow-up implementation tracked in the linked issue. --- .../brd.md | 137 ++++++ .../nfr.md | 106 +++++ .../plan.md | 412 ++++++++++++++++++ .../spec.md | 381 ++++++++++++++++ .../srs.md | 356 +++++++++++++++ .../tasks.md | 148 +++++++ specs/MOC-specs.md | 2 + specs/README.md | 1 + 8 files changed, 1543 insertions(+) create mode 100644 specs/075-orchestration-node-control-parity/brd.md create mode 100644 specs/075-orchestration-node-control-parity/nfr.md create mode 100644 specs/075-orchestration-node-control-parity/plan.md create mode 100644 specs/075-orchestration-node-control-parity/spec.md create mode 100644 specs/075-orchestration-node-control-parity/srs.md create mode 100644 specs/075-orchestration-node-control-parity/tasks.md diff --git a/specs/075-orchestration-node-control-parity/brd.md b/specs/075-orchestration-node-control-parity/brd.md new file mode 100644 index 000000000..18e0ba84c --- /dev/null +++ b/specs/075-orchestration-node-control-parity/brd.md @@ -0,0 +1,137 @@ +--- +aliases: + - Orchestration Node Control Parity BRD + - Node Timeout / Retry-Exhausted Recovery BRD + - BRD 6021 +tags: + - sdd + - brd + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[specs/075-orchestration-node-control-parity/spec]]" + - "[[specs/075-orchestration-node-control-parity/srs]]" + - "[[specs/075-orchestration-node-control-parity/nfr]]" + - "[[009-orchestration/spec]]" +--- + +# BRD: Orchestration Node Control Parity — Per-Task Timeouts and Retry-Exhausted Recovery (GitHub #6021) + +## 1. Business Context + +`zeph-orchestration`'s `TaskGraph`/`DagScheduler` is this project's designated architectural +comparator to LangGraph (LangChain, Python) for the durable, checkpointed-DAG-execution +dimension — no other tracked reference agent covers it +(`.local/testing/playbooks/competitive-parity.md`). A prior research spec +(`.local/specs/059-orchestration-node-control-parity/spec.md`, 2026-07-11) identified two +LangGraph `add_node` capabilities Zeph lacks: per-node `TimeoutPolicy` (hard wall-clock vs. +idle/no-progress caps) and a node-level error handler that can recover a task after retries are +exhausted instead of collapsing straight to abort. This BRD formalizes the v1 slice of that gap +that a three-round architect/critic design review (final verdict: **minor / approved**, +`.local/handoff/2026-07-13T21-16-30-critic.md`) confirmed is implementation-ready. + +The review process itself materially narrowed scope: the initial design included a `route_to` +reroute-to-fallback-node recovery mode. Round-3 critique (`N5`) proved that mode's proposed safety +mechanism was **inverted** — a `depends_on == [failed_task]` fallback dispatches on the failed +task's *success*, not its failure, which is the exact opposite of a fallback. The architect +retracted that reasoning and dropped the reroute mode from v1 entirely (see §5, §8). + +## 2. Problem Statement + +Two gaps, both already documented by the prior research spec and re-confirmed against current +code (HEAD `d93d82e8`): + +1. **Timeout is a single global `Duration`, not a per-task override.** + `OrchestrationConfig.task_timeout_secs` (`crates/zeph-config/src/experiment.rs:274`, default + 300s) feeds exactly one `task_timeout: Duration` field on the scheduler, applied uniformly by + `check_timeouts()` (`crates/zeph-orchestration/src/scheduler/tick/mod.rs:727-768`) and by + `wait_event()`'s nearest-deadline computation (`tick/mod.rs:261-270`). `TaskNode` already + supports per-task overrides for `failure_strategy`, `max_retries`, and `token_budget_cents` + (`crates/zeph-orchestration/src/graph.rs:379-452`) — timeout is the one override the crate's + own established pattern is missing. +2. **Retry-exhausted failure always collapses to Abort-equivalent termination.** + `propagate_failure()` (`crates/zeph-orchestration/src/dag.rs:223-322`) implements + `FailureStrategy::Retry` by incrementing `retry_count` until `max_retries`, then falls through + to the same branch as `Abort` (`dag.rs:281-298`, comment: "Retry exhausted — treat as Abort"). + The only non-abort escape hatch, `FailureStrategy::Ask`, pauses the **entire graph** + (`GraphStatus::Paused`) for human intervention — there is no autonomous, programmatic recovery + path. + +## 3. Business Goals + +| ID | Goal | Priority | +|----|------|----------| +| BG-01 | An operator can override the graph-global task timeout on individual tasks, so heterogeneous DAG workloads (a fast classification subtask alongside a multi-minute code-generation subtask) do not share one ill-fitting timeout value | P1 | +| BG-02 | A task author can configure a node to substitute a synthetic output and continue (instead of aborting or pausing the whole graph) when that node's terminal failure is an `Abort`-default or retry-exhausted `Retry` outcome | P1 | +| BG-03 | The idle/no-progress timeout concept is defined and config-surfaced now (so the schema does not need a breaking change later) without pretending to be enforced before the progress-signal plumbing it depends on exists | P2 | +| BG-04 | Every new capability degrades to today's exact existing behavior when not configured — zero regression for graphs that opt into neither feature | P1 | +| BG-05 | The capability set is scoped to what the design review proved safe for v1: recovery is a single declarative "substitute output, keep going" mode, not a reroute-to-alternate-node mode (that mode is deferred pending a redesign — see §5) | P1 | + +## 4. Stakeholders + +| Role | Interest | +|------|----------| +| Operator running long DAG workflows with heterogeneous task durations | Wants per-task timeout control instead of one global value that is either too loose or too tight | +| Task/plan author designing a DAG with a legitimately-flaky node (e.g. a data-fetch task) | Wants an autonomous fallback path instead of a hard abort or a graph-wide pause on every transient exhaustion | +| Zeph maintainers | Want a minimal, spec-039-compliant, additive extension — no new crate, no new `tokio::spawn` site, zero behavior change for existing graphs | +| Future implementation session (`/rust-team` per this project's constraint that CI/spec sessions do not write source code) | Inherits a fully traceable, code-cited contract with no open architectural questions — the three-round review already resolved them | +| Future follow-up spec authors (Mode 2 redesign, Alt A idle-progress plumbing) | Inherit a precise, code-grounded problem statement for why those items were deferred, not just a "TODO" | + +## 5. Out of Scope + +| Item | Reason | +|------|--------| +| Mode 2 (`route_to` reroute-to-alternate-node recovery) | **N5 (root cause):** a naive `depends_on == [failed_task]` fallback-node design dispatches when the failed task *succeeds* (`ready_tasks()`'s `Pending` arm unblocks on `Completed`, `dag.rs:192-203`), not when it fails — the exact opposite of a fallback. No dependency-topology constraint can fix this; it requires a genuinely new mechanism (a `TaskStatus::Dormant` marker or an explicit on-failure edge). **N1:** reusing the existing Skip-BFS (`dag.rs:254-280`) to un-stick a revived fallback node would still leave that node's own downstream subtree permanently `Skipped`. **N3:** `build_task_prompt`'s `Completed`-only dependency filter (`crates/zeph-orchestration/src/scheduler/router.rs:23-34`) would silently drop a `Failed` source task's `state_injection`, so a rerouted fallback would receive zero context. All three require a real redesign, not a v1 fix — deferred to a follow-up issue | +| Idle-timeout progress-signal plumbing (Alt A: coalescing per-task `Arc`/`watch` progress timestamp) | No heartbeat/liveness/progress-signal mechanism exists anywhere in `zeph-subagent` or `zeph-orchestration` today (verified: zero hits). Building it is a distinct, cross-crate wiring effort (spawn path + `RunInline` loop instrumentation) — v1 defines the field and its target semantics but ships it as a documented no-op | +| Any change to `FailureStrategy::Abort`/`Skip`/`Ask` semantics for tasks that configure neither `timeout` nor `recovery` | Existing behavior for graphs that never opt in must be bit-for-bit unchanged (BG-04) | +| A resume-time re-scan for in-flight recovery | Verified unnecessary: the recovery mutation is synchronous (no `.await`) inside `propagate_failure()`, which runs inside the fully-synchronous `scheduler.tick()` (`crates/zeph-core/src/agent/scheduler_loop.rs:338`); `save_graph_snapshot()` runs later in the **same** loop iteration, gated on `take_graph_dirty()` (`scheduler_loop.rs:547-551`). A failure and its Mode-1 recovery always land in the same snapshot — no crash window exists where one persists without the other | +| Any change to the cascade-abort event-path ordering | The existing ordering (`handle_failed_outcome`, `crates/zeph-orchestration/src/scheduler/tick/mod.rs:590-681`: `Failed` → `record_outcome` → cascade checks that `return` early → `propagate_failure`) already makes cascade-abort take precedence over recovery with zero code reordering required | +| Cross-graph or cross-session recovery routing | Scope is intra-graph only | + +These deferrals are carried into `srs.md` as acknowledged-deferred requirements (FR-D-01, FR-D-02). + +## 6. Success Criteria + +| ID | Criterion | Measurable | +|----|-----------|-----------| +| SC-01 | Graphs that configure neither `timeout` nor `recovery` on any `TaskNode` behave identically to current behavior | Regression test suite covering `check_timeouts()`, `wait_event()`, and `propagate_failure()` passes unchanged | +| SC-02 | A per-task `run_timeout_secs` override supersedes the graph-global `task_timeout` for the task that declares it, for both spawned and `RunInline` tasks | Test: a short per-task override fires before the (longer) global default would have, on both dispatch paths | +| SC-03 | `idle_timeout_secs` is defined, serializable, and config-surfaced, but never fires in v1 | Test: a task configured with a short `idle_timeout_secs` and long-running (idle) execution is NOT flagged as timed out by that field; `--init` wizard text and config.toml comment both state "reserved — not yet enforced" | +| SC-04 | A node with `state_injection` configured, on `Abort`-default or retry-exhausted `Retry` terminal failure, transitions to `Completed` with the synthetic output, and dependents unblock and consume that output through the existing sanitizer | Test: single failing node with `state_injection` set — dependents receive the injected value through `build_task_prompt`, `graph.status` remains `Running` | +| SC-05 | A cascade-abort (fan-out or linear-chain) always takes precedence over recovery — recovery never fires once a cascade abort has triggered for the same event | Test: a node with `recovery` configured whose failure also trips the cascade-abort threshold ends the graph `Failed`, not recovered | +| SC-06 | `validate()` rejects a node that sets both `recovery` and `verify_predicate`, and warns (not rejects) when `recovery` is configured under `Skip`/`Ask` | Config/graph-construction validation tests for both cases | +| SC-07 | A recovered node is counted as `tasks_completed` (never `tasks_failed`) in `OrchestrationMetrics`, with no special-casing required | Verified against the existing status-derived counting in `finalize_plan_completed`/`finalize_plan_failed` (`crates/zeph-core/src/agent/plan.rs:722-833`) | +| SC-08 | The new `default_idle_timeout_secs` config field ships with full config.toml / `--init` / `--migrate-config` integration per this project's mandatory integration-point rule | `--init` wizard prompt, `--migrate-config` step, and config.toml documentation all present | + +## 7. Constraints + +- No new crate; all new code lands in `zeph-orchestration` (data model + scheduling logic) and + `zeph-config` (one new config field + validation + migration). +- Zero new `tokio::spawn()` call sites — recovery is a synchronous data mutation inside an + already-synchronous `tick()`; no async work, so no `*_provider` field is needed either (per this + project's multi-model design principle, which only applies to subsystems that call an LLM — + recovery does not). +- `TaskNode`'s new `timeout`/`recovery` fields follow the crate's existing + `Option`-with-graph-default override pattern (`failure_strategy`, `max_retries`, + `token_budget_cents`) and its existing `#[serde(default, skip_serializing_if = "Option::is_none")]` + forward-compatibility convention (`network_scope`, `asset_sensitivity`, + `crates/zeph-orchestration/src/graph.rs:441-451`). +- No code reordering in the existing cascade-abort event path — recovery slots into the existing + `propagate_failure()` call site unchanged. + +## 8. Dependencies + +| Dependency | Type | Notes | +|------------|------|-------| +| `TaskNode` (`crates/zeph-orchestration/src/graph.rs:379-452`) | Internal | New `timeout: Option` and `recovery: Option` fields, following the existing per-task override precedent | +| `check_timeouts()` / `wait_event()` (`crates/zeph-orchestration/src/scheduler/tick/mod.rs:727-768`, `:254-270`) | Internal | Extended to compute a per-task effective run-timeout instead of the single global `task_timeout` | +| `RunInline` inline `tokio::select!` (`crates/zeph-core/src/agent/scheduler_loop.rs:258-`) | Internal | The only structurally viable enforcement site for `RunInline` tasks, since the tick loop is blocked for the task's whole duration | +| `propagate_failure()` (`crates/zeph-orchestration/src/dag.rs:223-322`) | Internal | Gains the Mode-1 recovery branch on terminal `Abort`/retry-exhausted `Retry` | +| `validate()` (`crates/zeph-orchestration/src/dag.rs:37-91`) | Internal | Gains the recovery/predicate reject guard and the recovery/Skip-Ask warn guard | +| `handle_failed_outcome()` cascade-abort event path (`crates/zeph-orchestration/src/scheduler/tick/mod.rs:590-681`) | Internal | Read-only dependency — its existing ordering is the precedence mechanism; unchanged | +| `build_task_prompt()` (`crates/zeph-orchestration/src/scheduler/router.rs:18-59`) | Internal | Existing `Completed`-only dependency filter + SEC-ORCH-01 sanitizer; unmodified, consumes the synthetic recovery output as-is | +| `OrchestrationConfig` (`crates/zeph-config/src/experiment.rs:261-`) | Internal | New `default_idle_timeout_secs: Option` field; reuses existing `task_timeout_secs` as the run-timeout global default | +| `MIGRATIONS` registry (`crates/zeph-config/src/migrate/mod.rs:646-`) | Internal | New migration step adds the field with a `None` default | +| `step_orchestration()` (`src/init/agents.rs:11`) | Internal | `--init` wizard integration point | +| `finalize_plan_completed`/`finalize_plan_failed` (`crates/zeph-core/src/agent/plan.rs:722-833`) | Internal | Read-only confirmation that metrics are status-derived at graph finalization, not event-incremented — resolves the recovered-node metrics question with no code change needed | diff --git a/specs/075-orchestration-node-control-parity/nfr.md b/specs/075-orchestration-node-control-parity/nfr.md new file mode 100644 index 000000000..cff9a4da2 --- /dev/null +++ b/specs/075-orchestration-node-control-parity/nfr.md @@ -0,0 +1,106 @@ +--- +aliases: + - Orchestration Node Control Parity NFR + - Node Timeout / Retry-Exhausted Recovery NFR + - NFR 6021 +tags: + - sdd + - nfr + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[specs/075-orchestration-node-control-parity/brd]]" + - "[[specs/075-orchestration-node-control-parity/srs]]" + - "[[specs/075-orchestration-node-control-parity/spec]]" + - "[[039-background-task-supervisor/spec]]" +--- + +# NFR: Orchestration Node Control Parity — Per-Task Timeouts and Retry-Exhausted Recovery (GitHub #6021) + +ISO/IEC 25010:2011 quality model. + +--- + +## Performance Efficiency + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-PE-01 | `check_timeouts()` per-task effective-timeout lookup adds no complexity class | Remains `O(self.running)` — the per-task override lookup is an `Option`/`map`/`unwrap_or` chain on data already held by the loop, not a graph traversal (SRS FR-002) | +| NFR-PE-02 | `wait_event()` nearest-deadline computation adds no complexity class | Remains `O(self.running)` — per-task effective timeout replaces the single global value inside the existing `.map(...).min()` chain, no additional pass over `self.graph.tasks` (SRS FR-003) | +| NFR-PE-03 | Recovery mutation cost | `O(1)` — a status flip and a `TaskResult` construction inside `propagate_failure()`, no additional graph traversal beyond what the function already performs (SRS FR-007) | +| NFR-PE-04 | `validate()` guard cost | `O(1)` per task, added to the existing per-task loop (`dag.rs:51-77`) — no new pass over `tasks` (SRS FR-011, FR-012) | + +--- + +## Reliability + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-RE-01 | Zero regression for graphs using neither feature | A graph where every `TaskNode.timeout` and `TaskNode.recovery` is `None` produces byte-identical scheduling/failure behavior to pre-feature code — verified by a regression test covering `check_timeouts()`, `wait_event()`, and `propagate_failure()` (BRD SC-01) | +| NFR-RE-02 | `Skip`/`Ask` semantics are unchanged | Recovery is additive and scoped to the `Abort`-default/retry-exhausted-`Retry` branches only; a node with `recovery` configured under `Skip`/`Ask` is inert (warned, not enforced) — the `Skip`/`Ask` code paths themselves are untouched (SRS FR-012) | +| NFR-RE-03 | Cascade-abort precedence never regresses to recovery-first | Recovery is reachable only through `propagate_failure()`, which the existing cascade-check `return`s bypass entirely on a cascade trip — no new code path allows recovery to preempt a cascade abort (SRS FR-013) | +| NFR-RE-04 | No new panic path | `TimeoutPolicy`/`RecoveryAction` construction and the `validate()` guards are `Option`/`Result`-typed throughout; no `unwrap()`/`expect()` introduced on a value that can legitimately be absent | +| NFR-RE-05 | No crash-recovery window where a failure persists without its Mode-1 recovery | Same-tick snapshot atomicity (SRS FR-016) — verified against `scheduler_loop.rs:338,547-551` | + +--- + +## Durability / Crash-Resume + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-DU-01 | New `TaskNode` fields round-trip through the existing SQLite/journal persistence path | `#[serde(default, skip_serializing_if = "Option::is_none")]` on both `timeout` and `recovery` — a graph persisted before this feature existed deserializes with both fields `None`, no data loss, no migration required for the graph-data fields (SRS FR-019) | +| NFR-DU-02 | No new resume-time logic | Explicitly not added — the same-tick snapshot atomicity guarantee (NFR-RE-05) makes it unnecessary; resume rebuilds `running` from persisted `TaskStatus::Running` entries exactly as it does today (`crates/zeph-orchestration/src/scheduler/mod.rs:389-430`), unmodified by this feature | +| NFR-DU-03 | New config field forward/backward compatible | `default_idle_timeout_secs: Option` with `#[serde(default)]` and a dedicated `MIGRATIONS` step deserializes to `None` for configs written before this feature existed (SRS FR-018) | + +--- + +## Async Supervision (spec-039 Compliance) + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-AS-01 | Zero new `tokio::spawn()` call sites | Recovery is a synchronous data mutation inside the already-synchronous `propagate_failure()`/`scheduler.tick()` call chain; timeout enforcement reuses the existing `tokio::time::timeout` pattern already present on the `RunInline` `select!` (adding a branch, not a new spawn) and the existing `check_timeouts()`/`wait_event()` polling loop. Per `[[039-background-task-supervisor/spec]]`'s binding NEVER section, no new detached task is created | +| NFR-AS-02 | No lock held across `.await` | Neither the timeout-override lookup nor the recovery mutation introduces any lock (`parking_lot` or otherwise) — both operate on data already owned by `&mut self`/`&mut TaskGraph` inside a synchronous call | +| NFR-AS-03 | No `*_provider` field required | Recovery performs no LLM call — `state_injection` is a planner-authored literal string, not a generated value. This project's multi-model design principle (every subsystem that calls an LLM must expose a `*_provider` field) does not apply because no LLM call exists on this path | + +--- + +## Observability + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-OB-01 | Timeout-cause disambiguation | When a task times out, the log/trace record names which mechanism fired (per-task `run_timeout_secs` override vs. graph-global `task_timeout` fallback vs., in a future Alt A build, `idle_timeout_secs`) — never an aggregate/ambiguous flag | +| NFR-OB-02 | Recovery invocation is traced | Every Mode-1 recovery application (`propagate_failure()`'s new branch) is wrapped in or logged via `tracing::info_span!`/`tracing::warn!` following the `..` naming convention (e.g. `orchestration.dag.recover_task`), consistent with this project's instrumentation requirement | +| NFR-OB-03 | `validate()` guard failures name the exact defect | The FR-011 reject error names the offending task index/id and both conflicting fields; the FR-012 warn names the task and its effective failure strategy — never a generic validation failure | +| NFR-OB-04 | Idle-timeout no-op is loudly surfaced, not silent | `--init` wizard help text and `config.toml` comment both state "reserved — not yet enforced (see follow-up)" for `idle_timeout_secs` (per-task) and `default_idle_timeout_secs` (global) — critic finding M-b (SRS FR-005, FR-018) | + +--- + +## Maintainability + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-MA-01 | `TimeoutPolicy`/`RecoveryAction` follow the existing per-task override pattern | Structurally consistent with `failure_strategy: Option` and `max_retries: Option` already on `TaskNode` — no new override idiom introduced | +| NFR-MA-02 | `RecoveryAction` is additively extensible | `route_to` (Mode 2, deferred) can be added later as an additional `#[serde(default)]` field on the same struct without a breaking schema change or a new type | +| NFR-MA-03 | All new `pub` items carry doc comments | Per CLAUDE.md's rustdoc requirements; `RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps -p zeph-orchestration -p zeph-config` passes clean | +| NFR-MA-04 | Load-bearing bypass is documented in code, not only in this spec | The `ready_tasks()` `Ready`-arm dependency-completion bypass (`dag.rs:185-191`) gains a doc comment explaining its role in the recovery unblock path (SRS FR-020), so a future refactor does not silently break recovery semantics | + +--- + +## Compatibility / Scope Boundary + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-CO-01 | Default (all-`None`) behavior is byte-for-byte unchanged | Verified by NFR-RE-01's regression test (BRD SC-01) | +| NFR-CO-02 | `FailureStrategy` enum and its `Abort`/`Skip`/`Ask` arms are unchanged | Only the `Abort` arm and the retry-exhausted branch of the `Retry` arm gain a conditional recovery check before falling through to their existing behavior; `Skip` and `Ask` arms are not touched | +| NFR-CO-03 | No scheduler/subagent-spawn architecture change | This feature is scoped entirely to `TaskNode` data, `dag.rs` failure-propagation logic, `tick/mod.rs` timeout evaluation, one `RunInline` `select!` branch, and `validate()` — `DagScheduler`'s dispatch/spawn machinery, `zeph-subagent` grants, and transcripts are untouched | +| NFR-CO-04 | Mode 2 and Alt A remain schema-compatible follow-ups | Deferring them does not require a breaking change to ship later — `RecoveryAction.route_to` and `TimeoutPolicy`/`OrchestrationConfig`'s idle-timeout enforcement are additive when implemented | + +--- + +## Usability + +| ID | Requirement | Target | +|----|-------------|--------| +| NFR-US-01 | `validate()` errors and warnings are actionable | Name the specific task, the specific conflicting/inert configuration, and (for the reject case) which fields must not co-occur | +| NFR-US-02 | `--init` wizard framing for `default_idle_timeout_secs` | Prompt text states the field is not yet enforced before accepting a value, preventing an operator from assuming idle-based kills are active (NFR-OB-04) | diff --git a/specs/075-orchestration-node-control-parity/plan.md b/specs/075-orchestration-node-control-parity/plan.md new file mode 100644 index 000000000..d126791bf --- /dev/null +++ b/specs/075-orchestration-node-control-parity/plan.md @@ -0,0 +1,412 @@ +--- +aliases: + - Orchestration Node Control Parity Plan + - Node Timeout / Retry-Exhausted Recovery Plan + - Plan 6021 +tags: + - sdd + - plan + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[specs/075-orchestration-node-control-parity/spec]]" + - "[[specs/075-orchestration-node-control-parity/tasks]]" +--- + +# Implementation Plan: Orchestration Node Control Parity (GitHub #6021) + +Source of truth: architect handoffs `.local/handoff/2026-07-13T20-47-55-architect.md` (base), +`.local/handoff/2026-07-13T21-00-15-architect.md`/`21-09-12-architect.md` (round-2 revisions), +`.local/handoff/2026-07-13T21-12-06-architect.md` (v3, final); critic-approved +`.local/handoff/2026-07-13T21-16-30-critic.md` (verdict **minor / approved**). This plan sequences +the change set from `[[specs/075-orchestration-node-control-parity/spec]]` §3 into an implementable +order. No architectural re-derivation — this is a formalization of the already-approved design. + +**Decision Type:** `refactoring` (additive extension of an existing subsystem; no new crate). +**Structure:** `workspace` (existing); new code in `zeph-orchestration` (data model + `dag.rs` + +`tick/mod.rs` logic) and `zeph-config` (one new field + validation + migration); one new branch +in `zeph-core`'s `RunInline` `select!`. No cross-crate dependency added. + +## Recommended Implementation Order + +**Phase 1: `zeph-orchestration` — data model.** `TimeoutPolicy`/`RecoveryAction` types and the +`TaskNode.timeout`/`.recovery` fields. Implement first — every later phase reads these fields. + +**Phase 2: `zeph-orchestration` — `validate()` guards.** Depends on Phase 1's field existing; +independently unit-testable with no scheduler dependency. + +**Phase 3: `zeph-orchestration` — recovery in `propagate_failure()`.** Depends on Phase 1; the +core Mode-1 behavior, fully unit-testable against `TaskGraph`/`dag.rs` in isolation (no async, no +scheduler tick required). + +**Phase 4: `zeph-orchestration` — per-task timeout in `check_timeouts()`/`wait_event()`.** Depends +on Phase 1; independent of Phase 3. + +**Phase 5: `zeph-core` — `RunInline` timeout branch.** Depends on Phase 1 (reads +`TimeoutPolicy.run_timeout_secs`); the only cross-crate touch point besides config. + +**Phase 6: `zeph-config` — `default_idle_timeout_secs` + integration.** Independent of Phases +2-5; can be implemented in parallel with them once Phase 1 is merged (or even before, since it +does not depend on `TaskNode`'s new fields). + +**Phase 7: Documentation, doc-comment annotations, testing playbook, CHANGELOG.** Implement +last; lowest risk, most mechanical. + +--- + +## Phase 1: Data Model + +### P1-1: `TimeoutPolicy` and `RecoveryAction` types + +**File:** `crates/zeph-orchestration/src/graph.rs` (co-located with `TaskNode`, or a new +`crates/zeph-orchestration/src/timeout_policy.rs` / `recovery.rs` module if `graph.rs` is judged +too large already — developer's call, consistent with existing module-splitting conventions in +the crate) + +```rust +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct TimeoutPolicy { + pub run_timeout_secs: Option, + pub idle_timeout_secs: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RecoveryAction { + pub state_injection: Option, +} +``` + +Both `#[derive(Debug, Clone, Serialize, Deserialize)]`, both documented with `///` doc comments +per CLAUDE.md rustdoc rules, `idle_timeout_secs`'s doc comment explicitly states "not enforced in +v1 — reserved for a future progress-signal mechanism" (FR-005). + +### P1-2: `TaskNode` fields + +**File:** `crates/zeph-orchestration/src/graph.rs:379-452` + +```rust +#[serde(default, skip_serializing_if = "Option::is_none")] +pub timeout: Option, +#[serde(default, skip_serializing_if = "Option::is_none")] +pub recovery: Option, +``` + +Insert after the existing `asset_sensitivity` field, following the same doc-comment style. +Update the module-level doctest at the top of `graph.rs` (the existing `TaskNode::new(...)` +example) if it asserts on the full field list — add `assert!(node.timeout.is_none())` / +`assert!(node.recovery.is_none())` alongside the existing `network_scope`/`asset_sensitivity` +assertions if that pattern is present. + +### P1-3: Unit tests (Phase 1) + +- Serde round-trip: a `TaskNode` with `timeout`/`recovery` both `Some(...)` round-trips through + JSON; a `TaskNode` with both `None` serializes without the fields present at all + (`skip_serializing_if`). +- Deserialize a pre-feature-shaped JSON blob (no `timeout`/`recovery` keys) — both fields default + to `None` (`#[serde(default)]`). + +**Phase 1 gate:** `cargo nextest run -p zeph-orchestration` green (graph module) before Phase 2/3. + +--- + +## Phase 2: `validate()` Recovery Guards + +### P2-1: Reject `recovery` + `verify_predicate` + +**File:** `crates/zeph-orchestration/src/dag.rs:37-91` (`validate()`), inside the existing +per-task loop at `:51-77` + +```rust +if task.recovery.is_some() && task.verify_predicate.is_some() { + return Err(OrchestrationError::InvalidGraph(format!( + "task {i} sets both recovery and verify_predicate — a predicate-gated task \ + must not be recovery-eligible" + ))); +} +``` + +### P2-2: Warn on `recovery` under `Skip`/`Ask` + +Same loop. Compute the task's effective failure strategy +(`task.failure_strategy.unwrap_or(graph.default_failure_strategy)` — note `validate()` takes +`tasks: &[TaskNode]`, not `&TaskGraph`, so the effective-strategy computation needs the graph's +`default_failure_strategy` threaded in as a parameter, or the guard is added as a second pass +that takes `&TaskGraph` — developer's call on the cleanest signature, but the check must run at +the same validation boundary): + +```rust +if task.recovery.is_some() + && matches!(effective_strategy, FailureStrategy::Skip | FailureStrategy::Ask) +{ + tracing::warn!( + task_index = i, + strategy = ?effective_strategy, + "recovery configured but effective failure strategy is Skip/Ask — recovery is inert" + ); +} +``` + +### P2-3: Unit tests (Phase 2) + +- `recovery.is_some() && verify_predicate.is_some()` → `Err(InvalidGraph)`. +- `recovery.is_some()` alone → `Ok`. +- `recovery.is_some()` with effective strategy `Skip` → `Ok` (warns, does not reject) — assert on + the warning via `tracing`'s test-capture mechanism if the crate has one, else assert only on + `Ok` and treat the warning as a manual/live-test verification item. +- `recovery.is_some()` with effective strategy `Ask` → same as `Skip`. +- `recovery.is_some()` with effective strategy `Abort`/`Retry` → `Ok`, no warning. + +**Phase 2 gate:** `cargo nextest run -p zeph-orchestration` green (dag module) before merge. + +--- + +## Phase 3: Mode-1 Recovery in `propagate_failure()` + +### P3-1: Recovery branch + +**File:** `crates/zeph-orchestration/src/dag.rs:223-322` (`propagate_failure()`) + +Attach the recovery check at the top of the `FailureStrategy::Abort` arm (`:243-253`) and at the +retry-exhausted fallthrough inside the `FailureStrategy::Retry` arm (`:281-298`) — both currently +converge on the same "mark graph Failed, collect Running tasks to cancel" shape, so the cleanest +implementation is a small shared helper: + +```rust +fn try_recover(graph: &mut TaskGraph, failed_id: TaskId) -> bool { + let Some(injection) = graph.tasks[failed_id.index()] + .recovery + .as_ref() + .and_then(|r| r.state_injection.clone()) + else { + return false; + }; + let node = &mut graph.tasks[failed_id.index()]; + node.status = TaskStatus::Completed; + node.result = Some(TaskResult { + output: injection, + artifacts: Vec::new(), + duration_ms: 0, + agent_id: None, + agent_def: Some("__recovery__".to_string()), + }); + tracing::info!(task_id = %failed_id, "orchestration.dag.recover_task: Mode-1 recovery applied"); + true +} +``` + +Call `try_recover(graph, failed_id)` at the top of both the `Abort` arm and the retry-exhausted +branch of the `Retry` arm; on `true`, `return Vec::new()` (no tasks to cancel — the node +recovered, `graph.status` is untouched, so no `Running` task needs cancellation as a side effect +of this specific node's failure). On `false`, fall through to the existing behavior unchanged. + +### P3-2: Unit tests (Phase 3) + +- `Abort`-default failure, `recovery.state_injection = Some(v)` configured → node ends + `Completed`, `result.output == v`, `graph.status` unchanged (still `Running` if it was). +- Retry-exhausted `Retry` failure, `recovery.state_injection = Some(v)` configured → same + end-state as above. +- Either case with `recovery == None` → existing Abort-equivalent behavior, byte-identical to + pre-feature (regression test, BRD SC-01). +- Recovery + a dependent task: dependent's `depends_on` includes the recovered task; after + recovery, `ready_tasks()` includes the dependent (`Pending`→ eligible via the `Pending` arm's + `depends_on` completion check). +- `Skip`/`Ask` arms are never affected — assert `try_recover` is not called from those arms + (structural/code-review-level check, or an integration test asserting a `Skip`-strategy node + with `recovery` configured still ends `Skipped`, not `Completed`). + +**Phase 3 gate:** `cargo nextest run -p zeph-orchestration` green (dag module) before Phase 7. + +--- + +## Phase 4: Per-Task Timeout — Spawned Tasks + +### P4-1: `check_timeouts()` effective timeout + +**File:** `crates/zeph-orchestration/src/scheduler/tick/mod.rs:727-768` + +```rust +fn effective_run_timeout(&self, task_id: TaskId) -> Duration { + self.graph.tasks[task_id.index()] + .timeout + .as_ref() + .and_then(|t| t.run_timeout_secs) + .map(Duration::from_secs) + .unwrap_or(self.task_timeout) +} +``` + +Replace the existing `r.started_at.elapsed() > self.task_timeout` filter predicate (inside the +`self.running.iter().filter(...)` closure) with `r.started_at.elapsed() > +self.effective_run_timeout(*id)`. + +### P4-2: `wait_event()` per-task nearest-deadline + +**File:** `crates/zeph-orchestration/src/scheduler/tick/mod.rs:254-270` + +Replace the `self.task_timeout.checked_sub(r.started_at.elapsed())` inside the `.map(...)` closure +(`:264-268`) with `self.effective_run_timeout(id).checked_sub(r.started_at.elapsed())` — note this +requires iterating `self.running` as `(id, r)` pairs rather than `.values()` alone, since +`effective_run_timeout` needs the `TaskId` to look up the per-task override. + +### P4-3: Unit tests (Phase 4) + +- Two running tasks, one with a short `run_timeout_secs` override, one with none — only the + overridden task times out at the short interval; the other respects the (longer) global + default. +- `wait_event()`'s computed `wait_duration` reflects the nearer of the two effective deadlines, + not the uniform global one. +- Regression: no per-task overrides configured anywhere → identical timing behavior to + pre-feature code (BRD SC-01). + +**Phase 4 gate:** `cargo nextest run -p zeph-orchestration` green (tick module) before Phase 7. + +--- + +## Phase 5: Per-Task Timeout — `RunInline` Tasks + +### P5-1: Third `tokio::select!` branch + +**File:** `crates/zeph-core/src/agent/scheduler_loop.rs:258-` + +```rust +let effective_run_timeout = task.timeout.as_ref() + .and_then(|t| t.run_timeout_secs) + .map(Duration::from_secs) + .unwrap_or(self.services.orchestration.orchestration_config.task_timeout_secs_as_duration()); + // exact accessor name/shape for the graph-global fallback is an implementation + // detail — mirror however task_timeout is currently threaded into this scope + +let outcome = tokio::select! { + result = self.run_inline_tool_loop(&prompt, max_iter) => { /* existing arm, unchanged */ } + () = cancel_token.cancelled() => { /* existing arm, unchanged */ } + () = tokio::time::sleep(effective_run_timeout) => { + zeph_orchestration::TaskOutcome::Failed { + error: format!("RunInline task exceeded run_timeout ({effective_run_timeout:?})"), + } + } +}; +``` + +(`tokio::time::sleep` inside `select!` is equivalent to `tokio::time::timeout` wrapping the whole +arm set here, and avoids restructuring the other two arms — developer's call on which idiom reads +cleaner in context; both satisfy FR-004.) + +### P5-2: Unit/integration tests (Phase 5) + +- A `RunInline` task with a short `run_timeout_secs` override and a tool loop that would run + longer → the timeout branch fires, produces `TaskOutcome::Failed`, and downstream handling + (`propagate_failure`, recovery if configured) proceeds identically to a spawned-task timeout. +- A `RunInline` task with no override and a fast-completing tool loop → completes normally, + timeout branch never fires (regression, BRD SC-01). +- Integration test combining Phase 3 + Phase 5: a `RunInline` task with both `timeout` and + `recovery` configured — timeout fires, recovery applies, dependents unblock. + +**Phase 5 gate:** `cargo nextest run -p zeph-core --lib` (scheduler_loop tests) green before Phase 7. + +--- + +## Phase 6: Config — `default_idle_timeout_secs` + +### P6-1: Config field + +**File:** `crates/zeph-config/src/experiment.rs`, sibling to `task_timeout_secs` (`:274`) + +```rust +/// Global default idle/no-progress timeout in seconds. RESERVED — not yet enforced; +/// see the orchestration-node-control-parity spec's Alt A follow-up. `None` = off. +#[serde(default)] +pub default_idle_timeout_secs: Option, +``` + +### P6-2: Migration step + +**File:** `crates/zeph-config/src/migrate/mod.rs` (new step function, likely in +`crates/zeph-config/src/migrate/steps.rs` alongside other named steps per the existing pattern), +registered in the `MIGRATIONS` vec (`:646-`) + +Add-with-default step: existing configs gain `default_idle_timeout_secs = None` (i.e., the key is +simply absent — `#[serde(default)]` already handles this on load; the migration step exists +mainly to be an explicit, documented, named entry in the registry consistent with this project's +"every new config field gets a migration step" convention) — mirror the shape of a prior +similarly-trivial add-only migration (e.g. `MigrateOrchestrationAssetSensitivity`) rather than +inventing a new migration idiom. + +### P6-3: `--init` wizard + +**File:** `src/init/agents.rs:11` (`step_orchestration()`) + +Add a prompt, framed as reserved/not-yet-enforced (FR-005/NFR-OB-04): + +> "Idle-timeout (no-progress) detection — reserved for a future release, not yet enforced. Leave +> unset unless you want the value persisted for when this ships. [blank/skip default]" + +### P6-4: `config.toml` documentation + +Document the field in `docs/src/` (per branching.md's PR checklist) and inline in any +`config.toml` example/template file the project ships, with the same "reserved — not yet +enforced" wording. + +### P6-5: Unit tests (Phase 6) + +- Default config: `default_idle_timeout_secs == None`. +- TOML round-trip: explicit value set → round-trips correctly. +- Migration test: a pre-feature config (no key present) migrates to `default_idle_timeout_secs == + None` with the new step recorded as a no-op/trivial change in the migration diff. + +**Phase 6 gate:** `cargo nextest run -p zeph-config` green before Phase 7. + +--- + +## Phase 7: Documentation and Mandatory Integration Points + +### P7-1: `ready_tasks()` doc annotation + +**File:** `crates/zeph-orchestration/src/dag.rs:185-191` (the `Ready` arm) + +Add a doc comment (or extend the existing one) stating this arm's predicate-only bypass (no +`depends_on` re-check) is load-bearing for the recovery unblock path, per SRS FR-020's exact +wording. + +### P7-2: Recovered-node metrics note + +No code change required (FR-021 is resolved by existing status-derived counting) — add a short +doc comment on `OrchestrationMetrics.tasks_completed`/`tasks_failed` +(`crates/zeph-core/src/metrics.rs:104-105`) or on `finalize_plan_completed`/`finalize_plan_failed` +(`crates/zeph-core/src/agent/plan.rs:722,801`) noting that a Mode-1-recovered node is correctly +counted as completed by construction (final-status-derived counting), so future contributors do +not "fix" this into an event-time increment that would double-count or miscount it. + +### P7-3: Mandatory integration points checklist + +| # | Point | Where | +|---|-------|-------| +| 1 | `config.toml` section | `[orchestration]` gains `default_idle_timeout_secs` — documented in `docs/src/` (P6-4) | +| 2 | CLI subcommand/argument | N/A — passive config default, no dedicated CLI surface, consistent with `task_timeout_secs` precedent | +| 3 | TUI command palette / `/` command | N/A for the config field (same rationale as #2); no background/implicit operation is introduced by this feature that would need a TUI status spinner — recovery is a synchronous data mutation, not a background operation | +| 4 | `--init` wizard | New reserved-field prompt in `step_orchestration()` (P6-3) | +| 5 | `--migrate-config` | New named step (P6-2) | +| 6 | Testing playbook | Create `/Users/rabax/Dev/zeph/.local/testing/playbooks/orchestration-node-control-parity.md` (main-repo path) — scenarios: default-off regression, per-task run_timeout (spawned + RunInline), idle-timeout no-op verification, Mode-1 recovery (Abort + retry-exhausted), cascade-precedence, validate() reject/warn, metrics classification | +| 7 | Coverage status | Add rows in `/Users/rabax/Dev/zeph/.local/testing/coverage-status.md` for: per-task timeout override, RunInline timeout branch, Mode-1 recovery, validate() guards, `default_idle_timeout_secs` config — status `Untested` | + +### P7-4: CHANGELOG.md + +Add an `[Unreleased]` entry describing the per-task timeout override and Mode-1 recovery +capability, noting the idle-timeout field is reserved/not-yet-enforced and that Mode 2 is +deferred to a follow-up issue. + +--- + +## Pre-Merge Checklist + +- [ ] `cargo +nightly fmt --check` +- [ ] `cargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warnings` +- [ ] `cargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins` +- [ ] `RUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"` +- [ ] `cargo test --doc --workspace --features "desktop,ide,server,chat,pdf,scheduler"` +- [ ] Async-supervision scan (`.claude/rules/continuous-improvement.md`) confirms zero new `tokio::spawn()` sites +- [ ] `CHANGELOG.md` updated (`[Unreleased]`) +- [ ] `.local/testing/playbooks/orchestration-node-control-parity.md` created (main-repo path) +- [ ] `.local/testing/coverage-status.md` rows added (main-repo path) +- [ ] LLM serialization gate: N/A — no LLM request/response serialization path is touched by this feature (recovery injects a planner-authored literal, not an LLM-generated value); confirm and record in the PR description +- [ ] `specs/README.md` and `specs/MOC-specs.md` register `orchestration-node-control-parity` (team-lead: outside this spec package's write scope — see handoff) diff --git a/specs/075-orchestration-node-control-parity/spec.md b/specs/075-orchestration-node-control-parity/spec.md new file mode 100644 index 000000000..9fd36aedc --- /dev/null +++ b/specs/075-orchestration-node-control-parity/spec.md @@ -0,0 +1,381 @@ +--- +aliases: + - Orchestration Node Control Parity + - Node Timeout / Retry-Exhausted Recovery + - Spec 6021 +tags: + - sdd + - spec + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[MOC-specs]]" + - "[[constitution]]" + - "[[specs/075-orchestration-node-control-parity/brd]]" + - "[[specs/075-orchestration-node-control-parity/srs]]" + - "[[specs/075-orchestration-node-control-parity/nfr]]" + - "[[specs/075-orchestration-node-control-parity/plan]]" + - "[[001-system-invariants/spec]]" + - "[[009-orchestration/spec]]" + - "[[039-background-task-supervisor/spec]]" +issues: + - "#6021" +--- + +# Spec: Orchestration Node Control Parity — Per-Task Timeouts and Retry-Exhausted Recovery Routing (GitHub #6021) + +> [!info] +> `TaskNode` gains an optional per-task `TimeoutPolicy` (hard `run_timeout_secs` override, +> enforced; `idle_timeout_secs`, defined but a documented no-op in v1) and an optional +> `RecoveryAction` (`state_injection` — substitute a synthetic output and continue, on terminal +> `Abort`-default or retry-exhausted `Retry` failure). No new crate, no new `tokio::spawn` site, +> zero behavior change for graphs that configure neither field. This spec is the authoritative +> implementation contract, derived from a three-round architect/critic design review (final +> critic verdict: **minor / approved**, `.local/handoff/2026-07-13T21-16-30-critic.md`). It +> formalizes that design into traceable requirements; it does not re-derive the architecture. +> Supersedes `.local/specs/059-orchestration-node-control-parity/spec.md` (2026-07-11 research +> draft), whose FR-001..FR-010/NFR-001..NFR-006/SC-001..SC-004 baseline is carried forward and +> narrowed to what the design review proved safe for v1 — most significantly, the research +> draft's `route_to` reroute mode is dropped from v1 entirely (§7). + +## Sources + +### External +- LangGraph (LangChain, Python) v1.2.x, current 1.2.9 (2026-07-10) — `TimeoutPolicy(run_timeout=..., + idle_timeout=...)` on `add_node`; node-level error handler returning a `Command` after retry + exhaustion. Source material for the original parity finding. + +### Internal +| File | Contents | +|---|---| +| `crates/zeph-orchestration/src/graph.rs:379-452` | `TaskNode` struct; existing per-task override precedent (`failure_strategy`, `max_retries`, `token_budget_cents`) and existing `#[serde(default, skip_serializing_if = "Option::is_none")]` forward-compat pattern (`network_scope`, `asset_sensitivity`) — the new `timeout`/`recovery` fields follow both | +| `crates/zeph-orchestration/src/dag.rs:37-91` | `validate()` — structural DAG validation; new recovery guards join the existing per-task loop at `:51-77` | +| `crates/zeph-orchestration/src/dag.rs:179-208` | `ready_tasks()` — `Ready` arm (`:185-191`, predicate-only, no `depends_on` re-check) and `Pending` arm (`:192-203`, `depends_on` completion check) — the `Pending` arm is how a recovered node's dependents unblock | +| `crates/zeph-orchestration/src/dag.rs:223-322` | `propagate_failure()` — `Abort` arm (`:243-253`), `Skip` arm (`:254-280`), `Retry` arm with retry-exhausted fallthrough (`:281-298`), `Ask` arm (`:299-303`), non-exhaustive wildcard arm (`:308-320`, dead code today — no other `FailureStrategy` variant exists, logs+defaults to Abort-equivalent for a future variant) — the new recovery branch attaches to the `Abort` arm and the retry-exhausted fallthrough only | +| `crates/zeph-orchestration/src/scheduler/tick/mod.rs:590-681` | `handle_failed_outcome()` — event-path failure handling: `Failed` status set (`:599`), `record_outcome` (`:601-602`), fan-out cascade check (`:629-647`), linear-chain cascade check (`:649-662`), `propagate_failure()` call (`:664`) — the existing ordering is the cascade-vs-recovery precedence mechanism | +| `crates/zeph-orchestration/src/scheduler/tick/mod.rs:690-` | `abort_dag_with_lineage()` — sets `graph.status = Failed` unconditionally; called by both cascade checks before `propagate_failure()` is reached | +| `crates/zeph-orchestration/src/scheduler/tick/mod.rs:727-768` | `check_timeouts()` — per-running-task timeout evaluation; gains per-task effective-timeout lookup | +| `crates/zeph-orchestration/src/scheduler/tick/mod.rs:254-270` | `wait_event()` — nearest-timeout-deadline computation (`:261-270`), currently uniform on `self.task_timeout`; becomes per-task-aware | +| `crates/zeph-orchestration/src/scheduler/router.rs:18-59` | `build_task_prompt()` — `Completed`-only dependency filter (`:23-34`), SEC-ORCH-01 sanitizer (`:59`) — unmodified; consumes recovered `state_injection` output as-is | +| `crates/zeph-core/src/agent/scheduler_loop.rs:258-` | `RunInline` inline `tokio::select!` — gains a third `tokio::time::timeout` branch | +| `crates/zeph-core/src/agent/scheduler_loop.rs:338,547-551` | `scheduler.tick()` call site and `save_graph_snapshot()` gating on `take_graph_dirty()` — the same-tick snapshot atomicity durability guarantee | +| `crates/zeph-orchestration/src/scheduler/mod.rs:389-430` | `resume_from()` — rebuilds the `running` map from persisted `TaskStatus::Running` entries; unmodified by this feature (no resume re-scan added) | +| `crates/zeph-orchestration/src/scheduler/mod.rs:518` | Completion-event `mpsc::channel(64)` — cited as the reason Alt A (deferred) must not reuse this channel for progress signals | +| `crates/zeph-config/src/experiment.rs:261-,274` | `OrchestrationConfig`, existing `task_timeout_secs` (reused as the run-timeout global default, no new field) | +| `crates/zeph-config/src/migrate/mod.rs:646-` | `MIGRATIONS` registry — new step for `default_idle_timeout_secs` | +| `src/init/agents.rs:11` | `step_orchestration()` — `--init` wizard integration point | +| `crates/zeph-core/src/metrics.rs:101-107` | `OrchestrationMetrics` — `tasks_completed`/`tasks_failed` | +| `crates/zeph-core/src/agent/plan.rs:722-833` | `finalize_plan_completed()`/`finalize_plan_failed()` — confirms metrics are status-derived at graph-finalization time, resolving the recovered-node metrics classification with no code change (critic finding M-a) | + +--- + +## 1. Overview + +### Problem Statement + +`zeph-orchestration`'s `TaskGraph`/`DagScheduler` controls execution timing and terminal-failure +handling more coarsely than LangGraph's `add_node` API surface: timeout is a single global +`Duration` with no per-task override (inconsistent with the crate's own established override +pattern for `failure_strategy`/`max_retries`/`token_budget_cents`), and retry-exhausted failure +always collapses to Abort-equivalent termination, with the only non-abort escape hatch +(`FailureStrategy::Ask`) pausing the entire graph rather than allowing autonomous recovery. Full +problem framing: `[[specs/075-orchestration-node-control-parity/brd]]` §1-2. + +### Goal + +A `TaskNode` can declare a per-task run-timeout override enforced on both spawned and `RunInline` +tasks, and can declare a Mode-1 recovery action that substitutes a synthetic output and lets the +graph continue past a terminal `Abort`-default or retry-exhausted `Retry` failure — without +pausing unrelated concurrent work. Both are additive, `Option`-typed, and produce zero behavior +change for any graph that does not opt in. + +### Out of Scope + +See `[[specs/075-orchestration-node-control-parity/brd]]` §5 for the full list with rationale. +Summary: Mode 2 (`route_to` reroute-to-alternate-node recovery, blocked by findings N5/N1/N3 — +requires a `TaskStatus::Dormant`/on-failure-edge redesign), idle-timeout progress-signal plumbing +(Alt A), any change to default `Abort`/`Skip`/`Ask` semantics, and a resume-time re-scan (proven +unnecessary by same-tick snapshot atomicity). + +Full requirement-level detail: `[[specs/075-orchestration-node-control-parity/srs]]`. Quality +targets: `[[specs/075-orchestration-node-control-parity/nfr]]`. + +--- + +## 2. Functional Requirements + +See `[[specs/075-orchestration-node-control-parity/srs]]` for the complete EARS-notation requirement +set (FR-001 through FR-021, plus FR-D-01/FR-D-02 deferred) and traceability matrix. Summary: + +| ID | Requirement | Priority | +|----|------------|----------| +| FR-001..004 | `TimeoutPolicy` data model; per-task run-timeout enforcement on both spawned and `RunInline` dispatch; `wait_event()` made per-task-aware | must | +| FR-005 | `idle_timeout_secs` defined/config-surfaced, documented no-op, loudly marked reserved | must | +| FR-006..010 | `RecoveryAction` data model (`state_injection` only, `route_to` deferred); Mode-1 recovery in `propagate_failure()`; existing dependent-unblock/prompt-consumption path reused unmodified; no graph pause | must | +| FR-011, FR-012 | `validate()` reject (recovery + verify_predicate) and warn (recovery + Skip/Ask) guards | must | +| FR-013..015 | Cascade-abort precedence via existing ordering; documented timeout-vs-event asymmetry; documented recorded-then-recovered limitation | must | +| FR-016 | Durability: no resume re-scan, same-tick snapshot atomicity is the guarantee | must | +| FR-017..019 | Config: reuse `task_timeout_secs`; new `default_idle_timeout_secs` with full integration; per-task fields need only `#[serde(default)]` | must | +| FR-020 | Doc annotation: `ready_tasks()` `Ready`-arm bypass is load-bearing for recovery | must | +| FR-021 | Recovered-node metrics classification resolved (status-derived, no special case) | must | + +--- + +## 3. Architecture / Design + +### 3.1 Data Model + +```rust +/// Per-task timeout override, mirroring LangGraph's `TimeoutPolicy`. +/// `None` on either field falls back to the graph-global default. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct TimeoutPolicy { + /// Hard wall-clock cap. `None` falls back to `OrchestrationConfig.task_timeout_secs`. + pub run_timeout_secs: Option, + /// Idle/no-progress cap. Defined and config-surfaced but NOT enforced in v1 — see FR-005. + pub idle_timeout_secs: Option, +} + +/// Declarative recovery action for a node's terminal failure. +/// v1 supports Mode 1 only; `route_to` (Mode 2) is deferred and added later, +/// additively, as a further `#[serde(default)]` field on this same struct. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RecoveryAction { + /// Substitute output injected as this node's `TaskResult.output` on recovery. + pub state_injection: Option, +} +``` + +Both attach to `TaskNode` (`graph.rs:379-452`) as: + +```rust +#[serde(default, skip_serializing_if = "Option::is_none")] +pub timeout: Option, +#[serde(default, skip_serializing_if = "Option::is_none")] +pub recovery: Option, +``` + +### 3.2 Timeout Enforcement — Two Dispatch Kinds, Two Enforcement Sites + +``` +TaskNode.timeout.run_timeout_secs + │ + ├── Spawned task ──> check_timeouts() (tick/mod.rs:727) + │ effective = timeout.and_then(|t| t.run_timeout_secs) + │ .map(Duration::from_secs) + │ .unwrap_or(self.task_timeout) + │ wait_event() nearest-deadline (tick/mod.rs:261) uses the same + │ per-task effective value instead of the uniform self.task_timeout + │ + └── RunInline task ──> scheduler_loop.rs:258 inline tokio::select! gains a third branch: + tokio::time::timeout(effective, run_inline_tool_loop(...)) + (check_timeouts() cannot fire — the tick loop is blocked for the + task's whole duration on this path) +``` + +Both enforcement sites converge on the same `TaskOutcome::Failed`/`TaskStatus::Failed` shape, so +downstream failure handling (§3.3) is uniform regardless of which dispatch kind timed out. + +### 3.3 Recovery — Mode 1 (`state_injection`) + +``` +propagate_failure(graph, failed_id, rev_adj) [dag.rs:223] + │ + ├── strategy == Abort ─────────────────┐ + │ │ + └── strategy == Retry, retries exhausted┤ + ▼ + node.recovery?.state_injection == Some(v) ? + │Yes │No + ▼ ▼ + node.status = Completed existing Abort-equivalent + node.result = Some(TaskResult{ branch (graph.status = Failed, + output: v, artifacts: [], cancel Running tasks) — UNCHANGED + duration_ms: 0, agent_id: None, + agent_def: Some("__recovery__") + }) + graph.status: UNCHANGED (stays Running) + │ + ▼ + next tick: ready_tasks() Pending arm sees dependents' + depends_on now Completed → Ready → dispatched + │ + ▼ + build_task_prompt() Completed-only filter (scheduler/router.rs:28) + picks up the recovered node's result; SEC-ORCH-01 sanitizer (:59) + applies exactly as it would to any normal completion +``` + +No new consumption machinery: the recovered node looks, to every downstream consumer, like a +task that completed normally with an unusual `agent_def` marker. + +### 3.4 Precedence: Cascade-Abort Over Recovery (No Code Reordering) + +``` +handle_failed_outcome(task_id, error) [tick/mod.rs:590] + │ + ├── graph.tasks[task_id].status = Failed [:599] + ├── cascade_detector.record_outcome(false) [:601-602] + ├── build lineage chain + ├── fan-out cascade check: + │ trips? ──Yes──> return abort_dag_with_lineage(...) [:629-647] ── recovery + │ │No UNREACHABLE + ├── linear-chain cascade check: + │ trips? ──Yes──> return abort_dag_with_lineage(...) [:649-662] ── recovery + │ │No UNREACHABLE + ▼ + propagate_failure(...) [:664] ── recovery (§3.3) can fire HERE, only if no cascade tripped + +check_timeouts() [tick/mod.rs:727] ── NO record_outcome, NO cascade evaluation on this path + │ + ▼ + propagate_failure(...) [:749] ── recovery ALWAYS reachable on the timeout path +``` + +This asymmetry (event-path recovery is conditional on no cascade trip; timeout-path recovery is +unconditional) is a property of the pre-existing cascade design — the cascade detector was never +fed by the timeout path — and recovery inherits it unchanged. No code reordering is required or +performed. + +--- + +## 4. Key Invariants + +### Always (without asking) + +- A `TaskNode` with `timeout == None` and `recovery == None` behaves identically to current + behavior — global `task_timeout` applies, `Abort`/retry-exhausted-`Retry` always falls through + to Abort-equivalent termination, `Skip`/`Ask` are untouched (NFR-CO-01, NFR-CO-02). +- Both new `TaskNode` fields use `#[serde(default, skip_serializing_if = "Option::is_none")]`, + matching the existing `network_scope`/`asset_sensitivity` forward-compat precedent. +- Recovery mutates `node.status`/`node.result` synchronously, inside `propagate_failure()`, with + no `.await` — this is what makes the same-tick snapshot atomicity durability guarantee hold + (FR-016). +- `graph.status` is left unmodified by a Mode-1 recovery — independent branches always continue + (FR-009). +- The existing cascade-check-before-`propagate_failure()` ordering in `handle_failed_outcome()` + is preserved exactly — recovery attaches only inside `propagate_failure()`, never before it. +- `validate()` rejects `recovery.is_some() && verify_predicate.is_some()` on the same node + (FR-011) and warns (does not reject) when `recovery.is_some()` under `Skip`/`Ask` (FR-012). +- The `idle_timeout_secs` field (per-task and global) is documented "reserved — not yet + enforced" everywhere an operator can set it: `--init` wizard text and `config.toml` comment + (FR-005, FR-018, critic finding M-b). +- New tracing spans on the recovery-application and per-task-timeout-cancellation code paths + follow the `..` naming convention (NFR-OB-02). + +### Ask First + +- Whether the recovery-application tracing span name is `orchestration.dag.recover_task` or a + different name — cosmetic naming choice left to the implementing session, but must follow the + project convention. +- Whether `default_idle_timeout_secs` gets a dedicated CLI/TUI surface beyond config.toml/`--init` + — the existing `task_timeout_secs` precedent has none, and FR-018 does not require one, but this + is worth confirming against current TUI settings-editor conventions (`[[061-tui-settings-editor-parity/spec]]` + if it exists) before implementation. + +### Never + +- **NEVER** add a `route_to` field to `RecoveryAction` in this v1 implementation — Mode 2 is + proven unsafe as originally designed (N5) and requires a `TaskStatus::Dormant`/on-failure-edge + redesign that is explicitly out of scope here (FR-D-01). +- **NEVER** reorder the cascade-check-before-`propagate_failure()` sequence in + `handle_failed_outcome()` — the existing ordering IS the cascade-over-recovery precedence + mechanism (FR-013); reordering it is a correctness change, not a cleanup. +- **NEVER** add resume-time re-evaluation logic for recovery — same-tick snapshot atomicity + already closes the crash window; adding a re-scan reintroduces exactly the idempotency risk the + architect identified and retracted in round 2 (FR-016). +- **NEVER** let a predicate-gated node (`verify_predicate.is_some()`) also be recovery-eligible — + `validate()` must reject this combination unconditionally (FR-011); recovery bypasses the + completion-event handler where predicate verification runs. +- **NEVER** introduce a new `tokio::spawn()` call site for timeout enforcement or recovery — both + are synchronous/inline per NFR-AS-01; per + `[[039-background-task-supervisor/spec]]`'s binding NEVER section, this is a hard project-wide + constraint, not specific to this feature. +- **NEVER** silently enforce `idle_timeout_secs` partially or heuristically in v1 (e.g. "best + effort" using an unrelated existing signal) — it must be a clean no-op until the Alt A + progress-signal plumbing exists, per FR-005's explicit no-partial-enforcement requirement. + +--- + +## 5. Edge Cases and Error Handling + +| Scenario | Expected Behavior | +|----------|-------------------| +| Per-task `run_timeout_secs` shorter than time already elapsed when set mid-run (e.g. hot-reloaded graph data) | Flagged as timed out on the next `check_timeouts()`/`wait_event()` evaluation — no special-cased grace period, consistent with existing `check_timeouts()` semantics | +| `idle_timeout_secs` set (per-task or global) | Documented no-op in v1 — never fires, never treated as "always idle" by omission; `--init`/config.toml text states this explicitly (FR-005) | +| `recovery.state_injection` set but effective strategy is `Skip` | `validate()` warns at graph-construction time; at runtime the task is `Skipped` via the existing `Skip` arm — recovery is never consulted (that arm does not call the recovery branch) | +| `recovery.state_injection` set but effective strategy is `Ask` | Same as `Skip` — `validate()` warns; runtime pauses the graph via the existing `Ask` arm, recovery is never consulted | +| `recovery.state_injection` set AND `verify_predicate` set on the same node | `validate()` rejects the graph at construction time (FR-011) — never reaches runtime | +| A node recovers, and its recovered failure also would have tripped a cascade-abort threshold, but the cascade check ran first and already aborted the graph | Graph ends `Failed` via `abort_dag_with_lineage()` — recovery is structurally unreachable, `propagate_failure()` (where recovery lives) was never called for this event (FR-013) | +| A node recovers via the timeout path (no cascade evaluation exists there) | Recovery always fires if `state_injection` is configured — there is no cascade check to preempt it on this path; this is the documented timeout-vs-event asymmetry (FR-014), not a bug | +| A node recovers; its failure was already recorded by `record_outcome(false)` before recovery ran (event path only) | The recorded failure still counts toward later cascade-threshold evaluation for other tasks in the same graph — documented v1 limitation (FR-015), not corrected in v1 | +| Both `run_timeout_secs` and (in a future Alt A build) `idle_timeout_secs` would fire on the same tick | Out of scope for v1 (idle is a no-op) — when Alt A ships, the timeout-cause record must name exactly one firing mechanism, not an aggregate flag (carried forward as a design note for the Alt A follow-up, NFR-OB-01) | +| Graph crash-resumes mid-tick, between a task's `Failed` status set and its Mode-1 recovery application | Cannot happen mid-persisted-state: the snapshot only ever captures the pre-tick or post-tick graph, never an intermediate state within a single synchronous `tick()` call (FR-016) — resume rebuilds `running` from the last persisted snapshot and the failure+recovery sequence re-fires cleanly on the next tick if it had not yet been captured | +| A recovered node's `agent_def == Some("__recovery__")` is inspected by code that assumes `agent_def` always names a real `SubAgentDef` | Out of scope to audit every `agent_def` consumer in this spec; flagged for the implementing session to grep for `agent_def` consumers and confirm none panics or mis-renders on this synthetic marker (implementation-phase verification, not a v1 requirement gap) | + +--- + +## 6. Success Criteria + +Implementation-facing checklist (business-facing criteria: `[[specs/075-orchestration-node-control-parity/brd]]` §6): + +- [ ] Default (`timeout: None, recovery: None`) regression test: reproduces the exact pre-feature + `check_timeouts()`/`wait_event()`/`propagate_failure()` code paths, byte-for-byte +- [ ] Per-task `run_timeout_secs` override test: fires before the (longer) global default would + have, on both spawned and `RunInline` dispatch +- [ ] `idle_timeout_secs` no-op test: a task with a short `idle_timeout_secs` and long idle + execution is never flagged as timed out by that field +- [ ] Mode-1 recovery test: a node with `state_injection` set, on `Abort`-default failure, + transitions to `Completed`; its dependents unblock and receive the injected value through + `build_task_prompt()`; `graph.status` stays `Running` +- [ ] Mode-1 recovery test: same, but for retry-exhausted `Retry` (not just `Abort`-default) +- [ ] Cascade-precedence test: a node with `recovery` configured whose failure also trips a + cascade-abort threshold ends the graph `Failed`, not recovered +- [ ] `validate()` reject test: `recovery.is_some() && verify_predicate.is_some()` is rejected + with a message naming the offending task +- [ ] `validate()` warn test: `recovery.is_some()` under effective `Skip`/`Ask` strategy warns but + does not reject +- [ ] Metrics test: a recovered node is counted in `tasks_completed`, never `tasks_failed`, via + the existing status-derived `finalize_plan_completed`/`finalize_plan_failed` path — no new + metrics code required +- [ ] Config round-trip test: `default_idle_timeout_secs` serializes/deserializes correctly, + defaults to `None`, and a config persisted before this feature exists migrates cleanly +- [ ] `cargo +nightly fmt --check`, `cargo clippy --profile ci ... -D warnings`, + `cargo nextest run ...`, and the rustdoc gate all pass per `.claude/rules/branching.md` +- [ ] Zero new `tokio::spawn()` call sites: `.claude/rules/continuous-improvement.md` + async-supervision scan count non-increasing +- [ ] `.local/testing/playbooks/orchestration-node-control-parity.md` created (main-repo path) +- [ ] `.local/testing/coverage-status.md` rows added (main-repo path, status `Untested`) +- [ ] `--init` wizard and `config.toml` comment for `default_idle_timeout_secs` verified live to + state "reserved — not yet enforced" + +--- + +## 7. Relationship to Existing Specs + +| This spec | Existing spec | Relationship | +|-----------|---------------|---------------| +| `TaskNode.timeout`/`.recovery`, `propagate_failure()` recovery branch, `check_timeouts()`/`wait_event()` per-task awareness | `[[009-orchestration/spec]]` | Extends the existing `TaskGraph`/`DagScheduler` failure-handling and timeout model; does not change `FailureStrategy`'s enum shape, `Skip`/`Ask` semantics, or the cascade-detector's event-path ordering | +| No new `tokio::spawn` site, synchronous recovery mutation | `[[039-background-task-supervisor/spec]]` | Compliance claim verified against the binding NEVER section — see NFR-AS-01 | +| Original research/gap-audit spec | `.local/specs/059-orchestration-node-control-parity/spec.md` | This spec is the formal `/sdd` output resolving that draft's `[NEEDS CLARIFICATION]` items that are in-scope for v1; it also **narrows** that draft's scope — the research draft's `route_to`/Mode-2 sketch (its FR-004/FR-005, data-model `route_to` field) is retracted here as unsafe-as-designed (FR-D-01) rather than carried forward | +| Recovery-completed node skips predicate verification | `[[001-system-invariants/spec]]` | The `validate()` reject guard (FR-011) is the mechanism that keeps this consistent with any project-wide invariant about predicate-gated output never reaching downstream consumers unverified | + +--- + +## 8. See Also + +- [[MOC-specs]] — Map of all specifications +- [[constitution]] — Project-wide principles +- [[specs/075-orchestration-node-control-parity/brd]] — Business case and success criteria +- [[specs/075-orchestration-node-control-parity/srs]] — Full functional requirements (EARS) +- [[specs/075-orchestration-node-control-parity/nfr]] — Quality targets (ISO/IEC 25010) +- [[specs/075-orchestration-node-control-parity/plan]] — Step-by-step implementation plan +- [[specs/075-orchestration-node-control-parity/tasks]] — Ordered developer task breakdown +- [[001-system-invariants/spec]] — Cross-cutting architectural invariants +- [[009-orchestration/spec]] — DAG planner, `DagScheduler`, `TaskGraph`, parent spec for the + orchestration subsystem this feature extends +- [[039-background-task-supervisor/spec]] — Binding async-supervision contract (NFR-AS-01) +- GitHub issue #6021 — source issue +- `.local/handoff/2026-07-13T21-12-06-architect.md` — final (v3) architect design +- `.local/handoff/2026-07-13T21-16-30-critic.md` — final critic verdict (minor / approved) diff --git a/specs/075-orchestration-node-control-parity/srs.md b/specs/075-orchestration-node-control-parity/srs.md new file mode 100644 index 000000000..f9fbfa0dc --- /dev/null +++ b/specs/075-orchestration-node-control-parity/srs.md @@ -0,0 +1,356 @@ +--- +aliases: + - Orchestration Node Control Parity SRS + - Node Timeout / Retry-Exhausted Recovery SRS + - SRS 6021 +tags: + - sdd + - srs + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[specs/075-orchestration-node-control-parity/brd]]" + - "[[specs/075-orchestration-node-control-parity/spec]]" + - "[[specs/075-orchestration-node-control-parity/nfr]]" +--- + +# SRS: Orchestration Node Control Parity — Per-Task Timeouts and Retry-Exhausted Recovery (GitHub #6021) + +ISO/IEC/IEEE 29148:2018 compliant. Requirements use EARS notation. Technical basis: architect +handoffs `.local/handoff/2026-07-13T20-47-55-architect.md` (base plan), +`.local/handoff/2026-07-13T21-00-15-architect.md` and `.local/handoff/2026-07-13T21-09-12-architect.md` +(round-2 revisions), `.local/handoff/2026-07-13T21-12-06-architect.md` (v3, final — Mode 2 dropped, +resume re-scan dropped); critic handoffs `.local/handoff/2026-07-13T20-51-35-critic.md` (round 1), +`.local/handoff/2026-07-13T21-12-00-critic.md` (N5 correction), `.local/handoff/2026-07-13T21-16-30-critic.md` +(final verdict: minor / approved, two non-blocking notes M-a/M-b folded in below). All code +citations verified against HEAD `d93d82e8`. + +## 1. Scope + +This SRS specifies v1 of the orchestration node control parity capability: a per-task run-timeout +override, a config-surfaced-but-inert `idle_timeout_secs` field, and a single declarative +"substitute output and continue" (Mode 1) recovery mechanism for terminal `Abort`-default or +retry-exhausted `Retry` failures. Mode 2 (reroute-to-alternate-node) and the idle-timeout +progress-signal plumbing are explicitly deferred (§8). + +--- + +## 2. Per-Task Timeout Override + +### FR-001: `TimeoutPolicy` Data Model on `TaskNode` + +**THE SYSTEM SHALL** add a nested `TimeoutPolicy { run_timeout_secs: Option, +idle_timeout_secs: Option }` type and expose it as `timeout: Option` on +`TaskNode` (`crates/zeph-orchestration/src/graph.rs:379-452`), annotated +`#[serde(default, skip_serializing_if = "Option::is_none")]` consistent with the existing +`network_scope`/`asset_sensitivity` forward-compatibility pattern on the same struct. + +### FR-002: Per-Task Run-Timeout Enforcement — Spawned Tasks + +**WHEN** `check_timeouts()` (`crates/zeph-orchestration/src/scheduler/tick/mod.rs:727-768`) +evaluates a running spawned task, **THE SYSTEM SHALL** compute an effective run-timeout as +`task.timeout.and_then(|t| t.run_timeout_secs).map(Duration::from_secs)`, falling back to the +existing graph-global `self.task_timeout` **WHEN** no per-task override is set. + +**THE SYSTEM SHALL** compute this effective timeout in `O(1)` per running task — **THE SYSTEM +SHALL NOT** introduce a full-graph scan inside `check_timeouts()`'s existing `O(self.running)` +loop. + +### FR-003: `wait_event()` Nearest-Deadline Becomes Per-Task-Aware + +**THE SYSTEM SHALL** update the nearest-timeout computation in `wait_event()` +(`crates/zeph-orchestration/src/scheduler/tick/mod.rs:261-270`, currently +`self.task_timeout.checked_sub(r.started_at.elapsed())` applied uniformly) to use each running +task's effective run-timeout (FR-002) instead of the single global `self.task_timeout`. **THE +SYSTEM SHALL** preserve the existing `O(self.running)` complexity of this computation — no +additional graph traversal per call. + +### FR-004: Per-Task Run-Timeout Enforcement — `RunInline` Tasks + +**THE SYSTEM SHALL** add a third branch to the inline `tokio::select!` in the `RunInline` +execution path (`crates/zeph-core/src/agent/scheduler_loop.rs:258-`) that races +`tokio::time::timeout(effective_run_timeout, self.run_inline_tool_loop(...))` alongside the +existing tool-loop and cancellation-token branches. + +> **Rationale:** `check_timeouts()` runs on the scheduler's tick loop, which is blocked for the +> entire duration of a `RunInline` task's execution — `check_timeouts()` structurally never fires +> while a `RunInline` task is in flight. The inline `select!` is the only structurally viable +> enforcement site for this task kind. + +**WHEN** the `tokio::time::timeout` branch fires before the tool loop completes, **THE SYSTEM +SHALL** treat the outcome identically to the existing cancellation-token branch's `Failed` +outcome path (same `zeph_orchestration::TaskOutcome::Failed` construction), so downstream failure +handling (`propagate_failure`, recovery per §4) is uniform across both dispatch kinds. + +### FR-005: `idle_timeout_secs` — Defined, Config-Surfaced, Documented No-Op in v1 + +**THE SYSTEM SHALL** expose `idle_timeout_secs` as a serializable, config-surfaced field (on both +`TimeoutPolicy` per-task and a new graph-global default, FR-015) with its target semantics fully +documented (an idle/no-progress cap, distinct from the hard `run_timeout_secs` cap). + +**THE SYSTEM SHALL NOT** enforce `idle_timeout_secs` in v1 — no progress-signal mechanism exists +in `zeph-subagent` or `zeph-orchestration` today (verified: zero heartbeat/liveness/progress hits +across both crates), so there is no signal to evaluate the field against. + +**THE SYSTEM SHALL** mark this field's inert status loudly wherever an operator could configure +it: the `--init` wizard help text and the `config.toml` comment for both the per-task and the +graph-global field **SHALL** state "reserved — not yet enforced (see follow-up)" (critic finding +M-b), so a user who sets it does not assume idle-based kills are active. + +--- + +## 3. Terminal-Failure Recovery (Mode 1: `state_injection`) + +### FR-006: `RecoveryAction` Data Model on `TaskNode` + +**THE SYSTEM SHALL** add `RecoveryAction { state_injection: Option }` and expose it as +`recovery: Option` on `TaskNode` +(`crates/zeph-orchestration/src/graph.rs:379-452`), annotated +`#[serde(default, skip_serializing_if = "Option::is_none")]`. + +**THE SYSTEM SHALL NOT** add a `route_to` field to `RecoveryAction` in v1 (Mode 2 is deferred, +§8) — **THE SYSTEM SHALL** design `RecoveryAction` so a `route_to` field can be added later as an +additive `#[serde(default)]` field without a breaking schema change. + +### FR-007: Recovery Fires on Terminal Abort-Class Failure + +**WHEN** `propagate_failure()` (`crates/zeph-orchestration/src/dag.rs:223-322`) is invoked for a +task `T` **AND** `T`'s effective failure strategy is `FailureStrategy::Abort` (the default arm, +`dag.rs:243-253`) **OR** `FailureStrategy::Retry` with `retry_count >= max_retries` (the +retry-exhausted arm, `dag.rs:281-298`) **AND** `T.recovery.state_injection == Some(v)`, +**THE SYSTEM SHALL**, instead of the existing Abort-equivalent branch: + +1. Set `T.status = TaskStatus::Completed`. +2. Set `T.result = Some(TaskResult { output: v, artifacts: vec![], duration_ms: 0, agent_id: None, + agent_def: Some("__recovery__".to_string()) })`. +3. Leave `graph.status` unmodified (**not** set to `Failed`). + +**WHEN** `T.recovery` is `None` **OR** `T.recovery.state_injection` is `None`, **THE SYSTEM +SHALL** preserve the exact existing Abort-equivalent behavior with zero change (BG-04). + +### FR-008: Dependents Unblock Through the Existing Path — No New Consumption Machinery + +**THE SYSTEM SHALL** rely entirely on existing mechanisms to deliver a recovered node's output to +its dependents: the `Pending`→`Ready` transition in `ready_tasks()` +(`crates/zeph-orchestration/src/dag.rs:179-208`, which unblocks a dependent once all +`depends_on` entries are `Completed`) and `build_task_prompt()`'s existing `Completed`-only +dependency filter plus SEC-ORCH-01 sanitizer +(`crates/zeph-orchestration/src/scheduler/router.rs:18-59`). **THE SYSTEM SHALL NOT** introduce +any new prompt-construction or consumption code path for recovered output. + +### FR-009: Recovery Does Not Pause the Graph + +**WHEN** a node recovers via FR-007, **THE SYSTEM SHALL NOT** set `graph.status = +GraphStatus::Paused` or `GraphStatus::Failed` as a side effect — independent, non-dependent +branches in the same graph **SHALL** continue executing unaffected, and the graph **SHALL** reach +its normal terminal status (`Completed`/`Failed`/`Paused`) based only on the remaining tasks' +outcomes. + +### FR-010: Recovery-Completed Node Bypasses the Completion-Event Pipeline (Documented) + +**THE SYSTEM SHALL** document that a node completed via FR-007 skips the normal +completion-event handler entirely (predicate verification, `token_budget_cents` check, +`verify_completeness`/`Verify` emission) because the transition happens synchronously inside +`propagate_failure()`, not through the event-driven completion path. **THE SYSTEM SHALL** rely on +FR-011's `validate()` guard to make this safe (a predicate-gated node is never recovery-eligible). + +--- + +## 4. `validate()` Recovery Guards + +### FR-011: Reject `recovery` + `verify_predicate` Co-Configuration + +**WHEN** `validate()` (`crates/zeph-orchestration/src/dag.rs:37-91`) processes a `TaskNode` with +both `recovery.is_some()` **AND** `verify_predicate.is_some()`, **THE SYSTEM SHALL** return +`Err(OrchestrationError::InvalidGraph(...))` naming the offending task index. + +> **Rationale:** recovery (FR-007) bypasses the completion-event handler where predicate +> verification runs (FR-010). A predicate-gated node must not be recovery-eligible, or an +> unverified synthetic output could reach downstream consumers as if it had passed verification. + +### FR-012: Warn on `recovery` Under `Skip`/`Ask` Failure Strategy + +**WHEN** `validate()` processes a `TaskNode` with `recovery.is_some()` **AND** its effective +failure strategy (own override or graph default) is `FailureStrategy::Skip` or +`FailureStrategy::Ask`, **THE SYSTEM SHALL** emit `tracing::warn!` naming the task and its +strategy, but **SHALL NOT** reject the graph. + +> **Rationale:** recovery only fires from the `Abort`/retry-exhausted-`Retry` branches of +> `propagate_failure()` (FR-007) — under `Skip` or `Ask` it is configured but inert. This is a +> surfaced footgun, not an error: `Skip`/`Ask` semantics are explicit author choices and remain +> completely unchanged (BG-04). + +--- + +## 5. Cascade-Abort vs. Recovery Precedence + +### FR-013: Cascade-Abort Takes Precedence Over Recovery — No Code Reordering + +**THE SYSTEM SHALL** rely on the existing event-path ordering in `handle_failed_outcome()` +(`crates/zeph-orchestration/src/scheduler/tick/mod.rs:590-681`): task set `Failed` (`:598`) → +`record_outcome(false)` (`:601-602`) → lineage build → fan-out cascade check that `return`s +`abort_dag_with_lineage(...)` early on trip (`:629-647`) → linear-chain cascade check that +similarly `return`s early (`:649-662`) → **only then** `propagate_failure()` (`:664`). **THE +SYSTEM SHALL NOT** reorder this sequence — recovery is structurally unreachable whenever a cascade +abort fires, because `propagate_failure()` (where FR-007 lives) is never reached on that path. + +### FR-014: Document the Pre-Existing Timeout-vs-Event Recovery Asymmetry + +**THE SYSTEM SHALL** document, as a single authoritative statement of "recovery on terminal +failure": recovery fires inside `propagate_failure()`; whether that call site is reached depends +on failure origin. The **timeout** path (`check_timeouts()` → `propagate_failure()`, no +`record_outcome`/cascade evaluation on that path today) **always** reaches `propagate_failure()`. +The **event** path (`handle_failed_outcome()`) reaches it **only if no cascade abort fired** +(FR-013). **THE SYSTEM SHALL** state this is a property of the pre-existing cascade design that +recovery inherits unchanged, not a new inconsistency introduced by this feature. + +### FR-015: Document the Recorded-Then-Recovered Second-Order Effect (Accepted v1 Limitation) + +**THE SYSTEM SHALL** document that `record_outcome(task_id, false, ...)` (`tick/mod.rs:601-602`) +runs **before** Mode-1 recovery can rescue the node on the event path, so a subsequently-recovered +failure still counts once in the cascade detector's history and could contribute to a later +cascade threshold trip. **THE SYSTEM SHALL** accept this as a documented v1 limitation — amending +the cascade record on successful recovery is an explicitly out-of-scope candidate follow-up +refinement (§8), not a v1 requirement. + +--- + +## 6. Durability + +### FR-016: No Resume Re-Scan — Same-Tick Snapshot Atomicity Is the Guarantee + +**THE SYSTEM SHALL NOT** add any resume-time re-evaluation logic for pending or in-flight +recovery. **THE SYSTEM SHALL** rely on the verified existing invariant: the recovery mutation +inside `propagate_failure()` is synchronous (no `.await`), `propagate_failure()` runs inside the +fully-synchronous `scheduler.tick()` (`crates/zeph-core/src/agent/scheduler_loop.rs:338`), and +`save_graph_snapshot(...).await` runs later in the **same** loop iteration, gated on +`take_graph_dirty()` (`scheduler_loop.rs:547-551`, and `graph_dirty` is set at the start of +`handle_failed_outcome()`/`check_timeouts()`). A node's `Failed`→`Completed` recovery transition +therefore always lands in the **same** persisted snapshot as the triggering failure — there is no +crash window where a `Failed` status persists without its already-applied recovery, and no +crash window where a mid-tick crash leaves recovery "half-applied" (a mid-tick crash resumes from +the *prior* snapshot, where the task is still `Running`; the failure and recovery both re-fire +cleanly from that state on the next tick, per `crates/zeph-orchestration/src/scheduler/mod.rs:389-430`). + +--- + +## 7. Configuration + +### FR-017: Reuse `task_timeout_secs` as the Run-Timeout Global Default + +**THE SYSTEM SHALL NOT** add a new config field for the run-timeout global default — the existing +`OrchestrationConfig.task_timeout_secs` (`crates/zeph-config/src/experiment.rs:274`, default +300s) continues to serve as the fallback whenever a `TaskNode` sets no `timeout.run_timeout_secs` +override (FR-002). + +### FR-018: New `default_idle_timeout_secs` Config Field + +**THE SYSTEM SHALL** add exactly one new field, `default_idle_timeout_secs: Option` +(`None` = off, matching the field's v1 no-op status), to `OrchestrationConfig` +(`crates/zeph-config/src/experiment.rs`). **THE SYSTEM SHALL** provide the full mandatory +integration set for this field per this project's Development Rules: + +1. `config.toml` `[orchestration]` section entry, documented as reserved/not-yet-enforced (FR-005). +2. `--init` wizard entry in `step_orchestration()` (`src/init/agents.rs:11`), same reserved-field + wording. +3. A `--migrate-config` step in the `MIGRATIONS` registry + (`crates/zeph-config/src/migrate/mod.rs:646-`) that adds the field with a `None` default for + pre-existing configs. +4. `#[serde(default)]` on the field for forward compatibility with configs persisted before this + feature existed. + +**THE SYSTEM SHALL NOT** add a CLI subcommand or TUI command-palette entry for this field — it is +a passive config default, not an imperative action, consistent with how `task_timeout_secs` is +exposed today (config-only, no dedicated CLI/TUI surface). + +### FR-019: Per-Task Fields Are Graph Data, Not Config — No Wizard/Migration Surface + +**THE SYSTEM SHALL** treat `TaskNode.timeout` and `TaskNode.recovery` as planner-authored graph +data, following the existing precedent set by `failure_strategy`/`max_retries` overrides on the +same struct. **THE SYSTEM SHALL NOT** add `--init`/`--migrate-config` entries for these two +fields — **THE SYSTEM SHALL** rely solely on `#[serde(default, skip_serializing_if = +"Option::is_none")]` (FR-001, FR-006) for forward compatibility with graphs persisted before this +feature existed. + +--- + +## 8. Documentation and Metrics + +### FR-020: Annotate the `ready_tasks()` `Ready`-Arm Dependency Bypass as Load-Bearing + +**THE SYSTEM SHALL** add a doc comment to the `Ready` arm of `ready_tasks()` +(`crates/zeph-orchestration/src/dag.rs:185-191`, which checks only predicate clearance, **not** +`depends_on` completion) stating that this bypass is load-bearing for Mode-1 recovery's unblock +path — a recovered node's dependents transition through the `Pending` arm (which does check +`depends_on` completion) using the recovered node's now-`Completed` status, but a future +refactor that "fixes" the `Ready` arm to also re-check `depends_on` could change dispatch +semantics for predicate-gated tasks in ways that interact with recovery. **THE SYSTEM SHALL** +also note that this same bypass is why Mode 2 (deferred, §9) cannot be made safe by a +`depends_on`-based topology constraint alone. + +### FR-021: Recovered-Node Metrics Classification — Resolved, Status-Derived + +**THE SYSTEM SHALL** document, as the resolution to critic finding M-a, that a Mode-1-recovered +node requires **no special-case metrics handling**: `OrchestrationMetrics.tasks_completed`/ +`tasks_failed` (`crates/zeph-core/src/metrics.rs:101-107`) are populated exclusively by +`finalize_plan_completed()`/`finalize_plan_failed()` +(`crates/zeph-core/src/agent/plan.rs:722-833`), which filter `completed_graph.tasks` by **final** +`TaskStatus` at graph-finalization time — not incremented at failure-event time. Because a +recovered node's status is `Completed` (FR-007) by the time the graph reaches a terminal state, +it is counted in `tasks_completed` and never in `tasks_failed`, with zero code change required. + +--- + +## 9. Deferred Requirements (Acknowledged) + +### FR-D-01: Mode 2 (`route_to` Reroute-to-Alternate-Node Recovery) + +Deferred (BRD §5). Blocked by three findings from the design review: + +- **N5 (root cause):** a `depends_on == [failed_task]` fallback-node design dispatches on the + failed task's *success* (`ready_tasks()`'s `Pending` arm unblocks on `Completed`, + `dag.rs:192-203`), not its failure — inverted from the intended fallback semantics. Not fixable + by any dependency-topology constraint; requires a new `TaskStatus::Dormant` marker (excluded + from `ready_tasks()` dispatch, activated only by explicit recovery) or an explicit on-failure + edge concept distinct from `depends_on`. +- **N1:** reusing the existing Skip-BFS (`dag.rs:262-276`) to revive a fallback node would still + leave that node's own downstream subtree `Skipped` permanently — the BFS would need to exclude + the recovery target and its transitive closure. +- **N3:** `build_task_prompt()`'s `Completed`-only dependency filter + (`crates/zeph-orchestration/src/scheduler/router.rs:23-34`) silently drops a `Failed` source + task's `state_injection` — a rerouted fallback would dispatch with zero context from the failed + task, needing a targeted extension routed through the SEC-ORCH-01 sanitizer. + +If Mode 2 is redesigned with a real `Dormant`/on-failure-edge mechanism, the runtime status guard +on the reroute target (checking it is not unexpectedly `Running`/`Completed` at recovery time) +becomes load-bearing, not merely defensive, because a proper on-failure-edge fallback could +legitimately be reachable via other paths too. + +### FR-D-02: Idle-Timeout Progress-Signal Plumbing (Alt A) + +Deferred (BRD §5). Target design: a coalescing per-task `Arc`/`watch` progress +timestamp — explicitly **not** a second queued channel, since the existing completion-event +`mpsc::channel(64)` (`crates/zeph-orchestration/src/scheduler/mod.rs:518`) already drops events +under saturation, and multiplexing progress signals onto it risks evicting real completion +events. Requires cross-crate wiring: the `zeph-subagent` spawn path and the `zeph-core` +`RunInline` loop would both need to emit progress signals. Not built in v1; FR-005 ships the field +as a documented no-op instead. + +--- + +## 10. Traceability Matrix + +| Requirement | BRD Goal | Architect/Critic Source | +|-------------|----------|--------------------------| +| FR-001..FR-004 | BG-01, BG-04 | Architect base plan + v3 acceptance criterion 1 | +| FR-005 | BG-03 | Architect v3 decision (2); critic finding M-b | +| FR-006..FR-010 | BG-02, BG-04 | Architect v3 decision (3); critic re-confirmation of Mode-1 soundness | +| FR-011, FR-012 | BG-02, BG-05 | Architect v3 decision (4); critic re-confirmation ("correct and sufficient now that route_to is gone") | +| FR-013..FR-015 | BG-04 | Architect N2 (round 2, carried to v3); critic code-verified re-confirmation | +| FR-016 | BG-04 | Architect M2 correction (v3, supersedes v2's proposed resume re-scan); critic independent re-verification | +| FR-017..FR-019 | BG-01, BG-04 | Architect "Carried over unchanged (critic-endorsed)" section | +| FR-020 | BG-05 | Architect v3 acceptance criterion 7 | +| FR-021 | BG-04 | Critic finding M-a; resolved during spec formalization against `plan.rs:722-833` | +| FR-D-01 | (deferred) | Architect N5/N1/N3 retraction and deferral; critic "RESOLVED by deferral" | +| FR-D-02 | (deferred) | Architect v3 decision (2), "Carried to follow-up" | diff --git a/specs/075-orchestration-node-control-parity/tasks.md b/specs/075-orchestration-node-control-parity/tasks.md new file mode 100644 index 000000000..d9b6bea94 --- /dev/null +++ b/specs/075-orchestration-node-control-parity/tasks.md @@ -0,0 +1,148 @@ +--- +aliases: + - Orchestration Node Control Parity Tasks + - Node Timeout / Retry-Exhausted Recovery Tasks + - Tasks 6021 +tags: + - sdd + - tasks + - orchestration +created: 2026-07-13 +status: approved +related: + - "[[specs/075-orchestration-node-control-parity/plan]]" + - "[[specs/075-orchestration-node-control-parity/spec]]" +--- + +# Task Breakdown: Orchestration Node Control Parity (GitHub #6021) + +All tasks reference `[[specs/075-orchestration-node-control-parity/plan]]`. This is the developer's +primary implementation checklist alongside the architect/critic handoffs referenced there. +Implement in phase order; Phases 2-6 depend only on Phase 1, not on each other, and may be +parallelized across developers if desired. This document itself is a design artifact — per this +spec package's scope, no code is implemented as part of producing it (see spec.md §Out of Scope); +implementation is picked up by a future `new-feature`/`refactoring` team-develop session. + +--- + +## Phase 1: Data Model + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T1.1 | Add `TimeoutPolicy { run_timeout_secs, idle_timeout_secs }` struct | P1-1 | `crates/zeph-orchestration/src/graph.rs` (or new module) | `idle_timeout_secs` doc comment states "not enforced in v1" | +| T1.2 | Add `RecoveryAction { state_injection }` struct | P1-1 | same | No `route_to` field — designed for additive extension later | +| T1.3 | Add `timeout: Option` and `recovery: Option` to `TaskNode` | P1-2 | `crates/zeph-orchestration/src/graph.rs:379-452` | `#[serde(default, skip_serializing_if = "Option::is_none")]` on both | +| T1.4 | Update `TaskNode` module doctest if it asserts the full field list | P1-2 | same | Only if the existing doctest pattern requires it | +| T1.5 | Unit tests: serde round-trip (`Some`/`Some`, `None`/`None`, pre-feature JSON with no keys) | P1-3 | `graph.rs` | Required coverage | + +**Phase 1 gate:** `cargo nextest run -p zeph-orchestration` green before Phase 2-6. + +--- + +## Phase 2: `validate()` Recovery Guards + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T2.1 | Add reject guard: `recovery.is_some() && verify_predicate.is_some()` | P2-1 | `crates/zeph-orchestration/src/dag.rs:37-91` (`validate()`'s per-task loop, `:51-77`) | `Err(OrchestrationError::InvalidGraph(...))` naming the task index | +| T2.2 | Add warn guard: `recovery.is_some()` under effective `Skip`/`Ask` strategy | P2-2 | same | `tracing::warn!`, does not reject; effective-strategy computation needs `graph.default_failure_strategy` threaded in — resolve the `validate(tasks: &[TaskNode], ...)` vs. `&TaskGraph` signature question as part of this task | +| T2.3 | Unit test: reject case | P2-3 | `dag.rs` | Blocking acceptance criterion (SRS FR-011) | +| T2.4 | Unit test: warn case for `Skip` | P2-3 | same | | +| T2.5 | Unit test: warn case for `Ask` | P2-3 | same | | +| T2.6 | Unit test: `recovery` under `Abort`/`Retry` → no warning, `Ok` | P2-3 | same | | + +**Phase 2 gate:** `cargo nextest run -p zeph-orchestration` green before merge. + +--- + +## Phase 3: Mode-1 Recovery in `propagate_failure()` + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T3.1 | Add `try_recover(graph, failed_id) -> bool` helper | P3-1 | `crates/zeph-orchestration/src/dag.rs` | Sets `Completed` + synthetic `TaskResult`; `tracing::info!` on success | +| T3.2 | Call `try_recover` at the top of the `Abort` arm | P3-1 | `dag.rs:243-253` | On `true`, return `Vec::new()` (no cancellations) | +| T3.3 | Call `try_recover` at the top of the retry-exhausted branch of the `Retry` arm | P3-1 | `dag.rs:281-298` | Same short-circuit behavior | +| T3.4 | Unit test: `Abort`-default + `state_injection` → `Completed`, correct `result.output`, `graph.status` unchanged | P3-2 | `dag.rs` | Blocking (SRS FR-007, BRD SC-04) | +| T3.5 | Unit test: retry-exhausted `Retry` + `state_injection` → same end-state | P3-2 | same | Blocking (SRS FR-007) | +| T3.6 | Unit test: `recovery == None` on both paths → existing Abort-equivalent behavior unchanged | P3-2 | same | Blocking regression (BRD SC-01) | +| T3.7 | Unit test: recovered task's dependent becomes eligible via `ready_tasks()`'s `Pending` arm | P3-2 | `dag.rs` | Blocking (SRS FR-008) | +| T3.8 | Test: `Skip`/`Ask` strategy with `recovery` configured still ends `Skipped`/`Paused`, not `Completed` | P3-2 | `dag.rs` | Confirms guards from Phase 2 hold at runtime too | + +**Phase 3 gate:** `cargo nextest run -p zeph-orchestration` green before Phase 7. + +--- + +## Phase 4: Per-Task Timeout — Spawned Tasks + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T4.1 | Add `effective_run_timeout(&self, task_id) -> Duration` | P4-1 | `crates/zeph-orchestration/src/scheduler/tick/mod.rs` | Falls back to `self.task_timeout` when no override | +| T4.2 | Use `effective_run_timeout` in `check_timeouts()`'s filter predicate | P4-1 | `tick/mod.rs:727-768` | Replaces uniform `self.task_timeout` comparison | +| T4.3 | Use `effective_run_timeout` in `wait_event()`'s nearest-deadline computation | P4-2 | `tick/mod.rs:254-270` | Requires iterating `self.running` as `(id, r)` pairs | +| T4.4 | Unit test: two running tasks, one overridden (short), one default (long) — only the overridden one times out early | P4-3 | `tick/mod.rs` | Blocking (SRS FR-002, BRD SC-02) | +| T4.5 | Unit test: `wait_event()`'s computed wait reflects the nearer per-task deadline | P4-3 | same | Blocking (SRS FR-003) | +| T4.6 | Regression test: no overrides anywhere → identical timing to pre-feature | P4-3 | same | Blocking (BRD SC-01) | + +**Phase 4 gate:** `cargo nextest run -p zeph-orchestration` green before Phase 7. + +--- + +## Phase 5: Per-Task Timeout — `RunInline` Tasks + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T5.1 | Compute `effective_run_timeout` for the `RunInline` task at dispatch | P5-1 | `crates/zeph-core/src/agent/scheduler_loop.rs:258-` | Falls back to the graph-global `task_timeout_secs` | +| T5.2 | Add third `tokio::select!` branch (timeout) alongside the existing tool-loop and cancellation-token arms | P5-1 | same | Produces `TaskOutcome::Failed` on fire | +| T5.3 | Integration test: short override + slow tool loop → timeout branch fires, `TaskOutcome::Failed` | P5-2 | `scheduler_loop.rs` tests | Blocking (SRS FR-004, BRD SC-02) | +| T5.4 | Regression test: no override + fast tool loop → completes normally, timeout branch never fires | P5-2 | same | Blocking (BRD SC-01) | +| T5.5 | Integration test: `RunInline` + `timeout` + `recovery` both configured — timeout fires, recovery applies, dependents unblock | P5-2 | same | Cross-phase (Phase 3 + Phase 5) | + +**Phase 5 gate:** `cargo nextest run -p zeph-core --lib` green before Phase 7. + +--- + +## Phase 6: Config — `default_idle_timeout_secs` + +| # | Task | Plan Step | File | Notes | +|---|------|-----------|------|-------| +| T6.1 | Add `default_idle_timeout_secs: Option` field with `#[serde(default)]` | P6-1 | `crates/zeph-config/src/experiment.rs` | Doc comment states "RESERVED — not yet enforced" | +| T6.2 | Add named migration step (add-with-default) | P6-2 | `crates/zeph-config/src/migrate/mod.rs` (registry) + `steps.rs` (function) | Mirror a prior trivial add-only migration's shape | +| T6.3 | Add `--init` wizard prompt in `step_orchestration()` | P6-3 | `src/init/agents.rs:11` | Reserved/not-yet-enforced framing (NFR-OB-04) | +| T6.4 | Document field in `docs/src/` and any config.toml template | P6-4 | `docs/src/` | Same reserved wording | +| T6.5 | Unit test: default config → `None` | P6-5 | `experiment.rs` | | +| T6.6 | Unit test: TOML round-trip with explicit value | P6-5 | same | | +| T6.7 | Unit test: pre-feature config migrates cleanly to `None` | P6-5 | migration test module | Blocking (SRS FR-018, BRD SC-08) | + +**Phase 6 gate:** `cargo nextest run -p zeph-config` green before Phase 7. + +--- + +## Phase 7: Documentation and Mandatory Integration Points + +| # | Task | Integration Point | Path | Notes | +|---|------|--------------------|------|-------| +| T7.1 | Doc-annotate `ready_tasks()`'s `Ready` arm as load-bearing for recovery unblock | — | `crates/zeph-orchestration/src/dag.rs:185-191` | SRS FR-020, exact wording in spec.md §4 | +| T7.2 | Doc-note on `OrchestrationMetrics`/`finalize_plan_*` confirming recovered-node status-derived counting | — | `crates/zeph-core/src/metrics.rs:104-105` or `crates/zeph-core/src/agent/plan.rs:722,801` | No code change — resolves SRS FR-021 | +| T7.3 | Confirm no CLI/TUI surface needed beyond config.toml/`--init` (#2/#3) | #2, #3 | PR description | Documented rationale, not silent — mirrors `task_timeout_secs` precedent | +| T7.4 | Create testing playbook | #6 | `/Users/rabax/Dev/zeph/.local/testing/playbooks/orchestration-node-control-parity.md` | Main-repo path; scenarios per plan.md P7-3 | +| T7.5 | Add coverage-status rows | #7 | `/Users/rabax/Dev/zeph/.local/testing/coverage-status.md` | Rows: per-task timeout (spawned + RunInline), Mode-1 recovery, validate() guards, config field — status `Untested` | +| T7.6 | Update `CHANGELOG.md` `[Unreleased]` | — | `CHANGELOG.md` | Root; note idle-timeout reserved status and Mode-2 deferral | +| T7.7 | Register spec in `specs/README.md` and `specs/MOC-specs.md` | — | `specs/` | **Outside this spec package's write scope (sdd role is restricted to `specs/075-orchestration-node-control-parity/`) — team-lead action, not a developer task** | + +--- + +## Acceptance Criteria (for PR merge) + +- [ ] All Phase 1-6 unit/integration tests pass: `cargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins` +- [ ] `cargo +nightly fmt --check` +- [ ] `cargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warnings` +- [ ] Rustdoc gate: `RUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"` +- [ ] Doc-tests: `cargo test --doc --workspace --features "desktop,ide,server,chat,pdf,scheduler"` +- [ ] Default-off regression tests present and passing (T1.5 pre-feature-JSON case, T3.6, T4.6, T5.4) +- [ ] Mode-1 recovery tests present and passing for both `Abort`-default and retry-exhausted `Retry` (T3.4, T3.5) +- [ ] `validate()` guard tests present and passing (T2.3-T2.6) +- [ ] Cascade-precedence behavior verified live or via integration test (no code change needed — existing ordering — but the PR description must state this was explicitly checked, not merely assumed) +- [ ] Async-supervision scan shows zero new `tokio::spawn()` sites introduced by this PR +- [ ] `CHANGELOG.md` updated +- [ ] Testing playbook + coverage-status rows added (main-repo `.local/testing/` path) +- [ ] `specs/README.md` and `specs/MOC-specs.md` register `orchestration-node-control-parity` (team-lead action) +- [ ] Follow-up issues filed for Mode 2 (`route_to` redesign, N5/N1/N3) and Alt A (idle-timeout progress-signal plumbing) — team-lead action, not part of this PR diff --git a/specs/MOC-specs.md b/specs/MOC-specs.md index b23f9d03c..7d929d3c6 100644 --- a/specs/MOC-specs.md +++ b/specs/MOC-specs.md @@ -93,6 +93,7 @@ status: moc ### Planning & DAG - [[009-orchestration/spec|Orchestration & Planning]] — DAG planner, DagScheduler, AgentRouter, /plan command, plan template cache, VMAO adaptive replanning, cascade-aware DAG routing with CascadeDetector, tree-optimized dispatch; defines strategy for multi-step task execution - [[074-orchestration-hitl-interrupt/spec|Declarative HITL Interrupt]] — LangGraph `interrupt()` parity: `TaskNode.interrupt_before`/`resolved_input` pre-dispatch gate, `TaskGraph.pause_reason` (blob-only, no `DurablePromise` in Phase 1), `/plan provide ` command, `GraphStatus::Paused` reuse; extends [[009-orchestration/spec|Orchestration & Planning]]; GitHub #5918 +- [[075-orchestration-node-control-parity/spec|Node Timeout / Retry-Exhausted Recovery]] — LangGraph `TimeoutPolicy`/error-handler parity: per-task `TimeoutPolicy` (`run_timeout_secs` enforced on spawned + RunInline tasks, `idle_timeout_secs` defined but a documented no-op in v1), `RecoveryAction { state_injection }` Mode-1 substitute-and-continue recovery on terminal failure (cascade-abort takes precedence, no resume re-scan needed); `route_to` reroute-to-alternate (Mode 2) deferred — dependency-based dormancy proved inverted; extends [[009-orchestration/spec|Orchestration & Planning]]; GitHub #6021 --- @@ -247,6 +248,7 @@ status: moc | 072 | [[072-multimodal-mcp-passthrough/spec\|Multimodal MCP Passthrough]] | specify | draft | | 073 | [[073-orch-ensemble-merge/spec\|ORCH Ensemble-Merge]] | specify | approved | | 074 | [[074-orchestration-hitl-interrupt/spec\|Declarative HITL Interrupt]] | tasks | draft | +| 075 | [[075-orchestration-node-control-parity/spec\|Node Timeout / Retry-Exhausted Recovery]] | tasks | approved | --- diff --git a/specs/README.md b/specs/README.md index 6d8a7399c..1230adc16 100644 --- a/specs/README.md +++ b/specs/README.md @@ -167,3 +167,4 @@ Spec IDs (001–069) follow a logical grouping: | `072-multimodal-mcp-passthrough/spec.md` | Multimodal MCP `ContentBlock` passthrough: opt-in per-server (default OFF, hard-blocked for `Sandboxed`) decode of `ContentBlock::Image` at `McpToolExecutor::execute_tool_call`, new `MediaSanitizer` (magic-byte sniff, format allowlist, size/dimension/pixel caps via `image` crate on `spawn_blocking`), `ToolOutput.media`/`ToolResultClassification.media` plumbing (+`#[derive(Default)]` 271-site migration), sibling `MessagePart::Image` emitted in `process_one_tool_result` gated on a concrete vision-capable tier (never a 400/422), ephemeral-only lifetime (stripped before SQLite/Qdrant/durable-JSONL persistence in `Agent::persist_message`), redacted `ImageData` `Debug`; Audio/blob passthrough deferred (Ask-First `MessagePart` variant); GitHub #5366 [draft] | `zeph-mcp`, `zeph-tools`, `zeph-sanitizer`, `zeph-config`, `zeph-core`, `zeph-agent-persistence`, `zeph-llm` | | `073-orch-ensemble-merge/spec.md` | ORCH deterministic verifier ensemble-merge (arXiv:2602.01797): N-fold parallel `chat_typed::` dispatch over configured provider `members` via inline `join_all` (zero new `tokio::spawn`), pure deterministic binary-majority merge on `complete` (not the 4-way ordinal `GapSeverity`), merged `confidence` = mean of winning-side members' self-reported confidence (never `agreement_ratio`, preserving the unchanged `should_replan` gate), errored/timed-out members excluded from the ballot (not fail-open votes), derived quorum with graceful fallback to the existing single-provider `verify_provider` path, telemetry-only `EnsembleTracker` (no EMA-gated participation), load-time odd/≥3/no-duplicate `members` validation; opt-in, default OFF; GitHub #5912 [approved] | `zeph-orchestration`, `zeph-config`, `zeph-core`, `src/` (binary) | | `074-orchestration-hitl-interrupt/spec.md` | Declarative task-level HITL interrupt for the orchestration DAG (LangGraph `interrupt()` parity, #5918): `TaskNode.interrupt_before`/`resolved_input` + `TaskGraph.pause_reason: Option` (blob-only, ALT-1 — no `DurablePromise` in Phase 1, `PromiseId` forward-compat hook only), `GraphStatus::Paused` reused not extended, pre-dispatch gate in `dispatch_ready_tasks` (leaves gated `TaskStatus::Ready` to avoid a `check_graph_completion` false-deadlock), `/plan provide ` command, prompt-interpolation injection into `build_task_prompt`, `/plan retry` blocked on an `AwaitingInput` pause, `interrupt_enabled` config toggle (default off); Phase 2 (imperative mid-loop interrupt) and `AcpPermissionGate`→`DurablePromise` migration deferred as follow-ups; GitHub #5918 [draft] | `zeph-orchestration`, `zeph-core`, `zeph-config`, `zeph-commands` | +| `075-orchestration-node-control-parity/spec.md` | Orchestration node control parity (LangGraph `TimeoutPolicy`/error-handler parity, #6021): optional per-task `TimeoutPolicy { run_timeout_secs, idle_timeout_secs }` on `TaskNode` — `run_timeout` fully enforced (spawned via `check_timeouts()` per-task effective deadline, RunInline via a third `tokio::time::timeout` branch in the inline `select!`), `idle_timeout_secs` defined/config-surfaced but a documented no-op in v1 (no progress-signal plumbing exists yet; eviction-safe coalescing `Arc` design spec'd as the Alt-A future target); optional `RecoveryAction { state_injection }` — Mode-1-only substitute-and-continue recovery on terminal `Abort`-default or retry-exhausted `Retry` failure (failed node → `Completed` with synthetic `TaskResult`, zero new consumption machinery), inert under `Skip`/`Ask` (validate warns), rejected alongside `verify_predicate`; cascade-abort takes precedence over recovery (existing event-path ordering, no code reordering) with the pre-existing timeout-vs-event asymmetry documented; no resume re-scan needed (same-tick snapshot atomicity); `route_to` reroute-to-alternate (Mode 2) explicitly deferred — its `depends_on`-based dormancy is inverted (fires on the source task's success, not failure) and needs a `TaskStatus::Dormant`/on-failure-edge redesign; new `default_idle_timeout_secs` config field with full `--init`/`--migrate-config` integration; three-round architect/critic design review (final verdict: minor/approved); GitHub #6021 [approved] | `zeph-orchestration`, `zeph-core`, `zeph-config` |