fix(redis): stale-job reaping, single-writer reservoir refresh, no double-execution on release failure

[taukirsheikh]

Fixes three defects in DistributedRateLimiter that surface under real-world distributed conditions — process crashes, multiple processes sharing one limiter, and Redis blips. All three were found while investigating a production outage caused by a different limiter library (Bottleneck) wedging an entire worker fleet; this PR closes the equivalent failure modes here before they bite.

Issue 1 — Crashed processes leak slots forever; the limiter can wedge permanently

The problem. ACQUIRE_SLOT increments running/currentWeight and records the job in <state>:jobs. If the process dies before RELEASE_SLOT runs (SIGKILL, OOM, a deploy), that weight is never reclaimed. The HEARTBEAT script even had the placeholder comment — -- Could add stale job cleanup here if needed — but no cleanup. And because heartbeats from surviving processes keep extending the key TTL, the leaked state never expires on a busy system.

Example. maxConcurrent: 10. A deploy SIGKILLs a worker holding 4 slots. currentWeight is now permanently 4, so only 6 slots exist. Three deploys later the limiter admits zero jobs — every acquire returns 'concurrency' — and it stays wedged through every restart, because the poisoned state lives in Redis, not in any process. (This is exactly how a production fleet using Bottleneck died: running: 18263 leaked in Redis, surviving restarts.)

The fix. Job start times are tracked in a <state>:jobs:started ZSET. The heartbeat now reaps jobs older than the new staleJobTimeout option (default 60s — set it above your longest expected job): their weight is reclaimed, counters are clamped at ≥ 0, and a reaped event is emitted. A reaped job's late release is a no-op (release only decrements when it actually finds the job), so counters can't drift negative.

Issue 2 — Every process refreshed the shared reservoir: N processes = N× the configured rate

The problem. The reservoir refresh ran in a client-side setInterval per limiter instance, each doing a blind HSET reservoir = amount. The reservoir is shared state in Redis, but every process refilled it on its own staggered timer.

Example. reservoir: 25, reservoirRefreshInterval: 1000 across 10 worker processes — intended 25 req/s. Ten timers each reset the reservoir to 25 at staggered offsets, so capacity is re-granted up to 10 times per second: an effective rate approaching 250 req/s. The more you scale out, the less your rate limit means — the opposite of what a distributed limiter is for.

The fix. The client timers are gone. Refresh now happens lazily inside the ACQUIRE_SLOT Lua script (lastReservoirRefresh stored in the state hash), so refill is atomic and single-writer by construction — the rate is the same whether one process or fifty share the limiter.

Issue 3 — A Redis blip on the success path re-executed an already-succeeded job

The problem. In executeJob, the success-path releaseSlot call sat inside the main try. If Redis hiccupped right there — after the job's fn had already succeeded — control fell into the catch, which (a) called releaseSlot a second time, drifting counters negative, and (b) ran the retry logic, re-executing a job whose side effects had already happened.

Example. The job books an appointment via a third-party API. The booking succeeds; Redis times out on the release. The catch path retries the job → the appointment is booked twice. The caller's promise eventually resolves as if everything ran once.

The fix. executeJob is restructured into explicit steps: run the fn, release the slot exactly once in its own try/catch, then settle. A succeeded fn always resolves with its result — a release failure only emits an error event and can never trigger a retry or a second release.

Hardening from adversarial review

The initial implementation of the three fixes was put through a three-lens adversarial review (Lua atomicity, JS job lifecycle, test adequacy — each claim verified against a live Redis with PoC scripts). Five additional defects were found and fixed in this PR:

• First acquire clobbered the configured initial reservoir (introduced by the lazy refresh): lastReservoirRefresh seeded to 0 made the first acquire "refresh" immediately, overwriting reservoir: 1 with reservoirRefreshAmount: 5. Init now stamps the current time; a legacy/missing stamp seeds without refreshing.
• Duplicate job ids leaked slots permanently (JobOptions.id is public API): two acquires with the same id incremented running twice but tracked one entry; the second release was a no-op. Acquire now rejects an id that is still in flight with a 'duplicate' reason, restoring increment/decrement symmetry.
• fn could still run twice (pre-existing, blocker): tryProcess peeked the queue head, awaited the Redis acquire, then blindly dequeue()d — if the heap root changed during the await (higher-priority job, cancel(), abort), the peeked job ran while a different job was removed and its promise hung forever. The loop now removes the job by identity and releases the slot if the job vanished mid-acquire.
• Unhandled rejection from the acquire loop: any Redis error inside tryProcess (fire-and-forget on five call sites) crashed Node ≥ 15 with an unhandledRejection and stranded queued jobs. The loop now catches, emits error, and reschedules polling.
• The reservoir test wasn't a regression test — it passed on the pre-fix code. Replaced with a cross-instance test asserting capacity is granted exactly once per interval (verified to fail on the old implementation).

Two more pre-existing bugs surfaced by the new tests are included: a processing-guard race letting two concurrent schedule() calls double-execute one job, and a priority-queue tie-break (same priority, same millisecond) that could starve an earlier job behind a depleted reservoir.

Tests

• npm test: 18 passed
• npm run test:redis (live Redis): 19 passed, including new suites for all three fixes — dead-process reap + healthy-job non-reap, cross-instance single refresh with exact reservoir assertions, and success-path release failure (resolves with result, fn ran exactly once, error emitted)
• npm run build: clean

Known pre-existing issue left untouched: the invalid-host connection test leaks a retrying ioredis client, so the redis suite prints its summary but the process needs a kill to exit.

🤖 Generated with Claude Code