Skip to content

Commit 7814885

Browse files
committed
fix(webapp): keep a throwing bypass from failing the request
The batch item bypass re-authenticates the caller, so a transient failure there threw out of the middleware and returned a server error instead of falling back to normal rate limiting. The bypass now catches its own failures, and the middleware treats a throwing bypass as "no bypass" rather than trusting callers to honour the contract. Redis options are now required on the middleware, dropping an unused environment fallback so the module no longer pulls env into test import graphs.
1 parent 49f0d3c commit 7814885

3 files changed

Lines changed: 37 additions & 27 deletions

File tree

apps/webapp/app/services/apiRateLimit.server.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { tryCatch } from "@trigger.dev/core/v3";
12
import { env } from "~/env.server";
23
import { batchStreamGrants } from "~/runEngine/concerns/batchStreamGrantsInstance.server";
34
import { authenticateAuthorizationHeader } from "./apiAuth.server";
@@ -92,12 +93,14 @@ export const apiRateLimiter = authorizationRateLimitMiddleware({
9293
return false;
9394
}
9495

95-
const authenticated = await authenticateAuthorizationHeader(authorizationValue, {
96-
allowPublicKey: true,
97-
allowJWT: true,
98-
});
96+
const [authError, authenticated] = await tryCatch(
97+
authenticateAuthorizationHeader(authorizationValue, {
98+
allowPublicKey: true,
99+
allowJWT: true,
100+
})
101+
);
99102

100-
if (!authenticated || !authenticated.ok) {
103+
if (authError || !authenticated || !authenticated.ok) {
101104
return false;
102105
}
103106

apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { Ratelimit } from "@upstash/ratelimit";
55
import type { Request as ExpressRequest, Response as ExpressResponse, NextFunction } from "express";
66
import { createHash } from "node:crypto";
77
import { z } from "zod";
8-
import { env } from "~/env.server";
98
import type { RedisWithClusterOptions } from "~/redis.server";
109
import { logger } from "./logger.server";
1110
import type { Duration, Limiter } from "./rateLimiter.server";
@@ -56,7 +55,7 @@ export type RateLimiterConfig = z.infer<typeof RateLimiterConfig>;
5655
type LimitConfigOverrideFunction = (authorizationValue: string) => Promise<unknown>;
5756

5857
type Options = {
59-
redis?: RedisWithClusterOptions;
58+
redis: RedisWithClusterOptions;
6059
keyPrefix: string;
6160
pathMatchers: (RegExp | string)[];
6261
pathWhiteList?: (RegExp | string)[];
@@ -184,16 +183,7 @@ export function authorizationRateLimitMiddleware({
184183
}),
185184
});
186185

187-
const redisClient = createRedisRateLimitClient(
188-
redis ?? {
189-
port: env.RATE_LIMIT_REDIS_PORT,
190-
host: env.RATE_LIMIT_REDIS_HOST,
191-
username: env.RATE_LIMIT_REDIS_USERNAME,
192-
password: env.RATE_LIMIT_REDIS_PASSWORD,
193-
tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true",
194-
clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",
195-
}
196-
);
186+
const redisClient = createRedisRateLimitClient(redis);
197187

198188
return async (req: ExpressRequest, res: ExpressResponse, next: NextFunction) => {
199189
if (log.requests) {
@@ -255,11 +245,24 @@ export function authorizationRateLimitMiddleware({
255245
);
256246
}
257247

258-
if (bypass && (await bypass(req))) {
259-
if (log.requests) {
260-
logger.info(`RateLimiter (${keyPrefix}): bypassed ${req.path}`);
248+
if (bypass) {
249+
let bypassed = false;
250+
251+
try {
252+
bypassed = await bypass(req);
253+
} catch (error) {
254+
logger.warn(`RateLimiter (${keyPrefix}): bypass threw, applying the limit`, {
255+
path: req.path,
256+
error: error instanceof Error ? error.message : String(error),
257+
});
258+
}
259+
260+
if (bypassed) {
261+
if (log.requests) {
262+
logger.info(`RateLimiter (${keyPrefix}): bypassed ${req.path}`);
263+
}
264+
return next();
261265
}
262-
return next();
263266
}
264267

265268
const hash = createHash("sha256");

internal-packages/run-engine/src/engine/tests/batchTwoPhase.test.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -712,12 +712,16 @@ describe("RunEngine 2-Phase Batch API", () => {
712712
prisma
713713
);
714714

715-
await setTimeout(500);
716-
const dequeued = await engine.dequeueFromWorkerQueue({
717-
consumerId: "test_12345",
718-
workerQueue: "main",
719-
});
720-
expect(dequeued.length).toBe(1);
715+
await vi.waitFor(
716+
async () => {
717+
const dequeued = await engine.dequeueFromWorkerQueue({
718+
consumerId: "test_12345",
719+
workerQueue: "main",
720+
});
721+
expect(dequeued.length).toBe(1);
722+
},
723+
{ timeout: 15_000, interval: 100 }
724+
);
721725

722726
const initialExecutionData = await engine.getRunExecutionData({ runId: parentRun.id });
723727
assertNonNullable(initialExecutionData);

0 commit comments

Comments
 (0)