fix(odin): Confirm cancellation for a cancel_pending action never dispatched - #578
Conversation
…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.
|
/gemini review |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
|
[risk_review] approving risk review |
Summary
The extractor now explicitly confirms cancellation when it receives a
cancel_pendingaction it has no record of (never dispatched, or already finished), instead of silently doing nothing.Type of change
What changed
_dispatch_single_action'scancel_pendingbranch (unstable/core/base.py) now queues an explicitActionUpdate(status=canceled)when no localCancellationTokenis found for the action'sexternal_id, instead of a silent no-op.test_cancel_pending_unknown_action_is_a_no_op→test_cancel_pending_unknown_action_confirms_canceledto lock in the new behavior.Why it changed
Odin is changing how it handles cancelling a
pendingaction: instead of finalizing tocanceledimmediately (which gave this extractor no chance to stop work it may have already started locally), it now always moves the action tocancel_pendingand 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
canceledfor an already-finished action: if the action already completed and reported a terminal status (succeeded/failed/canceled) via an earlier checkin, this latecanceledack is harmless — Odin'sexecute_update_triggered_actionstreats any of those as terminal and drops later updates for the same action as a no-op.ActionUpdate's ownstatusvalidator already rejectspending/cancel_pending, socanceledis 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 onmasterwith 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.base.pylocally and confirmedtest_cancel_pending_unknown_action_confirms_canceledfails (0 == 1— no update queued) against the old code, passes with the fix.Risks and unknowns
pendingaction ascancel_pendingin the first place.git log, e.g.a88af30vs the separate8648712"Release" commit) keeps version bumps as their own dedicated commit, not bundled with individual fixes.Rollout and rollback
Checklist