perf: scale shared tenant backend cache - #801
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:
📝 WalkthroughWalkthroughThe PR replaces quota configuration version polling with lazy TTL-based caching, removes the quota version API, adds provider-aware tenant cache defaults, and changes shared-tenant idle reaping and LRU behavior. ChangesQuota cache refresh
Quota configuration API removal
Tenant cache defaults
Shared tenant reaping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Backend
participant QuotaConfigCache
participant QuotaStore
Backend->>QuotaConfigCache: get(ctx)
QuotaConfigCache->>QuotaStore: load quota configuration when expired
QuotaStore-->>QuotaConfigCache: configuration or load error
QuotaConfigCache-->>Backend: snapshot, stale snapshot, or nil
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves scalability for high-cardinality “shared tenant” deployments by tuning the tenant backend pool behavior (capacity/eviction defaults and idle handling) and by eliminating per-tenant quota-config polling goroutines in favor of a request-driven, TTL-based cache.
Changes:
- Skip time-based idle eviction for shared-DB tenant backends (shared entries are retained until capacity/LRU eviction).
- Introduce provider-aware defaults for
DRIVE9_POOL_MAX_TENANTS(larger default fortidb_cloud_native_shared, unchanged override behavior). - Replace background quota-config polling with a passive, coalesced, lazy-refresh cache (TTL + stale-on-error + cooldown), and update tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tenant/pool.go | Skips idle TTL eviction for shared-DB tenant entries; retains capacity/LRU eviction and shared-handle reaping. |
| pkg/tenant/pool_test.go | Adds coverage ensuring shared entries bypass idle eviction but are still removed by capacity eviction. |
| pkg/backend/quota.go | Routes quota config reads through the new request-driven cache API. |
| pkg/backend/quota_cache.go | Replaces background quota config polling with passive TTL-based lazy loading, coalescing, and failure cooldown. |
| pkg/backend/quota_cache_test.go | Updates/expands tests to validate passive behavior, TTL reuse, coalescing, and stale-on-error + cooldown semantics. |
| pkg/backend/quota_integration_test.go | Adjusts integration tests to force quota-config reloads under the new cache model. |
| cmd/drive9-server/main.go | Adds provider-aware default sizing for tenant backend cache and updates env help text. |
| cmd/drive9-server/main_test.go | Adds tests for provider-default cache sizing and explicit env override behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 806623310f
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/backend/quota_cache_test.go (1)
55-236: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
t.Errorffor assertion checks,t.Fatal(f)only for setup failures.All new tests in this file (
TestQuotaConfigCacheIsPassiveUntilFirstAccess,TestQuotaConfigCacheReturnsDefensiveCopy,TestQuotaConfigCacheReusesSnapshotUntilTTLExpires,TestQuotaConfigCacheCoalescesConcurrentExpiredAccess,TestQuotaConfigCacheRefreshFailureReturnsStaleAndUsesRetryCooldown,TestQuotaConfigCacheInitialFailureUsesRetryCooldown) uset.Fatalffor every value-comparison assertion (e.g. lines 67-89, 105-107, 122-140, 170-179, 198-213, 222-236), not just setup failures. Per coding guidelines, assertion failures should uset.Errorfso a single mismatch doesn't hide subsequent assertions in the same test.As per coding guidelines, "Use
t.Fatalort.Fatalffor setup failures andt.Errorffor assertion failures."🤖 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/backend/quota_cache_test.go` around lines 55 - 236, The new quota cache tests use t.Fatalf for value assertions instead of reserving fatal failures for setup. In TestQuotaConfigCacheIsPassiveUntilFirstAccess, TestQuotaConfigCacheReturnsDefensiveCopy, TestQuotaConfigCacheReusesSnapshotUntilTTLExpires, TestQuotaConfigCacheCoalescesConcurrentExpiredAccess, TestQuotaConfigCacheRefreshFailureReturnsStaleAndUsesRetryCooldown, and TestQuotaConfigCacheInitialFailureUsesRetryCooldown, change assertion-only t.Fatalf calls to t.Errorf while preserving t.Fatal/t.Fatalf for setup failures and synchronization prerequisites.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/drive9-server/main_test.go (1)
22-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd fallback-boundary test cases.
These tests cover only an unset variable and a positive override. Add cases for
0, negative values, malformed input, and an unrecognized provider to protect the helper’s fallback behavior.🤖 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 `@cmd/drive9-server/main_test.go` around lines 22 - 63, Extend the tests for tenantBackendCacheMaxTenants to cover DRIVE9_POOL_MAX_TENANTS values of 0, negative, and malformed input, plus an unrecognized provider. Assert each invalid or unsupported case returns the appropriate provider/default fallback rather than the configured value, while preserving the existing unset and positive-override coverage.
🤖 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/backend/quota_cache.go`:
- Around line 118-146: Decouple the coalesced refresh in quotaConfigCache.load
from the caller’s cancellation by creating a background-owned context with a
bounded timeout before calling c.store.GetQuotaConfig. Preserve the existing
locking, caching, failure handling, and metrics flow, but ensure one request’s
canceled ctx cannot fail the shared load for other waiters.
---
Outside diff comments:
In `@pkg/backend/quota_cache_test.go`:
- Around line 55-236: The new quota cache tests use t.Fatalf for value
assertions instead of reserving fatal failures for setup. In
TestQuotaConfigCacheIsPassiveUntilFirstAccess,
TestQuotaConfigCacheReturnsDefensiveCopy,
TestQuotaConfigCacheReusesSnapshotUntilTTLExpires,
TestQuotaConfigCacheCoalescesConcurrentExpiredAccess,
TestQuotaConfigCacheRefreshFailureReturnsStaleAndUsesRetryCooldown, and
TestQuotaConfigCacheInitialFailureUsesRetryCooldown, change assertion-only
t.Fatalf calls to t.Errorf while preserving t.Fatal/t.Fatalf for setup failures
and synchronization prerequisites.
---
Nitpick comments:
In `@cmd/drive9-server/main_test.go`:
- Around line 22-63: Extend the tests for tenantBackendCacheMaxTenants to cover
DRIVE9_POOL_MAX_TENANTS values of 0, negative, and malformed input, plus an
unrecognized provider. Assert each invalid or unsupported case returns the
appropriate provider/default fallback rather than the configured value, while
preserving the existing unset and positive-override coverage.
🪄 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: fd63e473-15e6-447d-a19a-a3cdd9ebdb0c
📒 Files selected for processing (8)
cmd/drive9-server/main.gocmd/drive9-server/main_test.gopkg/backend/quota.gopkg/backend/quota_cache.gopkg/backend/quota_cache_test.gopkg/backend/quota_integration_test.gopkg/tenant/pool.gopkg/tenant/pool_test.go
ReviewReviewed at
OverviewThree coupled changes to make shared-schema tenants cheap to keep warm:
The direction is right, and (3) is the prerequisite that makes (1) affordable — 20480 goroutines plus 20480 Findings1. A cached shared entry holds a That may well be the intent ("keep them warm"), but it is a behavior change the description doesn't mention, and it has cost implications for serverless/scale-to-zero shared pools plus lingering connections against a 2. Failure cooldown amplifies load during a meta DB outage At 20480 warm shared tenants, a central DB outage turns into up to 3. No jitter on the success TTL either ( Tenants warmed together (pod start, a burst of cold opens, post-outage recovery) get near-identical 4. Provider default is chosen from the pod's default provider, not per tenant (
5. Per-tenant metric cardinality now grows with the shared cache
6. Nits
TestsGood coverage of the new cache semantics: passivity at construction, defensive copies, TTL reuse, coalescing under 32 concurrent callers, stale-on-error, and initial-failure cooldown. Gaps worth closing:
SecurityNothing material. The stale-config window widens from "up to 30s" to "up to 30s, or the duration of a meta DB outage" for an already-loaded tenant, meaning a quota lowered during an outage takes effect later. That matches the pre-existing fail-open posture of these soft admission checks, and strict upload reservations still read central usage directly. |
|
@srstack Review follow-up is in Implemented:
The shared-handle retention is intentional and is now explicit in the PR description. A cached shared tenant keeps the lightweight physical No code change for the pool-wide mixed-provider cap: No metric-label change in this PR. The cardinality concern is real but changing or capping tenant-labelled series needs measurement and a separate compatibility decision; it is not required for cache/provisioning correctness.
Verified after the follow-up: full |
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)
cmd/drive9-server/main_test.go (1)
24-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve presence when restoring environment snapshots.
snapshotEnvcollapses an originally present empty variable the same way as a missing variable, andrestoreEnvturns empty snapshots intoos.Unsetenv. If a test starts withDRIVE9_POOL_MAX_TENANTScleared to"", cleanup leaves it absent instead of preserving the empty value. Useos.LookupEnvor a restore helper that records presence separately.🤖 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 `@cmd/drive9-server/main_test.go` around lines 24 - 26, Update snapshotEnv and restoreEnv to record whether each environment variable was present separately from its value, using os.LookupEnv or an equivalent presence-aware representation. During restoration, set originally present variables even when their value is empty, and unset only variables that were originally absent; preserve the existing cleanup flow around unsetEnv.
🤖 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/backend/quota_cache.go`:
- Around line 161-167: Restore a dedicated five-second failure retry cooldown in
quotaConfigCache.finishFailedLoad instead of clamping to the normal refresh TTL;
update pkg/backend/quota_cache.go lines 161-167 accordingly. Adjust the related
assertions in pkg/backend/quota_cache_test.go lines 92-112 to verify the
five-second cooldown.
---
Outside diff comments:
In `@cmd/drive9-server/main_test.go`:
- Around line 24-26: Update snapshotEnv and restoreEnv to record whether each
environment variable was present separately from its value, using os.LookupEnv
or an equivalent presence-aware representation. During restoration, set
originally present variables even when their value is empty, and unset only
variables that were originally absent; preserve the existing cleanup flow around
unsetEnv.
🪄 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: bbb9319f-d2ff-4c72-8e90-f6b0f2be73e3
📒 Files selected for processing (3)
cmd/drive9-server/main_test.gopkg/backend/quota_cache.gopkg/backend/quota_cache_test.go
|
@coderabbitai Follow-up commit 053a701 fixes the valid environment-snapshot finding. The test helper now records value and presence separately with os.LookupEnv, so an originally present empty value is restored as present rather than being unset. Added a regression test that fails with the old helper and passes with the presence-aware snapshot. Verified the complete cmd/drive9-server package, its race run, and lint. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Follow-up: two reproduced regressions in the config-cache rewriteTook a closer second pass, this time diffing the old Old shape: func (b *Dat9Backend) cachedQuotaConfig(ctx) *QuotaConfigView {
if cfg := b.quotaConfigCache.get(); cfg != nil { return cfg } // no ctx, never blocks, never fails
return b.quotaConfigCache.load(ctx) // ctx used ONLY when snapshot is nil
}New shape: 1. A canceled/timed-out client request disables quota enforcement tenant-wide for 5s
Reproduced against this branch with a ctx-honouring store (the existing store := &ctxAwareStore{fakeMetaQuotaStore: newFakeMetaQuotaStore()} // returns ctx.Err() if ctx is done
c := newQuotaConfigCache("tenant-poison", "org", store)
canceled, cancel := context.WithCancel(context.Background())
cancel()
c.get(canceled) // nil, as expected
if cfg := c.get(context.Background()); cfg == nil { // healthy, unrelated request
t.Fatalf("POISONED: healthy request got nil config; quota fails open")
}Note Scope, to be fair to the change:
It is also cheaply self-inflictable: a tenant that wants to exceed its own quota can fire a request and abandon it immediately, then issue real writes inside the 5s fail-open window, repeating every 5s. Low sophistication, and the tenant is exactly the party motivated to do it. Suggested fix — detach the store read from the caller's cancellation, and don't treat caller cancellation as evidence the meta DB is unhealthy: loadCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), quotaConfigLoadTimeout)
defer cancel()
cfg, err := c.store.GetQuotaConfig(loadCtx, c.tenantID)
if err != nil {
if ctx.Err() != nil {
// caller vanished; don't burn the tenant's cooldown on it
return c.staleSnapshot()
}
return c.finishFailedLoad(start, "config_error")
}
2. Waiters on
|
|
@srstack Follow-up implemented in
Verified after the change: focused red/green regressions, full backend/tenant/drive9-server tests, the same packages under |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/backend/quota_cache.go`:
- Around line 20-24: Update the failed-load retry path associated with
quotaConfigCacheFailureRetryInterval to use quotaConfigCacheRefreshDelay instead
of the fixed 5-second interval, preserving the default jittered 27–30 second
delay for MetaDB failures. Remove the obsolete fixed retry interval constant and
ensure both referenced retry sites use quotaConfigCacheRefreshDelay.
🪄 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: d46ba5cd-0c07-41f2-97d4-08a1afe3bc6f
📒 Files selected for processing (3)
cmd/drive9-server/main_test.gopkg/backend/quota_cache.gopkg/backend/quota_cache_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/backend/quota_cache_test.go
Third pass: the capacity LRU that now carries all shared-tenant reclaim is inverted by the safety-net scanMy first two comments covered the quota cache and the pinned shared handles. This pass looked at the part I had waved past: now that The mechanism
// pkg/tenant/pool.go:79
// AcquireCached (safety-net scan) does NOT refresh the idle timer on ...
// pkg/tenant/pool.go:240
lastUsed time.Time // refreshed by Get/Acquire/S3Backend (foreground + durable work) ...; never by AcquireCachedBut it does call Pre-PR that inconsistency was harmless: the 5m idle TTL keyed on
ReproducedTwo shared tenants, The actively-used tenant is evicted and pays a cold open; the tenant with zero user traffic stays resident. At 20480 capacity with a 5m scan over every owned tenant, this isn't an edge case — it's the steady state once a pod is at capacity. And a shared-tenant cold open is exactly what the FixDropping the e.refs++
- p.order.MoveToFront(e.elem)
p.mu.Unlock()
metrics.RecordOperation("tenant_pool", "cache_lookup", "hit", 0)Verified on this branch: the repro flips to If you'd rather keep list position as "recently referenced by anything", the alternative is to have capacity eviction pick the victim by minimum Running tallyBlocking, in the order I'd fix them:
Non-blocking, still open: shared handles pinned via The core idea — drop the per-tenant goroutine so keeping shared tenants warm is affordable — is right, and the warm-path stale-on-error handling in the new cache is genuinely careful work. The problems are all in what became load-bearing as a side effect. |
Complete review dispositionThe third-pass LRU finding is valid and fixed in
Blocking findings
Non-blocking findings and decisions
Verification for |
Fourth pass: I measured the metrics-cardinality point I raised — it's a request-path stall, not just a scrape-size concernIn my first comment I asked whether you'd measured MeasurementSeeded the global registry with the per-tenant series a normal request mix emits (7 component/operation/result combos per tenant: storage_check, file_count_check, usage_cache, pending_delta_cache, quota_config_cache load, acquire_cached, safety_net_tenant_scan), then measured one eviction and one scrape: Why the eviction number is the one that matters
// pkg/metrics/registry.go:322
func (r *Registry) DeleteCountersByLabel(labelKey, labelValue string) {
r.mu.Lock()
defer r.mu.Unlock()
for _, inst := range r.counters {
for labels := range inst.values { // every series, every instrument
if labelHasKeyValue(labels, match) { // substring scan per seriesThat's the same And it runs synchronously on the acquire path. Chaining that with finding 2 from my third comment: once a pod is at capacity, every cold open evicts, and the safety-net scan keeps rewriting the LRU so cold opens stay frequent even for actively-used tenants. So the steady state at 20480 is a ~18ms global metrics stall per cold open, on a request. At 1024 it was ~0.3ms and rare, because the idle TTL did most of the reclaiming outside the request path. SuggestionsThe cheap, targeted fix is to make deletion not depend on total registry size — index series by // registry: map[tenantID][]seriesKey maintained on first write per (tenant, series)
func (r *Registry) DeleteByTenant(tenantID string) {
r.mu.Lock(); defer r.mu.Unlock()
for _, key := range r.tenantSeries[tenantID] { /* targeted deletes */ }
delete(r.tenantSeries, tenantID)
}Failing that, moving the Worth noting the 20.57 MiB scrape payload independently: at a typical 15–30s scrape interval that's a real bandwidth and TSDB-ingest cost per pod, and 73ms of scrape holds the same lock. If per-tenant granularity isn't needed on the highest-fanout counters, dropping Caveat on these numbersSingle-run wall-clock on my machine, one seeding pattern, real series counts will differ with your actual operation mix — treat the ratios (x54 eviction, x21 scrape) as the signal rather than the absolute µs. The structural point doesn't depend on the timings: StatusThis upgrades comment 1's finding 5 from "worth measuring" to a fourth blocking item, since it's on the request path and compounds with the LRU inversion. Full list now:
All four are consequences of the same thing: the PR raises the resident-tenant ceiling 20x and makes capacity eviction the primary reclaim path, which puts previously-rare O(N) work onto the hot path. The goroutine removal that motivates the change is sound — it's the second-order effects at 20480 that need attention before this ships. Happy to look at a revision. |
|
This finding is valid, and the measured request-path impact makes the merge dependency explicit. The fix is intentionally owned by PR #802 rather than duplicated here:
That addresses both parts measured here: the synchronous global metrics-lock stall and the dominant resident-series growth. The safety-net LRU inversion mentioned in the chain is already fixed in I am not duplicating the metrics implementation in #801 because the same files and metric contracts are already being changed comprehensively in #802. #801 should merge only after #802 is merged and this branch is rebased onto it, or after equivalent #802 commits are otherwise present in the merge result. |
Follow-up cleanup dispositionThe reasonable architecture-cleanup findings are now addressed in
The shared tenant / physical shared DB lifetime behavior remains intentional and is already covered by the existing pool tests: shared tenant entries bypass the standalone idle TTL, capacity eviction still closes the tenant store and releases its shared DB reference, and an unreferenced physical handle is reaped after the longer shared-handle TTL. No duplicate test-only abstraction was added. The newly measured O(total metrics series) eviction stall is valid, but is being fixed comprehensively in PR #802. That PR removes synchronous The fixed five-second failed quota-config retry remains an owner requirement; successful refreshes retain the 27–30 second jitter. Mixed-provider capacity remains a deployment-level LRU by design, with standalone entries still protected by their idle TTL; splitting the pool into provider-specific eviction budgets would be a separate policy change rather than cleanup of this PR. Focused verification after |
Verified all four fixes independently — they holdRe-reviewed at Fix verification
The Jitter lands strictly within Also confirmed Full-suite verificationAnd On the #802 deferralI verified the claims rather than assume them. In
That addresses both halves of what I measured — the request-path stall and the resident-series growth — so not duplicating it here is the right call. The stated merge order (801 after 802, or rebased onto it) is the load-bearing part: merging #801 alone would ship the 20x resident-tenant ceiling with the O(N) eviction delete still on the request path. Worth enforcing that as an actual gate rather than a note, since the two PRs are independently mergeable today. On the two declined itemsBoth reasonable, recording my read for the archive:
The RemainingOnly the shared-handle lifetime item (comment 1, finding 1), now explicitly documented in the PR description as intentional: shared entries stay warm until capacity eviction, connections are pruned by the 5m pool idle setting, and the 30m handle reaper still applies once refs hit zero. That answers my actual concern — connections don't accumulate unboundedly — so I'm satisfied. LGTM once #802 is in the merge result. Nice work turning these around; the stale-first restructuring of |
mornyx
left a comment
mornyx
left a comment
There was a problem hiding this comment.
Deep review (pool/cache mechanics, quota cache concurrency, cross-cutting lifecycle) with every finding traced end-to-end. Approving: the design is sound — provider-aware default + env precedence is correct, exempting shared tenants from idle eviction cannot leak (every removal reason still funnels through removeLocked/closeEntry with exactly-once shared-handle refcount pairing, and the 30m handle reaper covers refs==0), AcquireCached really is the only LRU-order consumer removed, the polling goroutines are genuinely gone with no per-refresh goroutine spawning, and the channel-based load coalescing has no missed wakeup or double-leader. Tests pin the new semantics (idle exemption, capacity still evicts, LRU untouched by AcquireCached).
One worth-fixing robustness regression (panic wedge) and a few nits inline.
adversary-2 R2 at
|
adversary-2 — re-anchored to
|
|
Fixed in commit |
adversary-2 — retracting GREEN, REVISE (re-anchored to
|
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Reviewed exact base bf995bbab94f13379380097f2838941323b993ac → head 6f1570695550adc59680217f6412fce38e4a7265.
CHANGES_REQUESTED
The quota-cache revision substantively fixes the two prior cache blockers: a panicking load now clears/wakes loadDone before re-panicking, and an expired warm leader serves stale immediately while one detached refresh completes. The latest 06591fd1 reaper revision also correctly keeps shared physical-handle expiry independent from standalone tenant idle eviction. Two merge blockers remain on this exact head:
-
The 20× shared resident-cap change still requires the metrics-lifecycle fix in the same tested merge result.
cmd/drive9-server/main.gostill sets the shared-provider default to20480rather than1024, while this branch'sPool.closeEntrystill synchronously callsmetrics.DeleteTenantCountersfrom capacity eviction. PR #802 contains the intended lifecycle change, but it is not in this head, its current review decision is still CHANGES_REQUESTED, and a synthetic merge of exact #801 head6f157069with #802 heade13d5e98still conflicts inpkg/tenant/pool_test.go. Required: either remove the cap increase from #801, or produce a real resolved/rebased #801+#802 merge result and rerun both PRs' correctness and race gates on that exact result. Merge-order intent is not an executable gate. -
The exact-head test suite is red because three pre-existing regressions still encode the synchronous/disabled behavior that this revision removed. Hosted
cirun30234214795fails:TestQuotaConfigCacheReusesSnapshotUntilTTLExpiresexpects the first expired warm call to synchronously return refreshed2000, but the new contract returns stale and refreshes asynchronously.TestQuotaConfigCacheRefreshFailureReturnsStaleAndUsesRetryCooldownreadsconfigCallsbefore the detached failed refresh runs/completes.TestIdleEvictionStartNoOpWhenDisabledstill assertsreapStop == nilforIdleTimeout=0, contradicting06591fd1, which intentionally starts the shared physical-handle reaper.
I independently reproduced all three failures locally by bypassing only the package TestMain database bootstrap; the production tests themselves were unchanged. Update these regressions to synchronize on the detached load and assert the new reaper contract, then require fresh green
ciand local-e2e on the final exact head.
Validation on this exact revision / its direct parents:
- full PR
git diff --check; changed-filegofmt -l - normal and race compile-only gates for
pkg/backend,pkg/tenant, andcmd/drive9-server go vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/server- normal and race
TestTenantBackendCacheMaxTenants make lint;make build-server- focused normal/race tests for panic cleanup, cold-waiter cancellation, expired-warm leader stale return, and a review-only cold-leader boundary proof
- hosted
cifailure log confirmation for the same three tests
The worktree was restored clean after review-only TestMain isolation/reproduction. Please request another exact-head review after the red tests are corrected and the #801/#802 composition gate is executable.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
adversary-1 R2 at 6f1570695550adc59680217f6412fce38e4a7265
Requesting changes. This head is not mergeable.
Blocking findings:
- Hosted
code-ciis red on this exact head: https://github.com/mem9-ai/drive9/actions/runs/30234214795/job/89878726463. These are PR-owned contract/test failures, not infrastructure noise. The PR changed warm-expired quota config refresh to return stale immediately and refresh asynchronously, but the existing tests still assert synchronous refresh:
pkg/backend/quota_cache_test.go:319: expected refreshed storage2000, got stale1000.pkg/backend/quota_cache_test.go:322: expectedconfigCalls=2, got1.pkg/backend/quota_cache_test.go:380/:387: expected refresh call count2, got1.
The latest reaper change also intentionally makes Pool.Start run when IdleTimeout=0, but pkg/tenant/pool_test.go:1463 still asserts reapStop == nil. Update the stale regressions to the new intended contracts and get code-ci green. The last commit only changed unrelated tests from fatal to nonfatal assertions; it does not address these blockers.
- #801 still cannot merge independently from #802. The 20x shared backend cap raise remains in
cmd/drive9-server/main.go:49(defaultSharedBackendCacheMaxTenants = 20480), while this PR still hasPool.closeEntrycallmetrics.DeleteTenantCountersatpkg/tenant/pool.go:1647. #802 contains the request-path metrics cleanup, but current #802 is stillCHANGES_REQUESTEDand an exact synthetic merge of current #801 + #802 still conflicts inpkg/tenant/pool_test.go. Until fixed #802 or equivalent cleanup is present in the same tested merge result, #801 alone is unsafe.
Resolved/positive checks: the prior quota-cache panic wedge is structurally addressed by the panic defer and load completion cleanup; warm-expired callers now avoid foreground DB latency by returning stale and refreshing asynchronously; cached shared acquire is LRU-neutral; and the shared physical-DB handle reaper now runs independently of standalone idle eviction. Those fixes do not override the red CI and unresolved #802 coupling.
Validation performed on this head:
- Passed locally:
git diff --check origin/main...HEAD, changed-filegofmt -l,go test ./cmd/drive9-server -run '^TestTenantBackendCacheMaxTenants' -count=1, test binary compilation forpkg/backend,pkg/tenant,cmd/drive9-server,go vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/server, andgo build ./.... - Hosted
drive9-server-local e2e smokeis green, but hostedcode-cifails inTest, so the required gate is still red.
adversary-2 — re-anchored to
|
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
adversary-1 R3 at 60dfea1999ab825aebbabe403d00eb5e3113d699
Requesting changes for the remaining composition blocker. The #801-owned red-test blocker from 6f157069 is closed on this head, but #801 still is not safe to merge independently.
What is now fixed:
- Hosted
code-cianddrive9-server-local e2e smokeare green on this exact head. - The quota-cache tests now assert the intended stale-then-async-refresh behavior instead of the old synchronous refresh contract.
waitForQuotaConfigLoadwaits onloadDoneand does not bypass the behavior assertion: the first expired warm call must still return stale, and the later read must see the refreshed snapshot. - The
IdleTimeout=0reaper regression is now aligned with the intended contract:Pool.Startshould keep the shared physical-DB handle reaper running while standalone tenant idle eviction remains disabled. - Prior #801 code concerns remain closed: panic-wedge recovery, warm-expired foreground latency, LRU-neutral shared cached acquire, and shared-handle reaper independence.
Remaining blocker:
- #801 still contains the 20x shared backend cap increase (
cmd/drive9-server/main.go:49,defaultSharedBackendCacheMaxTenants = 20480) while this branch still hasPool.closeEntrycallmetrics.DeleteTenantCounters(pkg/tenant/pool.go:1647). That makes the metrics-lifecycle cleanup in #802 part of the same safety boundary, not an optional follow-up. Current #802 head is09debf242306518087545c06b8d69a5cddd9be1c, stillCHANGES_REQUESTED, with hostedcode-cifailing; an exact synthetic merge of #80160dfea19+ #80209debf24still conflicts inpkg/tenant/pool_test.go. Therefore there is still no executable, tested merge result that proves the 20x cap and metrics cleanup land together.
Required before this can be green: either remove the 20x cap increase from #801, or include/prove the metrics cleanup in the same resolved and green merge result with #802/equivalent changes.
Validation performed on this head:
- Passed locally:
git diff --check origin/main...HEAD, changed-filegofmt -l,go test ./cmd/drive9-server -run '^TestTenantBackendCacheMaxTenants' -count=1, test binary compilation forpkg/backend,pkg/tenant, andcmd/drive9-server,go vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/server, andgo build ./.... - Local runtime
go test ./pkg/tenant ...is blocked by the machine's knownrootless Docker not foundTestMain bootstrap, so I relied on hosted code-ci for package runtime coverage.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Reviewed exact base bf995bbab94f13379380097f2838941323b993ac → head 60dfea1999ab825aebbabe403d00eb5e3113d699.
CHANGES_REQUESTED
The new test-only revision correctly closes the prior exact-head CI blocker. waitForQuotaConfigLoad synchronizes with the detached refresh instead of weakening assertions; the expiry test now pins stale-first then refreshed publication, the failure test waits before checking the retry cooldown, and the reaper test now asserts the intentional IdleTimeout=0 physical-handle-reaper behavior. Fresh hosted ci and local-e2e are green. The panic-wedge, expired-warm-leader latency, LRU-neutrality, and shared-handle-reaper fixes remain sound.
One merge blocker remains:
- This exact PR still cannot merge independently while it contains the 20× shared resident-cap increase without the metrics-lifecycle fix.
cmd/drive9-server/main.gostill changes the shared-provider default from1024to20480, while this head's capacity-eviction path still reachesPool.closeEntryand synchronously callsmetrics.DeleteTenantCounters, which scans tenant-labeled metric vectors under registry locks. PR #802 is the intended companion, but it is not in this head. Its latest head is09debf242306518087545c06b8d69a5cddd9be1c, its hostedciand review decision are currently failing/CHANGES_REQUESTED, andgit merge-tree --write-treefor exact #801 head60dfea19plus exact #802 head09debf24still conflicts inpkg/tenant/pool_test.go.
Required: choose one executable boundary:
- remove the 20× cap/provider-cap wiring from #801 so the now-green quota-cache/LRU/reaper work can merge independently; or
- produce a real resolved/rebased #801+#802 combined head, close #802's correctness blockers, and run both normal/race/full hosted gates on that exact merge result.
Validation on exact head 60dfea19:
- full PR
git diff --check; changed-filegofmt -l - normal and race compile-only gates for
pkg/backend,pkg/tenant, andcmd/drive9-server go vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/server- normal/race
TestTenantBackendCacheMaxTenants - isolated normal/race execution of all
TestQuotaConfigCache*tests andTestIdleEvictionStartKeepsSharedHandleReaperWhenDisabled make lint;make build-server- hosted
ci,drive9-server-local e2e smoke, and CodeRabbit are green - exact #801/#802 merge-tree conflict reproduced
The worktree is clean. Please request another exact-head review after the cap is split out or the real combined merge boundary exists.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Correction / refreshed companion check for exact #801 head 60dfea1999ab825aebbabe403d00eb5e3113d699:
PR #802 advanced during my review from 09debf24 to 5e9af58e42779a11e9213dd1d4f0eb607723637b. I fetched that new exact companion head and reran the composition gate. The core verdict is unchanged:
CHANGES_REQUESTED
- #801's own quota-cache/LRU/reaper implementation and revised tests are green on
60dfea19. - #801 still contains the shared cap increase to
20480and still lacks the companion metrics-lifecycle change in its own merge result. git merge-tree --write-tree refs/review/pr801-r5 refs/review/pr802-r3for exact #80160dfea19+ exact #8025e9af58estill produces a content conflict inpkg/tenant/pool_test.go; there is no real combined revision to test or approve.- #802's hosted
ciand local-e2e are currently in progress on5e9af58e, and its formal review decision has not yet cleared.
Therefore #801 remains non-mergeable as an independent PR unless the 20× cap change is removed. Otherwise, resolve the cross-PR conflict into a real combined head and run both PRs' normal/race/full hosted gates on that exact result.
This note supersedes only the stale #802 head/status identifiers in my immediately preceding review; all #801 exact-head validation and the composition requirement remain valid.
|
Correction to my exact-head review above: PR #802 advanced during submission from The verdict remains CHANGES_REQUESTED for #801 head This correction supersedes only the stale #802 head/status identifiers in review #801 (review); the #801 validation and required boundary are unchanged: either remove the 20× cap increase from #801 or provide a resolved, fully tested combined head. |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
R4 at exact head 60dfea1999ab825aebbabe403d00eb5e3113d699: REQUEST_CHANGES.
The #802 composition blocker has changed, but this PR is still not approvable at the current head. #802 is now merged on main as c67c7105 (PR #802 head 5e9af58e) and its checks are green, so the missing metrics-cleanup prerequisite is available on main. However #801 is still based on bf995bba, GitHub reports mergeStateStatus=DIRTY, and a local synthetic merge of current origin/main (c67c7105) with #801 head fails with a content conflict in pkg/tenant/pool_test.go.
Because there is no resolved merge/rebase head, the green checks on 60dfea19 do not validate the actual post-#802 combined state. Please rebase/merge #801 onto current main, resolve the pkg/tenant/pool_test.go conflict, and rerun code-ci plus drive9-server-local e2e smoke on the new exact head before approval.
I did not find a new #801-owned correctness blocker on the stale head in this pass. The 20x shared default cap, lazy quota config refresh semantics, refresh ownership/waiter behavior, LRU-neutral cached acquire behavior, and independent shared-handle reaper tests still match the previously reviewed intent.
Validation performed:
gh pr view 801: head60dfea19, basebf995bba,mergeStateStatus=DIRTY, checks green on the stale head.gh pr view 802: merged atc67c7105, checks green.git merge-tree --write-tree origin/main HEAD: conflict inpkg/tenant/pool_test.go.git diff --check origin/main...HEADgofmt -lon changed Go filesgo test ./cmd/drive9-server -run '^TestTenantBackendCacheMaxTenants' -count=1go test -c ./pkg/backend ./pkg/tenant ./cmd/drive9-servergo vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/servergo build ./...
|
@qiffang PR #802 has now been merged into main as c67c710. I merged the current origin/main into this branch and resolved the pkg/tenant/pool_test.go conflict. The #801 merge result now includes the metrics cleanup from #802; new head is 26c14d3. code-ci and drive9-server-local e2e smoke are rerunning on this combined head. |
Re-review at
|
adversary-2 final verdict at
|
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
APPROVED at exact post-#802 head 26c14d3b80b98fefed0e234aa4237b9424aac39f (parents 60dfea1 + main@c67c7105). The conflict resolution preserves both #801 shared-reaper/LRU regressions and #802 live-counter lifecycle coverage; production Pool.closeEntry no longer deletes tenant counters, so the 20x cap and off-request-path metrics cleanup coexist in the tested build. Fresh local diff/gofmt/vet/lint/build, normal/race compile and focused quota/reaper/counter/cap gates pass. Hosted exact-head code-ci (including Test, failpoint, build), local E2E, and CodeRabbit are green.
|
@qiffang Fixed in cac4d5a. Warm quota-config refreshes now use a package-level non-blocking budget of 32 detached MetaDB loads, independent of the 20,480 tenant LRU capacity. A tenant that cannot claim a slot returns stale immediately, clears its loadDone ownership, and gets a 1s retry cooldown; no goroutine waits for a slot. Completed, failed, and panicking loads release their slot. Added TestQuotaConfigCacheAsyncRefreshHasGlobalSlotBudget with 40 simultaneously expired tenants; it verifies at most 32 blocked MetaDB loads and that deferred tenants can retry instead of remaining wedged. Verified normal and -race TestQuotaConfigCache runs. |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Holding approval at current exact head cac4d5abf90624be66a32752edcc9d5764856a93.
This PR is no longer at the reviewed/announced post-#802 head 26c14d3b80b98fefed0e234aa4237b9424aac39f. A new commit, cac4d5ab (fix: bound async quota refresh concurrency), was pushed on top and changes pkg/backend/quota_cache.go plus pkg/backend/quota_cache_test.go (+140/-3). That new code has not received my final review, and GitHub currently shows both hosted exact-head gates still running:
code-ci: IN_PROGRESS, run30236952247/ job89886453095drive9-server-local e2e smoke: IN_PROGRESS, run30236952243/ job89886453165- CodeRabbit: SUCCESS
Do not merge based on the 26c14d3 approval evidence. Re-review must be against cac4d5ab with exact-head hosted checks green. I will inspect the new quota-cache concurrency delta and update the verdict after the current head is fully reviewed and gated.
adversary-2 — re-anchored to
|
Re-review at
|
| # | Finding | Resolution |
|---|---|---|
| 1 | Canceled request → 5s tenant-wide quota fail-open | 5a998767 — verified |
| 2 | Safety-net scan inverts capacity LRU | fd2a813a — verified |
| 3 | loadMu waiters ignore deadline inside open tx |
5fe0d367 — verified |
| 4 | ~18ms global metrics-lock stall per cold open | #802, merged as c67c7105 and present here |
| 5 | Unbounded async refresh fan-out | cac4d5ab — verified |
| — | Shared handle lifetime / IdleTimeout=0 reaper |
06591fd1 — verified |
Non-blocking items were resolved or consciously declined with sound reasoning: 5s failure retry without backoff (owner decision, and fix 3b means warm callers add zero queries during an outage, which materially shrinks the concern I'd raised), mixed-provider capacity budget (out of scope; standalone entries still get the idle TTL), and the GetQuotaConfigVersion / stop() cleanup (done in c0b8625a).
One note for the archive rather than a change request: the 32-slot budget and the 1s slot-retry interval are both compile-time constants. If a shared pod's steady state turns out to saturate the budget, the symptom will be quota config converging slower than the nominal 30s TTL, visible only as a rising "deferred" rate. Worth a dashboard line, and worth making env-tunable if it ever does saturate — but not something to hold the merge for, since the current defaults leave sensible headroom against the 100-connection meta pool.
LGTM. The end state is better than where this started: 06591fd1 fixed a genuine latent bug (IdleTimeout=0 silently disabling physical shared-handle reclamation) that was independent of the scaling work, and the stale-first get() is a cleaner design than the TryLock I originally sketched, since it makes the warm path unconditionally non-blocking rather than usually-non-blocking. Good iteration on a change with a lot of subtle second-order effects.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Final R5 at exact head cac4d5abf90624be66a32752edcc9d5764856a93: APPROVED.
This approval supersedes my prior hold on the same head. The branch no longer relies on the superseded 26c14d3 evidence: I reviewed the new cac4d5ab delta (fix: bound async quota refresh concurrency) and found no remaining blocker.
What changed in R5:
- Adds a global 32-slot budget for warm-cache async quota config refreshes, preventing a MetaDB stall from producing one detached refresh query per resident tenant under the 20x shared-cache cap.
- Cold initial loads are not throttled by this async budget.
- Successful async refreshes release the slot via deferred release, including panic/recover paths.
- Full-slot warm refreshes close
loadDone, return stale config, and defer retry for 1s instead of busy-looping per request. - The new
deferredservice result is not classified as a tenant operation failure.
Post-#802 composition remains closed at this head: the 20x shared cap is present, and production Pool.closeEntry no longer calls metrics.DeleteTenantCounters, so capacity eviction no longer triggers request-path counter-registry scanning.
Validation:
- Hosted exact-head
code-ci: SUCCESS (30236952247/89886453095) - Hosted exact-head
drive9-server-local e2e smoke: SUCCESS (30236952243/89886453165) - CodeRabbit: SUCCESS
- Local
git diff --check origin/main...HEAD - Local changed-Go
gofmt -l - Local
go test -c ./pkg/backend - Local
go test -race -c ./pkg/backend - Local
go test ./cmd/drive9-server -run '^TestTenantBackendCacheMaxTenants' -count=1 - Local
go test -race ./cmd/drive9-server -count=1 - Local
go vet ./pkg/backend ./pkg/tenant ./cmd/drive9-server ./pkg/meta ./pkg/server - Local
go build ./...
Local runtime go test ./pkg/backend -run '^TestQuotaConfigCache' remains blocked before test execution by the known package-level Docker/TestMain bootstrap error (rootless Docker not found); hosted CI covers the runtime package tests for this exact head.
Summary
tidb_cloud_native_sharedand 1,024 for standalone/native providers, while preservingDRIVE9_POOL_MAX_TENANTSoverridesAcquireCachedscans from changing capacity LRU order, so only foreground/durable tenant activity influences eviction priorityWhy
Shared tenants are lightweight
fs_id-scoped backends over shared physical DB handles. Applying the standalone idle TTL caused stable high-cardinality traffic to churn through cold opens. Raising the shared cache without removing per-tenant quota polling would also scale goroutine count and MetaDB version polling linearly, so quota config is now loaded only for active request paths.Shared physical
sql.DBhandles intentionally remain available while cached tenant entries reference them. This does not pin active database connections indefinitely: the shared connection pool prunes idle connections after its configured idle time, 5m by default. Capacity eviction releases tenant references; once a handle has no references, the existing 30m handle reaper closes it. Explicit invalidation and process shutdown close handles immediately.Test plan
make testgo test -race ./pkg/backend ./pkg/tenant ./cmd/drive9-server -count=1go test ./pkg/server ./pkg/meta -count=1make lintCGO_ENABLED=0 go build ./...git diff --checkSummary by CodeRabbit