diff --git a/surf/limiter.go b/surf/limiter.go index b05afa4..8369686 100644 --- a/surf/limiter.go +++ b/surf/limiter.go @@ -93,8 +93,8 @@ func (l *FixedWindowLimiter) Middleware(param string) pact.Middleware { return } key := b.Key(r) - if l.store.TooManyAttempts(key, b.Max) { - retryAfter := l.store.AvailableIn(key) + allowed, attempts, retryAfter := l.store.Attempt(key, b.Max, b.Decay) + if !allowed { secs := int(retryAfter / time.Second) w.Header().Set("Content-Type", "application/json") w.Header().Set("Retry-After", strconv.Itoa(secs)) @@ -105,7 +105,6 @@ func (l *FixedWindowLimiter) Middleware(param string) pact.Middleware { _, _ = w.Write([]byte(tooManyAttemptsBody)) return } - attempts := l.store.Hit(key, b.Decay) remaining := b.Max - attempts if remaining < 0 { remaining = 0 @@ -157,11 +156,7 @@ func (l *FixedWindowLimiter) resolve(param string) (Bucket, error) { return "u:" + strconv.FormatUint(uint64(u.ID), 10) } } - host := "" - if r != nil { - host = r.Host - } - return host + "|" + ClientIP(r, trusted) + return "inline:domainless|" + ClientIP(r, trusted) }, } l.inline[param] = b diff --git a/surf/limiter_coverage_test.go b/surf/limiter_coverage_test.go index dc0dfcd..a4bbef8 100644 --- a/surf/limiter_coverage_test.go +++ b/surf/limiter_coverage_test.go @@ -12,20 +12,20 @@ import ( "git.golem15.com/golem15/summercms/compass" ) -// Gap (b): MemoryStore's sweep goroutine (purge), not TooManyAttempts' lazy -// expiry. Construct with a short sweep and assert the internal map drops -// an expired entry without calling TooManyAttempts. +// Gap (b): MemoryStore's sweep goroutine (purge), not Attempt's lazy expiry. +// Construct with a short sweep and assert the internal map drops +// an expired entry without a later Attempt. func TestMemoryStoreSweepRemovesExpiredEntry(t *testing.T) { s := NewMemoryStore(15 * time.Millisecond) t.Cleanup(func() { close(s.stop) }) - s.Hit("k", 25*time.Millisecond) + s.Attempt("k", 1, 25*time.Millisecond) s.mu.Lock() n := len(s.entries) s.mu.Unlock() if n != 1 { - t.Fatalf("after Hit, entries = %d", n) + t.Fatalf("after Attempt, entries = %d", n) } deadline := time.Now().Add(200 * time.Millisecond) @@ -41,18 +41,20 @@ func TestMemoryStoreSweepRemovesExpiredEntry(t *testing.T) { t.Fatalf("sweep did not drop expired entry, count=%d", n) } -func TestMemoryStoreAvailableInExpiredAndMissing(t *testing.T) { +func TestMemoryStoreAttemptRetryAfterAndExpiry(t *testing.T) { s := NewMemoryStore(0) - if d := s.AvailableIn("missing"); d != 0 { - t.Fatalf("missing AvailableIn = %s", d) + allowed, attempts, retryAfter := s.Attempt("k", 1, 20*time.Millisecond) + if !allowed || attempts != 1 || retryAfter <= 0 { + t.Fatalf("first attempt = allowed %v, attempts %d, retryAfter %s", allowed, attempts, retryAfter) } - s.Hit("k", 20*time.Millisecond) - if d := s.AvailableIn("k"); d <= 0 { - t.Fatalf("live AvailableIn = %s", d) + allowed, attempts, retryAfter = s.Attempt("k", 1, 20*time.Millisecond) + if allowed || attempts != 1 || retryAfter <= 0 { + t.Fatalf("denied attempt = allowed %v, attempts %d, retryAfter %s", allowed, attempts, retryAfter) } time.Sleep(30 * time.Millisecond) - if d := s.AvailableIn("k"); d != 0 { - t.Fatalf("expired AvailableIn = %s", d) + allowed, attempts, retryAfter = s.Attempt("k", 1, 20*time.Millisecond) + if !allowed || attempts != 1 || retryAfter <= 0 { + t.Fatalf("expired attempt = allowed %v, attempts %d, retryAfter %s", allowed, attempts, retryAfter) } } diff --git a/surf/limiter_store.go b/surf/limiter_store.go index 21eaeaa..a77ddc5 100644 --- a/surf/limiter_store.go +++ b/surf/limiter_store.go @@ -5,14 +5,11 @@ import ( "time" ) -// Store mirrors Illuminate\Cache\RateLimiter's hit/tooManyAttempts/ -// availableIn control flow: a fixed window, first-hit-wins (an existing -// unexpired window is never extended), with resetAttempts as a side effect -// of TooManyAttempts observing an expired window. +// Store owns fixed-window admission as one atomic operation. Attempt performs +// lazy expiry, threshold comparison, and an admitted increment together so +// concurrent callers cannot pass a split check-then-increment boundary. type Store interface { - Hit(key string, decay time.Duration) (attempts int) - TooManyAttempts(key string, max int) bool - AvailableIn(key string) time.Duration + Attempt(key string, max int, decay time.Duration) (allowed bool, attempts int, retryAfter time.Duration) } type counterEntry struct { @@ -68,48 +65,29 @@ func (s *MemoryStore) purge() { } } -// Hit increments key's counter, opening a decay window on first hit -// (first-hit-wins: an existing unexpired window is never extended). -func (s *MemoryStore) Hit(key string, decay time.Duration) int { +// Attempt admits and counts one request when key is below max. The first +// attempt opens the window; later attempts never extend it. A denied attempt +// leaves the exhausted count unchanged. +func (s *MemoryStore) Attempt(key string, max int, decay time.Duration) (bool, int, time.Duration) { s.mu.Lock() defer s.mu.Unlock() now := time.Now() e, ok := s.entries[key] - if !ok || now.After(e.resetAt) { + if ok && now.After(e.resetAt) { + delete(s.entries, key) + ok = false + } + if !ok { e = &counterEntry{count: 0, resetAt: now.Add(decay)} s.entries[key] = e } + retryAfter := e.resetAt.Sub(now) + if retryAfter < 0 { + retryAfter = 0 + } + if e.count >= max { + return false, e.count, retryAfter + } e.count++ - return e.count -} - -// TooManyAttempts is true only while count >= max and the window has not -// expired. An expired window is deleted (PHP resetAttempts) and returns false. -func (s *MemoryStore) TooManyAttempts(key string, max int) bool { - s.mu.Lock() - defer s.mu.Unlock() - e, ok := s.entries[key] - if !ok { - return false - } - if time.Now().After(e.resetAt) { - delete(s.entries, key) - return false - } - return e.count >= max -} - -// AvailableIn is the time until the window resets, or 0 if the key is absent. -func (s *MemoryStore) AvailableIn(key string) time.Duration { - s.mu.Lock() - defer s.mu.Unlock() - e, ok := s.entries[key] - if !ok { - return 0 - } - d := e.resetAt.Sub(time.Now()) - if d < 0 { - return 0 - } - return d + return true, e.count, retryAfter } diff --git a/surf/limiter_test.go b/surf/limiter_test.go index 0b55639..02961ad 100644 --- a/surf/limiter_test.go +++ b/surf/limiter_test.go @@ -129,39 +129,37 @@ func TestFixedWindowLimiterMemoryStoreWindow(t *testing.T) { s := NewMemoryStore(0) decay := 80 * time.Millisecond - if c := s.Hit("k", decay); c != 1 { - t.Fatalf("hit 1 = %d", c) + if allowed, attempts, _ := s.Attempt("k", 3, decay); !allowed || attempts != 1 { + t.Fatalf("attempt 1 = allowed %v, attempts %d", allowed, attempts) } - if c := s.Hit("k", decay); c != 2 { - t.Fatalf("hit 2 = %d", c) + if allowed, attempts, _ := s.Attempt("k", 3, decay); !allowed || attempts != 2 { + t.Fatalf("attempt 2 = allowed %v, attempts %d", allowed, attempts) } - if s.TooManyAttempts("k", 3) { - t.Fatal("TooManyAttempts at count==max-1") + if allowed, attempts, _ := s.Attempt("k", 3, decay); !allowed || attempts != 3 { + t.Fatalf("attempt 3 = allowed %v, attempts %d", allowed, attempts) } - if c := s.Hit("k", decay); c != 3 { - t.Fatalf("hit 3 = %d", c) - } - if !s.TooManyAttempts("k", 3) { - t.Fatal("TooManyAttempts at count==max") + if allowed, attempts, _ := s.Attempt("k", 3, decay); allowed || attempts != 3 { + t.Fatalf("denied attempt = allowed %v, attempts %d", allowed, attempts) } time.Sleep(decay + 20*time.Millisecond) - if s.TooManyAttempts("k", 3) { - t.Fatal("TooManyAttempts after window elapsed") - } - if c := s.Hit("k", decay); c != 1 { - t.Fatalf("fresh window hit = %d", c) + if allowed, attempts, _ := s.Attempt("k", 3, decay); !allowed || attempts != 1 { + t.Fatalf("fresh-window attempt = allowed %v, attempts %d", allowed, attempts) } } func TestFixedWindowLimiterMemoryStoreFirstHitWins(t *testing.T) { s := NewMemoryStore(0) decay := 200 * time.Millisecond - s.Hit("k", decay) + if allowed, _, _ := s.Attempt("k", 2, decay); !allowed { + t.Fatal("first attempt denied") + } time.Sleep(120 * time.Millisecond) - s.Hit("k", decay) // must not extend the original window + if allowed, _, _ := s.Attempt("k", 2, decay); !allowed { + t.Fatal("second attempt denied") + } time.Sleep(100 * time.Millisecond) - if s.TooManyAttempts("k", 1) { + if allowed, attempts, _ := s.Attempt("k", 1, decay); !allowed || attempts != 1 { t.Fatal("window was extended by a later hit") } }