ReviewOS

stacks/ts-rate-limiter

fix: snapshot fixed-window increment results for concurrent callers

#523
Closed chris opened this 10 days ago ·
10 days ago

Found while profiling the Stacks action wrapper in Stacks #2451.

An initialized fixed-window limiter with a maximum of 3 rejects every call in a simultaneous burst of 10. It should admit 3 and reject 7. The wrapper's separate cold-construction race is tracked in Stacks #2451; this issue reproduces directly against the dependency.

const limiter = new RateLimiter({ maxRequests: 3, windowMs: 60000, algorithm: 'fixed-window' })
const results = await Promise.allSettled(Array.from({ length: 10 }, () => limiter.enforce('one-client')))
console.log(results.filter(result => result.status === 'fulfilled').length) // 0, expected 3

Source at e7e1ded: MemoryStorage.increment() returns the internal mutable record both on creation and update. Subsequent synchronous increments change that same object's count before checkFixedWindow() resumes its await. Every concurrent caller therefore sees the final count of 10. Sliding-window consumption already has a per-call snapshot, but the fixed-window implementation does not.

Return a stable per-increment result using the actual recorded count and reset time, without sharing mutable storage records with callers. Verify direct MemoryStorage increments produce counts 1 through N, previously returned values remain stable, and concurrent fixed-window enforcement admits exactly the configured quota. Run existing limiter tests and benchmark the allocation/CPU effect. Do not serialize Stacks requests or change quota semantics to compensate.

10 days ago

Implemented and pushed to origin/main as Chris in bbf0ba1.

Both MemoryStorage.increment return paths now copy the recorded count/resetTime into a per-call result. No request serialization, alternate quota rules, or Stacks-side compensation is involved. The storage keeps its existing bucket update path; callers cannot mutate that bucket through a returned result.

All four regressions failed against e7e1ded and pass after the fix. Concurrent results contain positions 1 through 10, old values remain stable, caller mutations cannot reset quota state, a burst of 10 at a quota of 3 admits exactly 3, and warm bursts/separate keys/reset preserve their expected behavior. The actual built package was also checked for positions 1 through 10 and exactly three admissions.

Local cost diagnostic on Bun 1.3.14/macOS arm64, using built package snapshots with the package's target/minification settings:

CPU per 200,000 sequential callsBefore e7e1dedAfter bbf0ba1
Direct MemoryStorage.increment12.434 ms12.620 ms
RateLimiter.consume, fixed window31.756 ms32.024 ms

Eight fresh-process pairs per workload alternated version order. Each process warmed 20,000 calls and measured nine batches of 200,000; the table reports the median of those process medians. Direct increment CPU rose about 1.5% in the aggregate; full consume CPU rose about 0.8%. Per-pair consume ratios ranged from 1.002 to 1.077, so this is not a claim of a precise universal cost. This is a correctness fix with a measured allocation cost, not a throughput optimization. The returned snapshot adds one short-lived result object per increment. Post-GC heap medians remained around 2.5 MB in both variants; this is a small single-key diagnostic, not a general memory bound.

Reproduction: build each revision to a separate temporary output, import MemoryStorage and RateLimiter from that output, and measure process.cpuUsage around sequential awaits. Use a 600,000 ms fixed window, maximum 1e9, one client key, and assert the last count equals 20,000 + (batch + 1) * 200,000. A high quota keeps the two sequential workloads behavior-equivalent. Concurrent before/after throughput would not be comparable because the old version rejects requests incorrectly.

Validation: bun run test reports 54 passing tests; bun run typecheck and bun run build pass. bun run lint exits successfully with zero errors and three existing documentation warnings. Both review axes are clear. No unrelated files changed.

The registry still reports 0.4.8. Stacks adoption requires a published version containing this commit and a framework-level burst check. Stacks #2451 already fixes its separate cold-initialization race; its dependency has not been patched or linked locally.

10 days ago

Downstream published-package adoption and the Stacks-level burst regression are tracked in Stacks #2453.

Sign in to comment on this issue.