Code Review of a Go Token Bucket Rate Limiter Used as HTTP Middleware

In a code review round you are given a pull request and asked to review it as you would for a teammate: find what is wrong, decide what blocks the merge, and explain how you would fix and test it. The reported exercise was a token bucket rate limiter written in Go. Its code is not reproduced in the report, so practice with the stand-in below, written for this purpose. It is meant to be used as HTTP middleware in a Go web service, giving each client a burst of up to `capacity` requests that refills at `refillPerSec` tokens per second. ```go package ratelimit import ( "net/http" "time" ) // Limiter gives each client a burst of capacity requests, // refilled at refillPerSec tokens per second. type Limiter struct { capacity int refillPerSec int buckets map[string]*bucket } type bucket struct { tokens int lastRefill time.Time } func NewLimiter(capacity, refillPerSec int) *Limiter { return &Limiter{capacity: capacity, refillPerSec: refillPerSec} } // Allow reports whether one more request from key may proceed. func (l *Limiter) Allow(key string) bool { b, ok := l.buckets[key] if !ok { b = &bucket{tokens: l.capacity, lastRefill: time.Now()} l.buckets[key] = b } now := time.Now() elapsed := now.Sub(b.lastRefill) b.tokens += int(elapsed.Seconds()) * l.refillPerSec b.lastRefill = now if b.tokens > 0 { b.tokens-- return true } return false } // Middleware rejects requests from clients that are over their limit. func (l *Limiter) Middleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !l.Allow(r.RemoteAddr) { w.WriteHeader(http.StatusTooManyRequests) } next.ServeHTTP(w, r) }) } ``` ### Constraints and Clarifications - Assume Go 1.21 or later and the standard library only. - The middleware wraps handlers of a `net/http` server. - Review the code as submitted; you may propose any change to it, including to its exported API. ### Clarifying Questions - Should the limit apply per client address, per authenticated user, or per API key? - Is the limit meant per server instance or across all replicas of the service? - Does traffic reach the service directly from clients, or through a load balancer or proxy? - Roughly how many distinct clients does the service see, and can some of them be hostile? ### Part 1 — Review the code Find the defects, classify each as blocking or non-blocking with a reason, and write the review comments you would leave, ending with a verdict. ```hint Trace a fresh limiter Walk the very first request through a Limiter fresh from NewLimiter, one statement at a time, then follow a client that keeps sending requests at a steady pace. ``` #### What This Part Should Cover - Behavior at runtime once the server handles real traffic - Whether the token arithmetic honors the limiter's contract under realistic call patterns - Middleware control flow and the choice of client key - Resource growth, configuration safety, and a clear verdict ### Part 2 — Fix and test it Write or describe the corrected limiter and middleware, then the tests you would require before approving. ```hint Control time Decide how the tests can move time forward without sleeping. ``` #### What This Part Should Cover - A corrected limiter and middleware that resolve every blocking finding - Correct behavior under production traffic and long uptime - Tests that would fail on the original code and control time instead of sleeping - The limits of a per-instance limiter ### What a Strong Answer Covers - Ranks findings by their impact on the running service, ahead of style issues - Checks the token math against concrete call patterns rather than just reading it - Questions the key choice and what happens behind a proxy - Separates blocking issues from suggestions and gives a clear verdict - Proposes a test that would have caught each blocking defect ### Follow-up Questions - The service runs on many replicas and the limit must be global. What changes, and what happens when the shared store is unavailable? - How would you tell a limited client when it can retry? - At peak traffic, profiling shows the limiter has become the slowest step of every request. How would you make it cheaper?