MotirBuilding in public
MOTIR · moooon
onMotir
You’re viewing a public project. Anyone can view it — no account needed. Sign in to submit, upvote, or comment on requests.View-only — you can’t edit work items
MOTIR-2647

(motir-core) `rate-limit-gate.test.ts` pins the BUDGET but not the WINDOW, so two calls straddling a minute boundary read as under-budget and the assertion gets PROJECT_NOT_FOUND

Done
Description

Repo: motir-core. One PR. A test that shipped with MOTIR-2610 (#2016) is time-dependent: it pins the rate-limit BUDGET but leaves the WINDOW at its epoch-aligned 60-second default, so it fails whenever its two calls land on opposite sides of a minute.

The signature

FAIL tests/mcp/rate-limit-gate.test.ts > the billable-tool gate, wired into a real server
     > refuses an over-budget expand_item with an isError tool result, not a transport error

AssertionError: expected '[{"type":"text","text":"PROJECT_NOT_F…' to contain 'RATE_LIMITED'
  Received: "[{"type":"text","text":"PROJECT_NOT_FOUND: Project ACME not found."}]"

> 88 |     expect(textOf(second.content)).toContain(RATE_LIMITED_CODE);

Note expect(second.isError).toBe(true) on line 87 PASSES — the call was allowed through, reached the tool, and failed on the fixture's missing project. Only the reason is wrong.

Mechanism

tests/mcp/rate-limit-gate.test.ts:76 sets MOTIR_AI_GENERATE_RATE_LIMIT = '1' and nothing else. lib/rateLimit/limiter.ts:67 buckets on an epoch-aligned fixed window:

const windowStart = Math.floor(now / budget.windowMs) * budget.windowMs;  // windowMs = 60_000

Call 1 spends the single unit; call 2 is expected to be refused. If a minute boundary falls between them the counter starts fresh and call 2 is allowed. The file's own ENVS array already lists MOTIR_AI_GENERATE_RATE_LIMIT_WINDOW_MS for cleanup while never setting it — the harness anticipates a pin nobody wrote.

⚠️ Rule out the other mechanism before fixing — it produces the IDENTICAL symptom

consumeRateLimit fails open when the store is slow (lib/rateLimit/limiter.ts:77):

} catch (err) {
  console.error('[rateLimit] store unavailable; allowing the request', err);
  return { allowed: true, …, degraded: true };
}

Under a loaded runner a withDeadline timeout on the Postgres increment lets call 2 through the same way. grep 'store unavailable' in the job log decides it: present ⇒ the deadline, absent ⇒ the window. On the occurrence below it was absent, so this card is about the window. If a future occurrence has it present, that is a different bug about the store deadline — do not fold them together.

The cure already exists in this repo

tests/helpers/rateLimitWindow.ts exports ALIGNED_WINDOW_MS = 2_000 and waitForWindowBoundary(). tests/api/v1/wrapper.test.ts and tests/publicProjects/publicSubmit.test.ts both use them for exactly this reason. This test uses neither.

Do this

  1. Pin MOTIR_AI_GENERATE_RATE_LIMIT_WINDOW_MS to ALIGNED_WINDOW_MS, and await waitForWindowBoundary(ALIGNED_WINDOW_MS) before the first call, so both calls provably share one bucket.
  2. Sweep the file — every case that spends then re-spends a budget has the same exposure, not just this one.
  3. While there: the first call's PROJECT_NOT_FOUND is load-bearing but silent. Assert it, so "the gate let it through" is checked rather than implied by the absence of RATE_LIMITED.

Acceptance criteria

  1. Both calls in the over-budget case are pinned to one window, and the test does not depend on wall-clock position within a minute.
  2. Proven by CONSTRUCTION, not by re-running: with the window pinned, a deliberately-crossed boundary (advance past ALIGNED_WINDOW_MS) still refuses — and removing the pin reproduces PROJECT_NOT_FOUND. A fix nobody can watch fail first is a fix nobody can tell worked.
  3. Every budget-spending case in the file is swept, not only line 88.
  4. The first call's outcome is asserted explicitly rather than left as a .catch(() => null) whose success is inferred.

Out of scope

The fail-open store-deadline path above (a different bug if it ever fires), and MOTIR-2645's signIn navigation race. No production code changes — lib/rateLimit/limiter.ts's epoch-aligned bucket is correct and is the same shape /api/v1 uses.