fix(graph): [OCISDEV-1466] speed up user search when sharing from the vault

[gauravsoni119]

Description

The user search behind $filter=vaultEligible eq true now checks vault eligibility only for the users the directory search matched. It fetches the roles once, then looks up each matched user's role assignments, up to 10 in parallel, keeping the search order. It no longer asks the settings service for every holder of a vault role, which made settings scan every account's assignments. The same users come back as before, and the request still fails closed on any lookup error.

Related Issue

• Fixes: OCISDEV-1466

Motivation and Context

Searching for share recipients in the vault ("Safe") took seconds to minutes, because each search's cost grew with the total number of accounts in the system. With 3,000 role assignments it took 7.4 s, against 26 ms outside the vault; it now takes about 40 ms.

How Has This Been Tested?

1. Open a file in the vault, go to Share with people → Search, and type an admin's name (e.g. "katherine"): the user appears almost instantly.
2. Search for a Space Admin (e.g. "moss"): the user appears.
3. Search for a user who only has the User role (e.g. "einstein"): no results, as before the change.
4. Search for the same users outside the vault: all of them appear, as before.

Screenshots (if appropriate):

N.A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised: N.A

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[jvillafanez]

I guess it's out of scope, but the users, err := g.identityBackend.GetUsers(ctx, req) call might fetch all the users and bring them into memory. That's a lot of potential memory that we might need to use (10k or even 100k of users, multiplied by several searches at the same time).

In addition, we're traversing all of those users twice.

Even with the parallel processing, I'm not sure this will improve the speed in large environments.
If we consider that the GRPC requests will take the most time, the previous code usually needs 3 requests (up to N+2, where N is the number of available roles). However, the new code needs M+2 requests, where M is the number of users in fetched. Even if we consider concurrent requests as only one requests, that would be (with the current limits) M/10 + 2 requests.
If my calculations are right, I don't think the new code will improve the performance with over 20 users, and the performance will degrade the more users are added.

I think we'll need some performance data with a large number of users in order to prove that the PR really improves the performance on all scenarios.

[jvillafanez]

With 3,000 role assignments it took 7.4 s, against 26 ms outside the vault; it now takes about 40 ms.

I wonder if the problem is that the g.roleService.ListRoleAssignmentsFiltered(...) call isn't optimized. If it's searching by user and then filtering, that could explain why the performance is degraded with the amount of users.
A cache using the role as key should perform with constant time, and that cache is what the ListRoleAssignmentsFiltered call should be using.

[gauravsoni119]

With 3,000 role assignments it took 7.4 s, against 26 ms outside the vault; it now takes about 40 ms.

I wonder if the problem is that the g.roleService.ListRoleAssignmentsFiltered(...) call isn't optimized. If it's searching by user and then filtering, that could explain why the performance is degraded with the amount of users. A cache using the role as key should perform with constant time, and that cache is what the ListRoleAssignmentsFiltered call should be using.

Thanks for the careful look. I ran a benchmark to check it with numbers.

Setup: a local ocis_full stack with about 4,000 accounts in the settings store. There are 1,000 real directory users, bench0000–bench0999, so one search term matches a known number of them: bench0000 matches 1, bench000 10, bench00 100, bench0 1,000. Every matched user has a role assignment, and 1 in 10 is vault-eligible. Old is master; new is this PR. Each cell is the median of 6 runs, in ms.

matched users (M)	old, cache off	new, cache off	old, cache warm	new, cache warm
1	10,100	64	80	49
10	9,880	66	78	50
100	9,872	116	80	56
1,000	10,312	531	114	118

The first vault search after a restart (cold cache, M = 1) took 5,149 ms with the old code and 46 ms with the new.

On the request count: you're right that the old code makes 3 gRPC calls and the new one makes M + 1. The difference is what each call costs inside settings.

• For TYPE_ROLE, ListRoleAssignmentsFiltered goes to ListRoleAssignmentsByRole, which reads every account's folder one after another. The code comment there says so: "This is very inefficient…". So each of the old code's calls is O(all accounts).
• ListRoleAssignments reads a single account's folder.
• So it's 2 roles × all accounts in sequence, against M single-account reads, 10 at a time. M can't be larger than the total number of accounts.

On the cache: Once the settings cache is warm, the old path is fast, and at 1,000 matched users the two are level. But whoever searches first after the cache expires or the service restarts pays for the whole scan (5 s here, growing with the number of accounts). The cache is per settings instance, so it happens again on every replica. That fits the roughly 2 minutes reported in the ticket. The new code has no such cliff; its cost grows with the size of the search result.

On GetUsers loading every matched user into memory: agreed that it's a concern, but this PR doesn't change it. The old code made the same call first, and plain user search without the filter does the same. Looping over the result once more in memory costs next to nothing compared with the network calls.

On a role-keyed cache or index in settings: I agree that's the proper fix for ListRoleAssignmentsFiltered by role. I kept it out of this PR because it changes how settings stores data and has to stay correct on every assign/remove across replicas, and a stale entry would mean wrong vault eligibility. This PR limits the cost to the size of the search result now. We can open a follow-up ticket for the settings index, if that works for you.

I ran it on a single machine, with all services in one oCIS process and the settings data on local disk, so each storage round trip is very cheap. In a production deployment those round trips go over the network, often to NFS or S3. I'd expect that to hurt the old sequential scan much more than the per-user lookups (10 at a time), but I haven't measured it.

[jvillafanez]

Ok , I think I get it now. The expectation is that the g.identityBackend.GetUsers(ctx, req) returns just a bunch of users, but not all of them. In that case, getting the information for a few users directly will be faster than searching in all of them.

Basically, there are some details that explain those results:

• The web client forces to enter a few chars in the user search, meaning that don't expect to return all the users but a few of them.
• Getting the information for a few particular users one by one is faster than getting the same information having to traverse through all the users.
• Performing search requests in parallel cuts down quite some time.

Without touching how the information is stored in the FS, this solution is fine 👍

[jvillafanez]

Just a couple of minor things:

• Include a comment in the line with g.identityBackend.GetUsers(ctx, req) to clarify we expect to get a few users, but not all of them.
• Please run the code with the race detector enabled if not done already (need to build the binary with the -race option). This is mostly to check that the elegible list doesn't cause problems. I don't think it will, but better to double check and get confirmation.

[gauravsoni119]

Just a couple of minor things:

• Include a comment in the line with g.identityBackend.GetUsers(ctx, req) to clarify we expect to get a few users, but not all of them.
• Please run the code with the race detector enabled if not done already (need to build the binary with the -race option). This is mostly to check that the elegible list doesn't cause problems. I don't think it will, but better to double check and get confirmation.

1. Comment added.
2. No races in the new code.