Dirt/pm 33527/multi delete task types by prograhamming · Pull Request #7910 · bitwarden/server

prograhamming · 2026-07-01T15:06:27Z

🎟️ Tracking & 📔 Objective

This PR is stacked on top of this pull request in order to make the original ask more flexible with different types.

prograhamming · 2026-07-13T20:54:51Z

+            foreach (var deleteTaskType in deleteTaskTypes)
+            {
+                var deleteTask = new Dirt.Models.OrganizationDeleteTask
+                {
+                    OrganizationId = organization.Id,
+                    TaskType = deleteTaskType,
+                    CreationDate = creationDate,
+                    RevisionDate = creationDate,
+                };
+                deleteTask.SetNewId();
+                await dbContext.OrganizationDeleteTasks.AddAsync(deleteTask);
+            }


Done in 12d38fe — the mapping is now a Select projection passed to AddRangeAsync, matching the Dapper implementation's existing .Select(...).ToList() approach.

prograhamming · 2026-07-13T20:54:53Z

+
+namespace Bit.Infrastructure.EntityFramework.Dirt.Models;
+
+public class OrganizationDeleteTask : Core.Dirt.Entities.OrganizationDeleteTask


Won't fix — sharing the Core entity's class name (in a different namespace) is the established convention for EF models throughout this codebase, e.g. EF.Vault.Models.Cipher : Core.Vault.Entities.Cipher and EF.AdminConsole.Models.Organization : Core.AdminConsole.Entities.Organization. Renaming this one model would make it inconsistent with that pattern and the AutoMapper profile conventions.

Banrion · 2026-07-01T17:15:57Z

+    @OrganizationDeleteTaskId UNIQUEIDENTIFIER = NULL,
+    @OrganizationDeleteTaskType TINYINT = NULL,
+    @OrganizationDeleteTaskCreationDate DATETIME2(7) = NULL,
+    @OrganizationDeleteTasks [dbo].[OrganizationDeleteTaskArray] READONLY


❓ Should @OrganizationDeleteTasks argument default to NULL?

⚠️ Remove @OrganizationDeleteTaskId, @OrganizationDeleteTaskType, @OrganizationDeleteTaskCreationDate arguments and usage since they are replaced by @OrganizationDeleteTasks

Done in 31551b3 — the three single-task arguments and their legacy enqueue branch are removed; the proc now takes only @Id plus the TVP.

On defaulting to NULL: a TVP can't take an explicit default (assigning NULL to a table type is an operand type clash), and it doesn't need one — per the CREATE PROCEDURE docs, a TVP omitted at call time passes an empty table, which the IF EXISTS guard already handles.

Note this migration file no longer exists — per the other thread it was folded into the base PR's migration, now named 2026-07-14_03_AlterOrganizationDeleteByIdEnqueueDeleteTask.sql.

Banrion · 2026-07-01T17:18:50Z

+END
+GO
+
+CREATE OR ALTER PROCEDURE [dbo].[Organization_DeleteById]


⚠️ Update existing migrations and sql scripts from #7783 rather than creating new ones since those have not been merged into main yet

Done in 31551b3 — this file is deleted and the TVP change is folded into #7783's existing migration. The stack's migrations were subsequently renamed (a7d6599) to sort after main's latest, so the combined script is now 2026-07-14_03_AlterOrganizationDeleteByIdEnqueueDeleteTask.sql. Since this PR now edits migrations owned by #7783, the two need to merge together (or this one rebases after #7783 lands) — noted in the PR description.

Banrion · 2026-07-01T17:24:59Z

    @OrganizationDeleteTaskId UNIQUEIDENTIFIER = NULL,
    @OrganizationDeleteTaskType TINYINT = NULL,
-    @OrganizationDeleteTaskCreationDate DATETIME2(7) = NULL
+    @OrganizationDeleteTaskCreationDate DATETIME2(7) = NULL,
+    @OrganizationDeleteTasks [dbo].[OrganizationDeleteTaskArray] READONLY


⚠️ Remove the three single task arguments in preference of the one multiple tasks argument

❓ Should @OrganizationDeleteTasks default to NULL

Done in 31551b3 — the three single-task parameters are removed in preference of @OrganizationDeleteTasks.

On = NULL: TVPs can't be given an explicit default and don't need one — they're implicitly optional. Per the CREATE PROCEDURE docs: "If a procedure contains table-valued parameters, and the parameter is missing in the call, an empty table is passed in." The IF EXISTS (SELECT 1 FROM @OrganizationDeleteTasks) guard handles that case.

Remove the legacy single-task scalar params (@OrganizationDeleteTaskId, @OrganizationDeleteTaskType, @OrganizationDeleteTaskCreationDate) and the legacy single-task enqueue branch from Organization_DeleteById, keeping only the @OrganizationDeleteTasks table-valued parameter. The single-task version (PR #7783) has not merged to main, so no rolling-deployment backwards compat is needed. Fold the multi-task change into the existing #7783 migration (2026-06-17_02) and remove the redundant new 2026-06-30_00 migration.

Map deleteTaskTypes into OrganizationDeleteTask entities via a LINQ Select projection and enqueue them with AddRangeAsync, matching the Dapper repository's pattern instead of mapping inside a foreach loop.

codecov · 2026-07-13T20:51:07Z

Codecov Report

❌ Patch coverage is 75.93985% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.71%. Comparing base (2d8a0e8) to head (6c95b9c).

Files with missing lines	Patch %	Lines
...tityFramework/Dirt/Repositories/EventRepository.cs	0.00%	10 Missing ⚠️
...ions/EventsCleanupOrganizationDeleteTaskHandler.cs	0.00%	6 Missing ⚠️
.../Dirt/Repositories/TableStorage/EventRepository.cs	0.00%	5 Missing ⚠️
src/Admin/Jobs/JobsHostedService.cs	0.00%	4 Missing ⚠️
src/Admin/Jobs/OrganizationDeleteTasksJob.cs	93.65%	1 Missing and 3 partials ⚠️
...ucture.Dapper/Dirt/Repositories/EventRepository.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           dirt/pm-33527/server-db-combined    #7910      +/-   ##
====================================================================
- Coverage                             65.72%   65.71%   -0.01%     
====================================================================
  Files                                  2214     2215       +1     
  Lines                                 97929    97979      +50     
  Branches                               8828     8830       +2     
====================================================================
+ Hits                                  64361    64391      +30     
- Misses                                31346    31366      +20     
  Partials                               2222     2222

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

github-actions · 2026-07-13T20:53:49Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the generalization of the organization-delete cleanup path from a single events-cleanup job to a type-dispatched OrganizationDeleteTask queue drained by OrganizationDeleteTasksJob. Examined the dual-ORM DeleteAndCreateDeleteTasksAsync implementations (Dapper JSON/OPENJSON enqueue and EF AddRangeAsync), the bounded/resumable batch-delete handlers across SQL Server, EF Core, and Table Storage, the new OrganizationDeleteTask schema plus stored procedures, the four dated migrations, and the regenerated EF migrations for all three providers. Verified lease/claim semantics, sanitized error handling, orphaned-task escalation logic, and DI wiring; unit and integration tests cover the claim/reclaim/failure-cap, multi-row enqueue, and no-enqueue-on-plain-delete paths.

No new blocking findings. Prior reviewer feedback (JSON-instead-of-TVP, single-task argument removal, migration re-dating, Select projection) has been addressed, and the two open documentation-level threads (IX_Event_OrganizationId ORM divergence, Event.sql SendId) are already tracked.

Code Review Details

No findings at or above the confidence threshold for posting.

Notes considered and intentionally not raised as inline findings:

Table Storage DeleteManyByOrganizationIdAsync can submit up to ~1.8M deletes per call with lease refresh only occurring between calls; the 4-minute run budget against a 10-minute lease provides margin, and the bounded-run-budget design was already discussed in an existing thread. No profiling evidence of lease overrun, so not flagged.
Per-provider batch sizes differ (SQL Server 50000, EF 1000, Table Storage 1.8M) but each is bounded and only the provider-appropriate path runs in a given deployment (cloud uses Table Storage).

claude · 2026-07-14T15:22:58Z

+        builder
+            .HasIndex(e => e.OrganizationId)
+            .IsClustered(false)
+            .HasDatabaseName("IX_Event_OrganizationId");


♻️ DEBT: IX_Event_OrganizationId diverges between ORMs — MSSQL is filtered, EF is not.

Details

The MSSQL definition (Event.sql:34-35 and the 2026-07-14_04 migration) creates a filtered index:

CREATE NONCLUSTERED INDEX [IX_Event_OrganizationId] ON [dbo].[Event]([OrganizationId] ASC) WHERE [OrganizationId] IS NOT NULL;

The EF configuration here (and the three generated EF migrations) creates a non-filtered index — no HasFilter. The migration comment states the filter is intentional ("keeps the index small since many events are user-scoped"), so the EF providers miss that optimization and the four supported databases end up with different index shapes.

PostgreSQL and SQLite support partial indexes, so parity is achievable there:

builder .HasIndex(e => e.OrganizationId) .IsClustered(false) .HasFilter("[OrganizationId] IS NOT NULL") .HasDatabaseName("IX_Event_OrganizationId");

MySQL does not support partial indexes, so this is a genuine per-provider limitation rather than a fix you can apply everywhere. Not a correctness issue (the index is still used), but worth aligning where the ORMs allow it, or noting explicitly why they differ.

Reference: dual-ORM parity — a schema change should be reflected consistently across both tracks.

This follows the repo's existing precedent: merged PR #7690 added IX_Event_OrganizationIdSendIdDate filtered on MSSQL but unfiltered on the EF side, with a comment explaining that MySQL doesn't support filtered indexes. In 6c95b9c I added the same explanatory comment to this index's EF configuration rather than introducing per-provider HasFilter branching for one index — keeping the three EF providers uniform matches how the codebase already handles this divergence.

mkincaid-bw · 2026-07-15T14:22:01Z

+GO
+CREATE NONCLUSTERED INDEX [IX_Event_OrganizationId]
+    ON [dbo].[Event]([OrganizationId] ASC) WHERE [OrganizationId] IS NOT NULL;
+


Merged PR 7690 adds a new column SendId and new index to this table as well, so your Event.sql is stale.

Done in 6c95b9c — Event.sql now matches main's post-#7690 shape (SendId column plus the filtered IX_Event_OrganizationIdSendIdDate index) with this PR's IX_Event_OrganizationId added after it. The diff against the stacked base branch will show the SendId lines as additions since that branch predates #7690, but the file content now agrees with main so the eventual merge won't clobber it.

mkincaid-bw · 2026-07-15T14:23:44Z

@@ -0,0 +1,4 @@
+CREATE TYPE [dbo].[OrganizationDeleteTaskArray] AS TABLE (


New datatypes should not by created. If you need to pass an object, JSON is the preferred type.

Done in 6c95b9c — the OrganizationDeleteTaskArray type is removed. Organization_DeleteById now takes @OrganizationDeleteTasks NVARCHAR(MAX) = NULL (a JSON array of { Id, TaskType, CreationDate }) parsed with OPENJSON ... WITH, and the Dapper repository serializes with JsonSerializer like the other JSON-taking procs. The migration script also drops the type for anyone who ran the earlier revision.

mkincaid-bw · 2026-07-15T14:27:37Z

-    @OrganizationDeleteTaskId UNIQUEIDENTIFIER = NULL,
-    @OrganizationDeleteTaskType TINYINT = NULL,
-    @OrganizationDeleteTaskCreationDate DATETIME2(7) = NULL
+    @OrganizationDeleteTasks [dbo].[OrganizationDeleteTaskArray] READONLY


Per my previous comment, this should be a VARCHAR(MAX)/NVARCHAR(MAX) and processed with JSON_VALUE/OPENJSON as needed.

Done in 6c95b9c — the parameter is now NVARCHAR(MAX) = NULL and the tasks are read via OPENJSON(@OrganizationDeleteTasks) WITH ([Id] UNIQUEIDENTIFIER, [TaskType] TINYINT, [CreationDate] DATETIME2(7)).

mkincaid-bw · 2026-07-15T14:48:43Z

@@ -9,8 +9,6 @@ BEGIN

    WHILE @BatchSize > 0


I don't generally focus too much on the C# code but my Claude review helper noted this about this procedure (not sure if the design is intentional):

The job/repository layer was correctly redesigned (bounded run budget, lease refresh per batch, explicit "bounded per call" contract), but the only implementation still calls the unchanged Event_DeleteManyByOrganizationId, which loops internally until the entire org's events are deleted (commandTimeout: 3600). A large org can still blow past the 10-minute lease and get reclaimed by a concurrent run.

Good catch — fixed in 6c95b9c. Event_DeleteManyByOrganizationId now takes @MaxRows INT = 50000 and stops once that cap is hit, returning the count so the job calls it again after refreshing the lease via its per-batch progress update; the Dapper commandTimeout drops from 3600 to 300 accordingly. The EF implementation had the same problem (ExecuteDeleteAsync over the whole org) and is now bounded to 1000 rows per call. A large org therefore never holds a single call longer than one bounded chunk, and the 4-minute run budget / 10-minute lease math holds.

PM-33527 initial commit

285b531

github-code-quality Bot found potential problems Jul 1, 2026

View reviewed changes

prograhamming changed the base branch from main to dirt/pm-33527/server-db-combined July 1, 2026 16:33

Banrion requested changes Jul 1, 2026

View reviewed changes

prograhamming added 4 commits July 13, 2026 15:26

PM-33527 project delete task types with Select in EF repository

12d38fe

Map deleteTaskTypes into OrganizationDeleteTask entities via a LINQ Select projection and enqueue them with AddRangeAsync, matching the Dapper repository's pattern instead of mapping inside a foreach loop.

PM-33527 register delete task handlers with TryAddEnumerable

c5b60a9

PM-33527 thread cancellation token through event cleanup delete path

2fcb1f0

github-actions Bot added the DB-migrations-changed label Jul 13, 2026

PM-33527 rename migrations to sort after latest main migrations

a7d6599

prograhamming removed the DB-migrations-changed label Jul 13, 2026

prograhamming marked this pull request as ready for review July 13, 2026 20:52

prograhamming requested review from a team as code owners July 13, 2026 20:52

prograhamming requested review from Banrion and JimmyVo16 and removed request for a team July 13, 2026 20:52

prograhamming added 2 commits July 13, 2026 16:02

PM-33527 fix file encodings and remove unused using for dotnet format

804ef98

PM-33527 adding fixes from other PR

3772c3a

prograhamming added the ai-review Request a Claude code review label Jul 14, 2026

claude Bot reviewed Jul 14, 2026

View reviewed changes

JimmyVo16 approved these changes Jul 14, 2026

View reviewed changes

mkincaid-bw requested changes Jul 15, 2026

View reviewed changes

PM-33527 addressing PR comments

6c95b9c


		namespace Bit.Infrastructure.EntityFramework.Dirt.Models;

		public class OrganizationDeleteTask : Core.Dirt.Entities.OrganizationDeleteTask

		@@ -0,0 +1,4 @@
		CREATE TYPE [dbo].[OrganizationDeleteTaskArray] AS TABLE (

Uh oh!

Conversation

prograhamming commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking & 📔 Objective

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

claude Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prograhamming commented Jul 1, 2026 •

edited

Loading

codecov Bot commented Jul 13, 2026 •

edited

Loading

github-actions Bot commented Jul 13, 2026 •

edited

Loading