rate-limit: Redis INCR+EXPIRE is not atomic — a TTL-less key denies its bucket forever (worst case: an installation's webhooks, permanently)

[JSONbored]

Summary

The Redis rate limiter increments a counter and then sets its expiry as two separate commands, with the expiry applied only when the counter reads exactly 1. If anything interrupts the pair, the key is left with no TTL: the fixed window never resets, the counter grows forever, and every subsequent request on that key is denied until someone deletes the key by hand.

The most damaging key this can happen to is strict:/v1/github/webhook:installation:{id} — a stuck key means every webhook delivery for that installation is 429'd indefinitely, and GitHub does not auto-redeliver.

Mechanism (verified at HEAD, 776c414)

src/selfhost/redis-ratelimit.ts:24-27:

const count = await redis.incr(k);
if (count === 1) await redis.expire(k, body.windowSeconds); // start the window on first hit
const ttlMs = await redis.pttl(k);
const resetMs = ttlMs > 0 ? ttlMs : body.windowSeconds * 1000;

Failure paths that leave a TTL-less key:

• the process dies or the connection drops between INCR and EXPIRE;
• the EXPIRE itself errors — which surfaces as fail-open for that one request via checkRateLimitBucket's catch, and then never runs again, because count will never be 1 for that key afterwards.

The symptom is then actively masked: pttl returning -1 (no expiry) is treated identically to "no key", so resetMs falls back to a full window and the 429 keeps promising "retry in 60 s" forever.

This file has not been touched since it was created (#977/#1157) and has never been revisited.

Requirements

1. Counter increment and window establishment must be atomic.
2. A key that somehow ends up without a TTL must self-heal rather than deny forever.
3. The reported retryAfter must not claim a reset that will never happen.

Deliverables

• Make it atomic. Options, in preference order: a Lua script doing INCR + PEXPIRE NX in one round trip; SET k 0 EX w NX before INCR; or an unconditional EXPIRE k w NX (Redis ≥ 7). Note the atomic form also removes the second round trip, so this is a latency win as well.
• Self-heal: when pttl returns -1 (key exists, no expiry), set the expiry and treat the window as fresh rather than reporting a bogus reset.
• Add a counter/log for the self-heal path so a recurrence is visible instead of silent.

Tests (must fail against current main)

• Simulate an EXPIRE failure after a successful INCR ⇒ the key still acquires a TTL on the next request, and the bucket resets on schedule.
• A pre-existing TTL-less key is healed rather than denying indefinitely.
• Normal limiting behaviour is unchanged (limit, remaining, resetAt).
• Concurrent increments do not lose the window.

Related

• security(rate-limit): webhook bucket keyed on unverified installation.id — 10 unauthenticated req/min silences a target installation's webhooks #9469 — the same webhook buckets, keyed on an unverified id; that issue is how an attacker chooses which bucket to attack, this one is how a bucket can become permanently stuck on its own.

[linear-code]

JSO-6557