Skip to content

fix: retain recently written external resources missing from a stale update - #3565

Open
csviri wants to merge 1 commit into
operator-framework:nextfrom
csviri:external-bulk-flake
Open

fix: retain recently written external resources missing from a stale update#3565
csviri wants to merge 1 commit into
operator-framework:nextfrom
csviri:external-bulk-flake

Conversation

@csviri

@csviri csviri commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

An update of the whole resource set of a primary (a poll result or a received
event) might have been created before the reconciler wrote a resource, thus not
containing it yet. Since such updates are handled as the full actual state, the
write was lost from the cache, and the next reconciliation created a duplicate
of an already created resource or repeated an already executed update.

Writes are now marked as unconfirmed and retained for the next update if it
either does not contain the resource at all - the expected case for a create -
or still contains the state that the write replaced. Any other state is treated
as a change made outside of the reconciler and accepted as actual. Marks are
dropped on the first update, so a resource really deleted or changed meanwhile
is not retained indefinitely.

Also guards handleRecentResourceUpdate against a missing cache entry, and
resolves the actual resources from the state resources in the external state
bulk dependent integration test, which is the recommended approach for
resources that take longer to become visible.

…update

An update of the whole resource set of a primary (a poll result or a received
event) might have been created before the reconciler wrote a resource, thus not
containing it yet. Since such updates are handled as the full actual state, the
write was lost from the cache, and the next reconciliation created a duplicate
of an already created resource or repeated an already executed update.

Writes are now marked as unconfirmed and retained for the next update if it
either does not contain the resource at all - the expected case for a create -
or still contains the state that the write replaced. Any other state is treated
as a change made outside of the reconciler and accepted as actual. Marks are
dropped on the first update, so a resource really deleted or changed meanwhile
is not retained indefinitely.

Also guards handleRecentResourceUpdate against a missing cache entry, and
resolves the actual resources from the state resources in the external state
bulk dependent integration test, which is the recommended approach for
resources that take longer to become visible.
@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 Aug 18, 2026
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1f161349-6788-425d-8328-a8802ceb38b1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@csviri csviri changed the title fix: retain recently written external resources missing from a stale update [WIP] fix: retain recently written external resources missing from a stale update Aug 18, 2026
@csviri csviri changed the title [WIP] fix: retain recently written external resources missing from a stale update fix: retain recently written external resources missing from a stale update Aug 21, 2026
@csviri
csviri requested a lite review from Copilot August 21, 2026 11:03
@csviri
csviri marked this pull request as ready for review August 21, 2026 11:03
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2026
@csviri
csviri requested a review from xstefank August 21, 2026 11:03
@openshift-ci
openshift-ci Bot requested a review from metacosm August 21, 2026 11:03

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

This PR improves external-resource cache consistency when full-state updates are stale relative to reconciler writes.

Changes:

  • Retains recently written resources during stale cache refreshes.
  • Adds regression tests for stale and externally changed resources.
  • Updates external-state integration behavior and documentation.

A critical issue remains: consecutive writes to the same resource can overwrite prior unconfirmed state, allowing a stale update to replace the latest value and trigger repeated reconciliation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Summary
operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/externalstate/externalstatebulkdependent/BulkDependentResourceExternalWithState.java Resolves resources through persisted state.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java Adds cache consistency regression tests.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java Implements unconfirmed-write retention.
docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md Documents external-state consistency guidance.
Suppressed comments (3)

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:84

  • These marks are not cleared when a primary is deleted through CachingInboundEventSource: its onResourceDeleted only removes fetchedForPrimaries and never calls handleDelete to remove this map. If the same ResourceID is later recreated, the first full update can satisfy newResource == null and reinsert the old written resource into the new primary's cache. Tie this state to primary deletion (and clear the corresponding cache) in that lifecycle path.
  private final Map<ResourceID, Map<ID, RecentWrite<R>>> unconfirmedWrites =
      new ConcurrentHashMap<>();

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:219

  • This branch also retains an updated resource when newResource.equals(write.replaced()), so the log message is inaccurate for that common stale-update case: the resource is present, but its new state is not reflected. Please use wording such as "not reflected in the update" so debug logs do not misdiagnose retained updates.
                "Retaining recently written resource missing from the update. Primary ID: {},"
                    + " resource ID: {}",

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:298

  • The null check is a stated behavior change, but the added tests only call handleRecentResourceUpdate after seeding the primary/resource in the cache. Please add a regression test for an absent primary or resource entry that verifies this path does not throw and does not cache the update.
      if (actualResource != null && actualResource.equals(previousVersionOfResource)) {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

}

private void markUnconfirmedWrite(ResourceID primaryID, ID resourceId, RecentWrite<R> write) {
unconfirmedWrites.computeIfAbsent(primaryID, id -> new HashMap<>()).put(resourceId, write);
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