Skip to content

fix(workflow-block): remove process specific circular dependency check#1293

Merged
icecrasher321 merged 2 commits intostagingfrom
fix/cyclic-dep-workflow-block
Sep 9, 2025
Merged

fix(workflow-block): remove process specific circular dependency check#1293
icecrasher321 merged 2 commits intostagingfrom
fix/cyclic-dep-workflow-block

Conversation

@icecrasher321
Copy link
Copy Markdown
Collaborator

Summary

Process specific execution id stack doesn't make sense, and leads to arbitrary failures.

Type of Change

  • Bug fix

Testing

No functional change

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 9, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
sim Ready Ready Preview Comment Sep 9, 2025 8:02pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Sep 9, 2025 8:02pm

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR removes process-specific circular dependency detection from the workflow block handler to eliminate arbitrary execution failures. The change removes a static executionStack Set that tracked composite execution IDs (format: ${context.workflowId}_sub_${workflowId}_${block.id}) to prevent cyclic workflow dependencies.

The previous implementation was causing false positives where legitimate parallel or sequential workflow executions were incorrectly flagged as cycles. The execution ID format was too restrictive and didn't account for valid scenarios where the same child workflow might be called multiple times within different execution contexts or parallel branches.

The system now relies exclusively on the existing MAX_WORKFLOW_DEPTH limit (10 levels) and execution timeouts as protection against infinite recursion. This change simplifies the codebase by removing complex cycle detection logic while maintaining essential safeguards through depth-based protection that works more reliably in concurrent execution environments.

The corresponding test for cyclic dependency detection has been removed from the test suite, with comments indicating the shift to relying on depth limits and timeout mechanisms. The depth limit test remains to verify the alternative protection mechanism still functions correctly.

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it removes problematic logic causing false positives
  • Score reflects that while explicit cycle detection is removed, robust depth limits and timeouts provide adequate protection
  • Pay close attention to workflow-handler.ts to ensure the depth limit mechanism is sufficient for preventing infinite recursion

2 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

Comment thread apps/sim/executor/handlers/workflow/workflow-handler.ts Outdated
@vercel vercel Bot temporarily deployed to Preview – docs September 9, 2025 19:44 Inactive
@icecrasher321 icecrasher321 merged commit a5c224e into staging Sep 9, 2025
5 of 6 checks passed
arenadeveloper02 pushed a commit to arenadeveloper02/p2-sim that referenced this pull request Sep 19, 2025
simstudioai#1293)

* fix(workflow-block): remove process specific circular dep check

* remove comments
@waleedlatif1 waleedlatif1 deleted the fix/cyclic-dep-workflow-block branch September 25, 2025 22:40
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.

1 participant