Skip to content

feat(odin): Action support in ContinuousTask - #560

Merged
vikramlc-cognite merged 5 commits into
masterfrom
EDG-714-continuous-task-action-support
Jul 27, 2026
Merged

feat(odin): Action support in ContinuousTask#560
vikramlc-cognite merged 5 commits into
masterfrom
EDG-714-continuous-task-action-support

Conversation

@vikramlc-cognite

Copy link
Copy Markdown
Contributor

Summary

Extends the Start/Stop action mechanism (previously ScheduledTask-only) to also cover ContinuousTask, 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

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Refactor (no functional change)
  • Documentation update
  • Chore / tooling / CI

What changed

  • base.py: widened five isinstance(t, ScheduledTask) gates to isinstance(t, (ScheduledTask, ContinuousTask))add_action's and add_task's reserved-name checks, _get_startup_request's Start/Stop action registration and action= 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 from ScheduledTask to ScheduledTask | ContinuousTask — no logic change, it only ever touched .name/.target.
  • Extracted the continuous-task launch out of run() into a new _launch_continuous_task(task) method. It now pre-registers the task's cancellation token in _running_task_tokens before starting the thread (previously the token was created and handed straight to the thread, never tracked), then runs the task through _run_task_with_token instead of calling task.target directly. run() now just calls this once per continuous task.
  • No changes needed to _handle_stop_task_action (already generic — looks up by name only) or to the automatic-scheduling match in add_task / the task-classification match in run() (a ContinuousTask still isn't auto-scheduled via TaskScheduler, and still launches once at boot — it's just trackable now).
  • fetch_logs and db-extractor needed no changes: fetch_logs was already task-type-agnostic, and db-extractor doesn't currently define any ContinuousTask.
  • Tests: split the one test that asserted ContinuousTask gets no actions into a StartupTask-only variant, added a test confirming ContinuousTask now gets Start/Stop actions with the correct ActionType/task fields, and added four tests in test_action_dispatch.py covering the actual runtime behavior — token registered before the thread starts (no race), Stop cancels a boot-launched instance, Start on an already-running instance reports failed, and Start after Stop successfully relaunches it.

Why it changed

  • Related issue: EDG-714
  • Related docs / discussion: follow-up to the fetch_logs result-metadata fix (fix(odin): Fix fetch logs result metadata #555 / 7.13.1) — while testing that, we found Start/Stop was scoped to ScheduledTask only, with no way to stop/restart a ContinuousTask on demand.

What to focus on during review

  • The core risk is in _launch_continuous_task: the token must be registered in _running_task_tokens before 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_action runs the task synchronously inside its own per-action dispatch thread. For a ContinuousTask (which runs indefinitely), this means the action's own status stays running until a Stop cancels 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, reporting succeeded immediately after a fire-and-forget launch.
  • Crash-then-restart interaction: _run_task_with_token's finally clears the registry entry regardless of how the target exited (including a crash, since add_task's wrapped run_task swallows exceptions internally before RESTART_POLICY decides whether to restart the whole extractor). That means a Start action can immediately relaunch a continuous task that crashed on its own — intentional, but double-check it doesn't fight with RESTART_POLICY in a way that's surprising for extractors using WHEN_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 (missing COGNITE_* env vars in the local shell), confirmed identical against the unmodified base via git 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

  • No test exercises the real 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_task method instead, which is exercised directly.
  • Haven't verified behavior when a ContinuousTask's target doesn't check its cancellation_token cooperatively — same pre-existing caveat as ScheduledTask, just now more visible since Stop is exposed for this task type too.
  • CHANGELOG.md not yet updated for this change.

Rollout and rollback

  • Purely additive to the action-dispatch surface (new isinstance branches, one extracted method) — no config or migration required.
    Existing extractors using only ScheduledTask/StartupTask are unaffected. Revert is a plain git revert of this commit if needed.

Checklist

  • Self-reviewed the diff
  • Tests added or updated (or N/A with reason)
  • Docs updated (or N/A) — CHANGELOG.md entry still outstanding
  • No secrets, credentials, or PII committed
  • Breaking changes called out above and communicated to affected teams — N/A, not a breaking change

@vikramlc-cognite vikramlc-cognite self-assigned this Jul 22, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.97%. Comparing base (936adb0) to head (18c9148).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cognite/extractorutils/unstable/core/base.py 95.65% 1 Missing ⚠️
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              
Files with missing lines Coverage Δ
cognite/extractorutils/unstable/core/base.py 85.50% <95.65%> (+0.72%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vikramlc-cognite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vikramlc-cognite
vikramlc-cognite marked this pull request as ready for review July 24, 2026 04:03
@vikramlc-cognite
vikramlc-cognite requested a review from a team as a code owner July 24, 2026 04:03
@vikramlc-cognite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cognite/extractorutils/unstable/core/base.py Outdated
@Yaseen-A-Khan

Yaseen-A-Khan commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

changes look good, just one nit question.
will the CHANGELOG.md get updated as part of this PR?

@vikramlc-cognite

Copy link
Copy Markdown
Contributor Author

changes look good, just one nit question. will the CHANGELOG.md get updated as part of this PR?

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

Yaseen-A-Khan
Yaseen-A-Khan previously approved these changes Jul 27, 2026

@Yaseen-A-Khan Yaseen-A-Khan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vikramlc-cognite vikramlc-cognite added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jul 27, 2026
@andersfylling andersfylling added risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Jul 27, 2026
@andersfylling andersfylling self-assigned this Jul 27, 2026
Comment thread cognite/extractorutils/unstable/core/base.py Outdated
@vikramlc-cognite vikramlc-cognite added waiting-for-risk-review Waiting for a member of the risk review team to take an action and removed waiting-for-team Waiting for the submitter or reviewer of the PR to take an action labels Jul 27, 2026
@andersfylling

Copy link
Copy Markdown

🦄

@andersfylling andersfylling added waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Jul 27, 2026
@vikramlc-cognite
vikramlc-cognite enabled auto-merge (squash) July 27, 2026 14:23
@vikramlc-cognite
vikramlc-cognite merged commit a97846a into master Jul 27, 2026
6 checks passed
@vikramlc-cognite
vikramlc-cognite deleted the EDG-714-continuous-task-action-support branch July 27, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants