Skip to content

[PM-39455] Reuse the connection and transaction during user key rotation - #8165

Open
mzieniukbw wants to merge 2 commits into
mainfrom
km/pm-39455-ef-user-key-rotation-re-use-connection-transaction
Open

[PM-39455] Reuse the connection and transaction during user key rotation#8165
mzieniukbw wants to merge 2 commits into
mainfrom
km/pm-39455-ef-user-key-rotation-re-use-connection-transaction

Conversation

@mzieniukbw

@mzieniukbw mzieniukbw commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39455

📔 Objective

Replace UpdateEncryptedDataForKeyRotation with the generic DatabaseTransactionAction, which takes a DbConnection and DbTransaction rather than the SQL Server specific SqlConnection and SqlTransaction. This matches the delegate introduced in #7548, which is planned to get merged first.

Every EF repository that takes part in a key rotation now binds its DatabaseContext to the caller's connection and transaction through GetTransactionalDatabaseContext, instead of opening a second connection. Opening a second connection deadlocks on single-writer providers such as SQLite, and left each write outside the rotation transaction on the others, which breaks integration tests.

📸 Screenshots

Replace UpdateEncryptedDataForKeyRotation with the generic DatabaseTransactionAction, which takes a DbConnection and DbTransaction rather than the SQL Server specific SqlConnection and SqlTransaction. This matches the delegate introduced in #7548.

Every EF repository that takes part in a key rotation now binds its DatabaseContext to the caller's connection and transaction through GetTransactionalDatabaseContext, instead of opening a second connection. Opening a second connection deadlocks on single-writer providers such as SQLite, and left each write outside the rotation transaction on the others, which breaks integration tests.
@mzieniukbw
mzieniukbw requested review from a team as code owners August 7, 2026 13:20
@mzieniukbw mzieniukbw added the t:tech-debt Change Type - Tech debt label Aug 7, 2026
@mzieniukbw
mzieniukbw requested a review from a team as a code owner August 7, 2026 13:20
jrmccannon
jrmccannon previously approved these changes Aug 7, 2026

@jrmccannon jrmccannon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from AC

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 32.75862% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.49%. Comparing base (cded079) to head (8e9bdbc).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...ure.EntityFramework/Repositories/UserRepository.cs 50.00% 5 Missing ⚠️
...per/Auth/Repositories/EmergencyAccessRepository.cs 0.00% 4 Missing ⚠️
...ucture.Dapper/Tools/Repositories/SendRepository.cs 0.00% 4 Missing ⚠️
...ture.Dapper/Vault/Repositories/CipherRepository.cs 0.00% 4 Missing ⚠️
...ture.Dapper/Vault/Repositories/FolderRepository.cs 0.00% 4 Missing ⚠️
...ent/Repositories/UserSignatureKeyPairRepository.cs 0.00% 2 Missing ⚠️
...Console/Repositories/OrganizationUserRepository.cs 0.00% 2 Missing ⚠️
...ork/Auth/Repositories/EmergencyAccessRepository.cs 0.00% 2 Missing ⚠️
.../Auth/Repositories/WebAuthnCredentialRepository.cs 0.00% 2 Missing ⚠️
...e.EntityFramework/Repositories/DeviceRepository.cs 0.00% 2 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8165      +/-   ##
==========================================
+ Coverage   63.02%   67.49%   +4.47%     
==========================================
  Files        2316     2315       -1     
  Lines      100453   100534      +81     
  Branches     9037     9043       +6     
==========================================
+ Hits        63306    67857    +4551     
+ Misses      34962    30392    -4570     
- Partials     2185     2285     +100     

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

Comment thread test/Infrastructure.IntegrationTest/DatabaseTransactionActionTestHelper.cs Dismissed

@harr1424 harr1424 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tools-owned changes look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants