Skip to content

fix: split rate limit deadline outcome - #23

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

garrettladley merged 1 commit into
mainfrom
gml/separate-ratelimit-deadline-outcome

Conversation

@garrettladley

Copy link
Copy Markdown
Owner
  • add a distinct deadline_exceeded value for ratelimit.outcome
  • map context.Canceled and context.DeadlineExceeded through separate RateLimitOutcome values
  • keep both request-lifecycle errors off the kv.Store failure log path
  • add focused coverage for rateLimitContextOutcome classification

@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR splits the previously unified canceled rate-limit outcome into two distinct values — canceled for context.Canceled and deadline_exceeded for context.DeadlineExceeded — giving operators finer-grained observability on the ratelimit.outcome span attribute without changing any error-handling behavior.

  • Replaces the boolean isContextEnded helper with rateLimitContextOutcome, which returns the concrete RateLimitOutcome value directly, eliminating the hardcoded RateLimitOutcomeCanceled at the call site.
  • Adds RateLimitOutcomeDeadline = "deadline_exceeded" to attrs.go alongside the existing outcome constants.
  • Covers the new classification logic with a focused parallel table-driven test for all three cases (canceled, deadline_exceeded, and non-context errors).

Confidence Score: 5/5

Safe to merge — the change is purely additive, only affects span attribute values, and all existing error-handling behavior is preserved.

The refactor is well-scoped: a new string constant, a small function replacing a boolean helper, and tests that cover all three branches including the zero-value default. No request-lifecycle behaviour changes, no new failure modes, and the existing integration tests still cover the middleware end-to-end.

No files require special attention.

Important Files Changed

Filename Overview
internal/observability/attrs.go Adds RateLimitOutcomeDeadline = "deadline_exceeded" constant alongside the existing canceled/store_error outcomes — clean, isolated addition.
internal/middleware/ratelimit.go Replaces isContextEnded bool helper with rateLimitContextOutcome returning the specific outcome value; logic is correct, uses errors.Is for wrapping, and the early-return (no HTTP response) on context errors is preserved intentionally.
internal/middleware/ratelimit_test.go Adds TestRateLimitContextOutcome covering canceled, deadline_exceeded, and a non-context error; test for the "other" case correctly validates both ok=false and outcome="" zero value.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[store.Increment returns error] --> B{rateLimitContextOutcome}
    B -->|errors.Is Canceled| C[outcome = canceled]
    B -->|errors.Is DeadlineExceeded| D[outcome = deadline_exceeded]
    B -->|other| E[outcome = store_error]
    C --> F[SetAttributes on span
return — no HTTP response]
    D --> F
    E --> G[SetAttributes on span
log error
503 Service Unavailable]
Loading

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

@garrettladley
garrettladley merged commit 9424b06 into main May 27, 2026
3 checks passed
@garrettladley
garrettladley deleted the gml/separate-ratelimit-deadline-outcome branch May 27, 2026 19:57
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