Skip to content

Commit c72ebf9

Browse files
authored
fix(webapp,run-engine): stop batchTriggerAndWait hanging when item streaming never completes (#4397)
## Summary `batchTriggerAndWait()` could leave a parent run waiting forever. The 2-phase batch API blocks the parent on the batch's waitpoint as soon as the batch is created, but the batch is only sealed at the end of item streaming. If streaming never completed, nothing sealed the batch, nothing completed the waitpoint, and the parent stayed suspended with no timeout and no way to recover. Supersedes #4016, which added the reaper alone. ## Fix Admission for item streaming was being decided twice. Batch creation passes its own rate limiter, which fixes `expectedCount` and blocks the parent, and then the item stream had to pass the general API limiter as well, competing with unrelated traffic. A second limiter could therefore veto work the first had already committed the parent to. Creation now mints a bounded grant that the item stream spends, so an admitted batch can finish streaming. The grant is capped per batch rather than exempting the path, and every failure mode (no grant, spent grant, unreachable store) falls back to the normal limiter. That makes stranding much rarer but not impossible, since a request timeout or a crash can still end streaming for good. So a seal-timeout reaper aborts any batch still unsealed after `BATCH_SEAL_TIMEOUT_MS` and completes the parent's waitpoint with an error, letting `batchTriggerAndWait()` reject instead of hang. It is race-safe against a late seal, and it is only scheduled for batches that actually block a parent, so fire-and-forget batches cost nothing. Finally, the batches page used to report "Batch completion checked." for these batches while doing nothing, because the completion path returns early on an unsealed batch. It now says the batch cannot be resumed. Rate limiting is no longer the reason a batch strands, so the reaper's default stays at 30 minutes, comfortably above the SDK's worst-case stream-retry budget. ## Verification Unit and container tests cover the grant cap, the bypass ordering (it runs after the authorization check, so it can never skip authentication), and the reaper's abort, seal race, idempotency, and no-waitpoint cases. Also verified end-to-end against a running stack. With the general limit exhausted, batch creation and other API calls returned 429 while a granted batch still streamed and sealed; an ungranted batch id was rate limited rather than bypassed; and the grant cut off exactly at its configured attempt count. Reproducing the stranded state on a real parent run, the batch was aborted at the timeout, the waitpoint completed with an error, and the parent resumed and finished instead of hanging. A parentless batch left unsealed was untouched well past the reaper window. ## Verified against deployed runs The reaper was proven end to end with a real deployed run (locally-run supervisor, containerised run) and a real network fault, rather than a simulated one: toxiproxy severs the phase 2 item stream mid-flight so every SDK stream retry genuinely fails, while phase 1 still succeeds. Only the batch calls traverse the fault, so control-plane traffic is untouched. The reproduction is the shape that actually strands a parent: the task catches the `BatchTriggerError` the SDK throws and carries on, so the phase 1 block outlives the thrown error and the parent hangs at its next suspension point. With the reaper disabled, the parent sat in `EXECUTING_WITH_WAITPOINTS` for over 24 minutes holding two blockers, and stayed stuck across a full infrastructure restart: ``` type | status | has_timeout BATCH | PENDING | f <- orphan, completedAfter NULL DATETIME | COMPLETED | t <- the wait already elapsed ``` With the reaper enabled the same task under the same fault completed in about 75 seconds with zero blockers left, the batch `ABORTED`, and its waitpoint completed carrying the error. Two conditions are required to observe this at all, which is worth knowing for any future test: the run must be deployed rather than `trigger dev` (dev runs execute in process and finish while still holding blocker rows), and the wait after the caught error must exceed the checkpoint threshold, or it is served in process and never suspends. ### Why completing the batch waitpoint is sufficient `batchTriggerAndWait` runs create, then stream, then wait. A phase 2 failure throws before the wait is ever reached, and the reaper only fires on an unsealed batch, so the parent is never suspended awaiting the batch when it runs. The parent therefore does not need a synthetic result, only to stop being blocked. Note this reasoning depends on that ordering: if the wait were ever reached with an unsealed batch, completing the batch waitpoint alone would not settle the caller. ## Follow-ups - Batches stranded before this ships still need a one-off recovery; the reaper only schedules at creation time. - That same property leaves a gap if the process dies between creating the batch and scheduling the job. A periodic sweep would close it, but wants a supporting index. - When a partially streamed batch aborts, children already enqueued keep running while the parent fails. Left as-is deliberately, since cancelling triggered work is a bigger semantic call.
1 parent 17d849b commit c72ebf9

17 files changed

Lines changed: 934 additions & 33 deletions
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Batch triggers no longer fail to start their runs when an environment is under heavy API load. If a batch still can't finish being created, `batchTriggerAndWait` now fails with an error instead of leaving the parent run waiting forever, and the batches page says so rather than reporting that it resumed.

apps/webapp/app/env.server.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,20 @@ const EnvironmentSchema = z
884884
BATCH_RATE_LIMIT_MAX: z.coerce.number().int().default(1200),
885885
BATCH_RATE_LIMIT_REFILL_INTERVAL: z.string().default("10s"),
886886
BATCH_CONCURRENCY_LIMIT_DEFAULT: z.coerce.number().int().default(5),
887+
/**
888+
* How long a created batch may remain unsealed before the seal-timeout reaper
889+
* aborts it and resumes any blocked parent with an error. Must exceed the SDK's
890+
* worst-case stream-retry budget (maxAttempts x server request timeout).
891+
* Doubles as the TTL of the phase 2 streaming grant, so the grant and the reaper
892+
* always agree on how long a batch is allowed to be sealing.
893+
*/
894+
BATCH_SEAL_TIMEOUT_MS: z.coerce.number().int().positive().default(1_800_000),
895+
/**
896+
* Number of phase 2 (`POST /api/v3/batches/:id/items`) requests a created batch is
897+
* granted, exempt from the general API rate limit. Sized above the SDK's stream
898+
* maxAttempts so a batch admitted by the batch limiter can always finish streaming.
899+
*/
900+
BATCH_STREAM_GRANT_ATTEMPTS: z.coerce.number().int().positive().default(10),
887901

888902
REALTIME_STREAM_VERSION: z.enum(["v1", "v2"]).default("v1"),
889903
REALTIME_STREAM_MAX_LENGTH: z.coerce.number().int().default(1000),

apps/webapp/app/routes/resources.batches.$batchId.check-completion.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,20 @@ export const action: ActionFunction = async ({ request, params }) => {
4242
return redirectWithErrorMessage(safeRedirectUrl, request, "Batch not found");
4343
}
4444

45+
const batch = await runStore.findBatchTaskRunById(ownedBatchRunId);
46+
47+
if (!batch) {
48+
return redirectWithErrorMessage(safeRedirectUrl, request, "Batch not found");
49+
}
50+
51+
if (!batch.sealed) {
52+
return redirectWithErrorMessage(
53+
safeRedirectUrl,
54+
request,
55+
"This batch was never finished being created, so it can't be resumed. Please get in touch and we'll recover it for you."
56+
);
57+
}
58+
4559
try {
4660
// v3 (engine V1) is retired; finalize the batch through the v2 completion path (no-op if not ready).
4761
await tryCompleteBatchV3(ownedBatchRunId, prisma, true);
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import { createRedisClient, type RedisClient, type RedisWithClusterOptions } from "~/redis.server";
2+
import { logger } from "~/services/logger.server";
3+
4+
export type BatchStreamGrantsOptions = {
5+
redis: RedisWithClusterOptions;
6+
/** How many phase 2 requests a created batch is allowed. */
7+
attempts: number;
8+
/** How long the grant survives, matching how long a batch may legitimately be sealing. */
9+
ttlMs: number;
10+
};
11+
12+
const KEY_PREFIX = "batch-stream-grant:";
13+
14+
/**
15+
* Admission for phase 2 of the 2-phase batch API.
16+
*
17+
* Phase 1 (`POST /api/v3/batches`) already passes its own batch rate limiter, which fixes
18+
* the batch's `expectedCount` and blocks the parent run on the batch's waitpoint. Phase 2
19+
* (`POST /api/v3/batches/:id/items`) is the only thing that can seal that batch, so having
20+
* the general API limiter reject it strands the batch and the parent with it.
21+
*
22+
* Phase 1 therefore mints a bounded grant, and phase 2 spends it to bypass the general
23+
* limiter. Admission stays a single decision made in phase 1, but the bypass is capped at
24+
* `attempts` requests per batch rather than being unconditional.
25+
*/
26+
export class BatchStreamGrants {
27+
private readonly redis: RedisClient;
28+
29+
constructor(private readonly options: BatchStreamGrantsOptions) {
30+
this.redis = createRedisClient("batchStreamGrants", options.redis);
31+
this.#registerCommands();
32+
}
33+
34+
/**
35+
* Grant a newly created batch its phase 2 budget. Never throws: a batch that fails to get
36+
* a grant still works, it just falls back to the general rate limiter for streaming.
37+
*/
38+
async mint(environmentId: string, batchId: string): Promise<void> {
39+
try {
40+
await this.redis.set(
41+
this.#key(environmentId, batchId),
42+
this.options.attempts,
43+
"PX",
44+
this.options.ttlMs
45+
);
46+
} catch (error) {
47+
logger.warn("BatchStreamGrants: failed to mint grant", {
48+
batchId,
49+
error: error instanceof Error ? error.message : String(error),
50+
});
51+
}
52+
}
53+
54+
/**
55+
* Consume one phase 2 request from the batch's grant.
56+
*
57+
* Returns false when there is no grant, when the budget is spent, or when Redis is
58+
* unreachable, so the caller falls back to the general rate limiter rather than opening
59+
* an unbounded bypass.
60+
*/
61+
async spend(environmentId: string, batchId: string): Promise<boolean> {
62+
try {
63+
// @ts-expect-error - Custom command defined via defineCommand
64+
const remaining = (await this.redis.spendBatchStreamGrant(
65+
this.#key(environmentId, batchId)
66+
)) as number;
67+
68+
return remaining >= 0;
69+
} catch (error) {
70+
logger.warn("BatchStreamGrants: failed to spend grant", {
71+
batchId,
72+
error: error instanceof Error ? error.message : String(error),
73+
});
74+
75+
return false;
76+
}
77+
}
78+
79+
async quit(): Promise<void> {
80+
await this.redis.quit();
81+
}
82+
83+
/**
84+
* Scoped to the environment as well as the batch, so a caller authenticated against a
85+
* different environment can never spend this batch's grant even if they know its id.
86+
*/
87+
#key(environmentId: string, batchId: string): string {
88+
return `${KEY_PREFIX}${environmentId}:${batchId}`;
89+
}
90+
91+
#registerCommands(): void {
92+
this.redis.defineCommand("spendBatchStreamGrant", {
93+
numberOfKeys: 1,
94+
lua: `
95+
local remaining = tonumber(redis.call('GET', KEYS[1]))
96+
97+
if not remaining or remaining <= 0 then
98+
return -1
99+
end
100+
101+
return redis.call('DECR', KEYS[1])
102+
`,
103+
});
104+
}
105+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { env } from "~/env.server";
2+
import { singleton } from "~/utils/singleton";
3+
import { BatchStreamGrants } from "./batchStreamGrants.server";
4+
5+
export const batchStreamGrants = singleton(
6+
"batchStreamGrants",
7+
() =>
8+
new BatchStreamGrants({
9+
redis: {
10+
port: env.RATE_LIMIT_REDIS_PORT,
11+
host: env.RATE_LIMIT_REDIS_HOST,
12+
username: env.RATE_LIMIT_REDIS_USERNAME,
13+
password: env.RATE_LIMIT_REDIS_PASSWORD,
14+
tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true",
15+
clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",
16+
},
17+
attempts: env.BATCH_STREAM_GRANT_ATTEMPTS,
18+
ttlMs: env.BATCH_SEAL_TIMEOUT_MS,
19+
})
20+
);

apps/webapp/app/runEngine/services/createBatch.server.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { RunId } from "@trigger.dev/core/v3/isomorphic";
44
import { type BatchTaskRun, Prisma } from "@trigger.dev/database";
55
import { Evt } from "evt";
66
import { prisma, type PrismaClientOrTransaction } from "~/db.server";
7+
import { env } from "~/env.server";
8+
import { batchStreamGrants } from "../concerns/batchStreamGrantsInstance.server";
79
import type { AuthenticatedEnvironment } from "~/services/apiAuth.server";
810
import { logger } from "~/services/logger.server";
911
import { controlPlaneResolver } from "~/v3/runOpsMigration/controlPlaneResolver.server";
@@ -120,8 +122,15 @@ export class CreateBatchService extends WithRunEngine {
120122

121123
this.onBatchTaskRunCreated.post(batch);
122124

125+
await batchStreamGrants.mint(environment.id, friendlyId);
126+
123127
// Block parent run if this is a batchTriggerAndWait
124128
if (body.parentRunId && body.resumeParentOnCompletion) {
129+
await this._engine.scheduleExpireBatch({
130+
batchId: batch.id,
131+
availableAt: new Date(Date.now() + env.BATCH_SEAL_TIMEOUT_MS),
132+
});
133+
125134
await this._engine.blockRunWithCreatedBatch({
126135
runId: RunId.fromFriendlyId(body.parentRunId),
127136
batchId: batch.id,

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1+
import { tryCatch } from "@trigger.dev/core/v3";
12
import { env } from "~/env.server";
3+
import { batchStreamGrants } from "~/runEngine/concerns/batchStreamGrantsInstance.server";
24
import { authenticateAuthorizationHeader } from "./apiAuth.server";
35
import { authorizationRateLimitMiddleware } from "./authorizationRateLimitMiddleware.server";
46
import type { Duration } from "./rateLimiter.server";
57

8+
const BATCH_STREAM_ITEMS_PATH = /^\/api\/v3\/batches\/([^/]+)\/items$/;
9+
610
export const apiRateLimiter = authorizationRateLimitMiddleware({
711
redis: {
812
port: env.RATE_LIMIT_REDIS_PORT,
@@ -75,6 +79,32 @@ export const apiRateLimiter = authorizationRateLimitMiddleware({
7579
/^\/api\/v2\/packets\//,
7680
/^\/api\/v1\/sessions\/[^/]+\/snapshot-url$/,
7781
],
82+
bypass: async (req) => {
83+
const match = BATCH_STREAM_ITEMS_PATH.exec(req.path);
84+
85+
if (!match) {
86+
return false;
87+
}
88+
89+
const batchFriendlyId = match[1];
90+
const authorizationValue = req.headers.authorization;
91+
92+
if (!batchFriendlyId || !authorizationValue) {
93+
return false;
94+
}
95+
96+
const [authError, authenticated] = await tryCatch(
97+
authenticateAuthorizationHeader(authorizationValue, {
98+
allowPublicKey: true,
99+
})
100+
);
101+
102+
if (authError || !authenticated || !authenticated.ok) {
103+
return false;
104+
}
105+
106+
return batchStreamGrants.spend(authenticated.environment.id, batchFriendlyId);
107+
},
78108
log: {
79109
rejections: env.API_RATE_LIMIT_REJECTION_LOGS_ENABLED === "1",
80110
requests: env.API_RATE_LIMIT_REQUEST_LOGS_ENABLED === "1",

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

Lines changed: 30 additions & 12 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,10 +55,17 @@ 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)[];
62+
/**
63+
* Escape hatch for requests that can only be admitted by consulting state, rather than by
64+
* matching a path. Runs after the authorization header check, so an unauthenticated
65+
* request is still rejected, and only skips the rate limit itself. Must not throw: a
66+
* bypass that cannot decide should return false and let the limiter apply.
67+
*/
68+
bypass?: (req: ExpressRequest) => Promise<boolean>;
6369
defaultLimiter: RateLimiterConfig;
6470
limiterConfigOverride?: LimitConfigOverrideFunction;
6571
limiterCache?: {
@@ -151,6 +157,7 @@ export function authorizationRateLimitMiddleware({
151157
defaultLimiter,
152158
pathMatchers,
153159
pathWhiteList = [],
160+
bypass,
154161
log = {
155162
rejections: true,
156163
requests: true,
@@ -176,16 +183,7 @@ export function authorizationRateLimitMiddleware({
176183
}),
177184
});
178185

179-
const redisClient = createRedisRateLimitClient(
180-
redis ?? {
181-
port: env.RATE_LIMIT_REDIS_PORT,
182-
host: env.RATE_LIMIT_REDIS_HOST,
183-
username: env.RATE_LIMIT_REDIS_USERNAME,
184-
password: env.RATE_LIMIT_REDIS_PASSWORD,
185-
tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true",
186-
clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",
187-
}
188-
);
186+
const redisClient = createRedisRateLimitClient(redis);
189187

190188
return async (req: ExpressRequest, res: ExpressResponse, next: NextFunction) => {
191189
if (log.requests) {
@@ -247,6 +245,26 @@ export function authorizationRateLimitMiddleware({
247245
);
248246
}
249247

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();
265+
}
266+
}
267+
250268
const hash = createHash("sha256");
251269
hash.update(authorizationValue);
252270
const hashedAuthorizationValue = hash.digest("hex");

apps/webapp/app/v3/runOpsMigration/unblockRouteCatalog.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ export const UNBLOCK_ROUTES: readonly UnblockRoute[] = [
6565
site: BATCH_SYSTEM,
6666
symbol: "#tryCompleteBatch",
6767
},
68+
{
69+
id: "batch.expireBatch",
70+
kind: "RUN",
71+
site: BATCH_SYSTEM,
72+
symbol: "expireBatch",
73+
},
6874
{
6975
id: "ttl.expireRun",
7076
kind: "RUN",

0 commit comments

Comments
 (0)