Skip to content

fix: workflow incorrectly marked as completed while nodes are still executing#39

Open
tomerqodo wants to merge 4 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr441from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr441
Open

fix: workflow incorrectly marked as completed while nodes are still executing#39
tomerqodo wants to merge 4 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr441from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr441

Conversation

@tomerqodo
Copy link

Benchmark PR from qodo-benchmark#441

Comment on lines 82 to +86
outgoing_edges = self._graph.get_outgoing_edges(node_id)
for edge in outgoing_edges:
self._state_manager.mark_edge_skipped(edge.id)
# Recursively propagate skip
self.propagate_skip_from_edge(edge.id)
self._state_manager.mark_edge_skipped(edge.id)
Copy link

Choose a reason for hiding this comment

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

Bug: The reordering of propagate_skip_from_edge before mark_edge_skipped causes propagation to halt prematurely because the edge's state is still UNKNOWN when checked downstream.
Severity: HIGH

Suggested Fix

Revert the order of the calls within the for loop in _propagate_skip_to_node. Call self._state_manager.mark_edge_skipped(edge.id) before calling self.propagate_skip_from_edge(edge.id) to ensure the edge's state is updated before propagation continues.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: api/core/workflow/graph_engine/graph_traversal/skip_propagator.py#L82-L86

Potential issue: In `_propagate_skip_to_node`, the call to `propagate_skip_from_edge`
now occurs before `_state_manager.mark_edge_skipped`. When `propagate_skip_from_edge` is
called for an edge, it analyzes the states of incoming edges for the downstream node.
Because the current edge has not yet been marked as skipped, its state is `UNKNOWN`.
This causes the `analyze_edge_states` logic to return early, halting the skip
propagation prematurely. As a result, downstream nodes that should be skipped will
remain in an `UNKNOWN` state, potentially causing the workflow to be marked as complete
incorrectly.

Did we get this right? 👍 / 👎 to inform future reviews.

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