Skip to content

perf: parallelize delegation export fetches across subunits - #2338

Open
TheTechArch wants to merge 1 commit into
mainfrom
perf/delegation-export-parallel-subunits
Open

perf: parallelize delegation export fetches across subunits#2338
TheTechArch wants to merge 1 commit into
mainfrom
perf/delegation-export-parallel-subunits

Conversation

@TheTechArch

@TheTechArch TheTechArch commented Jun 24, 2026

Copy link
Copy Markdown
Member

Why

The delegation export (zip of CSV files) is slow for large companies with many subunits.

The export fetched delegation data one giver at a time (parent org + each subunit), with a sequential await inside the per-giver loop for each of the four data types (roles, access packages, single rights, instances). For an org with N subunits this chained roughly 4 × N downstream calls end-to-end, so export time grew linearly with subunit count.

What changed

DelegationExportService now fans the per-giver calls out concurrently:

  • New FetchPerGiver helper runs the per-giver downstream calls in parallel (Task.WhenAll), bounded by MaxGiverConcurrency = 10 via a SemaphoreSlim gate — speeds things up without overwhelming the downstream API.
  • Results are returned in giver order, so CSV row content and ordering are unchanged.
  • Role-name and package-name lookups now overlap with the per-giver fetches instead of running before them.

Per data type, ~N sequential round-trips collapse to ~⌈N/10⌉ batches (e.g. 50 subunits → ~5 batches instead of 50 chained calls).

Notes

  • The four data types still run sequentially relative to one another (a constant 4× factor). Parallelizing those too is a possible follow-up; left out here to keep the per-type error handling simple.
  • MaxGiverConcurrency = 10 is a conservative cap and is the knob to tune if the downstream API tolerates more.

Testing

  • dotnet build succeeds.
  • All 21 existing delegation-export tests pass (--filter FullyQualifiedName~DelegationExport).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Performance Improvements
    • Export generation now processes multiple source entries in parallel, improving responsiveness and reducing wait times for large exports.
    • Export results continue to appear in the same order as before.

The export fetched delegation data one giver (parent org + each subunit)
at a time, with a sequential await inside the per-giver loop for each of
the four data types (roles, access packages, single rights, instances).
For an org with N subunits this chained ~4*N downstream calls end-to-end,
so export time grew linearly with subunit count and was slow for large
companies.

Fan the per-giver calls out concurrently via a bounded FetchPerGiver
helper (Task.WhenAll, capped at MaxGiverConcurrency=10 via SemaphoreSlim)
so we speed things up without overwhelming the downstream API. Results
are returned in giver order, keeping CSV row content and ordering
unchanged. Role- and package-name lookups now overlap with the per-giver
fetches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 24, 2026 13:27
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 08016ffb-544a-4cae-bd80-bed8be1fd2b8

📥 Commits

Reviewing files that changed from the base of the PR and between a6fd2ed and 40efd7f.

📒 Files selected for processing (1)
  • backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/DelegationExportService.cs

📝 Walkthrough

Walkthrough

DelegationExportService is refactored to fetch per-giver export data concurrently. A MaxGiverConcurrency constant and a generic FetchPerGiver<TResult> helper using SemaphoreSlim are introduced. All four row-builder methods (BuildRoleRows, BuildAccessPackageRows, BuildInstanceRows, BuildSingleRightRows) are updated to use this helper instead of sequential awaits per giver.

Changes

Bounded Concurrent Per-Giver Export Fetching

Layer / File(s) Summary
FetchPerGiver helper and concurrency constant
...Services/DelegationExportService.cs
Adds MaxGiverConcurrency constant and a new generic FetchPerGiver<TResult> private method that fans out async fetch tasks for each giver, bounds concurrency with SemaphoreSlim, releases in a finally block, and returns a giver-ordered array via Task.WhenAll.
Row-builder concurrency updates
...Services/DelegationExportService.cs
BuildRoleRows, BuildAccessPackageRows, BuildInstanceRows, and BuildSingleRightRows each replace sequential per-giver await calls with FetchPerGiver, then iterate by index over the pre-fetched results to construct export rows. Role and access package methods additionally parallelize their name-metadata lookups.

Sequence Diagram(s)

sequenceDiagram
  participant ExportService as DelegationExportService
  participant FetchPerGiver
  participant Semaphore as SemaphoreSlim
  participant API as Downstream APIs

  ExportService->>FetchPerGiver: givers[] + fetchFunc (BuildRoleRows / BuildAccessPackageRows / etc.)
  loop Fan-out per giver (bounded by MaxGiverConcurrency)
    FetchPerGiver->>Semaphore: WaitAsync()
    FetchPerGiver->>API: fetchFunc(giver)
    API-->>FetchPerGiver: TResult
    FetchPerGiver->>Semaphore: Release()
  end
  FetchPerGiver-->>ExportService: TResult[] (giver-ordered)
  ExportService->>ExportService: iterate by index to build export rows
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Poem

🚀 No more waiting one by one,
Each giver fetched under the sun,
A semaphore keeps the chaos tame,
WhenAll completes — results the same,
Concurrent rows, in order, done! ✅

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: parallelizing delegation export fetches across subunits.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/delegation-export-parallel-subunits

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.

Copilot AI 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.

Pull request overview

Improves delegation export performance for organizations with many subunits by fetching per-giver delegation data concurrently (bounded) while preserving deterministic output ordering.

Changes:

  • Introduces bounded parallel fan-out for per-giver downstream fetches via a new FetchPerGiver helper using SemaphoreSlim + Task.WhenAll.
  • Overlaps role/package name lookups with the per-giver permission/delegation fetches to reduce end-to-end latency.
  • Refactors row-building loops to consume pre-fetched per-giver results in giver order (stable CSV ordering).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +176 to +177
Dictionary<Guid, string> roleNameLookup = roleNameLookupTask.Result;
List<RolePermission>[] permissionsPerGiver = permissionsTask.Result;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Taking the change, but noting the stated reasoning does not hold here. After await Task.WhenAll(a, b) both tasks are already complete, so there is no deadlock risk; and if either had faulted, the await would have thrown before this line is reached, so there is no AggregateException wrapping either. That said, await roleNameLookupTask is free and more idiomatic, so I will switch it.

Comment on lines +229 to +230
Dictionary<Guid, string> packageNames = packageNamesTask.Result;
Dictionary<Guid, List<PackagePermission>>[] delegationsPerGiver = delegationsTask.Result;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as the sibling thread — accepting for readability, though the AggregateException/deadlock rationale does not apply after an await Task.WhenAll.

@sonarqubecloud

Copy link
Copy Markdown

@TheTechArch

Copy link
Copy Markdown
Member Author

Triaging the automated review so this is easier to pick up — all seven Copilot comments now have replies, and CodeRabbit reported no actionable comments.

Accepted (3):

  • The MaxGiverConcurrency comment overstates its guarantee — the cap is per export invocation, not global. Rewording.
  • Two .Result accesses after await Task.WhenAll → plain await. Style only; the flagged deadlock/AggregateException risks do not actually apply once WhenAll has been awaited.

Declined (4): the ?? new List<T>()Enumerable.Empty<T>() nits. These allocate once per giver on a null response, not per iteration as stated, and the pattern is pre-existing throughout this file — changing only the moved lines would make it inconsistent for no measurable gain.

What I would actually value a human opinion on, since the bots did not raise it:

  1. Ordering is the correctness risk, and no test pins it. The whole safety argument is that FetchPerGiver returns results in giver order, so CSV content and row ordering are unchanged. The 21 existing tests pass, but they were written against the sequential path — none of them would fail if the fan-out returned results out of order. A test with mocks completing in reverse order (giver 2 fast, giver 1 slow) is the one that would genuinely protect this. Happy to add it here if reviewers want it in-scope.
  2. Failure path does more work now. Previously the name lookup was awaited first, so a failure there meant zero per-giver calls. With Task.WhenAll, all N fetches run to completion before the exception surfaces. Error type and the HttpStatusException re-wrap in ExportReporteeDelegations are unchanged, so this is a cost trade-off rather than a bug — flagging it in case anyone objects.
  3. Is MaxGiverConcurrency = 10 the right cap? Chosen conservatively. Anyone with knowledge of the downstream API tolerances, please weigh in.

For what it is worth on thread-safety: the clients only read from IHttpContextAccessor and share an HttpClient (thread-safe for concurrent GetAsync), and the AsyncLocal context flows correctly into the fan-out lambdas since the tasks start synchronously on the current execution context.

Copilot AI 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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/DelegationExportService.cs:27

  • The comment on MaxGiverConcurrency reads like a global safeguard, but the semaphore gate is created inside FetchPerGiver, so the bound applies per export invocation. Clarify the comment to avoid implying a global cap across concurrent exports.
        // Upper bound on concurrent downstream calls when fanning out over givers (parent + subunits),
        // so exports for orgs with many subunits stay fast without overwhelming the downstream API.
        private const int MaxGiverConcurrency = 10;

backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/DelegationExportService.cs:177

  • After awaiting Task.WhenAll(...), prefer awaiting the already-completed tasks instead of accessing .Result. This keeps the async flow idiomatic and preserves the original exception propagation semantics without sync-over-async accessors.
            Dictionary<Guid, string> roleNameLookup = roleNameLookupTask.Result;
            List<RolePermission>[] permissionsPerGiver = permissionsTask.Result;

backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/DelegationExportService.cs:230

  • Same pattern here: after Task.WhenAll(...), use await on the tasks instead of .Result for idiomatic async code and consistent exception behavior.
            Dictionary<Guid, string> packageNames = packageNamesTask.Result;
            Dictionary<Guid, List<PackagePermission>>[] delegationsPerGiver = delegationsTask.Result;

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