Multiple `FetchForWriting` calls can fail to detect concurrency changes

[elexisvenator]

FetchForWriting creates an optimistic lock by setting startingVersion property on the stream action. However there is an edge case where if the stream action already exists and already has a startinVersion set, calling FetchForWriting will overwrite the value. This leads to the possibility of the following scenario, where two FetchForWriting calls made against the same stream will not detect a new event written between the two calls.

StoreOptions(opts =>
{
    opts.Projections.LiveStreamAggregation<SomeProjection>();
    opts.Projections.LiveStreamAggregation<SomeOtherProjection>();
});

var streamId = CombGuidIdGeneration.NewGuid();
theSession.Events.StartStream(streamId, new EventA(), new EventA(), new EventA());
await theSession.SaveChangesAsync();
// stream version is now 3

var otherSession = theStore.LightweightSession();
var firstProjection = await otherSession.Events.FetchForWriting<SomeProjection>(streamId);

firstProjection.StartingVersion.ShouldBe(3);
firstProjection.Aggregate.ShouldNotBeNull();
firstProjection.Aggregate.Version.ShouldBe(3);

// in another session, append more events

theSession.Events.Append(streamId, new EventA(), new EventA());
await theSession.SaveChangesAsync();
// stream version is now 5

var secondProjection = await otherSession.Events.FetchForWriting<SomeOtherProjection>(streamId);
secondProjection.Aggregate.ShouldNotBeNull();
secondProjection.Aggregate.Version.ShouldBe(5);

// attempt to append
otherSession.Events.Append(streamId, new EventA());

// should fail with concurrency error
// because current version of the stream (5) is ahead of the first optimistic lock (3)
await otherSession.SaveChangesAsync();

I think the solution to this is likely to only set the StartingVersion if it doesnt exist. This does lead to the problem where marten knows that something is going to fail before the save changes but says nothing, but I don't think this can be avoided as marten does not know if there is even going to be a save changes call at all.

[jeremydmiller]

@elexisvenator Why are you reusing the same session in a long lived way like that? I do understand what it's doing wrong because the sessions cache the stream version, but first help me understand why you keep a session around that long?

[elexisvenator]

We definitely don't have long lived sessions, though there are a few places where we fetch multiple aggregates of the same stream using FetchForWriting. This issue was picked up as part of some artificial load testing and I doubt that it has happened in real usage.