Skip to content

[PM-39197] - improve vault sync performance#7829

Open
jaasen-livefront wants to merge 5 commits into
mainfrom
PM-12715
Open

[PM-39197] - improve vault sync performance#7829
jaasen-livefront wants to merge 5 commits into
mainfrom
PM-12715

Conversation

@jaasen-livefront

@jaasen-livefront jaasen-livefront commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39197

📔 Objective

Reduce vault sync latency for users with very large direct collection assignments.

This PR optimizes the SQL Server collection-access query path used by sync by updating UserCollectionDetails to avoid the expensive join/OR shape that was causing high latency at scale.

Outcome from local verification on the same seeded dataset:

  • Before: /sync?excludeDomains=true around 21s
  • After: /sync?excludeDomains=true around 426ms

🔧 Performance approach

The sync bottleneck was isolated to collection access expansion in SQL Server (Collection_ReadByUserId via UserCollectionDetails), not API serialization or cipher loading.

To improve performance, UserCollectionDetails was reshaped from a broad left-join + OR filter pattern into two explicit branches:

  1. Direct user-to-collection access branch (CollectionUser)
  2. Group-derived access branch (GroupUser + CollectionGroup) with exclusion of directly-assigned rows

This reduces unnecessary row expansion and gives SQL Server a cleaner execution path for large collection cardinalities.

✅ Measured impact (same dataset)

  • Before: /sync?excludeDomains=true ≈ 21s
  • After: /sync?excludeDomains=true ≈ 426ms
  • Approximate improvement: ~50x faster

📸 Screenshots

Not applicable (no UI changes).

@jaasen-livefront jaasen-livefront requested review from a team as code owners June 17, 2026 16:25
@jaasen-livefront jaasen-livefront requested a review from r-tome June 17, 2026 16:25
@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the UserCollectionDetails SQL Server function refactor that reshapes the broad left-join + OR filter into two explicit UNION ALL branches (direct CollectionUser access and group-derived CollectionGroup access, with directly-assigned collections excluded from the group branch). Verified semantic equivalence with the prior implementation across the key authorization cases — direct-access precedence over group access, group-only multi-group permission aggregation, and the combined direct + group case — all of which are preserved by the CU.[CollectionId] IS NULL anti-join and the existing MIN/MIN/MAX aggregation in Collection_ReadByUserId. The SSDT source (src/Sql/dbo/...) and the dated migration match, and the function's return shape and only consumer remain unchanged, so no _V2 versioning or EF Core parity change is required.

Code Review Details

No blocking findings.

  • 🎨 : This change reshapes authorization-resolution SQL but adds no integration test asserting the combined group + direct-access case resolves identically to the prior behavior. Consider a [DatabaseData] test in CollectionRepositoryTests covering a user with both a direct CollectionUser and group CollectionGroup assignment to the same collection to lock in the equivalence.
    • test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.70%. Comparing base (1d75502) to head (74dfef5).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7829      +/-   ##
==========================================
+ Coverage   65.67%   65.70%   +0.03%     
==========================================
  Files        2195     2209      +14     
  Lines       97542    97716     +174     
  Branches     8807     8815       +8     
==========================================
+ Hits        64062    64209     +147     
- Misses      31259    31288      +29     
+ Partials     2221     2219       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jaasen-livefront jaasen-livefront changed the title [PM-12715] - improve user collection details performance [PM-12715] - improve vault sync performance Jun 17, 2026
@sonarqubecloud

Copy link
Copy Markdown

@r-tome r-tome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is really great! The query changes don't seem to change the existing query logic.

Non-blocker: I'd love to see some tests for GetManyByUserIdAsync added to https://github.com/bitwarden/server/blob/main/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs

@mkincaid-bw mkincaid-bw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jaasen-livefront jaasen-livefront changed the title [PM-12715] - improve vault sync performance [PM-39197] - improve vault sync performance Jun 22, 2026
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.

3 participants