Skip to content

adding locking for userset weight writes as it is on demand#514

Merged
yissellokta merged 3 commits intomainfrom
feat/locking
Nov 21, 2025
Merged

adding locking for userset weight writes as it is on demand#514
yissellokta merged 3 commits intomainfrom
feat/locking

Conversation

@yissellokta
Copy link
Copy Markdown
Contributor

@yissellokta yissellokta commented Nov 20, 2025

Description

What problem is being solved?

How is it being solved?

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

Release Notes

  • Tests

    • Added comprehensive concurrent testing suite to validate thread-safe behavior across multiple concurrent scenarios.
  • Bug Fixes

    • Enhanced thread-safety for concurrent weight operations to prevent potential race conditions.

✏️ Tip: You can customize this high-level summary in your review settings.

@yissellokta yissellokta requested a review from a team as a code owner November 20, 2025 18:54
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 20, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds thread-safety to weighted graph userset weight mutations by introducing per-node and per-edge sync.RWMutex instances stored in global sync.Map caches, with helper functions to lazily retrieve mutexes and synchronization wrapping around weight-setting operations.

Changes

Cohort / File(s) Summary
Concurrency protection implementation
pkg/go/graph/weighted_graph.go
Adds sync.Map caches (nodeUsersetMutexes, edgeUsersetMutexes) for node and edge-level mutexes; introduces helper functions getUsersetNodeMutex and getUsersetEdgeMutex for lazy mutex retrieval; wraps setUsersetWeightToNode and setUsersetWeightToEdge with per-node/edge mutex acquire-release; imports sync package.
Concurrent test suite
pkg/go/graph/weighted_graph_concurrent_test.go
Adds TestConcurrentUsersetWeights covering five scenarios: concurrent writes to node, concurrent writes to edge, concurrent same-key writes, parallel node-and-edge writes, and interleaved read-write patterns; validates thread-safe behavior and final state consistency across all goroutines.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Areas requiring attention:
    • Verify the lazy mutex initialization pattern in helper functions is thread-safe (double-checked locking in sync.Map)
    • Ensure mutex granularity (per-node, per-edge) is appropriate and doesn't introduce bottlenecks
    • Validate test coverage adequately exercises race conditions for both node and edge weight mutations
    • Confirm test assertions correctly verify final state under mutex protection

Possibly related PRs

  • add userset weights #505: The main PR directly builds on the userset-weight implementation by adding per-node/per-edge mutexes and wrapping previously added weight-setting methods with synchronization for thread-safety.

Suggested reviewers

  • adriantam
  • elbuo8

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding mutex locking for concurrent userset weight write operations in the weighted graph implementation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

elbuo8
elbuo8 previously approved these changes Nov 20, 2025
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
pkg/go/graph/weighted_graph_concurrent_test.go (1)

1-252: Concurrent tests are solid; consider minor cleanups

The subtests cover several important patterns (disjoint keys, shared key, node+edge in parallel, interleaved read/write) and exercise the new locking paths well. This is a good level of stress for the usersetWeights behavior.

Two small, optional tweaks:

  • In the verification loops (e.g., Lines 47–60, 86–99, 169–197, 229–238), you can fetch the mutex once per loop, outside the inner body, to avoid repeated sync.Map lookups via getUsersetNodeMutex/getUsersetEdgeMutex.
  • Since all subtests share the same node/edge, keep in mind future additions: if a new subtest depends on a “clean” state, it may need to reset or use a fresh graph. Right now the keys are disjoint enough that there’s no interference.

These are purely polish; the tests as written are functionally fine.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2fbf9d and 7381c39.

📒 Files selected for processing (2)
  • pkg/go/graph/weighted_graph.go (3 hunks)
  • pkg/go/graph/weighted_graph_concurrent_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/go/graph/weighted_graph.go (2)
pkg/go/graph/weighted_graph_node.go (1)
  • WeightedAuthorizationModelNode (18-27)
pkg/go/graph/weighted_graph_edge.go (1)
  • WeightedAuthorizationModelEdge (58-76)
pkg/go/graph/weighted_graph_concurrent_test.go (3)
pkg/go/graph/weighted_graph.go (1)
  • NewWeightedAuthorizationModelGraph (56-61)
pkg/go/graph/weighted_graph_node.go (1)
  • SpecificTypeAndRelation (7-7)
pkg/go/graph/weighted_graph_edge.go (2)
  • DirectEdge (11-11)
  • NoCond (55-55)
⏰ 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). (1)
  • GitHub Check: Analyze (go)

Comment thread pkg/go/graph/weighted_graph.go Outdated
Comment thread pkg/go/graph/weighted_graph.go
@yissellokta yissellokta added this pull request to the merge queue Nov 21, 2025
Merged via the queue into main with commit e4d9ef4 Nov 21, 2025
10 checks passed
@yissellokta yissellokta deleted the feat/locking branch November 21, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants