Skip to content

fix: suppress canceled rate limit logs - #21

Merged
garrettladley merged 1 commit into
mainfrom
gml/suppress-canceled-ratelimit-logs
May 27, 2026
Merged

garrettladley merged 1 commit into
mainfrom
gml/suppress-canceled-ratelimit-logs

Conversation

@garrettladley

Copy link
Copy Markdown
Owner
  • treat context.Canceled from RateLimit store checks as a canceled request instead of a store failure
  • keep fail-closed 503 behavior and error logging for non-cancellation kv.Store errors
  • add ratelimit.outcome coverage for canceled checks
  • add a regression test proving canceled requests do not emit error-level slog records

@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR suppresses noisy error logs for rate-limit store checks that fail because the client's request was canceled rather than because the KV store itself is broken. Fail-closed 503 behavior and error logging are preserved for genuine store failures.

  • ratelimit.go: when store.Increment returns context.Canceled, the middleware now records a ratelimit.outcome=canceled span attribute and returns silently instead of logging an error and writing a 503.
  • ratelimit_test.go: adds a regression test that pre-cancels a context and asserts zero error-level log records are emitted.
  • attrs.go: extends the RateLimitOutcome enum with the new canceled value.

Confidence Score: 4/5

Safe to merge; the change narrows a single error branch in the rate-limit middleware and is covered by a targeted regression test.

The fix is small and well-scoped, and the regression test directly validates the intended behavior. The only open question is whether context.DeadlineExceeded deserves the same quiet treatment, but that is a forward-looking gap rather than a defect in what is merged here.

No files require special attention; the change is contained to the rate-limit middleware and its test.

Important Files Changed

Filename Overview
internal/middleware/ratelimit.go Adds early-return for context.Canceled from the KV store, recording a 'canceled' span attribute instead of logging a 503; non-cancellation errors retain fail-closed behavior. context.DeadlineExceeded is not similarly suppressed.
internal/middleware/ratelimit_test.go Adds regression test verifying that a pre-canceled context does not produce error-level slog records; introduces a minimal countingErrorHandler slog.Handler helper.
internal/observability/attrs.go Adds RateLimitOutcomeCanceled constant to the existing RateLimitOutcome set.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming request to /mcp] --> B{store.Increment}
    B -->|success| C{count > limit?}
    B -->|context.Canceled| D[Set span: ratelimit.outcome=canceled]
    B -->|other error| E[Set span: ratelimit.outcome=store_error - HTTP 503]
    C -->|yes| F[HTTP 429 + Retry-After]
    C -->|no| G[ratelimit.outcome=allowed - next.ServeHTTP]
Loading

Reviews (1): Last reviewed commit: "fix: suppress canceled rate limit logs" | Re-trigger Greptile

@garrettladley
garrettladley merged commit b44ca2d into main May 27, 2026
3 checks passed
@garrettladley
garrettladley deleted the gml/suppress-canceled-ratelimit-logs branch May 27, 2026 19:47
Comment on lines +44 to +47
if errors.Is(err, context.Canceled) {
trace.SpanFromContext(r.Context()).SetAttributes(observability.RateLimitAttrs{Outcome: observability.RateLimitOutcomeCanceled, Limit: cfg.Requests, Window: cfg.Window}.Attributes()...)
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The cancellation guard only covers context.Canceled, but context.DeadlineExceeded surfaces through the same path when a client-set deadline expires before the store responds. Both represent the client's request lifecycle ending, not a store malfunction. Without this, a deadline-exceeded error from store.Increment would still be logged at error level and return a 503 to a client that has already gone. Consider broadening the check to errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant