test: improve memory management in engine check functions#2323
test: improve memory management in engine check functions#2323
Conversation
WalkthroughThe changes optimize internal slice allocations and update error handling in the permission check engine. They also introduce a comprehensive test suite to detect goroutine leaks, verify context cancellation handling, and ensure proper concurrency management during permission checks, using behavior-driven tests with Ginkgo and Gomega. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant DB as InMemoryDB
participant Engine as PermissionCheckEngine
participant Invoker as DirectInvoker
participant GoRuntime
TestSuite->>DB: Setup schema and test data
TestSuite->>GoRuntime: Record initial goroutine count
loop Concurrent Checks
TestSuite->>Invoker: Invoke permission check (concurrent)
Invoker->>Engine: Check permission
Engine-->>Invoker: Permission result
Invoker-->>TestSuite: Result
end
TestSuite->>GoRuntime: Wait for goroutine cleanup
TestSuite->>GoRuntime: Record final goroutine count
TestSuite->>TestSuite: Assert no significant goroutine leaks
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/engines/check.go (1)
613-629: Critical: CEL evaluation errors should not be silently converted to denials.The change at line 629 suppresses the error from CEL evaluation and returns a denial instead. This is problematic because:
- It hides actual evaluation errors (syntax errors, type mismatches, runtime errors)
- It makes debugging difficult as administrators won't know why the evaluation failed
- It conflates "permission denied" with "evaluation error"
The error handling at line 615 was more appropriate as it provided context about the failure.
Consider reverting this change to maintain proper error propagation:
- return denied(emptyResponseMetadata()), nil + return denied(emptyResponseMetadata()), err
🧹 Nitpick comments (3)
internal/engines/check.go (2)
289-289: Consider documenting the rationale for the fixed capacity of 4.While preallocating capacity is a good optimization, the magic number 4 seems arbitrary. Consider either:
- Adding a comment explaining why 4 was chosen (e.g., based on profiling data)
- Using a named constant for better maintainability
- Making it configurable if this is a tuning parameter
- checkFunctions := make([]CheckFunction, 0, 4) + // Preallocate with capacity 4 based on typical relationship count + const initialCheckFunctionsCap = 4 + checkFunctions := make([]CheckFunction, 0, initialCheckFunctionsCap)
369-369: Apply the same capacity constant here for consistency.This uses the same magic number 4 as in
checkDirectRelation. If you implement a named constant as suggested above, apply it here as well for consistency.internal/engines/check_goroutine_leak_test.go (1)
258-385: Consider strengthening the test assertions.While the test validates concurrency limits and goroutine management well, the assertions could be more comprehensive:
- The test collects
errorCountbut doesn't validate it- Only checking that
successCount > 0is weak - all requests should completeExpect(successCount).Should(BeNumerically(">", 0)) + // Ensure all requests completed (either success or error) + Expect(successCount + errorCount).Should(Equal(numRequests)) + // If there are errors, they should be context-related (timeout/cancellation) + if errorCount > 0 { + GinkgoWriter.Printf("Completed with %d successes and %d errors\n", successCount, errorCount) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/engines/check.go(8 hunks)internal/engines/check_goroutine_leak_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
🔇 Additional comments (5)
internal/engines/check.go (3)
226-226: Good performance optimization!Preallocating the slice capacity to match
len(children)avoids unnecessary reallocations during the append operations in the loop below.
544-545: Good modernization to Go 1.18+ conventions.Using
anyinstead ofinterface{}improves readability while maintaining the same functionality.
663-663: Go 1.22+ Syntax Supported
The project’s go.mod specifies Go 1.24, so thefor range len(functions)syntax is fully supported. No changes needed.internal/engines/check_goroutine_leak_test.go (2)
24-167: Well-structured goroutine leak test with appropriate stabilization.The test effectively validates that concurrent permission checks don't leak goroutines. The use of
runtime.GC()and sleep intervals helps ensure accurate goroutine counts.Consider making the tolerance configurable or environment-aware, as different test environments might have varying baseline goroutine counts.
169-256: Good test coverage for context cancellation.The test correctly validates that the engine respects context cancellation and returns an error when the context is cancelled.
Summary by CodeRabbit
Bug Fixes
Tests