Skip to content

fix: suppress deadline rate limit logs - #22

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

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

Conversation

@garrettladley

Copy link
Copy Markdown
Owner
  • treat context.DeadlineExceeded from RateLimit store checks as an ended request
  • route both cancellation sentinels through isContextEnded
  • keep kv.Store failures on the existing error logging and 503 path
  • extend rate limit middleware coverage for canceled and deadline-exceeded checks

@garrettladley
garrettladley enabled auto-merge (squash) May 27, 2026 19:50
@garrettladley
garrettladley merged commit d0ad99a into main May 27, 2026
3 checks passed
@garrettladley
garrettladley deleted the gml/suppress-deadline-ratelimit-logs branch May 27, 2026 19:51
@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extends the rate-limit middleware to treat context.DeadlineExceeded the same as context.Canceled — silently returning without logging a store error — by extracting a small isContextEnded helper. The test for this path is expanded into a table-driven form that exercises both sentinel errors.

  • isContextEnded helper (ratelimit.go): replaces the single context.Canceled check with a two-predicate helper that also matches context.DeadlineExceeded, keeping kv.Store failures on the existing error-log/503 path.
  • Test expansion (ratelimit_test.go): the single canceled-request test becomes two parallel subtests; the store stub now returns the error directly rather than through ctx.Err(), which is sufficient given the middleware only inspects the returned error value.

Confidence Score: 4/5

Safe to merge; the change correctly prevents noisy logs and 503s for requests whose contexts have ended before the store responds.

Both context.Canceled and context.DeadlineExceeded are now emitted under the single RateLimitOutcomeCanceled span attribute, which merges two distinct failure causes in telemetry. This won't affect runtime behavior, but deadline-exceeded events become indistinguishable from client disconnections in dashboards.

internal/middleware/ratelimit.go — the span attribute choice at line 45 is worth a second look.

Important Files Changed

Filename Overview
internal/middleware/ratelimit.go Introduces isContextEnded helper to handle both context.Canceled and context.DeadlineExceeded; both errors are tagged with RateLimitOutcomeCanceled in telemetry, which conflates two distinct failure modes.
internal/middleware/ratelimit_test.go Test refactored to a table-driven structure covering both sentinel errors; subtests are correctly parallelized and loop-variable capture is safe under Go 1.22+ semantics.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[RateLimit middleware] --> B{path == /mcp?}
    B -- No --> C[Skipped outcome → next handler]
    B -- Yes --> D[store.Increment]
    D --> E{err != nil?}
    E -- No --> F{count > limit?}
    F -- No --> G[Allowed outcome → next handler]
    F -- Yes --> H[Limited outcome → 429]
    E -- Yes --> I{isContextEnded?}
    I -- "Canceled or DeadlineExceeded" --> J["RateLimitOutcomeCanceled span attr → return (no response)"]
    I -- Other error --> K[StoreError outcome → log + 503]
Loading

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

Comment on lines +44 to 47
if isContextEnded(err) {
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 DeadlineExceeded mapped to canceled outcome in telemetry

Both context.Canceled and context.DeadlineExceeded flow through the same RateLimitOutcomeCanceled span attribute. In practice these represent meaningfully different situations — a client disconnect vs. a request timeout — so dashboards and alerts that bucket on ratelimit.outcome = "canceled" will silently absorb deadline-exceeded events. Consider adding a RateLimitOutcomeDeadlineExceeded constant (parallel to the existing canceled one) and branching on the specific sentinel so each cause is independently observable.

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