Don't commit external transaction in BackgroundJobStateChanger

Author: odinserj

src/Hangfire.Core/States/BackgroundJobStateChanger.cs

@@ -160,7 +160,10 @@ public IState ChangeState(StateChangeContext context)
                         }
                     }
 
-                    transaction.Commit();
+                    if (context.Transaction == null)
+                    {
+                        transaction.Commit();
+                    }
 
                     context.ProcessedJob = backgroundJob;
                     return appliedState;

tests/Hangfire.Core.Tests/Mocks/StateChangeContextMock.cs

@@ -26,10 +26,12 @@ public StateChangeContextMock()
                 () => new StateChangeContext(
                     Storage.Object,
                     Connection.Object,
+                    Transaction?.Object,
                     BackgroundJobId,
                     NewState.Object,
                     ExpectedStates,
                     DisableFilters,
+                    CompleteJob?.Object,
                     CancellationToken,
                     EmptyProfiler.Instance,
                     ServerId,
@@ -38,6 +40,8 @@ public StateChangeContextMock()
 
         public Mock<JobStorage> Storage { get; set; }
         public Mock<IStorageConnection> Connection { get; set; }
+        public Mock<JobStorageTransaction> Transaction { get; set; }
+        public Mock<IFetchedJob> CompleteJob { get; set; }
         public string BackgroundJobId { get; set; }
         public Mock<IState> NewState { get; set; }
         public IEnumerable<string> ExpectedStates { get; set; }

tests/Hangfire.Core.Tests/States/BackgroundJobStateChangerFacts.cs

@@ -495,6 +495,45 @@ public void ChangeState_DoesNotInvokeApplyStateFilters_WhenFiltersDisabled()
             _transaction.Verify(x => x.Commit());
         }
 
+        [Fact]
+        public void ChangeState_WithTransaction_PassesTheGivenTransaction_AndDoesNotCommitTheImplicitOne()
+        {
+            _context.Transaction = new Mock<JobStorageTransaction>();
+            var stateChanger = CreateStateChanger();
+
+            stateChanger.ChangeState(_context.Object);
+
+            _stateMachine.Verify(x => x.ApplyState(It.Is<ApplyStateContext>(
+                ctx => ctx.Transaction == _context.Transaction.Object)));
+
+            _connection.Verify(x => x.CreateWriteTransaction(), Times.Never);
+            _transaction.Verify(x => x.Commit(), Times.Never);
+        }
+
+        [Fact]
+        public void ChangeState_WithTransaction_DoesNotCommitAndDoesNotDisposeTheExplicitTransaction()
+        {
+            _context.Transaction = new Mock<JobStorageTransaction>();
+            var stateChanger = CreateStateChanger();
+
+            stateChanger.ChangeState(_context.Object);
+
+            _context.Transaction.Verify(x => x.Commit(), Times.Never);
+            _context.Transaction.Verify(x => x.Dispose(), Times.Never);
+        }
+
+        [Fact]
+        public void ChangeState_WithTransaction_AcquiresATransactionLevelLockInstead()
+        {
+            _context.Transaction = new Mock<JobStorageTransaction>();
+            var stateChanger = CreateStateChanger();
+
+            stateChanger.ChangeState(_context.Object);
+
+            _context.Transaction.Verify(x => x.AcquireDistributedLock($"job:{JobId}:state-lock", It.IsAny<TimeSpan>()));
+            _connection.Verify(x => x.AcquireDistributedLock(It.IsAny<string>(), It.IsAny<TimeSpan>()), Times.Never);
+        }
+
         private BackgroundJobStateChanger CreateStateChanger()
         {
             return new BackgroundJobStateChanger(_filterProvider.Object, _stateMachine.Object);