Grafana PR #8 — Live Benchmark — FriendlyReviewer

Golden Comments (1/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Missing double-check in locking pattern — the write lock is acquired without re-verifying whether the cache was populated in the meantime	MEDIUM	✓ Found	FriendlyReviewer: "Missing double-check inside write-locked section. All goroutines that pass the initial nil check will re-read the manifest serially, defeating caching." (webassets.go L40) — severity upgraded from Medium to High.
#2	Nil cache overwrite on error — `entryPointAssetsCache = result` assigns unconditionally (even nil), overwriting valid cached data permanently	HIGH	✗ Missed	FriendlyReviewer did not detect this issue. The unconditional assignment `entryPointAssetsCache = result` skips the error check from `GetWebAssets`. If the function fails, `result` is nil and the cache is permanently corrupted — no subsequent request will retrieve assets until the server restarts. This is the most critical bug in the PR.

Supplementary Findings (1 finding)

Finding	File	Severity	Legitimate?
Mutable pointer returned from cache — `return entryPointAssetsCache` returns a `*dtos.EntryPointAssets` that callers can mutate (e.g., append to `JSFiles`), corrupting the cache for all subsequent requests	webassets.go:52	MEDIUM	✓ True bug — callers can inadvertently corrupt cached data without synchronization

Honest Analysis

FriendlyReviewer correctly identified golden #1 (missing double-check) with a severity upgrade from Medium to High. The description is precise: all goroutines passing the initial nil check enter the write-locked section and re-read the manifest serially, defeating the purpose of caching entirely. This is a textbook double-checked locking bug, and FriendlyReviewer caught it exactly.

However, FriendlyReviewer missed golden #2, which is the most critical issue in the PR. The unconditional assignment entryPointAssetsCache = result happens before checking whether GetWebAssets returned an error. If the fetch fails, result is nil, and the cache is permanently overwritten with nil — no subsequent request will ever retrieve valid assets until the server restarts. This is a silent data-loss bug with no recovery path. FriendlyReviewer's analysis of the locking pattern was thorough, but it did not trace the error-return path through the assignment, which is arguably the more dangerous defect.

The one supplementary finding (mutable pointer returned from cache) is legitimate and valuable. FriendlyReviewer identified that callers receiving *dtos.EntryPointAssets can mutate the slices (JSFiles, CSSFiles) directly, corrupting the shared cache without any synchronization. This is a different race condition than the one the PR was trying to fix, and the golden comments did not account for it.

For a 16-line diff in a single file, FriendlyReviewer's analysis is focused but incomplete. It found the locking bug well but missed the error-handling bug that is functionally more severe. The supplementary finding adds genuine value beyond the golden set. The combination of the two golden bugs and the supplementary finding together provide a complete picture of what is wrong with this code, but no single review source — human or automated — caught everything.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
1 high, 1 medium

The changes introduce a sync.RWMutex to protect the global entryPointAssetsCache, replacing unsynchronized access. However, the double-checked locking implementation is broken – the re-check inside the write-locked section is missing, causing redundant manifest reads under concurrent load. This is a critical correctness issue. Additionally, returning a mutable pointer to the cached data risks cache corruption if callers mutate the returned struct.

These might need a close look

🔴 pkg/api/webassets/webassets.go (L40)
Critical: Missing double-check inside write-locked section. The classic double-checked locking pattern requires a second check of the cache after acquiring the exclusive write lock to prevent redundant manifest reads. Currently, all goroutines that pass the initial nil check will re-read the manifest serially, defeating caching.

Worth checking

🟡 pkg/api/webassets/webassets.go (L52)
Medium: Returning a mutable pointer (*dtos.EntryPointAssets) from the cache. Callers could mutate the cached data (e.g., append to JSFiles), corrupting the cache for all subsequent requests. Consider returning a deep copy or storing an immutable value type.