feat: enforce TiDB Cloud free plan limits - #785
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTiDB Cloud provisioning now resolves billing plans, enforces free-tier quotas and tenant limits, supports early cluster binding with metadata polling, persists tenant state atomically, and applies authorization and structured error handling across provisioning, admin, quota, and native provider flows. ChangesTiDB Cloud plan resolution and provisioning
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
060e7ae to
5430661
Compare
There was a problem hiding this comment.
Pull request overview
This PR strengthens TiDB Cloud “free plan” enforcement across provisioning, admin, and quota mutation paths by adding Billing-plan resolution (with positive-only caching), persisting billing-organization bindings, and improving TiDB Cloud native provisioning to persist cluster identity before polling for metadata.
Changes:
- Add TiDB Cloud Billing-plan resolution, non-free plan caching, and access gating (admin + quota mutation restrictions for free orgs).
- Persist tenant ↔ billing organization bindings (including backfill + new meta APIs) and enforce free-plan tenant/quota limits.
- Split TiDB Cloud native provisioning into “create cluster” vs “wait for metadata,” persisting cluster ID early and improving stale pending-tenant reconciliation + metrics.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tenant/tidbcloudnative/provisioner_test.go | Expands TiDB Cloud native provisioner test coverage for Billing plan resolution, typed errors, and early-binding behavior. |
| pkg/tenant/tidbcloud_plan.go | Introduces tenant-level Billing/free-plan error types and plan resolver interface. |
| pkg/tenant/provisioner.go | Adds TiDBCloudAPIError typed error and updates the credential provisioner interface to early-binding semantics. |
| pkg/tenant/provisioner_error_test.go | Adds tests validating TiDBCloudAPIError structure and formatting. |
| pkg/server/tidbcloud_plan_cache.go | Adds a positive-only (non-free) Billing plan cache with TTL and bounded size. |
| pkg/server/tidbcloud_plan_cache_test.go | Tests cache hit/expiry/removal and max-entries bounds. |
| pkg/server/tidbcloud_free_limits.go | Adds server-side free-plan quota normalization and enforcement for provision requests. |
| pkg/server/tidbcloud_free_limits_test.go | Tests free-plan quota normalization and rejection conditions. |
| pkg/server/tidbcloud_access.go | Adds access-profile resolution (IAM + Billing), cache integration, and Billing error classification helpers. |
| pkg/server/tidbcloud_access_test.go | Tests caching rules (non-free only), cross-credential sharing, and Billing error response mapping. |
| pkg/server/shared_db_pool.go | Aligns shared-tenant quota materialization with default file-count behavior. |
| pkg/server/server.go | Wires Billing cache + free-plan limits into server config; adds early-binding cluster reference persistence and improved pending-tenant reconciliation. |
| pkg/server/quota.go | Rejects free-org quota mutations and maps Billing lookup failures to appropriate client responses. |
| pkg/server/quota_test.go | Adds tests ensuring no handler-level OpenAPI double-counting and free-org admin/quota mutation rejections. |
| pkg/server/provision_test.go | Adds extensive coverage for free-plan provisioning, billing bindings, early-binding lifecycle, and error mapping behavior. |
| pkg/server/admin_tenants.go | Gates admin tenant operations behind Billing-plan access checks and preserves Billing error contracts. |
| pkg/server/admin_tenant_pool.go | Gates pool operations behind Billing-plan access checks; adds free-plan headroom checks and customer binding/quota persistence during pool claims. |
| pkg/server/admin_tenant_pool_test.go | Adds tests for free-plan pool-claim headroom, binding persistence, and quota clamping. |
| pkg/metrics/operations.go | Adds Billing cache metric and changes TiDB Cloud OpenAPI metric labels to api/operation/result. |
| pkg/metrics/operations_test.go | Updates metric tests to cover the new Billing cache metric and new OpenAPI label set. |
| pkg/meta/tenant_pool_membership.go | Adds shared pool-claim variant that can persist billing org bindings during claim. |
| pkg/meta/tenant_pool_failed_cleanup_test.go | Adds cleanup to avoid cross-test interference for pool membership fixtures. |
| pkg/meta/tenant_billing_org_binding.go | Adds tenant billing-org binding schema + CRUD, free-tenant reservation helpers, free quota lock, and backfill routine. |
| pkg/meta/tenant_billing_org_binding_test.go | Adds extensive tests for binding backfill, free-tenant reservation/locking, and stale reservation cleanup. |
| pkg/meta/quota.go | Introduces configurable default max file count and uses it for quota defaults/patches. |
| pkg/meta/quota_setting_test.go | Adds tests for default max file count behavior and updates expectations accordingly. |
| pkg/meta/meta.go | Adds tenant billing-org binding table, free-quota lock settings, a reusable insertTenantTx, and runs billing-org binding backfill in migration. |
| pkg/meta/meta_test.go | Adds tests for exact-candidate pool claim behavior, early cluster reference persistence, and atomic finalize connection CAS. |
| cmd/drive9-server/main.go | Adds env parsing for Billing cache TTL, free-plan limits, and global max tenant file-count default; updates shared access validation hook. |
| cmd/drive9-server/main_test.go | Adds unit tests for the new env parsing helpers. |
Comments suppressed due to low confidence (1)
pkg/server/quota.go:257
rejectFreeQuotaMutationruns before loading the tenant and checking its provider. That means quota updates for non‑TiDBCloud tenants (or when the provisioner doesn't support billing/identity resolution) can now fail with a Billing/IAM error instead of the intended "quota setting is only supported for tidb_cloud_native tenants" conflict, and it also adds unnecessary billing lookups.
Move the free-plan mutation check to after quotaTenant + provider validation so it only applies to TiDB Cloud tenants.
if err := s.rejectFreeQuotaMutation(r.Context(), cred, "quota_set"); err != nil {
writeQuotaSetError(w, r.Context(), err, "authorize")
return
}
t, ok := s.quotaTenant(w, r.Context(), req.TenantID)
if !ok {
return
}
if t.Provider != tenant.ProviderTiDBCloudNative && t.Provider != tenant.ProviderTiDBCloudNativeShared {
errJSON(w, http.StatusConflict, "quota setting is only supported for tidb_cloud_native tenants")
return
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/server/tidbcloud_access.go (1)
92-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the billing-lookup operation constant across packages.
pkg/server/tidbcloud_access.gomatchesapiErr.Operationwith the"Billing plan lookup"string thatpkg/tenant/tidbcloudnativesets viastatusError. Add an exported typed constant inpkg/tenantshared by both packages so this detection cannot drift silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/tidbcloud_access.go` around lines 92 - 98, Define an exported typed constant in pkg/tenant for the billing-plan lookup operation currently represented by "Billing plan lookup" in tidbcloudnative. Update statusError usage there and the comparison in isTiDBCloudBillingLookupError to reference this shared tenant constant, preserving the existing case-insensitive trimmed matching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/tidbcloud_free_limits.go`:
- Around line 45-56: Update normalizeTiDBCloudFreeProvisionQuota so positive
MaxStorageBytes and MaxFileSizeBytes convert to quota units using ceiling
division rather than floor division, ensuring any positive configured byte limit
yields at least one unit. Guard the maxStorageSize and maxFileSize defaults
against zero before applying omitted-field values, while preserving the existing
validation for explicitly supplied quota fields.
In `@pkg/tenant/tidbcloudnative/provisioner.go`:
- Around line 1489-1490: The delete response recording in both branch deletion
(pkg/tenant/tidbcloudnative/provisioner.go#L1489-L1490) and cluster deprovision
(pkg/tenant/tidbcloudnative/provisioner.go#L1515-L1516) must classify an
accepted 404 as tidbCloudResultOK rather than client_error. Adjust the
recordTiDBCloudHTTPResponse calls or their placement while preserving existing
handling for other statuses.
---
Nitpick comments:
In `@pkg/server/tidbcloud_access.go`:
- Around line 92-98: Define an exported typed constant in pkg/tenant for the
billing-plan lookup operation currently represented by "Billing plan lookup" in
tidbcloudnative. Update statusError usage there and the comparison in
isTiDBCloudBillingLookupError to reference this shared tenant constant,
preserving the existing case-insensitive trimmed matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aff542a7-6d7f-4b19-9e86-e013833dab7d
📒 Files selected for processing (31)
cmd/drive9-server/main.gocmd/drive9-server/main_test.gopkg/meta/meta.gopkg/meta/meta_test.gopkg/meta/quota.gopkg/meta/quota_setting_test.gopkg/meta/tenant_billing_org_binding.gopkg/meta/tenant_billing_org_binding_test.gopkg/meta/tenant_pool_failed_cleanup_test.gopkg/meta/tenant_pool_membership.gopkg/metrics/operations.gopkg/metrics/operations_test.gopkg/server/admin_tenant_pool.gopkg/server/admin_tenant_pool_test.gopkg/server/admin_tenants.gopkg/server/provision_test.gopkg/server/quota.gopkg/server/quota_test.gopkg/server/server.gopkg/server/shared_db_pool.gopkg/server/tidbcloud_access.gopkg/server/tidbcloud_access_test.gopkg/server/tidbcloud_free_limits.gopkg/server/tidbcloud_free_limits_test.gopkg/server/tidbcloud_plan_cache.gopkg/server/tidbcloud_plan_cache_test.gopkg/tenant/provisioner.gopkg/tenant/provisioner_error_test.gopkg/tenant/tidbcloud_plan.gopkg/tenant/tidbcloudnative/provisioner.gopkg/tenant/tidbcloudnative/provisioner_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060e7ae368
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
LGTM. Reviewed the full diff in depth (31 files, +4437) — I read the core access-control, billing-classification, and provisioning-flow paths directly and fanned out parallel deep-dives across the provisioner, server wiring, and meta layer. This is a well-architected, fail-closed implementation of billing-plan-based access control. CI is green. (Posting as a comment — GitHub blocks approving one's own PR.) Security properties verifiedFree/non-free classification fails closed. Access-control gating is complete and correctly ordered. Every admin route ( Cluster create/poll split preserves the cluster ID. Free-tenant cap is race-free at both creation entry points. Direct provisioning uses Stale-reservation reconciliation is well-guarded. Migration backfill is conflict-safe. Non-blocking nits
None of these block. Test coverage across the new files (plan cache, free limits, access profile, billing-org binding, provisioner billing) is thorough, secret-redaction is well-tested, and the create/poll refactor is guarded end-to-end. Nice work. |
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Reviewed against the approved design doc. Overall this is a high-quality implementation — the 16 commits map cleanly onto the design's three-phase sequence (typed errors → metric unification → feature), and the correctness-critical mechanisms hold up under close reading. I ran focused passes over four risk areas; three are clean, one has a single item worth fixing before merge.
✅ Verified correct
Free-slot reservation + pending reconciler
- The reconciler pool-discriminator fix is correct. The old
ClusterID != "" && DBUser == ""proxy is now only a trigger to do the real lookup; the skip decision goes through the explicitHasTenantPoolOwnership()(checkstenant_tidbcloud_org_bindings.pool_id/tenant_pool_memberships/tenant_placements). An early-bound direct-native tenant with no pool metadata correctly falls through topending → failedinstead of being silently skipped. This was the main risk from the design review and it's handled properly. - Free-count query uses
t.status(notstate), three INNER JOINs,tidbcloud_spending_limit = 0— matches spec. - Recount happens inside the org lock; the three inserts (
tenants+tenant_billing_org_bindings+tenant_quota_config) are one transaction; the lock is released before any cloud HTTP call. 5s lock wait →ErrTiDBCloudFreeQuotaBusy→ 503 +Retry-After: 1. DeleteStaleTiDBCloudFreeReservationchecks all nine required conditions; a tenant with acluster_idcan never be local-deleted.- Native early-cluster-reference persistence happens before the metadata wait; connection persist +
pending → provisioningCAS is one transaction; org mismatch triggers cluster cleanup.
Billing resolver + response parsing
poc_credits: strict decimal regex →big.Rat.SetString→Sign(), nofloat64anywhere.tenant_iddecoded viajson.Number(exact, no float64 truncation); bothtenant_id/tenant_id_strmust match when present.- Classification, fail-closed behavior, body size-limiting, and org-ID redaction (
/v1beta1/tenants/***/plans) all match the design.
Access profile + admin/quota gates
- Resolution order is correct: IAM identity+role first → cache hit skips billing → miss calls billing → non-free cached / free removes stale positive / failure fails closed with no cache write. Cache keyed by org ID only, stores just the expiry marker (no plan/credentials), bounded at 10k.
- Every admin endpoint rejects a free org with 403 before any tenant/pool/cloud lookup (no existence oracle); malformed/missing creds still 400. Free quota is readable; every free mutation is 403 regardless of direction. The zero-file-count → 403 case (0 = unlimited) is correct end-to-end.
- Startup validation: billing URL required only for native/native-shared, absolute HTTPS + host + trailing-slash trim;
ValidateSharedAccessruns IAM+billing, requires org match and non-free.
⚠️ Worth fixing before merge — typed error dropped Service, so billing classification fell back to cross-package string matching
The design specifies TiDBCloudAPIError{Service, Operation, Status}. The implementation is:
type TiDBCloudAPIError struct {
Operation string
StatusCode int
UpstreamBody string
}There's no Service field, so the server distinguishes a billing error from IAM/cluster errors by matching the operation string:
func isTiDBCloudBillingLookupError(err error) bool {
apiErr, ok := tiDBCloudAPIError(err)
return ok && strings.EqualFold(strings.TrimSpace(apiErr.Operation), "Billing plan lookup")
}That literal "Billing plan lookup" is produced in a different package (statusError("Billing plan lookup", ...) in tidbcloudnative/provisioner.go). This is exactly the fragile cross-package string coupling the typed-error refactor set out to eliminate: rename either magic string and billing 401/403 silently degrades to 502. It's functionally correct today (tests pin the strings), but it reintroduces the failure mode the refactor was meant to remove.
Suggestion: add back a Service field ("billing" / "iam" / "cluster") and switch isTiDBCloudBillingLookupError to apiErr.Service == "billing".
Related: the struct retains UpstreamBody, which the design says the type must not do. IAM and billing paths both pass "" (so no credential/body leak on the sensitive paths), but cluster paths store sanitizeUpstreamBody(raw). If "sanitized body is acceptable" is the intended behavior, worth relaxing the design wording so doc and code agree.
Minor (non-blocking)
- Cache eviction can drop the just-inserted key.
rememberNonFreeinserts first (size → max+1) thenfor key := range c.entries { delete; break }, which can evict the entry just added. The bound is respected and the direction is safe (at worst one extra billing lookup), but evicting-before-insert would be cleaner. - Spending-limit 400 vs 403 deviates from the spec table. The design says "positive → 403", but the existing
validateQuotaSetRequestruns first and returns 400 for 1–9 RMB and for values above max ("must be 0 or at least 10 RMB"). Only a positive value in [10, max] reaches the 403 path. Either adjust the spec table or normalize free-path spending rejection to 403. normalizeTiDBCloudFreeProvisionQuotafolds negatives into the 403 branch. Currently masked becausevalidateQuotaSetRequestalways runs first (negatives → 400), so no live bug — but the function isn't self-defensive if a future caller bypasses validation.get_branchmetric operation isn't in the design's enumerated operation list. Low-cardinality, so not a cardinality problem — just a doc gap.
Verdict
Good to merge once the Service field / billing-classification string-matching item is addressed — it doesn't affect current behavior, but it will silently regress billing 401/403 mapping on any future rename, contrary to the refactor's own goal. Everything else is polish.
|
Addressed the typed-error review in 9934e79. TiDBCloudAPIError now carries a typed Service field with iam/billing/cluster constants; every native OpenAPI status error sets it, and server billing classification checks Service instead of an operation string. The tests assert service propagation and prove that a cluster error using the same operation text is not misclassified as Billing. This commit also makes every nonzero free-org provision spending limit return 403 before generic quota validation, including negative, below-minimum, and above-maximum values. I intentionally left the positive-cache eviction policy and sanitized cluster UpstreamBody behavior unchanged because they are non-blocking and outside these correctness fixes. |
8ef95b1 to
4d42871
Compare
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest push. The fix: address TiDB Cloud free plan review findings commit resolves the blocking item.
✅ Blocking item resolved
TiDBCloudAPIError now carries a Service field, and isTiDBCloudBillingLookupError checks apiErr.Service == tenant.TiDBCloudAPIServiceBilling instead of matching the "Billing plan lookup" string across package boundaries. IAM and cluster classification switched to Service too. The regression test locks it in — a renamed operation with Service=billing still classifies as billing, and Service=cluster with the old magic string does not. The cross-package string coupling is gone.
Follow-up nits (non-blocking)
- Default free tenant count changed 3 → 5 (
DefaultTiDBCloudFreeTenantCount = 5), but the design doc still says "a default of three" /Default: 3, and I don't see a--help/ quota-guide update for the new value in this PR. Please sync the doc + operator help to 5. - Cache eviction still inserts-then-evicts (
rememberNonFree): the entry is added before the over-cap loop runs, so the just-inserted key can be the one dropped. Bound is respected and it's safe (at worst one extra billing lookup), but evicting before insert would be cleaner. UpstreamBodyretained onTiDBCloudAPIError: cluster paths still populate it withsanitizeUpstreamBody(...), which is against the design's "must not retain an upstream body" wording. IAM/billing paths pass"", so no leak on the sensitive paths — if a sanitized cluster body is intentional, worth relaxing the design text so doc and code agree.
Net: merge-blocker cleared. Remaining items are polish plus the 3→5 doc sync.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/admin_tenants.go (1)
1-1: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winReuse the resolved TiDB Cloud identity/plan in the tenant-authorization paths.
authorizeTiDBCloudAdminAccessandrejectFreeQuotaMutationalready resolve the caller’s API key identity once; the follow-up tenant checks callresolveTiDBCloudIdentityagain for the same request. Return and pass the resolved identity through the free-plan gates soauthorizedAdminTenant,authorizeNativeTenantCredentials, andauthorizeSharedQuotaCredentialscan skip the duplicate IAM lookup, while the billing lookup remains needed when the plan cache misses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/admin_tenants.go` at line 1, Update authorizeTiDBCloudAdminAccess and rejectFreeQuotaMutation to retain the already resolved TiDB Cloud identity and plan, then pass them through the free-plan authorization flow. Extend authorizedAdminTenant, authorizeNativeTenantCredentials, and authorizeSharedQuotaCredentials to accept and reuse the resolved identity, avoiding duplicate resolveTiDBCloudIdentity calls while preserving the billing lookup when the plan cache misses.
🧹 Nitpick comments (4)
pkg/tenant/provisioner.go (1)
58-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a typed string type for the TiDB Cloud API service enum.
Serviceis a domain enum compared acrosspkg/tenant,pkg/tenant/tidbcloudnative, andpkg/server; a defined type (e.g.type TiDBCloudAPIService string) would prevent accidental assignment of arbitrary operation/label strings to this field.As per coding guidelines: "Use typed string constants for domain enums."
♻️ Suggested typing
+type TiDBCloudAPIService string + const ( - TiDBCloudAPIServiceIAM = "iam" - TiDBCloudAPIServiceBilling = "billing" - TiDBCloudAPIServiceCluster = "cluster" + TiDBCloudAPIServiceIAM TiDBCloudAPIService = "iam" + TiDBCloudAPIServiceBilling TiDBCloudAPIService = "billing" + TiDBCloudAPIServiceCluster TiDBCloudAPIService = "cluster" ) type TiDBCloudAPIError struct { - Service string + Service TiDBCloudAPIService Operation string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tenant/provisioner.go` around lines 58 - 69, Introduce a defined string type, such as TiDBCloudAPIService, for the TiDB Cloud service enum and declare TiDBCloudAPIServiceIAM, TiDBCloudAPIServiceBilling, and TiDBCloudAPIServiceCluster with that type. Update TiDBCloudAPIError.Service and all related comparisons or assignments across the affected tenant and server code to use the typed constants while preserving existing behavior.Source: Coding guidelines
pkg/metrics/operations.go (1)
307-313: 📐 Maintainability & Code Quality | 🔵 TrivialLabel rename is a dashboard/alert-breaking change.
drive9_tidbcloud_openapi_requests_totalswitches its first label frompathtoapi; existing queries, recording rules, and alerts selecting onpath=will silently return no series. Worth calling out in the release/migration notes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/operations.go` around lines 307 - 313, Document the label rename from path to api for drive9_tidbcloud_openapi_requests_total in the release or migration notes, explicitly noting that existing queries, recording rules, and alerts filtering on path= must be updated.pkg/tenant/tidbcloudnative/provisioner_test.go (1)
456-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAbsolute metric counts couple these tests to execution order.
assertTiDBCloudOpenAPIMetricasserts an exact cumulative value from the process-global registry, so any other test in this package that produces the sameapi/operation/resulttriple shifts the count. Line 404 already encodes that accumulation (wantCount: 2forprotocol_error), andiam/resolve_api_key_identity/okis also emitted byTestValidateSharedAccessUsesConfiguredKeyAtStartup. The delta helpertiDBCloudOpenAPIMetricValuealready in this file is order-independent; consider asserting deltas instead.♻️ Sketch
-func assertTiDBCloudOpenAPIMetric(t *testing.T, api, operation, result string, count int) { +func assertTiDBCloudOpenAPIMetricDelta(t *testing.T, api, operation, result string, before float64, want float64) { t.Helper() - rec := httptest.NewRecorder() - metrics.WritePrometheus(rec) - want := fmt.Sprintf(`drive9_tidbcloud_openapi_requests_total{api=%q,operation=%q,result=%q} %d`, api, operation, result, count) - if !strings.Contains(rec.Body.String(), want) { - t.Fatalf("missing metric %q:\n%s", want, rec.Body.String()) - } + if got := tiDBCloudOpenAPIMetricValue(t, api, operation, result) - before; got != want { + t.Fatalf("metric delta for %s/%s/%s = %v, want %v", api, operation, result, got, want) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tenant/tidbcloudnative/provisioner_test.go` around lines 456 - 464, Update assertTiDBCloudOpenAPIMetric to assert metric deltas using the existing tiDBCloudOpenAPIMetricValue helper instead of exact cumulative values from the global registry. Adjust its callers or setup to capture the baseline and compare the expected increment, preserving existing expectations such as the protocol_error accumulation and shared IAM metric behavior.pkg/meta/tidbcloud_free_quota.go (1)
185-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one implementation with
SetQuotaConfigPatchto avoid divergence.This is a near-verbatim copy of
SetQuotaConfigPatch(pkg/meta/quota.goLines 251-304), including the default-vs-COALESCEsemantics. Two copies will drift the moment a quota column or default is added. Extracting the argument construction + SQL into a helper that both the*sql.DBand*sql.Txpaths call (e.g. via a smallexecerinterface) keeps the semantics single-sourced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/tidbcloud_free_quota.go` around lines 185 - 227, Refactor upsertTenantQuotaPatchTx and SetQuotaConfigPatch to share one implementation for quota patch argument construction and SQL execution, using a small common execer interface if needed for *sql.DB and *sql.Tx. Preserve the existing default-on-insert and COALESCE-on-update semantics, including all quota columns and TiDB Cloud fields, so future schema or default changes are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/admin_tenants.go`:
- Around line 323-326: Update handleAdminTenantQuotaSet and the related
authorizedAdminTenant flow to reuse the TiDB Cloud identity returned by
authorizeTiDBCloudAdminAccess instead of resolving it again through
resolveTiDBCloudIdentity and adminTenantMetricPath. Preserve the existing
authorization and quota-update behavior while ensuring each request performs
only one external IAM identity check.
- Around line 287-290: Reuse the resolved TiDB Cloud identity/profile from
authorizeTiDBCloudAdminAccess throughout handleAdminTenantGet,
handleAdminTenantQuotaSet, and handleAdminTenantDelete, passing it into
authorizedAdminTenant and subsequent IAM/metric checks instead of resolving cred
again. Apply the same propagation through rejectFreeQuotaMutation and
shared/native tenant authorization in quota.go, preserving existing
authorization outcomes while eliminating duplicate credential cache lookups.
In `@pkg/server/quota.go`:
- Around line 242-245: Refactor the quota mutation authorization flow around
rejectFreeQuotaMutation and the subsequent authorizeTiDBCloudOrganization calls
to resolve the TiDB Cloud identity once and reuse the resulting authorization
data. Return or propagate the shared authorization result from the initial
check, then pass it through later native/shared authorization checks without
invoking resolveTiDBCloudIdentity again.
In `@pkg/server/server.go`:
- Around line 5446-5458: Update the reservationErr busy-error branch in the
server provisioning flow to match errors against meta.ErrTiDBCloudFreeQuotaBusy
instead of tenant.ErrTiDBCloudFreeQuotaBusy, while preserving the existing 503
response and retry behavior. Ensure the corresponding error message uses the
meta sentinel consistently with AcquireTiDBCloudFreeQuotaLock.
In `@pkg/tenant/tidbcloudnative/provisioner.go`:
- Around line 268-275: Update the IAM error handling in the response path around
recordTiDBCloudHTTPResponse to ignore any readErr from draining resp.Body and
always return the typed statusError containing the HTTP status, matching the
existing Billing path behavior and preserving downstream error classification.
---
Outside diff comments:
In `@pkg/server/admin_tenants.go`:
- Line 1: Update authorizeTiDBCloudAdminAccess and rejectFreeQuotaMutation to
retain the already resolved TiDB Cloud identity and plan, then pass them through
the free-plan authorization flow. Extend authorizedAdminTenant,
authorizeNativeTenantCredentials, and authorizeSharedQuotaCredentials to accept
and reuse the resolved identity, avoiding duplicate resolveTiDBCloudIdentity
calls while preserving the billing lookup when the plan cache misses.
---
Nitpick comments:
In `@pkg/meta/tidbcloud_free_quota.go`:
- Around line 185-227: Refactor upsertTenantQuotaPatchTx and SetQuotaConfigPatch
to share one implementation for quota patch argument construction and SQL
execution, using a small common execer interface if needed for *sql.DB and
*sql.Tx. Preserve the existing default-on-insert and COALESCE-on-update
semantics, including all quota columns and TiDB Cloud fields, so future schema
or default changes are made in one place.
In `@pkg/metrics/operations.go`:
- Around line 307-313: Document the label rename from path to api for
drive9_tidbcloud_openapi_requests_total in the release or migration notes,
explicitly noting that existing queries, recording rules, and alerts filtering
on path= must be updated.
In `@pkg/tenant/provisioner.go`:
- Around line 58-69: Introduce a defined string type, such as
TiDBCloudAPIService, for the TiDB Cloud service enum and declare
TiDBCloudAPIServiceIAM, TiDBCloudAPIServiceBilling, and
TiDBCloudAPIServiceCluster with that type. Update TiDBCloudAPIError.Service and
all related comparisons or assignments across the affected tenant and server
code to use the typed constants while preserving existing behavior.
In `@pkg/tenant/tidbcloudnative/provisioner_test.go`:
- Around line 456-464: Update assertTiDBCloudOpenAPIMetric to assert metric
deltas using the existing tiDBCloudOpenAPIMetricValue helper instead of exact
cumulative values from the global registry. Adjust its callers or setup to
capture the baseline and compare the expected increment, preserving existing
expectations such as the protocol_error accumulation and shared IAM metric
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc7b1993-5ed5-4cda-986e-0f9fd3f22ad1
📒 Files selected for processing (30)
cmd/drive9-server/main.gocmd/drive9-server/main_test.gopkg/meta/meta.gopkg/meta/meta_test.gopkg/meta/quota.gopkg/meta/quota_setting_test.gopkg/meta/tenant_pool_failed_cleanup_test.gopkg/meta/tidbcloud_free_quota.gopkg/meta/tidbcloud_free_quota_test.gopkg/metrics/operations.gopkg/metrics/operations_test.gopkg/server/admin_tenant_pool.gopkg/server/admin_tenant_pool_test.gopkg/server/admin_tenants.gopkg/server/provision_test.gopkg/server/quota.gopkg/server/quota_test.gopkg/server/server.gopkg/server/shared_db_pool.gopkg/server/tidbcloud_access.gopkg/server/tidbcloud_access_test.gopkg/server/tidbcloud_free_limits.gopkg/server/tidbcloud_free_limits_test.gopkg/server/tidbcloud_plan_cache.gopkg/server/tidbcloud_plan_cache_test.gopkg/tenant/provisioner.gopkg/tenant/provisioner_error_test.gopkg/tenant/tidbcloud_plan.gopkg/tenant/tidbcloudnative/provisioner.gopkg/tenant/tidbcloudnative/provisioner_test.go
…lan-limits # Conflicts: # cmd/drive9-server/main.go # pkg/server/provision_test.go # pkg/server/server.go
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push (simplify TiDB Cloud free quota accounting, preserve IAM status on body drain errors, plus main merges). This is a larger change than the commit titles suggest — +2800/-1093 across accounting, shared-pool, provisioning, and admin-pool — so I traced the correctness-critical paths again.
✅ Confirmed fixed from the prior round
- IAM body-drain fix is correct. A body-read error on a non-2xx IAM response previously discarded the HTTP status (returned the read error); now
statusError(...)is returned with the real status regardless of drain outcome. Status mapping is preserved. Servicefield fix holds (from the earlier round): billing classification isService-based, not string-matched.
⚠️ Architecture change worth an explicit sign-off: tenant_billing_org_bindings removed
The approved design added a dedicated tenant_billing_org_bindings table as the source of truth for free-tenant counting. This push deletes that table and rewrites CountTiDBCloudFreeTenants to derive org ownership from existing metadata:
- native arm:
tenant_tidbcloud_org_bindings(org binding,pool_status <> 'free') - shared arm:
db_pool.org_id → tenant_placements → fs_registry → tenants
I verified the new count query is sound on its own terms:
- No double-counting. The two UNION arms are disjoint by
provider(native vs native_shared), which is single-valued per tenant. The shared arm yields exactly one row per tenant (tenant_placements.fs_idis PK,fs_registry.tenant_idis unique), so no intra-arm multiplicity. - Cleanup guards are safe.
DeleteStaleTiDBCloudFreeReservation/DeleteTiDBCloudFreeReservationstill refuse any tenant with acluster_id, api key, native binding, pool membership, or placement — a placement-owning shared tenant can never be local-deleted. - reconciler not regressed. Still uses explicit
HasTenantPoolOwnership()(native bindingpool_id<>''/ membership / placement), not theClusterID/DBUserproxy.
⚠️ Consequence of the rewrite — the free-quota lock now spans a cloud create (deviates from the design)
This is the one item reviewers should consciously accept. With the dedicated binding table gone, a freshly reserved pending tenant is not yet counted — its tenant_tidbcloud_org_bindings row (native) / placement (shared) is written later, during cloud provisioning. To keep the cross-pod atomicity guarantee (two concurrent reservations must not both pass the recount), the code now holds the free-quota org lock across the cloud cluster/pool create HTTP call:
- native: acquire → recount → reserve →
CreateClusterWithCredentialsAndQuota(HTTP) →PersistTiDBCloudTenantClusterReference(writes the binding,pool_status=Usedso it counts) → release, then the metadata wait runs unlocked. - shared: acquire → recount → reserve →
ensureManagedSharedDBPhysical(HTTP) / placement → release before the readiness continuation.
The design explicitly stated (spec line 514): "The lock does not cover external HTTP calls, shared database readiness waits, or schema initialization," with release right after the count-and-insert transaction (steps 8-10). The implementation is self-consistent and correct — the extended hold is exactly what preserves the guarantee once the binding table is removed — but it trades the design's property for a real cost:
- The 5-second free-quota lock now wraps a cloud create for the first concurrent provision per org, so a second concurrent free-provision for the same org can hit
ErrTiDBCloudFreeQuotaBusy→ 503 while the first is mid-create. - It pins a dedicated
*sql.Connacross that external call.
Both are bounded (readiness/metadata waits are still unlocked, and it only bites concurrent same-org provisions), but it's a deliberate departure from the approved spec. Please either (a) update the design doc to record that the lock intentionally covers the create call and why, or (b) reintroduce a lightweight countable marker written inside the reservation transaction so the lock can be released immediately as originally specced.
Minor / non-blocking
retryTenantPoolClaimCASnow returns(zero, nil)after exhausting retries — this is correct "miss" semantics (only reached when every attempt wasErrNotFound; other errors propagate). Just confirm all callers treat a nil+zero result as "no claim."- Prior round's nits still open: default free tenant count 3→5 not reflected in the design doc /
--help; cacherememberNonFreestill inserts-then-evicts;TiDBCloudAPIError.UpstreamBodyretained on cluster paths.
Verdict
No new correctness bugs found — the accounting rewrite is internally consistent and the atomicity guarantee is preserved. The one thing that needs an explicit decision is the lock-across-cloud-create deviation from the approved design: it's correct but changes the concurrency/latency profile the spec promised, so it shouldn't land silently. Everything else is polish.
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
One migration follow-up to the accounting-rewrite review above, after tracing the dropped-table path fully:
- No explicit
DROP TABLE tenant_billing_org_bindings. The table's DDL and all Go references are removed and the count no longer touches it (verified: zero non-test references remain), but there's no drop in the migration path. On already-migrated deployments the table is left orphaned and unused — harmless dead state, but worth a deliberate call: either add a drop, or note in the doc that it's intentionally left in place for rollback safety.
Also confirming the atomicity analysis for the reviewers: the freshly-reserved pending tenant is genuinely invisible to CountTiDBCloudFreeTenants (both count arms inner-join a provisioning artifact — native binding / shared placement — that the reservation transaction does not write). This is safe only because that invisibility window is fully contained inside the per-org GET_LOCK critical section, which is held until the binding/placement is persisted. A concurrent same-org reservation blocks on the lock and cannot recount until the first is visible, so two reservations can't both pass count < limit. That's the crux of why the lock-across-cloud-create hold (flagged above) is load-bearing rather than incidental — if it's ever shortened, the atomic guarantee goes with it. A reserved-but-crashed tenant (quota row, no binding) consumes no slot and is reaped by DeleteStaleTiDBCloudFreeReservation, whose guard matches exactly that shape.
|
Addressed the latest review and rebased/merged the current main into this branch.
The branch is now up to date with |
|
Correction to my previous review reply: The free-quota lock trade-off described in the previous reply is unchanged: after removing that never-released marker-table approach, the existing per-org lock remains held through native binding/shared placement persistence so the cross-pod free-slot count stays atomic. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/tenant/tidbcloudnative/provisioner.go (1)
887-893: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve decoded pools when a partial response is malformed.
BatchCreateClustersmay return a non-empty subset with an error. A later malformed/unknown returned cluster still causes an immediatenilreturn, discarding already decoded pools; the caller can retry them and create duplicate physical pools. Accumulate that validation error intopartialErrand return the successfuloutslice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tenant/tidbcloudnative/provisioner.go` around lines 887 - 893, Update the BatchCreateClusters handling and subsequent cluster-validation flow so malformed or unknown clusters are accumulated into partialErr when createdClusters is non-empty, rather than returning nil immediately. Preserve the successfully decoded pools in the out slice and return them alongside the accumulated validation error, while keeping the existing immediate return for an entirely empty response.
🧹 Nitpick comments (3)
pkg/tenant/tidbcloudnative/clusters_api.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a package-level documentation comment.
Add
// Package tidbcloudnative ...immediately before the package declaration. As per coding guidelines, “Add a package-level documentation comment to the first file in each package.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tenant/tidbcloudnative/clusters_api.go` at line 1, Add a package-level documentation comment immediately before the tidbcloudnative package declaration in clusters_api.go, beginning with “Package tidbcloudnative” and briefly describing the package.Source: Coding guidelines
pkg/meta/meta.go (1)
2184-2209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the scan error with operation context.
The final error path returns the raw
row.Scanerror unwrapped, unlike sibling meta functions (e.g.GetSharedDB,GetTenantPlacement) that wrap DB errors withfmt.Errorf("...: %w", ...). As per coding guidelines,**/*.go: "Wrap errors with operation context using%w; check sentinel errors witherrors.Israther than comparing strings."♻️ Proposed fix
row := s.db.QueryRowContext(ctx, fmt.Sprintf(countFreeTenantPoolBindingsSQL, placeholders, placeholders), args...) if err = row.Scan(&out); err != nil { - return 0, err + return 0, fmt.Errorf("count free tidbcloud pool bindings for organization %s: %w", organizationID, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/meta.go` around lines 2184 - 2209, Update countFreeTenantPoolBindingsByStatus so the row.Scan failure is wrapped with descriptive operation context using fmt.Errorf and %w before returning. Preserve the existing output and error handling behavior, including the named err used by observeMeta.Source: Coding guidelines
pkg/server/provision_test.go (1)
402-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated managed-shared-DB test fixture setup into a helper.
The
meta.Open→t.Cleanup→testmysql.ResetMetaDB→ generate AES key →encrypt.NewLocalAESEncryptor→tenant.NewPool→pool.SetMetaStoresequence is duplicated near-verbatim in this test and inTestProvisionTiDBCloudNativeSharedPlansManagedPoolAndReturnsPending(544-563),TestManagedSharedDBContinuationBatchesDoNotEnterBlockingMetadataWait(1381-1398),TestManagedSharedDBBatchCreateRunsAllChunksConcurrently(1732-1747), andTestManagedSharedDBBatchCreateLocksEachAllocationIdentityIndependently(1808-1823). Consider factoring this into a sharedt.Helper()-based fixture constructor (mirroring the existingmustTempDir(t)helper) to cut duplication and centralize future setup changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/provision_test.go` around lines 402 - 496, Extract the repeated metadata store, encryption, and tenant pool setup into a shared t.Helper()-based fixture constructor, modeled after mustTempDir(t). Replace the duplicated setup in TestManagedSharedDBMetadataWorkerRefillsReadySlots, TestProvisionTiDBCloudNativeSharedPlansManagedPoolAndReturnsPending, TestManagedSharedDBContinuationBatchesDoNotEnterBlockingMetadataWait, TestManagedSharedDBBatchCreateRunsAllChunksConcurrently, and TestManagedSharedDBBatchCreateLocksEachAllocationIdentityIndependently, preserving each test’s cleanup and returned fixture usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tenant/tidbcloudnative/http_clusters_api.go`:
- Around line 191-193: Avoid double-counting successful delete metrics in the
delete-cluster and delete-branch response branches: in
pkg/tenant/tidbcloudnative/http_clusters_api.go lines 191-193 and 260-262, keep
the accepted 200, 204, and 404 responses returning success, but call
recordTiDBCloudOpenAPIRequest only for StatusNotFound because
doDigestAuthRequest already records 200/204 responses.
---
Outside diff comments:
In `@pkg/tenant/tidbcloudnative/provisioner.go`:
- Around line 887-893: Update the BatchCreateClusters handling and subsequent
cluster-validation flow so malformed or unknown clusters are accumulated into
partialErr when createdClusters is non-empty, rather than returning nil
immediately. Preserve the successfully decoded pools in the out slice and return
them alongside the accumulated validation error, while keeping the existing
immediate return for an entirely empty response.
---
Nitpick comments:
In `@pkg/meta/meta.go`:
- Around line 2184-2209: Update countFreeTenantPoolBindingsByStatus so the
row.Scan failure is wrapped with descriptive operation context using fmt.Errorf
and %w before returning. Preserve the existing output and error handling
behavior, including the named err used by observeMeta.
In `@pkg/server/provision_test.go`:
- Around line 402-496: Extract the repeated metadata store, encryption, and
tenant pool setup into a shared t.Helper()-based fixture constructor, modeled
after mustTempDir(t). Replace the duplicated setup in
TestManagedSharedDBMetadataWorkerRefillsReadySlots,
TestProvisionTiDBCloudNativeSharedPlansManagedPoolAndReturnsPending,
TestManagedSharedDBContinuationBatchesDoNotEnterBlockingMetadataWait,
TestManagedSharedDBBatchCreateRunsAllChunksConcurrently, and
TestManagedSharedDBBatchCreateLocksEachAllocationIdentityIndependently,
preserving each test’s cleanup and returned fixture usage.
In `@pkg/tenant/tidbcloudnative/clusters_api.go`:
- Line 1: Add a package-level documentation comment immediately before the
tidbcloudnative package declaration in clusters_api.go, beginning with “Package
tidbcloudnative” and briefly describing the package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6c66bf9-d2cc-48d2-ab0e-5c93d1b42409
📒 Files selected for processing (12)
cmd/drive9-server/main.gopkg/meta/meta.gopkg/server/admin_tenant_pool.gopkg/server/admin_tenant_pool_test.gopkg/server/provision_test.gopkg/server/server.gopkg/server/shared_db_pool.gopkg/tenant/provisioner.gopkg/tenant/tidbcloudnative/clusters_api.gopkg/tenant/tidbcloudnative/http_clusters_api.gopkg/tenant/tidbcloudnative/provisioner.gopkg/tenant/tidbcloudnative/provisioner_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/server/shared_db_pool.go
- pkg/server/admin_tenant_pool_test.go
- cmd/drive9-server/main.go
- pkg/server/admin_tenant_pool.go
- pkg/server/server.go
|
Handled the latest review and merged the current
Focused TiDB Cloud, server, command, and repository compile checks passed. The branch is up to date with |
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 (3)
pkg/metrics/operations.go (1)
521-537: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify business events using the normalized event value.
The metric label is trimmed, but
tenantAttributedBusinessEventreceives the raw input. An event such as" tenant_provision "is emitted astenant_provisionwithout tenant/org labels, producing inconsistent attribution. Normalize once before both uses.Proposed fix
+ event = cleanMetricValue(event, "unknown") attrs = append(attrs, Attr("event", cleanMetricValue(event, "unknown")))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/operations.go` around lines 521 - 537, Normalize the event value once before the tenant attribution check and metric emission, then pass that normalized value to tenantAttributedBusinessEvent and use it consistently for the event label. Update the surrounding metric-building function rather than changing the classification cases in tenantAttributedBusinessEvent.pkg/meta/quota.go (2)
677-692: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate request cancellation to this quota update.
applyQuotaCounterDeltasTxis called from transactions with an application context, butIncrQuotaUsageCountersTxusestx.Execand never observes cancellation or SQL errors. Addctx context.Contextas the first parameter, usetx.ExecContext(ctx, ...), wrap errors with%w, and update the sharedMetaQuotaStore/adapter/fake signatures and callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/quota.go` around lines 677 - 692, Update IncrQuotaUsageCountersTx to accept ctx context.Context first and execute the quota UPDATE via tx.ExecContext, wrapping returned SQL errors with %w while preserving ensureRowsAffected handling. Propagate the new context parameter through MetaQuotaStore, its adapter and fake implementations, applyQuotaCounterDeltasTx, and every caller.Source: Coding guidelines
1509-1511: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
RowsAffectedfailures before interpreting a row count mismatch.A driver error from
res.RowsAffected()is currently discarded and converted toerrMutationAlreadyApplied, which can make a transient/SQL failure look like a duplicate-applied mutation. Wrap the read error directly and only treatn != len(ids)as “already applied.”Proposed fix
- n, _ := res.RowsAffected() + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("read batch mark-applied result: %w", err) + } if int(n) != len(ids) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/quota.go` around lines 1509 - 1511, Update the RowsAffected handling in the batch mutation flow to capture and check its returned error before comparing the affected count. Return the RowsAffected error directly with appropriate context when it fails; only return errMutationAlreadyApplied when the successful count differs from len(ids).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/meta/quota.go`:
- Around line 677-692: Update IncrQuotaUsageCountersTx to accept ctx
context.Context first and execute the quota UPDATE via tx.ExecContext, wrapping
returned SQL errors with %w while preserving ensureRowsAffected handling.
Propagate the new context parameter through MetaQuotaStore, its adapter and fake
implementations, applyQuotaCounterDeltasTx, and every caller.
- Around line 1509-1511: Update the RowsAffected handling in the batch mutation
flow to capture and check its returned error before comparing the affected
count. Return the RowsAffected error directly with appropriate context when it
fails; only return errMutationAlreadyApplied when the successful count differs
from len(ids).
In `@pkg/metrics/operations.go`:
- Around line 521-537: Normalize the event value once before the tenant
attribution check and metric emission, then pass that normalized value to
tenantAttributedBusinessEvent and use it consistently for the event label.
Update the surrounding metric-building function rather than changing the
classification cases in tenantAttributedBusinessEvent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f741621e-05fd-4dec-a601-afb357de67d2
📒 Files selected for processing (19)
cmd/drive9-server/main.gopkg/meta/meta.gopkg/meta/meta_test.gopkg/meta/quota.gopkg/meta/quota_setting_test.gopkg/meta/tenant_pool_failed_cleanup_test.gopkg/metrics/operations.gopkg/metrics/operations_test.gopkg/server/admin_tenant_pool.gopkg/server/admin_tenant_pool_test.gopkg/server/admin_tenants.gopkg/server/provision_test.gopkg/server/quota_test.gopkg/server/server.gopkg/server/shared_db_pool.gopkg/tenant/provisioner.gopkg/tenant/tidbcloudnative/http_clusters_api.gopkg/tenant/tidbcloudnative/provisioner.gopkg/tenant/tidbcloudnative/provisioner_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/server/shared_db_pool.go
- pkg/meta/tenant_pool_failed_cleanup_test.go
- pkg/tenant/provisioner.go
- pkg/meta/meta_test.go
- pkg/server/admin_tenant_pool_test.go
- pkg/meta/quota_setting_test.go
- pkg/server/quota_test.go
- pkg/server/admin_tenants.go
- pkg/metrics/operations_test.go
- cmd/drive9-server/main.go
- pkg/server/admin_tenant_pool.go
- pkg/meta/meta.go
- pkg/server/provision_test.go
- pkg/server/server.go
- pkg/tenant/tidbcloudnative/provisioner.go
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)
pkg/meta/meta.go (1)
3780-3791: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
RowsAffected()errors inFinalizeTenantConnection.
n, _ := res.RowsAffected()currently discards driver errors fromRowsAffected(). If that call fails, the function returns(false, nil), which can make callers treat finalization as a harmless no-op. Assign and handle the error, and wrapExecContexterrors with operation context.Proposed fix
res, err := s.db.ExecContext(ctx, ...) if err != nil { - return false, err + return false, fmt.Errorf("finalize tenant connection: %w", err) } - n, _ := res.RowsAffected() + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("read finalized tenant rows affected: %w", err) + } return n > 0, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/meta.go` around lines 3780 - 3791, Update FinalizeTenantConnection to handle and return errors from res.RowsAffected() instead of discarding them, and wrap ExecContext failures with clear operation context while preserving the existing success result based on affected rows.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/server/notify_coalescer_test.go (1)
219-223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the actual retry-success path.
Because
rec.erris cleared before thec.flushcall, this flush succeeds on its initial attempt; the new assertion therefore does not coverfinishFlushafter an internal retry. Use an inserter that fails once and succeeds on the retry, and retain a direct assertion for the pending gauge if that metric remains part of the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/notify_coalescer_test.go` around lines 219 - 223, Update the retry-success test around the coalescer flush to use an inserter that fails on its first call and succeeds on the retry, ensuring the assertion exercises finishFlush after an internal retry. Retain a direct assertion for the pending gauge if it remains part of the contract, alongside the existing pending and inFlight state checks.pkg/meta/meta.go (1)
2677-2691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap raw transaction errors with operation context.
Both new metadata flows lose the operation stage when returning database or transaction errors. Apply
%wcontext at each affected boundary:
pkg/meta/meta.go#L2677-L2691: identify binding update, affected-row validation, and quota deletion failures.pkg/meta/meta.go#L3831-L3868: identify lock, transaction, conflict cleanup, availability, tenant update, upsert, and commit failures.As per coding guidelines, errors must be wrapped with operation context using
%w.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/meta.go` around lines 2677 - 2691, Wrap each returned error with operation-specific context using %w. In pkg/meta/meta.go lines 2677-2691, label binding update, requireAffected validation, and quota deletion failures. In pkg/meta/meta.go lines 3831-3868, label lock, transaction, conflict cleanup, availability, tenant update, upsert, and commit failures; preserve the underlying errors for unwrapping.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/meta/meta.go`:
- Around line 3780-3791: Update FinalizeTenantConnection to handle and return
errors from res.RowsAffected() instead of discarding them, and wrap ExecContext
failures with clear operation context while preserving the existing success
result based on affected rows.
---
Nitpick comments:
In `@pkg/meta/meta.go`:
- Around line 2677-2691: Wrap each returned error with operation-specific
context using %w. In pkg/meta/meta.go lines 2677-2691, label binding update,
requireAffected validation, and quota deletion failures. In pkg/meta/meta.go
lines 3831-3868, label lock, transaction, conflict cleanup, availability, tenant
update, upsert, and commit failures; preserve the underlying errors for
unwrapping.
In `@pkg/server/notify_coalescer_test.go`:
- Around line 219-223: Update the retry-success test around the coalescer flush
to use an inserter that fails on its first call and succeeds on the retry,
ensuring the assertion exercises finishFlush after an internal retry. Retain a
direct assertion for the pending gauge if it remains part of the contract,
alongside the existing pending and inFlight state checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61b7b889-e097-4842-8615-92f6610cf8fd
📒 Files selected for processing (14)
cmd/drive9-server/main.gocmd/drive9-server/main_test.gopkg/meta/meta.gopkg/meta/quota.gopkg/meta/quota_setting_test.gopkg/metrics/operations.gopkg/metrics/operations_test.gopkg/server/admin_tenant_pool.gopkg/server/admin_tenant_pool_test.gopkg/server/notify_coalescer_test.gopkg/server/provision_test.gopkg/server/quota_test.gopkg/server/server.gopkg/server/shared_db_pool.go
💤 Files with no reviewable changes (1)
- pkg/meta/quota.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/server/shared_db_pool.go
- pkg/metrics/operations.go
- pkg/meta/quota_setting_test.go
- pkg/metrics/operations_test.go
- cmd/drive9-server/main_test.go
- pkg/server/admin_tenant_pool_test.go
- pkg/server/quota_test.go
- cmd/drive9-server/main.go
- pkg/server/admin_tenant_pool.go
- pkg/server/provision_test.go
- pkg/server/server.go
Re-review at
|
|
Addressed the valid items from the latest review in 4122bde.\n\n- FinalizeTenantConnection now wraps ExecContext failures and propagates RowsAffected failures instead of treating them as a silent no-op; added a driver-level regression test.\n- Native, IAM, and Billing base URL validation now rejects query strings, fragments, empty query/fragment markers, opaque URLs, and userinfo; added table-driven startup validation coverage.\n- Added an explicit code comment documenting that the per-org free-quota lock must remain held until the native binding or shared placement becomes countable.\n\nIntentionally unchanged:\n- Free provision quota-shape violations remain HTTP 403, matching the requested policy that free-org rejections use 403.\n- Non-free cache eviction remains bounded and fail-closed; evicting the just-inserted key only causes an extra Billing lookup and does not change authorization.\n\nThe branch also includes the latest main commit 80ed57b. Fresh verification passed: pkg/meta, pkg/tenant/tidbcloudnative, focused pkg/server tests, repository compile-only test, and make lint. |
There was a problem hiding this comment.
Re-reviewed 4122bde (fix: address remaining TiDB Cloud review findings) on top of the main merge. Tight, well-tested commit — no new correctness issues. Build passes.
Confirmed fixes:
-
FinalizeTenantConnectionswallowedRowsAffectederror — previouslyn, _ := res.RowsAffected()meant a driver that couldn't report affected rows returnedupdated=false, err=nil, indistinguishable from a lost CAS race, letting the caller proceed on an unknown write status. Now wrapped and propagated, with a driver-injecting test assertingupdated=false && errors.Is(err, wantErr). 👍 -
validTiDBCloudBaseURL— good consolidation of the triplicated validation, and it's stricter than before: rejects userinfo, query, fragment, and opaque forms in addition to non-https/empty-host.ContainsAny(rawURL, "?#")is a reasonable belt-and-suspenders alongside theRawQuery/Fragmentemptiness checks. Table test covers the empty-?, empty-#, anduser:password@cases. LeavingPathunrestricted is correct — path-prefixed base URLs are legitimate. -
Lock-scope deviation now documented at the acquisition site (the reservation is uncountable until its native binding / shared placement is durable, so the lock must span the persist). Good to have it recorded next to the code that depends on it.
One item still open — look like deliberate choices, just flagging for an explicit call:
- The in-code comment documents the lock-scope deviation, but the design doc (§ "The lock does not cover external HTTP calls...") still states the opposite. Either amend the doc or note there that the implementation intentionally supersedes it.
Nothing blocking from my side.
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Re-reviewed 6d11171 (refactor: reuse TiDB Cloud access profiles). Pure dedup, no behavior change — resolve the IAM identity once in each handler, then thread the resolved OrganizationID into the downstream authorizers instead of having each of them re-resolve. Build + pkg/server tests pass on this commit.
Verified:
-
Semantically equivalent. All three former resolvers (
firstManagedOrganization,authorizeTiDBCloudOrganizationviaresolveTiDBCloudIdentity, andresolveTiDBCloudAccessProfile) derived their org from the sameresolveTiDBCloudIdentity(...).OrganizationID. So passingaccess.OrganizationIDdown is identical to each callee resolving it itself — just one IAM round-trip per request instead of two. The newauthorizedTenantPool/authorizedAdminTenant/authorizeSharedQuotaOrganization/authorizeNativeTenantOrganizationallTrimSpacethe org before thetiDBCloudOrganizationMatchescompare, matching the prior trimmed comparison. -
No dead code / dangling refs.
authorizeTiDBCloudOrganizationis fully removed (zero references). The read path (quota_get) still uses the*Credentialsvariants, so those aren't orphaned. Removed struct fields (tiDBCloudAccessProfile.Role,TiDBCloudOrganizationPlan.EffectivePlan) have no remaining readers — the surviving.Rolereads are ontenant.TiDBCloudAPIKeyIdentity.Role(owner-role check, still needed), andeffective_planis still parsed locally to computeIsFree, just no longer stored. -
authorizeTiDBCloudQuotaMutationcorrectly generalizes the oldrejectFreeQuotaMutation— still fail-closed onIsFree, now also returns the profile so the caller reuses its org.
Looks good. Nothing blocking.
Re-review at
|
mornyx
left a comment
mornyx
left a comment
There was a problem hiding this comment.
Review summary
Verdict: Approve
Reviewed in an isolated worktree at 6d11171 (detached), against merge-base 80ed57b with origin/main. This is a large but coherent free-plan enforcement stack: Billing plan resolution → non-free-only cache → free provision quotas/tenant cap → org advisory lock → early native cluster binding → stale free-reservation reconcile. CI green; local focused suites passed:
go test ./pkg/meta -run 'TiDBCloudFree|FreeQuota'go test ./pkg/server -run 'TiDBCloudFree|FreeQuota|FreePlan|FreeAdmin|FreeTenant|Billing'go test ./pkg/tenant/tidbcloudnative -run 'Plan|Billing|Free'
Correctness checks (deep)
- Free classification —
parseOrganizationPlanResponse: free by default; non-free only foron_demand, orpocwith strictly positivepoc_credits(decimal viabig.Rat). Org identity cross-check against request org. Local clusters API short-circuits to non-free (correct for local e2e). - Cache policy — only non-free is cached (30m, max 10k). Free always re-hits Billing. Paid→free downgrade can remain treated as non-free until TTL (fail-open toward paid privileges for ≤30m). Acceptable product tradeoff; free→paid upgrades are immediate on next miss.
- Admin / quota mutation —
authorizeTiDBCloudAdminAccess/authorizeTiDBCloudQuotaMutationhard-forbid free orgs (403). Shared credentials startupValidateSharedAccessrequires non-free shared org. - Free provision quotas —
normalizeTiDBCloudFreeProvisionQuotaforces spending limit 0, caps storage/file size/count, rejects positive spending and over-cap requests with 403. - Occupied free count —
CountTiDBCloudFreeTenantsunions native bindings and shared placements; excludesstatus=deleted,spending_limit != 0(and NULL via SQL= 0), and unclaimed pool inventory (pool_status = free). Matches the PR write-up. - Lock + countability window — free path holds
GET_LOCKuntil org binding / shared placement is durable; reservation rows alone are not countable. Concurrent pods serialize. Crash between insert and binding is under-count (fail-open free slots) untilDeleteStaleTiDBCloudFreeReservationinreconcilePendingTenant— intentional and wired. - Early native binding —
CreateCluster…thenPersistTiDBCloudTenantClusterReference(cluster id + binding) before metadata wait; free lock released after durable binding, before long polling. Failure after cluster id keeps failed+counted until deprovision+metadata clear, then reservation release. - HTTP mapping — free limit 403; free quota busy 503 +
Retry-After: 1viawriteProvisionTenantError. Billing lookup failures mapped by status (401/403/502). - Metrics — unified TiDB Cloud OpenAPI observation path for Billing with cache hit/miss counters.
Non-blocking / follow-up
- Paid→free cache lag (≤30m): document in ops notes if free-admin ban must be hard real-time.
- Crash under-count window for free reservations before binding: mitigated by stale pending reconcile; worth an alert if pending free rows without binding grow.
- Cache eviction at 10k entries deletes an arbitrary map key (not LRU). Fine at current scale.
- Full
pkg/serversuite may still hit shared MySQL env flake noted in the PR body; feature-focused coverage is solid.
No merge blockers found on this pass. Approved.
Summary
on_demand, or when it ispocwith positive POC credits. Cache only non-free results for 30 minutes.tenant_tidbcloud_org_bindings; native-shared usestenant_placementsthroughdb_pool. Deleted tenants, NULL/nonzero spending limits, and unclaimed pool inventory withpool_status=freeare excluded.Validation
GOCACHE=/tmp/drive9-go-cache go test ./pkg/meta -count=1pkg/servertests covering free limits, Billing, native early binding, shared claims, and pending reconciliation.GOCACHE=/tmp/drive9-go-cache go test ./pkg/... -run "^$" -count=1GOCACHE=/tmp/drive9-go-cache GOLANGCI_LINT_CACHE=/tmp/drive9-golangci-cache make lintorigin/main; branch is not behind and merge-tree reports no conflict.Test infrastructure note
A full
pkg/serverrun reached the existing shared MySQL test environment failure after about 146 seconds: the first failures wereunexpected EOF/invalid connection, followed by cascading setup failures after the database closed. The feature-focused server tests pass.Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests