[PM-25520] - Adding a transaction for mutliple repo calls#7555
[PM-25520] - Adding a transaction for mutliple repo calls#7555jrmccannon wants to merge 24 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7555 +/- ##
==========================================
+ Coverage 61.15% 65.65% +4.49%
==========================================
Files 2175 2188 +13
Lines 96792 97381 +589
Branches 8730 8786 +56
==========================================
+ Hits 59196 63938 +4742
+ Misses 35486 31225 -4261
- Partials 2110 2218 +108 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
Fixed Issues (27)Great job! The following issues were fixed in this Pull Request
|
# Conflicts: # bitwarden_license/src/Scim/Users/PostUserCommand.cs
|
Test is failing due to the fact that its creating a SqlServerTestDatabase and it won't be able to pull the connection string to do so. Unsure of how to proceed with this. Do we want to add a SQL server db for this test or just skip it in CI? |
We definitely don't want to skip it in CI, or it'll never be run in practice. The CI job does set up and connect to SQL server, e.g. https://github.com/bitwarden/server/blob/master/.github/workflows/test-database.yml#L131-L132. I assume your issue is caused by the |
mzieniukbw
left a comment
mzieniukbw
left a comment
There was a problem hiding this comment.
Nice, i love the direction. There is a bit duplication between EF and Dapper, but generally good direction in my opinion.
| Guid organizationId, | ||
| ScimProviderType scimProvider) | ||
| { | ||
| await using var transactionScope = await transactionManager.BeginTransactionAsync(IsolationLevel.Serializable); |
There was a problem hiding this comment.
❓ Serializable transaction isolation is the strictest you can get, but it have side effects compared to default, like it can throw error if there was a concurrent write (in different transaciton). Is this by choice here ?
On a side note, this changes the business logic, which i wonder, whether this PR should do.
In most cases this transaction isolation is unnecessary, in which case, it is better to begin the transaction just before the first write, not during the get.
One important note, the copy paste code trend is unavoidable and we will see other places using this isolation level for no reason, just because it was used somewhere else. If we would use serializable isolation level by choice here, i think a reasonable comment should be added explaining why it might be needed here.
@bitwarden/dept-dbops Opinions ?
There was a problem hiding this comment.
The isolation is needed so that we can get the current seat count and ensure that it won't change before we complete our request.
For this specific command, multiple requests come into SCIM grab the organization at the same time and attempt to add their user. Currently, the orgservice#inviteuser method grabs resources multiple times including calling out to stripe. An optimized command for inviting users was created, but that introduced an issue where multiple requests would race to update the seat count and all set it to the same value instead of incrementing it.
3 requests come in => get Org: { Seats: 2 } and each add a seat to the org.Seat value. Each request would set org.Seat to 3 when in reality they should set it to 5 (assuming both Org.Seats are occupied). There's also the ability for an Org to set a limit (Org.MaxSeats: 4). So two should succeed and one should fail.
The SCIM protocol requires the server tell it whether the user was safely provisioned so we have to succeed or fail and can't just switch it to an asynchronous add.
With serialized, this would ensure that the seat count is updated correctly as the requests come in.
So that was the reason for this approach. I'd be interested in another way as this is very heavy handed.
There was a problem hiding this comment.
I agree that serializable isolation level should be avoided if possible. A couple other options are:
- Query the data with an UPDATE lock, which locks only that row rather than everything involved in the transaction.
- Pros: Simple, no schema changes needed
- Cons: You have to write/maintain 4 separate queries (one for each DB)
- Use a row version column
- Pros: Supported natively in SQL Server and EF, can be reused for future concurrency checks
- Cons: Requires new column on table
I'd recommend the row version column, since it requires minimal code changes and retains optimistic concurrency. The main caveat is that instead of blocking other callers upfront, each caller proceeds as normal and you deal with race conditions (if any) afterwards, like this:
- Read the org row — you get back Seats = 9 and RowVersion = 42
- Check the seat limit in C# — passes
- Issue the update: UPDATE Organization SET Seats = 10 WHERE Id = @id AND RowVersion = 42
- If 1 row affected — you won the race, commit
- If 0 rows affected — someone else updated the row between your read and your write; RowVersion is no longer 42, so the WHERE clause matched nothing
Step 5 is the conflict. EF Core detects the 0-rows-affected case and throws DbUpdateConcurrencyException.
try
{
await _organizationRepository.IncrementSeatCountAsync(org);
}
catch (DbUpdateConcurrencyException)
{
// Option A: retry — reload the org and try again
// Option B: surface a conflict error to the SCIM caller
throw new ConflictException("Organization seat count changed concurrently, please retry.");
}There was a problem hiding this comment.
I have a branch i was trying something similar with Polly in order to not just fail everything. I'll give that a try.
There was a problem hiding this comment.
@mkincaid-bw I would also prefer to avoid SERIALIZABLE and use an optimistic solution (even at the expense of a bit of hand-crafting), however we went down this path at @withinfocus 's direction. I would prefer Architecture's confirmation before we spend more time exploring other solutions. (Thoughts on the above suggestions @withinfocus ?)
…transaction manager and updated dapper/ef to use it.
|
| /// Executes an action using the ambient transaction connection (if active) or a new | ||
| /// owned connection. The connection is opened and disposed automatically when owned. | ||
| /// </summary> | ||
| protected async Task<TResult> ExecuteWithConnectionAsync<TResult>( |
There was a problem hiding this comment.
I think we need solid integration test coverage with ITransactionManager and without it, for botj Dapper and EF. Include all edge cases and databases. Without it, it's hard to say if it is working as expected / stable to use in prod.
| /// or creates a new owned connection. The caller must dispose the connection only if | ||
| /// <c>Owned</c> is true. | ||
| /// </summary> | ||
| protected (SqlConnection Connection, DbTransaction? Transaction, bool Owned) GetConnection() |
There was a problem hiding this comment.
Should this be private? Looks like it should just be a helper for the methods below.
| private (DatabaseContext DbContext, IServiceScope? OwnedScope) GetDatabaseContextOrAmbient() | ||
| { | ||
| var holder = TransactionState.Current; | ||
| if (holder?.DbContext is DatabaseContext ambientContext) | ||
| { | ||
| return (ambientContext, null); | ||
| } | ||
|
|
||
| var scope = ServiceScopeFactory.CreateScope(); |
There was a problem hiding this comment.
This way both the begin transaction flow and this have the same type of scope.
| private (DatabaseContext DbContext, IServiceScope? OwnedScope) GetDatabaseContextOrAmbient() | |
| { | |
| var holder = TransactionState.Current; | |
| if (holder?.DbContext is DatabaseContext ambientContext) | |
| { | |
| return (ambientContext, null); | |
| } | |
| var scope = ServiceScopeFactory.CreateScope(); | |
| private (DatabaseContext DbContext, AsyncServiceScope? OwnedScope) GetDatabaseContextOrAmbient() | |
| { | |
| var holder = TransactionState.Current; | |
| if (holder?.DbContext is DatabaseContext ambientContext) | |
| { | |
| return (ambientContext, null); | |
| } | |
| var scope = ServiceScopeFactory.CreateAsyncScope(); |
| if (provider != SupportedDatabaseProviders.SqlServer) | ||
| { | ||
| services.AddPasswordManagerEFRepositories(globalSettings.SelfHosted); | ||
| services.AddSingleton<ITransactionManager, EfTransactionManager>(); |
There was a problem hiding this comment.
| services.AddSingleton<ITransactionManager, EfTransactionManager>(); | |
| services.TryAddSingleton<ITransactionManager, EfTransactionManager>(); |
| else | ||
| { | ||
| services.AddDapperRepositories(globalSettings.SelfHosted); | ||
| services.AddSingleton<ITransactionManager, DapperTransactionManager>(); |
There was a problem hiding this comment.
Same
| services.AddSingleton<ITransactionManager, DapperTransactionManager>(); | |
| services.TryAddSingleton<ITransactionManager, DapperTransactionManager>(); |
| } | ||
| finally | ||
| { | ||
| ownedScope?.Dispose(); |
There was a problem hiding this comment.
This would become:
| ownedScope?.Dispose(); | |
| if (ownedScope is not null) | |
| { | |
| await ownedScope.DisposeAsync(); | |
| } |
|
|
||
| public async Task CommitAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| if (_holder.Doomed) |
There was a problem hiding this comment.
| if (_holder.Doomed) | |
| ObjectDisposedException.ThrowIf(_disposed, this); | |
| if (_holder.Doomed) |
|
|
||
| public async Task RollbackAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| _holder.Doomed = true; |
There was a problem hiding this comment.
Same here
| _holder.Doomed = true; | |
| ObjectDisposedException.ThrowIf(_disposed, this); | |
| _holder.Doomed = true; |
| } | ||
|
|
||
| var scope = ServiceScopeFactory.CreateScope(); | ||
| return (GetDatabaseContext(scope), scope); |
There was a problem hiding this comment.
There is a bug happening here if secrets manager code tries to use a transaction, it would just silently not use a transaction when it could.
| return (GetDatabaseContext(scope), scope); | |
| var context = GetDatabaseContext(scope); | |
| if (holder is not null) | |
| { | |
| context.UseTransaction(holder.Transaction); | |
| holder.DbContext = context; | |
| holder.Scope = scope; | |
| } | |
| return (context, scope); |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
| catch | ||
| { | ||
| // Best-effort rollback; connection may already be broken. | ||
| } |
5 participants
🎟️ Tracking
PM-25520
📔 Objective
Adding a transaction around subsequent repo calls.
📸 Screenshots