feat(odin): Action support in ContinuousTask - #560
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables ContinuousTask to be treated as an actionable task, allowing it to support auto-generated Start and Stop actions similar to ScheduledTask. It introduces a synchronous tracking mechanism (_launch_continuous_task) to prevent race conditions when starting continuous tasks, and updates the task dispatching and registration logic accordingly. Comprehensive unit tests have been added to verify the tracking, cancellation, and restarting of continuous tasks. I have no feedback to provide as there are no review comments.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #560 +/- ##
==========================================
+ Coverage 83.91% 83.97% +0.05%
==========================================
Files 46 46
Lines 4670 4686 +16
==========================================
+ Hits 3919 3935 +16
Misses 751 751
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the task management system to support start and stop actions for ContinuousTask in addition to ScheduledTask. It introduces a new _launch_continuous_task method to safely start continuous tasks in their own threads with pre-registered cancellation tokens, preventing race conditions during startup. Additionally, corresponding unit tests have been added to verify action dispatching and registration for continuous tasks. There are no review comments, and we have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the task management framework to support start and stop actions for ContinuousTasks, bringing them on par with ScheduledTasks. It updates task registration, action dispatching, and introduces a synchronous pre-registration of cancellation tokens when launching continuous tasks to prevent race conditions. Comprehensive tests have been added to verify these changes. Feedback is provided regarding a potential token leak in _launch_continuous_task if thread startup fails, along with a suggested try-except block to clean up the token on failure.
|
changes look good, just one nit question. |
Will add the changelog and update the version as part of a different PR where I need to fix the task status during extractor shutdown. Jira for the same here: https://cognitedata.atlassian.net/browse/EDG-717 |
|
🦄 |
Summary
Extends the Start/Stop action mechanism (previously
ScheduledTask-only) to also coverContinuousTask, so a continuously-running task can now be stopped and restarted on demand via Odin, the same way scheduled tasks already can.Type of change
What changed
base.py: widened fiveisinstance(t, ScheduledTask)gates toisinstance(t, (ScheduledTask, ContinuousTask))—add_action's andadd_task's reserved-name checks,_get_startup_request'sStart/Stopaction registration andaction=bool on the reported task,_dispatch_single_action's name-matching sets, and_handle_start_task_action's task lookup._run_task_with_token's type hint widened fromScheduledTasktoScheduledTask | ContinuousTask— no logic change, it only ever touched.name/.target.run()into a new_launch_continuous_task(task)method. It now pre-registers the task's cancellation token in_running_task_tokensbefore starting the thread (previously the token was created and handed straight to the thread, never tracked), then runs the task through_run_task_with_tokeninstead of callingtask.targetdirectly.run()now just calls this once per continuous task._handle_stop_task_action(already generic — looks up by name only) or to the automatic-schedulingmatchinadd_task/ the task-classificationmatchinrun()(aContinuousTaskstill isn't auto-scheduled viaTaskScheduler, and still launches once at boot — it's just trackable now).fetch_logsand db-extractor needed no changes:fetch_logswas already task-type-agnostic, and db-extractor doesn't currently define anyContinuousTask.ContinuousTaskgets no actions into aStartupTask-only variant, added a test confirmingContinuousTasknow getsStart/Stopactions with the correctActionType/taskfields, and added four tests intest_action_dispatch.pycovering the actual runtime behavior — token registered before the thread starts (no race),Stopcancels a boot-launched instance,Starton an already-running instance reportsfailed, andStartafterStopsuccessfully relaunches it.Why it changed
fetch_logsresult-metadata fix (fix(odin): Fix fetch logs result metadata #555 / 7.13.1) — while testing that, we found Start/Stop was scoped toScheduledTaskonly, with no way to stop/restart aContinuousTaskon demand.What to focus on during review
_launch_continuous_task: the token must be registered in_running_task_tokensbefore the thread starts, not inside it, or there's a race where a Stop/Start arriving immediately after boot could miss it. Worth double-checking the lock scoping there._handle_start_task_actionruns the task synchronously inside its own per-action dispatch thread. For aContinuousTask(which runs indefinitely), this means the action's own status staysrunninguntil aStopcancels it — this is a deliberate design choice (reuses the existing dispatch-thread-per-action model with no new threading), not an oversight, but worth confirming that's the semantic you want rather than, say, reportingsucceededimmediately after a fire-and-forget launch._run_task_with_token'sfinallyclears the registry entry regardless of how the target exited (including a crash, sinceadd_task's wrappedrun_taskswallows exceptions internally beforeRESTART_POLICYdecides whether to restart the whole extractor). That means aStartaction can immediately relaunch a continuous task that crashed on its own — intentional, but double-check it doesn't fight withRESTART_POLICYin a way that's surprising for extractors usingWHEN_CONTINUOUS_TASKS_CRASHES.Test evidence
poetry run pytest tests/test_unstable/test_action_dispatch.py tests/test_unstable/test_action_registration.py -q→ 43 passed, run repeatedly (3x) to rule out flakiness in the new threading-based tests.poetry run pytest tests/test_unstable/ -q→ 237 passed, 1 failed, 48 errors — the failure and errors are pre-existing (missingCOGNITE_*env vars in the local shell), confirmed identical against the unmodified base viagit stash.poetry run mypy cognite/extractorutils/unstable/core/→ no issues (13 files).poetry run ruff check cognite/extractorutils/unstable/core/→ all checks passed.Risks and unknowns
Extractor.run()entrypoint end-to-end (it also drives startup tasks and blocks on the scheduler/cancellation wait, which makes it awkward to test directly) — coverage is via the extracted_launch_continuous_taskmethod instead, which is exercised directly.ContinuousTask's target doesn't check itscancellation_tokencooperatively — same pre-existing caveat asScheduledTask, just now more visible since Stop is exposed for this task type too.Rollout and rollback
isinstancebranches, one extracted method) — no config or migration required.Existing extractors using only
ScheduledTask/StartupTaskare unaffected. Revert is a plaingit revertof this commit if needed.Checklist