feat(quota): enforce aggregate user limits - #263
Conversation
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Reviewed the complete current diff at head cde69dd. Local full Go tests and go vet, frontend lint/typecheck/tests/build, all 122 repository tests, translation/UI/environment/source-line gates, and git diff --check passed. The required Backend and aggregate CI checks are still failing because four newly added helper functions are unused. I also reproduced one blocking attribution-continuity issue inline, so this is a comment-only review.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete diff at a20bd517. All current GitHub checks are green, and local backend tests/vet, frontend lint/typecheck/tests/build, 125 repository tests, explicit translation/UI/environment/source-line gates, and git diff --check pass. The prior attribution finding is only partially fixed (I replied on that thread with a failing same-period transfer reproduction), and the following additional issues still block approval.
[P1] Make image admission atomic with durable image-job persistence (backend/internal/server/image_generation.go:95-109,241-255, backend/internal/server/store_routing_calls.go:450-462). startImageCall commits quota reservations and concurrency leases before CreateImageJob runs in a separate transaction. A process crash between those calls leaves no image-job row for FailUnfinishedImageJobs to discover, so request/token reservations remain charged and leases have no durable owner to recover. Persist the image job and its admission in one transaction, as the background Responses path does, and add a crash/restart regression.
[P1] Backfill existing current-period quota buckets safely (backend/internal/server/store_backup_quota.go:395-425,479-486). AutoMigrate adds attributed_user_id, but existing rows start blank. The first post-upgrade request stamps the whole pre-existing day/month row with the current owner, silently assigning all earlier requests, tokens, and cost to that user; untouched rows are ignored by user aggregation. Reconstruct attribution from request-time records or define an explicit conservative rollout path, with upgrade coverage for ownership changes.
[P2] Correct the PR's database and rollback impact. The Compatibility section says no database migrations are required, but this head adds persisted columns to quota_buckets, image_jobs, and response_jobs, and changes key/project deletion to retain attributed quota history (backend/internal/server/store.go:18-23, backend/internal/server/types.go:518-588, backend/internal/server/store_projects.go:187-196,684-696). Document the automatic schema changes, retained user-attribution data, upgrade/backfill behavior, and rollback compatibility.
[P2] Document team-leader authorization in all three administrator guides (docs/administrator-guide.md:73 and locale counterparts). The docs say platform administrators configure user quotas, while the frontend exposes quota policies to team leaders and the backend lets them manage policies for users in their own team (frontend/features/admin/core/navigation.tsx:210-216, backend/internal/server/admin_authorization.go:1034-1108). Describe that scoped capability and its restrictions consistently, or remove the access.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Found one remaining startup blocker in the PostgreSQL upgrade path:
[P1] Add the new column before the PostgreSQL backfill (backend/internal/server/store_database.go:378-380). migrate() invokes ensureQuotaBucketAttributionSchema before db.AutoMigrate (:214-232). On a database created by the base revision, quota_buckets has only the original (key_id, scope, bucket) columns, so this branch executes UPDATE quota_buckets SET attributed_user_id ... against a column that does not exist and aborts startup before AutoMigrate can add it. The SQLite branch handles the missing column explicitly, but the PostgreSQL branch does not. Please add/guard ALTER TABLE quota_buckets ADD COLUMN attributed_user_id ... (and make the backfill/PK replacement safe for both legacy and partially upgraded schemas), plus a PostgreSQL migration regression test.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
The original legacy-PostgreSQL startup blocker is fixed at c34adddb, but two issues still block approval:
[P1] Run the new PostgreSQL regressions in CI (.github/workflows/ci.yml:100-103). Both TestQuotaBucketMigrationPostgresAddsAttributionColumnBeforeBackfill and TestUserQuotaIsAtomicAcrossPostgresInstances are behind the integration build tag, while the PostgreSQL job filters execution to exactly ^TestMultiInstancePostgresE2E$; default go test ./... excludes them as well. The current green checks therefore execute neither the migration regression added by this commit nor the PR's direct cross-instance user-quota test. Expand the filter or add an explicit invocation so these tests actually run on every PR.
[P2] Scope the PostgreSQL primary-key metadata query to the active schema (backend/internal/server/store_database.go:390). It currently selects every primary key named on a quota_buckets table across all schemas. In a database containing public.quota_buckets plus a TokenHub schema (the new migration test creates exactly this shape), pkColumns combines both schemas, len(pkColumns) is never 4, and every startup unnecessarily drops and recreates the active table's primary key under an exclusive lock. Add tc.table_schema = current_schema() (or equivalent) and make the second migration assertion verify that the PK was not rebuilt.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head d636b4dd. No remaining code, standards, or spec findings were found. All previously reported blockers are fixed, and exact-head CI is fully green, including the legacy PostgreSQL quota-bucket migration, primary-key idempotence, and cross-instance aggregate user-quota tests. Focused local quota/image/migration tests, go vet ./..., all 125 repository tests, explicit translation/UI/environment/source-line gates, and git diff --check pass. GitHub rejected the attempted APPROVE review because the authenticated astaxie account is also this PR's author; a non-author reviewer must approve if approval is required. Not merged.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 4739b3e. One new quota-settlement blocker remains.
[P1] finishCallTransaction wraps minute-token reconciliation, attributed key usage, and all user day/month reservation settlement inside the successful lookup of the live API key row at store_routing_calls.go:655-743. If an administrator deletes a key after StartCall admits a request but before FinishCall runs, the request and usage logs are still written, but the user reservation is never reconciled to actual tokens and the attributed quota buckets are not updated. This can under-enforce when actual usage exceeds the reservation or over-enforce when the reservation exceeds actual usage, breaking continuity across key deletion and rotation.
Please settle the immutable CallContext quota data even when the key row no longer exists, keep only key-dependent alerting or model data conditional, and add a regression that deletes the key between admission and completion.
All GitHub checks are green. The authenticated astaxie account is also the PR author, so GitHub cannot accept an APPROVE review from this account.
Summary
Adds aggregate user-scoped quota policies so one user's request, token, cost, rate, and concurrency limits apply across all API keys attributed to that user. This closes the per-key bypass caused by multiple keys and key rotation, while preserving ownership history when a key changes owner within one billing period.
Related Issue
Closes #259
Changes
scope: "user"quota policies with API-key owner, key creator, and project owner attribution fallback.Type of Change
Verification
gofmt -won changed Go filesgo test ./...go vet ./...npm run lintnpm run typechecknpm testnpm run buildnpm run test:e2enode --test tools/*.test.mjsnode tools/check-doc-translations.mjs --base origin/main --head HEADnode tools/check-ui-translations.mjsnode tools/check-env-contract.mjsnode tools/check-source-lines.mjsgit diff --checkCompatibility, Security, and Operations
The existing OpenAI-compatible
/v1contract remains unchanged; user-scoped policies are additive and existing scopes retain their strictest-limit behavior. This change updates the frozen database adoption baseline:quota_bucketsnow uses(key_id, scope, bucket, attributed_user_id)as its primary key, legacy rows are retained under an explicit unattributed marker rather than guessed ownership, and image/response admission recovery fields are persistent. Pre-ledger databases are upgraded under the shared schema lock before verification. The attribution column defaults to the canonical unattributed marker so verified N-1 releases can continue writing during the rollback window; operators should still preserve the database and take the usual backup before rollout.Image admission and job creation now commit together, so a crash cannot leave a reservation or concurrency lease without a recoverable job. User metrics and quota errors expose only bounded scope labels, never raw user IDs or secrets. Team leaders can configure only user policies within their own teams; platform administrators retain global access. Disabling or deleting a policy provides an application-level rollback without changing stored API keys.
Checklist
.envfiles, databases, backups, or runtime logs are included.start.sh, and deployment documentation where applicable.data/model-catalog.yamlremains tracked and catalog changes were reviewed where applicable.git diff --checkpasses.