DEV Community

Daniel Pertu
Daniel Pertu

Posted on

Two rate limit checks, two Redis round trips, and why the parallel version had to be a second function

Scanning a barcode is the loop Munchable exists for, so the latency of one API route is, as far as anybody using the app is concerned, the latency of the product. Before a warm cache hit could leave the building, that route did four network-shaped things strictly one after another: authenticate, check the rate limiter, check the second rate limiter, read the taxonomy version header, and only then read the cache.

None of the first four needs another's result. This post is about the two in the middle, because fixing them turned out to be a question about the nine other routes that share the helper.

Two dimensions, every time

Each endpoint is limited along two axes, never one:

_limiters = {
  productDev: mk(Ratelimit.slidingWindow(60, '1 m'), 'rl:product:dev'),
  productIp: mk(Ratelimit.slidingWindow(300, '1 m'), 'rl:product:ip'),
  searchDev: mk(Ratelimit.slidingWindow(60, '1 m'), 'rl:search:dev'),
  searchIp: mk(Ratelimit.slidingWindow(300, '1 m'), 'rl:search:ip'),
  ocrDevMin: mk(Ratelimit.slidingWindow(8, '1 m'), 'rl:ocr:dev:m'),
  ocrDevDay: mk(Ratelimit.slidingWindow(40, '1 d'), 'rl:ocr:dev:d'),
  ocrIp: mk(Ratelimit.slidingWindow(30, '1 m'), 'rl:ocr:ip'),
  // ... and so on, per endpoint
};
Enter fullscreen mode Exit fullscreen mode

The per-user key is the JWT subject. The per-IP key exists because the per-user one is trivially defeated by making more accounts, and the per-user one exists because the per-IP one punishes everybody behind one mobile carrier's NAT. Neither is sufficient and together they are cheap.

Two details in the constructor are worth stealing. mk passes analytics: false to every limiter, because that option exists to record per-identifier usage events for a dashboard, and a rate limiter has no business keeping a log of who asked for what. And the whole map is built lazily on first use, which keeps a missing Redis configuration from turning into a module-load crash in a test runner that never makes a request.

For the expensive endpoints there is a second pattern: a per-minute limiter chained in front of a per-day limiter. ocrDevMin then ocrDevDay. That chain is the thing this post is about.

The helper, and the one property nobody wrote down

/** Enforce a chain of limiter checks; returns the FIRST failing decision. */
export async function enforce(
  checks: Array<{ limiter: Ratelimit; key: string }>,
): Promise<LimitDecision> {
  for (const { limiter, key } of checks) {
    const r = await limiter.limit(key);
    if (!r.success) {
      return { success: false, limit: r.limit, remaining: r.remaining, reset: r.reset };
    }
  }
  return { success: true };
}
Enter fullscreen mode Exit fullscreen mode

Nine routes call this. Read it as a performance problem and the fix is obvious: the checks do not depend on each other, so Promise.all them and pay one round trip instead of N.

Read it as a behaviour and something else is going on. limiter.limit(key) is not a question, it is a consumption: it increments the counter for that key. The return inside the loop therefore means that a request already rejected by the minute limiter never touches the day limiter's counter. For the chained callers, that is not an optimisation, it is the semantics. Burn your eight-per-minute ceiling, and you have spent eight of your daily forty, not sixteen, and not eight plus however many times you kept pressing.

Swap that loop for Promise.all and every chained caller starts consuming the daily budget on requests the minute limiter already refused. A user who hits a wall and retries for a minute can spend a day's allowance on responses they never got. That is a real regression, in routes the change was not about.

So the parallel version is a different function

export async function enforceParallel(
  checks: Array<{ limiter: Ratelimit; key: string }>,
): Promise<LimitDecision> {
  const results = await Promise.all(checks.map(({ limiter, key }) => limiter.limit(key)));
  for (const r of results) {
    if (!r.success) {
      return { success: false, limit: r.limit, remaining: r.remaining, reset: r.reset };
    }
  }
  return { success: true };
}
Enter fullscreen mode Exit fullscreen mode

Same contract on the way out, opposite trade on the way in: it always increments every counter, and in exchange it collapses N sequential round trips into one. The doc comment above it is longer than the function, and the last line of it is the part that matters:

 * Do not repoint an existing `enforce` caller at this function without
 * checking whether it relies on the skip-on-earlier-failure property.
Enter fullscreen mode Exit fullscreen mode

enforce was left byte for byte unchanged. The barcode lookup is the route the new function was written for:

  // Abuse limiters fail OPEN for /product-lookup (availability > fairness on a
  // read that reveals no health data). `enforceParallel` rather than `enforce`:
  // the two checks below are independent of each other and of everything
  // above, so they now cost one Redis round trip instead of two in series.
  let decision: LimitDecision;
  try {
    const ip = getClientIdentifier(request);
    const lim = limiters();
    decision = await enforceParallel([
      { limiter: lim.productDev, key: user.id },
      { limiter: lim.productIp, key: ip },
    ]);
  } catch {
    decision = { success: true };
  }
Enter fullscreen mode Exit fullscreen mode

Two checks, one round trip, and no chain to preserve because a per-user minute limiter and a per-IP minute limiter are not a ladder. The product search route, whose two checks have the same shape, moved with it. The other nine kept the loop.

The test is about a race, not about the happy path

The interesting test is not "it limits things". It is this one:

test('enforceParallel: the first-by-array-order failure wins, not the first to settle', async () => {
  const slowAndFailing = fakeLimiter({ success: false, limit: 60, remaining: 0, reset: 111 }, 20);
  const fastAndFailing = fakeLimiter({ success: false, limit: 300, remaining: 0, reset: 222 }, 0);
  const decision = await enforceParallel([
    { limiter: slowAndFailing.limiter, key: 'user-1' },
    { limiter: fastAndFailing.limiter, key: 'ip-1' },
  ]);
  // fastAndFailing resolves first in wall-clock time; slowAndFailing is first
  // in the `checks` array and must still be what the caller sees.
  assert.equal(decision.limit, 60);
  assert.equal(decision.reset, 111);
});
Enter fullscreen mode Exit fullscreen mode

When more than one check fails, the returned decision is what the client sees, because the caller reads limit, remaining and reset off it to build the 429:

export function tooManyRequests(scope: string, d: LimitDecision) {
  const retryAfter = d.reset
    ? Math.max(1, Math.ceil((d.reset - Date.now()) / 1000))
    : 1;
  return {
    body: { error: 'rate_limited', scope, retryAfter },
    headers: { 'Retry-After': String(retryAfter), ...rateLimitHeaders(d) },
  };
}
Enter fullscreen mode Exit fullscreen mode

Had enforceParallel been written with Promise.race, or with a loop over results in settle order, the Retry-After on two identical requests would flicker between two limiters' numbers depending on which Redis call happened to come back first. A client doing the correct thing with Retry-After would then back off for an interval chosen at random. Iterating results in array order, after awaiting all of them, is what makes it deterministic, and that is a one-line property that a future refactor could lose without any test failing. Hence a test whose entire purpose is to pin it.

The second test is similarly unglamorous and similarly load-bearing: it asserts that the later check was called after an earlier one failed. That is the one behaviour difference from enforce, it is the whole reason the function exists, and it is exactly what a well-meaning "simplify this" edit would delete.

Which way does a limiter fail?

This is the question I would ask first in a review, and it has a different answer per route.

Product lookup fails open. If Redis is unreachable, the catch above hands back { success: true } and the scan proceeds. It is a condition-blind read that reveals nothing about anybody, and an app that cannot scan a barcode because a cache is down is worse than an app that briefly cannot rate limit.

The webhook limiter does the same, for a sharper reason:

/** Fail-open webhook rate limit (a dropped real webhook is worse than a dup). */
Enter fullscreen mode Exit fullscreen mode

The spend limiter fails closed:

export async function reserveInlineBudget(principal: BudgetPrincipal | string): Promise<boolean> {
  // ...
  } catch {
    return false;
  }
}
Enter fullscreen mode Exit fullscreen mode

No Redis, no model call. The thing being protected there is a bill, and a bill with no ceiling is not a thing to be optimistic about. The same function has one more decision in it worth copying: the per-principal counter is incremented before the global one, so a caller already over its own daily cap cannot drain the shared budget with every rejected call.

The identifier the limiter keys on

A rate limiter is only as good as its key, and this is the easiest thing in the file to get wrong:

/**
 * Trusted client IP. Prefer `x-real-ip` (platform-set, not client-forgeable);
 * fall back to the closest-hop entry of `x-forwarded-for`. Never key on the
 * left-most XFF token, it is client-supplied and spoofable.
 */
export function getClientIdentifier(request: NextRequest): string {
  const realIp = request.headers.get('x-real-ip');
  if (realIp?.trim()) return realIp.trim();
  const fwd = request.headers.get('x-forwarded-for');
  if (fwd?.trim()) {
    const parts = fwd.split(',').map((s) => s.trim()).filter(Boolean);
    if (parts.length) return parts[parts.length - 1];
  }
  return 'unknown';
}
Enter fullscreen mode Exit fullscreen mode

The left-most entry of X-Forwarded-For is the conventional "original client", and it is also whatever the client decided to send. Key a limiter on it and anybody can have a fresh quota per request by incrementing a number. The right-most entry is the one appended by the hop closest to you, which is the only part of that header you did not take on trust.

One more thing the 429 must not do

if (!decision.success) {
  // Discard the speculative cache read: a 429 must not carry product data
  // any more than a 401 may, and this path never reaches `lookupProduct`.
  cachePromise?.catch(() => {});
  const { body, headers } = tooManyRequests('product', decision);
  return NextResponse.json(body, { status: 429, headers });
}
Enter fullscreen mode Exit fullscreen mode

Starting the cache read before the limiter is what makes the route fast. It also means a rejected request has a promise in flight holding exactly the data it was not allowed to have. Awaiting it to put it in the error response would be an interesting way to make a rate limiter optional.

If you want to see the half of Munchable that has no rate limiter at all, it is the public half: a few hundred prerendered pages like the ingredient answers index and the SIBO guide are static files from a CDN, with no user, no key and no Redis anywhere in the path. The homepage explains what the scanning side does, and that side is the one that has to count.

Top comments (0)