PM-33527 Database SQL scripts

[prograhamming]

🎟️ 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.

Server side changes are implemented in #7517

📸 Screenshots

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.96%. Comparing base (705995d) to head (ea479d8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7616      +/-   ##
==========================================
+ Coverage   60.55%   64.96%   +4.41%     
==========================================
  Files        2142     2144       +2     
  Lines       94847    94897      +50     
  Branches     8487     8487              
==========================================
+ Hits        57430    61648    +4218     
+ Misses      35401    31142    -4259     
- Partials     2016     2107      +91     

☔ View full report in Codecov by Sentry.
📢 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.

[Banrion]

@claude Please add a review

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR introduces the database foundation for GDPR-compliant event log cleanup: a new OrganizationEventCleanup tracking table, MSSQL stored procedures, a Dapper repository, and integration tests. The lease-based claim pattern (ClaimNextPending using UPDLOCK, READPAST with a CTE + UPDATE + OUTPUT) is correctly implemented for atomic concurrent worker coordination, and the integration test suite exercises the key concurrency, staleness, and max-failure scenarios. Prior review iterations addressed the major issues (atomic claim, DI registration, SetNewId(), stored-procedure naming, datetime parameter passing). The intentional omission of EF Core implementations is documented and justified — self-hosted operators on EF providers can drop their event table directly.

Code Review Details

No new actionable findings beyond those already addressed in earlier review rounds.

[mkincaid-bw]

This PR creates a new table and modifies an existing table but there are no EF migrations to handle these changes for self-host/lite customers. Is that intentional?

[mkincaid-bw]

ℹ️ Just noting that Events for our cloud database are kept in Azure Table storage, not in the SQL database, so this index would only be applicable to self-host/lite customers.

[prograhamming]

Removed.

[mkincaid-bw]

This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateProgress.

[prograhamming]

fixed.

[mkincaid-bw]

When updating a value in the database, the value should be generated in code and passed in as parameters to the stored procedure.. We should not be using built-in SQL functions like SYSUTCDATETIME or GETUTCDATE

[prograhamming]

fixed.

[mkincaid-bw]

This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateCompleted.

[prograhamming]

fixed.

[mkincaid-bw]

See previous comment regarding SYSUTCDATETIME

[prograhamming]

fixed.

[mkincaid-bw]

This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateStarted.

[prograhamming]

fixed.

[mkincaid-bw]

See previous comments regarding SYSUTCDATETIME

[prograhamming]

fixed.

[mkincaid-bw]

This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateError.

[prograhamming]

fixed.

[mkincaid-bw]

See previous comments regarding SYSUTCDATETIME

[prograhamming]

fixed.

[prograhamming]

This PR creates a new table and modifies an existing table but there are no EF migrations to handle these changes for self-host/lite customers. Is that intentional?

Yes because my thinking is if they are self-hosted and they want to delete their organization they should have the ability to just delete the table or database on their server since Events are stored in a table for self-hosted/lite customers. @mkincaid-bw

[prograhamming]

@claude Please re-review

[claude]

Claude finished @prograhamming's task in 2m 49s —— View job

---

Code Review — PM-33527 Database SQL scripts (re-review)

• Fetch latest branch state
• Review changed files (table, sprocs, repository, entity, migration)
• Verify previously raised issues are fixed
• Identify any new issues
• Post review feedback

---

Summary

Most prior feedback has been addressed. The atomic claim implementation is solid, naming conventions are now consistent with the contributing guide, and the runaway Event table index has been removed. A few smaller items remain.

✅ Resolved since last review

Prior finding	Status
ReadNextPending not atomic (concurrent workers dequeue same row)	✅ Fixed via ClaimNextPending CTE with UPDLOCK, READPAST + OUTPUT inserted.*, with stale-lease re-claim
SetNewId() not called in CreateAsync	✅ Fixed (OrganizationEventCleanupRepository.cs:19)
CreationDate not defaulted on entity	✅ Fixed (OrganizationEventCleanup.cs:10)
SELECT * in claim sproc	✅ Fixed — explicit column list
DI registration missing	✅ Fixed (DapperServiceCollectionExtensions.cs:85)
SYSUTCDATETIME inside sprocs	✅ Fixed — @Now passed in from code for all update sprocs
Sproc names not following Update* convention	✅ Fixed (UpdateProgress, UpdateError, UpdateCompleted)
Datetime columns ending At instead of Date	✅ Fixed (CreationDate, RevisionDate, StartDate, CompletedDate)
IX_Event_OrganizationId deploy risk on large table	✅ Fixed — index removed entirely (Events not in SQL for cloud)
review-inline-comments.md / review-summary.md artifacts at repo root	✅ Fixed — removed
EF Core implementation missing	✅ Intentional per author response — self-host customers manage their own event tables

🟠 Important — still open

1. SonarCloud reliability gate still failing
The latest SonarCloud run (2026-05-21) reports E Reliability Rating on New Code — required ≥ D. Please address before merge.

2. No tests added
.claude/CLAUDE.md requires unit tests for new feature development. No tests cover OrganizationEventCleanupRepository or the claim/update sprocs. Consider at minimum:

• A Dapper integration test (in test/Infrastructure.IntegrationTest/Dirt/...) covering CreateAsync → ClaimNextPendingAsync → UpdateProgressAsync → UpdateCompletedAsync happy path
• A concurrent-claim test confirming two simultaneous ClaimNextPendingAsync calls don't return the same row
• A lease-recovery test confirming a stale RevisionDate allows re-claim

🟡 Minor

3. Hardcoded 10-minute lease in ClaimNextPendingAsync
OrganizationEventCleanupRepository.cs:33 — now.AddMinutes(-10) is a magic number. Consider a named constant on the repository or a GlobalSettings/options value so operators can tune lease duration without a code change.

4. Filtered index would be cheaper for queue read pattern
OrganizationEventCleanup.sql:15-16 — both reads filter CompletedDate IS NULL, so a filtered non-clustered index would stay small as completed rows accumulate:

CREATE NONCLUSTERED INDEX [IX_OrganizationEventCleanup_Pending]
    ON [dbo].[OrganizationEventCleanup]([CreationDate] ASC)
    INCLUDE ([StartDate], [RevisionDate])
    WHERE [CompletedDate] IS NULL;

5. Attempts semantics still ambiguous
OrganizationEventCleanup_UpdateError.sql:12 is the only place Attempts is incremented, so it actually counts failures. Rename to FailureCount, or also increment in ClaimNextPending when a fresh lease is taken — current name doesn't match behavior.

6. No terminal-failure / max-attempts cutoff
A permanently broken row will keep cycling through claim → fail → lease expires → claim forever. Either gate ClaimNextPending on Attempts < @MaxAttempts, or add a Status / FailedAt column for explicit terminal state. Without it, a poison row blocks the queue head indefinitely (and the operator only finds out via LastError).

7. Whitespace-only change to Event.sql
src/Sql/dbo/Dirt/Tables/Event.sql now has an extra trailing blank line and no other change. Probably worth reverting to keep the diff clean — touching Event.sql will be confusing for future archaeology since the change is invisible.

8. @LeaseExpiry parameter name
OrganizationEventCleanup_ClaimNextPending.sql:3 — the parameter is the threshold for "stale", not when the new lease expires. @StaleLeaseThreshold or @LeaseExpiredBefore would read more naturally next to the [RevisionDate] < @LeaseExpiry predicate.

---

The shape is much closer to mergeable. The SonarCloud reliability finding and the missing tests are the two items I would not ship without; the rest are polish.

• Branch: dirt/pm-33527/db-orphaned-event-logs
•

[prograhamming]

@claude re-run the review

[claude]

Claude finished @prograhamming's task in 3m 34s —— View job

---

Code Review — PM-33527 Database SQL scripts (re-review)

• Fetch latest branch state
• Review changed files (table, sprocs, repository, entity, migration, tests)
• Verify previously raised issues are fixed
• Identify any new issues
• Post review feedback

---

Summary

Nearly all prior feedback has been addressed cleanly. The atomic claim works correctly, naming conventions match the SQL style guide, the Attempts → FailureCount rename now matches behavior, a MaxFailureCount gate prevents poison rows from blocking the queue head, the magic 10-minute lease is now a named constant, and integration tests have been added covering happy path, concurrent claim, stale-lease re-claim, and max-failure cutoff. Good iteration.

One important new finding around the integration tests and the prior SonarCloud failure remain.

✅ Resolved since last review

Prior finding	Status
Attempts semantics ambiguous	✅ Renamed to FailureCount
No terminal-failure / max-attempts cutoff	✅ MaxFailureCount gate in ClaimNextPending
Hardcoded 10-minute lease	✅ LeaseDurationMinutes const (OrganizationEventCleanupRepository.cs:13)
@LeaseExpiry parameter name	✅ Renamed to @StaleLeaseThreshold
Whitespace-only Event.sql change	✅ Reverted
No tests added	✅ Four integration tests added — happy path, concurrent claim, stale-lease re-claim, max-failure cutoff

🟠 Important — still open

1. Integration tests will fail on EF providers (Postgres/MySQL/SQLite/MariaDB)

OrganizationEventCleanupRepositoryTests.cs uses [Theory, DatabaseData], which expands to every configured DB provider in CI (test-database.yml:125-138 configures SqlServer, Postgres, MySQL, SQLite, MariaDB). For each provider, DatabaseDataAttribute.cs:183 does _serviceProvider.GetRequiredService(parameter.ParameterType) to inject IOrganizationEventCleanupRepository.

Because IOrganizationEventCleanupRepository is only registered in AddDapperRepositories (no EF counterpart by design), GetRequiredService will throw InvalidOperationException on the four EF runs — all of those test theories will fail in CI.

There's no existing helper to scope a [DatabaseData] test to SqlServer-only. Options:

• Recommended: add a SupportedDatabaseProviders[] OnlyOn { get; set; } (or similar) to DatabaseDataAttribute and skip theory rows where the provider isn't in the list. Use it here: [Theory, DatabaseData(OnlyOn = [SupportedDatabaseProviders.SqlServer])].
• Alternative: register a Bit.Infrastructure.EntityFramework no-op stub of IOrganizationEventCleanupRepository that throws NotSupportedException, and mark these tests Skip = "SqlServer-only" for non-Dapper rows.

Either way the current state will break CI on the integration test workflow.

2. SonarCloud reliability gate still failing

Latest SonarCloud run (2026-05-21) still reports E Reliability Rating on New Code — required ≥ D. Worth opening the analysis to see which file/line; this gate is blocking merge.

🟡 Minor

3. Filtered index on the queue would stay small as completed rows accumulate

OrganizationEventCleanup.sql:15-16 — the current (CompletedDate, CreationDate) index works, but both queries filter on CompletedDate IS NULL. A filtered index would be smaller and a better fit:

CREATE NONCLUSTERED INDEX [IX_OrganizationEventCleanup_Pending]
    ON [dbo].[OrganizationEventCleanup]([CreationDate] ASC)
    INCLUDE ([FailureCount], [StartDate], [RevisionDate])
    WHERE [CompletedDate] IS NULL;

Not blocking; volume is low enough either way.

4. BackdateRevisionDateAsync test helper uses SYSUTCDATETIME()

OrganizationEventCleanupRepositoryTests.cs:142 — the rest of the codebase moved to pushing time from C#. In a test helper it's harmless, but to stay consistent with the convention the reviewer enforced on the sprocs, you could pass the computed timestamp from the test instead.

5. LastError is NVARCHAR(MAX)

OrganizationEventCleanup.sql:10 — full stack traces from a worker exception could be very large. Consider capping at NVARCHAR(2000) or NVARCHAR(4000). Not security-sensitive since this isn't PII or vault data, just hygiene.

6. Test UpdateProgressAsync_And_UpdateCompletedAsync_UpdatesRow doesn't assert RevisionDate

OrganizationEventCleanupRepositoryTests.cs:31-44 — would be worth asserting that RevisionDate advanced after each call, since that's the field driving lease semantics.

---

The atomic-claim shape is solid, the failure-cap closes the previous poison-row hole, and the lease const/parameter renames make the code much easier to follow. Item 1 (EF test runs) and 2 (SonarCloud) are the two things I'd want resolved before merge — the rest is polish.
• Branch: dirt/pm-33527/db-orphaned-event-logs

[sonarqubecloud]

Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ D)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[mkincaid-bw]

@prograhamming I was looking at this with my team and @withinfocus, and it's hard for us to review it only seeing the DB code. Can you please point us to the rest of the change so we can get the whole picture?

[Banrion]

@prograhamming I was looking at this with my team and @withinfocus, and it's hard for us to review it only seeing the DB code. Can you please point us to the rest of the change so we can get the whole picture?

@mkincaid-bw , #7517 is the server side pull request. I added the link in the description since it was left out while the other pull request linked to this one. The jira ticket also has a link to the GDPR document.

Is this the information you need? We try to keep pull request reviews on the lighter side for easier reviews, but if there is a better way to handle this, we are open to ideas!

[withinfocus]

It would be ideal to have these changes in one PR -- the DB work is concise and tightly-coupled to the logic, so the DB engineers' review really must have it to provide any accurate feedback, as would any AI review. This has brought up some questions from me as well, and not just about the DB work.

Aside: you invoked direct claude.ai review here and bypassed our configured agent which operates rather differently; let's be sure #7616 (comment) (the sticky comment) is updating. I didn't know this would happen.

Reviewing this PR together with #7517, I want to flag a design issue before more shape is locked in. OrganizationDeleteCommand.DeleteAsync does:

await _organizationRepository.DeleteAsync(organization);          // org gone
await _organizationEventCleanupRepository.CreateAsync(new OrganizationEventCleanup
{
    OrganizationId = organization.Id,                              // org no longer exists
});

If the second call fails then this gap remains. This needs to be one transaction.

Second, OrganizationDeleteCommand already does four cross-trust-boundary cleanups: Stripe cancellation, Send files in Blob Storage, cipher attachments in Blob Storage, and now Table Storage events. Three of the four have no durability story today and a transient failure silently orphans data in an external system.

This table is the right primitive to solve that class of problem, not just events. I would like to see this renamed to OrganizationDeleteTask with a TaskType discriminator column. Populate only EventsCleanup in this PR pair (scope stays the same).

Future cross-system cleanups (Sends, attachments, Stripe) can ride the same checkpoint / retry / observability surface without another bespoke table. This is a big deal for us as we scale.

Also, some minor things:

• DeleteManyByOrganizationIdAsync fans out and this will throttle. Consider using bounded concurrency.
• totalDeleted is counted before transactions complete; partial failure inflates the progress count. Reporting bug, not correctness.
• LastError truncated to 4000 chars still leaks row-key identifiers from Azure SDK exception messages (can include UserId=..., CipherId=...). Sanitize or use exception type + stable code instead of ex.Message.
• The EF OrganizationRepository.DeleteAsync reordering (SaveChangesAsync before CommitAsync) looks like a real bug fix and should probably be its own PR.

[Banrion]

Closing in favor of a new pull request combining the database changes with the endpoint changes and updates as per feedback. New pull request: #7783