Skip to content

execution: Add anti-cycle validation#13169

Open
rattus128 wants to merge 1 commit intoComfy-Org:masterfrom
rattus128:prs/cycle-validation
Open

execution: Add anti-cycle validation#13169
rattus128 wants to merge 1 commit intoComfy-Org:masterfrom
rattus128:prs/cycle-validation

Conversation

@rattus128
Copy link
Copy Markdown
Contributor

I fat fingered a cycle into a large flow without noticing tonight and it took ages to figure out what was wrong.

Currently if the graph contains a cycle, the just inifitiate recursions, hits a catch all then throws a generic error against the output node that seeded the validation. Instead, fail the offending cycling node chain and handling it as an error in its own right.

Example test case:

A workflow with a graph cycle.

Before:

image

After:

image

Regression Tests:

Firered template (I have none of the models downloaded): Errors look right:

image

Linux 5090, stable-cascade -> Flux2 ✅
Linux 5090, LTX2.3 FLF2V ✅

Currently if the graph contains a cycle, the just inifitiate recursions,
hits a catch all then throws a generic error against the output node
that seeded the validation. Instead, fail the offending cycling mode
chain and handlng it as an error in its own right.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 26, 2026

📝 Walkthrough

Walkthrough

The change adds cycle detection to the validate_inputs function by introducing a visiting parameter that tracks the current recursion path. When a circular dependency is detected (a node appears twice in the same traversal), the function records validation failures for all nodes in the cycle and returns immediately. The implementation uses push/pop operations on the visiting list during recursive traversal, protected by try/finally blocks to maintain stack integrity. The validation result logic is updated to merge locally detected errors with pre-recorded cycle-detection results.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'execution: Add anti-cycle validation' clearly and concisely describes the main change—adding cycle detection to prevent infinite recursion in node graphs.
Description check ✅ Passed The description clearly relates to the changeset by explaining the problem (cycles causing generic errors), the solution (detecting cycles and reporting them explicitly), and providing test evidence.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@execution.py`:
- Around line 793-810: The current order checks validated before visiting, which
causes cached cycle results (validated[unique_id]) to hide later back-edges and
fail to mark all nodes in overlapping cycles; change the logic in the function
that uses unique_id, visiting, and validated so that the visiting membership
test (if unique_id in visiting) runs before the cache lookup (if unique_id in
validated), compute and populate validated entries for all nodes in the detected
cycle as currently done, and only then return validated[unique_id]; ensure you
update references to visiting and validated in that function consistently so
overlapping cycles like B->C->B and B->D->B are both reported.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8b319c51-8869-40b2-990b-a0b4bd0c385f

📥 Commits

Reviewing files that changed from the base of the PR and between 2a1f402 and 61e29f2.

📒 Files selected for processing (1)
  • execution.py

Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants