fix: fix concurrency issue: do not merge head snapshot/version requests fr… #2580

maxmanuylov · 2025-10-27T14:59:23Z

…om different tenants.

May be related to #2573.

Summary by CodeRabbit

Refactor
- Updated request deduplication to operate on a per-tenant basis rather than globally, affecting how concurrent requests are handled in multi-tenant scenarios.

…om different tenants. May be related to Permify#2573.

coderabbitai · 2025-10-27T15:00:03Z

Walkthrough

Two singleflight decorator files are modified to use tenantID as the deduplication key instead of an empty string in HeadSnapshot and HeadVersion methods, changing request deduplication scope from global to per-tenant.

Changes

Cohort / File(s)	Change Summary
Singleflight tenant-scoped deduplication `internal/storage/decorators/singleflight/data_reader.go`, `internal/storage/decorators/singleflight/schema_reader.go`	HeadSnapshot and HeadVersion methods updated to pass tenantID instead of empty string as singleflight key, restricting deduplication scope per tenant rather than globally.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Verify tenantID variable is properly in scope and available at both call sites
Confirm this per-tenant scoping is the intended behavior for both decorator methods

Poem

🐰 Hoppy hopping through the keys so bright,
Tenants now get their own dedup might!
No more sharing a lonely empty string,
Each gets their own lock—hear the bells ring! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix: fix concurrency issue: do not merge head snapshot/version requests fr…" is directly related to the changeset. The main changes involve updating both `HeadSnapshot` and `HeadVersion` methods to use `tenantID` as the singleflight key instead of an empty string, which scopes deduplication per tenant. This technical change directly implements the stated objective of preventing merging/deduplication of head snapshot and version requests across different tenants. The title accurately conveys the functional intent and primary purpose of the changes, making it clear and specific enough for a reviewer to understand the concurrency fix being applied.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.}

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28e4d97 and 15d40e0.

📒 Files selected for processing (2)

internal/storage/decorators/singleflight/data_reader.go (1 hunks)
internal/storage/decorators/singleflight/schema_reader.go (1 hunks)

🔇 Additional comments (1)

internal/storage/decorators/singleflight/data_reader.go (1)

57-57: Verification confirmed: singleflight decorators are correctly implemented.

The codebase search found no instances of empty string keys in singleflight patterns. Both singleflight decorator files (data_reader.go and schema_reader.go) correctly use tenantID as the deduplication key, properly scoping operations by tenant. No similar issues exist elsewhere.

coderabbitai · 2025-10-27T15:03:37Z

internal/storage/decorators/singleflight/data_reader.go

 // HeadSnapshot - Reads the latest version of the snapshot from the repository.
 func (r *DataReader) HeadSnapshot(ctx context.Context, tenantID string) (token.SnapToken, error) {
-	rev, _, err := r.group.Do(ctx, "", func(ctx context.Context) (token.SnapToken, error) {
+	rev, _, err := r.group.Do(ctx, tenantID, func(ctx context.Context) (token.SnapToken, error) {


⚠️ Potential issue | 🔴 Critical

Critical fix: Proper tenant isolation in singleflight deduplication.

This change correctly scopes the singleflight deduplication to per-tenant instead of global. Previously, using an empty string as the key would cause concurrent HeadSnapshot requests from different tenants to be merged into a single underlying call, returning the same snapshot token to all tenants. This violated tenant isolation and could lead to authorization checks being performed against the wrong tenant's data.

The fix appropriately uses tenantID as the singleflight key, mirroring the same correction made in schema_reader.go for HeadVersion. This ensures that only concurrent requests for the same tenant are deduplicated.

🤖 Prompt for AI Agents

In internal/storage/decorators/singleflight/data_reader.go around line 57, the singleflight key is incorrectly scoped (empty string) causing cross-tenant deduplication; change the call to r.group.Do to use tenantID as the key (i.e., pass tenantID instead of ""/empty) so concurrent HeadSnapshot requests are deduplicated per-tenant only, matching the fix in schema_reader.go for HeadVersion and preserving tenant isolation.

coderabbitai · 2025-10-27T15:03:37Z

internal/storage/decorators/singleflight/schema_reader.go

 // HeadVersion - Finds the latest version of the schema.
 func (r *SchemaReader) HeadVersion(ctx context.Context, tenantID string) (version string, err error) {
-	rev, _, err := r.group.Do(ctx, "", func(ctx context.Context) (string, error) {
+	rev, _, err := r.group.Do(ctx, tenantID, func(ctx context.Context) (string, error) {


⚠️ Potential issue | 🔴 Critical

Critical fix: Proper tenant isolation in singleflight deduplication.

This change correctly scopes the singleflight deduplication to per-tenant instead of global. Previously, using an empty string as the key would cause concurrent HeadVersion requests from different tenants to be merged into a single underlying call, resulting in the same version being returned to all tenants regardless of their actual latest versions. This was a critical tenant isolation bug that could lead to incorrect behavior and potential data leakage.

The fix appropriately uses tenantID as the singleflight key, ensuring that only concurrent requests for the same tenant are deduplicated.

🤖 Prompt for AI Agents

In internal/storage/decorators/singleflight/schema_reader.go around line 46, the singleflight call is using an empty-string key which causes deduplication across all tenants; change the key to tenantID so deduplication is scoped per-tenant (i.e., pass tenantID as the key to r.group.Do). Ensure tenantID is the canonical tenant identifier (string) used throughout this code path and convert it to a string if necessary, so concurrent HeadVersion requests are only merged for the same tenant.

Fix concurrency issue: do not merge head snapshot/version requests fr…

15d40e0

…om different tenants. May be related to Permify#2573.

coderabbitai bot reviewed Oct 27, 2025

View reviewed changes

maxmanuylov mentioned this pull request Oct 27, 2025

[BUG] Two concurrent transactions get the same snapshot #2573

Closed

maxmanuylov changed the title ~~Fix concurrency issue: do not merge head snapshot/version requests fr…~~ fix: Fix concurrency issue: do not merge head snapshot/version requests fr… Oct 27, 2025

maxmanuylov changed the title ~~fix: Fix concurrency issue: do not merge head snapshot/version requests fr…~~ fix: fix concurrency issue: do not merge head snapshot/version requests fr… Oct 27, 2025

tolgaozen merged commit c219fe8 into Permify:master Oct 27, 2025
8 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: fix concurrency issue: do not merge head snapshot/version requests fr… #2580

fix: fix concurrency issue: do not merge head snapshot/version requests fr… #2580

Uh oh!

maxmanuylov commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 27, 2025

Uh oh!

coderabbitai bot Oct 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

fix: fix concurrency issue: do not merge head snapshot/version requests fr… #2580

fix: fix concurrency issue: do not merge head snapshot/version requests fr… #2580

Uh oh!

Conversation

maxmanuylov commented Oct 27, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxmanuylov commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 27, 2025 •

edited

Loading