AppendToStream can write a subset of events when errors occur.

[rcknight]

Describe the bug When calling AppendToStreamAsync, and something throws midway through writing the events, events to that point will be written to the DB. This appears to be because of the use of a finally block to commit the events here: https://github.com/EventStore/EventStore-Client-Dotnet/blob/master/src/EventStore.Client.Streams/EventStoreClient.Append.cs#L124-L127

This only happens when supplying credentials to the append call, which bypasses the batch appender and sends it down this broken codepath (not sure what other circumstances that batch appender is "unusable": https://github.com/EventStore/EventStore-Client-Dotnet/blob/master/src/EventStore.Client.Streams/EventStoreClient.Append.cs#L79

To Reproduce Steps to reproduce the behavior:

Simple reproduction with a purposefully dodgy IEnumerable:

// Produces 5 events, then throws
IEnumerable<EventData> GetBrokenEventEnumerable()
{
    for(var i = 0; i < 5; i++)
    {
        yield return new EventData(Uuid.NewUuid(), "test-event", Encoding.UTF8.GetBytes(i.ToString()));
    }

    throw new Exception("IEnumerable broke :)");
}

try
{
    await client.AppendToStreamAsync("test-stream", StreamState.NoStream, GetBrokenEventEnumerable(), userCredentials: new UserCredentials("foo", "bar"));
}
catch (Exception ex)
{
    // we expect to see our exception
    Console.WriteLine(ex.Message);
}

var streamResult = client.ReadStreamAsync(Direction.Forwards,"test-stream", StreamPosition.Start);

// Stream does exist at this point
Console.WriteLine(await streamResult.ReadState == ReadState.Ok
    ? $"test-stream Contained {await streamResult.CountAsync()} events :("
    : "test-stream not found :)");

Expected behavior No events are written if an exception occurs halfway through processing the IEnumerable

Actual behavior Events processed up to that point are written to the database.

Config/Logs/Screenshots If applicable, please attach your node configuration, logs or any screenshots.

EventStore details

• EventStore server version: 22.10
• Operating system: Official Docker Image
• EventStore client version (if applicable): GRPC 23.0.0

_DEV-125

[alexeyzimarev]

I think we should enumerate right away, so any error in the enumerator will fail before even going further.

[rcknight]

I think we should enumerate right away, so any error in the enumerator will fail before even going further.

Not sure that would entirely solve the issue ... a broken enumerable was a convenient way to make it fail from outside, but if for example the write call can throw you'd have the same issue https://github.com/EventStore/EventStore-Client-Dotnet/blob/master/src/EventStore.Client.Streams/EventStoreClient.Append.cs#LL112C33-L112C33

[alexeyzimarev]

True, I didn't look into that. Good point 👍