Skip to content

feat(webapp): share rate limit bucket across additional API keys per environment - #4508

Open
carderne wants to merge 10 commits into
feat/multi-keys-surfacefrom
feature/tri-12888-add-redis-cache-for-additional-api-key-environment-lookup
Open

feat(webapp): share rate limit bucket across additional API keys per environment#4508
carderne wants to merge 10 commits into
feat/multi-keys-surfacefrom
feature/tri-12888-add-redis-cache-for-additional-api-key-environment-lookup

Conversation

@carderne

@carderne carderne commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

What

Rate-limit the API by environment rather than per API key.

Previously the limiter keyed its bucket on the hash of the full Authorization header — 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

  • authorizationRateLimitMiddleware now lets the override return { config?, identifier? }. identifier, when present, is the rate limit bucket key; otherwise it falls back to the hashed Authorization header (unchanged legacy behavior, still used by engineRateLimiter and any unauthenticated fallthrough).
  • apiRateLimiter's override resolves the environment id and uses it as the identifier:
    • Additional keys (isAdditionalApiKey) resolve via a new resolveAdditionalApiKeyRateLimitScope() — 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.
    • Root/legacy keys reuse the environment already resolved by authenticateAuthorizationHeader and key on environment.id too.
  • The identifier is always the stable environment id, never the secret key (which can rotate and would split the bucket).
  • The whole override result is cached per key by the existing SWR cache, so no extra per-request lookup and no separate Redis mapping is added.

Behavior notes

  • Root + additional keys of the same environment now share one bucket (ceiling = configured limit, not a multiple of it). Restricted additional keys are included — they were the biggest gap, since they authenticate via the RBAC controller and previously fell back to per-key buckets.
  • Public JWTs keep their existing fixed-window, per-token bucketing.
  • One-time bucket reset on deploy (bucket keys change); harmless.

Tests

  • New: two tokens resolving to the same identifier share one bucket.
  • New: with no identifier, bucketing stays per-key (legacy behavior preserved).
  • Updated existing override tests to the new { config } return shape.

Base: feat/multi-keys-surface. Closes TRI-12888.

@changeset-bot

changeset-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: eb0e8f7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b8f51634-85c3-4560-92e7-4fcd28cf47a9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

Comment @coderabbitai help to get the list of available commands.

@carderne
carderne marked this pull request as ready for review August 5, 2026 09:20
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@carderne
carderne force-pushed the feature/tri-12888-add-redis-cache-for-additional-api-key-environment-lookup branch from ba817b9 to 316db63 Compare August 5, 2026 17:08
@pkg-pr-new

pkg-pr-new Bot commented Aug 5, 2026

Copy link
Copy Markdown

Open in StackBlitz

@trigger.dev/build

npm i https://pkg.pr.new/@trigger.dev/build@316db63

trigger.dev

npm i https://pkg.pr.new/trigger.dev@316db63

@trigger.dev/core

npm i https://pkg.pr.new/@trigger.dev/core@316db63

@trigger.dev/python

npm i https://pkg.pr.new/@trigger.dev/python@316db63

@trigger.dev/react-hooks

npm i https://pkg.pr.new/@trigger.dev/react-hooks@316db63

@trigger.dev/redis-worker

npm i https://pkg.pr.new/@trigger.dev/redis-worker@316db63

@trigger.dev/rsc

npm i https://pkg.pr.new/@trigger.dev/rsc@316db63

@trigger.dev/schema-to-json

npm i https://pkg.pr.new/@trigger.dev/schema-to-json@316db63

@trigger.dev/sdk

npm i https://pkg.pr.new/@trigger.dev/sdk@316db63

commit: 316db63

devin-ai-integration[bot]

This comment was marked as resolved.

@carderne
carderne force-pushed the feature/tri-12888-add-redis-cache-for-additional-api-key-environment-lookup branch from 316db63 to cd2a0c0 Compare August 5, 2026 17:14
…rojectParam.env.$envParam.limits/route.tsx

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Open in Devin Review

Comment on lines +85 to +86
// API traffic for a branch is bucketed on the parent environment id.
environmentId: environment.parentEnvironmentId ?? environment.id,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +343 to +350
const environment = await tx.runtimeEnvironment.findFirst({
where: { apiKey },
select: {
id: true,
project: { select: { deletedAt: true } },
organization: { select: { apiRateLimiterConfig: true } },
},
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant