Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .server-changes/additional-api-key-rate-limit-bucket.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

API rate limits now apply per environment, so creating extra API keys no longer increases how many requests an environment can make.
83 changes: 83 additions & 0 deletions apps/webapp/app/models/runtimeEnvironment.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,89 @@ export async function findEnvironmentByApiKeyWithResolution(
return resolveEnvironmentByApiKey(apiKey, branchName, tx, additionalApiKeyLookupEnabled);
}

export type PrivateApiKeyRateLimitScope = {
environmentId: string;
apiRateLimiterConfig: unknown;
};

export async function resolvePrivateApiKeyRateLimitScope(
apiKey: string,
tx: PrismaClientOrTransaction = $replica
): Promise<PrivateApiKeyRateLimitScope | null> {
const now = new Date();

if (isAdditionalApiKey(apiKey)) {
const match = await tx.apiKey.findFirst({
where: {
keyHash: hashApiKey(apiKey),
revokedAt: null,
OR: [{ expiresAt: null }, { expiresAt: { gt: now } }],
},
select: {
runtimeEnvironment: {
select: {
id: true,
project: { select: { deletedAt: true } },
organization: { select: { apiRateLimiterConfig: true } },
},
},
},
});
Comment thread
carderne marked this conversation as resolved.

if (!match?.runtimeEnvironment || match.runtimeEnvironment.project.deletedAt) {
return null;
}

return {
environmentId: match.runtimeEnvironment.id,
apiRateLimiterConfig: match.runtimeEnvironment.organization.apiRateLimiterConfig,
};
}

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

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.


if (environment) {
if (environment.project.deletedAt) {
return null;
}

return {
environmentId: environment.id,
apiRateLimiterConfig: environment.organization.apiRateLimiterConfig,
};
}

const revokedApiKey = await tx.revokedApiKey.findFirst({
where: { apiKey, expiresAt: { gt: now } },
select: {
runtimeEnvironment: {
select: {
id: true,
project: { select: { deletedAt: true } },
organization: { select: { apiRateLimiterConfig: true } },
},
},
},
});

const revokedEnvironment = revokedApiKey?.runtimeEnvironment;
if (!revokedEnvironment || revokedEnvironment.project.deletedAt) {
return null;
}

return {
environmentId: revokedEnvironment.id,
apiRateLimiterConfig: revokedEnvironment.organization.apiRateLimiterConfig,
};
}

/**
* @deprecated We don't use public API keys (`pk_*` tokens) anymore — public
* access goes through public JWTs (see `isPublicJWT` / `validatePublicJwtKey`).
Expand Down
21 changes: 6 additions & 15 deletions apps/webapp/app/presenters/v3/LimitsPresenter.server.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Ratelimit } from "@upstash/ratelimit";
import type { RuntimeEnvironmentType } from "@trigger.dev/database";
import { createHash } from "node:crypto";
import { env } from "~/env.server";
import { getCurrentPlan } from "~/services/platform.v3.server";
import {
Expand Down Expand Up @@ -90,13 +89,11 @@ export class LimitsPresenter extends BasePresenter {
projectId,
environmentId,
environmentType,
environmentApiKey,
}: {
organizationId: string;
projectId: string;
environmentId: string;
environmentType: RuntimeEnvironmentType;
environmentApiKey: string;
}): Promise<LimitsResult> {
// Get organization with all limit-related fields
const organization = await this._replica.organization.findFirstOrThrow({
Expand Down Expand Up @@ -168,10 +165,10 @@ export class LimitsPresenter extends BasePresenter {
where: { organizationId },
});

// Get current rate limit tokens for this environment's API key
// Get current rate limit tokens for this environment's API bucket
const apiRateLimitTokens = await getRateLimitRemainingTokens(
"api",
environmentApiKey,
environmentId,
apiRateLimitConfig
);
// Batch rate limiter uses environment ID directly (not hashed) with a different key prefix
Expand Down Expand Up @@ -454,20 +451,14 @@ function resolveBatchConcurrencyConfig(batchConcurrencyConfig?: unknown): {

/**
* Query the current remaining tokens for a rate limiter using the Upstash getRemaining method.
* This uses the same configuration and hashing logic as the rate limit middleware.
* The API limiter uses the environment ID as the bucket identifier for private API keys.
*/
async function getRateLimitRemainingTokens(
keyPrefix: string,
apiKey: string,
identifier: string,
config: RateLimiterConfig
): Promise<number | null> {
try {
// Hash the authorization header the same way the rate limiter does
const authorizationValue = `Bearer ${apiKey}`;
const hash = createHash("sha256");
hash.update(authorizationValue);
const hashedKey = hash.digest("hex");

// Create a Ratelimit instance with the same configuration
const limiter = createLimiterFromConfig(config);
const ratelimit = new Ratelimit({
Expand All @@ -478,9 +469,9 @@ async function getRateLimitRemainingTokens(
prefix: `ratelimit:${keyPrefix}`,
});

// Use the getRemaining method to get the current remaining tokens
// Use the same identifier as the API rate-limit middleware.
// getRemaining returns a Promise<number>
const remaining = await ratelimit.getRemaining(hashedKey);
const remaining = await ratelimit.getRemaining(identifier);
return remaining;
} catch (error) {
logger.warn("Failed to get rate limit remaining tokens", {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => {
presenter.call({
organizationId: project.organizationId,
projectId: project.id,
environmentId: environment.id,
// API traffic for a branch is bucketed on the parent environment id.
environmentId: environment.parentEnvironmentId ?? environment.id,
Comment on lines +85 to +86

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.

environmentType: environment.type,
environmentApiKey: environment.apiKey,
})
);

Expand Down
40 changes: 35 additions & 5 deletions apps/webapp/app/services/apiRateLimit.server.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { tryCatch } from "@trigger.dev/core/v3";
import { trail } from "agentcrumbs"; // @crumbs
import { env } from "~/env.server";
import { resolvePrivateApiKeyRateLimitScope } from "~/models/runtimeEnvironment.server";
import { batchStreamGrants } from "~/runEngine/concerns/batchStreamGrantsInstance.server";
import { authenticateAuthorizationHeader } from "./apiAuth.server";
import { authorizationRateLimitMiddleware } from "./authorizationRateLimitMiddleware.server";
import type { Duration } from "./rateLimiter.server";

const BATCH_STREAM_ITEMS_PATH = /^\/api\/v3\/batches\/([^/]+)\/items$/;
const crumb = trail("webapp"); // @crumbs

export const apiRateLimiter = authorizationRateLimitMiddleware({
redis: {
Expand All @@ -29,6 +32,27 @@ export const apiRateLimiter = authorizationRateLimitMiddleware({
maxItems: 1000,
},
limiterConfigOverride: async (authorizationValue) => {
const rawApiKey = authorizationValue.replace(/^Bearer /, "");

if (rawApiKey.startsWith("tr_")) {
const scope = await resolvePrivateApiKeyRateLimitScope(rawApiKey);

if (!scope) {
return;
}

// #region @crumbs
crumb("resolved private API key rate limit scope", {
environmentId: scope.environmentId,
});
// #endregion @crumbs

return {
config: scope.apiRateLimiterConfig,
identifier: scope.environmentId,
};
}

const authenticatedEnv = await authenticateAuthorizationHeader(authorizationValue, {
allowPublicKey: true,
allowJWT: true,
Expand All @@ -40,13 +64,19 @@ export const apiRateLimiter = authorizationRateLimitMiddleware({

if (authenticatedEnv.type === "PUBLIC_JWT") {
return {
type: "fixedWindow",
window: env.API_RATE_LIMIT_JWT_WINDOW,
tokens: env.API_RATE_LIMIT_JWT_TOKENS,
config: {
type: "fixedWindow",
window: env.API_RATE_LIMIT_JWT_WINDOW,
tokens: env.API_RATE_LIMIT_JWT_TOKENS,
},
};
} else {
return authenticatedEnv.environment.organization.apiRateLimiterConfig;
}

return {
config: authenticatedEnv.environment.organization.apiRateLimiterConfig,
// Public keys are browser-distributed, so keep them on per-key buckets.
identifier: authenticatedEnv.type === "PRIVATE" ? authenticatedEnv.environment.id : undefined,
};
Comment thread
carderne marked this conversation as resolved.
Comment thread
carderne marked this conversation as resolved.
},
pathMatchers: [/^\/api/],
// Allow /api/v1/tasks/:id/callback/:secret
Expand Down
64 changes: 50 additions & 14 deletions apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,14 @@ export const RateLimiterConfig = z.discriminatedUnion("type", [

export type RateLimiterConfig = z.infer<typeof RateLimiterConfig>;

type LimitConfigOverrideFunction = (authorizationValue: string) => Promise<unknown>;
type RateLimitOverride = {
config?: unknown;
identifier?: string;
};

type LimitConfigOverrideFunction = (
authorizationValue: string
) => Promise<RateLimitOverride | undefined>;

type Options = {
redis: RedisWithClusterOptions;
Expand Down Expand Up @@ -80,16 +87,22 @@ type Options = {
};
};

async function resolveLimitConfig(
type ResolvedRateLimit = {
config: RateLimiterConfig;
// Bucket key to use, or undefined to fall back to the hashed Authorization header.
identifier?: string;
};

async function resolveRateLimit(
authorizationValue: string,
hashedAuthorizationValue: string,
defaultLimiter: RateLimiterConfig,
cache: UnkeyCache<{ limiter: RateLimiterConfig }>,
cache: UnkeyCache<{ limiter: ResolvedRateLimit }>,
logsEnabled: boolean,
limiterConfigOverride?: LimitConfigOverrideFunction
): Promise<RateLimiterConfig> {
): Promise<ResolvedRateLimit> {
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
if (!limiterConfigOverride) {
return defaultLimiter;
return { config: defaultLimiter };
}

if (logsEnabled) {
Expand All @@ -110,18 +123,24 @@ async function resolveLimitConfig(
});
}

return defaultLimiter;
return { config: defaultLimiter } satisfies ResolvedRateLimit;
}

const parsedOverride = RateLimiterConfig.safeParse(override);
const identifier = override.identifier;

if (!override.config) {
return { config: defaultLimiter, identifier } satisfies ResolvedRateLimit;
}

const parsedOverride = RateLimiterConfig.safeParse(override.config);

if (!parsedOverride.success) {
logger.error("Error parsing rate limiter override", {
override,
errors: parsedOverride.error.errors,
});

return defaultLimiter;
return { config: defaultLimiter, identifier } satisfies ResolvedRateLimit;
}

if (logsEnabled && parsedOverride.data) {
Expand All @@ -132,10 +151,22 @@ async function resolveLimitConfig(
});
}

return parsedOverride.data;
return { config: parsedOverride.data, identifier } satisfies ResolvedRateLimit;
});

return cacheResult.val ?? defaultLimiter;
// Defensive read: the cache is keyed on a shared Redis namespace, so during a
// deploy an entry could have been written by a server running a different
// code version (a different stored shape). Re-validate here so a stale/foreign
// entry can never reach createLimiterFromConfig with an undefined config and
// throw. The cache key is also versioned (see RedisCacheStore keyPrefix), so
// this is belt-and-suspenders.
const cached = cacheResult.val;
const parsedConfig = RateLimiterConfig.safeParse(cached?.config);

return {
config: parsedConfig.success ? parsedConfig.data : defaultLimiter,
identifier: typeof cached?.identifier === "string" ? cached.identifier : undefined,
};
}

/**
Expand Down Expand Up @@ -169,14 +200,17 @@ export function authorizationRateLimitMiddleware({
const memory = createLRUMemoryStore(limiterCache?.maxItems ?? 1000);
const redisCacheStore = new RedisCacheStore({
connection: {
keyPrefix: `cache:${keyPrefix}:rate-limit-cache:`,
// Versioned namespace: the cached value shape is part of this key. Bump
// the version whenever ResolvedRateLimit changes so a rolling deploy never
// reads entries written in a previous shape (and vice versa).
keyPrefix: `cache:${keyPrefix}:rate-limit-cache:v2:`,
Comment thread
carderne marked this conversation as resolved.
...redis,
},
});

// This cache holds the rate limit configuration for each org, so we don't have to fetch it every request
const cache = createCache({
limiter: new Namespace<RateLimiterConfig>(ctx, {
limiter: new Namespace<ResolvedRateLimit>(ctx, {
stores: [memory, redisCacheStore],
fresh: limiterCache?.fresh ?? 30_000,
stale: limiterCache?.stale ?? 60_000,
Expand Down Expand Up @@ -269,7 +303,7 @@ export function authorizationRateLimitMiddleware({
hash.update(authorizationValue);
const hashedAuthorizationValue = hash.digest("hex");

const limiterConfig = await resolveLimitConfig(
const { config: limiterConfig, identifier } = await resolveRateLimit(
authorizationValue,
hashedAuthorizationValue,
defaultLimiter,
Expand All @@ -278,6 +312,8 @@ export function authorizationRateLimitMiddleware({
limiterConfigOverride
);

const rateLimitIdentifier = identifier ?? hashedAuthorizationValue;

const limiter = createLimiterFromConfig(limiterConfig);

const rateLimiter = new RateLimiter({
Expand All @@ -288,7 +324,7 @@ export function authorizationRateLimitMiddleware({
logFailure: log.rejections,
});

const { success, limit, reset, remaining } = await rateLimiter.limit(hashedAuthorizationValue);
const { success, limit, reset, remaining } = await rateLimiter.limit(rateLimitIdentifier);

const $remaining = Math.max(0, remaining); // remaining can be negative if the user has exceeded the limit, so clamp it to 0

Expand Down
Loading
Loading