Repository navigation
Correctness AI check: collection of possible issues and related PRs #3517
Description
An audit of the codebase turned up 15 correctness bugs plus one cosmetic nit.
Each is fixed by its own PR, linked below; the reasoning, impact and (where
present) the regression test live in the individual commit messages.
SpotBugs (effort=Max, threshold=Medium) reports zero findings across all
modules, so none of these are reachable by the existing static analysis.
Confirmed with regression tests
Each of these ships a test that fails without the fix and passes with it.
| Area | Bug |
|---|---|
ExternalResourceCachingEventSource |
Configuring only a generic filter throws NPE on add/delete/update — the branch guard is || but the body dereferences the specific filter. Affects all polling and inbound event sources. |
ExternalResourceCachingEventSource |
handleRecentResourceUpdate NPEs when the primary is cached but the secondary id is not. |
AbstractDependentResource / bulk |
A BulkDependentResource that is not a Creator/Updater still gets create/update attempted, failing with ClassCastException. The creatable()/updatable() hooks added for this were never wired up. |
LinearRateLimiter |
isLimited returns time elapsed instead of time remaining, so rate-limited resources are rescheduled almost immediately (burst of timer events) and then wait roughly a full extra period. |
MicrometerMetricsV2 |
processing.started.latency is registered against a boxed Long that nothing holds a strong reference to, so the gauge always reports NaN. |
PollingEventSource |
The timer is a non-daemon Timer created in a final field, so it keeps the JVM alive and the event source cannot be restarted (IllegalStateException: Timer already cancelled). |
ExternalResourceCachingEventSource |
Cache reads are unsynchronized while every mutator is synchronized, and getSecondaryResources looks the primary up twice (TOCTOU NPE). |
EventProcessor |
When the primary is gone from cache at execution time the submission is never completed: the resource stays flagged as under processing (never reconciled again) and the in-flight metrics gauges drift. |
ExecutorServiceManager |
The scheduled executor is never shut down; on interrupt the helper pool leaks and started stays true, leaving a "started" operator whose executors are all terminated. |
Fix only
Covered by integration tests only, or needing clock/HTTP-layer control that the
unit tests are not set up for. Each commit says which.
| Area | Bug |
|---|---|
KubernetesResourceFetcher |
Optional.orElse eagerly evaluates the cluster-scoped fallback, so every namespaced bounded-cache refresh issues a second, wrong-scope API request. |
AbstractExternalDependentResource |
selectTargetSecondaryResource calls desired() directly instead of getOrComputeDesired(), breaking the once-per-reconciliation contract. |
PrimaryUpdateAndCacheUtils |
pollLocalCache builds its deadline from LocalTime, which wraps at midnight — a 10s window each day where the poll times out instantly. |
ResourceOperations |
addFinalizer's precondition NPEs if the resource is deleted mid-retry; removeFinalizer already guards for this. |
Controller |
The constructor's null-Metrics guard is bypassed on the last line, which calls the getter again. |
EventSourceManager |
Cosmetic: a log message has one placeholder too many, rendering a literal {}. (Originally reported as a lost stack trace — that was wrong, SLF4J extracts a trailing Throwable regardless of placeholder count.) |
| assorted | ManagedInformerEventSource.get can throw on a null last-sync resource version or an unwatched namespace; ExpectationResult.name() throws a bare NPE; an unused parameter and a misleading comment in ResourceOperations. |
Notes
- The branches are based on a commit slightly behind
mainand may need a rebase. - The PRs are opened as drafts; they reference this issue with "Part of", not a
closing keyword, so merging one will not close this issue.
Pull requests
- fix: NPE in external resource caching event source when only a generic filter is set #3518
- fix: NPE in handleRecentResourceUpdate for an unknown secondary resource #3519
- fix: honour bulk dependent resource capabilities during reconciliation #3520
- fix: report remaining, not elapsed, time from LinearRateLimiter #3521
- fix: keep a strong reference to the processing started latency gauge #3522
- fix: create the PollingEventSource timer on start and make it a daemon #3523
- fix: read the external resource cache under the event source monitor #3524
- fix: complete the submission when the primary is missing from cache at execution time #3525
- fix: always release executors and reset state when stopping the executor manager #3526
- fix: do not eagerly evaluate the cluster-scoped fallback in KubernetesResourceFetcher #3527
- fix: use the cached desired state when selecting an external secondary resource #3528
- fix: use a monotonic clock for the cache polling timeout #3529
- fix: do not NPE when adding a finalizer to a resource deleted mid-retry #3530
- fix: use the null-guarded metrics field in the Controller constructor #3531
- fix: remove dangling placeholder from event source close warning #3532
- fix: assorted robustness and clarity fixes #3533
Activity