[DBOPS-231] Rewrite SecurityTask_ReadByUserIdStatus to remove OPTION (RECOMPILE)

[rkac-bw]

🎟️ Tracking

DBOPS-231

📔 Objective

SecurityTask_ReadByUserIdStatus carries OPTION (RECOMPILE), added in PM-21044 (#5779) alongside the four-CTE body it still has today. The hint makes the procedure compile on every execution, and for this procedure compilation is the dominant cost — it outweighs actual execution by well over an order of magnitude. Each compile also takes schema-stability locks across the referential-integrity closure of the tables in the query, which puts a very frequently called read in the way of schema changes on those tables.

Deleting the hint on its own is not equivalent. The current body's cost depends on the caller's organization size, so with a cached plan whichever caller compiles first imposes their shape on everyone else; removing only the hint measurably regressed some callers under test.

This PR removes the reason the plan had to vary:

1. Stage the caller's confirmed memberships in enabled organizations in a table variable — always a small set, whoever is asking.
2. Stage the collections they can edit (granted directly or through a group), but only when those organizations have any tasks at all.
3. If that set is empty, route to a separate trivial statement and return — this also keeps such callers from caching a plan for the query in step 4.
4. Otherwise, one EXISTS probe per task against the small staged set.

Because every step is driven by a small staged set, a single cached plan is correct for all callers and the hint is no longer needed. The body is heavily commented: each step explains what it does and why it is shaped that way, including why the empty-set routing exists and why the UNION is load-bearing.

Unchanged: parameters, the seven projected columns and their order, and the result ordering (CreationDate DESC). No repository, EF Core or _V2 change is required — this is a backwards-compatible stored-procedure modification.

Validation: result equivalence was verified against the current body on a restore of production, comparing full result sets in both directions. The test population was built to exercise every access shape on purpose — heavy direct members, group-only members, read-only members, members with no access at all, disabled organizations, invited/accepted/revoked memberships, and users with no organization. Zero row or column differences across 36,185 paired calls covering 224,545 rows. Plan stability was separately confirmed across four different compile orders.

Out of scope, tracked separately:

• A covering index on SecurityTask that pairs with this body — deliberately not bundled here.
• A pre-existing divergence between the T-SQL and the EF Core reimplementation on the group-collection path (EF honours a group grant only when no CollectionUser row exists on the same collection). Not introduced or changed by this PR.

/cc @bitwarden/team-data-insights-and-reporting-dev — Security Tasks is your feature area, so I would like your eyes on the access semantics. CODEOWNERS routes these paths to Vault and DBOps, so you would not otherwise be requested automatically.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.70%. Comparing base (7cd925b) to head (b6da358).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8331      +/-   ##
==========================================
+ Coverage   64.03%   69.70%   +5.66%     
==========================================
  Files        2473     2473              
  Lines      106017   106017              
  Branches     9613     9613              
==========================================
+ Hits        67885    73894    +6009     
+ Misses      35772    29641    -6131     
- Partials     2360     2482     +122     

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

[rkac-bw]

Holding this draft — please don't review yet.

Further measurement on a production-shaped restore found that the main SELECT's cached plan is sensitive to the size of the staged collection set belonging to whichever caller compiles it. The IF NOT EXISTS routing isolates callers with an empty set but not callers with a small one, so a narrow caller can compile a plan that is significantly worse for a caller with many collections — worse, for that shape, than the body this PR replaces.

I'm revising the procedure to remove that sensitivity and will force-push once the replacement is measured across compile orders. The parameters, projected columns and result ordering will not change.

[rkac-bw]

Revised and ready for review.

The instability I flagged above is fixed, and without adding a query hint. The set that decides the plan shape is now held in a #temp table rather than a table variable.

Why that works. The access probe has two possible shapes: seek CollectionCipher once per task and probe the collection set (good), or scan the collection set and seek CollectionCipher once per collection (bad — a caller with 173 editable collections pays 173 seeks per task). Which one the optimizer picks depends on how many rows it thinks are in that set. A table variable has no statistics, so under deferred compilation the first caller's row count is baked into the cached plan for everyone; a one-collection caller pins the bad shape. A table with statistics gets costed per caller.

Measured on a production-shaped restore, 13 access shapes, index in its production form:

body	best compile order	worst compile order	spread
current (OPTION (RECOMPILE))	748,152 reads	748,152	n/a — recompiles every call
this PR, table variable	133,821	279,057	2.09x
this PR, #temp table	134,013	134,138	0.09% across six compile orders

The obvious objection to a temp table is that it trades per-call recompilation for statistics-driven recompilation — so that was measured too. Rotating all 13 shapes six times: 8 recompiles in 78 calls at 16.3 ms CPU per call, against the current body's one recompile per call at 60.7 ms. The thresholds are the documented ones for temp tables (6 rows, then 500, then 500 + 20%), so callers within a band share a plan.

The table is declared with an unnamed inline primary key so SQL Server can cache it between executions.

Result parity re-verified against the current body over the 13 shapes x 3 status values: 39 pairs, 11,720 rows, zero differences.

[rkac-bw]

Added the covering index, so this is now three files.

IX_SecurityTask_OrganizationId is rebuilt with an INCLUDE list covering the columns the procedure returns. It is a DROP_EXISTING rebuild of an index that already exists on a table of roughly 17,600 rows — not a large index build — and its previous WHERE OrganizationId IS NOT NULL filter was redundant on a NOT NULL column.

Measured across the same 13 access shapes:

shape	proc only	proc + index
no-access member, 3,352-task org	7,206 reads	92
no-access member, 1,270-task org	2,738	58
heavy direct, 3,352-task org	28,444	21,319
narrow direct, 3,352-task org	27,356	20,258
group-only, 3,352-task org	27,364	20,268
single-collection member, 1,150-task org	9,409	6,982
total	134,013	92,342

About 31% overall, and the no-access shapes — members of a large task organization who cannot edit anything, a very common caller — improve by 78x and 47x.

Worth being explicit that the two changes are independent: the procedure is plan-stable without the index (0.09% across six compile orders) and equally stable with it (0.02%). The index is a straight reduction in reads, not a correctness or stability crutch.

No EF change. SecurityTaskEntityTypeConfiguration already declares HasIndex(s => s.OrganizationId) with no filter and no includes, and the EF providers' migrations created it unfiltered, so the EF model is untouched and has-pending-model-changes stays clean. INCLUDE is not expressed in the EF model; adding IncludeProperties would generate three provider migrations to chase a benefit not measured on those engines. Dropping the SQL Server filter in fact brings the two tracks closer together.

[rkac-bw]

Force-pushed a whitespace-only revision (9903866f2 → e85f7b761): the joins now follow the layout used by the large majority of procedures under src/Sql/dbo — INNER JOIN on its own line, the table and its ON clause indented on the next — and the two compact inline FROMs were split the same way.

No statement changed; the token stream is identical to the previous head. Re-gated against a freshly migrated database: DACPAC DeployReport clean, migration naming check passes, SecurityTask integration tests 9/9.

[shane-melton]

Vault's business logic looks to behave the same and is good for merge.

[shane-melton]

👍 These explanations are very much appreciated!

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the rewritten SecurityTask_ReadByUserIdStatus body, the IX_SecurityTask_OrganizationId rebuild in the SSDT table script, and the dated migration. Access semantics were traced against the previous four-CTE body and are equivalent: SecurityTaskView is an unfiltered SELECT * and OrganizationView projects Enabled from Organization, so the base-table reads match; UNION dedupes the two grant routes, so the #Cols primary key cannot be violated; and the empty-set branch returns exactly the CipherId IS NULL rows the old CipherId IS NULL OR EXISTS (...) predicate would have returned for a caller with no editable collections. The migration's stored-procedure body is character-identical to the SSDT copy, both index branches match the SSDT definition, the INCLUDE list plus the clustered PK_SecurityTask on Id covers all seven projected columns, and the dropped WHERE OrganizationId IS NOT NULL filter was redundant on a NOT NULL column and was never present in the EF providers' migrations, so leaving SecurityTaskEntityTypeConfiguration untouched is consistent.

A few things checked and found not to be problems, recorded so they are not re-raised: removing OPTION (RECOMPILE) does not expose the (@Status IS NULL OR [ST].[Status] = @Status) catch-all to parameter sniffing, because Status is only an included column and no seek is available on it either way; the group-grant route is now additionally constrained to enabled organizations, which is observable only if a task in one organization referenced a cipher in another; and both branches emit a single result set with the same column order, which is what QueryAsync<SecurityTask> requires.

[mkincaid-bw]

LGTM