Saga Updates Fail When MS SQL Server Default Connections Options No Count is On

[wleader]

Describe the bug

Description

In Microsoft SQL Sever, the default connection options can be configured. One of the options is to Set NOCOUNT at the server level via SQL Server Management Studio or sp_configure 'user options'. When NOCOUNT is ON at the server level, after a saga message is handled, an exception is logged.

Optimistic concurrency violation when trying to save saga <saga name> <guid>. Expected version 1.

Expected behavior

The Saga Data should be updated, and the message handler completes. No matter what the SQL Server's NO COUNT configuration is.

Actual behavior

The message fails and ends up in the Error queue.

Versions

Seen in NServiceBus.Persistence.Sql version 7.0.6

Steps to reproduce

Set NOCOUNT OFF at the MS SQL Server level configuration, and run a saga using MS SQL persistence.

Relevant log output

Additional Information

Workarounds

Set the SQL Server NOCOUNT option to OFF
https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/configure-the-user-options-server-configuration-option?view=sql-server-ver17

Possible solutions

This appears to be coming from:

NServiceBus.Persistence.Sql/src/SqlPersistence/Saga/SagaPersister_Update.cs

Line 30 in f8c1dc2

var affected = await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);

The code is checking for a Row Count of 1 as returned from ExecuteNonQueryAsync method which uses the count returned by SQL server.

The code is executing a statement as defined here:

NServiceBus.Persistence.Sql/src/SqlPersistence/Saga/SqlDialect_MsSqlServer.cs

Line 69 in f8c1dc2

update {tableName}

update {tableName}
set
    Data = @Data,
    PersistenceVersion = @PersistenceVersion,
    SagaTypeVersion = @SagaTypeVersion,
    Concurrency = @Concurrency + 1{correlationSet}
where
    Id = @Id and Concurrency = @Concurrency

I think this could be made to work when NOCOUNT is ON, by having the statement deliberately select the rowcount:

update {tableName}
set
    Data = @Data,
    PersistenceVersion = @PersistenceVersion,
    SagaTypeVersion = @SagaTypeVersion,
    Concurrency = @Concurrency + 1{correlationSet}
where
    Id = @Id and Concurrency = @Concurrency;
select @@ROWCOUNT

There would also need to be a change to SagaPersister_Update.cs to use ExecuteScalarAsync instead of ExecuteNonQueryAsync. Though this would affect the other Dialects as well:

Alternatively, add a NOCOUNT OFF to the statement beforehand. Though I don't know what affect this would have in subsequent statements being executed.

SET NOCOUNT OFF;
update {tableName}
set
    Data = @Data,
    PersistenceVersion = @PersistenceVersion,
    SagaTypeVersion = @SagaTypeVersion,
    Concurrency = @Concurrency + 1{correlationSet}
where
    Id = @Id and Concurrency = @Concurrency

Additional information

[wleader]

I've thought about this some more, and I think the check for the number of rows in SagaPersister_Update.cs can be removed entirely. (In fact so could the Concurrency column from the table) because all the get operations that read saga data, lock it. The MS SQL dialect uses 'With UPDLOCK', and the other three all use the 'For Update' syntax. Therefore any saga data row will be pessimistically locked and can not be updated concurrently, so the optimistic concurrency checks are totally pointless.

[RyanThomasForge]

Hello and thank you for this bug report. We haven't had a chance to reproduce this yet, but after reviewing the issue this does appear to be a potential bug. We're going to begin the triage process and will get back to you with the results.

[hazel-bohon]

Hi @wleader. Thanks for submitting this issue. I've validated the behavior and have added it to our bugs backlog.

There is no ETA for when we will get to this bug, as it may be prioritized behind other, more urgent bugs. We will update this issue when it has a team assigned and work begins.