Skip to content

introduce caib cancel#228

Merged
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-image-cancel
Apr 19, 2026
Merged

introduce caib cancel#228
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-image-cancel

Conversation

@bennyz

@bennyz bennyz commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Allow cancellation of builds

Summary

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • CI/CD improvement
  • Refactoring

Testing

  • Unit tests pass (make test)
  • Linter passes (make lint)
  • Manifests are up to date (make manifests generate)
  • Tested on OpenShift cluster (if applicable)

Summary by CodeRabbit

  • New Features

    • Added caib image cancel <build-name> to cancel in‑progress builds from the CLI.
  • Improvements

    • Cancellation now enforces requester ownership and returns clear confirmation messages.
    • Build lifecycle treats "Cancelled" as a terminal phase; logs, status timestamps, and token handling reflect cancellations consistently.
    • Delete/cancel actions now report unified success/failure feedback.
  • Tests

    • Added server- and API-level tests covering cancel success and error cases.

@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@bennyz has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 14 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 14 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0927a9a7-cea9-49b3-b04d-f22d26a8f84d

📥 Commits

Reviewing files that changed from the base of the PR and between d38acd5 and 1f710a7.

⛔ Files ignored due to path filters (1)
  • config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml is excluded by !config/crd/bases/**
📒 Files selected for processing (10)
  • api/v1alpha1/imagebuild_types.go
  • cmd/caib/buildcmd/build.go
  • cmd/caib/buildcmd/logs.go
  • cmd/caib/image/image.go
  • cmd/caib/runtime_wiring.go
  • internal/buildapi/client/client.go
  • internal/buildapi/container_builds.go
  • internal/buildapi/server.go
  • internal/buildapi/server_test.go
  • internal/controller/imagebuild/controller.go
📝 Walkthrough

Walkthrough

Adds a cancel feature across CLI, client, server, controller, and types: caib image cancel <build-name> calls a new POST /v1/builds/{name}/cancel endpoint which enforces ownership, only allows cancellation in non-terminal phases, optionally cancels an associated Tekton PipelineRun, and marks ImageBuild as Cancelled (terminal).

Changes

Cohort / File(s) Summary
CLI Command Handler
cmd/caib/buildcmd/build.go
Added exported RunCancel, refactored RunDelete to use a shared runBuildAction, replaced hardcoded phase strings with automotivev1alpha1 phase constants and introduced phaseCancelled/isTerminalPhase.
CLI Log Streaming
cmd/caib/buildcmd/logs.go
Treat cancelled builds as terminal: added explicit cancelled branch in wait logic and switched terminal checks to isTerminalPhase(...).
CLI Image Subcommand & Wiring
cmd/caib/image/image.go, cmd/caib/runtime_wiring.go
Added cancel subcommand, wired RunCancel into Options, registered --server and --token flags, and connected runtime wiring to handler.
API Client
internal/buildapi/client/client.go
Added CancelBuild(ctx,name) and refactored DeleteBuild to use shared doBuildAction; unified request construction, auth header handling, and non-200 error messages.
Server Cancel Endpoint & Logic
internal/buildapi/server.go
Added POST /v1/builds/:name/cancel; validates ownership, enforces phase gating, optionally cancels Tekton PipelineRun (patch to Cancelled), updates ImageBuild status to Cancelled with message and CompletionTime, and centralizes terminal-phase checks via isTerminalPhase.
Server Tests
internal/buildapi/server_test.go
Added cancel endpoint tests: 404 (missing), 403 (wrong owner), 409 (non-cancellable / already completed pipeline), and 200 success cases (with/without PipelineRun), asserting status updates and PipelineRun patching.
Controller Phase Handling
internal/controller/imagebuild/controller.go
Replaced literal phase strings with automotivev1alpha1 constants, added phaseCancelled, introduced isTerminalPhase predicate, and treat Cancelled as terminal in reconcile/cleanup paths.
API Types
api/v1alpha1/imagebuild_types.go
Added exported phase constants including Cancelled, added IsTerminalBuildPhase(phase string) bool, and updated kubebuilder enum to include Cancelled.
Container Build Log/Token Flow
internal/buildapi/container_builds.go
Replaced explicit completed/failed checks with isTerminalPhase(...) for log-stream loop termination and registry-token minting gating.

Sequence Diagram

sequenceDiagram
    participant User as User/CLI
    participant APIClient as API Client
    participant Server as API Server
    participant K8s as Kubernetes API
    participant Controller as ImageBuild Controller

    User->>APIClient: caib image cancel <build-name>
    APIClient->>Server: POST /v1/builds/<build-name>/cancel (Bearer token)
    Server->>K8s: Get ImageBuild <build-name>
    alt not found
        Server-->>APIClient: 404 build not found
    else found
        Server->>Server: Check requester vs requested-by annotation
        alt unauthorized
            Server-->>APIClient: 403 unauthorized
        else authorized
            Server->>Server: Check phase ∈ cancellable set
            alt not cancellable
                Server-->>APIClient: 409 cannot be cancelled
            else cancellable
                alt has PipelineRun
                    Server->>K8s: Get PipelineRun
                    alt exists and not completed
                        Server->>K8s: Patch PipelineRun.Spec.Status = "Cancelled"
                    else completed
                        Server-->>APIClient: 409 already completed
                    end
                end
                Server->>K8s: Update ImageBuild.Status -> Phase="Cancelled", CompletionTime=now
                Server-->>APIClient: 200 cancelled
                K8s->>Controller: ImageBuild updated event
                Controller->>K8s: Reconcile terminal cleanup for Cancelled
            end
        end
    end
    APIClient-->>User: Confirmation / error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • bkhizgiy

Poem

🐰 I tapped the keys and sent a gentle stop,

Pipelines hush, the busy engines drop.
Phase "Cancelled" settles, tidy and small,
Owner's word obeyed — no more build call.
A carrot cheer for graceful shutdown all!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'introduce caib cancel' clearly and concisely describes the main feature addition across the changeset: implementing build cancellation functionality in the caib CLI tool.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/controller/imagebuild/controller.go (1)

153-158: ⚠️ Potential issue | 🟠 Major

Cancelled uploads can leak the upload pod.

At Line 153, cancelled builds only run cleanupTransientSecrets. If cancellation happens during Uploading, the upload pod is not shut down and can remain running.

💡 Proposed fix
-	case phaseCancelled, phaseFailed:
-		// Retry cleanup of any transient secrets that failed to delete.
-		if err := r.cleanupTransientSecrets(ctx, imageBuild, r.Log); err != nil {
-			return ctrl.Result{RequeueAfter: secretCleanupRequeue}, nil
-		}
-		return ctrl.Result{}, nil
+	case phaseCancelled:
+		// Ensure upload pod is stopped if cancellation happened during upload phase.
+		if err := r.shutdownUploadPod(ctx, imageBuild); err != nil {
+			log.Error(err, "Failed to shutdown upload pod for cancelled build")
+		}
+		if err := r.cleanupTransientSecrets(ctx, imageBuild, r.Log); err != nil {
+			return ctrl.Result{RequeueAfter: secretCleanupRequeue}, nil
+		}
+		return ctrl.Result{}, nil
+	case phaseFailed:
+		// Retry cleanup of any transient secrets that failed to delete.
+		if err := r.cleanupTransientSecrets(ctx, imageBuild, r.Log); err != nil {
+			return ctrl.Result{RequeueAfter: secretCleanupRequeue}, nil
+		}
+		return ctrl.Result{}, nil
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/controller/imagebuild/controller.go` around lines 153 - 158, The
cancelled/failed branch currently only calls cleanupTransientSecrets, which can
leave the upload pod running; add a call to a cleanup routine that deletes the
upload pod (e.g., r.cleanupUploadPod(ctx, imageBuild, r.Log) or
r.deleteUploadPodIfExists) inside the case for phaseCancelled and phaseFailed,
mirror the error handling used for cleanupTransientSecrets (requeue on error
using secretCleanupRequeue or a dedicated requeue duration), and ensure the
upload-pod cleanup runs before returning ctrl.Result so cancelled uploads cannot
leak pods.
🧹 Nitpick comments (2)
cmd/caib/buildcmd/build.go (1)

840-856: Consider adding a bounded timeout to cancel requests.

RunCancel currently uses context.Background() for the API call. A short timeout would avoid hanging indefinitely on transport stalls.

💡 Suggested adjustment
 func (h *Handler) RunCancel(_ *cobra.Command, args []string) {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
 	buildName := args[0]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/caib/buildcmd/build.go` around lines 840 - 856, RunCancel uses
context.Background() which can block indefinitely; replace it by creating a
cancellable context with a bounded timeout (e.g., context.WithTimeout) before
calling api.CancelBuild in Handler.RunCancel, pass that ctx into
api.CancelBuild, and ensure you defer the cancel() to release resources; choose
an appropriate short duration (e.g., 5–15s) and handle the context deadline
error via the existing h.handleError path.
internal/buildapi/server_test.go (1)

415-480: Consider adding an explicit Uploading cancellation test case.

Pending and Building are covered, but Uploading is also documented as cancellable; adding that case would close the phase-policy gap.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/buildapi/server_test.go` around lines 415 - 480, Add a new test case
mirroring the existing "Pending" and "Building" specs that exercises cancelling
an ImageBuild in phase "Uploading": create a build via
newCancelTestBuild("Uploading", "") (or with a PipelineRun if your cancellation
logic expects one), use newCancelFakeClient to return the fake client, call
server.cancelBuild(c, "my-build") and assert HTTP 200, that the ImageBuild
Status.Phase becomes "Cancelled", Status.Message equals "Build cancelled by
user" and CompletionTime is set; place this alongside the other It(...) blocks
so cancel behavior for the "Uploading" phase is explicitly covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmd/caib/buildcmd/logs.go`:
- Around line 151-155: The cancellation branch in the loop (when st.Phase ==
phaseCancelled) returns an error but skips the centralized error handling used
by other branches; update the branch that currently calls pb.Clear(), prints
"Build was cancelled." and returns fmt.Errorf("build cancelled") to also invoke
h.handleError(...) with the same contextual error (or wrap the fmt.Errorf)
before returning so cancellation flows through the same error handling path as
the timeout/failed cases; reference the st.Phase check for phaseCancelled,
pb.Clear(), the printed message, and h.handleError to locate and modify the code
in logs.go.

In `@internal/buildapi/server.go`:
- Around line 909-931: The code currently proceeds to mark ImageBuild as
Cancelled even when the fetched PipelineRun already completed; change the logic
in the block that handles k8sClient.Get(...) and pipelineRun to check if the
PipelineRun exists and pipelineRun.Status.CompletionTime != nil and, in that
case, abort with c.JSON(http.StatusConflict, ...) (409) instead of falling
through to update build.Status; specifically update the branch that inspects
pipelineRun.Status.CompletionTime (referencing pipelineRun, k8sClient.Get, and
ImageBuild build.Status.Phase) so that a completed PipelineRun returns 409 and
skips modifying build.Status.
- Around line 924-936: The cancel handler sets build.Status.Phase =
phaseCancelled but the log-stream loop still only treats
phaseCompleted/phaseFailed as terminal; update the log-stream exit logic in
shouldExitLogStream (and any callers) to also consider phaseCancelled as a
terminal phase so the /v1/builds/:name/logs polling stops immediately after
cancel. Locate shouldExitLogStream in the same file and add phaseCancelled to
the terminal-phase check (and update any switch/if that compares
build.Status.Phase) so cancelled builds break out the log streaming path.

---

Outside diff comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 153-158: The cancelled/failed branch currently only calls
cleanupTransientSecrets, which can leave the upload pod running; add a call to a
cleanup routine that deletes the upload pod (e.g., r.cleanupUploadPod(ctx,
imageBuild, r.Log) or r.deleteUploadPodIfExists) inside the case for
phaseCancelled and phaseFailed, mirror the error handling used for
cleanupTransientSecrets (requeue on error using secretCleanupRequeue or a
dedicated requeue duration), and ensure the upload-pod cleanup runs before
returning ctrl.Result so cancelled uploads cannot leak pods.

---

Nitpick comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 840-856: RunCancel uses context.Background() which can block
indefinitely; replace it by creating a cancellable context with a bounded
timeout (e.g., context.WithTimeout) before calling api.CancelBuild in
Handler.RunCancel, pass that ctx into api.CancelBuild, and ensure you defer the
cancel() to release resources; choose an appropriate short duration (e.g.,
5–15s) and handle the context deadline error via the existing h.handleError
path.

In `@internal/buildapi/server_test.go`:
- Around line 415-480: Add a new test case mirroring the existing "Pending" and
"Building" specs that exercises cancelling an ImageBuild in phase "Uploading":
create a build via newCancelTestBuild("Uploading", "") (or with a PipelineRun if
your cancellation logic expects one), use newCancelFakeClient to return the fake
client, call server.cancelBuild(c, "my-build") and assert HTTP 200, that the
ImageBuild Status.Phase becomes "Cancelled", Status.Message equals "Build
cancelled by user" and CompletionTime is set; place this alongside the other
It(...) blocks so cancel behavior for the "Uploading" phase is explicitly
covered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4050a13e-9bd5-47f2-afad-0e6889d54930

📥 Commits

Reviewing files that changed from the base of the PR and between 0bddab6 and d5856cc.

📒 Files selected for processing (8)
  • cmd/caib/buildcmd/build.go
  • cmd/caib/buildcmd/logs.go
  • cmd/caib/image/image.go
  • cmd/caib/runtime_wiring.go
  • internal/buildapi/client/client.go
  • internal/buildapi/server.go
  • internal/buildapi/server_test.go
  • internal/controller/imagebuild/controller.go

Comment thread cmd/caib/buildcmd/logs.go
Comment thread internal/buildapi/server.go
Comment thread internal/buildapi/server.go Outdated
@bennyz bennyz force-pushed the worktree-image-cancel branch from d5856cc to a516c40 Compare April 16, 2026 10:53
@bennyz

bennyz commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@bennyz bennyz force-pushed the worktree-image-cancel branch from a516c40 to b66c008 Compare April 19, 2026 09:51

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
internal/buildapi/server_test.go (1)

299-342: Consider sharing setup helpers with the Delete Build context.

originalGetClientFromRequestFn/originalNamespace save/restore plumbing and the newFakeClient/newCancelFakeClient builders largely duplicate the equivalents in the Delete Build context (lines 112-161). Extracting a shared BeforeEach/helper at the outer Describe scope (or a small package-private helper like newTestSchemeClient) would reduce drift as more endpoints are added.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/buildapi/server_test.go` around lines 299 - 342, The test contains
duplicated setup/teardown and fake client builders between the "Cancel Build"
and "Delete Build" contexts (symbols: originalGetClientFromRequestFn,
originalNamespace, newCancelFakeClient, newFakeClient); extract the shared
save/restore of getClientFromRequestFn and BUILD_API_NAMESPACE plus a shared
fake-client builder (e.g., newTestSchemeClient or a BeforeEach/AfterEach at the
outer Describe) and have both contexts call those helpers so the duplicated
plumbing is centralized and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/v1alpha1/imagebuild_types.go`:
- Around line 216-218: The CRD enum on the ImageBuild status field Phase (field
Phase in type Status in imagebuild_types.go) was extended to include "Cancelled"
but the generated CRD/schema wasn't updated; run the generator to regenerate
DeepCopy methods and CRD manifests and commit the results: run the project
generation targets (e.g. make generate manifests) to update the OpenAPI
validation schema for Status.Phase so the API server accepts "Cancelled", then
review and stage the updated CRD YAML and deepcopy files produced by the
generator.

---

Nitpick comments:
In `@internal/buildapi/server_test.go`:
- Around line 299-342: The test contains duplicated setup/teardown and fake
client builders between the "Cancel Build" and "Delete Build" contexts (symbols:
originalGetClientFromRequestFn, originalNamespace, newCancelFakeClient,
newFakeClient); extract the shared save/restore of getClientFromRequestFn and
BUILD_API_NAMESPACE plus a shared fake-client builder (e.g., newTestSchemeClient
or a BeforeEach/AfterEach at the outer Describe) and have both contexts call
those helpers so the duplicated plumbing is centralized and reused.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0cc634b5-870f-433e-aedd-036bb809735f

📥 Commits

Reviewing files that changed from the base of the PR and between d5856cc and b66c008.

⛔ Files ignored due to path filters (1)
  • config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml is excluded by !config/crd/bases/**
📒 Files selected for processing (10)
  • api/v1alpha1/imagebuild_types.go
  • cmd/caib/buildcmd/build.go
  • cmd/caib/buildcmd/logs.go
  • cmd/caib/image/image.go
  • cmd/caib/runtime_wiring.go
  • internal/buildapi/client/client.go
  • internal/buildapi/container_builds.go
  • internal/buildapi/server.go
  • internal/buildapi/server_test.go
  • internal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (1)
  • internal/buildapi/container_builds.go
🚧 Files skipped from review as they are similar to previous changes (6)
  • cmd/caib/runtime_wiring.go
  • internal/buildapi/client/client.go
  • cmd/caib/image/image.go
  • cmd/caib/buildcmd/build.go
  • internal/controller/imagebuild/controller.go
  • internal/buildapi/server.go

Comment thread api/v1alpha1/imagebuild_types.go
@bennyz bennyz requested a review from bkhizgiy April 19, 2026 09:55

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

nice addition! will be useful XD

@bennyz bennyz force-pushed the worktree-image-cancel branch from b66c008 to d38acd5 Compare April 19, 2026 15:11

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
internal/controller/imagebuild/controller.go (2)

159-164: ⚠️ Potential issue | 🟠 Major

Delete the upload pod when a build is cancelled during upload.

phaseCancelled now reaches this terminal branch, but only transient secrets are cleaned up. If cancellation happens in Uploading, the upload pod is preserved with the ImageBuild and can keep running indefinitely.

Proposed fix
 	case phaseCancelled, phaseFailed:
+		if imageBuild.Status.Phase == phaseCancelled && imageBuild.Spec.GetInputFilesServer() {
+			if err := r.shutdownUploadPod(ctx, imageBuild); err != nil {
+				log.Error(err, "Failed to shutdown upload pod after cancellation")
+				return ctrl.Result{}, err
+			}
+		}
 		// Retry cleanup of any transient secrets that failed to delete.
 		if err := r.cleanupTransientSecrets(ctx, imageBuild, r.Log); err != nil {
 			return ctrl.Result{RequeueAfter: secretCleanupRequeue}, nil
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/controller/imagebuild/controller.go` around lines 159 - 164, When
handling the terminal branch for phaseCancelled/phaseFailed in the reconcile
(the switch handling these phases around the case with
r.cleanupTransientSecrets), also delete the upload pod so a cancelled build in
Uploading doesn't leave the pod running; call the existing helper that removes
the upload pod (e.g., r.deleteUploadPod(ctx, imageBuild, r.Log) or the
equivalent function that deletes the associated Pod) after or before
cleanupTransientSecrets, treat any deletion error as transient and requeue
(return ctrl.Result{RequeueAfter: secretCleanupRequeue}, err) and otherwise
return ctrl.Result{} when both cleanupTransientSecrets and upload-pod deletion
succeed.

2103-2111: ⚠️ Potential issue | 🟠 Major

Prevent stale reconciles from overwriting Cancelled.

The cancel API can set Cancelled while an older reconcile is still running. Since updateStatus refetches the fresh object but then unconditionally assigns phase, a stale path can move a cancelled build back to Building, Failed, or Completed.

Proposed fix
 	oldPhase := fresh.Status.Phase
 	oldMessage := fresh.Status.Message
 
+	if isTerminalPhase(oldPhase) && oldPhase != phase {
+		return nil
+	}
+
 	fresh.Status.Phase = phase
 	fresh.Status.Message = message
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/controller/imagebuild/controller.go` around lines 2103 - 2111, The
updateStatus path unconditionally sets fresh.Status.Phase and can overwrite a
user-cancelled build; modify updateStatus (the block that assigns
fresh.Status.Phase, fresh.Status.Message and sets StartTime/CompletionTime) to
first fetch fresh and then skip changing Phase (and related timestamps/messages)
if fresh.Status.Phase == Cancelled (or equals the package constant/enum for
cancelled, e.g. phaseCancelled/Cancelled), otherwise proceed with the existing
logic; ensure you still set StartTime/CompletionTime only when you actually
change the phase (keep checks using phaseBuilding/isTerminalPhase).
♻️ Duplicate comments (1)
cmd/caib/buildcmd/logs.go (1)

151-154: ⚠️ Potential issue | 🟠 Major

Route cancellations through handleError so the CLI exits non-zero.

This branch returns an error but skips the centralized error path used by timeout and failed builds, so callers that just return from Run* can exit successfully after cancellation.

Proposed fix
 			if st.Phase == phaseCancelled {
 				pb.Clear()
+				cancelErr := fmt.Errorf("build cancelled")
 				fmt.Println("Build was cancelled.")
-				return fmt.Errorf("build cancelled")
+				h.handleError(cancelErr)
+				return cancelErr
 			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/caib/buildcmd/logs.go` around lines 151 - 154, The cancellation branch
currently prints a message and returns fmt.Errorf("build cancelled") directly
(see st.Phase == phaseCancelled, pb.Clear(), fmt.Println("Build was
cancelled.")), bypassing the centralized exit behavior; change it to route the
cancellation through the shared error handler by clearing the progress
(pb.Clear()) then calling handleError with the constructed error (or passing an
error value into the same path used for timeouts/failures) instead of printing
and returning directly so callers exit non-zero consistently.
🧹 Nitpick comments (1)
internal/buildapi/server_test.go (1)

379-413: Add coverage for already-cancelled builds.

The terminal-state cases cover Completed and Failed, but not Cancelled. Since cancel sets Cancelled as terminal, a repeat cancel should also return conflict instead of rewriting status.

Suggested test case
 		It("should return 409 when build has already failed", func() {
 			build := newCancelTestBuild("Failed", "")
 			fakeClient := newCancelFakeClient(build)
 			getClientFromRequestFn = func(_ *gin.Context) (ctrlclient.Client, error) {
 				return fakeClient, nil
 			}
@@
 			Expect(w.Code).To(Equal(http.StatusConflict))
 			Expect(w.Body.String()).To(ContainSubstring("cannot be cancelled"))
 		})
+
+		It("should return 409 when build is already cancelled", func() {
+			build := newCancelTestBuild("Cancelled", "")
+			fakeClient := newCancelFakeClient(build)
+			getClientFromRequestFn = func(_ *gin.Context) (ctrlclient.Client, error) {
+				return fakeClient, nil
+			}
+
+			w := httptest.NewRecorder()
+			c, _ := gin.CreateTestContext(w)
+			c.Request, _ = http.NewRequest(http.MethodPost, "/v1/builds/my-build/cancel", nil)
+			c.Set("requester", "alice")
+
+			server.cancelBuild(c, "my-build")
+
+			Expect(w.Code).To(Equal(http.StatusConflict))
+			Expect(w.Body.String()).To(ContainSubstring("cannot be cancelled"))
+		})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/buildapi/server_test.go` around lines 379 - 413, Add a test case
mirroring the "Completed" and "Failed" specs to assert a repeat cancel on an
already-cancelled build returns 409 and an appropriate message: create a build
via newCancelTestBuild("Cancelled", ""), wrap it in newCancelFakeClient, set
getClientFromRequestFn to return that fake client, create a gin test context and
request as in the other tests, call server.cancelBuild(c, "my-build"), and
Expect the response code to Equal(http.StatusConflict) and the body to
ContainSubstring("cannot be cancelled").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/buildapi/server.go`:
- Around line 895-903: The cancel endpoint currently rejects builds whose
build.Status.Phase is empty string; update the switch in the cancellation logic
to treat an empty phase as cancellable (same as phasePending) so new ImageBuilds
that haven't had the controller set the initial phase can still be cancelled;
modify the switch that checks build.Status.Phase (the block handling
phasePending, phaseUploading, phaseBuilding, phasePushing, phaseFlashing) to
include the "" case (or explicitly check build.Status.Phase == "" before the
switch and allow cancellation) so a call to caib image cancel won’t return 409
for newly-created builds.

---

Outside diff comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 159-164: When handling the terminal branch for
phaseCancelled/phaseFailed in the reconcile (the switch handling these phases
around the case with r.cleanupTransientSecrets), also delete the upload pod so a
cancelled build in Uploading doesn't leave the pod running; call the existing
helper that removes the upload pod (e.g., r.deleteUploadPod(ctx, imageBuild,
r.Log) or the equivalent function that deletes the associated Pod) after or
before cleanupTransientSecrets, treat any deletion error as transient and
requeue (return ctrl.Result{RequeueAfter: secretCleanupRequeue}, err) and
otherwise return ctrl.Result{} when both cleanupTransientSecrets and upload-pod
deletion succeed.
- Around line 2103-2111: The updateStatus path unconditionally sets
fresh.Status.Phase and can overwrite a user-cancelled build; modify updateStatus
(the block that assigns fresh.Status.Phase, fresh.Status.Message and sets
StartTime/CompletionTime) to first fetch fresh and then skip changing Phase (and
related timestamps/messages) if fresh.Status.Phase == Cancelled (or equals the
package constant/enum for cancelled, e.g. phaseCancelled/Cancelled), otherwise
proceed with the existing logic; ensure you still set StartTime/CompletionTime
only when you actually change the phase (keep checks using
phaseBuilding/isTerminalPhase).

---

Duplicate comments:
In `@cmd/caib/buildcmd/logs.go`:
- Around line 151-154: The cancellation branch currently prints a message and
returns fmt.Errorf("build cancelled") directly (see st.Phase == phaseCancelled,
pb.Clear(), fmt.Println("Build was cancelled.")), bypassing the centralized exit
behavior; change it to route the cancellation through the shared error handler
by clearing the progress (pb.Clear()) then calling handleError with the
constructed error (or passing an error value into the same path used for
timeouts/failures) instead of printing and returning directly so callers exit
non-zero consistently.

---

Nitpick comments:
In `@internal/buildapi/server_test.go`:
- Around line 379-413: Add a test case mirroring the "Completed" and "Failed"
specs to assert a repeat cancel on an already-cancelled build returns 409 and an
appropriate message: create a build via newCancelTestBuild("Cancelled", ""),
wrap it in newCancelFakeClient, set getClientFromRequestFn to return that fake
client, create a gin test context and request as in the other tests, call
server.cancelBuild(c, "my-build"), and Expect the response code to
Equal(http.StatusConflict) and the body to ContainSubstring("cannot be
cancelled").
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 91a3c879-ac81-4689-a300-caece90e637d

📥 Commits

Reviewing files that changed from the base of the PR and between b66c008 and d38acd5.

⛔ Files ignored due to path filters (1)
  • config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml is excluded by !config/crd/bases/**
📒 Files selected for processing (10)
  • api/v1alpha1/imagebuild_types.go
  • cmd/caib/buildcmd/build.go
  • cmd/caib/buildcmd/logs.go
  • cmd/caib/image/image.go
  • cmd/caib/runtime_wiring.go
  • internal/buildapi/client/client.go
  • internal/buildapi/container_builds.go
  • internal/buildapi/server.go
  • internal/buildapi/server_test.go
  • internal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (1)
  • cmd/caib/runtime_wiring.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • internal/buildapi/container_builds.go
  • internal/buildapi/client/client.go
  • api/v1alpha1/imagebuild_types.go
  • cmd/caib/image/image.go

Comment thread internal/buildapi/server.go
@bennyz bennyz force-pushed the worktree-image-cancel branch from d38acd5 to a4c3c68 Compare April 19, 2026 15:54
Allow cancellation of builds

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-opus-4.6
@bennyz bennyz force-pushed the worktree-image-cancel branch from a4c3c68 to 1f710a7 Compare April 19, 2026 16:06
@bennyz bennyz merged commit 57d4f97 into centos-automotive-suite:main Apr 19, 2026
4 checks passed
@bennyz bennyz deleted the worktree-image-cancel branch April 19, 2026 16:26
@coderabbitai coderabbitai Bot mentioned this pull request Apr 27, 2026
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants