c72ebf9084
## 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.
419 lines
14 KiB
TypeScript
419 lines
14 KiB
TypeScript
import { redisTest } from "@internal/testcontainers";
|
|
import { describe, expect, vi, beforeEach } from "vitest";
|
|
|
|
vi.setConfig({ testTimeout: 30_000 }); // 30 seconds timeout
|
|
|
|
// Mock the logger
|
|
vi.mock("./logger.server", () => ({
|
|
logger: {
|
|
info: vi.fn(),
|
|
error: vi.fn(),
|
|
},
|
|
}));
|
|
|
|
import type { Express } from "express";
|
|
import express from "express";
|
|
import request from "supertest";
|
|
import { authorizationRateLimitMiddleware } from "../app/services/authorizationRateLimitMiddleware.server.js";
|
|
|
|
describe.skipIf(process.env.GITHUB_ACTIONS)("authorizationRateLimitMiddleware", () => {
|
|
let app: Express;
|
|
|
|
beforeEach(() => {
|
|
app = express();
|
|
});
|
|
|
|
redisTest("should allow requests within the rate limit", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 10,
|
|
interval: "1m",
|
|
maxTokens: 100,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
log: {
|
|
rejections: false,
|
|
requests: false,
|
|
},
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => {
|
|
res.status(200).json({ message: "Success" });
|
|
});
|
|
|
|
const response = await request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
expect(response.status).toBe(200);
|
|
expect(response.body).toEqual({ message: "Success" });
|
|
expect(response.headers["x-ratelimit-limit"]).toBeDefined();
|
|
expect(response.headers["x-ratelimit-remaining"]).toBeDefined();
|
|
expect(response.headers["x-ratelimit-reset"]).toBeDefined();
|
|
});
|
|
|
|
redisTest("should reject requests without an Authorization header", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 10,
|
|
interval: "1m",
|
|
maxTokens: 100,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => {
|
|
res.status(200).json({ message: "Success" });
|
|
});
|
|
|
|
const response = await request(app).get("/api/test");
|
|
|
|
expect(response.status).toBe(401);
|
|
expect(response.body).toHaveProperty("title", "Unauthorized");
|
|
});
|
|
|
|
redisTest("should reject requests that exceed the rate limit", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 1,
|
|
interval: "1m",
|
|
maxTokens: 1,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => {
|
|
res.status(200).json({ message: "Success" });
|
|
});
|
|
|
|
// First request should succeed
|
|
await request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
// Second request should be rate limited
|
|
const response = await request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
expect(response.status).toBe(429);
|
|
expect(response.body).toHaveProperty("title", "Rate Limit Exceeded");
|
|
});
|
|
|
|
redisTest("should not apply rate limiting to whitelisted paths", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 10,
|
|
interval: "1m",
|
|
maxTokens: 100,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
pathWhiteList: ["/api/whitelist"],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/whitelist", (req, res) => {
|
|
res.status(200).json({ message: "Whitelisted" });
|
|
});
|
|
|
|
const response = await request(app)
|
|
.get("/api/whitelist")
|
|
.set("Authorization", "Bearer test-token");
|
|
|
|
expect(response.status).toBe(200);
|
|
expect(response.body).toEqual({ message: "Whitelisted" });
|
|
expect(response.headers["x-ratelimit-limit"]).toBeUndefined();
|
|
});
|
|
|
|
redisTest(
|
|
"should apply different rate limits based on limiterConfigOverride",
|
|
async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 1,
|
|
interval: "1m",
|
|
maxTokens: 1,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
limiterConfigOverride: async (authorizationValue) => {
|
|
if (authorizationValue === "Bearer premium-token") {
|
|
return {
|
|
type: "tokenBucket",
|
|
refillRate: 10,
|
|
interval: "1m",
|
|
maxTokens: 100,
|
|
};
|
|
}
|
|
return undefined;
|
|
},
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => {
|
|
res.status(200).json({ message: "Success" });
|
|
});
|
|
|
|
// Regular user should be rate limited after 1 request
|
|
await request(app).get("/api/test").set("Authorization", "Bearer regular-token");
|
|
const regularResponse = await request(app)
|
|
.get("/api/test")
|
|
.set("Authorization", "Bearer regular-token");
|
|
expect(regularResponse.status).toBe(429);
|
|
|
|
// Premium user should be able to make multiple requests
|
|
const premiumResponse1 = await request(app)
|
|
.get("/api/test")
|
|
.set("Authorization", "Bearer premium-token");
|
|
expect(premiumResponse1.status).toBe(200);
|
|
const premiumResponse2 = await request(app)
|
|
.get("/api/test")
|
|
.set("Authorization", "Bearer premium-token");
|
|
expect(premiumResponse2.status).toBe(200);
|
|
}
|
|
);
|
|
|
|
describe("Advanced Cases", () => {
|
|
// 1. Test different rate limit configurations
|
|
redisTest("should enforce fixed window rate limiting", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test-fixed",
|
|
defaultLimiter: {
|
|
type: "fixedWindow",
|
|
window: "10s",
|
|
tokens: 3,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => res.status(200).json({ message: "Success" }));
|
|
|
|
const makeRequest = () =>
|
|
request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
// Should allow 3 requests
|
|
for (let i = 0; i < 3; i++) {
|
|
const response = await makeRequest();
|
|
expect(response.status).toBe(200);
|
|
}
|
|
|
|
// 4th request should be rate limited
|
|
const limitedResponse = await makeRequest();
|
|
expect(limitedResponse.status).toBe(429);
|
|
|
|
// Wait for the window to reset
|
|
await new Promise((resolve) => setTimeout(resolve, 10000));
|
|
|
|
// Should allow requests again
|
|
const newResponse = await makeRequest();
|
|
expect(newResponse.status).toBe(200);
|
|
});
|
|
|
|
redisTest("should enforce sliding window rate limiting", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test-sliding",
|
|
defaultLimiter: {
|
|
type: "slidingWindow",
|
|
window: "10s",
|
|
tokens: 3,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => res.status(200).json({ message: "Success" }));
|
|
|
|
const makeRequest = () =>
|
|
request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
// Should allow 3 requests
|
|
for (let i = 0; i < 3; i++) {
|
|
const response = await makeRequest();
|
|
expect(response.status).toBe(200);
|
|
}
|
|
|
|
// 4th request should be rate limited
|
|
const limitedResponse = await makeRequest();
|
|
expect(limitedResponse.status).toBe(429);
|
|
|
|
// Wait for part of the window to pass
|
|
await new Promise((resolve) => setTimeout(resolve, 1000));
|
|
|
|
// Should still be limited
|
|
const stillLimitedResponse = await makeRequest();
|
|
expect(stillLimitedResponse.status).toBe(429);
|
|
|
|
// Wait for the full window to pass
|
|
await new Promise((resolve) => setTimeout(resolve, 10000));
|
|
|
|
// Should allow requests again
|
|
const newResponse = await makeRequest();
|
|
expect(newResponse.status).toBe(200);
|
|
});
|
|
|
|
// 2. Test edge cases around rate limit calculations
|
|
redisTest("should handle token refill correctly", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test-refill",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 1,
|
|
interval: "5s",
|
|
maxTokens: 3,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => res.status(200).json({ message: "Success" }));
|
|
|
|
const makeRequest = () =>
|
|
request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
// Use up all tokens
|
|
for (let i = 0; i < 3; i++) {
|
|
const response = await makeRequest();
|
|
expect(response.status).toBe(200);
|
|
}
|
|
|
|
// Next request should be limited
|
|
const limitedResponse = await makeRequest();
|
|
expect(limitedResponse.status).toBe(429);
|
|
|
|
// Wait for one token to be refilled
|
|
await new Promise((resolve) => setTimeout(resolve, 5000));
|
|
|
|
// Should allow one request
|
|
const newResponse = await makeRequest();
|
|
expect(newResponse.status).toBe(200);
|
|
|
|
// But the next one should be limited again
|
|
const limitedAgainResponse = await makeRequest();
|
|
expect(limitedAgainResponse.status).toBe(429);
|
|
});
|
|
|
|
redisTest("should handle near-zero remaining tokens correctly", async ({ redisOptions }) => {
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test-near-zero",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 1, // 1 token every 5 seconds
|
|
interval: "5s",
|
|
maxTokens: 1,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => res.status(200).json({ message: "Success" }));
|
|
|
|
const makeRequest = () =>
|
|
request(app).get("/api/test").set("Authorization", "Bearer test-token");
|
|
|
|
// First request should succeed
|
|
const firstResponse = await makeRequest();
|
|
expect(firstResponse.status).toBe(200);
|
|
|
|
// Immediate second request should fail
|
|
const secondResponse = await makeRequest();
|
|
expect(secondResponse.status).toBe(429);
|
|
|
|
// Wait for almost one token to be refilled (4.9 seconds)
|
|
await new Promise((resolve) => setTimeout(resolve, 4900));
|
|
|
|
// This request should still fail as we're just shy of a full token
|
|
const thirdResponse = await makeRequest();
|
|
expect(thirdResponse.status).toBe(429);
|
|
|
|
// Wait for the full token to be refilled (additional 200ms)
|
|
await new Promise((resolve) => setTimeout(resolve, 200));
|
|
|
|
// This request should now succeed
|
|
const fourthResponse = await makeRequest();
|
|
expect(fourthResponse.status).toBe(200);
|
|
|
|
// Immediate next request should fail again
|
|
const fifthResponse = await makeRequest();
|
|
expect(fifthResponse.status).toBe(429);
|
|
});
|
|
|
|
// 3. Test the limiterCache functionality
|
|
redisTest("should use cached limiter configurations", async ({ redisOptions }) => {
|
|
let configOverrideCalls = 0;
|
|
const rateLimitMiddleware = authorizationRateLimitMiddleware({
|
|
redis: { ...redisOptions, tlsDisabled: true },
|
|
keyPrefix: "test-cache",
|
|
defaultLimiter: {
|
|
type: "tokenBucket",
|
|
refillRate: 1,
|
|
interval: "1m",
|
|
maxTokens: 10,
|
|
},
|
|
pathMatchers: [/^\/api/],
|
|
limiterCache: {
|
|
fresh: 1000, // 1 second
|
|
stale: 2000, // 2 seconds
|
|
maxItems: 1000,
|
|
},
|
|
limiterConfigOverride: async (authorizationValue) => {
|
|
configOverrideCalls++;
|
|
if (authorizationValue === "Bearer premium-token") {
|
|
return {
|
|
type: "tokenBucket",
|
|
refillRate: 10,
|
|
interval: "1m",
|
|
maxTokens: 100,
|
|
};
|
|
}
|
|
return undefined;
|
|
},
|
|
});
|
|
|
|
app.use(rateLimitMiddleware);
|
|
app.get("/api/test", (req, res) => res.status(200).json({ message: "Success" }));
|
|
|
|
const makeRequest = () =>
|
|
request(app).get("/api/test").set("Authorization", "Bearer premium-token");
|
|
|
|
// First request should call the override
|
|
await makeRequest();
|
|
expect(configOverrideCalls).toBe(1);
|
|
|
|
// Subsequent requests within 1 second should use the cache
|
|
await makeRequest();
|
|
await makeRequest();
|
|
expect(configOverrideCalls).toBe(1);
|
|
|
|
// Wait for the cache to become stale
|
|
await new Promise((resolve) => setTimeout(resolve, 1100));
|
|
|
|
// This should still use the cache, but also trigger a refresh
|
|
await makeRequest();
|
|
expect(configOverrideCalls).toBe(2);
|
|
|
|
// Wait for the cache to expire completely
|
|
await new Promise((resolve) => setTimeout(resolve, 1000));
|
|
|
|
// This should trigger a new override call
|
|
await makeRequest();
|
|
expect(configOverrideCalls).toBe(3);
|
|
});
|
|
});
|
|
});
|