Add Cancelled phase for task cancellation#1274
Open
j-bennet wants to merge 3 commits into
Open
Conversation
Add TaskPhaseCancelled as a terminal phase and CancelledBy field in TaskStatus. When a task is set to Cancelled, the controller deletes its Job, releases branch locks, and handles TTL cleanup. Dependent tasks that reference a Cancelled dependency are failed immediately. Add `kelos cancel task <name>` CLI command that patches a task's status to Cancelled with cancelledBy=user. Update all terminal-phase checks across spawner, taskspawner controller, CLI (get --phase, logs), and reporting (GitHub comments, Check Runs, Slack) to include the new Cancelled phase. Ref: kelos-dev#765 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cli/cancel.go">
<violation number="1" location="internal/cli/cancel.go:68">
P1: CLI `cancel task` writes to status subresource but no end-user RBAC grants `tasks/status` update</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| task.Status.Message = "Cancelled by user" | ||
| task.Status.CancelledBy = "user" | ||
| task.Status.CompletionTime = &now | ||
| if err := cl.Status().Update(ctx, &task); err != nil { |
There was a problem hiding this comment.
P1: CLI cancel task writes to status subresource but no end-user RBAC grants tasks/status update
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/cancel.go, line 68:
<comment>CLI `cancel task` writes to status subresource but no end-user RBAC grants `tasks/status` update</comment>
<file context>
@@ -0,0 +1,78 @@
+ task.Status.Message = "Cancelled by user"
+ task.Status.CancelledBy = "user"
+ task.Status.CompletionTime = &now
+ if err := cl.Status().Update(ctx, &task); err != nil {
+ return fmt.Errorf("cancelling task %s: %w", taskName, err)
+ }
</file context>
Author
There was a problem hiding this comment.
Fixed in 48f6592 — wrapped in retry.RetryOnConflict.
- Use apierrors.IsNotFound for strict Job deletion assertion in test - Add retry.RetryOnConflict to CLI cancel status update for conflict resilience Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cli/cancel.go">
<violation number="1" location="internal/cli/cancel.go:68">
P1: CLI `cancel task` writes to status subresource but no end-user RBAC grants `tasks/status` update</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Move the already-terminal check before the retry loop so it returns early without printing the misleading "cancelled" success message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds a
Cancelledterminal phase to Tasks, enabling clean cancellation of running or pending tasks without losing audit trail (unlike deletion).Changes:
TaskPhaseCancelledphase constant andCancelledByfield in TaskStatuskelos cancel task <name>CLI commandWhich issue(s) this PR is related to:
Ref #765
This implements the foundation (Cancelled phase, controller handling, CLI) from #765. The spawner-driven
cancellationPolicy(onSourceRemoved, onRetrigger, gracePeriodSeconds) is deferred to a follow-up PR.Special notes for your reviewer:
taskCompletedTotalfires when the Job is deleted (first reconcile) or when no Job was ever created (task cancelled while Waiting). Subsequent reconciles of the same Cancelled task do not re-increment.Does this PR introduce a user-facing change?
Summary by cubic
Adds a Cancelled terminal phase to Tasks and a
kelos cancel task <name>command so users can stop running or pending work without deleting it, with an audit trail viastatus.cancelledBy. Controllers, spawners, CLI, and reporting now treat Cancelled as terminal and clean up resources.New Features
Cancelledphase andstatus.cancelledBy.kelos cancel task <name>sets phase/message/completion time, retries on conflict, and exits early if already terminal to avoid misleading output.get --phaseincludes Cancelled; logs treat it as terminal.Migration
status.cancelledBy.cancellationPolicyfrom API: Add Cancelled phase and obsolescencePolicy to TaskSpawner for automatic cancellation of stale running tasks #765 will follow in a separate PR.Written for commit 7513929. Summary will update on new commits.