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

[tomerqodo]

Benchmark PR from qodo-benchmark#441

[greptile-apps]

Greptile Summary

Fixed critical bug where workflows were incorrectly marked as completed while nodes were still executing, and added comprehensive unit tests.

Key Changes:

• Added missing start_execution() call before enqueue_node() in skip_propagator.py:63 - this ensures the executing node count is properly tracked so is_execution_complete() correctly returns false while nodes are running
• Changed order in _propagate_skip_to_node() to call propagate_skip_from_edge() before mark_edge_skipped() (line 85-86) - however, this may cause the edge to still be in UNKNOWN state during recursive propagation checks
• Added comprehensive unit tests in new test file covering all skip propagation scenarios
• Introduced print() statement on line 62 which violates backend logging compliance rules

Confidence Score: 3/5

• PR fixes a critical execution tracking bug but introduces compliance violations and a potentially problematic order change
• The core fix (adding start_execution before enqueue_node) is correct and addresses a real bug where workflows terminate prematurely. However, the PR has two issues: (1) uses print() instead of logging which violates compliance rules, and (2) changes the order of edge marking in _propagate_skip_to_node which may cause edges to be in UNKNOWN state during recursive skip propagation checks, potentially breaking the skip logic
• Pay close attention to api/core/workflow/graph_engine/graph_traversal/skip_propagator.py - the order change on lines 85-86 needs verification

Important Files Changed

Filename	Overview
api/core/workflow/graph_engine/graph_traversal/skip_propagator.py	Fixed missing start_execution call after enqueue_node (critical bug fix), but introduced print statement and potentially problematic order change in _propagate_skip_to_node
api/tests/unit_tests/core/workflow/graph_engine/graph_traversal/test_skip_propagator.py	Added comprehensive unit tests for SkipPropagator covering all code paths including edge states, skip propagation, and recursive behavior

Sequence Diagram

sequenceDiagram
    participant SP as SkipPropagator
    participant SM as GraphStateManager
    participant G as Graph
    participant RQ as ReadyQueue

    Note over SP: propagate_skip_from_edge(edge_id)
    SP->>G: edges[edge_id].head
    G-->>SP: downstream_node_id
    SP->>G: get_incoming_edges(downstream_node_id)
    G-->>SP: incoming_edges[]
    
    SP->>SM: analyze_edge_states(incoming_edges)
    SM-->>SP: {has_unknown, has_taken, all_skipped}
    
    alt has_unknown == true
        Note over SP: Stop processing (node not ready)
    else has_taken == true
        Note over SP: Fix: Added start_execution before enqueue
        SP->>SM: start_execution(downstream_node_id)
        Note over SM: _executing_nodes.add(node_id)
        SP->>SM: enqueue_node(downstream_node_id)
        SM->>G: nodes[node_id].state = TAKEN
        SM->>RQ: put(node_id)
    else all_skipped == true
        SP->>SP: _propagate_skip_to_node(downstream_node_id)
        SP->>SM: mark_node_skipped(downstream_node_id)
        SM->>G: nodes[node_id].state = SKIPPED
        SP->>G: get_outgoing_edges(node_id)
        G-->>SP: outgoing_edges[]
        loop for each edge in outgoing_edges
            Note over SP: Order changed: propagate before marking
            SP->>SP: propagate_skip_from_edge(edge.id)
            SP->>SM: mark_edge_skipped(edge.id)
            SM->>G: edges[edge_id].state = SKIPPED
        end
    end

Loading

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

syntax: print() statement violates compliance rules - backend code must use logging instead of print statements.

Suggested change

	print(f"Starting execution for node: {downstream_node_id}")
	logger.debug("Starting execution for node: %s", downstream_node_id)

Context Used: Context from dashboard - AGENTS.md (source)

Prompt To Fix With AI

This is a comment left during a code review.
Path: api/core/workflow/graph_engine/graph_traversal/skip_propagator.py
Line: 62:62

Comment:
**syntax:** `print()` statement violates compliance rules - backend code must use logging instead of print statements.

```suggestion
            logger.debug("Starting execution for node: %s", downstream_node_id)
```

**Context Used:** Context from `dashboard` - AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=1028e8c2-7220-42d3-94ac-c7ba65fc7240))

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Swapping the order may cause issues - propagate_skip_from_edge checks edge states of the downstream node, but the edge hasn't been marked as skipped yet. This means the edge will still be in UNKNOWN state when analyze_edge_states is called in the recursive call, potentially causing incorrect skip propagation logic. Was this order change intentional and tested with scenarios where downstream nodes check the state of this edge?

Prompt To Fix With AI

This is a comment left during a code review.
Path: api/core/workflow/graph_engine/graph_traversal/skip_propagator.py
Line: 85:86

Comment:
**logic:** Swapping the order may cause issues - `propagate_skip_from_edge` checks edge states of the downstream node, but the edge hasn't been marked as skipped yet. This means the edge will still be in UNKNOWN state when `analyze_edge_states` is called in the recursive call, potentially causing incorrect skip propagation logic. Was this order change intentional and tested with scenarios where downstream nodes check the state of this edge?

How can I resolve this? If you propose a fix, please make it concise.