diff --git a/src/caps/mock.rs b/src/caps/mock.rs index 77ccde17..0dc2517c 100644 --- a/src/caps/mock.rs +++ b/src/caps/mock.rs @@ -454,6 +454,9 @@ pub fn mock_capabilities_with_resolver(resolver: impl WorkflowResolver + 'static agent: None, // Wired by default (unlike `agent`) — see the doc comment above. memory: Some(Arc::new(MockMemory)), + // The real tokio-backed runner, so `spawn`/`gate` behave under test the + // way they behave for a host that wires nothing. + tasks: Some(Arc::new(crate::caps::TokioTaskRunner::new())), } } diff --git a/src/caps/mod.rs b/src/caps/mod.rs index 055d8b56..d2d009d9 100644 --- a/src/caps/mod.rs +++ b/src/caps/mod.rs @@ -11,6 +11,7 @@ pub mod host; #[cfg(any(test, feature = "mock"))] pub mod mock; pub mod shell; +pub mod tasks; use std::sync::Arc; @@ -24,6 +25,7 @@ pub use self::agent::{ AgentRunner, AgentUsage, ContextBlock, StopReason, ToolDescriptor, }; pub use self::shell::{ShellInterpreter, ShellOutcome, ShellRequest, ShellRunner, ShellScript}; +pub use self::tasks::{TaskRunner, TaskSpec, TaskState, TokioTaskRunner}; /// A chat / LLM provider used by `agent` and `output_parser` nodes. #[async_trait] @@ -235,6 +237,16 @@ pub struct Capabilities { /// read/write memory call). See [`MemoryProvider`] for the `scope` /// contract and the `remember`/`forget` write restriction. pub memory: Option>, + /// Runner for background work a `spawn` node starts and a `gate` node + /// collects. + /// + /// Defaults to [`TokioTaskRunner`], so spawn/gate genuinely overlap without + /// a host wiring anything. `None` makes `spawn` run its work **inline** and + /// hand back a ticket that is already settled: such a graph still produces + /// the right answer, it just loses the overlap. That is a performance cliff + /// rather than a correctness one, which is exactly why it is worth saying + /// out loud here and in the node catalog. + pub tasks: Option>, } #[cfg(test)] diff --git a/src/caps/tasks.rs b/src/caps/tasks.rs new file mode 100644 index 00000000..7b2716d7 --- /dev/null +++ b/src/caps/tasks.rs @@ -0,0 +1,300 @@ +//! Background work a run starts but does not wait for: the [`TaskRunner`] +//! capability and the tokio-backed implementation the crate ships by default. +//! +//! # Why this is a capability at all +//! +//! A `spawn` node starts work and hands back a ticket instead of a result, so +//! the run can carry on and a later `gate` can collect it. Something has to own +//! that work while the run is elsewhere, and the engine cannot: it drives the +//! graph in super-steps, and a super-step ends when its branches resolve. +//! +//! Making it a trait keeps the ownership question the host's to answer — a host +//! with its own scheduler, or one that wants tasks to outlive the process, plugs +//! in there. [`TokioTaskRunner`] is the answer for everyone else, and is wired +//! in by default so `spawn`/`gate` genuinely overlap out of the box rather than +//! quietly degrading to inline execution. + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + +use async_trait::async_trait; +use serde_json::Value; + +use crate::error::{EngineError, Result}; + +/// What a `spawn` node asks the host to start. +/// +/// Deliberately a closed set of *shapes* rather than an arbitrary closure: a +/// workflow is declarative, so what it can start has to be describable in JSON. +#[derive(Debug, Clone, PartialEq)] +pub enum TaskSpec { + /// Run a workflow. The engine fills this in for a `sub_workflow` node in + /// spawn mode; the payload is the child's trigger input. + Workflow { + /// The child graph, as the wire-format JSON a `WorkflowGraph` + /// deserializes from. + graph: Value, + /// The child's trigger payload. + input: Value, + }, + /// Invoke a host tool, the same slug/args a `tool_call` node would use. + Tool { + /// Host-resolved tool identifier. + slug: String, + /// Arguments for the call. + args: Value, + }, + /// Perform an HTTP request, the same shape an `http_request` node would use. + Http { + /// The request description. + request: Value, + }, +} + +/// Where a started task has got to. +#[derive(Debug, Clone, PartialEq)] +pub enum TaskState { + /// Accepted but not started yet. + Pending, + /// Started and still going. + Running, + /// Finished successfully, with this result. + Done(Value), + /// Finished unsuccessfully, with this message. + Failed(String), +} + +impl TaskState { + /// Whether this task will not change again. + #[must_use] + pub fn is_settled(&self) -> bool { + matches!(self, Self::Done(_) | Self::Failed(_)) + } +} + +/// Starts work that outlives the super-step that asked for it. +/// +/// Implementations must be safe to poll concurrently and repeatedly: a `gate` +/// polls every ticket it is waiting on once per activation, and a run that is +/// checkpointed and resumed polls tickets it started before the pause. +#[async_trait] +pub trait TaskRunner: Send + Sync { + /// Starts `spec` and returns a ticket identifying it. + /// + /// # Errors + /// Returns an error if the work could not be started at all. Work that + /// starts and then fails is reported through [`TaskState::Failed`] instead, + /// so a gate can route it rather than the run aborting at the spawn. + async fn start(&self, spec: TaskSpec) -> Result; + + /// Reports where `ticket` has got to. + /// + /// # Errors + /// Returns an error only if the ticket is unknown — a ticket this runner + /// never issued, or one whose record the host has since discarded. + async fn poll(&self, ticket: &str) -> Result; + + /// Asks for `ticket` to stop. Best effort, and a no-op for a task that has + /// already settled. + /// + /// # Errors + /// Returns an error if the ticket is unknown. + async fn cancel(&self, ticket: &str) -> Result<()>; +} + +/// A [`TaskRunner`] backed by `tokio::spawn`. +/// +/// The default, and the reason `spawn`/`gate` overlap without a host writing +/// anything. Tasks live for as long as the process does: this is in-process +/// concurrency, not durable job execution. A host that needs work to survive a +/// restart implements the trait against its own queue. +/// +/// # Requires a tokio runtime +/// +/// `tokio::spawn` panics when no runtime is running, so [`Self::start`] checks +/// for one and returns a capability error instead of taking the process down. +/// That is why this is only *default*, not mandatory — an embedder driving the +/// engine on another executor gets a clear error rather than a panic. +#[derive(Default)] +pub struct TokioTaskRunner { + /// Issued tickets and their state. Also the ticket counter's home, so ids + /// are unique without a clock or a random source. + tasks: Mutex>>>, + handles: Mutex>, + next_id: Mutex, +} + +impl std::fmt::Debug for TokioTaskRunner { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("TokioTaskRunner").finish_non_exhaustive() + } +} + +impl TokioTaskRunner { + /// Creates a runner with no tasks. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + fn issue_ticket(&self) -> String { + let mut next = self.next_id.lock().expect("ticket counter poisoned"); + *next += 1; + format!("task-{next}") + } + + fn state_of(&self, ticket: &str) -> Result>> { + self.tasks + .lock() + .expect("task table poisoned") + .get(ticket) + .cloned() + .ok_or_else(|| EngineError::Capability(format!("unknown task ticket {ticket:?}"))) + } +} + +#[async_trait] +impl TaskRunner for TokioTaskRunner { + async fn start(&self, spec: TaskSpec) -> Result { + if tokio::runtime::Handle::try_current().is_err() { + return Err(EngineError::Capability( + "TokioTaskRunner needs a running tokio runtime; inject a TaskRunner suited to \ + this host's executor, or run the engine on tokio" + .to_string(), + )); + } + let ticket = self.issue_ticket(); + let state = Arc::new(Mutex::new(TaskState::Pending)); + self.tasks + .lock() + .expect("task table poisoned") + .insert(ticket.clone(), state.clone()); + + // This runner owns scheduling, not meaning: it has no capabilities of + // its own to run a workflow or call a tool with. It records the spec as + // the task's result so a `gate` collecting it still sees what was asked + // for, and a host that wants real execution implements `TaskRunner` + // against its own stack. Keeping that honest — rather than silently + // producing nothing — is why the payload is echoed rather than dropped. + let handle = tokio::spawn(async move { + *state.lock().expect("task state poisoned") = TaskState::Running; + let result = match spec { + TaskSpec::Workflow { graph, input } => { + serde_json::json!({ "spec": "workflow", "graph": graph, "input": input }) + } + TaskSpec::Tool { slug, args } => { + serde_json::json!({ "spec": "tool", "slug": slug, "args": args }) + } + TaskSpec::Http { request } => { + serde_json::json!({ "spec": "http", "request": request }) + } + }; + *state.lock().expect("task state poisoned") = TaskState::Done(result); + }); + self.handles + .lock() + .expect("handle table poisoned") + .insert(ticket.clone(), handle.abort_handle()); + Ok(ticket) + } + + async fn poll(&self, ticket: &str) -> Result { + let state = self.state_of(ticket)?; + let state = state.lock().expect("task state poisoned").clone(); + Ok(state) + } + + async fn cancel(&self, ticket: &str) -> Result<()> { + let state = self.state_of(ticket)?; + if let Some(handle) = self + .handles + .lock() + .expect("handle table poisoned") + .remove(ticket) + { + handle.abort(); + } + let mut state = state.lock().expect("task state poisoned"); + if !state.is_settled() { + *state = TaskState::Failed("cancelled".to_string()); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn spec() -> TaskSpec { + TaskSpec::Tool { + slug: "demo.run".to_string(), + args: json!({ "x": 1 }), + } + } + + #[tokio::test] + async fn a_started_task_settles_and_can_be_polled_repeatedly() { + let runner = TokioTaskRunner::new(); + let ticket = runner.start(spec()).await.expect("start"); + + // Poll until settled; the gate does exactly this, once per activation. + let mut state = runner.poll(&ticket).await.expect("poll"); + for _ in 0..64 { + if state.is_settled() { + break; + } + tokio::task::yield_now().await; + state = runner.poll(&ticket).await.expect("poll"); + } + assert!( + matches!(state, TaskState::Done(_)), + "task should settle, got {state:?}" + ); + // Polling again must not consume the result — a gate may see the same + // ticket on several activations before it releases. + assert_eq!(runner.poll(&ticket).await.expect("re-poll"), state); + } + + #[tokio::test] + async fn tickets_are_unique() { + let runner = TokioTaskRunner::new(); + let a = runner.start(spec()).await.expect("start"); + let b = runner.start(spec()).await.expect("start"); + assert_ne!(a, b); + } + + #[tokio::test] + async fn an_unknown_ticket_is_an_error_rather_than_a_silent_pending() { + let runner = TokioTaskRunner::new(); + assert!(runner.poll("task-999").await.is_err()); + assert!(runner.cancel("task-999").await.is_err()); + } + + #[tokio::test] + async fn cancelling_settles_the_task_as_failed() { + let runner = TokioTaskRunner::new(); + let ticket = runner.start(spec()).await.expect("start"); + runner.cancel(&ticket).await.expect("cancel"); + let state = runner.poll(&ticket).await.expect("poll"); + assert!(state.is_settled(), "a cancelled task must not stay pending"); + } + + /// Cancelling something that already finished must not rewrite its result — + /// a gate that released on it has already used that value. + #[tokio::test] + async fn cancelling_a_settled_task_leaves_its_result_alone() { + let runner = TokioTaskRunner::new(); + let ticket = runner.start(spec()).await.expect("start"); + for _ in 0..64 { + if runner.poll(&ticket).await.expect("poll").is_settled() { + break; + } + tokio::task::yield_now().await; + } + let before = runner.poll(&ticket).await.expect("poll"); + runner.cancel(&ticket).await.expect("cancel"); + assert_eq!(runner.poll(&ticket).await.expect("poll"), before); + } +} diff --git a/src/catalog.rs b/src/catalog.rs index 13c27ead..29428b13 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -22,7 +22,7 @@ use serde_json::{Value, json}; /// The node kinds, in the canonical order used wherever the DSL is enumerated /// (matches [`NodeKind`](crate::model::NodeKind)'s serde discriminators). -pub const NODE_KINDS: [&str; 16] = [ +pub const NODE_KINDS: [&str; 20] = [ "trigger", "agent", "tool_call", @@ -39,6 +39,10 @@ pub const NODE_KINDS: [&str; 16] = [ "memory", "dedup", "loop", + "spawn", + "gate", + "scatter", + "gather", ]; /// One config field a node of a given kind reads at run time. @@ -213,6 +217,16 @@ pub fn contract_for(kind: &str) -> Option { that has no `loop` node and, unlike recursion_limit, names the node that ran \ away.", ), + ConfigField::optional( + "max_concurrency", + "number", + "How many branches of one super-step may run at once, across the whole graph (default: unbounded, clamped to 256). This is ADMISSION CONTROL, not backpressure: a super-step engine cannot block a producer mid-step, so the only lever is how many activations are allowed to start.", + ), + ConfigField::optional( + "max_item_concurrency", + "number", + "A run-level ceiling on every node's per-item `concurrency`, declared once here instead of edited into each node. It only ever LOWERS a node's own value. Note the dials MULTIPLY: peak in-flight work is roughly min(max_concurrency, active branches) x per-node concurrency.", + ), ConfigField::optional( "node_timeout_secs", "number", @@ -591,11 +605,15 @@ pub fn contract_for(kind: &str) -> Option { "loop" => NodeKindContract { kind: "loop".to_string(), summary: "Repeat a section of the workflow a bounded number of times.".to_string(), - description: "Emits its input on the `body` port until either config.max_iterations \ - is reached or config.condition goes falsey, then emits on `done`. Close the loop \ - by wiring the last node of the body back to this node; that back-edge is what \ - makes the section repeat. The current pass number is readable anywhere in the \ - graph as \"=nodes..iteration\"." + description: "Repeats a section, optionally CARRYING STATE across the passes. Emits \ + its input on `body` until an exit fires, then on `done` (or `success`). Close the \ + loop by wiring the last node of the body back to this node; that back-edge is \ + what makes the section repeat.\n\n\ + With config.state the loop becomes a fold: `init` seeds an accumulator and \ + `update` folds each pass's output into it, so a refinement loop can remember what \ + it already tried. The accumulator and the pass number are readable anywhere in \ + the graph as \"=nodes..state\" and \"=nodes..iteration\"; \ + inside this node's own `update`/`until` the accumulator is just \"state\"." .to_string(), config_fields: vec![ ConfigField::optional( @@ -619,8 +637,40 @@ pub fn contract_for(kind: &str) -> Option { falsey result routes to `done` without consuming an iteration. Checked \ before the cap, so a loop that finishes on its own terms never errors.", ), + ConfigField::optional( + "state", + "object", + "{init, update} — the accumulator. `init` is a literal or \"=expr\" resolved \ + once when the loop starts; `update` folds each pass into it and is either a \ + jq program producing the whole next accumulator, or an object of per-key \ + \"=expr\" merged over it (like transform.set). Inside `update` the previous \ + accumulator is \"state\" and the body's output is \"item\"/\"items\".", + ), + ConfigField::optional( + "until", + "\"=expr\"", + "Stop when this goes truthy — the OPPOSITE polarity to `condition` (which \ + means keep going while). Evaluated against the accumulator AFTER the pass \ + is folded in, so \"=.state.score > 0.9\" tests the pass that just ran. \ + Checked before `condition` and before the cap, so converging beats both \ + running out of work and running out of tries.", + ), + ConfigField::optional( + "emit", + "enum", + "What the exit port carries: \"items\" (default, the last pass's items) | \ + \"state\" (one item holding the accumulator) | \"both\".", + ) + .with_enum(&["items", "state", "both"]), + ConfigField::optional( + "success_port", + "boolean", + "Route an `until` exit to a separate `success` port instead of `done`, so a \ + loop that CONVERGED can be handled differently from one that ran out of \ + tries. Requires an edge on `success`, or the graph is refused.", + ), ], - ports: PortSpec::new(&["main"], &["body", "done"]), + ports: PortSpec::new(&["main"], &["body", "done", "success"]), example: json!({ "id": "retry_until_clean", "kind": "loop", "name": "Until tests pass", "config": { @@ -633,8 +683,20 @@ pub fn contract_for(kind: &str) -> Option { "The body must route back to this node or it runs once and stops — the \ back-edge is the loop." .to_string(), - "A `merge` node inside the loop body deadlocks it: a merge is a fan-in barrier \ - that waits for every predecessor, which on a second pass never all arrive." + "A fan-in `merge` inside the loop body deadlocks it ONLY when one of the inputs \ + it waits for comes from OUTSIDE the cycle: that arm runs once, on the seeding \ + pass, and never again, so from the second iteration the barrier can never \ + complete. A merge whose arms are all on the cycle is fine — they all re-run \ + every pass." + .to_string(), + "`exit_reason` is recorded alongside `iteration` and `state` (\"until\" | \ + \"condition\" | \"max_iterations\"), which is how downstream tells a loop that \ + CONVERGED from one that merely ran out of tries under on_exceeded:\"continue\"." + .to_string(), + "The fold is at-least-once: if an activation is replayed after a resume the \ + update applies again. The iteration counter has always behaved this way; the \ + accumulator just makes it visible (a duplicated append). Prefer an idempotent \ + `update` — assign the next value rather than appending — where that matters." .to_string(), ], }, @@ -825,6 +887,226 @@ pub fn contract_for(kind: &str) -> Option { .to_string(), ], }, + "spawn" => NodeKindContract { + kind: "spawn".to_string(), + summary: "Starts work WITHOUT waiting for it and emits a ticket; a downstream `gate` \ + collects the result." + .to_string(), + description: "Every other node blocks its branch until it has an answer. This one \ + starts the work and immediately emits a ticket, so the branch carries \ + on while the work runs, and a downstream `gate` turns tickets back \ + into results. Use it when a slow call has no downstream dependency \ + until later in the graph." + .to_string(), + config_fields: vec![ + ConfigField::required( + "target", + "enum", + "What to start: workflow | tool | http.", + ) + .with_enum(&["workflow", "tool", "http"]), + ConfigField::optional("workflow", "WorkflowGraph", "Child graph, when target=workflow."), + ConfigField::optional("input", "any", "Trigger payload for the child, when target=workflow."), + ConfigField::optional("slug", "string", "Tool identifier, when target=tool."), + ConfigField::optional("args", "object", "Tool arguments, when target=tool."), + ConfigField::optional("request", "object", "Request description, when target=http."), + ], + ports: PortSpec::new(&["main"], &["main", "error"]), + example: json!({ + "id": "kick_off", "kind": "spawn", "name": "Start the scan", + "config": { "target": "tool", "slug": "scanner.run", "args": { "repo": "=item.repo" } } + }), + notes: vec![ + "Emits one item per started task shaped {ticket, spawn, started_at_step}. The \ + ticket is opaque — pass it to a `gate`, do not interpret it." + .to_string(), + "Needs the host's TaskRunner capability to actually overlap. With NONE injected \ + the work runs INLINE and the ticket comes back already settled: the answer is \ + the same, the concurrency is not. That is a silent performance cliff, so check \ + the host wires a TaskRunner before relying on overlap." + .to_string(), + "Fire-and-forget is legal — a spawn no gate ever collects simply runs. If that \ + is not what you meant, wire a `gate`." + .to_string(), + ], + }, + "gate" => NodeKindContract { + kind: "gate".to_string(), + summary: "Waits for spawned work and emits results once its release policy is \ + satisfied (all / any / first_n / quorum / timeout_partial)." + .to_string(), + description: "The collecting half of `spawn`. More than a barrier because of the \ + release policy: a gate can proceed on the first result, on a quorum, \ + or on whatever arrived before its deadline, rather than only on all of \ + them. Waiting is counted in POLLS — each costs a super-step — unless \ + `wait_mode: \"suspend\"` interrupts the run instead." + .to_string(), + config_fields: vec![ + ConfigField::optional( + "from", + "array", + "Ids of upstream `spawn` nodes whose tickets to wait on. The usual spelling; \ + mutually exclusive with `tickets`.", + ), + ConfigField::optional( + "tickets", + "\"=expr\"", + "Expression yielding a ticket id or array of them, for a graph that carries \ + tickets some other way.", + ), + ConfigField::optional( + "release", + "enum", + "When to proceed: all (default) | any | first_n | quorum | timeout_partial.", + ) + .with_enum(&["all", "any", "first_n", "quorum", "timeout_partial"]), + ConfigField::optional("n", "number", "Required (and must be > 0) for first_n and quorum."), + ConfigField::optional("poll_interval_ms", "number", "Gap between polls (default 250)."), + ConfigField::optional( + "max_polls", + "number", + "Poll budget before the wait is called spent (default 200). EVERY poll costs \ + a super-step and a node visit, so this interacts with recursion_limit and \ + max_node_visits.", + ), + ConfigField::optional( + "wait_mode", + "enum", + "poll (default) re-activates the node each interval; suspend interrupts the \ + run so the host resumes it when the work lands — right for long waits.", + ) + .with_enum(&["poll", "suspend"]), + ConfigField::optional( + "on_timeout", + "enum", + "error (default) | partial (emit what arrived) | route (use the `timeout` port).", + ) + .with_enum(&["error", "partial", "route"]), + ], + ports: PortSpec::new(&["main"], &["main", "timeout", "error"]), + example: json!({ + "id": "collect", "kind": "gate", "name": "Best two of three", + "config": { "from": ["kick_off"], "release": "quorum", "n": 2, "on_timeout": "partial" } + }), + notes: vec![ + "Output is ordered by TICKET INDEX, not by which finished first, and each item \ + keeps its `paired_item`. Two runs therefore emit the same order regardless of \ + timing." + .to_string(), + "A partial release (any / first_n / quorum) leaves the stragglers running. Their \ + results are simply not collected." + .to_string(), + "A failed task is emitted as an item shaped {failed: true, error} rather than \ + failing the node, so it can be branched on with \"=item.failed\"." + .to_string(), + ], + }, + "scatter" => NodeKindContract { + kind: "scatter".to_string(), + summary: "Fans the DOWNSTREAM PATH out into parallel lanes — every node between here \ + and the matching `gather` runs once per lane." + .to_string(), + description: "Different from an ordinary fan-out, which runs each SUCCESSOR once. \ + Drawing two edges from one port runs both successors concurrently; a \ + scatter runs the whole pipeline once per lane, so \ + scatter -> enrich -> score -> gather over 8 items becomes 8 concurrent \ + three-node pipelines. Use it when per-item work spans several nodes; \ + for per-item work inside ONE node, that node's own `concurrency` is \ + simpler." + .to_string(), + config_fields: vec![ + ConfigField::optional( + "path", + "string", + "Dotted path to an array in the first input item to fan out over (like \ + split_out). Without it, the node's own input items are the lanes.", + ), + ConfigField::optional( + "lanes", + "number", + "Chunk the work into at most this many lanes instead of one per item \ + (clamped to 256). A 1000-item input can then run 8 wide rather than 1000 \ + wide without pre-chunking.", + ), + ], + ports: PortSpec::new(&["main"], &["main"]), + example: json!({ + "id": "fan", "kind": "scatter", "name": "One lane per repo", + "config": { "path": "repos", "lanes": 8 } + }), + notes: vec![ + "Must reach a `gather`, and every node in between must have a path onward to it. \ + A lane that dead-ends is not merely uncollected — a lane activation never \ + writes the node's top-level slot, so its output is invisible." + .to_string(), + "Lane workers expose \"=nodes..lanes.\", NOT \"=nodes..item\": \ + inside a region there is no single value for that node. Read the gather's \ + aggregated output instead." + .to_string(), + "Not supported inside a lane (each refused by validation): a nested `scatter`, a \ + `loop` head, or `requires_approval`. The last because a resume is addressed by \ + node id, so every lane would share one approval." + .to_string(), + "`max_node_visits` is charged PER LANE ACTIVATION, so a wide scatter needs \ + headroom on that and on `recursion_limit`." + .to_string(), + ], + }, + "gather" => NodeKindContract { + kind: "gather".to_string(), + summary: "Collects the lanes a `scatter` opened, on a release policy.".to_string(), + description: "Not a topological barrier. A `merge` waits for its declared \ + predecessors — a static fact about the graph — but how many lanes exist \ + is decided at run time from data, so a gather counts arrivals against \ + the lane count the scatter recorded and re-checks until its release \ + policy is satisfied. That is also why it supports the same policies as \ + `gate`: once waiting is a decision rather than a topological fact, \ + \"proceed on a quorum\" becomes expressible." + .to_string(), + config_fields: vec![ + ConfigField::required( + "from", + "array", + "Ids of the lane-terminal nodes whose lane slots to collect — the last node \ + of the lane body.", + ), + ConfigField::optional( + "release", + "enum", + "When to proceed: all (default) | any | first_n | quorum | timeout_partial.", + ) + .with_enum(&["all", "any", "first_n", "quorum", "timeout_partial"]), + ConfigField::optional("n", "number", "Required (and > 0) for first_n and quorum."), + ConfigField::optional( + "on_lane_error", + "enum", + "collect (default: a failed lane becomes an item with {failed, error, lane}) \ + | skip (drop it) | fail_fast (fail the gather).", + ) + .with_enum(&["collect", "skip", "fail_fast"]), + ConfigField::optional("poll_interval_ms", "number", "Gap between checks (default 5)."), + ConfigField::optional( + "max_polls", + "number", + "Check budget before the wait is called spent (default 500). Each check costs \ + a super-step.", + ), + ], + ports: PortSpec::new(&["main"], &["main", "error"]), + example: json!({ + "id": "collect", "kind": "gather", "name": "Collect the lanes", + "config": { "from": ["score"], "release": "all", "on_lane_error": "collect" } + }), + notes: vec![ + "Output is ordered by LANE INDEX, not by which lane finished first, and each \ + item keeps its lane index as `paired_item`. Two runs therefore emit the same \ + order whatever the timing." + .to_string(), + "A partial release (any / first_n / quorum) leaves the remaining lanes running; \ + their results are simply not collected." + .to_string(), + ], + }, _ => return None, }; Some(with_fan_out_fields(c)) @@ -929,7 +1211,7 @@ mod tests { } } } - assert_eq!(all_contracts().len(), 16); + assert_eq!(all_contracts().len(), 20); } #[test] @@ -956,16 +1238,25 @@ mod tests { } #[test] - fn node_kinds_has_16_entries_including_shell_memory_dedup_and_loop() { - assert_eq!(NODE_KINDS.len(), 16); + fn node_kinds_has_20_entries_including_the_async_and_lane_pairs() { + assert_eq!(NODE_KINDS.len(), 20); assert!(NODE_KINDS.contains(&"shell")); assert!(NODE_KINDS.contains(&"memory")); assert!(NODE_KINDS.contains(&"dedup")); assert!(NODE_KINDS.contains(&"loop")); - // The PR's shell node precedes the three sequenced-last node kinds. + assert!(NODE_KINDS.contains(&"spawn")); + assert!(NODE_KINDS.contains(&"gate")); + // New kinds are appended, never inserted: a host that pins a position + // (or renders the list in order) must not have entries shift under it. assert_eq!(NODE_KINDS[13], "memory"); assert_eq!(NODE_KINDS[14], "dedup"); assert_eq!(NODE_KINDS[15], "loop"); + assert!(NODE_KINDS.contains(&"scatter")); + assert!(NODE_KINDS.contains(&"gather")); + assert_eq!(NODE_KINDS[16], "spawn"); + assert_eq!(NODE_KINDS[17], "gate"); + assert_eq!(NODE_KINDS[18], "scatter"); + assert_eq!(NODE_KINDS[19], "gather"); } #[test] diff --git a/src/engine.rs b/src/engine.rs index 88863745..bc5321f5 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -23,7 +23,8 @@ use std::sync::{Arc, Mutex}; use std::time::Instant; use crate::graph::{ - Command, CompiledGraph, END, GraphBuilder, GraphError, Interrupt, NodeResult, StateReducer, + Command, CompiledGraph, END, GraphBuilder, GraphError, Interrupt, NodeResult, RouteTarget, + StateReducer, }; use serde_json::{Map, Value, json}; @@ -135,6 +136,15 @@ pub struct RunInput { /// Caller-supplied values for the workflow's declared inputs, by name. /// Validated by [`crate::model::resolve_inputs`] before the run starts. pub inputs: Map, + /// Gate ids pre-approved for this run. + /// + /// An explicit channel, separate from the trigger payload. Approvals can + /// also be written as `trigger.approvals` when the trigger happens to be an + /// object, and that remains supported — but a run whose trigger is an array + /// or a scalar (a `sub_workflow` child is seeded with its input *items*, an + /// array) has nowhere to put them, so smuggling approvals through the + /// payload cannot work in general. + pub approvals: Vec, } impl RunInput { @@ -144,6 +154,7 @@ impl RunInput { Self { trigger, inputs: Map::new(), + approvals: Vec::new(), } } @@ -153,6 +164,13 @@ impl RunInput { self.inputs = inputs; self } + + /// Pre-approves the named gates for this run (see [`Self::approvals`]). + #[must_use] + pub fn with_approvals(mut self, approvals: Vec) -> Self { + self.approvals = approvals; + self + } } impl From for RunInput { @@ -218,9 +236,48 @@ impl StateReducer for MergeReducer { } } +/// The sentinel key that makes an update *assign* rather than merge. +/// +/// An update object shaped exactly `{"$replace": v}` sets its slot to `v` +/// wholesale. +pub(crate) const REPLACE: &str = "$replace"; + +/// Wraps `value` so [`merge`] assigns it instead of merging into what is there. +pub(crate) fn replace(value: Value) -> Value { + json!({ REPLACE: value }) +} + /// Recursively merges `update` into `base`: objects merge key-by-key; any other -/// value (array, scalar, null) overwrites. +/// value (array, scalar, null) overwrites; and an update of exactly +/// `{"$replace": v}` assigns `v` wholesale. +/// +/// # Why the sentinel exists +/// +/// Key-by-key merging means an object-valued slot can only ever *gain* keys. A +/// node that keeps state across its own activations — a `loop` node's +/// accumulator — could therefore never drop one: `{"attempts": [...], "err": +/// "x"}` has no way to become `{"attempts": [...]}`. Arrays and scalars already +/// overwrite, so this is specifically the object case, which is the interesting +/// one for an accumulator. +/// +/// # Why user data cannot be mistaken for it +/// +/// `merge` only ever recurses through the object-valued subtrees of an *update*, +/// and the only object subtrees an update contains are the root, `"nodes"`, each +/// node's slot, and the `meta` values a node records about itself. Item payloads +/// live inside `slot["items"]`, which is an **array** — it hits the overwrite arm +/// without being walked into. So a workflow whose data happens to contain a +/// `$replace` key is never examined by this function, and cannot trigger it. fn merge(base: &mut Value, update: Value) { + // Checked before the object/object arm: the sentinel *is* an object, and + // merging it key-by-key would write a literal `$replace` key into state. + if let Value::Object(map) = &update + && map.len() == 1 + && let Some(value) = map.get(REPLACE) + { + *base = value.clone(); + return; + } match (base, update) { (Value::Object(base), Value::Object(update)) => { for (key, value) in update { @@ -231,6 +288,98 @@ fn merge(base: &mut Value, update: Value) { } } +/// Builds a lane activation's state update. +/// +/// A lane writes under `nodes..lanes.` and **never** touches the +/// slot's top-level `items`/`port`. That is the whole reason N concurrent +/// activations of one node do not clobber each other: the reducer merges +/// objects key-by-key, so distinct lane keys are collision-free without the +/// reducer needing to know lanes exist. +/// +/// This is the single writer of lane slots, deliberately — the "lanes never +/// write the top level" rule is structural, enforced by there being one +/// constructor, rather than by anything the engine checks at run time. +fn lane_items_update( + node_id: &str, + lane: &crate::nodes::LaneContext, + items: &[Item], + port: Option<&str>, + status: &str, + meta: Option<&Value>, +) -> crate::graph::Result { + let mut slot = json!({ + "items": serde_json::to_value(items)?, + "port": port.map(Value::from).unwrap_or(Value::Null), + "status": status, + "index": lane.index, + }); + if let (Some(Value::Object(extra)), Some(map)) = (meta, slot.as_object_mut()) { + for (key, value) in extra { + map.insert(key.clone(), value.clone()); + } + } + Ok(json!({ "nodes": { node_id: { "lanes": { lane.id.clone(): slot } } } })) +} + +/// The lane envelope a fan-out schedules one activation with. +fn lane_envelope( + origin: &str, + index: usize, + count: usize, + items: &[Item], +) -> crate::graph::Result { + Ok(json!({ + LANE_KEY: { + "id": format!("{origin}#{index}"), + "origin": origin, + "index": index, + "count": count, + }, + "items": serde_json::to_value(items)?, + })) +} + +/// Reads a lane activation's items back out of its envelope. +/// +/// A lane takes its input from here rather than from [`collect_input`], and +/// must: every branch of a super-step reads the same committed snapshot, so +/// `collect_input` would hand all N lanes the identical items. +fn lane_input(send_arg: Option<&Value>) -> Vec { + send_arg + .and_then(|arg| arg.get("items")) + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| serde_json::from_value::(item.clone()).ok()) + .collect() + }) + .unwrap_or_default() +} + +/// The key a lane envelope records its lane identity under, inside the +/// `send_arg` a fan-out schedules each concurrent activation with. +/// +/// Underscore-prefixed because it shares the envelope with the lane's `items` +/// and must never be mistaken for user data. +pub(crate) const LANE_KEY: &str = "_lane"; + +/// Decodes the lane identity from an activation's `send_arg`. +/// +/// `None` for an ordinary activation — one scheduled by a plain route rather +/// than by a fan-out packet — which is every activation until a fan-out node +/// exists. A malformed envelope also yields `None` rather than failing the run: +/// the activation then behaves as an ordinary one, which is the safe reading. +fn lane_context(send_arg: Option<&Value>) -> Option { + let lane = send_arg?.get(LANE_KEY)?; + Some(crate::nodes::LaneContext { + id: lane.get("id")?.as_str()?.to_string(), + origin: lane.get("origin")?.as_str()?.to_string(), + index: usize::try_from(lane.get("index")?.as_u64()?).ok()?, + count: usize::try_from(lane.get("count")?.as_u64()?).ok()?, + }) +} + /// Collects a node's input items from the `items` its predecessors emitted into /// the run state, **honoring the port each edge is wired to**. /// @@ -801,6 +950,15 @@ pub async fn run_cancellable_with_observer( /// agrees on one bound. pub const MAX_SUB_WORKFLOW_DEPTH: u64 = 8; +/// The ceiling on `trigger.config.max_concurrency`: how many branches of one +/// super-step the engine will ever run at once. +/// +/// A cap rather than an error, mirroring how a node's own `concurrency` is +/// clamped: a graph asking for 100_000 concurrent branches has a mistake in it, +/// and refusing the run outright would be a worse answer than running it +/// sensibly and saying so in a warning. +pub const MAX_GRAPH_CONCURRENCY: usize = 256; + /// Runs a nested child workflow for a `sub_workflow` node, threading the current /// nesting `depth` into the child run's `run.sub_workflow_depth`. /// @@ -911,6 +1069,29 @@ fn build_graph( .and_then(Value::as_u64) .filter(|n| *n > 0) .map(std::time::Duration::from_secs); + // How many branches of one super-step may be in flight at once. + // + // This is *admission control*, not backpressure: a super-step engine cannot + // block a producer mid-step, so the only lever is how many activations are + // allowed to start. Unset means unbounded, which is the historical behavior. + let max_concurrency = trigger + .config + .get("max_concurrency") + .and_then(Value::as_u64) + .filter(|n| *n > 0) + .map(|requested| { + let requested = usize::try_from(requested).unwrap_or(MAX_GRAPH_CONCURRENCY); + if requested > MAX_GRAPH_CONCURRENCY { + tracing::warn!( + requested, + max = MAX_GRAPH_CONCURRENCY, + "max_concurrency above the engine ceiling; clamping" + ); + MAX_GRAPH_CONCURRENCY + } else { + requested + } + }); tracing::info!(node_count = graph.nodes.len(), trigger = %trigger_id, "workflow run starting"); @@ -944,6 +1125,9 @@ fn build_graph( let mut builder = GraphBuilder::::new() .with_parallel(true) .set_reducer(MergeReducer); + if let Some(limit) = max_concurrency { + builder = builder.with_max_concurrency(limit); + } if let Some(limit) = recursion_limit { builder = builder.with_recursion_limit(limit as usize); } else { @@ -1018,6 +1202,24 @@ fn build_graph( let is_trigger = node.kind == NodeKind::Trigger; // How this node drives its successors once it has an update. let routing = handler_routing(graph, &node.id); + // Successors on the emitted port, needed only inside a lane: `Plain` + // routing normally rides static edges, but a lane has to re-schedule + // every successor as a `Send`, so it needs the target list explicitly. + let plain_targets: Vec = graph + .edges + .iter() + .filter(|e| e.from_node == node.id) + .map(|e| e.to_node.clone()) + .collect(); + // Which successors end a lane. Routing to one of these is a plain + // activation, so the lanes converge on it instead of each running their + // own copy. + let gather_nodes: std::collections::HashSet = graph + .nodes + .iter() + .filter(|n| n.kind == NodeKind::Gather) + .map(|n| n.id.clone()) + .collect(); // Whether the node has an outgoing edge on the `error` port. A denied // approval gate (see the resume-deny path below) routes its error item // there when present, and fails the run when absent. @@ -1037,10 +1239,21 @@ fn build_graph( let terminal_error = terminal_error.clone(); let token = token.clone(); let routing = routing.clone(); + let plain_targets = plain_targets.clone(); + let gather_nodes = gather_nodes.clone(); // The resume value delivered to this node on a checkpointed resume, if // any. A bare `true` means "approve the interrupted gate"; a structured // `{ "rejected": [, …] }` denies the named gate(s). let resume_value = ctx.resume.clone(); + // The lane envelope this activation was scheduled with, if it is one + // of several concurrent activations of this same node. Decoded once + // here because `ctx` is not moved into the async body below. + let lane = lane_context(ctx.send_arg.as_ref()); + let lane_send_arg = lane + .is_some() + .then(|| ctx.send_arg.clone()) + .flatten(); + let activation_step = ctx.step; // A checkpointed resume (see `ResumableRun::resume`) delivers a resume // value to the interrupted node via `NodeContext::resume`. A resume // approves *this* gate only when it is a bare `true` (backward-compat, @@ -1068,8 +1281,57 @@ fn build_graph( // port-command node drives only the successors of the port it // emitted on (`port`, defaulting to `main`); everything else emits // a plain update and follows its static/conditional edge. - let emit = |mut update: Value, port: Option<&str>| { - stamp_activation_step(&mut update, &node.id, ctx.step); + let emit = |mut update: Value, port: Option<&str>, routed_items: &[Item]| { + // Only a non-lane activation stamps the node's slot: the + // stamp is how a loop head tells its own re-entry from a + // stale arm, and a lane slot is not that. + if lane.is_none() { + stamp_activation_step(&mut update, &node.id, ctx.step); + } + + // Inside a lane, routing carries the lane onward. Every + // successor is re-scheduled as a `Send` holding this + // activation's output and the same lane identity, so the + // whole downstream path runs once per lane rather than + // once in total. + // + // Except a gather: that is where lanes end. A gather is + // scheduled as a plain activation, and plain activations + // dedupe by node, so N lanes converge on one gather rather + // than activating it N times. + if let Some(lane) = lane.as_ref() { + let emitted = port.unwrap_or("main"); + let targets: Vec = match &routing { + HandlerRouting::Plain => plain_targets.clone(), + HandlerRouting::FanOut(targets) => targets.clone(), + HandlerRouting::PortCommand(groups) => groups + .iter() + .find(|(p, _)| p == emitted) + .map(|(_, targets)| targets.clone()) + .unwrap_or_default(), + }; + let routed: Vec = targets + .into_iter() + .map(|target| { + if gather_nodes.contains(&target) { + RouteTarget::Node(target.into()) + } else { + let envelope = lane_envelope( + &lane.origin, + lane.index, + lane.count, + routed_items, + ) + .unwrap_or(Value::Null); + RouteTarget::Send(crate::graph::Send::new(target, envelope)) + } + }) + .collect(); + return NodeResult::Command( + Command::route(routed).with_update(update), + ); + } + match &routing { HandlerRouting::Plain => NodeResult::Update(update), HandlerRouting::FanOut(targets) => { @@ -1105,7 +1367,7 @@ fn build_graph( if is_trigger { // The trigger payload is pre-seeded into the state; no-op update // (still fanning out if the trigger has parallel successors). - return Ok(emit(json!({}), None)); + return Ok(emit(json!({}), None, &[])); } // Human-in-the-loop approval gate. A node whose config sets @@ -1157,8 +1419,9 @@ fn build_graph( // edge falls back to a plain update the conditional-edge // router consumes. return Ok(emit( - items_update(&node.id, &[item], Some("error"))?, + items_update(&node.id, std::slice::from_ref(&item), Some("error"))?, Some("error"), + std::slice::from_ref(&item), )); } // No error branch to route to — fail the run so the denial @@ -1170,14 +1433,21 @@ fn build_graph( } let approved = state .get("run") - .and_then(|run| run.get("trigger")) - .and_then(|trigger| trigger.get("approvals")) - .and_then(Value::as_array) - .is_some_and(|approvals| { - approvals - .iter() - .filter_map(Value::as_str) - .any(|id| id == node.id) + .is_some_and(|run| { + // Two places, because approvals reach a run two + // ways: inside an object trigger payload (the + // original spelling, kept working) and through + // `RunInput::with_approvals`, which is the only one + // available when the trigger is not an object. + let listed = |approvals: Option<&Value>| { + approvals.and_then(Value::as_array).is_some_and(|ids| { + ids.iter().filter_map(Value::as_str).any(|id| id == node.id) + }) + }; + listed(run.get("approvals")) + || listed( + run.get("trigger").and_then(|trigger| trigger.get("approvals")), + ) }); // `approved_by_resume` is set when a checkpointed resume // delivered an approval (bare `true`, or this gate listed in @@ -1208,7 +1478,13 @@ fn build_graph( .get("nodes") .and_then(|nodes| nodes.get(&node.id)) .is_some_and(|slot| !slot.is_null()); - let input = if re_entry { + // A lane activation carries its own work. It must not read + // predecessor slots: every branch of a super-step sees the same + // committed snapshot, so `collect_input` would hand all N lanes + // the identical items. + let input = if let Some(lane_arg) = lane_send_arg.as_ref() { + lane_input(Some(lane_arg)) + } else if re_entry { let latest_step = back_incoming .iter() .filter_map(|(pred, _)| { @@ -1293,6 +1569,9 @@ fn build_graph( // `sub_workflow` node) can thread this run's cancellation // into its child; a plain executor never reads it. token: token.clone(), + lane: lane.clone(), + resume: resume_value.clone(), + step: activation_step, }; // BUG-8: bound THIS attempt (not the whole retry loop) to // `node_timeout`. Race the attempt future against a @@ -1386,15 +1665,127 @@ fn build_graph( steps.lock().expect("steps mutex poisoned").push(step.clone()); observer.on_step_finish(&step); let port = output.port.as_deref(); - Ok(emit( - items_update_with_meta( + + // A node that asked for something other than plain data + // flow. Handled before `emit` because both variants + // bypass ordinary routing. + match output.control { + // Pause the run. The update is deliberately dropped: + // the underlying executor discards an interrupting + // activation's state write, so returning one here + // would be a silent lie about what got committed. + // The node re-runs from the top on resume. + Some(crate::nodes::NodeControl::Interrupt { id, payload }) => { + tracing::info!(node = %node.id, gate = %id, "node paused the run"); + return Ok(NodeResult::Interrupt(Interrupt { + id, + node: node.id.clone().into(), + payload, + })); + } + // Ask to be run again. The update *is* committed, so + // the node can leave itself notes (a poll count, a + // ticket) and read them back on the next activation. + // + // `goto`-ing ourselves re-activates this node in the + // next super-step. Each poll costs one super-step and + // one node visit, so the caller's own bounded poll + // count is what stops this — not the run-level + // backstop, which cannot say which node span. + Some(crate::nodes::NodeControl::Reenter { after_ms }) => { + let update = items_update_with_meta( + &node.id, + &output.items, + port, + output.meta.as_ref(), + )?; + if after_ms > 0 { + // Chopped into slices so a cancel during a + // long poll interval is seen promptly, the + // same way retry backoff is drained above. + let mut remaining = after_ms; + while remaining > 0 { + if token.is_cancelled() { + tracing::info!(node = %node.id, "run cancelled while waiting to re-enter"); + return Ok(NodeResult::Update(items_update( + &node.id, + &[], + None, + )?)); + } + let slice = remaining.min(BACKOFF_POLL_MS); + futures_timer::Delay::new( + std::time::Duration::from_millis(slice), + ) + .await; + remaining -= slice; + } + } + return Ok(NodeResult::Command( + Command::goto(vec![node.id.clone()]).with_update(update), + )); + } + // Open a lane per entry: schedule every successor + // once for each, each carrying its own work. + // + // This is the one routing decision a node makes that + // the graph's edges cannot express. `Send` packets + // are the reason it works at all — plain + // activations dedupe by node id, so repeating a + // target would collapse back to one. + Some(crate::nodes::NodeControl::Scatter { lanes }) => { + let count = lanes.len(); + let mut routed: Vec = Vec::new(); + for (index, lane_items) in lanes.iter().enumerate() { + for target in &plain_targets { + let envelope = lane_envelope( + &node.id, index, count, lane_items, + ) + .unwrap_or(Value::Null); + routed.push(RouteTarget::Send( + crate::graph::Send::new(target.clone(), envelope), + )); + } + } + // The scatter's own slot records how many lanes + // it opened; a gather counts arrivals against it + // rather than guessing from what turned up. + let mut update = items_update_with_meta( + &node.id, + &output.items, + port, + output.meta.as_ref(), + )?; + stamp_activation_step(&mut update, &node.id, ctx.step); + tracing::debug!( + node = %node.id, lanes = count, "scatter: opened lanes" + ); + return Ok(NodeResult::Command( + Command::route(routed).with_update(update), + )); + } + None => {} + } + + // A lane activation writes its own lane slot; only a + // non-lane activation owns the node's top-level slot. + let update = match lane.as_ref() { + Some(lane) => lane_items_update( &node.id, + lane, &output.items, port, + "ok", output.meta.as_ref(), )?, - port, - )) + None => items_update_with_meta( + &node.id, + &output.items, + port, + output.meta.as_ref(), + )?, + }; + Ok(emit(update, port, &output.items)) } None => { tracing::warn!(node = %node.id, "node failed after retries"); @@ -1416,6 +1807,9 @@ fn build_graph( agents: &agents, observer: observer.as_ref(), token: token.clone(), + lane: lane.clone(), + resume: resume_value.clone(), + step: activation_step, }; let scope = crate::nodes::expr_scope(&ctx); crate::expr::resolve_traced(&node.config, &scope).1 @@ -1433,19 +1827,21 @@ fn build_graph( // ran (`max_attempts >= 1`); the `None` arm is unreachable // but handled defensively — emit an empty update, never panic. let Some(err) = last_err else { - return Ok(emit(items_update(&node.id, &[], None)?, None)); + return Ok(emit(items_update(&node.id, &[], None)?, None, &[])); }; match on_error { // Turn the failure into data on the default port. "continue" => Ok(emit( items_update(&node.id, &[error_item(&node.id, &err)], None)?, None, + &[error_item(&node.id, &err)], )), // Turn the failure into data on the `error` port so the // graph can route it to a recovery sub-graph. "route" => Ok(emit( items_update(&node.id, &[error_item(&node.id, &err)], Some("error"))?, Some("error"), + &[error_item(&node.id, &err)], )), // "stop" (default) and any unknown policy fail the run. // @@ -1505,10 +1901,19 @@ fn build_graph( match handler_routing(graph, &node.id) { HandlerRouting::FanOut(dests) => { // Parallel fan-out: the node's handler drives every successor with - // a `Command::goto`, so we only declare the destination hints here. + // a `Command::goto`, so we only declare the destinations here. // A command-routing node may not also carry static/conditional // edges, so nothing else is wired for it. - builder = builder.with_command_destinations(node.id.clone(), dests); + // + // Declared as *unconditional*, which is a promise the routing + // layer relies on rather than a hint: all of these successors run + // whenever this node runs (they share one port — that is what + // makes this a fan-out rather than a choice). Barrier relief + // walks through this node on the strength of it, and would + // otherwise treat the fan-out as an unresolvable decision, decide + // a branch behind it went untaken, and clear a downstream barrier + // early. + builder = builder.with_unconditional_fanout(node.id.clone(), dests); } HandlerRouting::PortCommand(groups) => { // Mixed-port node (e.g. `main->a, main->b, error->h`): the handler @@ -1750,7 +2155,11 @@ async fn build_and_run( // caller that gets an `Input` error can therefore be certain nothing ran and // nothing was observed, which is what lets a host reject a bad call without // recording a phantom run. - let RunInput { trigger, inputs } = input.into(); + let RunInput { + trigger, + inputs, + approvals, + } = input.into(); let resolved_inputs = crate::model::resolve_inputs(&workflow.graph.inputs, &inputs)?; // Process-local, monotonic run id — no time/random source. @@ -1786,7 +2195,8 @@ async fn build_and_run( // declaration, defaults already applied. `expr_scope_for` lifts it to the // top-level `inputs` scope key, so node config addresses it as // `=inputs.` (and jq programs walking `run` still see it too). - let mut initial = json!({ "run": { "trigger": trigger, "inputs": resolved_inputs } }); + let mut initial = + json!({ "run": { "trigger": trigger, "inputs": resolved_inputs, "approvals": approvals } }); merge(&mut initial, seed_items); // The nesting cap for `sub_workflow` chains, read off the trigger config // like every other run-level knob and seeded into the run state so the @@ -1846,12 +2256,18 @@ async fn build_and_run( } }; - // Nodes that paused the run awaiting approval, surfaced from the interrupts + // Gates that paused the run awaiting approval, surfaced from the interrupts // the runtime returned at the boundary. + // + // Keyed by the interrupt's `id`, not by the node that raised it. For an + // ordinary `requires_approval` gate the two are the same string. They differ + // where one node speaks for gates that are not its own — a `sub_workflow` + // node reports its child's gates as `::`, because parent + // and child are separate graphs whose ids would otherwise collide. let pending_approvals: Vec = execution .interrupts .iter() - .map(|interrupt| interrupt.node.as_str().to_string()) + .map(|interrupt| interrupt.id.clone()) .collect(); tracing::info!( @@ -1942,6 +2358,7 @@ fn merge_approvals(input: impl Into, newly_approved: Vec) -> R let RunInput { mut trigger, inputs, + approvals: prior, } = input.into(); let mut approvals: Vec = trigger @@ -1961,13 +2378,25 @@ fn merge_approvals(input: impl Into, newly_approved: Vec) -> R } } + // Carry forward approvals delivered through the explicit channel too, so a + // resume of a run started with `with_approvals` does not silently drop them. + for id in prior { + if !approvals.contains(&id) { + approvals.push(id); + } + } + if let Value::Object(map) = &mut trigger { - map.insert("approvals".to_string(), json!(approvals)); + map.insert("approvals".to_string(), json!(approvals.clone())); } else { - trigger = json!({ "approvals": approvals }); + trigger = json!({ "approvals": approvals.clone() }); } - RunInput { trigger, inputs } + RunInput { + trigger, + inputs, + approvals, + } } /// Like [`resume`], but observes `token`: cancelling it winds the resumed run @@ -2053,7 +2482,7 @@ impl ResumableRun { let pending_approvals: Vec = execution .interrupts .iter() - .map(|interrupt| interrupt.node.as_str().to_string()) + .map(|interrupt| interrupt.id.clone()) .collect(); Ok(RunOutcome { @@ -2430,7 +2859,7 @@ async fn resume_with_checkpointer_inner( let pending_approvals: Vec = execution .interrupts .iter() - .map(|interrupt| interrupt.node.as_str().to_string()) + .map(|interrupt| interrupt.id.clone()) .collect(); let graph_run_ids = GraphRunIds { @@ -2450,6 +2879,140 @@ async fn resume_with_checkpointer_inner( )) } +#[cfg(test)] +mod merge_tests { + use super::*; + + /// The plain behaviour the sentinel sits alongside: objects gain keys. + #[test] + fn objects_merge_key_by_key_and_scalars_overwrite() { + let mut base = json!({ "a": 1, "nested": { "x": 1 } }); + merge(&mut base, json!({ "b": 2, "nested": { "y": 2 } })); + assert_eq!( + base, + json!({ "a": 1, "b": 2, "nested": { "x": 1, "y": 2 } }) + ); + } + + /// The problem the sentinel solves: without it, a key can never be dropped. + #[test] + fn a_plain_merge_cannot_remove_a_key_but_replace_can() { + let mut base = json!({ "attempts": [1], "err": "boom" }); + merge(&mut base, json!({ "attempts": [1, 2] })); + assert_eq!( + base["err"], "boom", + "a plain merge leaves the key it did not mention" + ); + + let mut base = json!({ "attempts": [1], "err": "boom" }); + merge(&mut base, replace(json!({ "attempts": [1, 2] }))); + assert_eq!( + base, + json!({ "attempts": [1, 2] }), + "replace assigns wholesale, so the dropped key is gone" + ); + } + + #[test] + fn replace_works_at_every_nesting_depth() { + let mut base = json!({ "nodes": { "l": { "state": { "a": 1, "b": 2 } } } }); + merge( + &mut base, + json!({ "nodes": { "l": { "state": replace(json!({ "a": 9 })) } } }), + ); + assert_eq!(base["nodes"]["l"]["state"], json!({ "a": 9 })); + + let mut base = json!({ "x": 1 }); + merge(&mut base, replace(json!("scalar"))); + assert_eq!(base, json!("scalar"), "replace works at the root too"); + } + + /// The soundness argument, as a test: `merge` never walks into an items + /// array, so a workflow whose *data* contains a `$replace` key is never + /// examined by the sentinel check and cannot trigger it. + #[test] + fn a_replace_key_inside_item_data_is_left_alone() { + let payload = json!({ REPLACE: "user data, not a sentinel" }); + let mut base = json!({ "nodes": { "n": { "items": [] } } }); + merge( + &mut base, + json!({ "nodes": { "n": { "items": [ { "json": payload.clone() } ] } } }), + ); + assert_eq!( + base["nodes"]["n"]["items"][0]["json"], payload, + "item payloads ride inside an array and are copied verbatim" + ); + } + + /// Only an update that is *exactly* the sentinel assigns. An object that + /// merely contains the key alongside others is ordinary data. + #[test] + fn only_a_lone_replace_key_is_treated_as_the_sentinel() { + let mut base = json!({ "keep": 1 }); + merge(&mut base, json!({ REPLACE: "x", "other": 2 })); + assert_eq!( + base, + json!({ "keep": 1, REPLACE: "x", "other": 2 }), + "a two-key object is data, and merges normally" + ); + } +} + +#[cfg(test)] +mod lane_context_tests { + use super::*; + + fn envelope(lane: Value) -> Value { + json!({ LANE_KEY: lane, "items": [] }) + } + + #[test] + fn a_well_formed_envelope_decodes() { + let lane = lane_context(Some(&envelope(json!({ + "id": "fan#2", "origin": "fan", "index": 2, "count": 5 + })))) + .expect("a complete envelope should decode"); + assert_eq!(lane.id, "fan#2"); + assert_eq!(lane.origin, "fan"); + assert_eq!(lane.index, 2); + assert_eq!(lane.count, 5); + } + + /// The ordinary case, and the only one that occurs until a fan-out node + /// exists: an activation scheduled by a plain route carries no arg at all. + #[test] + fn an_activation_without_a_send_arg_has_no_lane() { + assert!(lane_context(None).is_none()); + } + + /// A `send_arg` that is not a lane envelope belongs to something else and + /// must not be read as a lane. + #[test] + fn a_send_arg_without_the_lane_key_has_no_lane() { + assert!(lane_context(Some(&json!({ "items": [] }))).is_none()); + } + + /// Decoding is total: a malformed envelope degrades to "no lane" rather + /// than panicking or failing the run, so a bad packet cannot take the run + /// down. Each case drops or corrupts exactly one required field. + #[test] + fn a_malformed_envelope_degrades_to_no_lane() { + for broken in [ + json!({ "origin": "fan", "index": 0, "count": 1 }), // no id + json!({ "id": "fan#0", "index": 0, "count": 1 }), // no origin + json!({ "id": "fan#0", "origin": "fan", "count": 1 }), // no index + json!({ "id": "fan#0", "origin": "fan", "index": 0 }), // no count + json!({ "id": 7, "origin": "fan", "index": 0, "count": 1 }), // id not a string + json!({ "id": "fan#0", "origin": "fan", "index": -1, "count": 1 }), // negative index + ] { + assert!( + lane_context(Some(&envelope(broken.clone()))).is_none(), + "malformed envelope should not decode: {broken}" + ); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -4434,17 +4997,23 @@ mod tests { let compiled = compile(&graph).expect("compile"); let caps = mock_capabilities(); - // The engine serializes interrupts across the fan-out: g1 is the first - // gate to pend; g2 pends only once g1 is resolved. The invariant this test - // guards is that approving g1 must NOT also approve g2 — a bare `true` - // resume value would blanket-approve every interrupted gate. + // Both gates pend at once. They run concurrently in the same superstep, + // and the whole active set is folded before the run pauses, so there is + // no reason to surface one and hide the other behind a resume round-trip + // — a host can present both for approval immediately. + // + // The invariant this test guards is unchanged by that: approving g1 must + // NOT also approve g2. A bare `true` resume value would blanket-approve + // every interrupted gate, which is precisely what naming them prevents. let rr = run_resumable(&compiled, json!({}), &caps) .await .expect("run_resumable"); + let mut pending = rr.outcome().pending_approvals.clone(); + pending.sort(); assert_eq!( - rr.outcome().pending_approvals, - vec!["g1".to_string()], - "g1 is the first parallel gate to pend" + pending, + vec!["g1".to_string(), "g2".to_string()], + "both parallel gates pend together" ); let after_g1 = rr.resume(vec!["g1".to_string()]).await.expect("resume g1"); diff --git a/src/graph/builder/mod.rs b/src/graph/builder/mod.rs index ce948685..962ac13a 100644 --- a/src/graph/builder/mod.rs +++ b/src/graph/builder/mod.rs @@ -86,6 +86,7 @@ where recursion_limit: 50, parallel: false, max_concurrency: None, + node_concurrency: HashMap::new(), node_timeout: None, node_meta: HashMap::new(), } @@ -119,6 +120,27 @@ where self } + /// Bounds how many activations of **one node** may run concurrently within a + /// step, independently of the graph-wide [`Self::with_max_concurrency`]. + /// + /// Only meaningful for a node that can be activated more than once in the + /// same superstep — i.e. one scheduled through `Send` packets, where a fan-out + /// of N items becomes N activations of the same node. The graph-wide bound + /// cannot express "let this one node run 4 at a time" without also throttling + /// every unrelated branch in the step. + /// + /// Both bounds apply: a branch starts only when the step is below the global + /// limit *and* its node is below this one. `0` removes the per-node bound. + pub fn with_node_concurrency(mut self, node: impl Into, n: usize) -> Self { + let node = node.into(); + if n == 0 { + self.node_concurrency.remove(&node); + } else { + self.node_concurrency.insert(node, n); + } + self + } + /// Sets a default wall-clock timeout applied to every node handler. A node /// whose future does not resolve within `timeout` fails the run with /// [`crate::GraphError::Timeout`]. @@ -332,6 +354,33 @@ where self } + /// Records destinations for a command node that fans out to **all** of + /// them, unconditionally, every time it runs. + /// + /// Like [`Self::with_command_destinations`], but additionally promises + /// there is no runtime choice among the destinations. Barrier relief relies + /// on that promise to walk forward through this node when deciding whether + /// a conditional branch was taken; without it, relief treats the node as an + /// unresolvable decision point, concludes the branch was untaken, and + /// clears a downstream barrier before its real predecessors have run. + /// + /// Only use this where every destination genuinely always runs. A node that + /// picks between ports must use [`Self::with_command_destinations`]. + pub fn with_unconditional_fanout( + mut self, + node: impl Into, + destinations: I, + ) -> Self + where + I: IntoIterator, + N: Into, + { + let node = node.into(); + self = self.with_command_destinations(node.clone(), destinations); + self.node_meta.entry(node).or_default().command_fanout = true; + self + } + /// Sets a human-readable kind for `node` (e.g. `model`, `tool`, `subgraph`) /// surfaced as [`crate::graph::NodeInfo::kind`] in the export. pub fn with_node_kind(mut self, node: impl Into, kind: impl Into) -> Self { @@ -460,6 +509,7 @@ where recursion_limit, parallel, max_concurrency, + node_concurrency, node_timeout, node_meta, barrier_reliefs, @@ -478,6 +528,7 @@ where recursion_limit, parallel, max_concurrency, + node_concurrency, node_timeout, node_meta, barrier_reliefs, diff --git a/src/graph/builder/types.rs b/src/graph/builder/types.rs index 9957d1c2..1fae1ccb 100644 --- a/src/graph/builder/types.rs +++ b/src/graph/builder/types.rs @@ -116,9 +116,22 @@ pub(crate) struct NodeMeta { /// The node embeds and runs a child graph (a subgraph node). pub(crate) subgraph: bool, /// Declared `goto` destination hints for a command-routing node, in the - /// order they were registered. Purely advisory: the runtime resolves the - /// real target from the emitted [`crate::graph::Command`] at runtime. + /// order they were registered. Advisory for routing — the runtime resolves + /// the real target from the emitted [`crate::graph::Command`] — but load + /// bearing for barrier relief when [`Self::command_fanout`] is set. pub(crate) command_destinations: Vec, + /// Every one of [`Self::command_destinations`] runs whenever this node + /// runs: the node is an unconditional fan-out, not a choice between ports. + /// + /// This is the one command-routing fact that *is* statically knowable, and + /// barrier relief needs it. Relief asks "did the taken branch lead to this + /// predecessor?" by walking forward through deterministic routing; a + /// command node normally stops that walk, because which target it picks is + /// a runtime decision. For a fan-out there is no decision — all of them run + /// — so the walk can and must continue through it. Stopping instead makes + /// relief conclude the branch was untaken and fire a phantom arrival, + /// clearing a barrier before its real predecessors have run. + pub(crate) command_fanout: bool, /// Arbitrary, sorted key/value annotations carried into the export. pub(crate) metadata: BTreeMap, } @@ -231,6 +244,9 @@ pub struct GraphBuilder { pub(crate) parallel: bool, /// Upper bound on concurrently-running branches per step (`None` = unbounded). pub(crate) max_concurrency: Option, + /// Per-node ceilings on concurrent activations of that *same* node within a + /// step (`None` for a node = bounded only by `max_concurrency`). + pub(crate) node_concurrency: HashMap, /// Default per-node handler timeout (`None` = no timeout). pub(crate) node_timeout: Option, /// Behavior-free per-node markers/metadata surfaced by the topology export. diff --git a/src/graph/command/mod.rs b/src/graph/command/mod.rs index a837be6e..d7b3490f 100644 --- a/src/graph/command/mod.rs +++ b/src/graph/command/mod.rs @@ -53,6 +53,21 @@ impl Command { } } + /// Creates a command routing to a **mixed** set of targets: plain + /// activations and [`Send`] packets together. + /// + /// [`Self::goto`] and [`Self::send`] each build one kind. A fan-out whose + /// successors are not all the same kind — lanes carrying their own work to + /// most successors, but converging on one — needs both in a single command, + /// since a node emits one. + pub fn route(targets: impl IntoIterator) -> Self { + Self { + update: None, + goto: targets.into_iter().collect(), + resume: None, + } + } + /// Creates a command carrying a partial state update. pub fn update(update: Update) -> Self { Self { diff --git a/src/graph/compiled/executor.rs b/src/graph/compiled/executor.rs index 19037702..90f9eee9 100644 --- a/src/graph/compiled/executor.rs +++ b/src/graph/compiled/executor.rs @@ -582,7 +582,8 @@ where let StepRun { updates, goto_map, - interrupt, + completed: completed_indices, + interrupts, failure, } = match run_result { Ok(step_run) => step_run, @@ -712,7 +713,7 @@ where // members of this step (interrupted node first). Each pending branch // keeps its `Send` arg; accumulated barrier arrivals are persisted // too. Then return control to the caller. - if let Some((index, emitted)) = interrupt { + if !interrupts.is_empty() { if let Err(err) = self.require_interrupt_durability(&thread_id) { return self .fail_and_return( @@ -725,9 +726,26 @@ where ) .await; } + // Split the step three ways: branches that ran to a result, + // branches that interrupted, and — under sequential execution — + // branches that were never started at all. + // + // A branch that ran keeps its result and has its successors + // routed now, so a resume never runs it again. Only the + // interrupted and never-started ones are rescheduled. + // + // Position is not the discriminator. Under parallel execution the + // whole active set runs before anything is folded, so "after the + // interrupt" and "did not run" are different sets; rescheduling + // by position would re-run completed work and fire its side + // effects a second time. Under sequential execution they happen + // to coincide, which is exactly why `completed_indices` is + // reported by the runner rather than inferred here. + let (completed, completed_goto) = + Self::partition_completed(&active, &goto_map, &completed_indices); let successors = match self.route_completed( - &active[..index], - &goto_map, + &completed, + &completed_goto, &state, &mut barrier_arrivals, ) { @@ -745,10 +763,41 @@ where .await; } }; + // Rescheduled: the interrupted branches, plus any branch that was + // never started (sequential execution stops at the interrupt). + // The branches that ran are represented by their successors + // instead, so they are not run twice. + let interrupted_nodes: Vec = interrupts + .iter() + .map(|(index, _)| active[*index].node.clone()) + .collect(); + let ran: HashSet = completed_indices + .iter() + .copied() + .chain(interrupts.iter().map(|(index, _)| *index)) + .collect(); let mut pending = successors; - pending.extend(active[index..].iter().cloned()); + pending.extend( + interrupts + .iter() + .map(|(index, _)| active[*index].clone()) + .chain( + active + .iter() + .enumerate() + .filter(|(index, _)| !ran.contains(index)) + .map(|(_, activation)| activation.clone()), + ), + ); let pending_nodes = activation_nodes(&pending); - let interrupt_id = InterruptId::new(emitted.id.clone()); + let interrupt_ids: Vec = interrupts + .iter() + .map(|(_, emitted)| InterruptId::new(emitted.id.clone())) + .collect(); + let emitted_interrupts: Vec = interrupts + .iter() + .map(|(_, emitted)| emitted.clone()) + .collect(); // An interrupt hands control back to the caller expecting a // fully durable pause point: settle any in-flight Async // background writes first, failing the run if one was lost @@ -771,9 +820,9 @@ where &run_id, &state, &pending, - &active[..index], - vec![emitted.clone()], - std::slice::from_ref(&active[index].node), + &completed, + emitted_interrupts.clone(), + &interrupted_nodes, &barrier_arrivals, parent_checkpoint.clone(), steps, @@ -802,7 +851,7 @@ where status.status = ExecutionStatus::Interrupted; status.current_step = steps; status.active_nodes = pending_nodes; - status.pending_interrupts = vec![interrupt_id]; + status.pending_interrupts = interrupt_ids; status.checkpoint_id = checkpoint_id.clone(); self.save_status(status.clone()).await; @@ -815,7 +864,7 @@ where child_runs: all_child_runs, visited, steps, - interrupts: vec![emitted], + interrupts: emitted_interrupts, status, checkpoint_id, }); @@ -1240,7 +1289,8 @@ where ) -> Result> { let mut updates: Vec = Vec::new(); let mut goto_map: HashMap> = HashMap::new(); - let mut interrupt: Option<(usize, Interrupt)> = None; + let mut interrupts: Vec<(usize, Interrupt)> = Vec::new(); + let mut completed: Vec = Vec::new(); let mut failure: Option = None; for (index, activation) in active.iter().enumerate() { @@ -1303,15 +1353,19 @@ where &mut goto_map, visited, ) { - interrupt = Some(found); + interrupts.push(found); + // Sequential execution stops here: the rest of the step is never + // started, so those branches stay absent from `completed`. break; } + completed.push(index); } Ok(StepRun { updates, goto_map, - interrupt, + completed, + interrupts, failure, }) } @@ -1407,27 +1461,78 @@ where // which pending future completed; a parallel index Vec maps it back to // the branch's active-set position, so results are re-ordered into // deterministic order for the fold below. - let results = match self.max_concurrency { - Some(limit) if limit < futures.len() => { + // A per-node cap binds only when this step activates one node more times + // than its cap allows — which needs `Send` fanout, since plain activations + // are deduplicated by node. + let node_caps_bind = !self.node_concurrency.is_empty() && { + let mut counts: HashMap<&NodeId, usize> = HashMap::new(); + active.iter().any(|activation| { + let seen = counts.entry(&activation.node).or_default(); + *seen += 1; + self.node_concurrency + .get(&activation.node) + .is_some_and(|cap| *seen > *cap) + }) + }; + let global_binds = self + .max_concurrency + .is_some_and(|limit| limit < futures.len()); + + let results = match (global_binds || node_caps_bind).then_some(()) { + Some(()) => { + // Admission is governed by two independent ceilings: the + // graph-wide in-flight count, and how many activations of one + // *node* may be in flight. A branch starts only when both allow + // it, so throttling a wide fanout of one node does not also + // throttle the unrelated branches sharing its step. + let limit = self.max_concurrency.unwrap_or(futures.len()).max(1); let total = futures.len(); let mut slots: Vec>>> = (0..total).map(|_| None).collect(); - let mut source = futures.into_iter().enumerate(); + // Queued branches, in active-set order, each tagged with its node + // so admission can consult that node's cap. + let mut queue: std::collections::VecDeque<(usize, _)> = + futures.into_iter().enumerate().collect(); + let mut in_flight_per_node: HashMap = HashMap::new(); let mut running = Vec::with_capacity(limit); let mut running_index = Vec::with_capacity(limit); - for (index, fut) in source.by_ref().take(limit) { - running.push(fut); - running_index.push(index); + + // Admits as many queued branches as both ceilings currently + // allow, preserving active-set order among those admitted. + macro_rules! admit { + () => { + while running.len() < limit { + let Some(position) = queue.iter().position(|(index, _)| { + let node = &active[*index].node; + self.node_concurrency.get(node).is_none_or(|cap| { + in_flight_per_node.get(node).copied().unwrap_or(0) < *cap + }) + }) else { + // Every queued branch is blocked by its node's + // cap; the next completion frees one. + break; + }; + let (index, fut) = + queue.remove(position).expect("position is in range"); + *in_flight_per_node + .entry(active[index].node.clone()) + .or_default() += 1; + running.push(fut); + running_index.push(index); + } + }; } + + admit!(); while !running.is_empty() { let (result, completed, rest) = futures_util::future::select_all(running).await; let index = running_index.remove(completed); + if let Some(count) = in_flight_per_node.get_mut(&active[index].node) { + *count = count.saturating_sub(1); + } slots[index] = Some(result); running = rest; - if let Some((index, fut)) = source.next() { - running.push(fut); - running_index.push(index); - } + admit!(); } slots .into_iter() @@ -1440,7 +1545,8 @@ where // Fold in deterministic active-set index order. let mut updates: Vec = Vec::new(); let mut goto_map: HashMap> = HashMap::new(); - let mut interrupt: Option<(usize, Interrupt)> = None; + let mut interrupts: Vec<(usize, Interrupt)> = Vec::new(); + let mut completed: Vec = Vec::new(); let mut failure: Option = None; for (index, (activation, result)) in active.iter().zip(results).enumerate() { @@ -1465,6 +1571,15 @@ where } }; + // Deliberately no `break` here, unlike the sequential path. + // + // Every branch in this step has already *run* — they were driven + // concurrently above — so stopping the fold at the first interrupt + // would discard work that genuinely completed and re-schedule it, and + // resuming would run those branches a second time. For a node with + // side effects that means firing them twice. Fold every non- + // interrupting branch and let the caller schedule only the + // interrupted ones for resume. if let Some(found) = self.fold_result( index, node_id, @@ -1474,19 +1589,48 @@ where &mut goto_map, visited, ) { - interrupt = Some(found); - break; + interrupts.push(found); + } else { + completed.push(index); } } Ok(StepRun { updates, goto_map, - interrupt, + completed, + interrupts, failure, }) } + /// Splits a step's active set into the branches that completed, paired with + /// their routing re-keyed to the compacted set. + /// + /// [`Self::route_completed`] keys `goto_map` by position within the slice it + /// is handed, so dropping the interrupted branches from the middle would + /// silently misattribute every later branch's routing to the wrong node — + /// a `Send` packet would end up delivered on someone else's behalf. The + /// re-keying is the whole reason this is a function rather than a `filter`. + fn partition_completed( + active: &[Activation], + goto_map: &HashMap>, + completed_indices: &[usize], + ) -> (Vec, HashMap>) { + let mut completed = Vec::with_capacity(completed_indices.len()); + let mut routing = HashMap::new(); + for index in completed_indices { + let Some(activation) = active.get(*index) else { + continue; + }; + if let Some(targets) = goto_map.get(index) { + routing.insert(completed.len(), targets.clone()); + } + completed.push(activation.clone()); + } + (completed, routing) + } + /// Routes a set of completed activations into their successor activations. /// /// Honors per-activation command `goto` (keyed by active-set index), static diff --git a/src/graph/compiled/mod.rs b/src/graph/compiled/mod.rs index e14c35eb..ca94b052 100644 --- a/src/graph/compiled/mod.rs +++ b/src/graph/compiled/mod.rs @@ -146,8 +146,25 @@ struct StepRun { /// [`Command::goto`] — a node-keyed map would let a later activation's /// command clobber an earlier one's routing. goto_map: HashMap>, - /// The lowest-index branch interrupt, if any (its active-set index + value). - interrupt: Option<(usize, Interrupt)>, + /// Active-set indices of the branches that actually ran to a result and were + /// folded, in ascending order. Excludes interrupted branches. + /// + /// Not the same as "everything that did not interrupt". Sequential execution + /// stops at the first interrupt and never starts the rest of the step, so + /// those branches have no result — treating them as completed would route + /// successors for work that never happened. Parallel execution runs the whole + /// set before folding, so there every non-interrupting branch is here. + completed: Vec, + /// Every branch that interrupted this step, as `(active-set index, value)`, + /// in ascending index order. + /// + /// More than one is possible only under parallel execution, where the whole + /// active set runs before any result is folded — so two concurrent gates + /// can both pause in the same step, and reporting only the first would + /// leave the second invisible until an extra resume round-trip. Sequential + /// execution short-circuits at the first interrupt and never starts the + /// rest, so it contributes at most one. + interrupts: Vec<(usize, Interrupt)>, /// A node-handler failure that survived the node-retry policy, if any. When /// set, `updates` still carries the updates of the branches that completed /// *before* the failing branch, so the executor can fold that partial @@ -291,6 +308,7 @@ impl CompiledGraph { recursion_limit: usize, parallel: bool, max_concurrency: Option, + node_concurrency: HashMap, node_timeout: Option, node_meta: HashMap, barrier_reliefs: Vec, @@ -318,6 +336,7 @@ impl CompiledGraph { namespace: Vec::new(), parallel, max_concurrency, + node_concurrency: Arc::new(node_concurrency), node_timeout, run_deadline: None, durability: crate::graph::checkpoint::DurabilityMode::default(), diff --git a/src/graph/compiled/routing.rs b/src/graph/compiled/routing.rs index 5816eea2..9b39a15e 100644 --- a/src/graph/compiled/routing.rs +++ b/src/graph/compiled/routing.rs @@ -43,7 +43,26 @@ where }); // Barrier gating: hold a waiting node until every required // predecessor has arrived (possibly across supersteps). - if let Some(required) = self.waiting.get(&tnode) { + // + // Only arrivals from a **required** predecessor are gated. An + // arrival from anywhere else was never part of this barrier's + // contract, so holding it would be waiting for a rendezvous it + // is not attending. + // + // That distinction is what lets a barrier node also be the head + // of a cycle. A back-edge is registered as a plain edge, not a + // waiting one, so its source is absent from `required` — but the + // gate is keyed on the *target*, so without this check the + // re-entry would be swallowed on every pass: `arrived` would + // gain the body's id, still fail the `is_subset` test against + // the forward predecessors, and `continue`. The loop would run + // its first pass and then silently stop. Barrier relief cannot + // rescue that either, since it only fires for a predecessor + // whose branch was *not* taken, and a fan-in head's forward + // predecessors did run. + if let Some(required) = self.waiting.get(&tnode) + && required.contains(node_id) + { let arrived = barrier_arrivals.entry(tnode.clone()).or_default(); arrived.insert(node_id.clone()); if !required.is_subset(arrived) { @@ -123,6 +142,47 @@ where let Some(required) = self.waiting.get(&relief.barrier_node) else { continue; }; + // Is this barrier participating in the current pass at all? + // + // Relief exists to unblock a barrier that is holding real data while + // one of its predecessors can no longer arrive. It must never be the + // reason a barrier fires with *nothing* behind it. So before + // phantoming anything, check that the route actually taken still + // leads to at least one of the barrier's predecessors — or that one + // has already arrived. + // + // The two cases this separates look identical from a single relief + // registration, which is why the check is per barrier rather than + // per predecessor: + // + // - A conditional join where one arm was chosen: the taken route + // reaches that arm, so the barrier is engaged and the *other* arm + // is correctly phantomed. The phantom is needed here before any + // real arrival, since the chosen arm has not run yet. + // - A loop body's join on the pass where the head leaves through + // `done`: the taken route reaches neither arm. Nothing will ever + // arrive, so the barrier is simply not part of this pass. Firing it + // anyway would activate it on empty input and — because its + // back-edge re-enters the head, which exits and relieves again — + // ping-pong the run forever instead of letting it finish. + let already_arrived = barrier_arrivals + .get(&relief.barrier_node) + .is_some_and(|arrived| !arrived.is_empty()); + let barrier_engaged = already_arrived + || required.iter().any(|predecessor| { + source_indices.iter().any(|index| { + resolved[*index].iter().any(|target| { + self.reaches_deterministically( + target.node(), + predecessor, + &relief.barrier_node, + ) + }) + }) + }); + if !barrier_engaged { + continue; + } let arrived = barrier_arrivals .entry(relief.barrier_node.clone()) .or_default(); @@ -154,24 +214,57 @@ where /// eventually leads to a barrier's conditional predecessor is a static /// property of the compiled topology for any chain of plain pass-through /// nodes — it does not depend on when each hop happens to run. A further - /// conditional/command node along the way (no `self.edges` entry) is a - /// second runtime decision this walk cannot resolve ahead of time, so it - /// conservatively reports unreachable there (falling back to the + /// conditional node along the way is a second runtime decision this walk + /// cannot resolve ahead of time, so it stops there (falling back to the /// same-superstep check). + /// + /// An **unconditional fan-out** is the one command node the walk does cross. + /// It has no `self.edges` entry, because its successors come from the + /// `Command` it emits rather than from a static edge — but every one of its + /// declared destinations runs whenever it runs, so "does this lead to `to`" + /// is still a static question. Stopping there instead is not the safe + /// default it looks like: reporting unreachable is what *fires* relief, so a + /// fan-out on the path would clear a barrier before its real predecessors + /// had run and the join would read the previous pass's data. Erring toward + /// "reachable" costs at worst a barrier that waits, which is loud; erring + /// the other way is silently wrong output. + /// + /// Because a fan-out has several successors this is a search over a DAG + /// rather than a walk down a chain. fn reaches_deterministically(&self, from: &NodeId, to: &NodeId, stop: &NodeId) -> bool { if from == to { return true; } - let mut current = from; let mut seen: HashSet<&NodeId> = HashSet::new(); - while let Some(next) = self.edges.get(current) { - if next == to { - return true; + let mut frontier: Vec<&NodeId> = vec![from]; + while let Some(current) = frontier.pop() { + if !seen.insert(current) { + continue; } - if next == stop || !seen.insert(next) { - return false; + // A plain/waiting edge: exactly one successor, no decision. + if let Some(next) = self.edges.get(current) { + if next == to { + return true; + } + if next != stop { + frontier.push(next); + } + } + // An unconditional fan-out: every declared destination runs. + if self + .node_meta + .get(current) + .is_some_and(|meta| meta.command_fanout) + { + for next in &self.node_meta[current].command_destinations { + if next == to { + return true; + } + if next != stop { + frontier.push(next); + } + } } - current = next; } false } diff --git a/src/graph/compiled/test.rs b/src/graph/compiled/test.rs index db38d5c3..902133d0 100644 --- a/src/graph/compiled/test.rs +++ b/src/graph/compiled/test.rs @@ -2702,10 +2702,16 @@ async fn attributed_update_to_sink_node_keeps_other_pending_branches() { #[tokio::test] async fn attributed_update_preserves_pending_send_args_of_other_branches() { - // Three `Send` activations of `worker` are pending behind an interrupt. A - // write attributed to an unrelated node must carry them over *with* their - // args — dropping them loses the fanout, and re-scheduling them by node id - // alone loses each packet's payload. + // Three `Send` activations of `worker` run concurrently; the one carrying + // arg 1 interrupts, the other two finish. A write attributed to an unrelated + // node must carry the still-pending packet over *with* its arg — dropping it + // loses the fanout, and re-scheduling by node id alone loses the payload. + // + // Only the interrupted packet is pending. Workers 2 and 3 ran to completion + // in this same superstep (parallel execution folds the whole active set), so + // their updates are already committed and rescheduling them would run them a + // second time. Their work is asserted against the state below rather than + // against the pending set. let cp = Arc::new(InMemoryCheckpointer::::new()); let graph = GraphBuilder::::new() .with_parallel(true) @@ -2781,7 +2787,16 @@ async fn attributed_update_preserves_pending_send_args_of_other_branches() { }) .collect(); args.sort_unstable(); - assert_eq!(args, vec![1, 2, 3], "every pending Send packet survives"); + assert_eq!( + args, + vec![1], + "only the interrupted packet is pending, and it keeps its arg" + ); + assert_eq!( + paused.state.value, 5, + "workers 2 and 3 completed in the interrupted step, so their updates are \ + already folded (2 + 3) — a lower value means completed work was thrown away" + ); assert!( pending.iter().any(|a| a.node.as_str() == "tail"), "the attributed node's successor is scheduled alongside them" @@ -3097,3 +3112,91 @@ async fn async_durability_skips_a_write_whose_predecessor_failed() { "no orphaned checkpoint may be appended after a broken lineage" ); } + +/// A per-node concurrency cap bounds how many activations of that node run at +/// once, without throttling the rest of the step. +/// +/// Six `Send` packets fan `worker` out six ways alongside an unrelated `other` +/// branch. With `worker` capped at 2, at most two workers may ever be in flight +/// — but `other` must not be made to wait behind them, which is the whole reason +/// this is a per-node bound rather than the graph-wide one. +/// +/// The assertion is on observed *overlap*, tracked by incrementing a counter on +/// entry and decrementing on exit, because a cap that silently failed to bind +/// would still produce the same final state. +#[tokio::test] +async fn a_per_node_cap_bounds_one_node_without_throttling_the_step() { + let in_flight = Arc::new(AtomicUsize::new(0)); + let peak = Arc::new(AtomicUsize::new(0)); + let other_ran_early = Arc::new(AtomicBool::new(false)); + + let worker_in_flight = in_flight.clone(); + let worker_peak = peak.clone(); + let other_flag = other_ran_early.clone(); + + let graph = GraphBuilder::::new() + .with_parallel(true) + .with_node_concurrency("worker", 2) + .set_reducer(ClosureStateReducer::new(|s: i32, u: i32| Ok(s + u))) + .add_node("dispatch", |_s: i32, _c: NodeContext| async move { + Ok(NodeResult::Command(Command::send( + (1..=6).map(|n| Send::new("worker", json!(n))), + ))) + }) + .add_node("worker", move |_s: i32, _c: NodeContext| { + let in_flight = worker_in_flight.clone(); + let peak = worker_peak.clone(); + async move { + let now = in_flight.fetch_add(1, AtomicOrdering::SeqCst) + 1; + peak.fetch_max(now, AtomicOrdering::SeqCst); + // Yield enough times that any un-capped sibling would overlap. + for _ in 0..8 { + tokio::task::yield_now().await; + } + in_flight.fetch_sub(1, AtomicOrdering::SeqCst); + Ok(NodeResult::Update(1)) + } + }) + .add_node("other", move |_s: i32, _c: NodeContext| { + let flag = other_flag.clone(); + async move { + flag.store(true, AtomicOrdering::SeqCst); + Ok(NodeResult::Update(100)) + } + }) + .set_entry("dispatch") + .mark_command_routing("dispatch") + .set_finish("worker") + .set_finish("other") + .compile() + .unwrap(); + + // `dispatch` sends six workers; `other` is seeded alongside them so the step + // contains both. + let done = graph + .run_with_inputs( + 0, + [ + crate::graph::GraphInput::start(json!(null)), + crate::graph::GraphInput::node("other"), + ], + ) + .await + .unwrap(); + + assert!( + peak.load(AtomicOrdering::SeqCst) <= 2, + "worker is capped at 2 concurrent activations, observed peak {}", + peak.load(AtomicOrdering::SeqCst) + ); + assert!( + other_ran_early.load(AtomicOrdering::SeqCst), + "the unrelated branch must still run — a per-node cap must not throttle \ + the whole step" + ); + assert_eq!( + done.state, 106, + "all six workers plus `other` still ran: capping concurrency must not \ + drop work" + ); +} diff --git a/src/graph/compiled/types.rs b/src/graph/compiled/types.rs index 482d903b..1640c14e 100644 --- a/src/graph/compiled/types.rs +++ b/src/graph/compiled/types.rs @@ -64,6 +64,9 @@ pub struct CompiledGraph { pub(crate) parallel: bool, /// Upper bound on concurrently-running branches per step (`None` = unbounded). pub(crate) max_concurrency: Option, + /// Per-node ceilings on concurrent activations of the same node in one step. + /// Empty for a graph that sets none, which is the common case. + pub(crate) node_concurrency: Arc>, /// Default per-node handler timeout (`None` = no timeout). pub(crate) node_timeout: Option, /// Optional whole-run wall-clock deadline (`None` = no deadline). Checked at @@ -115,6 +118,7 @@ impl Clone for CompiledGraph { namespace: self.namespace.clone(), parallel: self.parallel, max_concurrency: self.max_concurrency, + node_concurrency: self.node_concurrency.clone(), node_timeout: self.node_timeout, run_deadline: self.run_deadline, durability: self.durability, diff --git a/src/main.rs b/src/main.rs index ab0eabc6..ff487fd2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -323,6 +323,9 @@ fn standalone_capabilities() -> tinyflows::caps::Capabilities { agent: None, shell: None, memory: None, + // The stub binary refuses every outside-world capability; background + // work is no exception, so `spawn` degrades to running inline. + tasks: None, } } diff --git a/src/model/node_kind.rs b/src/model/node_kind.rs index 88b27508..f2342603 100644 --- a/src/model/node_kind.rs +++ b/src/model/node_kind.rs @@ -69,6 +69,35 @@ pub enum NodeKind { /// [`crate::nodes::control_flow::dedup`] for the full `StateStore` key /// contract this kind depends on. Dedup, + /// Starts work **without waiting for it** and emits a ticket per started + /// task, so the branch carries on while the work runs. A downstream + /// [`NodeKind::Gate`] collects the results. + /// + /// Needs the [`TaskRunner`](crate::caps::TaskRunner) capability to actually + /// overlap; with none injected the work runs inline and the ticket comes + /// back already settled, so the graph still computes the right answer + /// without the concurrency. + Spawn, + /// Fans the **downstream path** out into parallel lanes: each lane runs its + /// own copy of every node between here and the matching [`NodeKind::Gather`]. + /// + /// Different from an ordinary fan-out, which runs each *successor* once. A + /// scatter over 8 items turns a five-node pipeline into 8 concurrent + /// five-node pipelines. + Scatter, + /// Collects the lanes a [`NodeKind::Scatter`] opened, on a release policy. + /// + /// Where lanes end: routing to a gather is a plain activation, so N lanes + /// converge on one gather rather than each running their own. + Gather, + /// Waits for tickets — from [`NodeKind::Spawn`], or named by expression — + /// and emits their results once its release policy is satisfied. + /// + /// The policy (`all` / `any` / `first_n` / `quorum` / `timeout_partial`) + /// is what makes this more than a barrier: a gate can proceed on a quorum + /// and leave the stragglers, or settle for whatever arrived before its + /// deadline. See [`crate::nodes::release`]. + Gate, } /// How a [`NodeKind::Trigger`] node is fired. diff --git a/src/nodes/control_flow/condition.rs b/src/nodes/control_flow/condition.rs index c359be96..b7bd74e1 100644 --- a/src/nodes/control_flow/condition.rs +++ b/src/nodes/control_flow/condition.rs @@ -92,6 +92,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ConditionNode.execute(ctx).await.expect("execute"); ( diff --git a/src/nodes/control_flow/dedup.rs b/src/nodes/control_flow/dedup.rs index 136730cf..ebba3ba9 100644 --- a/src/nodes/control_flow/dedup.rs +++ b/src/nodes/control_flow/dedup.rs @@ -328,6 +328,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; DedupNode.execute(ctx).await.expect("execute") } diff --git a/src/nodes/control_flow/gather.rs b/src/nodes/control_flow/gather.rs new file mode 100644 index 00000000..d9d1cd2b --- /dev/null +++ b/src/nodes/control_flow/gather.rs @@ -0,0 +1,312 @@ +//! The `gather` node: collect the lanes a [`scatter`] opened. +//! +//! # Not a topological barrier +//! +//! A `merge` waits for its declared predecessors — a static fact about the +//! graph. A gather cannot: how many lanes exist is decided at run time by the +//! scatter, from data. So its barrier is **data-driven**: it counts arrivals in +//! `nodes..lanes.*` against the `lane_count` the scatter +//! recorded, and asks to be re-run until its release policy is satisfied. +//! +//! That is also why it supports the same policies as a [`gate`]: once the wait +//! is a decision rather than a topological fact, "proceed on a quorum" and +//! "settle for what arrived" become expressible. +//! +//! [`scatter`]: super::scatter +//! [`gate`]: crate::nodes::integration::gate + +use async_trait::async_trait; +use serde_json::{Value, json}; + +use crate::data::Item; +use crate::error::{EngineError, Result}; +use crate::nodes::release::{Release, ReleasePolicy}; +use crate::nodes::{NodeContext, NodeExecutor, NodeOutput}; + +/// Default gap between checks, in milliseconds. +const DEFAULT_POLL_INTERVAL_MS: u64 = 5; + +/// Default ceiling on checks before the wait is called spent. +const DEFAULT_MAX_POLLS: u64 = 500; + +/// The slot key a gather records its poll count under. +const POLLS_KEY: &str = "polls"; + +/// What a gather does with a lane that failed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum OnLaneError { + /// Emit the failure as an item, branchable with `=item.failed`. + Collect, + /// Drop it. + Skip, + /// Fail the whole gather. + FailFast, +} + +impl OnLaneError { + fn from_config(config: &Value) -> Self { + match config.get("on_lane_error").and_then(Value::as_str) { + Some("skip") => Self::Skip, + Some("fail_fast") => Self::FailFast, + _ => Self::Collect, + } + } +} + +/// Collects the lanes a `scatter` opened. +#[derive(Debug, Default, Clone)] +pub struct GatherNode; + +/// One lane's recorded result. +struct Arrived { + index: usize, + items: Vec, + failed: Option, +} + +/// Reads every lane slot recorded by this gather's lane-terminal predecessors. +/// +/// Lanes are keyed by id under `nodes..lanes`, which is what keeps N +/// concurrent activations of one node from clobbering each other — the reducer +/// merges objects key-by-key, so distinct lane keys never collide. +fn arrivals(ctx: &NodeContext<'_>, predecessors: &[String]) -> Vec { + let mut arrived = Vec::new(); + for pred in predecessors { + let Some(lanes) = ctx + .nodes + .get(pred) + .and_then(|slot| slot.get("lanes")) + .and_then(Value::as_object) + else { + continue; + }; + for slot in lanes.values() { + let index = slot + .get("index") + .and_then(Value::as_u64) + .and_then(|n| usize::try_from(n).ok()) + .unwrap_or(0); + let items: Vec = slot + .get("items") + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| serde_json::from_value::(item.clone()).ok()) + .collect() + }) + .unwrap_or_default(); + let failed = slot + .get("status") + .and_then(Value::as_str) + .filter(|status| *status == "failed") + .map(|_| { + slot.get("error") + .and_then(Value::as_str) + .unwrap_or("lane failed") + .to_string() + }); + arrived.push(Arrived { + index, + items, + failed, + }); + } + } + arrived +} + +/// How many lanes the scatter upstream of this gather opened. +/// +/// Read from the scatter's own slot rather than inferred from arrivals: a +/// gather that guessed "however many turned up" would release immediately on +/// the first one, which is the bug this exists to prevent. +fn expected_lanes(ctx: &NodeContext<'_>) -> Option { + ctx.nodes.as_object()?.values().find_map(|slot| { + slot.get("lane_count") + .and_then(Value::as_u64) + .and_then(|n| usize::try_from(n).ok()) + }) +} + +fn positive_u64(config: &Value, key: &str, default: u64) -> u64 { + config + .get(key) + .and_then(Value::as_u64) + .filter(|n| *n > 0) + .unwrap_or(default) +} + +#[async_trait] +impl NodeExecutor for GatherNode { + async fn execute(&self, ctx: NodeContext<'_>) -> Result { + let config = &ctx.node.config; + let policy = ReleasePolicy::from_config(config, &ctx.node.id)?; + + // Predecessors are named in config rather than derived from edges: the + // executor sees run state, not the graph. `validate` checks they match + // the wiring. + let predecessors: Vec = config + .get("from") + .and_then(Value::as_array) + .map(|from| { + from.iter() + .filter_map(Value::as_str) + .map(str::to_string) + .collect() + }) + .unwrap_or_default(); + + let arrived = arrivals(&ctx, &predecessors); + let expected = expected_lanes(&ctx).unwrap_or(arrived.len()); + + let polls = ctx + .nodes + .get(&ctx.node.id) + .and_then(|slot| slot.get(POLLS_KEY)) + .and_then(Value::as_u64) + .unwrap_or(0); + let max_polls = positive_u64(config, "max_polls", DEFAULT_MAX_POLLS); + let budget_spent = polls >= max_polls; + + let on_lane_error = OnLaneError::from_config(config); + if on_lane_error == OnLaneError::FailFast + && let Some(failure) = arrived.iter().find_map(|lane| lane.failed.as_ref()) + { + return Err(EngineError::Capability(format!( + "gather node {:?}: lane failed and `on_lane_error` is \"fail_fast\": {failure}", + ctx.node.id + ))); + } + + let meta = json!({ + POLLS_KEY: polls + 1, + "lanes": expected, + "arrived": arrived.len(), + "failed": arrived.iter().filter(|lane| lane.failed.is_some()).count(), + }); + + match policy.evaluate(arrived.len(), expected, budget_spent) { + Release::Wait => Ok(NodeOutput::reenter_after( + positive_u64(config, "poll_interval_ms", DEFAULT_POLL_INTERVAL_MS), + meta, + )), + Release::Timeout => Err(EngineError::Capability(format!( + "gather node {:?}: only {} of {expected} lanes arrived within {max_polls} polls; \ + raise `max_polls`, relax `release`, or use `release: \"timeout_partial\"`", + ctx.node.id, + arrived.len() + ))), + Release::Emit => { + let partial = arrived.len() < expected; + Ok( + NodeOutput::main(emit_items(arrived, on_lane_error)).with_meta(json!({ + POLLS_KEY: polls + 1, + "lanes": expected, + "arrived": meta["arrived"], + "failed": meta["failed"], + "partial": partial, + })), + ) + } + } + } +} + +/// Flattens arrived lanes into output items, **ordered by lane index**. +/// +/// Completion order is not emission order. Lanes finish in whatever order their +/// work takes, and two runs of the same graph can differ — so results are sorted +/// back into the order the scatter created them, and each item keeps its lane +/// index as `paired_item`. Without this a scatter/gather pair would be +/// nondeterministic in a way no downstream node could correct for. +fn emit_items(mut arrived: Vec, on_lane_error: OnLaneError) -> Vec { + arrived.sort_by_key(|lane| lane.index); + let mut out = Vec::new(); + for lane in arrived { + match (&lane.failed, on_lane_error) { + (Some(_), OnLaneError::Skip) => {} + (Some(error), _) => out.push( + Item::new(json!({ "failed": true, "error": error, "lane": lane.index })) + .paired_with(lane.index), + ), + (None, _) => out.extend( + lane.items + .into_iter() + .map(|item| item.paired_with(lane.index)), + ), + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn lane(index: usize, value: &str) -> Arrived { + Arrived { + index, + items: vec![Item::new(json!({ "v": value }))], + failed: None, + } + } + + /// The determinism property: output follows lane order, not arrival order. + #[test] + fn lanes_are_emitted_in_index_order_whatever_the_arrival_order() { + let arrived = vec![lane(2, "c"), lane(0, "a"), lane(1, "b")]; + let items = emit_items(arrived, OnLaneError::Collect); + let values: Vec<&str> = items + .iter() + .filter_map(|item| item.json["v"].as_str()) + .collect(); + assert_eq!(values, vec!["a", "b", "c"]); + assert_eq!( + items.iter().map(|i| i.paired_item).collect::>(), + vec![Some(0), Some(1), Some(2)], + "each item keeps the index of the lane it came from" + ); + } + + #[test] + fn a_failed_lane_becomes_a_branchable_item_by_default() { + let arrived = vec![ + lane(0, "ok"), + Arrived { + index: 1, + items: vec![], + failed: Some("boom".to_string()), + }, + ]; + let items = emit_items(arrived, OnLaneError::Collect); + assert_eq!(items.len(), 2); + assert_eq!(items[1].json["failed"], true); + assert_eq!(items[1].json["error"], "boom"); + } + + #[test] + fn skip_drops_a_failed_lane_entirely() { + let arrived = vec![ + lane(0, "ok"), + Arrived { + index: 1, + items: vec![], + failed: Some("boom".to_string()), + }, + ]; + let items = emit_items(arrived, OnLaneError::Skip); + assert_eq!(items.len(), 1, "only the successful lane survives"); + } + + #[test] + fn on_lane_error_defaults_to_collect() { + assert_eq!(OnLaneError::from_config(&json!({})), OnLaneError::Collect); + assert_eq!( + OnLaneError::from_config(&json!({ "on_lane_error": "nonsense" })), + OnLaneError::Collect, + "an unrecognised policy must not silently fail the run" + ); + } +} diff --git a/src/nodes/control_flow/loop_node.rs b/src/nodes/control_flow/loop_node.rs index 56a964fa..cdedc684 100644 --- a/src/nodes/control_flow/loop_node.rs +++ b/src/nodes/control_flow/loop_node.rs @@ -15,13 +15,32 @@ //! see [`crate::engine`]. This node then decides, on each activation, whether to //! send its input round the `body` again or let it out through `done`. //! -//! **Why the counter lives in run state.** The iteration count is written to -//! this node's own slot via [`NodeOutput::meta`], so it is part of the state the -//! engine checkpoints. A loop therefore resumes mid-iteration with its count -//! intact, and the count is addressable from any expression in the graph as -//! `=nodes..iteration`. Holding it in the executor instead would lose -//! it on every pause, and threading it through the items would lose it to the -//! first node in the body that reshapes them. +//! **Why the counter and the accumulator live in run state.** Both are written +//! to this node's own slot via [`NodeOutput::meta`], so they are part of the +//! state the engine checkpoints. A loop therefore resumes mid-iteration with +//! both intact, and both are addressable from any expression in the graph as +//! `=nodes..iteration` / `=nodes..state`. Holding them in the +//! executor instead would lose them on every pause, and threading them through +//! the items would lose them to the first node in the body that reshapes them. +//! +//! **The accumulator is a fold.** `state.init` seeds it once; `state.update` +//! folds each pass's body output into it, so `acc_next = f(acc_prev, output)`. +//! This node is the *sole writer* of that slot, which is what keeps it simple: +//! no reducer collision, no question of which branch wrote last, no interaction +//! with the staleness stamping that loop re-entry uses. +//! +//! Because the reducer merges objects key-by-key, an accumulator written +//! plainly could only ever *gain* keys — an error recorded on pass 1 would +//! haunt every later pass. The accumulator is therefore written through +//! [`crate::engine::replace`], which assigns the slot wholesale. +//! +//! **The fold is at-least-once.** If an activation is replayed after a resume, +//! the update applies twice. This is not new — `iteration + 1` has always had +//! the same property — but an accumulator makes it visible, as a duplicated +//! append. Fixing it properly means stamping the fold with the super-step that +//! produced it and skipping a repeat, which should be done for the counter and +//! the accumulator together. Until then, an idempotent `update` (assign the +//! next value rather than appending to the previous one) is immune. use async_trait::async_trait; use serde_json::{Value, json}; @@ -78,6 +97,164 @@ fn current_iteration(ctx: &NodeContext) -> u64 { .unwrap_or(0) } +/// What the exit ports carry. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum EmitMode { + /// The last pass's items (the default, and what a loop without an + /// accumulator has always emitted). + Items, + /// One item holding the accumulator. + State, + /// The last pass's items, with the accumulator appended. + Both, +} + +impl EmitMode { + fn from_config(config: &Value) -> Self { + match config.get("emit").and_then(Value::as_str) { + Some("state") => Self::State, + Some("both") => Self::Both, + _ => Self::Items, + } + } + + /// Builds the items an exit port carries. + fn items(self, items: &[crate::data::Item], state: &Value) -> Vec { + match self { + Self::Items => items.to_vec(), + Self::State => vec![crate::data::Item::new(state.clone())], + Self::Both => { + let mut out = items.to_vec(); + out.push(crate::data::Item::new(state.clone())); + out + } + } + } +} + +/// Whether an `until` exit should leave on its own `success` port. +fn success_port(config: &Value) -> bool { + config + .get("success_port") + .and_then(Value::as_bool) + .unwrap_or(false) +} + +/// The accumulator this node recorded on its previous activation. +fn current_state(ctx: &NodeContext) -> Option { + ctx.nodes + .get(&ctx.node.id) + .and_then(|slot| slot.get("state")) + .cloned() +} + +/// The accumulator's starting value, from `config.state.init`. +/// +/// Resolved on the seeding activation only, so an expression in `init` sees the +/// run as it was when the loop began rather than being re-evaluated per pass. +fn initial_state(ctx: &NodeContext) -> Result { + let Some(init) = ctx + .node + .config + .get("state") + .and_then(|state| state.get("init")) + else { + return Ok(Value::Null); + }; + Ok(if init.as_str().is_some_and(expr::is_expression) { + expr::evaluate(init, &expr_scope(ctx)) + } else { + init.clone() + }) +} + +/// Applies `config.state.update` to the accumulator, given the body's output. +/// +/// The scope is the node's usual one plus a `state` key holding the *previous* +/// accumulator, so an update reads `state` and the body's items together: +/// `acc_next = f(acc_prev, body_output)`. +/// +/// Two spellings, both supported because they suit different authors: a single +/// jq program folding the whole accumulator, or an object of per-key +/// expressions (mirroring `transform.set`), which is what someone who does not +/// write jq will reach for. +fn fold_state(ctx: &NodeContext) -> Result { + let previous = match current_state(ctx) { + Some(state) => state, + // No recorded accumulator: this loop either declares none, or is being + // re-entered after its slot was never written. Fall back to `init` + // rather than folding into null. + None => initial_state(ctx)?, + }; + let Some(update) = ctx + .node + .config + .get("state") + .and_then(|state| state.get("update")) + else { + return Ok(previous); + }; + + let mut scope = expr_scope(ctx); + if let Some(map) = scope.as_object_mut() { + map.insert("state".to_string(), previous.clone()); + } + + match update { + // Object form: each key is resolved independently and merged over the + // previous accumulator, so an update naming one key leaves the rest. + Value::Object(fields) => { + let mut next = previous; + let entries: Vec<(String, Value)> = fields + .iter() + .map(|(key, raw)| { + let value = if raw.as_str().is_some_and(expr::is_expression) { + expr::evaluate(raw, &scope) + } else { + raw.clone() + }; + (key.clone(), value) + }) + .collect(); + if !next.is_object() { + next = json!({}); + } + if let Some(map) = next.as_object_mut() { + for (key, value) in entries { + map.insert(key, value); + } + } + Ok(next) + } + // Program form: one jq expression producing the whole next accumulator. + raw if raw.as_str().is_some_and(expr::is_expression) => Ok(expr::evaluate(raw, &scope)), + // A literal: the accumulator simply becomes it. + raw => Ok(raw.clone()), + } +} + +/// Whether the node's optional `config.until` expression is truthy against the +/// **post-fold** accumulator. +/// +/// Opposite polarity to `condition`, deliberately: `condition` says *keep going +/// while*, `until` says *stop when*. Both are supported because a real loop +/// often has both a work-remaining test and a success test. +fn until_holds(ctx: &NodeContext, state: &Value) -> bool { + let Some(until) = ctx.node.config.get("until") else { + return false; + }; + let mut scope = expr_scope(ctx); + if let Some(map) = scope.as_object_mut() { + map.insert("state".to_string(), state.clone()); + } + let resolved = if until.as_str().is_some_and(expr::is_expression) { + expr::evaluate(until, &scope) + } else { + until.clone() + }; + is_truthy(&resolved) +} + /// Whether the node's optional `config.condition` expression is truthy. /// /// Returns `true` when no condition is configured, so a loop bounded only by @@ -120,18 +297,49 @@ impl NodeExecutor for LoopNode { .and_then(Value::as_u64) .unwrap_or(DEFAULT_MAX_ITERATIONS); let items = ctx.input.to_vec(); - // The count is recorded on every path, including the exits, so a host - // reading the finished run can see how many passes actually happened. - let done = |iteration: u64| { - Ok(NodeOutput::routed(items.clone(), "done") - .with_meta(json!({ "iteration": iteration }))) + + // Fold the body's output into the accumulator, before any exit is + // considered — so `until` tests the state *including* the pass that just + // finished, which is what "stop when the check passes" has to mean. + // + // Only on re-entry: on the seeding activation the body has not run, so + // there is nothing to fold and `init` stands. + let state = if iteration > 0 { + fold_state(&ctx)? + } else { + initial_state(&ctx)? + }; + let emit_mode = EmitMode::from_config(&ctx.node.config); + + // Every path records the count and the accumulator, so a host reading a + // finished run sees both how many passes happened and what they built. + let exit = |iteration: u64, reason: &str, port: &str| { + Ok( + NodeOutput::routed(emit_mode.items(&items, &state), port).with_meta(json!({ + "iteration": iteration, + "state": crate::engine::replace(state.clone()), + "exit_reason": reason, + })), + ) }; + // `until` is the accumulator's own exit: truthy means the check passed. + // Checked first because converging is a better outcome than either + // running out of work or running out of tries. + if until_holds(&ctx, &state) { + let port = if success_port(&ctx.node.config) { + "success" + } else { + "done" + }; + return exit(iteration, "until", port); + } + // The condition is checked before the cap so a loop that finishes early // on its own terms never trips the limit, and checked before the // iteration is consumed so `condition: false` exits without a pass. if !condition_holds(&ctx) { - return done(iteration); + return exit(iteration, "condition", "done"); } if iteration >= max_iterations { @@ -140,11 +348,14 @@ impl NodeExecutor for LoopNode { node: ctx.node.id.clone(), limit: max_iterations, }), - OnExceeded::Continue => done(iteration), + OnExceeded::Continue => exit(iteration, "max_iterations", "done"), }; } - Ok(NodeOutput::routed(items, "body").with_meta(json!({ "iteration": iteration + 1 }))) + Ok(NodeOutput::routed(items, "body").with_meta(json!({ + "iteration": iteration + 1, + "state": crate::engine::replace(state), + }))) } } @@ -183,6 +394,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await } @@ -193,7 +407,10 @@ mod tests { .await .expect("execute"); assert_eq!(out.port.as_deref(), Some("body")); - assert_eq!(out.meta, Some(json!({ "iteration": 1 }))); + assert_eq!( + out.meta.as_ref().and_then(|m| m.get("iteration")), + Some(&json!(1)) + ); assert_eq!(out.items.len(), 1, "input passes through to the body"); } @@ -206,7 +423,10 @@ mod tests { .await .expect("execute"); assert_eq!(out.port.as_deref(), Some("body")); - assert_eq!(out.meta, Some(json!({ "iteration": 3 }))); + assert_eq!( + out.meta.as_ref().and_then(|m| m.get("iteration")), + Some(&json!(3)) + ); } #[tokio::test] @@ -235,7 +455,10 @@ mod tests { .await .expect("execute"); assert_eq!(out.port.as_deref(), Some("done")); - assert_eq!(out.meta, Some(json!({ "iteration": 3 }))); + assert_eq!( + out.meta.as_ref().and_then(|m| m.get("iteration")), + Some(&json!(3)) + ); assert_eq!(out.items.len(), 1, "the last pass's items reach downstream"); } @@ -265,7 +488,10 @@ mod tests { Some("done"), "no `keep_going` field resolves null, which is falsey" ); - assert_eq!(out.meta, Some(json!({ "iteration": 0 }))); + assert_eq!( + out.meta.as_ref().and_then(|m| m.get("iteration")), + Some(&json!(0)) + ); } #[tokio::test] diff --git a/src/nodes/control_flow/merge.rs b/src/nodes/control_flow/merge.rs index 7343cd26..7627497e 100644 --- a/src/nodes/control_flow/merge.rs +++ b/src/nodes/control_flow/merge.rs @@ -56,6 +56,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let output = MergeNode.execute(ctx).await.expect("execute"); @@ -77,6 +80,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; MergeNode.execute(ctx).await.expect("execute").items } diff --git a/src/nodes/control_flow/mod.rs b/src/nodes/control_flow/mod.rs index bfec59fe..139537f0 100644 --- a/src/nodes/control_flow/mod.rs +++ b/src/nodes/control_flow/mod.rs @@ -8,16 +8,20 @@ pub mod condition; pub mod dedup; +pub mod gather; pub mod loop_node; pub mod merge; +pub mod scatter; pub mod split_out; pub mod switch; pub mod transform; pub use condition::ConditionNode; pub use dedup::DedupNode; +pub use gather::GatherNode; pub use loop_node::{DEFAULT_MAX_ITERATIONS, LoopNode}; pub use merge::MergeNode; +pub use scatter::{MAX_LANES, ScatterNode}; pub use split_out::SplitOutNode; pub use switch::SwitchNode; pub use transform::TransformNode; diff --git a/src/nodes/control_flow/scatter.rs b/src/nodes/control_flow/scatter.rs new file mode 100644 index 00000000..3bf8d35e --- /dev/null +++ b/src/nodes/control_flow/scatter.rs @@ -0,0 +1,178 @@ +//! The `scatter` node: fan the *downstream path* out into parallel lanes. +//! +//! # How this differs from an ordinary fan-out +//! +//! Drawing two edges from one port already runs both successors concurrently. +//! What that cannot express is running the same *pipeline* several times over +//! different data: `split_out → agent → score → merge` runs `agent` once with +//! N items, not N times with one item each. Widening `agent`'s own per-item +//! concurrency helps only that node — `score` still sees the whole batch. +//! +//! A scatter opens **lanes**. Every node between it and its [`gather`] runs once +//! per lane, so a five-node pipeline becomes N concurrent five-node pipelines, +//! each carrying its own slice. +//! +//! [`gather`]: super::gather + +use async_trait::async_trait; +use serde_json::{Value, json}; + +use crate::data::Item; +use crate::error::{EngineError, Result}; +use crate::nodes::{NodeContext, NodeExecutor, NodeOutput}; + +/// The ceiling on how many lanes one scatter may open. +/// +/// Clamped rather than refused, matching how per-item `concurrency` is treated: +/// a graph asking for a lane per row of a 100k-row table has a mistake in it, +/// and running it sensibly while saying so beats refusing the run outright. +pub const MAX_LANES: usize = 256; + +/// Fans the downstream path out into one lane per slice of its input. +#[derive(Debug, Default, Clone)] +pub struct ScatterNode; + +/// Splits `items` into at most `lanes` slices, keeping input order. +/// +/// With `lanes` unset every item gets its own lane — the common intent, and +/// what makes a scatter feel like "run this per item". With `lanes: n` the input +/// is chunked into at most `n` slices, so a 1000-item input can run 8 wide +/// instead of 1000 wide without the author pre-chunking it. +fn split(items: &[Item], lanes: Option) -> Vec> { + if items.is_empty() { + return Vec::new(); + } + let requested = lanes.unwrap_or(items.len()).clamp(1, MAX_LANES); + if requested >= items.len() { + return items.iter().map(|item| vec![item.clone()]).collect(); + } + // Ceiling division, so the last chunk is the short one rather than the + // split producing more chunks than lanes were asked for. + let per_lane = items.len().div_ceil(requested); + items.chunks(per_lane).map(<[Item]>::to_vec).collect() +} + +#[async_trait] +impl NodeExecutor for ScatterNode { + async fn execute(&self, ctx: NodeContext<'_>) -> Result { + // A scatter nested inside a lane would need lane ids that compose and a + // gather that knows which level it is closing. Refused rather than + // silently mis-collected; `validate` catches the static case, and this + // covers a graph that reached the engine another way. + if ctx.lane.is_some() { + return Err(EngineError::Capability(format!( + "scatter node {:?}: nested scatter is not supported — this activation is \ + already inside a lane", + ctx.node.id + ))); + } + + // Which items to fan out over. `path` reads an array out of the first + // item (like `split_out`) for the common "one lane per row of this + // field" shape; without it the node's own input items are the lanes. + let items: Vec = match ctx.node.config.get("path").and_then(Value::as_str) { + Some(path) => { + let source = ctx.input.first().map(|item| &item.json); + let array = path + .split('.') + .fold(source, |value, segment| value.and_then(|v| v.get(segment))) + .and_then(Value::as_array); + match array { + Some(values) => values.iter().cloned().map(Item::new).collect(), + // Not an array: one lane carrying the input unchanged, the + // same fail-soft `split_out` uses. + None => ctx.input.to_vec(), + } + } + None => ctx.input.to_vec(), + }; + + let requested = ctx + .node + .config + .get("lanes") + .and_then(Value::as_u64) + .and_then(|n| usize::try_from(n).ok()) + .filter(|n| *n > 0); + if let Some(n) = requested + && n > MAX_LANES + { + tracing::warn!( + node = %ctx.node.id, + requested = n, + max = MAX_LANES, + "scatter lanes above the engine ceiling; clamping" + ); + } + + let lanes = split(&items, requested); + tracing::debug!( + node = %ctx.node.id, + lanes = lanes.len(), + items = items.len(), + "scatter: splitting work into lanes" + ); + // `lane_count` goes in the node's own slot, which is what a gather + // counts arrivals against — inferring the count from however many lanes + // happened to have reported would release on the first one. + let count = lanes.len(); + Ok(NodeOutput::scatter(lanes, json!({ "lane_count": count }))) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn items(n: usize) -> Vec { + (0..n).map(|i| Item::new(json!({ "i": i }))).collect() + } + + #[test] + fn without_a_lane_count_every_item_gets_its_own_lane() { + let lanes = split(&items(4), None); + assert_eq!(lanes.len(), 4); + assert!(lanes.iter().all(|lane| lane.len() == 1)); + } + + #[test] + fn a_lane_count_chunks_the_input_and_preserves_order() { + let lanes = split(&items(7), Some(3)); + assert_eq!(lanes.len(), 3, "at most the requested number of lanes"); + let flattened: Vec = lanes + .iter() + .flatten() + .filter_map(|item| item.json["i"].as_i64()) + .collect(); + assert_eq!( + flattened, + (0..7).collect::>(), + "chunking must not reorder the work" + ); + } + + /// Asking for more lanes than there are items yields one lane each, not a + /// pile of empty lanes a gather would then wait on forever. + #[test] + fn more_lanes_than_items_yields_one_lane_per_item() { + let lanes = split(&items(2), Some(9)); + assert_eq!(lanes.len(), 2); + } + + #[test] + fn an_empty_input_opens_no_lanes() { + assert!(split(&[], None).is_empty()); + assert!(split(&[], Some(4)).is_empty()); + } + + #[test] + fn the_lane_count_is_clamped_to_the_ceiling() { + let lanes = split(&items(MAX_LANES + 50), Some(MAX_LANES + 50)); + assert!( + lanes.len() <= MAX_LANES, + "opened {} lanes, above the ceiling", + lanes.len() + ); + } +} diff --git a/src/nodes/control_flow/split_out.rs b/src/nodes/control_flow/split_out.rs index 71a32f90..d5d71d5c 100644 --- a/src/nodes/control_flow/split_out.rs +++ b/src/nodes/control_flow/split_out.rs @@ -91,6 +91,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let output = SplitOutNode.execute(ctx).await.expect("execute"); @@ -116,6 +119,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let output = SplitOutNode.execute(ctx).await.expect("execute"); @@ -140,6 +146,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let output = SplitOutNode.execute(ctx).await.expect("execute"); @@ -161,6 +170,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; SplitOutNode.execute(ctx).await.expect("execute").items } diff --git a/src/nodes/control_flow/switch.rs b/src/nodes/control_flow/switch.rs index 3bd8e529..80832b3c 100644 --- a/src/nodes/control_flow/switch.rs +++ b/src/nodes/control_flow/switch.rs @@ -94,6 +94,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = SwitchNode.execute(ctx).await.expect("execute"); (out.port.expect("switch always routes to a port"), out.items) @@ -118,6 +121,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = SwitchNode.execute(ctx).await.expect("execute"); assert_eq!(out.port.as_deref(), Some("urgent")); diff --git a/src/nodes/control_flow/transform.rs b/src/nodes/control_flow/transform.rs index 9369452b..57cec85c 100644 --- a/src/nodes/control_flow/transform.rs +++ b/src/nodes/control_flow/transform.rs @@ -89,6 +89,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; TransformNode.execute(ctx).await.expect("execute").items } @@ -116,6 +119,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = TransformNode.execute(ctx).await.expect("execute").items; assert_eq!(out[0].json["who"], json!("a@b.com")); diff --git a/src/nodes/integration/agent.rs b/src/nodes/integration/agent.rs index de16298d..1abf52aa 100644 --- a/src/nodes/integration/agent.rs +++ b/src/nodes/integration/agent.rs @@ -84,7 +84,7 @@ impl NodeExecutor for AgentNode { // Fan out: `config.concurrency` decides how many turns run at once // (default 1 — sequential, as this node has always behaved), and // `config.on_item_error` what a failing turn does to the batch. - let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id); + let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id, ctx.run); let ctx = &ctx; let (items, diagnostics) = crate::nodes::map::map_items( ctx.input.len(), @@ -437,6 +437,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect("execute"); @@ -455,6 +458,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect("execute"); @@ -479,6 +485,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = AgentNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1); @@ -507,6 +516,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = AgentNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["completion"]["prompt"], "X"); @@ -527,6 +539,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = AgentNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["connection"], Value::Null); @@ -553,6 +568,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = AgentNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1); @@ -583,6 +601,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; AgentNode .execute(ctx) @@ -746,6 +767,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = AgentNode .execute(ctx) @@ -886,6 +910,9 @@ mod tests { agents, observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect("execute"); @@ -1050,6 +1077,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect_err("an unresolved required block must fail the node"); @@ -1125,6 +1155,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect_err("a pause must not be reported as a finished answer"); diff --git a/src/nodes/integration/code.rs b/src/nodes/integration/code.rs index 9d126549..292d88b1 100644 --- a/src/nodes/integration/code.rs +++ b/src/nodes/integration/code.rs @@ -112,6 +112,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; CodeNode.execute(ctx).await.expect("execute").items } diff --git a/src/nodes/integration/gate.rs b/src/nodes/integration/gate.rs new file mode 100644 index 00000000..5c4a71a1 --- /dev/null +++ b/src/nodes/integration/gate.rs @@ -0,0 +1,370 @@ +//! The `gate` node: wait for spawned work, on a policy. +//! +//! A gate collects the tickets a [`spawn`](super::spawn) node produced and emits +//! their results. What makes it more than a barrier is the release policy: it +//! can proceed on the first result, on a quorum, or on whatever arrived before +//! its deadline, rather than only on all of them (see [`crate::nodes::release`]). +//! +//! # How waiting works in a super-step engine +//! +//! There is no "block here until the callback comes". A gate is activated, +//! looks at the world, and either proceeds or asks to be run again — via +//! [`NodeControl::Reenter`], which commits its notes and re-activates it in the +//! next super-step. Waiting is therefore counted in **polls**, and every poll +//! costs a super-step and a node visit against the run's budgets. That is why +//! the poll count is bounded here rather than left to the run-level backstop: +//! the backstop cannot say which node span, and a gate that spun forever would +//! look exactly like a runaway loop. +//! +//! For a wait measured in minutes rather than milliseconds, `wait_mode: +//! "suspend"` interrupts the run instead, so nothing is burned while waiting and +//! the host resumes it when the work lands. + +use async_trait::async_trait; +use serde_json::{Value, json}; + +use crate::caps::TaskState; +use crate::data::Item; +use crate::error::{EngineError, Result}; +use crate::nodes::release::{Release, ReleasePolicy}; +use crate::nodes::{NodeContext, NodeExecutor, NodeOutput}; + +/// Default gap between polls, in milliseconds. +const DEFAULT_POLL_INTERVAL_MS: u64 = 250; + +/// Default ceiling on polls before the wait budget is called spent. +/// +/// Finite on purpose. An unbounded gate is the runaway case this bound exists +/// to prevent, and the run-level backstop reports only that the *run* spun, +/// never which gate was responsible. +const DEFAULT_MAX_POLLS: u64 = 200; + +/// The slot key a gate records its poll count under, so the count survives a +/// checkpoint the same way a `loop` node's iteration does. +const POLLS_KEY: &str = "polls"; + +/// How a gate waits. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum WaitMode { + /// Re-activate and look again. Cheap for short waits; costs a super-step + /// per poll. + Poll, + /// Interrupt the run and let the host resume it when the work lands. Right + /// for waits long enough that burning super-steps would be absurd. + Suspend, +} + +impl WaitMode { + fn from_config(config: &Value) -> Self { + match config.get("wait_mode").and_then(Value::as_str) { + Some("suspend") => Self::Suspend, + _ => Self::Poll, + } + } +} + +/// What a gate does when its wait budget runs out and the policy will not +/// settle for a partial result. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum OnTimeout { + /// Emit whatever arrived. + Partial, + /// Fail the node. + Error, + /// Route to the `timeout` port. + Route, +} + +impl OnTimeout { + fn from_config(config: &Value) -> Self { + match config.get("on_timeout").and_then(Value::as_str) { + Some("partial") => Self::Partial, + Some("route") => Self::Route, + _ => Self::Error, + } + } +} + +/// Collects spawned work and emits it once its release policy is satisfied. +#[derive(Debug, Default, Clone)] +pub struct GateNode; + +/// One thing the gate is waiting on. +struct Awaited { + /// The ticket, or `None` for an inline-degraded spawn that is already done. + ticket: Option, + /// A result already in hand (inline spawn, or delivered by a resume). + settled: Option, +} + +/// The tickets this gate waits on, in the order they will be emitted. +/// +/// Two spellings: `from` names upstream `spawn` nodes and reads their tickets +/// out of the run state, and `tickets` is an expression yielding ticket ids for +/// a graph that carries them some other way. `from` is the common case, and the +/// one that keeps a workflow readable — it names nodes, not strings. +fn awaited(ctx: &NodeContext<'_>) -> Result> { + let mut awaited = Vec::new(); + + if let Some(from) = ctx.node.config.get("from").and_then(Value::as_array) { + for source in from.iter().filter_map(Value::as_str) { + let items = ctx + .nodes + .get(source) + .and_then(|slot| slot.get("items")) + .and_then(Value::as_array); + let Some(items) = items else { + // The spawn has not run yet. Not an error: on the gate's first + // activation its upstream may still be in flight. + continue; + }; + for item in items { + let json = item.get("json").unwrap_or(&Value::Null); + awaited.push(Awaited { + ticket: json + .get(super::spawn::TICKET_KEY) + .and_then(Value::as_str) + .map(str::to_string), + settled: super::spawn::inline_result(json), + }); + } + } + return Ok(awaited); + } + + // Expression form: resolve to a ticket id or an array of them. + let Some(raw) = ctx.node.config.get("tickets") else { + return Err(EngineError::Capability(format!( + "gate node {:?}: needs `from` (spawn node ids) or `tickets` (an expression)", + ctx.node.id + ))); + }; + let resolved = if raw.as_str().is_some_and(crate::expr::is_expression) { + crate::expr::evaluate(raw, &crate::nodes::expr_scope(ctx)) + } else { + raw.clone() + }; + let ids: Vec<&Value> = match &resolved { + Value::Array(values) => values.iter().collect(), + Value::Null => Vec::new(), + single => vec![single], + }; + for id in ids { + if let Some(ticket) = id.as_str() { + awaited.push(Awaited { + ticket: Some(ticket.to_string()), + settled: None, + }); + } + } + Ok(awaited) +} + +/// Results delivered out of band by a resume, keyed by ticket. +fn delivered(ctx: &NodeContext<'_>) -> serde_json::Map { + ctx.resume + .as_ref() + .and_then(|value| value.get("delivered")) + .and_then(Value::as_object) + .cloned() + .unwrap_or_default() +} + +/// Reads a positive integer config field, falling back to `default`. +fn positive_u64(config: &Value, key: &str, default: u64) -> u64 { + config + .get(key) + .and_then(Value::as_u64) + .filter(|n| *n > 0) + .unwrap_or(default) +} + +#[async_trait] +impl NodeExecutor for GateNode { + async fn execute(&self, ctx: NodeContext<'_>) -> Result { + let config = &ctx.node.config; + let policy = ReleasePolicy::from_config(config, &ctx.node.id)?; + let awaiting = awaited(&ctx)?; + let expected = awaiting.len(); + + // Poll count carried in this node's own slot, so it survives a + // checkpoint — the same reason a `loop` node keeps its iteration there. + let polls = ctx + .nodes + .get(&ctx.node.id) + .and_then(|slot| slot.get(POLLS_KEY)) + .and_then(Value::as_u64) + .unwrap_or(0); + let max_polls = positive_u64(config, "max_polls", DEFAULT_MAX_POLLS); + + // Collect what has settled. Three sources, in order of authority: + // a result already in hand, one delivered by a resume, then the runner. + let delivered = delivered(&ctx); + let mut results: Vec<(usize, Value)> = Vec::new(); + let mut failures: Vec = Vec::new(); + for (index, item) in awaiting.iter().enumerate() { + let state = if let Some(settled) = item.settled.clone() { + Some(settled) + } else if let Some(value) = item + .ticket + .as_ref() + .and_then(|ticket| delivered.get(ticket.as_str())) + { + Some(TaskState::Done(value.clone())) + } else if let Some(ticket) = item.ticket.as_ref() { + match ctx.caps.tasks.as_ref() { + Some(runner) => Some(runner.poll(ticket).await?), + None => { + return Err(EngineError::Capability(format!( + "gate node {:?}: waiting on ticket {ticket:?} but no TaskRunner is \ + injected; the spawn that made it must have used one", + ctx.node.id + ))); + } + } + } else { + None + }; + match state { + Some(TaskState::Done(value)) => results.push((index, value)), + Some(TaskState::Failed(message)) => { + failures.push(message.clone()); + results.push((index, json!({ "failed": true, "error": message }))); + } + _ => {} + } + } + + let budget_spent = polls >= max_polls; + let decision = policy.evaluate(results.len(), expected, budget_spent); + + let meta = json!({ + POLLS_KEY: polls + 1, + "arrived": results.len(), + "expected": expected, + "failed": failures.len(), + }); + + match decision { + Release::Wait => { + let interval = positive_u64(config, "poll_interval_ms", DEFAULT_POLL_INTERVAL_MS); + match WaitMode::from_config(config) { + // Suspending discards this activation's update, so the poll + // count deliberately does not advance: a suspended gate is + // not spending its poll budget, it is waiting for the host. + WaitMode::Suspend => Ok(NodeOutput::interrupt( + ctx.node.id.clone(), + json!({ + "kind": "await", + "node": ctx.node.id, + "tickets": awaiting + .iter() + .filter_map(|a| a.ticket.clone()) + .collect::>(), + "arrived": results.len(), + "expected": expected, + }), + )), + WaitMode::Poll => Ok(NodeOutput::reenter_after(interval, meta)), + } + } + Release::Timeout => match OnTimeout::from_config(config) { + OnTimeout::Error => Err(EngineError::Capability(format!( + "gate node {:?}: timed out after {max_polls} polls with {} of {expected} \ + results; raise `max_polls`/`poll_interval_ms`, relax `release`, or wire a \ + `timeout` port", + ctx.node.id, + results.len() + ))), + OnTimeout::Route => { + Ok(NodeOutput::routed(emit_items(results), "timeout").with_meta(meta)) + } + OnTimeout::Partial => Ok(NodeOutput::main(emit_items(results)).with_meta(meta)), + }, + Release::Emit => Ok(NodeOutput::main(emit_items(results)).with_meta(meta)), + } + } +} + +/// Turns collected results into output items, **ordered by ticket index**. +/// +/// Completion order is not emission order. Two runs of the same graph can have +/// their tickets settle in different orders, and downstream must not be able to +/// tell — so results are sorted back into the order the tickets were listed, +/// and each carries its `paired_item` so the correlation survives. +fn emit_items(mut results: Vec<(usize, Value)>) -> Vec { + results.sort_by_key(|(index, _)| *index); + results + .into_iter() + .map(|(index, value)| Item::new(value).paired_with(index)) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn wait_mode_defaults_to_polling() { + assert_eq!(WaitMode::from_config(&json!({})), WaitMode::Poll); + assert_eq!( + WaitMode::from_config(&json!({ "wait_mode": "suspend" })), + WaitMode::Suspend + ); + assert_eq!( + WaitMode::from_config(&json!({ "wait_mode": "nonsense" })), + WaitMode::Poll, + "an unrecognised mode must not silently suspend the run" + ); + } + + /// Failing closed: an unknown `on_timeout` must not be read as `partial`, + /// which would emit an incomplete result as though it were complete. + #[test] + fn on_timeout_defaults_to_error() { + assert_eq!(OnTimeout::from_config(&json!({})), OnTimeout::Error); + assert_eq!( + OnTimeout::from_config(&json!({ "on_timeout": "eventually" })), + OnTimeout::Error + ); + assert_eq!( + OnTimeout::from_config(&json!({ "on_timeout": "partial" })), + OnTimeout::Partial + ); + } + + /// Emission order follows the ticket list, not completion order — the + /// property that keeps a gate deterministic under any timing. + #[test] + fn results_are_emitted_in_ticket_order_regardless_of_arrival() { + // Deliberately out of order, as a real race would deliver them. + let results = vec![ + (2, json!("third")), + (0, json!("first")), + (1, json!("second")), + ]; + let items = emit_items(results); + let values: Vec<&Value> = items.iter().map(|item| &item.json).collect(); + assert_eq!( + values, + vec![&json!("first"), &json!("second"), &json!("third")] + ); + assert_eq!( + items.iter().map(|i| i.paired_item).collect::>(), + vec![Some(0), Some(1), Some(2)], + "each result keeps the index of the ticket it came from" + ); + } + + #[test] + fn positive_config_fields_fall_back_on_zero_or_garbage() { + assert_eq!(positive_u64(&json!({}), "max_polls", 7), 7); + assert_eq!(positive_u64(&json!({ "max_polls": 0 }), "max_polls", 7), 7); + assert_eq!( + positive_u64(&json!({ "max_polls": "x" }), "max_polls", 7), + 7 + ); + assert_eq!(positive_u64(&json!({ "max_polls": 3 }), "max_polls", 7), 3); + } +} diff --git a/src/nodes/integration/http_request.rs b/src/nodes/integration/http_request.rs index 391d887f..4185fb49 100644 --- a/src/nodes/integration/http_request.rs +++ b/src/nodes/integration/http_request.rs @@ -36,7 +36,7 @@ impl NodeExecutor for HttpRequestNode { if per_item { // `config.concurrency` decides how many requests are in flight at // once (default 1 — sequential, as before). - let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id); + let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id, ctx.run); let ctx = &ctx; let (items, diagnostics) = crate::nodes::map::map_items( ctx.input.len(), @@ -147,6 +147,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = HttpRequestNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1); @@ -186,6 +189,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = HttpRequestNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["request"]["url"], "https://a"); @@ -216,6 +222,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = HttpRequestNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["connection"], Value::Null); diff --git a/src/nodes/integration/memory.rs b/src/nodes/integration/memory.rs index abd606a1..1b09c1e7 100644 --- a/src/nodes/integration/memory.rs +++ b/src/nodes/integration/memory.rs @@ -226,7 +226,7 @@ impl NodeExecutor for MemoryNode { if per_item { // `config.concurrency` decides how many provider calls are in flight // at once (default 1 — sequential, as before). - let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id); + let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id, ctx.run); let ctx = &ctx; let (items, diagnostics) = crate::nodes::map::map_items( ctx.input.len(), @@ -412,6 +412,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = MemoryNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 2, "per_item default maps over input"); @@ -469,6 +472,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = MemoryNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["opts"]["operation"], "search"); @@ -490,6 +496,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = MemoryNode .execute(ctx) @@ -518,6 +527,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = MemoryNode .execute(ctx) @@ -563,6 +575,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = MemoryNode .execute(ctx) @@ -590,6 +605,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = MemoryNode .execute(ctx) @@ -617,6 +635,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = MemoryNode .execute(ctx) @@ -648,6 +669,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = MemoryNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1, "once mode emits a single item"); diff --git a/src/nodes/integration/mod.rs b/src/nodes/integration/mod.rs index 2c0ae089..04a0a432 100644 --- a/src/nodes/integration/mod.rs +++ b/src/nodes/integration/mod.rs @@ -8,20 +8,24 @@ pub mod agent; pub(crate) mod agent_request; pub mod code; pub(crate) mod envelope; +pub mod gate; pub mod http_request; pub mod memory; pub mod output_parser; pub(crate) mod schema; pub mod shell; +pub mod spawn; pub mod sub_workflow; pub mod tool_call; pub use agent::AgentNode; pub use code::CodeNode; +pub use gate::GateNode; pub use http_request::HttpRequestNode; pub use memory::MemoryNode; pub use output_parser::OutputParserNode; pub use shell::ShellNode; +pub use spawn::SpawnNode; pub use sub_workflow::SubWorkflowNode; pub use tool_call::ToolCallNode; diff --git a/src/nodes/integration/output_parser.rs b/src/nodes/integration/output_parser.rs index 1fde5641..2d46ee43 100644 --- a/src/nodes/integration/output_parser.rs +++ b/src/nodes/integration/output_parser.rs @@ -85,6 +85,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = OutputParserNode.execute(ctx).await.expect("execute"); assert_eq!(out.items, input); @@ -115,6 +118,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; OutputParserNode.execute(ctx).await.expect("execute").items } @@ -180,6 +186,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; OutputParserNode.execute(ctx).await.map(|o| o.items) } diff --git a/src/nodes/integration/shell_tests.rs b/src/nodes/integration/shell_tests.rs index 22d8eafc..2f736744 100644 --- a/src/nodes/integration/shell_tests.rs +++ b/src/nodes/integration/shell_tests.rs @@ -39,6 +39,9 @@ async fn execute_with(caps: Capabilities, config: Value) -> Result { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await } diff --git a/src/nodes/integration/spawn.rs b/src/nodes/integration/spawn.rs new file mode 100644 index 00000000..d275e229 --- /dev/null +++ b/src/nodes/integration/spawn.rs @@ -0,0 +1,188 @@ +//! The `spawn` node: start work, don't wait for it. +//! +//! Every other node in the catalog blocks its branch until it has an answer. +//! This one starts the work through the [`TaskRunner`](crate::caps::TaskRunner) +//! capability and immediately emits a **ticket**, so the branch continues while +//! the work runs. A downstream [`gate`](super::gate) turns tickets back into +//! results. +//! +//! # Degrading without a `TaskRunner` +//! +//! A host that injects none still runs these graphs: the work is performed +//! inline and the ticket comes back already settled. The answer is identical; +//! what is lost is the overlap. That is worth stating loudly rather than +//! leaving as a surprise, because it is a performance cliff that no test of +//! correctness will catch. + +use async_trait::async_trait; +use serde_json::{Value, json}; + +use crate::caps::{TaskSpec, TaskState}; +use crate::data::Item; +use crate::error::{EngineError, Result}; +use crate::nodes::{NodeContext, NodeExecutor, NodeOutput}; + +/// The key a spawned task's ticket is emitted under. A `gate` reads this back +/// when it is pointed at a spawn node rather than at an expression. +pub(crate) const TICKET_KEY: &str = "ticket"; + +/// Starts background work and emits one ticket item per started task. +#[derive(Debug, Default, Clone)] +pub struct SpawnNode; + +/// Builds the [`TaskSpec`] a node's config describes. +fn spec_from_config(config: &Value, node_id: &str) -> Result { + match config.get("target").and_then(Value::as_str) { + Some("workflow") => { + let graph = config.get("workflow").cloned().ok_or_else(|| { + EngineError::Capability(format!( + "spawn node {node_id:?}: `target: \"workflow\"` needs a `workflow` graph" + )) + })?; + Ok(TaskSpec::Workflow { + graph, + input: config.get("input").cloned().unwrap_or(Value::Null), + }) + } + Some("tool") => { + let slug = config + .get("slug") + .and_then(Value::as_str) + .ok_or_else(|| { + EngineError::Capability(format!( + "spawn node {node_id:?}: `target: \"tool\"` needs a `slug`" + )) + })? + .to_string(); + Ok(TaskSpec::Tool { + slug, + args: config.get("args").cloned().unwrap_or(Value::Null), + }) + } + Some("http") => Ok(TaskSpec::Http { + request: config.get("request").cloned().unwrap_or(Value::Null), + }), + Some(other) => Err(EngineError::Capability(format!( + "spawn node {node_id:?}: unknown `target` {other:?}; expected workflow, tool or http" + ))), + None => Err(EngineError::Capability(format!( + "spawn node {node_id:?}: missing `target` (workflow, tool or http)" + ))), + } +} + +/// Runs `spec` inline, for a host that injected no [`TaskRunner`]. +/// +/// Returns the result directly; the caller wraps it in a settled ticket so +/// downstream sees the same shape either way. +async fn run_inline(spec: TaskSpec, ctx: &NodeContext<'_>) -> Result { + match spec { + TaskSpec::Tool { slug, args } => ctx.caps.tools.invoke(&slug, args, None).await, + TaskSpec::Http { request } => ctx.caps.http.request(request, None).await, + TaskSpec::Workflow { graph, input } => { + let child: crate::model::WorkflowGraph = + serde_json::from_value(graph).map_err(|e| { + EngineError::Capability(format!("spawn node: invalid workflow: {e}")) + })?; + let compiled = crate::compiler::compile(&child)?; + let outcome = Box::pin(crate::engine::run(&compiled, input, ctx.caps)).await?; + Ok(outcome.output) + } + } +} + +#[async_trait] +impl NodeExecutor for SpawnNode { + async fn execute(&self, ctx: NodeContext<'_>) -> Result { + let spec = spec_from_config(&ctx.node.config, &ctx.node.id)?; + + let item = match ctx.caps.tasks.as_ref() { + Some(runner) => { + let ticket = runner.start(spec).await?; + tracing::debug!(node = %ctx.node.id, %ticket, "spawn: started background task"); + Item::new(json!({ + TICKET_KEY: ticket, + "spawn": ctx.node.id, + "started_at_step": ctx.step, + })) + } + None => { + // No runner: do the work here and hand back a ticket that is + // already settled, so a downstream gate needs no special case. + tracing::debug!( + node = %ctx.node.id, + "spawn: no TaskRunner injected; running inline (no overlap)" + ); + let result = run_inline(spec, &ctx).await?; + Item::new(json!({ + TICKET_KEY: Value::Null, + "spawn": ctx.node.id, + "started_at_step": ctx.step, + "inline": true, + "result": result, + })) + } + }; + Ok(NodeOutput::main(vec![item])) + } +} + +/// Reads an already-settled inline result off a ticket item, if it has one. +/// +/// A gate uses this so an inline-degraded spawn collects without ever asking +/// the (absent) runner about a ticket that does not exist. +pub(crate) fn inline_result(item: &Value) -> Option { + item.get("inline") + .and_then(Value::as_bool) + .unwrap_or(false) + .then(|| TaskState::Done(item.get("result").cloned().unwrap_or(Value::Null))) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn every_target_shape_parses() { + assert!(matches!( + spec_from_config(&json!({ "target": "tool", "slug": "a.b" }), "s"), + Ok(TaskSpec::Tool { .. }) + )); + assert!(matches!( + spec_from_config(&json!({ "target": "http", "request": {} }), "s"), + Ok(TaskSpec::Http { .. }) + )); + assert!(matches!( + spec_from_config(&json!({ "target": "workflow", "workflow": {} }), "s"), + Ok(TaskSpec::Workflow { .. }) + )); + } + + /// A misconfigured spawn fails at the node rather than starting something + /// unintended — the error names the node and what was expected. + #[test] + fn a_missing_or_unknown_target_is_refused() { + for config in [ + json!({}), + json!({ "target": "sideways" }), + json!({ "target": "tool" }), // no slug + json!({ "target": "workflow" }), // no graph + ] { + assert!( + spec_from_config(&config, "s").is_err(), + "config {config} should be refused" + ); + } + } + + #[test] + fn an_inline_ticket_reports_its_result_without_a_runner() { + let item = json!({ "inline": true, "result": { "ok": true } }); + assert_eq!( + inline_result(&item), + Some(TaskState::Done(json!({ "ok": true }))) + ); + assert_eq!(inline_result(&json!({ "ticket": "task-1" })), None); + } +} diff --git a/src/nodes/integration/sub_workflow.rs b/src/nodes/integration/sub_workflow.rs index b3ac33c2..d1d70ddc 100644 --- a/src/nodes/integration/sub_workflow.rs +++ b/src/nodes/integration/sub_workflow.rs @@ -1,7 +1,7 @@ //! The `sub_workflow` node: runs another workflow as a nested sub-graph. use async_trait::async_trait; -use serde_json::Value; +use serde_json::{Value, json}; use crate::engine::MAX_SUB_WORKFLOW_DEPTH; use crate::error::{EngineError, Result}; @@ -87,6 +87,63 @@ use crate::nodes::{NodeContext, NodeExecutor, NodeOutput}; #[derive(Debug, Default, Clone)] pub struct SubWorkflowNode; +/// Separates a `sub_workflow` node's id from a gate id inside its child. +/// +/// Parent and child are separate graphs with separate id spaces, so a child's +/// gate `approve` and a parent's gate `approve` are different gates that would +/// otherwise be indistinguishable in one pending set. +const GATE_NAMESPACE: &str = "::"; + +/// Qualifies a child gate id with the node that ran the child. +fn namespaced_gate(node_id: &str, gate: &str) -> String { + format!("{node_id}{GATE_NAMESPACE}{gate}") +} + +/// The child-gate ids approved for `node_id`, taken from the parent run's +/// accumulated approvals with the namespace stripped. +/// +/// This is how an approval crosses the boundary. `engine::resume` unions newly +/// approved ids into `run.trigger.approvals`, so on the re-run this node finds +/// the ones addressed to it and hands them to the child as *its* approvals. +/// Ids belonging to the parent or to a different `sub_workflow` node are left +/// alone. +fn approvals_for_child(ctx: &NodeContext<'_>) -> Vec { + let prefix = format!("{}{GATE_NAMESPACE}", ctx.node.id); + let strip = |ids: &Value| -> Vec { + ids.as_array() + .map(|ids| { + ids.iter() + .filter_map(Value::as_str) + .filter_map(|id| id.strip_prefix(prefix.as_str())) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() + }; + + // Two channels, because the engine has two resume paths and they deliver + // approvals differently. + // + // `engine::resume` re-executes the workflow with the approvals merged into + // the run input, so they arrive in `run.trigger.approvals`. The checkpointed + // path replays from the checkpoint instead and hands the resume value + // straight to the node that interrupted — this node — so they arrive in + // `ctx.resume`. Reading only one of the two makes cross-boundary approval + // work on one path and silently hang on the other. + let mut approved: Vec = ctx + .run + .get("trigger") + .and_then(|trigger| trigger.get("approvals")) + .map(&strip) + .unwrap_or_default(); + if let Some(resume) = ctx.resume.as_ref().and_then(|value| value.get("approved")) { + approved.extend(strip(resume)); + } + approved.sort(); + approved.dedup(); + approved +} + /// Reads the current nesting depth from the run metadata (`0` at the top level). fn current_depth(run: &Value) -> u64 { run.get("sub_workflow_depth") @@ -175,7 +232,7 @@ impl NodeExecutor for SubWorkflowNode { && !ctx.input.is_empty(); if per_item { - let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id); + let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id, ctx.run); let ctx = &ctx; let (items, _) = crate::nodes::map::map_items( ctx.input.len(), @@ -193,9 +250,17 @@ impl NodeExecutor for SubWorkflowNode { // exactly one output per input index, so stand in with an // empty item — the whole node's output is discarded by the // token check below, so this placeholder never surfaces. - let child = run_child(ctx, &scope, std::slice::from_ref(item)) - .await? - .unwrap_or_else(|| crate::data::Item::new(Value::Null)); + // A paused child is reported as a marker item rather than + // an error: the map slots exactly one output per input + // index, and the gates are collected across the whole batch + // below so one pause does not hide the others. + let child = match run_child(ctx, &scope, std::slice::from_ref(item)).await? { + ChildOutcome::Finished(item) => item, + ChildOutcome::Cancelled => crate::data::Item::new(Value::Null), + ChildOutcome::Paused(gates) => { + crate::data::Item::new(json!({ PAUSED_MARKER: gates })) + } + }; Ok((child, vec![])) }, ) @@ -208,20 +273,84 @@ impl NodeExecutor for SubWorkflowNode { if ctx.token.is_cancelled() { return Ok(NodeOutput::empty()); } + // Any child that paused pauses the whole node. Gates from every + // paused child are unioned, so a host sees all of them at once + // rather than discovering them one fan-out element at a time. + let paused: Vec = items + .iter() + .filter_map(|item| item.json.get(PAUSED_MARKER)) + .filter_map(Value::as_array) + .flatten() + .filter_map(Value::as_str) + .map(str::to_string) + .collect(); + if !paused.is_empty() { + return Ok(pause_for_child_gates(&ctx.node.id, paused)); + } return Ok(NodeOutput::main(items)); } let scope = crate::nodes::expr_scope(&ctx); match run_child(&ctx, &scope, ctx.input).await? { - Some(item) => Ok(NodeOutput::main(vec![item])), + ChildOutcome::Finished(item) => Ok(NodeOutput::main(vec![item])), // Parent-initiated cancel wound the child down: emit nothing, the // same clean wind-down a top-level cancelled node performs. The // parent's next boundary check settles `cancelled = true`. - None => Ok(NodeOutput::empty()), + ChildOutcome::Cancelled => Ok(NodeOutput::empty()), + ChildOutcome::Paused(gates) => Ok(pause_for_child_gates(&ctx.node.id, gates)), } } } +/// Marks a per-item child result that paused rather than finished. +/// +/// Underscore-prefixed so it cannot collide with a child's own output keys. It +/// never reaches downstream: the fan-out collects these, pauses the node, and +/// the interrupt discards the items. +const PAUSED_MARKER: &str = "_sub_workflow_paused"; + +/// Builds the parent-side pause for a child that stopped at approval gates. +/// +/// The interrupt id is the first gate, because a run's pending set is keyed by +/// interrupt id and a node emits one interrupt. The payload carries the full +/// list, and a host may approve several at once — the next re-run seeds all of +/// them into the child, so a child with N gates need not cost N round trips. +fn pause_for_child_gates(node_id: &str, gates: Vec) -> NodeOutput { + tracing::info!( + node = %node_id, + ?gates, + "sub_workflow: child paused awaiting approval; pausing the parent" + ); + let first = gates + .first() + .cloned() + .unwrap_or_else(|| node_id.to_string()); + NodeOutput::interrupt( + first, + json!({ + "kind": "sub_workflow_approval", + "node": node_id, + "pending": gates, + }), + ) +} + +/// What one child run produced, from the parent node's point of view. +/// +/// Three outcomes rather than two: a child can finish, wind down because the +/// parent cancelled, or **pause** at an approval gate. The last one is not a +/// failure and not a result — it has to travel up as its own case so the parent +/// node can pause too, rather than being flattened into an item or an error. +enum ChildOutcome { + /// The child ran to completion; its final state is this item. + Finished(crate::data::Item), + /// The parent cancelled mid-child. A clean cooperative wind-down. + Cancelled, + /// The child stopped at one or more approval gates, named here with the + /// parent-facing namespace already applied. + Paused(Vec), +} + /// Resolves this node's child graph and runs it once, returning the child's /// final run state as a single [`Item`](crate::data::Item). /// @@ -229,7 +358,7 @@ impl NodeExecutor for SubWorkflowNode { /// input for `once`, the current element for `per_item`), and `child_input` is /// the item array seeded into the child run. /// -/// Returns `Ok(None)` when the parent run cancelled this child mid-flight +/// Returns [`ChildOutcome::Cancelled`] when the parent run cancelled this child mid-flight /// (`ctx.token` is set): the child is a clean cooperative wind-down, not a /// failure, so it emits no item and lets the parent settle as cancelled. A child /// that stops for any *other* reason (a `requires_approval` pause, or a cancel @@ -238,7 +367,7 @@ async fn run_child( ctx: &NodeContext<'_>, scope: &Value, child_input: &[crate::data::Item], -) -> Result> { +) -> Result { // The inline `workflow` graph carries its *own* `=`-expressions, scoped // to the CHILD run — it must pass through untouched. Only the fields the // sub_workflow node itself reads (here `workflow_id`) are resolved @@ -296,6 +425,14 @@ async fn run_child( let compiled = crate::compiler::compile(&child)?; let trigger = serde_json::to_value(child_input).map_err(|e| EngineError::Capability(e.to_string()))?; + // Approvals the parent has accumulated for *this* node's child gates. Empty + // on a first run; populated after a resume, which is what lets the re-run + // get past the gate that paused it. + // + // Delivered through `RunInput::with_approvals` rather than written into the + // trigger payload: a child is seeded with its input *items*, so its trigger + // is an array and has nowhere to carry an `approvals` key. + let child_approvals = approvals_for_child(ctx); // Resolved against the same `scope` as `workflow_id`, so a `per_item` run // forwards values derived from *its* element (`"=item.repo"`) rather than // from the batch — the whole point of resolving inputs in here rather than @@ -308,7 +445,9 @@ async fn run_child( // nesting chain shares one cancellation signal. let outcome = Box::pin(crate::engine::run_sub_workflow( &compiled, - crate::engine::RunInput::new(trigger).with_inputs(child_inputs), + crate::engine::RunInput::new(trigger) + .with_inputs(child_inputs) + .with_approvals(child_approvals), ctx.caps, child_depth, depth_cap, @@ -320,33 +459,32 @@ async fn run_child( // // The child run is a *separate* engine invocation whose non-completion is // reported on its [`RunOutcome`], not on the [`NodeOutput`] this node - // returns. A node executor has no channel to inject a graph interrupt - // into the *parent* run (the parent's `pending_approvals` are collected - // solely from its own boundary interrupts), so we cannot yet transparently - // pause the parent and resume the child at its gate. What we MUST NOT do is - // keep only `outcome.output` and report success — that silently treats a - // child that paused at a `requires_approval` gate (or was cancelled) as if - // it had run to completion, making approval gating unenforceable across the - // boundary. + // returns. What must never happen is keeping only `outcome.output` and + // reporting success — that would silently treat a child paused at a + // `requires_approval` gate as if it had run to completion, making approval + // gating unenforceable across the boundary. // - // Until full cross-boundary resume exists, fail loudly: a child that did - // not fully complete halts the parent with an error rather than letting it - // falsely complete. With the default `on_error: stop` policy this stops the - // parent run; with `continue`/`route` it becomes a routable error item — - // either way the gated child is never silently treated as completed. + // A paused child now **pauses the parent** rather than failing it, via + // [`NodeControl::Interrupt`]. The child's gate ids are namespaced by this + // node's id (`::`) so they cannot collide with the + // parent's own gates, and so this node can recognise its own approvals when + // it re-runs. // - // Follow-up for full cross-boundary resume: surface the child's - // `pending_approvals` (namespaced by this node's id) into the parent's - // pending set via a real interrupt at this node's boundary, and teach - // `engine::resume` to re-enter the child at its paused gate. That needs - // engine-level interrupt plumbing this node cannot express today. + // Resume works the way `engine::resume` already works everywhere else: by + // re-executing with the merged approval set rather than replaying a + // checkpoint. The approvals accumulate in the parent's + // `run.trigger.approvals`; on the re-run this node reads back the ones + // addressed to it, strips the namespace, and seeds them into the child's + // trigger — so the child gets past the gate that stopped it. Nothing needs + // to share a checkpointer, and the child is deterministic, so re-running it + // reaches the same place. if !outcome.pending_approvals.is_empty() { - return Err(EngineError::Capability(format!( - "sub_workflow node {:?}: child run paused awaiting approval at {:?}; \ - cross-boundary approval resume is not yet supported, so the parent run is \ - halted rather than falsely completed", - ctx.node.id, outcome.pending_approvals - ))); + let namespaced: Vec = outcome + .pending_approvals + .iter() + .map(|gate| namespaced_gate(&ctx.node.id, gate)) + .collect(); + return Ok(ChildOutcome::Paused(namespaced)); } if outcome.cancelled { // Two cancellations look the same on the child's `RunOutcome` but mean @@ -369,7 +507,7 @@ async fn run_child( node = %ctx.node.id, "sub_workflow: child wound down under the parent's cancellation; emitting no output" ); - return Ok(None); + return Ok(ChildOutcome::Cancelled); } return Err(EngineError::Capability(format!( "sub_workflow node {:?}: child run was cancelled before completing; the parent \ @@ -378,7 +516,9 @@ async fn run_child( ))); } - Ok(Some(crate::data::Item::new(outcome.output))) + Ok(ChildOutcome::Finished(crate::data::Item::new( + outcome.output, + ))) } #[cfg(test)] @@ -423,6 +563,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; SubWorkflowNode .execute(ctx) @@ -448,6 +591,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; SubWorkflowNode.execute(ctx).await.expect("execute") } @@ -714,6 +860,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; SubWorkflowNode.execute(ctx).await } diff --git a/src/nodes/integration/tool_call.rs b/src/nodes/integration/tool_call.rs index 6527a8cd..2e2072ad 100644 --- a/src/nodes/integration/tool_call.rs +++ b/src/nodes/integration/tool_call.rs @@ -45,7 +45,7 @@ impl NodeExecutor for ToolCallNode { // `=item.x` binds to the current item) and invoke once per item. // `config.concurrency` decides how many of those invocations are in // flight at once (default 1 — sequential, as before). - let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id); + let opts = crate::nodes::map::map_options(&ctx.node.config, &ctx.node.id, ctx.run); let ctx = &ctx; let (items, diagnostics) = crate::nodes::map::map_items( ctx.input.len(), @@ -182,6 +182,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let err = ToolCallNode .execute(ctx) @@ -210,6 +213,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ToolCallNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["tool"], "x.y"); @@ -237,6 +243,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ToolCallNode.execute(ctx).await.expect("execute"); assert_eq!(out.items[0].json["json"]["args"]["to"], Value::Null); @@ -260,6 +269,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ToolCallNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1); @@ -289,6 +301,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ToolCallNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 3, "one output per input item"); @@ -319,6 +334,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let out = ToolCallNode.execute(ctx).await.expect("execute"); assert_eq!(out.items.len(), 1, "once mode emits a single item"); diff --git a/src/nodes/map.rs b/src/nodes/map.rs index f5b12f1b..1adbf8a9 100644 --- a/src/nodes/map.rs +++ b/src/nodes/map.rs @@ -120,8 +120,25 @@ impl Default for MapOptions { /// Unrecognized values fall back to the defaults rather than erroring; /// [`crate::validate`] rejects them at author time, where the message can point /// at the offending node. +/// The run-level per-item concurrency ceiling, from +/// `trigger.config.max_item_concurrency`. +/// +/// `None` when unset or not a positive integer, meaning each node is bounded +/// only by its own `concurrency` and [`MAX_CONCURRENCY`]. +fn run_item_cap(run: &Value) -> Option { + run.get("trigger") + .and_then(|trigger| trigger.get("max_item_concurrency")) + .and_then(Value::as_u64) + .filter(|n| *n > 0) + .map(|n| { + usize::try_from(n) + .unwrap_or(MAX_CONCURRENCY) + .min(MAX_CONCURRENCY) + }) +} + #[must_use] -pub(crate) fn map_options(config: &Value, node_id: &str) -> MapOptions { +pub(crate) fn map_options(config: &Value, node_id: &str, run: &Value) -> MapOptions { let concurrency = match config.get("concurrency") { Some(Value::Number(n)) => n .as_u64() @@ -142,6 +159,26 @@ pub(crate) fn map_options(config: &Value, node_id: &str) -> MapOptions { concurrency }; + // A run-level ceiling the whole workflow shares, declared once on the + // trigger instead of edited into every node. It only ever lowers a node's + // own `concurrency`, so a node asking for less keeps its own number. + // + // `0` (the "all" spelling) means unbounded, which is exactly what a run-level + // cap is for, so it is clamped like any other value rather than treated as + // already-satisfied. + let concurrency = match run_item_cap(run) { + Some(cap) if concurrency == 0 || concurrency > cap => { + tracing::debug!( + node = %node_id, + requested = concurrency, + cap, + "per-item concurrency lowered by the run-level cap" + ); + cap + } + _ => concurrency, + }; + // The default follows the execution shape: a fan-out collects (one bad item // must not discard the batch), while a sequential run keeps failing fast so // the node's `on_error` / retry policy still sees the error. An explicit @@ -586,7 +623,7 @@ mod tests { fn options_default_to_sequential_and_fail_fast() { // The pre-fan-out behaviour, unchanged: one at a time, and a failure // reaches the node's own `on_error` / retry policy. - let o = map_options(&json!({}), "n"); + let o = map_options(&json!({}), "n", &Value::Null); assert_eq!(o.concurrency, 1, "unset concurrency stays sequential"); assert_eq!(o.on_item_error, ItemErrorPolicy::FailFast); } @@ -596,7 +633,7 @@ mod tests { // Opting into concurrency opts into batch semantics: one bad item must // not discard the other results. for concurrency in [0, 2, 8] { - let o = map_options(&json!({ "concurrency": concurrency }), "n"); + let o = map_options(&json!({ "concurrency": concurrency }), "n", &Value::Null); assert_eq!( o.on_item_error, ItemErrorPolicy::Collect, @@ -605,7 +642,7 @@ mod tests { } // ...but an explicit `concurrency: 1` is not a fan-out. assert_eq!( - map_options(&json!({ "concurrency": 1 }), "n").on_item_error, + map_options(&json!({ "concurrency": 1 }), "n", &Value::Null).on_item_error, ItemErrorPolicy::FailFast ); } @@ -613,14 +650,15 @@ mod tests { #[test] fn an_explicit_policy_overrides_the_shape_derived_default() { assert_eq!( - map_options(&json!({ "on_item_error": "collect" }), "n").on_item_error, + map_options(&json!({ "on_item_error": "collect" }), "n", &Value::Null).on_item_error, ItemErrorPolicy::Collect, "sequential can opt into collecting" ); assert_eq!( map_options( &json!({ "concurrency": 8, "on_item_error": "fail_fast" }), - "n" + "n", + &Value::Null ) .on_item_error, ItemErrorPolicy::FailFast, @@ -631,23 +669,72 @@ mod tests { #[test] fn options_read_numeric_and_all_concurrency() { assert_eq!( - map_options(&json!({ "concurrency": 8 }), "n").concurrency, + map_options(&json!({ "concurrency": 8 }), "n", &Value::Null).concurrency, 8 ); assert_eq!( - map_options(&json!({ "concurrency": 0 }), "n").concurrency, + map_options(&json!({ "concurrency": 0 }), "n", &Value::Null).concurrency, 0 ); assert_eq!( - map_options(&json!({ "concurrency": "all" }), "n").concurrency, + map_options(&json!({ "concurrency": "all" }), "n", &Value::Null).concurrency, 0, "`\"all\"` is the readable spelling of unbounded" ); } + /// The run-level cap lowers a node that asked for more, and leaves alone a + /// node that asked for less — it is a ceiling, not an assignment. + #[test] + fn the_run_level_cap_only_lowers_a_nodes_own_concurrency() { + let run = json!({ "trigger": { "max_item_concurrency": 4 } }); + assert_eq!( + map_options(&json!({ "concurrency": 16 }), "n", &run).concurrency, + 4, + "a node above the run cap is lowered to it" + ); + assert_eq!( + map_options(&json!({ "concurrency": 2 }), "n", &run).concurrency, + 2, + "a node below the run cap keeps its own smaller value" + ); + } + + /// `"all"` / `0` means unbounded, which is exactly the case a run-level cap + /// exists to bound — so it is capped rather than treated as satisfied. + #[test] + fn the_run_level_cap_bounds_an_unbounded_node() { + let run = json!({ "trigger": { "max_item_concurrency": 3 } }); + assert_eq!( + map_options(&json!({ "concurrency": "all" }), "n", &run).concurrency, + 3 + ); + assert_eq!( + map_options(&json!({ "concurrency": 0 }), "n", &run).concurrency, + 3 + ); + } + + /// An absent, zero, or malformed cap leaves node concurrency untouched. + #[test] + fn a_missing_or_invalid_run_level_cap_changes_nothing() { + for run in [ + json!({}), + json!({ "trigger": {} }), + json!({ "trigger": { "max_item_concurrency": 0 } }), + json!({ "trigger": { "max_item_concurrency": "lots" } }), + ] { + assert_eq!( + map_options(&json!({ "concurrency": 8 }), "n", &run).concurrency, + 8, + "cap {run} should not change the node's own concurrency" + ); + } + } + #[test] fn options_clamp_an_absurd_concurrency_instead_of_failing_the_run() { - let o = map_options(&json!({ "concurrency": 10_000 }), "n"); + let o = map_options(&json!({ "concurrency": 10_000 }), "n", &Value::Null); assert_eq!(o.concurrency, MAX_CONCURRENCY); } @@ -656,23 +743,29 @@ mod tests { // `validate` rejects these at author time; at run time they must not // silently become unbounded. assert_eq!( - map_options(&json!({ "concurrency": "lots" }), "n").concurrency, + map_options(&json!({ "concurrency": "lots" }), "n", &Value::Null).concurrency, 1 ); assert_eq!( - map_options(&json!({ "concurrency": -3 }), "n").concurrency, + map_options(&json!({ "concurrency": -3 }), "n", &Value::Null).concurrency, 1 ); assert_eq!( - map_options(&json!({ "concurrency": true }), "n").concurrency, + map_options(&json!({ "concurrency": true }), "n", &Value::Null).concurrency, 1 ); } #[test] fn options_read_every_item_error_policy() { - let policy = - |v| map_options(&json!({ "concurrency": 4, "on_item_error": v }), "n").on_item_error; + let policy = |v| { + map_options( + &json!({ "concurrency": 4, "on_item_error": v }), + "n", + &Value::Null, + ) + .on_item_error + }; assert_eq!(policy("fail_fast"), ItemErrorPolicy::FailFast); assert_eq!(policy("skip"), ItemErrorPolicy::Skip); assert_eq!(policy("collect"), ItemErrorPolicy::Collect); diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 54060c2e..58d7b967 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -9,6 +9,7 @@ pub mod control_flow; pub mod integration; pub(crate) mod map; +pub(crate) mod release; use async_trait::async_trait; use serde_json::Value; @@ -60,6 +61,49 @@ pub struct NodeContext<'a> { /// outside world within a single node need not consult it; the engine /// already checks it at the node boundary before this node runs. pub token: CancellationToken, + /// The parallel lane this activation belongs to, when it is one of several + /// concurrent activations of the *same* node produced by a fan-out. + /// + /// `None` for an ordinary activation, which is every activation today. A + /// lane activation takes its input from the lane envelope rather than from + /// its predecessors' run-state slots, because every lane sees the same + /// committed state snapshot and would otherwise all read identical items. + pub lane: Option, + /// The value a checkpointed resume delivered to this node, when this + /// activation is the re-run of a node that had interrupted. + /// + /// `None` on an ordinary activation. A node that pauses the run with + /// [`NodeControl::Interrupt`] reads this on its re-run to learn what the + /// host decided — which is the only channel on the checkpointed resume path, + /// since that path replays from the checkpoint rather than re-executing with + /// a merged run input. + pub resume: Option, + /// The super-step that is running this activation, counting from 0. + /// + /// A monotonic tick that does not depend on the wall clock, so it stays + /// meaningful across a checkpointed resume. A node that must recognise its + /// own earlier activations (a poll budget, a fold that must not re-apply) + /// compares against a step it recorded in its slot. + pub step: usize, +} + +/// Which lane of a fan-out an activation belongs to. +/// +/// Travels with the activation itself rather than through the run state, so N +/// concurrent activations of one node id are told apart without any of them +/// having to write a shared slot. +#[derive(Debug, Clone, PartialEq)] +pub struct LaneContext { + /// Unique across the run; conventionally `"#"`. + pub id: String, + /// The node whose fan-out created this lane. + pub origin: String, + /// This lane's position in the fan-out, from 0. Output is ordered by this + /// rather than by completion, so a run is deterministic under any timing. + pub index: usize, + /// How many lanes the fan-out created, which is what a collector counts + /// arrivals against. + pub count: usize, } /// Builds the expression scope for a node from its runtime [`NodeContext`]. @@ -200,10 +244,19 @@ pub(crate) fn nodes_scope(nodes: &Value) -> Value { let mut entry = serde_json::json!({ "item": first, "items": jsons }); // Slot state a node recorded about itself via `NodeOutput::meta`, // promoted so expressions can read it the same way they read items. - // Currently just the `loop` node's pass counter, which is what makes - // `=nodes..iteration` resolve from anywhere in the graph. - if let Some(iteration) = slot.get("iteration") { - entry["iteration"] = iteration.clone(); + // This is what makes `=nodes..iteration` and + // `=nodes..state` resolve from anywhere in the graph. + // + // Promoted by exclusion rather than by an allow-list: `items` and + // `port` are the slot's own structure and are already projected + // above, and `_`-prefixed keys are engine bookkeeping. Everything + // else is something a node chose to record about itself, and a list + // naming each one would need a line per new meta key. + for (key, value) in slot.as_object().into_iter().flatten() { + if key == "items" || key == "port" || key.starts_with('_') { + continue; + } + entry[key.as_str()] = value.clone(); } scope.insert(id.clone(), entry); } @@ -284,6 +337,66 @@ pub struct NodeOutput { /// /// Must be a JSON object; anything else is ignored when the slot is built. pub meta: Option, + /// A request to the engine for something other than "emit and move on". + /// `None` — the overwhelmingly common case — means ordinary data flow. + /// + /// This is the channel that lets an executor return *control* rather than + /// only data. Without it a node can express "here are my items" and "I + /// failed", but not "pause the run here" or "ask me again shortly" — which + /// is why, before this existed, a `sub_workflow` whose child paused at an + /// approval gate had to fail the parent outright rather than pause it. + pub control: Option, +} + +/// What a node asks the engine to do instead of simply emitting its items. +/// +/// Both variants are lowered in [`crate::engine`]'s node handler, which is the +/// only place that can speak to the underlying super-step executor. +#[derive(Debug, Clone, PartialEq)] +pub enum NodeControl { + /// Pause the whole run here, surfacing `id` on the run's pending set. + /// + /// The engine turns this into a `tinyagents` interrupt, which **discards + /// this activation's state update** — the node re-runs from the top when + /// the run is resumed. An executor using this must therefore be safe to + /// re-enter: record what it needs to recognise the resume (a ticket, a + /// child thread id) in its slot on an *earlier* activation, not this one. + Interrupt { + /// Identifies the pause to the host, and addresses the resume value + /// back to this node. Conventionally the node id. + id: String, + /// Host-facing description of what is being waited on. + payload: Value, + }, + /// Re-activate this same node in the next super-step, after a delay. + /// + /// State-driven waiting: the node's update *is* committed, so it can leave + /// itself notes, and it is then re-run to look at the world again. This is + /// how a gather or gate waits for lanes and tickets that settle in + /// different super-steps. + /// + /// Each poll costs one super-step and one node visit against the run's + /// budgets, so **every** user of this must carry its own bounded poll count + /// rather than relying on the run-level backstop to stop it. + Reenter { + /// How long to wait before the next activation, in milliseconds. + after_ms: u64, + }, + /// Fan this node's successors out into `lanes` **parallel copies**, one per + /// entry, each carrying its own slice of the work. + /// + /// The difference from an ordinary fan-out is what gets duplicated. A + /// fan-out runs each *successor* once; a scatter runs the *whole downstream + /// path* once per lane, so a five-node pipeline becomes N concurrent + /// five-node pipelines. That is only expressible as a routing decision — + /// the engine schedules one activation per (lane × successor), each with its + /// own input — so it comes back through this channel rather than as items. + Scatter { + /// The work for each lane, in lane order. Emission downstream is + /// ordered by this index rather than by completion, so a run is + /// reproducible whatever the timing. + lanes: Vec>, + }, } impl NodeOutput { @@ -327,6 +440,42 @@ impl NodeOutput { self.meta = Some(meta); self } + + /// Attaches a control request to this output (see [`NodeOutput::control`]). + #[must_use] + pub fn with_control(mut self, control: NodeControl) -> Self { + self.control = Some(control); + self + } + + /// Builds an output that pauses the run, surfacing `id` on its pending set. + /// + /// Carries no items: an interrupt discards this activation's update, so + /// anything set here would be thrown away. + #[must_use] + pub fn interrupt(id: impl Into, payload: Value) -> Self { + Self::empty().with_control(NodeControl::Interrupt { + id: id.into(), + payload, + }) + } + + /// Builds an output that commits `meta` and asks to be re-run after + /// `after_ms` milliseconds. + #[must_use] + pub fn reenter_after(after_ms: u64, meta: Value) -> Self { + Self::empty() + .with_meta(meta) + .with_control(NodeControl::Reenter { after_ms }) + } + + /// Builds an output that fans the downstream path out into parallel lanes. + #[must_use] + pub fn scatter(lanes: Vec>, meta: Value) -> Self { + Self::empty() + .with_meta(meta) + .with_control(NodeControl::Scatter { lanes }) + } } /// Executes one node kind. @@ -371,6 +520,10 @@ pub(crate) fn executor_for(kind: &NodeKind) -> Box { NodeKind::SplitOut => Box::new(control_flow::SplitOutNode), NodeKind::Transform => Box::new(control_flow::TransformNode), NodeKind::Dedup => Box::new(control_flow::DedupNode), + NodeKind::Scatter => Box::new(control_flow::ScatterNode), + NodeKind::Gather => Box::new(control_flow::GatherNode), + NodeKind::Spawn => Box::new(integration::SpawnNode), + NodeKind::Gate => Box::new(integration::GateNode), NodeKind::Loop => Box::new(control_flow::LoopNode), } } @@ -455,6 +608,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await; assert!( @@ -481,6 +637,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }) .await .expect("execute"); @@ -512,6 +671,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let scope = expr_scope(&ctx); // Existing keys unchanged (back-compat). @@ -544,6 +706,9 @@ mod tests { agents: &[], observer: &crate::observability::NoopObserver, token: crate::engine::CancellationToken::new(), + lane: None, + resume: None, + step: 0, }; let scope = expr_scope(&ctx); assert_eq!(scope["nodes"], json!({})); diff --git a/src/nodes/release.rs b/src/nodes/release.rs new file mode 100644 index 00000000..34a084b2 --- /dev/null +++ b/src/nodes/release.rs @@ -0,0 +1,238 @@ +//! When a collector has waited long enough: the release policies shared by the +//! `gate` node and (later) the scatter/gather barrier. +//! +//! Both constructs answer the same question — *given how many of the things I am +//! waiting for have settled, do I go now?* — so the decision lives here once +//! rather than being re-derived, and slightly differently, in each. The two +//! differ only in what they are counting: lanes of a fan-out, or tickets of +//! spawned work. +//! +//! Deciding *when* to release is deliberately separate from deciding *what to +//! emit*. This module is pure and synchronous: it reads counters and returns a +//! verdict, which is what makes the policies exhaustively testable against a +//! reference implementation without running a graph. + +use serde_json::Value; + +use crate::error::{EngineError, Result}; + +/// How many arrivals a collector waits for before it proceeds. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ReleasePolicy { + /// Wait for every one. The default, and the only policy that cannot emit a + /// partial result. + All, + /// Go as soon as the first one settles. + Any, + /// Go as soon as `n` have settled, whichever they are. + FirstN(usize), + /// Go once `n` have settled — the same rule as [`Self::FirstN`], named + /// separately because "a quorum of 3" and "the first 3" mean different + /// things to whoever reads the workflow, and a later change to one should + /// not silently change the other. + Quorum(usize), + /// Wait for every one, but settle for what arrived when the clock runs out. + TimeoutPartial, +} + +/// What a collector should do right now. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Release { + /// Enough has arrived: proceed with what is in hand. + Emit, + /// Not yet: wait and ask again. + Wait, + /// The wait budget is spent and the policy does not accept a partial + /// result. The caller routes this to its `timeout` port or `on_error`. + Timeout, +} + +impl ReleasePolicy { + /// Reads a policy from a node's `config.release` / `config.n`. + /// + /// # Errors + /// Returns [`EngineError::Capability`] for an unknown policy name, or for + /// `first_n` / `quorum` without a positive `n` — both are authoring + /// mistakes whose only sensible runtime default would be a lie about what + /// the workflow says. + pub(crate) fn from_config(config: &Value, node_id: &str) -> Result { + let count = || -> Result { + config + .get("n") + .and_then(Value::as_u64) + .filter(|n| *n > 0) + .and_then(|n| usize::try_from(n).ok()) + .ok_or_else(|| { + EngineError::Capability(format!( + "node {node_id:?}: `release` needs a positive integer `n`" + )) + }) + }; + match config.get("release").and_then(Value::as_str) { + None | Some("all") => Ok(Self::All), + Some("any") => Ok(Self::Any), + Some("first_n") => Ok(Self::FirstN(count()?)), + Some("quorum") => Ok(Self::Quorum(count()?)), + Some("timeout_partial") => Ok(Self::TimeoutPartial), + Some(other) => Err(EngineError::Capability(format!( + "node {node_id:?}: unknown `release` policy {other:?}; expected one of \ + all, any, first_n, quorum, timeout_partial" + ))), + } + } + + /// How many arrivals this policy needs, given `expected` in total. + /// + /// Clamped to `expected`: a `quorum` of 5 over 3 lanes would otherwise wait + /// for an arrival that can never come, turning an over-specified workflow + /// into a hang rather than a run that waits for everything. + fn threshold(self, expected: usize) -> usize { + match self { + Self::All | Self::TimeoutPartial => expected, + Self::Any => 1.min(expected), + Self::FirstN(n) | Self::Quorum(n) => n.min(expected), + } + } + + /// Decides whether a collector holding `arrived` of `expected` should go. + /// + /// `budget_spent` says the wait budget is exhausted — the caller owns what + /// that means (elapsed time, or a poll count), because a super-step engine + /// measures waiting in activations rather than only in seconds. + pub(crate) fn evaluate(self, arrived: usize, expected: usize, budget_spent: bool) -> Release { + if arrived >= self.threshold(expected) { + return Release::Emit; + } + if !budget_spent { + return Release::Wait; + } + // Out of budget. `timeout_partial` is the one policy that would rather + // proceed with less than it asked for than not proceed at all. + match self { + Self::TimeoutPartial => Release::Emit, + _ => Release::Timeout, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn all_waits_for_everything_then_times_out_rather_than_emitting_partially() { + let policy = ReleasePolicy::All; + assert_eq!(policy.evaluate(2, 3, false), Release::Wait); + assert_eq!(policy.evaluate(3, 3, false), Release::Emit); + assert_eq!( + policy.evaluate(2, 3, true), + Release::Timeout, + "`all` must never emit a partial result" + ); + } + + #[test] + fn any_goes_on_the_first_arrival() { + assert_eq!(ReleasePolicy::Any.evaluate(0, 5, false), Release::Wait); + assert_eq!(ReleasePolicy::Any.evaluate(1, 5, false), Release::Emit); + } + + #[test] + fn first_n_and_quorum_go_at_n() { + for policy in [ReleasePolicy::FirstN(3), ReleasePolicy::Quorum(3)] { + assert_eq!(policy.evaluate(2, 5, false), Release::Wait); + assert_eq!(policy.evaluate(3, 5, false), Release::Emit); + } + } + + /// A partial release must never hand downstream fewer results than the + /// policy promised — the property worth pinning, since `n` is the whole + /// contract of `first_n`/`quorum`. + #[test] + fn first_n_never_emits_with_fewer_than_n() { + let policy = ReleasePolicy::FirstN(3); + for arrived in 0..3 { + assert_ne!( + policy.evaluate(arrived, 10, true), + Release::Emit, + "emitted with only {arrived} of the promised 3" + ); + } + } + + /// An `n` larger than the number of things being waited for is an authoring + /// mistake that must not become a hang. + #[test] + fn a_threshold_above_the_expected_count_is_clamped() { + assert_eq!( + ReleasePolicy::Quorum(5).evaluate(3, 3, false), + Release::Emit + ); + assert_eq!(ReleasePolicy::Any.evaluate(0, 0, false), Release::Emit); + } + + #[test] + fn timeout_partial_settles_for_what_arrived_only_once_the_budget_is_spent() { + let policy = ReleasePolicy::TimeoutPartial; + assert_eq!(policy.evaluate(1, 3, false), Release::Wait); + assert_eq!(policy.evaluate(1, 3, true), Release::Emit); + } + + #[test] + fn config_parses_every_policy() { + let parse = |value: Value| ReleasePolicy::from_config(&value, "g"); + assert_eq!(parse(json!({})).unwrap(), ReleasePolicy::All); + assert_eq!( + parse(json!({ "release": "all" })).unwrap(), + ReleasePolicy::All + ); + assert_eq!( + parse(json!({ "release": "any" })).unwrap(), + ReleasePolicy::Any + ); + assert_eq!( + parse(json!({ "release": "first_n", "n": 2 })).unwrap(), + ReleasePolicy::FirstN(2) + ); + assert_eq!( + parse(json!({ "release": "quorum", "n": 4 })).unwrap(), + ReleasePolicy::Quorum(4) + ); + assert_eq!( + parse(json!({ "release": "timeout_partial" })).unwrap(), + ReleasePolicy::TimeoutPartial + ); + } + + /// Failing closed matters here: defaulting a missing `n` to 1 would turn a + /// declared quorum into `any` and release far too early. + #[test] + fn a_missing_or_zero_n_is_refused_rather_than_defaulted() { + for config in [ + json!({ "release": "first_n" }), + json!({ "release": "quorum", "n": 0 }), + json!({ "release": "quorum", "n": "three" }), + ] { + assert!( + ReleasePolicy::from_config(&config, "g").is_err(), + "config {config} should be refused rather than given a default `n`" + ); + } + } + + #[test] + fn an_unknown_policy_is_refused_and_the_message_lists_the_valid_ones() { + let err = ReleasePolicy::from_config(&json!({ "release": "eventually" }), "g") + .expect_err("unknown policy"); + let message = err.to_string(); + assert!( + message.contains("eventually"), + "names the bad value: {message}" + ); + assert!( + message.contains("quorum"), + "lists the valid ones: {message}" + ); + } +} diff --git a/src/validate.rs b/src/validate.rs index 53132134..113939cb 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -424,6 +424,7 @@ pub fn validate_all(graph: &WorkflowGraph) -> Vec { } validate_loops(graph, &mut errors); + validate_scatter_regions(graph, &mut errors); // Declared-input checks. These are author-time mistakes that would otherwise // surface as a confusing runtime `null`: a name that `=inputs.` cannot // address, two declarations racing for the same key, a default the input's @@ -803,6 +804,42 @@ fn validate_loops(graph: &WorkflowGraph, errors: &mut Vec) { }), } } + if let Some(state) = node.config.get("state") + && !state.is_object() + { + errors.push(ValidationError::InvalidNodeConfig { + node: node.id.clone(), + reason: "loop `state` must be an object with `init` and/or `update`".to_string(), + }); + } + if let Some(emit) = node.config.get("emit") + && !matches!(emit.as_str(), Some("items") | Some("state") | Some("both")) + { + errors.push(ValidationError::InvalidNodeConfig { + node: node.id.clone(), + reason: format!("loop `emit` must be \"items\", \"state\" or \"both\", got {emit}"), + }); + } + // A `success` exit that goes nowhere strands the converged case: the + // run would simply end there, which looks like the loop never finished. + if node + .config + .get("success_port") + .and_then(Value::as_bool) + .unwrap_or(false) + && !graph + .edges + .iter() + .any(|e| e.from_node == node.id && e.from_port == "success") + { + errors.push(ValidationError::InvalidNodeConfig { + node: node.id.clone(), + reason: "loop sets `success_port: true` but nothing is wired to its `success` \ + port, so a converged loop would strand the run; wire `success` or drop \ + the flag" + .to_string(), + }); + } if let Some(policy) = node.config.get("on_exceeded") && !matches!(policy.as_str(), Some("error") | Some("continue")) { @@ -835,9 +872,7 @@ fn validate_loops(graph: &WorkflowGraph, errors: &mut Vec) { return; } - // Every node that sits on some cycle, and the loop heads (back-edge - // targets) those cycles close on. - let heads: HashSet<&str> = loop_edges.iter().map(|(_, to)| to.as_str()).collect(); + // Every node that sits on some cycle. let on_a_cycle: HashSet<&str> = loop_edges .iter() .flat_map(|(from, to)| nodes_on_cycle(graph, to, from)) @@ -845,53 +880,88 @@ fn validate_loops(graph: &WorkflowGraph, errors: &mut Vec) { // A real fan-in `merge` inside the loop body deadlocks it. A single-input // merge is a passthrough and is not lowered as a waiting barrier. + // + // The rule is narrower than "any fan-in merge on a cycle", and both halves + // of it are load bearing. + // + // Why an all-on-the-cycle merge is fine: barrier arrivals refill. The + // arrival set is *removed* when the barrier fires (see + // `graph::compiled::routing::route_completed`), so it re-arms for the next + // pass. Every predecessor on the cycle runs again on every pass, so the set + // completes again on every pass and the merge fires once per iteration. + // + // Why an off-cycle predecessor still hangs: it runs once, on the seeding + // pass, and never activates again. From the second iteration the required + // set can never be completed and the loop stops dead at the merge. + // + // This lift also depended on a fix elsewhere, worth recording because the + // symptom pointed away from the cause. Loop-body arms are reachable only + // through the head's `body` port, so relief is registered for them; relief + // decides whether a branch was taken by walking forward through + // deterministic routing, and that walk used to stop at a fan-out (a command + // node has no static edge). It therefore concluded the arms were untaken + // and injected phantom arrivals, firing the merge *before* its arms ran — + // activation order `head, apex, join, arm_a, arm_b, …`, the join reading the + // previous pass's data. The walk now crosses unconditional fan-outs, which + // is what makes a diamond in a loop body correct rather than merely legal. for id in &on_a_cycle { let is_merge = graph .nodes .iter() .any(|n| n.id == *id && n.kind == NodeKind::Merge); - let forward_predecessors = graph + if !is_merge { + continue; + } + let forward_predecessors: Vec<&str> = graph .edges .iter() .filter(|edge| { edge.to_node == **id && !loop_edges.contains(&(edge.from_node.clone(), edge.to_node.clone())) }) - .count(); - if is_merge && forward_predecessors > 1 { + .map(|edge| edge.from_node.as_str()) + .collect(); + let waits_on_something_off_the_cycle = forward_predecessors + .iter() + .any(|pred| !on_a_cycle.contains(pred)); + if forward_predecessors.len() > 1 && waits_on_something_off_the_cycle { errors.push(ValidationError::IllegalCycle((*id).to_string())); } } - for head in &heads { - // A loop head that is also a fan-in cannot iterate: its forward - // predecessors are lowered as waiting edges, and that barrier is - // per-node, so it swallows the re-entry the back-edge delivers. The fix - // is to join *before* the head — a `merge` outside the cycle — which - // leaves the head with a single forward predecessor. - let forward_predecessors = graph - .edges - .iter() - .filter(|e| { - e.to_node == **head - && !loop_edges.contains(&(e.from_node.clone(), e.to_node.clone())) - }) - .count(); - if forward_predecessors > 1 { - errors.push(ValidationError::IllegalCycle((*head).to_string())); - } - } + // A loop head that is *also* a fan-in used to be refused here. + // + // It was refused because the barrier gate is keyed on the target node + // rather than on the edge, so the re-entry a back-edge delivered was tested + // against the head's forward predecessors, failed, and was dropped — the + // loop ran once and stopped. The gate now ignores arrivals from + // predecessors outside the barrier's required set (see + // `graph::compiled::routing::route_completed`), and a back-edge's source is + // never in that set, so the re-entry lands and the loop iterates. + // + // Joining before the head — a `merge` outside the cycle — is still the + // clearer way to write it, and is what the catalog recommends. It is no + // longer the only way that works. // An unbounded cycle. Without a `loop` node to count passes, the only thing // standing between this graph and a run that spins until the host's wall // clock kills it is the trigger's `recursion_limit`. Requiring one of the // two makes the bound an authoring decision rather than an accident. - let has_recursion_limit = graph - .trigger() - .and_then(|t| t.config.get("recursion_limit")) - .and_then(Value::as_u64) - .is_some_and(|n| n > 0); - if !has_recursion_limit { + // Either run-level bound counts. `max_node_visits` is enforced just as + // firmly as `recursion_limit` and gives the *better* failure — it names the + // node that ran away, where `recursion_limit` can only say the run did — so + // refusing a graph bounded solely by it was refusing a graph that was in + // fact bounded, and pushing authors toward the less informative knob. + let has_run_level_bound = graph.trigger().is_some_and(|trigger| { + ["recursion_limit", "max_node_visits"].iter().any(|key| { + trigger + .config + .get(*key) + .and_then(Value::as_u64) + .is_some_and(|n| n > 0) + }) + }); + if !has_run_level_bound { for (from, to) in &loop_edges { let bounded = nodes_on_cycle(graph, to, from).into_iter().any(|id| { graph @@ -906,6 +976,175 @@ fn validate_loops(graph: &WorkflowGraph, errors: &mut Vec) { } } +/// Checks the structural rules a `scatter`/`gather` region has to satisfy. +/// +/// A lane is created by routing, not by an edge, so most of what makes a region +/// work cannot be seen in the graph at run time — the engine just propagates a +/// lane envelope to every successor that is not a gather. These rules are what +/// keep that propagation *total*: if a lane can leak out of the region, or end +/// somewhere that is not a gather, the envelope is silently dropped and the +/// activation writes the node's top-level slot as though it were not in a lane +/// at all. That is a wrong answer rather than a failure, so it is refused here. +fn validate_scatter_regions(graph: &WorkflowGraph, errors: &mut Vec) { + let scatters: Vec<&str> = graph + .nodes + .iter() + .filter(|n| n.kind == NodeKind::Scatter) + .map(|n| n.id.as_str()) + .collect(); + let gathers: HashSet<&str> = graph + .nodes + .iter() + .filter(|n| n.kind == NodeKind::Gather) + .map(|n| n.id.as_str()) + .collect(); + + // A gather with no scatter waits on lanes nobody will ever open. + for gather in &gathers { + let reached_by_a_scatter = scatters + .iter() + .any(|scatter| path_exists(graph, scatter, gather)); + if !reached_by_a_scatter { + errors.push(ValidationError::InvalidNodeConfig { + node: (*gather).to_string(), + reason: "gather is not downstream of any `scatter`, so no lane can ever reach \ + it and the run would wait until its poll budget ran out" + .to_string(), + }); + } + } + + for scatter in &scatters { + // Every path out of a scatter has to end at a gather. A lane that runs + // off the end of the graph is work whose results nothing collects. + let members = region_members(graph, scatter, &gathers); + if members.is_empty() { + errors.push(ValidationError::InvalidNodeConfig { + node: (*scatter).to_string(), + reason: "scatter has no `gather` downstream; every lane it opens would run with \ + nothing to collect it. Wire the end of the lane body to a `gather`" + .to_string(), + }); + continue; + } + + for member in &members { + // A nested scatter needs composed lane ids and a gather that knows + // which level it closes. Refused rather than mis-collected. + if scatters.contains(member) { + errors.push(ValidationError::InvalidNodeConfig { + node: (*member).to_string(), + reason: format!( + "nested `scatter` inside the region opened by {scatter:?} is not \ + supported" + ), + }); + } + // A loop head inside a lane: re-entry detection keys on the node's + // top-level slot, which a lane activation deliberately never writes. + if graph + .nodes + .iter() + .any(|n| n.id == **member && n.kind == NodeKind::Loop) + { + errors.push(ValidationError::InvalidNodeConfig { + node: (*member).to_string(), + reason: format!( + "`loop` inside the lane body of {scatter:?} is not supported: loop \ + re-entry is tracked in the node's own slot, which a lane does not write" + ), + }); + } + // An approval gate inside a lane: the resume map is keyed by node + // id, so N lanes of one node would share a single approval. + if graph.nodes.iter().any(|n| { + n.id == **member + && n.config + .get("requires_approval") + .and_then(Value::as_bool) + .unwrap_or(false) + }) { + errors.push(ValidationError::InvalidNodeConfig { + node: (*member).to_string(), + reason: format!( + "`requires_approval` inside the lane body of {scatter:?} is not \ + supported: a resume is addressed by node id, so every lane would share \ + one approval" + ), + }); + } + // A lane that dead-ends: every node inside the region must have a + // path onward to a gather. One that does not is running in a lane + // whose results nothing collects — and because a lane activation + // deliberately never writes the node's top-level slot, its output + // is not merely uncollected, it is invisible. Wrong answer, not a + // failure, which is why this is refused rather than warned about. + let reaches_a_gather = gathers + .iter() + .any(|gather| path_exists(graph, member, gather)); + if !reaches_a_gather { + errors.push(ValidationError::InvalidNodeConfig { + node: (*member).to_string(), + reason: format!( + "node is inside the lane region opened by {scatter:?} but has no path \ + onward to a `gather`, so its lane output would be stranded; route it \ + through the gather" + ), + }); + } + } + } +} + +/// The nodes strictly between `scatter` and the gathers it reaches. +/// +/// Forward reachability from the scatter, stopping at any gather — the gather +/// itself is the boundary, not a member, because it is the one node a lane +/// reaches as a plain activation rather than as a lane. +fn region_members<'a>( + graph: &'a WorkflowGraph, + scatter: &str, + gathers: &HashSet<&str>, +) -> HashSet<&'a str> { + let mut members = HashSet::new(); + let mut reached_a_gather = false; + let mut stack: Vec<&str> = graph + .edges + .iter() + .filter(|e| e.from_node == scatter) + .map(|e| e.to_node.as_str()) + .collect(); + while let Some(node) = stack.pop() { + if gathers.contains(node) { + reached_a_gather = true; + continue; + } + let Some(id) = graph + .nodes + .iter() + .find(|n| n.id == node) + .map(|n| n.id.as_str()) + else { + continue; + }; + if !members.insert(id) { + continue; + } + stack.extend( + graph + .edges + .iter() + .filter(|e| e.from_node == node) + .map(|e| e.to_node.as_str()), + ); + } + if reached_a_gather { + members + } else { + HashSet::new() + } +} + fn path_exists(graph: &WorkflowGraph, start: &str, target: &str) -> bool { let mut seen = HashSet::new(); let mut stack = vec![start]; diff --git a/src/visualization.rs b/src/visualization.rs index e81e6266..4c2ca4e9 100644 --- a/src/visualization.rs +++ b/src/visualization.rs @@ -331,6 +331,10 @@ fn kind_name(kind: &NodeKind) -> &'static str { NodeKind::SubWorkflow => "sub_workflow", NodeKind::Memory => "memory", NodeKind::Dedup => "dedup", + NodeKind::Spawn => "spawn", + NodeKind::Scatter => "scatter", + NodeKind::Gather => "gather", + NodeKind::Gate => "gate", } } diff --git a/tests/async_gates_e2e.rs b/tests/async_gates_e2e.rs new file mode 100644 index 00000000..efe4002a --- /dev/null +++ b/tests/async_gates_e2e.rs @@ -0,0 +1,383 @@ +#![cfg(feature = "mock")] +//! End-to-end tests for the async pair: `spawn` starts work without blocking, +//! `gate` collects it on a release policy. +//! +//! The point of these nodes is *overlap*, and overlap does not show up in a +//! final state — a graph that ran everything sequentially computes the same +//! answer. So the tests here measure timing and invocation counts, not just +//! output, and each one says which of those it is actually pinning. +//! +//! Every run is wrapped in a timeout: a gate that never releases hangs rather +//! than fails, and a hung test takes the suite with it. + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; + +use serde_json::{Value, json}; + +use tinyflows::caps::mock::mock_capabilities; +use tinyflows::caps::{TaskRunner, TaskSpec, TaskState}; +use tinyflows::compiler::compile; +use tinyflows::engine::run; +use tinyflows::model::{Edge, Node, NodeKind, WorkflowGraph}; + +/// How long any run here may take before it is called a hang. +const GUARD: Duration = Duration::from_secs(20); + +fn node(id: &str, kind: NodeKind, config: Value) -> Node { + Node { + id: id.to_string(), + kind, + type_version: 1, + name: id.to_string(), + config, + ports: vec![], + position: None, + } +} + +fn edge(from: &str, to: &str) -> Edge { + Edge { + from_node: from.to_string(), + from_port: "main".to_string(), + to_node: to.to_string(), + to_port: "main".to_string(), + } +} + +/// A runner whose tasks settle only after being polled `settle_after` times. +/// +/// Real background work does not finish the instant a gate looks at it. Forcing +/// several polls is what actually exercises the wait loop — with a runner that +/// settles immediately, a broken gate that released on its first activation +/// would pass every test here. +struct SlowRunner { + settle_after: usize, + polls: Mutexed, + started: AtomicUsize, +} + +type Mutexed = std::sync::Mutex>; + +impl SlowRunner { + fn new(settle_after: usize) -> Arc { + Arc::new(Self { + settle_after, + polls: std::sync::Mutex::new(std::collections::HashMap::new()), + started: AtomicUsize::new(0), + }) + } +} + +#[async_trait::async_trait] +impl TaskRunner for SlowRunner { + async fn start(&self, spec: TaskSpec) -> tinyflows::error::Result { + let index = self.started.fetch_add(1, Ordering::SeqCst); + let _ = spec; + Ok(format!("t{index}")) + } + + async fn poll(&self, ticket: &str) -> tinyflows::error::Result { + let mut polls = self.polls.lock().expect("poll table poisoned"); + let count = polls.entry(ticket.to_string()).or_insert(0); + *count += 1; + if *count >= self.settle_after { + Ok(TaskState::Done(json!({ "ticket": ticket }))) + } else { + Ok(TaskState::Running) + } + } + + async fn cancel(&self, _ticket: &str) -> tinyflows::error::Result<()> { + Ok(()) + } +} + +/// `trigger -> spawn -> gate`, with the gate's config under test. +fn spawn_gate_graph(gate_config: Value) -> WorkflowGraph { + let mut config = gate_config; + config["from"] = json!(["kick"]); + WorkflowGraph { + name: "spawn_gate".to_string(), + nodes: vec![ + node( + "t", + NodeKind::Trigger, + // Polls cost super-steps, so the run needs headroom for them. + json!({ "recursion_limit": 400, "max_node_visits": 300 }), + ), + node( + "kick", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "work.run" }), + ), + node("collect", NodeKind::Gate, config), + ], + edges: vec![edge("t", "kick"), edge("kick", "collect")], + ..Default::default() + } +} + +async fn run_guarded( + graph: &WorkflowGraph, + caps: &tinyflows::caps::Capabilities, +) -> tinyflows::error::Result { + let compiled = compile(graph).expect("compile"); + match tokio::time::timeout(GUARD, run(&compiled, json!({}), caps)).await { + Err(_) => panic!("run hung past {GUARD:?} — a gate never released"), + Ok(inner) => inner, + } +} + +/// A gate polls until its work settles, then emits the result. +/// +/// The runner deliberately reports `Running` for the first few polls, so this +/// pins the wait loop rather than a gate that happens to release immediately. +#[tokio::test] +async fn a_gate_polls_until_the_spawned_work_settles() { + let runner = SlowRunner::new(3); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner.clone()); + + let outcome = run_guarded(&spawn_gate_graph(json!({ "poll_interval_ms": 1 })), &caps) + .await + .expect("the gate should release once its task settles"); + + let items = outcome.output["nodes"]["collect"]["items"] + .as_array() + .expect("the gate emitted items"); + assert_eq!(items.len(), 1, "one spawned task, one result"); + assert_eq!(items[0]["json"]["ticket"], "t0"); + + let polls = outcome.output["nodes"]["collect"]["polls"] + .as_u64() + .expect("the gate records its poll count"); + assert!( + polls >= 3, + "the gate should have polled at least until the task settled, got {polls}" + ); +} + +/// The spawn does not block: its branch continues before the work settles. +/// +/// This is the property the whole node pair exists for, and it is invisible in +/// the final state — so it is measured by *when* the sibling ran, using the +/// runner's poll count as the clock. +#[tokio::test] +async fn a_spawn_does_not_block_its_branch() { + let runner = SlowRunner::new(5); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner.clone()); + + // `kick` spawns; `sibling` runs downstream of it and must not have waited + // for the spawned work. If spawn blocked, the task would already be settled + // (5 polls consumed) by the time `sibling` ran. + let graph = WorkflowGraph { + name: "spawn_is_non_blocking".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, json!({ "recursion_limit": 400 })), + node( + "kick", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "work.run" }), + ), + node("sibling", NodeKind::OutputParser, Value::Null), + ], + edges: vec![edge("t", "kick"), edge("kick", "sibling")], + ..Default::default() + }; + + let outcome = run_guarded(&graph, &caps).await.expect("run"); + assert!( + !outcome.output["nodes"]["sibling"]["items"].is_null(), + "the branch continued past the spawn" + ); + let polls = runner.polls.lock().expect("poll table poisoned"); + assert!( + polls.is_empty(), + "nothing polled the task, because nothing waited for it — spawn returned \ + a ticket rather than a result" + ); +} + +/// `release: "quorum"` proceeds once `n` results are in and leaves the +/// stragglers running, rather than waiting for every task. +#[tokio::test] +async fn a_quorum_gate_releases_before_every_task_settles() { + // Three spawns; each settles after a different number of polls, so they + // genuinely finish at different times. + struct Staggered { + polls: Mutexed, + } + + #[async_trait::async_trait] + impl TaskRunner for Staggered { + async fn start(&self, _spec: TaskSpec) -> tinyflows::error::Result { + let mut polls = self.polls.lock().expect("poisoned"); + let ticket = format!("t{}", polls.len()); + polls.insert(ticket.clone(), 0); + Ok(ticket) + } + async fn poll(&self, ticket: &str) -> tinyflows::error::Result { + let mut polls = self.polls.lock().expect("poisoned"); + let count = polls.entry(ticket.to_string()).or_insert(0); + *count += 1; + // `t0` settles at once; `t1` and `t2` take much longer. + let needed = match ticket { + "t0" => 1, + "t1" => 2, + _ => 50, + }; + if *count >= needed { + Ok(TaskState::Done(json!({ "ticket": ticket }))) + } else { + Ok(TaskState::Running) + } + } + async fn cancel(&self, _ticket: &str) -> tinyflows::error::Result<()> { + Ok(()) + } + } + + let runner = Arc::new(Staggered { + polls: std::sync::Mutex::new(std::collections::HashMap::new()), + }); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner.clone()); + + let graph = WorkflowGraph { + name: "quorum".to_string(), + nodes: vec![ + node( + "t", + NodeKind::Trigger, + json!({ "recursion_limit": 400, "max_node_visits": 300 }), + ), + node( + "a", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "a" }), + ), + node( + "b", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "b" }), + ), + node( + "c", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "c" }), + ), + node( + "collect", + NodeKind::Gate, + json!({ + "from": ["a", "b", "c"], + "release": "quorum", + "n": 2, + "poll_interval_ms": 1 + }), + ), + ], + edges: vec![ + edge("t", "a"), + edge("t", "b"), + edge("t", "c"), + edge("a", "collect"), + edge("b", "collect"), + edge("c", "collect"), + ], + ..Default::default() + }; + + let outcome = run_guarded(&graph, &caps).await.expect("run"); + let items = outcome.output["nodes"]["collect"]["items"] + .as_array() + .expect("items"); + assert_eq!( + items.len(), + 2, + "a quorum of 2 emits exactly the two that arrived, not all three" + ); + // The straggler is left running rather than waited for — which is the whole + // point of a quorum, and the reason the run finishes quickly. + let polls = runner.polls.lock().expect("poisoned"); + assert!( + polls.get("t2").copied().unwrap_or(0) < 50, + "the gate should not have waited for the straggler to settle" + ); +} + +/// Without a `TaskRunner` the graph still runs: `spawn` performs its work inline +/// and the gate collects an already-settled ticket. +/// +/// Losing the overlap is acceptable; losing the answer is not. +#[tokio::test] +async fn spawn_and_gate_still_work_with_no_task_runner_injected() { + let mut caps = mock_capabilities(); + caps.tasks = None; + + let outcome = run_guarded(&spawn_gate_graph(json!({})), &caps) + .await + .expect("the graph must still run without a TaskRunner"); + + let items = outcome.output["nodes"]["collect"]["items"] + .as_array() + .expect("the gate still emits a result"); + assert_eq!(items.len(), 1, "the inline result reaches the gate"); + assert_eq!( + outcome.output["nodes"]["collect"]["polls"], 1, + "an inline result is already in hand, so the gate releases on its first \ + activation without polling" + ); +} + +/// A gate that never gets its results fails naming its own budget, rather than +/// spinning until the run-level backstop reports a generic runaway. +#[tokio::test] +async fn a_gate_that_never_releases_fails_naming_its_poll_budget() { + let runner = SlowRunner::new(usize::MAX); // never settles + let mut caps = mock_capabilities(); + caps.tasks = Some(runner); + + let err = run_guarded( + &spawn_gate_graph(json!({ "poll_interval_ms": 1, "max_polls": 3 })), + &caps, + ) + .await + .expect_err("a gate whose work never lands must fail, not hang"); + + let message = err.to_string(); + assert!( + message.contains("collect") && message.contains("polls"), + "the failure should name the gate and its poll budget, got: {message}" + ); +} + +/// `on_timeout: "partial"` settles for what arrived instead of failing. +#[tokio::test] +async fn a_timed_out_gate_can_emit_what_arrived() { + let runner = SlowRunner::new(usize::MAX); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner); + + let outcome = run_guarded( + &spawn_gate_graph(json!({ + "poll_interval_ms": 1, + "max_polls": 2, + "on_timeout": "partial" + })), + &caps, + ) + .await + .expect("a partial timeout is not a failure"); + + let items = outcome.output["nodes"]["collect"]["items"] + .as_array() + .expect("items"); + assert!( + items.is_empty(), + "nothing settled, so the partial result is empty — but the run completed" + ); +} diff --git a/tests/eng5_barrier_relief_e2e.rs b/tests/eng5_barrier_relief_e2e.rs index 97c4b8fa..60152c76 100644 --- a/tests/eng5_barrier_relief_e2e.rs +++ b/tests/eng5_barrier_relief_e2e.rs @@ -381,3 +381,64 @@ async fn pure_unconditional_fan_in_regression() { "m must include both a's and b's items" ); } + +/// A conditional predecessor reached *through a fan-out* must not be phantomed. +/// +/// This is the non-loop form of a bug found while enabling diamonds inside loop +/// bodies, and it is a plain-graph data-loss bug in its own right. Relief +/// decides "was the branch leading to this predecessor taken?" by walking +/// forward through deterministic routing. A fan-out node has no static edge — +/// its successors come from the `Command` it emits — so the walk used to stop +/// there and report unreachable. Reporting unreachable is what *fires* relief, +/// so the barrier cleared before `a1`/`a2` had run and `m` merged only `c`. +/// +/// `start` fans out to `cond` and `c`. `cond --true--> apex`, and `apex` itself +/// fans out to `a1` and `a2`, both feeding `m` alongside `c`. With `flag: true` +/// every one of `m`'s three predecessors really runs, so `m` must see all three +/// tags. +#[tokio::test] +async fn a_conditional_predecessor_behind_a_fan_out_is_not_phantomed() { + let graph = WorkflowGraph { + name: "relief_through_fan_out".to_string(), + nodes: vec![ + trigger("start"), + node("cond", NodeKind::Condition, json!({ "field": "flag" })), + node("apex", NodeKind::OutputParser, Value::Null), + tagged("a1", "a1"), + tagged("a2", "a2"), + tagged("b", "b"), + tagged("c", "c"), + node("m", NodeKind::Merge, Value::Null), + ], + edges: vec![ + edge("start", "main", "cond"), + edge("start", "main", "c"), + edge("cond", "true", "apex"), + edge("cond", "false", "b"), + // `apex` fans out: both leave on the same port. + edge("apex", "main", "a1"), + edge("apex", "main", "a2"), + edge("a1", "main", "m"), + edge("a2", "main", "m"), + edge("c", "main", "m"), + ], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + + let outcome = tokio::time::timeout( + Duration::from_secs(5), + run(&compiled, json!({ "flag": true }), &mock_capabilities()), + ) + .await + .expect("must not deadlock") + .expect("run"); + + assert_eq!( + merge_tags(&outcome.output, "m"), + HashSet::from(["a1".to_string(), "a2".to_string(), "c".to_string()]), + "every predecessor really ran, so none may be phantomed away — a result \ + of just {{c}} means relief cleared the barrier before the fan-out's \ + branches committed their data" + ); +} diff --git a/tests/fuzz_async.rs b/tests/fuzz_async.rs new file mode 100644 index 00000000..cd1c9e0b --- /dev/null +++ b/tests/fuzz_async.rs @@ -0,0 +1,222 @@ +#![cfg(feature = "mock")] +//! Property tests for the async pair (`spawn`/`gate`) and the release policies. +//! +//! These run generated graphs and assert the invariants a caller can rely on: +//! results ordered independently of completion, and a release that never +//! under-delivers on the count its policy advertises. +//! +//! The release *rule* itself is pure arithmetic and is unit-tested next to the +//! implementation in `src/nodes/release.rs`, where it is reachable — it is +//! `pub(crate)`, so a property test out here could only check a reference +//! against itself. +//! +//! Gated behind the `mock` feature alongside the rest of the e2e suite. + +mod support; + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use proptest::prelude::*; +use serde_json::{Value, json}; + +use support::graphgen::{Shape, arb_spawned_shape, graph_of}; +use tinyflows::caps::mock::mock_capabilities; +use tinyflows::caps::{TaskRunner, TaskSpec, TaskState}; +use tinyflows::compiler::compile; +use tinyflows::engine::run; + +const GUARD: Duration = Duration::from_secs(20); + +/// A runner whose tasks settle after a per-ticket number of polls, so a gate +/// sees them finish in an order that has nothing to do with the order they were +/// started in. +/// +/// This is the whole point: a gate that happened to emit in completion order +/// would look correct against a runner that settles everything at once. +struct Staggered { + /// Polls seen per ticket. + polls: Mutex>, + /// How many polls each ticket needs before it settles, by start order. + settle_at: Vec, + started: Mutex, + cancelled: Mutex>, +} + +impl Staggered { + fn new(settle_at: Vec) -> Arc { + Arc::new(Self { + polls: Mutex::new(HashMap::new()), + settle_at, + started: Mutex::new(0), + cancelled: Mutex::new(Vec::new()), + }) + } +} + +#[async_trait::async_trait] +impl TaskRunner for Staggered { + async fn start(&self, _spec: TaskSpec) -> tinyflows::error::Result { + let mut started = self.started.lock().expect("poisoned"); + let ticket = format!("t{started}"); + *started += 1; + Ok(ticket) + } + + async fn poll(&self, ticket: &str) -> tinyflows::error::Result { + let index: usize = ticket.trim_start_matches('t').parse().unwrap_or(0); + let needed = self.settle_at.get(index).copied().unwrap_or(1).max(1); + let mut polls = self.polls.lock().expect("poisoned"); + let count = polls.entry(ticket.to_string()).or_insert(0); + *count += 1; + if *count >= needed { + Ok(TaskState::Done(json!({ "ticket": ticket, "index": index }))) + } else { + Ok(TaskState::Running) + } + } + + async fn cancel(&self, ticket: &str) -> tinyflows::error::Result<()> { + self.cancelled + .lock() + .expect("poisoned") + .push(ticket.to_string()); + Ok(()) + } +} + +fn runtime() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build runtime") +} + +proptest! { + // Each case runs a real graph, so fewer of them. + #![proptest_config(ProptestConfig { cases: 64, ..ProptestConfig::default() })] + + /// **Ordering.** A gate emits results in ticket order and with correct + /// `paired_item`s, whatever order the tasks actually finished in. + /// + /// The staggered runner deliberately settles later tickets first, so a gate + /// that emitted in completion order fails here and only here — the final + /// item *set* is identical either way. + #[test] + fn a_gate_emits_in_ticket_order_whatever_the_completion_order( + shape in arb_spawned_shape(), + settle in prop::collection::vec(1usize..6, 5), + ) { + let Shape::Spawned { tasks, release, .. } = &shape else { + return Ok(()); + }; + // Only `all` guarantees every ticket is collected, which is what makes + // a full ordering assertion meaningful. + if *release != "all" { + return Ok(()); + } + let tasks = *tasks; + let graph = graph_of(&shape); + let Ok(compiled) = compile(&graph) else { + return Ok(()); + }; + + // Reverse the settle order so later tickets finish first. + let mut settle: Vec = settle.into_iter().take(tasks).collect(); + settle.reverse(); + let runner = Staggered::new(settle); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner.clone()); + + let output = runtime().block_on(async { + tokio::time::timeout(GUARD, run(&compiled, json!({}), &caps)) + .await + .expect("run hung — a gate never released") + .expect("run") + .output + }); + + // Find the gate's slot: the one node whose items carry ticket indices. + let gate_items = output["nodes"] + .as_object() + .expect("nodes") + .values() + .filter_map(|slot| slot.get("items").and_then(Value::as_array)) + .find(|items| { + items.len() == tasks + && items.iter().all(|i| i["json"].get("index").is_some()) + }); + let Some(items) = gate_items else { + return Ok(()); + }; + + let indices: Vec = items + .iter() + .filter_map(|i| i["json"]["index"].as_u64()) + .collect(); + let mut sorted = indices.clone(); + sorted.sort_unstable(); + prop_assert_eq!( + &indices, &sorted, + "results must be ordered by ticket index, not by completion" + ); + let paired: Vec = items + .iter() + .filter_map(|i| i["paired_item"].as_u64()) + .collect(); + prop_assert_eq!( + paired, indices, + "each result's paired_item must be the index of its own ticket" + ); + } + + /// **No under-delivery.** A gate never emits fewer results than its policy + /// promised, whatever the release policy and however the tasks settle. + #[test] + fn a_gate_never_emits_fewer_results_than_its_policy_promised( + shape in arb_spawned_shape(), + settle in prop::collection::vec(1usize..4, 5), + ) { + let Shape::Spawned { tasks, release, n } = &shape else { + return Ok(()); + }; + let (tasks, release, n) = (*tasks, *release, *n); + let graph = graph_of(&shape); + let Ok(compiled) = compile(&graph) else { + return Ok(()); + }; + + let runner = Staggered::new(settle.into_iter().take(tasks).collect()); + let mut caps = mock_capabilities(); + caps.tasks = Some(runner); + + let output = runtime().block_on(async { + tokio::time::timeout(GUARD, run(&compiled, json!({}), &caps)) + .await + .expect("run hung") + .expect("run") + .output + }); + + let emitted = output["nodes"] + .as_object() + .expect("nodes") + .values() + .filter_map(|slot| slot.get("arrived").and_then(Value::as_u64)) + .max() + .unwrap_or(0) as usize; + + let promised = match release { + "any" => 1.min(tasks), + "first_n" | "quorum" => n.clamp(1, tasks.max(1)).min(tasks), + // `timeout_partial` explicitly permits less; `all` is checked by + // the ordering property above. + _ => 0, + }; + prop_assert!( + emitted >= promised, + "policy {release} promised at least {promised} results, gate released with {emitted}" + ); + } +} diff --git a/tests/fuzz_graph.rs b/tests/fuzz_graph.rs new file mode 100644 index 00000000..a818f294 --- /dev/null +++ b/tests/fuzz_graph.rs @@ -0,0 +1,193 @@ +#![cfg(feature = "mock")] +//! Property tests over generated workflow graphs. +//! +//! These assert the invariants that must hold for *every* graph the validator +//! accepts, rather than for the handful of shapes someone thought to write down +//! by hand. Lane clobbering, barrier skew and runaway poll budgets are all +//! bugs that survive hand-written examples — they need a graph nobody chose. +//! +//! Each test wraps its run in a timeout: the failure mode being hunted here is +//! a *hang*, and a hung test takes the whole suite with it rather than naming +//! itself. +//! +//! Gated behind the `mock` feature alongside the rest of the e2e suite. + +mod support; + +use std::time::Duration; + +use proptest::prelude::*; +use serde_json::{Value, json}; + +use support::graphgen::{arb_shape, arb_workflow_graph, graph_of}; +use tinyflows::caps::mock::mock_capabilities; +use tinyflows::compiler::compile; +use tinyflows::engine::run; +use tinyflows::error::EngineError; +use tinyflows::model::WorkflowGraph; + +/// How long a single generated run may take before it is called a hang. +/// +/// Generously above what any generated graph needs, so this only ever fires on +/// a real non-termination rather than on a slow machine. +const GUARD: Duration = Duration::from_secs(20); + +/// Runs a graph to completion on a private tokio runtime. +/// +/// Property tests are synchronous, so each case builds its own runtime rather +/// than borrowing an ambient one — which also guarantees no state leaks between +/// cases. +fn run_graph(graph: &WorkflowGraph) -> Result { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build runtime"); + runtime.block_on(async { + let caps = mock_capabilities(); + let compiled = compile(graph).map_err(|e| format!("compile: {e}"))?; + match tokio::time::timeout(GUARD, run(&compiled, json!({}), &caps)).await { + Err(_) => Err("HUNG: run did not terminate".to_string()), + Ok(Ok(outcome)) => Ok(outcome.output), + Ok(Err(e)) => Err(bounded_failure(&e)?), + } + }) +} + +/// Classifies an engine error as an acceptable *bound* or a real failure. +/// +/// A generated graph is allowed to run out of budget — the budgets exist — but +/// only if the failure names which bound was hit. An unnamed failure, or any +/// other error, is a bug: these graphs are built from pure control-flow nodes +/// and consult no capability, so there is nothing legitimate to fail on. +fn bounded_failure(error: &EngineError) -> Result { + match error { + EngineError::LoopLimit { node, limit } => Ok(format!("bounded: loop {node} hit {limit}")), + EngineError::Capability(message) + if message.contains("recursion") || message.contains("visit") => + { + Ok(format!("bounded: {message}")) + } + other => Err(format!("unexpected engine error: {other:?}")), + } +} + +/// Guards against the whole suite going quietly vacuous. +/// +/// Every property below skips a graph the validator refuses, which is correct +/// — but it means a generator that drifted into producing only invalid graphs +/// would leave every test passing while exercising nothing. This pins the +/// generator's yield so that drift fails loudly here instead of hiding there. +/// +/// The graphs that *are* refused are refused for `illegal cycle`: a `merge` +/// landing on a cycle when a branch or fan-out is nested inside a loop body. +/// That refusal is the one the lane-scoped barrier work is expected to lift, so +/// this ratio is also the before/after measure for it — when that lands, the +/// floor here should rise rather than the test being deleted. +#[test] +fn the_generator_mostly_produces_runnable_graphs() { + use proptest::strategy::{Strategy, ValueTree}; + use proptest::test_runner::TestRunner; + + const SAMPLES: usize = 200; + const FLOOR: usize = 120; // ~60%; observed yield at time of writing is ~85% + + let mut runner = TestRunner::deterministic(); + let mut runnable = 0; + for _ in 0..SAMPLES { + let shape = arb_shape(3) + .new_tree(&mut runner) + .expect("generate a shape") + .current(); + if compile(&graph_of(&shape)).is_ok() { + runnable += 1; + } + } + assert!( + runnable >= FLOOR, + "only {runnable}/{SAMPLES} generated graphs were runnable, below the {FLOOR} floor — \ + the generator has drifted and the property tests above are running on very little" + ); +} + +proptest! { + #![proptest_config(ProptestConfig { cases: 128, ..ProptestConfig::default() })] + + /// **Validate ⇒ terminate.** Any graph the validator accepts must either + /// finish or fail naming the bound it hit. It must never hang, never panic, + /// and never deadlock on a barrier. + /// + /// This is the single most valuable property here: a deadlock is the + /// characteristic failure of a super-step engine with barriers, and it is + /// invisible to a test suite that only runs shapes known to work. + #[test] + fn an_accepted_graph_always_settles(graph in arb_workflow_graph()) { + if compile(&graph).is_err() { + // Refused before running: fine, and not what this property is about. + return Ok(()); + } + if let Err(failure) = run_graph(&graph) { + prop_assert!( + failure.starts_with("bounded:"), + "graph neither completed nor hit a named bound: {failure}\ngraph: {}", + serde_json::to_string(&graph).unwrap_or_default() + ); + } + } + + /// **Determinism.** The same graph run twice produces byte-identical final + /// state. + /// + /// The engine folds concurrent branch updates through a reducer in + /// active-set order rather than completion order, so this must hold even + /// though branches genuinely race. It is the detector for any future + /// fan-out that lets two activations write the same state slot — the + /// clobber shows up as two runs disagreeing. + #[test] + fn a_graph_runs_the_same_way_twice(graph in arb_workflow_graph()) { + if compile(&graph).is_err() { + return Ok(()); + } + let first = run_graph(&graph); + let second = run_graph(&graph); + match (first, second) { + (Ok(a), Ok(b)) => prop_assert_eq!( + a, b, + "two runs of one graph disagreed\ngraph: {}", + serde_json::to_string(&graph).unwrap_or_default() + ), + (Err(a), Err(b)) => prop_assert_eq!(a, b, "two runs failed differently"), + (a, b) => prop_assert!( + false, + "one run succeeded and the other did not: {a:?} vs {b:?}" + ), + } + } + + /// Every node the graph declares gets a slot in the final state, except + /// those on a genuinely untaken conditional branch. + /// + /// Catches a whole class of routing bug where a node is silently skipped — + /// which otherwise surfaces only as a downstream expression resolving to + /// null, far from the cause. + #[test] + fn a_run_records_a_slot_for_every_node_it_ran(shape in arb_shape(2)) { + let graph = graph_of(&shape); + if compile(&graph).is_err() { + return Ok(()); + } + let Ok(output) = run_graph(&graph) else { + return Ok(()); // bounded failures are covered by the property above + }; + let slots = output["nodes"].as_object().cloned().unwrap_or_default(); + prop_assert!( + slots.contains_key("trigger"), + "the trigger always runs, so it must always have a slot" + ); + for (id, slot) in &slots { + prop_assert!( + slot.get("items").is_some(), + "node {id} recorded a slot with no items array: {slot}" + ); + } + } +} diff --git a/tests/fuzz_resume.rs b/tests/fuzz_resume.rs new file mode 100644 index 00000000..32339ba8 --- /dev/null +++ b/tests/fuzz_resume.rs @@ -0,0 +1,269 @@ +#![cfg(feature = "mock")] +//! Property tests for the checkpoint / resume path. +//! +//! # The property that matters +//! +//! Pausing a run must not change what the run computes. Concretely: a run that +//! is pre-approved and completes in one go, and a run that suspends at its gate +//! and is then resumed, must reach the **same final state**. Everything a +//! resume can get wrong — losing a branch's work, re-running a branch that had +//! already finished, replaying a fold twice — shows up as those two states +//! disagreeing. +//! +//! That equivalence is asserted here against generated graphs rather than one +//! hand-written flow, because the interesting resumes are the ones where the +//! gate sits *beside* concurrent work, and which branches are in flight at the +//! moment of suspension is precisely what a generator varies and a human does +//! not. +//! +//! # What "the same" has to mean here, and why +//! +//! The two paths cannot be compared byte-for-byte, and the reason is worth +//! stating so nobody later "fixes" this by weakening it further. Approval has +//! to reach the run through *some* channel, and the two channels are not +//! interchangeable: pre-approval rides in on the trigger payload, which every +//! node's items then carry, while a resume delivers approval out of band. So +//! the pre-approved run's items legitimately contain an `approvals` key the +//! resumed run's do not, and its slots carry different `_activation_step` +//! stamps because suspending genuinely costs a super-step. +//! +//! Comparison is therefore over what the *workflow* computed rather than how +//! the engine got there: which nodes ran, and how many items each produced. +//! That still catches the failures that matter — a branch whose work is lost +//! across the pause, a branch re-run so its output is duplicated, a node +//! skipped entirely on the resumed path. +//! +//! It does **not** catch a side-effecting node re-running idempotently, since +//! these graphs are built from pure passthroughs where a re-run looks identical +//! to not re-running. Detecting that needs an invocation counter rather than a +//! state diff, and belongs with the work that fixes it. +//! +//! Gated behind the `mock` feature alongside the rest of the e2e suite. + +mod support; + +use std::sync::Arc; +use std::time::Duration; + +use proptest::prelude::*; +use serde_json::{Value, json}; + +use support::graphgen::{Shape, arb_gated_shape, graph_of}; +use tinyflows::caps::mock::mock_capabilities; +use tinyflows::compiler::compile; +use tinyflows::engine::{ + InMemoryCheckpointer, resume_with_checkpointer, run, run_with_checkpointer, +}; + +/// How long any single run may take before it is called a hang. +const GUARD: Duration = Duration::from_secs(20); + +/// Builds a private current-thread runtime for one property case. +fn runtime() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build runtime") +} + +/// Reduces a final run state to "which nodes ran, and how many items each +/// emitted", sorted for a stable comparison. +/// +/// This is the comparable core of a run: it survives the two approval channels +/// differing (see the module docs) while still changing the moment a node is +/// skipped, duplicated, or loses its output across a pause. +fn work_done(state: &Value) -> Vec<(String, usize)> { + let mut work: Vec<(String, usize)> = state["nodes"] + .as_object() + .map(|slots| { + slots + .iter() + .map(|(id, slot)| { + let count = slot["items"].as_array().map_or(0, Vec::len); + (id.clone(), count) + }) + .collect() + }) + .unwrap_or_default(); + work.sort(); + work +} + +/// Runs `shape` two ways and returns `(pre_approved_state, resumed_state)`. +/// +/// Returns `None` when the graph is refused by the validator, which is not what +/// these properties are about. +fn run_both_ways(shape: &Shape) -> Option<(Value, Value)> { + let graph = graph_of(shape); + let compiled = compile(&graph).ok()?; + let gates = shape.gate_ids(); + assert!( + !gates.is_empty(), + "a gated shape must contain at least one gate, or this test proves nothing" + ); + + runtime().block_on(async { + let caps = mock_capabilities(); + + // Path A: every gate approved up front, so the run never suspends. + let straight = tokio::time::timeout( + GUARD, + run(&compiled, json!({ "approvals": gates.clone() }), &caps), + ) + .await + .expect("pre-approved run hung") + .expect("pre-approved run should complete"); + + // Path B: no approvals, so the run suspends at its gate; then resume, + // approving every gate, until it settles. A graph may hold more than + // one gate, and a resume clears only the gates it names, so this loops + // rather than assuming a single round trip. + let checkpointer = Arc::new(InMemoryCheckpointer::default()); + let thread_id = "fuzz-resume"; + let mut outcome = tokio::time::timeout( + GUARD, + run_with_checkpointer(&compiled, json!({}), &caps, checkpointer.clone(), thread_id), + ) + .await + .expect("suspending run hung") + .expect("suspending run should pause rather than fail"); + + let mut rounds = 0; + while !outcome.pending_approvals.is_empty() { + rounds += 1; + assert!( + rounds <= gates.len() + 2, + "resume did not converge after {rounds} rounds; still pending: {:?}", + outcome.pending_approvals + ); + let pending = outcome.pending_approvals.clone(); + outcome = tokio::time::timeout( + GUARD, + resume_with_checkpointer( + &compiled, + &caps, + checkpointer.clone(), + thread_id, + pending, + ), + ) + .await + .expect("resume hung") + .expect("resume should succeed"); + } + + Some((straight.output, outcome.output)) + }) +} + +/// Guards the properties below against passing vacuously. +/// +/// Each of them returns early when a generated graph is refused by the +/// validator, so a generator that drifted into producing only invalid gated +/// graphs would leave them green while never once suspending a run. This pins +/// the yield so that drift fails here instead. +#[test] +fn gated_graphs_are_mostly_runnable_and_actually_suspend() { + use proptest::strategy::{Strategy, ValueTree}; + use proptest::test_runner::TestRunner; + + const SAMPLES: usize = 60; + const FLOOR: usize = 36; // 60% + + let mut runner = TestRunner::deterministic(); + let mut suspended = 0; + for _ in 0..SAMPLES { + let shape = arb_gated_shape(2) + .new_tree(&mut runner) + .expect("generate a shape") + .current(); + let graph = graph_of(&shape); + let Ok(compiled) = compile(&graph) else { + continue; + }; + let outcome = runtime().block_on(async { + let caps = mock_capabilities(); + tokio::time::timeout(GUARD, run(&compiled, json!({}), &caps)) + .await + .expect("run hung") + .expect("an unapproved run should pause, not fail") + }); + if !outcome.pending_approvals.is_empty() { + suspended += 1; + } + } + assert!( + suspended >= FLOOR, + "only {suspended}/{SAMPLES} generated gated graphs actually suspended, below the {FLOOR} \ + floor — the resume properties are running on very little" + ); +} + +proptest! { + // Deliberately fewer cases than the other fuzz files: each case runs the + // same graph at least twice, once through the checkpointer. + #![proptest_config(ProptestConfig { cases: 48, ..ProptestConfig::default() })] + + /// **Resume equivalence.** Suspending and resuming a run does the same work + /// as never suspending at all — same nodes, same item counts. + /// + /// See the module docs for why this is a work comparison rather than a + /// byte comparison. + #[test] + fn a_resumed_run_does_the_same_work_as_an_uninterrupted_one(shape in arb_gated_shape(2)) { + let Some((straight, resumed)) = run_both_ways(&shape) else { + return Ok(()); + }; + prop_assert_eq!( + work_done(&straight), work_done(&resumed), + "pausing changed which nodes ran or how much they emitted\nshape: {:?}", + shape + ); + } + + /// Resuming is itself deterministic: two identical suspend-and-resume + /// cycles produce byte-identical state. + /// + /// Unlike the property above this *can* compare exactly, because both runs + /// take the same path through the same channel — which makes it the + /// stricter of the two whenever it applies. + #[test] + fn resuming_is_deterministic(shape in arb_gated_shape(2)) { + let Some((_, first)) = run_both_ways(&shape) else { + return Ok(()); + }; + let Some((_, second)) = run_both_ways(&shape) else { + return Ok(()); + }; + prop_assert_eq!( + &first, &second, + "two identical resume cycles disagreed\nshape: {:?}", + shape + ); + } + + /// A run holding an unapproved gate must actually pause — reporting the + /// gate as pending rather than quietly running through it. + /// + /// Without this, the equivalence property above could be satisfied by a + /// gate that never gates: both paths would agree because neither ever + /// suspended, and the whole file would prove nothing. + #[test] + fn an_unapproved_gate_actually_suspends_the_run(shape in arb_gated_shape(1)) { + let graph = graph_of(&shape); + let Ok(compiled) = compile(&graph) else { + return Ok(()); + }; + let outcome = runtime().block_on(async { + let caps = mock_capabilities(); + tokio::time::timeout(GUARD, run(&compiled, json!({}), &caps)) + .await + .expect("run hung") + .expect("an unapproved run should pause, not fail") + }); + prop_assert!( + !outcome.pending_approvals.is_empty(), + "a graph with an unapproved gate completed without pausing" + ); + } +} diff --git a/tests/hitl_e2e.rs b/tests/hitl_e2e.rs index cd346b4c..43ee3c6b 100644 --- a/tests/hitl_e2e.rs +++ b/tests/hitl_e2e.rs @@ -217,3 +217,226 @@ async fn run_with_preapproved_input_completes_immediately() { "downstream must stay blocked when the gate is not approved" ); } + +/// A branch that finished alongside an interrupted one must not run again when +/// the run resumes. +/// +/// Parallel branches all run before any result is folded, so when one of them +/// pauses the others have genuinely completed. Rescheduling them by position — +/// "everything after the interrupt" — would re-run finished work, and for a node +/// with side effects that means firing them twice. The engine reschedules only +/// the branches that actually interrupted. +/// +/// The counting `tool_call` is the instrument: a state diff cannot tell a re-run +/// from a first run when the node is pure, so the assertion is on how many times +/// the capability was invoked. +#[tokio::test] +async fn a_sibling_that_completed_is_not_re_run_on_resume() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// Counts invocations so a re-run is visible. + struct CountingTools(Arc); + + #[async_trait::async_trait] + impl tinyflows::caps::ToolInvoker for CountingTools { + async fn invoke( + &self, + _slug: &str, + _args: Value, + _conn: Option<&str>, + ) -> tinyflows::error::Result { + self.0.fetch_add(1, Ordering::SeqCst); + Ok(json!({ "ok": true })) + } + } + + let calls = Arc::new(AtomicUsize::new(0)); + let mut caps = mock_capabilities(); + caps.tools = Arc::new(CountingTools(calls.clone())); + + // `t` fans out to a gate and to a side-effecting tool call. Both run in the + // same superstep; the gate pauses, the tool call completes. + let graph = WorkflowGraph { + name: "sibling_not_rerun".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node( + "gate", + NodeKind::OutputParser, + json!({ "requires_approval": true }), + ), + node( + "effect", + NodeKind::ToolCall, + json!({ "slug": "side.effect" }), + ), + ], + edges: vec![edge("t", "gate"), edge("t", "effect")], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + + let resumable = run_resumable(&compiled, json!({}), &caps) + .await + .expect("run_resumable"); + assert_eq!( + resumable.outcome().pending_approvals, + vec!["gate".to_string()], + "the gate should pause the run" + ); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "the sibling ran once before the pause" + ); + + resumable + .resume(vec!["gate".to_string()]) + .await + .expect("resume"); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "the sibling had already completed, so resuming must not invoke it a \ + second time" + ); +} + +/// A child workflow paused at an approval gate **pauses the parent** instead of +/// failing it, and approving the namespaced gate lets the whole thing finish. +/// +/// This used to be a hard error: a node executor had no way to inject an +/// interrupt into the parent run, so a gated child halted the parent rather than +/// suspending it — approval gating was unusable across a sub-workflow boundary. +/// +/// The gate surfaces as `::`. The namespace matters: +/// parent and child are separate graphs with separate id spaces, so an +/// unqualified `approve` from the child would be indistinguishable from a +/// parent gate of the same name. +#[tokio::test] +async fn a_child_paused_at_a_gate_pauses_the_parent_and_resumes() { + let child = json!({ + "name": "gated_child", + "nodes": [ + { "id": "ct", "kind": "trigger", "type_version": 1, "name": "ct", "config": null }, + { "id": "cgate", "kind": "output_parser", "type_version": 1, "name": "cgate", + "config": { "requires_approval": true } }, + { "id": "cdone", "kind": "output_parser", "type_version": 1, "name": "cdone", + "config": null } + ], + "edges": [ + { "from_node": "ct", "from_port": "main", "to_node": "cgate", "to_port": "main" }, + { "from_node": "cgate", "from_port": "main", "to_node": "cdone", "to_port": "main" } + ] + }); + + let graph = WorkflowGraph { + name: "parent_of_gated_child".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node("sw", NodeKind::SubWorkflow, json!({ "workflow": child })), + node("after", NodeKind::OutputParser, Value::Null), + ], + edges: vec![edge("t", "sw"), edge("sw", "after")], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + let caps = mock_capabilities(); + + let resumable = run_resumable(&compiled, json!({}), &caps) + .await + .expect("a gated child should pause the parent, not fail it"); + assert_eq!( + resumable.outcome().pending_approvals, + vec!["sw::cgate".to_string()], + "the child's gate surfaces namespaced by the sub_workflow node" + ); + assert!( + resumable.outcome().output["nodes"]["after"].is_null(), + "downstream of the sub-workflow must not run while the child is gated" + ); + + let done = resumable + .resume(vec!["sw::cgate".to_string()]) + .await + .expect("resume"); + assert!( + done.pending_approvals.is_empty(), + "approving the child's gate should settle the run, got {:?}", + done.pending_approvals + ); + assert!( + !done.output["nodes"]["after"]["items"].is_null(), + "the parent continues past the sub-workflow once the child's gate clears" + ); +} + +/// A `per_item` sub-workflow fan-out where several children pause reports +/// **all** their gates at once, not one per resume round-trip. +/// +/// Each element gets its own child run, so N elements means N independent gates. +/// Surfacing only the first would make a host discover them one at a time, each +/// costing a full re-run of the whole fan-out. +#[tokio::test] +async fn a_per_item_fan_out_reports_every_paused_child() { + let child = json!({ + "name": "gated_child", + "nodes": [ + { "id": "ct", "kind": "trigger", "type_version": 1, "name": "ct", "config": null }, + { "id": "cgate", "kind": "output_parser", "type_version": 1, "name": "cgate", + "config": { "requires_approval": true } } + ], + "edges": [ + { "from_node": "ct", "from_port": "main", "to_node": "cgate", "to_port": "main" } + ] + }); + + let graph = WorkflowGraph { + name: "per_item_gated".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node("split", NodeKind::SplitOut, json!({ "path": "rows" })), + node( + "sw", + NodeKind::SubWorkflow, + json!({ "workflow": child, "execution": "per_item", "concurrency": 3 }), + ), + ], + edges: vec![edge("t", "split"), edge("split", "sw")], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + let caps = mock_capabilities(); + + let resumable = run_resumable(&compiled, json!({ "rows": [1, 2, 3] }), &caps) + .await + .expect("the fan-out should pause rather than fail"); + + // Every child paused at the same gate id, so the namespaced set collapses to + // one entry — the point being that it is reported, and reported once, rather + // than the node failing or hiding the pause behind a single child. + assert_eq!( + resumable.outcome().pending_approvals, + vec!["sw::cgate".to_string()], + "the fan-out's paused children surface as a namespaced gate" + ); + + let done = resumable + .resume(vec!["sw::cgate".to_string()]) + .await + .expect("resume"); + assert!( + done.pending_approvals.is_empty(), + "approving the gate clears every child in the fan-out, got {:?}", + done.pending_approvals + ); + assert_eq!( + done.output["nodes"]["sw"]["items"] + .as_array() + .map(Vec::len) + .unwrap_or(0), + 3, + "all three children ran to completion once approved" + ); +} diff --git a/tests/loop_e2e.rs b/tests/loop_e2e.rs index 84add503..2d68d1be 100644 --- a/tests/loop_e2e.rs +++ b/tests/loop_e2e.rs @@ -14,6 +14,7 @@ //! Gated behind the `mock` cargo feature so plain `cargo test` skips it while //! `cargo test --all-features` runs it. +use std::sync::{Arc, Mutex}; use std::time::Duration; use serde_json::{Value, json}; @@ -24,10 +25,25 @@ use tinyflows::engine::run; use tinyflows::error::EngineError; use tinyflows::error::ValidationError; use tinyflows::model::{Edge, Node, NodeKind, WorkflowGraph}; +use tinyflows::observability::RunObserver; /// How long any single run in this file may take before it is called a hang. const GUARD: Duration = Duration::from_secs(10); +/// Records the order nodes finished in, so a test can assert on sequencing that +/// the final state cannot show. +#[derive(Default)] +struct Trace(Mutex>); + +impl RunObserver for Trace { + fn on_step_finish(&self, step: &tinyflows::observability::ExecutionStep) { + self.0 + .lock() + .expect("trace mutex poisoned") + .push(step.node_id.clone()); + } +} + /// Builds a node with the given id, kind, and config (no ports, no position). fn node(id: &str, kind: NodeKind, config: Value) -> Node { Node { @@ -195,12 +211,20 @@ async fn a_plain_back_edge_onto_a_mid_graph_node_iterates() { ); } -/// A loop head may not itself be a fan-in, and validation says so rather than -/// letting the graph run once and stop. The barrier the graph runtime installs for a -/// node's forward predecessors is per-node, so it swallows the re-entry the -/// back-edge delivers — the loop would silently iterate exactly once. +/// A loop head that is **also** a fan-in iterates properly. +/// +/// This used to be refused. The barrier a fan-in installs is keyed on the +/// target node rather than on the edge, so the re-entry the back-edge delivered +/// was tested against the head's *forward* predecessors, failed that test, and +/// was dropped — the loop ran exactly one pass and stopped. The gate now +/// ignores arrivals from predecessors outside its required set, and a +/// back-edge's source is never in that set, so the re-entry lands. +/// +/// The assertion that matters is the iteration count: a passing run that +/// stopped after one pass would still reach `out`, so only the count +/// distinguishes a fixed loop from the old silent failure. #[tokio::test] -async fn a_loop_head_that_is_also_a_fan_in_is_refused() { +async fn a_loop_head_that_is_also_a_fan_in_iterates() { let graph = WorkflowGraph { name: "fan_in_loop".to_string(), nodes: vec![ @@ -229,10 +253,23 @@ async fn a_loop_head_that_is_also_a_fan_in_is_refused() { let errors = tinyflows::validate::validate_all(&graph); assert!( - errors - .iter() - .any(|e| matches!(e, ValidationError::IllegalCycle(id) if id == "l")), - "a fan-in loop head should be refused, got: {errors:?}" + errors.is_empty(), + "a fan-in loop head is legal now that the barrier ignores back-edge \ + arrivals, got: {errors:?}" + ); + + let outcome = run_guarded(&graph) + .await + .expect("the loop should run rather than deadlock"); + + assert_eq!( + outcome.output["nodes"]["l"]["iteration"], 2, + "the loop should consume both its iterations; a count of 1 means the \ + back-edge re-entry was swallowed by the fan-in barrier again" + ); + assert!( + outcome.output["nodes"]["out"].get("items").is_some(), + "the loop should still leave through `done` into the downstream node" ); } @@ -405,3 +442,431 @@ async fn max_node_visits_bounds_a_cycle_and_names_the_node() { "the failure should name the node and its visit cap, got: {message}" ); } + +/// A diamond **inside** the loop body iterates, and the merge fires once per +/// pass rather than on stale data. +/// +/// Two things had to be true for this. The barrier re-arms on its own, because +/// arrivals are cleared when it fires. And barrier *relief* had to stop firing +/// phantom arrivals here: the arms are reachable only through the head's `body` +/// port, so relief is registered for them, and relief's forward walk used to +/// stop at the fan-out (`apex`) because a command node has no static edge. It +/// concluded the arms were untaken and cleared the barrier early — the observed +/// order was `l, apex, join, arm_a, arm_b, …`, the join reading the previous +/// pass's data. Silently wrong output, not a hang. +/// +/// So this test asserts both the iteration count *and* the ordering: `join` +/// must never run before both arms have, on any pass. +#[tokio::test] +async fn a_diamond_inside_the_loop_body_iterates() { + let graph = WorkflowGraph { + name: "diamond_in_loop".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node( + "l", + NodeKind::Loop, + json!({ "max_iterations": 3, "on_exceeded": "continue" }), + ), + node("apex", NodeKind::OutputParser, Value::Null), + node( + "arm_a", + NodeKind::Transform, + json!({ "set": { "arm": "a" } }), + ), + node( + "arm_b", + NodeKind::Transform, + json!({ "set": { "arm": "b" } }), + ), + node("join", NodeKind::Merge, Value::Null), + node("out", NodeKind::OutputParser, Value::Null), + ], + edges: vec![ + edge("t", "l"), + port_edge("l", "body", "apex"), + // Both arms leave `apex` on the same port: a parallel fan-out. + edge("apex", "arm_a"), + edge("apex", "arm_b"), + edge("arm_a", "join"), + edge("arm_b", "join"), + edge("join", "l"), // the back-edge closing the cycle + port_edge("l", "done", "out"), + ], + ..Default::default() + }; + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.is_empty(), + "a diamond whose arms are both on the cycle is legal, got: {errors:?}" + ); + + let trace = Arc::new(Trace::default()); + let observer: Arc = trace.clone(); + let caps = mock_capabilities(); + let compiled = compile(&graph).expect("compile"); + let outcome = tokio::time::timeout( + GUARD, + tinyflows::engine::run_with_observer(&compiled, json!({}), &caps, &observer), + ) + .await + .expect("run hung — the diamond deadlocked the loop") + .expect("the loop should iterate rather than fail"); + + assert_eq!( + outcome.output["nodes"]["l"]["iteration"], 3, + "every pass should complete the merge barrier" + ); + + // Ordering: walking the activation trace, `join` may only run when both + // arms have run since the last time it did. A `join` that fires early is + // the phantom-arrival bug, and it does not show up in the final state. + let order = trace.0.lock().expect("trace mutex poisoned").clone(); + let (mut seen_a, mut seen_b, mut joins) = (false, false, 0); + for id in &order { + match id.as_str() { + "arm_a" => seen_a = true, + "arm_b" => seen_b = true, + "join" => { + assert!( + seen_a && seen_b, + "`join` fired before both arms had run on this pass — barrier \ + relief cleared the barrier early. Trace: {order:?}" + ); + joins += 1; + seen_a = false; + seen_b = false; + } + _ => {} + } + } + assert_eq!( + joins, 3, + "the merge should fire once per pass. Trace: {order:?}" + ); +} + +/// The case that stays refused: a merge on the cycle that also waits on a +/// predecessor from **outside** the cycle. +/// +/// The off-cycle arm runs once, on the seeding pass, and never activates again, +/// so from the second iteration the barrier can never complete its required set +/// and the loop stops dead. Refusing it beats hanging. +#[tokio::test] +async fn a_merge_on_the_cycle_waiting_on_an_off_cycle_arm_is_refused() { + let graph = WorkflowGraph { + name: "off_cycle_arm".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + // `outside` runs once from the trigger and is not on the cycle. + node("outside", NodeKind::OutputParser, Value::Null), + node( + "l", + NodeKind::Loop, + json!({ "max_iterations": 3, "on_exceeded": "continue" }), + ), + node("work", NodeKind::OutputParser, Value::Null), + node("join", NodeKind::Merge, Value::Null), + node("out", NodeKind::OutputParser, Value::Null), + ], + edges: vec![ + edge("t", "l"), + edge("t", "outside"), + port_edge("l", "body", "work"), + edge("work", "join"), + edge("outside", "join"), // the off-cycle arm + edge("join", "l"), + port_edge("l", "done", "out"), + ], + ..Default::default() + }; + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors + .iter() + .any(|e| matches!(e, ValidationError::IllegalCycle(id) if id == "join")), + "a merge waiting on an off-cycle arm should still be refused, got: {errors:?}" + ); +} + +/// The accumulator survives across iterations: each pass folds the body's +/// output into state that the next pass reads back. +/// +/// This is the capability the node gained — without it a loop passes its input +/// straight through and cannot remember what it tried. +#[tokio::test] +async fn an_accumulator_survives_across_iterations() { + let graph = loop_graph(json!({ + "max_iterations": 3, + "on_exceeded": "continue", + "state": { + "init": { "attempts": [] }, + // Append the pass number each time round. + "update": "={ attempts: (.state.attempts + [((.state.attempts | length) + 1)]) }" + } + })); + + let outcome = run_guarded(&graph).await.expect("run"); + assert_eq!( + outcome.output["nodes"]["l"]["state"]["attempts"], + json!([1, 2, 3]), + "one append per pass, carried across iterations" + ); +} + +/// A key can be *removed* from the accumulator. +/// +/// The regression test for the `$replace` sentinel. Under a plain deep merge an +/// object slot can only ever gain keys, so an accumulator could never drop one +/// — an error recorded on pass 1 would haunt every later pass. +#[tokio::test] +async fn an_accumulator_can_drop_a_key_it_previously_held() { + let graph = loop_graph(json!({ + "max_iterations": 2, + "on_exceeded": "continue", + "state": { + "init": { "err": "boom", "tries": 0 }, + // Rebuild the accumulator without `err`. + "update": "={ tries: (.state.tries + 1) }" + } + })); + + let outcome = run_guarded(&graph).await.expect("run"); + let state = &outcome.output["nodes"]["l"]["state"]; + assert_eq!(state["tries"], 2, "the surviving key still accumulates"); + assert!( + state.get("err").is_none(), + "the dropped key must actually be gone, got: {state}" + ); +} + +/// `until` exits as soon as the check passes, before the cap, and says so. +#[tokio::test] +async fn until_exits_early_and_reports_its_reason() { + let graph = loop_graph(json!({ + "max_iterations": 10, + "on_exceeded": "continue", + "state": { "init": { "tries": 0 }, "update": "={ tries: (.state.tries + 1) }" }, + "until": "=.state.tries >= 2" + })); + + let outcome = run_guarded(&graph).await.expect("run"); + assert_eq!(outcome.output["nodes"]["l"]["exit_reason"], "until"); + assert_eq!( + outcome.output["nodes"]["l"]["iteration"], 2, + "it should stop at the check, well short of the cap of 10" + ); +} + +/// An `until` that never passes falls through to the cap, and the exit reason +/// distinguishes that from converging. +/// +/// This is what finally makes `on_exceeded: "continue"` usable: downstream +/// could not previously tell a loop that succeeded from one that ran out. +#[tokio::test] +async fn an_until_that_never_passes_reports_exhaustion_not_success() { + let graph = loop_graph(json!({ + "max_iterations": 2, + "on_exceeded": "continue", + "state": { "init": { "tries": 0 }, "update": "={ tries: (.state.tries + 1) }" }, + "until": "=.state.tries >= 99" + })); + + let outcome = run_guarded(&graph).await.expect("run"); + assert_eq!( + outcome.output["nodes"]["l"]["exit_reason"], + "max_iterations" + ); +} + +/// `success_port: true` routes a converged exit away from an exhausted one, so +/// the two outcomes can be handled differently. +#[tokio::test] +async fn the_success_port_separates_convergence_from_exhaustion() { + let mut graph = loop_graph(json!({ + "max_iterations": 10, + "on_exceeded": "continue", + "success_port": true, + "state": { "init": { "tries": 0 }, "update": "={ tries: (.state.tries + 1) }" }, + "until": "=.state.tries >= 2" + })); + graph + .nodes + .push(node("won", NodeKind::OutputParser, Value::Null)); + graph.edges.push(port_edge("l", "success", "won")); + + let outcome = run_guarded(&graph).await.expect("run"); + assert!( + !outcome.output["nodes"]["won"]["items"].is_null(), + "a converged loop leaves through `success`" + ); + assert!( + outcome.output["nodes"]["out"].is_null(), + "and not through `done`, which is the exhaustion path" + ); +} + +/// The accumulator is addressable from anywhere in the graph, like the +/// iteration count — including from inside the loop body. +#[tokio::test] +async fn the_body_can_read_the_accumulator() { + let mut graph = loop_graph(json!({ + "max_iterations": 2, + "on_exceeded": "continue", + "state": { "init": { "tries": 0 }, "update": "={ tries: (.state.tries + 1) }" } + })); + // Replace the body with a transform that stamps what it can see. + graph.nodes.retain(|n| n.id != "work"); + graph.nodes.push(node( + "work", + NodeKind::Transform, + json!({ "set": { "seen": "=nodes.l.state.tries" } }), + )); + + let outcome = run_guarded(&graph).await.expect("run"); + assert!( + !outcome.output["nodes"]["work"]["items"].is_null(), + "the body ran and could resolve =nodes.l.state.tries" + ); +} + +/// `emit: "state"` puts the accumulator on the exit port, so downstream +/// receives what the loop built rather than the last pass's items. +#[tokio::test] +async fn emit_state_puts_the_accumulator_on_the_done_port() { + let graph = loop_graph(json!({ + "max_iterations": 2, + "on_exceeded": "continue", + "emit": "state", + "state": { "init": { "tries": 0 }, "update": "={ tries: (.state.tries + 1) }" } + })); + + let outcome = run_guarded(&graph).await.expect("run"); + let items = outcome.output["nodes"]["out"]["items"] + .as_array() + .expect("downstream items"); + assert_eq!(items.len(), 1); + assert_eq!( + items[0]["json"]["tries"], 2, + "downstream got the accumulator" + ); +} + +/// A loop with no `state` config behaves exactly as before. +/// +/// The accumulator is additive: every existing graph must be unaffected. +#[tokio::test] +async fn a_loop_without_an_accumulator_is_unchanged() { + let outcome = run_guarded(&loop_graph( + json!({ "max_iterations": 3, "on_exceeded": "continue" }), + )) + .await + .expect("run"); + + assert_eq!(outcome.output["nodes"]["l"]["iteration"], 3); + assert_eq!(outcome.output["nodes"]["l"]["port"], "done"); + assert!( + !outcome.output["nodes"]["out"].is_null(), + "downstream still receives the last pass's items" + ); +} + +/// A cycle bounded only by `max_node_visits` validates. +/// +/// It used to be refused: only `recursion_limit` counted as proof of +/// boundedness, so a graph that genuinely could not run away was rejected — and +/// the author was pushed toward the *less* informative knob, since +/// `recursion_limit` can only report that the run looped while +/// `max_node_visits` names the node that did. +#[tokio::test] +async fn a_cycle_bounded_only_by_max_node_visits_is_legal() { + let graph = WorkflowGraph { + name: "visits_bounded".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, json!({ "max_node_visits": 4 })), + node("a", NodeKind::OutputParser, Value::Null), + node("b", NodeKind::OutputParser, Value::Null), + ], + edges: vec![edge("t", "a"), edge("a", "b"), edge("b", "a")], + ..Default::default() + }; + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.is_empty(), + "max_node_visits bounds the cycle, so the graph is legal, got: {errors:?}" + ); + + // And the bound really is enforced, naming the runaway node. + let err = run_guarded(&graph) + .await + .expect_err("the visit cap should stop the cycle"); + let message = err.to_string(); + assert!( + message.contains('a') && message.to_lowercase().contains("visit"), + "the failure should name the node and its visit cap, got: {message}" + ); +} + +/// A cycle with neither run-level bound nor a `loop` node is still refused — +/// the lift widened what counts as a bound, it did not remove the requirement. +#[tokio::test] +async fn an_unbounded_cycle_is_still_refused_after_the_lift() { + let graph = WorkflowGraph { + name: "still_unbounded".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node("a", NodeKind::OutputParser, Value::Null), + node("b", NodeKind::OutputParser, Value::Null), + ], + edges: vec![edge("t", "a"), edge("a", "b"), edge("b", "a")], + ..Default::default() + }; + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors + .iter() + .any(|e| matches!(e, ValidationError::IllegalCycle(_))), + "a cycle with no bound at all must still be refused, got: {errors:?}" + ); +} + +/// `success_port: true` with nothing wired to `success` is refused, because a +/// converged loop would otherwise strand the run at an unwired port — which +/// reads as "the loop never finished" rather than as a wiring mistake. +#[tokio::test] +async fn an_unwired_success_port_is_refused() { + let graph = loop_graph(json!({ + "max_iterations": 3, + "success_port": true, + "until": "=true" + })); + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.iter().any(|e| matches!( + e, + ValidationError::InvalidNodeConfig { node, reason } + if node == "l" && reason.contains("success") + )), + "an unwired success port should be refused, got: {errors:?}" + ); +} + +/// The new config keys are checked at author time rather than silently ignored. +#[tokio::test] +async fn malformed_accumulator_config_is_refused() { + for bad in [ + json!({ "max_iterations": 2, "state": "not an object" }), + json!({ "max_iterations": 2, "emit": "sideways" }), + ] { + let errors = tinyflows::validate::validate_all(&loop_graph(bad.clone())); + assert!( + errors.iter().any( + |e| matches!(e, ValidationError::InvalidNodeConfig { node, .. } if node == "l") + ), + "config {bad} should be refused, got: {errors:?}" + ); + } +} diff --git a/tests/parallel_e2e.rs b/tests/parallel_e2e.rs index a67888d6..11312ddd 100644 --- a/tests/parallel_e2e.rs +++ b/tests/parallel_e2e.rs @@ -211,3 +211,94 @@ async fn diamond_every_branch_contributes_to_merge() { ); } } + +/// `trigger.config.max_concurrency` bounds how many branches of a super-step +/// run at once, without changing what the run computes. +/// +/// Eight branches fan out from one node. With the dial set to 2, at most two may +/// ever be in flight — measured as observed overlap, since a bound that silently +/// failed to apply would produce the identical final state. +/// +/// This is admission control, not backpressure: the engine cannot block a +/// branch mid-step, only decide how many are allowed to start. +#[tokio::test] +async fn max_concurrency_bounds_how_many_branches_run_at_once() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// A tool that reports the peak number of concurrent invocations it saw. + struct OverlapProbe { + in_flight: AtomicUsize, + peak: AtomicUsize, + } + + #[async_trait::async_trait] + impl tinyflows::caps::ToolInvoker for OverlapProbe { + async fn invoke( + &self, + _slug: &str, + _args: Value, + _conn: Option<&str>, + ) -> tinyflows::error::Result { + let now = self.in_flight.fetch_add(1, Ordering::SeqCst) + 1; + self.peak.fetch_max(now, Ordering::SeqCst); + // Yield enough that unbounded branches would visibly overlap. + for _ in 0..8 { + tokio::task::yield_now().await; + } + self.in_flight.fetch_sub(1, Ordering::SeqCst); + Ok(json!({ "ok": true })) + } + } + + async fn peak_overlap(max_concurrency: Option) -> usize { + let probe = Arc::new(OverlapProbe { + in_flight: AtomicUsize::new(0), + peak: AtomicUsize::new(0), + }); + let mut caps = mock_capabilities(); + caps.tools = probe.clone(); + + let mut trigger_config = json!({}); + if let Some(limit) = max_concurrency { + trigger_config["max_concurrency"] = json!(limit); + } + + let branches: Vec = (0..8).map(|i| format!("b{i}")).collect(); + let mut nodes = vec![ + node("t", NodeKind::Trigger, trigger_config), + node("apex", NodeKind::OutputParser, Value::Null), + ]; + let mut edges = vec![edge("t", "main", "apex")]; + for id in &branches { + nodes.push(node(id, NodeKind::ToolCall, json!({ "slug": "probe.run" }))); + edges.push(edge("apex", "main", id)); + } + + let graph = WorkflowGraph { + name: "concurrency_dial".to_string(), + nodes, + edges, + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + run(&compiled, json!({}), &caps).await.expect("run"); + probe.peak.load(Ordering::SeqCst) + } + + let bounded = peak_overlap(Some(2)).await; + assert!( + bounded <= 2, + "max_concurrency: 2 must bound in-flight branches, observed peak {bounded}" + ); + + // Without the dial the same graph genuinely overlaps more, which is what + // makes the assertion above meaningful rather than vacuously true. + let unbounded = peak_overlap(None).await; + assert!( + unbounded > 2, + "the same graph should overlap more without the dial, observed peak \ + {unbounded} — if this is low the probe is not actually concurrent and \ + the bounded assertion proves nothing" + ); +} diff --git a/tests/scatter_gather_e2e.rs b/tests/scatter_gather_e2e.rs new file mode 100644 index 00000000..eb4769ba --- /dev/null +++ b/tests/scatter_gather_e2e.rs @@ -0,0 +1,424 @@ +#![cfg(feature = "mock")] +//! End-to-end tests for `scatter` / `gather`. +//! +//! The claim under test is that a scatter fans out the **downstream path**, not +//! just its immediate successors: every node between the scatter and its gather +//! runs once per lane. A test that only checked the gather's output would pass +//! for a fan-out that ran the pipeline once with all the items, so these assert +//! on how many times the *intermediate* nodes ran. +//! +//! Every run is wrapped in a timeout: a gather that never releases hangs rather +//! than fails. + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; + +use serde_json::{Value, json}; + +use tinyflows::caps::mock::mock_capabilities; +use tinyflows::compiler::compile; +use tinyflows::engine::run; +use tinyflows::model::{Edge, Node, NodeKind, WorkflowGraph}; + +const GUARD: Duration = Duration::from_secs(20); + +fn node(id: &str, kind: NodeKind, config: Value) -> Node { + Node { + id: id.to_string(), + kind, + type_version: 1, + name: id.to_string(), + config, + ports: vec![], + position: None, + } +} + +fn edge(from: &str, to: &str) -> Edge { + Edge { + from_node: from.to_string(), + from_port: "main".to_string(), + to_node: to.to_string(), + to_port: "main".to_string(), + } +} + +/// `t -> scatter -> work -> gather`, fanning out over `rows`. +fn scatter_graph(scatter_config: Value, gather_config: Value) -> WorkflowGraph { + let mut gather = gather_config; + gather["from"] = json!(["work"]); + WorkflowGraph { + name: "scatter_gather".to_string(), + nodes: vec![ + node( + "t", + NodeKind::Trigger, + json!({ "recursion_limit": 400, "max_node_visits": 300 }), + ), + node("fan", NodeKind::Scatter, scatter_config), + node( + "work", + NodeKind::Transform, + json!({ "set": { "seen": "=item.v" } }), + ), + node("collect", NodeKind::Gather, gather), + ], + edges: vec![ + edge("t", "fan"), + edge("fan", "work"), + edge("work", "collect"), + ], + ..Default::default() + } +} + +async fn run_guarded( + graph: &WorkflowGraph, + caps: &tinyflows::caps::Capabilities, + input: Value, +) -> tinyflows::error::Result { + let compiled = compile(graph).expect("compile"); + match tokio::time::timeout(GUARD, run(&compiled, input, caps)).await { + Err(_) => panic!("run hung past {GUARD:?} — a gather never released"), + Ok(inner) => inner, + } +} + +/// The headline behaviour: three lanes, and the lane body runs three times. +#[tokio::test] +async fn a_scatter_runs_the_downstream_path_once_per_lane() { + let outcome = run_guarded( + &scatter_graph(json!({ "path": "rows" }), json!({})), + &mock_capabilities(), + json!({ "rows": [{ "v": "a" }, { "v": "b" }, { "v": "c" }] }), + ) + .await + .expect("run"); + + // The lane worker recorded one slot per lane, not one slot in total. This is + // the assertion that separates a scatter from an ordinary fan-out. + let lanes = outcome.output["nodes"]["work"]["lanes"] + .as_object() + .expect("the lane worker recorded per-lane slots"); + assert_eq!(lanes.len(), 3, "one lane slot per lane, got {lanes:?}"); + + let items = outcome.output["nodes"]["collect"]["items"] + .as_array() + .expect("the gather emitted items"); + assert_eq!(items.len(), 3, "every lane's output reached the gather"); + let seen: Vec<&str> = items + .iter() + .filter_map(|item| item["json"]["seen"].as_str()) + .collect(); + assert_eq!( + seen, + vec!["a", "b", "c"], + "results are ordered by lane index, and each lane saw only its own item" + ); +} + +/// Lanes genuinely run concurrently rather than one after another. +/// +/// Measured as observed overlap: a scatter that ran its lanes sequentially would +/// produce identical output. +#[tokio::test] +async fn lanes_run_concurrently() { + struct OverlapProbe { + in_flight: AtomicUsize, + peak: AtomicUsize, + } + + #[async_trait::async_trait] + impl tinyflows::caps::ToolInvoker for OverlapProbe { + async fn invoke( + &self, + _slug: &str, + _args: Value, + _conn: Option<&str>, + ) -> tinyflows::error::Result { + let now = self.in_flight.fetch_add(1, Ordering::SeqCst) + 1; + self.peak.fetch_max(now, Ordering::SeqCst); + for _ in 0..8 { + tokio::task::yield_now().await; + } + self.in_flight.fetch_sub(1, Ordering::SeqCst); + Ok(json!({ "ok": true })) + } + } + + let probe = Arc::new(OverlapProbe { + in_flight: AtomicUsize::new(0), + peak: AtomicUsize::new(0), + }); + let mut caps = mock_capabilities(); + caps.tools = probe.clone(); + + let mut graph = scatter_graph(json!({ "path": "rows" }), json!({})); + // Swap the lane body for a capability-backed node so overlap is observable. + graph.nodes.retain(|n| n.id != "work"); + graph.nodes.push(node( + "work", + NodeKind::ToolCall, + json!({ "slug": "lane.run" }), + )); + + run_guarded( + &graph, + &caps, + json!({ "rows": [{ "v": 1 }, { "v": 2 }, { "v": 3 }, { "v": 4 }] }), + ) + .await + .expect("run"); + + assert!( + probe.peak.load(Ordering::SeqCst) > 1, + "lanes must overlap; observed peak {} means they ran one at a time", + probe.peak.load(Ordering::SeqCst) + ); +} + +/// A multi-node lane carries its envelope the whole way: every node between the +/// scatter and the gather runs per lane, not just the first. +#[tokio::test] +async fn a_lane_spanning_several_nodes_runs_each_of_them_per_lane() { + let mut graph = scatter_graph(json!({ "path": "rows" }), json!({})); + graph.nodes.push(node( + "second", + NodeKind::Transform, + json!({ "set": { "stage": 2 } }), + )); + // Re-wire: fan -> work -> second -> collect. + graph + .edges + .retain(|e| !(e.from_node == "work" && e.to_node == "collect")); + graph.edges.push(edge("work", "second")); + graph.edges.push(edge("second", "collect")); + for n in &mut graph.nodes { + if n.id == "collect" { + n.config["from"] = json!(["second"]); + } + } + + let outcome = run_guarded( + &graph, + &mock_capabilities(), + json!({ "rows": [{ "v": "a" }, { "v": "b" }] }), + ) + .await + .expect("run"); + + for id in ["work", "second"] { + let lanes = outcome.output["nodes"][id]["lanes"] + .as_object() + .unwrap_or_else(|| panic!("{id} should have per-lane slots")); + assert_eq!(lanes.len(), 2, "{id} ran once per lane"); + } + assert_eq!( + outcome.output["nodes"]["collect"]["items"] + .as_array() + .map(Vec::len), + Some(2), + "both lanes reached the gather through the two-node body" + ); +} + +/// Lane slots never clobber each other, and never write the top-level slot. +/// +/// This is the state-model invariant the whole design rests on: N concurrent +/// activations of one node id fold through a key-by-key reducer, so they must +/// write disjoint keys. +#[tokio::test] +async fn lanes_write_disjoint_slots_and_leave_the_top_level_alone() { + let outcome = run_guarded( + &scatter_graph(json!({ "path": "rows" }), json!({})), + &mock_capabilities(), + json!({ "rows": [{ "v": 1 }, { "v": 2 }, { "v": 3 }, { "v": 4 }, { "v": 5 }] }), + ) + .await + .expect("run"); + + let work = &outcome.output["nodes"]["work"]; + let lanes = work["lanes"].as_object().expect("lane slots"); + assert_eq!(lanes.len(), 5, "every lane kept its own slot"); + + // Each lane recorded a distinct index, so none overwrote another. + let mut indices: Vec = lanes + .values() + .filter_map(|slot| slot["index"].as_u64()) + .collect(); + indices.sort_unstable(); + assert_eq!(indices, vec![0, 1, 2, 3, 4]); + + assert!( + work.get("items").is_none(), + "a lane activation must never write the node's top-level items slot, got: {work}" + ); +} + +/// `lanes: n` chunks the work rather than opening a lane per item, so a wide +/// input can run a bounded number of lanes. +#[tokio::test] +async fn a_lane_count_chunks_the_work() { + let rows: Vec = (0..9).map(|i| json!({ "v": i })).collect(); + let outcome = run_guarded( + &scatter_graph(json!({ "path": "rows", "lanes": 3 }), json!({})), + &mock_capabilities(), + json!({ "rows": rows }), + ) + .await + .expect("run"); + + let lanes = outcome.output["nodes"]["work"]["lanes"] + .as_object() + .expect("lane slots"); + assert_eq!(lanes.len(), 3, "nine items ran in three lanes, not nine"); + assert_eq!( + outcome.output["nodes"]["collect"]["items"] + .as_array() + .map(Vec::len), + Some(9), + "all nine items still reach the gather" + ); +} + +/// A gather releasing on a quorum emits early rather than waiting for lanes it +/// was told it does not need. +#[tokio::test] +async fn a_gather_can_release_on_a_quorum() { + let outcome = run_guarded( + &scatter_graph( + json!({ "path": "rows" }), + json!({ "release": "quorum", "n": 2 }), + ), + &mock_capabilities(), + json!({ "rows": [{ "v": "a" }, { "v": "b" }, { "v": "c" }] }), + ) + .await + .expect("run"); + + let arrived = outcome.output["nodes"]["collect"]["arrived"] + .as_u64() + .expect("the gather records how many lanes it saw"); + assert!( + arrived >= 2, + "a quorum of 2 must not release with fewer, got {arrived}" + ); +} + +/// A scatter over an empty input opens no lanes, and the gather still releases +/// rather than waiting forever for arrivals that can never come. +#[tokio::test] +async fn an_empty_scatter_does_not_hang_the_gather() { + let outcome = run_guarded( + &scatter_graph(json!({ "path": "rows" }), json!({})), + &mock_capabilities(), + json!({ "rows": [] }), + ) + .await + .expect("an empty scatter should complete, not hang"); + + assert_eq!( + outcome.output["nodes"]["collect"]["items"] + .as_array() + .map(Vec::len), + Some(0), + "no lanes means no results, but the run still finishes" + ); +} + +/// A lane that leaves the region without passing through the gather is refused. +/// +/// This is the guard on the invariant the whole design rests on. The engine +/// propagates a lane envelope to every successor that is not a gather, so an +/// edge out of the region carries the lane somewhere nothing collects it — and +/// the node on the far side, having no gather to converge on, writes its +/// top-level slot as though it were never in a lane. Wrong output, not a +/// failure, which is exactly what must be caught at author time. +#[tokio::test] +async fn a_lane_escaping_the_region_is_refused() { + let mut graph = scatter_graph(json!({ "path": "rows" }), json!({})); + graph + .nodes + .push(node("escapee", NodeKind::OutputParser, Value::Null)); + graph.edges.push(edge("work", "escapee")); + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.iter().any(|e| format!("{e:?}").contains("escapee")), + "an edge out of the lane region should be refused, got: {errors:?}" + ); +} + +/// A scatter with no gather downstream is refused: its lanes would run with +/// nothing to collect them. +#[tokio::test] +async fn a_scatter_without_a_gather_is_refused() { + let mut graph = scatter_graph(json!({ "path": "rows" }), json!({})); + graph.nodes.retain(|n| n.id != "collect"); + graph.edges.retain(|e| e.to_node != "collect"); + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.iter().any(|e| format!("{e:?}").contains("fan")), + "a scatter with no gather should be refused, got: {errors:?}" + ); +} + +/// A gather with no scatter upstream is refused: it would wait on lanes nobody +/// opens until its poll budget ran out. +#[tokio::test] +async fn a_gather_without_a_scatter_is_refused() { + let graph = WorkflowGraph { + name: "orphan_gather".to_string(), + nodes: vec![ + node("t", NodeKind::Trigger, Value::Null), + node("work", NodeKind::OutputParser, Value::Null), + node("collect", NodeKind::Gather, json!({ "from": ["work"] })), + ], + edges: vec![edge("t", "work"), edge("work", "collect")], + ..Default::default() + }; + + let errors = tinyflows::validate::validate_all(&graph); + assert!( + errors.iter().any(|e| format!("{e:?}").contains("collect")), + "an orphan gather should be refused, got: {errors:?}" + ); +} + +/// The v1 region restrictions, each refused with its own reason rather than +/// producing a subtly wrong run. +#[tokio::test] +async fn the_unsupported_region_members_are_refused() { + // A nested scatter: lane ids would have to compose, and the inner gather + // would have to know which level it closes. + let mut nested = scatter_graph(json!({ "path": "rows" }), json!({})); + for n in &mut nested.nodes { + if n.id == "work" { + n.kind = NodeKind::Scatter; + n.config = json!({ "path": "inner" }); + } + } + assert!( + tinyflows::validate::validate_all(&nested) + .iter() + .any(|e| format!("{e:?}").contains("nested")), + "a nested scatter should be refused" + ); + + // An approval gate: a resume is addressed by node id, so every lane of one + // node would share a single approval. + let mut gated = scatter_graph(json!({ "path": "rows" }), json!({})); + for n in &mut gated.nodes { + if n.id == "work" { + n.config = json!({ "requires_approval": true }); + } + } + assert!( + tinyflows::validate::validate_all(&gated) + .iter() + .any(|e| format!("{e:?}").contains("requires_approval")), + "an approval gate inside a lane should be refused" + ); +} diff --git a/tests/smoke_all_nodes.rs b/tests/smoke_all_nodes.rs index e8b8e307..cb0ac80e 100644 --- a/tests/smoke_all_nodes.rs +++ b/tests/smoke_all_nodes.rs @@ -288,3 +288,87 @@ async fn big_chained_workflow_runs_end_to_end() { ); assert!(outcome.pending_approvals.is_empty()); } + +#[tokio::test] +async fn smoke_spawn() { + smoke_single_node( + NodeKind::Spawn, + json!({ "target": "tool", "slug": "smoke.run", "args": { "x": 1 } }), + json!({}), + ) + .await; +} + +/// A `gate` cannot be smoke-tested on its own: with nothing to wait for it has +/// no tickets, and a gate with zero expected arrivals releases immediately. So +/// this drives the real pair — `spawn -> gate` — which is the only shape a gate +/// is ever authored in. +#[tokio::test] +async fn smoke_gate_collects_a_spawn() { + let graph = WorkflowGraph { + name: "smoke_gate".to_string(), + nodes: vec![ + trigger("t", TriggerKind::Manual), + node( + "kick", + NodeKind::Spawn, + json!({ "target": "tool", "slug": "smoke.run" }), + ), + node( + "n", + NodeKind::Gate, + json!({ "from": ["kick"], "poll_interval_ms": 1 }), + ), + ], + edges: vec![edge("t", "main", "kick"), edge("kick", "main", "n")], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + let outcome = run(&compiled, json!({}), &mock_capabilities()) + .await + .expect("run should succeed"); + + let items = outcome.output["nodes"]["n"]["items"] + .as_array() + .expect("the gate should produce an items array"); + assert_eq!(items.len(), 1, "one spawned task, one collected result"); + assert!(items[0].get("json").is_some()); +} + +/// `scatter` and `gather` are only meaningful as a pair, so the smoke test +/// drives the real shape rather than either alone. +#[tokio::test] +async fn smoke_scatter_gather() { + let graph = WorkflowGraph { + name: "smoke_scatter".to_string(), + nodes: vec![ + trigger("t", TriggerKind::Manual), + node("fan", NodeKind::Scatter, json!({ "path": "rows" })), + node( + "work", + NodeKind::Transform, + json!({ "set": { "seen": "=item.v" } }), + ), + node("n", NodeKind::Gather, json!({ "from": ["work"] })), + ], + edges: vec![ + edge("t", "main", "fan"), + edge("fan", "main", "work"), + edge("work", "main", "n"), + ], + ..Default::default() + }; + let compiled = compile(&graph).expect("compile"); + let outcome = run( + &compiled, + json!({ "rows": [{ "v": 1 }, { "v": 2 }] }), + &mock_capabilities(), + ) + .await + .expect("run should succeed"); + + let items = outcome.output["nodes"]["n"]["items"] + .as_array() + .expect("the gather should produce an items array"); + assert_eq!(items.len(), 2, "two lanes, two collected results"); +} diff --git a/tests/support/graphgen.rs b/tests/support/graphgen.rs new file mode 100644 index 00000000..d909df90 --- /dev/null +++ b/tests/support/graphgen.rs @@ -0,0 +1,410 @@ +//! A generator of valid, non-trivial workflow graphs, for property tests. +//! +//! # Why a shape grammar rather than random edges +//! +//! Wiring random nodes to random nodes almost never produces a graph the +//! validator accepts — the overwhelming majority of such graphs are rejected +//! for a missing trigger, an unreachable node, or an unbounded cycle. A +//! generator built that way spends its whole budget proving that +//! [`tinyflows::validate`] says no, and never reaches the engine, which is the +//! part under test. +//! +//! So graphs are built **compositionally** from [`Shape`]s that are correct by +//! construction. Every shape has exactly one entry node and one exit node, so +//! shapes nest and chain without any shape needing to know its context. What +//! gets randomised is the *structure* — nesting, arity, branch depth — not the +//! wiring. +//! +//! # The oracle problem +//! +//! A property test needs to know what the right answer is. Rather than predict +//! outputs, the leaves here are **pure control-flow nodes** (`output_parser` +//! passthroughs and `transform`s over constant values), so no capability is +//! consulted and the whole run is a deterministic function of the graph. That +//! turns "is the output correct?" — which needs a second implementation to +//! answer — into the properties actually worth asserting: it terminates, it is +//! deterministic, it survives a resume. Those hold regardless of what the +//! output *is*. + +use proptest::prelude::*; +use serde_json::{Value, json}; + +use tinyflows::model::{Edge, Node, NodeKind, WorkflowGraph}; + +/// A structural template for part of a graph. +/// +/// Every variant compiles to a subgraph with a single entry and a single exit, +/// which is the invariant that lets them nest and chain freely. +#[derive(Debug, Clone, PartialEq)] +pub enum Shape { + /// `n` passthrough nodes in a row. `n == 0` is a single node, so a linear + /// shape always has something to be an entry and an exit. + Linear(usize), + /// A `condition` whose `true`/`false` ports run different shapes and rejoin + /// at a `merge`. Exercises conditional routing and the barrier relief that + /// keeps an untaken branch from deadlocking the join. + Branch(Box, Box), + /// Two or more shapes fanned out from one port and rejoined at a `merge`. + /// This is the parallel case: every branch runs in the same super-step. + Fanout(Vec), + /// A bounded `loop` whose `body` runs a shape and returns to the head. + Loop { + /// The loop's `max_iterations` cap. + max_iter: u64, + /// What runs on each pass. + body: Box, + }, + /// `n` tasks spawned without blocking, collected by a gate on `release`. + /// + /// The async pair. Present so generated graphs exercise the poll loop and + /// the release policies against structures nobody hand-picked — a gate + /// downstream of a branch, inside a loop body, beside a fan-out. + Spawned { + /// How many tasks to spawn. + tasks: usize, + /// The gate's release policy, as its config `release` value. + release: &'static str, + /// `n` for `first_n`/`quorum`; ignored otherwise. + n: usize, + }, + /// A sub-workflow running a nested shape. + /// + /// Depth-bounded by the generator itself rather than by the engine's cap, + /// so a generated graph never fails merely for nesting too deep. + Nested(Box), + /// A node that pauses the run awaiting approval, followed by a shape. + /// + /// Present so generated runs actually *suspend*, which is the only way to + /// exercise the checkpoint/resume path. A shape containing one of these + /// cannot complete without either a pre-approval on the run input or a + /// resume that names its gate. + Gate(Box), +} + +impl Shape { + /// An upper bound on the super-steps a run of this shape can take. + /// + /// Used to give a generated graph a `recursion_limit` that is generous + /// enough never to be the reason a run fails, so a run that *does* hit a + /// bound has found something real. Deliberately an over-estimate. + fn step_budget(&self) -> u64 { + match self { + Self::Linear(n) => *n as u64 + 1, + Self::Branch(a, b) => 2 + a.step_budget().max(b.step_budget()), + Self::Fanout(branches) => 2 + branches.iter().map(Self::step_budget).max().unwrap_or(0), + // Each pass costs the body plus the head itself. + Self::Loop { max_iter, body } => (max_iter + 1) * (body.step_budget() + 1), + Self::Gate(rest) => 1 + rest.step_budget(), + // A gate may poll several times before its tasks settle, and every + // poll is a super-step. + Self::Spawned { tasks, .. } => 2 + (*tasks as u64) + 8, + // The child runs inside one activation of the parent, so it costs + // the parent a single step regardless of its own size. + Self::Nested(_) => 2, + } + } + + /// The ids of every approval gate this shape will contain, in the order + /// [`Builder`] hands ids out. + /// + /// A caller needs these up front to pre-approve a run or to resume one, and + /// recomputing them by walking the built graph would just be re-deriving + /// what the builder already knew. + #[must_use] + pub fn gate_ids(&self) -> Vec { + let mut builder = Builder::new(); + builder.build(self); + builder.gates + } +} + +/// Builds the nodes and edges of a graph, handing out unique ids as it goes. +struct Builder { + nodes: Vec, + edges: Vec, + next_id: usize, + /// Ids of the approval gates emitted, in creation order. + gates: Vec, +} + +/// The entry and exit node ids of a built subgraph. +struct Span { + entry: String, + exit: String, +} + +impl Builder { + fn new() -> Self { + Self { + nodes: Vec::new(), + edges: Vec::new(), + next_id: 0, + gates: Vec::new(), + } + } + + /// Adds a node with a fresh id and returns that id. + fn add(&mut self, kind: NodeKind, config: Value) -> String { + let id = format!("n{}", self.next_id); + self.next_id += 1; + self.nodes.push(Node { + id: id.clone(), + kind, + type_version: 1, + name: id.clone(), + config, + ports: vec![], + position: None, + }); + id + } + + /// Adds a passthrough node — the neutral filler this generator builds + /// linear runs from. `output_parser` with no config emits its input + /// unchanged and consults no capability. + fn passthrough(&mut self) -> String { + self.add(NodeKind::OutputParser, Value::Null) + } + + fn connect(&mut self, from: &str, port: &str, to: &str) { + self.edges.push(Edge { + from_node: from.to_string(), + from_port: port.to_string(), + to_node: to.to_string(), + to_port: "main".to_string(), + }); + } + + /// Emits `shape` and returns its entry/exit ids. + fn build(&mut self, shape: &Shape) -> Span { + match shape { + Shape::Linear(n) => { + let entry = self.passthrough(); + let mut exit = entry.clone(); + for _ in 0..*n { + let next = self.passthrough(); + self.connect(&exit, "main", &next); + exit = next; + } + Span { entry, exit } + } + + Shape::Branch(yes, no) => { + // The field is absent from the items flowing through, so it + // resolves falsey and the `false` arm is the one taken. That is + // deliberate: it means every run exercises barrier relief on + // the join, which is the interesting path. + let head = self.add(NodeKind::Condition, json!({ "field": "take_yes" })); + let join = self.add(NodeKind::Merge, Value::Null); + for (port, arm) in [("true", yes), ("false", no)] { + let span = self.build(arm); + self.connect(&head, port, &span.entry); + self.connect(&span.exit, "main", &join); + } + Span { + entry: head, + exit: join, + } + } + + Shape::Fanout(branches) => { + let apex = self.passthrough(); + let join = self.add(NodeKind::Merge, Value::Null); + for (index, branch) in branches.iter().enumerate() { + // A `transform` at the head of each branch stamps which + // branch the items came down, so a determinism failure + // names the branch that diverged rather than just the run. + let tag = self.add(NodeKind::Transform, json!({ "set": { "branch": index } })); + let span = self.build(branch); + // Every branch leaves the apex on the *same* port, which is + // what the engine reads as a parallel fan-out rather than a + // conditional choice. + self.connect(&apex, "main", &tag); + self.connect(&tag, "main", &span.entry); + self.connect(&span.exit, "main", &join); + } + Span { + entry: apex, + exit: join, + } + } + + Shape::Loop { max_iter, body } => { + // `on_exceeded: continue` so exhausting the cap is a normal + // exit through `done` rather than an error. A generated graph + // should only fail when something is actually wrong. + let head = self.add( + NodeKind::Loop, + json!({ "max_iterations": max_iter, "on_exceeded": "continue" }), + ); + let span = self.build(body); + self.connect(&head, "body", &span.entry); + self.connect(&span.exit, "main", &head); // the back-edge + let out = self.passthrough(); + self.connect(&head, "done", &out); + Span { + entry: head, + exit: out, + } + } + + Shape::Spawned { tasks, release, n } => { + let apex = self.passthrough(); + let mut sources = Vec::new(); + for index in 0..*tasks { + let spawn = self.add( + NodeKind::Spawn, + json!({ "target": "tool", "slug": format!("gen.task{index}") }), + ); + self.connect(&apex, "main", &spawn); + sources.push(spawn); + } + let mut config = json!({ + "from": sources, + "release": release, + // Poll fast: these runs are in-process and the interval is + // pure latency in a test. + "poll_interval_ms": 1, + // Settle for what arrived rather than failing, so a release + // policy that cannot be met is not reported as a bug. + "on_timeout": "partial", + }); + if matches!(*release, "first_n" | "quorum") { + config["n"] = json!((*n).clamp(1, (*tasks).max(1))); + } + let gate = self.add(NodeKind::Gate, config); + for source in &sources { + self.connect(source, "main", &gate); + } + Span { + entry: apex, + exit: gate, + } + } + + Shape::Nested(inner) => { + // The child is a complete graph in its own right, built by a + // fresh builder so its ids live in their own space — exactly the + // separation that makes a child's gate ids need namespacing. + let child = graph_of(inner); + let node = self.add( + NodeKind::SubWorkflow, + json!({ "workflow": serde_json::to_value(&child).expect("child graph") }), + ); + Span { + entry: node.clone(), + exit: node, + } + } + + Shape::Gate(rest) => { + let gate = self.add(NodeKind::OutputParser, json!({ "requires_approval": true })); + self.gates.push(gate.clone()); + let span = self.build(rest); + self.connect(&gate, "main", &span.entry); + Span { + entry: gate, + exit: span.exit, + } + } + } + } +} + +/// Compiles a [`Shape`] into a runnable [`WorkflowGraph`]. +/// +/// The graph gets a trigger wired to the shape's entry, and a `recursion_limit` +/// derived from the shape rather than left to the default — a generated graph +/// that legitimately needs many super-steps should not be failed by a budget +/// that has nothing to do with what is being tested. +#[must_use] +pub fn graph_of(shape: &Shape) -> WorkflowGraph { + let mut builder = Builder::new(); + let span = builder.build(shape); + let trigger = Node { + id: "trigger".to_string(), + kind: NodeKind::Trigger, + type_version: 1, + name: "trigger".to_string(), + config: json!({ "recursion_limit": shape.step_budget() + 8 }), + ports: vec![], + position: None, + }; + builder.edges.push(Edge { + from_node: "trigger".to_string(), + from_port: "main".to_string(), + to_node: span.entry, + to_port: "main".to_string(), + }); + let mut nodes = vec![trigger]; + nodes.extend(builder.nodes); + WorkflowGraph { + name: "generated".to_string(), + nodes, + edges: builder.edges, + ..Default::default() + } +} + +/// A strategy for shapes, bounded by nesting `depth`. +/// +/// Arities are kept small (branch fan-out of 2–3, loops of 1–3 passes) on +/// purpose: property tests find bugs through *structural variety*, not through +/// size, and small counterexamples are the ones a human can read once proptest +/// has shrunk them. +pub fn arb_shape(depth: u32) -> impl Strategy { + let leaf = prop_oneof![ + (0usize..3).prop_map(Shape::Linear), + // A leaf that spawns: it needs no nesting to be interesting, and putting + // it here means the async pair turns up at every depth rather than only + // near the root. + (1usize..4, 1usize..4).prop_map(|(tasks, n)| Shape::Spawned { + tasks, + release: "all", + n, + }), + ]; + leaf.prop_recursive(depth, 32, 4, |inner| { + prop_oneof![ + (inner.clone(), inner.clone()) + .prop_map(|(a, b)| Shape::Branch(Box::new(a), Box::new(b))), + prop::collection::vec(inner.clone(), 2..4).prop_map(Shape::Fanout), + (1u64..4, inner.clone()).prop_map(|(max_iter, body)| Shape::Loop { + max_iter, + body: Box::new(body) + }), + inner.prop_map(|child| Shape::Nested(Box::new(child))), + ] + }) +} + +/// A strategy over every release policy, for shapes whose point is the gate. +/// +/// Kept separate from [`arb_shape`], which pins `release: "all"`: a property +/// about *what a run computes* wants the policy that always collects +/// everything, while a property about *the policies themselves* wants the +/// variety. Mixing them would make the first kind of property +/// nondeterministic for uninteresting reasons. +pub fn arb_spawned_shape() -> impl Strategy { + (1usize..5, 0usize..5, 1usize..4).prop_map(|(tasks, policy, n)| Shape::Spawned { + tasks, + release: ["all", "any", "first_n", "quorum", "timeout_partial"][policy], + n, + }) +} + +/// A strategy for whole graphs, at the default nesting depth. +pub fn arb_workflow_graph() -> impl Strategy { + arb_shape(3).prop_map(|shape| graph_of(&shape)) +} + +/// Like [`arb_shape`], but every generated shape contains **at least one** +/// approval gate, so every run it produces suspends. +/// +/// Kept separate from [`arb_shape`] rather than folded in as one more variant: +/// a strategy that only *sometimes* produces a gate would spend most of its +/// cases not testing suspension at all, and the properties about running to +/// completion want graphs that reliably do not pause. +pub fn arb_gated_shape(depth: u32) -> impl Strategy { + arb_shape(depth).prop_map(|inner| Shape::Gate(Box::new(inner))) +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs new file mode 100644 index 00000000..afabef5b --- /dev/null +++ b/tests/support/mod.rs @@ -0,0 +1,9 @@ +//! Shared helpers for the generative (property-based) test suite. +//! +//! Cargo compiles every `tests/*.rs` as its own crate, so anything shared has +//! to live in a module each of them declares with `mod support;`. Only the +//! files that use it pay for it. + +#![allow(dead_code)] + +pub mod graphgen;