perf: use batches to decrease the number of concurrent db connections, use singleflight for more db queries, use subject filter where possible, use per-request concurrency limit - #3023
Open
maxmanuylov wants to merge 13 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR converts permission checks to batch requests with per-entity results. It adds streaming lookup, configurable batching, concurrency semaphores, subject-filtered storage queries, keyed singleflight reads, and atomic consistent-hash balancer state. Batch permission execution
Lookup batching and streaming
Storage filtering and concurrency
Consistent-hash balancer state
Service wiring and command migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/storage/proxies/singleflight/data_reader_test.go`:
- Around line 385-390: The test goroutine in data_reader_test.go is calling
Ginkgo assertions inside go func() without recovery, which can make failures
escape the spec. Add defer GinkgoRecover() at the start of each goroutine that
uses Expect, including the QueryRelationships helper in the singleflight data
reader tests, so assertion failures are handled by Ginkgo instead of crashing or
flaking the suite.
In `@internal/storage/proxies/singleflight/data_reader.go`:
- Around line 33-43: Clone the materialized protobuf results before returning
from the singleflight paths in data_reader.go: the tuples returned by the
QueryRelationships flow and the attribute objects returned by
QuerySingleAttribute and QueryAttributes should be deep-copied after
draining/materializing and before constructing the final iterator or response.
Use the existing helpers around queryRelationshipsGroup and any attribute-query
singleflight logic to ensure each caller gets its own copies, preventing shared
protobuf pointers from being mutated across duplicate requests.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74726ccc-8d17-4eb3-bb5e-08eedb224cb7
📒 Files selected for processing (2)
internal/storage/proxies/singleflight/data_reader.gointernal/storage/proxies/singleflight/data_reader_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3023 +/- ##
==========================================
- Coverage 76.26% 75.64% -0.61%
==========================================
Files 83 83
Lines 9215 9712 +497
==========================================
+ Hits 7027 7346 +319
- Misses 1638 1797 +159
- Partials 550 569 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
maxmanuylov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/balancer/balancer.go (1)
185-233: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftApplication picker never recovers after a transient failure without a fresh resolver update.
b.pickeris only cleared or rebuilt insideUpdateClientConnState(lines 167-178) andResolverError(lines 55-57).UpdateSubConnStatecomputesb.statethroughRecordTransitionand callsupdateGRPCState(), but never rebuildsb.picker.If
b.pickeris cleared tonil(aggregateTransientFailure) and SubConns later recover toReadypurely throughUpdateSubConnStatetransitions,b.pickerstaysniluntil the next resolver-drivenUpdateClientConnStatecall.internal/engines/balancer/balancer.go'sCheck()falls back to the local checker wheneverbuilder.Picker()returns nil, so distributed batch routing can silently stay degraded for as long as the resolver does not push a new address list, which defeats a core goal of this PR (reducing duplicate DB load through distributed routing).For reference,
grpc-go's own base balancer regenerates its picker on every SubConn state transition insideupdateSubConnState, not only onUpdateClientConnState. This implementation diverges from that pattern.Extract the picker-rebuild logic into a shared method and call it from
UpdateSubConnStatetoo.🔧 Proposed fix: rebuild the picker on SubConn transitions
+// rebuildPicker recomputes the application-level picker from the current state. +func (b *Balancer) rebuildPicker() { + if b.state == connectivity.TransientFailure { + b.picker.Store(nil) + return + } + if b.consistent == nil || b.config == nil { + return + } + width := b.config.PickerWidth + if width < 1 { + width = 1 + } + b.picker.Store(&picker{consistent: b.consistent, width: width}) +}Then reuse it in both places:
// Update the application picker. - if b.state == connectivity.TransientFailure { - slog.Warn("Transient failure detected") - b.picker.Store(nil) - } else { - width := b.config.PickerWidth - if width < 1 { - width = 1 - } - slog.Info("Creating new picker", slog.Int("width", width)) - b.picker.Store(&picker{consistent: b.consistent, width: width}) - } + b.rebuildPicker() b.updateGRPCState()b.state = b.connectivityEvaluator.RecordTransition(oldS, s) + b.rebuildPicker() b.updateGRPCState() }🤖 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/balancer/balancer.go` around lines 185 - 233, Extract the existing picker construction and assignment logic from UpdateClientConnState into a shared Balancer method, then invoke that method from UpdateSubConnState after updating the connectivity state and before publishing the gRPC state. Reuse the shared method in UpdateClientConnState so SubConn recovery transitions rebuild b.picker without requiring a resolver update.
🧹 Nitpick comments (12)
internal/engines/bulk.go (1)
660-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sync.Onceinstead of a recovered double close.
recover()makes a second call safe, but it hides the real intent and suppresses any other panic raised inside the function. Async.Oncefield states the idempotency directly.♻️ Proposed refactor
func (bc *BulkChecker) SignalProducerDone() { - defer func() { _ = recover() }() - close(bc.producerDone) + bc.producerDoneOnce.Do(func() { + close(bc.producerDone) + }) }Add the field to
BulkChecker:// producerDoneOnce guards closing producerDone exactly once. producerDoneOnce sync.Once🤖 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 `@internal/engines/bulk.go` around lines 660 - 665, Update BulkChecker to add a producerDoneOnce sync.Once field, then change SignalProducerDone to close producerDone through producerDoneOnce.Do instead of using a deferred recover. Preserve idempotent behavior while allowing unrelated panics to propagate.internal/engines/lookup.go (1)
356-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept the common boolean spellings and name the header.
The check requires the exact value
"true". A client that sends"True"or"1"gets the standard path with no signal. Usestrconv.ParseBoolso the flag is robust. Also declare the header name as a constant, becauseinternal/engines/lookup_test.gorepeats the literal.♻️ Proposed refactor
+// SkipOrderingHeader enables unordered streaming lookup. +const SkipOrderingHeader = "x-permify-skip-ordering" + // skipOrdering checks gRPC metadata for the x-permify-skip-ordering flag. func skipOrdering(ctx context.Context) bool { md, ok := metadata.FromIncomingContext(ctx) if !ok { return false } - vals := md.Get("x-permify-skip-ordering") - return len(vals) > 0 && vals[0] == "true" + vals := md.Get(SkipOrderingHeader) + if len(vals) == 0 { + return false + } + enabled, err := strconv.ParseBool(vals[0]) + return err == nil && enabled }🤖 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 `@internal/engines/lookup.go` around lines 356 - 364, Update skipOrdering to use a named constant for the x-permify-skip-ordering metadata key, reusing that constant wherever the header is referenced. Parse the first metadata value with strconv.ParseBool so accepted boolean spellings such as "true", "True", and "1" enable skipping, while missing or invalid values continue returning false.internal/engines/lookup_test.go (1)
6284-6287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected entity IDs explicitly.
ConsistOf(standardResp.GetEntityIds())compares two results from the same build. If both paths regress to an empty result, the assertion still passes. Add an independent expectation for the standard response so the test detects a shared regression.💚 Proposed change
+ // user:1 owns doc:2 and doc:3. + Expect(standardResp.GetEntityIds()).Should(ConsistOf("2", "3")) // Same entity IDs (order may differ) Expect(streamingResp.GetEntityIds()).Should(ConsistOf(standardResp.GetEntityIds()))🤖 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 `@internal/engines/lookup_test.go` around lines 6284 - 6287, Add an independent assertion in the test around streamingResp and standardResp that verifies standardResp.GetEntityIds() against the expected entity IDs, rather than only comparing both responses to each other. Keep the existing ConsistOf comparison to validate equivalence and preserve the continuous-token assertion.internal/engines/utils.go (1)
34-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against a non-positive batch size.
Both options assign the value directly. A configuration value of
0or a negative value replaces the safe default and can produce an empty batch or a non-terminating batching loop. Fall back to the default instead.♻️ Proposed guard
func CheckMaxBatchSize(size int) CheckOption { return func(c *CheckEngine) { + if size <= 0 { + size = _defaultMaxBatchSize + } c.maxBatchSize = size } }Apply the same guard in
LookupMaxBatchSize.Also applies to: 49-53
🤖 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 `@internal/engines/utils.go` around lines 34 - 39, Update CheckMaxBatchSize and LookupMaxBatchSize to validate the supplied size before assigning it; when size is non-positive, retain or apply the existing safe default instead of storing it. Preserve positive values unchanged and ensure both option functions use the same guard behavior.internal/invoke/batch.go (2)
54-61: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Mergepanics ifr.Metadataorr.Resultsis nil.
NewBatchCheckResponsealways sets both fields, butBatchCheckResponseis also built with struct literals (for exampleinternal/engines/cache/check.golines 76-79). A literal withoutResultsproduces a nil map, and a write to a nil map panics. Add guards to make the exported method safe for all construction paths.🛡️ Proposed guard
func (r *BatchCheckResponse) Merge(other *BatchCheckResponse) { + if other == nil { + return + } + if r.Results == nil { + r.Results = make(map[string]base.CheckResult, len(other.Results)) + } for id, result := range other.Results { r.Results[id] = result } - if other.Metadata != nil { + if other.Metadata != nil && r.Metadata != nil { r.Metadata.CheckCount += other.Metadata.CheckCount } }🤖 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 `@internal/invoke/batch.go` around lines 54 - 61, Update BatchCheckResponse.Merge to safely handle responses created via struct literals: initialize r.Results before copying entries when it is nil, and initialize r.Metadata before adding other.Metadata.CheckCount when needed. Preserve the existing merge behavior for non-nil fields and avoid changing the source response.
108-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Cloneshares theMetadatapointer with the source request.
DirectInvoker.Checkmutatesrequest.Metadata.SnapTokenandrequest.Metadata.SchemaVersionin place (internal/invoke/invoke.golines 144 and 149). A clone therefore observes and can propagate those mutations.CloneWithDepthavoids this because it replacesMetadata. If any code path clones a request and then mutates metadata concurrently with the original, this becomes a data race. Consider copying the metadata message inCloneas well.🤖 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 `@internal/invoke/batch.go` around lines 108 - 122, Update BatchCheckRequest.Clone to deep-copy the Metadata message instead of reusing the source pointer, while preserving all existing metadata fields and leaving unrelated shallow-copied fields unchanged. Use the same metadata-copy approach as CloneWithDepth to ensure mutations to a clone do not affect the original request.internal/engines/balancer/balancer.go (1)
156-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the routed request timeout configurable.
Line 159 applies a fixed 4-second timeout to every routed group. A group that carries many entity IDs needs more time than a single-entity
Check, so the fixed value can turn a large batch into a deadline error. The value is also a magic number with no named constant.Derive the deadline from the incoming context when the caller already set one, and expose the fallback through
config.Distributed.🤖 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 `@internal/engines/balancer/balancer.go` around lines 156 - 161, Update the routed request setup in the goroutine around routeCtx and context.WithTimeout so it preserves an existing deadline from the incoming ctx and otherwise uses a configurable fallback from config.Distributed. Replace the hard-coded 4-second value with a named configuration field and ensure the derived timeout still applies cancellation to the routed request.internal/engines/check_test.go (1)
141-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for multi-entity batch requests.
Every migrated case wraps one entity through
invoke.NewBatchCheckRequest, soEntityIDsalways has length 1. The new per-entity merge logic incheckUnion,checkIntersection, andcheckExclusiontherefore has no direct coverage, and neither does the userset grouping and chunking incheckDirectRelation.Add cases that build
invoke.BatchCheckRequestdirectly with several entity IDs and assertresponse.Resultsper entity. Include a case where different entities resolve through different userset groups, and a case where the batch exceedsmaxBatchSize.Do you want me to generate those test cases?
🤖 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 `@internal/engines/check_test.go` around lines 141 - 154, Extend the tests in the existing batch-check coverage around Check to construct invoke.BatchCheckRequest directly with multiple EntityIDs and assert each response.Results entry. Cover per-entity merging in checkUnion, checkIntersection, and checkExclusion, userset grouping in checkDirectRelation with entities resolving through different groups, and a batch larger than maxBatchSize to exercise chunking.internal/engines/check.go (2)
318-343: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDeduplicate grouped subject IDs before chunking. Both traversal helpers append one ID per tuple, so an entity referenced by many parents appears many times in the grouped ID slice. The duplicates enlarge each
IN (...)list and inflate the chunk count without changing the result. Each site already builds anentityRef, which works as the set key.
internal/engines/check.go#L318-L343: incheckDirectRelation, skip appending tousersetGroups[key]when theentityRefis already present inusersetToParents.internal/engines/check.go#L442-L458: incheckTupleToUserSet, skip appending tosubjectsByType[s.GetType()]when theentityRefis already present insubjectToParents.🤖 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 `@internal/engines/check.go` around lines 318 - 343, The grouped subject ID lists contain duplicates when one userset entity has multiple parent entities. In internal/engines/check.go lines 318-343, update checkDirectRelation to use usersetToParents’ entityRef key as a set and append to usersetGroups only for the first occurrence, while still recording every parent in usersetToParents; in internal/engines/check.go lines 442-458, apply the same pattern in checkTupleToUserSet using subjectToParents and subjectsByType.
1085-1089: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated doc comment.
Lines 1085-1086 and lines 1087-1089 both document
checkRun, and the first block contradicts the second about the local limit. Keep one block.♻️ Proposed cleanup
-// checkRun executes a list of CheckFunctions concurrently. -// DB-level concurrency is controlled by the semaphore DataReader proxy, not here. // checkRun executes a list of CheckFunctions concurrently with a local concurrency limit. // DB-level concurrency is controlled by the semaphore DataReader proxy. // The local limit here prevents excessive goroutine fan-out and depth exhaustion.🤖 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 `@internal/engines/check.go` around lines 1085 - 1089, Remove the first duplicated doc-comment block above checkRun, keeping the later comment that documents the local concurrency limit and DB-level semaphore behavior.internal/storage/postgres/data_reader.go (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpan and log names always say "subject filter", even for unfiltered calls.
QueryRelationshipsnow delegates to this method withsubject == nil. The span name, at Line 50, and the debug logs, at Lines 53, 101, and 125, always mention "subject filter" regardless of whethersubjectis nil. Traces and logs for plain relationship queries become indistinguishable from subject-pushdown queries.Make the span name and log text conditional on
subject != nil.♻️ Proposed fix
func (r *DataReader) QueryRelationshipsWithSubjectFilter(ctx context.Context, tenantID string, filter *base.TupleFilter, subject *base.Subject, snap string, pagination database.CursorPagination) (it *database.TupleIterator, err error) { - ctx, span := internal.Tracer.Start(ctx, "data-reader.query-relationships-with-subject-filter") + spanName := "data-reader.query-relationships" + if subject != nil { + spanName = "data-reader.query-relationships-with-subject-filter" + } + ctx, span := internal.Tracer.Start(ctx, spanName) defer span.End() - // Log query operation - slog.DebugContext(ctx, "querying relationships with subject filter for tenant_id", slog.String("tenant_id", tenantID)) + // Log query operation + slog.DebugContext(ctx, "querying relationships for tenant_id", slog.String("tenant_id", tenantID), slog.Bool("subject_pushdown", subject != nil))Also applies to: 101-101, 125-125
🤖 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 `@internal/storage/postgres/data_reader.go` around lines 49 - 53, Update QueryRelationshipsWithSubjectFilter so the span name and all related debug log messages at the method entry and later query-operation points use subject-filter wording only when subject != nil, and use plain relationship-query wording otherwise; preserve the existing tenant and query context fields.internal/storage/proxies/semaphore/data_reader.go (1)
25-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplication across the 8 Acquire/Release wrappers.
Every method repeats the same acquire-defer-release-delegate pattern. Extract a small helper to centralize the semaphore logic and keep each method to a single delegate call.
♻️ Proposed helper and one example usage
+func (r *DataReader) withSemaphore(ctx context.Context, fn func() error) error { + sem := invoke.ConcurrencySemaphoreFromContext(ctx) + if err := sem.Acquire(ctx, 1); err != nil { + return err + } + defer sem.Release(1) + return fn() +} + func (r *DataReader) QueryRelationships(ctx context.Context, tenantID string, filter *base.TupleFilter, snap string, pagination database.CursorPagination) (*database.TupleIterator, error) { - sem := invoke.ConcurrencySemaphoreFromContext(ctx) - if err := sem.Acquire(ctx, 1); err != nil { - return nil, err - } - defer sem.Release(1) - return r.delegate.QueryRelationships(ctx, tenantID, filter, snap, pagination) + var it *database.TupleIterator + err := r.withSemaphore(ctx, func() (err error) { + it, err = r.delegate.QueryRelationships(ctx, tenantID, filter, snap, pagination) + return err + }) + return it, err }Apply the same pattern to the remaining 7 methods.
🤖 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 `@internal/storage/proxies/semaphore/data_reader.go` around lines 25 - 95, In DataReader, centralize the repeated semaphore Acquire/deferred Release behavior in a small helper and have QueryRelationships, QueryRelationshipsWithSubjectFilter, ReadRelationships, QuerySingleAttribute, QueryAttributes, ReadAttributes, QueryUniqueSubjectReferences, and HeadSnapshot delegate through it. Preserve each method’s existing return values and error propagation while reducing each wrapper to the helper-backed delegate invocation.
🤖 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 `@internal/engines/balancer/balancer.go`:
- Around line 174-195: Update the multi-entity path in the balancer’s BulkCheck
handling to split each node group into chunks of at most 100 items, issuing and
merging one RPC per chunk while preserving each entity’s result and error
handling. Define and reuse a shared maximum-items constant with the permission
server’s validation so both client batching and server acceptance remain
aligned.
In `@internal/engines/bulk.go`:
- Around line 403-456: Update executeBatchEntity to process unresolved entity
IDs in chunks limited by bc.config.BufferSize, rather than sending every
candidate in one BatchCheckRequest. Track allowed results and stop issuing
further batch checks once size allowed results have been found, while preserving
precomputed results, result-index mapping, denied defaults, and existing error
handling.
In `@internal/engines/cache/check.go`:
- Around line 103-108: Update the cached-results merge in the surrounding
checker method so it does not write directly to the delegate-owned cres.Results
map. Initialize or construct a local BatchCheckResponse with a writable Results
map, then merge cachedResults into it while preserving the delegate’s existing
results and return behavior.
- Around line 91-101: Update the loop handling uncachedIDs to cache only IDs
present in cres.Results. Remove the fabricated default CHECK_RESULT_DENIED path
and skip setCheckKey for missing results, while preserving the existing response
construction for returned entries.
In `@internal/engines/check.go`:
- Around line 699-723: Update the per-entity evaluation loop around prg.Eval so
CEL errors and non-boolean results mark only the current entityID as
CHECK_RESULT_DENIED, then continue evaluating the remaining entities. Remove the
batch-level returns for these entity-specific failures while preserving the
existing allowed result assignment for successful boolean evaluations.
- Around line 758-809: Remove the unsafe early-return block in the union
result-merging loop within checkUnion, including the entityIDsSeen and
deniedCount tracking that exists only to support it. Continue consuming all
function results and merging them into mergedResults before returning, so
entities introduced by later functions are preserved.
In `@internal/engines/entity_filter.go`:
- Around line 467-486: The chunk-processing loop at
internal/engines/entity_filter.go lines 467-486 must break when no IDs were
consumed, and use entrance.TargetEntrance for the entity type and relation
instead of the last tuple; apply the same len(chunk)==0 progress guard to the
loop at lines 570-583.
In `@internal/engines/lookup.go`:
- Around line 128-156: Update the streaming branch around
checker.ExecuteStreamingRequests to return an empty continuous token instead of
assigning "<unordered>" to ct, preserving ContinuousToken as a decodable opaque
cursor and allowing pagination clients to terminate. Represent unordered results
through the existing response metadata mechanism if available; do not pass a
sentinel through the request cursor path.
In `@internal/invoke/batch.go`:
- Around line 54-61: Enforce nil-safety for the batch response fields: in
internal/invoke/batch.go lines 54-61, update BatchCheckResponse.Merge to guard a
nil other response, lazily initialize r.Results, and ensure r.Metadata exists
before updating CheckCount; in internal/invoke/invoke.go lines 134-156,
initialize request.Metadata before assigning SnapToken and SchemaVersion; in
internal/engines/cache/check.go lines 103-108, allocate cres.Results before
merging cached entries.
In `@internal/invoke/invoke.go`:
- Around line 158-171: Update the CheckCount handling in the delegation path
around invoker.cc.Check and BatchCheckResponse.Merge to use one consistent
synchronization strategy instead of mixing atomic.AddInt32 with plain +=; apply
the chosen approach to every update of this counter. Before incrementing in the
delegated response path, handle a nil response.Metadata safely so a checker
response without metadata cannot panic.
In `@internal/servers/permission_server.go`:
- Around line 165-175: Update the result-mapping loop in the batch permission
handling to construct a distinct metadata message for each
`PermissionCheckResponse`, rather than assigning the shared `resp.Metadata`
pointer. Preserve each item’s individual check-count value from the original
per-item result and ensure later metadata mutations cannot affect other
responses.
- Around line 135-153: In the concurrent group batch construction within the
permission server handler, stop sharing request.GetMetadata() across
BatchCheckRequest instances. Use google.golang.org/protobuf/proto to clone the
metadata separately for each group before assigning it to batchReq.Metadata,
while preserving nil handling and the existing request values.
In `@internal/storage/proxies/semaphore/data_reader.go`:
- Around line 1-23: Remove the storage-layer dependency on internal/invoke by
relocating ConcurrencySemaphoreFromContext and WithConcurrencySemaphore to a
lower-level shared package, then update DataReader and all callers to use the
new package. Preserve the existing request-scoped semaphore behavior while
ensuring internal/invoke no longer participates in a circular dependency with
internal/storage.
In `@pkg/balancer/balancer.go`:
- Around line 96-102: In the b.consistent == nil error path, set b.state to
connectivity.TransientFailure before clearing the picker and calling
updateGRPCState(). Preserve the existing error logging, picker reset, state
publication, and returned error behavior.
In `@pkg/balancer/builder.go`:
- Around line 119-134: Update the initialization comment in builder.Build to
remove the obsolete errPicker and UpdateState claims. Describe only the actual
default initialization behavior, including that the balancer starts in
connectivity.Connecting with no SubConns and the picker is initially nil.
---
Outside diff comments:
In `@pkg/balancer/balancer.go`:
- Around line 185-233: Extract the existing picker construction and assignment
logic from UpdateClientConnState into a shared Balancer method, then invoke that
method from UpdateSubConnState after updating the connectivity state and before
publishing the gRPC state. Reuse the shared method in UpdateClientConnState so
SubConn recovery transitions rebuild b.picker without requiring a resolver
update.
---
Nitpick comments:
In `@internal/engines/balancer/balancer.go`:
- Around line 156-161: Update the routed request setup in the goroutine around
routeCtx and context.WithTimeout so it preserves an existing deadline from the
incoming ctx and otherwise uses a configurable fallback from config.Distributed.
Replace the hard-coded 4-second value with a named configuration field and
ensure the derived timeout still applies cancellation to the routed request.
In `@internal/engines/bulk.go`:
- Around line 660-665: Update BulkChecker to add a producerDoneOnce sync.Once
field, then change SignalProducerDone to close producerDone through
producerDoneOnce.Do instead of using a deferred recover. Preserve idempotent
behavior while allowing unrelated panics to propagate.
In `@internal/engines/check_test.go`:
- Around line 141-154: Extend the tests in the existing batch-check coverage
around Check to construct invoke.BatchCheckRequest directly with multiple
EntityIDs and assert each response.Results entry. Cover per-entity merging in
checkUnion, checkIntersection, and checkExclusion, userset grouping in
checkDirectRelation with entities resolving through different groups, and a
batch larger than maxBatchSize to exercise chunking.
In `@internal/engines/check.go`:
- Around line 318-343: The grouped subject ID lists contain duplicates when one
userset entity has multiple parent entities. In internal/engines/check.go lines
318-343, update checkDirectRelation to use usersetToParents’ entityRef key as a
set and append to usersetGroups only for the first occurrence, while still
recording every parent in usersetToParents; in internal/engines/check.go lines
442-458, apply the same pattern in checkTupleToUserSet using subjectToParents
and subjectsByType.
- Around line 1085-1089: Remove the first duplicated doc-comment block above
checkRun, keeping the later comment that documents the local concurrency limit
and DB-level semaphore behavior.
In `@internal/engines/lookup_test.go`:
- Around line 6284-6287: Add an independent assertion in the test around
streamingResp and standardResp that verifies standardResp.GetEntityIds() against
the expected entity IDs, rather than only comparing both responses to each
other. Keep the existing ConsistOf comparison to validate equivalence and
preserve the continuous-token assertion.
In `@internal/engines/lookup.go`:
- Around line 356-364: Update skipOrdering to use a named constant for the
x-permify-skip-ordering metadata key, reusing that constant wherever the header
is referenced. Parse the first metadata value with strconv.ParseBool so accepted
boolean spellings such as "true", "True", and "1" enable skipping, while missing
or invalid values continue returning false.
In `@internal/engines/utils.go`:
- Around line 34-39: Update CheckMaxBatchSize and LookupMaxBatchSize to validate
the supplied size before assigning it; when size is non-positive, retain or
apply the existing safe default instead of storing it. Preserve positive values
unchanged and ensure both option functions use the same guard behavior.
In `@internal/invoke/batch.go`:
- Around line 54-61: Update BatchCheckResponse.Merge to safely handle responses
created via struct literals: initialize r.Results before copying entries when it
is nil, and initialize r.Metadata before adding other.Metadata.CheckCount when
needed. Preserve the existing merge behavior for non-nil fields and avoid
changing the source response.
- Around line 108-122: Update BatchCheckRequest.Clone to deep-copy the Metadata
message instead of reusing the source pointer, while preserving all existing
metadata fields and leaving unrelated shallow-copied fields unchanged. Use the
same metadata-copy approach as CloneWithDepth to ensure mutations to a clone do
not affect the original request.
In `@internal/storage/postgres/data_reader.go`:
- Around line 49-53: Update QueryRelationshipsWithSubjectFilter so the span name
and all related debug log messages at the method entry and later query-operation
points use subject-filter wording only when subject != nil, and use plain
relationship-query wording otherwise; preserve the existing tenant and query
context fields.
In `@internal/storage/proxies/semaphore/data_reader.go`:
- Around line 25-95: In DataReader, centralize the repeated semaphore
Acquire/deferred Release behavior in a small helper and have QueryRelationships,
QueryRelationshipsWithSubjectFilter, ReadRelationships, QuerySingleAttribute,
QueryAttributes, ReadAttributes, QueryUniqueSubjectReferences, and HeadSnapshot
delegate through it. Preserve each method’s existing return values and error
propagation while reducing each wrapper to the helper-backed delegate
invocation.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0069c28-eb6c-4b32-8686-c557166dd65c
📒 Files selected for processing (36)
internal/engines/balancer/balancer.gointernal/engines/bulk.gointernal/engines/bulk_benchmark_test.gointernal/engines/bulk_test.gointernal/engines/cache/check.gointernal/engines/cache/check_test.gointernal/engines/check.gointernal/engines/check_goroutine_leak_test.gointernal/engines/check_test.gointernal/engines/entity_filter.gointernal/engines/lookup.gointernal/engines/lookup_test.gointernal/engines/subject_permission.gointernal/engines/utils.gointernal/invoke/batch.gointernal/invoke/concurrency.gointernal/invoke/invoke.gointernal/invoke/utils.gointernal/servers/permission_server.gointernal/servers/server.gointernal/storage/memory/data_reader.gointernal/storage/postgres/data_reader.gointernal/storage/postgres/gc/gc_test.gointernal/storage/postgres/utils/filter.gointernal/storage/proxies/circuitbreaker/data_reader.gointernal/storage/proxies/semaphore/data_reader.gointernal/storage/proxies/singleflight/data_reader.gointernal/storage/storage.gopkg/balancer/balancer.gopkg/balancer/balancer_test.gopkg/balancer/builder.gopkg/balancer/picker.gopkg/balancer/picker_test.gopkg/cmd/serve.gopkg/cmd/validate.gopkg/development/development.go
…lar parallel requests
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 `@integration-test/Dockerfile`:
- Around line 2-8: Update both Go base-image references in the integration-test
Dockerfile to use the project’s declared Go 1.25.7 toolchain, including the
matching Alpine tag and digest, instead of golang:1.27rc2-alpine. Keep the
modules and tests stages otherwise unchanged.
In `@internal/servers/server_behavior_test.go`:
- Around line 106-127: Update fakePermissionInvoker.Check to implement
invoke.Check: accept *invoke.BatchCheckRequest and return
*invoke.BatchCheckResponse, preserving the existing error and allowed-result
behavior. Adjust its stored request field and all call-site assertions to use
the batch request type and fields exposed by invoke.BatchCheckRequest.
In `@sdk/java/grpc/pom.xml`:
- Around line 56-61: Align the gRPC-Java dependency versions in the Maven
configuration by updating grpc-netty, grpc-core, and grpc-protobuf from 1.81.0
to 1.82.1, matching grpc-api and grpc-stub; alternatively, centralize all gRPC
versions through grpc-bom.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24d72f19-1210-4c31-a8be-f8ccf62d45c8
⛔ Files ignored due to path filters (6)
docs/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumpkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.goplayground/yarn.lockis excluded by!**/yarn.lock,!**/*.locksdk/go/grpc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
DockerfileDockerfile.localDockerfile.releasedocs/api-reference/apidocs.swagger.jsondocs/api-reference/openapi.jsondocs/api-reference/openapiv2/apidocs.swagger.jsondocs/package.jsongo.modintegration-test/Dockerfileinternal/info.gointernal/servers/server_behavior_test.gopkg/development/development.gopkg/development/file/decoder.gopkg/dsl/lexer/lexer.gopkg/dsl/lexer/lexer_test.goplayground/package.jsonproto/base/v1/openapi.protosdk/go/grpc/go.modsdk/java/grpc/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/development/development.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/servers/server_behavior_test.go`:
- Around line 343-345: Extend the assertions after the check response validation
in the relevant test to verify invoker.checkReq is non-nil and matches the
relevant fields of invoke.NewBatchCheckRequest(checkReq). Keep the existing
CHECK_RESULT_ALLOWED assertion, and compare the forwarded batch request rather
than relying solely on fakePermissionInvoker.Check’s response.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec5a7132-7a6a-444b-953e-a77e85919f8f
📒 Files selected for processing (1)
internal/servers/server_behavior_test.go
maxmanuylov
force-pushed
the
fix-concurrency
branch
from
August 3, 2026 17:54
a04c4a7 to
8e07b4f
Compare
Contributor
Author
|
@tolgaozen these fixes drastically improved the performance of our production Permify instance (Permission-service endpoints: Check, LookupEntity). Please see the attached before/after request durations. |
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.
So far many similar parallel requests lead to a lot of DB calls fetching the same tuples and competing for DB pool.
This PR uses singleflight for such requests as well to improve the performance.
Summary by CodeRabbit
New Features
Performance
Reliability