-
Notifications
You must be signed in to change notification settings - Fork 0
fix: workflow incorrectly marked as completed while nodes are still executing #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devin_pr_code_review_bench_100_devin2_base_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr441
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Recursively propagate skip | ||
| self.propagate_skip_from_edge(edge.id) | ||
| self._state_manager.mark_edge_skipped(edge.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Skip propagation fails due to incorrect order of operations - edge marked as skipped AFTER propagation
The _propagate_skip_to_node method calls propagate_skip_from_edge(edge.id) before mark_edge_skipped(edge.id), which breaks skip propagation.
Click to expand
Root Cause
In the changed code at lines 84-86, the order of operations was swapped:
for edge in outgoing_edges:
# Recursively propagate skip
self.propagate_skip_from_edge(edge.id) # Called first
self._state_manager.mark_edge_skipped(edge.id) # Called secondHowever, propagate_skip_from_edge at line 53 calls analyze_edge_states(incoming_edges) which checks the state of each incoming edge. Since edges default to NodeState.UNKNOWN (see api/core/workflow/graph/edge.py:15), the edge being processed is still UNKNOWN when propagate_skip_from_edge is called.
Actual vs Expected Behavior
- Actual:
analyze_edge_statesreturnshas_unknown=Truebecause the edge hasn't been marked as SKIPPED yet, causingpropagate_skip_from_edgeto return early at line 56-57 without propagating the skip. - Expected: The edge should be marked as SKIPPED first, so
analyze_edge_statescorrectly identifies all edges as skipped and propagation continues.
Impact
Skip propagation will fail to propagate through the graph correctly. Downstream nodes that should be skipped will not be marked as skipped, potentially causing:
- Nodes executing when they shouldn't
- Workflow completing incorrectly
- The very issue this PR claims to fix (workflow marked as completed while nodes are still executing) may actually be made worse
Recommendation: Restore the original order - call mark_edge_skipped(edge.id) before propagate_skip_from_edge(edge.id):
for edge in outgoing_edges:
self._state_manager.mark_edge_skipped(edge.id)
# Recursively propagate skip
self.propagate_skip_from_edge(edge.id)Was this helpful? React with 👍 or 👎 to provide feedback.
| if edge_states["has_taken"]: | ||
| # Enqueue node | ||
| # Start execution and enqueue node | ||
| print(f"Starting execution for node: {downstream_node_id}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Debug print statement left in production code
A debug print() statement was left in the production code, which violates the project's coding standards.
Click to expand
Issue
Line 62 contains:
print(f"Starting execution for node: {downstream_node_id}")According to AGENTS.md rule #3 ("Backend Code Must Use Logging Instead of Print Statements"), all output should use the logging module rather than print statements.
Impact
- Debug output will appear in production logs/stdout
- Inconsistent with the rest of the codebase which uses proper logging
- May cause issues in production environments where stdout is not expected
Recommendation: Either remove the debug statement entirely, or replace it with proper logging:
import logging
logger = logging.getLogger(__name__)
# ...
logger.debug(f"Starting execution for node: {downstream_node_id}")Was this helpful? React with 👍 or 👎 to provide feedback.
Benchmark PR from qodo-benchmark#441