fix(recommendations): apply per-account override when no global ServiceConfig exists (closes #202)#241
Conversation
…viceConfig absent (closes #202) ResolveAccountConfigsForRecs previously skipped a triple entirely when GetServiceConfig returned nil, even if a per-account AccountServiceOverride existed. The override was silently dropped, causing the recommendation list to appear unfiltered for that account — contrary to what the operator configured. Fix: - ResolveServiceConfig now accepts provider and service as explicit parameters and handles a nil global by synthesising a safe baseline (Enabled: true) before applying the override. All scalar/slice fields from the override are merged as before. - ResolveAccountConfigsForRecs no longer short-circuits the override lookup when the global is absent. The triple is only skipped when *both* global and override are nil (nothing to apply). Tests: - TestResolveServiceConfig_NilGlobalWithOverride — override without global synthesises correct baseline and applies override fields. - TestResolveServiceConfig_BothNil — both nil returns nil. - TestResolveAccountConfigsForRecs_OverrideWithoutGlobal_OverrideApplied — regression test for #202: override-without-global entry lands in the map with correct values. - TestResolveAccountConfigsForRecs_NoGlobalNoOverride_TripleSkipped — renamed from GlobalAbsent; semantics clarified (skip only when both absent). - TestResolveAccountConfigsForRecs_OverrideWithGlobal_MergesCorrectly — regression test for existing #200 merge path. - TestResolveAccountConfigsForRecs_GlobalAbsentCachedNegative — updated to assert overrideCalls=3 (one per account; previously 0).
cristim
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughResolveAccountConfigsForRecs now memoizes global ServiceConfig lookups, deduplicates by (account, provider, service), and only omits a triple when both global and per-account override are absent. ResolveServiceConfig now accepts provider/service and will synthesize a baseline global when an override exists but global is nil. ChangesResolve & Caching Flow
Sequence Diagram(s)sequenceDiagram
participant RecsLoop as Recs Loop
participant Cache as globalConfigCache
participant Store as Config Store
participant Resolver as ResolveServiceConfig
RecsLoop->>Cache: lookup(provider, service)
alt cache miss
Cache->>Store: GetServiceConfig(provider, service)
Store-->>Cache: ServiceConfig or nil
end
RecsLoop->>Store: GetAccountServiceOverride(account, provider, service)
RecsLoop->>Resolver: ResolveServiceConfig(provider, service, global?, override?)
Resolver-->>RecsLoop: resolved ServiceConfig or nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/recommendation_overrides.go (1)
102-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDedup skipped triples independently of
resolved.Using
resolvedas the seen-set means Line 119 skips without recording the triple. For repeated recs with the same(account, provider, service)and no global/override,GetAccountServiceOverridewill run once per rec instead of once per triple, which adds unnecessary store traffic on a hot path. A separateseenmap fixes this and keeps the dedupe contract true even when nothing is written to the result.Proposed fix
func ResolveAccountConfigsForRecs( ctx context.Context, store AccountConfigReader, recs []RecommendationRecord, ) (map[string]*ServiceConfig, error) { resolved := make(map[string]*ServiceConfig) + seenTriples := make(map[string]struct{}) if len(recs) == 0 { return resolved, nil } @@ accountID := *rec.CloudAccountID key := AccountConfigKey(accountID, rec.Provider, rec.Service) - if _, seen := resolved[key]; seen { + if _, seen := seenTriples[key]; seen { continue } + seenTriples[key] = struct{}{} global, err := cache.lookup(ctx, store, rec.Provider, rec.Service) if err != nil { return resolved, err } @@ if global == nil && override == nil { continue } resolved[key] = ResolveServiceConfig(rec.Provider, rec.Service, global, override) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/recommendation_overrides.go` around lines 102 - 125, The loop currently uses the result map `resolved` to deduplicate triples, so when a triple has no global and no override it gets skipped without recording and can be processed repeatedly; introduce a separate seen set (e.g., `seen := map[string]struct{}{}`) and replace the `if _, seen := resolved[key]; seen { continue }` check with `if _, ok := seen[key]; ok { continue }`, and ensure you set `seen[key] = struct{}{}` as soon as you decide to skip or process the triple (i.e., before `continue` and also before calling `ResolveServiceConfig`/writing into `resolved`), leaving calls to `cache.lookup`, `store.GetAccountServiceOverride`, and `ResolveServiceConfig` unchanged except for using the new dedupe map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/config/recommendation_overrides.go`:
- Around line 102-125: The loop currently uses the result map `resolved` to
deduplicate triples, so when a triple has no global and no override it gets
skipped without recording and can be processed repeatedly; introduce a separate
seen set (e.g., `seen := map[string]struct{}{}`) and replace the `if _, seen :=
resolved[key]; seen { continue }` check with `if _, ok := seen[key]; ok {
continue }`, and ensure you set `seen[key] = struct{}{}` as soon as you decide
to skip or process the triple (i.e., before `continue` and also before calling
`ResolveServiceConfig`/writing into `resolved`), leaving calls to
`cache.lookup`, `store.GetAccountServiceOverride`, and `ResolveServiceConfig`
unchanged except for using the new dedupe map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0dad4b1-ad33-4706-bc58-0c5353b34a83
📒 Files selected for processing (4)
internal/config/recommendation_overrides.gointernal/config/recommendation_overrides_test.gointernal/config/resolver.gointernal/config/resolver_test.go
Using resolved as the seen-set meant triples with no global and no override were never recorded, causing redundant store lookups on duplicate recs for the same (account, provider, service) triple. Introduce a dedicated seen map that records every triple on first visit, regardless of whether it writes to resolved. Also add TestResolveAccountConfigsForRecs_NoGlobalNoOverride_DedupedCorrectly as a regression guard.
|
Fixed the CR finding: introduced a dedicated seen map to dedup triples independently of resolved. Added regression guard test. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Return the same timeout-shaped error when the migration goroutine observes the bounded context deadline before the select timeout branch wins. This keeps runMigrationsBounded deterministic under CI scheduling.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Addressed the pre-commit failure in 0751f5a by normalizing runMigrationsBounded context deadline errors into the existing migration timeout error shape. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/app.go (1)
215-225:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one timeout source here.
time.After(timeout)can still win the race and return the unwrapped timeout error, so this change does not fully normalize the result. Switching the select tomigCtx.Done()/migCtx.Err()would make the timeout path deterministic and preserve the underlying deadline cause.♻️ Suggested adjustment
select { case err := <-done: if errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("migration timed out after %s: %w", timeout, err) } return err - case <-time.After(timeout): - cancelMig() + case <-migCtx.Done(): <-done - return fmt.Errorf("migration timed out after %s", timeout) + return fmt.Errorf("migration timed out after %s: %w", timeout, migCtx.Err()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/app.go` around lines 215 - 225, The select currently races between done and time.After(timeout) causing an unwrapped timeout path; replace the time.After branch with a branch that waits on migCtx.Done() and inspects migCtx.Err() so the code deterministically uses the migration context deadline as the timeout source. In the select that references the done channel and cancelMig(), change the second case to <-migCtx.Done(), call cancelMig() if needed, then read <-done and return migCtx.Err() (or wrap it like you do for context.DeadlineExceeded) so the returned error preserves the underlying context deadline error instead of an unwrapped timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/server/app.go`:
- Around line 215-225: The select currently races between done and
time.After(timeout) causing an unwrapped timeout path; replace the time.After
branch with a branch that waits on migCtx.Done() and inspects migCtx.Err() so
the code deterministically uses the migration context deadline as the timeout
source. In the select that references the done channel and cancelMig(), change
the second case to <-migCtx.Done(), call cancelMig() if needed, then read <-done
and return migCtx.Err() (or wrap it like you do for context.DeadlineExceeded) so
the returned error preserves the underlying context deadline error instead of an
unwrapped timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5afeff61-8485-4492-9d15-a97ecaa0d6af
📒 Files selected for processing (3)
internal/config/recommendation_overrides.gointernal/config/recommendation_overrides_test.gointernal/server/app.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/recommendation_overrides_test.go
Select on the migration context deadline instead of a separate timer so runMigrationsBounded consistently wraps the underlying deadline error.
|
Addressed latest CR comment in d5ce72a: runMigrationsBounded now uses migCtx.Done() as the timeout source and wraps migCtx.Err(), removing the separate time.After race. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Both CR items are already addressed in prior commits on this branch: Actionable item (recommendation_overrides.go dedup): Fixed in commit a94fdb8 — introduced a separate Outside-diff item (internal/server/app.go timeout race): Also already fixed — commit d5ce72a replaced No new commits required — everything CR flagged is resolved. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Kicking off a fresh review now. [review] |
1 participant
Summary
AccountServiceOverridewhen no globalServiceConfigrow exists for the same(provider, service)pair (closes fix(recommendations): per-account override silently ignored when no global ServiceConfig exists #202)ResolveServiceConfigextended to handle a nil global by synthesising a safe default baseline (Enabled: true;Provider/Servicefrom call parameters) before applying override fieldsResolveAccountConfigsForRecsrefactored: global-fetch logic extracted into aglobalConfigCachehelper (reduces cyclomatic complexity), override lookup now always runs even when global is absent, triple skipped only when both global and override are nilTest plan
TestResolveServiceConfig_NilGlobalWithOverride— override-without-global synthesises correct baseline and applies override fieldsTestResolveServiceConfig_BothNil— both nil returns nil (no panic)TestResolveAccountConfigsForRecs_OverrideWithoutGlobal_OverrideApplied— regression test for fix(recommendations): per-account override silently ignored when no global ServiceConfig exists #202: entry lands in the map with correct valuesTestResolveAccountConfigsForRecs_NoGlobalNoOverride_TripleSkipped— renamed + semantics clarified (skip only when both absent)TestResolveAccountConfigsForRecs_OverrideWithGlobal_MergesCorrectly— regression guard for existing feat(recommendations): apply per-account overrides at read time (#196) #200 merge pathTestResolveAccountConfigsForRecs_GlobalAbsentCachedNegative— updated: overrideCalls=3 (one per account); global still fetched only oncego vetclean;gofmtclean; cyclomatic complexity within limitSummary by CodeRabbit
Refactor
Tests
Bug Fix