Skip to content

fix: do not NPE when adding a finalizer to a resource deleted mid-retry - #3530

Draft
csviri wants to merge 1 commit into
operator-framework:mainfrom
csviri:fix/add-finalizer-null-resource
Draft

fix: do not NPE when adding a finalizer to a resource deleted mid-retry#3530
csviri wants to merge 1 commit into
operator-framework:mainfrom
csviri:fix/add-finalizer-null-resource

Conversation

@csviri

@csviri csviri commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

conflictRetryingPatchPrimary / conflictRetryingPatch re-read the
resource from the API server after a 409 or 422 and then re-evaluate the
precondition:

resource = operation.inNamespace(ns).withName(name).get();

get() returns null if the resource was deleted in the meantime, so the
next iteration calls the precondition with null. removeFinalizer
anticipates this:

r -> {
  if (r == null) {
    log.warn("Cannot remove finalizer since resource not exists.");
    return false;
  }
  return r.hasFinalizer(finalizerName);
}

but addFinalizer passes r -> !r.hasFinalizer(finalizerName), which
throws a NullPointerException instead of exiting cleanly.

Gives addFinalizer the same null guard, in both ResourceOperations and
the deprecated PrimaryUpdateAndCacheUtils.

No test is added: reaching the retry path requires stubbing the client to
answer 409/422 and then 404 through the whole fabric8 DSL chain, which the
existing unit tests are not set up for.

Part of #3517

`conflictRetryingPatchPrimary` / `conflictRetryingPatch` re-read the
resource from the API server after a 409 or 422 and then re-evaluate the
precondition:

    resource = operation.inNamespace(ns).withName(name).get();

`get()` returns null if the resource was deleted in the meantime, so the
next iteration calls the precondition with null. `removeFinalizer`
anticipates this:

    r -> {
      if (r == null) {
        log.warn("Cannot remove finalizer since resource not exists.");
        return false;
      }
      return r.hasFinalizer(finalizerName);
    }

but `addFinalizer` passes `r -> !r.hasFinalizer(finalizerName)`, which
throws a NullPointerException instead of exiting cleanly.

Gives `addFinalizer` the same null guard, in both `ResourceOperations` and
the deprecated `PrimaryUpdateAndCacheUtils`.

No test is added: reaching the retry path requires stubbing the client to
answer 409/422 and then 404 through the whole fabric8 DSL chain, which the
existing unit tests are not set up for.
Copilot AI review requested due to automatic review settings July 30, 2026 09:05
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2026

Copilot AI 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.

Pull request overview

Prevents a NullPointerException when addFinalizer is executed through the conflict-retry patch path and the primary resource is deleted between retries (i.e., the re-read get() returns null). This makes addFinalizer behave consistently with existing removeFinalizer handling in ResourceOperations.

Changes:

  • Add a null-guard to the addFinalizer precondition in ResourceOperations to avoid dereferencing a deleted resource during retry.
  • Add the same null-guard to the deprecated PrimaryUpdateAndCacheUtils.addFinalizer retry precondition (and log when the resource is gone).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java Adds null-guarded precondition for addFinalizer during conflict-retry patch.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java Adds null-guarded precondition for deprecated addFinalizer during conflict-retry patch.

r -> !r.hasFinalizer(finalizerName));
r -> {
if (r == null) {
log.warn("Cannot add finalizer since resource not exists.");
Comment on lines +1082 to +1086
r -> {
if (r == null) {
log.warn("Cannot add finalizer since resource no longer exists.");
return false;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants