fix(engine): thread parent cancellation token into sub_workflow children - #31
Conversation
A sub_workflow node ran its child via run_sub_workflow, which always started the child behind a freshly-constructed CancellationToken. Cancelling the parent run therefore never crossed the sub_workflow boundary: the child kept scheduling node work while the parent wound down, and on the child's completion the node reported the child's cancelled outcome as a hard error, falsely failing a run the operator merely stopped. Carry the run's token to executors on NodeContext (an owned clone), forward it through run_sub_workflow into the child's build_and_run, and hand it to the child's own node contexts so a nested sub_workflow propagates it transitively. When a child winds down under the parent's own token, run_child now emits no output (mirroring the top-level cancelled-node contract) and lets the parent settle as cancelled; a child that reports cancelled without the parent's token being set still errors defensively.
Drive a real run_cancellable over a parent -> sub_workflow(child) graph and prove the parent's token reaches the child: cancelling mid-flight skips the child's downstream node (T1), propagates through two nesting levels (T2), leaves an uncancelled run untouched (T3), short-circuits the sub_workflow node when the token is pre-cancelled so the child never starts (T4), and keeps the defensive independent-cancel error arm (T5). The cancel is made deterministic under parallel load by holding the in-flight node open until the token flips rather than racing a wall-clock sleep.
Cancellation now propagates into sub_workflow children (previous commits); publish it as 0.6.1 so the OpenCompany host picks it up within its "0.6" req.
|
@graycyrus this is the tinyflows-side fix for the sub_workflow cancellation propagation (downstream: tinyhumansai/opencompany#675). Requesting your review — couldn't add you via the reviewer field (no write access from the fork). Note the open API-shape question in the description re: bare |
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds cancellation tokens to ChangesNested workflow cancellation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeContext
participant Engine
participant ChildWorkflow
participant ChildNode
NodeContext->>Engine: provide parent CancellationToken
Engine->>ChildWorkflow: forward token to nested execution
ChildWorkflow->>ChildNode: execute with cloned token
ChildNode-->>ChildWorkflow: cancellation or result
ChildWorkflow-->>Engine: empty output or error
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nodes/mod.rs`:
- Around line 42-50: Make the new public NodeContext token field
backward-compatible by avoiding a required field in downstream struct literals,
or update the crate version to 0.7.0 to declare the breaking API change.
Preserve the owned CancellationToken behavior required by nested workflow
execution and cancellation propagation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef4d8a91-bbf2-41de-afe7-fc08ba2c5a0a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlsrc/engine.rssrc/nodes/control_flow/condition.rssrc/nodes/control_flow/dedup.rssrc/nodes/control_flow/loop_node.rssrc/nodes/control_flow/merge.rssrc/nodes/control_flow/split_out.rssrc/nodes/control_flow/switch.rssrc/nodes/control_flow/transform.rssrc/nodes/integration/agent.rssrc/nodes/integration/code.rssrc/nodes/integration/http_request.rssrc/nodes/integration/memory.rssrc/nodes/integration/output_parser.rssrc/nodes/integration/sub_workflow.rssrc/nodes/integration/tool_call.rssrc/nodes/mod.rs
| /// The run's cooperative-cancellation token (see | ||
| /// [`crate::engine::CancellationToken`]). An **owned clone** of the run | ||
| /// token, not a borrow — an executor that spawns nested engine work (today | ||
| /// only [`sub_workflow`](crate::nodes::integration)) must thread a clone | ||
| /// into that child run, so a parent cancel winds the whole subtree down at | ||
| /// the next node boundary instead of orphaning it. Executors that touch the | ||
| /// 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, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the module and type are exported by the crate.
rg -nP '^\s*pub(\([^)]*\))?\s+mod\s+nodes\b|^\s*pub\s+use\s+.*\bnodes\b' src Cargo.toml || true
rg -n -C 3 'pub struct NodeContext|NodeContext\s*\{' src
# Confirm the planned release version.
rg -nP '^\s*version\s*=' Cargo.tomlRepository: tinyhumansai/tinyflows
Length of output: 26134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
lib = Path("src/lib.rs").read_text()
nodes = Path("src/nodes/mod.rs").read_text()
cargo = Path("Cargo.toml").read_text()
print("pub_mod_nodes:", bool(re.search(r"(?m)^\s*pub\s+mod\s+nodes\s*;", lib)))
m = re.search(r"(?s)pub struct NodeContext<'a>\s*\{(.*?)\n\}", nodes)
if not m:
raise SystemExit("NodeContext definition not found")
fields = re.findall(r"(?m)^\s*pub\s+(\w+)\s*:", m.group(1))
print("NodeContext_public_fields:", fields)
print("package_version:", re.search(r'(?m)^\s*version\s*=\s*"([^"]+)"', cargo).group(1))
PY
git diff --unified=3 -- src/nodes/mod.rs Cargo.tomlRepository: tinyhumansai/tinyflows
Length of output: 281
Use a compatible API design or release as 0.7.0.
NodeContext is public through crate::nodes, and the required token field breaks downstream struct literals. Version 0.6.1 cannot contain this API break.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nodes/mod.rs` around lines 42 - 50, Make the new public NodeContext token
field backward-compatible by avoiding a required field in downstream struct
literals, or update the crate version to 0.7.0 to declare the breaking API
change. Preserve the owned CancellationToken behavior required by nested
workflow execution and cancellation propagation.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0873 · 104,618 in / 23,426 out · 74,374 cached (71%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
critique: $0.0429 · 41,664 in / 13,751 out · 32,213 cached (77%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
security: $0.0269 · 34,227 in / 5,978 out · 21,285 cached (62%) · z-ai/glm-5.2
tests: $0.0080 · 13,345 in / 1,593 out · 9,542 cached (72%) · z-ai/glm-5.2
description: $0.0080 · 13,891 in / 1,580 out · 10,190 cached (73%) · z-ai/glm-5.2
| #[test] | ||
| fn t5_defensive_independent_cancel_arm_is_present() { | ||
| let src = include_str!("sub_workflow.rs"); | ||
| assert!( |
There was a problem hiding this comment.
T5 source-string guardrail is tautological and cannot detect deletion of the arm
The T5 test uses include_str!("sub_workflow.rs") to assert that the production source still contains the defensive cancel arm, but include_str! pulls in the entire file — including the test module itself. The string "if ctx.token.is_cancelled() {" appears verbatim inside the test's own assert! call, so src.contains(...) is trivially true even if the production arm is deleted. The stated guardrail ("a future refactor cannot delete it") does not actually fire: a refactor that removes the run_child arm while leaving the test untouched would still pass, which is exactly the silent false-completion the test claims to prevent.
[RULE] unchanged ·
| run: &Value::Null, | ||
| nodes: &nodes, | ||
| caps: &caps, | ||
| token: crate::engine::CancellationToken::new(), |
There was a problem hiding this comment.
Propagate the parent cancellation token instead of creating a new one in the l
The diff adds token: crate::engine::CancellationToken::new() to the execution context built inside the loop node's run method. CancellationToken::new() creates a token in the non-cancelled state; it has no link to any parent token that an engine or caller might use to cancel the workflow. Because this is a loop node, the body (or sub-nodes) executed with this context will be effectively uncancellable from the outside. If the loop condition or iteration source is influenced by workflow data or external input, a caller cannot interrupt a runaway loop, creating a denial-of-service path. The loop node should propagate the cancellation token it received from its own execution context rather than minting a new one.
[RULE] Keep the workflow model declarative; no arbitrary embedded scripting—code execution is a sandboxed capability. ·
What this change touches18 files, +466 -7 across 5 components. It reaches 6 untouched components (60 graph nodes walked). flowchart LR
n0["src/nodes/integration<br/>8 files +422 -5<br/>1 finding"]:::flagged
n1["src<br/>1 file +16 -1"]:::changed
n2["src/nodes<br/>1 file +14 -0"]:::changed
n3["src/nodes/control_flow<br/>7 files +13 -0"]:::changed
n4["root<br/>1 file +1 -1"]:::changed
n5["src<br/>3 files reached"]:::impacted
n6["src/caps<br/>2 files reached"]:::impacted
n7["extension/tests/e2e<br/>1 file reached"]:::impacted
n8["src/model<br/>1 file reached"]:::impacted
n9["src/nodes/control_flow<br/>1 file reached"]:::impacted
n10["src/nodes/integration<br/>1 file reached"]:::impacted
n10 -->|34 refs| n5
n9 -->|20 refs| n5
n10 -->|18 refs| n2
n10 -->|18 refs| n6
n9 -->|16 refs| n2
n9 -->|10 refs| n6
n10 -->|9 refs| n8
n10 -->|8 refs| n7
n9 -->|7 refs| n7
n9 -->|7 refs| n8
n2 -->|4 refs| n5
n5 -->|4 refs| n2
n5 -->|4 refs| n6
n2 -->|3 refs| n6
n5 -->|3 refs| n8
n6 -->|3 refs| n5
n2 -->|2 refs| n8
n2 -->|2 refs| n10
n2 -->|1 ref| n7
n2 -->|1 ref| n9
n6 -->|1 ref| n8
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p2
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nodes/integration/shell_tests.rs (1)
110-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the
ShellScriptvariant in the path test.At Line 116, the test checks only the echoed string.
MockShellinsrc/caps/mock.rs:93-109emits the samestdoutforShellScript::InlineandShellScript::Path. The test can pass ifscript_pathis incorrectly sent as inline content. Use a runner that matchesShellScript::Pathand verifies"scripts/build.sh"before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nodes/integration/shell_tests.rs` around lines 110 - 117, Update a_script_path_reaches_the_host_verbatim_for_validation to use a runner that matches the received ShellScript::Path variant, verifies its value is "scripts/build.sh", and only then returns the expected output. Do not rely solely on stdout, since MockShell emits identical output for ShellScript::Inline and ShellScript::Path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/nodes/integration/shell_tests.rs`:
- Around line 110-117: Update
a_script_path_reaches_the_host_verbatim_for_validation to use a runner that
matches the received ShellScript::Path variant, verifies its value is
"scripts/build.sh", and only then returns the expected output. Do not rely
solely on stdout, since MockShell emits identical output for ShellScript::Inline
and ShellScript::Path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 061c2a6b-bad5-40bc-a510-5c5952747f3e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/nodes/integration/shell_tests.rssrc/nodes/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/nodes/mod.rs
The include_str! check matched its own assertion strings, so deleting the run_child arm would not have failed the test. Slice at the test module.
|
Thanks — addressed the review, and one open call for @graycyrus as crate owner. CI: the Rust SDK failure was a merge-with-main break — main's new shell node (#28) added a T5 (tinysweeper — tautological guardrail): valid, fixed in loop_node.rs:183 (tinysweeper — propagate the parent token): false positive. That line is inside Semver (CodeRabbit — 0.6.1 vs 0.7.0): your call. |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0903 · 59,619 in / 30,498 out · 43,733 cached (73%) · z-ai/glm-5.2
critique: $0.0262 · 19,472 in / 8,492 out · 14,013 cached (72%) · z-ai/glm-5.2
security: $0.0224 · 11,965 in / 7,886 out · 8,649 cached (72%) · z-ai/glm-5.2
tests: $0.0295 · 13,823 in / 10,627 out · 9,894 cached (72%) · z-ai/glm-5.2
description: $0.0122 · 14,359 in / 3,493 out · 11,177 cached (78%) · z-ai/glm-5.2
| .split("mod tests") | ||
| .next() | ||
| .expect("source file has a body before its test module"); | ||
| assert!( |
There was a problem hiding this comment.
T5 guardrail cannot detect deletion of the run_child wind-down arm
The T5 test claims to guard both the parent-cancel wind-down arm and the defensive error arm in run_child, but the string check for the wind-down arm is satisfied by a different copy of the same guard in execute, so deleting the run_child guard would not fail the test.
The test checks:
production.contains("if ctx.token.is_cancelled() {")
&& production.contains("run is halted rather than falsely completed"),The string "if ctx.token.is_cancelled() {" appears in the production region (before mod tests) in two places: once inside execute at the post-try_join_all check:
if ctx.token.is_cancelled() {
return Ok(NodeOutput::empty());
}and again inside run_child at the wind-down check:
if ctx.token.is_cancelled() {
tracing::debug!(
node = %ctx.node.id,
"sub_workflow: child wound down under the parent's cancellation; emitting no output"
);
return Ok(None);
}If a future refactor deletes the run_child copy (and its return Ok(None)), the contains check still passes because the execute copy remains. The second half of the assertion ("run is halted rather than falsely completed") is specific to the error arm, but since the two halves are ANDed and the first half is always true regardless of whether the run_child guard exists, the test cannot detect deletion of the wind-down arm it claims to protect.
This was raised as a prior finding and is not fixed in the current diff; the test was added in this PR, so it is this author's concern.
[RULE] Record design decisions in local/docs/11-decisions.md. ·
2 participants
The bug
Cancelling a workflow run doesn't stop a running
sub_workflowchild.run_sub_workflowalways built the child behind a freshly-constructedCancellationToken, so operator cancellation never crossed the sub_workflow boundary — the child kept scheduling node work while the parent wound down, and effectful nodes could run after the operator hit Cancel. Worse, when the child eventually finished it reported its own cancelled outcome as a hard error, falsely failing a run the operator merely stopped.The fix
NodeContextgains an ownedtoken(a clone of the run's cancellation token), handed to executors.run_sub_workflowforwards the parent token through into the child'sbuild_and_run, and the child's own node contexts receive it — so a nestedsub_workflowinherits the token transitively at any depth.run_childnow emits no output (mirroring the top-level cancelled-node contract) and lets the parent settle as cancelled. A child that reports cancelled without the parent's token being set still errors defensively — so a genuine child-internal failure is not masked.Tests (T1–T5) + genuineness
Added
test(engine)covering the cross-boundary matrix (T1–T5). T1 and T2 fail on the pre-fix engine and pass after — they prove the propagation actually crosses the boundary rather than just exercising the happy path. Transitive depth inheritance is covered.Honest caveat
Cancellation is boundary-only: settle is bounded by the in-flight node's remainder — an already-running node completes before the child winds down. This is exact parity with how top-level run cancellation already behaves; there is no mid-node preemption here and none is claimed.
Open API-shape question (maintainer's call)
The fix adds a bare
tokenfield toNodeContext. If you'd prefer aRunControls { token, … }struct instead, so #617's later interrupt channel can extend it without re-churning the ~40 literalNodeContext { … }construction sites, that's an easy swap — your call. I kept it a bare field to stay minimal, but I'm happy to wrap it.Related
Downstream consumer:
tinyhumansai/opencompany#675(this crate is vendored there via openhuman). The opencompany-side regression test is the end-to-end proof that cancellation now propagates through the vendored engine.Summary by CodeRabbit