fix(odin): Update stop action status to succeeded instead of canceled - #571
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the stop task action handler to report a status of ActionStatus.succeeded instead of ActionStatus.canceled when stopping a task. The corresponding unit tests have been updated to assert this new behavior. There are no review comments, and I have no additional feedback to provide.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #571 +/- ##
==========================================
+ Coverage 83.97% 84.01% +0.04%
==========================================
Files 46 46
Lines 4686 4686
==========================================
+ Hits 3935 3937 +2
+ Misses 751 749 -2
🚀 New features to boost your workflow:
|
|
I don't quite understand this? If I cancelled a task and I would expect it to show up as cancelled? |
In this system, a Task (e.g. "Task1") is the actual running job - but a Task itself doesn't carry a succeeded/failed/canceled status. Only Actions do. When you click "Stop" on Task1, that's not a status change on Task1 directly - it dispatches a brand-new, separate Action called "Stop Task1" (type stop_task). Meanwhile, Task1 is actually being run by a different Action - the "Start Task1" action (type start_task), which has been sitting there this whole time, blocked executing Task1's code until it finishes or is interrupted. So when you cancel Task1, there are two separate Action records involved, each with its own independent status: "Start Task1" - the action that's actually running the task. When "Stop Task1" interrupts it, this action correctly reports canceled - because the thing it was doing (running Task1) really was cut short. This is what you are picturing when you say "I cancelled a task, I'd expect it to show cancelled" - and it's still exactly true after this PR. Nothing about this changes. "Stop Task1" - a separate, short-lived action whose only job is to signal "Start Task1" to stop. This is the one this PR touches. Before the fix, whenever "Stop Task1" successfully delivered that signal, it also reported its own status as canceled - which reads as if the stop request itself was aborted or failed, when actually it did exactly what it was supposed to do. After the fix, "Stop Task1" reports succeeded (the stop request worked), while "Start Task1" - the action that was actually running your task - still reports canceled, unaffected. So the confusion is: you're thinking of Task1's own outcome (still canceled, unchanged), but the status this PR changes belongs to a different action - the "Stop" request itself, not the thing it stopped. |
Summary
Fixes stop-task actions incorrectly reporting
Canceledinstead ofSucceededwhen they successfully stop the target task.Type of change
What changed
cognite/extractorutils/unstable/core/base.py::_handle_stop_task_action: reportActionStatus.succeeded(wasActionStatus.canceled) when the target task'sCancellationTokenis found and cancelled.tests/test_unstable/test_action_dispatch.py(test_stop_task_action_cancels_child_token_and_reports_succeeded,test_stop_action_cancels_boot_launched_continuous_task) to expectsucceeded.Why it changed
ActionStatus.canceledis meant to mean "this action was itself canceled," not "this action successfully performed a cancellation." A stop action that does its job correctly was showing up as aborted in Odin, misleading operators.What to focus on during review
succeededin a way that depended on the oldcanceledvalue.Test evidence
python -m pytest tests/test_unstable/test_action_dispatch.py -q→ 27 passed.Risks and unknowns
failed/token-not-found path unaffected.Rollout and rollback
Checklist