perf: replace lifetime spending scans with reconciled counters

[henrypark133]

API-key lists, API-key admission checks, and the platform billing summary currently sum lifetime usage logs for every read. Replace those aggregates with the transactional counters maintained by #1116 and reconciled by #1119.

Stack: #1116 → #1119 → this PR. Deploy and run the #1119 backfill before deploying these readers. API startup now refuses incomplete or missing organization balances, using the existing database-prerequisite pattern. There is no raw-query fallback or feature flag.

• Key lists join one api_key_spend row per key; workspace visibility, deleted-key filtering, sorting, pagination, and unused-key zeroes remain unchanged.
• API-key admission reads inference_spent only, preserving the existing policy that excludes service charges.
• Billing consumed totals use one scan of organization_balance. The legacy total remains independent of the inference/service splits, preserving historical discrepancies.

Repository tests invoke the actual reader methods in an isolated schema with no raw usage tables. All three fail on the original queries and pass on the counter queries. They cover inference/service/mixed/unused/deleted keys, both sort directions, workspace isolation, pagination beyond the end, admission service exclusion, and legacy billing totals. The existing API-key ordering fixture now posts through the real accounting repositories. Both database test targets are included in PostgreSQL CI.

This removes the lifetime usage-log scans from these three paths. Date-range analytics and rollups are separate work. No production backfill or deployment was performed, and no Markdown plans are committed.

Validation: 25 selected PostgreSQL/API regressions passed (784 unrelated tests skipped), including all API-key tests, writer counters, backfill concurrency/rollback, billing summary, and scan-free reader tests. The existing billing-summary test now uses the established exclusive nextest override because its exact before/after platform totals race parallel organization creation; all assertions remain intact. Formatting passed.

cargo clippy -p database -p api --all-targets --all-features -- -D warnings passed.

[ironloopai]

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details

• Run: 82d09bbc-a662-4817-b283-85dd9d16ef98
• Base: codex/analytics-counter-backfill at 4cd053e
• Head: codex/analytics-counter-readers at eacfd85
• Created: 2026-09-22 17:59 UTC
• Updated: 2026-09-22 18:04 UTC

_{Automatic trigger · attempt 1 of 3 · completed in 4m 10s}

[ironloopai]

Review · Summary

🟢 No actionable findings

No actionable issues found in the reviewed change.

Validation

• ✅ Counter-reader consistency — Static inspection traced each changed reader to its transactional counter writer and readiness prerequisite; ordering, deletion filtering, and admission semantics remain preserved.
• ✅ Reader isolation coverage — The added database coverage exercises key listing, admission spend, and billing totals without raw usage tables present.

Review details

• Run: 82d09bbc-a662-4817-b283-85dd9d16ef98
• Attempts: 1

[claude]

Review — perf: replace lifetime spending scans with reconciled counters

Building on the earlier IronLoop review (no actionable findings). I re-verified the three reader rewrites and agree they are semantically equivalent to the queries they replace given a correct backfill:

• api_key.rs:307-312 — api_key_spend is 1:1 with api_keys (PK + FK), and organization_usage_log.api_key_id is NOT NULL REFERENCES api_keys(id), so dropping the per-subquery WHERE workspace_id = $1 scoping is safe; a key never moves workspace. Ordering alias, tie-breakers, deleted_at filter and unused-key zeroes are preserved.
• organization_usage.rs:60-80 — the scalar sub-select always returns exactly one row, so query_one stays safe for keys with no counter row; inference-only admission semantics preserved.
• analytics.rs:550-562 — single scan, same one-row shape; total_spent is only ever mutated by the usage writers (credit_allocation.rs touches unresolved_unfunded_amount only), so the legacy total stays consistent.
• pool() is the Patroni write pool, so the startup gate cannot be fooled by replica lag. Good.

Two production-safety issues, both at the deployment seam rather than in the SQL:

⚠️ 1. Orgs created during the writer rollout are marked ready-by-construction and can never be repaired

V0082 sets spend_counters_ready_at DEFAULT NOW(), and trigger_create_organization_balance (V0004) creates the balance row on org insert. The "ready by construction" claim only holds once every writer process runs #1116. Concrete failure:

1. The first new pod applies V0081/V0082; old pods (no increment_api_key_spend) keep serving until the rollout drains.
2. An org created in that window gets spend_counters_ready_at = NOW().
3. An old pod posts usage for it: organization_usage_log grows, api_key_spend / organization_balance.inference_spent do not.
4. PreparedSpendBackfill::prepare returns None for that org (spend_counters_backfill.rs:83), and --organization <id> cannot override it — there is no force/re-reconcile mode.

After this PR that under-count becomes enforcement-relevant: check_usage_for_api_key (middleware/usage.rs:83) reads get_api_key_spend, so a key with a spend_limit can overspend indefinitely and the key list under-reports usage — silently, with no drift signal. The same window exists between prepare() and apply() if an old writer posts in between.

Suggested: a --verify / --force mode on backfill-spend-counters that recomputes raw-vs-counted for already-ready orgs and reports drift, or at minimum a documented pre-deploy drift query gating this rollout. The help text already says "drain all older writers first" — the gap is that a violation is undetectable after the fact.

⚠️ 2. expect on the readiness gate turns one bad row into a fleet-wide data-plane outage

// crates/api/src/main.rs:30
database::ensure_spend_counters_ready(database.pool())
    .await
    .expect("Spend counters are incomplete; ...");

The check is global over organizations. A single unreconciled org — partially failed backfill, a restored or imported org, a balance row written with an explicit NULL — crash-loops every pod, taking down chat completions, responses and conversations for all tenants. The degradation actually at risk is three read paths (key list, key-level admission, admin billing summary), so failing the whole data plane closed is disproportionate. Consider failing only those paths, or an explicit env escape hatch, and logging the offending org count (IDs only) instead of a bare panic.

This also blocks cargo run --bin api against any pre-existing dev/staging database until the backfill binary is run — worth calling out in the deploy notes.

Nits (non-blocking)

• counter_readers.rs: drop_schema only runs on the success path, so any ? early-return leaks a counter_readers_<uuid> schema in the shared database. A guard, or a sweep of stale schemas at start, would keep local DBs clean.
• The e2e-db-bootstrap setup script filter is package(api) & binary(e2e_all), so cargo nextest run --test counter_readers on its own fails when TEST_DATABASE_NAME does not exist yet. Pre-existing for spend_counter_backfill; only matters for the single-target invocation.
• The admin_analytics::test_admin_platform_billing_summary exclusive override is orthogonal to the counter change (it serializes on paying_org_count deltas). Fine, just noting the extra scope.

⚠️ Issues found — the SQL itself is correct; the open items are the unrepairable-drift window and the blast radius of the startup gate.

[henrypark133]

Code Review (multi-agent)

Intent: Replace lifetime usage-log scans in API-key lists, admission checks, and billing summary with reconciled transactional spend counters for performance.

Stats: 3 findings (from 3 raw, 3 after filter, 3 after dedup) across 3 files. Reviewers run: correctness, security, performance, design, coverage. Reviewers failed: none. Body-only: 0.

Mechanical

1. High Production .expect() introduced on the new spend-counter readiness check (crates/api/src/main.rs:46-48, confidence 90) — anchor: crates/api/src/main.rs:44
   database::ensure_spend_counters_ready(...).await.expect(...) will panic and crash the process at startup if spend counters are incomplete. This mirrors the existing .expect("Usage reporting index prerequisites are not satisfied") pattern three lines above for the same fail-fast-at-startup purpose, so it is likely intentional rather than an oversight — flagged per the mechanical unwrap/expect check for author confirmation. If intended, no change needed; just confirm this only runs once at process start, not on any hot/retry path.

Design & Maintainability

1. Medium New test file re-implements Postgres connection/pool-config boilerplate already in spend_counter_backfill.rs (crates/database/tests/counter_readers.rs:12-41, confidence 72) — anchor: crates/database/tests/spend_counter_backfill.rs:415-470
   env_value/pool_config/new_pool reimplement the same env-var-driven connection config already in spend_counter_backfill.rs's pool_config/test_database, differing only in database-selection strategy (schema search_path vs. CREATE DATABASE + migrations). No shared tests/support module exists to extend into. Suggested fix: extract a shared helper parameterized by selection strategy.

Coverage & Verification

1. Medium New test file hand-rolls a subset schema instead of running real migrations, risking future silent drift from production schema (crates/database/tests/counter_readers.rs:345-403, confidence 72, verified) — anchor: crates/database/tests/spend_counter_backfill.rs:415-434
   scoped_pool() builds a 5-table schema subset via inline CREATE TABLE instead of database::migrations::run() like the sibling spend_counter_backfill.rs. Adversarial verification confirmed the hand-rolled schema already diverges from the real migrated schema for organizations/organization_limits_history (missing columns), though none of the currently-missing columns are read by the 3 repository methods under test — so today's tests pass correctly. The risk is forward-looking: a future migration adding a required column these tests' tables would not be caught here. Suggested fix: use database::migrations::run() scoped via search_path, mirroring the sibling file.

Also flagged by: design/Medium (same file, related root cause — no shared connection-config/migration harness between the two test files).

[github-actions]

🔍 OpenCodeReview found 6 issue(s) in this PR.

• ✅ 6 posted as inline comment(s)
• 📝 0 posted as summary

⚠️ 1 warning(s) occurred during review.

[henrypark133]

Review pass: reader tests reuse the existing support connection configuration, use the actual database name for standalone runs, and clean up their isolated schemas after test-body errors or panics. The missing-raw-table proof remains intact.

Rollout is strictly staged: deploy #1116 everywhere and drain all older writers before applying V0082 from #1119, then run backfill to completion, then deploy #1120. Deploying both migrations in the first writer rollout violates that prerequisite; readiness markers do not detect such a violation, and this PR does not provide an audit/force-repair command. Returning to a writer version that does not maintain counters likewise invalidates readiness and requires reconciliation before these readers can be used.

The global startup gate remains intentional and runs once before traffic. A partial backfill blocks this new reader deployment; keep the previous reader release serving until reconciliation is complete. Route-specific fallback or an environment bypass would create another spending-read mode and is not added in this review pass. This also applies to an existing dev/staging database.

Test-file changes: api_keys.rs expresses the second fixture request with a struct update and removes an unused clone without changing values/assertions. counter_readers.rs replaces duplicate env parsing with existing support configuration and wraps each unchanged reader assertion set in cleanup that also runs after task errors/panics. tests/support/mod.rs exposes that configuration and adds DATABASE_* fallbacks, rejects invalid ports, and selects DATABASE_NAME rather than an unbootstrapped test database. Full migrations are intentionally not added to the isolated no-raw-table tests. Setup failures before the test body can still leave a partially created schema.

Local aggregate validation (all four PRs plus review fixes):

cargo nextest run --test e2e_all --test spend_counter_backfill --test counter_readers --bin backfill-spend-counters --no-fail-fast
Summary: 819 passed, 0 failed, 12 existing skips

Formatting and diff checks passed. The final run used DATABASE_* configuration without PG* overrides. This local aggregate validation is not a production backfill or deployment.

GitHub CI verified on 47cdf1a1: Test Suite, lint, unit, integration, E2E, script tests, cargo audit, and cargo deny all succeeded. Build release was skipped by the workflow. No CI reruns were needed.

[lloydmak99]

Straightforward perf change: three lifetime-spending log scans are replaced by reads against the reconciled api_key_spend/organization_balance counters, guarded by a startup readiness gate. The rewrites are semantically equivalent to what they replace, and the writer paths maintain the counters, so admission and ordering semantics are preserved.

Non-blocking follow-up:

• crates/api/src/main.rs:30 — ensure_spend_counters_ready is a global startup gate that .expect()s, so it hard-requires the #1116→#1119 backfill to complete before this deploys; an unreconciled organization_balance row will crash-loop pods (and blocks cargo run --bin api against a pre-backfill dev/staging DB). Confirmed intentional staged-rollout design — just make sure the deploy runbook orders the backfill first.

Checks: cargo fmt --all -- --check passed; full workspace compile skipped locally (no cc linker / cold cache), CI reported green on the latest head.