Bugs#113
Merged
Merged
Conversation
Add BUGS.md with detailed progress tracker for 20 bugs identified during a comprehensive pkg/ audit. Covers confirmed reproductions and inspection findings with fix descriptions and regression test references to preserve context on rate-limiting, middleware, concurrency, and configuration fixes.
Embed baseResponseWriter by value in recoveryResponseWriter instead of by pointer. This eliminates a separate heap allocation per request, reducing the recovery middleware wrapper to a single allocation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 97.57% 98.47% +0.90%
==========================================
Files 18 18
Lines 1935 2098 +163
==========================================
+ Hits 1888 2066 +178
+ Misses 35 25 -10
+ Partials 12 7 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- scontext: allocate the Flags map lazily in WithFlag instead of eagerly in NewSRouterContext; every request was paying for a map it almost never uses (WithFlag/GetFlag/clone already handle a nil map) - router: embed baseResponseWriter by value in the pooled metricsResponseWriter so reinitializing it no longer allocates per request - router/middleware: skip the http.Request clone in the trace middleware, auth middleware, and ClientIPMiddleware when an SRouterContext already exists - the With* helpers mutate it in place and return the same context, so the clone was pure overhead on every routed request - router: drop the dead trace-ID re-set in authenticateRequest (the trace ID lives on the same shared SRouterContext and WithTraceID never overwrites) - router: build the request summary log fields with exact capacity instead of growing the baseFields slice - router: scan X-Forwarded-For from the right without strings.Split, and remove a redundant WithCORSInfo + request clone on the disallowed-origin CORS path - middleware: build the rate limiter composite key with strconv instead of fmt.Sprintf - add BenchmarkInstrumentedAuthRoute covering the trace + summary logging + auth + X-Forwarded-For path On the new instrumented benchmark: 38 -> 31 allocs/op, 5754 -> 3929 B/op (-32%), ~23% less CPU. Existing benchmarks drop one alloc per request. https://claude.ai/code/session_01T2Vm5Rr6NbsHdxzRzVeL76 Co-authored-by: Claude <noreply@anthropic.com>
Cover the remaining uncovered branches of the modified code: - scontext: every getter returns its zero value and not-found when the context has no SRouterContext, and when values were never set - metrics: Handler applies DefaultTags to all emitted metrics (skipping empty values) and reuses cached per-route metric instances across requests instead of rebuilding them - middleware/ratelimit: sliding-window rollover semantics (boundary bursts stay limited via weighted previous window, full reset after an idle gap), non-positive limit denies everything, non-positive window defaults to one second - middleware/trace: inbound X-Trace-ID validation (character classes, length bound) with valid IDs propagated and invalid ones replaced by a generated valid ID - middleware + router timeout writers: a started response is never overwritten by the 408, and late handler writes are rejected both before and after acquiring the writer lock - router: MetricsConfig.MiddlewareFactory takes precedence and receives the ServiceName, getEffectiveRateLimit adapts UserIDFromUser / UserIDToString across the [any,any] conversion (including type mismatch degrading to the zero ID), and blank X-Forwarded-For entries fall back to RemoteAddr The only modified lines left uncovered are defensive clamps in ratelimit.go that are mathematically unreachable and a two-line CAS race window in the timeout middleware that cannot be exercised deterministically. https://claude.ai/code/session_01K1jbTLDqeuC16p7eJTWJaN Co-authored-by: Claude <noreply@anthropic.com>
…#116) Cover the six lines Codecov flagged on the patch: - Remove provably-dead clamps in slidingWindowLimiter.allow: after the window rollover, elapsed is always within [0, window), so reset is always positive; and remaining is never negative when estimated is below the limit. - Drop the error return from extractUserKey, which could never return a non-nil error, along with the unreachable error-handling branch in the StrategyUser case. - Extract the timed-out transition in the timeout middleware into mutexResponseWriter.markTimedOut so the race-window branch (a handler write claiming the response between the wroteHeader check and the timed-out transition) is unit-testable; behavior is unchanged. - Add tests for the fmt.Stringer branch of convertUserIDToString, the unknown-strategy fallback using the context client IP, the user-object fallthrough when UserIDFromUser is nil, and both markTimedOut branches. https://claude.ai/code/session_013sdsD6aw5dnaZhZXexg882 Co-authored-by: Claude <noreply@anthropic.com>
Introduce pkg/router/e2e_test.go to validate the router through a real TCP stack using httptest.NewServer and http.Client, rather than in-memory ServeHTTP calls. Covers trace IDs, global/sub-router middleware, bearer-token auth, generic JSON routes, IP-based rate limiting (including Retry-After), and CORS preflight handling.
Rewrite TestE2ECORS to mirror how a real browser handles CORS, including same-origin requests without Origin headers, stricter preflight assertions (204 NoContent, Allow-Headers, Max-Age), and authenticated cross-origin requests to a protected endpoint using credentials.
…iction Bound the `limiters` map in UberRateLimiter, which previously accumulated one entry per unique key forever (memory leak / DoS vector). Entries idle for at least two of their own windows are now swept by a short-lived goroutine triggered only on entry creation, guaranteeing at most one sweep per minute and zero hot-path overhead for stable key sets. Resolve sweeper-vs-request races with an `evicted` tombstone and re-fetch retry, and harden `allowLocked` against negative elapsed times. Update BUGS.md to 21/21 fixed and add comprehensive regression tests (staleness, survivor counts, chaos, hot-path benchmarks).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.