PM-33527 - Don't leave orphaned event logs in storage - #7783
PM-33527 - Don't leave orphaned event logs in storage#7783prograhamming wants to merge 51 commits into
Conversation
…:bitwarden/server into dirt/pm-33527/remove-orphaned-event-logs
…warden/server into dirt/pm-33527/db-orphaned-event-logs
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review DetailsNo new findings. The two items below are already tracked in existing unresolved review threads and are not re-posted:
Notes from validation:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public async Task<int> DeleteManyByOrganizationIdAsync(Guid organizationId) | ||
| { | ||
| using var scope = ServiceScopeFactory.CreateScope(); | ||
| var dbContext = GetDatabaseContext(scope); | ||
| return await dbContext.Events | ||
| .Where(e => e.OrganizationId == organizationId) | ||
| .ExecuteDeleteAsync(); | ||
| } |
There was a problem hiding this comment.
🔵 Suggested: This EF implementation deletes every event for the organization in a single unbounded ExecuteDeleteAsync(), while the other implementations of DeleteManyByOrganizationIdAsync deliberately batch to keep transactions short — the Dapper stored procedure (Event_DeleteManyByOrganizationId) deletes in TOP(1000) chunks, and the Table Storage version caps work per call. For an organization with a very large Event history this single statement can produce a long-running transaction with lock escalation and transaction-log growth.
The cleanup job is cloud-only and currently runs against Table Storage, so this path isn't exercised today, but since it satisfies the same interface contract it's worth batching here too for consistency and safety if it's ever wired up. No change required if this method is intentionally a placeholder.
There was a problem hiding this comment.
Leaving this one as-is intentionally: Take() + ExecuteDeleteAsync doesn't translate across all three EF providers, and this path is only reachable on self-host EF deployments where a single set-based delete is acceptable. If it ever becomes a problem, the durable delete-task queue retries on failure, so a provider-specific batched implementation can be added later without contract changes.
| @@ -0,0 +1,45 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationDeleteTask_ClaimNextPending] | |||
There was a problem hiding this comment.
The name of the proc doesn't follow the T-SQL guidelines. This should be named something like OrganizationDeleteTask_UpdateClaimNextPending.
There was a problem hiding this comment.
| BEGIN | ||
| DECLARE @OrganizationDeleteTaskDate DATETIME2(7) = COALESCE(@OrganizationDeleteTaskCreationDate, SYSUTCDATETIME()) | ||
|
|
||
| INSERT INTO [dbo].[OrganizationDeleteTask] |
There was a problem hiding this comment.
Any reason why you don't just call OrganizationDeleteTask_Create here?
There was a problem hiding this comment.
This changed shape in the follow-up PR #7910: the scalar task parameters were replaced with a table-valued parameter (OrganizationDeleteTaskArray) and a set-based INSERT … SELECT, so any number of task types can be enqueued atomically in the same transaction as the delete. The single-row OrganizationDeleteTask_Create proc no longer fits that shape — it's still used for the standalone create path.
|
|
||
| WHILE @BatchSize > 0 | ||
| BEGIN | ||
| BEGIN TRANSACTION Event_DeleteManyByOrganizationId |
There was a problem hiding this comment.
This explicit transaction is unnecessary and potentially harmful. Since the only thing affected by the transaction in this case is the DELETE statement, it's already guaranteed to be atomic (it's a single statement). However, with the explicit transaction and no error handling, it's possible that SQL Server could error on the statement and leave the transaction uncommitted.
If you want to guarantee that all records for an org are deleted (or none in the event of a failure), you'd need to move the BEGIN/COMMIT outside of the WHILE loop, and add a TRY/CATCH block to handle any errors. See this for an example.
If you don't care about all or nothing, then the explicit transaction should be removed completely.
There was a problem hiding this comment.
| @@ -0,0 +1,27 @@ | |||
| CREATE PROCEDURE [dbo].[Event_DeleteManyByOrganizationId] | |||
There was a problem hiding this comment.
❓Is this proc being added mainly for parity or do you expected self-host clients to run it? The main reason I ask is because the functionality does not match cloud (where looping is handled in code vs in the DB). The first call to the proc from code will cause this to loop to completion, and this may timeout for large customers.
There was a problem hiding this comment.
Self-host clients are expected to run it — cloud stays on the Table Storage implementation. Looping in the proc keeps the same contract as the other implementations ("delete until done" per call). On the timeout concern, PR #7910 mitigates it from a few directions: each batch is now a seek via the new IX_Event_OrganizationId filtered index rather than a table scan, the explicit transaction was removed so every batch auto-commits independently, and the caller uses a 3600s command timeout. If a very large customer still times out, progress persists and the durable delete-task queue simply re-claims and resumes on the next run.
| DELETE TOP(@BatchSize) | ||
| FROM | ||
| [dbo].[Event] | ||
| WHERE |
There was a problem hiding this comment.
There is no index on the OrganizationId column, so this delete batch will cause thousands of table scans for large customers. Based on my previous question (parity vs actual use), you may want to add a filtered index on OrganizationId where OrganizationId IS NOT NULL.
There was a problem hiding this comment.
Added in PR #7910 (commit 3772c3a): a filtered index IX_Event_OrganizationId ON [dbo].[Event]([OrganizationId]) WHERE [OrganizationId] IS NOT NULL per your suggestion, plus the EF Core equivalent (unfiltered, since MySQL doesn't support filtered indexes — same approach as IX_Notification_OrganizationId / IX_SecurityTask_OrganizationId) with generated migrations for all three EF providers.
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.
|
Superseded by #8182 |
4 participants
🎟️ Tracking
PM-33527
📔 Objective
In order to stay within GDPR compliance when a organization wants to be deleted we need to remove the event logs from Azure Table Storage. This PR is the database changes needed in order to delete the event logs using jobs and long running processes.