handle ErrNotAnImage in RemoveImage for concurrent deletion idempotency by jnovy · Pull Request #9803 · cri-o/cri-o

jnovy · 2026-03-06T06:04:55Z

When multiple concurrent RemoveImage calls race on the same image, the first call succeeds but subsequent calls fail with "identifier is not an image" (ErrNotAnImage). This happens because the image is already gone from storage by the time the other goroutines attempt deletion.

The existing idempotency check only handled ErrImageUnknown but not ErrNotAnImage, which is the error returned by containers/storage when the image ID no longer resolves. Add ErrNotAnImage to the idempotency checks in both the ID-based and name-based deletion paths.

Fixes #9796

What type of PR is this?

/kind bug

What this PR does / why we need it:

When multiple concurrent RemoveImage calls race on the same image, the first call succeeds but subsequent calls fail with "identifier is not an image" (ErrNotAnImage). This happens because the image is already gone from storage by the time the other goroutines attempt deletion.

The existing idempotency check only handled ErrImageUnknown but not ErrNotAnImage, which is the error returned by containers/storage when the image ID no longer resolves. This PR adds ErrNotAnImage to the idempotency checks in both the ID-based and name-based deletion paths.

Which issue(s) this PR fixes:

Fixes #9796

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Fix concurrent RemoveImage race condition by handling ErrNotAnImage as an idempotent deletion result.

Summary by CodeRabbit

Release Notes

Bug Fixes
- Image removal operations are now idempotent. Attempting to remove an image that is not found no longer returns an error, allowing safe repeated removal attempts.
- Improved handling of concurrent deletion scenarios in both direct and indirect image removal paths for more robust operations.

…potency When multiple concurrent RemoveImage calls race on the same image, the first call succeeds but subsequent calls fail with "identifier is not an image" (ErrNotAnImage). This happens because the image is already gone from storage by the time the other goroutines attempt deletion. The existing idempotency check only handled ErrImageUnknown but not ErrNotAnImage, which is the error returned by containers/storage when the image ID no longer resolves. Add ErrNotAnImage to the idempotency checks in both the ID-based and name-based deletion paths. Fixes cri-o#9796 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jindrich Novy <jnovy@redhat.com>

openshift-ci · 2026-03-06T06:05:08Z

Hi @jnovy. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

coderabbitai · 2026-03-06T06:05:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0c5a4350-696b-4a1a-a19b-fe63db70d8cd

📥 Commits

Reviewing files that changed from the base of the PR and between 075c4db and 71e9bab.

📒 Files selected for processing (2)

server/image_remove.go
server/image_remove_test.go

📝 Walkthrough

Walkthrough

Modified removeImage function to treat ErrImageUnknown and ErrNotAnImage errors as non-errors when returned by DeleteImage or UntagImage, enabling idempotent concurrent image removal. Added test cases validating this behavior in race scenarios.

Changes

Cohort / File(s)	Summary
Error Handling for Idempotent Removal `server/image_remove.go`	Modified `removeImage` to return nil instead of propagating `ErrImageUnknown` or `ErrNotAnImage` from both `DeleteImage` and `UntagImage` calls, widening idempotence handling across direct delete and untag paths.
Concurrent Deletion Test Cases `server/image_remove_test.go`	Added two test cases verifying that concurrent `RemoveImage` calls do not error when the underlying delete/untag operations encounter not-an-image errors due to race conditions.

Possibly related issues

RemoveImage race condition: concurrent calls fail with "identifier is not an image" #9796: RemoveImage race condition — concurrent calls failing with "identifier is not an image" — directly addressed by these changes treating image-not-found errors as non-errors during concurrent removal scenarios.
Concurrent RemoveImage calls show inconsistent ImageStatus results #9717: Related modifications to server/image_remove.go for idempotence and error handling during DeleteImage/UntagImage operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Five rabbits hopped to clear their feast,
But racing for the same image, they found no beast—
Now errors hush to silence, so wise,
Concurrent deletions: no tears, no sighs!
Idempotent harmony beneath the skies ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately and concisely summarizes the main change: handling ErrNotAnImage errors in RemoveImage to achieve idempotency for concurrent deletion scenarios, which is the core objective of the changeset.
Linked Issues check	✅ Passed	The code changes directly address issue `#9796` by adding ErrNotAnImage handling to idempotency checks in both ID-based and name-based deletion paths, making RemoveImage resilient to concurrent deletion races as required.
Out of Scope Changes check	✅ Passed	All changes in both image_remove.go and image_remove_test.go are directly scoped to the issue: handling ErrNotAnImage errors for concurrent deletion idempotency with corresponding test coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

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.}

codecov · 2026-03-06T08:01:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.53%. Comparing base (075c4db) to head (71e9bab).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9803      +/-   ##
==========================================
+ Coverage   64.65%   67.53%   +2.88%     
==========================================
  Files         212      212              
  Lines       29233    29235       +2     
==========================================
+ Hits        18900    19745     +845     
+ Misses       8643     7801     -842     
+ Partials     1690     1689       -1

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

saschagrunert · 2026-03-06T08:02:35Z

/ok-to-test

saschagrunert

Clean and well-targeted fix. The change is correct, tests cover both the ID-based and name-based deletion paths, and codecov confirms full coverage.

Minor observation: the statusErr check at line 119 is now functionally redundant since line 126 also returns nil unconditionally. It reads as documentation of intent, which is fine, but worth noting for a future cleanup.

LGTM.

saschagrunert · 2026-03-06T08:05:07Z

 	}

 	if !deleted && untagErr != nil {
+		if errors.Is(untagErr, storagetypes.ErrImageUnknown) || errors.Is(untagErr, storagetypes.ErrNotAnImage) {


Good catch adding ErrImageUnknown here as well. The existing code only had ErrImageUnknown handling in the ID-based path (line 49) and ErrNotAnImage handling via statusErr at line 119. This closes the gap where UntagImage could also return ErrImageUnknown during a concurrent deletion race.

openshift-ci · 2026-03-06T08:05:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jnovy, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jnovy · 2026-03-06T13:13:19Z

/retest

saschagrunert · 2026-03-06T17:15:48Z

/retest

bitoku · 2026-03-09T11:55:41Z

/retest

jnovy requested a review from mrunalp as a code owner March 6, 2026 06:04

openshift-ci Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 6, 2026

openshift-ci Bot requested review from bitoku and klihub March 6, 2026 06:05

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 6, 2026

openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2026

saschagrunert approved these changes Mar 6, 2026

View reviewed changes

openshift-ci Bot assigned saschagrunert Mar 6, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2026

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2026

openshift-merge-bot Bot merged commit 2754216 into cri-o:main Mar 9, 2026
74 checks passed

Conversation

jnovy commented Mar 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Summary by CodeRabbit

Release Notes

Uh oh!

openshift-ci Bot commented Mar 6, 2026

Uh oh!

coderabbitai Bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related issues

Estimated code review effort

Poem

Uh oh!

codecov Bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

saschagrunert commented Mar 6, 2026

Uh oh!

saschagrunert left a comment

Choose a reason for hiding this comment

Uh oh!

saschagrunert Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Mar 6, 2026

Uh oh!

jnovy commented Mar 6, 2026

Uh oh!

saschagrunert commented Mar 6, 2026

Uh oh!

bitoku commented Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jnovy commented Mar 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 6, 2026 •

edited

Loading

codecov Bot commented Mar 6, 2026 •

edited

Loading