Copilot Adoption: make the page load on a large tenant

[sambetts]

Addresses #360. Related to #392 (found while diagnosing this, but a separate defect in the user import — not fixed here).

The Copilot Adoption page did not load on a large tenant: minutes of "Analysing Copilot adoption…" followed by "the analysis is taking longer than expected". Two independent causes, only one of which was SQL.

Measured end to end on the customer tenant that exhibited the problem, via three test builds:

	Page load	State
Before	>730 s	never completed; client polling ceiling tripped
Build 1811	93.6 s	completed, but the agent section hit the query timeout
Build 1813	~16 s	complete, no polling at all, no failed steps

At 16 s every endpoint now returns 200 on its first request, inside FirstResponseBudget, so the polling loop never engages.

---

1. The web app processed one request at a time, per browser

Global.asax.cs called SetSessionStateBehavior(SessionStateBehavior.Required) on every request and declared an (empty) Session_Start. Declaring Session_Start is what makes ASP.NET persist the new session and issue the ASP.NET_SessionId cookie; Required then makes SessionStateModule take an exclusive per-session lock for the duration of each request. Every request carrying one browser's cookie was therefore serialised.

Nothing in the solution reads or writes Session or TempData (grepped the whole solution); sign-in is OIDC held in an auth cookie. The line dates from the initial code commit and never had a purpose.

Invisible on fast pages, fatal on this one: the SPA polls three endpoints concurrently and each poll parks for up to the 20 s first-response budget. Serialised, the site answers one poll per budget while three arrive per budget, so the queue grows without bound.

Evidence. A browser trace of the failure shows 32 consecutive completions spaced 19.4–21.2 s apart, never overlapping, across three separate TCP connections with ~4 ms of stalled time — server-side serialisation, not browser connection queueing. Response times climbed 20 → 35 → 76 → 137 → 177 s until the client gave up.

Reproduced under IIS Express with three concurrent requests to an async handler with a 2 s budget:

sessionMode	Wall clock	Handler-start spread
Required + Session_Start (production)	6,287 ms	4,156 ms — serialised
Disabled (this PR)	2,146 ms	0 ms — concurrent

Reviewer hotspot: this changes request handling for the whole web app, not just this page. The argument that it is safe is the absence of any Session/TempData usage — worth a second pair of eyes on that claim.

2. Three queries read the interaction table four times each

Each defined a CTE over copilot_chats and then selected from it in four separate aggregates. SQL Server does not materialise a CTE — it expands it at every reference — so the scan ran four times. WeeklyAdoptionTrendSql already documents this rule; these violated it, and AgentUsageSql carried a comment asserting the opposite ("one projected pass").

Confirmed on the customer database from Query Store: every slow plan showed four separate operators against copilot_chats.

Fixed in two of them by reading the table once into a grain temp table and aggregating that:

Query	Metric	Before	After
AgentUsageSql	copilot_chats logical reads	234,007	33,945
	spool (worktable) reads	13,223	0
	CPU	6,845 ms	1,861 ms
	elapsed	2,127 ms	920 ms
UnlicensedUsageRowsSql	logical reads	150,506,448	26,037
	duration	1,066,710 ms	3,370 ms

The unlicensed read collapse is far larger than the repeated reference alone accounts for: evaluating the CTE four times also pushed the optimiser into a much worse plan for the licence and guest lookups.

Both verified row-for-row identical against the previous shape with EXCEPT in both directions, on the customer database (500 and 7,480 rows respectively, zero differences each way).

Separately, WeeklyAdoptionTrendSql was joining the seat list and dbo.users per chat row to answer two questions that are constant per user. Both lookups moved below the aggregate: 2.4–2.5× faster elapsed, ~45% less CPU, logical reads unchanged (the win is join/aggregation CPU), verified identical via EXCEPT.

3. Steps now run concurrently, bounded

AnalyseAsync ran ten steps sequentially, so the page cost their sum with each step allowed up to QueryTimeoutSecs. They are independent, so they now overlap, bounded by MaxConcurrentSteps (default 2).

Deliberately low, and the measurement is in the code: overlapping does not create database capacity, and both the gain and the per-step inflation saturate at 2 (28-day window: 14,674 ms sequential → 12,945 ms at 2 → 12,470 ms at 4). Going to 4 buys another 3% for twice the concurrent load on a database shared with the importer. Overlapping also makes each step ~2× slower, moving every step ~2× closer to the query timeout — and a step that hits it degrades to a warning and silently drops a whole section.

Step side-effects (warnings, the SQL tab, incomplete-data reasons) are buffered per step in StepOutput and merged in step order. Those collections are a plain List/Dictionary, so concurrent writers would corrupt them; locking alone would leave their order decided by whichever query finished first, which would fill the workbook export's before/after comparison with reordering noise.

4. A failed step is now reported as failed

SafeAsync converts a query failure into a warning and returns null, so the step runner's try/catch never saw an exception and the step was timed as a normal completion. A section that silently vanished was indistinguishable from one that was simply fast.

This is not academic: during this work a syntactically broken query showed up in the performance harness as a step that had got 6.5× faster, because it failed instantly and the failure was invisible.

StepOutput now records whether any of its queries failed. Verified by hiding a table the report needs: the step is reported FAILED at 5 ms and the harness fails the run, where previously it was a success.

5. Two row caps that had become binding

Both existed because the queries were slow. They are not any more, and both were distorting headline figures on a real tenant:

• MaxAgents 500 → 5000. The inventory, the "agents to retire" count and the health breakdown were all reduced to whichever agents were busiest — the figures an agent clean-up is run from.
• MaxOpportunityCandidates 5000 → 50000, matching its siblings. The "recommended for a licence" headline is a COUNT of this list, so a tenant with more candidates than the cap saw a KPI that was simply the cap value.

Cost, measured: the opportunity query roughly doubles on a synthetic 200k-user tenant (5,242 → 12,146 ms) because it returns ten times the rows. That bench has far more unlicensed candidates than a real tenant and the step has ample headroom now. The better fix — taking the headline count from its own cheap aggregate and leaving the returned list small — is noted in the code as deferred.

Performance harness

Tests.FakeDataGen/StressTests/CopilotAdoptionPerfTest.cs drives the real CopilotAdoptionService (not copied SQL), reports per-step medians with a discarded cold run, writes JSON and diffs against a baseline so it can gate a change. COPILOTPERF_MAXSTEPS makes the step concurrency measurable rather than asserted.

It deliberately does not use ConnectionStringAnalyticsDbContextFactory: that passes autoUpdate: true, which installs MigrateDatabaseToLatestVersion. A measurement harness must never alter the schema of the database it is measuring — pointed at a restored copy of a customer database it would silently start migrating it. A local non-migrating factory is used instead.

---

Schema, config and compatibility

Database migrations	None. No schema change. No manual SQL scripts required.
Config schema	None. No BaseSolutionInstallConfig change, so no CONFIG_VERSION bump.
Breaking changes	None.
Behaviour changes	Session state disabled app-wide; two row caps raised; steps overlap.

Testing

• Full solution builds clean.
• 128/128 Copilot Adoption tests pass.
• Full unit suite: 1010 passed, 12 failed — all pre-existing environment failures (4 need a local InstallerTestConfig.json, 8 need real Azure/Graph credentials). Only the three CopilotAdoption*Tests.cs files reference the changed code.
• Both rewritten queries were executed directly against a database and their generated SQL inspected, not merely exercised through the harness — see §4 for why that distinction matters.
• Validated on the customer tenant across test builds 1811, 1812 and 1813.

What was measured and rejected

Recorded because the negative results are as useful as the positive ones, and two of them are counter-intuitive:

• AgentUsageSql rolled up with COUNT(DISTINCT) over a single CTE — 4.6× worse. It reintroduced exactly the spool the original multi-CTE shape was written to avoid. Materialising once is what gets both no repeated scan and no spool.
• LicensedUsersSql given the same grain treatment — 3.2× worse on the bench, so not included. That bench has a far larger licensed population than a real tenant, so the intermediate does not pay for itself there. It still has the four-reference defect and is ~5 s in production, not failing; it needs a production A/B before it ships.
• Step concurrency of 4 — only 3% better than 2 for twice the database load and no reduction in timeout risk.

Deferred

• LicensedUsersSql (above).
• Decoupling the "recommended for a licence" count from the returned list size.
• The synthetic bench's agent generator emits roughly one grain row per interaction where real usage repeats, so it does not reproduce the agent win and measures the worst case. Noted in the code; worth fixing before the bench is trusted on that query again.
• ResourceTypes needs no work: it went 53,941 → 944 ms without being touched, once the two pathological queries stopped monopolising the database. Its slowness was contention (3.4 s CPU against 53.9 s elapsed).