Allocate item numbers without a read-after-write lock upgrade #252
Description
Problem
Under Serializable, the access check read the project row and held a shared lock on it until commit. Allocating the next item number then had to raise that lock to exclusive. Two creations in one project each held a shared lock and each waited for the other to let go, so SQL Server broke the cycle by killing one — reaching the agent as a bare failure of task_create.
Confirmed, not assumed
The deadlock was reproduced against real SQL Server 2019 through LocalDB before anything was changed. Eight concurrent task_create calls in one project failed every time:
Transaction (Process ID 76) was deadlocked on lock resources with another process and has been chosen as the deadlock victim.
Solution
ProjectItemService.Reserve takes an update lock on the project row immediately after the transaction opens, before anything reads the project. Two update locks are not compatible, so the second creation waits there and no lock is ever raised. Called in all four paths that draw a number: task creation, memory creation, memory proposal drafts and proposal batches. SQLite has no row locks and skips the step.
Allocate now refuses a call without an ambient transaction instead of silently handing two callers the same number. The table name for the lock hint is read from the EF model, so a schema rename cannot leave it pointing at a table that no longer exists.
The read-after-write in Allocate was kept. It is correct while the row is held, which is now enforced rather than assumed, and it stays provider-neutral — no OUTPUT/RETURNING dialect split on the one code path that assigns identities.
Acceptance
- Concurrent creation in one project does not deadlock under SQL Server Serializable semantics.
- Allocated numbers stay unique.
- Calling the allocation outside a transaction is rejected.
- The existing concurrency test still passes; new tests cover the read-then-write order.
Measurements
| before | after | |
|---|---|---|
| Concurrency tests (SQL Server) | 3 of 3 failed, deadlock | 0 of 20 runs failed |
| Full suite | 105 green | 106 green |
Each new test was run against the unfixed code and does fail there, so none of them is a vacuous assertion.
Retry policy: deliberately not added
EnableRetryOnFailure does not fail at BeginTransactionAsync — that call is accepted. The first operation inside the transaction throws instead, demanding CreateExecutionStrategy(). Measured, not recalled. Wrapping all eleven explicit transactions is its own decision with its own re-run semantics, so it moved to #253 rather than riding along here.
Limits
The new tests need a real SQL Server and use LocalDB; without it they report inconclusive rather than failing, so a machine without LocalDB silently loses this coverage. Reserve costs one extra round trip per creation on SQL Server.
Deployed in e8cc315.