Repository navigation
fix(sqlserver): record transitions before the claim and the report take X locks - #11
Merged
Merged
Conversation
Concurrent_relinquishes_and_claims_never_deadlock failed on CI with an absorbed deadlock. The deadlock graphs show claim against claim. Each claim held X locks on its leased jobs and waited for S locks inside the FK check of its Transition Log INSERT. When the plan scans jobs, that check reads the rows that other claimers hold. The claim now writes its Transition Log entries in the same batch as the lease, before the UPDATE. At that point the candidates hold only U locks, and U locks are compatible with S locks. The expiry and relinquish paths already obey this rule. The claim also uses one less round trip. The budget for a claim of 32 jobs decreases from 6 statements to 5.
… locks Concurrent_outcome_reports_never_deadlock failed on CI for this branch with 19 absorbed deadlocks. The fault is older than the claim fix. On main, the test fails in 6 runs out of 6 when it runs alone, with 9 absorbed deadlocks each time. In the full run, the tests before it change the plan cache, so CI usually passes. The report wrote its outcomes first and its Transition Log entries second. The FK check of the transition INSERT can scan jobs, and its S locks wait behind the X locks of concurrent reporters. The report now uses the same order as the claim. One batch locks the fenced rows with UPDLOCK into a table variable, writes the transitions while the rows hold only U locks, and then updates the rows. The U locks keep the fence valid until the UPDATE. The report also uses one less round trip. The drain budget decreases from 3 statements to 2, and the budget with job output from 35 to 34.
SQL Server caches one plan per batch text. The report now writes its transitions in its own batch, so the 96-row lease sweep no longer changed the plan of the report, and the test passed on any lock order. Clear the plan cache, then compile the report plan with a 96-row report. With X locks taken before the transition INSERT, the test now fails 3 runs out of 3. The old setup passed 3 out of 3 on that order.
The deadlock tests for the claim and the report are races, so they catch the old lock order only when the plan loses the race. Two new tests hold an S lock on one batch row in a rival session. U passes the S lock and X does not, so the store blocks at its UPDATE. A dirty read then counts the transition entries that the blocked store already wrote. With the store from main, both tests see 0 of 4 entries and fail on every run.
The report no longer goes through the batch recorder, the prune payload is not always TransitionRow JSON, and one method named its cap twice.
…der note Replace the two repeated deadlock explanations with the one-line reference that the relinquish uses, and document the @Batch columns that InsertTransitionsFromBatch reads.
…ish-deadlock-flake # Conflicts: # src/BackWave.SqlServer/SqlServerJobStore.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two SQL Server tests can fail with absorbed deadlocks (error 1205). Both tests pin that count to 0.
Concurrent_relinquishes_and_claims_never_deadlockfailed on CI onmain.Concurrent_outcome_reports_never_deadlockfailed on CI for the first commit of this PR. Onmain, it fails in 6 runs out of 6 when it runs alone.The claim and the report took X locks on their jobs first, then wrote the Transition Log. The FK check of the transition INSERT can scan
jobs, and the deadlock graphs show that scan wait for S locks on the rows of a concurrent claim or report. Each side holds X locks that the other side scans, so SQL Server kills one side.The claim and the report now write the Transition Log before the
UPDATE, in the same batch:The expiry and relinquish paths already obey this order. All batch writers (claim, report, expiry, relinquish) now share one transition INSERT (
InsertTransitionsFromBatch) and one prune step (PruneTransitionsAsync). The single-rowRecordTransitionAsyncpaths (enqueue, cancel, requeue, cascades, schedule mint) still do theUPDATEfirst.Each path also uses one less round trip:
Test changes
Concurrent_outcome_reports_never_deadlocknow runsALTER DATABASE SCOPED CONFIGURATION CLEAR PROCEDURE_CACHEfirst. Before, an earlier test in the run could leave a narrow plan in the cache, and the test then passed on any lock order.Bounds.MaxClaimBatch(32) rows. The old poison claimed only 32.A_claim_writes_its_transitions_before_it_takes_X_locksandA_report_writes_its_transitions_before_it_takes_X_lockspin the lock order with no race. A rival session holds an S lock on one row of the batch. The store passes it with U, inserts the transitions, then waits at theUPDATE. A dirty read taken during the wait must show all 4 entries.Evidence
Local SQL Server 2022 container, limited to 2 CPUs.
Before (
main, and the first commit of this PR):main, and 7 out of 8 after the first commit.After (
f00a23c):Concurrent_outcome_reports_never_deadlock, aloneConcurrent_relinquishes_and_claims_never_deadlock, alone (both history policies)SqlServerConcurrentMaintenanceTests(10 tests)BackWave.SqlServer.TestsThe build has 0 warnings.
After the merge of
main(with feat(monitor): add a filtered job count #9 and feat: record why a job went back to Scheduled and show Retrying jobs #10): all 273 tests inBackWave.SqlServer.Testspassed, andSqlServerConcurrentMaintenanceTestspassed 3 runs out of 3. The claimOUTPUTnow also returnsretry_cause.Merge Danger
Door: two-way
The change is SQL text inside
SqlServerJobStore. It has no schema change and no change to the stored data, so a revert puts the old statements back.Blast Radius: SQL Server claim and report
Only the SQL Server adapter changes. Every claim and every outcome report on SQL Server runs the new batch, so a fault here stops work on SQL Server fleets. The Transition Log entries keep the same content.