perf: improve authorino caching with coalescing concurrent requests - #668
perf: improve authorino caching with coalescing concurrent requests#668hanna836 wants to merge 1 commit into
Conversation
…r the same API key
📝 WalkthroughWalkthroughThe evaluators now coalesce concurrent cacheable authorization and metadata calls with ChangesEvaluator singleflight
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MetadataConfig
participant singleflight.Group
participant Cache
participant Evaluator
Caller->>MetadataConfig: Call with configuration and cache key
MetadataConfig->>Cache: Check cached result
MetadataConfig->>singleflight.Group: Join flight for configuration and cache key
singleflight.Group->>Cache: Recheck cached result
singleflight.Group->>Evaluator: Evaluate on cache miss
Evaluator-->>singleflight.Group: Return metadata result
singleflight.Group->>Cache: Store successful result
singleflight.Group-->>MetadataConfig: Share result with callers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/evaluators/authorization.go`:
- Around line 77-92: The flight paths in pkg/evaluators/authorization.go:77-92
and pkg/evaluators/metadata.go:73-88 must preserve each caller’s context while
coalescing requests. Update the authorization and metadata evaluator flows to
use a context-aware singleflight mechanism, derive a shared evaluation context
with an explicit timeout policy for the in-flight evaluator call, and ensure
each caller can stop waiting when its own context is canceled.
In `@pkg/evaluators/metadata_singleflight_test.go`:
- Around line 55-82: Update the metadata singleflight tests to synchronize
goroutines at a barrier before invoking concurrent cache misses. Use one shared
MetadataConfig for same-key and different-key cases, asserting one evaluation
for repeated keys and two evaluations for distinct keys; ensure the third test
also performs overlapping calls rather than a sequential invocation.
- Line 53: Update the tests around config cleanup and concurrent calls: register
cleanup through t.Cleanup and assert any returned Clean error, and capture each
Call error from the goroutines before waiting on the WaitGroup, then assert all
captured errors after wg.Wait. Apply the same error handling to the additional
Clean and Call sites identified in the diff, including the different-keys
singleflight test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 403d1999-a29b-4402-82b2-0202b8e05660
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modpkg/evaluators/authorization.gopkg/evaluators/metadata.gopkg/evaluators/metadata_singleflight_test.go
| result, err, shared := config.flight.Do(flightKey, func() (interface{}, error) { | ||
| if cachedObj, _ := cache.Get(cacheKey); cachedObj != nil { | ||
| return cachedObj, nil | ||
| } | ||
| obj, err := evaluator.Call(pipeline, log.IntoContext(ctx, logger)) | ||
| if err == nil { | ||
| if setErr := cache.Set(cacheKey, obj); setErr != nil { | ||
| logger.V(1).Info("unable to store data in the cache", "err", setErr) | ||
| } | ||
| } | ||
| return obj, err | ||
| }) | ||
| if shared { | ||
| logger.V(1).Info("singleflight: coalesced duplicate authorization call", "key", flightKey) | ||
| } | ||
|
|
||
| return obj, err | ||
| return result, err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'pkg/evaluators/*.go' | sed -n '1,120p'
printf '\n== evaluation snippets ==\n'
for f in pkg/evaluators/authorization.go pkg/evaluators/metadata.go; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,130p'
fi
done
printf '\n== singleflight call sites/context related search ==\n'
rg -n "singleflight|flight:|Do\\(|WithContext|evaluator.Call|cache\\.Get|cache\\.Set|context\\.WithCancel|context\\.WithTimeout" pkg/evaluators || true
printf '\n== module deps for singleflight package ==\n'
rg -n "golang.org/x/sync|singleflight" go.mod go.sum 2>/dev/null || true
printf '\n== local golang.org/x/sync/source check if present in vendor/cache? ==\n'
go env GOPATH GOMODCACHE 2>/dev/null || true
find "$HOME/go/pkg/mod/golang.org/x/sync"* -name 'singleflight*.go' 2>/dev/null | head -20 || trueRepository: Kuadrant/authorino
Length of output: 2395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== authorization.go relevant lines ==\n'
awk '{printf "%6d\t%s\n", NR, $0}' pkg/evaluators/authorization.go | sed -n '1,130p'
printf '\n== metadata.go relevant lines ==\n'
awk '{printf "%6d\t%s\n", NR, $0}' pkg/evaluators/metadata.go | sed -n '1,120p'
printf '\n== singleflight import and call sites ==\n'
rg -n "singleflight|flight:|Do\\(|WithContext|evaluator.Call|cache\\.Get|cache\\.Set|context\\.WithCancel|context\\.WithTimeout" pkg/evaluators || true
printf '\n== go.mod relevant deps ==\n'
awk '{if($0 ~ /golang.org|x\/sync|singleflight/ || NR<=200) print}' go.mod 2>/dev/null || true
printf '\n== candidate singleflight source if cached ==\n'
gopath=$(go env GOPATH 2>/dev/null || true)
modcache=$(go env GOMODCACHE 2>/dev/null || true)
printf "GOPATH=%s\nGOMODCACHE=%s\n" "$gopath" "$modcache"
find "${gopath:-/tmp/gopath}/pkg/mod/golang.org/x/sync"* "${modcache:-/tmp/modcache}/golang.org/x/sync"* -type f \( -name 'singleflight*.go' -o -name 'go.mod' \) 2>/dev/null | head -20 || trueRepository: Kuadrant/authorino
Length of output: 20298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== cached singleflight implementation ==\n'
awk '{printf "%6d\t%s\n", NR, $0}' /home/jailuser/go/pkg/mod/golang.org/x/sync@v0.22.0/singleflight/singleflight.go | sed -n '1,220p'
printf '\n== cached singleflight tests around cancellation/DuplicateCalls ==\n'
rg -n "Cancel|Context|WithCancel|DuplicateCalls|shared|Wait|Do(" /home/jailuser/go/pkg/mod/golang.org/x/sync@v0.22.0/singleflight/singleflight_test.go -C 4 || trueRepository: Kuadrant/authorino
Length of output: 7291
🌐 Web query:
golang.org/x/sync/singleflight Group Do Wait duplicate calls context cancellation
💡 Result:
The standard golang.org/x/sync/singleflight package does not provide native, integrated support for context cancellation within its Group.Do method [1][2]. Key behavior regarding duplicate calls and context: 1. Blocking Nature: The Do method is synchronous and blocks until the primary execution finishes [3][4]. If a caller's context is cancelled while it is waiting for a result, the Do method itself does not observe or react to that cancellation; the caller will remain blocked until the underlying function completes or panics [1][5]. 2. Risk of Propagation: If the function passed to Do uses a context derived from one of the callers, the cancellation of that specific caller can cause the entire shared operation to fail for all other waiting callers [1][5]. It is a common best practice to use context.WithoutCancel(ctx) or a background context to detach the shared work from the lifetime of any individual request [1][5]. 3. Alternatives for Cancellation: - DoChan: If you need to respect context cancellation for the waiting period, you can use DoChan [1][6]. This returns a channel, allowing you to use a select statement to wait for either the result or the cancellation of your context [1]: ch:= group.DoChan(key, fn) select { case <-ctx.Done: return nil, ctx.Err case res:= <-ch: return res.Val, res.Err } - External Libraries: If your application requires more sophisticated, context-aware duplicate suppression (e.g., where the underlying work's context is tied to the collective state of waiting callers), consider third-party implementations like resenje.org/singleflight, which are designed to handle context propagation and cancellation more gracefully [7][8][9].
Citations:
- 1: https://dev.to/gabrielanhaia/singleflight-in-go-collapsing-duplicate-work-under-load-5f9h
- 2: https://www.okuka.com/p/go-quick-start-concurrent-programming-with-single-flight/
- 3: https://github.com/golang/sync/blob/master/singleflight/singleflight.go
- 4: https://go.dev/src/internal/singleflight/singleflight.go
- 5: https://rednafi.com/go/request-coalescing/
- 6: https://pkg.go.dev/internal/singleflight@go1.26.5
- 7: https://github.com/janos/singleflight
- 8: https://pkg.go.dev/resenje.org/singleflight@v0.4.3
- 9: https://pkg.go.dev/resenje.org/singleflight
Preserve each caller context while joining a flight.
singleflight.Group.Do does not accept a context, so callers wait unconditionally while an in-flight evaluator.Call can be tied to the first caller’s request context. Use a context-aware flight path and a shared evaluation context with an explicit timeout policy in both:
pkg/evaluators/authorization.go:77-92pkg/evaluators/metadata.go:73-88
📍 Affects 2 files
pkg/evaluators/authorization.go#L77-L92(this comment)pkg/evaluators/metadata.go#L73-L88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/evaluators/authorization.go` around lines 77 - 92, The flight paths in
pkg/evaluators/authorization.go:77-92 and pkg/evaluators/metadata.go:73-88 must
preserve each caller’s context while coalescing requests. Update the
authorization and metadata evaluator flows to use a context-aware singleflight
mechanism, derive a shared evaluation context with an explicit timeout policy
for the in-flight evaluator call, and ensure each caller can stop waiting when
its own context is canceled.
| Cache: cache, | ||
| GenericHTTP: evaluator, | ||
| } | ||
| defer config.Clean(context.Background()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Handle each returned error.
errcheck reports every discarded Clean and Call result here. A failed concurrent Call can currently pass TestMetadataSingleflight_DifferentKeysDoNotCoalesce because it asserts only the HTTP call count.
Use t.Cleanup to assert cleanup errors. Store each concurrent call error and assert it after wg.Wait.
Also applies to: 123-124, 136-140, 181-181
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 53-53: Error return value of config.Clean is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/evaluators/metadata_singleflight_test.go` at line 53, Update the tests
around config cleanup and concurrent calls: register cleanup through t.Cleanup
and assert any returned Clean error, and capture each Call error from the
goroutines before waiting on the WaitGroup, then assert all captured errors
after wg.Wait. Apply the same error handling to the additional Clean and Call
sites identified in the diff, including the different-keys singleflight test.
Source: Linters/SAST tools
| const concurrency = 50 | ||
| var wg sync.WaitGroup | ||
| wg.Add(concurrency) | ||
| errors := make([]error, concurrency) | ||
| results := make([]interface{}, concurrency) | ||
|
|
||
| pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) | ||
| pipelineMock.EXPECT().GetAuthorizationJSON().AnyTimes().Return(`{}`) | ||
|
|
||
| for i := 0; i < concurrency; i++ { | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| results[idx], errors[idx] = config.Call(pipelineMock, context.Background()) | ||
| }(i) | ||
| } | ||
|
|
||
| wg.Wait() | ||
|
|
||
| for i, err := range errors { | ||
| assert.NilError(t, err, "goroutine %d returned error", i) | ||
| } | ||
|
|
||
| for i, res := range results { | ||
| assert.Assert(t, res != nil, "goroutine %d returned nil result", i) | ||
| } | ||
|
|
||
| actual := callCount.Load() | ||
| assert.Assert(t, actual == 1, "expected 1 HTTP call, got %d (singleflight did not coalesce)", actual) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the tests force overlapping cache misses.
These tests can pass if singleflight is removed. The first test does not synchronise concurrent cache misses. The second test uses separate MetadataConfig instances, so each call uses a separate flight. The third test has no concurrent call.
Use a barrier-controlled cache miss. Test two keys through one MetadataConfig. Assert one evaluation for the same key and two evaluations for different keys.
Also applies to: 110-146, 186-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/evaluators/metadata_singleflight_test.go` around lines 55 - 82, Update
the metadata singleflight tests to synchronize goroutines at a barrier before
invoking concurrent cache misses. Use one shared MetadataConfig for same-key and
different-key cases, asserting one evaluation for repeated keys and two
evaluations for distinct keys; ensure the third test also performs overlapping
calls rather than a sequential invocation.
1 participant
...
Summary by CodeRabbit
Performance Improvements
Reliability