feat(webapp): share rate limit bucket across additional API keys per environment - #4508
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ba817b9 to
316db63
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
Use environment identifiers when displaying remaining API capacity and ignore additional keys tied to deleted projects.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
316db63 to
cd2a0c0
Compare
…rojectParam.env.$envParam.limits/route.tsx Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| // API traffic for a branch is bucketed on the parent environment id. | ||
| environmentId: environment.parentEnvironmentId ?? environment.id, |
There was a problem hiding this comment.
🟡 Limits page shows another environment's batch limit and queue numbers when viewing a branch
The limits page now looks up all of an environment's numbers under the parent environment (environment.parentEnvironmentId ?? environment.id at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.limits/route.tsx:86) even though only the API bucket moved to the parent, so a branch's batch and queue figures come from the wrong place.
Impact: When viewing a preview branch, the batch rate limit remaining and current queue size shown are those of the parent environment (usually zero/unused) instead of the branch the user selected.
Why the single environmentId argument is now overloaded
LimitsPresenter.call uses the one environmentId parameter for three different things: the API bucket identifier (apps/webapp/app/presenters/v3/LimitsPresenter.server.ts:169-172), the batch rate limit bucket (apps/webapp/app/presenters/v3/LimitsPresenter.server.ts:175-178) and the queue-size lookup (apps/webapp/app/presenters/v3/LimitsPresenter.server.ts:182-203).
Only the API bucket is keyed on the parent environment id (the middleware resolves the identifier from the root API key, which belongs to the parent — see resolvePrivateApiKeyRateLimitScope in apps/webapp/app/models/runtimeEnvironment.server.ts:309-385). The batch limiter keys on the authenticated environment id, which for a branch request is the child env (apps/webapp/app/runEngine/services/createBatch.server.ts:90), and engine.lengthOfEnvQueue is also per concrete environment.
So passing the parent id makes the batch tokens and queue size (and the concurrency fields read for the engine query) resolve against the parent instead of the branch.
Prompt for agents
LimitsPresenter.call takes a single environmentId that is used for three distinct lookups: the API rate limit bucket identifier, the batch rate limit bucket (keyed on the authenticated/child environment id in createBatch.server.ts) and the queue size / runtimeEnvironment row for engine.lengthOfEnvQueue. The limits route now passes environment.parentEnvironmentId ?? environment.id, which is correct only for the API bucket; for preview/dev branch environments the batch tokens and queue size are now computed for the parent environment instead of the selected branch. Consider giving the presenter two inputs (e.g. environmentId for the concrete environment and apiRateLimitIdentifier / bucketEnvironmentId for the parent-derived API bucket key) and wiring the route to pass both.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const environment = await tx.runtimeEnvironment.findFirst({ | ||
| where: { apiKey }, | ||
| select: { | ||
| id: true, | ||
| project: { select: { deletedAt: true } }, | ||
| organization: { select: { apiRateLimiterConfig: true } }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔍 Preview/dev branch keys resolve to different buckets than the parent root key
The identifier is resolved purely from the raw key: the root path does runtimeEnvironment.findFirst({ where: { apiKey } }). Branch environments have their own apiKey column value, so if a branch key is ever presented directly it resolves to the child environment id, while the normal flow (parent key + x-trigger-branch) resolves to the parent id — two buckets for the same logical environment. Similarly, an additional API key created against a branch environment (runtimeEnvironment relation on apiKey) buckets on the child id, not the parent. Worth confirming that additional keys can only be minted against parent/branchable environments, otherwise the per-environment ceiling can still be multiplied by creating branch-scoped keys.
Was this helpful? React with 👍 or 👎 to provide feedback.
What
Rate-limit the API by environment rather than per API key.
Previously the limiter keyed its bucket on the hash of the full
Authorizationheader — one bucket per key. With additional environment API keys (tr_*_sk_*), an environment can mint many keys and each got its own full bucket, so more keys = higher effective rate limit. This collapses all of an environment's keys onto a single shared per-environment bucket, so the ceiling is exactly the configured limit regardless of key mix.How
authorizationRateLimitMiddlewarenow lets the override return{ config?, identifier? }.identifier, when present, is the rate limit bucket key; otherwise it falls back to the hashedAuthorizationheader (unchanged legacy behavior, still used byengineRateLimiterand any unauthenticated fallthrough).apiRateLimiter's override resolves the environment id and uses it as the identifier:isAdditionalApiKey) resolve via a newresolveAdditionalApiKeyRateLimitScope()— a scope-agnostic keyHash → (environmentId, org limiter config) lookup. It is deliberately permissive (restricted keys resolve too) because it's used only for bucketing, never as an auth decision — request auth still goes through the RBAC bearer controller, which enforces scopes. Revoked/expired keys are excluded so they can't hold a bucket warm.authenticateAuthorizationHeaderand key onenvironment.idtoo.Behavior notes
Tests
{ config }return shape.Base:
feat/multi-keys-surface. Closes TRI-12888.