fix: reclaim tenant metrics on pool close

[srstack]

Summary

• reclaim only drive9_tenant_requests_total tenant series when a local tenant backend cache entry closes
• account for an authenticated request's tenant counter before its final backend release, so outer HTTP observation cannot recreate the series after close cleanup
• keep tenant gauges, histograms, SSE metrics, storage metrics, and the durable tenant-deletion cleanup contract unchanged
• delete from the request counter's own label map outside the tenant-pool mutex; no whole-registry scan is performed on eviction

Why this revisits #802

#802 deliberately stopped deleting metrics on pool eviction to preserve a never-reset tenant request counter. Production Tencent data shows that policy leaves roughly 20k unique request-counter tenants per pod while the local backend cache contains only hundreds of tenants. The agreed metric contract is now that tenant_requests_total may reset after cache eviction; dashboards must use rate() or increase() rather than treat the raw process-local counter as a durable tenant request ledger.

This PR deliberately does not restore #773/#802-era broad DeleteTenantCounters behavior on cache close. Durable deleting/deleted lifecycle transitions continue to use DeleteTenantCounters and the cross-pod WorkMetricsCleanup outbox.

Validation

• go test ./pkg/server -run '^TestRecordTenantHTTPRequestAtCompletionDoesNotRecreateCleanedCounter$' -count=1
• go test ./pkg/server ./pkg/metrics ./pkg/tenant -count=1
• make test was run earlier; it is not fully green due to unrelated existing failure cmd/drive9/cli.TestCtxImport_RejectsWorldReadableFile (mode-0644 JWT file expected to be rejected but received nil).

Summary by CodeRabbit

• Bug Fixes

  • Improved tenant request metrics accuracy by preventing duplicate request recordings.
  • Tenant request counters are now cleared when tenant resources are closed, while active in-flight metrics remain intact.
  • Prevented cleared counters from being recreated unexpectedly during request completion.
  • Added safeguards to keep request metrics consistent across authentication, streaming, and standard request flows.

• Tests

  • Added regression coverage for counter cleanup, preservation of in-flight metrics, and prevention of duplicate recordings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8421c5a1-026a-4873-a956-ceb324501d91

📥 Commits

Reviewing files that changed from the base of the PR and between f7dd66e and 409a892.

📒 Files selected for processing (7)

• pkg/metrics/operations.go
• pkg/metrics/registry.go
• pkg/server/auth.go
• pkg/server/instrumentation.go
• pkg/server/instrumentation_test.go
• pkg/tenant/pool.go
• pkg/tenant/pool_test.go

🚧 Files skipped from review as they are similar to previous changes (7)

• pkg/metrics/registry.go
• pkg/tenant/pool.go
• pkg/tenant/pool_test.go
• pkg/server/instrumentation.go
• pkg/server/instrumentation_test.go
• pkg/server/auth.go
• pkg/metrics/operations.go

---

📝 Walkthrough

Walkthrough

This PR adds selective deletion of tenant request counters, one-time completion recording, authentication middleware wiring, and cleanup during tenant pool entry closure. Tests verify that cleanup removes request counters without removing in-flight metrics.

Changes

Tenant Request Counter Cleanup

Layer / File(s)	Summary
Selective counter deletion pkg/metrics/operations.go, pkg/metrics/registry.go	The new functions delete only the matching tenant request counter series.
Guarded completion recording and auth wiring pkg/server/instrumentation.go, pkg/server/auth.go, pkg/server/instrumentation_test.go	Request state invokes tenant HTTP recording once at completion. Both authentication paths defer the recorder. A regression test checks that deleted counters are not recreated.
Pool entry close cleanup pkg/tenant/pool.go, pkg/tenant/pool_test.go	closeEntry removes tenant request counters after resource cleanup. Tests confirm that in-flight metrics remain.

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

Sequence Diagram(s)

sequenceDiagram
  participant AuthMiddleware
  participant RequestState
  participant Registry
  participant TenantPool

  AuthMiddleware->>RequestState: defer recordTenantHTTPRequestAtCompletion
  RequestState->>RequestState: observe registers recorder callback
  RequestState->>Registry: record tenant request counter once at completion
  TenantPool->>Registry: DeleteTenantRequestCounters on closeEntry
  Registry->>Registry: DeleteCounterByLabel removes matching series

Loading

Possibly related PRs

• mem9-ai/drive9#801: Both PRs modify tenant pool cleanup paths for tenant request metrics.
• mem9-ai/drive9#773: Both PRs refine tenant metric cleanup and request-counter deletion.
• mem9-ai/drive9#624: Both PRs modify tenant-scoped metrics in pkg/metrics/operations.go and pkg/server/instrumentation.go.

Suggested reviewers: mornyx

🚥 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 identifies tenant metric cleanup during pool entry closure, which matches the pull request’s primary change.
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 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/tenant-pool-metrics-cleanup

---

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: 5fae274dc5

ℹ️ 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 primarily bounds per-process Prometheus tenant-metric cardinality by tying tenant-scoped metric series to the lifecycle of the local tenant backend cache (deleting tenant series when the final cached entry closes, while avoiding deletion of a replacement entry’s series). It also introduces a small HTTP auth response behavior update for provisioning-related tenant statuses.

Changes:

• Reclaim tenant-scoped counters/histograms/gauges on tenant.Pool.closeEntry, while holding the pool mutex to avoid deleting metrics for a concurrently inserted replacement entry.
• Update tenant-pool tests to assert metric cleanup on close and preservation when a replacement entry is present.
• Add a retryable JSON error helper (Retry-After) and adjust auth middleware responses for pending/provisioning/failed tenant statuses, with new test coverage; add accompanying design/plan docs.

Reviewed changes

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

Show a summary per file

File	Description
pkg/tenant/pool.go	Deletes tenant metric series on entry close when no active cache entry exists for that tenant (mutex-serialized replacement detection).
pkg/tenant/pool_test.go	Adds/updates tests to validate metric cleanup on close and preservation for replacement entries.
pkg/server/server.go	Introduces errJSONRetryable helper (adds Retry-After + 503 JSON error).
pkg/server/auth.go	Uses retryable errors for pending/provisioning and changes TenantFailed handling (status code change).
pkg/server/auth_test.go	Adds coverage for auth responses (status codes and Retry-After presence).
docs/superpowers/specs/2026-07-31-tenant-pool-metrics-lifecycle-design.md	Documents intended metric lifecycle and concurrency contract.
docs/superpowers/plans/2026-07-31-tenant-pool-metrics-cleanup.md	Implementation plan for tenant-pool metric cleanup.

---

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

[srstack]

Review at a701c87

Reviewed the full diff, all 10 closeEntry call sites, the metrics registry internals, and both durable-cleanup paths. go test -race ./pkg/tenant -count=1 passes, the two new tests pass, go vet and gofmt are clean.

What's correct: the replacement-detection logic is precise. closeEntry only ever receives an entry with retired=true && refs==0 that removeLockedWithoutMetrics already deleted from p.items under p.mu, so a hit on p.items[e.tenantID] can only be a newly inserted replacement. And because removeLocked(..., "replace") and p.items[t.ID] = e happen in the same critical section (pool.go:495-501), the later re-lock in closeEntry is guaranteed to observe the replacement — the serialization claim in the description holds. I also verified no call site holds p.mu when calling closeEntry (all 10 call it after Unlock), so the added p.mu.Lock() cannot self-deadlock, and p.mu → registry.mu nesting already exists via recordCachedBackendCountLocked, so there's no new inversion.

---

Two things I'd want addressed before merge:

[MAJOR] This is an unacknowledged revert of #802, and the PR body doesn't say so.

git log -S shows the removed test TestCloseEntryPreservesLiveTenantCounters was introduced by c67c710 (#802, "reduce metric cardinality and close observability gaps"), which deliberately deleted this exact block from closeEntry — the diff being restored here is character-for-character what #802 removed, minus moving the delete inside the lock. So #823 reverses a 4-day-old intentional decision, and inverts the test that pinned it (...PreservesLiveTenantCounters → ...CleansTenantCounters).

That may well be the right call — #802's version had the replacement race this PR fixes, which is plausibly why it was pulled rather than fixed. But an inverted assertion on a deliberately-added regression test is exactly the change that needs its rationale in the commit message, or the next person re-reverts it. Please state in the body why #802's removal was wrong (or was an overcorrection) and why re-adding it under the lock is now safe.

[MAJOR] Local cache eviction is not tenant deletion — the two lifecycles are now conflated.

Every other DeleteTenantCounters caller is a durable death signal: clearLocalTenantMetrics (server.go:793, on deleting/deleted transition) and the WorkMetricsCleanup outbox row (tenant_outbox_poller.go:219, cross-pod). In all of those, the tenant is gone for good, so dropping its series is correct.

closeEntry is different: it fires on evict (LRU pressure), shared_idle, invalidate, and shutdown — for a tenant that is still alive and will likely be re-acquired minutes later. On a busy pod at maxSize, an active tenant can be evicted and re-acquired repeatedly, so its series get dropped and recreated repeatedly.

The body addresses this for counters ("consumers should use rate()/increase()"), and I agree that argument is sound for counters. But DeleteTenantCounters also calls DeleteHistogramsByLabel and DeleteGaugesByLabel, and it matches on the tenant_id label alone across the whole registry — roughly two dozen tenant_id-labelled instruments. rate() does not rescue a gauge: a tenant-scoped gauge that disappears and reappears reads as a genuine state change, not a reset, and histogram quantiles over a re-created series lose their window. Could you either narrow this to the counters the rate() argument actually covers, or confirm no tenant-labelled gauge/histogram is alerted on as an absolute value? The runbook expressions from #802 are the thing to check against.

---

[MINOR] DeleteTenantCounters under p.mu is a cost change, not just a placement change: it's three full registry scans (counters + histograms + gauges, each iterating every instrument's label set under the global registry.mu) now inside the pool mutex that serializes all tenant acquires. #802's version did the p.items lookup under the lock and the delete outside it. Since a replacement can only be inserted while p.mu is held, you can keep the exact same safety by snapshotting active under the lock and calling DeleteTenantCounters after Unlock — the replacement can't appear in between, because inserting it requires the lock you just released and it would have been visible to your read. Worth doing on the shutdown path especially, where this runs once per cached entry in a loop.

[NIT] TestCloseEntryPreservesReplacementTenantCounters builds the replacement with pool.items[tenantID] = &entry{...} directly rather than going through the acquire path, so it pins the p.items-lookup behavior but not the 495-501 same-critical-section property that actually makes it safe. A test that races a real replacing Acquire against the old entry's close would be a stronger guard — optional.

Mechanically the change is sound and well-tested; my hesitation is entirely about the two semantic points above. Happy to re-review once the rationale is in the body and the gauge/histogram question is settled.

(GitHub blocks a formal review verdict on one's own PR, so this comment stands in for it — treat it as changes-requested on the two MAJOR items.)

[srstack]

Addressed in f0480f63.

• The PR description now explicitly documents the intentional change from fix: reduce metric cardinality and close observability gaps #802: request counters may reset on local cache eviction because production has demonstrated that preserving them indefinitely creates unbounded per-process cardinality.
• closeEntry no longer calls broad DeleteTenantCounters. It now removes only drive9_tenant_requests_total; tenant gauges, tenant-labelled histograms, SSE metrics, storage metrics, and their durable-deletion cleanup semantics remain unchanged. This addresses the gauge/histogram concern.
• The new Registry.DeleteCounterByLabel scans only the request counter's label map. It is called outside p.mu, so eviction no longer performs a whole-registry scan while serializing acquires.
• The replacement-preservation test and p.items check were removed intentionally: the accepted contract is that cache close can reset this request counter, including during replacement. The next recorded request recreates the series.

Validation: go test ./pkg/metrics ./pkg/tenant -count=1 passes.

[srstack]

Re-review at f0480f6 — LGTM ✅

All three actionable items from my previous review are addressed, and the approach you took is better than what I suggested. go test -race ./pkg/tenant ./pkg/metrics -count=1 passes, the new test passes, go vet and gofmt clean.

[MAJOR] #802 rationale — resolved. The body now states the reversal explicitly and quantifies it: ~20k unique request-counter tenants per pod against a local cache holding hundreds. That's the concrete justification the inverted assertion needed, and it makes clear this is a scoped cardinality fix rather than a blanket revert. The explicit "this PR deliberately does not restore #773/#802-era broad DeleteTenantCounters behavior" line is exactly the kind of note that stops the next person from re-reverting.

[MAJOR] gauge/histogram conflation — resolved, better than proposed. Rather than auditing which tenant-labelled gauges are alerted on as absolute values, you narrowed the blast radius to the single instrument the rate() argument actually covers. DeleteTenantRequestCounters → DeleteCounterByLabel("drive9_tenant_requests_total", ...) touches one counter's label map; gauges and histograms are untouched by construction, so the reset-semantics mismatch I was worried about can't arise. I verified the instrument-name string matches the registration at operations.go:82 and that the registry keys counters by that exact name.

[MINOR] lock cost — resolved. The delete is now outside p.mu entirely (pool.go is a single added line), and it's no longer three whole-registry scans — one map lookup plus one instrument's label set. The shutdown loop concern is gone.

Test got stronger too. metricHasTenantID does per-line prefix matching instead of whole-body strings.Contains, which removes a real false-negative risk in the old assertion (a tenant_id substring match anywhere in the payload would have satisfied it). More importantly it now asserts the positive half of the new contract — drive9_tenant_inflight_requests must survive the close — so the "counter only, never gauges" boundary is pinned in both directions rather than assumed.

---

Two non-blocking notes:

• [NIT] Dropping the p.items replacement check is fine here, but for a different reason than the v1 design, so worth recording: the counter is fed only from recordTenantHTTPRequest (instrumentation.go:710) on the live HTTP path, independent of pool state. A replacement acquiring concurrently with an old entry's close can therefore lose a few just-recorded increments. Under the now-explicit rate()/increase() contract that's a bounded, self-healing reset — the same class of event the contract already sanctions — so no check is needed. v1's p.items guard was protecting a much wider deletion, and it correctly went away with it.
• [NIT] DeleteCounterByLabel returns silently when r.counters[name] is nil, so if drive9_tenant_requests_total is ever renamed, this cleanup becomes a no-op and the cardinality regression returns quietly. The name is already duplicated as a literal at operations.go:302 while registered at :82 — extracting a shared const would make a rename fail loudly at compile time instead.

Scoped correctly, cheaper, better documented, and the contract is now pinned by tests in both directions. LGTM.

(GitHub blocks a formal approval on one's own PR, so this comment stands in for it.)

[mornyx]

Review summary

Verdict: Approve

Reviewed in an isolated worktree at f0480f6. Focused fix for process-local Prometheus cardinality of drive9_tenant_requests_total after #802 stopped reclaiming series on cache eviction. Local tests passed:

• go test ./pkg/tenant -run '^TestCloseEntryCleansTenantRequestCounters$'
• go test ./pkg/metrics

CI green. The design matches the stated contract: request counters may reset on cache eviction and must be queried with rate() / increase(), while durable deleting/deleted cleanup still uses full DeleteTenantCounters + cross-pod WorkMetricsCleanup.

Correctness

1. Scope is intentionally narrow — DeleteTenantRequestCounters only touches the drive9_tenant_requests_total counter map for one tenant_id. Gauges/histograms/SSE/storage are left alone. Request duration histogram is already not labeled by tenant_id (only surface/status_class), so it is not the cardinality hot spot.
2. Placement — called from closeEntry, which runs when an entry is retired and refs hit 0 (idle eviction, invalidate after last release, replace, shutdown). Not under the pool mutex; only the metrics registry lock is taken for a single counter instrument — good.
3. In-flight request order — observe is outer: next.ServeHTTP completes (auth defer release() may closeEntry + delete counters) then recordTenantHTTPRequest increments the counter again. So the completing request is not lost; the series is recreated from 0 with the last sample. Compatible with rate()/increase(). Historical pre-eviction series intentionally disappear.
4. Pin safety — removeLockedWithoutMetrics returns the entry for immediate close only when refs == 0; otherwise close is deferred to the last releaseEntry. No close while pins remain.
5. Lifecycle cleanup unchanged — durable delete path still calls full DeleteTenantCounters via outbox / work mask.
6. Test — asserts request counter gone and inflight gauge retained after closeEntry.

Non-blocking notes

• Codex P2 about “delete before observe” is real order-wise but not a functional loss of the completing request; series reset is the product intent. No code change needed if dashboards use rate()/increase().
• Optional unit test for Registry.DeleteCounterByLabel (name-scoped delete) to lock the O(series for one counter) contract.
• Inflight gauges with tenant_id can still linger at last value after eviction; out of scope here and probably lower cardinality, but worth watching if gauge series also grow.

No blockers. Approved.

[coderabbitai]

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/tenant/pool_test.go`:
- Around line 1896-1902: Update the test helper metricHasTenantID to accept
*testing.T as its first parameter and call t.Helper() immediately at the start;
then update both metricHasTenantID call sites to pass the current testing
handle.

🪄 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: cd61e2cc-0780-46c4-8268-772a87a40b19

📥 Commits

Reviewing files that changed from the base of the PR and between f4ef4d2 and f7dd66e.

📒 Files selected for processing (7)

• pkg/metrics/operations.go
• pkg/metrics/registry.go
• pkg/server/auth.go
• pkg/server/instrumentation.go
• pkg/server/instrumentation_test.go
• pkg/tenant/pool.go
• pkg/tenant/pool_test.go