Skip to content

Conversation

@zaibkhan
Copy link

@zaibkhan zaibkhan commented Sep 9, 2025

Mirrors ai-code-review-evaluation#8 for like-for-like benchmarking.

  • Base: asset-loading-baseline
  • Head: asset-loading-optimized

Original PR excerpt:

Test 8

@codoki-pr-intelligence
Copy link

codoki-pr-intelligence bot commented Sep 9, 2025

Codoki PR Review

Summary: Add RWMutex around cache, prevent data races
What’s good: Global asset cache is now protected with RWMutex, eliminating potential data races under concurrent access.
Review Status: ✅ Safe to merge

Next Steps:

  • 🔷 Medium: Review pkg/api/webassets/webassets.go (see inline suggestions)
Key Feedback (click to expand)
  • Needs improvement: Add a second check after acquiring the write lock to avoid redundant manifest loading and narrow error handling for CDN responses.
  • Testing: Consider a concurrency test that fires multiple parallel GetWebAssets calls and asserts only one manifest read occurs (e.g., via a stubbed reader or counter).
  • Performance: Currently the write lock is held while performing I/O and may repeat work on the second goroutine; adding an in-lock re-check avoids duplicate work and reduces latency under bursty concurrency.
  • Open questions: Is GetWebAssets invoked frequently on hot paths? If so, do we want to avoid I/O under the write lock (e.g., via singleflight) for better scalability?

Confidence: 4/5 — Looks good; minor fixes (2 medium)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant WebAssets
    participant CDN
    participant FS
    Caller->>WebAssets: GetWebAssets(ctx, cfg, license)
    alt cache hit (non-dev)
        WebAssets-->>Caller: return cached EntryPointAssets
    else cache miss or dev
        opt try CDN
            WebAssets->>CDN: GET assets-manifest.json
            CDN-->>WebAssets: manifest body
            WebAssets->>WebAssets: readWebAssets(body)
        end
        alt result == nil
            WebAssets->>FS: read assets-manifest.json
            FS-->>WebAssets: manifest body
            WebAssets->>WebAssets: readWebAssets(body)
        end
        WebAssets-->>Caller: EntryPointAssets
    end
Loading

React with 👍 or 👎 if you found this review useful.

if cfg.Env != setting.Dev && ret != nil {
return ret, nil
}
entryPointAssetsCacheMu.Lock()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Add a second check after acquiring the write lock to avoid redundant manifest loading under concurrent calls; the current flow can perform the entire I/O twice (first goroutine computes, second acquires the lock afterward and computes again).

suggestion
if cfg.Env != setting.Dev && entryPointAssetsCache != nil {
entryPointAssetsCacheMu.Unlock()
return entryPointAssetsCache, nil
}
defer entryPointAssetsCacheMu.Unlock()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants