Fix pinned ref cache miss to avoid policy refetch

[simonbaird]

URL pinning mutates p.Url after the first GetPolicy call (e.g. github.com/org/repo -> git::github.com/org/repo?ref=abc123), causing subsequent cache lookups to miss and re-download into a second directory. OPA then sees duplicate packages and errors out. Fix by also storing the cached download function under the pinned URL.

I noticed this problem when testing the new http service with a git url in the policy.yaml.

Ref: https://redhat.atlassian.net/browse/EC-2017

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 85a2c2a4-a2aa-4fad-951e-3c1f7bedd502

📥 Commits

Reviewing files that changed from the base of the PR and between 01e1e95 and 2d19ccd.

📒 Files selected for processing (3)

• internal/policy/source/source.go
• internal/policy/source/source_test.go
• internal/server/server.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/policy/source/source.go

---

📝 Walkthrough

Walkthrough

The changes synchronize PolicyUrl URL access, transfer download-cache entries when pinning changes the URL, add coverage for repeated pinned lookups, standardize cache cleanup in tests, and include the application version in server startup logs.

Changes

Policy cache consistency

Layer / File(s)	Summary
Synchronized URL pinning and cache transfer internal/policy/source/source.go	PolicyUrl.GetPolicy protects URL reads and updates with a mutex, transfers cache entries from the original URL to the pinned URL, and uses the synchronized accessor.
Cache consistency test coverage internal/policy/source/source_test.go	Tests use ClearDownloadCache() for cleanup and verify repeated pinned lookups reuse one destination directory and one downloader call.

Startup version logging

Layer / File(s)	Summary
Startup version field internal/server/server.go	Server startup structured logs now include version.Version.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GetPolicy
  participant Downloader
  participant DownloadCache
  GetPolicy->>Downloader: Pin and download policy
  Downloader-->>GetPolicy: Return pinned URL and destination
  GetPolicy->>DownloadCache: Copy entry to pinned URL
  GetPolicy->>DownloadCache: Reuse cached destination

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: preventing cache misses caused by pinned policy refs.
Description check	✅ Passed	The description covers what, why, and a Jira ticket link, matching the required template content.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-conforma]

PR Summary by Qodo

Fix pinned URL cache miss by aliasing download cache entries

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Prevent downloadCache misses after URL pinning mutates PolicyUrl.Url.
• Reuse the first download directory instead of re-downloading into a second path.
• Add regression test to ensure pinned URLs still hit the cache and avoid OPA duplicates.

Diagram

graph TD
  A["PolicyUrl.GetPolicy"] --> B["getPolicyThroughCache"] --> C[("downloadCache")]
  B --> D["Downloader"]
  A --> E["metadata.GetPinnedURL"] --> F["Pinned URL"]
  C --> G["Policy directory"] --> H["OPA loader"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Stop mutating PolicyUrl.Url (store pinned URL separately)

• ➕ Avoids hidden side effects that change cache keys mid-lifecycle
• ➕ Makes it explicit which URL is used for display vs. caching vs. download
• ➖ Requires broader API/struct changes (more call sites to update)
• ➖ May be harder to thread through existing interfaces expecting PolicyUrl() == effective URL

2. Canonicalize cache key (always key by pinned URL)

• ➕ Single canonical key; no aliasing/duplication in cache map
• ➕ Ensures different callers converge on the same cache entry
• ➖ Still needs a way to map the first (unpinned) call to the pinned key
• ➖ May require pinning earlier (before first cache lookup), impacting behavior/latency

Recommendation: The chosen aliasing approach is a good low-impact fix: it preserves current behavior (pinning after first fetch) while preventing the post-mutation cache miss. Longer-term, consider separating original vs. pinned URL fields to eliminate the class of “mutating key” issues entirely, but that would be a larger refactor than warranted for this bug fix.

Files changed (2) +51 / -0

Bug fix (1) +8 / -0

source.goAlias cache entry after URL pinning to prevent cache-key drift +8/-0

Alias cache entry after URL pinning to prevent cache-key drift

• Captures the original URL before pinning and, after a successful pin, stores the existing cached download function under the pinned URL key as well. This keeps subsequent GetPolicy calls (which see mutated p.Url) hitting the original cache entry instead of re-downloading into a second directory.

internal/policy/source/source.go

Tests (1) +43 / -0

source_test.goAdd regression test for pinned URL cache consistency +43/-0

Add regression test for pinned URL cache consistency

• Adds a test that calls PolicyUrl.GetPolicy twice and asserts the URL becomes pinned after the first call while the second call reuses the same cached directory. Also verifies only one policy directory exists and the downloader is invoked once.

internal/policy/source/source_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:54 PM UTC · Completed 9:05 PM UTC
Commit: 87c4a29 · View workflow run →

[qodo-for-conforma]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 27 rules

1. PolicyUrl.Url data race ✓ Resolved 🐞 Bug ☼ Reliability

Description

PolicyUrl.GetPolicy now reads p.Url (originalUrl := p.Url) outside of pinOnce.Do while
pinOnce.Do writes p.Url, so concurrent GetPolicy calls can hit a read/write data race. This
violates the evaluator contract that instances are safe for concurrent use and can lead to
nondeterministic behavior under load (and will be flagged by the Go race detector).

Code

internal/policy/source/source.go[R191-201]

+	originalUrl := p.Url
	p.pinOnce.Do(func() {
		p.Url, pinErr = metadata.GetPinnedURL(p.Url)
		log.Debug("Pinned URL: ", p.Url)
+		// Register the cached download under the pinned URL too, so
+		// subsequent lookups (which use the now-mutated p.Url) still hit.
+		if pinErr == nil && p.Url != originalUrl {
+			if cached, ok := downloadCache.Load(originalUrl); ok {
+				downloadCache.LoadOrStore(p.Url, cached)
+			}
+		}

Relevance

⭐⭐⭐ High

PR #3386 explicitly accepted fixing PolicyUrl.Url data race for concurrent evaluators; team treats
this as real bug.

PR-#3386
PR-#2964

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a new unsynchronized read of p.Url (originalUrl := p.Url) and pinOnce.Do still
writes p.Url. Evaluators are explicitly shared across concurrent requests, and the conftest
evaluator calls GetPolicy on stored policySources during Evaluate, so concurrent access to the
same *PolicyUrl is expected. This is the same class of bug previously accepted as a real issue in
this area.

internal/policy/source/source.go[169-208]
internal/evaluator/evaluator.go[30-33]
internal/evaluator/conftest_evaluator.go[198-212]
internal/evaluator/conftest_evaluator.go[416-448]
PR-#3386

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PolicyUrl.GetPolicy` reads `p.Url` (new `originalUrl := p.Url`) outside the `pinOnce.Do` critical section while `pinOnce.Do` writes `p.Url`. When evaluators are shared across concurrent HTTP requests, concurrent `GetPolicy` calls can therefore read/write `PolicyUrl.Url` concurrently, producing a Go data race.

## Issue Context
The evaluator interface explicitly requires implementations be safe for concurrent use (the validate server shares evaluator instances across requests). `conftestEvaluator` holds `policySources` and calls `GetPolicy` during `Evaluate`, so the same `*PolicyUrl` can be accessed by multiple goroutines.

## Fix Focus Areas
- internal/policy/source/source.go[169-208]
- internal/evaluator/evaluator.go[30-33]
- internal/evaluator/conftest_evaluator.go[198-212]
- internal/evaluator/conftest_evaluator.go[416-448]

## Suggested fix
1. Add synchronization for all accesses to `PolicyUrl.Url` (reads and writes), e.g. a `sync.RWMutex` field in `PolicyUrl`.
  - Use `RLock/RUnlock` in `PolicyUrl.PolicyUrl()` and anywhere else `p.Url` is read.
  - Use `Lock/Unlock` around the assignment to `p.Url` inside `pinOnce.Do`.
2. Prefer reading the URL via a single helper (e.g. `func (p *PolicyUrl) url() string`) to avoid missing any read sites.
3. Keep the cache-aliasing logic, but make sure both `originalUrl` and the pinned `p.Url` values used for cache operations are captured under the same synchronization so they cannot race with concurrent callers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Findings

Low

• [consistency] internal/policy/source/source_test.go:641 — The new test uses t.Cleanup(ClearDownloadCache) while all other tests in this file use the inline form t.Cleanup(func() { downloadCache = sync.Map{} }). Notably, ClearDownloadCache() also resets symlinkMutexes in addition to downloadCache, so the two patterns are not semantically identical — the new test actually cleans up more state, which is arguably more correct.
  Remediation: Either update this test to use the inline form to match sibling tests, or (preferred) update all sibling tests to use ClearDownloadCache for consistency and more thorough cleanup.

---

Labels: PR fixes a cache consistency bug in Go policy source code

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/policy/source/source.go	92.85%	1 Missing ⚠️

Flag	Coverage Δ
acceptance	54.31% <93.33%> (+0.05%)	⬆️
generative	16.78% <0.00%> (-0.02%)	⬇️
integration	27.94% <0.00%> (-0.03%)	⬇️
unit	71.76% <86.66%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/server/server.go	87.50% <100.00%> (+0.17%)	⬆️
internal/policy/source/source.go	94.48% <92.85%> (+0.47%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:16 PM UTC · Completed 9:50 PM UTC
Commit: 87c4a29 · View workflow run →