From 76d331248b36bcdd43fe0b47da665c326f2dea3c Mon Sep 17 00:00:00 2001 From: Glenn Thompson Date: Thu, 9 Apr 2026 08:04:26 +0100 Subject: [PATCH] Replace r-simple-rate monkey-patches with fixed-window-check The previous approach overrode simple-rate::tax-rate and rate:left directly, but Radiance reloads r-simple-rate at startup and clobbers the overrides. New approach: define-page-with-limit and define-api-with-limit now call fixed-window-check directly, bypassing rate:with-limitation entirely. The fixed-window logic is self-contained and immune to module reload ordering. --- limiter.lisp | 134 ++++++++++++++++++++++++--------------------------- 1 file changed, 64 insertions(+), 70 deletions(-) diff --git a/limiter.lisp b/limiter.lisp index 9c65fec..efba535 100644 --- a/limiter.lisp +++ b/limiter.lisp @@ -1,10 +1,10 @@ ;;;; limiter.lisp - Rate limiter definitions for the application ;;;; -;;;; Includes monkey-patches for r-simple-rate's sliding-window bug: -;;;; upstream tax-rate updates the timestamp on EVERY request, which -;;;; prevents the window from ever resetting while polling is active. -;;;; Our overrides use a proper fixed window — the timestamp is only -;;;; updated when the window expires and the counter resets. +;;;; Replaces r-simple-rate's with-limitation with a fixed-window +;;;; implementation. The upstream tax-rate updates the timestamp on +;;;; EVERY request, preventing the window from ever resetting while +;;;; polling is active. Our define-*-with-limit macros bypass +;;;; rate:with-limitation entirely and call fixed-window-check instead. (in-package :asteroid) @@ -21,55 +21,49 @@ (error (e) (l:warn :rate-limiter "Failed to cleanup rate limits: ~a" e)))) -;;; ——— r-simple-rate fixed-window overrides ——— +;;; ——— Fixed-window rate limiter ——— +;;; +;;; r-simple-rate has a sliding-window bug: tax-rate updates the timestamp +;;; on every request, so the window never resets while polling is active. +;;; Rather than monkey-patching (Radiance may reload the module and clobber +;;; our overrides), we implement our own fixed-window logic directly. -(defun simple-rate::tax-rate (limit &key (ip (remote *request*))) - "Fixed-window version of tax-rate. - Only updates the timestamp when the window resets, not on every request. - This prevents the sliding-window bug where continuous polling starves - the counter because the reset condition never triggers." - (let* ((limit (simple-rate::limit limit)) +(defun fixed-window-check (limit-name max-requests timeout-seconds) + "Check and tax a fixed-window rate limit. Returns T if the request + is allowed, or (VALUES NIL seconds-remaining) if rate-limited. + LIMIT-NAME is a string key, MAX-REQUESTS and TIMEOUT-SECONDS are integers. + Uses the SIMPLE-RATE/TRACKING table for storage." + (let* ((ip (remote *request*)) (tracking (dm:get-one 'simple-rate::tracking - (db:query (:and (:= 'limit (simple-rate::name limit)) - (:= 'ip ip)))))) - (cond (tracking - ;; If the window has expired, reset counter AND timestamp - (when (<= (+ (dm:field tracking "time") - (simple-rate::timeout limit)) - (get-universal-time)) - (setf (dm:field tracking "amount") (simple-rate::amount limit)) - (setf (dm:field tracking "time") (get-universal-time))) - ;; Tax it (do NOT touch timestamp here — fixed window) - (decf (dm:field tracking "amount")) - (dm:save tracking)) - (t - (db:insert 'simple-rate::tracking - `((limit . ,(simple-rate::name limit)) - (time . ,(get-universal-time)) - (amount . ,(simple-rate::amount limit)) - (ip . ,ip))))))) - -(defun rate:left (limit &key (ip (remote *request*))) - "Fixed-window version of rate:left. - Returns correct remaining amount even for expired windows, so that - with-limitation does not block on stale tracking entries." - (let* ((limit (simple-rate::limit limit)) - (tracking (dm:get-one 'simple-rate::tracking - (db:query (:and (:= 'limit (simple-rate::name limit)) - (:= 'ip ip)))))) - (if tracking - (let ((window-end (+ (dm:field tracking "time") - (simple-rate::timeout limit))) - (now (get-universal-time))) - (if (<= window-end now) - ;; Window expired — report full budget - (values (simple-rate::amount limit) 0) - ;; Window still active - (values (dm:field tracking "amount") - (- window-end now)))) - ;; No tracking entry yet — full budget - (values (simple-rate::amount limit) - (simple-rate::timeout limit))))) + (db:query (:and (:= 'limit limit-name) + (:= 'ip ip))))) + (now (get-universal-time))) + (cond + ;; Existing entry + (tracking + (let ((window-end (+ (dm:field tracking "time") timeout-seconds))) + (when (<= window-end now) + ;; Window expired — reset counter and start new window + (setf (dm:field tracking "amount") max-requests) + (setf (dm:field tracking "time") now) + (setf window-end (+ now timeout-seconds))) + ;; Check budget + (if (<= (dm:field tracking "amount") 0) + ;; Exhausted — report time remaining + (values nil (- window-end now)) + ;; Allowed — decrement and save + (progn + (decf (dm:field tracking "amount")) + (dm:save tracking) + t)))) + ;; First request ever from this IP for this limit + (t + (db:insert 'simple-rate::tracking + `((limit . ,limit-name) + (time . ,now) + (amount . ,(1- max-requests)) + (ip . ,ip))) + t)))) (define-trigger db:connected () "Clean up any corrupted rate limit entries on startup" @@ -99,31 +93,31 @@ (defmacro define-page-with-limit (name uri options &body body) - "Rate limit for a page route. Defaults to 30 requests per minute." + "Rate limit for a page route. Defaults to 30 requests per minute. + Uses fixed-window rate limiting (not r-simple-rate's sliding window)." (multiple-value-bind (limit timeout group rest) (extract-limit-options options) (let* ((limit-name (string-upcase (format nil "~a-route-limit" (or group name)))) - (limit-sym (intern limit-name)) (limit (or limit 30)) (timeout (or timeout 60))) - `(eval-when (:compile-toplevel :load-toplevel :execute) - (rate:define-limit ,limit-sym (time-left :limit ,limit :timeout ,timeout) - ;; (format t "Route limit '~a' hit. Wait ~a seconds and retry.~%" ,(string name) time-left) - (render-rate-limit-error-page)) - (define-page ,name ,uri ,rest - (rate:with-limitation (,limit-sym) - ,@body)))))) + `(define-page ,name ,uri ,rest + (multiple-value-bind (allowed time-left) + (fixed-window-check ,limit-name ,limit ,timeout) + (declare (ignorable time-left)) + (if allowed + (progn ,@body) + (render-rate-limit-error-page))))))) (defmacro define-api-with-limit (name args options &body body) - "Rate limit for api routes. Defaults to 60 requests per minute." + "Rate limit for api routes. Defaults to 60 requests per minute. + Uses fixed-window rate limiting (not r-simple-rate's sliding window)." (multiple-value-bind (limit timeout group rest) (extract-limit-options options) (let* ((limit-name (string-upcase (format nil "~a-api-limit" (or group name)))) - (limit-sym (intern limit-name)) (limit (or limit 60)) (timeout (or timeout 60))) - `(eval-when (:compile-toplevel :load-toplevel :execute) - (rate:define-limit ,limit-sym (time-left :limit ,limit :timeout ,timeout) - ;; (format t "API Rate limit '~a' hit. Wait ~a seconds and retry.~%" ,(string name) time-left) - (api-limit-error-output)) - (define-api ,name ,args ,rest - (rate:with-limitation (,limit-sym) - ,@body)))))) + `(define-api ,name ,args ,rest + (multiple-value-bind (allowed time-left) + (fixed-window-check ,limit-name ,limit ,timeout) + (declare (ignorable time-left)) + (if allowed + (progn ,@body) + (api-limit-error-output)))))))