From 91fa6d1c9d2543eb55d20cad2b5d5b41e6e279c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 29 Jul 2026 17:58:44 +0200 Subject: [PATCH 1/2] fix: read the external resource cache under the event source monitor `ExternalResourceCachingEventSource` mutates its cache from `synchronized` methods (`handleResources`, `handleDelete`, `handleRecentResourceCreate/Update`), but the read paths were not synchronized. The outer map is a `ConcurrentHashMap`; the nested per-primary maps are plain `HashMap`s that `handleDelete` mutates in place, so a reconciler thread reading them while a poll or informer thread writes can observe a corrupted map or throw. `getSecondaryResources(ResourceID)` additionally looked the primary up twice: var cachedValues = cache.get(primaryID); if (cachedValues == null) { return Collections.emptySet(); } else { return new HashSet<>(cache.get(primaryID).values()); } If a concurrent `handleDelete` removes the entry between the two calls, the second `get` returns null and this throws a NullPointerException. Adds a `cachedResourcesFor` helper that snapshots the cached resources while holding the monitor, and routes `getSecondaryResources` plus the `PerResourcePollingEventSource` and `CachingInboundEventSource` overrides (and `checkAndRegisterTask`) through it. The helper only copies, so the potentially slow `ResourceFetcher` calls in those overrides still run outside the lock and cannot block the informer or poll threads. `getCache()` still returns a live view for backwards compatibility, but now documents that iterating the nested maps requires synchronizing on the event source. Adds a test asserting `getSecondaryResources` returns a snapshot rather than a live view. --- .../ExternalResourceCachingEventSource.java | 21 +++++++++++++++++-- .../inbound/CachingInboundEventSource.java | 7 +++---- .../PerResourcePollingEventSource.java | 12 +++++------ ...xternalResourceCachingEventSourceTest.java | 11 ++++++++++ 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java index 6dbf5b7fb4..61fb2c841a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java @@ -250,12 +250,23 @@ public Set getSecondaryResources(P primary) { } public Set getSecondaryResources(ResourceID primaryID) { + return cachedResourcesFor(primaryID); + } + + /** + * Snapshot of the currently cached secondary resources for the target primary. The per-primary + * maps held in the cache are not thread safe and are mutated while holding this object's monitor, + * so they must not be read outside of it. + * + * @param primaryID id of the primary resource + * @return a copy of the cached secondary resources, empty if none are cached + */ + protected synchronized Set cachedResourcesFor(ResourceID primaryID) { var cachedValues = cache.get(primaryID); if (cachedValues == null) { return Collections.emptySet(); - } else { - return new HashSet<>(cache.get(primaryID).values()); } + return new HashSet<>(cachedValues.values()); } public Optional getSecondaryResource(ResourceID primaryID) { @@ -269,6 +280,12 @@ public Optional getSecondaryResource(ResourceID primaryID) { } } + /** + * @return a live, unmodifiable view of the cache. Note that the nested per-primary maps are not + * thread safe and are mutated while holding this object's monitor, so iterating them without + * synchronizing on this event source is not safe. Prefer {@link + * #getSecondaryResources(ResourceID)}, which returns a snapshot. + */ public Map> getCache() { return Collections.unmodifiableMap(cache); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSource.java index 44e0a684b6..8e7518dcb3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSource.java @@ -16,7 +16,6 @@ package io.javaoperatorsdk.operator.processing.event.source.inbound; import java.util.Collections; -import java.util.HashSet; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -76,9 +75,9 @@ private Set getAndCacheResource(P primary) { @Override public Set getSecondaryResources(P primary) { var primaryID = ResourceID.fromResource(primary); - var cachedValue = cache.get(primaryID); - if (cachedValue != null && !cachedValue.isEmpty()) { - return new HashSet<>(cachedValue.values()); + var cachedValues = cachedResourcesFor(primaryID); + if (!cachedValues.isEmpty()) { + return cachedValues; } else { if (fetchedForPrimaries.contains(primaryID)) { return Collections.emptySet(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java index 0f0eb78a69..886c0ecb05 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java @@ -17,7 +17,6 @@ import java.time.Duration; import java.util.Collections; -import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -123,9 +122,8 @@ private void checkAndRegisterTask(P resource) { var primaryID = ResourceID.fromResource(resource); if (scheduledFutures.get(primaryID) == null && (registerPredicate == null || registerPredicate.test(resource))) { - var cachedResources = cache.get(primaryID); - var actualResources = - cachedResources == null ? null : new HashSet<>(cachedResources.values()); + var cachedResources = cachedResourcesFor(primaryID); + var actualResources = cachedResources.isEmpty() ? null : cachedResources; // note that there is a delay, to not do two fetches when the resources first appeared // and getSecondaryResource is called on reconciliation. scheduleNextExecution(resource, actualResources); @@ -167,9 +165,9 @@ public void run() { @Override public Set getSecondaryResources(P primary) { var primaryID = ResourceID.fromResource(primary); - var cachedValue = cache.get(primaryID); - if (cachedValue != null && !cachedValue.isEmpty()) { - return new HashSet<>(cachedValue.values()); + var cachedValues = cachedResourcesFor(primaryID); + if (!cachedValues.isEmpty()) { + return cachedValues; } else { if (fetchedForPrimaries.contains(primaryID)) { return Collections.emptySet(); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java index 024bd95dfc..2aed5539e7 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java @@ -211,6 +211,17 @@ void genericFilteringEvents() { verify(eventHandler, times(0)).handleEvent(any()); } + @Test + void getSecondaryResourcesReturnsASnapshotNotALiveView() { + source.handleResources(primaryID1(), Set.of(testResource1())); + + var snapshot = source.getSecondaryResources(primaryID1()); + source.handleDelete(primaryID1()); + + assertThat(snapshot).containsExactly(testResource1()); + assertThat(source.getSecondaryResources(primaryID1())).isEmpty(); + } + @Test void recentResourceUpdateIsIgnoredForUnknownSecondaryResource() { source.handleResources(primaryID1(), Set.of(testResource1())); From 5b9b6cd70cddc366c233453fbe75a985391b48d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 3 Aug 2026 10:26:42 +0200 Subject: [PATCH 2/2] fix: NPE in external resource caching event source when only a generic filter is set (#3518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `acceptedByFiler` guards each of its three filter branches with `onXFilter != null || genericFilter != null`, but the branch body dereferences `onXFilter` unconditionally: if (onAddFilter != null || genericFilter != null) { ... .anyMatch(r -> acceptedByGenericFiler(r) && onAddFilter.accept(r)); So configuring only a generic filter via `setGenericFilter(...)` throws a NullPointerException as soon as a resource is added, deleted or updated. All three branches (add / delete / update) are affected, which means `PollingEventSource`, `PerResourcePollingEventSource` and `CachingInboundEventSource` all break when used with a generic filter only. The existing `genericFilteringEvents` test missed this because it uses a filter that returns `false`: `&&` short-circuits before the null dereference. Only a generic filter that accepts a resource reaches the NPE. Each filter check is now null-safe (an absent filter accepts), which preserves the previous behaviour whenever the specific filter is set. Adds three regression tests, one per branch; they fail with NullPointerException without this change. Signed-off-by: Attila Mészáros # Conflicts: # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java --- ...xternalResourceCachingEventSourceTest.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java index 2aed5539e7..efd48bb6a2 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java @@ -223,16 +223,6 @@ void getSecondaryResourcesReturnsASnapshotNotALiveView() { } @Test - void recentResourceUpdateIsIgnoredForUnknownSecondaryResource() { - source.handleResources(primaryID1(), Set.of(testResource1())); - - // testResource2 has a different id, so it is not present in the cache for primaryID1 - var unknown = testResource2(); - source.handleRecentResourceUpdate(primaryID1(), unknown, unknown); - - assertThat(source.getSecondaryResources(primaryID1())).containsExactly(testResource1()); - } - void onlyGenericFilterSetDoesNotFailOnAdd() { var eventSource = new TestExternalCachingEventSource(); eventSource.setGenericFilter(res -> true); @@ -243,6 +233,17 @@ void onlyGenericFilterSetDoesNotFailOnAdd() { verify(eventHandler, times(1)).handleEvent(any()); } + @Test + void recentResourceUpdateIsIgnoredForUnknownSecondaryResource() { + source.handleResources(primaryID1(), Set.of(testResource1())); + + // testResource2 has a different id, so it is not present in the cache for primaryID1 + var unknown = testResource2(); + source.handleRecentResourceUpdate(primaryID1(), unknown, unknown); + + assertThat(source.getSecondaryResources(primaryID1())).containsExactly(testResource1()); + } + @Test void onlyGenericFilterSetDoesNotFailOnDelete() { var eventSource = new TestExternalCachingEventSource();