APIGOV-31191 validate cache

[alrosca]

Cache validation on start up and reconnect

[jcollins-axway]

Looks good from a high level, needs pounded testing wise

[jcollins-axway]

copilots review

Code Review: APIGOV-31191 vs main

Summary

• Scope: reviewed 8 files (314 insertions, 4 deletions), focused on new cache validation flow, reconnect hooks, and cache manager interfaces.
• Verdict: request changes
• Severity: medium (2), low (0), high (0), blocker (0)

Findings

• Medium — Cache validation uses non-unique key (name) and can collide across scoped resources

  • Where: pkg/agent/cache/cachevalidation.go, pkg/agent/cache/cachevalidation.go, pkg/agent/cachevalidationjob.go, pkg/agent/cachevalidationjob.go
  • Description: both server and cache summaries are keyed only by resource.Name. If multiple resources share the same name under different scopes, entries overwrite each other, making the comparison unreliable.
  • Possible impacts to the codebase: false negatives (stale cache undetected) or false positives (unnecessary full cache rebuilds), especially in multi-scope environments.

• Medium — Cache validator hard-codes API version to v1alpha1, which can skip/incorrectly query some filters

  • Where: pkg/agent/cachevalidationjob.go, pkg/agent/cachevalidationjob.go
  • Description: validation URL is built from a synthetic ResourceInstance with fixed APIVersion: "v1alpha1". For filters whose resource version differs, GetKindLink() may produce the wrong endpoint or empty link, and validation is skipped (return true).
  • Possible impacts to the codebase: out-of-sync persisted cache may pass validation and remain in use after startup/reconnect for affected resource kinds.

Positives

• Cache validation logic is cleanly encapsulated (cacheValidator) and integrated into both startup and reconnect paths.
• Reconnect hooks were added consistently to poll and stream clients with minimal API surface change (WithOnReconnect options).
• Logging around validation failures includes useful operational context (resource counts, resource names, timestamps).

Recommendations

• Use a stable unique key for comparisons (for example selfLink or a composite of group/kind/scope/name) instead of name alone.
• Derive API version from filter/resource metadata or from server-discovered GVK mappings rather than hard-coding "v1alpha1".
• Add targeted unit tests for: duplicate names across scopes, non-v1alpha1 resources, and reconnect-triggered validation behavior.

[alrosca]

Not a bad review at all

[jcollins-axway]

caching changes require an extra amount of testing as its usually to blame for our duplicate service issues

[sbolosan]

Give me time to look at this closer

[sbolosan]

Dang! @vivekschauhan @jcollins-axway I put this note in a very long time ago ;).
I was looking at another race condition down stream, but decided to see who called RebuildCache().

So RebuildCache is reachable by WithOnClientStop (poller) and With EventSyncError (stream) while the event listener go routine is alive, well technically alive.

In those paths the race is meh, maybe not a big deal because event flow has stopped like a harvester error or when the stream is not yet producing. But PublishingLock still doesn't coordinate with the listener. Maybe we should take a look at this again?

[sbolosan]

should we eat the error?
At least log the error/warn and then if requirement is to rebuild, then rebuild?

[sbolosan]

maybe something like

    timeToRebuild, err := a.shouldRebuildCache()
    if err != nil {
        a.logger.WithError(err).Warn("unable to determine cache rebuild state, triggering rebuild")
        timeToRebuild = true
    }
    if timeToRebuild && a.rebuildCache != nil {
        a.rebuildCache.RebuildCache()
    }

[alrosca]

I think we can skip the error logging outside shouldRebuildCache since we already log it inside the method. Returning true on error inside the method would not require checking the return error and explicitly putting timeToRebuild on true

[sbolosan]

I know this is old and not part of your changes, but can we do a safe guard here

strVal, ok := value.(string)
            if !ok {
                logger.Warn("cacheUpdateTime is not a string, triggering rebuild")
                return true, nil
            }

[alrosca]

Do we need this defensive code since in pkg/agent/evensync.go, line 162 that value is put in agent details and will always be a string? I don't think there are permissions to alter the agent details subresource from cli and I don't think anyone would do that, right?

[jasonmscollins]

See comments from Shane

[jcollins-axway]

see comments