Skip to content

fix(odin): Confirm cancellation for a cancel_pending action never dispatched - #578

Merged
vikramlc-cognite merged 2 commits into
masterfrom
EDG-826-cancel-pending-action-race
Sep 24, 2026
Merged

vikramlc-cognite merged 2 commits into
masterfrom
EDG-826-cancel-pending-action-race

Conversation

@vikramlc-cognite

Copy link
Copy Markdown
Contributor

Summary

The extractor now explicitly confirms cancellation when it receives a cancel_pending action it has no record of (never dispatched, or already finished), instead of silently doing nothing.

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

  • _dispatch_single_action's cancel_pending branch (unstable/core/base.py) now queues an explicit ActionUpdate(status=canceled) when no local CancellationToken is found for the action's external_id, instead of a silent no-op.
  • Renamed/rewrote test_cancel_pending_unknown_action_is_a_no_op → test_cancel_pending_unknown_action_confirms_canceled to lock in the new behavior.

Why it changed

  • Related issue: EDG-826 (companion to the odin-side fix in the same ticket)
  • Related docs / discussion: N/A

Odin is changing how it handles cancelling a pending action: instead of finalizing to canceled immediately (which gave this extractor no chance to stop work it may have already started locally), it now always moves the action to cancel_pending and waits for this extractor to acknowledge. For actions this extractor genuinely never dispatched (or already finished and cleaned up), the old silent no-op meant Odin would wait indefinitely for an ack that would only ever arrive at this integration's next startup. This change closes that gap by confirming immediately.

What to focus on during review

  • Safety of reporting canceled for an already-finished action: if the action already completed and reported a terminal status (succeeded/failed/canceled) via an earlier checkin, this late canceled ack is harmless — Odin's execute_update_triggered_actions treats any of those as terminal and drops later updates for the same action as a no-op.
  • ActionUpdate's own status validator already rejects pending/cancel_pending, so canceled is the only valid choice here — no new validation needed.

Test evidence

  • pre-commit run --all-files (ruff check, ruff format, mypy) — clean.
  • uv run pytest tests/test_unstable/test_action_dispatch.py -q — 37 passed.
  • uv run pytest tests/test_unstable/ -q — same 1 failed / 248 passed / 48 errors as on master with no changes applied (confirmed via a stashed re-run); all pre-existing, unrelated (mostly missing-credential setup errors for live-CDF integration tests), not introduced by this change.
  • Proof-of-effect: reverted base.py locally and confirmed test_cancel_pending_unknown_action_confirms_canceled fails (0 == 1 — no update queued) against the old code, passes with the fix.

Risks and unknowns

  • This is one half of a two-repo fix; it has no effect on its own until Odin's companion change (separate PR, EDG-826) ships, since today Odin never re-delivers a pending action as cancel_pending in the first place.
  • Skipped a changelog/version entry in this PR — this repo's convention (confirmed via git log, e.g. a88af30 vs the separate 8648712 "Release" commit) keeps version bumps as their own dedicated commit, not bundled with individual fixes.

Rollout and rollback

  • No config or schema changes. Extractors need to upgrade to pick up this behavior; until they do, an Odin-side cancel on an undispatched action resolves at the extractor's next startup instead of its next checkin (documented as an accepted tradeoff in the companion odin PR).
  • Rollback is a plain revert of the one code change; no state to migrate.

Checklist

  • Self-reviewed the diff
  • Tests added or updated (or N/A with reason)
  • Docs updated (or N/A) — N/A, internal dispatch behavior, no public API change
  • No secrets, credentials, or PII committed
  • Breaking changes called out above and communicated to affected teams — non-breaking; additive behavior only

…patched locally

If a cancel_pending action has no registered CancellationToken (never dispatched
by this process, or already finished), the extractor silently did nothing. Odin
would then wait on an acknowledgement that never comes until the integration's
next startup. Queue an explicit canceled ActionUpdate instead.
@vikramlc-cognite vikramlc-cognite self-assigned this Sep 10, 2026
@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 updates the action dispatching logic to immediately queue a cancellation confirmation (ActionStatus.canceled) when attempting to cancel an unknown or already finished action, preventing indefinite waiting for an acknowledgment. The corresponding test has been updated to verify this behavior. There are no review comments to address.

@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 updates the action dispatching logic to immediately confirm the cancellation of an unknown or already finished action, preventing external systems from waiting indefinitely for an acknowledgment. The corresponding test was also updated to verify this behavior. There are no review comments, and I have no feedback to provide.

@codecov

codecov Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.14%. Comparing base (196dcc1) to head (1d30bd1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
+ Coverage   84.09%   84.14%   +0.04%     
==========================================
  Files          46       46              
  Lines        4709     4710       +1     
==========================================
+ Hits         3960     3963       +3     
+ Misses        749      747       -2     
Files with missing lines Coverage Δ
cognite/extractorutils/unstable/core/base.py 86.94% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vikramlc-cognite
vikramlc-cognite marked this pull request as ready for review September 22, 2026 09:32
@vikramlc-cognite
vikramlc-cognite requested a review from a team as a code owner September 22, 2026 09:32
@vikramlc-cognite vikramlc-cognite added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Sep 23, 2026
@gaelenm

gaelenm commented Sep 24, 2026

Copy link
Copy Markdown

[risk_review] approving risk review

@gaelenm gaelenm 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 Sep 24, 2026
@gaelenm gaelenm self-assigned this Sep 24, 2026

@gaelenm gaelenm 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.

🦄

@vikramlc-cognite
vikramlc-cognite merged commit 539c57c into master Sep 24, 2026
8 checks passed
@vikramlc-cognite
vikramlc-cognite deleted the EDG-826-cancel-pending-action-race branch September 24, 2026 09:31
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