Conversation
There was a problem hiding this comment.
Pull request overview
Adds a small in-memory cache around fmsgid lookups (plus a dedicated HTTP client with a timeout) to reduce repeated upstream calls during JWT auth.
Changes:
- Introduces a dedicated
http.Clientwith a 5s timeout for fmsgid requests. - Adds a TTL-based in-memory cache for successful (200) fmsgid responses.
- Updates
checkFmsgIDto consult/store cache entries around the network call.
| var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = addr | ||
|
|
||
| // checkFmsgID queries the fmsgid service for a user address. | ||
| // Returns (statusCode, acceptingNew, error). | ||
| // Returns (statusCode, acceptingNew, error). Successful 200 responses are | ||
| // cached for fmsgIDCacheTTL to avoid hammering fmsgid when a browser fires | ||
| // many concurrent requests with the same JWT. | ||
| func checkFmsgID(idURL, addr string) (int, bool, error) { | ||
| cacheKey := idURL + "|" + addr | ||
| if v, ok := fmsgIDCache.Load(cacheKey); ok { | ||
| entry := v.(fmsgIDEntry) | ||
| if time.Now().Before(entry.expires) { | ||
| return entry.code, entry.acceptingNew, nil | ||
| } | ||
| fmsgIDCache.Delete(cacheKey) | ||
| } |
There was a problem hiding this comment.
fmsgIDCache entries are only deleted on cache hits after expiry. If an address is looked up once and never again, the expired entry will remain in the sync.Map indefinitely, so the cache can grow without bound over the process lifetime. Consider adding periodic cleanup (e.g., a ticker that Ranges and deletes expired entries, similar to the cleanup loop in middleware/ratelimit.go) or switching to a bounded LRU/TTL cache implementation.
| func checkFmsgID(idURL, addr string) (int, bool, error) { | ||
| cacheKey := idURL + "|" + addr | ||
| if v, ok := fmsgIDCache.Load(cacheKey); ok { | ||
| entry := v.(fmsgIDEntry) |
There was a problem hiding this comment.
cacheKey := idURL + "|" + addr relies on a string delimiter, which is easy to get wrong and can theoretically collide. Using a comparable struct key (e.g., {idURL, addr}) avoids delimiter issues and also makes it clearer what dimensions are being cached.
| url := strings.TrimRight(idURL, "/") + "/fmsgid/" + addr | ||
| resp, err := http.Get(url) //nolint:gosec // URL constructed from trusted config + validated addr | ||
| resp, err := fmsgIDClient.Get(url) //nolint:gosec // URL constructed from trusted config + validated addr | ||
| if err != nil { |
There was a problem hiding this comment.
addr is concatenated directly into the URL path. IsValidAddr currently allows characters like '/', '?', or '#' (it only checks leading '@' and another '@'), so a crafted sub could change the requested path/query on the fmsgid service. Please build the URL using url.JoinPath (or escape the path segment) and/or tighten IsValidAddr to the exact allowed character set so the gosec suppression is justified.
| // Returns (statusCode, acceptingNew, error). Successful 200 responses are | ||
| // cached for fmsgIDCacheTTL to avoid hammering fmsgid when a browser fires | ||
| // many concurrent requests with the same JWT. | ||
| func checkFmsgID(idURL, addr string) (int, bool, error) { | ||
| cacheKey := idURL + "|" + addr | ||
| if v, ok := fmsgIDCache.Load(cacheKey); ok { | ||
| entry := v.(fmsgIDEntry) | ||
| if time.Now().Before(entry.expires) { | ||
| return entry.code, entry.acceptingNew, nil | ||
| } | ||
| fmsgIDCache.Delete(cacheKey) | ||
| } |
There was a problem hiding this comment.
The comment says caching avoids hammering fmsgid "when a browser fires many concurrent requests", but this implementation does not deduplicate concurrent cache misses—multiple goroutines can still call fmsgid at once before the first response is stored. If reducing burst load is a goal, consider adding per-key request coalescing (e.g., singleflight.Group) so concurrent lookups share one upstream request.
| fmsgIDCache.Store(cacheKey, fmsgIDEntry{ | ||
| expires: time.Now().Add(fmsgIDCacheTTL), | ||
| code: http.StatusOK, | ||
| acceptingNew: result.AcceptingNew, | ||
| }) | ||
| return http.StatusOK, result.AcceptingNew, nil | ||
| } |
There was a problem hiding this comment.
This PR introduces new caching behavior for fmsgid lookups, but there isn't a test asserting the cache is actually used (e.g., that a second request with the same addr/idURL does not trigger a second upstream HTTP call, and that non-200 responses are not cached). Since src/middleware/jwt_test.go already covers fmsgid interactions, consider adding a test that counts requests or makes the second upstream call fail to verify caching.
| acceptingNew bool | ||
| } | ||
|
|
||
| var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = addr |
There was a problem hiding this comment.
The inline comment on fmsgIDCache says the key is addr, but the implementation keys by idURL + "|" + addr. Please update the comment to match reality (and ideally clarify the key format) to avoid misleading future maintainers.
| var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = addr | |
| var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = idURL + "|" + addr |
No description provided.