[PM-25056] - Deadlock testing fix

[jrmccannon]

🎟️ Tracking

PM-25056

📔 Objective

This identified the root cause for the testing framework deadlock.

When tests are being run in parallel, tests that called OrganizationRepo.DeleteAsync would call the giant sproc that would go across all the other tables to clean up related data. Each opened up its own transaction and would lock tables. This caused the intermittent deadlocks across the different tests as they were all locking and trying to delete records in the same tables.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR addresses intermittent deadlocks in the integration test suite that surfaced when tests ran in parallel. The root cause was identified as OrganizationRepository.DeleteAsync invoking a large cross-table cleanup sproc from many concurrent tests, causing transactions to contend on the same tables. The fix removes the cleanup DeleteAsync calls from integration tests (treating the test databases as ephemeral) and switches test entity ID generation from Guid.NewGuid() to CoreHelpers.GenerateComb() to produce sequential, index-friendly GUIDs. Test assertions remain correctly scoped by the created user/organization IDs, so removing cleanup does not affect result isolation.

Code Review Details

No findings.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.59%. Comparing base (d9aef5c) to head (050e2fb).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7478      +/-   ##
==========================================
+ Coverage   58.75%   63.59%   +4.84%     
==========================================
  Files        2071     2078       +7     
  Lines       91252    91526     +274     
  Branches     8130     8141      +11     
==========================================
+ Hits        53611    58206    +4595     
+ Misses      35726    31308    -4418     
- Partials     1915     2012      +97     

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

[github-actions]

Checkmarx One – Scan Summary & Details – 6310eedf-68bd-40ba-bebe-a172ebdab8ee

---

New Issues (1)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	src/Api/Vault/Controllers/CiphersController.cs: 287	details Method at line 287 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows t... Attack Vector

[sven-bitwarden]

https://kevsoft.net/2025/05/14/banning-api-calls-in-dotnet-without-writing-your-own-analyzer.html

Could we add the banned API analyzer to our integration tests, so that we get an immediate red flag warning if we try to use the method that causes deadlocks? It's a Microsoft package, but not sure if there's some difficulties with adding it. We would also need to just bypass the warnings for the test files that are trying to test this method.

[jrmccannon]

Could we add the banned API analyzer to our integration tests, so that we get an immediate red flag warning if we try to use the method that causes deadlocks? It's a Microsoft package, but not sure if there's some difficulties with adding it. We would also need to just bypass the warnings for the test files that are trying to test this method.

That could definitely be something added later on. We'd have to review the package and document the tool for potentially more future uses. @sven-bitwarden

[eliykat]

As discussed, most of these calls are because we delete our test data after each test. However, test databases should be treated as ephemeral; cleaning up our own test data just makes tests slower and competes for locks.

Please delete all test cleanup logic instead. If we're still having deadlock issues, that should ideally be addressed at a more general level (e.g. how testing framework or the db is configured).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[eliykat]

Thanks!