fix: reclaim idle shared tenant cache groups

[srstack]

Summary

• keep only two tenant-cache access paths: Acquire for all real activity and AcquireCached for the warm-only safety-net scan
• treat request traffic, tenant workers, object GC, warm local S3 access, purge, and schema maintenance as real activity that promotes the tenant LRU and refreshes physical shared-DB activity
• reclaim an idle physical shared-DB group after 10 minutes of no real activity; with the existing 2-minute reaper interval, cleanup occurs after approximately 10-12 minutes
• reserve the physical shared handle before tenant cold-open and transfer that generation-specific lease to the cached tenant entry, preventing the reaper from closing the handle in flight and preventing stale releases from corrupting reopened-handle refs
• pin local S3 requests for their complete handler lifetime
• keep the unsigned local mock-S3 gateway warm-only so a URL tenant ID cannot cold-open a tenant database
• release the shared-handle mutex before scanning the capacity LRU and batch removal metrics outside the request-path lock
• preserve the target-TiDB advisory schema lock behavior merged in fix: improve shared pool provisioning and capacity reuse #808

AcquireCached never cold-opens, promotes the capacity LRU, or refreshes tenant/physical idle activity. There is no separate foreground/work state machine: background work is demand-driven real use, while the periodic safety-net observation remains explicitly warm-only.

Local mock-S3 requests also cannot cold-open because those development presigned URLs do not carry independent tenant authentication. An already-warm hit is promoted and refreshed as real activity, then pinned until the HTTP handler completes.

No schema, environment variable, durable state, per-tenant TTL, or new goroutine is added. The shared cache capacity default remains 20,480. Obsolete unpinned Get/S3Backend accessors were removed so new callers cannot bypass the two-path ownership model.

Safety

• a physical handle lease increments the same sharedDBRefs counter used by cached tenant ownership before the handle leaves sharedMu
• lease release verifies both db_pool.id and the exact *sql.DB generation, so a delayed release after invalidation cannot decrement a reopened handle
• cold-open failures and duplicate cold-open losers release their lease; successful cold opens transfer it to the cache entry until entry close
• active tenant entries can be retired from the LRU but remain pinned and usable until their request/work references drain
• tenant stores and physical SQL handles close outside pool locks
• cascade expiration is revalidated under p.mu -> p.sharedMu, then the potentially 20,480-entry LRU scan runs with only p.mu

Verification

• GOCACHE=/tmp/drive9-go-cache go test ./pkg/tenant -count=1
• GOCACHE=/tmp/drive9-go-cache go test ./pkg/server -run 'Test.*(TenantWorker|ObjectGC|Auth|S3|LeaderGatedWorkers)' -count=1
• GOCACHE=/tmp/drive9-go-cache go test -race ./pkg/tenant -run 'TestSharedDBHandleLease|TestSharedDBIdleReap|TestAcquireCached|TestAcquireCounts|TestAcquireRefreshes|TestLoadS3Backend' -count=1
• git diff --check

[coderabbitai]

📝 Walkthrough

Walkthrough

Tenant backend loading now uses releasable acquisitions. Shared database handles use generation-aware leases, activity refresh, and coordinated idle reaping. Server proxy handling and lifecycle tests were updated for the new pool API and cleanup behavior.

Changes

Tenant pool lease lifecycle

Layer / File(s)	Summary
Shared handle lease ownership pkg/tenant/pool.go, pkg/tenant/pool_test.go	Shared database handles use leases transferred through backend creation, with consistent cleanup and wrapped failure returns.
Acquire, reaping, and release flow pkg/tenant/pool.go	Tenant acquisition refreshes activity, cached entries retain shared leases, and expired shared-database entries are retired while references drain.
Releasable backend loading pkg/tenant/pool.go, pkg/server/server.go	LoadS3Backend returns a backend and release callback; the /s3/ proxy defers release and handles unavailable backends.
Lease and lifecycle regression coverage pkg/tenant/pool_test.go, pkg/server/leader_lifecycle_test.go	Tests cover lease generations, pinned backends, idle reaping, metrics, LRU activity, and updated acquisition-based setup.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant S3Proxy
  participant Pool
  participant SharedDB
  Client->>S3Proxy: Request tenant S3 path
  S3Proxy->>Pool: LoadS3Backend
  Pool->>SharedDB: Acquire shared handle lease
  SharedDB-->>Pool: Backend and release callback
  Pool-->>S3Proxy: Backend and release callback
  S3Proxy-->>Client: Serve S3 request
  S3Proxy->>Pool: Release backend

Loading

Possibly related issues

• InvalidateSharedDB corrupts shared DB ref count across invalidate-then-reopen #783 — The lease and generation-aware release changes address invalidate-then-reopen reference accounting.

Possibly related PRs

• mem9-ai/drive9#782 — Overlaps the shared-handle cache lifecycle and gauge updates.
• mem9-ai/drive9#801 — Overlaps shared-tenant cache acquisition and idle-reaping behavior.
• mem9-ai/drive9#773 — Overlaps tenant entry cleanup in closeEntry.

Suggested reviewers: mornyx, qiffang, jayson-huang

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: reclaiming idle shared tenant cache groups.

✨ 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/shared-db-cache-lifecycle

---

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.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 00915be970

ℹ️ 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".

[Copilot]

Pull request overview

This PR refines the tenant backend pool’s caching semantics for shared-schema tenants by separating foreground (request-path) access from background work, and by reclaiming cached shared-DB tenant groups when their physical shared DB has been idle from a foreground perspective for a fixed TTL.

Changes:

• Introduces AcquireForeground and AcquireForWork (with Acquire as a foreground compatibility alias) to prevent background work from promoting capacity-LRU entries or extending shared physical-DB foreground activity.
• Adds shared physical-DB “foreground-idle” reaping that cascades retirement of cached tenant entries for an expired shared DB, then closes the shared handle after refs drain.
• Updates request authentication and background workers to use the appropriate acquisition mode, and extends pool tests to cover the new behaviors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/tenant/pool.go	Adds access-kind-aware acquisition paths and shared-DB foreground-idle cascade reaping/handle cleanup.
pkg/tenant/pool_test.go	Updates and adds tests for work-vs-foreground LRU behavior and shared-DB foreground-idle reaping cascades.
pkg/server/tenant_worker.go	Switches kick-driven background tenant work to AcquireForWork.
pkg/server/object_gc_worker.go	Switches object-GC candidate processing to AcquireForWork.
pkg/server/auth.go	Switches request-path auth acquisition to AcquireForeground.

---

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

[mornyx]

Careful review pass — the lifecycle split is correct and complete. Verified against the PR head:

• Locking is sound. The documented order p.mu -> p.sharedMu holds at every site (acquire hit/insert paths, releaseEntry, the cascade revalidation). removeLocked never touches sharedMu, so the cascade's double-lock section cannot deadlock; closeEntry -> releaseSharedDB runs after both locks are released. The two-phase candidate-then-revalidate design closes the race where a foreground acquire refreshes the physical timestamp mid-cascade — every touchSharedDBForegroundLocked call site holds p.mu, which the revalidation phase blocks on.
• Caller classification is complete. Grepped all non-test Acquire call sites: auth middleware (API-key + capability) uses AcquireForeground; tenantWorkerManager.targetForRef and the object-GC worker use AcquireForWork; the safety-net scan keeps AcquireCached (correctly refresh-free). Get/S3Backend/LoadS3Backend only serve the local mock-S3 endpoint (cfg.S3Dir), so no production foreground path bypasses the foreground timestamp.
• Retired-but-active entries stay usable until refs drain (covered by TestSharedDBIdleReapDefersActiveEntryClose), and the physical handle closes only after both the foreground TTL expiry and reference drain — the reap order in reapOnce (cascade, then reapIdleSharedDBs) lets both happen in one tick.
• Standalone tenants are unaffected: sharedDBID == 0 skips the touch, the cascade only iterates real handle IDs, and work access still refreshes the standalone idle TTL.

Tests: all pool lifecycle tests pass locally, including a -race run over the cascade/foreground-activity/work-LRU tests; CI green.

Two non-blocking operational notes inline. LGTM.

[srstack]

Reviewed the full change plus the downstream call graph (pkg/server/auth.go, tenant_worker.go, object_gc_worker.go, safety_net_scan.go, LoadS3Backend, PurgeSharedTenant/EnsureSharedDBReady). Build and go vet are clean, and the new tests pass. The foreground/work split is the right model, but dropping the timestamp refresh from the handle lookup opens a window that did not exist before this PR. Two correctness issues; both reproduced with throwaway probe tests against the real pool.

1. An expired handle can be closed while a cold open is in flight

sharedDBHandle no longer refreshes sharedDBLastForegroundUsed on a cache hit (pkg/tenant/pool.go:1160). That makes this interleaving reachable:

1. createBackend gets the handle from the cache hit — no timestamp refresh.
2. The reaper runs reapIdleSharedDBs: the foreground TTL is expired and sharedDBRefs[dbID] == 0 (every other tenant on that physical DB has already been retired by the cascade), so it closes the handle and deletes it from sharedDBs.
3. The cold open continues and builds a datastore.Store over the now-closed handle.

Probe result: the handle is sql: database is closed, and sharedDBIDForStore returns 0, because it identifies the shared DB by reverse-looking-up the *sql.DB pointer in p.sharedDBs (pkg/tenant/pool.go:994). So the shared tenant gets cached as if it were standalone — it is invisible to the cascade, it becomes subject to the standalone idle TTL instead, and closeEntry never releases a shared ref for it. The request fails until the entry is replaced.

Before this PR the handle lookup refreshed the timestamp, so the reaper could not see an expired window while a cold open was resolving. That protection is what the change removes.

2. The same race leaks sharedDBRefs permanently, so the handle can never be reclaimed again

If sharedDBIDForStore observes the handle but the reaper deletes it before retainSharedDB runs (pkg/tenant/pool.go:503):

• retainSharedDB (:1009) increments unconditionally.
• releaseSharedDB (:1019) returns early when the dbID is absent from sharedDBs, so that +1 is never given back.

Probe result: refs == 1 after release with no cached handle. Once the handle is reopened, the sharedDBRefs[dbID] > 0 guard in reapIdleSharedDBs is permanently true and that db_pool's handle is never reclaimed again. This directly defeats the goal of the PR, and it is monotonically worsening — every occurrence pins one more physical handle for the lifetime of the pod.

Suggested fix for both: have sharedDBHandle hand back an in-flight reference under sharedMu before returning (e.g. increment sharedDBRefs[dbID] on the way out, with the caller returning it on the failure path), so the reaper can see the pending cold open. The in-flight reference and retainSharedDB must share one counter, otherwise #2 survives.

Non-blocking

• Cascade lock-hold time. reapExpiredSharedDBTenantEntries (:1080) walks the entire LRU while holding both p.mu and p.sharedMu. At the shared-deployment default of 20,480 entries a full cascade measured 20.6ms (750ns when nothing is expired). p.mu is the request-path AcquireForeground lock, so that is a ~20ms stall once per reap interval. Acceptable, but each removal emits two metrics inside the lock (removeLocked -> recordCachedBackendCountLocked plus RecordOperation); batching those and emitting after the unlock removes most of it cheaply.
• The Acquire alias has no production callers — only tests. Keeping it means new code can silently pick up foreground semantics. Consider deleting it and making the tests say AcquireForeground explicitly, so the distinction stays enforced by the compiler.
• Get is not part of the new model. Get (:314) unconditionally does MoveToFront but never touches the shared foreground timestamp. Its only production caller is the S3 gateway via LoadS3Backend (pkg/server/server.go:663), which is foreground traffic. Net effect: S3 gateway requests promote the capacity LRU but do not stop the 30-minute cascade from retiring their own entry. Either add touchSharedDBForegroundLocked there, or document why the S3 gateway is deliberately not foreground. S3Backend (:840) has the same asymmetry — it refreshes lastUsed only.
• PR description vs. behavior. The description says a work-only cold entry at full capacity "retires itself instead of evicting foreground-hot tenants." The implementation is PushBack plus eviction from Back(), so at a full cache the new entry is removed within the same acquire call. That means AcquireForWork cold-opens on every kick for a full cache, even for a tenant kicked repeatedly. Probably fine at the shared default of 20,480, but on standalone (default 1,024) with high tenant counts this turns background work into sustained cold opens of the same tenant TiDBs — which is exactly the cost feat: unified tenant outbox — eliminate all periodic tenant TiDB scans for serverless scale-to-zero #660 removed. Worth a comment at minimum, or exempting the work miss from its own eviction pass.

Merge-order note with #808

#808 touches the same lines and keeps the sharedDBLastUsed refresh inside sharedDBHandleWithSchemaLockWait, which this PR removes. GitHub still reports both as MERGEABLE. If #808 lands second, its restored refresh would incidentally mask #1 and #2 — coincidence, not design. Please resolve the two issues here on their own terms regardless of merge order.

[srstack]

Addressed the full review in d805f44 after rebasing onto merged #808. The two reproduced correctness races are fixed with a generation-specific physical-handle lease acquired under sharedMu and transferred into cached-entry ownership; cold-open failures and duplicate losers release it, and stale releases cannot affect a reopened handle. The long cascade no longer holds sharedMu during the LRU scan, and removal metrics are emitted outside the request-path lock. I also removed the foreground/work split plus the obsolete unpinned Get and S3Backend paths: all demand-driven request, worker, object-GC, and local-S3 use now goes through Acquire, while only the periodic safety-net uses warm-only AcquireCached. Local S3 is pinned for the full handler lifetime. The physical idle TTL is a fixed 10 minutes, yielding roughly 10-12 minute cleanup with the existing reaper. Full pkg/tenant, affected pkg/server tests, focused race tests, and diff checks pass locally; new-head CI is running.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/tenant/pool_test.go (1)

1604-1840: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer t.Errorf for the assertion failures in these new tests.

Lines 1632, 1704, 1748, 1801, and 1837 use t.Fatal/t.Fatalf for assertions rather than setup failures, which aborts before the remaining store/handle checks in the same test can report. As per coding guidelines: "Use t.Fatal or t.Fatalf for setup failures and t.Errorf for assertion failures."

♻️ Example for Line 1703-1705

 	if firstCached || secondCached || !otherCached {
-		t.Fatalf("cache after shared cascade: first=%t second=%t other=%t, want false false true", firstCached, secondCached, otherCached)
+		t.Errorf("cache after shared cascade: first=%t second=%t other=%t, want false false true", firstCached, secondCached, otherCached)
 	}

🤖 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/pool_test.go` around lines 1604 - 1840, Update the assertion
failures in TestAcquireCountsBackgroundWorkAsCapacityLRUActivity,
TestSharedDBIdleReapCascadesTenantEntries,
TestSharedDBIdleReapDefersActiveEntryClose,
TestAcquireRefreshesSharedDBActivity, and TestAcquireRefreshesStandaloneIdleTTL
to use t.Errorf or t.Errorf-style reporting instead of t.Fatal/t.Fatalf. Keep
t.Fatal/t.Fatalf for setup failures such as Acquire, sql.Open, and Ping errors.

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.

Inline comments:
In `@pkg/tenant/pool.go`:
- Around line 878-896: The LoadS3Backend path currently acquires tenant backends
before the local S3 handler authenticates requests. Update LoadS3Backend and its
/s3/ integration so backend loading is warm-only or otherwise gated by
successful tenant/S3 authentication, ensuring unauthenticated cold requests
cannot open a tenant backend while preserving authenticated S3 operations.

---

Nitpick comments:
In `@pkg/tenant/pool_test.go`:
- Around line 1604-1840: Update the assertion failures in
TestAcquireCountsBackgroundWorkAsCapacityLRUActivity,
TestSharedDBIdleReapCascadesTenantEntries,
TestSharedDBIdleReapDefersActiveEntryClose,
TestAcquireRefreshesSharedDBActivity, and TestAcquireRefreshesStandaloneIdleTTL
to use t.Errorf or t.Errorf-style reporting instead of t.Fatal/t.Fatalf. Keep
t.Fatal/t.Fatalf for setup failures such as Acquire, sql.Open, and Ping errors.

🪄 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: 5208f5f5-385a-4d5a-9933-c26581a27256

📥 Commits

Reviewing files that changed from the base of the PR and between 6ce870c and d805f44.

📒 Files selected for processing (4)

• pkg/server/leader_lifecycle_test.go
• pkg/server/server.go
• pkg/tenant/pool.go
• pkg/tenant/pool_test.go

[srstack]

Re-review of the revised version (3ff728a) — LGTM ✅

Verified the two blocking correctness bugs from my earlier comment are fixed, and the two non-blocking notes were addressed.

Both blocking bugs fixed (re-confirmed with throwaway probe tests):

1. Handle closed under an in-flight cold open → the new sharedDBHandleLease pins the shared *sql.DB across the reaper, so a cold open can no longer be mis-cached as a standalone store over a closed handle. Release() is generation-safe: it checks sharedDBs[dbID] == l.db (exact pointer) before decrementing, so a stale lease can't decrement a re-created handle.

2. Permanent sharedDBRefs leak → gone. Refs are taken via leaseSharedDBLocked and always returned through lease.Release() (guarded by sync.Once), including closeEntry now calling e.sharedDBLease.Release() instead of the old releaseSharedDB(id). Handles become reclaimable again.

Non-blocking notes addressed:

• Cascade lock: reapExpiredSharedDBTenantEntries now releases sharedMu before the LRU scan and coalesces metrics after unlocking p.mu — the combined-lock stall I measured is gone.
• Dead code removed (Get, S3Backend, the foreground/work Acquire split). Cleaner surface: single Acquire + warm-only AcquireCached.
• Idle TTL reduced 30m → 10m.

One minor note (non-blocking): LoadS3Backend is now warm-only and returns 404 for a cold tenant, while the client only re-presigns on 403 (404 is a hard error). This is the local mock-S3 gateway path only, and the PR body frames it as intentional (an unsigned URL tenant ID must not cold-open a tenant DB) — I agree with that tradeoff. The only observable effect is a cold tenant evicted mid-transfer getting a non-retryable 404 on the local path; with 10-min presign TTLs the window is small and it's dev/test-only. Fine as-is; flagging only for awareness.

Clean, no technical debt left behind. LGTM.

(Note: GitHub blocks formally approving one's own PR, so this comment stands in for the approval.)

[mornyx]

Re-reviewed the new head after the redesign (5609f37f9..3ff728a22). The lease-based ownership model is a strict improvement over both the original and my previously approved revision:

• Unified Acquire with all activity counting removes the foreground/work split — my earlier note about background single-use churn on a saturated cache is now moot, and the entry-level lastUsed semantics are much easier to reason about.
• The generation-safe sharedDBHandleLease fixes three real latent bugs: a delayed release can no longer decrement refs of a reopened handle generation (the old releaseSharedDB(dbID) had no generation check); PurgeSharedTenant now pins the handle across its batched deletes instead of using an unpinned raw handle; and an in-flight cold open holds a lease, so the 10-minute reaper can't close a handle mid-open.
• LoadS3Backend going warm-only with a pinned release closes the two holes in the old path: unauthenticated presigned-URL requests can no longer trigger a cold open, and the returned backend is pinned until release() instead of being closeable underfoot by eviction/cascade.
• Cascade reaper changes check out: releasing sharedMu before the LRU walk keeps unrelated handle opens unblocked, while the expiry revalidation still happens under p.mu so no activity touch can interleave between revalidation and retirement (every touchSharedDBActivityLocked caller holds p.mu). A new acquire landing between the cascade and reapIdleSharedDBs re-leases and refreshes lastUsed, so the handle correctly survives the same reap tick.
• Lock order p.mu -> p.sharedMu holds at every site; closeEntry releases backend -> store -> lease in the right order; the double-create loser path releases all three.

Verified locally: full pkg/tenant suite, pkg/server leader-lifecycle + shared-DB tests, -race over the new lease/cascade/activity tests and TestLeaderGatedWorkersCloseRacesOnLead; CI green.

Two non-blocking notes inline. LGTM.

[mornyx]

Worth calling out in the deploy note: LoadS3Backend is now warm-only, so a local mock-S3 presigned-URL fetch for a tenant whose entry has expired returns 404 instead of lazily cold-opening. That is strictly better (an unauthenticated URL should not trigger a cold open, and the pin-until-release semantics are now correct), but any local tooling that fetches URLs long after the upload request must expect to re-warm the tenant through the API first.

[mornyx]

Non-blocking: with the TTL shortened 30m -> 10m and sharedDBHandle leasing itself counting as activity, a physical group with only sporadic traffic (less than one acquire/lease per ~10 min) will now be fully reclaimed each window and pay a cold reopen plus per-tenant cold opens on the first touch of the next window. Expected given the reclamation goal — just note that drive9_shared_db_pool_cache_handles will show this sawtooth for low-traffic orgs, which is distinguishable from a regression only if you know to look for it.