fix: workflow incorrectly marked as completed while nodes are still executing#13
Conversation
🤖 Augment PR SummarySummary: Fixes a race where a workflow run could be marked completed while downstream nodes are still pending execution. Changes:
Technical Notes: Execution completion is determined by the ready-queue being empty and the executing-node set being empty; this change aims to ensure nodes made runnable during skip-propagation are reflected in that executing-node tracking. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| 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.
print(...) in the graph engine path will write to stdout during normal runs and tests; this is likely unintended and can create noisy/slow execution for large graphs. Consider using the project logger (or removing the output) so workflow runs don’t emit debug lines by default.
🤖 Was this useful? React with 👍 or 👎
| for edge in outgoing_edges: | ||
| self._state_manager.mark_edge_skipped(edge.id) | ||
| # Recursively propagate skip | ||
| self.propagate_skip_from_edge(edge.id) |
There was a problem hiding this comment.
propagate_skip_from_edge() assumes it’s being called “from a skipped edge”, but here the outgoing edge isn’t marked SKIPPED until after the recursive call. This can cause downstream nodes to see that incoming edge as UNKNOWN and stop propagation early, leaving nodes/edges unskipped.
🤖 Was this useful? React with 👍 or 👎
| mock_graph.get_outgoing_edges.return_value = [edge1, edge2] | ||
|
|
||
| # Setup get_incoming_edges to return empty list to stop recursion | ||
| mock_graph.get_incoming_edges.return_value = [] |
There was a problem hiding this comment.
In this test (and similarly in test_skip_branch_paths...), analyze_edge_states isn’t configured, so it returns a MagicMock and edge_states["has_unknown"] will be truthy, causing propagate_skip_from_edge to return immediately. That means these tests can pass without actually exercising the intended skip-propagation path.
Other Locations
api/tests/unit_tests/core/workflow/graph_engine/graph_traversal/test_skip_propagator.py:172
🤖 Was this useful? React with 👍 or 👎
1 participant
Benchmark PR from agentic-review-benchmarks#13