Frontend Asset Optimization

[hussam789]

User description

PR #8

---

PR Type

Bug fix

---

Description

• Add sync.RWMutex to protect entryPointAssetsCache global variable

• Fix data race condition in GetWebAssets function

• Implement read-lock for cache reads and write-lock for updates

• Add TODO comment to remove global state in future

---

Diagram Walkthrough

flowchart LR
  A["GetWebAssets function"] -->|"acquire RLock"| B["Read cache safely"]
  B -->|"cache hit"| C["Return cached result"]
  B -->|"cache miss"| D["Acquire Lock"]
  D -->|"compute assets"| E["Update cache"]
  E -->|"release Lock"| F["Return result"]

Loading

File Walkthrough

Relevant files

Bug fix

webassets.go Add mutex protection to web assets cache                                  pkg/api/webassets/webassets.go Import sync package for synchronization primitives Replace single global cache variable with guarded variable using sync.RWMutex Implement read-lock pattern for cache reads to prevent data races Implement write-lock pattern for cache updates with defer unlock Add TODO comment indicating need to eliminate global state +13/-3

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Concurrency correctness Description: Possible lock upgrade pattern where a cached value is read under RLock and rechecked only after acquiring Lock without verifying it wasn't updated concurrently, which could allow multiple goroutines to enter the recomputation path unnecessarily (double-check without revalidation). webassets.go [41-50] Referred Code entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock()
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new cache read/write paths add concurrency control around serving web assets but do not log critical actions (e.g., cache hits/misses or updates), making it unclear if these actions are auditable. Referred Code entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock()
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unclear error paths: The added locking and early return change control flow around caching without confirming that cache-miss paths and potential errors during asset computation are logged or handled with context in this diff. Referred Code entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock() var err error var result *dtos.EntryPointAssets
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error exposure unknown: The diff does not show user-facing error handling changes; without visibility into returned errors from this function, it is unclear whether sensitive details could be exposed. Referred Code var err error var result *dtos.EntryPointAssets
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: No logging context: The new cache logic does not add structured logs for cache decisions or errors, and the diff provides no evidence that logs (if any elsewhere) avoid sensitive data. Referred Code entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock()

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Use sync.Once for one-time initialization The current RWMutex implementation has a double-checked locking bug, allowing concurrent re-computation of assets. Replace it with sync.Once for a safer and more idiomatic one-time initialization. Examples: pkg/api/webassets/webassets.go [40-71] func GetWebAssets(ctx context.Context, cfg *setting.Cfg, license licensing.Licensing) (*dtos.EntryPointAssets, error) { entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock() ... (clipped 22 lines) Solution Walkthrough: Before: var ( entryPointAssetsCacheMu sync.RWMutex entryPointAssetsCache *dtos.EntryPointAssets ) func GetWebAssets(...) (*dtos.EntryPointAssets, error) { entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock() // ... asset computation logic ... // This part is executed by multiple goroutines if they all miss the cache. entryPointAssetsCache = result return entryPointAssetsCache, err } After: var ( assetsOnce sync.Once entryPointAssetsCache *dtos.EntryPointAssets initErr error ) func GetWebAssets(...) (*dtos.EntryPointAssets, error) { if cfg.Env == setting.Dev { return computeWebAssets(...) // Always recompute in dev } assetsOnce.Do(func() { // This function is guaranteed to run exactly once. entryPointAssetsCache, initErr = computeWebAssets(...) }) return entryPointAssetsCache, initErr } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a subtle but important double-checked locking bug in the PR's logic and proposes using sync.Once as a more idiomatic, simpler, and safer solution for one-time initialization.	High
Possible issue	Fix race condition in caching To fix a race condition in the double-checked locking implementation, re-check if the cache is populated immediately after acquiring the write lock. pkg/api/webassets/webassets.go [41-70] entryPointAssetsCacheMu.RLock() ret := entryPointAssetsCache entryPointAssetsCacheMu.RUnlock() if cfg.Env != setting.Dev && ret != nil { return ret, nil } entryPointAssetsCacheMu.Lock() defer entryPointAssetsCacheMu.Unlock() + // Re-check after acquiring write lock to prevent race condition + if entryPointAssetsCache != nil { + return entryPointAssetsCache, nil + } + var err error var result *dtos.EntryPointAssets ... } entryPointAssetsCache = result return entryPointAssetsCache, err [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a classic race condition in the implemented double-checked locking pattern, which could lead to redundant work. The proposed fix is the standard and correct solution for this problem.	Medium
More