Keep quota reads side-effect free and tune MySQL pool lifetimes

[srstack]

Summary

• validate TiDB Cloud credential-based quota reads with BASIC cluster information only on RBAC cache misses, while preserving the local Drive9 API-key fast path
• stop quota and admin read endpoints from backfilling cloud spending limits into the meta database; explicit quota updates and lifecycle seeds remain unchanged
• increase default meta and user-schema connection lifetimes and the meta idle timeout

Test Plan

• go test ./pkg/mysqlutil -count=1
• go test ./pkg/tenant/tidbcloudnative -count=1
• go test ./pkg/server -run Test(QuotaGet|DeprecatedQuotaGet|QuotaSet|AdminTenantGet|AdminTenantList|AdminTenantQuotaSet|AdminTenantPoolReplenishUsesDefaultSpendingLimit) -count=1
• make lint
• git diff --check origin/main...HEAD

Full server test note

A full go test ./pkg/server -count=1 was attempted twice. In both runs, the package-local MySQL test instance returned invalid connection after roughly two minutes, causing cascading failures in later token, upload, and vault tests. No quota or admin-quota assertion failed, and the focused affected test set above passes.

Summary by CodeRabbit

• Bug Fixes

  • Quota and tenant administration views no longer overwrite or backfill locally configured spending limits when retrieving quota information.
  • Quota responses continue to include current quota details without triggering background spending-limit synchronization.
  • Improved connection pool defaults help maintain connections longer during normal operation.

• Tests

  • Added coverage confirming quota and tenant requests preserve local spending-limit settings and avoid unintended updates.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7781e3dc-ad0d-4f4c-888a-36810fb87b15

📥 Commits

Reviewing files that changed from the base of the PR and between 56469fe and 9af1af8.

📒 Files selected for processing (6)

• pkg/mysqlutil/pool.go
• pkg/server/admin_tenants.go
• pkg/server/quota.go
• pkg/server/quota_test.go
• pkg/tenant/tidbcloudnative/provisioner.go
• pkg/tenant/tidbcloudnative/provisioner_test.go

---

📝 Walkthrough

Walkthrough

Quota reads and admin tenant operations no longer synchronize or backfill TiDB Cloud spending limits. BASIC cluster metadata now supplies labels, and role-based MySQL pool timing defaults are increased.

Changes

TiDB Cloud quota flow

Layer / File(s)	Summary
BASIC quota metadata retrieval pkg/tenant/tidbcloudnative/provisioner.go, pkg/tenant/tidbcloudnative/provisioner_test.go	Cluster metadata operations use BASIC responses and quota reads return labels without populating monthly spending limits. Tests verify view=BASIC and no PATCH request.
Server quota and tenant handling pkg/server/quota.go, pkg/server/admin_tenants.go	Quota and admin tenant handlers remove spending-limit synchronization, fresh listing, and backfill helper paths while retaining quota loading and RBAC caching.
No-backfill regression coverage pkg/server/quota_test.go	Tests verify local spending limits and checked timestamps remain unchanged across quota and admin tenant reads, including cached requests.

Connection pool defaults

Layer / File(s)	Summary
Role-based pool timing defaults pkg/mysqlutil/pool.go	Meta connection lifetime changes from 5 to 10 minutes, meta idle time from 45 seconds to 1 minute, and user-schema lifetime from 1 to 3 minutes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• mem9-ai/drive9#723: Modifies the same tenant authorization and quota paths for spending-limit caching and backfill.
• mem9-ai/drive9#762: Changes the /v1/quota flow and TiDB Cloud spending-limit handling.
• mem9-ai/drive9#759: Updates TiDB Cloud spending-limit synchronization and quota seeding behavior.

Suggested reviewers: copilot, mornyx, qiffang

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the two main changes: side-effect-free quota reads and increased MySQL pool lifetimes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/quota-read-side-effects

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Pull request overview

This PR makes TiDB Cloud quota/authorization reads side‑effect free (no meta DB backfills) while still validating credentials using minimal (BASIC) cluster data, and it tunes default MySQL connection pool lifetimes to reduce churn/invalid-connection issues.

Changes:

• Switch credential-based authorization reads to view=BASIC cluster lookups and stop reading cloud spending limits on quota GET.
• Remove spending-limit backfill behavior from quota and admin tenant read endpoints (explicit quota updates / lifecycle seeding remain the mutation points).
• Increase default meta DB and user-schema DB connection max lifetimes (and slightly adjust meta idle timeout).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/tenant/tidbcloudnative/provisioner.go	Replaces full cluster/quota fetch with BASIC cluster info for label-based authorization and removes spending-limit reads from GetQuota.
pkg/tenant/tidbcloudnative/provisioner_test.go	Updates/extends tests to assert BASIC view usage and absence of spending-limit reads on quota authorization.
pkg/server/quota.go	Removes spending-limit sync/backfill side effects from the quota GET handler while keeping RBAC caching behavior.
pkg/server/quota_test.go	Adjusts expectations to ensure GET paths do not mutate persisted spending limits / checked-at timestamps and adds coverage for admin list include_quota behavior.
pkg/server/admin_tenants.go	Removes admin tenant list/get spending-limit sync/backfill logic so reads no longer mutate quota config state.
pkg/mysqlutil/pool.go	Tunes default connection max lifetimes/idle times for meta and user-schema pools.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[srstack]

Reviewed the full branch (read-only, did not compile; noting the test-plan already covers the focused quota/admin suites + lint). This is a clean, well-scoped change — net -176/+162, mostly deleting read-path side effects. LGTM with a couple of things to confirm, no blockers.

✅ Theme 1: side-effect-free quota reads — correct

handleQuotaGet / handleAdminTenantList / authorizedAdminTenant no longer call syncTiDBCloudSpendingLimit as a side effect of a read. writeQuotaResponse returns the persisted meta-DB value (GetQuotaConfig). Verified getter.GetQuota at quota.go:129 is now a pure credential/RBAC validation call with its result discarded (_,) — and it's the only caller of GetQuota in the server, so nothing depends on the spending limit it used to return. Good.

This is the right resolution to the concern I raised on #762 (the sync-failure-downgraded-to-warning): rather than a read that best-effort-writes, reads are now purely reads, and spending limits are written only by explicit quota updates + lifecycle seeds. One consequence worth stating in the PR body: a GET can now return a stale spending limit (no cloud refresh on read). That's the intended trade and it's consistent, but it is a client-visible contract change from "GET refreshes from cloud" to "GET returns last-persisted" — worth a sentence so downstream consumers know.

✅ Theme 2: BASIC view + removing clusterQuotaInfo — clean

getClusterBasicInfoWithCredentials unifies on ?view=BASIC, and the heavier unmasked clusterQuotaInfo (which parsed SpendingLimit) is deleted. GetQuota now returns {Labels} only. Verified the nil-map guarantee: getClusterBasicInfoWithCredentials sets info.Labels = make(...) when nil before returning, so all four call sites (MarkClusterPoolUsed/Free, MarkQuotaUpdateStarted, GetQuota) can safely write labels[...] without a nil-map panic. GetQuota also correctly copies the map before returning it rather than aliasing info.Labels. The dead helpers (syncAdminTenantSpendingLimits, adminTenantRowsNeedSpendingLimitBackfill, cloudClusterMap, listAllManagedClustersFresh) are all removed with their last callers — no orphans.

One thing to confirm: authorizedAdminTenant now always passes allowCache=true (previously it bypassed the cache when the spending limit was nil, to force a fresh read for backfill). Since backfill is gone that's consistent, but it does mean admin authz can now be decided entirely off cached cluster listings. Confirm the RBAC cache TTL is short enough that a revoked/rotated credential can't authorize against a stale cluster list for longer than acceptable.

🟡 Theme 3: MySQL pool lifetimes — low-risk tuning, no data given

meta lifetime 5→10m, meta idle 45s→1m, user-schema lifetime 1→3m. Direction is reasonable (fewer reconnects, less churn — plausibly related to the earlier context canceled refresh noise). Non-blocking, but these are empirical knobs with no benchmark in the PR — worth a one-line rationale, and note that longer ConnMaxLifetime keeps connections to a rotated backend alive a bit longer (generally fine behind the TiDB LB).

Nit

• The test note says full ./pkg/server couldn't complete due to the package-local MySQL instance returning invalid connection after ~2 min. Given this very PR changes conn lifetimes, it'd be reassuring to confirm that flakiness is pre-existing infra and not interaction with the new ConnMaxLifetime values — even just a run on main showing the same failure.

Net: correct, subtractive, and it resolves the read-side-effect concern from #762. Happy to see it land once the stale-GET contract note and the admin-cache-TTL check are addressed.