Fix Infinite Loop #272
Fix Infinite Loop #272
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/sync/syncer.go(4 hunks)pkg/sync/syncer_test.go(6 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/sync/syncer.go
855-855: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
857-857: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
868-868: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
870-870: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
875-875: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
1429-1429: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
1440-1440: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
pkg/sync/syncer.go
[failure] 855-855:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 857-857:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 868-868:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 870-870:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 875-875:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 1429-1429:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
[failure] 1440-1440:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
🔇 Additional comments (14)
pkg/sync/syncer_test.go (14)
44-44: **Field initialization **
Appending resource types to mc.RtDb looks fine.
No issues found here.
94-99: **Commented-out lines **
These lines are just commented out. No functional change.
No further action is needed.
100-146: **BenchmarkExpandGrants block **
The approach of creating groups, users, and measuring expansion performance is straightforward, though large data sets (2500 users per layer) might lead to high memory usage.
Overall, this benchmark is valid and helps stress test the expansion logic.
182-186: **Renaming or initializing DB fields **
Logical changes to field names are consistent with the rest of the mockConnector code.
No immediate issues found here.
193-197: **Updated struct field names **
Renamed fields appear consistent with the new naming scheme.
No further changes needed.
218-218: **Appending to ResourceDB in AddGroup **
Storing the group resource in ResourceDB is correct.
No issues found.
226-226: **Storing the Entitlement in EntDB **
Looks correct. No issues found.
243-243: **Appending to UserDB in AddUser **
Storing the user resource in UserDB is valid.
No issues found.
265-265: **Storing grants in GrantDB **
No issues found here.
271-271: **Returning resource types in ListResourceTypes **
Implementation looks straightforward and correct.
276-279: Listing group vs user resources [approve]
The resource listing logic is consistent. No issues detected.
If new resource types are introduced later, consider a more dynamic approach.
285-285: **ListEntitlements logic **
Returning entitlements from the in-memory database is correct.
289-289: **ListGrants logic **
Implementation is correct. No issues found.
72-73: **Potential out-of-range error when accessing mc.EntDB[groupId] **
You are indexing the first element ([0]) of mc.EntDB[groupId] and mc.EntDB[nextGroupId]. If either slice is empty, this will panic.
You might want to check the slice length or handle missing entries gracefully.
To verify if any code path leads to an empty slice for these group IDs, run:
pkg/sync/syncer.go
Outdated
|
|
||
| fmt.Printf("%v\n", entitlementGraph) | ||
| if entitlementGraph.Loaded { | ||
| fmt.Printf("getting cycles1\n") |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("getting cycles1\n")
+ l.Debug("getting cycles1")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
857-857: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 857-857:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
| return nil | ||
| } | ||
|
|
||
| fmt.Printf("not done: %v\n", graph.Depth) |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("not done: %v\n", graph.Depth)
+ l.Debug("graph not done yet", zap.Int("depth", graph.Depth))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
1440-1440: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 1440-1440:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
|
|
||
| fmt.Printf("fixing cycles1\n") | ||
| err := entitlementGraph.FixCycles() | ||
| fmt.Printf("fixed cycles1\n") |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("fixed cycles1\n")
+ l.Debug("fixed cycles1")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
870-870: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 870-870:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
| actions := len(graph.Actions) | ||
| if actions%250 == 0 || actions < 10 { | ||
| l.Info("Expanding grants", zap.Int("actions", actions)) | ||
| fmt.Printf("actions: %v: %v\n", actions, graph.Actions) |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("actions: %v: %v\n", actions, graph.Actions)
+ l.Info("expanding grants: actions", zap.Int("actions_count", actions), zap.Any("actions", graph.Actions))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
1429-1429: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 1429-1429:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
| return err | ||
| } | ||
| } | ||
| fmt.Printf("expandGrantsForEntitlements\n") |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("expandGrantsForEntitlements\n")
+ l.Debug("expandGrantsForEntitlements")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
875-875: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 875-875:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
| return fmt.Errorf("cycles detected in entitlement graph") | ||
| } | ||
|
|
||
| fmt.Printf("fixing cycles1\n") |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
Please switch to structured logging instead of fmt.Printf.
Suggested fix:
- fmt.Printf("fixing cycles1\n")
+ l.Debug("fixing cycles1")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
868-868: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 868-868:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
pkg/sync/syncer.go
Outdated
| return nil | ||
| } | ||
|
|
||
| fmt.Printf("%v\n", entitlementGraph) |
There was a problem hiding this comment.
**Avoid using fmt.Printf in production code **
According to the lint rules (forbidigo), usage of fmt.Printf is disallowed:
Please switch to structured logging (e.g. l.Debug) to ensure consistency, traceability, and adherence to logging policy.
Suggested fix:
- fmt.Printf("%v\n", entitlementGraph)
+ l.Debug("entitlementGraph state", zap.Any("graph", entitlementGraph))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
855-855: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: go-lint
[failure] 855-855:
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
kans
force-pushed
the
kans/grant-expansion-halting
branch
from
9da9d5a to
05051db
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/sync/syncer_test.go (1)
36-38: Consider documenting the test parametersThe magic numbers
2500and5would benefit from documentation explaining their significance, especially since they are related to a pagination bug that caused infinite loops.- // 2500 * 4 = 10K - used to cause an infinite loop on pagition - usersPerLayer := 2500 - groupCount := 5 + // These parameters are chosen to reproduce a pagination-related infinite loop bug: + // - usersPerLayer=2500 creates 10K users (2500 * 4) which exceeds typical page sizes + // - groupCount=5 creates enough group layers to test deep grant expansion + usersPerLayer := 2500 + groupCount := 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/sync/expand/graph.go(1 hunks)pkg/sync/syncer.go(1 hunks)pkg/sync/syncer_test.go(1 hunks)
🔇 Additional comments (2)
pkg/sync/expand/graph.go (1)
42-52: Excellent change to use pointer slice for Actions
The modification of Actions field to use pointers ([]*EntitlementGraphAction) instead of values is a good improvement because:
- It ensures modifications to actions are visible across the codebase
- It reduces memory usage by avoiding copies of potentially large action objects
- It aligns with how actions are manipulated in the syncer code
pkg/sync/syncer.go (1)
Line range hint 1472-1479: Correctly using pointer when appending action
The code properly creates and appends a pointer to EntitlementGraphAction, which aligns with the updated Actions field type in the EntitlementGraph struct.
| // This isn't needed because grant expansion will create this grant | ||
| // _ = mc.AddGroupMember(ctx, group, principal) | ||
| _ = mc.AddGroupMember(ctx, group, principal) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant comment and duplicate code
The code contains both a commented-out line and its uncommented duplicate, which is confusing.
- // This isn't needed because grant expansion will create this grant
- // _ = mc.AddGroupMember(ctx, group, principal)
- _ = mc.AddGroupMember(ctx, group, principal)
+ _ = mc.AddGroupMember(ctx, group, principal)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This isn't needed because grant expansion will create this grant | |
| // _ = mc.AddGroupMember(ctx, group, principal) | |
| _ = mc.AddGroupMember(ctx, group, principal) | |
| _ = mc.AddGroupMember(ctx, group, principal) |
| tempDir, err := os.MkdirTemp("", "baton-benchmark-expand-grants") | ||
| require.NoError(t, err) | ||
| defer os.RemoveAll(tempDir) | ||
| c1zpath := filepath.Join(tempDir, "expand-grants.c1z") | ||
| syncer, err := NewSyncer(ctx, mc, WithC1ZPath(c1zpath), WithTmpDir(tempDir)) | ||
| require.NoError(t, err) | ||
| err = syncer.Sync(ctx) | ||
| require.NoError(t, err) | ||
| err = syncer.Close(ctx) | ||
| require.NoError(t, err) | ||
| _ = os.Remove(c1zpath) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test assertions
The test creates the data structure but doesn't verify the results of the grant expansion.
Add assertions to verify:
- The correct number of grants were created
- The grants have the expected relationships
- No infinite loops occurred during expansion
Example:
err = syncer.Close(ctx)
require.NoError(t, err)
_ = os.Remove(c1zpath)
+
+ // Verify grant expansion results
+ grants, err := syncer.store.ListGrants(ctx, &v2.GrantsServiceListGrantsRequest{})
+ require.NoError(t, err)
+
+ // Assert expected number of grants
+ expectedGrantCount := groupCount * usersPerLayer
+ assert.Equal(t, expectedGrantCount, len(grants.List), "Unexpected number of grants")
+
+ // Verify grant relationships
+ for _, grant := range grants.List {
+ assert.NotNil(t, grant.Principal, "Grant should have a principal")
+ assert.NotNil(t, grant.Entitlement, "Grant should have an entitlement")
+ }Committable suggestion skipped: line range outside the PR's diff.
We were accidentally updating actions by value instead of by reference. This meant we never updated the page token on the action. I'm guessing the default page size is 10K, which my test mysteriously hit at 2500 users in 5 nested groups (4 * 2500). This bug is 1 year old.