feat: modelcatalog main composer added - #4192
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 3/5Safe to merge after fixing the URL snapshot inconsistency in loadPricingFromURL — the rest of the refactor is structurally sound. The URL snapshotting flaw in loadPricingFromURL is a concrete defect: the function validates rawURL but then hands s.URL() (a fresh live read) to the HTTP client. A concurrent UpdateSyncConfig call between those two lines would cause a request to go to an unvalidated URL. The companion loadModelParametersFromURL does this correctly, making the inconsistency easy to spot and fix. The Init goroutine-drain gap on error paths is real but bounded — the goroutines self-terminate via context cancellation and the window is only open when Init itself fails. framework/modelcatalog/datasheet/sync.go (URL snapshot/request mismatch) and framework/modelcatalog/main.go (goroutine drain on error paths). Important Files Changed
Reviews (1): Last reviewed commit: "feat: modelcatalog main composer added" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return nil, fmt.Errorf("pricing URL validation failed: %w", err) | ||
| } | ||
| client := &http.Client{Timeout: DefaultPricingTimeout} | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, s.URL(), nil) |
There was a problem hiding this comment.
loadPricingFromURL is built with s.URL() — a fresh lock-guarded read — instead of the already-snapshotted rawURL. The snapshot was taken precisely to avoid TOCTOU: rawURL is validated by bifrost.ValidateExternalURL, but if UpdateSyncConfig swaps the URL between that call and the http.NewRequestWithContext call, the request will go to the new, unvalidated URL. loadModelParametersFromURL handles this correctly by using rawURL for its request; this function should match.
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, s.URL(), nil) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) |
| @@ -211,131 +183,122 @@ func Init(ctx context.Context, config *Config, configStore configstore.ConfigSto | |||
| return nil, paramsErr | |||
| } | |||
There was a problem hiding this comment.
The two mc.wg.Add(1) background-startup goroutines (lines ~131–141 and ~159–169) are added to mc.wg before wg.Wait() returns. If Init returns an error after wg.Wait() (e.g., LoadOverridesFromStore fails at line 196), mc.syncCancel() is invoked by the deferred guard, causing the goroutines to self-terminate — but mc.wg.Wait() is never called. The caller receives nil, err and can never call Cleanup(), so there is no caller-visible wait for those goroutines to finish. The leak is bounded (they exit when the context is cancelled) but the process can shut down or the test harness can exit while they are still holding the distributed lock.
2 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines