internal/storage/image.go (1)

833-847: context.Canceled falls through to return true, triggering an unnecessary artifact attempt.

context.DeadlineExceeded satisfies net.Error (it has Timeout()/Temporary()) and is already excluded. However, context.Canceled does not implement net.Error, so a cancelled-context failure from copy.Image would still attempt the artifact pull — with the already-cancelled context — and produce a confusing combined error.

♻️ Suggested addition

 func shouldTryArtifact(err error) bool {
 	var netErr net.Error
 	switch {
 	case err == nil:
 		return false
+	case errors.Is(err, context.Canceled):
+		// Context was already cancelled; an artifact pull would fail
+		// the same way immediately.
+		return false
 	case errors.As(err, &netErr):
 		// Network errors indicate the registry is unreachable, so an artifact
 		// pull would fail the same way. Rely on Kubelet retries instead.
 		return false
 	default:
 		return true
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/storage/image.go` around lines 833 - 847, shouldTryArtifact
currently treats context.Canceled as a regular error and returns true, causing
pointless artifact attempts; update shouldTryArtifact to explicitly detect
context.Canceled (using errors.Is(err, context.Canceled)) and return false
before the switch/after the nil check so cancelled contexts do not trigger
artifact pulls; keep the existing net.Error check and behavior for
context.DeadlineExceeded intact and ensure you reference the shouldTryArtifact
function when making the change.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

internal/storage/image.go (1)

833-847: Consider also skipping artifact fallback on context cancellation/deadline errors.

context.Canceled and context.DeadlineExceeded don't implement net.Error, so a pull that times out via context will still attempt the artifact fallback. If the parent context is already done, the artifact pull will fail immediately anyway, but it adds unnecessary code path execution and potentially a confusing combined error message. You may want to add a case for ctx.Err() != nil or errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded).

Suggested change

 func shouldTryArtifact(err error) bool {
 	var netErr net.Error
 	switch {
 	case err == nil:
 		return false
 	case errors.As(err, &netErr):
 		// Network errors indicate the registry is unreachable, so an artifact
 		// pull would fail the same way. Rely on Kubelet retries instead.
 		return false
+	case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded):
+		// Context is already done; the artifact pull would fail immediately.
+		return false
 	default:
 		return true
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/storage/image.go` around lines 833 - 847, The shouldTryArtifact
function should also skip artifact fallback for context cancellation/deadline
errors; update the switch to check for context.Canceled and
context.DeadlineExceeded (e.g. using errors.Is(err, context.Canceled) ||
errors.Is(err, context.DeadlineExceeded)) and return false in that case, so that
shouldTryArtifact returns false when the pull failed due to a cancelled or
timed-out context rather than only for net.Error cases.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

internal/storage/image.go (1)

916-933: shouldTryArtifact looks correct.

The context.Canceled / context.DeadlineExceeded short-circuit at line 923 is properly in place, the errors.As net-error check covers all wrapped network failures, and the "why" comment at lines 927–929 is clear. The function is defined after its call site, consistent with Go convention and the prior nit being acted on.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/storage/image.go` around lines 916 - 933, No changes required: the
shouldTryArtifact function correctly handles nil, context.Canceled,
context.DeadlineExceeded, and wrapped network errors via errors.As, so leave
shouldTryArtifact as-is (no code edits needed) and mark the review as approved
for function shouldTryArtifact.

internal/storage/image.go (1)

873-876: Consider including the original copy error in the store-creation failure path.

Line 875 wraps only artifactErr, while line 888 wraps both the original copy.Image error and the artifact error. If the store can't be created, the caller loses the reason the initial image pull failed, making it harder to triage.

♻️ Proposed fix

-		return RegistryImageReference{}, fmt.Errorf("unable to pull image or OCI artifact: create store err: %w", artifactErr)
+		return RegistryImageReference{}, fmt.Errorf("unable to pull image or OCI artifact: pull image err: %w; create store err: %w", err, artifactErr)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/storage/image.go` around lines 873 - 876, The store-creation failure
currently returns only artifactErr from ociartifact.NewStore, which loses the
original copy.Image error; update the error return in the ociartifact.NewStore
failure path to include the earlier image copy error (the variable produced by
copy.Image / the image pull call) alongside artifactErr so callers see both
causes (e.g., mention the copy error and the store creation error in the
fmt.Errorf message), referencing ociartifact.NewStore, artifactErr and the
earlier copy/Image error variable to locate and fix the code.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro