test(azure): concurrency timing test for parallel dispatcher (closes #262) by cristim · Pull Request #873 · LeanerCloud/CUDly

cristim · 2026-05-30T17:43:38Z

Summary

Add serviceRecsGetter interface + 4 newXxxClientFn package-level vars in recommendations.go (defaults to real constructors; no behaviour change)
Three new tests in recommendations_test.go: parallelism timing proof, order preservation, error isolation
All 663 azure provider tests pass

Changes

providers/azure/recommendations.go — DI seam only:

serviceRecsGetter interface (GetRecommendations(ctx, params) ([]Recommendation, error))
newComputeClientFn, newDatabaseClientFn, newCacheClientFn, newCosmosDBClientFn vars wrapping the real constructors
GetRecommendations calls newXxxClientFn(...) instead of xxx.NewClient(...) directly

providers/azure/recommendations_test.go — three tests:

TestGetRecommendations_Parallelism: 4 fakes × 100ms sleep; asserts elapsed < 200ms (vs ~400ms serial)
TestGetRecommendations_OrderPreservation: staggered sleeps (10–40ms) complete out-of-order; asserts merged slice is compute→db→cache→cosmosdb
TestGetRecommendations_ErrorIsolation: database fake returns error; asserts other 3 services' recs still present

Test plan

go build ./providers/azure/... clean
go test github.com/LeanerCloud/CUDly/providers/azure/... — 663 passed, 14 packages
New tests run in ~200ms total (parallel), not ~400ms (serial)

coderabbitai · 2026-05-30T17:43:44Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 29d5cd88-64ed-44fb-9ac3-b19e4123f894

📥 Commits

Reviewing files that changed from the base of the PR and between 1d9f1b8 and 01c4583.

📒 Files selected for processing (2)

providers/azure/recommendations.go
providers/azure/recommendations_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/262-wave19

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-05-30T17:43:45Z

@coderabbitai review

coderabbitai · 2026-05-30T17:43:50Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-04T14:03:07Z

@coderabbitai review

coderabbitai · 2026-06-04T14:03:14Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-08T06:30:10Z

@coderabbitai full review

Generated by Claude Code

coderabbitai · 2026-06-08T06:30:23Z

@cristim Sure, I'll redo the full review of PR #873.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-04T14:28:19.903Z
Learning: In providers/azure/recommendations.go (Go, Azure provider), the service constructors (compute.NewClient, cache.NewClient, etc.) build concrete Azure SDK types with no interface injection seam. Introducing a concurrency timing test that asserts elapsed ≈ max-single-service latency would require adding package-level function variables — a refactor that is out of scope for small performance PRs.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T04:21:35.802Z
Learning: In LeanerCloud/CUDly `frontend/src/recommendations.ts` (PR `#838`, commit d7c5764), `perRecPayments` in `FanOutBucket` must be seeded **sparsely** (override-only: only recs whose resolved payment differs from the bucket-level default). The execute path in `frontend/src/app.ts` resolves each rec's payment as `perRecPayments.get(rec.id) ?? b.payment`; if the map is fully populated the fallback is dead and changing the bucket-level dropdown has no effect on unedited rows. The per-rec change handler deletes the map entry when the user reverts to the bucket default (resuming tracking via the fallback); the bucket-level change handler re-syncs visible per-rec selects that have no override, skipping rows whose service does not support the new payment (mixed-SP buckets). Tests (i) and (j) cover the override-only contract and bucket-level propagation respectively; 322 tests pass, `tsc --noEmit` clean.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/recommendations/parser_services.go`, `parseRDSDetails` uses an exhaustive switch on `DeploymentOption`: "Multi-AZ" maps to "multi-az", "Single-AZ" maps to "single-az", nil leaves AZConfig unset, and any other non-nil value returns an error. Silent defaulting to "single-az" for unknown values was removed in PR `#1085` commit 681fd9e81. Regression test added.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` no longer hardcodes `Term = "1yr"` (fixed in PR `#1047` commit 8f9a787, H-3 audit finding). It propagates `params.Term` with a `"1yr"` default, validates via `termPlan`, and returns nil on unknown term. This is stricter than memorystore/cloudstorage/cloudsql (which default and continue): computeengine returns nil on unknown term because it is on the purchase path. Regression tests: `TestConvertGCPRecommendation_PropagatesParamsTerm`, `TestConvertGCPRecommendation_RejectsUnknownTerm`.

✅ Action performed

Full review finished.

cristim · 2026-06-08T06:46:17Z

@coderabbitai full review

coderabbitai · 2026-06-08T06:46:22Z

✅ Action performed

Full review finished.

cristim · 2026-06-19T21:53:04Z

@coderabbitai review

coderabbitai · 2026-06-19T21:53:10Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-07-10T15:43:35Z

@coderabbitai review

coderabbitai · 2026-07-10T15:43:41Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…er (closes #262) Introduce four package-level newXxxClientFn vars (defaulting to the real compute/database/cache/cosmosdb constructors) so tests can inject fake clients without changing production behaviour. Add three tests in recommendations_test.go: - TestGetRecommendations_Parallelism: 4 fakes sleep 100ms each; asserts wall-clock < 200ms, proving concurrent dispatch (serial would be ~400ms). - TestGetRecommendations_OrderPreservation: staggered sleeps force out-of-order completion; asserts merged slice is compute->db->cache->cosmos. - TestGetRecommendations_ErrorIsolation: one fake errors; asserts the other three services still contribute recs (no sibling cancellation).

…etic TestGetRecommendations_Parallelism was failing (~300-700ms wall clock vs 200ms threshold) because the savingsplans and advisor goroutines used real constructors that made ARM network calls, adding unbounded latency outside the four fakeServiceClient mocks. Add newSavingsPlansClientFn package-level var (parallel to the other four) and a getAdvisorRecsFn field on RecommendationsClientAdapter (defaulting to r.getAdvisorRecommendations, with a nil guard for struct-literal adapters in existing tests). Wire newSavingsPlansClientFn in GetRecommendations to replace the direct savingsplans.NewClient call. In the three concurrency tests (Parallelism, OrderPreservation, ErrorIsolation) inject noopAdvisorFn and a zero-sleep fakeServiceClient for savingsplans so all latency comes exclusively from the four injectable mocks. Tests are now fully hermetic: no real ARM calls, deterministic rec count, pass consistently under -race. 699 azure provider tests pass; 0 lint issues in touched files.

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/s Hours type/feat New capability labels May 30, 2026

cristim force-pushed the fix/262-wave19 branch from 0cd29e8 to 4c7a8fe Compare June 8, 2026 04:17

cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:43

cristim force-pushed the fix/262-wave19 branch from 4c7a8fe to 100c05b Compare June 19, 2026 21:52

cristim force-pushed the fix/262-wave19 branch from 100c05b to 76e446d Compare July 10, 2026 15:43

cristim added 2 commits July 17, 2026 21:30

cristim force-pushed the fix/262-wave19 branch from 76e446d to 01c4583 Compare July 17, 2026 18:31

Uh oh!

Conversation

cristim commented May 30, 2026

Summary

Changes

Test plan

Uh oh!

coderabbitai Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

coderabbitai Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented May 30, 2026 •

edited

Loading

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading