Improve exception information when outbox primary key violation occurs

[ramonsmits]

Describe the suggested improvement

When the outbox INSERT operation fails the following exception is thrown for SqlServer:

Microsoft.Data.SqlClient.SqlException (0x80131904): Violation of PRIMARY KEY constraint 'PK__OutboxDa__C87C0C9D6A2BFF12'. Cannot insert duplicate key in object 'DataTransfer_P.OutboxData'. The duplicate key value is (da427334-d683-45bc-96b3-b18300945103).

Exception with full stack trace

System.Exception: Failed to ExecuteNonQuery. CommandText:

insert into [DataTransfer_P].[OutboxData]
(
    MessageId,
    Operations,
    PersistenceVersion
)
values
(
    @MessageId,
    @Operations,
    @PersistenceVersion
)
---> Microsoft.Data.SqlClient.SqlException (0x80131904): Violation of PRIMARY KEY constraint 'PK__OutboxDa__C87C0C9D6A2BFF12'. Cannot insert duplicate key in object 'DataTransfer_P.OutboxData'. The duplicate key value is (da427334-d683-45bc-96b3-b18300945103).

The statement has been terminated.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)
   at Microsoft.Data.SqlClient.SqlCommand.CompleteAsyncExecuteReader(Boolean isInternal, Boolean forDescribeParameterEncryption)
   at Microsoft.Data.SqlClient.SqlCommand.InternalEndExecuteNonQuery(IAsyncResult asyncResult, Boolean isInternal, String endMethod)
   at Microsoft.Data.SqlClient.SqlCommand.EndExecuteNonQueryInternal(IAsyncResult asyncResult)
   at Microsoft.Data.SqlClient.SqlCommand.EndExecuteNonQueryAsync(IAsyncResult asyncResult)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)
--- End of stack trace from previous location ---
   at Extensions.ExecuteNonQueryEx(DbCommand command, CancellationToken cancellationToken) in /_/src/SqlPersistence/Extensions.cs:line 113
ClientConnectionId:0cc0537e-a381-4dc6-b0bd-3c5e4ad6c3b4
Error Number:2627,State:1,Class:14
ClientConnectionId before routing:74b87e11-aa6f-4e26-bcc7-45bc2935bb78
Routing Destination:DB24C4.tr10688.uksouth1-a.worker.database.windows.net,11000
   --- End of inner exception stack trace ---
   at Extensions.ExecuteNonQueryEx(DbCommand command, CancellationToken cancellationToken) in /_/src/SqlPersistence/Extensions.cs:line 118
   at OptimisticConcurrencyControlStrategy.Complete(OutboxMessage outboxMessage, DbConnection connection, DbTransaction transaction, ContextBag context, CancellationToken cancellationToken) in /_/src/SqlPersistence/Outbox/OptimisticConcurrencyControlStrategy.cs:line 33
   at NServiceBus.TransportReceiveToPhysicalMessageConnector.Invoke(ITransportReceiveContext context, Func`2 next) in /_/src/NServiceBus.Core/Pipeline/Incoming/TransportReceiveToPhysicalMessageConnector.cs:line 38
   at NServiceBus.RetryAcknowledgementBehavior.Invoke(ITransportReceiveContext context, Func`2 next) in /_/src/NServiceBus.Core/ServicePlatform/Retries/RetryAcknowledgementBehavior.cs:line 25
   at NServiceBus.MainPipelineExecutor.Invoke(MessageContext messageContext, CancellationToken cancellationToken) in /_/src/NServiceBus.Core/Pipeline/MainPipelineExecutor.cs:line 45
   at NServiceBus.MainPipelineExecutor.Invoke(MessageContext messageContext, CancellationToken cancellationToken) in /_/src/NServiceBus.Core/Pipeline/MainPipelineExecutor.cs:line 64
   at NServiceBus.Transport.AzureServiceBus.MessagePump.ProcessMessage(ServiceBusReceivedMessage message, ProcessMessageEventArgs processMessageEventArgs, String messageId, Dictionary`2 headers, BinaryData body, CancellationToken messageProcessingCancellationToken) in /_/src/Transport/Receiving/MessagePump.cs:line 295

This is confusing to users as they see an error and don't understand why this happens.

We should catch Unique Key Violation SqlExceptions and wrap that in a new Exception that informs users

catch (SqlException ex) when (ex.Number=2627) //Unique Key Violation
{
	// Does need to check its the primary key contraint of the OUTBOX table and not maybe another
	// table via the storage context sharing

	throw new Exception("Message already processed successfully as outbox record already exists. If this happens very often consider enabling pessimistic locking. See https://docs.particular.net/search?q=sql+persistence+pessimistic+concurrency+control", ex);
}

Is your improvement related to a problem? Please describe.

Describe the suggested solution

Describe alternatives you've considered

Additional Context

No response

[ramonsmits]

This is not an easy change as the dialect abstraction is only about creating SqlCommands and unfortunately not about executing the query.

NServiceBus.Persistence.Sql/src/SqlPersistence/Outbox/OptimisticConcurrencyControlStrategy.cs

Lines 21 to 35 in 623e6b5

	public override async Task Complete(OutboxMessage outboxMessage, DbConnection connection, DbTransaction transaction, ContextBag context, CancellationToken cancellationToken = default)
	{
	string json = Serializer.Serialize(outboxMessage.TransportOperations.ToSerializable());
	json = sqlDialect.AddOutboxPadding(json);
	using (var command = sqlDialect.CreateCommand(connection))
	{
	command.CommandText = outboxCommands.OptimisticStore;
	command.Transaction = transaction;
	command.AddParameter("MessageId", outboxMessage.MessageId);
	command.AddJsonParameter("Operations", json);
	command.AddParameter("PersistenceVersion", StaticVersions.PersistenceVersion);
	await command.ExecuteNonQueryEx(cancellationToken).ConfigureAwait(false);
	}
	}

This means that at that code point we need to add a try..catch for each dialect including the logic to understand on what table the primary key violation occurs.

That is not ideal and needs a refactoring of the dialect API so that result/exception handling is part of the dialect. It can thrown exception that either is specific for the dialect or shared between dialects.