feat: unified management stop event - #28
Conversation
|
augment review |
🤖 Augment PR SummarySummary: This PR unifies workflow cancellation by introducing a shared Changes:
Technical Notes: Cancellation is now coordinated by a single event shared across orchestration and execution components, aiming for consistent shutdown behavior and faster teardown. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| self._stop_event = stop_event | ||
| self._layers = layers if layers is not None else [] | ||
|
|
||
| def stop(self) -> None: |
There was a problem hiding this comment.
With Worker.stop() now a no-op, WorkerPool._remove_worker() (used for scale-down) can no longer actually terminate an individual worker, so “removed” workers may keep running and processing tasks outside the pool’s accounting.
🤖 Was this useful? React with 👍 or 👎
| self._thread = threading.Thread(target=self._dispatcher_loop, name="GraphDispatcher", daemon=True) | ||
| self._thread.start() | ||
|
|
||
| def stop(self) -> None: |
There was a problem hiding this comment.
Since Dispatcher.stop() no longer sets _stop_event, callers that invoke stop() without first setting the shared event may leave the dispatcher thread running (the join() just times out).
🤖 Was this useful? React with 👍 or 👎
|
|
||
| def _should_stop(self) -> bool: | ||
| """Check if execution should be stopped.""" | ||
| return self.graph_runtime_state.stop_event and self.graph_runtime_state.stop_event.is_set() |
There was a problem hiding this comment.
_should_stop() can return None when graph_runtime_state.stop_event is unset, despite being annotated -> bool; that can leak non-bool values into callers/tests.
🤖 Was this useful? React with 👍 or 👎
| node_type=self.node_type, | ||
| start_at=self._start_at, | ||
| node_run_result=NodeRunResult( | ||
| status=WorkflowNodeExecutionStatus.FAILED, |
There was a problem hiding this comment.
Cancellation emits a NodeRunFailedEvent with WorkflowNodeExecutionStatus.FAILED; since there is also a STOPPED status, marking cancelled nodes as failed may misclassify user stops in UI/metrics.
🤖 Was this useful? React with 👍 or 👎
| assert not answer_node._should_stop() | ||
|
|
||
| # Set the stop_event | ||
| runtime_state.stop_event.set() |
There was a problem hiding this comment.
In this test, GraphRuntimeState.stop_event defaults to None, so runtime_state.stop_event.set() will raise; this also applies to the other runtime_state.stop_event.set() usages in this file.
🤖 Was this useful? React with 👍 or 👎
2 participants
Benchmark PR from qodo-benchmark#171